mmc: core: enable CMD19 tuning for DDR50 mode

PATCH Superseded Barry Song 5 participants 17 messages Aug 11, 2015 – Dec 18, 2015

Subsystems (2)

MMC, SD and SDIO Secure digital host controller interface (SDHCI) driver

lore

Share & tools

lore

Barry Song

author

2015-08-11 08:34 UTC #0 lore raw

PATCH 1/2 mmc: core: enable CMD19 tuning for DDR50 mode

From: Weijun Yang <[email protected]>

As SD Specifications Part1 Physical Layer Specification Version 3.01 says, CMD19 tuning is available for unlocked cards in transfer state of 1.8V signaling mode. The small difference between v3.00 and 3.01 spec means that CMD19 tuning is also available for DDR50 mode.

Signed-off-by Weijun Yang <[email protected]>

Signed-off-by Barry Song <[email protected]>

---
 drivers/mmc/core/sd.c    | 1 +
 drivers/mmc/host/sdhci.c | 1 +
 2 files changed, 2 insertions(+)

drivers/mmc/core/sd.c +1 -0

index 4e7366a..a1ed24d 100644
@@ -629,6 +629,7 @@ static int mmc_sd_init_uhs_card(struct mmc_card *card)
 	 */
 	if (!mmc_host_is_spi(card->host) &&
 	    (card->sd_bus_speed == UHS_SDR50_BUS_SPEED ||
+	     card->sd_bus_speed == UHS_DDR50_BUS_SPEED ||
 	     card->sd_bus_speed == UHS_SDR104_BUS_SPEED))
 		err = mmc_execute_tuning(card);
 out:

drivers/mmc/host/sdhci.c +1 -0

index 1dbe932..812c19b 100644
@@ -1891,6 +1891,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
 		break;
 
 	case MMC_TIMING_UHS_SDR104:
+	case MMC_TIMING_UHS_DDR50:
 		break;
 
 	case MMC_TIMING_UHS_SDR50:

Signature · 1 line

1.9.1

Ulf Hansson

2015-08-17 17:11 UTC #1 lore raw

PATCH 1/2 mmc: core: enable CMD19 tuning for DDR50 mode

Quoted 7 lines · Barry Song

From: Weijun Yang <[email protected]>
As SD Specifications Part1 Physical Layer Specification Version 3.01 says, CMD19 tuning is available for unlocked cards in transfer state of 1.8V signaling mode. The small difference between v3.00 and 3.01 spec means that CMD19 tuning is also available for DDR50 mode.

So what happens with cards following the 3.0 spec version, those doesn't need to support the tuning CMD right? Perhaps that needs to be addressed in this patch well!?

Kind regards
Uffe

Quoted 35 lines

Signed-off-by Weijun Yang <[email protected]>

Signed-off-by Barry Song <[email protected]>

---
 drivers/mmc/core/sd.c    | 1 +
 drivers/mmc/host/sdhci.c | 1 +
 2 files changed, 2 insertions(+)

drivers/mmc/core/sd.c +1 -0

index 4e7366a..a1ed24d 100644
@@ -629,6 +629,7 @@ static int mmc_sd_init_uhs_card(struct mmc_card *card)
         */
        if (!mmc_host_is_spi(card->host) &&
            (card->sd_bus_speed == UHS_SDR50_BUS_SPEED ||
+            card->sd_bus_speed == UHS_DDR50_BUS_SPEED ||
             card->sd_bus_speed == UHS_SDR104_BUS_SPEED))
                err = mmc_execute_tuning(card);
 out:

drivers/mmc/host/sdhci.c +1 -1

index 1dbe932..812c19b 100644
@@ -1891,6 +1891,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
                break;
 
        case MMC_TIMING_UHS_SDR104:
+       case MMC_TIMING_UHS_DDR50:
                break;
 
        case MMC_TIMING_UHS_SDR50:
--

1.9.1

Barry Song

author

2015-08-20 00:16 UTC #2 lore raw

PATCH 1/2 mmc: core: enable CMD19 tuning for DDR50 mode

2015-08-18 1:11 GMT+08:00 Ulf Hansson <[email protected]>:

Quoted 9 lines · Barry Song

Quoted 7 lines
From: Weijun Yang <[email protected]>
As SD Specifications Part1 Physical Layer Specification Version 3.01 says, CMD19 tuning is available for unlocked cards in transfer state of 1.8V signaling mode. The small difference between v3.00 and 3.01 spec means that CMD19 tuning is also available for DDR50 mode.

