Hi Ziyuan, On 14 July 2016 at 09:43, Ziyuan Xu <xzy...@rock-chips.com> wrote: > Hi Simon, > > > On 2016年07月14日 23:00, Simon Glass wrote: >> >> On 12 July 2016 at 19:06, Ziyuan Xu <xzy...@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...@rock-chips.com> wrote: >>>>> >>>>> From: Xu Ziyuan <xzy...@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...@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(®->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.
Gosh that is horrible. > Thanks for your review! > > >> >> Reviewed-by: Simon Glass <s...@chromium.org> >> >>>>> + >>>>> 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 >>>> >>>> >>>> >>> >> >> > > Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
