[PATCH 1/4] mmc: mmc_card_keep_power cleanups

[PATCH 1/4] mmc: mmc_card_keep_power cleanups

[Ohad Ben-Cohen]

mmc_card_is_powered_resumed is a mouthful; instead, simply use
mmc_card_keep_power, which also better reflects the meaning of
the predicate.

Employ mmc_card_keep_power() where possible.

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

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 286e6ce..de8a3d3 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1723,7 +1723,7 @@ int mmc_suspend_host(struct mmc_host *host)
 	}
 	mmc_bus_put(host);
 
-	if (!err && !(host->pm_flags & MMC_PM_KEEP_POWER))
+	if (!err && !mmc_card_keep_power(host))
 		mmc_power_off(host);
 
 	return err;
@@ -1741,7 +1741,7 @@ int mmc_resume_host(struct mmc_host *host)
 
 	mmc_bus_get(host);
 	if (host->bus_ops && !host->bus_dead) {
-		if (!(host->pm_flags & MMC_PM_KEEP_POWER)) {
+		if (!mmc_card_keep_power(host)) {
 			mmc_power_up(host);
 			mmc_select_voltage(host, host->ocr);
 			/*
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 5c4a54d..97a6182 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -616,7 +616,7 @@ static int mmc_sdio_suspend(struct mmc_host *host)
 		}
 	}
 
-	if (!err && host->pm_flags & MMC_PM_KEEP_POWER) {
+	if (!err && mmc_card_keep_power(host)) {
 		mmc_claim_host(host);
 		sdio_disable_wide(host->card);
 		mmc_release_host(host);
@@ -636,10 +636,10 @@ static int mmc_sdio_resume(struct mmc_host *host)
 	mmc_claim_host(host);
 
 	/* No need to reinitialize powered-resumed nonremovable cards */
-	if (mmc_card_is_removable(host) || !mmc_card_is_powered_resumed(host))
+	if (mmc_card_is_removable(host) || !mmc_card_keep_power(host))
 		err = mmc_sdio_init_card(host, host->ocr, host->card,
-				 (host->pm_flags & MMC_PM_KEEP_POWER));
-	else if (mmc_card_is_powered_resumed(host)) {
+					mmc_card_keep_power(host));
+	else if (mmc_card_keep_power(host)) {
 		/* We may have switched to 1-bit mode during suspend */
 		err = sdio_enable_4bit_bus(host->card);
 		if (err > 0) {
@@ -682,7 +682,7 @@ static int mmc_sdio_power_restore(struct mmc_host *host)
 
 	mmc_claim_host(host);
 	ret = mmc_sdio_init_card(host, host->ocr, host->card,
-			(host->pm_flags & MMC_PM_KEEP_POWER));
+				mmc_card_keep_power(host));
 	if (!ret && host->sdio_irqs)
 		mmc_signal_sdio_irq(host);
 	mmc_release_host(host);
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index bcb793e..cccb5cf 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -321,7 +321,7 @@ static inline int mmc_card_is_removable(struct mmc_host *host)
 	return !(host->caps & MMC_CAP_NONREMOVABLE) && mmc_assume_removable;
 }
 
-static inline int mmc_card_is_powered_resumed(struct mmc_host *host)
+static inline int mmc_card_keep_power(struct mmc_host *host)
 {
 	return host->pm_flags & MMC_PM_KEEP_POWER;
 }
-- 
1.7.1

[PATCH 2/4] mmc: do not switch to 1-bit mode if not required

[Ohad Ben-Cohen]

6b5eda36 followed SDIO spec part E1 section 8, which states that
in case SDIO interrupts are being used to wake up a suspended host,
then it is required to switch to 1-bit mode before stopping the clock.

Before switching to 1-bit mode (or back to 4-bit mode on resume),
make sure that SDIO interrupts are really being used to wake the host.

This is helpful for devices which have an external irq line (e.g.
wl1271), and do not use SDIO interrupts to wake up the host.

In this case, switching to 1-bit mode (and back to 4-bit mode on resume)
is not necessary.

Reported-by: Eliad Peller <eliad@wizery.com>
Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
 drivers/mmc/core/sdio.c  |    4 ++--
 include/linux/mmc/host.h |    4 ++++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 97a6182..78be7d9 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -616,7 +616,7 @@ static int mmc_sdio_suspend(struct mmc_host *host)
 		}
 	}
 
-	if (!err && mmc_card_keep_power(host)) {
+	if (!err && mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host)) {
 		mmc_claim_host(host);
 		sdio_disable_wide(host->card);
 		mmc_release_host(host);
@@ -639,7 +639,7 @@ static int mmc_sdio_resume(struct mmc_host *host)
 	if (mmc_card_is_removable(host) || !mmc_card_keep_power(host))
 		err = mmc_sdio_init_card(host, host->ocr, host->card,
 					mmc_card_keep_power(host));
-	else if (mmc_card_keep_power(host)) {
+	else if (mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host)) {
 		/* We may have switched to 1-bit mode during suspend */
 		err = sdio_enable_4bit_bus(host->card);
 		if (err > 0) {
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index cccb5cf..4f705eb 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -326,5 +326,9 @@ static inline int mmc_card_keep_power(struct mmc_host *host)
 	return host->pm_flags & MMC_PM_KEEP_POWER;
 }
 
+static inline int mmc_card_wake_sdio_irq(struct mmc_host *host)
+{
+	return host->pm_flags & MMC_PM_WAKE_SDIO_IRQ;
+}
 #endif
 
-- 
1.7.1

[PATCH 3/4] mmc: add MMC_QUIRK_DISABLE_CD

[Ohad Ben-Cohen]

006ebd5d introduced sdio_disable_cd(), which disconnects the pull-up
resistor on CD/DAT[3] (pin 1) of the card.

Make it possible to start using sdio_disable_cd() by introducing
MMC_QUIRK_DISABLE_CD.

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

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 78be7d9..2118df2 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -181,7 +181,7 @@ static int sdio_disable_cd(struct mmc_card *card)
 	int ret;
 	u8 ctrl;
 
-	if (!card->cccr.disable_cd)
+	if (!mmc_card_disable_cd(card))
 		return 0;
 
 	ret = mmc_io_rw_direct(card, 0, 0, SDIO_CCCR_IF, 0, &ctrl);
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 8ce0827..3a3c079 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -80,8 +80,7 @@ struct sdio_cccr {
 				low_speed:1,
 				wide_bus:1,
 				high_power:1,
-				high_speed:1,
-				disable_cd:1;
+				high_speed:1;
 };
 
 struct sdio_cis {
@@ -121,6 +120,7 @@ struct mmc_card {
 						/* for byte mode */
 #define MMC_QUIRK_NONSTD_SDIO	(1<<2)		/* non-standard SDIO card attached */
 						/* (missing CIA registers) */
+#define MMC_QUIRK_DISABLE_CD	(1<<3)		/* disconnect CD/DAT[3] resistor */
 
 	unsigned int		erase_size;	/* erase size in sectors */
  	unsigned int		erase_shift;	/* if erase unit is power 2 */
@@ -174,6 +174,11 @@ static inline int mmc_blksz_for_byte_mode(const struct mmc_card *c)
 	return c->quirks & MMC_QUIRK_BLKSZ_FOR_BYTE_MODE;
 }
 
+static inline int mmc_card_disable_cd(const struct mmc_card *c)
+{
+	return c->quirks & MMC_QUIRK_DISABLE_CD;
+}
+
 #define mmc_card_name(c)	((c)->cid.prod_name)
 #define mmc_card_id(c)		(dev_name(&(c)->dev))
 
-- 
1.7.1

[PATCH 4/4] mmc: add MMC_QUIRK_NONSTD_FUNC_IF

[Ohad Ben-Cohen]

Use MMC_QUIRK_NONSTD_FUNC_IF to ignore the "SDIO Standard Function interface
code" as indicated by the card's FBR, and instead treat all functions as
non-standard interfaces.

This is required to prevent standard drivers (e.g. btsdio) from facing
errors when trying to communicate with SDIO cards that erroneously
indicate standard function interface codes.

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

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 2118df2..bee7641 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -16,6 +16,7 @@
 #include <linux/mmc/card.h>
 #include <linux/mmc/sdio.h>
 #include <linux/mmc/sdio_func.h>
+#include <linux/mmc/sdio_ids.h>
 
 #include "core.h"
 #include "bus.h"
@@ -31,6 +32,11 @@ static int sdio_read_fbr(struct sdio_func *func)
 	int ret;
 	unsigned char data;
 
+	if (mmc_card_nonstd_func_interface(func->card)) {
+		func->class = SDIO_CLASS_NONE;
+		return 0;
+	}
+
 	ret = mmc_io_rw_direct(func->card, 0, 0,
 		SDIO_FBR_BASE(func->num) + SDIO_FBR_STD_IF, 0, &data);
 	if (ret)
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 3a3c079..40441df 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -121,6 +121,7 @@ struct mmc_card {
 #define MMC_QUIRK_NONSTD_SDIO	(1<<2)		/* non-standard SDIO card attached */
 						/* (missing CIA registers) */
 #define MMC_QUIRK_DISABLE_CD	(1<<3)		/* disconnect CD/DAT[3] resistor */
+#define MMC_QUIRK_NONSTD_FUNC_IF (1<<4)		/* SDIO card has nonstd function interfaces */
 
 	unsigned int		erase_size;	/* erase size in sectors */
  	unsigned int		erase_shift;	/* if erase unit is power 2 */
@@ -179,6 +180,11 @@ static inline int mmc_card_disable_cd(const struct mmc_card *c)
 	return c->quirks & MMC_QUIRK_DISABLE_CD;
 }
 
+static inline int mmc_card_nonstd_func_interface(const struct mmc_card *c)
+{
+	return c->quirks & MMC_QUIRK_NONSTD_FUNC_IF;
+}
+
 #define mmc_card_name(c)	((c)->cid.prod_name)
 #define mmc_card_id(c)		(dev_name(&(c)->dev))
 
-- 
1.7.1

Re: [PATCH 3/4] mmc: add MMC_QUIRK_DISABLE_CD

[David Vrabel]

On 04/01/2011 21:10, Ohad Ben-Cohen wrote:
> 006ebd5d introduced sdio_disable_cd(), which disconnects the pull-up
> resistor on CD/DAT[3] (pin 1) of the card.
> 
> Make it possible to start using sdio_disable_cd() by introducing
> MMC_QUIRK_DISABLE_CD.

How are you intending to use this?  Whether this pull up should be
enabled or disabled is determined by the board (not the card).  The
pull-up should be disabled if the board has a pull-up external to the
card, otherwise it must remain enabled.

David

Re: [PATCH 3/4] mmc: add MMC_QUIRK_DISABLE_CD

[Ohad Ben-Cohen]

On Wed, Jan 5, 2011 at 12:09 AM, David Vrabel <david.vrabel@csr.com> wrote:
> How are you intending to use this?

diff --git a/arch/arm/mach-omap2/board-zoom-peripherals.c
b/arch/arm/mach-omap2/board-zoom-peripherals.c
index 9db9203..ae69ba9 100644
--- a/arch/arm/mach-omap2/board-zoom-peripherals.c
+++ b/arch/arm/mach-omap2/board-zoom-peripherals.c
@@ -19,6 +19,7 @@
 #include <linux/regulator/fixed.h>
 #include <linux/wl12xx.h>
 #include <linux/mmc/host.h>
+#include <linux/mmc/card.h>

 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
@@ -196,6 +197,18 @@ struct wl12xx_platform_data omap_zoom_wlan_data
__initdata = {
        .board_ref_clock = 1,
 };

+static void zoom_wl1271_init_card(struct mmc_card *card)
+{
+       /*
+        * Tell SDIO core to disconnect the pull-up resistor on CD/DAT[3]
+        * (pin 1) of the wl1271 card, which is not required since we
+        * already have OMAP pulling up this line, and we don't need any
+        * card detection functionality. As a result, this may save some
+        * power.
+        */
+       card->quirks |= MMC_QUIRK_DISABLE_CD;
+}
+
 static struct omap2_hsmmc_info mmc[] __initdata = {
        {
                .name           = "external",
@@ -220,6 +233,7 @@ static struct omap2_hsmmc_info mmc[] __initdata = {
                .gpio_wp        = -EINVAL,
                .gpio_cd        = -EINVAL,
                .nonremovable   = true,
+               .init_card      = zoom_wl1271_init_card,
        },
        {}      /* Terminator */
 };


> Whether this pull up should be
> enabled or disabled is determined by the board (not the card).  The
> pull-up should be disabled if the board has a pull-up external to the
> card, otherwise it must remain enabled.

Agree.

Re: [PATCH 3/4] mmc: add MMC_QUIRK_DISABLE_CD

[Ohad Ben-Cohen]

On Wed, Jan 5, 2011 at 12:13 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>> Whether this pull up should be
>> enabled or disabled is determined by the board (not the card).  The
>> pull-up should be disabled if the board has a pull-up external to the
>> card, otherwise it must remain enabled.
>
> Agree.

It might make more sense to use a host CAP though for this (i.e.
MMC_CAP_DISABLE_CD).

What do you think ?

Thanks,
Ohad.

Re: [PATCH 4/4] mmc: add MMC_QUIRK_NONSTD_FUNC_IF

[Pierre Tardy]

On Tue, Jan 4, 2011 at 10:10 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Use MMC_QUIRK_NONSTD_FUNC_IF to ignore the "SDIO Standard Function interface
> code" as indicated by the card's FBR, and instead treat all functions as
> non-standard interfaces.
Hi Ohad,

This sounds good, and may solve this problem we have with wl1271.
Then, how do you tell the mmc stack the wl1271 has the quirk
MMC_QUIRK_NONSTD_FUNC_IF?
Regards,

Pierre

Re: [PATCH 3/4] mmc: add MMC_QUIRK_DISABLE_CD

[David Vrabel]

On 04/01/2011 22:27, Ohad Ben-Cohen wrote:
> On Wed, Jan 5, 2011 at 12:13 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>>> Whether this pull up should be
>>> enabled or disabled is determined by the board (not the card).  The
>>> pull-up should be disabled if the board has a pull-up external to the
>>> card, otherwise it must remain enabled.
>>
>> Agree.
> 
> It might make more sense to use a host CAP though for this (i.e.
> MMC_CAP_DISABLE_CD).

Could we unconditionally disable the card's internal pull-up?  A board
that's using DAT3 for card detection should have a switchable pull-down
/and/ a switchable pull-up.

This would cause regressions on boards that aren't properly designed and
have omitted the pull-up.  Are there boards like this?

David

Re: [PATCH 4/4] mmc: add MMC_QUIRK_NONSTD_FUNC_IF

[Ohad Ben-Cohen]

Hi Pierre,

On Wed, Jan 5, 2011 at 12:48 AM, Pierre Tardy <tardyp@gmail.com> wrote:
> This sounds good, and may solve this problem we have with wl1271.
> Then, how do you tell the mmc stack the wl1271 has the quirk
> MMC_QUIRK_NONSTD_FUNC_IF?

Something like this:

diff --git a/arch/arm/mach-omap2/board-zoom-peripherals.c b/arch/arm/mach-omap2/
index 5a8311f..f9f9c8a 100644
--- a/arch/arm/mach-omap2/board-zoom-peripherals.c
+++ b/arch/arm/mach-omap2/board-zoom-peripherals.c
@@ -19,6 +19,7 @@
 #include <linux/regulator/fixed.h>
 #include <linux/wl12xx.h>
 #include <linux/mmc/host.h>
+#include <linux/mmc/card.h>

 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
@@ -196,6 +197,17 @@ struct wl12xx_platform_data omap_zoom_wlan_data __initdata
        .board_ref_clock = 1,
 };

+static void zoom_wl1271_init_card(struct mmc_card *card)
+{
+       /*
+        * Tell SDIO core to ignore the standard SDIO function interface
+        * codes indicated by the wl1271. This is required because the
+        * wl1271 erronouesly indicates its first function as a standard
+        * Bluetooth SDIO interface.
+        */
+       card->quirks |= MMC_QUIRK_NONSTD_FUNC_IF;
+}
+
 static struct omap2_hsmmc_info mmc[] __initdata = {
        {
                .name           = "external",
@@ -221,6 +233,7 @@ static struct omap2_hsmmc_info mmc[] __initdata = {
                .gpio_wp        = -EINVAL,
                .gpio_cd        = -EINVAL,
                .nonremovable   = true,
+               .init_card      = zoom_wl1271_init_card,
        },
        {}      /* Terminator */
 };

This applies on the ZOOM board. in your case it would be a bit
different, but the core principle should stay the same.

Thanks,
Ohad.

Re: [PATCH 3/4] mmc: add MMC_QUIRK_DISABLE_CD

[Ohad Ben-Cohen]

On Wed, Jan 5, 2011 at 1:04 AM, David Vrabel <david.vrabel@csr.com> wrote:
> Could we unconditionally disable the card's internal pull-up?

Good question. Empirically, I can say that we have done this with many
types of boards and never encountered any issue.

Re: [PATCH 4/4] mmc: add MMC_QUIRK_NONSTD_FUNC_IF

[Pierre Tardy]

>  static struct omap2_hsmmc_info mmc[] __initdata = {
>        {
>                .name           = "external",
> @@ -221,6 +233,7 @@ static struct omap2_hsmmc_info mmc[] __initdata = {
>                .gpio_wp        = -EINVAL,
>                .gpio_cd        = -EINVAL,
>                .nonremovable   = true,
> +               .init_card      = zoom_wl1271_init_card,
>        },
>        {}      /* Terminator */
>  };
>
> This applies on the ZOOM board. in your case it would be a bit
> different, but the core principle should stay the same.
Mmmh...
There is no such platform data in intel_mid platforms.. Sound like
another platform hack will be needed for wl1271 to be integrated. :-(

Our first idea to resolve this problem was to hardcode wl1273 vendor
id/product id in a black list table near read_fbr...

Regards,
Pierre

Re: [PATCH 4/4] mmc: add MMC_QUIRK_NONSTD_FUNC_IF

[Ohad Ben-Cohen]

On Wed, Jan 5, 2011 at 1:25 AM, Pierre Tardy <tardyp@gmail.com> wrote:
> There is no such platform data in intel_mid platforms.. Sound like
> another platform hack will be needed for wl1271 to be integrated. :-(

John (cc'ed) ported this to your platform and can share the solution
with you; IIRC the idea was the same: identify the card according to
the slot id (this is reasonable because the card is nonremovable) and
set it as MMC_QUIRK_NONSTD_FUNC_IF.

> Our first idea to resolve this problem was to hardcode wl1273 vendor
> id/product id in a black list table near read_fbr...

I'm not sure we want to hardcode such details in the SDIO core itself
(think of how it would look if more devices will show up..).

Thanks,
Ohad.

Re: [PATCH 4/4] mmc: add MMC_QUIRK_NONSTD_FUNC_IF

[Pierre Tardy]

On Wed, Jan 5, 2011 at 12:34 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> On Wed, Jan 5, 2011 at 1:25 AM, Pierre Tardy <tardyp@gmail.com> wrote:
>> There is no such platform data in intel_mid platforms.. Sound like
>> another platform hack will be needed for wl1271 to be integrated. :-(
>
> John (cc'ed) ported this to your platform and can share the solution
> with you; IIRC the idea was the same: identify the card according to
> the slot id (this is reasonable because the card is nonremovable) and
> set it as MMC_QUIRK_NONSTD_FUNC_IF.
Well, we dont do it like this in intel_mid. Every platform quirk has
to come somehow from the firmware (SFI), and there is no such
descriptor to my knowledge. Lets see what John is proposing...


>> Our first idea to resolve this problem was to hardcode wl1273 vendor
>> id/product id in a black list table near read_fbr...
>
> I'm not sure we want to hardcode such details in the SDIO core itself
> (think of how it would look if more devices will show up..).

Yes, that's why I thought of a table, we might want to make this more
generic, and, as you are proposing more than one quirk those few days
for cards, we could add those quirks in struct sdio_device_id, like
they do for PCI...


The thing is that the semantic of your solution is wrong. You want to say:
Hey! wl1271 is broken
and you actually say:
Hey! the card in slot named "external" of omap4 zoom is brocken.


Regards,
Pierre

Re: [PATCH 4/4] mmc: add MMC_QUIRK_NONSTD_FUNC_IF

[Ohad Ben-Cohen]

On Wed, Jan 5, 2011 at 11:10 AM, Pierre Tardy <tardyp@gmail.com> wrote:
> Well, we dont do it like this in intel_mid. Every platform quirk has
> to come somehow from the firmware (SFI), and there is no such
> descriptor to my knowledge. Lets see what John is proposing...

I think that John shared it with you this morning. I haven't looked
into your platform too much, but if it's using upstream code, let's
take the discussion to the list (by rebasing John's solution and
posting it here).

> Yes, that's why I thought of a table, we might want to make this more
> generic, and, as you are proposing more than one quirk those few days
> for cards, we could add those quirks in struct sdio_device_id, like
> they do for PCI...

Interesting. Care to send a patch for review ?

> The thing is that the semantic of your solution is wrong. You want to say:
> Hey! wl1271 is broken
> and you actually say:
> Hey! the card in slot named "external" of omap4 zoom is brocken.

Not exactly; I do say that the specific wl1271 card is quirky, but I
must set it early before the card is probed, so I use the host
controller's quirks callback ->init_card().

I agree that a quirks table, if done right, might be easier and won't
involve the board in the process; let's try to explore it.

Thanks,
Ohad.
>
>
> Regards,
> Pierre
>

Re: [PATCH 4/4] mmc: add MMC_QUIRK_NONSTD_FUNC_IF

[Pierre Tardy]

On Wed, Jan 5, 2011 at 10:40 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> On Wed, Jan 5, 2011 at 11:10 AM, Pierre Tardy <tardyp@gmail.com> wrote:
>> Well, we dont do it like this in intel_mid. Every platform quirk has
>> to come somehow from the firmware (SFI), and there is no such
>> descriptor to my knowledge. Lets see what John is proposing...
>
> I think that John shared it with you this morning. I haven't looked
> into your platform too much, but if it's using upstream code, let's
> take the discussion to the list (by rebasing John's solution and
> posting it here).
>
>> Yes, that's why I thought of a table, we might want to make this more
>> generic, and, as you are proposing more than one quirk those few days
>> for cards, we could add those quirks in struct sdio_device_id, like
>> they do for PCI...
>
> Interesting. Care to send a patch for review ?
Well... TI is selling quirky hw, would make sense that you handle this. :-)

Here is a first attempt anyway...

From: Pierre Tardy <pierre.tardy@intel.com>
Date: Wed, 5 Jan 2011 13:21:02 +0100
Subject: [PATCH] mmc: add per device quirk placeholder

Some cards have quirks valid for every platforms
using current platform quirk hooks leads to a lot
of code and debug duplication.

So we inspire a bit from what exists in PCI subsystem
and do out own per vendorid/deviceid quirk
We still drop the complexity of the pci quirk system
(with special section tables, and so on)
That can be added later if needed.


Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>
---
 drivers/mmc/core/Makefile |    3 ++-
 drivers/mmc/core/core.h   |    2 ++
 drivers/mmc/core/quirks.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 drivers/mmc/core/sdio.c   |    1 +
 include/linux/mmc/card.h  |    2 ++
 5 files changed, 50 insertions(+), 1 deletions(-)
 create mode 100644 drivers/mmc/core/quirks.c

diff --git a/drivers/mmc/core/Makefile b/drivers/mmc/core/Makefile
index 86b4791..6395019 100644
--- a/drivers/mmc/core/Makefile
+++ b/drivers/mmc/core/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_MMC)		+= mmc_core.o
 mmc_core-y			:= core.o bus.o host.o \
 				   mmc.o mmc_ops.o sd.o sd_ops.o \
 				   sdio.o sdio_ops.o sdio_bus.o \
-				   sdio_cis.o sdio_io.o sdio_irq.o
+				   sdio_cis.o sdio_io.o sdio_irq.o \
+				   quirks.o

 mmc_core-$(CONFIG_DEBUG_FS)	+= debugfs.o
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index 026c975..36d9a25 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -61,6 +61,8 @@ int mmc_attach_mmc(struct mmc_host *host, u32 ocr);
 int mmc_attach_sd(struct mmc_host *host, u32 ocr);
 int mmc_attach_sdio(struct mmc_host *host, u32 ocr);

+void mmc_fixup_device(struct mmc_card *card);
+
 /* Module parameters */
 extern int use_spi_crc;

diff --git a/drivers/mmc/core/quirks.c b/drivers/mmc/core/quirks.c
new file mode 100644
index 0000000..d75e68c
--- /dev/null
+++ b/drivers/mmc/core/quirks.c
@@ -0,0 +1,43 @@
+/*
+ *  This file contains work-arounds for many known sdio hardware
+ *  bugs.
+ *
+ *  Copyright (c) 2011 Pierre Tardy <tardyp@gmail.com>
+ *  Inspired from pci fixup code:
+ *  Copyright (c) 1999 Martin Mares <mj@ucw.cz>
+ *
+ */
+
+#include <linux/types.h>
+#include <linux/kernel.h>
+#include <linux/mmc/card.h>
+#include <linux/mod_devicetable.h>
+
+/*
+ *  The world is not perfect and supplies us with broken mmc/sdio devices.
+ *  For at least a part of these bugs we need a work-around
+ */
+
+struct mmc_fixup {
+	u16 vendor, device;	/* You can use SDIO_ANY_ID here of course */
+	void (*hook)(struct mmc_card *card);
+};
+
+static const struct mmc_fixup mmc_fixup_methods[] = {
+	{ 0 }
+};
+
+void mmc_fixup_device(struct mmc_card *card)
+{
+	const struct mmc_fixup *f;
+
+	for (f = mmc_fixup_methods; f->hook; f++) {
+		if ((f->vendor == card->cis.vendor || f->vendor == (u16) SDIO_ANY_ID) &&
+		    (f->device == card->cis.device || f->device == (u16) SDIO_ANY_ID)) {
+			dev_dbg(&card->dev, "calling %pF\n", f->hook);
+			f->hook(card);
+		}
+	}
+}
+EXPORT_SYMBOL(mmc_fixup_device);
+
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index db18ef5..d290a1c 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -386,6 +386,7 @@ static int mmc_sdio_init_card(struct mmc_host
*host, u32 ocr,
 	 */
 	if (host->ops->init_card)
 		host->ops->init_card(host, card);
+	mmc_fixup_device(card);

 	/*
 	 * For native busses:  set card RCA and quit open drain mode.
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 5071eb1..3fe9db0 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -149,6 +149,8 @@ struct mmc_card {
 	struct dentry		*debugfs_root;
 };

+void mmc_fixup_device(struct mmc_card *dev);
+
 #define mmc_card_mmc(c)		((c)->type == MMC_TYPE_MMC)
 #define mmc_card_sd(c)		((c)->type == MMC_TYPE_SD)
 #define mmc_card_sdio(c)	((c)->type == MMC_TYPE_SDIO)
-- 
1.7.0.4

Re: [PATCH 4/4] mmc: add MMC_QUIRK_NONSTD_FUNC_IF

[Ohad Ben-Cohen]

Hi Pierre,

On Wed, Jan 5, 2011 at 2:28 PM, Pierre Tardy <tardyp@gmail.com> wrote:
> Well... TI is selling quirky hw, would make sense that you handle this. :-)

Heh I'd happily take over - just say the word :)

> Subject: [PATCH] mmc: add per device quirk placeholder
>
> Some cards have quirks valid for every platforms
> using current platform quirk hooks leads to a lot
> of code and debug duplication.
>
> So we inspire a bit from what exists in PCI subsystem
> and do out own per vendorid/deviceid quirk
> We still drop the complexity of the pci quirk system
> (with special section tables, and so on)
> That can be added later if needed.

I like the idea. It would allow us to handle card quirks without
involving board code (and obviously we can't rely on drivers for that
because that may be too late).

> +struct mmc_fixup {
> +       u16 vendor, device;     /* You can use SDIO_ANY_ID here of course */
> +       void (*hook)(struct mmc_card *card);

Do we really need to hook up code with quirky cards ? why not just
directly indicate the exact MMC_QUIRK_* instead (just like USB is
doing) ? that may simplify the whole thing.

So instead of the ->hook() function pointer, we can just put a uint
quirks member,

> +};
> +
> +static const struct mmc_fixup mmc_fixup_methods[] = {
> +       { 0 }

Fill it up here,

> +};
> +
> +void mmc_fixup_device(struct mmc_card *card)
> +{
> +       const struct mmc_fixup *f;
> +
> +       for (f = mmc_fixup_methods; f->hook; f++) {
> +               if ((f->vendor == card->cis.vendor || f->vendor == (u16) SDIO_ANY_ID) &&
> +                   (f->device == card->cis.device || f->device == (u16) SDIO_ANY_ID)) {
> +                       dev_dbg(&card->dev, "calling %pF\n", f->hook);
> +                       f->hook(card);

And then here instead of calling a dedicated ->hook() function, we can
just do something like this:

card->quirks |= f->quirks;

This way we don't need to add a function for every quirky card.

And if we will happen to need it in the future we can always bring it
back in, but right now we really only need to specify the relevant
QUIRK(s).

Thanks,
Ohad.

Re: [PATCH 3/4] mmc: add MMC_QUIRK_DISABLE_CD

[Ohad Ben-Cohen]

On Wed, Jan 5, 2011 at 1:04 AM, David Vrabel <david.vrabel@csr.com> wrote:
> This would cause regressions on boards that aren't properly designed and
> have omitted the pull-up.  Are there boards like this?

I'm not sure there are.

All hosts shall provide pull-up resistors on all data lines DAT[3:0]
as described in section 6 of the SD physical specification. A board
that doesn't pull up DAT3, and instead relies on the card's internal
pull-up, sound very quirky to me.

So we might really be able to unconditionally disable the card's DAT3
internal pull-up. We can do that for cards that specifically want it,
or, we can even consider a bolder approach where we do that for all
the nonremovable SDIO cards (which therefore don't need the card
detection functionality anyway).

What do you think ?

Thanks,
Ohad.

Re: [PATCH 4/4] mmc: add MMC_QUIRK_NONSTD_FUNC_IF

[Pierre Tardy]

On Fri, Jan 7, 2011 at 12:16 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Hi Pierre,
>
> On Wed, Jan 5, 2011 at 2:28 PM, Pierre Tardy <tardyp@gmail.com> wrote:
>> Well... TI is selling quirky hw, would make sense that you handle this. :-)
>
> Heh I'd happily take over - just say the word :)
Well anyway, I needed this for my power/clock gating stuff also.

> Do we really need to hook up code with quirky cards ? why not just
> directly indicate the exact MMC_QUIRK_* instead (just like USB is
> doing) ? that may simplify the whole thing.
Semi agree, see the new patches for example of needed hooks, and how I
made it generic.
The problem I was trying to resolve is add anti clock gating quirk for
every sdio but wl1271

> Fill it up here,
hope example patches answer your issues.

Pierre