[PATCH] drivers: tpm2: update reset gpio semantics

[PATCH] drivers: tpm2: update reset gpio semantics

[Jorge Ramirez-Ortiz]

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",
-- 
2.31.1

Re: [PATCH] drivers: tpm2: update reset gpio semantics

[Michal Simek]

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

Re: [PATCH] drivers: tpm2: update reset gpio semantics

[Simon Glass]

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>

Re: [PATCH] drivers: tpm2: update reset gpio semantics

[Jorge Ramirez-Ortiz, Gmail]

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

Re: [PATCH] drivers: tpm2: update reset gpio semantics

[Bruno Thomsen]

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

Re: [PATCH] drivers: tpm2: update reset gpio semantics

[Michal Simek]

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

Re: [PATCH] drivers: tpm2: update reset gpio semantics

[Jorge Ramirez-Ortiz, Foundries]

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

Re: [PATCH] drivers: tpm2: update reset gpio semantics

[Michal Simek]

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