MMC block quirk support + Toshiba performance quirk

MMC block quirk support + Toshiba performance quirk

[Andrei Warkentin]

This patch set is meant for as a review for comments, and is the
result of the discussion on MMC quirks. Sorry ahead of time if I didn't
format this correctly. It's my first tiem with git mail-send :-).

For 1/3: Arnd suggested setting the default write alignment to
adjust on to be 32K. Since I don't have conclusive data and I don't
have a large set of flash memory to test on, the default behavior is
"do nothing", i.e. only quirk 3/3 sets the parameter to 8K as per 
manufacturer suggestion.

Table of Contents:
[RFC 1/3] MMC: Adjust unaligned write accesses.
[RFC 2/3] MMC: Add block quirks support.
[RFC 3/3] MMC: Toshiba eMMC - Split 8K-unaligned accesses.

Here is an example of the effects of 3/3 on the Toshiba MMC32G card.

Before -

/data/flashbench -A -b 1024 /dev/block/platform/sdhci-tegra.3/by-name/cache 
write align 8388608	pre 3.84ms	on 7.45ms	post 4.32ms	diff 3.37ms
write align 4194304	pre 3.53ms	on 6.43ms	post 3.28ms	diff 3.02ms
write align 2097152	pre 4.1ms	on 7.19ms	post 3.99ms	diff 3.14ms
write align 1048576	pre 4.2ms	on 7.54ms	post 4.38ms	diff 3.26ms
write align 524288	pre 4.19ms	on 7.37ms	post 4.37ms	diff 3.1ms
write align 262144	pre 4.2ms	on 7.52ms	post 4.38ms	diff 3.23ms
write align 131072	pre 4.19ms	on 7.48ms	post 4.38ms	diff 3.2ms
write align 65536	pre 4.19ms	on 7.38ms	post 4.38ms	diff 3.1ms
write align 32768	pre 4.17ms	on 7.44ms	post 4.37ms	diff 3.17ms
write align 16384	pre 3.67ms	on 146ms	post 3.55ms	diff 143ms
write align 8192	pre 3.63ms	on 137ms	post 3.54ms	diff 134ms
write align 4096	pre 4.35ms	on 4.42ms	post 4.24ms	diff 121µs
write align 2048	pre 4.4ms	on 4.18ms	post 4.43ms	diff -235734

8KB write alignment -

/data/flashbench -A -b 1024 /dev/block/platform/sdhci-tegra.3/by-name/cache
write align 8388608	pre 3.22ms	on 6.48ms	post 3.24ms	diff 3.25ms
write align 4194304	pre 3.43ms	on 6.67ms	post 3.4ms	diff 3.25ms
write align 2097152	pre 3.54ms	on 6.81ms	post 3.35ms	diff 3.37ms
write align 1048576	pre 3.68ms	on 6.77ms	post 3.57ms	diff 3.14ms
write align 524288	pre 3.64ms	on 6.9ms	post 3.66ms	diff 3.25ms
write align 262144	pre 3.65ms	on 6.9ms	post 3.33ms	diff 3.41ms
write align 131072	pre 3.64ms	on 6.9ms	post 3.68ms	diff 3.24ms
write align 65536	pre 3.65ms	on 6.89ms	post 3.4ms	diff 3.37ms
write align 32768	pre 3.39ms	on 6.78ms	post 3.67ms	diff 3.25ms
write align 16384	pre 3.67ms	on 6.89ms	post 3.67ms	diff 3.22ms
write align 8192	pre 3.68ms	on 6.89ms	post 3.66ms	diff 3.22ms
write align 4096	pre 3.74ms	on 3.73ms	post 3.73ms	diff -4281ns
write align 2048	pre 3.73ms	on 3.73ms	post 3.73ms	diff 2.59µs

[RFC 1/3] MMC: Adjust unaligned write accesses.

[Andrei Warkentin]

Adjust unaligned write accesses spanning preferred align
size into two accesses - an unaligned and an aligned access.
This is meant to be used for card quirks, and is off
by default.

Change-Id: Ic4ce561a4a72a13f34d1a88ad669f98e3c7b9d90
Signed-off-by: Andrei Warkentin <andreiw@motorola.com>
---
 drivers/mmc/card/block.c |   38 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 7054fd5..498c439 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -63,6 +63,7 @@ struct mmc_blk_data {
 
 	unsigned int	usage;
 	unsigned int	read_only;
+	unsigned int	write_align_size;
 };
 
 static DEFINE_MUTEX(open_lock);
@@ -312,6 +313,39 @@ out:
 	return err ? 0 : 1;
 }
 
+/*
+ * If the request is not aligned, split it into an unaligned
+ * and an aligned portion. Here we can adjust
+ * the size of the MMC request and let the block layer request handle
+ * deal with generating another MMC request.
+ */
+
+static bool mmc_adjust_write(struct mmc_card *card,
+			     struct mmc_request *mrq)
+{
+	unsigned int left_in_page;
+	unsigned int wa_size_blocks;
+	struct mmc_blk_data *md = mmc_get_drvdata(card);
+
+	if (!md->write_align_size)
+		return false;
+
+	wa_size_blocks = md->write_align_size / mrq->data->blksz;
+	left_in_page = wa_size_blocks -
+		(mrq->cmd->arg % wa_size_blocks);
+
+	/* Aligned access. */
+	if (left_in_page == wa_size_blocks)
+		return false;
+
+	/* Not straddling page boundary. */
+	if (mrq->data->blocks <= left_in_page)
+		return false;
+
+	mrq->data->blocks = left_in_page;
+	return true;
+}
+
 static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *req)
 {
 	struct mmc_blk_data *md = mq->data;
@@ -339,6 +373,10 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *req)
 		brq.stop.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
 		brq.data.blocks = blk_rq_sectors(req);
 
+		/* Check for unaligned accesses straddling pages. */
+		if (rq_data_dir(req) == WRITE)
+			mmc_adjust_write(card, &brq.mrq);
+
 		/*
 		 * The block layer doesn't support all sector count
 		 * restrictions, so we need to be prepared for too big
-- 
1.7.0.4

[RFC 2/3] MMC: Add block quirks support.

[Andrei Warkentin]

Quirks are card-specific workarounds. Usually they involve
tuning mmcblk parameters at mmc_blk_probe time, but can
involve affecting the way mmcblk I/O requests are handled.
The later is necessary to handle Sandisk out-of-spec discard support,
and the small-writes reliability workaround for Toshiba.

Change-Id: Ia5e56d7a2803daba4b9085f2ca12afeadc7d9095
Signed-off-by: Andrei Warkentin <andreiw@motorola.com>
---
 drivers/mmc/card/Kconfig        |    7 +++
 drivers/mmc/card/Makefile       |    1 +
 drivers/mmc/card/blk.h          |   58 ++++++++++++++++++++
 drivers/mmc/card/block-quirks.c |  111 +++++++++++++++++++++++++++++++++++++++
 drivers/mmc/card/block.c        |   30 ++++++-----
 5 files changed, 194 insertions(+), 13 deletions(-)
 create mode 100644 drivers/mmc/card/blk.h
 create mode 100644 drivers/mmc/card/block-quirks.c

diff --git a/drivers/mmc/card/Kconfig b/drivers/mmc/card/Kconfig
index 86948f9..063fa16 100644
--- a/drivers/mmc/card/Kconfig
+++ b/drivers/mmc/card/Kconfig
@@ -14,6 +14,13 @@ config MMC_BLOCK
 	  mount the filesystem. Almost everyone wishing MMC support
 	  should say Y or M here.
 
+config MMC_BLOCK_QUIRKS
+       tristate "MMC block device quirks"
+       depends on MMC_BLOCK
+       default y
+       help
+         Say Y here to enable various workarounds for known cards.
+
 config MMC_BLOCK_BOUNCE
 	bool "Use bounce buffer for simple hosts"
 	depends on MMC_BLOCK
diff --git a/drivers/mmc/card/Makefile b/drivers/mmc/card/Makefile
index 0d40751..fcd3f45 100644
--- a/drivers/mmc/card/Makefile
+++ b/drivers/mmc/card/Makefile
@@ -7,6 +7,7 @@ ifeq ($(CONFIG_MMC_DEBUG),y)
 endif
 
 obj-$(CONFIG_MMC_BLOCK)		+= mmc_block.o
+obj-$(CONFIG_MMC_BLOCK_QUIRKS)  += block-quirks.o
 mmc_block-objs			:= block.o queue.o
 obj-$(CONFIG_MMC_TEST)		+= mmc_test.o
 
diff --git a/drivers/mmc/card/blk.h b/drivers/mmc/card/blk.h
new file mode 100644
index 0000000..a45a37f
--- /dev/null
+++ b/drivers/mmc/card/blk.h
@@ -0,0 +1,58 @@
+#ifndef MMC_BLK_H
+#define MMC_BLK_H
+
+struct gendisk;
+
+#ifdef CONFIG_MMC_BLOCK_QUIRKS
+struct mmc_blk_data;
+
+#define mmc_blk_qrev(hwrev, fwrev, year, month) \
+	(((u64) hwrev) << 40 |                  \
+	 ((u64) fwrev) << 32 |                  \
+	 ((u64) year) << 16 |                   \
+	 ((u64) month))
+
+#define mmc_blk_qrev_card(card)       \
+	mmc_blk_qrev(card->cid.hwrev, \
+		     card->cid.fwrev, \
+		     card->cid.year,  \
+		     card->cid.month)
+
+struct mmc_blk_quirk {
+	struct rb_node rb_node;
+	const char *name;
+
+	/* First valid revision */
+	u64 rev_start;
+
+	/* Last valid revision */
+	u64 rev_end;
+
+	unsigned int manfid;
+	unsigned short oemid;
+	int (*probe)(struct mmc_blk_data *, struct mmc_card *);
+	int (*adjust)(struct mmc_queue *, struct request *, struct mmc_request *);
+};
+
+
+struct mmc_blk_quirk *mmc_blk_quirk_find(struct mmc_card *card);
+int mmc_blk_quirk_register(struct mmc_blk_quirk *quirk, bool is_mmc);
+#endif /* CONFIG_MMC_BLOCK_QUIRKS */
+
+/*
+ * There is one mmc_blk_data per slot.
+ */
+struct mmc_blk_data {
+	spinlock_t	lock;
+	struct gendisk	*disk;
+	struct mmc_queue queue;
+
+	unsigned int	usage;
+	unsigned int	read_only;
+	unsigned int	write_align_size;
+#ifdef CONFIG_MMC_BLOCK_QUIRKS
+	struct mmc_blk_quirk *quirk;
+#endif /* CONFIG_MMC_BLOCK_QUIRKS */
+};
+
+#endif
diff --git a/drivers/mmc/card/block-quirks.c b/drivers/mmc/card/block-quirks.c
new file mode 100644
index 0000000..ceae70c
--- /dev/null
+++ b/drivers/mmc/card/block-quirks.c
@@ -0,0 +1,111 @@
+/*
+ * linux/drivers/mmc/card/block-quirks.c
+ *
+ * Copyright (C) 2010 Andrei Warkentin <andreiw@motorola.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+#include <linux/kernel.h>
+#include <linux/semaphore.h>
+#include <linux/mmc/card.h>
+
+#include "queue.h"
+#include "blk.h"
+
+/*
+   Since the goal is to support quirks in removable media, ideally
+   such quirks would be always built into a kernel, hence we need
+   a smarter way to search.
+*/
+static struct rb_root quirk_tree[2] = { RB_ROOT, RB_ROOT };
+
+static int mmc_blk_quirk_cmp(const struct mmc_blk_quirk *quirk,
+			     const struct mmc_blk_quirk *cquirk)
+{
+	int ret;
+
+	if (quirk->manfid > cquirk->manfid)
+		return 1;
+	else if (quirk->manfid < cquirk->manfid)
+		return -1;
+
+	if (quirk->oemid > cquirk->oemid)
+		return 1;
+	else if (quirk->oemid < cquirk->oemid)
+		return -1;
+
+	ret = strcmp(quirk->name, cquirk->name);
+	if (!ret)
+		return ret;
+
+	if (quirk->rev_start > cquirk->rev_end)
+		return 1;
+	else if (quirk->rev_end < cquirk->rev_start)
+		return -1;
+
+	/* Overlap in revs or equal. */
+	return 0;
+}
+
+static int mmc_blk_quirk_cmpc(const struct mmc_blk_quirk *quirk,
+			      const struct mmc_card *card)
+{
+	struct mmc_blk_quirk cquirk;
+	cquirk.name = card->cid.prod_name;
+	cquirk.rev_start = mmc_blk_qrev_card(card);
+	cquirk.rev_end = cquirk.rev_start;
+	cquirk.manfid = card->cid.manfid;
+	cquirk.oemid = card->cid.oemid;
+	return mmc_blk_quirk_cmp(quirk, &cquirk);
+}
+
+struct mmc_blk_quirk *mmc_blk_quirk_find(struct mmc_card *card)
+{
+	int ret;
+	struct mmc_blk_quirk *quirk;
+	struct rb_node *node = quirk_tree[!mmc_card_mmc(card)].rb_node;
+
+	while (node) {
+		quirk = container_of(node, struct mmc_blk_quirk, rb_node);
+		ret = mmc_blk_quirk_cmpc(quirk,	card);
+
+		if (ret < 0)
+			node = node->rb_left;
+		else if (ret > 0)
+			node = node->rb_right;
+		else
+			return quirk;
+	}
+
+	return NULL;
+}
+
+int mmc_blk_quirk_register(struct mmc_blk_quirk *quirk, bool is_mmc)
+{
+	int ret;
+	struct mmc_blk_quirk *cq;
+	struct rb_node **new = &(quirk_tree[!is_mmc].rb_node), *parent = NULL;
+
+	while (*new) {
+		cq = container_of(*new, struct mmc_blk_quirk, rb_node);
+
+		ret = mmc_blk_quirk_cmp(quirk, cq);
+
+		parent = *new;
+		if (ret < 0)
+			new = &(parent->rb_left);
+		else if(ret > 0)
+			new = &(parent->rb_right);
+		else
+			/* Overlap */
+			return -EINVAL;
+
+	}
+
+	rb_link_node(&quirk->rb_node, parent, new);
+	rb_insert_color(&quirk->rb_node, &quirk_tree[!is_mmc]);
+	return 0;
+}
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 498c439..8b72407 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -42,6 +42,7 @@
 #include <asm/uaccess.h>
 
 #include "queue.h"
