From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id E56932E1C4E for ; Thu, 14 May 2026 09:07:33 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778749656; cv=none; b=GgJ4pI4fABiR4HA+gGuTL6Gr1TdA4RQRBl3d4S651wHdj1vgFANx0lpOfmuuJfQbDDNLsgKMkEy1d4TcfuhwEp4kxzoaTicH6jhXSwdvoDlJpG2leJI4vW/igg/W+4e/JVGNvMVyiC5cLi+e3qDw/GwbSmps9xZi05GvRdKlZjg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778749656; c=relaxed/simple; bh=E7faLS142q3Oy02zg+cZwS+5yEmbxbOqJ2R80DdaHAI=; h=Message-ID:Date:MIME-Version:Subject:From:To:Cc:References: In-Reply-To:Content-Type; b=aDrJioBbWmPalfrfYKVCYnp/UL5BIVof5u9nJzhHptKYhokIqnI5IccZdt3jSvuDCwiO/ouwQOFLki5eJIKpfKfvF/5NQ6OO2O2+Lz2zu+6mhXQUeQ8t18d28yIKP7XoVIUPPon1xbp2ppf80IPKGGagH/CxW/8wCLQqL9LpQ2Y= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; dkim=pass (1024-bit key) header.d=arm.com header.i=@arm.com header.b=bXNGzKD+; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=arm.com header.i=@arm.com header.b="bXNGzKD+" Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id ACEC21D31; Thu, 14 May 2026 02:07:27 -0700 (PDT) Received: from [10.1.37.28] (e122027.cambridge.arm.com [10.1.37.28]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 003923F85F; Thu, 14 May 2026 02:07:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=arm.com; s=foss; t=1778749653; bh=E7faLS142q3Oy02zg+cZwS+5yEmbxbOqJ2R80DdaHAI=; h=Date:Subject:From:To:Cc:References:In-Reply-To:From; b=bXNGzKD+qCE8BPYSEPKRt034Eb8ICrn21+riMBuy53kCLZNhD5saIoL7aih1fGW3c hvPmmLwVtlRXgJzlSn/eN+38sVBTzE+57jrH4Suw2NqMYGPnnEm+Fm5zoWO56GVmjO Y4PpIxwzUm/jW111HaO18m80+gUCUzBCJms+bx5k= Message-ID: Date: Thu, 14 May 2026 10:07:29 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] reset: use a shared SRCU domain for reset controls From: Steven Price To: Philipp Zabel , Heiko Stuebner Cc: linux-kernel@vger.kernel.org, Bartosz Golaszewski References: <20260417154809.1984386-1-steven.price@arm.com> <9d06e549-ad7e-4089-99f1-88eb4b95a329@arm.com> Content-Language: en-GB In-Reply-To: <9d06e549-ad7e-4089-99f1-88eb4b95a329@arm.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Gentle ping (see below) On 24/04/2026 10:04, Steven Price wrote: > On 23/04/2026 15:15, Philipp Zabel wrote: >> 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. > > That makes sense - I hadn't really looked into whether this was a single > reset for all CPUs or somehow one shared between them all. Thinking > about it more I can't see how a shared reset would have worked ;) > >>> 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. > > Cool. > >>> 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? > > Indeed it does - thanks. Feel free to add my T-b when posting: > > Tested-by: Steven Price Can we pick the below patch up please? Let me know if there's anything I can do to move this along. Thanks, Steve > > Thanks, > Steve > >> >> ----------8<---------- >> From: Philipp Zabel >> 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 >> --- >> 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); >