Quoted 3 lines

So what happens with cards following the 3.0 spec version, those doesn't need to support the tuning CMD right? Perhaps that needs to be addressed in this patch well!?

from HW registers of the card, we cann't know whether the HW needs tuning. it is said 3.0.x need tuning, but 3.0 doesn't need. @weijun, pls fix me if i am wrong.
if so, it seems we need a static flag somewhere to indicate whether the tuning is needed. we can't detect to find the tuning requirement.

Quoted 31 lines

Kind regards
Uffe

Quoted 27 lines

Signed-off-by Weijun Yang <[email protected]>

Signed-off-by Barry Song <[email protected]>

---
 drivers/mmc/core/sd.c    | 1 +
 drivers/mmc/host/sdhci.c | 1 +
 2 files changed, 2 insertions(+)

drivers/mmc/core/sd.c +1 -0

index 4e7366a..a1ed24d 100644
@@ -629,6 +629,7 @@ static int mmc_sd_init_uhs_card(struct mmc_card *card)
         */
        if (!mmc_host_is_spi(card->host) &&
            (card->sd_bus_speed == UHS_SDR50_BUS_SPEED ||
+            card->sd_bus_speed == UHS_DDR50_BUS_SPEED ||
             card->sd_bus_speed == UHS_SDR104_BUS_SPEED))
                err = mmc_execute_tuning(card);
 out:

drivers/mmc/host/sdhci.c +0 -0

index 1dbe932..812c19b 100644
@@ -1891,6 +1891,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
                break;

Quoted 8 lines

drivers/mmc/host/sdhci.c +1 -1

        case MMC_TIMING_UHS_SDR104:
+       case MMC_TIMING_UHS_DDR50:
                break;
 
        case MMC_TIMING_UHS_SDR50:
--

1.9.1

-barry

Ulf Hansson

2015-08-25 12:05 UTC #3 lore raw

PATCH 1/2 mmc: core: enable CMD19 tuning for DDR50 mode

Quoted 14 lines · Barry Song

2015-08-18 1:11 GMT+08:00 Ulf Hansson <[email protected]>:
Quoted 9 lines · Barry Song
Quoted 7 lines
From: Weijun Yang <[email protected]>
As SD Specifications Part1 Physical Layer Specification Version 3.01 says, CMD19 tuning is available for unlocked cards in transfer state of 1.8V signaling mode. The small difference between v3.00 and 3.01 spec means that CMD19 tuning is also available for DDR50 mode.
Quoted 3 lines
So what happens with cards following the 3.0 spec version, those doesn't need to support the tuning CMD right? Perhaps that needs to be addressed in this patch well!?

Quoted 5 lines

from HW registers of the card, we cann't know whether the HW needs tuning. it is said 3.0.x need tuning, but 3.0 doesn't need. @weijun, pls fix me if i am wrong.
if so, it seems we need a static flag somewhere to indicate whether the tuning is needed. we can't detect to find the tuning requirement.

Another way is to always try doing the tuning for DDR50, but in case of errors just ignore them and print a debug/info message!?

Kind regards
Uffe

Barry Song

author

2015-09-15 07:13 UTC #4 lore raw

PATCH 1/2 mmc: core: enable CMD19 tuning for DDR50 mode

2015-08-25 20:05 GMT+08:00 Ulf Hansson <[email protected]>:

Quoted 15 lines · Barry Song

Quoted 14 lines
2015-08-18 1:11 GMT+08:00 Ulf Hansson <[email protected]>:
Quoted 12 lines · Barry Song
Quoted 7 lines
From: Weijun Yang <[email protected]>
As SD Specifications Part1 Physical Layer Specification Version 3.01 says, CMD19 tuning is available for unlocked cards in transfer state of 1.8V signaling mode. The small difference between v3.00 and 3.01 spec means that CMD19 tuning is also available for DDR50 mode.
So what happens with cards following the 3.0 spec version, those doesn't need to support the tuning CMD right? Perhaps that needs to be addressed in this patch well!?

Quoted 8 lines

