Message ID | 20231118135446.52783-2-amit.kumar-mahapatra@amd.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | spi: Add support for stacked/parallel memories | expand |
On Sat, Nov 18, 2023 at 07:24:39PM +0530, Amit Kumar Mahapatra wrote: > AMD-Xilinx GQSPI controller has two advanced mode that allows the > controller to consider two flashes as one single device. This breaks an x86 allmodconfig build: /build/stage/linux/sound/pci/hda/cs35l56_hda_spi.c: In function ‘cs35l56_hda_spi _probe’: /build/stage/linux/sound/pci/hda/cs35l56_hda_spi.c:32:52: error: passing argumen t 2 of ‘cs35l56_hda_common_probe’ makes integer from pointer without a cast [-We rror=int-conversion] 32 | ret = cs35l56_hda_common_probe(cs35l56, spi->chip_select); | ~~~^~~~~~~~~~~~~ | | | u8 * {aka unsigned ch ar *} In file included from /build/stage/linux/sound/pci/hda/cs35l56_hda_spi.c:12: /build/stage/linux/sound/pci/hda/cs35l56_hda.h:45:63: note: expected ‘int’ but a rgument is of type ‘u8 *’ {aka ‘unsigned char *’} 45 | int cs35l56_hda_common_probe(struct cs35l56_hda *cs35l56, int id); | ~~~~^~
On 18/11/2023 13:54, Amit Kumar Mahapatra wrote: > AMD-Xilinx GQSPI controller has two advanced mode that allows the > controller to consider two flashes as one single device. > > One of these two mode is the parallel mode in which each byte of data is > stored in both devices, the even bits in the lower flash & the odd bits in > the upper flash. The byte split is automatically handled by the QSPI > controller. > > The other mode is the stacked mode in which both the flashes share the > same SPI bus but each of the device contain half of the data. In this mode, > the controller does not follow CS requests but instead internally wires the > two CS levels with the value of the most significant address bit. > > For supporting both these modes SPI core need to be updated for providing > multiple CS for a single SPI device. > > For adding multi CS support the SPI device need to be aware of all the CS > values. So, the "chip_select" member in the spi_device structure is now an > array that holds all the CS values. > > spi_device structure now has a "cs_index_mask" member. This acts as an > index to the chip_select array. If nth bit of spi->cs_index_mask is set > then the driver would assert spi->chip_select[n]. > > In parallel mode all the chip selects are asserted/de-asserted > simultaneously and each byte of data is stored in both devices, the even > bits in one, the odd bits in the other. The split is automatically handled > by the GQSPI controller. The GQSPI controller supports a maximum of two > flashes connected in parallel mode. A SPI_CONTROLLER_MULTI_CS flag bit is > added in the spi controller flags, through ctlr->flags the spi core > will make sure that the controller is capable of handling multiple chip > selects at once. > > For supporting multiple CS via GPIO the cs_gpiod member of the spi_device > structure is now an array that holds the gpio descriptor for each > chipselect. > > CS GPIO is not tested due to unavailability of necessary hardware setup. > > Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com> > --- > Hello @Stefen, > We restructured the SPI core implementation, for handling arbitrary number > of devices connected in parallel(multi-cs) or stacked mode. We have tested > this updated patch on native-cs setup but couldn't test cs-gpio due to > unavailability of a proper setup. If possible, could you please retest > the cs-gpio use case with this updated patch and provide your feedback. Hi, I tested this chain on 2 different systems using GPIOs as chip selects, and see the same error on both: [ 2.842045] pxa2xx-spi pxa2xx-spi.6: chipselect 0 already in use Let me know if you need any further testing. Thanks, Stefan Binding > --- > drivers/spi/spi.c | 192 +++++++++++++++++++++++++++++++--------- > include/linux/spi/spi.h | 51 ++++++++--- > 2 files changed, 191 insertions(+), 52 deletions(-) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index 8ead7acb99f3..ff66147ba95f 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -612,10 +612,21 @@ static int spi_dev_check(struct device *dev, void *data) > { > struct spi_device *spi = to_spi_device(dev); > struct spi_device *new_spi = data; > - > - if (spi->controller == new_spi->controller && > - spi_get_chipselect(spi, 0) == spi_get_chipselect(new_spi, 0)) > - return -EBUSY; > + int idx, nw_idx; > + u8 cs, cs_nw; > + > + if (spi->controller == new_spi->controller) { > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > + cs = spi_get_chipselect(spi, idx); > + for (nw_idx = 0; nw_idx < SPI_CS_CNT_MAX; nw_idx++) { > + cs_nw = spi_get_chipselect(spi, nw_idx); > + if (cs != 0xFF && cs_nw != 0xFF && cs == cs_nw) { > + dev_err(dev, "chipselect %d already in use\n", cs_nw); > + return -EBUSY; > + } > + } > + } > + } > return 0; > } > > @@ -629,13 +640,32 @@ static int __spi_add_device(struct spi_device *spi) > { > struct spi_controller *ctlr = spi->controller; > struct device *dev = ctlr->dev.parent; > - int status; > + int status, idx, nw_idx; > + u8 cs, nw_cs; > + > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > + /* Chipselects are numbered 0..max; validate. */ > + cs = spi_get_chipselect(spi, idx); > + if (cs != 0xFF && cs >= ctlr->num_chipselect) { > + dev_err(dev, "cs%d >= max %d\n", spi_get_chipselect(spi, idx), > + ctlr->num_chipselect); > + return -EINVAL; > + } > + } > > - /* Chipselects are numbered 0..max; validate. */ > - if (spi_get_chipselect(spi, 0) >= ctlr->num_chipselect) { > - dev_err(dev, "cs%d >= max %d\n", spi_get_chipselect(spi, 0), > - ctlr->num_chipselect); > - return -EINVAL; > + /* > + * Make sure that multiple logical CS doesn't map to the same physical CS. > + * For example, spi->chip_select[0] != spi->chip_select[1] and so on. > + */ > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > + cs = spi_get_chipselect(spi, idx); > + for (nw_idx = idx + 1; nw_idx < SPI_CS_CNT_MAX; nw_idx++) { > + nw_cs = spi_get_chipselect(spi, nw_idx); > + if (cs != 0xFF && nw_cs != 0xFF && cs == nw_cs) { > + dev_err(dev, "chipselect %d already in use\n", nw_cs); > + return -EBUSY; > + } > + } > } > > /* Set the bus ID string */ > @@ -647,11 +677,8 @@ static int __spi_add_device(struct spi_device *spi) > * its configuration. > */ > status = bus_for_each_dev(&spi_bus_type, NULL, spi, spi_dev_check); > - if (status) { > - dev_err(dev, "chipselect %d already in use\n", > - spi_get_chipselect(spi, 0)); > + if (status) > return status; > - } > > /* Controller may unregister concurrently */ > if (IS_ENABLED(CONFIG_SPI_DYNAMIC) && > @@ -659,8 +686,15 @@ static int __spi_add_device(struct spi_device *spi) > return -ENODEV; > } > > - if (ctlr->cs_gpiods) > - spi_set_csgpiod(spi, 0, ctlr->cs_gpiods[spi_get_chipselect(spi, 0)]); > + if (ctlr->cs_gpiods) { > + u8 cs; > + > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > + cs = spi_get_chipselect(spi, idx); > + if (cs != 0xFF) > + spi_set_csgpiod(spi, idx, ctlr->cs_gpiods[cs]); > + } > + } > > /* > * Drivers may modify this initial i/o setup, but will > @@ -701,6 +735,9 @@ int spi_add_device(struct spi_device *spi) > struct spi_controller *ctlr = spi->controller; > int status; > > + /* Set the bus ID string */ > + spi_dev_set_name(spi); > + > mutex_lock(&ctlr->add_lock); > status = __spi_add_device(spi); > mutex_unlock(&ctlr->add_lock); > @@ -942,32 +979,51 @@ static void spi_res_release(struct spi_controller *ctlr, struct spi_message *mes > } > > /*-------------------------------------------------------------------------*/ > +static inline bool spi_is_last_cs(struct spi_device *spi) > +{ > + u8 idx; > + bool last = false; > + > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > + if ((spi->cs_index_mask >> idx) & 0x01) { > + if (spi->controller->last_cs[idx] == spi_get_chipselect(spi, idx)) > + last = true; > + } > + } > + return last; > +} > + > > static void spi_set_cs(struct spi_device *spi, bool enable, bool force) > { > bool activate = enable; > + u8 idx; > > /* > * Avoid calling into the driver (or doing delays) if the chip select > * isn't actually changing from the last time this was called. > */ > - if (!force && ((enable && spi->controller->last_cs == spi_get_chipselect(spi, 0)) || > - (!enable && spi->controller->last_cs != spi_get_chipselect(spi, 0))) && > + if (!force && ((enable && spi->controller->last_cs_index_mask == spi->cs_index_mask && > + spi_is_last_cs(spi)) || > + (!enable && spi->controller->last_cs_index_mask == spi->cs_index_mask && > + !spi_is_last_cs(spi))) && > (spi->controller->last_cs_mode_high == (spi->mode & SPI_CS_HIGH))) > return; > > trace_spi_set_cs(spi, activate); > > - spi->controller->last_cs = enable ? spi_get_chipselect(spi, 0) : -1; > + spi->controller->last_cs_index_mask = spi->cs_index_mask; > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > + spi->controller->last_cs[idx] = enable ? spi_get_chipselect(spi, 0) : -1; > spi->controller->last_cs_mode_high = spi->mode & SPI_CS_HIGH; > > - if ((spi_get_csgpiod(spi, 0) || !spi->controller->set_cs_timing) && !activate) > - spi_delay_exec(&spi->cs_hold, NULL); > - > if (spi->mode & SPI_CS_HIGH) > enable = !enable; > > - if (spi_get_csgpiod(spi, 0)) { > + if (spi_is_csgpiod(spi)) { > + if (!spi->controller->set_cs_timing && !activate) > + spi_delay_exec(&spi->cs_hold, NULL); > + > if (!(spi->mode & SPI_NO_CS)) { > /* > * Historically ACPI has no means of the GPIO polarity and > @@ -979,26 +1035,38 @@ static void spi_set_cs(struct spi_device *spi, bool enable, bool force) > * ambiguity. That's why we use enable, that takes SPI_CS_HIGH > * into account. > */ > - if (has_acpi_companion(&spi->dev)) > - gpiod_set_value_cansleep(spi_get_csgpiod(spi, 0), !enable); > - else > - /* Polarity handled by GPIO library */ > - gpiod_set_value_cansleep(spi_get_csgpiod(spi, 0), activate); > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > + if (((spi->cs_index_mask >> idx) & 0x01) && > + spi_get_csgpiod(spi, idx)) { > + if (has_acpi_companion(&spi->dev)) > + gpiod_set_value_cansleep(spi_get_csgpiod(spi, idx), > + !enable); > + else > + /* Polarity handled by GPIO library */ > + gpiod_set_value_cansleep(spi_get_csgpiod(spi, idx), > + activate); > + > + if (activate) > + spi_delay_exec(&spi->cs_setup, NULL); > + else > + spi_delay_exec(&spi->cs_inactive, NULL); > + } > + } > } > /* Some SPI masters need both GPIO CS & slave_select */ > if ((spi->controller->flags & SPI_CONTROLLER_GPIO_SS) && > spi->controller->set_cs) > spi->controller->set_cs(spi, !enable); > + > + if (!spi->controller->set_cs_timing) { > + if (activate) > + spi_delay_exec(&spi->cs_setup, NULL); > + else > + spi_delay_exec(&spi->cs_inactive, NULL); > + } > } else if (spi->controller->set_cs) { > spi->controller->set_cs(spi, !enable); > } > - > - if (spi_get_csgpiod(spi, 0) || !spi->controller->set_cs_timing) { > - if (activate) > - spi_delay_exec(&spi->cs_setup, NULL); > - else > - spi_delay_exec(&spi->cs_inactive, NULL); > - } > } > > #ifdef CONFIG_HAS_DMA > @@ -2222,8 +2290,8 @@ static void of_spi_parse_dt_cs_delay(struct device_node *nc, > static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi, > struct device_node *nc) > { > - u32 value; > - int rc; > + u32 value, cs[SPI_CS_CNT_MAX]; > + int rc, idx; > > /* Mode (clock phase/polarity/etc.) */ > if (of_property_read_bool(nc, "spi-cpha")) > @@ -2295,14 +2363,52 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi, > return 0; > } > > + if (ctlr->num_chipselect > SPI_CS_CNT_MAX) { > + dev_err(&ctlr->dev, "No. of CS is more than max. no. of supported CS\n"); > + return -EINVAL; > + } > + > + /* > + * Zero(0) is a valid physical CS value and can be located at any > + * logical CS in the spi->chip_select[]. If all the physical CS > + * are initialized to 0 then It would be difficult to differentiate > + * between a valid physical CS 0 & an unused logical CS whose physical > + * CS can be 0. As a solution to this issue initialize all the CS to 0xFF. > + * Now all the unused logical CS will have 0xFF physical CS value & can be > + * ignore while performing physical CS validity checks. > + */ > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > + spi_set_chipselect(spi, idx, 0xFF); > + > /* Device address */ > - rc = of_property_read_u32(nc, "reg", &value); > - if (rc) { > + rc = of_property_read_variable_u32_array(nc, "reg", &cs[0], 1, > + SPI_CS_CNT_MAX); > + if (rc < 0) { > dev_err(&ctlr->dev, "%pOF has no valid 'reg' property (%d)\n", > nc, rc); > return rc; > } > - spi_set_chipselect(spi, 0, value); > + if (rc > ctlr->num_chipselect) { > + dev_err(&ctlr->dev, "%pOF has number of CS > ctlr->num_chipselect (%d)\n", > + nc, rc); > + return rc; > + } > + if ((of_property_read_bool(nc, "parallel-memories")) && > + (!(ctlr->flags & SPI_CONTROLLER_MULTI_CS))) { > + dev_err(&ctlr->dev, "SPI controller doesn't support multi CS\n"); > + return -EINVAL; > + } > + for (idx = 0; idx < rc; idx++) > + spi_set_chipselect(spi, idx, cs[idx]); > + > + /* spi->chip_select[i] gives the corresponding physical CS for logical CS i > + * logical CS number is represented by setting the ith bit in spi->cs_index_mask > + * So, for example, if spi->cs_index_mask = 0x01 then logical CS number is 0 and > + * spi->chip_select[0] will give the physical CS. > + * By default spi->chip_select[0] will hold the physical CS number so, set > + * spi->cs_index_mask as 0x01. > + */ > + spi->cs_index_mask = 0x01; > > /* Device speed */ > if (!of_property_read_u32(nc, "spi-max-frequency", &value)) > @@ -3100,6 +3206,7 @@ int spi_register_controller(struct spi_controller *ctlr) > struct boardinfo *bi; > int first_dynamic; > int status; > + int idx; > > if (!dev) > return -ENODEV; > @@ -3164,7 +3271,8 @@ int spi_register_controller(struct spi_controller *ctlr) > } > > /* Setting last_cs to -1 means no chip selected */ > - ctlr->last_cs = -1; > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > + ctlr->last_cs[idx] = -1; > > status = device_add(&ctlr->dev); > if (status < 0) > @@ -3889,7 +3997,7 @@ static int __spi_validate(struct spi_device *spi, struct spi_message *message) > * cs_change is set for each transfer. > */ > if ((spi->mode & SPI_CS_WORD) && (!(ctlr->mode_bits & SPI_CS_WORD) || > - spi_get_csgpiod(spi, 0))) { > + spi_is_csgpiod(spi))) { > size_t maxsize = BITS_TO_BYTES(spi->bits_per_word); > int ret; > > diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h > index 7b4baff63c5c..871d3a6d879b 100644 > --- a/include/linux/spi/spi.h > +++ b/include/linux/spi/spi.h > @@ -20,6 +20,9 @@ > > #include <uapi/linux/spi/spi.h> > > +/* Max no. of CS supported per spi device */ > +#define SPI_CS_CNT_MAX 4 > + > struct dma_chan; > struct software_node; > struct ptp_system_timestamp; > @@ -132,7 +135,8 @@ extern void spi_transfer_cs_change_delay_exec(struct spi_message *msg, > * @max_speed_hz: Maximum clock rate to be used with this chip > * (on this board); may be changed by the device's driver. > * The spi_transfer.speed_hz can override this for each transfer. > - * @chip_select: Chipselect, distinguishing chips handled by @controller. > + * @chip_select: Array of physical chipselect, spi->chipselect[i] gives > + * the corresponding physical CS for logical CS i. > * @mode: The spi mode defines how data is clocked out and in. > * This may be changed by the device's driver. > * The "active low" default for chipselect mode can be overridden > @@ -157,8 +161,8 @@ extern void spi_transfer_cs_change_delay_exec(struct spi_message *msg, > * the device will bind to the named driver and only the named driver. > * Do not set directly, because core frees it; use driver_set_override() to > * set or clear it. > - * @cs_gpiod: GPIO descriptor of the chipselect line (optional, NULL when > - * not using a GPIO line) > + * @cs_gpiod: Array of GPIO descriptors of the corresponding chipselect lines > + * (optional, NULL when not using a GPIO line) > * @word_delay: delay to be inserted between consecutive > * words of a transfer > * @cs_setup: delay to be introduced by the controller after CS is asserted > @@ -167,6 +171,7 @@ extern void spi_transfer_cs_change_delay_exec(struct spi_message *msg, > * deasserted. If @cs_change_delay is used from @spi_transfer, then the > * two delays will be added up. > * @pcpu_statistics: statistics for the spi_device > + * @cs_index_mask: Bit mask of the active chipselect(s) in the chipselect array > * > * A @spi_device is used to interchange data between an SPI slave > * (usually a discrete chip) and CPU memory. > @@ -182,7 +187,7 @@ struct spi_device { > struct spi_controller *controller; > struct spi_controller *master; /* Compatibility layer */ > u32 max_speed_hz; > - u8 chip_select; > + u8 chip_select[SPI_CS_CNT_MAX]; > u8 bits_per_word; > bool rt; > #define SPI_NO_TX BIT(31) /* No transmit wire */ > @@ -213,7 +218,7 @@ struct spi_device { > void *controller_data; > char modalias[SPI_NAME_SIZE]; > const char *driver_override; > - struct gpio_desc *cs_gpiod; /* Chip select GPIO descriptor */ > + struct gpio_desc *cs_gpiod[SPI_CS_CNT_MAX]; /* Chip select gpio desc */ > struct spi_delay word_delay; /* Inter-word delay */ > /* CS delays */ > struct spi_delay cs_setup; > @@ -223,6 +228,13 @@ struct spi_device { > /* The statistics */ > struct spi_statistics __percpu *pcpu_statistics; > > + /* Bit mask of the chipselect(s) that the driver need to use from > + * the chipselect array.When the controller is capable to handle > + * multiple chip selects & memories are connected in parallel > + * then more than one bit need to be set in cs_index_mask. > + */ > + u32 cs_index_mask : SPI_CS_CNT_MAX; > + > /* > * Likely need more hooks for more protocol options affecting how > * the controller talks to each chip, like: > @@ -279,22 +291,33 @@ static inline void *spi_get_drvdata(const struct spi_device *spi) > > static inline u8 spi_get_chipselect(const struct spi_device *spi, u8 idx) > { > - return spi->chip_select; > + return spi->chip_select[idx]; > } > > static inline void spi_set_chipselect(struct spi_device *spi, u8 idx, u8 chipselect) > { > - spi->chip_select = chipselect; > + spi->chip_select[idx] = chipselect; > } > > static inline struct gpio_desc *spi_get_csgpiod(const struct spi_device *spi, u8 idx) > { > - return spi->cs_gpiod; > + return spi->cs_gpiod[idx]; > } > > static inline void spi_set_csgpiod(struct spi_device *spi, u8 idx, struct gpio_desc *csgpiod) > { > - spi->cs_gpiod = csgpiod; > + spi->cs_gpiod[idx] = csgpiod; > +} > + > +static inline bool spi_is_csgpiod(struct spi_device *spi) > +{ > + u8 idx; > + > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > + if (spi_get_csgpiod(spi, idx)) > + return true; > + } > + return false; > } > > /** > @@ -399,6 +422,8 @@ extern struct spi_device *spi_new_ancillary_device(struct spi_device *spi, u8 ch > * @bus_lock_spinlock: spinlock for SPI bus locking > * @bus_lock_mutex: mutex for exclusion of multiple callers > * @bus_lock_flag: indicates that the SPI bus is locked for exclusive use > + * @multi_cs_cap: indicates that the SPI Controller can assert/de-assert > + * more than one chip select at once. > * @setup: updates the device mode and clocking records used by a > * device's SPI controller; protocol code may call this. This > * must fail if an unrecognized or unsupported mode is requested. > @@ -567,6 +592,11 @@ struct spi_controller { > #define SPI_CONTROLLER_MUST_TX BIT(4) /* Requires tx */ > #define SPI_CONTROLLER_GPIO_SS BIT(5) /* GPIO CS must select slave */ > #define SPI_CONTROLLER_SUSPENDED BIT(6) /* Currently suspended */ > + /* > + * The spi-controller has multi chip select capability and can > + * assert/de-assert more than one chip select at once. > + */ > +#define SPI_CONTROLLER_MULTI_CS BIT(7) > > /* Flag indicating if the allocation of this struct is devres-managed */ > bool devm_allocated; > @@ -677,7 +707,8 @@ struct spi_controller { > bool rt; > bool auto_runtime_pm; > bool cur_msg_mapped; > - char last_cs; > + char last_cs[SPI_CS_CNT_MAX]; > + char last_cs_index_mask; > bool last_cs_mode_high; > bool fallback; > struct completion xfer_completion;
Hello Mark, > -----Original Message----- > From: Mark Brown <broonie@kernel.org> > Sent: Monday, November 20, 2023 7:33 PM > To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com> > Cc: tudor.ambarus@linaro.org; pratyush@kernel.org; > miquel.raynal@bootlin.com; richard@nod.at; vigneshr@ti.com; > sbinding@opensource.cirrus.com; linux-spi@vger.kernel.org; linux- > kernel@vger.kernel.org; michael@walle.cc; linux-mtd@lists.infradead.org; > nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com; > claudiu.beznea@tuxon.dev; Simek, Michal <michal.simek@amd.com>; linux- > arm-kernel@lists.infradead.org; git (AMD-Xilinx) <git@amd.com>; > amitrkcian2002@gmail.com > Subject: Re: [PATCH v10 1/8] spi: Add multi-cs memories support in SPI core > > On Sat, Nov 18, 2023 at 07:24:39PM +0530, Amit Kumar Mahapatra wrote: > > AMD-Xilinx GQSPI controller has two advanced mode that allows the > > controller to consider two flashes as one single device. > > This breaks an x86 allmodconfig build: The cause of this failure are the following patches, which neglects to utilize spi_get_chipselect() for retrieving the CS value. Instead, it directly accesses spi->chip_select. https://lore.kernel.org/all/20230525150659.25409-14-rf@opensource.cirrus.com/ https://lore.kernel.org/all/20230216114410.183489-3-jpanis@baylibre.com/ In my upcoming series, I will address these issues. Regards, Amit > > /build/stage/linux/sound/pci/hda/cs35l56_hda_spi.c: In function > ‘cs35l56_hda_spi > _probe’: > /build/stage/linux/sound/pci/hda/cs35l56_hda_spi.c:32:52: error: passing > argumen t 2 of ‘cs35l56_hda_common_probe’ makes integer from pointer > without a cast [-We rror=int-conversion] > 32 | ret = cs35l56_hda_common_probe(cs35l56, spi->chip_select); > | ~~~^~~~~~~~~~~~~ > | | > | u8 * {aka unsigned ch > ar *} > In file included from /build/stage/linux/sound/pci/hda/cs35l56_hda_spi.c:12: > /build/stage/linux/sound/pci/hda/cs35l56_hda.h:45:63: note: expected ‘int’ > but a rgument is of type ‘u8 *’ {aka ‘unsigned char *’} > 45 | int cs35l56_hda_common_probe(struct cs35l56_hda *cs35l56, int id); > | ~~~~^~
Hello Stefan, > -----Original Message----- > From: Stefan Binding <sbinding@opensource.cirrus.com> > Sent: Monday, November 20, 2023 8:00 PM > To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; > broonie@kernel.org; tudor.ambarus@linaro.org; pratyush@kernel.org; > miquel.raynal@bootlin.com; richard@nod.at; vigneshr@ti.com > Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; > michael@walle.cc; linux-mtd@lists.infradead.org; > nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com; > claudiu.beznea@tuxon.dev; Simek, Michal <michal.simek@amd.com>; linux- > arm-kernel@lists.infradead.org; git (AMD-Xilinx) <git@amd.com>; > amitrkcian2002@gmail.com; patches@opensource.cirrus.com > Subject: Re: [PATCH v10 1/8] spi: Add multi-cs memories support in SPI core > > > On 18/11/2023 13:54, Amit Kumar Mahapatra wrote: > > AMD-Xilinx GQSPI controller has two advanced mode that allows the > > controller to consider two flashes as one single device. > > > > One of these two mode is the parallel mode in which each byte of data > > is stored in both devices, the even bits in the lower flash & the odd > > bits in the upper flash. The byte split is automatically handled by > > the QSPI controller. > > > > The other mode is the stacked mode in which both the flashes share the > > same SPI bus but each of the device contain half of the data. In this > > mode, the controller does not follow CS requests but instead > > internally wires the two CS levels with the value of the most significant > address bit. > > > > For supporting both these modes SPI core need to be updated for > > providing multiple CS for a single SPI device. > > > > For adding multi CS support the SPI device need to be aware of all the > > CS values. So, the "chip_select" member in the spi_device structure is > > now an array that holds all the CS values. > > > > spi_device structure now has a "cs_index_mask" member. This acts as an > > index to the chip_select array. If nth bit of spi->cs_index_mask is > > set then the driver would assert spi->chip_select[n]. > > > > In parallel mode all the chip selects are asserted/de-asserted > > simultaneously and each byte of data is stored in both devices, the > > even bits in one, the odd bits in the other. The split is > > automatically handled by the GQSPI controller. The GQSPI controller > > supports a maximum of two flashes connected in parallel mode. A > > SPI_CONTROLLER_MULTI_CS flag bit is added in the spi controller flags, > > through ctlr->flags the spi core will make sure that the controller is > > capable of handling multiple chip selects at once. > > > > For supporting multiple CS via GPIO the cs_gpiod member of the > > spi_device structure is now an array that holds the gpio descriptor > > for each chipselect. > > > > CS GPIO is not tested due to unavailability of necessary hardware setup. > > > > Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com> > > --- > > Hello @Stefen, > > We restructured the SPI core implementation, for handling arbitrary > > number of devices connected in parallel(multi-cs) or stacked mode. We > > have tested this updated patch on native-cs setup but couldn't test > > cs-gpio due to unavailability of a proper setup. If possible, could > > you please retest the cs-gpio use case with this updated patch and provide > your feedback. > > Hi, > > I tested this chain on 2 different systems using GPIOs as chip selects, and see > the same error on both: > [ 2.842045] pxa2xx-spi pxa2xx-spi.6: chipselect 0 already in use > > Let me know if you need any further testing. > > Thanks, > > Stefan Binding > > > --- > > drivers/spi/spi.c | 192 +++++++++++++++++++++++++++++++--------- > > include/linux/spi/spi.h | 51 ++++++++--- > > 2 files changed, 191 insertions(+), 52 deletions(-) > > > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index > > 8ead7acb99f3..ff66147ba95f 100644 > > --- a/drivers/spi/spi.c > > +++ b/drivers/spi/spi.c > > @@ -612,10 +612,21 @@ static int spi_dev_check(struct device *dev, void > *data) > > { > > struct spi_device *spi = to_spi_device(dev); > > struct spi_device *new_spi = data; > > - > > - if (spi->controller == new_spi->controller && > > - spi_get_chipselect(spi, 0) == spi_get_chipselect(new_spi, 0)) > > - return -EBUSY; > > + int idx, nw_idx; > > + u8 cs, cs_nw; > > + > > + if (spi->controller == new_spi->controller) { > > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > > + cs = spi_get_chipselect(spi, idx); > > + for (nw_idx = 0; nw_idx < SPI_CS_CNT_MAX; > nw_idx++) { > > + cs_nw = spi_get_chipselect(spi, nw_idx); Thank you for dedicating your time to test my patch. As per my analysis the error is reported from here as we are supplying the former SPI device structure to retrieve the CS value for the new SPI devices. To fix this, could you kindly substitute the above line with cs_nw = spi_get_chipselect(new_spi, nw_idx); and rerun your tests? If it works correctly, I will incorporate this fix into my upcoming series. Regards, Amit > > + if (cs != 0xFF && cs_nw != 0xFF && cs == > cs_nw) { > > + dev_err(dev, "chipselect %d already in > use\n", cs_nw); > > + return -EBUSY; > > + } > > + } > > + } > > + } > > return 0; > > } > > > > @@ -629,13 +640,32 @@ static int __spi_add_device(struct spi_device *spi) > > { > > struct spi_controller *ctlr = spi->controller; > > struct device *dev = ctlr->dev.parent; > > - int status; > > + int status, idx, nw_idx; > > + u8 cs, nw_cs; > > + > > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > > + /* Chipselects are numbered 0..max; validate. */ > > + cs = spi_get_chipselect(spi, idx); > > + if (cs != 0xFF && cs >= ctlr->num_chipselect) { > > + dev_err(dev, "cs%d >= max %d\n", > spi_get_chipselect(spi, idx), > > + ctlr->num_chipselect); > > + return -EINVAL; > > + } > > + } > > > > - /* Chipselects are numbered 0..max; validate. */ > > - if (spi_get_chipselect(spi, 0) >= ctlr->num_chipselect) { > > - dev_err(dev, "cs%d >= max %d\n", spi_get_chipselect(spi, 0), > > - ctlr->num_chipselect); > > - return -EINVAL; > > + /* > > + * Make sure that multiple logical CS doesn't map to the same > physical CS. > > + * For example, spi->chip_select[0] != spi->chip_select[1] and so on. > > + */ > > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > > + cs = spi_get_chipselect(spi, idx); > > + for (nw_idx = idx + 1; nw_idx < SPI_CS_CNT_MAX; nw_idx++) { > > + nw_cs = spi_get_chipselect(spi, nw_idx); > > + if (cs != 0xFF && nw_cs != 0xFF && cs == nw_cs) { > > + dev_err(dev, "chipselect %d already in use\n", > nw_cs); > > + return -EBUSY; > > + } > > + } > > } > > > > /* Set the bus ID string */ > > @@ -647,11 +677,8 @@ static int __spi_add_device(struct spi_device *spi) > > * its configuration. > > */ > > status = bus_for_each_dev(&spi_bus_type, NULL, spi, spi_dev_check); > > - if (status) { > > - dev_err(dev, "chipselect %d already in use\n", > > - spi_get_chipselect(spi, 0)); > > + if (status) > > return status; > > - } > > > > /* Controller may unregister concurrently */ > > if (IS_ENABLED(CONFIG_SPI_DYNAMIC) && @@ -659,8 +686,15 @@ > static > > int __spi_add_device(struct spi_device *spi) > > return -ENODEV; > > } > > > > - if (ctlr->cs_gpiods) > > - spi_set_csgpiod(spi, 0, ctlr->cs_gpiods[spi_get_chipselect(spi, > 0)]); > > + if (ctlr->cs_gpiods) { > > + u8 cs; > > + > > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > > + cs = spi_get_chipselect(spi, idx); > > + if (cs != 0xFF) > > + spi_set_csgpiod(spi, idx, ctlr->cs_gpiods[cs]); > > + } > > + } > > > > /* > > * Drivers may modify this initial i/o setup, but will @@ -701,6 > > +735,9 @@ int spi_add_device(struct spi_device *spi) > > struct spi_controller *ctlr = spi->controller; > > int status; > > > > + /* Set the bus ID string */ > > + spi_dev_set_name(spi); > > + > > mutex_lock(&ctlr->add_lock); > > status = __spi_add_device(spi); > > mutex_unlock(&ctlr->add_lock); > > @@ -942,32 +979,51 @@ static void spi_res_release(struct spi_controller > *ctlr, struct spi_message *mes > > } > > > > > > /*-------------------------------------------------------------------- > > -----*/ > > +static inline bool spi_is_last_cs(struct spi_device *spi) { > > + u8 idx; > > + bool last = false; > > + > > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > > + if ((spi->cs_index_mask >> idx) & 0x01) { > > + if (spi->controller->last_cs[idx] == > spi_get_chipselect(spi, idx)) > > + last = true; > > + } > > + } > > + return last; > > +} > > + > > > > static void spi_set_cs(struct spi_device *spi, bool enable, bool force) > > { > > bool activate = enable; > > + u8 idx; > > > > /* > > * Avoid calling into the driver (or doing delays) if the chip select > > * isn't actually changing from the last time this was called. > > */ > > - if (!force && ((enable && spi->controller->last_cs == > spi_get_chipselect(spi, 0)) || > > - (!enable && spi->controller->last_cs != > spi_get_chipselect(spi, 0))) && > > + if (!force && ((enable && spi->controller->last_cs_index_mask == spi- > >cs_index_mask && > > + spi_is_last_cs(spi)) || > > + (!enable && spi->controller->last_cs_index_mask == spi- > >cs_index_mask && > > + !spi_is_last_cs(spi))) && > > (spi->controller->last_cs_mode_high == (spi->mode & > SPI_CS_HIGH))) > > return; > > > > trace_spi_set_cs(spi, activate); > > > > - spi->controller->last_cs = enable ? spi_get_chipselect(spi, 0) : -1; > > + spi->controller->last_cs_index_mask = spi->cs_index_mask; > > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > > + spi->controller->last_cs[idx] = enable ? spi_get_chipselect(spi, > 0) > > +: -1; > > spi->controller->last_cs_mode_high = spi->mode & SPI_CS_HIGH; > > > > - if ((spi_get_csgpiod(spi, 0) || !spi->controller->set_cs_timing) && > !activate) > > - spi_delay_exec(&spi->cs_hold, NULL); > > - > > if (spi->mode & SPI_CS_HIGH) > > enable = !enable; > > > > - if (spi_get_csgpiod(spi, 0)) { > > + if (spi_is_csgpiod(spi)) { > > + if (!spi->controller->set_cs_timing && !activate) > > + spi_delay_exec(&spi->cs_hold, NULL); > > + > > if (!(spi->mode & SPI_NO_CS)) { > > /* > > * Historically ACPI has no means of the GPIO polarity > and @@ > > -979,26 +1035,38 @@ static void spi_set_cs(struct spi_device *spi, bool > enable, bool force) > > * ambiguity. That's why we use enable, that takes > SPI_CS_HIGH > > * into account. > > */ > > - if (has_acpi_companion(&spi->dev)) > > - > gpiod_set_value_cansleep(spi_get_csgpiod(spi, 0), !enable); > > - else > > - /* Polarity handled by GPIO library */ > > - > gpiod_set_value_cansleep(spi_get_csgpiod(spi, 0), activate); > > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > > + if (((spi->cs_index_mask >> idx) & 0x01) && > > + spi_get_csgpiod(spi, idx)) { > > + if (has_acpi_companion(&spi->dev)) > > + > gpiod_set_value_cansleep(spi_get_csgpiod(spi, idx), > > + > !enable); > > + else > > + /* Polarity handled by GPIO > library */ > > + > gpiod_set_value_cansleep(spi_get_csgpiod(spi, idx), > > + > activate); > > + > > + if (activate) > > + spi_delay_exec(&spi- > >cs_setup, NULL); > > + else > > + spi_delay_exec(&spi- > >cs_inactive, NULL); > > + } > > + } > > } > > /* Some SPI masters need both GPIO CS & slave_select */ > > if ((spi->controller->flags & SPI_CONTROLLER_GPIO_SS) && > > spi->controller->set_cs) > > spi->controller->set_cs(spi, !enable); > > + > > + if (!spi->controller->set_cs_timing) { > > + if (activate) > > + spi_delay_exec(&spi->cs_setup, NULL); > > + else > > + spi_delay_exec(&spi->cs_inactive, NULL); > > + } > > } else if (spi->controller->set_cs) { > > spi->controller->set_cs(spi, !enable); > > } > > - > > - if (spi_get_csgpiod(spi, 0) || !spi->controller->set_cs_timing) { > > - if (activate) > > - spi_delay_exec(&spi->cs_setup, NULL); > > - else > > - spi_delay_exec(&spi->cs_inactive, NULL); > > - } > > } > > > > #ifdef CONFIG_HAS_DMA > > @@ -2222,8 +2290,8 @@ static void of_spi_parse_dt_cs_delay(struct > device_node *nc, > > static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi, > > struct device_node *nc) > > { > > - u32 value; > > - int rc; > > + u32 value, cs[SPI_CS_CNT_MAX]; > > + int rc, idx; > > > > /* Mode (clock phase/polarity/etc.) */ > > if (of_property_read_bool(nc, "spi-cpha")) @@ -2295,14 +2363,52 > @@ > > static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi, > > return 0; > > } > > > > + if (ctlr->num_chipselect > SPI_CS_CNT_MAX) { > > + dev_err(&ctlr->dev, "No. of CS is more than max. no. of > supported CS\n"); > > + return -EINVAL; > > + } > > + > > + /* > > + * Zero(0) is a valid physical CS value and can be located at any > > + * logical CS in the spi->chip_select[]. If all the physical CS > > + * are initialized to 0 then It would be difficult to differentiate > > + * between a valid physical CS 0 & an unused logical CS whose > physical > > + * CS can be 0. As a solution to this issue initialize all the CS to 0xFF. > > + * Now all the unused logical CS will have 0xFF physical CS value & can > be > > + * ignore while performing physical CS validity checks. > > + */ > > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > > + spi_set_chipselect(spi, idx, 0xFF); > > + > > /* Device address */ > > - rc = of_property_read_u32(nc, "reg", &value); > > - if (rc) { > > + rc = of_property_read_variable_u32_array(nc, "reg", &cs[0], 1, > > + SPI_CS_CNT_MAX); > > + if (rc < 0) { > > dev_err(&ctlr->dev, "%pOF has no valid 'reg' property > (%d)\n", > > nc, rc); > > return rc; > > } > > - spi_set_chipselect(spi, 0, value); > > + if (rc > ctlr->num_chipselect) { > > + dev_err(&ctlr->dev, "%pOF has number of CS > ctlr- > >num_chipselect (%d)\n", > > + nc, rc); > > + return rc; > > + } > > + if ((of_property_read_bool(nc, "parallel-memories")) && > > + (!(ctlr->flags & SPI_CONTROLLER_MULTI_CS))) { > > + dev_err(&ctlr->dev, "SPI controller doesn't support multi > CS\n"); > > + return -EINVAL; > > + } > > + for (idx = 0; idx < rc; idx++) > > + spi_set_chipselect(spi, idx, cs[idx]); > > + > > + /* spi->chip_select[i] gives the corresponding physical CS for logical CS > i > > + * logical CS number is represented by setting the ith bit in spi- > >cs_index_mask > > + * So, for example, if spi->cs_index_mask = 0x01 then logical CS > number is 0 and > > + * spi->chip_select[0] will give the physical CS. > > + * By default spi->chip_select[0] will hold the physical CS number so, > set > > + * spi->cs_index_mask as 0x01. > > + */ > > + spi->cs_index_mask = 0x01; > > > > /* Device speed */ > > if (!of_property_read_u32(nc, "spi-max-frequency", &value)) @@ > > -3100,6 +3206,7 @@ int spi_register_controller(struct spi_controller *ctlr) > > struct boardinfo *bi; > > int first_dynamic; > > int status; > > + int idx; > > > > if (!dev) > > return -ENODEV; > > @@ -3164,7 +3271,8 @@ int spi_register_controller(struct spi_controller > *ctlr) > > } > > > > /* Setting last_cs to -1 means no chip selected */ > > - ctlr->last_cs = -1; > > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > > + ctlr->last_cs[idx] = -1; > > > > status = device_add(&ctlr->dev); > > if (status < 0) > > @@ -3889,7 +3997,7 @@ static int __spi_validate(struct spi_device *spi, > struct spi_message *message) > > * cs_change is set for each transfer. > > */ > > if ((spi->mode & SPI_CS_WORD) && (!(ctlr->mode_bits & > SPI_CS_WORD) || > > - spi_get_csgpiod(spi, 0))) { > > + spi_is_csgpiod(spi))) { > > size_t maxsize = BITS_TO_BYTES(spi->bits_per_word); > > int ret; > > > > diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index > > 7b4baff63c5c..871d3a6d879b 100644 > > --- a/include/linux/spi/spi.h > > +++ b/include/linux/spi/spi.h > > @@ -20,6 +20,9 @@ > > > > #include <uapi/linux/spi/spi.h> > > > > +/* Max no. of CS supported per spi device */ #define SPI_CS_CNT_MAX 4 > > + > > struct dma_chan; > > struct software_node; > > struct ptp_system_timestamp; > > @@ -132,7 +135,8 @@ extern void > spi_transfer_cs_change_delay_exec(struct spi_message *msg, > > * @max_speed_hz: Maximum clock rate to be used with this chip > > * (on this board); may be changed by the device's driver. > > * The spi_transfer.speed_hz can override this for each transfer. > > - * @chip_select: Chipselect, distinguishing chips handled by @controller. > > + * @chip_select: Array of physical chipselect, spi->chipselect[i] gives > > + * the corresponding physical CS for logical CS i. > > * @mode: The spi mode defines how data is clocked out and in. > > * This may be changed by the device's driver. > > * The "active low" default for chipselect mode can be overridden > > @@ -157,8 +161,8 @@ extern void > spi_transfer_cs_change_delay_exec(struct spi_message *msg, > > * the device will bind to the named driver and only the named driver. > > * Do not set directly, because core frees it; use driver_set_override() to > > * set or clear it. > > - * @cs_gpiod: GPIO descriptor of the chipselect line (optional, NULL when > > - * not using a GPIO line) > > + * @cs_gpiod: Array of GPIO descriptors of the corresponding chipselect > lines > > + * (optional, NULL when not using a GPIO line) > > * @word_delay: delay to be inserted between consecutive > > * words of a transfer > > * @cs_setup: delay to be introduced by the controller after CS is > > asserted @@ -167,6 +171,7 @@ extern void > spi_transfer_cs_change_delay_exec(struct spi_message *msg, > > * deasserted. If @cs_change_delay is used from @spi_transfer, then > the > > * two delays will be added up. > > * @pcpu_statistics: statistics for the spi_device > > + * @cs_index_mask: Bit mask of the active chipselect(s) in the > > + chipselect array > > * > > * A @spi_device is used to interchange data between an SPI slave > > * (usually a discrete chip) and CPU memory. > > @@ -182,7 +187,7 @@ struct spi_device { > > struct spi_controller *controller; > > struct spi_controller *master; /* Compatibility layer */ > > u32 max_speed_hz; > > - u8 chip_select; > > + u8 chip_select[SPI_CS_CNT_MAX]; > > u8 bits_per_word; > > bool rt; > > #define SPI_NO_TX BIT(31) /* No transmit wire */ > > @@ -213,7 +218,7 @@ struct spi_device { > > void *controller_data; > > char modalias[SPI_NAME_SIZE]; > > const char *driver_override; > > - struct gpio_desc *cs_gpiod; /* Chip select GPIO descriptor > */ > > + struct gpio_desc *cs_gpiod[SPI_CS_CNT_MAX]; /* Chip select > gpio desc */ > > struct spi_delay word_delay; /* Inter-word delay */ > > /* CS delays */ > > struct spi_delay cs_setup; > > @@ -223,6 +228,13 @@ struct spi_device { > > /* The statistics */ > > struct spi_statistics __percpu *pcpu_statistics; > > > > + /* Bit mask of the chipselect(s) that the driver need to use from > > + * the chipselect array.When the controller is capable to handle > > + * multiple chip selects & memories are connected in parallel > > + * then more than one bit need to be set in cs_index_mask. > > + */ > > + u32 cs_index_mask : SPI_CS_CNT_MAX; > > + > > /* > > * Likely need more hooks for more protocol options affecting how > > * the controller talks to each chip, like: > > @@ -279,22 +291,33 @@ static inline void *spi_get_drvdata(const struct > > spi_device *spi) > > > > static inline u8 spi_get_chipselect(const struct spi_device *spi, u8 idx) > > { > > - return spi->chip_select; > > + return spi->chip_select[idx]; > > } > > > > static inline void spi_set_chipselect(struct spi_device *spi, u8 idx, u8 > chipselect) > > { > > - spi->chip_select = chipselect; > > + spi->chip_select[idx] = chipselect; > > } > > > > static inline struct gpio_desc *spi_get_csgpiod(const struct spi_device *spi, > u8 idx) > > { > > - return spi->cs_gpiod; > > + return spi->cs_gpiod[idx]; > > } > > > > static inline void spi_set_csgpiod(struct spi_device *spi, u8 idx, struct > gpio_desc *csgpiod) > > { > > - spi->cs_gpiod = csgpiod; > > + spi->cs_gpiod[idx] = csgpiod; > > +} > > + > > +static inline bool spi_is_csgpiod(struct spi_device *spi) { > > + u8 idx; > > + > > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > > + if (spi_get_csgpiod(spi, idx)) > > + return true; > > + } > > + return false; > > } > > > > /** > > @@ -399,6 +422,8 @@ extern struct spi_device > *spi_new_ancillary_device(struct spi_device *spi, u8 ch > > * @bus_lock_spinlock: spinlock for SPI bus locking > > * @bus_lock_mutex: mutex for exclusion of multiple callers > > * @bus_lock_flag: indicates that the SPI bus is locked for > > exclusive use > > + * @multi_cs_cap: indicates that the SPI Controller can assert/de-assert > > + * more than one chip select at once. > > * @setup: updates the device mode and clocking records used by a > > * device's SPI controller; protocol code may call this. This > > * must fail if an unrecognized or unsupported mode is requested. > > @@ -567,6 +592,11 @@ struct spi_controller { > > #define SPI_CONTROLLER_MUST_TX BIT(4) /* Requires tx */ > > #define SPI_CONTROLLER_GPIO_SS BIT(5) /* GPIO CS must > select slave */ > > #define SPI_CONTROLLER_SUSPENDED BIT(6) /* Currently > suspended */ > > + /* > > + * The spi-controller has multi chip select capability and can > > + * assert/de-assert more than one chip select at once. > > + */ > > +#define SPI_CONTROLLER_MULTI_CS BIT(7) > > > > /* Flag indicating if the allocation of this struct is devres-managed */ > > bool devm_allocated; > > @@ -677,7 +707,8 @@ struct spi_controller { > > bool rt; > > bool auto_runtime_pm; > > bool cur_msg_mapped; > > - char last_cs; > > + char last_cs[SPI_CS_CNT_MAX]; > > + char last_cs_index_mask; > > bool last_cs_mode_high; > > bool fallback; > > struct completion xfer_completion;
On 21/11/2023 07:39, Mahapatra, Amit Kumar wrote: > Hello Stefan, > >> -----Original Message----- >> From: Stefan Binding <sbinding@opensource.cirrus.com> >> Sent: Monday, November 20, 2023 8:00 PM >> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; >> broonie@kernel.org; tudor.ambarus@linaro.org; pratyush@kernel.org; >> miquel.raynal@bootlin.com; richard@nod.at; vigneshr@ti.com >> Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; >> michael@walle.cc; linux-mtd@lists.infradead.org; >> nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com; >> claudiu.beznea@tuxon.dev; Simek, Michal <michal.simek@amd.com>; linux- >> arm-kernel@lists.infradead.org; git (AMD-Xilinx) <git@amd.com>; >> amitrkcian2002@gmail.com; patches@opensource.cirrus.com >> Subject: Re: [PATCH v10 1/8] spi: Add multi-cs memories support in SPI core >> >> >> On 18/11/2023 13:54, Amit Kumar Mahapatra wrote: >>> AMD-Xilinx GQSPI controller has two advanced mode that allows the >>> controller to consider two flashes as one single device. >>> >>> One of these two mode is the parallel mode in which each byte of data >>> is stored in both devices, the even bits in the lower flash & the odd >>> bits in the upper flash. The byte split is automatically handled by >>> the QSPI controller. >>> >>> The other mode is the stacked mode in which both the flashes share the >>> same SPI bus but each of the device contain half of the data. In this >>> mode, the controller does not follow CS requests but instead >>> internally wires the two CS levels with the value of the most significant >> address bit. >>> For supporting both these modes SPI core need to be updated for >>> providing multiple CS for a single SPI device. >>> >>> For adding multi CS support the SPI device need to be aware of all the >>> CS values. So, the "chip_select" member in the spi_device structure is >>> now an array that holds all the CS values. >>> >>> spi_device structure now has a "cs_index_mask" member. This acts as an >>> index to the chip_select array. If nth bit of spi->cs_index_mask is >>> set then the driver would assert spi->chip_select[n]. >>> >>> In parallel mode all the chip selects are asserted/de-asserted >>> simultaneously and each byte of data is stored in both devices, the >>> even bits in one, the odd bits in the other. The split is >>> automatically handled by the GQSPI controller. The GQSPI controller >>> supports a maximum of two flashes connected in parallel mode. A >>> SPI_CONTROLLER_MULTI_CS flag bit is added in the spi controller flags, >>> through ctlr->flags the spi core will make sure that the controller is >>> capable of handling multiple chip selects at once. >>> >>> For supporting multiple CS via GPIO the cs_gpiod member of the >>> spi_device structure is now an array that holds the gpio descriptor >>> for each chipselect. >>> >>> CS GPIO is not tested due to unavailability of necessary hardware setup. >>> >>> Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com> >>> --- >>> Hello @Stefen, >>> We restructured the SPI core implementation, for handling arbitrary >>> number of devices connected in parallel(multi-cs) or stacked mode. We >>> have tested this updated patch on native-cs setup but couldn't test >>> cs-gpio due to unavailability of a proper setup. If possible, could >>> you please retest the cs-gpio use case with this updated patch and provide >> your feedback. >> >> Hi, >> >> I tested this chain on 2 different systems using GPIOs as chip selects, and see >> the same error on both: >> [ 2.842045] pxa2xx-spi pxa2xx-spi.6: chipselect 0 already in use >> >> Let me know if you need any further testing. >> >> Thanks, >> >> Stefan Binding >> >>> --- >>> drivers/spi/spi.c | 192 +++++++++++++++++++++++++++++++--------- >>> include/linux/spi/spi.h | 51 ++++++++--- >>> 2 files changed, 191 insertions(+), 52 deletions(-) >>> >>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index >>> 8ead7acb99f3..ff66147ba95f 100644 >>> --- a/drivers/spi/spi.c >>> +++ b/drivers/spi/spi.c >>> @@ -612,10 +612,21 @@ static int spi_dev_check(struct device *dev, void >> *data) >>> { >>> struct spi_device *spi = to_spi_device(dev); >>> struct spi_device *new_spi = data; >>> - >>> - if (spi->controller == new_spi->controller && >>> - spi_get_chipselect(spi, 0) == spi_get_chipselect(new_spi, 0)) >>> - return -EBUSY; >>> + int idx, nw_idx; >>> + u8 cs, cs_nw; >>> + >>> + if (spi->controller == new_spi->controller) { >>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { >>> + cs = spi_get_chipselect(spi, idx); >>> + for (nw_idx = 0; nw_idx < SPI_CS_CNT_MAX; >> nw_idx++) { >>> + cs_nw = spi_get_chipselect(spi, nw_idx); > Thank you for dedicating your time to test my patch. > As per my analysis the error is reported from here as we are > supplying the former SPI device structure to retrieve the CS > value for the new SPI devices. > To fix this, could you kindly substitute the above line with > > cs_nw = spi_get_chipselect(new_spi, nw_idx); > > and rerun your tests? > If it works correctly, I will incorporate this fix into my > upcoming series. > > Regards, > Amit Hi, I still see the same error: [ 2.748546] pxa2xx-spi pxa2xx-spi.6: chipselect 0 already in use Thanks, Stefan > >>> + if (cs != 0xFF && cs_nw != 0xFF && cs == >> cs_nw) { >>> + dev_err(dev, "chipselect %d already in >> use\n", cs_nw); >>> + return -EBUSY; >>> + } >>> + } >>> + } >>> + } >>> return 0; >>> } >>> >>> @@ -629,13 +640,32 @@ static int __spi_add_device(struct spi_device *spi) >>> { >>> struct spi_controller *ctlr = spi->controller; >>> struct device *dev = ctlr->dev.parent; >>> - int status; >>> + int status, idx, nw_idx; >>> + u8 cs, nw_cs; >>> + >>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { >>> + /* Chipselects are numbered 0..max; validate. */ >>> + cs = spi_get_chipselect(spi, idx); >>> + if (cs != 0xFF && cs >= ctlr->num_chipselect) { >>> + dev_err(dev, "cs%d >= max %d\n", >> spi_get_chipselect(spi, idx), >>> + ctlr->num_chipselect); >>> + return -EINVAL; >>> + } >>> + } >>> >>> - /* Chipselects are numbered 0..max; validate. */ >>> - if (spi_get_chipselect(spi, 0) >= ctlr->num_chipselect) { >>> - dev_err(dev, "cs%d >= max %d\n", spi_get_chipselect(spi, 0), >>> - ctlr->num_chipselect); >>> - return -EINVAL; >>> + /* >>> + * Make sure that multiple logical CS doesn't map to the same >> physical CS. >>> + * For example, spi->chip_select[0] != spi->chip_select[1] and so on. >>> + */ >>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { >>> + cs = spi_get_chipselect(spi, idx); >>> + for (nw_idx = idx + 1; nw_idx < SPI_CS_CNT_MAX; nw_idx++) { >>> + nw_cs = spi_get_chipselect(spi, nw_idx); >>> + if (cs != 0xFF && nw_cs != 0xFF && cs == nw_cs) { >>> + dev_err(dev, "chipselect %d already in use\n", >> nw_cs); >>> + return -EBUSY; >>> + } >>> + } >>> } >>> >>> /* Set the bus ID string */ >>> @@ -647,11 +677,8 @@ static int __spi_add_device(struct spi_device *spi) >>> * its configuration. >>> */ >>> status = bus_for_each_dev(&spi_bus_type, NULL, spi, spi_dev_check); >>> - if (status) { >>> - dev_err(dev, "chipselect %d already in use\n", >>> - spi_get_chipselect(spi, 0)); >>> + if (status) >>> return status; >>> - } >>> >>> /* Controller may unregister concurrently */ >>> if (IS_ENABLED(CONFIG_SPI_DYNAMIC) && @@ -659,8 +686,15 @@ >> static >>> int __spi_add_device(struct spi_device *spi) >>> return -ENODEV; >>> } >>> >>> - if (ctlr->cs_gpiods) >>> - spi_set_csgpiod(spi, 0, ctlr->cs_gpiods[spi_get_chipselect(spi, >> 0)]); >>> + if (ctlr->cs_gpiods) { >>> + u8 cs; >>> + >>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { >>> + cs = spi_get_chipselect(spi, idx); >>> + if (cs != 0xFF) >>> + spi_set_csgpiod(spi, idx, ctlr->cs_gpiods[cs]); >>> + } >>> + } >>> >>> /* >>> * Drivers may modify this initial i/o setup, but will @@ -701,6 >>> +735,9 @@ int spi_add_device(struct spi_device *spi) >>> struct spi_controller *ctlr = spi->controller; >>> int status; >>> >>> + /* Set the bus ID string */ >>> + spi_dev_set_name(spi); >>> + >>> mutex_lock(&ctlr->add_lock); >>> status = __spi_add_device(spi); >>> mutex_unlock(&ctlr->add_lock); >>> @@ -942,32 +979,51 @@ static void spi_res_release(struct spi_controller >> *ctlr, struct spi_message *mes >>> } >>> >>> >>> /*-------------------------------------------------------------------- >>> -----*/ >>> +static inline bool spi_is_last_cs(struct spi_device *spi) { >>> + u8 idx; >>> + bool last = false; >>> + >>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { >>> + if ((spi->cs_index_mask >> idx) & 0x01) { >>> + if (spi->controller->last_cs[idx] == >> spi_get_chipselect(spi, idx)) >>> + last = true; >>> + } >>> + } >>> + return last; >>> +} >>> + >>> >>> static void spi_set_cs(struct spi_device *spi, bool enable, bool force) >>> { >>> bool activate = enable; >>> + u8 idx; >>> >>> /* >>> * Avoid calling into the driver (or doing delays) if the chip select >>> * isn't actually changing from the last time this was called. >>> */ >>> - if (!force && ((enable && spi->controller->last_cs == >> spi_get_chipselect(spi, 0)) || >>> - (!enable && spi->controller->last_cs != >> spi_get_chipselect(spi, 0))) && >>> + if (!force && ((enable && spi->controller->last_cs_index_mask == spi- >>> cs_index_mask && >>> + spi_is_last_cs(spi)) || >>> + (!enable && spi->controller->last_cs_index_mask == spi- >>> cs_index_mask && >>> + !spi_is_last_cs(spi))) && >>> (spi->controller->last_cs_mode_high == (spi->mode & >> SPI_CS_HIGH))) >>> return; >>> >>> trace_spi_set_cs(spi, activate); >>> >>> - spi->controller->last_cs = enable ? spi_get_chipselect(spi, 0) : -1; >>> + spi->controller->last_cs_index_mask = spi->cs_index_mask; >>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) >>> + spi->controller->last_cs[idx] = enable ? spi_get_chipselect(spi, >> 0) >>> +: -1; >>> spi->controller->last_cs_mode_high = spi->mode & SPI_CS_HIGH; >>> >>> - if ((spi_get_csgpiod(spi, 0) || !spi->controller->set_cs_timing) && >> !activate) >>> - spi_delay_exec(&spi->cs_hold, NULL); >>> - >>> if (spi->mode & SPI_CS_HIGH) >>> enable = !enable; >>> >>> - if (spi_get_csgpiod(spi, 0)) { >>> + if (spi_is_csgpiod(spi)) { >>> + if (!spi->controller->set_cs_timing && !activate) >>> + spi_delay_exec(&spi->cs_hold, NULL); >>> + >>> if (!(spi->mode & SPI_NO_CS)) { >>> /* >>> * Historically ACPI has no means of the GPIO polarity >> and @@ >>> -979,26 +1035,38 @@ static void spi_set_cs(struct spi_device *spi, bool >> enable, bool force) >>> * ambiguity. That's why we use enable, that takes >> SPI_CS_HIGH >>> * into account. >>> */ >>> - if (has_acpi_companion(&spi->dev)) >>> - >> gpiod_set_value_cansleep(spi_get_csgpiod(spi, 0), !enable); >>> - else >>> - /* Polarity handled by GPIO library */ >>> - >> gpiod_set_value_cansleep(spi_get_csgpiod(spi, 0), activate); >>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { >>> + if (((spi->cs_index_mask >> idx) & 0x01) && >>> + spi_get_csgpiod(spi, idx)) { >>> + if (has_acpi_companion(&spi->dev)) >>> + >> gpiod_set_value_cansleep(spi_get_csgpiod(spi, idx), >>> + >> !enable); >>> + else >>> + /* Polarity handled by GPIO >> library */ >>> + >> gpiod_set_value_cansleep(spi_get_csgpiod(spi, idx), >>> + >> activate); >>> + >>> + if (activate) >>> + spi_delay_exec(&spi- >>> cs_setup, NULL); >>> + else >>> + spi_delay_exec(&spi- >>> cs_inactive, NULL); >>> + } >>> + } >>> } >>> /* Some SPI masters need both GPIO CS & slave_select */ >>> if ((spi->controller->flags & SPI_CONTROLLER_GPIO_SS) && >>> spi->controller->set_cs) >>> spi->controller->set_cs(spi, !enable); >>> + >>> + if (!spi->controller->set_cs_timing) { >>> + if (activate) >>> + spi_delay_exec(&spi->cs_setup, NULL); >>> + else >>> + spi_delay_exec(&spi->cs_inactive, NULL); >>> + } >>> } else if (spi->controller->set_cs) { >>> spi->controller->set_cs(spi, !enable); >>> } >>> - >>> - if (spi_get_csgpiod(spi, 0) || !spi->controller->set_cs_timing) { >>> - if (activate) >>> - spi_delay_exec(&spi->cs_setup, NULL); >>> - else >>> - spi_delay_exec(&spi->cs_inactive, NULL); >>> - } >>> } >>> >>> #ifdef CONFIG_HAS_DMA >>> @@ -2222,8 +2290,8 @@ static void of_spi_parse_dt_cs_delay(struct >> device_node *nc, >>> static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi, >>> struct device_node *nc) >>> { >>> - u32 value; >>> - int rc; >>> + u32 value, cs[SPI_CS_CNT_MAX]; >>> + int rc, idx; >>> >>> /* Mode (clock phase/polarity/etc.) */ >>> if (of_property_read_bool(nc, "spi-cpha")) @@ -2295,14 +2363,52 >> @@ >>> static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi, >>> return 0; >>> } >>> >>> + if (ctlr->num_chipselect > SPI_CS_CNT_MAX) { >>> + dev_err(&ctlr->dev, "No. of CS is more than max. no. of >> supported CS\n"); >>> + return -EINVAL; >>> + } >>> + >>> + /* >>> + * Zero(0) is a valid physical CS value and can be located at any >>> + * logical CS in the spi->chip_select[]. If all the physical CS >>> + * are initialized to 0 then It would be difficult to differentiate >>> + * between a valid physical CS 0 & an unused logical CS whose >> physical >>> + * CS can be 0. As a solution to this issue initialize all the CS to 0xFF. >>> + * Now all the unused logical CS will have 0xFF physical CS value & can >> be >>> + * ignore while performing physical CS validity checks. >>> + */ >>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) >>> + spi_set_chipselect(spi, idx, 0xFF); >>> + >>> /* Device address */ >>> - rc = of_property_read_u32(nc, "reg", &value); >>> - if (rc) { >>> + rc = of_property_read_variable_u32_array(nc, "reg", &cs[0], 1, >>> + SPI_CS_CNT_MAX); >>> + if (rc < 0) { >>> dev_err(&ctlr->dev, "%pOF has no valid 'reg' property >> (%d)\n", >>> nc, rc); >>> return rc; >>> } >>> - spi_set_chipselect(spi, 0, value); >>> + if (rc > ctlr->num_chipselect) { >>> + dev_err(&ctlr->dev, "%pOF has number of CS > ctlr- >>> num_chipselect (%d)\n", >>> + nc, rc); >>> + return rc; >>> + } >>> + if ((of_property_read_bool(nc, "parallel-memories")) && >>> + (!(ctlr->flags & SPI_CONTROLLER_MULTI_CS))) { >>> + dev_err(&ctlr->dev, "SPI controller doesn't support multi >> CS\n"); >>> + return -EINVAL; >>> + } >>> + for (idx = 0; idx < rc; idx++) >>> + spi_set_chipselect(spi, idx, cs[idx]); >>> + >>> + /* spi->chip_select[i] gives the corresponding physical CS for logical CS >> i >>> + * logical CS number is represented by setting the ith bit in spi- >>> cs_index_mask >>> + * So, for example, if spi->cs_index_mask = 0x01 then logical CS >> number is 0 and >>> + * spi->chip_select[0] will give the physical CS. >>> + * By default spi->chip_select[0] will hold the physical CS number so, >> set >>> + * spi->cs_index_mask as 0x01. >>> + */ >>> + spi->cs_index_mask = 0x01; >>> >>> /* Device speed */ >>> if (!of_property_read_u32(nc, "spi-max-frequency", &value)) @@ >>> -3100,6 +3206,7 @@ int spi_register_controller(struct spi_controller *ctlr) >>> struct boardinfo *bi; >>> int first_dynamic; >>> int status; >>> + int idx; >>> >>> if (!dev) >>> return -ENODEV; >>> @@ -3164,7 +3271,8 @@ int spi_register_controller(struct spi_controller >> *ctlr) >>> } >>> >>> /* Setting last_cs to -1 means no chip selected */ >>> - ctlr->last_cs = -1; >>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) >>> + ctlr->last_cs[idx] = -1; >>> >>> status = device_add(&ctlr->dev); >>> if (status < 0) >>> @@ -3889,7 +3997,7 @@ static int __spi_validate(struct spi_device *spi, >> struct spi_message *message) >>> * cs_change is set for each transfer. >>> */ >>> if ((spi->mode & SPI_CS_WORD) && (!(ctlr->mode_bits & >> SPI_CS_WORD) || >>> - spi_get_csgpiod(spi, 0))) { >>> + spi_is_csgpiod(spi))) { >>> size_t maxsize = BITS_TO_BYTES(spi->bits_per_word); >>> int ret; >>> >>> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index >>> 7b4baff63c5c..871d3a6d879b 100644 >>> --- a/include/linux/spi/spi.h >>> +++ b/include/linux/spi/spi.h >>> @@ -20,6 +20,9 @@ >>> >>> #include <uapi/linux/spi/spi.h> >>> >>> +/* Max no. of CS supported per spi device */ #define SPI_CS_CNT_MAX 4 >>> + >>> struct dma_chan; >>> struct software_node; >>> struct ptp_system_timestamp; >>> @@ -132,7 +135,8 @@ extern void >> spi_transfer_cs_change_delay_exec(struct spi_message *msg, >>> * @max_speed_hz: Maximum clock rate to be used with this chip >>> * (on this board); may be changed by the device's driver. >>> * The spi_transfer.speed_hz can override this for each transfer. >>> - * @chip_select: Chipselect, distinguishing chips handled by @controller. >>> + * @chip_select: Array of physical chipselect, spi->chipselect[i] gives >>> + * the corresponding physical CS for logical CS i. >>> * @mode: The spi mode defines how data is clocked out and in. >>> * This may be changed by the device's driver. >>> * The "active low" default for chipselect mode can be overridden >>> @@ -157,8 +161,8 @@ extern void >> spi_transfer_cs_change_delay_exec(struct spi_message *msg, >>> * the device will bind to the named driver and only the named driver. >>> * Do not set directly, because core frees it; use driver_set_override() to >>> * set or clear it. >>> - * @cs_gpiod: GPIO descriptor of the chipselect line (optional, NULL when >>> - * not using a GPIO line) >>> + * @cs_gpiod: Array of GPIO descriptors of the corresponding chipselect >> lines >>> + * (optional, NULL when not using a GPIO line) >>> * @word_delay: delay to be inserted between consecutive >>> * words of a transfer >>> * @cs_setup: delay to be introduced by the controller after CS is >>> asserted @@ -167,6 +171,7 @@ extern void >> spi_transfer_cs_change_delay_exec(struct spi_message *msg, >>> * deasserted. If @cs_change_delay is used from @spi_transfer, then >> the >>> * two delays will be added up. >>> * @pcpu_statistics: statistics for the spi_device >>> + * @cs_index_mask: Bit mask of the active chipselect(s) in the >>> + chipselect array >>> * >>> * A @spi_device is used to interchange data between an SPI slave >>> * (usually a discrete chip) and CPU memory. >>> @@ -182,7 +187,7 @@ struct spi_device { >>> struct spi_controller *controller; >>> struct spi_controller *master; /* Compatibility layer */ >>> u32 max_speed_hz; >>> - u8 chip_select; >>> + u8 chip_select[SPI_CS_CNT_MAX]; >>> u8 bits_per_word; >>> bool rt; >>> #define SPI_NO_TX BIT(31) /* No transmit wire */ >>> @@ -213,7 +218,7 @@ struct spi_device { >>> void *controller_data; >>> char modalias[SPI_NAME_SIZE]; >>> const char *driver_override; >>> - struct gpio_desc *cs_gpiod; /* Chip select GPIO descriptor >> */ >>> + struct gpio_desc *cs_gpiod[SPI_CS_CNT_MAX]; /* Chip select >> gpio desc */ >>> struct spi_delay word_delay; /* Inter-word delay */ >>> /* CS delays */ >>> struct spi_delay cs_setup; >>> @@ -223,6 +228,13 @@ struct spi_device { >>> /* The statistics */ >>> struct spi_statistics __percpu *pcpu_statistics; >>> >>> + /* Bit mask of the chipselect(s) that the driver need to use from >>> + * the chipselect array.When the controller is capable to handle >>> + * multiple chip selects & memories are connected in parallel >>> + * then more than one bit need to be set in cs_index_mask. >>> + */ >>> + u32 cs_index_mask : SPI_CS_CNT_MAX; >>> + >>> /* >>> * Likely need more hooks for more protocol options affecting how >>> * the controller talks to each chip, like: >>> @@ -279,22 +291,33 @@ static inline void *spi_get_drvdata(const struct >>> spi_device *spi) >>> >>> static inline u8 spi_get_chipselect(const struct spi_device *spi, u8 idx) >>> { >>> - return spi->chip_select; >>> + return spi->chip_select[idx]; >>> } >>> >>> static inline void spi_set_chipselect(struct spi_device *spi, u8 idx, u8 >> chipselect) >>> { >>> - spi->chip_select = chipselect; >>> + spi->chip_select[idx] = chipselect; >>> } >>> >>> static inline struct gpio_desc *spi_get_csgpiod(const struct spi_device *spi, >> u8 idx) >>> { >>> - return spi->cs_gpiod; >>> + return spi->cs_gpiod[idx]; >>> } >>> >>> static inline void spi_set_csgpiod(struct spi_device *spi, u8 idx, struct >> gpio_desc *csgpiod) >>> { >>> - spi->cs_gpiod = csgpiod; >>> + spi->cs_gpiod[idx] = csgpiod; >>> +} >>> + >>> +static inline bool spi_is_csgpiod(struct spi_device *spi) { >>> + u8 idx; >>> + >>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { >>> + if (spi_get_csgpiod(spi, idx)) >>> + return true; >>> + } >>> + return false; >>> } >>> >>> /** >>> @@ -399,6 +422,8 @@ extern struct spi_device >> *spi_new_ancillary_device(struct spi_device *spi, u8 ch >>> * @bus_lock_spinlock: spinlock for SPI bus locking >>> * @bus_lock_mutex: mutex for exclusion of multiple callers >>> * @bus_lock_flag: indicates that the SPI bus is locked for >>> exclusive use >>> + * @multi_cs_cap: indicates that the SPI Controller can assert/de-assert >>> + * more than one chip select at once. >>> * @setup: updates the device mode and clocking records used by a >>> * device's SPI controller; protocol code may call this. This >>> * must fail if an unrecognized or unsupported mode is requested. >>> @@ -567,6 +592,11 @@ struct spi_controller { >>> #define SPI_CONTROLLER_MUST_TX BIT(4) /* Requires tx */ >>> #define SPI_CONTROLLER_GPIO_SS BIT(5) /* GPIO CS must >> select slave */ >>> #define SPI_CONTROLLER_SUSPENDED BIT(6) /* Currently >> suspended */ >>> + /* >>> + * The spi-controller has multi chip select capability and can >>> + * assert/de-assert more than one chip select at once. >>> + */ >>> +#define SPI_CONTROLLER_MULTI_CS BIT(7) >>> >>> /* Flag indicating if the allocation of this struct is devres-managed */ >>> bool devm_allocated; >>> @@ -677,7 +707,8 @@ struct spi_controller { >>> bool rt; >>> bool auto_runtime_pm; >>> bool cur_msg_mapped; >>> - char last_cs; >>> + char last_cs[SPI_CS_CNT_MAX]; >>> + char last_cs_index_mask; >>> bool last_cs_mode_high; >>> bool fallback; >>> struct completion xfer_completion;
Hello Stefan, > -----Original Message----- > From: Stefan Binding <sbinding@opensource.cirrus.com> > Sent: Tuesday, November 21, 2023 7:28 PM > To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; > broonie@kernel.org; tudor.ambarus@linaro.org; pratyush@kernel.org; > miquel.raynal@bootlin.com; richard@nod.at; vigneshr@ti.com > Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; > michael@walle.cc; linux-mtd@lists.infradead.org; > nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com; > claudiu.beznea@tuxon.dev; Simek, Michal <michal.simek@amd.com>; linux- > arm-kernel@lists.infradead.org; git (AMD-Xilinx) <git@amd.com>; > amitrkcian2002@gmail.com; patches@opensource.cirrus.com > Subject: Re: [PATCH v10 1/8] spi: Add multi-cs memories support in SPI core > > > On 21/11/2023 07:39, Mahapatra, Amit Kumar wrote: > > Hello Stefan, > > > >> -----Original Message----- > >> From: Stefan Binding <sbinding@opensource.cirrus.com> > >> Sent: Monday, November 20, 2023 8:00 PM > >> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; > >> broonie@kernel.org; tudor.ambarus@linaro.org; pratyush@kernel.org; > >> miquel.raynal@bootlin.com; richard@nod.at; vigneshr@ti.com > >> Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; > >> michael@walle.cc; linux-mtd@lists.infradead.org; > >> nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com; > >> claudiu.beznea@tuxon.dev; Simek, Michal <michal.simek@amd.com>; > >> linux- arm-kernel@lists.infradead.org; git (AMD-Xilinx) > >> <git@amd.com>; amitrkcian2002@gmail.com; > >> patches@opensource.cirrus.com > >> Subject: Re: [PATCH v10 1/8] spi: Add multi-cs memories support in > >> SPI core > >> > >> > >> On 18/11/2023 13:54, Amit Kumar Mahapatra wrote: > >>> AMD-Xilinx GQSPI controller has two advanced mode that allows the > >>> controller to consider two flashes as one single device. > >>> > >>> One of these two mode is the parallel mode in which each byte of > >>> data is stored in both devices, the even bits in the lower flash & > >>> the odd bits in the upper flash. The byte split is automatically > >>> handled by the QSPI controller. > >>> > >>> The other mode is the stacked mode in which both the flashes share > >>> the same SPI bus but each of the device contain half of the data. In > >>> this mode, the controller does not follow CS requests but instead > >>> internally wires the two CS levels with the value of the most > >>> significant > >> address bit. > >>> For supporting both these modes SPI core need to be updated for > >>> providing multiple CS for a single SPI device. > >>> > >>> For adding multi CS support the SPI device need to be aware of all > >>> the CS values. So, the "chip_select" member in the spi_device > >>> structure is now an array that holds all the CS values. > >>> > >>> spi_device structure now has a "cs_index_mask" member. This acts as > >>> an index to the chip_select array. If nth bit of spi->cs_index_mask > >>> is set then the driver would assert spi->chip_select[n]. > >>> > >>> In parallel mode all the chip selects are asserted/de-asserted > >>> simultaneously and each byte of data is stored in both devices, the > >>> even bits in one, the odd bits in the other. The split is > >>> automatically handled by the GQSPI controller. The GQSPI controller > >>> supports a maximum of two flashes connected in parallel mode. A > >>> SPI_CONTROLLER_MULTI_CS flag bit is added in the spi controller > >>> flags, through ctlr->flags the spi core will make sure that the > >>> controller is capable of handling multiple chip selects at once. > >>> > >>> For supporting multiple CS via GPIO the cs_gpiod member of the > >>> spi_device structure is now an array that holds the gpio descriptor > >>> for each chipselect. > >>> > >>> CS GPIO is not tested due to unavailability of necessary hardware setup. > >>> > >>> Signed-off-by: Amit Kumar Mahapatra <amit.kumar- > mahapatra@amd.com> > >>> --- > >>> Hello @Stefen, > >>> We restructured the SPI core implementation, for handling arbitrary > >>> number of devices connected in parallel(multi-cs) or stacked mode. > >>> We have tested this updated patch on native-cs setup but couldn't > >>> test cs-gpio due to unavailability of a proper setup. If possible, > >>> could you please retest the cs-gpio use case with this updated patch > >>> and provide > >> your feedback. > >> > >> Hi, > >> > >> I tested this chain on 2 different systems using GPIOs as chip > >> selects, and see the same error on both: > >> [ 2.842045] pxa2xx-spi pxa2xx-spi.6: chipselect 0 already in use > >> > >> Let me know if you need any further testing. > >> > >> Thanks, > >> > >> Stefan Binding > >> > >>> --- > >>> drivers/spi/spi.c | 192 +++++++++++++++++++++++++++++++--------- > >>> include/linux/spi/spi.h | 51 ++++++++--- > >>> 2 files changed, 191 insertions(+), 52 deletions(-) > >>> > >>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index > >>> 8ead7acb99f3..ff66147ba95f 100644 > >>> --- a/drivers/spi/spi.c > >>> +++ b/drivers/spi/spi.c > >>> @@ -612,10 +612,21 @@ static int spi_dev_check(struct device *dev, > >>> void > >> *data) > >>> { > >>> struct spi_device *spi = to_spi_device(dev); > >>> struct spi_device *new_spi = data; > >>> - > >>> - if (spi->controller == new_spi->controller && > >>> - spi_get_chipselect(spi, 0) == spi_get_chipselect(new_spi, 0)) > >>> - return -EBUSY; > >>> + int idx, nw_idx; > >>> + u8 cs, cs_nw; > >>> + > >>> + if (spi->controller == new_spi->controller) { > >>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > >>> + cs = spi_get_chipselect(spi, idx); > >>> + for (nw_idx = 0; nw_idx < SPI_CS_CNT_MAX; > >> nw_idx++) { > >>> + cs_nw = spi_get_chipselect(spi, nw_idx); > > Thank you for dedicating your time to test my patch. > > As per my analysis the error is reported from here as we are supplying > > the former SPI device structure to retrieve the CS value for the new > > SPI devices. > > To fix this, could you kindly substitute the above line with > > > > cs_nw = spi_get_chipselect(new_spi, nw_idx); > > > > and rerun your tests? > > If it works correctly, I will incorporate this fix into my upcoming > > series. > > > > Regards, > > Amit > > Hi, > > I still see the same error: > > [ 2.748546] pxa2xx-spi pxa2xx-spi.6: chipselect 0 already in use Thank you for the quick testing. For further analysis I have incorporated additional debug prints on top of 1/8 patch. The corresponding diff is shared below. Kindly implement the specified changes, rerun your test and share the kernel logs. diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index ff66147ba95f..7f59ea81593d 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -618,8 +618,10 @@ static int spi_dev_check(struct device *dev, void *data) if (spi->controller == new_spi->controller) { for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { cs = spi_get_chipselect(spi, idx); + printk("%s() [%d] CS[%d] = [%d]\n", __func__, __LINE__, idx, cs); for (nw_idx = 0; nw_idx < SPI_CS_CNT_MAX; nw_idx++) { - cs_nw = spi_get_chipselect(spi, nw_idx); + cs_nw = spi_get_chipselect(new_spi, nw_idx); + printk("%s() [%d] CS_NEW[%d] = [%d]\n", __func__, __LINE__, nw_idx, cs_nw); if (cs != 0xFF && cs_nw != 0xFF && cs == cs_nw) { dev_err(dev, "chipselect %d already in use\n", cs_nw); return -EBUSY; @@ -659,8 +661,10 @@ static int __spi_add_device(struct spi_device *spi) */ for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { cs = spi_get_chipselect(spi, idx); + printk("%s() [%d] CS[%d] = [%d]\n", __func__, __LINE__, idx, cs); for (nw_idx = idx + 1; nw_idx < SPI_CS_CNT_MAX; nw_idx++) { nw_cs = spi_get_chipselect(spi, nw_idx); + printk("%s() [%d] CS_NEW[%d] = [%d]\n", __func__, __LINE__, nw_idx, nw_cs); if (cs != 0xFF && nw_cs != 0xFF && cs == nw_cs) { dev_err(dev, "chipselect %d already in use\n", nw_cs); return -EBUSY; @@ -2401,6 +2405,9 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi, for (idx = 0; idx < rc; idx++) spi_set_chipselect(spi, idx, cs[idx]); + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) + printk("%s() [%d] CS[%d] = [%d]\n", __func__, __LINE__, idx, spi_get_chipselect(spi, idx)); + /* spi->chip_select[i] gives the corresponding physical CS for logical CS i * logical CS number is represented by setting the ith bit in spi->cs_index_mask * So, for example, if spi->cs_index_mask = 0x01 then logical CS number is 0 and Regards, Amit > > Thanks, > > Stefan > > > > >>> + if (cs != 0xFF && cs_nw != 0xFF && cs == > >> cs_nw) { > >>> + dev_err(dev, "chipselect %d already in > >> use\n", cs_nw); > >>> + return -EBUSY; > >>> + } > >>> + } > >>> + } > >>> + } > >>> return 0; > >>> } > >>> > >>> @@ -629,13 +640,32 @@ static int __spi_add_device(struct spi_device > *spi) > >>> { > >>> struct spi_controller *ctlr = spi->controller; > >>> struct device *dev = ctlr->dev.parent; > >>> - int status; > >>> + int status, idx, nw_idx; > >>> + u8 cs, nw_cs; > >>> + > >>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > >>> + /* Chipselects are numbered 0..max; validate. */ > >>> + cs = spi_get_chipselect(spi, idx); > >>> + if (cs != 0xFF && cs >= ctlr->num_chipselect) { > >>> + dev_err(dev, "cs%d >= max %d\n", > >> spi_get_chipselect(spi, idx), > >>> + ctlr->num_chipselect); > >>> + return -EINVAL; > >>> + } > >>> + } > >>> > >>> - /* Chipselects are numbered 0..max; validate. */ > >>> - if (spi_get_chipselect(spi, 0) >= ctlr->num_chipselect) { > >>> - dev_err(dev, "cs%d >= max %d\n", spi_get_chipselect(spi, 0), > >>> - ctlr->num_chipselect); > >>> - return -EINVAL; > >>> + /* > >>> + * Make sure that multiple logical CS doesn't map to the same > >> physical CS. > >>> + * For example, spi->chip_select[0] != spi->chip_select[1] and so on. > >>> + */ > >>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > >>> + cs = spi_get_chipselect(spi, idx); > >>> + for (nw_idx = idx + 1; nw_idx < SPI_CS_CNT_MAX; nw_idx++) { > >>> + nw_cs = spi_get_chipselect(spi, nw_idx); > >>> + if (cs != 0xFF && nw_cs != 0xFF && cs == nw_cs) { > >>> + dev_err(dev, "chipselect %d already in use\n", > >> nw_cs); > >>> + return -EBUSY; > >>> + } > >>> + } > >>> } > >>> > >>> /* Set the bus ID string */ > >>> @@ -647,11 +677,8 @@ static int __spi_add_device(struct spi_device > *spi) > >>> * its configuration. > >>> */ > >>> status = bus_for_each_dev(&spi_bus_type, NULL, spi, spi_dev_check); > >>> - if (status) { > >>> - dev_err(dev, "chipselect %d already in use\n", > >>> - spi_get_chipselect(spi, 0)); > >>> + if (status) > >>> return status; > >>> - } > >>> > >>> /* Controller may unregister concurrently */ > >>> if (IS_ENABLED(CONFIG_SPI_DYNAMIC) && @@ -659,8 +686,15 @@ > >> static > >>> int __spi_add_device(struct spi_device *spi) > >>> return -ENODEV; > >>> } > >>> > >>> - if (ctlr->cs_gpiods) > >>> - spi_set_csgpiod(spi, 0, ctlr->cs_gpiods[spi_get_chipselect(spi, > >> 0)]); > >>> + if (ctlr->cs_gpiods) { > >>> + u8 cs; > >>> + > >>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > >>> + cs = spi_get_chipselect(spi, idx); > >>> + if (cs != 0xFF) > >>> + spi_set_csgpiod(spi, idx, ctlr->cs_gpiods[cs]); > >>> + } > >>> + } > >>> > >>> /* > >>> * Drivers may modify this initial i/o setup, but will @@ -701,6 > >>> +735,9 @@ int spi_add_device(struct spi_device *spi) > >>> struct spi_controller *ctlr = spi->controller; > >>> int status; > >>> > >>> + /* Set the bus ID string */ > >>> + spi_dev_set_name(spi); > >>> + > >>> mutex_lock(&ctlr->add_lock); > >>> status = __spi_add_device(spi); > >>> mutex_unlock(&ctlr->add_lock); > >>> @@ -942,32 +979,51 @@ static void spi_res_release(struct > >>> spi_controller > >> *ctlr, struct spi_message *mes > >>> } > >>> > >>> > >>> /*------------------------------------------------------------------ > >>> -- > >>> -----*/ > >>> +static inline bool spi_is_last_cs(struct spi_device *spi) { > >>> + u8 idx; > >>> + bool last = false; > >>> + > >>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > >>> + if ((spi->cs_index_mask >> idx) & 0x01) { > >>> + if (spi->controller->last_cs[idx] == > >> spi_get_chipselect(spi, idx)) > >>> + last = true; > >>> + } > >>> + } > >>> + return last; > >>> +} > >>> + > >>> > >>> static void spi_set_cs(struct spi_device *spi, bool enable, bool force) > >>> { > >>> bool activate = enable; > >>> + u8 idx; > >>> > >>> /* > >>> * Avoid calling into the driver (or doing delays) if the chip select > >>> * isn't actually changing from the last time this was called. > >>> */ > >>> - if (!force && ((enable && spi->controller->last_cs == > >> spi_get_chipselect(spi, 0)) || > >>> - (!enable && spi->controller->last_cs != > >> spi_get_chipselect(spi, 0))) && > >>> + if (!force && ((enable && spi->controller->last_cs_index_mask == > >>> +spi- > >>> cs_index_mask && > >>> + spi_is_last_cs(spi)) || > >>> + (!enable && spi->controller->last_cs_index_mask == spi- > >>> cs_index_mask && > >>> + !spi_is_last_cs(spi))) && > >>> (spi->controller->last_cs_mode_high == (spi->mode & > >> SPI_CS_HIGH))) > >>> return; > >>> > >>> trace_spi_set_cs(spi, activate); > >>> > >>> - spi->controller->last_cs = enable ? spi_get_chipselect(spi, 0) : -1; > >>> + spi->controller->last_cs_index_mask = spi->cs_index_mask; > >>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > >>> + spi->controller->last_cs[idx] = enable ? spi_get_chipselect(spi, > >> 0) > >>> +: -1; > >>> spi->controller->last_cs_mode_high = spi->mode & SPI_CS_HIGH; > >>> > >>> - if ((spi_get_csgpiod(spi, 0) || !spi->controller->set_cs_timing) && > >> !activate) > >>> - spi_delay_exec(&spi->cs_hold, NULL); > >>> - > >>> if (spi->mode & SPI_CS_HIGH) > >>> enable = !enable; > >>> > >>> - if (spi_get_csgpiod(spi, 0)) { > >>> + if (spi_is_csgpiod(spi)) { > >>> + if (!spi->controller->set_cs_timing && !activate) > >>> + spi_delay_exec(&spi->cs_hold, NULL); > >>> + > >>> if (!(spi->mode & SPI_NO_CS)) { > >>> /* > >>> * Historically ACPI has no means of the GPIO polarity > >> and @@ > >>> -979,26 +1035,38 @@ static void spi_set_cs(struct spi_device *spi, > >>> bool > >> enable, bool force) > >>> * ambiguity. That's why we use enable, that takes > >> SPI_CS_HIGH > >>> * into account. > >>> */ > >>> - if (has_acpi_companion(&spi->dev)) > >>> - > >> gpiod_set_value_cansleep(spi_get_csgpiod(spi, 0), !enable); > >>> - else > >>> - /* Polarity handled by GPIO library */ > >>> - > >> gpiod_set_value_cansleep(spi_get_csgpiod(spi, 0), activate); > >>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > >>> + if (((spi->cs_index_mask >> idx) & 0x01) && > >>> + spi_get_csgpiod(spi, idx)) { > >>> + if (has_acpi_companion(&spi->dev)) > >>> + > >> gpiod_set_value_cansleep(spi_get_csgpiod(spi, idx), > >>> + > >> !enable); > >>> + else > >>> + /* Polarity handled by GPIO > >> library */ > >>> + > >> gpiod_set_value_cansleep(spi_get_csgpiod(spi, idx), > >>> + > >> activate); > >>> + > >>> + if (activate) > >>> + spi_delay_exec(&spi- > >>> cs_setup, NULL); > >>> + else > >>> + spi_delay_exec(&spi- > >>> cs_inactive, NULL); > >>> + } > >>> + } > >>> } > >>> /* Some SPI masters need both GPIO CS & slave_select */ > >>> if ((spi->controller->flags & SPI_CONTROLLER_GPIO_SS) && > >>> spi->controller->set_cs) > >>> spi->controller->set_cs(spi, !enable); > >>> + > >>> + if (!spi->controller->set_cs_timing) { > >>> + if (activate) > >>> + spi_delay_exec(&spi->cs_setup, NULL); > >>> + else > >>> + spi_delay_exec(&spi->cs_inactive, NULL); > >>> + } > >>> } else if (spi->controller->set_cs) { > >>> spi->controller->set_cs(spi, !enable); > >>> } > >>> - > >>> - if (spi_get_csgpiod(spi, 0) || !spi->controller->set_cs_timing) { > >>> - if (activate) > >>> - spi_delay_exec(&spi->cs_setup, NULL); > >>> - else > >>> - spi_delay_exec(&spi->cs_inactive, NULL); > >>> - } > >>> } > >>> > >>> #ifdef CONFIG_HAS_DMA > >>> @@ -2222,8 +2290,8 @@ static void of_spi_parse_dt_cs_delay(struct > >> device_node *nc, > >>> static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device > *spi, > >>> struct device_node *nc) > >>> { > >>> - u32 value; > >>> - int rc; > >>> + u32 value, cs[SPI_CS_CNT_MAX]; > >>> + int rc, idx; > >>> > >>> /* Mode (clock phase/polarity/etc.) */ > >>> if (of_property_read_bool(nc, "spi-cpha")) @@ -2295,14 +2363,52 > >> @@ > >>> static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi, > >>> return 0; > >>> } > >>> > >>> + if (ctlr->num_chipselect > SPI_CS_CNT_MAX) { > >>> + dev_err(&ctlr->dev, "No. of CS is more than max. no. of > >> supported CS\n"); > >>> + return -EINVAL; > >>> + } > >>> + > >>> + /* > >>> + * Zero(0) is a valid physical CS value and can be located at any > >>> + * logical CS in the spi->chip_select[]. If all the physical CS > >>> + * are initialized to 0 then It would be difficult to differentiate > >>> + * between a valid physical CS 0 & an unused logical CS whose > >> physical > >>> + * CS can be 0. As a solution to this issue initialize all the CS to 0xFF. > >>> + * Now all the unused logical CS will have 0xFF physical CS value > >>> +& can > >> be > >>> + * ignore while performing physical CS validity checks. > >>> + */ > >>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > >>> + spi_set_chipselect(spi, idx, 0xFF); > >>> + > >>> /* Device address */ > >>> - rc = of_property_read_u32(nc, "reg", &value); > >>> - if (rc) { > >>> + rc = of_property_read_variable_u32_array(nc, "reg", &cs[0], 1, > >>> + SPI_CS_CNT_MAX); > >>> + if (rc < 0) { > >>> dev_err(&ctlr->dev, "%pOF has no valid 'reg' property > >> (%d)\n", > >>> nc, rc); > >>> return rc; > >>> } > >>> - spi_set_chipselect(spi, 0, value); > >>> + if (rc > ctlr->num_chipselect) { > >>> + dev_err(&ctlr->dev, "%pOF has number of CS > ctlr- > >>> num_chipselect (%d)\n", > >>> + nc, rc); > >>> + return rc; > >>> + } > >>> + if ((of_property_read_bool(nc, "parallel-memories")) && > >>> + (!(ctlr->flags & SPI_CONTROLLER_MULTI_CS))) { > >>> + dev_err(&ctlr->dev, "SPI controller doesn't support multi > >> CS\n"); > >>> + return -EINVAL; > >>> + } > >>> + for (idx = 0; idx < rc; idx++) > >>> + spi_set_chipselect(spi, idx, cs[idx]); > >>> + > >>> + /* spi->chip_select[i] gives the corresponding physical CS for > >>> +logical CS > >> i > >>> + * logical CS number is represented by setting the ith bit in spi- > >>> cs_index_mask > >>> + * So, for example, if spi->cs_index_mask = 0x01 then logical CS > >> number is 0 and > >>> + * spi->chip_select[0] will give the physical CS. > >>> + * By default spi->chip_select[0] will hold the physical CS number > >>> +so, > >> set > >>> + * spi->cs_index_mask as 0x01. > >>> + */ > >>> + spi->cs_index_mask = 0x01; > >>> > >>> /* Device speed */ > >>> if (!of_property_read_u32(nc, "spi-max-frequency", &value)) @@ > >>> -3100,6 +3206,7 @@ int spi_register_controller(struct spi_controller *ctlr) > >>> struct boardinfo *bi; > >>> int first_dynamic; > >>> int status; > >>> + int idx; > >>> > >>> if (!dev) > >>> return -ENODEV; > >>> @@ -3164,7 +3271,8 @@ int spi_register_controller(struct > >>> spi_controller > >> *ctlr) > >>> } > >>> > >>> /* Setting last_cs to -1 means no chip selected */ > >>> - ctlr->last_cs = -1; > >>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > >>> + ctlr->last_cs[idx] = -1; > >>> > >>> status = device_add(&ctlr->dev); > >>> if (status < 0) > >>> @@ -3889,7 +3997,7 @@ static int __spi_validate(struct spi_device > >>> *spi, > >> struct spi_message *message) > >>> * cs_change is set for each transfer. > >>> */ > >>> if ((spi->mode & SPI_CS_WORD) && (!(ctlr->mode_bits & > >> SPI_CS_WORD) || > >>> - spi_get_csgpiod(spi, 0))) { > >>> + spi_is_csgpiod(spi))) { > >>> size_t maxsize = BITS_TO_BYTES(spi->bits_per_word); > >>> int ret; > >>> > >>> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index > >>> 7b4baff63c5c..871d3a6d879b 100644 > >>> --- a/include/linux/spi/spi.h > >>> +++ b/include/linux/spi/spi.h > >>> @@ -20,6 +20,9 @@ > >>> > >>> #include <uapi/linux/spi/spi.h> > >>> > >>> +/* Max no. of CS supported per spi device */ #define SPI_CS_CNT_MAX > >>> +4 > >>> + > >>> struct dma_chan; > >>> struct software_node; > >>> struct ptp_system_timestamp; > >>> @@ -132,7 +135,8 @@ extern void > >> spi_transfer_cs_change_delay_exec(struct spi_message *msg, > >>> * @max_speed_hz: Maximum clock rate to be used with this chip > >>> * (on this board); may be changed by the device's driver. > >>> * The spi_transfer.speed_hz can override this for each transfer. > >>> - * @chip_select: Chipselect, distinguishing chips handled by @controller. > >>> + * @chip_select: Array of physical chipselect, spi->chipselect[i] gives > >>> + * the corresponding physical CS for logical CS i. > >>> * @mode: The spi mode defines how data is clocked out and in. > >>> * This may be changed by the device's driver. > >>> * The "active low" default for chipselect mode can be overridden > >>> @@ -157,8 +161,8 @@ extern void > >> spi_transfer_cs_change_delay_exec(struct spi_message *msg, > >>> * the device will bind to the named driver and only the named driver. > >>> * Do not set directly, because core frees it; use driver_set_override() to > >>> * set or clear it. > >>> - * @cs_gpiod: GPIO descriptor of the chipselect line (optional, NULL > when > >>> - * not using a GPIO line) > >>> + * @cs_gpiod: Array of GPIO descriptors of the corresponding > >>> + chipselect > >> lines > >>> + * (optional, NULL when not using a GPIO line) > >>> * @word_delay: delay to be inserted between consecutive > >>> * words of a transfer > >>> * @cs_setup: delay to be introduced by the controller after CS > >>> is asserted @@ -167,6 +171,7 @@ extern void > >> spi_transfer_cs_change_delay_exec(struct spi_message *msg, > >>> * deasserted. If @cs_change_delay is used from @spi_transfer, then > >> the > >>> * two delays will be added up. > >>> * @pcpu_statistics: statistics for the spi_device > >>> + * @cs_index_mask: Bit mask of the active chipselect(s) in the > >>> + chipselect array > >>> * > >>> * A @spi_device is used to interchange data between an SPI slave > >>> * (usually a discrete chip) and CPU memory. > >>> @@ -182,7 +187,7 @@ struct spi_device { > >>> struct spi_controller *controller; > >>> struct spi_controller *master; /* Compatibility layer */ > >>> u32 max_speed_hz; > >>> - u8 chip_select; > >>> + u8 chip_select[SPI_CS_CNT_MAX]; > >>> u8 bits_per_word; > >>> bool rt; > >>> #define SPI_NO_TX BIT(31) /* No transmit wire */ > >>> @@ -213,7 +218,7 @@ struct spi_device { > >>> void *controller_data; > >>> char modalias[SPI_NAME_SIZE]; > >>> const char *driver_override; > >>> - struct gpio_desc *cs_gpiod; /* Chip select GPIO descriptor > >> */ > >>> + struct gpio_desc *cs_gpiod[SPI_CS_CNT_MAX]; /* Chip select > >> gpio desc */ > >>> struct spi_delay word_delay; /* Inter-word delay */ > >>> /* CS delays */ > >>> struct spi_delay cs_setup; > >>> @@ -223,6 +228,13 @@ struct spi_device { > >>> /* The statistics */ > >>> struct spi_statistics __percpu *pcpu_statistics; > >>> > >>> + /* Bit mask of the chipselect(s) that the driver need to use from > >>> + * the chipselect array.When the controller is capable to handle > >>> + * multiple chip selects & memories are connected in parallel > >>> + * then more than one bit need to be set in cs_index_mask. > >>> + */ > >>> + u32 cs_index_mask : SPI_CS_CNT_MAX; > >>> + > >>> /* > >>> * Likely need more hooks for more protocol options affecting how > >>> * the controller talks to each chip, like: > >>> @@ -279,22 +291,33 @@ static inline void *spi_get_drvdata(const > >>> struct spi_device *spi) > >>> > >>> static inline u8 spi_get_chipselect(const struct spi_device *spi, u8 idx) > >>> { > >>> - return spi->chip_select; > >>> + return spi->chip_select[idx]; > >>> } > >>> > >>> static inline void spi_set_chipselect(struct spi_device *spi, u8 > >>> idx, u8 > >> chipselect) > >>> { > >>> - spi->chip_select = chipselect; > >>> + spi->chip_select[idx] = chipselect; > >>> } > >>> > >>> static inline struct gpio_desc *spi_get_csgpiod(const struct > >>> spi_device *spi, > >> u8 idx) > >>> { > >>> - return spi->cs_gpiod; > >>> + return spi->cs_gpiod[idx]; > >>> } > >>> > >>> static inline void spi_set_csgpiod(struct spi_device *spi, u8 > >>> idx, struct > >> gpio_desc *csgpiod) > >>> { > >>> - spi->cs_gpiod = csgpiod; > >>> + spi->cs_gpiod[idx] = csgpiod; > >>> +} > >>> + > >>> +static inline bool spi_is_csgpiod(struct spi_device *spi) { > >>> + u8 idx; > >>> + > >>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > >>> + if (spi_get_csgpiod(spi, idx)) > >>> + return true; > >>> + } > >>> + return false; > >>> } > >>> > >>> /** > >>> @@ -399,6 +422,8 @@ extern struct spi_device > >> *spi_new_ancillary_device(struct spi_device *spi, u8 ch > >>> * @bus_lock_spinlock: spinlock for SPI bus locking > >>> * @bus_lock_mutex: mutex for exclusion of multiple callers > >>> * @bus_lock_flag: indicates that the SPI bus is locked for > >>> exclusive use > >>> + * @multi_cs_cap: indicates that the SPI Controller can assert/de-assert > >>> + * more than one chip select at once. > >>> * @setup: updates the device mode and clocking records used by a > >>> * device's SPI controller; protocol code may call this. This > >>> * must fail if an unrecognized or unsupported mode is requested. > >>> @@ -567,6 +592,11 @@ struct spi_controller { > >>> #define SPI_CONTROLLER_MUST_TX BIT(4) /* Requires tx > */ > >>> #define SPI_CONTROLLER_GPIO_SS BIT(5) /* GPIO CS > must > >> select slave */ > >>> #define SPI_CONTROLLER_SUSPENDED BIT(6) /* Currently > >> suspended */ > >>> + /* > >>> + * The spi-controller has multi chip select capability and can > >>> + * assert/de-assert more than one chip select at once. > >>> + */ > >>> +#define SPI_CONTROLLER_MULTI_CS BIT(7) > >>> > >>> /* Flag indicating if the allocation of this struct is devres-managed */ > >>> bool devm_allocated; > >>> @@ -677,7 +707,8 @@ struct spi_controller { > >>> bool rt; > >>> bool auto_runtime_pm; > >>> bool cur_msg_mapped; > >>> - char last_cs; > >>> + char last_cs[SPI_CS_CNT_MAX]; > >>> + char last_cs_index_mask; > >>> bool last_cs_mode_high; > >>> bool fallback; > >>> struct completion xfer_completion;
Hello Stefan, > -----Original Message----- > From: Stefan Binding <sbinding@opensource.cirrus.com> > Sent: Tuesday, November 21, 2023 11:07 PM > To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; > broonie@kernel.org; tudor.ambarus@linaro.org; pratyush@kernel.org; > miquel.raynal@bootlin.com; richard@nod.at; vigneshr@ti.com > Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; > michael@walle.cc; linux-mtd@lists.infradead.org; > nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com; > claudiu.beznea@tuxon.dev; Simek, Michal <michal.simek@amd.com>; linux- > arm-kernel@lists.infradead.org; git (AMD-Xilinx) <git@amd.com>; > amitrkcian2002@gmail.com; patches@opensource.cirrus.com > Subject: Re: [PATCH v10 1/8] spi: Add multi-cs memories support in SPI core > > > On 21/11/2023 16:35, Mahapatra, Amit Kumar wrote: > > Hello Stefan, > > > >> -----Original Message----- > >> From: Stefan Binding <sbinding@opensource.cirrus.com> > >> Sent: Tuesday, November 21, 2023 7:28 PM > >> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; > >> broonie@kernel.org; tudor.ambarus@linaro.org; pratyush@kernel.org; > >> miquel.raynal@bootlin.com; richard@nod.at; vigneshr@ti.com > >> Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; > >> michael@walle.cc; linux-mtd@lists.infradead.org; > >> nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com; > >> claudiu.beznea@tuxon.dev; Simek, Michal <michal.simek@amd.com>; > >> linux- arm-kernel@lists.infradead.org; git (AMD-Xilinx) > >> <git@amd.com>; amitrkcian2002@gmail.com; > >> patches@opensource.cirrus.com > >> Subject: Re: [PATCH v10 1/8] spi: Add multi-cs memories support in > >> SPI core > >> > >> > >> On 21/11/2023 07:39, Mahapatra, Amit Kumar wrote: > >>> Hello Stefan, > >>> > >>>> -----Original Message----- > >>>> From: Stefan Binding <sbinding@opensource.cirrus.com> > >>>> Sent: Monday, November 20, 2023 8:00 PM > >>>> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; > >>>> broonie@kernel.org; tudor.ambarus@linaro.org; pratyush@kernel.org; > >>>> miquel.raynal@bootlin.com; richard@nod.at; vigneshr@ti.com > >>>> Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; > >>>> michael@walle.cc; linux-mtd@lists.infradead.org; > >>>> nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com; > >>>> claudiu.beznea@tuxon.dev; Simek, Michal <michal.simek@amd.com>; > >>>> linux- arm-kernel@lists.infradead.org; git (AMD-Xilinx) > >>>> <git@amd.com>; amitrkcian2002@gmail.com; > >>>> patches@opensource.cirrus.com > >>>> Subject: Re: [PATCH v10 1/8] spi: Add multi-cs memories support in > >>>> SPI core > >>>> > >>>> > >>>> On 18/11/2023 13:54, Amit Kumar Mahapatra wrote: > >>>>> AMD-Xilinx GQSPI controller has two advanced mode that allows the > >>>>> controller to consider two flashes as one single device. > >>>>> > >>>>> One of these two mode is the parallel mode in which each byte of > >>>>> data is stored in both devices, the even bits in the lower flash & > >>>>> the odd bits in the upper flash. The byte split is automatically > >>>>> handled by the QSPI controller. > >>>>> > >>>>> The other mode is the stacked mode in which both the flashes share > >>>>> the same SPI bus but each of the device contain half of the data. > >>>>> In this mode, the controller does not follow CS requests but > >>>>> instead internally wires the two CS levels with the value of the > >>>>> most significant > >>>> address bit. > >>>>> For supporting both these modes SPI core need to be updated for > >>>>> providing multiple CS for a single SPI device. > >>>>> > >>>>> For adding multi CS support the SPI device need to be aware of all > >>>>> the CS values. So, the "chip_select" member in the spi_device > >>>>> structure is now an array that holds all the CS values. > >>>>> > >>>>> spi_device structure now has a "cs_index_mask" member. This acts > >>>>> as an index to the chip_select array. If nth bit of > >>>>> spi->cs_index_mask is set then the driver would assert spi- > >chip_select[n]. > >>>>> > >>>>> In parallel mode all the chip selects are asserted/de-asserted > >>>>> simultaneously and each byte of data is stored in both devices, > >>>>> the even bits in one, the odd bits in the other. The split is > >>>>> automatically handled by the GQSPI controller. The GQSPI > >>>>> controller supports a maximum of two flashes connected in parallel > >>>>> mode. A SPI_CONTROLLER_MULTI_CS flag bit is added in the spi > >>>>> controller flags, through ctlr->flags the spi core will make sure > >>>>> that the controller is capable of handling multiple chip selects at once. > >>>>> > >>>>> For supporting multiple CS via GPIO the cs_gpiod member of the > >>>>> spi_device structure is now an array that holds the gpio > >>>>> descriptor for each chipselect. > >>>>> > >>>>> CS GPIO is not tested due to unavailability of necessary hardware > setup. > >>>>> > >>>>> Signed-off-by: Amit Kumar Mahapatra <amit.kumar- > >> mahapatra@amd.com> > >>>>> --- > >>>>> Hello @Stefen, > >>>>> We restructured the SPI core implementation, for handling > >>>>> arbitrary number of devices connected in parallel(multi-cs) or stacked > mode. > >>>>> We have tested this updated patch on native-cs setup but couldn't > >>>>> test cs-gpio due to unavailability of a proper setup. If possible, > >>>>> could you please retest the cs-gpio use case with this updated > >>>>> patch and provide > >>>> your feedback. > >>>> > >>>> Hi, > >>>> > >>>> I tested this chain on 2 different systems using GPIOs as chip > >>>> selects, and see the same error on both: > >>>> [ 2.842045] pxa2xx-spi pxa2xx-spi.6: chipselect 0 already in use > >>>> > >>>> Let me know if you need any further testing. > >>>> > >>>> Thanks, > >>>> > >>>> Stefan Binding > >>>> > >>>>> --- > >>>>> drivers/spi/spi.c | 192 +++++++++++++++++++++++++++++++------- > -- > >>>>> include/linux/spi/spi.h | 51 ++++++++--- > >>>>> 2 files changed, 191 insertions(+), 52 deletions(-) > >>>>> > >>>>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index > >>>>> 8ead7acb99f3..ff66147ba95f 100644 > >>>>> --- a/drivers/spi/spi.c > >>>>> +++ b/drivers/spi/spi.c > >>>>> @@ -612,10 +612,21 @@ static int spi_dev_check(struct device *dev, > >>>>> void > >>>> *data) > >>>>> { > >>>>> struct spi_device *spi = to_spi_device(dev); > >>>>> struct spi_device *new_spi = data; > >>>>> - > >>>>> - if (spi->controller == new_spi->controller && > >>>>> - spi_get_chipselect(spi, 0) == spi_get_chipselect(new_spi, 0)) > >>>>> - return -EBUSY; > >>>>> + int idx, nw_idx; > >>>>> + u8 cs, cs_nw; > >>>>> + > >>>>> + if (spi->controller == new_spi->controller) { > >>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > >>>>> + cs = spi_get_chipselect(spi, idx); > >>>>> + for (nw_idx = 0; nw_idx < SPI_CS_CNT_MAX; > >>>> nw_idx++) { > >>>>> + cs_nw = spi_get_chipselect(spi, > nw_idx); > >>> Thank you for dedicating your time to test my patch. > >>> As per my analysis the error is reported from here as we are > >>> supplying the former SPI device structure to retrieve the CS value > >>> for the new SPI devices. > >>> To fix this, could you kindly substitute the above line with > >>> > >>> cs_nw = spi_get_chipselect(new_spi, nw_idx); > >>> > >>> and rerun your tests? > >>> If it works correctly, I will incorporate this fix into my upcoming > >>> series. > >>> > >>> Regards, > >>> Amit > > Hi, > > I've attached my log. > I notice that you add a print to of_spi_parse_dt, however since the laptop I > am using is an x86 laptop, it uses ACPI rather than OF, and I don't think this > function isnt even compiled in. That’s correct, I missed it. Upon reviewing the logs, I discovered that in the ACPI flow, I am not initializing the unused CS[] to FF, as I am doing in the OF flow. Consequently, in the ACPI flow, all the unused CS[] are automatically initialized to 0. During the __spi_add_device process, the SPI core faces difficulty distinguishing between a valid CS value of 0 and an unused CS value of 0, leading to a failure in the driver probe. Incorporating the following modifications should resolve this issue. Kindly apply these changes on top of 1/8 and re-run the test. diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index ff66147ba95f..d0301c17d4f8 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -619,7 +619,7 @@ static int spi_dev_check(struct device *dev, void *data) for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { cs = spi_get_chipselect(spi, idx); for (nw_idx = 0; nw_idx < SPI_CS_CNT_MAX; nw_idx++) { - cs_nw = spi_get_chipselect(spi, nw_idx); + cs_nw = spi_get_chipselect(new_spi, nw_idx); if (cs != 0xFF && cs_nw != 0xFF && cs == cs_nw) { dev_err(dev, "chipselect %d already in use\n", cs_nw); return -EBUSY; @@ -764,6 +764,7 @@ struct spi_device *spi_new_device(struct spi_controller *ctlr, { struct spi_device *proxy; int status; + u8 idx; /* * NOTE: caller did any chip->bus_num checks necessary. @@ -779,6 +780,9 @@ struct spi_device *spi_new_device(struct spi_controller *ctlr, WARN_ON(strlen(chip->modalias) >= sizeof(proxy->modalias)); + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) + spi_set_chipselect(proxy, idx, 0xFF); + spi_set_chipselect(proxy, 0, chip->chip_select); proxy->max_speed_hz = chip->max_speed_hz; proxy->mode = chip->mode; @@ -2514,6 +2518,7 @@ struct spi_device *spi_new_ancillary_device(struct spi_device *spi, struct spi_controller *ctlr = spi->controller; struct spi_device *ancillary; int rc = 0; + u8 idx; /* Alloc an spi_device */ ancillary = spi_alloc_device(ctlr); @@ -2524,6 +2529,9 @@ struct spi_device *spi_new_ancillary_device(struct spi_device *spi, strscpy(ancillary->modalias, "dummy", sizeof(ancillary->modalias)); + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) + spi_set_chipselect(ancillary, idx, 0xFF); + /* Use provided chip-select for ancillary device */ spi_set_chipselect(ancillary, 0, chip_select); @@ -2732,6 +2740,7 @@ struct spi_device *acpi_spi_device_alloc(struct spi_controller *ctlr, struct acpi_spi_lookup lookup = {}; struct spi_device *spi; int ret; + u8 idx; if (!ctlr && index == -1) return ERR_PTR(-EINVAL); @@ -2767,6 +2776,9 @@ struct spi_device *acpi_spi_device_alloc(struct spi_controller *ctlr, return ERR_PTR(-ENOMEM); } + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) + spi_set_chipselect(spi, idx, 0xFF); + ACPI_COMPANION_SET(&spi->dev, adev); spi->max_speed_hz = lookup.max_speed_hz; spi->mode |= lookup.mode; Regards, Amit > Thanks, > > Stefan > > >> Hi, > >> > >> I still see the same error: > >> > >> [ 2.748546] pxa2xx-spi pxa2xx-spi.6: chipselect 0 already in use > > Thank you for the quick testing. For further analysis I have > > incorporated additional debug prints on top of 1/8 patch. The > > corresponding diff is shared below. Kindly implement the specified > > changes, rerun your test and share the kernel logs. > > > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index > > ff66147ba95f..7f59ea81593d 100644 > > --- a/drivers/spi/spi.c > > +++ b/drivers/spi/spi.c > > @@ -618,8 +618,10 @@ static int spi_dev_check(struct device *dev, void > *data) > > if (spi->controller == new_spi->controller) { > > for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > > cs = spi_get_chipselect(spi, idx); > > + printk("%s() [%d] CS[%d] = [%d]\n", __func__, > > + __LINE__, idx, cs); > > for (nw_idx = 0; nw_idx < SPI_CS_CNT_MAX; nw_idx++) { > > - cs_nw = spi_get_chipselect(spi, nw_idx); > > + cs_nw = spi_get_chipselect(new_spi, nw_idx); > > + printk("%s() [%d] CS_NEW[%d] = > > + [%d]\n", __func__, __LINE__, nw_idx, cs_nw); > > if (cs != 0xFF && cs_nw != 0xFF && cs == cs_nw) { > > dev_err(dev, "chipselect %d already in use\n", > cs_nw); > > return -EBUSY; @@ -659,8 > > +661,10 @@ static int __spi_add_device(struct spi_device *spi) > > */ > > for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > > cs = spi_get_chipselect(spi, idx); > > + printk("%s() [%d] CS[%d] = [%d]\n", __func__, > > + __LINE__, idx, cs); > > for (nw_idx = idx + 1; nw_idx < SPI_CS_CNT_MAX; nw_idx++) { > > nw_cs = spi_get_chipselect(spi, nw_idx); > > + printk("%s() [%d] CS_NEW[%d] = [%d]\n", > > + __func__, __LINE__, nw_idx, nw_cs); > > if (cs != 0xFF && nw_cs != 0xFF && cs == nw_cs) { > > dev_err(dev, "chipselect %d already in use\n", nw_cs); > > return -EBUSY; @@ -2401,6 +2405,9 @@ > > static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi, > > for (idx = 0; idx < rc; idx++) > > spi_set_chipselect(spi, idx, cs[idx]); > > > > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > > + printk("%s() [%d] CS[%d] = [%d]\n", __func__, > > + __LINE__, idx, spi_get_chipselect(spi, idx)); > > + > > /* spi->chip_select[i] gives the corresponding physical CS for logical CS i > > * logical CS number is represented by setting the ith bit in spi- > >cs_index_mask > > * So, for example, if spi->cs_index_mask = 0x01 then logical > > CS number is 0 and > > > > Regards, > > Amit > > > >> Thanks, > >> > >> Stefan > >> > >>>>> + if (cs != 0xFF && cs_nw != 0xFF && cs > == > >>>> cs_nw) { > >>>>> + dev_err(dev, "chipselect %d > already in > >>>> use\n", cs_nw); > >>>>> + return -EBUSY; > >>>>> + } > >>>>> + } > >>>>> + } > >>>>> + } > >>>>> return 0; > >>>>> } > >>>>> > >>>>> @@ -629,13 +640,32 @@ static int __spi_add_device(struct > >>>>> spi_device > >> *spi) > >>>>> { > >>>>> struct spi_controller *ctlr = spi->controller; > >>>>> struct device *dev = ctlr->dev.parent; > >>>>> - int status; > >>>>> + int status, idx, nw_idx; > >>>>> + u8 cs, nw_cs; > >>>>> + > >>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > >>>>> + /* Chipselects are numbered 0..max; validate. */ > >>>>> + cs = spi_get_chipselect(spi, idx); > >>>>> + if (cs != 0xFF && cs >= ctlr->num_chipselect) { > >>>>> + dev_err(dev, "cs%d >= max %d\n", > >>>> spi_get_chipselect(spi, idx), > >>>>> + ctlr->num_chipselect); > >>>>> + return -EINVAL; > >>>>> + } > >>>>> + } > >>>>> > >>>>> - /* Chipselects are numbered 0..max; validate. */ > >>>>> - if (spi_get_chipselect(spi, 0) >= ctlr->num_chipselect) { > >>>>> - dev_err(dev, "cs%d >= max %d\n", spi_get_chipselect(spi, 0), > >>>>> - ctlr->num_chipselect); > >>>>> - return -EINVAL; > >>>>> + /* > >>>>> + * Make sure that multiple logical CS doesn't map to the same > >>>> physical CS. > >>>>> + * For example, spi->chip_select[0] != spi->chip_select[1] and > so on. > >>>>> + */ > >>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > >>>>> + cs = spi_get_chipselect(spi, idx); > >>>>> + for (nw_idx = idx + 1; nw_idx < SPI_CS_CNT_MAX; > nw_idx++) { > >>>>> + nw_cs = spi_get_chipselect(spi, nw_idx); > >>>>> + if (cs != 0xFF && nw_cs != 0xFF && cs == > nw_cs) { > >>>>> + dev_err(dev, "chipselect %d already in > use\n", > >>>> nw_cs); > >>>>> + return -EBUSY; > >>>>> + } > >>>>> + } > >>>>> } > >>>>> > >>>>> /* Set the bus ID string */ > >>>>> @@ -647,11 +677,8 @@ static int __spi_add_device(struct spi_device > >> *spi) > >>>>> * its configuration. > >>>>> */ > >>>>> status = bus_for_each_dev(&spi_bus_type, NULL, spi, > spi_dev_check); > >>>>> - if (status) { > >>>>> - dev_err(dev, "chipselect %d already in use\n", > >>>>> - spi_get_chipselect(spi, 0)); > >>>>> + if (status) > >>>>> return status; > >>>>> - } > >>>>> > >>>>> /* Controller may unregister concurrently */ > >>>>> if (IS_ENABLED(CONFIG_SPI_DYNAMIC) && @@ -659,8 > +686,15 @@ > >>>> static > >>>>> int __spi_add_device(struct spi_device *spi) > >>>>> return -ENODEV; > >>>>> } > >>>>> > >>>>> - if (ctlr->cs_gpiods) > >>>>> - spi_set_csgpiod(spi, 0, ctlr->cs_gpiods[spi_get_chipselect(spi, > >>>> 0)]); > >>>>> + if (ctlr->cs_gpiods) { > >>>>> + u8 cs; > >>>>> + > >>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > >>>>> + cs = spi_get_chipselect(spi, idx); > >>>>> + if (cs != 0xFF) > >>>>> + spi_set_csgpiod(spi, idx, ctlr- > >cs_gpiods[cs]); > >>>>> + } > >>>>> + } > >>>>> > >>>>> /* > >>>>> * Drivers may modify this initial i/o setup, but will @@ > >>>>> -701,6 > >>>>> +735,9 @@ int spi_add_device(struct spi_device *spi) > >>>>> struct spi_controller *ctlr = spi->controller; > >>>>> int status; > >>>>> > >>>>> + /* Set the bus ID string */ > >>>>> + spi_dev_set_name(spi); > >>>>> + > >>>>> mutex_lock(&ctlr->add_lock); > >>>>> status = __spi_add_device(spi); > >>>>> mutex_unlock(&ctlr->add_lock); @@ -942,32 +979,51 @@ > static > >>>>> void spi_res_release(struct spi_controller > >>>> *ctlr, struct spi_message *mes > >>>>> } > >>>>> > >>>>> > >>>>> /*---------------------------------------------------------------- > >>>>> -- > >>>>> -- > >>>>> -----*/ > >>>>> +static inline bool spi_is_last_cs(struct spi_device *spi) { > >>>>> + u8 idx; > >>>>> + bool last = false; > >>>>> + > >>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > >>>>> + if ((spi->cs_index_mask >> idx) & 0x01) { > >>>>> + if (spi->controller->last_cs[idx] == > >>>> spi_get_chipselect(spi, idx)) > >>>>> + last = true; > >>>>> + } > >>>>> + } > >>>>> + return last; > >>>>> +} > >>>>> + > >>>>> > >>>>> static void spi_set_cs(struct spi_device *spi, bool enable, bool force) > >>>>> { > >>>>> bool activate = enable; > >>>>> + u8 idx; > >>>>> > >>>>> /* > >>>>> * Avoid calling into the driver (or doing delays) if the chip > select > >>>>> * isn't actually changing from the last time this was called. > >>>>> */ > >>>>> - if (!force && ((enable && spi->controller->last_cs == > >>>> spi_get_chipselect(spi, 0)) || > >>>>> - (!enable && spi->controller->last_cs != > >>>> spi_get_chipselect(spi, 0))) && > >>>>> + if (!force && ((enable && spi->controller->last_cs_index_mask > == > >>>>> +spi- > >>>>> cs_index_mask && > >>>>> + spi_is_last_cs(spi)) || > >>>>> + (!enable && spi->controller->last_cs_index_mask > == spi- > >>>>> cs_index_mask && > >>>>> + !spi_is_last_cs(spi))) && > >>>>> (spi->controller->last_cs_mode_high == (spi->mode & > >>>> SPI_CS_HIGH))) > >>>>> return; > >>>>> > >>>>> trace_spi_set_cs(spi, activate); > >>>>> > >>>>> - spi->controller->last_cs = enable ? spi_get_chipselect(spi, 0) : -1; > >>>>> + spi->controller->last_cs_index_mask = spi->cs_index_mask; > >>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > >>>>> + spi->controller->last_cs[idx] = enable ? > >>>>> +spi_get_chipselect(spi, > >>>> 0) > >>>>> +: -1; > >>>>> spi->controller->last_cs_mode_high = spi->mode & > SPI_CS_HIGH; > >>>>> > >>>>> - if ((spi_get_csgpiod(spi, 0) || !spi->controller->set_cs_timing) && > >>>> !activate) > >>>>> - spi_delay_exec(&spi->cs_hold, NULL); > >>>>> - > >>>>> if (spi->mode & SPI_CS_HIGH) > >>>>> enable = !enable; > >>>>> > >>>>> - if (spi_get_csgpiod(spi, 0)) { > >>>>> + if (spi_is_csgpiod(spi)) { > >>>>> + if (!spi->controller->set_cs_timing && !activate) > >>>>> + spi_delay_exec(&spi->cs_hold, NULL); > >>>>> + > >>>>> if (!(spi->mode & SPI_NO_CS)) { > >>>>> /* > >>>>> * Historically ACPI has no means of the GPIO > polarity > >>>> and @@ > >>>>> -979,26 +1035,38 @@ static void spi_set_cs(struct spi_device *spi, > >>>>> bool > >>>> enable, bool force) > >>>>> * ambiguity. That's why we use enable, that > takes > >>>> SPI_CS_HIGH > >>>>> * into account. > >>>>> */ > >>>>> - if (has_acpi_companion(&spi->dev)) > >>>>> - > >>>> gpiod_set_value_cansleep(spi_get_csgpiod(spi, 0), !enable); > >>>>> - else > >>>>> - /* Polarity handled by GPIO library */ > >>>>> - > >>>> gpiod_set_value_cansleep(spi_get_csgpiod(spi, 0), activate); > >>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > >>>>> + if (((spi->cs_index_mask >> idx) & > 0x01) && > >>>>> + spi_get_csgpiod(spi, idx)) { > >>>>> + if (has_acpi_companion(&spi- > >dev)) > >>>>> + > >>>> gpiod_set_value_cansleep(spi_get_csgpiod(spi, idx), > >>>>> + > >>>> !enable); > >>>>> + else > >>>>> + /* Polarity handled by > GPIO > >>>> library */ > >>>>> + > >>>> gpiod_set_value_cansleep(spi_get_csgpiod(spi, idx), > >>>>> + > >>>> activate); > >>>>> + > >>>>> + if (activate) > >>>>> + spi_delay_exec(&spi- > >>>>> cs_setup, NULL); > >>>>> + else > >>>>> + spi_delay_exec(&spi- > >>>>> cs_inactive, NULL); > >>>>> + } > >>>>> + } > >>>>> } > >>>>> /* Some SPI masters need both GPIO CS & > slave_select */ > >>>>> if ((spi->controller->flags & > SPI_CONTROLLER_GPIO_SS) && > >>>>> spi->controller->set_cs) > >>>>> spi->controller->set_cs(spi, !enable); > >>>>> + > >>>>> + if (!spi->controller->set_cs_timing) { > >>>>> + if (activate) > >>>>> + spi_delay_exec(&spi->cs_setup, > NULL); > >>>>> + else > >>>>> + spi_delay_exec(&spi->cs_inactive, > NULL); > >>>>> + } > >>>>> } else if (spi->controller->set_cs) { > >>>>> spi->controller->set_cs(spi, !enable); > >>>>> } > >>>>> - > >>>>> - if (spi_get_csgpiod(spi, 0) || !spi->controller->set_cs_timing) { > >>>>> - if (activate) > >>>>> - spi_delay_exec(&spi->cs_setup, NULL); > >>>>> - else > >>>>> - spi_delay_exec(&spi->cs_inactive, NULL); > >>>>> - } > >>>>> } > >>>>> > >>>>> #ifdef CONFIG_HAS_DMA > >>>>> @@ -2222,8 +2290,8 @@ static void of_spi_parse_dt_cs_delay(struct > >>>> device_node *nc, > >>>>> static int of_spi_parse_dt(struct spi_controller *ctlr, struct > >>>>> spi_device > >> *spi, > >>>>> struct device_node *nc) > >>>>> { > >>>>> - u32 value; > >>>>> - int rc; > >>>>> + u32 value, cs[SPI_CS_CNT_MAX]; > >>>>> + int rc, idx; > >>>>> > >>>>> /* Mode (clock phase/polarity/etc.) */ > >>>>> if (of_property_read_bool(nc, "spi-cpha")) @@ -2295,14 > >>>>> +2363,52 > >>>> @@ > >>>>> static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device > *spi, > >>>>> return 0; > >>>>> } > >>>>> > >>>>> + if (ctlr->num_chipselect > SPI_CS_CNT_MAX) { > >>>>> + dev_err(&ctlr->dev, "No. of CS is more than max. no. > of > >>>> supported CS\n"); > >>>>> + return -EINVAL; > >>>>> + } > >>>>> + > >>>>> + /* > >>>>> + * Zero(0) is a valid physical CS value and can be located at > any > >>>>> + * logical CS in the spi->chip_select[]. If all the physical CS > >>>>> + * are initialized to 0 then It would be difficult to differentiate > >>>>> + * between a valid physical CS 0 & an unused logical CS whose > >>>> physical > >>>>> + * CS can be 0. As a solution to this issue initialize all the CS to > 0xFF. > >>>>> + * Now all the unused logical CS will have 0xFF physical CS > >>>>> +value & can > >>>> be > >>>>> + * ignore while performing physical CS validity checks. > >>>>> + */ > >>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > >>>>> + spi_set_chipselect(spi, idx, 0xFF); > >>>>> + > >>>>> /* Device address */ > >>>>> - rc = of_property_read_u32(nc, "reg", &value); > >>>>> - if (rc) { > >>>>> + rc = of_property_read_variable_u32_array(nc, "reg", &cs[0], > 1, > >>>>> + SPI_CS_CNT_MAX); > >>>>> + if (rc < 0) { > >>>>> dev_err(&ctlr->dev, "%pOF has no valid 'reg' property > >>>> (%d)\n", > >>>>> nc, rc); > >>>>> return rc; > >>>>> } > >>>>> - spi_set_chipselect(spi, 0, value); > >>>>> + if (rc > ctlr->num_chipselect) { > >>>>> + dev_err(&ctlr->dev, "%pOF has number of CS > ctlr- > >>>>> num_chipselect (%d)\n", > >>>>> + nc, rc); > >>>>> + return rc; > >>>>> + } > >>>>> + if ((of_property_read_bool(nc, "parallel-memories")) && > >>>>> + (!(ctlr->flags & SPI_CONTROLLER_MULTI_CS))) { > >>>>> + dev_err(&ctlr->dev, "SPI controller doesn't support > multi > >>>> CS\n"); > >>>>> + return -EINVAL; > >>>>> + } > >>>>> + for (idx = 0; idx < rc; idx++) > >>>>> + spi_set_chipselect(spi, idx, cs[idx]); > >>>>> + > >>>>> + /* spi->chip_select[i] gives the corresponding physical CS for > >>>>> +logical CS > >>>> i > >>>>> + * logical CS number is represented by setting the ith bit in > >>>>> +spi- > >>>>> cs_index_mask > >>>>> + * So, for example, if spi->cs_index_mask = 0x01 then logical > CS > >>>> number is 0 and > >>>>> + * spi->chip_select[0] will give the physical CS. > >>>>> + * By default spi->chip_select[0] will hold the physical CS > >>>>> +number so, > >>>> set > >>>>> + * spi->cs_index_mask as 0x01. > >>>>> + */ > >>>>> + spi->cs_index_mask = 0x01; > >>>>> > >>>>> /* Device speed */ > >>>>> if (!of_property_read_u32(nc, "spi-max-frequency", &value)) > >>>>> @@ > >>>>> -3100,6 +3206,7 @@ int spi_register_controller(struct spi_controller > *ctlr) > >>>>> struct boardinfo *bi; > >>>>> int first_dynamic; > >>>>> int status; > >>>>> + int idx; > >>>>> > >>>>> if (!dev) > >>>>> return -ENODEV; > >>>>> @@ -3164,7 +3271,8 @@ int spi_register_controller(struct > >>>>> spi_controller > >>>> *ctlr) > >>>>> } > >>>>> > >>>>> /* Setting last_cs to -1 means no chip selected */ > >>>>> - ctlr->last_cs = -1; > >>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > >>>>> + ctlr->last_cs[idx] = -1; > >>>>> > >>>>> status = device_add(&ctlr->dev); > >>>>> if (status < 0) > >>>>> @@ -3889,7 +3997,7 @@ static int __spi_validate(struct spi_device > >>>>> *spi, > >>>> struct spi_message *message) > >>>>> * cs_change is set for each transfer. > >>>>> */ > >>>>> if ((spi->mode & SPI_CS_WORD) && (!(ctlr->mode_bits & > >>>> SPI_CS_WORD) || > >>>>> - spi_get_csgpiod(spi, 0))) { > >>>>> + spi_is_csgpiod(spi))) { > >>>>> size_t maxsize = BITS_TO_BYTES(spi->bits_per_word); > >>>>> int ret; > >>>>> > >>>>> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h > >>>>> index 7b4baff63c5c..871d3a6d879b 100644 > >>>>> --- a/include/linux/spi/spi.h > >>>>> +++ b/include/linux/spi/spi.h > >>>>> @@ -20,6 +20,9 @@ > >>>>> > >>>>> #include <uapi/linux/spi/spi.h> > >>>>> > >>>>> +/* Max no. of CS supported per spi device */ #define > >>>>> +SPI_CS_CNT_MAX > >>>>> +4 > >>>>> + > >>>>> struct dma_chan; > >>>>> struct software_node; > >>>>> struct ptp_system_timestamp; > >>>>> @@ -132,7 +135,8 @@ extern void > >>>> spi_transfer_cs_change_delay_exec(struct spi_message *msg, > >>>>> * @max_speed_hz: Maximum clock rate to be used with this chip > >>>>> * (on this board); may be changed by the device's driver. > >>>>> * The spi_transfer.speed_hz can override this for each transfer. > >>>>> - * @chip_select: Chipselect, distinguishing chips handled by > @controller. > >>>>> + * @chip_select: Array of physical chipselect, spi->chipselect[i] gives > >>>>> + * the corresponding physical CS for logical CS i. > >>>>> * @mode: The spi mode defines how data is clocked out and in. > >>>>> * This may be changed by the device's driver. > >>>>> * The "active low" default for chipselect mode can be > overridden > >>>>> @@ -157,8 +161,8 @@ extern void > >>>> spi_transfer_cs_change_delay_exec(struct spi_message *msg, > >>>>> * the device will bind to the named driver and only the named > driver. > >>>>> * Do not set directly, because core frees it; use > driver_set_override() to > >>>>> * set or clear it. > >>>>> - * @cs_gpiod: GPIO descriptor of the chipselect line (optional, > >>>>> NULL > >> when > >>>>> - * not using a GPIO line) > >>>>> + * @cs_gpiod: Array of GPIO descriptors of the corresponding > >>>>> + chipselect > >>>> lines > >>>>> + * (optional, NULL when not using a GPIO line) > >>>>> * @word_delay: delay to be inserted between consecutive > >>>>> * words of a transfer > >>>>> * @cs_setup: delay to be introduced by the controller after > >>>>> CS is asserted @@ -167,6 +171,7 @@ extern void > >>>> spi_transfer_cs_change_delay_exec(struct spi_message *msg, > >>>>> * deasserted. If @cs_change_delay is used from @spi_transfer, > then > >>>> the > >>>>> * two delays will be added up. > >>>>> * @pcpu_statistics: statistics for the spi_device > >>>>> + * @cs_index_mask: Bit mask of the active chipselect(s) in the > >>>>> + chipselect array > >>>>> * > >>>>> * A @spi_device is used to interchange data between an SPI slave > >>>>> * (usually a discrete chip) and CPU memory. > >>>>> @@ -182,7 +187,7 @@ struct spi_device { > >>>>> struct spi_controller *controller; > >>>>> struct spi_controller *master; /* Compatibility layer > */ > >>>>> u32 max_speed_hz; > >>>>> - u8 chip_select; > >>>>> + u8 chip_select[SPI_CS_CNT_MAX]; > >>>>> u8 bits_per_word; > >>>>> bool rt; > >>>>> #define SPI_NO_TX BIT(31) /* No transmit wire */ > >>>>> @@ -213,7 +218,7 @@ struct spi_device { > >>>>> void *controller_data; > >>>>> char modalias[SPI_NAME_SIZE]; > >>>>> const char *driver_override; > >>>>> - struct gpio_desc *cs_gpiod; /* Chip select GPIO descriptor > >>>> */ > >>>>> + struct gpio_desc *cs_gpiod[SPI_CS_CNT_MAX]; /* > Chip select > >>>> gpio desc */ > >>>>> struct spi_delay word_delay; /* Inter-word delay */ > >>>>> /* CS delays */ > >>>>> struct spi_delay cs_setup; > >>>>> @@ -223,6 +228,13 @@ struct spi_device { > >>>>> /* The statistics */ > >>>>> struct spi_statistics __percpu *pcpu_statistics; > >>>>> > >>>>> + /* Bit mask of the chipselect(s) that the driver need to use > from > >>>>> + * the chipselect array.When the controller is capable to > handle > >>>>> + * multiple chip selects & memories are connected in parallel > >>>>> + * then more than one bit need to be set in cs_index_mask. > >>>>> + */ > >>>>> + u32 cs_index_mask : SPI_CS_CNT_MAX; > >>>>> + > >>>>> /* > >>>>> * Likely need more hooks for more protocol options affecting > how > >>>>> * the controller talks to each chip, like: > >>>>> @@ -279,22 +291,33 @@ static inline void *spi_get_drvdata(const > >>>>> struct spi_device *spi) > >>>>> > >>>>> static inline u8 spi_get_chipselect(const struct spi_device *spi, u8 idx) > >>>>> { > >>>>> - return spi->chip_select; > >>>>> + return spi->chip_select[idx]; > >>>>> } > >>>>> > >>>>> static inline void spi_set_chipselect(struct spi_device *spi, > >>>>> u8 idx, u8 > >>>> chipselect) > >>>>> { > >>>>> - spi->chip_select = chipselect; > >>>>> + spi->chip_select[idx] = chipselect; > >>>>> } > >>>>> > >>>>> static inline struct gpio_desc *spi_get_csgpiod(const struct > >>>>> spi_device *spi, > >>>> u8 idx) > >>>>> { > >>>>> - return spi->cs_gpiod; > >>>>> + return spi->cs_gpiod[idx]; > >>>>> } > >>>>> > >>>>> static inline void spi_set_csgpiod(struct spi_device *spi, u8 > >>>>> idx, struct > >>>> gpio_desc *csgpiod) > >>>>> { > >>>>> - spi->cs_gpiod = csgpiod; > >>>>> + spi->cs_gpiod[idx] = csgpiod; > >>>>> +} > >>>>> + > >>>>> +static inline bool spi_is_csgpiod(struct spi_device *spi) { > >>>>> + u8 idx; > >>>>> + > >>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > >>>>> + if (spi_get_csgpiod(spi, idx)) > >>>>> + return true; > >>>>> + } > >>>>> + return false; > >>>>> } > >>>>> > >>>>> /** > >>>>> @@ -399,6 +422,8 @@ extern struct spi_device > >>>> *spi_new_ancillary_device(struct spi_device *spi, u8 ch > >>>>> * @bus_lock_spinlock: spinlock for SPI bus locking > >>>>> * @bus_lock_mutex: mutex for exclusion of multiple callers > >>>>> * @bus_lock_flag: indicates that the SPI bus is locked for > >>>>> exclusive use > >>>>> + * @multi_cs_cap: indicates that the SPI Controller can assert/de- > assert > >>>>> + * more than one chip select at once. > >>>>> * @setup: updates the device mode and clocking records used by a > >>>>> * device's SPI controller; protocol code may call this. This > >>>>> * must fail if an unrecognized or unsupported mode is > requested. > >>>>> @@ -567,6 +592,11 @@ struct spi_controller { > >>>>> #define SPI_CONTROLLER_MUST_TX BIT(4) /* Requires tx > >> */ > >>>>> #define SPI_CONTROLLER_GPIO_SS BIT(5) /* GPIO CS > >> must > >>>> select slave */ > >>>>> #define SPI_CONTROLLER_SUSPENDED BIT(6) /* Currently > >>>> suspended */ > >>>>> + /* > >>>>> + * The spi-controller has multi chip select capability and can > >>>>> + * assert/de-assert more than one chip select at once. > >>>>> + */ > >>>>> +#define SPI_CONTROLLER_MULTI_CS BIT(7) > >>>>> > >>>>> /* Flag indicating if the allocation of this struct is devres- > managed */ > >>>>> bool devm_allocated; > >>>>> @@ -677,7 +707,8 @@ struct spi_controller { > >>>>> bool rt; > >>>>> bool auto_runtime_pm; > >>>>> bool cur_msg_mapped; > >>>>> - char last_cs; > >>>>> + char last_cs[SPI_CS_CNT_MAX]; > >>>>> + char last_cs_index_mask; > >>>>> bool last_cs_mode_high; > >>>>> bool fallback; > >>>>> struct completion xfer_completion;
On 21/11/2023 19:18, Mahapatra, Amit Kumar wrote: > Hello Stefan, > >> -----Original Message----- >> From: Stefan Binding <sbinding@opensource.cirrus.com> >> Sent: Tuesday, November 21, 2023 11:07 PM >> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; >> broonie@kernel.org; tudor.ambarus@linaro.org; pratyush@kernel.org; >> miquel.raynal@bootlin.com; richard@nod.at; vigneshr@ti.com >> Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; >> michael@walle.cc; linux-mtd@lists.infradead.org; >> nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com; >> claudiu.beznea@tuxon.dev; Simek, Michal <michal.simek@amd.com>; linux- >> arm-kernel@lists.infradead.org; git (AMD-Xilinx) <git@amd.com>; >> amitrkcian2002@gmail.com; patches@opensource.cirrus.com >> Subject: Re: [PATCH v10 1/8] spi: Add multi-cs memories support in SPI core >> >> >> On 21/11/2023 16:35, Mahapatra, Amit Kumar wrote: >>> Hello Stefan, >>> >>>> -----Original Message----- >>>> From: Stefan Binding <sbinding@opensource.cirrus.com> >>>> Sent: Tuesday, November 21, 2023 7:28 PM >>>> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; >>>> broonie@kernel.org; tudor.ambarus@linaro.org; pratyush@kernel.org; >>>> miquel.raynal@bootlin.com; richard@nod.at; vigneshr@ti.com >>>> Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; >>>> michael@walle.cc; linux-mtd@lists.infradead.org; >>>> nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com; >>>> claudiu.beznea@tuxon.dev; Simek, Michal <michal.simek@amd.com>; >>>> linux- arm-kernel@lists.infradead.org; git (AMD-Xilinx) >>>> <git@amd.com>; amitrkcian2002@gmail.com; >>>> patches@opensource.cirrus.com >>>> Subject: Re: [PATCH v10 1/8] spi: Add multi-cs memories support in >>>> SPI core >>>> >>>> >>>> On 21/11/2023 07:39, Mahapatra, Amit Kumar wrote: >>>>> Hello Stefan, >>>>> >>>>>> -----Original Message----- >>>>>> From: Stefan Binding <sbinding@opensource.cirrus.com> >>>>>> Sent: Monday, November 20, 2023 8:00 PM >>>>>> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; >>>>>> broonie@kernel.org; tudor.ambarus@linaro.org; pratyush@kernel.org; >>>>>> miquel.raynal@bootlin.com; richard@nod.at; vigneshr@ti.com >>>>>> Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; >>>>>> michael@walle.cc; linux-mtd@lists.infradead.org; >>>>>> nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com; >>>>>> claudiu.beznea@tuxon.dev; Simek, Michal <michal.simek@amd.com>; >>>>>> linux- arm-kernel@lists.infradead.org; git (AMD-Xilinx) >>>>>> <git@amd.com>; amitrkcian2002@gmail.com; >>>>>> patches@opensource.cirrus.com >>>>>> Subject: Re: [PATCH v10 1/8] spi: Add multi-cs memories support in >>>>>> SPI core >>>>>> >>>>>> >>>>>> On 18/11/2023 13:54, Amit Kumar Mahapatra wrote: >>>>>>> AMD-Xilinx GQSPI controller has two advanced mode that allows the >>>>>>> controller to consider two flashes as one single device. >>>>>>> >>>>>>> One of these two mode is the parallel mode in which each byte of >>>>>>> data is stored in both devices, the even bits in the lower flash & >>>>>>> the odd bits in the upper flash. The byte split is automatically >>>>>>> handled by the QSPI controller. >>>>>>> >>>>>>> The other mode is the stacked mode in which both the flashes share >>>>>>> the same SPI bus but each of the device contain half of the data. >>>>>>> In this mode, the controller does not follow CS requests but >>>>>>> instead internally wires the two CS levels with the value of the >>>>>>> most significant >>>>>> address bit. >>>>>>> For supporting both these modes SPI core need to be updated for >>>>>>> providing multiple CS for a single SPI device. >>>>>>> >>>>>>> For adding multi CS support the SPI device need to be aware of all >>>>>>> the CS values. So, the "chip_select" member in the spi_device >>>>>>> structure is now an array that holds all the CS values. >>>>>>> >>>>>>> spi_device structure now has a "cs_index_mask" member. This acts >>>>>>> as an index to the chip_select array. If nth bit of >>>>>>> spi->cs_index_mask is set then the driver would assert spi- >>> chip_select[n]. >>>>>>> In parallel mode all the chip selects are asserted/de-asserted >>>>>>> simultaneously and each byte of data is stored in both devices, >>>>>>> the even bits in one, the odd bits in the other. The split is >>>>>>> automatically handled by the GQSPI controller. The GQSPI >>>>>>> controller supports a maximum of two flashes connected in parallel >>>>>>> mode. A SPI_CONTROLLER_MULTI_CS flag bit is added in the spi >>>>>>> controller flags, through ctlr->flags the spi core will make sure >>>>>>> that the controller is capable of handling multiple chip selects at once. >>>>>>> >>>>>>> For supporting multiple CS via GPIO the cs_gpiod member of the >>>>>>> spi_device structure is now an array that holds the gpio >>>>>>> descriptor for each chipselect. >>>>>>> >>>>>>> CS GPIO is not tested due to unavailability of necessary hardware >> setup. >>>>>>> Signed-off-by: Amit Kumar Mahapatra <amit.kumar- >>>> mahapatra@amd.com> >>>>>>> --- >>>>>>> Hello @Stefen, >>>>>>> We restructured the SPI core implementation, for handling >>>>>>> arbitrary number of devices connected in parallel(multi-cs) or stacked >> mode. >>>>>>> We have tested this updated patch on native-cs setup but couldn't >>>>>>> test cs-gpio due to unavailability of a proper setup. If possible, >>>>>>> could you please retest the cs-gpio use case with this updated >>>>>>> patch and provide >>>>>> your feedback. >>>>>> >>>>>> Hi, >>>>>> >>>>>> I tested this chain on 2 different systems using GPIOs as chip >>>>>> selects, and see the same error on both: >>>>>> [ 2.842045] pxa2xx-spi pxa2xx-spi.6: chipselect 0 already in use >>>>>> >>>>>> Let me know if you need any further testing. >>>>>> >>>>>> Thanks, >>>>>> >>>>>> Stefan Binding >>>>>> >>>>>>> --- >>>>>>> drivers/spi/spi.c | 192 +++++++++++++++++++++++++++++++------- >> -- >>>>>>> include/linux/spi/spi.h | 51 ++++++++--- >>>>>>> 2 files changed, 191 insertions(+), 52 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index >>>>>>> 8ead7acb99f3..ff66147ba95f 100644 >>>>>>> --- a/drivers/spi/spi.c >>>>>>> +++ b/drivers/spi/spi.c >>>>>>> @@ -612,10 +612,21 @@ static int spi_dev_check(struct device *dev, >>>>>>> void >>>>>> *data) >>>>>>> { >>>>>>> struct spi_device *spi = to_spi_device(dev); >>>>>>> struct spi_device *new_spi = data; >>>>>>> - >>>>>>> - if (spi->controller == new_spi->controller && >>>>>>> - spi_get_chipselect(spi, 0) == spi_get_chipselect(new_spi, 0)) >>>>>>> - return -EBUSY; >>>>>>> + int idx, nw_idx; >>>>>>> + u8 cs, cs_nw; >>>>>>> + >>>>>>> + if (spi->controller == new_spi->controller) { >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { >>>>>>> + cs = spi_get_chipselect(spi, idx); >>>>>>> + for (nw_idx = 0; nw_idx < SPI_CS_CNT_MAX; >>>>>> nw_idx++) { >>>>>>> + cs_nw = spi_get_chipselect(spi, >> nw_idx); >>>>> Thank you for dedicating your time to test my patch. >>>>> As per my analysis the error is reported from here as we are >>>>> supplying the former SPI device structure to retrieve the CS value >>>>> for the new SPI devices. >>>>> To fix this, could you kindly substitute the above line with >>>>> >>>>> cs_nw = spi_get_chipselect(new_spi, nw_idx); >>>>> >>>>> and rerun your tests? >>>>> If it works correctly, I will incorporate this fix into my upcoming >>>>> series. >>>>> >>>>> Regards, >>>>> Amit >> Hi, >> >> I've attached my log. >> I notice that you add a print to of_spi_parse_dt, however since the laptop I >> am using is an x86 laptop, it uses ACPI rather than OF, and I don't think this >> function isnt even compiled in. > That’s correct, I missed it. > Upon reviewing the logs, I discovered that in the ACPI flow, I am not > initializing the unused CS[] to FF, as I am doing in the OF flow. > Consequently, in the ACPI flow, all the unused CS[] are automatically > initialized to 0. During the __spi_add_device process, the SPI core faces > difficulty distinguishing between a valid CS value of 0 and an unused CS > value of 0, leading to a failure in the driver probe. > > Incorporating the following modifications should resolve this issue. > Kindly apply these changes on top of 1/8 and re-run the test. > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index ff66147ba95f..d0301c17d4f8 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -619,7 +619,7 @@ static int spi_dev_check(struct device *dev, void *data) > for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > cs = spi_get_chipselect(spi, idx); > for (nw_idx = 0; nw_idx < SPI_CS_CNT_MAX; nw_idx++) { > - cs_nw = spi_get_chipselect(spi, nw_idx); > + cs_nw = spi_get_chipselect(new_spi, nw_idx); > if (cs != 0xFF && cs_nw != 0xFF && cs == cs_nw) { > dev_err(dev, "chipselect %d already in use\n", cs_nw); > return -EBUSY; > @@ -764,6 +764,7 @@ struct spi_device *spi_new_device(struct spi_controller *ctlr, > { > struct spi_device *proxy; > int status; > + u8 idx; > > /* > * NOTE: caller did any chip->bus_num checks necessary. > @@ -779,6 +780,9 @@ struct spi_device *spi_new_device(struct spi_controller *ctlr, > > WARN_ON(strlen(chip->modalias) >= sizeof(proxy->modalias)); > > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > + spi_set_chipselect(proxy, idx, 0xFF); > + > spi_set_chipselect(proxy, 0, chip->chip_select); > proxy->max_speed_hz = chip->max_speed_hz; > proxy->mode = chip->mode; > @@ -2514,6 +2518,7 @@ struct spi_device *spi_new_ancillary_device(struct spi_device *spi, > struct spi_controller *ctlr = spi->controller; > struct spi_device *ancillary; > int rc = 0; > + u8 idx; > > /* Alloc an spi_device */ > ancillary = spi_alloc_device(ctlr); > @@ -2524,6 +2529,9 @@ struct spi_device *spi_new_ancillary_device(struct spi_device *spi, > > strscpy(ancillary->modalias, "dummy", sizeof(ancillary->modalias)); > > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > + spi_set_chipselect(ancillary, idx, 0xFF); > + > /* Use provided chip-select for ancillary device */ > spi_set_chipselect(ancillary, 0, chip_select); > > @@ -2732,6 +2740,7 @@ struct spi_device *acpi_spi_device_alloc(struct spi_controller *ctlr, > struct acpi_spi_lookup lookup = {}; > struct spi_device *spi; > int ret; > + u8 idx; > > if (!ctlr && index == -1) > return ERR_PTR(-EINVAL); > @@ -2767,6 +2776,9 @@ struct spi_device *acpi_spi_device_alloc(struct spi_controller *ctlr, > return ERR_PTR(-ENOMEM); > } > > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > + spi_set_chipselect(spi, idx, 0xFF); > + > ACPI_COMPANION_SET(&spi->dev, adev); > spi->max_speed_hz = lookup.max_speed_hz; > spi->mode |= lookup.mode; > > Regards, > Amit > Hi, I no longer see that error, or any errors printed from SPI, however, all of the transactions are now broken. Every transaction seems to read back 0x0, on all SPI devices, whether native CS or GPIO CS, and I dont see it toggling the CS GPIO anymore. Thanks, Stefan >> Thanks, >> >> Stefan >> >>>> Hi, >>>> >>>> I still see the same error: >>>> >>>> [ 2.748546] pxa2xx-spi pxa2xx-spi.6: chipselect 0 already in use >>> Thank you for the quick testing. For further analysis I have >>> incorporated additional debug prints on top of 1/8 patch. The >>> corresponding diff is shared below. Kindly implement the specified >>> changes, rerun your test and share the kernel logs. >>> >>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index >>> ff66147ba95f..7f59ea81593d 100644 >>> --- a/drivers/spi/spi.c >>> +++ b/drivers/spi/spi.c >>> @@ -618,8 +618,10 @@ static int spi_dev_check(struct device *dev, void >> *data) >>> if (spi->controller == new_spi->controller) { >>> for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { >>> cs = spi_get_chipselect(spi, idx); >>> + printk("%s() [%d] CS[%d] = [%d]\n", __func__, >>> + __LINE__, idx, cs); >>> for (nw_idx = 0; nw_idx < SPI_CS_CNT_MAX; nw_idx++) { >>> - cs_nw = spi_get_chipselect(spi, nw_idx); >>> + cs_nw = spi_get_chipselect(new_spi, nw_idx); >>> + printk("%s() [%d] CS_NEW[%d] = >>> + [%d]\n", __func__, __LINE__, nw_idx, cs_nw); >>> if (cs != 0xFF && cs_nw != 0xFF && cs == cs_nw) { >>> dev_err(dev, "chipselect %d already in use\n", >> cs_nw); >>> return -EBUSY; @@ -659,8 >>> +661,10 @@ static int __spi_add_device(struct spi_device *spi) >>> */ >>> for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { >>> cs = spi_get_chipselect(spi, idx); >>> + printk("%s() [%d] CS[%d] = [%d]\n", __func__, >>> + __LINE__, idx, cs); >>> for (nw_idx = idx + 1; nw_idx < SPI_CS_CNT_MAX; nw_idx++) { >>> nw_cs = spi_get_chipselect(spi, nw_idx); >>> + printk("%s() [%d] CS_NEW[%d] = [%d]\n", >>> + __func__, __LINE__, nw_idx, nw_cs); >>> if (cs != 0xFF && nw_cs != 0xFF && cs == nw_cs) { >>> dev_err(dev, "chipselect %d already in use\n", nw_cs); >>> return -EBUSY; @@ -2401,6 +2405,9 @@ >>> static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi, >>> for (idx = 0; idx < rc; idx++) >>> spi_set_chipselect(spi, idx, cs[idx]); >>> >>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) >>> + printk("%s() [%d] CS[%d] = [%d]\n", __func__, >>> + __LINE__, idx, spi_get_chipselect(spi, idx)); >>> + >>> /* spi->chip_select[i] gives the corresponding physical CS for logical CS i >>> * logical CS number is represented by setting the ith bit in spi- >>> cs_index_mask >>> * So, for example, if spi->cs_index_mask = 0x01 then logical >>> CS number is 0 and >>> >>> Regards, >>> Amit >>> >>>> Thanks, >>>> >>>> Stefan >>>> >>>>>>> + if (cs != 0xFF && cs_nw != 0xFF && cs >> == >>>>>> cs_nw) { >>>>>>> + dev_err(dev, "chipselect %d >> already in >>>>>> use\n", cs_nw); >>>>>>> + return -EBUSY; >>>>>>> + } >>>>>>> + } >>>>>>> + } >>>>>>> + } >>>>>>> return 0; >>>>>>> } >>>>>>> >>>>>>> @@ -629,13 +640,32 @@ static int __spi_add_device(struct >>>>>>> spi_device >>>> *spi) >>>>>>> { >>>>>>> struct spi_controller *ctlr = spi->controller; >>>>>>> struct device *dev = ctlr->dev.parent; >>>>>>> - int status; >>>>>>> + int status, idx, nw_idx; >>>>>>> + u8 cs, nw_cs; >>>>>>> + >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { >>>>>>> + /* Chipselects are numbered 0..max; validate. */ >>>>>>> + cs = spi_get_chipselect(spi, idx); >>>>>>> + if (cs != 0xFF && cs >= ctlr->num_chipselect) { >>>>>>> + dev_err(dev, "cs%d >= max %d\n", >>>>>> spi_get_chipselect(spi, idx), >>>>>>> + ctlr->num_chipselect); >>>>>>> + return -EINVAL; >>>>>>> + } >>>>>>> + } >>>>>>> >>>>>>> - /* Chipselects are numbered 0..max; validate. */ >>>>>>> - if (spi_get_chipselect(spi, 0) >= ctlr->num_chipselect) { >>>>>>> - dev_err(dev, "cs%d >= max %d\n", spi_get_chipselect(spi, 0), >>>>>>> - ctlr->num_chipselect); >>>>>>> - return -EINVAL; >>>>>>> + /* >>>>>>> + * Make sure that multiple logical CS doesn't map to the same >>>>>> physical CS. >>>>>>> + * For example, spi->chip_select[0] != spi->chip_select[1] and >> so on. >>>>>>> + */ >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { >>>>>>> + cs = spi_get_chipselect(spi, idx); >>>>>>> + for (nw_idx = idx + 1; nw_idx < SPI_CS_CNT_MAX; >> nw_idx++) { >>>>>>> + nw_cs = spi_get_chipselect(spi, nw_idx); >>>>>>> + if (cs != 0xFF && nw_cs != 0xFF && cs == >> nw_cs) { >>>>>>> + dev_err(dev, "chipselect %d already in >> use\n", >>>>>> nw_cs); >>>>>>> + return -EBUSY; >>>>>>> + } >>>>>>> + } >>>>>>> } >>>>>>> >>>>>>> /* Set the bus ID string */ >>>>>>> @@ -647,11 +677,8 @@ static int __spi_add_device(struct spi_device >>>> *spi) >>>>>>> * its configuration. >>>>>>> */ >>>>>>> status = bus_for_each_dev(&spi_bus_type, NULL, spi, >> spi_dev_check); >>>>>>> - if (status) { >>>>>>> - dev_err(dev, "chipselect %d already in use\n", >>>>>>> - spi_get_chipselect(spi, 0)); >>>>>>> + if (status) >>>>>>> return status; >>>>>>> - } >>>>>>> >>>>>>> /* Controller may unregister concurrently */ >>>>>>> if (IS_ENABLED(CONFIG_SPI_DYNAMIC) && @@ -659,8 >> +686,15 @@ >>>>>> static >>>>>>> int __spi_add_device(struct spi_device *spi) >>>>>>> return -ENODEV; >>>>>>> } >>>>>>> >>>>>>> - if (ctlr->cs_gpiods) >>>>>>> - spi_set_csgpiod(spi, 0, ctlr->cs_gpiods[spi_get_chipselect(spi, >>>>>> 0)]); >>>>>>> + if (ctlr->cs_gpiods) { >>>>>>> + u8 cs; >>>>>>> + >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { >>>>>>> + cs = spi_get_chipselect(spi, idx); >>>>>>> + if (cs != 0xFF) >>>>>>> + spi_set_csgpiod(spi, idx, ctlr- >>> cs_gpiods[cs]); >>>>>>> + } >>>>>>> + } >>>>>>> >>>>>>> /* >>>>>>> * Drivers may modify this initial i/o setup, but will @@ >>>>>>> -701,6 >>>>>>> +735,9 @@ int spi_add_device(struct spi_device *spi) >>>>>>> struct spi_controller *ctlr = spi->controller; >>>>>>> int status; >>>>>>> >>>>>>> + /* Set the bus ID string */ >>>>>>> + spi_dev_set_name(spi); >>>>>>> + >>>>>>> mutex_lock(&ctlr->add_lock); >>>>>>> status = __spi_add_device(spi); >>>>>>> mutex_unlock(&ctlr->add_lock); @@ -942,32 +979,51 @@ >> static >>>>>>> void spi_res_release(struct spi_controller >>>>>> *ctlr, struct spi_message *mes >>>>>>> } >>>>>>> >>>>>>> >>>>>>> /*---------------------------------------------------------------- >>>>>>> -- >>>>>>> -- >>>>>>> -----*/ >>>>>>> +static inline bool spi_is_last_cs(struct spi_device *spi) { >>>>>>> + u8 idx; >>>>>>> + bool last = false; >>>>>>> + >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { >>>>>>> + if ((spi->cs_index_mask >> idx) & 0x01) { >>>>>>> + if (spi->controller->last_cs[idx] == >>>>>> spi_get_chipselect(spi, idx)) >>>>>>> + last = true; >>>>>>> + } >>>>>>> + } >>>>>>> + return last; >>>>>>> +} >>>>>>> + >>>>>>> >>>>>>> static void spi_set_cs(struct spi_device *spi, bool enable, bool force) >>>>>>> { >>>>>>> bool activate = enable; >>>>>>> + u8 idx; >>>>>>> >>>>>>> /* >>>>>>> * Avoid calling into the driver (or doing delays) if the chip >> select >>>>>>> * isn't actually changing from the last time this was called. >>>>>>> */ >>>>>>> - if (!force && ((enable && spi->controller->last_cs == >>>>>> spi_get_chipselect(spi, 0)) || >>>>>>> - (!enable && spi->controller->last_cs != >>>>>> spi_get_chipselect(spi, 0))) && >>>>>>> + if (!force && ((enable && spi->controller->last_cs_index_mask >> == >>>>>>> +spi- >>>>>>> cs_index_mask && >>>>>>> + spi_is_last_cs(spi)) || >>>>>>> + (!enable && spi->controller->last_cs_index_mask >> == spi- >>>>>>> cs_index_mask && >>>>>>> + !spi_is_last_cs(spi))) && >>>>>>> (spi->controller->last_cs_mode_high == (spi->mode & >>>>>> SPI_CS_HIGH))) >>>>>>> return; >>>>>>> >>>>>>> trace_spi_set_cs(spi, activate); >>>>>>> >>>>>>> - spi->controller->last_cs = enable ? spi_get_chipselect(spi, 0) : -1; >>>>>>> + spi->controller->last_cs_index_mask = spi->cs_index_mask; >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) >>>>>>> + spi->controller->last_cs[idx] = enable ? >>>>>>> +spi_get_chipselect(spi, >>>>>> 0) >>>>>>> +: -1; >>>>>>> spi->controller->last_cs_mode_high = spi->mode & >> SPI_CS_HIGH; >>>>>>> - if ((spi_get_csgpiod(spi, 0) || !spi->controller->set_cs_timing) && >>>>>> !activate) >>>>>>> - spi_delay_exec(&spi->cs_hold, NULL); >>>>>>> - >>>>>>> if (spi->mode & SPI_CS_HIGH) >>>>>>> enable = !enable; >>>>>>> >>>>>>> - if (spi_get_csgpiod(spi, 0)) { >>>>>>> + if (spi_is_csgpiod(spi)) { >>>>>>> + if (!spi->controller->set_cs_timing && !activate) >>>>>>> + spi_delay_exec(&spi->cs_hold, NULL); >>>>>>> + >>>>>>> if (!(spi->mode & SPI_NO_CS)) { >>>>>>> /* >>>>>>> * Historically ACPI has no means of the GPIO >> polarity >>>>>> and @@ >>>>>>> -979,26 +1035,38 @@ static void spi_set_cs(struct spi_device *spi, >>>>>>> bool >>>>>> enable, bool force) >>>>>>> * ambiguity. That's why we use enable, that >> takes >>>>>> SPI_CS_HIGH >>>>>>> * into account. >>>>>>> */ >>>>>>> - if (has_acpi_companion(&spi->dev)) >>>>>>> - >>>>>> gpiod_set_value_cansleep(spi_get_csgpiod(spi, 0), !enable); >>>>>>> - else >>>>>>> - /* Polarity handled by GPIO library */ >>>>>>> - >>>>>> gpiod_set_value_cansleep(spi_get_csgpiod(spi, 0), activate); >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { >>>>>>> + if (((spi->cs_index_mask >> idx) & >> 0x01) && >>>>>>> + spi_get_csgpiod(spi, idx)) { >>>>>>> + if (has_acpi_companion(&spi- >>> dev)) >>>>>>> + >>>>>> gpiod_set_value_cansleep(spi_get_csgpiod(spi, idx), >>>>>>> + >>>>>> !enable); >>>>>>> + else >>>>>>> + /* Polarity handled by >> GPIO >>>>>> library */ >>>>>>> + >>>>>> gpiod_set_value_cansleep(spi_get_csgpiod(spi, idx), >>>>>>> + >>>>>> activate); >>>>>>> + >>>>>>> + if (activate) >>>>>>> + spi_delay_exec(&spi- >>>>>>> cs_setup, NULL); >>>>>>> + else >>>>>>> + spi_delay_exec(&spi- >>>>>>> cs_inactive, NULL); >>>>>>> + } >>>>>>> + } >>>>>>> } >>>>>>> /* Some SPI masters need both GPIO CS & >> slave_select */ >>>>>>> if ((spi->controller->flags & >> SPI_CONTROLLER_GPIO_SS) && >>>>>>> spi->controller->set_cs) >>>>>>> spi->controller->set_cs(spi, !enable); >>>>>>> + >>>>>>> + if (!spi->controller->set_cs_timing) { >>>>>>> + if (activate) >>>>>>> + spi_delay_exec(&spi->cs_setup, >> NULL); >>>>>>> + else >>>>>>> + spi_delay_exec(&spi->cs_inactive, >> NULL); >>>>>>> + } >>>>>>> } else if (spi->controller->set_cs) { >>>>>>> spi->controller->set_cs(spi, !enable); >>>>>>> } >>>>>>> - >>>>>>> - if (spi_get_csgpiod(spi, 0) || !spi->controller->set_cs_timing) { >>>>>>> - if (activate) >>>>>>> - spi_delay_exec(&spi->cs_setup, NULL); >>>>>>> - else >>>>>>> - spi_delay_exec(&spi->cs_inactive, NULL); >>>>>>> - } >>>>>>> } >>>>>>> >>>>>>> #ifdef CONFIG_HAS_DMA >>>>>>> @@ -2222,8 +2290,8 @@ static void of_spi_parse_dt_cs_delay(struct >>>>>> device_node *nc, >>>>>>> static int of_spi_parse_dt(struct spi_controller *ctlr, struct >>>>>>> spi_device >>>> *spi, >>>>>>> struct device_node *nc) >>>>>>> { >>>>>>> - u32 value; >>>>>>> - int rc; >>>>>>> + u32 value, cs[SPI_CS_CNT_MAX]; >>>>>>> + int rc, idx; >>>>>>> >>>>>>> /* Mode (clock phase/polarity/etc.) */ >>>>>>> if (of_property_read_bool(nc, "spi-cpha")) @@ -2295,14 >>>>>>> +2363,52 >>>>>> @@ >>>>>>> static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device >> *spi, >>>>>>> return 0; >>>>>>> } >>>>>>> >>>>>>> + if (ctlr->num_chipselect > SPI_CS_CNT_MAX) { >>>>>>> + dev_err(&ctlr->dev, "No. of CS is more than max. no. >> of >>>>>> supported CS\n"); >>>>>>> + return -EINVAL; >>>>>>> + } >>>>>>> + >>>>>>> + /* >>>>>>> + * Zero(0) is a valid physical CS value and can be located at >> any >>>>>>> + * logical CS in the spi->chip_select[]. If all the physical CS >>>>>>> + * are initialized to 0 then It would be difficult to differentiate >>>>>>> + * between a valid physical CS 0 & an unused logical CS whose >>>>>> physical >>>>>>> + * CS can be 0. As a solution to this issue initialize all the CS to >> 0xFF. >>>>>>> + * Now all the unused logical CS will have 0xFF physical CS >>>>>>> +value & can >>>>>> be >>>>>>> + * ignore while performing physical CS validity checks. >>>>>>> + */ >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) >>>>>>> + spi_set_chipselect(spi, idx, 0xFF); >>>>>>> + >>>>>>> /* Device address */ >>>>>>> - rc = of_property_read_u32(nc, "reg", &value); >>>>>>> - if (rc) { >>>>>>> + rc = of_property_read_variable_u32_array(nc, "reg", &cs[0], >> 1, >>>>>>> + SPI_CS_CNT_MAX); >>>>>>> + if (rc < 0) { >>>>>>> dev_err(&ctlr->dev, "%pOF has no valid 'reg' property >>>>>> (%d)\n", >>>>>>> nc, rc); >>>>>>> return rc; >>>>>>> } >>>>>>> - spi_set_chipselect(spi, 0, value); >>>>>>> + if (rc > ctlr->num_chipselect) { >>>>>>> + dev_err(&ctlr->dev, "%pOF has number of CS > ctlr- >>>>>>> num_chipselect (%d)\n", >>>>>>> + nc, rc); >>>>>>> + return rc; >>>>>>> + } >>>>>>> + if ((of_property_read_bool(nc, "parallel-memories")) && >>>>>>> + (!(ctlr->flags & SPI_CONTROLLER_MULTI_CS))) { >>>>>>> + dev_err(&ctlr->dev, "SPI controller doesn't support >> multi >>>>>> CS\n"); >>>>>>> + return -EINVAL; >>>>>>> + } >>>>>>> + for (idx = 0; idx < rc; idx++) >>>>>>> + spi_set_chipselect(spi, idx, cs[idx]); >>>>>>> + >>>>>>> + /* spi->chip_select[i] gives the corresponding physical CS for >>>>>>> +logical CS >>>>>> i >>>>>>> + * logical CS number is represented by setting the ith bit in >>>>>>> +spi- >>>>>>> cs_index_mask >>>>>>> + * So, for example, if spi->cs_index_mask = 0x01 then logical >> CS >>>>>> number is 0 and >>>>>>> + * spi->chip_select[0] will give the physical CS. >>>>>>> + * By default spi->chip_select[0] will hold the physical CS >>>>>>> +number so, >>>>>> set >>>>>>> + * spi->cs_index_mask as 0x01. >>>>>>> + */ >>>>>>> + spi->cs_index_mask = 0x01; >>>>>>> >>>>>>> /* Device speed */ >>>>>>> if (!of_property_read_u32(nc, "spi-max-frequency", &value)) >>>>>>> @@ >>>>>>> -3100,6 +3206,7 @@ int spi_register_controller(struct spi_controller >> *ctlr) >>>>>>> struct boardinfo *bi; >>>>>>> int first_dynamic; >>>>>>> int status; >>>>>>> + int idx; >>>>>>> >>>>>>> if (!dev) >>>>>>> return -ENODEV; >>>>>>> @@ -3164,7 +3271,8 @@ int spi_register_controller(struct >>>>>>> spi_controller >>>>>> *ctlr) >>>>>>> } >>>>>>> >>>>>>> /* Setting last_cs to -1 means no chip selected */ >>>>>>> - ctlr->last_cs = -1; >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) >>>>>>> + ctlr->last_cs[idx] = -1; >>>>>>> >>>>>>> status = device_add(&ctlr->dev); >>>>>>> if (status < 0) >>>>>>> @@ -3889,7 +3997,7 @@ static int __spi_validate(struct spi_device >>>>>>> *spi, >>>>>> struct spi_message *message) >>>>>>> * cs_change is set for each transfer. >>>>>>> */ >>>>>>> if ((spi->mode & SPI_CS_WORD) && (!(ctlr->mode_bits & >>>>>> SPI_CS_WORD) || >>>>>>> - spi_get_csgpiod(spi, 0))) { >>>>>>> + spi_is_csgpiod(spi))) { >>>>>>> size_t maxsize = BITS_TO_BYTES(spi->bits_per_word); >>>>>>> int ret; >>>>>>> >>>>>>> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h >>>>>>> index 7b4baff63c5c..871d3a6d879b 100644 >>>>>>> --- a/include/linux/spi/spi.h >>>>>>> +++ b/include/linux/spi/spi.h >>>>>>> @@ -20,6 +20,9 @@ >>>>>>> >>>>>>> #include <uapi/linux/spi/spi.h> >>>>>>> >>>>>>> +/* Max no. of CS supported per spi device */ #define >>>>>>> +SPI_CS_CNT_MAX >>>>>>> +4 >>>>>>> + >>>>>>> struct dma_chan; >>>>>>> struct software_node; >>>>>>> struct ptp_system_timestamp; >>>>>>> @@ -132,7 +135,8 @@ extern void >>>>>> spi_transfer_cs_change_delay_exec(struct spi_message *msg, >>>>>>> * @max_speed_hz: Maximum clock rate to be used with this chip >>>>>>> * (on this board); may be changed by the device's driver. >>>>>>> * The spi_transfer.speed_hz can override this for each transfer. >>>>>>> - * @chip_select: Chipselect, distinguishing chips handled by >> @controller. >>>>>>> + * @chip_select: Array of physical chipselect, spi->chipselect[i] gives >>>>>>> + * the corresponding physical CS for logical CS i. >>>>>>> * @mode: The spi mode defines how data is clocked out and in. >>>>>>> * This may be changed by the device's driver. >>>>>>> * The "active low" default for chipselect mode can be >> overridden >>>>>>> @@ -157,8 +161,8 @@ extern void >>>>>> spi_transfer_cs_change_delay_exec(struct spi_message *msg, >>>>>>> * the device will bind to the named driver and only the named >> driver. >>>>>>> * Do not set directly, because core frees it; use >> driver_set_override() to >>>>>>> * set or clear it. >>>>>>> - * @cs_gpiod: GPIO descriptor of the chipselect line (optional, >>>>>>> NULL >>>> when >>>>>>> - * not using a GPIO line) >>>>>>> + * @cs_gpiod: Array of GPIO descriptors of the corresponding >>>>>>> + chipselect >>>>>> lines >>>>>>> + * (optional, NULL when not using a GPIO line) >>>>>>> * @word_delay: delay to be inserted between consecutive >>>>>>> * words of a transfer >>>>>>> * @cs_setup: delay to be introduced by the controller after >>>>>>> CS is asserted @@ -167,6 +171,7 @@ extern void >>>>>> spi_transfer_cs_change_delay_exec(struct spi_message *msg, >>>>>>> * deasserted. If @cs_change_delay is used from @spi_transfer, >> then >>>>>> the >>>>>>> * two delays will be added up. >>>>>>> * @pcpu_statistics: statistics for the spi_device >>>>>>> + * @cs_index_mask: Bit mask of the active chipselect(s) in the >>>>>>> + chipselect array >>>>>>> * >>>>>>> * A @spi_device is used to interchange data between an SPI slave >>>>>>> * (usually a discrete chip) and CPU memory. >>>>>>> @@ -182,7 +187,7 @@ struct spi_device { >>>>>>> struct spi_controller *controller; >>>>>>> struct spi_controller *master; /* Compatibility layer >> */ >>>>>>> u32 max_speed_hz; >>>>>>> - u8 chip_select; >>>>>>> + u8 chip_select[SPI_CS_CNT_MAX]; >>>>>>> u8 bits_per_word; >>>>>>> bool rt; >>>>>>> #define SPI_NO_TX BIT(31) /* No transmit wire */ >>>>>>> @@ -213,7 +218,7 @@ struct spi_device { >>>>>>> void *controller_data; >>>>>>> char modalias[SPI_NAME_SIZE]; >>>>>>> const char *driver_override; >>>>>>> - struct gpio_desc *cs_gpiod; /* Chip select GPIO descriptor >>>>>> */ >>>>>>> + struct gpio_desc *cs_gpiod[SPI_CS_CNT_MAX]; /* >> Chip select >>>>>> gpio desc */ >>>>>>> struct spi_delay word_delay; /* Inter-word delay */ >>>>>>> /* CS delays */ >>>>>>> struct spi_delay cs_setup; >>>>>>> @@ -223,6 +228,13 @@ struct spi_device { >>>>>>> /* The statistics */ >>>>>>> struct spi_statistics __percpu *pcpu_statistics; >>>>>>> >>>>>>> + /* Bit mask of the chipselect(s) that the driver need to use >> from >>>>>>> + * the chipselect array.When the controller is capable to >> handle >>>>>>> + * multiple chip selects & memories are connected in parallel >>>>>>> + * then more than one bit need to be set in cs_index_mask. >>>>>>> + */ >>>>>>> + u32 cs_index_mask : SPI_CS_CNT_MAX; >>>>>>> + >>>>>>> /* >>>>>>> * Likely need more hooks for more protocol options affecting >> how >>>>>>> * the controller talks to each chip, like: >>>>>>> @@ -279,22 +291,33 @@ static inline void *spi_get_drvdata(const >>>>>>> struct spi_device *spi) >>>>>>> >>>>>>> static inline u8 spi_get_chipselect(const struct spi_device *spi, u8 idx) >>>>>>> { >>>>>>> - return spi->chip_select; >>>>>>> + return spi->chip_select[idx]; >>>>>>> } >>>>>>> >>>>>>> static inline void spi_set_chipselect(struct spi_device *spi, >>>>>>> u8 idx, u8 >>>>>> chipselect) >>>>>>> { >>>>>>> - spi->chip_select = chipselect; >>>>>>> + spi->chip_select[idx] = chipselect; >>>>>>> } >>>>>>> >>>>>>> static inline struct gpio_desc *spi_get_csgpiod(const struct >>>>>>> spi_device *spi, >>>>>> u8 idx) >>>>>>> { >>>>>>> - return spi->cs_gpiod; >>>>>>> + return spi->cs_gpiod[idx]; >>>>>>> } >>>>>>> >>>>>>> static inline void spi_set_csgpiod(struct spi_device *spi, u8 >>>>>>> idx, struct >>>>>> gpio_desc *csgpiod) >>>>>>> { >>>>>>> - spi->cs_gpiod = csgpiod; >>>>>>> + spi->cs_gpiod[idx] = csgpiod; >>>>>>> +} >>>>>>> + >>>>>>> +static inline bool spi_is_csgpiod(struct spi_device *spi) { >>>>>>> + u8 idx; >>>>>>> + >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { >>>>>>> + if (spi_get_csgpiod(spi, idx)) >>>>>>> + return true; >>>>>>> + } >>>>>>> + return false; >>>>>>> } >>>>>>> >>>>>>> /** >>>>>>> @@ -399,6 +422,8 @@ extern struct spi_device >>>>>> *spi_new_ancillary_device(struct spi_device *spi, u8 ch >>>>>>> * @bus_lock_spinlock: spinlock for SPI bus locking >>>>>>> * @bus_lock_mutex: mutex for exclusion of multiple callers >>>>>>> * @bus_lock_flag: indicates that the SPI bus is locked for >>>>>>> exclusive use >>>>>>> + * @multi_cs_cap: indicates that the SPI Controller can assert/de- >> assert >>>>>>> + * more than one chip select at once. >>>>>>> * @setup: updates the device mode and clocking records used by a >>>>>>> * device's SPI controller; protocol code may call this. This >>>>>>> * must fail if an unrecognized or unsupported mode is >> requested. >>>>>>> @@ -567,6 +592,11 @@ struct spi_controller { >>>>>>> #define SPI_CONTROLLER_MUST_TX BIT(4) /* Requires tx >>>> */ >>>>>>> #define SPI_CONTROLLER_GPIO_SS BIT(5) /* GPIO CS >>>> must >>>>>> select slave */ >>>>>>> #define SPI_CONTROLLER_SUSPENDED BIT(6) /* Currently >>>>>> suspended */ >>>>>>> + /* >>>>>>> + * The spi-controller has multi chip select capability and can >>>>>>> + * assert/de-assert more than one chip select at once. >>>>>>> + */ >>>>>>> +#define SPI_CONTROLLER_MULTI_CS BIT(7) >>>>>>> >>>>>>> /* Flag indicating if the allocation of this struct is devres- >> managed */ >>>>>>> bool devm_allocated; >>>>>>> @@ -677,7 +707,8 @@ struct spi_controller { >>>>>>> bool rt; >>>>>>> bool auto_runtime_pm; >>>>>>> bool cur_msg_mapped; >>>>>>> - char last_cs; >>>>>>> + char last_cs[SPI_CS_CNT_MAX]; >>>>>>> + char last_cs_index_mask; >>>>>>> bool last_cs_mode_high; >>>>>>> bool fallback; >>>>>>> struct completion xfer_completion;
> -----Original Message----- > From: Stefan Binding <sbinding@opensource.cirrus.com> > Sent: Wednesday, November 22, 2023 4:51 PM > To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; > broonie@kernel.org; tudor.ambarus@linaro.org; pratyush@kernel.org; > miquel.raynal@bootlin.com; richard@nod.at; vigneshr@ti.com > Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; > michael@walle.cc; linux-mtd@lists.infradead.org; > nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com; > claudiu.beznea@tuxon.dev; Simek, Michal <michal.simek@amd.com>; linux- > arm-kernel@lists.infradead.org; git (AMD-Xilinx) <git@amd.com>; > amitrkcian2002@gmail.com; patches@opensource.cirrus.com > Subject: Re: [PATCH v10 1/8] spi: Add multi-cs memories support in SPI core > > > On 21/11/2023 19:18, Mahapatra, Amit Kumar wrote: > > Hello Stefan, > > > >> -----Original Message----- > >> From: Stefan Binding <sbinding@opensource.cirrus.com> > >> Sent: Tuesday, November 21, 2023 11:07 PM > >> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; > >> broonie@kernel.org; tudor.ambarus@linaro.org; pratyush@kernel.org; > >> miquel.raynal@bootlin.com; richard@nod.at; vigneshr@ti.com > >> Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; > >> michael@walle.cc; linux-mtd@lists.infradead.org; > >> nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com; > >> claudiu.beznea@tuxon.dev; Simek, Michal <michal.simek@amd.com>; > >> linux- arm-kernel@lists.infradead.org; git (AMD-Xilinx) > >> <git@amd.com>; amitrkcian2002@gmail.com; > >> patches@opensource.cirrus.com > >> Subject: Re: [PATCH v10 1/8] spi: Add multi-cs memories support in > >> SPI core > >> > >> > >> On 21/11/2023 16:35, Mahapatra, Amit Kumar wrote: > >>> Hello Stefan, > >>> > >>>> -----Original Message----- > >>>> From: Stefan Binding <sbinding@opensource.cirrus.com> > >>>> Sent: Tuesday, November 21, 2023 7:28 PM > >>>> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; > >>>> broonie@kernel.org; tudor.ambarus@linaro.org; pratyush@kernel.org; > >>>> miquel.raynal@bootlin.com; richard@nod.at; vigneshr@ti.com > >>>> Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; > >>>> michael@walle.cc; linux-mtd@lists.infradead.org; > >>>> nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com; > >>>> claudiu.beznea@tuxon.dev; Simek, Michal <michal.simek@amd.com>; > >>>> linux- arm-kernel@lists.infradead.org; git (AMD-Xilinx) > >>>> <git@amd.com>; amitrkcian2002@gmail.com; > >>>> patches@opensource.cirrus.com > >>>> Subject: Re: [PATCH v10 1/8] spi: Add multi-cs memories support in > >>>> SPI core > >>>> > >>>> > >>>> On 21/11/2023 07:39, Mahapatra, Amit Kumar wrote: > >>>>> Hello Stefan, > >>>>> > >>>>>> -----Original Message----- > >>>>>> From: Stefan Binding <sbinding@opensource.cirrus.com> > >>>>>> Sent: Monday, November 20, 2023 8:00 PM > >>>>>> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; > >>>>>> broonie@kernel.org; tudor.ambarus@linaro.org; > >>>>>> pratyush@kernel.org; miquel.raynal@bootlin.com; richard@nod.at; > >>>>>> vigneshr@ti.com > >>>>>> Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; > >>>>>> michael@walle.cc; linux-mtd@lists.infradead.org; > >>>>>> nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com; > >>>>>> claudiu.beznea@tuxon.dev; Simek, Michal > <michal.simek@amd.com>; > >>>>>> linux- arm-kernel@lists.infradead.org; git (AMD-Xilinx) > >>>>>> <git@amd.com>; amitrkcian2002@gmail.com; > >>>>>> patches@opensource.cirrus.com > >>>>>> Subject: Re: [PATCH v10 1/8] spi: Add multi-cs memories support > >>>>>> in SPI core > >>>>>> > >>>>>> > >>>>>> On 18/11/2023 13:54, Amit Kumar Mahapatra wrote: > >>>>>>> AMD-Xilinx GQSPI controller has two advanced mode that allows > >>>>>>> the controller to consider two flashes as one single device. > >>>>>>> > >>>>>>> One of these two mode is the parallel mode in which each byte of > >>>>>>> data is stored in both devices, the even bits in the lower flash > >>>>>>> & the odd bits in the upper flash. The byte split is > >>>>>>> automatically handled by the QSPI controller. > >>>>>>> > >>>>>>> The other mode is the stacked mode in which both the flashes > >>>>>>> share the same SPI bus but each of the device contain half of the > data. > >>>>>>> In this mode, the controller does not follow CS requests but > >>>>>>> instead internally wires the two CS levels with the value of the > >>>>>>> most significant > >>>>>> address bit. > >>>>>>> For supporting both these modes SPI core need to be updated for > >>>>>>> providing multiple CS for a single SPI device. > >>>>>>> > >>>>>>> For adding multi CS support the SPI device need to be aware of > >>>>>>> all the CS values. So, the "chip_select" member in the > >>>>>>> spi_device structure is now an array that holds all the CS values. > >>>>>>> > >>>>>>> spi_device structure now has a "cs_index_mask" member. This acts > >>>>>>> as an index to the chip_select array. If nth bit of > >>>>>>> spi->cs_index_mask is set then the driver would assert spi- > >>> chip_select[n]. > >>>>>>> In parallel mode all the chip selects are asserted/de-asserted > >>>>>>> simultaneously and each byte of data is stored in both devices, > >>>>>>> the even bits in one, the odd bits in the other. The split is > >>>>>>> automatically handled by the GQSPI controller. The GQSPI > >>>>>>> controller supports a maximum of two flashes connected in > >>>>>>> parallel mode. A SPI_CONTROLLER_MULTI_CS flag bit is added in > >>>>>>> the spi controller flags, through ctlr->flags the spi core will > >>>>>>> make sure that the controller is capable of handling multiple chip > selects at once. > >>>>>>> > >>>>>>> For supporting multiple CS via GPIO the cs_gpiod member of the > >>>>>>> spi_device structure is now an array that holds the gpio > >>>>>>> descriptor for each chipselect. > >>>>>>> > >>>>>>> CS GPIO is not tested due to unavailability of necessary > >>>>>>> hardware > >> setup. > >>>>>>> Signed-off-by: Amit Kumar Mahapatra <amit.kumar- > >>>> mahapatra@amd.com> > >>>>>>> --- > >>>>>>> Hello @Stefen, > >>>>>>> We restructured the SPI core implementation, for handling > >>>>>>> arbitrary number of devices connected in parallel(multi-cs) or > >>>>>>> stacked > >> mode. > >>>>>>> We have tested this updated patch on native-cs setup but > >>>>>>> couldn't test cs-gpio due to unavailability of a proper setup. > >>>>>>> If possible, could you please retest the cs-gpio use case with > >>>>>>> this updated patch and provide > >>>>>> your feedback. > >>>>>> > >>>>>> Hi, > >>>>>> > >>>>>> I tested this chain on 2 different systems using GPIOs as chip > >>>>>> selects, and see the same error on both: > >>>>>> [ 2.842045] pxa2xx-spi pxa2xx-spi.6: chipselect 0 already in > >>>>>> use > >>>>>> > >>>>>> Let me know if you need any further testing. > >>>>>> > >>>>>> Thanks, > >>>>>> > >>>>>> Stefan Binding > >>>>>> > >>>>>>> --- > >>>>>>> drivers/spi/spi.c | 192 +++++++++++++++++++++++++++++++--- > ---- > >> -- > >>>>>>> include/linux/spi/spi.h | 51 ++++++++--- > >>>>>>> 2 files changed, 191 insertions(+), 52 deletions(-) > >>>>>>> > >>>>>>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index > >>>>>>> 8ead7acb99f3..ff66147ba95f 100644 > >>>>>>> --- a/drivers/spi/spi.c > >>>>>>> +++ b/drivers/spi/spi.c > >>>>>>> @@ -612,10 +612,21 @@ static int spi_dev_check(struct device > >>>>>>> *dev, void > >>>>>> *data) > >>>>>>> { > >>>>>>> struct spi_device *spi = to_spi_device(dev); > >>>>>>> struct spi_device *new_spi = data; > >>>>>>> - > >>>>>>> - if (spi->controller == new_spi->controller && > >>>>>>> - spi_get_chipselect(spi, 0) == spi_get_chipselect(new_spi, 0)) > >>>>>>> - return -EBUSY; > >>>>>>> + int idx, nw_idx; > >>>>>>> + u8 cs, cs_nw; > >>>>>>> + > >>>>>>> + if (spi->controller == new_spi->controller) { > >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > >>>>>>> + cs = spi_get_chipselect(spi, idx); > >>>>>>> + for (nw_idx = 0; nw_idx < SPI_CS_CNT_MAX; > >>>>>> nw_idx++) { > >>>>>>> + cs_nw = spi_get_chipselect(spi, > >> nw_idx); > >>>>> Thank you for dedicating your time to test my patch. > >>>>> As per my analysis the error is reported from here as we are > >>>>> supplying the former SPI device structure to retrieve the CS value > >>>>> for the new SPI devices. > >>>>> To fix this, could you kindly substitute the above line with > >>>>> > >>>>> cs_nw = spi_get_chipselect(new_spi, nw_idx); > >>>>> > >>>>> and rerun your tests? > >>>>> If it works correctly, I will incorporate this fix into my > >>>>> upcoming series. > >>>>> > >>>>> Regards, > >>>>> Amit > >> Hi, > >> > >> I've attached my log. > >> I notice that you add a print to of_spi_parse_dt, however since the > >> laptop I am using is an x86 laptop, it uses ACPI rather than OF, and > >> I don't think this function isnt even compiled in. > > That’s correct, I missed it. > > Upon reviewing the logs, I discovered that in the ACPI flow, I am not > > initializing the unused CS[] to FF, as I am doing in the OF flow. > > Consequently, in the ACPI flow, all the unused CS[] are automatically > > initialized to 0. During the __spi_add_device process, the SPI core > > faces difficulty distinguishing between a valid CS value of 0 and an > > unused CS value of 0, leading to a failure in the driver probe. > > > > Incorporating the following modifications should resolve this issue. > > Kindly apply these changes on top of 1/8 and re-run the test. > > > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index > > ff66147ba95f..d0301c17d4f8 100644 > > --- a/drivers/spi/spi.c > > +++ b/drivers/spi/spi.c > > @@ -619,7 +619,7 @@ static int spi_dev_check(struct device *dev, void > *data) > > for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > > cs = spi_get_chipselect(spi, idx); > > for (nw_idx = 0; nw_idx < SPI_CS_CNT_MAX; nw_idx++) { > > - cs_nw = spi_get_chipselect(spi, nw_idx); > > + cs_nw = spi_get_chipselect(new_spi, > > + nw_idx); > > if (cs != 0xFF && cs_nw != 0xFF && cs == cs_nw) { > > dev_err(dev, "chipselect %d already in use\n", > cs_nw); > > return -EBUSY; @@ -764,6 > > +764,7 @@ struct spi_device *spi_new_device(struct spi_controller *ctlr, > > { > > struct spi_device *proxy; > > int status; > > + u8 idx; > > > > /* > > * NOTE: caller did any chip->bus_num checks necessary. > > @@ -779,6 +780,9 @@ struct spi_device *spi_new_device(struct > > spi_controller *ctlr, > > > > WARN_ON(strlen(chip->modalias) >= sizeof(proxy->modalias)); > > > > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > > + spi_set_chipselect(proxy, idx, 0xFF); > > + > > spi_set_chipselect(proxy, 0, chip->chip_select); > > proxy->max_speed_hz = chip->max_speed_hz; > > proxy->mode = chip->mode; > > @@ -2514,6 +2518,7 @@ struct spi_device > *spi_new_ancillary_device(struct spi_device *spi, > > struct spi_controller *ctlr = spi->controller; > > struct spi_device *ancillary; > > int rc = 0; > > + u8 idx; > > > > /* Alloc an spi_device */ > > ancillary = spi_alloc_device(ctlr); @@ -2524,6 +2529,9 @@ > > struct spi_device *spi_new_ancillary_device(struct spi_device *spi, > > > > strscpy(ancillary->modalias, "dummy", > > sizeof(ancillary->modalias)); > > > > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > > + spi_set_chipselect(ancillary, idx, 0xFF); > > + > > /* Use provided chip-select for ancillary device */ > > spi_set_chipselect(ancillary, 0, chip_select); > > > > @@ -2732,6 +2740,7 @@ struct spi_device *acpi_spi_device_alloc(struct > spi_controller *ctlr, > > struct acpi_spi_lookup lookup = {}; > > struct spi_device *spi; > > int ret; > > + u8 idx; > > > > if (!ctlr && index == -1) > > return ERR_PTR(-EINVAL); @@ -2767,6 +2776,9 @@ struct > > spi_device *acpi_spi_device_alloc(struct spi_controller *ctlr, > > return ERR_PTR(-ENOMEM); > > } > > > > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > > + spi_set_chipselect(spi, idx, 0xFF); > > + > > ACPI_COMPANION_SET(&spi->dev, adev); > > spi->max_speed_hz = lookup.max_speed_hz; > > spi->mode |= lookup.mode; > > > > Regards, > > Amit > > > > Hi, > > I no longer see that error, or any errors printed from SPI, however, all of the > transactions are now broken. > Every transaction seems to read back 0x0, on all SPI devices, whether native > CS or GPIO CS, and I dont see it toggling the CS GPIO anymore. I believe the issue stems from the fact that in the ACPI patch, spi->cs_index_mask is not set to 0x01. As a result, in spi_set_cs, it fails to meet the condition that governs the CS asset code snippet. In the following code diff, I've included the necessary fix along with the earlier changes on top of 1/8. I've also included a couple of debug prints to aid in further debugging in case my analysis is incorrect. Please apply these changes on top of 1/8 and re-run the test. diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index ff66147ba95f..b2e035c3d494 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -619,7 +619,7 @@ static int spi_dev_check(struct device *dev, void *data) for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { cs = spi_get_chipselect(spi, idx); for (nw_idx = 0; nw_idx < SPI_CS_CNT_MAX; nw_idx++) { - cs_nw = spi_get_chipselect(spi, nw_idx); + cs_nw = spi_get_chipselect(new_spi, nw_idx); if (cs != 0xFF && cs_nw != 0xFF && cs == cs_nw) { dev_err(dev, "chipselect %d already in use\n", cs_nw); return -EBUSY; @@ -764,6 +764,7 @@ struct spi_device *spi_new_device(struct spi_controller *ctlr, { struct spi_device *proxy; int status; + u8 idx; /* * NOTE: caller did any chip->bus_num checks necessary. @@ -779,6 +780,9 @@ struct spi_device *spi_new_device(struct spi_controller *ctlr, WARN_ON(strlen(chip->modalias) >= sizeof(proxy->modalias)); + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) + spi_set_chipselect(proxy, idx, 0xFF); + spi_set_chipselect(proxy, 0, chip->chip_select); proxy->max_speed_hz = chip->max_speed_hz; proxy->mode = chip->mode; @@ -787,6 +791,7 @@ struct spi_device *spi_new_device(struct spi_controller *ctlr, proxy->dev.platform_data = (void *) chip->platform_data; proxy->controller_data = chip->controller_data; proxy->controller_state = NULL; + proxy->cs_index_mask = 0x01; if (chip->swnode) { status = device_add_software_node(&proxy->dev, chip->swnode); @@ -1024,6 +1029,8 @@ static void spi_set_cs(struct spi_device *spi, bool enable, bool force) if (!spi->controller->set_cs_timing && !activate) spi_delay_exec(&spi->cs_hold, NULL); + printk("%s() [%d] spi->cs_index_mask == [%d]\n",__func__, __LINE__, spi->cs_index_mask); + if (!(spi->mode & SPI_NO_CS)) { /* * Historically ACPI has no means of the GPIO polarity and @@ -1038,6 +1045,7 @@ static void spi_set_cs(struct spi_device *spi, bool enable, bool force) for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { if (((spi->cs_index_mask >> idx) & 0x01) && spi_get_csgpiod(spi, idx)) { + printk("%s() [%d] idx == [%d]\n",__func__, __LINE__, idx); if (has_acpi_companion(&spi->dev)) gpiod_set_value_cansleep(spi_get_csgpiod(spi, idx), !enable); @@ -2514,6 +2522,7 @@ struct spi_device *spi_new_ancillary_device(struct spi_device *spi, struct spi_controller *ctlr = spi->controller; struct spi_device *ancillary; int rc = 0; + u8 idx; /* Alloc an spi_device */ ancillary = spi_alloc_device(ctlr); @@ -2524,12 +2533,16 @@ struct spi_device *spi_new_ancillary_device(struct spi_device *spi, strscpy(ancillary->modalias, "dummy", sizeof(ancillary->modalias)); + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) + spi_set_chipselect(ancillary, idx, 0xFF); + /* Use provided chip-select for ancillary device */ spi_set_chipselect(ancillary, 0, chip_select); /* Take over SPI mode/speed from SPI main device */ ancillary->max_speed_hz = spi->max_speed_hz; ancillary->mode = spi->mode; + ancillary->cs_index_mask = 0x01; WARN_ON(!mutex_is_locked(&ctlr->add_lock)); @@ -2732,6 +2745,7 @@ struct spi_device *acpi_spi_device_alloc(struct spi_controller *ctlr, struct acpi_spi_lookup lookup = {}; struct spi_device *spi; int ret; + u8 idx; if (!ctlr && index == -1) return ERR_PTR(-EINVAL); @@ -2767,12 +2781,16 @@ struct spi_device *acpi_spi_device_alloc(struct spi_controller *ctlr, return ERR_PTR(-ENOMEM); } + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) + spi_set_chipselect(spi, idx, 0xFF); + ACPI_COMPANION_SET(&spi->dev, adev); spi->max_speed_hz = lookup.max_speed_hz; spi->mode |= lookup.mode; spi->irq = lookup.irq; spi->bits_per_word = lookup.bits_per_word; spi_set_chipselect(spi, 0, lookup.chip_select); + spi->cs_index_mask = 0x01; return spi; } Regards, Amit > > Thanks, > > Stefan > > >> Thanks, > >> > >> Stefan > >> > >>>> Hi, > >>>> > >>>> I still see the same error: > >>>> > >>>> [ 2.748546] pxa2xx-spi pxa2xx-spi.6: chipselect 0 already in use > >>> Thank you for the quick testing. For further analysis I have > >>> incorporated additional debug prints on top of 1/8 patch. The > >>> corresponding diff is shared below. Kindly implement the specified > >>> changes, rerun your test and share the kernel logs. > >>> > >>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index > >>> ff66147ba95f..7f59ea81593d 100644 > >>> --- a/drivers/spi/spi.c > >>> +++ b/drivers/spi/spi.c > >>> @@ -618,8 +618,10 @@ static int spi_dev_check(struct device *dev, > >>> void > >> *data) > >>> if (spi->controller == new_spi->controller) { > >>> for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > >>> cs = spi_get_chipselect(spi, idx); > >>> + printk("%s() [%d] CS[%d] = [%d]\n", > >>> + __func__, __LINE__, idx, cs); > >>> for (nw_idx = 0; nw_idx < SPI_CS_CNT_MAX; nw_idx++) { > >>> - cs_nw = spi_get_chipselect(spi, nw_idx); > >>> + cs_nw = spi_get_chipselect(new_spi, nw_idx); > >>> + printk("%s() [%d] CS_NEW[%d] = > >>> + [%d]\n", __func__, __LINE__, nw_idx, cs_nw); > >>> if (cs != 0xFF && cs_nw != 0xFF && cs == cs_nw) { > >>> dev_err(dev, "chipselect > >>> %d already in use\n", > >> cs_nw); > >>> return -EBUSY; @@ -659,8 > >>> +661,10 @@ static int __spi_add_device(struct spi_device *spi) > >>> */ > >>> for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > >>> cs = spi_get_chipselect(spi, idx); > >>> + printk("%s() [%d] CS[%d] = [%d]\n", __func__, > >>> + __LINE__, idx, cs); > >>> for (nw_idx = idx + 1; nw_idx < SPI_CS_CNT_MAX; nw_idx++) { > >>> nw_cs = spi_get_chipselect(spi, nw_idx); > >>> + printk("%s() [%d] CS_NEW[%d] = [%d]\n", > >>> + __func__, __LINE__, nw_idx, nw_cs); > >>> if (cs != 0xFF && nw_cs != 0xFF && cs == nw_cs) { > >>> dev_err(dev, "chipselect %d already in use\n", nw_cs); > >>> return -EBUSY; @@ -2401,6 +2405,9 > >>> @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device > *spi, > >>> for (idx = 0; idx < rc; idx++) > >>> spi_set_chipselect(spi, idx, cs[idx]); > >>> > >>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > >>> + printk("%s() [%d] CS[%d] = [%d]\n", __func__, > >>> + __LINE__, idx, spi_get_chipselect(spi, idx)); > >>> + > >>> /* spi->chip_select[i] gives the corresponding physical CS for logical > CS i > >>> * logical CS number is represented by setting the ith bit > >>> in spi- cs_index_mask > >>> * So, for example, if spi->cs_index_mask = 0x01 then > >>> logical CS number is 0 and > >>> > >>> Regards, > >>> Amit > >>> > >>>> Thanks, > >>>> > >>>> Stefan > >>>> > >>>>>>> + if (cs != 0xFF && cs_nw != 0xFF && cs > >> == > >>>>>> cs_nw) { > >>>>>>> + dev_err(dev, "chipselect %d > >> already in > >>>>>> use\n", cs_nw); > >>>>>>> + return -EBUSY; > >>>>>>> + } > >>>>>>> + } > >>>>>>> + } > >>>>>>> + } > >>>>>>> return 0; > >>>>>>> } > >>>>>>> > >>>>>>> @@ -629,13 +640,32 @@ static int __spi_add_device(struct > >>>>>>> spi_device > >>>> *spi) > >>>>>>> { > >>>>>>> struct spi_controller *ctlr = spi->controller; > >>>>>>> struct device *dev = ctlr->dev.parent; > >>>>>>> - int status; > >>>>>>> + int status, idx, nw_idx; > >>>>>>> + u8 cs, nw_cs; > >>>>>>> + > >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > >>>>>>> + /* Chipselects are numbered 0..max; validate. */ > >>>>>>> + cs = spi_get_chipselect(spi, idx); > >>>>>>> + if (cs != 0xFF && cs >= ctlr->num_chipselect) { > >>>>>>> + dev_err(dev, "cs%d >= max %d\n", > >>>>>> spi_get_chipselect(spi, idx), > >>>>>>> + ctlr->num_chipselect); > >>>>>>> + return -EINVAL; > >>>>>>> + } > >>>>>>> + } > >>>>>>> > >>>>>>> - /* Chipselects are numbered 0..max; validate. */ > >>>>>>> - if (spi_get_chipselect(spi, 0) >= ctlr->num_chipselect) { > >>>>>>> - dev_err(dev, "cs%d >= max %d\n", > spi_get_chipselect(spi, 0), > >>>>>>> - ctlr->num_chipselect); > >>>>>>> - return -EINVAL; > >>>>>>> + /* > >>>>>>> + * Make sure that multiple logical CS doesn't map to the same > >>>>>> physical CS. > >>>>>>> + * For example, spi->chip_select[0] != spi->chip_select[1] and > >> so on. > >>>>>>> + */ > >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > >>>>>>> + cs = spi_get_chipselect(spi, idx); > >>>>>>> + for (nw_idx = idx + 1; nw_idx < SPI_CS_CNT_MAX; > >> nw_idx++) { > >>>>>>> + nw_cs = spi_get_chipselect(spi, nw_idx); > >>>>>>> + if (cs != 0xFF && nw_cs != 0xFF && cs == > >> nw_cs) { > >>>>>>> + dev_err(dev, "chipselect %d already in > >> use\n", > >>>>>> nw_cs); > >>>>>>> + return -EBUSY; > >>>>>>> + } > >>>>>>> + } > >>>>>>> } > >>>>>>> > >>>>>>> /* Set the bus ID string */ @@ -647,11 +677,8 @@ static > >>>>>>> int __spi_add_device(struct spi_device > >>>> *spi) > >>>>>>> * its configuration. > >>>>>>> */ > >>>>>>> status = bus_for_each_dev(&spi_bus_type, NULL, spi, > >> spi_dev_check); > >>>>>>> - if (status) { > >>>>>>> - dev_err(dev, "chipselect %d already in use\n", > >>>>>>> - spi_get_chipselect(spi, 0)); > >>>>>>> + if (status) > >>>>>>> return status; > >>>>>>> - } > >>>>>>> > >>>>>>> /* Controller may unregister concurrently */ > >>>>>>> if (IS_ENABLED(CONFIG_SPI_DYNAMIC) && @@ -659,8 > >> +686,15 @@ > >>>>>> static > >>>>>>> int __spi_add_device(struct spi_device *spi) > >>>>>>> return -ENODEV; > >>>>>>> } > >>>>>>> > >>>>>>> - if (ctlr->cs_gpiods) > >>>>>>> - spi_set_csgpiod(spi, 0, ctlr- > >cs_gpiods[spi_get_chipselect(spi, > >>>>>> 0)]); > >>>>>>> + if (ctlr->cs_gpiods) { > >>>>>>> + u8 cs; > >>>>>>> + > >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > >>>>>>> + cs = spi_get_chipselect(spi, idx); > >>>>>>> + if (cs != 0xFF) > >>>>>>> + spi_set_csgpiod(spi, idx, ctlr- > >>> cs_gpiods[cs]); > >>>>>>> + } > >>>>>>> + } > >>>>>>> > >>>>>>> /* > >>>>>>> * Drivers may modify this initial i/o setup, but will @@ > >>>>>>> -701,6 > >>>>>>> +735,9 @@ int spi_add_device(struct spi_device *spi) > >>>>>>> struct spi_controller *ctlr = spi->controller; > >>>>>>> int status; > >>>>>>> > >>>>>>> + /* Set the bus ID string */ > >>>>>>> + spi_dev_set_name(spi); > >>>>>>> + > >>>>>>> mutex_lock(&ctlr->add_lock); > >>>>>>> status = __spi_add_device(spi); > >>>>>>> mutex_unlock(&ctlr->add_lock); @@ -942,32 +979,51 @@ > >> static > >>>>>>> void spi_res_release(struct spi_controller > >>>>>> *ctlr, struct spi_message *mes > >>>>>>> } > >>>>>>> > >>>>>>> > >>>>>>> /*-------------------------------------------------------------- > >>>>>>> -- > >>>>>>> -- > >>>>>>> -- > >>>>>>> -----*/ > >>>>>>> +static inline bool spi_is_last_cs(struct spi_device *spi) { > >>>>>>> + u8 idx; > >>>>>>> + bool last = false; > >>>>>>> + > >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > >>>>>>> + if ((spi->cs_index_mask >> idx) & 0x01) { > >>>>>>> + if (spi->controller->last_cs[idx] == > >>>>>> spi_get_chipselect(spi, idx)) > >>>>>>> + last = true; > >>>>>>> + } > >>>>>>> + } > >>>>>>> + return last; > >>>>>>> +} > >>>>>>> + > >>>>>>> > >>>>>>> static void spi_set_cs(struct spi_device *spi, bool enable, bool > force) > >>>>>>> { > >>>>>>> bool activate = enable; > >>>>>>> + u8 idx; > >>>>>>> > >>>>>>> /* > >>>>>>> * Avoid calling into the driver (or doing delays) if the > >>>>>>> chip > >> select > >>>>>>> * isn't actually changing from the last time this was called. > >>>>>>> */ > >>>>>>> - if (!force && ((enable && spi->controller->last_cs == > >>>>>> spi_get_chipselect(spi, 0)) || > >>>>>>> - (!enable && spi->controller->last_cs != > >>>>>> spi_get_chipselect(spi, 0))) && > >>>>>>> + if (!force && ((enable && spi->controller->last_cs_index_mask > >> == > >>>>>>> +spi- > >>>>>>> cs_index_mask && > >>>>>>> + spi_is_last_cs(spi)) || > >>>>>>> + (!enable && spi->controller->last_cs_index_mask > >> == spi- > >>>>>>> cs_index_mask && > >>>>>>> + !spi_is_last_cs(spi))) && > >>>>>>> (spi->controller->last_cs_mode_high == (spi->mode & > >>>>>> SPI_CS_HIGH))) > >>>>>>> return; > >>>>>>> > >>>>>>> trace_spi_set_cs(spi, activate); > >>>>>>> > >>>>>>> - spi->controller->last_cs = enable ? spi_get_chipselect(spi, 0) : - > 1; > >>>>>>> + spi->controller->last_cs_index_mask = spi->cs_index_mask; > >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > >>>>>>> + spi->controller->last_cs[idx] = enable ? > >>>>>>> +spi_get_chipselect(spi, > >>>>>> 0) > >>>>>>> +: -1; > >>>>>>> spi->controller->last_cs_mode_high = spi->mode & > >> SPI_CS_HIGH; > >>>>>>> - if ((spi_get_csgpiod(spi, 0) || !spi->controller->set_cs_timing) > && > >>>>>> !activate) > >>>>>>> - spi_delay_exec(&spi->cs_hold, NULL); > >>>>>>> - > >>>>>>> if (spi->mode & SPI_CS_HIGH) > >>>>>>> enable = !enable; > >>>>>>> > >>>>>>> - if (spi_get_csgpiod(spi, 0)) { > >>>>>>> + if (spi_is_csgpiod(spi)) { > >>>>>>> + if (!spi->controller->set_cs_timing && !activate) > >>>>>>> + spi_delay_exec(&spi->cs_hold, NULL); > >>>>>>> + > >>>>>>> if (!(spi->mode & SPI_NO_CS)) { > >>>>>>> /* > >>>>>>> * Historically ACPI has no means of the GPIO > >> polarity > >>>>>> and @@ > >>>>>>> -979,26 +1035,38 @@ static void spi_set_cs(struct spi_device > >>>>>>> *spi, bool > >>>>>> enable, bool force) > >>>>>>> * ambiguity. That's why we use enable, that > >> takes > >>>>>> SPI_CS_HIGH > >>>>>>> * into account. > >>>>>>> */ > >>>>>>> - if (has_acpi_companion(&spi->dev)) > >>>>>>> - > >>>>>> gpiod_set_value_cansleep(spi_get_csgpiod(spi, 0), !enable); > >>>>>>> - else > >>>>>>> - /* Polarity handled by GPIO library */ > >>>>>>> - > >>>>>> gpiod_set_value_cansleep(spi_get_csgpiod(spi, 0), activate); > >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > >>>>>>> + if (((spi->cs_index_mask >> idx) & > >> 0x01) && > >>>>>>> + spi_get_csgpiod(spi, idx)) { > >>>>>>> + if (has_acpi_companion(&spi- > >>> dev)) > >>>>>>> + > >>>>>> gpiod_set_value_cansleep(spi_get_csgpiod(spi, idx), > >>>>>>> + > >>>>>> !enable); > >>>>>>> + else > >>>>>>> + /* Polarity handled by > >> GPIO > >>>>>> library */ > >>>>>>> + > >>>>>> gpiod_set_value_cansleep(spi_get_csgpiod(spi, idx), > >>>>>>> + > >>>>>> activate); > >>>>>>> + > >>>>>>> + if (activate) > >>>>>>> + spi_delay_exec(&spi- > >>>>>>> cs_setup, NULL); > >>>>>>> + else > >>>>>>> + spi_delay_exec(&spi- > >>>>>>> cs_inactive, NULL); > >>>>>>> + } > >>>>>>> + } > >>>>>>> } > >>>>>>> /* Some SPI masters need both GPIO CS & > >> slave_select */ > >>>>>>> if ((spi->controller->flags & > >> SPI_CONTROLLER_GPIO_SS) && > >>>>>>> spi->controller->set_cs) > >>>>>>> spi->controller->set_cs(spi, !enable); > >>>>>>> + > >>>>>>> + if (!spi->controller->set_cs_timing) { > >>>>>>> + if (activate) > >>>>>>> + spi_delay_exec(&spi->cs_setup, > >> NULL); > >>>>>>> + else > >>>>>>> + spi_delay_exec(&spi->cs_inactive, > >> NULL); > >>>>>>> + } > >>>>>>> } else if (spi->controller->set_cs) { > >>>>>>> spi->controller->set_cs(spi, !enable); > >>>>>>> } > >>>>>>> - > >>>>>>> - if (spi_get_csgpiod(spi, 0) || !spi->controller->set_cs_timing) { > >>>>>>> - if (activate) > >>>>>>> - spi_delay_exec(&spi->cs_setup, NULL); > >>>>>>> - else > >>>>>>> - spi_delay_exec(&spi->cs_inactive, NULL); > >>>>>>> - } > >>>>>>> } > >>>>>>> > >>>>>>> #ifdef CONFIG_HAS_DMA > >>>>>>> @@ -2222,8 +2290,8 @@ static void > >>>>>>> of_spi_parse_dt_cs_delay(struct > >>>>>> device_node *nc, > >>>>>>> static int of_spi_parse_dt(struct spi_controller *ctlr, > >>>>>>> struct spi_device > >>>> *spi, > >>>>>>> struct device_node *nc) > >>>>>>> { > >>>>>>> - u32 value; > >>>>>>> - int rc; > >>>>>>> + u32 value, cs[SPI_CS_CNT_MAX]; > >>>>>>> + int rc, idx; > >>>>>>> > >>>>>>> /* Mode (clock phase/polarity/etc.) */ > >>>>>>> if (of_property_read_bool(nc, "spi-cpha")) @@ -2295,14 > >>>>>>> +2363,52 > >>>>>> @@ > >>>>>>> static int of_spi_parse_dt(struct spi_controller *ctlr, struct > >>>>>>> spi_device > >> *spi, > >>>>>>> return 0; > >>>>>>> } > >>>>>>> > >>>>>>> + if (ctlr->num_chipselect > SPI_CS_CNT_MAX) { > >>>>>>> + dev_err(&ctlr->dev, "No. of CS is more than max. no. > >> of > >>>>>> supported CS\n"); > >>>>>>> + return -EINVAL; > >>>>>>> + } > >>>>>>> + > >>>>>>> + /* > >>>>>>> + * Zero(0) is a valid physical CS value and can be located at > >> any > >>>>>>> + * logical CS in the spi->chip_select[]. If all the physical CS > >>>>>>> + * are initialized to 0 then It would be difficult to differentiate > >>>>>>> + * between a valid physical CS 0 & an unused logical CS whose > >>>>>> physical > >>>>>>> + * CS can be 0. As a solution to this issue initialize all the > >>>>>>> +CS to > >> 0xFF. > >>>>>>> + * Now all the unused logical CS will have 0xFF physical CS > >>>>>>> +value & can > >>>>>> be > >>>>>>> + * ignore while performing physical CS validity checks. > >>>>>>> + */ > >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > >>>>>>> + spi_set_chipselect(spi, idx, 0xFF); > >>>>>>> + > >>>>>>> /* Device address */ > >>>>>>> - rc = of_property_read_u32(nc, "reg", &value); > >>>>>>> - if (rc) { > >>>>>>> + rc = of_property_read_variable_u32_array(nc, "reg", &cs[0], > >> 1, > >>>>>>> + SPI_CS_CNT_MAX); > >>>>>>> + if (rc < 0) { > >>>>>>> dev_err(&ctlr->dev, "%pOF has no valid 'reg' property > >>>>>> (%d)\n", > >>>>>>> nc, rc); > >>>>>>> return rc; > >>>>>>> } > >>>>>>> - spi_set_chipselect(spi, 0, value); > >>>>>>> + if (rc > ctlr->num_chipselect) { > >>>>>>> + dev_err(&ctlr->dev, "%pOF has number of CS > ctlr- > >>>>>>> num_chipselect (%d)\n", > >>>>>>> + nc, rc); > >>>>>>> + return rc; > >>>>>>> + } > >>>>>>> + if ((of_property_read_bool(nc, "parallel-memories")) && > >>>>>>> + (!(ctlr->flags & SPI_CONTROLLER_MULTI_CS))) { > >>>>>>> + dev_err(&ctlr->dev, "SPI controller doesn't support > >> multi > >>>>>> CS\n"); > >>>>>>> + return -EINVAL; > >>>>>>> + } > >>>>>>> + for (idx = 0; idx < rc; idx++) > >>>>>>> + spi_set_chipselect(spi, idx, cs[idx]); > >>>>>>> + > >>>>>>> + /* spi->chip_select[i] gives the corresponding physical CS for > >>>>>>> +logical CS > >>>>>> i > >>>>>>> + * logical CS number is represented by setting the ith bit in > >>>>>>> +spi- > >>>>>>> cs_index_mask > >>>>>>> + * So, for example, if spi->cs_index_mask = 0x01 then logical > >> CS > >>>>>> number is 0 and > >>>>>>> + * spi->chip_select[0] will give the physical CS. > >>>>>>> + * By default spi->chip_select[0] will hold the physical CS > >>>>>>> +number so, > >>>>>> set > >>>>>>> + * spi->cs_index_mask as 0x01. > >>>>>>> + */ > >>>>>>> + spi->cs_index_mask = 0x01; > >>>>>>> > >>>>>>> /* Device speed */ > >>>>>>> if (!of_property_read_u32(nc, "spi-max-frequency", > >>>>>>> &value)) @@ > >>>>>>> -3100,6 +3206,7 @@ int spi_register_controller(struct > >>>>>>> spi_controller > >> *ctlr) > >>>>>>> struct boardinfo *bi; > >>>>>>> int first_dynamic; > >>>>>>> int status; > >>>>>>> + int idx; > >>>>>>> > >>>>>>> if (!dev) > >>>>>>> return -ENODEV; > >>>>>>> @@ -3164,7 +3271,8 @@ int spi_register_controller(struct > >>>>>>> spi_controller > >>>>>> *ctlr) > >>>>>>> } > >>>>>>> > >>>>>>> /* Setting last_cs to -1 means no chip selected */ > >>>>>>> - ctlr->last_cs = -1; > >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > >>>>>>> + ctlr->last_cs[idx] = -1; > >>>>>>> > >>>>>>> status = device_add(&ctlr->dev); > >>>>>>> if (status < 0) > >>>>>>> @@ -3889,7 +3997,7 @@ static int __spi_validate(struct > >>>>>>> spi_device *spi, > >>>>>> struct spi_message *message) > >>>>>>> * cs_change is set for each transfer. > >>>>>>> */ > >>>>>>> if ((spi->mode & SPI_CS_WORD) && (!(ctlr->mode_bits & > >>>>>> SPI_CS_WORD) || > >>>>>>> - spi_get_csgpiod(spi, 0))) { > >>>>>>> + spi_is_csgpiod(spi))) { > >>>>>>> size_t maxsize = BITS_TO_BYTES(spi->bits_per_word); > >>>>>>> int ret; > >>>>>>> > >>>>>>> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h > >>>>>>> index 7b4baff63c5c..871d3a6d879b 100644 > >>>>>>> --- a/include/linux/spi/spi.h > >>>>>>> +++ b/include/linux/spi/spi.h > >>>>>>> @@ -20,6 +20,9 @@ > >>>>>>> > >>>>>>> #include <uapi/linux/spi/spi.h> > >>>>>>> > >>>>>>> +/* Max no. of CS supported per spi device */ #define > >>>>>>> +SPI_CS_CNT_MAX > >>>>>>> +4 > >>>>>>> + > >>>>>>> struct dma_chan; > >>>>>>> struct software_node; > >>>>>>> struct ptp_system_timestamp; @@ -132,7 +135,8 @@ extern > >>>>>>> void > >>>>>> spi_transfer_cs_change_delay_exec(struct spi_message *msg, > >>>>>>> * @max_speed_hz: Maximum clock rate to be used with this chip > >>>>>>> * (on this board); may be changed by the device's driver. > >>>>>>> * The spi_transfer.speed_hz can override this for each transfer. > >>>>>>> - * @chip_select: Chipselect, distinguishing chips handled by > >> @controller. > >>>>>>> + * @chip_select: Array of physical chipselect, spi->chipselect[i] gives > >>>>>>> + * the corresponding physical CS for logical CS i. > >>>>>>> * @mode: The spi mode defines how data is clocked out and in. > >>>>>>> * This may be changed by the device's driver. > >>>>>>> * The "active low" default for chipselect mode can be > >> overridden > >>>>>>> @@ -157,8 +161,8 @@ extern void > >>>>>> spi_transfer_cs_change_delay_exec(struct spi_message *msg, > >>>>>>> * the device will bind to the named driver and only the named > >> driver. > >>>>>>> * Do not set directly, because core frees it; use > >> driver_set_override() to > >>>>>>> * set or clear it. > >>>>>>> - * @cs_gpiod: GPIO descriptor of the chipselect line (optional, > >>>>>>> NULL > >>>> when > >>>>>>> - * not using a GPIO line) > >>>>>>> + * @cs_gpiod: Array of GPIO descriptors of the corresponding > >>>>>>> + chipselect > >>>>>> lines > >>>>>>> + * (optional, NULL when not using a GPIO line) > >>>>>>> * @word_delay: delay to be inserted between consecutive > >>>>>>> * words of a transfer > >>>>>>> * @cs_setup: delay to be introduced by the controller > >>>>>>> after CS is asserted @@ -167,6 +171,7 @@ extern void > >>>>>> spi_transfer_cs_change_delay_exec(struct spi_message *msg, > >>>>>>> * deasserted. If @cs_change_delay is used from @spi_transfer, > >> then > >>>>>> the > >>>>>>> * two delays will be added up. > >>>>>>> * @pcpu_statistics: statistics for the spi_device > >>>>>>> + * @cs_index_mask: Bit mask of the active chipselect(s) in the > >>>>>>> + chipselect array > >>>>>>> * > >>>>>>> * A @spi_device is used to interchange data between an SPI slave > >>>>>>> * (usually a discrete chip) and CPU memory. > >>>>>>> @@ -182,7 +187,7 @@ struct spi_device { > >>>>>>> struct spi_controller *controller; > >>>>>>> struct spi_controller *master; /* Compatibility layer > >> */ > >>>>>>> u32 max_speed_hz; > >>>>>>> - u8 chip_select; > >>>>>>> + u8 chip_select[SPI_CS_CNT_MAX]; > >>>>>>> u8 bits_per_word; > >>>>>>> bool rt; > >>>>>>> #define SPI_NO_TX BIT(31) /* No transmit wire */ > >>>>>>> @@ -213,7 +218,7 @@ struct spi_device { > >>>>>>> void *controller_data; > >>>>>>> char modalias[SPI_NAME_SIZE]; > >>>>>>> const char *driver_override; > >>>>>>> - struct gpio_desc *cs_gpiod; /* Chip select GPIO > descriptor > >>>>>> */ > >>>>>>> + struct gpio_desc *cs_gpiod[SPI_CS_CNT_MAX]; /* > >> Chip select > >>>>>> gpio desc */ > >>>>>>> struct spi_delay word_delay; /* Inter-word delay */ > >>>>>>> /* CS delays */ > >>>>>>> struct spi_delay cs_setup; > >>>>>>> @@ -223,6 +228,13 @@ struct spi_device { > >>>>>>> /* The statistics */ > >>>>>>> struct spi_statistics __percpu *pcpu_statistics; > >>>>>>> > >>>>>>> + /* Bit mask of the chipselect(s) that the driver need to use > >> from > >>>>>>> + * the chipselect array.When the controller is capable to > >> handle > >>>>>>> + * multiple chip selects & memories are connected in parallel > >>>>>>> + * then more than one bit need to be set in cs_index_mask. > >>>>>>> + */ > >>>>>>> + u32 cs_index_mask : SPI_CS_CNT_MAX; > >>>>>>> + > >>>>>>> /* > >>>>>>> * Likely need more hooks for more protocol options > >>>>>>> affecting > >> how > >>>>>>> * the controller talks to each chip, like: > >>>>>>> @@ -279,22 +291,33 @@ static inline void *spi_get_drvdata(const > >>>>>>> struct spi_device *spi) > >>>>>>> > >>>>>>> static inline u8 spi_get_chipselect(const struct spi_device *spi, u8 > idx) > >>>>>>> { > >>>>>>> - return spi->chip_select; > >>>>>>> + return spi->chip_select[idx]; > >>>>>>> } > >>>>>>> > >>>>>>> static inline void spi_set_chipselect(struct spi_device > >>>>>>> *spi, > >>>>>>> u8 idx, u8 > >>>>>> chipselect) > >>>>>>> { > >>>>>>> - spi->chip_select = chipselect; > >>>>>>> + spi->chip_select[idx] = chipselect; > >>>>>>> } > >>>>>>> > >>>>>>> static inline struct gpio_desc *spi_get_csgpiod(const > >>>>>>> struct spi_device *spi, > >>>>>> u8 idx) > >>>>>>> { > >>>>>>> - return spi->cs_gpiod; > >>>>>>> + return spi->cs_gpiod[idx]; > >>>>>>> } > >>>>>>> > >>>>>>> static inline void spi_set_csgpiod(struct spi_device *spi, > >>>>>>> u8 idx, struct > >>>>>> gpio_desc *csgpiod) > >>>>>>> { > >>>>>>> - spi->cs_gpiod = csgpiod; > >>>>>>> + spi->cs_gpiod[idx] = csgpiod; > >>>>>>> +} > >>>>>>> + > >>>>>>> +static inline bool spi_is_csgpiod(struct spi_device *spi) { > >>>>>>> + u8 idx; > >>>>>>> + > >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > >>>>>>> + if (spi_get_csgpiod(spi, idx)) > >>>>>>> + return true; > >>>>>>> + } > >>>>>>> + return false; > >>>>>>> } > >>>>>>> > >>>>>>> /** > >>>>>>> @@ -399,6 +422,8 @@ extern struct spi_device > >>>>>> *spi_new_ancillary_device(struct spi_device *spi, u8 ch > >>>>>>> * @bus_lock_spinlock: spinlock for SPI bus locking > >>>>>>> * @bus_lock_mutex: mutex for exclusion of multiple callers > >>>>>>> * @bus_lock_flag: indicates that the SPI bus is locked for > >>>>>>> exclusive use > >>>>>>> + * @multi_cs_cap: indicates that the SPI Controller can > >>>>>>> + assert/de- > >> assert > >>>>>>> + * more than one chip select at once. > >>>>>>> * @setup: updates the device mode and clocking records used by > a > >>>>>>> * device's SPI controller; protocol code may call this. This > >>>>>>> * must fail if an unrecognized or unsupported mode is > >> requested. > >>>>>>> @@ -567,6 +592,11 @@ struct spi_controller { > >>>>>>> #define SPI_CONTROLLER_MUST_TX BIT(4) /* > Requires tx > >>>> */ > >>>>>>> #define SPI_CONTROLLER_GPIO_SS BIT(5) /* GPIO CS > >>>> must > >>>>>> select slave */ > >>>>>>> #define SPI_CONTROLLER_SUSPENDED BIT(6) /* Currently > >>>>>> suspended */ > >>>>>>> + /* > >>>>>>> + * The spi-controller has multi chip select capability and can > >>>>>>> + * assert/de-assert more than one chip select at once. > >>>>>>> + */ > >>>>>>> +#define SPI_CONTROLLER_MULTI_CS BIT(7) > >>>>>>> > >>>>>>> /* Flag indicating if the allocation of this struct is > >>>>>>> devres- > >> managed */ > >>>>>>> bool devm_allocated; > >>>>>>> @@ -677,7 +707,8 @@ struct spi_controller { > >>>>>>> bool rt; > >>>>>>> bool auto_runtime_pm; > >>>>>>> bool cur_msg_mapped; > >>>>>>> - char last_cs; > >>>>>>> + char last_cs[SPI_CS_CNT_MAX]; > >>>>>>> + char last_cs_index_mask; > >>>>>>> bool last_cs_mode_high; > >>>>>>> bool fallback; > >>>>>>> struct completion xfer_completion;
> -----Original Message----- > From: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com> > Sent: Wednesday, November 22, 2023 1:23 PM > To: Stefan Binding <sbinding@opensource.cirrus.com>; > broonie@kernel.org; tudor.ambarus@linaro.org; pratyush@kernel.org; > miquel.raynal@bootlin.com; richard@nod.at; vigneshr@ti.com > Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; > michael@walle.cc; linux-mtd@lists.infradead.org; > nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com; > claudiu.beznea@tuxon.dev; Simek, Michal <michal.simek@amd.com>; > linux-arm-kernel@lists.infradead.org; git (AMD-Xilinx) <git@amd.com>; > amitrkcian2002@gmail.com; patches@opensource.cirrus.com > Subject: RE: [PATCH v10 1/8] spi: Add multi-cs memories support in SPI > core > > > > > -----Original Message----- > > From: Stefan Binding <sbinding@opensource.cirrus.com> > > Sent: Wednesday, November 22, 2023 4:51 PM > > To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; > > broonie@kernel.org; tudor.ambarus@linaro.org; pratyush@kernel.org; > > miquel.raynal@bootlin.com; richard@nod.at; vigneshr@ti.com > > Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; > > michael@walle.cc; linux-mtd@lists.infradead.org; > > nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com; > > claudiu.beznea@tuxon.dev; Simek, Michal <michal.simek@amd.com>; > linux- > > arm-kernel@lists.infradead.org; git (AMD-Xilinx) <git@amd.com>; > > amitrkcian2002@gmail.com; patches@opensource.cirrus.com > > Subject: Re: [PATCH v10 1/8] spi: Add multi-cs memories support in SPI > core > > > > > > On 21/11/2023 19:18, Mahapatra, Amit Kumar wrote: > > > Hello Stefan, > > > > > >> -----Original Message----- > > >> From: Stefan Binding <sbinding@opensource.cirrus.com> > > >> Sent: Tuesday, November 21, 2023 11:07 PM > > >> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; > > >> broonie@kernel.org; tudor.ambarus@linaro.org; > pratyush@kernel.org; > > >> miquel.raynal@bootlin.com; richard@nod.at; vigneshr@ti.com > > >> Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; > > >> michael@walle.cc; linux-mtd@lists.infradead.org; > > >> nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com; > > >> claudiu.beznea@tuxon.dev; Simek, Michal > <michal.simek@amd.com>; > > >> linux- arm-kernel@lists.infradead.org; git (AMD-Xilinx) > > >> <git@amd.com>; amitrkcian2002@gmail.com; > > >> patches@opensource.cirrus.com > > >> Subject: Re: [PATCH v10 1/8] spi: Add multi-cs memories support in > > >> SPI core > > >> > > >> > > >> On 21/11/2023 16:35, Mahapatra, Amit Kumar wrote: > > >>> Hello Stefan, > > >>> > > >>>> -----Original Message----- > > >>>> From: Stefan Binding <sbinding@opensource.cirrus.com> > > >>>> Sent: Tuesday, November 21, 2023 7:28 PM > > >>>> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; > > >>>> broonie@kernel.org; tudor.ambarus@linaro.org; > pratyush@kernel.org; > > >>>> miquel.raynal@bootlin.com; richard@nod.at; vigneshr@ti.com > > >>>> Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; > > >>>> michael@walle.cc; linux-mtd@lists.infradead.org; > > >>>> nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com; > > >>>> claudiu.beznea@tuxon.dev; Simek, Michal > <michal.simek@amd.com>; > > >>>> linux- arm-kernel@lists.infradead.org; git (AMD-Xilinx) > > >>>> <git@amd.com>; amitrkcian2002@gmail.com; > > >>>> patches@opensource.cirrus.com > > >>>> Subject: Re: [PATCH v10 1/8] spi: Add multi-cs memories support in > > >>>> SPI core > > >>>> > > >>>> > > >>>> On 21/11/2023 07:39, Mahapatra, Amit Kumar wrote: > > >>>>> Hello Stefan, > > >>>>> > > >>>>>> -----Original Message----- > > >>>>>> From: Stefan Binding <sbinding@opensource.cirrus.com> > > >>>>>> Sent: Monday, November 20, 2023 8:00 PM > > >>>>>> To: Mahapatra, Amit Kumar <amit.kumar- > mahapatra@amd.com>; > > >>>>>> broonie@kernel.org; tudor.ambarus@linaro.org; > > >>>>>> pratyush@kernel.org; miquel.raynal@bootlin.com; > richard@nod.at; > > >>>>>> vigneshr@ti.com > > >>>>>> Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; > > >>>>>> michael@walle.cc; linux-mtd@lists.infradead.org; > > >>>>>> nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com; > > >>>>>> claudiu.beznea@tuxon.dev; Simek, Michal > > <michal.simek@amd.com>; > > >>>>>> linux- arm-kernel@lists.infradead.org; git (AMD-Xilinx) > > >>>>>> <git@amd.com>; amitrkcian2002@gmail.com; > > >>>>>> patches@opensource.cirrus.com > > >>>>>> Subject: Re: [PATCH v10 1/8] spi: Add multi-cs memories support > > >>>>>> in SPI core > > >>>>>> > > >>>>>> > > >>>>>> On 18/11/2023 13:54, Amit Kumar Mahapatra wrote: > > >>>>>>> AMD-Xilinx GQSPI controller has two advanced mode that > allows > > >>>>>>> the controller to consider two flashes as one single device. > > >>>>>>> > > >>>>>>> One of these two mode is the parallel mode in which each byte > of > > >>>>>>> data is stored in both devices, the even bits in the lower flash > > >>>>>>> & the odd bits in the upper flash. The byte split is > > >>>>>>> automatically handled by the QSPI controller. > > >>>>>>> > > >>>>>>> The other mode is the stacked mode in which both the flashes > > >>>>>>> share the same SPI bus but each of the device contain half of > the > > data. > > >>>>>>> In this mode, the controller does not follow CS requests but > > >>>>>>> instead internally wires the two CS levels with the value of the > > >>>>>>> most significant > > >>>>>> address bit. > > >>>>>>> For supporting both these modes SPI core need to be updated > for > > >>>>>>> providing multiple CS for a single SPI device. > > >>>>>>> > > >>>>>>> For adding multi CS support the SPI device need to be aware of > > >>>>>>> all the CS values. So, the "chip_select" member in the > > >>>>>>> spi_device structure is now an array that holds all the CS values. > > >>>>>>> > > >>>>>>> spi_device structure now has a "cs_index_mask" member. This > acts > > >>>>>>> as an index to the chip_select array. If nth bit of > > >>>>>>> spi->cs_index_mask is set then the driver would assert spi- > > >>> chip_select[n]. > > >>>>>>> In parallel mode all the chip selects are asserted/de-asserted > > >>>>>>> simultaneously and each byte of data is stored in both devices, > > >>>>>>> the even bits in one, the odd bits in the other. The split is > > >>>>>>> automatically handled by the GQSPI controller. The GQSPI > > >>>>>>> controller supports a maximum of two flashes connected in > > >>>>>>> parallel mode. A SPI_CONTROLLER_MULTI_CS flag bit is added > in > > >>>>>>> the spi controller flags, through ctlr->flags the spi core will > > >>>>>>> make sure that the controller is capable of handling multiple > chip > > selects at once. > > >>>>>>> > > >>>>>>> For supporting multiple CS via GPIO the cs_gpiod member of the > > >>>>>>> spi_device structure is now an array that holds the gpio > > >>>>>>> descriptor for each chipselect. > > >>>>>>> > > >>>>>>> CS GPIO is not tested due to unavailability of necessary > > >>>>>>> hardware > > >> setup. > > >>>>>>> Signed-off-by: Amit Kumar Mahapatra <amit.kumar- > > >>>> mahapatra@amd.com> > > >>>>>>> --- > > >>>>>>> Hello @Stefen, > > >>>>>>> We restructured the SPI core implementation, for handling > > >>>>>>> arbitrary number of devices connected in parallel(multi-cs) or > > >>>>>>> stacked > > >> mode. > > >>>>>>> We have tested this updated patch on native-cs setup but > > >>>>>>> couldn't test cs-gpio due to unavailability of a proper setup. > > >>>>>>> If possible, could you please retest the cs-gpio use case with > > >>>>>>> this updated patch and provide > > >>>>>> your feedback. > > >>>>>> > > >>>>>> Hi, > > >>>>>> > > >>>>>> I tested this chain on 2 different systems using GPIOs as chip > > >>>>>> selects, and see the same error on both: > > >>>>>> [ 2.842045] pxa2xx-spi pxa2xx-spi.6: chipselect 0 already in > > >>>>>> use > > >>>>>> > > >>>>>> Let me know if you need any further testing. > > >>>>>> > > >>>>>> Thanks, > > >>>>>> > > >>>>>> Stefan Binding > > >>>>>> > > >>>>>>> --- > > >>>>>>> drivers/spi/spi.c | 192 > +++++++++++++++++++++++++++++++--- > > ---- > > >> -- > > >>>>>>> include/linux/spi/spi.h | 51 ++++++++--- > > >>>>>>> 2 files changed, 191 insertions(+), 52 deletions(-) > > >>>>>>> > > >>>>>>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index > > >>>>>>> 8ead7acb99f3..ff66147ba95f 100644 > > >>>>>>> --- a/drivers/spi/spi.c > > >>>>>>> +++ b/drivers/spi/spi.c > > >>>>>>> @@ -612,10 +612,21 @@ static int spi_dev_check(struct > device > > >>>>>>> *dev, void > > >>>>>> *data) > > >>>>>>> { > > >>>>>>> struct spi_device *spi = to_spi_device(dev); > > >>>>>>> struct spi_device *new_spi = data; > > >>>>>>> - > > >>>>>>> - if (spi->controller == new_spi->controller && > > >>>>>>> - spi_get_chipselect(spi, 0) == > spi_get_chipselect(new_spi, 0)) > > >>>>>>> - return -EBUSY; > > >>>>>>> + int idx, nw_idx; > > >>>>>>> + u8 cs, cs_nw; > > >>>>>>> + > > >>>>>>> + if (spi->controller == new_spi->controller) { > > >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > > >>>>>>> + cs = spi_get_chipselect(spi, idx); > > >>>>>>> + for (nw_idx = 0; nw_idx < > SPI_CS_CNT_MAX; > > >>>>>> nw_idx++) { > > >>>>>>> + cs_nw = spi_get_chipselect(spi, > > >> nw_idx); > > >>>>> Thank you for dedicating your time to test my patch. > > >>>>> As per my analysis the error is reported from here as we are > > >>>>> supplying the former SPI device structure to retrieve the CS value > > >>>>> for the new SPI devices. > > >>>>> To fix this, could you kindly substitute the above line with > > >>>>> > > >>>>> cs_nw = spi_get_chipselect(new_spi, nw_idx); > > >>>>> > > >>>>> and rerun your tests? > > >>>>> If it works correctly, I will incorporate this fix into my > > >>>>> upcoming series. > > >>>>> > > >>>>> Regards, > > >>>>> Amit > > >> Hi, > > >> > > >> I've attached my log. > > >> I notice that you add a print to of_spi_parse_dt, however since the > > >> laptop I am using is an x86 laptop, it uses ACPI rather than OF, and > > >> I don't think this function isnt even compiled in. > > > That’s correct, I missed it. > > > Upon reviewing the logs, I discovered that in the ACPI flow, I am not > > > initializing the unused CS[] to FF, as I am doing in the OF flow. > > > Consequently, in the ACPI flow, all the unused CS[] are automatically > > > initialized to 0. During the __spi_add_device process, the SPI core > > > faces difficulty distinguishing between a valid CS value of 0 and an > > > unused CS value of 0, leading to a failure in the driver probe. > > > > > > Incorporating the following modifications should resolve this issue. > > > Kindly apply these changes on top of 1/8 and re-run the test. > > > > > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index > > > ff66147ba95f..d0301c17d4f8 100644 > > > --- a/drivers/spi/spi.c > > > +++ b/drivers/spi/spi.c > > > @@ -619,7 +619,7 @@ static int spi_dev_check(struct device *dev, > void > > *data) > > > for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > > > cs = spi_get_chipselect(spi, idx); > > > for (nw_idx = 0; nw_idx < SPI_CS_CNT_MAX; nw_idx++) { > > > - cs_nw = spi_get_chipselect(spi, nw_idx); > > > + cs_nw = spi_get_chipselect(new_spi, > > > + nw_idx); > > > if (cs != 0xFF && cs_nw != 0xFF && cs == cs_nw) { > > > dev_err(dev, "chipselect %d already in use\n", > > cs_nw); > > > return -EBUSY; @@ -764,6 > > > +764,7 @@ struct spi_device *spi_new_device(struct spi_controller > *ctlr, > > > { > > > struct spi_device *proxy; > > > int status; > > > + u8 idx; > > > > > > /* > > > * NOTE: caller did any chip->bus_num checks necessary. > > > @@ -779,6 +780,9 @@ struct spi_device *spi_new_device(struct > > > spi_controller *ctlr, > > > > > > WARN_ON(strlen(chip->modalias) >= sizeof(proxy->modalias)); > > > > > > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > > > + spi_set_chipselect(proxy, idx, 0xFF); > > > + > > > spi_set_chipselect(proxy, 0, chip->chip_select); > > > proxy->max_speed_hz = chip->max_speed_hz; > > > proxy->mode = chip->mode; > > > @@ -2514,6 +2518,7 @@ struct spi_device > > *spi_new_ancillary_device(struct spi_device *spi, > > > struct spi_controller *ctlr = spi->controller; > > > struct spi_device *ancillary; > > > int rc = 0; > > > + u8 idx; > > > > > > /* Alloc an spi_device */ > > > ancillary = spi_alloc_device(ctlr); @@ -2524,6 +2529,9 @@ > > > struct spi_device *spi_new_ancillary_device(struct spi_device *spi, > > > > > > strscpy(ancillary->modalias, "dummy", > > > sizeof(ancillary->modalias)); > > > > > > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > > > + spi_set_chipselect(ancillary, idx, 0xFF); > > > + > > > /* Use provided chip-select for ancillary device */ > > > spi_set_chipselect(ancillary, 0, chip_select); > > > > > > @@ -2732,6 +2740,7 @@ struct spi_device > *acpi_spi_device_alloc(struct > > spi_controller *ctlr, > > > struct acpi_spi_lookup lookup = {}; > > > struct spi_device *spi; > > > int ret; > > > + u8 idx; > > > > > > if (!ctlr && index == -1) > > > return ERR_PTR(-EINVAL); @@ -2767,6 +2776,9 @@ struct > > > spi_device *acpi_spi_device_alloc(struct spi_controller *ctlr, > > > return ERR_PTR(-ENOMEM); > > > } > > > > > > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > > > + spi_set_chipselect(spi, idx, 0xFF); > > > + > > > ACPI_COMPANION_SET(&spi->dev, adev); > > > spi->max_speed_hz = lookup.max_speed_hz; > > > spi->mode |= lookup.mode; > > > > > > Regards, > > > Amit > > > > > > > Hi, > > > > I no longer see that error, or any errors printed from SPI, however, all of > the > > transactions are now broken. > > Every transaction seems to read back 0x0, on all SPI devices, whether > native > > CS or GPIO CS, and I dont see it toggling the CS GPIO anymore. > > I believe the issue stems from the fact that in the ACPI patch, > spi->cs_index_mask is not set to 0x01. As a result, in spi_set_cs, > it fails to meet the condition that governs the CS asset code snippet. > In the following code diff, I've included the necessary fix along with > the earlier changes on top of 1/8. I've also included a couple of debug > prints to aid in further debugging in case my analysis is incorrect. > Please apply these changes on top of 1/8 and re-run the test. > Hi, I've tested this on several laptops, and it all seems to work now. Tested-by: Stefan Binding <sbinding@opensource.cirrus.com> Thanks, Stefan > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index ff66147ba95f..b2e035c3d494 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -619,7 +619,7 @@ static int spi_dev_check(struct device *dev, void > *data) > for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > cs = spi_get_chipselect(spi, idx); > for (nw_idx = 0; nw_idx < SPI_CS_CNT_MAX; nw_idx++) { > - cs_nw = spi_get_chipselect(spi, nw_idx); > + cs_nw = spi_get_chipselect(new_spi, nw_idx); > if (cs != 0xFF && cs_nw != 0xFF && cs == cs_nw) { > dev_err(dev, "chipselect %d already in use\n", > cs_nw); > return -EBUSY; > @@ -764,6 +764,7 @@ struct spi_device *spi_new_device(struct > spi_controller *ctlr, > { > struct spi_device *proxy; > int status; > + u8 idx; > > /* > * NOTE: caller did any chip->bus_num checks necessary. > @@ -779,6 +780,9 @@ struct spi_device *spi_new_device(struct > spi_controller *ctlr, > > WARN_ON(strlen(chip->modalias) >= sizeof(proxy->modalias)); > > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > + spi_set_chipselect(proxy, idx, 0xFF); > + > spi_set_chipselect(proxy, 0, chip->chip_select); > proxy->max_speed_hz = chip->max_speed_hz; > proxy->mode = chip->mode; > @@ -787,6 +791,7 @@ struct spi_device *spi_new_device(struct > spi_controller *ctlr, > proxy->dev.platform_data = (void *) chip->platform_data; > proxy->controller_data = chip->controller_data; > proxy->controller_state = NULL; > + proxy->cs_index_mask = 0x01; > > if (chip->swnode) { > status = device_add_software_node(&proxy->dev, chip- > >swnode); > @@ -1024,6 +1029,8 @@ static void spi_set_cs(struct spi_device *spi, > bool enable, bool force) > if (!spi->controller->set_cs_timing && !activate) > spi_delay_exec(&spi->cs_hold, NULL); > > + printk("%s() [%d] spi->cs_index_mask == [%d]\n",__func__, > __LINE__, spi->cs_index_mask); > + > if (!(spi->mode & SPI_NO_CS)) { > /* > * Historically ACPI has no means of the GPIO polarity and > @@ -1038,6 +1045,7 @@ static void spi_set_cs(struct spi_device *spi, > bool enable, bool force) > for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > if (((spi->cs_index_mask >> idx) & 0x01) && > spi_get_csgpiod(spi, idx)) { > + printk("%s() [%d] idx == [%d]\n",__func__, __LINE__, > idx); > if (has_acpi_companion(&spi->dev)) > gpiod_set_value_cansleep(spi_get_csgpiod(spi, > idx), > !enable); > @@ -2514,6 +2522,7 @@ struct spi_device > *spi_new_ancillary_device(struct spi_device *spi, > struct spi_controller *ctlr = spi->controller; > struct spi_device *ancillary; > int rc = 0; > + u8 idx; > > /* Alloc an spi_device */ > ancillary = spi_alloc_device(ctlr); > @@ -2524,12 +2533,16 @@ struct spi_device > *spi_new_ancillary_device(struct spi_device *spi, > > strscpy(ancillary->modalias, "dummy", sizeof(ancillary->modalias)); > > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > + spi_set_chipselect(ancillary, idx, 0xFF); > + > /* Use provided chip-select for ancillary device */ > spi_set_chipselect(ancillary, 0, chip_select); > > /* Take over SPI mode/speed from SPI main device */ > ancillary->max_speed_hz = spi->max_speed_hz; > ancillary->mode = spi->mode; > + ancillary->cs_index_mask = 0x01; > > WARN_ON(!mutex_is_locked(&ctlr->add_lock)); > > @@ -2732,6 +2745,7 @@ struct spi_device *acpi_spi_device_alloc(struct > spi_controller *ctlr, > struct acpi_spi_lookup lookup = {}; > struct spi_device *spi; > int ret; > + u8 idx; > > if (!ctlr && index == -1) > return ERR_PTR(-EINVAL); > @@ -2767,12 +2781,16 @@ struct spi_device > *acpi_spi_device_alloc(struct spi_controller *ctlr, > return ERR_PTR(-ENOMEM); > } > > + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > + spi_set_chipselect(spi, idx, 0xFF); > + > ACPI_COMPANION_SET(&spi->dev, adev); > spi->max_speed_hz = lookup.max_speed_hz; > spi->mode |= lookup.mode; > spi->irq = lookup.irq; > spi->bits_per_word = lookup.bits_per_word; > spi_set_chipselect(spi, 0, lookup.chip_select); > + spi->cs_index_mask = 0x01; > > return spi; > } > > Regards, > Amit > > > > > Thanks, > > > > Stefan > > > > >> Thanks, > > >> > > >> Stefan > > >> > > >>>> Hi, > > >>>> > > >>>> I still see the same error: > > >>>> > > >>>> [ 2.748546] pxa2xx-spi pxa2xx-spi.6: chipselect 0 already in use > > >>> Thank you for the quick testing. For further analysis I have > > >>> incorporated additional debug prints on top of 1/8 patch. The > > >>> corresponding diff is shared below. Kindly implement the specified > > >>> changes, rerun your test and share the kernel logs. > > >>> > > >>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index > > >>> ff66147ba95f..7f59ea81593d 100644 > > >>> --- a/drivers/spi/spi.c > > >>> +++ b/drivers/spi/spi.c > > >>> @@ -618,8 +618,10 @@ static int spi_dev_check(struct device *dev, > > >>> void > > >> *data) > > >>> if (spi->controller == new_spi->controller) { > > >>> for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > > >>> cs = spi_get_chipselect(spi, idx); > > >>> + printk("%s() [%d] CS[%d] = [%d]\n", > > >>> + __func__, __LINE__, idx, cs); > > >>> for (nw_idx = 0; nw_idx < SPI_CS_CNT_MAX; > nw_idx++) { > > >>> - cs_nw = spi_get_chipselect(spi, nw_idx); > > >>> + cs_nw = spi_get_chipselect(new_spi, nw_idx); > > >>> + printk("%s() [%d] CS_NEW[%d] = > > >>> + [%d]\n", __func__, __LINE__, nw_idx, cs_nw); > > >>> if (cs != 0xFF && cs_nw != 0xFF && cs == cs_nw) { > > >>> dev_err(dev, "chipselect > > >>> %d already in use\n", > > >> cs_nw); > > >>> return -EBUSY; @@ -659,8 > > >>> +661,10 @@ static int __spi_add_device(struct spi_device *spi) > > >>> */ > > >>> for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > > >>> cs = spi_get_chipselect(spi, idx); > > >>> + printk("%s() [%d] CS[%d] = [%d]\n", __func__, > > >>> + __LINE__, idx, cs); > > >>> for (nw_idx = idx + 1; nw_idx < SPI_CS_CNT_MAX; > nw_idx++) { > > >>> nw_cs = spi_get_chipselect(spi, nw_idx); > > >>> + printk("%s() [%d] CS_NEW[%d] = [%d]\n", > > >>> + __func__, __LINE__, nw_idx, nw_cs); > > >>> if (cs != 0xFF && nw_cs != 0xFF && cs == nw_cs) { > > >>> dev_err(dev, "chipselect %d already in use\n", > nw_cs); > > >>> return -EBUSY; @@ -2401,6 +2405,9 > > >>> @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct > spi_device > > *spi, > > >>> for (idx = 0; idx < rc; idx++) > > >>> spi_set_chipselect(spi, idx, cs[idx]); > > >>> > > >>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > > >>> + printk("%s() [%d] CS[%d] = [%d]\n", __func__, > > >>> + __LINE__, idx, spi_get_chipselect(spi, idx)); > > >>> + > > >>> /* spi->chip_select[i] gives the corresponding physical CS for > logical > > CS i > > >>> * logical CS number is represented by setting the ith bit > > >>> in spi- cs_index_mask > > >>> * So, for example, if spi->cs_index_mask = 0x01 then > > >>> logical CS number is 0 and > > >>> > > >>> Regards, > > >>> Amit > > >>> > > >>>> Thanks, > > >>>> > > >>>> Stefan > > >>>> > > >>>>>>> + if (cs != 0xFF && cs_nw != 0xFF > && cs > > >> == > > >>>>>> cs_nw) { > > >>>>>>> + dev_err(dev, "chipselect > %d > > >> already in > > >>>>>> use\n", cs_nw); > > >>>>>>> + return -EBUSY; > > >>>>>>> + } > > >>>>>>> + } > > >>>>>>> + } > > >>>>>>> + } > > >>>>>>> return 0; > > >>>>>>> } > > >>>>>>> > > >>>>>>> @@ -629,13 +640,32 @@ static int __spi_add_device(struct > > >>>>>>> spi_device > > >>>> *spi) > > >>>>>>> { > > >>>>>>> struct spi_controller *ctlr = spi->controller; > > >>>>>>> struct device *dev = ctlr->dev.parent; > > >>>>>>> - int status; > > >>>>>>> + int status, idx, nw_idx; > > >>>>>>> + u8 cs, nw_cs; > > >>>>>>> + > > >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > > >>>>>>> + /* Chipselects are numbered 0..max; validate. */ > > >>>>>>> + cs = spi_get_chipselect(spi, idx); > > >>>>>>> + if (cs != 0xFF && cs >= ctlr->num_chipselect) { > > >>>>>>> + dev_err(dev, "cs%d >= max %d\n", > > >>>>>> spi_get_chipselect(spi, idx), > > >>>>>>> + ctlr->num_chipselect); > > >>>>>>> + return -EINVAL; > > >>>>>>> + } > > >>>>>>> + } > > >>>>>>> > > >>>>>>> - /* Chipselects are numbered 0..max; validate. */ > > >>>>>>> - if (spi_get_chipselect(spi, 0) >= ctlr->num_chipselect) { > > >>>>>>> - dev_err(dev, "cs%d >= max %d\n", > > spi_get_chipselect(spi, 0), > > >>>>>>> - ctlr->num_chipselect); > > >>>>>>> - return -EINVAL; > > >>>>>>> + /* > > >>>>>>> + * Make sure that multiple logical CS doesn't map to the > same > > >>>>>> physical CS. > > >>>>>>> + * For example, spi->chip_select[0] != spi->chip_select[1] > and > > >> so on. > > >>>>>>> + */ > > >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > > >>>>>>> + cs = spi_get_chipselect(spi, idx); > > >>>>>>> + for (nw_idx = idx + 1; nw_idx < SPI_CS_CNT_MAX; > > >> nw_idx++) { > > >>>>>>> + nw_cs = spi_get_chipselect(spi, nw_idx); > > >>>>>>> + if (cs != 0xFF && nw_cs != 0xFF && cs == > > >> nw_cs) { > > >>>>>>> + dev_err(dev, "chipselect %d > already in > > >> use\n", > > >>>>>> nw_cs); > > >>>>>>> + return -EBUSY; > > >>>>>>> + } > > >>>>>>> + } > > >>>>>>> } > > >>>>>>> > > >>>>>>> /* Set the bus ID string */ @@ -647,11 +677,8 @@ static > > >>>>>>> int __spi_add_device(struct spi_device > > >>>> *spi) > > >>>>>>> * its configuration. > > >>>>>>> */ > > >>>>>>> status = bus_for_each_dev(&spi_bus_type, NULL, spi, > > >> spi_dev_check); > > >>>>>>> - if (status) { > > >>>>>>> - dev_err(dev, "chipselect %d already in use\n", > > >>>>>>> - spi_get_chipselect(spi, 0)); > > >>>>>>> + if (status) > > >>>>>>> return status; > > >>>>>>> - } > > >>>>>>> > > >>>>>>> /* Controller may unregister concurrently */ > > >>>>>>> if (IS_ENABLED(CONFIG_SPI_DYNAMIC) && @@ -659,8 > > >> +686,15 @@ > > >>>>>> static > > >>>>>>> int __spi_add_device(struct spi_device *spi) > > >>>>>>> return -ENODEV; > > >>>>>>> } > > >>>>>>> > > >>>>>>> - if (ctlr->cs_gpiods) > > >>>>>>> - spi_set_csgpiod(spi, 0, ctlr- > > >cs_gpiods[spi_get_chipselect(spi, > > >>>>>> 0)]); > > >>>>>>> + if (ctlr->cs_gpiods) { > > >>>>>>> + u8 cs; > > >>>>>>> + > > >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > > >>>>>>> + cs = spi_get_chipselect(spi, idx); > > >>>>>>> + if (cs != 0xFF) > > >>>>>>> + spi_set_csgpiod(spi, idx, ctlr- > > >>> cs_gpiods[cs]); > > >>>>>>> + } > > >>>>>>> + } > > >>>>>>> > > >>>>>>> /* > > >>>>>>> * Drivers may modify this initial i/o setup, but will @@ > > >>>>>>> -701,6 > > >>>>>>> +735,9 @@ int spi_add_device(struct spi_device *spi) > > >>>>>>> struct spi_controller *ctlr = spi->controller; > > >>>>>>> int status; > > >>>>>>> > > >>>>>>> + /* Set the bus ID string */ > > >>>>>>> + spi_dev_set_name(spi); > > >>>>>>> + > > >>>>>>> mutex_lock(&ctlr->add_lock); > > >>>>>>> status = __spi_add_device(spi); > > >>>>>>> mutex_unlock(&ctlr->add_lock); @@ -942,32 +979,51 > @@ > > >> static > > >>>>>>> void spi_res_release(struct spi_controller > > >>>>>> *ctlr, struct spi_message *mes > > >>>>>>> } > > >>>>>>> > > >>>>>>> > > >>>>>>> /*-------------------------------------------------------------- > > >>>>>>> -- > > >>>>>>> -- > > >>>>>>> -- > > >>>>>>> -----*/ > > >>>>>>> +static inline bool spi_is_last_cs(struct spi_device *spi) { > > >>>>>>> + u8 idx; > > >>>>>>> + bool last = false; > > >>>>>>> + > > >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > > >>>>>>> + if ((spi->cs_index_mask >> idx) & 0x01) { > > >>>>>>> + if (spi->controller->last_cs[idx] == > > >>>>>> spi_get_chipselect(spi, idx)) > > >>>>>>> + last = true; > > >>>>>>> + } > > >>>>>>> + } > > >>>>>>> + return last; > > >>>>>>> +} > > >>>>>>> + > > >>>>>>> > > >>>>>>> static void spi_set_cs(struct spi_device *spi, bool enable, > bool > > force) > > >>>>>>> { > > >>>>>>> bool activate = enable; > > >>>>>>> + u8 idx; > > >>>>>>> > > >>>>>>> /* > > >>>>>>> * Avoid calling into the driver (or doing delays) if the > > >>>>>>> chip > > >> select > > >>>>>>> * isn't actually changing from the last time this was > called. > > >>>>>>> */ > > >>>>>>> - if (!force && ((enable && spi->controller->last_cs == > > >>>>>> spi_get_chipselect(spi, 0)) || > > >>>>>>> - (!enable && spi->controller->last_cs != > > >>>>>> spi_get_chipselect(spi, 0))) && > > >>>>>>> + if (!force && ((enable && spi->controller- > >last_cs_index_mask > > >> == > > >>>>>>> +spi- > > >>>>>>> cs_index_mask && > > >>>>>>> + spi_is_last_cs(spi)) || > > >>>>>>> + (!enable && spi->controller- > >last_cs_index_mask > > >> == spi- > > >>>>>>> cs_index_mask && > > >>>>>>> + !spi_is_last_cs(spi))) && > > >>>>>>> (spi->controller->last_cs_mode_high == (spi->mode & > > >>>>>> SPI_CS_HIGH))) > > >>>>>>> return; > > >>>>>>> > > >>>>>>> trace_spi_set_cs(spi, activate); > > >>>>>>> > > >>>>>>> - spi->controller->last_cs = enable ? spi_get_chipselect(spi, > 0) : - > > 1; > > >>>>>>> + spi->controller->last_cs_index_mask = spi- > >cs_index_mask; > > >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > > >>>>>>> + spi->controller->last_cs[idx] = enable ? > > >>>>>>> +spi_get_chipselect(spi, > > >>>>>> 0) > > >>>>>>> +: -1; > > >>>>>>> spi->controller->last_cs_mode_high = spi->mode & > > >> SPI_CS_HIGH; > > >>>>>>> - if ((spi_get_csgpiod(spi, 0) || !spi->controller- > >set_cs_timing) > > && > > >>>>>> !activate) > > >>>>>>> - spi_delay_exec(&spi->cs_hold, NULL); > > >>>>>>> - > > >>>>>>> if (spi->mode & SPI_CS_HIGH) > > >>>>>>> enable = !enable; > > >>>>>>> > > >>>>>>> - if (spi_get_csgpiod(spi, 0)) { > > >>>>>>> + if (spi_is_csgpiod(spi)) { > > >>>>>>> + if (!spi->controller->set_cs_timing && !activate) > > >>>>>>> + spi_delay_exec(&spi->cs_hold, NULL); > > >>>>>>> + > > >>>>>>> if (!(spi->mode & SPI_NO_CS)) { > > >>>>>>> /* > > >>>>>>> * Historically ACPI has no means of the > GPIO > > >> polarity > > >>>>>> and @@ > > >>>>>>> -979,26 +1035,38 @@ static void spi_set_cs(struct spi_device > > >>>>>>> *spi, bool > > >>>>>> enable, bool force) > > >>>>>>> * ambiguity. That's why we use enable, > that > > >> takes > > >>>>>> SPI_CS_HIGH > > >>>>>>> * into account. > > >>>>>>> */ > > >>>>>>> - if (has_acpi_companion(&spi->dev)) > > >>>>>>> - > > >>>>>> gpiod_set_value_cansleep(spi_get_csgpiod(spi, 0), > !enable); > > >>>>>>> - else > > >>>>>>> - /* Polarity handled by GPIO > library */ > > >>>>>>> - > > >>>>>> gpiod_set_value_cansleep(spi_get_csgpiod(spi, 0), > activate); > > >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; > idx++) { > > >>>>>>> + if (((spi->cs_index_mask >> idx) > & > > >> 0x01) && > > >>>>>>> + spi_get_csgpiod(spi, idx)) { > > >>>>>>> + if > (has_acpi_companion(&spi- > > >>> dev)) > > >>>>>>> + > > >>>>>> gpiod_set_value_cansleep(spi_get_csgpiod(spi, idx), > > >>>>>>> + > > >>>>>> !enable); > > >>>>>>> + else > > >>>>>>> + /* Polarity > handled by > > >> GPIO > > >>>>>> library */ > > >>>>>>> + > > >>>>>> gpiod_set_value_cansleep(spi_get_csgpiod(spi, idx), > > >>>>>>> + > > >>>>>> activate); > > >>>>>>> + > > >>>>>>> + if (activate) > > >>>>>>> + > spi_delay_exec(&spi- > > >>>>>>> cs_setup, NULL); > > >>>>>>> + else > > >>>>>>> + > spi_delay_exec(&spi- > > >>>>>>> cs_inactive, NULL); > > >>>>>>> + } > > >>>>>>> + } > > >>>>>>> } > > >>>>>>> /* Some SPI masters need both GPIO CS & > > >> slave_select */ > > >>>>>>> if ((spi->controller->flags & > > >> SPI_CONTROLLER_GPIO_SS) && > > >>>>>>> spi->controller->set_cs) > > >>>>>>> spi->controller->set_cs(spi, !enable); > > >>>>>>> + > > >>>>>>> + if (!spi->controller->set_cs_timing) { > > >>>>>>> + if (activate) > > >>>>>>> + spi_delay_exec(&spi->cs_setup, > > >> NULL); > > >>>>>>> + else > > >>>>>>> + spi_delay_exec(&spi- > >cs_inactive, > > >> NULL); > > >>>>>>> + } > > >>>>>>> } else if (spi->controller->set_cs) { > > >>>>>>> spi->controller->set_cs(spi, !enable); > > >>>>>>> } > > >>>>>>> - > > >>>>>>> - if (spi_get_csgpiod(spi, 0) || !spi->controller- > >set_cs_timing) { > > >>>>>>> - if (activate) > > >>>>>>> - spi_delay_exec(&spi->cs_setup, NULL); > > >>>>>>> - else > > >>>>>>> - spi_delay_exec(&spi->cs_inactive, NULL); > > >>>>>>> - } > > >>>>>>> } > > >>>>>>> > > >>>>>>> #ifdef CONFIG_HAS_DMA > > >>>>>>> @@ -2222,8 +2290,8 @@ static void > > >>>>>>> of_spi_parse_dt_cs_delay(struct > > >>>>>> device_node *nc, > > >>>>>>> static int of_spi_parse_dt(struct spi_controller *ctlr, > > >>>>>>> struct spi_device > > >>>> *spi, > > >>>>>>> struct device_node *nc) > > >>>>>>> { > > >>>>>>> - u32 value; > > >>>>>>> - int rc; > > >>>>>>> + u32 value, cs[SPI_CS_CNT_MAX]; > > >>>>>>> + int rc, idx; > > >>>>>>> > > >>>>>>> /* Mode (clock phase/polarity/etc.) */ > > >>>>>>> if (of_property_read_bool(nc, "spi-cpha")) @@ -2295,14 > > >>>>>>> +2363,52 > > >>>>>> @@ > > >>>>>>> static int of_spi_parse_dt(struct spi_controller *ctlr, struct > > >>>>>>> spi_device > > >> *spi, > > >>>>>>> return 0; > > >>>>>>> } > > >>>>>>> > > >>>>>>> + if (ctlr->num_chipselect > SPI_CS_CNT_MAX) { > > >>>>>>> + dev_err(&ctlr->dev, "No. of CS is more than max. > no. > > >> of > > >>>>>> supported CS\n"); > > >>>>>>> + return -EINVAL; > > >>>>>>> + } > > >>>>>>> + > > >>>>>>> + /* > > >>>>>>> + * Zero(0) is a valid physical CS value and can be located at > > >> any > > >>>>>>> + * logical CS in the spi->chip_select[]. If all the physical CS > > >>>>>>> + * are initialized to 0 then It would be difficult to > differentiate > > >>>>>>> + * between a valid physical CS 0 & an unused logical CS > whose > > >>>>>> physical > > >>>>>>> + * CS can be 0. As a solution to this issue initialize all the > > >>>>>>> +CS to > > >> 0xFF. > > >>>>>>> + * Now all the unused logical CS will have 0xFF physical CS > > >>>>>>> +value & can > > >>>>>> be > > >>>>>>> + * ignore while performing physical CS validity checks. > > >>>>>>> + */ > > >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > > >>>>>>> + spi_set_chipselect(spi, idx, 0xFF); > > >>>>>>> + > > >>>>>>> /* Device address */ > > >>>>>>> - rc = of_property_read_u32(nc, "reg", &value); > > >>>>>>> - if (rc) { > > >>>>>>> + rc = of_property_read_variable_u32_array(nc, "reg", > &cs[0], > > >> 1, > > >>>>>>> + > SPI_CS_CNT_MAX); > > >>>>>>> + if (rc < 0) { > > >>>>>>> dev_err(&ctlr->dev, "%pOF has no valid 'reg' > property > > >>>>>> (%d)\n", > > >>>>>>> nc, rc); > > >>>>>>> return rc; > > >>>>>>> } > > >>>>>>> - spi_set_chipselect(spi, 0, value); > > >>>>>>> + if (rc > ctlr->num_chipselect) { > > >>>>>>> + dev_err(&ctlr->dev, "%pOF has number of CS > > ctlr- > > >>>>>>> num_chipselect (%d)\n", > > >>>>>>> + nc, rc); > > >>>>>>> + return rc; > > >>>>>>> + } > > >>>>>>> + if ((of_property_read_bool(nc, "parallel-memories")) && > > >>>>>>> + (!(ctlr->flags & SPI_CONTROLLER_MULTI_CS))) { > > >>>>>>> + dev_err(&ctlr->dev, "SPI controller doesn't > support > > >> multi > > >>>>>> CS\n"); > > >>>>>>> + return -EINVAL; > > >>>>>>> + } > > >>>>>>> + for (idx = 0; idx < rc; idx++) > > >>>>>>> + spi_set_chipselect(spi, idx, cs[idx]); > > >>>>>>> + > > >>>>>>> + /* spi->chip_select[i] gives the corresponding physical CS > for > > >>>>>>> +logical CS > > >>>>>> i > > >>>>>>> + * logical CS number is represented by setting the ith bit in > > >>>>>>> +spi- > > >>>>>>> cs_index_mask > > >>>>>>> + * So, for example, if spi->cs_index_mask = 0x01 then > logical > > >> CS > > >>>>>> number is 0 and > > >>>>>>> + * spi->chip_select[0] will give the physical CS. > > >>>>>>> + * By default spi->chip_select[0] will hold the physical CS > > >>>>>>> +number so, > > >>>>>> set > > >>>>>>> + * spi->cs_index_mask as 0x01. > > >>>>>>> + */ > > >>>>>>> + spi->cs_index_mask = 0x01; > > >>>>>>> > > >>>>>>> /* Device speed */ > > >>>>>>> if (!of_property_read_u32(nc, "spi-max-frequency", > > >>>>>>> &value)) @@ > > >>>>>>> -3100,6 +3206,7 @@ int spi_register_controller(struct > > >>>>>>> spi_controller > > >> *ctlr) > > >>>>>>> struct boardinfo *bi; > > >>>>>>> int first_dynamic; > > >>>>>>> int status; > > >>>>>>> + int idx; > > >>>>>>> > > >>>>>>> if (!dev) > > >>>>>>> return -ENODEV; > > >>>>>>> @@ -3164,7 +3271,8 @@ int spi_register_controller(struct > > >>>>>>> spi_controller > > >>>>>> *ctlr) > > >>>>>>> } > > >>>>>>> > > >>>>>>> /* Setting last_cs to -1 means no chip selected */ > > >>>>>>> - ctlr->last_cs = -1; > > >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) > > >>>>>>> + ctlr->last_cs[idx] = -1; > > >>>>>>> > > >>>>>>> status = device_add(&ctlr->dev); > > >>>>>>> if (status < 0) > > >>>>>>> @@ -3889,7 +3997,7 @@ static int __spi_validate(struct > > >>>>>>> spi_device *spi, > > >>>>>> struct spi_message *message) > > >>>>>>> * cs_change is set for each transfer. > > >>>>>>> */ > > >>>>>>> if ((spi->mode & SPI_CS_WORD) && (!(ctlr->mode_bits & > > >>>>>> SPI_CS_WORD) || > > >>>>>>> - spi_get_csgpiod(spi, 0))) > { > > >>>>>>> + spi_is_csgpiod(spi))) { > > >>>>>>> size_t maxsize = BITS_TO_BYTES(spi- > >bits_per_word); > > >>>>>>> int ret; > > >>>>>>> > > >>>>>>> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h > > >>>>>>> index 7b4baff63c5c..871d3a6d879b 100644 > > >>>>>>> --- a/include/linux/spi/spi.h > > >>>>>>> +++ b/include/linux/spi/spi.h > > >>>>>>> @@ -20,6 +20,9 @@ > > >>>>>>> > > >>>>>>> #include <uapi/linux/spi/spi.h> > > >>>>>>> > > >>>>>>> +/* Max no. of CS supported per spi device */ #define > > >>>>>>> +SPI_CS_CNT_MAX > > >>>>>>> +4 > > >>>>>>> + > > >>>>>>> struct dma_chan; > > >>>>>>> struct software_node; > > >>>>>>> struct ptp_system_timestamp; @@ -132,7 +135,8 @@ > extern > > >>>>>>> void > > >>>>>> spi_transfer_cs_change_delay_exec(struct spi_message *msg, > > >>>>>>> * @max_speed_hz: Maximum clock rate to be used with this > chip > > >>>>>>> * (on this board); may be changed by the device's driver. > > >>>>>>> * The spi_transfer.speed_hz can override this for each > transfer. > > >>>>>>> - * @chip_select: Chipselect, distinguishing chips handled by > > >> @controller. > > >>>>>>> + * @chip_select: Array of physical chipselect, spi->chipselect[i] > gives > > >>>>>>> + * the corresponding physical CS for logical CS i. > > >>>>>>> * @mode: The spi mode defines how data is clocked out and > in. > > >>>>>>> * This may be changed by the device's driver. > > >>>>>>> * The "active low" default for chipselect mode can be > > >> overridden > > >>>>>>> @@ -157,8 +161,8 @@ extern void > > >>>>>> spi_transfer_cs_change_delay_exec(struct spi_message *msg, > > >>>>>>> * the device will bind to the named driver and only the > named > > >> driver. > > >>>>>>> * Do not set directly, because core frees it; use > > >> driver_set_override() to > > >>>>>>> * set or clear it. > > >>>>>>> - * @cs_gpiod: GPIO descriptor of the chipselect line (optional, > > >>>>>>> NULL > > >>>> when > > >>>>>>> - * not using a GPIO line) > > >>>>>>> + * @cs_gpiod: Array of GPIO descriptors of the corresponding > > >>>>>>> + chipselect > > >>>>>> lines > > >>>>>>> + * (optional, NULL when not using a GPIO line) > > >>>>>>> * @word_delay: delay to be inserted between consecutive > > >>>>>>> * words of a transfer > > >>>>>>> * @cs_setup: delay to be introduced by the controller > > >>>>>>> after CS is asserted @@ -167,6 +171,7 @@ extern void > > >>>>>> spi_transfer_cs_change_delay_exec(struct spi_message *msg, > > >>>>>>> * deasserted. If @cs_change_delay is used from > @spi_transfer, > > >> then > > >>>>>> the > > >>>>>>> * two delays will be added up. > > >>>>>>> * @pcpu_statistics: statistics for the spi_device > > >>>>>>> + * @cs_index_mask: Bit mask of the active chipselect(s) in the > > >>>>>>> + chipselect array > > >>>>>>> * > > >>>>>>> * A @spi_device is used to interchange data between an SPI > slave > > >>>>>>> * (usually a discrete chip) and CPU memory. > > >>>>>>> @@ -182,7 +187,7 @@ struct spi_device { > > >>>>>>> struct spi_controller *controller; > > >>>>>>> struct spi_controller *master; /* Compatibility > layer > > >> */ > > >>>>>>> u32 max_speed_hz; > > >>>>>>> - u8 chip_select; > > >>>>>>> + u8 chip_select[SPI_CS_CNT_MAX]; > > >>>>>>> u8 bits_per_word; > > >>>>>>> bool rt; > > >>>>>>> #define SPI_NO_TX BIT(31) /* No > transmit wire */ > > >>>>>>> @@ -213,7 +218,7 @@ struct spi_device { > > >>>>>>> void *controller_data; > > >>>>>>> char modalias[SPI_NAME_SIZE]; > > >>>>>>> const char *driver_override; > > >>>>>>> - struct gpio_desc *cs_gpiod; /* Chip select GPIO > > descriptor > > >>>>>> */ > > >>>>>>> + struct gpio_desc *cs_gpiod[SPI_CS_CNT_MAX]; /* > > >> Chip select > > >>>>>> gpio desc */ > > >>>>>>> struct spi_delay word_delay; /* Inter-word delay */ > > >>>>>>> /* CS delays */ > > >>>>>>> struct spi_delay cs_setup; > > >>>>>>> @@ -223,6 +228,13 @@ struct spi_device { > > >>>>>>> /* The statistics */ > > >>>>>>> struct spi_statistics __percpu *pcpu_statistics; > > >>>>>>> > > >>>>>>> + /* Bit mask of the chipselect(s) that the driver need to use > > >> from > > >>>>>>> + * the chipselect array.When the controller is capable to > > >> handle > > >>>>>>> + * multiple chip selects & memories are connected in > parallel > > >>>>>>> + * then more than one bit need to be set in > cs_index_mask. > > >>>>>>> + */ > > >>>>>>> + u32 cs_index_mask : > SPI_CS_CNT_MAX; > > >>>>>>> + > > >>>>>>> /* > > >>>>>>> * Likely need more hooks for more protocol options > > >>>>>>> affecting > > >> how > > >>>>>>> * the controller talks to each chip, like: > > >>>>>>> @@ -279,22 +291,33 @@ static inline void > *spi_get_drvdata(const > > >>>>>>> struct spi_device *spi) > > >>>>>>> > > >>>>>>> static inline u8 spi_get_chipselect(const struct spi_device > *spi, u8 > > idx) > > >>>>>>> { > > >>>>>>> - return spi->chip_select; > > >>>>>>> + return spi->chip_select[idx]; > > >>>>>>> } > > >>>>>>> > > >>>>>>> static inline void spi_set_chipselect(struct spi_device > > >>>>>>> *spi, > > >>>>>>> u8 idx, u8 > > >>>>>> chipselect) > > >>>>>>> { > > >>>>>>> - spi->chip_select = chipselect; > > >>>>>>> + spi->chip_select[idx] = chipselect; > > >>>>>>> } > > >>>>>>> > > >>>>>>> static inline struct gpio_desc *spi_get_csgpiod(const > > >>>>>>> struct spi_device *spi, > > >>>>>> u8 idx) > > >>>>>>> { > > >>>>>>> - return spi->cs_gpiod; > > >>>>>>> + return spi->cs_gpiod[idx]; > > >>>>>>> } > > >>>>>>> > > >>>>>>> static inline void spi_set_csgpiod(struct spi_device *spi, > > >>>>>>> u8 idx, struct > > >>>>>> gpio_desc *csgpiod) > > >>>>>>> { > > >>>>>>> - spi->cs_gpiod = csgpiod; > > >>>>>>> + spi->cs_gpiod[idx] = csgpiod; > > >>>>>>> +} > > >>>>>>> + > > >>>>>>> +static inline bool spi_is_csgpiod(struct spi_device *spi) { > > >>>>>>> + u8 idx; > > >>>>>>> + > > >>>>>>> + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { > > >>>>>>> + if (spi_get_csgpiod(spi, idx)) > > >>>>>>> + return true; > > >>>>>>> + } > > >>>>>>> + return false; > > >>>>>>> } > > >>>>>>> > > >>>>>>> /** > > >>>>>>> @@ -399,6 +422,8 @@ extern struct spi_device > > >>>>>> *spi_new_ancillary_device(struct spi_device *spi, u8 ch > > >>>>>>> * @bus_lock_spinlock: spinlock for SPI bus locking > > >>>>>>> * @bus_lock_mutex: mutex for exclusion of multiple callers > > >>>>>>> * @bus_lock_flag: indicates that the SPI bus is locked for > > >>>>>>> exclusive use > > >>>>>>> + * @multi_cs_cap: indicates that the SPI Controller can > > >>>>>>> + assert/de- > > >> assert > > >>>>>>> + * more than one chip select at once. > > >>>>>>> * @setup: updates the device mode and clocking records > used by > > a > > >>>>>>> * device's SPI controller; protocol code may call this. This > > >>>>>>> * must fail if an unrecognized or unsupported mode is > > >> requested. > > >>>>>>> @@ -567,6 +592,11 @@ struct spi_controller { > > >>>>>>> #define SPI_CONTROLLER_MUST_TX BIT(4) > /* > > Requires tx > > >>>> */ > > >>>>>>> #define SPI_CONTROLLER_GPIO_SS BIT(5) > /* GPIO CS > > >>>> must > > >>>>>> select slave */ > > >>>>>>> #define SPI_CONTROLLER_SUSPENDED BIT(6) /* > Currently > > >>>>>> suspended */ > > >>>>>>> + /* > > >>>>>>> + * The spi-controller has multi chip select capability and > can > > >>>>>>> + * assert/de-assert more than one chip select at once. > > >>>>>>> + */ > > >>>>>>> +#define SPI_CONTROLLER_MULTI_CS BIT(7) > > >>>>>>> > > >>>>>>> /* Flag indicating if the allocation of this struct is > > >>>>>>> devres- > > >> managed */ > > >>>>>>> bool devm_allocated; > > >>>>>>> @@ -677,7 +707,8 @@ struct spi_controller { > > >>>>>>> bool rt; > > >>>>>>> bool auto_runtime_pm; > > >>>>>>> bool cur_msg_mapped; > > >>>>>>> - char last_cs; > > >>>>>>> + char > last_cs[SPI_CS_CNT_MAX]; > > >>>>>>> + char last_cs_index_mask; > > >>>>>>> bool last_cs_mode_high; > > >>>>>>> bool fallback; > > >>>>>>> struct completion xfer_completion;
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 8ead7acb99f3..ff66147ba95f 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -612,10 +612,21 @@ static int spi_dev_check(struct device *dev, void *data) { struct spi_device *spi = to_spi_device(dev); struct spi_device *new_spi = data; - - if (spi->controller == new_spi->controller && - spi_get_chipselect(spi, 0) == spi_get_chipselect(new_spi, 0)) - return -EBUSY; + int idx, nw_idx; + u8 cs, cs_nw; + + if (spi->controller == new_spi->controller) { + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { + cs = spi_get_chipselect(spi, idx); + for (nw_idx = 0; nw_idx < SPI_CS_CNT_MAX; nw_idx++) { + cs_nw = spi_get_chipselect(spi, nw_idx); + if (cs != 0xFF && cs_nw != 0xFF && cs == cs_nw) { + dev_err(dev, "chipselect %d already in use\n", cs_nw); + return -EBUSY; + } + } + } + } return 0; } @@ -629,13 +640,32 @@ static int __spi_add_device(struct spi_device *spi) { struct spi_controller *ctlr = spi->controller; struct device *dev = ctlr->dev.parent; - int status; + int status, idx, nw_idx; + u8 cs, nw_cs; + + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { + /* Chipselects are numbered 0..max; validate. */ + cs = spi_get_chipselect(spi, idx); + if (cs != 0xFF && cs >= ctlr->num_chipselect) { + dev_err(dev, "cs%d >= max %d\n", spi_get_chipselect(spi, idx), + ctlr->num_chipselect); + return -EINVAL; + } + } - /* Chipselects are numbered 0..max; validate. */ - if (spi_get_chipselect(spi, 0) >= ctlr->num_chipselect) { - dev_err(dev, "cs%d >= max %d\n", spi_get_chipselect(spi, 0), - ctlr->num_chipselect); - return -EINVAL; + /* + * Make sure that multiple logical CS doesn't map to the same physical CS. + * For example, spi->chip_select[0] != spi->chip_select[1] and so on. + */ + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { + cs = spi_get_chipselect(spi, idx); + for (nw_idx = idx + 1; nw_idx < SPI_CS_CNT_MAX; nw_idx++) { + nw_cs = spi_get_chipselect(spi, nw_idx); + if (cs != 0xFF && nw_cs != 0xFF && cs == nw_cs) { + dev_err(dev, "chipselect %d already in use\n", nw_cs); + return -EBUSY; + } + } } /* Set the bus ID string */ @@ -647,11 +677,8 @@ static int __spi_add_device(struct spi_device *spi) * its configuration. */ status = bus_for_each_dev(&spi_bus_type, NULL, spi, spi_dev_check); - if (status) { - dev_err(dev, "chipselect %d already in use\n", - spi_get_chipselect(spi, 0)); + if (status) return status; - } /* Controller may unregister concurrently */ if (IS_ENABLED(CONFIG_SPI_DYNAMIC) && @@ -659,8 +686,15 @@ static int __spi_add_device(struct spi_device *spi) return -ENODEV; } - if (ctlr->cs_gpiods) - spi_set_csgpiod(spi, 0, ctlr->cs_gpiods[spi_get_chipselect(spi, 0)]); + if (ctlr->cs_gpiods) { + u8 cs; + + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { + cs = spi_get_chipselect(spi, idx); + if (cs != 0xFF) + spi_set_csgpiod(spi, idx, ctlr->cs_gpiods[cs]); + } + } /* * Drivers may modify this initial i/o setup, but will @@ -701,6 +735,9 @@ int spi_add_device(struct spi_device *spi) struct spi_controller *ctlr = spi->controller; int status; + /* Set the bus ID string */ + spi_dev_set_name(spi); + mutex_lock(&ctlr->add_lock); status = __spi_add_device(spi); mutex_unlock(&ctlr->add_lock); @@ -942,32 +979,51 @@ static void spi_res_release(struct spi_controller *ctlr, struct spi_message *mes } /*-------------------------------------------------------------------------*/ +static inline bool spi_is_last_cs(struct spi_device *spi) +{ + u8 idx; + bool last = false; + + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { + if ((spi->cs_index_mask >> idx) & 0x01) { + if (spi->controller->last_cs[idx] == spi_get_chipselect(spi, idx)) + last = true; + } + } + return last; +} + static void spi_set_cs(struct spi_device *spi, bool enable, bool force) { bool activate = enable; + u8 idx; /* * Avoid calling into the driver (or doing delays) if the chip select * isn't actually changing from the last time this was called. */ - if (!force && ((enable && spi->controller->last_cs == spi_get_chipselect(spi, 0)) || - (!enable && spi->controller->last_cs != spi_get_chipselect(spi, 0))) && + if (!force && ((enable && spi->controller->last_cs_index_mask == spi->cs_index_mask && + spi_is_last_cs(spi)) || + (!enable && spi->controller->last_cs_index_mask == spi->cs_index_mask && + !spi_is_last_cs(spi))) && (spi->controller->last_cs_mode_high == (spi->mode & SPI_CS_HIGH))) return; trace_spi_set_cs(spi, activate); - spi->controller->last_cs = enable ? spi_get_chipselect(spi, 0) : -1; + spi->controller->last_cs_index_mask = spi->cs_index_mask; + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) + spi->controller->last_cs[idx] = enable ? spi_get_chipselect(spi, 0) : -1; spi->controller->last_cs_mode_high = spi->mode & SPI_CS_HIGH; - if ((spi_get_csgpiod(spi, 0) || !spi->controller->set_cs_timing) && !activate) - spi_delay_exec(&spi->cs_hold, NULL); - if (spi->mode & SPI_CS_HIGH) enable = !enable; - if (spi_get_csgpiod(spi, 0)) { + if (spi_is_csgpiod(spi)) { + if (!spi->controller->set_cs_timing && !activate) + spi_delay_exec(&spi->cs_hold, NULL); + if (!(spi->mode & SPI_NO_CS)) { /* * Historically ACPI has no means of the GPIO polarity and @@ -979,26 +1035,38 @@ static void spi_set_cs(struct spi_device *spi, bool enable, bool force) * ambiguity. That's why we use enable, that takes SPI_CS_HIGH * into account. */ - if (has_acpi_companion(&spi->dev)) - gpiod_set_value_cansleep(spi_get_csgpiod(spi, 0), !enable); - else - /* Polarity handled by GPIO library */ - gpiod_set_value_cansleep(spi_get_csgpiod(spi, 0), activate); + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { + if (((spi->cs_index_mask >> idx) & 0x01) && + spi_get_csgpiod(spi, idx)) { + if (has_acpi_companion(&spi->dev)) + gpiod_set_value_cansleep(spi_get_csgpiod(spi, idx), + !enable); + else + /* Polarity handled by GPIO library */ + gpiod_set_value_cansleep(spi_get_csgpiod(spi, idx), + activate); + + if (activate) + spi_delay_exec(&spi->cs_setup, NULL); + else + spi_delay_exec(&spi->cs_inactive, NULL); + } + } } /* Some SPI masters need both GPIO CS & slave_select */ if ((spi->controller->flags & SPI_CONTROLLER_GPIO_SS) && spi->controller->set_cs) spi->controller->set_cs(spi, !enable); + + if (!spi->controller->set_cs_timing) { + if (activate) + spi_delay_exec(&spi->cs_setup, NULL); + else + spi_delay_exec(&spi->cs_inactive, NULL); + } } else if (spi->controller->set_cs) { spi->controller->set_cs(spi, !enable); } - - if (spi_get_csgpiod(spi, 0) || !spi->controller->set_cs_timing) { - if (activate) - spi_delay_exec(&spi->cs_setup, NULL); - else - spi_delay_exec(&spi->cs_inactive, NULL); - } } #ifdef CONFIG_HAS_DMA @@ -2222,8 +2290,8 @@ static void of_spi_parse_dt_cs_delay(struct device_node *nc, static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi, struct device_node *nc) { - u32 value; - int rc; + u32 value, cs[SPI_CS_CNT_MAX]; + int rc, idx; /* Mode (clock phase/polarity/etc.) */ if (of_property_read_bool(nc, "spi-cpha")) @@ -2295,14 +2363,52 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi, return 0; } + if (ctlr->num_chipselect > SPI_CS_CNT_MAX) { + dev_err(&ctlr->dev, "No. of CS is more than max. no. of supported CS\n"); + return -EINVAL; + } + + /* + * Zero(0) is a valid physical CS value and can be located at any + * logical CS in the spi->chip_select[]. If all the physical CS + * are initialized to 0 then It would be difficult to differentiate + * between a valid physical CS 0 & an unused logical CS whose physical + * CS can be 0. As a solution to this issue initialize all the CS to 0xFF. + * Now all the unused logical CS will have 0xFF physical CS value & can be + * ignore while performing physical CS validity checks. + */ + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) + spi_set_chipselect(spi, idx, 0xFF); + /* Device address */ - rc = of_property_read_u32(nc, "reg", &value); - if (rc) { + rc = of_property_read_variable_u32_array(nc, "reg", &cs[0], 1, + SPI_CS_CNT_MAX); + if (rc < 0) { dev_err(&ctlr->dev, "%pOF has no valid 'reg' property (%d)\n", nc, rc); return rc; } - spi_set_chipselect(spi, 0, value); + if (rc > ctlr->num_chipselect) { + dev_err(&ctlr->dev, "%pOF has number of CS > ctlr->num_chipselect (%d)\n", + nc, rc); + return rc; + } + if ((of_property_read_bool(nc, "parallel-memories")) && + (!(ctlr->flags & SPI_CONTROLLER_MULTI_CS))) { + dev_err(&ctlr->dev, "SPI controller doesn't support multi CS\n"); + return -EINVAL; + } + for (idx = 0; idx < rc; idx++) + spi_set_chipselect(spi, idx, cs[idx]); + + /* spi->chip_select[i] gives the corresponding physical CS for logical CS i + * logical CS number is represented by setting the ith bit in spi->cs_index_mask + * So, for example, if spi->cs_index_mask = 0x01 then logical CS number is 0 and + * spi->chip_select[0] will give the physical CS. + * By default spi->chip_select[0] will hold the physical CS number so, set + * spi->cs_index_mask as 0x01. + */ + spi->cs_index_mask = 0x01; /* Device speed */ if (!of_property_read_u32(nc, "spi-max-frequency", &value)) @@ -3100,6 +3206,7 @@ int spi_register_controller(struct spi_controller *ctlr) struct boardinfo *bi; int first_dynamic; int status; + int idx; if (!dev) return -ENODEV; @@ -3164,7 +3271,8 @@ int spi_register_controller(struct spi_controller *ctlr) } /* Setting last_cs to -1 means no chip selected */ - ctlr->last_cs = -1; + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) + ctlr->last_cs[idx] = -1; status = device_add(&ctlr->dev); if (status < 0) @@ -3889,7 +3997,7 @@ static int __spi_validate(struct spi_device *spi, struct spi_message *message) * cs_change is set for each transfer. */ if ((spi->mode & SPI_CS_WORD) && (!(ctlr->mode_bits & SPI_CS_WORD) || - spi_get_csgpiod(spi, 0))) { + spi_is_csgpiod(spi))) { size_t maxsize = BITS_TO_BYTES(spi->bits_per_word); int ret; diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index 7b4baff63c5c..871d3a6d879b 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -20,6 +20,9 @@ #include <uapi/linux/spi/spi.h> +/* Max no. of CS supported per spi device */ +#define SPI_CS_CNT_MAX 4 + struct dma_chan; struct software_node; struct ptp_system_timestamp; @@ -132,7 +135,8 @@ extern void spi_transfer_cs_change_delay_exec(struct spi_message *msg, * @max_speed_hz: Maximum clock rate to be used with this chip * (on this board); may be changed by the device's driver. * The spi_transfer.speed_hz can override this for each transfer. - * @chip_select: Chipselect, distinguishing chips handled by @controller. + * @chip_select: Array of physical chipselect, spi->chipselect[i] gives + * the corresponding physical CS for logical CS i. * @mode: The spi mode defines how data is clocked out and in. * This may be changed by the device's driver. * The "active low" default for chipselect mode can be overridden @@ -157,8 +161,8 @@ extern void spi_transfer_cs_change_delay_exec(struct spi_message *msg, * the device will bind to the named driver and only the named driver. * Do not set directly, because core frees it; use driver_set_override() to * set or clear it. - * @cs_gpiod: GPIO descriptor of the chipselect line (optional, NULL when - * not using a GPIO line) + * @cs_gpiod: Array of GPIO descriptors of the corresponding chipselect lines + * (optional, NULL when not using a GPIO line) * @word_delay: delay to be inserted between consecutive * words of a transfer * @cs_setup: delay to be introduced by the controller after CS is asserted @@ -167,6 +171,7 @@ extern void spi_transfer_cs_change_delay_exec(struct spi_message *msg, * deasserted. If @cs_change_delay is used from @spi_transfer, then the * two delays will be added up. * @pcpu_statistics: statistics for the spi_device + * @cs_index_mask: Bit mask of the active chipselect(s) in the chipselect array * * A @spi_device is used to interchange data between an SPI slave * (usually a discrete chip) and CPU memory. @@ -182,7 +187,7 @@ struct spi_device { struct spi_controller *controller; struct spi_controller *master; /* Compatibility layer */ u32 max_speed_hz; - u8 chip_select; + u8 chip_select[SPI_CS_CNT_MAX]; u8 bits_per_word; bool rt; #define SPI_NO_TX BIT(31) /* No transmit wire */ @@ -213,7 +218,7 @@ struct spi_device { void *controller_data; char modalias[SPI_NAME_SIZE]; const char *driver_override; - struct gpio_desc *cs_gpiod; /* Chip select GPIO descriptor */ + struct gpio_desc *cs_gpiod[SPI_CS_CNT_MAX]; /* Chip select gpio desc */ struct spi_delay word_delay; /* Inter-word delay */ /* CS delays */ struct spi_delay cs_setup; @@ -223,6 +228,13 @@ struct spi_device { /* The statistics */ struct spi_statistics __percpu *pcpu_statistics; + /* Bit mask of the chipselect(s) that the driver need to use from + * the chipselect array.When the controller is capable to handle + * multiple chip selects & memories are connected in parallel + * then more than one bit need to be set in cs_index_mask. + */ + u32 cs_index_mask : SPI_CS_CNT_MAX; + /* * Likely need more hooks for more protocol options affecting how * the controller talks to each chip, like: @@ -279,22 +291,33 @@ static inline void *spi_get_drvdata(const struct spi_device *spi) static inline u8 spi_get_chipselect(const struct spi_device *spi, u8 idx) { - return spi->chip_select; + return spi->chip_select[idx]; } static inline void spi_set_chipselect(struct spi_device *spi, u8 idx, u8 chipselect) { - spi->chip_select = chipselect; + spi->chip_select[idx] = chipselect; } static inline struct gpio_desc *spi_get_csgpiod(const struct spi_device *spi, u8 idx) { - return spi->cs_gpiod; + return spi->cs_gpiod[idx]; } static inline void spi_set_csgpiod(struct spi_device *spi, u8 idx, struct gpio_desc *csgpiod) { - spi->cs_gpiod = csgpiod; + spi->cs_gpiod[idx] = csgpiod; +} + +static inline bool spi_is_csgpiod(struct spi_device *spi) +{ + u8 idx; + + for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) { + if (spi_get_csgpiod(spi, idx)) + return true; + } + return false; } /** @@ -399,6 +422,8 @@ extern struct spi_device *spi_new_ancillary_device(struct spi_device *spi, u8 ch * @bus_lock_spinlock: spinlock for SPI bus locking * @bus_lock_mutex: mutex for exclusion of multiple callers * @bus_lock_flag: indicates that the SPI bus is locked for exclusive use + * @multi_cs_cap: indicates that the SPI Controller can assert/de-assert + * more than one chip select at once. * @setup: updates the device mode and clocking records used by a * device's SPI controller; protocol code may call this. This * must fail if an unrecognized or unsupported mode is requested. @@ -567,6 +592,11 @@ struct spi_controller { #define SPI_CONTROLLER_MUST_TX BIT(4) /* Requires tx */ #define SPI_CONTROLLER_GPIO_SS BIT(5) /* GPIO CS must select slave */ #define SPI_CONTROLLER_SUSPENDED BIT(6) /* Currently suspended */ + /* + * The spi-controller has multi chip select capability and can + * assert/de-assert more than one chip select at once. + */ +#define SPI_CONTROLLER_MULTI_CS BIT(7) /* Flag indicating if the allocation of this struct is devres-managed */ bool devm_allocated; @@ -677,7 +707,8 @@ struct spi_controller { bool rt; bool auto_runtime_pm; bool cur_msg_mapped; - char last_cs; + char last_cs[SPI_CS_CNT_MAX]; + char last_cs_index_mask; bool last_cs_mode_high; bool fallback; struct completion xfer_completion;
AMD-Xilinx GQSPI controller has two advanced mode that allows the controller to consider two flashes as one single device. One of these two mode is the parallel mode in which each byte of data is stored in both devices, the even bits in the lower flash & the odd bits in the upper flash. The byte split is automatically handled by the QSPI controller. The other mode is the stacked mode in which both the flashes share the same SPI bus but each of the device contain half of the data. In this mode, the controller does not follow CS requests but instead internally wires the two CS levels with the value of the most significant address bit. For supporting both these modes SPI core need to be updated for providing multiple CS for a single SPI device. For adding multi CS support the SPI device need to be aware of all the CS values. So, the "chip_select" member in the spi_device structure is now an array that holds all the CS values. spi_device structure now has a "cs_index_mask" member. This acts as an index to the chip_select array. If nth bit of spi->cs_index_mask is set then the driver would assert spi->chip_select[n]. In parallel mode all the chip selects are asserted/de-asserted simultaneously and each byte of data is stored in both devices, the even bits in one, the odd bits in the other. The split is automatically handled by the GQSPI controller. The GQSPI controller supports a maximum of two flashes connected in parallel mode. A SPI_CONTROLLER_MULTI_CS flag bit is added in the spi controller flags, through ctlr->flags the spi core will make sure that the controller is capable of handling multiple chip selects at once. For supporting multiple CS via GPIO the cs_gpiod member of the spi_device structure is now an array that holds the gpio descriptor for each chipselect. CS GPIO is not tested due to unavailability of necessary hardware setup. Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com> --- Hello @Stefen, We restructured the SPI core implementation, for handling arbitrary number of devices connected in parallel(multi-cs) or stacked mode. We have tested this updated patch on native-cs setup but couldn't test cs-gpio due to unavailability of a proper setup. If possible, could you please retest the cs-gpio use case with this updated patch and provide your feedback. --- drivers/spi/spi.c | 192 +++++++++++++++++++++++++++++++--------- include/linux/spi/spi.h | 51 ++++++++--- 2 files changed, 191 insertions(+), 52 deletions(-)