[PATCH 1/2] dt-bindings: reset: Add Infineon SLB9670 TPM reset driver

[PATCH 1/2] dt-bindings: reset: Add Infineon SLB9670 TPM reset driver

[Lukas Wunner]

A new reset driver is about to be added to perform the reset sequence of
the Infineon SLB9670 Trusted Platform Module.

Document its device tree bindings.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 .../bindings/reset/infineon,slb9670-reset.yaml     | 68 ++++++++++++++++++++++
 1 file changed, 68 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/infineon,slb9670-reset.yaml

diff --git a/Documentation/devicetree/bindings/reset/infineon,slb9670-reset.yaml b/Documentation/devicetree/bindings/reset/infineon,slb9670-reset.yaml
new file mode 100644
index 00000000..b1e23d47
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/infineon,slb9670-reset.yaml
@@ -0,0 +1,68 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reset/infineon,slb9670-reset.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Infineon SLB9670 TPM Reset Driver
+
+maintainers:
+  - Lukas Wunner <lukas@wunner.de>
+
+description: |
+  The Infineon SLB9670 Trusted Platform Module requires a specific reset
+  sequence on its RST# pin which is documented in sections 5.4 and 5.5 of
+  the datasheet [1]. This driver performs the reset sequence using a GPIO.
+
+  The sequence with minimum wait intervals is as follows:
+  deassert RST#
+  wait at least 60 ms
+  assert RST#
+  wait at least 2 usecs
+  deassert RST#
+  wait at least 60 ms
+  assert RST#
+  wait at least 2 usecs
+  deassert RST#
+  wait at least 60 ms before issuing the first TPM command
+
+  [1] https://www.infineon.com/dgdl/Infineon-SLB%209670VQ2.0-DataSheet-v01_04-EN.pdf?fileId=5546d4626fc1ce0b016fc78270350cd6
+
+properties:
+  compatible:
+    enum:
+      - infineon,slb9670-reset
+
+  reset-gpios:
+    maxItems: 1
+    description: Reference to the GPIO connected to the RST# pin.
+
+  "#reset-cells":
+    const: 0
+
+required:
+  - compatible
+  - reset-gpios
+  - "#reset-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    tpm_reset: reset-controller {
+        compatible = "infineon,slb9670-reset";
+        #reset-cells = <0>;
+        reset-gpios = <&gpio 0 GPIO_ACTIVE_LOW>;
+    };
+
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        tpm@0 {
+            compatible = "infineon,slb9670";
+            reg = <0>;
+            resets = <&tpm_reset>;
+        };
+    };
-- 
2.40.1

[PATCH 2/2] reset: Add Infineon SLB9670 TPM reset driver

[Lukas Wunner]

From: Lino Sanfilippo <l.sanfilippo@kunbus.com>

Normally the platform firmware is responsible for taking a Trusted
Platform Module out of reset on boot and storing measurements into it.

However if the platform firmware is incapable of doing that -- as is the
case on the Raspberry Pi -- then the onus is on the kernel to take the
TPM out of reset before trying to attach a driver to it.

Provide a reset driver for such platforms.

The Infineon SLB9670 TPM requires a specific reset sequence on its RST#
pin which is documented in sections 5.4 and 5.5 of the datasheet:

https://www.infineon.com/dgdl/Infineon-SLB%209670VQ2.0-DataSheet-v01_04-EN.pdf?fileId=5546d4626fc1ce0b016fc78270350cd6

The sequence with minimum wait intervals is as follows:

  deassert RST#
  wait at least 60 ms
  assert RST#
  wait at least 2 usecs
  deassert RST#
  wait at least 60 ms
  assert RST#
  wait at least 2 usecs
  deassert RST#
  wait at least 60 ms before issuing the first TPM command

Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/reset/Kconfig         |   9 +++
 drivers/reset/Makefile        |   1 +
 drivers/reset/reset-slb9670.c | 141 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 151 insertions(+)
 create mode 100644 drivers/reset/reset-slb9670.c

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index ccd59dd..3296e33 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -229,6 +229,15 @@ config RESET_SIMPLE
 	   - Allwinner SoCs
 	   - SiFive FU740 SoCs
 
