diff mbox series

[v3,1/2] dt-bindings: interconnect: Add MediaTek CCI dt-bindings

Message ID 20220425125546.4129-2-johnson.wang@mediatek.com
State Superseded
Headers show
Series Introduce MediaTek CCI devfreq driver | expand

Commit Message

Johnson Wang April 25, 2022, 12:55 p.m. UTC
Add devicetree binding of MediaTek CCI on MT8183 and MT8186.

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

Comments

Krzysztof Kozlowski April 25, 2022, 5:56 p.m. UTC | #1
On 25/04/2022 14:55, Johnson Wang wrote:
> Add devicetree binding of MediaTek CCI on MT8183 and MT8186.
> 
> Signed-off-by: Johnson Wang <johnson.wang@mediatek.com>
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> ---


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof
Chen-Yu Tsai April 26, 2022, 3:18 a.m. UTC | #2
On Mon, Apr 25, 2022 at 8:56 PM Johnson Wang <johnson.wang@mediatek.com> wrote:
>
> Add devicetree binding of MediaTek CCI on MT8183 and MT8186.
>
> Signed-off-by: Johnson Wang <johnson.wang@mediatek.com>
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> ---
>  .../bindings/interconnect/mediatek,cci.yaml   | 139 ++++++++++++++++++
>  1 file changed, 139 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
>
> diff --git a/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml b/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> new file mode 100644
> index 000000000000..e5221e17d11b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> @@ -0,0 +1,139 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interconnect/mediatek,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) is a hardware engine used by
> +  MT8183 and MT8186 SoCs to scale the frequency and adjust the voltage in
> +  hardware. It can also optimize the voltage to reduce the power consumption.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - mediatek,mt8183-cci
> +      - mediatek,mt8186-cci
> +
> +  clocks:
> +    items:
> +      - description:
> +          The multiplexer for clock input of CPU cluster.

of the bus, not 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.

This really should be handled in the clk controller, and not by every device
that happens to take a clock from a mux with upstream PLLs that can change
in clock rate. The end device hardware only takes one clock input. That's it.

> +
> +  clock-names:
> +    items:
> +      - const: cci
> +      - const: intermediate
> +
> +  operating-points-v2: true
> +  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 it presents, the cci devfreq driver needs to do

When it is present, the implementation needs to ...

ChenYu

> +      "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>;
> +    };
> +
> +    cci_opp: opp-table-cci {
> +        compatible = "operating-points-v2";
> +        opp-shared;
> +        opp2_00: opp-273000000 {
> +            opp-hz = /bits/ 64 <273000000>;
> +            opp-microvolt = <650000>;
> +        };
> +        opp2_01: opp-338000000 {
> +            opp-hz = /bits/ 64 <338000000>;
> +            opp-microvolt = <687500>;
> +        };
> +        opp2_02: opp-403000000 {
> +            opp-hz = /bits/ 64 <403000000>;
> +            opp-microvolt = <718750>;
> +        };
> +        opp2_03: opp-463000000 {
> +            opp-hz = /bits/ 64 <463000000>;
> +            opp-microvolt = <756250>;
> +        };
> +        opp2_04: opp-546000000 {
> +            opp-hz = /bits/ 64 <546000000>;
> +            opp-microvolt = <800000>;
> +        };
> +        opp2_05: opp-624000000 {
> +            opp-hz = /bits/ 64 <624000000>;
> +            opp-microvolt = <818750>;
> +        };
> +        opp2_06: opp-689000000 {
> +            opp-hz = /bits/ 64 <689000000>;
> +            opp-microvolt = <850000>;
> +        };
> +        opp2_07: opp-767000000 {
> +            opp-hz = /bits/ 64 <767000000>;
> +            opp-microvolt = <868750>;
> +        };
> +        opp2_08: opp-845000000 {
> +            opp-hz = /bits/ 64 <845000000>;
> +            opp-microvolt = <893750>;
> +        };
> +        opp2_09: opp-871000000 {
> +            opp-hz = /bits/ 64 <871000000>;
> +            opp-microvolt = <906250>;
> +        };
> +        opp2_10: opp-923000000 {
> +            opp-hz = /bits/ 64 <923000000>;
> +            opp-microvolt = <931250>;
> +        };
> +        opp2_11: opp-962000000 {
> +            opp-hz = /bits/ 64 <962000000>;
> +            opp-microvolt = <943750>;
> +        };
> +        opp2_12: opp-1027000000 {
> +            opp-hz = /bits/ 64 <1027000000>;
> +            opp-microvolt = <975000>;
> +        };
> +        opp2_13: opp-1092000000 {
> +            opp-hz = /bits/ 64 <1092000000>;
> +            opp-microvolt = <1000000>;
> +        };
> +        opp2_14: opp-1144000000 {
> +            opp-hz = /bits/ 64 <1144000000>;
> +            opp-microvolt = <1025000>;
> +        };
> +        opp2_15: opp-1196000000 {
> +            opp-hz = /bits/ 64 <1196000000>;
> +            opp-microvolt = <1050000>;
> +        };
> +    };
> --
> 2.18.0
>
>
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
Chanwoo Choi May 9, 2022, 12:09 p.m. UTC | #3
Hi,

