[PATCH 0/4] cpufreq: mediatek: introduce mtk cpufreq

[PATCH 0/4] cpufreq: mediatek: introduce mtk cpufreq

[Tim Chang]

CPUFREQ is DVFS driver used for power saving by scaling clock frequency
and supply voltage of CPUs. This module cooperates with CCI DEVFREQ for
certain Mediatek SoCs.

Jia-Wei Chang (4):
  dt-bindings: cpufreq: mediatek: transform cpufreq-mediatek into yaml
  dt-bindings: cpufreq: mediatek: add mt8186 cpufreq dt-bindings
  cpufreq: mediatek: clean up cpufreq driver
  cpufreq: mediatek: add platform data and clean up voltage tracking
    logic

 .../bindings/cpufreq/cpufreq-mediatek.yaml    | 172 +++++
 drivers/cpufreq/mediatek-cpufreq.c            | 717 +++++++++---------
 2 files changed, 521 insertions(+), 368 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.yaml

-- 
2.18.0

[PATCH 1/4] dt-bindings: cpufreq: mediatek: transform cpufreq-mediatek into yaml

[Tim Chang]

convert Mediatek cpufreq devicetree binding to YAML.

Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.corp-partner.google.com>
---
 .../bindings/cpufreq/cpufreq-mediatek.yaml    | 131 ++++++++++++++++++
 1 file changed, 131 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.yaml

diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.yaml b/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.yaml
new file mode 100644
index 000000000000..584946eb3790
--- /dev/null
+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.yaml
@@ -0,0 +1,131 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/cpufreq/cpufreq-mediatek.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Mediatek CPUFREQ driver Device Tree Bindings
+
+maintainers:
+  - Jia-Wei Chang <jia-wei.chang@mediatek.com>
+
+description: |
+  CPUFREQ is used for scaling clock frequency of CPUs.
+  The module cooperates with CCI DEVFREQ to manage frequency for some Mediatek
+  SoCs.
+
+properties:
+  clocks:
+    items:
+      - description:
+          The first one is the multiplexer for clock input of CPU cluster.
+      - description:
+          The other is used as an intermediate clock source when the original
+          CPU is under transition and not stable yet.
+
+  clock-names:
+    items:
+      - const: "cpu"
+      - const: "intermediate"
+
+  operating-points-v2:
+    description:
+      For details, please refer to
+      Documentation/devicetree/bindings/opp/opp-v2.yaml
+
+  opp-table: true
+
+  proc-supply:
+    description:
+      Phandle of the regulator for CPU cluster that provides the supply
+      voltage.
+
+  sram-supply:
+    description:
+      Phandle of the regulator for sram of CPU cluster that provides the supply
+      voltage. When present, the cpufreq driver needs to do "voltage tracking"
+      to step by step scale up/down Vproc and Vsram to fit SoC specific needs.
+      When absent, the voltage scaling flow is handled by hardware, hence no
+      software "voltage tracking" is needed.
+
+  "#cooling-cells":
+    description:
+      For details, please refer to
+      Documentation/devicetree/bindings/thermal/thermal-cooling-devices.yaml
+
+required:
+  - clocks
+  - clock-names
+  - operating-points-v2
+  - proc-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    /* Example 1 (MT7623 SoC) */
+    #include <dt-bindings/clock/mt2701-clk.h>
+    cpu_opp_table: opp-table-0 {
+      compatible = "operating-points-v2";
+      opp-shared;
+      opp-598000000 {
+        opp-hz = /bits/ 64 <598000000>;
+        opp-microvolt = <1050000>;
+      };
+
+      /* ... */
+
+    };
+
+    cpus {
+      #address-cells = <2>;
+      #size-cells = <0>;
+      cpu0: cpu@0 {
+        device_type = "cpu";
+        compatible = "arm,cortex-a7";
+        reg = <0x0>;
+        clocks = <&infracfg CLK_INFRA_CPUSEL>, <&apmixedsys CLK_APMIXED_MAINPLL>;
+        clock-names = "cpu", "intermediate";
+        operating-points-v2 = <&cpu_opp_table>;
+        proc-supply = <&mt6380_vcpu_reg>;
+        #cooling-cells = <2>;
+      };
+
+      /* ... */
+
+    };
+
+  - |
+    /* Example 2 (MT8173 SoC) */
+    #include <dt-bindings/clock/mt8173-clk.h>
+    cluster1_opp: opp-table-1 {
+      compatible = "operating-points-v2";
+      opp-shared;
+      opp-507000000 {
+        opp-hz = /bits/ 64 <507000000>;
+        opp-microvolt = <828000>;
+      };
+
+      /* ... */
+
+    };
+
+    cpus {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      cpu2: cpu@100 {
+        device_type = "cpu";
+        compatible = "arm,cortex-a72";
+        reg = <0x100>;
+        enable-method = "psci";
+        cpu-idle-states = <&CPU_SLEEP_0>;
+        #cooling-cells = <2>;
+        clocks = <&infracfg CLK_INFRA_CA72SEL>, <&apmixedsys CLK_APMIXED_MAINPLL>;
+        clock-names = "cpu", "intermediate";
+        operating-points-v2 = <&cluster1_opp>;
+        proc-supply = <&mt6397_vpca15_reg>;
+      };
+
+      /* ... */
+
+    };
-- 
2.18.0

Re: [PATCH 1/4] dt-bindings: cpufreq: mediatek: transform cpufreq-mediatek into yaml

[Krzysztof Kozlowski]

On 07/03/2022 13:21, Tim Chang wrote:
> convert Mediatek cpufreq devicetree binding to YAML.

Start with capital letter please.
> 
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.corp-partner.google.com>
> ---
>  .../bindings/cpufreq/cpufreq-mediatek.yaml    | 131 ++++++++++++++++++
>  1 file changed, 131 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.yaml

You wrote "convert" but where is the removal of TXT?

> 
> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.yaml b/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.yaml
> new file mode 100644
> index 000000000000..584946eb3790
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.yaml
> @@ -0,0 +1,131 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/cpufreq/cpufreq-mediatek.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mediatek CPUFREQ driver Device Tree Bindings

Please remove "driver Device Tree Bindings" because the title should
describe the hardware. Therefore it could be something like "Mediatek
SoC CPU frequency and voltage scaling".

How is it related to cpufreq-mediatek-hw.yaml? The names/title look
unfortunately too similar.

In general this does not look like proper bindings (see also below lack
of compatible). Bindings describe the hardware, so what is exactly the
hardware here?

> +
> +maintainers:
> +  - Jia-Wei Chang <jia-wei.chang@mediatek.com>
> +
> +description: |
> +  CPUFREQ is used for scaling clock frequency of CPUs.
> +  The module cooperates with CCI DEVFREQ to manage frequency for some Mediatek
> +  SoCs.
> +
> +properties:

How is this schema going to be applied? I don't see here select neither
compatible.

> +  clocks:
> +    items:
> +      - description:
> +          The first one is the multiplexer for clock input of CPU cluster.
> +      - description:
> +          The other is used as an intermediate clock source when the original
> +          CPU is under transition and not stable yet.
> +
> +  clock-names:
> +    items:
> +      - const: "cpu"
> +      - const: "intermediate"
> +
> +  operating-points-v2:
> +    description:
> +      For details, please refer to
> +      Documentation/devicetree/bindings/opp/opp-v2.yaml
> +
> +  opp-table: true

You have operating-points-v2. What is this for? Did it exist in original
bindings?

> +
> +  proc-supply:
> +    description:
> +      Phandle of the regulator for CPU cluster that provides the supply
> +      voltage.
> +
> +  sram-supply:
> +    description:
> +      Phandle of the regulator for sram of CPU cluster that provides the supply
> +      voltage. When present, the cpufreq driver needs to do "voltage tracking"
> +      to step by step scale up/down Vproc and Vsram to fit SoC specific needs.
> +      When absent, the voltage scaling flow is handled by hardware, hence no
> +      software "voltage tracking" is needed.
> +
> +  "#cooling-cells":
> +    description:
> +      For details, please refer to
> +      Documentation/devicetree/bindings/thermal/thermal-cooling-devices.yaml

Skip description, it's obvious. Instead add here const with value.


Best regards,
Krzysztof

Re: [PATCH 1/4] dt-bindings: cpufreq: mediatek: transform cpufreq-mediatek into yaml

[Jia-Wei Chang]

Dear Krzysztof,

Thanks for your comments.
Pardon me for not being familiar with upstream rules and my late reply.
I was supposed to have an internal review before submission and I will.

On Mon, 2022-03-07 at 19:57 +0100, Krzysztof Kozlowski wrote:
> On 07/03/2022 13:21, Tim Chang wrote:
> > convert Mediatek cpufreq devicetree binding to YAML.
> 
> Start with capital letter please.

Sure, I will update it in the next version.

> > 
> > Signed-off-by: Jia-Wei Chang <
> > jia-wei.chang@mediatek.corp-partner.google.com>
> > ---
> >  .../bindings/cpufreq/cpufreq-mediatek.yaml    | 131
> > ++++++++++++++++++
> >  1 file changed, 131 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.yaml
> 
> You wrote "convert" but where is the removal of TXT?

Sorry, I missed it and I will fix it in the next version.

> 
> > 
> > diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-
> > mediatek.yaml b/Documentation/devicetree/bindings/cpufreq/cpufreq-
> > mediatek.yaml
> > new file mode 100644
> > index 000000000000..584946eb3790
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-
> > mediatek.yaml
> > @@ -0,0 +1,131 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: 
> > https://urldefense.com/v3/__http://devicetree.org/schemas/cpufreq/cpufreq-mediatek.yaml*__;Iw!!CTRNKA9wMg0ARbw!xbKG4TgD0MRpMLyGJVBZEGpZFrNOclrcxOCx_APKo5Nmg8nF2x5PcBdE0unvL2NdpChkMA$
> >  
> > +$schema: 
> > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!xbKG4TgD0MRpMLyGJVBZEGpZFrNOclrcxOCx_APKo5Nmg8nF2x5PcBdE0unvL2O8T_oxCQ$
> >  
> > +
> > +title: Mediatek CPUFREQ driver Device Tree Bindings
> 
> Please remove "driver Device Tree Bindings" because the title should
> describe the hardware. Therefore it could be something like "Mediatek
> SoC CPU frequency and voltage scaling".

Thanks for your suggestion of title.
Or should I use the origin title "Binding for MediaTek's CPUFreq
driver"?

> 
> How is it related to cpufreq-mediatek-hw.yaml? The names/title look
> unfortunately too similar.

No, mediatek-cpufreq is performing in kernel driver rather than on
hardware.
On the other hand, mediatek-cpufreq-hw is performing on hardware.
That's why "hw" is present in its name.

> 
> In general this does not look like proper bindings (see also below
> lack
> of compatible). Bindings describe the hardware, so what is exactly
> the
> hardware here?

Except for SoC, there's no requirement of hardware binding for
mediatek-cpufreq.
mediatek-cpufreq recognizes the compatible of Mediatek SoC while
probing.

> 
> > +
> > +maintainers:
> > +  - Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > +
> > +description: |
> > +  CPUFREQ is used for scaling clock frequency of CPUs.
> > +  The module cooperates with CCI DEVFREQ to manage frequency for
> > some Mediatek
> > +  SoCs.
> > +
> > +properties:
> 
> How is this schema going to be applied? I don't see here select
> neither
> compatible.

As mentioned above, only compatible of SoC is required for mediatek-
cpufreq.

> 
> > +  clocks:
> > +    items:
> > +      - description:
> > +          The first one is the multiplexer for clock input of CPU
> > cluster.
> > +      - description:
> > +          The other is used as an intermediate clock source when
> > the original
> > +          CPU is under transition and not stable yet.
> > +
> > +  clock-names:
> > +    items:
> > +      - const: "cpu"
> > +      - const: "intermediate"
> > +
> > +  operating-points-v2:
> > +    description:
> > +      For details, please refer to
> > +      Documentation/devicetree/bindings/opp/opp-v2.yaml
> > +
> > +  opp-table: true
> 
> You have operating-points-v2. What is this for? Did it exist in
> original
> bindings?

Yes, all of these properties exist in the original bindings.
operating-point-v2 is used to determine the voltage and frequency of
dvfs which is further utilized by mediatek-cpufreq here.

> 
> > +
> > +  proc-supply:
> > +    description:
> > +      Phandle of the regulator for CPU cluster that provides the
> > supply
> > +      voltage.
> > +
> > +  sram-supply:
> > +    description:
> > +      Phandle of the regulator for sram of CPU cluster that
> > provides the supply
> > +      voltage. When present, the cpufreq driver needs to do
> > "voltage tracking"
> > +      to step by step scale up/down Vproc and Vsram to fit SoC
> > specific needs.
> > +      When absent, the voltage scaling flow is handled by
> > hardware, hence no
> > +      software "voltage tracking" is needed.
> > +
> > +  "#cooling-cells":
> > +    description:
> > +      For details, please refer to
> > +      Documentation/devicetree/bindings/thermal/thermal-cooling-
> > devices.yaml
> 
> Skip description, it's obvious. Instead add here const with value.

Sure, I'll remove description and add const value in the next version.

> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 1/4] dt-bindings: cpufreq: mediatek: transform cpufreq-mediatek into yaml

[Krzysztof Kozlowski]

On 24/03/2022 10:38, Jia-Wei Chang wrote:
>>
>>>
>>> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-
>>> mediatek.yaml b/Documentation/devicetree/bindings/cpufreq/cpufreq-
>>> mediatek.yaml
>>> new file mode 100644
>>> index 000000000000..584946eb3790
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-
>>> mediatek.yaml
>>> @@ -0,0 +1,131 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: 
>>> https://urldefense.com/v3/__http://devicetree.org/schemas/cpufreq/cpufreq-mediatek.yaml*__;Iw!!CTRNKA9wMg0ARbw!xbKG4TgD0MRpMLyGJVBZEGpZFrNOclrcxOCx_APKo5Nmg8nF2x5PcBdE0unvL2NdpChkMA$
>>>  
>>> +$schema: 
>>> https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!xbKG4TgD0MRpMLyGJVBZEGpZFrNOclrcxOCx_APKo5Nmg8nF2x5PcBdE0unvL2O8T_oxCQ$
>>>  
>>> +
>>> +title: Mediatek CPUFREQ driver Device Tree Bindings
>>
>> Please remove "driver Device Tree Bindings" because the title should
>> describe the hardware. Therefore it could be something like "Mediatek
>> SoC CPU frequency and voltage scaling".
> 
> Thanks for your suggestion of title.
> Or should I use the origin title "Binding for MediaTek's CPUFreq
> driver"?

Mediatek CPUFREQ
or
Mediatek CPU frequency scaling

> 
>>
>> How is it related to cpufreq-mediatek-hw.yaml? The names/title look
>> unfortunately too similar.
> 
> No, mediatek-cpufreq is performing in kernel driver rather than on
> hardware.
> On the other hand, mediatek-cpufreq-hw is performing on hardware.
> That's why "hw" is present in its name.

Unfortunately, I do not get it. The bindings are only about hardware, so
how bindings could be about CPU frequency scaling not in hardware?

> 
>>
>> In general this does not look like proper bindings (see also below
>> lack
>> of compatible). Bindings describe the hardware, so what is exactly
>> the
>> hardware here?
> 
> Except for SoC, there's no requirement of hardware binding for
> mediatek-cpufreq.
> mediatek-cpufreq recognizes the compatible of Mediatek SoC while
> probing.

