[PATCH] mmci: fixup broken_blockend variant patch v2

[PATCH] mmci: fixup broken_blockend variant patch v2

[Linus Walleij]

From: Ulf Hansson <ulf.hansson@stericsson.com>

host->last_blockend flag is now set only for the last
MCI_DATABLOCKEND, the previous code would just react to
any blockend, which was buggy. Consolidate Ux500 and U300
bug flags to a single one and use only the MCI_DATAEND irq
on U300 as well, it turns out it was broken just like the
Ux500.

Also make sure the MCI_DATABLOCKENDMASK is set only when
needed, instead of being set always and then masked off.

Tested successfully on Ux500, U300 and ARM RealView
PB1176.

Cc: Sebastian Rasmussen <sebastian.rasmussen@stericsson.com>
Cc: Rabin Vincent <rabin.vincent@stericsson.com>
Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
[Tweaking and adding err message]
Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
v2: also updated the comment.
---
 drivers/mmc/host/mmci.c |   93 +++++++++++++++++++++++++----------------------
 drivers/mmc/host/mmci.h |    4 +-
 2 files changed, 52 insertions(+), 45 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 0b4a5bf..7e0d38c 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -48,8 +48,6 @@ static unsigned int fmax = 515633;
  *		  is asserted (likewise for RX)
  * @broken_blockend: the MCI_DATABLOCKEND is broken on the hardware
  *		and will not work at all.
- * @broken_blockend_dma: the MCI_DATABLOCKEND is broken on the hardware when
- *		using DMA.
  * @sdio: variant supports SDIO
  * @st_clkdiv: true if using a ST-specific clock divider algorithm
  */
@@ -60,7 +58,6 @@ struct variant_data {
 	unsigned int		fifosize;
 	unsigned int		fifohalfsize;
 	bool			broken_blockend;
-	bool			broken_blockend_dma;
 	bool			sdio;
 	bool			st_clkdiv;
 };
@@ -76,7 +73,7 @@ static struct variant_data variant_u300 = {
 	.fifohalfsize		= 8 * 4,
 	.clkreg_enable		= 1 << 13, /* HWFCEN */
 	.datalength_bits	= 16,
-	.broken_blockend_dma	= true,
+	.broken_blockend	= true,
 	.sdio			= true,
 };
 
@@ -199,7 +196,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 int datactrl, timeout, irqmask0, irqmask1;
 	unsigned long long clks;
 	void __iomem *base;
 	int blksz_bits;
@@ -210,7 +207,7 @@ static void mmci_start_data(struct mmci_host *host, struct mmc_data *data)
 	host->data = data;
 	host->size = data->blksz * data->blocks;
 	host->data_xfered = 0;
-	host->blockend = false;
+	host->last_blockend = false;
 	host->dataend = false;
 
 	mmci_init_sg(host, data);
@@ -230,20 +227,20 @@ static void mmci_start_data(struct mmci_host *host, struct mmc_data *data)
 	datactrl = MCI_DPSM_ENABLE | blksz_bits << 4;
 	if (data->flags & MMC_DATA_READ) {
 		datactrl |= MCI_DPSM_DIRECTION;
-		irqmask = MCI_RXFIFOHALFFULLMASK;
+		irqmask1 = MCI_RXFIFOHALFFULLMASK;
 
 		/*
 		 * 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 < variant->fifosize)
-			irqmask |= MCI_RXDATAAVLBLMASK;
+			irqmask1 |= MCI_RXDATAAVLBLMASK;
 	} else {
 		/*
 		 * We don't actually need to include "FIFO empty" here
 		 * since its implicit in "FIFO half empty".
 		 */
-		irqmask = MCI_TXFIFOHALFEMPTYMASK;
+		irqmask1 = MCI_TXFIFOHALFEMPTYMASK;
 	}
 
 	/* The ST Micro variants has a special bit to enable SDIO */
@@ -252,8 +249,14 @@ static void mmci_start_data(struct mmci_host *host, struct mmc_data *data)
 			datactrl |= MCI_ST_DPSM_SDIOEN;
 
 	writel(datactrl, base + MMCIDATACTRL);
-	writel(readl(base + MMCIMASK0) & ~MCI_DATAENDMASK, base + MMCIMASK0);
-	mmci_set_mask1(host, irqmask);
+	irqmask0 = readl(base + MMCIMASK0);
+	if (variant->broken_blockend)
+		irqmask0 &= ~MCI_DATABLOCKENDMASK;
+	else
+		irqmask0 |= MCI_DATABLOCKENDMASK;
+	irqmask0 &= ~MCI_DATAENDMASK;
+	writel(irqmask0, base + MMCIMASK0);
+	mmci_set_mask1(host, irqmask1);
 }
 
 static void
@@ -301,7 +304,7 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
 			data->error = -EIO;
 
 		/* Force-complete the transaction */
