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

[Thierry Reding]

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

Attachment: signature.asc
Description: PGP signature