Re: [PATCH v4 05/10] ram: cadence: add driver for Cadence EDAC

[Marek Vasut <marek.vasut@mailbox.org>]

On 3/8/23 21:26, Ralph Siemsen wrote:

[...]

> +#define FUNCCTRL	0x00
> +#define  FUNCCTRL_MASKSDLOFS	(0x18 << 16)
> +#define  FUNCCTRL_DVDDQ_1_5V	(1 << 8)
> +#define  FUNCCTRL_RESET_N	(1 << 0)
> +#define DLLCTRL		0x04
> +#define  DLLCTRL_ASDLLOCK	(1 << 26)
> +#define  DLLCTRL_MFSL_500MHz	(2 << 1)
> +#define  DLLCTRL_MDLLSTBY	(1 << 0)

Use BIT() macro where applicable.

> +#define ZQCALCTRL	0x08
> +#define  ZQCALCTRL_ZQCALEND	(1 << 30)
> +#define  ZQCALCTRL_ZQCALRSTB	(1 << 0)
> +#define ZQODTCTRL	0x0c
> +#define RDCTRL		0x10
> +#define RDTMG		0x14
> +#define FIFOINIT	0x18
> +#define  FIFOINIT_RDPTINITEXE	(1 << 8)
> +#define  FIFOINIT_WRPTINITEXE	(1 << 0)
> +#define OUTCTRL		0x1c
> +#define  OUTCTRL_ADCMDOE	(1 << 0)
> +#define WLCTRL1		0x40
> +#define  WLCTRL1_WLSTR		(1 << 24)
> +#define DQCALOFS1	0xe8
> +
> +/* DDR PHY setup */
> +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);

Is there any macro which defines those magic bits in magic numbers ?
If so, please use them.

> +	/* 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))
> +		;
> +	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);
> +}

[...]

> +int rzn1_dram_init(struct cadence_ddr_info *priv)
> +{
> +	u32 version;
> +	u32 ddr_start_addr = 0;
> +
> +	ddr_phy_init(priv, RZN1_DDR3_SINGLE_BANK);
> +
> +	/*
> +	 * Override DDR PHY Interface (DFI) related settings
> +	 * DFI is the internal interface between the DDR controller and the DDR PHY.
> +	 * These settings are specific to the board and can't be known by the settings
> +	 * provided for each DDR model within the generated include.
> +	 */
> +	ddr_350_374_async[351 - 350] = 0x001e0000;
> +	ddr_350_374_async[352 - 350] = 0x1e680000;
> +	ddr_350_374_async[353 - 350] = 0x02000020;
> +	ddr_350_374_async[354 - 350] = 0x02000200;
> +	ddr_350_374_async[355 - 350] = 0x00000c30;
> +	ddr_350_374_async[356 - 350] = 0x00009808;
> +	ddr_350_374_async[357 - 350] = 0x020a0706;
> +	ddr_350_374_async[372 - 350] = 0x01000000;
> +
> +	/*
> +	 * On ES1.0 devices, the DDR start address that the DDR Controller sees
> +	 * is the physical address of the DDR. However, later devices changed it
> +	 * to be 0 in order to fix an issue with DDR out-of-range detection.
> +	 */
> +#define RZN1_SYSCTRL_REG_VERSION 412
> +	regmap_read(priv->syscon, RZN1_SYSCTRL_REG_VERSION, &version);
> +	if (version == 0x10)
> +		ddr_start_addr = RZN1_V_DDR_BASE;
> +
> +	/* DDR Controller is always in ASYNC mode */
> +	cdns_ddr_ctrl_init((void *)RZN1_DDR_BASE, 1,
> +			   ddr_00_87_async, ddr_350_374_async,
> +			   ddr_start_addr, CFG_SYS_SDRAM_SIZE,
> +			   priv->enable_ecc, priv->enable_8bit);
> +
> +	rzn1_ddr3_single_bank((void *)RZN1_DDR_BASE);

Can you obtain the DRAM base from DT ?

> +	cdns_ddr_set_diff_cs_delays((void *)RZN1_DDR_BASE, 2, 7, 2, 2);
> +	cdns_ddr_set_same_cs_delays((void *)RZN1_DDR_BASE, 0, 7, 0, 0);
> +	cdns_ddr_set_odt_times((void *)RZN1_DDR_BASE, 5, 6, 6, 0, 4);
> +	cdns_ddr_ctrl_start((void *)RZN1_DDR_BASE);
> +
> +	ddr_phy_enable_wl(priv);
> +
> +	if (priv->enable_ecc) {
> +		/*
> +		 * Any read before a write will trigger an ECC un-correctable error,
> +		 * causing a data abort. However, this is also true for any read with a
> +		 * size less than the AXI bus width. So, the only sensible solution is
> +		 * to write to all of DDR now and take the hit...
> +		 */
> +		memset((void *)RZN1_V_DDR_BASE, 0xff, CFG_SYS_SDRAM_SIZE);
> +	}
> +
> +	return 0;
> +}
> +
> +static int cadence_ddr_get_info(struct udevice *udev, struct ram_info *info)
> +{
> +	info->base = 0;
> +	info->size = gd->ram_size;
> +
> +	return 0;
> +}
> +
> +static struct ram_ops cadence_ddr_ops = {
> +	.get_info = cadence_ddr_get_info,
> +};
> +
> +static int cadence_ddr_probe(struct udevice *dev)
> +{
> +	struct cadence_ddr_info *priv = dev_get_priv(dev);
> +	int ret;
> +
> +	priv->ddrc = dev_remap_addr_name(dev, "ddrc");
> +	if (!priv->ddrc) {
> +		dev_err(dev, "No reg property for Cadence DDR CTRL\n");
> +		return -EINVAL;
> +	}
> +
> +	priv->phy = dev_remap_addr_name(dev, "phy");
> +	if (!priv->phy) {
> +		dev_err(dev, "No reg property for Cadence DDR PHY\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = clk_get_by_name(dev, "clk_ddrc", &priv->clk_ddrc);
> +	if (ret) {
> +		dev_err(dev, "No clock for Cadence DDR\n");
> +		return ret;
> +	}
> +
> +	ret = clk_get_by_name(dev, "hclk_ddrc", &priv->hclk_ddrc);
> +	if (ret) {
> +		dev_err(dev, "No HCLK for Cadence DDR\n");
> +		return ret;
> +	}
> +
> +	priv->syscon = syscon_regmap_lookup_by_phandle(dev, "syscon");
> +	if (IS_ERR(priv->syscon)) {
> +		dev_err(dev, "No syscon node found\n");
> +		//return PTR_ERR(priv->syscon);

This shouldn't be commented out, right ?

> +	}
> +
> +	priv->enable_ecc = dev_read_bool(dev, "enable-ecc");
> +	priv->enable_8bit = dev_read_bool(dev, "enable-8bit");
> +
> +	rzn1_dram_init(priv);
> +
> +	return 0;
> +}
> +
> +static const struct udevice_id cadence_ddr_ids[] = {
> +	{ .compatible = "cadence,ddr-ctrl" },
> +	{ }
> +};
> +
> +U_BOOT_DRIVER(cadence_ddr) = {
> +	.name		= "cadence_ddr",
> +	.id		= UCLASS_RAM,
> +	.of_match	= cadence_ddr_ids,
> +	.ops		= &cadence_ddr_ops,
> +	.probe		= cadence_ddr_probe,
> +	.priv_auto	= sizeof(struct cadence_ddr_info),
> +	.flags		= DM_FLAG_PRE_RELOC,
> +};

[...]