[PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related code.

Bough Chen haibo.chen at nxp.com
Tue Mar 9 08:46:49 CET 2021 

> -----Original Message-----
> From: Bough Chen
> Sent: 2021年3月8日 17:50
> To: ZHIZHIKIN Andrey <andrey.zhizhikin at leica-geosystems.com>; Peng Fan
> <peng.fan at nxp.com>; u-boot at lists.denx.de; sbabic at denx.de
> Cc: dl-uboot-imx <uboot-imx at nxp.com>; tharvey at gateworks.com;
> festevam at gmail.com; Ye Li <ye.li at nxp.com>
> Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> related code.
> 
> 
> > -----Original Message-----
> > From: ZHIZHIKIN Andrey [mailto:andrey.zhizhikin at leica-geosystems.com]
> > Sent: 2021年3月3日 20:50
> > To: Bough Chen <haibo.chen at nxp.com>; Peng Fan <peng.fan at nxp.com>;
> > u-boot at lists.denx.de; sbabic at denx.de
> > Cc: dl-uboot-imx <uboot-imx at nxp.com>; tharvey at gateworks.com;
> > festevam at gmail.com; Ye Li <ye.li at nxp.com>
> > Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> > related code.
> >
> > Hello Haibo,
> >
> > > -----Original Message-----
> > > From: Bough Chen <haibo.chen at nxp.com>
> > > Sent: Wednesday, March 3, 2021 12:27 PM
> > > To: ZHIZHIKIN Andrey <andrey.zhizhikin at leica-geosystems.com>; Peng Fan
> > > <peng.fan at nxp.com>; u-boot at lists.denx.de; sbabic at denx.de
> > > Cc: dl-uboot-imx <uboot-imx at nxp.com>; tharvey at gateworks.com;
> > > festevam at gmail.com; Ye Li <ye.li at nxp.com>
> > > Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> > > related code.
> > >
> > > > -----Original Message-----
> > > > From: ZHIZHIKIN Andrey
> > > > [mailto:andrey.zhizhikin at leica-geosystems.com]
> > > > Sent: 2021年3月3日 19:00
> > > > To: Bough Chen <haibo.chen at nxp.com>; Peng Fan <peng.fan at nxp.com>;
> > > > u-boot at lists.denx.de; sbabic at denx.de
> > > > Cc: dl-uboot-imx <uboot-imx at nxp.com>; tharvey at gateworks.com;
> > > > festevam at gmail.com; Ye Li <ye.li at nxp.com>
> > > > Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> > > > related code.
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: haibo.chen at nxp.com <haibo.chen at nxp.com>
> > > > > Sent: Wednesday, March 3, 2021 10:06 AM
> > > > > To: peng.fan at nxp.com; u-boot at lists.denx.de; sbabic at denx.de
> > > > > Cc: haibo.chen at nxp.com; uboot-imx at nxp.com;
> > tharvey at gateworks.com;
> > > > > ZHIZHIKIN Andrey <andrey.zhizhikin at leica-geosystems.com>;
> > > > > festevam at gmail.com; ye.li at nxp.com
> > > > > Subject: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> > > > > related code.
> > > > >
> > > > > From: Haibo Chen <haibo.chen at nxp.com>
> > > > >
> > > > > Common code already handle the voltage switch sequence based on
> > > > > spec, so remove the redundant voltage switch code.
> > > > >
> > > > > Signed-off-by: Haibo Chen <haibo.chen at nxp.com>
> > > > > ---
> > > > >  drivers/mmc/fsl_esdhc_imx.c | 10 +---------
> > > > >  1 file changed, 1 insertion(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/drivers/mmc/fsl_esdhc_imx.c
> > > > > b/drivers/mmc/fsl_esdhc_imx.c index
> > > > > af36558b3c..a199cf3df6 100644
> > > > > --- a/drivers/mmc/fsl_esdhc_imx.c
> > > > > +++ b/drivers/mmc/fsl_esdhc_imx.c
> > > > > @@ -515,15 +515,6 @@ static int esdhc_send_cmd_common(struct
> > > > > fsl_esdhc_priv *priv, struct mmc *mmc,
> > > > >                 goto out;
> > > > >         }
> > > > >
> > > > > -       /* Switch voltage to 1.8V if CMD11 succeeded */
> > > > > -       if (cmd->cmdidx == SD_CMD_SWITCH_UHS18V) {
> > > > > -               esdhc_setbits32(&regs->vendorspec,
> > > > ESDHC_VENDORSPEC_VSELECT);
> > > > > -
> > > > > -               printf("Run CMD11 1.8V switch\n");
> > > > > -               /* Sleep for 5 ms - max time for card to switch to
> 1.8V
> > */
> > > > > -               udelay(5000);
> > > > > -       }
> > > > > -
> > > > >         /* Workaround for ESDHC errata ENGcm03648 */
> > > > >         if (!data && (cmd->resp_type & MMC_RSP_BUSY)) {
> > > > >                 int timeout = 50000; @@ -839,6 +830,7 @@ static
> > > > > int esdhc_set_voltage(struct mmc *mmc)
> > > > >                 }
> > > > >  #endif
> > > > >                 esdhc_setbits32(&regs->vendorspec,
> > > > > ESDHC_VENDORSPEC_VSELECT);
> > > > > +               mdelay(5);
> > > >
> > > > Why is this delay introduced here? It is not clear from the commit
> > > > message whether and why it is required here.
> > > >
> > > > If this is kept from the removed block - maybe it is better to move
> > > > the corresponding comment here as well.
> > >
> > > Hi Andrev,
> > >
> > > Thanks for your careful review!
> >
> > Thanks for the patch series on the first place! This allows to switch uSDHC
> into
> > high-speed modes, which is quite valuable.
> >
> > > Without this 5ms delay, with patch1 and remove the upper redundant
> > > cmd11 related code,
> > > mmc_switch_voltage() will fail, due to the second mmc_wait_dat0()
> > > return timeout. Seems for usdhc, gate off clock for 10ms is not
> > > enough. If total delay 15ms, then the second
> > > mmc_wait_dat0() can return normal.
> > > So I add 5ms delay here. Yes, I should add a comment for this 5ms in the
> > code.
> >
> > Exactly this information is missing with the patch, as later on it would be
> quite
> > difficult to grasp on why this mdelay() was added on the first place.
> >
> > > You can also do the test on your side.
> >
> > I've already did and reported with the boot log, mmc info, and my Tested-by:
> > tag.
> >
> 
> I spend some time to dig into the extra 5ms, find this is board design issue. I
> test three boards,
> Imx8mn-ddr4-evk and imx8mp-evk board do not need this 5ms delay, only
> imx8mm-evkb board need this extra 5ms delay.
> This is because on imx8mm-evkb board, the voltage switch circuit is controlled
> by our PMIC, and this LD0 output connect one
> 10uF capacitance, makes the voltage switch from 3.3v to 1,8v need around
> 18ms. For other imx8mn and imx8mp board, the capacitance
> connected is only 1uF/4.7uF, the voltage switch time is 400us/8ms, so software
> 10ms delay can cover these two boards.
> 
> Seems this is board specific issue, I will send v2 patch, only imx8mm-evkb board
> need this delay, and will add a comment in the code.
> 

Hi all, 

Double confirm with our board design team, this is also related with the PMIC chip.
We are asking our PMIC team, whether can config the LDO transition time from the PMIC side.
So let's first hold this patch.

But for patch 1, there is no problem, Peng/Stefano, can you help pick that patch?
Only apply patch 1 can also fix the previous issue.


Best Regards
Haibo Chen


> Thanks!
> > >
> > > Best Regards
> > > Haibo
> > >
> > > >
> > > > >                 if (esdhc_read32(&regs->vendorspec) &
> > > > ESDHC_VENDORSPEC_VSELECT)
> > > > >                         return 0;
> > > > >
> > > > > --
> > > > > 2.17.1
> > > >
> > > > -- andrey
> >
> > -- andrey
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 9571 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210309/668fffed/attachment.bin>

More information about the U-Boot mailing list