diff mbox series

[5/7] ARM: dts: rtd1195: Introduce r-bus

Message ID 20191111030434.29977-6-afaerber@suse.de
State Superseded
Headers show
Series arm64: dts: Initial RTD1395 and BPi-M4 support | expand

Commit Message

Andreas Färber Nov. 11, 2019, 3:04 a.m. UTC
Model Realtek's register bus in DT.

Signed-off-by: Andreas Färber <afaerber@suse.de>

---
 This could be squashed into the original RTD1195 patch.
 
 arch/arm/boot/dts/rtd1195.dtsi | 52 ++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 22 deletions(-)

-- 
2.16.4

Comments

James Tai Nov. 13, 2019, 2:53 a.m. UTC | #1
Hi Andreas,

> +		rbus: r-bus@18000000 {

> +			compatible = "simple-bus";

> +			reg = <0x18000000 0x100000>;

> +			#address-cells = <1>;

> +			#size-cells = <1>;

> +			ranges = <0x0 0x18000000 0x100000>;

> +


The r-bus size of RTD1195 is 0x70000‬.

Regards,
James
Andreas Färber Nov. 15, 2019, 12:16 a.m. UTC | #2
Hi James,

Am 13.11.19 um 03:53 schrieb James Tai:
>> +		rbus: r-bus@18000000 {

>> +			compatible = "simple-bus";

>> +			reg = <0x18000000 0x100000>;

>> +			#address-cells = <1>;

>> +			#size-cells = <1>;

>> +			ranges = <0x0 0x18000000 0x100000>;

>> +

> 

> The r-bus size of RTD1195 is 0x70000‬.


Fixed, also further above for the soc node. This now leaves a gap until
0x18100000 - is that gap RAM or non-r-bus registers then?

		ranges = <0x18000000 0x18000000 0x00070000>,
		         <0x18100000 0x18100000 0x01000000>,
		         <0x40000000 0x40000000 0xc0000000>;

Did you also review the other two ranges? The middle one was labeled NOR
flash somewhere - are start and size correct? The final one depends on
the maximum RAM size - does RTD1195 allow more than 1 GiB RAM? All
non-RAM regions should be covered here.

So another question, applicable to all SoCs: This reserved Boot ROM area
at the start of the address space, here of size 0xa800, is that copied
into RAM, or is that the actual ROM overlapping RAM? If the latter, we
should exclude it from /memory@0's reg (making it /memory@a800), and add
it to soc's ranges here for correctness.

With the follow-up question: Is it correct that, given the size 0xa800,
I have a gap between /memreserve/s from 0xa800 to 0xc000, or should we
reserve that gap by extending the next /memreserve/ or inserting one?

Thanks,
Andreas

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)
Rob Herring Nov. 15, 2019, 1:34 a.m. UTC | #3
On Sun, Nov 10, 2019 at 9:04 PM Andreas Färber <afaerber@suse.de> wrote:
>

> Model Realtek's register bus in DT.

>

> Signed-off-by: Andreas Färber <afaerber@suse.de>

> ---

>  This could be squashed into the original RTD1195 patch.

>

>  arch/arm/boot/dts/rtd1195.dtsi | 52 ++++++++++++++++++++++++------------------

>  1 file changed, 30 insertions(+), 22 deletions(-)

>

> diff --git a/arch/arm/boot/dts/rtd1195.dtsi b/arch/arm/boot/dts/rtd1195.dtsi

> index a8cc2d23e7ef..3439647ecf97 100644

> --- a/arch/arm/boot/dts/rtd1195.dtsi

> +++ b/arch/arm/boot/dts/rtd1195.dtsi

> @@ -92,28 +92,36 @@

>                          <0x18100000 0x18100000 0x01000000>,

>                          <0x40000000 0x40000000 0xc0000000>;

>

> -               wdt: watchdog@18007680 {

> -                       compatible = "realtek,rtd1295-watchdog";

> -                       reg = <0x18007680 0x100>;

> -                       clocks = <&osc27M>;

> -               };

> -

> -               uart0: serial@18007800 {

> -                       compatible = "snps,dw-apb-uart";

> -                       reg = <0x18007800 0x400>;

> -                       reg-shift = <2>;

> -                       reg-io-width = <4>;

> -                       clock-frequency = <27000000>;

> -                       status = "disabled";

> -               };

> -

> -               uart1: serial@1801b200 {

> -                       compatible = "snps,dw-apb-uart";

> -                       reg = <0x1801b200 0x100>;

> -                       reg-shift = <2>;

> -                       reg-io-width = <4>;

> -                       clock-frequency = <27000000>;

> -                       status = "disabled";

> +               rbus: r-bus@18000000 {


Following node names should be generic: bus@...

> +                       compatible = "simple-bus";

> +                       reg = <0x18000000 0x100000>;

> +                       #address-cells = <1>;

> +                       #size-cells = <1>;

> +                       ranges = <0x0 0x18000000 0x100000>;

> +

> +                       wdt: watchdog@7680 {

> +                               compatible = "realtek,rtd1295-watchdog";

> +                               reg = <0x7680 0x100>;

> +                               clocks = <&osc27M>;

> +                       };

> +

> +                       uart0: serial@7800 {

> +                               compatible = "snps,dw-apb-uart";

> +                               reg = <0x7800 0x400>;

> +                               reg-shift = <2>;

> +                               reg-io-width = <4>;

> +                               clock-frequency = <27000000>;

> +                               status = "disabled";

> +                       };

> +

> +                       uart1: serial@1b200 {

> +                               compatible = "snps,dw-apb-uart";

> +                               reg = <0x1b200 0x100>;

> +                               reg-shift = <2>;

> +                               reg-io-width = <4>;

> +                               clock-frequency = <27000000>;

> +                               status = "disabled";

> +                       };

>                 };

>

>                 gic: interrupt-controller@ff011000 {

> --

> 2.16.4

>
Andreas Färber Nov. 15, 2019, 1:51 a.m. UTC | #4
Am 15.11.19 um 02:34 schrieb Rob Herring:
>> diff --git a/arch/arm/boot/dts/rtd1195.dtsi b/arch/arm/boot/dts/rtd1195.dtsi

>> index a8cc2d23e7ef..3439647ecf97 100644

>> --- a/arch/arm/boot/dts/rtd1195.dtsi

>> +++ b/arch/arm/boot/dts/rtd1195.dtsi

>> @@ -92,28 +92,36 @@

>>                          <0x18100000 0x18100000 0x01000000>,

>>                          <0x40000000 0x40000000 0xc0000000>;

>>

>> -               wdt: watchdog@18007680 {

>> -                       compatible = "realtek,rtd1295-watchdog";

>> -                       reg = <0x18007680 0x100>;

>> -                       clocks = <&osc27M>;

>> -               };

>> -

>> -               uart0: serial@18007800 {

>> -                       compatible = "snps,dw-apb-uart";

>> -                       reg = <0x18007800 0x400>;

>> -                       reg-shift = <2>;

>> -                       reg-io-width = <4>;

>> -                       clock-frequency = <27000000>;

>> -                       status = "disabled";

>> -               };

>> -

>> -               uart1: serial@1801b200 {

>> -                       compatible = "snps,dw-apb-uart";

>> -                       reg = <0x1801b200 0x100>;

>> -                       reg-shift = <2>;

>> -                       reg-io-width = <4>;

>> -                       clock-frequency = <27000000>;

>> -                       status = "disabled";

>> +               rbus: r-bus@18000000 {

> 

> Following node names should be generic: bus@...


Fixed for all four SoCs.

That seems inconsistent with specific pci@... & usb@... (both from OF),
but I see now the Amlogic-specific buses that I was inspired by do use
bus@..., too.

Thanks,
Andreas

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)
James Tai Nov. 18, 2019, 6:53 a.m. UTC | #5
Hi Andreas,

> 

> Fixed, also further above for the soc node. This now leaves a gap until

> 0x18100000 - is that gap RAM or non-r-bus registers then?

> 

> 		ranges = <0x18000000 0x18000000 0x00070000>,

> 		         <0x18100000 0x18100000 0x01000000>,

> 		         <0x40000000 0x40000000 0xc0000000>;

> 


> Did you also review the other two ranges? The middle one was labeled NOR

> flash somewhere - are start and size correct? The final one depends on the

> maximum RAM size - does RTD1195 allow more than 1 GiB RAM? All

> non-RAM regions should be covered here.

> 

It is reserved for NOR flash. The start and size is correct.
The rtd1195 can support to 2GiB RAM.

> So another question, applicable to all SoCs: This reserved Boot ROM area at

> the start of the address space, here of size 0xa800, is that copied into RAM, or

> is that the actual ROM overlapping RAM? If the latter, we should exclude it

> from /memory@0's reg (making it /memory@a800), and add it to soc's ranges

> here for correctness.

> 

Yes, we should exclude it from /memory@0's reg.

> With the follow-up question: Is it correct that, given the size 0xa800, I have a

> gap between /memreserve/s from 0xa800 to 0xc000, or should we reserve that

> gap by extending the next /memreserve/ or inserting one?


We should reserve memory address from 0x0000 to 0xa800 for the internal ROM.


Regards,
James
Andreas Färber Nov. 19, 2019, 11:15 a.m. UTC | #6
Hi James,

Am 18.11.19 um 07:53 schrieb James Tai:
>> So another question, applicable to all SoCs: This reserved Boot ROM area at

>> the start of the address space, here of size 0xa800, is that copied into RAM, or

>> is that the actual ROM overlapping RAM? If the latter, we should exclude it

>> from /memory@0's reg (making it /memory@a800), and add it to soc's ranges

>> here for correctness.

>>

> Yes, we should exclude it from /memory@0's reg.


OK, will look into it.

> 

>> With the follow-up question: Is it correct that, given the size 0xa800, I have a

>> gap between /memreserve/s from 0xa800 to 0xc000, or should we reserve that

>> gap by extending the next /memreserve/ or inserting one?

> 

> We should reserve memory address from 0x0000 to 0xa800 for the internal ROM.


Please see [1] - I had already updated the second reservation to start
at 0xa800 and extended it to 0x100000 before your response here.

The previous "bootcode" size of 0xc000 can be found here:
https://github.com/BPI-SINOVOIP/BPI-M4-bsp/blob/master/linux-rtk/arch/arm/mach-rtd119x/include/mach/memory.h
https://github.com/BPI-SINOVOIP/BPI-M4-bsp/blob/master/linux-rtk/arch/arm/boot/dts/realtek/rtd119x/rtd-119x-horseradish.dts

As you can see the 0xc000 and 0xf4000 were hardcoded and did not depend
on SYS_BOOTCODE_MEMSIZE...
For later SoCs I saw some FIXME(?) comment that area up to 0x100000 was
reserved due to some Jira ticket and should get fixed? Any insights on
what is in that memory range causing problems?

Regards,
Andreas

[1] https://patchwork.kernel.org/patch/11248033/

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)
James Tai Nov. 20, 2019, 9:20 a.m. UTC | #7
Hi Andreas,

> Please see [1] - I had already updated the second reservation to start at

> 0xa800 and extended it to 0x100000 before your response here.


Thank you.

> The previous "bootcode" size of 0xc000 can be found here:

> https://github.com/BPI-SINOVOIP/BPI-M4-bsp/blob/master/linux-rtk/arch/arm

> /mach-rtd119x/include/mach/memory.h

> https://github.com/BPI-SINOVOIP/BPI-M4-bsp/blob/master/linux-rtk/arch/arm

> /boot/dts/realtek/rtd119x/rtd-119x-horseradish.dts

> 

> As you can see the 0xc000 and 0xf4000 were hardcoded and did not depend on

> SYS_BOOTCODE_MEMSIZE...

> For later SoCs I saw some FIXME(?) comment that area up to 0x100000 was

> reserved due to some Jira ticket and should get fixed? Any insights on what is

> in that memory range causing problems?

> 

The problem is solved. (memory overwrite by FW)

Regards,
James
diff mbox series

Patch

diff --git a/arch/arm/boot/dts/rtd1195.dtsi b/arch/arm/boot/dts/rtd1195.dtsi
index a8cc2d23e7ef..3439647ecf97 100644
--- a/arch/arm/boot/dts/rtd1195.dtsi
+++ b/arch/arm/boot/dts/rtd1195.dtsi
@@ -92,28 +92,36 @@ 
 		         <0x18100000 0x18100000 0x01000000>,
 		         <0x40000000 0x40000000 0xc0000000>;
 
-		wdt: watchdog@18007680 {
-			compatible = "realtek,rtd1295-watchdog";
-			reg = <0x18007680 0x100>;
-			clocks = <&osc27M>;
-		};
-
-		uart0: serial@18007800 {
-			compatible = "snps,dw-apb-uart";
-			reg = <0x18007800 0x400>;
-			reg-shift = <2>;
-			reg-io-width = <4>;
-			clock-frequency = <27000000>;
-			status = "disabled";
-		};
-
-		uart1: serial@1801b200 {
-			compatible = "snps,dw-apb-uart";
-			reg = <0x1801b200 0x100>;
-			reg-shift = <2>;
-			reg-io-width = <4>;
-			clock-frequency = <27000000>;
-			status = "disabled";
+		rbus: r-bus@18000000 {
+			compatible = "simple-bus";
+			reg = <0x18000000 0x100000>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0x0 0x18000000 0x100000>;
+
+			wdt: watchdog@7680 {
+				compatible = "realtek,rtd1295-watchdog";
+				reg = <0x7680 0x100>;
+				clocks = <&osc27M>;
+			};
+
+			uart0: serial@7800 {
+				compatible = "snps,dw-apb-uart";
+				reg = <0x7800 0x400>;
+				reg-shift = <2>;
+				reg-io-width = <4>;
+				clock-frequency = <27000000>;
+				status = "disabled";
+			};
+
+			uart1: serial@1b200 {
+				compatible = "snps,dw-apb-uart";
+				reg = <0x1b200 0x100>;
+				reg-shift = <2>;
+				reg-io-width = <4>;
+				clock-frequency = <27000000>;
+				status = "disabled";
+			};
 		};
 
 		gic: interrupt-controller@ff011000 {