On Wed, 27 Mar 2024 at 20:47, Tim Harvey <thar...@gateworks.com> wrote:
>
> On Wed, Mar 27, 2024 at 10:26 AM Ilias Apalodimas
> <ilias.apalodi...@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
Thanks. It's a bit unfortunate we ended up with broken devices, but if
we remove the GPIO reset I am pretty sure the side effect will be
having an unusable TPM after warm resets since PCRs will retain the
pre-reboot values...
/Ilias
>
> Best Regards,
>
> Tim
> [1]
> https://patchwork.ozlabs.org/project/uboot/patch/20240321180219.1039622-1-thar...@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 <thar...@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
> > > > > > > >
