[2/2] spi: imx: fix use of native chip-selects with devicetree

Commit Message

Greg Ungerer March 17, 2017, 5:03 a.m. UTC

The commonly used mechanism of specifying the hardware or native
chip-select on an SPI device in devicetree (that is "cs-gpios = <0>")
does not result in the native chip-select being configured for use.
So external SPI devices that require use of the native chip-select
will not work.

You can successfully specify native chip-selects if using a platform
setup by specifying the cs-gpio as negative offset by 32. And that
works correctly. You cannot use the same method in devicetree.

The logic in the spi-imx.c driver during probe uses core spi function
of_spi_register_master() in spi.c to parse the "cs-gpios" devicetree tag.
For valid GPIO values that will be recorded for use, all other entries in
the cs_gpios list will be set to -ENOENT. So entries like "<0>" will be
set to -ENOENT in the cs_gpios list.

When the SPI device registers are setup the code will use the GPIO
listed in the cs_gpios list for the desired chip-select. If the cs_gpio
is less then 0 then it is intended to be for a native chip-select, and
its cs_gpio value is added to 32 to get the chipselect number to use.
Problem is that with devicetree this can only ever be -ENOENT (which
is -2), and that alone results in an invalid chip-select number. But also
doesn't allow selection of the native chip-select at all.

To fix, if the cs_gpio specified for this spi device is not a
valid GPIO then use the "chip_select" (that is the native chip-select
number) for hardware setup.

Signed-off-by: Greg Ungerer <[e-mail]>
---
 drivers/spi/spi-imx.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Shawn Guo March 20, 2017, 7:39 a.m. UTC | #1

On Fri, Mar 17, 2017 at 03:03:03PM +1000, Greg Ungerer wrote:
> The commonly used mechanism of specifying the hardware or native
> chip-select on an SPI device in devicetree (that is "cs-gpios = <0>")
> does not result in the native chip-select being configured for use.
> So external SPI devices that require use of the native chip-select
> will not work.
> 
> You can successfully specify native chip-selects if using a platform
> setup by specifying the cs-gpio as negative offset by 32. And that
> works correctly. You cannot use the same method in devicetree.
> 
> The logic in the spi-imx.c driver during probe uses core spi function
> of_spi_register_master() in spi.c to parse the "cs-gpios" devicetree tag.
> For valid GPIO values that will be recorded for use, all other entries in
> the cs_gpios list will be set to -ENOENT. So entries like "<0>" will be
> set to -ENOENT in the cs_gpios list.
> 
> When the SPI device registers are setup the code will use the GPIO
> listed in the cs_gpios list for the desired chip-select. If the cs_gpio
> is less then 0 then it is intended to be for a native chip-select, and
> its cs_gpio value is added to 32 to get the chipselect number to use.
> Problem is that with devicetree this can only ever be -ENOENT (which
> is -2), and that alone results in an invalid chip-select number. But also
> doesn't allow selection of the native chip-select at all.
> 
> To fix, if the cs_gpio specified for this spi device is not a
> valid GPIO then use the "chip_select" (that is the native chip-select
> number) for hardware setup.
> 
> Signed-off-by: Greg Ungerer <[e-mail]>

Acked-by: Shawn Guo <[e-mail]>
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Vladimir Zapolskiy March 20, 2017, 1:22 p.m. UTC | #2

Hi Greg,

