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

["Alexey Klimov" <alexey.klimov@linaro.org>]

On Thu Oct 1, 2026 at 2:27 PM BST, Tudor Ambarus wrote:
>
> 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@linaro.org>
>>>> ---
>>>>  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):

Thanks for the good questions. You hit the exact pain points we have been
debugging on hardware with cpufreq-dt and Panfrost devfreq.

> 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?

Correct. Is it a problem now since there are no "clients" or consumers or
users of these clocks?

Or do you suggest to link them all to parents clocks to have a sensible
rate?

> 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?

This was already asked and seems there are no answers. The problem with
stale rate is present.
https://lore.kernel.org/linux-samsung-soc/DLAZXSNZK6QC.1VFAX37XCXBOV@linaro.org/

Any suggestions? It does seem to be a real problem since we discussed this
again off the list and cpufreq may stuck because of this.

We can cache the ACPM clock set_rate if the call succeeded for instance
(option 2 in the URL above).
I have such patch somewhere locally and Thomas also cooked a version
with caching. Will it be an acceptable approach?

The other thing is that we may need an initial rate set if reading of
a rate happens before first ACPM set rate call.
Is it fine to return parent's clock rate in such case? For this approach
we actually need linking to parent clocks.
Or should we hardcode some boot values in the driver for instance?
We can also potentially hardcode PLL clocks to get one-time boot
(initial values) for ACPM clocks and avoid using parent's clocks
toplogy.


> 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?

I don't know yet what to do with that. I didn't encounter any problems
yet because of this. Is it related to early initialisation only?
I thought that exynos CMU driver doesn't touch PLL clocks after init.

Thanks,
Alexey