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

[Marek Vasut]

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.

[...]

> 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/ ?

[...]

> +/* DDR PHY setup */
> +static void ddr_phy_init(struct cadence_ddr_info *priv, int ddr_type)
> +{
> +	u32 val;
> +
> +	/* Disable DDR Controller clock and FlexWAY connection */
> +	clk_disable(&priv->hclk_ddrc);
> +	clk_disable(&priv->clk_ddrc);
> +
> +	clk_rzn1_reset_state(&priv->hclk_ddrc, 0);
> +	clk_rzn1_reset_state(&priv->clk_ddrc, 0);
> +
> +	/* Enable DDR Controller clock and FlexWAY connection */
> +	clk_enable(&priv->clk_ddrc);
> +	clk_enable(&priv->hclk_ddrc);
> +
> +	/* DDR PHY Soft reset assert */
> +	ddrc_writel(FUNCCTRL_MASKSDLOFS | FUNCCTRL_DVDDQ_1_5V, FUNCCTRL);
> +
> +	clk_rzn1_reset_state(&priv->hclk_ddrc, 1);
> +	clk_rzn1_reset_state(&priv->clk_ddrc, 1);
> +
> +	/* DDR PHY setup */
> +	phy_writel(DLLCTRL_MFSL_500MHz | DLLCTRL_MDLLSTBY, DLLCTRL);
> +	phy_writel(0x00000182, ZQCALCTRL);
> +	if (ddr_type == RZN1_DDR3_DUAL_BANK)
> +		phy_writel(0xAB330031, ZQODTCTRL);
> +	else if (ddr_type == RZN1_DDR3_SINGLE_BANK)
> +		phy_writel(0xAB320051, ZQODTCTRL);
> +	else /* DDR2 */
> +		phy_writel(0xAB330071, ZQODTCTRL);
> +	phy_writel(0xB545B544, RDCTRL);
> +	phy_writel(0x000000B0, RDTMG);
> +	phy_writel(0x020A0806, OUTCTRL);
> +	if (ddr_type == RZN1_DDR3_DUAL_BANK)
> +		phy_writel(0x80005556, WLCTRL1);
> +	else
> +		phy_writel(0x80005C5D, WLCTRL1);
> +	phy_writel(0x00000101, FIFOINIT);
> +	phy_writel(0x00004545, DQCALOFS1);
> +
> +	/* Step 9 MDLL reset release */
> +	val = phy_readl(DLLCTRL);
> +	val &= ~DLLCTRL_MDLLSTBY;
> +	phy_writel(val, DLLCTRL);
> +
> +	/* Step 12 Soft reset release */
> +	val = phy_readl(FUNCCTRL);
> +	val |= FUNCCTRL_RESET_N;
> +	phy_writel(val, FUNCCTRL);
> +
> +	/* Step 13 FIFO pointer initialize */
> +	phy_writel(FIFOINIT_RDPTINITEXE | FIFOINIT_WRPTINITEXE, FIFOINIT);
> +
> +	/* Step 14 Execute ZQ Calibration */
> +	val = phy_readl(ZQCALCTRL);
> +	val |= ZQCALCTRL_ZQCALRSTB;
> +	phy_writel(val, ZQCALCTRL);
> +
> +	/* 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.

> +	while (!(phy_readl(ZQCALCTRL) & ZQCALCTRL_ZQCALEND))
> +		;
> +
> +	/* Step 16 Enable Address and Command output */
> +	val = phy_readl(OUTCTRL);
> +	val |= OUTCTRL_ADCMDOE;
> +	phy_writel(val, OUTCTRL);
> +
> +	/* Step 17 Wait for 200us or more(from MRESETB=0) */
> +	udelay(200);
> +}

[...]

> diff --git a/board/schneider/rzn1-snarc/rzn1.c b/board/schneider/rzn1-snarc/rzn1.c
> new file mode 100644
> index 0000000000..6eb86c2592
> --- /dev/null
> +++ b/board/schneider/rzn1-snarc/rzn1.c
> @@ -0,0 +1,40 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <ram.h>
> +#include <asm/global_data.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +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 ?

> +	gd->bd->bi_boot_params = gd->ram_base + 0x100;
> +
> +	return 0;
> +}
> +
> +int dram_init(void)
> +{
> +	struct udevice *dev;
> +	int err;
> +
> +	/*
> +	 * This will end up calling cadence_ddr_probe(),
> +	 * and will also update gd->ram_size.
> +	 */
> +	err = uclass_get_device(UCLASS_RAM, 0, &dev);
> +	if (err) {
> +		debug("DRAM init failed: %d\n", err);

You can just do this here:

err = uclass...();
if (err)
   debug(...);

return err;

> +		return err;
> +	}
> +
> +	return 0;
> +}

[...]

narc.h b/include/configs/rzn1-snarc.h
> new file mode 100644
> index 0000000000..dce0de0d4c
> --- /dev/null
> +++ b/include/configs/rzn1-snarc.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Configuration settings for the Schneider RZ/N1 board
> + */
> +
> +#ifndef __RZN1_H

Better use __RZN1_SNARC_H , so that when other boards get added, there 
won't be trouble.

> +#define __RZN1_H
> +
> +/* Internal RAM */
> +#define CFG_SYS_INIT_RAM_ADDR	0x20000000
> +#define CFG_SYS_INIT_RAM_SIZE	(1 * 1024 * 1024)
> +
> +#endif	/* __RZN1_H */