| Message ID | 20220307122513.11822-2-jia-wei.chang@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | devfreq: mediatek: introduce MTK cci devfreq | expand |
On 07/03/2022 13:25, Tim Chang wrote: > add devicetree binding of mtk cci devfreq on MediaTek SoC. Start with capital letter. > > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.corp-partner.google.com> This does not match your From. Please fix this in all your submissions. > --- > .../devicetree/bindings/devfreq/mtk-cci.yaml | 73 +++++++++++++++++++ > 1 file changed, 73 insertions(+) > create mode 100644 Documentation/devicetree/bindings/devfreq/mtk-cci.yaml > > diff --git a/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml b/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml > new file mode 100644 > index 000000000000..e64ac4c56758 > --- /dev/null > +++ b/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml > @@ -0,0 +1,73 @@ > +# 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) Devfreq driver Device Tree Bindings Similarly to your other patches - the title describes hardware. Please fix it in all your submissions of all your series. Remove "driver Device Tree Bindings". "Devfreq" is Linuxism, so this maybe "bus frequency scaling"? Although later you call the device node as cci. > + > +maintainers: > + - Jia-Wei Chang <jia-wei.chang@mediatek.com> > + > +description: | > + This module is used to create CCI DEVFREQ. > + The performance will depend on both CCI frequency and CPU frequency. > + For MT8186, CCI co-buck with Little core. > + Contain CCI opp table for voltage and frequency scaling. Half of this description (first and last sentence) does not describe the actual hardware. Please describe hardware, not driver. > + > +properties: > + compatible: > + const: "mediatek,mt8186-cci" No need for quotes. > + > + 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: "cci" > + - const: "intermediate" No need for quotes. > + > + operating-points-v2: > + description: > + For details, please refer to > + Documentation/devicetree/bindings/opp/opp-v2.yaml > + > + opp-table: true Same comments as your CPU freq bindings apply. > + > + 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 present, 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/mt8186-clk.h> > + cci: cci { Node names should be generic and describe type of device. Are you sure this is a CCI? Maybe "interconnect" suits it better? > + compatible = "mediatek,mt8186-cci"; > + clocks = <&mcusys CLK_MCU_ARMPLL_BUS_SEL>, <&apmixedsys CLK_APMIXED_MAINPLL>; > + clock-names = "cci", "intermediate"; > + operating-points-v2 = <&cci_opp>; > + proc-supply = <&mt6358_vproc12_reg>; > + sram-supply = <&mt6358_vsram_proc12_reg>; > + }; Best regards, Krzysztof
Dear Krzysztof, Thanks for your comments. Pardon me for my late reply. On Mon, 2022-03-07 at 22:42 +0100, Krzysztof Kozlowski wrote: > On 07/03/2022 13:25, Tim Chang wrote: > > add devicetree binding of mtk cci devfreq on MediaTek SoC. > > Start with capital letter. Sure, I will update it for the whole series in next version. > > > > > Signed-off-by: Jia-Wei Chang < > > jia-wei.chang@mediatek.corp-partner.google.com> > > This does not match your From. Please fix this in all your > submissions. Sure, I will update it for the whole series in next version. > > > --- > > .../devicetree/bindings/devfreq/mtk-cci.yaml | 73 > > +++++++++++++++++++ > > 1 file changed, 73 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/devfreq/mtk- > > cci.yaml > > > > diff --git a/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml > > b/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml > > new file mode 100644 > > index 000000000000..e64ac4c56758 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml > > @@ -0,0 +1,73 @@ > > +# 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!2apx_16V_XMrl28ae1aDO3-2WFga3xJiACU_40mgGydrumBmFuHcQFpW_LnX6DHny5Zpig$ > > > > +$schema: > > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!2apx_16V_XMrl28ae1aDO3-2WFga3xJiACU_40mgGydrumBmFuHcQFpW_LnX6DEPqkDN4g$ > > > > + > > +title: Mediatek Cache Coherent Interconnect (CCI) Devfreq driver > > Device Tree Bindings > > Similarly to your other patches - the title describes hardware. > Please > fix it in all your submissions of all your series. Sure, I will fix them in the next version. > > Remove "driver Device Tree Bindings". "Devfreq" is Linuxism, so this > maybe "bus frequency scaling"? Although later you call the device > node > as cci. Should I use "Binding for MediaTek's Cache Coherent Interconnect (CCI) frequency and voltage scaling" as new title? > > > + > > +maintainers: > > + - Jia-Wei Chang <jia-wei.chang@mediatek.com> > > + > > +description: | > > + This module is used to create CCI DEVFREQ. > > + The performance will depend on both CCI frequency and CPU > > frequency. > > + For MT8186, CCI co-buck with Little core. > > + Contain CCI opp table for voltage and frequency scaling. > > Half of this description (first and last sentence) does not describe > the > actual hardware. Please describe hardware, not driver. Sure, I will fix it in the next version. > > > + > > +properties: > > + compatible: > > + const: "mediatek,mt8186-cci" > > No need for quotes. Sure, I will fix it in the next version. > > > + > > + 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: "cci" > > + - const: "intermediate" > > No need for quotes. Sure, I will fix it in the next version. > > > + > > + operating-points-v2: > > + description: > > + For details, please refer to > > + Documentation/devicetree/bindings/opp/opp-v2.yaml > > + > > + opp-table: true > > Same comments as your CPU freq bindings apply. mtk-cci-devfreq is a new driver and its arch is same as mediatek- cpufreq so that the properties of mtk-cci are refer to mediatek-cpufreq bindings. operating-point-v2 is used to determine the voltage and frequency of dvfs which is further utilized by mtk-cci-devfreq. > > > + > > + 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 present, 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/mt8186-clk.h> > > + cci: cci { > > Node names should be generic and describe type of device. Are you > sure > this is a CCI? Maybe "interconnect" suits it better? Yes, this is a CCI and it is generic type of device like CPU in my opinion. If my understanding is correct, CCI is more suitable. > > > + compatible = "mediatek,mt8186-cci"; > > + clocks = <&mcusys CLK_MCU_ARMPLL_BUS_SEL>, <&apmixedsys > > CLK_APMIXED_MAINPLL>; > > + clock-names = "cci", "intermediate"; > > + operating-points-v2 = <&cci_opp>; > > + proc-supply = <&mt6358_vproc12_reg>; > > + sram-supply = <&mt6358_vsram_proc12_reg>; > > + }; > > > Best regards, > Krzysztof
On 24/03/2022 13:11, Jia-Wei Chang wrote: >> >> Remove "driver Device Tree Bindings". "Devfreq" is Linuxism, so this >> maybe "bus frequency scaling"? Although later you call the device >> node >> as cci. > > Should I use "Binding for MediaTek's Cache Coherent Interconnect (CCI) > frequency and voltage scaling" as new title? I just suggested to remove word "bindings" so do not add it again. This should be a title for hardware. Now what exactly is it - you should know better than me. :) "MediaTek's Cache Coherent Interconnect (CCI) frequency and voltage scaling" sounds good to me, assuming that this is the hardware we talk here about. :) > >> >>> + >>> +maintainers: >>> + - Jia-Wei Chang <jia-wei.chang@mediatek.com> >>> + >>> +description: | >>> + This module is used to create CCI DEVFREQ. >>> + The performance will depend on both CCI frequency and CPU >>> frequency. >>> + For MT8186, CCI co-buck with Little core. >>> + Contain CCI opp table for voltage and frequency scaling. >> >> Half of this description (first and last sentence) does not describe >> the >> actual hardware. Please describe hardware, not driver. > > Sure, I will fix it in the next version. > >> >>> + >>> +properties: >>> + compatible: >>> + const: "mediatek,mt8186-cci" >> >> No need for quotes. > > Sure, I will fix it in the next version. > >> >>> + >>> + 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: "cci" >>> + - const: "intermediate" >> >> No need for quotes. > > Sure, I will fix it in the next version. > >> >>> + >>> + operating-points-v2: >>> + description: >>> + For details, please refer to >>> + Documentation/devicetree/bindings/opp/opp-v2.yaml >>> + >>> + opp-table: true >> >> Same comments as your CPU freq bindings apply. > > mtk-cci-devfreq is a new driver and its arch is same as mediatek- > cpufreq so that the properties of mtk-cci are refer to mediatek-cpufreq > bindings. > operating-point-v2 is used to determine the voltage and frequency of > dvfs which is further utilized by mtk-cci-devfreq. "operating-point-v2" is understood, but the same as in cpufreq bindings, I am questioning why do you have "opp-table: true". It's a bit confusing, so maybe I miss something? > >> >>> + >>> + 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 present, 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/mt8186-clk.h> >>> + cci: cci { >> >> Node names should be generic and describe type of device. Are you >> sure >> this is a CCI? Maybe "interconnect" suits it better? > > Yes, this is a CCI and it is generic type of device like CPU in my > opinion. > If my understanding is correct, CCI is more suitable. OK. > >> >>> + compatible = "mediatek,mt8186-cci"; >>> + clocks = <&mcusys CLK_MCU_ARMPLL_BUS_SEL>, <&apmixedsys >>> CLK_APMIXED_MAINPLL>; >>> + clock-names = "cci", "intermediate"; >>> + operating-points-v2 = <&cci_opp>; >>> + proc-supply = <&mt6358_vproc12_reg>; >>> + sram-supply = <&mt6358_vsram_proc12_reg>; >>> + }; >> >> >> Best regards, >> Krzysztof > Best regards, Krzysztof
On Thu, 2022-03-24 at 13:44 +0100, Krzysztof Kozlowski wrote: > On 24/03/2022 13:11, Jia-Wei Chang wrote: > > > > > > Remove "driver Device Tree Bindings". "Devfreq" is Linuxism, so > > > this > > > maybe "bus frequency scaling"? Although later you call the device > > > node > > > as cci. > > > > Should I use "Binding for MediaTek's Cache Coherent Interconnect > > (CCI) > > frequency and voltage scaling" as new title? > > I just suggested to remove word "bindings" so do not add it again. > This > should be a title for hardware. Sure, I will remove the word "bindings" from title. > > Now what exactly is it - you should know better than me. :) > "MediaTek's Cache Coherent Interconnect (CCI) frequency and voltage > scaling" sounds good to me, assuming that this is the hardware we > talk > here about. :) Appreciate your comments. It's a bit hard to do upstream at first time, thank you for understanding. > > > > > > > > > > + > > > > +maintainers: > > > > + - Jia-Wei Chang <jia-wei.chang@mediatek.com> > > > > + > > > > +description: | > > > > + This module is used to create CCI DEVFREQ. > > > > + The performance will depend on both CCI frequency and CPU > > > > frequency. > > > > + For MT8186, CCI co-buck with Little core. > > > > + Contain CCI opp table for voltage and frequency scaling. > > > > > > Half of this description (first and last sentence) does not > > > describe > > > the > > > actual hardware. Please describe hardware, not driver. > > > > Sure, I will fix it in the next version. > > > > > > > > > + > > > > +properties: > > > > + compatible: > > > > + const: "mediatek,mt8186-cci" > > > > > > No need for quotes. > > > > Sure, I will fix it in the next version. > > > > > > > > > + > > > > + 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: "cci" > > > > + - const: "intermediate" > > > > > > No need for quotes. > > > > Sure, I will fix it in the next version. > > > > > > > > > + > > > > + operating-points-v2: > > > > + description: > > > > + For details, please refer to > > > > + Documentation/devicetree/bindings/opp/opp-v2.yaml > > > > + > > > > + opp-table: true > > > > > > Same comments as your CPU freq bindings apply. > > > > mtk-cci-devfreq is a new driver and its arch is same as mediatek- > > cpufreq so that the properties of mtk-cci are refer to mediatek- > > cpufreq > > bindings. > > operating-point-v2 is used to determine the voltage and frequency > > of > > dvfs which is further utilized by mtk-cci-devfreq. > > "operating-point-v2" is understood, but the same as in cpufreq > bindings, > I am questioning why do you have "opp-table: true". It's a bit > confusing, so maybe I miss something? Yes, you're correct. "opp-table: true" should be removed. I messed it up. > > > > > > > > > > + > > > > + 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 present, 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/mt8186-clk.h> > > > > + cci: cci { > > > > > > Node names should be generic and describe type of device. Are you > > > sure > > > this is a CCI? Maybe "interconnect" suits it better? > > > > Yes, this is a CCI and it is generic type of device like CPU in my > > opinion. > > If my understanding is correct, CCI is more suitable. > > OK. :) > > > > > > > > > > + compatible = "mediatek,mt8186-cci"; > > > > + clocks = <&mcusys CLK_MCU_ARMPLL_BUS_SEL>, <&apmixedsys > > > > CLK_APMIXED_MAINPLL>; > > > > + clock-names = "cci", "intermediate"; > > > > + operating-points-v2 = <&cci_opp>; > > > > + proc-supply = <&mt6358_vproc12_reg>; > > > > + sram-supply = <&mt6358_vsram_proc12_reg>; > > > > + }; > > > > > > > > > Best regards, > > > Krzysztof > > > Best regards, > Krzysztof
On 01/04/2022 15:39, Jia-Wei Chang wrote: >>>> >>>>> + >>>>> + operating-points-v2: >>>>> + description: >>>>> + For details, please refer to >>>>> + Documentation/devicetree/bindings/opp/opp-v2.yaml >>>>> + >>>>> + opp-table: true >>>> >>>> Same comments as your CPU freq bindings apply. >>> >>> mtk-cci-devfreq is a new driver and its arch is same as mediatek- >>> cpufreq so that the properties of mtk-cci are refer to mediatek- >>> cpufreq >>> bindings. >>> operating-point-v2 is used to determine the voltage and frequency >>> of >>> dvfs which is further utilized by mtk-cci-devfreq. >> >> "operating-point-v2" is understood, but the same as in cpufreq >> bindings, >> I am questioning why do you have "opp-table: true". It's a bit >> confusing, so maybe I miss something? > > Yes, you're correct. > "opp-table: true" should be removed. > I messed it up. No, I think I was wrong. The opp-table pretty frequently is embedded in the the device node itself. The operating-points-v2 references it. You don't use it in the example, but it might be a valid usage, so it can stay. Sorry for the confusion, it passed some time since I looked at OPP bindings. Best regards, Krzysztof
On Sat, 2022-04-02 at 13:31 +0200, Krzysztof Kozlowski wrote: > On 01/04/2022 15:39, Jia-Wei Chang wrote: > > > > > > > > > > > + > > > > > > + operating-points-v2: > > > > > > + description: > > > > > > + For details, please refer to > > > > > > + Documentation/devicetree/bindings/opp/opp-v2.yaml > > > > > > + > > > > > > + opp-table: true > > > > > > > > > > Same comments as your CPU freq bindings apply. > > > > > > > > mtk-cci-devfreq is a new driver and its arch is same as > > > > mediatek- > > > > cpufreq so that the properties of mtk-cci are refer to > > > > mediatek- > > > > cpufreq > > > > bindings. > > > > operating-point-v2 is used to determine the voltage and > > > > frequency > > > > of > > > > dvfs which is further utilized by mtk-cci-devfreq. > > > > > > "operating-point-v2" is understood, but the same as in cpufreq > > > bindings, > > > I am questioning why do you have "opp-table: true". It's a bit > > > confusing, so maybe I miss something? > > > > Yes, you're correct. > > "opp-table: true" should be removed. > > I messed it up. > > No, I think I was wrong. The opp-table pretty frequently is embedded > in > the the device node itself. The operating-points-v2 references it. > > You don't use it in the example, but it might be a valid usage, so it > can stay. Sorry for the confusion, it passed some time since I looked > at > OPP bindings. You remind me of "opp-table: true" and the reason why I use it here is exactly as you mentioned. Sorry I was not familiar enough with this to respond it clearly and confidently. I think it is proper to keep "opp-table: true" and add a complete opp table information in dts example here as well. Thanks for your comments. > > > Best regards, > Krzysztof
diff --git a/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml b/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml new file mode 100644 index 000000000000..e64ac4c56758 --- /dev/null +++ b/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml @@ -0,0 +1,73 @@ +# 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) Devfreq driver Device Tree Bindings + +maintainers: + - Jia-Wei Chang <jia-wei.chang@mediatek.com> + +description: | + This module is used to create CCI DEVFREQ. + The performance will depend on both CCI frequency and CPU frequency. + For MT8186, CCI co-buck with Little core. + Contain CCI opp table for voltage and frequency scaling. + +properties: + compatible: + const: "mediatek,mt8186-cci" + + 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: "cci" + - 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 CCI that provides the supply voltage. + + sram-supply: + description: + Phandle of the regulator for sram of CCI that provides the supply + voltage. When present, 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/mt8186-clk.h> + cci: cci { + compatible = "mediatek,mt8186-cci"; + clocks = <&mcusys CLK_MCU_ARMPLL_BUS_SEL>, <&apmixedsys CLK_APMIXED_MAINPLL>; + clock-names = "cci", "intermediate"; + operating-points-v2 = <&cci_opp>; + proc-supply = <&mt6358_vproc12_reg>; + sram-supply = <&mt6358_vsram_proc12_reg>; + };
add devicetree binding of mtk cci devfreq on MediaTek SoC. Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.corp-partner.google.com> --- .../devicetree/bindings/devfreq/mtk-cci.yaml | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) create mode 100644 Documentation/devicetree/bindings/devfreq/mtk-cci.yaml