+config RESET_SLB9670
+	tristate "Infineon SLB9670 TPM Reset Driver"
+	depends on TCG_TIS_SPI
+	help
+	  This enables the reset driver for the Infineon SLB9670 Trusted
+	  Platform Module. Only say Y here if your platform firmware is
+	  incapable of taking the TPM out of reset on boot, requiring the
+	  kernel to do so.
+
 config RESET_SOCFPGA
 	bool "SoCFPGA Reset Driver" if COMPILE_TEST && (!ARM || !ARCH_INTEL_SOCFPGA)
 	default ARM && ARCH_INTEL_SOCFPGA
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 8270da8..d9c182e 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_RESET_RASPBERRYPI) += reset-raspberrypi.o
 obj-$(CONFIG_RESET_RZG2L_USBPHY_CTRL) += reset-rzg2l-usbphy-ctrl.o
 obj-$(CONFIG_RESET_SCMI) += reset-scmi.o
 obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
+obj-$(CONFIG_RESET_SLB9670) += reset-slb9670.o
 obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o
 obj-$(CONFIG_RESET_SUNPLUS) += reset-sunplus.o
 obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
diff --git a/drivers/reset/reset-slb9670.c b/drivers/reset/reset-slb9670.c
new file mode 100644
index 00000000..cc09ab5
--- /dev/null
+++ b/drivers/reset/reset-slb9670.c
@@ -0,0 +1,141 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Reset driver for Infineon SLB9670 Trusted Platform Module
+ *
+ * Copyright (C) 2022 KUNBUS GmbH
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+
+/*
+ * Time intervals used in the reset sequence:
+ *
+ * RSTIN: minimum time to hold the reset line deasserted
+ * WRST: minimum time to hold the reset line asserted
+ */
+#define SLB9670_TIME_RSTIN	60 /* msecs */
+#define SLB9670_TIME_WRST	2  /* usecs */
+
+struct reset_slb9670 {
+	struct reset_controller_dev rcdev;
+	struct gpio_desc *gpio;
+};
+
+static inline struct reset_slb9670 *
+to_reset_slb9670(struct reset_controller_dev *rcdev)
+{
+	return container_of(rcdev, struct reset_slb9670, rcdev);
+}
+
+static int reset_slb9670_assert(struct reset_controller_dev *rcdev,
+				unsigned long id)
+{
+	struct reset_slb9670 *reset_slb9670 = to_reset_slb9670(rcdev);
+
+	gpiod_set_value(reset_slb9670->gpio, 1);
+	return 0;
+}
+
+static int reset_slb9670_deassert(struct reset_controller_dev *rcdev,
+				  unsigned long id)
+{
+	struct reset_slb9670 *reset_slb9670 = to_reset_slb9670(rcdev);
+
+	/*
+	 * Perform the reset sequence: Deassert and assert the reset line twice
+	 * and wait the respective time intervals. After a last wait interval
+	 * of RSTIN the chip is ready to receive the first command.
+	 */
+	gpiod_set_value(reset_slb9670->gpio, 0);
+	msleep(SLB9670_TIME_RSTIN);
+	gpiod_set_value(reset_slb9670->gpio, 1);
+	udelay(SLB9670_TIME_WRST);
+	gpiod_set_value(reset_slb9670->gpio, 0);
+	msleep(SLB9670_TIME_RSTIN);
+	gpiod_set_value(reset_slb9670->gpio, 1);
+	udelay(SLB9670_TIME_WRST);
+	gpiod_set_value(reset_slb9670->gpio, 0);
+	msleep(SLB9670_TIME_RSTIN);
+
+	return 0;
+}
+
+static int reset_slb9670_reset(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	int ret;
+
+	ret = reset_slb9670_assert(rcdev, id);
+	if (ret)
+		return ret;
+
+	return reset_slb9670_deassert(rcdev, id);
+}
+
+static int reset_slb9670_status(struct reset_controller_dev *rcdev,
+				unsigned long id)
+{
+	struct reset_slb9670 *reset_slb9670 = to_reset_slb9670(rcdev);
+
+	return gpiod_get_value(reset_slb9670->gpio);
+}
+
+static const struct reset_control_ops reset_slb9670_ops = {
+	.assert		= reset_slb9670_assert,
+	.deassert	= reset_slb9670_deassert,
+	.reset		= reset_slb9670_reset,
+	.status		= reset_slb9670_status,
+};
+
+static int reset_slb9670_of_xlate(struct reset_controller_dev *rcdev,
+				  const struct of_phandle_args *reset_spec)
+{
+	return 0;
+}
+
+static int reset_slb9670_probe(struct platform_device *pdev)
+{
+	struct reset_slb9670 *reset_slb9670;
+	struct device *dev = &pdev->dev;
+
+	reset_slb9670 = devm_kzalloc(dev, sizeof(*reset_slb9670), GFP_KERNEL);
+	if (!reset_slb9670)
+		return -ENOMEM;
+
+	reset_slb9670->gpio = devm_gpiod_get(dev, "reset", GPIOD_ASIS);
+	if (IS_ERR(reset_slb9670->gpio))
+		return dev_err_probe(dev, PTR_ERR(reset_slb9670->gpio),
+				     "cannot get reset gpio\n");
+
+	reset_slb9670->rcdev.nr_resets = 1;
+	reset_slb9670->rcdev.owner = THIS_MODULE;
+	reset_slb9670->rcdev.of_node = dev->of_node;
+	reset_slb9670->rcdev.ops = &reset_slb9670_ops;
+	reset_slb9670->rcdev.of_xlate = reset_slb9670_of_xlate;
+	reset_slb9670->rcdev.of_reset_n_cells = 0;
+
+	return devm_reset_controller_register(dev, &reset_slb9670->rcdev);
+}
+
+static const struct of_device_id reset_slb9670_dt_ids[] = {
+	{ .compatible = "infineon,slb9670-reset" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, reset_slb9670_dt_ids);
+
+static struct platform_driver reset_slb9670_driver = {
+	.probe	= reset_slb9670_probe,
+	.driver = {
+		.name		= "reset-slb9670",
+		.of_match_table	= reset_slb9670_dt_ids,
+	},
+};
+module_platform_driver(reset_slb9670_driver);
+
+MODULE_DESCRIPTION("Infineon SLB9670 TPM Reset Driver");
+MODULE_AUTHOR("Lino Sanfilippo <l.sanfilippo@kunbus.com>");
+MODULE_LICENSE("GPL");
-- 
2.40.1

Re: [PATCH 2/2] reset: Add Infineon SLB9670 TPM reset driver

[Francesco Dolcini]

Hello Lino, hello Lukas,

On Tue, Sep 26, 2023 at 09:09:36PM +0200, Lukas Wunner wrote:
> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> 
> Normally the platform firmware is responsible for taking a Trusted
> Platform Module out of reset on boot and storing measurements into it.
> 
> However if the platform firmware is incapable of doing that -- as is the
> case on the Raspberry Pi -- then the onus is on the kernel to take the
> TPM out of reset before trying to attach a driver to it.
> 
> Provide a reset driver for such platforms.
> 
> The Infineon SLB9670 TPM requires a specific reset sequence on its RST#
> pin which is documented in sections 5.4 and 5.5 of the datasheet:
> 
> https://www.infineon.com/dgdl/Infineon-SLB%209670VQ2.0-DataSheet-v01_04-EN.pdf?fileId=5546d4626fc1ce0b016fc78270350cd6
> 
> The sequence with minimum wait intervals is as follows:
> 
>   deassert RST#
>   wait at least 60 ms
>   assert RST#
>   wait at least 2 usecs
>   deassert RST#
>   wait at least 60 ms
>   assert RST#
>   wait at least 2 usecs
>   deassert RST#
>   wait at least 60 ms before issuing the first TPM command

Are you really sure that this change is required?
I have seen the RST# Timing diagram in the datasheet, however I wonder
if a reset is required at all during power-up, for example.

Not to mention that I would have expected some firmware to implement
such reset timing and I was not able to find any (I looked at
arm/arm64), if this is really required I the driver can work at all?
Which platform firmware implements such reset sequence?

Not to mention that I was able to see the driver probe succeed in a
similar setup to the one you are describing in the commit message
(different board, arm64, but nothing done by the platform firmware).

What am I missing?

Thanks,
Francesco

Re: [PATCH 2/2] reset: Add Infineon SLB9670 TPM reset driver

[Francesco Dolcini]

On Wed, Nov 22, 2023 at 12:34:09AM +0100, Francesco Dolcini wrote:
> Not to mention that I would have expected some firmware to implement
> such reset timing and I was not able to find any (I looked at
> arm/arm64), if this is really required I the driver can work at all?
                                        ^^^ 
                     ...really required how the driver...

Francesco

Re: [PATCH 2/2] reset: Add Infineon SLB9670 TPM reset driver

[Lukas Wunner]

On Wed, Nov 22, 2023 at 12:33:58AM +0100, Francesco Dolcini wrote:
> On Tue, Sep 26, 2023 at 09:09:36PM +0200, Lukas Wunner wrote:
> > Normally the platform firmware is responsible for taking a Trusted
> > Platform Module out of reset on boot and storing measurements into it.
> > 
> > However if the platform firmware is incapable of doing that -- as is the
> > case on the Raspberry Pi -- then the onus is on the kernel to take the
> > TPM out of reset before trying to attach a driver to it.
> > 
> > Provide a reset driver for such platforms.
> > 
> > The Infineon SLB9670 TPM requires a specific reset sequence on its RST#
> > pin which is documented in sections 5.4 and 5.5 of the datasheet:
> 
> Are you really sure that this change is required?
> I have seen the RST# Timing diagram in the datasheet, however I wonder
> if a reset is required at all during power-up, for example.

If the RST# pin is not toggled at all upon a warm reset (reboot),
the TPM will remain in whatever state it was during the previous boot.

Also, the pin controller connected to RST# might be reset upon a reboot
(think of a SoC internal pin controller setting all its registers to 0)
and RST# might be asserted as a result.  It is then necessary to take
the TPM out of reset.


> Not to mention that I would have expected some firmware to implement
> such reset timing and I was not able to find any (I looked at
> arm/arm64), if this is really required I the driver can work at all?
> Which platform firmware implements such reset sequence?

I can't answer how a TPM is reset by firmware on arm/arm64, you'd have
to ask an FAE at ARM.  Normally I'd expect firmware in ROM do that so
all subsequently executed code which is mutable (EFI, bootloader, kernel)
can be measured.  Again, on simple platforms such as the Raspberry Pi
there's no support to reset a TPM in ROM.


> Not to mention that I was able to see the driver probe succeed in a
> similar setup to the one you are describing in the commit message
> (different board, arm64, but nothing done by the platform firmware).

Hm, is the RST# pin even connected on that board?

Thanks,

Lukas

Re: [PATCH 2/2] reset: Add Infineon SLB9670 TPM reset driver

[Francesco Dolcini]

Hello Lukas,

On Wed, Nov 22, 2023 at 12:29:49PM +0100, Lukas Wunner wrote:
> On Wed, Nov 22, 2023 at 12:33:58AM +0100, Francesco Dolcini wrote:
> > On Tue, Sep 26, 2023 at 09:09:36PM +0200, Lukas Wunner wrote:
> > > Normally the platform firmware is responsible for taking a Trusted
> > > Platform Module out of reset on boot and storing measurements into it.
> > > 
> > > However if the platform firmware is incapable of doing that -- as is the
> > > case on the Raspberry Pi -- then the onus is on the kernel to take the
> > > TPM out of reset before trying to attach a driver to it.
> > > 
> > > Provide a reset driver for such platforms.
> > > 
> > > The Infineon SLB9670 TPM requires a specific reset sequence on its RST#
> > > pin which is documented in sections 5.4 and 5.5 of the datasheet:
> > 
> > Are you really sure that this change is required?
> > I have seen the RST# Timing diagram in the datasheet, however I wonder
> > if a reset is required at all during power-up, for example.
> 
> If the RST# pin is not toggled at all upon a warm reset (reboot),
> the TPM will remain in whatever state it was during the previous boot.
...
> Also, the pin controller connected to RST# might be reset upon a reboot
> (think of a SoC internal pin controller setting all its registers to 0)
> and RST# might be asserted as a result.  It is then necessary to take
> the TPM out of reset.

Toggled at boot is different from what you are doing here.

> > Not to mention that I was able to see the driver probe succeed in a
> > similar setup to the one you are describing in the commit message
> > (different board, arm64, but nothing done by the platform firmware).
> 
> Hm, is the RST# pin even connected on that board?

Yes, it's connected and it is asserted/de-asserted (aka toggled) during
startup from the HW reset circuit. However this is not implementing the
reset sequence you are implementing here.

Francesco

Re: [PATCH 2/2] reset: Add Infineon SLB9670 TPM reset driver

[Lukas Wunner]

On Wed, Nov 22, 2023 at 04:15:18PM +0100, Francesco Dolcini wrote:
> On Wed, Nov 22, 2023 at 12:29:49PM +0100, Lukas Wunner wrote:
> > On Wed, Nov 22, 2023 at 12:33:58AM +0100, Francesco Dolcini wrote:
> > > Not to mention that I was able to see the driver probe succeed in a
> > > similar setup to the one you are describing in the commit message
> > > (different board, arm64, but nothing done by the platform firmware).
> > 
> > Hm, is the RST# pin even connected on that board?
> 
> Yes, it's connected and it is asserted/de-asserted (aka toggled) during
> startup from the HW reset circuit. However this is not implementing the
> reset sequence you are implementing here.

Section 4.5 of the datasheet seems to indicate that unless the sequence
in Figure 3 is observed, the TPM may enter a defense mode against
dictionary attacks "from which a recovery is very complex or even not
possible."

Simply toggling the RST# pin might therefore not be sufficient to ensure
the TPM is operable.

Here's the relevant section in the datasheet:

   "The OPTIGA TPM SLB 9670 features a sophisticated protection mechanism
    against dictionary attacks on TPM-based authorization data. Basically,
    the device counts the number of failed authorization attempts in a
    counter which is located in the non-volatile memory. An attacker who
    has physical access to the device could try to cirumvent that mechanism
    by resetting the device after the authorization attempt but before the
    updated failure counter has been written into the NVM.

    Certain countermeasures have been added to the OPTIGA TPM SLB 9670.
    In certain time windows during power-on or warm boot of the device,
    such reset events might influence the dictionary attack counters and
    trigger other security mechanisms as well. In worst case, this might
    trigger special security defense modes from which a recovery is very
    complex or even not possible.

    To avoid that the OPTIGA TPM SLB 9670 reaches such a security defense
    state, the RST# signal must not be asserted in certain time windows.
    After the deassertion of the RST# signal, the system should wait for
    a minimum time of tRSTIN before asserting RST# again (see Figure 3
    and Table 11).

    TPM commands should only be started after tRSTIN has expired (see
    Figure 3 again). If a TPM command is running, RST# should not be
    asserted; otherwise, this might also trigger some security functions.
    When the TPM shall be reset, the command TPM2_Shutdown should be
    issued before the assertion of the RST# signal.

    https://www.infineon.com/dgdl/?fileId=5546d4626fc1ce0b016fc78270350cd6

