[1/3] spi: dw: select SS0 when gpio cs is used

Commit Message

Edmund Berenson Dec. 2, 2022, 9:48 a.m. UTC

SER register contains only 4-bit bit-field for enabling 4 SPI chip selects.
If gpio cs are used the cs number may be >= 4. To ensure we do not write
outside of the valid area, we choose SS0 in case of gpio cs to start
spi transfer.

Co-developed-by: Lukasz Zemla <[e-mail]>
Signed-off-by: Lukasz Zemla <[e-mail]>
Signed-off-by: Edmund Berenson <[e-mail]>
---
 drivers/spi/spi-dw-core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Serge Semin Dec. 9, 2022, 11:46 a.m. UTC | #1

Hello Edmund

On Fri, Dec 02, 2022 at 10:48:59AM +0100, Edmund Berenson wrote:
> SER register contains only 4-bit bit-field for enabling 4 SPI chip selects.
> If gpio cs are used the cs number may be >= 4. To ensure we do not write
> outside of the valid area, we choose SS0 in case of gpio cs to start
> spi transfer.

Next time please don't forget to add me to the whole series Cc-list. I
am missing the patch #2 in my inbox.

> 
> Co-developed-by: Lukasz Zemla <[e-mail]>
> Signed-off-by: Lukasz Zemla <[e-mail]>
> Signed-off-by: Edmund Berenson <[e-mail]>
> ---
>  drivers/spi/spi-dw-core.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index 99edddf9958b..57c9e384d6d4 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -94,6 +94,10 @@ void dw_spi_set_cs(struct spi_device *spi, bool enable)
>  {
>  	struct dw_spi *dws = spi_controller_get_devdata(spi->controller);
>  	bool cs_high = !!(spi->mode & SPI_CS_HIGH);
> +	u8 enable_cs = 0;
> +
> +	if (!spi->cs_gpiod)
> +		enable_cs = spi->chip_select;
>  
>  	/*
>  	 * DW SPI controller demands any native CS being set in order to
> @@ -103,7 +107,7 @@ void dw_spi_set_cs(struct spi_device *spi, bool enable)
>  	 * support active-high or active-low CS level.
>  	 */
>  	if (cs_high == enable)

> -		dw_writel(dws, DW_SPI_SER, BIT(spi->chip_select));
> +		dw_writel(dws, DW_SPI_SER, BIT(enable_cs));

No, it's not that easy. By applying this patch we'll get a regression
for the platforms which make use of both the GPIO-based and native
chip-selects. Consider the next platform setup:
 +--------------+
 | SoC X        |
 |              |    +-------------+
 |   DW SSI CS0-+----+ SPI-slave 0 |
 |              |    +-------------+
 |              |    +-------------+
 |        GPIOn-+----+ SPI-slave 1 |
 |              |    +-------------+
 +--------------+

If we apply this patch then the communications targeted to the
SPI-slave 1 will also reach the SPI-slave 0 device, which isn't what
we'd want.

That's why currently the DW SSI driver activates the native CS line
with the corresponding ID irrespective whether it is a GPIO-based
CS or a native one.

-Serge(y)

>  	else
>  		dw_writel(dws, DW_SPI_SER, 0);
>  }
> -- 
> 2.37.4
>

Edmund Berenson Dec. 9, 2022, 12:13 p.m. UTC | #2

Hi Serge,

