[Patch V3 0/3] Tegra TPM driver with HW flow control

[Patch V3 0/3] Tegra TPM driver with HW flow control

[Krishna Yarlagadda]

TPM interface spec defines flow control where TPM device would drive
MISO at same cycle as last address bit sent by controller on MOSI. This
state of wait can be detected by software reading the MISO line or
by controller hardware. Support sending transfers to controller in
single message and handle flow control in hardware. Half duplex
controllers have to support flow control in hardware.

Tegra234 and Tegra241 chips have QSPI controller that supports TPM
Interface Specification (TIS) flow control.
Since the controller only supports half duplex, SW wait polling
(flow control using full duplex transfers) method implemented in
tpm_tis_spi_main.c will not work and have to us HW flow control.

Updates in this patchset 
 - 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.

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):
  tpm_tis-spi: Support hardware wait polling
  spi: tegra210-quad: set half duplex flag
  spi: tegra210-quad: Enable TPM wait polling

 drivers/char/tpm/tpm_tis_spi_main.c | 90 ++++++++++++++++++++++++++++-
 drivers/spi/spi-tegra210-quad.c     | 22 +++++++
 include/linux/spi/spi.h             |  7 ++-
 3 files changed, 114 insertions(+), 5 deletions(-)

-- 
2.17.1

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

[Krishna Yarlagadda]

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(-)

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;
-- 
2.17.1

[Patch V3 2/3] spi: tegra210-quad: set half duplex flag

[Krishna Yarlagadda]

Tegra QSPI controller only supports half duplex transfers.
Set half duplex constrain flag.

Signed-off-by: Krishna Yarlagadda <kyarlagadda@nvidia.com>
---
 drivers/spi/spi-tegra210-quad.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/spi/spi-tegra210-quad.c b/drivers/spi/spi-tegra210-quad.c
index 9ddb02fc6f1b..b967576b6c96 100644
--- a/drivers/spi/spi-tegra210-quad.c
+++ b/drivers/spi/spi-tegra210-quad.c
@@ -1532,6 +1532,7 @@ static int tegra_qspi_probe(struct platform_device *pdev)
 	master->mode_bits = SPI_MODE_0 | SPI_MODE_3 | SPI_CS_HIGH |
 			    SPI_TX_DUAL | SPI_RX_DUAL | SPI_TX_QUAD | SPI_RX_QUAD;
 	master->bits_per_word_mask = SPI_BPW_MASK(32) | SPI_BPW_MASK(16) | SPI_BPW_MASK(8);
+	master->flags = SPI_CONTROLLER_HALF_DUPLEX;
 	master->setup = tegra_qspi_setup;
 	master->transfer_one_message = tegra_qspi_transfer_one_message;
 	master->num_chipselect = 1;
-- 
2.17.1

[Patch V3 3/3] spi: tegra210-quad: Enable TPM wait polling

[Krishna Yarlagadda]

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.
Tegra241 QSPI controller has TPM wait state detection feature which is
enabled for TPM client devices reported in SPI device mode bits.
Set half duplex flag for TPM device to detect and send entire message
to controller in one shot.

Signed-off-by: Krishna Yarlagadda <kyarlagadda@nvidia.com>
---
 drivers/spi/spi-tegra210-quad.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/spi/spi-tegra210-quad.c b/drivers/spi/spi-tegra210-quad.c
index b967576b6c96..fe15fa6eecd1 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 tpm_wait_poll;
 	unsigned int cs_count;
 };
 
@@ -991,6 +993,14 @@ static void tegra_qspi_dump_regs(struct tegra_qspi *tqspi)
 	dev_dbg(tqspi->dev, "TRANS_STAT:  0x%08x | FIFO_STATUS: 0x%08x\n",
 		tegra_qspi_readl(tqspi, QSPI_TRANS_STATUS),
 		tegra_qspi_readl(tqspi, QSPI_FIFO_STATUS));
+	dev_dbg(tqspi->dev, "GLOBAL_CFG: 0x%08x\n",
+		tegra_qspi_readl(tqspi, QSPI_GLOBAL_CONFIG));
+	dev_dbg(tqspi->dev, "CMB_CMD: 0x%08x | CMB_CMD_CFG: 0x%08x\n",
+		tegra_qspi_readl(tqspi, QSPI_CMB_SEQ_CMD),
+		tegra_qspi_readl(tqspi, QSPI_CMB_SEQ_CMD_CFG));
+	dev_dbg(tqspi->dev, "CMB_ADDR: 0x%08x | CMB_ADDR_CFG: 0x%08x\n",
+		tegra_qspi_readl(tqspi, QSPI_CMB_SEQ_ADDR),
+		tegra_qspi_readl(tqspi, QSPI_CMB_SEQ_ADDR_CFG));
 }
 
 static void tegra_qspi_handle_error(struct tegra_qspi *tqspi)
