Re: [PATCH v3 2/6] spi: support simultaneous assertion of multiple CS

[Jonathan Santos]

On 08/18, David Lechner wrote:
> On 8/17/26 6:32 PM, Jonathan Santos wrote:
> > Some SPI controllers allow multiple CS lines to be toggled at the same
> > time. The existing code always used CS index 0 when tracking the last
> > active CS in spi_set_cs(), and unconditionally set cs_index_mask to
> > BIT(0) when parsing DT, both forcing the single CS usage.
> > 
> > Modify spi_set_cs() to iterate last_cs[] using each logical CS index
> > instead of always reading index 0. Modify of_spi_parse_dt() to build
> > cs_index_mask from all parsed CS entries rather than hardcoding BIT(0),
> > so the controller correctly identifies which CS lines belong to a device
> > when asserting them simultaneously.
> > 
> > Board info, ACPI, and ancillary device paths are not updated here.
> > Board info would require an API change to accept an array of CS values
> > and is left for a follow-up when we have a use case for this. Ancillary
> > devices are by design single-CS, so multi-CS is not a current use case for
> > them. ACPI represents the CS as a 64-bit integer with no established
> > convention for encoding multiple CS indices yet, so any extension there
> > would require a separate specification effort.
> > 
> > Acked-by: Nuno Sá <nuno.sa@xxxxxxxxxx>
> > Signed-off-by: Jonathan Santos <Jonathan.Santos@xxxxxxxxxx>
> > ---
> > Changes in v3:
> > * None.
> > 
> > Changes in v2:
> > * Include Summary describind why the other SPI paths were not addressed
> >   here.
> > ---
> >  drivers/spi/spi.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> > index d9e6b4b87c89..55fb96fea243 100644
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -1090,7 +1090,7 @@ static void spi_set_cs(struct spi_device *spi, bool enable, bool force)
> >  	spi->controller->last_cs_index_mask = spi->cs_index_mask;
> >  	for (idx = 0; idx < SPI_DEVICE_CS_CNT_MAX; idx++) {
> >  		if (enable && idx < spi->num_chipselect)
> > -			spi->controller->last_cs[idx] = spi_get_chipselect(spi, 0);
> > +			spi->controller->last_cs[idx] = spi_get_chipselect(spi, idx);
> >  		else
> >  			spi->controller->last_cs[idx] = SPI_INVALID_CS;
> >  	}
> > @@ -2594,10 +2594,11 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
> >  		spi_set_chipselect(spi, idx, cs[idx]);
> >  
> >  	/*
> > -	 * By default spi->chip_select[0] will hold the physical CS number,
> > -	 * so set bit 0 in spi->cs_index_mask.
> > +	 * Set cs_index_mask to indicate which logical CS indices are active.
> > +	 * Each bit corresponds to a logical CS index in the spi->chip_select array.
> >  	 */
> > -	spi->cs_index_mask = BIT(0);
> > +	for (idx = 0; idx < rc; idx++)
> > +		spi->cs_index_mask |= BIT(idx);
> >  
> >  	/* Device speed */
> >  	if (!of_property_read_u32(nc, "spi-max-frequency", &value))
> 
> I have the same concern that sashiko calls out here.
> 
> Existing users of multi-cs (not including spi-mem) follow the pattern
> that a SPI device gets registered with the CS at index 0 and they later
> create an auxiliary using the additional CS. This would cause the main
> device to now assert both CS. Not what we want to happen.

Indeed, enalbing all CS for those cases would cause problems (like you
describe below). AD4080 would be an example.

The devicetree property seems to be a good ideia to fix this and simpler
to execute, but the composite device is interesting.

> 
> If I understood (and remember) the previous explanations of this series
> correctly, we have a different case for this one.
> 
> We want a main device that acts as a single composite device that asserts
> all 4 CS at the same time. Then we also need 4 auxiliary devices that
> only assert one CS at a time for configuring the individual chips.
> 
> So it seems to me like we need a new DT property or some way to be able to
> tell the difference to decide whether we just use the first CS here or all
> of them.
> 
> Perhaps another possibility would be to leave this code the way it is and
> do it this way instead:
> - The SPI device passed to the IIO driver is just the first chip (one CS)
> - The IIO driver then registers auxiliary drivers for the other 3 chips
>   (also 1 CS each)
> - These 4 devices will be used individual to handle configuration.
> - The IIO driver registers a separate composite device that has the
>   multiple lanes and and multiple CS. Whether this using the same
>   auxiliary mechanism with additional parameters or something new
>   probably doesn't matter too much.
> - In this way of doing things, it would not make sense to have the
>   spi-rx-bus-width property in the devcietree since as far as the
>   devicetree is concerned, these are more-or-less 4 separate devices
>   (from the SPI point of view).  Instead, this new composite device
>   registration function would be the one setting the number of lanes
>   based on the number of chip selects.
> 

My initial idea was to use the main device as the "composite" because it
holds the hw description, therefore the multi-lane would be handled by
the existing flow (that's why the spi-rx-bus-width property was
mandatory). This way each ancillary device would take the lane index
from the main device. Using the main + 3 ancillaries for individual
access and the new composite for simultaneous is a great alternative,
but removing the spi-rx-bus-width kind of excludes the information that
we are using multiple lanes for this device. Unless we document that 
"number of CS = number of lanes" on multiple-data-lanes.rst, the 
assumption is:

spi-rx-bus-width = <1>
spi-rx-lane-map = <1>

Also, it would make more sense to take the lane description from the
main device described in the DT considering cases like non-default
mapping. Or at least take it into consideration.
> ---
> sashiko output for reference:
> 
> --- Patch [2]: [PATCH v3 2/6] spi: support simultaneous assertion of multiple CS ---
> commit 17290644299c39d6ca397b14180cd885c5b04919
> Author: Jonathan Santos <Jonathan.Santos@xxxxxxxxxx>
> Subject: spi: support simultaneous assertion of multiple CS
> 
> This commit modifies spi_set_cs() and of_spi_parse_dt() to allow
> simultaneous toggling of multiple chip select lines. It builds
> cs_index_mask from all parsed CS entries instead of hardcoding
> bit 0.
> 
> > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> > index d9e6b4b87c899..55fb96fea2432 100644
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> 
> [ ... ]
> 
> > @@ -2594,10 +2594,11 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
> >  		spi_set_chipselect(spi, idx, cs[idx]);
> >  
> >  	/*
> > -	 * By default spi->chip_select[0] will hold the physical CS number,
> > -	 * so set bit 0 in spi->cs_index_mask.
> > +	 * Set cs_index_mask to indicate which logical CS indices are active.
> > +	 * Each bit corresponds to a logical CS index in the spi->chip_select array.
> >  	 */
> > -	spi->cs_index_mask = BIT(0);
> > +	for (idx = 0; idx < rc; idx++)
> > +		spi->cs_index_mask |= BIT(idx);
> 
> Will this loop break backward compatibility for devices that use multiple
> reg entries for independent logical chip selects?
> 
> For example, if a device has multiple reg entries and uses
> spi_new_ancillary_device(), the primary device's cs_index_mask will now have
> multiple bits set. During a transfer on the primary device, all of these
> parsed chip selects would be asserted simultaneously.
> 
> Could this result in multiple chips driving the shared MISO line at the same
> time, causing bus contention and data corruption?
> 

Yes, that could happen.

> >  
> >  	/* Device speed */
> >  	if (!of_property_read_u32(nc, "spi-max-frequency", &value))