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

[Serge Semin]

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 <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.

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
> > >