On 22. 4. 25. 21:55, Johnson Wang wrote:
> Add devicetree binding of MediaTek CCI on MT8183 and MT8186.
> 
> Signed-off-by: Johnson Wang <johnson.wang@mediatek.com>
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> ---
>   .../bindings/interconnect/mediatek,cci.yaml   | 139 ++++++++++++++++++
>   1 file changed, 139 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> 
> diff --git a/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml b/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> new file mode 100644
> index 000000000000..e5221e17d11b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> @@ -0,0 +1,139 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interconnect/mediatek,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>

Why did you add your author information?
Please add your author information.

And add this dt-binding information to MAINTAINERS
as following: because I cannot catch the later patch
of modification.

cwchoi00@chanwoo:~/kernel/linux.chanwoo$ d
diff --git a/MAINTAINERS b/MAINTAINERS
index edc96cdb85e8..a11e9c1947b7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5698,6 +5698,7 @@ L:        linux-pm@vger.kernel.org
  S:     Maintained
  T:     git git://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git
  F:     Documentation/devicetree/bindings/devfreq/
+F:     Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
  F:     drivers/devfreq/
  F:     include/linux/devfreq.h
  F:     include/trace/events/devfreq.h


> +
> +description: |
> +  MediaTek Cache Coherent Interconnect (CCI) is a hardware engine used by
> +  MT8183 and MT8186 SoCs to scale the frequency and adjust the voltage in
> +  hardware. It can also optimize the voltage to reduce the power consumption.
> +
> +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: true
> +  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 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>;
> +    };
> +
> +    cci_opp: opp-table-cci {
> +        compatible = "operating-points-v2";
> +        opp-shared;
> +        opp2_00: opp-273000000 {
> +            opp-hz = /bits/ 64 <273000000>;
> +            opp-microvolt = <650000>;
> +        };
> +        opp2_01: opp-338000000 {
> +            opp-hz = /bits/ 64 <338000000>;
> +            opp-microvolt = <687500>;
> +        };
> +        opp2_02: opp-403000000 {
> +            opp-hz = /bits/ 64 <403000000>;
> +            opp-microvolt = <718750>;
> +        };
> +        opp2_03: opp-463000000 {
> +            opp-hz = /bits/ 64 <463000000>;
> +            opp-microvolt = <756250>;
> +        };
> +        opp2_04: opp-546000000 {
> +            opp-hz = /bits/ 64 <546000000>;
> +            opp-microvolt = <800000>;
> +        };
> +        opp2_05: opp-624000000 {
> +            opp-hz = /bits/ 64 <624000000>;
> +            opp-microvolt = <818750>;
> +        };
> +        opp2_06: opp-689000000 {
> +            opp-hz = /bits/ 64 <689000000>;
> +            opp-microvolt = <850000>;
> +        };
> +        opp2_07: opp-767000000 {
> +            opp-hz = /bits/ 64 <767000000>;
> +            opp-microvolt = <868750>;
> +        };
> +        opp2_08: opp-845000000 {
> +            opp-hz = /bits/ 64 <845000000>;
> +            opp-microvolt = <893750>;
> +        };
> +        opp2_09: opp-871000000 {
> +            opp-hz = /bits/ 64 <871000000>;
> +            opp-microvolt = <906250>;
> +        };
> +        opp2_10: opp-923000000 {
> +            opp-hz = /bits/ 64 <923000000>;
> +            opp-microvolt = <931250>;
> +        };
> +        opp2_11: opp-962000000 {
> +            opp-hz = /bits/ 64 <962000000>;
> +            opp-microvolt = <943750>;
> +        };
> +        opp2_12: opp-1027000000 {
> +            opp-hz = /bits/ 64 <1027000000>;
> +            opp-microvolt = <975000>;
> +        };
> +        opp2_13: opp-1092000000 {
> +            opp-hz = /bits/ 64 <1092000000>;
> +            opp-microvolt = <1000000>;
> +        };
> +        opp2_14: opp-1144000000 {
> +            opp-hz = /bits/ 64 <1144000000>;
> +            opp-microvolt = <1025000>;
> +        };
> +        opp2_15: opp-1196000000 {
> +            opp-hz = /bits/ 64 <1196000000>;
> +            opp-microvolt = <1050000>;
> +        };
> +    };
Johnson Wang May 9, 2022, 12:14 p.m. UTC | #4
Hi Chen-Yu,

