[U-Boot] [PATCH 01/10] usb:udc:s3c: Reduce dcache invalidate range for UDC receive buffer

[Lukasz Majewski]

Hi Marek,

> Dear Lukasz Majewski,
> 
> > Hi Marek,
> > 
> > > Dear Lukasz Majewski,
> > > 
> > > > Hi Marek,
> > > > 
> > > > > Dear Lukasz Majewski,
> > > > > 
> > > > > > The s3c udc driver sends data in a max packet size.
> > > > > > Therefore the dcache invalidate range shall be equal to max
> > > > > > packet, not the entire DMA_BUFFER_SIZE.
> > > > > > 
> > > > > > Signed-off-by: Lukasz Majewski <l.majewski at samsung.com>
> > > > > > Cc: Marek Vasut <marex at denx.de>
> > > > > > ---
> > > > > > 
> > > > > >  drivers/usb/gadget/s3c_udc_otg_xfer_dma.c |    2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/usb/gadget/s3c_udc_otg_xfer_dma.c
> > > > > > b/drivers/usb/gadget/s3c_udc_otg_xfer_dma.c index
> > > > > > d7af5e9..5e3ba76 100644 ---
> > > > > > a/drivers/usb/gadget/s3c_udc_otg_xfer_dma.c +++
> > > > > > b/drivers/usb/gadget/s3c_udc_otg_xfer_dma.c @@ -117,7
> > > > > > +117,7 @@ static int setdma_rx(struct s3c_ep *ep, struct
> > > > > > s3c_request *req)
> > > > > > 
> > > > > >  	invalidate_dcache_range((unsigned long)
> > > > > > 
> > > > > > ep->dev->dma_buf[ep_num], (unsigned long)
> > > > > > ep->dev->dma_buf[ep_num]
> > > > > > -				+ DMA_BUFFER_SIZE);
> > > > > > +				+ ep->ep.maxpacket);
> > > > > 
> > > > > Is this maxpacket _always_ multiple of cacheline big or will
> > > > > you need some ROUND_UP() call here ?
> > > > 
> > > > The maxpacket value is equal to 64 B for EP0 and 512 B for EP1
> > > > and EP2 (wMaxPacketSize field of the descriptor).
> > > > 
> > > > Moreover this invalidation is done on already memaligned buffer
> > > > (which is 16 KiB). In other words, we are copying data to
> > > > specially prepared buffer (per UDC device EP, not usb_request).
> > > > 
> > > > In my opinion the maxpacket don't need to be rounded up.
> > > 
> > > So it can never happen that ep maxpacket is unaligned?
> > 
> > maxpacket can be defined as e.g. 16 Bytes (wMaxPacketSize).
> > 
> > However the underlying buffer (on which we perform dcache
> > invalidation) is already cache line aware (defined with memalign).
> > Also we copy the usb request data there (yes I know that this is not
> > the best possible solution). Afterwards address of this buffer is a
> > starting point for DMA.
> > Since we are sending in the HW one packet at a time (with maxpacket
> > size), then it should be enough to invalidate only up to 512 B.
> > Previously the whole buffer (DMA_BUFFER_SIZE -> 16 KiB) was
> > invalidated each time.
> > 
> > Since we are invalidating cache content corresponding to buffer
> > mapped memory, we are safe when D-cache controller invalidates 32B
> > (CACHE_LINE_SIZE) instead of 16B.
> 
> I get it, but if the size is lower than CACHE_LINE_SIZE, it will not
> get invalidated at all.

I've looked into the v7_dcache_inval_range() function at cache_v7.c
and .... you are right here.
Cache doesn't touch the last and fist line if they aren't properly
aligned. Obviously I need ROUND(CACHE_LINE_SIZE) there.

Thanks for opening my eyes....

> Thus the roundup might be needed.
>  
> > > > > Best regards,
> > > > > Marek Vasut
> > > > 
> > > > Since this is Samsung's UDC specific (not THOR download) -
> > > > would it be possible to take this patch from this patch series
> > > > (of course if my above rationale is acceptable for you)?
> > > 
> > > You mean for .10 ?
> > 
> > This patch hasn't introduced regressions for DFU/UMS. It can be
> > applied to .10 or to u-boot-usb/next.
> 
> If it's not an explicit fix, this will go to -next

I will prepare v2 with rounding here.

> 
> Best regards,



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group