[PATCH 1/2] sdhci: add quirk for lack of 1.8v support

[PATCH 1/2] sdhci: add quirk for lack of 1.8v support

[Daniel Drake]

The OLPC XO-1.75 laptop includes a SDHCI controller which is 1.8v
capable, and it truthfully reports so in its capabilities. This
alternate voltage is used for driving new "UHS-I" SD cards at their
full speed.

However, what the controller doesn't know is that the motherboard
physically doesn't have a 1.8v supply available.

Add a quirk so that systems such as this one can override disable
1.8v support, adding support for UHS-I cards (by running them at
3.3v).

This avoids a problem where the system would first try to run the
card at 1.8v, fail, and then not be able to fully reset the card
to retry at the normal 3.3v voltage.

This is more appropriate than using the MISSING_CAPS quirk, which
is intended for cases where the SDHCI controller is actually lying
about its capabilities, and would force us to somehow override both
caps words from another source.

Signed-off-by: Daniel Drake <dsd@laptop.org>
---
 drivers/mmc/host/sdhci.c  | 4 ++++
 include/linux/mmc/sdhci.h | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index c7851c0..7273523 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2865,6 +2865,10 @@ int sdhci_add_host(struct sdhci_host *host)
 		caps[1] &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 |
 		       SDHCI_SUPPORT_DDR50);
 