-		host->blockend = true;
+		host->last_blockend = true;
 		host->dataend = true;
 
 		/*
@@ -322,27 +325,18 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
 	}
 
 	/*
-	 * On ARM variants in PIO mode, MCI_DATABLOCKEND
-	 * is always sent first, and we increase the
-	 * transfered number of bytes for that IRQ. Then
-	 * MCI_DATAEND follows and we conclude the transaction.
+	 * On ARM variants in PIO mode, MCI_DATABLOCKEND is always sent first,
+	 * and we increase the transfered number of bytes for that IRQ. Repeat
+	 * for the number of blocks in the transfer. Then MCI_DATAEND follows
+	 * and we conclude the transaction.
 	 *
-	 * On the Ux500 single-IRQ variant 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, and
-	 * then the flag is still up. So we use the MCI_DATAEND
-	 * IRQ at the end of the entire transfer because
-	 * MCI_DATABLOCKEND is broken.
-	 *
-	 * In the U300, the IRQs can arrive out-of-order,
-	 * e.g. MCI_DATABLOCKEND sometimes arrives after MCI_DATAEND,
-	 * so for this case we use the flags "blockend" and
-	 * "dataend" to make sure both IRQs have arrived before
-	 * concluding the transaction. (This does not apply
-	 * to the Ux500 which doesn't fire MCI_DATABLOCKEND
-	 * at all.) In DMA mode it suffers from the same problem
-	 * as the Ux500.
+	 * On the U300 and Ux500 a few to many MCI_DATABLOCKEND interrupts
+	 * are usually missing, so the data counter is not properly increased.
+	 * This is likely because new block arrive while the IRQ is being
+	 * processed without the transfers being held back, and ACK:in the
+	 * interrupt will only clear the very last block and intermediate
+	 * blockend interrupts get lost. So we simply mask of the blockend
+	 * interrupt on these and only use the dataend interrupt.
 	 */
 	if (status & MCI_DATABLOCKEND) {
 		/*
@@ -353,22 +347,41 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
 		 */
 		if (!variant->broken_blockend)
 			host->data_xfered += data->blksz;
-		host->blockend = true;
+		if (host->data_xfered == data->blksz * data->blocks)
+			host->last_blockend = true;
 	}
 
-	if (status & MCI_DATAEND)
+	if (status & MCI_DATAEND) {
+		if (!host->last_blockend && !variant->broken_blockend &&
+		    !data->error) {
+			/*
+			 * This typically occur on hardware with broken
+			 * blockend reporting, which will fire less blockend
+			 * IRQs than would be expected. Print an error and
+			 * repair the situation, but the hardware should
+			 * really be flagged as broken so this IRQ is not used
+			 * at all.
+			 */
+			dev_err(mmc_dev(host->mmc),
+				"missing %d block interrupts!\n",
+				(data->blksz * data->blocks - host->data_xfered) / data->blksz);
+			host->data_xfered = data->blksz * data->blocks;
+			host->last_blockend = true;
+		}
 		host->dataend = true;
+	}
 
 	/*
 	 * On variants with broken blockend we shall only wait for dataend,
 	 * on others we must sync with the blockend signal since they can
 	 * appear out-of-order.
 	 */
-	if (host->dataend && (host->blockend || variant->broken_blockend)) {
+	if (host->dataend &&
+	    (host->last_blockend || variant->broken_blockend)) {
 		mmci_stop_data(host);
 
 		/* Reset these flags */
-		host->blockend = false;
+		host->last_blockend = false;
 		host->dataend = false;
 
 		/*
@@ -770,7 +783,6 @@ 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 */
@@ -951,12 +963,7 @@ static int __devinit mmci_probe(struct amba_device *dev, struct amba_id *id)
 			goto irq0_free;
 	}
 
-	mask = MCI_IRQENABLE;
-	/* Don't use the datablockend flag if it's broken */
-	if (variant->broken_blockend)
-		mask &= ~MCI_DATABLOCKEND;
-
-	writel(mask, host->base + MMCIMASK0);
+	writel(MCI_IRQENABLE, host->base + MMCIMASK0);
 
 	amba_set_drvdata(dev, mmc);
 
diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
index df06f01..7ac8c4d 100644
--- a/drivers/mmc/host/mmci.h
+++ b/drivers/mmc/host/mmci.h
@@ -137,7 +137,7 @@
 #define MCI_IRQENABLE	\
 	(MCI_CMDCRCFAILMASK|MCI_DATACRCFAILMASK|MCI_CMDTIMEOUTMASK|	\
 	MCI_DATATIMEOUTMASK|MCI_TXUNDERRUNMASK|MCI_RXOVERRUNMASK|	\
-	MCI_CMDRESPENDMASK|MCI_CMDSENTMASK|MCI_DATABLOCKENDMASK)
+	MCI_CMDRESPENDMASK|MCI_CMDSENTMASK)
 
 /* These interrupts are directed to IRQ1 when two IRQ lines are available */
 #define MCI_IRQ1MASK \
