linux-kernel - Re: [PATCH v5 23/24] arm64: dts: mediatek: Add mt8192 clock controllers

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <160819523908.1580929.4609336150609889474@swboyd.mtv.corp.google.com>
Date:   Thu, 17 Dec 2020 00:53:59 -0800
From:   Stephen Boyd <sboyd@...nel.org>
To:     Ikjoon Jang <ikjn@...omium.org>, Weiyi Lu <weiyi.lu@...iatek.com>
Cc:     Matthias Brugger <matthias.bgg@...il.com>,
        Rob Herring <robh@...nel.org>,
        Nicolas Boichat <drinkcat@...omium.org>,
        srv_heupstream@...iatek.com, linux-kernel@...r.kernel.org,
        linux-mediatek@...ts.infradead.org,
        Yingjoe Chen <yingjoe.chen@...iatek.com>,
        linux-clk@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v5 23/24] arm64: dts: mediatek: Add mt8192 clock controllers

Quoting Ikjoon Jang (2020-11-22 20:02:37)
> On Mon, Nov 09, 2020 at 10:03:48AM +0800, Weiyi Lu wrote:
> > Add clock controller nodes for SoC mt8192
> > 
> > Signed-off-by: Weiyi Lu <weiyi.lu@...iatek.com>
> > ---
> >  arch/arm64/boot/dts/mediatek/mt8192.dtsi | 163 +++++++++++++++++++++++++++++++
> >  1 file changed, 163 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8192.dtsi b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > index e12e024..92dcfbd 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > +++ b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > @@ -5,6 +5,7 @@
> >   */
> >  
> >  /dts-v1/;
> > +#include <dt-bindings/clock/mt8192-clk.h>
> >  #include <dt-bindings/interrupt-controller/arm-gic.h>
> >  #include <dt-bindings/interrupt-controller/irq.h>
> >  #include <dt-bindings/pinctrl/mt8192-pinfunc.h>
> > @@ -213,6 +214,24 @@
> >                       };
> >               };
> >  
> > +             topckgen: syscon@...00000 {
> > +                     compatible = "mediatek,mt8192-topckgen", "syscon";
> > +                     reg = <0 0x10000000 0 0x1000>;
> > +                     #clock-cells = <1>;
> > +             };
> > +
> > +             infracfg: syscon@...01000 {
> > +                     compatible = "mediatek,mt8192-infracfg", "syscon";
> > +                     reg = <0 0x10001000 0 0x1000>;
> > +                     #clock-cells = <1>;
> > +             };
> > +
> > +             pericfg: syscon@...03000 {
> > +                     compatible = "mediatek,mt8192-pericfg", "syscon";
> > +                     reg = <0 0x10003000 0 0x1000>;
> > +                     #clock-cells = <1>;
> > +             };
> > +
> 
> There are 26 new bindings for mt8192 clock providers, "mediatek,mt8192-*'.
> I guess the one reason of doing this is that those mmio blocks are
> just scattered all around over different memory regions.

Yeah it seems that Mediatek likes to scatter the clks into basically
every IP block. The alternate approach would be to create virtual
platform device children of those IP blocks to register the clks.

> 
> I wonder if there could be a simpler way of merging them into one
> binding of "mediatek,mt8192-clocks" and converting all new bindings
> into generic syscon:
> 
>         mt8192-clocks: mt8192_clocks {
>                 compatible = "mediatek,mt8192-clocks";
>                 #clock-cells = <1>;
> 
>                 infracfg: clk_infracfg {
>                         syscon = <&syscon_infracfg>;
>                 };
>                 pericfg: clk_pericfg {
>                         syscon = <&syscon_pericfg>:
>                 };
>         };
> 
>         syscon_pericfg: syscon@...03000 {
>                 compatible = "syscon";
>                 reg = <0 0x10003000 0 0x1000>;
>         };

Is the syscon used for anything besides a clk provider? Having a
mt8192_clocks node is wrong. The syscon is the clk provider and it
should have a #clock-cells property. Making up a node that doesn't have
a reg property is usually a sign that something is wrong.