+#include "blk.h"
 
 MODULE_ALIAS("mmc:block");
 
@@ -53,19 +54,6 @@ MODULE_ALIAS("mmc:block");
 
 static DECLARE_BITMAP(dev_use, MMC_NUM_MINORS);
 
-/*
- * There is one mmc_blk_data per slot.
- */
-struct mmc_blk_data {
-	spinlock_t	lock;
-	struct gendisk	*disk;
-	struct mmc_queue queue;
-
-	unsigned int	usage;
-	unsigned int	read_only;
-	unsigned int	write_align_size;
-};
-
 static DEFINE_MUTEX(open_lock);
 
 static struct mmc_blk_data *mmc_blk_get(struct gendisk *disk)
@@ -421,6 +409,11 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *req)
 		brq.data.sg = mq->sg;
 		brq.data.sg_len = mmc_queue_map_sg(mq);
 
+#ifdef CONFIG_MMC_BLOCK_QUIRKS
+		if (md->quirk && md->quirk->adjust)
+			md->quirk->adjust(mq, req, &brq.mrq);
+#endif /* CONFIG_MMC_BLOCK_QUIRKS */
+
 		/*
 		 * Adjust the sg list so it is the same size as the
 		 * request.
@@ -727,6 +720,9 @@ mmc_blk_set_blksize(struct mmc_blk_data *md, struct mmc_card *card)
 static int mmc_blk_probe(struct mmc_card *card)
 {
 	struct mmc_blk_data *md;
+#ifdef CONFIG_MMC_BLOCK_QUIRKS
+	struct mmc_blk_quirk *quirk;
+#endif /* CONFIG_MMC_BLOCK_QUIRKS */
 	int err;
 
 	char cap_str[10];
@@ -745,6 +741,14 @@ static int mmc_blk_probe(struct mmc_card *card)
 	if (err)
 		goto out;
 
+#ifdef CONFIG_MMC_BLOCK_QUIRKS
+	md->quirk = mmc_blk_quirk_find(card);
+	if (md->quirk && md->quirk->probe)
+		err = quirk->probe(md, card);
+	if (err)
+		goto out;
+#endif /* CONFIG_MMC_BLOCK_QUIRKS */
+
 	string_get_size((u64)get_capacity(md->disk) << 9, STRING_UNITS_2,
 			cap_str, sizeof(cap_str));
 	printk(KERN_INFO "%s: %s %s %s %s\n",
-- 
1.7.0.4

Re: [RFC 2/3] MMC: Add block quirks support.

[Arnd Bergmann]

On Tuesday 01 March 2011, Andrei Warkentin wrote:
> Quirks are card-specific workarounds. Usually they involve
> tuning mmcblk parameters at mmc_blk_probe time, but can
> involve affecting the way mmcblk I/O requests are handled.
> The later is necessary to handle Sandisk out-of-spec discard support,
> and the small-writes reliability workaround for Toshiba.

I was hoping for a simpler method, let me elaborate below
 
> Change-Id: Ia5e56d7a2803daba4b9085f2ca12afeadc7d9095

We don't put a Change-Id into kernel patches, they are meaningless.

> +struct gendisk;
> +
> +#ifdef CONFIG_MMC_BLOCK_QUIRKS
> +struct mmc_blk_data;
> +
> +#define mmc_blk_qrev(hwrev, fwrev, year, month) \
> +	(((u64) hwrev) << 40 |                  \
> +	 ((u64) fwrev) << 32 |                  \
> +	 ((u64) year) << 16 |                   \
> +	 ((u64) month))
> +
> +#define mmc_blk_qrev_card(card)       \
> +	mmc_blk_qrev(card->cid.hwrev, \
> +		     card->cid.fwrev, \
> +		     card->cid.year,  \
> +		     card->cid.month)
> +
> +struct mmc_blk_quirk {
> +	struct rb_node rb_node;
> +	const char *name;
> +
> +	/* First valid revision */
> +	u64 rev_start;
> +
> +	/* Last valid revision */
> +	u64 rev_end;
> +
> +	unsigned int manfid;
> +	unsigned short oemid;
> +	int (*probe)(struct mmc_blk_data *, struct mmc_card *);

So far so good.

> +	int (*adjust)(struct mmc_queue *, struct request *, struct mmc_request *);
> +};

The adjust function looks out of place here, I would leave it out until it
really is used.

> +/*
> + * There is one mmc_blk_data per slot.
> + */
> +struct mmc_blk_data {
> +	spinlock_t	lock;
> +	struct gendisk	*disk;
> +	struct mmc_queue queue;
> +
> +	unsigned int	usage;
> +	unsigned int	read_only;
> +	unsigned int	write_align_size;
> +#ifdef CONFIG_MMC_BLOCK_QUIRKS
> +	struct mmc_blk_quirk *quirk;
> +#endif /* CONFIG_MMC_BLOCK_QUIRKS */
> +};

I don't think a pointer to the quirk is needed here, you can probably
leave the mmc_blk_data alone.


> +/*
> +   Since the goal is to support quirks in removable media, ideally
> +   such quirks would be always built into a kernel, hence we need
> +   a smarter way to search.
> +*/
> +static struct rb_root quirk_tree[2] = { RB_ROOT, RB_ROOT };
> +
> +static int mmc_blk_quirk_cmp(const struct mmc_blk_quirk *quirk,
> +			     const struct mmc_blk_quirk *cquirk)
> +{
> +	int ret;
> +
> +	if (quirk->manfid > cquirk->manfid)
> +		return 1;
> +	else if (quirk->manfid < cquirk->manfid)
> +		return -1;
> +
> +	if (quirk->oemid > cquirk->oemid)
> +		return 1;
> +	else if (quirk->oemid < cquirk->oemid)
> +		return -1;
> +
> +	ret = strcmp(quirk->name, cquirk->name);
> +	if (!ret)
> +		return ret;
> +
> +	if (quirk->rev_start > cquirk->rev_end)
> +		return 1;
> +	else if (quirk->rev_end < cquirk->rev_start)
> +		return -1;
> +
> +	/* Overlap in revs or equal. */
> +	return 0;
> +}
> +
> +static int mmc_blk_quirk_cmpc(const struct mmc_blk_quirk *quirk,
> +			      const struct mmc_card *card)
> +{
> +	struct mmc_blk_quirk cquirk;
> +	cquirk.name = card->cid.prod_name;
> +	cquirk.rev_start = mmc_blk_qrev_card(card);
> +	cquirk.rev_end = cquirk.rev_start;
> +	cquirk.manfid = card->cid.manfid;
> +	cquirk.oemid = card->cid.oemid;
> +	return mmc_blk_quirk_cmp(quirk, &cquirk);
> +}

This can be simplified slightly if you avoid calling mmc_blk_quirk_cmp.

> +struct mmc_blk_quirk *mmc_blk_quirk_find(struct mmc_card *card)
> +{
> +	int ret;
> +	struct mmc_blk_quirk *quirk;
> +	struct rb_node *node = quirk_tree[!mmc_card_mmc(card)].rb_node;
> +
> +	while (node) {
> +		quirk = container_of(node, struct mmc_blk_quirk, rb_node);
> +		ret = mmc_blk_quirk_cmpc(quirk,	card);
> +
> +		if (ret < 0)
> +			node = node->rb_left;
> +		else if (ret > 0)
> +			node = node->rb_right;
> +		else
> +			return quirk;
> +	}
> +
> +	return NULL;
> +}
> +
> +int mmc_blk_quirk_register(struct mmc_blk_quirk *quirk, bool is_mmc)
> +{
> +	int ret;
> +	struct mmc_blk_quirk *cq;
> +	struct rb_node **new = &(quirk_tree[!is_mmc].rb_node), *parent = NULL;
> +
> +	while (*new) {
> +		cq = container_of(*new, struct mmc_blk_quirk, rb_node);
> +
> +		ret = mmc_blk_quirk_cmp(quirk, cq);
> +
> +		parent = *new;
> +		if (ret < 0)
> +			new = &(parent->rb_left);
> +		else if(ret > 0)
> +			new = &(parent->rb_right);
> +		else
> +			/* Overlap */
> +			return -EINVAL;
> +
> +	}
> +
> +	rb_link_node(&quirk->rb_node, parent, new);
> +	rb_insert_color(&quirk->rb_node, &quirk_tree[!is_mmc]);
> +	return 0;
> +}

Instead of the dynamic registration, I'd just put all quirks into
one file and have an array of them:

#define MMC_BLK_QUIRK_VERSION(_name, _manfid, _oemid, _rev_start, _rev_end, _probe) \
{				\
       .name = (_name),		\
       .manfid = (_manfid),	\
       .oemid = (_oemid),	\
       .rev_start = (_rev_start),\
       .rev_end = (_rev_end),	\
       .probe = (_probe),	\
}

#define MMC_BLK_QUIRK(_name, _manfid, _oemid, _probe) \
 MMC_BLK_QUIRK_VERSION(_name, _manfid, _oemid, 0, -1ull, _probe)

struct mmc_blk_quirk mmc_blk_quirks[] = {
	MMC_BLK_QUIRK("MMC32G", 0x11, 0x0100, toshiba_mmc32g),
	...
};

> +#ifdef CONFIG_MMC_BLOCK_QUIRKS
> +		if (md->quirk && md->quirk->adjust)
> +			md->quirk->adjust(mq, req, &brq.mrq);
> +#endif /* CONFIG_MMC_BLOCK_QUIRKS */

Instead of having a function pointer here, I'd start out with
an open-coded version of the one function that you know is required,
and make it depend on members of struct mmc_card that can get
set in the .probe function.

> +#ifdef CONFIG_MMC_BLOCK_QUIRKS
> +	md->quirk = mmc_blk_quirk_find(card);
> +	if (md->quirk && md->quirk->probe)
> +		err = quirk->probe(md, card);
> +	if (err)
> +		goto out;
> +#endif /* CONFIG_MMC_BLOCK_QUIRKS */

Just make this an inline function in the header and call it unconditionally:

#ifdef CONFIG_MMC_BLOCK_QUIRKS
extern int mmc_blk_quirk_find(struct mmc_card *card);
#else
static inline int mmc_blk_quirk_find(struct mmc_card *card)
{
	return 0;
}
#endif

	Arnd

Re: [RFC 2/3] MMC: Add block quirks support.

[Andrei Warkentin]

On Wed, Mar 2, 2011 at 11:19 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 01 March 2011, Andrei Warkentin wrote:
>> Quirks are card-specific workarounds. Usually they involve
>> tuning mmcblk parameters at mmc_blk_probe time, but can
>> involve affecting the way mmcblk I/O requests are handled.
>> The later is necessary to handle Sandisk out-of-spec discard support,
>> and the small-writes reliability workaround for Toshiba.
>
> I was hoping for a simpler method, let me elaborate below
>
>> Change-Id: Ia5e56d7a2803daba4b9085f2ca12afeadc7d9095
>
> We don't put a Change-Id into kernel patches, they are meaningless.
>

Oops, sorry about that.

>> +struct gendisk;
>> +
>> +#ifdef CONFIG_MMC_BLOCK_QUIRKS
>> +struct mmc_blk_data;
>> +
>> +#define mmc_blk_qrev(hwrev, fwrev, year, month) \
>> +     (((u64) hwrev) << 40 |                  \
>> +      ((u64) fwrev) << 32 |                  \
>> +      ((u64) year) << 16 |                   \
>> +      ((u64) month))
>> +
>> +#define mmc_blk_qrev_card(card)       \
>> +     mmc_blk_qrev(card->cid.hwrev, \
>> +                  card->cid.fwrev, \
>> +                  card->cid.year,  \
>> +                  card->cid.month)
>> +
>> +struct mmc_blk_quirk {
>> +     struct rb_node rb_node;
>> +     const char *name;
>> +
>> +     /* First valid revision */
>> +     u64 rev_start;
>> +
>> +     /* Last valid revision */
>> +     u64 rev_end;
>> +
>> +     unsigned int manfid;
>> +     unsigned short oemid;
>> +     int (*probe)(struct mmc_blk_data *, struct mmc_card *);
>
> So far so good.
>
>> +     int (*adjust)(struct mmc_queue *, struct request *, struct mmc_request *);
>> +};
>
> The adjust function looks out of place here, I would leave it out until it
> really is used.
>

Yes, out of place at the moment, I'll pull it out into the patch where
it will be used.

>> +/*
>> + * There is one mmc_blk_data per slot.
>> + */
>> +struct mmc_blk_data {
>> +     spinlock_t      lock;
>> +     struct gendisk  *disk;
>> +     struct mmc_queue queue;
>> +
>> +     unsigned int    usage;
>> +     unsigned int    read_only;
>> +     unsigned int    write_align_size;
>> +#ifdef CONFIG_MMC_BLOCK_QUIRKS
>> +     struct mmc_blk_quirk *quirk;
>> +#endif /* CONFIG_MMC_BLOCK_QUIRKS */
>> +};
>
> I don't think a pointer to the quirk is needed here, you can probably
> leave the mmc_blk_data alone.
>
>

I'll pull it out along with the adjust pointer.

>> +/*
>> +   Since the goal is to support quirks in removable media, ideally
>> +   such quirks would be always built into a kernel, hence we need
>> +   a smarter way to search.
>> +*/
>> +static struct rb_root quirk_tree[2] = { RB_ROOT, RB_ROOT };
>> +
>> +static int mmc_blk_quirk_cmp(const struct mmc_blk_quirk *quirk,
>> +                          const struct mmc_blk_quirk *cquirk)
>> +{
>> +     int ret;
>> +
>> +     if (quirk->manfid > cquirk->manfid)
>> +             return 1;
>> +     else if (quirk->manfid < cquirk->manfid)
>> +             return -1;
>> +
>> +     if (quirk->oemid > cquirk->oemid)
>> +             return 1;
>> +     else if (quirk->oemid < cquirk->oemid)
>> +             return -1;
>> +
>> +     ret = strcmp(quirk->name, cquirk->name);
>> +     if (!ret)
>> +             return ret;
>> +
>> +     if (quirk->rev_start > cquirk->rev_end)
>> +             return 1;
>> +     else if (quirk->rev_end < cquirk->rev_start)
>> +             return -1;
>> +
>> +     /* Overlap in revs or equal. */
>> +     return 0;
>> +}
>> +
>> +static int mmc_blk_quirk_cmpc(const struct mmc_blk_quirk *quirk,
>> +                           const struct mmc_card *card)
>> +{
>> +     struct mmc_blk_quirk cquirk;
>> +     cquirk.name = card->cid.prod_name;
>> +     cquirk.rev_start = mmc_blk_qrev_card(card);
>> +     cquirk.rev_end = cquirk.rev_start;
>> +     cquirk.manfid = card->cid.manfid;
>> +     cquirk.oemid = card->cid.oemid;
>> +     return mmc_blk_quirk_cmp(quirk, &cquirk);
>> +}
>
> This can be simplified slightly if you avoid calling mmc_blk_quirk_cmp.
>

If you get rid of the rb-tree, sure, but I need to be able to compare
an mmc_card against the mmc_blk_quirk, and duplicating
mmc_blk_quirk_cmp didn't seem like a good idea :).

>> +struct mmc_blk_quirk *mmc_blk_quirk_find(struct mmc_card *card)
>> +{
>> +     int ret;
>> +     struct mmc_blk_quirk *quirk;
>> +     struct rb_node *node = quirk_tree[!mmc_card_mmc(card)].rb_node;
>> +
>> +     while (node) {
>> +             quirk = container_of(node, struct mmc_blk_quirk, rb_node);
>> +             ret = mmc_blk_quirk_cmpc(quirk, card);
>> +
>> +             if (ret < 0)
>> +                     node = node->rb_left;
>> +             else if (ret > 0)
>> +                     node = node->rb_right;
>> +             else
>> +                     return quirk;
>> +     }
>> +
>> +     return NULL;
>> +}
>> +
>> +int mmc_blk_quirk_register(struct mmc_blk_quirk *quirk, bool is_mmc)
>> +{
>> +     int ret;
>> +     struct mmc_blk_quirk *cq;
>> +     struct rb_node **new = &(quirk_tree[!is_mmc].rb_node), *parent = NULL;
>> +
>> +     while (*new) {
>> +             cq = container_of(*new, struct mmc_blk_quirk, rb_node);
>> +
>> +             ret = mmc_blk_quirk_cmp(quirk, cq);
>> +
>> +             parent = *new;
>> +             if (ret < 0)
>> +                     new = &(parent->rb_left);
>> +             else if(ret > 0)
>> +                     new = &(parent->rb_right);
>> +             else
>> +                     /* Overlap */
>> +                     return -EINVAL;
>> +
>> +     }
>> +
>> +     rb_link_node(&quirk->rb_node, parent, new);
>> +     rb_insert_color(&quirk->rb_node, &quirk_tree[!is_mmc]);
>> +     return 0;
>> +}
>
> Instead of the dynamic registration, I'd just put all quirks into
> one file and have an array of them:

Ideally, you'd have the most important workarounds always built in, to
deal with important problems like out-of-spec devices. The eMMC ones
you would select, but the external device ones would be "by default".
This could potentially explode in the amount of quirks, so maybe
linear search isn't the best?

Or you think that there will be sufficiently small number that doesn't
justify the complexity?

>
> #define MMC_BLK_QUIRK_VERSION(_name, _manfid, _oemid, _rev_start, _rev_end, _probe) \
> {                               \
>       .name = (_name),         \
>       .manfid = (_manfid),     \
>       .oemid = (_oemid),       \
>       .rev_start = (_rev_start),\
>       .rev_end = (_rev_end),   \
>       .probe = (_probe),       \
> }
>
> #define MMC_BLK_QUIRK(_name, _manfid, _oemid, _probe) \
>  MMC_BLK_QUIRK_VERSION(_name, _manfid, _oemid, 0, -1ull, _probe)
>
> struct mmc_blk_quirk mmc_blk_quirks[] = {
>        MMC_BLK_QUIRK("MMC32G", 0x11, 0x0100, toshiba_mmc32g),
>        ...
> };
>
>> +#ifdef CONFIG_MMC_BLOCK_QUIRKS
>> +             if (md->quirk && md->quirk->adjust)
>> +                     md->quirk->adjust(mq, req, &brq.mrq);
>> +#endif /* CONFIG_MMC_BLOCK_QUIRKS */
>
> Instead of having a function pointer here, I'd start out with
> an open-coded version of the one function that you know is required,
> and make it depend on members of struct mmc_card that can get
> set in the .probe function.
>

I'll pull out the adjust member right now. It's not relevant to this patch set.

>> +#ifdef CONFIG_MMC_BLOCK_QUIRKS
>> +     md->quirk = mmc_blk_quirk_find(card);
>> +     if (md->quirk && md->quirk->probe)
>> +             err = quirk->probe(md, card);
>> +     if (err)
>> +             goto out;
>> +#endif /* CONFIG_MMC_BLOCK_QUIRKS */
>
> Just make this an inline function in the header and call it unconditionally:
>
> #ifdef CONFIG_MMC_BLOCK_QUIRKS
> extern int mmc_blk_quirk_find(struct mmc_card *card);
> #else
> static inline int mmc_blk_quirk_find(struct mmc_card *card)
> {
>        return 0;
> }
> #endif

Ok, great, I'll do that.

Thanks for the feedback,
A

Re: [RFC 2/3] MMC: Add block quirks support.

[Arnd Bergmann]

On Wednesday 02 March 2011 21:48:25 Andrei Warkentin wrote:

> >
> > Instead of the dynamic registration, I'd just put all quirks into
> > one file and have an array of them:
> 
> Ideally, you'd have the most important workarounds always built in, to
> deal with important problems like out-of-spec devices. The eMMC ones
> you would select, but the external device ones would be "by default".
> This could potentially explode in the amount of quirks, so maybe
> linear search isn't the best?
> 
> Or you think that there will be sufficiently small number that doesn't
> justify the complexity?

I think we will start with a small number, and a linear search once
at insertion time is good for a significant amount of quirks. We do this
for all PCI and USB devices, see drivers/usb/storage/usual-tables.c.

I would certainly not do anything more complex than this unless we
get into scalability problems with the simple approach.

	Arnd

Re: [RFC 2/3] MMC: Add block quirks support.

[Andrei Warkentin]

On Wed, Mar 2, 2011 at 3:04 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 02 March 2011 21:48:25 Andrei Warkentin wrote:
>
>> >
>> > Instead of the dynamic registration, I'd just put all quirks into
>> > one file and have an array of them:
>>
>> Ideally, you'd have the most important workarounds always built in, to
>> deal with important problems like out-of-spec devices. The eMMC ones
>> you would select, but the external device ones would be "by default".
>> This could potentially explode in the amount of quirks, so maybe
>> linear search isn't the best?
>>
>> Or you think that there will be sufficiently small number that doesn't
>> justify the complexity?
>
> I think we will start with a small number, and a linear search once
> at insertion time is good for a significant amount of quirks. We do this
> for all PCI and USB devices, see drivers/usb/storage/usual-tables.c.
>
> I would certainly not do anything more complex than this unless we
> get into scalability problems with the simple approach.

Alright, good point, no need to over-complicate :)

