> -----Original Message-----
> From: York Sun
> Sent: Friday, September 15, 2017 2:33 AM
> To: Rajesh Bhagat <rajesh.bha...@nxp.com>; u-boot@lists.denx.de
> Cc: Prabhakar Kushwaha <prabhakar.kushw...@nxp.com>; Ashish Kumar
> <ashish.ku...@nxp.com>
> 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 <ashish.ku...@nxp.com>
> > Signed-off-by: Rajesh Bhagat <rajesh.bha...@nxp.com>
> > ---
> > 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(&gur->rcwsr[FSL_CHASSIS3_SRDS1_REGSR -
> > +1]); #ifdef CONFIG_SYS_FSL_SRDS_2
> > + struct ccsr_serdes __iomem *serdes2_base;
> > + u32 cfg_rcwsrds2 = gur_in32(&gur->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 +
> > +0x10000); #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(&serdes1_base->lane[i].gcr0);
> > + reg &= 0xFF9FFFFF;
> > + out_le32(&serdes1_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(&serdes2_base->lane[i].gcr0);
> > + reg &= 0xFF9FFFFF;
> > + out_le32(&serdes2_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(&serdes1_base->bank[i].rstctl);
> > + reg &= 0xFFFFFFBF;
> > + reg |= 0x10000000;
> > + out_le32(&serdes1_base->bank[i].rstctl, reg);
> > + }
> > + udelay(1);
> > +
> > + reg = in_le32(&serdes1_base->bank[i].rstctl);
> > + reg &= 0xFFFFFF1F;
> > + out_le32(&serdes1_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(&serdes2_base->bank[i].rstctl);
> > + reg &= 0xFFFFFFBF;
> > + reg |= 0x10000000;
> > + out_le32(&serdes2_base->bank[i].rstctl, reg);
> > + }
> > + udelay(1);
>
> What's this delay for?
>
These delays are part of SERDES reset requirements, each step of
SERDES reset requires some delay to be added.
> > +
> > + reg = in_le32(&serdes2_base->bank[i].rstctl);
> > + reg &= 0xFFFFFF1F;
> > + out_le32(&serdes2_base->bank[i].rstctl, reg); #endif
> > +
> > + /* Put the Rx/Tx calibration into reset */ #ifdef
> > +CONFIG_SYS_FSL_SRDS_1
> > + reg = in_le32(&serdes1_base->srdstcalcr);
> > + reg &= 0xF7FFFFFF;
> > + out_le32(&serdes1_base->srdstcalcr, reg);
> > + reg = in_le32(&serdes1_base->srdsrcalcr);
> > + reg &= 0xF7FFFFFF;
> > + out_le32(&serdes1_base->srdsrcalcr, reg); #endif
> > +
> > +#ifdef CONFIG_SYS_FSL_SRDS_2
> > + reg = in_le32(&serdes2_base->srdstcalcr);
> > + reg &= 0xF7FFFFFF;
> > + out_le32(&serdes2_base->srdstcalcr, reg);
> > + reg = in_le32(&serdes2_base->srdsrcalcr);
> > + reg &= 0xF7FFFFFF;
> > + out_le32(&serdes2_base->srdsrcalcr, reg); #endif
> > +
> > + ret = set_serdes_volt(svdd);
> > + if (ret < 0) {
> > + printf("could not change SVDD\n");
> > + ret = -1;
> > + }
> > +
> > + /* For each PLL that's not disabled via RCW enable the SERDES */
> > +#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(&serdes1_base->bank[i].rstctl);
> > + reg |= 0x00000020;
> > + out_le32(&serdes1_base->bank[i].rstctl, reg);
> > + udelay(1);
>
> Why delay here?
>
> > +
> > + reg = in_le32(&serdes1_base->bank[i].rstctl);
> > + reg |= 0x00000080;
> > + out_le32(&serdes1_base->bank[i].rstctl, reg);
> > + udelay(1);
>
> Here.
>
> > + /* Take the Rx/Tx calibration out of reset */
> > + if (!(cfg_tmp == 0x3 && i == 1)) {
> > + udelay(1);
> > + reg = in_le32(&serdes1_base->srdstcalcr);
> > + reg |= 0x08000000;
> > + out_le32(&serdes1_base->srdstcalcr, reg);
> > + reg = in_le32(&serdes1_base->srdsrcalcr);
> > + reg |= 0x08000000;
> > + out_le32(&serdes1_base->srdsrcalcr, reg);
> > + }
> > + udelay(1);
>
> Why delay here?
>
> > + }
> > +#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(&serdes2_base->bank[i].rstctl);
> > + reg |= 0x00000020;
> > + out_le32(&serdes2_base->bank[i].rstctl, reg);
> > + udelay(1);
> > +
> > + reg = in_le32(&serdes2_base->bank[i].rstctl);
> > + reg |= 0x00000080;
> > + out_le32(&serdes2_base->bank[i].rstctl, reg);
> > + udelay(1);
> > +
> > + /* Take the Rx/Tx calibration out of reset */
> > + if (!(cfg_tmp == 0x3 && i == 1)) {
> > + udelay(1);
> > + reg = in_le32(&serdes2_base->srdstcalcr);
> > + reg |= 0x08000000;
> > + out_le32(&serdes2_base->srdstcalcr, reg);
> > + reg = in_le32(&serdes2_base->srdsrcalcr);
> > + reg |= 0x08000000;
> > + out_le32(&serdes2_base->srdsrcalcr, reg);
> > + }
> > + udelay(1);
> > + }
> > +#endif
>
> You have many duplicated code. Use functions or macros.
>
Will take care in v3
> > +
> > + /* Wait for at atleast 625us, ensure the PLLs being reset are locked */
> > + udelay(800);
>
> You said 625us, but using 800.
>
It says atleast 625us, and delay is added for 800us. Again the SERDES
requirement
states this.
> > +
> > +#ifdef CONFIG_SYS_FSL_SRDS_1
> > + cfg_tmp = cfg_rcwsrds1 & 0x3;
> > + for (i = 0; i < 2 && !(cfg_tmp & (0x1 << (1 - i))); i++) {
> > + /* if the PLL is not locked, set RST_ERR */
> > + reg = in_le32(&serdes1_base->bank[i].pllcr0);
> > + if (!((reg >> 23) & 0x1)) {
> > + reg = in_le32(&serdes1_base->bank[i].rstctl);
> > + reg |= 0x20000000;
> > + out_le32(&serdes1_base->bank[i].rstctl, reg);
> > + } else {
> > + udelay(1);
> > + reg = in_le32(&serdes1_base->bank[i].rstctl);
> > + reg &= 0xFFFFFFEF;
> > + reg |= 0x00000040;
> > + out_le32(&serdes1_base->bank[i].rstctl, reg);
> > + udelay(1);
> > + }
> > + }
> > +#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(&serdes2_base->bank[i].pllcr0);
> > + if (!((reg >> 23) & 0x1)) {
> > + reg = in_le32(&serdes2_base->bank[i].rstctl);
> > + reg |= 0x20000000;
> > + out_le32(&serdes2_base->bank[i].rstctl, reg);
> > + } else {
> > + udelay(1);
> > + reg = in_le32(&serdes2_base->bank[i].rstctl);
> > + reg &= 0xFFFFFFEF;
> > + reg |= 0x00000040;
> > + out_le32(&serdes2_base->bank[i].rstctl, reg);
> > + udelay(1);
> > + }
> > + }
> > +#endif
> > + /* Take the all enabled lanes out of 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(&serdes1_base->lane[i].gcr0);
> > + reg |= 0x00600000;
> > + out_le32(&serdes1_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(&serdes2_base->lane[i].gcr0);
> > + reg |= 0x00600000;
> > + out_le32(&serdes2_base->lane[i].gcr0, reg);
> > + }
> > +#endif
> > +
> > + /* For each PLL being reset, and achieved PLL lock set RST_DONE */
> > +#ifdef CONFIG_SYS_FSL_SRDS_1
> > + cfg_tmp = cfg_rcwsrds1 & 0x3;
> > + for (i = 0; i < 2; i++) {
> > + reg = in_le32(&serdes1_base->bank[i].pllcr0);
> > + if (!(cfg_tmp & (0x1 << (1 - i))) && ((reg >> 23) & 0x1)) {
> > + reg = in_le32(&serdes1_base->bank[i].rstctl);
> > + reg |= 0x40000000;
> > + out_le32(&serdes1_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; i++) {
> > + reg = in_le32(&serdes2_base->bank[i].pllcr0);
> > + if (!(cfg_tmp & (0x1 << (1 - i))) && ((reg >> 23) & 0x1)) {
> > + reg = in_le32(&serdes2_base->bank[i].rstctl);
> > + reg |= 0x40000000;
> > + out_le32(&serdes2_base->bank[i].rstctl, reg);
> > + }
> > + }
> > +#endif
> > +
> > + return ret;
> > +}
> > +
> > void fsl_serdes_init(void)
> > {
> > #if defined(CONFIG_FSL_MC_ENET) && !defined(CONFIG_SPL_BUILD) diff
> > --git a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> > b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> > index 83e2871..4e96c7b 100644
> > --- a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> > +++ b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> > @@ -404,23 +404,6 @@ static int setup_core_volt(u32 vdd)
> > return board_setup_core_volt(vdd);
> > }
> >
> > -#ifdef CONFIG_SYS_FSL_DDR
> > -static void ddr_enable_0v9_volt(bool en) -{
> > - struct ccsr_ddr __iomem *ddr = (void *)CONFIG_SYS_FSL_DDR_ADDR;
> > - u32 tmp;
> > -
> > - tmp = ddr_in32(&ddr->ddr_cdr1);
> > -
> > - if (en)
> > - tmp |= DDR_CDR1_V0PT9_EN;
> > - else
> > - tmp &= ~DDR_CDR1_V0PT9_EN;
> > -
> > - ddr_out32(&ddr->ddr_cdr1, tmp);
> > -}
> > -#endif
> > -
> > int setup_chip_volt(void)
> > {
> > int vdd;
> > @@ -485,6 +468,23 @@ void fsl_lsch2_early_init_f(void)
> > }
> > #endif
> >
> > +#ifdef CONFIG_SYS_FSL_DDR
> > +void ddr_enable_0v9_volt(bool en)
> > +{
> > + struct ccsr_ddr __iomem *ddr = (void *)CONFIG_SYS_FSL_DDR_ADDR;
> > + u32 tmp;
> > +
> > + tmp = ddr_in32(&ddr->ddr_cdr1);
> > +
> > + if (en)
> > + tmp |= DDR_CDR1_V0PT9_EN;
> > + else
> > + tmp &= ~DDR_CDR1_V0PT9_EN;
> > +
> > + ddr_out32(&ddr->ddr_cdr1, tmp);
> > +}
> > +#endif
> > +
>
>
> I lost track. When do you use 0.9v for DDR?
>
When core voltage is set to 0.9v, it requires setting up the DDR controller
with 0.9 v setting. Prior to this patch, this code was used only in chasis2.
Making it non-static to use in chasis3 architecture also.
> York
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
