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

[Greg Ungerer <gerg-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>]

Hi Uwe,

On 10/08/17 22:40, Uwe Kleine-König wrote:
> Hello Greg,
> 
> On Thu, Aug 10, 2017 at 10:24:09PM +1000, Greg Ungerer wrote:
>> On 10/08/17 21:49, Uwe Kleine-König wrote:
>>> On Thu, Aug 10, 2017 at 09:35:15PM +1000, Greg Ungerer wrote:
>>>> On 10/08/17 19:35, Vladimir Zapolskiy wrote:
>>>>> Errors in DTB (or platform data) may confuse a driver and lead to runtime
>>>>> misbehaviour. You describe an error in a board DTB, which is definitely
>>>>> better to handle in the SPI driver, but I don't think it is strictly
>>>>> mandatory to do it, because DTB errors are supposed to be fixed in DTB.
>>>>>
>>>>> May be one day a formal check of DTBs against Documentation/devicetree
>>>>> descriptions will be added and such DTB errors could be captured on DTB
>>>>> compilation stage.
>>>>
>>>> I completely agree with Vladmir here. Since "cs-gpios" defines the
>>>> number of chip selects, as per the code you point out, it is the range
>>>> limit. So if a DTB defines it wrongly then you can expect some things
>>>> not to work right. The spi code quite rightly relies on the DTB
>>>> definitions to be correct for proper operation.
>>>>
>>>>
>>>>>> And in this case:
>>>>>> cs-gpios = <&gpio1 0 0>, <&gpio1 1 0>, <&gpio1 2 0>, <&gpio1 3 0>, <0>;
>>>>>>
>>>>>> we should produce a 3 bit value b100 which will be shifted left and
>>>>>> "or"-ed with other ctrl bits.
>>>>
>>>> So the register settings will be wrong and the device will not work.
>>>> You can't really expect any other behavior from an incorrect DTB.
>>>
>>> I think nobody expects that a wrong dtb is good enough to make
>>> everything work. Maybe you can argue what should happen in a driver if
>>> it gets a wrong dtb. It can make the kernel oops, just silently not work
>>> or issue an error message; probably there are more options.
>>>
>>> The current state is that a broken dtb makes the spi-imx driver write to
>>> a unrelated register bit when asked to set a chip select signal. Oleksij
>>> tries to improve that and make the driver error out instead. It makes it
>>> easier for users to report problems or find the culprit themselves. I
>>> like that.
>>
>> But the check if the chip select is valid is based on information
>> that comes from the DTB. In the example of the wrong cs-gpios list
>> the DTB is saying it has more chip selects that the actual hardware
>> device does. How can you possibly protect against that?
>>
>> The current code seems to do its best to range check the chip select,
>> based on what the DTB says this hardware has.
> 
> If I understood correctly the current state is the following:
> 
> The spi-imx driver supports GPIOs as CS and a "native" mode where the
> hardware drives the CS line. If you write:
> 
> 	cs-gpios = <&gpio2 0 0>, <0>, <&gpio1 17 0>
> 
> That means: Use gpio2.0 as CS 0, native CS as CS 1 and gpio1.17 as CS 2.

That is correct, yes. But the current driver is buggy and does not
handle the "native" mode correctly - the intention of the patch
at the start of this thread is to fix this.


> So what the driver can check here: Does the dtb request a native CS at a
> position that is not supported by the hardware. Of course even if there

How does the driver know how many chip-selects are natively supported
by the hardware?

I am not familar with all iMX parts and their spi controllers, but
Oleksij suggested in an earlier email that some have 3 native chip
selects, some have 4, other variants may have some other number.

Regards
Greg


> are only 4 hardware CS the driver can still be used with 7 spi devices
> given that CS 4 .. 6 are using a GPIO as CS.
> 
> Oleksij: please correct me if I'm wrong.
> 
> Best regards
> Uwe
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html