@@ -177,7 +177,7 @@ struct mmci_host {
 	struct timer_list	timer;
 	unsigned int		oldstat;
 
-	bool			blockend;
+	bool			last_blockend;
 	bool			dataend;
 
 	/* pio stuff */
-- 
1.7.3.2

[PATCH] mmci: fixup broken_blockend variant patch v2

[Rabin Vincent]

On Mon, Jan 17, 2011 at 20:07, Linus Walleij
<linus.walleij@stericsson.com> wrote:
> + ? ? ? ?* On the U300 and Ux500 a few to many MCI_DATABLOCKEND interrupts
> + ? ? ? ?* are usually missing, so the data counter is not properly increased.
> + ? ? ? ?* This is likely because new block arrive while the IRQ is being
> + ? ? ? ?* processed without the transfers being held back, and ACK:in the
> + ? ? ? ?* interrupt will only clear the very last block and intermediate
> + ? ? ? ?* blockend interrupts get lost. So we simply mask of the blockend
> + ? ? ? ?* interrupt on these and only use the dataend interrupt.

Have you observed this to be different on the ARM variants?  I asked a
related question about this earlier:

http://thread.gmane.org/gmane.linux.ports.arm.kernel/101118:
| In MMCI, on variants which use the DATABLOCKEND interrupt, if we cross
| more than one block boundary in the loop inside mmc_pio_read() or
| mmci_pio_write(), when the DATABLOCKEND irq is handled after interrupts
| are enabled in mmci_pio_irq(), won't the block count be incorrect
| because we would consider that only one block has been transferred, when
| in fact it has been two?  Or can this never happen for some reason?

[PATCH] mmci: fixup broken_blockend variant patch v2

[Rabin Vincent]

On Mon, Jan 17, 2011 at 20:07, Linus Walleij
<linus.walleij@stericsson.com> wrote:
> - ? ? ? ?* In the U300, the IRQs can arrive out-of-order,
> - ? ? ? ?* e.g. MCI_DATABLOCKEND sometimes arrives after MCI_DATAEND,
> - ? ? ? ?* so for this case we use the flags "blockend" and
> - ? ? ? ?* "dataend" to make sure both IRQs have arrived before
> - ? ? ? ?* concluding the transaction.

Since this was your original rationale for adding the "blockend" flag,
why don't you just remove that flag in this patch instead of renaming to
last_blockend and still keeping it around?

On Mon, Jan 17, 2011 at 20:07, Linus Walleij
<linus.walleij@stericsson.com> wrote:
> Also make sure the MCI_DATABLOCKENDMASK is set only when
> needed, instead of being set always and then masked off.

This seems to be unrelated to the actual problem this patch is trying to
solve.

> ? ? ? ?writel(datactrl, base + MMCIDATACTRL);
> - ? ? ? writel(readl(base + MMCIMASK0) & ~MCI_DATAENDMASK, base + MMCIMASK0);
> - ? ? ? mmci_set_mask1(host, irqmask);
> + ? ? ? irqmask0 = readl(base + MMCIMASK0);
> + ? ? ? if (variant->broken_blockend)
> + ? ? ? ? ? ? ? irqmask0 &= ~MCI_DATABLOCKENDMASK;
> + ? ? ? else
> + ? ? ? ? ? ? ? irqmask0 |= MCI_DATABLOCKENDMASK;
> + ? ? ? irqmask0 &= ~MCI_DATAENDMASK;
> + ? ? ? writel(irqmask0, base + MMCIMASK0);
> + ? ? ? mmci_set_mask1(host, irqmask1);
> ?}

If your intention is to not have to mask MCI_DATABLOCKENDMASK off, why
are you doing so?

[PATCH] mmci: fixup broken_blockend variant patch v2

[Linus Walleij]

On 01/17/2011 04:36 PM, Rabin Vincent wrote:
> On Mon, Jan 17, 2011 at 20:07, Linus Walleij
> <linus.walleij@stericsson.com>  wrote:
>    
>> -        * In the U300, the IRQs can arrive out-of-order,
>> -        * e.g. MCI_DATABLOCKEND sometimes arrives after MCI_DATAEND,
>> -        * so for this case we use the flags "blockend" and
>> -        * "dataend" to make sure both IRQs have arrived before
>> -        * concluding the transaction.
>>      
> Since this was your original rationale for adding the "blockend" flag,
> why don't you just remove that flag in this patch instead of renaming to
> last_blockend and still keeping it around?
>    

Because now I use it to soft-detect the error and auto-correct
when it occurs. (Try removing the broken_blockend flag
from Ux500 for example, see the prints and working
transfers.)

So as to get help from the code in future hardware.

Which is probably going to be useful, since I'm told that there
are actually two more drivers for PL180 derivates in the
kernel, just under different names... So one day when we
may consolidate them, such stuff is going to be helpful.

> On Mon, Jan 17, 2011 at 20:07, Linus Walleij
> <linus.walleij@stericsson.com>  wrote:
>    
>> Also make sure the MCI_DATABLOCKENDMASK is set only when
>> needed, instead of being set always and then masked off.
>>      
> This seems to be unrelated to the actual problem this patch is trying to
> solve.
>    

Yeah I can split it off in a two-series patch, no problem.

>>         writel(datactrl, base + MMCIDATACTRL);
>> -       writel(readl(base + MMCIMASK0)&  ~MCI_DATAENDMASK, base + MMCIMASK0);
>> -       mmci_set_mask1(host, irqmask);
>> +       irqmask0 = readl(base + MMCIMASK0);
>> +       if (variant->broken_blockend)
>> +               irqmask0&= ~MCI_DATABLOCKENDMASK;
>> +       else
>> +               irqmask0 |= MCI_DATABLOCKENDMASK;
>> +       irqmask0&= ~MCI_DATAENDMASK;
>> +       writel(irqmask0, base + MMCIMASK0);
>> +       mmci_set_mask1(host, irqmask1);
>>   }
>>      
> If your intention is to not have to mask MCI_DATABLOCKENDMASK off, why
> are you doing so?
>    

The intention is to turn it off for HW which we know is broken,
(has broken_blocken set true) which is what happens above.