On 03/17/2017 07:03 AM, Greg Ungerer wrote:
> The commonly used mechanism of specifying the hardware or native
> chip-select on an SPI device in devicetree (that is "cs-gpios = <0>")
> does not result in the native chip-select being configured for use.
> So external SPI devices that require use of the native chip-select
> will not work.
> 
> You can successfully specify native chip-selects if using a platform
> setup by specifying the cs-gpio as negative offset by 32. And that
> works correctly. You cannot use the same method in devicetree.
> 
> The logic in the spi-imx.c driver during probe uses core spi function
> of_spi_register_master() in spi.c to parse the "cs-gpios" devicetree tag.
> For valid GPIO values that will be recorded for use, all other entries in
> the cs_gpios list will be set to -ENOENT. So entries like "<0>" will be
> set to -ENOENT in the cs_gpios list.
> 
> When the SPI device registers are setup the code will use the GPIO
> listed in the cs_gpios list for the desired chip-select. If the cs_gpio
> is less then 0 then it is intended to be for a native chip-select, and
> its cs_gpio value is added to 32 to get the chipselect number to use.
> Problem is that with devicetree this can only ever be -ENOENT (which
> is -2), and that alone results in an invalid chip-select number. But also
> doesn't allow selection of the native chip-select at all.
> 
> To fix, if the cs_gpio specified for this spi device is not a
> valid GPIO then use the "chip_select" (that is the native chip-select
> number) for hardware setup.
> 

Can you please share an example of "cs-gpios" property for a particular
case, which I'm able to test? It is an Atmel AT93C66A EEPROM connected
to CSPI1 and sitting on chip select 2 (= selected by CSPI1_SS2), there
is no other SPI devices on CSPI1.

Since I raise the question, please add the correspondent updates and
and an example to Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt

In fact my expectation would be to have a device description like one
below:

&spi1 {
	status = "okay";
	pinctrl-names = "default";
	pinctrl-0 = <&pinctrl_cspi1>;

	eeprom@2 {
		compatible = "atmel,at93c66a";
		reg = <2>;
		spi-cs-high;
		spi-max-frequency = <1000000>;
	};
};

Note, that there is no cs-gpios property at all, which is mandatory
at the moment, and unit address / reg property defines chip select
number.

For that type of bindings locally I have a hackish spi-imx driver change,
which supports this option, but I'm unsure if it is universal enough.

--
With best wishes,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Greg Ungerer March 21, 2017, 2:05 a.m. UTC | #3

Hi Vladimir,

On 20/03/17 23:22, Vladimir Zapolskiy wrote:
> On 03/17/2017 07:03 AM, Greg Ungerer wrote:
>> The commonly used mechanism of specifying the hardware or native
>> chip-select on an SPI device in devicetree (that is "cs-gpios = <0>")
>> does not result in the native chip-select being configured for use.
>> So external SPI devices that require use of the native chip-select
>> will not work.
>>
>> You can successfully specify native chip-selects if using a platform
>> setup by specifying the cs-gpio as negative offset by 32. And that
>> works correctly. You cannot use the same method in devicetree.
>>
>> The logic in the spi-imx.c driver during probe uses core spi function
>> of_spi_register_master() in spi.c to parse the "cs-gpios" devicetree tag.
>> For valid GPIO values that will be recorded for use, all other entries in
>> the cs_gpios list will be set to -ENOENT. So entries like "<0>" will be
>> set to -ENOENT in the cs_gpios list.
>>
>> When the SPI device registers are setup the code will use the GPIO
>> listed in the cs_gpios list for the desired chip-select. If the cs_gpio
>> is less then 0 then it is intended to be for a native chip-select, and
>> its cs_gpio value is added to 32 to get the chipselect number to use.
>> Problem is that with devicetree this can only ever be -ENOENT (which
>> is -2), and that alone results in an invalid chip-select number. But also
>> doesn't allow selection of the native chip-select at all.
>>
>> To fix, if the cs_gpio specified for this spi device is not a
>> valid GPIO then use the "chip_select" (that is the native chip-select
>> number) for hardware setup.
>>
> 
> Can you please share an example of "cs-gpios" property for a particular
> case, which I'm able to test? It is an Atmel AT93C66A EEPROM connected
> to CSPI1 and sitting on chip select 2 (= selected by CSPI1_SS2), there
> is no other SPI devices on CSPI1.

Is the AT93C66A going to work if you use native chip selects?

I have an iMX25 based board with a NOR flash on SPI1 using
a GPIO chip select, and on SPI2 a Silicon Labs 32260 SLIC
using the native chip select 1. It requires the chip select line
to be asserted and de-asserted between each byte, as done when
using the native chip select setup of the iMX SPI module.

