diff mbox series

[v4,1/2] dt-bindings: cpufreq: Document support for Airoha EN7581 CPUFreq

Message ID 20241202151228.32609-1-ansuelsmth@gmail.com
State New
Headers show
Series [v4,1/2] dt-bindings: cpufreq: Document support for Airoha EN7581 CPUFreq | expand

Commit Message

Christian Marangi Dec. 2, 2024, 3:12 p.m. UTC
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@gmail.com>
---
Changes v4:
- Add this patch

 .../cpufreq/airoha,en7581-cpufreq.yaml        | 259 ++++++++++++++++++
 1 file changed, 259 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/airoha,en7581-cpufreq.yaml

Comments

Christian Marangi Dec. 2, 2024, 8:59 p.m. UTC | #1
On Mon, Dec 02, 2024 at 02:57:38PM -0600, Rob Herring wrote:
> On Mon, Dec 02, 2024 at 04:45:17PM +0100, Christian Marangi wrote:
> > On Mon, Dec 02, 2024 at 04:42:33PM +0100, Ulf Hansson wrote:
> > > On Mon, 2 Dec 2024 at 16:20, Christian Marangi <ansuelsmth@gmail.com> 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@gmail.com>
> > > > ---
> > > > Changes v4:
> > > > - Add this patch
> > > >
> > > >  .../cpufreq/airoha,en7581-cpufreq.yaml        | 259 ++++++++++++++++++
> > > >  1 file changed, 259 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..a5bdea7f34b5
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/cpufreq/airoha,en7581-cpufreq.yaml
> > > 
> > > [...]
> > > 
> > > > +examples:
> > > > +  - |
> > > > +    / {
> > > > +        #address-cells = <2>;
> > > > +       #size-cells = <2>;
> > > > +
> > > > +        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 = <&cpufreq>;
> > > > +                clock-names = "cpu";
> > > > +                power-domains = <&cpufreq>;
> > > > +                power-domain-names = "cpu_pd";
> > > 
> > > Nitpick: Perhaps clarify the name to be "perf" or "cpu_perf", to
> > > indicate it's a power-domain with performance scaling support.
> > > 
> > 
> > Will change to cpu_perf. Thanks a lot for the review!
> 
> Is that defined in arm/cpus.yaml? No.
> 
> The current choices are perf or psci though those aren't enforced (yet). 
> Or nothing which is my preference if there is only 1 power domain.
> 

I would also prefer not having to define the names property but I guess
that is mandatory for the PD APIs? Maybe Ulf can confirm.
Viresh Kumar Dec. 3, 2024, 4:41 a.m. UTC | #2
+Ulf,

On 02-12-24, 16:12, 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@gmail.com>
> ---
> Changes v4:
> - Add this patch
> 
>  .../cpufreq/airoha,en7581-cpufreq.yaml        | 259 ++++++++++++++++++
>  1 file changed, 259 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..a5bdea7f34b5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/airoha,en7581-cpufreq.yaml
> @@ -0,0 +1,259 @@
> +# 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@gmail.com>
> +
> +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
> +
> +  '#power-domain-cells':
> +    const: 0
> +
> +  operating-points-v2: true
> +
> +required:
> +  - compatible
> +  - '#clock-cells'
> +  - '#power-domain-cells'
> +  - operating-points-v2
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    / {
> +        #address-cells = <2>;
> +      	#size-cells = <2>;
> +
> +        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 = <&cpufreq>;
> +                clock-names = "cpu";
> +                power-domains = <&cpufreq>;
> +                power-domain-names = "cpu_pd";
> +                next-level-cache = <&l2>;
> +                #cooling-cells = <2>;
> +            };
> +
> +            cpu1: cpu@1 {
> +                device_type = "cpu";
> +                compatible = "arm,cortex-a53";
> +                reg = <0x1>;
> +                operating-points-v2 = <&cpu_opp_table>;
> +                enable-method = "psci";
> +                clocks = <&cpufreq>;
> +                clock-names = "cpu";
> +                power-domains = <&cpufreq>;
> +                power-domain-names = "cpu_pd";
> +                next-level-cache = <&l2>;
> +                #cooling-cells = <2>;
> +            };
> +
> +            cpu2: cpu@2 {
> +                device_type = "cpu";
> +                compatible = "arm,cortex-a53";
> +                reg = <0x2>;
> +                operating-points-v2 = <&cpu_opp_table>;
> +                enable-method = "psci";
> +                clocks = <&cpufreq>;
> +                clock-names = "cpu";
> +                power-domains = <&cpufreq>;
> +                power-domain-names = "cpu_pd";
> +                next-level-cache = <&l2>;
> +                #cooling-cells = <2>;
> +            };
> +
> +            cpu3: cpu@3 {
> +                device_type = "cpu";
> +                compatible = "arm,cortex-a53";
> +                reg = <0x3>;
> +                operating-points-v2 = <&cpu_opp_table>;
> +                enable-method = "psci";
> +                clocks = <&cpufreq>;
> +                clock-names = "cpu";
> +                power-domains = <&cpufreq>;
> +                power-domain-names = "cpu_pd";
> +                next-level-cache = <&l2>;
> +                #cooling-cells = <2>;
> +            };
> +        };
> +
> +        cpu_opp_table: opp-table {
> +            compatible = "operating-points-v2";
> +            opp-shared;
> +
> +            opp-500000000 {
> +                opp-hz = /bits/ 64 <500000000>;
> +                required-opps = <&smcc_opp0>;

In your case I think you can simply mention opp-level here itself and remove the
OPP table for the genpd. Right Ulf ?

> +            };
> +
> +            opp-550000000 {
> +                opp-hz = /bits/ 64 <550000000>;
> +                required-opps = <&smcc_opp1>;
> +            };
> +
> +            opp-600000000 {
> +                opp-hz = /bits/ 64 <600000000>;
> +                required-opps = <&smcc_opp2>;
> +            };
> +
> +            opp-650000000 {
> +                opp-hz = /bits/ 64 <650000000>;
> +                required-opps = <&smcc_opp3>;
> +            };
> +
> +            opp-7000000000 {
> +                opp-hz = /bits/ 64 <700000000>;
> +                required-opps = <&smcc_opp4>;
> +            };
> +
> +            opp-7500000000 {
> +                opp-hz = /bits/ 64 <750000000>;
> +                required-opps = <&smcc_opp5>;
> +            };
> +
> +            opp-8000000000 {
> +                opp-hz = /bits/ 64 <800000000>;
> +                required-opps = <&smcc_opp6>;
> +            };
> +
> +            opp-8500000000 {
> +                opp-hz = /bits/ 64 <850000000>;
> +                required-opps = <&smcc_opp7>;
> +            };
> +
> +            opp-9000000000 {
> +                opp-hz = /bits/ 64 <900000000>;
> +                required-opps = <&smcc_opp8>;
> +            };
> +
> +            opp-9500000000 {
> +                opp-hz = /bits/ 64 <950000000>;
> +                required-opps = <&smcc_opp9>;
> +            };
> +
> +            opp-10000000000 {
> +                opp-hz = /bits/ 64 <1000000000>;
> +                required-opps = <&smcc_opp10>;
> +            };
> +
> +            opp-10500000000 {
> +                opp-hz = /bits/ 64 <1050000000>;
> +                required-opps = <&smcc_opp11>;
> +            };
> +
> +            opp-11000000000 {
> +                opp-hz = /bits/ 64 <1100000000>;
> +                required-opps = <&smcc_opp12>;
> +            };
> +
> +            opp-11500000000 {
> +                opp-hz = /bits/ 64 <1150000000>;
> +                required-opps = <&smcc_opp13>;
> +            };
> +
> +            opp-12000000000 {
> +                opp-hz = /bits/ 64 <1200000000>;
> +                required-opps = <&smcc_opp14>;
> +            };
> +        };
> +
> +        cpu_smcc_opp_table: opp-table-cpu-smcc {

So this won't be required I guess.

> +            compatible = "operating-points-v2";
> +
> +            smcc_opp0: opp0 {
> +               opp-level = <0>;
> +            };
> +
> +            smcc_opp1: opp1 {
> +                opp-level = <1>;
> +            };
> +
> +            smcc_opp2: opp2 {
> +               opp-level = <2>;
> +            };
> +
> +            smcc_opp3: opp3 {
> +               opp-level = <3>;
> +            };
> +
> +            smcc_opp4: opp4 {
> +                opp-level = <4>;
> +            };
> +
> +            smcc_opp5: opp5 {
> +                opp-level = <5>;
> +            };
> +
> +            smcc_opp6: opp6 {
> +               opp-level = <6>;
> +            };
> +
> +            smcc_opp7: opp7 {
> +               opp-level = <7>;
> +            };
> +
> +            smcc_opp8: opp8 {
> +                opp-level = <8>;
> +            };
> +
> +            smcc_opp9: opp9 {
> +               opp-level = <9>;
> +            };
> +
> +            smcc_opp10: opp10 {
> +                opp-level = <10>;
> +            };
> +
> +            smcc_opp11: opp11 {
> +                opp-level = <11>;
> +            };
> +
> +            smcc_opp12: opp12 {
> +                opp-level = <12>;
> +            };
> +
> +            smcc_opp13: opp13 {
> +                opp-level = <13>;
> +            };
> +
> +            smcc_opp14: opp14 {
> +                opp-level = <14>;
> +            };
> +        };
> +
> +        cpufreq: cpufreq {

And I would name it like a genpd, instead of cpufreq. But I am not sure what's
the right name is..

> +            compatible = "airoha,en7581-cpufreq";
> +
> +            operating-points-v2 = <&cpu_smcc_opp_table>;
> +
> +            #power-domain-cells = <0>;
> +            #clock-cells = <0>;
> +        };
> +    };
> -- 
> 2.45.2
Uwe Kleine-König Dec. 3, 2024, 7:44 a.m. UTC | #3
Hello,