If it is still broken, i.e. you get too few IRQs anyway, the driver
will now detect that and report the error, and attempt to cover
up for it, instead of hanging the system, which is what happens
otherwise (the driver hangs perpetually waiting for it's
last blockend IRQ).

Sort of thing I find helpful, but taste varies of course... Some
would call it over-cautious.

Russell can you indicate whether you want that to detect and
recover (as now) or detect and error or just hang?

Yours,
Linus Walleij

[PATCH] mmci: fixup broken_blockend variant patch v2

[Linus Walleij]

On 01/17/2011 04:12 PM, Rabin Vincent wrote:
> On Mon, Jan 17, 2011 at 20:07, Linus Walleij
> <linus.walleij@stericsson.com>  wrote:
>    
>> +        * On the U300 and Ux500 a few to many MCI_DATABLOCKEND interrupts
>> +        * are usually missing, so the data counter is not properly increased.
>> +        * This is likely because new block arrive while the IRQ is being
>> +        * processed without the transfers being held back, and ACK:in the
>> +        * interrupt will only clear the very last block and intermediate
>> +        * blockend interrupts get lost. So we simply mask of the blockend
>> +        * interrupt on these and only use the dataend interrupt.
>>      
> Have you observed this to be different on the ARM variants?  I asked a
> related question about this earlier:
>
> http://thread.gmane.org/gmane.linux.ports.arm.kernel/101118:
> | In MMCI, on variants which use the DATABLOCKEND interrupt, if we cross
> | more than one block boundary in the loop inside mmc_pio_read() or
> | mmci_pio_write(), when the DATABLOCKEND irq is handled after interrupts
> | are enabled in mmci_pio_irq(), won't the block count be incorrect
> | because we would consider that only one block has been transferred, when
> | in fact it has been two?  Or can this never happen for some reason?
>    

Yes it is very different, see the log I sent in reply to Russells earlier
mail. It's not just one blockend missing, sometimes it's two (on U300)
and on Ux500 it's something like 5 out of 128 blockends that actually
get fired, the rest are missing.

Now the only variant actually using that interrupt is the original
ARM version found in RealView & Versatile. I have run this code
on the PB1176 but it wasn't missing any blockends. No errors.

I tried to increase the clock speed to see if I could provoke the
error in the RealView, but hit FIFO overflow (since these variants
does not have hardware flow control) before I got to the speed
where I presume the error could occur.

Yours,
Linus Walleij

[PATCH] mmci: fixup broken_blockend variant patch v2

[Rabin Vincent]

On Mon, Jan 17, 2011 at 21:26, Linus Walleij
<linus.walleij@stericsson.com> wrote:
> On 01/17/2011 04:36 PM, Rabin Vincent wrote:
>> On Mon, Jan 17, 2011 at 20:07, Linus Walleij <linus.walleij@stericsson.com> ?wrote:
>>> ? ? ? ?writel(datactrl, base + MMCIDATACTRL);
>>> - ? ? ? writel(readl(base + MMCIMASK0)& ?~MCI_DATAENDMASK, base +
>>> MMCIMASK0);
>>> - ? ? ? mmci_set_mask1(host, irqmask);
>>> + ? ? ? irqmask0 = readl(base + MMCIMASK0);
>>> + ? ? ? if (variant->broken_blockend)
>>> + ? ? ? ? ? ? ? irqmask0&= ~MCI_DATABLOCKENDMASK;
>>> + ? ? ? else
>>> + ? ? ? ? ? ? ? irqmask0 |= MCI_DATABLOCKENDMASK;
>>> + ? ? ? irqmask0&= ~MCI_DATAENDMASK;
>>> + ? ? ? writel(irqmask0, base + MMCIMASK0);
>>> + ? ? ? mmci_set_mask1(host, irqmask1);
>>> ?}
>>>
>>
>> If your intention is to not have to mask MCI_DATABLOCKENDMASK off, why
>> are you doing so?
>>
> The intention is to turn it off for HW which we know is broken,
> (has broken_blocken set true) which is what happens above.

Right, but don't you already remove the place where it was being set?

| -       mask = MCI_IRQENABLE;
| -       /* Don't use the datablockend flag if it's broken */
| -       if (variant->broken_blockend)
| -               mask &= ~MCI_DATABLOCKEND;
| -
| -       writel(mask, host->base + MMCIMASK0);
| +       writel(MCI_IRQENABLE, host->base + MMCIMASK0);
|
|        amba_set_drvdata(dev, mmc);
|
| diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
| index df06f01..7ac8c4d 100644
| --- a/drivers/mmc/host/mmci.h
| +++ b/drivers/mmc/host/mmci.h
| @@ -137,7 +137,7 @@
|  #define MCI_IRQENABLE  \
|        (MCI_CMDCRCFAILMASK|MCI_DATACRCFAILMASK|MCI_CMDTIMEOUTMASK|     \
|        MCI_DATATIMEOUTMASK|MCI_TXUNDERRUNMASK|MCI_RXOVERRUNMASK|       \
| -       MCI_CMDRESPENDMASK|MCI_CMDSENTMASK|MCI_DATABLOCKENDMASK)
| +       MCI_CMDRESPENDMASK|MCI_CMDSENTMASK)
|

[PATCH] mmci: fixup broken_blockend variant patch v2

[Russell King - ARM Linux]

