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(-)
--
2.37.2
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/[email protected]/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