RE: [PATCH 2/2] mmc: fsl_esdhc: make sure delay chain locked for HS400

[Y.b. Lu]

Hi Jaehoon,

> -----Original Message-----
> From: Jaehoon Chung <jh80.ch...@samsung.com>
> Sent: Tuesday, October 20, 2020 5:52 AM
> To: Y.b. Lu <yangbo...@nxp.com>; u-boot@lists.denx.de; Peng Fan
> <peng....@nxp.com>
> Subject: Re: [PATCH 2/2] mmc: fsl_esdhc: make sure delay chain locked for
> HS400
> 
> Dear Yangbo,
> 
> On 10/16/20 12:13 PM, Yangbo Lu wrote:
> > For eMMC HS400 mode, the DLL reset is a required step for mmc rescan.
> > This step has not been documented in reference manual, but the RM will
> > be fixed sooner or later.
> >
> > This patch is to add the step of DLL reset, and make sure delay chain
> > locked for HS400.
> >
> > Fixes: db8f93672b42 ("mmc: fsl_esdhc: support eMMC HS400 mode")
> > Signed-off-by: Yangbo Lu <yangbo...@nxp.com>
> 
> Reviewed-by: Jaehoon Chung <jh80.ch...@samsung.com>
> 
> Just added minor comments.
> 
> 
> > ---
> >  drivers/mmc/fsl_esdhc.c | 28 +++++++++++++++++++++++++---
> >  include/fsl_esdhc.h     |  4 ++++
> >  2 files changed, 29 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
> > index 68130ee..a18316e 100644
> > --- a/drivers/mmc/fsl_esdhc.c
> > +++ b/drivers/mmc/fsl_esdhc.c
> > @@ -70,7 +70,9 @@ struct fsl_esdhc {
> >     uint    sdtimingctl;    /* SD timing control register */
> >     char    reserved8[20];  /* reserved */
> >     uint    dllcfg0;        /* DLL config 0 register */
> > -   char    reserved9[680]; /* reserved */
> > +   char    reserved9[12];  /* reserved */
> > +   uint    dllstat0;       /* DLL status 0 register */
> > +   char    reserved10[664];/* reserved */
> >     uint    esdhcctl;       /* eSDHC control register */
> >  };
> >
> > @@ -617,9 +619,11 @@ static void esdhc_exit_hs400(struct fsl_esdhc_priv
> *priv)
> >     esdhc_tuning_block_enable(priv, false);
> >  }
> >
> > -static void esdhc_set_timing(struct fsl_esdhc_priv *priv, enum bus_mode
> mode)
> > +static int esdhc_set_timing(struct fsl_esdhc_priv *priv, enum bus_mode
> mode)
> >  {
> >     struct fsl_esdhc *regs = priv->esdhc_regs;
> > +   ulong start;
> > +   u32 val;
> >
> >     /* Exit HS400 mode before setting any other mode */
> >     if (esdhc_read32(&regs->tbctl) & HS400_MODE &&
> > @@ -640,17 +644,33 @@ static void esdhc_set_timing(struct fsl_esdhc_priv
> *priv, enum bus_mode mode)
> >                     esdhc_setbits32(&regs->dllcfg0, DLL_FREQ_SEL);
> >
> >             esdhc_setbits32(&regs->dllcfg0, DLL_ENABLE);
> > +
> > +           esdhc_setbits32(&regs->dllcfg0, DLL_RESET);
> > +           udelay(1);
> 
> Could you add a light comment why need to put udelay(1)?

Actually this is just per fixed reference manual.
I sent out v2 patch, to explain in commit message in case some one what to know 
why.

"    In previous commit to support eMMC HS400,
      db8f936 mmc: fsl_esdhc: support eMMC HS400 mode

    the steps to configure DLL could be found in commit message,
      13. Set DLLCFG0[DLL_ENABLE] and DLLCFG0[DLL_FREQ_SEL].
      14. Wait for delay chain to lock.

    these would be fixed as,
      13.   Set DLLCFG0[DLL_ENABLE] and DLLCFG0[DLL_FREQ_SEL].
      13.1  Write DLLCFG0[DLL_RESET] to 1 and wait for 1us,
            then write DLLCFG0[DLL_RESET]
      14.   Wait for delay chain to lock."

Thanks.

> 
> Best Regards,
> Jaehoon Chung
> 
> > +           esdhc_clrbits32(&regs->dllcfg0, DLL_RESET);
> > +
> > +           start = get_timer(0);
> > +           val = DLL_STS_SLV_LOCK;
> > +           while (!(esdhc_read32(&regs->dllstat0) & val)) {
> > +                   if (get_timer(start) > 1000) {
> > +                           printf("fsl_esdhc: delay chain lock timeout\n");
> > +                           return -ETIMEDOUT;
> > +                   }
> > +           }
> > +
> >             esdhc_setbits32(&regs->tbctl, HS400_WNDW_ADJUST);
> >
> >             esdhc_clock_control(priv, false);
> >             esdhc_flush_async_fifo(priv);
> >     }
> >     esdhc_clock_control(priv, true);
> > +   return 0;
> >  }
> >
> >  static int esdhc_set_ios_common(struct fsl_esdhc_priv *priv, struct mmc
> *mmc)
> >  {
> >     struct fsl_esdhc *regs = priv->esdhc_regs;
> > +   int ret;
> >
> >     if (priv->is_sdhc_per_clk) {
> >             /* Select to use peripheral clock */
> > @@ -667,7 +687,9 @@ static int esdhc_set_ios_common(struct
> fsl_esdhc_priv *priv, struct mmc *mmc)
> >             set_sysctl(priv, mmc, mmc->clock);
> >
> >     /* Set timing */
> > -   esdhc_set_timing(priv, mmc->selected_mode);
> > +   ret = esdhc_set_timing(priv, mmc->selected_mode);
> > +   if (ret)
> > +           return ret;
> >
> >     /* Set the bus width */
> >     esdhc_clrbits32(&regs->proctl, PROCTL_DTW_4 | PROCTL_DTW_8);
> > diff --git a/include/fsl_esdhc.h b/include/fsl_esdhc.h
> > index e6f1c75..850a304 100644
> > --- a/include/fsl_esdhc.h
> > +++ b/include/fsl_esdhc.h
> > @@ -187,8 +187,12 @@
> >
> >  /* DLL config 0 register */
> >  #define DLL_ENABLE         0x80000000
> > +#define DLL_RESET          0x40000000
> >  #define DLL_FREQ_SEL               0x08000000
> >
> > +/* DLL status 0 register */
> > +#define DLL_STS_SLV_LOCK   0x08000000
> > +
> >  #define MAX_TUNING_LOOP            40
> >
> >  #define HOSTVER_VENDOR(x)  (((x) >> 8) & 0xff)
> >