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

2023-04-17 Thread Ralph Siemsen

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 DLLCTRL0x04
+#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(0x0182, 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(0x00B0, RDTMG);
+   phy_writel(0x020A0806, OUTCTRL);
+   if (ddr_type == RZN1_DDR3_DUAL_BANK)
+   phy_writel(0x80005556, WLCTRL1);
+   else
+   phy_writel(0x80005C5D, WLCTRL1);
+   phy_writel(0x0101, FIFOINIT);
+   phy_writel(0x4545, 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


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

2023-04-17 Thread Marek Vasut

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 DLLCTRL0x04
+#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 OUTCTRL0x1c
+#define  OUTCTRL_ADCMDOE   (1 << 0)
+#define WLCTRL10x40
+#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(>hclk_ddrc);
+   clk_disable(>clk_ddrc);
+
+   clk_rzn1_reset_state(>hclk_ddrc, 0);
+   clk_rzn1_reset_state(>clk_ddrc, 0);
+
+   /* Enable DDR Controller clock and FlexWAY connection */
+   clk_enable(>clk_ddrc);
+   clk_enable(>hclk_ddrc);
+
+   /* DDR PHY Soft reset assert */
+   ddrc_writel(FUNCCTRL_MASKSDLOFS | FUNCCTRL_DVDDQ_1_5V, FUNCCTRL);
+
+   clk_rzn1_reset_state(>hclk_ddrc, 1);
+   clk_rzn1_reset_state(>clk_ddrc, 1);
+
+   /* DDR PHY setup */
+   phy_writel(DLLCTRL_MFSL_500MHz | DLLCTRL_MDLLSTBY, DLLCTRL);
+   phy_writel(0x0182, 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(0x00B0, RDTMG);
+   phy_writel(0x020A0806, OUTCTRL);
+   if (ddr_type == RZN1_DDR3_DUAL_BANK)
+   phy_writel(0x80005556, WLCTRL1);
+   else
+   phy_writel(0x80005C5D, WLCTRL1);
+   phy_writel(0x0101, FIFOINIT);
+   phy_writel(0x4545, 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] = 0x001e;
+   ddr_350_374_async[352 - 350] = 0x1e68;
+   ddr_350_374_async[353 - 350] = 0x0220;
+   ddr_350_374_async[354 - 350] = 0x02000200;
+   ddr_350_374_async[355 - 350] = 0x0c30;
+   ddr_350_374_async[356 - 350] = 0x9808;
+   ddr_350_374_async[357 - 350] = 0x020a0706;
+   ddr_350_374_async[372 - 350] = 0x0100;
+
+   /*
+* 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, );
+   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,