RE: [PATCH V2] mmc: sdhci-of_dwcmshc: Change to dwcmshc_phy_init for reusing codes

["Jaehoon Chung" <jh80.chung@samsung.com>]

> -----Original Message-----
> From: Adrian Hunter <adrian.hunter@intel.com>
> Sent: Wednesday, January 8, 2025 10:52 PM
>
> On 16/12/24 12:51, Jaehoon Chung wrote:
> > dwcmshc_phy_1_8v_init and dwcmshc_phy_3_3v_init differ only by a few
> > lines of code. This allow us to reuse code depending on voltage.
> >
> > Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
> > ---
> > Changelog V2:
> > - Add more conditions to clarify if it's MMC_SIGNAL_VOLTAGE_180
> > - Order the local variable according to Adrian's comment
> > - Use local variable to make more readable
> > ---
> >  drivers/mmc/host/sdhci-of-dwcmshc.c | 75 +++++++----------------------
> >  1 file changed, 18 insertions(+), 57 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> > index 7ea3da45db32..08a9b963fb1a 100644
> > --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> > +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> > @@ -328,12 +328,18 @@ static void dwcmshc_request(struct mmc_host *mmc, struct mmc_request *mrq)
> >  	sdhci_request(mmc, mrq);
> >  }
> >
> > -static void dwcmshc_phy_1_8v_init(struct sdhci_host *host)
> > +static void dwcmshc_phy_init(struct sdhci_host *host)
> >  {
> >  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >  	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> > +	u32 rxsel = PHY_PAD_RXSEL_3V3;
> >  	u32 val;
> >
> > +	if (priv->flags & FLAG_IO_FIXED_1V8 ||
> > +		(host->mmc->ios.timing & MMC_SIGNAL_VOLTAGE_180 &&
> > +		 host->flags & SDHCI_SIGNALING_180))
>
> Thanks for making the changes.  It looks like
> "&& host->flags & SDHCI_SIGNALING_180" is not needed

Will check after changing your suggestion.

>
> > +		rxsel = PHY_PAD_RXSEL_1V8;
> > +
> >  	/* deassert phy reset & set tx drive strength */
> >  	val = PHY_CNFG_RSTN_DEASSERT;
> >  	val |= FIELD_PREP(PHY_CNFG_PAD_SP_MASK, PHY_CNFG_PAD_SP);
> > @@ -353,7 +359,7 @@ static void dwcmshc_phy_1_8v_init(struct sdhci_host *host)
> >  	sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R);
> >
> >  	/* configure phy pads */
> > -	val = PHY_PAD_RXSEL_1V8;
> > +	val = rxsel;
> >  	val |= FIELD_PREP(PHY_PAD_WEAKPULL_MASK, PHY_PAD_WEAKPULL_PULLUP);
> >  	val |= FIELD_PREP(PHY_PAD_TXSLEW_CTRL_P_MASK, PHY_PAD_TXSLEW_CTRL_P);
> >  	val |= FIELD_PREP(PHY_PAD_TXSLEW_CTRL_N_MASK, PHY_PAD_TXSLEW_CTRL_N);
> > @@ -365,65 +371,24 @@ static void dwcmshc_phy_1_8v_init(struct sdhci_host *host)
> >  	val |= FIELD_PREP(PHY_PAD_TXSLEW_CTRL_N_MASK, PHY_PAD_TXSLEW_CTRL_N);
> >  	sdhci_writew(host, val, PHY_CLKPAD_CNFG_R);
> >
> > -	val = PHY_PAD_RXSEL_1V8;
> > +	val = rxsel;
> >  	val |= FIELD_PREP(PHY_PAD_WEAKPULL_MASK, PHY_PAD_WEAKPULL_PULLDOWN);
> >  	val |= FIELD_PREP(PHY_PAD_TXSLEW_CTRL_P_MASK, PHY_PAD_TXSLEW_CTRL_P);
> >  	val |= FIELD_PREP(PHY_PAD_TXSLEW_CTRL_N_MASK, PHY_PAD_TXSLEW_CTRL_N);
> >  	sdhci_writew(host, val, PHY_STBPAD_CNFG_R);
> >
> >  	/* enable data strobe mode */
> > -	sdhci_writeb(host, FIELD_PREP(PHY_DLLDL_CNFG_SLV_INPSEL_MASK, PHY_DLLDL_CNFG_SLV_INPSEL),
> > -		     PHY_DLLDL_CNFG_R);
> > -
> > -	/* enable phy dll */
> > -	sdhci_writeb(host, PHY_DLL_CTRL_ENABLE, PHY_DLL_CTRL_R);
> > -}
> > -
> > -static void dwcmshc_phy_3_3v_init(struct sdhci_host *host)
> > -{
> > -	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > -	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> > -	u32 val;
> > -
> > -	/* deassert phy reset & set tx drive strength */
> > -	val = PHY_CNFG_RSTN_DEASSERT;
> > -	val |= FIELD_PREP(PHY_CNFG_PAD_SP_MASK, PHY_CNFG_PAD_SP);
> > -	val |= FIELD_PREP(PHY_CNFG_PAD_SN_MASK, PHY_CNFG_PAD_SN);
> > -	sdhci_writel(host, val, PHY_CNFG_R);
> > -
> > -	/* disable delay line */
> > -	sdhci_writeb(host, PHY_SDCLKDL_CNFG_UPDATE, PHY_SDCLKDL_CNFG_R);
> > -
> > -	/* set delay line */
> > -	sdhci_writeb(host, priv->delay_line, PHY_SDCLKDL_DC_R);
> > -	sdhci_writeb(host, PHY_DLL_CNFG2_JUMPSTEP, PHY_DLL_CNFG2_R);
> > -
> > -	/* enable delay lane */
> > -	val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R);
> > -	val &= ~(PHY_SDCLKDL_CNFG_UPDATE);
> > -	sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R);
> > +	if (priv->flags & FLAG_IO_FIXED_1V8 ||
> > +		(host->mmc->ios.timing & MMC_SIGNAL_VOLTAGE_180 &&
> > +		 host->flags & SDHCI_SIGNALING_180)) {
>
> Probably neater to check rxsel e.g.
>
> 	if (rxcel == PHY_PAD_RXSEL_1V8) {

Got it. Thanks!

Best Regards,
Jaehoon Chung

>
> > +		u8 sel = FIELD_PREP(PHY_DLLDL_CNFG_SLV_INPSEL_MASK, PHY_DLLDL_CNFG_SLV_INPSEL);
> >
> > -	/* configure phy pads */
> > -	val = PHY_PAD_RXSEL_3V3;
> > -	val |= FIELD_PREP(PHY_PAD_WEAKPULL_MASK, PHY_PAD_WEAKPULL_PULLUP);
> > -	val |= FIELD_PREP(PHY_PAD_TXSLEW_CTRL_P_MASK, PHY_PAD_TXSLEW_CTRL_P);
> > -	val |= FIELD_PREP(PHY_PAD_TXSLEW_CTRL_N_MASK, PHY_PAD_TXSLEW_CTRL_N);
> > -	sdhci_writew(host, val, PHY_CMDPAD_CNFG_R);
> > -	sdhci_writew(host, val, PHY_DATAPAD_CNFG_R);
> > -	sdhci_writew(host, val, PHY_RSTNPAD_CNFG_R);
> > -
> > -	val = FIELD_PREP(PHY_PAD_TXSLEW_CTRL_P_MASK, PHY_PAD_TXSLEW_CTRL_P);
> > -	val |= FIELD_PREP(PHY_PAD_TXSLEW_CTRL_N_MASK, PHY_PAD_TXSLEW_CTRL_N);
> > -	sdhci_writew(host, val, PHY_CLKPAD_CNFG_R);
> > -
> > -	val = PHY_PAD_RXSEL_3V3;
> > -	val |= FIELD_PREP(PHY_PAD_WEAKPULL_MASK, PHY_PAD_WEAKPULL_PULLDOWN);
> > -	val |= FIELD_PREP(PHY_PAD_TXSLEW_CTRL_P_MASK, PHY_PAD_TXSLEW_CTRL_P);
> > -	val |= FIELD_PREP(PHY_PAD_TXSLEW_CTRL_N_MASK, PHY_PAD_TXSLEW_CTRL_N);
> > -	sdhci_writew(host, val, PHY_STBPAD_CNFG_R);
> > +		sdhci_writeb(host, sel, PHY_DLLDL_CNFG_R);
> > +	}
> >
> >  	/* enable phy dll */
> >  	sdhci_writeb(host, PHY_DLL_CTRL_ENABLE, PHY_DLL_CTRL_R);
> > +
> >  }
> >
> >  static void th1520_sdhci_set_phy(struct sdhci_host *host)
> > @@ -433,11 +398,7 @@ static void th1520_sdhci_set_phy(struct sdhci_host *host)
> >  	u32 emmc_caps = MMC_CAP2_NO_SD | MMC_CAP2_NO_SDIO;
> >  	u16 emmc_ctrl;
> >
> > -	/* Before power on, set PHY configs */
> > -	if (priv->flags & FLAG_IO_FIXED_1V8)
> > -		dwcmshc_phy_1_8v_init(host);
> > -	else
> > -		dwcmshc_phy_3_3v_init(host);
> > +	dwcmshc_phy_init(host);
> >
> >  	if ((host->mmc->caps2 & emmc_caps) == emmc_caps) {
> >  		emmc_ctrl = sdhci_readw(host, priv->vendor_specific_area1 + DWCMSHC_EMMC_CONTROL);
> > @@ -1163,7 +1124,7 @@ static const struct sdhci_ops sdhci_dwcmshc_th1520_ops = {
> >  	.get_max_clock		= dwcmshc_get_max_clock,
> >  	.reset			= th1520_sdhci_reset,
> >  	.adma_write_desc	= dwcmshc_adma_write_desc,
> > -	.voltage_switch		= dwcmshc_phy_1_8v_init,
> > +	.voltage_switch		= dwcmshc_phy_init,
> >  	.platform_execute_tuning = th1520_execute_tuning,
> >  };
> >
>