headers
Rabin Vincent | 1 Jan 2011 12:05
Picon

mmci: U300 "sync with blockend" broken for multi-block?

In MMCI, there is some code to sync between the MCI_DATABLOCKEND and the
MCI_DATAEND for U300 variants, which ensures that the transfer
terminates only when both MCI_DATABLOCKEND and MCI_DATAEND occurs:

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

It seems to me that this code won't work correctly for multi-block
transfers, because there MCI_DATABLOCKEND will hit for the earlier
blocks and the blockend flag will be set, and if on the last block the
MCI_DATABLOCKEND hits after the MCI_DATAEND, this synching code won't do
what it's trying to do and will instead just terminate the transfer
after MCI_DATAEND.
Permalink | Reply |
headers
Russell King - ARM Linux | 1 Jan 2011 13:10
Picon

Re: mmci: U300 "sync with blockend" broken for multi-block?

On Sat, Jan 01, 2011 at 04:35:14PM +0530, Rabin Vincent wrote:
> In MMCI, there is some code to sync between the MCI_DATABLOCKEND and the
> MCI_DATAEND for U300 variants, which ensures that the transfer
> terminates only when both MCI_DATABLOCKEND and MCI_DATAEND occurs:
> 
> 	 * 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.
> 
> It seems to me that this code won't work correctly for multi-block
> transfers, because there MCI_DATABLOCKEND will hit for the earlier
> blocks and the blockend flag will be set, and if on the last block the
> MCI_DATABLOCKEND hits after the MCI_DATAEND, this synching code won't do
> what it's trying to do and will instead just terminate the transfer
> after MCI_DATAEND.

It would be good to characterize what's actually going on with U300 some
more, especially the timing between these signals and the FIFO interrupts,
rather than just stating that they occur "out of order".

Is the data block end interrupt being triggered when you've read the
required data from the FIFO, and the data end interrupt triggered when
the card has transferred the required amount of data (iow, data into
the FIFO)?

Once they have been properly characterized, then it may be possible to
come up with an alternative solution.  At the moment, it's very had to
guess what's going on from the descriptions given.
(Continue reading)

Permalink | Reply |
headers
Linus Walleij | 5 Jan 2011 17:15
Picon

Re: mmci: U300 "sync with blockend" broken for multi-block?

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

> It would be good to characterize what's actually going on with U300 some
> more, especially the timing between these signals and the FIFO interrupts,
> rather than just stating that they occur "out of order".

I will try to document more closely. OTOMH it was like
for reads they would come in one order first one then
another and for writes the other way around. That was
why the older quirk for U300 was working, wiring the
DATAEND high, though it was no good in modeling
what was actually happening.

> Is the data block end interrupt being triggered when you've read the
> required data from the FIFO, and the data end interrupt triggered when
> the card has transferred the required amount of data (iow, data into
> the FIFO)?
>
> Once they have been properly characterized, then it may be possible to
> come up with an alternative solution.  At the moment, it's very had to
> guess what's going on from the descriptions given.

Ulf, do you know the details of what is happening here?
I think you have the most up-to-date knowledge.

I've been trying to determine this a number of times,
empirically mostly, probably failing to understand the
most important variables. The current solution is as
far as I've been able to model what's actually happening.
(Continue reading)

Permalink | Reply |
headers
Russell King - ARM Linux | 5 Jan 2011 17:43
Picon

Re: mmci: U300 "sync with blockend" broken for multi-block?

On Wed, Jan 05, 2011 at 05:15:04PM +0100, Linus Walleij wrote:
> 2011/1/1 Russell King - ARM Linux <linux <at> arm.linux.org.uk>:
> 
> > It would be good to characterize what's actually going on with U300 some
> > more, especially the timing between these signals and the FIFO interrupts,
> > rather than just stating that they occur "out of order".
> 
> I will try to document more closely. OTOMH it was like
> for reads they would come in one order first one then
> another and for writes the other way around. That was
> why the older quirk for U300 was working, wiring the
> DATAEND high, though it was no good in modeling
> what was actually happening.

