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

[Heinrich Schuchardt]

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.

Best regards

Heinrich