Skip to contentSkip to conversation

mmci: use sg_miter API to fix multi-page sg handling

PATCH Superseded Rabin Vincent 5 participants 37 messages Jun 22, 2010 – Oct 19, 2010

Subsystems (5)
Arm (32-bit) Arm sub-architectures MMC, SD and SDIO Arm/versatile express platform +1 more subsystems
lore
Share & tools
lore

Rabin Vincent

author
#0 lore raw

PATCH 1/12 mmci: use sg_miter API to fix multi-page sg handling

The mmci driver's SG list iteration logic assumes that each SG entry spans only one page, and only maps and flushes one page of the sg. This is not a valid assumption. Fix it by converting the driver to the sg_miter API, which correctly handles sgs which span multiple pages. Cache flushing is handled inside the sg_miter API implementation.

Acked-by Linus Walleij <[email protected]>
Signed-off-by Rabin Vincent <[email protected]>
---
 drivers/mmc/host/mmci.c |   55 ++++++++++++++++++++++------------------------
 drivers/mmc/host/mmci.h |   35 +-----------------------------
 2 files changed, 27 insertions(+), 63 deletions(-)
drivers/mmc/host/mmci.c +26 -29
index 4917af9..683516b 100644
@@ -26,7 +26,6 @@
 #include <linux/amba/mmci.h>
 #include <linux/regulator/consumer.h>
 
-#include <asm/cacheflush.h>
 #include <asm/div64.h>
 #include <asm/io.h>
 #include <asm/sizes.h>

@@ -98,6 +97,18 @@ static void mmci_stop_data(struct mmci_host *host)
 	host->data = NULL;
 }
 
+static void mmci_init_sg(struct mmci_host *host, struct mmc_data *data)
+{
+	unsigned int flags = SG_MITER_ATOMIC;
+
+	if (data->flags & MMC_DATA_READ)
+		flags |= SG_MITER_TO_SG;
+	else
+		flags |= SG_MITER_FROM_SG;
+
+	sg_miter_start(&host->sg_miter, data->sg, data->sg_len, flags);
+}
+
 static void mmci_start_data(struct mmci_host *host, struct mmc_data *data)
 {
 	unsigned int datactrl, timeout, irqmask;

@@ -205,13 +216,6 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
 		else if (status & (MCI_TXUNDERRUN|MCI_RXOVERRUN))
 			data->error = -EIO;
 		status |= MCI_DATAEND;
-
-		/*
-		 * We hit an error condition.  Ensure that any data
-		 * partially written to a page is properly coherent.
-		 */
-		if (host->sg_len && data->flags & MMC_DATA_READ)
-			flush_dcache_page(sg_page(host->sg_ptr));
 	}
 	if (status & MCI_DATAEND) {
 		mmci_stop_data(host);

@@ -314,15 +318,18 @@ static int mmci_pio_write(struct mmci_host *host, char *buffer, unsigned int rem
 static irqreturn_t mmci_pio_irq(int irq, void *dev_id)
 {
 	struct mmci_host *host = dev_id;
+	struct sg_mapping_iter *sg_miter = &host->sg_miter;
 	void __iomem *base = host->base;
+	unsigned long flags;
 	u32 status;
 
 	status = readl(base + MMCISTATUS);
 
 	dev_dbg(mmc_dev(host->mmc), "irq1 (pio) %08x\n", status);
 
+	local_irq_save(flags);
+
 	do {
-		unsigned long flags;
 		unsigned int remain, len;
 		char *buffer;
 

@@ -336,11 +343,11 @@ static irqreturn_t mmci_pio_irq(int irq, void *dev_id)
 		if (!(status & (MCI_TXFIFOHALFEMPTY|MCI_RXDATAAVLBL)))
 			break;
 
-		/*
-		 * Map the current scatter buffer.
-		 */
-		buffer = mmci_kmap_atomic(host, &flags) + host->sg_off;
-		remain = host->sg_ptr->length - host->sg_off;
+		if (!sg_miter_next(sg_miter))
+			break;
+
+		buffer = sg_miter->addr;
+		remain = sg_miter->length;
 
 		len = 0;
 		if (status & MCI_RXACTIVE)

@@ -348,31 +355,21 @@ static irqreturn_t mmci_pio_irq(int irq, void *dev_id)
 		if (status & MCI_TXACTIVE)
 			len = mmci_pio_write(host, buffer, remain, status);
 
-		/*
-		 * Unmap the buffer.
-		 */
-		mmci_kunmap_atomic(host, buffer, &flags);
+		sg_miter->consumed = len;
 
-		host->sg_off += len;
 		host->size -= len;
 		remain -= len;
 
 		if (remain)
 			break;
 
-		/*
-		 * If we were reading, and we have completed this
-		 * page, ensure that the data cache is coherent.
-		 */
-		if (status & MCI_RXACTIVE)
-			flush_dcache_page(sg_page(host->sg_ptr));
-
-		if (!mmci_next_sg(host))
-			break;
-
 		status = readl(base + MMCISTATUS);
 	} while (1);
 
+	sg_miter_stop(sg_miter);
+
+	local_irq_restore(flags);
+
 	/*
 	 * If we're nearing the end of the read, switch to
 	 * "any data available" mode.
drivers/mmc/host/mmci.h +1 -34
index d77062e..7cb24ab 100644
@@ -171,42 +171,9 @@ struct mmci_host {
 	struct timer_list	timer;
 	unsigned int		oldstat;
 
-	unsigned int		sg_len;
-
 	/* pio stuff */
-	struct scatterlist	*sg_ptr;
-	unsigned int		sg_off;
+	struct sg_mapping_iter	sg_miter;
 	unsigned int		size;
 	struct regulator	*vcc;
 };
 
-static inline void mmci_init_sg(struct mmci_host *host, struct mmc_data *data)
-{
-	/*
-	 * Ideally, we want the higher levels to pass us a scatter list.
-	 */
-	host->sg_len = data->sg_len;
-	host->sg_ptr = data->sg;
-	host->sg_off = 0;
-}
-
-static inline int mmci_next_sg(struct mmci_host *host)
-{
-	host->sg_ptr++;
-	host->sg_off = 0;
-	return --host->sg_len;
-}
-
-static inline char *mmci_kmap_atomic(struct mmci_host *host, unsigned long *flags)
-{
-	struct scatterlist *sg = host->sg_ptr;
-
-	local_irq_save(*flags);
-	return kmap_atomic(sg_page(sg), KM_BIO_SRC_IRQ) + sg->offset;
-}
-
-static inline void mmci_kunmap_atomic(struct mmci_host *host, void *buffer, unsigned long *flags)
-{
-	kunmap_atomic(buffer, KM_BIO_SRC_IRQ);
-	local_irq_restore(*flags);
-}
Signature · 1 line
1.7.0
author
#1 lore raw

PATCH 2/12 mmci: fix multi block transfers

Fix the data transfer size to allow multi block transfers to work.

Acked-by Linus Walleij <[email protected]>
Signed-off-by Rabin Vincent <[email protected]>
---
 drivers/mmc/host/mmci.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/mmc/host/mmci.c +1 -1
index 683516b..84d782b 100644
@@ -120,7 +120,7 @@ static void mmci_start_data(struct mmci_host *host, struct mmc_data *data)
 		data->blksz, data->blocks, data->flags);
 
 	host->data = data;
-	host->size = data->blksz;
+	host->size = data->blksz * data->blocks;
 	host->data_xfered = 0;
 
 	mmci_init_sg(host, data);
Signature · 1 line
1.7.0
#2 lore raw

PATCH 2/12 mmci: fix multi block transfers

Quoted 4 lines · Rabin Vincent

Fix the data transfer size to allow multi block transfers to work.

Acked-by Linus Walleij <[email protected]>
Signed-off-by Rabin Vincent <[email protected]>

The first two patches improve performance on Versatile, from 20.5s to read 1MB down to 18.5s - a 10% improvement.

I was worried that the first patch would add too much overhead, and cause the hardware to overrun - as the ARM version of these primecells does not have the capability to stop the clock when the FIFO fills up, any increase in IRQ latency to read the FIFO will result in an overrun.

So this is good news. I've applied the first three patches in this series, but we do need to sort out the card detect situation properly before continuing.

#3 lore raw

PATCH 2/12 mmci: fix multi block transfers

Quoted 7 lines

-----Original Message-----
From: linux-arm-kernel-bounces at lists.infradead.org [mailto:linux-arm- kernel-bounces at lists.infradead.org] On Behalf Of Russell King - ARM

So this is good news. I've applied the first three patches in this series, but we do need to sort out the card detect situation properly before continuing.

On the subject of card detection. I recently noticed that although it works on the PB926 it doesn't on the AB926 despite it being the same kernel binary. On the AB926 the detection is doing something when a card is inserted but the device never appears.

Colin

--
Colin Tuckley - ARM Ltd.
110 Fulbourn Rd
Cambridge, CB1 9NJ
Tel: +44 1223 400536

#4 lore raw

PATCH 2/12 mmci: fix multi block transfers

Quoted 11 lines · Colin Tuckley
Quoted 7 lines

-----Original Message-----
From: linux-arm-kernel-bounces at lists.infradead.org [mailto:linux-arm- kernel-bounces at lists.infradead.org] On Behalf Of Russell King - ARM

So this is good news. I've applied the first three patches in this series, but we do need to sort out the card detect situation properly before continuing.

On the subject of card detection. I recently noticed that although it works on the PB926 it doesn't on the AB926 despite it being the same kernel binary.

No, it doesn't work on Versatile PB926 in mainline (I tried it this morning) because the card detection (via the ->status callback) is inverted - just like it was for realview. What happens is you get commands issued to the card on card removal, and nothing on card insertion.

What I'm proposing is that we get rid of these multiple levels of negation that we're gaining. The negations only add additional complexity and confusion over what's the right thing to return at any given point. Lets stick to one positive logic method throughout the code - 0 means no card, !0 means card inserted.

That means your original patch needs to be reverted, and we need to change the 'return !status;' to just 'return status;' in mmci.c's get_cd function as per my patch.

However, I think this will make GPIO on Realview wrong - iirc, the card detection bits from PL061 are inverted.

#5 lore raw

PATCH 2/12 mmci: fix multi block transfers

Quoted 10 lines

-----Original Message-----
From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]

No, it doesn't work on Versatile PB926 in mainline (I tried it this morning)
because the card detection (via the ->status callback) is inverted - just
like it was for realview. What happens is you get commands issued to the
card on card removal, and nothing on card insertion.

Sorry, I wasn't clear - I was talking about the current ARM tree 2.6.33-arm1 where Catalin and I fixed Realview and Versatile so that it worked with GPIOLIB where the detection was done with GPIO and to revert to the old method on boards where the detect pins are not available as gpio.

