[PATCH V2] spi: Update speed/mode on change

[Marek Vasut]

On 3/31/21 11:45 PM, Tom Rini wrote:
> On Wed, Mar 31, 2021 at 10:05:28PM +0200, Marek Vasut wrote:
>> On 3/31/21 9:40 PM, Tom Rini wrote:
>>> On Wed, Mar 31, 2021 at 03:26:27PM -0400, Tom Rini wrote:
>>>> On Wed, Mar 31, 2021 at 09:18:28PM +0200, Marek Vasut wrote:
>>>>> On 3/29/21 4:40 AM, Marek Vasut wrote:
>>>>>> On 3/29/21 4:09 AM, Marek Vasut wrote:
>>>>>>> On 3/29/21 3:59 AM, Tom Rini wrote:
>>>>>>>> On Mon, Mar 29, 2021 at 03:32:51AM +0200, Marek Vasut wrote:
>>>>>>>>> On 3/28/21 11:30 PM, Tom Rini wrote:
>>>>>>>>>> On Fri, Feb 26, 2021 at 03:21:24PM +0100, Marek Vasut wrote:
>>>>>>>>>>
>>>>>>>>>>> The spi_get_bus_and_cs() may be called on the same bus and chipselect
>>>>>>>>>>> with different frequency or mode. This is valid usecase, but the code
>>>>>>>>>>> fails to notify the controller of such a configuration change. Call
>>>>>>>>>>> spi_set_speed_mode() in case bus frequency or bus mode changed to let
>>>>>>>>>>> the controller update the configuration.
>>>>>>>>>>>
>>>>>>>>>>> The problem can easily be triggered using the sspi command:
>>>>>>>>>>> => sspi 0:0 at 1000
>>>>>>>>>>> => sspi 0:0 at 2000
>>>>>>>>>>> Without this patch, both transfers happen at 1000
>>>>>>>>>>> Hz. With this patch,
>>>>>>>>>>> the later transfer happens correctly at 2000 Hz.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Marek Vasut <marex at denx.de>
>>>>>>>>>>> Cc: Jagan Teki <jagan at amarulasolutions.com>
>>>>>>>>>>> Cc: Patrick Delaunay <patrick.delaunay at st.com>
>>>>>>>>>>
>>>>>>>>>> So, very reliably I can make:
>>>>>>>>>> https://source.denx.de/u-boot/u-boot/-/jobs/245517
>>>>>>>>>> happen locally as well building with clang.  It's not
>>>>>>>>>> obvious to me why
>>>>>>>>>> the test now fails however.
>>>>>>>>>
>>>>>>>>> Can you please be more specific / clear ? I have no idea what those 300
>>>>>>>>> lines of cryptic output mean, nor what are you trying to say
>>>>>>>>> by the above,
>>>>>>>>> sorry.
>>>>>>>>
>>>>>>>> If you build with clang, for sandbox, and run the tests, U-Boot crashes
>>>>>>>> in the unit tests that you start with "ut dm".
>>>>>>>
>>>>>>> And that is related to this patch somehow ? How ?
>>>>>>
>>>>>> ... after further discussion off-list to get a better test case
>>>>>> description ...
>>>>>>
>>>>>> It seems the problem is in sound_beep() and it is unrelated to this
>>>>>> patch, as the same problem happens with / without this patch being
>>>>>> applied, on:
>>>>>>
>>>>>> a7d3ac61482 ("Merge branch '2021-03-28-assorted-bugfixes'")
>>>>>>
>>>>>> $ clang --version
>>>>>> Debian clang version 11.0.1-2
>>>>>> ...
>>>>>> $ make CC=clang HOSTCC=clang sandbox_defconfig
>>>>>> $ make CC=clang HOSTCC=clang
>>>>>> $ gdb --args ./u-boot -d arch/sandbox/dts/test.dtb
>>>>>> => ut dm
>>>>>> ...
>>>>>> Program received signal SIGSEGV, Segmentation fault.
>>>>>> dlfree (mem=<optimized out>, mem at entry=0x15c19c50) at
>>>>>> common/dlmalloc.c:1623
>>>>>> 1623      if (!(inuse_bit_at_offset(next, nextsz)))   /* consolidate
>>>>>> forward */
>>>>>> (gdb) bt
>>>>>> #0  dlfree (mem=<optimized out>, mem at entry=0x15c19c50) at
>>>>>> common/dlmalloc.c:1623
>>>>>> #1  0x00000000004759e3 in sound_beep (dev=0x15c05580, msecs=<optimized
>>>>>> out>, frequency_hz=100)
>>>>>>       at drivers/sound/sound-uclass.c:118
>>>>>
>>>>> Tom, any further comments ? It seems your claim that this bugfix is causing
>>>>> problems with clang is not correct, so I think it should still be applied to
>>>>> v2021.04 , unless you can provide more details about the clang problem ?
>>>>
>>>> There's at least a few funny things going on.  Yes, apparently outside
>>>> of CI building sandbox with clang and running the tests manually crashes
>>>> in sound, before it gets to the SPI tests.  Except when it crashes on
>>>> the SPI tests instead, which I did get to happen once.  So there's some
>>>> sort of use-after-free or similar bug around here somewhere, similar to
>>>> how we can't use the -fstack-protector patch as it exposes other real
>>>> problems that need to be fixed first.
>>>>
>>>> So, I'll see if with a new tag and a few other changes having been made
>>>> we once again get to the point where your patch doesn't trigger this
>>>> issue.  But since it's also not a regression fix, if no one can figure
>>>> out what to do about fixing what clang shows us, then it can wait for
>>>> v2021.07.
>>>
>>> Nope, still gets things to blow up:
>>> https://source.denx.de/u-boot/u-boot/-/jobs/246848
>>>
>>> and yes, it would be nice if when the reason a pytest fails is that
>>> U-Boot crashed, it would say something more clear than "OSError: [Errno
>>> 5] Input/output error" and you have to know that means U-Boot died.
>>
>> Can you please provide a detailed test case how to reproduce this ?
>> So far I don't have one, the only test description I got is "install random
>> version of clang, run ut dm and it fails", but that kind of imprecise test
>> description is really not useful. Please provide more specific instructions.
> 
> Yes, follow what CI does.  It's done in a container, so you can have the
> exact same runtime and the tests are in .azure-pipelines.yml and
> .gitlab-ci.yml or if you look at
> https://source.denx.de/u-boot/u-boot/-/jobs/246848/raw you can see the
> commands in there.

