[U-Boot] [PATCH 1/2] spi: ST33ZP24 SPI TPM driver

[Simon Glass]

Hi Jean-Luc,

On 8 July 2014 16:05, Jean-Luc BLANC <stmicroelectronics.tpm at gmail.com> wrote:
> This driver add support for STMicroelectronics ST33ZP24 SPI TPM.

s/add/adds/

> Driver support 2 SPI TPMs.
> Driver support also hash in Locality 4 feature (the only way to

On both lines s/support/supports/

> update PCR17).
> ---
>  README                         |   29 ++
>  common/cmd_tpm.c               |   63 +++-
>  drivers/tpm/Makefile           |    1 +
>  drivers/tpm/tpm_spi_stm_st33.c |  724 ++++++++++++++++++++++++++++++++++++++++
>  include/tis.h                  |   11 +-
>  include/tpm.h                  |   22 ++
>  lib/tpm.c                      |   26 ++
>  7 files changed, 874 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/tpm/tpm_spi_stm_st33.c
>
> diff --git a/README b/README
> index a248ab5..a4aa28a 100644
> --- a/README
> +++ b/README
> @@ -1397,6 +1397,35 @@ The following options need to be configured:
>                 Define this to enable authorized functions in the TPM library.
>                 Requires CONFIG_TPM and CONFIG_SHA1.
>
> +               CONFIG_TPM_ST
> +               Support additional hash in locality 4 command for
> +               STMicroelectronics TPMs (SPI or I2C). Require CONFIG_CMD_TPM.
> +
> +               CONFIG_TPM_ST_SPI
> +               Support SPI STMicroelectronics TPM. Require SPI support

s/Require/Requires/

> +
> +                       TPM0_SPI_MAX_SPEED
> +                       Define SPI frequency for TPM, 10000000 Hz max
> +
> +                       TPM0_SPI_BUS_NUM
> +                       Define SPI Bus ID connected to TPM
> +
> +                       TPM0_SPI_CS
> +                       Define SPI Chip Select ID connected to TPM
> +
> +               CONFIG_TPM_ST_2TPM
> +               Support additional STMicoelectronics SPI TPM.
> +               Require CONFIG_TPM_ST_SPI
> +
> +                       TPM1_SPI_MAX_SPEED
> +                       Define SPI frequency for TPM, 10000000 Hz max
> +
> +                       TPM1_SPI_BUS_NUM
> +                       Define SPI Bus ID connected to TPM
> +
> +                       TPM1_SPI_CS
> +                       Define SPI Chip Select ID connected to TPM

Not essential, but it would be nice to support CONFIG_OF_CONTROL and
get these parameters from the device tree, as other TPM drivers do.

> +
>  - USB Support:
>                 At the moment only the UHCI host controller is
>                 supported (PIP405, MIP405, MPC5200); define
> diff --git a/common/cmd_tpm.c b/common/cmd_tpm.c
> index 0294952..63f52e4 100644
> --- a/common/cmd_tpm.c
> +++ b/common/cmd_tpm.c
> @@ -334,6 +334,29 @@ static int do_tpm_extend(cmd_tbl_t *cmdtp, int flag,
>         return convert_return_code(rc);
>  }
>
> +#ifdef CONFIG_TPM_ST

Is this a feature only supported by ST, in which case this code is
correct, or is it a standard feature that other chips may support, in
which case you should add a new CONFIG like CONFIG_TPM_HASH_LOC4?

> +static int do_tpm_hash_loc4(cmd_tbl_t *cmdtp, int flag,
> +               int argc, char * const argv[])
> +{
> +       uint32_t rc;
> +       size_t count;
> +       void *data;
> +
> +       if (argc != 2)
> +               return CMD_RET_USAGE;
> +
> +       data = parse_byte_string(argv[1], NULL, &count);
> +       if (!data) {
> +               printf("Couldn't parse byte string %s\n", argv[1]);

Can we move this error into the parse_byte_string() to avoid repeating it?

> +               return CMD_RET_FAILURE;
> +       }
> +
> +       rc = tpm_hash_loc4(data, count);
> +       free(data);
> +       return convert_return_code(rc);
> +}
> +#endif /* CONFIG_TPM_ST */
> +
>  static int do_tpm_pcr_read(cmd_tbl_t *cmdtp, int flag,
>                 int argc, char * const argv[])
>  {
> @@ -355,6 +378,25 @@ static int do_tpm_pcr_read(cmd_tbl_t *cmdtp, int flag,
>         return convert_return_code(rc);
>  }
>
> +#ifdef CONFIG_TPM_ST_2TPM
> +static int do_tpm_spi_select(cmd_tbl_t *cmdtp, int flag,
> +                            int argc, char * const argv[])
> +{
> +       uint32_t rc, spi_number;
> +
> +       if (argc != 2)
> +               return CMD_RET_USAGE;
> +       spi_number = simple_strtoul(argv[1], NULL, 0);
> +       if (spi_number < CONFIG_TPM_ST_2TPM) {
> +               rc = tpm_spi_select(spi_number);
> +       } else {
> +               printf("Couldn't parse argument %s\n", argv[1]);
> +               return CMD_RET_FAILURE;
> +       }
> +       return convert_return_code(rc);
> +}
> +#endif /* CONFIG_TPM_ST_2TPM */

This seems quite specific to SPI. Why not have a 'tpm number' and have
it wholly kept in this file? It looks like the tpm.h file should be
changed to add a TPM number to each call? If so, that should be a
separate patch. This is what the 'i2c dev' command does. See how SPI
FLASH works for another example (sf probe command).

> +
>  static int do_tpm_tsc_physical_presence(cmd_tbl_t *cmdtp, int flag,
>                 int argc, char * const argv[])
>  {
> @@ -629,8 +671,16 @@ static cmd_tbl_t tpm_commands[] = {
>                         do_tpm_nv_write_value, "", ""),
>         U_BOOT_CMD_MKENT(extend, 0, 1,
>                         do_tpm_extend, "", ""),
> +#ifdef CONFIG_TPM_ST
> +       U_BOOT_CMD_MKENT(hash_loc4, 0, 1,
> +                        do_tpm_hash_loc4, "", ""),
> +#endif /* CONFIG_TPM_ST */
>         U_BOOT_CMD_MKENT(pcr_read, 0, 1,
> -                       do_tpm_pcr_read, "", ""),
> +                        do_tpm_pcr_read, "", ""),
> +#ifdef CONFIG_TPM_ST_2TPM
> +       U_BOOT_CMD_MKENT(spi_select, 0, 1,
> +                        do_tpm_spi_select, "", ""),
> +#endif /* CONFIG_TPM_ST_2TPM */
>         U_BOOT_CMD_MKENT(tsc_physical_presence, 0, 1,
>                         do_tpm_tsc_physical_presence, "", ""),
>         U_BOOT_CMD_MKENT(read_pubek, 0, 1,

Regards,
Simon