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

[Simon Glass]

On 12 July 2016 at 19:06, Ziyuan Xu <xzy.xu at 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 at rock-chips.com> wrote:
>>>
>>> From: Xu Ziyuan <xzy.xu at 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 at 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.

Reviewed-by: Simon Glass <sjg at 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
>>
>>
>>
>
>