| Message ID | 20230425134527.483607-1-linux@rasmusvillemoes.dk (mailing list archive) |
|---|---|
| Headers | show |
| Series | spi: spi-imx: fix use of more than four chip selects | expand |
On 25/04/2023 15.45, Rasmus Villemoes wrote: > The current spi-imx driver completely fails when used with more than > four (gpio) chip-selects, since the chip select number is used > unconditionally as shift amount when updating the control and > configuration registers, so the code ends up modifying random bits > outside the intended fields. > > This fixes it by making use of the unused_native_cs variable filled in > by the spi core, and use that as the "channel number" for all gpiod > chip selects. So I obviously hadn't seen https://lore.kernel.org/lkml/20230318222132.3373-1-kgroeneveld@lenbrook.com/T/#u when I sent this. I did consider that approach, but rejected it because it wouldn't work with mixing native and gpio chip selects. Say, somebody uses SS0 natively, but then also have four additional gpios. Then chipselect 4 would end up activating both the SS0 pin as well as the gpio, selecting both devices. I don't know if that's really a realistic scenario. But at least I think the driver should then somehow have a way to indicate to the core that one should either use native or gpio chip selects, but not a mix. Thoughts? Rasmus
On Wed, Apr 26, 2023 at 09:19:29AM +0200, Rasmus Villemoes wrote: > I did consider that approach, but rejected it because it wouldn't work > with mixing native and gpio chip selects. Say, somebody uses SS0 > natively, but then also have four additional gpios. Then chipselect 4 > would end up activating both the SS0 pin as well as the gpio, selecting > both devices. > I don't know if that's really a realistic scenario. But at least I think > the driver should then somehow have a way to indicate to the core that > one should either use native or gpio chip selects, but not a mix. I'm not sure this is sensible, it'll be a fairly rare situation and we don't want to preclude using the built in chip select functionality for some of the chip selects. In a situation like this we only need to have a single chip select to be managed as a GPIO rather than all of them, which I'd expect to end up handled in the DT by not allocating that chip select number.
On 26/04/2023 14.25, Mark Brown wrote: > On Wed, Apr 26, 2023 at 09:19:29AM +0200, Rasmus Villemoes wrote: > >> I did consider that approach, but rejected it because it wouldn't work >> with mixing native and gpio chip selects. Say, somebody uses SS0 >> natively, but then also have four additional gpios. Then chipselect 4 >> would end up activating both the SS0 pin as well as the gpio, selecting >> both devices. > >> I don't know if that's really a realistic scenario. But at least I think >> the driver should then somehow have a way to indicate to the core that >> one should either use native or gpio chip selects, but not a mix. > > I'm not sure this is sensible, it'll be a fairly rare situation and we > don't want to preclude using the built in chip select functionality for > some of the chip selects. In a situation like this we only need to have > a single chip select to be managed as a GPIO rather than all of them, > which I'd expect to end up handled in the DT by not allocating that chip > select number. Sorry, I don't understand what you're saying. What exactly is not sensible? And what is "a situation like this"? I described a problem with what is now 87c614175bbf in linux-next: If one has five spi devices, the first four of which use the four native chip selects, there is no way to use a gpio for the fifth, because whichever "channel" you choose in the CHANNEL_SELECT field will cause the ecspi IP block to drive some SSx pin low, while the spi core is also driving the gpio low, so two different devices would be selected. It's not exactly a regression, because any chip_select >= 4 never actually worked, but what I'm saying is that 87c614175bbf also isn't a complete fix if one wants to support mixing native and gpio chip selects. For that, one really needs the unused_native_cs to be used for all gpio chip selects; in particular, one needs some unused native cs to exist. IOW, what my series tries to do. [OK, now that I re-read what I wrote, I didn't exactly describe "four native CS, one gpio", but "one native CS, four gpios". That scenario _could_ of course work with my series, but with 87c614175bbf just masking the chip-select number, we do get the problem that two devices would be selected at the same time. And I don't think expecting the DT author to know to use regs 1, 2, 3, 5 for those four gpio chip selects is reasonable; nor do I think it would actually work, since the missing gpio phandle at index 4 in cs-gpios would be treated by the spi core as a "that device, if any, uses native chip select 4", and that would/should fail.] Rasmus
On Wed, Apr 26, 2023 at 02:47:44PM +0200, Rasmus Villemoes wrote: > On 26/04/2023 14.25, Mark Brown wrote: > > I'm not sure this is sensible, it'll be a fairly rare situation and we > > don't want to preclude using the built in chip select functionality for > > some of the chip selects. In a situation like this we only need to have > > a single chip select to be managed as a GPIO rather than all of them, > > which I'd expect to end up handled in the DT by not allocating that chip > > select number. > Sorry, I don't understand what you're saying. What exactly is not > sensible? And what is "a situation like this"? Building hardware which uses all the native chip selects and also GPIO ones and then describing it in DT as using native chip selects. > I described a problem with what is now 87c614175bbf in linux-next: If > one has five spi devices, the first four of which use the four native > chip selects, there is no way to use a gpio for the fifth, because > whichever "channel" you choose in the CHANNEL_SELECT field will cause > the ecspi IP block to drive some SSx pin low, while the spi core is also > driving the gpio low, so two different devices would be selected. Sure, and therefore I'd not expect anyone to actually describe the hardware like that but to instead describe the hardware as using three or fewer of the native chip selects with the remaining chip selects described as GPIOs. If the device requires that a native chip select be controlled the hardware simply won't work without at least one native chip select being unallocated. > It's not exactly a regression, because any chip_select >= 4 never > actually worked, but what I'm saying is that 87c614175bbf also isn't a > complete fix if one wants to support mixing native and gpio chip > selects. For that, one really needs the unused_native_cs to be used for > all gpio chip selects; in particular, one needs some unused native cs to > exist. IOW, what my series tries to do. No, we only need one unused chip select to be available.
On 26/04/2023 15.10, Mark Brown wrote: > On Wed, Apr 26, 2023 at 02:47:44PM +0200, Rasmus Villemoes wrote: >> On 26/04/2023 14.25, Mark Brown wrote: >> I described a problem with what is now 87c614175bbf in linux-next: If >> one has five spi devices, the first four of which use the four native >> chip selects, there is no way to use a gpio for the fifth, because >> whichever "channel" you choose in the CHANNEL_SELECT field will cause >> the ecspi IP block to drive some SSx pin low, while the spi core is also >> driving the gpio low, so two different devices would be selected. > > Sure, and therefore I'd not expect anyone to actually describe the > hardware like that but to instead describe the hardware as using three > or fewer of the native chip selects with the remaining chip selects > described as GPIOs. If the device requires that a native chip select be > controlled the hardware simply won't work without at least one native > chip select being unallocated. Exactly. But the current state (as of next-20230425) of the spi-imx driver also doesn't work if one describes the hardware using between 1 and 3 native chip selects and the rest as gpios, because the naive masking of ->chip_select could easily hit one of those native ones. >> It's not exactly a regression, because any chip_select >= 4 never >> actually worked, but what I'm saying is that 87c614175bbf also isn't a >> complete fix if one wants to support mixing native and gpio chip >> selects. For that, one really needs the unused_native_cs to be used for >> all gpio chip selects; in particular, one needs some unused native cs to >> exist. IOW, what my series tries to do. > > No, we only need one unused chip select to be available. Which is exactly what I'm saying, so I think we're in agreement. I.e., something like this 3-patch series is needed to actually support mixing native and gpio chip selects (having the core verify that there is an unused chip select available, and provide that in the ->unused_native_cs field in the spi_controller). I don't think there's any textual conflict with 87c614175bbf, and the masking done by 87c614175bbf doesn't hurt, but also becomes irrelevant if this series is applied, since we'd never pass any value > 3 to those macros. Rasmus
On 26/04/2023 15.23, Rasmus Villemoes wrote: >>> It's not exactly a regression, because any chip_select >= 4 never >>> actually worked, but what I'm saying is that 87c614175bbf also isn't a >>> complete fix if one wants to support mixing native and gpio chip >>> selects. For that, one really needs the unused_native_cs to be used for >>> all gpio chip selects; in particular, one needs some unused native cs to >>> exist. IOW, what my series tries to do. >> >> No, we only need one unused chip select to be available. > > Which is exactly what I'm saying, so I think we're in agreement. > > I.e., something like this 3-patch series is needed to actually support > mixing native and gpio chip selects (having the core verify that there > is an unused chip select available, and provide that in the > ->unused_native_cs field in the spi_controller). I don't think there's > any textual conflict with 87c614175bbf, and the masking done by > 87c614175bbf doesn't hurt, but also becomes irrelevant if this series is > applied, since we'd never pass any value > 3 to those macros. So, what's the conclusion here? Will these three patches be applied, or will we just live with the status as of next-20230516, namely that * for up to four slaves, any combination of native and gpio chip select works * with more then four slaves, CSn and CS(n&3) must be be gpios for all n >= 4 ? Rasmus
On Tue, May 16, 2023 at 01:43:23PM +0200, Rasmus Villemoes wrote: > So, what's the conclusion here? Will these three patches be applied, or > will we just live with the status as of next-20230516, namely that As pointed out by Amit this will need a resend against current code.
On Tue, 25 Apr 2023 15:45:24 +0200, Rasmus Villemoes wrote: > The current spi-imx driver completely fails when used with more than > four (gpio) chip-selects, since the chip select number is used > unconditionally as shift amount when updating the control and > configuration registers, so the code ends up modifying random bits > outside the intended fields. > > This fixes it by making use of the unused_native_cs variable filled in > by the spi core, and use that as the "channel number" for all gpiod > chip selects. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/3] spi: spi-imx: use "controller" variable consistently in spi_imx_probe() commit: d9032b304541e1f560349e461611f25d67f44a49 [2/3] spi: spi-imx: set max_native_cs for imx51/imx53/imx6 variants commit: 8ce1bb9a5935385e9ef65bda1e8ca923c7fbb887 [3/3] spi: spi-imx: fix use of more than four chipselects (no commit info) All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
The current spi-imx driver completely fails when used with more than four (gpio) chip-selects, since the chip select number is used unconditionally as shift amount when updating the control and configuration registers, so the code ends up modifying random bits outside the intended fields. This fixes it by making use of the unused_native_cs variable filled in by the spi core, and use that as the "channel number" for all gpiod chip selects. In the presumably common case where all chip selects are gpios, this means we end up using channel 0 exclusively, so the optimization where the config register is left alone if it is unchanged (see 184434fcd617) might become less effective, if the workload consists of different slaves with differing spi modes being accessed one after the other. It would be nice if one could make use of the unused native chip selects in a round-robin manner, but for that the core would have to tell us not just unused_native_cs, but the whole ~native_cs_mask from spi_get_gpio_descs(). Maybe a simpler fix, if there is anything to fix, is to make the new mx51_ecspi_channel() do if (!spi->cs_gpiod || spi->controller->num_chipselect <= 4) Rasmus Villemoes (3): spi: spi-imx: use "controller" variable consistently in spi_imx_probe() spi: spi-imx: set max_native_cs for imx51/imx53/imx6 variants spi: spi-imx: fix use of more than four chipselects drivers/spi/spi-imx.c | 56 +++++++++++++++++++++++++++---------------- 1 file changed, 35 insertions(+), 21 deletions(-)