| Message ID | 20210526195712.15210-1-jorge@foundries.io |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | drivers: tpm2: update reset gpio semantics | expand |
On 5/26/21 9:57 PM, Jorge Ramirez-Ortiz wrote: > Use the more generic reset-gpios propery name. > > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> > --- > doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt | 2 +- > drivers/tpm/tpm2_tis_spi.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt b/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt > index 3a2ee4bd17..bbcd12950f 100644 > --- a/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt > +++ b/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt > @@ -6,7 +6,7 @@ Required properties: > - reg : SPI Chip select > > Optional properties: > -- gpio-reset : Reset GPIO (if not connected to the SoC reset line) > +- reset-gpios : Reset GPIO (if not connected to the SoC reset line) > - spi-max-frequency : See spi-bus.txt > > Example: > diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c > index 4b33ac8fd3..94ac52d9ce 100644 > --- a/drivers/tpm/tpm2_tis_spi.c > +++ b/drivers/tpm/tpm2_tis_spi.c > @@ -589,7 +589,7 @@ static int tpm_tis_spi_probe(struct udevice *dev) > if (CONFIG_IS_ENABLED(DM_GPIO)) { > struct gpio_desc reset_gpio; > > - ret = gpio_request_by_name(dev, "gpio-reset", 0, > + ret = gpio_request_by_name(dev, "reset-gpios", 0, > &reset_gpio, GPIOD_IS_OUT); > if (ret) { > log(LOGC_NONE, LOGL_NOTICE, "%s: missing reset GPIO\n", > I think you should deprecate gpio-reset but keep supporting that option with any warning and add code for reset-gpios. Also would be good to add it as optional property to Linux kernel to keep it in sync. Thanks, Michal
On Wed, 26 May 2021 at 13:57, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote: > > Use the more generic reset-gpios propery name. > > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> > --- > doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt | 2 +- > drivers/tpm/tpm2_tis_spi.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Simon Glass <sjg@chromium.org>
On 27/05/21, Simon Glass wrote: > On Wed, 26 May 2021 at 13:57, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote: > > > > Use the more generic reset-gpios propery name. > > > > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> > > --- > > doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt | 2 +- > > drivers/tpm/tpm2_tis_spi.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > Reviewed-by: Simon Glass <sjg@chromium.org> thanks. should I add your reviewed-by by and resubmit the patch or do I also need to include the change proposed by Michal Simek (keeping the legacy property)? TIA
Den tor. 27. maj 2021 kl. 09.15 skrev Michal Simek <michal.simek@xilinx.com>: > > > > On 5/26/21 9:57 PM, Jorge Ramirez-Ortiz wrote: > > Use the more generic reset-gpios propery name. > > > > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> > > --- > > doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt | 2 +- > > drivers/tpm/tpm2_tis_spi.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt b/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt > > index 3a2ee4bd17..bbcd12950f 100644 > > --- a/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt > > +++ b/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt > > @@ -6,7 +6,7 @@ Required properties: > > - reg : SPI Chip select > > > > Optional properties: > > -- gpio-reset : Reset GPIO (if not connected to the SoC reset line) > > +- reset-gpios : Reset GPIO (if not connected to the SoC reset line) > > - spi-max-frequency : See spi-bus.txt > > > > Example: > > diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c > > index 4b33ac8fd3..94ac52d9ce 100644 > > --- a/drivers/tpm/tpm2_tis_spi.c > > +++ b/drivers/tpm/tpm2_tis_spi.c > > @@ -589,7 +589,7 @@ static int tpm_tis_spi_probe(struct udevice *dev) > > if (CONFIG_IS_ENABLED(DM_GPIO)) { > > struct gpio_desc reset_gpio; > > > > - ret = gpio_request_by_name(dev, "gpio-reset", 0, > > + ret = gpio_request_by_name(dev, "reset-gpios", 0, > > &reset_gpio, GPIOD_IS_OUT); > > if (ret) { > > log(LOGC_NONE, LOGL_NOTICE, "%s: missing reset GPIO\n", > > > > I think you should deprecate gpio-reset but keep supporting that option > with any warning and add code for reset-gpios. > > Also would be good to add it as optional property to Linux kernel to > keep it in sync. Hi The reason the Linux kernel does not have a TPM reset signal, is that being able to reset the chip from software is a vulnerability. There was a discussion on it over on the Barebox mailing list a while ago. TLDR: TPM reset needs to follow SOC reset. /Bruno
On 5/28/21 6:18 PM, Bruno Thomsen wrote: > Den tor. 27. maj 2021 kl. 09.15 skrev Michal Simek <michal.simek@xilinx.com>: >> >> >> >> On 5/26/21 9:57 PM, Jorge Ramirez-Ortiz wrote: >>> Use the more generic reset-gpios propery name. >>> >>> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> >>> --- >>> doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt | 2 +- >>> drivers/tpm/tpm2_tis_spi.c | 2 +- >>> 2 files changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt b/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt >>> index 3a2ee4bd17..bbcd12950f 100644 >>> --- a/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt >>> +++ b/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt >>> @@ -6,7 +6,7 @@ Required properties: >>> - reg : SPI Chip select >>> >>> Optional properties: >>> -- gpio-reset : Reset GPIO (if not connected to the SoC reset line) >>> +- reset-gpios : Reset GPIO (if not connected to the SoC reset line) >>> - spi-max-frequency : See spi-bus.txt >>> >>> Example: >>> diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c >>> index 4b33ac8fd3..94ac52d9ce 100644 >>> --- a/drivers/tpm/tpm2_tis_spi.c >>> +++ b/drivers/tpm/tpm2_tis_spi.c >>> @@ -589,7 +589,7 @@ static int tpm_tis_spi_probe(struct udevice *dev) >>> if (CONFIG_IS_ENABLED(DM_GPIO)) { >>> struct gpio_desc reset_gpio; >>> >>> - ret = gpio_request_by_name(dev, "gpio-reset", 0, >>> + ret = gpio_request_by_name(dev, "reset-gpios", 0, >>> &reset_gpio, GPIOD_IS_OUT); >>> if (ret) { >>> log(LOGC_NONE, LOGL_NOTICE, "%s: missing reset GPIO\n", >>> >> >> I think you should deprecate gpio-reset but keep supporting that option >> with any warning and add code for reset-gpios. >> >> Also would be good to add it as optional property to Linux kernel to >> keep it in sync. > > Hi > > The reason the Linux kernel does not have a TPM reset signal, is > that being able to reset the chip from software is a vulnerability. > There was a discussion on it over on the Barebox mailing list > a while ago. > > TLDR: TPM reset needs to follow SOC reset. I expect chip has the reset in both cases and it is just about who should be calling it. But we should be using the same DT for u-boot and Linux. It means it should be handled properly but described properly. Thanks, Michal
On 31/05/21, Michal Simek wrote: > > > On 5/28/21 6:18 PM, Bruno Thomsen wrote: > > Den tor. 27. maj 2021 kl. 09.15 skrev Michal Simek <michal.simek@xilinx.com>: > >> > >> > >> > >> On 5/26/21 9:57 PM, Jorge Ramirez-Ortiz wrote: > >>> Use the more generic reset-gpios propery name. > >>> > >>> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> > >>> --- > >>> doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt | 2 +- > >>> drivers/tpm/tpm2_tis_spi.c | 2 +- > >>> 2 files changed, 2 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt b/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt > >>> index 3a2ee4bd17..bbcd12950f 100644 > >>> --- a/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt > >>> +++ b/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt > >>> @@ -6,7 +6,7 @@ Required properties: > >>> - reg : SPI Chip select > >>> > >>> Optional properties: > >>> -- gpio-reset : Reset GPIO (if not connected to the SoC reset line) > >>> +- reset-gpios : Reset GPIO (if not connected to the SoC reset line) > >>> - spi-max-frequency : See spi-bus.txt > >>> > >>> Example: > >>> diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c > >>> index 4b33ac8fd3..94ac52d9ce 100644 > >>> --- a/drivers/tpm/tpm2_tis_spi.c > >>> +++ b/drivers/tpm/tpm2_tis_spi.c > >>> @@ -589,7 +589,7 @@ static int tpm_tis_spi_probe(struct udevice *dev) > >>> if (CONFIG_IS_ENABLED(DM_GPIO)) { > >>> struct gpio_desc reset_gpio; > >>> > >>> - ret = gpio_request_by_name(dev, "gpio-reset", 0, > >>> + ret = gpio_request_by_name(dev, "reset-gpios", 0, > >>> &reset_gpio, GPIOD_IS_OUT); > >>> if (ret) { > >>> log(LOGC_NONE, LOGL_NOTICE, "%s: missing reset GPIO\n", > >>> > >> > >> I think you should deprecate gpio-reset but keep supporting that option > >> with any warning and add code for reset-gpios. > >> > >> Also would be good to add it as optional property to Linux kernel to > >> keep it in sync. > > > > Hi > > > > The reason the Linux kernel does not have a TPM reset signal, is > > that being able to reset the chip from software is a vulnerability. > > There was a discussion on it over on the Barebox mailing list > > a while ago. > > > > TLDR: TPM reset needs to follow SOC reset. > > I expect chip has the reset in both cases and it is just about who > should be calling it. But we should be using the same DT for u-boot and > Linux. It means it should be handled properly but described properly. right, I agree that it should be described properly (that was the patch intent). but do we need to keep the legacy property? > > Thanks, > Michal > >
On 5/31/21 3:17 PM, Jorge Ramirez-Ortiz, Foundries wrote: > On 31/05/21, Michal Simek wrote: >> >> >> On 5/28/21 6:18 PM, Bruno Thomsen wrote: >>> Den tor. 27. maj 2021 kl. 09.15 skrev Michal Simek <michal.simek@xilinx.com>: >>>> >>>> >>>> >>>> On 5/26/21 9:57 PM, Jorge Ramirez-Ortiz wrote: >>>>> Use the more generic reset-gpios propery name. >>>>> >>>>> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> >>>>> --- >>>>> doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt | 2 +- >>>>> drivers/tpm/tpm2_tis_spi.c | 2 +- >>>>> 2 files changed, 2 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt b/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt >>>>> index 3a2ee4bd17..bbcd12950f 100644 >>>>> --- a/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt >>>>> +++ b/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt >>>>> @@ -6,7 +6,7 @@ Required properties: >>>>> - reg : SPI Chip select >>>>> >>>>> Optional properties: >>>>> -- gpio-reset : Reset GPIO (if not connected to the SoC reset line) >>>>> +- reset-gpios : Reset GPIO (if not connected to the SoC reset line) >>>>> - spi-max-frequency : See spi-bus.txt >>>>> >>>>> Example: >>>>> diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c >>>>> index 4b33ac8fd3..94ac52d9ce 100644 >>>>> --- a/drivers/tpm/tpm2_tis_spi.c >>>>> +++ b/drivers/tpm/tpm2_tis_spi.c >>>>> @@ -589,7 +589,7 @@ static int tpm_tis_spi_probe(struct udevice *dev) >>>>> if (CONFIG_IS_ENABLED(DM_GPIO)) { >>>>> struct gpio_desc reset_gpio; >>>>> >>>>> - ret = gpio_request_by_name(dev, "gpio-reset", 0, >>>>> + ret = gpio_request_by_name(dev, "reset-gpios", 0, >>>>> &reset_gpio, GPIOD_IS_OUT); >>>>> if (ret) { >>>>> log(LOGC_NONE, LOGL_NOTICE, "%s: missing reset GPIO\n", >>>>> >>>> >>>> I think you should deprecate gpio-reset but keep supporting that option >>>> with any warning and add code for reset-gpios. >>>> >>>> Also would be good to add it as optional property to Linux kernel to >>>> keep it in sync. >>> >>> Hi >>> >>> The reason the Linux kernel does not have a TPM reset signal, is >>> that being able to reset the chip from software is a vulnerability. >>> There was a discussion on it over on the Barebox mailing list >>> a while ago. >>> >>> TLDR: TPM reset needs to follow SOC reset. >> >> I expect chip has the reset in both cases and it is just about who >> should be calling it. But we should be using the same DT for u-boot and >> Linux. It means it should be handled properly but described properly. > > right, I agree that it should be described properly (that was the > patch intent). > > but do we need to keep the legacy property? I prefer all the time to have some time for transition. It means add support for new property. Keep there old one with message that this will be removed in near future. Not aware if there is any time defined. We normally keep it there for a year. Thanks, Michal
diff --git a/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt b/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt index 3a2ee4bd17..bbcd12950f 100644 --- a/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt +++ b/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt @@ -6,7 +6,7 @@ Required properties: - reg : SPI Chip select Optional properties: -- gpio-reset : Reset GPIO (if not connected to the SoC reset line) +- reset-gpios : Reset GPIO (if not connected to the SoC reset line) - spi-max-frequency : See spi-bus.txt Example: diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c index 4b33ac8fd3..94ac52d9ce 100644 --- a/drivers/tpm/tpm2_tis_spi.c +++ b/drivers/tpm/tpm2_tis_spi.c @@ -589,7 +589,7 @@ static int tpm_tis_spi_probe(struct udevice *dev) if (CONFIG_IS_ENABLED(DM_GPIO)) { struct gpio_desc reset_gpio; - ret = gpio_request_by_name(dev, "gpio-reset", 0, + ret = gpio_request_by_name(dev, "reset-gpios", 0, &reset_gpio, GPIOD_IS_OUT); if (ret) { log(LOGC_NONE, LOGL_NOTICE, "%s: missing reset GPIO\n",
Use the more generic reset-gpios propery name. Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> --- doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt | 2 +- drivers/tpm/tpm2_tis_spi.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)