| Message ID | 20241204100507.330025-1-jh80.chung@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: sdhci-of_dwcmshc: Change to dwcmshc_phy_init for reusing codes | expand |
On 4/12/24 12:05, 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> > --- > drivers/mmc/host/sdhci-of-dwcmshc.c | 69 +++++------------------------ > 1 file changed, 12 insertions(+), 57 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c > index 7ea3da45db32..87bc32d13cc0 100644 > --- a/drivers/mmc/host/sdhci-of-dwcmshc.c > +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c > @@ -328,11 +328,15 @@ 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 val; > + u32 rxsel = PHY_PAD_RXSEL_3V3; Nicer to order local variables by descending line length when possible i.e. u32 rxsel = PHY_PAD_RXSEL_3V3; u32 val; > + > + if (host->flags & SDHCI_SIGNALING_180 || priv->flags & FLAG_IO_FIXED_1V8) 'host->flags & SDHCI_SIGNALING_180' only means 1.8V signaling is supported, so this looks strange. Can you clarify this? > + rxsel = PHY_PAD_RXSEL_1V8; > > /* deassert phy reset & set tx drive strength */ > val = PHY_CNFG_RSTN_DEASSERT; > @@ -353,7 +357,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 +369,20 @@ 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); > + if (host->flags & SDHCI_SIGNALING_180 || priv->flags & FLAG_IO_FIXED_1V8) > + sdhci_writeb(host, FIELD_PREP(PHY_DLLDL_CNFG_SLV_INPSEL_MASK, > + PHY_DLLDL_CNFG_SLV_INPSEL), PHY_DLLDL_CNFG_R); Perhaps slightly more readable to use a variable e.g. /* enable data strobe mode */ if (???) { u8 sel = FIELD_PREP(PHY_DLLDL_CNFG_SLV_INPSEL_MASK, PHY_DLLDL_CNFG_SLV_INPSEL); sdhci_writeb(host, sel, 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); > - > - /* 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); > > - /* 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 +392,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 +1118,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, > }; >
> -----Original Message----- > From: Adrian Hunter <adrian.hunter@intel.com> > Sent: Thursday, December 5, 2024 10:17 PM > > On 4/12/24 12:05, 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> > > --- > > drivers/mmc/host/sdhci-of-dwcmshc.c | 69 +++++------------------------ > > 1 file changed, 12 insertions(+), 57 deletions(-) > > > > diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c > > index 7ea3da45db32..87bc32d13cc0 100644 > > --- a/drivers/mmc/host/sdhci-of-dwcmshc.c > > +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c > > @@ -328,11 +328,15 @@ 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 val; > > + u32 rxsel = PHY_PAD_RXSEL_3V3; > > Nicer to order local variables by descending line length when > possible i.e. > > u32 rxsel = PHY_PAD_RXSEL_3V3; > u32 val; > > > + > > + if (host->flags & SDHCI_SIGNALING_180 || priv->flags & FLAG_IO_FIXED_1V8) > > 'host->flags & SDHCI_SIGNALING_180' only means 1.8V signaling > is supported, so this looks strange. Can you clarify this? AFAIK, SDHCI_SIGNALING_180 is to check if its IP(Host controller) supports 1.8V as you mentioned. The previous function (dwcmshc_phy_1_8v_init) is called in sdhci.c. through host->ops->voltage_switch(). I couldn't find any other place where its function is called. When ios->signal_voltage is set to MMC_SIGNAL_VOLTAGE_180, dwcmshc_pyh_1_8v_init is called. In the switch statement, if '(!(host->flags & SDHCI_SIGNALING_180))' evaluates to true, it returns -EINVAL. So I think that there are only two possible scenarios involving "SDHCI_SIGNALING_180 or FLAG_IO_FIXED_1V8". To clarify this, I will change to if (!!(host->flags & SDHCI_SIGNALING_180) || priv->flags & FLAG_IO_FIXED_1V8). And if my understanding is something wrong, let me know, plz. Additionally, I have tested on LicheePi4A. > > > + rxsel = PHY_PAD_RXSEL_1V8; > > > > /* deassert phy reset & set tx drive strength */ > > val = PHY_CNFG_RSTN_DEASSERT; > > @@ -353,7 +357,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 +369,20 @@ 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); > > + if (host->flags & SDHCI_SIGNALING_180 || priv->flags & FLAG_IO_FIXED_1V8) > > + sdhci_writeb(host, FIELD_PREP(PHY_DLLDL_CNFG_SLV_INPSEL_MASK, > > + PHY_DLLDL_CNFG_SLV_INPSEL), PHY_DLLDL_CNFG_R); > > Perhaps slightly more readable to use a variable e.g. Okay. I will update. It was kept the previous code. Best Regards, Jaehoon Chung > > /* enable data strobe mode */ > if (???) { > u8 sel = FIELD_PREP(PHY_DLLDL_CNFG_SLV_INPSEL_MASK, PHY_DLLDL_CNFG_SLV_INPSEL); > > sdhci_writeb(host, sel, 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); > > - > > - /* 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); > > > > - /* 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 +392,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 +1118,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, > > }; > >
On 6/12/24 01:05, Jaehoon Chung wrote: > > >> -----Original Message----- >> From: Adrian Hunter <adrian.hunter@intel.com> >> Sent: Thursday, December 5, 2024 10:17 PM >> >> On 4/12/24 12:05, 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> >>> --- >>> drivers/mmc/host/sdhci-of-dwcmshc.c | 69 +++++------------------------ >>> 1 file changed, 12 insertions(+), 57 deletions(-) >>> >>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c >>> index 7ea3da45db32..87bc32d13cc0 100644 >>> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c >>> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c >>> @@ -328,11 +328,15 @@ 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 val; >>> + u32 rxsel = PHY_PAD_RXSEL_3V3; >> >> Nicer to order local variables by descending line length when >> possible i.e. >> >> u32 rxsel = PHY_PAD_RXSEL_3V3; >> u32 val; >> >>> + >>> + if (host->flags & SDHCI_SIGNALING_180 || priv->flags & FLAG_IO_FIXED_1V8) >> >> 'host->flags & SDHCI_SIGNALING_180' only means 1.8V signaling >> is supported, so this looks strange. Can you clarify this? > > AFAIK, SDHCI_SIGNALING_180 is to check if its IP(Host controller) supports 1.8V as you mentioned. > The previous function (dwcmshc_phy_1_8v_init) is called in sdhci.c. through host->ops->voltage_switch(). > I couldn't find any other place where its function is called. > > When ios->signal_voltage is set to MMC_SIGNAL_VOLTAGE_180, dwcmshc_pyh_1_8v_init is called. > > In the switch statement, if '(!(host->flags & SDHCI_SIGNALING_180))' evaluates to true, it returns -EINVAL. > > So I think that there are only two possible scenarios involving "SDHCI_SIGNALING_180 or FLAG_IO_FIXED_1V8". A third possibility is host->flags has both MMC_SIGNAL_VOLTAGE_180 and SDHCI_SIGNALING_330, and FLAG_IO_FIXED_1V8 is false. What should it do then? > > To clarify this, I will change to if (!!(host->flags & SDHCI_SIGNALING_180) || priv->flags & FLAG_IO_FIXED_1V8). > > And if my understanding is something wrong, let me know, plz. > > Additionally, I have tested on LicheePi4A. > >> >>> + rxsel = PHY_PAD_RXSEL_1V8; >>> >>> /* deassert phy reset & set tx drive strength */ >>> val = PHY_CNFG_RSTN_DEASSERT; >>> @@ -353,7 +357,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 +369,20 @@ 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); >>> + if (host->flags & SDHCI_SIGNALING_180 || priv->flags & FLAG_IO_FIXED_1V8) >>> + sdhci_writeb(host, FIELD_PREP(PHY_DLLDL_CNFG_SLV_INPSEL_MASK, >>> + PHY_DLLDL_CNFG_SLV_INPSEL), PHY_DLLDL_CNFG_R); >> >> Perhaps slightly more readable to use a variable e.g. > > Okay. I will update. It was kept the previous code. > > Best Regards, > Jaehoon Chung > >> >> /* enable data strobe mode */ >> if (???) { >> u8 sel = FIELD_PREP(PHY_DLLDL_CNFG_SLV_INPSEL_MASK, PHY_DLLDL_CNFG_SLV_INPSEL); >> >> sdhci_writeb(host, sel, 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); >>> - >>> - /* 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); >>> >>> - /* 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 +392,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 +1118,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, >>> }; >>> > > >
Hi Adrian, > -----Original Message----- > From: Adrian Hunter <adrian.hunter@intel.com> > Sent: Monday, December 9, 2024 5:14 PM > To: Jaehoon Chung <jh80.chung@samsung.com>; linux-mmc@vger.kernel.org > Cc: ulf.hansson@linaro.org > Subject: Re: [PATCH] mmc: sdhci-of_dwcmshc: Change to dwcmshc_phy_init for reusing codes > > On 6/12/24 01:05, Jaehoon Chung wrote: > > > > > >> -----Original Message----- > >> From: Adrian Hunter <adrian.hunter@intel.com> > >> Sent: Thursday, December 5, 2024 10:17 PM > >> > >> On 4/12/24 12:05, 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> > >>> --- > >>> drivers/mmc/host/sdhci-of-dwcmshc.c | 69 +++++------------------------ > >>> 1 file changed, 12 insertions(+), 57 deletions(-) > >>> > >>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c > >>> index 7ea3da45db32..87bc32d13cc0 100644 > >>> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c > >>> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c > >>> @@ -328,11 +328,15 @@ 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 val; > >>> + u32 rxsel = PHY_PAD_RXSEL_3V3; > >> > >> Nicer to order local variables by descending line length when > >> possible i.e. > >> > >> u32 rxsel = PHY_PAD_RXSEL_3V3; > >> u32 val; > >> > >>> + > >>> + if (host->flags & SDHCI_SIGNALING_180 || priv->flags & FLAG_IO_FIXED_1V8) > >> > >> 'host->flags & SDHCI_SIGNALING_180' only means 1.8V signaling > >> is supported, so this looks strange. Can you clarify this? > > > > AFAIK, SDHCI_SIGNALING_180 is to check if its IP(Host controller) supports 1.8V as you mentioned. > > The previous function (dwcmshc_phy_1_8v_init) is called in sdhci.c. through host->ops- > >voltage_switch(). > > I couldn't find any other place where its function is called. > > > > When ios->signal_voltage is set to MMC_SIGNAL_VOLTAGE_180, dwcmshc_pyh_1_8v_init is called. > > > > In the switch statement, if '(!(host->flags & SDHCI_SIGNALING_180))' evaluates to true, it returns - > EINVAL. > > > > So I think that there are only two possible scenarios involving "SDHCI_SIGNALING_180 or > FLAG_IO_FIXED_1V8". > > A third possibility is host->flags has both MMC_SIGNAL_VOLTAGE_180 > and SDHCI_SIGNALING_330, and FLAG_IO_FIXED_1V8 is false. > What should it do then? Ah, got it. I didn't consider about called from sdhci_set_uhs_signaling(). How about checking below? + if (host->flags & FLAG_IO_FIXED_1V8 || + (host->mmc->ios.timing & MMC_SIGNAL_VOLTAGE_180 && host->flags & SDHCI_SIGNALING_180)) I think that it's clarifying about other cases. Best Regards, Jaehoon Chung > > > > > To clarify this, I will change to if (!!(host->flags & SDHCI_SIGNALING_180) || priv->flags & > FLAG_IO_FIXED_1V8). > > > > And if my understanding is something wrong, let me know, plz. > > > > Additionally, I have tested on LicheePi4A. > > > >> > >>> + rxsel = PHY_PAD_RXSEL_1V8; > >>> > >>> /* deassert phy reset & set tx drive strength */ > >>> val = PHY_CNFG_RSTN_DEASSERT; > >>> @@ -353,7 +357,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 +369,20 @@ 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); > >>> + if (host->flags & SDHCI_SIGNALING_180 || priv->flags & FLAG_IO_FIXED_1V8) > >>> + sdhci_writeb(host, FIELD_PREP(PHY_DLLDL_CNFG_SLV_INPSEL_MASK, > >>> + PHY_DLLDL_CNFG_SLV_INPSEL), PHY_DLLDL_CNFG_R); > >> > >> Perhaps slightly more readable to use a variable e.g. > > > > Okay. I will update. It was kept the previous code. > > > > Best Regards, > > Jaehoon Chung > > > >> > >> /* enable data strobe mode */ > >> if (???) { > >> u8 sel = FIELD_PREP(PHY_DLLDL_CNFG_SLV_INPSEL_MASK, PHY_DLLDL_CNFG_SLV_INPSEL); > >> > >> sdhci_writeb(host, sel, 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); > >>> - > >>> - /* 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); > >>> > >>> - /* 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 +392,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 +1118,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, > >>> }; > >>> > > > > > >
diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c index 7ea3da45db32..87bc32d13cc0 100644 --- a/drivers/mmc/host/sdhci-of-dwcmshc.c +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c @@ -328,11 +328,15 @@ 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 val; + u32 rxsel = PHY_PAD_RXSEL_3V3; + + if (host->flags & SDHCI_SIGNALING_180 || priv->flags & FLAG_IO_FIXED_1V8) + rxsel = PHY_PAD_RXSEL_1V8; /* deassert phy reset & set tx drive strength */ val = PHY_CNFG_RSTN_DEASSERT; @@ -353,7 +357,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 +369,20 @@ 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); + if (host->flags & SDHCI_SIGNALING_180 || priv->flags & FLAG_IO_FIXED_1V8) + 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); - - /* 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); - /* 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 +392,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 +1118,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, };
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> --- drivers/mmc/host/sdhci-of-dwcmshc.c | 69 +++++------------------------ 1 file changed, 12 insertions(+), 57 deletions(-)