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

[Edmund Berenson <edmund.berenson@xxxxxxxxx>]

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 <Lukasz.Zemla@xxxxxxxxxxxx>
> > Signed-off-by: Lukasz Zemla <Lukasz.Zemla@xxxxxxxxxxxx>
> > Signed-off-by: Edmund Berenson <edmund.berenson@xxxxxxxxx>
> > ---
> >  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
> >