[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 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