Re: [PATCH v5 1/2] dt-bindings: cpufreq: Document support for Airoha EN7581 CPUFreq

[Christian Marangi]

On Wed, Dec 04, 2024 at 12:42:53PM -0600, Rob Herring wrote:
> On Tue, Dec 03, 2024 at 05:31:49PM +0100, Christian Marangi wrote:
> > Document required property for Airoha EN7581 CPUFreq .
> > 
> > On newer Airoha SoC, CPU Frequency is scaled indirectly with SMCCC commands
> > to ATF and no clocks are exposed to the OS.
> > 
> > The SoC have performance state described by ID for each OPP, for this a
> > Power Domain is used that sets the performance state ID according to the
> > required OPPs defined in the CPU OPP tables.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@xxxxxxxxx>
> > Reviewed-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> > ---
> > Changes v5:
> > - Add Reviewed-by tag
> > - Fix OPP node name error
> > - Rename cpufreq node name to power-domain
> > - Rename CPU node power domain name to perf
> > - Add model and compatible to example
> > Changes v4:
> > - Add this patch
> > 
> >  .../cpufreq/airoha,en7581-cpufreq.yaml        | 262 ++++++++++++++++++
> >  1 file changed, 262 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/cpufreq/airoha,en7581-cpufreq.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/cpufreq/airoha,en7581-cpufreq.yaml b/Documentation/devicetree/bindings/cpufreq/airoha,en7581-cpufreq.yaml
> > new file mode 100644
> > index 000000000000..7e36fa037e4b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/cpufreq/airoha,en7581-cpufreq.yaml
> > @@ -0,0 +1,262 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/cpufreq/airoha,en7581-cpufreq.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Airoha EN7581 CPUFreq
> > +
> > +maintainers:
> > +  - Christian Marangi <ansuelsmth@xxxxxxxxx>
> > +
> > +description: |
> > +  On newer Airoha SoC, CPU Frequency is scaled indirectly with SMCCC commands
> > +  to ATF and no clocks are exposed to the OS.
> > +
> > +  The SoC have performance state described by ID for each OPP, for this a
> > +  Power Domain is used that sets the performance state ID according to the
> > +  required OPPs defined in the CPU OPP tables.
> > +
> > +properties:
> > +  compatible:
> > +    const: airoha,en7581-cpufreq
> > +
> > +  '#clock-cells':
> > +    const: 0
> 
> You just said no clocks are exposed to the OS.
>

Well we now simulate one due to request from cpufreq reviewers.

Everything is still handled by SMC that only report the current
frequency of the CPU.

> > +
> > +  '#power-domain-cells':
> > +    const: 0
> > +
> > +  operating-points-v2: true
> > +
> > +required:
> > +  - compatible
> > +  - '#clock-cells'
> > +  - '#power-domain-cells'
> > +  - operating-points-v2
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    / {
> > +        model = "Airoha EN7581 Evaluation Board";
> > +        compatible = "airoha,en7581-evb", "airoha,en7581";
> > +
> > +        #address-cells = <2>;
> > +      	#size-cells = <2>;
> 
> mixed tab and spaces.
> 
> Can't I just go read the actual .dts files if I want to see 
> *everything*? Examples should generally be just what the schema covers.
>

Idea here is to give example as both clock and power-domain property are
needed in the CPU nodes for the CPUFreq driver to correctly work.

Should I drop and just define the CPUFreq node?

> > +
> > +        cpus {
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            cpu0: cpu@0 {
> > +                device_type = "cpu";
> > +                compatible = "arm,cortex-a53";
> > +                reg = <0x0>;
> > +                operating-points-v2 = <&cpu_opp_table>;
> > +                enable-method = "psci";
> > +                clocks = <&cpu_pd>;
> > +                clock-names = "cpu";
> > +                power-domains = <&cpu_pd>;
> > +                power-domain-names = "perf";
> > +                next-level-cache = <&l2>;
> > +                #cooling-cells = <2>;
> 
> I don't understand why you have clocks, power-domains and OPP? 
> Certainly that's conceivable, but not with how you're abusing 
> power-domains for performance points and you said clocks are not exposed 
> to the OS.
>

SMC scale based on index values not frequency. That really resembles a
power-domain. SMC provide frequency in MHz tho so we model that as a
get-only clock.

At times with no clocks are exposed I intend that they SoC doesn't
provide any raw control on them in the normal way with a register, bits
to change and logic to apply for mux and divisor, this thing is very
special and works only with 2 command and nothing else so I'm trying my
best to model this in the most descriptive and complete way possible.

-- 
	Ansuel