On Tue, 2022-04-26 at 11:18 +0800, Chen-Yu Tsai wrote:
> On Mon, Apr 25, 2022 at 8:56 PM Johnson Wang <
> johnson.wang@mediatek.com> wrote:
> > 
> > Add devicetree binding of MediaTek CCI on MT8183 and MT8186.
> > 
> > Signed-off-by: Johnson Wang <johnson.wang@mediatek.com>
> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > ---
> >  .../bindings/interconnect/mediatek,cci.yaml   | 139
> > ++++++++++++++++++
> >  1 file changed, 139 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> > b/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> > new file mode 100644
> > index 000000000000..e5221e17d11b
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> > @@ -0,0 +1,139 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: 
> > https://urldefense.com/v3/__http://devicetree.org/schemas/interconnect/mediatek,cci.yaml*__;Iw!!CTRNKA9wMg0ARbw!zuufEcqpKbditY3eqLTHpL8P8humMCyh4D4QWsximmw124tJUPE3ZBUyBqBtDlQ9pSDO$
> >  
> > +$schema: 
> > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!zuufEcqpKbditY3eqLTHpL8P8humMCyh4D4QWsximmw124tJUPE3ZBUyBqBtDoE9YHyu$
> >  
> > +
> > +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) is a hardware engine
> > used by
> > +  MT8183 and MT8186 SoCs to scale the frequency and adjust the
> > voltage in
> > +  hardware. It can also optimize the voltage to reduce the power
> > consumption.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - mediatek,mt8183-cci
> > +      - mediatek,mt8186-cci
> > +
> > +  clocks:
> > +    items:
> > +      - description:
> > +          The multiplexer for clock input of CPU cluster.
> 
> of the bus, not CPU cluster.

Thanks for your suggestion.
I will correct it in the next version.

> 
> > +      - 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.
> 
> This really should be handled in the clk controller, and not by every
> device
> that happens to take a clock from a mux with upstream PLLs that can
> change
> in clock rate. The end device hardware only takes one clock input.
> That's it.
> 

To make this intermediate clock works properly, this driver is also
responsible for handling the Vproc voltage and ensures the voltage is
high enough to support intermediate clock rate.

If we move intermediate clock rate control to clock driver, then
intermediate voltage control may be handled by the clock driver itself
as well.

We believe that is not reasonable because clock driver shouldn't handle
voltage control. On the other hand, DVFS driver is more suitable for
doing this job.

> > +
> > +  clock-names:
> > +    items:
> > +      - const: cci
> > +      - const: intermediate
> > +
> > +  operating-points-v2: true
> > +  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 it presents, the cci devfreq driver needs to
> > do
> 
> When it is present, the implementation needs to ...
> 
> ChenYu

I will modify it in the next version.

BRs,
Johnson Wang

