Message-ID: <f16b3688-91e1-4760-ac2b-ce53db98ed68@kwiboo.se>
Date: Tue, 10 Dec 2024 17:05:22 +0100
From: Jonas Karlman <jonas@...boo.se>
To: Peter Geis <pgwipeout@...il.com>
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

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

Regards,
Jonas

>>>>>                               #power-domain-cells = <0>;
>>>>>                       };
>>>>>                       power-domain@...328_PD_VIDEO {

Powered by blists - more mailing lists