mmc: mx28: sd card detection broken since 3.18-rc1

Hi,

i was testing Linux Kernel 3.18-rc2 with my i.MX28 board (I2SE Duckbill) and ran into the problem that the sd card isn't detected from the Kernel at booting (driver: mxs-mmc.c). That results in a endless wait for the root partition

Here are the relevant messages (bad case):

[...]

[    1.501883] mxs-mmc 80010000.ssp: initialized
[    1.521203] TCP: cubic registered
[    1.530850] NET: Registered protocol family 10
[    1.548469] mip6: Mobile IPv6
[    1.551698] sit: IPv6 over IPv4 tunneling driver
[    1.566016] ip6_gre: GRE over IPv6 tunneling driver
[    1.575831] NET: Registered protocol family 17
[    1.581640] bridge: automatic filtering via arp/ip/ip6tables has been
deprecated. Update your scripts to load br_netfilter if you need this.
[    1.595635] Key type dns_resolver registered
[    1.604302] registered taskstats version 1
[    1.618188] stmp3xxx-rtc 80056000.rtc: setting system clock to 1970-01-01
00:00:03 UTC (3)
[    1.675580] Waiting for root device /dev/mmcblk0p3...

In Linux Kernel 3.17 that problem didn't exist (good case):

[...]

[    1.546857] mxs-mmc 80010000.ssp: initialized
[    1.576363] TCP: cubic registered
[    1.588856] NET: Registered protocol family 10
[    1.608208] mmc0: host does not support reading read-only switch. assuming
write-enable.
[    1.616927] mip6: Mobile IPv6
[    1.620028] sit: IPv6 over IPv4 tunneling driver
[    1.629900] mmc0: new high speed SDHC card at address 0007
[    1.642901] ip6_gre: GRE over IPv6 tunneling driver
[    1.652047] mmcblk0: mmc0:0007 SD16G 14.6 GiB
[    1.662108] NET: Registered protocol family 17
[    1.678091]  mmcblk0: p1 p2 p3

[...]

I've have bisected the problem to this commit:

commit 89168b48991537bec2573b3b6a8841df74465b12

Author: Linus Walleij <[email protected]>
Date:   Thu Oct 2 09:08:46 2014 +0200

mmc: core: restore detect line inversion semantics

commit 98e90de99a0c43bd434da814c882c4332441871e "mmc: host: switch OF parser to use gpio descriptors" switched the semantic behaviour of card detect and read only flags such that the inversion capability flag would only be set if inversion was explicitly specified in the device tree, in the hopes that no-one was using double inversion.

It turns out that the XOR:ing between the explicit inversion was indeed in use, so we need to restore the old semantics where both ways of inversion are checked and the end result XOR:ed.

Reported-by Javier Martinez Canillas <[email protected]>

Tested-by Javier Martinez Canillas <[email protected]>

Signed-off-by Linus Walleij <[email protected]>

Signed-off-by Ulf Hansson <[email protected]>

Kernel command line: -e noinitrd console=ttyAMA0,115200 root=/dev/mmcblk0p3 rw rootwait

It looks to me that the patch didn't fix all host controller.

I ran into this issue as well. Seems that a card-detect flag (MMC_CAP2_CD_ACTIVE_HIGH) can currently be set based on an uninitialized variable, which can lead to the card being reported as not present. This patch fixes it for me:

[...]
I ran into this issue as well. Seems that a card-detect flag (MMC_CAP2_CD_ACTIVE_HIGH) can currently be set based on an uninitialized variable, which can lead to the card being reported as not present. This patch fixes it for me:

drivers/mmc/core/host.c +2 -1

index 03c53b72a2d6..f0e187682d3b 100644
@@ -311,7 +311,7 @@ int mmc_of_parse(struct mmc_host *host)
 	struct device_node *np;
 	u32 bus_width;
 	int len, ret;
-	bool cap_invert, gpio_invert;
+	bool cap_invert, gpio_invert = false;
 
 	if (!host->parent || !host->parent->of_node)
 		return 0;

@@ -401,6 +401,7 @@ int mmc_of_parse(struct mmc_host *host)
 	else
 		cap_invert = false;
 