On Mon, Jan 17, 2011 at 05:02:08PM +0100, Linus Walleij wrote:
> Yes it is very different, see the log I sent in reply to Russells earlier
> mail. It's not just one blockend missing, sometimes it's two (on U300)
> and on Ux500 it's something like 5 out of 128 blockends that actually
> get fired, the rest are missing.
>
> Now the only variant actually using that interrupt is the original
> ARM version found in RealView & Versatile. I have run this code
> on the PB1176 but it wasn't missing any blockends. No errors.
>
> I tried to increase the clock speed to see if I could provoke the
> error in the RealView, but hit FIFO overflow (since these variants
> does not have hardware flow control) before I got to the speed
> where I presume the error could occur.

How reliable is the FIFOCNT register ?

What I'm wondering is whether we can get rid of the DATABLOCKEND
interrupts completely, and instead read the FIFOCNT register to
discover how many blocks have been successfully transferred.  FIFOCNT
on read gives you the remaining number of words to be transferred into
the FIFO from the card, not the number of words still to be read by the
host CPU.

It's going to require some thought though, as I don't think you can use
the register to say X bytes, transferred, that means we got X bytes
successfully.  If you have a data CRC error, you will have transferred
most, if not all of the block in error, so you'd need to wind back to
the start of the block.  In theory, data CRC errors should cause the
card to stop transmission.

A data timeout (meaning a missing start condition) on the other hand
means the data block hasn't started to be transferred.

So, what I think's required (for read) is:

	remain = readl(host->base + MMCIFIFOCNT) << 2;
	success = data->blksz * data->blocks - remain;

	if (status & MCI_DATACRCFAIL) {
		/* round down to last block */
		host->data_xfered = ((success / data->blksz) - 1) * data->blksz;
		data->error = -EILSEQ;
	} else if (status & MCI_DATATIMEOUT) {
		host->data_xfered = success;
		data->error = -ETIMEDOUT;
	} else if (status & (MCI_TXUNDERRUN|MCI_RXOVERRUN)) {
		host->data_xfered = success;
		data->error = -EIO;
	}

Not sure if that's valid for write atm.

[PATCH] mmci: fixup broken_blockend variant patch v2

[Linus Walleij]

2011/1/17 Rabin Vincent <rabin@rab.in>:
> On Mon, Jan 17, 2011 at 21:26, Linus Walleij
>> The intention is to turn it off for HW which we know is broken,
>> (has broken_blocken set true) which is what happens above.
>
> Right, but don't you already remove the place where it was being set?
>
> | - ? ? ? mask = MCI_IRQENABLE;
> | - ? ? ? /* Don't use the datablockend flag if it's broken */
> | - ? ? ? if (variant->broken_blockend)
> | - ? ? ? ? ? ? ? mask &= ~MCI_DATABLOCKEND;
> | -
> | - ? ? ? writel(mask, host->base + MMCIMASK0);
> | + ? ? ? writel(MCI_IRQENABLE, host->base + MMCIMASK0);
> |
> | ? ? ? ?amba_set_drvdata(dev, mmc);
> |
> | diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
> | index df06f01..7ac8c4d 100644
> | --- a/drivers/mmc/host/mmci.h
> | +++ b/drivers/mmc/host/mmci.h
> | @@ -137,7 +137,7 @@
> | ?#define MCI_IRQENABLE ?\
> | ? ? ? ?(MCI_CMDCRCFAILMASK|MCI_DATACRCFAILMASK|MCI_CMDTIMEOUTMASK| ? ? \
> | ? ? ? ?MCI_DATATIMEOUTMASK|MCI_TXUNDERRUNMASK|MCI_RXOVERRUNMASK| ? ? ? \
> | - ? ? ? MCI_CMDRESPENDMASK|MCI_CMDSENTMASK|MCI_DATABLOCKENDMASK)
> | + ? ? ? MCI_CMDRESPENDMASK|MCI_CMDSENTMASK)

Um, this shows I should really split the patch, yes MCI_DATABLOCKENDMASK
is removed as set by default, then added back depending on whether the
.broken_blockend flag is set or not.

In the code quoted earlier actually:

       writel(datactrl, base + MMCIDATACTRL);
-       writel(readl(base + MMCIMASK0)&  ~MCI_DATAENDMASK, base + MMCIMASK0);
-       mmci_set_mask1(host, irqmask);
+       irqmask0 = readl(base + MMCIMASK0);
+       if (variant->broken_blockend)
+               irqmask0&= ~MCI_DATABLOCKENDMASK;
+       else
+               irqmask0 |= MCI_DATABLOCKENDMASK;
+       irqmask0&= ~MCI_DATAENDMASK;
+       writel(irqmask0, base + MMCIMASK0);
+       mmci_set_mask1(host, irqmask1);

Sorry for the confusion...

Yours,
Linus Walleij

[PATCH] mmci: fixup broken_blockend variant patch v2

[Linus Walleij]

2011/1/18 Russell King - ARM Linux <linux@arm.linux.org.uk>:

> How reliable is the FIFOCNT register ?
>
> What I'm wondering is whether we can get rid of the DATABLOCKEND
> interrupts completely, and instead read the FIFOCNT register to
> discover how many blocks have been successfully transferred. ?FIFOCNT
> on read gives you the remaining number of words to be transferred into
> the FIFO from the card, not the number of words still to be read by the
> host CPU.

Hmm... the FIFOCNT is just the FIFO.