> 
> > +      "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>;
> > +    };
> > +
> > +    cci_opp: opp-table-cci {
> > +        compatible = "operating-points-v2";
> > +        opp-shared;
> > +        opp2_00: opp-273000000 {
> > +            opp-hz = /bits/ 64 <273000000>;
> > +            opp-microvolt = <650000>;
> > +        };
> > +        opp2_01: opp-338000000 {
> > +            opp-hz = /bits/ 64 <338000000>;
> > +            opp-microvolt = <687500>;
> > +        };
> > +        opp2_02: opp-403000000 {
> > +            opp-hz = /bits/ 64 <403000000>;
> > +            opp-microvolt = <718750>;
> > +        };
> > +        opp2_03: opp-463000000 {
> > +            opp-hz = /bits/ 64 <463000000>;
> > +            opp-microvolt = <756250>;
> > +        };
> > +        opp2_04: opp-546000000 {
> > +            opp-hz = /bits/ 64 <546000000>;
> > +            opp-microvolt = <800000>;
> > +        };
> > +        opp2_05: opp-624000000 {
> > +            opp-hz = /bits/ 64 <624000000>;
> > +            opp-microvolt = <818750>;
> > +        };
> > +        opp2_06: opp-689000000 {
> > +            opp-hz = /bits/ 64 <689000000>;
> > +            opp-microvolt = <850000>;
> > +        };
> > +        opp2_07: opp-767000000 {
> > +            opp-hz = /bits/ 64 <767000000>;
> > +            opp-microvolt = <868750>;
> > +        };
> > +        opp2_08: opp-845000000 {
> > +            opp-hz = /bits/ 64 <845000000>;
> > +            opp-microvolt = <893750>;
> > +        };
> > +        opp2_09: opp-871000000 {
> > +            opp-hz = /bits/ 64 <871000000>;
> > +            opp-microvolt = <906250>;
> > +        };
> > +        opp2_10: opp-923000000 {
> > +            opp-hz = /bits/ 64 <923000000>;
> > +            opp-microvolt = <931250>;
> > +        };
> > +        opp2_11: opp-962000000 {
> > +            opp-hz = /bits/ 64 <962000000>;
> > +            opp-microvolt = <943750>;
> > +        };
> > +        opp2_12: opp-1027000000 {
> > +            opp-hz = /bits/ 64 <1027000000>;
> > +            opp-microvolt = <975000>;
> > +        };
> > +        opp2_13: opp-1092000000 {
> > +            opp-hz = /bits/ 64 <1092000000>;
> > +            opp-microvolt = <1000000>;
> > +        };
> > +        opp2_14: opp-1144000000 {
> > +            opp-hz = /bits/ 64 <1144000000>;
> > +            opp-microvolt = <1025000>;
> > +        };
> > +        opp2_15: opp-1196000000 {
> > +            opp-hz = /bits/ 64 <1196000000>;
> > +            opp-microvolt = <1050000>;
> > +        };
> > +    };
> > --
> > 2.18.0
> > 
> > 
> > _______________________________________________
> > Linux-mediatek mailing list
> > Linux-mediatek@lists.infradead.org
> > 
https://urldefense.com/v3/__http://lists.infradead.org/mailman/listinfo/linux-mediatek__;!!CTRNKA9wMg0ARbw!zuufEcqpKbditY3eqLTHpL8P8humMCyh4D4QWsximmw124tJUPE3ZBUyBqBtDvdNpOFZ$
> >
Chanwoo Choi May 9, 2022, 12:43 p.m. UTC | #5
On 22. 5. 9. 21:09, Chanwoo Choi wrote:
> Hi,
> 
> On 22. 4. 25. 21:55, Johnson Wang wrote:
>> Add devicetree binding of MediaTek CCI on MT8183 and MT8186.
>>
>> Signed-off-by: Johnson Wang <johnson.wang@mediatek.com>
>> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
>> ---
>>   .../bindings/interconnect/mediatek,cci.yaml   | 139 ++++++++++++++++++
>>   1 file changed, 139 insertions(+)
>>   create mode 100644 
>> Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml 
>> b/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
>> new file mode 100644
>> index 000000000000..e5221e17d11b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
>> @@ -0,0 +1,139 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/interconnect/mediatek,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>
> 
> Why did you add your author information?

I'm sorry. I missed 'not' above sentence.
IMHO, please add your author information.