Quoted 5 lines
from HW registers of the card, we cann't know whether the HW needs tuning. it is said 3.0.x need tuning, but 3.0 doesn't need. @weijun, pls fix me if i am wrong.
if so, it seems we need a static flag somewhere to indicate whether the tuning is needed. we can't detect to find the tuning requirement.
Another way is to always try doing the tuning for DDR50, but in case of errors just ignore them and print a debug/info message!?

Uffe, do you mean something like below:

drivers/mmc/core/sd.c +16 -1

index 4e7366a..d4df9f0 100644
@@ -629,8 +629,23 @@ static int mmc_sd_init_uhs_card(struct mmc_card *card)
      */
     if (!mmc_host_is_spi(card->host) &&
         (card->sd_bus_speed == UHS_SDR50_BUS_SPEED ||
-         card->sd_bus_speed == UHS_SDR104_BUS_SPEED))
+         card->sd_bus_speed == UHS_DDR50_BUS_SPEED ||
+         card->sd_bus_speed == UHS_SDR104_BUS_SPEED)) {
         err = mmc_execute_tuning(card);
+
+        /*
+         * As SD Specifications Part1 Physical Layer Specification Version
+         * 3.01 says, CMD19 tuning is available for unlocked cards in transfer
+         * state of 1.8V signaling mode. The small difference between v3.00
+         * and 3.01 spec means that CMD19 tuning is also available for DDR50
+         * mode.
+         */
+        if (err && (card->sd_bus_speed == UHS_DDR50_BUS_SPEED))
+            pr_err("%s: ddr50 tuning failed\n", mmc_hostname(card->host));
+            kfree(status);
+            return 0;
+        }
+    }
 out:
     kfree(status);

drivers/mmc/host/sdhci.c +0 -0

index 64b7fdb..382810d 100644
@@ -1915,6 +1915,7 @@ static int sdhci_execute_tuning(struct mmc_host

*mmc, u32 opcode)

         break;

     case MMC_TIMING_UHS_SDR104:
+    case MMC_TIMING_UHS_DDR50:
         break;

     case MMC_TIMING_UHS_SDR50:

Quoted 3 lines

Kind regards
Uffe

-barry

Ulf Hansson

2015-09-17 12:27 UTC #5 lore raw

PATCH 1/2 mmc: core: enable CMD19 tuning for DDR50 mode

Quoted 39 lines · Barry Song

2015-08-25 20:05 GMT+08:00 Ulf Hansson <[email protected]>:
Quoted 15 lines · Barry Song
Quoted 14 lines
2015-08-18 1:11 GMT+08:00 Ulf Hansson <[email protected]>:
Quoted 12 lines · Barry Song
Quoted 7 lines
From: Weijun Yang <[email protected]>
As SD Specifications Part1 Physical Layer Specification Version 3.01 says, CMD19 tuning is available for unlocked cards in transfer state of 1.8V signaling mode. The small difference between v3.00 and 3.01 spec means that CMD19 tuning is also available for DDR50 mode.
So what happens with cards following the 3.0 spec version, those doesn't need to support the tuning CMD right? Perhaps that needs to be addressed in this patch well!?
Quoted 8 lines
Quoted 5 lines
from HW registers of the card, we cann't know whether the HW needs tuning. it is said 3.0.x need tuning, but 3.0 doesn't need. @weijun, pls fix me if i am wrong.
if so, it seems we need a static flag somewhere to indicate whether the tuning is needed. we can't detect to find the tuning requirement.
Another way is to always try doing the tuning for DDR50, but in case of errors just ignore them and print a debug/info message!?
Uffe, do you mean something like below:
drivers/mmc/core/sd.c +2 -1
index 4e7366a..d4df9f0 100644
@@ -629,8 +629,23 @@ static int mmc_sd_init_uhs_card(struct mmc_card *card)
      */
     if (!mmc_host_is_spi(card->host) &&
         (card->sd_bus_speed == UHS_SDR50_BUS_SPEED ||
-         card->sd_bus_speed == UHS_SDR104_BUS_SPEED))
+         card->sd_bus_speed == UHS_DDR50_BUS_SPEED ||
+         card->sd_bus_speed == UHS_SDR104_BUS_SPEED)) {
         err = mmc_execute_tuning(card);

drivers/mmc/core/sd.c static int mmc_sd_init_uhs_card(struct mmc_card *card) · line 644

Quoted 10 lines

drivers/mmc/core/sd.c +10 -0

+
+        /*
+         * As SD Specifications Part1 Physical Layer Specification Version
+         * 3.01 says, CMD19 tuning is available for unlocked cards in transfer
+         * state of 1.8V signaling mode. The small difference between v3.00
+         * and 3.01 spec means that CMD19 tuning is also available for DDR50
+         * mode.
+         */
+        if (err && (card->sd_bus_speed == UHS_DDR50_BUS_SPEED))
+            pr_err("%s: ddr50 tuning failed\n", mmc_hostname(card->host));

Perhaps pr_warn() instead.

Moreover, you may just assign err = 0 and leave the return/kfree to the "out" label.

drivers/mmc/core/sd.c static int mmc_sd_init_uhs_card(struct mmc_card *card) · line 651

Quoted 7 lines

drivers/mmc/core/sd.c +4 -0

+            kfree(status);
+            return 0;
+        }
+    }
 out:
     kfree(status);

