| Message ID | 20170301115116.19696-1-enric.balletbo@collabora.com |
|---|---|
| State | New |
| Headers | show |
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)
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) > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
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. ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> > +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 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
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 ------------------------------------------------------------------------------ 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 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 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
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 ------------------------------------------------------------------------------ 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/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)