Anyway, the cs-gpio I use is:

  cs-gpios = <0>, <0>, <0>, <0>;


> Since I raise the question, please add the correspondent updates and
> and an example to Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt

Ok.

There is a few other devicetrees in arc/arm/boot/dts that use the "<0>"
notation for cs-gpios. Documentation/devicetree/bindings/spi/spi-davinci.txt
does describe its meaning.


> In fact my expectation would be to have a device description like one
> below:
> 
> &spi1 {
> 	status = "okay";
> 	pinctrl-names = "default";
> 	pinctrl-0 = <&pinctrl_cspi1>;
> 
> 	eeprom@2 {
> 		compatible = "atmel,at93c66a";
> 		reg = <2>;
> 		spi-cs-high;
> 		spi-max-frequency = <1000000>;
> 	};
> };
> 
> Note, that there is no cs-gpios property at all, which is mandatory
> at the moment, and unit address / reg property defines chip select
> number.

Ideally it would be allowed to have no cs-gpios at all. But currently
the spi-imx driver explicitly fails on this:

  spi.1: No CS GPIOs available

As an example my devicetree entry is similar to your above, but
it has "cs-gpios = <0>, <0>, <0>, <0>;" as well. Thats all.


> For that type of bindings locally I have a hackish spi-imx driver change,
> which supports this option, but I'm unsure if it is universal enough.

Do you mean supporting no cs-gpios tag?
That would be nice, but it would seem not many users of this are
using native chip selects. 

Regards
Greg



--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Uwe Kleine-König March 21, 2017, 8:05 a.m. UTC | #4

Hello,

On Tue, Mar 21, 2017 at 12:05:20PM +1000, Greg Ungerer wrote:
> On 20/03/17 23:22, Vladimir Zapolskiy wrote:
> > For that type of bindings locally I have a hackish spi-imx driver change,
> > which supports this option, but I'm unsure if it is universal enough.
> 
> Do you mean supporting no cs-gpios tag?
> That would be nice, but it would seem not many users of this are
> using native chip selects. 

The reason for this is that the native chip selects are less flexible
than gpios because you cannot control when they deassert. IIRC they do
it too much for some chips. So the only reason to stick to them is that
on some SoCs not all pins have a GPIO function. Not sure if transfer
speed is another reason, but I would expect that the gain isn't that
big.

Best regards
Uwe

Greg Ungerer March 21, 2017, 11:53 a.m. UTC | #5

On 21/03/17 18:05, Uwe Kleine-König wrote:
> On Tue, Mar 21, 2017 at 12:05:20PM +1000, Greg Ungerer wrote:
>> On 20/03/17 23:22, Vladimir Zapolskiy wrote:
>>> For that type of bindings locally I have a hackish spi-imx driver change,
>>> which supports this option, but I'm unsure if it is universal enough.
>>
>> Do you mean supporting no cs-gpios tag?
>> That would be nice, but it would seem not many users of this are
>> using native chip selects.
>
> The reason for this is that the native chip selects are less flexible
> than gpios because you cannot control when they deassert. IIRC they do
> it too much for some chips. So the only reason to stick to them is that
> on some SoCs not all pins have a GPIO function. Not sure if transfer
> speed is another reason, but I would expect that the gain isn't that
> big.

For the particular SPI device I am using, a Silicon Labs 32260,
it actually wants the assertion and de-assertion of the chip-select
between each byte. So it is the only way it can work for me.

Regards
Greg


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Uwe Kleine-König March 21, 2017, 12:11 p.m. UTC | #6

