diff mbox series

[v2] mmc: sdhci: Fix HISPD bit handling

Message ID 20200609140135.131887-1-jagan@amarulasolutions.com
State New
Headers
Series [v2] mmc: sdhci: Fix HISPD bit handling |

Commit Message

Jagan Teki June 9, 2020, 2:01 p.m. UTC 
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: Kever Yang <kever.yang@rock-chips.com>
Cc: Peng Fan <peng.fan@nxp.com>
Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v2:
- collect Jaehoon R-b

 drivers/mmc/sdhci.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

Comments

Robin Murphy June 9, 2020, 2:38 p.m. UTC | #1 
On 2020-06-09 15:01, 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: Kever Yang <kever.yang@rock-chips.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> Changes for v2:
> - collect Jaehoon R-b
> 
>   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..280b8c88eb 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) ||

Should that be "&&" rather than "||"? Otherwise this will always 
evaluate to true unless *both* quirks are set, which isn't equivalent to 
the check being removed above.

Robin.

> +	    !(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);
>   
>
Jaehoon Chung June 10, 2020, 3:45 a.m. UTC | #2 
On 6/9/20 11:38 PM, Robin Murphy wrote:
> On 2020-06-09 15:01, 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: Kever Yang <kever.yang@rock-chips.com>
>> Cc: Peng Fan <peng.fan@nxp.com>
>> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>> ---
>> Changes for v2:
>> - collect Jaehoon R-b
>>
>>   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..280b8c88eb 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) ||
> 
> Should that be "&&" rather than "||"? Otherwise this will always evaluate to true unless *both* quirks are set, which isn't equivalent to the check being removed above.


You're right.

> 
> Robin.
> 
>> +        !(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);
>>  
>
Marc Zyngier June 10, 2020, 10:37 a.m. UTC | #3 
On Wed, 10 Jun 2020 12:45:33 +0900
Jaehoon Chung <jh80.chung@samsung.com> wrote:

> On 6/9/20 11:38 PM, Robin Murphy wrote:
> > On 2020-06-09 15:01, 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: Kever Yang <kever.yang@rock-chips.com>
> >> Cc: Peng Fan <peng.fan@nxp.com>
> >> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
> >> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> >> ---
> >> Changes for v2:
> >> - collect Jaehoon R-b
> >>
> >>   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..280b8c88eb 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) ||  
> > 
> > Should that be "&&" rather than "||"? Otherwise this will always
> > evaluate to true unless *both* quirks are set, which isn't
> > equivalent to the check being removed above.  
> 
> 
> You're right.

It'd be great if you could respin this patch quickly and get it merged,
as it just helped me getting my NanoPC-T4 up and running.

FWIW:

Tested-by: Marc Zyngier <maz@kernel.org>

	M.
Jagan Teki June 10, 2020, 11:44 a.m. UTC | #4 
On Tue, Jun 9, 2020 at 8:08 PM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2020-06-09 15:01, 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: Kever Yang <kever.yang@rock-chips.com>
> > Cc: Peng Fan <peng.fan@nxp.com>
> > Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> > Changes for v2:
> > - collect Jaehoon R-b
> >
> >   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..280b8c88eb 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) ||
>
> Should that be "&&" rather than "||"? Otherwise this will always
> evaluate to true unless *both* quirks are set, which isn't equivalent to
> the check being removed above.

Correct, thanks for the catch. I have updated ib v3.

Jagan.
diff mbox series

Patch

diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
index 92cc8434af..280b8c88eb 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);