On Fri, Mar 18, 2022 at 04:16:47PM +0100, Johannes Holland wrote:
> Only tpm_tis and tpm_tis_synquacer have a dedicated way to access
> multiple bytes at once, every other driver will just fall back to
> read_bytes/write_bytes. Therefore, remove the read16/read32/write32
> calls and move their logic to read_bytes/write_bytes.
>
> * v2:
> * rebase and apply changes to tpm_tis_synquacer, as well
> * move variable declarations to beginning of functions
> * v3:
> * remove read16/read32/write32 api calls altogether and always call
> read_bytes/write_bytes
>
> Signed-off-by: Johannes Holland <johannes.holland@xxxxxxxxxxxx>
> ---
Please put the change log exactly here, so that it won't be included to the
commit log.
If you don't mind, there should be also
Suggested-by: Jarkko Sakkinen <jarkko@xxxxxxxxxx>
> drivers/char/tpm/tpm_tis.c | 67 ++++++++---------
> drivers/char/tpm/tpm_tis_core.h | 53 ++++++++++---
> drivers/char/tpm/tpm_tis_spi.h | 4 -
> drivers/char/tpm/tpm_tis_spi_cr50.c | 7 +-
> drivers/char/tpm/tpm_tis_spi_main.c | 45 +----------
> drivers/char/tpm/tpm_tis_synquacer.c | 108 ++++++++++++---------------
> 6 files changed, 123 insertions(+), 161 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index d3f2e5364c27..bcff6429e0b4 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -153,50 +153,46 @@ static int check_acpi_tpm2(struct device *dev)
> #endif
>
> static int tpm_tcg_read_bytes(struct tpm_tis_data *data, u32 addr, u16 len,
> - u8 *result)
> + u8 *result, enum tpm_tis_io_mode io_mode)
> {
> struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
> -
> - while (len--)
> - *result++ = ioread8(phy->iobase + addr);
> + __le16 result_le16;
> + __le32 result_le32;
> +
> + switch (io_mode) {
> + case TPM_TIS_PHYS_8:
> + while (len--)
> + *result++ = ioread8(phy->iobase + addr);
> + break;
> + case TPM_TIS_PHYS_16:
> + result_le16 = cpu_to_le16(ioread16(phy->iobase + addr));
> + memcpy(result, &result_le16, sizeof(u16));
> + break;
> + case TPM_TIS_PHYS_32:
> + result_le32 = cpu_to_le32(ioread32(phy->iobase + addr));
> + memcpy(result, &result_le32, sizeof(u32));
> + break;
> + }
>
> return 0;
> }
>
> static int tpm_tcg_write_bytes(struct tpm_tis_data *data, u32 addr, u16 len,
> - const u8 *value)
> + const u8 *value, enum tpm_tis_io_mode io_mode)
> {
> struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
>
> - while (len--)
> - iowrite8(*value++, phy->iobase + addr);
> -
> - return 0;
> -}
> -
> -static int tpm_tcg_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
> -{
> - struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
> -
> - *result = ioread16(phy->iobase + addr);
> -
> - return 0;
> -}
> -
> -static int tpm_tcg_read32(struct tpm_tis_data *data, u32 addr, u32 *result)
> -{
> - struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
> -
> - *result = ioread32(phy->iobase + addr);
> -
> - return 0;
> -}
> -
> -static int tpm_tcg_write32(struct tpm_tis_data *data, u32 addr, u32 value)
> -{
> - struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
> -
> - iowrite32(value, phy->iobase + addr);
> + switch (io_mode) {
> + case TPM_TIS_PHYS_8:
> + while (len--)
> + iowrite8(*value++, phy->iobase + addr);
> + break;
> + case TPM_TIS_PHYS_16:
> + return -EINVAL;
> + case TPM_TIS_PHYS_32:
> + iowrite32(le32_to_cpu(*((__le32 *)value)), phy->iobase + addr);
> + break;
> + }
>
> return 0;
> }
> @@ -204,9 +200,6 @@ static int tpm_tcg_write32(struct tpm_tis_data *data, u32 addr, u32 value)
> static const struct tpm_tis_phy_ops tpm_tcg = {
> .read_bytes = tpm_tcg_read_bytes,
> .write_bytes = tpm_tcg_write_bytes,
> - .read16 = tpm_tcg_read16,
> - .read32 = tpm_tcg_read32,
> - .write32 = tpm_tcg_write32,
> };
>
> static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info)
> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
> index 3be24f221e32..e2b8e6de25b4 100644
> --- a/drivers/char/tpm/tpm_tis_core.h
> +++ b/drivers/char/tpm/tpm_tis_core.h
> @@ -104,54 +104,83 @@ struct tpm_tis_data {
> unsigned int timeout_max; /* usecs */
> };
>
Please add a comment here to explain this enum:
/*
* <description>
*/
> +enum tpm_tis_io_mode {
> + TPM_TIS_PHYS_8,
> + TPM_TIS_PHYS_16,
> + TPM_TIS_PHYS_32,
> +};
> +
> struct tpm_tis_phy_ops {
> + /* data is passed in little endian */
> int (*read_bytes)(struct tpm_tis_data *data, u32 addr, u16 len,
> - u8 *result);
> + u8 *result, enum tpm_tis_io_mode mode);
> int (*write_bytes)(struct tpm_tis_data *data, u32 addr, u16 len,
> - const u8 *value);
> - int (*read16)(struct tpm_tis_data *data, u32 addr, u16 *result);
> - int (*read32)(struct tpm_tis_data *data, u32 addr, u32 *result);
> - int (*write32)(struct tpm_tis_data *data, u32 addr, u32 src);
> + const u8 *value, enum tpm_tis_io_mode mode);
> };
>
> static inline int tpm_tis_read_bytes(struct tpm_tis_data *data, u32 addr,
> u16 len, u8 *result)
> {
> - return data->phy_ops->read_bytes(data, addr, len, result);
> + return data->phy_ops->read_bytes(data, addr, len, result,
> + TPM_TIS_PHYS_8);
> }
>
> static inline int tpm_tis_read8(struct tpm_tis_data *data, u32 addr, u8 *result)
> {
> - return data->phy_ops->read_bytes(data, addr, 1, result);
> + return data->phy_ops->read_bytes(data, addr, 1, result, TPM_TIS_PHYS_8);
> }
>
> static inline int tpm_tis_read16(struct tpm_tis_data *data, u32 addr,
> u16 *result)
> {
> - return data->phy_ops->read16(data, addr, result);
> + __le16 result_le;
> + int rc;
> +
> + rc = data->phy_ops->read_bytes(data, addr, sizeof(u16),
> + (u8 *)&result_le, TPM_TIS_PHYS_16);
> + if (!rc)
> + *result = le16_to_cpu(result_le);
> +
> + return rc;
> }
>
> static inline int tpm_tis_read32(struct tpm_tis_data *data, u32 addr,
> u32 *result)
> {
> - return data->phy_ops->read32(data, addr, result);
> + __le32 result_le;
> + int rc;
> +
> + rc = data->phy_ops->read_bytes(data, addr, sizeof(u32),
> + (u8 *)&result_le, TPM_TIS_PHYS_32);
> + if (!rc)
> + *result = le32_to_cpu(result_le);
> +
> + return rc;
> }
>
> static inline int tpm_tis_write_bytes(struct tpm_tis_data *data, u32 addr,
> u16 len, const u8 *value)
> {
> - return data->phy_ops->write_bytes(data, addr, len, value);
> + return data->phy_ops->write_bytes(data, addr, len, value,
> + TPM_TIS_PHYS_8);
> }
>
> static inline int tpm_tis_write8(struct tpm_tis_data *data, u32 addr, u8 value)
> {
> - return data->phy_ops->write_bytes(data, addr, 1, &value);
> + return data->phy_ops->write_bytes(data, addr, 1, &value,
> + TPM_TIS_PHYS_8);
> }
>
> static inline int tpm_tis_write32(struct tpm_tis_data *data, u32 addr,
> u32 value)
> {
> - return data->phy_ops->write32(data, addr, value);
> + __le32 value_le;
> + int rc;
> +
> + value_le = cpu_to_le32(value);
> + rc = data->phy_ops->write_bytes(data, addr, sizeof(u32),
> + (u8 *)&value_le, TPM_TIS_PHYS_32);
> + return rc;
> }
>
> static inline bool is_bsw(void)
> diff --git a/drivers/char/tpm/tpm_tis_spi.h b/drivers/char/tpm/tpm_tis_spi.h
> index bba73979c368..d0f66f6f1931 100644
> --- a/drivers/char/tpm/tpm_tis_spi.h
> +++ b/drivers/char/tpm/tpm_tis_spi.h
> @@ -31,10 +31,6 @@ extern int tpm_tis_spi_init(struct spi_device *spi, struct tpm_tis_spi_phy *phy,
> extern int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
> u8 *in, const u8 *out);
>
> -extern int tpm_tis_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result);
> -extern int tpm_tis_spi_read32(struct tpm_tis_data *data, u32 addr, u32 *result);
> -extern int tpm_tis_spi_write32(struct tpm_tis_data *data, u32 addr, u32 value);
> -
> #ifdef CONFIG_TCG_TIS_SPI_CR50
> extern int cr50_spi_probe(struct spi_device *spi);
> #else
> diff --git a/drivers/char/tpm/tpm_tis_spi_cr50.c b/drivers/char/tpm/tpm_tis_spi_cr50.c
> index 7bf123d3c537..f4937280e940 100644
> --- a/drivers/char/tpm/tpm_tis_spi_cr50.c
> +++ b/drivers/char/tpm/tpm_tis_spi_cr50.c
> @@ -222,13 +222,13 @@ static int tpm_tis_spi_cr50_transfer(struct tpm_tis_data *data, u32 addr, u16 le
> }
>
> static int tpm_tis_spi_cr50_read_bytes(struct tpm_tis_data *data, u32 addr,
> - u16 len, u8 *result)
> + u16 len, u8 *result, enum tpm_tis_io_mode io_mode)
> {
> return tpm_tis_spi_cr50_transfer(data, addr, len, result, NULL);
> }
>
> static int tpm_tis_spi_cr50_write_bytes(struct tpm_tis_data *data, u32 addr,
> - u16 len, const u8 *value)
> + u16 len, const u8 *value, enum tpm_tis_io_mode io_mode)
> {
> return tpm_tis_spi_cr50_transfer(data, addr, len, NULL, value);
> }
> @@ -236,9 +236,6 @@ static int tpm_tis_spi_cr50_write_bytes(struct tpm_tis_data *data, u32 addr,
> static const struct tpm_tis_phy_ops tpm_spi_cr50_phy_ops = {
> .read_bytes = tpm_tis_spi_cr50_read_bytes,
> .write_bytes = tpm_tis_spi_cr50_write_bytes,
> - .read16 = tpm_tis_spi_read16,
> - .read32 = tpm_tis_spi_read32,
> - .write32 = tpm_tis_spi_write32,
> };
>
> static void cr50_print_fw_version(struct tpm_tis_data *data)
> diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c
> index aaa59a00eeae..d0920c3c400f 100644
> --- a/drivers/char/tpm/tpm_tis_spi_main.c
> +++ b/drivers/char/tpm/tpm_tis_spi_main.c
> @@ -141,55 +141,17 @@ int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
> }
>
> static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
> - u16 len, u8 *result)
> + u16 len, u8 *result, enum tpm_tis_io_mode io_mode)
> {
> return tpm_tis_spi_transfer(data, addr, len, result, NULL);
> }
>
> static int tpm_tis_spi_write_bytes(struct tpm_tis_data *data, u32 addr,
> - u16 len, const u8 *value)
> + u16 len, const u8 *value, enum tpm_tis_io_mode io_mode)
> {
> return tpm_tis_spi_transfer(data, addr, len, NULL, value);
> }
>
> -int tpm_tis_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
> -{
> - __le16 result_le;
> - int rc;
> -
> - rc = data->phy_ops->read_bytes(data, addr, sizeof(u16),
> - (u8 *)&result_le);
> - if (!rc)
> - *result = le16_to_cpu(result_le);
> -
> - return rc;
> -}
> -
> -int tpm_tis_spi_read32(struct tpm_tis_data *data, u32 addr, u32 *result)
> -{
> - __le32 result_le;
> - int rc;
> -
> - rc = data->phy_ops->read_bytes(data, addr, sizeof(u32),
> - (u8 *)&result_le);
> - if (!rc)
> - *result = le32_to_cpu(result_le);
> -
> - return rc;
> -}
> -
> -int tpm_tis_spi_write32(struct tpm_tis_data *data, u32 addr, u32 value)
> -{
> - __le32 value_le;
> - int rc;
> -
> - value_le = cpu_to_le32(value);
> - rc = data->phy_ops->write_bytes(data, addr, sizeof(u32),
> - (u8 *)&value_le);
> -
> - return rc;
> -}
> -
> int tpm_tis_spi_init(struct spi_device *spi, struct tpm_tis_spi_phy *phy,
> int irq, const struct tpm_tis_phy_ops *phy_ops)
> {
> @@ -205,9 +167,6 @@ int tpm_tis_spi_init(struct spi_device *spi, struct tpm_tis_spi_phy *phy,
> static const struct tpm_tis_phy_ops tpm_spi_phy_ops = {
> .read_bytes = tpm_tis_spi_read_bytes,
> .write_bytes = tpm_tis_spi_write_bytes,
> - .read16 = tpm_tis_spi_read16,
> - .read32 = tpm_tis_spi_read32,
> - .write32 = tpm_tis_spi_write32,
> };
>
> static int tpm_tis_spi_probe(struct spi_device *dev)
> diff --git a/drivers/char/tpm/tpm_tis_synquacer.c b/drivers/char/tpm/tpm_tis_synquacer.c
> index e47bdd272704..2751be8e6065 100644
> --- a/drivers/char/tpm/tpm_tis_synquacer.c
> +++ b/drivers/char/tpm/tpm_tis_synquacer.c
> @@ -35,72 +35,63 @@ static inline struct tpm_tis_synquacer_phy *to_tpm_tis_tcg_phy(struct tpm_tis_da
> }
>
> static int tpm_tis_synquacer_read_bytes(struct tpm_tis_data *data, u32 addr,
> - u16 len, u8 *result)
> + u16 len, u8 *result,
> + enum tpm_tis_io_mode io_mode)
> {
> struct tpm_tis_synquacer_phy *phy = to_tpm_tis_tcg_phy(data);
> -
> - while (len--)
> - *result++ = ioread8(phy->iobase + addr);
> + __le16 result_le16;
> + __le32 result_le32;
> + u16 result16;
> + u32 result32;
> +
> + switch (io_mode) {
> + case TPM_TIS_PHYS_8:
> + while (len--)
> + *result++ = ioread8(phy->iobase + addr);
> + break;
> + case TPM_TIS_PHYS_16:
> + result[1] = ioread8(phy->iobase + addr + 1);
> + result[0] = ioread8(phy->iobase + addr);
> + break;
> + case TPM_TIS_PHYS_32:
> + result[3] = ioread8(phy->iobase + addr + 3);
> + result[2] = ioread8(phy->iobase + addr + 2);
> + result[1] = ioread8(phy->iobase + addr + 1);
> + result[0] = ioread8(phy->iobase + addr);
> + break;
> + }
>
> return 0;
> }
>
> static int tpm_tis_synquacer_write_bytes(struct tpm_tis_data *data, u32 addr,
> - u16 len, const u8 *value)
> + u16 len, const u8 *value,
> + enum tpm_tis_io_mode io_mode)
> {
> struct tpm_tis_synquacer_phy *phy = to_tpm_tis_tcg_phy(data);
> -
> - while (len--)
> - iowrite8(*value++, phy->iobase + addr);
> -
> - return 0;
> -}
> -
> -static int tpm_tis_synquacer_read16_bw(struct tpm_tis_data *data,
> - u32 addr, u16 *result)
> -{
> - struct tpm_tis_synquacer_phy *phy = to_tpm_tis_tcg_phy(data);
> -
> - /*
> - * Due to the limitation of SPI controller on SynQuacer,
> - * 16/32 bits access must be done in byte-wise and descending order.
> - */
> - *result = (ioread8(phy->iobase + addr + 1) << 8) |
> - (ioread8(phy->iobase + addr));
> -
> - return 0;
> -}
> -
> -static int tpm_tis_synquacer_read32_bw(struct tpm_tis_data *data,
> - u32 addr, u32 *result)
> -{
> - struct tpm_tis_synquacer_phy *phy = to_tpm_tis_tcg_phy(data);
> -
> - /*
> - * Due to the limitation of SPI controller on SynQuacer,
> - * 16/32 bits access must be done in byte-wise and descending order.
> - */
> - *result = (ioread8(phy->iobase + addr + 3) << 24) |
> - (ioread8(phy->iobase + addr + 2) << 16) |
> - (ioread8(phy->iobase + addr + 1) << 8) |
> - (ioread8(phy->iobase + addr));
> -
> - return 0;
> -}
> -
> -static int tpm_tis_synquacer_write32_bw(struct tpm_tis_data *data,
> - u32 addr, u32 value)
> -{
> - struct tpm_tis_synquacer_phy *phy = to_tpm_tis_tcg_phy(data);
> -
> - /*
> - * Due to the limitation of SPI controller on SynQuacer,
> - * 16/32 bits access must be done in byte-wise and descending order.
> - */
> - iowrite8(value >> 24, phy->iobase + addr + 3);
> - iowrite8(value >> 16, phy->iobase + addr + 2);
> - iowrite8(value >> 8, phy->iobase + addr + 1);
> - iowrite8(value, phy->iobase + addr);
> + __le16 result_le16;
> + __le32 result_le32;
> + u16 result16;
> + u32 result32;
> +
> + switch (io_mode) {
> + case TPM_TIS_PHYS_8:
> + while (len--)
> + iowrite8(*value++, phy->iobase + addr);
> + break;
> + case TPM_TIS_PHYS_16:
> + return -EINVAL;
> + case TPM_TIS_PHYS_32:
> + /*
> + * Due to the limitation of SPI controller on SynQuacer,
> + * 16/32 bits access must be done in byte-wise and descending order.
> + */
> + iowrite8(&value[3], phy->iobase + addr + 3);
> + iowrite8(&value[2], phy->iobase + addr + 2);
> + iowrite8(&value[1], phy->iobase + addr + 1);
> + iowrite8(&value[0], phy->iobase + addr);
> + break;
> + }
>
> return 0;
> }
> @@ -108,9 +99,6 @@ static int tpm_tis_synquacer_write32_bw(struct tpm_tis_data *data,
> static const struct tpm_tis_phy_ops tpm_tcg_bw = {
> .read_bytes = tpm_tis_synquacer_read_bytes,
> .write_bytes = tpm_tis_synquacer_write_bytes,
> - .read16 = tpm_tis_synquacer_read16_bw,
> - .read32 = tpm_tis_synquacer_read32_bw,
> - .write32 = tpm_tis_synquacer_write32_bw,
> };
>
> static int tpm_tis_synquacer_init(struct device *dev,
> --
> 2.31.1.windows.1
>
Other than that I did not find anything else to complain about :-)
BR, Jarkko
