Message-ID: <CAMdYzYoGo=dLag30Dp3p8xzrqvb0peB7Oke=zKR-9qbaTns+zQ@mail.gmail.com>
Date: Tue, 10 Dec 2024 15:05:06 -0500
From: Peter Geis <pgwipeout@...il.com>
To: Jonas Karlman <jonas@...boo.se>
Cc: Dragan Simic <dsimic@...jaro.org>, Heiko Stuebner <heiko@...ech.de>, Alex Bee <knaerzche@...il.com>, 
	Conor Dooley <conor+dt@...nel.org>, Diederik de Haas <didi.debian@...ow.org>, 
	Johan Jonker <jbx6244@...il.com>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Liang Chen <cl@...k-chips.com>, 
	Rob Herring <robh@...nel.org>, devicetree@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	linux-rockchip@...ts.infradead.org, shironeko <shironeko@...aguri.club>
Subject: Re: [PATCH 4/6] arm64: dts: rockchip: add hevc power domain clock to rk3328

On Tue, Dec 10, 2024 at 11:05 AM Jonas Karlman <jonas@...boo.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@...il.com> wrote:
> >>> On Tue, Dec 10, 2024 at 5:04 AM Dragan Simic <dsimic@...jaro.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@...il.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@...jaro.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@...328_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@...328_PD_VIDEO {
>

Powered by blists - more mailing lists