2023-03-02 04:19:27

by Krishna Yarlagadda

[permalink] [raw]
Subject: [Patch V8 0/3] Tegra TPM driver with HW flow control

TPM devices may insert wait state on last clock cycle of ADDR phase.
For SPI controllers that support full-duplex transfers, this can be
detected using software by reading the MISO line. For SPI controllers
that only support half-duplex transfers, such as the Tegra QSPI, it is
not possible to detect the wait signal from software. The QSPI
controller in Tegra234 and Tegra241 implement hardware detection of the
wait signal which can be enabled in the controller for TPM devices.

Add HW flow control in TIS driver and a flag in SPI data to indicate
wait detection is required in HW. SPI controller driver determines if
this is supported. Add HW detection in Tegra QSPI controller.

Updates in this patch set
- Tegra QSPI identifies itself as half duplex.
- TPM TIS SPI driver skips flow control for half duplex and send
transfers in single message for controller to handle it.
- TPM device identifies as TPM device for controller to detect and
enable HW TPM wait poll feature.

Verified with a TPM device on Tegra241 ref board using TPM2 tools.

V8:
- fix compile warning.
V7:
- updated patch description.
- TPM flag set in probe.
- minor comments.
V6:
- Fix typo in chip name Tegra234.
- Debug logs change skipped to be sent later.
- Consistent usage of soc flag.
V5:
- No SPI bus locking.
V4:
- Split api change to different patch.
- Describe TPM HW flow control.
V3:
- Use SPI device mode flag and SPI controller flags.
- Drop usage of device tree flags.
- Generic TPM half duplex controller handling.
- HW & SW flow control for TPM. Drop additional driver.
V2:
- Fix dt schema errors.

Krishna Yarlagadda (3):
spi: Add TPM HW flow flag
tpm_tis-spi: Add hardware wait polling
spi: tegra210-quad: Enable TPM wait polling

drivers/char/tpm/tpm_tis_spi_main.c | 95 ++++++++++++++++++++++++++++-
drivers/spi/spi-tegra210-quad.c | 14 +++++
include/linux/spi/spi.h | 16 ++++-
3 files changed, 120 insertions(+), 5 deletions(-)

--
2.17.1



2023-03-02 04:19:32

by Krishna Yarlagadda

[permalink] [raw]
Subject: [Patch V8 2/3] tpm_tis-spi: Add hardware wait polling

TPM devices may insert wait state on last clock cycle of ADDR phase.
For SPI controllers that support full-duplex transfers, this can be
detected using software by reading the MISO line. For SPI controllers
that only support half-duplex transfers, such as the Tegra QSPI, it is
not possible to detect the wait signal from software. The QSPI
controller in Tegra234 and Tegra241 implement hardware detection of the
wait signal which can be enabled in the controller for TPM devices.

The current TPM TIS driver only supports software detection of the wait
signal. To support SPI controllers that use hardware to detect the wait
signal, add the function tpm_tis_spi_hw_flow_transfer() and move the
existing code for software based detection into a function called
tpm_tis_spi_sw_flow_transfer(). SPI controllers that only support
half-duplex transfers will always call tpm_tis_spi_hw_flow_transfer()
because they cannot support software based detection. The bit
SPI_TPM_HW_FLOW is set to indicate to the SPI controller that hardware
detection is required and it is the responsibility of the SPI controller
driver to determine if this is supported or not.

For hardware flow control, CMD-ADDR-DATA messages are combined into a
single message where as for software flow control exiting method of
CMD-ADDR in a message and DATA in another is followed.

Signed-off-by: Krishna Yarlagadda <[email protected]>
---
drivers/char/tpm/tpm_tis_spi_main.c | 95 ++++++++++++++++++++++++++++-
1 file changed, 93 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c
index a0963a3e92bd..d0c1073bfa06 100644
--- a/drivers/char/tpm/tpm_tis_spi_main.c
+++ b/drivers/char/tpm/tpm_tis_spi_main.c
@@ -71,8 +71,76 @@ static int tpm_tis_spi_flow_control(struct tpm_tis_spi_phy *phy,
return 0;
}

