| Message ID | 20200610114347.118501-1-jagan@amarulasolutions.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] mmc: sdhci: Fix HISPD bit handling | expand |
Hi Jagan, On 6/10/20 8:43 PM, Jagan Teki wrote: > 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, > > 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: Robin Murphy <robin.murphy@arm.com> > Cc: Kever Yang <kever.yang@rock-chips.com> > Cc: Peng Fan <peng.fan@nxp.com> > Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com> > Tested-by: Marc Zyngier <maz@kernel.org> # nanopc-t4 > Tested-by: Suniel Mahesh <sunil@amarulasolutions.com> # roc-rk3399-pc > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > --- > Changes for v3: > - use && for quirk check. > > drivers/mmc/sdhci.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c > index 92cc8434af..a7db278a0e 100644 > --- a/drivers/mmc/sdhci.c > +++ b/drivers/mmc/sdhci.c > @@ -594,14 +594,21 @@ 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; > + if (!(host->quirks & SDHCI_QUIRK_NO_HISPD_BIT) && > + !(host->quirks & SDHCI_QUIRK_BROKEN_HISPD_MODE)) { Sorry, it needs to check more. Because it's different with kernel code. Kernel code is only checking condition about SDHCI_QUIRK_NO_HISPD_BIT. but in u-boot, one more checking about SDHCI_QUIRK_BROKEN_HISPD_MODE. So if it doesn't set to both, it's possible to set SDHCI_CTRL_HISPD as ctrl's flags. > + if (mmc->selected_mode == MMC_HS || > + mmc->selected_mode == SD_HS || > + mmc->selected_mode == MMC_DDR_52 || > + mmc->selected_mode == MMC_HS_200 || > + mmc->selected_mode == MMC_HS_400 || > + mmc->selected_mode == UHS_SDR25 || > + mmc->selected_mode == UHS_SDR50 || > + mmc->selected_mode == UHS_SDR104 || > + mmc->selected_mode == UHS_DDR50) > + ctrl |= SDHCI_CTRL_HISPD; > + else > + ctrl &= ~SDHCI_CTRL_HISPD; > + } > I think that needs to add at here else ctrl &= ~SDHCI_CTRL_HISPD; > sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL); > >
On Wed, Jun 10, 2020 at 5:36 PM Jaehoon Chung <jh80.chung@samsung.com> wrote: > > Hi Jagan, > > On 6/10/20 8:43 PM, Jagan Teki wrote: > > 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, > > > > 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: Robin Murphy <robin.murphy@arm.com> > > Cc: Kever Yang <kever.yang@rock-chips.com> > > Cc: Peng Fan <peng.fan@nxp.com> > > Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com> > > Tested-by: Marc Zyngier <maz@kernel.org> # nanopc-t4 > > Tested-by: Suniel Mahesh <sunil@amarulasolutions.com> # roc-rk3399-pc > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > --- > > Changes for v3: > > - use && for quirk check. > > > > drivers/mmc/sdhci.c | 23 +++++++++++++++-------- > > 1 file changed, 15 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c > > index 92cc8434af..a7db278a0e 100644 > > --- a/drivers/mmc/sdhci.c > > +++ b/drivers/mmc/sdhci.c > > @@ -594,14 +594,21 @@ 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; > > + if (!(host->quirks & SDHCI_QUIRK_NO_HISPD_BIT) && > > + !(host->quirks & SDHCI_QUIRK_BROKEN_HISPD_MODE)) { > > Sorry, it needs to check more. Because it's different with kernel code. > Kernel code is only checking condition about SDHCI_QUIRK_NO_HISPD_BIT. > but in u-boot, one more checking about SDHCI_QUIRK_BROKEN_HISPD_MODE. > > So if it doesn't set to both, it's possible to set SDHCI_CTRL_HISPD as ctrl's flags. > > > + if (mmc->selected_mode == MMC_HS || > > + mmc->selected_mode == SD_HS || > > + mmc->selected_mode == MMC_DDR_52 || > > + mmc->selected_mode == MMC_HS_200 || > > + mmc->selected_mode == MMC_HS_400 || > > + mmc->selected_mode == UHS_SDR25 || > > + mmc->selected_mode == UHS_SDR50 || > > + mmc->selected_mode == UHS_SDR104 || > > + mmc->selected_mode == UHS_DDR50) > > + ctrl |= SDHCI_CTRL_HISPD; > > + else > > + ctrl &= ~SDHCI_CTRL_HISPD; > > + } > > > I think that needs to add at here > else > ctrl &= ~SDHCI_CTRL_HISPD; Okay, Can you look at this logic? --- 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,13 +595,16 @@ 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)) + no_hispd_bit = true; + + if (((mmc->selected_mode != MMC_LEGACY) || + (mmc->selected_mode != MMC_HS_52) || + (mmc->selected_mode != UHS_SDR12) || + (mmc->selected_mode != MMC_HS_400_ES)) && !no_hispd_bit) + ctrl |= SDHCI_CTRL_HISPD; + else ctrl &= ~SDHCI_CTRL_HISPD; sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c index 92cc8434af..a7db278a0e 100644 --- a/drivers/mmc/sdhci.c +++ b/drivers/mmc/sdhci.c @@ -594,14 +594,21 @@ 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; + if (!(host->quirks & SDHCI_QUIRK_NO_HISPD_BIT) && + !(host->quirks & SDHCI_QUIRK_BROKEN_HISPD_MODE)) { + if (mmc->selected_mode == MMC_HS || + mmc->selected_mode == SD_HS || + mmc->selected_mode == MMC_DDR_52 || + mmc->selected_mode == MMC_HS_200 || + mmc->selected_mode == MMC_HS_400 || + mmc->selected_mode == UHS_SDR25 || + mmc->selected_mode == UHS_SDR50 || + mmc->selected_mode == UHS_SDR104 || + mmc->selected_mode == UHS_DDR50) + ctrl |= SDHCI_CTRL_HISPD; + else + ctrl &= ~SDHCI_CTRL_HISPD; + } sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
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, 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: Robin Murphy <robin.murphy@arm.com> Cc: Kever Yang <kever.yang@rock-chips.com> Cc: Peng Fan <peng.fan@nxp.com> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com> Tested-by: Marc Zyngier <maz@kernel.org> # nanopc-t4 Tested-by: Suniel Mahesh <sunil@amarulasolutions.com> # roc-rk3399-pc Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> --- Changes for v3: - use && for quirk check. drivers/mmc/sdhci.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)