[PATCH v4] mmc: sdhci: Fix HISPD bit handling

[Jaehoon Chung]

On 6/24/20 10:36 AM, Peng Fan wrote:
> All:
> 
>> Subject: Re: [PATCH v4] mmc: sdhci: Fix HISPD bit handling
>>
>> On 6/22/20 6:26 PM, Jagan Teki wrote:
>>> On Mon, Jun 22, 2020 at 2:54 PM Jaehoon Chung
>> <jh80.chung at samsung.com> wrote:
>>>>
>>>> Hi Peng,
>>>>
>>>> On 6/22/20 10:49 AM, Peng Fan wrote:
>>>>> Jaehoon,
>>>>>
>>>>>> Subject: [PATCH v4] mmc: sdhci: Fix HISPD bit handling
>>>>>
>>>>> Are you fine with this v4?
>>>>
>>>> Thanks for sharing it. i didn't see patch v4.
>>>>
>>>>>
>>>>> Thanks,
>>>>> Peng.
>>>>>
>>>>>>
>>>>>> SDHCI HISPD bits need to be configured based on desired mmc timings
>>>>>> mode and some HISPD quirks.
>>>>>>
>>>>>> So, handle the HISPD bit based on the mmc computed selected
>>>>>> mode(timing
>>>>>> parameter) rather than fixed mmc card clock frequency.
>>>>>>
>>>>>> Linux handle the HISPD similar like this in below commit but no
>>>>>> SDHCI_QUIRK_BROKEN_HISPD_MODE,
>>>>>>
>>>>>> commit <501639bf2173> ("mmc: sdhci: fix
>> SDHCI_QUIRK_NO_HISPD_BIT
>>>>>> handling")
>>>>>>
>>>>>> This eventually fixed the mmc write issue observed in
>>>>>> rk3399 sdhci controller.
>>>>>>
>>>>>> Bug log for refernece,
>>>>>> => gpt write mmc 0 $partitions
>>>>>> Writing GPT: mmc write failed
>>>>>> ** Can't write to device 0 **
>>>>>> ** Can't write to device 0 **
>>>>>> error!
>>>>>>
>>>>>> Cc: Kever Yang <kever.yang at rock-chips.com>
>>>>>> Cc: Peng Fan <peng.fan at nxp.com>
>>>>>> Tested-by: Suniel Mahesh <sunil at amarulasolutions.com> #
>>>>>> roc-rk3399-pc
>>>>>> Signed-off-by: Jagan Teki <jagan at amarulasolutions.com>
>>>>>> ---
>>>>>> Changes for v4:
>>>>>> - update commit message
>>>>>> - simplify the logic.
>>>>>>
>>>>>>  drivers/mmc/sdhci.c | 23 +++++++++++++++++------
>>>>>>  1 file changed, 17 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c index
>>>>>> 92cc8434af..6cb702111b 100644
>>>>>> --- a/drivers/mmc/sdhci.c
>>>>>> +++ b/drivers/mmc/sdhci.c
>>>>>> @@ -567,6 +567,7 @@ static int sdhci_set_ios(struct mmc *mmc)
>> #endif
>>>>>>      u32 ctrl;
>>>>>>      struct sdhci_host *host = mmc->priv;
>>>>>> +    bool no_hispd_bit = false;
>>>>>>
>>>>>>      if (host->ops && host->ops->set_control_reg)
>>>>>>              host->ops->set_control_reg(host); @@ -594,14
>> +595,24
>>>>>> @@ static int sdhci_set_ios(struct mmc *mmc)
>>>>>>                      ctrl &= ~SDHCI_CTRL_4BITBUS;
>>>>>>      }
>>>>>>
>>>>>> -    if (mmc->clock > 26000000)
>>>>>> -            ctrl |= SDHCI_CTRL_HISPD;
>>>>>> -    else
>>>>>> -            ctrl &= ~SDHCI_CTRL_HISPD;
>>>>>> -
>>>>>>      if ((host->quirks & SDHCI_QUIRK_NO_HISPD_BIT) ||
>>>>>>          (host->quirks & SDHCI_QUIRK_BROKEN_HISPD_MODE))
>>>>>> -            ctrl &= ~SDHCI_CTRL_HISPD;
>>>>>> +            no_hispd_bit = true;
>>>>
>>>> No. ctrl variable is set to bit[2] of HOST_CONTROL register.
>>>> But Some samsung SoCs are using its bit[2] as other purpose.
>>>> So it needs to revert the value with "ctrl &= ~SDHCI_CTRL_HISPD".
>>>> Because it's possible to set to 1.
>>>>
>>>> SDHCI_QUIRK_NO_HISPD_BIT means that it's used other purpose.
>>>
>>> So, what are you suggesting? could you please elaborate?
>>
>>
>> if ((host->quirks & SDHCI_QUIRK_NO_HISPD_BIT) ||
>>     (host->quirks & SDHCI_QUIRK_NO_BROKEN_HISPD_MODE)) {
>> 	ctrl &= ~SDHCI_CTRL_HISPD;
>> 	no_hispd_bit = true;
>> }
>>
>> Just adding "ctrl &= ~SDHCI_CTRL_HISPD;" into its condition.
> 
> I pushed this patch with the upper code added to my CI
> with a update in commit log:
> https://protect2.fireeye.com/url?k=a0ad72d4-fd674763-a0acf99b-0cc47a3003e8-b7ff57d36725a01c&q=1&u=https%3A%2F%2Ftravis-ci.org%2Fgithub%2FMrVan%2Fu-boot%2Fbuilds%2F701486010
> 
> Please let me know if any objections.

Thanks!

> 
> Thanks,
> Peng.
> 
>> Then it's helpful to me.
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>>
>>> Jagan.
>>>
>