Quoted 4 lines

What I'm proposing is that we get rid of these multiple levels of negation
that we're gaining. The negations only add additional complexity and confusion over what's the right thing to return at any given point.

Ack.

Quoted 5 lines

That means your original patch needs to be reverted, and we need to change
the 'return !status;' to just 'return status;' in mmci.c's get_cd function
as per my patch.

More massaged than reverted I think, but Ack.

Quoted 2 lines

However, I think this will make GPIO on Realview wrong - iirc, the card detection bits from PL061 are inverted.

See above.

Colin

#6 lore raw

PATCH 2/12 mmci: fix multi block transfers

Quoted 4 lines · Colin Tuckley
Quoted 2 lines

However, I think this will make GPIO on Realview wrong - iirc, the card detection bits from PL061 are inverted.

See above.

I've no idea what the right thing on Realview is because the documentation is too poor in this area. Although the Realview EB user manual gives you the GPIO block signals, it doesn't say what sense they are.

It looks to me from the hardware description diagrams, that the GPIO block is fed with an inverted signal from the socket card detect pin - which itself is active low. That means the GPIO signal is in a positive logic state.

So I don't think that the "return !status;" is right even for the GPIO case.

#7 lore raw

PATCH 2/12 mmci: fix multi block transfers

Quoted 10 lines · Russell King - ARM Linux
Quoted 4 lines · Colin Tuckley
Quoted 2 lines

However, I think this will make GPIO on Realview wrong - iirc, the card detection bits from PL061 are inverted.

See above.

I've no idea what the right thing on Realview is because the documentation is too poor in this area. Although the Realview EB user manual gives you the GPIO block signals, it doesn't say what sense they are.

Quoted 7 lines

It looks to me from the hardware description diagrams, that the GPIO block is fed with an inverted signal from the socket card detect pin - which itself is active low. That means the GPIO signal is in a positive logic state.

So I don't think that the "return !status;" is right even for the GPIO case.

Hmm. Firstly, Versatile doesn't support reading the card detect via GPIO.

Secondly, my Realview hardware appears to have the GPIO card detect status inverted - zero means card inserted, non-zero means card removed. That's contary to what the documentation suggests.

Confused.

author
#8 lore raw

PATCH 3/12 mmci: let core poll for card detection

In reply to Rabin Vincent

Use the MMC core's ability to poll for card detection. This also has the advantage of doing the gpio_get_value from a workqueue instead of timer, allowing the gpio to be on a sleeping gpiochip.

Acked-by Linus Walleij <[email protected]>
Signed-off-by Rabin Vincent <[email protected]>
---
 drivers/mmc/host/mmci.c |   22 +---------------------
 1 files changed, 1 insertions(+), 21 deletions(-)
drivers/mmc/host/mmci.c +1 -21
index 84d782b..65f0e12 100644
@@ -548,18 +548,6 @@ static const struct mmc_host_ops mmci_ops = {
 	.get_cd		= mmci_get_cd,
 };
 
-static void mmci_check_status(unsigned long data)
-{
-	struct mmci_host *host = (struct mmci_host *)data;
-	unsigned int status = mmci_get_cd(host->mmc);
-
-	if (status ^ host->oldstat)
-		mmc_detect_change(host->mmc, 0);
-
-	host->oldstat = status;
-	mod_timer(&host->timer, jiffies + HZ);
-}
-
 static int __devinit mmci_probe(struct amba_device *dev, struct amba_id *id)
 {
 	struct mmci_platform_data *plat = dev->dev.platform_data;

@@ -666,6 +654,7 @@ static int __devinit mmci_probe(struct amba_device *dev, struct amba_id *id)
 	if (host->vcc == NULL)
 		mmc->ocr_avail = plat->ocr_mask;
 	mmc->caps = plat->capabilities;
+	mmc->caps |= MMC_CAP_NEEDS_POLL;
 
 	/*
 	 * We can do SGIO

@@ -731,7 +720,6 @@ static int __devinit mmci_probe(struct amba_device *dev, struct amba_id *id)
 	writel(MCI_IRQENABLE, host->base + MMCIMASK0);
 
 	amba_set_drvdata(dev, mmc);
-	host->oldstat = mmci_get_cd(host->mmc);
 
 	mmc_add_host(mmc);
 

@@ -739,12 +727,6 @@ static int __devinit mmci_probe(struct amba_device *dev, struct amba_id *id)
 		mmc_hostname(mmc), amba_rev(dev), amba_config(dev),
 		(unsigned long long)dev->res.start, dev->irq[0], dev->irq[1]);
 
-	init_timer(&host->timer);
-	host->timer.data = (unsigned long)host;
-	host->timer.function = mmci_check_status;
-	host->timer.expires = jiffies + HZ;
-	add_timer(&host->timer);
-
 	return 0;
 
  irq0_free:

@@ -778,8 +760,6 @@ static int __devexit mmci_remove(struct amba_device *dev)
 	if (mmc) {
 		struct mmci_host *host = mmc_priv(mmc);
 
-		del_timer_sync(&host->timer);
-
 		mmc_remove_host(mmc);
 
 		writel(0, host->base + MMCIMASK0);
Signature · 1 line
1.7.0
author
#9 lore raw

PATCH 4/12 mmci: allow the card detect status not to be inverted

In reply to Rabin Vincent

On some platforms, the GPIO value from the gpio_cd pin doesn't need to be inverted to get it active high.

Acked-by Linus Walleij <[email protected]>
Signed-off-by Rabin Vincent <[email protected]>
---
 drivers/mmc/host/mmci.c   |    2 +-
 include/linux/amba/mmci.h |    2 ++
 2 files changed, 3 insertions(+), 1 deletions(-)
drivers/mmc/host/mmci.c +1 -1
index 65f0e12..7e56726 100644
@@ -538,7 +538,7 @@ static int mmci_get_cd(struct mmc_host *mmc)
 	else
 		status = gpio_get_value(host->gpio_cd);
 
-	return !status;
+	return !status ^ host->plat->cd_noinvert;
 }
 
 static const struct mmc_host_ops mmci_ops = {
include/linux/amba/mmci.h +2 -0
index 7e466fe..f9d1bb5 100644
@@ -23,6 +23,7 @@
  * whether a card is present in the MMC slot or not
  * @gpio_wp: read this GPIO pin to see if the card is write protected
  * @gpio_cd: read this GPIO pin to detect card insertion
+ * @cd_noinvert: true if the gpio_cd pin value or status are active high
  * @capabilities: the capabilities of the block as implemented in
  * this platform, signify anything MMC_CAP_* from mmc/host.h
  */

@@ -33,6 +34,7 @@ struct mmci_platform_data {
 	unsigned int (*status)(struct device *);
 	int	gpio_wp;
 	int	gpio_cd;
+	bool	cd_noinvert;
 	unsigned long capabilities;
 };
Signature · 1 line
1.7.0
#10 lore raw

PATCH 4/12 mmci: allow the card detect status not to be inverted

Quoted 2 lines · Rabin Vincent

On some platforms, the GPIO value from the gpio_cd pin doesn't need to be inverted to get it active high.

Argh.

Actually, I think the inversion of the card status is a bug - certainly for Versatile it is. It was on Realview as well before b56ba8a changed the sense for Realview - so it seems b56ba8a was the wrong fix.

So, this patch restores the card detect functionality across the ARM devel platforms to what it was prior to the addition of GPIOLIB support.

What now needs to happen is the sense of the GPIOLIB support on platforms needs to be checked - eg, I suspect the card detect via the GPIO primecell is inverted on Realview - in which case we need a 'cd_invert' rather than the negative 'cd_noinvert'.

arch/arm/mach-realview/core.c +2 -2
index d8179ea..f9dc99c 100644
@@ -243,7 +243,7 @@ static unsigned int realview_mmc_status(struct device *dev)
 		 * way to do this on the PB1176.
 		 */
 		inserted = !inserted;
-		return inserted ? 0 : 1;
+		return inserted ? 1 : 0;
 	}
 
 	if (adev->res.start == REALVIEW_MMCI0_BASE)

@@ -251,7 +251,7 @@ static unsigned int realview_mmc_status(struct device *dev)
 	else
 		mask = 2;
 
-	return !(readl(REALVIEW_SYSMCI) & mask);
+	return readl(REALVIEW_SYSMCI) & mask;
 }
 
 struct mmci_platform_data realview_mmc0_plat_data = {
drivers/mmc/host/mmci.c +1 -1
index 9bf9194..02bf365 100644
@@ -557,7 +557,7 @@ static int mmci_get_cd(struct mmc_host *mmc)
 	else
 		status = gpio_get_value(host->gpio_cd);
 
-	return !status;
+	return status;
 }
 
 static const struct mmc_host_ops mmci_ops = {
#11 lore raw

PATCH 4/12 mmci: allow the card detect status not to be inverted

2010/7/29 Russell King - ARM Linux <[email protected]>:

Quoted 4 lines

What now needs to happen is the sense of the GPIOLIB support on platforms needs to be checked - eg, I suspect the card detect via the GPIO primecell is inverted on Realview - in which case we need a 'cd_invert' rather than the negative 'cd_noinvert'.

Doesn't seem so. I applied this patch and it works like a charm just like before on PB1176. (PL061 is compiled in, so it's not that other weird stuff being used.)

I hope I'm doing the right thing, boot with card inserted, this appears in the bootlog:

mmc0: new SD card at address 80ca
mmcblk0: mmc0:80ca SU512 483 MiB
mmcblk0: p1

Remove card:
mmc0: card 80ca removed

Insert card:
mmc0: new SD card at address 80ca
mmcblk0: mmc0:80ca SU512 483 MiB
mmcblk0: p1

And so on.

Yours,
Linus Walleij

#12 lore raw

PATCH 4/12 mmci: allow the card detect status not to be inverted

Quoted 10 lines · Linus Walleij

2010/7/29 Russell King - ARM Linux <[email protected]>:

Quoted 4 lines

What now needs to happen is the sense of the GPIOLIB support on platforms needs to be checked - eg, I suspect the card detect via the GPIO primecell is inverted on Realview - in which case we need a 'cd_invert' rather than the negative 'cd_noinvert'.

Doesn't seem so. I applied this patch and it works like a charm just like before on PB1176. (PL061 is compiled in, so it's not that other weird stuff being used.)

Well, as I said in the other thread which seems to be dealing with this, I'm completely confused (as seems to be the case with this) as to what sense the various card detect signals are on ARMs various hardware platforms.

It seems to be a totally undocumented - and I've no idea whether the behaviour I see on my platforms is what other people see (from some suggestions in these threads, my platforms behave differently.)

So, for now I'm going to go with this patch, which fixes it across the board for the non-GPIO cases.

