Re: [PATCH v4 08/10] board: schneider: add RZN1 board support

[Ralph Siemsen <ralph.siemsen@linaro.org>]

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 0000000000..8bc3fe7be4
>>--- /dev/null
>>+++ b/board/schneider/rzn1-snarc/ddr_timing.c
>>@@ -0,0 +1,140 @@
>>+// SPDX-License-Identifier: GPL-2.0
>>+
>>+#include <asm/types.h>
>>+
>>+#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 0x00000600
>
>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 0x00000000
>>+#define               DENALI_CTL_02_DATA 0x00000000
>>+#define               DENALI_CTL_03_DATA 0x00000000
>>+#define               DENALI_CTL_04_DATA 0x00000000

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 <common.h>
>>+#include <dm.h>
>>+#include <asm/global_data.h>
>>+
>>+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, &dev);
>>+	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