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

[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