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

[Dragan Simic <dsimic@manjaro.org>]

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 {