From:   Krishna Yarlagadda <kyarlagadda@nvidia.com>
To:     Jarkko Sakkinen <jarkko@kernel.org>
CC:     "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "broonie@kernel.org" <broonie@kernel.org>,
        "peterhuewe@gmx.de" <peterhuewe@gmx.de>,
        "jgg@ziepe.ca" <jgg@ziepe.ca>,
        "krzysztof.kozlowski+dt@linaro.org" 
        <krzysztof.kozlowski+dt@linaro.org>,
        "linux-spi@vger.kernel.org" <linux-spi@vger.kernel.org>,
        "linux-tegra@vger.kernel.org" <linux-tegra@vger.kernel.org>,
        "linux-integrity@vger.kernel.org" <linux-integrity@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "thierry.reding@gmail.com" <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
Thread-Topic: [Patch V5 2/3] tpm_tis-spi: Support hardware wait polling
Thread-Index: AQHZSqQUcW4yC19F/EyRW9qbZsaOO67jpaAAgAANsKA=
Date:   Tue, 28 Feb 2023 03:32:24 +0000
Message-ID: <DM4PR12MB5769BCBCD410C75DF3EB26FBC3AC9@DM4PR12MB5769.namprd12.prod.outlook.com>
References: <20230227120702.13180-1-kyarlagadda@nvidia.com>
 <20230227120702.13180-3-kyarlagadda@nvidia.com> <Y/1oqr0RfD7KVA4y@kernel.org>
In-Reply-To: <Y/1oqr0RfD7KVA4y@kernel.org>
Accept-Language: en-IN, en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?xWlbzMXKu8Su0p3CORQKfNApO/KdGyNK2IUmfZS1SNyVaoqLiNVME08ZHc5q?=
 =?us-ascii?Q?U8i1NqcxDi6cCOwbUtmj33Av7RKpq2+SyeDlXF2W7FGnd9HeouyG1RqjyRSx?=
 =?us-ascii?Q?/2xCC8NlAvzAmOyfMVLGwHOTQxBgWC4g/Y9pnCjvJO+tOy+7yW85C1ecYYrO?=
 =?us-ascii?Q?4UWClXDBX1IGu2nuaonDKS5pT+vnXmQdpGQGAyjOIhgVwMfNdI0klQOMxU5a?=
 =?us-ascii?Q?RoKagVH1v8enNZwBwLtUlfjRMP0cTS1WVPQHHDGzDQpwCftxB+mS4o5qQgp3?=
 =?us-ascii?Q?qttpM+8opo4vZ8oRPh3yg0Xg2vgtHnxoY+ttoCvsI7jZEx8gyrwgB0jANWJc?=
 =?us-ascii?Q?m60vAeyZID4EbtxqRfWycsNySM7l0A7biflyLG1lAtQpYG9MACUlNSEc7IgX?=
 =?us-ascii?Q?SFS4A08zCWKD8aVf5TCN+p4KM//sVJfVFp60ASEoe0IhyoWwdWzB+VnTaL73?=
 =?us-ascii?Q?2IgrU18AMaAWfe3PJBgLStb4Urwzaoaunq3c/0UPhrEaZRrzUQYpbU35thTv?=
 =?us-ascii?Q?8ETmL6YFklk3muMELv69vSl9DQO+GNieghtQRtKYRm08qEAfW+vyL0Lzk741?=
 =?us-ascii?Q?o2MJwBs2f0r8MeLBUUdvbQgAevKHRxlbZ66J3764t68XYu4/kwFjT4sDaHnB?=
 =?us-ascii?Q?pwTnwmOn4id70kwA3eFMfAcB7ZF325DtyAmnD7KjaBA+r4eTK55wSkRFCJuK?=
 =?us-ascii?Q?aJgJY912gYKB949VUq2Z44kxg3dYOs57Yi4QgUH7bkMm1o8ov5+MLaDJhkmy?=
 =?us-ascii?Q?Ihj+LV5PTNndcM3rTLV8MXVjXwFziviDrC6rR5OP5ArivxT7FnJWvuMmGqqh?=
 =?us-ascii?Q?nl71PAaOFGioqueduGPY5rVk1IOdwhZGu8GVk5ECTirofQVrs3ZUKybQTSUC?=
 =?us-ascii?Q?HYZvzzMxPXtm+SorClTPJgCpqIpHo+0CokWrjTdqltVSA4QpF0yu+vy9e/Fa?=
 =?us-ascii?Q?MrRDLSLZfW7KE0Cemjf+eFqGVU15I9FLUXWsQI9SlvF2QnBZHgl4PMRDRN7f?=
 =?us-ascii?Q?nojL4uGgIYgoey385xvPOPYUq8LIkIb5KdAViKjjAJ5/9v8sgmL0PT43S+nn?=
 =?us-ascii?Q?7fZ0EW4rrAVOUmGm4JX2LdCqWupoGsOA4aQZmT1x/3nsOIZ/rkK7MJmI6Y9Q?=
 =?us-ascii?Q?5ok8IPk9bO2EIN/jcumA5PoJdKkviF+AJg+YwMbZWjXlxudJkFRGGnQ2T8lS?=
 =?us-ascii?Q?p5QyKh9Ce0GPybrWDXuVk1oiXP31KfsROs4XZorisjnY80K8GRF/dwTX6kC1?=
 =?us-ascii?Q?0nTeIvf4HCOXTC5Le8cJtZ/MGgcmf4AUAZQndWtV/v5ElhhbSyowlKy+6BUn?=
 =?us-ascii?Q?7ahRmMXERXUdcDc2mhhSdi8g0YCjtKbxm8tBYZ4boG3V/Lu7+KDME6su6Cbv?=
 =?us-ascii?Q?b1YI+kVDktZ8bb4fWu7RUedT0gnoFV2k5aRrkwnLnv9h/p6Sg3upD/plwyOf?=
 =?us-ascii?Q?6w2eeqClN2zW92Nx3tKuDbhnYpuqrrlEMRLtFLuXWaOL4JHWcqjN6VKVob2e?=
 =?us-ascii?Q?RILbbmc7RGPk4Hf/874MhUKiz+DYm6aul75Y9GBqQSAzLErXJlZnaeuI9fPY?=
 =?us-ascii?Q?T1TzX56gcXVQ4jq6FyS1z9NNp+nxFX96QUVMHYUM?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM4PR12MB5769.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 66d596a3-dc7f-4de6-90c8-08db193c6897
