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

[Marek Vasut]

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.

[...]