Re: [PATCH 1/2] arm64: dts: exynos850: Add ACPM firmware node

[Tudor Ambarus]

On 9/30/26 2:54 PM, Alexey Klimov wrote:
> Hi Tudor,
> 
> On Tue Sep 29, 2026 at 10:02 AM BST, Tudor Ambarus wrote:
>> Hi, Alexey,
>> On 9/29/26 6:10 AM, Alexey Klimov wrote:
>>> Add the ACPM firmware node with cpucl0 and cpucl1 clocks. ACPM firmware
>>> protocol provides interface for all client drivers to actually use
>>> features exposed by the APM co-processor.
>>>
>>> Signed-off-by: Alexey Klimov <alexey.klimov@xxxxxxxxxx>
>>> ---
>>>  arch/arm64/boot/dts/exynos/exynos850.dtsi | 13 +++++++++++++
>>>  1 file changed, 13 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/exynos/exynos850.dtsi b/arch/arm64/boot/dts/exynos/exynos850.dtsi
>>> index 8a4771899a8e..91c6cee8f483 100644
>>> --- a/arch/arm64/boot/dts/exynos/exynos850.dtsi
>>> +++ b/arch/arm64/boot/dts/exynos/exynos850.dtsi
>>> @@ -11,6 +11,7 @@
>>>   */
>>>  
>>>  #include <dt-bindings/clock/exynos850.h>
>>> +#include <dt-bindings/clock/samsung,exynos850-acpm.h>
>>>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>>>  #include <dt-bindings/soc/samsung,exynos-usi.h>
>>>  
>>> @@ -170,6 +171,18 @@ timer: timer {
>>>  		     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>;
>>>  	};
>>>  
>>> +	firmware {
>>> +		acpm_ipc: power-management {
>>> +			compatible = "samsung,exynos850-acpm-ipc";
>>> +			mboxes = <&ap2apm_mailbox>;
>>> +			shmem = <&apm_sram>;
>>> +			clocks = <&cmu_cpucl0 CLK_FOUT_CPUCL0_PLL>,
>>> +				 <&cmu_cpucl1 CLK_FOUT_CPUCL1_PLL>;
>>
>> Why do you describe these clocks?
> 
>>> +			clock-names = "cpucl0", "cpucl1";
> 
> To link these clocks with ACPM clocks from this list:
> (file drivers/clk/samsung/clk-acpm.c)
> 
> static const struct acpm_clk_variant exynos850_acpm_clks[] = {
> 	ACPM_CLK("mif"),
> 	ACPM_CLK("int"),
> 	ACPM_CLK("cpucl0"),
> 	ACPM_CLK("cpucl1"),
> 	ACPM_CLK("g3d"),
> 	ACPM_CLK("aud"),
> 	ACPM_CLK("cam"),
> 	ACPM_CLK("disp"),
> 	ACPM_CLK("cp"),
> };
> 
> Eventually to have some sensible/working ->recalc_rate() for ACPM
> cpucl{0,1} clocks.
> 
> Which is needed, for instance, for cpufreq_dt because it registers with:
> 
> static struct cpufreq_driver dt_cpufreq_driver = {
>         .flags = CPUFREQ_NEED_INITIAL_FREQ_CHECK |
>                  CPUFREQ_IS_COOLING_DEV,
> 
> Don't know if it answers the question (if I understood it correctly)?
> 

Thanks, Alexey. I see now that clk-acpm.c uses pdata.fw_name on the
parent (acpm_ipc) DT node to link the ACPM clocks to the CMU PLLs for
->recalc_rate(). Could you please clarify a few things in the commit
message (and below):

1. Physically, CLK_FOUT_CPUCL{0,1}_PLL are not input clocks feeding the
   APM hardware, they are the PLLs that ACPM reconfigures behind the
   scenes. Meanwhile, acpm_clk_register() sets num_parents = 1 and
   pdata.fw_name = name for all 9 clocks in exynos850_acpm_clks[].
   Since the DT node only lists "cpucl0" and "cpucl1", won't the other
   7 ACPM clocks (mif, int, g3d, aud, cam, disp, cp) remain permanently
   orphaned in CCF with rate = 0?

2. Does bypass_acpm_exynos850_get_rate() actually return the updated rate
   after clk_set_rate()? When ACPM changes the PLL frequency via IPC,
   CCF is unaware that the parent clock (fout_cpucl0_pll) changed in
   hardware. Even with CLK_GET_RATE_NOCACHE on the ACPM clock,
   __clk_recalc_rates() only reads the cached core->parent->rate without
   calling ->recalc_rate() on the parent itself. Won't parent_rate remain
   stuck at the initial boot frequency?

3. In drivers/clk/samsung/clk-exynos850.c, cpucl0_cmu_info and
   cpucl1_cmu_info still set `.manual_plls = true`.
   Does that conflict with ACPM firmware managing the CPU PLLs?

Cheers,
ta