+	gpio_invert = false;
 	ret = mmc_gpiod_request_ro(host, "wp", 0, false, 0, &gpio_invert);
 	if (ret) {
 		if (ret == -EPROBE_DEFER)

Let me know if this also fixes it for you, and I'll send in a proper patch.

Thanks,
Kristina

Quoted 67 lines

Hi,

i was testing Linux Kernel 3.18-rc2 with my i.MX28 board (I2SE Duckbill) and ran into the problem that the sd card isn't detected from the Kernel at booting (driver: mxs-mmc.c). That results in a endless wait for the root partition

Here are the relevant messages (bad case):

[...]

[    1.501883] mxs-mmc 80010000.ssp: initialized
[    1.521203] TCP: cubic registered
[    1.530850] NET: Registered protocol family 10
[    1.548469] mip6: Mobile IPv6
[    1.551698] sit: IPv6 over IPv4 tunneling driver
[    1.566016] ip6_gre: GRE over IPv6 tunneling driver
[    1.575831] NET: Registered protocol family 17
[    1.581640] bridge: automatic filtering via arp/ip/ip6tables has been
deprecated. Update your scripts to load br_netfilter if you need this.
[    1.595635] Key type dns_resolver registered
[    1.604302] registered taskstats version 1
[    1.618188] stmp3xxx-rtc 80056000.rtc: setting system clock to 1970-01-01
00:00:03 UTC (3)
[    1.675580] Waiting for root device /dev/mmcblk0p3...

In Linux Kernel 3.17 that problem didn't exist (good case):

[...]

[    1.546857] mxs-mmc 80010000.ssp: initialized
[    1.576363] TCP: cubic registered
[    1.588856] NET: Registered protocol family 10
[    1.608208] mmc0: host does not support reading read-only switch. assuming
write-enable.
[    1.616927] mip6: Mobile IPv6
[    1.620028] sit: IPv6 over IPv4 tunneling driver
[    1.629900] mmc0: new high speed SDHC card at address 0007
[    1.642901] ip6_gre: GRE over IPv6 tunneling driver
[    1.652047] mmcblk0: mmc0:0007 SD16G 14.6 GiB
[    1.662108] NET: Registered protocol family 17
[    1.678091]  mmcblk0: p1 p2 p3

[...]

I've have bisected the problem to this commit:

commit 89168b48991537bec2573b3b6a8841df74465b12

Author: Linus Walleij <[email protected]>
Date:   Thu Oct 2 09:08:46 2014 +0200

mmc: core: restore detect line inversion semantics

commit 98e90de99a0c43bd434da814c882c4332441871e "mmc: host: switch OF parser to use gpio descriptors" switched the semantic behaviour of card detect and read only flags such that the inversion capability flag would only be set if inversion was explicitly specified in the device tree, in the hopes that no-one was using double inversion.

It turns out that the XOR:ing between the explicit inversion was indeed in use, so we need to restore the old semantics where both ways of inversion are checked and the end result XOR:ed.

Reported-by Javier Martinez Canillas <[email protected]>

Tested-by Javier Martinez Canillas <[email protected]>

Signed-off-by Linus Walleij <[email protected]>

Signed-off-by Ulf Hansson <[email protected]>

Quoted 4 lines

Kernel command line: -e noinitrd console=ttyAMA0,115200 root=/dev/mmcblk0p3 rw rootwait

It looks to me that the patch didn't fix all host controller.

I ran into this issue as well. Seems that a card-detect flag (MMC_CAP2_CD_ACTIVE_HIGH) can currently be set based on an uninitialized variable, which can lead to the card being reported as not present. This patch fixes it for me:

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

index 03c53b72a2d6..f0e187682d3b 100644

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

 	if (ret) {
 		if (ret == -EPROBE_DEFER)

Let me know if this also fixes it for you, and I'll send in a proper patch.

Thanks,
Kristina
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi,

Am Montag, 3. November 2014, 01:01:07 schrieben Sie:

Quoted 17 lines · Stefan Wahren

Quoted 5 lines

Hi,

i was testing Linux Kernel 3.18-rc2 with my i.MX28 board (I2SE Duckbill) and ran into the problem that the sd card isn't detected from the Kernel at booting (driver: mxs-mmc.c). That results in a endless wait for the root partition

I ran into this issue as well. Seems that a card-detect flag (MMC_CAP2_CD_ACTIVE_HIGH) can currently be set based on an uninitialized variable, which can lead to the card being reported as not present. This patch fixes it for me:

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

index 03c53b72a2d6..f0e187682d3b 100644

drivers/mmc/core/host.c int mmc_of_parse(struct mmc_host *host) · line 315

Quoted 6 lines

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

@@ -311,7 +311,7 @@ int mmc_of_parse(struct mmc_host *host)
 	struct device_node *np;
 	u32 bus_width;
 	int len, ret;
-	bool cap_invert, gpio_invert;
+	bool cap_invert, gpio_invert = false;
 

sorry, but I don't understand how your patch fixes the problem.

First use of the gpio_invert bool is in line 370/371 within mmc_gpiod_request_cd (re-wrapped into a single line here for better reading):

-snip-
ret = mmc_gpiod_request_cd(host, "cd", 0, true, 0, &gpio_invert); -snap-

A pointer to the bool is passed, and inside mmc_gpiod_request_cd (drivers/mmc/core/slot-gpio.c, line 322), always a value is assigned:

-snip-
if (gpio_invert)

                *gpio_invert = !gpiod_is_active_low(desc);

-snap-

So returning to mmc_of_parse, the bool should always have an initialized value. Apart from some error handling, the bool is used immediately in the xor expression and results in setting MMC_CAP2_CD_ACTIVE_HIGH bit, or not.

I also cannot see a code path, where gpio_invert is used without a call to mmc_gpiod_request_cd.

Would be nice, if you could point me to what I'm missing.

drivers/mmc/core/host.c int mmc_of_parse(struct mmc_host *host) · line 405

Quoted 8 lines

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

 	if (!host->parent || !host->parent->of_node)
 		return 0;

@@ -401,6 +401,7 @@ int mmc_of_parse(struct mmc_host *host)
 	else
 		cap_invert = false;
 
+	gpio_invert = false;
 	ret = mmc_gpiod_request_ro(host, "wp", 0, false, 0, &gpio_invert);

Same here. The functions always assigns a value when a pointer is given.

(And this change is unrelated to the reporters problem, so should be fixed with a dedicated patch.)

Quoted 1 line

Hi,

Hi Michael,

Quoted 42 lines

Am Montag, 3. November 2014, 01:01:07 schrieben Sie:

Quoted 17 lines · Stefan Wahren

Quoted 5 lines

Hi,

i was testing Linux Kernel 3.18-rc2 with my i.MX28 board (I2SE Duckbill) and ran into the problem that the sd card isn't detected from the Kernel at booting (driver: mxs-mmc.c). That results in a endless wait for the root partition

I ran into this issue as well. Seems that a card-detect flag (MMC_CAP2_CD_ACTIVE_HIGH) can currently be set based on an uninitialized variable, which can lead to the card being reported as not present. This patch fixes it for me:

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

index 03c53b72a2d6..f0e187682d3b 100644

drivers/mmc/core/host.c int mmc_of_parse(struct mmc_host *host) · line 315

Quoted 6 lines

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

@@ -311,7 +311,7 @@ int mmc_of_parse(struct mmc_host *host)
 	struct device_node *np;
 	u32 bus_width;
 	int len, ret;
-	bool cap_invert, gpio_invert;
+	bool cap_invert, gpio_invert = false;
 

sorry, but I don't understand how your patch fixes the problem.

First use of the gpio_invert bool is in line 370/371 within mmc_gpiod_request_cd (re-wrapped into a single line here for better reading):

-snip-
ret = mmc_gpiod_request_cd(host, "cd", 0, true, 0, &gpio_invert); -snap-

A pointer to the bool is passed, and inside mmc_gpiod_request_cd (drivers/mmc/core/slot-gpio.c, line 322), always a value is assigned:

-snip-
if (gpio_invert)

                *gpio_invert = !gpiod_is_active_low(desc);

-snap-

Quoted 7 lines

So returning to mmc_of_parse, the bool should always have an initialized value. Apart from some error handling, the bool is used immediately in the xor expression and results in setting MMC_CAP2_CD_ACTIVE_HIGH bit, or not.

I also cannot see a code path, where gpio_invert is used without a call to mmc_gpiod_request_cd.

Would be nice, if you could point me to what I'm missing.

mmc_gpiod_request_cd can return without ever reaching the line where the value is assigned to gpio_invert:

desc = devm_gpiod_get_index(host->parent, con_id, idx, GPIOD_IN); if (IS_ERR(desc))

	return PTR_ERR(desc);

This can happen when the host controller doesn't use a GPIO for card detection (but instead uses a dedicated pin). In this case devm_gpiod_get_index will return -ENOENT.

Quoted 10 lines

drivers/mmc/core/host.c int mmc_of_parse(struct mmc_host *host) · line 405

Quoted 8 lines

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

 	if (!host->parent || !host->parent->of_node)
 		return 0;

@@ -401,6 +401,7 @@ int mmc_of_parse(struct mmc_host *host)
 	else
 		cap_invert = false;
 
+	gpio_invert = false;
 	ret = mmc_gpiod_request_ro(host, "wp", 0, false, 0, &gpio_invert);

Same here. The functions always assigns a value when a pointer is given.

Same thing, the function can return before gpio_invert is ever assigned to.

Quoted 2 lines

(And this change is unrelated to the reporters problem, so should be fixed with a dedicated patch.)

Since both flags are set wrong for the exact same reason, and they're both regressions in 3.18, I thought it would make sense to fix them both in one patch. (Especially since I think it's cleaner to split the gpio_invert variable into cd_gpio_invert and ro_gpio_invert, and I was going to do that in the patch I'd send.) Let me know if you still think I should send separate patches for them.

I ran into this issue as well. Seems that a card-detect flag (MMC_CAP2_CD_ACTIVE_HIGH) can currently be set based on an uninitialized variable, which can lead to the card being reported as not present. This patch fixes it for me:

drivers/mmc/core/host.c +2 -1

index 03c53b72a2d6..f0e187682d3b 100644
@@ -311,7 +311,7 @@ int mmc_of_parse(struct mmc_host *host)
        struct device_node *np;
        u32 bus_width;
        int len, ret;
-       bool cap_invert, gpio_invert;
+       bool cap_invert, gpio_invert = false;
 
        if (!host->parent || !host->parent->of_node)
                return 0;

@@ -401,6 +401,7 @@ int mmc_of_parse(struct mmc_host *host)
        else
                cap_invert = false;
 
+       gpio_invert = false;
        ret = mmc_gpiod_request_ro(host, "wp", 0, false, 0, &gpio_invert);
        if (ret) {
                if (ret == -EPROBE_DEFER)

Let me know if this also fixes it for you, and I'll send in a proper patch.

Since commit 89168b489915 ("mmc: core: restore detect line inversion semantics"), the SD card on i.MX28 (and possibly other) devices isn't detected and booting stops at:

[    4.120617] Waiting for root device /dev/mmcblk0p3...

This is caused by the MMC_CAP2_CD_ACTIVE_HIGH flag being set incorrectly when the host controller doesn't use a GPIO for card detection (but instead uses a dedicated pin). In this case mmc_gpiod_request_cd() will return before assigning to the gpio_invert variable, leaving the variable uninitialized. The variable then gets used to set the flag. This patch fixes the issue by making sure gpio_invert is set to false when a GPIO isn't used. After this patch, i.MX28 boots fine.

The MMC_CAP2_RO_ACTIVE_HIGH (write protect) flag is also set incorrectly for the exact same reason (it uses the same uninitialized variable), so this patch fixes that too.

Fixes 89168b489915 ("mmc: core: restore detect line inversion semantics")

Reported-by Stefan Wahren <[email protected]>

Signed-off-by Kristina Mart?enko <[email protected]>

Since commit 89168b489915 ("mmc: core: restore detect line inversion semantics"), the SD card on i.MX28 (and possibly other) devices isn't detected and booting stops at:

[    4.120617] Waiting for root device /dev/mmcblk0p3...

This is caused by the MMC_CAP2_CD_ACTIVE_HIGH flag being set incorrectly when the host controller doesn't use a GPIO for card detection (but instead uses a dedicated pin). In this case mmc_gpiod_request_cd() will return before assigning to the gpio_invert variable, leaving the variable uninitialized. The variable then gets used to set the flag. This patch fixes the issue by making sure gpio_invert is set to false when a GPIO isn't used. After this patch, i.MX28 boots fine.

The MMC_CAP2_RO_ACTIVE_HIGH (write protect) flag is also set incorrectly for the exact same reason (it uses the same uninitialized variable), so this patch fixes that too.

Fixes 89168b489915 ("mmc: core: restore detect line inversion semantics")

Reported-by Stefan Wahren <[email protected]>

Signed-off-by Kristina Mart?enko <[email protected]>