Do you mean the MMCIDATACNT register? According to the manual
it has exactly that meaning, and is valid "when the data transfer is
complete" which you could assume it to be if you hit an error.
But I'm not betting on it...

Possibly you should add/subtract the words in FIFOCNT too,
but I would say that's a later excercise.

But I think it's a good idea. I'll prep an RFC patch we can test,
we may need some broken cards to test it on though.

> Not sure if that's valid for write atm.

In that specific case you may need to also add the FIFOCNT I
suspect, but I guess we can/will find out.

This approach surely gives better precision than using the blockend
interrupts any day.

I'll hack up something and give it a spin.

I don't know what the purpose of the blockend interrupt really
is, but suspect it may be about reusing ring buffers for MMC
transfers which Linux currently doesn't have need for.

Yours,
Linus Walleij

[PATCH] mmci: fixup broken_blockend variant patch v2

[Russell King - ARM Linux]

On Wed, Jan 19, 2011 at 09:34:49PM +0100, Linus Walleij wrote:
> 2011/1/18 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> 
> > How reliable is the FIFOCNT register ?
> >
> > What I'm wondering is whether we can get rid of the DATABLOCKEND
> > interrupts completely, and instead read the FIFOCNT register to
> > discover how many blocks have been successfully transferred. ?FIFOCNT
> > on read gives you the remaining number of words to be transferred into
> > the FIFO from the card, not the number of words still to be read by the
> > host CPU.
> 
> Hmm... the FIFOCNT is just the FIFO.

No, FIFOCNT.

 The MCIFifoCnt register contains the remaining number of words to be
 written to or read from the FIFO. The FIFO counter loads the value from
 the data length register (see Data length register, MCIDataLength on page
 3-11) when the Enable bit is set in the data control register. If the data
 length is not word aligned (multiple of 4), the remaining 1 to 3 bytes are
 regarded as a word. Table 3-19 shows the bit assignment of the MCIFifoCnt
 register.

