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@xxxxxxxxxxx>
> ---
> 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,
> };
>