Any chance of pr_debug'ing the complete status register each time you
service an interrupt?  You'll probably need to set the kernel log
buffer fairly large to ensure that you capture everything.

I wouldn't recommend dumping the messages through the serial port
directly as that'd put far too much latency on the servicing of them,
but using dmesg after the accesses to retrieve them.  (IOW, don't
pass 'debug' to the kernel...)
Permalink | Reply |
headers
Linus Walleij | 14 Jan 2011 21:13
Picon

Re: mmci: U300 "sync with blockend" broken for multi-block?

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

> Any chance of pr_debug'ing the complete status register each time you
> service an interrupt?  You'll probably need to set the kernel log
> buffer fairly large to ensure that you capture everything.

I did this test now.

Typical read/write test:
dd if=/dev/mmcblk0 of=/dev/null bs=16384 count=16 of=/dev/null
dd if=/dev/zero of=/dev/mmcblk0 bs=16384 count=16

The MCI_DATABLOCKENDMASK is set in both modes so it should appear
after every block (512 bytes) no matter whether you're using DMA
or not. In this case you would expect 0x80 x MCI_DATABLOCKEND
followed by MCI_DATAEND, repeated 16 times.

But this is what happens:

TEST WITH PIO ON U8500 READ:
mmci-pl18x sdi2: START DATA: blksz 0200 blks 0018 flags 00000200
mmci-pl18x sdi2: MCI_DATABLOCKEND && MCI_DATAEND
mmci-pl18x sdi2: START DATA: blksz 0200 blks 0028 flags 00000200
mmci-pl18x sdi2: MCI_DATABLOCKEND && MCI_DATAEND
mmci-pl18x sdi2: START DATA: blksz 0200 blks 0080 flags 00000200
mmci-pl18x sdi2: MCI_DATABLOCKEND && MCI_DATAEND
mmci-pl18x sdi2: START DATA: blksz 0200 blks 0080 flags 00000200
mmci-pl18x sdi2: MCI_DATABLOCKEND && MCI_DATAEND
mmci-pl18x sdi2: START DATA: blksz 0200 blks 0080 flags 00000200
mmci-pl18x sdi2: MCI_DATABLOCKEND && MCI_DATAEND
(Continue reading)

Permalink | Reply |
headers
Russell King - ARM Linux | 14 Jan 2011 23:44
Picon

Re: mmci: U300 "sync with blockend" broken for multi-block?

On Fri, Jan 14, 2011 at 09:13:05PM +0100, Linus Walleij wrote:
> 2011/1/5 Russell King - ARM Linux <linux <at> arm.linux.org.uk>:
> 
> > Any chance of pr_debug'ing the complete status register each time you
> > service an interrupt?  You'll probably need to set the kernel log
> > buffer fairly large to ensure that you capture everything.
> 
> I did this test now.

Just to complete the picture, can I see the patch between the version
which produced this and mainline?
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Permalink | Reply |
headers
Linus Walleij | 16 Jan 2011 22:11
Picon

Re: mmci: U300 "sync with blockend" broken for multi-block?

2011/1/14 Russell King - ARM Linux <linux <at> arm.linux.org.uk>:
> On Fri, Jan 14, 2011 at 09:13:05PM +0100, Linus Walleij wrote:
>> 2011/1/5 Russell King - ARM Linux <linux <at> arm.linux.org.uk>:
>>
>> > Any chance of pr_debug'ing the complete status register each time you
>> > service an interrupt?  You'll probably need to set the kernel log
>> > buffer fairly large to ensure that you capture everything.
>>
>> I did this test now.
>
> Just to complete the picture, can I see the patch between the version
> which produced this and mainline?

