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

[Simon Glass]

Hi Heinrich,

On Mon, 12 Jul 2021 at 16:47, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> Am 12. Juli 2021 21:43:53 MESZ schrieb Simon Glass <sjg at chromium.org>:
> >Hi Heinrich,
> >
> >On Mon, 12 Jul 2021 at 13:19, Heinrich Schuchardt <xypron.glpk at 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 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.
> >
> >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