Re: [U-Boot] [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.Kumar@nxp.com Signed-off-by: Rajesh Bhagat rajesh.bhagat@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