diff mbox series

[V3,1/3] tpm_tis-spi: Support hardware wait polling

Message ID 20230223162635.19747-2-kyarlagadda@nvidia.com (mailing list archive)
State Superseded
Headers show
Series Tegra TPM driver with HW flow control | expand

Commit Message

Krishna Yarlagadda Feb. 23, 2023, 4:26 p.m. UTC
TPM devices raise wait signal on last addr cycle. This can be detected
by software driver by reading MISO line on same clock which requires
full duplex support. In case of half duplex controllers wait detection
has to be implemented in HW.
Support hardware wait state detection by sending entire message and let
controller handle flow control.
QSPI controller in Tegra236 & Tegra241 implement TPM wait polling.

Signed-off-by: Krishna Yarlagadda <kyarlagadda@nvidia.com>
---
 drivers/char/tpm/tpm_tis_spi_main.c | 90 ++++++++++++++++++++++++++++-
 include/linux/spi/spi.h             |  7 ++-
 2 files changed, 92 insertions(+), 5 deletions(-)

Comments

Mark Brown Feb. 23, 2023, 5:18 p.m. UTC | #1
On Thu, Feb 23, 2023 at 09:56:33PM +0530, Krishna Yarlagadda wrote:

> +       spi_bus_lock(phy->spi_device->master);
> +
> +       while (len) {

Why?

> +		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);

Why would we make these two separate transfers?

> +		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];
> +		}

This will use the same buffer for rx and tx if some bug manages to leave
them both set.  That shouldn't be an issue but it's an alarm bell
reading the code.

