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

[Simon Glass]

Hi Ilias,

On Wed, 7 Jul 2021 at 10:26, Ilias Apalodimas
<ilias.apalodimas at 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.

> 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 at 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
- 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

> +};
> +
> +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?

>  };
>
>  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).

Regards,
Simon