* [PATCH 0/2] reset: fix reset_get_by_index_nodev index handling
@ 2021-04-20 8:42 ` Neil Armstrong
0 siblings, 0 replies; 11+ messages in thread
From: Neil Armstrong @ 2021-04-20 8:42 UTC (permalink / raw)
To: u-boot
A regression weas detected on Amlogic G12A/G12B SoCs, where HDMI output was disable
even when Linux was booting.
Bisect reports 139e4a1cbe ("drivers: reset: Add a managed API to get reset controllers from the DT")
as the offending commit.
But the error is in ea9dc35aab ("reset: Get the RESET by index without device") where a spurius "> 0"
was added to the index handling.
But the dm_test_reset_base() test did not catch it.
The first commit extends the test to catch the regression, and the second patch fixes the regression.
Neil Armstrong (2):
test: reset: Extend base reset test to catch error
reset: fix reset_get_by_index_nodev index handling
arch/sandbox/dts/test.dts | 4 ++--
drivers/reset/reset-uclass.c | 2 +-
test/dm/reset.c | 39 +++++++++++++++++++++++++++++++-----
3 files changed, 37 insertions(+), 8 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 0/2] reset: fix reset_get_by_index_nodev index handling @ 2021-04-20 8:42 ` Neil Armstrong 0 siblings, 0 replies; 11+ messages in thread From: Neil Armstrong @ 2021-04-20 8:42 UTC (permalink / raw) To: sjg; +Cc: jagan, u-boot, trini, u-boot-amlogic, Neil Armstrong A regression weas detected on Amlogic G12A/G12B SoCs, where HDMI output was disable even when Linux was booting. Bisect reports 139e4a1cbe ("drivers: reset: Add a managed API to get reset controllers from the DT") as the offending commit. But the error is in ea9dc35aab ("reset: Get the RESET by index without device") where a spurius "> 0" was added to the index handling. But the dm_test_reset_base() test did not catch it. The first commit extends the test to catch the regression, and the second patch fixes the regression. Neil Armstrong (2): test: reset: Extend base reset test to catch error reset: fix reset_get_by_index_nodev index handling arch/sandbox/dts/test.dts | 4 ++-- drivers/reset/reset-uclass.c | 2 +- test/dm/reset.c | 39 +++++++++++++++++++++++++++++++----- 3 files changed, 37 insertions(+), 8 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] test: reset: Extend base reset test to catch error 2021-04-20 8:42 ` Neil Armstrong @ 2021-04-20 8:42 ` Neil Armstrong -1 siblings, 0 replies; 11+ messages in thread From: Neil Armstrong @ 2021-04-20 8:42 UTC (permalink / raw) To: u-boot With this extended test, we get the following failure : => ut dm reset_base Test: dm_test_reset_base: reset.c test/dm/reset.c:52, dm_test_reset_base(): reset_method3.id == reset_method3_1.id: Expected 0x14 (20), got 0x2 (2) Test: dm_test_reset_base: reset.c (flat tree) test/dm/reset.c:52, dm_test_reset_base(): reset_method3.id == reset_method3_1.id: Expected 0x14 (20), got 0x2 (2) Failures: 2 A fix is needed in reset_get_by_index_nodev() when introduced in [1]. [1] ea9dc35aab ("reset: Get the RESET by index without device") Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> --- arch/sandbox/dts/test.dts | 4 ++-- test/dm/reset.c | 39 ++++++++++++++++++++++++++++++++++----- 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts index 48240aa26f..4fde923e9a 100644 --- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -997,8 +997,8 @@ reset-ctl-test { compatible = "sandbox,reset-ctl-test"; - resets = <&resetc 100>, <&resetc 2>; - reset-names = "other", "test"; + resets = <&resetc 100>, <&resetc 2>, <&resetc 20>, <&resetc 40>; + reset-names = "other", "test", "test2", "test3"; }; rng { diff --git a/test/dm/reset.c b/test/dm/reset.c index fc8e9250b0..9c00452336 100644 --- a/test/dm/reset.c +++ b/test/dm/reset.c @@ -24,18 +24,47 @@ static int dm_test_reset_base(struct unit_test_state *uts) { struct udevice *dev; - struct reset_ctl reset_method1; - struct reset_ctl reset_method2; + struct reset_ctl reset_method1, reset_method1_1; + struct reset_ctl reset_method2, reset_method2_1; + struct reset_ctl reset_method3, reset_method3_1; + struct reset_ctl reset_method4, reset_method4_1; /* Get the device using the reset device */ ut_assertok(uclass_get_device_by_name(UCLASS_MISC, "reset-ctl-test", &dev)); /* Get the same reset port in 2 different ways and compare */ - ut_assertok(reset_get_by_index(dev, 1, &reset_method1)); + ut_assertok(reset_get_by_index(dev, 0, &reset_method1)); + ut_assertok(reset_get_by_index_nodev(dev_ofnode(dev), 0, + &reset_method1_1)); + ut_assertok(reset_get_by_index(dev, 1, &reset_method2)); ut_assertok(reset_get_by_index_nodev(dev_ofnode(dev), 1, - &reset_method2)); - ut_asserteq(reset_method1.id, reset_method2.id); + &reset_method2_1)); + ut_assertok(reset_get_by_index(dev, 2, &reset_method3)); + ut_assertok(reset_get_by_index_nodev(dev_ofnode(dev), 2, + &reset_method3_1)); + ut_assertok(reset_get_by_index(dev, 3, &reset_method4)); + ut_assertok(reset_get_by_index_nodev(dev_ofnode(dev), 3, + &reset_method4_1)); + + ut_asserteq(reset_method1.id, reset_method1_1.id); + ut_asserteq(reset_method2.id, reset_method2_1.id); + ut_asserteq(reset_method3.id, reset_method3_1.id); + ut_asserteq(reset_method4.id, reset_method4_1.id); + + ut_asserteq(true, reset_method1.id != reset_method2.id); + ut_asserteq(true, reset_method1.id != reset_method3.id); + ut_asserteq(true, reset_method1.id != reset_method4.id); + ut_asserteq(true, reset_method2.id != reset_method3.id); + ut_asserteq(true, reset_method2.id != reset_method4.id); + ut_asserteq(true, reset_method3.id != reset_method4.id); + + ut_asserteq(true, reset_method1_1.id != reset_method2_1.id); + ut_asserteq(true, reset_method1_1.id != reset_method3_1.id); + ut_asserteq(true, reset_method1_1.id != reset_method4_1.id); + ut_asserteq(true, reset_method2_1.id != reset_method3_1.id); + ut_asserteq(true, reset_method2_1.id != reset_method4_1.id); + ut_asserteq(true, reset_method3_1.id != reset_method4_1.id); return 0; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 1/2] test: reset: Extend base reset test to catch error @ 2021-04-20 8:42 ` Neil Armstrong 0 siblings, 0 replies; 11+ messages in thread From: Neil Armstrong @ 2021-04-20 8:42 UTC (permalink / raw) To: sjg; +Cc: jagan, u-boot, trini, u-boot-amlogic, Neil Armstrong With this extended test, we get the following failure : => ut dm reset_base Test: dm_test_reset_base: reset.c test/dm/reset.c:52, dm_test_reset_base(): reset_method3.id == reset_method3_1.id: Expected 0x14 (20), got 0x2 (2) Test: dm_test_reset_base: reset.c (flat tree) test/dm/reset.c:52, dm_test_reset_base(): reset_method3.id == reset_method3_1.id: Expected 0x14 (20), got 0x2 (2) Failures: 2 A fix is needed in reset_get_by_index_nodev() when introduced in [1]. [1] ea9dc35aab ("reset: Get the RESET by index without device") Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> --- arch/sandbox/dts/test.dts | 4 ++-- test/dm/reset.c | 39 ++++++++++++++++++++++++++++++++++----- 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts index 48240aa26f..4fde923e9a 100644 --- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -997,8 +997,8 @@ reset-ctl-test { compatible = "sandbox,reset-ctl-test"; - resets = <&resetc 100>, <&resetc 2>; - reset-names = "other", "test"; + resets = <&resetc 100>, <&resetc 2>, <&resetc 20>, <&resetc 40>; + reset-names = "other", "test", "test2", "test3"; }; rng { diff --git a/test/dm/reset.c b/test/dm/reset.c index fc8e9250b0..9c00452336 100644 --- a/test/dm/reset.c +++ b/test/dm/reset.c @@ -24,18 +24,47 @@ static int dm_test_reset_base(struct unit_test_state *uts) { struct udevice *dev; - struct reset_ctl reset_method1; - struct reset_ctl reset_method2; + struct reset_ctl reset_method1, reset_method1_1; + struct reset_ctl reset_method2, reset_method2_1; + struct reset_ctl reset_method3, reset_method3_1; + struct reset_ctl reset_method4, reset_method4_1; /* Get the device using the reset device */ ut_assertok(uclass_get_device_by_name(UCLASS_MISC, "reset-ctl-test", &dev)); /* Get the same reset port in 2 different ways and compare */ - ut_assertok(reset_get_by_index(dev, 1, &reset_method1)); + ut_assertok(reset_get_by_index(dev, 0, &reset_method1)); + ut_assertok(reset_get_by_index_nodev(dev_ofnode(dev), 0, + &reset_method1_1)); + ut_assertok(reset_get_by_index(dev, 1, &reset_method2)); ut_assertok(reset_get_by_index_nodev(dev_ofnode(dev), 1, - &reset_method2)); - ut_asserteq(reset_method1.id, reset_method2.id); + &reset_method2_1)); + ut_assertok(reset_get_by_index(dev, 2, &reset_method3)); + ut_assertok(reset_get_by_index_nodev(dev_ofnode(dev), 2, + &reset_method3_1)); + ut_assertok(reset_get_by_index(dev, 3, &reset_method4)); + ut_assertok(reset_get_by_index_nodev(dev_ofnode(dev), 3, + &reset_method4_1)); + + ut_asserteq(reset_method1.id, reset_method1_1.id); + ut_asserteq(reset_method2.id, reset_method2_1.id); + ut_asserteq(reset_method3.id, reset_method3_1.id); + ut_asserteq(reset_method4.id, reset_method4_1.id); + + ut_asserteq(true, reset_method1.id != reset_method2.id); + ut_asserteq(true, reset_method1.id != reset_method3.id); + ut_asserteq(true, reset_method1.id != reset_method4.id); + ut_asserteq(true, reset_method2.id != reset_method3.id); + ut_asserteq(true, reset_method2.id != reset_method4.id); + ut_asserteq(true, reset_method3.id != reset_method4.id); + + ut_asserteq(true, reset_method1_1.id != reset_method2_1.id); + ut_asserteq(true, reset_method1_1.id != reset_method3_1.id); + ut_asserteq(true, reset_method1_1.id != reset_method4_1.id); + ut_asserteq(true, reset_method2_1.id != reset_method3_1.id); + ut_asserteq(true, reset_method2_1.id != reset_method4_1.id); + ut_asserteq(true, reset_method3_1.id != reset_method4_1.id); return 0; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 1/2] test: reset: Extend base reset test to catch error 2021-04-20 8:42 ` Neil Armstrong (?) @ 2021-04-27 16:46 ` Tom Rini -1 siblings, 0 replies; 11+ messages in thread From: Tom Rini @ 2021-04-27 16:46 UTC (permalink / raw) To: u-boot On Tue, Apr 20, 2021 at 10:42:25AM +0200, Neil Armstrong wrote: > With this extended test, we get the following failure : > > => ut dm reset_base > Test: dm_test_reset_base: reset.c > test/dm/reset.c:52, dm_test_reset_base(): reset_method3.id == reset_method3_1.id: Expected 0x14 (20), got 0x2 (2) > Test: dm_test_reset_base: reset.c (flat tree) > test/dm/reset.c:52, dm_test_reset_base(): reset_method3.id == reset_method3_1.id: Expected 0x14 (20), got 0x2 (2) > Failures: 2 > > A fix is needed in reset_get_by_index_nodev() when introduced in [1]. > > [1] ea9dc35aab ("reset: Get the RESET by index without device") > > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> Applied to u-boot/master, thanks! -- 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/20210427/98e48691/attachment.sig> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] reset: fix reset_get_by_index_nodev index handling 2021-04-20 8:42 ` Neil Armstrong @ 2021-04-20 8:42 ` Neil Armstrong -1 siblings, 0 replies; 11+ messages in thread From: Neil Armstrong @ 2021-04-20 8:42 UTC (permalink / raw) To: u-boot This fixes an issue getting resets index 1 and 3+, the spurius "> 0" made it return the index 0 or 1, whatever index was passed. The dm_test_reset_base() did not catch it, but the dm_test_reset_base() extension catches it and this fixes the regression. This also fixes a reggression on Amlogic G12A/G12B SoCs, where HDMI output was disable even when Linux was booting. Fixes: ea9dc35aab ("reset: Get the RESET by index without device") Reported-by: B1oHazard <ty3uk@mail.ua> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> --- drivers/reset/reset-uclass.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/reset/reset-uclass.c b/drivers/reset/reset-uclass.c index 071c389ca0..ac89eaf098 100644 --- a/drivers/reset/reset-uclass.c +++ b/drivers/reset/reset-uclass.c @@ -95,7 +95,7 @@ int reset_get_by_index_nodev(ofnode node, int index, int ret; ret = ofnode_parse_phandle_with_args(node, "resets", "#reset-cells", 0, - index > 0, &args); + index, &args); return reset_get_by_index_tail(ret, node, &args, "resets", index > 0, reset_ctl); -- 2.25.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] reset: fix reset_get_by_index_nodev index handling @ 2021-04-20 8:42 ` Neil Armstrong 0 siblings, 0 replies; 11+ messages in thread From: Neil Armstrong @ 2021-04-20 8:42 UTC (permalink / raw) To: sjg; +Cc: jagan, u-boot, trini, u-boot-amlogic, Neil Armstrong, B1oHazard This fixes an issue getting resets index 1 and 3+, the spurius "> 0" made it return the index 0 or 1, whatever index was passed. The dm_test_reset_base() did not catch it, but the dm_test_reset_base() extension catches it and this fixes the regression. This also fixes a reggression on Amlogic G12A/G12B SoCs, where HDMI output was disable even when Linux was booting. Fixes: ea9dc35aab ("reset: Get the RESET by index without device") Reported-by: B1oHazard <ty3uk@mail.ua> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> --- drivers/reset/reset-uclass.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/reset/reset-uclass.c b/drivers/reset/reset-uclass.c index 071c389ca0..ac89eaf098 100644 --- a/drivers/reset/reset-uclass.c +++ b/drivers/reset/reset-uclass.c @@ -95,7 +95,7 @@ int reset_get_by_index_nodev(ofnode node, int index, int ret; ret = ofnode_parse_phandle_with_args(node, "resets", "#reset-cells", 0, - index > 0, &args); + index, &args); return reset_get_by_index_tail(ret, node, &args, "resets", index > 0, reset_ctl); -- 2.25.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] reset: fix reset_get_by_index_nodev index handling 2021-04-20 8:42 ` Neil Armstrong (?) @ 2021-04-27 16:46 ` Tom Rini -1 siblings, 0 replies; 11+ messages in thread From: Tom Rini @ 2021-04-27 16:46 UTC (permalink / raw) To: u-boot On Tue, Apr 20, 2021 at 10:42:26AM +0200, Neil Armstrong wrote: > This fixes an issue getting resets index 1 and 3+, the spurius "> 0" > made it return the index 0 or 1, whatever index was passed. > > The dm_test_reset_base() did not catch it, but the dm_test_reset_base() extension > catches it and this fixes the regression. > > This also fixes a reggression on Amlogic G12A/G12B SoCs, where HDMI output was disable > even when Linux was booting. > > Fixes: ea9dc35aab ("reset: Get the RESET by index without device") > Reported-by: B1oHazard <ty3uk@mail.ua> > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> Applied to u-boot/master, thanks! -- 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/20210427/039d55c5/attachment.sig> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 0/2] reset: fix reset_get_by_index_nodev index handling 2021-04-20 8:42 ` Neil Armstrong @ 2021-04-27 7:37 ` Neil Armstrong -1 siblings, 0 replies; 11+ messages in thread From: Neil Armstrong @ 2021-04-27 7:37 UTC (permalink / raw) To: u-boot Hi Tom, Simon, On 20/04/2021 10:42, Neil Armstrong wrote: > A regression weas detected on Amlogic G12A/G12B SoCs, where HDMI output was disable > even when Linux was booting. > > Bisect reports 139e4a1cbe ("drivers: reset: Add a managed API to get reset controllers from the DT") > as the offending commit. > > But the error is in ea9dc35aab ("reset: Get the RESET by index without device") where a spurius "> 0" > was added to the index handling. > > But the dm_test_reset_base() test did not catch it. > > The first commit extends the test to catch the regression, and the second patch fixes the regression. So you have any comment on the patch order here ? Should I push the fixed test after the fix ? Neil > > Neil Armstrong (2): > test: reset: Extend base reset test to catch error > reset: fix reset_get_by_index_nodev index handling > > arch/sandbox/dts/test.dts | 4 ++-- > drivers/reset/reset-uclass.c | 2 +- > test/dm/reset.c | 39 +++++++++++++++++++++++++++++++----- > 3 files changed, 37 insertions(+), 8 deletions(-) > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] reset: fix reset_get_by_index_nodev index handling @ 2021-04-27 7:37 ` Neil Armstrong 0 siblings, 0 replies; 11+ messages in thread From: Neil Armstrong @ 2021-04-27 7:37 UTC (permalink / raw) To: sjg, trini; +Cc: jagan, u-boot, u-boot-amlogic Hi Tom, Simon, On 20/04/2021 10:42, Neil Armstrong wrote: > A regression weas detected on Amlogic G12A/G12B SoCs, where HDMI output was disable > even when Linux was booting. > > Bisect reports 139e4a1cbe ("drivers: reset: Add a managed API to get reset controllers from the DT") > as the offending commit. > > But the error is in ea9dc35aab ("reset: Get the RESET by index without device") where a spurius "> 0" > was added to the index handling. > > But the dm_test_reset_base() test did not catch it. > > The first commit extends the test to catch the regression, and the second patch fixes the regression. So you have any comment on the patch order here ? Should I push the fixed test after the fix ? Neil > > Neil Armstrong (2): > test: reset: Extend base reset test to catch error > reset: fix reset_get_by_index_nodev index handling > > arch/sandbox/dts/test.dts | 4 ++-- > drivers/reset/reset-uclass.c | 2 +- > test/dm/reset.c | 39 +++++++++++++++++++++++++++++++----- > 3 files changed, 37 insertions(+), 8 deletions(-) > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 0/2] reset: fix reset_get_by_index_nodev index handling 2021-04-27 7:37 ` Neil Armstrong (?) @ 2021-04-27 11:31 ` Tom Rini -1 siblings, 0 replies; 11+ messages in thread From: Tom Rini @ 2021-04-27 11:31 UTC (permalink / raw) To: u-boot On Tue, Apr 27, 2021 at 09:37:11AM +0200, Neil Armstrong wrote: > Hi Tom, Simon, > > On 20/04/2021 10:42, Neil Armstrong wrote: > > A regression weas detected on Amlogic G12A/G12B SoCs, where HDMI output was disable > > even when Linux was booting. > > > > Bisect reports 139e4a1cbe ("drivers: reset: Add a managed API to get reset controllers from the DT") > > as the offending commit. > > > > But the error is in ea9dc35aab ("reset: Get the RESET by index without device") where a spurius "> 0" > > was added to the index handling. > > > > But the dm_test_reset_base() test did not catch it. > > > > The first commit extends the test to catch the regression, and the second patch fixes the regression. > > So you have any comment on the patch order here ? Should I push the fixed test after the fix ? I'll grab these shortly, thanks for the reminder. -- 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/20210427/f223429e/attachment.sig> ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-04-27 16:46 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-20 8:42 [PATCH 0/2] reset: fix reset_get_by_index_nodev index handling Neil Armstrong 2021-04-20 8:42 ` Neil Armstrong 2021-04-20 8:42 ` [PATCH 1/2] test: reset: Extend base reset test to catch error Neil Armstrong 2021-04-20 8:42 ` Neil Armstrong 2021-04-27 16:46 ` Tom Rini 2021-04-20 8:42 ` [PATCH 2/2] reset: fix reset_get_by_index_nodev index handling Neil Armstrong 2021-04-20 8:42 ` Neil Armstrong 2021-04-27 16:46 ` Tom Rini 2021-04-27 7:37 ` [PATCH 0/2] " Neil Armstrong 2021-04-27 7:37 ` Neil Armstrong 2021-04-27 11:31 ` Tom Rini
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.