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 for Android: free password hash cracker in your pocket
[<prev] [next>] [day] [month] [year] [list]
Date:   Thu, 24 Mar 2022 09:57:50 -0400
From:   Nícolas F. R. A. Prado 
        <nfraprado@...labora.com>
To:     "allen-kh.cheng" <allen-kh.cheng@...iatek.com>
Cc:     Matthias Brugger <matthias.bgg@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>,
        Project_Global_Chrome_Upstream_Group@...iatek.com,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linux-mediatek@...ts.infradead.org,
        Chen-Yu Tsai <wenst@...omium.org>,
        Ryder Lee <ryder.lee@...nel.org>,
        Hui Liu <hui.liu@...iatek.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
Subject: Re: [PATCH v4 08/22] arm64: dts: mt8192: Add infracfg_rst node

On Wed, Mar 23, 2022 at 02:27:00PM +0800, allen-kh.cheng wrote:
> Hi Nícolas,
> 
> On Tue, 2022-03-22 at 17:57 -0400, Nícolas F. R. A. Prado wrote:
> > Hi Allen,
> > 
> > please see my comment below.
> > 
> > On Fri, Mar 18, 2022 at 10:45:20PM +0800, Allen-KH Cheng wrote:
> > > Add infracfg_rst node for mt8192 SoC.
> > >  - Add simple-mfd to allow probing the ti,syscon-reset node.
> > > 
> > > Signed-off-by: Allen-KH Cheng <allen-kh.cheng@...iatek.com>
> > > Reviewed-by: AngeloGioacchino Del Regno <
> > > angelogioacchino.delregno@...labora.com>
> > > ---
> > >  arch/arm64/boot/dts/mediatek/mt8192.dtsi | 18 ++++++++++++++++--
> > >  1 file changed, 16 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > > b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > > index 40cf6dacca3e..82de1af3f6aa 100644
> > > --- a/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > > +++ b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > > @@ -12,6 +12,7 @@
> > >  #include <dt-bindings/pinctrl/mt8192-pinfunc.h>
> > >  #include <dt-bindings/phy/phy.h>
> > >  #include <dt-bindings/power/mt8192-power.h>
> > > +#include <dt-bindings/reset/ti-syscon.h>
> > >  
> > >  / {
> > >  	compatible = "mediatek,mt8192";
> > > @@ -267,10 +268,23 @@
> > >  			#clock-cells = <1>;
> > >  		};
> > >  
> > > -		infracfg: syscon@...01000 {
> > > -			compatible = "mediatek,mt8192-infracfg",
> > > "syscon";
> > > +		infracfg: infracfg@...01000 {
> > > +			compatible = "mediatek,mt8192-infracfg",
> > > "syscon", "simple-mfd";
> > >  			reg = <0 0x10001000 0 0x1000>;
> > >  			#clock-cells = <1>;
> > > +
> > > +			infracfg_rst: reset-controller {
> > > +				compatible = "ti,syscon-reset";
> > > +				#reset-cells = <1>;
> > > +
> > > +				ti,reset-bits = <
> > > +					0x120 0 0x124 0 0 0	(ASSERT_SET
> > > | DEASSERT_SET | STATUS_NONE) /* 0: lvts_ap */
> > > +					0x730 12 0x734 12 0 0	(AS
> > > SERT_SET | DEASSERT_SET | STATUS_NONE) /* 1: lvts_mcu */
> > > +					0x140 15 0x144 15 0 0	(AS
> > > SERT_SET | DEASSERT_SET | STATUS_NONE) /* 2: pcie phy */
> > > +					0x730 1 0x734 1 0 0	(ASSERT_SET
> > > | DEASSERT_SET | STATUS_NONE) /* 3: pcie top */
> > > +					0x150 5 0x154 5 0 0	(ASSERT_SET
> > > | DEASSERT_SET | STATUS_NONE) /* 4: svs */
> > > +				>;
> > 
> > If you see [1], Rob has previously said that there shouldn't be new
> > users of the
> > ti,reset-bits property. I suggest doing like proposed on [2]: moving
> > these bit
> > definitions to the reset-ti-syscon driver, and have them selected
> > through the
> > compatible. You'd need to add a mt8192 specific compatible here too
> > for that.
> > 
> > [1] 
> > https://urldefense.com/v3/__https://lore.kernel.org/all/CAL_JsqJq6gqoXtvG1U7UDsOQpz7oMLMunZHq2njN6nvPr8PZMA@mail.gmail.com/__;!!CTRNKA9wMg0ARbw!1wQAhHnu8bAxe2O51XZ61oWVQU7EFEZcgluzwgP4x4VHRxtb6kAySvsKCGzv8cs8IzVjanDNzBQvOa_Y4OABdRVOzg$
> >  
> > [2] 
> > https://urldefense.com/v3/__https://lore.kernel.org/all/CAATdQgA5pKhjOf5gxo*h7cs7kCts3DeKGU5axeX2t*OaJFHyBg@mail.gmail.com/__;Kys!!CTRNKA9wMg0ARbw!1wQAhHnu8bAxe2O51XZ61oWVQU7EFEZcgluzwgP4x4VHRxtb6kAySvsKCGzv8cs8IzVjanDNzBQvOa_Y4OBLvOYlyQ$
> >  
> > 
> > Thanks,
> > Nícolas
> > 
> 
> Thanks for your comment.
> 
> For nfracfg_rst node, I prefer remove it from this series and
> send another patch series(dts and driver).

Yes, that sounds the best approach to me as well.

> 
> Based on [2], is it ok that we can add mt8192 compatible in reset-ti
> syscon driver? (even if mt8192 is a mediatek platform)

Actually, I think there's an even better way of handling this. Instead of using
the TI syscon reset controller, you could give reset controller capabilities to
the infracfg node directly. This means adding reset controller support to the
common mtk clock driver (clk-mtk.c) and registering the reset controller on
clk-mt8192.c for infracfg. By making this common code you'll also be able to
reuse it for mt8195 as well. And there would no longer be a infracfg_rst node.

Thanks,
Nícolas

> 
> best regards,
> Allen
> 
> > > +			};
> > >  		};
> > >  
> > >  		pericfg: syscon@...03000 {
> > > -- 
> > > 2.18.0
> > > 
> > > 
> 

Powered by blists - more mailing lists