[...]

Also, I was giving this a second thought...

What happens with the SD 3.0 card when it *doesn't* support CMD19. Will it still be functional afterwards or will it move from "transfer state" to another not known state?

So maybe we should just do the tuning and return the error if it fails, as you suggested in the original patch. :-)

Kind regards
Uffe

Barry Song

author

2015-09-18 09:40 UTC #6 lore raw

PATCH 1/2 mmc: core: enable CMD19 tuning for DDR50 mode

2015-09-17 20:27 GMT+08:00 Ulf Hansson <[email protected]>:

Quoted 64 lines · Barry Song

Quoted 39 lines
2015-08-25 20:05 GMT+08:00 Ulf Hansson <[email protected]>:
Quoted 15 lines · Barry Song
Quoted 14 lines
2015-08-18 1:11 GMT+08:00 Ulf Hansson <[email protected]>:
Quoted 12 lines · Barry Song
Quoted 7 lines
From: Weijun Yang <[email protected]>
As SD Specifications Part1 Physical Layer Specification Version 3.01 says, CMD19 tuning is available for unlocked cards in transfer state of 1.8V signaling mode. The small difference between v3.00 and 3.01 spec means that CMD19 tuning is also available for DDR50 mode.
So what happens with cards following the 3.0 spec version, those doesn't need to support the tuning CMD right? Perhaps that needs to be addressed in this patch well!?
Quoted 8 lines
Quoted 5 lines
from HW registers of the card, we cann't know whether the HW needs tuning. it is said 3.0.x need tuning, but 3.0 doesn't need. @weijun, pls fix me if i am wrong.
if so, it seems we need a static flag somewhere to indicate whether the tuning is needed. we can't detect to find the tuning requirement.
Another way is to always try doing the tuning for DDR50, but in case of errors just ignore them and print a debug/info message!?
Uffe, do you mean something like below:
drivers/mmc/core/sd.c +2 -1
index 4e7366a..d4df9f0 100644
@@ -629,8 +629,23 @@ static int mmc_sd_init_uhs_card(struct mmc_card *card)
      */
     if (!mmc_host_is_spi(card->host) &&
         (card->sd_bus_speed == UHS_SDR50_BUS_SPEED ||
-         card->sd_bus_speed == UHS_SDR104_BUS_SPEED))
+         card->sd_bus_speed == UHS_DDR50_BUS_SPEED ||
+         card->sd_bus_speed == UHS_SDR104_BUS_SPEED)) {
         err = mmc_execute_tuning(card);
drivers/mmc/core/sd.c static int mmc_sd_init_uhs_card(struct mmc_card *card) · line 644
Quoted 10 lines
drivers/mmc/core/sd.c +10 -0
+
+        /*
+         * As SD Specifications Part1 Physical Layer Specification Version
+         * 3.01 says, CMD19 tuning is available for unlocked cards in transfer
+         * state of 1.8V signaling mode. The small difference between v3.00
+         * and 3.01 spec means that CMD19 tuning is also available for DDR50
+         * mode.
+         */
+        if (err && (card->sd_bus_speed == UHS_DDR50_BUS_SPEED))
+            pr_err("%s: ddr50 tuning failed\n", mmc_hostname(card->host));
Perhaps pr_warn() instead.
Moreover, you may just assign err = 0 and leave the return/kfree to the "out" label.
drivers/mmc/core/sd.c static int mmc_sd_init_uhs_card(struct mmc_card *card) · line 651
Quoted 7 lines
drivers/mmc/core/sd.c +4 -0
+            kfree(status);
+            return 0;
+        }
+    }
 out:
     kfree(status);
 

