[PATCH 1/2] mmc: sdhci-esdhc-imx: refactor clock loopback selection logic

[Bough Chen]

> -----Original Message-----
> From: Bough Chen
> Sent: 2025年5月22日 11:53
> To: Frank Li <frank.li at nxp.com>; Luke Wang <ziniu.wang_1 at nxp.com>
> Cc: adrian.hunter at intel.com; ulf.hansson at linaro.org;
> linux-mmc at vger.kernel.org; shawnguo at kernel.org; s.hauer at pengutronix.de;
> kernel at pengutronix.de; festevam at gmail.com; imx at lists.linux.dev; dl-S32
> <S32 at nxp.com>; linux-arm-kernel at lists.infradead.org;
> linux-kernel at vger.kernel.org
> Subject: RE: [PATCH 1/2] mmc: sdhci-esdhc-imx: refactor clock loopback
> selection logic
> 
> > -----Original Message-----
> > From: Frank Li <frank.li at nxp.com>
> > Sent: 2025年5月22日 0:10
> > To: Luke Wang <ziniu.wang_1 at nxp.com>
> > Cc: Bough Chen <haibo.chen at nxp.com>; adrian.hunter at intel.com;
> > ulf.hansson at linaro.org; linux-mmc at vger.kernel.org;
> > shawnguo at kernel.org; s.hauer at pengutronix.de; kernel at pengutronix.de;
> > festevam at gmail.com; imx at lists.linux.dev; dl-S32 <S32 at nxp.com>;
> > linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org
> > Subject: Re: [PATCH 1/2] mmc: sdhci-esdhc-imx: refactor clock loopback
> > selection logic
> >
> > On Wed, May 21, 2025 at 10:55:01AM +0800, ziniu.wang_1 at nxp.com wrote:
> > > From: Luke Wang <ziniu.wang_1 at nxp.com>
> > >
> > > i.MX reference manual specifies that internal clock loopback should
> > > be used for SDR104/HS200/HS400 modes. Move
> ESDHC_MIX_CTRL_FBCLK_SEL
> > > configuration into the timing selection function to:
> > >
> > > 1. Explicitly set internal loopback path for SDR104/HS200/HS400
> > > modes 2. Avoid redundant bit manipulation across multiple functions
> > >
> > > Preserve ESDHC_MIX_CTRL_FBCLK_SEL during system resume for SDIO
> > > devices with MMC_PM_KEEP_POWER and MMC_PM_WAKE_SDIO_IRQ flag,
> > as the
> > > controller might lose register state during suspend while skipping
> > > card re-initialization.
> >
> > what's means?
> >
> > controller lost power during suspend, so
> > u32 m = readl(host->ioaddr + ESDHC_MIX_CTRL) to get reset value and
> > miss
> >
> > set ~ESDHC_MIX_CTRL_FBCLK_SEL?
> 
> Miss set ESDHC_MIX_CTRL_FBCLK_SEL.
> 
> Previous SDIO3.0 device with MMC_PM_KEEP_POWER will set
> ESDHC_MIX_CTRL_FBCLK_SEL, After system resume, need to re-config
> ESDHC_MIX_CTRL_FBCLK_SEL. But SDIO3.0 device with
> MMC_PM_KEEP_POWER will not re-initialization, but will call set_ios->
> set_uhs_signaling().

sdhc_esdhc_tuning_restore() will also set back the ESDHC_MIX_CTRL_FBCLK_SEL..

