[PATCH 0/6] rockchip: rk3328 fixes in preparation for usb3-phy

[PATCH 0/6] rockchip: rk3328 fixes in preparation for usb3-phy

[Peter Geis]

This is a series of fixes I uncovered during my work on the next
generation rk3328 usb3 phy driver.

The first patch fixes the error handling of the pm-domain driver. I
don't expect this to break anything, but it is entirely possible some
driver code makes some bad assumptions on the fact that this has been
broken from the very beginning.

The second patch fixes the ref_usb3otg clock parent. This was preventing
correct reclocking of the usb3 phy.

The third patch fixes the ethernet alias that was accidentlly readded
during the rk3328-roc dtsi conversion.

The fourth patch fixes a race condition between power domains and clocks
being shut off during boot, which would cause an ugly splat on rk3328
during boot on recent kernels.

The fifth patch corrects the rk3328-roc fixed regulators and power input
map. It also cleans up the fixed regulator flags to be consistent.

The sixth patch removes address aligned beats and the redundant rxpbl
and txpbl flags from the rk3328-roc, which are unnecessary now.

Please examine and test these as necessary, especially the pm-domain fix
patch.

Very Respectfully,
Peter Geis


Peter Geis (6):
  pmdomain: rockchip: fix rockchip_pd_power error handling
  clk: rockchip: fix wrong clk_ref_usb3otg parent for rk3328
  arm64: dts: rockchip: remove ethernet alias from rk3328-roc
  arm64: dts: rockchip: add hevc power domain clock to rk3328
  arm64: dts: rockchip: correct rk3328-roc regulator map
  arm64: dts: rockchip: Remove address aligned beats from rk3328-roc

 arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi | 27 +++++++++++---------
 arch/arm64/boot/dts/rockchip/rk3328.dtsi     |  1 +
 drivers/clk/rockchip/clk-rk3328.c            |  2 +-
 drivers/pmdomain/rockchip/pm-domains.c       |  8 ++++--
 4 files changed, 23 insertions(+), 15 deletions(-)

-- 
2.39.5

[PATCH 1/6] pmdomain: rockchip: fix rockchip_pd_power error handling

[Peter Geis]

The calls rockchip_pd_power makes to rockchip_pmu_set_idle_request lack
any return error handling, causing device drivers to incorrectly believe
the hardware idle requests succeed when they may have failed. This leads
to software possibly accessing hardware that is powered off and the
subsequent SError panic that follows.

Add error checking and return errors to the calling function to prevent
such crashes.

gst-launch-1.0 videotestsrc num-buffers=2000 ! v4l2jpegenc ! fakesink
Setting pipeline to PAUSED ...er-x64
Pipeline is PREROLLING ...
Redistribute latency...
rockchip-pm-domain ff100000.syscon:power-controller: failed to get ack on
domain 'hevc', val=0x98260, idle = 0
SError Interrupt on CPU2, code 0x00000000bf000002 -- SError
CPU: 2 UID: 0 PID: 804 Comm: videotestsrc0:s Not tainted 6.12.0-rc5+ #54
Hardware name: Firefly roc-rk3328-cc (DT)
pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : rockchip_vpu2_jpeg_enc_run+0x168/0xbc8
lr : device_run+0xb0/0x128
sp : ffff800082143a20
x29: ffff800082143a20 x28: 0000000000000140 x27: 0000000000000000
x26: ffff582c47a313e8 x25: ffff582c53e95000 x24: ffff582c53e92800
x23: ffff582c5bbe0000 x22: 0000000000000000 x21: ffff582c47a31080
x20: ffffa0d78cfa4168 x19: ffffa0d78cfa4168 x18: ffffb755b0519000
x17: 000000040044ffff x16: 00500072b5503510 x15: a7a6a5a4a3a29a99
x14: 989796959493928a x13: 0000000051eb851f x12: 00000000000000ff
x11: ffffa0d78d812880 x10: ffffa0d78d7fbca0 x9 : 000000000000003f
x8 : 0000000000000063 x7 : 000000000000003f x6 : 0000000000000040
x5 : ffff80008010d000 x4 : ffffa0d78cfa4168 x3 : ffffa0d78cfbfdd8
x2 : ffff80008010d0f4 x1 : 0000000000000020 x0 : 0000000000000140
Kernel panic - not syncing: Asynchronous SError Interrupt
CPU: 2 UID: 0 PID: 804 Comm: videotestsrc0:s Not tainted 6.12.0-rc5+ #54
Hardware name: Firefly roc-rk3328-cc (DT)
Call trace:
dump_backtrace+0xa0/0x128
show_stack+0x20/0x38
dump_stack_lvl+0xc8/0xf8
dump_stack+0x18/0x28
panic+0x3ec/0x428
nmi_panic+0x48/0xa0
arm64_serror_panic+0x6c/0x88
do_serror+0x30/0x70
el1h_64_error_handler+0x38/0x60
el1h_64_error+0x7c/0x80
rockchip_vpu2_jpeg_enc_run+0x168/0xbc8
device_run+0xb0/0x128
v4l2_m2m_try_run+0xac/0x230
v4l2_m2m_ioctl_streamon+0x70/0x90
v4l_streamon+0x2c/0x40
__video_do_ioctl+0x194/0x400
video_usercopy+0x10c/0x808
video_ioctl2+0x20/0x80
v4l2_ioctl+0x48/0x70
__arm64_sys_ioctl+0xb0/0x100
invoke_syscall+0x50/0x120
el0_svc_common.constprop.0+0x48/0xf0
do_el0_svc+0x24/0x38
el0_svc+0x38/0x100
el0t_64_sync_handler+0xc0/0xc8
el0t_64_sync+0x1a8/0x1b0
SMP: stopping secondary CPUs
Kernel Offset: 0x20d70c000000 from 0xffff800080000000
PHYS_OFFSET: 0xffffa7d3c0000000
CPU features: 0x00,00000090,00200000,0200421b
Memory Limit: none
---[ end Kernel panic - not syncing: Asynchronous SError Interrupt ]---

Fixes: 7c696693a4f5 ("soc: rockchip: power-domain: Add power domain driver")
Signed-off-by: Peter Geis <pgwipeout@gmail.com>
---

 drivers/pmdomain/rockchip/pm-domains.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
index cb0f93800138..57e8fa25d2bd 100644
--- a/drivers/pmdomain/rockchip/pm-domains.c
+++ b/drivers/pmdomain/rockchip/pm-domains.c
@@ -590,14 +590,18 @@ static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
 			rockchip_pmu_save_qos(pd);
 
 			/* if powering down, idle request to NIU first */
-			rockchip_pmu_set_idle_request(pd, true);
+			ret = rockchip_pmu_set_idle_request(pd, true);
+			if (ret < 0)
+				return ret;
 		}
 
 		rockchip_do_pmu_set_power_domain(pd, power_on);
 
 		if (power_on) {
 			/* if powering up, leave idle mode */
-			rockchip_pmu_set_idle_request(pd, false);
+			ret = rockchip_pmu_set_idle_request(pd, false);
+			if (ret < 0)
+				return ret;
 
 			rockchip_pmu_restore_qos(pd);
 		}
-- 
2.39.5

Re: [PATCH 1/6] pmdomain: rockchip: fix rockchip_pd_power error handling

[Dragan Simic]

Hello Peter,

On 2024-12-10 02:30, Peter Geis wrote:
> The calls rockchip_pd_power makes to rockchip_pmu_set_idle_request lack
> any return error handling, causing device drivers to incorrectly 
> believe
> the hardware idle requests succeed when they may have failed. This 
> leads
> to software possibly accessing hardware that is powered off and the
> subsequent SError panic that follows.
> 
> Add error checking and return errors to the calling function to prevent
> such crashes.
> 
> gst-launch-1.0 videotestsrc num-buffers=2000 ! v4l2jpegenc ! fakesink
> Setting pipeline to PAUSED ...er-x64
> Pipeline is PREROLLING ...
> Redistribute latency...
> rockchip-pm-domain ff100000.syscon:power-controller: failed to get ack 
> on
> domain 'hevc', val=0x98260, idle = 0
> SError Interrupt on CPU2, code 0x00000000bf000002 -- SError
> CPU: 2 UID: 0 PID: 804 Comm: videotestsrc0:s Not tainted 6.12.0-rc5+ 
> #54
> Hardware name: Firefly roc-rk3328-cc (DT)
> pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : rockchip_vpu2_jpeg_enc_run+0x168/0xbc8
> lr : device_run+0xb0/0x128
> sp : ffff800082143a20
> x29: ffff800082143a20 x28: 0000000000000140 x27: 0000000000000000
> x26: ffff582c47a313e8 x25: ffff582c53e95000 x24: ffff582c53e92800
> x23: ffff582c5bbe0000 x22: 0000000000000000 x21: ffff582c47a31080
> x20: ffffa0d78cfa4168 x19: ffffa0d78cfa4168 x18: ffffb755b0519000
> x17: 000000040044ffff x16: 00500072b5503510 x15: a7a6a5a4a3a29a99
> x14: 989796959493928a x13: 0000000051eb851f x12: 00000000000000ff
> x11: ffffa0d78d812880 x10: ffffa0d78d7fbca0 x9 : 000000000000003f
> x8 : 0000000000000063 x7 : 000000000000003f x6 : 0000000000000040
> x5 : ffff80008010d000 x4 : ffffa0d78cfa4168 x3 : ffffa0d78cfbfdd8
> x2 : ffff80008010d0f4 x1 : 0000000000000020 x0 : 0000000000000140
> Kernel panic - not syncing: Asynchronous SError Interrupt
> CPU: 2 UID: 0 PID: 804 Comm: videotestsrc0:s Not tainted 6.12.0-rc5+ 
> #54
> Hardware name: Firefly roc-rk3328-cc (DT)
> Call trace:
> dump_backtrace+0xa0/0x128
> show_stack+0x20/0x38
> dump_stack_lvl+0xc8/0xf8
> dump_stack+0x18/0x28
> panic+0x3ec/0x428
> nmi_panic+0x48/0xa0
> arm64_serror_panic+0x6c/0x88
> do_serror+0x30/0x70
> el1h_64_error_handler+0x38/0x60
> el1h_64_error+0x7c/0x80
> rockchip_vpu2_jpeg_enc_run+0x168/0xbc8
> device_run+0xb0/0x128
> v4l2_m2m_try_run+0xac/0x230
> v4l2_m2m_ioctl_streamon+0x70/0x90
> v4l_streamon+0x2c/0x40
> __video_do_ioctl+0x194/0x400
> video_usercopy+0x10c/0x808
> video_ioctl2+0x20/0x80
> v4l2_ioctl+0x48/0x70
> __arm64_sys_ioctl+0xb0/0x100
> invoke_syscall+0x50/0x120
> el0_svc_common.constprop.0+0x48/0xf0
> do_el0_svc+0x24/0x38
> el0_svc+0x38/0x100
> el0t_64_sync_handler+0xc0/0xc8
> el0t_64_sync+0x1a8/0x1b0
> SMP: stopping secondary CPUs
> Kernel Offset: 0x20d70c000000 from 0xffff800080000000
> PHYS_OFFSET: 0xffffa7d3c0000000
> CPU features: 0x00,00000090,00200000,0200421b
> Memory Limit: none
> ---[ end Kernel panic - not syncing: Asynchronous SError Interrupt ]---
> 
> Fixes: 7c696693a4f5 ("soc: rockchip: power-domain: Add power domain 
> driver")
> Signed-off-by: Peter Geis <pgwipeout@gmail.com>

This patch is obviously correct, because not checking what
rockchip_pmu_set_idle_request() returns was simply wrong.
Thanks for the patch!

Though, shouldn't we improve further the way proper error
handling is performed in rockchip_do_pmu_set_power_domain(),
by "rolling back" what rockchip_do_pmu_set_power_domain()
did after powering up fails?

> ---
> 
>  drivers/pmdomain/rockchip/pm-domains.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pmdomain/rockchip/pm-domains.c
> b/drivers/pmdomain/rockchip/pm-domains.c
> index cb0f93800138..57e8fa25d2bd 100644
> --- a/drivers/pmdomain/rockchip/pm-domains.c
> +++ b/drivers/pmdomain/rockchip/pm-domains.c
> @@ -590,14 +590,18 @@ static int rockchip_pd_power(struct
> rockchip_pm_domain *pd, bool power_on)
>  			rockchip_pmu_save_qos(pd);
> 
>  			/* if powering down, idle request to NIU first */
> -			rockchip_pmu_set_idle_request(pd, true);
> +			ret = rockchip_pmu_set_idle_request(pd, true);
> +			if (ret < 0)
> +				return ret;
>  		}
> 
>  		rockchip_do_pmu_set_power_domain(pd, power_on);
> 
>  		if (power_on) {
>  			/* if powering up, leave idle mode */
> -			rockchip_pmu_set_idle_request(pd, false);
> +			ret = rockchip_pmu_set_idle_request(pd, false);
> +			if (ret < 0)
> +				return ret;
> 
>  			rockchip_pmu_restore_qos(pd);
>  		}

Re: [PATCH 1/6] pmdomain: rockchip: fix rockchip_pd_power error handling

[Peter Geis]

