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

Hi Jean-Luc,

On 8 July 2014 16:05, Jean-Luc BLANC stmicroelectronics.tpm@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