[PATCH] mmc: sdhci: Fix incorrect switch to HS mode

[PATCH] mmc: sdhci: Fix incorrect switch to HS mode

[Al Cooper]

When switching from any MMC speed mode that requires 1.8v
(HS200, HS400 and HS400ES) to High Speed (HS) mode, the system
ends up configured for SDR12 with a 50MHz clock which is an illegal
mode.

This happens because the SDHCI_CTRL_VDD_180 bit in the
SDHCI_HOST_CONTROL2 register is left set and when this bit is
set, the speed mode is controlled by the SDHCI_CTRL_UHS field
in the SDHCI_HOST_CONTROL2 register. The SDHCI_CTRL_UHS field
will end up being set to 0 (SDR12) by sdhci_set_uhs_signaling()
because there is no UHS mode being set.

The fix is to change sdhci_set_uhs_signaling() to set the
SDHCI_CTRL_UHS field to SDR25 (which is the same as HS) for
any switch to HS mode.

This was found on a new eMMC controller that does strict checking
of the speed mode and the corresponding clock rate. It caused the
switch to HS400 mode to fail because part of the sequence to switch
to HS400 requires a switch from HS200 to HS before going to HS400.

This fix was suggested by Adrian Hunter

Signed-off-by: Al Cooper <alcooperx@gmail.com>
---
 drivers/mmc/host/sdhci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 61d845fe0b97..068149640ecd 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1858,7 +1858,9 @@ void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing)
 		ctrl_2 |= SDHCI_CTRL_UHS_SDR104;
 	else if (timing == MMC_TIMING_UHS_SDR12)
 		ctrl_2 |= SDHCI_CTRL_UHS_SDR12;
-	else if (timing == MMC_TIMING_UHS_SDR25)
+	else if (timing == MMC_TIMING_SD_HS ||
+		 timing == MMC_TIMING_MMC_HS ||
+		 timing == MMC_TIMING_UHS_SDR25)
 		ctrl_2 |= SDHCI_CTRL_UHS_SDR25;
 	else if (timing == MMC_TIMING_UHS_SDR50)
 		ctrl_2 |= SDHCI_CTRL_UHS_SDR50;
-- 
2.17.1

Re: [PATCH] mmc: sdhci: Fix incorrect switch to HS mode

[Ulf Hansson]

On Tue, 3 Sep 2019 at 13:51, Al Cooper <alcooperx@gmail.com> wrote:
>
> When switching from any MMC speed mode that requires 1.8v
> (HS200, HS400 and HS400ES) to High Speed (HS) mode, the system
> ends up configured for SDR12 with a 50MHz clock which is an illegal
> mode.
>
> This happens because the SDHCI_CTRL_VDD_180 bit in the
> SDHCI_HOST_CONTROL2 register is left set and when this bit is
> set, the speed mode is controlled by the SDHCI_CTRL_UHS field
> in the SDHCI_HOST_CONTROL2 register. The SDHCI_CTRL_UHS field
> will end up being set to 0 (SDR12) by sdhci_set_uhs_signaling()
> because there is no UHS mode being set.
>
> The fix is to change sdhci_set_uhs_signaling() to set the
> SDHCI_CTRL_UHS field to SDR25 (which is the same as HS) for
> any switch to HS mode.
>
> This was found on a new eMMC controller that does strict checking
> of the speed mode and the corresponding clock rate. It caused the
> switch to HS400 mode to fail because part of the sequence to switch
> to HS400 requires a switch from HS200 to HS before going to HS400.
>
> This fix was suggested by Adrian Hunter
>
> Signed-off-by: Al Cooper <alcooperx@gmail.com>

Should this be applied for fixes and tagged for stable you think?

Kind regards
Uffe

> ---
>  drivers/mmc/host/sdhci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 61d845fe0b97..068149640ecd 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1858,7 +1858,9 @@ void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing)
>                 ctrl_2 |= SDHCI_CTRL_UHS_SDR104;
>         else if (timing == MMC_TIMING_UHS_SDR12)
>                 ctrl_2 |= SDHCI_CTRL_UHS_SDR12;
> -       else if (timing == MMC_TIMING_UHS_SDR25)
> +       else if (timing == MMC_TIMING_SD_HS ||
> +                timing == MMC_TIMING_MMC_HS ||
> +                timing == MMC_TIMING_UHS_SDR25)
>                 ctrl_2 |= SDHCI_CTRL_UHS_SDR25;
>         else if (timing == MMC_TIMING_UHS_SDR50)
>                 ctrl_2 |= SDHCI_CTRL_UHS_SDR50;
> --
> 2.17.1
>

Re: [PATCH] mmc: sdhci: Fix incorrect switch to HS mode

