* [PATCH v4 1/6] tpm: Add a flag to indicate TPM power is managed by firmware
2019-08-12 22:36 [PATCH v4 0/6] tpm: Add driver for cr50 Stephen Boyd
@ 2019-08-12 22:36 ` Stephen Boyd
2019-08-15 20:34 ` Jarkko Sakkinen
2019-08-12 22:36 ` [PATCH v4 2/6] tpm: tpm_tis_spi: Introduce a flow control callback Stephen Boyd
` (5 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Stephen Boyd @ 2019-08-12 22:36 UTC (permalink / raw)
To: Peter Huewe, Jarkko Sakkinen
Cc: linux-kernel, linux-integrity, Andrey Pronin, Duncan Laurie,
Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman,
Guenter Roeck, Alexander Steffen
On some platforms, the TPM power is managed by firmware and therefore we
don't need to stop the TPM on suspend when going to a light version of
suspend such as S0ix ("freeze" suspend state). Add a chip flag,
TPM_CHIP_FLAG_FIRMWARE_POWER_MANAGED, to indicate this so that certain
platforms can probe for the usage of this light suspend and avoid
touching the TPM state across suspend/resume.
Cc: Andrey Pronin <apronin@chromium.org>
Cc: Duncan Laurie <dlaurie@chromium.org>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Alexander Steffen <Alexander.Steffen@infineon.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
drivers/char/tpm/tpm-interface.c | 8 +++++++-
drivers/char/tpm/tpm.h | 1 +
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 1b4f95c13e00..0b3def8e8186 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -23,6 +23,7 @@
#include <linux/slab.h>
#include <linux/mutex.h>
#include <linux/spinlock.h>
+#include <linux/suspend.h>
#include <linux/freezer.h>
#include <linux/tpm_eventlog.h>
@@ -395,7 +396,11 @@ int tpm_pm_suspend(struct device *dev)
return -ENODEV;
if (chip->flags & TPM_CHIP_FLAG_ALWAYS_POWERED)
- return 0;
+ goto suspended;
+
+ if ((chip->flags & TPM_CHIP_FLAG_FIRMWARE_POWER_MANAGED) &&
+ !pm_suspend_via_firmware())
+ goto suspended;
if (!tpm_chip_start(chip)) {
if (chip->flags & TPM_CHIP_FLAG_TPM2)
@@ -406,6 +411,7 @@ int tpm_pm_suspend(struct device *dev)
tpm_chip_stop(chip);
}
+suspended:
return rc;
}
EXPORT_SYMBOL_GPL(tpm_pm_suspend);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index e503ffc3aa39..28f73331aa2e 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -162,6 +162,7 @@ enum tpm_chip_flags {
TPM_CHIP_FLAG_VIRTUAL = BIT(3),
TPM_CHIP_FLAG_HAVE_TIMEOUTS = BIT(4),
TPM_CHIP_FLAG_ALWAYS_POWERED = BIT(5),
+ TPM_CHIP_FLAG_FIRMWARE_POWER_MANAGED = BIT(6),
};
#define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)
--
Sent by a computer through tubes
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4 1/6] tpm: Add a flag to indicate TPM power is managed by firmware
2019-08-12 22:36 ` [PATCH v4 1/6] tpm: Add a flag to indicate TPM power is managed by firmware Stephen Boyd
@ 2019-08-15 20:34 ` Jarkko Sakkinen
0 siblings, 0 replies; 20+ messages in thread
From: Jarkko Sakkinen @ 2019-08-15 20:34 UTC (permalink / raw)
To: Stephen Boyd
Cc: Peter Huewe, linux-kernel, linux-integrity, Andrey Pronin,
Duncan Laurie, Jason Gunthorpe, Arnd Bergmann,
Greg Kroah-Hartman, Guenter Roeck, Alexander Steffen
On Mon, Aug 12, 2019 at 03:36:17PM -0700, Stephen Boyd wrote:
> On some platforms, the TPM power is managed by firmware and therefore we
> don't need to stop the TPM on suspend when going to a light version of
> suspend such as S0ix ("freeze" suspend state). Add a chip flag,
> TPM_CHIP_FLAG_FIRMWARE_POWER_MANAGED, to indicate this so that certain
> platforms can probe for the usage of this light suspend and avoid
> touching the TPM state across suspend/resume.
>
> Cc: Andrey Pronin <apronin@chromium.org>
> Cc: Duncan Laurie <dlaurie@chromium.org>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Guenter Roeck <groeck@chromium.org>
> Cc: Alexander Steffen <Alexander.Steffen@infineon.com>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
LGTM
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
/Jarkko
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 2/6] tpm: tpm_tis_spi: Introduce a flow control callback
2019-08-12 22:36 [PATCH v4 0/6] tpm: Add driver for cr50 Stephen Boyd
2019-08-12 22:36 ` [PATCH v4 1/6] tpm: Add a flag to indicate TPM power is managed by firmware Stephen Boyd
@ 2019-08-12 22:36 ` Stephen Boyd
2019-08-19 16:32 ` Jarkko Sakkinen
2019-08-12 22:36 ` [PATCH v4 3/6] tpm: tpm_tis_spi: Add a pre-transfer callback Stephen Boyd
` (4 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Stephen Boyd @ 2019-08-12 22:36 UTC (permalink / raw)
To: Peter Huewe, Jarkko Sakkinen
Cc: linux-kernel, linux-integrity, Andrey Pronin, Duncan Laurie,
Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman,
Guenter Roeck, Alexander Steffen
Cr50 firmware has a different flow control protocol than the one used by
this TPM PTP SPI driver. Introduce a flow control callback so we can
override the standard sequence with the custom one that Cr50 uses.
Cc: Andrey Pronin <apronin@chromium.org>
Cc: Duncan Laurie <dlaurie@chromium.org>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Alexander Steffen <Alexander.Steffen@infineon.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
drivers/char/tpm/tpm_tis_spi.c | 55 +++++++++++++++++++++-------------
1 file changed, 34 insertions(+), 21 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 19513e622053..819602e85b34 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -42,6 +42,8 @@
struct tpm_tis_spi_phy {
struct tpm_tis_data priv;
struct spi_device *spi_device;
+ int (*flow_control)(struct tpm_tis_spi_phy *phy,
+ struct spi_transfer *xfer);
u8 *iobuf;
};
@@ -50,12 +52,39 @@ static inline struct tpm_tis_spi_phy *to_tpm_tis_spi_phy(struct tpm_tis_data *da
return container_of(data, struct tpm_tis_spi_phy, priv);
}
+static int tpm_tis_spi_flow_control(struct tpm_tis_spi_phy *phy,
+ struct spi_transfer *spi_xfer)
+{
+ struct spi_message m;
+ int ret, i;
+
+ if ((phy->iobuf[3] & 0x01) == 0) {
+ // handle SPI wait states
+ phy->iobuf[0] = 0;
+
+ for (i = 0; i < TPM_RETRY; i++) {
+ spi_xfer->len = 1;
+ spi_message_init(&m);
+ spi_message_add_tail(spi_xfer, &m);
+ ret = spi_sync_locked(phy->spi_device, &m);
+ if (ret < 0)
+ return ret;
+ if (phy->iobuf[0] & 0x01)
+ break;
+ }
+
+ if (i == TPM_RETRY)
+ return -ETIMEDOUT;
+ }
+
+ return 0;
+}
+
static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
u8 *in, const u8 *out)
{
struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
int ret = 0;
- int i;
struct spi_message m;
struct spi_transfer spi_xfer;
u8 transfer_len;
@@ -82,26 +111,9 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
if (ret < 0)
goto exit;
- if ((phy->iobuf[3] & 0x01) == 0) {
- // handle SPI wait states
- phy->iobuf[0] = 0;
-
- for (i = 0; i < TPM_RETRY; i++) {
- spi_xfer.len = 1;
- spi_message_init(&m);
- spi_message_add_tail(&spi_xfer, &m);
- ret = spi_sync_locked(phy->spi_device, &m);
- if (ret < 0)
- goto exit;
- if (phy->iobuf[0] & 0x01)
- break;
- }
-
- if (i == TPM_RETRY) {
- ret = -ETIMEDOUT;
- goto exit;
- }
- }
+ ret = phy->flow_control(phy, &spi_xfer);
+ if (ret < 0)
+ goto exit;
spi_xfer.cs_change = 0;
spi_xfer.len = transfer_len;
@@ -207,6 +219,7 @@ static int tpm_tis_spi_probe(struct spi_device *dev)
phy->iobuf = devm_kmalloc(&dev->dev, MAX_SPI_FRAMESIZE, GFP_KERNEL);
if (!phy->iobuf)
return -ENOMEM;
+ phy->flow_control = tpm_tis_spi_flow_control;
/* If the SPI device has an IRQ then use that */
if (dev->irq > 0)
--
Sent by a computer through tubes
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4 2/6] tpm: tpm_tis_spi: Introduce a flow control callback
2019-08-12 22:36 ` [PATCH v4 2/6] tpm: tpm_tis_spi: Introduce a flow control callback Stephen Boyd
@ 2019-08-19 16:32 ` Jarkko Sakkinen
2019-08-19 17:06 ` Stephen Boyd
0 siblings, 1 reply; 20+ messages in thread
From: Jarkko Sakkinen @ 2019-08-19 16:32 UTC (permalink / raw)
To: Stephen Boyd
Cc: Peter Huewe, linux-kernel, linux-integrity, Andrey Pronin,
Duncan Laurie, Jason Gunthorpe, Arnd Bergmann,
Greg Kroah-Hartman, Guenter Roeck, Alexander Steffen
On Mon, Aug 12, 2019 at 03:36:18PM -0700, Stephen Boyd wrote:
> Cr50 firmware has a different flow control protocol than the one used by
> this TPM PTP SPI driver. Introduce a flow control callback so we can
> override the standard sequence with the custom one that Cr50 uses.
>
> Cc: Andrey Pronin <apronin@chromium.org>
> Cc: Duncan Laurie <dlaurie@chromium.org>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Guenter Roeck <groeck@chromium.org>
> Cc: Alexander Steffen <Alexander.Steffen@infineon.com>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
> drivers/char/tpm/tpm_tis_spi.c | 55 +++++++++++++++++++++-------------
> 1 file changed, 34 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
> index 19513e622053..819602e85b34 100644
> --- a/drivers/char/tpm/tpm_tis_spi.c
> +++ b/drivers/char/tpm/tpm_tis_spi.c
> @@ -42,6 +42,8 @@
> struct tpm_tis_spi_phy {
> struct tpm_tis_data priv;
> struct spi_device *spi_device;
> + int (*flow_control)(struct tpm_tis_spi_phy *phy,
> + struct spi_transfer *xfer);
> u8 *iobuf;
> };
>
> @@ -50,12 +52,39 @@ static inline struct tpm_tis_spi_phy *to_tpm_tis_spi_phy(struct tpm_tis_data *da
> return container_of(data, struct tpm_tis_spi_phy, priv);
> }
>
> +static int tpm_tis_spi_flow_control(struct tpm_tis_spi_phy *phy,
> + struct spi_transfer *spi_xfer)
> +{
> + struct spi_message m;
> + int ret, i;
> +
> + if ((phy->iobuf[3] & 0x01) == 0) {
> + // handle SPI wait states
> + phy->iobuf[0] = 0;
> +
> + for (i = 0; i < TPM_RETRY; i++) {
> + spi_xfer->len = 1;
> + spi_message_init(&m);
> + spi_message_add_tail(spi_xfer, &m);
> + ret = spi_sync_locked(phy->spi_device, &m);
> + if (ret < 0)
> + return ret;
> + if (phy->iobuf[0] & 0x01)
> + break;
> + }
> +
> + if (i == TPM_RETRY)
> + return -ETIMEDOUT;
> + }
> +
> + return 0;
> +}
AFAIK the flow control is not part of the SPI standard itself but is
proprietary for each slave device. Thus, the flow control should be
documented to the source code. I do not want flow control mechanisms to
be multiplied before this is done.
The magic number 0x01 would be also good to get rid off.
/Jarkko
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 2/6] tpm: tpm_tis_spi: Introduce a flow control callback
2019-08-19 16:32 ` Jarkko Sakkinen
@ 2019-08-19 17:06 ` Stephen Boyd
2019-08-21 18:13 ` Jarkko Sakkinen
0 siblings, 1 reply; 20+ messages in thread
From: Stephen Boyd @ 2019-08-19 17:06 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Peter Huewe, linux-kernel, linux-integrity, Andrey Pronin,
Duncan Laurie, Jason Gunthorpe, Arnd Bergmann,
Greg Kroah-Hartman, Guenter Roeck, Alexander Steffen
Quoting Jarkko Sakkinen (2019-08-19 09:32:40)
> On Mon, Aug 12, 2019 at 03:36:18PM -0700, Stephen Boyd wrote:
> > Cr50 firmware has a different flow control protocol than the one used by
> > this TPM PTP SPI driver. Introduce a flow control callback so we can
> > override the standard sequence with the custom one that Cr50 uses.
> >
> > Cc: Andrey Pronin <apronin@chromium.org>
> > Cc: Duncan Laurie <dlaurie@chromium.org>
> > Cc: Jason Gunthorpe <jgg@ziepe.ca>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Guenter Roeck <groeck@chromium.org>
> > Cc: Alexander Steffen <Alexander.Steffen@infineon.com>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> > drivers/char/tpm/tpm_tis_spi.c | 55 +++++++++++++++++++++-------------
> > 1 file changed, 34 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
> > index 19513e622053..819602e85b34 100644
> > --- a/drivers/char/tpm/tpm_tis_spi.c
> > +++ b/drivers/char/tpm/tpm_tis_spi.c
> > @@ -42,6 +42,8 @@
> > struct tpm_tis_spi_phy {
> > struct tpm_tis_data priv;
> > struct spi_device *spi_device;
> > + int (*flow_control)(struct tpm_tis_spi_phy *phy,
> > + struct spi_transfer *xfer);
> > u8 *iobuf;
> > };
> >
> > @@ -50,12 +52,39 @@ static inline struct tpm_tis_spi_phy *to_tpm_tis_spi_phy(struct tpm_tis_data *da
> > return container_of(data, struct tpm_tis_spi_phy, priv);
> > }
> >
> > +static int tpm_tis_spi_flow_control(struct tpm_tis_spi_phy *phy,
> > + struct spi_transfer *spi_xfer)
> > +{
> > + struct spi_message m;
> > + int ret, i;
> > +
> > + if ((phy->iobuf[3] & 0x01) == 0) {
> > + // handle SPI wait states
> > + phy->iobuf[0] = 0;
> > +
> > + for (i = 0; i < TPM_RETRY; i++) {
> > + spi_xfer->len = 1;
> > + spi_message_init(&m);
> > + spi_message_add_tail(spi_xfer, &m);
> > + ret = spi_sync_locked(phy->spi_device, &m);
> > + if (ret < 0)
> > + return ret;
> > + if (phy->iobuf[0] & 0x01)
> > + break;
> > + }
> > +
> > + if (i == TPM_RETRY)
> > + return -ETIMEDOUT;
> > + }
> > +
> > + return 0;
> > +}
>
> AFAIK the flow control is not part of the SPI standard itself but is
> proprietary for each slave device. Thus, the flow control should be
> documented to the source code. I do not want flow control mechanisms to
> be multiplied before this is done.
Can you clarify this please? I don't understand what "the flow control
should be documented to the source code" means.
>
> The magic number 0x01 would be also good to get rid off.
>
Ok. What name should the #define be? I can make that another patch.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 2/6] tpm: tpm_tis_spi: Introduce a flow control callback
2019-08-19 17:06 ` Stephen Boyd
@ 2019-08-21 18:13 ` Jarkko Sakkinen
0 siblings, 0 replies; 20+ messages in thread
From: Jarkko Sakkinen @ 2019-08-21 18:13 UTC (permalink / raw)
To: Stephen Boyd
Cc: Peter Huewe, linux-kernel, linux-integrity, Andrey Pronin,
Duncan Laurie, Jason Gunthorpe, Arnd Bergmann,
Greg Kroah-Hartman, Guenter Roeck, Alexander Steffen
On Mon, Aug 19, 2019 at 10:06:40AM -0700, Stephen Boyd wrote:
> > AFAIK the flow control is not part of the SPI standard itself but is
> > proprietary for each slave device. Thus, the flow control should be
> > documented to the source code. I do not want flow control mechanisms to
> > be multiplied before this is done.
>
> Can you clarify this please? I don't understand what "the flow control
> should be documented to the source code" means.
Off the top of my head:
/* TCG SPI flow control is documented in the section 6.4 of [1]. However,
* Google's CR50 chip has its own proprietary flow control. This struct
* is used to bind the appropriate flow control mechanism.
*
* [1] https://trustedcomputinggroup.org/resource/pc-client-platform-tpm-profile-ptp-specification/
*/
> >
> > The magic number 0x01 would be also good to get rid off.
> >
>
> Ok. What name should the #define be? I can make that another patch.
Do nothing. Not part of your patch set scope, was a stupid comment from
my side.
/Jarkko
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 3/6] tpm: tpm_tis_spi: Add a pre-transfer callback
2019-08-12 22:36 [PATCH v4 0/6] tpm: Add driver for cr50 Stephen Boyd
2019-08-12 22:36 ` [PATCH v4 1/6] tpm: Add a flag to indicate TPM power is managed by firmware Stephen Boyd
2019-08-12 22:36 ` [PATCH v4 2/6] tpm: tpm_tis_spi: Introduce a flow control callback Stephen Boyd
@ 2019-08-12 22:36 ` Stephen Boyd
2019-08-19 16:35 ` Jarkko Sakkinen
2019-08-12 22:36 ` [PATCH v4 4/6] tpm: tpm_tis_spi: Export functionality to other drivers Stephen Boyd
` (3 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Stephen Boyd @ 2019-08-12 22:36 UTC (permalink / raw)
To: Peter Huewe, Jarkko Sakkinen
Cc: linux-kernel, linux-integrity, Andrey Pronin, Duncan Laurie,
Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman,
Guenter Roeck, Alexander Steffen
Cr50 firmware has a requirement to wait for the TPM to wakeup before
sending commands over the SPI bus. Otherwise, the firmware could be in
deep sleep and not respond. Add a hook to tpm_tis_spi_transfer() before
we start a SPI transfer so we can keep track of the last time the TPM
driver accessed the SPI bus.
Cc: Andrey Pronin <apronin@chromium.org>
Cc: Duncan Laurie <dlaurie@chromium.org>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Alexander Steffen <Alexander.Steffen@infineon.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
drivers/char/tpm/tpm_tis_spi.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 819602e85b34..93f49b1941f0 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -44,6 +44,7 @@ struct tpm_tis_spi_phy {
struct spi_device *spi_device;
int (*flow_control)(struct tpm_tis_spi_phy *phy,
struct spi_transfer *xfer);
+ void (*pre_transfer)(struct tpm_tis_spi_phy *phy);
u8 *iobuf;
};
@@ -129,6 +130,8 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
spi_message_init(&m);
spi_message_add_tail(&spi_xfer, &m);
+ if (phy->pre_transfer)
+ phy->pre_transfer(phy);
ret = spi_sync_locked(phy->spi_device, &m);
if (ret < 0)
goto exit;
--
Sent by a computer through tubes
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4 3/6] tpm: tpm_tis_spi: Add a pre-transfer callback
2019-08-12 22:36 ` [PATCH v4 3/6] tpm: tpm_tis_spi: Add a pre-transfer callback Stephen Boyd
@ 2019-08-19 16:35 ` Jarkko Sakkinen
2019-08-19 17:07 ` Stephen Boyd
0 siblings, 1 reply; 20+ messages in thread
From: Jarkko Sakkinen @ 2019-08-19 16:35 UTC (permalink / raw)
To: Stephen Boyd
Cc: Peter Huewe, linux-kernel, linux-integrity, Andrey Pronin,
Duncan Laurie, Jason Gunthorpe, Arnd Bergmann,
Greg Kroah-Hartman, Guenter Roeck, Alexander Steffen
On Mon, Aug 12, 2019 at 03:36:19PM -0700, Stephen Boyd wrote:
> Cr50 firmware has a requirement to wait for the TPM to wakeup before
> sending commands over the SPI bus. Otherwise, the firmware could be in
> deep sleep and not respond. Add a hook to tpm_tis_spi_transfer() before
> we start a SPI transfer so we can keep track of the last time the TPM
> driver accessed the SPI bus.
>
> Cc: Andrey Pronin <apronin@chromium.org>
> Cc: Duncan Laurie <dlaurie@chromium.org>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Guenter Roeck <groeck@chromium.org>
> Cc: Alexander Steffen <Alexander.Steffen@infineon.com>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
> drivers/char/tpm/tpm_tis_spi.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
> index 819602e85b34..93f49b1941f0 100644
> --- a/drivers/char/tpm/tpm_tis_spi.c
> +++ b/drivers/char/tpm/tpm_tis_spi.c
> @@ -44,6 +44,7 @@ struct tpm_tis_spi_phy {
> struct spi_device *spi_device;
> int (*flow_control)(struct tpm_tis_spi_phy *phy,
> struct spi_transfer *xfer);
> + void (*pre_transfer)(struct tpm_tis_spi_phy *phy);
A callback should have somewhat well defined purpose. A callback named
as pre_transfer() could have any purpose.
/Jarkko
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 3/6] tpm: tpm_tis_spi: Add a pre-transfer callback
2019-08-19 16:35 ` Jarkko Sakkinen
@ 2019-08-19 17:07 ` Stephen Boyd
2019-08-21 19:11 ` Jarkko Sakkinen
0 siblings, 1 reply; 20+ messages in thread
From: Stephen Boyd @ 2019-08-19 17:07 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Peter Huewe, linux-kernel, linux-integrity, Andrey Pronin,
Duncan Laurie, Jason Gunthorpe, Arnd Bergmann,
Greg Kroah-Hartman, Guenter Roeck, Alexander Steffen
Quoting Jarkko Sakkinen (2019-08-19 09:35:05)
> On Mon, Aug 12, 2019 at 03:36:19PM -0700, Stephen Boyd wrote:
> > Cr50 firmware has a requirement to wait for the TPM to wakeup before
> > sending commands over the SPI bus. Otherwise, the firmware could be in
> > deep sleep and not respond. Add a hook to tpm_tis_spi_transfer() before
> > we start a SPI transfer so we can keep track of the last time the TPM
> > driver accessed the SPI bus.
> >
> > Cc: Andrey Pronin <apronin@chromium.org>
> > Cc: Duncan Laurie <dlaurie@chromium.org>
> > Cc: Jason Gunthorpe <jgg@ziepe.ca>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Guenter Roeck <groeck@chromium.org>
> > Cc: Alexander Steffen <Alexander.Steffen@infineon.com>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> > drivers/char/tpm/tpm_tis_spi.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
> > index 819602e85b34..93f49b1941f0 100644
> > --- a/drivers/char/tpm/tpm_tis_spi.c
> > +++ b/drivers/char/tpm/tpm_tis_spi.c
> > @@ -44,6 +44,7 @@ struct tpm_tis_spi_phy {
> > struct spi_device *spi_device;
> > int (*flow_control)(struct tpm_tis_spi_phy *phy,
> > struct spi_transfer *xfer);
> > + void (*pre_transfer)(struct tpm_tis_spi_phy *phy);
>
> A callback should have somewhat well defined purpose. A callback named
> as pre_transfer() could have any purpose.
>
Any name is fine for me. Any suggestions?
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 3/6] tpm: tpm_tis_spi: Add a pre-transfer callback
2019-08-19 17:07 ` Stephen Boyd
@ 2019-08-21 19:11 ` Jarkko Sakkinen
2019-08-21 21:44 ` Stephen Boyd
0 siblings, 1 reply; 20+ messages in thread
From: Jarkko Sakkinen @ 2019-08-21 19:11 UTC (permalink / raw)
To: Stephen Boyd
Cc: Peter Huewe, linux-kernel, linux-integrity, Andrey Pronin,
Duncan Laurie, Jason Gunthorpe, Arnd Bergmann,
Greg Kroah-Hartman, Guenter Roeck, Alexander Steffen
On Mon, Aug 19, 2019 at 10:07:41AM -0700, Stephen Boyd wrote:
> Any name is fine for me. Any suggestions?
What if just add @ready to struct tpm_tis_spi_phy add drop this patch
altogether?
It is only used only by CR50 but I think it is less of an overkill than
adding a callback.
/Jarkko
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 3/6] tpm: tpm_tis_spi: Add a pre-transfer callback
2019-08-21 19:11 ` Jarkko Sakkinen
@ 2019-08-21 21:44 ` Stephen Boyd
0 siblings, 0 replies; 20+ messages in thread
From: Stephen Boyd @ 2019-08-21 21:44 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Peter Huewe, linux-kernel, linux-integrity, Andrey Pronin,
Duncan Laurie, Jason Gunthorpe, Arnd Bergmann,
Greg Kroah-Hartman, Guenter Roeck, Alexander Steffen
Quoting Jarkko Sakkinen (2019-08-21 12:11:31)
> On Mon, Aug 19, 2019 at 10:07:41AM -0700, Stephen Boyd wrote:
> > Any name is fine for me. Any suggestions?
>
> What if just add @ready to struct tpm_tis_spi_phy add drop this patch
> altogether?
>
> It is only used only by CR50 but I think it is less of an overkill than
> adding a callback.
>
Ok, sounds good.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 4/6] tpm: tpm_tis_spi: Export functionality to other drivers
2019-08-12 22:36 [PATCH v4 0/6] tpm: Add driver for cr50 Stephen Boyd
` (2 preceding siblings ...)
2019-08-12 22:36 ` [PATCH v4 3/6] tpm: tpm_tis_spi: Add a pre-transfer callback Stephen Boyd
@ 2019-08-12 22:36 ` Stephen Boyd
2019-08-19 16:40 ` Jarkko Sakkinen
2019-08-12 22:36 ` [PATCH v4 5/6] dt-bindings: tpm: document properties for cr50 Stephen Boyd
` (2 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Stephen Boyd @ 2019-08-12 22:36 UTC (permalink / raw)
To: Peter Huewe, Jarkko Sakkinen
Cc: linux-kernel, linux-integrity, Andrey Pronin, Duncan Laurie,
Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman,
Guenter Roeck, Alexander Steffen
Export a new function, tpm_tis_spi_init(), and the associated
read/write/transfer APIs so that we can create variant drivers based on
the core functionality of this TPM SPI driver. Variant drivers can wrap
the tpm_tis_spi_phy struct with their own struct and override the
behavior of tpm_tis_spi_transfer() by supplying their own flow control
and pre-transfer hooks. This shares the most code between the core
driver and any variants that want to override certain behavior without
cluttering the core driver.
Cc: Andrey Pronin <apronin@chromium.org>
Cc: Duncan Laurie <dlaurie@chromium.org>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Alexander Steffen <Alexander.Steffen@infineon.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
drivers/char/tpm/tpm_tis_spi.c | 52 ++++++++++++++++------------------
drivers/char/tpm/tpm_tis_spi.h | 37 ++++++++++++++++++++++++
2 files changed, 62 insertions(+), 27 deletions(-)
create mode 100644 drivers/char/tpm/tpm_tis_spi.h
diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 93f49b1941f0..fd3fb4f9f506 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -36,23 +36,10 @@
#include <linux/tpm.h>
#include "tpm.h"
#include "tpm_tis_core.h"
+#include "tpm_tis_spi.h"
#define MAX_SPI_FRAMESIZE 64
-struct tpm_tis_spi_phy {
- struct tpm_tis_data priv;
- struct spi_device *spi_device;
- int (*flow_control)(struct tpm_tis_spi_phy *phy,
- struct spi_transfer *xfer);
- void (*pre_transfer)(struct tpm_tis_spi_phy *phy);
- u8 *iobuf;
-};
-
-static inline struct tpm_tis_spi_phy *to_tpm_tis_spi_phy(struct tpm_tis_data *data)
-{
- return container_of(data, struct tpm_tis_spi_phy, priv);
-}
-
static int tpm_tis_spi_flow_control(struct tpm_tis_spi_phy *phy,
struct spi_transfer *spi_xfer)
{
@@ -81,7 +68,7 @@ static int tpm_tis_spi_flow_control(struct tpm_tis_spi_phy *phy,
return 0;
}
-static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
+int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
u8 *in, const u8 *out)
{
struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
@@ -148,9 +135,10 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
spi_bus_unlock(phy->spi_device->master);
return ret;
}
+EXPORT_SYMBOL_GPL(tpm_tis_spi_transfer);
-static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
- u16 len, u8 *result)
+static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr, u16 len,
+ u8 *result)
{
return tpm_tis_spi_transfer(data, addr, len, result, NULL);
}
@@ -161,7 +149,7 @@ static int tpm_tis_spi_write_bytes(struct tpm_tis_data *data, u32 addr,
return tpm_tis_spi_transfer(data, addr, len, NULL, value);
}
-static int tpm_tis_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
+int tpm_tis_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
{
__le16 result_le;
int rc;
@@ -173,8 +161,9 @@ static int tpm_tis_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
return rc;
}
+EXPORT_SYMBOL_GPL(tpm_tis_spi_read16);
-static int tpm_tis_spi_read32(struct tpm_tis_data *data, u32 addr, u32 *result)
+int tpm_tis_spi_read32(struct tpm_tis_data *data, u32 addr, u32 *result)
{
__le32 result_le;
int rc;
@@ -186,8 +175,9 @@ static int tpm_tis_spi_read32(struct tpm_tis_data *data, u32 addr, u32 *result)
return rc;
}
+EXPORT_SYMBOL_GPL(tpm_tis_spi_read32);
-static int tpm_tis_spi_write32(struct tpm_tis_data *data, u32 addr, u32 value)
+int tpm_tis_spi_write32(struct tpm_tis_data *data, u32 addr, u32 value)
{
__le32 value_le;
int rc;
@@ -198,6 +188,20 @@ static int tpm_tis_spi_write32(struct tpm_tis_data *data, u32 addr, u32 value)
return rc;
}
+EXPORT_SYMBOL_GPL(tpm_tis_spi_write32);
+
+int tpm_tis_spi_init(struct spi_device *spi, struct tpm_tis_spi_phy *phy,
+ int irq, const struct tpm_tis_phy_ops *phy_ops)
+{
+ phy->iobuf = devm_kmalloc(&spi->dev, MAX_SPI_FRAMESIZE, GFP_KERNEL);
+ if (!phy->iobuf)
+ return -ENOMEM;
+
+ phy->spi_device = spi;
+
+ return tpm_tis_core_init(&spi->dev, &phy->priv, irq, phy_ops, NULL);
+}
+EXPORT_SYMBOL_GPL(tpm_tis_spi_init);
static const struct tpm_tis_phy_ops tpm_spi_phy_ops = {
.read_bytes = tpm_tis_spi_read_bytes,
@@ -217,11 +221,6 @@ static int tpm_tis_spi_probe(struct spi_device *dev)
if (!phy)
return -ENOMEM;
- phy->spi_device = dev;
-
- phy->iobuf = devm_kmalloc(&dev->dev, MAX_SPI_FRAMESIZE, GFP_KERNEL);
- if (!phy->iobuf)
- return -ENOMEM;
phy->flow_control = tpm_tis_spi_flow_control;
/* If the SPI device has an IRQ then use that */
@@ -230,8 +229,7 @@ static int tpm_tis_spi_probe(struct spi_device *dev)
else
irq = -1;
- return tpm_tis_core_init(&dev->dev, &phy->priv, irq, &tpm_spi_phy_ops,
- NULL);
+ return tpm_tis_spi_init(dev, phy, irq, &tpm_spi_phy_ops);
}
static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
diff --git a/drivers/char/tpm/tpm_tis_spi.h b/drivers/char/tpm/tpm_tis_spi.h
new file mode 100644
index 000000000000..48be5130794a
--- /dev/null
+++ b/drivers/char/tpm/tpm_tis_spi.h
@@ -0,0 +1,37 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2015 Infineon Technologies AG
+ * Copyright (C) 2016 STMicroelectronics SAS
+ */
+
+#ifndef TPM_TIS_SPI_H
+#define TPM_TIS_SPI_H
+
+#include "tpm_tis_core.h"
+
+struct tpm_tis_spi_phy {
+ struct tpm_tis_data priv;
+ struct spi_device *spi_device;
+ int (*flow_control)(struct tpm_tis_spi_phy *phy,
+ struct spi_transfer *xfer);
+ void (*pre_transfer)(struct tpm_tis_spi_phy *phy);
+
+ u8 *iobuf;
+};
+
+static inline struct tpm_tis_spi_phy *to_tpm_tis_spi_phy(struct tpm_tis_data *data)
+{
+ return container_of(data, struct tpm_tis_spi_phy, priv);
+}
+
+extern int tpm_tis_spi_init(struct spi_device *spi, struct tpm_tis_spi_phy *phy,
+ int irq, const struct tpm_tis_phy_ops *phy_ops);
+
+extern int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
+ u8 *in, const u8 *out);
+
+extern int tpm_tis_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result);
+extern int tpm_tis_spi_read32(struct tpm_tis_data *data, u32 addr, u32 *result);
+extern int tpm_tis_spi_write32(struct tpm_tis_data *data, u32 addr, u32 value);
+
+#endif
--
Sent by a computer through tubes
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4 4/6] tpm: tpm_tis_spi: Export functionality to other drivers
2019-08-12 22:36 ` [PATCH v4 4/6] tpm: tpm_tis_spi: Export functionality to other drivers Stephen Boyd
@ 2019-08-19 16:40 ` Jarkko Sakkinen
2019-08-19 17:10 ` Stephen Boyd
0 siblings, 1 reply; 20+ messages in thread
From: Jarkko Sakkinen @ 2019-08-19 16:40 UTC (permalink / raw)
To: Stephen Boyd
Cc: Peter Huewe, linux-kernel, linux-integrity, Andrey Pronin,
Duncan Laurie, Jason Gunthorpe, Arnd Bergmann,
Greg Kroah-Hartman, Guenter Roeck, Alexander Steffen
On Mon, Aug 12, 2019 at 03:36:20PM -0700, Stephen Boyd wrote:
> Export a new function, tpm_tis_spi_init(), and the associated
> read/write/transfer APIs so that we can create variant drivers based on
> the core functionality of this TPM SPI driver. Variant drivers can wrap
> the tpm_tis_spi_phy struct with their own struct and override the
> behavior of tpm_tis_spi_transfer() by supplying their own flow control
> and pre-transfer hooks. This shares the most code between the core
> driver and any variants that want to override certain behavior without
> cluttering the core driver.
I think this is adding way too much complexity for the purpose. We
definitely do want this three layer architecture here.
Instead there should be a single tpm_tis_spi driver that dynamically
either TCG or CR50. I rather take some extra bytes in the LKM than
the added complexity.
/Jarkko
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 4/6] tpm: tpm_tis_spi: Export functionality to other drivers
2019-08-19 16:40 ` Jarkko Sakkinen
@ 2019-08-19 17:10 ` Stephen Boyd
2019-08-21 17:58 ` Jarkko Sakkinen
0 siblings, 1 reply; 20+ messages in thread
From: Stephen Boyd @ 2019-08-19 17:10 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Peter Huewe, linux-kernel, linux-integrity, Andrey Pronin,
Duncan Laurie, Jason Gunthorpe, Arnd Bergmann,
Greg Kroah-Hartman, Guenter Roeck, Alexander Steffen
Quoting Jarkko Sakkinen (2019-08-19 09:40:05)
> On Mon, Aug 12, 2019 at 03:36:20PM -0700, Stephen Boyd wrote:
> > Export a new function, tpm_tis_spi_init(), and the associated
> > read/write/transfer APIs so that we can create variant drivers based on
> > the core functionality of this TPM SPI driver. Variant drivers can wrap
> > the tpm_tis_spi_phy struct with their own struct and override the
> > behavior of tpm_tis_spi_transfer() by supplying their own flow control
> > and pre-transfer hooks. This shares the most code between the core
> > driver and any variants that want to override certain behavior without
> > cluttering the core driver.
>
> I think this is adding way too much complexity for the purpose. We
> definitely do want this three layer architecture here.
>
> Instead there should be a single tpm_tis_spi driver that dynamically
> either TCG or CR50. I rather take some extra bytes in the LKM than
> the added complexity.
>
Ok. I had that patch originally[1]. Do you want me to resend that patch
and start review over from there?
[1] https://lkml.kernel.org/r/5d2f955d.1c69fb81.35877.7018@mx.google.com
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 4/6] tpm: tpm_tis_spi: Export functionality to other drivers
2019-08-19 17:10 ` Stephen Boyd
@ 2019-08-21 17:58 ` Jarkko Sakkinen
2019-08-22 17:29 ` Stephen Boyd
0 siblings, 1 reply; 20+ messages in thread
From: Jarkko Sakkinen @ 2019-08-21 17:58 UTC (permalink / raw)
To: Stephen Boyd
Cc: Peter Huewe, linux-kernel, linux-integrity, Andrey Pronin,
Duncan Laurie, Jason Gunthorpe, Arnd Bergmann,
Greg Kroah-Hartman, Guenter Roeck, Alexander Steffen
On Mon, Aug 19, 2019 at 10:10:08AM -0700, Stephen Boyd wrote:
> Quoting Jarkko Sakkinen (2019-08-19 09:40:05)
> > On Mon, Aug 12, 2019 at 03:36:20PM -0700, Stephen Boyd wrote:
> > > Export a new function, tpm_tis_spi_init(), and the associated
> > > read/write/transfer APIs so that we can create variant drivers based on
> > > the core functionality of this TPM SPI driver. Variant drivers can wrap
> > > the tpm_tis_spi_phy struct with their own struct and override the
> > > behavior of tpm_tis_spi_transfer() by supplying their own flow control
> > > and pre-transfer hooks. This shares the most code between the core
> > > driver and any variants that want to override certain behavior without
> > > cluttering the core driver.
> >
> > I think this is adding way too much complexity for the purpose. We
> > definitely do want this three layer architecture here.
> >
> > Instead there should be a single tpm_tis_spi driver that dynamically
> > either TCG or CR50. I rather take some extra bytes in the LKM than
> > the added complexity.
> >
>
> Ok. I had that patch originally[1]. Do you want me to resend that patch
> and start review over from there?
>
> [1] https://lkml.kernel.org/r/5d2f955d.1c69fb81.35877.7018@mx.google.com
What if:
1. You mostly use this solution but have it as a separate source module
only.
2. Use TPM_IS_CR50 only once to bind the callbacks.
/Jarkko
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 4/6] tpm: tpm_tis_spi: Export functionality to other drivers
2019-08-21 17:58 ` Jarkko Sakkinen
@ 2019-08-22 17:29 ` Stephen Boyd
0 siblings, 0 replies; 20+ messages in thread
From: Stephen Boyd @ 2019-08-22 17:29 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Peter Huewe, linux-kernel, linux-integrity, Andrey Pronin,
Duncan Laurie, Jason Gunthorpe, Arnd Bergmann,
Greg Kroah-Hartman, Guenter Roeck, Alexander Steffen
Quoting Jarkko Sakkinen (2019-08-21 10:58:46)
> On Mon, Aug 19, 2019 at 10:10:08AM -0700, Stephen Boyd wrote:
> > Quoting Jarkko Sakkinen (2019-08-19 09:40:05)
> > >
> > > Instead there should be a single tpm_tis_spi driver that dynamically
> > > either TCG or CR50. I rather take some extra bytes in the LKM than
> > > the added complexity.
> > >
> >
> > Ok. I had that patch originally[1]. Do you want me to resend that patch
> > and start review over from there?
> >
> > [1] https://lkml.kernel.org/r/5d2f955d.1c69fb81.35877.7018@mx.google.com
>
> What if:
>
> 1. You mostly use this solution but have it as a separate source module
> only.
> 2. Use TPM_IS_CR50 only once to bind the callbacks.
>
Ok I think I understand. Take the callback approach from these patches
and combine that with the TPM_IS_CR50 changes I made in [1]. I'll try it
out and resend.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 5/6] dt-bindings: tpm: document properties for cr50
2019-08-12 22:36 [PATCH v4 0/6] tpm: Add driver for cr50 Stephen Boyd
` (3 preceding siblings ...)
2019-08-12 22:36 ` [PATCH v4 4/6] tpm: tpm_tis_spi: Export functionality to other drivers Stephen Boyd
@ 2019-08-12 22:36 ` Stephen Boyd
2019-08-12 22:36 ` [PATCH v4 6/6] tpm: add driver for cr50 on SPI Stephen Boyd
2019-08-27 0:58 ` [PATCH v4 0/6] tpm: Add driver for cr50 Heiko Stuebner
6 siblings, 0 replies; 20+ messages in thread
From: Stephen Boyd @ 2019-08-12 22:36 UTC (permalink / raw)
To: Peter Huewe, Jarkko Sakkinen
Cc: Andrey Pronin, linux-kernel, linux-integrity, Duncan Laurie,
Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman,
Guenter Roeck, Alexander Steffen, Rob Herring
From: Andrey Pronin <apronin@chromium.org>
Add TPM2.0 PTP FIFO compatible SPI interface for chips with Cr50
firmware.
Cc: Andrey Pronin <apronin@chromium.org>
Cc: Duncan Laurie <dlaurie@chromium.org>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Alexander Steffen <Alexander.Steffen@infineon.com>
Signed-off-by: Andrey Pronin <apronin@chromium.org>
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
.../bindings/security/tpm/google,cr50.txt | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
create mode 100644 Documentation/devicetree/bindings/security/tpm/google,cr50.txt
diff --git a/Documentation/devicetree/bindings/security/tpm/google,cr50.txt b/Documentation/devicetree/bindings/security/tpm/google,cr50.txt
new file mode 100644
index 000000000000..7aa65224c8b9
--- /dev/null
+++ b/Documentation/devicetree/bindings/security/tpm/google,cr50.txt
@@ -0,0 +1,19 @@
+* H1 Secure Microcontroller with Cr50 Firmware on SPI Bus.
+
+H1 Secure Microcontroller running Cr50 firmware provides several
+functions, including TPM-like functionality. It communicates over
+SPI using the FIFO protocol described in the PTP Spec, section 6.
+
+Required properties:
+- compatible: Should be "google,cr50".
+- spi-max-frequency: Maximum SPI frequency.
+
+Example:
+
+&spi0 {
+ tpm@0 {
+ compatible = "google,cr50";
+ reg = <0>;
+ spi-max-frequency = <800000>;
+ };
+};
--
Sent by a computer through tubes
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v4 6/6] tpm: add driver for cr50 on SPI
2019-08-12 22:36 [PATCH v4 0/6] tpm: Add driver for cr50 Stephen Boyd
` (4 preceding siblings ...)
2019-08-12 22:36 ` [PATCH v4 5/6] dt-bindings: tpm: document properties for cr50 Stephen Boyd
@ 2019-08-12 22:36 ` Stephen Boyd
2019-08-27 0:58 ` [PATCH v4 0/6] tpm: Add driver for cr50 Heiko Stuebner
6 siblings, 0 replies; 20+ messages in thread
From: Stephen Boyd @ 2019-08-12 22:36 UTC (permalink / raw)
To: Peter Huewe, Jarkko Sakkinen
Cc: Andrey Pronin, linux-kernel, linux-integrity, Duncan Laurie,
Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman,
Guenter Roeck, Alexander Steffen
From: Andrey Pronin <apronin@chromium.org>
Add TPM2.0 PTP FIFO compatible SPI interface for chips with Cr50
firmware. The firmware running on the currently supported H1
Secure Microcontroller requires a special driver to handle its
specifics:
- need to ensure a certain delay between spi transactions, or else
the chip may miss some part of the next transaction;
- if there is no spi activity for some time, it may go to sleep,
and needs to be waken up before sending further commands;
- access to vendor-specific registers.
Signed-off-by: Andrey Pronin <apronin@chromium.org>
Cc: Andrey Pronin <apronin@chromium.org>
Cc: Duncan Laurie <dlaurie@chromium.org>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Alexander Steffen <Alexander.Steffen@infineon.com>
[swboyd@chromium.org: Replace boilerplate with SPDX tag, drop
suspended bit and remove ifdef checks in cr50.h, push tpm.h
include into cr50.c, migrate to functions exported from tpm_tis_spi.h]
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
drivers/char/tpm/Kconfig | 9 +
drivers/char/tpm/Makefile | 1 +
drivers/char/tpm/cr50_spi.c | 372 ++++++++++++++++++++++++++++++++++++
3 files changed, 382 insertions(+)
create mode 100644 drivers/char/tpm/cr50_spi.c
diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index 88a3c06fc153..1fb23f145ef6 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -114,6 +114,15 @@ config TCG_ATMEL
will be accessible from within Linux. To compile this driver
as a module, choose M here; the module will be called tpm_atmel.
+config TCG_CR50_SPI
+ tristate "Cr50 SPI Interface"
+ depends on TCG_TIS_SPI
+ ---help---
+ If you have a H1 secure module running Cr50 firmware on SPI bus,
+ say Yes and it will be accessible from within Linux. To compile
+ this driver as a module, choose M here; the module will be called
+ cr50_spi.
+
config TCG_INFINEON
tristate "Infineon Technologies TPM Interface"
depends on PNP
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index a01c4cab902a..4713e52ce1b0 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o
obj-$(CONFIG_TCG_NSC) += tpm_nsc.o
obj-$(CONFIG_TCG_ATMEL) += tpm_atmel.o
+obj-$(CONFIG_TCG_CR50_SPI) += cr50_spi.o
obj-$(CONFIG_TCG_INFINEON) += tpm_infineon.o
obj-$(CONFIG_TCG_IBMVTPM) += tpm_ibmvtpm.o
obj-$(CONFIG_TCG_TIS_ST33ZP24) += st33zp24/
diff --git a/drivers/char/tpm/cr50_spi.c b/drivers/char/tpm/cr50_spi.c
new file mode 100644
index 000000000000..a163864e9c63
--- /dev/null
+++ b/drivers/char/tpm/cr50_spi.c
@@ -0,0 +1,372 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2016 Google, Inc
+ *
+ * This device driver implements a TCG PTP FIFO interface over SPI for chips
+ * with Cr50 firmware.
+ * It is based on tpm_tis_spi driver by Peter Huewe and Christophe Ricard.
+ */
+
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pm.h>
+#include <linux/spi/spi.h>
+#include <linux/wait.h>
+
+#include "tpm_tis_core.h"
+#include "tpm_tis_spi.h"
+
+/*
+ * Cr50 timing constants:
+ * - can go to sleep not earlier than after CR50_SLEEP_DELAY_MSEC.
+ * - needs up to CR50_WAKE_START_DELAY_USEC to wake after sleep.
+ * - requires waiting for "ready" IRQ, if supported; or waiting for at least
+ * CR50_NOIRQ_ACCESS_DELAY_MSEC between transactions, if IRQ is not supported.
+ * - waits for up to CR50_FLOW_CONTROL for flow control 'ready' indication.
+ */
+#define CR50_SLEEP_DELAY_MSEC 1000
+#define CR50_WAKE_START_DELAY_USEC 1000
+#define CR50_NOIRQ_ACCESS_DELAY msecs_to_jiffies(2)
+#define CR50_READY_IRQ_TIMEOUT msecs_to_jiffies(TPM2_TIMEOUT_A)
+#define CR50_FLOW_CONTROL msecs_to_jiffies(TPM2_TIMEOUT_A)
+#define MAX_IRQ_CONFIRMATION_ATTEMPTS 3
+
+#define TPM_CR50_FW_VER(l) (0x0f90 | ((l) << 12))
+#define TPM_CR50_MAX_FW_VER_LEN 64
+
+struct cr50_spi_phy {
+ struct tpm_tis_spi_phy spi_phy;
+
+ struct mutex time_track_mutex;
+ unsigned long last_access;
+ unsigned long wake_after;
+
+ unsigned long access_delay;
+
+ struct completion ready;
+
+ unsigned int irq_confirmation_attempt;
+ bool irq_needs_confirmation;
+ bool irq_confirmed;
+};
+
+static inline struct cr50_spi_phy *to_cr50_spi_phy(struct tpm_tis_spi_phy *phy)
+{
+ return container_of(phy, struct cr50_spi_phy, spi_phy);
+}
+
+/*
+ * The cr50 interrupt handler just signals waiting threads that the
+ * interrupt was asserted. It does not do any processing triggered
+ * by interrupts but is instead used to avoid fixed delays.
+ */
+static irqreturn_t cr50_spi_irq_handler(int dummy, void *dev_id)
+{
+ struct cr50_spi_phy *cr50_phy = dev_id;
+
+ cr50_phy->irq_confirmed = true;
+ complete(&cr50_phy->ready);
+
+ return IRQ_HANDLED;
+}
+
+/*
+ * Cr50 needs to have at least some delay between consecutive
+ * transactions. Make sure we wait.
+ */
+static void cr50_ensure_access_delay(struct cr50_spi_phy *phy)
+{
+ unsigned long allowed_access = phy->last_access + phy->access_delay;
+ unsigned long time_now = jiffies;
+ struct device *dev = &phy->spi_phy.spi_device->dev;
+
+ /*
+ * Note: There is a small chance, if Cr50 is not accessed in a few days,
+ * that time_in_range will not provide the correct result after the wrap
+ * around for jiffies. In this case, we'll have an unneeded short delay,
+ * which is fine.
+ */
+ if (time_in_range_open(time_now, phy->last_access, allowed_access)) {
+ unsigned long remaining, timeout = allowed_access - time_now;
+
+ remaining = wait_for_completion_timeout(&phy->ready, timeout);
+ if (!remaining && phy->irq_confirmed)
+ dev_warn(dev, "Timeout waiting for TPM ready IRQ\n");
+ }
+
+ if (phy->irq_needs_confirmation) {
+ unsigned int attempt = ++phy->irq_confirmation_attempt;
+
+ if (phy->irq_confirmed) {
+ phy->irq_needs_confirmation = false;
+ phy->access_delay = CR50_READY_IRQ_TIMEOUT;
+ dev_info(dev, "TPM ready IRQ confirmed on attempt %u\n",
+ attempt);
+ } else if (attempt > MAX_IRQ_CONFIRMATION_ATTEMPTS) {
+ phy->irq_needs_confirmation = false;
+ dev_warn(dev, "IRQ not confirmed - will use delays\n");
+ }
+ }
+}
+
+/*
+ * Cr50 might go to sleep if there is no SPI activity for some time and
+ * miss the first few bits/bytes on the bus. In such case, wake it up
+ * by asserting CS and give it time to start up.
+ */
+static bool cr50_needs_waking(struct cr50_spi_phy *phy)
+{
+ /*
+ * Note: There is a small chance, if Cr50 is not accessed in a few days,
+ * that time_in_range will not provide the correct result after the wrap
+ * around for jiffies. In this case, we'll probably timeout or read
+ * incorrect value from TPM_STS and just retry the operation.
+ */
+ return !time_in_range_open(jiffies, phy->last_access, phy->wake_after);
+}
+
+static void cr50_wake_if_needed(struct cr50_spi_phy *cr50_phy)
+{
+ struct tpm_tis_spi_phy *phy = &cr50_phy->spi_phy;
+
+ if (cr50_needs_waking(cr50_phy)) {
+ /* Assert CS, wait 1 msec, deassert CS */
+ struct spi_transfer spi_cs_wake = { .delay_usecs = 1000 };
+
+ spi_sync_transfer(phy->spi_device, &spi_cs_wake, 1);
+ /* Wait for it to fully wake */
+ usleep_range(CR50_WAKE_START_DELAY_USEC,
+ CR50_WAKE_START_DELAY_USEC * 2);
+ }
+
+ /* Reset the time when we need to wake Cr50 again */
+ cr50_phy->wake_after = jiffies + msecs_to_jiffies(CR50_SLEEP_DELAY_MSEC);
+}
+
+/*
+ * Flow control: clock the bus and wait for cr50 to set LSB before
+ * sending/receiving data. TCG PTP spec allows it to happen during
+ * the last byte of header, but cr50 never does that in practice,
+ * and earlier versions had a bug when it was set too early, so don't
+ * check for it during header transfer.
+ */
+static int cr50_spi_flow_control(struct tpm_tis_spi_phy *phy,
+ struct spi_transfer *spi_xfer)
+{
+ struct device *dev = &phy->spi_device->dev;
+ unsigned long timeout = jiffies + CR50_FLOW_CONTROL;
+ struct spi_message m;
+ int ret;
+
+ spi_xfer->len = 1;
+
+ do {
+ spi_message_init(&m);
+ spi_message_add_tail(spi_xfer, &m);
+ ret = spi_sync_locked(phy->spi_device, &m);
+ if (ret < 0)
+ return ret;
+
+ if (time_after(jiffies, timeout)) {
+ dev_warn(dev, "Timeout during flow control\n");
+ return -EBUSY;
+ }
+ } while (!(phy->iobuf[0] & 0x01));
+
+ return 0;
+}
+
+static void cr50_spi_reinit_completion(struct tpm_tis_spi_phy *phy)
+{
+ struct cr50_spi_phy *cr50_phy = to_cr50_spi_phy(phy);
+ reinit_completion(&cr50_phy->ready);
+}
+
+static int tpm_tis_spi_cr50_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
+ u8 *in, const u8 *out)
+{
+ struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
+ struct cr50_spi_phy *cr50_phy = to_cr50_spi_phy(phy);
+ int ret;
+
+ mutex_lock(&cr50_phy->time_track_mutex);
+ /*
+ * Do this outside of spi_bus_lock in case cr50 is not the
+ * only device on that spi bus.
+ */
+ cr50_ensure_access_delay(cr50_phy);
+ cr50_wake_if_needed(cr50_phy);
+
+ ret = tpm_tis_spi_transfer(data, addr, len, in, out);
+
+ cr50_phy->last_access = jiffies;
+ mutex_unlock(&cr50_phy->time_track_mutex);
+
+ return ret;
+}
+
+static int tpm_tis_spi_cr50_read_bytes(struct tpm_tis_data *data, u32 addr,
+ u16 len, u8 *result)
+{
+ return tpm_tis_spi_cr50_transfer(data, addr, len, result, NULL);
+}
+
+static int tpm_tis_spi_cr50_write_bytes(struct tpm_tis_data *data, u32 addr,
+ u16 len, const u8 *value)
+{
+ return tpm_tis_spi_cr50_transfer(data, addr, len, NULL, value);
+}
+
+static const struct tpm_tis_phy_ops tpm_spi_cr50_phy_ops = {
+ .read_bytes = tpm_tis_spi_cr50_read_bytes,
+ .write_bytes = tpm_tis_spi_cr50_write_bytes,
+ .read16 = tpm_tis_spi_read16,
+ .read32 = tpm_tis_spi_read32,
+ .write32 = tpm_tis_spi_write32,
+};
+
+static void cr50_print_fw_version(struct tpm_tis_data *data)
+{
+ struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
+ int i, len = 0;
+ char fw_ver[TPM_CR50_MAX_FW_VER_LEN + 1];
+ char fw_ver_block[4];
+
+ /*
+ * Write anything to TPM_CR50_FW_VER to start from the beginning
+ * of the version string
+ */
+ tpm_tis_write8(data, TPM_CR50_FW_VER(data->locality), 0);
+
+ /* Read the string, 4 bytes at a time, until we get '\0' */
+ do {
+ tpm_tis_read_bytes(data, TPM_CR50_FW_VER(data->locality), 4,
+ fw_ver_block);
+ for (i = 0; i < 4 && fw_ver_block[i]; ++len, ++i)
+ fw_ver[len] = fw_ver_block[i];
+ } while (i == 4 && len < TPM_CR50_MAX_FW_VER_LEN);
+ fw_ver[len] = '\0';
+
+ dev_info(&phy->spi_device->dev, "Cr50 firmware version: %s\n", fw_ver);
+}
+
+static int cr50_spi_probe(struct spi_device *spi)
+{
+ struct tpm_tis_spi_phy *phy;
+ struct cr50_spi_phy *cr50_phy;
+ int ret;
+ struct tpm_chip *chip;
+
+ cr50_phy = devm_kzalloc(&spi->dev, sizeof(*cr50_phy), GFP_KERNEL);
+ if (!cr50_phy)
+ return -ENOMEM;
+
+ phy = &cr50_phy->spi_phy;
+ phy->flow_control = cr50_spi_flow_control;
+ phy->pre_transfer = cr50_spi_reinit_completion;
+
+ cr50_phy->access_delay = CR50_NOIRQ_ACCESS_DELAY;
+
+ mutex_init(&cr50_phy->time_track_mutex);
+ cr50_phy->wake_after = jiffies;
+ cr50_phy->last_access = jiffies;
+
+ init_completion(&cr50_phy->ready);
+ if (spi->irq > 0) {
+ ret = devm_request_irq(&spi->dev, spi->irq, cr50_spi_irq_handler,
+ IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+ "cr50_spi", cr50_phy);
+ if (ret < 0) {
+ if (ret == -EPROBE_DEFER)
+ return ret;
+ dev_warn(&spi->dev, "Requesting IRQ %d failed: %d\n",
+ spi->irq, ret);
+ /*
+ * This is not fatal, the driver will fall back to
+ * delays automatically, since ready will never
+ * be completed without a registered irq handler.
+ * So, just fall through.
+ */
+ } else {
+ /*
+ * IRQ requested, let's verify that it is actually
+ * triggered, before relying on it.
+ */
+ cr50_phy->irq_needs_confirmation = true;
+ }
+ } else {
+ dev_warn(&spi->dev,
+ "No IRQ - will use delays between transactions.\n");
+ }
+
+ ret = tpm_tis_spi_init(spi, phy, -1, &tpm_spi_cr50_phy_ops);
+ if (ret)
+ return ret;
+
+ cr50_print_fw_version(&phy->priv);
+
+ chip = dev_get_drvdata(&spi->dev);
+ chip->flags |= TPM_CHIP_FLAG_FIRMWARE_POWER_MANAGED;
+
+ return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int cr50_spi_resume(struct device *dev)
+{
+ struct tpm_chip *chip = dev_get_drvdata(dev);
+ struct tpm_tis_data *data = dev_get_drvdata(&chip->dev);
+ struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
+ struct cr50_spi_phy *cr50_phy = to_cr50_spi_phy(phy);
+
+ /*
+ * Jiffies not increased during suspend, so we need to reset
+ * the time to wake Cr50 after resume.
+ */
+ cr50_phy->wake_after = jiffies;
+
+ return tpm_tis_resume(dev);
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(cr50_spi_pm, tpm_pm_suspend, cr50_spi_resume);
+
+static int cr50_spi_remove(struct spi_device *dev)
+{
+ struct tpm_chip *chip = spi_get_drvdata(dev);
+
+ tpm_chip_unregister(chip);
+ tpm_tis_remove(chip);
+ return 0;
+}
+
+static const struct spi_device_id cr50_spi_id[] = {
+ { "cr50", 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(spi, cr50_spi_id);
+
+#ifdef CONFIG_OF
+static const struct of_device_id of_cr50_spi_match[] = {
+ { .compatible = "google,cr50", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, of_cr50_spi_match);
+#endif
+
+static struct spi_driver cr50_spi_driver = {
+ .driver = {
+ .name = "cr50_spi",
+ .pm = &cr50_spi_pm,
+ .of_match_table = of_match_ptr(of_cr50_spi_match),
+ },
+ .probe = cr50_spi_probe,
+ .remove = cr50_spi_remove,
+ .id_table = cr50_spi_id,
+};
+module_spi_driver(cr50_spi_driver);
+
+MODULE_DESCRIPTION("Cr50 TCG PTP FIFO SPI driver");
+MODULE_LICENSE("GPL v2");
--
Sent by a computer through tubes
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4 0/6] tpm: Add driver for cr50
2019-08-12 22:36 [PATCH v4 0/6] tpm: Add driver for cr50 Stephen Boyd
` (5 preceding siblings ...)
2019-08-12 22:36 ` [PATCH v4 6/6] tpm: add driver for cr50 on SPI Stephen Boyd
@ 2019-08-27 0:58 ` Heiko Stuebner
6 siblings, 0 replies; 20+ messages in thread
From: Heiko Stuebner @ 2019-08-27 0:58 UTC (permalink / raw)
To: Stephen Boyd
Cc: Peter Huewe, Jarkko Sakkinen, linux-kernel, linux-integrity,
Andrey Pronin, Duncan Laurie, Jason Gunthorpe, Arnd Bergmann,
Greg Kroah-Hartman, Guenter Roeck, Alexander Steffen
Am Dienstag, 13. August 2019, 00:36:16 CEST schrieb Stephen Boyd:
> This patch series adds support for the the H1 secure microcontroller
> running cr50 firmware found on various recent Chromebooks. This driver
> is necessary to boot into a ChromeOS userspace environment. It
> implements support for several functions, including TPM-like
> functionality over a SPI interface.
>
> The last time this was series sent looks to be [1]. I've looked over the
> patches and review comments and tried to address any feedback that
> Andrey didn't address (really minor things like newlines). I've reworked
> the patches from the last version to layer on top of the existing TPM
> TIS SPI implementation in tpm_tis_spi.c. Hopefully this is more
> palatable than combining the two drivers together into one file.
>
> [1] https://lkml.kernel.org/r/1469757314-116169-1-git-send-email-apronin@chromium.org
Gave this a spin on a rk3399-gru-scarlet and it seems to have worked fine
and tpm2-tools was happy talking to it, so
Tested-by: Heiko Stuebner <heiko@sntech.de>
From looking through the patches everything also looks nice and peachy
but my tpm-insights are limited so I don't really feel comfortable with a RB.
Heiko
^ permalink raw reply [flat|nested] 20+ messages in thread