linux-kernel - Re: [PATCH v3 1/2] dt-bindings: interconnect: Add MediaTek CCI dt-bindings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAGXv+5EfoOx8LHwHeL+wva_M0KX4S3qLNsBgk_003hvXxYPRVQ@mail.gmail.com>
Date:   Fri, 13 May 2022 11:31:25 +0800
From:   Chen-Yu Tsai <wenst@...omium.org>
To:     Johnson Wang <johnson.wang@...iatek.com>
Cc:     krzk+dt@...nel.org, 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

On Thu, May 12, 2022 at 9:05 PM Johnson Wang <johnson.wang@...iatek.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@...iatek.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@...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.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@...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.
> >
> > 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