diff mbox series

[v4,08/22] arm64: dts: mt8192: Add infracfg_rst node

Message ID 20220318144534.17996-9-allen-kh.cheng@mediatek.com (mailing list archive)
State New, archived
Headers show
Series Add driver nodes for MT8192 SoC | expand

Commit Message

Allen-KH Cheng March 18, 2022, 2:45 p.m. UTC 
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@mediatek.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 arch/arm64/boot/dts/mediatek/mt8192.dtsi | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

Comments

Nícolas F. R. A. Prado March 22, 2022, 9:57 p.m. UTC | #1 
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@mediatek.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.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@10001000 {
> -			compatible = "mediatek,mt8192-infracfg", "syscon";
> +		infracfg: infracfg@10001000 {
> +			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	(ASSERT_SET | DEASSERT_SET | STATUS_NONE) /* 1: lvts_mcu */
> +					0x140 15 0x144 15 0 0	(ASSERT_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://lore.kernel.org/all/CAL_JsqJq6gqoXtvG1U7UDsOQpz7oMLMunZHq2njN6nvPr8PZMA@mail.gmail.com/
[2] https://lore.kernel.org/all/CAATdQgA5pKhjOf5gxo+h7cs7kCts3DeKGU5axeX2t+OaJFHyBg@mail.gmail.com/

Thanks,
Nícolas

> +			};
>  		};
>  
>  		pericfg: syscon@10003000 {
> -- 
> 2.18.0
> 
>
Allen-KH Cheng March 23, 2022, 6:27 a.m. UTC | #2 
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@mediatek.com>
> > Reviewed-by: AngeloGioacchino Del Regno <
> > angelogioacchino.delregno@collabora.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@10001000 {
> > -			compatible = "mediatek,mt8192-infracfg",
> > "syscon";
> > +		infracfg: infracfg@10001000 {
> > +			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).

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

best regards,
Allen

> > +			};
> >  		};
> >  
> >  		pericfg: syscon@10003000 {
> > -- 
> > 2.18.0
> > 
> >
Nícolas F. R. A. Prado March 24, 2022, 1:57 p.m. UTC | #3 
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@mediatek.com>
> > > Reviewed-by: AngeloGioacchino Del Regno <
> > > angelogioacchino.delregno@collabora.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@10001000 {
> > > -			compatible = "mediatek,mt8192-infracfg",
> > > "syscon";
> > > +		infracfg: infracfg@10001000 {
> > > +			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@10003000 {
> > > -- 
> > > 2.18.0
> > > 
> > > 
>
Allen-KH Cheng March 29, 2022, 3:10 a.m. UTC | #4 
On Thu, 2022-03-24 at 09:57 -0400, Nícolas F. R. A. Prado wrote:
> 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@mediatek.com>
> > > > Reviewed-by: AngeloGioacchino Del Regno <
> > > > angelogioacchino.delregno@collabora.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@10001000 {
> > > > -			compatible = "mediatek,mt8192-
> > > > infracfg",
> > > > "syscon";
> > > > +		infracfg: infracfg@10001000 {
> > > > +			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
> 

HI Nícolas,

Thanks for your suggestion.

We will send another patch for reset conroller.


Best regards,
Allenn

> > 
> > best regards,
> > Allen
> > 
> > > > +			};
> > > >  		};
> > > >  
> > > >  		pericfg: syscon@10003000 {
> > > > -- 
> > > > 2.18.0
> > > > 
> > > >
diff mbox series

Patch

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@10001000 {
-			compatible = "mediatek,mt8192-infracfg", "syscon";
+		infracfg: infracfg@10001000 {
+			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	(ASSERT_SET | DEASSERT_SET | STATUS_NONE) /* 1: lvts_mcu */
+					0x140 15 0x144 15 0 0	(ASSERT_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 */
+				>;
+			};
 		};
 
 		pericfg: syscon@10003000 {