When the right sense for the GPIO case can be _properly_ determined, we can see about fixing that. What I don't want to get into is a "it works this way on my realview revision X platform, but not revision Y." If that's the case, we might as well stick with reading the status from the reliable MCI register which always uses positive logic.

arch/arm/mach-realview/core.c +1 -1
index 595be19..02e9fde 100644
@@ -237,7 +237,7 @@ static unsigned int realview_mmc_status(struct device *dev)
 	else
 		mask = 2;
 
-	return !(readl(REALVIEW_SYSMCI) & mask);
+	return readl(REALVIEW_SYSMCI) & mask;
 }
 
 struct mmci_platform_data realview_mmc0_plat_data = {
arch/arm/mach-vexpress/v2m.c +1 -1
index d250711..c842397 100644
@@ -241,7 +241,7 @@ static struct platform_device v2m_flash_device = {
 
 static unsigned int v2m_mmci_status(struct device *dev)
 {
-	return !(readl(MMIO_P2V(V2M_SYS_MCI)) & (1 << 0));
+	return readl(MMIO_P2V(V2M_SYS_MCI)) & (1 << 0);
 }
 
 static struct mmci_platform_data v2m_mmci_data = {
drivers/mmc/host/mmci.c +5 -1
index 4917af9..8ca38d9 100644
@@ -541,7 +541,11 @@ static int mmci_get_cd(struct mmc_host *mmc)
 	else
 		status = gpio_get_value(host->gpio_cd);
 
-	return !status;
+	/*
+	 * Use positive logic throughout - status is zero for no card,
+	 * non-zero for card inserted.
+	 */
+	return status;
 }
 
 static const struct mmc_host_ops mmci_ops = {
author
#13 lore raw

PATCH 4/12 mmci: allow the card detect status not to be inverted

Quoted 5 lines · Russell King - ARM Linux
drivers/mmc/host/mmci.c +0 -0
index 4917af9..8ca38d9 100644

drivers/mmc/host/mmci.c static int mmci_get_cd(struct mmc_host *mmc) · line 549

Quoted 10 lines
drivers/mmc/host/mmci.c +5 -1
@@ -541,7 +541,11 @@ static int mmci_get_cd(struct mmc_host *mmc)
 	else
 		status = gpio_get_value(host->gpio_cd);
 
-	return !status;
+	/*
+	 * Use positive logic throughout - status is zero for no card,
+	 * non-zero for card inserted.
+	 */
+	return status;
 }

Your patch in -next has a !gpio_get_value, so are you OK with the cd_noinvert addition (for the GPIO case)? Or a cd_invert and a patch to existing platforms?

Rabin

#14 lore raw

PATCH 4/12 mmci: allow the card detect status not to be inverted

Quoted 9 lines · Rabin VINCENT
Quoted 8 lines · Russell King - ARM Linux
drivers/mmc/host/mmci.c +0 -0
index 4917af9..8ca38d9 100644
@@ -541,7 +541,11 @@ static int mmci_get_cd(struct mmc_host *mmc)
 	else
 		status = gpio_get_value(host->gpio_cd);
Quoted 9 lines

drivers/mmc/host/mmci.c static int mmci_get_cd(struct mmc_host *mmc) · line 549

Quoted 7 lines
drivers/mmc/host/mmci.c +5 -1
-	return !status;
+	/*
+	 * Use positive logic throughout - status is zero for no card,
+	 * non-zero for card inserted.
+	 */
+	return status;
 }

Your patch in -next has a !gpio_get_value,

Yes, because I at least need it for it to work on my Realview platform. As no one else seemed interested in replying to my questions about it, I decided that I'd fix it so at least what I had continued to work.

Quoted 2 lines

so are you OK with the cd_noinvert addition (for the GPIO case)? Or a cd_invert and a patch to existing platforms?

I'd much prefer positive logic. Double negatives (eg, not noinverted) are always bad news.

author
#15 lore raw

PATCH 4/12 mmci: allow the card detect status not to be inverted

Quoted 6 lines · Russell King - ARM Linux
Quoted 2 lines · Rabin VINCENT

so are you OK with the cd_noinvert addition (for the GPIO case)? Or a cd_invert and a patch to existing platforms?

I'd much prefer positive logic. Double negatives (eg, not noinverted) are always bad news.

Here's the new patch. I'll rebase the rest of the unapplied patches in the series on top of this.

#16 lore raw

PATCH 4/12 mmci: allow the card detect status not to be inverted

Quoted 10 lines · Rabin VINCENT
Quoted 6 lines · Russell King - ARM Linux
Quoted 2 lines · Rabin VINCENT

so are you OK with the cd_noinvert addition (for the GPIO case)? Or a cd_invert and a patch to existing platforms?

I'd much prefer positive logic. Double negatives (eg, not noinverted) are always bad news.

Here's the new patch. I'll rebase the rest of the unapplied patches in the series on top of this.

This looks much better, thanks.

author
#17 lore raw

PATCH 5/12 mmci: support card detection interrupts

In reply to Rabin Vincent

If an IRQ can be requested on the card detected GPIO, use it instead of polling.

Acked-by Linus Walleij <[email protected]>
Signed-off-by Rabin Vincent <[email protected]>
---
 drivers/mmc/host/mmci.c |   24 +++++++++++++++++++++++-
 drivers/mmc/host/mmci.h |    1 +
 2 files changed, 24 insertions(+), 1 deletions(-)
drivers/mmc/host/mmci.c +23 -1
index 7e56726..e6477e6 100644
@@ -541,6 +541,15 @@ static int mmci_get_cd(struct mmc_host *mmc)
 	return !status ^ host->plat->cd_noinvert;
 }
 
+static irqreturn_t mmci_cd_irq(int irq, void *dev_id)
+{
+	struct mmci_host *host = dev_id;
+
+	mmc_detect_change(host->mmc, msecs_to_jiffies(500));
+
+	return IRQ_HANDLED;
+}
+
 static const struct mmc_host_ops mmci_ops = {
 	.request	= mmci_request,
 	.set_ios	= mmci_set_ios,

@@ -576,6 +585,7 @@ static int __devinit mmci_probe(struct amba_device *dev, struct amba_id *id)
 
 	host->gpio_wp = -ENOSYS;
 	host->gpio_cd = -ENOSYS;
+	host->gpio_cd_irq = -1;
 
 	host->hw_designer = amba_manf(dev);
 	host->hw_revision = amba_rev(dev);

@@ -654,7 +664,6 @@ static int __devinit mmci_probe(struct amba_device *dev, struct amba_id *id)
 	if (host->vcc == NULL)
 		mmc->ocr_avail = plat->ocr_mask;
 	mmc->caps = plat->capabilities;
-	mmc->caps |= MMC_CAP_NEEDS_POLL;
 
 	/*
 	 * We can do SGIO

@@ -698,6 +707,12 @@ static int __devinit mmci_probe(struct amba_device *dev, struct amba_id *id)
 			host->gpio_cd = plat->gpio_cd;
 		else if (ret != -ENOSYS)
 			goto err_gpio_cd;
+
+		ret = request_any_context_irq(gpio_to_irq(plat->gpio_cd),
+					      mmci_cd_irq, 0,
+					      DRIVER_NAME " (cd)", host);
+		if (ret >= 0)
+			host->gpio_cd_irq = gpio_to_irq(plat->gpio_cd);
 	}
 	if (gpio_is_valid(plat->gpio_wp)) {
 		ret = gpio_request(plat->gpio_wp, DRIVER_NAME " (wp)");

@@ -709,6 +724,9 @@ static int __devinit mmci_probe(struct amba_device *dev, struct amba_id *id)
 			goto err_gpio_wp;
 	}
 
+	if (host->gpio_cd_irq < 0)
+		mmc->caps |= MMC_CAP_NEEDS_POLL;
+
 	ret = request_irq(dev->irq[0], mmci_irq, IRQF_SHARED, DRIVER_NAME " (cmd)", host);
 	if (ret)
 		goto unmap;

@@ -735,6 +753,8 @@ static int __devinit mmci_probe(struct amba_device *dev, struct amba_id *id)
 	if (host->gpio_wp != -ENOSYS)
 		gpio_free(host->gpio_wp);
  err_gpio_wp:
+	if (host->gpio_cd_irq >= 0)
+		free_irq(host->gpio_cd_irq, host);
 	if (host->gpio_cd != -ENOSYS)
 		gpio_free(host->gpio_cd);
  err_gpio_cd:

@@ -773,6 +793,8 @@ static int __devexit mmci_remove(struct amba_device *dev)
 
 		if (host->gpio_wp != -ENOSYS)
 			gpio_free(host->gpio_wp);
+		if (host->gpio_cd_irq >= 0)
+			free_irq(host->gpio_cd_irq, host);
 		if (host->gpio_cd != -ENOSYS)
 			gpio_free(host->gpio_cd);
drivers/mmc/host/mmci.h +1 -0
index 7cb24ab..516adc6 100644
@@ -155,6 +155,7 @@ struct mmci_host {
 	struct clk		*clk;
 	int			gpio_cd;
 	int			gpio_wp;
+	int			gpio_cd_irq;
 
 	unsigned int		data_xfered;
Signature · 1 line
1.7.0
author
#18 lore raw

PATCH 6/12 mmci: allow neither ->status nor gpio_cd to be specified

In reply to Rabin Vincent

The card may be always present on the board, and for these cases neither a status callback nor a card detect GPIO is required, and card detection polling can be disabled.

Acked-by Linus Walleij <[email protected]>
Signed-off-by Rabin Vincent <[email protected]>
---
 drivers/mmc/host/mmci.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)
drivers/mmc/host/mmci.c +7 -3
index e6477e6..322958f 100644
@@ -533,9 +533,12 @@ static int mmci_get_cd(struct mmc_host *mmc)
 	struct mmci_host *host = mmc_priv(mmc);
 	unsigned int status;
 
-	if (host->gpio_cd == -ENOSYS)
+	if (host->gpio_cd == -ENOSYS) {
+		if (!host->plat->status)
+			return 1; /* Assume always present */
+
 		status = host->plat->status(mmc_dev(host->mmc));
-	else
+	} else
 		status = gpio_get_value(host->gpio_cd);
 
 	return !status ^ host->plat->cd_noinvert;

@@ -724,7 +727,8 @@ static int __devinit mmci_probe(struct amba_device *dev, struct amba_id *id)
 			goto err_gpio_wp;
 	}
 
-	if (host->gpio_cd_irq < 0)
+	if ((host->plat->status || host->gpio_cd != -ENOSYS)
+	    && host->gpio_cd_irq < 0)
 		mmc->caps |= MMC_CAP_NEEDS_POLL;
 
 	ret = request_irq(dev->irq[0], mmci_irq, IRQF_SHARED, DRIVER_NAME " (cmd)", host);