Quoted 7 lines

[...]
Also, I was giving this a second thought...
What happens with the SD 3.0 card when it *doesn't* support CMD19. Will it still be functional afterwards or will it move from "transfer state" to another not known state?

uffe, i am not the expert of this issue but weijun is. weijun told that for the cards who don't support cmd19, it will take it as illegal command and not response it. and it also sets illegal command error in registers.
but the card status is still functional and doesn't cause unknown state.

so maybe we can just ignore the err of tuning, and use the second patch with the refine you mentioned?

Quoted 6 lines

So maybe we should just do the tuning and return the error if it fails, as you suggested in the original patch. :-)
Kind regards
Uffe

-barry

Ulf Hansson

2015-09-23 19:25 UTC #7 lore raw

PATCH 1/2 mmc: core: enable CMD19 tuning for DDR50 mode

[...]

Quoted 7 lines

Quoted 6 lines
Also, I was giving this a second thought...
What happens with the SD 3.0 card when it *doesn't* support CMD19. Will it still be functional afterwards or will it move from "transfer state" to another not known state?

Quoted 8 lines

uffe, i am not the expert of this issue but weijun is. weijun told that for the cards who don't support cmd19, it will take it as illegal command and not response it. and it also sets illegal command error in registers.
but the card status is still functional and doesn't cause unknown state.
so maybe we can just ignore the err of tuning, and use the second patch with the refine you mentioned?

Sure, let's do that. Please send a v2 of the patchset then.

Kind regards
Uffe

Carlo Caione

2015-12-12 23:51 UTC #8 lore raw

PATCH 1/2 mmc: core: enable CMD19 tuning for DDR50 mode

Quoted 15 lines · Ulf Hansson

[...]
Quoted 13 lines
Quoted 6 lines
Also, I was giving this a second thought...
What happens with the SD 3.0 card when it *doesn't* support CMD19. Will it still be functional afterwards or will it move from "transfer state" to another not known state?
uffe, i am not the expert of this issue but weijun is. weijun told that for the cards who don't support cmd19, it will take it as illegal command and not response it. and it also sets illegal command error in registers.
but the card status is still functional and doesn't cause unknown state.

Quoted 4 lines

Quoted 2 lines
so maybe we can just ignore the err of tuning, and use the second patch with the refine you mentioned?
Sure, let's do that. Please send a v2 of the patchset then.

Replying here since I'm not subscribed to linux-mmc.

Commit 9faac7b95 "mmc: sdhci: enable tuning for DDR50" actually breaks WiFi on SDIO at least for the ASUS X205TA.

I tried to ignore the error from the tuning also for the SDIO card patching mmc_sdio_init_uhs_card() in drivers/mmc/core/sdio.c but I end up with the error "Controller never released inhibit bit(s)". At least in my case CMD19 shouldn't be issued at all to have the SDIO card working.

Any suggestion how to approach this problem?

Signature · 1 line

Carlo Caione

Yang, York

2015-12-14 08:46 UTC #9 lore raw

PATCH 1/2 mmc: core: enable CMD19 tuning for DDR50 mode

Hi, Carlo
In 9faac7b95, tuning for DDR50 is enabled in mmc_sd_init_uhs_card(), so only SD card is affected.
This is based on the SD spec version 3.01.
mmc_sdio_init_uhs_card() is untouched. I don't know who add the code into mmc_sdio_init_uhs_card() and you may have a check on your side.

-----Original Message-----
From: [email protected] [mailto:carlo.caione at gmail.com] On Behalf Of Carlo Caione
Sent: 2015?12?13? 7:52
To: Ulf Hansson <[email protected]>
Cc: Barry Song <[email protected]>; Yang, York <[email protected]>; linux-mmc <[email protected]>; DL-SHA-WorkGroupLinux <[email protected]>; linux-arm-kernel at lists.infradead.org; Song, Barry <[email protected]>
Subject: Re: [PATCH 1/2] mmc: core: enable CMD19 tuning for DDR50 mode

Quoted 15 lines · Ulf Hansson