> index 988aabc31871..b88494e31239 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -184,8 +184,9 @@ struct spi_device {
>  	u8			chip_select;
>  	u8			bits_per_word;
>  	bool			rt;
> -#define SPI_NO_TX	BIT(31)		/* No transmit wire */
> -#define SPI_NO_RX	BIT(30)		/* No receive wire */
> +#define SPI_NO_TX		BIT(31)		/* No transmit wire */
> +#define SPI_NO_RX		BIT(30)		/* No receive wire */
> +#define SPI_TPM_HW_FLOW		BIT(29)		/* TPM flow control */

Additions to the SPI API should be a separate commit for SPI rather than
merged into a driver change.
Krishna Yarlagadda Feb. 23, 2023, 6:41 p.m. UTC | #2
> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: 23 February 2023 22:49
> To: Krishna Yarlagadda <kyarlagadda@nvidia.com>
> Cc: robh+dt@kernel.org; peterhuewe@gmx.de; jgg@ziepe.ca;
> jarkko@kernel.org; krzysztof.kozlowski+dt@linaro.org; linux-
> spi@vger.kernel.org; linux-tegra@vger.kernel.org; linux-
> integrity@vger.kernel.org; linux-kernel@vger.kernel.org;
> thierry.reding@gmail.com; Jonathan Hunter <jonathanh@nvidia.com>;
> Sowjanya Komatineni <skomatineni@nvidia.com>; Laxman Dewangan
> <ldewangan@nvidia.com>
> Subject: Re: [Patch V3 1/3] tpm_tis-spi: Support hardware wait polling
> 
> On Thu, Feb 23, 2023 at 09:56:33PM +0530, Krishna Yarlagadda wrote:
> 
> > +       spi_bus_lock(phy->spi_device->master);
> > +
> > +       while (len) {
> 
> Why?
TPM supports max 64B in single transaction. Loop to send rest of it.
> 
> > +		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);
> 
> Why would we make these two separate transfers?
Tegra QSPI combined sequence requires cmd, addr and data in different
transfers. This eliminates need for additional flag for combined sequence.
> 
> > +		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];
> > +		}
> 
> This will use the same buffer for rx and tx if some bug manages to leave
> them both set.  That shouldn't be an issue but it's an alarm bell
> reading the code.
> 
> > index 988aabc31871..b88494e31239 100644
> > --- a/include/linux/spi/spi.h
> > +++ b/include/linux/spi/spi.h
> > @@ -184,8 +184,9 @@ struct spi_device {
> >  	u8			chip_select;
> >  	u8			bits_per_word;
> >  	bool			rt;
> > -#define SPI_NO_TX	BIT(31)		/* No transmit wire */
> > -#define SPI_NO_RX	BIT(30)		/* No receive wire */
> > +#define SPI_NO_TX		BIT(31)		/* No transmit wire */
> > +#define SPI_NO_RX		BIT(30)		/* No receive wire */
> > +#define SPI_TPM_HW_FLOW		BIT(29)		/* TPM flow
> control */
> 
> Additions to the SPI API should be a separate commit for SPI rather than
> merged into a driver change.
Will split this into new patch.
Mark Brown Feb. 23, 2023, 6:43 p.m. UTC | #3
On Thu, Feb 23, 2023 at 06:41:43PM +0000, Krishna Yarlagadda wrote:

> > > +       spi_bus_lock(phy->spi_device->master);
> > > +
> > > +       while (len) {

> > Why?

> TPM supports max 64B in single transaction. Loop to send rest of it.

No, why is there a bus lock?

> > > +		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);

> > Why would we make these two separate transfers?

> Tegra QSPI combined sequence requires cmd, addr and data in different
> transfers. This eliminates need for additional flag for combined sequence.

That needs some documentation, and we might need a flag to ensure the
core doesn't coalesce the transfers.
Krishna Yarlagadda Feb. 24, 2023, 2:16 p.m. UTC | #4
> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: 24 February 2023 00:13
> To: Krishna Yarlagadda <kyarlagadda@nvidia.com>
> Cc: robh+dt@kernel.org; peterhuewe@gmx.de; jgg@ziepe.ca;
> jarkko@kernel.org; krzysztof.kozlowski+dt@linaro.org; linux-
> spi@vger.kernel.org; linux-tegra@vger.kernel.org; linux-
> integrity@vger.kernel.org; linux-kernel@vger.kernel.org;
> thierry.reding@gmail.com; Jonathan Hunter <jonathanh@nvidia.com>;
> Sowjanya Komatineni <skomatineni@nvidia.com>; Laxman Dewangan
> <ldewangan@nvidia.com>
> Subject: Re: [Patch V3 1/3] tpm_tis-spi: Support hardware wait polling
> 
> On Thu, Feb 23, 2023 at 06:41:43PM +0000, Krishna Yarlagadda wrote:
> 
> > > > +       spi_bus_lock(phy->spi_device->master);
> > > > +
> > > > +       while (len) {
> 
> > > Why?
> 
> > TPM supports max 64B in single transaction. Loop to send rest of it.
> 
> No, why is there a bus lock?
Bus lock to avoid other clients to be accessed between TPM transfers.

> 
> > > > +		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);
> 
> > > Why would we make these two separate transfers?
> 
> > Tegra QSPI combined sequence requires cmd, addr and data in different
> > transfers. This eliminates need for additional flag for combined sequence.
> 
> That needs some documentation, and we might need a flag to ensure the
> core doesn't coalesce the transfers.
Will add comment at top of the function. Bus lock should avoid coalesce of
transfer of single message from others.
KY
Mark Brown Feb. 24, 2023, 3:51 p.m. UTC | #5
On Fri, Feb 24, 2023 at 02:16:27PM +0000, Krishna Yarlagadda wrote:

> > > > > +       spi_bus_lock(phy->spi_device->master);
> > > > > +
> > > > > +       while (len) {

> > > > Why?

> > > TPM supports max 64B in single transaction. Loop to send rest of it.

> > No, why is there a bus lock?

> Bus lock to avoid other clients to be accessed between TPM transfers.

That's what a bus lock does but what would be the problem if something
else sent a message between messages?  Note that a message will always
be sent atomically.
Krishna Yarlagadda Feb. 24, 2023, 4:21 p.m. UTC | #6
> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: 24 February 2023 21:22
> To: Krishna Yarlagadda <kyarlagadda@nvidia.com>
> Cc: robh+dt@kernel.org; peterhuewe@gmx.de; jgg@ziepe.ca;
> jarkko@kernel.org; krzysztof.kozlowski+dt@linaro.org; linux-
> spi@vger.kernel.org; linux-tegra@vger.kernel.org; linux-
> integrity@vger.kernel.org; linux-kernel@vger.kernel.org;
> thierry.reding@gmail.com; Jonathan Hunter <jonathanh@nvidia.com>;
> Sowjanya Komatineni <skomatineni@nvidia.com>; Laxman Dewangan
> <ldewangan@nvidia.com>
> Subject: Re: [Patch V3 1/3] tpm_tis-spi: Support hardware wait polling
> 
> On Fri, Feb 24, 2023 at 02:16:27PM +0000, Krishna Yarlagadda wrote:
> 
> > > > > > +       spi_bus_lock(phy->spi_device->master);
> > > > > > +
> > > > > > +       while (len) {
> 
> > > > > Why?
> 
> > > > TPM supports max 64B in single transaction. Loop to send rest of it.
> 
> > > No, why is there a bus lock?
> 
> > Bus lock to avoid other clients to be accessed between TPM transfers.
> 
> That's what a bus lock does but what would be the problem if something
> else sent a message between messages?  Note that a message will always
> be sent atomically.
QSPI has multi-chip-select support. Idea was to prevent transfers from both
devices at the same time. But it should be fine if it is single message.
I will verify with bus lock removed.
diff mbox series

Patch

diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c
index a0963a3e92bd..0d3da7ef9a89 100644
--- a/drivers/char/tpm/tpm_tis_spi_main.c
+++ b/drivers/char/tpm/tpm_tis_spi_main.c
@@ -71,8 +71,72 @@  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)
+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;
+
+	spi_bus_lock(phy->spi_device->master);
+
+	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)
+			goto exit;
+
+		if (in) {
+			memcpy(in, &phy->iobuf[4], transfer_len);
+			in += transfer_len;
+		}
+
+		len -= transfer_len;
+	}
+
+exit:
+	spi_bus_unlock(phy->spi_device->master);
+	return ret;
+}
+
+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 +204,28 @@  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) {
+		phy->spi_device->mode |= SPI_TPM_HW_FLOW;
+		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)
 {
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 988aabc31871..b88494e31239 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -184,8 +184,9 @@  struct spi_device {
 	u8			chip_select;
 	u8			bits_per_word;
 	bool			rt;
-#define SPI_NO_TX	BIT(31)		/* No transmit wire */
-#define SPI_NO_RX	BIT(30)		/* No receive wire */
+#define SPI_NO_TX		BIT(31)		/* No transmit wire */
+#define SPI_NO_RX		BIT(30)		/* No receive wire */
+#define SPI_TPM_HW_FLOW		BIT(29)		/* TPM flow control */
 	/*
 	 * All bits defined above should be covered by SPI_MODE_KERNEL_MASK.
 	 * The SPI_MODE_KERNEL_MASK has the SPI_MODE_USER_MASK counterpart,
@@ -195,7 +196,7 @@  struct spi_device {
 	 * These bits must not overlap. A static assert check should make sure of that.
 	 * If adding extra bits, make sure to decrease the bit index below as well.
 	 */
-#define SPI_MODE_KERNEL_MASK	(~(BIT(30) - 1))
+#define SPI_MODE_KERNEL_MASK	(~(BIT(29) - 1))
 	u32			mode;
 	int			irq;
 	void			*controller_state;