Re: [PATCH v2 1/2] cpufreq: CPPC: keep target core awake when reading its cpufreq rate

[Zeng Heng <zengheng4@huawei.com>]

在 2023/5/17 16:17, Pierre Gondois 写道:
> +Ionela, Sumit, Yang,
>
> Hello Zeng,
>
> I think solutions around related issues were suggested at:
>
> [1] https://lore.kernel.org/all/20230418113459.12860-7-sumitg@nvidia.com/
> [2] 
> https://lore.kernel.org/all/20230328193846.8757-1-yang@os.amperecomputing.com/
> [3] https://lore.kernel.org/all/ZEl1Fms%2FJmdEZsVn@arm.com/
>
> About this patch, it seems to mean that CPPC counters of CPUx are always
> accessed from CPUx, even when they are not AMUs. For instance CPPC
> counters could be memory mapped and accessible from any CPU.
> cpu_has_amu_feat() should allow to probe if a CPU uses AMUs or not,
> and [2] had an implementation using it.
>
> Another comment about PATCH 2/2 is that if the counters are accessed
> through FFH, arm64 version of cpc_read_ffh() is calling
> counters_read_on_cpu(), and a comment in counters_read_on_cpu() seems
> to specify the function must be called with interrupt enabled.
>
> I think the best solution so far was the one at [3], suggested by Ionela,
> but it doesn't seem to solve your issue. Indeed, it is not checked 
> whether
> the counters are AMU counters and that they must be remotely read (to
> have the CPU awake),
>
> Regards,
> Pierre


Here is segment I picked from patch[3], and there is a modification is 
deserved to be discussed:

---------------------------------------------------------------------------

@@ -1336,8 +1361,25 @@ int cppc_get_perf_ctrs(int cpunum, struct 
cppc_perf_fb_ctrs *perf_fb_ctrs)
          }
      }

-    cpc_read(cpunum, delivered_reg, &delivered);
-    cpc_read(cpunum, reference_reg, &reference);
+    ctrs.cpunum = cpunum;
+    ctrs.delivered_reg = delivered_reg;
+    ctrs.reference_reg = reference_reg;
+    ctrs.delivered = &delivered;
+    ctrs.reference = &reference;
+

+    if (CPC_IN_FFH(delivered_reg) && CPC_IN_FFH(reference_reg)) {

Here we call cpu_has_amu_feat() as precondition (like Sumit's mail 
mentions), which could be compatible with

any SoCs with AMU that could be accessible via sys-register and system 
memory both.


+        ret = smp_call_on_cpu(cpunum, cppc_get_cycle_ctrs, &ctrs, false);
+    } else {
+        if (CPC_IN_SYSTEM_MEMORY(delivered_reg) &&
+            CPC_IN_SYSTEM_MEMORY(reference_reg)) {
+            local_irq_save(flags);
+            cppc_get_cycle_ctrs(&ctrs);
+            local_irq_restore(flags);
+        } else {
+            cppc_get_cycle_ctrs(&ctrs);
+        }
+    }
+
      cpc_read(cpunum, ref_perf_reg, &ref_perf);

-----------------------------------------------------------------------------------

If there were a new version patch released, please loop me at that time.

Thanks a lot in advance.


B.R.,

Zeng Heng