A

[RFC 3/3] MMC: Toshiba eMMC - Split 8K-unaligned accesses.

[Andrei Warkentin]

These cards show abysmal write performance when an
8K unaligned write crosses an 8K barrier. This is
the manufacturer recommendation.

Change-Id: I527fc44ff968f7ead61328bf655515fdf27b9f48
Signed-off-by: Andrei Warkentin <andreiw@motorola.com>
---
 drivers/mmc/card/Kconfig        |    7 +++++++
 drivers/mmc/card/block-quirks.c |   30 ++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/card/Kconfig b/drivers/mmc/card/Kconfig
index 063fa16..6d9f391 100644
--- a/drivers/mmc/card/Kconfig
+++ b/drivers/mmc/card/Kconfig
@@ -21,6 +21,13 @@ config MMC_BLOCK_QUIRKS
        help
          Say Y here to enable various workarounds for known cards.
 
+config MMC_BLOCK_QUIRK_TOSHIBA_MMC32G
+       tristate "Toshiba MMC32G quirks"
+       depends on MMC_BLOCK_QUIRKS
+       default n
+       help
+         Say Y if you have a Toshiba MMC32G eMMC card.
+
 config MMC_BLOCK_BOUNCE
 	bool "Use bounce buffer for simple hosts"
 	depends on MMC_BLOCK
diff --git a/drivers/mmc/card/block-quirks.c b/drivers/mmc/card/block-quirks.c
index ceae70c..8aea551 100644
--- a/drivers/mmc/card/block-quirks.c
+++ b/drivers/mmc/card/block-quirks.c
@@ -109,3 +109,33 @@ int mmc_blk_quirk_register(struct mmc_blk_quirk *quirk, bool is_mmc)
 	rb_insert_color(&quirk->rb_node, &quirk_tree[!is_mmc]);
 	return 0;
 }
+
+#ifdef CONFIG_MMC_BLOCK_QUIRK_TOSHIBA_MMC32G
+static int toshiba_mmc32g(struct mmc_blk_data *md, struct mmc_card *card)
+{
+	printk(KERN_INFO "Applying Toshiba MMC32G workarounds\n");
+
+	/* Page size 8K, this card doesn't like unaligned writes
+	   across 8K boundary. */
+	md->write_align_size = 8192;
+	return 0;
+}
+
+static struct mmc_blk_quirk toshiba_mmc32g_q = {
+	.name = "MMC32G",
+	.manfid = 0x11,
+	.oemid = 0x0100,
+
+	/* Any date, any revision, */
+	.rev_start = 0,
+	.rev_end = (u64) -1,
+	.probe = toshiba_mmc32g
+};
+
+int __init mmc_toshiba_quirks(void)
+{
+	return mmc_blk_quirk_register(&toshiba_mmc32g_q, true);
+}
+
+device_initcall(mmc_toshiba_quirks);
+#endif /* CONFIG_MMC_BLOCK_QUIRK_TOSHIBA_MMC32G */
-- 
1.7.0.4

MMC block quirk support + Toshiba quirks

[Andrei Warkentin]

This is a second version of the patch set that adds MMC block
quirks as well as two workarounds for Toshiba 32nm part issues.

It incorporates feedback from Arnd.

Table of Contents:
[[RFC] 1/5] MMC: Adjust unaligned write accesses.
[[RFC] 2/5] MMC: Add block quirks support.
[[RFC] 3/5] MMC: Toshiba eMMC - Split 8K-unaligned accesses.
[[RFC] 4/5] MMC: Block quirks request adjust support.
[[RFC] 5/5] MMC: Toshiba eMMC - Part reliability improvement.

Thanks,
A

[[RFC] 1/5] MMC: Adjust unaligned write accesses.

[Andrei Warkentin]

Adjust unaligned write accesses spanning preferred align
size into two accesses - an unaligned and an aligned access.
This is meant to be used for card quirks, and is off
by default. A limiting value in transfer size
for this adjustment is available, as on some cards there is a
perf decrease for larger transfers.

Signed-off-by: Andrei Warkentin <andreiw@motorola.com>
---
 drivers/mmc/card/block.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 7054fd5..9d44480 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -63,6 +63,8 @@ struct mmc_blk_data {
 
 	unsigned int	usage;
 	unsigned int	read_only;
+	unsigned int	write_align_size;
+	unsigned int	write_align_limit;
 };
 
 static DEFINE_MUTEX(open_lock);
@@ -312,6 +314,43 @@ out:
 	return err ? 0 : 1;
 }
 
+/*
+ * If the request is not aligned, split it into an unaligned
+ * and an aligned portion. Here we can adjust
+ * the size of the MMC request and let the block layer request handle
+ * deal with generating another MMC request.
+ */
+
+static void mmc_adjust_write(struct mmc_card *card,
+			     struct mmc_request *mrq)
+{
+	unsigned int left_in_page;
+	unsigned int wa_size_blocks;
+	struct mmc_blk_data *md = mmc_get_drvdata(card);
+
+	if (!md->write_align_size)
+		return;
+
+	if (md->write_align_limit &&
+	    (md->write_align_limit / mrq->data->blksz)
+	    < mrq->data->blocks)
+		return;
+
+	wa_size_blocks = md->write_align_size / mrq->data->blksz;
+	left_in_page = wa_size_blocks -
+		(mrq->cmd->arg % wa_size_blocks);
+
+	/* Aligned access. */
+	if (left_in_page == wa_size_blocks)
+		return;
+
+	/* Not straddling page boundary. */
+	if (mrq->data->blocks <= left_in_page)
+		return;
+
+	mrq->data->blocks = left_in_page;
+}
+
 static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *req)
 {
 	struct mmc_blk_data *md = mq->data;
@@ -339,6 +378,10 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *req)
 		brq.stop.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
 		brq.data.blocks = blk_rq_sectors(req);
 