What is the hardware here? If there is no requirement for bindings for
mediate-cpufreq, why do we have this patch here?

> 
>>
>>> +
>>> +maintainers:
>>> +  - Jia-Wei Chang <jia-wei.chang@mediatek.com>
>>> +
>>> +description: |
>>> +  CPUFREQ is used for scaling clock frequency of CPUs.
>>> +  The module cooperates with CCI DEVFREQ to manage frequency for
>>> some Mediatek
>>> +  SoCs.
>>> +
>>> +properties:
>>
>> How is this schema going to be applied? I don't see here select
>> neither
>> compatible.
> 
> As mentioned above, only compatible of SoC is required for mediatek-
> cpufreq.

It does not answer my questions. How the schema is going to be applied?


Best regards,
Krzysztof

Re: [PATCH 1/4] dt-bindings: cpufreq: mediatek: transform cpufreq-mediatek into yaml

[Jia-Wei Chang]

On Thu, 2022-03-24 at 11:33 +0100, Krzysztof Kozlowski wrote:
> On 24/03/2022 10:38, Jia-Wei Chang wrote:
> > > 
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-
> > > > mediatek.yaml
> > > > b/Documentation/devicetree/bindings/cpufreq/cpufreq-
> > > > mediatek.yaml
> > > > new file mode 100644
> > > > index 000000000000..584946eb3790
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-
> > > > mediatek.yaml
> > > > @@ -0,0 +1,131 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: 
> > > > 
https://urldefense.com/v3/__http://devicetree.org/schemas/cpufreq/cpufreq-mediatek.yaml*__;Iw!!CTRNKA9wMg0ARbw!xbKG4TgD0MRpMLyGJVBZEGpZFrNOclrcxOCx_APKo5Nmg8nF2x5PcBdE0unvL2NdpChkMA$
> > > >  
> > > > +$schema: 
> > > > 
https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!xbKG4TgD0MRpMLyGJVBZEGpZFrNOclrcxOCx_APKo5Nmg8nF2x5PcBdE0unvL2O8T_oxCQ$
> > > >  
> > > > +
> > > > +title: Mediatek CPUFREQ driver Device Tree Bindings
> > > 
> > > Please remove "driver Device Tree Bindings" because the title
> > > should
> > > describe the hardware. Therefore it could be something like
> > > "Mediatek
> > > SoC CPU frequency and voltage scaling".
> > 
> > Thanks for your suggestion of title.
> > Or should I use the origin title "Binding for MediaTek's CPUFreq
> > driver"?
> 
> Mediatek CPUFREQ
> or
> Mediatek CPU frequency scaling

Ok, I will choose one of it.

> 
> > 
> > > 
> > > How is it related to cpufreq-mediatek-hw.yaml? The names/title
> > > look
> > > unfortunately too similar.
> > 
> > No, mediatek-cpufreq is performing in kernel driver rather than on
> > hardware.
> > On the other hand, mediatek-cpufreq-hw is performing on hardware.
> > That's why "hw" is present in its name.
> 
> Unfortunately, I do not get it. The bindings are only about hardware,
> so
> how bindings could be about CPU frequency scaling not in hardware?

Sorry, let me correct my statements.

For mediatek-cpufreq here, the required hardware are clock and
regulator which have to be under control of mediatek-cpufreq. That's
the reason why it needs bindings.

mediatek-cpufreq scales up and down voltage and frequency via kernel
framework of clock and regulator, however, mediatek-cpufreq-hw delegate
the voltage and frequency control to a hardware agent instead.

> 
> > 
> > > 
> > > In general this does not look like proper bindings (see also
> > > below
> > > lack
> > > of compatible). Bindings describe the hardware, so what is
> > > exactly
> > > the
> > > hardware here?
> > 
> > Except for SoC, there's no requirement of hardware binding for
> > mediatek-cpufreq.
> > mediatek-cpufreq recognizes the compatible of Mediatek SoC while
> > probing.
> 
> What is the hardware here? If there is no requirement for bindings
> for
> mediate-cpufreq, why do we have this patch here?

Sorry, that's my mistake.
Clock and regulator are required hardware for mediatek-cpufreq.

> 
> > 
> > > 
> > > > +
> > > > +maintainers:
> > > > +  - Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > > > +
> > > > +description: |
> > > > +  CPUFREQ is used for scaling clock frequency of CPUs.
> > > > +  The module cooperates with CCI DEVFREQ to manage frequency
> > > > for
> > > > some Mediatek
> > > > +  SoCs.
> > > > +
> > > > +properties:
> > > 
> > > How is this schema going to be applied? I don't see here select
> > > neither
> > > compatible.
> > 
> > As mentioned above, only compatible of SoC is required for
> > mediatek-
> > cpufreq.
> 
> It does not answer my questions. How the schema is going to be
> applied?

Currently, we do use compatible of SoC to probe mediatek-cpufreq.

If the better way is using clock and regulator opp, do you have a
suggestion to approach that?
I mean I can't find a good example from other vendors trying to do that
way. Or maybe I miss something?

> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 1/4] dt-bindings: cpufreq: mediatek: transform cpufreq-mediatek into yaml

[Krzysztof Kozlowski]

On 01/04/2022 15:26, Jia-Wei Chang wrote:
> On Thu, 2022-03-24 at 11:33 +0100, Krzysztof Kozlowski wrote:
>> On 24/03/2022 10:38, Jia-Wei Chang wrote:
>>>>
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-
>>>>> mediatek.yaml
>>>>> b/Documentation/devicetree/bindings/cpufreq/cpufreq-
>>>>> mediatek.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..584946eb3790
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-
>>>>> mediatek.yaml
>>>>> @@ -0,0 +1,131 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: 
>>>>>
> https://urldefense.com/v3/__http://devicetree.org/schemas/cpufreq/cpufreq-mediatek.yaml*__;Iw!!CTRNKA9wMg0ARbw!xbKG4TgD0MRpMLyGJVBZEGpZFrNOclrcxOCx_APKo5Nmg8nF2x5PcBdE0unvL2NdpChkMA$
>>>>>  
>>>>> +$schema: 
>>>>>
> https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!xbKG4TgD0MRpMLyGJVBZEGpZFrNOclrcxOCx_APKo5Nmg8nF2x5PcBdE0unvL2O8T_oxCQ$
>>>>>  
>>>>> +
>>>>> +title: Mediatek CPUFREQ driver Device Tree Bindings
>>>>
>>>> Please remove "driver Device Tree Bindings" because the title
>>>> should
>>>> describe the hardware. Therefore it could be something like
>>>> "Mediatek
>>>> SoC CPU frequency and voltage scaling".
>>>
>>> Thanks for your suggestion of title.
>>> Or should I use the origin title "Binding for MediaTek's CPUFreq
>>> driver"?
>>
>> Mediatek CPUFREQ
>> or
>> Mediatek CPU frequency scaling
> 
> Ok, I will choose one of it.
> 
>>
>>>
>>>>
>>>> How is it related to cpufreq-mediatek-hw.yaml? The names/title
>>>> look
>>>> unfortunately too similar.
>>>
>>> No, mediatek-cpufreq is performing in kernel driver rather than on
>>> hardware.
>>> On the other hand, mediatek-cpufreq-hw is performing on hardware.
>>> That's why "hw" is present in its name.
>>
>> Unfortunately, I do not get it. The bindings are only about hardware,
>> so
>> how bindings could be about CPU frequency scaling not in hardware?
> 
> Sorry, let me correct my statements.
> 
> For mediatek-cpufreq here, the required hardware are clock and
> regulator which have to be under control of mediatek-cpufreq. That's
> the reason why it needs bindings.
> 
> mediatek-cpufreq scales up and down voltage and frequency via kernel
> framework of clock and regulator, however, mediatek-cpufreq-hw delegate
> the voltage and frequency control to a hardware agent instead.

OK, that makes sense, thanks for explanation.

> 
>>
>>>
>>>>
>>>> In general this does not look like proper bindings (see also
>>>> below
>>>> lack
>>>> of compatible). Bindings describe the hardware, so what is
>>>> exactly
>>>> the
>>>> hardware here?
>>>
>>> Except for SoC, there's no requirement of hardware binding for
>>> mediatek-cpufreq.
>>> mediatek-cpufreq recognizes the compatible of Mediatek SoC while
>>> probing.
>>
>> What is the hardware here? If there is no requirement for bindings
>> for
>> mediate-cpufreq, why do we have this patch here?
> 
> Sorry, that's my mistake.
> Clock and regulator are required hardware for mediatek-cpufreq.
> 
>>
>>>
>>>>
>>>>> +
>>>>> +maintainers:
>>>>> +  - Jia-Wei Chang <jia-wei.chang@mediatek.com>
>>>>> +
>>>>> +description: |
>>>>> +  CPUFREQ is used for scaling clock frequency of CPUs.
>>>>> +  The module cooperates with CCI DEVFREQ to manage frequency
>>>>> for
>>>>> some Mediatek
>>>>> +  SoCs.
>>>>> +
>>>>> +properties:
>>>>
>>>> How is this schema going to be applied? I don't see here select
>>>> neither
>>>> compatible.
>>>
>>> As mentioned above, only compatible of SoC is required for
>>> mediatek-
>>> cpufreq.
>>
>> It does not answer my questions. How the schema is going to be
>> applied?
> 
> Currently, we do use compatible of SoC to probe mediatek-cpufreq.

Probing and binding to compatible is correct, but there is no compatible
here, so the schema is a no-op. Does nothing.

> If the better way is using clock and regulator opp, do you have a
> suggestion to approach that?
> I mean I can't find a good example from other vendors trying to do that
> way. Or maybe I miss something?

One other way (proper) is to use cpufreq-dt and existing bindings. I
understand that maybe you need some specific bindings here, but I fail
to see how they would work. IOW, you don't have the compatible, no
select, so nothing can use these bindings. Also bindings do not refer to
any specific hardware, like SoC model.

It's good that you try to convert existing bindings to DT schema, but
with that they should be probably fixed/updated to match proper bindings.

Best regards,
Krzysztof

Re: [PATCH 1/4] dt-bindings: cpufreq: mediatek: transform cpufreq-mediatek into yaml

[Jia-Wei Chang]

On Fri, 2022-04-01 at 18:32 +0200, Krzysztof Kozlowski wrote:
> On 01/04/2022 15:26, Jia-Wei Chang wrote:
> > On Thu, 2022-03-24 at 11:33 +0100, Krzysztof Kozlowski wrote:
> > > On 24/03/2022 10:38, Jia-Wei Chang wrote:
> > > > > 
> > > > > > 
> > > > > > diff --git
> > > > > > a/Documentation/devicetree/bindings/cpufreq/cpufreq-
> > > > > > mediatek.yaml
> > > > > > b/Documentation/devicetree/bindings/cpufreq/cpufreq-
> > > > > > mediatek.yaml
> > > > > > new file mode 100644
> > > > > > index 000000000000..584946eb3790
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-
> > > > > > mediatek.yaml
> > > > > > @@ -0,0 +1,131 @@
> > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > > +%YAML 1.2
> > > > > > +---
> > > > > > +$id: 
> > > > > > 
> > 
> > 
https://urldefense.com/v3/__http://devicetree.org/schemas/cpufreq/cpufreq-mediatek.yaml*__;Iw!!CTRNKA9wMg0ARbw!xbKG4TgD0MRpMLyGJVBZEGpZFrNOclrcxOCx_APKo5Nmg8nF2x5PcBdE0unvL2NdpChkMA$
> > > > > >  
> > > > > > +$schema: 
> > > > > > 
> > 
> > 
https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!xbKG4TgD0MRpMLyGJVBZEGpZFrNOclrcxOCx_APKo5Nmg8nF2x5PcBdE0unvL2O8T_oxCQ$
> > > > > >  
> > > > > > +
> > > > > > +title: Mediatek CPUFREQ driver Device Tree Bindings
> > > > > 
> > > > > Please remove "driver Device Tree Bindings" because the title
> > > > > should
> > > > > describe the hardware. Therefore it could be something like
> > > > > "Mediatek
> > > > > SoC CPU frequency and voltage scaling".
> > > > 
> > > > Thanks for your suggestion of title.
> > > > Or should I use the origin title "Binding for MediaTek's
> > > > CPUFreq
> > > > driver"?
> > > 
> > > Mediatek CPUFREQ
> > > or
> > > Mediatek CPU frequency scaling
> > 
> > Ok, I will choose one of it.
> > 
> > > 
> > > > 
> > > > > 
> > > > > How is it related to cpufreq-mediatek-hw.yaml? The
> > > > > names/title
> > > > > look
> > > > > unfortunately too similar.
> > > > 
> > > > No, mediatek-cpufreq is performing in kernel driver rather than
> > > > on
> > > > hardware.
> > > > On the other hand, mediatek-cpufreq-hw is performing on
> > > > hardware.
> > > > That's why "hw" is present in its name.
> > > 
> > > Unfortunately, I do not get it. The bindings are only about
> > > hardware,
> > > so
> > > how bindings could be about CPU frequency scaling not in
> > > hardware?
> > 
> > Sorry, let me correct my statements.
> > 
> > For mediatek-cpufreq here, the required hardware are clock and
> > regulator which have to be under control of mediatek-cpufreq.
> > That's
> > the reason why it needs bindings.
> > 
> > mediatek-cpufreq scales up and down voltage and frequency via
> > kernel
> > framework of clock and regulator, however, mediatek-cpufreq-hw
> > delegate
> > the voltage and frequency control to a hardware agent instead.
> 
> OK, that makes sense, thanks for explanation.
> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > In general this does not look like proper bindings (see also
> > > > > below
> > > > > lack
> > > > > of compatible). Bindings describe the hardware, so what is
> > > > > exactly
> > > > > the
> > > > > hardware here?
> > > > 
> > > > Except for SoC, there's no requirement of hardware binding for
> > > > mediatek-cpufreq.
> > > > mediatek-cpufreq recognizes the compatible of Mediatek SoC
> > > > while
> > > > probing.
> > > 
> > > What is the hardware here? If there is no requirement for
> > > bindings
> > > for
> > > mediate-cpufreq, why do we have this patch here?
> > 
> > Sorry, that's my mistake.
> > Clock and regulator are required hardware for mediatek-cpufreq.
> > 
> > > 
> > > > 
> > > > > 
> > > > > > +
> > > > > > +maintainers:
> > > > > > +  - Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > > > > > +
> > > > > > +description: |
> > > > > > +  CPUFREQ is used for scaling clock frequency of CPUs.
> > > > > > +  The module cooperates with CCI DEVFREQ to manage
> > > > > > frequency
> > > > > > for
> > > > > > some Mediatek
> > > > > > +  SoCs.
> > > > > > +
> > > > > > +properties:
> > > > > 
> > > > > How is this schema going to be applied? I don't see here
> > > > > select
> > > > > neither
> > > > > compatible.
> > > > 
> > > > As mentioned above, only compatible of SoC is required for
> > > > mediatek-
> > > > cpufreq.
> > > 
> > > It does not answer my questions. How the schema is going to be
> > > applied?
> > 
> > Currently, we do use compatible of SoC to probe mediatek-cpufreq.
> 
> Probing and binding to compatible is correct, but there is no
> compatible
> here, so the schema is a no-op. Does nothing.

Correct. I will update it in the next version.

