mbox series

[0/3] spi: tegra quad: Add Tegra Grace features

Message ID 20220317012006.15080-1-kyarlagadda@nvidia.com
Headers show
Series spi: tegra quad: Add Tegra Grace features | expand

Message

Krishna Yarlagadda March 17, 2022, 1:20 a.m. UTC
Add multiple chip selects and hardware wait state polling supported
by Tegra Grace.

Krishna Yarlagadda (3):
  spi: tegra210-quad: Multi-cs support
  spi: tegra210-quad: Add wait polling support
  spi: dt-bindings: Add wait state polling flag

 .../bindings/spi/nvidia,tegra210-quad.yaml    |  6 ++
 drivers/spi/spi-tegra210-quad.c               | 64 ++++++++++++++++++-
 2 files changed, 68 insertions(+), 2 deletions(-)

Comments

Krishna Yarlagadda March 17, 2022, 9:02 a.m. UTC | #1
> -----Original Message-----
> From: Jonathan Hunter <jonathanh@nvidia.com>
> Sent: 17 March 2022 14:25
> To: Krishna Yarlagadda <kyarlagadda@nvidia.com>; broonie@kernel.org; thierry.reding@gmail.com; linux-spi@vger.kernel.org; linux-
> tegra@vger.kernel.org; Ashish Singhal <ashishsingha@nvidia.com>
> Cc: Sowjanya Komatineni <skomatineni@nvidia.com>; Laxman Dewangan <ldewangan@nvidia.com>; robh+dt@kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/3] spi: tegra210-quad: Add wait polling support
> 
> 
> On 17/03/2022 01:20, Krishna Yarlagadda wrote:
> > Controller can poll for wait state inserted by TPM device and
> > handle it.
> >
> > Signed-off-by: Krishna Yarlagadda <kyarlagadda@nvidia.com>
> > ---
> >   drivers/spi/spi-tegra210-quad.c | 31 +++++++++++++++++++++++++++++++
> >   1 file changed, 31 insertions(+)
> >
> > diff --git a/drivers/spi/spi-tegra210-quad.c b/drivers/spi/spi-tegra210-quad.c
> > index a2e225e8f7f0..ecf171bfcdce 100644
> > --- a/drivers/spi/spi-tegra210-quad.c
> > +++ b/drivers/spi/spi-tegra210-quad.c
> > @@ -142,6 +142,7 @@
> >
> >   #define QSPI_GLOBAL_CONFIG			0X1a4
> >   #define QSPI_CMB_SEQ_EN				BIT(0)
> > +#define QSPI_TPM_WAIT_POLL_EN			BIT(1)
> >
> >   #define QSPI_CMB_SEQ_ADDR			0x1a8
> >   #define QSPI_ADDRESS_VALUE_SET(X)		(((x) & 0xFFFF) << 0)
> > @@ -165,11 +166,13 @@ struct tegra_qspi_soc_data {
> >   	bool has_dma;
> >   	bool cmb_xfer_capable;
> >   	bool cs_count;
> > +	bool has_wait_polling;
> >   };
> >
> >   struct tegra_qspi_client_data {
> >   	int tx_clk_tap_delay;
> >   	int rx_clk_tap_delay;
> > +	bool wait_polling;
> >   };
> >
> >   struct tegra_qspi {
> > @@ -833,6 +836,11 @@ static u32 tegra_qspi_setup_transfer_one(struct spi_device *spi, struct spi_tran
> >   		else
> >   			command1 |= QSPI_CONTROL_MODE_0;
> >
> > +		if (tqspi->soc_data->cmb_xfer_capable)
> > +			command1 &= ~QSPI_CS_SW_HW;
> > +		else
> > +			command1 |= QSPI_CS_SW_HW;
> > +
> >   		if (spi->mode & SPI_CS_HIGH)
> >   			command1 |= QSPI_CS_SW_VAL;
> >   		else
> > @@ -917,6 +925,7 @@ static int tegra_qspi_start_transfer_one(struct spi_device *spi,
> >
> >   static struct tegra_qspi_client_data *tegra_qspi_parse_cdata_dt(struct spi_device *spi)
> >   {
> > +	struct tegra_qspi *tqspi = spi_master_get_devdata(spi->master);
> >   	struct tegra_qspi_client_data *cdata;
> >
> >   	cdata = devm_kzalloc(&spi->dev, sizeof(*cdata), GFP_KERNEL);
> > @@ -927,6 +936,11 @@ static struct tegra_qspi_client_data *tegra_qspi_parse_cdata_dt(struct spi_devic
> >   				 &cdata->tx_clk_tap_delay);
> >   	device_property_read_u32(&spi->dev, "nvidia,rx-clk-tap-delay",
> >   				 &cdata->rx_clk_tap_delay);
> > +	if (tqspi->soc_data->has_wait_polling)
> > +		cdata->wait_polling = device_property_read_bool
> > +					(&spi->dev,
> > +					 "nvidia,wait-polling");
> > +
> 
> 
> This looks odd. Why do we need this device-tree property if it is
> already specified in the SoC data?
Soc data specifies chip is capable of wait-polling.
Wait polling still has to be selected on slave devices that can support it.
I will add one line description for the properties in next version.
> 
> Jon
> 
> --
> nvpublic
Jon Hunter March 17, 2022, 3:26 p.m. UTC | #2
On 17/03/2022 09:44, Jon Hunter wrote:
> 
> On 17/03/2022 09:02, Krishna Yarlagadda wrote:
>>> -----Original Message-----
>>> From: Jonathan Hunter <jonathanh@nvidia.com>
>>> Sent: 17 March 2022 14:25
>>> To: Krishna Yarlagadda <kyarlagadda@nvidia.com>; broonie@kernel.org; 
>>> thierry.reding@gmail.com; linux-spi@vger.kernel.org; linux-
>>> tegra@vger.kernel.org; Ashish Singhal <ashishsingha@nvidia.com>
>>> Cc: Sowjanya Komatineni <skomatineni@nvidia.com>; Laxman Dewangan 
>>> <ldewangan@nvidia.com>; robh+dt@kernel.org;
>>> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
>>> Subject: Re: [PATCH 2/3] spi: tegra210-quad: Add wait polling support
>>>
>>>
>>> On 17/03/2022 01:20, Krishna Yarlagadda wrote:
>>>> Controller can poll for wait state inserted by TPM device and
>>>> handle it.
>>>>
>>>> Signed-off-by: Krishna Yarlagadda <kyarlagadda@nvidia.com>
>>>> ---
>>>>    drivers/spi/spi-tegra210-quad.c | 31 +++++++++++++++++++++++++++++++
>>>>    1 file changed, 31 insertions(+)
>>>>
>>>> diff --git a/drivers/spi/spi-tegra210-quad.c 
>>>> b/drivers/spi/spi-tegra210-quad.c
>>>> index a2e225e8f7f0..ecf171bfcdce 100644
>>>> --- a/drivers/spi/spi-tegra210-quad.c
>>>> +++ b/drivers/spi/spi-tegra210-quad.c
>>>> @@ -142,6 +142,7 @@
>>>>
>>>>    #define QSPI_GLOBAL_CONFIG            0X1a4
>>>>    #define QSPI_CMB_SEQ_EN                BIT(0)
>>>> +#define QSPI_TPM_WAIT_POLL_EN            BIT(1)
>>>>
>>>>    #define QSPI_CMB_SEQ_ADDR            0x1a8
>>>>    #define QSPI_ADDRESS_VALUE_SET(X)        (((x) & 0xFFFF) << 0)
>>>> @@ -165,11 +166,13 @@ struct tegra_qspi_soc_data {
>>>>        bool has_dma;
>>>>        bool cmb_xfer_capable;
>>>>        bool cs_count;
>>>> +    bool has_wait_polling;
>>>>    };
>>>>
>>>>    struct tegra_qspi_client_data {
>>>>        int tx_clk_tap_delay;
>>>>        int rx_clk_tap_delay;
>>>> +    bool wait_polling;
>>>>    };
>>>>
>>>>    struct tegra_qspi {
>>>> @@ -833,6 +836,11 @@ static u32 tegra_qspi_setup_transfer_one(struct 
>>>> spi_device *spi, struct spi_tran
>>>>            else
>>>>                command1 |= QSPI_CONTROL_MODE_0;
>>>>
>>>> +        if (tqspi->soc_data->cmb_xfer_capable)
>>>> +            command1 &= ~QSPI_CS_SW_HW;
>>>> +        else
>>>> +            command1 |= QSPI_CS_SW_HW;
>>>> +
>>>>            if (spi->mode & SPI_CS_HIGH)
>>>>                command1 |= QSPI_CS_SW_VAL;
>>>>            else
>>>> @@ -917,6 +925,7 @@ static int tegra_qspi_start_transfer_one(struct 
>>>> spi_device *spi,
>>>>
>>>>    static struct tegra_qspi_client_data 
>>>> *tegra_qspi_parse_cdata_dt(struct spi_device *spi)
>>>>    {
>>>> +    struct tegra_qspi *tqspi = spi_master_get_devdata(spi->master);
>>>>        struct tegra_qspi_client_data *cdata;
>>>>
>>>>        cdata = devm_kzalloc(&spi->dev, sizeof(*cdata), GFP_KERNEL);
>>>> @@ -927,6 +936,11 @@ static struct tegra_qspi_client_data 
>>>> *tegra_qspi_parse_cdata_dt(struct spi_devic
>>>>                     &cdata->tx_clk_tap_delay);
>>>>        device_property_read_u32(&spi->dev, "nvidia,rx-clk-tap-delay",
>>>>                     &cdata->rx_clk_tap_delay);
>>>> +    if (tqspi->soc_data->has_wait_polling)
>>>> +        cdata->wait_polling = device_property_read_bool
>>>> +                    (&spi->dev,
>>>> +                     "nvidia,wait-polling");
>>>> +
>>>
>>>
>>> This looks odd. Why do we need this device-tree property if it is
>>> already specified in the SoC data?
>> Soc data specifies chip is capable of wait-polling.
>> Wait polling still has to be selected on slave devices that can 
>> support it.
>> I will add one line description for the properties in next version.
> 
> 
> I can't say I am familiar with this, but it seems that the ideal 
> solution would be able to detect if this needs to be enabled depending 
> on the device connected. Is that not possible?

Also, given that Grace supports 4 chip-selects per device, how does this 
work if there is TPM connected to one chip-select and something else 
connected to another?

Jon
Krishna Yarlagadda March 17, 2022, 6 p.m. UTC | #3
Regards
KY

> -----Original Message-----
> From: Jonathan Hunter <jonathanh@nvidia.com>
> Sent: 17 March 2022 20:56
> To: Krishna Yarlagadda <kyarlagadda@nvidia.com>; broonie@kernel.org; thierry.reding@gmail.com; linux-spi@vger.kernel.org;
> linux-tegra@vger.kernel.org; Ashish Singhal <ashishsingha@nvidia.com>
> Cc: Sowjanya Komatineni <skomatineni@nvidia.com>; Laxman Dewangan <ldewangan@nvidia.com>; robh+dt@kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/3] spi: tegra210-quad: Add wait polling support
> 
> 
> On 17/03/2022 09:44, Jon Hunter wrote:
> >
> > On 17/03/2022 09:02, Krishna Yarlagadda wrote:
> >>> -----Original Message-----
> >>> From: Jonathan Hunter <jonathanh@nvidia.com>
> >>> Sent: 17 March 2022 14:25
> >>> To: Krishna Yarlagadda <kyarlagadda@nvidia.com>; broonie@kernel.org;
> >>> thierry.reding@gmail.com; linux-spi@vger.kernel.org; linux-
> >>> tegra@vger.kernel.org; Ashish Singhal <ashishsingha@nvidia.com>
> >>> Cc: Sowjanya Komatineni <skomatineni@nvidia.com>; Laxman Dewangan
> >>> <ldewangan@nvidia.com>; robh+dt@kernel.org;
> >>> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> >>> Subject: Re: [PATCH 2/3] spi: tegra210-quad: Add wait polling support
> >>>
> >>>
> >>> On 17/03/2022 01:20, Krishna Yarlagadda wrote:
> >>>> Controller can poll for wait state inserted by TPM device and
> >>>> handle it.
> >>>>
> >>>> Signed-off-by: Krishna Yarlagadda <kyarlagadda@nvidia.com>
> >>>> ---
> >>>>    drivers/spi/spi-tegra210-quad.c | 31 +++++++++++++++++++++++++++++++
> >>>>    1 file changed, 31 insertions(+)
> >>>>
> >>>> diff --git a/drivers/spi/spi-tegra210-quad.c
> >>>> b/drivers/spi/spi-tegra210-quad.c
> >>>> index a2e225e8f7f0..ecf171bfcdce 100644
> >>>> --- a/drivers/spi/spi-tegra210-quad.c
> >>>> +++ b/drivers/spi/spi-tegra210-quad.c
> >>>> @@ -142,6 +142,7 @@
> >>>>
> >>>>    #define QSPI_GLOBAL_CONFIG            0X1a4
> >>>>    #define QSPI_CMB_SEQ_EN                BIT(0)
> >>>> +#define QSPI_TPM_WAIT_POLL_EN            BIT(1)
> >>>>
> >>>>    #define QSPI_CMB_SEQ_ADDR            0x1a8
> >>>>    #define QSPI_ADDRESS_VALUE_SET(X)        (((x) & 0xFFFF) << 0)
> >>>> @@ -165,11 +166,13 @@ struct tegra_qspi_soc_data {
> >>>>        bool has_dma;
> >>>>        bool cmb_xfer_capable;
> >>>>        bool cs_count;
> >>>> +    bool has_wait_polling;
> >>>>    };
> >>>>
> >>>>    struct tegra_qspi_client_data {
> >>>>        int tx_clk_tap_delay;
> >>>>        int rx_clk_tap_delay;
> >>>> +    bool wait_polling;
> >>>>    };
> >>>>
> >>>>    struct tegra_qspi {
> >>>> @@ -833,6 +836,11 @@ static u32 tegra_qspi_setup_transfer_one(struct
> >>>> spi_device *spi, struct spi_tran
> >>>>            else
> >>>>                command1 |= QSPI_CONTROL_MODE_0;
> >>>>
> >>>> +        if (tqspi->soc_data->cmb_xfer_capable)
> >>>> +            command1 &= ~QSPI_CS_SW_HW;
> >>>> +        else
> >>>> +            command1 |= QSPI_CS_SW_HW;
> >>>> +
> >>>>            if (spi->mode & SPI_CS_HIGH)
> >>>>                command1 |= QSPI_CS_SW_VAL;
> >>>>            else
> >>>> @@ -917,6 +925,7 @@ static int tegra_qspi_start_transfer_one(struct
> >>>> spi_device *spi,
> >>>>
> >>>>    static struct tegra_qspi_client_data
> >>>> *tegra_qspi_parse_cdata_dt(struct spi_device *spi)
> >>>>    {
> >>>> +    struct tegra_qspi *tqspi = spi_master_get_devdata(spi->master);
> >>>>        struct tegra_qspi_client_data *cdata;
> >>>>
> >>>>        cdata = devm_kzalloc(&spi->dev, sizeof(*cdata), GFP_KERNEL);
> >>>> @@ -927,6 +936,11 @@ static struct tegra_qspi_client_data
> >>>> *tegra_qspi_parse_cdata_dt(struct spi_devic
> >>>>                     &cdata->tx_clk_tap_delay);
> >>>>        device_property_read_u32(&spi->dev, "nvidia,rx-clk-tap-delay",
> >>>>                     &cdata->rx_clk_tap_delay);
> >>>> +    if (tqspi->soc_data->has_wait_polling)
> >>>> +        cdata->wait_polling = device_property_read_bool
> >>>> +                    (&spi->dev,
> >>>> +                     "nvidia,wait-polling");
> >>>> +
> >>>
> >>>
> >>> This looks odd. Why do we need this device-tree property if it is
> >>> already specified in the SoC data?
> >> Soc data specifies chip is capable of wait-polling.
> >> Wait polling still has to be selected on slave devices that can
> >> support it.
> >> I will add one line description for the properties in next version.
> >
> >
> > I can't say I am familiar with this, but it seems that the ideal
> > solution would be able to detect if this needs to be enabled depending
> > on the device connected. Is that not possible?
We cannot detect at runtime which slave supports wait polling.
> 
> Also, given that Grace supports 4 chip-selects per device, how does this
> work if there is TPM connected to one chip-select and something else
> connected to another?
Wait polling is part of client data which is unique for each cs slave.
This flag will be enabled on controller register while initiating transfer
for TPM slave and disabled for all other transfers.
> 
> Jon
> 
> --
> nvpublic
Mark Brown March 17, 2022, 6:09 p.m. UTC | #4
On Thu, Mar 17, 2022 at 06:00:25PM +0000, Krishna Yarlagadda wrote:

> > >> Wait polling still has to be selected on slave devices that can
> > >> support it.
> > >> I will add one line description for the properties in next version.

> > > I can't say I am familiar with this, but it seems that the ideal
> > > solution would be able to detect if this needs to be enabled depending
> > > on the device connected. Is that not possible?

> We cannot detect at runtime which slave supports wait polling.

How would the client device have any knowledge of how the controller
internal implementation works?  What even is a "wait state inserted by
TPM device"?  If this is something the client device is doing then it
should be something that is expressed through the generic SPI API by the
client device, if some controllers have some way of optimising this then
we can add support for that but presumably it's just something that's
generically supported by that device so we shouldn't need a DT property
for it on the device side either.