+		/* Check for unaligned accesses straddling pages. */
+		if (rq_data_dir(req) == WRITE)
+			mmc_adjust_write(card, &brq.mrq);
+
 		/*
 		 * The block layer doesn't support all sector count
 		 * restrictions, so we need to be prepared for too big
-- 
1.7.0.4

[[RFC] 2/5] MMC: Add block quirks support.

[Andrei Warkentin]

Quirks are card-specific workarounds. Usually they involve
tuning mmcblk parameters at mmc_blk_probe time.

Signed-off-by: Andrei Warkentin <andreiw@motorola.com>
---
 drivers/mmc/card/Kconfig        |    7 ++++
 drivers/mmc/card/Makefile       |    1 +
 drivers/mmc/card/blk.h          |   70 +++++++++++++++++++++++++++++++++++++++
 drivers/mmc/card/block-quirks.c |   51 ++++++++++++++++++++++++++++
 drivers/mmc/card/block.c        |   22 ++++--------
 5 files changed, 137 insertions(+), 14 deletions(-)
 create mode 100644 drivers/mmc/card/blk.h
 create mode 100644 drivers/mmc/card/block-quirks.c

diff --git a/drivers/mmc/card/Kconfig b/drivers/mmc/card/Kconfig
index 86948f9..063fa16 100644
--- a/drivers/mmc/card/Kconfig
+++ b/drivers/mmc/card/Kconfig
@@ -14,6 +14,13 @@ config MMC_BLOCK
 	  mount the filesystem. Almost everyone wishing MMC support
 	  should say Y or M here.
 
+config MMC_BLOCK_QUIRKS
+       tristate "MMC block device quirks"
+       depends on MMC_BLOCK
+       default y
+       help
+         Say Y here to enable various workarounds for known cards.
+
 config MMC_BLOCK_BOUNCE
 	bool "Use bounce buffer for simple hosts"
 	depends on MMC_BLOCK
diff --git a/drivers/mmc/card/Makefile b/drivers/mmc/card/Makefile
index 0d40751..fcd3f45 100644
--- a/drivers/mmc/card/Makefile
+++ b/drivers/mmc/card/Makefile
@@ -7,6 +7,7 @@ ifeq ($(CONFIG_MMC_DEBUG),y)
 endif
 
 obj-$(CONFIG_MMC_BLOCK)		+= mmc_block.o
+obj-$(CONFIG_MMC_BLOCK_QUIRKS)  += block-quirks.o
 mmc_block-objs			:= block.o queue.o
 obj-$(CONFIG_MMC_TEST)		+= mmc_test.o
 
diff --git a/drivers/mmc/card/blk.h b/drivers/mmc/card/blk.h
new file mode 100644
index 0000000..3e7b8e6
--- /dev/null
+++ b/drivers/mmc/card/blk.h
@@ -0,0 +1,70 @@
+#ifndef MMC_BLK_H
+#define MMC_BLK_H
+
+struct gendisk;
+
+#ifdef CONFIG_MMC_BLOCK_QUIRKS
+struct mmc_blk_data;
+
+#define mmc_blk_qrev(hwrev, fwrev, year, month) \
+	(((u64) hwrev) << 40 |                  \
+	 ((u64) fwrev) << 32 |                  \
+	 ((u64) year) << 16 |                   \
+	 ((u64) month))
+
+#define mmc_blk_qrev_card(card)       \
+	mmc_blk_qrev(card->cid.hwrev, \
+		     card->cid.fwrev, \
+		     card->cid.year,  \
+		     card->cid.month)
+
+#define MMC_BLK_QUIRK_VERSION(_name, _manfid, _oemid, _rev_start, _rev_end, _probe) \
+	{							\
+		.name = (_name),				\
+		.manfid = (_manfid),				\
+		.oemid = (_oemid),				\
+		.rev_start = (_rev_start),			\
+		.rev_end = (_rev_end),				\
+		.probe = (_probe),				\
+	}
+
+#define MMC_BLK_QUIRK(_name, _manfid, _oemid, _probe) \
+	MMC_BLK_QUIRK_VERSION(_name, _manfid, _oemid, 0, -1ull, _probe)
+
+extern struct mmc_blk_quirk *mmc_blk_quirk_find(struct mmc_card *card);
+#else
+static inline struct mmc_blk_quirk *mmc_blk_quirk_find(struct mmc_card *card)
+{
+	return NULL;
+}
+#endif /* CONFIG_MMC_BLOCK_QUIRKS */
+
+struct mmc_blk_quirk {
+	const char *name;
+
+	/* First valid revision */
+	u64 rev_start;
+
+	/* Last valid revision */
+	u64 rev_end;
+
+	unsigned int manfid;
+	unsigned short oemid;
+	int (*probe)(struct mmc_blk_data *, struct mmc_card *);
+};
+
+/*
+ * There is one mmc_blk_data per slot.
+ */
+struct mmc_blk_data {
+	spinlock_t	lock;
+	struct gendisk	*disk;
+	struct mmc_queue queue;
+
+	unsigned int	usage;
+	unsigned int	read_only;
+	unsigned int	write_align_size;
+	unsigned int	write_align_limit;
+};
+
+#endif
diff --git a/drivers/mmc/card/block-quirks.c b/drivers/mmc/card/block-quirks.c
new file mode 100644
index 0000000..e122a74
--- /dev/null
+++ b/drivers/mmc/card/block-quirks.c
@@ -0,0 +1,51 @@
+/*
+ * linux/drivers/mmc/card/block-quirks.c
+ *
+ * Copyright (C) 2010 Andrei Warkentin <andreiw@motorola.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+#include <linux/kernel.h>
+#include <linux/semaphore.h>
+#include <linux/mmc/card.h>
+
+#include "queue.h"
+#include "blk.h"
+
+/*
+   Caveat: Because this list is just looked up with a linear
+   search, take care that either overlapping revision ranges
+   don't exist, or that returning the first quirk that matches
+   a given revision has the desired effect.
+*/
+struct mmc_blk_quirk mmc_blk_quirks[] = {
+};
+
+static int mmc_blk_quirk_cmp(const struct mmc_blk_quirk *quirk,
+			      const struct mmc_card *card)
+{
+	u64 rev = mmc_blk_qrev_card(card);
+
+	if (quirk->manfid == card->cid.manfid &&
+	    quirk->oemid == card->cid.oemid &&
+	    !strcmp(quirk->name, card->cid.prod_name) &&
+	    rev >= quirk->rev_start &&
+	    rev <= quirk->rev_end)
+		return 0;
+
+	return -1;
+}
+
+struct mmc_blk_quirk *mmc_blk_quirk_find(struct mmc_card *card)
+{
+	unsigned index;
+
+	for (index = 0; index < ARRAY_SIZE(mmc_blk_quirks); index++)
+		if (!mmc_blk_quirk_cmp(&mmc_blk_quirks[index], card))
+			return &mmc_blk_quirks[index];
+
+	return NULL;
+}
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 9d44480..8640902 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -42,6 +42,7 @@
 #include <asm/uaccess.h>
 
 #include "queue.h"
+#include "blk.h"
 
 MODULE_ALIAS("mmc:block");
 
@@ -53,20 +54,6 @@ MODULE_ALIAS("mmc:block");
 
 static DECLARE_BITMAP(dev_use, MMC_NUM_MINORS);
 
-/*
- * There is one mmc_blk_data per slot.
- */
-struct mmc_blk_data {
-	spinlock_t	lock;
-	struct gendisk	*disk;
-	struct mmc_queue queue;
-
-	unsigned int	usage;
-	unsigned int	read_only;
-	unsigned int	write_align_size;
-	unsigned int	write_align_limit;
-};
-
 static DEFINE_MUTEX(open_lock);
 
 static struct mmc_blk_data *mmc_blk_get(struct gendisk *disk)
@@ -732,6 +719,7 @@ mmc_blk_set_blksize(struct mmc_blk_data *md, struct mmc_card *card)
 static int mmc_blk_probe(struct mmc_card *card)
 {
 	struct mmc_blk_data *md;
+	struct mmc_blk_quirk *quirk;
 	int err;
 
 	char cap_str[10];
@@ -750,6 +738,12 @@ static int mmc_blk_probe(struct mmc_card *card)
 	if (err)
 		goto out;
 
+	quirk = mmc_blk_quirk_find(card);
+	if (quirk && quirk->probe)
+		err = quirk->probe(md, card);
+	if (err)
+		goto out;
+
 	string_get_size((u64)get_capacity(md->disk) << 9, STRING_UNITS_2,
 			cap_str, sizeof(cap_str));
 	printk(KERN_INFO "%s: %s %s %s %s\n",
-- 
1.7.0.4

[[RFC] 3/5] MMC: Toshiba eMMC - Split 8K-unaligned accesses.

[Andrei Warkentin]

These cards show abysmal write performance when
writes < 12K that are 8K unaligned cross an 8K barrier.

Signed-off-by: Andrei Warkentin <andreiw@motorola.com>
---
 drivers/mmc/card/Kconfig        |    8 ++++++++
 drivers/mmc/card/block-quirks.c |   22 ++++++++++++++++++++++
 2 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/card/Kconfig b/drivers/mmc/card/Kconfig
index 063fa16..af2800e 100644
--- a/drivers/mmc/card/Kconfig
+++ b/drivers/mmc/card/Kconfig
@@ -21,6 +21,14 @@ config MMC_BLOCK_QUIRKS
        help
          Say Y here to enable various workarounds for known cards.
 
+config MMC_BLOCK_QUIRK_TOSHIBA_32NM
+       tristate "Toshiba MMC 32nm technology flash device quirks"
+       depends on MMC_BLOCK_QUIRKS
+       default n
+       help
+         Say Y if you have a Toshiba 32nm technology flash device,
+	 such as MMC32G or MMC16G eMMCs.
+
 config MMC_BLOCK_BOUNCE
 	bool "Use bounce buffer for simple hosts"
 	depends on MMC_BLOCK
diff --git a/drivers/mmc/card/block-quirks.c b/drivers/mmc/card/block-quirks.c
index e122a74..c142ffb 100644
--- a/drivers/mmc/card/block-quirks.c
+++ b/drivers/mmc/card/block-quirks.c
@@ -15,6 +15,23 @@
 #include "queue.h"
 #include "blk.h"
 
+#ifdef CONFIG_MMC_BLOCK_QUIRK_TOSHIBA_32NM
+static int toshiba_32nm_probe(struct mmc_blk_data *md, struct mmc_card *card)
+{
+	printk(KERN_INFO "Applying Toshiba 32nm workarounds\n");
+
+	/* Page size 8K, this card doesn't like unaligned writes
+	   across 8K boundary. */
+	md->write_align_size = 8192;
+
+	/* Doing the alignment for accesses > 12K seems to
+	   result in decreased perf. */
+	md->write_align_limit = 12288;
+	return 0;
+}
+#endif /* CONFIG_MMC_BLOCK_QUIRK_TOSHIBA_32NM */
+
+
 /*
    Caveat: Because this list is just looked up with a linear
    search, take care that either overlapping revision ranges
@@ -22,6 +39,10 @@
    a given revision has the desired effect.
 */
 struct mmc_blk_quirk mmc_blk_quirks[] = {
+#ifdef CONFIG_MMC_BLOCK_QUIRK_TOSHIBA_32NM
+        MMC_BLK_QUIRK("MMC16G", 0x11, 0x0, toshiba_32nm_probe),
+        MMC_BLK_QUIRK("MMC32G", 0x11, 0x0100, toshiba_32nm_probe),
+#endif /* CONFIG_MMC_BLOCK_QUIRK_TOSHIBA_32NM */
 };
 
 static int mmc_blk_quirk_cmp(const struct mmc_blk_quirk *quirk,
@@ -49,3 +70,4 @@ struct mmc_blk_quirk *mmc_blk_quirk_find(struct mmc_card *card)
 
 	return NULL;
 }
+
-- 
1.7.0.4

[[RFC] 4/5] MMC: Block quirks request adjust support.

[Andrei Warkentin]

Lets a card specific quirk affect how MMC requests are made.
Necessary for Toshiba block-splitting part reliability workaround.

Signed-off-by: Andrei Warkentin <andreiw@motorola.com>
---
 drivers/mmc/card/blk.h          |    9 ++++++---
 drivers/mmc/card/block-quirks.c |    5 ++---
 drivers/mmc/card/block.c        |   12 ++++++++----
 3 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/card/blk.h b/drivers/mmc/card/blk.h
index 3e7b8e6..3e72b67 100644
--- a/drivers/mmc/card/blk.h
+++ b/drivers/mmc/card/blk.h
@@ -18,7 +18,7 @@ struct mmc_blk_data;
 		     card->cid.year,  \
 		     card->cid.month)
 
-#define MMC_BLK_QUIRK_VERSION(_name, _manfid, _oemid, _rev_start, _rev_end, _probe) \
+#define MMC_BLK_QUIRK_VERSION(_name, _manfid, _oemid, _rev_start, _rev_end, _probe, _adjust) \
 	{							\
 		.name = (_name),				\
 		.manfid = (_manfid),				\
@@ -26,10 +26,11 @@ struct mmc_blk_data;
 		.rev_start = (_rev_start),			\
 		.rev_end = (_rev_end),				\
 		.probe = (_probe),				\
+		.adjust = (_adjust),				\
 	}
 
-#define MMC_BLK_QUIRK(_name, _manfid, _oemid, _probe) \
-	MMC_BLK_QUIRK_VERSION(_name, _manfid, _oemid, 0, -1ull, _probe)
+#define MMC_BLK_QUIRK(_name, _manfid, _oemid, _probe, _adjust)	\
+	MMC_BLK_QUIRK_VERSION(_name, _manfid, _oemid, 0, -1ull, _probe, _adjust)
 
 extern struct mmc_blk_quirk *mmc_blk_quirk_find(struct mmc_card *card);
 #else
@@ -51,6 +52,7 @@ struct mmc_blk_quirk {
 	unsigned int manfid;
 	unsigned short oemid;
 	int (*probe)(struct mmc_blk_data *, struct mmc_card *);
+	void (*adjust)(struct mmc_queue *, struct request *, struct mmc_request *);
 };
 
 /*
@@ -65,6 +67,7 @@ struct mmc_blk_data {
 	unsigned int	read_only;
 	unsigned int	write_align_size;
 	unsigned int	write_align_limit;
+	struct mmc_blk_quirk *quirk;
 };
 
 #endif
diff --git a/drivers/mmc/card/block-quirks.c b/drivers/mmc/card/block-quirks.c
index c142ffb..4afa872 100644
--- a/drivers/mmc/card/block-quirks.c
+++ b/drivers/mmc/card/block-quirks.c
@@ -31,7 +31,6 @@ static int toshiba_32nm_probe(struct mmc_blk_data *md, struct mmc_card *card)
 }
 #endif /* CONFIG_MMC_BLOCK_QUIRK_TOSHIBA_32NM */
 
-
 /*
    Caveat: Because this list is just looked up with a linear
    search, take care that either overlapping revision ranges
@@ -40,8 +39,8 @@ static int toshiba_32nm_probe(struct mmc_blk_data *md, struct mmc_card *card)
 */
 struct mmc_blk_quirk mmc_blk_quirks[] = {
 #ifdef CONFIG_MMC_BLOCK_QUIRK_TOSHIBA_32NM
-        MMC_BLK_QUIRK("MMC16G", 0x11, 0x0, toshiba_32nm_probe),
-        MMC_BLK_QUIRK("MMC32G", 0x11, 0x0100, toshiba_32nm_probe),
+        MMC_BLK_QUIRK("MMC16G", 0x11, 0x0, toshiba_32nm_probe, NULL),
+        MMC_BLK_QUIRK("MMC32G", 0x11, 0x0100, toshiba_32nm_probe, NULL),
 #endif /* CONFIG_MMC_BLOCK_QUIRK_TOSHIBA_32NM */
 };
 
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 8640902..52e66d6 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -413,6 +413,11 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *req)
 		brq.data.sg = mq->sg;
 		brq.data.sg_len = mmc_queue_map_sg(mq);
 
