[U-Boot] [PATCH v2] dm: spi: prevent setting a speed of 0 Hz

[Simon Goldschmidt]

On 23.11.2018 14:12, Jagan Teki wrote:
> On Fri, Nov 23, 2018 at 6:24 PM Simon Goldschmidt
> <sgoldschmidt at de.pepperl-fuchs.com> wrote:
>>
>> [Responding from my work address due to problems with gmail]
>>
>> On 23.11.2018 13:27, Jagan Teki wrote:
>>> On Fri, Nov 23, 2018 at 1:51 AM <sjg at google.com> wrote:
>>>>
>>>> On Fri, 16 Nov 2018 at 12:40, Simon Goldschmidt
>>>> <simon.k.r.goldschmidt at gmail.com> wrote:
>>>>>
>>>>> On 31.10.2018 07:42, Simon Goldschmidt wrote:
>>>>>> On Wed, Oct 31, 2018 at 7:22 AM Jagan Teki <jagan at amarulasolutions.com> wrote:
>>>>>>> On Wed, Oct 31, 2018 at 1:40 AM Simon Goldschmidt
>>>>>>> <simon.k.r.goldschmidt at gmail.com> wrote:
>>>>>>>> When the device tree is missing a correct spi slave description below
>>>>>>>> the bus (compatible "spi-flash" or spi-max-frequency are missing),
>>>>>>>> the 'set_speed' callback can be called with 'speed' == 0 Hz.
>>>>>>>> At least with cadence qspi, this leads to a division by zero.
>>>>>>>>
>>>>>>>> Prevent this by initializing speed to 100 kHz in this case (same
>>>>>>>> fallback value as is done in 'dm_spi_claim_bus') and issue a warning
>>>>>>>> to console.
>>>>>>> Why can't driver plat->frequency in cadence driver initialize 100KH?
>>>>>>> plat->frequency = fdtdec_get_int(blob, node, "spi-max-frequency", 100000)
>>>>>> I'm not sure I understand.
>>>>>> The cadence driver initializes its 'plat->max_hz' from
>>>>>> "spi-max-frequency" property and defaults to 500 kHz. No problem
>>>>>> there.
>>>>>>
>>>>>> However, the problem I want to solve is if someone puts a flash chip
>>>>>> below there which has compatible != "spi-flash", they will get a hard
>>>>>> fault which is hard to debug. This is because the node is not parsed
>>>>>> because of the wrong compatible string (even if there is an
>>>>>> "spi-max-frequency" property) and thus, "sf probe" tries to continue
>>>>>> with 0 Hz.
>>>>>>
>>>>>> And this can happen easily when porting device trees from Linux as
>>>>>> there, the boards have compatible "n25q00" etc. instead of
>>>>>> "spi-flash", which is U-Boot specific (sadly).
>>>>>>
>>>>>> This patch is not required to make valid U-Boot devicetrees work, it
>>>>>> is meant to get better error handling for devicetrees ported from
>>>>>> Linux.
>>>>>>
>>>>>> An even better fix would be for U-Boot not to require the compatible =
>>>>>> "spi-flash" string but just work correctly with Linux device trees,
>>>>>> but that's not within my possibilities right now :-(
>>>>>
>>>>> Ping? This is a bug that should hit nearly everyone porting a board dts
>>>>> working in Linux to U-Boot, can we please have some kind of fix for this?
>>>>> Currently, using a dts with an spi flash chip that works in Linux may
>>>>> abort in U-Boot...
>>>>
>>>> Reviewed-by: Simon Glass <sjg at chromium.org>
>>>>
>>>> Applied to u-boot-dm/master, thanks!
>>>
>>> I hold this because, there is a discussion going on with me and
>>> Simon.k about Linux compatible "jedec,spi-nor".  adding support for
>>> this binding would eventually not reproduce this issue.
>>
>> That's not completely true. Adding "jedec,spi-nor" to U-Boot would
>> definitively make it less likely to run into this issue.
> 
> I don't this is what we discussed above, adding the Linux compatible
> with relevant spi-frequnecy make the issue non-reproducible this is
> what exactly we ended-up.
> 
> 
>>
>> However, Linux works correctly without "jedec,spi-nor", so people can
>> still run into problems when porting a dts from Linux. This is why I
>> still would like to get this in.
> 
> Why? do they specify property?

There are dts files with compatible = "n25q256a" and others. I don't 
find the thread right now, but I said I would try to upstream changes to 
Linux so that the socfgpa_cyclone5 dts files would have an additional 
"jedec,spi-nor" compatible strings.

However, I cannot change all Linux devicetress and having 
"jedec-spi-nor" does *not* seem mandatory for Linux devicetrees.

This patch ensures a devicetree that works with Linux does not lead to a 
"divide-by-zero" abort with U-Boot. I do think that's an improvement for 
U-Boot!

Regards,
Simon