Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1408550 > unrolled thread

[PATCH 1/3] mmc: fix mmc mode selection for HS-DDR and higher

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/3] mmc: fix mmc mode selection for HS-DDR and higher Chen-Yu Tsai <wens@csie.org> - 2016-05-29 09:10 +0200
    Re: [PATCH 1/3] mmc: fix mmc mode selection for HS-DDR and higher Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-05-31 11:40 +0200
    Re: [PATCH 1/3] mmc: fix mmc mode selection for HS-DDR and higher Jaehoon Chung <jh80.chung@samsung.com> - 2016-06-01 03:30 +0200
    Re: [PATCH 1/3] mmc: fix mmc mode selection for HS-DDR and higher Shawn Lin <shawn.lin@rock-chips.com> - 2016-06-01 04:40 +0200
    Re: [PATCH 1/3] mmc: fix mmc mode selection for HS-DDR and higher Marcel Ziswiler <marcel@ziswiler.com> - 2016-06-01 11:30 +0200
    Re: [PATCH 1/3] mmc: fix mmc mode selection for HS-DDR and higher Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-06-01 21:10 +0200
      Re: [PATCH 1/3] mmc: fix mmc mode selection for HS-DDR and higher Chen-Yu Tsai <wens@csie.org> - 2016-06-02 10:10 +0200
    Re: [PATCH 1/3] mmc: fix mmc mode selection for HS-DDR and higher Ulf Hansson <ulf.hansson@linaro.org> - 2016-06-02 10:40 +0200
      Re: [PATCH 1/3] mmc: fix mmc mode selection for HS-DDR and higher Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-06-02 11:40 +0200
        Re: [PATCH 1/3] mmc: fix mmc mode selection for HS-DDR and higher Ulf Hansson <ulf.hansson@linaro.org> - 2016-06-02 17:10 +0200

#1408550 — [PATCH 1/3] mmc: fix mmc mode selection for HS-DDR and higher

When IS_ERR_VALUE was removed from the mmc core code, it was replaced
with a simple not-zero check. This does not work, as the value checked
is the return value for mmc_select_bus_width, which returns the set
bit width on success. This made eMMC modes higher than HS-DDR unusable.

Fix this by checking for a positive return value instead.

Fixes: 287980e49ffc ("remove lots of IS_ERR_VALUE abuses")
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/mmc/core/mmc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index c984321d1881..aafb73d080ca 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1276,7 +1276,7 @@ static int mmc_select_hs200(struct mmc_card *card)
 	 * switch to HS200 mode if bus width is set successfully.
 	 */
 	err = mmc_select_bus_width(card);