-int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
- u8 *in, const u8 *out)
+/*
+ * Half duplex controller with support for TPM wait state detection like
+ * Tegra QSPI need CMD, ADDR & DATA sent in single message to manage HW flow
+ * control. Each phase sent in different transfer for controller to idenity
+ * phase.
+ */
+static int tpm_tis_spi_hw_flow_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 spi_transfer spi_xfer[3];
+ struct spi_message m;
+ u8 transfer_len;
+ int ret;
+
+ while (len) {
+ transfer_len = min_t(u16, len, MAX_SPI_FRAMESIZE);
+
+ spi_message_init(&m);
+ phy->iobuf[0] = (in ? 0x80 : 0) | (transfer_len - 1);
+ phy->iobuf[1] = 0xd4;
+ phy->iobuf[2] = addr >> 8;
+ phy->iobuf[3] = addr;
+
+ memset(&spi_xfer, 0, sizeof(spi_xfer));
+
+ spi_xfer[0].tx_buf = phy->iobuf;
+ spi_xfer[0].len = 1;
+ spi_message_add_tail(&spi_xfer[0], &m);
+
+ spi_xfer[1].tx_buf = phy->iobuf + 1;
+ spi_xfer[1].len = 3;
+ spi_message_add_tail(&spi_xfer[1], &m);
+
+ if (out) {
+ spi_xfer[2].tx_buf = &phy->iobuf[4];
+ spi_xfer[2].rx_buf = NULL;
+ memcpy(&phy->iobuf[4], out, transfer_len);
+ out += transfer_len;
+ }
+
+ if (in) {
+ spi_xfer[2].tx_buf = NULL;
+ spi_xfer[2].rx_buf = &phy->iobuf[4];
+ }
+
+ spi_xfer[2].len = transfer_len;
+ spi_message_add_tail(&spi_xfer[2], &m);
+
+ reinit_completion(&phy->ready);
+
+ ret = spi_sync_locked(phy->spi_device, &m);
+ if (ret < 0)
+ return ret;
+
+ if (in) {
+ memcpy(in, &phy->iobuf[4], transfer_len);
+ in += transfer_len;
+ }
+
+ len -= transfer_len;
+ }
+
+ return ret;
+}
+
+static int tpm_tis_spi_sw_flow_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;
@@ -140,6 +208,26 @@ int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
return ret;
}

+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);
+ struct spi_controller *ctlr = phy->spi_device->controller;
+
+ /*
+ * TPM flow control over SPI requires full duplex support.
+ * Send entire message to a half duplex controller to handle
+ * wait polling in controller.
+ * Set TPM HW flow control flag..
+ */
+ if (ctlr->flags & SPI_CONTROLLER_HALF_DUPLEX)
+ return tpm_tis_spi_hw_flow_transfer(data, addr, len, in,
+ out);
+ else
+ return tpm_tis_spi_sw_flow_transfer(data, addr, len, in,
+ out);
+}
+
static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
u16 len, u8 *result, enum tpm_tis_io_mode io_mode)
{
@@ -181,6 +269,9 @@ static int tpm_tis_spi_probe(struct spi_device *dev)

phy->flow_control = tpm_tis_spi_flow_control;

+ if (dev->controller->flags & SPI_CONTROLLER_HALF_DUPLEX)
+ dev->mode |= SPI_TPM_HW_FLOW;
+
/* If the SPI device has an IRQ then use that */
if (dev->irq > 0)
irq = dev->irq;
--
2.17.1


2023-03-02 04:19:42

by Krishna Yarlagadda

[permalink] [raw]
Subject: [Patch V8 3/3] spi: tegra210-quad: Enable TPM wait polling

Trusted Platform Module requires flow control. As defined in TPM
interface specification, client would drive MISO line at same cycle as
last address bit on MOSI.
Tegra234 and Tegra241 QSPI controllers have TPM wait state detection
feature which is enabled for TPM client devices reported in SPI device
mode bits.

Signed-off-by: Krishna Yarlagadda <[email protected]>
---
drivers/spi/spi-tegra210-quad.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)

diff --git a/drivers/spi/spi-tegra210-quad.c b/drivers/spi/spi-tegra210-quad.c
index 0b9bc3b7f53a..82aec6cb7863 100644
--- a/drivers/spi/spi-tegra210-quad.c
+++ b/drivers/spi/spi-tegra210-quad.c
@@ -142,6 +142,7 @@

#define QSPI_GLOBAL_CONFIG 0X1a4
#define QSPI_CMB_SEQ_EN BIT(0)
+#define QSPI_TPM_WAIT_POLL_EN BIT(1)