Thanks,

Lukas

Re: [PATCH 2/2] reset: Add Infineon SLB9670 TPM reset driver

[Francesco Dolcini]

Hello Lukas,

On Thu, Nov 23, 2023 at 09:59:43AM +0100, Lukas Wunner wrote:
> On Wed, Nov 22, 2023 at 04:15:18PM +0100, Francesco Dolcini wrote:
> > On Wed, Nov 22, 2023 at 12:29:49PM +0100, Lukas Wunner wrote:
> > > On Wed, Nov 22, 2023 at 12:33:58AM +0100, Francesco Dolcini wrote:
> > > > Not to mention that I was able to see the driver probe succeed in a
> > > > similar setup to the one you are describing in the commit message
> > > > (different board, arm64, but nothing done by the platform firmware).
> > > 
> > > Hm, is the RST# pin even connected on that board?
> > 
> > Yes, it's connected and it is asserted/de-asserted (aka toggled) during
> > startup from the HW reset circuit. However this is not implementing the
> > reset sequence you are implementing here.
> 
> Section 4.5 of the datasheet seems to indicate that unless the sequence
> in Figure 3 is observed, the TPM may enter a defense mode against
> dictionary attacks "from which a recovery is very complex or even not
> possible."
> 
> Simply toggling the RST# pin might therefore not be sufficient to ensure
> the TPM is operable.

