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

[Marek Vasut]

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.