Signature · 1 line
1.7.0
author
#19 lore raw

PATCH 7/12 mmci: pass power_mode to the translate_vdd callback

In reply to Rabin Vincent

Platforms may have some external power control which need to be controlled from board specific code. This is the case for some versions of the MOP500 board (U8500 platform).

Acked-by Linus Walleij <[email protected]>
Signed-off-by Rabin Vincent <[email protected]>
---
 drivers/mmc/host/mmci.c   |    3 ++-
 include/linux/amba/mmci.h |    6 ++++--
 2 files changed, 6 insertions(+), 3 deletions(-)
drivers/mmc/host/mmci.c +2 -1
index 322958f..63eb7f4 100644
@@ -483,7 +483,8 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 		 * power control behind this translate function.
 		 */
 		if (!host->vcc && host->plat->translate_vdd)
-			pwr |= host->plat->translate_vdd(mmc_dev(mmc), ios->vdd);
+			pwr |= host->plat->translate_vdd(mmc_dev(mmc), ios->vdd,
+							 ios->power_mode);
 		/* The ST version does not have this, fall through to POWER_ON */
 		if (host->hw_designer != AMBA_VENDOR_ST) {
 			pwr |= MCI_PWR_UP;
include/linux/amba/mmci.h +4 -2
index f9d1bb5..61b5810 100644
@@ -17,7 +17,8 @@
  * mmc/host.h
  * @translate_vdd: a callback function to translate a MMC_VDD_*
  * mask into a value to be binary or:ed and written into the
- * MMCIPWR register of the block
+ * MMCIPWR register of the block.  May also control external power
+ * based on the power_mode.
  * @status: if no GPIO read function was given to the block in
  * gpio_wp (below) this function will be called to determine
  * whether a card is present in the MMC slot or not

@@ -30,7 +31,8 @@
 struct mmci_platform_data {
 	unsigned int f_max;
 	unsigned int ocr_mask;
-	u32 (*translate_vdd)(struct device *, unsigned int);
+	u32 (*translate_vdd)(struct device *, unsigned int vdd,
+			     unsigned char power_mode);
 	unsigned int (*status)(struct device *);
 	int	gpio_wp;
 	int	gpio_cd;
Signature · 1 line
1.7.0
#20 lore raw

PATCH 7/12 mmci: pass power_mode to the translate_vdd callback

Thinking a bit about this patch tonight:

2010/6/22 Rabin Vincent <[email protected]>:

Quoted 30 lines

Platforms may have some external power control which need to be controlled from board specific code. ?This is the case for some versions of the MOP500 board (U8500 platform).

Acked-by Linus Walleij <[email protected]>
Signed-off-by Rabin Vincent <[email protected]>

---
?drivers/mmc/host/mmci.c ? | ? ?3 ++-
?include/linux/amba/mmci.h | ? ?6 ++++--
?2 files changed, 6 insertions(+), 3 deletions(-)

drivers/mmc/host/mmci.c +0 -0
index 322958f..63eb7f4 100644
@@ -483,7 +483,8 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)

? ? ? ? ? ? ? ? * power control behind this translate function.
? ? ? ? ? ? ? ? */
? ? ? ? ? ? ? ?if (!host->vcc && host->plat->translate_vdd)
- ? ? ? ? ? ? ? ? ? ? ? pwr |= host->plat->translate_vdd(mmc_dev(mmc), ios->vdd);
+ ? ? ? ? ? ? ? ? ? ? ? pwr |= host->plat->translate_vdd(mmc_dev(mmc), ios->vdd,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?ios->power_mode); ? ? ? ? ? ? ? ?/* The ST version does not have this, fall through to POWER_ON */ ? ? ? ? ? ? ? ?if (host->hw_designer != AMBA_VENDOR_ST) {
? ? ? ? ? ? ? ? ? ? ? ?pwr |= MCI_PWR_UP;

include/linux/amba/mmci.h +0 -0
index f9d1bb5..61b5810 100644
@@ -17,7 +17,8 @@
Quoted 6 lines

?* mmc/host.h
?* @translate_vdd: a callback function to translate a MMC_VDD_* ?* mask into a value to be binary or:ed and written into the
- * MMCIPWR register of the block
+ * MMCIPWR register of the block. ?May also control external power
+ * based on the power_mode.

Actually this callback is named like that for a reason: it is to be used to translate the MMC_VDD_* into a u8:4 bitmask for the MMCIPWR register, 4 bits possibly routed out of the PL180 block on some designs (never seen in practice, but could be used in theory).

So it translates a voltage into a 4-bit bitmask, hence the name.

Now the semantics are altered to have other side-effects in the platform, so the function should be renamed, something like platform_vdd_handler() would be more appropriate.

Yours,
Linus Walleij

author
#21 lore raw

PATCH 7/12 mmci: pass power_mode to the translate_vdd callback

Quoted 14 lines · Linus Walleij

2010/6/22 Rabin Vincent <[email protected]>:

Quoted 7 lines
(quoted diff) +0 -0
@@ -17,7 +17,8 @@

?* mmc/host.h
?* @translate_vdd: a callback function to translate a MMC_VDD_* ?* mask into a value to be binary or:ed and written into the
- * MMCIPWR register of the block
+ * MMCIPWR register of the block. ?May also control external power
+ * based on the power_mode.

Actually this callback is named like that for a reason: it is to be used to translate the MMC_VDD_* into a u8:4 bitmask for the MMCIPWR register, 4 bits possibly routed out of the PL180 block on some designs (never seen in practice, but could be used in theory).

Quoted 6 lines

So it translates a voltage into a 4-bit bitmask, hence the name.

Now the semantics are altered to have other side-effects in the platform, so the function should be renamed, something like platform_vdd_handler() would be more appropriate.

This is already inside platform data, so how about just ->vdd_handler()? Or ->set_power(), like some other drivers?

Note that we'd also like to use this to do the board-specific settings for the *DIREN and FBCLK bits, which replace the Voltage bits on some ST variants, like so:

+static u32 mop500_sdi0_translate_vdd(struct device *dev, unsigned int vdd,
+ unsigned char power_mode)
+{
+ if (power_mode == MMC_POWER_UP)
+ gpio_set_value(gpio_sdmmc_en, 1);
+ else if (power_mode == MMC_POWER_OFF)
+ gpio_set_value(gpio_sdmmc_en, 0);
+
+ return MCI_FBCLKEN | MCI_CMDDIREN | MCI_DATA0DIREN |
+ MCI_DATA2DIREN | MCI_DATA31DIREN;
+}

Rabin

#22 lore raw

PATCH 7/12 mmci: pass power_mode to the translate_vdd callback

[Rabin]

Quoted 18 lines · Linus Walleij
Quoted 10 lines

2010/6/22 Rabin Vincent <[email protected]>:

Quoted 7 lines
(quoted diff) +0 -0
@@ -17,7 +17,8 @@

?* mmc/host.h
?* @translate_vdd: a callback function to translate a MMC_VDD_* ?* mask into a value to be binary or:ed and written into the
- * MMCIPWR register of the block
+ * MMCIPWR register of the block. ?May also control external power
+ * based on the power_mode.

Actually this callback is named like that for a reason: it is to be

used

Quoted 6 lines

to translate the MMC_VDD_* into a u8:4 bitmask for the MMCIPWR register, 4 bits possibly routed out of the PL180 block on some designs (never seen in practice, but could be used in theory).

So it translates a voltage into a 4-bit bitmask, hence the name.

Quoted 8 lines
Quoted 4 lines

Now the semantics are altered to have other side-effects in the platform, so the function should be renamed, something like platform_vdd_handler() would be more appropriate.

This is already inside platform data, so how about just -

Quoted 1 line

vdd_handler()?

Or ->set_power(), like some other drivers?

Any of these look good to me atleast.

But now another thing I came to think of (the fun never ends):

Since the ux500 also have a regulator for this and since that mechanism is mutually exclusive in the code (since the semantics implied that it was only a voltage translation function) you should also remove the condition that this only gets called in case there is no regulator.

Quoted 7 lines

Note that we'd also like to use this to do the board-specific settings for the *DIREN and FBCLK bits, which replace the Voltage bits on some ST
variants, like so:

+static u32 mop500_sdi0_translate_vdd(struct device *dev, unsigned int vdd,

Review

Quoted 10 lines
(quoted diff) +10 -0
+				     unsigned char power_mode)
+{
+	if (power_mode == MMC_POWER_UP)
+		gpio_set_value(gpio_sdmmc_en, 1);
+	else if (power_mode == MMC_POWER_OFF)
+		gpio_set_value(gpio_sdmmc_en, 0);
+
+	return MCI_FBCLKEN | MCI_CMDDIREN | MCI_DATA0DIREN |
+	       MCI_DATA2DIREN | MCI_DATA31DIREN;
+}

OK just update the kerneldoc to reflect the fact that the platform can OR on any *custom* bits it like to the PWR register in this function.

Side notice: some of bits are for the ST version only, when you patch in the latter, can you take this opportunity to also rename MCI_FBCLKEN to MCI_ST_FBCLKEN and so on for all stuff that is ST-specific? I think I am the sinner here, we had no convention for how to do this naming when we first began modifying this driver. It should be crystal clear that all custom extensions are named MCI_<VENDOR>_FOO.

As a consequence, anything that is OR:ed into from the vdd_handler() should typically be MCI_<VENDOR>_FOO, and it will be clear why it's there.

Yours,
Linus Walleij

author
#23 lore raw

PATCH v2 7/12 mmci: pass power_mode to the translate_vdd callback

In reply to Rabin Vincent

Platforms may have some external power control which need to be controlled from board specific code. Rename the translate_vdd() callback to vdd_handler() and pass it the power mode.

Acked-by Linus Walleij <[email protected]>
Signed-off-by Rabin Vincent <[email protected]>

---
v2:
- Call vdd_handler even if regulator is used
- Rename translate_vdd to vdd_handler

 drivers/mmc/host/mmci.c   |   13 +++----------
 include/linux/amba/mmci.h |   10 ++++++----
 2 files changed, 9 insertions(+), 14 deletions(-)
drivers/mmc/host/mmci.c +3 -10
index 5ef262b..d8e0ea3 100644
@@ -477,16 +477,9 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 			/* This implicitly enables the regulator */
 			mmc_regulator_set_ocr(host->vcc, ios->vdd);
 #endif
-		/*
-		 * The translate_vdd function is not used if you have
-		 * an external regulator, or your design is really weird.
-		 * Using it would mean sending in power control BOTH using
-		 * a regulator AND the 4 MMCIPWR bits. If we don't have
-		 * a regulator, we might have some other platform specific
-		 * power control behind this translate function.
-		 */
-		if (!host->vcc && host->plat->translate_vdd)
-			pwr |= host->plat->translate_vdd(mmc_dev(mmc), ios->vdd);
+		if (host->plat->vdd_handler)
+			pwr |= host->plat->vdd_handler(mmc_dev(mmc), ios->vdd,
+						       ios->power_mode);
 		/* The ST version does not have this, fall through to POWER_ON */
 		if (host->hw_designer != AMBA_VENDOR_ST) {
 			pwr |= MCI_PWR_UP;
include/linux/amba/mmci.h +6 -4
index f9d1bb5..71a2576 100644
@@ -15,9 +15,10 @@
  * @ocr_mask: available voltages on the 4 pins from the block, this
  * is ignored if a regulator is used, see the MMC_VDD_* masks in
  * mmc/host.h
- * @translate_vdd: a callback function to translate a MMC_VDD_*
- * mask into a value to be binary or:ed and written into the
- * MMCIPWR register of the block
+ * @vdd_handler: a callback function to translate a MMC_VDD_*
+ * mask into a value to be binary (or set some other custom bits
+ * in MMCIPWR) or:ed and written into the MMCIPWR register of the
+ * block.  May also control external power based on the power_mode.
  * @status: if no GPIO read function was given to the block in
  * gpio_wp (below) this function will be called to determine
  * whether a card is present in the MMC slot or not

@@ -30,7 +31,8 @@
 struct mmci_platform_data {
 	unsigned int f_max;
 	unsigned int ocr_mask;
-	u32 (*translate_vdd)(struct device *, unsigned int);
+	u32 (*vdd_handler)(struct device *, unsigned int vdd,
+			   unsigned char power_mode);
 	unsigned int (*status)(struct device *);
 	int	gpio_wp;
 	int	gpio_cd;
Signature · 1 line
1.7.0
author
#24 lore raw

PATCH 8/12 mmci: add variant data and default MCICLOCK support

In reply to Rabin Vincent

Add a variant_data structure to handle the differences between the various variants of this peripheral. Add a first quirk for a default MCICLOCK value, required on the Ux500 variant where the enable bit needs to be always set, since it controls access to some registers.

Acked-by Linus Walleij <[email protected]>
Signed-off-by Rabin Vincent <[email protected]>
---
 drivers/mmc/host/mmci.c |   31 ++++++++++++++++++++++++++++++-
 drivers/mmc/host/mmci.h |    2 ++
 2 files changed, 32 insertions(+), 1 deletions(-)
drivers/mmc/host/mmci.c +30 -1
index 63eb7f4..a1d1023 100644
@@ -36,12 +36,30 @@
 
 static unsigned int fmax = 515633;
 
+/**
+ * struct variant_data - MMCI variant-specific quirks
+ * @clkreg: default value for MCICLOCK register
+ */
+struct variant_data {
+	unsigned int		clkreg;
+};
+
+static struct variant_data variant_arm = {
+};
+
+static struct variant_data variant_u300 = {
+};
+
+static struct variant_data variant_ux500 = {
+	.clkreg			= MCI_CLK_ENABLE,
+};
 /*
  * This must be called with host->lock held
  */
 static void mmci_set_clkreg(struct mmci_host *host, unsigned int desired)
 {
-	u32 clk = 0;
+	struct variant_data *variant = host->variant;
+	u32 clk = variant->clkreg;
 
 	if (desired) {
 		if (desired >= host->mclk) {

@@ -564,6 +582,7 @@ static const struct mmc_host_ops mmci_ops = {
 static int __devinit mmci_probe(struct amba_device *dev, struct amba_id *id)
 {
 	struct mmci_platform_data *plat = dev->dev.platform_data;
+	struct variant_data *variant = id->data;
 	struct mmci_host *host;
 	struct mmc_host *mmc;
 	int ret;

@@ -608,6 +627,7 @@ static int __devinit mmci_probe(struct amba_device *dev, struct amba_id *id)
 		goto clk_free;
 
 	host->plat = plat;
+	host->variant = variant;
 	host->mclk = clk_get_rate(host->clk);
 	/*
 	 * According to the spec, mclk is max 100 MHz,

@@ -860,19 +880,28 @@ static struct amba_id mmci_ids[] = {
 	{
 		.id	= 0x00041180,
 		.mask	= 0x000fffff,
+		.data	= &variant_arm,
 	},
 	{
 		.id	= 0x00041181,
 		.mask	= 0x000fffff,
+		.data	= &variant_arm,
 	},
 	/* ST Micro variants */
 	{
 		.id     = 0x00180180,
 		.mask   = 0x00ffffff,
+		.data	= &variant_u300,
 	},
 	{
 		.id     = 0x00280180,
 		.mask   = 0x00ffffff,
+		.data	= &variant_u300,
+	},
+	{
+		.id     = 0x00480180,
+		.mask   = 0x00ffffff,
+		.data	= &variant_ux500,
 	},
 	{ 0, 0 },
 };
drivers/mmc/host/mmci.h +2 -0
index 516adc6..5e635e1 100644
@@ -145,6 +145,7 @@
 #define NR_SG		16
 
 struct clk;
+struct variant_data;
 
 struct mmci_host {
 	void __iomem		*base;

@@ -165,6 +166,7 @@ struct mmci_host {
 	unsigned int		cclk;
 	u32			pwr;
 	struct mmci_platform_data *plat;
+	struct variant_data	*variant;
 
 	u8			hw_designer;
 	u8			hw_revision:4;
Signature · 1 line
1.7.0
author
#25 lore raw

PATCH 9/12 mmci: enable hardware flow control on Ux500 variants

In reply to Rabin Vincent

Although both the U300 and Ux500 use ST variants, the HWFCEN bits are at different positions, so use the variant_data to store the information.

Acked-by Linus Walleij <[email protected]>
Signed-off-by Rabin Vincent <[email protected]>
---
 drivers/mmc/host/mmci.c |    8 ++++++--
 drivers/mmc/host/mmci.h |    2 --
 2 files changed, 6 insertions(+), 4 deletions(-)
drivers/mmc/host/mmci.c +6 -2
index a1d1023..3fca46d 100644
@@ -39,19 +39,23 @@ static unsigned int fmax = 515633;
 /**
  * struct variant_data - MMCI variant-specific quirks
  * @clkreg: default value for MCICLOCK register
+ * @clkreg_enable: enable value for MMCICLOCK register
  */
 struct variant_data {
 	unsigned int		clkreg;
+	unsigned int		clkreg_enable;
 };
 
 static struct variant_data variant_arm = {
 };
 
 static struct variant_data variant_u300 = {
+	.clkreg_enable		= 1 << 13, /* HWFCEN */
 };
 
 static struct variant_data variant_ux500 = {
 	.clkreg			= MCI_CLK_ENABLE,
+	.clkreg_enable		= 1 << 14, /* HWFCEN */
 };
 /*
  * This must be called with host->lock held

@@ -71,8 +75,8 @@ static void mmci_set_clkreg(struct mmci_host *host, unsigned int desired)
 				clk = 255;
 			host->cclk = host->mclk / (2 * (clk + 1));
 		}
-		if (host->hw_designer == AMBA_VENDOR_ST)
-			clk |= MCI_ST_FCEN; /* Bug fix in ST IP block */
+
+		clk |= variant->clkreg_enable;
 		clk |= MCI_CLK_ENABLE;
 		/* This hasn't proven to be worthwhile */
 		/* clk |= MCI_CLK_PWRSAVE; */
drivers/mmc/host/mmci.h +0 -2
index 5e635e1..0a11ddc 100644
@@ -28,8 +28,6 @@
 #define MCI_4BIT_BUS		(1 << 11)
 /* 8bit wide buses supported in ST Micro versions */
 #define MCI_ST_8BIT_BUS		(1 << 12)
-/* HW flow control on the ST Micro version */
-#define MCI_ST_FCEN		(1 << 13)
 
 #define MMCIARGUMENT		0x008
 #define MMCICOMMAND		0x00c
Signature · 1 line
1.7.0
author
#26 lore raw

PATCH 10/12 mmci: support larger MMCIDATALENGTH register

In reply to Rabin Vincent

The Ux500 variant has a 24-bit MMCIDATALENGTH register, as opposed to the 16-bit one on the ARM version.

Acked-by Linus Walleij <[email protected]>
Signed-off-by Rabin Vincent <[email protected]>
---
 drivers/mmc/host/mmci.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)
drivers/mmc/host/mmci.c +9 -3
index 3fca46d..6c8e41b 100644
@@ -40,22 +40,27 @@ static unsigned int fmax = 515633;
  * struct variant_data - MMCI variant-specific quirks
  * @clkreg: default value for MCICLOCK register
  * @clkreg_enable: enable value for MMCICLOCK register
+ * @datalength_bits: number of bits in the MMCIDATALENGTH register
  */
 struct variant_data {
 	unsigned int		clkreg;
 	unsigned int		clkreg_enable;
+	unsigned int		datalength_bits;
 };
 
 static struct variant_data variant_arm = {
+	.datalength_bits	= 16,
 };
 
 static struct variant_data variant_u300 = {
 	.clkreg_enable		= 1 << 13, /* HWFCEN */
+	.datalength_bits	= 16,
 };
 
 static struct variant_data variant_ux500 = {
 	.clkreg			= MCI_CLK_ENABLE,
 	.clkreg_enable		= 1 << 14, /* HWFCEN */
+	.datalength_bits	= 24,
 };
 /*
  * This must be called with host->lock held

@@ -700,10 +705,11 @@ static int __devinit mmci_probe(struct amba_device *dev, struct amba_id *id)
 	mmc->max_phys_segs = NR_SG;
 
 	/*
-	 * Since we only have a 16-bit data length register, we must
-	 * ensure that we don't exceed 2^16-1 bytes in a single request.
+	 * Since only a certain number of bits are valid in the data length
+	 * register, we must ensure that we don't exceed 2^num-1 bytes in a
+	 * single request.
 	 */
-	mmc->max_req_size = 65535;
+	mmc->max_req_size = (1 << variant->datalength_bits) - 1;
 
 	/*
 	 * Set the maximum segment size.  Since we aren't doing DMA
Signature · 1 line
1.7.0
author
#27 lore raw

PATCH 11/12 mmci: support different FIFO sizes

In reply to Rabin Vincent

The Ux500 variant has a 32-word FIFO (TXFIFOEMPTY is asserted when it has 2 left) and TXFIFOHALFEMPTY is repurposed as TXFIFOBURSTWRITEABLE, with a burst being defined as 8-words. Likewise for RX.

Acked-by Linus Walleij <[email protected]>
Signed-off-by Rabin Vincent <[email protected]>
---
 drivers/mmc/host/mmci.c |   22 +++++++++++++++++++---
 drivers/mmc/host/mmci.h |    7 -------
 2 files changed, 19 insertions(+), 10 deletions(-)
drivers/mmc/host/mmci.c +19 -3
index 6c8e41b..4eb7265 100644
@@ -41,23 +41,35 @@ static unsigned int fmax = 515633;
  * @clkreg: default value for MCICLOCK register
  * @clkreg_enable: enable value for MMCICLOCK register
  * @datalength_bits: number of bits in the MMCIDATALENGTH register
+ * @fifosize: number of bytes that can be written when MMCI_TXFIFOEMPTY
+ *	      is asserted (likewise for RX)
+ * @fifohalfsize: number of bytes that can be written when MCI_TXFIFOHALFEMPTY
+ *		  is asserted (likewise for RX)
  */
 struct variant_data {
 	unsigned int		clkreg;
 	unsigned int		clkreg_enable;
 	unsigned int		datalength_bits;
+	unsigned int		fifosize;
+	unsigned int		fifohalfsize;
 };
 
 static struct variant_data variant_arm = {
+	.fifosize		= 16 * 4,
+	.fifohalfsize		= 8 * 4,
 	.datalength_bits	= 16,
 };
 
 static struct variant_data variant_u300 = {
+	.fifosize		= 16 * 4,
+	.fifohalfsize		= 8 * 4,
 	.clkreg_enable		= 1 << 13, /* HWFCEN */
 	.datalength_bits	= 16,
 };
 
 static struct variant_data variant_ux500 = {
+	.fifosize		= 30 * 4,
+	.fifohalfsize		= 8 * 4,
 	.clkreg			= MCI_CLK_ENABLE,
 	.clkreg_enable		= 1 << 14, /* HWFCEN */
 	.datalength_bits	= 24,

@@ -138,6 +150,7 @@ static void mmci_init_sg(struct mmci_host *host, struct mmc_data *data)
 
 static void mmci_start_data(struct mmci_host *host, struct mmc_data *data)
 {
+	struct variant_data *variant = host->variant;
 	unsigned int datactrl, timeout, irqmask;
 	unsigned long long clks;
 	void __iomem *base;

@@ -173,7 +186,7 @@ static void mmci_start_data(struct mmci_host *host, struct mmc_data *data)
 		 * If we have less than a FIFOSIZE of bytes to transfer,
 		 * trigger a PIO interrupt as soon as any data is available.
 		 */
-		if (host->size < MCI_FIFOSIZE)
+		if (host->size < variant->fifosize)
 			irqmask |= MCI_RXDATAAVLBLMASK;
 	} else {
 		/*

@@ -316,13 +329,15 @@ static int mmci_pio_read(struct mmci_host *host, char *buffer, unsigned int rema
 
 static int mmci_pio_write(struct mmci_host *host, char *buffer, unsigned int remain, u32 status)
 {
+	struct variant_data *variant = host->variant;
 	void __iomem *base = host->base;
 	char *ptr = buffer;
 
 	do {
 		unsigned int count, maxcnt;
 
-		maxcnt = status & MCI_TXFIFOEMPTY ? MCI_FIFOSIZE : MCI_FIFOHALFSIZE;
+		maxcnt = status & MCI_TXFIFOEMPTY ?
+			 variant->fifosize : variant->fifohalfsize;
 		count = min(remain, maxcnt);
 
 		writesl(base + MMCIFIFO, ptr, count >> 2);

@@ -346,6 +361,7 @@ static irqreturn_t mmci_pio_irq(int irq, void *dev_id)
 {
 	struct mmci_host *host = dev_id;
 	struct sg_mapping_iter *sg_miter = &host->sg_miter;
+	struct variant_data *variant = host->variant;
 	void __iomem *base = host->base;
 	unsigned long flags;
 	u32 status;

@@ -401,7 +417,7 @@ static irqreturn_t mmci_pio_irq(int irq, void *dev_id)
 	 * If we're nearing the end of the read, switch to
 	 * "any data available" mode.
 	 */
-	if (status & MCI_RXACTIVE && host->size < MCI_FIFOSIZE)
+	if (status & MCI_RXACTIVE && host->size < variant->fifosize)
 		writel(MCI_RXDATAAVLBLMASK, base + MMCIMASK1);
 
 	/*
drivers/mmc/host/mmci.h +0 -7
index 0a11ddc..c7d373c 100644
@@ -133,13 +133,6 @@
 	MCI_DATATIMEOUTMASK|MCI_TXUNDERRUNMASK|MCI_RXOVERRUNMASK|	\
 	MCI_CMDRESPENDMASK|MCI_CMDSENTMASK|MCI_DATABLOCKENDMASK)
 
-/*
- * The size of the FIFO in bytes.
- */
-#define MCI_FIFOSIZE	(16*4)
-	
-#define MCI_FIFOHALFSIZE (MCI_FIFOSIZE / 2)
-
 #define NR_SG		16
 
 struct clk;
Signature · 1 line
1.7.0
author
#28 lore raw

PATCH 12/12 mmci: support variants with only one irq

In reply to Rabin Vincent

The Ux500 variants have only one IRQ line hooked up. Allow these to work by directing the PIO interrupts also to the first IRQ line.

Acked-by Linus Walleij <[email protected]>
Signed-off-by Rabin Vincent <[email protected]>
---
 drivers/mmc/host/mmci.c |   67 ++++++++++++++++++++++++++++++++++++++++------
 drivers/mmc/host/mmci.h |    5 +++
 2 files changed, 63 insertions(+), 9 deletions(-)
drivers/mmc/host/mmci.c +58 -9
index 4eb7265..5baa3bd 100644
@@ -45,6 +45,7 @@ static unsigned int fmax = 515633;
  *	      is asserted (likewise for RX)
  * @fifohalfsize: number of bytes that can be written when MCI_TXFIFOHALFEMPTY
  *		  is asserted (likewise for RX)
+ * @singleirq: true if only one IRQ line is available
  */
 struct variant_data {
 	unsigned int		clkreg;

@@ -52,6 +53,7 @@ struct variant_data {
 	unsigned int		datalength_bits;
 	unsigned int		fifosize;
 	unsigned int		fifohalfsize;
+	bool			singleirq;
 };
 
 static struct variant_data variant_arm = {

@@ -73,7 +75,9 @@ static struct variant_data variant_ux500 = {
 	.clkreg			= MCI_CLK_ENABLE,
 	.clkreg_enable		= 1 << 14, /* HWFCEN */
 	.datalength_bits	= 24,
+	.singleirq		= true,
 };
+
 /*
  * This must be called with host->lock held
  */

@@ -129,10 +133,27 @@ mmci_request_end(struct mmci_host *host, struct mmc_request *mrq)
 	spin_lock(&host->lock);
 }
 
+static void mmci_set_mask1(struct mmci_host *host, unsigned int mask)
+{
+	struct variant_data *variant = host->variant;
+	void __iomem *base = host->base;
+
+	if (variant->singleirq) {
+		unsigned int mask0 = readl(base + MMCIMASK0);
+
+		mask0 &= ~MCI_IRQ1MASK;
+		mask0 |= mask;
+
+		writel(mask0, base + MMCIMASK0);
+	}
+
+	writel(mask, base + MMCIMASK1);
+}
+
 static void mmci_stop_data(struct mmci_host *host)
 {
 	writel(0, host->base + MMCIDATACTRL);
-	writel(0, host->base + MMCIMASK1);
+	mmci_set_mask1(host, 0);
 	host->data = NULL;
 }
 

@@ -198,7 +219,7 @@ static void mmci_start_data(struct mmci_host *host, struct mmc_data *data)
 
 	writel(datactrl, base + MMCIDATACTRL);
 	writel(readl(base + MMCIMASK0) & ~MCI_DATAENDMASK, base + MMCIMASK0);
-	writel(irqmask, base + MMCIMASK1);
+	mmci_set_mask1(host, irqmask);
 }
 
 static void

@@ -260,6 +281,10 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
 	if (status & MCI_DATAEND) {
 		mmci_stop_data(host);
 
+		/* MCI_DATABLOCKEND not used with single irq */
+		if (host->variant->singleirq && !data->error)
+			host->data_xfered = data->blksz * data->blocks;
+
 		if (!data->stop) {
 			mmci_request_end(host, data->mrq);
 		} else {

@@ -418,7 +443,7 @@ static irqreturn_t mmci_pio_irq(int irq, void *dev_id)
 	 * "any data available" mode.
 	 */
 	if (status & MCI_RXACTIVE && host->size < variant->fifosize)
-		writel(MCI_RXDATAAVLBLMASK, base + MMCIMASK1);
+		mmci_set_mask1(host, MCI_RXDATAAVLBLMASK);
 
 	/*
 	 * If we run out of data, disable the data IRQs; this

@@ -427,7 +452,7 @@ static irqreturn_t mmci_pio_irq(int irq, void *dev_id)
 	 * stops us racing with our data end IRQ.
 	 */
 	if (host->size == 0) {
-		writel(0, base + MMCIMASK1);
+		mmci_set_mask1(host, 0);
 		writel(readl(base + MMCIMASK0) | MCI_DATAENDMASK, base + MMCIMASK0);
 	}
 

@@ -440,6 +465,7 @@ static irqreturn_t mmci_pio_irq(int irq, void *dev_id)
 static irqreturn_t mmci_irq(int irq, void *dev_id)
 {
 	struct mmci_host *host = dev_id;
+	struct variant_data *variant = host->variant;
 	u32 status;
 	int ret = 0;
 

@@ -450,6 +476,14 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
 		struct mmc_data *data;
 
 		status = readl(host->base + MMCISTATUS);
+
+		if (variant->singleirq) {
+			if (status & readl(host->base + MMCIMASK1))
+				mmci_pio_irq(irq, dev_id);
+
+			status &= ~MCI_IRQ1MASK;
+		}
+
 		status &= readl(host->base + MMCIMASK0);
 		writel(status, host->base + MMCICLEAR);
 

@@ -610,6 +644,7 @@ static int __devinit mmci_probe(struct amba_device *dev, struct amba_id *id)
 	struct variant_data *variant = id->data;
 	struct mmci_host *host;
 	struct mmc_host *mmc;
+	unsigned int mask;
 	int ret;
 
 	/* must have platform data */

@@ -782,11 +817,23 @@ static int __devinit mmci_probe(struct amba_device *dev, struct amba_id *id)
 	if (ret)
 		goto unmap;
 
-	ret = request_irq(dev->irq[1], mmci_pio_irq, IRQF_SHARED, DRIVER_NAME " (pio)", host);
-	if (ret)
-		goto irq0_free;
+	if (!variant->singleirq) {
+		ret = request_irq(dev->irq[1], mmci_pio_irq, IRQF_SHARED,
+				  DRIVER_NAME " (pio)", host);
+		if (ret)
+			goto irq0_free;
+	}
+
+	/*
+	 * MCI_DATABLOCKEND doesn't seem to immediately clear from the status,
+	 * so we can't use it keep count when only one irq is used because the
+	 * irq will hit for other reasons.
+	 */
+	mask = MCI_IRQENABLE;
+	if (variant->singleirq)
+		mask &= ~MCI_DATABLOCKEND;
 
-	writel(MCI_IRQENABLE, host->base + MMCIMASK0);
+	writel(mask, host->base + MMCIMASK0);
 
 	amba_set_drvdata(dev, mmc);
 

@@ -830,6 +877,7 @@ static int __devexit mmci_remove(struct amba_device *dev)
 
 	if (mmc) {
 		struct mmci_host *host = mmc_priv(mmc);
+		struct variant_data *variant = host->variant;
 
 		mmc_remove_host(mmc);
 

@@ -840,7 +888,8 @@ static int __devexit mmci_remove(struct amba_device *dev)
 		writel(0, host->base + MMCIDATACTRL);
 
 		free_irq(dev->irq[0], host);
-		free_irq(dev->irq[1], host);
+		if (!variant->singleirq)
+			free_irq(dev->irq[1], host);
 
 		if (host->gpio_wp != -ENOSYS)
 			gpio_free(host->gpio_wp);
drivers/mmc/host/mmci.h +5 -0
index c7d373c..ba203b8 100644
@@ -133,6 +133,11 @@
 	MCI_DATATIMEOUTMASK|MCI_TXUNDERRUNMASK|MCI_RXOVERRUNMASK|	\
 	MCI_CMDRESPENDMASK|MCI_CMDSENTMASK|MCI_DATABLOCKENDMASK)
 
+/* These interrupts are directed to IRQ1 when two IRQ lines are available */
+#define MCI_IRQ1MASK \
+	(MCI_RXFIFOHALFFULLMASK | MCI_RXDATAAVLBLMASK | \
+	 MCI_TXFIFOHALFEMPTYMASK)
+
 #define NR_SG		16
 
 struct clk;
Signature · 1 line
1.7.0
#29 lore raw

PATCH 12/12 mmci: support variants with only one irq

2010/6/22 Rabin Vincent <[email protected]>:

Quoted 2 lines

The Ux500 variants have only one IRQ line hooked up. ?Allow these to work by directing the PIO interrupts also to the first IRQ line.

I see this one is dangling in the patch tracker, FYI I have regression tested this on the PB1176MPcore with its unmodified ARM PL180 MMCI block - read/write files copy mount/unmount - no problems.

Yours,
Linus Walleij

author
#30 lore raw

PATCH v2 1/1 mmci: work with only one irq

In reply to Rabin Vincent

The DBx500 variants have only one IRQ line hooked up. Allow these (and any other implementations which choose to use only one irq) to work by directing the PIO interrupts also to the first IRQ line.

Acked-by Linus Walleij <[email protected]>
Signed-off-by Rabin Vincent <[email protected]>

---
v2: don't depend on variant, as suggested by Russell in the patch tracker

 drivers/mmc/host/mmci.c |   62 ++++++++++++++++++++++++++++++++++++++++-------
 drivers/mmc/host/mmci.h |    6 ++++
 2 files changed, 59 insertions(+), 9 deletions(-)
drivers/mmc/host/mmci.c +53 -9
index 87b4fc6..8255333 100644
@@ -129,10 +129,26 @@ mmci_request_end(struct mmci_host *host, struct mmc_request *mrq)
 	spin_lock(&host->lock);
 }
 
+static void mmci_set_mask1(struct mmci_host *host, unsigned int mask)
+{
+	void __iomem *base = host->base;
+
+	if (host->singleirq) {
+		unsigned int mask0 = readl(base + MMCIMASK0);
+
+		mask0 &= ~MCI_IRQ1MASK;
+		mask0 |= mask;
+
+		writel(mask0, base + MMCIMASK0);
+	}
+
+	writel(mask, base + MMCIMASK1);
+}
+
 static void mmci_stop_data(struct mmci_host *host)
 {
 	writel(0, host->base + MMCIDATACTRL);
-	writel(0, host->base + MMCIMASK1);
+	mmci_set_mask1(host, 0);
 	host->data = NULL;
 }
 

@@ -198,7 +214,7 @@ static void mmci_start_data(struct mmci_host *host, struct mmc_data *data)
 
 	writel(datactrl, base + MMCIDATACTRL);
 	writel(readl(base + MMCIMASK0) & ~MCI_DATAENDMASK, base + MMCIMASK0);
-	writel(irqmask, base + MMCIMASK1);
+	mmci_set_mask1(host, irqmask);
 }
 
 static void

@@ -276,6 +292,10 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
 	if (status & MCI_DATAEND) {
 		mmci_stop_data(host);
 
+		/* MCI_DATABLOCKEND not used with single irq */
+		if (host->singleirq && !data->error)
+			host->data_xfered = data->blksz * data->blocks;
+
 		if (!data->stop) {
 			mmci_request_end(host, data->mrq);
 		} else {

@@ -437,7 +457,7 @@ static irqreturn_t mmci_pio_irq(int irq, void *dev_id)
 	 * "any data available" mode.
 	 */
 	if (status & MCI_RXACTIVE && host->size < variant->fifosize)
-		writel(MCI_RXDATAAVLBLMASK, base + MMCIMASK1);
+		mmci_set_mask1(host, MCI_RXDATAAVLBLMASK);
 
 	/*
 	 * If we run out of data, disable the data IRQs; this

@@ -446,7 +466,7 @@ static irqreturn_t mmci_pio_irq(int irq, void *dev_id)
 	 * stops us racing with our data end IRQ.
 	 */
 	if (host->size == 0) {
-		writel(0, base + MMCIMASK1);
+		mmci_set_mask1(host, 0);
 		writel(readl(base + MMCIMASK0) | MCI_DATAENDMASK, base + MMCIMASK0);
 	}
 

@@ -469,6 +489,14 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
 		struct mmc_data *data;
 
 		status = readl(host->base + MMCISTATUS);
+
+		if (host->singleirq) {
+			if (status & readl(host->base + MMCIMASK1))
+				mmci_pio_irq(irq, dev_id);
+
+			status &= ~MCI_IRQ1MASK;
+		}
+
 		status &= readl(host->base + MMCIMASK0);
 		writel(status, host->base + MMCICLEAR);
 

@@ -635,6 +663,7 @@ static int __devinit mmci_probe(struct amba_device *dev, struct amba_id *id)
 	struct variant_data *variant = id->data;
 	struct mmci_host *host;
 	struct mmc_host *mmc;
+	unsigned int mask;
 	int ret;
 
 	/* must have platform data */

@@ -806,11 +835,25 @@ static int __devinit mmci_probe(struct amba_device *dev, struct amba_id *id)
 	if (ret)
 		goto unmap;
 
-	ret = request_irq(dev->irq[1], mmci_pio_irq, IRQF_SHARED, DRIVER_NAME " (pio)", host);
-	if (ret)
-		goto irq0_free;
+	if (dev->irq[1] == NO_IRQ)
+		host->singleirq = true;
+	else {
+		ret = request_irq(dev->irq[1], mmci_pio_irq, IRQF_SHARED,
+				  DRIVER_NAME " (pio)", host);
+		if (ret)
+			goto irq0_free;
+	}
+
+	/*
+	 * MCI_DATABLOCKEND doesn't seem to immediately clear from the status,
+	 * so we can't use it keep count when only one irq is used because the
+	 * irq will hit for other reasons.
+	 */
+	mask = MCI_IRQENABLE;
+	if (host->singleirq)
+		mask &= ~MCI_DATABLOCKEND;
 
-	writel(MCI_IRQENABLE, host->base + MMCIMASK0);
+	writel(mask, host->base + MMCIMASK0);
 
 	amba_set_drvdata(dev, mmc);
 

@@ -864,7 +907,8 @@ static int __devexit mmci_remove(struct amba_device *dev)
 		writel(0, host->base + MMCIDATACTRL);
 
 		free_irq(dev->irq[0], host);
-		free_irq(dev->irq[1], host);
+		if (!host->singleirq)
+			free_irq(dev->irq[1], host);
 
 		if (host->gpio_wp != -ENOSYS)
 			gpio_free(host->gpio_wp);
drivers/mmc/host/mmci.h +6 -0
index c7d373c..b4fdd65 100644
@@ -133,6 +133,11 @@
 	MCI_DATATIMEOUTMASK|MCI_TXUNDERRUNMASK|MCI_RXOVERRUNMASK|	\
 	MCI_CMDRESPENDMASK|MCI_CMDSENTMASK|MCI_DATABLOCKENDMASK)
 
+/* These interrupts are directed to IRQ1 when two IRQ lines are available */
+#define MCI_IRQ1MASK \
+	(MCI_RXFIFOHALFFULLMASK | MCI_RXDATAAVLBLMASK | \
+	 MCI_TXFIFOHALFEMPTYMASK)
+
 #define NR_SG		16
 
 struct clk;

@@ -148,6 +153,7 @@ struct mmci_host {
 	int			gpio_cd;
 	int			gpio_wp;
 	int			gpio_cd_irq;
+	bool			singleirq;
 
 	unsigned int		data_xfered;
Signature · 1 line
1.7.1
#31 lore raw

PATCH v2 1/1 mmci: work with only one irq

2010/10/11 Rabin Vincent <[email protected]>:

Quoted 8 lines

The DBx500 variants have only one IRQ line hooked up. ?Allow these (and any other implementations which choose to use only one irq) to work by directing the PIO interrupts also to the first IRQ line. (...)
+ ? ? ? ? ? ? ? /* MCI_DATABLOCKEND not used with single irq */
+ ? ? ? ? ? ? ? if (host->singleirq && !data->error)
+ ? ? ? ? ? ? ? ? ? ? ? host->data_xfered = data->blksz * data->blocks; +

I replaced that last line with:
host->data_xfered += data->blksz * data->blocks;

In my follow-on "support out-of-order IRQ events" patch, does it make sense?

Once this is in the patch system I'll rebase that patch too...

Yours,
Linus Walleij

#32 lore raw

PATCH v2 1/1 mmci: work with only one irq

In reply to Rabin Vincent

drivers/mmc/host/mmci.c mmci_data_irq(struct mmci_host *host, struct mmc_data *data, · line 298

Quoted 8 lines · Rabin Vincent
drivers/mmc/host/mmci.c +4 -0
@@ -276,6 +292,10 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
 	if (status & MCI_DATAEND) {
 		mmci_stop_data(host);
 
+		/* MCI_DATABLOCKEND not used with single irq */
+		if (host->singleirq && !data->error)
+			host->data_xfered = data->blksz * data->blocks;
+

I'm uneasy about this. If we don't get datablockend interrupts, then when there's an error we don't know how much data was transferred, and with big requests, we're going to have a very long time spent retrying block by block.

#33 lore raw

PATCH v2 1/1 mmci: work with only one irq

2010/10/11 Russell King - ARM Linux <[email protected]>:

Quoted 5 lines · Rabin Vincent
Quoted 4 lines
(quoted diff) +0 -0
@@ -276,6 +292,10 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
 ? ? ? if (status & MCI_DATAEND) {
 ? ? ? ? ? ? ? mmci_stop_data(host);
Quoted 9 lines

Review mmci_data_irq(struct mmci_host *host, struct mmc_data *data, · line 298

Quoted 4 lines
(quoted diff) +4 -0
+ ? ? ? ? ? ? /* MCI_DATABLOCKEND not used with single irq */
+ ? ? ? ? ? ? if (host->singleirq && !data->error)
+ ? ? ? ? ? ? ? ? ? ? host->data_xfered = data->blksz * data->blocks;
+

I'm uneasy about this. ?If we don't get datablockend interrupts, then when there's an error we don't know how much data was transferred, and with big requests, we're going to have a very long time spent retrying block by block.

This one should probably remain a ux500-specific flag, unlike the single IRQ flag, this is something else.

AFAICT the DBCKEND signal is simply not routed in the ux500 (probably neither on the Nomadik) variant, so we can't wait for it.

On the U300 on the other hand, it is routed and available, if and only if you're using PIO mode. (However it can arrive *after* the DATAEND signal, which the other pending patch takes care of).

During DMA this signal is disfunct also in U300...

Yours,
Linus Walleij

#34 lore raw

PATCH v2 1/1 mmci: work with only one irq

In reply to Russell King - ARM Linux

2010/10/11 Russell King - ARM Linux <[email protected]>:

Quoted 5 lines · Rabin Vincent
Quoted 4 lines
(quoted diff) +0 -0
@@ -276,6 +292,10 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
 ? ? ? if (status & MCI_DATAEND) {
 ? ? ? ? ? ? ? mmci_stop_data(host);
Quoted 9 lines

Review mmci_data_irq(struct mmci_host *host, struct mmc_data *data, · line 298

Quoted 4 lines
(quoted diff) +4 -0
+ ? ? ? ? ? ? /* MCI_DATABLOCKEND not used with single irq */
+ ? ? ? ? ? ? if (host->singleirq && !data->error)
+ ? ? ? ? ? ? ? ? ? ? host->data_xfered = data->blksz * data->blocks;
+

I'm uneasy about this. ?If we don't get datablockend interrupts, then when there's an error we don't know how much data was transferred, and with big requests, we're going to have a very long time spent retrying block by block.

I have sent a respin of Rabins patch (he's on vacation, so I got his permission to fix this up) which removes this offending handling of MCI_DATABLOCKEND, it's now also in the patch tracker as 6311/2.

On top of this I have updated my patch for out-of-order IRQ arrival to cover also this case and renamed it
"handle broken MCI_DATABLOCKEND hardware"
it will do away with the confusing U300 #ifdef and adds a comment block that explains what the problem really is on the Ux hardwares, basically that under some circumstances in the U300 and under all circumstances in the Ux500, the MCI_DATABLOCKEND flag is totally unreliable and "hangs" at 1.

Yours,
Linus Walleij

#35 lore raw

PATCH 1/12 mmci: use sg_miter API to fix multi-page sg handling

In reply to Rabin Vincent

I've tested this MMCI patch set on RealView PB1176 and U300, mounted cards, copied files back and forth, unmount diff files. No problems.

This should cover all MMCI variants, since the PB1176 is a vanilla ARM variant.

Just fix the comments on patch #7 and it should be ripe for the patch tracker methinks.

Yours,
Linus Walleij

author
#36 lore raw

PATCH v2 1/12 mmci: use sg_miter API to fix multi-page sg handling

In reply to Rabin Vincent

The mmci driver's SG list iteration logic assumes that each SG entry spans only one page, and only maps and flushes one page of the sg. This is not a valid assumption. Fix it by converting the driver to the sg_miter API, which correctly handles sgs which span multiple pages.

Acked-by Linus Walleij <[email protected]>
Signed-off-by Rabin Vincent <[email protected]>

---
v2:
- Keep the flush_dcache_page() inside the driver, since the flush_kernel_dcache_page() call inside the sg_miter API is inadequate

 drivers/mmc/host/mmci.c |   60 +++++++++++++++++++++++++++++-----------------
 drivers/mmc/host/mmci.h |   35 +--------------------------
 2 files changed, 39 insertions(+), 56 deletions(-)
drivers/mmc/host/mmci.c +38 -22
index 4917af9..d63d756 100644
@@ -26,7 +26,6 @@
 #include <linux/amba/mmci.h>
 #include <linux/regulator/consumer.h>
 
-#include <asm/cacheflush.h>
 #include <asm/div64.h>
 #include <asm/io.h>
 #include <asm/sizes.h>

@@ -98,6 +97,18 @@ static void mmci_stop_data(struct mmci_host *host)
 	host->data = NULL;
 }
 
+static void mmci_init_sg(struct mmci_host *host, struct mmc_data *data)
+{
+	unsigned int flags = SG_MITER_ATOMIC;
+
+	if (data->flags & MMC_DATA_READ)
+		flags |= SG_MITER_TO_SG;
+	else
+		flags |= SG_MITER_FROM_SG;
+
+	sg_miter_start(&host->sg_miter, data->sg, data->sg_len, flags);
+}
+
 static void mmci_start_data(struct mmci_host *host, struct mmc_data *data)
 {
 	unsigned int datactrl, timeout, irqmask;

@@ -210,8 +221,17 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
 		 * We hit an error condition.  Ensure that any data
 		 * partially written to a page is properly coherent.
 		 */
-		if (host->sg_len && data->flags & MMC_DATA_READ)
-			flush_dcache_page(sg_page(host->sg_ptr));
+		if (data->flags & MMC_DATA_READ) {
+			struct sg_mapping_iter *sg_miter = &host->sg_miter;
+			unsigned long flags;
+
+			local_irq_save(flags);
+			if (sg_miter_next(sg_miter)) {
+				flush_dcache_page(sg_miter->page);
+				sg_miter_stop(sg_miter);
+			}
+			local_irq_restore(flags);
+		}
 	}
 	if (status & MCI_DATAEND) {
 		mmci_stop_data(host);

@@ -314,15 +334,18 @@ static int mmci_pio_write(struct mmci_host *host, char *buffer, unsigned int rem
 static irqreturn_t mmci_pio_irq(int irq, void *dev_id)
 {
 	struct mmci_host *host = dev_id;
+	struct sg_mapping_iter *sg_miter = &host->sg_miter;
 	void __iomem *base = host->base;
+	unsigned long flags;
 	u32 status;
 
 	status = readl(base + MMCISTATUS);
 
 	dev_dbg(mmc_dev(host->mmc), "irq1 (pio) %08x\n", status);
 
+	local_irq_save(flags);
+
 	do {
-		unsigned long flags;
 		unsigned int remain, len;
 		char *buffer;
 

@@ -336,11 +359,11 @@ static irqreturn_t mmci_pio_irq(int irq, void *dev_id)
 		if (!(status & (MCI_TXFIFOHALFEMPTY|MCI_RXDATAAVLBL)))
 			break;
 
-		/*
-		 * Map the current scatter buffer.
-		 */
-		buffer = mmci_kmap_atomic(host, &flags) + host->sg_off;
-		remain = host->sg_ptr->length - host->sg_off;
+		if (!sg_miter_next(sg_miter))
+			break;
+
+		buffer = sg_miter->addr;
+		remain = sg_miter->length;
 
 		len = 0;
 		if (status & MCI_RXACTIVE)

@@ -348,31 +371,24 @@ static irqreturn_t mmci_pio_irq(int irq, void *dev_id)
 		if (status & MCI_TXACTIVE)
 			len = mmci_pio_write(host, buffer, remain, status);
 
-		/*
-		 * Unmap the buffer.
-		 */
-		mmci_kunmap_atomic(host, buffer, &flags);
+		sg_miter->consumed = len;
 
-		host->sg_off += len;
 		host->size -= len;
 		remain -= len;
 
 		if (remain)
 			break;
 
-		/*
-		 * If we were reading, and we have completed this
-		 * page, ensure that the data cache is coherent.
-		 */
 		if (status & MCI_RXACTIVE)
-			flush_dcache_page(sg_page(host->sg_ptr));
-
-		if (!mmci_next_sg(host))
-			break;
+			flush_dcache_page(sg_miter->page);
 
 		status = readl(base + MMCISTATUS);
 	} while (1);
 
+	sg_miter_stop(sg_miter);
+
+	local_irq_restore(flags);
+
 	/*
 	 * If we're nearing the end of the read, switch to
 	 * "any data available" mode.
drivers/mmc/host/mmci.h +1 -34
index d77062e..7cb24ab 100644
@@ -171,42 +171,9 @@ struct mmci_host {
 	struct timer_list	timer;
 	unsigned int		oldstat;
 
-	unsigned int		sg_len;
-
 	/* pio stuff */
-	struct scatterlist	*sg_ptr;
-	unsigned int		sg_off;
+	struct sg_mapping_iter	sg_miter;
 	unsigned int		size;
 	struct regulator	*vcc;
 };
 
-static inline void mmci_init_sg(struct mmci_host *host, struct mmc_data *data)
-{
-	/*
-	 * Ideally, we want the higher levels to pass us a scatter list.
-	 */
-	host->sg_len = data->sg_len;
-	host->sg_ptr = data->sg;
-	host->sg_off = 0;
-}
-
-static inline int mmci_next_sg(struct mmci_host *host)
-{
-	host->sg_ptr++;
-	host->sg_off = 0;
-	return --host->sg_len;
-}
-
-static inline char *mmci_kmap_atomic(struct mmci_host *host, unsigned long *flags)
-{
-	struct scatterlist *sg = host->sg_ptr;
-
-	local_irq_save(*flags);
-	return kmap_atomic(sg_page(sg), KM_BIO_SRC_IRQ) + sg->offset;
-}
-
-static inline void mmci_kunmap_atomic(struct mmci_host *host, void *buffer, unsigned long *flags)
-{
-	kunmap_atomic(buffer, KM_BIO_SRC_IRQ);
-	local_irq_restore(*flags);
-}
Signature · 1 line
1.7.0