I am trying to follow-up with infineon on this regard, do you already
have any insight from them maybe?

Maybe this procedure is relevant only when the device is in "security
defense state"?

Francesco

Re: [PATCH 2/2] reset: Add Infineon SLB9670 TPM reset driver

[Lukas Wunner]

Hi Francesco,

On Mon, Dec 18, 2023 at 06:34:00PM +0100, Francesco Dolcini wrote:
> On Thu, Nov 23, 2023 at 09:59:43AM +0100, Lukas Wunner wrote:
> > On Wed, Nov 22, 2023 at 04:15:18PM +0100, Francesco Dolcini wrote:
> > > On Wed, Nov 22, 2023 at 12:29:49PM +0100, Lukas Wunner wrote:
> > > > On Wed, Nov 22, 2023 at 12:33:58AM +0100, Francesco Dolcini wrote:
> > > > > Not to mention that I was able to see the driver probe succeed in a
> > > > > similar setup to the one you are describing in the commit message
> > > > > (different board, arm64, but nothing done by the platform firmware).
> > > > 
> > > > Hm, is the RST# pin even connected on that board?
> > > 
> > > Yes, it's connected and it is asserted/de-asserted (aka toggled) during
> > > startup from the HW reset circuit. However this is not implementing the
> > > reset sequence you are implementing here.
> > 
> > Section 4.5 of the datasheet seems to indicate that unless the sequence
> > in Figure 3 is observed, the TPM may enter a defense mode against
> > dictionary attacks "from which a recovery is very complex or even not
> > possible."
> > 
> > Simply toggling the RST# pin might therefore not be sufficient to ensure
> > the TPM is operable.
> 
> I am trying to follow-up with infineon on this regard, do you already
> have any insight from them maybe?
> 
> Maybe this procedure is relevant only when the device is in "security
> defense state"?

