[PATCH v2] mmc: sdhci: add quirk for broken write protect detection

[PATCH v2] mmc: sdhci: add quirk for broken write protect detection

[Eli Billauer]

The write protection signal is absent on a board based upon Xilinx' Zynq
processor ("ZyBo"). This leads the kernel to think that the MicroSD card is
write protected, and causes a kernel panic during boot, as root fails to
mount RW.

This patch adds a quirk and an optional OF property, sdhci,broken-wp to
work around this issue.

Signed-off-by: Eli Billauer <eli.billauer@gmail.com>
---
 Documentation/devicetree/bindings/mmc/mmc.txt |    2 ++
 drivers/mmc/host/sdhci-pltfm.c                |    4 +++-
 drivers/mmc/host/sdhci.c                      |    2 ++
 include/linux/mmc/sdhci.h                     |    2 ++
 4 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
index 458b57f..83a39c1 100644
--- a/Documentation/devicetree/bindings/mmc/mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc.txt
@@ -21,6 +21,8 @@ Optional properties:
   below for the case, when a GPIO is used for the CD line
 - wp-inverted: when present, polarity on the WP line is inverted. See the note
   below for the case, when a GPIO is used for the WP line
+- broken-wp: when present, no indication of write protection is available,
+  and write protection is assumed always off.
 - max-frequency: maximum operating clock frequency
 - no-1-8-v: when present, denotes that 1.8v card voltage is not supported on
   this system, even if the controller claims it is.
diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
index bef250e..dac20af 100644
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
@@ -80,7 +80,9 @@ void sdhci_get_of_property(struct platform_device *pdev)
 		    bus_width == 1))
 			host->quirks |= SDHCI_QUIRK_FORCE_1_BIT_DATA;
 
-		if (sdhci_of_wp_inverted(np))
+		if (of_get_property(np, "sdhci,broken-wp", NULL))
+			host->quirks2 |= SDHCI_QUIRK2_BROKEN_WRITE_PROTECT;
+		else if (sdhci_of_wp_inverted(np))
 			host->quirks |= SDHCI_QUIRK_INVERTED_WRITE_PROTECT;
 
 		if (of_get_property(np, "broken-cd", NULL))
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 9ddef47..6c77f2e 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1657,6 +1657,8 @@ static int sdhci_check_ro(struct sdhci_host *host)
 		is_readonly = 0;
 	else if (host->ops->get_ro)
 		is_readonly = host->ops->get_ro(host);
+	else if (host->quirks2 & SDHCI_QUIRK2_BROKEN_WRITE_PROTECT)
+		is_readonly = 0;
 	else
 		is_readonly = !(sdhci_readl(host, SDHCI_PRESENT_STATE)
 				& SDHCI_WRITE_PROTECT);
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index 362927c4..0f6ef9d 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -100,6 +100,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK2_BROKEN_HOST_CONTROL		(1<<5)
 /* Controller does not support HS200 */
 #define SDHCI_QUIRK2_BROKEN_HS200			(1<<6)
+/* There is no indication for write protection at all, assume RW */
+#define SDHCI_QUIRK2_BROKEN_WRITE_PROTECT		(1<<7)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
-- 
1.7.2.3

Re: [PATCH v2] mmc: sdhci: add quirk for broken write protect detection

[Sören Brinkmann]

Hi Eli,

On Sun, 2014-03-02 at 01:20PM +0200, Eli Billauer wrote:
> The write protection signal is absent on a board based upon Xilinx' Zynq
> processor ("ZyBo"). This leads the kernel to think that the MicroSD card is
> write protected, and causes a kernel panic during boot, as root fails to
> mount RW.

I talked to some people here at Xilinx. According to them, you have the
option to pin out the WP signal, which would mean the board needs to
tie/connect the signal properly. Or, if you select to not pin out the WP
signal, it should be tied to 0 within the chip.
Currently, I have some doubts that is the case, since Mike reported the
same issue, but would you mind double checking?
In theory the signal should default to logic zero which would at most
require to add the, already existing, 'wp-inverted' quirk when using
micro-sd on Zynq.

	Thanks,
	Sören

