Re: [PATCH] reset: use a shared SRCU domain for reset controls

[Philipp Zabel]

Hi Steven, Heiko,

On Do, 2026-04-23 at 13:45 +0100, Steven Price wrote:
> +Heiko for the Rockchip questions.
> 
> On 23/04/2026 11:27, Philipp Zabel wrote:
> > Hi Steven,
> > 
> > On Fr, 2026-04-17 at 16:48 +0100, Steven Price wrote:
> > > Commit 78ebbff6d1a0 ("reset: handle removing supplier before consumers")
> > > added a dynamically initialized srcu_struct to every reset_control and
> > > cleaned it up again when the handle was dropped.
> > > 
> > > That breaks early boot users which acquire and release reset handles
> > > before workqueues are online. On rk3288 this shows up during
> > > rockchip_smp_prepare_cpus(), where pmu_set_power_domain() gets a reset
> > > control for a CPU core and then drops it again before SMP bring-up has
> > > finished.
> > 
> > Can the reset_control_put() call be dropped from pmu_set_power_domain()
> > to fix the problem?
> 
> I'm not that familiar with the code, so I'm not sure.
> 
> Just dropping that call causes a WARN_ON() bringing the secondary CPUs
> on (because the call to rockchip_get_core_reset() expects to have
> exclusive access to the reset).

Yes, caused by re-requesting the same reset again. I was thinking of
storing the controls in an array in rockchip_smp_prepare_cpus() and
reusing them in pmu_set_power_domain(), see the patch below.

> Switching to a shared reset then his a
> WARN_ON() in reset_control_assert because deassert_count == 0. I could
> keep digging blindly but I'm not really sure how this code is meant to work.

Shared reset is not the right mechanism here, that would be for
multiple drivers/devices sharing the same reset line.

> Hopefully Heiko might be able to shed some more light on this?
>
> > Putting the reset control should mean that the driver doesn't care
> > about the state of the reset line anymore, but the platsmp code very
> > much expects the reset line to stay deasserted after enabling a CPU.
> > Acquiring reset controls in rockchip_smp_prepare_cpus() once and never
> > giving them up via reset_control_put() seems like a correct fix,
> > regardless of whether this patch is applied or not.
> > 
> > It looks like the meson platsmp suffers from the same issue.
> 
> This is why I did the fix in the reset code -

And I'm grateful, because that made sashiko.dev point out an ABBA
deadlock opportunity in the existing code.

> how many other platforms
> might have similar issues? But obviously if these platforms are buggy
> then they should be fixed.

Agreed.

> My interest is keeping the devboard working so I can keep testing
> Panfrost on it.

Does this patch work?


----------8<----------
From: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
Subject: [PATCH] ARM: rockchip: keep reset control around

Do not put the reset control, retain exclusive control over it.
After turning on a CPU, the corresponding reset line must stay
deasserted.

This also avoids calling reset_control_put() before workqueues
are operational.

Signed-off-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
---
 arch/arm/mach-rockchip/platsmp.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-rockchip/platsmp.c b/arch/arm/mach-rockchip/platsmp.c
index f432d22bfed8..c28816fffce8 100644
--- a/arch/arm/mach-rockchip/platsmp.c
+++ b/arch/arm/mach-rockchip/platsmp.c
@@ -34,6 +34,7 @@ static int ncores;
 
 static struct regmap *pmu;
 static int has_pmu = true;
+static struct reset_control *cpu_rstc[4];
 
 static int pmu_power_domain_is_on(int pd)
 {
@@ -64,9 +65,11 @@ static struct reset_control *rockchip_get_core_reset(int cpu)
 static int pmu_set_power_domain(int pd, bool on)
 {
 	u32 val = (on) ? 0 : BIT(pd);
-	struct reset_control *rstc = rockchip_get_core_reset(pd);
+	struct reset_control *rstc;
 	int ret;
 
+	rstc = pd < ARRAY_SIZE(cpu_rstc) ? cpu_rstc[pd] : ERR_PTR(-EINVAL);
+
 	if (IS_ERR(rstc) && read_cpuid_part() != ARM_CPU_PART_CORTEX_A9) {
 		pr_err("%s: could not get reset control for core %d\n",
 		       __func__, pd);
@@ -100,11 +103,8 @@ static int pmu_set_power_domain(int pd, bool on)
 		}
 	}
 
-	if (!IS_ERR(rstc)) {
-		if (on)
-			reset_control_deassert(rstc);
-		reset_control_put(rstc);
-	}
+	if (!IS_ERR(rstc) && on)
+		reset_control_deassert(rstc);
 
 	return 0;
 }
@@ -312,6 +312,9 @@ static void __init rockchip_smp_prepare_cpus(unsigned int max_cpus)
 		ncores = ((l2ctlr >> 24) & 0x3) + 1;
 	}
 
+	for (i = 0; i < ncores; i++)
+		cpu_rstc[i] = rockchip_get_core_reset(i);
+
 	/* Make sure that all cores except the first are really off */
 	for (i = 1; i < ncores; i++)
 		pmu_set_power_domain(0 + i, false);
-- 
2.47.3
---------->8----------

regards
Philipp