Sorry, I honestly don't know.  A colleague has talked to an FAE at an
Infineon reseller but they couldn't give a definitive answer either.
I'm very interested to hear whatever you learn from Infineon.

Thanks,

Lukas

Re: [PATCH 2/2] reset: Add Infineon SLB9670 TPM reset driver

[Alexander Steffen]

On 18.12.2023 18:51, Lukas Wunner wrote:
> Hi Francesco,
> 
> On Mon, Dec 18, 2023 at 06:34:00PM +0100, Francesco Dolcini wrote:
>> On Thu, Nov 23, 2023 at 09:59:43AM +0100, Lukas Wunner wrote:
>>> On Wed, Nov 22, 2023 at 04:15:18PM +0100, Francesco Dolcini wrote:
>>>> On Wed, Nov 22, 2023 at 12:29:49PM +0100, Lukas Wunner wrote:
>>>>> On Wed, Nov 22, 2023 at 12:33:58AM +0100, Francesco Dolcini wrote:
>>>>>> Not to mention that I was able to see the driver probe succeed in a
>>>>>> similar setup to the one you are describing in the commit message
>>>>>> (different board, arm64, but nothing done by the platform firmware).
>>>>>
>>>>> Hm, is the RST# pin even connected on that board?
>>>>
>>>> Yes, it's connected and it is asserted/de-asserted (aka toggled) during
>>>> startup from the HW reset circuit. However this is not implementing the
>>>> reset sequence you are implementing here.
>>>
>>> Section 4.5 of the datasheet seems to indicate that unless the sequence
>>> in Figure 3 is observed, the TPM may enter a defense mode against
>>> dictionary attacks "from which a recovery is very complex or even not
>>> possible."
>>>
>>> Simply toggling the RST# pin might therefore not be sufficient to ensure
>>> the TPM is operable.
>>
>> I am trying to follow-up with infineon on this regard, do you already
>> have any insight from them maybe?
>>
>> Maybe this procedure is relevant only when the device is in "security
>> defense state"?
> 
> Sorry, I honestly don't know.  A colleague has talked to an FAE at an
> Infineon reseller but they couldn't give a definitive answer either.
> I'm very interested to hear whatever you learn from Infineon.

