On Tue, 2021-11-02 at 17:20 +0200, amirmizi6@xxxxxxxxx wrote: > From: Amir Mizinski <amirmizi6@xxxxxxxxx> > > I2C support for TPM devices on the TIS interface is currently available > only as a proprietary driver implementation for each TPM vendor. > This patch implements the infrastructure for a TCG-compliant TPM TIS I2C > driver with the functionality required to communicate with a TPM device > over I2C according to the "TCG PC Client PTP Interface Specification" > and "Device Driver Design Principles for TPM 2.0" > > References: > [1] "TCG PC Client Platform TPM Profile(PTP) Interface Specification for > TPM 2.0" Version 01.03 Revision 22 at: > https://trustedcomputinggroup.org/resource/pc-client-platform-tpm-profile-ptp-specification/ > [2] "TCG PC Client Device Driver Design Principles for TPM 2.0" > Version 1.0 Revision 27 at: > https://trustedcomputinggroup.org/resource/tcg-pc-client-device-driver-design-principles-for-tpm-2-0/ > > Signed-off-by: Amir Mizinski <amirmizi6@xxxxxxxxx> > Tested-by: Eddie James <eajames@xxxxxxxxxxxxx> > Tested-by: Joel Stanley <joel@xxxxxxxxx> > --- > drivers/char/tpm/Kconfig | 12 ++ > drivers/char/tpm/Makefile | 1 + > drivers/char/tpm/tpm_tis_i2c.c | 268 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 281 insertions(+) > create mode 100644 drivers/char/tpm/tpm_tis_i2c.c > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig > index 4308f9c..4137ded 100644 > --- a/drivers/char/tpm/Kconfig > +++ b/drivers/char/tpm/Kconfig > @@ -86,6 +86,18 @@ config TCG_TIS_SYNQUACER > To compile this driver as a module, choose M here; > the module will be called tpm_tis_synquacer. > > +config TCG_TIS_I2C > + tristate "TPM I2C Interface Specification" > + depends on I2C > + select CRC_CCITT > + select TCG_TIS_CORE > + help > + If you have a TPM security chip, compliant with the TCG TPM PTP > + (I2C interface) specification and connected to an I2C bus master, > + say Yes and it will be accessible from within Linux. > + To compile this driver as a module, choose M here; > + the module will be called tpm_tis_i2c. > + > config TCG_TIS_I2C_CR50 > tristate "TPM Interface Specification 2.0 Interface (I2C - CR50)" > depends on I2C > diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile > index 66d39ea..0222b1d 100644 > --- a/drivers/char/tpm/Makefile > +++ b/drivers/char/tpm/Makefile > @@ -29,6 +29,7 @@ tpm_tis_spi-$(CONFIG_TCG_TIS_SPI_CR50) += tpm_tis_spi_cr50.o > > obj-$(CONFIG_TCG_TIS_I2C_CR50) += tpm_tis_i2c_cr50.o > > +obj-$(CONFIG_TCG_TIS_I2C) += tpm_tis_i2c.o > obj-$(CONFIG_TCG_TIS_I2C_ATMEL) += tpm_i2c_atmel.o > obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o > obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o > diff --git a/drivers/char/tpm/tpm_tis_i2c.c b/drivers/char/tpm/tpm_tis_i2c.c > new file mode 100644 > index 0000000..7557c77 > --- /dev/null > +++ b/drivers/char/tpm/tpm_tis_i2c.c > @@ -0,0 +1,268 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2014-2021 Nuvoton Technology corporation > + * Snip these two lines > + * TPM TIS I2C > + * away. > + * TPM TIS I2C Device Driver Interface for devices that implement the TPM > + * I2C Interface defined by "TCG PC Client Platform TPM Profile (PTP) > + * Specification version 01.05 r14" and "TCG PC Client Device Driver > + * Design Principles version 1.0 r27" for TPM 2.0 at > + * www.trustedcomputinggroup.org > + */ > + > +#include <linux/init.h> > +#include <linux/module.h> > +#include <linux/moduleparam.h> > +#include <linux/slab.h> > +#include <linux/interrupt.h> > +#include <linux/wait.h> > +#include <linux/acpi.h> > +#include <linux/freezer.h> > + > +#include <linux/module.h> > +#include <linux/i2c.h> > +#include <linux/gpio.h> > +#include <linux/of_irq.h> > +#include <linux/of_gpio.h> > +#include <linux/tpm.h> > +#include "tpm.h" > +#include "tpm_tis_core.h" tpm_tis_core.h already includes tpm.h. > + > +#define TPM_LOC_SEL 0x04 > +#define TPM_I2C_INTERFACE_CAPABILITY 0x30 > +#define TPM_I2C_DEVICE_ADDRESS 0x38 > +#define TPM_DATA_CSUM_ENABLE 0x40 > +#define TPM_I2C_DID_VID 0x48 > +#define TPM_I2C_RID 0x4C Is the alignment correct here? > + > +struct tpm_tis_i2c_phy { > + struct tpm_tis_data priv; > + struct i2c_client *i2c_client; > + u8 *iobuf; > +}; > + > +static inline struct tpm_tis_i2c_phy *to_tpm_tis_i2c_phy(struct tpm_tis_data > + *data) No need to split this into two lines. > +{ > + return container_of(data, struct tpm_tis_i2c_phy, priv); > +} > + > +static u8 address_to_register(u32 addr) > +{ > + addr &= 0xFFF; > + > + switch (addr) { > + // adapt register addresses that have changed compared to > + // older TIS versions > + case TPM_ACCESS(0): > + return 0x04; > + case TPM_LOC_SEL: > + return 0x00; > + case TPM_DID_VID(0): > + return 0x48; > + case TPM_RID(0): > + return 0x4C; > + default: > + return addr; > + } > +} > + > +static int tpm_tis_i2c_read_bytes(struct tpm_tis_data *data, u32 addr, > + u16 len, u8 *result) Ditto. > +{ > + struct tpm_tis_i2c_phy *phy = to_tpm_tis_i2c_phy(data); > + int ret = 0; Why do you initialize ret? I don't see any good reason to do it. > + int i = 0; > + u8 reg = address_to_register(addr); Put these into reverse christmas tree order, i.e. move reg as the second declaration. > + struct i2c_msg msgs[] = { > + { > + .addr = phy->i2c_client->addr, > + .len = sizeof(reg), > + .buf = ®, > + }, > + { > + .addr = phy->i2c_client->addr, > + .len = len, > + .buf = result, > + .flags = I2C_M_RD, > + }, > + }; > + > + do { > + ret = i2c_transfer(phy->i2c_client->adapter, msgs, > + ARRAY_SIZE(msgs)); > + usleep_range(250, 300); // wait default GUARD_TIME of 250µs > + > + } while (ret < 0 && i++ < TPM_RETRY); > + > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +static int tpm_tis_i2c_write_bytes(struct tpm_tis_data *data, u32 addr, > + u16 len, const u8 *value) > +{ > + struct tpm_tis_i2c_phy *phy = to_tpm_tis_i2c_phy(data); > + int ret = 0; > + int i = 0; > + > + if (phy->iobuf) { > + if (len > TPM_BUFSIZE - 1) > + return -EIO; > + > + phy->iobuf[0] = address_to_register(addr); > + memcpy(phy->iobuf + 1, value, len); > + > + { > + struct i2c_msg msgs[] = { > + { > + .addr = phy->i2c_client->addr, > + .len = len + 1, > + .buf = phy->iobuf, > + }, > + }; > + > + do { > + ret = i2c_transfer(phy->i2c_client->adapter, > + msgs, ARRAY_SIZE(msgs)); > + // wait default GUARD_TIME of 250µs > + usleep_range(250, 300); > + } while (ret < 0 && i++ < TPM_RETRY); > + } Please remove the unnecessary nesting. > + } else { > + u8 reg = address_to_register(addr); > + > + struct i2c_msg msgs[] = { > + { > + .addr = phy->i2c_client->addr, > + .len = sizeof(reg), > + .buf = ®, > + }, > + { > + .addr = phy->i2c_client->addr, > + .len = len, > + .buf = (u8 *)value, > + .flags = I2C_M_NOSTART, > + }, > + }; Empty line. > + do { > + ret = i2c_transfer(phy->i2c_client->adapter, msgs, > + ARRAY_SIZE(msgs)); > + // wait default GUARD_TIME of 250µs > + usleep_range(250, 300); > + } while (ret < 0 && i++ < TPM_RETRY); > + } > + > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +int tpm_tis_i2c_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_i2c_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_i2c_write32(struct tpm_tis_data *data, u32 addr, u32 value) > +{ > + __le32 value_le; > + int rc; > + > + value_le = cpu_to_le32(value); Empty line. > + rc = data->phy_ops->write_bytes(data, addr, sizeof(u32), > + (u8 *)&value_le); > + > + return rc; > +} Empty line. > +static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume); > + > +static const struct tpm_tis_phy_ops tpm_i2c_phy_ops = { > + .read_bytes = tpm_tis_i2c_read_bytes, > + .write_bytes = tpm_tis_i2c_write_bytes, > + .read16 = tpm_tis_i2c_read16, > + .read32 = tpm_tis_i2c_read32, > + .write32 = tpm_tis_i2c_write32, > +}; > + > +static int tpm_tis_i2c_probe(struct i2c_client *dev, > + const struct i2c_device_id *id) Again, no need to split into two lines. > +{ > + struct tpm_tis_i2c_phy *phy; > + int rc; > + const u8 loc_init = 0; Reverse christmas tree declaration order. > + > + phy = devm_kzalloc(&dev->dev, sizeof(struct tpm_tis_i2c_phy), > + GFP_KERNEL); > + if (!phy) > + return -ENOMEM; > + > + phy->i2c_client = dev; > + > + if (!i2c_check_functionality(dev->adapter, I2C_FUNC_NOSTART)) { > + phy->iobuf = devm_kmalloc(&dev->dev, TPM_BUFSIZE, GFP_KERNEL); > + if (!phy->iobuf) > + return -ENOMEM; > + } > + > + // select locality 0 (the driver will access only via locality 0) You should use "/* */". > + rc = tpm_tis_i2c_write_bytes(&phy->priv, TPM_LOC_SEL, 1, &loc_init); > + if (rc < 0) > + return rc; > + > + return tpm_tis_core_init(&dev->dev, &phy->priv, -1, &tpm_i2c_phy_ops, > + NULL); > +} > + > +static const struct i2c_device_id tpm_tis_i2c_id[] = { > + {"tpm_tis_i2c", 0}, > + {} > +}; > +MODULE_DEVICE_TABLE(i2c, tpm_tis_i2c_id); > + > +static const struct of_device_id of_tis_i2c_match[] = { > + { .compatible = "nuvoton,npct75x", }, > + { .compatible = "tcg,tpm-tis-i2c", }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, of_tis_i2c_match); > + > +static struct i2c_driver tpm_tis_i2c_driver = { > + .driver = { > + .owner = THIS_MODULE, > + .name = "tpm_tis_i2c", > + .pm = &tpm_tis_pm, > + .of_match_table = of_match_ptr(of_tis_i2c_match), > + }, > + .probe = tpm_tis_i2c_probe, > + .id_table = tpm_tis_i2c_id, > +}; > + > +module_i2c_driver(tpm_tis_i2c_driver); > + > +MODULE_DESCRIPTION("TPM Driver"); > +MODULE_LICENSE("GPL"); /Jarkko