On Tue, Mar 21, 2017 at 09:53:52PM +1000, Greg Ungerer wrote:
> On 21/03/17 18:05, Uwe Kleine-König wrote:
> > On Tue, Mar 21, 2017 at 12:05:20PM +1000, Greg Ungerer wrote:
> > > On 20/03/17 23:22, Vladimir Zapolskiy wrote:
> > > > For that type of bindings locally I have a hackish spi-imx driver change,
> > > > which supports this option, but I'm unsure if it is universal enough.
> > > 
> > > Do you mean supporting no cs-gpios tag?
> > > That would be nice, but it would seem not many users of this are
> > > using native chip selects.
> > 
> > The reason for this is that the native chip selects are less flexible
> > than gpios because you cannot control when they deassert. IIRC they do
> > it too much for some chips. So the only reason to stick to them is that
> > on some SoCs not all pins have a GPIO function. Not sure if transfer
> > speed is another reason, but I would expect that the gain isn't that
> > big.
> 
> For the particular SPI device I am using, a Silicon Labs 32260,
> it actually wants the assertion and de-assertion of the chip-select
> between each byte. So it is the only way it can work for me.

That should be doable with gpio-cs, too. You just need the right flags
in your spi transfer IIRC.

Best regards
Uwe

Greg Ungerer March 21, 2017, 1:22 p.m. UTC | #7

Hi Uwe,

On 21/03/17 22:11, Uwe Kleine-König wrote:
> On Tue, Mar 21, 2017 at 09:53:52PM +1000, Greg Ungerer wrote:
>> On 21/03/17 18:05, Uwe Kleine-König wrote:
>>> On Tue, Mar 21, 2017 at 12:05:20PM +1000, Greg Ungerer wrote:
>>>> On 20/03/17 23:22, Vladimir Zapolskiy wrote:
>>>>> For that type of bindings locally I have a hackish spi-imx driver change,
>>>>> which supports this option, but I'm unsure if it is universal enough.
>>>>
>>>> Do you mean supporting no cs-gpios tag?
>>>> That would be nice, but it would seem not many users of this are
>>>> using native chip selects.
>>>
>>> The reason for this is that the native chip selects are less flexible
>>> than gpios because you cannot control when they deassert. IIRC they do
>>> it too much for some chips. So the only reason to stick to them is that
>>> on some SoCs not all pins have a GPIO function. Not sure if transfer
>>> speed is another reason, but I would expect that the gain isn't that
>>> big.
>>
>> For the particular SPI device I am using, a Silicon Labs 32260,
>> it actually wants the assertion and de-assertion of the chip-select
>> between each byte. So it is the only way it can work for me.
>
> That should be doable with gpio-cs, too. You just need the right flags
> in your spi transfer IIRC.

Do you know which flag(s)  do that?

Regards
Greg



--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Uwe Kleine-König March 21, 2017, 7:23 p.m. UTC | #8

On Tue, Mar 21, 2017 at 11:22:27PM +1000, Greg Ungerer wrote:
> Hi Uwe,
> 
> On 21/03/17 22:11, Uwe Kleine-König wrote:
> > On Tue, Mar 21, 2017 at 09:53:52PM +1000, Greg Ungerer wrote:
> > > On 21/03/17 18:05, Uwe Kleine-König wrote:
> > > > On Tue, Mar 21, 2017 at 12:05:20PM +1000, Greg Ungerer wrote:
> > > > > On 20/03/17 23:22, Vladimir Zapolskiy wrote:
> > > > > > For that type of bindings locally I have a hackish spi-imx driver change,
> > > > > > which supports this option, but I'm unsure if it is universal enough.
> > > > > 
> > > > > Do you mean supporting no cs-gpios tag?
> > > > > That would be nice, but it would seem not many users of this are
> > > > > using native chip selects.
> > > > 
> > > > The reason for this is that the native chip selects are less flexible
> > > > than gpios because you cannot control when they deassert. IIRC they do
> > > > it too much for some chips. So the only reason to stick to them is that
> > > > on some SoCs not all pins have a GPIO function. Not sure if transfer
> > > > speed is another reason, but I would expect that the gain isn't that
> > > > big.
> > > 
> > > For the particular SPI device I am using, a Silicon Labs 32260,
> > > it actually wants the assertion and de-assertion of the chip-select
> > > between each byte. So it is the only way it can work for me.
> > 
> > That should be doable with gpio-cs, too. You just need the right flags
> > in your spi transfer IIRC.
> 
> Do you know which flag(s)  do that?