Can you please provide me the exact commands to run to reproduce this 
problem ? I just spent a lot of time trying to find this one single 
command in a wall of text, "ut dm spi_find", which I think is the one 
failing for you ? It seems to work for me with and without the patch:

$ ./u-boot -d arch/sandbox/dts/test.dtb
sandbox: starting...


U-Boot 2021.04-rc5-00005-g583b6858733 (Apr 01 2021 - 00:34:09 +0200)

Model: sandbox
DRAM:  128 MiB
WDT:   Started with servicing (60s timeout)
MMC:   mmc2: 2 (SD), mmc1: 1 (SD), mmc0: 0 (SD)
In:    serial
Out:   vidconsole
Err:   vidconsole
Model: sandbox
SCSI:
Net:   eth0: eth at 10002000, eth5: eth at 10003000, eth3: sbe5, eth6: 
eth at 10004000
Hit any key to stop autoboot:  0

Without patch
=> ut dm spi_find
Test: dm_test_spi_find: spi.c
test/dm/spi.c:71, dm_test_spi_find(): 0 == spi_get_bus_and_cs(busnum, 
cs, speed, mode, "jedec_spi_nor", "name", &bus, &slave): Expected 0x0 
(0), got 0xfffffffb (-5)
Test: dm_test_spi_find: spi.c (flat tree)
test/dm/spi.c:71, dm_test_spi_find(): 0 == spi_get_bus_and_cs(busnum, 
cs, speed, mode, "jedec_spi_nor", "name", &bus, &slave): Expected 0x0 
(0), got 0xfffffffb (-5)
Failures: 2

With patch
=> ut dm spi_find
Test: dm_test_spi_find: spi.c
test/dm/spi.c:71, dm_test_spi_find(): 0 == spi_get_bus_and_cs(busnum, 
cs, speed, mode, "jedec_spi_nor", "name", &bus, &slave): Expected 0x0 
(0), got 0xfffffffb (-5)
Test: dm_test_spi_find: spi.c (flat tree)
test/dm/spi.c:71, dm_test_spi_find(): 0 == spi_get_bus_and_cs(busnum, 
cs, speed, mode, "jedec_spi_nor", "name", &bus, &slave): Expected 0x0 
(0), got 0xfffffffb (-5)
Failures: 2