On Tue, Dec 10, 2024 at 3:18 AM Dragan Simic <dsimic@manjaro.org> wrote:
>
> Hello Peter,
>
> On 2024-12-10 02:30, Peter Geis wrote:
> > The calls rockchip_pd_power makes to rockchip_pmu_set_idle_request lack
> > any return error handling, causing device drivers to incorrectly
> > believe
> > the hardware idle requests succeed when they may have failed. This
> > leads
> > to software possibly accessing hardware that is powered off and the
> > subsequent SError panic that follows.
> >
> > Add error checking and return errors to the calling function to prevent
> > such crashes.
> >
> > gst-launch-1.0 videotestsrc num-buffers=2000 ! v4l2jpegenc ! fakesink
> > Setting pipeline to PAUSED ...er-x64
> > Pipeline is PREROLLING ...
> > Redistribute latency...
> > rockchip-pm-domain ff100000.syscon:power-controller: failed to get ack
> > on
> > domain 'hevc', val=0x98260, idle = 0
> > SError Interrupt on CPU2, code 0x00000000bf000002 -- SError
> > CPU: 2 UID: 0 PID: 804 Comm: videotestsrc0:s Not tainted 6.12.0-rc5+
> > #54
> > Hardware name: Firefly roc-rk3328-cc (DT)
> > pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > pc : rockchip_vpu2_jpeg_enc_run+0x168/0xbc8
> > lr : device_run+0xb0/0x128
> > sp : ffff800082143a20
> > x29: ffff800082143a20 x28: 0000000000000140 x27: 0000000000000000
> > x26: ffff582c47a313e8 x25: ffff582c53e95000 x24: ffff582c53e92800
> > x23: ffff582c5bbe0000 x22: 0000000000000000 x21: ffff582c47a31080
> > x20: ffffa0d78cfa4168 x19: ffffa0d78cfa4168 x18: ffffb755b0519000
> > x17: 000000040044ffff x16: 00500072b5503510 x15: a7a6a5a4a3a29a99
> > x14: 989796959493928a x13: 0000000051eb851f x12: 00000000000000ff
> > x11: ffffa0d78d812880 x10: ffffa0d78d7fbca0 x9 : 000000000000003f
> > x8 : 0000000000000063 x7 : 000000000000003f x6 : 0000000000000040
> > x5 : ffff80008010d000 x4 : ffffa0d78cfa4168 x3 : ffffa0d78cfbfdd8
> > x2 : ffff80008010d0f4 x1 : 0000000000000020 x0 : 0000000000000140
> > Kernel panic - not syncing: Asynchronous SError Interrupt
> > CPU: 2 UID: 0 PID: 804 Comm: videotestsrc0:s Not tainted 6.12.0-rc5+
> > #54
> > Hardware name: Firefly roc-rk3328-cc (DT)
> > Call trace:
> > dump_backtrace+0xa0/0x128
> > show_stack+0x20/0x38
> > dump_stack_lvl+0xc8/0xf8
> > dump_stack+0x18/0x28
> > panic+0x3ec/0x428
> > nmi_panic+0x48/0xa0
> > arm64_serror_panic+0x6c/0x88
> > do_serror+0x30/0x70
> > el1h_64_error_handler+0x38/0x60
> > el1h_64_error+0x7c/0x80
> > rockchip_vpu2_jpeg_enc_run+0x168/0xbc8
> > device_run+0xb0/0x128
> > v4l2_m2m_try_run+0xac/0x230
> > v4l2_m2m_ioctl_streamon+0x70/0x90
> > v4l_streamon+0x2c/0x40
> > __video_do_ioctl+0x194/0x400
> > video_usercopy+0x10c/0x808
> > video_ioctl2+0x20/0x80
> > v4l2_ioctl+0x48/0x70
> > __arm64_sys_ioctl+0xb0/0x100
> > invoke_syscall+0x50/0x120
> > el0_svc_common.constprop.0+0x48/0xf0
> > do_el0_svc+0x24/0x38
> > el0_svc+0x38/0x100
> > el0t_64_sync_handler+0xc0/0xc8
> > el0t_64_sync+0x1a8/0x1b0
> > SMP: stopping secondary CPUs
> > Kernel Offset: 0x20d70c000000 from 0xffff800080000000
> > PHYS_OFFSET: 0xffffa7d3c0000000
> > CPU features: 0x00,00000090,00200000,0200421b
> > Memory Limit: none
> > ---[ end Kernel panic - not syncing: Asynchronous SError Interrupt ]---
> >
> > Fixes: 7c696693a4f5 ("soc: rockchip: power-domain: Add power domain
> > driver")
> > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
>
> This patch is obviously correct, because not checking what
> rockchip_pmu_set_idle_request() returns was simply wrong.
> Thanks for the patch!
>
> Though, shouldn't we improve further the way proper error
> handling is performed in rockchip_do_pmu_set_power_domain(),
> by "rolling back" what rockchip_do_pmu_set_power_domain()
> did after powering up fails?

Eventually, but the reality is if we hit this path the hardware is
already broken. I wanted to provide a fix with the least amount of
risk of breaking things further. I'm open to suggestions for further
improvements here.

Current behavior with this patch with the example above causes
gstreamer to wait indefinitely. I'll trace the return path and see if
returning an error other than -ETIMEDOUT triggers more robust
handling.

>
> > ---
> >
> >  drivers/pmdomain/rockchip/pm-domains.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pmdomain/rockchip/pm-domains.c
> > b/drivers/pmdomain/rockchip/pm-domains.c
> > index cb0f93800138..57e8fa25d2bd 100644
> > --- a/drivers/pmdomain/rockchip/pm-domains.c
> > +++ b/drivers/pmdomain/rockchip/pm-domains.c
> > @@ -590,14 +590,18 @@ static int rockchip_pd_power(struct
> > rockchip_pm_domain *pd, bool power_on)
> >                       rockchip_pmu_save_qos(pd);
> >
> >                       /* if powering down, idle request to NIU first */
> > -                     rockchip_pmu_set_idle_request(pd, true);
> > +                     ret = rockchip_pmu_set_idle_request(pd, true);
> > +                     if (ret < 0)
> > +                             return ret;
> >               }
> >
> >               rockchip_do_pmu_set_power_domain(pd, power_on);
> >
> >               if (power_on) {
> >                       /* if powering up, leave idle mode */
> > -                     rockchip_pmu_set_idle_request(pd, false);
> > +                     ret = rockchip_pmu_set_idle_request(pd, false);
> > +                     if (ret < 0)
> > +                             return ret;
> >
> >                       rockchip_pmu_restore_qos(pd);
> >               }

Re: [PATCH 1/6] pmdomain: rockchip: fix rockchip_pd_power error handling

[Dragan Simic]

Hello Peter,

On 2024-12-10 21:12, Peter Geis wrote:
> On Tue, Dec 10, 2024 at 3:18 AM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> On 2024-12-10 02:30, Peter Geis wrote:
>> > The calls rockchip_pd_power makes to rockchip_pmu_set_idle_request lack
>> > any return error handling, causing device drivers to incorrectly
>> > believe
>> > the hardware idle requests succeed when they may have failed. This
>> > leads
>> > to software possibly accessing hardware that is powered off and the
>> > subsequent SError panic that follows.
>> >
>> > Add error checking and return errors to the calling function to prevent
>> > such crashes.
>> >
>> > gst-launch-1.0 videotestsrc num-buffers=2000 ! v4l2jpegenc ! fakesink
>> > Setting pipeline to PAUSED ...er-x64
>> > Pipeline is PREROLLING ...
>> > Redistribute latency...
>> > rockchip-pm-domain ff100000.syscon:power-controller: failed to get ack
>> > on
>> > domain 'hevc', val=0x98260, idle = 0
>> > SError Interrupt on CPU2, code 0x00000000bf000002 -- SError
>> > CPU: 2 UID: 0 PID: 804 Comm: videotestsrc0:s Not tainted 6.12.0-rc5+
>> > #54
>> > Hardware name: Firefly roc-rk3328-cc (DT)
>> > pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>> > pc : rockchip_vpu2_jpeg_enc_run+0x168/0xbc8
>> > lr : device_run+0xb0/0x128
>> > sp : ffff800082143a20
>> > x29: ffff800082143a20 x28: 0000000000000140 x27: 0000000000000000
>> > x26: ffff582c47a313e8 x25: ffff582c53e95000 x24: ffff582c53e92800
>> > x23: ffff582c5bbe0000 x22: 0000000000000000 x21: ffff582c47a31080
>> > x20: ffffa0d78cfa4168 x19: ffffa0d78cfa4168 x18: ffffb755b0519000
>> > x17: 000000040044ffff x16: 00500072b5503510 x15: a7a6a5a4a3a29a99
>> > x14: 989796959493928a x13: 0000000051eb851f x12: 00000000000000ff
>> > x11: ffffa0d78d812880 x10: ffffa0d78d7fbca0 x9 : 000000000000003f
>> > x8 : 0000000000000063 x7 : 000000000000003f x6 : 0000000000000040
>> > x5 : ffff80008010d000 x4 : ffffa0d78cfa4168 x3 : ffffa0d78cfbfdd8
>> > x2 : ffff80008010d0f4 x1 : 0000000000000020 x0 : 0000000000000140
>> > Kernel panic - not syncing: Asynchronous SError Interrupt
>> > CPU: 2 UID: 0 PID: 804 Comm: videotestsrc0:s Not tainted 6.12.0-rc5+
>> > #54
>> > Hardware name: Firefly roc-rk3328-cc (DT)
>> > Call trace:
>> > dump_backtrace+0xa0/0x128
>> > show_stack+0x20/0x38
>> > dump_stack_lvl+0xc8/0xf8
>> > dump_stack+0x18/0x28
>> > panic+0x3ec/0x428
>> > nmi_panic+0x48/0xa0
>> > arm64_serror_panic+0x6c/0x88
>> > do_serror+0x30/0x70
>> > el1h_64_error_handler+0x38/0x60
>> > el1h_64_error+0x7c/0x80
>> > rockchip_vpu2_jpeg_enc_run+0x168/0xbc8
>> > device_run+0xb0/0x128
>> > v4l2_m2m_try_run+0xac/0x230
>> > v4l2_m2m_ioctl_streamon+0x70/0x90
>> > v4l_streamon+0x2c/0x40
>> > __video_do_ioctl+0x194/0x400
>> > video_usercopy+0x10c/0x808
>> > video_ioctl2+0x20/0x80
>> > v4l2_ioctl+0x48/0x70
>> > __arm64_sys_ioctl+0xb0/0x100
>> > invoke_syscall+0x50/0x120
>> > el0_svc_common.constprop.0+0x48/0xf0
>> > do_el0_svc+0x24/0x38
>> > el0_svc+0x38/0x100
>> > el0t_64_sync_handler+0xc0/0xc8
>> > el0t_64_sync+0x1a8/0x1b0
>> > SMP: stopping secondary CPUs
>> > Kernel Offset: 0x20d70c000000 from 0xffff800080000000
>> > PHYS_OFFSET: 0xffffa7d3c0000000
>> > CPU features: 0x00,00000090,00200000,0200421b
>> > Memory Limit: none
>> > ---[ end Kernel panic - not syncing: Asynchronous SError Interrupt ]---
>> >
>> > Fixes: 7c696693a4f5 ("soc: rockchip: power-domain: Add power domain
>> > driver")
>> > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
>> 
>> This patch is obviously correct, because not checking what
>> rockchip_pmu_set_idle_request() returns was simply wrong.
>> Thanks for the patch!
>> 
>> Though, shouldn't we improve further the way proper error
>> handling is performed in rockchip_do_pmu_set_power_domain(),
>> by "rolling back" what rockchip_do_pmu_set_power_domain()
>> did after powering up fails?
> 
> Eventually, but the reality is if we hit this path the hardware is
> already broken. I wanted to provide a fix with the least amount of
> risk of breaking things further. I'm open to suggestions for further
> improvements here.

Perhaps a good approach, at the moment, would be to add no more
code for the "rollback", but to add a TODO note about the need for
that addition.  That way we'd keep the amount of changes at the
minimum, to hopefully not cause any regressions, while leaving
a note for the second round of improvements in the future.

> Current behavior with this patch with the example above causes
> gstreamer to wait indefinitely. I'll trace the return path and see if
> returning an error other than -ETIMEDOUT triggers more robust
> handling.

Sounds like a plan to me.  Thanks for working on this!

>> 
>> > ---
>> >
>> >  drivers/pmdomain/rockchip/pm-domains.c | 8 ++++++--
>> >  1 file changed, 6 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/pmdomain/rockchip/pm-domains.c
>> > b/drivers/pmdomain/rockchip/pm-domains.c
>> > index cb0f93800138..57e8fa25d2bd 100644
>> > --- a/drivers/pmdomain/rockchip/pm-domains.c
>> > +++ b/drivers/pmdomain/rockchip/pm-domains.c
>> > @@ -590,14 +590,18 @@ static int rockchip_pd_power(struct
>> > rockchip_pm_domain *pd, bool power_on)
>> >                       rockchip_pmu_save_qos(pd);
>> >
>> >                       /* if powering down, idle request to NIU first */
>> > -                     rockchip_pmu_set_idle_request(pd, true);
>> > +                     ret = rockchip_pmu_set_idle_request(pd, true);
>> > +                     if (ret < 0)
>> > +                             return ret;
>> >               }
>> >
>> >               rockchip_do_pmu_set_power_domain(pd, power_on);
>> >
>> >               if (power_on) {
>> >                       /* if powering up, leave idle mode */
>> > -                     rockchip_pmu_set_idle_request(pd, false);
>> > +                     ret = rockchip_pmu_set_idle_request(pd, false);
>> > +                     if (ret < 0)
>> > +                             return ret;
>> >
>> >                       rockchip_pmu_restore_qos(pd);
>> >               }

Re: [PATCH 1/6] pmdomain: rockchip: fix rockchip_pd_power error handling

[Dan Carpenter]

Hi Peter,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Peter-Geis/pmdomain-rockchip-fix-rockchip_pd_power-error-handling/20241210-093424
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git for-next
patch link:    https://lore.kernel.org/r/20241210013010.81257-2-pgwipeout%40gmail.com
patch subject: [PATCH 1/6] pmdomain: rockchip: fix rockchip_pd_power error handling
config: powerpc-randconfig-r072-20241223 (https://download.01.org/0day-ci/archive/20241224/202412240015.MfjYhpNz-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 14.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202412240015.MfjYhpNz-lkp@intel.com/

smatch warnings:
drivers/pmdomain/rockchip/pm-domains.c:614 rockchip_pd_power() warn: inconsistent returns '&pmu->mutex'.

vim +614 drivers/pmdomain/rockchip/pm-domains.c

7c696693a4f54d drivers/soc/rockchip/pm_domains.c      Caesar Wang     2015-09-08  572  static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
7c696693a4f54d drivers/soc/rockchip/pm_domains.c      Caesar Wang     2015-09-08  573  {
d909072d0521a8 drivers/soc/rockchip/pm_domains.c      Jeffy Chen      2018-02-28  574  	struct rockchip_pmu *pmu = pd->pmu;
d909072d0521a8 drivers/soc/rockchip/pm_domains.c      Jeffy Chen      2018-02-28  575  	int ret;
7c696693a4f54d drivers/soc/rockchip/pm_domains.c      Caesar Wang     2015-09-08  576  
d909072d0521a8 drivers/soc/rockchip/pm_domains.c      Jeffy Chen      2018-02-28  577  	mutex_lock(&pmu->mutex);
7c696693a4f54d drivers/soc/rockchip/pm_domains.c      Caesar Wang     2015-09-08  578  
7c696693a4f54d drivers/soc/rockchip/pm_domains.c      Caesar Wang     2015-09-08  579  	if (rockchip_pmu_domain_is_on(pd) != power_on) {
d909072d0521a8 drivers/soc/rockchip/pm_domains.c      Jeffy Chen      2018-02-28  580  		ret = clk_bulk_enable(pd->num_clks, pd->clks);
d909072d0521a8 drivers/soc/rockchip/pm_domains.c      Jeffy Chen      2018-02-28  581  		if (ret < 0) {
d909072d0521a8 drivers/soc/rockchip/pm_domains.c      Jeffy Chen      2018-02-28  582  			dev_err(pmu->dev, "failed to enable clocks\n");
d909072d0521a8 drivers/soc/rockchip/pm_domains.c      Jeffy Chen      2018-02-28  583  			mutex_unlock(&pmu->mutex);
d909072d0521a8 drivers/soc/rockchip/pm_domains.c      Jeffy Chen      2018-02-28  584  			return ret;
d909072d0521a8 drivers/soc/rockchip/pm_domains.c      Jeffy Chen      2018-02-28  585  		}
7c696693a4f54d drivers/soc/rockchip/pm_domains.c      Caesar Wang     2015-09-08  586  
8b579881de295d drivers/pmdomain/rockchip/pm-domains.c Detlev Casanova 2024-08-29  587  		rockchip_pmu_ungate_clk(pd, true);
8b579881de295d drivers/pmdomain/rockchip/pm-domains.c Detlev Casanova 2024-08-29  588  
7c696693a4f54d drivers/soc/rockchip/pm_domains.c      Caesar Wang     2015-09-08  589  		if (!power_on) {
074c6a422d86ff drivers/soc/rockchip/pm_domains.c      Elaine Zhang    2016-04-14  590  			rockchip_pmu_save_qos(pd);
7c696693a4f54d drivers/soc/rockchip/pm_domains.c      Caesar Wang     2015-09-08  591  
7c696693a4f54d drivers/soc/rockchip/pm_domains.c      Caesar Wang     2015-09-08  592  			/* if powering down, idle request to NIU first */
a15817772c0ae6 drivers/pmdomain/rockchip/pm-domains.c Peter Geis      2024-12-10  593  			ret = rockchip_pmu_set_idle_request(pd, true);
a15817772c0ae6 drivers/pmdomain/rockchip/pm-domains.c Peter Geis      2024-12-10  594  			if (ret < 0)
a15817772c0ae6 drivers/pmdomain/rockchip/pm-domains.c Peter Geis      2024-12-10  595  				return ret;

mutex_unlock(&pmu->mutex);

7c696693a4f54d drivers/soc/rockchip/pm_domains.c      Caesar Wang     2015-09-08  596  		}
7c696693a4f54d drivers/soc/rockchip/pm_domains.c      Caesar Wang     2015-09-08  597  
7c696693a4f54d drivers/soc/rockchip/pm_domains.c      Caesar Wang     2015-09-08  598  		rockchip_do_pmu_set_power_domain(pd, power_on);
7c696693a4f54d drivers/soc/rockchip/pm_domains.c      Caesar Wang     2015-09-08  599  
7c696693a4f54d drivers/soc/rockchip/pm_domains.c      Caesar Wang     2015-09-08  600  		if (power_on) {
7c696693a4f54d drivers/soc/rockchip/pm_domains.c      Caesar Wang     2015-09-08  601  			/* if powering up, leave idle mode */
a15817772c0ae6 drivers/pmdomain/rockchip/pm-domains.c Peter Geis      2024-12-10  602  			ret = rockchip_pmu_set_idle_request(pd, false);
a15817772c0ae6 drivers/pmdomain/rockchip/pm-domains.c Peter Geis      2024-12-10  603  			if (ret < 0)
a15817772c0ae6 drivers/pmdomain/rockchip/pm-domains.c Peter Geis      2024-12-10  604  				return ret;

mutex_unlock(&pmu->mutex);

7c696693a4f54d drivers/soc/rockchip/pm_domains.c      Caesar Wang     2015-09-08  605  
074c6a422d86ff drivers/soc/rockchip/pm_domains.c      Elaine Zhang    2016-04-14  606  			rockchip_pmu_restore_qos(pd);
7c696693a4f54d drivers/soc/rockchip/pm_domains.c      Caesar Wang     2015-09-08  607  		}
7c696693a4f54d drivers/soc/rockchip/pm_domains.c      Caesar Wang     2015-09-08  608  
8b579881de295d drivers/pmdomain/rockchip/pm-domains.c Detlev Casanova 2024-08-29  609  		rockchip_pmu_ungate_clk(pd, false);
d909072d0521a8 drivers/soc/rockchip/pm_domains.c      Jeffy Chen      2018-02-28  610  		clk_bulk_disable(pd->num_clks, pd->clks);
7c696693a4f54d drivers/soc/rockchip/pm_domains.c      Caesar Wang     2015-09-08  611  	}
7c696693a4f54d drivers/soc/rockchip/pm_domains.c      Caesar Wang     2015-09-08  612  
d909072d0521a8 drivers/soc/rockchip/pm_domains.c      Jeffy Chen      2018-02-28  613  	mutex_unlock(&pmu->mutex);
7c696693a4f54d drivers/soc/rockchip/pm_domains.c      Caesar Wang     2015-09-08 @614  	return 0;
7c696693a4f54d drivers/soc/rockchip/pm_domains.c      Caesar Wang     2015-09-08  615  }
7c696693a4f54d drivers/soc/rockchip/pm_domains.c      Caesar Wang     2015-09-08  616  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 2/6] clk: rockchip: fix wrong clk_ref_usb3otg parent for rk3328