Re: [PATCH v2] mmc: sdhci: add quirk for broken write protect detection

[Eli Billauer]

Hello Sören,

wp-inverted solves the practical problem indeed, and fools the driver 
into thinking that the card has an inverted write protection sensor, and 
the logic zero that it finds in the hardware register means that the 
card isn't write protected.

I'm insisting on this patch, because I think that the device tree should 
describe the hardware as it is, and not fool the driver into behaving 
the way we want it to. These tricks always bite back later on.

Regards,
    Eli

On 04/03/14 21:26, Sören Brinkmann wrote:
> Hi Eli,
>
> On Sun, 2014-03-02 at 01:20PM +0200, Eli Billauer wrote:
>    
>> The write protection signal is absent on a board based upon Xilinx' Zynq
>> processor ("ZyBo"). This leads the kernel to think that the MicroSD card is
>> write protected, and causes a kernel panic during boot, as root fails to
>> mount RW.
>>      
> I talked to some people here at Xilinx. According to them, you have the
> option to pin out the WP signal, which would mean the board needs to
> tie/connect the signal properly. Or, if you select to not pin out the WP
> signal, it should be tied to 0 within the chip.
> Currently, I have some doubts that is the case, since Mike reported the
> same issue, but would you mind double checking?
> In theory the signal should default to logic zero which would at most
> require to add the, already existing, 'wp-inverted' quirk when using
> micro-sd on Zynq.
>
> 	Thanks,
> 	Sören
>
>
>
>    

Re: [PATCH v2] mmc: sdhci: add quirk for broken write protect detection

[Sören Brinkmann]

On Tue, 2014-03-04 at 10:06PM +0200, Eli Billauer wrote:
> Hello Sören,
> 
> wp-inverted solves the practical problem indeed, and fools the
> driver into thinking that the card has an inverted write protection
> sensor, and the logic zero that it finds in the hardware register
> means that the card isn't write protected.
> 
> I'm insisting on this patch, because I think that the device tree
> should describe the hardware as it is, and not fool the driver into
> behaving the way we want it to. These tricks always bite back later
> on.
Well, why is broken-wp more accurate than wp-inverted? Strictly
speaking the WP is there and working, it's just tied off to some value
you want to have interpreted the other way.
Anyway, seems like this is solvable with wp-inverted and whether the
additional quirk is needed I leave to others do decide.

	Sören

Re: [PATCH v2] mmc: sdhci: add quirk for broken write protect detection

[Mike Looijmans]

On 03/04/2014 10:00 PM, Sören Brinkmann wrote:
> On Tue, 2014-03-04 at 10:06PM +0200, Eli Billauer wrote:
>> Hello Sören,
>>
>> wp-inverted solves the practical problem indeed, and fools the
>> driver into thinking that the card has an inverted write protection
>> sensor, and the logic zero that it finds in the hardware register
>> means that the card isn't write protected.
>>
>> I'm insisting on this patch, because I think that the device tree
>> should describe the hardware as it is, and not fool the driver into
>> behaving the way we want it to. These tricks always bite back later
>> on.
> Well, why is broken-wp more accurate than wp-inverted? Strictly
> speaking the WP is there and working, it's just tied off to some value
> you want to have interpreted the other way.
> Anyway, seems like this is solvable with wp-inverted and whether the
> additional quirk is needed I leave to others do decide.

I've begged for this patch - or a similar one - to be included too, because on 
our boards, the "wp" value appears to be sort of random. Out of 5 prototype 
boards, 3 would only boot with wp-inverted while the other 2 wouldn't boot 
with wp-inverted set.

In our case I really don't know (and I don't care either) to which logic level 
the wp happens to think it's wired. I just want to be able to tell the driver 
that the WP line is 
free-floating-and-might-have-any-random-value-at-any-given-moment which is a 
bit long, so I'd go for disable-wp instead.

Mike.


Met vriendelijke groet / kind regards,

Mike Looijmans

