Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220425125546.4129-1-johnson.wang@mediatek.com>
 <20220425125546.4129-2-johnson.wang@mediatek.com> <CAGXv+5HgyN+kp86M2GgFtbruXSAMSLxsh9vf8zVE5TxRMyTyaA@mail.gmail.com>
 <a8e5fd9de1feece9051e1624c5cf3b672131a122.camel@mediatek.com>
In-Reply-To: <a8e5fd9de1feece9051e1624c5cf3b672131a122.camel@mediatek.com>
From:   Chen-Yu Tsai <wenst@chromium.org>
Date:   Wed, 11 May 2022 18:48:06 +0800
Message-ID: <CAGXv+5G6Qv1JcsZNrw5=USJSzPkczW42FgXRYqAK=xS-moO6nw@mail.gmail.com>
Subject: Re: [PATCH v3 1/2] dt-bindings: interconnect: Add MediaTek CCI dt-bindings
To:     Johnson Wang <johnson.wang@mediatek.com>
Cc:     krzk+dt@kernel.org, cw00.choi@samsung.com, robh+dt@kernel.org,
        kyungmin.park@samsung.com, khilman@kernel.org,
        linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org, jia-wei.chang@mediatek.com,
        Project_Global_Chrome_Upstream_Group@mediatek.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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