* [PATCH V2] spi: Update speed/mode on change
@ 2021-02-26 14:21 Marek Vasut
2021-03-25 17:41 ` Marek Vasut
2021-03-28 21:30 ` Tom Rini
0 siblings, 2 replies; 31+ messages in thread
From: Marek Vasut @ 2021-02-26 14:21 UTC (permalink / raw)
To: u-boot
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@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Patrick Delaunay <patrick.delaunay@st.com>
---
V2: - Use bus_data->speed for the check
- Release the bus on error
---
drivers/spi/spi-uclass.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
index 7155d4aebd6..5617f6645ee 100644
--- a/drivers/spi/spi-uclass.c
+++ b/drivers/spi/spi-uclass.c
@@ -405,12 +405,22 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
goto err;
}
+ /* In case bus frequency or mode changed, update it. */
+ if ((speed && bus_data->speed && bus_data->speed != speed) ||
+ (plat->mode != mode)) {
+ ret = spi_set_speed_mode(bus, speed, mode);
+ if (ret)
+ goto err_speed_mode;
+ }
+
*busp = bus;
*devp = slave;
log_debug("%s: bus=%p, slave=%p\n", __func__, bus, *devp);
return 0;
+err_speed_mode:
+ spi_release_bus(slave);
err:
log_debug("%s: Error path, created=%d, device '%s'\n", __func__,
created, dev->name);
--
2.30.0
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH V2] spi: Update speed/mode on change 2021-02-26 14:21 [PATCH V2] spi: Update speed/mode on change Marek Vasut @ 2021-03-25 17:41 ` Marek Vasut 2021-03-28 21:30 ` Tom Rini 1 sibling, 0 replies; 31+ messages in thread From: Marek Vasut @ 2021-03-25 17:41 UTC (permalink / raw) To: u-boot On 2/26/21 3:21 PM, 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@denx.de> > Cc: Jagan Teki <jagan@amarulasolutions.com> > Cc: Patrick Delaunay <patrick.delaunay@st.com> This bugfix is still missing from upstream. ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH V2] spi: Update speed/mode on change 2021-02-26 14:21 [PATCH V2] spi: Update speed/mode on change Marek Vasut 2021-03-25 17:41 ` Marek Vasut @ 2021-03-28 21:30 ` Tom Rini 2021-03-29 1:32 ` Marek Vasut 1 sibling, 1 reply; 31+ messages in thread From: Tom Rini @ 2021-03-28 21:30 UTC (permalink / raw) To: u-boot 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@denx.de> > Cc: Jagan Teki <jagan@amarulasolutions.com> > Cc: Patrick Delaunay <patrick.delaunay@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. -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 659 bytes Desc: not available URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210328/f0efd921/attachment.sig> ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH V2] spi: Update speed/mode on change 2021-03-28 21:30 ` Tom Rini @ 2021-03-29 1:32 ` Marek Vasut 2021-03-29 1:59 ` Tom Rini 0 siblings, 1 reply; 31+ messages in thread From: Marek Vasut @ 2021-03-29 1:32 UTC (permalink / raw) To: u-boot 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@denx.de> >> Cc: Jagan Teki <jagan@amarulasolutions.com> >> Cc: Patrick Delaunay <patrick.delaunay@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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH V2] spi: Update speed/mode on change 2021-03-29 1:32 ` Marek Vasut @ 2021-03-29 1:59 ` Tom Rini 2021-03-29 2:09 ` Marek Vasut 0 siblings, 1 reply; 31+ messages in thread From: Tom Rini @ 2021-03-29 1:59 UTC (permalink / raw) To: u-boot 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@denx.de> > > > Cc: Jagan Teki <jagan@amarulasolutions.com> > > > Cc: Patrick Delaunay <patrick.delaunay@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". -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 659 bytes Desc: not available URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210328/f875dfc4/attachment.sig> ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH V2] spi: Update speed/mode on change 2021-03-29 1:59 ` Tom Rini @ 2021-03-29 2:09 ` Marek Vasut 2021-03-29 2:40 ` Marek Vasut 0 siblings, 1 reply; 31+ messages in thread From: Marek Vasut @ 2021-03-29 2:09 UTC (permalink / raw) To: u-boot 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@denx.de> >>>> Cc: Jagan Teki <jagan@amarulasolutions.com> >>>> Cc: Patrick Delaunay <patrick.delaunay@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 ? ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH V2] spi: Update speed/mode on change 2021-03-29 2:09 ` Marek Vasut @ 2021-03-29 2:40 ` Marek Vasut 2021-03-31 19:18 ` Marek Vasut 0 siblings, 1 reply; 31+ messages in thread From: Marek Vasut @ 2021-03-29 2:40 UTC (permalink / raw) To: u-boot 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@denx.de> >>>>> Cc: Jagan Teki <jagan@amarulasolutions.com> >>>>> Cc: Patrick Delaunay <patrick.delaunay@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 #2 0x000000000055e083 in dm_test_sound (uts=0x652400 <global_dm_test_state>) at test/dm/sound.c:28 #3 0x0000000000514c4d in dm_do_test (uts=0x652400 <global_dm_test_state>, test=test at entry=0x621520 <_u_boot_list_2_dm_test_2_dm_test_sound>, of_live=<optimized out>) at test/dm/test-main.c:107 #4 0x00000000005147a4 in dm_test_main (test_name=0x0) at test/dm/test-main.c:189 #5 0x00000000004420c8 in cmd_call (cmdtp=0x61ebf8 <_u_boot_list_2_cmd_2_ut>, flag=<optimized out>, argc=2, argv=0x15bd63d0, repeatable=0x7fffffffda34) at common/command.c:580 #6 cmd_process (flag=<optimized out>, flag at entry=0, argc=<optimized out>, argv=0x15bd63d0, repeatable=0x63b97c <flag_repeat>, ticks=ticks at entry=0x0) at common/command.c:635 #7 0x000000000042c574 in run_pipe_real (pi=0x15bd6300) at common/cli_hush.c:1676 #8 run_list_real (pi=<optimized out>, pi at entry=0x15bd6300) at common/cli_hush.c:1873 #9 0x000000000042b415 in run_list (pi=0x15bd6300) at common/cli_hush.c:2022 #10 parse_stream_outer (inp=inp at entry=0x7fffffffdbe8, flag=<optimized out>, flag at entry=2) at common/cli_hush.c:3206 #11 0x000000000042b57a in parse_file_outer () at common/cli_hush.c:3289 #12 0x0000000000441517 in cli_loop () at common/cli.c:230 #13 0x000000000042a0ad in main_loop () at common/main.c:66 #14 0x000000000042d57e in run_main_loop () at common/board_r.c:588 #15 0x000000000042d32e in initcall_run_list (init_sequence=<optimized out>) at /u-boot/include/initcall.h:46 #16 board_init_r (new_gd=<optimized out>, dest_addr=dest_addr at entry=0) at common/board_r.c:830 #17 0x0000000000403a85 in main (argc=3, argv=0x7fffffffdfa8) at arch/sandbox/cpu/start.c:498 ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH V2] spi: Update speed/mode on change 2021-03-29 2:40 ` Marek Vasut @ 2021-03-31 19:18 ` Marek Vasut 2021-03-31 19:26 ` Tom Rini 0 siblings, 1 reply; 31+ messages in thread From: Marek Vasut @ 2021-03-31 19:18 UTC (permalink / raw) To: u-boot 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@denx.de> >>>>>> Cc: Jagan Teki <jagan@amarulasolutions.com> >>>>>> Cc: Patrick Delaunay <patrick.delaunay@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 ? ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH V2] spi: Update speed/mode on change 2021-03-31 19:18 ` Marek Vasut @ 2021-03-31 19:26 ` Tom Rini 2021-03-31 19:40 ` Tom Rini 0 siblings, 1 reply; 31+ messages in thread From: Tom Rini @ 2021-03-31 19:26 UTC (permalink / raw) To: u-boot 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@denx.de> > > > > > > > Cc: Jagan Teki <jagan@amarulasolutions.com> > > > > > > > Cc: Patrick Delaunay <patrick.delaunay@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. -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 659 bytes Desc: not available URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210331/8ed34325/attachment.sig> ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH V2] spi: Update speed/mode on change 2021-03-31 19:26 ` Tom Rini @ 2021-03-31 19:40 ` Tom Rini 2021-03-31 20:05 ` Marek Vasut 0 siblings, 1 reply; 31+ messages in thread From: Tom Rini @ 2021-03-31 19:40 UTC (permalink / raw) To: u-boot 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@denx.de> > > > > > > > > Cc: Jagan Teki <jagan@amarulasolutions.com> > > > > > > > > Cc: Patrick Delaunay <patrick.delaunay@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. -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 659 bytes Desc: not available URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210331/70292369/attachment.sig> ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH V2] spi: Update speed/mode on change 2021-03-31 19:40 ` Tom Rini @ 2021-03-31 20:05 ` Marek Vasut 2021-03-31 21:45 ` Tom Rini 0 siblings, 1 reply; 31+ messages in thread From: Marek Vasut @ 2021-03-31 20:05 UTC (permalink / raw) To: u-boot 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@denx.de> >>>>>>>>> Cc: Jagan Teki <jagan@amarulasolutions.com> >>>>>>>>> Cc: Patrick Delaunay <patrick.delaunay@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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH V2] spi: Update speed/mode on change 2021-03-31 20:05 ` Marek Vasut @ 2021-03-31 21:45 ` Tom Rini 2021-03-31 22:19 ` Simon Glass 2021-03-31 22:39 ` Marek Vasut 0 siblings, 2 replies; 31+ messages in thread From: Tom Rini @ 2021-03-31 21:45 UTC (permalink / raw) To: u-boot 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 <marex@denx.de> > > > > > > > > > > Cc: Jagan Teki <jagan@amarulasolutions.com> > > > > > > > > > > Cc: Patrick Delaunay <patrick.delaunay@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. 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. -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 659 bytes Desc: not available URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210331/38cdd2fd/attachment.sig> ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH V2] spi: Update speed/mode on change 2021-03-31 21:45 ` Tom Rini @ 2021-03-31 22:19 ` Simon Glass 2021-04-01 14:18 ` Tom Rini 2021-03-31 22:39 ` Marek Vasut 1 sibling, 1 reply; 31+ messages in thread From: Simon Glass @ 2021-03-31 22:19 UTC (permalink / raw) To: u-boot +Heinrich Schuchardt On Thu, 1 Apr 2021 at 10:45, Tom Rini <trini@konsulko.com> 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 <marex@denx.de> > > > > > > > > > > > Cc: Jagan Teki <jagan@amarulasolutions.com> > > > > > > > > > > > Cc: Patrick Delaunay <patrick.delaunay@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. > > 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 I have not seen this issue but did see a strange problem on sandbox when I tried to malloc() more space that was available. I haven't got back to it yet. Regards, Simon ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH V2] spi: Update speed/mode on change 2021-03-31 22:19 ` Simon Glass @ 2021-04-01 14:18 ` Tom Rini 0 siblings, 0 replies; 31+ messages in thread From: Tom Rini @ 2021-04-01 14:18 UTC (permalink / raw) To: u-boot 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 <trini@konsulko.com> 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 <marex@denx.de> > > > > > > > > > > > > Cc: Jagan Teki <jagan@amarulasolutions.com> > > > > > > > > > > > > Cc: Patrick Delaunay <patrick.delaunay@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. > > > > 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: <https://lists.denx.de/pipermail/u-boot/attachments/20210401/476d87fd/attachment.sig> ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH V2] spi: Update speed/mode on change 2021-03-31 21:45 ` Tom Rini 2021-03-31 22:19 ` Simon Glass @ 2021-03-31 22:39 ` Marek Vasut 2021-03-31 22:50 ` Simon Glass 1 sibling, 1 reply; 31+ messages in thread From: Marek Vasut @ 2021-03-31 22:39 UTC (permalink / raw) To: u-boot 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 <marex@denx.de> >>>>>>>>>>> Cc: Jagan Teki <jagan@amarulasolutions.com> >>>>>>>>>>> Cc: Patrick Delaunay <patrick.delaunay@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. > > 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH V2] spi: Update speed/mode on change 2021-03-31 22:39 ` Marek Vasut @ 2021-03-31 22:50 ` Simon Glass 2021-03-31 23:19 ` Marek Vasut 0 siblings, 1 reply; 31+ messages in thread From: Simon Glass @ 2021-03-31 22:50 UTC (permalink / raw) To: u-boot Hi Marek, On Thu, 1 Apr 2021 at 11:39, Marek Vasut <marex@denx.de> 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 <marex@denx.de> > >>>>>>>>>>> Cc: Jagan Teki <jagan@amarulasolutions.com> > >>>>>>>>>>> Cc: Patrick Delaunay <patrick.delaunay@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. > > > > 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 Regards, Simon ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH V2] spi: Update speed/mode on change 2021-03-31 22:50 ` Simon Glass @ 2021-03-31 23:19 ` Marek Vasut 2021-03-31 23:41 ` Simon Glass 0 siblings, 1 reply; 31+ messages in thread From: Marek Vasut @ 2021-03-31 23:19 UTC (permalink / raw) To: u-boot On 4/1/21 12:50 AM, Simon Glass wrote: > Hi Marek, > > On Thu, 1 Apr 2021 at 11:39, Marek Vasut <marex@denx.de> 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 <marex@denx.de> >>>>>>>>>>>>> Cc: Jagan Teki <jagan@amarulasolutions.com> >>>>>>>>>>>>> Cc: Patrick Delaunay <patrick.delaunay@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. >>> >>> 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH V2] spi: Update speed/mode on change 2021-03-31 23:19 ` Marek Vasut @ 2021-03-31 23:41 ` Simon Glass 2021-03-31 23:47 ` Marek Vasut 0 siblings, 1 reply; 31+ messages in thread From: Simon Glass @ 2021-03-31 23:41 UTC (permalink / raw) To: u-boot Hi Marek, On Thu, 1 Apr 2021 at 12:19, Marek Vasut <marex@denx.de> wrote: > > On 4/1/21 12:50 AM, Simon Glass wrote: > > Hi Marek, > > > > On Thu, 1 Apr 2021 at 11:39, Marek Vasut <marex@denx.de> 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 <marex@denx.de> > >>>>>>>>>>>>> Cc: Jagan Teki <jagan@amarulasolutions.com> > >>>>>>>>>>>>> Cc: Patrick Delaunay <patrick.delaunay@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. > >>> > >>> 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. Regards, Simon ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH V2] spi: Update speed/mode on change 2021-03-31 23:41 ` Simon Glass @ 2021-03-31 23:47 ` Marek Vasut 2021-03-31 23:54 ` Tom Rini 0 siblings, 1 reply; 31+ messages in thread From: Marek Vasut @ 2021-03-31 23:47 UTC (permalink / raw) To: u-boot On 4/1/21 1:41 AM, Simon Glass wrote: > Hi Marek, > > On Thu, 1 Apr 2021 at 12:19, Marek Vasut <marex@denx.de> wrote: >> >> On 4/1/21 12:50 AM, Simon Glass wrote: >>> Hi Marek, >>> >>> On Thu, 1 Apr 2021 at 11:39, Marek Vasut <marex@denx.de> 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 <marex@denx.de> >>>>>>>>>>>>>>> Cc: Jagan Teki <jagan@amarulasolutions.com> >>>>>>>>>>>>>>> Cc: Patrick Delaunay <patrick.delaunay@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. >>>>> >>>>> 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 ? ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH V2] spi: Update speed/mode on change 2021-03-31 23:47 ` Marek Vasut @ 2021-03-31 23:54 ` Tom Rini 0 siblings, 0 replies; 31+ messages in thread From: Tom Rini @ 2021-03-31 23:54 UTC (permalink / raw) To: u-boot On Thu, Apr 01, 2021 at 01:47:09AM +0200, Marek Vasut wrote: > On 4/1/21 1:41 AM, Simon Glass wrote: > > Hi Marek, > > > > On Thu, 1 Apr 2021 at 12:19, Marek Vasut <marex@denx.de> wrote: > > > > > > On 4/1/21 12:50 AM, Simon Glass wrote: > > > > Hi Marek, > > > > > > > > On Thu, 1 Apr 2021 at 11:39, Marek Vasut <marex@denx.de> 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 <marex@denx.de> > > > > > > > > > > > > > > > > Cc: Jagan Teki <jagan@amarulasolutions.com> > > > > > > > > > > > > > > > > Cc: Patrick Delaunay <patrick.delaunay@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. > > > > > > > > > > > > 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 ? But part of the problem, quite literally, is that the existing tests for SPI functionality are failing. Given that it seems "this change directly is wrong" is unlikely but "this change exposes some more general problem within any of {sandbox,unit tests,unit test framework,spi}" is likely, there is not yet a more minimal test for the problem because one has not been found. The easy reproducer is "run the tests like CI does". It further seems likely to me that it's a problem in either sandbox or unit test framework as you and I both made that sound test fail sometime. And note that doc/develop/py_testing.rst tells how to hook pytest and GDB up. -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 659 bytes Desc: not available URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210331/c424fdb1/attachment.sig> ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH V2] spi: Update speed/mode on change @ 2021-06-10 12:00 Marek Vasut 2021-06-30 16:49 ` Tom Rini 0 siblings, 1 reply; 31+ messages in thread From: Marek Vasut @ 2021-06-10 12:00 UTC (permalink / raw) To: u-boot; +Cc: Marek Vasut, Jagan Teki, Patrick Delaunay 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@1000 => sspi 0:0@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@denx.de> Cc: Jagan Teki <jagan@amarulasolutions.com> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com> --- V2: Set plat in case it is NULL --- drivers/spi/spi-uclass.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c index ee30110b56..d867b27806 100644 --- a/drivers/spi/spi-uclass.c +++ b/drivers/spi/spi-uclass.c @@ -391,6 +391,8 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, } else if (ret) { dev_err(bus, "Invalid chip select %d:%d (err=%d)\n", busnum, cs, ret); return ret; + } else if (dev) { + plat = dev_get_parent_plat(dev); } if (!device_active(dev)) { @@ -416,12 +418,22 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, goto err; } + /* In case bus frequency or mode changed, update it. */ + if ((speed && bus_data->speed && bus_data->speed != speed) || + (plat && plat->mode != mode)) { + ret = spi_set_speed_mode(bus, speed, mode); + if (ret) + goto err_speed_mode; + } + *busp = bus; *devp = slave; log_debug("%s: bus=%p, slave=%p\n", __func__, bus, *devp); return 0; +err_speed_mode: + spi_release_bus(slave); err: log_debug("%s: Error path, created=%d, device '%s'\n", __func__, created, dev->name); -- 2.30.2 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH V2] spi: Update speed/mode on change 2021-06-10 12:00 Marek Vasut @ 2021-06-30 16:49 ` Tom Rini 2021-07-02 17:24 ` Da Xue 0 siblings, 1 reply; 31+ messages in thread From: Tom Rini @ 2021-06-30 16:49 UTC (permalink / raw) To: Marek Vasut; +Cc: u-boot, Jagan Teki, Patrick Delaunay [-- Attachment #1: Type: text/plain, Size: 832 bytes --] On Thu, Jun 10, 2021 at 02:00:00PM +0200, 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@1000 > => sspi 0:0@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@denx.de> > Cc: Jagan Teki <jagan@amarulasolutions.com> > Cc: Patrick Delaunay <patrick.delaunay@foss.st.com> Applied to u-boot/master, thanks! -- Tom [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 659 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH V2] spi: Update speed/mode on change 2021-06-30 16:49 ` Tom Rini @ 2021-07-02 17:24 ` Da Xue 2021-07-02 17:44 ` Marek Vasut 0 siblings, 1 reply; 31+ messages in thread From: Da Xue @ 2021-07-02 17:24 UTC (permalink / raw) To: Marek Vasut, Neil Armstrong Cc: U-Boot Mailing List, Jagan Teki, Patrick Delaunay, Tom Rini Hi Marek, This patch breaks meson_spifc: SF: Detected gd25lq128 with page size 256 Bytes, erase size 64 KiB, total 16 MiB meson_spifc spi@8c80: Cannot set mode (err=-19) Failed to initialize SPI flash at 0:0 (error -19) Best, Da On Wed, Jun 30, 2021 at 12:49 PM Tom Rini <trini@konsulko.com> wrote: > On Thu, Jun 10, 2021 at 02:00:00PM +0200, 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@1000 > > => sspi 0:0@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@denx.de> > > Cc: Jagan Teki <jagan@amarulasolutions.com> > > Cc: Patrick Delaunay <patrick.delaunay@foss.st.com> > > Applied to u-boot/master, thanks! > > -- > Tom > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH V2] spi: Update speed/mode on change 2021-07-02 17:24 ` Da Xue @ 2021-07-02 17:44 ` Marek Vasut [not found] ` <CACdvmAh4MZ-rACRAY1nv1d0myCMyNnXjR5htkHTpe3A-wm=B1g@mail.gmail.com> 0 siblings, 1 reply; 31+ messages in thread From: Marek Vasut @ 2021-07-02 17:44 UTC (permalink / raw) To: Da Xue, Neil Armstrong Cc: U-Boot Mailing List, Jagan Teki, Patrick Delaunay, Tom Rini On 7/2/21 7:24 PM, Da Xue wrote: > Hi Marek, > > This patch breaks meson_spifc: > > SF: Detected gd25lq128 with page size 256 Bytes, erase size 64 KiB, total > 16 MiB > meson_spifc spi@8c80: Cannot set mode (err=-19) > Failed to initialize SPI flash at 0:0 (error -19) > > Best, > > Da > > On Wed, Jun 30, 2021 at 12:49 PM Tom Rini <trini@konsulko.com> wrote: > >> On Thu, Jun 10, 2021 at 02:00:00PM +0200, 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@1000 >>> => sspi 0:0@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@denx.de> >>> Cc: Jagan Teki <jagan@amarulasolutions.com> >>> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com> >> >> Applied to u-boot/master, thanks! >> >> -- >> Tom Seems like you're hitting this code in drivers/spi/meson_spifc.c 250 static int meson_spifc_set_mode(struct udevice *dev, uint mode) 251 { 252 struct meson_spifc_priv *spifc = dev_get_priv(dev); 253 254 if (mode & (SPI_CPHA | SPI_RX_QUAD | SPI_RX_DUAL | 255 SPI_TX_QUAD | SPI_TX_DUAL)) 256 return -ENODEV; (the -ENODEV code doesn't look right, it should be some -EOPNOTSUP or so) Can you check which of the mode bits is set and triggers the condition ? I think you might be missing something like spi-rx-bus-width = <1>; spi-tx-bus-width = <1>; in your DT, but that's a guess. ^ permalink raw reply [flat|nested] 31+ messages in thread
[parent not found: <CACdvmAh4MZ-rACRAY1nv1d0myCMyNnXjR5htkHTpe3A-wm=B1g@mail.gmail.com>]
* Re: [PATCH V2] spi: Update speed/mode on change [not found] ` <CACdvmAh4MZ-rACRAY1nv1d0myCMyNnXjR5htkHTpe3A-wm=B1g@mail.gmail.com> @ 2021-07-02 18:10 ` Marek Vasut 2021-07-02 18:28 ` Da Xue 0 siblings, 1 reply; 31+ messages in thread From: Marek Vasut @ 2021-07-02 18:10 UTC (permalink / raw) To: Da Xue, u-boot; +Cc: Neil Armstrong On 7/2/21 8:03 PM, Da Xue wrote: > SF: Detected gd25lq128 with page size 256 Bytes, erase size 64 KiB, total > 16 MiB > SPI mode: 3 > meson_spifc spi@8c80: Cannot set mode (err=-19) > Failed to initialize SPI flash at 0:0 (error -19) > > I added spi-rx-bus-width = <1> and spi-tx-bus-width = <1> > > On Fri, Jul 2, 2021 at 1:44 PM Marek Vasut <marex@denx.de> wrote: > >> On 7/2/21 7:24 PM, Da Xue wrote: >>> Hi Marek, >>> >>> This patch breaks meson_spifc: >>> >>> SF: Detected gd25lq128 with page size 256 Bytes, erase size 64 KiB, total >>> 16 MiB >>> meson_spifc spi@8c80: Cannot set mode (err=-19) >>> Failed to initialize SPI flash at 0:0 (error -19) >>> >>> Best, >>> >>> Da >>> >>> On Wed, Jun 30, 2021 at 12:49 PM Tom Rini <trini@konsulko.com> wrote: >>> >>>> On Thu, Jun 10, 2021 at 02:00:00PM +0200, 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@1000 >>>>> => sspi 0:0@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@denx.de> >>>>> Cc: Jagan Teki <jagan@amarulasolutions.com> >>>>> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com> >>>> >>>> Applied to u-boot/master, thanks! >>>> >>>> -- >>>> Tom >> >> Seems like you're hitting this code in drivers/spi/meson_spifc.c >> >> 250 static int meson_spifc_set_mode(struct udevice *dev, uint mode) >> 251 { >> 252 struct meson_spifc_priv *spifc = dev_get_priv(dev); >> 253 >> 254 if (mode & (SPI_CPHA | SPI_RX_QUAD | SPI_RX_DUAL | >> 255 SPI_TX_QUAD | SPI_TX_DUAL)) >> 256 return -ENODEV; >> >> (the -ENODEV code doesn't look right, it should be some -EOPNOTSUP or so) >> >> Can you check which of the mode bits is set and triggers the condition ? >> >> I think you might be missing something like >> spi-rx-bus-width = <1>; >> spi-tx-bus-width = <1>; >> in your DT, but that's a guess. Can you check which of the mode bits is set and triggers the condition ? Also, where in the DT did you add spi-rx-bus-width = <1> and spi-tx-bus-width = <1> ? Finally, please do not top-post and keep the list on CC. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH V2] spi: Update speed/mode on change 2021-07-02 18:10 ` Marek Vasut @ 2021-07-02 18:28 ` Da Xue 2021-07-02 19:06 ` Marek Vasut 0 siblings, 1 reply; 31+ messages in thread From: Da Xue @ 2021-07-02 18:28 UTC (permalink / raw) To: Marek Vasut; +Cc: u-boot, Neil Armstrong On Fri, Jul 2, 2021 at 2:10 PM Marek Vasut <marex@denx.de> wrote: > On 7/2/21 8:03 PM, Da Xue wrote: > > SF: Detected gd25lq128 with page size 256 Bytes, erase size 64 KiB, total > > 16 MiB > > SPI mode: 3 > > meson_spifc spi@8c80: Cannot set mode (err=-19) > > Failed to initialize SPI flash at 0:0 (error -19) > > > > I added spi-rx-bus-width = <1> and spi-tx-bus-width = <1> > > > > On Fri, Jul 2, 2021 at 1:44 PM Marek Vasut <marex@denx.de> wrote: > > > >> On 7/2/21 7:24 PM, Da Xue wrote: > >>> Hi Marek, > >>> > >>> This patch breaks meson_spifc: > >>> > >>> SF: Detected gd25lq128 with page size 256 Bytes, erase size 64 KiB, > total > >>> 16 MiB > >>> meson_spifc spi@8c80: Cannot set mode (err=-19) > >>> Failed to initialize SPI flash at 0:0 (error -19) > >>> > >>> Best, > >>> > >>> Da > >>> > >>> On Wed, Jun 30, 2021 at 12:49 PM Tom Rini <trini@konsulko.com> wrote: > >>> > >>>> On Thu, Jun 10, 2021 at 02:00:00PM +0200, 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@1000 > >>>>> => sspi 0:0@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@denx.de> > >>>>> Cc: Jagan Teki <jagan@amarulasolutions.com> > >>>>> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com> > >>>> > >>>> Applied to u-boot/master, thanks! > >>>> > >>>> -- > >>>> Tom > >> > >> Seems like you're hitting this code in drivers/spi/meson_spifc.c > >> > >> 250 static int meson_spifc_set_mode(struct udevice *dev, uint mode) > >> 251 { > >> 252 struct meson_spifc_priv *spifc = dev_get_priv(dev); > >> 253 > >> 254 if (mode & (SPI_CPHA | SPI_RX_QUAD | SPI_RX_DUAL | > >> 255 SPI_TX_QUAD | SPI_TX_DUAL)) > >> 256 return -ENODEV; > >> > >> (the -ENODEV code doesn't look right, it should be some -EOPNOTSUP or > so) > >> > >> Can you check which of the mode bits is set and triggers the condition ? > >> > >> I think you might be missing something like > >> spi-rx-bus-width = <1>; > >> spi-tx-bus-width = <1>; > >> in your DT, but that's a guess. > > Can you check which of the mode bits is set and triggers the condition ? > > Also, where in the DT did you add spi-rx-bus-width = <1> and > spi-tx-bus-width = <1> ? > > Finally, please do not top-post and keep the list on CC. > My apologies about the top-posting. --- a/arch/arm/dts/meson-gxl-s805x-libretech-ac.dts +++ b/arch/arm/dts/meson-gxl-s805x-libretech-ac.dts @@ -304,6 +304,8 @@ compatible = "jedec,spi-nor"; reg = <0>; spi-max-frequency = <80000000>; + spi-rx-bus-width = <1>; + spi-tx-bus-width = <1>; }; }; ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH V2] spi: Update speed/mode on change 2021-07-02 18:28 ` Da Xue @ 2021-07-02 19:06 ` Marek Vasut 2021-07-02 19:35 ` Da Xue 0 siblings, 1 reply; 31+ messages in thread From: Marek Vasut @ 2021-07-02 19:06 UTC (permalink / raw) To: Da Xue; +Cc: u-boot, Neil Armstrong On 7/2/21 8:28 PM, Da Xue wrote: > On Fri, Jul 2, 2021 at 2:10 PM Marek Vasut <marex@denx.de> wrote: > >> On 7/2/21 8:03 PM, Da Xue wrote: >>> SF: Detected gd25lq128 with page size 256 Bytes, erase size 64 KiB, total >>> 16 MiB >>> SPI mode: 3 >>> meson_spifc spi@8c80: Cannot set mode (err=-19) >>> Failed to initialize SPI flash at 0:0 (error -19) >>> >>> I added spi-rx-bus-width = <1> and spi-tx-bus-width = <1> >>> >>> On Fri, Jul 2, 2021 at 1:44 PM Marek Vasut <marex@denx.de> wrote: >>> >>>> On 7/2/21 7:24 PM, Da Xue wrote: >>>>> Hi Marek, >>>>> >>>>> This patch breaks meson_spifc: >>>>> >>>>> SF: Detected gd25lq128 with page size 256 Bytes, erase size 64 KiB, >> total >>>>> 16 MiB >>>>> meson_spifc spi@8c80: Cannot set mode (err=-19) >>>>> Failed to initialize SPI flash at 0:0 (error -19) >>>>> >>>>> Best, >>>>> >>>>> Da >>>>> >>>>> On Wed, Jun 30, 2021 at 12:49 PM Tom Rini <trini@konsulko.com> wrote: >>>>> >>>>>> On Thu, Jun 10, 2021 at 02:00:00PM +0200, 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@1000 >>>>>>> => sspi 0:0@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@denx.de> >>>>>>> Cc: Jagan Teki <jagan@amarulasolutions.com> >>>>>>> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com> >>>>>> >>>>>> Applied to u-boot/master, thanks! >>>>>> >>>>>> -- >>>>>> Tom >>>> >>>> Seems like you're hitting this code in drivers/spi/meson_spifc.c >>>> >>>> 250 static int meson_spifc_set_mode(struct udevice *dev, uint mode) >>>> 251 { >>>> 252 struct meson_spifc_priv *spifc = dev_get_priv(dev); >>>> 253 >>>> 254 if (mode & (SPI_CPHA | SPI_RX_QUAD | SPI_RX_DUAL | >>>> 255 SPI_TX_QUAD | SPI_TX_DUAL)) >>>> 256 return -ENODEV; >>>> >>>> (the -ENODEV code doesn't look right, it should be some -EOPNOTSUP or >> so) >>>> >>>> Can you check which of the mode bits is set and triggers the condition ? >>>> >>>> I think you might be missing something like >>>> spi-rx-bus-width = <1>; >>>> spi-tx-bus-width = <1>; >>>> in your DT, but that's a guess. >> >> Can you check which of the mode bits is set and triggers the condition ? >> >> Also, where in the DT did you add spi-rx-bus-width = <1> and >> spi-tx-bus-width = <1> ? >> >> Finally, please do not top-post and keep the list on CC. >> > > My apologies about the top-posting. > > --- a/arch/arm/dts/meson-gxl-s805x-libretech-ac.dts > +++ b/arch/arm/dts/meson-gxl-s805x-libretech-ac.dts > @@ -304,6 +304,8 @@ > compatible = "jedec,spi-nor"; > reg = <0>; > spi-max-frequency = <80000000>; > + spi-rx-bus-width = <1>; > + spi-tx-bus-width = <1>; > }; > }; That should do the trick. Can you check which of the mode bits is set in meson_spifc_set_mode() and triggers the ENODEV condition ? ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH V2] spi: Update speed/mode on change 2021-07-02 19:06 ` Marek Vasut @ 2021-07-02 19:35 ` Da Xue 2021-07-02 19:40 ` Marek Vasut 0 siblings, 1 reply; 31+ messages in thread From: Da Xue @ 2021-07-02 19:35 UTC (permalink / raw) To: Marek Vasut; +Cc: u-boot, Neil Armstrong On Fri, Jul 2, 2021 at 3:06 PM Marek Vasut <marex@denx.de> wrote: > On 7/2/21 8:28 PM, Da Xue wrote: > > On Fri, Jul 2, 2021 at 2:10 PM Marek Vasut <marex@denx.de> wrote: > > > >> On 7/2/21 8:03 PM, Da Xue wrote: > >>> SF: Detected gd25lq128 with page size 256 Bytes, erase size 64 KiB, > total > >>> 16 MiB > >>> SPI mode: 3 > >>> meson_spifc spi@8c80: Cannot set mode (err=-19) > >>> Failed to initialize SPI flash at 0:0 (error -19) > >>> > >>> I added spi-rx-bus-width = <1> and spi-tx-bus-width = <1> > >>> > >>> On Fri, Jul 2, 2021 at 1:44 PM Marek Vasut <marex@denx.de> wrote: > >>> > >>>> On 7/2/21 7:24 PM, Da Xue wrote: > >>>>> Hi Marek, > >>>>> > >>>>> This patch breaks meson_spifc: > >>>>> > >>>>> SF: Detected gd25lq128 with page size 256 Bytes, erase size 64 KiB, > >> total > >>>>> 16 MiB > >>>>> meson_spifc spi@8c80: Cannot set mode (err=-19) > >>>>> Failed to initialize SPI flash at 0:0 (error -19) > >>>>> > >>>>> Best, > >>>>> > >>>>> Da > >>>>> > >>>>> On Wed, Jun 30, 2021 at 12:49 PM Tom Rini <trini@konsulko.com> > wrote: > >>>>> > >>>>>> On Thu, Jun 10, 2021 at 02:00:00PM +0200, 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@1000 > >>>>>>> => sspi 0:0@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@denx.de> > >>>>>>> Cc: Jagan Teki <jagan@amarulasolutions.com> > >>>>>>> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com> > >>>>>> > >>>>>> Applied to u-boot/master, thanks! > >>>>>> > >>>>>> -- > >>>>>> Tom > >>>> > >>>> Seems like you're hitting this code in drivers/spi/meson_spifc.c > >>>> > >>>> 250 static int meson_spifc_set_mode(struct udevice *dev, uint mode) > >>>> 251 { > >>>> 252 struct meson_spifc_priv *spifc = dev_get_priv(dev); > >>>> 253 > >>>> 254 if (mode & (SPI_CPHA | SPI_RX_QUAD | SPI_RX_DUAL | > >>>> 255 SPI_TX_QUAD | SPI_TX_DUAL)) > >>>> 256 return -ENODEV; > >>>> > >>>> (the -ENODEV code doesn't look right, it should be some -EOPNOTSUP or > >> so) > >>>> > >>>> Can you check which of the mode bits is set and triggers the > condition ? > >>>> > >>>> I think you might be missing something like > >>>> spi-rx-bus-width = <1>; > >>>> spi-tx-bus-width = <1>; > >>>> in your DT, but that's a guess. > >> > >> Can you check which of the mode bits is set and triggers the condition ? > >> > >> Also, where in the DT did you add spi-rx-bus-width = <1> and > >> spi-tx-bus-width = <1> ? > >> > >> Finally, please do not top-post and keep the list on CC. > >> > > > > My apologies about the top-posting. > > > > --- a/arch/arm/dts/meson-gxl-s805x-libretech-ac.dts > > +++ b/arch/arm/dts/meson-gxl-s805x-libretech-ac.dts > > @@ -304,6 +304,8 @@ > > compatible = "jedec,spi-nor"; > > reg = <0>; > > spi-max-frequency = <80000000>; > > + spi-rx-bus-width = <1>; > > + spi-tx-bus-width = <1>; > > }; > > }; > > That should do the trick. Can you check which of the mode bits is set in > meson_spifc_set_mode() and triggers the ENODEV condition ? > SPI_CPHA seems to be the culprit. I tried adding spi-cpha = <0> to no avail. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH V2] spi: Update speed/mode on change 2021-07-02 19:35 ` Da Xue @ 2021-07-02 19:40 ` Marek Vasut 2021-07-02 20:10 ` Da Xue 0 siblings, 1 reply; 31+ messages in thread From: Marek Vasut @ 2021-07-02 19:40 UTC (permalink / raw) To: Da Xue; +Cc: u-boot, Neil Armstrong On 7/2/21 9:35 PM, Da Xue wrote: [...] >>>>>> Seems like you're hitting this code in drivers/spi/meson_spifc.c >>>>>> >>>>>> 250 static int meson_spifc_set_mode(struct udevice *dev, uint mode) >>>>>> 251 { >>>>>> 252 struct meson_spifc_priv *spifc = dev_get_priv(dev); >>>>>> 253 >>>>>> 254 if (mode & (SPI_CPHA | SPI_RX_QUAD | SPI_RX_DUAL | >>>>>> 255 SPI_TX_QUAD | SPI_TX_DUAL)) >>>>>> 256 return -ENODEV; >>>>>> >>>>>> (the -ENODEV code doesn't look right, it should be some -EOPNOTSUP or >>>> so) >>>>>> >>>>>> Can you check which of the mode bits is set and triggers the >> condition ? >>>>>> >>>>>> I think you might be missing something like >>>>>> spi-rx-bus-width = <1>; >>>>>> spi-tx-bus-width = <1>; >>>>>> in your DT, but that's a guess. >>>> >>>> Can you check which of the mode bits is set and triggers the condition ? >>>> >>>> Also, where in the DT did you add spi-rx-bus-width = <1> and >>>> spi-tx-bus-width = <1> ? >>>> >>>> Finally, please do not top-post and keep the list on CC. >>>> >>> >>> My apologies about the top-posting. >>> >>> --- a/arch/arm/dts/meson-gxl-s805x-libretech-ac.dts >>> +++ b/arch/arm/dts/meson-gxl-s805x-libretech-ac.dts >>> @@ -304,6 +304,8 @@ >>> compatible = "jedec,spi-nor"; >>> reg = <0>; >>> spi-max-frequency = <80000000>; >>> + spi-rx-bus-width = <1>; >>> + spi-tx-bus-width = <1>; >>> }; >>> }; >> >> That should do the trick. Can you check which of the mode bits is set in >> meson_spifc_set_mode() and triggers the ENODEV condition ? >> > > SPI_CPHA seems to be the culprit. I tried adding spi-cpha = <0> to no > avail. Can you find out what is setting the SPI_CPHA in the first place on your machine ? ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH V2] spi: Update speed/mode on change 2021-07-02 19:40 ` Marek Vasut @ 2021-07-02 20:10 ` Da Xue 2021-07-02 20:20 ` Marek Vasut 0 siblings, 1 reply; 31+ messages in thread From: Da Xue @ 2021-07-02 20:10 UTC (permalink / raw) To: Marek Vasut; +Cc: u-boot, Neil Armstrong On Fri, Jul 2, 2021 at 3:40 PM Marek Vasut <marex@denx.de> wrote: > On 7/2/21 9:35 PM, Da Xue wrote: > > [...] > > >>>>>> Seems like you're hitting this code in drivers/spi/meson_spifc.c > >>>>>> > >>>>>> 250 static int meson_spifc_set_mode(struct udevice *dev, uint mode) > >>>>>> 251 { > >>>>>> 252 struct meson_spifc_priv *spifc = dev_get_priv(dev); > >>>>>> 253 > >>>>>> 254 if (mode & (SPI_CPHA | SPI_RX_QUAD | SPI_RX_DUAL | > >>>>>> 255 SPI_TX_QUAD | SPI_TX_DUAL)) > >>>>>> 256 return -ENODEV; > >>>>>> > >>>>>> (the -ENODEV code doesn't look right, it should be some -EOPNOTSUP > or > >>>> so) > >>>>>> > >>>>>> Can you check which of the mode bits is set and triggers the > >> condition ? > >>>>>> > >>>>>> I think you might be missing something like > >>>>>> spi-rx-bus-width = <1>; > >>>>>> spi-tx-bus-width = <1>; > >>>>>> in your DT, but that's a guess. > >>>> > >>>> Can you check which of the mode bits is set and triggers the > condition ? > >>>> > >>>> Also, where in the DT did you add spi-rx-bus-width = <1> and > >>>> spi-tx-bus-width = <1> ? > >>>> > >>>> Finally, please do not top-post and keep the list on CC. > >>>> > >>> > >>> My apologies about the top-posting. > >>> > >>> --- a/arch/arm/dts/meson-gxl-s805x-libretech-ac.dts > >>> +++ b/arch/arm/dts/meson-gxl-s805x-libretech-ac.dts > >>> @@ -304,6 +304,8 @@ > >>> compatible = "jedec,spi-nor"; > >>> reg = <0>; > >>> spi-max-frequency = <80000000>; > >>> + spi-rx-bus-width = <1>; > >>> + spi-tx-bus-width = <1>; > >>> }; > >>> }; > >> > >> That should do the trick. Can you check which of the mode bits is set in > >> meson_spifc_set_mode() and triggers the ENODEV condition ? > >> > > > > SPI_CPHA seems to be the culprit. I tried adding spi-cpha = <0> to no > > avail. > > Can you find out what is setting the SPI_CPHA in the first place on your > machine ? > The Kconfig was setting it to 3. I manually added the default mode (0) to my board's config and it worked. Thanks Marek. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH V2] spi: Update speed/mode on change 2021-07-02 20:10 ` Da Xue @ 2021-07-02 20:20 ` Marek Vasut 0 siblings, 0 replies; 31+ messages in thread From: Marek Vasut @ 2021-07-02 20:20 UTC (permalink / raw) To: Da Xue; +Cc: u-boot, Neil Armstrong, Tom Rini On 7/2/21 10:10 PM, Da Xue wrote: [...] >>>> That should do the trick. Can you check which of the mode bits is set in >>>> meson_spifc_set_mode() and triggers the ENODEV condition ? >>>> >>> >>> SPI_CPHA seems to be the culprit. I tried adding spi-cpha = <0> to no >>> avail. >> >> Can you find out what is setting the SPI_CPHA in the first place on your >> machine ? >> > > The Kconfig was setting it to 3. I manually added the default mode (0) to > my board's config and it worked. Thanks Marek. Sure, good thing this was fixed. Please send a patch for the board, so it can be added to this release. ^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2021-07-02 20:20 UTC | newest]
Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-26 14:21 [PATCH V2] spi: Update speed/mode on change Marek Vasut
2021-03-25 17:41 ` Marek Vasut
2021-03-28 21:30 ` Tom Rini
2021-03-29 1:32 ` Marek Vasut
2021-03-29 1:59 ` Tom Rini
2021-03-29 2:09 ` Marek Vasut
2021-03-29 2:40 ` Marek Vasut
2021-03-31 19:18 ` Marek Vasut
2021-03-31 19:26 ` Tom Rini
2021-03-31 19:40 ` Tom Rini
2021-03-31 20:05 ` Marek Vasut
2021-03-31 21:45 ` Tom Rini
2021-03-31 22:19 ` Simon Glass
2021-04-01 14:18 ` Tom Rini
2021-03-31 22:39 ` Marek Vasut
2021-03-31 22:50 ` Simon Glass
2021-03-31 23:19 ` Marek Vasut
2021-03-31 23:41 ` Simon Glass
2021-03-31 23:47 ` Marek Vasut
2021-03-31 23:54 ` Tom Rini
2021-06-10 12:00 Marek Vasut
2021-06-30 16:49 ` Tom Rini
2021-07-02 17:24 ` Da Xue
2021-07-02 17:44 ` Marek Vasut
[not found] ` <CACdvmAh4MZ-rACRAY1nv1d0myCMyNnXjR5htkHTpe3A-wm=B1g@mail.gmail.com>
2021-07-02 18:10 ` Marek Vasut
2021-07-02 18:28 ` Da Xue
2021-07-02 19:06 ` Marek Vasut
2021-07-02 19:35 ` Da Xue
2021-07-02 19:40 ` Marek Vasut
2021-07-02 20:10 ` Da Xue
2021-07-02 20:20 ` Marek Vasut
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.