TOPIC Embedded Systems
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: (+31) (0) 499 33 69 79
Telefax:  (+31) (0) 499 33 69 70
E-mail: mike.looijmans@topic.nl
Website: www.topic.nl

Please consider the environment before printing this e-mail

Re: [PATCH v2] mmc: sdhci: add quirk for broken write protect detection

[Sören Brinkmann]

On Thu, 2014-03-06 at 02:31PM +0100, Mike Looijmans wrote:
> On 03/04/2014 10:00 PM, Sören Brinkmann wrote:
> >On Tue, 2014-03-04 at 10:06PM +0200, Eli Billauer wrote:
> >>Hello Sören,
> >>
> >>wp-inverted solves the practical problem indeed, and fools the
> >>driver into thinking that the card has an inverted write protection
> >>sensor, and the logic zero that it finds in the hardware register
> >>means that the card isn't write protected.
> >>
> >>I'm insisting on this patch, because I think that the device tree
> >>should describe the hardware as it is, and not fool the driver into
> >>behaving the way we want it to. These tricks always bite back later
> >>on.
> >Well, why is broken-wp more accurate than wp-inverted? Strictly
> >speaking the WP is there and working, it's just tied off to some value
> >you want to have interpreted the other way.
> >Anyway, seems like this is solvable with wp-inverted and whether the
> >additional quirk is needed I leave to others do decide.
> 
> I've begged for this patch - or a similar one - to be included too,
> because on our boards, the "wp" value appears to be sort of random.
> Out of 5 prototype boards, 3 would only boot with wp-inverted while
> the other 2 wouldn't boot with wp-inverted set.
> 
> In our case I really don't know (and I don't care either) to which
> logic level the wp happens to think it's wired. I just want to be
> able to tell the driver that the WP line is
> free-floating-and-might-have-any-random-value-at-any-given-moment
> which is a bit long, so I'd go for disable-wp instead.

Could you provide the design you use and give more details? According to
the people I talked to, the signal should never float, unless you pin it
out and don't drive it.
Actually, you should open a support case for this. It is not supposed to
happen.

	Sören

Re: [PATCH v2] mmc: sdhci: add quirk for broken write protect detection

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 2772 bytes --]

Hi,

On 03/06/2014 05:42 PM, Sören Brinkmann wrote:
> On Thu, 2014-03-06 at 02:31PM +0100, Mike Looijmans wrote:
>> On 03/04/2014 10:00 PM, Sören Brinkmann wrote:
>>> On Tue, 2014-03-04 at 10:06PM +0200, Eli Billauer wrote:
>>>> Hello Sören,
>>>>
>>>> wp-inverted solves the practical problem indeed, and fools the
>>>> driver into thinking that the card has an inverted write protection
>>>> sensor, and the logic zero that it finds in the hardware register
>>>> means that the card isn't write protected.
>>>>
>>>> I'm insisting on this patch, because I think that the device tree
>>>> should describe the hardware as it is, and not fool the driver into
>>>> behaving the way we want it to. These tricks always bite back later
>>>> on.
>>> Well, why is broken-wp more accurate than wp-inverted? Strictly
>>> speaking the WP is there and working, it's just tied off to some value
>>> you want to have interpreted the other way.
>>> Anyway, seems like this is solvable with wp-inverted and whether the
>>> additional quirk is needed I leave to others do decide.
>>
>> I've begged for this patch - or a similar one - to be included too,
>> because on our boards, the "wp" value appears to be sort of random.
>> Out of 5 prototype boards, 3 would only boot with wp-inverted while
>> the other 2 wouldn't boot with wp-inverted set.
>>
>> In our case I really don't know (and I don't care either) to which
>> logic level the wp happens to think it's wired. I just want to be
>> able to tell the driver that the WP line is
>> free-floating-and-might-have-any-random-value-at-any-given-moment
>> which is a bit long, so I'd go for disable-wp instead.
> 
> Could you provide the design you use and give more details? According to
> the people I talked to, the signal should never float, unless you pin it
> out and don't drive it.
> Actually, you should open a support case for this. It is not supposed to
> happen.