[...]
Quoted 13 lines
Quoted 6 lines
Also, I was giving this a second thought...
What happens with the SD 3.0 card when it *doesn't* support CMD19. Will it still be functional afterwards or will it move from "transfer state" to another not known state?
uffe, i am not the expert of this issue but weijun is. weijun told that for the cards who don't support cmd19, it will take it as illegal command and not response it. and it also sets illegal command error in registers.
but the card status is still functional and doesn't cause unknown state.

Quoted 4 lines

Quoted 2 lines
so maybe we can just ignore the err of tuning, and use the second patch with the refine you mentioned?
Sure, let's do that. Please send a v2 of the patchset then.

Replying here since I'm not subscribed to linux-mmc.

Commit 9faac7b95 "mmc: sdhci: enable tuning for DDR50" actually breaks WiFi on SDIO at least for the ASUS X205TA.

Any suggestion how to approach this problem?

--
Carlo Caione

To report this email as spam click https://www.mailcontrol.com/sr/t!EO05MqfQ7GX2PQPOmvUkWM85sEKD4+jZQCQMWcEF+Ktu75GnJjKIuILC13YpF6Ld90r0YXMUaNc!rJiVGaJg== .

Carlo Caione

2015-12-14 09:13 UTC #10 lore raw

PATCH 1/2 mmc: core: enable CMD19 tuning for DDR50 mode

Quoted 4 lines · York

Hi, Carlo
In 9faac7b95, tuning for DDR50 is enabled in mmc_sd_init_uhs_card(), so only SD card is affected.
This is based on the SD spec version 3.01.
mmc_sdio_init_uhs_card() is untouched. I don't know who add the code into mmc_sdio_init_uhs_card() and you may have a check on your side.

Hi Yang,
sorry for not being clear. Modifying mmc_sdio_init_uhs_card() was only a test I did to check whether that could fix the issue, but it didn't.

Right now the situation is:
- In the latest master with commit 9faac7b95 WiFi is broken on the ASUS X205TA (bcm43341)
- Reverting 9faac7b95 makes it working fine again

My conclusion is that issuing the CMD19 is basically breaking WiFi on this platform.

Signature · 1 line

Carlo Caione

Yang, York

2015-12-14 09:24 UTC #11 lore raw

PATCH 1/2 mmc: core: enable CMD19 tuning for DDR50 mode

Hi, Carlo

I don't think so. You may check the 9faac7b95, there are two changes in sd.c and sdhc.c.
Surely the change in sdhc.c will not affect ASUS X205TA (bcm43341) since it is a SDIO device.
Actually the calling sequence is this: mmc_sd_init_uhs_card() calls sdhci_execute_tuning(). In 9faac7b95, I enabled tuning for DDR50 in these two functions. If your function calling sequence is mmc_sdio_init_uhs_card()->sdhci_execute_tuning(), the tuning will not carried out, since you didn't enable tuning for DDR50 in mmc_sdio_init_uhs_card(). Unless you call sdhci_execute_tuning directly, the tuning command may go through be sent out.

-----Original Message-----
From: [email protected] [mailto:carlo.caione at gmail.com] On Behalf Of Carlo Caione
Sent: 2015?12?14? 17:14
To: Yang, York <[email protected]>
Cc: Carlo Caione <[email protected]>; Ulf Hansson <[email protected]>; Barry Song <[email protected]>; linux-mmc <[email protected]>; DL-SHA-WorkGroupLinux <[email protected]>; linux-arm-kernel at lists.infradead.org; Song, Barry <[email protected]>
Subject: Re: [PATCH 1/2] mmc: core: enable CMD19 tuning for DDR50 mode

Quoted 4 lines · York

Hi, Carlo
In 9faac7b95, tuning for DDR50 is enabled in mmc_sd_init_uhs_card(), so only SD card is affected.
This is based on the SD spec version 3.01.
mmc_sdio_init_uhs_card() is untouched. I don't know who add the code into mmc_sdio_init_uhs_card() and you may have a check on your side.

Hi Yang,
sorry for not being clear. Modifying mmc_sdio_init_uhs_card() was only a test I did to check whether that could fix the issue, but it didn't.

Right now the situation is:
- In the latest master with commit 9faac7b95 WiFi is broken on the ASUS X205TA (bcm43341)
- Reverting 9faac7b95 makes it working fine again

My conclusion is that issuing the CMD19 is basically breaking WiFi on this platform.

--
Carlo Caione

Carlo Caione

2015-12-14 10:56 UTC #12 lore raw