Sure, but I patched on top of the pending DMA-patches, since I
needed to test the workings in DMA mode. Here is that patch, if
you want the entire DMA patch series too I can resend them:

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index af0cae9..8ae32d9 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
 <at>  <at>  -81,7 +81,7  <at>  <at>  static struct variant_data variant_u300 = {
        .fifohalfsize           = 8 * 4,
        .clkreg_enable          = 1 << 13, /* HWFCEN */
        .datalength_bits        = 16,
-       .broken_blockend        = true,
+       .broken_blockend        = false,
        .sdio                   = true,
 };
(Continue reading)

Permalink | Reply |
headers
Linus Walleij | 16 Jan 2011 22:16
Picon

Re: mmci: U300 "sync with blockend" broken for multi-block?

2011/1/14 Linus Walleij <linus.ml.walleij <at> gmail.com>:

> What it problematic with the reads *and* writes is that
> sometimes there us a MCI_DATABLOCKEND missing, so the
> blocks simply don't add up! This is the real bug that
> the sync code was trying to solve, and not in a very
> good way.

Here is a follow-up explanation as to why the code as it
is today actually works: the sync code has a bug. It is
intended to check whether the *last* blockend interrupt
*and* the dataend interrupt has occurred. However it
actually checks whether *any* blockend interrupt and
the datend interrupt has occured.

Thus it works, with the side effect of ACK:ing multiblock
transfers on U300 while the host->data_xfered value is lower
that what was requested - the blocks have been read
but not all are accounted for, because there are blockend
IRQs missing.

I'll follow up with a patch fixing the *real* issue, in an
atleast a little more elegant way :-/

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
(Continue reading)

Permalink | Reply |
headers
Linus Walleij | 16 Jan 2011 23:09
Picon

Re: mmci: U300 "sync with blockend" broken for multi-block?

...and to round off a theory of why the U300 and Ux500 is
missing interrupts:

Maybe the IP-block does not really handle the case of a block
interrupt not being ACK:ed before the next block is ready. So
it will just fire another "1" flag, which gets ACK:ed at the end
of the interrupt handler with the IRQ currently being processed.

So the interrupt handler will unknowingly consume interrupts
for other blocks.

The right way would be to either:

- Queue block ACKs so that they are ACK:ed one at the time
  if the hardware reads ahead. (Which requires a quite deep
  queue on large writes.)

- Hold back further block reading from the card until the IRQ
  has been ACK:ed. (Which is not good for speed.)

So to avoid both it is indeed a logical thing to remove the
block interrupt altogether and just wait for the last one to
avoid trouble, it's just not documented and
HW-implemented as such.

Now I'll send that patch.

Yours,
Linus Walleij
--
(Continue reading)

Permalink | Reply |
headers
Linus Walleij | 5 Jan 2011 17:02
Picon

Re: mmci: U300 "sync with blockend" broken for multi-block?

2011/1/1 Rabin Vincent <rabin <at> rab.in>:

> In MMCI, there is some code to sync between the MCI_DATABLOCKEND and the
> MCI_DATAEND for U300 variants, which ensures that the transfer
> terminates only when both MCI_DATABLOCKEND and MCI_DATAEND occurs:
>
>         * 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.
>
> It seems to me that this code won't work correctly for multi-block
> transfers, because there MCI_DATABLOCKEND will hit for the earlier
> blocks and the blockend flag will be set, and if on the last block the
> MCI_DATABLOCKEND hits after the MCI_DATAEND, this synching code won't do
> what it's trying to do and will instead just terminate the transfer
> after MCI_DATAEND.

Yes Ulf Hansson has spotted this problem...

Actually, there is this patch in the public ST-Ericsson git:
http://git.linaro.org/gitweb?p=bsp/st-ericsson/linux-2.6.35-ux500.git;a=commitdiff;h=2f0534d9527540a0a28e124f4e827f146f3bc128

The intention is to send out patches ASAP right now the
vacations have been holding things back a bit. (Else I
would have done it myself.)

Ulf would you like to submit this patch to the maillist?
Acked-by: Linus Walleij <linus.walleij <at> stericsson.com>
(Continue reading)

Permalink | Reply |

Gmane