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