PATCH 1/2 mmc: core: enable CMD19 tuning for DDR50 mode

Quoted 7 lines · York

Hi, Carlo
I don't think so. You may check the 9faac7b95, there are two changes in sd.c and sdhc.c.
Surely the change in sdhc.c will not affect ASUS X205TA (bcm43341) since it is a SDIO device.
Actually the calling sequence is this: mmc_sd_init_uhs_card() calls sdhci_execute_tuning(). In 9faac7b95, I enabled tuning for DDR50 in these two functions. If your function calling sequence is mmc_sdio_init_uhs_card()->sdhci_execute_tuning(), the tuning will not carried out, since you didn't enable tuning for DDR50 in mmc_sdio_init_uhs_card(). Unless you call sdhci_execute_tuning directly, the tuning command may go through be sent out.

Hi Yang,

On the unmodified master what I see is:

- mmc_sdio_init_uhs_card() is called
- card->sw_caps.sd3_bus_mode is 0x14
- card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR50 is actually verified (even though the tuning for DDR50 is not enabled)
- mmc_execute_tuning() is actually called

Signature · 1 line

Carlo Caione

Jaehoon Chung

2015-12-15 23:56 UTC #13 lore raw

PATCH 1/2 mmc: core: enable CMD19 tuning for DDR50 mode

Hi,

Quoted 11 lines · Carlo Caione

Quoted 7 lines · York
Hi, Carlo
I don't think so. You may check the 9faac7b95, there are two changes in sd.c and sdhc.c.
Surely the change in sdhc.c will not affect ASUS X205TA (bcm43341) since it is a SDIO device.
Actually the calling sequence is this: mmc_sd_init_uhs_card() calls sdhci_execute_tuning(). In 9faac7b95, I enabled tuning for DDR50 in these two functions. If your function calling sequence is mmc_sdio_init_uhs_card()->sdhci_execute_tuning(), the tuning will not carried out, since you didn't enable tuning for DDR50 in mmc_sdio_init_uhs_card(). Unless you call sdhci_execute_tuning directly, the tuning command may go through be sent out.
Hi Yang,

Quoted 7 lines

On the unmodified master what I see is:
- mmc_sdio_init_uhs_card() is called
- card->sw_caps.sd3_bus_mode is 0x14
- card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR50 is actually verified (even though the tuning for DDR50 is not enabled)
- mmc_execute_tuning() is actually called

As i know, When mode is UHS_SDR50, tuning is not mandatory.
Even though i need to check the spec file, i think that condition check is strange.

Best Regards,
Jaehoon Chung

Quoted 1 line

Yang, York

2015-12-17 02:00 UTC #14 lore raw

PATCH 1/2 mmc: core: enable CMD19 tuning for DDR50 mode

In reply to Carlo Caione

Hi, Carlo

I don't understand. We can see that 9faac7b95 is all about DDR50. It should not affect the behavior of SDR50. Since the commit is very simple, you may remove the change one by one by yourself. And see if any difference occurs. Besides, please check if the card->sw_caps.sd3_bus_mode changes with or without DDR50 tuning enabled.

-----Original Message-----
From: [email protected] [mailto:carlo.caione at gmail.com] On Behalf Of Carlo Caione
Sent: 2015?12?14? 18:57
To: Yang, York <[email protected]>
Cc: Carlo Caione <[email protected]>; Ulf Hansson <[email protected]>; Barry Song <[email protected]>; linux-mmc <[email protected]>; DL-SHA-WorkGroupLinux <[email protected]>; linux-arm-kernel at lists.infradead.org; Song, Barry <[email protected]>
Subject: Re: [PATCH 1/2] mmc: core: enable CMD19 tuning for DDR50 mode

Quoted 7 lines · York

Hi, Carlo
I don't think so. You may check the 9faac7b95, there are two changes in sd.c and sdhc.c.
Surely the change in sdhc.c will not affect ASUS X205TA (bcm43341) since it is a SDIO device.
Actually the calling sequence is this: mmc_sd_init_uhs_card() calls sdhci_execute_tuning(). In 9faac7b95, I enabled tuning for DDR50 in these two functions. If your function calling sequence is mmc_sdio_init_uhs_card()->sdhci_execute_tuning(), the tuning will not carried out, since you didn't enable tuning for DDR50 in mmc_sdio_init_uhs_card(). Unless you call sdhci_execute_tuning directly, the tuning command may go through be sent out.

