On Wed, Jun 24, 2020 at 7:06 AM Peng Fan <peng....@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.ch...@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.y...@rock-chips.com> > > >>>> Cc: Peng Fan <peng....@nxp.com> > > >>>> Tested-by: Suniel Mahesh <su...@amarulasolutions.com> # > > >>>> roc-rk3399-pc > > >>>> Signed-off-by: Jagan Teki <ja...@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.