[PATCH 2/2] tpm2: Add a TPMv2 MMIO TIS driver

[Heinrich Schuchardt]

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 at 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 at 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
>