@@ -1065,6 +1075,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->tpm_wait_poll)
+			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 */
@@ -1192,6 +1208,7 @@ 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;
+	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;
@@ -1450,24 +1467,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,
+	.tpm_wait_poll = false,
 	.cs_count = 1,
 };
 
 static struct tegra_qspi_soc_data tegra186_qspi_soc_data = {
 	.has_dma = true,
 	.cmb_xfer_capable = true,
+	.tpm_wait_poll = false,
 	.cs_count = 1,
 };
 
 static struct tegra_qspi_soc_data tegra234_qspi_soc_data = {
 	.has_dma = false,
 	.cmb_xfer_capable = true,
+	.tpm_wait_poll = true,
 	.cs_count = 1,
 };
 
 static struct tegra_qspi_soc_data tegra241_qspi_soc_data = {
 	.has_dma = false,
 	.cmb_xfer_capable = true,
+	.tpm_wait_poll = true,
 	.cs_count = 4,
 };
 
-- 
2.17.1

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1449 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Patch V3 3/3] spi: tegra210-quad: Enable TPM wait polling

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1203 bytes --]

On Thu, Feb 23, 2023 at 09:56:35PM +0530, Krishna Yarlagadda wrote:

> 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.
> Tegra241 QSPI controller has TPM wait state detection feature which is
> enabled for TPM client devices reported in SPI device mode bits.
> Set half duplex flag for TPM device to detect and send entire message
> to controller in one shot.

I don't really understand what the controller is actually doing here, or
what the intended effect of the SPI_TPM_HW_FLOW flag is supposed to be.

>  	/* Enable Combined sequence mode */
>  	val = tegra_qspi_readl(tqspi, QSPI_GLOBAL_CONFIG);
> +	if (spi->mode & SPI_TPM_HW_FLOW) {
> +		if (tqspi->soc_data->tpm_wait_poll)
> +			val |= QSPI_TPM_WAIT_POLL_EN;
> +		else
> +			return -EIO;
> +	}

This just sets a bit in a register...

>  	val |= QSPI_CMB_SEQ_EN;
>  	tegra_qspi_writel(tqspi, val, QSPI_GLOBAL_CONFIG);
>  	/* Process individual transfer list */

...my guess is that setting that bit causes the individual transfers to
be delayed in completing without further changes?  Is is just some
transfers or all of them?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: (subset) [Patch V3 0/3] Tegra TPM driver with HW flow control

[Mark Brown]

On Thu, 23 Feb 2023 21:56:32 +0530, Krishna Yarlagadda wrote:
> TPM interface spec defines flow control where TPM device would drive
> MISO at same cycle as last address bit sent by controller on MOSI. This
> state of wait can be detected by software reading the MISO line or
> by controller hardware. Support sending transfers to controller in
> single message and handle flow control in hardware. Half duplex
> controllers have to support flow control in hardware.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[2/3] spi: tegra210-quad: set half duplex flag
      commit: f7482d8285b638be87a594a30edaaf1341135c1a

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

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

[Krishna Yarlagadda]

> -----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.

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 836 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [Patch V3 3/3] spi: tegra210-quad: Enable TPM wait polling

[Krishna Yarlagadda]

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: 23 February 2023 22:59
> 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 3/3] spi: tegra210-quad: Enable TPM wait polling
> 
> On Thu, Feb 23, 2023 at 09:56:35PM +0530, Krishna Yarlagadda wrote:
> 
> > 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.
> > Tegra241 QSPI controller has TPM wait state detection feature which is
> > enabled for TPM client devices reported in SPI device mode bits.
> > Set half duplex flag for TPM device to detect and send entire message
> > to controller in one shot.
> 
> I don't really understand what the controller is actually doing here, or
> what the intended effect of the SPI_TPM_HW_FLOW flag is supposed to be.
> 
> >  	/* Enable Combined sequence mode */
> >  	val = tegra_qspi_readl(tqspi, QSPI_GLOBAL_CONFIG);
> > +	if (spi->mode & SPI_TPM_HW_FLOW) {
> > +		if (tqspi->soc_data->tpm_wait_poll)
> > +			val |= QSPI_TPM_WAIT_POLL_EN;
> > +		else
> > +			return -EIO;
> > +	}
> 
> This just sets a bit in a register...
> 
> >  	val |= QSPI_CMB_SEQ_EN;
> >  	tegra_qspi_writel(tqspi, val, QSPI_GLOBAL_CONFIG);
> >  	/* Process individual transfer list */
> 
> ...my guess is that setting that bit causes the individual transfers to
> be delayed in completing without further changes?  Is is just some
> transfers or all of them?
TPM spec define flow control over SPI.  TPM device/client inserts
wait state on MISO line when address is transferred on MOSI. This state
has to be detected by reading the MISO line which needs full duplex
transfer during address phase. Tegra QSPI controller can only support
half duplex. But in combined sequence mode, it can detect wait state
inserted by TPM device and send or receive data when device is ready.
Detection happens on all transfers with TPM.

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

[Krishna Yarlagadda]

> -----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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 530 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Krishna Yarlagadda]

> -----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.