diff mbox series

[v2,1/2] dt-bindings: devfreq: mediatek: Add mtk cci devfreq dt-bindings

Message ID 20220408052150.22536-2-johnson.wang@mediatek.com (mailing list archive)
State New, archived
Headers show
Series Introduce MediaTek CCI devfreq driver | expand

Commit Message

Johnson Wang April 8, 2022, 5:21 a.m. UTC
Add devicetree binding of mtk cci devfreq on MediaTek SoC.

Signed-off-by: Johnson Wang <johnson.wang@mediatek.com>
Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
---
 .../devicetree/bindings/devfreq/mtk-cci.yaml  | 72 +++++++++++++++++++
 1 file changed, 72 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/devfreq/mtk-cci.yaml

Comments

Krzysztof Kozlowski April 8, 2022, 8:17 a.m. UTC | #1
On 08/04/2022 07:21, Johnson Wang wrote:
> Add devicetree binding of mtk cci devfreq on MediaTek SoC.
> 

Thank you for your patch. There is something to discuss/improve.

> Signed-off-by: Johnson Wang <johnson.wang@mediatek.com>
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> ---
>  .../devicetree/bindings/devfreq/mtk-cci.yaml  | 72 +++++++++++++++++++
>  1 file changed, 72 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/devfreq/mtk-cci.yaml

Filename with vendor prefix, so something like:

mediatek,cci.yaml

Also please put it in the "interconnect" directory.

> 
> diff --git a/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml b/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml
> new file mode 100644
> index 000000000000..ef4ea951025c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml
> @@ -0,0 +1,72 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/devfreq/mtk-cci.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek Cache Coherent Interconnect (CCI) frequency and voltage scaling
> +
> +maintainers:
> +  - Jia-Wei Chang <jia-wei.chang@mediatek.com>
> +
> +description: |
> +  MediaTek Cache Coherent Interconnect (CCI) uses the software devfreq module

Do not reference software implementation (devfreq).

> +  to scale the clock frequency and adjust the voltage. MediaTek CCI shares
> +  the same power supplies with CPU, so the scheduling involves with CPUfreq.

The same - cpufreq.

Instead, focus on the hardware, what do you describe here?

> +
> +properties:
> +  compatible:
> +    enum:
> +      - mediatek,mt8183-cci
> +      - mediatek,mt8186-cci
> +
> +  clocks:
> +    items:
> +      - description:
> +          The multiplexer for clock input of CPU cluster.
> +      - description:
> +          A parent of "cpu" clock which is used as an intermediate clock source
> +          when the original CPU is under transition and not stable yet.
> +
> +  clock-names:
> +    items:
> +      - const: cci
> +      - const: intermediate
> +
> +  operating-points-v2:
> +    description:
> +      For details, please refer to
> +      Documentation/devicetree/bindings/opp/opp-v2.yaml

No need for description. Just "operating-points-v2: true".

"opp-table:true" could stay. My previous comment about its removal was a
wrong advice, because opp-table is used for a table being a children of
this device node.

Best regards,
Krzysztof
Johnson Wang April 11, 2022, 12:10 p.m. UTC | #2
Hi Krzysztof,
On Fri, 2022-04-08 at 10:17 +0200, Krzysztof Kozlowski wrote:
> On 08/04/2022 07:21, Johnson Wang wrote:
> > Add devicetree binding of mtk cci devfreq on MediaTek SoC.
> > 
> 
> Thank you for your patch. There is something to discuss/improve.
> 
> > Signed-off-by: Johnson Wang <johnson.wang@mediatek.com>
> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > ---
> >  .../devicetree/bindings/devfreq/mtk-cci.yaml  | 72
> > +++++++++++++++++++
> >  1 file changed, 72 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/devfreq/mtk-
> > cci.yaml
> 
> Filename with vendor prefix, so something like:
> 
> mediatek,cci.yaml

Thank you for your review.
I will take your advice in the next version.
> 
> Also please put it in the "interconnect" directory.
> 

