Hi Ilias,
On Mon, 12 Jul 2021 at 00:24, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Sat, Jul 10, 2021 at 06:00:57PM -0600, Simon Glass wrote:
Hi Ilias,
On Wed, 7 Jul 2021 at 10:26, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
There's a lot of code duplication in U-Boot right now. All the TPM TIS
You mean in the TPM code I think.
Yes. Basically al TPM drivers duplicate this.
compatible drivers we have at the moment have their own copy of a TIS implementation.
So let's create a common layer which implements the core TIS functions. Any driver added from now own, which is compatible with the TIS spec, will only have to provide the underlying bus communication mechanisms.
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org
Changes since v1:
drivers/tpm/tpm2_tis_core.c | 545 ++++++++++++++++++++++++++++++++++++ drivers/tpm/tpm_tis.h | 40 +++ include/tpm-v2.h | 1 + 3 files changed, 586 insertions(+) create mode 100644 drivers/tpm/tpm2_tis_core.c
[..]
diff --git a/drivers/tpm/tpm_tis.h b/drivers/tpm/tpm_tis.h index 2a160fe05c9a..fde3bb71f7c2 100644 --- a/drivers/tpm/tpm_tis.h +++ b/drivers/tpm/tpm_tis.h @@ -21,6 +21,37 @@ #include <linux/compiler.h> #include <linux/types.h>
+struct tpm_tis_phy_ops {
int (*read_bytes)(struct udevice *udev, u32 addr, u16 len,u8 *result);int (*write_bytes)(struct udevice *udev, u32 addr, u16 len,const u8 *value);int (*read16)(struct udevice *udev, u32 addr, u16 *result);int (*read32)(struct udevice *udev, u32 addr, u32 *result);int (*write32)(struct udevice *udev, u32 addr, u32 src);A few points:
- these need comments
- can we use uint instead of u32 for the value args? We should use
native types where we can
Yes probably, I'll have a look `
- it seems like this should be a driver interface - see for example
how cros_ec.c works. It has a shared code library and the drivers each implement an interface similar to the above, but on different buses. In general function pointers are a sign we should be using a driver
I am not sure I am following, but I'll have a look on the code you pointed out.
+};
+enum tis_int_flags {
TPM_GLOBAL_INT_ENABLE = 0x80000000,TPM_INTF_BURST_COUNT_STATIC = 0x100,TPM_INTF_CMD_READY_INT = 0x080,TPM_INTF_INT_EDGE_FALLING = 0x040,TPM_INTF_INT_EDGE_RISING = 0x020,TPM_INTF_INT_LEVEL_LOW = 0x010,TPM_INTF_INT_LEVEL_HIGH = 0x008,TPM_INTF_LOCALITY_CHANGE_INT = 0x004,TPM_INTF_STS_VALID_INT = 0x002,TPM_INTF_DATA_AVAIL_INT = 0x001,+};
+#define TPM_ACCESS(l) (0x0000 | ((l) << 12)) +#define TPM_INT_ENABLE(l) (0x0008 | ((l) << 12)) +#define TPM_STS(l) (0x0018 | ((l) << 12)) +#define TPM_DATA_FIFO(l) (0x0024 | ((l) << 12)) +#define TPM_DID_VID(l) (0x0F00 | ((l) << 12)) +#define TPM_RID(l) (0x0F04 | ((l) << 12)) +#define TPM_INTF_CAPS(l) (0x0014 | ((l) << 12))
enum tpm_timeout { TPM_TIMEOUT_MS = 5, TIS_SHORT_TIMEOUT_MS = 750, @@ -43,6 +74,7 @@ struct tpm_chip { u8 rid; unsigned long timeout_a, timeout_b, timeout_c, timeout_d; /* msec */ ulong chip_type;
struct tpm_tis_phy_ops *phy_ops;};
struct tpm_input_header { @@ -130,4 +162,12 @@ enum tis_status { }; #endif
+int tpm_tis_open(struct udevice *udev); +int tpm_tis_close(struct udevice *udev); +int tpm_tis_cleanup(struct udevice *udev); +int tpm_tis_send(struct udevice *udev, const u8 *buf, size_t len); +int tpm_tis_recv(struct udevice *udev, u8 *buf, size_t count); +int tpm_tis_get_desc(struct udevice *udev, char *buf, int size); +int tpm_tis_init(struct udevice *udev); +void tpm_tis_ops_register(struct udevice *udev, struct tpm_tis_phy_ops *ops);
comments on all of these
#endif diff --git a/include/tpm-v2.h b/include/tpm-v2.h index 247b38696766..3e48e358613f 100644 --- a/include/tpm-v2.h +++ b/include/tpm-v2.h @@ -378,6 +378,7 @@ enum { TPM_STS_DATA_EXPECT = 1 << 3, TPM_STS_SELF_TEST_DONE = 1 << 2, TPM_STS_RESPONSE_RETRY = 1 << 1,
TPM_STS_READ_ZERO = 0x23Does this below in another patch?
It's a general tpm2 update. I can move it to the driver patch if it makes more sense.
};
enum {
2.32.0.rc0
I feel that this API could be useful in reducing code duplication, but in fact it has just created more, so far as I can see from this series :-) So I think you should convert at least one driver to show its value (and not make things any worse).
The mmio tpm driver uses it and instead of ~700 lines (like the tpmv2 spi driver) it drops down to ~100. I don't have access to any other TPM hardware to rewrite any of those.
Yes, but I hope you see my point, that you have added a new interface. It is definitely better than adding a new driver and duplicating all the code, but it is still one more copy and in fact, the code is duplicated.
Can you get access to TPM hardware? I see that you have offered to be the maintainer for this subsystem, so I think that would be useful. Can sandbox use your new API?
Regards, Simon