| Message ID | 20170303160959.27422-1-enric.balletbo@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 03, 2017 at 05:09:59PM +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. This is useful for the cases > when hardware does not power-off the TPM. > > Signed-off-by: Sonny Rao <sonnyrao@chromium.org> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > +++ b/drivers/char/tpm/tpm_of.c > @@ -34,6 +34,9 @@ int tpm_read_log_of(struct tpm_chip *chip) > else > return -ENODEV; > > + if (of_property_read_bool(np, "powered-while-suspended")) > + chip->flags |= TPM_CHIP_FLAG_ALWAYS_POWERED; > + It is really weird that this is in a function called tpm_read_log_of, but it makes sense to use the existing conditional infrastructure for OF as well. We should try to tidy this later... Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
On Fri, Mar 03, 2017 at 05:09:59PM +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. This is useful for the cases > when hardware does not power-off the TPM. > > Signed-off-by: Sonny Rao <sonnyrao@chromium.org> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> Thanks. Looks good to me. Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> /Jarkko > --- > Changes since v2: > Jarkko Sakkinen > - Add a new TPM_CHIP_FLAG_ALWAYS_POWERED flag instead of using a boolean variable. > - Remove a trailing newline. > Changes since v1: > Jason Gunthorpe : > - Move the code to handle suspend/resume in the common chip code. > > Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt | 6 ++++++ > drivers/char/tpm/tpm-interface.c | 3 +++ > drivers/char/tpm/tpm.h | 1 + > drivers/char/tpm/tpm_of.c | 3 +++ > 4 files changed, 13 insertions(+) > > diff --git a/Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt b/Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt > index 8cb638b..85c8216 100644 > --- a/Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt > +++ b/Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt > @@ -8,6 +8,12 @@ Required properties: > the firmware event log > - linux,sml-size : size of the memory allocated for the firmware event log > > +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 (for OpenPower Systems with Nuvoton TPM 2.0 on I2C) > ---------------------------------------------------------- > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c > index a2688ac..6869093 100644 > --- a/drivers/char/tpm/tpm-interface.c > +++ b/drivers/char/tpm/tpm-interface.c > @@ -944,6 +944,9 @@ int tpm_pm_suspend(struct device *dev) > if (chip == NULL) > return -ENODEV; > > + if (chip->flags & TPM_CHIP_FLAG_ALWAYS_POWERED) > + return 0; > + > if (chip->flags & TPM_CHIP_FLAG_TPM2) { > tpm2_shutdown(chip, TPM2_SU_STATE); > return 0; > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > index 1ae9768..0ce379c 100644 > --- a/drivers/char/tpm/tpm.h > +++ b/drivers/char/tpm/tpm.h > @@ -146,6 +146,7 @@ enum tpm_chip_flags { > TPM_CHIP_FLAG_IRQ = BIT(2), > TPM_CHIP_FLAG_VIRTUAL = BIT(3), > TPM_CHIP_FLAG_HAVE_TIMEOUTS = BIT(4), > + TPM_CHIP_FLAG_ALWAYS_POWERED = BIT(5), > }; > > struct tpm_chip_seqops { > diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c > index 7dee42d7..e89276c 100644 > --- a/drivers/char/tpm/tpm_of.c > +++ b/drivers/char/tpm/tpm_of.c > @@ -34,6 +34,9 @@ int tpm_read_log_of(struct tpm_chip *chip) > else > return -ENODEV; > > + if (of_property_read_bool(np, "powered-while-suspended")) > + chip->flags |= TPM_CHIP_FLAG_ALWAYS_POWERED; > + > sizep = of_get_property(np, "linux,sml-size", NULL); > basep = of_get_property(np, "linux,sml-base", NULL); > if (sizep == NULL && basep == NULL) > -- > 2.9.3 > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
On Fri, Mar 03, 2017 at 05:09:59PM +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. This is useful for the cases > when hardware does not power-off the TPM. > > Signed-off-by: Sonny Rao <sonnyrao@chromium.org> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > --- > Changes since v2: > Jarkko Sakkinen > - Add a new TPM_CHIP_FLAG_ALWAYS_POWERED flag instead of using a boolean variable. > - Remove a trailing newline. > Changes since v1: > Jason Gunthorpe : > - Move the code to handle suspend/resume in the common chip code. > > Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt | 6 ++++++ > drivers/char/tpm/tpm-interface.c | 3 +++ > drivers/char/tpm/tpm.h | 1 + > drivers/char/tpm/tpm_of.c | 3 +++ > 4 files changed, 13 insertions(+) > > diff --git a/Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt b/Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt > index 8cb638b..85c8216 100644 > --- a/Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt > +++ b/Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt > @@ -8,6 +8,12 @@ Required properties: > the firmware event log > - linux,sml-size : size of the memory allocated for the firmware event log > > +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 (for OpenPower Systems with Nuvoton TPM 2.0 on I2C) > ---------------------------------------------------------- Hey, just noticed something. Shouldn't this be a separate commit? I'm also wondering whether this can be submitted through my tree upper maintainers. Does not change my reviewed-by for the actual code change but you would have to split this into a patch set if this is the case. /Jarkko ------------------------------------------------------------------------------ Announcing the Oxford Dictionaries API! The API offers world-renowned dictionary content that is easy and intuitive to access. Sign up for an account today to start using our lexical data to power your apps and projects. Get started today and enter our developer competition. http://sdm.link/oxford
Hi Jarkko, On 06/03/17 21:59, Jarkko Sakkinen wrote: > On Fri, Mar 03, 2017 at 05:09:59PM +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. This is useful for the cases >> when hardware does not power-off the TPM. >> >> Signed-off-by: Sonny Rao <sonnyrao@chromium.org> >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> >> --- >> Changes since v2: >> Jarkko Sakkinen >> - Add a new TPM_CHIP_FLAG_ALWAYS_POWERED flag instead of using a boolean variable. >> - Remove a trailing newline. >> Changes since v1: >> Jason Gunthorpe : >> - Move the code to handle suspend/resume in the common chip code. >> >> Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt | 6 ++++++ >> drivers/char/tpm/tpm-interface.c | 3 +++ >> drivers/char/tpm/tpm.h | 1 + >> drivers/char/tpm/tpm_of.c | 3 +++ >> 4 files changed, 13 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt b/Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt >> index 8cb638b..85c8216 100644 >> --- a/Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt >> +++ b/Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt >> @@ -8,6 +8,12 @@ Required properties: >> the firmware event log >> - linux,sml-size : size of the memory allocated for the firmware event log >> >> +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 (for OpenPower Systems with Nuvoton TPM 2.0 on I2C) >> ---------------------------------------------------------- > > Hey, just noticed something. Shouldn't this be a separate commit? During my life submitting patches I saw the both options, sometimes the maintainer asked me to join the DT patch and the driver and sometimes he asked me to do in different patches, so I think this is more a maintainer option. Maybe Rob Herring or Mark Rutland can share their preferences? I'll do what you want I do, TBH I don't have a strong opinion about this. > I'm also wondering whether this can be submitted through my tree > upper maintainers. > > Does not change my reviewed-by for the actual code change but you > would have to split this into a patch set if this is the case. > > /Jarkko > Cheers, Enric ------------------------------------------------------------------------------ Announcing the Oxford Dictionaries API! The API offers world-renowned dictionary content that is easy and intuitive to access. Sign up for an account today to start using our lexical data to power your apps and projects. Get started today and enter our developer competition. http://sdm.link/oxford
On Tue, Mar 07, 2017 at 09:44:55AM +0100, Enric Balletbo i Serra wrote: > Hi Jarkko, > > On 06/03/17 21:59, Jarkko Sakkinen wrote: > > On Fri, Mar 03, 2017 at 05:09:59PM +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. This is useful for the cases > >> when hardware does not power-off the TPM. > >> > >> Signed-off-by: Sonny Rao <sonnyrao@chromium.org> > >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > >> --- > >> Changes since v2: > >> Jarkko Sakkinen > >> - Add a new TPM_CHIP_FLAG_ALWAYS_POWERED flag instead of using a boolean variable. > >> - Remove a trailing newline. > >> Changes since v1: > >> Jason Gunthorpe : > >> - Move the code to handle suspend/resume in the common chip code. > >> > >> Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt | 6 ++++++ > >> drivers/char/tpm/tpm-interface.c | 3 +++ > >> drivers/char/tpm/tpm.h | 1 + > >> drivers/char/tpm/tpm_of.c | 3 +++ > >> 4 files changed, 13 insertions(+) > >> > >> diff --git a/Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt b/Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt > >> index 8cb638b..85c8216 100644 > >> --- a/Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt > >> +++ b/Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt > >> @@ -8,6 +8,12 @@ Required properties: > >> the firmware event log > >> - linux,sml-size : size of the memory allocated for the firmware event log > >> > >> +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 (for OpenPower Systems with Nuvoton TPM 2.0 on I2C) > >> ---------------------------------------------------------- > > > > Hey, just noticed something. Shouldn't this be a separate commit? > > During my life submitting patches I saw the both options, sometimes > the maintainer asked me to join the DT patch and the driver and > sometimes he asked me to do in different patches, so I think this is > more a maintainer option. Maybe Rob Herring or Mark Rutland can share > their preferences? > > I'll do what you want I do, TBH I don't have a strong opinion about this. I'll add this to my tree and put it to my next branch so that it will flow to linux-next since it only touch TPM specific file in bindings. I do not have perfect answer for this so I'll use common sense (or try to). Rob, is this OK? /Jarkko ------------------------------------------------------------------------------ Announcing the Oxford Dictionaries API! The API offers world-renowned dictionary content that is easy and intuitive to access. Sign up for an account today to start using our lexical data to power your apps and projects. Get started today and enter our developer competition. http://sdm.link/oxford
On Tue, Mar 07, 2017 at 09:44:55AM +0100, Enric Balletbo i Serra wrote: > Hi Jarkko, > > On 06/03/17 21:59, Jarkko Sakkinen wrote: > > On Fri, Mar 03, 2017 at 05:09:59PM +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. This is useful for the cases > >> when hardware does not power-off the TPM. > >> > >> Signed-off-by: Sonny Rao <sonnyrao@chromium.org> > >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > >> --- > >> Changes since v2: > >> Jarkko Sakkinen > >> - Add a new TPM_CHIP_FLAG_ALWAYS_POWERED flag instead of using a boolean variable. > >> - Remove a trailing newline. > >> Changes since v1: > >> Jason Gunthorpe : > >> - Move the code to handle suspend/resume in the common chip code. > >> > >> Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt | 6 ++++++ > >> drivers/char/tpm/tpm-interface.c | 3 +++ > >> drivers/char/tpm/tpm.h | 1 + > >> drivers/char/tpm/tpm_of.c | 3 +++ > >> 4 files changed, 13 insertions(+) > >> > >> diff --git a/Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt b/Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt > >> index 8cb638b..85c8216 100644 > >> --- a/Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt > >> +++ b/Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt > >> @@ -8,6 +8,12 @@ Required properties: > >> the firmware event log > >> - linux,sml-size : size of the memory allocated for the firmware event log > >> > >> +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 (for OpenPower Systems with Nuvoton TPM 2.0 on I2C) > >> ---------------------------------------------------------- > > > > Hey, just noticed something. Shouldn't this be a separate commit? > > During my life submitting patches I saw the both options, sometimes the maintainer asked me to join the DT patch and the driver and sometimes he asked me to do in different patches, so I think this is more a maintainer option. Maybe Rob Herring or Mark Rutland can share their preferences? > Need to fix your mailer to wrap lines... We prefer DT bindings to be separate commits, but don't enforce it. I don't ask for on small changes or if there's no other reason to respin the patch. It makes the history for the DT only tree generated from git filter-branch cleaner. Any other maintainers tell you to combine them, they need to talk to me. Rob ------------------------------------------------------------------------------ Announcing the Oxford Dictionaries API! The API offers world-renowned dictionary content that is easy and intuitive to access. Sign up for an account today to start using our lexical data to power your apps and projects. Get started today and enter our developer competition. http://sdm.link/oxford
On Fri, Mar 03, 2017 at 05:09:59PM +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. This is useful for the cases > when hardware does not power-off the TPM. > > Signed-off-by: Sonny Rao <sonnyrao@chromium.org> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > --- > Changes since v2: > Jarkko Sakkinen > - Add a new TPM_CHIP_FLAG_ALWAYS_POWERED flag instead of using a boolean variable. > - Remove a trailing newline. > Changes since v1: > Jason Gunthorpe : > - Move the code to handle suspend/resume in the common chip code. > > Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt | 6 ++++++ I would say this shouldn't be TPM specific, but looks like we already have a dts with this, so: Acked-by: Rob Herring <robh@kernel.org> > drivers/char/tpm/tpm-interface.c | 3 +++ > drivers/char/tpm/tpm.h | 1 + > drivers/char/tpm/tpm_of.c | 3 +++ > 4 files changed, 13 insertions(+) ------------------------------------------------------------------------------ Announcing the Oxford Dictionaries API! The API offers world-renowned dictionary content that is easy and intuitive to access. Sign up for an account today to start using our lexical data to power your apps and projects. Get started today and enter our developer competition. http://sdm.link/oxford
diff --git a/Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt b/Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt index 8cb638b..85c8216 100644 --- a/Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt +++ b/Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt @@ -8,6 +8,12 @@ Required properties: the firmware event log - linux,sml-size : size of the memory allocated for the firmware event log +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 (for OpenPower Systems with Nuvoton TPM 2.0 on I2C) ---------------------------------------------------------- diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c index a2688ac..6869093 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -944,6 +944,9 @@ int tpm_pm_suspend(struct device *dev) if (chip == NULL) return -ENODEV; + if (chip->flags & TPM_CHIP_FLAG_ALWAYS_POWERED) + return 0; + if (chip->flags & TPM_CHIP_FLAG_TPM2) { tpm2_shutdown(chip, TPM2_SU_STATE); return 0; diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index 1ae9768..0ce379c 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -146,6 +146,7 @@ enum tpm_chip_flags { TPM_CHIP_FLAG_IRQ = BIT(2), TPM_CHIP_FLAG_VIRTUAL = BIT(3), TPM_CHIP_FLAG_HAVE_TIMEOUTS = BIT(4), + TPM_CHIP_FLAG_ALWAYS_POWERED = BIT(5), }; struct tpm_chip_seqops { diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c index 7dee42d7..e89276c 100644 --- a/drivers/char/tpm/tpm_of.c +++ b/drivers/char/tpm/tpm_of.c @@ -34,6 +34,9 @@ int tpm_read_log_of(struct tpm_chip *chip) else return -ENODEV; + if (of_property_read_bool(np, "powered-while-suspended")) + chip->flags |= TPM_CHIP_FLAG_ALWAYS_POWERED; + sizep = of_get_property(np, "linux,sml-size", NULL); basep = of_get_property(np, "linux,sml-base", NULL); if (sizep == NULL && basep == NULL)