On Fri, Dec 09, 2022 at 02:46:25PM +0300, Serge Semin wrote:
> Hello Edmund
> 
> On Fri, Dec 02, 2022 at 10:48:59AM +0100, Edmund Berenson wrote:
> > SER register contains only 4-bit bit-field for enabling 4 SPI chip selects.
> > If gpio cs are used the cs number may be >= 4. To ensure we do not write
> > outside of the valid area, we choose SS0 in case of gpio cs to start
> > spi transfer.
> 
> Next time please don't forget to add me to the whole series Cc-list. I
> am missing the patch #2 in my inbox.
> 
I am sorry, I probably made some mistake when sending the mail.
I forwarded you patch 2 as well.
> > 
> > Co-developed-by: Lukasz Zemla <[e-mail]>
> > Signed-off-by: Lukasz Zemla <[e-mail]>
> > Signed-off-by: Edmund Berenson <[e-mail]>
> > ---
> >  drivers/spi/spi-dw-core.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> > index 99edddf9958b..57c9e384d6d4 100644
> > --- a/drivers/spi/spi-dw-core.c
> > +++ b/drivers/spi/spi-dw-core.c
> > @@ -94,6 +94,10 @@ void dw_spi_set_cs(struct spi_device *spi, bool enable)
> >  {
> >  	struct dw_spi *dws = spi_controller_get_devdata(spi->controller);
> >  	bool cs_high = !!(spi->mode & SPI_CS_HIGH);
> > +	u8 enable_cs = 0;
> > +
> > +	if (!spi->cs_gpiod)
> > +		enable_cs = spi->chip_select;
> >  
> >  	/*
> >  	 * DW SPI controller demands any native CS being set in order to
> > @@ -103,7 +107,7 @@ void dw_spi_set_cs(struct spi_device *spi, bool enable)
> >  	 * support active-high or active-low CS level.
> >  	 */
> >  	if (cs_high == enable)
> 
> > -		dw_writel(dws, DW_SPI_SER, BIT(spi->chip_select));
> > +		dw_writel(dws, DW_SPI_SER, BIT(enable_cs));
> 
> No, it's not that easy. By applying this patch we'll get a regression
> for the platforms which make use of both the GPIO-based and native
> chip-selects. Consider the next platform setup:
>  +--------------+
>  | SoC X        |
>  |              |    +-------------+
>  |   DW SSI CS0-+----+ SPI-slave 0 |
>  |              |    +-------------+
>  |              |    +-------------+
>  |        GPIOn-+----+ SPI-slave 1 |
>  |              |    +-------------+
>  +--------------+
> 
> If we apply this patch then the communications targeted to the
> SPI-slave 1 will also reach the SPI-slave 0 device, which isn't what
> we'd want.
> 
> That's why currently the DW SSI driver activates the native CS line
> with the corresponding ID irrespective whether it is a GPIO-based
> CS or a native one.
Okay that is actually true... but we will have to guard against CS>4 as only two
bits are reserved for cs in the register.
If both gpio and native cs are used at least one native cs
has to be empty otherwise we will have at least one double activation.
I am not sure if there is a "clean" way to determine which native cs is unused
inside the driver. Do you think it would be an acceptable workaround to
add an entry to the device tree like native-gpio cs?
> 
> -Serge(y)
> 
> >  	else
> >  		dw_writel(dws, DW_SPI_SER, 0);
> >  }
> > -- 
> > 2.37.4
> >

Serge Semin Dec. 12, 2022, 12:16 p.m. UTC | #3

