| Message ID | 20210805065429.27485-8-clin@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: dts: initial NXP S32G2 support | expand |
Hi Chester et al., On 05.08.21 08:54, Chester Lin wrote: > 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? > > Signed-off-by: Chester Lin <clin@suse.com> > --- > 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? > */ > > /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. > + 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. > + <8 0x80000000 0 0x80000000>; Maybe use 0x8 here and 0x0 above? (second 0 stays same, so don't mind) > + }; > }; > > &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? > */ > > /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. > }; > > &uart0 { Regards, Andreas
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: > > 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! > > > > Signed-off-by: Chester Lin <clin@suse.com> > > --- > > 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? > > > */ > > > > /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. > > + 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. > > + <8 0x80000000 0 0x80000000>; > > Maybe use 0x8 here and 0x0 above? (second 0 stays same, so don't mind) > Will fix it. > > + }; > > }; > > > > &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? > > > */ > > > > /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. > > > }; > > > > &uart0 { > > Regards, > Andreas > > -- > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Felix Imendörffer > HRB 36809 (AG Nürnberg) >
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 */ /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>; + }; }; &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 */ /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>; + }; }; &uart0 {
Add memory nodes for S32G-VNP-EVB and S32G-VNP-RDB2 since they have fixed RAM size. Signed-off-by: Chester Lin <clin@suse.com> --- arch/arm64/boot/dts/freescale/s32g274a-evb.dts | 8 ++++++++ arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts | 8 ++++++++ 2 files changed, 16 insertions(+)