Re: [PATCH 1/2 v2] tpm2: Introduce TIS tpm core

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

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               = 0x23
> 
> Does 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.  

> 
> Regards,
> Simon