[Alan Cooper]

This does correct the sequence of switching to HS400 but it might be
safest to just add this to the latest until it gets a little testing
to make sure it doesn't expose some bug in existing controllers.

Thanks
Al

On Tue, Sep 3, 2019 at 10:52 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Tue, 3 Sep 2019 at 13:51, Al Cooper <alcooperx@gmail.com> wrote:
> >
> > When switching from any MMC speed mode that requires 1.8v
> > (HS200, HS400 and HS400ES) to High Speed (HS) mode, the system
> > ends up configured for SDR12 with a 50MHz clock which is an illegal
> > mode.
> >
> > This happens because the SDHCI_CTRL_VDD_180 bit in the
> > SDHCI_HOST_CONTROL2 register is left set and when this bit is
> > set, the speed mode is controlled by the SDHCI_CTRL_UHS field
> > in the SDHCI_HOST_CONTROL2 register. The SDHCI_CTRL_UHS field
> > will end up being set to 0 (SDR12) by sdhci_set_uhs_signaling()
> > because there is no UHS mode being set.
> >
> > The fix is to change sdhci_set_uhs_signaling() to set the
> > SDHCI_CTRL_UHS field to SDR25 (which is the same as HS) for
> > any switch to HS mode.
> >
> > This was found on a new eMMC controller that does strict checking
> > of the speed mode and the corresponding clock rate. It caused the
> > switch to HS400 mode to fail because part of the sequence to switch
> > to HS400 requires a switch from HS200 to HS before going to HS400.
> >
> > This fix was suggested by Adrian Hunter
> >
> > Signed-off-by: Al Cooper <alcooperx@gmail.com>
>
> Should this be applied for fixes and tagged for stable you think?
>
> Kind regards
> Uffe
>
> > ---
> >  drivers/mmc/host/sdhci.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index 61d845fe0b97..068149640ecd 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -1858,7 +1858,9 @@ void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing)
> >                 ctrl_2 |= SDHCI_CTRL_UHS_SDR104;
> >         else if (timing == MMC_TIMING_UHS_SDR12)
> >                 ctrl_2 |= SDHCI_CTRL_UHS_SDR12;
> > -       else if (timing == MMC_TIMING_UHS_SDR25)
> > +       else if (timing == MMC_TIMING_SD_HS ||
> > +                timing == MMC_TIMING_MMC_HS ||
> > +                timing == MMC_TIMING_UHS_SDR25)
> >                 ctrl_2 |= SDHCI_CTRL_UHS_SDR25;
> >         else if (timing == MMC_TIMING_UHS_SDR50)
> >                 ctrl_2 |= SDHCI_CTRL_UHS_SDR50;
> > --
> > 2.17.1
> >

Re: [PATCH] mmc: sdhci: Fix incorrect switch to HS mode

[Faiz Abbas]

Hi,

On 19/09/19 5:27 PM, Alan Cooper wrote:
> This does correct the sequence of switching to HS400 but it might be
> safest to just add this to the latest until it gets a little testing
> to make sure it doesn't expose some bug in existing controllers.
> 
> Thanks
> Al
> 
> On Tue, Sep 3, 2019 at 10:52 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>
>> On Tue, 3 Sep 2019 at 13:51, Al Cooper <alcooperx@gmail.com> wrote:
>>>
>>> When switching from any MMC speed mode that requires 1.8v
>>> (HS200, HS400 and HS400ES) to High Speed (HS) mode, the system
>>> ends up configured for SDR12 with a 50MHz clock which is an illegal
>>> mode.
>>>
>>> This happens because the SDHCI_CTRL_VDD_180 bit in the
>>> SDHCI_HOST_CONTROL2 register is left set and when this bit is
>>> set, the speed mode is controlled by the SDHCI_CTRL_UHS field
>>> in the SDHCI_HOST_CONTROL2 register. The SDHCI_CTRL_UHS field
>>> will end up being set to 0 (SDR12) by sdhci_set_uhs_signaling()
>>> because there is no UHS mode being set.
>>>
>>> The fix is to change sdhci_set_uhs_signaling() to set the
>>> SDHCI_CTRL_UHS field to SDR25 (which is the same as HS) for
>>> any switch to HS mode.

This change has broken High speed mode in SD card for me in AM65x-evm. I
guess this change only needs to be done for eMMC. SDR25 is decidedly not
the same as high speed for SD card.

Thanks,
Faiz

Re: [PATCH] mmc: sdhci: Fix incorrect switch to HS mode

[Adrian Hunter]

