Re: [PATCH 6/7] k3dma: Fix occasional DMA ERR issue by using proper dma api

From: Andy Green
Date: Thu Jul 21 2016 - 15:57:59 EST

On July 22, 2016 12:18:48 AM GMT+08:00, John Stultz <john.stultz@xxxxxxxxxx> wrote:
>On Wed, Jul 20, 2016 at 11:27 PM, Andy Green <andy@xxxxxxxxxxx> wrote:
>> On July 21, 2016 1:22:02 PM GMT+08:00, John Stultz
><john.stultz@xxxxxxxxxx> wrote:
>>>On Wed, Jul 20, 2016 at 9:26 PM, zhangfei <zhangfei.gao@xxxxxxxxxx>
>>>> On 07/21/2016 11:53 AM, John Stultz wrote:
>>>>> After lots of debugging on an occasional DMA ERR issue, I realized
>>>>> that the desc structures which we point the dma hardware are being
>>>>> allocated out of regular memory. This means when we fill the desc
>>>>> structures, that data doesn't always get flushed out to memory by
>>>>> the time we start the dma transfer, resulting in the dma engine
>>>>> some null values, resulting in a DMA ERR on the first irq.
>>>> How about using wmb() flush before start dma to sync desc?
>>>So I'm not going to pretend to be an expert here, but my
>>>is that wmb() syncrhonizes cpu write ordering operations across cpus,
>> IIUI what the memory barrier does is tell the *compiler* to actually
>do any writes that the code asked for, but which otherwise might
>actually be deferred past that point. The compiler doesn't know that
>buffer area has other hardware snooping it, so by default it feels it
>can play tricks with what seems to it like just generally deferring
>spilling registers to memory. wmb makes sure the compiler's pending
>writes actually happen right there. (writel() etc definitions have one
>built-in, so they always do what you asked when you asked).
>> That can be of interest when syncing with other cores but also to
>other IPs that intend to use the written-to area immediately, which is
>what's happening here. Without the barrier the write may not be issued
>anywhere, even to local cpu cache, until after the hw is asked to go
>read the buffer, corrupting what the hw sees.
>>>so the cpus see all the changes before the wmb() before they see any
>>>changes after. But I'm not sure what effect wmb() has across cpu
>>>cache to device ordering. I don't think it works as a cache flush
>>>Andy's patch introducing the cyclic support actually had a wmb() in
>>>that I removed as I couldn't understand clearly why it was there (and
>>>there wasn't a comment explaining, as required by checkpatch :).
>>>even with that wmb(), the DMA ERR was still seen.
>> The rule about the comment is there partially because there's a
>general tendancy for throwing voodoo smbs on broken things in case it
>helps. But writing out memory descriptors that other hw is going to
>read is a legit use for it I think. If the compiler you use wasn't
>deferring the write, you won't notice any difference removing it, but
>that doesn't mean it shouldn't be there.
>Though from your comments above, wouldn't using writel() instead of
>writel_relaxed(), followed by a wmb() be sufficient?

Yes, since on Arm writel() sticks a wmb (ultimately a dsb instruction + outer_sync()) after every write it does.