On Wed, 27 Mar 2024 at 20:47, Tim Harvey tharvey@gateworks.com wrote:
On Wed, Mar 27, 2024 at 10:26 AM Ilias Apalodimas ilias.apalodimas@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-th...
Cheers /Ilias
Regards /Ilias
Best regards,
Tim
Thanks /Ilias
Best Regards,
Tim [1] https://ww1.microchip.com/downloads/en/DeviceDoc/ATTPM20P-Trusted-Platform-M...
> Thanks > /Ilias > > > > Signed-off-by: Tim Harvey tharvey@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 > >