| Message ID | 20210707162604.84196-2-ilias.apalodimas@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2,v2] tpm2: Introduce TIS tpm core | expand |
Hi Ilias, On Wed, 7 Jul 2021 at 10:26, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Add support for devices that expose a TPMv2 though MMIO. > Apart from those devices, we can use the driver in our QEMU setups and > test TPM related code which is difficult to achieve using the sandbox > driver (e.g test the EFI TCG2 protocol). > > It's worth noting that a previous patch added TPMv2 TIS core functions, > which the current driver is consuming. > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > Changes since v1: > - split off the tis core code into a different file > drivers/tpm/Kconfig | 9 +++ > drivers/tpm/Makefile | 1 + > drivers/tpm/tpm2_tis_mmio.c | 156 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 166 insertions(+) > create mode 100644 drivers/tpm/tpm2_tis_mmio.c Looks good to me > > diff --git a/drivers/tpm/Kconfig b/drivers/tpm/Kconfig > index 9eebab5cfd90..406ee8716e1e 100644 > --- a/drivers/tpm/Kconfig > +++ b/drivers/tpm/Kconfig > @@ -161,6 +161,15 @@ config TPM2_FTPM_TEE > help > This driver supports firmware TPM running in TEE. > > +config TPM2_MMIO > + bool "MMIO based TPM2 Interface" > + depends on TPM_V2 > + help > + This driver supports firmware TPM2.0 MMIO interface. > + The usual TPM operations and the 'tpm' command can be used to talk > + to the device using the standard TPM Interface Specification (TIS) > + protocol. > + > endif # TPM_V2 > > endmenu > diff --git a/drivers/tpm/Makefile b/drivers/tpm/Makefile > index f64d20067f88..1065c1874f58 100644 > --- a/drivers/tpm/Makefile > +++ b/drivers/tpm/Makefile > @@ -14,3 +14,4 @@ obj-$(CONFIG_$(SPL_TPL_)TPM2_CR50_I2C) += cr50_i2c.o > obj-$(CONFIG_TPM2_TIS_SANDBOX) += tpm2_tis_sandbox.o > obj-$(CONFIG_TPM2_TIS_SPI) += tpm2_tis_spi.o > obj-$(CONFIG_TPM2_FTPM_TEE) += tpm2_ftpm_tee.o > +obj-$(CONFIG_TPM2_MMIO) += tpm2_tis_core.o tpm2_tis_mmio.o > diff --git a/drivers/tpm/tpm2_tis_mmio.c b/drivers/tpm/tpm2_tis_mmio.c > new file mode 100644 > index 000000000000..2183a2807162 > --- /dev/null > +++ b/drivers/tpm/tpm2_tis_mmio.c > @@ -0,0 +1,156 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * driver for mmio TCG/TIS TPM (trusted platform module). > + * > + * Specifications at www.trustedcomputinggroup.org > + */ > + > +#include <common.h> > +#include <dm.h> > +#include <log.h> > +#include <tpm-v2.h> > +#include <linux/bitops.h> > +#include <linux/compiler.h> > +#include <linux/delay.h> > +#include <linux/errno.h> > +#include <linux/types.h> > +#include <linux/io.h> > +#include <linux/unaligned/be_byteshift.h> > +#include "tpm_tis.h" > +#include "tpm_internal.h" > + > +struct tpm_tis_chip_data { > + unsigned int pcr_count; > + unsigned int pcr_select_min; > + unsigned int time_before_first_cmd_ms; > + void __iomem *iobase; > +}; comments > + > +static int mmio_read_bytes(struct udevice *udev, u32 addr, u16 len, > + u8 *result) > +{ > + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > + > + while (len--) > + *result++ = ioread8(drv_data->iobase + addr); We normally put a blank line before the final return > + return 0; > +} > + > +static int mmio_write_bytes(struct udevice *udev, u32 addr, u16 len, > + const u8 *value) > +{ > + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > + > + while (len--) > + iowrite8(*value++, drv_data->iobase + addr); > + return 0; > +} > + > +static int mmio_read16(struct udevice *udev, u32 addr, u16 *result) > +{ > + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > + > + *result = ioread16(drv_data->iobase + addr); > + return 0; > +} > + > +static int mmio_read32(struct udevice *udev, u32 addr, u32 *result) > +{ > + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > + > + *result = ioread32(drv_data->iobase + addr); > + return 0; > +} > + > +static int mmio_write32(struct udevice *udev, u32 addr, u32 value) > +{ > + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > + > + iowrite32(value, drv_data->iobase + addr); > + return 0; > +} > + > +static struct tpm_tis_phy_ops phy_ops = { Should be a uclass interface. > + .read_bytes = mmio_read_bytes, > + .write_bytes = mmio_write_bytes, > + .read16 = mmio_read16, > + .read32 = mmio_read32, > + .write32 = mmio_write32, > +}; > + > +static int tpm_tis_probe(struct udevice *udev) > +{ > + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > + struct tpm_chip_priv *priv = dev_get_uclass_priv(udev); > + int ret = 0; > + fdt_addr_t ioaddr; > + u64 sz; > + > + ioaddr = dev_read_addr(udev); > + if (ioaddr == FDT_ADDR_T_NONE) > + return -EINVAL; consider this for easier debugging: return log_msg_ret("ioaddr", -EINVAL); > + > + ret = dev_read_u64(udev, "reg", &sz); > + if (ret) > + return -EINVAL; > + > + drv_data->iobase = ioremap(ioaddr, sz); > + log_info("Remapped TPM2 base: 0x%llx size: 0x%llx\n", ioaddr, sz); log_debug() I think? > + tpm_tis_ops_register(udev, &phy_ops); > + ret = tpm_tis_init(udev); > + if (ret) > + goto iounmap; > + > + priv->pcr_count = drv_data->pcr_count; > + priv->pcr_select_min = drv_data->pcr_select_min; > + /* > + * Although the driver probably works with a TPMv1 our Kconfig > + * limits the driver to TPMv2 only > + */ > + priv->version = TPM_V2; > + > + return ret; > +iounmap: > + iounmap(drv_data->iobase); > + return -EINVAL; > +} > + > +static int tpm_tis_remove(struct udevice *udev) > +{ > + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > + > + iounmap(drv_data->iobase); > + return tpm_tis_cleanup(udev); > +} > + > +static const struct tpm_ops tpm_tis_ops = { > + .open = tpm_tis_open, > + .close = tpm_tis_close, > + .get_desc = tpm_tis_get_desc, > + .send = tpm_tis_send, > + .recv = tpm_tis_recv, > + .cleanup = tpm_tis_cleanup, > +}; > + > +static const struct tpm_tis_chip_data tpm_tis_std_chip_data = { > + .pcr_count = 24, > + .pcr_select_min = 3, > +}; > + > +static const struct udevice_id tpm_tis_ids[] = { > + { > + .compatible = "tcg,tpm-tis-mmio", > + .data = (ulong)&tpm_tis_std_chip_data, > + }, > + { } > +}; > + > +U_BOOT_DRIVER(tpm_tis_mmio) = { > + .name = "tpm_tis_mmio", > + .id = UCLASS_TPM, > + .of_match = tpm_tis_ids, > + .ops = &tpm_tis_ops, > + .probe = tpm_tis_probe, > + .remove = tpm_tis_remove, > + .priv_auto = sizeof(struct tpm_chip), > +}; > -- > 2.32.0.rc0 > Regards, Simon
Hi Simon, On Sat, Jul 10, 2021 at 06:00:59PM -0600, Simon Glass wrote: > Hi Ilias, > > On Wed, 7 Jul 2021 at 10:26, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Add support for devices that expose a TPMv2 though MMIO. > > Apart from those devices, we can use the driver in our QEMU setups and > > test TPM related code which is difficult to achieve using the sandbox > > driver (e.g test the EFI TCG2 protocol). > > > > It's worth noting that a previous patch added TPMv2 TIS core functions, > > which the current driver is consuming. > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > --- > > Changes since v1: > > - split off the tis core code into a different file > > drivers/tpm/Kconfig | 9 +++ > > drivers/tpm/Makefile | 1 + > > drivers/tpm/tpm2_tis_mmio.c | 156 ++++++++++++++++++++++++++++++++++++ > > 3 files changed, 166 insertions(+) > > create mode 100644 drivers/tpm/tpm2_tis_mmio.c > > Looks good to me Thanks! > > > > +struct tpm_tis_chip_data { > > + unsigned int pcr_count; > > + unsigned int pcr_select_min; > > + unsigned int time_before_first_cmd_ms; > > + void __iomem *iobase; > > +}; > > comments > You mean on all the internal functions? Sure I can add them > > + > > +static int mmio_read_bytes(struct udevice *udev, u32 addr, u16 len, > > + u8 *result) > > +{ > > + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > > + > > + while (len--) > > + *result++ = ioread8(drv_data->iobase + addr); > > We normally put a blank line before the final return > sure > > + return 0; > > +} > > + > > +static int mmio_write_bytes(struct udevice *udev, u32 addr, u16 len, > > + const u8 *value) > > +{ > > + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > > + > > + while (len--) > > + iowrite8(*value++, drv_data->iobase + addr); > > + return 0; > > +} > > + > > +static int mmio_read16(struct udevice *udev, u32 addr, u16 *result) > > +{ > > + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > > + > > + *result = ioread16(drv_data->iobase + addr); > > + return 0; > > +} > > + > > +static int mmio_read32(struct udevice *udev, u32 addr, u32 *result) > > +{ > > + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > > + > > + *result = ioread32(drv_data->iobase + addr); > > + return 0; > > +} > > + > > +static int mmio_write32(struct udevice *udev, u32 addr, u32 value) > > +{ > > + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > > + > > + iowrite32(value, drv_data->iobase + addr); > > + return 0; > > +} > > + > > +static struct tpm_tis_phy_ops phy_ops = { > > Should be a uclass interface. > Why? A uclass is supposed to describe and abstract hardware. This is just a specific implementation of a TPM, not all TPMs are TIS compliant. We already have a uclass for those. > > + .read_bytes = mmio_read_bytes, > > + .write_bytes = mmio_write_bytes, > > + .read16 = mmio_read16, > > + .read32 = mmio_read32, > > + .write32 = mmio_write32, > > +}; > > + > > +static int tpm_tis_probe(struct udevice *udev) > > +{ > > + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > > + struct tpm_chip_priv *priv = dev_get_uclass_priv(udev); > > + int ret = 0; > > + fdt_addr_t ioaddr; > > + u64 sz; > > + > > + ioaddr = dev_read_addr(udev); > > + if (ioaddr == FDT_ADDR_T_NONE) > > + return -EINVAL; > > consider this for easier debugging: > > return log_msg_ret("ioaddr", -EINVAL); > Sure > > + > > + ret = dev_read_u64(udev, "reg", &sz); > > + if (ret) > > + return -EINVAL; > > + > > + drv_data->iobase = ioremap(ioaddr, sz); > > + log_info("Remapped TPM2 base: 0x%llx size: 0x%llx\n", ioaddr, sz); > > log_debug() I think? > Yea, that's a leftover of my initial code, were I needed to make sure the ioremap worked. > > + tpm_tis_ops_register(udev, &phy_ops); > > + ret = tpm_tis_init(udev); > > + if (ret) > > + goto iounmap; > > + > > + priv->pcr_count = drv_data->pcr_count; > > + priv->pcr_select_min = drv_data->pcr_select_min; > > + /* > > + * Although the driver probably works with a TPMv1 our Kconfig > > + * limits the driver to TPMv2 only > > + */ > > + priv->version = TPM_V2; > > + > > + return ret; > > +iounmap: > > + iounmap(drv_data->iobase); > > + return -EINVAL; > > +} > > + > > +static int tpm_tis_remove(struct udevice *udev) > > +{ > > + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > > + > > + iounmap(drv_data->iobase); > > + return tpm_tis_cleanup(udev); > > +} > > + > > +static const struct tpm_ops tpm_tis_ops = { > > + .open = tpm_tis_open, > > + .close = tpm_tis_close, > > + .get_desc = tpm_tis_get_desc, > > + .send = tpm_tis_send, > > + .recv = tpm_tis_recv, > > + .cleanup = tpm_tis_cleanup, > > +}; > > + > > +static const struct tpm_tis_chip_data tpm_tis_std_chip_data = { > > + .pcr_count = 24, > > + .pcr_select_min = 3, > > +}; > > + > > +static const struct udevice_id tpm_tis_ids[] = { > > + { > > + .compatible = "tcg,tpm-tis-mmio", > > + .data = (ulong)&tpm_tis_std_chip_data, > > + }, > > + { } > > +}; > > + > > +U_BOOT_DRIVER(tpm_tis_mmio) = { > > + .name = "tpm_tis_mmio", > > + .id = UCLASS_TPM, > > + .of_match = tpm_tis_ids, > > + .ops = &tpm_tis_ops, > > + .probe = tpm_tis_probe, > > + .remove = tpm_tis_remove, > > + .priv_auto = sizeof(struct tpm_chip), > > +}; > > -- > > 2.32.0.rc0 > > > > Regards, > Simon Thanks for looking at this /Ilias
On 7/12/21 8:22 PM, Ilias Apalodimas wrote: > Hi Simon, > On Sat, Jul 10, 2021 at 06:00:59PM -0600, Simon Glass wrote: >> Hi Ilias, >> >> On Wed, 7 Jul 2021 at 10:26, Ilias Apalodimas >> <ilias.apalodimas@linaro.org> wrote: >>> >>> Add support for devices that expose a TPMv2 though MMIO. >>> Apart from those devices, we can use the driver in our QEMU setups and >>> test TPM related code which is difficult to achieve using the sandbox >>> driver (e.g test the EFI TCG2 protocol). >>> >>> It's worth noting that a previous patch added TPMv2 TIS core functions, >>> which the current driver is consuming. >>> >>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> >>> --- >>> Changes since v1: >>> - split off the tis core code into a different file >>> drivers/tpm/Kconfig | 9 +++ >>> drivers/tpm/Makefile | 1 + >>> drivers/tpm/tpm2_tis_mmio.c | 156 ++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 166 insertions(+) >>> create mode 100644 drivers/tpm/tpm2_tis_mmio.c >> >> Looks good to me > > Thanks! > >>> >>> +struct tpm_tis_chip_data { >>> + unsigned int pcr_count; >>> + unsigned int pcr_select_min; >>> + unsigned int time_before_first_cmd_ms; >>> + void __iomem *iobase; >>> +}; >> >> comments >> > > You mean on all the internal functions? > Sure I can add them > >>> + >>> +static int mmio_read_bytes(struct udevice *udev, u32 addr, u16 len, >>> + u8 *result) >>> +{ >>> + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); >>> + >>> + while (len--) >>> + *result++ = ioread8(drv_data->iobase + addr); >> >> We normally put a blank line before the final return >> > > sure > >>> + return 0; >>> +} >>> + >>> +static int mmio_write_bytes(struct udevice *udev, u32 addr, u16 len, >>> + const u8 *value) >>> +{ >>> + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); >>> + >>> + while (len--) >>> + iowrite8(*value++, drv_data->iobase + addr); >>> + return 0; >>> +} >>> + >>> +static int mmio_read16(struct udevice *udev, u32 addr, u16 *result) >>> +{ >>> + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); >>> + >>> + *result = ioread16(drv_data->iobase + addr); >>> + return 0; >>> +} >>> + >>> +static int mmio_read32(struct udevice *udev, u32 addr, u32 *result) >>> +{ >>> + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); >>> + >>> + *result = ioread32(drv_data->iobase + addr); >>> + return 0; >>> +} >>> + >>> +static int mmio_write32(struct udevice *udev, u32 addr, u32 value) >>> +{ >>> + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); >>> + >>> + iowrite32(value, drv_data->iobase + addr); >>> + return 0; >>> +} >>> + >>> +static struct tpm_tis_phy_ops phy_ops = { >> >> Should be a uclass interface. >> > > Why? A uclass is supposed to describe and abstract hardware. This is just > a specific implementation of a TPM, not all TPMs are TIS compliant. We already > have a uclass for those. The design proposed by Ilias matches what we find in Linux for TPM. Keeping the same structure should render porting of drivers for additional devices easier. Best regards Heinrich > >>> + .read_bytes = mmio_read_bytes, >>> + .write_bytes = mmio_write_bytes, >>> + .read16 = mmio_read16, >>> + .read32 = mmio_read32, >>> + .write32 = mmio_write32, >>> +}; >>> + >>> +static int tpm_tis_probe(struct udevice *udev) >>> +{ >>> + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); >>> + struct tpm_chip_priv *priv = dev_get_uclass_priv(udev); >>> + int ret = 0; >>> + fdt_addr_t ioaddr; >>> + u64 sz; >>> + >>> + ioaddr = dev_read_addr(udev); >>> + if (ioaddr == FDT_ADDR_T_NONE) >>> + return -EINVAL; >> >> consider this for easier debugging: >> >> return log_msg_ret("ioaddr", -EINVAL); >> > > Sure > >>> + >>> + ret = dev_read_u64(udev, "reg", &sz); >>> + if (ret) >>> + return -EINVAL; >>> + >>> + drv_data->iobase = ioremap(ioaddr, sz); >>> + log_info("Remapped TPM2 base: 0x%llx size: 0x%llx\n", ioaddr, sz); >> >> log_debug() I think? >> > Yea, that's a leftover of my initial code, were I needed to make sure the > ioremap worked. > >>> + tpm_tis_ops_register(udev, &phy_ops); >>> + ret = tpm_tis_init(udev); >>> + if (ret) >>> + goto iounmap; >>> + >>> + priv->pcr_count = drv_data->pcr_count; >>> + priv->pcr_select_min = drv_data->pcr_select_min; >>> + /* >>> + * Although the driver probably works with a TPMv1 our Kconfig >>> + * limits the driver to TPMv2 only >>> + */ >>> + priv->version = TPM_V2; >>> + >>> + return ret; >>> +iounmap: >>> + iounmap(drv_data->iobase); >>> + return -EINVAL; >>> +} >>> + >>> +static int tpm_tis_remove(struct udevice *udev) >>> +{ >>> + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); >>> + >>> + iounmap(drv_data->iobase); >>> + return tpm_tis_cleanup(udev); >>> +} >>> + >>> +static const struct tpm_ops tpm_tis_ops = { >>> + .open = tpm_tis_open, >>> + .close = tpm_tis_close, >>> + .get_desc = tpm_tis_get_desc, >>> + .send = tpm_tis_send, >>> + .recv = tpm_tis_recv, >>> + .cleanup = tpm_tis_cleanup, >>> +}; >>> + >>> +static const struct tpm_tis_chip_data tpm_tis_std_chip_data = { >>> + .pcr_count = 24, >>> + .pcr_select_min = 3, >>> +}; >>> + >>> +static const struct udevice_id tpm_tis_ids[] = { >>> + { >>> + .compatible = "tcg,tpm-tis-mmio", >>> + .data = (ulong)&tpm_tis_std_chip_data, >>> + }, >>> + { } >>> +}; >>> + >>> +U_BOOT_DRIVER(tpm_tis_mmio) = { >>> + .name = "tpm_tis_mmio", >>> + .id = UCLASS_TPM, >>> + .of_match = tpm_tis_ids, >>> + .ops = &tpm_tis_ops, >>> + .probe = tpm_tis_probe, >>> + .remove = tpm_tis_remove, >>> + .priv_auto = sizeof(struct tpm_chip), >>> +}; >>> -- >>> 2.32.0.rc0 >>> >> >> Regards, >> Simon > > Thanks for looking at this > /Ilias >
Hi Ilias, On Mon, 12 Jul 2021 at 12:22, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Simon, > On Sat, Jul 10, 2021 at 06:00:59PM -0600, Simon Glass wrote: > > Hi Ilias, > > > > On Wed, 7 Jul 2021 at 10:26, Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > Add support for devices that expose a TPMv2 though MMIO. > > > Apart from those devices, we can use the driver in our QEMU setups and > > > test TPM related code which is difficult to achieve using the sandbox > > > driver (e.g test the EFI TCG2 protocol). > > > > > > It's worth noting that a previous patch added TPMv2 TIS core functions, > > > which the current driver is consuming. > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > --- > > > Changes since v1: > > > - split off the tis core code into a different file > > > drivers/tpm/Kconfig | 9 +++ > > > drivers/tpm/Makefile | 1 + > > > drivers/tpm/tpm2_tis_mmio.c | 156 ++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 166 insertions(+) > > > create mode 100644 drivers/tpm/tpm2_tis_mmio.c > > > > Looks good to me > > Thanks! > > > > > > > +struct tpm_tis_chip_data { > > > + unsigned int pcr_count; > > > + unsigned int pcr_select_min; > > > + unsigned int time_before_first_cmd_ms; > > > + void __iomem *iobase; > > > +}; > > > > comments > > > > You mean on all the internal functions? > Sure I can add them I mean for struct members. For internal functions a comment is a good idea depending on how valuable it is - things like return values, args. But if just implementing a fully documented API, it may not really add much. > > > > + > > > +static int mmio_read_bytes(struct udevice *udev, u32 addr, u16 len, > > > + u8 *result) > > > +{ > > > + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > > > + > > > + while (len--) > > > + *result++ = ioread8(drv_data->iobase + addr); > > > > We normally put a blank line before the final return > > > > sure > > > > + return 0; > > > +} > > > + > > > +static int mmio_write_bytes(struct udevice *udev, u32 addr, u16 len, > > > + const u8 *value) > > > +{ > > > + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > > > + > > > + while (len--) > > > + iowrite8(*value++, drv_data->iobase + addr); > > > + return 0; > > > +} > > > + > > > +static int mmio_read16(struct udevice *udev, u32 addr, u16 *result) > > > +{ > > > + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > > > + > > > + *result = ioread16(drv_data->iobase + addr); > > > + return 0; > > > +} > > > + > > > +static int mmio_read32(struct udevice *udev, u32 addr, u32 *result) > > > +{ > > > + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > > > + > > > + *result = ioread32(drv_data->iobase + addr); > > > + return 0; > > > +} > > > + > > > +static int mmio_write32(struct udevice *udev, u32 addr, u32 value) > > > +{ > > > + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > > > + > > > + iowrite32(value, drv_data->iobase + addr); > > > + return 0; > > > +} > > > + > > > +static struct tpm_tis_phy_ops phy_ops = { > > > > Should be a uclass interface. > > > > Why? A uclass is supposed to describe and abstract hardware. This is just > a specific implementation of a TPM, not all TPMs are TIS compliant. We already > have a uclass for those. Who told you that a uclass is supposed to describe and abstract hardware? :-) The uclass is how driver model does APIs, so normally a uclass would be used for any API. There are exceptions, but this one actually looks like a useful interface we should have. > > > > + .read_bytes = mmio_read_bytes, > > > + .write_bytes = mmio_write_bytes, > > > + .read16 = mmio_read16, > > > + .read32 = mmio_read32, > > > + .write32 = mmio_write32, > > > +}; > > > + > > > +static int tpm_tis_probe(struct udevice *udev) > > > +{ > > > + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > > > + struct tpm_chip_priv *priv = dev_get_uclass_priv(udev); > > > + int ret = 0; > > > + fdt_addr_t ioaddr; > > > + u64 sz; > > > + > > > + ioaddr = dev_read_addr(udev); > > > + if (ioaddr == FDT_ADDR_T_NONE) > > > + return -EINVAL; > > > > consider this for easier debugging: > > > > return log_msg_ret("ioaddr", -EINVAL); > > > > Sure > > > > + > > > + ret = dev_read_u64(udev, "reg", &sz); > > > + if (ret) > > > + return -EINVAL; > > > + > > > + drv_data->iobase = ioremap(ioaddr, sz); > > > + log_info("Remapped TPM2 base: 0x%llx size: 0x%llx\n", ioaddr, sz); > > > > log_debug() I think? > > > Yea, that's a leftover of my initial code, were I needed to make sure the > ioremap worked. > > > > + tpm_tis_ops_register(udev, &phy_ops); > > > + ret = tpm_tis_init(udev); > > > + if (ret) > > > + goto iounmap; > > > + > > > + priv->pcr_count = drv_data->pcr_count; > > > + priv->pcr_select_min = drv_data->pcr_select_min; > > > + /* > > > + * Although the driver probably works with a TPMv1 our Kconfig > > > + * limits the driver to TPMv2 only > > > + */ > > > + priv->version = TPM_V2; > > > + > > > + return ret; > > > +iounmap: > > > + iounmap(drv_data->iobase); > > > + return -EINVAL; > > > +} > > > + > > > +static int tpm_tis_remove(struct udevice *udev) > > > +{ > > > + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > > > + > > > + iounmap(drv_data->iobase); > > > + return tpm_tis_cleanup(udev); > > > +} > > > + > > > +static const struct tpm_ops tpm_tis_ops = { > > > + .open = tpm_tis_open, > > > + .close = tpm_tis_close, > > > + .get_desc = tpm_tis_get_desc, > > > + .send = tpm_tis_send, > > > + .recv = tpm_tis_recv, > > > + .cleanup = tpm_tis_cleanup, > > > +}; > > > + > > > +static const struct tpm_tis_chip_data tpm_tis_std_chip_data = { > > > + .pcr_count = 24, > > > + .pcr_select_min = 3, > > > +}; > > > + > > > +static const struct udevice_id tpm_tis_ids[] = { > > > + { > > > + .compatible = "tcg,tpm-tis-mmio", > > > + .data = (ulong)&tpm_tis_std_chip_data, > > > + }, > > > + { } > > > +}; > > > + > > > +U_BOOT_DRIVER(tpm_tis_mmio) = { > > > + .name = "tpm_tis_mmio", > > > + .id = UCLASS_TPM, > > > + .of_match = tpm_tis_ids, > > > + .ops = &tpm_tis_ops, > > > + .probe = tpm_tis_probe, > > > + .remove = tpm_tis_remove, > > > + .priv_auto = sizeof(struct tpm_chip), > > > +}; > > > -- > > > 2.32.0.rc0 > > > > > > > Regards, > > Simon > > Thanks for looking at this > /Ilias Likewise. Regards, Simon
Hi Heinrich, On Mon, 12 Jul 2021 at 13:19, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 7/12/21 8:22 PM, Ilias Apalodimas wrote: > > Hi Simon, > > On Sat, Jul 10, 2021 at 06:00:59PM -0600, Simon Glass wrote: > >> Hi Ilias, > >> > >> On Wed, 7 Jul 2021 at 10:26, Ilias Apalodimas > >> <ilias.apalodimas@linaro.org> wrote: > >>> > >>> Add support for devices that expose a TPMv2 though MMIO. > >>> Apart from those devices, we can use the driver in our QEMU setups and > >>> test TPM related code which is difficult to achieve using the sandbox > >>> driver (e.g test the EFI TCG2 protocol). > >>> > >>> It's worth noting that a previous patch added TPMv2 TIS core functions, > >>> which the current driver is consuming. > >>> > >>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > >>> --- > >>> Changes since v1: > >>> - split off the tis core code into a different file > >>> drivers/tpm/Kconfig | 9 +++ > >>> drivers/tpm/Makefile | 1 + > >>> drivers/tpm/tpm2_tis_mmio.c | 156 ++++++++++++++++++++++++++++++++++++ > >>> 3 files changed, 166 insertions(+) > >>> create mode 100644 drivers/tpm/tpm2_tis_mmio.c > >> > >> Looks good to me > > > > Thanks! > > > >>> > >>> +struct tpm_tis_chip_data { > >>> + unsigned int pcr_count; > >>> + unsigned int pcr_select_min; > >>> + unsigned int time_before_first_cmd_ms; > >>> + void __iomem *iobase; > >>> +}; > >> > >> comments > >> > > > > You mean on all the internal functions? > > Sure I can add them > > > >>> + > >>> +static int mmio_read_bytes(struct udevice *udev, u32 addr, u16 len, > >>> + u8 *result) > >>> +{ > >>> + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > >>> + > >>> + while (len--) > >>> + *result++ = ioread8(drv_data->iobase + addr); > >> > >> We normally put a blank line before the final return > >> > > > > sure > > > >>> + return 0; > >>> +} > >>> + > >>> +static int mmio_write_bytes(struct udevice *udev, u32 addr, u16 len, > >>> + const u8 *value) > >>> +{ > >>> + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > >>> + > >>> + while (len--) > >>> + iowrite8(*value++, drv_data->iobase + addr); > >>> + return 0; > >>> +} > >>> + > >>> +static int mmio_read16(struct udevice *udev, u32 addr, u16 *result) > >>> +{ > >>> + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > >>> + > >>> + *result = ioread16(drv_data->iobase + addr); > >>> + return 0; > >>> +} > >>> + > >>> +static int mmio_read32(struct udevice *udev, u32 addr, u32 *result) > >>> +{ > >>> + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > >>> + > >>> + *result = ioread32(drv_data->iobase + addr); > >>> + return 0; > >>> +} > >>> + > >>> +static int mmio_write32(struct udevice *udev, u32 addr, u32 value) > >>> +{ > >>> + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > >>> + > >>> + iowrite32(value, drv_data->iobase + addr); > >>> + return 0; > >>> +} > >>> + > >>> +static struct tpm_tis_phy_ops phy_ops = { > >> > >> Should be a uclass interface. > >> > > > > Why? A uclass is supposed to describe and abstract hardware. This is just > > a specific implementation of a TPM, not all TPMs are TIS compliant. We already > > have a uclass for those. > > The design proposed by Ilias matches what we find in Linux for TPM. > Keeping the same structure should render porting of drivers for > additional devices easier. Can you please be more specific in your comment? What change are you asking for? Regards, Simon
Am 12. Juli 2021 21:43:53 MESZ schrieb Simon Glass <sjg@chromium.org>: >Hi Heinrich, > >On Mon, 12 Jul 2021 at 13:19, Heinrich Schuchardt <xypron.glpk@gmx.de> >wrote: >> >> On 7/12/21 8:22 PM, Ilias Apalodimas wrote: >> > Hi Simon, >> > On Sat, Jul 10, 2021 at 06:00:59PM -0600, Simon Glass wrote: >> >> Hi Ilias, >> >> >> >> On Wed, 7 Jul 2021 at 10:26, Ilias Apalodimas >> >> <ilias.apalodimas@linaro.org> wrote: >> >>> >> >>> Add support for devices that expose a TPMv2 though MMIO. >> >>> Apart from those devices, we can use the driver in our QEMU >setups and >> >>> test TPM related code which is difficult to achieve using the >sandbox >> >>> driver (e.g test the EFI TCG2 protocol). >> >>> >> >>> It's worth noting that a previous patch added TPMv2 TIS core >functions, >> >>> which the current driver is consuming. >> >>> >> >>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> >> >>> --- >> >>> Changes since v1: >> >>> - split off the tis core code into a different file >> >>> drivers/tpm/Kconfig | 9 +++ >> >>> drivers/tpm/Makefile | 1 + >> >>> drivers/tpm/tpm2_tis_mmio.c | 156 >++++++++++++++++++++++++++++++++++++ >> >>> 3 files changed, 166 insertions(+) >> >>> create mode 100644 drivers/tpm/tpm2_tis_mmio.c >> >> >> >> Looks good to me >> > >> > Thanks! >> > >> >>> >> >>> +struct tpm_tis_chip_data { >> >>> + unsigned int pcr_count; >> >>> + unsigned int pcr_select_min; >> >>> + unsigned int time_before_first_cmd_ms; >> >>> + void __iomem *iobase; >> >>> +}; >> >> >> >> comments >> >> >> > >> > You mean on all the internal functions? >> > Sure I can add them >> > >> >>> + >> >>> +static int mmio_read_bytes(struct udevice *udev, u32 addr, u16 >len, >> >>> + u8 *result) >> >>> +{ >> >>> + struct tpm_tis_chip_data *drv_data = (void >*)dev_get_driver_data(udev); >> >>> + >> >>> + while (len--) >> >>> + *result++ = ioread8(drv_data->iobase + addr); >> >> >> >> We normally put a blank line before the final return >> >> >> > >> > sure >> > >> >>> + return 0; >> >>> +} >> >>> + >> >>> +static int mmio_write_bytes(struct udevice *udev, u32 addr, u16 >len, >> >>> + const u8 *value) >> >>> +{ >> >>> + struct tpm_tis_chip_data *drv_data = (void >*)dev_get_driver_data(udev); >> >>> + >> >>> + while (len--) >> >>> + iowrite8(*value++, drv_data->iobase + addr); >> >>> + return 0; >> >>> +} >> >>> + >> >>> +static int mmio_read16(struct udevice *udev, u32 addr, u16 >*result) >> >>> +{ >> >>> + struct tpm_tis_chip_data *drv_data = (void >*)dev_get_driver_data(udev); >> >>> + >> >>> + *result = ioread16(drv_data->iobase + addr); >> >>> + return 0; >> >>> +} >> >>> + >> >>> +static int mmio_read32(struct udevice *udev, u32 addr, u32 >*result) >> >>> +{ >> >>> + struct tpm_tis_chip_data *drv_data = (void >*)dev_get_driver_data(udev); >> >>> + >> >>> + *result = ioread32(drv_data->iobase + addr); >> >>> + return 0; >> >>> +} >> >>> + >> >>> +static int mmio_write32(struct udevice *udev, u32 addr, u32 >value) >> >>> +{ >> >>> + struct tpm_tis_chip_data *drv_data = (void >*)dev_get_driver_data(udev); >> >>> + >> >>> + iowrite32(value, drv_data->iobase + addr); >> >>> + return 0; >> >>> +} >> >>> + >> >>> +static struct tpm_tis_phy_ops phy_ops = { >> >> >> >> Should be a uclass interface. >> >> >> > >> > Why? A uclass is supposed to describe and abstract hardware. This >is just >> > a specific implementation of a TPM, not all TPMs are TIS compliant. >We already >> > have a uclass for those. >> >> The design proposed by Ilias matches what we find in Linux for TPM. >> Keeping the same structure should render porting of drivers for >> additional devices easier. > >Can you please be more specific in your comment? What change are you >asking for? > >Regards, >Simon None. I think the design is ok as is. I was responding to your idea of adding another uclass. Best regards Heinrich
Hi Heinrich, On Mon, 12 Jul 2021 at 16:47, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > Am 12. Juli 2021 21:43:53 MESZ schrieb Simon Glass <sjg@chromium.org>: > >Hi Heinrich, > > > >On Mon, 12 Jul 2021 at 13:19, Heinrich Schuchardt <xypron.glpk@gmx.de> > >wrote: > >> > >> On 7/12/21 8:22 PM, Ilias Apalodimas wrote: > >> > Hi Simon, > >> > On Sat, Jul 10, 2021 at 06:00:59PM -0600, Simon Glass wrote: > >> >> Hi Ilias, > >> >> > >> >> On Wed, 7 Jul 2021 at 10:26, Ilias Apalodimas > >> >> <ilias.apalodimas@linaro.org> wrote: > >> >>> > >> >>> Add support for devices that expose a TPMv2 though MMIO. > >> >>> Apart from those devices, we can use the driver in our QEMU > >setups and > >> >>> test TPM related code which is difficult to achieve using the > >sandbox > >> >>> driver (e.g test the EFI TCG2 protocol). > >> >>> > >> >>> It's worth noting that a previous patch added TPMv2 TIS core > >functions, > >> >>> which the current driver is consuming. > >> >>> > >> >>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > >> >>> --- > >> >>> Changes since v1: > >> >>> - split off the tis core code into a different file > >> >>> drivers/tpm/Kconfig | 9 +++ > >> >>> drivers/tpm/Makefile | 1 + > >> >>> drivers/tpm/tpm2_tis_mmio.c | 156 > >++++++++++++++++++++++++++++++++++++ > >> >>> 3 files changed, 166 insertions(+) > >> >>> create mode 100644 drivers/tpm/tpm2_tis_mmio.c > >> >> > >> >> Looks good to me > >> > > >> > Thanks! > >> > > >> >>> > >> >>> +struct tpm_tis_chip_data { > >> >>> + unsigned int pcr_count; > >> >>> + unsigned int pcr_select_min; > >> >>> + unsigned int time_before_first_cmd_ms; > >> >>> + void __iomem *iobase; > >> >>> +}; > >> >> > >> >> comments > >> >> > >> > > >> > You mean on all the internal functions? > >> > Sure I can add them > >> > > >> >>> + > >> >>> +static int mmio_read_bytes(struct udevice *udev, u32 addr, u16 > >len, > >> >>> + u8 *result) > >> >>> +{ > >> >>> + struct tpm_tis_chip_data *drv_data = (void > >*)dev_get_driver_data(udev); > >> >>> + > >> >>> + while (len--) > >> >>> + *result++ = ioread8(drv_data->iobase + addr); > >> >> > >> >> We normally put a blank line before the final return > >> >> > >> > > >> > sure > >> > > >> >>> + return 0; > >> >>> +} > >> >>> + > >> >>> +static int mmio_write_bytes(struct udevice *udev, u32 addr, u16 > >len, > >> >>> + const u8 *value) > >> >>> +{ > >> >>> + struct tpm_tis_chip_data *drv_data = (void > >*)dev_get_driver_data(udev); > >> >>> + > >> >>> + while (len--) > >> >>> + iowrite8(*value++, drv_data->iobase + addr); > >> >>> + return 0; > >> >>> +} > >> >>> + > >> >>> +static int mmio_read16(struct udevice *udev, u32 addr, u16 > >*result) > >> >>> +{ > >> >>> + struct tpm_tis_chip_data *drv_data = (void > >*)dev_get_driver_data(udev); > >> >>> + > >> >>> + *result = ioread16(drv_data->iobase + addr); > >> >>> + return 0; > >> >>> +} > >> >>> + > >> >>> +static int mmio_read32(struct udevice *udev, u32 addr, u32 > >*result) > >> >>> +{ > >> >>> + struct tpm_tis_chip_data *drv_data = (void > >*)dev_get_driver_data(udev); > >> >>> + > >> >>> + *result = ioread32(drv_data->iobase + addr); > >> >>> + return 0; > >> >>> +} > >> >>> + > >> >>> +static int mmio_write32(struct udevice *udev, u32 addr, u32 > >value) > >> >>> +{ > >> >>> + struct tpm_tis_chip_data *drv_data = (void > >*)dev_get_driver_data(udev); > >> >>> + > >> >>> + iowrite32(value, drv_data->iobase + addr); > >> >>> + return 0; > >> >>> +} > >> >>> + > >> >>> +static struct tpm_tis_phy_ops phy_ops = { > >> >> > >> >> Should be a uclass interface. > >> >> > >> > > >> > Why? A uclass is supposed to describe and abstract hardware. This > >is just > >> > a specific implementation of a TPM, not all TPMs are TIS compliant. > >We already > >> > have a uclass for those. > >> > >> The design proposed by Ilias matches what we find in Linux for TPM. > >> Keeping the same structure should render porting of drivers for > >> additional devices easier. > > > >Can you please be more specific in your comment? What change are you > >asking for? > > > >Regards, > >Simon > > None. I think the design is ok as is. > > I was responding to your idea of adding another uclass. It seems obvious to me that the proposed API is useful outside TPMs. It provides an MMIO interface, after all. Regards, Simon
[...] > > > Should be a uclass interface. > > > > > > > Why? A uclass is supposed to describe and abstract hardware. This is just > > a specific implementation of a TPM, not all TPMs are TIS compliant. We already > > have a uclass for those. > > Who told you that a uclass is supposed to describe and abstract hardware? :-) > That's what I've mostly seen it used for, maybe i got the idea wrong. > The uclass is how driver model does APIs, so normally a uclass would > be used for any API. There are exceptions, but this one actually looks > like a useful interface we should have. > the point is we already have a uclass for tpm devices. So why should the we add another one that just describes the TIS interface? > > > > > > + .read_bytes = mmio_read_bytes, > > > > + .write_bytes = mmio_write_bytes, > > > > + .read16 = mmio_read16, > > > > + .read32 = mmio_read32, > > > > + .write32 = mmio_write32, > > > > +}; > > > > + > > > > +static int tpm_tis_probe(struct udevice *udev) > > > > +{ > > > > + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > > > > + struct tpm_chip_priv *priv = dev_get_uclass_priv(udev); > > > > + int ret = 0; > > > > + fdt_addr_t ioaddr; > > > > + u64 sz; > > > > + > > > > + ioaddr = dev_read_addr(udev); > > > > + if (ioaddr == FDT_ADDR_T_NONE) > > > > + return -EINVAL; > > > > > > consider this for easier debugging: > > > > > > return log_msg_ret("ioaddr", -EINVAL); > > > > > > > Sure > > > > > > + > > > > + ret = dev_read_u64(udev, "reg", &sz); > > > > + if (ret) > > > > + return -EINVAL; > > > > + > > > > + drv_data->iobase = ioremap(ioaddr, sz); > > > > + log_info("Remapped TPM2 base: 0x%llx size: 0x%llx\n", ioaddr, sz); > > > > > > log_debug() I think? > > > > > Yea, that's a leftover of my initial code, were I needed to make sure the > > ioremap worked. > > > > > > + tpm_tis_ops_register(udev, &phy_ops); > > > > + ret = tpm_tis_init(udev); > > > > + if (ret) > > > > + goto iounmap; > > > > + > > > > + priv->pcr_count = drv_data->pcr_count; > > > > + priv->pcr_select_min = drv_data->pcr_select_min; > > > > + /* > > > > + * Although the driver probably works with a TPMv1 our Kconfig > > > > + * limits the driver to TPMv2 only > > > > + */ > > > > + priv->version = TPM_V2; > > > > + > > > > + return ret; > > > > +iounmap: > > > > + iounmap(drv_data->iobase); > > > > + return -EINVAL; > > > > +} > > > > + > > > > +static int tpm_tis_remove(struct udevice *udev) > > > > +{ > > > > + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > > > > + > > > > + iounmap(drv_data->iobase); > > > > + return tpm_tis_cleanup(udev); > > > > +} > > > > + > > > > +static const struct tpm_ops tpm_tis_ops = { > > > > + .open = tpm_tis_open, > > > > + .close = tpm_tis_close, > > > > + .get_desc = tpm_tis_get_desc, > > > > + .send = tpm_tis_send, > > > > + .recv = tpm_tis_recv, > > > > + .cleanup = tpm_tis_cleanup, > > > > +}; > > > > + > > > > +static const struct tpm_tis_chip_data tpm_tis_std_chip_data = { > > > > + .pcr_count = 24, > > > > + .pcr_select_min = 3, > > > > +}; > > > > + > > > > +static const struct udevice_id tpm_tis_ids[] = { > > > > + { > > > > + .compatible = "tcg,tpm-tis-mmio", > > > > + .data = (ulong)&tpm_tis_std_chip_data, > > > > + }, > > > > + { } > > > > +}; > > > > + > > > > +U_BOOT_DRIVER(tpm_tis_mmio) = { > > > > + .name = "tpm_tis_mmio", > > > > + .id = UCLASS_TPM, > > > > + .of_match = tpm_tis_ids, > > > > + .ops = &tpm_tis_ops, > > > > + .probe = tpm_tis_probe, > > > > + .remove = tpm_tis_remove, > > > > + .priv_auto = sizeof(struct tpm_chip), > > > > +}; > > > > -- > > > > 2.32.0.rc0 > > > > > > > > > > Regards, > > > Simon > > > > Thanks for looking at this > > /Ilias > > Likewise. > > Regards, > Simon
Hi Ilias, On Tue, 13 Jul 2021 at 14:11, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > > [...] > > > > Should be a uclass interface. > > > > > > > > > > Why? A uclass is supposed to describe and abstract hardware. This is just > > > a specific implementation of a TPM, not all TPMs are TIS compliant. We already > > > have a uclass for those. > > > > Who told you that a uclass is supposed to describe and abstract hardware? :-) > > > > That's what I've mostly seen it used for, maybe i got the idea wrong. A uclass is basically a software construct. It is an interface between clients and the driver, typically. Quite often the uclass is an interface on top of the hardware (actually the driver). But quite often it is not. For example, we use an GPIO uclass to access a pmic's GPIOs, we use an I2C uclass to access the cros_ec I2C tunnel. Anywhere where it makes sense to have an abstraction, we use a uclass. > > > The uclass is how driver model does APIs, so normally a uclass would > > be used for any API. There are exceptions, but this one actually looks > > like a useful interface we should have. > > > > the point is we already have a uclass for tpm devices. So why should the > we add another one that just describes the TIS interface? You have already added another API, right? All we are discussing is whether it should be a uclass or not. Unless there is a very good reason, we should avoid creating custom interfaces that don't use driver model. I actually think the interface you've created (MMIO) will be very useful as a uclass. [..] Regards, Simon
On Tue, Jul 13, 2021 at 08:49:21PM -0600, Simon Glass wrote: > Hi Ilias, > > On Tue, 13 Jul 2021 at 14:11, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > > > [...] > > > > > Should be a uclass interface. > > > > > > > > > > > > > Why? A uclass is supposed to describe and abstract hardware. This is just > > > > a specific implementation of a TPM, not all TPMs are TIS compliant. We already > > > > have a uclass for those. > > > > > > Who told you that a uclass is supposed to describe and abstract hardware? :-) > > > > > > > That's what I've mostly seen it used for, maybe i got the idea wrong. > > A uclass is basically a software construct. It is an interface between > clients and the driver, typically. Quite often the uclass is an > interface on top of the hardware (actually the driver). But quite > often it is not. For example, we use an GPIO uclass to access a pmic's > GPIOs, we use an I2C uclass to access the cros_ec I2C tunnel. Anywhere > where it makes sense to have an abstraction, we use a uclass. > > > > > > The uclass is how driver model does APIs, so normally a uclass would > > > be used for any API. There are exceptions, but this one actually looks > > > like a useful interface we should have. > > > > > > > the point is we already have a uclass for tpm devices. So why should the > > we add another one that just describes the TIS interface? > > You have already added another API, right? All we are discussing is > whether it should be a uclass or not. Unless there is a very good > reason, we should avoid creating custom interfaces that don't use > driver model. I actually think the interface you've created (MMIO) > will be very useful as a uclass. > So you are basically looking into adding something similar to dm_i2c_read/dm_i2c_write etc? I assume this is gong to be the default read method passed on the TIS API when we want to support i2c TPMs. For the MMIO case that would essentially mean, move the functions on a different file, add them on a header and define a UCLASS_DRIVER with only the .id and .name defined? Thanks /Ilias > [..] > > Regards, > Simon
Hi Ilias, On Tue, 13 Jul 2021 at 23:23, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > On Tue, Jul 13, 2021 at 08:49:21PM -0600, Simon Glass wrote: > > Hi Ilias, > > > > On Tue, 13 Jul 2021 at 14:11, Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > [...] > > > > > > Should be a uclass interface. > > > > > > > > > > > > > > > > Why? A uclass is supposed to describe and abstract hardware. This is just > > > > > a specific implementation of a TPM, not all TPMs are TIS compliant. We already > > > > > have a uclass for those. > > > > > > > > Who told you that a uclass is supposed to describe and abstract hardware? :-) > > > > > > > > > > That's what I've mostly seen it used for, maybe i got the idea wrong. > > > > A uclass is basically a software construct. It is an interface between > > clients and the driver, typically. Quite often the uclass is an > > interface on top of the hardware (actually the driver). But quite > > often it is not. For example, we use an GPIO uclass to access a pmic's > > GPIOs, we use an I2C uclass to access the cros_ec I2C tunnel. Anywhere > > where it makes sense to have an abstraction, we use a uclass. > > > > > > > > > The uclass is how driver model does APIs, so normally a uclass would > > > > be used for any API. There are exceptions, but this one actually looks > > > > like a useful interface we should have. > > > > > > > > > > the point is we already have a uclass for tpm devices. So why should the > > > we add another one that just describes the TIS interface? > > > > You have already added another API, right? All we are discussing is > > whether it should be a uclass or not. Unless there is a very good > > reason, we should avoid creating custom interfaces that don't use > > driver model. I actually think the interface you've created (MMIO) > > will be very useful as a uclass. > > > > So you are basically looking into adding something similar to > dm_i2c_read/dm_i2c_write etc? > I assume this is gong to be the default read method passed on the TIS API > when we want to support i2c TPMs. Well I'm not quite sure, but if MMIO can sit on top of I2C, then yes. > For the MMIO case that would essentially mean, move the functions on a > different file, add them on a header and define a UCLASS_DRIVER with only > the .id and .name defined? Yes. Something like: - add a new uclass ID to dm/uciass-id.h - add include/mmio.h (or whatever it is called) with operations (see pch.h as an example) - add drivers/misc/mmio-uclass.c with the UCLASS_DRIVER and stubs for the API - add drivers/misc/sandbox_mmio.c with a sandbox driver that does very little (perhaps just read canned data?) - add test/dm/mmio.c sandbox test that calls each API function and checks the result As I said pch is a good example since it is fairly simple. Regards, Simon
diff --git a/drivers/tpm/Kconfig b/drivers/tpm/Kconfig index 9eebab5cfd90..406ee8716e1e 100644 --- a/drivers/tpm/Kconfig +++ b/drivers/tpm/Kconfig @@ -161,6 +161,15 @@ config TPM2_FTPM_TEE help This driver supports firmware TPM running in TEE. +config TPM2_MMIO + bool "MMIO based TPM2 Interface" + depends on TPM_V2 + help + This driver supports firmware TPM2.0 MMIO interface. + The usual TPM operations and the 'tpm' command can be used to talk + to the device using the standard TPM Interface Specification (TIS) + protocol. + endif # TPM_V2 endmenu diff --git a/drivers/tpm/Makefile b/drivers/tpm/Makefile index f64d20067f88..1065c1874f58 100644 --- a/drivers/tpm/Makefile +++ b/drivers/tpm/Makefile @@ -14,3 +14,4 @@ obj-$(CONFIG_$(SPL_TPL_)TPM2_CR50_I2C) += cr50_i2c.o obj-$(CONFIG_TPM2_TIS_SANDBOX) += tpm2_tis_sandbox.o obj-$(CONFIG_TPM2_TIS_SPI) += tpm2_tis_spi.o obj-$(CONFIG_TPM2_FTPM_TEE) += tpm2_ftpm_tee.o +obj-$(CONFIG_TPM2_MMIO) += tpm2_tis_core.o tpm2_tis_mmio.o diff --git a/drivers/tpm/tpm2_tis_mmio.c b/drivers/tpm/tpm2_tis_mmio.c new file mode 100644 index 000000000000..2183a2807162 --- /dev/null +++ b/drivers/tpm/tpm2_tis_mmio.c @@ -0,0 +1,156 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * driver for mmio TCG/TIS TPM (trusted platform module). + * + * Specifications at www.trustedcomputinggroup.org + */ + +#include <common.h> +#include <dm.h> +#include <log.h> +#include <tpm-v2.h> +#include <linux/bitops.h> +#include <linux/compiler.h> +#include <linux/delay.h> +#include <linux/errno.h> +#include <linux/types.h> +#include <linux/io.h> +#include <linux/unaligned/be_byteshift.h> +#include "tpm_tis.h" +#include "tpm_internal.h" + +struct tpm_tis_chip_data { + unsigned int pcr_count; + unsigned int pcr_select_min; + unsigned int time_before_first_cmd_ms; + void __iomem *iobase; +}; + +static int mmio_read_bytes(struct udevice *udev, u32 addr, u16 len, + u8 *result) +{ + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); + + while (len--) + *result++ = ioread8(drv_data->iobase + addr); + return 0; +} + +static int mmio_write_bytes(struct udevice *udev, u32 addr, u16 len, + const u8 *value) +{ + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); + + while (len--) + iowrite8(*value++, drv_data->iobase + addr); + return 0; +} + +static int mmio_read16(struct udevice *udev, u32 addr, u16 *result) +{ + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); + + *result = ioread16(drv_data->iobase + addr); + return 0; +} + +static int mmio_read32(struct udevice *udev, u32 addr, u32 *result) +{ + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); + + *result = ioread32(drv_data->iobase + addr); + return 0; +} + +static int mmio_write32(struct udevice *udev, u32 addr, u32 value) +{ + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); + + iowrite32(value, drv_data->iobase + addr); + return 0; +} + +static struct tpm_tis_phy_ops phy_ops = { + .read_bytes = mmio_read_bytes, + .write_bytes = mmio_write_bytes, + .read16 = mmio_read16, + .read32 = mmio_read32, + .write32 = mmio_write32, +}; + +static int tpm_tis_probe(struct udevice *udev) +{ + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); + struct tpm_chip_priv *priv = dev_get_uclass_priv(udev); + int ret = 0; + fdt_addr_t ioaddr; + u64 sz; + + ioaddr = dev_read_addr(udev); + if (ioaddr == FDT_ADDR_T_NONE) + return -EINVAL; + + ret = dev_read_u64(udev, "reg", &sz); + if (ret) + return -EINVAL; + + drv_data->iobase = ioremap(ioaddr, sz); + log_info("Remapped TPM2 base: 0x%llx size: 0x%llx\n", ioaddr, sz); + tpm_tis_ops_register(udev, &phy_ops); + ret = tpm_tis_init(udev); + if (ret) + goto iounmap; + + priv->pcr_count = drv_data->pcr_count; + priv->pcr_select_min = drv_data->pcr_select_min; + /* + * Although the driver probably works with a TPMv1 our Kconfig + * limits the driver to TPMv2 only + */ + priv->version = TPM_V2; + + return ret; +iounmap: + iounmap(drv_data->iobase); + return -EINVAL; +} + +static int tpm_tis_remove(struct udevice *udev) +{ + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); + + iounmap(drv_data->iobase); + return tpm_tis_cleanup(udev); +} + +static const struct tpm_ops tpm_tis_ops = { + .open = tpm_tis_open, + .close = tpm_tis_close, + .get_desc = tpm_tis_get_desc, + .send = tpm_tis_send, + .recv = tpm_tis_recv, + .cleanup = tpm_tis_cleanup, +}; + +static const struct tpm_tis_chip_data tpm_tis_std_chip_data = { + .pcr_count = 24, + .pcr_select_min = 3, +}; + +static const struct udevice_id tpm_tis_ids[] = { + { + .compatible = "tcg,tpm-tis-mmio", + .data = (ulong)&tpm_tis_std_chip_data, + }, + { } +}; + +U_BOOT_DRIVER(tpm_tis_mmio) = { + .name = "tpm_tis_mmio", + .id = UCLASS_TPM, + .of_match = tpm_tis_ids, + .ops = &tpm_tis_ops, + .probe = tpm_tis_probe, + .remove = tpm_tis_remove, + .priv_auto = sizeof(struct tpm_chip), +};
Add support for devices that expose a TPMv2 though MMIO. Apart from those devices, we can use the driver in our QEMU setups and test TPM related code which is difficult to achieve using the sandbox driver (e.g test the EFI TCG2 protocol). It's worth noting that a previous patch added TPMv2 TIS core functions, which the current driver is consuming. Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- Changes since v1: - split off the tis core code into a different file drivers/tpm/Kconfig | 9 +++ drivers/tpm/Makefile | 1 + drivers/tpm/tpm2_tis_mmio.c | 156 ++++++++++++++++++++++++++++++++++++ 3 files changed, 166 insertions(+) create mode 100644 drivers/tpm/tpm2_tis_mmio.c -- 2.32.0.rc0