Hi Yang,

On the unmodified master what I see is:

--
Carlo Caione

Carlo Caione

2015-12-17 19:54 UTC #15 lore raw

PATCH 1/2 mmc: core: enable CMD19 tuning for DDR50 mode

Quoted 1 line · York

Hi, Carlo

Hi Yang,

Quoted 2 lines

I don't understand. We can see that 9faac7b95 is all about DDR50. It should not affect the behavior of SDR50.

What I'm saying is that this commit is negatively affecting some DDR50 SDIO card as in my case.

Ok, let's try to go through the code with and without 9faac7b95.

Without 9faac7b95:
- mmc_sdio_init_uhs_card() is called
- sw_caps.sd3_bus_mode is 0x14 (SD_MODE_UHS_SDR50 | SD_MODE_UHS_DDR50)
- card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR50 is verified and mmc_execute_tuning() is called
- sdhci_execute_tuning() is called
- host->timing is MMC_TIMING_UHS_DDR50
- since there is no case for that we end up in 'default', goto out_unlock and CMD19 is never issued

With 9faac7b95:
- mmc_sdio_init_uhs_card() is called
- sw_caps.sd3_bus_mode is 0x14 (SD_MODE_UHS_SDR50 | SD_MODE_UHS_DDR50)
- card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR50 is verified and mmc_execute_tuning() is called
- sdhci_execute_tuning() is called
- host->timing is MMC_TIMING_UHS_DDR50
- this time we have the the 'case MMC_TIMING_UHS_DDR50' and we break on that
- CMD19 is issued and the card stops working

Quoted 2 lines

Since the commit is very simple, you may remove the change one by one by yourself. And see if any difference occurs.

As stated above the problem is in the new 'case MMC_TIMING_UHS_DDR50' we have in sdhci_execute_tuning()

Quoted 3 lines

Besides, please check
if the card->sw_caps.sd3_bus_mode changes with or without DDR50 tuning enabled.

It doesn't.

Signature · 1 line

Carlo Caione

Yang, York

2015-12-18 05:52 UTC #16 lore raw

PATCH 1/2 mmc: core: enable CMD19 tuning for DDR50 mode

Hi, Carlo

	I understand now. So the problem comes out because the sw_caps.sd3_bus_mode marks the capability of a card, while the host->timing is the mode we actually do with.
	For your case, you are running DDR50 mode, what if we add more condition for coming into the tuning logic?
	struct sdhci_host *host = mmc_priv(card->host);
	if (!mmc_host_is_spi(card->host) && card->host->ops->execute_tuning &&
			((card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR50) ||
			 (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR104))&&
			((host->timing & MMC_TIMING_UHS_SDR50) ||
				(host->timing & MMC_TIMING_UHS_SDR104))) {
		mmc_host_clk_hold(card->host);
		err = card->host->ops->execute_tuning(card->host,
						      MMC_SEND_TUNING_BLOCK);
		mmc_host_clk_release(card->host);
	}

-----Original Message-----
From: [email protected] [mailto:carlo.caione at gmail.com] On Behalf Of Carlo Caione
Sent: 2015?12?18? 3:54
To: Yang, York <[email protected]>
Cc: Ulf Hansson <[email protected]>; Barry Song <[email protected]>; linux-mmc <[email protected]>; DL-SHA-WorkGroupLinux <[email protected]>; linux-arm-kernel at lists.infradead.org; Song, Barry <[email protected]>
Subject: Re: [PATCH 1/2] mmc: core: enable CMD19 tuning for DDR50 mode

Quoted 1 line · York

Hi, Carlo

Hi Yang,

Quoted 2 lines

I don't understand. We can see that 9faac7b95 is all about DDR50. It should not affect the behavior of SDR50.

What I'm saying is that this commit is negatively affecting some DDR50 SDIO card as in my case.

Ok, let's try to go through the code with and without 9faac7b95.

Quoted 2 lines

Since the commit is very simple, you may remove the change one by one by yourself. And see if any difference occurs.

As stated above the problem is in the new 'case MMC_TIMING_UHS_DDR50' we have in sdhci_execute_tuning()

Quoted 3 lines

Besides, please check
if the card->sw_caps.sd3_bus_mode changes with or without DDR50 tuning enabled.

It doesn't.

--
Carlo Caione