[U-Boot] [PATCH v5 1/7] armv8: lsch3: Add serdes and DDR voltage setup
Rajesh Bhagat
rajesh.bhagat at nxp.com
Tue Nov 14 03:48:24 UTC 2017
> -----Original Message-----
> From: York Sun
> Sent: Monday, November 13, 2017 10:33 PM
> To: Rajesh Bhagat <rajesh.bhagat at nxp.com>; u-boot at lists.denx.de
> Cc: Prabhakar Kushwaha <prabhakar.kushwaha at nxp.com>; Priyanka Jain
> <priyanka.jain at nxp.com>; Ashish Kumar <ashish.kumar at nxp.com>
> Subject: Re: [PATCH v5 1/7] armv8: lsch3: Add serdes and DDR voltage setup
>
> On 11/12/2017 09:30 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 at nxp.com>
> > Signed-off-by: Rajesh Bhagat <rajesh.bhagat at nxp.com>
> > ---
> > Changes in v5:
> > - Moved local macros to static functions
> > - Used array to handle PRCTL mask and shift operations
> >
> > Changes in v4:
> > - Added local macros instead of magical numbers
> > - Created macros to remove duplicate code
> >
> > 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 | 306
> +++++++++++++++++++++
> > 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, 359 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..6a87350 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,312 @@ 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;
> > +}
> > +
> > +#define LNAGCR0_RESET_MASK 0xFF9FFFFF
> > +#define LNAGCR0_RT_RSTB 0x00600000
> > +#define RSTCTL_RESET_MASK_1 0xFFFFFFBF
> > +#define RSTCTL_RESET_MASK_2 0xFFFFFF1F
> > +#define RSTCTL_RESET_MASK_3 0xFFFFFFEF
> > +#define RSTCTL_RSTREQ 0x80000000
> > +#define RSTCTL_RSTERR 0x20000000
> > +#define RSTCTL_SDEN 0x00000020
> > +#define RSTCTL_SDRST_B 0x00000040
> > +#define RSTCTL_PLLRST_B 0x00000080
> > +#define RSTCTL_RST_DONE 0x40000000
> > +#define TCALCR_RESET_MASK 0xF7FFFFFF
> > +#define TCALCR_CALRST_B 0x08000000
> > +
> > +struct serdes_prctl_info {
> > + u32 id;
> > + u32 mask;
> > + u32 shift;
> > +};
> > +
> > +struct serdes_prctl_info srds_prctl_info[] = { #ifdef
> > +CONFIG_SYS_FSL_SRDS_1
> > + {.id = 1, .mask = FSL_CHASSIS3_SRDS1_PRTCL_MASK,
> > + .shift = FSL_CHASSIS3_SRDS1_PRTCL_SHIFT},
> > +
>
> This is much better. Please align each line. Even checkpatch doesn't give out warning,
> it is still a good practice to keep every assignment in individual line, and match
> parenthesis.
>
Will change in v6.
> > +#endif
> > +#ifdef CONFIG_SYS_FSL_SRDS_2
> > + {.id = 2, .mask = FSL_CHASSIS3_SRDS2_PRTCL_MASK,
> > + .shift = FSL_CHASSIS3_SRDS2_PRTCL_SHIFT}, #endif
> > + {.id = 0xf, .mask = 0, .shift = 0} /* NULL ENTRY */ };
>
> Can you use all zeros for the last line, including "id"?
>
Will change in v6.
> > +
> > +static int get_serdes_prctl_info_idx(u32 serdes_id) {
> > + int pos = 0;
> > + struct serdes_prctl_info *srds_info;
> > +
> > + /* loop until NULL ENTRY defined by .id=0xf */
> > + for (srds_info = srds_prctl_info; srds_info->id != 0xf;
> > + srds_info++, pos++) {
>
> Please fix the alignment.
Will change in v6.
>
> > + if (srds_info->id == serdes_id)
> > + return pos;
> > + }
> > + return -1;
>
> Put a blank line above return.
>
Will change in v6.
> > +}
> > +
> > +static void do_enabled_lanes_reset(u32 serdes_id, u32 cfg,
> > + struct ccsr_serdes __iomem *serdes_base,
> > + bool cmplt)
>
> Alignment doesn't match parenthesis. You have several of same issue below.
>
Will change in v6.
> > +{
> > + int i, pos;
> > + u32 cfg_tmp, reg = 0;
> > +
> > + pos = get_serdes_prctl_info_idx(serdes_id);
> > + if (pos == -1) {
> > + printf("invalid serdes_id %d\n", serdes_id);
> > + return;
> > + }
> > +
> > + cfg_tmp = cfg & srds_prctl_info[pos].mask;
> > + cfg_tmp >>= srds_prctl_info[pos].shift;
> > +
> > + for (i = 0; i < 4 && cfg_tmp & (0xf << (3 - i)); i++) {
> > + reg = in_le32(&serdes_base->lane[i].gcr0);
> > + reg = (cmplt ? reg | LNAGCR0_RT_RSTB :
> > + reg & LNAGCR0_RESET_MASK);
> > + out_le32(&serdes_base->lane[i].gcr0, reg);
> > + }
> > +}
> > +
> > +static void do_pll_reset(u32 cfg,
> > + struct ccsr_serdes __iomem *serdes_base) {
> > + int i;
> > + u32 reg = 0;
> > +
> > + for (i = 0; i < 2 && !(cfg & (0x1 << (1 - i))); i++) {
> > + reg = in_le32(&serdes_base->bank[i].rstctl);
> > + reg &= RSTCTL_RESET_MASK_1;
> > + reg |= RSTCTL_RSTREQ;
> > + out_le32(&serdes_base->bank[i].rstctl, reg);
> > + }
> > + udelay(1);
> > +
> > + reg = in_le32(&serdes_base->bank[i].rstctl);
> > + reg &= RSTCTL_RESET_MASK_2;
> > + out_le32(&serdes_base->bank[i].rstctl, reg);
>
> The above three lines are suspicious as the variable "i" is used out of the loop.
>
> The steps above are different from the protocol of "PLL Reset and Reconfiguration",
> described in LS2088A reference manual. Please explain if you are following different
> protocol.
>
This code was implemented as part of LS1088A, Let me check the steps in "PLL Reset and Reconfiguration"
section and update this code.
> > +}
> > +
> > +static void do_rx_tx_cal_reset(struct ccsr_serdes __iomem
> > +*serdes_base) {
> > + u32 reg = 0;
> > +
> > + reg = in_le32(&serdes_base->srdstcalcr);
> > + reg &= TCALCR_RESET_MASK;
> > + out_le32(&serdes_base->srdstcalcr, reg);
> > + reg = in_le32(&serdes_base->srdsrcalcr);
> > + reg &= TCALCR_RESET_MASK;
> > + out_le32(&serdes_base->srdsrcalcr, reg); }
>
> Please check if you can use clrbits_le32(), setbits_le32(),
> clrsetbits_le32() throughout your patch.
>
>
> <snip>
>
> > +
> > +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;
> > + int svdd_cur, svdd_tar;
> > + int ret = 1;
> > +
> > + /* Only support switch SVDD to 900mV */
> > + if (svdd != 900)
> > + return -1;
>
> Shouldn't this be -EINVAL?
>
Will take care v6
> > +
> > + /* 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
>
> To simplify the code, you can move the assignment of these variables to the beginning
> of this function, where you declare the variables.
>
> > +
> > + /* Put the all enabled lanes in reset */ #ifdef
> > +CONFIG_SYS_FSL_SRDS_1
> > + do_enabled_lanes_reset(1, cfg_rcwsrds1, serdes1_base, false); #endif
> > +
> > +#ifdef CONFIG_SYS_FSL_SRDS_2
> > + do_enabled_lanes_reset(2, cfg_rcwsrds2, serdes2_base, false); #endif
> > +
> > + /* Put the all enabled PLL in reset */ #ifdef CONFIG_SYS_FSL_SRDS_1
> > + cfg_tmp = cfg_rcwsrds1 & 0x3;
> > + do_pll_reset(cfg_tmp, serdes1_base); #endif
> > +
> > +#ifdef CONFIG_SYS_FSL_SRDS_2
> > + cfg_tmp = cfg_rcwsrds1 & 0xC;
> > + cfg_tmp >>= 2;
> > + do_pll_reset(cfg_tmp, serdes2_base); #endif
> > +
> > + /* Put the Rx/Tx calibration into reset */ #ifdef
> > +CONFIG_SYS_FSL_SRDS_1
> > + do_rx_tx_cal_reset(serdes1_base);
> > +#endif
> > +
> > +#ifdef CONFIG_SYS_FSL_SRDS_2
> > + do_rx_tx_cal_reset(serdes2_base);
> > +#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;
> > + do_serdes_enable(cfg_tmp, serdes1_base); #endif #ifdef
> > +CONFIG_SYS_FSL_SRDS_2
> > + cfg_tmp = cfg_rcwsrds1 & 0xC;
> > + cfg_tmp >>= 2;
> > + do_serdes_enable(cfg_tmp, serdes2_base); #endif
> > +
> > + /* Wait for at atleast 625us, ensure the PLLs being reset are locked
> > +*/
>
> s/atleast/at least/
>
Will take care v6
> For my education, where can I find the document describing this procedure?
>
> > + udelay(800);
> > +
> > +#ifdef CONFIG_SYS_FSL_SRDS_1
> > + cfg_tmp = cfg_rcwsrds1 & 0x3;
> > + do_pll_lock(cfg_tmp, serdes1_base);
> > +#endif
> > +
> > +#ifdef CONFIG_SYS_FSL_SRDS_2
> > + cfg_tmp = cfg_rcwsrds1 & 0xC;
> > + cfg_tmp >>= 2;
> > + do_pll_lock(cfg_tmp, serdes2_base);
> > +#endif
> > + /* Take the all enabled lanes out of reset */ #ifdef
> > +CONFIG_SYS_FSL_SRDS_1
> > + do_enabled_lanes_reset(1, cfg_rcwsrds1, serdes1_base, true); #endif
> > +#ifdef CONFIG_SYS_FSL_SRDS_2
> > + do_enabled_lanes_reset(2, cfg_rcwsrds2, serdes2_base, true); #endif
> > +
> > + /* For each PLL being reset, and achieved PLL lock set RST_DONE */
> > +#ifdef CONFIG_SYS_FSL_SRDS_1
> > + cfg_tmp = cfg_rcwsrds1 & 0x3;
> > + do_pll_reset_done(cfg_tmp, serdes1_base); #endif #ifdef
> > +CONFIG_SYS_FSL_SRDS_2
> > + cfg_tmp = cfg_rcwsrds1 & 0xC;
> > + cfg_tmp >>= 2;
> > + do_pll_reset_done(cfg_tmp, serdes2_base); #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 497a4b5..47d89b2 100644
> > --- a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> > +++ b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> > @@ -513,23 +513,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;
> > @@ -598,6 +581,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
>
> While you are on this file, can you explain why not set cdr1 in board ddr.c file? It
> seems too late to change here. You may consult Zhiqiang Hou. He added the original
> code.
>
Ok, Let me consult with him.
- Rajesh
> York
More information about the U-Boot
mailing list