#define QSPI_CMB_SEQ_ADDR 0x1a8
#define QSPI_ADDRESS_VALUE_SET(X) (((x) & 0xFFFF) << 0)
@@ -164,6 +165,7 @@
struct tegra_qspi_soc_data {
bool has_dma;
bool cmb_xfer_capable;
+ bool supports_tpm;
unsigned int cs_count;
};

@@ -1065,6 +1067,12 @@ static int tegra_qspi_combined_seq_xfer(struct tegra_qspi *tqspi,

/* Enable Combined sequence mode */
val = tegra_qspi_readl(tqspi, QSPI_GLOBAL_CONFIG);
+ if (spi->mode & SPI_TPM_HW_FLOW) {
+ if (tqspi->soc_data->supports_tpm)
+ val |= QSPI_TPM_WAIT_POLL_EN;
+ else
+ return -EIO;
+ }
val |= QSPI_CMB_SEQ_EN;
tegra_qspi_writel(tqspi, val, QSPI_GLOBAL_CONFIG);
/* Process individual transfer list */
@@ -1196,6 +1204,8 @@ static int tegra_qspi_non_combined_seq_xfer(struct tegra_qspi *tqspi,
/* Disable Combined sequence mode */
val = tegra_qspi_readl(tqspi, QSPI_GLOBAL_CONFIG);
val &= ~QSPI_CMB_SEQ_EN;
+ if (tqspi->soc_data->supports_tpm)
+ val &= ~QSPI_TPM_WAIT_POLL_EN;
tegra_qspi_writel(tqspi, val, QSPI_GLOBAL_CONFIG);
list_for_each_entry(transfer, &msg->transfers, transfer_list) {
struct spi_transfer *xfer = transfer;
@@ -1454,24 +1464,28 @@ static irqreturn_t tegra_qspi_isr_thread(int irq, void *context_data)
static struct tegra_qspi_soc_data tegra210_qspi_soc_data = {
.has_dma = true,
.cmb_xfer_capable = false,
+ .supports_tpm = false,
.cs_count = 1,
};

static struct tegra_qspi_soc_data tegra186_qspi_soc_data = {
.has_dma = true,
.cmb_xfer_capable = true,
+ .supports_tpm = false,
.cs_count = 1,
};

static struct tegra_qspi_soc_data tegra234_qspi_soc_data = {
.has_dma = false,
.cmb_xfer_capable = true,
+ .supports_tpm = true,
.cs_count = 1,
};

static struct tegra_qspi_soc_data tegra241_qspi_soc_data = {
.has_dma = false,
.cmb_xfer_capable = true,
+ .supports_tpm = true,
.cs_count = 4,
};

--
2.17.1


2023-03-11 21:49:09

by Jarkko Sakkinen

[permalink] [raw]
Subject: Re: [Patch V8 2/3] tpm_tis-spi: Add hardware wait polling

On Thu, 2023-03-02 at 09:48 +0530, Krishna Yarlagadda wrote:
> +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);
> +       struct spi_controller *ctlr = phy->spi_device->controller;
> +
> +       /*
> +        * TPM flow control over SPI requires full duplex support.
> +        * Send entire message to a half duplex controller to handle
> +        * wait polling in controller.
> +        * Set TPM HW flow control flag..
> +        */
> +       if (ctlr->flags & SPI_CONTROLLER_HALF_DUPLEX)
> +               return tpm_tis_spi_hw_flow_transfer(data, addr, len,
> in,
> +                                                   out);
> +       else
> +               return tpm_tis_spi_sw_flow_transfer(data, addr, len,
> in,
> +                                                   out);
> +}
> +

Based on the condition, better names would be

1. tpm_tis_spi_transfer_half()
2. tpm_tis_spi_transfer_full()

BR, Jarkko

2023-03-15 15:47:40

by Krishna Yarlagadda

[permalink] [raw]
Subject: RE: [Patch V8 2/3] tpm_tis-spi: Add hardware wait polling


> -----Original Message-----
> From: Jarkko Sakkinen <[email protected]>
> Sent: 12 March 2023 03:19
> To: Krishna Yarlagadda <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; linux-
> [email protected]
> Cc: [email protected]; Jonathan Hunter <[email protected]>;
> Sowjanya Komatineni <[email protected]>; Laxman Dewangan
> <[email protected]>
> Subject: Re: [Patch V8 2/3] tpm_tis-spi: Add hardware wait polling
>
> External email: Use caution opening links or attachments
>
>
> On Thu, 2023-03-02 at 09:48 +0530, Krishna Yarlagadda wrote:
> > +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);
> > + struct spi_controller *ctlr = phy->spi_device->controller;
> > +
> > + /*
> > + * TPM flow control over SPI requires full duplex support.
> > + * Send entire message to a half duplex controller to handle
> > + * wait polling in controller.
> > + * Set TPM HW flow control flag..
> > + */
> > + if (ctlr->flags & SPI_CONTROLLER_HALF_DUPLEX)
> > + return tpm_tis_spi_hw_flow_transfer(data, addr, len,
> > in,
> > + out);
> > + else
> > + return tpm_tis_spi_sw_flow_transfer(data, addr, len,
> > in,
> > + out);
> > +}
> > +
>
> Based on the condition, better names would be
Though condition is based on half duplex, functions are implementing
HW or SW flow of the transfer.
KY
>
> 1. tpm_tis_spi_transfer_half()
> 2. tpm_tis_spi_transfer_full()
>
> BR, Jarkko

