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

[Ralph Siemsen <ralph.siemsen@linaro.org>]

On Mon, Apr 17, 2023 at 07:32:30PM +0200, Marek Vasut wrote:
>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.

Will do.

>>+	/* 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.

This init sequence came from the u-boot 2017 repo published by Renesas. 
There do not appear to be any macros to help with all these magic 
numbers.

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

I'll check if it is possible.

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

Does indeed look like an oversight. I'll fix it up, thanks for spotting!

Ralph