[U-Boot] [PATCH v3 4/4] usb: dwc2: invalidate dcache before starting DMA

[Ziyuan Xu <xzy.xu@rock-chips.com>]

Hi Simon,

On 2016?07?14? 23:00, Simon Glass wrote:
> On 12 July 2016 at 19:06, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
>>
>> On 2016?07?12? 20:59, Simon Glass wrote:
>>> Hi Ziyuan,
>>>
>>> On 6 July 2016 at 03:34, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
>>>> From: Xu Ziyuan <xzy.xu@rock-chips.com>
>>>>
>>>> Invalidate dcache before starting the DMA to ensure coherency. In case
>>>> there are any dirty lines from the DMA buffer in the cache, subsequent
>>>> cache-line replacements may corrupt the buffer in memory while the DMA
>>>> is still going on. Cache-line replacement can happen if the CPU tries to
>>>> bring some other memory locations into the cache while the DMA is going
>>>> on.
>>>>
>>>> Signed-off-by: Ziyuan Xu <xzy.xu@rock-chips.com>
>>>> ---
>>>>
>>>> Changes in v3:
>>>> - New commit since v3 to fix the coherence issue between memory and
>>>> cache
>>>>
>>>> Changes in v2: None
>>>>
>>>>    drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c | 3 +++
>>>>    1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>>> b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>>> index 12f5c85..0d6d2fb 100644
>>>> --- a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>>> +++ b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>>> @@ -110,6 +110,9 @@ static int setdma_rx(struct dwc2_ep *ep, struct
>>>> dwc2_request *req)
>>>>
>>>>           ctrl =  readl(&reg->out_endp[ep_num].doepctl);
>>>>
>>>> +       invalidate_dcache_range((unsigned long) ep->dma_buf,
>>>> +                               (unsigned long) ep->dma_buf + ep->len);
>>> There is an invalidate in complete_rx() which is one of the callers
>>> for this function. It seems to me that we should not have this in two
>>> places. Why do we have this problem? Is it because the other calls to
>>> setdma_rx() don't invalidate?
>>
>> Yup, invoke invalidate method twice during one complete transmission:
>> 1- invalidate in setdma_rx() in case of  the write back cache, present from
>> cache-line replacement after DMA transmission complete.
>> i.e.
>> 1) dma_buf = "123456789abcd";
>> 2) didn't invalidate in setdma_rx()
>> 3) DMA complete interrupt coming
>> 4) invalidate in complete_rx()
>> 5) read dma_buf, it's "123456789abcd"
>>
>> If invalidate in step 2, we will achieve correct data.
>> I think it's necessary to invalidate before starting DMA, and
>> doc/README.arm-caches describe  details.
>> 2- invalidate in complete_rx(), cpu read the dma_buf from memory directly.
>>
>>> I think the invalidate should happen just before reading the data. Can
>>> you please check if the other invalidate is needed? Also see how it
>>> cache-aligns the end address.
>> memalign(CONFIG_SYS_CACHELINE_SIZE, EP_BUFFER_SIZE);
>> cache-aligns: 64 bytes.
>> dma_buffer size: 4096
>>
>> I had check cache-aligins and end address, rightful.
>> Furthermore, everything works fine with noncached_alloc().
>>
> OK, thanks for the details. Can the invalidate in (4) be dropped? We
> should only need one invalidate.
We also need invalidate in after  DMA transfer completed, because in 
some processors memory contents can spontaneouslycome to the cache due 
to speculative memory access by the CPU. If this happens with the DMA 
buffer while DMA is going on we have a coherency problem.
Thanks for your review!

>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
>>>> +
>>>>           writel((unsigned int) ep->dma_buf,
>>>> &reg->out_endp[ep_num].doepdma);
>>>>           writel(DOEPT_SIZ_PKT_CNT(pktcnt) | DOEPT_SIZ_XFER_SIZE(length),
>>>>                  &reg->out_endp[ep_num].doeptsiz);
>>>> --
>>>> 1.9.1
>>>>
>>>>
>>> Regards,
>>> Simon
>>>
>>>
>>>
>>
>
>