Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <14cdb1e17015e299231e72356e661203d4beda17.camel@mediatek.com>
Subject: Re: [PATCH v3 1/2] dt-bindings: interconnect: Add MediaTek CCI
 dt-bindings
From:   Johnson Wang <johnson.wang@mediatek.com>
To:     Chen-Yu Tsai <wenst@chromium.org>
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>
Date:   Thu, 12 May 2022 21:04:59 +0800
In-Reply-To: <CAGXv+5G6Qv1JcsZNrw5=USJSzPkczW42FgXRYqAK=xS-moO6nw@mail.gmail.com>
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>
         <CAGXv+5G6Qv1JcsZNrw5=USJSzPkczW42FgXRYqAK=xS-moO6nw@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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