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