Regards
Haibo Chen
> 
> Regards
> Haibo Chen
> >
> > Frank
> >
> > >
> > > Signed-off-by: Luke Wang <ziniu.wang_1 at nxp.com>
> > > ---
> > >  drivers/mmc/host/sdhci-esdhc-imx.c | 27 ++++++++++++++-------------
> > >  1 file changed, 14 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
> > > b/drivers/mmc/host/sdhci-esdhc-imx.c
> > > index 7611682f10c3..c448a53530a5 100644
> > > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > > @@ -728,23 +728,17 @@ static void esdhc_writew_le(struct sdhci_host
> > *host, u16 val, int reg)
> > >  		writel(new_val, host->ioaddr + ESDHC_VENDOR_SPEC);
> > >  		if (imx_data->socdata->flags & ESDHC_FLAG_STD_TUNING) {
> > >  			u32 v = readl(host->ioaddr + SDHCI_AUTO_CMD_STATUS);
> > > -			u32 m = readl(host->ioaddr + ESDHC_MIX_CTRL);
> > > -			if (val & SDHCI_CTRL_TUNED_CLK) {
> > > +			if (val & SDHCI_CTRL_TUNED_CLK)
> > >  				v |= ESDHC_MIX_CTRL_SMPCLK_SEL;
> > > -			} else {
> > > +			else
> > >  				v &= ~ESDHC_MIX_CTRL_SMPCLK_SEL;
> > > -				m &= ~ESDHC_MIX_CTRL_FBCLK_SEL;
> > > -			}
> > >
> > > -			if (val & SDHCI_CTRL_EXEC_TUNING) {
> > > +			if (val & SDHCI_CTRL_EXEC_TUNING)
> > >  				v |= ESDHC_MIX_CTRL_EXE_TUNE;
> > > -				m |= ESDHC_MIX_CTRL_FBCLK_SEL;
> > > -			} else {
> > > +			else
> > >  				v &= ~ESDHC_MIX_CTRL_EXE_TUNE;
> > > -			}
> > >
> > >  			writel(v, host->ioaddr + SDHCI_AUTO_CMD_STATUS);
> > > -			writel(m, host->ioaddr + ESDHC_MIX_CTRL);
> > >  		}
> > >  		return;
> > >  	case SDHCI_TRANSFER_MODE:
> > > @@ -1082,7 +1076,6 @@ static void esdhc_reset_tuning(struct
> > > sdhci_host
> > *host)
> > >  		ctrl &= ~ESDHC_MIX_CTRL_AUTO_TUNE_EN;
> > >  		if (imx_data->socdata->flags & ESDHC_FLAG_MAN_TUNING) {
> > >  			ctrl &= ~ESDHC_MIX_CTRL_SMPCLK_SEL;
> > > -			ctrl &= ~ESDHC_MIX_CTRL_FBCLK_SEL;
> > >  			writel(ctrl, host->ioaddr + ESDHC_MIX_CTRL);
> > >  			writel(0, host->ioaddr + ESDHC_TUNE_CTRL_STATUS);
> > >  		} else if (imx_data->socdata->flags &
> ESDHC_FLAG_STD_TUNING)
> > { @@
> > > -1177,8 +1170,7 @@ static void esdhc_prepare_tuning(struct
> > > sdhci_host
> > *host, u32 val)
> > >  		"warning! RESET_ALL never complete before sending tuning
> > > command\n");
> > >
> > >  	reg = readl(host->ioaddr + ESDHC_MIX_CTRL);
> > > -	reg |= ESDHC_MIX_CTRL_EXE_TUNE | ESDHC_MIX_CTRL_SMPCLK_SEL |
> > > -			ESDHC_MIX_CTRL_FBCLK_SEL;
> > > +	reg |= ESDHC_MIX_CTRL_EXE_TUNE |
> ESDHC_MIX_CTRL_SMPCLK_SEL;
> > >  	writel(reg, host->ioaddr + ESDHC_MIX_CTRL);
> > >
> > 	writel(FIELD_PREP(ESDHC_TUNE_CTRL_STATUS_DLY_CELL_SET_PRE_MAS
> > K, val),
> > >  	       host->ioaddr + ESDHC_TUNE_CTRL_STATUS); @@ -1432,6
> > +1424,15
> > > @@ static void esdhc_set_uhs_signaling(struct sdhci_host *host,
> > > unsigned
> > timing)
> > >  		break;
> > >  	}
> > >
> > > +	if (timing == MMC_TIMING_UHS_SDR104 ||
> > > +	    timing == MMC_TIMING_MMC_HS200 ||
> > > +	    timing == MMC_TIMING_MMC_HS400)
> > > +		m |= ESDHC_MIX_CTRL_FBCLK_SEL;
> > > +	else
> > > +		m &= ~ESDHC_MIX_CTRL_FBCLK_SEL;
> > > +
> > > +	writel(m, host->ioaddr + ESDHC_MIX_CTRL);
> > > +
> > >  	esdhc_change_pinstate(host, timing);  }
> > >
> > > --
> > > 2.34.1
> > >