diff mbox series

[V6,1/3] spi: Add TPM HW flow flag

Message ID 20230227172108.8206-2-kyarlagadda@nvidia.com
State New
Headers
Series Tegra TPM driver with HW flow control |

Commit Message

Krishna Yarlagadda Feb. 27, 2023, 5:21 p.m. UTC 
  TPM spec defines flow control over SPI. Client device can insert a wait
state on MISO when address is trasmitted by controller on MOSI. It can
work only on full duplex.
Half duplex controllers need to implement flow control in HW.
Add a flag for TPM to indicate flow control is expected in controller.

Signed-off-by: Krishna Yarlagadda <kyarlagadda@nvidia.com>
---
 include/linux/spi/spi.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Thierry Reding March 1, 2023, 1:34 p.m. UTC | #1 
On Mon, Feb 27, 2023 at 10:51:06PM +0530, Krishna Yarlagadda wrote:
> TPM spec defines flow control over SPI. Client device can insert a wait

Maybe add a reference to where in the TPM specification this can be
found? It looks like the specifications are publicly available, though
I'm less sure about stability of the links, so perhaps it's enough to
name the document and section that this can be found in. QEMU seems to
be using this link to point to the specification, which I suppose has a
good chance of remaining stable:

	https://trustedcomputinggroup.org/resource/pc-client-work-group-pc-client-specific-tpm-interface-specification-tis/

It looks like the latest version is 1.3 revision 27 and the details of
this flow control mechanism are in section "6.4.5. Flow Control".

> state on MISO when address is trasmitted by controller on MOSI. It can

"transmitted"

> work only on full duplex.
> Half duplex controllers need to implement flow control in HW.

This is a bit confusing because you first say it will only work for full
duplex controllers and then you say it's also possible for half-duplex
controllers.

Maybe reword this to something like:

	Detecting the wait state in software is only possible for full
	duplex controllers. For controllers that support only half-
	duplex, the wait state detection needs to be implemented in
	hardware.

> Add a flag for TPM to indicate flow control is expected in controller.

That's not exactly what the flag indicates, though, is it? It primarily
indicates that the device uses TPM flow control. It's then up to the
controller to configure itself accordingly (i.e. if it supports half-
duplex, enable detection of the wait state, otherwise leave it up to the
client driver to detect the wait state).

> 
> Signed-off-by: Krishna Yarlagadda <kyarlagadda@nvidia.com>
> ---
>  include/linux/spi/spi.h | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 4fa26b9a3572..6b32c90e9e20 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 */

Maybe some (or all?) of the information in the commit message should be
duplicated here? That way people wouldn't need to go look for the commit
message in order to find out.

Given what I said above about the flag, it may be better to name this
SPI_TPM_FLOW_CONTROL, but I suppose what you have here is fine, too.

Thierry
Krishna Yarlagadda March 1, 2023, 3:27 p.m. UTC | #2 
> -----Original Message-----
> From: Thierry Reding <thierry.reding@gmail.com>
> Sent: 01 March 2023 19:04
> 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; Jonathan Hunter
> <jonathanh@nvidia.com>; Sowjanya Komatineni
> <skomatineni@nvidia.com>; Laxman Dewangan <ldewangan@nvidia.com>
> Subject: Re: [Patch V6 1/3] spi: Add TPM HW flow flag
> 
> On Mon, Feb 27, 2023 at 10:51:06PM +0530, Krishna Yarlagadda wrote:
> > TPM spec defines flow control over SPI. Client device can insert a wait
> 
> Maybe add a reference to where in the TPM specification this can be
> found? It looks like the specifications are publicly available, though
> I'm less sure about stability of the links, so perhaps it's enough to
> name the document and section that this can be found in. QEMU seems to
> be using this link to point to the specification, which I suppose has a
> good chance of remaining stable:
> 
> 	https://trustedcomputinggroup.org/resource/pc-client-work-group-
> pc-client-specific-tpm-interface-specification-tis/
> 
> It looks like the latest version is 1.3 revision 27 and the details of
> this flow control mechanism are in section "6.4.5. Flow Control".
> 
> > state on MISO when address is trasmitted by controller on MOSI. It can
> 
> "transmitted"
> 
> > work only on full duplex.
> > Half duplex controllers need to implement flow control in HW.
> 
> This is a bit confusing because you first say it will only work for full
> duplex controllers and then you say it's also possible for half-duplex
> controllers.
> 
> Maybe reword this to something like:
> 
> 	Detecting the wait state in software is only possible for full
> 	duplex controllers. For controllers that support only half-
> 	duplex, the wait state detection needs to be implemented in
> 	hardware.
> 
> > Add a flag for TPM to indicate flow control is expected in controller.
> 
> That's not exactly what the flag indicates, though, is it? It primarily
> indicates that the device uses TPM flow control. It's then up to the
> controller to configure itself accordingly (i.e. if it supports half-
> duplex, enable detection of the wait state, otherwise leave it up to the
> client driver to detect the wait state).
Flag is enabled only if controller is half-duplex and HW flow control is
needed. Will change wording to say it is enabled for HW flow control.
> 
> >
> > Signed-off-by: Krishna Yarlagadda <kyarlagadda@nvidia.com>
> > ---
> >  include/linux/spi/spi.h | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> > index 4fa26b9a3572..6b32c90e9e20 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 */
> 
> Maybe some (or all?) of the information in the commit message should be
> duplicated here? That way people wouldn't need to go look for the commit
> message in order to find out.
> 
> Given what I said above about the flag, it may be better to name this
> SPI_TPM_FLOW_CONTROL, but I suppose what you have here is fine, too.
> 
> Thierry
diff mbox series

Patch

diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 4fa26b9a3572..6b32c90e9e20 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;