[PATCH] sdio: add CD disable support

[PATCH] sdio: add CD disable support

[Ohad Ben-Cohen]

From: Ohad Ben-Cohen <ohad@wizery.com>

To save power, the pull-up resistor on CD/DAT[3] (pin 1)
of the card can be disconnected. This is desired, e.g.,
with embedded SDIO devices which do not rely on this pin
for card detection.

Signed-off-by: Ohad Ben-Cohen  <ohad@wizery.com>
---
 drivers/mmc/core/sdio.c  |   32 ++++++++++++++++++++++++++++++++
 include/linux/mmc/card.h |    3 ++-
 2 files changed, 34 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index fb99ccf..4c4c9a9 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -165,6 +165,31 @@ static int sdio_enable_wide(struct mmc_card *card)
 }
 
 /*
+ * Disconnect the pull-up resistor on CD/DAT[3] if requested by the card.
+ * If card detection is not needed, this can save some power.
+ */
+static int sdio_disable_cd(struct mmc_card *card)
+{
+	int ret;
+	u8 ctrl;
+
+	if (!card->cccr.disable_cd)
+		return 0;
+
+	ret = mmc_io_rw_direct(card, 0, 0, SDIO_CCCR_IF, 0, &ctrl);
+	if (ret)
+		return ret;
+
+	ctrl |= SDIO_BUS_CD_DISABLE;
+
+	ret = mmc_io_rw_direct(card, 1, 0, SDIO_CCCR_IF, ctrl, NULL);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+/*
  * Test if the card supports high-speed mode and, if so, switch to it.
  */
 static int sdio_enable_hs(struct mmc_card *card)
@@ -392,6 +417,13 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
 		goto remove;
 
 	/*
+	 * Disconnect card detection pull-up resistor (if requested by card).
+	 */
+	err = sdio_disable_cd(card);
+	if (err)
+		goto remove;
+
+	/*
 	 * Initialize (but don't add) all present functions.
 	 */
 	for (i = 0;i < funcs;i++) {
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 403aa50..82a8488 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -62,7 +62,8 @@ struct sdio_cccr {
 				low_speed:1,
 				wide_bus:1,
 				high_power:1,
-				high_speed:1;
+				high_speed:1,
+				disable_cd:1;
 };
 
 struct sdio_cis {
-- 
1.5.4.3

Re: [PATCH] sdio: add CD disable support

[Matt Fleming]

On Thu, Jul 30, 2009 at 02:54:08PM +0300, Ohad Ben-Cohen wrote:
> From: Ohad Ben-Cohen <ohad@wizery.com>
> 
> To save power, the pull-up resistor on CD/DAT[3] (pin 1)
> of the card can be disconnected. This is desired, e.g.,
> with embedded SDIO devices which do not rely on this pin
> for card detection.
> 
> Signed-off-by: Ohad Ben-Cohen  <ohad@wizery.com>
> ---
>  drivers/mmc/core/sdio.c  |   32 ++++++++++++++++++++++++++++++++
>  include/linux/mmc/card.h |    3 ++-
>  2 files changed, 34 insertions(+), 1 deletions(-)
> 

Do you have a follow-up patch to make use of this new functionality? I
can't see anywhere where disable_cd is set.

Re: [PATCH] sdio: add CD disable support

[David Vrabel]

Ohad Ben-Cohen wrote:
> From: Ohad Ben-Cohen <ohad@wizery.com>
> 
> To save power, the pull-up resistor on CD/DAT[3] (pin 1)
> of the card can be disconnected. This is desired, e.g.,
> with embedded SDIO devices which do not rely on this pin
> for card detection.

Platforms may rely on the card's pull-up and not fit/configure an
external one.  There may need to be a way for host controller drivers to
 say this and prevent the disabling of the card's DAT3 pull-up.

> Signed-off-by: Ohad Ben-Cohen  <ohad@wizery.com>
> ---
>  drivers/mmc/core/sdio.c  |   32 ++++++++++++++++++++++++++++++++
>  include/linux/mmc/card.h |    3 ++-
>  2 files changed, 34 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index fb99ccf..4c4c9a9 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -165,6 +165,31 @@ static int sdio_enable_wide(struct mmc_card *card)
>  }
>  
>  /*
> + * Disconnect the pull-up resistor on CD/DAT[3] if requested by the card.
> + * If card detection is not needed, this can save some power.
> + */

The first sentence of this comment doesn't make sense.  The decision to
disable the pull-up is for the controller driver to make, not that card.

David
-- 
David Vrabel, Senior Software Engineer, Drivers
CSR, Churchill House, Cambridge Business Park,  Tel: +44 (0)1223 692562
Cowley Road, Cambridge, CB4 0WZ                 http://www.csr.com/


'member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom'

Re: [PATCH] sdio: add CD disable support

[Ohad Ben-Cohen]

On Thu, Jul 30, 2009 at 3:45 PM, Matt Fleming<matt@console-pimps.org> wrote:
> Do you have a follow-up patch to make use of this new functionality?

Well, sort of:

The patch sets disable_cd for TI wl1271 embedded wlan sdio device on a
ZOOM2 platform.

Since the patch is very board/device specific, we first route it via
internal maintainers, so it is not posted yet.

It's nothing fancy:

diff --git a/arch/arm/mach-omap2/mmc-twl4030.c
b/arch/arm/mach-omap2/mmc-twl4030.c
index 5b64bf1..caa8b16 100644
--- a/arch/arm/mach-omap2/mmc-twl4030.c
+++ b/arch/arm/mach-omap2/mmc-twl4030.c
@@ -81,6 +81,7 @@ static struct embedded_sdio_data omap_wifi_emb_data = {
 		.wide_bus       = 1,
 		.high_power     = 0,
 		.high_speed     = 0,
+		.disable_cd	= 1,
 	},
 	.funcs  = wifi_func_array,
 	.num_funcs = 2,

Re: [PATCH] sdio: add CD disable support

[Matt Fleming]

On Thu, Jul 30, 2009 at 06:55:14PM +0300, Ohad Ben-Cohen wrote:
> On Thu, Jul 30, 2009 at 3:45 PM, Matt Fleming<matt@console-pimps.org> wrote:
> > Do you have a follow-up patch to make use of this new functionality?
> 
> Well, sort of:
> 
> The patch sets disable_cd for TI wl1271 embedded wlan sdio device on a
> ZOOM2 platform.
> 
> Since the patch is very board/device specific, we first route it via
> internal maintainers, so it is not posted yet.
> 
> It's nothing fancy:
> 
> diff --git a/arch/arm/mach-omap2/mmc-twl4030.c
> b/arch/arm/mach-omap2/mmc-twl4030.c
> index 5b64bf1..caa8b16 100644
> --- a/arch/arm/mach-omap2/mmc-twl4030.c
> +++ b/arch/arm/mach-omap2/mmc-twl4030.c
> @@ -81,6 +81,7 @@ static struct embedded_sdio_data omap_wifi_emb_data = {
>  		.wide_bus       = 1,
>  		.high_power     = 0,
>  		.high_speed     = 0,
> +		.disable_cd	= 1,
>  	},
>  	.funcs  = wifi_func_array,
>  	.num_funcs = 2,

Ah, that's fair enough then. I just wanted to make sure that a patch
that used this functionality was in the pipeline :-)

As David has already said, both comments need fixing up. It is the host
controller driver that decides whether to disable the DAT[3] pull-up,
not the card. If you make those changes then you can add my Acked-by.

Re: [PATCH] sdio: add CD disable support

[Ohad Ben-Cohen]

Hi David,

Thank you for your comments.

On Thu, Jul 30, 2009 at 3:54 PM, David Vrabel<david.vrabel@csr.com> wrote:
> Platforms may rely on the card's pull-up and not fit/configure an
> external one.  There may need to be a way for host controller drivers to
>  say this and prevent the disabling of the card's DAT3 pull-up.

Currently the only way to really disable the CD using this patch is by
setting the disable_cd bit on for a specific controller. We are doing so
for a specific controller to which an embedded sdio device is wired,
on certain boards. This is done using the embedded_sdio's .cccr field,
which is used instead of reading the card's cccr.
So as you correctly noted, card has no say here.

> The first sentence of this comment doesn't make sense.

Fixed;

I'll follow-up with a correction of the patch and will appreciate your review.

Thanks,
Ohad.

Re: [PATCH] sdio: add CD disable support

[Ohad Ben-Cohen]

Hi Matt,

On Thu, Jul 30, 2009 at 9:50 PM, Matt Fleming<matt@console-pimps.org> wrote:
> As David has already said, both comments need fixing up. It is the host
> controller driver that decides whether to disable the DAT[3] pull-up,
> not the card. If you make those changes then you can add my Acked-by.

I'm posting a follow-up patch with the fixes and your Ack,

will appreciate your second review.

Thanks,
Ohad.