From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Thu, 14 Jul 2016 09:00:06 -0600 Subject: [U-Boot] [PATCH v3 4/4] usb: dwc2: invalidate dcache before starting DMA In-Reply-To: <5785940F.5090008@rock-chips.com> References: <1467797663-16276-1-git-send-email-xzy.xu@rock-chips.com> <1467797663-16276-5-git-send-email-xzy.xu@rock-chips.com> <5785940F.5090008@rock-chips.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 12 July 2016 at 19:06, Ziyuan Xu wrote: > > > On 2016?07?12? 20:59, Simon Glass wrote: >> >> Hi Ziyuan, >> >> On 6 July 2016 at 03:34, Ziyuan Xu wrote: >>> >>> From: Xu Ziyuan >>> >>> 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 >>> --- >>> >>> 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(®->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 >> >>> + >>> writel((unsigned int) ep->dma_buf, >>> ®->out_endp[ep_num].doepdma); >>> writel(DOEPT_SIZ_PKT_CNT(pktcnt) | DOEPT_SIZ_XFER_SIZE(length), >>> ®->out_endp[ep_num].doeptsiz); >>> -- >>> 1.9.1 >>> >>> >> Regards, >> Simon >> >> >> > >