static int mmci_pio_read(struct mmci_host *host, char *buffer, unsigned int remain)
{
        do {
                int count = host_remain - (readl(base + MMCIFIFOCNT) << 2);

                if (count > remain)
                        count = remain;

                if (count <= 0)
                        break;

                readsl(base + MMCIFIFO, ptr, count >> 2);

We use FIFOCNT to work out how many bytes are needing to be transferred
from the FIFO on PIO reads.

[PATCH] mmci: fixup broken_blockend variant patch v2

[Linus Walleij]

2011/1/19 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Wed, Jan 19, 2011 at 09:34:49PM +0100, Linus Walleij wrote:
>> 2011/1/18 Russell King - ARM Linux <linux@arm.linux.org.uk>:
>>
>> > How reliable is the FIFOCNT register ?
>>
>> Hmm... the FIFOCNT is just the FIFO.
>
> No, FIFOCNT.
>
> ?The MCIFifoCnt register contains the remaining number of words to be
> ?written to or read from the FIFO.

Yeah I B damned that's right. Sorry for the confusion. I'll respin
the latest patch on the FIFOCNT register instead and test
tomorrow.

But if the transfer is complete, MCIDataCnt should be valid
as well. I'll read the datasheet(s) closer and see if I get some
useful clues about which one is more reliable in the error
case.

Sorry for late night confusion...
Linus Walleij

[PATCH] mmci: fixup broken_blockend variant patch v2

[Linus Walleij]

2011/1/20 Linus Walleij <linus.ml.walleij@gmail.com>:
> 2011/1/19 Russell King - ARM Linux <linux@arm.linux.org.uk>:
>> On Wed, Jan 19, 2011 at 09:34:49PM +0100, Linus Walleij wrote:
>>> 2011/1/18 Russell King - ARM Linux <linux@arm.linux.org.uk>:
>>>
>>> > How reliable is the FIFOCNT register ?
>>>
>>> Hmm... the FIFOCNT is just the FIFO.
>>
>> No, FIFOCNT.
>>
>> ?The MCIFifoCnt register contains the remaining number of words to be
>> ?written to or read from the FIFO.
>
> Yeah I B damned that's right. Sorry for the confusion. I'll respin
> the latest patch on the FIFOCNT register instead and test
> tomorrow.
>
> But if the transfer is complete, MCIDataCnt should be valid
> as well. I'll read the datasheet(s) closer and see if I get some
> useful clues about which one is more reliable in the error
> case.

I've looked into this. It appears AFAICT that the FIFOCNT is
reset to zero whenever Tx or Rx is not active, i.e. on an error. So
this register is not helpful for determining the number of blocks
left at an error, it will always be zero at that point.

MCIDATACNT on the other hand comes from the datapath
state machine which in turn controls the FIFO. It is a totally
separate counter inside the state machine, loaded from the
same length register, but updated from the datapath control
machinery.

The latter counter is valid whenever the datapath state machine
is in IDLE state, and it goes to IDLE when an error occurs.
The register value will not be reset until a new transfer sequence
or reset occurs.

To verify I added this patch to print out the registers:

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 60d0c7e..12b213b 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -518,7 +518,10 @@ mmci_data_irq(struct mmci_host *host, struct
mmc_data *data,
                remain = readl(host->base + MMCIDATACNT) << 2;
                success = data->blksz * data->blocks - remain;

-               dev_dbg(mmc_dev(host->mmc), "MCI ERROR IRQ (status
%08x)\n", status);
+               dev_err(mmc_dev(host->mmc), "MCI ERROR IRQ (status
%08x) lost 0x%08x bytes\n", status, remain);
+               dev_err(mmc_dev(host->mmc), "MMCIDATACNT = 0x%08x\n",
readl(host->base + MMCIDATACNT));
+               dev_err(mmc_dev(host->mmc), "MMCIFIFOCNT = 0x%08x\n",
readl(host->base + MMCIFIFOCNT));
+
                if (status & MCI_DATACRCFAIL) {
                        /* Last block was not successful */
                        host->data_xfered = ((success / data->blksz) -
1 * data->blksz);


Then I started to copy a file and ejected the card in the
middle of the transfer:

root at ME:/mnt cp bar.bin /
mmci-pl18x fpga:mmc0: MCI ERROR IRQ (status 00000002) lost 0x00003000 bytes
mmci-pl18x fpga:mmc0: MMCIDATACNT = 0x00000c00
mmci-pl18x fpga:mmc0: MMCIFIFOCNT = 0x00000000

So the more I look at it, I get the impression that MCIDATACNT
is the register to use.

This means that I think the patch I've sent should be the way
forward.

Yours,
Linus Walleij

[PATCH] mmci: fixup broken_blockend variant patch v2

[Russell King - ARM Linux]

On Fri, Jan 21, 2011 at 01:53:06PM +0100, Linus Walleij wrote:
> This means that I think the patch I've sent should be the way
> forward.

Which particular patch - there seems to be soo many fiddling with
the various broken flags and so forth.

If we're going to move away from the blockend IRQs to using the data
counter, can we have a patch which switches the driver to use the data
counter and then a followup patch which removes the broken blockend
stuff, rather than somethign that changes the broken blockend stuff
and then removes it?

[PATCH] mmci: fixup broken_blockend variant patch v2

[Linus Walleij]

2011/1/23 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Fri, Jan 21, 2011 at 01:53:06PM +0100, Linus Walleij wrote:

> If we're going to move away from the blockend IRQs to using the data
> counter, can we have a patch which switches the driver to use the data
> counter and then a followup patch which removes the broken blockend
> stuff, rather than somethign that changes the broken blockend stuff
> and then removes it?

6631/1 fixes the broken blockend stuff into a state that works,
then 6632/1 removes it in favor of using the MMCIDATACNT.

Then we have 6630/1, which is an unrelated change, fixing a bug
with suspend/resume.

Hope this helps.

Yours,
Linus Walleij

[PATCH] mmci: fixup broken_blockend variant patch v2

[Russell King - ARM Linux]

On Sun, Jan 23, 2011 at 10:40:05PM +0100, Linus Walleij wrote:
> 2011/1/23 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> > On Fri, Jan 21, 2011 at 01:53:06PM +0100, Linus Walleij wrote:
> 
> > If we're going to move away from the blockend IRQs to using the data
> > counter, can we have a patch which switches the driver to use the data
> > counter and then a followup patch which removes the broken blockend
> > stuff, rather than somethign that changes the broken blockend stuff
> > and then removes it?
> 
> 6631/1 fixes the broken blockend stuff into a state that works,
> then 6632/1 removes it in favor of using the MMCIDATACNT.

What I'm saying is why do we need to fix the blockend stuff if in the
next patch we remove it - why not just combine the two patches and
remove the thing in one go as a "we give up using the blockend irq"?

[PATCH] mmci: fixup broken_blockend variant patch v2

[Linus Walleij]

2011/1/24 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Sun, Jan 23, 2011 at 10:40:05PM +0100, Linus Walleij wrote:
>> 2011/1/23 Russell King - ARM Linux <linux@arm.linux.org.uk>:
>> > On Fri, Jan 21, 2011 at 01:53:06PM +0100, Linus Walleij wrote:
>>
>> > If we're going to move away from the blockend IRQs to using the data
>> > counter, can we have a patch which switches the driver to use the data
>> > counter and then a followup patch which removes the broken blockend
>> > stuff, rather than somethign that changes the broken blockend stuff
>> > and then removes it?
>>
>> 6631/1 fixes the broken blockend stuff into a state that works,
>> then 6632/1 removes it in favor of using the MMCIDATACNT.
>
> What I'm saying is why do we need to fix the blockend stuff if in the
> next patch we remove it - why not just combine the two patches and
> remove the thing in one go as a "we give up using the blockend irq"?

OK I get it, no, of course 6631 and 6632 can be squashed. I'll do
this, take out 6631 and have 6632 be the squashed result, will
be fixed in a few minutes.

Thanks
Linus Walleij

[PATCH] mmci: fixup broken_blockend variant patch v2

[Russell King - ARM Linux]

On Mon, Jan 24, 2011 at 09:18:19AM +0100, Linus Walleij wrote:
> 2011/1/24 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> > On Sun, Jan 23, 2011 at 10:40:05PM +0100, Linus Walleij wrote:
> >> 2011/1/23 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> >> > On Fri, Jan 21, 2011 at 01:53:06PM +0100, Linus Walleij wrote:
> >>
> >> > If we're going to move away from the blockend IRQs to using the data
> >> > counter, can we have a patch which switches the driver to use the data
> >> > counter and then a followup patch which removes the broken blockend
> >> > stuff, rather than somethign that changes the broken blockend stuff
> >> > and then removes it?
> >>
> >> 6631/1 fixes the broken blockend stuff into a state that works,
> >> then 6632/1 removes it in favor of using the MMCIDATACNT.
> >
> > What I'm saying is why do we need to fix the blockend stuff if in the
> > next patch we remove it - why not just combine the two patches and
> > remove the thing in one go as a "we give up using the blockend irq"?
> 
> OK I get it, no, of course 6631 and 6632 can be squashed. I'll do
> this, take out 6631 and have 6632 be the squashed result, will
> be fixed in a few minutes.

I think we're getting there - but what about 6630/1 as well?  Is there a
good reason to keep that separate?  After combining those two, I see this,
which doesn't look like a change we need:

@@ -199,7 +191,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 int datactrl, timeout, irqmask0, irqmask1;
        unsigned long long clks;
        void __iomem *base;
        int blksz_bits;
@@ -230,20 +220,20 @@ static void mmci_start_data(struct mmci_host *host, struct mmc_data *data)
        datactrl = MCI_DPSM_ENABLE | blksz_bits << 4;
        if (data->flags & MMC_DATA_READ) {
                datactrl |= MCI_DPSM_DIRECTION;
-               irqmask = MCI_RXFIFOHALFFULLMASK;
+               irqmask1 = MCI_RXFIFOHALFFULLMASK;

                /*
                 * 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 < variant->fifosize)
-                       irqmask |= MCI_RXDATAAVLBLMASK;
+                       irqmask1 |= MCI_RXDATAAVLBLMASK;
        } else {
                /*
                 * We don't actually need to include "FIFO empty" here
                 * since its implicit in "FIFO half empty".
                 */
-               irqmask = MCI_TXFIFOHALFEMPTYMASK;
+               irqmask1 = MCI_TXFIFOHALFEMPTYMASK;
        }

        /* The ST Micro variants has a special bit to enable SDIO */
@@ -252,8 +242,10 @@ static void mmci_start_data(struct mmci_host *host, struct
mmc_data *data)
                        datactrl |= MCI_ST_DPSM_SDIOEN;

        writel(datactrl, base + MMCIDATACTRL);
-       writel(readl(base + MMCIMASK0) & ~MCI_DATAENDMASK, base + MMCIMASK0);
-       mmci_set_mask1(host, irqmask);
+       irqmask0 = readl(base + MMCIMASK0);
+       irqmask0 &= ~MCI_DATAENDMASK;
+       writel(irqmask0, base + MMCIMASK0);
+       mmci_set_mask1(host, irqmask1);
 }

 static void

The rest of the combined patch looks sensible, so maybe 6630/1 should be
combined into 6632/2 as well, and the above changes stripped out?

[PATCH] mmci: fixup broken_blockend variant patch v2

[Linus Walleij]

2011/1/24 Russell King - ARM Linux <linux@arm.linux.org.uk>:

> The rest of the combined patch looks sensible, so maybe 6630/1 should be
> combined into 6632/2 as well, and the above changes stripped out?

OK squashed them, took out the noop code and put in as 6632/3.

Linus Walleij

[PATCH] mmci: fixup broken_blockend variant patch v2

[Russell King - ARM Linux]

On Fri, Jan 21, 2011 at 01:53:06PM +0100, Linus Walleij wrote:
> 2011/1/20 Linus Walleij <linus.ml.walleij@gmail.com>:
> > But if the transfer is complete, MCIDataCnt should be valid
> > as well. I'll read the datasheet(s) closer and see if I get some
> > useful clues about which one is more reliable in the error
> > case.
> 
> I've looked into this. It appears AFAICT that the FIFOCNT is
> reset to zero whenever Tx or Rx is not active, i.e. on an error. So
> this register is not helpful for determining the number of blocks
> left at an error, it will always be zero at that point.
> 
> MCIDATACNT on the other hand comes from the datapath
> state machine which in turn controls the FIFO. It is a totally
> separate counter inside the state machine, loaded from the
> same length register, but updated from the datapath control
> machinery.

Something occurs to me - isn't DATACNT a byte counter rather than a word
counter?  I think that's something which needs to be confirmed.  The TRM
is a little confusing:

| Data counter register, MCIDataCnt The MCIDataCnt register loads the value
| from the data length register (see Data length register, MCIDataLength on
| page 3-11) when the DPSM moves from the IDLE state to the WAIT_R or WAIT_S
| state. As data is transferred, the counter decrements the value until it
| reaches 0.

The first sentence implies that the value which appears here initially
is the value which was written to the data length register, which is a
byte count.  The second sentence _could_ be interpreted as it decrementing
by one each time data is transferred to/from the FIFO (which are 32-bit
words).

I think this requires some experimentation...

[PATCH] mmci: fixup broken_blockend variant patch v2

[Linus Walleij]

2011/1/27 Russell King - ARM Linux <linux@arm.linux.org.uk>:

> Something occurs to me - isn't DATACNT a byte counter rather than a word
> counter?

It is.

> I think that's something which needs to be confirmed. ?The TRM
> is a little confusing: (...)
>
> I think this requires some experimentation...

All experiments I've done by ejecting the card while reading seem to
have the transfer end on thing like 0xc00, 0x800, 0x1000 etc, all
even multiples of 512 bytes.

I have also read the VHDL code, which is shows it is clearly
a byte counter.

Found another bug as well when calculating the number of bytes
left, so sent off a patch.

Linus Walleij