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 > > > > > > > >