[U-Boot] [PATCH v5 1/7] armv8: lsch3: Add serdes and DDR voltage setup
Rajesh Bhagat
rajesh.bhagat at nxp.com
Tue Nov 14 06:08:56 UTC 2017
> -----Original Message-----
> From: Rajesh Bhagat
> Sent: Tuesday, November 14, 2017 9:18 AM
> To: York Sun <york.sun 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
>
>
>
> > -----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.
>
Checked with Zhiqiang Hou, he added the code so that it can be re-used by other boards.
IMO, this is required for special handling of 0.9v to readjust DDR controller to operate at 0.9v,
Hence not part of board ddr.c.
> - Rajesh
>
> > York
More information about the U-Boot
mailing list