diff mbox series

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

Message ID 20220307122151.11666-2-jia-wei.chang@mediatek.com (mailing list archive)
State New, archived
Headers show
Series cpufreq: mediatek: introduce mtk cpufreq | expand

Commit Message

Jia-wei Chang (張佳偉) March 7, 2022, 12:21 p.m. UTC
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

Comments

Krzysztof Kozlowski March 7, 2022, 6:57 p.m. UTC | #1
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
Jia-wei Chang (張佳偉) March 24, 2022, 9:38 a.m. UTC | #2
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
Krzysztof Kozlowski March 24, 2022, 10:33 a.m. UTC | #3
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
Jia-wei Chang (張佳偉) April 1, 2022, 1:26 p.m. UTC | #4
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
Krzysztof Kozlowski April 1, 2022, 4:32 p.m. UTC | #5
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
Jia-wei Chang (張佳偉) April 6, 2022, 8:42 a.m. UTC | #6
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
Rex-BC Chen (陳柏辰) April 8, 2022, 3:14 a.m. UTC | #7
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
diff mbox series

Patch

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>;
+      };
+
+      /* ... */
+
+    };