> 
> > If the better way is using clock and regulator opp, do you have a
> > suggestion to approach that?
> > I mean I can't find a good example from other vendors trying to do
> > that
> > way. Or maybe I miss something?
> 
> One other way (proper) is to use cpufreq-dt and existing bindings. I
> understand that maybe you need some specific bindings here, but I
> fail
> to see how they would work. IOW, you don't have the compatible, no
> select, so nothing can use these bindings. Also bindings do not refer
> to
> any specific hardware, like SoC model.
> 
> It's good that you try to convert existing bindings to DT schema, but
> with that they should be probably fixed/updated to match proper
> bindings.

I got it. I will add compatible information to property of bindings and
dts example here as well.

Should I split the overall change of yaml into two patches? One for
conversion of bindings and the other for the rest of change.

> 
> Best regards,
> Krzysztof

Re: [PATCH 1/4] dt-bindings: cpufreq: mediatek: transform cpufreq-mediatek into yaml

[Rex-BC Chen]

On Fri, 2022-04-01 at 18:32 +0200, Krzysztof Kozlowski wrote:
> On 01/04/2022 15:26, Jia-Wei Chang wrote:
> > On Thu, 2022-03-24 at 11:33 +0100, Krzysztof Kozlowski wrote:
> > > On 24/03/2022 10:38, Jia-Wei Chang wrote:
> > > > > 
> > > > > > 
> > > > > > diff --git
> > > > > > a/Documentation/devicetree/bindings/cpufreq/cpufreq-
> > > > > > mediatek.yaml
> > > > > > b/Documentation/devicetree/bindings/cpufreq/cpufreq-
> > > > > > mediatek.yaml
> > > > > > new file mode 100644
> > > > > > index 000000000000..584946eb3790
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-
> > > > > > mediatek.yaml
> > > > > > @@ -0,0 +1,131 @@
> > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > > +%YAML 1.2
> > > > > > +---
> > > > > > +$id: 
> > > > > > 
> > 
> > 
https://urldefense.com/v3/__http://devicetree.org/schemas/cpufreq/cpufreq-mediatek.yaml*__;Iw!!CTRNKA9wMg0ARbw!xbKG4TgD0MRpMLyGJVBZEGpZFrNOclrcxOCx_APKo5Nmg8nF2x5PcBdE0unvL2NdpChkMA$
> > > > > >  
> > > > > > +$schema: 
> > > > > > 
> > 
> > 
https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!xbKG4TgD0MRpMLyGJVBZEGpZFrNOclrcxOCx_APKo5Nmg8nF2x5PcBdE0unvL2O8T_oxCQ$
> > > > > >  
> > > > > > +
> > > > > > +title: Mediatek CPUFREQ driver Device Tree Bindings
> > > > > 
> > > > > Please remove "driver Device Tree Bindings" because the title
> > > > > should
> > > > > describe the hardware. Therefore it could be something like
> > > > > "Mediatek
> > > > > SoC CPU frequency and voltage scaling".
> > > > 
> > > > Thanks for your suggestion of title.
> > > > Or should I use the origin title "Binding for MediaTek's
> > > > CPUFreq
> > > > driver"?
> > > 
> > > Mediatek CPUFREQ
> > > or
> > > Mediatek CPU frequency scaling
> > 
> > Ok, I will choose one of it.
> > 
> > > 
> > > > 
> > > > > 
> > > > > How is it related to cpufreq-mediatek-hw.yaml? The
> > > > > names/title
> > > > > look
> > > > > unfortunately too similar.
> > > > 
> > > > No, mediatek-cpufreq is performing in kernel driver rather than
> > > > on
> > > > hardware.
> > > > On the other hand, mediatek-cpufreq-hw is performing on
> > > > hardware.
> > > > That's why "hw" is present in its name.
> > > 
> > > Unfortunately, I do not get it. The bindings are only about
> > > hardware,
> > > so
> > > how bindings could be about CPU frequency scaling not in
> > > hardware?
> > 
> > Sorry, let me correct my statements.
> > 
> > For mediatek-cpufreq here, the required hardware are clock and
> > regulator which have to be under control of mediatek-cpufreq.
> > That's
> > the reason why it needs bindings.
> > 
> > mediatek-cpufreq scales up and down voltage and frequency via
> > kernel
> > framework of clock and regulator, however, mediatek-cpufreq-hw
> > delegate
> > the voltage and frequency control to a hardware agent instead.
> 
> OK, that makes sense, thanks for explanation.
> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > In general this does not look like proper bindings (see also
> > > > > below
> > > > > lack
> > > > > of compatible). Bindings describe the hardware, so what is
> > > > > exactly
> > > > > the
> > > > > hardware here?
> > > > 
> > > > Except for SoC, there's no requirement of hardware binding for
> > > > mediatek-cpufreq.
> > > > mediatek-cpufreq recognizes the compatible of Mediatek SoC
> > > > while
> > > > probing.
> > > 
> > > What is the hardware here? If there is no requirement for
> > > bindings
> > > for
> > > mediate-cpufreq, why do we have this patch here?
> > 
> > Sorry, that's my mistake.
> > Clock and regulator are required hardware for mediatek-cpufreq.
> > 
> > > 
> > > > 
> > > > > 
> > > > > > +
> > > > > > +maintainers:
> > > > > > +  - Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > > > > > +
> > > > > > +description: |
> > > > > > +  CPUFREQ is used for scaling clock frequency of CPUs.
> > > > > > +  The module cooperates with CCI DEVFREQ to manage
> > > > > > frequency
> > > > > > for
> > > > > > some Mediatek
> > > > > > +  SoCs.
> > > > > > +
> > > > > > +properties:
> > > > > 
> > > > > How is this schema going to be applied? I don't see here
> > > > > select
> > > > > neither
> > > > > compatible.
> > > > 
> > > > As mentioned above, only compatible of SoC is required for
> > > > mediatek-
> > > > cpufreq.
> > > 
> > > It does not answer my questions. How the schema is going to be
> > > applied?
> > 
> > Currently, we do use compatible of SoC to probe mediatek-cpufreq.
> 
> Probing and binding to compatible is correct, but there is no
> compatible
> here, so the schema is a no-op. Does nothing.
> 
> > If the better way is using clock and regulator opp, do you have a
> > suggestion to approach that?
> > I mean I can't find a good example from other vendors trying to do
> > that
> > way. Or maybe I miss something?
> 
> One other way (proper) is to use cpufreq-dt and existing bindings. I
> understand that maybe you need some specific bindings here, but I
> fail
> to see how they would work. IOW, you don't have the compatible, no
> select, so nothing can use these bindings. Also bindings do not refer
> to
> any specific hardware, like SoC model.
> 
> It's good that you try to convert existing bindings to DT schema, but
> with that they should be probably fixed/updated to match proper
> bindings.
> 
> Best regards,
> Krzysztof
> 

Hello Krzysztof,

Thanks for your suggestion.
I have discussed with Jia-wei internally.
We want to push next version because we finish to prepare the driver
parts.

For binding part, we want to cancel the transformation to yaml first
and only add the mediatek cci property for cpufreq series in next
version.

I will help Jia-wei to push next version.
If you have any suggestion, we can discuss in the next version (v2) of
this series.

Thanks for your big support!

BRs,
Rex

[PATCH 2/4] dt-bindings: cpufreq: mediatek: add mt8186 cpufreq dt-bindings

[Tim Chang]

1. add cci property.
2. add example of MT8186.

Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.corp-partner.google.com>
---
 .../bindings/cpufreq/cpufreq-mediatek.yaml    | 41 +++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.yaml b/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.yaml
index 584946eb3790..d3ce17fd8fcf 100644
--- a/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.yaml
+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.yaml
@@ -48,6 +48,10 @@ properties:
       When absent, the voltage scaling flow is handled by hardware, hence no
       software "voltage tracking" is needed.
 
+  cci:
+    description:
+      Phandle of the cci to be linked with the phandle of CPU if present.
+
   "#cooling-cells":
     description:
       For details, please refer to
@@ -129,3 +133,40 @@ examples:
       /* ... */
 
     };
+
+  - |
+    /* Example 3 (MT8186 SoC) */
+    #include <dt-bindings/clock/mt8186-clk.h>
+    cluster0_opp: opp-table-0 {
+      compatible = "operating-points-v2";
+      opp-shared;
+      opp0_00: opp-500000000 {
+        opp-hz = /bits/ 64 <500000000>;
+        opp-microvolt = <600000>;
+        opp-level = <15>;
+        required-opps = <&opp2_00>;
+      };
+
+      /* ... */
+
+    };
+
+    cpus {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      cpu1: cpu@1 {
+        device_type = "cpu";
+        compatible = "arm,cortex-a55";
+        reg = <0x0100>;
+        enable-method = "psci";
+        clocks = <&mcusys CLK_MCU_ARMPLL_LL_SEL>, <&apmixedsys CLK_APMIXED_MAINPLL>;
+        clock-names = "cpu", "intermediate";
+        operating-points-v2 = <&cluster0_opp>;
+        proc-supply = <&mt6358_vproc12_reg>;
+        sram-supply = <&mt6358_vsram_proc12_reg>;
+        cci = <&cci>;
+      };
+
+      /* ... */
+
+    };
-- 
2.18.0

Re: [PATCH 2/4] dt-bindings: cpufreq: mediatek: add mt8186 cpufreq dt-bindings

[Krzysztof Kozlowski]

On 07/03/2022 13:21, Tim Chang wrote:
> 1. add cci property.
> 2. add example of MT8186.

One logical change at a time. Are these related? Why entirely new
example just for "cci" node? Maybe this should be part of existing example?

> 
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.corp-partner.google.com>
> ---
>  .../bindings/cpufreq/cpufreq-mediatek.yaml    | 41 +++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.yaml b/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.yaml
> index 584946eb3790..d3ce17fd8fcf 100644
> --- a/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.yaml
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.yaml
> @@ -48,6 +48,10 @@ properties:
>        When absent, the voltage scaling flow is handled by hardware, hence no
>        software "voltage tracking" is needed.
>  
> +  cci:
> +    description:
> +      Phandle of the cci to be linked with the phandle of CPU if present.

This does not look like a standard type, so you need type.



Best regards,
Krzysztof

Re: [PATCH 2/4] dt-bindings: cpufreq: mediatek: add mt8186 cpufreq dt-bindings

[Jia-Wei Chang]

On Mon, 2022-03-07 at 19:59 +0100, Krzysztof Kozlowski wrote:
> On 07/03/2022 13:21, Tim Chang wrote:
> > 1. add cci property.
> > 2. add example of MT8186.
> 
> One logical change at a time. Are these related? Why entirely new
> example just for "cci" node? Maybe this should be part of existing
> example?

Yes, the cci property is required in some SoC, e.g. mt8183 and mt8186,
because cpu and cci share the same power supplies.
I will update the commit message and add an example of mt8186 to
present usage of cci.

> 
> > 
> > Signed-off-by: Jia-Wei Chang <
> > jia-wei.chang@mediatek.corp-partner.google.com>
> > ---
> >  .../bindings/cpufreq/cpufreq-mediatek.yaml    | 41
> > +++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-
> > mediatek.yaml b/Documentation/devicetree/bindings/cpufreq/cpufreq-
> > mediatek.yaml
> > index 584946eb3790..d3ce17fd8fcf 100644
> > --- a/Documentation/devicetree/bindings/cpufreq/cpufreq-
> > mediatek.yaml
> > +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-
> > mediatek.yaml
> > @@ -48,6 +48,10 @@ properties:
> >        When absent, the voltage scaling flow is handled by
> > hardware, hence no
> >        software "voltage tracking" is needed.
> >  
> > +  cci:
> > +    description:
> > +      Phandle of the cci to be linked with the phandle of CPU if
> > present.
> 
> This does not look like a standard type, so you need type.

Sure, I will add the type for it in the next version.

> 
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 2/4] dt-bindings: cpufreq: mediatek: add mt8186 cpufreq dt-bindings

[Krzysztof Kozlowski]

On 24/03/2022 10:42, Jia-Wei Chang wrote:
> On Mon, 2022-03-07 at 19:59 +0100, Krzysztof Kozlowski wrote:
>> On 07/03/2022 13:21, Tim Chang wrote:
>>> 1. add cci property.
>>> 2. add example of MT8186.
>>
>> One logical change at a time. Are these related? Why entirely new
>> example just for "cci" node? Maybe this should be part of existing
>> example?
> 
> Yes, the cci property is required in some SoC, e.g. mt8183 and mt8186,
> because cpu and cci share the same power supplies.

I asked why this cannot be part of existing example.

> I will update the commit message and add an example of mt8186 to
> present usage of cci.

You added the example here, didn't you?

Best regards,
Krzysztof

Re: [PATCH 2/4] dt-bindings: cpufreq: mediatek: add mt8186 cpufreq dt-bindings

[Jia-Wei Chang]

On Thu, 2022-03-24 at 11:35 +0100, Krzysztof Kozlowski wrote:
> On 24/03/2022 10:42, Jia-Wei Chang wrote:
> > On Mon, 2022-03-07 at 19:59 +0100, Krzysztof Kozlowski wrote:
> > > On 07/03/2022 13:21, Tim Chang wrote:
> > > > 1. add cci property.
> > > > 2. add example of MT8186.
> > > 
> > > One logical change at a time. Are these related? Why entirely new
> > > example just for "cci" node? Maybe this should be part of
> > > existing
> > > example?
> > 
> > Yes, the cci property is required in some SoC, e.g. mt8183 and
> > mt8186,
> > because cpu and cci share the same power supplies.
> 
> I asked why this cannot be part of existing example.

I misunderstood that.
I will update the complete example in the next version.

> 
> > I will update the commit message and add an example of mt8186 to
> > present usage of cci.
> 
> You added the example here, didn't you?

Yes, I did add it here.

> 
> Best regards,
> Krzysztof

Re: [PATCH 2/4] dt-bindings: cpufreq: mediatek: add mt8186 cpufreq dt-bindings

[Rob Herring]

On Mon, Mar 07, 2022 at 08:21:49PM +0800, Tim Chang wrote:
> 1. add cci property.
> 2. add example of MT8186.
> 
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.corp-partner.google.com>
> ---
>  .../bindings/cpufreq/cpufreq-mediatek.yaml    | 41 +++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.yaml b/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.yaml
> index 584946eb3790..d3ce17fd8fcf 100644
> --- a/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.yaml
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.yaml
> @@ -48,6 +48,10 @@ properties:
>        When absent, the voltage scaling flow is handled by hardware, hence no
>        software "voltage tracking" is needed.
>  
> +  cci:
> +    description:
> +      Phandle of the cci to be linked with the phandle of CPU if present.

We already have a binding for this. See cci-control-port.

> +
>    "#cooling-cells":
>      description:
>        For details, please refer to

Re: [PATCH 2/4] dt-bindings: cpufreq: mediatek: add mt8186 cpufreq dt-bindings

[Jia-Wei Chang]