Looking at the source it's not about flags, but you have to split your
transfer into several messages. AFAICT that's how the spi stuff is
supposed to work. That is, at the start of a message CS is asserted and
(only) at it's end CS is deasserted. So the imx core with native chip
select actually misbehaves by toggling CS between each word.

@broonie: Can you confirm or contradict please?

Best regards
Uwe

Geert Uytterhoeven March 21, 2017, 8:15 p.m. UTC | #9

Hi Uwe,

On Tue, Mar 21, 2017 at 8:23 PM, Uwe Kleine-König
<[e-mail]> wrote:
> On Tue, Mar 21, 2017 at 11:22:27PM +1000, Greg Ungerer wrote:
>> On 21/03/17 22:11, Uwe Kleine-König wrote:
>> > On Tue, Mar 21, 2017 at 09:53:52PM +1000, Greg Ungerer wrote:
>> > > On 21/03/17 18:05, Uwe Kleine-König wrote:
>> > > > On Tue, Mar 21, 2017 at 12:05:20PM +1000, Greg Ungerer wrote:
>> > > > > On 20/03/17 23:22, Vladimir Zapolskiy wrote:
>> > > > > > For that type of bindings locally I have a hackish spi-imx driver change,
>> > > > > > which supports this option, but I'm unsure if it is universal enough.
>> > > > >
>> > > > > Do you mean supporting no cs-gpios tag?
>> > > > > That would be nice, but it would seem not many users of this are
>> > > > > using native chip selects.
>> > > >
>> > > > The reason for this is that the native chip selects are less flexible
>> > > > than gpios because you cannot control when they deassert. IIRC they do
>> > > > it too much for some chips. So the only reason to stick to them is that
>> > > > on some SoCs not all pins have a GPIO function. Not sure if transfer
>> > > > speed is another reason, but I would expect that the gain isn't that
>> > > > big.
>> > >
>> > > For the particular SPI device I am using, a Silicon Labs 32260,
>> > > it actually wants the assertion and de-assertion of the chip-select
>> > > between each byte. So it is the only way it can work for me.
>> >
>> > That should be doable with gpio-cs, too. You just need the right flags
>> > in your spi transfer IIRC.
>>
>> Do you know which flag(s)  do that?
>
> Looking at the source it's not about flags, but you have to split your
> transfer into several messages.

... and set spi_transfer.cs_change.

> AFAICT that's how the spi stuff is
> supposed to work. That is, at the start of a message CS is asserted and
> (only) at it's end CS is deasserted. So the imx core with native chip
> select actually misbehaves by toggling CS between each word.

Indeed.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Greg Ungerer March 22, 2017, 12:50 a.m. UTC | #10

Hi Geert, Uwe,

On 22/03/17 06:15, Geert Uytterhoeven wrote:
> On Tue, Mar 21, 2017 at 8:23 PM, Uwe Kleine-König
> <[e-mail]> wrote:
>> On Tue, Mar 21, 2017 at 11:22:27PM +1000, Greg Ungerer wrote:
>>> On 21/03/17 22:11, Uwe Kleine-König wrote:
>>>> On Tue, Mar 21, 2017 at 09:53:52PM +1000, Greg Ungerer wrote:
>>>>> On 21/03/17 18:05, Uwe Kleine-König wrote:
>>>>>> On Tue, Mar 21, 2017 at 12:05:20PM +1000, Greg Ungerer wrote:
>>>>>>> On 20/03/17 23:22, Vladimir Zapolskiy wrote:
>>>>>>>> For that type of bindings locally I have a hackish spi-imx driver change,
>>>>>>>> which supports this option, but I'm unsure if it is universal enough.
>>>>>>>
>>>>>>> Do you mean supporting no cs-gpios tag?
>>>>>>> That would be nice, but it would seem not many users of this are
>>>>>>> using native chip selects.
>>>>>>
>>>>>> The reason for this is that the native chip selects are less flexible
>>>>>> than gpios because you cannot control when they deassert. IIRC they do
>>>>>> it too much for some chips. So the only reason to stick to them is that
>>>>>> on some SoCs not all pins have a GPIO function. Not sure if transfer
>>>>>> speed is another reason, but I would expect that the gain isn't that
>>>>>> big.
>>>>>
>>>>> For the particular SPI device I am using, a Silicon Labs 32260,
>>>>> it actually wants the assertion and de-assertion of the chip-select
>>>>> between each byte. So it is the only way it can work for me.
>>>>
>>>> That should be doable with gpio-cs, too. You just need the right flags
>>>> in your spi transfer IIRC.
>>>
>>> Do you know which flag(s)  do that?
>>
>> Looking at the source it's not about flags, but you have to split your
>> transfer into several messages.
> 
> ... and set spi_transfer.cs_change.

