mmc: add quirks.c file and CLK_GATING users
cover mmc: add quirks.c file and CLK_GATING users
Hello,
this is a followup patchset of discussion that happen before on linux-mmc mailing list We add a mmc/core/quirk.c file holding list of quirks per devices or more generic quirks e.g. all sdio devices have this quirk.
For now, only sdio card hook has been added. Hook for mmc or sd can be added in a followup patch. Just call mmc_fixup_device() in the proper place, after deviceid and vendorid is known.
Pierre Tardy (3):
mmc: add per device quirk placeholder
mmc: add MMC_QUIRK_BROKEN_CLK_GATING
mmc: remove anti clock gating quirk for wl1271
drivers/mmc/core/Makefile | 3 +- drivers/mmc/core/core.h | 2 + drivers/mmc/core/host.c | 5 +-- drivers/mmc/core/quirks.c | 83 +++++++++++++++++++++++++++++++++++++++++++++ drivers/mmc/core/sdio.c | 1 + include/linux/mmc/card.h | 3 ++ 6 files changed, 92 insertions(+), 5 deletions(-) create mode 100644 drivers/mmc/core/quirks.c
PATCH 1/3 mmc: add per device quirk placeholder
From: Pierre Tardy <[email protected]>
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 our 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.
--- drivers/mmc/core/Makefile | 3 +- drivers/mmc/core/core.h | 2 + drivers/mmc/core/quirks.c | 54 +++++++++++++++++++++++++++++++++++++++++++++ drivers/mmc/core/sdio.c | 1 + include/linux/mmc/card.h | 2 + 5 files changed, 61 insertions(+), 1 deletions(-)
drivers/mmc/core/Makefile +2 -1
@@ -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
drivers/mmc/core/core.h +2 -0
@@ -61,6 +61,8 @@ int mmc_attach_mmc(struct mmc_host *host);int mmc_attach_sd (struct mmc_host *host);int mmc_attach_sdio (struct mmc_host *host); +void mmc_fixup_device (struct mmc_card *card); +/* Module parameters */ extern int use_spi_crc;
drivers/mmc/core/quirks.c new file +54 -0
54 lines not on this page · raw message
drivers/mmc/core/sdio.c +1 -0
@@ -458,6 +458,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr, card = oldcard; } +mmc_fixup_device (card);if (card->type ==MMC_TYPE_SD_COMBO ) { err =mmc_sd_setup_card (host, card, oldcard !=NULL );
include/linux/mmc/card.h +2 -0
@@ -148,6 +148,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)
Signature · 1 line
1.7.1
Re: [PATCH 1/3] mmc: add per device quirk placeholder
Quoted 92 lines · Pierre Tardy
From: Pierre Tardy <[email protected]>
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 our 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 <[email protected]>---
drivers/mmc/core/Makefile | 3 +- drivers/mmc/core/core.h | 2 + drivers/mmc/core/quirks.c | 54 +++++++++++++++++++++++++++++++++++++++++++++ drivers/mmc/core/sdio.c | 1 + include/linux/mmc/card.h | 2 + 5 files changed, 61 insertions(+), 1 deletions(-)drivers/mmc/core/Makefile +2
-1@@ -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.ommc_core-$(CONFIG_DEBUG_FS) += debugfs.o
drivers/mmc/core/core.h +2
-0@@ -61,6 +61,8 @@ int mmc_attach_mmc(struct mmc_host *host);int mmc_attach_sd (struct mmc_host *host);int mmc_attach_sdio (struct mmc_host *host); +void mmc_fixup_device (struct mmc_card *card); +/* Module parameters */ extern int use_spi_crc;drivers/mmc/core/quirks.c new file +39
-0@@ -0,0 +1,54 @@ +/* +* This file contains work-arounds for many known sdio hardware +* bugs. +* +* Copyright (c) 2011 Pierre Tardy <[email protected]> +* Inspired from pci fixup code: +* Copyright (c) 1999 Martin Mares <[email protected]> +* +*/ + +#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,int data); +int data; +}; + +/* +* This hook just adds a quirk unconditionnally +*/ +static void add_quirk (struct mmc_card *card,int data) +{ + card->quirks |= data; +} + +static const struct mmc_fixup mmc_fixup_methods[] = { + {0 } +}; + +void (struct mmc_card *card)
drivers/mmc/core/quirks.c line 49
Quoted 10 lines
drivers/mmc/core/quirks.c new file +10
-0+{ +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, f->data);
hook is not a very nice name --- something more meaningful: vendor_fixup ?
Quoted 37 lines
drivers/mmc/core/quirks.c new file +5
-0+ } + } +} +EXPORT_SYMBOL (mmc_fixup_device); +drivers/mmc/core/sdio.c +1
-0@@ -458,6 +458,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr, card = oldcard; } +mmc_fixup_device (card);if (card->type ==MMC_TYPE_SD_COMBO ) { err =mmc_sd_setup_card (host, card, oldcard !=NULL );include/linux/mmc/card.h +2
-0@@ -148,6 +148,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)Signature · 6 lines
1.7.1 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] mmc: add per device quirk placeholder
Quoted 11 lines
Quoted 8 lines
+
+ 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, f->data);hook is not a very nice name --- something more meaningful: vendor_fixup ?
I just copied the pci/quirks.c way of doing. I think is more coherent to keep is this way, this name will forever appear only in 4 lines inside the kernel, so I dont care that much.
I'll change that if you really do care. :-)
Waiting for a few more comments to do all in a row.
Thanks,
Signature · 1 line
Pierre
Re: [PATCH 1/3] mmc: add per device quirk placeholder
In reply to Pierre TardyQuoted 11 lines · Pierre Tardy
From: Pierre Tardy <[email protected]>
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 our 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 this.
It's exactly what we need for the wl12xx, so thanks for pushing this.
I'm not sure about the necessity of the function hooks though - it just looks like complexity that we don't really need, and I would rather have this framework directly set the required quirks like USB is doing. I do see the benefit it provides you with the gating framework and the SDIO set/unset issue, and I don't have a cleaner alternative approach, so I guess we can live with it.
PATCH 2/3 mmc: add MMC_QUIRK_BROKEN_CLK_GATING
In reply to Pierre TardySome sdio card are not following sdio standard, and does not work when the sdio bus's clock is gated
To keep functionnality for all legacy driver, we turn this quirk on for every sdio card.
Drivers needs to disable the quirk manually when someone verified that their supported card works with clock gating.
--- drivers/mmc/core/host.c | 5 +---- drivers/mmc/core/quirks.c | 11 +++++++++++ include/linux/mmc/card.h | 1 + 3 files changed, 13 insertions(+), 4 deletions(-)
drivers/mmc/core/host.c +1 -4
@@ -160,10 +160,7 @@ static bool mmc_host_may_gate_card(struct mmc_card *card)* gate the clock, because there is somebody out there that may still * be using it. */ -if (mmc_card_sdio (card)) -return false; - -return true; +return !(card->quirks &MMC_QUIRK_BROKEN_CLK_GATING ); }/**
drivers/mmc/core/quirks.c +11 -0
@@ -32,7 +32,18 @@ static void add_quirk(struct mmc_card *card, int data) card->quirks |= data; } +/* +* This hook just adds a quirk for all sdio devices +*/ +static void add_quirk_for_sdio_devices (struct mmc_card *card,int data) +{ +if (mmc_card_sdio (card)) + card->quirks |= data; +} +static const struct mmc_fixup mmc_fixup_methods[] = { + { SDIO_ANY_ID, SDIO_ANY_ID, + add_quirk_for_sdio_devices, MMC_QUIRK_BROKEN_CLK_GATING } {0 } };
include/linux/mmc/card.h +1 -0
@@ -121,6 +121,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_BROKEN_CLK_GATING (1<<3)/* clock gating the sdio bus will make card fail */ unsigned int erase_size;/* erase size in sectors */ unsigned int erase_shift;/* if erase unit is power 2 */
Signature · 1 line
1.7.1
Re: [PATCH 2/3] mmc: add MMC_QUIRK_BROKEN_CLK_GATING
Quoted 20 lines · Pierre Tardy
Some sdio card are not following sdio standard, and does not work when the sdio bus's clock is gated
To keep functionnality for all legacy driver, we turn this quirk on for every sdio card.
Drivers needs to disable the quirk manually when someone verified that their supported card works with clock gating.Signed-off-by Pierre Tardy <[email protected]>---
drivers/mmc/core/host.c | 5 +---- drivers/mmc/core/quirks.c | 11 +++++++++++ include/linux/mmc/card.h | 1 + 3 files changed, 13 insertions(+), 4 deletions(-)drivers/mmc/core/host.c +0
-0
drivers/mmc/core/host.c static bool mmc_host_may_gate_card(struct mmc_card *card) · line 164
Quoted 9 lines
drivers/mmc/core/host.c +1
-4@@ -160,10 +160,7 @@ static bool mmc_host_may_gate_card(struct mmc_card *card)* gate the clock, because there is somebody out there that may still * be using it. */ -if (mmc_card_sdio (card)) -return false; - -return true; +return !(card->quirks &MMC_QUIRK_BROKEN_CLK_GATING ); }
Since the code is not called that often could you change the above code to:
drivers/mmc/core/host.c static bool mmc_host_may_gate_card(struct mmc_card *card) · line 166
Quoted 2 lines
drivers/mmc/core/host.c +0
-0if (mmc_card_sdio (card))return !(card->quirks &MMC_QUIRK_BROKEN_CLK_GATING );
return true;
more obvious that quirk only applies to sdio.
Quoted 44 lines
/**
drivers/mmc/core/quirks.c +11
-0@@ -32,7 +32,18 @@ static void add_quirk(struct mmc_card *card, int data) card->quirks |= data; } +/* +* This hook just adds a quirk for all sdio devices +*/ +static void add_quirk_for_sdio_devices (struct mmc_card *card,int data) +{ +if (mmc_card_sdio (card)) + card->quirks |= data; +} +static const struct mmc_fixup mmc_fixup_methods[] = { + { SDIO_ANY_ID, SDIO_ANY_ID, + add_quirk_for_sdio_devices, MMC_QUIRK_BROKEN_CLK_GATING } {0 } };include/linux/mmc/card.h +1
-0@@ -121,6 +121,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_BROKEN_CLK_GATING (1<<3)/* clock gating the sdio bus will make card fail */ unsigned int erase_size;/* erase size in sectors */ unsigned int erase_shift;/* if erase unit is power 2 */ Signature · 6 lines
1.7.1 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] mmc: add MMC_QUIRK_BROKEN_CLK_GATING
Quoted 6 lines
Since the code is not called that often could you change the above code to:
Quoted 2 lines
if (
mmc_card_sdio(card))
return !(card->quirks &MMC_QUIRK_BROKEN_CLK_GATING);
Well, The comment in the
Quoted 3 lines
return true;
more obvious that quirk only applies to sdio.
Disagree.Actually, before this code, there is a pretty long comment, that explain that this quirk only apply to sdio.
If we figure out later that some sdcard does not like clock gating either, we could add this quirk to those bad sdcard as well.
Here is the whole resulting function. I think this is clear enough:
static bool mmc_host_may_gate_card(struct mmc_card *card) {
/* If there is no card we may gate it */
if (!card)
return true;
/*
* Don't gate SDIO cards! These need to be clocked at all times
* since they may be independent systems generating interrupts
* and other events. The clock requests counter from the core will
* go down to zero since the core does not need it, but we will not
* gate the clock, because there is somebody out there that may still
* be using it.
*/
return !(card->quirks & MMC_QUIRK_BROKEN_CLK_GATING);
}
Pierre
Re: [PATCH 2/3] mmc: add MMC_QUIRK_BROKEN_CLK_GATING
Quoted 9 lines · Pierre Tardy
Quoted 6 lines
Since the code is not called that often could you change the above code to:
Quoted 2 lines
if (mmc_card_sdio(card)) return !(card->quirks & MMC_QUIRK_BROKEN_CLK_GATING);Well, The comment in the
Quoted 2 lines
return true;
Quoted 7 lines
Quoted 1 line
more obvious that quirk only applies to sdio.
Disagree.Actually, before this code, there is a pretty long comment, that explain that this quirk only apply to sdio.
If we figure out later that some sdcard does not like clock gating either, we could add this quirk to those bad sdcard as well.
I am okay with this -- if you resubmit -- just indicate the quirk is more generic than just sdio
Quoted 22 lines
Here is the whole resulting function. I think this is clear enough:
static bool
mmc_host_may_gate_card(struct mmc_card *card){/* If there is no card we may gate it */
if (!card)
return true;
/*
* Don't gate SDIO cards! These need to be clocked at all times
* since they may be independent systems generating interrupts
* and other events. The clock requests counter from the core will
* go down to zero since the core does not need it, but we will not
* gate the clock, because there is somebody out there that may still
* be using it.
*/
return !(card->quirks &MMC_QUIRK_BROKEN_CLK_GATING);}
Pierre
PATCH 3/3 mmc: remove anti clock gating quirk for wl1271
In reply to Pierre TardyFrom: Pierre Tardy <[email protected]>
--- drivers/mmc/core/quirks.c | 18 ++++++++++++++++++ 1 files changed, 18 insertions(+), 0 deletions(-)
drivers/mmc/core/quirks.c +18 -0
@@ -33,6 +33,14 @@ static void add_quirk(struct mmc_card *card, int data) }/* +* This hook just removes a quirk unconditionnally +*/ +static void remove_quirk_hook (struct mmc_card *card,int data) +{ + card->quirks &= ~data; +} + +/* * This hook just adds a quirk for all sdio devices */ static void add_quirk_for_sdio_devices (struct mmc_card *card,int data)
@@ -41,9 +49,19 @@ static void add_quirk_for_sdio_devices(struct mmc_card *card, int data) card->quirks |= data; } +#ifndef SDIO_VENDOR_ID_TI +#define SDIO_VENDOR_ID_TI 0x0097 +#endif + +#ifndef SDIO_DEVICE_ID_TI_WL1271 +#define SDIO_DEVICE_ID_TI_WL1271 0x4076 +#endif +static const struct mmc_fixup mmc_fixup_methods[] = { { SDIO_ANY_ID, SDIO_ANY_ID, add_quirk_for_sdio_devices, MMC_QUIRK_BROKEN_CLK_GATING } + {SDIO_VENDOR_ID_TI ,SDIO_DEVICE_ID_TI_WL1271 , + remove_quirk_hook,MMC_QUIRK_BROKEN_CLK_GATING }, {0 } };
Signature · 1 line
1.7.1
Re: [PATCH 3/3] mmc: remove anti clock gating quirk for wl1271
Hi Pierre,
Quoted 12 lines · Pierre Tardy
From: Pierre Tardy <[email protected]>
Signed-off-by Pierre Tardy <[email protected]>--- drivers/mmc/core/quirks.c | 18 ++++++++++++++++++ 1 files changed, 18 insertions(+), 0 deletions(-)drivers/mmc/core/quirks.c +0
-0
drivers/mmc/core/quirks.c static void add_quirk(struct mmc_card *card, int data) · line 41
Quoted 9 lines
drivers/mmc/core/quirks.c +6
-0@@ -33,6 +33,14 @@ static void add_quirk(struct mmc_card *card, int data) }/* +* This hook just removes a quirk unconditionnally +*/ +static void remove_quirk_hook (struct mmc_card *card,int data) +{ + card->quirks &= ~data; +}
This adds a defined-but-not-used warning. The MMC tree doesn't currently generate any warnings, so it would be good to avoid adding new ones, though I understand why it might make sense in this case.
drivers/mmc/core/quirks.c:30:13: warning: ‘add_quirk’ defined but not used drivers/mmc/core/quirks.c:38:13: warning: ‘remove_quirk_hook’ defined but not used
Quoted 15 lines
drivers/mmc/core/quirks.c +8
-0+ +/* * This hook just adds a quirk for all sdio devices */ static void add_quirk_for_sdio_devices (struct mmc_card *card,int data)@@ -41,9 +49,19 @@ static void add_quirk_for_sdio_devices(struct mmc_card *card, int data) card->quirks |= data; } +#ifndef SDIO_VENDOR_ID_TI +#define SDIO_VENDOR_ID_TI 0x0097 +#endif + +#ifndef SDIO_DEVICE_ID_TI_WL1271 +#define SDIO_DEVICE_ID_TI_WL1271 0x4076
drivers/mmc/core/quirks.c static void add_quirk_for_sdio_devices(struct mmc_card *card, int data) · line 67
Quoted 10 lines
drivers/mmc/core/quirks.c +4
-0+#endif +static const struct mmc_fixup mmc_fixup_methods[] = { { SDIO_ANY_ID, SDIO_ANY_ID, add_quirk_for_sdio_devices, MMC_QUIRK_BROKEN_CLK_GATING } + {SDIO_VENDOR_ID_TI ,SDIO_DEVICE_ID_TI_WL1271 , + remove_quirk_hook,MMC_QUIRK_BROKEN_CLK_GATING }, {0 } };
This doesn't compile, missing a comma. Was it tested?
drivers/mmc/core/quirks.c:63:2: error: expected ‘}’ before ‘{’ token
(Otherwise, looks good.)
Signature · 2 lines
Chris Ball <[email protected]> <http://printf.net/> One Laptop Per Child
Re: [PATCH 3/3] mmc: remove anti clock gating quirk for wl1271
Quoted 4 lines
This adds a defined-but-not-used warning. The MMC tree doesn't currently generate any warnings, so it would be good to avoid adding new ones, though I understand why it might make sense in this case.
Yes, this is supposed to be used by future user patches.
Do you suggest to add __attribute__ ((unused)), or we just completly remove those and will add them whenever we have user?
Quoted 10 lines
Quoted 8 lines
static const struct
mmc_fixupmmc_fixup_methods[] = {
{SDIO_ANY_ID,SDIO_ANY_ID,add_quirk_for_sdio_devices,MMC_QUIRK_BROKEN_CLK_GATING}
+ {SDIO_VENDOR_ID_TI,SDIO_DEVICE_ID_TI_WL1271,
+remove_quirk_hook,MMC_QUIRK_BROKEN_CLK_GATING}, { 0 }
};This doesn't compile, missing a comma. Was it tested?
Hum, I must have screwed up with patch version.
I'm using this every day,,,
Regards,
Pierre
Re: [PATCH 3/3] mmc: remove anti clock gating quirk for wl1271
Quoted 7 lines · Pierre Tardy
Quoted 3 lines
currently generate any warnings, so it would be good to avoid adding new ones, though I understand why it might make sense in this case.
Yes, this is supposed to be used by future user patches.
Do you suggest to add
__attribute__((unused)), or we just completly remove those and will add them whenever we have user?
Yes, let's go with __attribute__ ((unused)) for now. Thanks!
Signature · 2 lines
Chris Ball <[email protected]> <http://printf.net/> One Laptop Per Child
2011/1/9 Pierre Tardy <[email protected]>:
Quoted 3 lines
this is a followup patchset of discussion that happen before on linux-mmc mailing list We add a
mmc/core/quirk.cfile holding list of quirks per devices or more generic quirks e.g. all sdio devices have this quirk.
I like this approach.
Yours,
Linus Walleij