Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1590287 > unrolled thread

[PATCH] tpm: do not suspend/resume if power stays on

Back to article view | Back to linux.kernel

Contents

  [PATCH] tpm: do not suspend/resume if power stays on Enric Balletbo i Serra <enric.balletbo@collabora.com> - 2017-03-01 13:00 +0100
    Re: [PATCH] tpm: do not suspend/resume if power stays on Peter Huewe <peterhuewe@gmx.de> - 2017-03-01 13:10 +0100
      Re: [PATCH] tpm: do not suspend/resume if power stays on Enric Balletbo i Serra <enric.balletbo@collabora.com> - 2017-03-01 13:20 +0100
    Re: [PATCH] tpm: do not suspend/resume if power stays on Mark Rutland <mark.rutland@arm.com> - 2017-03-01 15:00 +0100
      Re: [tpmdd-devel] [PATCH] tpm: do not suspend/resume if power stays  on Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-03-01 20:50 +0100
        Re: [tpmdd-devel] [PATCH] tpm: do not suspend/resume if power stays on Sonny Rao <sonnyrao@chromium.org> - 2017-03-01 23:40 +0100
          Re: [tpmdd-devel] [PATCH] tpm: do not suspend/resume if power stays on Sonny Rao <sonnyrao@chromium.org> - 2017-03-02 01:10 +0100
          Re: [tpmdd-devel] [PATCH] tpm: do not suspend/resume if power stays  on Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-03-02 01:50 +0100

#1590287 — [PATCH] tpm: do not suspend/resume if power stays on

From: Sonny Rao <sonnyrao@chromium.org>

The suspend/resume behavior of the TPM can be controlled
by setting "powered-while-suspended" in the DTS.

Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
 Documentation/devicetree/bindings/tpm/tpm.txt | 25 +++++++++++++++++++++++++
 drivers/char/tpm/tpm_i2c_infineon.c           | 25 ++++++++++++++++++++++++-
 2 files changed, 49 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/tpm/tpm.txt

diff --git a/Documentation/devicetree/bindings/tpm/tpm.txt b/Documentation/devicetree/bindings/tpm/tpm.txt
new file mode 100644
index 0000000..af4de0d
--- /dev/null
+++ b/Documentation/devicetree/bindings/tpm/tpm.txt
@@ -0,0 +1,25 @@
+TPM (Trusted Platform Module)
+
+A TPM on the I2C bus is a child of the node for the bus.
+
+Required properties:
+- compatible: should be "infineon,<chip>"
+- reg: the I2C address
+
+Optional properties:
+- powered-while-suspended: present when the TPM is left powered on between
+  suspend and resume (makes the suspend/resume callbacks do nothing).
+
+Example:
+	i2c@12C90000 {
+		samsung,i2c-sda-delay = <100>;
+		samsung,i2c-max-bus-freq = <66000>;
+		gpios = <&gpa1 2 3 3 0>,
+			<&gpa1 3 3 3 0>;
+
+		tpm {
+			compatible = "infineon,slb9635tt";
+			reg = <0x20>;
+			powered-while-suspended;
+		};
+	};
diff --git a/drivers/char/tpm/tpm_i2c_infineon.c b/drivers/char/tpm/tpm_i2c_infineon.c
index 62ee44e..19d9522 100644
--- a/drivers/char/tpm/tpm_i2c_infineon.c
+++ b/drivers/char/tpm/tpm_i2c_infineon.c
@@ -70,6 +70,7 @@ struct tpm_inf_dev {
 	u8 buf[TPM_BUFSIZE + sizeof(u8)]; /* max. buffer size + addr */
 	struct tpm_chip *chip;
 	enum i2c_chip_type chip_type;
+	bool powered_while_suspended;
 };
 
 static struct tpm_inf_dev tpm_dev;
@@ -599,6 +600,11 @@ static int tpm_tis_i2c_init(struct device *dev)
 		goto out_err;
 	}
 