+#ifdef CONFIG_MMC_BLOCK_QUIRKS
+	if (md->quirk && md->quirk->adjust)
+		md->quirk->adjust(mq, req, &brq.mrq);
+#endif /* CONFIG_MMC_BLOCK_QUIRKS */
+
 		/*
 		 * Adjust the sg list so it is the same size as the
 		 * request.
@@ -719,7 +724,6 @@ mmc_blk_set_blksize(struct mmc_blk_data *md, struct mmc_card *card)
 static int mmc_blk_probe(struct mmc_card *card)
 {
 	struct mmc_blk_data *md;
-	struct mmc_blk_quirk *quirk;
 	int err;
 
 	char cap_str[10];
@@ -738,9 +742,9 @@ static int mmc_blk_probe(struct mmc_card *card)
 	if (err)
 		goto out;
 
-	quirk = mmc_blk_quirk_find(card);
-	if (quirk && quirk->probe)
-		err = quirk->probe(md, card);
+	md->quirk = mmc_blk_quirk_find(card);
+	if (md->quirk && md->quirk->probe)
+		err = md->quirk->probe(md, card);
 	if (err)
 		goto out;
 
-- 
1.7.0.4

[[RFC] 5/5] MMC: Toshiba eMMC - Part reliability improvement.

[Andrei Warkentin]

Split-up accesses into smaller chunks, to improve
lifespan, by using 8K reliable writes into 8K Buffer A,
instead of 4MB Buffer B, reducing number of 4MB write-erase
cycles. Upper and lower bounds should be experimentally
found to match the desired performance/reliability
characteristics.

Signed-off-by: Andrei Warkentin <andreiw@motorola.com>
---
 drivers/mmc/card/Kconfig        |   31 ++++++++++++++++++++++++
 drivers/mmc/card/block-quirks.c |   50 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 79 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/card/Kconfig b/drivers/mmc/card/Kconfig
index af2800e..5c2f090 100644
--- a/drivers/mmc/card/Kconfig
+++ b/drivers/mmc/card/Kconfig
@@ -29,6 +29,37 @@ config MMC_BLOCK_QUIRK_TOSHIBA_32NM
          Say Y if you have a Toshiba 32nm technology flash device,
 	 such as MMC32G or MMC16G eMMCs.
 
+config MMC_BLOCK_QUIRK_TOSHIBA_32NM_REL
+       tristate "Toshiba MMC 32nm reliability quirk"
+       depends on MMC_BLOCK_QUIRK_TOSHIBA_32NM
+       default n
+       help
+         Say Y if you want to enable the improved reliability workaround
+	 for your Toshiba 32nm parts. This will result in certain-sized
+	 writes to be split up into 8K chunks, to ensure they are placed
+	 in the smaller 8KB buffer instead of the 4MB buffer, which should
+	 reduce the number of flash write-erase cycles and improve
+	 reliability. By default accesses in the 24k-32k range are
+	 split.
+
+config MMC_BLOCK_QUIRK_TOSHIBA_32NM_REL_L
+       hex "Toshiba reliability lower bound (in blocks)"
+       depends on MMC_BLOCK_QUIRK_TOSHIBA_32NM_REL
+       default "30"
+       help
+         Accesses smaller than the lower bound will not be split.
+	 This value should be experimentally found to match load
+	 and performance characteristics.
+
+config MMC_BLOCK_QUIRK_TOSHIBA_32NM_REL_U
+       hex "Toshiba reliability upper bound (in blocks)"
+       depends on MMC_BLOCK_QUIRK_TOSHIBA_32NM_REL
+       default "40"
+       help
+         Accesses bigger than the upper bound will not be split.
+	 This value should be experimentally found to match load
+	 and performance characteristics.
+
 config MMC_BLOCK_BOUNCE
 	bool "Use bounce buffer for simple hosts"
 	depends on MMC_BLOCK
diff --git a/drivers/mmc/card/block-quirks.c b/drivers/mmc/card/block-quirks.c
index 4afa872..c918e12 100644
--- a/drivers/mmc/card/block-quirks.c
+++ b/drivers/mmc/card/block-quirks.c
@@ -9,8 +9,10 @@
  *
  */
 #include <linux/kernel.h>
+#include <linux/blkdev.h>
 #include <linux/semaphore.h>
 #include <linux/mmc/card.h>
+#include <linux/mmc/mmc.h>
 
 #include "queue.h"
 #include "blk.h"
@@ -19,6 +21,11 @@
 static int toshiba_32nm_probe(struct mmc_blk_data *md, struct mmc_card *card)
 {
 	printk(KERN_INFO "Applying Toshiba 32nm workarounds\n");
+#ifdef CONFIG_MMC_BLOCK_QUIRK_TOSHIBA_32NM_REL
+	printk(KERN_INFO "Toshiba 32nm reliability splits over 0x%x-0x%x blocks\n",
+	       CONFIG_MMC_BLOCK_QUIRK_TOSHIBA_32NM_REL_L,
+	       CONFIG_MMC_BLOCK_QUIRK_TOSHIBA_32NM_REL_U);
+#endif
 
 	/* Page size 8K, this card doesn't like unaligned writes
 	   across 8K boundary. */
@@ -29,6 +36,43 @@ static int toshiba_32nm_probe(struct mmc_blk_data *md, struct mmc_card *card)
 	md->write_align_limit = 12288;
 	return 0;
 }
+
+#ifdef CONFIG_MMC_BLOCK_QUIRK_TOSHIBA_32NM_REL
+static void toshiba_32nm_adjust(struct mmc_queue *mq,
+				struct request *req,
+				struct mmc_request *mrq)
+{
+
+	int err;
+	struct mmc_command cmd;
+	struct mmc_blk_data *md = mq->data;
+	struct mmc_card *card = md->queue.card;
+
+	if (rq_data_dir(req) != WRITE)
+		return;
+
+	if (blk_rq_sectors(req) > CONFIG_MMC_BLOCK_QUIRK_TOSHIBA_32NM_REL_U ||
+	    blk_rq_sectors(req) < CONFIG_MMC_BLOCK_QUIRK_TOSHIBA_32NM_REL_L)
+		return;
+
+	/* 8K chunks */
+	if (mrq->data->blocks > 16)
+		mrq->data->blocks = 16;
+
+	/*
+	  We know what the valid values for this card are,
+	  no need to check EXT_CSD_REL_WR_SEC_C.
+	 */
+	cmd.opcode = MMC_SET_BLOCK_COUNT | (1 << 31);
+	cmd.arg = mrq->data->blocks;
+	cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
+	err = mmc_wait_for_cmd(card->host, &cmd, 0);
+	if (!err)
+		mrq->stop = NULL;
+}
+#else
+#define toshiba_32nm_adjust NULL
+#endif /* CONFIG_MMC_BLOCK_QUIRK_TOSHIBA_32NM_REL */
 #endif /* CONFIG_MMC_BLOCK_QUIRK_TOSHIBA_32NM */
 
 /*
@@ -39,8 +83,10 @@ static int toshiba_32nm_probe(struct mmc_blk_data *md, struct mmc_card *card)
 */
 struct mmc_blk_quirk mmc_blk_quirks[] = {
 #ifdef CONFIG_MMC_BLOCK_QUIRK_TOSHIBA_32NM
-        MMC_BLK_QUIRK("MMC16G", 0x11, 0x0, toshiba_32nm_probe, NULL),
-        MMC_BLK_QUIRK("MMC32G", 0x11, 0x0100, toshiba_32nm_probe, NULL),
+        MMC_BLK_QUIRK("MMC16G", 0x11, 0x0,
+		      toshiba_32nm_probe, toshiba_32nm_adjust),
+        MMC_BLK_QUIRK("MMC32G", 0x11, 0x0100,
+		      toshiba_32nm_probe, toshiba_32nm_adjust),
 #endif /* CONFIG_MMC_BLOCK_QUIRK_TOSHIBA_32NM */
 };
 
-- 
1.7.0.4

Re: [[RFC] 2/5] MMC: Add block quirks support.

[Linus Walleij]

2011/3/5 Andrei Warkentin <andreiw@motorola.com>:

> Quirks are card-specific workarounds. Usually they involve
> tuning mmcblk parameters at mmc_blk_probe time.

Can't you just drop off all the *block* and *blk* pre- and
suffixes off filenames and functions?

It's inevitably going to be used for all kind of card quirks
soon enough, not just those relating to block issues
(could be power quirks etc) won't it?

"Card quirks" is more intuitive to me.

Correct me if I'm wrong.

Yours,
Linus Walleij

Re: [[RFC] 2/5] MMC: Add block quirks support.

[Linus Walleij]

2011/3/6 Linus Walleij <linus.walleij@linaro.org>:
> 2011/3/5 Andrei Warkentin <andreiw@motorola.com>:
>
>> Quirks are card-specific workarounds. Usually they involve
>> tuning mmcblk parameters at mmc_blk_probe time.
>
> Can't you just drop off all the *block* and *blk* pre- and
> suffixes off filenames and functions?
>
> It's inevitably going to be used for all kind of card quirks
> soon enough, not just those relating to block issues
> (could be power quirks etc) won't it?
>
> "Card quirks" is more intuitive to me.

Or rather - if you check out Chris latest tree or the next
tree - you will find the new quirks.c file that Pierre Tardy
created for some SDIO quirks.

Can you re-use this infrastructure and refactor/expand
with the quirks you need for the block stuff?

Yours,
Linus Walleij

Re: [[RFC] 2/5] MMC: Add block quirks support.

[Andrei Warkentin]

On Sun, Mar 6, 2011 at 3:20 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> 2011/3/6 Linus Walleij <linus.walleij@linaro.org>:
>> 2011/3/5 Andrei Warkentin <andreiw@motorola.com>:
>>
>>> Quirks are card-specific workarounds. Usually they involve
>>> tuning mmcblk parameters at mmc_blk_probe time.
>>
>> Can't you just drop off all the *block* and *blk* pre- and
>> suffixes off filenames and functions?
>>
>> It's inevitably going to be used for all kind of card quirks
>> soon enough, not just those relating to block issues
>> (could be power quirks etc) won't it?
>>
>> "Card quirks" is more intuitive to me.
>
> Or rather - if you check out Chris latest tree or the next
> tree - you will find the new quirks.c file that Pierre Tardy
> created for some SDIO quirks.
>
> Can you re-use this infrastructure and refactor/expand
> with the quirks you need for the block stuff?
>
> Yours,
> Linus Walleij
>

I'll check it out. I wish I looked earlier :-).

A

Re: [[RFC] 2/5] MMC: Add block quirks support.

[Andrei Warkentin]

On Mon, Mar 7, 2011 at 2:16 AM, Andrei Warkentin <andreiw@motorola.com> wrote:
> On Sun, Mar 6, 2011 at 3:20 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> 2011/3/6 Linus Walleij <linus.walleij@linaro.org>:
>>> 2011/3/5 Andrei Warkentin <andreiw@motorola.com>:
>>>
>>>> Quirks are card-specific workarounds. Usually they involve
>>>> tuning mmcblk parameters at mmc_blk_probe time.
>>>
>>> Can't you just drop off all the *block* and *blk* pre- and
>>> suffixes off filenames and functions?
>>>
>>> It's inevitably going to be used for all kind of card quirks
>>> soon enough, not just those relating to block issues
>>> (could be power quirks etc) won't it?
>>>
>>> "Card quirks" is more intuitive to me.
>>
>> Or rather - if you check out Chris latest tree or the next
>> tree - you will find the new quirks.c file that Pierre Tardy
>> created for some SDIO quirks.
>>
>> Can you re-use this infrastructure and refactor/expand
>> with the quirks you need for the block stuff?
>>
>> Yours,
>> Linus Walleij
>>
>
> I'll check it out. I wish I looked earlier :-).
>
> A
>

I took a look at quirks.c in linux-next. It's a bit sdio-specific... I
would extend mmc_fixup to contain card type (to know if cis
vendor/device should be checked) as well as name/manfid/oemid and rev
id (which is just a combination of hwrev, fwrev and date).

Vendor_fixup would be the top-level card fixup.
I would add blk_fixup (so block specific information like desired
write align size can be kept inside mmc_blk_data instead of polluting
mmc_card).

...and to support part reliability workarounds for Toshiba cards, will
add a blk_adjust method to be invoked inside blk req handling.

Arnd, what do you think?

Pierre, what do you think about my above ideas of extending the
current quirks.c? Please see the following patches, which were my
original submissions...
http://article.gmane.org/gmane.linux.kernel.mmc/6468/match=
http://article.gmane.org/gmane.linux.kernel.mmc/6467/match=
http://article.gmane.org/gmane.linux.kernel.mmc/6469/match=
http://article.gmane.org/gmane.linux.kernel.mmc/6470/match=
http://article.gmane.org/gmane.linux.kernel.mmc/6466/match=

Thanks,
A

Re: [[RFC] 2/5] MMC: Add block quirks support.

[Andrei Warkentin]

On Mon, Mar 7, 2011 at 2:39 PM, Andrei Warkentin <andreiw@motorola.com> wrote:
> On Mon, Mar 7, 2011 at 2:16 AM, Andrei Warkentin <andreiw@motorola.com> wrote:
>> On Sun, Mar 6, 2011 at 3:20 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> 2011/3/6 Linus Walleij <linus.walleij@linaro.org>:
>>>> 2011/3/5 Andrei Warkentin <andreiw@motorola.com>:
>>>>
>>>>> Quirks are card-specific workarounds. Usually they involve
>>>>> tuning mmcblk parameters at mmc_blk_probe time.
>>>>
>>>> Can't you just drop off all the *block* and *blk* pre- and
>>>> suffixes off filenames and functions?
>>>>
>>>> It's inevitably going to be used for all kind of card quirks
>>>> soon enough, not just those relating to block issues
>>>> (could be power quirks etc) won't it?
>>>>
>>>> "Card quirks" is more intuitive to me.
>>>
>>> Or rather - if you check out Chris latest tree or the next
>>> tree - you will find the new quirks.c file that Pierre Tardy
>>> created for some SDIO quirks.
>>>
>>> Can you re-use this infrastructure and refactor/expand
>>> with the quirks you need for the block stuff?
>>>
>>> Yours,
>>> Linus Walleij
>>>
>>
>> I'll check it out. I wish I looked earlier :-).
>>
>> A
>>
>
> I took a look at quirks.c in linux-next. It's a bit sdio-specific... I
> would extend mmc_fixup to contain card type (to know if cis
> vendor/device should be checked) as well as name/manfid/oemid and rev
> id (which is just a combination of hwrev, fwrev and date).
>
> Vendor_fixup would be the top-level card fixup.
> I would add blk_fixup (so block specific information like desired
> write align size can be kept inside mmc_blk_data instead of polluting
> mmc_card).
>
> ...and to support part reliability workarounds for Toshiba cards, will
> add a blk_adjust method to be invoked inside blk req handling.
>
> Arnd, what do you think?
>
> Pierre, what do you think about my above ideas of extending the
> current quirks.c? Please see the following patches, which were my
> original submissions...
> http://article.gmane.org/gmane.linux.kernel.mmc/6468/match=
> http://article.gmane.org/gmane.linux.kernel.mmc/6467/match=
> http://article.gmane.org/gmane.linux.kernel.mmc/6469/match=
> http://article.gmane.org/gmane.linux.kernel.mmc/6470/match=
> http://article.gmane.org/gmane.linux.kernel.mmc/6466/match=
>
> Thanks,
> A
>

The other real issue I see is that it's kind of nasty to put
block-related workarounds into core/quirks.c. The later seems more of
generic card interface workarounds, rather than workarounds for
specific functionality. It would be like putting workarounds for, say,
sdio_uart into core/quirk.c, IMHO block workarounds are still
function-level workarounds, and so should be dealt with at the
function-level under mmc/card. Thoughts? Keep in mind also that for
various emmc brokeness (from writing to erasing), there is an express
need to be able to modify actual mmc commands during request
processing, or base behavior on per-card tunables (which are block
specific and shouldn't be in struct mmc_card). Can't get away with
just setting a quirk flag...

A

Re: [[RFC] 2/5] MMC: Add block quirks support.

[Linus Walleij]

2011/3/7 Andrei Warkentin <andreiw@motorola.com>:

> The other real issue I see is that it's kind of nasty to put
> block-related workarounds into core/quirks.c. The later seems more of
> generic card interface workarounds, rather than workarounds for
> specific functionality. It would be like putting workarounds for, say,
> sdio_uart into core/quirk.c, IMHO block workarounds are still
> function-level workarounds, and so should be dealt with at the
> function-level under mmc/card. Thoughts?

The SDIO cards are function-level too, so what you're saying is
that the file quirks.c should be moved from core/ to card/
and extended there?
If so, yes.

Linus Walleij

Re: [[RFC] 2/5] MMC: Add block quirks support.

[Andrei Warkentin]

On Tue, Mar 8, 2011 at 2:28 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> 2011/3/7 Andrei Warkentin <andreiw@motorola.com>:
>
>> The other real issue I see is that it's kind of nasty to put
>> block-related workarounds into core/quirks.c. The later seems more of
>> generic card interface workarounds, rather than workarounds for
>> specific functionality. It would be like putting workarounds for, say,
>> sdio_uart into core/quirk.c, IMHO block workarounds are still
>> function-level workarounds, and so should be dealt with at the
>> function-level under mmc/card. Thoughts?
>
> The SDIO cards are function-level too, so what you're saying is
> that the file quirks.c should be moved from core/ to card/
> and extended there?
> If so, yes.
>
> Linus Walleij
>

Alright :-).

I'll extend the current mechanism in quirks.c to match on manfid/oemid
and revision. In fact, the current code
assumes all cards use cis.vendor and cis.device, but that only applies
to SDIO cards.

Thanks,
A

Re: [[RFC] 2/5] MMC: Add block quirks support.

[Andrei Warkentin]

On Tue, Mar 8, 2011 at 2:34 PM, Andrei Warkentin <andreiw@motorola.com> wrote:
> On Tue, Mar 8, 2011 at 2:28 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> 2011/3/7 Andrei Warkentin <andreiw@motorola.com>:
>>
>>> The other real issue I see is that it's kind of nasty to put
>>> block-related workarounds into core/quirks.c. The later seems more of
>>> generic card interface workarounds, rather than workarounds for
>>> specific functionality. It would be like putting workarounds for, say,
>>> sdio_uart into core/quirk.c, IMHO block workarounds are still
>>> function-level workarounds, and so should be dealt with at the
>>> function-level under mmc/card. Thoughts?
>>
>> The SDIO cards are function-level too, so what you're saying is
>> that the file quirks.c should be moved from core/ to card/
>> and extended there?
>> If so, yes.
>>
>> Linus Walleij
>>
>
> Alright :-).
>
> I'll extend the current mechanism in quirks.c to match on manfid/oemid
> and revision. In fact, the current code
> assumes all cards use cis.vendor and cis.device, but that only applies
> to SDIO cards.
>
> Thanks,
> A
>

On a second thought it should still belong within core, since it's
getting built as part of the MMC core support... The tables could be
per-function-driver, I suppose...
So mmc_fixup_device takes as an additional parameter a pointer to the
lookup table...

A

RE: [[RFC] 2/5] MMC: Add block quirks support.

[Tardy, Pierre]

> I took a look at quirks.c in linux-next. It's a bit sdio-specific... I
> would extend mmc_fixup to contain card type (to know if cis
> vendor/device should be checked) as well as name/manfid/oemid and rev
> id (which is just a combination of hwrev, fwrev and date).
Feel free to change mmc_fixup. The quirk.c is made to be extended.

> 
> Vendor_fixup would be the top-level card fixup.
> I would add blk_fixup (so block specific information like desired
> write align size can be kept inside mmc_blk_data instead of polluting
> mmc_card).
> 
> ...and to support part reliability workarounds for Toshiba cards, will
> add a blk_adjust method to be invoked inside blk req handling.
> 
> Arnd, what do you think?
> 
> Pierre, what do you think about my above ideas of extending the
> current quirks.c? Please see the following patches, which were my
> original submissions...
I think your work sounds promising.
Look at pci/quirk.c it's 3k loc, with very various things.
IMHO, we should just list cards, and set quirk flags, and quirk parameters in quirk.c.
And put nice generic card independent quirk code in block.c.

How is it possible to say generically: "avoid writes < 12K that are 8K unaligned and cross an 8K barrier"?

Pierre
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [[RFC] 2/5] MMC: Add block quirks support.

[Arnd Bergmann]

On Monday 07 March 2011, Andrei Warkentin wrote:
> 
> I took a look at quirks.c in linux-next. It's a bit sdio-specific... I
> would extend mmc_fixup to contain card type (to know if cis
> vendor/device should be checked) as well as name/manfid/oemid and rev
> id (which is just a combination of hwrev, fwrev and date).

Makes sense.
 
> Vendor_fixup would be the top-level card fixup.
> I would add blk_fixup (so block specific information like desired
> write align size can be kept inside mmc_blk_data instead of polluting
> mmc_card).
> 
> ...and to support part reliability workarounds for Toshiba cards, will
> add a blk_adjust method to be invoked inside blk req handling.
> 
> Arnd, what do you think?

I'm still not comfortable with having the blk_adjust method. I think it
would be much better to encode that behavior in an abstract way in
the block driver and set a flag to enable it in the quirks file.

	Arnd

Re: [[RFC] 2/5] MMC: Add block quirks support.

[Andrei Warkentin]

On Tue, Mar 8, 2011 at 8:42 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>
> I'm still not comfortable with having the blk_adjust method. I think it
> would be much better to encode that behavior in an abstract way in
> the block driver and set a flag to enable it in the quirks file.

Ok, will encode the behavior abstractly and enable it with a flag. No more
adjust method.

Thanks,
A

MMC quirks support.

[Andrei Warkentin]

This is the third version of the patch set that adds MMC block. Omitting
the Toshiba quirks for now as I need to revalidate the data behind the quirks.

Table of Contents:
[PATCH 1/2] MMC: Extends card quicks with MMC/SD quirks matching the CID.
[PATCH 2/2] MMC: Support for block quirks.

Thanks,
A

[PATCH 1/2] MMC: Extends card quicks with MMC/SD quirks matching the CID.

[Andrei Warkentin]

The current mechanism is SDIO-only. This allows us to create
function-specific quirks, without creating messy Kconfig dependencies,
or polluting core/ with function-specific code.

Signed-off-by: Andrei Warkentin <andreiw@motorola.com>
---
 drivers/mmc/core/core.h   |    2 -
 drivers/mmc/core/quirks.c |   82 ++++++++++++++++++----------------------
 drivers/mmc/core/sdio.c   |    2 +-
 include/linux/mmc/card.h  |   92 ++++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 129 insertions(+), 49 deletions(-)

diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index a2a956b..406a50f 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -59,8 +59,6 @@ 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;
 extern int mmc_assume_removable;
diff --git a/drivers/mmc/core/quirks.c b/drivers/mmc/core/quirks.c
index 4fb16ac..8cc359d 100644
--- a/drivers/mmc/core/quirks.c
+++ b/drivers/mmc/core/quirks.c
@@ -11,71 +11,63 @@
 #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 (*vendor_fixup)(struct mmc_card *card, int data);
-	int data;
-};
+#ifndef SDIO_VENDOR_ID_TI
+#define SDIO_VENDOR_ID_TI		0x0097
+#endif
 
-/*
- * This hook just adds a quirk unconditionnally
- */
-static void __maybe_unused add_quirk(struct mmc_card *card, int data)
-{
-	card->quirks |= data;
-}
+#ifndef SDIO_DEVICE_ID_TI_WL1271
+#define SDIO_DEVICE_ID_TI_WL1271	0x4076
+#endif
 
-/*
- * This hook just removes a quirk unconditionnally
- */
-static void __maybe_unused remove_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;
 }
 
-#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[] = {
+
 	/* by default sdio devices are considered CLK_GATING broken */
 	/* good cards will be whitelisted as they are tested */
-	{ 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, MMC_QUIRK_BROKEN_CLK_GATING },
-	{ 0 }
+	SDIO_FIXUP(SDIO_ANY_ID, SDIO_ANY_ID,
+		   add_quirk_for_sdio_devices,
+		   MMC_QUIRK_BROKEN_CLK_GATING),
+
+	SDIO_FIXUP(SDIO_VENDOR_ID_TI, SDIO_DEVICE_ID_TI_WL1271,
+		   remove_quirk, MMC_QUIRK_BROKEN_CLK_GATING),
+
+	END_FIXUP
 };
 
-void mmc_fixup_device(struct mmc_card *card)
+void mmc_fixup_device(struct mmc_card *card,
+	const struct mmc_fixup *table)
 {
 	const struct mmc_fixup *f;
+	u64 rev = cid_rev_card(card);
+
+	/* Non-core specific workarounds. */
+	if (!table)
+		table = mmc_fixup_methods;
 
-	for (f = mmc_fixup_methods; f->vendor_fixup; f++) {
-		if ((f->vendor == card->cis.vendor
-		     || f->vendor == (u16) SDIO_ANY_ID) &&
-		    (f->device == card->cis.device
-		     || f->device == (u16) SDIO_ANY_ID)) {
+	for (f = table; f->vendor_fixup; f++) {
+		if ((f->manfid == CID_MANFID_ANY
+		     || f->manfid == card->cid.manfid) &&
+		    (f->oemid == CID_OEMID_ANY
+		     || f->oemid == card->cid.oemid) &&
+		    (f->name == CID_NAME_ANY
+		     || !strcmp(f->name, card->cid.prod_name)) &&
+		    (f->cis_vendor == card->cis.vendor
+		     || f->cis_vendor == (u16) SDIO_ANY_ID) &&
+		    (f->cis_device == card->cis.device
+		    || f->cis_device == (u16) SDIO_ANY_ID) &&
+		    rev >= f->rev_start &&
+		    rev <= f->rev_end)	{
 			dev_dbg(&card->dev, "calling %pF\n", f->vendor_fixup);
 			f->vendor_fixup(card, f->data);
 		}
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 50749f5..d3ec4dc 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -480,7 +480,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 		card = oldcard;
 		return 0;
 	}
-	mmc_fixup_device(card);
+	mmc_fixup_device(card, NULL);
 
 	if (card->type == MMC_TYPE_SD_COMBO) {
 		err = mmc_sd_setup_card(host, card, oldcard != NULL);
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index fe9d7be..00fdeb9 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -11,6 +11,7 @@
 #define LINUX_MMC_CARD_H
 
 #include <linux/mmc/core.h>
+#include <linux/mod_devicetable.h>
 
 struct mmc_cid {
 	unsigned int		manfid;
@@ -149,7 +150,93 @@ struct mmc_card {
 	struct dentry		*debugfs_root;
 };
 
-void mmc_fixup_device(struct mmc_card *dev);
+/*
+ *  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 {
+
+	/* CID-specific fields. */
+	const char *name;
+
+	/* Valid revision range */
+	u64 rev_start, rev_end;
+
+	unsigned int manfid;
+	unsigned short oemid;
+
+       /* SDIO-specfic fields. You can use SDIO_ANY_ID here of course */
+	u16 cis_vendor, cis_device;
+
+	void (*vendor_fixup)(struct mmc_card *card, int data);
+	int data;
+};
+
+#define CID_MANFID_ANY (-1ul)
+#define CID_OEMID_ANY ((unsigned short) -1)
+#define CID_NAME_ANY (NULL)
+
+#define END_FIXUP { 0 }
+
+#define _FIXUP_EXT(_name, _manfid, _oemid, _rev_start, _rev_end,	\
+		   _cis_vendor, _cis_device,				\
+		   _fixup, _data)					\
+	{						   \
+		.name = (_name),			   \
+		.manfid = (_manfid),			   \
+		.oemid = (_oemid),			   \
+		.rev_start = (_rev_start),		   \
+		.rev_end = (_rev_end),			   \
+		.cis_vendor = (_cis_vendor),		   \
+		.cis_device = (_cis_device),		   \
+		.vendor_fixup = (_fixup),		   \
+		.data = (_data),			   \
+	 }
+
+#define MMC_FIXUP_REV(_name, _manfid, _oemid, _rev_start, _rev_end,	\
+		      _fixup, _data)					\
+	_FIXUP_EXT(_name, _manfid,					\
+		   _oemid, _rev_start, _rev_end,			\
+		   SDIO_ANY_ID, SDIO_ANY_ID,				\
+		   _fixup, _data)					\
+
+#define MMC_FIXUP(_name, _manfid, _oemid, _fixup, _data) \
+	MMC_FIXUP_REV(_name, _manfid, _oemid, 0, -1ull, _fixup, _data)
+
+#define SDIO_FIXUP(_vendor, _device, _fixup, _data)			\
+	_FIXUP_EXT(CID_NAME_ANY, CID_MANFID_ANY,			\
+		    CID_OEMID_ANY, 0, -1ull,				\
+		   _vendor, _device,					\
+		   _fixup, _data)					\
+
+#define cid_rev(hwrev, fwrev, year, month)	\
+	(((u64) hwrev) << 40 |                  \
+	 ((u64) fwrev) << 32 |                  \
+	 ((u64) year) << 16 |                   \
+	 ((u64) month))
+
+#define cid_rev_card(card)		  \
+	cid_rev(card->cid.hwrev,	  \
+		    card->cid.fwrev,      \
+		    card->cid.year,	  \
+		    card->cid.month)
+
+/*
+ * This hook just adds a quirk unconditionnally
+ */
+static inline void __maybe_unused add_quirk(struct mmc_card *card, int data)
+{
+	card->quirks |= data;
+}
+
+/*
+ * This hook just removes a quirk unconditionnally
+ */
+static inline void __maybe_unused remove_quirk(struct mmc_card *card, int data)
+{
+	card->quirks &= ~data;
+}
 
 #define mmc_card_mmc(c)		((c)->type == MMC_TYPE_MMC)
 #define mmc_card_sd(c)		((c)->type == MMC_TYPE_SD)
@@ -196,4 +283,7 @@ struct mmc_driver {
 extern int mmc_register_driver(struct mmc_driver *);
 extern void mmc_unregister_driver(struct mmc_driver *);
 
+extern void mmc_fixup_device(struct mmc_card *card,
+			     const struct mmc_fixup *table);
+
 #endif
-- 
1.7.0.4

Re: [PATCH 1/2] MMC: Extends card quicks with MMC/SD quirks matching the CID.

[Arnd Bergmann]

On Sunday 13 March 2011 14:48:37 Andrei Warkentin wrote:
> The current mechanism is SDIO-only. This allows us to create
> function-specific quirks, without creating messy Kconfig dependencies,
> or polluting core/ with function-specific code.
> 
> Signed-off-by: Andrei Warkentin <andreiw@motorola.com>

Looks good to me.

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH 1/2] MMC: Extends card quicks with MMC/SD quirks matching the CID.

[Chris Ball]

Hi Andrei,

I've rebased this patch to include Ohad's recent wl1271 quirks, and was
about to apply it, but have one question -- would you mind resubmitting
with your use of strcmp() switched over to strncmp()?  It looks safe in
this case, but the less code that has to be audited for safety the
better.

Here's the rebased (and slightly punctuation-tidied) version:

diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index 20b1c08..ca1fdde 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -61,8 +61,6 @@ 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;
 
diff --git a/drivers/mmc/core/quirks.c b/drivers/mmc/core/quirks.c
index a4c42ed..9b79ddf 100644
--- a/drivers/mmc/core/quirks.c
+++ b/drivers/mmc/core/quirks.c
@@ -11,34 +11,14 @@
 #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 (*vendor_fixup)(struct mmc_card *card, int data);
-	int data;
-};
-
-/*
- * This hook just adds a quirk unconditionnally
- */
-static void __maybe_unused add_quirk(struct mmc_card *card, int data)
-{
-	card->quirks |= data;
-}
+#ifndef SDIO_VENDOR_ID_TI
+#define SDIO_VENDOR_ID_TI		0x0097
+#endif
 
-/*
- * This hook just removes a quirk unconditionnally
- */
-static void __maybe_unused remove_quirk(struct mmc_card *card, int data)
-{
-	card->quirks &= ~data;
-}
+#ifndef SDIO_DEVICE_ID_TI_WL1271
+#define SDIO_DEVICE_ID_TI_WL1271	0x4076
+#endif
 
 /*
  * This hook just adds a quirk for all sdio devices
@@ -49,37 +29,46 @@ 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[] = {
 	/* by default sdio devices are considered CLK_GATING broken */
 	/* good cards will be whitelisted as they are tested */
-	{ 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, MMC_QUIRK_BROKEN_CLK_GATING },
-	{ SDIO_VENDOR_ID_TI, SDIO_DEVICE_ID_TI_WL1271,
-		add_quirk, MMC_QUIRK_NONSTD_FUNC_IF },
-	{ SDIO_VENDOR_ID_TI, SDIO_DEVICE_ID_TI_WL1271,
-		add_quirk, MMC_QUIRK_DISABLE_CD },
-	{ 0 }
+	SDIO_FIXUP(SDIO_ANY_ID, SDIO_ANY_ID,
+		   add_quirk_for_sdio_devices,
+		   MMC_QUIRK_BROKEN_CLK_GATING),
+
+	SDIO_FIXUP(SDIO_VENDOR_ID_TI, SDIO_DEVICE_ID_TI_WL1271,
+		   remove_quirk, MMC_QUIRK_BROKEN_CLK_GATING),
+
+	SDIO_FIXUP(SDIO_VENDOR_ID_TI, SDIO_DEVICE_ID_TI_WL1271,
+		   add_quirk, MMC_QUIRK_NONSTD_FUNC_IF),
+
+	SDIO_FIXUP(SDIO_VENDOR_ID_TI, SDIO_DEVICE_ID_TI_WL1271,
+		   add_quirk, MMC_QUIRK_DISABLE_CD),
+
+	END_FIXUP
 };
 
-void mmc_fixup_device(struct mmc_card *card)
+void mmc_fixup_device(struct mmc_card *card, const struct mmc_fixup *table)
 {
 	const struct mmc_fixup *f;
+	u64 rev = cid_rev_card(card);
+
+	/* Non-core specific workarounds. */
+	if (!table)
+		table = mmc_fixup_methods;
 
-	for (f = mmc_fixup_methods; f->vendor_fixup; f++) {
-		if ((f->vendor == card->cis.vendor
-		     || f->vendor == (u16) SDIO_ANY_ID) &&
-		    (f->device == card->cis.device
-		     || f->device == (u16) SDIO_ANY_ID)) {
+	for (f = table; f->vendor_fixup; f++) {
+		if ((f->manfid == CID_MANFID_ANY ||
+		     f->manfid == card->cid.manfid) &&
+		    (f->oemid == CID_OEMID_ANY ||
+		     f->oemid == card->cid.oemid) &&
+		    (f->name == CID_NAME_ANY ||
+		     !strcmp(f->name, card->cid.prod_name)) &&
+		    (f->cis_vendor == card->cis.vendor ||
+		     f->cis_vendor == (u16) SDIO_ANY_ID) &&
+		    (f->cis_device == card->cis.device ||
+		     f->cis_device == (u16) SDIO_ANY_ID) &&
+		    rev >= f->rev_start && rev <= f->rev_end) {
 			dev_dbg(&card->dev, "calling %pF\n", f->vendor_fixup);
 			f->vendor_fixup(card, f->data);
 		}
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index a5840c0..1e60959 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -472,7 +472,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 
 		card = oldcard;
 	}
-	mmc_fixup_device(card);
+	mmc_fixup_device(card, NULL);
 
 	if (card->type == MMC_TYPE_SD_COMBO) {
 		err = mmc_sd_setup_card(host, card, oldcard != NULL);
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 2a7e549..c651317 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -11,6 +11,7 @@
 #define LINUX_MMC_CARD_H
 
 #include <linux/mmc/core.h>
+#include <linux/mod_devicetable.h>
 
 struct mmc_cid {
 	unsigned int		manfid;
@@ -157,7 +158,92 @@ struct mmc_card {
 	struct dentry		*debugfs_root;
 };
 
-void mmc_fixup_device(struct mmc_card *dev);
+/*
+ *  The world is not perfect and supplies us with broken mmc/sdio devices.
+ *  For at least some of these bugs we need a work-around.
+ */
+
+struct mmc_fixup {
+	/* CID-specific fields. */
+	const char *name;
+
+	/* Valid revision range */
+	u64 rev_start, rev_end;
+
+	unsigned int manfid;
+	unsigned short oemid;
+
+	/* SDIO-specfic fields. You can use SDIO_ANY_ID here of course */
+	u16 cis_vendor, cis_device;
+
+	void (*vendor_fixup)(struct mmc_card *card, int data);
+	int data;
+};
+
+#define CID_MANFID_ANY (-1ul)
+#define CID_OEMID_ANY ((unsigned short) -1)
+#define CID_NAME_ANY (NULL)
+
+#define END_FIXUP { 0 }
+
+#define _FIXUP_EXT(_name, _manfid, _oemid, _rev_start, _rev_end,	\
+		   _cis_vendor, _cis_device,				\
+		   _fixup, _data)					\
+	{						   \
+		.name = (_name),			   \
+		.manfid = (_manfid),			   \
+		.oemid = (_oemid),			   \
+		.rev_start = (_rev_start),		   \
+		.rev_end = (_rev_end),			   \
+		.cis_vendor = (_cis_vendor),		   \
+		.cis_device = (_cis_device),		   \
+		.vendor_fixup = (_fixup),		   \
+		.data = (_data),			   \
+	 }
+
+#define MMC_FIXUP_REV(_name, _manfid, _oemid, _rev_start, _rev_end,	\
+		      _fixup, _data)					\
+	_FIXUP_EXT(_name, _manfid,					\
+		   _oemid, _rev_start, _rev_end,			\
+		   SDIO_ANY_ID, SDIO_ANY_ID,				\
+		   _fixup, _data)					\
+
+#define MMC_FIXUP(_name, _manfid, _oemid, _fixup, _data) \
+	MMC_FIXUP_REV(_name, _manfid, _oemid, 0, -1ull, _fixup, _data)
+
+#define SDIO_FIXUP(_vendor, _device, _fixup, _data)			\
+	_FIXUP_EXT(CID_NAME_ANY, CID_MANFID_ANY,			\
+		    CID_OEMID_ANY, 0, -1ull,				\
+		   _vendor, _device,					\
+		   _fixup, _data)					\
+
+#define cid_rev(hwrev, fwrev, year, month)	\
+	(((u64) hwrev) << 40 |                  \
+	 ((u64) fwrev) << 32 |                  \
+	 ((u64) year) << 16 |                   \
+	 ((u64) month))
+
+#define cid_rev_card(card)		  \
+	cid_rev(card->cid.hwrev,	  \
+		    card->cid.fwrev,      \
+		    card->cid.year,	  \
+		    card->cid.month)
+
+/*
+ * This hook just adds a quirk unconditionally.
+ */
+static inline void __maybe_unused add_quirk(struct mmc_card *card, int data)
+{
+	card->quirks |= data;
+}
+
+/*
+ * This hook just removes a quirk unconditionally.
+ */
+static inline void __maybe_unused remove_quirk(struct mmc_card *card, int data)
+{
+	card->quirks &= ~data;
+}
 
 #define mmc_card_mmc(c)		((c)->type == MMC_TYPE_MMC)
 #define mmc_card_sd(c)		((c)->type == MMC_TYPE_SD)
@@ -218,4 +304,7 @@ struct mmc_driver {
 extern int mmc_register_driver(struct mmc_driver *);
 extern void mmc_unregister_driver(struct mmc_driver *);
 
+extern void mmc_fixup_device(struct mmc_card *card,
+			     const struct mmc_fixup *table);
+
 #endif


Thanks,

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

Re: [PATCH 1/2] MMC: Extends card quicks with MMC/SD quirks matching the CID.

[Andrei Warkentin]

On Mon, Apr 11, 2011 at 3:55 PM, Chris Ball <cjb@laptop.org> wrote:
> Hi Andrei,
>
> I've rebased this patch to include Ohad's recent wl1271 quirks, and was
> about to apply it, but have one question -- would you mind resubmitting
> with your use of strcmp() switched over to strncmp()?  It looks safe in
> this case, but the less code that has to be audited for safety the
> better.
>

Ok. Just did. Thanks for looking at this. I have a SanDisk TRIM bug
workaround using
this that  I was about to send, I guess I'll send it now.

A

[PATCH] MMC: Extends card quicks with MMC/SD quirks matching the CID.

[Andrei Warkentin]

From: Chris Ball <cjb@laptop.org>

The current mechanism is SDIO-only. This allows us to create
function-specific quirks, without creating messy Kconfig dependencies,
or polluting core/ with function-specific code.

Signed-off-by: Andrei Warkentin <andreiw@motorola.com>
---
 drivers/mmc/core/core.h   |    2 -
 drivers/mmc/core/quirks.c |   93 ++++++++++++++++++++------------------------
 drivers/mmc/core/sdio.c   |    2 +-
 include/linux/mmc/card.h  |   91 +++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 133 insertions(+), 55 deletions(-)

diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index 20b1c08..ca1fdde 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -61,8 +61,6 @@ 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;
 
diff --git a/drivers/mmc/core/quirks.c b/drivers/mmc/core/quirks.c
index a4c42ed..3a59621 100644
--- a/drivers/mmc/core/quirks.c
+++ b/drivers/mmc/core/quirks.c
@@ -1,7 +1,8 @@
 /*
- *  This file contains work-arounds for many known sdio hardware
- *  bugs.
+ *  This file contains work-arounds for many known SD/MMC
+ *  and SDIO hardware bugs.
  *
+ *  Copyright (c) 2011 Andrei Warkentin <andreiw@motorola.com>
  *  Copyright (c) 2011 Pierre Tardy <tardyp@gmail.com>
  *  Inspired from pci fixup code:
  *  Copyright (c) 1999 Martin Mares <mj@ucw.cz>
@@ -11,34 +12,14 @@
 #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 (*vendor_fixup)(struct mmc_card *card, int data);
-	int data;
-};
-
-/*
- * This hook just adds a quirk unconditionnally
- */
-static void __maybe_unused add_quirk(struct mmc_card *card, int data)
-{
-	card->quirks |= data;
-}
+#ifndef SDIO_VENDOR_ID_TI
+#define SDIO_VENDOR_ID_TI		0x0097
+#endif
 
-/*
- * This hook just removes a quirk unconditionnally
- */
-static void __maybe_unused remove_quirk(struct mmc_card *card, int data)
-{
-	card->quirks &= ~data;
-}
+#ifndef SDIO_DEVICE_ID_TI_WL1271
+#define SDIO_DEVICE_ID_TI_WL1271	0x4076
+#endif
 
 /*
  * This hook just adds a quirk for all sdio devices
@@ -49,37 +30,47 @@ 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[] = {
 	/* by default sdio devices are considered CLK_GATING broken */
 	/* good cards will be whitelisted as they are tested */
-	{ 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, MMC_QUIRK_BROKEN_CLK_GATING },
-	{ SDIO_VENDOR_ID_TI, SDIO_DEVICE_ID_TI_WL1271,
-		add_quirk, MMC_QUIRK_NONSTD_FUNC_IF },
-	{ SDIO_VENDOR_ID_TI, SDIO_DEVICE_ID_TI_WL1271,
-		add_quirk, MMC_QUIRK_DISABLE_CD },
-	{ 0 }
+	SDIO_FIXUP(SDIO_ANY_ID, SDIO_ANY_ID,
+		   add_quirk_for_sdio_devices,
+		   MMC_QUIRK_BROKEN_CLK_GATING),
+
+	SDIO_FIXUP(SDIO_VENDOR_ID_TI, SDIO_DEVICE_ID_TI_WL1271,
+		   remove_quirk, MMC_QUIRK_BROKEN_CLK_GATING),
+
+	SDIO_FIXUP(SDIO_VENDOR_ID_TI, SDIO_DEVICE_ID_TI_WL1271,
+		   add_quirk, MMC_QUIRK_NONSTD_FUNC_IF),
+
+	SDIO_FIXUP(SDIO_VENDOR_ID_TI, SDIO_DEVICE_ID_TI_WL1271,
+		   add_quirk, MMC_QUIRK_DISABLE_CD),
+
+	END_FIXUP
 };
 
-void mmc_fixup_device(struct mmc_card *card)
+void mmc_fixup_device(struct mmc_card *card, const struct mmc_fixup *table)
 {
 	const struct mmc_fixup *f;
+	u64 rev = cid_rev_card(card);
+
+	/* Non-core specific workarounds. */
+	if (!table)
+		table = mmc_fixup_methods;
 
-	for (f = mmc_fixup_methods; f->vendor_fixup; f++) {
-		if ((f->vendor == card->cis.vendor
-		     || f->vendor == (u16) SDIO_ANY_ID) &&
-		    (f->device == card->cis.device
-		     || f->device == (u16) SDIO_ANY_ID)) {
+	for (f = table; f->vendor_fixup; f++) {
+		if ((f->manfid == CID_MANFID_ANY ||
+		     f->manfid == card->cid.manfid) &&
+		    (f->oemid == CID_OEMID_ANY ||
+		     f->oemid == card->cid.oemid) &&
+		    (f->name == CID_NAME_ANY ||
+		     !strncmp(f->name, card->cid.prod_name,
+			      sizeof(card->cid.prod_name))) &&
+		    (f->cis_vendor == card->cis.vendor ||
+		     f->cis_vendor == (u16) SDIO_ANY_ID) &&
+		    (f->cis_device == card->cis.device ||
+		     f->cis_device == (u16) SDIO_ANY_ID) &&
+		    rev >= f->rev_start && rev <= f->rev_end) {
 			dev_dbg(&card->dev, "calling %pF\n", f->vendor_fixup);
 			f->vendor_fixup(card, f->data);
 		}
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index a5840c0..1e60959 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -472,7 +472,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 
 		card = oldcard;
 	}
-	mmc_fixup_device(card);
+	mmc_fixup_device(card, NULL);
 
 	if (card->type == MMC_TYPE_SD_COMBO) {
 		err = mmc_sd_setup_card(host, card, oldcard != NULL);
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 2a7e549..c651317 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -11,6 +11,7 @@
 #define LINUX_MMC_CARD_H
 
 #include <linux/mmc/core.h>
+#include <linux/mod_devicetable.h>
 
 struct mmc_cid {
 	unsigned int		manfid;
@@ -157,7 +158,92 @@ struct mmc_card {
 	struct dentry		*debugfs_root;
 };
 
-void mmc_fixup_device(struct mmc_card *dev);
+/*
+ *  The world is not perfect and supplies us with broken mmc/sdio devices.
+ *  For at least some of these bugs we need a work-around.
+ */
+
+struct mmc_fixup {
+	/* CID-specific fields. */
+	const char *name;
+
+	/* Valid revision range */
+	u64 rev_start, rev_end;
+
+	unsigned int manfid;
+	unsigned short oemid;
+
+	/* SDIO-specfic fields. You can use SDIO_ANY_ID here of course */
+	u16 cis_vendor, cis_device;
+
+	void (*vendor_fixup)(struct mmc_card *card, int data);
+	int data;
+};
+
+#define CID_MANFID_ANY (-1ul)
+#define CID_OEMID_ANY ((unsigned short) -1)
+#define CID_NAME_ANY (NULL)
+
+#define END_FIXUP { 0 }
+
+#define _FIXUP_EXT(_name, _manfid, _oemid, _rev_start, _rev_end,	\
+		   _cis_vendor, _cis_device,				\
+		   _fixup, _data)					\
+	{						   \
+		.name = (_name),			   \
+		.manfid = (_manfid),			   \
+		.oemid = (_oemid),			   \
+		.rev_start = (_rev_start),		   \
+		.rev_end = (_rev_end),			   \
+		.cis_vendor = (_cis_vendor),		   \
+		.cis_device = (_cis_device),		   \
+		.vendor_fixup = (_fixup),		   \
+		.data = (_data),			   \
+	 }
+
+#define MMC_FIXUP_REV(_name, _manfid, _oemid, _rev_start, _rev_end,	\
+		      _fixup, _data)					\
+	_FIXUP_EXT(_name, _manfid,					\
+		   _oemid, _rev_start, _rev_end,			\
+		   SDIO_ANY_ID, SDIO_ANY_ID,				\
+		   _fixup, _data)					\
+
+#define MMC_FIXUP(_name, _manfid, _oemid, _fixup, _data) \
+	MMC_FIXUP_REV(_name, _manfid, _oemid, 0, -1ull, _fixup, _data)
+
+#define SDIO_FIXUP(_vendor, _device, _fixup, _data)			\
+	_FIXUP_EXT(CID_NAME_ANY, CID_MANFID_ANY,			\
+		    CID_OEMID_ANY, 0, -1ull,				\
+		   _vendor, _device,					\
+		   _fixup, _data)					\
+
+#define cid_rev(hwrev, fwrev, year, month)	\
+	(((u64) hwrev) << 40 |                  \
+	 ((u64) fwrev) << 32 |                  \
+	 ((u64) year) << 16 |                   \
+	 ((u64) month))
+
+#define cid_rev_card(card)		  \
+	cid_rev(card->cid.hwrev,	  \
+		    card->cid.fwrev,      \
+		    card->cid.year,	  \
+		    card->cid.month)
+
+/*
+ * This hook just adds a quirk unconditionally.
+ */
+static inline void __maybe_unused add_quirk(struct mmc_card *card, int data)
+{
+	card->quirks |= data;
+}
+
+/*
+ * This hook just removes a quirk unconditionally.
+ */
+static inline void __maybe_unused remove_quirk(struct mmc_card *card, int data)
+{
+	card->quirks &= ~data;
+}
 
 #define mmc_card_mmc(c)		((c)->type == MMC_TYPE_MMC)
 #define mmc_card_sd(c)		((c)->type == MMC_TYPE_SD)
@@ -218,4 +304,7 @@ struct mmc_driver {
 extern int mmc_register_driver(struct mmc_driver *);
 extern void mmc_unregister_driver(struct mmc_driver *);
 
+extern void mmc_fixup_device(struct mmc_card *card,
+			     const struct mmc_fixup *table);
+
 #endif
-- 
1.7.0.4

Re: [PATCH] MMC: Extends card quicks with MMC/SD quirks matching the CID.

[Chris Ball]

Hi,

On Mon, Apr 11 2011, Andrei Warkentin wrote:
> From: Chris Ball <cjb@laptop.org>
>
> The current mechanism is SDIO-only. This allows us to create
> function-specific quirks, without creating messy Kconfig dependencies,
> or polluting core/ with function-specific code.
>
> Signed-off-by: Andrei Warkentin <andreiw@motorola.com>

Thanks, pushed to mmc-next for .40 with Arnd's ACK.  (And with you as
patch author, not me.)

I made a mistake in not noticing that your later RFC patchset includes
(the same version of) this patch -- are you happy with adding block
quirks on top of this later, and merging this patch for now?

(Ohad, heads-up that I've rebased your wl1271 SDIO quirks onto this
patchset; there should be no behavior change, but just in case.)

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

Re: [PATCH] MMC: Extends card quicks with MMC/SD quirks matching the CID.

[Andrei Warkentin]

On Mon, Apr 11, 2011 at 4:50 PM, Chris Ball <cjb@laptop.org> wrote:
> Hi,
>
> On Mon, Apr 11 2011, Andrei Warkentin wrote:
>> From: Chris Ball <cjb@laptop.org>
>>
>> The current mechanism is SDIO-only. This allows us to create
>> function-specific quirks, without creating messy Kconfig dependencies,
>> or polluting core/ with function-specific code.
>>
>> Signed-off-by: Andrei Warkentin <andreiw@motorola.com>
>
> Thanks, pushed to mmc-next for .40 with Arnd's ACK.  (And with you as
> patch author, not me.)
>
> I made a mistake in not noticing that your later RFC patchset includes
> (the same version of) this patch -- are you happy with adding block
> quirks on top of this later, and merging this patch for now?
>

Thanks! Yes, no problem, I've just sent a block quirk which uses this
(SanDisk), and that includes a re-send of the block quirks support.

A

[PATCH 2/2] MMC: Support for block quirks.

[Andrei Warkentin]

Block quirks implemented using core/quirks.c support.

Signed-off-by: Andrei Warkentin <andreiw@motorola.com>
---
 drivers/mmc/card/block.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 7054fd5..913f394 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -686,6 +686,11 @@ mmc_blk_set_blksize(struct mmc_blk_data *md, struct mmc_card *card)
 	return 0;
 }
 
+static const struct mmc_fixup blk_fixups[] =
+{
+	END_FIXUP
+};
+
 static int mmc_blk_probe(struct mmc_card *card)
 {
 	struct mmc_blk_data *md;
@@ -714,6 +719,8 @@ static int mmc_blk_probe(struct mmc_card *card)
 		cap_str, md->read_only ? "(ro)" : "");
 
 	mmc_set_drvdata(card, md);
+	mmc_fixup_device(card, blk_fixups);
+
 #ifdef CONFIG_MMC_BLOCK_DEFERRED_RESUME
 	mmc_set_bus_resume_policy(card->host, 1);
 #endif
-- 
1.7.0.4