we have got this from Mike (we couldn't reply because he has lost this email
thread.

Mike:
"I think I found the issue. In ps7_init.c as generated by the tools, it sets the "WP" pin not to EMIO, but to MIO 0. We use pin 0 for a status LED.

# devmem 0XF8000830
0x002E0000

Register 0XF8000830 is SD0_WP_CD_SEL, and 0x002E0000 sets CD to pin 46 and WP to pin "0", not to EMIO as I specified in the design.
"

Eli: Maybe you have the same issue as Mike. Can you please check it?

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH v2] mmc: sdhci: add quirk for broken write protect detection

[Eli Billauer]

Hello Michal.

The Zybo board doesn't have any WP pin connected to the MicroSD card. 
There is no physical possibility for the processor to know whether the 
card is write-protected or not.

As I mentioned earlier, the practical problem can be worked around by 
inverting the polarity of the WP bit, using wp-inverted. Practically 
speaking, there's no need for this patch.

I insisted on this patch, because I think that the device tree should 
reflect the hardware as it is, and not contain tricks for fooling the 
driver into doing what we want. But I guess this wasn't a reason good 
enough for adding yet another quirk (to the existing 38 or so).

Regards,
    Eli

On 20/03/14 14:26, Michal Simek wrote:
> we have got this from Mike (we couldn't reply because he has lost this email
> thread.
>
> Mike:
> "I think I found the issue. In ps7_init.c as generated by the tools, it sets the "WP" pin not to EMIO, but to MIO 0. We use pin 0 for a status LED.
>
> # devmem 0XF8000830
> 0x002E0000
>
> Register 0XF8000830 is SD0_WP_CD_SEL, and 0x002E0000 sets CD to pin 46 and WP to pin "0", not to EMIO as I specified in the design.
> "
>
> Eli: Maybe you have the same issue as Mike. Can you please check it?
>    

Re: [PATCH v2] mmc: sdhci: add quirk for broken write protect detection

[Mike Looijmans]

I totally agree with Eli. The devicetree should read something like "WP is not 
present" (which will be the case on all micro SD readers). Having "WP is 
inverted" there is just misleading.

On our boards, I use MIO0 as a "heartbeat" LED. Combined with a quirk in XPS 
and/or Vivado that the pinmuxing for the WP line is actually routed to MIO0 
when you request it to be unrouted or to EMIO, this made the WP line on our 
systems appear as semi-random.

Mike.

On 03/20/2014 01:39 PM, Eli Billauer wrote:
> Hello Michal.
>
> The Zybo board doesn't have any WP pin connected to the MicroSD card. There is
> no physical possibility for the processor to know whether the card is
> write-protected or not.
>
> As I mentioned earlier, the practical problem can be worked around by
> inverting the polarity of the WP bit, using wp-inverted. Practically speaking,
> there's no need for this patch.
>
> I insisted on this patch, because I think that the device tree should reflect
> the hardware as it is, and not contain tricks for fooling the driver into
> doing what we want. But I guess this wasn't a reason good enough for adding
> yet another quirk (to the existing 38 or so).
>
> Regards,
>     Eli
>
> On 20/03/14 14:26, Michal Simek wrote:
>> we have got this from Mike (we couldn't reply because he has lost this email
>> thread.
>>
>> Mike:
>> "I think I found the issue. In ps7_init.c as generated by the tools, it sets
>> the "WP" pin not to EMIO, but to MIO 0. We use pin 0 for a status LED.
>>
>> # devmem 0XF8000830
>> 0x002E0000
>>
>> Register 0XF8000830 is SD0_WP_CD_SEL, and 0x002E0000 sets CD to pin 46 and
>> WP to pin "0", not to EMIO as I specified in the design.
>> "
>>
>> Eli: Maybe you have the same issue as Mike. Can you please check it?
>
>



Met vriendelijke groet / kind regards,

Mike Looijmans

TOPIC Embedded Systems
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: (+31) (0) 499 33 69 79
Telefax:  (+31) (0) 499 33 69 70
E-mail: mike.looijmans@topic.nl
Website: www.topic.nl

Please consider the environment before printing this e-mail