On 28/11/19 10:21 AM, Faiz Abbas wrote:
> Hi,
> 
> On 19/09/19 5:27 PM, Alan Cooper wrote:
>> This does correct the sequence of switching to HS400 but it might be
>> safest to just add this to the latest until it gets a little testing
>> to make sure it doesn't expose some bug in existing controllers.
>>
>> Thanks
>> Al
>>
>> On Tue, Sep 3, 2019 at 10:52 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>
>>> On Tue, 3 Sep 2019 at 13:51, Al Cooper <alcooperx@gmail.com> wrote:
>>>>
>>>> When switching from any MMC speed mode that requires 1.8v
>>>> (HS200, HS400 and HS400ES) to High Speed (HS) mode, the system
>>>> ends up configured for SDR12 with a 50MHz clock which is an illegal
>>>> mode.
>>>>
>>>> This happens because the SDHCI_CTRL_VDD_180 bit in the
>>>> SDHCI_HOST_CONTROL2 register is left set and when this bit is
>>>> set, the speed mode is controlled by the SDHCI_CTRL_UHS field
>>>> in the SDHCI_HOST_CONTROL2 register. The SDHCI_CTRL_UHS field
>>>> will end up being set to 0 (SDR12) by sdhci_set_uhs_signaling()
>>>> because there is no UHS mode being set.
>>>>
>>>> The fix is to change sdhci_set_uhs_signaling() to set the
>>>> SDHCI_CTRL_UHS field to SDR25 (which is the same as HS) for
>>>> any switch to HS mode.
> 
> This change has broken High speed mode in SD card for me in AM65x-evm. I
> guess this change only needs to be done for eMMC. SDR25 is decidedly not
> the same as high speed for SD card.

Shall we revert c894e33ddc1910e14d6f2a2016f60ab613fd8b37 and then Alan
could send a patch providing the desired behaviour in ->set_uhs_signaling()
of the relevant driver e.g.

void ???_set_uhs_signaling(struct sdhci_host *host, unsigned int timing)
{
	if (timing == MMC_TIMING_SD_HS || timing == MMC_TIMING_MMC_HS)
		timing = MMC_TIMING_UHS_SDR25;
	sdhci_set_uhs_signaling(host, timing);
}

Re: [PATCH] mmc: sdhci: Fix incorrect switch to HS mode

[Faiz Abbas]

Adrian,

On 28/11/19 2:56 PM, Adrian Hunter wrote:
> On 28/11/19 10:21 AM, Faiz Abbas wrote:
>> Hi,
>>
>> On 19/09/19 5:27 PM, Alan Cooper wrote:
>>> This does correct the sequence of switching to HS400 but it might be
>>> safest to just add this to the latest until it gets a little testing
>>> to make sure it doesn't expose some bug in existing controllers.
>>>
>>> Thanks
>>> Al
>>>
>>> On Tue, Sep 3, 2019 at 10:52 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>
>>>> On Tue, 3 Sep 2019 at 13:51, Al Cooper <alcooperx@gmail.com> wrote:
>>>>>
>>>>> When switching from any MMC speed mode that requires 1.8v
>>>>> (HS200, HS400 and HS400ES) to High Speed (HS) mode, the system
>>>>> ends up configured for SDR12 with a 50MHz clock which is an illegal
>>>>> mode.
>>>>>
>>>>> This happens because the SDHCI_CTRL_VDD_180 bit in the
>>>>> SDHCI_HOST_CONTROL2 register is left set and when this bit is
>>>>> set, the speed mode is controlled by the SDHCI_CTRL_UHS field
>>>>> in the SDHCI_HOST_CONTROL2 register. The SDHCI_CTRL_UHS field
>>>>> will end up being set to 0 (SDR12) by sdhci_set_uhs_signaling()
>>>>> because there is no UHS mode being set.
>>>>>
>>>>> The fix is to change sdhci_set_uhs_signaling() to set the
>>>>> SDHCI_CTRL_UHS field to SDR25 (which is the same as HS) for
>>>>> any switch to HS mode.
>>
>> This change has broken High speed mode in SD card for me in AM65x-evm. I
>> guess this change only needs to be done for eMMC. SDR25 is decidedly not
>> the same as high speed for SD card.
> 
> Shall we revert c894e33ddc1910e14d6f2a2016f60ab613fd8b37 and then Alan
> could send a patch providing the desired behaviour in ->set_uhs_signaling()
> of the relevant driver e.g.
> 
> void ???_set_uhs_signaling(struct sdhci_host *host, unsigned int timing)
> {
> 	if (timing == MMC_TIMING_SD_HS || timing == MMC_TIMING_MMC_HS)
> 		timing = MMC_TIMING_UHS_SDR25;
> 	sdhci_set_uhs_signaling(host, timing);
> }
> 
Yes. Lets do that. I will send the revert.

Thanks,
Faiz