X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Feb 2023 03:32:24.6083
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: EwIJGU2R/PxPJODJVDtaiLc0g7T637f+XJFjHPs5qNcYlz7sXRRAemkbyr2/DHc30+J6D3g2IEBuCwCdkS02bA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR12MB7770
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> -----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.or=
g;
> 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
>=20
> External email: Use caution opening links or attachments
>=20
>=20
> 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.
>=20
> 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"
> :-)
>=20
> 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.
>=20
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 li=
ke
> > + * Tegra241 need cmd, addr & data sent in single message to manage HW
> flow
> > + * control. Each phase sent in different transfer for controller to id=
enity
> > + * 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 =3D 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 =3D min_t(u16, len, MAX_SPI_FRAMESIZE);
> > +
> > +             spi_message_init(&m);
> > +             phy->iobuf[0] =3D (in ? 0x80 : 0) | (transfer_len - 1);
> > +             phy->iobuf[1] =3D 0xd4;
> > +             phy->iobuf[2] =3D addr >> 8;
> > +             phy->iobuf[3] =3D addr;
> > +
> > +             memset(&spi_xfer, 0, sizeof(spi_xfer));
> > +
> > +             spi_xfer[0].tx_buf =3D phy->iobuf;
> > +             spi_xfer[0].len =3D 1;
> > +             spi_message_add_tail(&spi_xfer[0], &m);
> > +
> > +             spi_xfer[1].tx_buf =3D phy->iobuf + 1;
> > +             spi_xfer[1].len =3D 3;
> > +             spi_message_add_tail(&spi_xfer[1], &m);
> > +
> > +             if (out) {
> > +                     spi_xfer[2].tx_buf =3D &phy->iobuf[4];
> > +                     spi_xfer[2].rx_buf =3D NULL;
> > +                     memcpy(&phy->iobuf[4], out, transfer_len);
> > +                     out +=3D transfer_len;
> > +             }
> > +
> > +             if (in) {
> > +                     spi_xfer[2].tx_buf =3D NULL;
> > +                     spi_xfer[2].rx_buf =3D &phy->iobuf[4];
> > +             }
> > +
> > +             spi_xfer[2].len =3D transfer_len;
> > +             spi_message_add_tail(&spi_xfer[2], &m);
> > +
> > +             reinit_completion(&phy->ready);
> > +
> > +             ret =3D spi_sync_locked(phy->spi_device, &m);
> > +             if (ret < 0)
> > +                     return ret;
> > +
> > +             if (in) {
> > +                     memcpy(in, &phy->iobuf[4], transfer_len);
> > +                     in +=3D transfer_len;
> > +             }
> > +
> > +             len -=3D 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 =3D to_tpm_tis_spi_phy(data);
> >       int ret =3D 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 =3D to_tpm_tis_spi_phy(data);
> > +     struct spi_controller *ctlr =3D 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 |=3D 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_mo=
de io_mode)
> >  {
> > --
> > 2.17.1
> >
>=20
> Looking pretty good but do you really want to export
> tpm_tis_spi_{hw,sw}_flow_transfer?
>=20
> BR, Jarkko
No need to export tpm_tis_spi_{hw,sw}_flow_transfer as well.
I will update this in next version.

KY