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

[John Stultz]

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>
>>wrote:
>>>
>>>
>>> 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
>>getting
>>>> 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 understanding
>>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 to
>>memory.
>>
>>Andy's patch introducing the cyclic support actually had a wmb() in it
>>that I removed as I couldn't understand clearly why it was there (and
>>there wasn't a comment explaining, as required by checkpatch :).   But
>>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?

thanks
-john