+	if (dev->of_node &&
+	    of_get_property(dev->of_node, "powered-while-suspended", NULL)) {
+		tpm_dev.powered_while_suspended = true;
+	}
+
 	/* read four bytes from DID_VID register */
 	if (iic_tpm_read(TPM_DID_VID(0), (u8 *)&vendor, 4) < 0) {
 		dev_err(dev, "could not read vendor id\n");
@@ -662,7 +668,24 @@ static const struct of_device_id tpm_tis_i2c_of_match[] = {
 MODULE_DEVICE_TABLE(of, tpm_tis_i2c_of_match);
 #endif
 
-static SIMPLE_DEV_PM_OPS(tpm_tis_i2c_ops, tpm_pm_suspend, tpm_pm_resume);
+static int __maybe_unused tpm_tis_i2c_suspend(struct device *dev)
+{
+	if (tpm_dev.powered_while_suspended)
+		return 0;
+
+	return tpm_pm_suspend(dev);
+}
+
+static int __maybe_unused tpm_tis_i2c_resume(struct device *dev)
+{
+	if (tpm_dev.powered_while_suspended)
+		return 0;
+
+	return tpm_pm_resume(dev);
+}
+
+static SIMPLE_DEV_PM_OPS(tpm_tis_i2c_ops, tpm_tis_i2c_suspend,
+			 tpm_tis_i2c_resume);
 
 static int tpm_tis_i2c_probe(struct i2c_client *client,
 			     const struct i2c_device_id *id)
-- 
2.9.3

[toc] | [next] | [standalone]

#1590294

Am 1. März 2017 12:51:16 MEZ schrieb Enric Balletbo i Serra <enric.balletbo@collabora.com>:
>From: Sonny Rao <sonnyrao@chromium.org>
>
>The suspend/resume behavior of the TPM can be controlled
>by setting "powered-while-suspended" in the DTS.
>
>Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>---
>Documentation/devicetree/bindings/tpm/tpm.txt | 25
>+++++++++++++++++++++++++
>drivers/char/tpm/tpm_i2c_infineon.c           | 25
>++++++++++++++++++++++++-
> 2 files changed, 49 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/devicetree/bindings/tpm/tpm.txt
>
>diff --git a/Documentation/devicetree/bindings/tpm/tpm.txt
>b/Documentation/devicetree/bindings/tpm/tpm.txt
>new file mode 100644
>index 0000000..af4de0d
>--- /dev/null
>+++ b/Documentation/devicetree/bindings/tpm/tpm.txt

Hi, for this device there exists a binding in the i2c subfolder

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/i2c/trivial-devices.txt?id=refs/tags/v4.10

Peter
>@@ -0,0 +1,25 @@
>+TPM (Trusted Platform Module)
>+
>+A TPM on the I2C bus is a child of the node for the bus.
>+
>+Required properties:
>+- compatible: should be "infineon,<chip>"
>+- reg: the I2C address
>+
>+Optional properties:
>+- powered-while-suspended: present when the TPM is left powered on
>between
>+  suspend and resume (makes the suspend/resume callbacks do nothing).
>+
>+Example:
>+	i2c@12C90000 {
>+		samsung,i2c-sda-delay = <100>;
>+		samsung,i2c-max-bus-freq = <66000>;
>+		gpios = <&gpa1 2 3 3 0>,
>+			<&gpa1 3 3 3 0>;
>+
>+		tpm {
>+			compatible = "infineon,slb9635tt";
>+			reg = <0x20>;
>+			powered-while-suspended;
>+		};
>+	};
>diff --git a/drivers/char/tpm/tpm_i2c_infineon.c
>b/drivers/char/tpm/tpm_i2c_infineon.c
>index 62ee44e..19d9522 100644
>--- a/drivers/char/tpm/tpm_i2c_infineon.c
>+++ b/drivers/char/tpm/tpm_i2c_infineon.c
>@@ -70,6 +70,7 @@ struct tpm_inf_dev {
> 	u8 buf[TPM_BUFSIZE + sizeof(u8)]; /* max. buffer size + addr */
> 	struct tpm_chip *chip;
> 	enum i2c_chip_type chip_type;
>+	bool powered_while_suspended;
> };
> 
> static struct tpm_inf_dev tpm_dev;
>@@ -599,6 +600,11 @@ static int tpm_tis_i2c_init(struct device *dev)
> 		goto out_err;
> 	}
> 
>+	if (dev->of_node &&
>+	    of_get_property(dev->of_node, "powered-while-suspended", NULL)) {
>+		tpm_dev.powered_while_suspended = true;
>+	}
>+
> 	/* read four bytes from DID_VID register */
> 	if (iic_tpm_read(TPM_DID_VID(0), (u8 *)&vendor, 4) < 0) {
> 		dev_err(dev, "could not read vendor id\n");
>@@ -662,7 +668,24 @@ static const struct of_device_id
>tpm_tis_i2c_of_match[] = {
> MODULE_DEVICE_TABLE(of, tpm_tis_i2c_of_match);
> #endif
> 
>-static SIMPLE_DEV_PM_OPS(tpm_tis_i2c_ops, tpm_pm_suspend,
>tpm_pm_resume);
>+static int __maybe_unused tpm_tis_i2c_suspend(struct device *dev)
>+{
>+	if (tpm_dev.powered_while_suspended)
>+		return 0;
>+
>+	return tpm_pm_suspend(dev);
>+}
>+
>+static int __maybe_unused tpm_tis_i2c_resume(struct device *dev)
>+{
>+	if (tpm_dev.powered_while_suspended)
>+		return 0;
>+
>+	return tpm_pm_resume(dev);
>+}
>+
>+static SIMPLE_DEV_PM_OPS(tpm_tis_i2c_ops, tpm_tis_i2c_suspend,
>+			 tpm_tis_i2c_resume);
> 
> static int tpm_tis_i2c_probe(struct i2c_client *client,
> 			     const struct i2c_device_id *id)

-- 
Sent from my mobile

[toc] | [prev] | [next] | [standalone]

#1590295

Hi Peter,

On 01/03/17 13:00, Peter Huewe wrote:
> 
> 
> Am 1. März 2017 12:51:16 MEZ schrieb Enric Balletbo i Serra <enric.balletbo@collabora.com>:
>> From: Sonny Rao <sonnyrao@chromium.org>
>>
>> The suspend/resume behavior of the TPM can be controlled
>> by setting "powered-while-suspended" in the DTS.
>>
>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ---
>> Documentation/devicetree/bindings/tpm/tpm.txt | 25
>> +++++++++++++++++++++++++
>> drivers/char/tpm/tpm_i2c_infineon.c           | 25
>> ++++++++++++++++++++++++-
>> 2 files changed, 49 insertions(+), 1 deletion(-)
>> create mode 100644 Documentation/devicetree/bindings/tpm/tpm.txt
>>
>> diff --git a/Documentation/devicetree/bindings/tpm/tpm.txt
>> b/Documentation/devicetree/bindings/tpm/tpm.txt
>> new file mode 100644
>> index 0000000..af4de0d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/tpm/tpm.txt
> 
> Hi, for this device there exists a binding in the i2c subfolder
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/i2c/trivial-devices.txt?id=refs/tags/v4.10
> 

Thanks to catch this, I propose remove from trivial-devices.txt and create a new binding called Documentation/devicetree/bindings/i2c/i2c-tpm-infineon.txt for tpm-infineon devices? What do you think?

Thanks,
 Enric

> Peter
>> @@ -0,0 +1,25 @@
>> +TPM (Trusted Platform Module)
>> +
>> +A TPM on the I2C bus is a child of the node for the bus.
>> +
>> +Required properties:
>> +- compatible: should be "infineon,<chip>"
>> +- reg: the I2C address
>> +
>> +Optional properties:
>> +- powered-while-suspended: present when the TPM is left powered on
>> between
>> +  suspend and resume (makes the suspend/resume callbacks do nothing).
>> +
>> +Example:
>> +	i2c@12C90000 {
>> +		samsung,i2c-sda-delay = <100>;
>> +		samsung,i2c-max-bus-freq = <66000>;
>> +		gpios = <&gpa1 2 3 3 0>,
>> +			<&gpa1 3 3 3 0>;
>> +
>> +		tpm {
>> +			compatible = "infineon,slb9635tt";
>> +			reg = <0x20>;
>> +			powered-while-suspended;
>> +		};
>> +	};
>> diff --git a/drivers/char/tpm/tpm_i2c_infineon.c
>> b/drivers/char/tpm/tpm_i2c_infineon.c
>> index 62ee44e..19d9522 100644
>> --- a/drivers/char/tpm/tpm_i2c_infineon.c
>> +++ b/drivers/char/tpm/tpm_i2c_infineon.c
>> @@ -70,6 +70,7 @@ struct tpm_inf_dev {
>> 	u8 buf[TPM_BUFSIZE + sizeof(u8)]; /* max. buffer size + addr */
>> 	struct tpm_chip *chip;
>> 	enum i2c_chip_type chip_type;
>> +	bool powered_while_suspended;
>> };
>>
>> static struct tpm_inf_dev tpm_dev;
>> @@ -599,6 +600,11 @@ static int tpm_tis_i2c_init(struct device *dev)
>> 		goto out_err;
>> 	}
>>
>> +	if (dev->of_node &&
>> +	    of_get_property(dev->of_node, "powered-while-suspended", NULL)) {
>> +		tpm_dev.powered_while_suspended = true;
>> +	}
>> +
>> 	/* read four bytes from DID_VID register */
>> 	if (iic_tpm_read(TPM_DID_VID(0), (u8 *)&vendor, 4) < 0) {
>> 		dev_err(dev, "could not read vendor id\n");
>> @@ -662,7 +668,24 @@ static const struct of_device_id
>> tpm_tis_i2c_of_match[] = {
>> MODULE_DEVICE_TABLE(of, tpm_tis_i2c_of_match);
>> #endif
>>
>> -static SIMPLE_DEV_PM_OPS(tpm_tis_i2c_ops, tpm_pm_suspend,
>> tpm_pm_resume);
>> +static int __maybe_unused tpm_tis_i2c_suspend(struct device *dev)
>> +{
>> +	if (tpm_dev.powered_while_suspended)
>> +		return 0;
>> +
>> +	return tpm_pm_suspend(dev);
>> +}
>> +
>> +static int __maybe_unused tpm_tis_i2c_resume(struct device *dev)
>> +{
>> +	if (tpm_dev.powered_while_suspended)
>> +		return 0;
>> +
>> +	return tpm_pm_resume(dev);
>> +}
>> +
>> +static SIMPLE_DEV_PM_OPS(tpm_tis_i2c_ops, tpm_tis_i2c_suspend,
>> +			 tpm_tis_i2c_resume);
>>
>> static int tpm_tis_i2c_probe(struct i2c_client *client,
>> 			     const struct i2c_device_id *id)
> 

[toc] | [prev] | [next] | [standalone]

#1590336

On Wed, Mar 01, 2017 at 12:51:16PM +0100, Enric Balletbo i Serra wrote:
> From: Sonny Rao <sonnyrao@chromium.org>
> 
> The suspend/resume behavior of the TPM can be controlled
> by setting "powered-while-suspended" in the DTS.
> 
> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
>  Documentation/devicetree/bindings/tpm/tpm.txt | 25 +++++++++++++++++++++++++
>  drivers/char/tpm/tpm_i2c_infineon.c           | 25 ++++++++++++++++++++++++-
>  2 files changed, 49 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/tpm/tpm.txt
> 
> diff --git a/Documentation/devicetree/bindings/tpm/tpm.txt b/Documentation/devicetree/bindings/tpm/tpm.txt
> new file mode 100644
> index 0000000..af4de0d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/tpm/tpm.txt
> @@ -0,0 +1,25 @@
> +TPM (Trusted Platform Module)
> +
> +A TPM on the I2C bus is a child of the node for the bus.
> +
> +Required properties:
> +- compatible: should be "infineon,<chip>"
> +- reg: the I2C address
> +
> +Optional properties:
> +- powered-while-suspended: present when the TPM is left powered on between
> +  suspend and resume (makes the suspend/resume callbacks do nothing).

This reads like configuration rather than a HW property.

Why do you want this property?

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1590602 — Re: [tpmdd-devel] [PATCH] tpm: do not suspend/resume if power stays on

> > +Optional properties:
> > +- powered-while-suspended: present when the TPM is left powered on between
> > +  suspend and resume (makes the suspend/resume callbacks do nothing).
> 
> This reads like configuration rather than a HW property.

I read this to mean the HW does not cut power to the TPM when Linux
does 'suspend'.

We recently added global suspend/resume callbacks to the TPM
core. Those call backs do not power off the TPM, they just prepare its
internal state to loose power to the chip. Skipping that process on
hardware that does not power-off the TPM makes sense to me.

But, Sonny, perhaps this should be a global flag in tpm_chip, not a
per-interface-driver override?

Jason

[toc] | [prev] | [next] | [standalone]

#1590666 — Re: [tpmdd-devel] [PATCH] tpm: do not suspend/resume if power stays on

On Wed, Mar 1, 2017 at 10:43 AM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
>> > +Optional properties:
>> > +- powered-while-suspended: present when the TPM is left powered on between
>> > +  suspend and resume (makes the suspend/resume callbacks do nothing).
>>
>> This reads like configuration rather than a HW property.
>
> I read this to mean the HW does not cut power to the TPM when Linux
> does 'suspend'.

That's correct, it is a hardware property describing whether power is
removed during suspend.

>
> We recently added global suspend/resume callbacks to the TPM
> core. Those call backs do not power off the TPM, they just prepare its
> internal state to loose power to the chip. Skipping that process on
> hardware that does not power-off the TPM makes sense to me.
>
> But, Sonny, perhaps this should be a global flag in tpm_chip, not a
> per-interface-driver override?

It's a property of the board design not the chip -- maybe I'm misunderstanding?

>
> Jason

[toc] | [prev] | [next] | [standalone]

#1590708 — Re: [tpmdd-devel] [PATCH] tpm: do not suspend/resume if power stays on

On Wed, Mar 1, 2017 at 3:18 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Wed, Mar 01, 2017 at 02:39:09PM -0800, Sonny Rao wrote:
>
>> > We recently added global suspend/resume callbacks to the TPM
>> > core. Those call backs do not power off the TPM, they just prepare its
>> > internal state to loose power to the chip. Skipping that process on
>> > hardware that does not power-off the TPM makes sense to me.
>> >
>> > But, Sonny, perhaps this should be a global flag in tpm_chip, not a
>> > per-interface-driver override?
>>
>> It's a property of the board design not the chip -- maybe I'm
>> misunderstanding?
>
> I mean do not add the code to handle this to tpm_i2c_infineon.c but in
> the common chip code instead.
>
> tpm_i2c_infineon.c should only parse DT properties that are relavent
> to the bus that delivers commands to the TPM, things that apply to how
> a TPM chip operates should be handled in the core code because they
> apply to any command transport bus.

Oh right, sorry -- yes this makes perfect sense.

>
> Jason

[toc] | [prev] | [next] | [standalone]

#1590747 — Re: [tpmdd-devel] [PATCH] tpm: do not suspend/resume if power stays on

On Wed, Mar 01, 2017 at 02:39:09PM -0800, Sonny Rao wrote:

> > We recently added global suspend/resume callbacks to the TPM
> > core. Those call backs do not power off the TPM, they just prepare its
> > internal state to loose power to the chip. Skipping that process on
> > hardware that does not power-off the TPM makes sense to me.
> >
> > But, Sonny, perhaps this should be a global flag in tpm_chip, not a
> > per-interface-driver override?
> 
> It's a property of the board design not the chip -- maybe I'm
> misunderstanding?

I mean do not add the code to handle this to tpm_i2c_infineon.c but in
the common chip code instead.

tpm_i2c_infineon.c should only parse DT properties that are relavent
to the bus that delivers commands to the TPM, things that apply to how
a TPM chip operates should be handled in the core code because they
apply to any command transport bus.

Jason

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web