[PATCH] tpm: display message when using gpio-reset instead of when missing it

[Tim Harvey]

On Wed, Mar 27, 2024 at 10:26 AM Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> [...]
>
> > >
> > > missing an invalid property?
> > >
> > > I deal with users all the time that think things like that are
> > > 'errors' and contact tech support. In this case its not an error
> > > because there is no gpio reset in the official dt-bindings for the tpm
> > > and its generally considered bad form to add non official properties.
> > >
> > > And from what your explaining we shouldn't have a GPIO connected to
> > > the TPM so perhaps we should remove the reset completely and perhaps
> > > even spit out a warning if present:
> > > ignore Invalid DT property gpio reset to conform with the TCG specification
> >
> > We should, but those changes predate me being appointed as a TPM
> > maintainer. If I had to guess, I would say that was added for TPM that
> > are connected on an external SPI bus (e.g the RPI).
> > So what about not printing the error message, keeping the code so we
> > won't break 'test' devices, and print a warning message like "This
> > shouldn't be used on secure production devices"?
>
> Unless we can get a list of the devices that *currently* use it. If
> they aren't that many I am fine getting rid of the reset overall (and
> I can test it on the RPI4)
>

Adding Miquel who authored commit a174f0001f592 ("tpm2: tis_spi: add
the possibility to reset the chip with a gpio"):
On some designs, the reset line could not be connected to the SoC reset
line, in this case, request the GPIO and ensure the chip gets reset.

Adding Jorge who athoried commit cc5afabc9d329 ("drivers: tpm2: update
reset gpio semantics"):
Use the more generic reset-gpios property name.

I looked through all dts in U-Boot matching 'tpm' as well as Linux to
see who's using it:
Adding Adam who has a 'reset-gpios' in
arch/arm/dts/imx8mp-beacon-kit.dts (and somehow snuck it in upstream
as well)
Adding Rasmus who has a suspicious 'regulatot-tpm0-rst' (but no gpio
reset in tpm) in arch/arm/dts/imx8mm-cl-iot-gate-ied-tpm0.dtso

As there is at least one board that clearly uses a gpio reset for the
TPM in the tpm node I think we leave the tpm reset support, eliminate
the warning if its missing and print a message like "Warning: TPM gpio
reset should not be used on secure production device"

I'll send a v2 of my patch for review

Best Regards,

Tim
[1] https://patchwork.ozlabs.org/project/uboot/patch/20240321180219.1039622-1-tharvey@gateworks.com/

> Cheers
> /Ilias
> >
> > Regards
> > /Ilias
> > >
> > > Best regards,
> > >
> > > Tim
> > >
> > > >
> > > > Thanks
> > > > /Ilias
> > > > >
> > > > > Best Regards,
> > > > >
> > > > > Tim
> > > > > [1] https://ww1.microchip.com/downloads/en/DeviceDoc/ATTPM20P-Trusted-Platform-Module-TPM-2.0-SPI-Interface-Summary-Data-Sheet-DS40002082A.pdf
> > > > >
> > > > >
> > > > > > Thanks
> > > > > > /Ilias
> > > > > > >
> > > > > > > Signed-off-by: Tim Harvey <tharvey at gateworks.com>
> > > > > > > ---
> > > > > > >  drivers/tpm/tpm2_tis_spi.c | 8 ++++----
> > > > > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c
> > > > > > > index de9cf8f21e07..944540f7a711 100644
> > > > > > > --- a/drivers/tpm/tpm2_tis_spi.c
> > > > > > > +++ b/drivers/tpm/tpm2_tis_spi.c
> > > > > > > @@ -237,14 +237,14 @@ static int tpm_tis_spi_probe(struct udevice *dev)
> > > > > > >                         /* legacy reset */
> > > > > > >                         ret = gpio_request_by_name(dev, "gpio-reset", 0,
> > > > > > >                                                    &reset_gpio, GPIOD_IS_OUT);
> > > > > > > -                       if (ret) {
> > > > > > > -                               log(LOGC_NONE, LOGL_NOTICE,
> > > > > > > -                                   "%s: missing reset GPIO\n", __func__);
> > > > > > > +                       if (ret)
> > > > > > >                                 goto init;
> > > > > > > -                       }
> > > > > > >                         log(LOGC_NONE, LOGL_NOTICE,
> > > > > > >                             "%s: gpio-reset is deprecated\n", __func__);
> > > > > > >                 }
> > > > > > > +               log(LOGC_NONE, LOGL_NOTICE,
> > > > > > > +                   "%s: performing 1ms reset on %s:%d\n", dev->name,
> > > > > > > +                   reset_gpio.dev->name, reset_gpio.offset);
> > > > > > >                 dm_gpio_set_value(&reset_gpio, 1);
> > > > > > >                 mdelay(1);
> > > > > > >                 dm_gpio_set_value(&reset_gpio, 0);
> > > > > > > --
> > > > > > > 2.25.1
> > > > > > >