| Message ID | 20231125092137.2948-4-amit.kumar-mahapatra@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | spi: Add support for stacked/parallel memories | expand |
Hi, On Sat, Nov 25, 2023 at 02:51:30PM +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. > > 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 on our hardware, but it has been tested by @Stefan > https://lore.kernel.org/all/005001da1efc$619ad5a0$24d080e0$@opensource.cirrus.com/ > With this patch in the mainline kernel, two of my qemu emulations (quanta-q71l-bmc and almetto-bmc) fail to instantiate the first SPI controller and thus fail to boot from SPI. The error message is [ 3.006458] spi_master spi0: No. of CS is more than max. no. of supported CS [ 3.006775] spi_master spi0: Failed to create SPI device for /ahb/spi@1e620000/flash@0 The problem is no longer seen after reverting this patch. Bisect log attached for reference. Guenter --- # bad: [70d201a40823acba23899342d62bc2644051ad2e] Merge tag 'f2fs-for-6.8-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs # good: [0dd3ee31125508cd67f7e7172247f05b7fd1753a] Linux 6.7 git bisect start 'HEAD' 'v6.7' # bad: [de927f6c0b07d9e698416c5b287c521b07694cac] Merge tag 's390-6.8-1' of git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux git bisect bad de927f6c0b07d9e698416c5b287c521b07694cac # bad: [35f11a3710cdcbbc5090d14017a6295454e0cc73] Merge tag 'mtd/for-6.8' of git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux git bisect bad 35f11a3710cdcbbc5090d14017a6295454e0cc73 # good: [d30e51aa7b1f6fa7dd78d4598d1e4c047fcc3fb9] Merge tag 'slab-for-6.8' of git://git.kernel.org/pub/scm/linux/kernel/git/vbabka/slab git bisect good d30e51aa7b1f6fa7dd78d4598d1e4c047fcc3fb9 # good: [fb46e22a9e3863e08aef8815df9f17d0f4b9aede] Merge tag 'mm-stable-2024-01-08-15-31' of git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm git bisect good fb46e22a9e3863e08aef8815df9f17d0f4b9aede # good: [063a7ce32ddc2c4f2404b0dfd29e60e3dbcdffac] Merge tag 'lsm-pr-20240105' of git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/lsm git bisect good 063a7ce32ddc2c4f2404b0dfd29e60e3dbcdffac # bad: [f6cd66231aa58599526584ff4df1bdde8d86eac8] spi: stm32: add st,stm32mp25-spi compatible supporting STM32MP25 soc git bisect bad f6cd66231aa58599526584ff4df1bdde8d86eac8 # good: [18f78b5e609b19b56237f0dae47068d44b8b0ecd] spi: axi-spi-engine: improvements round 2 git bisect good 18f78b5e609b19b56237f0dae47068d44b8b0ecd # bad: [9d93c8d97b4cdb5edddb4c5530881c90eecb7e44] spi: spi-ti-qspi: switch to use modern name git bisect bad 9d93c8d97b4cdb5edddb4c5530881c90eecb7e44 # bad: [e6b7e64cb11966b26646a362677ca5a08481157e] spi: st-ssc4: switch to use modern name git bisect bad e6b7e64cb11966b26646a362677ca5a08481157e # bad: [c3aeaf2f0ec8af93189488bda3928a1ac7752388] spi: pxa2xx: Use inclusive language git bisect bad c3aeaf2f0ec8af93189488bda3928a1ac7752388 # good: [f05e2f61fe88092e0d341ea27644a84e3386358d] ALSA: hda/cs35l56: Use set/get APIs to access spi->chip_select git bisect good f05e2f61fe88092e0d341ea27644a84e3386358d # bad: [88a50c1663ffa9f6b31705c6bf7a887a2c8d9434] spi: Add support for stacked/parallel memories git bisect bad 88a50c1663ffa9f6b31705c6bf7a887a2c8d9434 # bad: [4d8ff6b0991d5e86b17b235fc46ec62e9195cb9b] spi: Add multi-cs memories support in SPI core git bisect bad 4d8ff6b0991d5e86b17b235fc46ec62e9195cb9b # first bad commit: [4d8ff6b0991d5e86b17b235fc46ec62e9195cb9b] spi: Add multi-cs memories support in SPI core
On Fri, Jan 12, 2024 at 11:11:07AM -0800, Guenter Roeck wrote: > On Sat, Nov 25, 2023 at 02:51:30PM +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. ... > With this patch in the mainline kernel, two of my qemu emulations > (quanta-q71l-bmc and almetto-bmc) fail to instantiate the first SPI > controller and thus fail to boot from SPI. The error message is Not sure what quanta-q711-bmc is - is almetto-bmc really palmetto-bmc (which has a mainline DT with a SPI flash)? > > [ 3.006458] spi_master spi0: No. of CS is more than max. no. of supported CS > [ 3.006775] spi_master spi0: Failed to create SPI device for /ahb/spi@1e620000/flash@0 > > The problem is no longer seen after reverting this patch. > > Bisect log attached for reference. > > Guenter > > --- > # bad: [70d201a40823acba23899342d62bc2644051ad2e] Merge tag 'f2fs-for-6.8-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs > # good: [0dd3ee31125508cd67f7e7172247f05b7fd1753a] Linux 6.7 > git bisect start 'HEAD' 'v6.7' > # bad: [de927f6c0b07d9e698416c5b287c521b07694cac] Merge tag 's390-6.8-1' of git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux > git bisect bad de927f6c0b07d9e698416c5b287c521b07694cac > # bad: [35f11a3710cdcbbc5090d14017a6295454e0cc73] Merge tag 'mtd/for-6.8' of git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux > git bisect bad 35f11a3710cdcbbc5090d14017a6295454e0cc73 > # good: [d30e51aa7b1f6fa7dd78d4598d1e4c047fcc3fb9] Merge tag 'slab-for-6.8' of git://git.kernel.org/pub/scm/linux/kernel/git/vbabka/slab > git bisect good d30e51aa7b1f6fa7dd78d4598d1e4c047fcc3fb9 > # good: [fb46e22a9e3863e08aef8815df9f17d0f4b9aede] Merge tag 'mm-stable-2024-01-08-15-31' of git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm > git bisect good fb46e22a9e3863e08aef8815df9f17d0f4b9aede > # good: [063a7ce32ddc2c4f2404b0dfd29e60e3dbcdffac] Merge tag 'lsm-pr-20240105' of git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/lsm > git bisect good 063a7ce32ddc2c4f2404b0dfd29e60e3dbcdffac > # bad: [f6cd66231aa58599526584ff4df1bdde8d86eac8] spi: stm32: add st,stm32mp25-spi compatible supporting STM32MP25 soc > git bisect bad f6cd66231aa58599526584ff4df1bdde8d86eac8 > # good: [18f78b5e609b19b56237f0dae47068d44b8b0ecd] spi: axi-spi-engine: improvements round 2 > git bisect good 18f78b5e609b19b56237f0dae47068d44b8b0ecd > # bad: [9d93c8d97b4cdb5edddb4c5530881c90eecb7e44] spi: spi-ti-qspi: switch to use modern name > git bisect bad 9d93c8d97b4cdb5edddb4c5530881c90eecb7e44 > # bad: [e6b7e64cb11966b26646a362677ca5a08481157e] spi: st-ssc4: switch to use modern name > git bisect bad e6b7e64cb11966b26646a362677ca5a08481157e > # bad: [c3aeaf2f0ec8af93189488bda3928a1ac7752388] spi: pxa2xx: Use inclusive language > git bisect bad c3aeaf2f0ec8af93189488bda3928a1ac7752388 > # good: [f05e2f61fe88092e0d341ea27644a84e3386358d] ALSA: hda/cs35l56: Use set/get APIs to access spi->chip_select > git bisect good f05e2f61fe88092e0d341ea27644a84e3386358d > # bad: [88a50c1663ffa9f6b31705c6bf7a887a2c8d9434] spi: Add support for stacked/parallel memories > git bisect bad 88a50c1663ffa9f6b31705c6bf7a887a2c8d9434 > # bad: [4d8ff6b0991d5e86b17b235fc46ec62e9195cb9b] spi: Add multi-cs memories support in SPI core > git bisect bad 4d8ff6b0991d5e86b17b235fc46ec62e9195cb9b > # first bad commit: [4d8ff6b0991d5e86b17b235fc46ec62e9195cb9b] spi: Add multi-cs memories support in SPI core
On 1/12/24 11:16, Mark Brown wrote: > On Fri, Jan 12, 2024 at 11:11:07AM -0800, Guenter Roeck wrote: >> On Sat, Nov 25, 2023 at 02:51:30PM +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. > > ... > >> With this patch in the mainline kernel, two of my qemu emulations >> (quanta-q71l-bmc and almetto-bmc) fail to instantiate the first SPI >> controller and thus fail to boot from SPI. The error message is > > Not sure what quanta-q711-bmc is - is almetto-bmc really palmetto-bmc > (which has a mainline DT with a SPI flash)? > Yes, sorry, it should have been palmetto-bmc. quanta-q71l-bmc: See arch/arm/boot/dts/aspeed/aspeed-bmc-quanta-q71l.dts (it is q71l, not q711) Guenter
On Fri, Jan 12, 2024 at 11:11:10AM -0800, Guenter Roeck wrote: > Hi, > > On Sat, Nov 25, 2023 at 02:51:30PM +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. > > > > 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 on our hardware, but it has been tested by @Stefan > > https://lore.kernel.org/all/005001da1efc$619ad5a0$24d080e0$@opensource.cirrus.com/ > > > > With this patch in the mainline kernel, two of my qemu emulations > (quanta-q71l-bmc and almetto-bmc) fail to instantiate the first SPI > controller and thus fail to boot from SPI. The error message is > > [ 3.006458] spi_master spi0: No. of CS is more than max. no. of supported CS > [ 3.006775] spi_master spi0: Failed to create SPI device for /ahb/spi@1e620000/flash@0 > > The problem is no longer seen after reverting this patch. > FWIW, the problem is due to +#define SPI_CS_CNT_MAX 4 in the offending patch, but apeed2400 FMC supports up to 5 SPI chip selects. static const struct aspeed_spi_data ast2400_fmc_data = { .max_cs = 5, ^^^^^^^^^^^^^^^^^^^ .hastype = true, Limiting .max_cs to 4 or increasing SPI_CS_CNT_MAX to 5 fixes the problem, though of course I don't know if increasing SPI_CS_CNT_MAX has other side effects. Guenter > Bisect log attached for reference. > > Guenter > > --- > # bad: [70d201a40823acba23899342d62bc2644051ad2e] Merge tag 'f2fs-for-6.8-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs > # good: [0dd3ee31125508cd67f7e7172247f05b7fd1753a] Linux 6.7 > git bisect start 'HEAD' 'v6.7' > # bad: [de927f6c0b07d9e698416c5b287c521b07694cac] Merge tag 's390-6.8-1' of git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux > git bisect bad de927f6c0b07d9e698416c5b287c521b07694cac > # bad: [35f11a3710cdcbbc5090d14017a6295454e0cc73] Merge tag 'mtd/for-6.8' of git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux > git bisect bad 35f11a3710cdcbbc5090d14017a6295454e0cc73 > # good: [d30e51aa7b1f6fa7dd78d4598d1e4c047fcc3fb9] Merge tag 'slab-for-6.8' of git://git.kernel.org/pub/scm/linux/kernel/git/vbabka/slab > git bisect good d30e51aa7b1f6fa7dd78d4598d1e4c047fcc3fb9 > # good: [fb46e22a9e3863e08aef8815df9f17d0f4b9aede] Merge tag 'mm-stable-2024-01-08-15-31' of git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm > git bisect good fb46e22a9e3863e08aef8815df9f17d0f4b9aede > # good: [063a7ce32ddc2c4f2404b0dfd29e60e3dbcdffac] Merge tag 'lsm-pr-20240105' of git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/lsm > git bisect good 063a7ce32ddc2c4f2404b0dfd29e60e3dbcdffac > # bad: [f6cd66231aa58599526584ff4df1bdde8d86eac8] spi: stm32: add st,stm32mp25-spi compatible supporting STM32MP25 soc > git bisect bad f6cd66231aa58599526584ff4df1bdde8d86eac8 > # good: [18f78b5e609b19b56237f0dae47068d44b8b0ecd] spi: axi-spi-engine: improvements round 2 > git bisect good 18f78b5e609b19b56237f0dae47068d44b8b0ecd > # bad: [9d93c8d97b4cdb5edddb4c5530881c90eecb7e44] spi: spi-ti-qspi: switch to use modern name > git bisect bad 9d93c8d97b4cdb5edddb4c5530881c90eecb7e44 > # bad: [e6b7e64cb11966b26646a362677ca5a08481157e] spi: st-ssc4: switch to use modern name > git bisect bad e6b7e64cb11966b26646a362677ca5a08481157e > # bad: [c3aeaf2f0ec8af93189488bda3928a1ac7752388] spi: pxa2xx: Use inclusive language > git bisect bad c3aeaf2f0ec8af93189488bda3928a1ac7752388 > # good: [f05e2f61fe88092e0d341ea27644a84e3386358d] ALSA: hda/cs35l56: Use set/get APIs to access spi->chip_select > git bisect good f05e2f61fe88092e0d341ea27644a84e3386358d > # bad: [88a50c1663ffa9f6b31705c6bf7a887a2c8d9434] spi: Add support for stacked/parallel memories > git bisect bad 88a50c1663ffa9f6b31705c6bf7a887a2c8d9434 > # bad: [4d8ff6b0991d5e86b17b235fc46ec62e9195cb9b] spi: Add multi-cs memories support in SPI core > git bisect bad 4d8ff6b0991d5e86b17b235fc46ec62e9195cb9b > # first bad commit: [4d8ff6b0991d5e86b17b235fc46ec62e9195cb9b] spi: Add multi-cs memories support in SPI core
On Sat, Jan 20, 2024 at 09:05:43AM -0800, Guenter Roeck wrote: > FWIW, the problem is due to > +#define SPI_CS_CNT_MAX 4 > in the offending patch, but apeed2400 FMC supports up to 5 SPI chip selects. > > static const struct aspeed_spi_data ast2400_fmc_data = { > .max_cs = 5, > ^^^^^^^^^^^^^^^^^^^ > .hastype = true, > Limiting .max_cs to 4 or increasing SPI_CS_CNT_MAX to 5 fixes the problem, > though of course I don't know if increasing SPI_CS_CNT_MAX has other side > effects. Yeah, I was coming to a similar conclusion myself - the limit is just too low. I can't see any problem with increasing it.
On 12.01.24 20:11, Guenter Roeck wrote: > > On Sat, Nov 25, 2023 at 02:51:30PM +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. >> >> 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. > [...] > With this patch in the mainline kernel, two of my qemu emulations > (quanta-q71l-bmc and almetto-bmc) fail to instantiate the first SPI > controller and thus fail to boot from SPI. The error message is > > [ 3.006458] spi_master spi0: No. of CS is more than max. no. of supported CS > [ 3.006775] spi_master spi0: Failed to create SPI device for /ahb/spi@1e620000/flash@0 > > The problem is no longer seen after reverting this patch. > [...] > # first bad commit: [4d8ff6b0991d5e86b17b235fc46ec62e9195cb9b] spi: Add multi-cs memories support in SPI core Thanks for the report. To be sure the issue doesn't fall through the cracks unnoticed, I'm adding it to regzbot, the Linux kernel regression tracking bot: #regzbot ^introduced 4d8ff6b0991d5e86b17b235fc46ec62e9195cb9 #regzbot title spi: qemu emulations quanta-q71l-bmc and almetto-bmc fail to boot #regzbot ignore-activity This isn't a regression? This issue or a fix for it are already discussed somewhere else? It was fixed already? You want to clarify when the regression started to happen? Or point out I got the title or something else totally wrong? Then just reply and tell me -- ideally while also telling regzbot about it, as explained by the page listed in the footer of this mail. Developers: When fixing the issue, remember to add 'Link:' tags pointing to the report (the parent of this mail). See page linked in footer for details. Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) -- Everything you wanna know about Linux kernel regression tracking: https://linux-regtracking.leemhuis.info/about/#tldr That page also explains what to do if mails like this annoy you.
On 1/20/24 17:04, Mark Brown wrote: > On Sat, Jan 20, 2024 at 09:05:43AM -0800, Guenter Roeck wrote: > >> FWIW, the problem is due to > >> +#define SPI_CS_CNT_MAX 4 > >> in the offending patch, but apeed2400 FMC supports up to 5 SPI chip selects. >> >> static const struct aspeed_spi_data ast2400_fmc_data = { >> .max_cs = 5, >> ^^^^^^^^^^^^^^^^^^^ >> .hastype = true, > >> Limiting .max_cs to 4 or increasing SPI_CS_CNT_MAX to 5 fixes the problem, >> though of course I don't know if increasing SPI_CS_CNT_MAX has other side >> effects. > > Yeah, I was coming to a similar conclusion myself - the limit is just > too low. I can't see any problem with increasing it. It would cost a bit of memory and somewhat affect performance sine many of the newly introduced loops are bound by SPI_CS_CNT_MAX and not by num_chipselect. It also might make sense to document the new limit somewhere. Prior to this commit it was not limited at all. Documentation/devicetree/bindings/spi/spi-davinci.txt lists 5 chip selects in its example for the use of cs-gpios. Documentation/devicetree/bindings/spi/spi-controller.yaml also does not list a limit. Thanks, Guenter
>>> FWIW, the problem is due to >> >>> +#define SPI_CS_CNT_MAX 4 >> >>> in the offending patch, but apeed2400 FMC supports up to 5 SPI chip >>> selects. >>> >>> static const struct aspeed_spi_data ast2400_fmc_data = { >>> .max_cs = 5, >>> ^^^^^^^^^^^^^^^^^^^ >>> .hastype = true, >> >>> Limiting .max_cs to 4 or increasing SPI_CS_CNT_MAX to 5 fixes the >>> problem, >>> though of course I don't know if increasing SPI_CS_CNT_MAX has other >>> side >>> effects. >> >> Yeah, I was coming to a similar conclusion myself - the limit is just >> too low. I can't see any problem with increasing it. > > It would cost a bit of memory and somewhat affect performance sine many > of the newly introduced loops are bound by SPI_CS_CNT_MAX and not by > num_chipselect. > > It also might make sense to document the new limit somewhere. Prior > to this commit it was not limited at all. > Documentation/devicetree/bindings/spi/spi-davinci.txt lists 5 chip > selects in its example for the use of cs-gpios. > Documentation/devicetree/bindings/spi/spi-controller.yaml also does not > list a limit. Given that, that the rest of this series is under discussion (and esp. whether it is the correct way to do it) it might make sense to just revert the picked patches. -michael
On 1/21/24 10:06, Michael Walle wrote: >>>> FWIW, the problem is due to >>> >>>> +#define SPI_CS_CNT_MAX 4 >>> >>>> in the offending patch, but apeed2400 FMC supports up to 5 SPI chip selects. >>>> >>>> static const struct aspeed_spi_data ast2400_fmc_data = { >>>> .max_cs = 5, >>>> ^^^^^^^^^^^^^^^^^^^ >>>> .hastype = true, >>> >>>> Limiting .max_cs to 4 or increasing SPI_CS_CNT_MAX to 5 fixes the problem, >>>> though of course I don't know if increasing SPI_CS_CNT_MAX has other side >>>> effects. >>> >>> Yeah, I was coming to a similar conclusion myself - the limit is just >>> too low. I can't see any problem with increasing it. >> >> It would cost a bit of memory and somewhat affect performance sine many >> of the newly introduced loops are bound by SPI_CS_CNT_MAX and not by >> num_chipselect. >> >> It also might make sense to document the new limit somewhere. Prior >> to this commit it was not limited at all. >> Documentation/devicetree/bindings/spi/spi-davinci.txt lists 5 chip >> selects in its example for the use of cs-gpios. >> Documentation/devicetree/bindings/spi/spi-controller.yaml also does not >> list a limit. > > Given that, that the rest of this series is under discussion (and esp. whether > it is the correct way to do it) it might make sense to just revert the picked > patches. > I can't really comment on that, but I found that there is another affected devicetree property: num-cs. Its range isn't limited in Documentation/devicetree/bindings/spi/spi-controller.yaml. Various dts/dtsi files use numbers as large as 8. The range is limited in some bindings files, but not all of them. Documentation/devicetree/bindings/spi/spi-lantiq-ssc.txt, for example, says "num-cs: see spi-bus.txt, set to 8 if unset" Various Broadcom dtsi files set it to 8. So I guess 8 would be the absolute minimum to re-enable support for affected systems. Guenter
On Sun, Jan 21, 2024 at 07:06:31PM +0100, Michael Walle wrote: > Given that, that the rest of this series is under discussion (and esp. > whether > it is the correct way to do it) it might make sense to just revert the > picked > patches. The API change in the patch is a pain out of tree due to the way it's easy to add new things that miss the helpers without it so I'd rather keep it in if possible. The underlying implementation does need some TLC though.
On Sun, Jan 21, 2024 at 11:29:48AM -0800, Guenter Roeck wrote: > So I guess 8 would be the absolute minimum to re-enable support for > affected systems. That's actually the number I went for, just waiting for CI to go through before I send it out.
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 8ead7acb99f3..45b6898cf0ee 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(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; + } + } + } + } 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); @@ -727,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. @@ -742,6 +780,18 @@ struct spi_device *spi_new_device(struct spi_controller *ctlr, WARN_ON(strlen(chip->modalias) >= sizeof(proxy->modalias)); + /* + * 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(proxy, idx, 0xFF); + spi_set_chipselect(proxy, 0, chip->chip_select); proxy->max_speed_hz = chip->max_speed_hz; proxy->mode = chip->mode; @@ -750,6 +800,15 @@ 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; + /* + * 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. + */ + proxy->cs_index_mask = 0x01; if (chip->swnode) { status = device_add_software_node(&proxy->dev, chip->swnode); @@ -942,32 +1001,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 +1057,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 +2312,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 +2385,53 @@ 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)) @@ -2408,6 +2537,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); @@ -2418,12 +2548,33 @@ struct spi_device *spi_new_ancillary_device(struct spi_device *spi, strscpy(ancillary->modalias, "dummy", sizeof(ancillary->modalias)); + /* + * 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(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; + /* + * 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. + */ + ancillary->cs_index_mask = 0x01; WARN_ON(!mutex_is_locked(&ctlr->add_lock)); @@ -2626,6 +2777,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); @@ -2661,12 +2813,33 @@ struct spi_device *acpi_spi_device_alloc(struct spi_controller *ctlr, return ERR_PTR(-ENOMEM); } + /* + * 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); + 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->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; return spi; } @@ -3100,6 +3273,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 +3338,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 +4064,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;