On Mon, Dec 02, 2024 at 09:47:58PM +0100, Christian Marangi wrote:
> On Mon, Dec 02, 2024 at 09:38:10PM +0100, Uwe Kleine-König wrote:
> > hello,
> > 
> > On Mon, Dec 02, 2024 at 04:12:04PM +0100, Christian Marangi wrote:
> > > +static struct platform_driver airoha_cpufreq_driver = {
> > > +	.probe = airoha_cpufreq_probe,
> > > +	.remove_new = airoha_cpufreq_remove,
> > 
> > That doesn't build on top of mainline since commit e70140ba0d2b ("Get rid of 'remove_new' relic from platform driver struct")
> > 
> > Please use .remove.
> 
> Hi Uwe,
> I rebased this on top of next and still compiled but yes sorry it's a
> leftover from testing this also on my OpenWrt buildroot. (seems the
> commit still needs to be merged)

The relevant commits are:

 - Since v6.3-rc1~106^2~108 ("platform: Provide a remove callback that
   returns no value") there is .remove_new()
 - Since v6.11-rc1~49^2~37^2 ("platform: Make platform_driver::remove()
   return void") the prototype of .remove() changed to return void
 - Since e70140ba0d2b ("Get rid of 'remove_new' relic from platform
   driver struct") (which is currently only in Linus Torvalds repo and
   probably in next starting today) .remove_new is gone.

Best regards
Uwe
Ulf Hansson Dec. 3, 2024, 10:22 a.m. UTC | #4
On Tue, 3 Dec 2024 at 05:41, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> +Ulf,
>
> On 02-12-24, 16:12, 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@gmail.com>
> > ---
> > Changes v4:
> > - Add this patch
> >
> >  .../cpufreq/airoha,en7581-cpufreq.yaml        | 259 ++++++++++++++++++
> >  1 file changed, 259 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..a5bdea7f34b5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/cpufreq/airoha,en7581-cpufreq.yaml
> > @@ -0,0 +1,259 @@
> > +# 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@gmail.com>
> > +
> > +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
> > +
> > +  '#power-domain-cells':
> > +    const: 0
> > +
> > +  operating-points-v2: true
> > +
> > +required:
> > +  - compatible
> > +  - '#clock-cells'
> > +  - '#power-domain-cells'
> > +  - operating-points-v2
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    / {
> > +        #address-cells = <2>;
> > +             #size-cells = <2>;
> > +
> > +        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 = <&cpufreq>;
> > +                clock-names = "cpu";
> > +                power-domains = <&cpufreq>;
> > +                power-domain-names = "cpu_pd";
> > +                next-level-cache = <&l2>;
> > +                #cooling-cells = <2>;
> > +            };
> > +
> > +            cpu1: cpu@1 {
> > +                device_type = "cpu";
> > +                compatible = "arm,cortex-a53";
> > +                reg = <0x1>;
> > +                operating-points-v2 = <&cpu_opp_table>;
> > +                enable-method = "psci";
> > +                clocks = <&cpufreq>;
> > +                clock-names = "cpu";
> > +                power-domains = <&cpufreq>;
> > +                power-domain-names = "cpu_pd";
> > +                next-level-cache = <&l2>;
> > +                #cooling-cells = <2>;
> > +            };
> > +
> > +            cpu2: cpu@2 {
> > +                device_type = "cpu";
> > +                compatible = "arm,cortex-a53";
> > +                reg = <0x2>;
> > +                operating-points-v2 = <&cpu_opp_table>;
> > +                enable-method = "psci";
> > +                clocks = <&cpufreq>;
> > +                clock-names = "cpu";
> > +                power-domains = <&cpufreq>;
> > +                power-domain-names = "cpu_pd";
> > +                next-level-cache = <&l2>;
> > +                #cooling-cells = <2>;
> > +            };
> > +
> > +            cpu3: cpu@3 {
> > +                device_type = "cpu";
> > +                compatible = "arm,cortex-a53";
> > +                reg = <0x3>;
> > +                operating-points-v2 = <&cpu_opp_table>;
> > +                enable-method = "psci";
> > +                clocks = <&cpufreq>;
> > +                clock-names = "cpu";
> > +                power-domains = <&cpufreq>;
> > +                power-domain-names = "cpu_pd";
> > +                next-level-cache = <&l2>;
> > +                #cooling-cells = <2>;
> > +            };
> > +        };
> > +
> > +        cpu_opp_table: opp-table {
> > +            compatible = "operating-points-v2";
> > +            opp-shared;
> > +
> > +            opp-500000000 {
> > +                opp-hz = /bits/ 64 <500000000>;
> > +                required-opps = <&smcc_opp0>;
>
> In your case I think you can simply mention opp-level here itself and remove the
> OPP table for the genpd. Right Ulf ?

No, that would not be the correct thing to do, I think.

The power-domain described here is modeling a performance-domain,
hence it needs an opp-table associated with it.

>
> > +            };
> > +
> > +            opp-550000000 {
> > +                opp-hz = /bits/ 64 <550000000>;
> > +                required-opps = <&smcc_opp1>;
> > +            };
> > +
> > +            opp-600000000 {
> > +                opp-hz = /bits/ 64 <600000000>;
> > +                required-opps = <&smcc_opp2>;
> > +            };
> > +
> > +            opp-650000000 {
> > +                opp-hz = /bits/ 64 <650000000>;
> > +                required-opps = <&smcc_opp3>;
> > +            };
> > +
> > +            opp-7000000000 {
> > +                opp-hz = /bits/ 64 <700000000>;
> > +                required-opps = <&smcc_opp4>;
> > +            };
> > +
> > +            opp-7500000000 {
> > +                opp-hz = /bits/ 64 <750000000>;
> > +                required-opps = <&smcc_opp5>;
> > +            };
> > +
> > +            opp-8000000000 {
> > +                opp-hz = /bits/ 64 <800000000>;
> > +                required-opps = <&smcc_opp6>;
> > +            };
> > +
> > +            opp-8500000000 {
> > +                opp-hz = /bits/ 64 <850000000>;
> > +                required-opps = <&smcc_opp7>;
> > +            };
> > +
> > +            opp-9000000000 {
> > +                opp-hz = /bits/ 64 <900000000>;
> > +                required-opps = <&smcc_opp8>;
> > +            };
> > +
> > +            opp-9500000000 {
> > +                opp-hz = /bits/ 64 <950000000>;
> > +                required-opps = <&smcc_opp9>;
> > +            };
> > +
> > +            opp-10000000000 {
> > +                opp-hz = /bits/ 64 <1000000000>;
> > +                required-opps = <&smcc_opp10>;
> > +            };
> > +
> > +            opp-10500000000 {
> > +                opp-hz = /bits/ 64 <1050000000>;
> > +                required-opps = <&smcc_opp11>;
> > +            };
> > +
> > +            opp-11000000000 {
> > +                opp-hz = /bits/ 64 <1100000000>;
> > +                required-opps = <&smcc_opp12>;
> > +            };
> > +
> > +            opp-11500000000 {
> > +                opp-hz = /bits/ 64 <1150000000>;
> > +                required-opps = <&smcc_opp13>;
> > +            };
> > +
> > +            opp-12000000000 {
> > +                opp-hz = /bits/ 64 <1200000000>;
> > +                required-opps = <&smcc_opp14>;
> > +            };
> > +        };
> > +
> > +        cpu_smcc_opp_table: opp-table-cpu-smcc {
>
> So this won't be required I guess.

See above, I think the OPP table here is the correct way to describe it.

>
> > +            compatible = "operating-points-v2";
> > +
> > +            smcc_opp0: opp0 {
> > +               opp-level = <0>;
> > +            };
> > +
> > +            smcc_opp1: opp1 {
> > +                opp-level = <1>;
> > +            };
> > +
> > +            smcc_opp2: opp2 {
> > +               opp-level = <2>;
> > +            };
> > +
> > +            smcc_opp3: opp3 {
> > +               opp-level = <3>;
> > +            };
> > +
> > +            smcc_opp4: opp4 {
> > +                opp-level = <4>;
> > +            };
> > +
> > +            smcc_opp5: opp5 {
> > +                opp-level = <5>;
> > +            };
> > +
> > +            smcc_opp6: opp6 {
> > +               opp-level = <6>;
> > +            };
> > +
> > +            smcc_opp7: opp7 {
> > +               opp-level = <7>;
> > +            };
> > +
> > +            smcc_opp8: opp8 {
> > +                opp-level = <8>;
> > +            };
> > +
> > +            smcc_opp9: opp9 {
> > +               opp-level = <9>;
> > +            };
> > +
> > +            smcc_opp10: opp10 {
> > +                opp-level = <10>;
> > +            };
> > +
> > +            smcc_opp11: opp11 {
> > +                opp-level = <11>;
> > +            };
> > +
> > +            smcc_opp12: opp12 {
> > +                opp-level = <12>;
> > +            };
> > +
> > +            smcc_opp13: opp13 {
> > +                opp-level = <13>;
> > +            };
> > +
> > +            smcc_opp14: opp14 {
> > +                opp-level = <14>;
> > +            };
> > +        };
> > +
> > +        cpufreq: cpufreq {
>
> And I would name it like a genpd, instead of cpufreq. But I am not sure what's
> the right name is..

Good point!

The node name should follow
Documentation/devicetree/bindings/power/power-domain.yaml, which
states:

pattern: "^(power-controller|power-domain|performance-domain)([@-].*)?$"

>
> > +            compatible = "airoha,en7581-cpufreq";
> > +
> > +            operating-points-v2 = <&cpu_smcc_opp_table>;
> > +
> > +            #power-domain-cells = <0>;
> > +            #clock-cells = <0>;
> > +        };
> > +    };
> > --
> > 2.45.2
>

Kind regards
Uffe
Ulf Hansson Dec. 3, 2024, 10:33 a.m. UTC | #5
On Mon, 2 Dec 2024 at 21:57, Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Dec 02, 2024 at 04:45:17PM +0100, Christian Marangi wrote:
> > On Mon, Dec 02, 2024 at 04:42:33PM +0100, Ulf Hansson wrote:
> > > On Mon, 2 Dec 2024 at 16:20, Christian Marangi <ansuelsmth@gmail.com> 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@gmail.com>
> > > > ---
> > > > Changes v4:
> > > > - Add this patch
> > > >
> > > >  .../cpufreq/airoha,en7581-cpufreq.yaml        | 259 ++++++++++++++++++
> > > >  1 file changed, 259 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..a5bdea7f34b5
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/cpufreq/airoha,en7581-cpufreq.yaml
> > >
> > > [...]
> > >
> > > > +examples:
> > > > +  - |
> > > > +    / {
> > > > +        #address-cells = <2>;
> > > > +       #size-cells = <2>;
> > > > +
> > > > +        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 = <&cpufreq>;
> > > > +                clock-names = "cpu";
> > > > +                power-domains = <&cpufreq>;
> > > > +                power-domain-names = "cpu_pd";
> > >
> > > Nitpick: Perhaps clarify the name to be "perf" or "cpu_perf", to
> > > indicate it's a power-domain with performance scaling support.
> > >
> >
> > Will change to cpu_perf. Thanks a lot for the review!
>
> Is that defined in arm/cpus.yaml? No.
>
> The current choices are perf or psci though those aren't enforced (yet).
> Or nothing which is my preference if there is only 1 power domain.

Right. It's not really clear in arm/cpus.yaml what name to use for a
perf domain, except for "perf" for SCMI.

If we want to move towards some alignment, perhaps we should update
the DT doc to make "perf" the common suggestion? I can send a patch if
you think it makes sense?

Even if there is only 1 power-domain at this point, we never know if
another one turns up later, for whatever reasons. That said, isn't it
better to be specific about a name, already at this point?

Kind regards
Uffe
Ulf Hansson Dec. 3, 2024, 10:36 a.m. UTC | #6
On Mon, 2 Dec 2024 at 21:59, Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> On Mon, Dec 02, 2024 at 02:57:38PM -0600, Rob Herring wrote:
> > On Mon, Dec 02, 2024 at 04:45:17PM +0100, Christian Marangi wrote:
> > > On Mon, Dec 02, 2024 at 04:42:33PM +0100, Ulf Hansson wrote:
> > > > On Mon, 2 Dec 2024 at 16:20, Christian Marangi <ansuelsmth@gmail.com> 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@gmail.com>
> > > > > ---
> > > > > Changes v4:
> > > > > - Add this patch
> > > > >
> > > > >  .../cpufreq/airoha,en7581-cpufreq.yaml        | 259 ++++++++++++++++++
> > > > >  1 file changed, 259 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..a5bdea7f34b5
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/cpufreq/airoha,en7581-cpufreq.yaml
> > > >
> > > > [...]
> > > >
> > > > > +examples:
> > > > > +  - |
> > > > > +    / {
> > > > > +        #address-cells = <2>;
> > > > > +       #size-cells = <2>;
> > > > > +
> > > > > +        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 = <&cpufreq>;
> > > > > +                clock-names = "cpu";
> > > > > +                power-domains = <&cpufreq>;
> > > > > +                power-domain-names = "cpu_pd";
> > > >
> > > > Nitpick: Perhaps clarify the name to be "perf" or "cpu_perf", to
> > > > indicate it's a power-domain with performance scaling support.
> > > >
> > >
> > > Will change to cpu_perf. Thanks a lot for the review!
> >
> > Is that defined in arm/cpus.yaml? No.
> >
> > The current choices are perf or psci though those aren't enforced (yet).
> > Or nothing which is my preference if there is only 1 power domain.
> >
>
> I would also prefer not having to define the names property but I guess
> that is mandatory for the PD APIs? Maybe Ulf can confirm.

No, that's not needed.

dev_pm_domain_attach() attaches a single PM domain, without requiring a name.

But as stated in the other thread, to be future proof, I would suggest
using a name already at this point. Even if it's not required.

Kind regards
Uffe
Christian Marangi Dec. 3, 2024, 10:47 a.m. UTC | #7
On Tue, Dec 03, 2024 at 11:33:28AM +0100, Ulf Hansson wrote:
> On Mon, 2 Dec 2024 at 21:57, Rob Herring <robh@kernel.org> wrote:
> >
> > On Mon, Dec 02, 2024 at 04:45:17PM +0100, Christian Marangi wrote:
> > > On Mon, Dec 02, 2024 at 04:42:33PM +0100, Ulf Hansson wrote:
> > > > On Mon, 2 Dec 2024 at 16:20, Christian Marangi <ansuelsmth@gmail.com> 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@gmail.com>
> > > > > ---
> > > > > Changes v4:
> > > > > - Add this patch
> > > > >
> > > > >  .../cpufreq/airoha,en7581-cpufreq.yaml        | 259 ++++++++++++++++++
> > > > >  1 file changed, 259 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..a5bdea7f34b5
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/cpufreq/airoha,en7581-cpufreq.yaml
> > > >
> > > > [...]
> > > >
> > > > > +examples:
> > > > > +  - |
> > > > > +    / {
> > > > > +        #address-cells = <2>;
> > > > > +       #size-cells = <2>;
> > > > > +
> > > > > +        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 = <&cpufreq>;
> > > > > +                clock-names = "cpu";
> > > > > +                power-domains = <&cpufreq>;
> > > > > +                power-domain-names = "cpu_pd";
> > > >
> > > > Nitpick: Perhaps clarify the name to be "perf" or "cpu_perf", to
> > > > indicate it's a power-domain with performance scaling support.
> > > >
> > >
> > > Will change to cpu_perf. Thanks a lot for the review!
> >
> > Is that defined in arm/cpus.yaml? No.
> >
> > The current choices are perf or psci though those aren't enforced (yet).
> > Or nothing which is my preference if there is only 1 power domain.
> 
> Right. It's not really clear in arm/cpus.yaml what name to use for a
> perf domain, except for "perf" for SCMI.
> 
> If we want to move towards some alignment, perhaps we should update
> the DT doc to make "perf" the common suggestion? I can send a patch if
> you think it makes sense?
> 
> Even if there is only 1 power-domain at this point, we never know if
> another one turns up later, for whatever reasons. That said, isn't it
> better to be specific about a name, already at this point?
>

Ok to summarize

- cpu node: use perf for PD
- cpufreq node change node name to power-domain
- Fix OPP format (already done)

Did I miss anything? It's strange these case weren't catch by
dt_binding_check. Sorry, wasn't aware of all these common name.
Viresh Kumar Dec. 3, 2024, 10:50 a.m. UTC | #8
On 03-12-24, 11:47, Christian Marangi wrote:
> Ok to summarize
> 
> - cpu node: use perf for PD
> - cpufreq node change node name to power-domain
> - Fix OPP format (already done)
> 
> Did I miss anything? It's strange these case weren't catch by
> dt_binding_check. Sorry, wasn't aware of all these common name.

Please also run "scripts/checkpatch.pl --strict" if you haven't already, I think
I saw some inconsistencies in the second patch.
diff mbox series

Patch

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..a5bdea7f34b5
--- /dev/null
+++ b/Documentation/devicetree/bindings/cpufreq/airoha,en7581-cpufreq.yaml
@@ -0,0 +1,259 @@ 
+# 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@gmail.com>
+
+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
+
+  '#power-domain-cells':
+    const: 0
+
+  operating-points-v2: true
+
+required:
+  - compatible
+  - '#clock-cells'
+  - '#power-domain-cells'
+  - operating-points-v2
+
+additionalProperties: false
+
+examples:
+  - |
+    / {
+        #address-cells = <2>;
+      	#size-cells = <2>;
+
+        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 = <&cpufreq>;
+                clock-names = "cpu";
+                power-domains = <&cpufreq>;
+                power-domain-names = "cpu_pd";
+                next-level-cache = <&l2>;
+                #cooling-cells = <2>;
+            };
+
+            cpu1: cpu@1 {
+                device_type = "cpu";
+                compatible = "arm,cortex-a53";
+                reg = <0x1>;
+                operating-points-v2 = <&cpu_opp_table>;
+                enable-method = "psci";
+                clocks = <&cpufreq>;
+                clock-names = "cpu";
+                power-domains = <&cpufreq>;
+                power-domain-names = "cpu_pd";
+                next-level-cache = <&l2>;
+                #cooling-cells = <2>;
+            };
+
+            cpu2: cpu@2 {
+                device_type = "cpu";
+                compatible = "arm,cortex-a53";
+                reg = <0x2>;
+                operating-points-v2 = <&cpu_opp_table>;
+                enable-method = "psci";
+                clocks = <&cpufreq>;
+                clock-names = "cpu";
+                power-domains = <&cpufreq>;
+                power-domain-names = "cpu_pd";
+                next-level-cache = <&l2>;
+                #cooling-cells = <2>;
+            };
+
+            cpu3: cpu@3 {
+                device_type = "cpu";
+                compatible = "arm,cortex-a53";
+                reg = <0x3>;
+                operating-points-v2 = <&cpu_opp_table>;
+                enable-method = "psci";
+                clocks = <&cpufreq>;
+                clock-names = "cpu";
+                power-domains = <&cpufreq>;
+                power-domain-names = "cpu_pd";
+                next-level-cache = <&l2>;
+                #cooling-cells = <2>;
+            };
+        };
+
+        cpu_opp_table: opp-table {
+            compatible = "operating-points-v2";
+            opp-shared;
+
+            opp-500000000 {
+                opp-hz = /bits/ 64 <500000000>;
+                required-opps = <&smcc_opp0>;
+            };
+
+            opp-550000000 {
+                opp-hz = /bits/ 64 <550000000>;
+                required-opps = <&smcc_opp1>;
+            };
+
+            opp-600000000 {
+                opp-hz = /bits/ 64 <600000000>;
+                required-opps = <&smcc_opp2>;
+            };
+
+            opp-650000000 {
+                opp-hz = /bits/ 64 <650000000>;
+                required-opps = <&smcc_opp3>;
+            };
+
+            opp-7000000000 {
+                opp-hz = /bits/ 64 <700000000>;
+                required-opps = <&smcc_opp4>;
+            };
+
+            opp-7500000000 {
+                opp-hz = /bits/ 64 <750000000>;
+                required-opps = <&smcc_opp5>;
+            };
+
+            opp-8000000000 {
+                opp-hz = /bits/ 64 <800000000>;
+                required-opps = <&smcc_opp6>;
+            };
+
+            opp-8500000000 {
+                opp-hz = /bits/ 64 <850000000>;
+                required-opps = <&smcc_opp7>;
+            };
+
+            opp-9000000000 {
+                opp-hz = /bits/ 64 <900000000>;
+                required-opps = <&smcc_opp8>;
+            };
+
+            opp-9500000000 {
+                opp-hz = /bits/ 64 <950000000>;
+                required-opps = <&smcc_opp9>;
+            };
+
+            opp-10000000000 {
+                opp-hz = /bits/ 64 <1000000000>;
+                required-opps = <&smcc_opp10>;
+            };
+
+            opp-10500000000 {
+                opp-hz = /bits/ 64 <1050000000>;
+                required-opps = <&smcc_opp11>;
+            };
+
+            opp-11000000000 {
+                opp-hz = /bits/ 64 <1100000000>;
+                required-opps = <&smcc_opp12>;
+            };
+
+            opp-11500000000 {
+                opp-hz = /bits/ 64 <1150000000>;
+                required-opps = <&smcc_opp13>;
+            };
+
+            opp-12000000000 {
+                opp-hz = /bits/ 64 <1200000000>;
+                required-opps = <&smcc_opp14>;
+            };
+        };
+
+        cpu_smcc_opp_table: opp-table-cpu-smcc {
+            compatible = "operating-points-v2";
+
+            smcc_opp0: opp0 {
+               opp-level = <0>;
+            };
+
+            smcc_opp1: opp1 {
+                opp-level = <1>;
+            };
+
+            smcc_opp2: opp2 {
+               opp-level = <2>;
+            };
+
+            smcc_opp3: opp3 {
+               opp-level = <3>;
+            };
+
+            smcc_opp4: opp4 {
+                opp-level = <4>;
+            };
+
+            smcc_opp5: opp5 {
+                opp-level = <5>;
+            };
+
+            smcc_opp6: opp6 {
+               opp-level = <6>;
+            };
+
+            smcc_opp7: opp7 {
+               opp-level = <7>;
+            };
+
+            smcc_opp8: opp8 {
+                opp-level = <8>;
+            };
+
+            smcc_opp9: opp9 {
+               opp-level = <9>;
+            };
+
+            smcc_opp10: opp10 {
+                opp-level = <10>;
+            };
+
+            smcc_opp11: opp11 {
+                opp-level = <11>;
+            };
+
+            smcc_opp12: opp12 {
+                opp-level = <12>;
+            };
+
+            smcc_opp13: opp13 {
+                opp-level = <13>;
+            };
+
+            smcc_opp14: opp14 {
+                opp-level = <14>;
+            };
+        };
+
+        cpufreq: cpufreq {
+            compatible = "airoha,en7581-cpufreq";
+
+            operating-points-v2 = <&cpu_smcc_opp_table>;
+
+            #power-domain-cells = <0>;
+            #clock-cells = <0>;
+        };
+    };