(snip)
Chen-Yu Tsai May 11, 2022, 10:48 a.m. UTC | #6
On Mon, May 9, 2022 at 8:14 PM Johnson Wang <johnson.wang@mediatek.com> wrote:
>
> Hi Chen-Yu,
>
> On Tue, 2022-04-26 at 11:18 +0800, Chen-Yu Tsai wrote:
> > On Mon, Apr 25, 2022 at 8:56 PM Johnson Wang <
> > johnson.wang@mediatek.com> wrote:
> > >
> > > Add devicetree binding of MediaTek CCI on MT8183 and MT8186.
> > >
> > > Signed-off-by: Johnson Wang <johnson.wang@mediatek.com>
> > > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > > ---
> > >  .../bindings/interconnect/mediatek,cci.yaml   | 139
> > > ++++++++++++++++++
> > >  1 file changed, 139 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> > > b/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> > > new file mode 100644
> > > index 000000000000..e5221e17d11b
> > > --- /dev/null
> > > +++
> > > b/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> > > @@ -0,0 +1,139 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id:
> > > https://urldefense.com/v3/__http://devicetree.org/schemas/interconnect/mediatek,cci.yaml*__;Iw!!CTRNKA9wMg0ARbw!zuufEcqpKbditY3eqLTHpL8P8humMCyh4D4QWsximmw124tJUPE3ZBUyBqBtDlQ9pSDO$
> > >
> > > +$schema:
> > > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!zuufEcqpKbditY3eqLTHpL8P8humMCyh4D4QWsximmw124tJUPE3ZBUyBqBtDoE9YHyu$
> > >
> > > +
> > > +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) is a hardware engine
> > > used by
> > > +  MT8183 and MT8186 SoCs to scale the frequency and adjust the
> > > voltage in
> > > +  hardware. It can also optimize the voltage to reduce the power
> > > consumption.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - mediatek,mt8183-cci
> > > +      - mediatek,mt8186-cci
> > > +
> > > +  clocks:
> > > +    items:
> > > +      - description:
> > > +          The multiplexer for clock input of CPU cluster.
> >
> > of the bus, not CPU cluster.
>
> Thanks for your suggestion.
> I will correct it in the next version.
>
> >
> > > +      - 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.
> >
> > This really should be handled in the clk controller, and not by every
> > device
> > that happens to take a clock from a mux with upstream PLLs that can
> > change
> > in clock rate. The end device hardware only takes one clock input.
> > That's it.
> >
>
> To make this intermediate clock works properly, this driver is also
> responsible for handling the Vproc voltage and ensures the voltage is
> high enough to support intermediate clock rate.
>
> If we move intermediate clock rate control to clock driver, then
> intermediate voltage control may be handled by the clock driver itself
> as well.
>
> We believe that is not reasonable because clock driver shouldn't handle
> voltage control. On the other hand, DVFS driver is more suitable for
> doing this job.

Either way the DVFS driver handles the voltage change.

Right now the driver is doing:

1. Raise voltage if scaling up
2. Mux CCI clock over to stable clock
3. Set rate for CCI PLL
4. Mux CCI clock back to CCI PLL
5. Drop voltage if scaling down

I'm saying that the clock driver should handle 2+4 transparently when any
driver requests a rate change on the CCI clock. So instead the driver would
do:

1. Raise voltage if scaling up
2. Set rate for CCI _clock_
   Here the clock driver would do:
   a. Mux CCI clock over to stable clock
   b. Change clock rate for original parent, i.e. the CCI PLL
   c. Mux CCI clock back to original parent, i.e. the CCI PLL
   and back to the devfreq driver ...
3. Drop voltage if scaling down

Does that make sense?


