| Message ID | 20230227120702.13180-3-kyarlagadda@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Tegra TPM driver with HW flow control | expand |
On Mon, Feb 27, 2023 at 05:37:01PM +0530, Krishna Yarlagadda wrote: > 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. When a is started sentence with the word "support" it translates to "I'm too lazy to write a proper and verbose description of the implementation" :-) It has some abstract ideas of the implementation, I give you that, but do you think anyone ever will get any value of reading that honestly? A bit more concrette description of the change helps e.g. when bisecting bugs. > 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 | 92 ++++++++++++++++++++++++++++- > 1 file changed, 90 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..5f66448ee09e 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 > + * Tegra241 need cmd, addr & data sent in single message to manage HW flow > + * control. Each phase sent in different transfer for controller to idenity > + * phase. > + */ > +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; > + > + 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; > +} > + > +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 +206,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) > { > -- > 2.17.1 > Looking pretty good but do you really want to export tpm_tis_spi_{hw,sw}_flow_transfer? BR, Jarkko
> -----Original Message----- > From: Jarkko Sakkinen <jarkko@kernel.org> > Sent: 28 February 2023 08:06 > To: Krishna Yarlagadda <kyarlagadda@nvidia.com> > Cc: robh+dt@kernel.org; broonie@kernel.org; peterhuewe@gmx.de; > jgg@ziepe.ca; 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 V5 2/3] tpm_tis-spi: Support hardware wait polling > > External email: Use caution opening links or attachments > > > On Mon, Feb 27, 2023 at 05:37:01PM +0530, Krishna Yarlagadda wrote: > > 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. > > When a is started sentence with the word "support" it translates to "I'm > too lazy to write a proper and verbose description of the implementation" > :-) > > It has some abstract ideas of the implementation, I give you that, but do > you think anyone ever will get any value of reading that honestly? A bit > more concrette description of the change helps e.g. when bisecting bugs. > I presented why we are making the change. Will add explanation on how it is implemented as well. > > 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 | 92 > ++++++++++++++++++++++++++++- > > 1 file changed, 90 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..5f66448ee09e 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 > > + * Tegra241 need cmd, addr & data sent in single message to manage HW > flow > > + * control. Each phase sent in different transfer for controller to idenity > > + * phase. > > + */ > > +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; > > + > > + 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; > > +} > > + > > +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 +206,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) > > { > > -- > > 2.17.1 > > > > Looking pretty good but do you really want to export > tpm_tis_spi_{hw,sw}_flow_transfer? > > BR, Jarkko No need to export tpm_tis_spi_{hw,sw}_flow_transfer as well. I will update this in next version. KY
On Tue, Feb 28, 2023 at 04:36:26AM +0200, Jarkko Sakkinen wrote: > On Mon, Feb 27, 2023 at 05:37:01PM +0530, Krishna Yarlagadda wrote: > > 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. > > When a is started sentence with the word "support" it translates to "I'm > too lazy to write a proper and verbose description of the implementation" > :-) > > It has some abstract ideas of the implementation, I give you that, but do > you think anyone ever will get any value of reading that honestly? A bit > more concrette description of the change helps e.g. when bisecting bugs. I would expect SPI_TPM_HW_FLOW to be documented in the kdocs to a level that any other HW could implement it as well. > > +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; Shouldn't we check that this special flow is supported when the SPI device is bound to the tpm in the first place? Jason
> -----Original Message----- > From: Jason Gunthorpe <jgg@nvidia.com> > Sent: 28 February 2023 17:58 > To: Jarkko Sakkinen <jarkko@kernel.org> > Cc: Krishna Yarlagadda <kyarlagadda@nvidia.com>; robh+dt@kernel.org; > broonie@kernel.org; peterhuewe@gmx.de; > 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 V5 2/3] tpm_tis-spi: Support hardware wait polling > > On Tue, Feb 28, 2023 at 04:36:26AM +0200, Jarkko Sakkinen wrote: > > On Mon, Feb 27, 2023 at 05:37:01PM +0530, Krishna Yarlagadda wrote: > > > 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. > > > > When a is started sentence with the word "support" it translates to "I'm > > too lazy to write a proper and verbose description of the implementation" > > :-) > > > > It has some abstract ideas of the implementation, I give you that, but do > > you think anyone ever will get any value of reading that honestly? A bit > > more concrette description of the change helps e.g. when bisecting bugs. > > I would expect SPI_TPM_HW_FLOW to be documented in the kdocs to a > level that any other HW could implement it as well. HW implementation can be controller specific. I would add comments in the header to say CMD-ADDR-DATA is sent as single message with this flag. > > > > +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; > > Shouldn't we check that this special flow is supported when the SPI > device is bound to the tpm in the first place? TPM device connected behind half duplex controller can only work this way. So, no additional flag needed to check. KY > > Jason
On Wed, Mar 01, 2023 at 11:56:53AM +0000, Krishna Yarlagadda wrote: > > > > +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; > > > > Shouldn't we check that this special flow is supported when the SPI > > device is bound to the tpm in the first place? > TPM device connected behind half duplex controller can only work > this way. So, no additional flag needed to check. Just because a DT hooks it up this way doesn't mean the kernel driver can support it, eg support hasn't been implemented in an older SPI driver or something. If the failure mode is anything other than the TPM doesn't probe we will need to check for support. Jason
On Wed, Mar 01, 2023 at 08:27:45AM -0400, Jason Gunthorpe wrote: > On Wed, Mar 01, 2023 at 11:56:53AM +0000, Krishna Yarlagadda wrote: > > TPM device connected behind half duplex controller can only work > > this way. So, no additional flag needed to check. > Just because a DT hooks it up this way doesn't mean the kernel driver > can support it, eg support hasn't been implemented in an older SPI > driver or something. > If the failure mode is anything other than the TPM doesn't probe we > will need to check for support. It's not like these buses are hot pluggable - someone would have to design and manufacture a board which doesn't work. It's probably reasonable for this to fail with the SPI subsystem saying it can't support things when the operation is tried.
On Wed, Mar 01, 2023 at 12:37:27PM +0000, Mark Brown wrote: > On Wed, Mar 01, 2023 at 08:27:45AM -0400, Jason Gunthorpe wrote: > > On Wed, Mar 01, 2023 at 11:56:53AM +0000, Krishna Yarlagadda wrote: > > > > TPM device connected behind half duplex controller can only work > > > this way. So, no additional flag needed to check. > > > Just because a DT hooks it up this way doesn't mean the kernel driver > > can support it, eg support hasn't been implemented in an older SPI > > driver or something. > > > If the failure mode is anything other than the TPM doesn't probe we > > will need to check for support. > > It's not like these buses are hot pluggable - someone would have to > design and manufacture a board which doesn't work. It's probably > reasonable for this to fail with the SPI subsystem saying it can't > support things when the operation is tried. If the spi subsystem fails this request with these flags that would be great, it would cause the TPM to fail probing reliably. But does this patch do that? It looks like non-supporting half duplex drivers will just ignore the new flag? Jason
On Wed, Mar 01, 2023 at 09:39:28AM -0400, Jason Gunthorpe wrote: > On Wed, Mar 01, 2023 at 12:37:27PM +0000, Mark Brown wrote: > > It's not like these buses are hot pluggable - someone would have to > > design and manufacture a board which doesn't work. It's probably > > reasonable for this to fail with the SPI subsystem saying it can't > > support things when the operation is tried. > If the spi subsystem fails this request with these flags that would be > great, it would cause the TPM to fail probing reliably. > But does this patch do that? It looks like non-supporting half duplex > drivers will just ignore the new flag? That's something we can fix up in SPI, we shouldn't worry about it for the client drivers.
On Wed, Mar 01, 2023 at 09:39:28AM -0400, Jason Gunthorpe wrote: > On Wed, Mar 01, 2023 at 12:37:27PM +0000, Mark Brown wrote: > > On Wed, Mar 01, 2023 at 08:27:45AM -0400, Jason Gunthorpe wrote: > > > On Wed, Mar 01, 2023 at 11:56:53AM +0000, Krishna Yarlagadda wrote: > > > > > > TPM device connected behind half duplex controller can only work > > > > this way. So, no additional flag needed to check. > > > > > Just because a DT hooks it up this way doesn't mean the kernel driver > > > can support it, eg support hasn't been implemented in an older SPI > > > driver or something. > > > > > If the failure mode is anything other than the TPM doesn't probe we > > > will need to check for support. > > > > It's not like these buses are hot pluggable - someone would have to > > design and manufacture a board which doesn't work. It's probably > > reasonable for this to fail with the SPI subsystem saying it can't > > support things when the operation is tried. > > If the spi subsystem fails this request with these flags that would be > great, it would cause the TPM to fail probing reliably. > > But does this patch do that? It looks like non-supporting half duplex > drivers will just ignore the new flag? I think the assumption is that there are currently no half duplex drivers that would be impacted by this. If I understand correctly, the TPM driver currently supports only full duplex controllers, because that's required in order to detect the wait state in software. So, yes, half duplex controllers would ignore this flag, but since they couldn't have supported TPM flow control before anyway it doesn't make a difference. Thierry
On Wed, Mar 01, 2023 at 03:09:24PM +0100, Thierry Reding wrote: > On Wed, Mar 01, 2023 at 09:39:28AM -0400, Jason Gunthorpe wrote: > > On Wed, Mar 01, 2023 at 12:37:27PM +0000, Mark Brown wrote: > > > On Wed, Mar 01, 2023 at 08:27:45AM -0400, Jason Gunthorpe wrote: > > > > On Wed, Mar 01, 2023 at 11:56:53AM +0000, Krishna Yarlagadda wrote: > > > > > > > > TPM device connected behind half duplex controller can only work > > > > > this way. So, no additional flag needed to check. > > > > > > > Just because a DT hooks it up this way doesn't mean the kernel driver > > > > can support it, eg support hasn't been implemented in an older SPI > > > > driver or something. > > > > > > > If the failure mode is anything other than the TPM doesn't probe we > > > > will need to check for support. > > > > > > It's not like these buses are hot pluggable - someone would have to > > > design and manufacture a board which doesn't work. It's probably > > > reasonable for this to fail with the SPI subsystem saying it can't > > > support things when the operation is tried. > > > > If the spi subsystem fails this request with these flags that would be > > great, it would cause the TPM to fail probing reliably. > > > > But does this patch do that? It looks like non-supporting half duplex > > drivers will just ignore the new flag? > > I think the assumption is that there are currently no half duplex > drivers that would be impacted by this. If I understand correctly, the > TPM driver currently supports only full duplex controllers, because > that's required in order to detect the wait state in software. > > So, yes, half duplex controllers would ignore this flag, but since they > couldn't have supported TPM flow control before anyway it doesn't make a > difference. If more HW uses this feature it will likely look a lot like these tegra drivers where an existing supported SPI driver gains a HW bit to do the flow. Meaning DTs will exist configuring a TPM to a half duplex SPI and kernels will exist that don't have the HW driver that implements it. So, I would like it if old kernels running against a new DT do not mis-operate the SPI because their SPI driver does not support TPM operation. Either because the spi layer refuses the request as unsupported or the TPM layer refuses to use the spi driver as unsupported. I do not like the idea that the SPI subsystem will take a request from a client driver and silently mis-execute it. Jason
On Tue, Feb 28, 2023 at 03:32:24AM +0000, Krishna Yarlagadda wrote: > > -----Original Message----- > > From: Jarkko Sakkinen <jarkko@kernel.org> > > Sent: 28 February 2023 08:06 > > To: Krishna Yarlagadda <kyarlagadda@nvidia.com> > > Cc: robh+dt@kernel.org; broonie@kernel.org; peterhuewe@gmx.de; > > jgg@ziepe.ca; 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 V5 2/3] tpm_tis-spi: Support hardware wait polling > > > > External email: Use caution opening links or attachments > > > > > > On Mon, Feb 27, 2023 at 05:37:01PM +0530, Krishna Yarlagadda wrote: > > > 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. > > > > When a is started sentence with the word "support" it translates to "I'm > > too lazy to write a proper and verbose description of the implementation" > > :-) > > > > It has some abstract ideas of the implementation, I give you that, but do > > you think anyone ever will get any value of reading that honestly? A bit > > more concrette description of the change helps e.g. when bisecting bugs. > > > I presented why we are making the change. Will add explanation on how > it is implemented as well. OK, cool, thank you. > > > > 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 | 92 > > ++++++++++++++++++++++++++++- > > > 1 file changed, 90 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..5f66448ee09e 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 > > > + * Tegra241 need cmd, addr & data sent in single message to manage HW > > flow > > > + * control. Each phase sent in different transfer for controller to idenity > > > + * phase. > > > + */ > > > +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; > > > + > > > + 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; > > > +} > > > + > > > +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 +206,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) > > > { > > > -- > > > 2.17.1 > > > > > > > Looking pretty good but do you really want to export > > tpm_tis_spi_{hw,sw}_flow_transfer? > > > > BR, Jarkko > No need to export tpm_tis_spi_{hw,sw}_flow_transfer as well. > I will update this in next version. Great. BR, Jarkko
diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c index a0963a3e92bd..5f66448ee09e 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 + * Tegra241 need cmd, addr & data sent in single message to manage HW flow + * control. Each phase sent in different transfer for controller to idenity + * phase. + */ +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; + + 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; +} + +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 +206,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) {
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 | 92 ++++++++++++++++++++++++++++- 1 file changed, 90 insertions(+), 2 deletions(-)