| Message ID | 20190812223622.73297-3-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tpm: Add driver for cr50 | expand |
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
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.
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
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)
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(-)