[Peter Geis]

Correct the clk_ref_usb3otg parent to fix clock control for the usb3
controller on rk3328. Verified against the rk3328 trm and usb3 clock tree
documentation.

Fixes: fe3511ad8a1c ("clk: rockchip: add clock controller for rk3328")
Signed-off-by: Peter Geis <pgwipeout@gmail.com>
---

 drivers/clk/rockchip/clk-rk3328.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/rockchip/clk-rk3328.c b/drivers/clk/rockchip/clk-rk3328.c
index 3bb87b27b662..cf60fcf2fa5c 100644
--- a/drivers/clk/rockchip/clk-rk3328.c
+++ b/drivers/clk/rockchip/clk-rk3328.c
@@ -201,7 +201,7 @@ PNAME(mux_aclk_peri_pre_p)	= { "cpll_peri",
 				    "gpll_peri",
 				    "hdmiphy_peri" };
 PNAME(mux_ref_usb3otg_src_p)	= { "xin24m",
-				    "clk_usb3otg_ref" };
+				    "clk_ref_usb3otg_src" };
 PNAME(mux_xin24m_32k_p)		= { "xin24m",
 				    "clk_rtc32k" };
 PNAME(mux_mac2io_src_p)		= { "clk_mac2io_src",
-- 
2.39.5

Re: [PATCH 2/6] clk: rockchip: fix wrong clk_ref_usb3otg parent for rk3328

[Dragan Simic]

Hello Peter,

On 2024-12-10 02:30, Peter Geis wrote:
> Correct the clk_ref_usb3otg parent to fix clock control for the usb3
> controller on rk3328. Verified against the rk3328 trm and usb3 clock 
> tree
> documentation.
> 
> Fixes: fe3511ad8a1c ("clk: rockchip: add clock controller for rk3328")
> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> ---
> 
>  drivers/clk/rockchip/clk-rk3328.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/rockchip/clk-rk3328.c
> b/drivers/clk/rockchip/clk-rk3328.c
> index 3bb87b27b662..cf60fcf2fa5c 100644
> --- a/drivers/clk/rockchip/clk-rk3328.c
> +++ b/drivers/clk/rockchip/clk-rk3328.c
> @@ -201,7 +201,7 @@ PNAME(mux_aclk_peri_pre_p)	= { "cpll_peri",
>  				    "gpll_peri",
>  				    "hdmiphy_peri" };
>  PNAME(mux_ref_usb3otg_src_p)	= { "xin24m",
> -				    "clk_usb3otg_ref" };
> +				    "clk_ref_usb3otg_src" };
>  PNAME(mux_xin24m_32k_p)		= { "xin24m",
>  				    "clk_rtc32k" };
>  PNAME(mux_mac2io_src_p)		= { "clk_mac2io_src",

Sorry, but I was unable to verify this in the part 1 of the
RK3328 TRM, in both versions 1.1 and 1.2, which is all I have
when it comes to the RK3328 TRM.  Is that maybe described in
the part 2, which I've been unable to locate for years?

Moreover, the downstream kernel source from Rockchip does it
the way [1] it's currently done in the mainline kernel, which
makes me confused a bit?  Could you, please, provide more
details about the two references you mentioned in the patch
description, or maybe even you could provide the links to
those two references?

[1] 
https://raw.githubusercontent.com/rockchip-linux/kernel/refs/heads/develop-4.4/drivers/clk/rockchip/clk-rk3328.c

Re: [PATCH 2/6] clk: rockchip: fix wrong clk_ref_usb3otg parent for rk3328

[Peter Geis]

On Tue, Dec 10, 2024 at 4:44 AM Dragan Simic <dsimic@manjaro.org> wrote:
>
> Hello Peter,
>
> On 2024-12-10 02:30, Peter Geis wrote:
> > Correct the clk_ref_usb3otg parent to fix clock control for the usb3
> > controller on rk3328. Verified against the rk3328 trm and usb3 clock
> > tree
> > documentation.
> >
> > Fixes: fe3511ad8a1c ("clk: rockchip: add clock controller for rk3328")
> > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> > ---
> >
> >  drivers/clk/rockchip/clk-rk3328.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/rockchip/clk-rk3328.c
> > b/drivers/clk/rockchip/clk-rk3328.c
> > index 3bb87b27b662..cf60fcf2fa5c 100644
> > --- a/drivers/clk/rockchip/clk-rk3328.c
> > +++ b/drivers/clk/rockchip/clk-rk3328.c
> > @@ -201,7 +201,7 @@ PNAME(mux_aclk_peri_pre_p)        = { "cpll_peri",
> >                                   "gpll_peri",
> >                                   "hdmiphy_peri" };
> >  PNAME(mux_ref_usb3otg_src_p) = { "xin24m",
> > -                                 "clk_usb3otg_ref" };
> > +                                 "clk_ref_usb3otg_src" };
> >  PNAME(mux_xin24m_32k_p)              = { "xin24m",
> >                                   "clk_rtc32k" };
> >  PNAME(mux_mac2io_src_p)              = { "clk_mac2io_src",
>
> Sorry, but I was unable to verify this in the part 1 of the
> RK3328 TRM, in both versions 1.1 and 1.2, which is all I have
> when it comes to the RK3328 TRM.  Is that maybe described in
> the part 2, which I've been unable to locate for years?
>
> Moreover, the downstream kernel source from Rockchip does it
> the way [1] it's currently done in the mainline kernel, which
> makes me confused a bit?  Could you, please, provide more
> details about the two references you mentioned in the patch
> description, or maybe even you could provide the links to
> those two references?
>
> [1]
> https://raw.githubusercontent.com/rockchip-linux/kernel/refs/heads/develop-4.4/drivers/clk/rockchip/clk-rk3328.c

It is unfortunate the TRM doesn't include the clock maps, because they
are extremely helpful when one can acquire them. It also doesn't help
that the TRM register definition only referred to this as "pll". I was
sent specifically the usb3 phy clock map for my work on the driver,
which had the location of each switch and divider along with the
register and bit that controlled it. That combined with the TRM
register map allowed me to find this error.

Thanks!
Peter

Re: [PATCH 2/6] clk: rockchip: fix wrong clk_ref_usb3otg parent for rk3328

[Dragan Simic]

Hello Peter,

On 2024-12-10 14:27, Peter Geis wrote:
> On Tue, Dec 10, 2024 at 4:44 AM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> On 2024-12-10 02:30, Peter Geis wrote:
>> > Correct the clk_ref_usb3otg parent to fix clock control for the usb3
>> > controller on rk3328. Verified against the rk3328 trm and usb3 clock
>> > tree
>> > documentation.
>> >
>> > Fixes: fe3511ad8a1c ("clk: rockchip: add clock controller for rk3328")
>> > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
>> > ---
>> >
>> >  drivers/clk/rockchip/clk-rk3328.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/clk/rockchip/clk-rk3328.c
>> > b/drivers/clk/rockchip/clk-rk3328.c
>> > index 3bb87b27b662..cf60fcf2fa5c 100644
>> > --- a/drivers/clk/rockchip/clk-rk3328.c
>> > +++ b/drivers/clk/rockchip/clk-rk3328.c
>> > @@ -201,7 +201,7 @@ PNAME(mux_aclk_peri_pre_p)        = { "cpll_peri",
>> >                                   "gpll_peri",
>> >                                   "hdmiphy_peri" };
>> >  PNAME(mux_ref_usb3otg_src_p) = { "xin24m",
>> > -                                 "clk_usb3otg_ref" };
>> > +                                 "clk_ref_usb3otg_src" };
>> >  PNAME(mux_xin24m_32k_p)              = { "xin24m",
>> >                                   "clk_rtc32k" };
>> >  PNAME(mux_mac2io_src_p)              = { "clk_mac2io_src",
>> 
>> Sorry, but I was unable to verify this in the part 1 of the
>> RK3328 TRM, in both versions 1.1 and 1.2, which is all I have
>> when it comes to the RK3328 TRM.  Is that maybe described in
>> the part 2, which I've been unable to locate for years?
>> 
>> Moreover, the downstream kernel source from Rockchip does it
>> the way [1] it's currently done in the mainline kernel, which
>> makes me confused a bit?  Could you, please, provide more
>> details about the two references you mentioned in the patch
>> description, or maybe even you could provide the links to
>> those two references?
>> 
>> [1] 
>> https://raw.githubusercontent.com/rockchip-linux/kernel/refs/heads/develop-4.4/drivers/clk/rockchip/clk-rk3328.c
> 
> It is unfortunate the TRM doesn't include the clock maps, because they
> are extremely helpful when one can acquire them. It also doesn't help
> that the TRM register definition only referred to this as "pll". I was
> sent specifically the usb3 phy clock map for my work on the driver,
> which had the location of each switch and divider along with the
> register and bit that controlled it. That combined with the TRM
> register map allowed me to find this error.

I see, thanks for the clarification.  I'd assume that you aren't allowed
to share the additional documentation you've got, which is unfortunate,
but it is what it is.  We should be happy that at least you got it, and
were able to put it into good use.

Thanks for fixing this!

Re: [PATCH 2/6] clk: rockchip: fix wrong clk_ref_usb3otg parent for rk3328

[Jonas Karlman]

On 2024-12-10 14:27, Peter Geis wrote:
> On Tue, Dec 10, 2024 at 4:44 AM Dragan Simic <dsimic@manjaro.org> wrote:
>>
>> Hello Peter,
>>
>> On 2024-12-10 02:30, Peter Geis wrote:
>>> Correct the clk_ref_usb3otg parent to fix clock control for the usb3
>>> controller on rk3328. Verified against the rk3328 trm and usb3 clock
>>> tree
>>> documentation.
>>>
>>> Fixes: fe3511ad8a1c ("clk: rockchip: add clock controller for rk3328")
>>> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
>>> ---
>>>
>>>  drivers/clk/rockchip/clk-rk3328.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clk/rockchip/clk-rk3328.c
>>> b/drivers/clk/rockchip/clk-rk3328.c
>>> index 3bb87b27b662..cf60fcf2fa5c 100644
>>> --- a/drivers/clk/rockchip/clk-rk3328.c
>>> +++ b/drivers/clk/rockchip/clk-rk3328.c
>>> @@ -201,7 +201,7 @@ PNAME(mux_aclk_peri_pre_p)        = { "cpll_peri",
>>>                                   "gpll_peri",
>>>                                   "hdmiphy_peri" };
>>>  PNAME(mux_ref_usb3otg_src_p) = { "xin24m",
>>> -                                 "clk_usb3otg_ref" };
>>> +                                 "clk_ref_usb3otg_src" };
>>>  PNAME(mux_xin24m_32k_p)              = { "xin24m",
>>>                                   "clk_rtc32k" };
>>>  PNAME(mux_mac2io_src_p)              = { "clk_mac2io_src",
>>
>> Sorry, but I was unable to verify this in the part 1 of the
>> RK3328 TRM, in both versions 1.1 and 1.2, which is all I have
>> when it comes to the RK3328 TRM.  Is that maybe described in
>> the part 2, which I've been unable to locate for years?
>>
>> Moreover, the downstream kernel source from Rockchip does it
>> the way [1] it's currently done in the mainline kernel, which
>> makes me confused a bit?  Could you, please, provide more
>> details about the two references you mentioned in the patch
>> description, or maybe even you could provide the links to
>> those two references?
>>
>> [1]
>> https://raw.githubusercontent.com/rockchip-linux/kernel/refs/heads/develop-4.4/drivers/clk/rockchip/clk-rk3328.c
> 
> It is unfortunate the TRM doesn't include the clock maps, because they
> are extremely helpful when one can acquire them. It also doesn't help
> that the TRM register definition only referred to this as "pll". I was
> sent specifically the usb3 phy clock map for my work on the driver,
> which had the location of each switch and divider along with the
> register and bit that controlled it. That combined with the TRM
> register map allowed me to find this error.

I can also confirm that the changes in this patch matches Fig. 3-8
RK3228H Clock Architecture Diagram 7 for the USB3OTG block.

                                    XIN24M -\
                                             S45_8 - ref_clk_usb3otg
S45_7 (2PLL) / G4_9 / S45_0 (DivFree 1~64) -/

Regards,
Jonas

> 
> Thanks!
> Peter
> 

[PATCH 3/6] arm64: dts: rockchip: remove ethernet alias from rk3328-roc

[Peter Geis]

Remove the ethernet alias added back in during the rk3328-roc dtsi
conversion.

Fixes: f3c6526d6fb2 ("arm64: dts: rockchip: Convert dts files used as parents to dtsi files")
Signed-off-by: Peter Geis <pgwipeout@gmail.com>
---

 arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
index b5bd5e7d5748..f782c8220dd3 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
@@ -9,7 +9,6 @@
 
 / {
 	aliases {
-		ethernet0 = &gmac2io;
 		mmc0 = &sdmmc;
 		mmc1 = &emmc;
 	};
-- 
2.39.5

Re: [PATCH 3/6] arm64: dts: rockchip: remove ethernet alias from rk3328-roc

[Dragan Simic]

Hello Peter,

On 2024-12-10 02:30, Peter Geis wrote:
> Remove the ethernet alias added back in during the rk3328-roc dtsi
> conversion.

I just checked again the dtsi parent conversion I performed in
the commit f3c6526d6fb2 ("arm64: dts: rockchip: Convert dts files
used as parents to dtsi files"), and both rk3328-roc-cc.dts and
rk3328-roc-pc.dts had the ethernet0 alias defined before the
conversion.  Thus, the alias wasn't added back by mistake during
the conversion, it was there before.

Moreover, I don't see why would we want to delete the ethernet0
alias(es) in the first place?  It's usual for Rockchip board dts
files to have ethernetX aliases defined, and both ROC-RK3328-CC
and ROC-RK3328-PC have their gmac2io DT nodes enabled, and the
boards have wired Ethernet ports, so they should also have the
ethernet0 alias(es) defined.

Am I missing something?

> Fixes: f3c6526d6fb2 ("arm64: dts: rockchip: Convert dts files used as
> parents to dtsi files")
> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> ---
> 
>  arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> index b5bd5e7d5748..f782c8220dd3 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> @@ -9,7 +9,6 @@
> 
>  / {
>  	aliases {
> -		ethernet0 = &gmac2io;
>  		mmc0 = &sdmmc;
>  		mmc1 = &emmc;
>  	};

Re: [PATCH 3/6] arm64: dts: rockchip: remove ethernet alias from rk3328-roc

[Peter Geis]

On Tue, Dec 10, 2024 at 3:01 AM Dragan Simic <dsimic@manjaro.org> wrote:
>
> Hello Peter,
>
> On 2024-12-10 02:30, Peter Geis wrote:
> > Remove the ethernet alias added back in during the rk3328-roc dtsi
> > conversion.
>
> I just checked again the dtsi parent conversion I performed in
> the commit f3c6526d6fb2 ("arm64: dts: rockchip: Convert dts files
> used as parents to dtsi files"), and both rk3328-roc-cc.dts and
> rk3328-roc-pc.dts had the ethernet0 alias defined before the
> conversion.  Thus, the alias wasn't added back by mistake during
> the conversion, it was there before.
>
> Moreover, I don't see why would we want to delete the ethernet0
> alias(es) in the first place?  It's usual for Rockchip board dts
> files to have ethernetX aliases defined, and both ROC-RK3328-CC
> and ROC-RK3328-PC have their gmac2io DT nodes enabled, and the
> boards have wired Ethernet ports, so they should also have the
> ethernet0 alias(es) defined.
>
> Am I missing something?

You aren't missing something, I just misunderstood what was happening
with your patch. I can safely drop this.

Thanks!
Peter

>
> > Fixes: f3c6526d6fb2 ("arm64: dts: rockchip: Convert dts files used as
> > parents to dtsi files")
> > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> > ---
> >
> >  arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> > b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> > index b5bd5e7d5748..f782c8220dd3 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> > @@ -9,7 +9,6 @@
> >
> >  / {
> >       aliases {
> > -             ethernet0 = &gmac2io;
> >               mmc0 = &sdmmc;
> >               mmc1 = &emmc;
> >       };

[PATCH 4/6] arm64: dts: rockchip: add hevc power domain clock to rk3328

[Peter Geis]

There is a race condition at startup between disabling power domains not
used and disabling clocks not used on the rk3328. When the clocks are
disabled first, the hevc power domain fails to shut off leading to a
splat of failures. Add the hevc core clock to the rk3328 power domain
node to prevent this condition.

rcu: INFO: rcu_sched detected expedited stalls on CPUs/tasks: { 3-.... }
1087 jiffies s: 89 root: 0x8/.
rcu: blocking rcu_node structures (internal RCU debug):
Sending NMI from CPU 0 to CPUs 3:
NMI backtrace for cpu 3
CPU: 3 UID: 0 PID: 86 Comm: kworker/3:3 Not tainted 6.12.0-rc5+ #53
Hardware name: Firefly ROC-RK3328-CC (DT)
Workqueue: pm genpd_power_off_work_fn
pstate: 20400005 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : regmap_unlock_spinlock+0x18/0x30
lr : regmap_read+0x60/0x88
sp : ffff800081123c00
x29: ffff800081123c00 x28: ffff2fa4c62cad80 x27: 0000000000000000
x26: ffffd74e6e660eb8 x25: ffff2fa4c62cae00 x24: 0000000000000040
x23: ffffd74e6d2f3ab8 x22: 0000000000000001 x21: ffff800081123c74
x20: 0000000000000000 x19: ffff2fa4c0412000 x18: 0000000000000000
x17: 77202c31203d2065 x16: 6c6469203a72656c x15: 6c6f72746e6f632d
x14: 7265776f703a6e6f x13: 2063766568206e69 x12: 616d6f64202c3431
x11: 347830206f742030 x10: 3430303034783020 x9 : ffffd74e6c7369e0
x8 : 3030316666206e69 x7 : 205d383738353733 x6 : 332e31202020205b
x5 : ffffd74e6c73fc88 x4 : ffffd74e6c73fcd4 x3 : ffffd74e6c740b40
x2 : ffff800080015484 x1 : 0000000000000000 x0 : ffff2fa4c0412000
Call trace:
regmap_unlock_spinlock+0x18/0x30
rockchip_pmu_set_idle_request+0xac/0x2c0
rockchip_pd_power+0x144/0x5f8
rockchip_pd_power_off+0x1c/0x30
_genpd_power_off+0x9c/0x180
genpd_power_off.part.0.isra.0+0x130/0x2a8
genpd_power_off_work_fn+0x6c/0x98
process_one_work+0x170/0x3f0
worker_thread+0x290/0x4a8
kthread+0xec/0xf8
ret_from_fork+0x10/0x20
rockchip-pm-domain ff100000.syscon:power-controller: failed to get ack on
domain 'hevc', val=0x88220

Fixes: 52e02d377a72 ("arm64: dts: rockchip: add core dtsi file for RK3328 SoCs")
Signed-off-by: Peter Geis <pgwipeout@gmail.com>
---

 arch/arm64/boot/dts/rockchip/rk3328.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
index 0597de415fe0..7d992c3c01ce 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
@@ -333,6 +333,7 @@ power: power-controller {
 
 			power-domain@RK3328_PD_HEVC {
 				reg = <RK3328_PD_HEVC>;
+				clocks = <&cru SCLK_VENC_CORE>;
 				#power-domain-cells = <0>;
 			};
 			power-domain@RK3328_PD_VIDEO {
-- 
2.39.5

Re: [PATCH 4/6] arm64: dts: rockchip: add hevc power domain clock to rk3328

[Dragan Simic]

Hello Peter,

On 2024-12-10 02:30, Peter Geis wrote:
> There is a race condition at startup between disabling power domains 
> not
> used and disabling clocks not used on the rk3328. When the clocks are
> disabled first, the hevc power domain fails to shut off leading to a
> splat of failures. Add the hevc core clock to the rk3328 power domain
> node to prevent this condition.
> 
> rcu: INFO: rcu_sched detected expedited stalls on CPUs/tasks: { 3-.... 
> }
> 1087 jiffies s: 89 root: 0x8/.
> rcu: blocking rcu_node structures (internal RCU debug):
> Sending NMI from CPU 0 to CPUs 3:
> NMI backtrace for cpu 3
> CPU: 3 UID: 0 PID: 86 Comm: kworker/3:3 Not tainted 6.12.0-rc5+ #53
> Hardware name: Firefly ROC-RK3328-CC (DT)
> Workqueue: pm genpd_power_off_work_fn
> pstate: 20400005 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : regmap_unlock_spinlock+0x18/0x30
> lr : regmap_read+0x60/0x88
> sp : ffff800081123c00
> x29: ffff800081123c00 x28: ffff2fa4c62cad80 x27: 0000000000000000
> x26: ffffd74e6e660eb8 x25: ffff2fa4c62cae00 x24: 0000000000000040
> x23: ffffd74e6d2f3ab8 x22: 0000000000000001 x21: ffff800081123c74
> x20: 0000000000000000 x19: ffff2fa4c0412000 x18: 0000000000000000
> x17: 77202c31203d2065 x16: 6c6469203a72656c x15: 6c6f72746e6f632d
> x14: 7265776f703a6e6f x13: 2063766568206e69 x12: 616d6f64202c3431
> x11: 347830206f742030 x10: 3430303034783020 x9 : ffffd74e6c7369e0
> x8 : 3030316666206e69 x7 : 205d383738353733 x6 : 332e31202020205b
> x5 : ffffd74e6c73fc88 x4 : ffffd74e6c73fcd4 x3 : ffffd74e6c740b40
> x2 : ffff800080015484 x1 : 0000000000000000 x0 : ffff2fa4c0412000
> Call trace:
> regmap_unlock_spinlock+0x18/0x30
> rockchip_pmu_set_idle_request+0xac/0x2c0
> rockchip_pd_power+0x144/0x5f8
> rockchip_pd_power_off+0x1c/0x30
> _genpd_power_off+0x9c/0x180
> genpd_power_off.part.0.isra.0+0x130/0x2a8
> genpd_power_off_work_fn+0x6c/0x98
> process_one_work+0x170/0x3f0
> worker_thread+0x290/0x4a8
> kthread+0xec/0xf8
> ret_from_fork+0x10/0x20
> rockchip-pm-domain ff100000.syscon:power-controller: failed to get ack 
> on
> domain 'hevc', val=0x88220
> 
> Fixes: 52e02d377a72 ("arm64: dts: rockchip: add core dtsi file for 
> RK3328 SoCs")
> Signed-off-by: Peter Geis <pgwipeout@gmail.com>

While I was unable to formally verify this clock assignment,
i.e. by using the RK3328 TRM or the downstream kernel source
from Rockchip, it makes perfect sense to me.  Thanks for the
patch, and please feel free to include:

Reviewed-by: Dragan Simic <dsimic@manjaro.org>

> ---
> 
>  arch/arm64/boot/dts/rockchip/rk3328.dtsi | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> index 0597de415fe0..7d992c3c01ce 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> @@ -333,6 +333,7 @@ power: power-controller {
> 
>  			power-domain@RK3328_PD_HEVC {
>  				reg = <RK3328_PD_HEVC>;
> +				clocks = <&cru SCLK_VENC_CORE>;
>  				#power-domain-cells = <0>;
>  			};
>  			power-domain@RK3328_PD_VIDEO {

Re: [PATCH 4/6] arm64: dts: rockchip: add hevc power domain clock to rk3328

[Peter Geis]

On Tue, Dec 10, 2024 at 5:04 AM Dragan Simic <dsimic@manjaro.org> wrote:
>
> Hello Peter,
>
> On 2024-12-10 02:30, Peter Geis wrote:
> > There is a race condition at startup between disabling power domains
> > not
> > used and disabling clocks not used on the rk3328. When the clocks are
> > disabled first, the hevc power domain fails to shut off leading to a
> > splat of failures. Add the hevc core clock to the rk3328 power domain
> > node to prevent this condition.
> >
> > rcu: INFO: rcu_sched detected expedited stalls on CPUs/tasks: { 3-....
> > }
> > 1087 jiffies s: 89 root: 0x8/.
> > rcu: blocking rcu_node structures (internal RCU debug):
> > Sending NMI from CPU 0 to CPUs 3:
> > NMI backtrace for cpu 3
> > CPU: 3 UID: 0 PID: 86 Comm: kworker/3:3 Not tainted 6.12.0-rc5+ #53
> > Hardware name: Firefly ROC-RK3328-CC (DT)
> > Workqueue: pm genpd_power_off_work_fn
> > pstate: 20400005 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > pc : regmap_unlock_spinlock+0x18/0x30
> > lr : regmap_read+0x60/0x88
> > sp : ffff800081123c00
> > x29: ffff800081123c00 x28: ffff2fa4c62cad80 x27: 0000000000000000
> > x26: ffffd74e6e660eb8 x25: ffff2fa4c62cae00 x24: 0000000000000040
> > x23: ffffd74e6d2f3ab8 x22: 0000000000000001 x21: ffff800081123c74
> > x20: 0000000000000000 x19: ffff2fa4c0412000 x18: 0000000000000000
> > x17: 77202c31203d2065 x16: 6c6469203a72656c x15: 6c6f72746e6f632d
> > x14: 7265776f703a6e6f x13: 2063766568206e69 x12: 616d6f64202c3431
> > x11: 347830206f742030 x10: 3430303034783020 x9 : ffffd74e6c7369e0
> > x8 : 3030316666206e69 x7 : 205d383738353733 x6 : 332e31202020205b
> > x5 : ffffd74e6c73fc88 x4 : ffffd74e6c73fcd4 x3 : ffffd74e6c740b40
> > x2 : ffff800080015484 x1 : 0000000000000000 x0 : ffff2fa4c0412000
> > Call trace:
> > regmap_unlock_spinlock+0x18/0x30
> > rockchip_pmu_set_idle_request+0xac/0x2c0
> > rockchip_pd_power+0x144/0x5f8
> > rockchip_pd_power_off+0x1c/0x30
> > _genpd_power_off+0x9c/0x180
> > genpd_power_off.part.0.isra.0+0x130/0x2a8
> > genpd_power_off_work_fn+0x6c/0x98
> > process_one_work+0x170/0x3f0
> > worker_thread+0x290/0x4a8
> > kthread+0xec/0xf8
> > ret_from_fork+0x10/0x20
> > rockchip-pm-domain ff100000.syscon:power-controller: failed to get ack
> > on
> > domain 'hevc', val=0x88220
> >
> > Fixes: 52e02d377a72 ("arm64: dts: rockchip: add core dtsi file for
> > RK3328 SoCs")
> > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
>
> While I was unable to formally verify this clock assignment,
> i.e. by using the RK3328 TRM or the downstream kernel source
> from Rockchip, it makes perfect sense to me.  Thanks for the
> patch, and please feel free to include:
>
> Reviewed-by: Dragan Simic <dsimic@manjaro.org>

It is unfortunate the TRM doesn't include the clock maps, because they
are extremely helpful when one can acquire them. It also doesn't help
that the TRM register definition only referred to this as "pll". I was
sent specifically the usb3 phy clock map for my work on the driver,
which had the location of each switch and divider along with the
register and bit that controlled it. That combined with the TRM
register map allowed me to find this error.

Thanks!
Peter

>
> > ---
> >
> >  arch/arm64/boot/dts/rockchip/rk3328.dtsi | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> > b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> > index 0597de415fe0..7d992c3c01ce 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> > @@ -333,6 +333,7 @@ power: power-controller {
> >
> >                       power-domain@RK3328_PD_HEVC {
> >                               reg = <RK3328_PD_HEVC>;
> > +                             clocks = <&cru SCLK_VENC_CORE>;
> >                               #power-domain-cells = <0>;
> >                       };
> >                       power-domain@RK3328_PD_VIDEO {

Re: [PATCH 4/6] arm64: dts: rockchip: add hevc power domain clock to rk3328

[Peter Geis]

On Tue, Dec 10, 2024 at 8:13 AM Peter Geis <pgwipeout@gmail.com> wrote:
>
> On Tue, Dec 10, 2024 at 5:04 AM Dragan Simic <dsimic@manjaro.org> wrote:
> >
> > Hello Peter,
> >
> > On 2024-12-10 02:30, Peter Geis wrote:
> > > There is a race condition at startup between disabling power domains
> > > not
> > > used and disabling clocks not used on the rk3328. When the clocks are
> > > disabled first, the hevc power domain fails to shut off leading to a
> > > splat of failures. Add the hevc core clock to the rk3328 power domain
> > > node to prevent this condition.
> > >
> > > rcu: INFO: rcu_sched detected expedited stalls on CPUs/tasks: { 3-....
> > > }
> > > 1087 jiffies s: 89 root: 0x8/.
> > > rcu: blocking rcu_node structures (internal RCU debug):
> > > Sending NMI from CPU 0 to CPUs 3:
> > > NMI backtrace for cpu 3
> > > CPU: 3 UID: 0 PID: 86 Comm: kworker/3:3 Not tainted 6.12.0-rc5+ #53
> > > Hardware name: Firefly ROC-RK3328-CC (DT)
> > > Workqueue: pm genpd_power_off_work_fn
> > > pstate: 20400005 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > > pc : regmap_unlock_spinlock+0x18/0x30
> > > lr : regmap_read+0x60/0x88
> > > sp : ffff800081123c00
> > > x29: ffff800081123c00 x28: ffff2fa4c62cad80 x27: 0000000000000000
> > > x26: ffffd74e6e660eb8 x25: ffff2fa4c62cae00 x24: 0000000000000040
> > > x23: ffffd74e6d2f3ab8 x22: 0000000000000001 x21: ffff800081123c74
> > > x20: 0000000000000000 x19: ffff2fa4c0412000 x18: 0000000000000000
> > > x17: 77202c31203d2065 x16: 6c6469203a72656c x15: 6c6f72746e6f632d
> > > x14: 7265776f703a6e6f x13: 2063766568206e69 x12: 616d6f64202c3431
> > > x11: 347830206f742030 x10: 3430303034783020 x9 : ffffd74e6c7369e0
> > > x8 : 3030316666206e69 x7 : 205d383738353733 x6 : 332e31202020205b
> > > x5 : ffffd74e6c73fc88 x4 : ffffd74e6c73fcd4 x3 : ffffd74e6c740b40
> > > x2 : ffff800080015484 x1 : 0000000000000000 x0 : ffff2fa4c0412000
> > > Call trace:
> > > regmap_unlock_spinlock+0x18/0x30
> > > rockchip_pmu_set_idle_request+0xac/0x2c0
> > > rockchip_pd_power+0x144/0x5f8
> > > rockchip_pd_power_off+0x1c/0x30
> > > _genpd_power_off+0x9c/0x180
> > > genpd_power_off.part.0.isra.0+0x130/0x2a8
> > > genpd_power_off_work_fn+0x6c/0x98
> > > process_one_work+0x170/0x3f0
> > > worker_thread+0x290/0x4a8
> > > kthread+0xec/0xf8
> > > ret_from_fork+0x10/0x20
> > > rockchip-pm-domain ff100000.syscon:power-controller: failed to get ack
> > > on
> > > domain 'hevc', val=0x88220
> > >
> > > Fixes: 52e02d377a72 ("arm64: dts: rockchip: add core dtsi file for
> > > RK3328 SoCs")
> > > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> >
> > While I was unable to formally verify this clock assignment,
> > i.e. by using the RK3328 TRM or the downstream kernel source
> > from Rockchip, it makes perfect sense to me.  Thanks for the
> > patch, and please feel free to include:
> >
> > Reviewed-by: Dragan Simic <dsimic@manjaro.org>
>
> It is unfortunate the TRM doesn't include the clock maps, because they
> are extremely helpful when one can acquire them. It also doesn't help
> that the TRM register definition only referred to this as "pll". I was
> sent specifically the usb3 phy clock map for my work on the driver,
> which had the location of each switch and divider along with the
> register and bit that controlled it. That combined with the TRM
> register map allowed me to find this error.
>
> Thanks!
> Peter

Apologies, that's the wrong response for this one.
This patch was the result of educated guess combined with testing. I
grabbed all of the clocks that looked like they could affect things,
then tested them one at a time until I isolated them to this clock. It
lives alone with cpll as the parent and has no children according to
the clock summary. (Though the writeup i mistakenly included above
proves the clock map isn't always accurate).

Thanks,
Peter
>
> >
> > > ---
> > >
> > >  arch/arm64/boot/dts/rockchip/rk3328.dtsi | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> > > b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> > > index 0597de415fe0..7d992c3c01ce 100644
> > > --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> > > +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> > > @@ -333,6 +333,7 @@ power: power-controller {
> > >
> > >                       power-domain@RK3328_PD_HEVC {
> > >                               reg = <RK3328_PD_HEVC>;
> > > +                             clocks = <&cru SCLK_VENC_CORE>;
> > >                               #power-domain-cells = <0>;
> > >                       };
> > >                       power-domain@RK3328_PD_VIDEO {

Re: [PATCH 4/6] arm64: dts: rockchip: add hevc power domain clock to rk3328

[Dragan Simic]

Hello Peter,

On 2024-12-10 14:23, Peter Geis wrote:
> On Tue, Dec 10, 2024 at 8:13 AM Peter Geis <pgwipeout@gmail.com> wrote:
>> On Tue, Dec 10, 2024 at 5:04 AM Dragan Simic <dsimic@manjaro.org> 
>> wrote:
>> > On 2024-12-10 02:30, Peter Geis wrote:
>> > > There is a race condition at startup between disabling power domains
>> > > not
>> > > used and disabling clocks not used on the rk3328. When the clocks are
>> > > disabled first, the hevc power domain fails to shut off leading to a
>> > > splat of failures. Add the hevc core clock to the rk3328 power domain
>> > > node to prevent this condition.
>> > >
>> > > rcu: INFO: rcu_sched detected expedited stalls on CPUs/tasks: { 3-....
>> > > }
>> > > 1087 jiffies s: 89 root: 0x8/.
>> > > rcu: blocking rcu_node structures (internal RCU debug):
>> > > Sending NMI from CPU 0 to CPUs 3:
>> > > NMI backtrace for cpu 3
>> > > CPU: 3 UID: 0 PID: 86 Comm: kworker/3:3 Not tainted 6.12.0-rc5+ #53
>> > > Hardware name: Firefly ROC-RK3328-CC (DT)
>> > > Workqueue: pm genpd_power_off_work_fn
>> > > pstate: 20400005 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>> > > pc : regmap_unlock_spinlock+0x18/0x30
>> > > lr : regmap_read+0x60/0x88
>> > > sp : ffff800081123c00
>> > > x29: ffff800081123c00 x28: ffff2fa4c62cad80 x27: 0000000000000000
>> > > x26: ffffd74e6e660eb8 x25: ffff2fa4c62cae00 x24: 0000000000000040
>> > > x23: ffffd74e6d2f3ab8 x22: 0000000000000001 x21: ffff800081123c74
>> > > x20: 0000000000000000 x19: ffff2fa4c0412000 x18: 0000000000000000
>> > > x17: 77202c31203d2065 x16: 6c6469203a72656c x15: 6c6f72746e6f632d
>> > > x14: 7265776f703a6e6f x13: 2063766568206e69 x12: 616d6f64202c3431
>> > > x11: 347830206f742030 x10: 3430303034783020 x9 : ffffd74e6c7369e0
>> > > x8 : 3030316666206e69 x7 : 205d383738353733 x6 : 332e31202020205b
>> > > x5 : ffffd74e6c73fc88 x4 : ffffd74e6c73fcd4 x3 : ffffd74e6c740b40
>> > > x2 : ffff800080015484 x1 : 0000000000000000 x0 : ffff2fa4c0412000
>> > > Call trace:
>> > > regmap_unlock_spinlock+0x18/0x30
>> > > rockchip_pmu_set_idle_request+0xac/0x2c0
>> > > rockchip_pd_power+0x144/0x5f8
>> > > rockchip_pd_power_off+0x1c/0x30
>> > > _genpd_power_off+0x9c/0x180
>> > > genpd_power_off.part.0.isra.0+0x130/0x2a8
>> > > genpd_power_off_work_fn+0x6c/0x98
>> > > process_one_work+0x170/0x3f0
>> > > worker_thread+0x290/0x4a8
>> > > kthread+0xec/0xf8
>> > > ret_from_fork+0x10/0x20
>> > > rockchip-pm-domain ff100000.syscon:power-controller: failed to get ack
>> > > on
>> > > domain 'hevc', val=0x88220
>> > >
>> > > Fixes: 52e02d377a72 ("arm64: dts: rockchip: add core dtsi file for
>> > > RK3328 SoCs")
>> > > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
>> >
>> > While I was unable to formally verify this clock assignment,
>> > i.e. by using the RK3328 TRM or the downstream kernel source
>> > from Rockchip, it makes perfect sense to me.  Thanks for the
>> > patch, and please feel free to include:
>> >
>> > Reviewed-by: Dragan Simic <dsimic@manjaro.org>
>> 
>> It is unfortunate the TRM doesn't include the clock maps, because they
>> are extremely helpful when one can acquire them. It also doesn't help
>> that the TRM register definition only referred to this as "pll". I was
>> sent specifically the usb3 phy clock map for my work on the driver,
>> which had the location of each switch and divider along with the
>> register and bit that controlled it. That combined with the TRM
>> register map allowed me to find this error.
> 
> Apologies, that's the wrong response for this one.

No worries.

> This patch was the result of educated guess combined with testing. I
> grabbed all of the clocks that looked like they could affect things,
> then tested them one at a time until I isolated them to this clock. It
> lives alone with cpll as the parent and has no children according to
> the clock summary. (Though the writeup i mistakenly included above
> proves the clock map isn't always accurate).

I see, thanks for all your work on this patch!  It surely took quite
a lot of time to perform all the testing.

>> > > ---
>> > >
>> > >  arch/arm64/boot/dts/rockchip/rk3328.dtsi | 1 +
>> > >  1 file changed, 1 insertion(+)
>> > >
>> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>> > > b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>> > > index 0597de415fe0..7d992c3c01ce 100644
>> > > --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>> > > +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>> > > @@ -333,6 +333,7 @@ power: power-controller {
>> > >
>> > >                       power-domain@RK3328_PD_HEVC {
>> > >                               reg = <RK3328_PD_HEVC>;
>> > > +                             clocks = <&cru SCLK_VENC_CORE>;
>> > >                               #power-domain-cells = <0>;
>> > >                       };
>> > >                       power-domain@RK3328_PD_VIDEO {

Re: [PATCH 4/6] arm64: dts: rockchip: add hevc power domain clock to rk3328

[Jonas Karlman]

Hi Peter,

On 2024-12-10 14:53, Dragan Simic wrote:
> Hello Peter,
> 
> On 2024-12-10 14:23, Peter Geis wrote:
>> On Tue, Dec 10, 2024 at 8:13 AM Peter Geis <pgwipeout@gmail.com> wrote:
>>> On Tue, Dec 10, 2024 at 5:04 AM Dragan Simic <dsimic@manjaro.org> 
>>> wrote:
>>>> On 2024-12-10 02:30, Peter Geis wrote:
>>>>> There is a race condition at startup between disabling power domains
>>>>> not
>>>>> used and disabling clocks not used on the rk3328. When the clocks are
>>>>> disabled first, the hevc power domain fails to shut off leading to a
>>>>> splat of failures. Add the hevc core clock to the rk3328 power domain
>>>>> node to prevent this condition.
>>>>>
>>>>> rcu: INFO: rcu_sched detected expedited stalls on CPUs/tasks: { 3-....
>>>>> }
>>>>> 1087 jiffies s: 89 root: 0x8/.
>>>>> rcu: blocking rcu_node structures (internal RCU debug):
>>>>> Sending NMI from CPU 0 to CPUs 3:
>>>>> NMI backtrace for cpu 3
>>>>> CPU: 3 UID: 0 PID: 86 Comm: kworker/3:3 Not tainted 6.12.0-rc5+ #53
>>>>> Hardware name: Firefly ROC-RK3328-CC (DT)
>>>>> Workqueue: pm genpd_power_off_work_fn
>>>>> pstate: 20400005 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>>>>> pc : regmap_unlock_spinlock+0x18/0x30
>>>>> lr : regmap_read+0x60/0x88
>>>>> sp : ffff800081123c00
>>>>> x29: ffff800081123c00 x28: ffff2fa4c62cad80 x27: 0000000000000000
>>>>> x26: ffffd74e6e660eb8 x25: ffff2fa4c62cae00 x24: 0000000000000040
>>>>> x23: ffffd74e6d2f3ab8 x22: 0000000000000001 x21: ffff800081123c74
>>>>> x20: 0000000000000000 x19: ffff2fa4c0412000 x18: 0000000000000000
>>>>> x17: 77202c31203d2065 x16: 6c6469203a72656c x15: 6c6f72746e6f632d
>>>>> x14: 7265776f703a6e6f x13: 2063766568206e69 x12: 616d6f64202c3431
>>>>> x11: 347830206f742030 x10: 3430303034783020 x9 : ffffd74e6c7369e0
>>>>> x8 : 3030316666206e69 x7 : 205d383738353733 x6 : 332e31202020205b
>>>>> x5 : ffffd74e6c73fc88 x4 : ffffd74e6c73fcd4 x3 : ffffd74e6c740b40
>>>>> x2 : ffff800080015484 x1 : 0000000000000000 x0 : ffff2fa4c0412000
>>>>> Call trace:
>>>>> regmap_unlock_spinlock+0x18/0x30
>>>>> rockchip_pmu_set_idle_request+0xac/0x2c0
>>>>> rockchip_pd_power+0x144/0x5f8
>>>>> rockchip_pd_power_off+0x1c/0x30
>>>>> _genpd_power_off+0x9c/0x180
>>>>> genpd_power_off.part.0.isra.0+0x130/0x2a8
>>>>> genpd_power_off_work_fn+0x6c/0x98
>>>>> process_one_work+0x170/0x3f0
>>>>> worker_thread+0x290/0x4a8
>>>>> kthread+0xec/0xf8
>>>>> ret_from_fork+0x10/0x20
>>>>> rockchip-pm-domain ff100000.syscon:power-controller: failed to get ack
>>>>> on
>>>>> domain 'hevc', val=0x88220
>>>>>
>>>>> Fixes: 52e02d377a72 ("arm64: dts: rockchip: add core dtsi file for
>>>>> RK3328 SoCs")
>>>>> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
>>>>
>>>> While I was unable to formally verify this clock assignment,
>>>> i.e. by using the RK3328 TRM or the downstream kernel source
>>>> from Rockchip, it makes perfect sense to me.  Thanks for the
>>>> patch, and please feel free to include:
>>>>
>>>> Reviewed-by: Dragan Simic <dsimic@manjaro.org>
>>>
>>> It is unfortunate the TRM doesn't include the clock maps, because they
>>> are extremely helpful when one can acquire them. It also doesn't help
>>> that the TRM register definition only referred to this as "pll". I was
>>> sent specifically the usb3 phy clock map for my work on the driver,
>>> which had the location of each switch and divider along with the
>>> register and bit that controlled it. That combined with the TRM
>>> register map allowed me to find this error.
>>
>> Apologies, that's the wrong response for this one.
> 
> No worries.
> 
>> This patch was the result of educated guess combined with testing. I
>> grabbed all of the clocks that looked like they could affect things,
>> then tested them one at a time until I isolated them to this clock. It
>> lives alone with cpll as the parent and has no children according to
>> the clock summary. (Though the writeup i mistakenly included above
>> proves the clock map isn't always accurate).
> 
> I see, thanks for all your work on this patch!  It surely took quite
> a lot of time to perform all the testing.
> 
>>>>> ---
>>>>>
>>>>>  arch/arm64/boot/dts/rockchip/rk3328.dtsi | 1 +
>>>>>  1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>>>>> b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>>>>> index 0597de415fe0..7d992c3c01ce 100644
>>>>> --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>>>>> @@ -333,6 +333,7 @@ power: power-controller {
>>>>>
>>>>>                       power-domain@RK3328_PD_HEVC {
>>>>>                               reg = <RK3328_PD_HEVC>;
>>>>> +                             clocks = <&cru SCLK_VENC_CORE>;

Do we also need to include one or all of the following clocks?

According to Fig. 3-6 RK3228H Clock Architecture Diagram 5 following
clocks point to the H265 block:

S51_6 (4PLL) / G6_3 / S51_0 (DivFree 1~32) / D4 ---- aclk_h265
                                                 \-- pclk_h265
S51_14 (4PLL) / G6_4 / S51_8 (DivFree 1~32) / D4 - clk_venc_core
S52_14 (4PLL) / G6_7 / S52_8 (DivFree 1~32) / D4 - clk_venc_dsp

Regards,
Jonas

>>>>>                               #power-domain-cells = <0>;
>>>>>                       };
>>>>>                       power-domain@RK3328_PD_VIDEO {

Re: [PATCH 4/6] arm64: dts: rockchip: add hevc power domain clock to rk3328

[Peter Geis]

On Tue, Dec 10, 2024 at 11:05 AM Jonas Karlman <jonas@kwiboo.se> wrote:
>
> Hi Peter,
>
> On 2024-12-10 14:53, Dragan Simic wrote:
> > Hello Peter,
> >
> > On 2024-12-10 14:23, Peter Geis wrote:
> >> On Tue, Dec 10, 2024 at 8:13 AM Peter Geis <pgwipeout@gmail.com> wrote:
> >>> On Tue, Dec 10, 2024 at 5:04 AM Dragan Simic <dsimic@manjaro.org>
> >>> wrote:
> >>>> On 2024-12-10 02:30, Peter Geis wrote:
> >>>>> There is a race condition at startup between disabling power domains
> >>>>> not
> >>>>> used and disabling clocks not used on the rk3328. When the clocks are
> >>>>> disabled first, the hevc power domain fails to shut off leading to a
> >>>>> splat of failures. Add the hevc core clock to the rk3328 power domain
> >>>>> node to prevent this condition.
> >>>>>
> >>>>> rcu: INFO: rcu_sched detected expedited stalls on CPUs/tasks: { 3-....
> >>>>> }
> >>>>> 1087 jiffies s: 89 root: 0x8/.
> >>>>> rcu: blocking rcu_node structures (internal RCU debug):
> >>>>> Sending NMI from CPU 0 to CPUs 3:
> >>>>> NMI backtrace for cpu 3
> >>>>> CPU: 3 UID: 0 PID: 86 Comm: kworker/3:3 Not tainted 6.12.0-rc5+ #53
> >>>>> Hardware name: Firefly ROC-RK3328-CC (DT)
> >>>>> Workqueue: pm genpd_power_off_work_fn
> >>>>> pstate: 20400005 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> >>>>> pc : regmap_unlock_spinlock+0x18/0x30
> >>>>> lr : regmap_read+0x60/0x88
> >>>>> sp : ffff800081123c00
> >>>>> x29: ffff800081123c00 x28: ffff2fa4c62cad80 x27: 0000000000000000
> >>>>> x26: ffffd74e6e660eb8 x25: ffff2fa4c62cae00 x24: 0000000000000040
> >>>>> x23: ffffd74e6d2f3ab8 x22: 0000000000000001 x21: ffff800081123c74
> >>>>> x20: 0000000000000000 x19: ffff2fa4c0412000 x18: 0000000000000000
> >>>>> x17: 77202c31203d2065 x16: 6c6469203a72656c x15: 6c6f72746e6f632d
> >>>>> x14: 7265776f703a6e6f x13: 2063766568206e69 x12: 616d6f64202c3431
> >>>>> x11: 347830206f742030 x10: 3430303034783020 x9 : ffffd74e6c7369e0
> >>>>> x8 : 3030316666206e69 x7 : 205d383738353733 x6 : 332e31202020205b
> >>>>> x5 : ffffd74e6c73fc88 x4 : ffffd74e6c73fcd4 x3 : ffffd74e6c740b40
> >>>>> x2 : ffff800080015484 x1 : 0000000000000000 x0 : ffff2fa4c0412000
> >>>>> Call trace:
> >>>>> regmap_unlock_spinlock+0x18/0x30
> >>>>> rockchip_pmu_set_idle_request+0xac/0x2c0
> >>>>> rockchip_pd_power+0x144/0x5f8
> >>>>> rockchip_pd_power_off+0x1c/0x30
> >>>>> _genpd_power_off+0x9c/0x180
> >>>>> genpd_power_off.part.0.isra.0+0x130/0x2a8
> >>>>> genpd_power_off_work_fn+0x6c/0x98
> >>>>> process_one_work+0x170/0x3f0
> >>>>> worker_thread+0x290/0x4a8
> >>>>> kthread+0xec/0xf8
> >>>>> ret_from_fork+0x10/0x20
> >>>>> rockchip-pm-domain ff100000.syscon:power-controller: failed to get ack
> >>>>> on
> >>>>> domain 'hevc', val=0x88220
> >>>>>
> >>>>> Fixes: 52e02d377a72 ("arm64: dts: rockchip: add core dtsi file for
> >>>>> RK3328 SoCs")
> >>>>> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> >>>>
> >>>> While I was unable to formally verify this clock assignment,
> >>>> i.e. by using the RK3328 TRM or the downstream kernel source
> >>>> from Rockchip, it makes perfect sense to me.  Thanks for the
> >>>> patch, and please feel free to include:
> >>>>
> >>>> Reviewed-by: Dragan Simic <dsimic@manjaro.org>
> >>>
> >>> It is unfortunate the TRM doesn't include the clock maps, because they
> >>> are extremely helpful when one can acquire them. It also doesn't help
> >>> that the TRM register definition only referred to this as "pll". I was
> >>> sent specifically the usb3 phy clock map for my work on the driver,
> >>> which had the location of each switch and divider along with the
> >>> register and bit that controlled it. That combined with the TRM
> >>> register map allowed me to find this error.
> >>
> >> Apologies, that's the wrong response for this one.
> >
> > No worries.
> >
> >> This patch was the result of educated guess combined with testing. I
> >> grabbed all of the clocks that looked like they could affect things,
> >> then tested them one at a time until I isolated them to this clock. It
> >> lives alone with cpll as the parent and has no children according to
> >> the clock summary. (Though the writeup i mistakenly included above
> >> proves the clock map isn't always accurate).
> >
> > I see, thanks for all your work on this patch!  It surely took quite
> > a lot of time to perform all the testing.
> >
> >>>>> ---
> >>>>>
> >>>>>  arch/arm64/boot/dts/rockchip/rk3328.dtsi | 1 +
> >>>>>  1 file changed, 1 insertion(+)
> >>>>>
> >>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> >>>>> b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> >>>>> index 0597de415fe0..7d992c3c01ce 100644
> >>>>> --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> >>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> >>>>> @@ -333,6 +333,7 @@ power: power-controller {
> >>>>>
> >>>>>                       power-domain@RK3328_PD_HEVC {
> >>>>>                               reg = <RK3328_PD_HEVC>;
> >>>>> +                             clocks = <&cru SCLK_VENC_CORE>;
>
> Do we also need to include one or all of the following clocks?
>
> According to Fig. 3-6 RK3228H Clock Architecture Diagram 5 following
> clocks point to the H265 block:
>
> S51_6 (4PLL) / G6_3 / S51_0 (DivFree 1~32) / D4 ---- aclk_h265
>                                                  \-- pclk_h265
> S51_14 (4PLL) / G6_4 / S51_8 (DivFree 1~32) / D4 - clk_venc_core
> S52_14 (4PLL) / G6_7 / S52_8 (DivFree 1~32) / D4 - clk_venc_dsp

Good Afternoon,

Thanks for asking! If we were implementing the full encoder, probably.
However even with all the clocks enabled currently the encoder hard
locks the board if we touch it. For now adding just the SCLK_VENC_CORE
is enough to enable control of the power domain.

Very Respectfully,
Peter Geis

>
> Regards,
> Jonas
>
> >>>>>                               #power-domain-cells = <0>;
> >>>>>                       };
> >>>>>                       power-domain@RK3328_PD_VIDEO {
>

[PATCH 5/6] arm64: dts: rockchip: correct rk3328-roc regulator map

[Peter Geis]

The rk3328-roc-cc input power is sourced from a micro-usb port, while
the rk3328-roc-pc input power is sourced from a usb-c port. Both inputs
are 5vdc only. Remove the 12v input from the device tree.

While we are at it, add missing voltages and supply to vcc_phy, missing
voltages to vcc_host1_5v, and standardize the order of regulator
properties among the fixed regulators.

Fixes: 2171f4fdac06 ("arm64: dts: rockchip: add roc-rk3328-cc board")
Signed-off-by: Peter Geis <pgwipeout@gmail.com>
---

 arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi | 23 +++++++++++++-------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
index f782c8220dd3..6984387ff8b3 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
@@ -24,22 +24,23 @@ gmac_clkin: external-gmac-clock {
 		#clock-cells = <0>;
 	};
 
-	dc_12v: regulator-dc-12v {
+	/* fed from passive usb input connector */
+	dc_5v: regulator-dc-5v {
 		compatible = "regulator-fixed";
-		regulator-name = "dc_12v";
+		regulator-name = "dc_5v";
 		regulator-always-on;
 		regulator-boot-on;
-		regulator-min-microvolt = <12000000>;
-		regulator-max-microvolt = <12000000>;
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
 	};
 
 	vcc_sd: regulator-sdmmc {
 		compatible = "regulator-fixed";
+		regulator-name = "vcc_sd";
 		gpio = <&gpio0 RK_PD6 GPIO_ACTIVE_LOW>;
 		pinctrl-names = "default";
 		pinctrl-0 = <&sdmmc0m1_pin>;
 		regulator-boot-on;
-		regulator-name = "vcc_sd";
 		regulator-min-microvolt = <3300000>;
 		regulator-max-microvolt = <3300000>;
 		vin-supply = <&vcc_io>;
@@ -50,22 +51,25 @@ vcc_sdio: regulator-sdmmcio {
 		states = <1800000 0x1>, <3300000 0x0>;
 		regulator-name = "vcc_sdio";
 		regulator-type = "voltage";
+		regulator-always-on;
 		regulator-min-microvolt = <1800000>;
 		regulator-max-microvolt = <3300000>;
-		regulator-always-on;
 		vin-supply = <&vcc_sys>;
 	};
 
 	vcc_host1_5v: vcc_otg_5v: regulator-vcc-host1-5v {
 		compatible = "regulator-fixed";
+		regulator-name = "vcc_host1_5v";
 		enable-active-high;
 		pinctrl-names = "default";
 		pinctrl-0 = <&usb20_host_drv>;
-		regulator-name = "vcc_host1_5v";
 		regulator-always-on;
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
 		vin-supply = <&vcc_sys>;
 	};
 
+	/* sourced from usb input through 3A fuse */
 	vcc_sys: regulator-vcc-sys {
 		compatible = "regulator-fixed";
 		regulator-name = "vcc_sys";
@@ -73,7 +77,7 @@ vcc_sys: regulator-vcc-sys {
 		regulator-boot-on;
 		regulator-min-microvolt = <5000000>;
 		regulator-max-microvolt = <5000000>;
-		vin-supply = <&dc_12v>;
+		vin-supply = <&dc_5v>;
 	};
 
 	vcc_phy: regulator-vcc-phy {
@@ -81,6 +85,9 @@ vcc_phy: regulator-vcc-phy {
 		regulator-name = "vcc_phy";
 		regulator-always-on;
 		regulator-boot-on;
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		vin-supply = <&vcc_io>;
 	};
 
 	leds {
-- 
2.39.5

Re: [PATCH 5/6] arm64: dts: rockchip: correct rk3328-roc regulator map

[Heiko Stübner]

Am Dienstag, 10. Dezember 2024, 02:30:09 CET schrieb Peter Geis:
> The rk3328-roc-cc input power is sourced from a micro-usb port, while
> the rk3328-roc-pc input power is sourced from a usb-c port. Both inputs
> are 5vdc only. Remove the 12v input from the device tree.

full stop. Please don't do "While we are at it" commits.

> While we are at it, add missing voltages and supply to vcc_phy, missing
> voltages to vcc_host1_5v, and standardize the order of regulator
> properties among the fixed regulators.

This second part wants to be its own commit :-) .

Thanks
Heiko

> Fixes: 2171f4fdac06 ("arm64: dts: rockchip: add roc-rk3328-cc board")
> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> ---
> 
>  arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi | 23 +++++++++++++-------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> index f782c8220dd3..6984387ff8b3 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> @@ -24,22 +24,23 @@ gmac_clkin: external-gmac-clock {
>  		#clock-cells = <0>;
>  	};
>  
> -	dc_12v: regulator-dc-12v {
> +	/* fed from passive usb input connector */
> +	dc_5v: regulator-dc-5v {
>  		compatible = "regulator-fixed";
> -		regulator-name = "dc_12v";
> +		regulator-name = "dc_5v";
>  		regulator-always-on;
>  		regulator-boot-on;
> -		regulator-min-microvolt = <12000000>;
> -		regulator-max-microvolt = <12000000>;
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
>  	};
>  
>  	vcc_sd: regulator-sdmmc {
>  		compatible = "regulator-fixed";
> +		regulator-name = "vcc_sd";
>  		gpio = <&gpio0 RK_PD6 GPIO_ACTIVE_LOW>;
>  		pinctrl-names = "default";
>  		pinctrl-0 = <&sdmmc0m1_pin>;
>  		regulator-boot-on;
> -		regulator-name = "vcc_sd";
>  		regulator-min-microvolt = <3300000>;
>  		regulator-max-microvolt = <3300000>;
>  		vin-supply = <&vcc_io>;
> @@ -50,22 +51,25 @@ vcc_sdio: regulator-sdmmcio {
>  		states = <1800000 0x1>, <3300000 0x0>;
>  		regulator-name = "vcc_sdio";
>  		regulator-type = "voltage";
> +		regulator-always-on;
>  		regulator-min-microvolt = <1800000>;
>  		regulator-max-microvolt = <3300000>;
> -		regulator-always-on;
>  		vin-supply = <&vcc_sys>;
>  	};
>  
>  	vcc_host1_5v: vcc_otg_5v: regulator-vcc-host1-5v {
>  		compatible = "regulator-fixed";
> +		regulator-name = "vcc_host1_5v";
>  		enable-active-high;
>  		pinctrl-names = "default";
>  		pinctrl-0 = <&usb20_host_drv>;
> -		regulator-name = "vcc_host1_5v";
>  		regulator-always-on;
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
>  		vin-supply = <&vcc_sys>;
>  	};
>  
> +	/* sourced from usb input through 3A fuse */
>  	vcc_sys: regulator-vcc-sys {
>  		compatible = "regulator-fixed";
>  		regulator-name = "vcc_sys";
> @@ -73,7 +77,7 @@ vcc_sys: regulator-vcc-sys {
>  		regulator-boot-on;
>  		regulator-min-microvolt = <5000000>;
>  		regulator-max-microvolt = <5000000>;
> -		vin-supply = <&dc_12v>;
> +		vin-supply = <&dc_5v>;
>  	};
>  
>  	vcc_phy: regulator-vcc-phy {
> @@ -81,6 +85,9 @@ vcc_phy: regulator-vcc-phy {
>  		regulator-name = "vcc_phy";
>  		regulator-always-on;
>  		regulator-boot-on;
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		vin-supply = <&vcc_io>;
>  	};
>  
>  	leds {
> 

Re: [PATCH 5/6] arm64: dts: rockchip: correct rk3328-roc regulator map

[Peter Geis]

On Tue, Dec 10, 2024 at 5:54 AM Heiko Stübner <heiko@sntech.de> wrote:
>
> Am Dienstag, 10. Dezember 2024, 02:30:09 CET schrieb Peter Geis:
> > The rk3328-roc-cc input power is sourced from a micro-usb port, while
> > the rk3328-roc-pc input power is sourced from a usb-c port. Both inputs
> > are 5vdc only. Remove the 12v input from the device tree.
>
> full stop. Please don't do "While we are at it" commits.
>
> > While we are at it, add missing voltages and supply to vcc_phy, missing
> > voltages to vcc_host1_5v, and standardize the order of regulator
> > properties among the fixed regulators.
>
> This second part wants to be its own commit :-) .

Thank you, you're right I was torn between doing not enough and doing
too much and ended up doing both. Thinking about it now this should be
at least three patches:
- Power input
- Drop the phy regulator (it's directly tied to vcc_io, not a separate device)
- Cosmetic changes.

Thanks again!
Peter

>
> Thanks
> Heiko
>
> > Fixes: 2171f4fdac06 ("arm64: dts: rockchip: add roc-rk3328-cc board")
> > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> > ---
> >
> >  arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi | 23 +++++++++++++-------
> >  1 file changed, 15 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> > index f782c8220dd3..6984387ff8b3 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> > @@ -24,22 +24,23 @@ gmac_clkin: external-gmac-clock {
> >               #clock-cells = <0>;
> >       };
> >
> > -     dc_12v: regulator-dc-12v {
> > +     /* fed from passive usb input connector */
> > +     dc_5v: regulator-dc-5v {
> >               compatible = "regulator-fixed";
> > -             regulator-name = "dc_12v";
> > +             regulator-name = "dc_5v";
> >               regulator-always-on;
> >               regulator-boot-on;
> > -             regulator-min-microvolt = <12000000>;
> > -             regulator-max-microvolt = <12000000>;
> > +             regulator-min-microvolt = <5000000>;
> > +             regulator-max-microvolt = <5000000>;
> >       };
> >
> >       vcc_sd: regulator-sdmmc {
> >               compatible = "regulator-fixed";
> > +             regulator-name = "vcc_sd";
> >               gpio = <&gpio0 RK_PD6 GPIO_ACTIVE_LOW>;
> >               pinctrl-names = "default";
> >               pinctrl-0 = <&sdmmc0m1_pin>;
> >               regulator-boot-on;
> > -             regulator-name = "vcc_sd";
> >               regulator-min-microvolt = <3300000>;
> >               regulator-max-microvolt = <3300000>;
> >               vin-supply = <&vcc_io>;
> > @@ -50,22 +51,25 @@ vcc_sdio: regulator-sdmmcio {
> >               states = <1800000 0x1>, <3300000 0x0>;
> >               regulator-name = "vcc_sdio";
> >               regulator-type = "voltage";
> > +             regulator-always-on;
> >               regulator-min-microvolt = <1800000>;
> >               regulator-max-microvolt = <3300000>;
> > -             regulator-always-on;
> >               vin-supply = <&vcc_sys>;
> >       };
> >
> >       vcc_host1_5v: vcc_otg_5v: regulator-vcc-host1-5v {
> >               compatible = "regulator-fixed";
> > +             regulator-name = "vcc_host1_5v";
> >               enable-active-high;
> >               pinctrl-names = "default";
> >               pinctrl-0 = <&usb20_host_drv>;
> > -             regulator-name = "vcc_host1_5v";
> >               regulator-always-on;
> > +             regulator-min-microvolt = <5000000>;
> > +             regulator-max-microvolt = <5000000>;
> >               vin-supply = <&vcc_sys>;
> >       };
> >
> > +     /* sourced from usb input through 3A fuse */
> >       vcc_sys: regulator-vcc-sys {
> >               compatible = "regulator-fixed";
> >               regulator-name = "vcc_sys";
> > @@ -73,7 +77,7 @@ vcc_sys: regulator-vcc-sys {
> >               regulator-boot-on;
> >               regulator-min-microvolt = <5000000>;
> >               regulator-max-microvolt = <5000000>;
> > -             vin-supply = <&dc_12v>;
> > +             vin-supply = <&dc_5v>;
> >       };
> >
> >       vcc_phy: regulator-vcc-phy {
> > @@ -81,6 +85,9 @@ vcc_phy: regulator-vcc-phy {
> >               regulator-name = "vcc_phy";
> >               regulator-always-on;
> >               regulator-boot-on;
> > +             regulator-min-microvolt = <3300000>;
> > +             regulator-max-microvolt = <3300000>;
> > +             vin-supply = <&vcc_io>;
> >       };
> >
> >       leds {
> >
>
>
>
>

Re: [PATCH 5/6] arm64: dts: rockchip: correct rk3328-roc regulator map

[Diederik de Haas]

[-- Attachment #1: Type: text/plain, Size: 3742 bytes --]

Hi Peter,

Thanks for this series, I already saw some familiar error msgs
mentioned, so will try this series out soon (tm).

On Tue Dec 10, 2024 at 2:30 AM CET, Peter Geis wrote:
> The rk3328-roc-cc input power is sourced from a micro-usb port, while
> the rk3328-roc-pc input power is sourced from a usb-c port. Both inputs
> are 5vdc only. Remove the 12v input from the device tree.
>
> While we are at it, add missing voltages and supply to vcc_phy, missing
> voltages to vcc_host1_5v, and standardize the order of regulator
> properties among the fixed regulators.

Big fan of standardization :-) ...

>
> Fixes: 2171f4fdac06 ("arm64: dts: rockchip: add roc-rk3328-cc board")
> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> ---
>
>  arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi | 23 +++++++++++++-------
>  1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> index f782c8220dd3..6984387ff8b3 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> @@ -24,22 +24,23 @@ gmac_clkin: external-gmac-clock {
>  		#clock-cells = <0>;
>  	};
>  
> -	dc_12v: regulator-dc-12v {
> +	/* fed from passive usb input connector */
> +	dc_5v: regulator-dc-5v {
>  		compatible = "regulator-fixed";
> -		regulator-name = "dc_12v";
> +		regulator-name = "dc_5v";
>  		regulator-always-on;
>  		regulator-boot-on;
> -		regulator-min-microvolt = <12000000>;
> -		regulator-max-microvolt = <12000000>;
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
>  	};
>  
>  	vcc_sd: regulator-sdmmc {
>  		compatible = "regulator-fixed";
> +		regulator-name = "vcc_sd";
>  		gpio = <&gpio0 RK_PD6 GPIO_ACTIVE_LOW>;
>  		pinctrl-names = "default";
>  		pinctrl-0 = <&sdmmc0m1_pin>;
>  		regulator-boot-on;
> -		regulator-name = "vcc_sd";
>  		regulator-min-microvolt = <3300000>;
>  		regulator-max-microvolt = <3300000>;
>  		vin-supply = <&vcc_io>;

... but why not put regulator-name as the first of the regulator
properties as is done in the rk3328-rock64.dts ...

> @@ -50,22 +51,25 @@ vcc_sdio: regulator-sdmmcio {
>  		states = <1800000 0x1>, <3300000 0x0>;
>  		regulator-name = "vcc_sdio";
>  		regulator-type = "voltage";
> +		regulator-always-on;
>  		regulator-min-microvolt = <1800000>;
>  		regulator-max-microvolt = <3300000>;
> -		regulator-always-on;
>  		vin-supply = <&vcc_sys>;
>  	};
>  
>  	vcc_host1_5v: vcc_otg_5v: regulator-vcc-host1-5v {
>  		compatible = "regulator-fixed";
> +		regulator-name = "vcc_host1_5v";
>  		enable-active-high;
>  		pinctrl-names = "default";
>  		pinctrl-0 = <&usb20_host_drv>;
> -		regulator-name = "vcc_host1_5v";
>  		regulator-always-on;
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
>  		vin-supply = <&vcc_sys>;
>  	};

... and was the case here?

Cheers,
  Diederik

>  
> +	/* sourced from usb input through 3A fuse */
>  	vcc_sys: regulator-vcc-sys {
>  		compatible = "regulator-fixed";
>  		regulator-name = "vcc_sys";
> @@ -73,7 +77,7 @@ vcc_sys: regulator-vcc-sys {
>  		regulator-boot-on;
>  		regulator-min-microvolt = <5000000>;
>  		regulator-max-microvolt = <5000000>;
> -		vin-supply = <&dc_12v>;
> +		vin-supply = <&dc_5v>;
>  	};
>  
>  	vcc_phy: regulator-vcc-phy {
> @@ -81,6 +85,9 @@ vcc_phy: regulator-vcc-phy {
>  		regulator-name = "vcc_phy";
>  		regulator-always-on;
>  		regulator-boot-on;
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		vin-supply = <&vcc_io>;
>  	};
>  
>  	leds {


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 5/6] arm64: dts: rockchip: correct rk3328-roc regulator map

[Peter Geis]

On Tue, Dec 10, 2024 at 6:31 AM Diederik de Haas <didi.debian@cknow.org> wrote:
>
> Hi Peter,
>
> Thanks for this series, I already saw some familiar error msgs
> mentioned, so will try this series out soon (tm).
>
> On Tue Dec 10, 2024 at 2:30 AM CET, Peter Geis wrote:
> > The rk3328-roc-cc input power is sourced from a micro-usb port, while
> > the rk3328-roc-pc input power is sourced from a usb-c port. Both inputs
> > are 5vdc only. Remove the 12v input from the device tree.
> >
> > While we are at it, add missing voltages and supply to vcc_phy, missing
> > voltages to vcc_host1_5v, and standardize the order of regulator
> > properties among the fixed regulators.
>
> Big fan of standardization :-) ...
>
> >
> > Fixes: 2171f4fdac06 ("arm64: dts: rockchip: add roc-rk3328-cc board")
> > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> > ---
> >
> >  arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi | 23 +++++++++++++-------
> >  1 file changed, 15 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> > index f782c8220dd3..6984387ff8b3 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> > @@ -24,22 +24,23 @@ gmac_clkin: external-gmac-clock {
> >               #clock-cells = <0>;
> >       };
> >
> > -     dc_12v: regulator-dc-12v {
> > +     /* fed from passive usb input connector */
> > +     dc_5v: regulator-dc-5v {
> >               compatible = "regulator-fixed";
> > -             regulator-name = "dc_12v";
> > +             regulator-name = "dc_5v";
> >               regulator-always-on;
> >               regulator-boot-on;
> > -             regulator-min-microvolt = <12000000>;
> > -             regulator-max-microvolt = <12000000>;
> > +             regulator-min-microvolt = <5000000>;
> > +             regulator-max-microvolt = <5000000>;
> >       };
> >
> >       vcc_sd: regulator-sdmmc {
> >               compatible = "regulator-fixed";
> > +             regulator-name = "vcc_sd";
> >               gpio = <&gpio0 RK_PD6 GPIO_ACTIVE_LOW>;
> >               pinctrl-names = "default";
> >               pinctrl-0 = <&sdmmc0m1_pin>;
> >               regulator-boot-on;
> > -             regulator-name = "vcc_sd";
> >               regulator-min-microvolt = <3300000>;
> >               regulator-max-microvolt = <3300000>;
> >               vin-supply = <&vcc_io>;
>
> ... but why not put regulator-name as the first of the regulator
> properties as is done in the rk3328-rock64.dts ...
>
> > @@ -50,22 +51,25 @@ vcc_sdio: regulator-sdmmcio {
> >               states = <1800000 0x1>, <3300000 0x0>;
> >               regulator-name = "vcc_sdio";
> >               regulator-type = "voltage";
> > +             regulator-always-on;
> >               regulator-min-microvolt = <1800000>;
> >               regulator-max-microvolt = <3300000>;
> > -             regulator-always-on;
> >               vin-supply = <&vcc_sys>;
> >       };
> >
> >       vcc_host1_5v: vcc_otg_5v: regulator-vcc-host1-5v {
> >               compatible = "regulator-fixed";
> > +             regulator-name = "vcc_host1_5v";
> >               enable-active-high;
> >               pinctrl-names = "default";
> >               pinctrl-0 = <&usb20_host_drv>;
> > -             regulator-name = "vcc_host1_5v";
> >               regulator-always-on;
> > +             regulator-min-microvolt = <5000000>;
> > +             regulator-max-microvolt = <5000000>;
> >               vin-supply = <&vcc_sys>;
> >       };
>
> ... and was the case here?

That's fair, thank you. I like the alphabetical approach, I'll go that
way when I split this out.

>
> Cheers,
>   Diederik
>
> >
> > +     /* sourced from usb input through 3A fuse */
> >       vcc_sys: regulator-vcc-sys {
> >               compatible = "regulator-fixed";
> >               regulator-name = "vcc_sys";
> > @@ -73,7 +77,7 @@ vcc_sys: regulator-vcc-sys {
> >               regulator-boot-on;
> >               regulator-min-microvolt = <5000000>;
> >               regulator-max-microvolt = <5000000>;
> > -             vin-supply = <&dc_12v>;
> > +             vin-supply = <&dc_5v>;
> >       };
> >
> >       vcc_phy: regulator-vcc-phy {
> > @@ -81,6 +85,9 @@ vcc_phy: regulator-vcc-phy {
> >               regulator-name = "vcc_phy";
> >               regulator-always-on;
> >               regulator-boot-on;
> > +             regulator-min-microvolt = <3300000>;
> > +             regulator-max-microvolt = <3300000>;
> > +             vin-supply = <&vcc_io>;
> >       };
> >
> >       leds {
>

Re: [PATCH 5/6] arm64: dts: rockchip: correct rk3328-roc regulator map

[Diederik de Haas]

[-- Attachment #1: Type: text/plain, Size: 3911 bytes --]

On Tue Dec 10, 2024 at 2:04 PM CET, Peter Geis wrote:
> On Tue, Dec 10, 2024 at 6:31 AM Diederik de Haas <didi.debian@cknow.org> wrote:
> > On Tue Dec 10, 2024 at 2:30 AM CET, Peter Geis wrote:
> > > voltages to vcc_host1_5v, and standardize the order of regulator
> > > properties among the fixed regulators.
> >
> > Big fan of standardization :-) ...
> >
> > >
> > > Fixes: 2171f4fdac06 ("arm64: dts: rockchip: add roc-rk3328-cc board")
> > > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> > > ---
> > >
> > >  arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi | 23 +++++++++++++-------
> > >  1 file changed, 15 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> > > index f782c8220dd3..6984387ff8b3 100644
> > > --- a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> > > +++ b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> > > @@ -24,22 +24,23 @@ gmac_clkin: external-gmac-clock {
> > >               #clock-cells = <0>;
> > >       };
> > >
> > > -     dc_12v: regulator-dc-12v {
> > > +     /* fed from passive usb input connector */
> > > +     dc_5v: regulator-dc-5v {
> > >               compatible = "regulator-fixed";
> > > -             regulator-name = "dc_12v";
> > > +             regulator-name = "dc_5v";
> > >               regulator-always-on;
> > >               regulator-boot-on;
> > > -             regulator-min-microvolt = <12000000>;
> > > -             regulator-max-microvolt = <12000000>;
> > > +             regulator-min-microvolt = <5000000>;
> > > +             regulator-max-microvolt = <5000000>;
> > >       };
> > >
> > >       vcc_sd: regulator-sdmmc {
> > >               compatible = "regulator-fixed";
> > > +             regulator-name = "vcc_sd";
> > >               gpio = <&gpio0 RK_PD6 GPIO_ACTIVE_LOW>;
> > >               pinctrl-names = "default";
> > >               pinctrl-0 = <&sdmmc0m1_pin>;
> > >               regulator-boot-on;
> > > -             regulator-name = "vcc_sd";
> > >               regulator-min-microvolt = <3300000>;
> > >               regulator-max-microvolt = <3300000>;
> > >               vin-supply = <&vcc_io>;
> >
> > ... but why not put regulator-name as the first of the regulator
> > properties as is done in the rk3328-rock64.dts ...
> >
> > > @@ -50,22 +51,25 @@ vcc_sdio: regulator-sdmmcio {
> > >               states = <1800000 0x1>, <3300000 0x0>;
> > >               regulator-name = "vcc_sdio";
> > >               regulator-type = "voltage";
> > > +             regulator-always-on;
> > >               regulator-min-microvolt = <1800000>;
> > >               regulator-max-microvolt = <3300000>;
> > > -             regulator-always-on;
> > >               vin-supply = <&vcc_sys>;
> > >       };
> > >
> > >       vcc_host1_5v: vcc_otg_5v: regulator-vcc-host1-5v {
> > >               compatible = "regulator-fixed";
> > > +             regulator-name = "vcc_host1_5v";
> > >               enable-active-high;
> > >               pinctrl-names = "default";
> > >               pinctrl-0 = <&usb20_host_drv>;
> > > -             regulator-name = "vcc_host1_5v";
> > >               regulator-always-on;
> > > +             regulator-min-microvolt = <5000000>;
> > > +             regulator-max-microvolt = <5000000>;
> > >               vin-supply = <&vcc_sys>;
> > >       };
> >
> > ... and was the case here?
>
> That's fair, thank you. I like the alphabetical approach, I'll go that
> way when I split this out.

FWIW, I'm fine when regulator-name and regulator-type would be put on
the top of the regulator-* properties, also because that's currently
the case for the rk3328-rock64.dts.
And a strict alphabetical order would look weird anyway as then
-max-microvolt should be ordered above -min-microvolt.

Cheers,
  Diederik

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 6/6] arm64: dts: rockchip: Remove address aligned beats from rk3328-roc

[Peter Geis]

Since commit 8a469ee35606 ("arm64: dts: rockchip: Add txpbl node for
RK3399/RK3328"), the snps,aal, snps,txpbl, and snps,rxpbl nodes have
been unnecessary in the separate device trees. There is also a
performance loss to using snps,aal. Remove these from the rk3328-roc
device tree.

Signed-off-by: Peter Geis <pgwipeout@gmail.com>

---

 arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
index 6984387ff8b3..0d476cc2144d 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
@@ -155,12 +155,9 @@ &gmac2io {
 	phy-mode = "rgmii";
 	pinctrl-names = "default";
 	pinctrl-0 = <&rgmiim1_pins>;
-	snps,aal;
 	snps,reset-gpio = <&gpio1 RK_PC2 GPIO_ACTIVE_LOW>;
 	snps,reset-active-low;
 	snps,reset-delays-us = <0 10000 50000>;
-	snps,rxpbl = <0x4>;
-	snps,txpbl = <0x4>;
 	tx_delay = <0x24>;
 	rx_delay = <0x18>;
 	status = "okay";
-- 
2.39.5

Re: [PATCH 6/6] arm64: dts: rockchip: Remove address aligned beats from rk3328-roc

[Dragan Simic]

Hello Peter,

Thanks for the patch.  Please, see some comments below.

On 2024-12-10 02:30, Peter Geis wrote:
> Since commit 8a469ee35606 ("arm64: dts: rockchip: Add txpbl node for
> RK3399/RK3328"), the snps,aal, snps,txpbl, and snps,rxpbl nodes have
> been unnecessary in the separate device trees. There is also a
> performance loss to using snps,aal. Remove these from the rk3328-roc
> device tree.
> 
> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> 
> ---
> 
>  arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> index 6984387ff8b3..0d476cc2144d 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> @@ -155,12 +155,9 @@ &gmac2io {
>  	phy-mode = "rgmii";
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&rgmiim1_pins>;
> -	snps,aal;

Huh, I see that quite a few RK3328 board dts files specify
the snps,aal node.  I wonder was it a "cargo cult" approach
at play, :) or was there some real need for it?

Actually, I see now that you added snps,aal to rk3328-roc-
cc.dts in the commit 393f3875c385 ("arm64: dts: rockchip:
improve rk3328-roc-cc rgmii performance."), so I guess that
your further research and testing showed that it actually
isn't needed for Ethernet stability?

>  	snps,reset-gpio = <&gpio1 RK_PC2 GPIO_ACTIVE_LOW>;
>  	snps,reset-active-low;
>  	snps,reset-delays-us = <0 10000 50000>;
> -	snps,rxpbl = <0x4>;
> -	snps,txpbl = <0x4>;

Unless I'm missing something, the commit 8a469ee35606 ("arm64:
dts: rockchip: Add txpbl node for RK3399/RK3328") doesn't add
the snps,rxpbl node to the RK3328 SoC dtsi, and the respective
driver does nothing about it when the snps,txpbl node is found.

Though, I see that rk3328-rock-pi-e.dts is the only other
RK3328 board dts file that specifies the snps,rxpbl node, so
it seems that removing the snps,rxpbl node here should be safe,
especially because it was you who added it in the same commit
mentioned above.  If there were some SoC-level issues, all
RK3328 boards would've needed it.

>  	tx_delay = <0x24>;
>  	rx_delay = <0x18>;
>  	status = "okay";

Re: [PATCH 6/6] arm64: dts: rockchip: Remove address aligned beats from rk3328-roc

[Peter Geis]

On Tue, Dec 10, 2024 at 5:45 AM Dragan Simic <dsimic@manjaro.org> wrote:
>
> Hello Peter,
>
> Thanks for the patch.  Please, see some comments below.
>
> On 2024-12-10 02:30, Peter Geis wrote:
> > Since commit 8a469ee35606 ("arm64: dts: rockchip: Add txpbl node for
> > RK3399/RK3328"), the snps,aal, snps,txpbl, and snps,rxpbl nodes have
> > been unnecessary in the separate device trees. There is also a
> > performance loss to using snps,aal. Remove these from the rk3328-roc
> > device tree.
> >
> > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> >
> > ---
> >
> >  arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi | 3 ---
> >  1 file changed, 3 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> > b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> > index 6984387ff8b3..0d476cc2144d 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> > @@ -155,12 +155,9 @@ &gmac2io {
> >       phy-mode = "rgmii";
> >       pinctrl-names = "default";
> >       pinctrl-0 = <&rgmiim1_pins>;
> > -     snps,aal;
>
> Huh, I see that quite a few RK3328 board dts files specify
> the snps,aal node.  I wonder was it a "cargo cult" approach
> at play, :) or was there some real need for it?
>
> Actually, I see now that you added snps,aal to rk3328-roc-
> cc.dts in the commit 393f3875c385 ("arm64: dts: rockchip:
> improve rk3328-roc-cc rgmii performance."), so I guess that
> your further research and testing showed that it actually
> isn't needed for Ethernet stability?
>
> >       snps,reset-gpio = <&gpio1 RK_PC2 GPIO_ACTIVE_LOW>;
> >       snps,reset-active-low;
> >       snps,reset-delays-us = <0 10000 50000>;
> > -     snps,rxpbl = <0x4>;
> > -     snps,txpbl = <0x4>;
>
> Unless I'm missing something, the commit 8a469ee35606 ("arm64:
> dts: rockchip: Add txpbl node for RK3399/RK3328") doesn't add
> the snps,rxpbl node to the RK3328 SoC dtsi, and the respective
> driver does nothing about it when the snps,txpbl node is found.
>
> Though, I see that rk3328-rock-pi-e.dts is the only other
> RK3328 board dts file that specifies the snps,rxpbl node, so
> it seems that removing the snps,rxpbl node here should be safe,
> especially because it was you who added it in the same commit
> mentioned above.  If there were some SoC-level issues, all
> RK3328 boards would've needed it.

Good Morning,

You'll notice the author of that patch was me. Setting aal, txpbl, and
rxpbl was the original fix I came up with for the rk3328, which I
applied to the only board I had. Someone else later on isolated it
specifically isolated it to just the txpbl and applied it to both the
rk3328 and rk3399 directly.

This was just something that was left hanging after that result.

Looking at how rk356x was done, I suspect there's an even more elegant
solution. However I don't have the deep level knowledge nor
documentation to implement it.

Very Respectfully,
Peter Geis
>
> >       tx_delay = <0x24>;
> >       rx_delay = <0x18>;
> >       status = "okay";

Re: [PATCH 6/6] arm64: dts: rockchip: Remove address aligned beats from rk3328-roc

[Dragan Simic]

Hello Peter,

On 2024-12-10 12:29, Peter Geis wrote:
> On Tue, Dec 10, 2024 at 5:45 AM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> Thanks for the patch.  Please, see some comments below.
>> 
>> On 2024-12-10 02:30, Peter Geis wrote:
>> > Since commit 8a469ee35606 ("arm64: dts: rockchip: Add txpbl node for
>> > RK3399/RK3328"), the snps,aal, snps,txpbl, and snps,rxpbl nodes have
>> > been unnecessary in the separate device trees. There is also a
>> > performance loss to using snps,aal. Remove these from the rk3328-roc
>> > device tree.
>> >
>> > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
>> >
>> > ---
>> >
>> >  arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi | 3 ---
>> >  1 file changed, 3 deletions(-)
>> >
>> > diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
>> > b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
>> > index 6984387ff8b3..0d476cc2144d 100644
>> > --- a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
>> > +++ b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
>> > @@ -155,12 +155,9 @@ &gmac2io {
>> >       phy-mode = "rgmii";
>> >       pinctrl-names = "default";
>> >       pinctrl-0 = <&rgmiim1_pins>;
>> > -     snps,aal;
>> 
>> Huh, I see that quite a few RK3328 board dts files specify
>> the snps,aal node.  I wonder was it a "cargo cult" approach
>> at play, :) or was there some real need for it?
>> 
>> Actually, I see now that you added snps,aal to rk3328-roc-
>> cc.dts in the commit 393f3875c385 ("arm64: dts: rockchip:
>> improve rk3328-roc-cc rgmii performance."), so I guess that
>> your further research and testing showed that it actually
>> isn't needed for Ethernet stability?
>> 
>> >       snps,reset-gpio = <&gpio1 RK_PC2 GPIO_ACTIVE_LOW>;
>> >       snps,reset-active-low;
>> >       snps,reset-delays-us = <0 10000 50000>;
>> > -     snps,rxpbl = <0x4>;
>> > -     snps,txpbl = <0x4>;
>> 
>> Unless I'm missing something, the commit 8a469ee35606 ("arm64:
>> dts: rockchip: Add txpbl node for RK3399/RK3328") doesn't add
>> the snps,rxpbl node to the RK3328 SoC dtsi, and the respective
>> driver does nothing about it when the snps,txpbl node is found.
>> 
>> Though, I see that rk3328-rock-pi-e.dts is the only other
>> RK3328 board dts file that specifies the snps,rxpbl node, so
>> it seems that removing the snps,rxpbl node here should be safe,
>> especially because it was you who added it in the same commit
>> mentioned above.  If there were some SoC-level issues, all
>> RK3328 boards would've needed it.
> 
> Good Morning,
> 
> You'll notice the author of that patch was me. Setting aal, txpbl, and
> rxpbl was the original fix I came up with for the rk3328, which I
> applied to the only board I had. Someone else later on isolated it
> specifically isolated it to just the txpbl and applied it to both the
> rk3328 and rk3399 directly.
> 
> This was just something that was left hanging after that result.
> 
> Looking at how rk356x was done, I suspect there's an even more elegant
> solution. However I don't have the deep level knowledge nor
> documentation to implement it.

Sure, I noticed that you authored the original Ethernet stability
fix. :)  With all this in mind, please feel free to include

Reviewed-by: Dragan Simic <dsimic@manjaro.org>

and I'll prepare a patch or two that clean up any and all leftovers
in other board dts(i) files.

Re: [PATCH 6/6] arm64: dts: rockchip: Remove address aligned beats from rk3328-roc

[Dragan Simic]

On 2024-12-10 14:44, Dragan Simic wrote:
> On 2024-12-10 12:29, Peter Geis wrote:
>> On Tue, Dec 10, 2024 at 5:45 AM Dragan Simic <dsimic@manjaro.org> 
>> wrote:
>>> Thanks for the patch.  Please, see some comments below.
>>> 
>>> On 2024-12-10 02:30, Peter Geis wrote:
>>> > Since commit 8a469ee35606 ("arm64: dts: rockchip: Add txpbl node for
>>> > RK3399/RK3328"), the snps,aal, snps,txpbl, and snps,rxpbl nodes have
>>> > been unnecessary in the separate device trees. There is also a
>>> > performance loss to using snps,aal. Remove these from the rk3328-roc
>>> > device tree.
>>> >
>>> > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
>>> >
>>> > ---
>>> >
>>> >  arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi | 3 ---
>>> >  1 file changed, 3 deletions(-)
>>> >
>>> > diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
>>> > b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
>>> > index 6984387ff8b3..0d476cc2144d 100644
>>> > --- a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
>>> > +++ b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
>>> > @@ -155,12 +155,9 @@ &gmac2io {
>>> >       phy-mode = "rgmii";
>>> >       pinctrl-names = "default";
>>> >       pinctrl-0 = <&rgmiim1_pins>;
>>> > -     snps,aal;
>>> 
>>> Huh, I see that quite a few RK3328 board dts files specify
>>> the snps,aal node.  I wonder was it a "cargo cult" approach
>>> at play, :) or was there some real need for it?
>>> 
>>> Actually, I see now that you added snps,aal to rk3328-roc-
>>> cc.dts in the commit 393f3875c385 ("arm64: dts: rockchip:
>>> improve rk3328-roc-cc rgmii performance."), so I guess that
>>> your further research and testing showed that it actually
>>> isn't needed for Ethernet stability?
>>> 
>>> >       snps,reset-gpio = <&gpio1 RK_PC2 GPIO_ACTIVE_LOW>;
>>> >       snps,reset-active-low;
>>> >       snps,reset-delays-us = <0 10000 50000>;
>>> > -     snps,rxpbl = <0x4>;
>>> > -     snps,txpbl = <0x4>;
>>> 
>>> Unless I'm missing something, the commit 8a469ee35606 ("arm64:
>>> dts: rockchip: Add txpbl node for RK3399/RK3328") doesn't add
>>> the snps,rxpbl node to the RK3328 SoC dtsi, and the respective
>>> driver does nothing about it when the snps,txpbl node is found.
>>> 
>>> Though, I see that rk3328-rock-pi-e.dts is the only other
>>> RK3328 board dts file that specifies the snps,rxpbl node, so
>>> it seems that removing the snps,rxpbl node here should be safe,
>>> especially because it was you who added it in the same commit
>>> mentioned above.  If there were some SoC-level issues, all
>>> RK3328 boards would've needed it.
>> 
>> Good Morning,
>> 
>> You'll notice the author of that patch was me. Setting aal, txpbl, and
>> rxpbl was the original fix I came up with for the rk3328, which I
>> applied to the only board I had. Someone else later on isolated it
>> specifically isolated it to just the txpbl and applied it to both the
>> rk3328 and rk3399 directly.
>> 
>> This was just something that was left hanging after that result.
>> 
>> Looking at how rk356x was done, I suspect there's an even more elegant
>> solution. However I don't have the deep level knowledge nor
>> documentation to implement it.
> 
> Sure, I noticed that you authored the original Ethernet stability
> fix. :)  With all this in mind, please feel free to include
> 
> Reviewed-by: Dragan Simic <dsimic@manjaro.org>
> 
> and I'll prepare a patch or two that clean up any and all leftovers
> in other board dts(i) files.

For future reference, to help anyone going through the mailing-list
archive, here's a link to the above-mentioned cleanup patch:

https://lore.kernel.org/linux-rockchip/e00f08d2351e82d6acd56271a68c7ed05b3362e8.1733901896.git.dsimic@manjaro.org/T/#u