Infineon is here :)

I'm sorry, the document is a little confusing, we'll fix that in the 
future. What the document wants to say is this: Any time you assert 
RST#, the TPM will reset. But if you reset the TPM at certain points in 
time, you will trigger some security functions. In general, as long as 
it only happens occasionally, this is not a problem (you can't avoid all 
power outages). Only if you happen to frequently issue resets (e.g. if 
your reset pin is not a dedicated TPM reset pin but is also used for 
other things), then you should make sure to wait at least t_RSTIN 
between those resets (and avoid interrupting TPM command execution).

So in your case, you probably don't need to do anything special: Just 
assert RST# once and the TPM will reset. This should work with basically 
any TPM, so there is no need for a dedicated SLB9670 reset driver.

Alexander

Re: [PATCH 1/2] dt-bindings: reset: Add Infineon SLB9670 TPM reset driver

[Rob Herring]

On Tue, 26 Sep 2023 21:09:35 +0200, Lukas Wunner wrote:
> A new reset driver is about to be added to perform the reset sequence of
> the Infineon SLB9670 Trusted Platform Module.
> 
> Document its device tree bindings.
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  .../bindings/reset/infineon,slb9670-reset.yaml     | 68 ++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/infineon,slb9670-reset.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/reset/infineon,slb9670-reset.yaml:29:111: [warning] line too long (124 > 110 characters) (line-length)

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/reset/infineon,slb9670-reset.example.dtb: /example-0/spi/tpm@0: failed to match any schema with compatible: ['infineon,slb9670']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/ae40859b82494d75e9ad7bf616b3264138ad1f6a.1695754856.git.lukas@wunner.de

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 1/2] dt-bindings: reset: Add Infineon SLB9670 TPM reset driver

