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?
>
> 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,
>>> };
>>>
>
>
>