2023-03-19 13:42:43

by Jarkko Sakkinen

[permalink] [raw]
Subject: Re: [Patch V8 2/3] tpm_tis-spi: Add hardware wait polling

On Wed, Mar 15, 2023 at 03:47:33PM +0000, Krishna Yarlagadda wrote:
>
> > -----Original Message-----
> > From: Jarkko Sakkinen <[email protected]>
> > Sent: 12 March 2023 03:19
> > To: Krishna Yarlagadda <[email protected]>; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; linux-
> > [email protected]; [email protected]; linux-
> > [email protected]
> > Cc: [email protected]; Jonathan Hunter <[email protected]>;
> > Sowjanya Komatineni <[email protected]>; Laxman Dewangan
> > <[email protected]>
> > Subject: Re: [Patch V8 2/3] tpm_tis-spi: Add hardware wait polling
> >
> > External email: Use caution opening links or attachments
> >
> >
> > On Thu, 2023-03-02 at 09:48 +0530, Krishna Yarlagadda wrote:
> > > +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);
> > > + struct spi_controller *ctlr = phy->spi_device->controller;
> > > +
> > > + /*
> > > + * TPM flow control over SPI requires full duplex support.
> > > + * Send entire message to a half duplex controller to handle
> > > + * wait polling in controller.
> > > + * Set TPM HW flow control flag..
> > > + */
> > > + if (ctlr->flags & SPI_CONTROLLER_HALF_DUPLEX)
> > > + return tpm_tis_spi_hw_flow_transfer(data, addr, len,
> > > in,
> > > + out);
> > > + else
> > > + return tpm_tis_spi_sw_flow_transfer(data, addr, len,
> > > in,
> > > + out);
> > > +}
> > > +
> >
> > Based on the condition, better names would be
> Though condition is based on half duplex, functions are implementing
> HW or SW flow of the transfer.

Both are hardwaw flows in the sense that you are controlling a piece of hardware.

BR, Jarkko

2023-03-23 12:01:17

by Krishna Yarlagadda

[permalink] [raw]
Subject: RE: [Patch V8 2/3] tpm_tis-spi: Add hardware wait polling

