Re: [PATCH v4 08/10] board: schneider: add RZN1 board support
On 4/17/23 21:45, Ralph Siemsen wrote:
On Mon, Apr 17, 2023 at 07:18:46PM +0200, Marek Vasut wrote:
On 3/8/23 21:26, Ralph Siemsen wrote:
diff --git a/board/schneider/rzn1-snarc/ddr_timing.c
b/board/schneider/rzn1-snarc/ddr_timing.c
new file mode 100644
index 00..8bc3fe7be4
--- /dev/null
+++ b/board/schneider/rzn1-snarc/ddr_timing.c
@@ -0,0 +1,140 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include
+
+#include "jedec_ddr3_2g_x16_1333h_500_cl8.h"
+
+u32 ddr_00_87_async[] = {
Should this be 'static u32...' ?
It should, but there is bit of kludge going on here. This table is used
when starting the DDR controller, in drivers/ram/cadence/ddr_async.c
There are several other boards in u-boot already which appear to use the
same (or a similar) DDR controller from Cadence. Some of these also use
a similar kludge as I have done. Others instead put the DDR parameters
into the device tree.
The DT alternative would be nice indeed.
While using the devicetree is cleaner, it does increase the size of the
DTB by quite a bit, which creates some additional challenges. Even more
so when you need to support multiple DDR chips, each with their own
configuration parameters.
Hmmm, stm32mp15xx does use the DT this way, indeed.
I don't suppose one can calculate those register values from some input
parameters (like, properties of the DRAM chip(s), bus, etc.) ?
It might be better to not expose the table itself, but provide some sort
of accessor function. In the worst case, add a comment to explain this
table is used elsewhere.
So I opted for the low-tech approach, at least in this initial version.
+#define DENALI_CTL_00_DATA 0x0600
You might want to run checkpatch --f --fix --fix-inplace on this to
fix formatting , esp. use one space after #define and tab after the
macro name. Note that diff -wdb will let you diff updates to this file
while ignoring space changes.
+#define DENALI_CTL_01_DATA 0x
+#define DENALI_CTL_02_DATA 0x
+#define DENALI_CTL_03_DATA 0x
+#define DENALI_CTL_04_DATA 0x
I had also considered eliminating the header file entirely, and just
putting the values directly into the array in the .c file. It is not
like the names of the #defines are particularly illuminating.
However, this runs into friction each time a new DDR appears, along with
the new set of parameters (autogenerated by some vendor tool). In fact
I've had to add some new DDR devices quite recently (but that didn't
make it into the current patches).
I'm happy to reformat this to match upstream preference. But I would
also be happy to discuss how we can better handle this type of "large
binary blob" of configuration data, to make it simpler for everyone.
NXP has the same problem on MX8M* , large tables of binary goo.
I think the best option would be to figure out how to calculate those
values based on input properties of the DRAM chips/bus/... , but while
that would be fantastic to have, that would be also a LOT of effort . If
you have the willpower to do it, woohoo ! Else, see above for low-tech
options.
[...]
Re: [PATCH v4 08/10] board: schneider: add RZN1 board support
On Mon, Apr 17, 2023 at 07:18:46PM +0200, Marek Vasut wrote:
On 3/8/23 21:26, Ralph Siemsen wrote:
diff --git a/board/schneider/rzn1-snarc/ddr_timing.c
b/board/schneider/rzn1-snarc/ddr_timing.c
new file mode 100644
index 00..8bc3fe7be4
--- /dev/null
+++ b/board/schneider/rzn1-snarc/ddr_timing.c
@@ -0,0 +1,140 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include
+
+#include "jedec_ddr3_2g_x16_1333h_500_cl8.h"
+
+u32 ddr_00_87_async[] = {
Should this be 'static u32...' ?
It should, but there is bit of kludge going on here. This table is used
when starting the DDR controller, in drivers/ram/cadence/ddr_async.c
There are several other boards in u-boot already which appear to use the
same (or a similar) DDR controller from Cadence. Some of these also use
a similar kludge as I have done. Others instead put the DDR parameters
into the device tree.
While using the devicetree is cleaner, it does increase the size of the
DTB by quite a bit, which creates some additional challenges. Even more
so when you need to support multiple DDR chips, each with their own
configuration parameters.
So I opted for the low-tech approach, at least in this initial version.
+#define DENALI_CTL_00_DATA 0x0600
You might want to run checkpatch --f --fix --fix-inplace on this to
fix formatting , esp. use one space after #define and tab after the
macro name. Note that diff -wdb will let you diff updates to this file
while ignoring space changes.
+#define DENALI_CTL_01_DATA 0x
+#define DENALI_CTL_02_DATA 0x
+#define DENALI_CTL_03_DATA 0x
+#define DENALI_CTL_04_DATA 0x
I had also considered eliminating the header file entirely, and just
putting the values directly into the array in the .c file. It is not
like the names of the #defines are particularly illuminating.
However, this runs into friction each time a new DDR appears, along with
the new set of parameters (autogenerated by some vendor tool). In fact
I've had to add some new DDR devices quite recently (but that didn't
make it into the current patches).
I'm happy to reformat this to match upstream preference. But I would
also be happy to discuss how we can better handle this type of "large
binary blob" of configuration data, to make it simpler for everyone.
+++ b/board/schneider/rzn1-snarc/rzn1.c
@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include
+#include
+#include
+
+DECLARE_GLOBAL_DATA_PTR;
+
+int board_init(void)
+{
+ gd->bd->bi_boot_params = CFG_SYS_SDRAM_BASE + 0x100;
+
+ return 0;
+}
+
+int dram_init(void)
+{
+ struct udevice *dev;
+ int err;
+
+ /* This will end up calling cadence_ddr_probe() */
+ err = uclass_get_device(UCLASS_RAM, 0, );
+ if (err) {
+ debug("DRAM init failed: %d\n", err);
+ return err;
+ }
+
+ if (fdtdec_setup_mem_size_base() != 0)
+ return -EINVAL;
+
+ return 0;
+}
+
+int dram_init_banksize(void)
+{
+ fdtdec_setup_memory_banksize();
+
+ return 0;
+}
I wonder how much of this should really be in arch/... since this is
common to all machines with RZN1 . Maybe move it there instead ?
I can certainly do that. When I first put this together, I looked at
many other dram_init() functions, both in arch/... and board/...
Ralph
Re: [PATCH v4 08/10] board: schneider: add RZN1 board support
On 3/8/23 21:26, Ralph Siemsen wrote:
[...]
diff --git a/board/schneider/rzn1-snarc/ddr_timing.c
b/board/schneider/rzn1-snarc/ddr_timing.c
new file mode 100644
index 00..8bc3fe7be4
--- /dev/null
+++ b/board/schneider/rzn1-snarc/ddr_timing.c
@@ -0,0 +1,140 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include
+
+#include "jedec_ddr3_2g_x16_1333h_500_cl8.h"
+
+u32 ddr_00_87_async[] = {
Should this be 'static u32...' ?
+ DENALI_CTL_00_DATA,
+ DENALI_CTL_01_DATA,
+ DENALI_CTL_02_DATA,
+ DENALI_CTL_03_DATA,
+ DENALI_CTL_04_DATA,
+ DENALI_CTL_05_DATA,
+ DENALI_CTL_06_DATA,
+ DENALI_CTL_07_DATA,
+ DENALI_CTL_08_DATA,
+ DENALI_CTL_09_DATA,
[...]
diff --git a/board/schneider/rzn1-snarc/jedec_ddr3_2g_x16_1333h_500_cl8.h
b/board/schneider/rzn1-snarc/jedec_ddr3_2g_x16_1333h_500_cl8.h
new file mode 100644
index 00..5c55518bc1
--- /dev/null
+++ b/board/schneider/rzn1-snarc/jedec_ddr3_2g_x16_1333h_500_cl8.h
@@ -0,0 +1,399 @@
+
+/*
+ *CADENCECopyright (c) 2001-2011 *
+ * Cadence Design Systems, Inc. *
+ * All rights reserved. *
+ **
+ **
+ * The values calculated from this script are meant to be*
+ * representative programmings. The values may not reflect the *
+ * actual required programming for production use. Please *
+ * closely review all programmed values for technical accuracy *
+ * before use in production parts. *
+ **
+ *
+ * Module: regconfig.h
+ * Documentation: Register programming header file
+ *
+ **
+ **
+ * WARNING: This file was automatically generated. Manual
+ * editing may result in undetermined behavior.
+ **
+ **/
+
+#define DENALI_CTL_00_DATA 0x0600
You might want to run checkpatch --f --fix --fix-inplace on this to fix
formatting , esp. use one space after #define and tab after the macro
name. Note that diff -wdb will let you diff updates to this file while
ignoring space changes.
+#define DENALI_CTL_01_DATA 0x
+#define DENALI_CTL_02_DATA 0x
+#define DENALI_CTL_03_DATA 0x
+#define DENALI_CTL_04_DATA 0x
[...]
+++ b/board/schneider/rzn1-snarc/rzn1.c
@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include
+#include
+#include
+
+DECLARE_GLOBAL_DATA_PTR;
+
+int board_init(void)
+{
+ gd->bd->bi_boot_params = CFG_SYS_SDRAM_BASE + 0x100;
+
+ return 0;
+}
+
+int dram_init(void)
+{
+ struct udevice *dev;
+ int err;
+
+ /* This will end up calling cadence_ddr_probe() */
+ err = uclass_get_device(UCLASS_RAM, 0, );
+ if (err) {
+ debug("DRAM init failed: %d\n", err);
+ return err;
+ }
+
+ if (fdtdec_setup_mem_size_base() != 0)
+ return -EINVAL;
+
+ return 0;
+}
+
+int dram_init_banksize(void)
+{
+ fdtdec_setup_memory_banksize();
+
+ return 0;
+}
I wonder how much of this should really be in arch/... since this is
common to all machines with RZN1 . Maybe move it there instead ?
[...]
[PATCH v4 08/10] board: schneider: add RZN1 board support
Add support for Schneider Electronics RZ/N1D and RZ/N1S boards, which
are based on the Reneasas RZ/N1 SoC devices.
The intention is to support both boards using a single defconfig, and to
handle the differences at runtime.
Signed-off-by: Ralph Siemsen
---
Changes in v4:
- add binman support via r9a06g032-rzn1-snarc-u-boot.dtsi
Changes in v3:
- rename board LCES to rzn1-snarc
- move CONFIG_SYS_NS16550_MEM32 to Kconfig
- define CFG_SYS_INIT_RAM_{ADDR,SIZE}
- removed debug uart settings from defconfig
arch/arm/dts/r9a06g032-rzn1-snarc-u-boot.dtsi | 23 +
arch/arm/dts/r9a06g032-rzn1-snarc.dts | 51 +++
arch/arm/mach-rzn1/Kconfig| 14 +
board/schneider/rzn1-snarc/Kconfig| 18 +
board/schneider/rzn1-snarc/Makefile | 3 +
board/schneider/rzn1-snarc/ddr_timing.c | 140 ++
.../jedec_ddr3_2g_x16_1333h_500_cl8.h | 399 ++
board/schneider/rzn1-snarc/rzn1.c | 39 ++
configs/rzn1_snarc_defconfig | 21 +
include/configs/rzn1-snarc.h | 17 +
10 files changed, 725 insertions(+)
create mode 100644 arch/arm/dts/r9a06g032-rzn1-snarc-u-boot.dtsi
create mode 100644 arch/arm/dts/r9a06g032-rzn1-snarc.dts
create mode 100644 board/schneider/rzn1-snarc/Kconfig
create mode 100644 board/schneider/rzn1-snarc/Makefile
create mode 100644 board/schneider/rzn1-snarc/ddr_timing.c
create mode 100644 board/schneider/rzn1-snarc/jedec_ddr3_2g_x16_1333h_500_cl8.h
create mode 100644 board/schneider/rzn1-snarc/rzn1.c
create mode 100644 configs/rzn1_snarc_defconfig
create mode 100644 include/configs/rzn1-snarc.h
diff --git a/arch/arm/dts/r9a06g032-rzn1-snarc-u-boot.dtsi
b/arch/arm/dts/r9a06g032-rzn1-snarc-u-boot.dtsi
new file mode 100644
index 00..794e711103
--- /dev/null
+++ b/arch/arm/dts/r9a06g032-rzn1-snarc-u-boot.dtsi
@@ -0,0 +1,23 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Configuration file for binman
+ *
+ * After building u-boot, can generate the SPKG output by running:
+ * tools/binman/binman build -d arch/arm/dts/r9a06g032-rzn1-snarc.dtb -O
+ */
+
+#include
+
+/ {
+ binman: binman {
+ };
+};
+
+ {
+ mkimage {
+ filename = "u-boot.bin.spkg";
+ args = "-n board/schneider/rzn1-snarc/spkgimage.cfg -T
spkgimage -a 0x2004 -e 0x2004";
+ u-boot {
+ };
+ };
+};
diff --git a/arch/arm/dts/r9a06g032-rzn1-snarc.dts
b/arch/arm/dts/r9a06g032-rzn1-snarc.dts
new file mode 100644
index 00..abb269cc21
--- /dev/null
+++ b/arch/arm/dts/r9a06g032-rzn1-snarc.dts
@@ -0,0 +1,51 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device Tree Source for Schneider RZ/N1 Board
+ *
+ * Based on r9a06g032-rzn1d400-db.dts
+ */
+
+/dts-v1/;
+
+#include "r9a06g032.dtsi"
+#include
+
+/ {
+ model = "Schneider RZ/N1 Board";
+ compatible = "schneider,rzn1", "renesas,r9a06g032";
+
+ chosen {
+ stdout-path = "serial0:115200n8";
+ };
+
+ aliases {
+ serial0 =
+ };
+
+ memory {
+ device_type = "memory";
+ reg = <0x8000 0x1000>;
+ };
+};
+
+ {
+ status = "okay";
+};
+
+ {
+ status = "okay";
+
+ pins_uart0: pins_uart0 {
+ pinmux = <
+ RZN1_PINMUX(103, RZN1_FUNC_UART0_I) /* UART0_TXD */
+ RZN1_PINMUX(104, RZN1_FUNC_UART0_I) /* UART0_RXD */
+ >;
+ bias-disable;
+ };
+};
+
+ {
+ pinctrl-0 = <_uart0>;
+ pinctrl-names = "default";
+ status = "okay";
+};
diff --git a/arch/arm/mach-rzn1/Kconfig b/arch/arm/mach-rzn1/Kconfig
index 707895874d..4b13afbb32 100644
--- a/arch/arm/mach-rzn1/Kconfig
+++ b/arch/arm/mach-rzn1/Kconfig
@@ -15,4 +15,18 @@ endchoice
config SYS_SOC
default "rzn1"
+choice
+ prompt "Board select"
+ default TARGET_SCHNEIDER_RZN1
+
+config TARGET_SCHNEIDER_RZN1
+ bool "Schneider RZN1 board"
+ help
+ Support the Schneider RZN1D and RZN1S boards, which are based
+ on the Renesas RZ/N1 SoC.
+
+endchoice
+
+source "board/schneider/rzn1-snarc/Kconfig"
+
endif
diff --git a/board/schneider/rzn1-snarc/Kconfig
b/board/schneider/rzn1-snarc/Kconfig
new file mode 100644
index 00..bb6d394077
--- /dev/null
+++ b/board/schneider/rzn1-snarc/Kconfig
@@ -0,0 +1,18 @@
+if TARGET_SCHNEIDER_RZN1
+
+config TEXT_BASE
+ default 0x2004 if ARCH_RZN1
+
+config SYS_MONITOR_LEN
+ default 524288 if ARCH_RZN1
+
+config SYS_BOARD
+ default "rzn1-snarc"
+
+config SYS_VENDOR
+ default "schneider"
+
+config SYS_CONFIG_NAME
+ default "rzn1-snarc"
+
+endif
diff --git a/board/schneider/rzn1-snarc/Makefile
b/board/schneider/rzn1-snarc/Makefile
new file mode 100644
index 00..95c151898b
--- /dev/null
+++ b/board/schneider/rzn1-snarc/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier:
