From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Rini Date: Thu, 1 Apr 2021 10:18:42 -0400 Subject: [PATCH V2] spi: Update speed/mode on change In-Reply-To: References: <20210329015921.GF1310@bill-the-cat> <919bbb15-bdc8-1727-3333-01bed29ec333@denx.de> <20210331192627.GC1310@bill-the-cat> <20210331194033.GF1310@bill-the-cat> <739d8699-a7cf-3c7e-350e-0ac9bbcc77e4@denx.de> <20210331214505.GH1310@bill-the-cat> Message-ID: <20210401141842.GJ1310@bill-the-cat> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Thu, Apr 01, 2021 at 11:19:27AM +1300, Simon Glass wrote: > +Heinrich Schuchardt > > On Thu, 1 Apr 2021 at 10:45, 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 > > > > > > > > > > > > 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. > > > > 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. > > This does not seem good. I wonder if this patch might have introduced > some instability? > > b308d9fd18f sandbox: Avoid using malloc() for system state The problem is normally seen on master, where this patch is not yet. But on next it fails the same way. -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 659 bytes Desc: not available URL: