[PATCH 1/2] mmc: sdhci: Replace SDHCI_QUIRK_FORCE_BLK_SZ_2048 with a pklatform hook.

[PATCH 1/2] mmc: sdhci: Replace SDHCI_QUIRK_FORCE_BLK_SZ_2048 with a pklatform hook.

[Chris Ball]

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>
---
 drivers/mmc/host/sdhci-esdhc-imx.c |    8 ++++++++
 drivers/mmc/host/sdhci-esdhc.h     |    3 +--
 drivers/mmc/host/sdhci-of-esdhc.c  |    8 ++++++++
 drivers/mmc/host/sdhci.c           |    4 ++--
 drivers/mmc/host/sdhci.h           |    1 +
 include/linux/mmc/sdhci.h          |    4 ++--
 6 files changed, 22 insertions(+), 6 deletions(-)

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 */
-- 
Chris Ball   <cjb@laptop.org>
One Laptop Per Child

Re: [PATCH 1/2] mmc: sdhci: Replace SDHCI_QUIRK_FORCE_BLK_SZ_2048 with a pklatform hook.

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1125 bytes --]

On Sun, Feb 06, 2011 at 01:13:10AM -0500, 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>
> 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;

I tend to think this could be fixed using io-accessors when reading the
caps register?

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 1/2] mmc: sdhci: Replace SDHCI_QUIRK_FORCE_BLK_SZ_2048 with a pklatform hook.

[Chris Ball]

Hi Wolfram, thanks for the review,

On Mon, Feb 07, 2011 at 06:40:39PM +0100, Wolfram Sang wrote:
> On Sun, Feb 06, 2011 at 01:13:10AM -0500, 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>
> > 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;
> 
> I tend to think this could be fixed using io-accessors when reading the
> caps register?

Hm, I agree that it would work, but I'm not sure it's going to be
cleaner/more readable that way.  I like that here we have one place
for setting the max_blk_size, and it's obvious from the code exactly
where you need to look to see if it's been overloaded by the driver.

Thanks,

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

Re: [PATCH 1/2] mmc: sdhci: Replace SDHCI_QUIRK_FORCE_BLK_SZ_2048 with a pklatform hook.

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2368 bytes --]

On Mon, Feb 07, 2011 at 05:54:24PM +0000, Chris Ball wrote:
> Hi Wolfram, thanks for the review,
> 
> On Mon, Feb 07, 2011 at 06:40:39PM +0100, Wolfram Sang wrote:
> > On Sun, Feb 06, 2011 at 01:13:10AM -0500, 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>
> > > 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;
> > 
> > I tend to think this could be fixed using io-accessors when reading the
> > caps register?
> 
> Hm, I agree that it would work, but I'm not sure it's going to be
> cleaner/more readable that way.  I like that here we have one place
> for setting the max_blk_size, and it's obvious from the code exactly
> where you need to look to see if it's been overloaded by the driver.

Without the quirk and its handling, we lose the if-else-block which will
lead to:

	mmc->max_blk_size = (caps & SDHCI_MAX_BLOCK_MASK) >>
			SDHCI_MAX_BLOCK_SHIFT;
	if (mmc->max_blk_size >= 3) {
		printk(KERN_WARNING "%s: Invalid maximum block size, "
			"assuming 512 bytes\n", mmc_hostname(mmc));
		mmc->max_blk_size = 0;
	}

This is what I would prefer. Straight code, handling an sdhci-controller
adhering to the standard. A controller failing to do that, will have to take
care of that in its specific source file. For the sdhci-core, it would then
simply look like a "good" controller. We probably can't achieve this for all
quirks, but still...

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 1/2] mmc: sdhci: Replace SDHCI_QUIRK_FORCE_BLK_SZ_2048 with a pklatform hook.

[Chris Ball]

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.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 1/2] mmc: sdhci: Replace SDHCI_QUIRK_FORCE_BLK_SZ_2048 with a pklatform hook.

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 931 bytes --]

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

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 1/2] mmc: sdhci: Replace SDHCI_QUIRK_FORCE_BLK_SZ_2048 with a platform hook.

[Chris Ball]

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.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 1/2] mmc: sdhci: Replace SDHCI_QUIRK_FORCE_BLK_SZ_2048 with a platform hook.

[Nicolas Pitre]

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

Re: [PATCH 1/2] mmc: sdhci: Replace SDHCI_QUIRK_FORCE_BLK_SZ_2048 with a platform hook.

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1159 bytes --]

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

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]