+	if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V)
+		caps[1] &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 |
+		       SDHCI_SUPPORT_DDR50);
+
 	/* Any UHS-I mode in caps implies SDR12 and SDR25 support. */
 	if (caps[1] & (SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 |
 		       SDHCI_SUPPORT_DDR50))
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index 1edcb4d..1a42747 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -92,6 +92,8 @@ struct sdhci_host {
 
 #define SDHCI_QUIRK2_HOST_OFF_CARD_ON			(1<<0)
 #define SDHCI_QUIRK2_HOST_NO_CMD23			(1<<1)
+/* The system physically doesn't support 1.8v, even if the host does */
+#define SDHCI_QUIRK2_NO_1_8_V				(1<<2)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
-- 
1.7.11.7

Re: [PATCH 1/2] sdhci: add quirk for lack of 1.8v support

[Philip Rakity]

Daniel 

The same effect can be achieved by setting the quirk
SDHCI_QUIRK_MISSING_CAPS
reading caps[0] and removing from caps[1] the UHS values in the controller specific code for your board.

What is the reason you cannot do this ?

Philip

On Nov 25, 2012, at 6:01 PM, Daniel Drake <dsd@laptop.org> wrote:

> The OLPC XO-1.75 laptop includes a SDHCI controller which is 1.8v
> capable, and it truthfully reports so in its capabilities. This
> alternate voltage is used for driving new "UHS-I" SD cards at their
> full speed.
> 
> However, what the controller doesn't know is that the motherboard
> physically doesn't have a 1.8v supply available.
> 
> Add a quirk so that systems such as this one can override disable
> 1.8v support, adding support for UHS-I cards (by running them at
> 3.3v).
> 
> This avoids a problem where the system would first try to run the
> card at 1.8v, fail, and then not be able to fully reset the card
> to retry at the normal 3.3v voltage.
> 
> This is more appropriate than using the MISSING_CAPS quirk, which
> is intended for cases where the SDHCI controller is actually lying
> about its capabilities, and would force us to somehow override both
> caps words from another source.
> 
> Signed-off-by: Daniel Drake <dsd@laptop.org>
> ---
> drivers/mmc/host/sdhci.c  | 4 ++++
> include/linux/mmc/sdhci.h | 2 ++
> 2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index c7851c0..7273523 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2865,6 +2865,10 @@ int sdhci_add_host(struct sdhci_host *host)
> 		caps[1] &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 |
> 		       SDHCI_SUPPORT_DDR50);
> 
> +	if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V)
> +		caps[1] &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 |
> +		       SDHCI_SUPPORT_DDR50);
> +
> 	/* Any UHS-I mode in caps implies SDR12 and SDR25 support. */
> 	if (caps[1] & (SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 |
> 		       SDHCI_SUPPORT_DDR50))
> diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
> index 1edcb4d..1a42747 100644
> --- a/include/linux/mmc/sdhci.h
> +++ b/include/linux/mmc/sdhci.h
> @@ -92,6 +92,8 @@ struct sdhci_host {
> 
> #define SDHCI_QUIRK2_HOST_OFF_CARD_ON			(1<<0)
> #define SDHCI_QUIRK2_HOST_NO_CMD23			(1<<1)
> +/* The system physically doesn't support 1.8v, even if the host does */
> +#define SDHCI_QUIRK2_NO_1_8_V				(1<<2)
> 
> 	int irq;		/* Device IRQ */
> 	void __iomem *ioaddr;	/* Mapped address */
> -- 
> 1.7.11.7
> 

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Re: [PATCH 1/2] sdhci: add quirk for lack of 1.8v support

[Daniel Drake]

On Sun, Nov 25, 2012 at 12:36 PM, Philip Rakity <prakity@nvidia.com> wrote:
> The same effect can be achieved by setting the quirk
> SDHCI_QUIRK_MISSING_CAPS
> reading caps[0] and removing from caps[1] the UHS values in the controller specific code for your board.
>
> What is the reason you cannot do this ?

The same reasons we already touched upon in the other thread.
http://marc.info/?l=linux-mmc&m=135240077826454&w=2
http://marc.info/?l=linux-mmc&m=135240181826846&w=2

To reiterate (I know this has been broken out over a couple of weeks):

There is no controller-specific or board-specific code. We are working
with the device tree. It must be done generically.

SDHCI_QUIRK_MISSING_CAPS seems to be aimed at cases where the
controller does not report caps, or it is lying about the caps it
does/doesn't support. In this case our controller is being truthful,
so this does not feel appropriate.

Also, the only realistic way that I can see to combine
SDHCI_QUIRK_MISSING_CAPS with the device tree would be to put the
caps/caps1 override data in the device tree. This just seems ugly, and
feels like it is taking the wrong approach of again suggesting that
the controller is providing bad data, when in this case actually it is
correctly saying that 1.8v is supported (by the controller - it
obviously cannot speak for external components).

We don't have a regulator to work with here.

Thanks
Daniel

Re: [PATCH 1/2] sdhci: add quirk for lack of 1.8v support

[Philip Rakity]

[-- Attachment #1.1: Type: text/plain, Size: 2735 bytes --]

Reviewed-by. Philip Rakity <prakity-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

----- Reply message -----
From: "Daniel Drake" <dsd-2X9k7bc8m7Mdnm+yROfE0A@public.gmane.org>
To: "Philip Rakity" <prakity-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Cc: "cjb-2X9k7bc8m7Mdnm+yROfE0A@public.gmane.org" <cjb-2X9k7bc8m7Mdnm+yROfE0A@public.gmane.org>, "linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "jrudholm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org" <jrudholm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, "devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org" <devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org>
Subject: [PATCH 1/2] sdhci: add quirk for lack of 1.8v support
Date: Sun, Nov 25, 2012 18:50



On Sun, Nov 25, 2012 at 12:36 PM, Philip Rakity <prakity-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> The same effect can be achieved by setting the quirk
> SDHCI_QUIRK_MISSING_CAPS
> reading caps[0] and removing from caps[1] the UHS values in the controller specific code for your board.
>
> What is the reason you cannot do this ?

The same reasons we already touched upon in the other thread.
http://marc.info/?l=linux-mmc&m=135240077826454&w=2
http://marc.info/?l=linux-mmc&m=135240181826846&w=2

To reiterate (I know this has been broken out over a couple of weeks):

There is no controller-specific or board-specific code. We are working
with the device tree. It must be done generically.

SDHCI_QUIRK_MISSING_CAPS seems to be aimed at cases where the
controller does not report caps, or it is lying about the caps it
does/doesn't support. In this case our controller is being truthful,
so this does not feel appropriate.

Also, the only realistic way that I can see to combine
SDHCI_QUIRK_MISSING_CAPS with the device tree would be to put the
caps/caps1 override data in the device tree. This just seems ugly, and
feels like it is taking the wrong approach of again suggesting that
the controller is providing bad data, when in this case actually it is
correctly saying that 1.8v is supported (by the controller - it
obviously cannot speak for external components).

We don't have a regulator to work with here.

Thanks
Daniel

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

[-- Attachment #1.2: Type: text/html, Size: 3577 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 1/2] sdhci: add quirk for lack of 1.8v support

[Philip Rakity]

On Nov 25, 2012, at 6:01 PM, Daniel Drake <dsd@laptop.org> wrote:

> The OLPC XO-1.75 laptop includes a SDHCI controller which is 1.8v
> capable, and it truthfully reports so in its capabilities. This
> alternate voltage is used for driving new "UHS-I" SD cards at their
> full speed.
> 
> However, what the controller doesn't know is that the motherboard
> physically doesn't have a 1.8v supply available.
> 
> Add a quirk so that systems such as this one can override disable
> 1.8v support, adding support for UHS-I cards (by running them at
> 3.3v).
> 
> This avoids a problem where the system would first try to run the
> card at 1.8v, fail, and then not be able to fully reset the card
> to retry at the normal 3.3v voltage.
> 
> This is more appropriate than using the MISSING_CAPS quirk, which
> is intended for cases where the SDHCI controller is actually lying
> about its capabilities, and would force us to somehow override both
> caps words from another source.
> 
> Signed-off-by: Daniel Drake <dsd@laptop.org>
> ---
> drivers/mmc/host/sdhci.c  | 4 ++++
> include/linux/mmc/sdhci.h | 2 ++
> 2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index c7851c0..7273523 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2865,6 +2865,10 @@ int sdhci_add_host(struct sdhci_host *host)
> 		caps[1] &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 |
> 		       SDHCI_SUPPORT_DDR50);
> 
> +	if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V)
> +		caps[1] &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 |
> +		       SDHCI_SUPPORT_DDR50);
> +
> 	/* Any UHS-I mode in caps implies SDR12 and SDR25 support. */
> 	if (caps[1] & (SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 |
> 		       SDHCI_SUPPORT_DDR50))
> diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
> index 1edcb4d..1a42747 100644
> --- a/include/linux/mmc/sdhci.h
> +++ b/include/linux/mmc/sdhci.h
> @@ -92,6 +92,8 @@ struct sdhci_host {
> 
> #define SDHCI_QUIRK2_HOST_OFF_CARD_ON			(1<<0)
> #define SDHCI_QUIRK2_HOST_NO_CMD23			(1<<1)
> +/* The system physically doesn't support 1.8v, even if the host does */
> +#define SDHCI_QUIRK2_NO_1_8_V				(1<<2)
> 
> 	int irq;		/* Device IRQ */
> 	void __iomem *ioaddr;	/* Mapped address */
> -- 
> 1.7.11.7
> 

Reviewed-by:  Philip Rakity <prakity@nvidia.com>

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Re: [PATCH 1/2] sdhci: add quirk for lack of 1.8v support

[Chris Ball]

Hi,

On Sun, Nov 25 2012, Daniel Drake wrote:
> The OLPC XO-1.75 laptop includes a SDHCI controller which is 1.8v
> capable, and it truthfully reports so in its capabilities. This
> alternate voltage is used for driving new "UHS-I" SD cards at their
> full speed.
>
> However, what the controller doesn't know is that the motherboard
> physically doesn't have a 1.8v supply available.
>
> Add a quirk so that systems such as this one can override disable
> 1.8v support, adding support for UHS-I cards (by running them at
> 3.3v).
>
> This avoids a problem where the system would first try to run the
> card at 1.8v, fail, and then not be able to fully reset the card
> to retry at the normal 3.3v voltage.
>
> This is more appropriate than using the MISSING_CAPS quirk, which
> is intended for cases where the SDHCI controller is actually lying
> about its capabilities, and would force us to somehow override both
> caps words from another source.
>
> Signed-off-by: Daniel Drake <dsd@laptop.org>

Thanks, pushed to mmc-next for 3.8.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child