-	if (!err) {
+	if (err > 0) {
 		val = EXT_CSD_TIMING_HS200 |
 		      card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
 		err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
@@ -1583,7 +1583,7 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
 	} else if (mmc_card_hs(card)) {
 		/* Select the desired bus width optionally */
 		err = mmc_select_bus_width(card);
-		if (!err) {
+		if (err > 0) {
 			err = mmc_select_hs_ddr(card);
 			if (err)
 				goto free_card;
-- 
2.8.1

[toc] | [next] | [standalone]

#1410038

On Sun, May 29, 2016 at 9:04 AM, Chen-Yu Tsai <wens@csie.org> wrote:
> When IS_ERR_VALUE was removed from the mmc core code, it was replaced
> with a simple not-zero check. This does not work, as the value checked
> is the return value for mmc_select_bus_width, which returns the set
> bit width on success. This made eMMC modes higher than HS-DDR unusable.
>
> Fix this by checking for a positive return value instead.
>
> Fixes: 287980e49ffc ("remove lots of IS_ERR_VALUE abuses")
> Cc: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
>  drivers/mmc/core/mmc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>

Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Could you be so kind and pick it up as fast as possible for current
RC? Half of my boards fail (because they work on eMMC) so testing
patches before applying is limited.

Best regards,
Krzysztof

[toc] | [prev] | [next] | [standalone]

#1410727

On 05/29/2016 04:04 PM, Chen-Yu Tsai wrote:
> When IS_ERR_VALUE was removed from the mmc core code, it was replaced
> with a simple not-zero check. This does not work, as the value checked
> is the return value for mmc_select_bus_width, which returns the set
> bit width on success. This made eMMC modes higher than HS-DDR unusable.
> 
> Fix this by checking for a positive return value instead.
> 
> Fixes: 287980e49ffc ("remove lots of IS_ERR_VALUE abuses")
> Cc: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Acked-by: Jaehoon Chung <jh80.chung@samsung.com>

Best Regards,
Jaehoon Chung

> ---
>  drivers/mmc/core/mmc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index c984321d1881..aafb73d080ca 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1276,7 +1276,7 @@ static int mmc_select_hs200(struct mmc_card *card)
>  	 * switch to HS200 mode if bus width is set successfully.
>  	 */
>  	err = mmc_select_bus_width(card);
> -	if (!err) {
> +	if (err > 0) {
>  		val = EXT_CSD_TIMING_HS200 |
>  		      card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
>  		err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> @@ -1583,7 +1583,7 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
>  	} else if (mmc_card_hs(card)) {
>  		/* Select the desired bus width optionally */
>  		err = mmc_select_bus_width(card);
> -		if (!err) {
> +		if (err > 0) {
>  			err = mmc_select_hs_ddr(card);
>  			if (err)
>  				goto free_card;
> 

[toc] | [prev] | [next] | [standalone]

#1410765

On 2016/5/29 15:04, Chen-Yu Tsai wrote:
> When IS_ERR_VALUE was removed from the mmc core code, it was replaced
> with a simple not-zero check. This does not work, as the value checked
> is the return value for mmc_select_bus_width, which returns the set
> bit width on success. This made eMMC modes higher than HS-DDR unusable.
>
> Fix this by checking for a positive return value instead.

Reviewed-by: Shawn Lin <shawn.lin@rock-chips.com>

>
> Fixes: 287980e49ffc ("remove lots of IS_ERR_VALUE abuses")
> Cc: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
>  drivers/mmc/core/mmc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index c984321d1881..aafb73d080ca 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1276,7 +1276,7 @@ static int mmc_select_hs200(struct mmc_card *card)
>  	 * switch to HS200 mode if bus width is set successfully.
>  	 */
>  	err = mmc_select_bus_width(card);
> -	if (!err) {
> +	if (err > 0) {
>  		val = EXT_CSD_TIMING_HS200 |
>  		      card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
>  		err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> @@ -1583,7 +1583,7 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
>  	} else if (mmc_card_hs(card)) {
>  		/* Select the desired bus width optionally */
>  		err = mmc_select_bus_width(card);
> -		if (!err) {
> +		if (err > 0) {
>  			err = mmc_select_hs_ddr(card);
>  			if (err)
>  				goto free_card;
>


-- 
Best Regards
Shawn Lin

[toc] | [prev] | [next] | [standalone]

#1411018

On Sun, 2016-05-29 at 15:04 +0800, Chen-Yu Tsai wrote:
> When IS_ERR_VALUE was removed from the mmc core code, it was replaced
> with a simple not-zero check. This does not work, as the value
> checked
> is the return value for mmc_select_bus_width, which returns the set
> bit width on success. This made eMMC modes higher than HS-DDR
> unusable.
> 
> Fix this by checking for a positive return value instead.
> 
> Fixes: 287980e49ffc ("remove lots of IS_ERR_VALUE abuses")
> Cc: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

This fixes the following eMMC issue as experienced on Apalis T30 as
well:

[    7.194625] mmc1: mmc_select_hs200 failed, error 3
[    7.201549] mmc1: error 3 whilst initialising MMC card

Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

> ---
>  drivers/mmc/core/mmc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index c984321d1881..aafb73d080ca 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1276,7 +1276,7 @@ static int mmc_select_hs200(struct mmc_card
> *card)
>  	 * switch to HS200 mode if bus width is set successfully.
>  	 */
>  	err = mmc_select_bus_width(card);
> -	if (!err) {
> +	if (err > 0) {
>  		val = EXT_CSD_TIMING_HS200 |
>  		      card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
>  		err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> @@ -1583,7 +1583,7 @@ static int mmc_init_card(struct mmc_host *host,
> u32 ocr,
>  	} else if (mmc_card_hs(card)) {
>  		/* Select the desired bus width optionally */
>  		err = mmc_select_bus_width(card);
> -		if (!err) {
> +		if (err > 0) {
>  			err = mmc_select_hs_ddr(card);
>  			if (err)
>  				goto free_card;

[toc] | [prev] | [next] | [standalone]

#1411481

On Sun, May 29, 2016 at 12:04 AM, Chen-Yu Tsai <wens@csie.org> wrote:
> When IS_ERR_VALUE was removed from the mmc core code, it was replaced
> with a simple not-zero check. This does not work, as the value checked
> is the return value for mmc_select_bus_width, which returns the set
> bit width on success. This made eMMC modes higher than HS-DDR unusable.
>
> Fix this by checking for a positive return value instead.

mmc_select_bus_width() can return 0 on "success" as well and the
previous check was !IS_ERR_VALUE(err), which coverts that. So I
believe these checks should be for err >= 0 rather than just > 0.


Either way this fixes the boot failures seen on my Qualcomm based
boards with v4.7-rc1.

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1411977

On Thu, Jun 2, 2016 at 2:58 AM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> On Sun, May 29, 2016 at 12:04 AM, Chen-Yu Tsai <wens@csie.org> wrote:
>> When IS_ERR_VALUE was removed from the mmc core code, it was replaced
>> with a simple not-zero check. This does not work, as the value checked
>> is the return value for mmc_select_bus_width, which returns the set
>> bit width on success. This made eMMC modes higher than HS-DDR unusable.
>>
>> Fix this by checking for a positive return value instead.
>
> mmc_select_bus_width() can return 0 on "success" as well and the
> previous check was !IS_ERR_VALUE(err), which coverts that. So I
> believe these checks should be for err >= 0 rather than just > 0.

From the comments above the function:
"Zero is returned instead of error value if the wide width is not supported."

The documents I found, which were more vendor datasheets, only list
bit widths 4 and 8 for high speed SDR/DDR and HS200.

Not sure what the MMC spec actually says though, as I do not have
it.

Regards
ChenYu

>
>
> Either way this fixes the boot failures seen on my Qualcomm based
> boards with v4.7-rc1.
>
> Regards,
> Bjorn

[toc] | [prev] | [next] | [standalone]

#1412000

+ Linus

On 29 May 2016 at 09:04, Chen-Yu Tsai <wens@csie.org> wrote:
> When IS_ERR_VALUE was removed from the mmc core code, it was replaced
> with a simple not-zero check. This does not work, as the value checked
> is the return value for mmc_select_bus_width, which returns the set
> bit width on success. This made eMMC modes higher than HS-DDR unusable.
>
> Fix this by checking for a positive return value instead.
>
> Fixes: 287980e49ffc ("remove lots of IS_ERR_VALUE abuses")
> Cc: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
>  drivers/mmc/core/mmc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index c984321d1881..aafb73d080ca 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1276,7 +1276,7 @@ static int mmc_select_hs200(struct mmc_card *card)
>          * switch to HS200 mode if bus width is set successfully.
>          */
>         err = mmc_select_bus_width(card);
> -       if (!err) {
> +       if (err > 0) {
>                 val = EXT_CSD_TIMING_HS200 |
>                       card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
>                 err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> @@ -1583,7 +1583,7 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
>         } else if (mmc_card_hs(card)) {
>                 /* Select the desired bus width optionally */
>                 err = mmc_select_bus_width(card);
> -               if (!err) {
> +               if (err > 0) {

As pointed out in the review by Björn, to restore the old behaviour we
should check for "err >= 0".
No need to send a new patch, I can amend the current version.

>                         err = mmc_select_hs_ddr(card);
>                         if (err)
>                                 goto free_card;
> --
> 2.8.1
>

Finally, I am a little concerned about the commit 287980e49ffc
("remove lots of IS_ERR_VALUE abuses") which introduced this
regression.

Surprisingly the IS_ERR_VALUE():s aren't being replaced by equivalent
checks, so perhaps there a more regressions. Moreover, I wonder why I
wasn't being on cc/to list when this patch was submitted a few days
ago, perhaps my review could prevented the regression from even
happen.

Anyway, let's fix this now! I will pick up $subject patch as fix asap...

and Arnd, can you please double-check that the commit 287980e49ffc
doesn’t seems to regress anything else!?

Kind regards
Uffe

[toc] | [prev] | [next] | [standalone]

#1412052

On Thu, Jun 2, 2016 at 10:31 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> + Linus
>
> On 29 May 2016 at 09:04, Chen-Yu Tsai <wens@csie.org> wrote:
>> When IS_ERR_VALUE was removed from the mmc core code, it was replaced
>> with a simple not-zero check. This does not work, as the value checked
>> is the return value for mmc_select_bus_width, which returns the set
>> bit width on success. This made eMMC modes higher than HS-DDR unusable.
>>
>> Fix this by checking for a positive return value instead.
>>
>> Fixes: 287980e49ffc ("remove lots of IS_ERR_VALUE abuses")
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>> ---
>>  drivers/mmc/core/mmc.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index c984321d1881..aafb73d080ca 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1276,7 +1276,7 @@ static int mmc_select_hs200(struct mmc_card *card)
>>          * switch to HS200 mode if bus width is set successfully.
>>          */
>>         err = mmc_select_bus_width(card);
>> -       if (!err) {
>> +       if (err > 0) {
>>                 val = EXT_CSD_TIMING_HS200 |
>>                       card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
>>                 err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>> @@ -1583,7 +1583,7 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
>>         } else if (mmc_card_hs(card)) {
>>                 /* Select the desired bus width optionally */
>>                 err = mmc_select_bus_width(card);
>> -               if (!err) {
>> +               if (err > 0) {
>
> As pointed out in the review by Björn, to restore the old behaviour we
> should check for "err >= 0".
> No need to send a new patch, I can amend the current version.
>
>>                         err = mmc_select_hs_ddr(card);
>>                         if (err)
>>                                 goto free_card;
>> --
>> 2.8.1
>>
>
> Finally, I am a little concerned about the commit 287980e49ffc
> ("remove lots of IS_ERR_VALUE abuses") which introduced this
> regression.
>
> Surprisingly the IS_ERR_VALUE():s aren't being replaced by equivalent
> checks, so perhaps there a more regressions. Moreover, I wonder why I
> wasn't being on cc/to list when this patch was submitted a few days
> ago, perhaps my review could prevented the regression from even
> happen.
>
> Anyway, let's fix this now! I will pick up $subject patch as fix asap...
>
> and Arnd, can you please double-check that the commit 287980e49ffc
> doesn’t seems to regress anything else!?

If only the 287980e49ffc could sit in linux-next for few days before
reaching v4.7-rc1... Could you please pick up the fix soon? Maybe
directly by Linus?

Best regards,
Krzysztof

[toc] | [prev] | [next] | [standalone]

#1412337

On 2 June 2016 at 11:35, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
> On Thu, Jun 2, 2016 at 10:31 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> + Linus
>>
>> On 29 May 2016 at 09:04, Chen-Yu Tsai <wens@csie.org> wrote:
>>> When IS_ERR_VALUE was removed from the mmc core code, it was replaced
>>> with a simple not-zero check. This does not work, as the value checked
>>> is the return value for mmc_select_bus_width, which returns the set
>>> bit width on success. This made eMMC modes higher than HS-DDR unusable.
>>>
>>> Fix this by checking for a positive return value instead.
>>>
>>> Fixes: 287980e49ffc ("remove lots of IS_ERR_VALUE abuses")
>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>>> ---
>>>  drivers/mmc/core/mmc.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>> index c984321d1881..aafb73d080ca 100644
>>> --- a/drivers/mmc/core/mmc.c
>>> +++ b/drivers/mmc/core/mmc.c
>>> @@ -1276,7 +1276,7 @@ static int mmc_select_hs200(struct mmc_card *card)
>>>          * switch to HS200 mode if bus width is set successfully.
>>>          */
>>>         err = mmc_select_bus_width(card);
>>> -       if (!err) {
>>> +       if (err > 0) {
>>>                 val = EXT_CSD_TIMING_HS200 |
>>>                       card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
>>>                 err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>>> @@ -1583,7 +1583,7 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
>>>         } else if (mmc_card_hs(card)) {
>>>                 /* Select the desired bus width optionally */
>>>                 err = mmc_select_bus_width(card);
>>> -               if (!err) {
>>> +               if (err > 0) {
>>
>> As pointed out in the review by Björn, to restore the old behaviour we
>> should check for "err >= 0".
>> No need to send a new patch, I can amend the current version.
>>
>>>                         err = mmc_select_hs_ddr(card);
>>>                         if (err)
>>>                                 goto free_card;
>>> --
>>> 2.8.1
>>>
>>
>> Finally, I am a little concerned about the commit 287980e49ffc
>> ("remove lots of IS_ERR_VALUE abuses") which introduced this
>> regression.
>>
>> Surprisingly the IS_ERR_VALUE():s aren't being replaced by equivalent
>> checks, so perhaps there a more regressions. Moreover, I wonder why I
>> wasn't being on cc/to list when this patch was submitted a few days
>> ago, perhaps my review could prevented the regression from even
>> happen.
>>
>> Anyway, let's fix this now! I will pick up $subject patch as fix asap...
>>
>> and Arnd, can you please double-check that the commit 287980e49ffc
>> doesn’t seems to regress anything else!?
>
> If only the 287980e49ffc could sit in linux-next for few days before
> reaching v4.7-rc1... Could you please pick up the fix soon? Maybe
> directly by Linus?

The fix has already been applied and published through my mmc tree. I
am waiting for reports from kernelci, assuming those will be okay, I
will send a PR tomorrow so it should reach rc2.

Kind regards
Uffe

>
> Best regards,
> Krzysztof

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web