> -----Original Message-----
> From: Hillf Danton <[email protected]>
> Sent: 19 March 2023 19:58
> To: Krishna Yarlagadda <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]
> Subject: Re: [Patch V8 2/3] tpm_tis-spi: Add hardware wait polling
>
> External email: Use caution opening links or attachments
>
>
> On 2 Mar 2023 09:48:03 +0530 Krishna Yarlagadda <[email protected]>
> > +static int tpm_tis_spi_hw_flow_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 spi_transfer spi_xfer[3];
> > + struct spi_message m;
> > + u8 transfer_len;
> > + int ret;
> > +
> > + while (len) {
> > + transfer_len = min_t(u16, len, MAX_SPI_FRAMESIZE);
> > +
> > + spi_message_init(&m);
> > + phy->iobuf[0] = (in ? 0x80 : 0) | (transfer_len - 1);
> > + phy->iobuf[1] = 0xd4;
> > + phy->iobuf[2] = addr >> 8;
> > + phy->iobuf[3] = addr;
> > +
> > + memset(&spi_xfer, 0, sizeof(spi_xfer));
> > +
> > + spi_xfer[0].tx_buf = phy->iobuf;
> > + spi_xfer[0].len = 1;
> > + spi_message_add_tail(&spi_xfer[0], &m);
> > +
> > + spi_xfer[1].tx_buf = phy->iobuf + 1;
> > + spi_xfer[1].len = 3;
> > + spi_message_add_tail(&spi_xfer[1], &m);
> > +
> > + if (out) {
> > + spi_xfer[2].tx_buf = &phy->iobuf[4];
> > + spi_xfer[2].rx_buf = NULL;
> > + memcpy(&phy->iobuf[4], out, transfer_len);
> > + out += transfer_len;
> > + }
> > +
> > + if (in) {
> > + spi_xfer[2].tx_buf = NULL;
> > + spi_xfer[2].rx_buf = &phy->iobuf[4];
> > + }
> > +
> > + spi_xfer[2].len = transfer_len;
> > + spi_message_add_tail(&spi_xfer[2], &m);
> > +
> > + reinit_completion(&phy->ready);
>
> What breaks without reinit? Or what sense made by init-ing it again?
When length is over frame size, this loop will run for more than
one iterations. Reinit to start transfer again.
KY
> > +
> > + ret = spi_sync_locked(phy->spi_device, &m);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (in) {
> > + memcpy(in, &phy->iobuf[4], transfer_len);
> > + in += transfer_len;
> > + }
> > +
> > + len -= transfer_len;
> > + }
> > +
> > + return ret;
> > +}

2023-03-23 12:02:07

by Krishna Yarlagadda

[permalink] [raw]
Subject: RE: [Patch V8 2/3] tpm_tis-spi: Add hardware wait polling

> -----Original Message-----
> From: Jarkko Sakkinen <[email protected]>
> Sent: 19 March 2023 19:13
> To: Krishna Yarlagadda <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; Jonathan Hunter
> <[email protected]>; Sowjanya Komatineni
> <[email protected]>; Laxman Dewangan <[email protected]>
> Subject: Re: [Patch V8 2/3] tpm_tis-spi: Add hardware wait polling
>
> External email: Use caution opening links or attachments
>
>
> On Wed, Mar 15, 2023 at 03:47:33PM +0000, Krishna Yarlagadda wrote:
> >
> > > -----Original Message-----
> > > From: Jarkko Sakkinen <[email protected]>
> > > Sent: 12 March 2023 03:19
> > > To: Krishna Yarlagadda <[email protected]>; [email protected];
> > > [email protected]; [email protected]; [email protected];
> > > [email protected]; [email protected]; linux-
> > > [email protected]; [email protected]; linux-
> > > [email protected]
> > > Cc: [email protected]; Jonathan Hunter
> <[email protected]>;
> > > Sowjanya Komatineni <[email protected]>; Laxman Dewangan
> > > <[email protected]>
> > > Subject: Re: [Patch V8 2/3] tpm_tis-spi: Add hardware wait polling
> > >
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > On Thu, 2023-03-02 at 09:48 +0530, Krishna Yarlagadda wrote:
> > > > +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);
> > > > + struct spi_controller *ctlr = phy->spi_device->controller;
> > > > +
> > > > + /*
> > > > + * TPM flow control over SPI requires full duplex support.
> > > > + * Send entire message to a half duplex controller to handle
> > > > + * wait polling in controller.
> > > > + * Set TPM HW flow control flag..
> > > > + */
> > > > + if (ctlr->flags & SPI_CONTROLLER_HALF_DUPLEX)
> > > > + return tpm_tis_spi_hw_flow_transfer(data, addr, len,
> > > > in,
> > > > + out);
> > > > + else
> > > > + return tpm_tis_spi_sw_flow_transfer(data, addr, len,
> > > > in,
> > > > + out);
> > > > +}
> > > > +
> > >
> > > Based on the condition, better names would be
> > Though condition is based on half duplex, functions are implementing
> > HW or SW flow of the transfer.
>
> Both are hardwaw flows in the sense that you are controlling a piece of
> hardware.
>
> BR, Jarkko
Yes. I will push new version with naming suggested.
KY