Message-ID: <a8e5fd9de1feece9051e1624c5cf3b672131a122.camel@mediatek.com>
Date:   Mon, 9 May 2022 20:14:44 +0800
From:   Johnson Wang <johnson.wang@...iatek.com>
To:     Chen-Yu Tsai <wenst@...omium.org>, <krzk+dt@...nel.org>
CC:     <cw00.choi@...sung.com>, <robh+dt@...nel.org>,
        <kyungmin.park@...sung.com>, <khilman@...nel.org>,
        <linux-pm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-mediatek@...ts.infradead.org>, <jia-wei.chang@...iatek.com>,
        <Project_Global_Chrome_Upstream_Group@...iatek.com>
Subject: Re: [PATCH v3 1/2] dt-bindings: interconnect: Add MediaTek CCI
 dt-bindings

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@...iatek.com> wrote:
> > 
> > Add devicetree binding of MediaTek CCI on MT8183 and MT8186.
> > 
> > Signed-off-by: Johnson Wang <johnson.wang@...iatek.com>
> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@...iatek.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@...iatek.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@...ts.infradead.org
> > 
https://urldefense.com/v3/__http://lists.infradead.org/mailman/listinfo/linux-mediatek__;!!CTRNKA9wMg0ARbw!zuufEcqpKbditY3eqLTHpL8P8humMCyh4D4QWsximmw124tJUPE3ZBUyBqBtDvdNpOFZ$
> >  

Powered by blists - more mailing lists