Date:	Fri, 22 Jul 2016 03:57:24 +0800
From:	Andy Green <andy@...mcat.com>
To:	John Stultz <john.stultz@...aro.org>
CC:	zhangfei <zhangfei.gao@...aro.org>,
	lkml <linux-kernel@...r.kernel.org>,
	Jingoo Han <jg1.han@...sung.com>,
	Krzysztof Kozlowski <k.kozlowski@...sung.com>,
	Maxime Ripard <maxime.ripard@...e-electrons.com>,
	Vinod Koul <vinod.koul@...el.com>,
	Dan Williams <dan.j.williams@...el.com>,
	Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH 6/7] k3dma: Fix occasional DMA ERR issue by using proper dma api



On July 22, 2016 12:18:48 AM GMT+08:00, John Stultz <john.stultz@...aro.org> wrote:
>On Wed, Jul 20, 2016 at 11:27 PM, Andy Green <andy@...mcat.com> wrote:
>> On July 21, 2016 1:22:02 PM GMT+08:00, John Stultz
><john.stultz@...aro.org> wrote:
>>>On Wed, Jul 20, 2016 at 9:26 PM, zhangfei <zhangfei.gao@...aro.org>
>>>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?

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

-Andy

>thanks
>-john

Powered by blists - more mailing lists