Hi Adrian,
> -----Original Message-----
> From: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> Sent: Monday, December 9, 2024 5:14 PM
> To: Jaehoon Chung <jh80.chung@xxxxxxxxxxx>; linux-mmc@xxxxxxxxxxxxxxx
> Cc: ulf.hansson@xxxxxxxxxx
> 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@xxxxxxxxx>
> >> 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@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?
> >
> > 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,
> >>> };
> >>>
> >
> >
> >