Regards
ChenYu
Johnson Wang May 12, 2022, 1:04 p.m. UTC | #7
On Wed, 2022-05-11 at 18:48 +0800, Chen-Yu Tsai wrote:
> On Mon, May 9, 2022 at 8:14 PM Johnson Wang <
> johnson.wang@mediatek.com> wrote:
> > 
> > Hi Chen-Yu,
> > 
> > On Tue, 2022-04-26 at 11:18 +0800, Chen-Yu Tsai wrote:
> > > On Mon, Apr 25, 2022 at 8:56 PM Johnson Wang <
> > > johnson.wang@mediatek.com> wrote:
> > > > 
> > > > Add devicetree binding of MediaTek CCI on MT8183 and MT8186.
> > > > 
> > > > Signed-off-by: Johnson Wang <johnson.wang@mediatek.com>
> > > > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > > > ---
> > > >  .../bindings/interconnect/mediatek,cci.yaml   | 139
> > > > ++++++++++++++++++
> > > >  1 file changed, 139 insertions(+)
> > > >  create mode 100644
> > > > Documentation/devicetree/bindings/interconnect/mediatek,cci.yam
> > > > l
> > > > 
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/interconnect/mediatek,cci.y
> > > > aml
> > > > b/Documentation/devicetree/bindings/interconnect/mediatek,cci.y
> > > > aml
> > > > new file mode 100644
> > > > index 000000000000..e5221e17d11b
> > > > --- /dev/null
> > > > +++
> > > > b/Documentation/devicetree/bindings/interconnect/mediatek,cci.y
> > > > aml
> > > > @@ -0,0 +1,139 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id:
> > > > 
https://urldefense.com/v3/__http://devicetree.org/schemas/interconnect/mediatek,cci.yaml*__;Iw!!CTRNKA9wMg0ARbw!zuufEcqpKbditY3eqLTHpL8P8humMCyh4D4QWsximmw124tJUPE3ZBUyBqBtDlQ9pSDO$
> > > > 
> > > > +$schema:
> > > > 
https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!zuufEcqpKbditY3eqLTHpL8P8humMCyh4D4QWsximmw124tJUPE3ZBUyBqBtDoE9YHyu$
> > > > 
> > > > +
> > > > +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) is a hardware
> > > > engine
> > > > used by
> > > > +  MT8183 and MT8186 SoCs to scale the frequency and adjust the
> > > > voltage in
> > > > +  hardware. It can also optimize the voltage to reduce the
> > > > power
> > > > consumption.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    enum:
> > > > +      - mediatek,mt8183-cci
> > > > +      - mediatek,mt8186-cci
> > > > +
> > > > +  clocks:
> > > > +    items:
> > > > +      - description:
> > > > +          The multiplexer for clock input of CPU cluster.
> > > 
> > > of the bus, not CPU cluster.
> > 
> > Thanks for your suggestion.
> > I will correct it in the next version.
> > 
> > > 
> > > > +      - 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.
> > > 
> > > This really should be handled in the clk controller, and not by
> > > every
> > > device
> > > that happens to take a clock from a mux with upstream PLLs that
> > > can
> > > change
> > > in clock rate. The end device hardware only takes one clock
> > > input.
> > > That's it.
> > > 
> > 
> > To make this intermediate clock works properly, this driver is also
> > responsible for handling the Vproc voltage and ensures the voltage
> > is
> > high enough to support intermediate clock rate.
> > 
> > If we move intermediate clock rate control to clock driver, then
> > intermediate voltage control may be handled by the clock driver
> > itself
> > as well.
> > 
> > We believe that is not reasonable because clock driver shouldn't
> > handle
> > voltage control. On the other hand, DVFS driver is more suitable
> > for
> > doing this job.
> 
> Either way the DVFS driver handles the voltage change.
> 
> Right now the driver is doing:
> 
> 1. Raise voltage if scaling up
> 2. Mux CCI clock over to stable clock
> 3. Set rate for CCI PLL
> 4. Mux CCI clock back to CCI PLL
> 5. Drop voltage if scaling down
> 
> I'm saying that the clock driver should handle 2+4 transparently when
> any
> driver requests a rate change on the CCI clock. So instead the driver
> would
> do:
> 
> 1. Raise voltage if scaling up
> 2. Set rate for CCI _clock_
>    Here the clock driver would do:
>    a. Mux CCI clock over to stable clock
>    b. Change clock rate for original parent, i.e. the CCI PLL
>    c. Mux CCI clock back to original parent, i.e. the CCI PLL
>    and back to the devfreq driver ...
> 3. Drop voltage if scaling down
> 
> Does that make sense?
> 
> 
> Regards
> ChenYu

Hi Chen-Yu,

Before we mux the CCI clock to an intermediate clock(MAINPLL), we must
ensure that regulator voltage is high enough (we call it intermediate
voltage) to support the intermediate clock rate.

Based on this concept, if we move mux control to clock driver, there
will be a dilemma about which driver to adjust the voltage.

1)When DVFS calls clk_set_rate(), clock driver scales up the regulator
voltage to higher than intermediate voltage and then mux the CCI clock.

This option is not reasonable because clock driver shouldn't handle the
regulators.


2)DVFS scales up the regulator voltage, then calls clk_set_rate(). 
Clock driver mux the CCI clock to the intermediate clock.

This option isn't straightforward and makes one confused easily. For a
person who reads this driver, he may not understand why we adjust the
voltage before clk_set_rate().

That's why we put intermediate voltage/freq together in the DVFS.

