| Message ID | 20220408052150.22536-2-johnson.wang@mediatek.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Chanwoo Choi |
| Headers | show |
| Series | Introduce MediaTek CCI devfreq driver | expand |
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
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
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
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.
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 --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>; + };