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 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.
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 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.
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.
[...]
