| Message ID | m31v3lofm1.fsf@pullcord.laptop.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Sun, Feb 06 2011, Chris Ball wrote: > Part of a quirk cleanup run. This quirk was only used by sdhci-esdhc. > This patch is untested. > > Signed-off-by: Chris Ball <cjb@laptop.org> > Cc: Anton Vorontsov <cbouatmailru@gmail.com> > Cc: Wolfram Sang <w.sang@pengutronix.de> I've queued this patch now, since we're out of quirk bits and didn't come up with any alternative patches. (Feel free to improve on it later.) Thanks, - Chris.
On Sat, May 28, 2011 at 10:27:41PM -0400, Chris Ball wrote: > Hi, > > On Sun, Feb 06 2011, Chris Ball wrote: > > Part of a quirk cleanup run. This quirk was only used by sdhci-esdhc. > > This patch is untested. > > > > Signed-off-by: Chris Ball <cjb@laptop.org> > > Cc: Anton Vorontsov <cbouatmailru@gmail.com> > > Cc: Wolfram Sang <w.sang@pengutronix.de> > > I've queued this patch now, since we're out of quirk bits and didn't come > up with any alternative patches. (Feel free to improve on it later.) :( I still like the io-accessor-method a lot better. I would have created a patch if I had got some feedback on my suggestion[1] (and will still do). Regards, Wolfram [1] http://thread.gmane.org/gmane.linux.kernel.mmc/5742/focus=5768
Hi Wolfram, Nico, On Wed, Jun 01 2011, Wolfram Sang wrote: > :( I still like the io-accessor-method a lot better. That's okay -- nothing's final yet, I just wanted to get things moving again since we're out of quirk space now. I'm still happy to take a patch from you instead if we decide it's the better way to go. > I would have created a patch if I had got some feedback on my > suggestion[1] (and will still do). > > [1] http://thread.gmane.org/gmane.linux.kernel.mmc/5742/focus=5768 Sorry for not replying. I'm still mildly in favor of having it handled as a platform check in the obvious place in sdhci.c, because I can recall when I didn't know what IO-accessors were, and if I were trying to track down a bug with with max_blk_size I would have found my way to sdhci.c but not thought to look at a different part of the host driver for code that had overloaded sdhci_readl() to mean something non-obvious. In short, I want the code to be maintainable even by hackers who don't yet understand how MMC chooses to organize itself, and I feel that your proposal eventually results in code that seems "magic" and can't easily be followed by a kernel hacker who happens to be unfamiliar with which functions we tend to make override-able; someone doing a board bringup, say. I'm willing to change my mind, though. You can counter-argue that my approach eventually pollutes sdhci.c with a bunch of potential calls into platform code, such that people have to spend time working out whether their platform is or isn't overwriting any particular function. That's certainly true. Perhaps it comes down to preferred coding style -- maybe we can get someone else to act as a tie-breaker? :) CC'ing Nico in case he's interested. Thanks! - Chris.
On Wed, 1 Jun 2011, Chris Ball wrote: > Hi Wolfram, Nico, > > On Wed, Jun 01 2011, Wolfram Sang wrote: > > :( I still like the io-accessor-method a lot better. > > That's okay -- nothing's final yet, I just wanted to get things moving > again since we're out of quirk space now. I'm still happy to take a > patch from you instead if we decide it's the better way to go. What about letting the platform specific code override the caps bits instead? That would work for many other things as well, like the various DMA quirks, etc. And that requires only one callback instead of one per parameter you might want to override. This should be made explicit with a dedicated callback of course, not via some magic in sdhci_readl() please. Nicolas -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Nicolas, > > > :( I still like the io-accessor-method a lot better. > > > > That's okay -- nothing's final yet, I just wanted to get things moving > > again since we're out of quirk space now. I'm still happy to take a > > patch from you instead if we decide it's the better way to go. > > What about letting the platform specific code override the caps bits > instead? That would work for many other things as well, like the > various DMA quirks, etc. And that requires only one callback instead of > one per parameter you might want to override. > > This should be made explicit with a dedicated callback of course, not > via some magic in sdhci_readl() please. I proposed a kind-of fixup() function once, but someone (Olof?) didn't like it because it broke abstraction. However, after Shawn's patch series turning sdhci into a library, things might have become easier for this approach. Hmmm, I'd like to try it, just have to see when. Regards, Wolfram
diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c index 9b82910..6249b75 100644 --- a/drivers/mmc/host/sdhci-esdhc-imx.c +++ b/drivers/mmc/host/sdhci-esdhc-imx.c @@ -100,6 +100,13 @@ static unsigned int esdhc_pltfm_get_min_clock(struct sdhci_host *host) return clk_get_rate(pltfm_host->clk) / 256 / 16; } +static unsigned int esdhc_pltfm_get_max_blk_size(struct sdhci_host *host) +{ + /* Force 2048 bytes, which is the maximum supported size in SDHCI. */ + + return 2; +} + static int esdhc_pltfm_init(struct sdhci_host *host, struct sdhci_pltfm_data *pdata) { struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); @@ -138,6 +145,7 @@ static struct sdhci_ops sdhci_esdhc_ops = { .set_clock = esdhc_set_clock, .get_max_clock = esdhc_pltfm_get_max_clock, .get_min_clock = esdhc_pltfm_get_min_clock, + .get_max_blk_size = esdhc_pltfm_get_max_blk_size, }; struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = { diff --git a/drivers/mmc/host/sdhci-esdhc.h b/drivers/mmc/host/sdhci-esdhc.h index afaf1bc..b18bb5f 100644 --- a/drivers/mmc/host/sdhci-esdhc.h +++ b/drivers/mmc/host/sdhci-esdhc.h @@ -18,8 +18,7 @@ * Ops and quirks for the Freescale eSDHC controller. */ -#define ESDHC_DEFAULT_QUIRKS (SDHCI_QUIRK_FORCE_BLK_SZ_2048 | \ - SDHCI_QUIRK_BROKEN_CARD_DETECTION | \ +#define ESDHC_DEFAULT_QUIRKS (SDHCI_QUIRK_BROKEN_CARD_DETECTION | \ SDHCI_QUIRK_NO_BUSY_IRQ | \ SDHCI_QUIRK_NONSTANDARD_CLOCK | \ SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK | \ diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c index fcd0e1f..43e5f56 100644 --- a/drivers/mmc/host/sdhci-of-esdhc.c +++ b/drivers/mmc/host/sdhci-of-esdhc.c @@ -72,6 +72,13 @@ static unsigned int esdhc_of_get_min_clock(struct sdhci_host *host) return of_host->clock / 256 / 16; } +static unsigned int esdhc_of_get_max_blk_size(struct sdhci_host *host) +{ + /* Force 2048 bytes, which is the maximum supported size in SDHCI. */ + + return 2; +} + struct sdhci_of_data sdhci_esdhc = { .quirks = ESDHC_DEFAULT_QUIRKS, .ops = { @@ -85,5 +92,6 @@ struct sdhci_of_data sdhci_esdhc = { .enable_dma = esdhc_of_enable_dma, .get_max_clock = esdhc_of_get_max_clock, .get_min_clock = esdhc_of_get_min_clock, + .get_max_blk_size = esdhc_of_get_max_blk_size, }, }; diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index 9e15f41..fcd6188 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -1962,8 +1962,8 @@ int sdhci_add_host(struct sdhci_host *host) * Maximum block size. This varies from controller to controller and * is specified in the capabilities register. */ - if (host->quirks & SDHCI_QUIRK_FORCE_BLK_SZ_2048) { - mmc->max_blk_size = 2; + if (host->ops->get_max_blk_size) { + mmc->max_blk_size = host->ops->get_max_blk_size(host); } else { mmc->max_blk_size = (caps & SDHCI_MAX_BLOCK_MASK) >> SDHCI_MAX_BLOCK_SHIFT; diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h index 6e0969e..08c1071 100644 --- a/drivers/mmc/host/sdhci.h +++ b/drivers/mmc/host/sdhci.h @@ -217,6 +217,7 @@ struct sdhci_ops { unsigned int (*get_max_clock)(struct sdhci_host *host); unsigned int (*get_min_clock)(struct sdhci_host *host); unsigned int (*get_timeout_clock)(struct sdhci_host *host); + unsigned int (*get_max_blk_size)(struct sdhci_host *host); int (*platform_8bit_width)(struct sdhci_host *host, int width); void (*platform_send_init_74_clocks)(struct sdhci_host *host, diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h index 83bd9f7..2fde25c 100644 --- a/include/linux/mmc/sdhci.h +++ b/include/linux/mmc/sdhci.h @@ -63,8 +63,8 @@ struct sdhci_host { #define SDHCI_QUIRK_PIO_NEEDS_DELAY (1<<18) /* Controller losing signal/interrupt enable states after reset */ #define SDHCI_QUIRK_RESTORE_IRQS_AFTER_RESET (1<<19) -/* Controller has to be forced to use block size of 2048 bytes */ -#define SDHCI_QUIRK_FORCE_BLK_SZ_2048 (1<<20) +/* Reclaimed */ +#define SDHCI_QUIRK_UNUSED_20 (1<<20) /* Controller cannot do multi-block transfers */ #define SDHCI_QUIRK_NO_MULTIBLOCK (1<<21) /* Controller can only handle 1-bit data transfers */