[Lukas Wunner]

On Tue, Sep 26, 2023 at 03:37:07PM -0500, Rob Herring wrote:
> On Tue, 26 Sep 2023 21:09:35 +0200, Lukas Wunner wrote:
> > A new reset driver is about to be added to perform the reset sequence of
> > the Infineon SLB9670 Trusted Platform Module.
> > 
> > Document its device tree bindings.
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> ./Documentation/devicetree/bindings/reset/infineon,slb9670-reset.yaml:29:111: [warning] line too long (124 > 110 characters) (line-length)

That's the following line:

  [1] https://www.infineon.com/dgdl/Infineon-SLB%209670VQ2.0-DataSheet-v01_04-EN.pdf?fileId=5546d4626fc1ce0b016fc78270350cd6

I'm not sure what to do about it.  Use an URL shortener?

I did see the warning when running static checks before submission,
but it seemed like a false positive to me.


> dtschema/dtc warnings/errors:
> Documentation/devicetree/bindings/reset/infineon,slb9670-reset.example.dtb: /example-0/spi/tpm@0: failed to match any schema with compatible: ['infineon,slb9670']

The TPM DT bindings in Documentation/devicetree/bindings/security/tpm/
haven't been converted to YAML yet, hence the warning/error.

Is it a prerequisite that I consolidate and convert them before
this patch is acceptable?

Thanks,

Lukas

Re: [PATCH 1/2] dt-bindings: reset: Add Infineon SLB9670 TPM reset driver

[Rob Herring]

On Wed, Sep 27, 2023 at 08:31:16AM +0200, Lukas Wunner wrote:
> On Tue, Sep 26, 2023 at 03:37:07PM -0500, Rob Herring wrote:
> > On Tue, 26 Sep 2023 21:09:35 +0200, Lukas Wunner wrote:
> > > A new reset driver is about to be added to perform the reset sequence of
> > > the Infineon SLB9670 Trusted Platform Module.
> > > 
> > > Document its device tree bindings.
> > 
> > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> > on your patch (DT_CHECKER_FLAGS is new in v5.13):
> > 
> > yamllint warnings/errors:
> > ./Documentation/devicetree/bindings/reset/infineon,slb9670-reset.yaml:29:111: [warning] line too long (124 > 110 characters) (line-length)
> 
> That's the following line:
> 
>   [1] https://www.infineon.com/dgdl/Infineon-SLB%209670VQ2.0-DataSheet-v01_04-EN.pdf?fileId=5546d4626fc1ce0b016fc78270350cd6
> 
> I'm not sure what to do about it.  Use an URL shortener?

