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

[Jagan Teki]

On Wed, Jun 24, 2020 at 7:06 AM Peng Fan <peng.fan at nxp.com> 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://travis-ci.org/github/MrVan/u-boot/builds/701486010
>
> Please let me know if any objections.

Thanks for taking care of this.

Jagan.