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

13 Jul 2021

      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