The link doesn't even work for me.

> I did see the warning when running static checks before submission,
> but it seemed like a false positive to me.

I suppose we could bump the limit by 1 as I think it is set to 110.

> 
> > dtschema/dtc warnings/errors:
> > Documentation/devicetree/bindings/reset/infineon,slb9670-reset.example.dtb: /example-0/spi/tpm@0: failed to match any schema with compatible: ['infineon,slb9670']
> 
> The TPM DT bindings in Documentation/devicetree/bindings/security/tpm/
> haven't been converted to YAML yet, hence the warning/error.

Yes, there's been multiple attempts. Everyone disappears when I say work 
together and consolidate the efforts.

> Is it a prerequisite that I consolidate and convert them before
> this patch is acceptable?

Yes. Can't have warnings.

Rob

Re: [PATCH 1/2] dt-bindings: reset: Add Infineon SLB9670 TPM reset driver

[Krzysztof Kozlowski]

On 26/09/2023 21:09, Lukas Wunner wrote:
> A new reset driver is about to be added to perform the reset sequence of
> the Infineon SLB9670 Trusted Platform Module.
> 
> Document its device tree bindings.
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  .../bindings/reset/infineon,slb9670-reset.yaml     | 68 ++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/infineon,slb9670-reset.yaml
> 
> diff --git a/Documentation/devicetree/bindings/reset/infineon,slb9670-reset.yaml b/Documentation/devicetree/bindings/reset/infineon,slb9670-reset.yaml
> new file mode 100644
> index 00000000..b1e23d47
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/infineon,slb9670-reset.yaml
> @@ -0,0 +1,68 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/reset/infineon,slb9670-reset.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Infineon SLB9670 TPM Reset Driver

Sorry, bindings are for hardware, not for drivers.

> +
> +maintainers:
> +  - Lukas Wunner <lukas@wunner.de>
> +
> +description: |
> +  The Infineon SLB9670 Trusted Platform Module requires a specific reset
> +  sequence on its RST# pin which is documented in sections 5.4 and 5.5 of
> +  the datasheet [1]. This driver performs the reset sequence using a GPIO.

Sorry, bindings are for hardware, not for drivers. I don't see the point
for this binding, especially that you refer to drivers. Why it cannot be
just part of other device?


> +
> +  The sequence with minimum wait intervals is as follows:
> +  deassert RST#
> +  wait at least 60 ms
> +  assert RST#
> +  wait at least 2 usecs
> +  deassert RST#
> +  wait at least 60 ms
> +  assert RST#
> +  wait at least 2 usecs
> +  deassert RST#
> +  wait at least 60 ms before issuing the first TPM command
> +
> +  [1] https://www.infineon.com/dgdl/Infineon-SLB%209670VQ2.0-DataSheet-v01_04-EN.pdf?fileId=5546d4626fc1ce0b016fc78270350cd6
> +
> +properties:
> +  compatible:
> +    enum:
> +      - infineon,slb9670-reset
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description: Reference to the GPIO connected to the RST# pin.
> +
> +  "#reset-cells":
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reset-gpios
> +  - "#reset-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    tpm_reset: reset-controller {

No need for label.

> +        compatible = "infineon,slb9670-reset";
> +        #reset-cells = <0>;
> +        reset-gpios = <&gpio 0 GPIO_ACTIVE_LOW>;
> +    };
> +
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        tpm@0 {
> +            compatible = "infineon,slb9670";

Drop node, not related to the binding. We never keep consumers in
binding providers.

> +            reg = <0>;
> +            resets = <&tpm_reset>;
> +        };
> +    };

Best regards,
Krzysztof