Re: [U-Boot] [RESEND PATCH v3 1/7] armv8: lsch3: Add serdes and DDR voltage setup

[Rajesh Bhagat]

> -Original Message-
> From: York Sun
> Sent: Friday, September 15, 2017 2:33 AM
> To: Rajesh Bhagat ; u-boot@lists.denx.de
> Cc: Prabhakar Kushwaha ; Ashish Kumar
> 
> Subject: Re: [RESEND PATCH v3 1/7] armv8: lsch3: Add serdes and DDR voltage
> setup
> 
> You have a lot of magic numbers and delays. See inline comments.
> 
> On 09/03/2017 11:24 PM, Rajesh Bhagat wrote:
> > Adds SERDES voltage and reset SERDES lanes API and makes
> > enable/disable DDR controller support 0.9V API common.
> >
> > Signed-off-by: Ashish Kumar 
> > Signed-off-by: Rajesh Bhagat 
> > ---
> > Changes in v3:
> >   Restructured LS1088A VID support to use common VID driver
> >   Cosmetic review comments fixed
> >   Added __iomem for accessing registers
> >
> > Changes in v2:
> >   Checkpatch errors fixed
> >
> >   .../cpu/armv8/fsl-layerscape/fsl_lsch3_serdes.c| 274
> +
> >   arch/arm/cpu/armv8/fsl-layerscape/soc.c|  34 +--
> >   .../include/asm/arch-fsl-layerscape/fsl_serdes.h   |   2 +-
> >   .../include/asm/arch-fsl-layerscape/immap_lsch3.h  |  34 +++
> >   arch/arm/include/asm/arch-fsl-layerscape/soc.h |   1 +
> >   5 files changed, 327 insertions(+), 18 deletions(-)
> >
> > diff --git a/arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch3_serdes.c
> > b/arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch3_serdes.c
> > index 179cac6..39f2cdf 100644
> > --- a/arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch3_serdes.c
> > +++ b/arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch3_serdes.c
> > @@ -158,6 +158,280 @@ void serdes_init(u32 sd, u32 sd_addr, u32 rcwsr, u32
> sd_prctl_mask,
> > serdes_prtcl_map[NONE] = 1;
> >   }
> >
> > +__weak int get_serdes_volt(void)
> > +{
> > +   return -1;
> > +}
> > +
> > +__weak int set_serdes_volt(int svdd)
> > +{
> > +   return -1;
> > +}
> > +
> > +int setup_serdes_volt(u32 svdd)
> > +{
> > +   struct ccsr_gur __iomem *gur = (void *)(CONFIG_SYS_FSL_GUTS_ADDR);
> > +   struct ccsr_serdes __iomem *serdes1_base;
> > +   u32 cfg_rcwsrds1 = gur_in32(>rcwsr[FSL_CHASSIS3_SRDS1_REGSR -
> > +1]); #ifdef CONFIG_SYS_FSL_SRDS_2
> > +   struct ccsr_serdes __iomem *serdes2_base;
> > +   u32 cfg_rcwsrds2 = gur_in32(>rcwsr[FSL_CHASSIS3_SRDS2_REGSR -
> > +1]); #endif
> > +   u32 cfg_tmp, reg = 0;
> > +   int svdd_cur, svdd_tar;
> > +   int ret = 1;
> > +   int i;
> > +
> > +   /* Only support switch SVDD to 900mV */
> > +   if (svdd != 900)
> > +   return -1;
> > +
> > +   /* Scale up to the LTC resolution is 1/4096V */
> > +   svdd = (svdd * 4096) / 1000;
> > +
> > +   svdd_tar = svdd;
> > +   svdd_cur = get_serdes_volt();
> > +   if (svdd_cur < 0)
> > +   return -EINVAL;
> > +
> > +   debug("%s: current SVDD: %x; target SVDD: %x\n",
> > + __func__, svdd_cur, svdd_tar);
> > +   if (svdd_cur == svdd_tar)
> > +   return 0;
> > +
> > +   serdes1_base = (void *)CONFIG_SYS_FSL_LSCH3_SERDES_ADDR;
> > +#ifdef CONFIG_SYS_FSL_SRDS_2
> > +   serdes2_base =  (void *)(CONFIG_SYS_FSL_LSCH3_SERDES_ADDR +
> > +0x1); #endif
> > +
> > +   /* Put the all enabled lanes in reset */ #ifdef
> > +CONFIG_SYS_FSL_SRDS_1
> > +   cfg_tmp = cfg_rcwsrds1 & FSL_CHASSIS3_SRDS1_PRTCL_MASK;
> > +   cfg_tmp >>= FSL_CHASSIS3_SRDS1_PRTCL_SHIFT;
> > +
> > +   for (i = 0; i < 4 && cfg_tmp & (0xf << (3 - i)); i++) {
> > +   reg = in_le32(_base->lane[i].gcr0);
> > +   reg &= 0xFF9F;
> > +   out_le32(_base->lane[i].gcr0, reg);
> > +   }
> > +#endif
> 
> Please use local macros instead of magic numbers here and below.
> 

Will take care in v3 

> > +
> > +#ifdef CONFIG_SYS_FSL_SRDS_2
> > +   cfg_tmp = cfg_rcwsrds2 & FSL_CHASSIS3_SRDS2_PRTCL_MASK;
> > +   cfg_tmp >>= FSL_CHASSIS3_SRDS2_PRTCL_SHIFT;
> > +
> > +   for (i = 0; i < 4 && cfg_tmp & (0xf << (3 - i)); i++) {
> > +   reg = in_le32(_base->lane[i].gcr0);
> > +   reg &= 0xFF9F;
> > +   out_le32(_base->lane[i].gcr0, reg);
> > +   }
> > +#endif
> > +
> > +   /* Put the all enabled PLL in reset */ #ifdef CONFIG_SYS_FSL_SRDS_1
> > +   cfg_tmp = cfg_rcwsrds1 & 0x3;
> > +   for (i = 0; i < 2 && !(cfg_tmp & (0x1 << (1 - i))); i++) {
> > +   reg = in_le32(_base->bank[i].rstctl);
> > +   reg &= 0xFFBF;
> > +   reg |= 0x1000;
> > +   out_le32(_base->bank[i].rstctl, reg);
> > +   }
> > +   udelay(1);
> > +
> > +   reg = in_le32(_base->bank[i].rstctl);
> > +   reg &= 0xFF1F;
> > +   out_le32(_base->bank[i].rstctl, reg); #endif
> > +
> > +#ifdef CONFIG_SYS_FSL_SRDS_2
> > +   cfg_tmp = cfg_rcwsrds1 & 0xC;
> > +   cfg_tmp >>= 2;
> > +   for (i = 0; i < 2 && !(cfg_tmp & (0x1 << (1 - i))); i++) {
> > +   reg = in_le32(_base->bank[i].rstctl);
> > +   reg &= 0xFFBF;
> > +   reg |= 0x1000;
> > +   out_le32(_base->bank[i].rstctl, reg);
> > +   }
> > +   udelay(1);
> 
> What's this delay for?
> 

These 

Re: [U-Boot] [RESEND PATCH v3 1/7] armv8: lsch3: Add serdes and DDR voltage setup

[York Sun]

You have a lot of magic numbers and delays. See inline comments.

On 09/03/2017 11:24 PM, Rajesh Bhagat wrote:
> Adds SERDES voltage and reset SERDES lanes API and makes
> enable/disable DDR controller support 0.9V API common.
> 
> Signed-off-by: Ashish Kumar 
> Signed-off-by: Rajesh Bhagat 
> ---
> Changes in v3:
>   Restructured LS1088A VID support to use common VID driver
>   Cosmetic review comments fixed
>   Added __iomem for accessing registers
> 
> Changes in v2:
>   Checkpatch errors fixed
> 
>   .../cpu/armv8/fsl-layerscape/fsl_lsch3_serdes.c| 274 
> +
>   arch/arm/cpu/armv8/fsl-layerscape/soc.c|  34 +--
>   .../include/asm/arch-fsl-layerscape/fsl_serdes.h   |   2 +-
>   .../include/asm/arch-fsl-layerscape/immap_lsch3.h  |  34 +++
>   arch/arm/include/asm/arch-fsl-layerscape/soc.h |   1 +
>   5 files changed, 327 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch3_serdes.c 
> b/arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch3_serdes.c
> index 179cac6..39f2cdf 100644
> --- a/arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch3_serdes.c
> +++ b/arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch3_serdes.c
> @@ -158,6 +158,280 @@ void serdes_init(u32 sd, u32 sd_addr, u32 rcwsr, u32 
> sd_prctl_mask,
>   serdes_prtcl_map[NONE] = 1;
>   }
>   
> +__weak int get_serdes_volt(void)
> +{
> + return -1;
> +}
> +
> +__weak int set_serdes_volt(int svdd)
> +{
> + return -1;
> +}
> +
> +int setup_serdes_volt(u32 svdd)
> +{
> + struct ccsr_gur __iomem *gur = (void *)(CONFIG_SYS_FSL_GUTS_ADDR);
> + struct ccsr_serdes __iomem *serdes1_base;
> + u32 cfg_rcwsrds1 = gur_in32(>rcwsr[FSL_CHASSIS3_SRDS1_REGSR - 1]);
> +#ifdef CONFIG_SYS_FSL_SRDS_2
> + struct ccsr_serdes __iomem *serdes2_base;
> + u32 cfg_rcwsrds2 = gur_in32(>rcwsr[FSL_CHASSIS3_SRDS2_REGSR - 1]);
> +#endif
> + u32 cfg_tmp, reg = 0;
> + int svdd_cur, svdd_tar;
> + int ret = 1;
> + int i;
> +
> + /* Only support switch SVDD to 900mV */
> + if (svdd != 900)
> + return -1;
> +
> + /* Scale up to the LTC resolution is 1/4096V */
> + svdd = (svdd * 4096) / 1000;
> +
> + svdd_tar = svdd;
> + svdd_cur = get_serdes_volt();
> + if (svdd_cur < 0)
> + return -EINVAL;
> +
> + debug("%s: current SVDD: %x; target SVDD: %x\n",
> +   __func__, svdd_cur, svdd_tar);
> + if (svdd_cur == svdd_tar)
> + return 0;
> +
> + serdes1_base = (void *)CONFIG_SYS_FSL_LSCH3_SERDES_ADDR;
> +#ifdef CONFIG_SYS_FSL_SRDS_2
> + serdes2_base =  (void *)(CONFIG_SYS_FSL_LSCH3_SERDES_ADDR + 0x1);
> +#endif
> +
> + /* Put the all enabled lanes in reset */
> +#ifdef CONFIG_SYS_FSL_SRDS_1
> + cfg_tmp = cfg_rcwsrds1 & FSL_CHASSIS3_SRDS1_PRTCL_MASK;
> + cfg_tmp >>= FSL_CHASSIS3_SRDS1_PRTCL_SHIFT;
> +
> + for (i = 0; i < 4 && cfg_tmp & (0xf << (3 - i)); i++) {
> + reg = in_le32(_base->lane[i].gcr0);
> + reg &= 0xFF9F;
> + out_le32(_base->lane[i].gcr0, reg);
> + }
> +#endif

Please use local macros instead of magic numbers here and below.

> +
> +#ifdef CONFIG_SYS_FSL_SRDS_2
> + cfg_tmp = cfg_rcwsrds2 & FSL_CHASSIS3_SRDS2_PRTCL_MASK;
> + cfg_tmp >>= FSL_CHASSIS3_SRDS2_PRTCL_SHIFT;
> +
> + for (i = 0; i < 4 && cfg_tmp & (0xf << (3 - i)); i++) {
> + reg = in_le32(_base->lane[i].gcr0);
> + reg &= 0xFF9F;
> + out_le32(_base->lane[i].gcr0, reg);
> + }
> +#endif
> +
> + /* Put the all enabled PLL in reset */
> +#ifdef CONFIG_SYS_FSL_SRDS_1
> + cfg_tmp = cfg_rcwsrds1 & 0x3;
> + for (i = 0; i < 2 && !(cfg_tmp & (0x1 << (1 - i))); i++) {
> + reg = in_le32(_base->bank[i].rstctl);
> + reg &= 0xFFBF;
> + reg |= 0x1000;
> + out_le32(_base->bank[i].rstctl, reg);
> + }
> + udelay(1);
> +
> + reg = in_le32(_base->bank[i].rstctl);
> + reg &= 0xFF1F;
> + out_le32(_base->bank[i].rstctl, reg);
> +#endif
> +
> +#ifdef CONFIG_SYS_FSL_SRDS_2
> + cfg_tmp = cfg_rcwsrds1 & 0xC;
> + cfg_tmp >>= 2;
> + for (i = 0; i < 2 && !(cfg_tmp & (0x1 << (1 - i))); i++) {
> + reg = in_le32(_base->bank[i].rstctl);
> + reg &= 0xFFBF;
> + reg |= 0x1000;
> + out_le32(_base->bank[i].rstctl, reg);
> + }
> + udelay(1);

What's this delay for?

> +
> + reg = in_le32(_base->bank[i].rstctl);
> + reg &= 0xFF1F;
> + out_le32(_base->bank[i].rstctl, reg);
> +#endif
> +
> + /* Put the Rx/Tx calibration into reset */
> +#ifdef CONFIG_SYS_FSL_SRDS_1
> + reg = in_le32(_base->srdstcalcr);
> + reg &= 0xF7FF;
> + out_le32(_base->srdstcalcr, reg);
> + reg = in_le32(_base->srdsrcalcr);
> + reg &= 0xF7FF;
> + out_le32(_base->srdsrcalcr, reg);
> +#endif
> +
> +#ifdef CONFIG_SYS_FSL_SRDS_2
> + 

[U-Boot] [RESEND PATCH v3 1/7] armv8: lsch3: Add serdes and DDR voltage setup

[Rajesh Bhagat]

Adds SERDES voltage and reset SERDES lanes API and makes
enable/disable DDR controller support 0.9V API common.

Signed-off-by: Ashish Kumar 
Signed-off-by: Rajesh Bhagat 
---
Changes in v3:
 Restructured LS1088A VID support to use common VID driver
 Cosmetic review comments fixed
 Added __iomem for accessing registers

Changes in v2:
 Checkpatch errors fixed

 .../cpu/armv8/fsl-layerscape/fsl_lsch3_serdes.c| 274 +
 arch/arm/cpu/armv8/fsl-layerscape/soc.c|  34 +--
 .../include/asm/arch-fsl-layerscape/fsl_serdes.h   |   2 +-
 .../include/asm/arch-fsl-layerscape/immap_lsch3.h  |  34 +++
 arch/arm/include/asm/arch-fsl-layerscape/soc.h |   1 +
 5 files changed, 327 insertions(+), 18 deletions(-)

diff --git a/arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch3_serdes.c 
b/arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch3_serdes.c
index 179cac6..39f2cdf 100644
--- a/arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch3_serdes.c
+++ b/arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch3_serdes.c
@@ -158,6 +158,280 @@ void serdes_init(u32 sd, u32 sd_addr, u32 rcwsr, u32 
sd_prctl_mask,
serdes_prtcl_map[NONE] = 1;
 }
 
+__weak int get_serdes_volt(void)
+{
+   return -1;
+}
+
+__weak int set_serdes_volt(int svdd)
+{
+   return -1;
+}
+
+int setup_serdes_volt(u32 svdd)
+{
+   struct ccsr_gur __iomem *gur = (void *)(CONFIG_SYS_FSL_GUTS_ADDR);
+   struct ccsr_serdes __iomem *serdes1_base;
+   u32 cfg_rcwsrds1 = gur_in32(>rcwsr[FSL_CHASSIS3_SRDS1_REGSR - 1]);
+#ifdef CONFIG_SYS_FSL_SRDS_2
+   struct ccsr_serdes __iomem *serdes2_base;
+   u32 cfg_rcwsrds2 = gur_in32(>rcwsr[FSL_CHASSIS3_SRDS2_REGSR - 1]);
+#endif
+   u32 cfg_tmp, reg = 0;
+   int svdd_cur, svdd_tar;
+   int ret = 1;
+   int i;
+
+   /* Only support switch SVDD to 900mV */
+   if (svdd != 900)
+   return -1;
+
+   /* Scale up to the LTC resolution is 1/4096V */
+   svdd = (svdd * 4096) / 1000;
+
+   svdd_tar = svdd;
+   svdd_cur = get_serdes_volt();
+   if (svdd_cur < 0)
+   return -EINVAL;
+
+   debug("%s: current SVDD: %x; target SVDD: %x\n",
+ __func__, svdd_cur, svdd_tar);
+   if (svdd_cur == svdd_tar)
+   return 0;
+
+   serdes1_base = (void *)CONFIG_SYS_FSL_LSCH3_SERDES_ADDR;
+#ifdef CONFIG_SYS_FSL_SRDS_2
+   serdes2_base =  (void *)(CONFIG_SYS_FSL_LSCH3_SERDES_ADDR + 0x1);
+#endif
+
+   /* Put the all enabled lanes in reset */
+#ifdef CONFIG_SYS_FSL_SRDS_1
+   cfg_tmp = cfg_rcwsrds1 & FSL_CHASSIS3_SRDS1_PRTCL_MASK;
+   cfg_tmp >>= FSL_CHASSIS3_SRDS1_PRTCL_SHIFT;
+
+   for (i = 0; i < 4 && cfg_tmp & (0xf << (3 - i)); i++) {
+   reg = in_le32(_base->lane[i].gcr0);
+   reg &= 0xFF9F;
+   out_le32(_base->lane[i].gcr0, reg);
+   }
+#endif
+
+#ifdef CONFIG_SYS_FSL_SRDS_2
+   cfg_tmp = cfg_rcwsrds2 & FSL_CHASSIS3_SRDS2_PRTCL_MASK;
+   cfg_tmp >>= FSL_CHASSIS3_SRDS2_PRTCL_SHIFT;
+
+   for (i = 0; i < 4 && cfg_tmp & (0xf << (3 - i)); i++) {
+   reg = in_le32(_base->lane[i].gcr0);
+   reg &= 0xFF9F;
+   out_le32(_base->lane[i].gcr0, reg);
+   }
+#endif
+
+   /* Put the all enabled PLL in reset */
+#ifdef CONFIG_SYS_FSL_SRDS_1
+   cfg_tmp = cfg_rcwsrds1 & 0x3;
+   for (i = 0; i < 2 && !(cfg_tmp & (0x1 << (1 - i))); i++) {
+   reg = in_le32(_base->bank[i].rstctl);
+   reg &= 0xFFBF;
+   reg |= 0x1000;
+   out_le32(_base->bank[i].rstctl, reg);
+   }
+   udelay(1);
+
+   reg = in_le32(_base->bank[i].rstctl);
+   reg &= 0xFF1F;
+   out_le32(_base->bank[i].rstctl, reg);
+#endif
+
+#ifdef CONFIG_SYS_FSL_SRDS_2
+   cfg_tmp = cfg_rcwsrds1 & 0xC;
+   cfg_tmp >>= 2;
+   for (i = 0; i < 2 && !(cfg_tmp & (0x1 << (1 - i))); i++) {
+   reg = in_le32(_base->bank[i].rstctl);
+   reg &= 0xFFBF;
+   reg |= 0x1000;
+   out_le32(_base->bank[i].rstctl, reg);
+   }
+   udelay(1);
+
+   reg = in_le32(_base->bank[i].rstctl);
+   reg &= 0xFF1F;
+   out_le32(_base->bank[i].rstctl, reg);
+#endif
+
+   /* Put the Rx/Tx calibration into reset */
+#ifdef CONFIG_SYS_FSL_SRDS_1
+   reg = in_le32(_base->srdstcalcr);
+   reg &= 0xF7FF;
+   out_le32(_base->srdstcalcr, reg);
+   reg = in_le32(_base->srdsrcalcr);
+   reg &= 0xF7FF;
+   out_le32(_base->srdsrcalcr, reg);
+#endif
+
+#ifdef CONFIG_SYS_FSL_SRDS_2
+   reg = in_le32(_base->srdstcalcr);
+   reg &= 0xF7FF;
+   out_le32(_base->srdstcalcr, reg);
+   reg = in_le32(_base->srdsrcalcr);
+   reg &= 0xF7FF;
+   out_le32(_base->srdsrcalcr, reg);
+#endif
+
+   ret = set_serdes_volt(svdd);
+   if (ret < 0) {
+   printf("could not change SVDD\n");
+   ret =