I don't really know about "interconnect".
However, it looks like a Linux framework about data transfer and "NoC".

While this cci driver is more like a power managment which is
responsible for adjusting voltages and frequencies.
In my opinion, "devfreq" should be more suitable.

Please correct me if my understanding is wrong.

> > diff --git a/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml 
> > b/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml
> > new file mode 100644
> > index 000000000000..ef4ea951025c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml
> > @@ -0,0 +1,72 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: 
> > https://urldefense.com/v3/__http://devicetree.org/schemas/devfreq/mtk-cci.yaml*__;Iw!!CTRNKA9wMg0ARbw!yd_wfLu2nSv0GsJZOGP1S8McsGD9A2SC4Qe0Xg1wEb_yEMVcqHRqdvs-M8YKOGckaagO$
> >  
> > +$schema: 
> > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!yd_wfLu2nSv0GsJZOGP1S8McsGD9A2SC4Qe0Xg1wEb_yEMVcqHRqdvs-M8YKOERouJvA$
> >  
> > +
> > +title: MediaTek Cache Coherent Interconnect (CCI) frequency and
> > voltage scaling
> > +
> > +maintainers:
> > +  - Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > +
> > +description: |
> > +  MediaTek Cache Coherent Interconnect (CCI) uses the software
> > devfreq module
> 
> Do not reference software implementation (devfreq).

I will modify it in the next version.

> 
> > +  to scale the clock frequency and adjust the voltage. MediaTek
> > CCI shares
> > +  the same power supplies with CPU, so the scheduling involves
> > with CPUfreq.
> 
> The same - cpufreq.
> 
> Instead, focus on the hardware, what do you describe here?

I will focus on hardware description in the next version.

> 
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - mediatek,mt8183-cci
> > +      - mediatek,mt8186-cci
> > +
> > +  clocks:
> > +    items:
> > +      - description:
> > +          The multiplexer for clock input of CPU cluster.
> > +      - description:
> > +          A parent of "cpu" clock which is used as an intermediate
> > clock source
> > +          when the original CPU is under transition and not stable
> > yet.
> > +
> > +  clock-names:
> > +    items:
> > +      - const: cci
> > +      - const: intermediate
> > +
> > +  operating-points-v2:
> > +    description:
> > +      For details, please refer to
> > +      Documentation/devicetree/bindings/opp/opp-v2.yaml
> 
> No need for description. Just "operating-points-v2: true".
> 
> "opp-table:true" could stay. My previous comment about its removal
> was a
> wrong advice, because opp-table is used for a table being a children
> of
> this device node.
> 
> Best regards,
> Krzysztof


I will remove it and add "opp-table:true"(also example) in the next
version.

Thanks.

BRs,
Johnson Wang
Krzysztof Kozlowski April 12, 2022, 9:17 a.m. UTC | #3
On 11/04/2022 14:10, Johnson Wang wrote:
>> Also please put it in the "interconnect" directory.
>>
> 
> I don't really know about "interconnect".
> However, it looks like a Linux framework about data transfer and "NoC".
> 
> While this cci driver is more like a power managment which is
> responsible for adjusting voltages and frequencies.
> In my opinion, "devfreq" should be more suitable.
> 
> Please correct me if my understanding is wrong.

devfreq is a Linux mechanism, not a real device/hardware. We try to put
the bindings in directories/subsystems matching the hardware, therefore
devfreq is not appropriate.

Whether interconnect - or other subsystem - is appropriate, I am not
sure. To me this looks exactly like bus bandwidth management and you
even use "interconnect" in several places. So interconnect matches.

