On Thu, Aug 12, 2021 at 08:25:00PM +0200, Andreas Färber wrote:

Hi Chester et al.,

On 05.08.21 08:54, Chester Lin wrote:

quoted

Add memory nodes for S32G-VNP-EVB and S32G-VNP-RDB2 since they have fixed
RAM size.

You can drop "since they have fixed RAM size" - if they didn't, you
would simply choose the lowest size and rely on the bootloader (U-Boot)
to overwrite it with the actually detected size.

Please expand why this patch is separate - BSP based, I assume?

Yes, the information of memory banks is from s32 downstream kernel, which is
listed in the board DTs of older releases [bsp27] although newer releases
[bsp28 & bsp29] have moved it into s32 downstream u-boot as runtime fdt-fixup.
I think we should still reveal this information in kernel DTs in order to have
better understanding of system memory ranges that each board can have.

@NXP: Please don't hesitate to let me know if any better idea, thanks!

quoted

Signed-off-by: Chester Lin <redacted>
---
 arch/arm64/boot/dts/freescale/s32g274a-evb.dts  | 8 ++++++++
 arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/s32g274a-evb.dts b/arch/arm64/boot/dts/freescale/s32g274a-evb.dts
index a1ae5031730a..cd41f0af5dd8 100644
--- a/arch/arm64/boot/dts/freescale/s32g274a-evb.dts
+++ b/arch/arm64/boot/dts/freescale/s32g274a-evb.dts

@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later OR MIT
 /*
  * Copyright (c) 2021 SUSE LLC
+ * Copyright 2019-2020 NXP

@NXP: Please review year, alignment. Do any Signed-off-bys apply?

quoted

  */
 
 /dts-v1/;

@@ -14,6 +15,13 @@ / {
 	chosen {
 		stdout-path = "serial0:115200n8";
 	};
+
+	memory@80000000 {
+		device_type = "memory";
+		/* 4GB RAM */

This looks strange to me - either put /* 4 GiB RAM */ before the node,
three lines above, and/or append comment /* 2 GiB */ on each line below.
Note the space, and suggest to be precise about factor 1024 vs. 1000.

Thank you for the suggestion.

quoted

+		reg = <0 0x80000000 0 0x80000000>,

Note that this gives you the range to use for the .dtsi /soc node:
Address 0x0 with size 0x80000000 gets mapped to 0x0 0x0, excluding the
upper 0x80000000 for the RAM here. Or address 0x0 0x0 for two /soc cells
if there are high-memory peripherals.

I don't know if memory ranges might be changed for new boards or CPU revisions
in the future, which means the first memory range might be expanded as well
[e.g. 0~4GB]. Based this assumption, I think the size should also be changed
accordingly. Not sure if overlays can still work with this case but overwriting
all reg properties under /soc could be awful.

However if we only have to think of current hardware spec, it's good to declare
"range = <0 0 0 0x80000000>".

Please feel free to let me know if any suggestions, thanks.

quoted

+		      <8 0x80000000 0 0x80000000>;

Maybe use 0x8 here and 0x0 above? (second 0 stays same, so don't mind)

Will fix it.

quoted

+	};
 };
 
 &uart0 {

diff --git a/arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts b/arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts
index b2faae306b70..8fbbf3b45eb8 100644
--- a/arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts
+++ b/arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts

@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later OR MIT
 /*
  * Copyright (c) 2021 SUSE LLC
+ * Copyright 2019-2020 NXP

@NXP: 2021?

quoted

  */
 
 /dts-v1/;

@@ -14,6 +15,13 @@ / {
 	chosen {
 		stdout-path = "serial0:115200n8";
 	};
+
+	memory@80000000 {
+		device_type = "memory";
+		/* 4GB RAM */
+		reg = <0 0x80000000 0 0x80000000>,
+		      <8 0x80000000 0 0x80000000>;
+	};

Same comments as for EVB.

quoted

 };
 
 &uart0 {

Regards,
Andreas

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)