Yep, that looks like the one. Though it would appear not all
spi drivers support it. The spi-imx driver for one doesn't look
at it at all.


>> AFAICT that's how the spi stuff is
>> supposed to work. That is, at the start of a message CS is asserted and
>> (only) at it's end CS is deasserted. So the imx core with native chip
>> select actually misbehaves by toggling CS between each word.
> 
> Indeed.

If you look around the kernel source for cs_change there is
a number of devices that use it. A bunch od ADC/DACs in
particular (including in backlight support). 

So I don't know that I would characterize the iMX one as
misbehaving so much as only natively supporting the model
where chip select is asserted/deasserted per burst. It is
strait forward to implement the GPIO method instead of the
native chip select with the iMX pin multiplexing.

Regards
Greg


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Geert Uytterhoeven March 22, 2017, 7:09 a.m. UTC | #11

Hi Greg,

On Wed, Mar 22, 2017 at 1:50 AM, Greg Ungerer <[e-mail]> wrote:
> On 22/03/17 06:15, Geert Uytterhoeven wrote:
>> On Tue, Mar 21, 2017 at 8:23 PM, Uwe Kleine-König
>> <[e-mail]> wrote:
>>> On Tue, Mar 21, 2017 at 11:22:27PM +1000, Greg Ungerer wrote:
>>>> On 21/03/17 22:11, Uwe Kleine-König wrote:
>>>>> On Tue, Mar 21, 2017 at 09:53:52PM +1000, Greg Ungerer wrote:
>>>>>> On 21/03/17 18:05, Uwe Kleine-König wrote:
>>>>>>> On Tue, Mar 21, 2017 at 12:05:20PM +1000, Greg Ungerer wrote:
>>>>>>>> On 20/03/17 23:22, Vladimir Zapolskiy wrote:
>>>>>>>>> For that type of bindings locally I have a hackish spi-imx driver change,
>>>>>>>>> which supports this option, but I'm unsure if it is universal enough.
>>>>>>>>
>>>>>>>> Do you mean supporting no cs-gpios tag?
>>>>>>>> That would be nice, but it would seem not many users of this are
>>>>>>>> using native chip selects.
>>>>>>>
>>>>>>> The reason for this is that the native chip selects are less flexible
>>>>>>> than gpios because you cannot control when they deassert. IIRC they do
>>>>>>> it too much for some chips. So the only reason to stick to them is that
>>>>>>> on some SoCs not all pins have a GPIO function. Not sure if transfer
>>>>>>> speed is another reason, but I would expect that the gain isn't that
>>>>>>> big.
>>>>>>
>>>>>> For the particular SPI device I am using, a Silicon Labs 32260,
>>>>>> it actually wants the assertion and de-assertion of the chip-select
>>>>>> between each byte. So it is the only way it can work for me.
>>>>>
>>>>> That should be doable with gpio-cs, too. You just need the right flags
>>>>> in your spi transfer IIRC.
>>>>
>>>> Do you know which flag(s)  do that?
>>>
>>> Looking at the source it's not about flags, but you have to split your
>>> transfer into several messages.
>>
>> ... and set spi_transfer.cs_change.
>
> Yep, that looks like the one. Though it would appear not all
> spi drivers support it. The spi-imx driver for one doesn't look
> at it at all.