Best regards,
Krzysztof
Chanwoo Choi April 13, 2022, 8:54 p.m. UTC | #4
On 22. 4. 12. 18:17, Krzysztof Kozlowski wrote:
> On 11/04/2022 14:10, Johnson Wang wrote:
>>> Also please put it in the "interconnect" directory.
>>>
>>
>> I don't really know about "interconnect".
>> However, it looks like a Linux framework about data transfer and "NoC".
>>
>> While this cci driver is more like a power managment which is
>> responsible for adjusting voltages and frequencies.
>> In my opinion, "devfreq" should be more suitable.
>>
>> Please correct me if my understanding is wrong.
> 
> devfreq is a Linux mechanism, not a real device/hardware. We try to put
> the bindings in directories/subsystems matching the hardware, therefore
> devfreq is not appropriate.
> 
> Whether interconnect - or other subsystem - is appropriate, I am not
> sure. To me this looks exactly like bus bandwidth management and you
> even use "interconnect" in several places. So interconnect matches.

I think that 'Documentation/devicetree/bindings/arm/mediatek' is proper.
Kevin Hilman April 14, 2022, 9:55 p.m. UTC | #5
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes:

> On 11/04/2022 14:10, Johnson Wang wrote:
>>> Also please put it in the "interconnect" directory.
>>>
>> 
>> I don't really know about "interconnect".
>> However, it looks like a Linux framework about data transfer and "NoC".
>> 
>> While this cci driver is more like a power managment which is
>> responsible for adjusting voltages and frequencies.
>> In my opinion, "devfreq" should be more suitable.
>> 
>> Please correct me if my understanding is wrong.
>
> devfreq is a Linux mechanism, not a real device/hardware. We try to put
> the bindings in directories/subsystems matching the hardware, therefore
> devfreq is not appropriate.
>
> Whether interconnect - or other subsystem - is appropriate, I am not
> sure. To me this looks exactly like bus bandwidth management and you
> even use "interconnect" in several places. So interconnect matches.

I agree with Krzysztof that "interconnect" is the right place.

I'm pretty sure CCI stands for "cache coherent interconnect".  At least
that's what it means for the Arm IP.  The Mediatek IP being described
here certainly seems like the same thing.  It's just that the only
aspects being described (so far) are the DVFS parts.  Even so, I still
think it belongs in "interconnect"

Kevin
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml b/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml
new file mode 100644
index 000000000000..ef4ea951025c
--- /dev/null
+++ b/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml
@@ -0,0 +1,72 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/devfreq/mtk-cci.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek Cache Coherent Interconnect (CCI) frequency and voltage scaling
+
+maintainers:
+  - Jia-Wei Chang <jia-wei.chang@mediatek.com>
+
+description: |
+  MediaTek Cache Coherent Interconnect (CCI) uses the software devfreq module
+  to scale the clock frequency and adjust the voltage. MediaTek CCI shares
+  the same power supplies with CPU, so the scheduling involves with CPUfreq.
+
+properties:
+  compatible:
+    enum:
+      - mediatek,mt8183-cci
+      - mediatek,mt8186-cci
+
+  clocks:
+    items:
+      - description:
+          The multiplexer for clock input of CPU cluster.
+      - description:
+          A parent of "cpu" clock which is used as an intermediate clock source
+          when the original CPU is under transition and not stable yet.
+
+  clock-names:
+    items:
+      - const: cci
+      - const: intermediate
+
+  operating-points-v2:
+    description:
+      For details, please refer to
+      Documentation/devicetree/bindings/opp/opp-v2.yaml
+
+  proc-supply:
+    description:
+      Phandle of the regulator for CCI that provides the supply voltage.
+
+  sram-supply:
+    description:
+      Phandle of the regulator for sram of CCI that provides the supply
+      voltage. When it presents, the cci devfreq 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.
+
+required:
+  - compatible
+  - clocks
+  - clock-names
+  - operating-points-v2
+  - proc-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/mt8183-clk.h>
+    cci: cci {
+      compatible = "mediatek,mt8183-cci";
+      clocks = <&mcucfg CLK_MCU_BUS_SEL>,
+               <&topckgen CLK_TOP_ARMPLL_DIV_PLL1>;
+      clock-names = "cci", "intermediate";
+      operating-points-v2 = <&cci_opp>;
+      proc-supply = <&mt6358_vproc12_reg>;
+    };