Re: [Patch V9 2/3] tpm_tis-spi: Add hardware wait polling

Re: [Patch V9 2/3] tpm_tis-spi: Add hardware wait polling

[kernel test robot]

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20230325183409.7695-3-kyarlagadda@nvidia.com>
References: <20230325183409.7695-3-kyarlagadda@nvidia.com>
TO: Krishna Yarlagadda <kyarlagadda@nvidia.com>
TO: robh+dt@kernel.org
TO: broonie@kernel.org
TO: peterhuewe@gmx.de
TO: jgg@ziepe.ca
TO: jarkko@kernel.org
TO: krzysztof.kozlowski+dt@linaro.org
TO: linux-spi@vger.kernel.org
TO: linux-tegra@vger.kernel.org
TO: linux-integrity@vger.kernel.org
TO: linux-kernel@vger.kernel.org
CC: thierry.reding@gmail.com
CC: jonathanh@nvidia.com
CC: skomatineni@nvidia.com
CC: ldewangan@nvidia.com
CC: Krishna Yarlagadda <kyarlagadda@nvidia.com>

Hi Krishna,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on broonie-spi/for-next]
[also build test WARNING on char-misc/char-misc-testing char-misc/char-misc-next char-misc/char-misc-linus linus/master v6.3-rc4 next-20230330]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Krishna-Yarlagadda/spi-Add-TPM-HW-flow-flag/20230326-023635
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
patch link:    https://lore.kernel.org/r/20230325183409.7695-3-kyarlagadda%40nvidia.com
patch subject: [Patch V9 2/3] tpm_tis-spi: Add hardware wait polling
:::::: branch date: 5 days ago
:::::: commit date: 5 days ago
config: m68k-randconfig-m041-20230329 (https://download.01.org/0day-ci/archive/20230331/202303310004.Ul47LRPD-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.0

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Link: https://lore.kernel.org/r/202303310004.Ul47LRPD-lkp@intel.com/

smatch warnings:
drivers/char/tpm/tpm_tis_spi_main.c:137 tpm_tis_spi_transfer_half() error: uninitialized symbol 'ret'.

vim +/ret +137 drivers/char/tpm/tpm_tis_spi_main.c

8ab5e82afa969b6 drivers/char/tpm/tpm_tis_spi.c      Stephen Boyd       2019-09-20   73  
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26   74  /*
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26   75   * Half duplex controller with support for TPM wait state detection like
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26   76   * Tegra QSPI need CMD, ADDR & DATA sent in single message to manage HW flow
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26   77   * control. Each phase sent in different transfer for controller to idenity
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26   78   * phase.
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26   79   */
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26   80  static int tpm_tis_spi_transfer_half(struct tpm_tis_data *data,	u32 addr,
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26   81  				     u16 len, u8 *in, const u8 *out)
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26   82  {
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26   83  	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26   84  	struct spi_transfer spi_xfer[3];
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26   85  	struct spi_message m;
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26   86  	u8 transfer_len;
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26   87  	int ret;
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26   88  
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26   89  	while (len) {
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26   90  		transfer_len = min_t(u16, len, MAX_SPI_FRAMESIZE);
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26   91  
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26   92  		spi_message_init(&m);
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26   93  		phy->iobuf[0] = (in ? 0x80 : 0) | (transfer_len - 1);
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26   94  		phy->iobuf[1] = 0xd4;
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26   95  		phy->iobuf[2] = addr >> 8;
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26   96  		phy->iobuf[3] = addr;
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26   97  
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26   98  		memset(&spi_xfer, 0, sizeof(spi_xfer));
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26   99  
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  100  		spi_xfer[0].tx_buf = phy->iobuf;
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  101  		spi_xfer[0].len = 1;
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  102  		spi_message_add_tail(&spi_xfer[0], &m);
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  103  
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  104  		spi_xfer[1].tx_buf = phy->iobuf + 1;
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  105  		spi_xfer[1].len = 3;
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  106  		spi_message_add_tail(&spi_xfer[1], &m);
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  107  
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  108  		if (out) {
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  109  			spi_xfer[2].tx_buf = &phy->iobuf[4];
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  110  			spi_xfer[2].rx_buf = NULL;
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  111  			memcpy(&phy->iobuf[4], out, transfer_len);
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  112  			out += transfer_len;
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  113  		}
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  114  
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  115  		if (in) {
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  116  			spi_xfer[2].tx_buf = NULL;
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  117  			spi_xfer[2].rx_buf = &phy->iobuf[4];
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  118  		}
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  119  
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  120  		spi_xfer[2].len = transfer_len;
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  121  		spi_message_add_tail(&spi_xfer[2], &m);
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  122  
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  123  		reinit_completion(&phy->ready);
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  124  
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  125  		ret = spi_sync_locked(phy->spi_device, &m);
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  126  		if (ret < 0)
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  127  			return ret;
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  128  
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  129  		if (in) {
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  130  			memcpy(in, &phy->iobuf[4], transfer_len);
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  131  			in += transfer_len;
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  132  		}
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  133  
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  134  		len -= transfer_len;
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  135  	}
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  136  
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26 @137  	return ret;
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  138  }
9e4db447b009ec2 drivers/char/tpm/tpm_tis_spi_main.c Krishna Yarlagadda 2023-03-26  139  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

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

[Krishna Yarlagadda]

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.

V9
 - renamed tpm spi transfer functions
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 | 91 ++++++++++++++++++++++++++++-
 drivers/spi/spi-tegra210-quad.c     | 14 +++++
 include/linux/spi/spi.h             | 16 ++++-
 3 files changed, 116 insertions(+), 5 deletions(-)

-- 
2.17.1

[Patch V9 2/3] tpm_tis-spi: Add hardware wait polling

[Krishna Yarlagadda]

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 <kyarlagadda@nvidia.com>
---
 drivers/char/tpm/tpm_tis_spi_main.c | 91 ++++++++++++++++++++++++++++-
 1 file changed, 89 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..db9afd0b83da 100644
--- a/drivers/char/tpm/tpm_tis_spi_main.c
+++ b/drivers/char/tpm/tpm_tis_spi_main.c
@@ -71,8 +71,74 @@ 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_transfer_half(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_transfer_full(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 +206,24 @@ 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_transfer_half(data, addr, len, in, out);
+	else
+		return tpm_tis_spi_transfer_full(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 +265,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

Re: [Patch V9 2/3] tpm_tis-spi: Add hardware wait polling

[Jarkko Sakkinen]

On Sun, Mar 26, 2023 at 12:04:08AM +0530, Krishna Yarlagadda wrote:
> 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 <kyarlagadda@nvidia.com>
> ---
>  drivers/char/tpm/tpm_tis_spi_main.c | 91 ++++++++++++++++++++++++++++-
>  1 file changed, 89 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..db9afd0b83da 100644
> --- a/drivers/char/tpm/tpm_tis_spi_main.c
> +++ b/drivers/char/tpm/tpm_tis_spi_main.c
> @@ -71,8 +71,74 @@ 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_transfer_half(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_transfer_full(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 +206,24 @@ 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_transfer_half(data, addr, len, in, out);
> +	else
> +		return tpm_tis_spi_transfer_full(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 +265,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
> 


Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko

Re: [Patch V9 2/3] tpm_tis-spi: Add hardware wait polling

[Jerry Snitselaar]

On Sun, Mar 26, 2023 at 12:04:08AM +0530, Krishna Yarlagadda wrote:
> 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 <kyarlagadda@nvidia.com>
> ---
>  drivers/char/tpm/tpm_tis_spi_main.c | 91 ++++++++++++++++++++++++++++-
>  1 file changed, 89 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..db9afd0b83da 100644
> --- a/drivers/char/tpm/tpm_tis_spi_main.c
> +++ b/drivers/char/tpm/tpm_tis_spi_main.c
> @@ -71,8 +71,74 @@ 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_transfer_half(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;

I haven't looked at much TPM code in the past couple of years, but
perhaps some defines instead of magic numbers here? 0x80 is the rw bit,
and 0xd4 the transaction offset?

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

Does tpm_tis_spi_transfer_half not need to lock the bus?  The doc comments for spi_sync_locked
state:

 This call should be used by drivers that require exclusive access to the
 SPI bus. It has to be preceded by a spi_bus_lock call. The SPI bus must
 be released by a spi_bus_unlock call when the exclusive access is over.

If that isn't the case should it be using spi_sync instead of spi_sync_locked?

Regards,
Jerry

> +
> +static int tpm_tis_spi_transfer_full(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 +206,24 @@ 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_transfer_half(data, addr, len, in, out);
> +	else
> +		return tpm_tis_spi_transfer_full(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 +265,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
> 

Re: [Patch V9 2/3] tpm_tis-spi: Add hardware wait polling

[Jerry Snitselaar]

On Wed, Apr 19, 2023 at 07:32:40PM -0700, Jerry Snitselaar wrote:
> On Sun, Mar 26, 2023 at 12:04:08AM +0530, Krishna Yarlagadda wrote:
> > 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 <kyarlagadda@nvidia.com>
> > ---
> >  drivers/char/tpm/tpm_tis_spi_main.c | 91 ++++++++++++++++++++++++++++-
> >  1 file changed, 89 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..db9afd0b83da 100644
> > --- a/drivers/char/tpm/tpm_tis_spi_main.c
> > +++ b/drivers/char/tpm/tpm_tis_spi_main.c
> > @@ -71,8 +71,74 @@ 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_transfer_half(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;
> 
> I haven't looked at much TPM code in the past couple of years, but
> perhaps some defines instead of magic numbers here? 0x80 is the rw bit,
> and 0xd4 the transaction offset?
> 
> > +
> > +		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;
> > +}
> 
> Does tpm_tis_spi_transfer_half not need to lock the bus?  The doc comments for spi_sync_locked
> state:
> 
>  This call should be used by drivers that require exclusive access to the
>  SPI bus. It has to be preceded by a spi_bus_lock call. The SPI bus must
>  be released by a spi_bus_unlock call when the exclusive access is over.
> 
> If that isn't the case should it be using spi_sync instead of spi_sync_locked?
> 
> Regards,
> Jerry

b4 mbox -c to the rescue. I found the earlier discussion with Mark about
the lock, so I guess the question is just should this call spi_sync
instead of spi_sync_locked then?

The magic numbers is a minor nit, and can probably be cleaned up
separately since the full duplex code was already doing the same
thing. The only other nit is just the older tcg spec being referenced
in patch 1.

Regards,
Jerry

> 
> > +
> > +static int tpm_tis_spi_transfer_full(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 +206,24 @@ 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_transfer_half(data, addr, len, in, out);
> > +	else
> > +		return tpm_tis_spi_transfer_full(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 +265,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
> > 
> 

RE: [Patch V9 2/3] tpm_tis-spi: Add hardware wait polling

[Krishna Yarlagadda]

> -----Original Message-----
> From: Jerry Snitselaar <jsnitsel@redhat.com>
> Sent: 20 April 2023 08:42
> To: Krishna Yarlagadda <kyarlagadda@nvidia.com>
> Cc: robh+dt@kernel.org; broonie@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 V9 2/3] tpm_tis-spi: Add hardware wait polling
> 
> External email: Use caution opening links or attachments
> 
> 
> On Wed, Apr 19, 2023 at 07:32:40PM -0700, Jerry Snitselaar wrote:
> > On Sun, Mar 26, 2023 at 12:04:08AM +0530, Krishna Yarlagadda wrote:
> > > 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 <kyarlagadda@nvidia.com>
> > > ---
> > >  drivers/char/tpm/tpm_tis_spi_main.c | 91
> ++++++++++++++++++++++++++++-
> > >  1 file changed, 89 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..db9afd0b83da 100644
> > > --- a/drivers/char/tpm/tpm_tis_spi_main.c
> > > +++ b/drivers/char/tpm/tpm_tis_spi_main.c
> > > @@ -71,8 +71,74 @@ 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_transfer_half(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;
> >
> > I haven't looked at much TPM code in the past couple of years, but
> > perhaps some defines instead of magic numbers here? 0x80 is the rw bit,
> > and 0xd4 the transaction offset?
> >
> > > +
> > > +           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;
> > > +}
> >
> > Does tpm_tis_spi_transfer_half not need to lock the bus?  The doc
> comments for spi_sync_locked
> > state:
> >
> >  This call should be used by drivers that require exclusive access to the
> >  SPI bus. It has to be preceded by a spi_bus_lock call. The SPI bus must
> >  be released by a spi_bus_unlock call when the exclusive access is over.
> >
> > If that isn't the case should it be using spi_sync instead of spi_sync_locked?
> >
> > Regards,
> > Jerry
> 
> b4 mbox -c to the rescue. I found the earlier discussion with Mark about
> the lock, so I guess the question is just should this call spi_sync
> instead of spi_sync_locked then?
> 
> The magic numbers is a minor nit, and can probably be cleaned up
> separately since the full duplex code was already doing the same
> thing. The only other nit is just the older tcg spec being referenced
> in patch 1.
> 
> Regards,
> Jerry
Magic number can be dealt in a different patch for both half and full
Transfer calls.
As we send single message for complete transaction, bus need not be
locked. I will replace the calls with spi_sync.
Will update referenced tcg spec as well to the latest.

Regards
KY
> 
> >
> > > +
> > > +static int tpm_tis_spi_transfer_full(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 +206,24 @@ 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_transfer_half(data, addr, len, in, out);
> > > +   else
> > > +           return tpm_tis_spi_transfer_full(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 +265,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
> > >
> >