Skip to contentSkip to conversation

mmci: fixup broken_blockend variant patch v2

PATCH New Linus Walleij 5 participants 20 messages Jan 17, 2011 – Jan 27, 2011

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 mmci: fixup broken_blockend variant patch v2

From: Ulf Hansson <[email protected]>

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 <[email protected]>
Cc Rabin Vincent <[email protected]>
[Tweaking and adding err message]

---
v2: also updated the comment.

---
 drivers/mmc/host/mmci.c |   93 +++++++++++++++++++++++++----------------------
 drivers/mmc/host/mmci.h |    4 +-
 2 files changed, 52 insertions(+), 45 deletions(-)
drivers/mmc/host/mmci.c +50 -43
index 0b4a5bf..7e0d38c 100644
@@ -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);
 
drivers/mmc/host/mmci.h +2 -2
index df06f01..7ac8c4d 100644
@@ -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 */
Signature · 1 line
1.7.3.2
#1 lore raw

PATCH 1/1 mmci: fixup broken_blockend variant patch v2

Quoted 7 lines · Linus Walleij

+ ? ? ? ?* 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?
#2 lore raw

PATCH 1/1 mmci: fixup broken_blockend variant patch v2

Quoted 14 lines · Rabin Vincent

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

Quoted 8 lines · Linus Walleij
drivers/mmc/host/mmci.c +7 -0
+        * 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:

Quoted 8 lines

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

#3 lore raw

PATCH 1/1 mmci: fixup broken_blockend variant patch v2

Quoted 5 lines · Linus Walleij

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.

Quoted 8 lines

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.

#4 lore raw

PATCH 1/1 mmci: fixup broken_blockend variant patch v2

2011/1/18 Russell King - ARM Linux <[email protected]>:

Quoted 8 lines

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.

Quoted 1 line

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

#5 lore raw

PATCH 1/1 mmci: fixup broken_blockend variant patch v2

Quoted 4 lines · Linus Walleij

2011/1/18 Russell King - ARM Linux <[email protected]>:

Quoted 2 lines

How reliable is the FIFOCNT register ?

Quoted 8 lines
Quoted 6 lines

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.

#6 lore raw

PATCH 1/1 mmci: fixup broken_blockend variant patch v2

2011/1/19 Russell King - ARM Linux <[email protected]>:

Quoted 11 lines · Linus Walleij
Quoted 5 lines

2011/1/18 Russell King - ARM Linux <[email protected]>:

Quoted 1 line

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

#7 lore raw

PATCH 1/1 mmci: fixup broken_blockend variant patch v2

2011/1/20 Linus Walleij <[email protected]>:

Quoted 13 lines

2011/1/19 Russell King - ARM Linux <[email protected]>:

Quoted 11 lines · Linus Walleij
Quoted 5 lines

2011/1/18 Russell King - ARM Linux <[email protected]>:

Quoted 1 line

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.

Quoted 8 lines

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:

drivers/mmc/host/mmci.c +0 -0
index 60d0c7e..12b213b 100644
@@ -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

#8 lore raw

PATCH 1/1 mmci: fixup broken_blockend variant patch v2

Quoted 2 lines · Linus Walleij

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?

#9 lore raw

PATCH 1/1 mmci: fixup broken_blockend variant patch v2

2011/1/23 Russell King - ARM Linux <[email protected]>:

Quoted 7 lines · Linus Walleij

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

#10 lore raw

PATCH 1/1 mmci: fixup broken_blockend variant patch v2

Quoted 11 lines · Linus Walleij

2011/1/23 Russell King - ARM Linux <[email protected]>:

Quoted 7 lines · Linus Walleij

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"?

#11 lore raw

PATCH 1/1 mmci: fixup broken_blockend variant patch v2

2011/1/24 Russell King - ARM Linux <[email protected]>:

Quoted 10 lines · Linus Walleij
Quoted 9 lines

2011/1/23 Russell King - ARM Linux <[email protected]>:

Quoted 7 lines · Linus Walleij

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?

Quoted 6 lines
Quoted 2 lines

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

#12 lore raw

PATCH 1/1 mmci: fixup broken_blockend variant patch v2

Quoted 14 lines · Linus Walleij

2011/1/24 Russell King - ARM Linux <[email protected]>:

Quoted 13 lines · Linus Walleij
Quoted 11 lines

2011/1/23 Russell King - ARM Linux <[email protected]>:

Quoted 7 lines · Linus Walleij

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.

Quoted 7 lines
Quoted 3 lines

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?

#13 lore raw

PATCH 1/1 mmci: fixup broken_blockend variant patch v2

2011/1/24 Russell King - ARM Linux <[email protected]>:

Quoted 2 lines

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

#14 lore raw

PATCH 1/1 mmci: fixup broken_blockend variant patch v2

In reply to Linus Walleij
Quoted 11 lines · Linus Walleij

2011/1/20 Linus Walleij <[email protected]>:

Quoted 4 lines

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.

Quoted 5 lines

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...

#15 lore raw

PATCH 1/1 mmci: fixup broken_blockend variant patch v2

2011/1/27 Russell King - ARM Linux <[email protected]>:

Quoted 2 lines

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

It is.

Quoted 4 lines

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

#16 lore raw

PATCH 1/1 mmci: fixup broken_blockend variant patch v2

In reply to Linus Walleij
Quoted 5 lines · Linus Walleij

- ? ? ? ?* 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?

Quoted 2 lines · Linus Walleij

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.

Quoted 4 lines

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

Quoted 8 lines

+ ? ? ? 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?

#17 lore raw

PATCH 1/1 mmci: fixup broken_blockend variant patch v2

Quoted 9 lines · Rabin Vincent

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

Quoted 6 lines · Linus Walleij
drivers/mmc/host/mmci.c +0 -5
-        * 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.
Quoted 4 lines

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.

Quoted 9 lines
Quoted 3 lines · Linus Walleij

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.

Quoted 13 lines

drivers/mmc/host/mmci.c static void mmci_start_data(struct mmci_host *host, struct mmc_data *data) · line 260

Quoted 13 lines
        writel(datactrl, base + MMCIDATACTRL);
drivers/mmc/host/mmci.c +8 -2
-       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);
  }
Quoted 3 lines

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

#18 lore raw

PATCH 1/1 mmci: fixup broken_blockend variant patch v2

Quoted 17 lines · Linus Walleij
Quoted 16 lines · Rabin Vincent
Quoted 14 lines · On Mon, Jan 17, 2011 at 20:07, Linus Walleij <[email protected]>…

? ? ? ?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);
?}

Quoted 5 lines
Quoted 3 lines

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)
|
#19 lore raw

PATCH 1/1 mmci: fixup broken_blockend variant patch v2

2011/1/17 Rabin Vincent <[email protected]>:

Quoted 18 lines

On Mon, Jan 17, 2011 at 21:26, Linus Walleij

Quoted 2 lines

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
Quoted 8 lines
| --- 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