From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Wed, 31 Mar 2021 22:05:28 +0200 Subject: [PATCH V2] spi: Update speed/mode on change In-Reply-To: <20210331194033.GF1310@bill-the-cat> References: <20210226142124.56305-1-marex@denx.de> <20210328213055.GB1310@bill-the-cat> <20210329015921.GF1310@bill-the-cat> <919bbb15-bdc8-1727-3333-01bed29ec333@denx.de> <20210331192627.GC1310@bill-the-cat> <20210331194033.GF1310@bill-the-cat> Message-ID: <739d8699-a7cf-3c7e-350e-0ac9bbcc77e4@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>>>>>>>> Cc: Jagan Teki >>>>>>>>> Cc: Patrick Delaunay >>>>>>>> >>>>>>>> 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=, mem at entry=0x15c19c50) at >>>> common/dlmalloc.c:1623 >>>> 1623????? if (!(inuse_bit_at_offset(next, nextsz)))?? /* consolidate >>>> forward */ >>>> (gdb) bt >>>> #0? dlfree (mem=, mem at entry=0x15c19c50) at >>>> common/dlmalloc.c:1623 >>>> #1? 0x00000000004759e3 in sound_beep (dev=0x15c05580, msecs=>>> 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.