mmci: sync DATAEND irq with dma transfer done

From: Ulf Hansson <[email protected]>

The end of a dma job must be synced with a DATAEND irq. This will prevent the mmci driver from ending the mmc request before the dma driver is completely done. By using DMA_PREP_INTERRUPT we register a callback function which will is called when from the dma driver when the dma transfer is completed.

Quoted 7 lines · Linus Walleij

From: Ulf Hansson <[email protected]>

The end of a dma job must be synced with a DATAEND irq. This will prevent the mmci driver from ending the mmc request before the dma driver is completely done. By using DMA_PREP_INTERRUPT we register a callback function which will is called when from the dma driver when the dma transfer is completed.

Why is what we currently do not sufficient?

This will break the detection of dead DMA controllers like on the ARM reference platforms, as I've said before.

Quoted 9 lines · Russell King - ARM Linux

Quoted 7 lines · Linus Walleij

From: Ulf Hansson <[email protected]>

The end of a dma job must be synced with a DATAEND irq. This will prevent the mmci driver from ending the mmc request before the dma driver is completely done. By using DMA_PREP_INTERRUPT we register a callback function which will is called when from the dma driver when the dma transfer is completed.

Quoted 1 line

Why is what we currently do not sufficient?

On a high-speeded ux500 the DATAEND IRQ will assert before the DMA data is actually finished, thus if we start hammering in the next request we break an ongoing transfer. :-(

Quoted 4 lines · Linus Walleij

On a high-speeded ux500 the DATAEND IRQ will assert before the DMA data is actually finished, thus if we start hammering in the next request we break an ongoing transfer. :-(

Yes, you've already said that in the past. ?And this is partly why we have this code in the dma unmap:

? ? ? ?/* Wait up to 1ms for the DMA to complete */
? ? ? ?for (i = 0; ; i++) {
? ? ? ? ? ? ? ?status = readl(host->base + MMCISTATUS); ? ? ? ? ? ? ? ?if (!(status & MCI_RXDATAAVLBLMASK) || i >= 100) ? ? ? ? ? ? ? ? ? ? ? ?break;
? ? ? ? ? ? ? ?udelay(10);
? ? ? ?}

So, we wait until the DMA has drained the FIFO before we fire off the next request - or even unmap the DMA buffer. ?Should the DMA fail to drain the FIFO in a reasonable time, we timeout and disable DMA.

Again, I ask, why is this not sufficient to cover the case where the data end IRQ occurs before the DMA engine has completed the transfer - which is likely to take a very short time indeed.

My rough guess (after looking at the VHDL code) is that RXDATAVLBL flag goes low when the FIFO is empty, but that doesn't mean that the DMA handshake logic is out of its send/recieve state and thus we screw it up if we hammer in another transfer before it has had time to deassert the single/burst request signals and go to idle state. This can only be seen by the side effect of the DMA transfer actually terminating, and the DMA engine calling its callback.

Quoted 9 lines

My rough guess (after looking at the VHDL code) is that RXDATAVLBL flag goes low when the FIFO is empty, but that doesn't mean that the DMA handshake logic is out of its send/recieve state and thus we screw it up if we hammer in another transfer before it has had time to deassert the single/burst request signals and go to idle state. This can only be seen by the side effect of the DMA transfer actually terminating, and the DMA engine calling its callback.

From what you are saying, RXDATAAVLBL seems to be equal to !RXFIFOEMPTY in your case.

Did you try to change the former to the
latter to see how it works?

What we need is a DMAXFERCOMPLETE flag so we know that the DMA state machine is really finished. Sadly the hardware lacks such a status flag, so there is no way to know that the DMA state machine is finished by looking at any status registers in the MMCI block.

We are thus reliant on the other end of the bitpipe telling us it's finished.

Okay, but can't we have something like this then in the unmap function:

? ? ? for (i = 0; i < 100; i++) {
? ? ? ? ? ? ? status = readl(host->base + MMCISTATUS); ? ? ? ? ? ? ? if (host->unreliable_rxflags) {
? ? ? ? ? ? ? ? ? ? ? if (host->dataend)
? ? ? ? ? ? ? ? ? ? ? ? ? ? ?break;
? ? ? ? ? ? ? } else if (!(status & MCI_RXDATAAVLBLMASK)) ? ? ? ? ? ? ? ? ? ? ? break;
? ? ? ? ? ? ? udelay(10);
? ? ? }

to minimize the changes and apparently address Russell's concern as well?

Quoted 17 lines · Russell King - ARM Linux

Quoted 4 lines · Linus Walleij

On a high-speeded ux500 the DATAEND IRQ will assert before the DMA data is actually finished, thus if we start hammering in the next request we break an ongoing transfer. :-(

Yes, you've already said that in the past. ?And this is partly why we have this code in the dma unmap:

? ? ? ?/* Wait up to 1ms for the DMA to complete */
? ? ? ?for (i = 0; ; i++) {
? ? ? ? ? ? ? ?status = readl(host->base + MMCISTATUS); ? ? ? ? ? ? ? ?if (!(status & MCI_RXDATAAVLBLMASK) || i >= 100) ? ? ? ? ? ? ? ? ? ? ? ?break;
? ? ? ? ? ? ? ?udelay(10);
? ? ? ?}

Quoted 7 lines

So, we wait until the DMA has drained the FIFO before we fire off the next request - or even unmap the DMA buffer. ?Should the DMA fail to drain the FIFO in a reasonable time, we timeout and disable DMA.

Again, I ask, why is this not sufficient to cover the case where the data end IRQ occurs before the DMA engine has completed the transfer - which is likely to take a very short time indeed.

It doesn't help, we have really tested this and at high speed transfers (especially if we use Per Fridens speed-up patches) apparently the flag RXDATAAVLBL goes to zero before the block is really finished.

My rough guess (after looking at the VHDL code) is that RXDATAVLBL flag goes low when the FIFO is empty, but that doesn't mean that the DMA handshake logic is out of its send/recieve state and thus we screw it up if we hammer in another transfer before it has had time to deassert the single/burst request signals and go to idle state. This can only be seen by the side effect of the DMA transfer actually terminating, and the DMA engine calling its callback.

Quoted 32 lines · Linus Walleij

Quoted 17 lines · Russell King - ARM Linux

Quoted 4 lines · Linus Walleij

On a high-speeded ux500 the DATAEND IRQ will assert before the DMA data is actually finished, thus if we start hammering in the next request we break an ongoing transfer. :-(

Yes, you've already said that in the past. And this is partly why we have this code in the dma unmap:

       /* Wait up to 1ms for the DMA to complete */
       for (i = 0; ; i++) {
               status = readl(host->base + MMCISTATUS);
               if (!(status & MCI_RXDATAAVLBLMASK) || i >= 100)
                       break;
               udelay(10);
       }

Quoted 7 lines

So, we wait until the DMA has drained the FIFO before we fire off the next request - or even unmap the DMA buffer. Should the DMA fail to drain the FIFO in a reasonable time, we timeout and disable DMA.

Again, I ask, why is this not sufficient to cover the case where the data end IRQ occurs before the DMA engine has completed the transfer - which is likely to take a very short time indeed.

It doesn't help, we have really tested this and at high speed transfers (especially if we use Per Fridens speed-up patches) apparently the flag RXDATAAVLBL goes to zero before the block is really finished.

Quoted 8 lines

My rough guess (after looking at the VHDL code) is that RXDATAVLBL flag goes low when the FIFO is empty, but that doesn't mean that the DMA handshake logic is out of its send/recieve state and thus we screw it up if we hammer in another transfer before it has had time to deassert the single/burst request signals and go to idle state. This can only be seen by the side effect of the DMA transfer actually terminating, and the DMA engine calling its callback.

That's rather unfortunate, because it means that trying it on ARM hardware is going to hang indefinitely waiting for the nonexistent DMA stuff to finish.

I remain unconvinced whether this problem applies only to ARMs evaluation boards as I believe the whole primecell DMA stuff from the outset is fundamentally misdesigned. I suspect there maybe SoCs out there which suffer from the same broken DMA issues which ARMs eval boards do.

Maybe an alternative solution is on data end to set a timer, which is cancelled when the DMA engine callback arrives. If the timer expires, it means we have broken DMA and that needs to be shutdown for that instance.

However, one thing worries me - what if the DMA callback comes before we get the data end interrupt. Given the weirdnesses of your implementation found so far (which are well beyond what's visible on ARMs own implementation) I wouldn't put any guarantees on the relative ordering of that either.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Even this would be no guarantee. The callback is hopefully a guarantee that the clr has been asserted, but for the peripheral's state machine there is still an uncertain time left, so the callback is a guess as good as any other, although the reference point has been shifted forwards. Since the time is uncertain, might just add a suitable delay. Preferably based on VHDL analysis and as a function of peripheral clock.

At the very least the last data element should be out of the FIFO when RxDataAvlbl deasserts

and a read of status should not arrive any
sooner than that element to the DMAC.

If this is true, then the read
of status itself implies that the DMAC has asserted clr (or in some very pessimistic scenario within a few cycles?),

That's rather unfortunate, because it means that trying it on ARM hardware is going to hang indefinitely waiting for the nonexistent DMA stuff to finish.

I remain unconvinced whether this problem applies only to ARMs evaluation boards as I believe the whole primecell DMA stuff from the outset is fundamentally misdesigned. I suspect there maybe SoCs out there which suffer from the same broken DMA issues which ARMs eval boards do.

Maybe an alternative solution is on data end to set a timer, which is cancelled when the DMA engine callback arrives. If the timer expires, it means we have broken DMA and that needs to be shutdown for that instance.

However, one thing worries me - what if the DMA callback comes before we get the data end interrupt. Given the weirdnesses of your implementation found so far (which are well beyond what's visible on ARMs own implementation) I wouldn't put any guarantees on the relative ordering of that either.

Quoted 4 lines

That's rather unfortunate, because it means that trying it on ARM hardware is going to hang indefinitely waiting for the nonexistent DMA stuff to finish.

I see the problem, we need a way of being able to switch between using the dma callback and not using it. I think the variant data should be used for this, what do you think?

host->dataend and host->size==0 controls whether the data transfer has finished successfully. I believe this should be handled correctly in my patch. Maybe it is possible to make some minor restructuring to make it more clear what the end condition really is, I can see if I can figure something out.

Quoted 6 lines · Ulf Hansson

Quoted 3 lines

That's rather unfortunate, because it means that trying it on ARM hardware is going to hang indefinitely waiting for the nonexistent DMA stuff to finish.

I see the problem, we need a way of being able to switch between using the dma callback and not using it. I think the variant data should be used for this, what do you think?

How can we do that when it actually depends on how the primecell is wired and the characteristics of the DMA controller to which it is connected?

Quoted 6 lines

host->dataend and host->size==0 controls whether the data transfer has finished successfully. I believe this should be handled correctly in my patch. Maybe it is possible to make some minor restructuring to make it more clear what the end condition really is, I can see if I can figure something out.

host->size never goes to zero when DMA is in progress, as it doesn't track the progress of the transfer. That's only tracked with PIO, and the remainder is calculated on error from the primecells counters.

Quoted 21 lines · Russell King - ARM Linux

Quoted 4 lines · Linus Walleij

On a high-speeded ux500 the DATAEND IRQ will assert before the DMA data is actually finished, thus if we start hammering in the next request we break an ongoing transfer. :-(

Yes, you've already said that in the past. ?And this is partly why we have this code in the dma unmap:

? ? ? ?/* Wait up to 1ms for the DMA to complete */
? ? ? ?for (i = 0; ; i++) {
? ? ? ? ? ? ? ?status = readl(host->base + MMCISTATUS); ? ? ? ? ? ? ? ?if (!(status & MCI_RXDATAAVLBLMASK) || i >= 100) ? ? ? ? ? ? ? ? ? ? ? ?break;
? ? ? ? ? ? ? ?udelay(10);
? ? ? ?}

So, we wait until the DMA has drained the FIFO before we fire off the next request - or even unmap the DMA buffer. ?Should the DMA fail to drain the FIFO in a reasonable time, we timeout and disable DMA.

Quoted 3 lines

Again, I ask, why is this not sufficient to cover the case where the data end IRQ occurs before the DMA engine has completed the transfer - which is likely to take a very short time indeed.

It doesn't help, we have really tested this and at high speed transfers (especially if we use Per Fridens speed-up patches) apparently the flag RXDATAAVLBL goes to zero before the block is really finished.

Yours,
Linus Walleij

I am willingly to test this dma race condition if there are any doubts whether it still exists or not.