From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Thu, 1 Apr 2021 01:47:09 +0200 Subject: [PATCH V2] spi: Update speed/mode on change In-Reply-To: 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> <739d8699-a7cf-3c7e-350e-0ac9bbcc77e4@denx.de> <20210331214505.GH1310@bill-the-cat> <1bebeabe-38cf-b132-0186-b885a5b9dc99@denx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 4/1/21 1:41 AM, Simon Glass wrote: > Hi Marek, > > On Thu, 1 Apr 2021 at 12:19, Marek Vasut wrote: >> >> On 4/1/21 12:50 AM, Simon Glass wrote: >>> Hi Marek, >>> >>> On Thu, 1 Apr 2021 at 11:39, Marek Vasut wrote: >>>> >>>> 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 >>>>>>>>>>>>>>> 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. >>>> >>>> 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 >>> >>> The Python test sets up a SPI-flash file (spi.bin) that is needed by the test. >>> >>> I use this alias: >>> >>> # Run a pytest on sandbox >>> # $1: Name of test to run (optional, else run all) >>> function pyt { >>> test/py/test.py -B sandbox --build-dir /tmp/b/sandbox ${1:+"-k $1"} >>> } >>> >>> then 'pyt spi_find' >>> >>> Once you've done that once you can do what you had before: >>> >>> ./u-boot -T >> >> Do you also have a minimal test which does not depend on pytest at all ? >> A testcase for a bug should be minimal and as simple as possible, it >> shouldn't depend on all kinds of variables which are only adding >> complexity and confusion. > > There is quite a bit more documentation languishing in the -next branch, e.g.: > > https://github.com/u-boot/u-boot/blob/next/doc/develop/tests_sandbox.rst > > The C SPI tests could perhaps be updated to automatically create the > spi.bin file, but several of them share it, so it would need to be a > shared function. > > I certainly only used pytest when needed, hence the substantial effort > I made to document how to run sandbox tests directly, but I should > point out that pytest is the overall framework that we use. Surely for the sake of trivial test , spi.bin file can be generated manually using e.g. dd ?