BRs,
Johnson Wang
Chen-Yu Tsai May 13, 2022, 3:31 a.m. UTC | #8
On Thu, May 12, 2022 at 9:05 PM Johnson Wang <johnson.wang@mediatek.com> wrote:
>
> On Wed, 2022-05-11 at 18:48 +0800, Chen-Yu Tsai wrote:
> > On Mon, May 9, 2022 at 8:14 PM Johnson Wang <
> > johnson.wang@mediatek.com> wrote:
> > >
> > > Hi Chen-Yu,
> > >
> > > On Tue, 2022-04-26 at 11:18 +0800, Chen-Yu Tsai wrote:
> > > > On Mon, Apr 25, 2022 at 8:56 PM Johnson Wang <
> > > > johnson.wang@mediatek.com> wrote:
> > > > >
> > > > > Add devicetree binding of MediaTek CCI on MT8183 and MT8186.
> > > > >
> > > > > Signed-off-by: Johnson Wang <johnson.wang@mediatek.com>
> > > > > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > > > > ---
> > > > >  .../bindings/interconnect/mediatek,cci.yaml   | 139
> > > > > ++++++++++++++++++
> > > > >  1 file changed, 139 insertions(+)
> > > > >  create mode 100644
> > > > > Documentation/devicetree/bindings/interconnect/mediatek,cci.yam
> > > > > l
> > > > >
> > > > > diff --git
> > > > > a/Documentation/devicetree/bindings/interconnect/mediatek,cci.y
> > > > > aml
> > > > > b/Documentation/devicetree/bindings/interconnect/mediatek,cci.y
> > > > > aml
> > > > > new file mode 100644
> > > > > index 000000000000..e5221e17d11b
> > > > > --- /dev/null
> > > > > +++
> > > > > b/Documentation/devicetree/bindings/interconnect/mediatek,cci.y
> > > > > aml
> > > > > @@ -0,0 +1,139 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id:
> > > > >
> https://urldefense.com/v3/__http://devicetree.org/schemas/interconnect/mediatek,cci.yaml*__;Iw!!CTRNKA9wMg0ARbw!zuufEcqpKbditY3eqLTHpL8P8humMCyh4D4QWsximmw124tJUPE3ZBUyBqBtDlQ9pSDO$
> > > > >
> > > > > +$schema:
> > > > >
> https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!zuufEcqpKbditY3eqLTHpL8P8humMCyh4D4QWsximmw124tJUPE3ZBUyBqBtDoE9YHyu$
> > > > >
> > > > > +
> > > > > +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) is a hardware
> > > > > engine
> > > > > used by
> > > > > +  MT8183 and MT8186 SoCs to scale the frequency and adjust the
> > > > > voltage in
> > > > > +  hardware. It can also optimize the voltage to reduce the
> > > > > power
> > > > > consumption.
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    enum:
> > > > > +      - mediatek,mt8183-cci
> > > > > +      - mediatek,mt8186-cci
> > > > > +
> > > > > +  clocks:
> > > > > +    items:
> > > > > +      - description:
> > > > > +          The multiplexer for clock input of CPU cluster.
> > > >
> > > > of the bus, not CPU cluster.
> > >
> > > Thanks for your suggestion.
> > > I will correct it in the next version.
> > >
> > > >
> > > > > +      - 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.
> > > >
> > > > This really should be handled in the clk controller, and not by
> > > > every
> > > > device
> > > > that happens to take a clock from a mux with upstream PLLs that
> > > > can
> > > > change
> > > > in clock rate. The end device hardware only takes one clock
> > > > input.
> > > > That's it.
> > > >
> > >
> > > To make this intermediate clock works properly, this driver is also
> > > responsible for handling the Vproc voltage and ensures the voltage
> > > is
> > > high enough to support intermediate clock rate.
> > >
> > > If we move intermediate clock rate control to clock driver, then
> > > intermediate voltage control may be handled by the clock driver
> > > itself
> > > as well.
> > >
> > > We believe that is not reasonable because clock driver shouldn't
> > > handle
> > > voltage control. On the other hand, DVFS driver is more suitable
> > > for
> > > doing this job.
> >
> > Either way the DVFS driver handles the voltage change.
> >
> > Right now the driver is doing:
> >
> > 1. Raise voltage if scaling up
> > 2. Mux CCI clock over to stable clock
> > 3. Set rate for CCI PLL
> > 4. Mux CCI clock back to CCI PLL
> > 5. Drop voltage if scaling down
> >
> > I'm saying that the clock driver should handle 2+4 transparently when
> > any
> > driver requests a rate change on the CCI clock. So instead the driver
> > would
> > do:
> >
> > 1. Raise voltage if scaling up
> > 2. Set rate for CCI _clock_
> >    Here the clock driver would do:
> >    a. Mux CCI clock over to stable clock
> >    b. Change clock rate for original parent, i.e. the CCI PLL
> >    c. Mux CCI clock back to original parent, i.e. the CCI PLL
> >    and back to the devfreq driver ...
> > 3. Drop voltage if scaling down
> >
> > Does that make sense?
> >
> >
> > Regards
> > ChenYu
>
> Hi Chen-Yu,
>
> Before we mux the CCI clock to an intermediate clock(MAINPLL), we must
> ensure that regulator voltage is high enough (we call it intermediate
> voltage) to support the intermediate clock rate.
>
> Based on this concept, if we move mux control to clock driver, there
> will be a dilemma about which driver to adjust the voltage.
>
> 1)When DVFS calls clk_set_rate(), clock driver scales up the regulator
> voltage to higher than intermediate voltage and then mux the CCI clock.
>
> This option is not reasonable because clock driver shouldn't handle the
> regulators.
>
>
> 2)DVFS scales up the regulator voltage, then calls clk_set_rate().
> Clock driver mux the CCI clock to the intermediate clock.
>
> This option isn't straightforward and makes one confused easily. For a
> person who reads this driver, he may not understand why we adjust the
> voltage before clk_set_rate().
>
> That's why we put intermediate voltage/freq together in the DVFS.

