diff mbox series

[v2,1/1] arm64: dts: ti: k3-am62-wakeup: Configure ti-sysc for wkup_uart0

Message ID 20231114073209.40756-1-tony@atomide.com
State Superseded
Headers show
Series [v2,1/1] arm64: dts: ti: k3-am62-wakeup: Configure ti-sysc for wkup_uart0 | expand

Commit Message

Tony Lindgren Nov. 14, 2023, 7:32 a.m. UTC
The devices in the wkup domain are capable of waking up the system from
suspend. We can configure the wkup domain devices in a generic way using
the ti-sysc interconnect target module driver like we have done with the
earlier TI SoCs.

As ti-sysc manages the SYSCONFIG related registers independent of the
child hardware device, the wake-up configuration is also set even if
wkup_uart0 is reserved by sysfw.

The wkup_uart0 device has interconnect target module register mapping like
dra7 wkup uart. There is a 1 MB interconnect target range with one uart IP
block in the target module. The power domain and clock affects the whole
interconnect target module.

Note we change the functional clock name to follow the ti-sysc binding
and use "fck" instead of "fclk".

Tested-by: Dhruva Gole <d-gole@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---

Changes since v1:

- Added Tested-by from Dhruva

---
 arch/arm64/boot/dts/ti/k3-am62-wakeup.dtsi | 33 ++++++++++++++++++----
 1 file changed, 27 insertions(+), 6 deletions(-)

Comments

Kevin Hilman Dec. 5, 2023, 6:13 p.m. UTC | #1
Tony Lindgren <tony@atomide.com> writes:

> The devices in the wkup domain are capable of waking up the system from
> suspend. We can configure the wkup domain devices in a generic way using
> the ti-sysc interconnect target module driver like we have done with the
> earlier TI SoCs.
>
> As ti-sysc manages the SYSCONFIG related registers independent of the
> child hardware device, the wake-up configuration is also set even if
> wkup_uart0 is reserved by sysfw.
>
> The wkup_uart0 device has interconnect target module register mapping like
> dra7 wkup uart. There is a 1 MB interconnect target range with one uart IP
> block in the target module. The power domain and clock affects the whole
> interconnect target module.
>
> Note we change the functional clock name to follow the ti-sysc binding
> and use "fck" instead of "fclk".
>
> Tested-by: Dhruva Gole <d-gole@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>
> Changes since v1:
>
> - Added Tested-by from Dhruva
>
> ---
>  arch/arm64/boot/dts/ti/k3-am62-wakeup.dtsi | 33 ++++++++++++++++++----
>  1 file changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/ti/k3-am62-wakeup.dtsi b/arch/arm64/boot/dts/ti/k3-am62-wakeup.dtsi
> --- a/arch/arm64/boot/dts/ti/k3-am62-wakeup.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-am62-wakeup.dtsi
> @@ -5,6 +5,8 @@
>   * Copyright (C) 2020-2022 Texas Instruments Incorporated - https://www.ti.com/
>   */
>  
> +#include <dt-bindings/bus/ti-sysc.h>
> +
>  &cbass_wakeup {
>  	wkup_conf: syscon@43000000 {
>  		bootph-all;
> @@ -21,14 +23,33 @@ chipid: chipid@14 {
>  		};
>  	};
>  
> -	wkup_uart0: serial@2b300000 {
> -		compatible = "ti,am64-uart", "ti,am654-uart";
> -		reg = <0x00 0x2b300000 0x00 0x100>;
> -		interrupts = <GIC_SPI 186 IRQ_TYPE_LEVEL_HIGH>;
> +	target-module@2b300000 {
> +		compatible = "ti,sysc-omap2", "ti,sysc";
> +		reg = <0 0x2b300050 0 0x4>,
> +		      <0 0x2b300054 0 0x4>,
> +		      <0 0x2b300058 0 0x4>;
> +		reg-names = "rev", "sysc", "syss";
> +		ti,sysc-mask = <(SYSC_OMAP2_ENAWAKEUP |
> +				 SYSC_OMAP2_SOFTRESET |
> +				 SYSC_OMAP2_AUTOIDLE)>;
> +		ti,sysc-sidle = <SYSC_IDLE_FORCE>,
> +				<SYSC_IDLE_NO>,
> +				<SYSC_IDLE_SMART>,
> +				<SYSC_IDLE_SMART_WKUP>;
> +		ti,syss-mask = <1>;
>  		power-domains = <&k3_pds 114 TI_SCI_PD_EXCLUSIVE>;
>  		clocks = <&k3_clks 114 0>;

I'm a little confused why these power-domain and clocks stay here and
are not moved under the wkup_uart0 node... 

> -		clock-names = "fclk";
> -		status = "disabled";
> +		clock-names = "fck";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0 0 0x2b300000 0x100000>;
> +
> +		wkup_uart0: serial@2b300000 {
> +			compatible = "ti,am64-uart", "ti,am654-uart";
> +			reg = <0 0x100>;
> +			interrupts = <GIC_SPI 186 IRQ_TYPE_LEVEL_HIGH>;
> +			status = "disabled";

...here.

The SCI device ID 114 is specifically for wkup_uart0[1], so it seems to
me those should be in the wkup_uart0 node.

Kevin

[1] https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/am62x/devices.html
Tony Lindgren Dec. 7, 2023, 6:08 a.m. UTC | #2
* Kevin Hilman <khilman@kernel.org> [231205 18:14]:
> I'm a little confused why these power-domain and clocks stay here and
> are not moved under the wkup_uart0 node... 

The resources are also needed by the interconnect target module. It's the
wrapper IP for the child device(s). In this case there's one chip 8250 IP
instance. In some other devices there can be multiple child IP devices
wired to one target module.

> > -		clock-names = "fclk";
> > -		status = "disabled";
> > +		clock-names = "fck";
> > +		#address-cells = <1>;
> > +		#size-cells = <1>;
> > +		ranges = <0 0 0x2b300000 0x100000>;
> > +
> > +		wkup_uart0: serial@2b300000 {
> > +			compatible = "ti,am64-uart", "ti,am654-uart";
> > +			reg = <0 0x100>;
> > +			interrupts = <GIC_SPI 186 IRQ_TYPE_LEVEL_HIGH>;
> > +			status = "disabled";
> 
> ...here.
> 
> The SCI device ID 114 is specifically for wkup_uart0[1], so it seems to
> me those should be in the wkup_uart0 node.

Those resources are also needed for the parent target module for revision
detection, quirks, reset, idle register configuration, and to probe the
child devices.

Here the 8250 IP can be set to status = "reserved" when used by the
firmware, and 8250 not touched by Linux. However, the parent interconnect
target module still needs to be configured for idle registers and wake-up
path register bit so the wake-up from deeper suspend states works.

Regards,

Tony
Kevin Hilman Dec. 8, 2023, 5:13 p.m. UTC | #3
Tony Lindgren <tony@atomide.com> writes:

> * Kevin Hilman <khilman@kernel.org> [231205 18:14]:
>> I'm a little confused why these power-domain and clocks stay here and
>> are not moved under the wkup_uart0 node... 
>
> The resources are also needed by the interconnect target module. It's the
> wrapper IP for the child device(s). In this case there's one chip 8250 IP
> instance. In some other devices there can be multiple child IP devices
> wired to one target module.
>
>> > -		clock-names = "fclk";
>> > -		status = "disabled";
>> > +		clock-names = "fck";
>> > +		#address-cells = <1>;
>> > +		#size-cells = <1>;
>> > +		ranges = <0 0 0x2b300000 0x100000>;
>> > +
>> > +		wkup_uart0: serial@2b300000 {
>> > +			compatible = "ti,am64-uart", "ti,am654-uart";
>> > +			reg = <0 0x100>;
>> > +			interrupts = <GIC_SPI 186 IRQ_TYPE_LEVEL_HIGH>;
>> > +			status = "disabled";
>> 
>> ...here.
>> 
>> The SCI device ID 114 is specifically for wkup_uart0[1], so it seems to
>> me those should be in the wkup_uart0 node.
>
> Those resources are also needed for the parent target module for revision
> detection, quirks, reset, idle register configuration, and to probe the
> child devices.
>
> Here the 8250 IP can be set to status = "reserved" when used by the
> firmware, and 8250 not touched by Linux. However, the parent interconnect
> target module still needs to be configured for idle registers and wake-up
> path register bit so the wake-up from deeper suspend states works.

OK, makes sense.  Thanks for the clarification.

In that case, shouldn't the same be done for the other wakeup sources
there (e.g. wkup_rtc0) ?

Kevin
Nishanth Menon Dec. 15, 2023, 4 p.m. UTC | #4
On 09:32-20231114, Tony Lindgren wrote:
> The devices in the wkup domain are capable of waking up the system from
> suspend. We can configure the wkup domain devices in a generic way using
> the ti-sysc interconnect target module driver like we have done with the
> earlier TI SoCs.
> 
> As ti-sysc manages the SYSCONFIG related registers independent of the
> child hardware device, the wake-up configuration is also set even if
> wkup_uart0 is reserved by sysfw.
> 
> The wkup_uart0 device has interconnect target module register mapping like
> dra7 wkup uart. There is a 1 MB interconnect target range with one uart IP
> block in the target module. The power domain and clock affects the whole
> interconnect target module.
> 
> Note we change the functional clock name to follow the ti-sysc binding
> and use "fck" instead of "fclk".
> 
> Tested-by: Dhruva Gole <d-gole@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
> 
> Changes since v1:
> 
> - Added Tested-by from Dhruva
> 
> ---
>  arch/arm64/boot/dts/ti/k3-am62-wakeup.dtsi | 33 ++++++++++++++++++----
>  1 file changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-am62-wakeup.dtsi b/arch/arm64/boot/dts/ti/k3-am62-wakeup.dtsi
> --- a/arch/arm64/boot/dts/ti/k3-am62-wakeup.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-am62-wakeup.dtsi
> @@ -5,6 +5,8 @@
>   * Copyright (C) 2020-2022 Texas Instruments Incorporated - https://www.ti.com/
>   */
>  
> +#include <dt-bindings/bus/ti-sysc.h>
> +
>  &cbass_wakeup {
>  	wkup_conf: syscon@43000000 {
>  		bootph-all;
> @@ -21,14 +23,33 @@ chipid: chipid@14 {
>  		};
>  	};
>  
> -	wkup_uart0: serial@2b300000 {
> -		compatible = "ti,am64-uart", "ti,am654-uart";
> -		reg = <0x00 0x2b300000 0x00 0x100>;
> -		interrupts = <GIC_SPI 186 IRQ_TYPE_LEVEL_HIGH>;
> +	target-module@2b300000 {

should be  target-module@2b300050 to match up with reg?

> +		compatible = "ti,sysc-omap2", "ti,sysc";
> +		reg = <0 0x2b300050 0 0x4>,
> +		      <0 0x2b300054 0 0x4>,
> +		      <0 0x2b300058 0 0x4>;
> +		reg-names = "rev", "sysc", "syss";
> +		ti,sysc-mask = <(SYSC_OMAP2_ENAWAKEUP |
> +				 SYSC_OMAP2_SOFTRESET |
> +				 SYSC_OMAP2_AUTOIDLE)>;
> +		ti,sysc-sidle = <SYSC_IDLE_FORCE>,
> +				<SYSC_IDLE_NO>,
> +				<SYSC_IDLE_SMART>,
> +				<SYSC_IDLE_SMART_WKUP>;
> +		ti,syss-mask = <1>;
>  		power-domains = <&k3_pds 114 TI_SCI_PD_EXCLUSIVE>;
>  		clocks = <&k3_clks 114 0>;
> -		clock-names = "fclk";
> -		status = "disabled";
> +		clock-names = "fck";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0 0 0x2b300000 0x100000>;
> +
> +		wkup_uart0: serial@2b300000 {

serial@0  to match up with reg?

> +			compatible = "ti,am64-uart", "ti,am654-uart";
> +			reg = <0 0x100>;
> +			interrupts = <GIC_SPI 186 IRQ_TYPE_LEVEL_HIGH>;
> +			status = "disabled";
> +		};
>  	};
>  
>  	wkup_i2c0: i2c@2b200000 {
> -- 
> 2.42.1
Tony Lindgren Dec. 18, 2023, 7:28 a.m. UTC | #5
* Nishanth Menon <nm@ti.com> [231215 16:00]:
> On 09:32-20231114, Tony Lindgren wrote:
> > -	wkup_uart0: serial@2b300000 {
> > -		compatible = "ti,am64-uart", "ti,am654-uart";
> > -		reg = <0x00 0x2b300000 0x00 0x100>;
> > -		interrupts = <GIC_SPI 186 IRQ_TYPE_LEVEL_HIGH>;
> > +	target-module@2b300000 {
> 
> should be  target-module@2b300050 to match up with reg?

It's best to use the target-module IO range here, not the first reg.
The first reg may be tossed anywhere in the target module address
space depending on the device.

Ideally of course there would be just a standardized range of target
module related registers at the end of the IO space..

> > +		wkup_uart0: serial@2b300000 {
> 
> serial@0  to match up with reg?

Yes thanks for catching this. The 8250 IP is at the beginning of the
target module IO space. Will post an updated patch.

Regards,

Tony
Nishanth Menon Dec. 18, 2023, 4:19 p.m. UTC | #6
On 09:28-20231218, Tony Lindgren wrote:
> * Nishanth Menon <nm@ti.com> [231215 16:00]:
> > On 09:32-20231114, Tony Lindgren wrote:
> > > -	wkup_uart0: serial@2b300000 {
> > > -		compatible = "ti,am64-uart", "ti,am654-uart";
> > > -		reg = <0x00 0x2b300000 0x00 0x100>;
> > > -		interrupts = <GIC_SPI 186 IRQ_TYPE_LEVEL_HIGH>;
> > > +	target-module@2b300000 {
> > 
> > should be  target-module@2b300050 to match up with reg?
> 
> It's best to use the target-module IO range here, not the first reg.
> The first reg may be tossed anywhere in the target module address
> space depending on the device.
> 
> Ideally of course there would be just a standardized range of target
> module related registers at the end of the IO space..

Sorry Tony, but the node address must matchup with the reg description
for the node. I'd rather not sit and argue about this with automated
tools and deal with trivial patches - so either tools like dtc need to
ignore this or lets just fix the node address.
 
> > > +		wkup_uart0: serial@2b300000 {
> > 
> > serial@0  to match up with reg?
> 
> Yes thanks for catching this. The 8250 IP is at the beginning of the
> target module IO space. Will post an updated patch.
> 
> Regards,
> 
> Tony
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/ti/k3-am62-wakeup.dtsi b/arch/arm64/boot/dts/ti/k3-am62-wakeup.dtsi
--- a/arch/arm64/boot/dts/ti/k3-am62-wakeup.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am62-wakeup.dtsi
@@ -5,6 +5,8 @@ 
  * Copyright (C) 2020-2022 Texas Instruments Incorporated - https://www.ti.com/
  */
 
+#include <dt-bindings/bus/ti-sysc.h>
+
 &cbass_wakeup {
 	wkup_conf: syscon@43000000 {
 		bootph-all;
@@ -21,14 +23,33 @@  chipid: chipid@14 {
 		};
 	};
 
-	wkup_uart0: serial@2b300000 {
-		compatible = "ti,am64-uart", "ti,am654-uart";
-		reg = <0x00 0x2b300000 0x00 0x100>;
-		interrupts = <GIC_SPI 186 IRQ_TYPE_LEVEL_HIGH>;
+	target-module@2b300000 {
+		compatible = "ti,sysc-omap2", "ti,sysc";
+		reg = <0 0x2b300050 0 0x4>,
+		      <0 0x2b300054 0 0x4>,
+		      <0 0x2b300058 0 0x4>;
+		reg-names = "rev", "sysc", "syss";
+		ti,sysc-mask = <(SYSC_OMAP2_ENAWAKEUP |
+				 SYSC_OMAP2_SOFTRESET |
+				 SYSC_OMAP2_AUTOIDLE)>;
+		ti,sysc-sidle = <SYSC_IDLE_FORCE>,
+				<SYSC_IDLE_NO>,
+				<SYSC_IDLE_SMART>,
+				<SYSC_IDLE_SMART_WKUP>;
+		ti,syss-mask = <1>;
 		power-domains = <&k3_pds 114 TI_SCI_PD_EXCLUSIVE>;
 		clocks = <&k3_clks 114 0>;
-		clock-names = "fclk";
-		status = "disabled";
+		clock-names = "fck";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0 0 0x2b300000 0x100000>;
+
+		wkup_uart0: serial@2b300000 {
+			compatible = "ti,am64-uart", "ti,am654-uart";
+			reg = <0 0x100>;
+			interrupts = <GIC_SPI 186 IRQ_TYPE_LEVEL_HIGH>;
+			status = "disabled";
+		};
 	};
 
 	wkup_i2c0: i2c@2b200000 {