Correct. With many drivers, you must use cs-gpios to support that feature.
Remember, SPI is a mixed bag of features, and not all of them are supported
on all controllers.

>>> AFAICT that's how the spi stuff is
>>> supposed to work. That is, at the start of a message CS is asserted and
>>> (only) at it's end CS is deasserted. So the imx core with native chip
>>> select actually misbehaves by toggling CS between each word.
>>
>> Indeed.
>
> If you look around the kernel source for cs_change there is
> a number of devices that use it. A bunch od ADC/DACs in
> particular (including in backlight support).
>
> So I don't know that I would characterize the iMX one as
> misbehaving so much as only natively supporting the model
> where chip select is asserted/deasserted per burst. It is
> strait forward to implement the GPIO method instead of the
> native chip select with the iMX pin multiplexing.

It's up to the system integrator to specify cs-gpios when needed.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Trent Piepho Oct. 10, 2017, 8:38 p.m. UTC | #12

On Fri, 2017-03-17 at 15:03 +1000, Greg Ungerer wrote:
> You can successfully specify native chip-selects if using a platform

> setup by specifying the cs-gpio as negative offset by 32. And that

> works correctly. You cannot use the same method in devicetree.


I came across some spi-imx bugs in an older kernel and in checking the
latest kernel, found this patch.  It fixed the issue with dt not
working for older spi imx devices (the imx51+ config function never
used the platform data 32 offset thing), but fails to address non-dt
platform data based users of the spi imx driver.

For instance, include/linux/platform_data/spi-imx.h still refers to the
offset by 32 for native cs:

 * @chipselect: Array of chipselects for this master. Numbers >= 0 mean gpio
 *              pins, numbers < 0 mean internal CSPI chipselects according
 *              to MXC_SPI_CS(). Normally you want to use gpio based chip

#define MXC_SPI_CS(no)  ((no) - 32)

After this patch, for no-DT users, any negative (or rather not a valid
gpio) value in chipselect will cause the native chipselect
corresponding to the spi slave's chipselect.  It's no longer possible
to specify which native cs to use in the spi-imx platform data.  So in
something like:

arch/arm/mach-imx/mach-mx31moboard.c-static int moboard_spi1_cs[] = {
arch/arm/mach-imx/mach-mx31moboard.c:   MXC_SPI_CS(0),
arch/arm/mach-imx/mach-mx31moboard.c:   MXC_SPI_CS(2),
arch/arm/mach-imx/mach-mx31moboard.c-};

The spi device configured to use chip select 1 will no longer use
native chip select line 2.

Looking at all in-tree users of spi imx platform data, it appears that
this board is the only one in which the cs selected does not match the
array index.  Though there are still several which still use
MSC_SPI_CS() even though it doesn't really work anymore.

Greg Ungerer Oct. 12, 2017, 6:26 a.m. UTC | #13

Hi Trent,

On 11/10/17 06:38, Trent Piepho wrote:
> On Fri, 2017-03-17 at 15:03 +1000, Greg Ungerer wrote:
>> You can successfully specify native chip-selects if using a platform
>> setup by specifying the cs-gpio as negative offset by 32. And that
>> works correctly. You cannot use the same method in devicetree.
> 
> I came across some spi-imx bugs in an older kernel and in checking the
> latest kernel, found this patch.  It fixed the issue with dt not
> working for older spi imx devices (the imx51+ config function never
> used the platform data 32 offset thing), but fails to address non-dt
> platform data based users of the spi imx driver.
> 
> For instance, include/linux/platform_data/spi-imx.h still refers to the
> offset by 32 for native cs:
> 
>  * @chipselect: Array of chipselects for this master. Numbers >= 0 mean gpio
>  *              pins, numbers < 0 mean internal CSPI chipselects according
>  *              to MXC_SPI_CS(). Normally you want to use gpio based chip
> 
> #define MXC_SPI_CS(no)  ((no) - 32)
> 
> After this patch, for no-DT users, any negative (or rather not a valid
> gpio) value in chipselect will cause the native chipselect
> corresponding to the spi slave's chipselect.  It's no longer possible
> to specify which native cs to use in the spi-imx platform data.  So in
> something like:
> 
> arch/arm/mach-imx/mach-mx31moboard.c-static int moboard_spi1_cs[] = {
> arch/arm/mach-imx/mach-mx31moboard.c:   MXC_SPI_CS(0),
> arch/arm/mach-imx/mach-mx31moboard.c:   MXC_SPI_CS(2),
> arch/arm/mach-imx/mach-mx31moboard.c-};
> 
> The spi device configured to use chip select 1 will no longer use
> native chip select line 2.
> 
> Looking at all in-tree users of spi imx platform data, it appears that
> this board is the only one in which the cs selected does not match the
> array index.  Though there are still several which still use
> MSC_SPI_CS() even though it doesn't really work anymore.

