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.

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

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

> +
> +     /* Wait for at atleast 625us, ensure the PLLs being reset are locked */
> +     udelay(800);

You said 625us, but using 800.

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

York
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Reply via email to