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