Commit 901f26bce64a ("ARM: imx: set correct chip_select in platform setup") 
fixes this issue. It specifically makes sure that the board setup chip
select is set correctly (so that spi-imx can then use it). It also pads
out the chipselect arrays so that the chip select and indexes match.

Is this not working for you?

Regards
Greg
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Trent Piepho Oct. 12, 2017, 8:41 p.m. UTC | #14

On Thu, 2017-10-12 at 16:26 +1000, Greg Ungerer wrote:
> On 11/10/17 06:38, Trent Piepho wrote:

> > 

> > arch/arm/mach-imx/mach-mx31moboard.c-static int moboard_spi1_cs[] = {

> > arch/arm/mach-imx/mach-mx31moboard.c:   MXC_SPI_CS(0),

> > arch/arm/mach-imx/mach-mx31moboard.c:   MXC_SPI_CS(2),

> > arch/arm/mach-imx/mach-mx31moboard.c-};

> > 

> > The spi device configured to use chip select 1 will no longer use

> > native chip select line 2.

> > 

> > Looking at all in-tree users of spi imx platform data, it appears that

> > this board is the only one in which the cs selected does not match the

> > array index.  Though there are still several which still use

> > MSC_SPI_CS() even though it doesn't really work anymore.

> 

> Commit 901f26bce64a ("ARM: imx: set correct chip_select in platform setup") 

> fixes this issue. It specifically makes sure that the board setup chip

> select is set correctly (so that spi-imx can then use it). It also pads

> out the chipselect arrays so that the chip select and indexes match.

> 

> Is this not working for you?


It looks like that patch missed one spi bus in mx31moboard.c, quoted
above.

I have a patch series that should fix that and also the documentation. 
Need to test it a bit more.

Patch

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 9a7c62f..dbb482c 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -516,8 +516,8 @@  static int mx31_config(struct spi_device *spi, struct spi_imx_config *config)
 		reg |= MX31_CSPICTRL_POL;
 	if (spi->mode & SPI_CS_HIGH)
 		reg |= MX31_CSPICTRL_SSPOL;
-	if (spi->cs_gpio < 0)
-		reg |= (spi->cs_gpio + 32) <<
+	if (!gpio_is_valid(spi->cs_gpio))
+		reg |= (spi->chip_select) <<
 			(is_imx35_cspi(spi_imx) ? MX35_CSPICTRL_CS_SHIFT :
 						  MX31_CSPICTRL_CS_SHIFT);
 
@@ -608,8 +608,8 @@  static int mx21_config(struct spi_device *spi, struct spi_imx_config *config)
 		reg |= MX21_CSPICTRL_POL;
 	if (spi->mode & SPI_CS_HIGH)
 		reg |= MX21_CSPICTRL_SSPOL;
-	if (spi->cs_gpio < 0)
-		reg |= (spi->cs_gpio + 32) << MX21_CSPICTRL_CS_SHIFT;
+	if (!gpio_is_valid(spi->cs_gpio))
+		reg |= spi->chip_select << MX21_CSPICTRL_CS_SHIFT;
 
 	writel(reg, spi_imx->base + MXC_CSPICTRL);