Message-ID: <9e8a12db-236d-474c-b110-b3be96edf057@samsung.com>
Date: Mon, 9 Jun 2025 20:49:57 +0200
From: Michal Wilczynski <m.wilczynski@...sung.com>
To: Drew Fustini <drew@...7.com>
Cc: Uwe Kleine-König <ukleinek@...nel.org>, Miguel Ojeda
	<ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng
	<boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, Benno Lossin
	<benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, Alice
	Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, Danilo
	Krummrich <dakr@...nel.org>, Guo Ren <guoren@...nel.org>, Fu Wei
	<wefu@...hat.com>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
	<krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Paul Walmsley
	<paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>, Albert Ou
	<aou@...s.berkeley.edu>, Alexandre Ghiti <alex@...ti.fr>, Marek Szyprowski
	<m.szyprowski@...sung.com>, linux-kernel@...r.kernel.org,
	linux-pwm@...r.kernel.org, rust-for-linux@...r.kernel.org,
	linux-riscv@...ts.infradead.org, devicetree@...r.kernel.org
Subject: Re: [PATCH RFC 5/6] riscv: dts: thead: Add PVT node



On 6/1/25 19:32, Drew Fustini wrote:
> On Sun, Jun 01, 2025 at 09:50:52AM +0200, Michal Wilczynski wrote:
>>
>>
>> On 5/27/25 10:00, Drew Fustini wrote:
>>> On Sat, May 24, 2025 at 11:14:59PM +0200, Michal Wilczynski wrote:
>>>> Add PVT DT node for thermal sensor.
>>>>
>>>> Signed-off-by: Michal Wilczynski <m.wilczynski@...sung.com>
>>>> ---
>>>>  arch/riscv/boot/dts/thead/th1520.dtsi | 11 +++++++++++
>>>>  1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
>>>> index f24e12d7259fabcfbdc2dfa966d759db06684ab4..faf5c3aaf209b24cd99ddc377a88e08a8cce24fe 100644
>>>> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
>>>> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
>>>> @@ -648,6 +648,17 @@ padctrl_aosys: pinctrl@...ff4a000 {
>>>>  			thead,pad-group = <1>;
>>>>  		};
>>>>  
>>>> +		pvt: pvt@...ff4e000 {
>>>> +			compatible = "moortec,mr75203";
>>>> +			reg = <0xff 0xfff4e000 0x0 0x80>,
>>>> +			      <0xff 0xfff4e080 0x0 0x100>,
>>>> +			      <0xff 0xfff4e180 0x0 0x680>,
>>>> +			      <0xff 0xfff4e800 0x0 0x600>;
>>>> +			reg-names = "common", "ts", "pd", "vm";
>>>> +			clocks = <&aonsys_clk>;
>>>> +			#thermal-sensor-cells = <1>;
>>>> +		};
>>>> +
>>>>  		gpio@...ff52000 {
>>>>  			compatible = "snps,dw-apb-gpio";
>>>>  			reg = <0xff 0xfff52000 0x0 0x1000>;
>>>>
>>>> -- 
>>>> 2.34.1
>>>>
>>>
>>> I found that on my lpi4a that boot while hang after applying this patch.
>>> I think that it is related to clocks as boot finished okay when using
>>> clk_ignore_unused on the kernel cmdline. Do you happen have that in your
>>> kernel cmdline?
>>>
>>> I need to investigate further to understand which clocks are causing the
>>> problem.
>>>
>>> Thanks,
>>> Drew
>>>
>>
>> Thanks for your earlier message. I've investigated, and you were right
>> about the clocks – the specific one causing the hang is CLK_CPU2AON_X2H.
> 
> Thanks for tracking down the clk causing the hang. I can confirm that
> this fixes the boot hang:
> 
> diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
> index ebfb1d59401d..4d0179b8c17c 100644
> --- a/drivers/clk/thead/clk-th1520-ap.c
> +++ b/drivers/clk/thead/clk-th1520-ap.c
> @@ -792,7 +792,7 @@ static CCU_GATE(CLK_AON2CPU_A2X, aon2cpu_a2x_clk, "aon2cpu-a2x", axi4_cpusys2_ac
>                 0x134, BIT(8), 0);
>  static CCU_GATE(CLK_X2X_CPUSYS, x2x_cpusys_clk, "x2x-cpusys", axi4_cpusys2_aclk_pd,
>                 0x134, BIT(7), 0);
> -static CCU_GATE(CLK_CPU2AON_X2H, cpu2aon_x2h_clk, "cpu2aon-x2h", axi_aclk_pd, 0x138, BIT(8), 0);
> +static CCU_GATE(CLK_CPU2AON_X2H, cpu2aon_x2h_clk, "cpu2aon-x2h", axi_aclk_pd, 0x138, BIT(8), CLK_IGNORE_UNUSED);
>  static CCU_GATE(CLK_CPU2PERI_X2H, cpu2peri_x2h_clk, "cpu2peri-x2h", axi4_cpusys2_aclk_pd,
>                 0x140, BIT(9), CLK_IGNORE_UNUSED);
>  static CCU_GATE(CLK_PERISYS_APB1_HCLK, perisys_apb1_hclk, "perisys-apb1-hclk", perisys_ahb_hclk_pd,
> 
>>
>> This appears to be an AHB bus clock required for CPU access to the AON
>> domain. My proposed solution is to make the pvt node a child of a new
>> parent bus node in the Device Tree. This new "AON bus" node would then
>> explicitly request and manage CLK_CPU2AON_X2H, ensuring it's enabled
>> when its children are accessed.
>>
>> What are your thoughts on this approach?
> 
> I think that is a good approach. The alternative would be to just add
> CLK_IGNORE_UNUSED like above. I've done it before but it is a bit of a
> hack.

I've followed up on the idea of creating a parent bus node. My attempt
using simple-pm-bus ran into a couple of significant issues that suggest
it's not the correct path.

First, the TRM doesn't seem to specify an address range for this bus.
The range I used in my test was only for the PVT controller itself,
which would be an incorrect abstraction in the device tree.

Second, simple-pm-bus requires its child nodes to use the PM runtime API
(pm_runtime_resume_and_get, etc.). Forcing this on consumer drivers like
the PVT sensor seems like an inappropriate dependency.

Additionally, I discovered that the PWM driver has a similar problem,
silently failing because another clock, CLK_PERISYS_APB1_HCLK, is not
enabled.

The most correct solution likely involves refactoring the clock parent
relationships in clk-th1520-ap.c. However, as a more immediate and less
invasive fix, I propose we apply the CLK_IGNORE_UNUSED flag for both
CLK_CPU2AON_X2H and CLK_PERISYS_APB1_HCLK in the v2 patch. This will fix
the boot hang and the PWM issue while we consider the larger clock
driver changes separately.

Does that sound like a reasonable plan for the v2 series?

> 
> Thanks,
> Drew
> 

Best regards,
-- 
Michal Wilczynski <m.wilczynski@...sung.com>

Powered by blists - more mailing lists