Am 12. Juli 2021 21:43:53 MESZ schrieb Simon Glass <s...@chromium.org>: >Hi Heinrich, > >On Mon, 12 Jul 2021 at 13:19, Heinrich Schuchardt <xypron.g...@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.apalodi...@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.apalodi...@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