On Thu, 2022-03-10 at 14:44 -0600, Rob Herring wrote:
> On Mon, Mar 07, 2022 at 08:21:49PM +0800, Tim Chang wrote:
> > 1. add cci property.
> > 2. add example of MT8186.
> > 
> > Signed-off-by: Jia-Wei Chang <
> > jia-wei.chang@mediatek.corp-partner.google.com>
> > ---
> >  .../bindings/cpufreq/cpufreq-mediatek.yaml    | 41
> > +++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-
> > mediatek.yaml b/Documentation/devicetree/bindings/cpufreq/cpufreq-
> > mediatek.yaml
> > index 584946eb3790..d3ce17fd8fcf 100644
> > --- a/Documentation/devicetree/bindings/cpufreq/cpufreq-
> > mediatek.yaml
> > +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-
> > mediatek.yaml
> > @@ -48,6 +48,10 @@ properties:
> >        When absent, the voltage scaling flow is handled by
> > hardware, hence no
> >        software "voltage tracking" is needed.
> >  
> > +  cci:
> > +    description:
> > +      Phandle of the cci to be linked with the phandle of CPU if
> > present.
> 
> We already have a binding for this. See cci-control-port.

Hi Rob,

Pardon me for my late reply.

It seems that "cci-control-port" is hardware IP from ARM.
But mediatek-cpufreq uses MTK internal CCI hardware IP.
I think I should keep this change here.

Thanks.

> 
> > +
> >    "#cooling-cells":
> >      description:
> >        For details, please refer to

[PATCH 3/4] cpufreq: mediatek: clean up cpufreq driver

[Tim Chang]

cleanup of naming, print log and comments.

Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.corp-partner.google.com>
---
 drivers/cpufreq/mediatek-cpufreq.c | 487 ++++++++++++++---------------
 1 file changed, 233 insertions(+), 254 deletions(-)

diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
index 8e9d706d8081..3f00c7eb01f1 100644
--- a/drivers/cpufreq/mediatek-cpufreq.c
+++ b/drivers/cpufreq/mediatek-cpufreq.c
@@ -1,7 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
- * Copyright (c) 2015 Linaro Ltd.
- * Author: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
+ * Copyright (C) 2022 MediaTek Inc.
  */
 
 #include <linux/clk.h>
@@ -22,7 +21,7 @@
 #define VOLT_TOL		(10000)
 
 /*
- * The struct mtk_cpu_dvfs_info holds necessary information for doing CPU DVFS
+ * The struct mtk_cpufreq_drv holds necessary information for doing CPU DVFS
  * on each CPU power/clock domain of Mediatek SoCs. Each CPU cluster in
  * Mediatek SoCs has two voltage inputs, Vproc and Vsram. In some cases the two
  * voltage inputs need to be controlled under a hardware limitation:
@@ -32,7 +31,7 @@
  * needs to be switched to another stable PLL clock temporarily until
  * the original PLL becomes stable at target frequency.
  */