Thanks for the explanation. The intermediate clock's rate being higher
than the lowest OPP is the key I missed.

I can't think of a better way to describe this in DT. The intermediate
clock's rate is stable, but it is set either through hardware reset defaults
or firmware, so we can't just assume a given clock rate and hard code that.
Having a direct reference to the clock seems simpler.


Regards
ChenYu
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml b/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
new file mode 100644
index 000000000000..e5221e17d11b
--- /dev/null
+++ b/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
@@ -0,0 +1,139 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interconnect/mediatek,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) is a hardware engine used by
+  MT8183 and MT8186 SoCs to scale the frequency and adjust the voltage in
+  hardware. It can also optimize the voltage to reduce the power consumption.
+
+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: true
+  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 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>;
+    };
+
+    cci_opp: opp-table-cci {
+        compatible = "operating-points-v2";
+        opp-shared;
+        opp2_00: opp-273000000 {
+            opp-hz = /bits/ 64 <273000000>;
+            opp-microvolt = <650000>;
+        };
+        opp2_01: opp-338000000 {
+            opp-hz = /bits/ 64 <338000000>;
+            opp-microvolt = <687500>;
+        };
+        opp2_02: opp-403000000 {
+            opp-hz = /bits/ 64 <403000000>;
+            opp-microvolt = <718750>;
+        };
+        opp2_03: opp-463000000 {
+            opp-hz = /bits/ 64 <463000000>;
+            opp-microvolt = <756250>;
+        };
+        opp2_04: opp-546000000 {
+            opp-hz = /bits/ 64 <546000000>;
+            opp-microvolt = <800000>;
+        };
+        opp2_05: opp-624000000 {
+            opp-hz = /bits/ 64 <624000000>;
+            opp-microvolt = <818750>;
+        };
+        opp2_06: opp-689000000 {
+            opp-hz = /bits/ 64 <689000000>;
+            opp-microvolt = <850000>;
+        };
+        opp2_07: opp-767000000 {
+            opp-hz = /bits/ 64 <767000000>;
+            opp-microvolt = <868750>;
+        };
+        opp2_08: opp-845000000 {
+            opp-hz = /bits/ 64 <845000000>;
+            opp-microvolt = <893750>;
+        };
+        opp2_09: opp-871000000 {
+            opp-hz = /bits/ 64 <871000000>;
+            opp-microvolt = <906250>;
+        };
+        opp2_10: opp-923000000 {
+            opp-hz = /bits/ 64 <923000000>;
+            opp-microvolt = <931250>;
+        };
+        opp2_11: opp-962000000 {
+            opp-hz = /bits/ 64 <962000000>;
+            opp-microvolt = <943750>;
+        };
+        opp2_12: opp-1027000000 {
+            opp-hz = /bits/ 64 <1027000000>;
+            opp-microvolt = <975000>;
+        };
+        opp2_13: opp-1092000000 {
+            opp-hz = /bits/ 64 <1092000000>;
+            opp-microvolt = <1000000>;
+        };
+        opp2_14: opp-1144000000 {
+            opp-hz = /bits/ 64 <1144000000>;
+            opp-microvolt = <1025000>;
+        };
+        opp2_15: opp-1196000000 {
+            opp-hz = /bits/ 64 <1196000000>;
+            opp-microvolt = <1050000>;
+        };
+    };