[PATCH 0/2] reset: fix reset_get_by_index_nodev index handling

[PATCH 0/2] reset: fix reset_get_by_index_nodev index handling

[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

[PATCH 0/2] reset: fix reset_get_by_index_nodev index handling

[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

[PATCH 1/2] test: reset: Extend base reset test to catch error

[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

[PATCH 1/2] test: reset: Extend base reset test to catch error

[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

[PATCH 2/2] reset: fix reset_get_by_index_nodev index handling

[Neil Armstrong]

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

[PATCH 2/2] reset: fix reset_get_by_index_nodev index handling

[Neil Armstrong]

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

[PATCH 0/2] reset: fix reset_get_by_index_nodev index handling

[Neil Armstrong]

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(-)
> 

Re: [PATCH 0/2] reset: fix reset_get_by_index_nodev index handling

[Neil Armstrong]

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(-)
> 

[PATCH 0/2] reset: fix reset_get_by_index_nodev index handling

[Tom Rini]

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>

[PATCH 1/2] test: reset: Extend base reset test to catch error

[Tom Rini]

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>

[PATCH 2/2] reset: fix reset_get_by_index_nodev index handling

[Tom Rini]

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>