-struct mtk_cpu_dvfs_info {
+struct mtk_cpufreq_drv {
 	struct cpumask cpus;
 	struct device *cpu_dev;
 	struct regulator *proc_reg;
@@ -40,45 +39,45 @@ struct mtk_cpu_dvfs_info {
 	struct clk *cpu_clk;
 	struct clk *inter_clk;
 	struct list_head list_head;
-	int intermediate_voltage;
+	int inter_voltage;
 	bool need_voltage_tracking;
-	int old_vproc;
-	struct mutex lock; /* avoid notify and policy race condition */
+	int old_voltage;
+	struct mutex lock;  /* avoid notify and policy race condition */
 	struct notifier_block opp_nb;
 	int opp_cpu;
 	unsigned long opp_freq;
 };
 
-static LIST_HEAD(dvfs_info_list);
+static LIST_HEAD(drv_list);
 
-static struct mtk_cpu_dvfs_info *mtk_cpu_dvfs_info_lookup(int cpu)
+static struct mtk_cpufreq_drv *mtk_cpufreq_drv_lookup(int cpu)
 {
-	struct mtk_cpu_dvfs_info *info;
+	struct mtk_cpufreq_drv *drv;
 
-	list_for_each_entry(info, &dvfs_info_list, list_head) {
-		if (cpumask_test_cpu(cpu, &info->cpus))
-			return info;
+	list_for_each_entry(drv, &drv_list, list_head) {
+		if (cpumask_test_cpu(cpu, &drv->cpus))
+			return drv;
 	}
 
 	return NULL;
 }
 
-static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
-					int new_vproc)
+static int mtk_cpufreq_voltage_tracking(struct mtk_cpufreq_drv *drv,
+					int new_voltage)
 {
-	struct regulator *proc_reg = info->proc_reg;
-	struct regulator *sram_reg = info->sram_reg;
-	int old_vproc, old_vsram, new_vsram, vsram, vproc, ret;
-
-	old_vproc = regulator_get_voltage(proc_reg);
-	if (old_vproc < 0) {
-		pr_err("%s: invalid Vproc value: %d\n", __func__, old_vproc);
-		return old_vproc;
+	struct regulator *proc_reg = drv->proc_reg;
+	struct regulator *sram_reg = drv->sram_reg;
+	int old_voltage, old_vsram, new_vsram, vsram, voltage, ret;
+
+	old_voltage = regulator_get_voltage(proc_reg);
+	if (old_voltage < 0) {
+		pr_err("%s: invalid vproc value: %d\n", __func__, old_voltage);
+		return old_voltage;
 	}
 	/* Vsram should not exceed the maximum allowed voltage of SoC. */
-	new_vsram = min(new_vproc + MIN_VOLT_SHIFT, MAX_VOLT_LIMIT);
+	new_vsram = min(new_voltage + MIN_VOLT_SHIFT, MAX_VOLT_LIMIT);
 
-	if (old_vproc < new_vproc) {
+	if (old_voltage < new_voltage) {
 		/*
 		 * When scaling up voltages, Vsram and Vproc scale up step
 		 * by step. At each step, set Vsram to (Vproc + 200mV) first,
@@ -88,18 +87,18 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 		do {
 			old_vsram = regulator_get_voltage(sram_reg);
 			if (old_vsram < 0) {
-				pr_err("%s: invalid Vsram value: %d\n",
+				pr_err("%s: invalid vsram value: %d\n",
 				       __func__, old_vsram);
 				return old_vsram;
 			}
-			old_vproc = regulator_get_voltage(proc_reg);
-			if (old_vproc < 0) {
-				pr_err("%s: invalid Vproc value: %d\n",
-				       __func__, old_vproc);
-				return old_vproc;
+			old_voltage = regulator_get_voltage(proc_reg);
+			if (old_voltage < 0) {
+				pr_err("%s: invalid vproc value: %d\n",
+				       __func__, old_voltage);
+				return old_voltage;
 			}
 
-			vsram = min(new_vsram, old_vproc + MAX_VOLT_SHIFT);
+			vsram = min(new_vsram, old_voltage + MAX_VOLT_SHIFT);
 
 			if (vsram + VOLT_TOL >= MAX_VOLT_LIMIT) {
 				vsram = MAX_VOLT_LIMIT;
@@ -115,25 +114,25 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 							vsram - VOLT_TOL,
 							vsram);
 
-				vproc = new_vproc;
+				voltage = new_voltage;
 			} else {
 				ret = regulator_set_voltage(sram_reg, vsram,
 							    vsram + VOLT_TOL);
 
-				vproc = vsram - MIN_VOLT_SHIFT;
+				voltage = vsram - MIN_VOLT_SHIFT;
 			}
 			if (ret)
 				return ret;
 
-			ret = regulator_set_voltage(proc_reg, vproc,
-						    vproc + VOLT_TOL);
+			ret = regulator_set_voltage(proc_reg, voltage,
+						    voltage + VOLT_TOL);
 			if (ret) {
 				regulator_set_voltage(sram_reg, old_vsram,
 						      old_vsram);
 				return ret;
 			}
-		} while (vproc < new_vproc || vsram < new_vsram);
-	} else if (old_vproc > new_vproc) {
+		} while (voltage < new_voltage || vsram < new_vsram);
+	} else if (old_voltage > new_voltage) {
 		/*
 		 * When scaling down voltages, Vsram and Vproc scale down step
 		 * by step. At each step, set Vproc to (Vsram - 200mV) first,
@@ -141,29 +140,29 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 		 * Keep doing it until Vsram and Vproc hit target voltages.
 		 */
 		do {
-			old_vproc = regulator_get_voltage(proc_reg);
-			if (old_vproc < 0) {
-				pr_err("%s: invalid Vproc value: %d\n",
-				       __func__, old_vproc);
-				return old_vproc;
+			old_voltage = regulator_get_voltage(proc_reg);
+			if (old_voltage < 0) {
+				pr_err("%s: invalid vproc value: %d\n",
+				       __func__, old_voltage);
+				return old_voltage;
 			}
 			old_vsram = regulator_get_voltage(sram_reg);
 			if (old_vsram < 0) {
-				pr_err("%s: invalid Vsram value: %d\n",
+				pr_err("%s: invalid vsram value: %d\n",
 				       __func__, old_vsram);
 				return old_vsram;
 			}
 
-			vproc = max(new_vproc, old_vsram - MAX_VOLT_SHIFT);
-			ret = regulator_set_voltage(proc_reg, vproc,
-						    vproc + VOLT_TOL);
+			voltage = max(new_voltage, old_vsram - MAX_VOLT_SHIFT);
+			ret = regulator_set_voltage(proc_reg, voltage,
+						    voltage + VOLT_TOL);
 			if (ret)
 				return ret;
 
-			if (vproc == new_vproc)
+			if (voltage == new_voltage)
 				vsram = new_vsram;
 			else
-				vsram = max(new_vsram, vproc + MIN_VOLT_SHIFT);
+				vsram = max(new_vsram, voltage + MIN_VOLT_SHIFT);
 
 			if (vsram + VOLT_TOL >= MAX_VOLT_LIMIT) {
 				vsram = MAX_VOLT_LIMIT;
@@ -184,112 +183,107 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 			}
 
 			if (ret) {
-				regulator_set_voltage(proc_reg, old_vproc,
-						      old_vproc);
+				regulator_set_voltage(proc_reg, old_voltage,
+						      old_voltage);
 				return ret;
 			}
-		} while (vproc > new_vproc + VOLT_TOL ||
+		} while (voltage > new_voltage + VOLT_TOL ||
 			 vsram > new_vsram + VOLT_TOL);
 	}
 
 	return 0;
 }
 
-static int mtk_cpufreq_set_voltage(struct mtk_cpu_dvfs_info *info, int vproc)
+static int mtk_cpufreq_set_voltage(struct mtk_cpufreq_drv *drv, int voltage)
 {
 	int ret;
 
-	if (info->need_voltage_tracking)
-		ret = mtk_cpufreq_voltage_tracking(info, vproc);
+	if (drv->need_voltage_tracking)
+		ret = mtk_cpufreq_voltage_tracking(drv, voltage);
 	else
-		ret = regulator_set_voltage(info->proc_reg, vproc,
+		ret = regulator_set_voltage(drv->proc_reg, voltage,
 					    MAX_VOLT_LIMIT);
 	if (!ret)
-		info->old_vproc = vproc;
+		drv->old_voltage = voltage;
+
 	return ret;
 }
 
-static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
-				  unsigned int index)
+static int mtk_cpufreq_target_index(struct cpufreq_policy *policy,
+				    unsigned int index)
 {
 	struct cpufreq_frequency_table *freq_table = policy->freq_table;
 	struct clk *cpu_clk = policy->clk;
 	struct clk *armpll = clk_get_parent(cpu_clk);
-	struct mtk_cpu_dvfs_info *info = policy->driver_data;
-	struct device *cpu_dev = info->cpu_dev;
+	struct mtk_cpufreq_drv *drv = policy->driver_data;
+	struct device *cpu_dev = drv->cpu_dev;
 	struct dev_pm_opp *opp;
-	long freq_hz, old_freq_hz;
-	int vproc, old_vproc, inter_vproc, target_vproc, ret;
-
-	inter_vproc = info->intermediate_voltage;
-
-	old_freq_hz = clk_get_rate(cpu_clk);
-	old_vproc = info->old_vproc;
-	if (old_vproc == 0)
-		old_vproc = regulator_get_voltage(info->proc_reg);
-	if (old_vproc < 0) {
-		pr_err("%s: invalid Vproc value: %d\n", __func__, old_vproc);
-		return old_vproc;
-	}
+	unsigned long freq, old_freq;
+	int voltage, old_voltage, inter_voltage, target_voltage, ret;
 
-	freq_hz = freq_table[index].frequency * 1000;
+	inter_voltage = drv->inter_voltage;
 
-	opp = dev_pm_opp_find_freq_ceil(cpu_dev, &freq_hz);
+	freq = freq_table[index].frequency * 1000;
+	opp = dev_pm_opp_find_freq_ceil(cpu_dev, &freq);
 	if (IS_ERR(opp)) {
-		pr_err("cpu%d: failed to find OPP for %ld\n",
-		       policy->cpu, freq_hz);
+		pr_err("cpu%d: failed to find opp for freq:%ld\n",
+		       policy->cpu, freq);
 		return PTR_ERR(opp);
 	}
-	vproc = dev_pm_opp_get_voltage(opp);
+	voltage = dev_pm_opp_get_voltage(opp);
 	dev_pm_opp_put(opp);
 
-	mutex_lock(&info->lock);
-	/*
-	 * If the new voltage or the intermediate voltage is higher than the
-	 * current voltage, scale up voltage first.
-	 */
-	target_vproc = (inter_vproc > vproc) ? inter_vproc : vproc;
-	if (old_vproc < target_vproc) {
-		ret = mtk_cpufreq_set_voltage(info, target_vproc);
+	old_freq = clk_get_rate(cpu_clk);
+	old_voltage = drv->old_voltage;
+	if (old_voltage == 0)
+		old_voltage = regulator_get_voltage(drv->proc_reg);
+	if (old_voltage < 0) {
+		pr_err("cpu%d: invalid vproc value: %d\n",
+		       policy->cpu, old_voltage);
+		return old_voltage;
+	}
+
+	mutex_lock(&drv->lock);
+	/* scale up: set voltage first then freq. */
+	target_voltage = (inter_voltage > voltage) ? inter_voltage : voltage;
+	if (old_voltage < target_voltage) {
+		ret = mtk_cpufreq_set_voltage(drv, target_voltage);
 		if (ret) {
-			pr_err("cpu%d: failed to scale up voltage!\n",
+			pr_err("cpu%d: failed to scale up voltage\n",
 			       policy->cpu);
-			mtk_cpufreq_set_voltage(info, old_vproc);
-			mutex_unlock(&info->lock);
+			mtk_cpufreq_set_voltage(drv, old_voltage);
+			mutex_unlock(&drv->lock);
 			return ret;
 		}
 	}
 
-	/* Reparent the CPU clock to intermediate clock. */
-	ret = clk_set_parent(cpu_clk, info->inter_clk);
+	/* switch the cpu clock to intermediate clock source. */
+	ret = clk_set_parent(cpu_clk, drv->inter_clk);
 	if (ret) {
-		pr_err("cpu%d: failed to re-parent cpu clock!\n",
-		       policy->cpu);
-		mtk_cpufreq_set_voltage(info, old_vproc);
+		pr_err("cpu%d: failed to re-parent cpu clock\n", policy->cpu);
+		mtk_cpufreq_set_voltage(drv, old_voltage);
 		WARN_ON(1);
-		mutex_unlock(&info->lock);
+		mutex_unlock(&drv->lock);
 		return ret;
 	}
 
-	/* Set the original PLL to target rate. */
-	ret = clk_set_rate(armpll, freq_hz);
+	/* set the original clock to target rate. */
+	ret = clk_set_rate(armpll, freq);
 	if (ret) {
-		pr_err("cpu%d: failed to scale cpu clock rate!\n",
-		       policy->cpu);
+		pr_err("cpu%d: failed to scale cpu clock rate\n", policy->cpu);
 		clk_set_parent(cpu_clk, armpll);
-		mtk_cpufreq_set_voltage(info, old_vproc);
-		mutex_unlock(&info->lock);
+		mtk_cpufreq_set_voltage(drv, old_voltage);
+		mutex_unlock(&drv->lock);
 		return ret;
 	}
 
-	/* Set parent of CPU clock back to the original PLL. */
+	/* switch the cpu clock back to the original clock source. */
 	ret = clk_set_parent(cpu_clk, armpll);
 	if (ret) {
-		pr_err("cpu%d: failed to re-parent cpu clock!\n",
-		       policy->cpu);
-		mtk_cpufreq_set_voltage(info, inter_vproc);
+		pr_err("cpu%d: failed to re-parent cpu clock\n", policy->cpu);
+		mtk_cpufreq_set_voltage(drv, inter_voltage);
 		WARN_ON(1);
-		mutex_unlock(&info->lock);
+		mutex_unlock(&drv->lock);
 		return ret;
 	}
 
@@ -297,21 +291,21 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
 	 * If the new voltage is lower than the intermediate voltage or the
 	 * original voltage, scale down to the new voltage.
 	 */
-	if (vproc < inter_vproc || vproc < old_vproc) {
-		ret = mtk_cpufreq_set_voltage(info, vproc);
+	if (voltage < inter_voltage || voltage < old_voltage) {
+		ret = mtk_cpufreq_set_voltage(drv, voltage);
 		if (ret) {
-			pr_err("cpu%d: failed to scale down voltage!\n",
+			pr_err("cpu%d: failed to scale down voltage\n",
 			       policy->cpu);
-			clk_set_parent(cpu_clk, info->inter_clk);
-			clk_set_rate(armpll, old_freq_hz);
+			clk_set_parent(cpu_clk, drv->inter_clk);
+			clk_set_rate(armpll, old_freq);
 			clk_set_parent(cpu_clk, armpll);
-			mutex_unlock(&info->lock);
+			mutex_unlock(&drv->lock);
 			return ret;
 		}
 	}
 
-	info->opp_freq = freq_hz;
-	mutex_unlock(&info->lock);
+	drv->opp_freq = freq;
+	mutex_unlock(&drv->lock);
 
 	return 0;
 }
@@ -323,35 +317,35 @@ static int mtk_cpufreq_opp_notifier(struct notifier_block *nb,
 {
 	struct dev_pm_opp *opp = data;
 	struct dev_pm_opp *new_opp;
-	struct mtk_cpu_dvfs_info *info;
+	struct mtk_cpufreq_drv *drv;
 	unsigned long freq, volt;
 	struct cpufreq_policy *policy;
 	int ret = 0;
 
-	info = container_of(nb, struct mtk_cpu_dvfs_info, opp_nb);
+	drv = container_of(nb, struct mtk_cpufreq_drv, opp_nb);
 
 	if (event == OPP_EVENT_ADJUST_VOLTAGE) {
 		freq = dev_pm_opp_get_freq(opp);
 
-		mutex_lock(&info->lock);
-		if (info->opp_freq == freq) {
+		mutex_lock(&drv->lock);
+		if (drv->opp_freq == freq) {
 			volt = dev_pm_opp_get_voltage(opp);
-			ret = mtk_cpufreq_set_voltage(info, volt);
+			ret = mtk_cpufreq_set_voltage(drv, volt);
 			if (ret)
-				dev_err(info->cpu_dev, "failed to scale voltage: %d\n",
+				dev_err(drv->cpu_dev, "failed to scale voltage: %d\n",
 					ret);
 		}
-		mutex_unlock(&info->lock);
+		mutex_unlock(&drv->lock);
 	} else if (event == OPP_EVENT_DISABLE) {
 		freq = dev_pm_opp_get_freq(opp);
 		/* case of current opp item is disabled */
-		if (info->opp_freq == freq) {
+		if (drv->opp_freq == freq) {
 			freq = 1;
-			new_opp = dev_pm_opp_find_freq_ceil(info->cpu_dev,
+			new_opp = dev_pm_opp_find_freq_ceil(drv->cpu_dev,
 							    &freq);
 			if (!IS_ERR(new_opp)) {
 				dev_pm_opp_put(new_opp);
-				policy = cpufreq_cpu_get(info->opp_cpu);
+				policy = cpufreq_cpu_get(drv->opp_cpu);
 				if (policy) {
 					cpufreq_driver_target(policy,
 							      freq / 1000,
@@ -368,210 +362,192 @@ static int mtk_cpufreq_opp_notifier(struct notifier_block *nb,
 	return notifier_from_errno(ret);
 }
 
-static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
+static int mtk_cpufreq_drv_init(struct mtk_cpufreq_drv *drv, int cpu)
 {
 	struct device *cpu_dev;
-	struct regulator *proc_reg = ERR_PTR(-ENODEV);
-	struct regulator *sram_reg = ERR_PTR(-ENODEV);
-	struct clk *cpu_clk = ERR_PTR(-ENODEV);
-	struct clk *inter_clk = ERR_PTR(-ENODEV);
 	struct dev_pm_opp *opp;
 	unsigned long rate;
 	int ret;
 
 	cpu_dev = get_cpu_device(cpu);
 	if (!cpu_dev) {
-		pr_err("failed to get cpu%d device\n", cpu);
+		dev_err(cpu_dev, "cpu%d: failed to get cpu device\n", cpu);
 		return -ENODEV;
 	}
 
-	cpu_clk = clk_get(cpu_dev, "cpu");
-	if (IS_ERR(cpu_clk)) {
-		if (PTR_ERR(cpu_clk) == -EPROBE_DEFER)
-			pr_warn("cpu clk for cpu%d not ready, retry.\n", cpu);
-		else
-			pr_err("failed to get cpu clk for cpu%d\n", cpu);
+	drv->opp_cpu = cpu;
+	drv->cpu_dev = cpu_dev;
+	mutex_init(&drv->lock);
 
-		ret = PTR_ERR(cpu_clk);
-		return ret;
+	drv->cpu_clk = clk_get(cpu_dev, "cpu");
+	if (IS_ERR(drv->cpu_clk)) {
+		ret = PTR_ERR(drv->cpu_clk);
+		return dev_err_probe(cpu_dev, ret,
+				     "cpu%d: failed to get cpu clk\n", cpu);
 	}
 
-	inter_clk = clk_get(cpu_dev, "intermediate");
-	if (IS_ERR(inter_clk)) {
-		if (PTR_ERR(inter_clk) == -EPROBE_DEFER)
-			pr_warn("intermediate clk for cpu%d not ready, retry.\n",
-				cpu);
-		else
-			pr_err("failed to get intermediate clk for cpu%d\n",
-			       cpu);
-
-		ret = PTR_ERR(inter_clk);
+	drv->inter_clk = clk_get(cpu_dev, "intermediate");
+	if (IS_ERR(drv->inter_clk)) {
+		ret = PTR_ERR(drv->inter_clk);
+		dev_err_probe(cpu_dev, ret,
+			      "cpu%d: failed to get intermediate clk\n", cpu);
 		goto out_free_resources;
 	}
 
-	proc_reg = regulator_get_optional(cpu_dev, "proc");
-	if (IS_ERR(proc_reg)) {
-		if (PTR_ERR(proc_reg) == -EPROBE_DEFER)
-			pr_warn("proc regulator for cpu%d not ready, retry.\n",
-				cpu);
-		else
-			pr_err("failed to get proc regulator for cpu%d\n",
-			       cpu);
-
-		ret = PTR_ERR(proc_reg);
+	drv->proc_reg = regulator_get_optional(cpu_dev, "proc");
+	if (IS_ERR(drv->proc_reg)) {
+		ret = PTR_ERR(drv->proc_reg);
+		dev_err_probe(cpu_dev, ret,
+			      "cpu%d: failed to get proc regulator\n", cpu);
 		goto out_free_resources;
 	}
-	ret = regulator_enable(proc_reg);
+
+	ret = regulator_enable(drv->proc_reg);
 	if (ret) {
-		pr_warn("enable vproc for cpu%d fail\n", cpu);
+		dev_warn(cpu_dev, "cpu%d: failed to enable proc regulator\n", cpu);
 		goto out_free_resources;
 	}
 
 	/* Both presence and absence of sram regulator are valid cases. */
-	sram_reg = regulator_get_exclusive(cpu_dev, "sram");
+	drv->sram_reg = regulator_get_exclusive(cpu_dev, "sram");
 
 	/* Get OPP-sharing information from "operating-points-v2" bindings */
-	ret = dev_pm_opp_of_get_sharing_cpus(cpu_dev, &info->cpus);
+	ret = dev_pm_opp_of_get_sharing_cpus(cpu_dev, &drv->cpus);
 	if (ret) {
-		pr_err("failed to get OPP-sharing information for cpu%d\n",
-		       cpu);
+		dev_err(cpu_dev, "cpu%d: failed to get opp-sharing information\n",
+			cpu);
 		goto out_free_resources;
 	}
 
-	ret = dev_pm_opp_of_cpumask_add_table(&info->cpus);
+	ret = dev_pm_opp_of_cpumask_add_table(&drv->cpus);
 	if (ret) {
-		pr_warn("no OPP table for cpu%d\n", cpu);
+		dev_warn(cpu_dev, "cpu%d: failed to add opp table\n", cpu);
 		goto out_free_resources;
 	}
 
-	ret = clk_prepare_enable(cpu_clk);
+	ret = clk_prepare_enable(drv->cpu_clk);
 	if (ret)
 		goto out_free_opp_table;
 
-	ret = clk_prepare_enable(inter_clk);
+	ret = clk_prepare_enable(drv->inter_clk);
 	if (ret)
 		goto out_disable_mux_clock;
 
 	/* Search a safe voltage for intermediate frequency. */
-	rate = clk_get_rate(inter_clk);
+	rate = clk_get_rate(drv->inter_clk);
 	opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate);
 	if (IS_ERR(opp)) {
-		pr_err("failed to get intermediate opp for cpu%d\n", cpu);
+		dev_err(cpu_dev, "cpu%d: failed to get intermediate opp\n",
+			cpu);
 		ret = PTR_ERR(opp);
 		goto out_disable_inter_clock;
 	}
-	info->intermediate_voltage = dev_pm_opp_get_voltage(opp);
+	drv->inter_voltage = dev_pm_opp_get_voltage(opp);
 	dev_pm_opp_put(opp);
 
-	info->opp_cpu = cpu;
-	info->opp_nb.notifier_call = mtk_cpufreq_opp_notifier;
-	ret = dev_pm_opp_register_notifier(cpu_dev, &info->opp_nb);
+	drv->opp_nb.notifier_call = mtk_cpufreq_opp_notifier;
+	ret = dev_pm_opp_register_notifier(cpu_dev, &drv->opp_nb);
 	if (ret) {
-		pr_warn("cannot register opp notification\n");
+		dev_warn(cpu_dev, "cpu%d: failed to register opp notifier\n",
+			 cpu);
 		goto out_disable_inter_clock;
 	}
 
-	mutex_init(&info->lock);
-	info->cpu_dev = cpu_dev;
-	info->proc_reg = proc_reg;
-	info->sram_reg = IS_ERR(sram_reg) ? NULL : sram_reg;
-	info->cpu_clk = cpu_clk;
-	info->inter_clk = inter_clk;
-	info->opp_freq = clk_get_rate(cpu_clk);
+	drv->opp_freq = clk_get_rate(drv->cpu_clk);
 
 	/*
 	 * If SRAM regulator is present, software "voltage tracking" is needed
 	 * for this CPU power domain.
 	 */
-	info->need_voltage_tracking = !IS_ERR(sram_reg);
+	drv->need_voltage_tracking = !IS_ERR(drv->sram_reg);
 
 	return 0;
 
 out_disable_inter_clock:
-	clk_disable_unprepare(inter_clk);
+	clk_disable_unprepare(drv->inter_clk);
 
 out_disable_mux_clock:
-	clk_disable_unprepare(cpu_clk);
+	clk_disable_unprepare(drv->cpu_clk);
 
 out_free_opp_table:
-	dev_pm_opp_of_cpumask_remove_table(&info->cpus);
+	dev_pm_opp_of_cpumask_remove_table(&drv->cpus);
 
 out_free_resources:
-	if (!IS_ERR(proc_reg))
-		regulator_put(proc_reg);
-	if (!IS_ERR(sram_reg))
-		regulator_put(sram_reg);
-	if (!IS_ERR(cpu_clk))
-		clk_put(cpu_clk);
-	if (!IS_ERR(inter_clk))
-		clk_put(inter_clk);
+	if (!IS_ERR(drv->proc_reg))
+		regulator_put(drv->proc_reg);
+	if (!IS_ERR(drv->sram_reg))
+		regulator_put(drv->sram_reg);
+	if (!IS_ERR(drv->cpu_clk))
+		clk_put(drv->cpu_clk);
+	if (!IS_ERR(drv->inter_clk))
+		clk_put(drv->inter_clk);
 
 	return ret;
 }
 
-static void mtk_cpu_dvfs_info_release(struct mtk_cpu_dvfs_info *info)
+static void mtk_cpufreq_drv_release(struct mtk_cpufreq_drv *drv)
 {
-	if (!IS_ERR(info->proc_reg)) {
-		regulator_disable(info->proc_reg);
-		regulator_put(info->proc_reg);
+	if (!IS_ERR(drv->proc_reg)) {
+		regulator_disable(drv->proc_reg);
+		regulator_put(drv->proc_reg);
 	}
-	if (!IS_ERR(info->sram_reg))
-		regulator_put(info->sram_reg);
-	if (!IS_ERR(info->cpu_clk)) {
-		clk_disable_unprepare(info->cpu_clk);
-		clk_put(info->cpu_clk);
+	if (!IS_ERR(drv->sram_reg))
+		regulator_put(drv->sram_reg);
+	if (!IS_ERR(drv->cpu_clk)) {
+		clk_disable_unprepare(drv->cpu_clk);
+		clk_put(drv->cpu_clk);
 	}
-	if (!IS_ERR(info->inter_clk)) {
-		clk_disable_unprepare(info->inter_clk);
-		clk_put(info->inter_clk);
+	if (!IS_ERR(drv->inter_clk)) {
+		clk_disable_unprepare(drv->inter_clk);
+		clk_put(drv->inter_clk);
 	}
 
-	dev_pm_opp_of_cpumask_remove_table(&info->cpus);
+	dev_pm_opp_of_cpumask_remove_table(&drv->cpus);
 }
 
 static int mtk_cpufreq_init(struct cpufreq_policy *policy)
 {
-	struct mtk_cpu_dvfs_info *info;
+	struct mtk_cpufreq_drv *drv;
 	struct cpufreq_frequency_table *freq_table;
 	int ret;
 
-	info = mtk_cpu_dvfs_info_lookup(policy->cpu);
-	if (!info) {
-		pr_err("dvfs info for cpu%d is not initialized.\n",
+	drv = mtk_cpufreq_drv_lookup(policy->cpu);
+	if (!drv) {
+		pr_err("cpu%d: failed to initialize cpufreq drv\n",
 		       policy->cpu);
 		return -EINVAL;
 	}
 
-	ret = dev_pm_opp_init_cpufreq_table(info->cpu_dev, &freq_table);
+	ret = dev_pm_opp_init_cpufreq_table(drv->cpu_dev, &freq_table);
 	if (ret) {
-		pr_err("failed to init cpufreq table for cpu%d: %d\n",
+		pr_err("cpu%d: failed to initialize cpufreq table: %d\n",
 		       policy->cpu, ret);
 		return ret;
 	}
 
-	cpumask_copy(policy->cpus, &info->cpus);
+	cpumask_copy(policy->cpus, &drv->cpus);
 	policy->freq_table = freq_table;
-	policy->driver_data = info;
-	policy->clk = info->cpu_clk;
+	policy->driver_data = drv;
+	policy->clk = drv->cpu_clk;
 
 	return 0;
 }
 
 static int mtk_cpufreq_exit(struct cpufreq_policy *policy)
 {
-	struct mtk_cpu_dvfs_info *info = policy->driver_data;
+	struct mtk_cpufreq_drv *drv = policy->driver_data;
 
-	dev_pm_opp_free_cpufreq_table(info->cpu_dev, &policy->freq_table);
+	dev_pm_opp_free_cpufreq_table(drv->cpu_dev, &policy->freq_table);
 
 	return 0;
 }
 
-static struct cpufreq_driver mtk_cpufreq_driver = {
+static struct cpufreq_driver cpufreq_mtk_driver = {
 	.flags = CPUFREQ_NEED_INITIAL_FREQ_CHECK |
 		 CPUFREQ_HAVE_GOVERNOR_PER_POLICY |
 		 CPUFREQ_IS_COOLING_DEV,
 	.verify = cpufreq_generic_frequency_table_verify,
-	.target_index = mtk_cpufreq_set_target,
+	.target_index = mtk_cpufreq_target_index,
 	.get = cpufreq_generic_get,
 	.init = mtk_cpufreq_init,
 	.exit = mtk_cpufreq_exit,
@@ -582,55 +558,48 @@ static struct cpufreq_driver mtk_cpufreq_driver = {
 
 static int mtk_cpufreq_probe(struct platform_device *pdev)
 {
-	struct mtk_cpu_dvfs_info *info, *tmp;
+	struct mtk_cpufreq_drv *drv, *tmp;
 	int cpu, ret;
 
 	for_each_possible_cpu(cpu) {
-		info = mtk_cpu_dvfs_info_lookup(cpu);
-		if (info)
+		drv = mtk_cpufreq_drv_lookup(cpu);
+		if (drv)
 			continue;
 
-		info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
-		if (!info) {
+		drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL);
+		if (!drv) {
 			ret = -ENOMEM;
-			goto release_dvfs_info_list;
+			goto out_release_drv_list;
 		}
 
-		ret = mtk_cpu_dvfs_info_init(info, cpu);
+		ret = mtk_cpufreq_drv_init(drv, cpu);
 		if (ret) {
 			dev_err(&pdev->dev,
-				"failed to initialize dvfs info for cpu%d\n",
+				"cpu%d: failed to initialize cpufreq drv\n",
 				cpu);
-			goto release_dvfs_info_list;
+			goto out_release_drv_list;
 		}
 
-		list_add(&info->list_head, &dvfs_info_list);
+		list_add(&drv->list_head, &drv_list);
 	}
 
-	ret = cpufreq_register_driver(&mtk_cpufreq_driver);
+	ret = cpufreq_register_driver(&cpufreq_mtk_driver);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to register mtk cpufreq driver\n");
-		goto release_dvfs_info_list;
+		goto out_release_drv_list;
 	}
 
 	return 0;
 
-release_dvfs_info_list:
-	list_for_each_entry_safe(info, tmp, &dvfs_info_list, list_head) {
-		mtk_cpu_dvfs_info_release(info);
-		list_del(&info->list_head);
+out_release_drv_list:
+	list_for_each_entry_safe(drv, tmp, &drv_list, list_head) {
+		mtk_cpufreq_drv_release(drv);
+		list_del(&drv->list_head);
 	}
 
 	return ret;
 }
 
-static struct platform_driver mtk_cpufreq_platdrv = {
-	.driver = {
-		.name	= "mtk-cpufreq",
-	},
-	.probe		= mtk_cpufreq_probe,
-};
-
 /* List of machines supported by this driver */
 static const struct of_device_id mtk_cpufreq_machines[] __initconst = {
 	{ .compatible = "mediatek,mt2701", },
@@ -638,23 +607,27 @@ static const struct of_device_id mtk_cpufreq_machines[] __initconst = {
 	{ .compatible = "mediatek,mt7622", },
 	{ .compatible = "mediatek,mt7623", },
 	{ .compatible = "mediatek,mt8167", },
-	{ .compatible = "mediatek,mt817x", },
 	{ .compatible = "mediatek,mt8173", },
-	{ .compatible = "mediatek,mt8176", },
 	{ .compatible = "mediatek,mt8183", },
 	{ .compatible = "mediatek,mt8365", },
 	{ .compatible = "mediatek,mt8516", },
-
 	{ }
 };
 MODULE_DEVICE_TABLE(of, mtk_cpufreq_machines);
 
-static int __init mtk_cpufreq_driver_init(void)
+static struct platform_driver mtk_cpufreq_platdrv = {
+	.probe = mtk_cpufreq_probe,
+	.driver = {
+		.name = "mtk-cpufreq",
+	},
+};
+
+static int __init mtk_cpufreq_platdrv_init(void)
 {
 	struct device_node *np;
 	const struct of_device_id *match;
 	struct platform_device *pdev;
-	int err;
+	int ret;
 
 	np = of_find_node_by_path("/");
 	if (!np)
@@ -667,9 +640,9 @@ static int __init mtk_cpufreq_driver_init(void)
 		return -ENODEV;
 	}
 
-	err = platform_driver_register(&mtk_cpufreq_platdrv);
-	if (err)
-		return err;
+	ret = platform_driver_register(&mtk_cpufreq_platdrv);
+	if (ret)
+		return ret;
 
 	/*
 	 * Since there's no place to hold device registration code and no
@@ -686,8 +659,14 @@ static int __init mtk_cpufreq_driver_init(void)
 
 	return 0;
 }
-device_initcall(mtk_cpufreq_driver_init);
+module_init(mtk_cpufreq_platdrv_init)
+
+static void __exit mtk_cpufreq_platdrv_exit(void)
+{
+	platform_driver_unregister(&mtk_cpufreq_platdrv);
+}
+module_exit(mtk_cpufreq_platdrv_exit)
 
 MODULE_DESCRIPTION("MediaTek CPUFreq driver");
-MODULE_AUTHOR("Pi-Cheng Chen <pi-cheng.chen@linaro.org>");
+MODULE_AUTHOR("Jia-Wei Chang <jia-wei.chang@mediatek.com>");
 MODULE_LICENSE("GPL v2");
-- 
2.18.0

Re: [PATCH 3/4] cpufreq: mediatek: clean up cpufreq driver

[Krzysztof Kozlowski]

On 07/03/2022 13:21, Tim Chang wrote:
> cleanup of naming, print log and comments.
> 
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.corp-partner.google.com>
> ---
>  drivers/cpufreq/mediatek-cpufreq.c | 487 ++++++++++++++---------------
>  1 file changed, 233 insertions(+), 254 deletions(-)
> 
> diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
> index 8e9d706d8081..3f00c7eb01f1 100644
> --- a/drivers/cpufreq/mediatek-cpufreq.c
> +++ b/drivers/cpufreq/mediatek-cpufreq.c
> @@ -1,7 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /*
> - * Copyright (c) 2015 Linaro Ltd.
> - * Author: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
> + * Copyright (C) 2022 MediaTek Inc.

Removal of authorship and existing copyrights does not fit into "clean
up". Please explain this thoroughly.

>   */
>  
>  #include <linux/clk.h>
> @@ -22,7 +21,7 @@
>  #define VOLT_TOL		(10000)
>  
>  /*
> - * The struct mtk_cpu_dvfs_info holds necessary information for doing CPU DVFS
> + * The struct mtk_cpufreq_drv holds necessary information for doing CPU DVFS
>   * on each CPU power/clock domain of Mediatek SoCs. Each CPU cluster in
>   * Mediatek SoCs has two voltage inputs, Vproc and Vsram. In some cases the two
>   * voltage inputs need to be controlled under a hardware limitation:
> @@ -32,7 +31,7 @@
>   * needs to be switched to another stable PLL clock temporarily until
>   * the original PLL becomes stable at target frequency.
>   */
> -struct mtk_cpu_dvfs_info {
> +struct mtk_cpufreq_drv {
>  	struct cpumask cpus;
>  	struct device *cpu_dev;
>  	struct regulator *proc_reg;
> @@ -40,45 +39,45 @@ struct mtk_cpu_dvfs_info {
>  	struct clk *cpu_clk;
>  	struct clk *inter_clk;
>  	struct list_head list_head;
> -	int intermediate_voltage;
> +	int inter_voltage;
>  	bool need_voltage_tracking;
> -	int old_vproc;
> -	struct mutex lock; /* avoid notify and policy race condition */
> +	int old_voltage;
> +	struct mutex lock;  /* avoid notify and policy race condition */
>  	struct notifier_block opp_nb;
>  	int opp_cpu;
>  	unsigned long opp_freq;
>  };
>  
> -static LIST_HEAD(dvfs_info_list);
> +static LIST_HEAD(drv_list);
>  
> -static struct mtk_cpu_dvfs_info *mtk_cpu_dvfs_info_lookup(int cpu)
> +static struct mtk_cpufreq_drv *mtk_cpufreq_drv_lookup(int cpu)
>  {
> -	struct mtk_cpu_dvfs_info *info;
> +	struct mtk_cpufreq_drv *drv;
>  
> -	list_for_each_entry(info, &dvfs_info_list, list_head) {
> -		if (cpumask_test_cpu(cpu, &info->cpus))
> -			return info;
> +	list_for_each_entry(drv, &drv_list, list_head) {
> +		if (cpumask_test_cpu(cpu, &drv->cpus))
> +			return drv;>  	}
>  
>  	return NULL;
>  }
>  
> -static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
> -					int new_vproc)
> +static int mtk_cpufreq_voltage_tracking(struct mtk_cpufreq_drv *drv,
> +					int new_voltage)
>  {
> -	struct regulator *proc_reg = info->proc_reg;
> -	struct regulator *sram_reg = info->sram_reg;
> -	int old_vproc, old_vsram, new_vsram, vsram, vproc, ret;
> -
> -	old_vproc = regulator_get_voltage(proc_reg);
> -	if (old_vproc < 0) {
> -		pr_err("%s: invalid Vproc value: %d\n", __func__, old_vproc);
> -		return old_vproc;
> +	struct regulator *proc_reg = drv->proc_reg;
> +	struct regulator *sram_reg = drv->sram_reg;
> +	int old_voltage, old_vsram, new_vsram, vsram, voltage, ret;
> +
> +	old_voltage = regulator_get_voltage(proc_reg);
> +	if (old_voltage < 0) {
> +		pr_err("%s: invalid vproc value: %d\n", __func__, old_voltage);
> +		return old_voltage;


Several different changes in one commit. Please read the document
"Submitting patches".

(...)

> -MODULE_AUTHOR("Pi-Cheng Chen <pi-cheng.chen@linaro.org>");
> +MODULE_AUTHOR("Jia-Wei Chang <jia-wei.chang@mediatek.com>");

Ekhm, why? He was not the author?

Best regards,
Krzysztof

Re: [PATCH 3/4] cpufreq: mediatek: clean up cpufreq driver

[Jia-Wei Chang]

On Mon, 2022-03-07 at 20:02 +0100, Krzysztof Kozlowski wrote:
> On 07/03/2022 13:21, Tim Chang wrote:
> > cleanup of naming, print log and comments.
> > 
> > Signed-off-by: Jia-Wei Chang <
> > jia-wei.chang@mediatek.corp-partner.google.com>
> > ---
> >  drivers/cpufreq/mediatek-cpufreq.c | 487 ++++++++++++++-----------
> > ----
> >  1 file changed, 233 insertions(+), 254 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/mediatek-cpufreq.c
> > b/drivers/cpufreq/mediatek-cpufreq.c
> > index 8e9d706d8081..3f00c7eb01f1 100644
> > --- a/drivers/cpufreq/mediatek-cpufreq.c
> > +++ b/drivers/cpufreq/mediatek-cpufreq.c
> > @@ -1,7 +1,6 @@
> >  // SPDX-License-Identifier: GPL-2.0-only
> >  /*
> > - * Copyright (c) 2015 Linaro Ltd.
> > - * Author: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
> > + * Copyright (C) 2022 MediaTek Inc.
> 
> Removal of authorship and existing copyrights does not fit into
> "clean
> up". Please explain this thoroughly.

This is my mistake.
I will keep it as before and add myself as a new author.

> 
> >   */
> >  
> >  #include <linux/clk.h>
> > @@ -22,7 +21,7 @@
> >  #define VOLT_TOL		(10000)
> >  
> >  /*
> > - * The struct mtk_cpu_dvfs_info holds necessary information for
> > doing CPU DVFS
> > + * The struct mtk_cpufreq_drv holds necessary information for
> > doing CPU DVFS
> >   * on each CPU power/clock domain of Mediatek SoCs. Each CPU
> > cluster in
> >   * Mediatek SoCs has two voltage inputs, Vproc and Vsram. In some
> > cases the two
> >   * voltage inputs need to be controlled under a hardware
> > limitation:
> > @@ -32,7 +31,7 @@
> >   * needs to be switched to another stable PLL clock temporarily
> > until
> >   * the original PLL becomes stable at target frequency.
> >   */
> > -struct mtk_cpu_dvfs_info {
> > +struct mtk_cpufreq_drv {
> >  	struct cpumask cpus;
> >  	struct device *cpu_dev;
> >  	struct regulator *proc_reg;
> > @@ -40,45 +39,45 @@ struct mtk_cpu_dvfs_info {
> >  	struct clk *cpu_clk;
> >  	struct clk *inter_clk;
> >  	struct list_head list_head;
> > -	int intermediate_voltage;
> > +	int inter_voltage;
> >  	bool need_voltage_tracking;
> > -	int old_vproc;
> > -	struct mutex lock; /* avoid notify and policy race condition */
> > +	int old_voltage;
> > +	struct mutex lock;  /* avoid notify and policy race condition
> > */
> >  	struct notifier_block opp_nb;
> >  	int opp_cpu;
> >  	unsigned long opp_freq;
> >  };
> >  
> > -static LIST_HEAD(dvfs_info_list);
> > +static LIST_HEAD(drv_list);
> >  
> > -static struct mtk_cpu_dvfs_info *mtk_cpu_dvfs_info_lookup(int cpu)
> > +static struct mtk_cpufreq_drv *mtk_cpufreq_drv_lookup(int cpu)
> >  {
> > -	struct mtk_cpu_dvfs_info *info;
> > +	struct mtk_cpufreq_drv *drv;
> >  
> > -	list_for_each_entry(info, &dvfs_info_list, list_head) {
> > -		if (cpumask_test_cpu(cpu, &info->cpus))
> > -			return info;
> > +	list_for_each_entry(drv, &drv_list, list_head) {
> > +		if (cpumask_test_cpu(cpu, &drv->cpus))
> > +			return drv;>  	}
> >  
> >  	return NULL;
> >  }
> >  
> > -static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info
> > *info,
> > -					int new_vproc)
> > +static int mtk_cpufreq_voltage_tracking(struct mtk_cpufreq_drv
> > *drv,
> > +					int new_voltage)
> >  {
> > -	struct regulator *proc_reg = info->proc_reg;
> > -	struct regulator *sram_reg = info->sram_reg;
> > -	int old_vproc, old_vsram, new_vsram, vsram, vproc, ret;
> > -
> > -	old_vproc = regulator_get_voltage(proc_reg);
> > -	if (old_vproc < 0) {
> > -		pr_err("%s: invalid Vproc value: %d\n", __func__,
> > old_vproc);
> > -		return old_vproc;
> > +	struct regulator *proc_reg = drv->proc_reg;
> > +	struct regulator *sram_reg = drv->sram_reg;
> > +	int old_voltage, old_vsram, new_vsram, vsram, voltage, ret;
> > +
> > +	old_voltage = regulator_get_voltage(proc_reg);
> > +	if (old_voltage < 0) {
> > +		pr_err("%s: invalid vproc value: %d\n", __func__,
> > old_voltage);
> > +		return old_voltage;
> 
> 
> Several different changes in one commit. Please read the document
> "Submitting patches".
> 
> (...)

Sorry for my ignorance.
I will split the changes and send one change in one commit.

> 
> > -MODULE_AUTHOR("Pi-Cheng Chen <pi-cheng.chen@linaro.org>");
> > +MODULE_AUTHOR("Jia-Wei Chang <jia-wei.chang@mediatek.com>");
> 
> Ekhm, why? He was not the author?

This is my mistake.
I will keep it as before and add myself as a new author.

> 
> Best regards,
> Krzysztof

Re: [PATCH 3/4] cpufreq: mediatek: clean up cpufreq driver

[Viresh Kumar]

On 07-03-22, 20:21, Tim Chang wrote:
> cleanup of naming, print log and comments.

As told by Krzysztof, this should be broken into several patches. One
patch for each rename, if you really need to do that.

You shouldn't just change "err" to "ret". It is fine if the driver
used "err" at some places and "ret" at others, but otherwise there is
no need of that change.

This should be easier for us to review, we aren't going to dig into
every minute change here, that you don't describe in the message, and
see if something is wrong. Please make this easier for us.

This patch alone may end up in 5-10 different patches.

-- 
viresh

[PATCH 4/4] cpufreq: mediatek: add platform data and clean up voltage tracking logic

[Tim Chang]

1. add required header files and remove unnecessary header files.
2. some soc needs different min/max voltage shift and voltage tracking
   attributes. make these variables into platform data to support
   future soc.
3. add need_voltage_tracking variable to platforma data. if true, it
   indicates soc is required to realize the voltage tracking between
   voltage of sram and voltage of cpu by software approach. otherwise,
   the voltage tracking is realized by hardware approach.
4. add opp frequency look-up function as mtk_cpufreq_get() and
   registered in cpufreq framework.
5. update voltage_tracking() logic and drv_init(). in drv_init(), it
   always sets highest opp voltage before return. it could prevent from
   high-freqeuncy-low-voltage issue if two or more clients using the
   same regulator.

Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.corp-partner.google.com>
---
 drivers/cpufreq/mediatek-cpufreq.c | 332 +++++++++++++++--------------
 1 file changed, 167 insertions(+), 165 deletions(-)

diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
index 3f00c7eb01f1..35c653eb59c7 100644
--- a/drivers/cpufreq/mediatek-cpufreq.c
+++ b/drivers/cpufreq/mediatek-cpufreq.c
@@ -7,18 +7,15 @@
 #include <linux/cpu.h>
 #include <linux/cpufreq.h>
 #include <linux/cpumask.h>
+#include <linux/minmax.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/pm_opp.h>
 #include <linux/regulator/consumer.h>
-#include <linux/slab.h>
-#include <linux/thermal.h>
 
-#define MIN_VOLT_SHIFT		(100000)
-#define MAX_VOLT_SHIFT		(200000)
-#define MAX_VOLT_LIMIT		(1150000)
-#define VOLT_TOL		(10000)
+struct mtk_cpufreq_platform_data;
 
 /*
  * The struct mtk_cpufreq_drv holds necessary information for doing CPU DVFS
@@ -46,8 +43,20 @@ struct mtk_cpufreq_drv {
 	struct notifier_block opp_nb;
 	int opp_cpu;
 	unsigned long opp_freq;
+	const struct mtk_cpufreq_platform_data *soc_data;
 };
 
+struct mtk_cpufreq_platform_data {
+	int min_volt_shift;
+	int max_volt_shift;
+	int proc_max_volt;
+	int sram_min_volt;
+	int sram_max_volt;
+	bool need_voltage_tracking;
+};
+
+static struct platform_device *cpufreq_pdev;
+
 static LIST_HEAD(drv_list);
 
 static struct mtk_cpufreq_drv *mtk_cpufreq_drv_lookup(int cpu)
@@ -62,9 +71,21 @@ static struct mtk_cpufreq_drv *mtk_cpufreq_drv_lookup(int cpu)
 	return NULL;
 }
 
+static unsigned int mtk_cpufreq_get(unsigned int cpu)
+{
+	struct mtk_cpufreq_drv *drv;
+
+	drv = mtk_cpufreq_drv_lookup(cpu);
+	if (!drv)
+		return 0;
+
+	return drv->opp_freq / 1000;
+}
+
 static int mtk_cpufreq_voltage_tracking(struct mtk_cpufreq_drv *drv,
 					int new_voltage)
 {
+	const struct mtk_cpufreq_platform_data *soc_data = drv->soc_data;
 	struct regulator *proc_reg = drv->proc_reg;
 	struct regulator *sram_reg = drv->sram_reg;
 	int old_voltage, old_vsram, new_vsram, vsram, voltage, ret;
@@ -74,122 +95,65 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpufreq_drv *drv,
 		pr_err("%s: invalid vproc value: %d\n", __func__, old_voltage);
 		return old_voltage;
 	}
-	/* Vsram should not exceed the maximum allowed voltage of SoC. */
-	new_vsram = min(new_voltage + MIN_VOLT_SHIFT, MAX_VOLT_LIMIT);
-
-	if (old_voltage < new_voltage) {
-		/*
-		 * When scaling up voltages, Vsram and Vproc scale up step
-		 * by step. At each step, set Vsram to (Vproc + 200mV) first,
-		 * then set Vproc to (Vsram - 100mV).
-		 * Keep doing it until Vsram and Vproc hit target voltages.
-		 */
-		do {
-			old_vsram = regulator_get_voltage(sram_reg);
-			if (old_vsram < 0) {
-				pr_err("%s: invalid vsram value: %d\n",
-				       __func__, old_vsram);
-				return old_vsram;
-			}
-			old_voltage = regulator_get_voltage(proc_reg);
-			if (old_voltage < 0) {
-				pr_err("%s: invalid vproc value: %d\n",
-				       __func__, old_voltage);
-				return old_voltage;
-			}
 
-			vsram = min(new_vsram, old_voltage + MAX_VOLT_SHIFT);
-
-			if (vsram + VOLT_TOL >= MAX_VOLT_LIMIT) {
-				vsram = MAX_VOLT_LIMIT;
+	old_vsram = regulator_get_voltage(sram_reg);
+	if (old_vsram < 0) {
+		pr_err("%s: invalid vsram value: %d\n", __func__, old_vsram);
+		return old_vsram;
+	}
 
-				/*
-				 * If the target Vsram hits the maximum voltage,
-				 * try to set the exact voltage value first.
-				 */
-				ret = regulator_set_voltage(sram_reg, vsram,
-							    vsram);
-				if (ret)
-					ret = regulator_set_voltage(sram_reg,
-							vsram - VOLT_TOL,
-							vsram);
+	new_vsram = clamp(new_voltage + soc_data->min_volt_shift,
+			  soc_data->sram_min_volt, soc_data->sram_max_volt);
 
-				voltage = new_voltage;
-			} else {
-				ret = regulator_set_voltage(sram_reg, vsram,
-							    vsram + VOLT_TOL);
+	do {
+		if (old_voltage <= new_voltage) {
+			vsram = clamp(old_voltage + soc_data->max_volt_shift,
+				      soc_data->sram_min_volt, new_vsram);
+			ret = regulator_set_voltage(sram_reg, vsram,
+						    soc_data->sram_max_volt);
 
-				voltage = vsram - MIN_VOLT_SHIFT;
-			}
 			if (ret)
 				return ret;
 
+			if (vsram == soc_data->sram_max_volt ||
+			    new_vsram == soc_data->sram_min_volt)
+				voltage = new_voltage;
+			else
+				voltage = vsram - soc_data->min_volt_shift;
+
 			ret = regulator_set_voltage(proc_reg, voltage,
-						    voltage + VOLT_TOL);
+						    soc_data->proc_max_volt);
 			if (ret) {
 				regulator_set_voltage(sram_reg, old_vsram,
-						      old_vsram);
+						      soc_data->sram_max_volt);
 				return ret;
 			}
-		} while (voltage < new_voltage || vsram < new_vsram);
-	} else if (old_voltage > new_voltage) {
-		/*
-		 * When scaling down voltages, Vsram and Vproc scale down step
-		 * by step. At each step, set Vproc to (Vsram - 200mV) first,
-		 * then set Vproc to (Vproc + 100mV).
-		 * Keep doing it until Vsram and Vproc hit target voltages.
-		 */
-		do {
-			old_voltage = regulator_get_voltage(proc_reg);
-			if (old_voltage < 0) {
-				pr_err("%s: invalid vproc value: %d\n",
-				       __func__, old_voltage);
-				return old_voltage;
-			}
-			old_vsram = regulator_get_voltage(sram_reg);
-			if (old_vsram < 0) {
-				pr_err("%s: invalid vsram value: %d\n",
-				       __func__, old_vsram);
-				return old_vsram;
-			}
-
-			voltage = max(new_voltage, old_vsram - MAX_VOLT_SHIFT);
+		} else if (old_voltage > new_voltage) {
+			voltage = max(new_voltage,
+				    old_vsram - soc_data->max_volt_shift);
 			ret = regulator_set_voltage(proc_reg, voltage,
-						    voltage + VOLT_TOL);
+						    soc_data->proc_max_volt);
 			if (ret)
 				return ret;
 
 			if (voltage == new_voltage)
 				vsram = new_vsram;
 			else
-				vsram = max(new_vsram, voltage + MIN_VOLT_SHIFT);
-
-			if (vsram + VOLT_TOL >= MAX_VOLT_LIMIT) {
-				vsram = MAX_VOLT_LIMIT;
-
-				/*
-				 * If the target Vsram hits the maximum voltage,
-				 * try to set the exact voltage value first.
-				 */
-				ret = regulator_set_voltage(sram_reg, vsram,
-							    vsram);
-				if (ret)
-					ret = regulator_set_voltage(sram_reg,
-							vsram - VOLT_TOL,
-							vsram);
-			} else {
-				ret = regulator_set_voltage(sram_reg, vsram,
-							    vsram + VOLT_TOL);
-			}
+				vsram = max(new_vsram,
+					    voltage + soc_data->min_volt_shift);
 
+			ret = regulator_set_voltage(sram_reg, vsram,
+						    soc_data->sram_max_volt);
 			if (ret) {
 				regulator_set_voltage(proc_reg, old_voltage,
-						      old_voltage);
+						      soc_data->proc_max_volt);
 				return ret;
 			}
-		} while (voltage > new_voltage + VOLT_TOL ||
-			 vsram > new_vsram + VOLT_TOL);
-	}
+		}
+
+		old_voltage = voltage;
+		old_vsram = vsram;
+	} while (voltage != new_voltage || vsram != new_vsram);
 
 	return 0;
 }
@@ -198,11 +162,12 @@ static int mtk_cpufreq_set_voltage(struct mtk_cpufreq_drv *drv, int voltage)
 {
 	int ret;
 
-	if (drv->need_voltage_tracking)
+	if (drv->soc_data->need_voltage_tracking)
 		ret = mtk_cpufreq_voltage_tracking(drv, voltage);
 	else
 		ret = regulator_set_voltage(drv->proc_reg, voltage,
-					    MAX_VOLT_LIMIT);
+					    drv->soc_data->proc_max_volt);
+
 	if (!ret)
 		drv->old_voltage = voltage;
 
@@ -218,7 +183,7 @@ static int mtk_cpufreq_target_index(struct cpufreq_policy *policy,
 	struct mtk_cpufreq_drv *drv = policy->driver_data;
 	struct device *cpu_dev = drv->cpu_dev;
 	struct dev_pm_opp *opp;
-	unsigned long freq, old_freq;
+	unsigned long freq;
 	int voltage, old_voltage, inter_voltage, target_voltage, ret;
 
 	inter_voltage = drv->inter_voltage;
@@ -233,7 +198,6 @@ static int mtk_cpufreq_target_index(struct cpufreq_policy *policy,
 	voltage = dev_pm_opp_get_voltage(opp);
 	dev_pm_opp_put(opp);
 
-	old_freq = clk_get_rate(cpu_clk);
 	old_voltage = drv->old_voltage;
 	if (old_voltage == 0)
 		old_voltage = regulator_get_voltage(drv->proc_reg);
@@ -244,9 +208,10 @@ static int mtk_cpufreq_target_index(struct cpufreq_policy *policy,
 	}
 
 	mutex_lock(&drv->lock);
+
 	/* scale up: set voltage first then freq. */
-	target_voltage = (inter_voltage > voltage) ? inter_voltage : voltage;
-	if (old_voltage < target_voltage) {
+	target_voltage = max(inter_voltage, voltage);
+	if (old_voltage <= target_voltage) {
 		ret = mtk_cpufreq_set_voltage(drv, target_voltage);
 		if (ret) {
 			pr_err("cpu%d: failed to scale up voltage\n",
@@ -296,9 +261,7 @@ static int mtk_cpufreq_target_index(struct cpufreq_policy *policy,
 		if (ret) {
 			pr_err("cpu%d: failed to scale down voltage\n",
 			       policy->cpu);
-			clk_set_parent(cpu_clk, drv->inter_clk);
-			clk_set_rate(armpll, old_freq);
-			clk_set_parent(cpu_clk, armpll);
+			WARN_ON(1);
 			mutex_unlock(&drv->lock);
 			return ret;
 		}
@@ -364,12 +327,11 @@ static int mtk_cpufreq_opp_notifier(struct notifier_block *nb,
 
 static int mtk_cpufreq_drv_init(struct mtk_cpufreq_drv *drv, int cpu)
 {
-	struct device *cpu_dev;
+	struct device *cpu_dev = get_cpu_device(cpu);
 	struct dev_pm_opp *opp;
-	unsigned long rate;
+	unsigned long rate, opp_volt;
 	int ret;
 
-	cpu_dev = get_cpu_device(cpu);
 	if (!cpu_dev) {
 		dev_err(cpu_dev, "cpu%d: failed to get cpu device\n", cpu);
 		return -ENODEV;
@@ -382,8 +344,9 @@ static int mtk_cpufreq_drv_init(struct mtk_cpufreq_drv *drv, int cpu)
 	drv->cpu_clk = clk_get(cpu_dev, "cpu");
 	if (IS_ERR(drv->cpu_clk)) {
 		ret = PTR_ERR(drv->cpu_clk);
-		return dev_err_probe(cpu_dev, ret,
-				     "cpu%d: failed to get cpu clk\n", cpu);
+		dev_err_probe(cpu_dev, ret, "cpu%d: failed to get cpu clk\n",
+			      cpu);
+		goto out_free_resources;
 	}
 
 	drv->inter_clk = clk_get(cpu_dev, "intermediate");
@@ -394,6 +357,23 @@ static int mtk_cpufreq_drv_init(struct mtk_cpufreq_drv *drv, int cpu)
 		goto out_free_resources;
 	}
 
+	if (drv->soc_data->need_voltage_tracking) {
+		drv->sram_reg = regulator_get_optional(cpu_dev, "sram");
+		if (IS_ERR_OR_NULL(drv->sram_reg)) {
+			ret = PTR_ERR(drv->sram_reg);
+			dev_err_probe(cpu_dev, ret,
+				      "cpu%d: failed to get sram regulator\n",
+				      cpu);
+			goto out_free_resources;
+		}
+
+		ret = regulator_enable(drv->sram_reg);
+		if (ret) {
+			dev_warn(cpu_dev, "cpu%d: failed to enable sram regulator\n", cpu);
+			goto out_free_resources;
+		}
+	}
+
 	drv->proc_reg = regulator_get_optional(cpu_dev, "proc");
 	if (IS_ERR(drv->proc_reg)) {
 		ret = PTR_ERR(drv->proc_reg);
@@ -408,10 +388,14 @@ static int mtk_cpufreq_drv_init(struct mtk_cpufreq_drv *drv, int cpu)
 		goto out_free_resources;
 	}
 
-	/* Both presence and absence of sram regulator are valid cases. */
-	drv->sram_reg = regulator_get_exclusive(cpu_dev, "sram");
+	ret = clk_prepare_enable(drv->cpu_clk);
+	if (ret)
+		goto out_free_opp_table;
+
+	ret = clk_prepare_enable(drv->inter_clk);
+	if (ret)
+		goto out_free_opp_table;
 
-	/* Get OPP-sharing information from "operating-points-v2" bindings */
 	ret = dev_pm_opp_of_get_sharing_cpus(cpu_dev, &drv->cpus);
 	if (ret) {
 		dev_err(cpu_dev, "cpu%d: failed to get opp-sharing information\n",
@@ -425,62 +409,66 @@ static int mtk_cpufreq_drv_init(struct mtk_cpufreq_drv *drv, int cpu)
 		goto out_free_resources;
 	}
 
-	ret = clk_prepare_enable(drv->cpu_clk);
-	if (ret)
-		goto out_free_opp_table;
-
-	ret = clk_prepare_enable(drv->inter_clk);
-	if (ret)
-		goto out_disable_mux_clock;
+	drv->opp_freq = clk_get_rate(drv->cpu_clk);
 
-	/* Search a safe voltage for intermediate frequency. */
 	rate = clk_get_rate(drv->inter_clk);
 	opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate);
 	if (IS_ERR(opp)) {
+		ret = PTR_ERR(opp);
 		dev_err(cpu_dev, "cpu%d: failed to get intermediate opp\n",
 			cpu);
-		ret = PTR_ERR(opp);
-		goto out_disable_inter_clock;
+		goto out_free_opp_table;
 	}
 	drv->inter_voltage = dev_pm_opp_get_voltage(opp);
 	dev_pm_opp_put(opp);
 
+	rate = U32_MAX;
+	opp = dev_pm_opp_find_freq_floor(drv->cpu_dev, &rate);
+	if (IS_ERR(opp)) {
+		ret = PTR_ERR(opp);
+		dev_err(cpu_dev, "cpu%d: failed to get opp\n", drv->opp_cpu);
+		goto out_free_opp_table;
+	}
+
+	opp_volt = dev_pm_opp_get_voltage(opp);
+	dev_pm_opp_put(opp);
+	ret = mtk_cpufreq_set_voltage(drv, opp_volt);
+	if (ret) {
+		dev_err(cpu_dev, "cpu%d: failed to scale to highest voltage %lu in proc_reg\n",
+			drv->opp_cpu, opp_volt);
+		goto out_free_opp_table;
+	}
+
 	drv->opp_nb.notifier_call = mtk_cpufreq_opp_notifier;
 	ret = dev_pm_opp_register_notifier(cpu_dev, &drv->opp_nb);
 	if (ret) {
 		dev_warn(cpu_dev, "cpu%d: failed to register opp notifier\n",
 			 cpu);
-		goto out_disable_inter_clock;
+		goto out_free_opp_table;
 	}
 
-	drv->opp_freq = clk_get_rate(drv->cpu_clk);
-
-	/*
-	 * If SRAM regulator is present, software "voltage tracking" is needed
-	 * for this CPU power domain.
-	 */
-	drv->need_voltage_tracking = !IS_ERR(drv->sram_reg);
-
 	return 0;
 
-out_disable_inter_clock:
-	clk_disable_unprepare(drv->inter_clk);
-
-out_disable_mux_clock:
-	clk_disable_unprepare(drv->cpu_clk);
-
 out_free_opp_table:
 	dev_pm_opp_of_cpumask_remove_table(&drv->cpus);
 
 out_free_resources:
-	if (!IS_ERR(drv->proc_reg))
+	if (!IS_ERR(drv->proc_reg)) {
+		regulator_disable(drv->proc_reg);
 		regulator_put(drv->proc_reg);
-	if (!IS_ERR(drv->sram_reg))
+	}
+	if (!IS_ERR(drv->sram_reg)) {
+		regulator_disable(drv->sram_reg);
 		regulator_put(drv->sram_reg);
-	if (!IS_ERR(drv->cpu_clk))
+	}
+	if (!IS_ERR(drv->cpu_clk)) {
+		clk_disable_unprepare(drv->cpu_clk);
 		clk_put(drv->cpu_clk);
-	if (!IS_ERR(drv->inter_clk))
+	}
+	if (!IS_ERR(drv->inter_clk)) {
+		clk_disable_unprepare(drv->inter_clk);
 		clk_put(drv->inter_clk);
+	}
 
 	return ret;
 }
@@ -491,8 +479,10 @@ static void mtk_cpufreq_drv_release(struct mtk_cpufreq_drv *drv)
 		regulator_disable(drv->proc_reg);
 		regulator_put(drv->proc_reg);
 	}
-	if (!IS_ERR(drv->sram_reg))
+	if (!IS_ERR(drv->sram_reg)) {
+		regulator_disable(drv->sram_reg);
 		regulator_put(drv->sram_reg);
+	}
 	if (!IS_ERR(drv->cpu_clk)) {
 		clk_disable_unprepare(drv->cpu_clk);
 		clk_put(drv->cpu_clk);
@@ -548,7 +538,7 @@ static struct cpufreq_driver cpufreq_mtk_driver = {
 		 CPUFREQ_IS_COOLING_DEV,
 	.verify = cpufreq_generic_frequency_table_verify,
 	.target_index = mtk_cpufreq_target_index,
-	.get = cpufreq_generic_get,
+	.get = mtk_cpufreq_get,
 	.init = mtk_cpufreq_init,
 	.exit = mtk_cpufreq_exit,
 	.register_em = cpufreq_register_em_with_opp,
@@ -558,9 +548,16 @@ static struct cpufreq_driver cpufreq_mtk_driver = {
 
 static int mtk_cpufreq_probe(struct platform_device *pdev)
 {
+	const struct of_device_id *match;
 	struct mtk_cpufreq_drv *drv, *tmp;
 	int cpu, ret;
 
+	match = dev_get_platdata(&pdev->dev);
+	if (!match || !match->data) {
+		dev_err(&pdev->dev, "no mtk cpufreq platform data?\n");
+		return -ENODEV;
+	}
+
 	for_each_possible_cpu(cpu) {
 		drv = mtk_cpufreq_drv_lookup(cpu);
 		if (drv)
@@ -572,6 +569,7 @@ static int mtk_cpufreq_probe(struct platform_device *pdev)
 			goto out_release_drv_list;
 		}
 
+		drv->soc_data = (const struct mtk_cpufreq_platform_data *)match->data;
 		ret = mtk_cpufreq_drv_init(drv, cpu);
 		if (ret) {
 			dev_err(&pdev->dev,
@@ -600,17 +598,26 @@ static int mtk_cpufreq_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static const struct mtk_cpufreq_platform_data mtk_platform_data = {
+	.min_volt_shift = 0,
+	.max_volt_shift = 0,
+	.proc_max_volt = 1150000,
+	.sram_min_volt = 0,
+	.sram_max_volt = 0,
+	.need_voltage_tracking = false,
+};
+
 /* List of machines supported by this driver */
 static const struct of_device_id mtk_cpufreq_machines[] __initconst = {
-	{ .compatible = "mediatek,mt2701", },
-	{ .compatible = "mediatek,mt2712", },
-	{ .compatible = "mediatek,mt7622", },
-	{ .compatible = "mediatek,mt7623", },
-	{ .compatible = "mediatek,mt8167", },
-	{ .compatible = "mediatek,mt8173", },
-	{ .compatible = "mediatek,mt8183", },
-	{ .compatible = "mediatek,mt8365", },
-	{ .compatible = "mediatek,mt8516", },
+	{ .compatible = "mediatek,mt2701", .data = &mtk_platform_data },
+	{ .compatible = "mediatek,mt2712", .data = &mtk_platform_data },
+	{ .compatible = "mediatek,mt7622", .data = &mtk_platform_data },
+	{ .compatible = "mediatek,mt7623", .data = &mtk_platform_data },
+	{ .compatible = "mediatek,mt8167", .data = &mtk_platform_data },
+	{ .compatible = "mediatek,mt8173", .data = &mtk_platform_data },
+	{ .compatible = "mediatek,mt8183", .data = &mtk_platform_data },
+	{ .compatible = "mediatek,mt8365", .data = &mtk_platform_data },
+	{ .compatible = "mediatek,mt8516", .data = &mtk_platform_data },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, mtk_cpufreq_machines);
@@ -626,7 +633,6 @@ static int __init mtk_cpufreq_platdrv_init(void)
 {
 	struct device_node *np;
 	const struct of_device_id *match;
-	struct platform_device *pdev;
 	int ret;
 
 	np = of_find_node_by_path("/");
@@ -644,17 +650,12 @@ static int __init mtk_cpufreq_platdrv_init(void)
 	if (ret)
 		return ret;
 
-	/*
-	 * Since there's no place to hold device registration code and no
-	 * device tree based way to match cpufreq driver yet, both the driver
-	 * and the device registration codes are put here to handle defer
-	 * probing.
-	 */
-	pdev = platform_device_register_simple("mtk-cpufreq", -1, NULL, 0);
-	if (IS_ERR(pdev)) {
+	cpufreq_pdev = platform_device_register_data(NULL, "mtk-cpufreq", -1,
+						     match, sizeof(*match));
+	if (IS_ERR(cpufreq_pdev)) {
 		pr_err("failed to register mtk-cpufreq platform device\n");
 		platform_driver_unregister(&mtk_cpufreq_platdrv);
-		return PTR_ERR(pdev);
+		return PTR_ERR(cpufreq_pdev);
 	}
 
 	return 0;
@@ -663,6 +664,7 @@ module_init(mtk_cpufreq_platdrv_init)
 
 static void __exit mtk_cpufreq_platdrv_exit(void)
 {
+	platform_device_unregister(cpufreq_pdev);
 	platform_driver_unregister(&mtk_cpufreq_platdrv);
 }
 module_exit(mtk_cpufreq_platdrv_exit)
-- 
2.18.0

Re: [PATCH 4/4] cpufreq: mediatek: add platform data and clean up voltage tracking logic

[Krzysztof Kozlowski]

On 07/03/2022 13:21, Tim Chang wrote:
> 1. add required header files and remove unnecessary header files.
> 2. some soc needs different min/max voltage shift and voltage tracking
>    attributes. make these variables into platform data to support
>    future soc.
> 3. add need_voltage_tracking variable to platforma data. if true, it
>    indicates soc is required to realize the voltage tracking between
>    voltage of sram and voltage of cpu by software approach. otherwise,
>    the voltage tracking is realized by hardware approach.
> 4. add opp frequency look-up function as mtk_cpufreq_get() and
>    registered in cpufreq framework.
> 5. update voltage_tracking() logic and drv_init(). in drv_init(), it
>    always sets highest opp voltage before return. it could prevent from
>    high-freqeuncy-low-voltage issue if two or more clients using the
>    same regulator.

One change at a time.

> 
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.corp-partner.google.com>

Your SoB does not match from field.

Best regards,
Krzysztof

Re: [PATCH 4/4] cpufreq: mediatek: add platform data and clean up voltage tracking logic

[Jia-Wei Chang]

On Mon, 2022-03-07 at 20:03 +0100, Krzysztof Kozlowski wrote:
> On 07/03/2022 13:21, Tim Chang wrote:
> > 1. add required header files and remove unnecessary header files.
> > 2. some soc needs different min/max voltage shift and voltage
> > tracking
> >    attributes. make these variables into platform data to support
> >    future soc.
> > 3. add need_voltage_tracking variable to platforma data. if true,
> > it
> >    indicates soc is required to realize the voltage tracking
> > between
> >    voltage of sram and voltage of cpu by software approach.
> > otherwise,
> >    the voltage tracking is realized by hardware approach.
> > 4. add opp frequency look-up function as mtk_cpufreq_get() and
> >    registered in cpufreq framework.
> > 5. update voltage_tracking() logic and drv_init(). in drv_init(),
> > it
> >    always sets highest opp voltage before return. it could prevent
> > from
> >    high-freqeuncy-low-voltage issue if two or more clients using
> > the
> >    same regulator.
> 
> One change at a time.

Sure, I will split the change and send one change in one commit.

> 
> > 
> > Signed-off-by: Jia-Wei Chang <
> > jia-wei.chang@mediatek.corp-partner.google.com>
> 
> Your SoB does not match from field.

I will update it for the whole series in next version.

> 
> Best regards,
> Krzysztof

Re: [PATCH 0/4] cpufreq: mediatek: introduce mtk cpufreq

[Viresh Kumar]

On 07-03-22, 20:21, Tim Chang wrote:
> CPUFREQ is DVFS driver used for power saving by scaling clock frequency
> and supply voltage of CPUs. This module cooperates with CCI DEVFREQ for
> certain Mediatek SoCs.

Both subject and this log talks as if you are adding a new cpufreq
driver, while what you are doing is just cleanup mostly. This isn't
how it should be done.

You need to be very explicit with what you are doing and make that
change in a separate patch. The cover letter should tell what you are
doing and why.

-- 
viresh

Re: [PATCH 0/4] cpufreq: mediatek: introduce mtk cpufreq

[Rex-BC Chen]

On Tue, 2022-03-08 at 10:06 +0530, Viresh Kumar wrote:
> On 07-03-22, 20:21, Tim Chang wrote:
> > CPUFREQ is DVFS driver used for power saving by scaling clock
> > frequency
> > and supply voltage of CPUs. This module cooperates with CCI DEVFREQ
> > for
> > certain Mediatek SoCs.
> 
> Both subject and this log talks as if you are adding a new cpufreq
> driver, while what you are doing is just cleanup mostly. This isn't
> how it should be done.
> 
> You need to be very explicit with what you are doing and make that
> change in a separate patch. The cover letter should tell what you are
> doing and why.
> 

Hello Viresh,

Thanks for your suggestion.
Indeed, the subject is not proper for this series.
I will help to upstream next version and fix the issue because of
resource issues.

Thanks.

BRs,
Rex