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

Marek Vasut marex at denx.de
Wed Mar 31 22:05:28 CEST 2021

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.

More information about the U-Boot mailing list