| Message ID | 1393759200-22819-1-git-send-email-eli.billauer@gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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?
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 */
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(-)