On Fri, Dec 09, 2022 at 01:13:54PM +0100, Edmund Berenson wrote:
> Hi Serge,
> 
> On Fri, Dec 09, 2022 at 02:46:25PM +0300, Serge Semin wrote:
> > Hello Edmund
> > 
> > On Fri, Dec 02, 2022 at 10:48:59AM +0100, Edmund Berenson wrote:
> > > SER register contains only 4-bit bit-field for enabling 4 SPI chip selects.
> > > If gpio cs are used the cs number may be >= 4. To ensure we do not write
> > > outside of the valid area, we choose SS0 in case of gpio cs to start
> > > spi transfer.
> > 
> > Next time please don't forget to add me to the whole series Cc-list. I
> > am missing the patch #2 in my inbox.
> > 
> I am sorry, I probably made some mistake when sending the mail.
> I forwarded you patch 2 as well.
> > > 
> > > Co-developed-by: Lukasz Zemla <[e-mail]>
> > > Signed-off-by: Lukasz Zemla <[e-mail]>
> > > Signed-off-by: Edmund Berenson <[e-mail]>
> > > ---
> > >  drivers/spi/spi-dw-core.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> > > index 99edddf9958b..57c9e384d6d4 100644
> > > --- a/drivers/spi/spi-dw-core.c
> > > +++ b/drivers/spi/spi-dw-core.c
> > > @@ -94,6 +94,10 @@ void dw_spi_set_cs(struct spi_device *spi, bool enable)
> > >  {
> > >  	struct dw_spi *dws = spi_controller_get_devdata(spi->controller);
> > >  	bool cs_high = !!(spi->mode & SPI_CS_HIGH);
> > > +	u8 enable_cs = 0;
> > > +
> > > +	if (!spi->cs_gpiod)
> > > +		enable_cs = spi->chip_select;
> > >  
> > >  	/*
> > >  	 * DW SPI controller demands any native CS being set in order to
> > > @@ -103,7 +107,7 @@ void dw_spi_set_cs(struct spi_device *spi, bool enable)
> > >  	 * support active-high or active-low CS level.
> > >  	 */
> > >  	if (cs_high == enable)
> > 
> > > -		dw_writel(dws, DW_SPI_SER, BIT(spi->chip_select));
> > > +		dw_writel(dws, DW_SPI_SER, BIT(enable_cs));
> > 
> > No, it's not that easy. By applying this patch we'll get a regression
> > for the platforms which make use of both the GPIO-based and native
> > chip-selects. Consider the next platform setup:
> >  +--------------+
> >  | SoC X        |
> >  |              |    +-------------+
> >  |   DW SSI CS0-+----+ SPI-slave 0 |
> >  |              |    +-------------+
> >  |              |    +-------------+
> >  |        GPIOn-+----+ SPI-slave 1 |
> >  |              |    +-------------+
> >  +--------------+
> > 
> > If we apply this patch then the communications targeted to the
> > SPI-slave 1 will also reach the SPI-slave 0 device, which isn't what
> > we'd want.
> > 
> > That's why currently the DW SSI driver activates the native CS line
> > with the corresponding ID irrespective whether it is a GPIO-based
> > CS or a native one.

> Okay that is actually true... but we will have to guard against CS>4 as only two
> bits are reserved for cs in the register.

Firstly note that DW SSI can be synthesized with having up to 16
slaves. The number of available native chip-selects is normally
defined by the "num-cs" DT-property. (Though the DT-bindings
incorrectly set the upper limit to 4 slaves only.)

Secondly if you want to have a sanity check of the specific slave ID
then I'd suggest to use the dw_spi_setup() method for that. Just add
the conditional statement like "if (spi->chip_select >= dws->num_cs)
return -EINVAL" in there. Note this modification will solidify the
semantic of having less than num_cs chip-selects.

Thirdly also note the number of native chip-selects is auto-detectable
by writing FFs to the SER register. So we can avoid defaulting the
num_cs to 4 in the spi-dw-mmio.c driver and try to auto-detect the
number of chip-selects (the dw_spi_hw_init() method is the best
candidate for that procedure), if no "num-cs" property was specified.

> If both gpio and native cs are used at least one native cs
> has to be empty otherwise we will have at least one double activation.
> I am not sure if there is a "clean" way to determine which native cs is unused
> inside the driver. Do you think it would be an acceptable workaround to
> add an entry to the device tree like native-gpio cs?

Currently the DW SSI driver implies having a native CS behind each
GPIO-based chip-select. It is used to activate the communications.
That semantic is implicitly advertised to the SPI subsystem core by
setting the SPI_MASTER_GPIO_SS flag.

Before thinking of a best way to implement what you suggest I need to
ask: Do you really need to have the extended number of CSs support? Do
you have any practical need in that? If you don't, then I would
suggest to leave things as is. (The suggested sanity check might be
useful though.) If you do, then we'll need to come up with a algo,
which would imply detecting the GPIO-based chip-selects in the
controller probe procedure and using one of their native counterpart
(for instance the very last one or very first one) to activate either
all the GPIO-based CS transfer exceeding the number of available
native chip-selects, or just all the GPIO-based communications.

-Serge(y)

> > 
> > -Serge(y)
> > 
> > >  	else
> > >  		dw_writel(dws, DW_SPI_SER, 0);
> > >  }
> > > -- 
> > > 2.37.4
> > >

Patch

diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index 99edddf9958b..57c9e384d6d4 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c
@@ -94,6 +94,10 @@  void dw_spi_set_cs(struct spi_device *spi, bool enable)
 {
 	struct dw_spi *dws = spi_controller_get_devdata(spi->controller);
 	bool cs_high = !!(spi->mode & SPI_CS_HIGH);
+	u8 enable_cs = 0;
+
+	if (!spi->cs_gpiod)
+		enable_cs = spi->chip_select;
 
 	/*
 	 * DW SPI controller demands any native CS being set in order to
@@ -103,7 +107,7 @@  void dw_spi_set_cs(struct spi_device *spi, bool enable)
 	 * support active-high or active-low CS level.
 	 */
 	if (cs_high == enable)
-		dw_writel(dws, DW_SPI_SER, BIT(spi->chip_select));
+		dw_writel(dws, DW_SPI_SER, BIT(enable_cs));
 	else
 		dw_writel(dws, DW_SPI_SER, 0);
 }