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

[Ralph Siemsen]

On Sun, May 07, 2023 at 06:06:40PM +0200, Marek Vasut wrote:
>On 4/24/23 03:15, Ralph Siemsen wrote:
>>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.
>
>The DT comes from Linux kernel, right ? Please include commit ID from 
>which the DT is imported in the commit message, I suspect that would 
>be Linux 6.3 commit ID.

Yes, the DT is copied verbatim from Linux. I have updated the commit 
message to mention 6.3 and include the hash. (There have been no changes 

>
>>diff --git a/board/schneider/rzn1-snarc/ddr_async.c b/board/schneider/rzn1-snarc/ddr_async.c
>>new file mode 100644
>>index 0000000000..4b4c280e45
>>--- /dev/null
>>+++ b/board/schneider/rzn1-snarc/ddr_async.c
>
>Please correct me if I'm wrong, but shouldn't this be in drivers/ram/ ?

I had it there originally, but moved it in v5 of the patch series. There 
is a lot of board-specific (or at least architecture-specific) logic in 
this file. For example the sequence of steps to setup the clocks and 
interconnect, prior to bringing the DDR controller out of reset. This 
depends on choices made by the RZ/N1 designers, rather than the CDNS IP 
that drivers/ram/cadence is aiming to cover.

Likewise for some board-specific PHY settings, and choices such as 
operating in async mode.

I moved it to board-specific directory as an interim step. Hopefully we 
can do some consolidation of the multiple CDNS DDR controller 
implementations, and then figure out the right way to split things up.

For now this seemed like the less-bad option.

>>+
>>+	/* Step 15 Wait for 200us or more, or wait for DFIINITCOMPLETE to be "1" */
>>+	while (!(phy_readl(DLLCTRL) & DLLCTRL_ASDLLOCK))
>>+		;
>
>Please avoid endless loops, use readl_poll_timeout() or 
>wait_for_bit*() where possible.

Will do.

>>+int board_init(void)
>>+{
>>+	/*
>>+	 * Initial values for gd->ram_base and gd->ram_size
>>+	 * are obtained from the "/memory" node in devicetree.
>>+	 * The size will be updated in later when probing DDR.
>>+	 */
>>+	fdtdec_setup_mem_size_base();
>
>Are you absolutely sure this call ^ is needed at all ?

I was sure at the time when I wrote it, however I just tried removing it 
now and it seems to work fine.

>You can just do this here:
>
>err = uclass...();
>if (err)
>  debug(...);
>
>return err;

Done.

>>+#ifndef __RZN1_H
>
>Better use __RZN1_SNARC_H , so that when other boards get added, there 
>won't be trouble.

Good catch, done.

Ralph