[U-Boot] [PATCH v8] usb: align buffers at cacheline

[Marek Vasut]

Dear puneets,

> Hi Marek,
> 
> On Thursday 08 March 2012 07:42 PM, Marek Vasut wrote:
> > Dear puneets,
> > 
> >> Hi Marek,
> >> 
> >> On Thursday 08 March 2012 03:36 AM, Marek Vasut wrote:
> >>> Dear puneets,
> >>> 
> >>>> Hi Mike,
> >>>> 
> >>>> On Tuesday 06 March 2012 08:37 AM, Mike Frysinger wrote:
> >>>>> * PGP Signed by an unknown key
> >>>>> 
> >>>>> On Monday 05 March 2012 09:46:21 Puneet Saxena wrote:
> >>>>>> As DMA expects the buffers to be equal and larger then
> >>>>>> cache lines, This aligns buffers at cacheline.
> >>>>> 
> >>>>> i don't think this statement is true.  DMA doesn't care about
> >>>>> alignment (well, some do, but it's not related to cache lines but
> >>>>> rather some other restriction in the peripheral DMA itself).  what
> >>>>> does matter is that cache operations operate on cache lines and not
> >>>>> individual bytes. hence the core arm code was updated to warn when
> >>>>> someone told it to invalidate X bytes but the hardware literally
> >>>>> could not, so it had to invalidate X + Y bytes.
> >>>> 
> >>>> Agreed, Will update the commit message in next patchset.
> >>>> 
> >>>>>> --- a/drivers/usb/host/ehci-hcd.c
> >>>>>> +++ b/drivers/usb/host/ehci-hcd.c
> >>>>>> 
> >>>>>>     static void flush_invalidate(u32 addr, int size, int flush)
> >>>>>>     {
> >>>>>> 
> >>>>>> +	/*
> >>>>>> +	 * Size is the bytes actually moved during transaction,
> >>>>>> +	 * which may not equal to the cache line. This results
> >>>>>> +	 * stop address passed for invalidating cache may not be
> > 
> > aligned.
> > 
> >>>>>> +	 * Therfore making size as multiple of cache line size.
> >>>>>> +	 */
> >>>>>> +	size = ALIGN(size, ARCH_DMA_MINALIGN);
> >>>>>> +
> >>>>>> 
> >>>>>>     	if (flush)
> >>>>>>     	
> >>>>>>     		flush_dcache_range(addr, addr + size);
> >>>>>>     	
> >>>>>>     	else
> >>>>> 
> >>>>> i think this is wrong and merely hides the errors from higher up
> >>>>> instead of fixing them.  the point of the warning was to tell you
> >>>>> that the code was invalidating *too many* bytes.  this code still
> >>>>> invalidates too many bytes without any justification as for why it's
> >>>>> OK to do here.  further, this code path only matters to the
> >>>>> invalidation logic, not the flush logic. -mike
> >>>> 
> >>>> The sole purpose of this patch to remove the warnings as start/stop
> >>>> address sent for invalidating
> >>>> is unaligned. Without this patch code works fine but with lots of
> >>>> spew...Which we don't want and discussed
> >>>> in earlier thread which Simon posted. Please have a look on following
> >>>> link.
> >>>> 
> >>>> As I understood, you agree that we need to align start/stop buffer
> >>>> address and also agree that
> >>>> to align stop address we need to align size as start address is
> >>>> already aligned.
> >>>> Now, "why its OK to do here"?
> >>>> We could have aligned the size in two places, cache_qtd() and
> >>>> cache_qh() but then we need to place alignment check
> >>>> at all the places where size is passed. So I thought better Aligning
> >>>> at flush_invalidate() and "ALIGN" macro does not
> >>>> increase the size if size is already aligned.
> >>> 
> >>> Actually I have to agree with Mike here. Can you please remove that
> >>> ALIGN() (and all others you might have added)? If it does spew, that's
> >>> ok and it tells us something is wrong in the USB core subsystem. Such
> >>> stuff can be fixed in subsequent patch.
> >> 
> >> Sorry, I could not understand "(and all others you might have added)".
> >> Do you want me remove any HACK in the patch which is using ALIGN or
> >> making stop address
> > 
> > No, only such hacks where it's certain they will either invalidate or
> > flush some areas that weren't allocated for them, like this ALIGN you
> > did here. This can cause trouble that will be very hard to find.
> > 
> >> aligned? The patch has only the above line to make stop address align
> >> and rest of the code makes
> >> start address align. Just to confirm, you are fine with the start
> >> address alignment code in the patch?
> > 
> > The start address alignment you do also aligns the end to the cacheline,
> > doesn't it? (at least that's what I believe the macro is supposed to
> > do).
> 
> Yes, start address alignment also aligns start address at the cache
> line. So, removing
> stop address alignment code as depicted above, should make this patch
> acceptable?
> 
Puneet, it's not a problem of acceptability, it's a problem of breaking uboot 
;-) Acceptability goes only after it we are sure doesn't break anything. The 
cache stuff is really fragile so we need to be very careful, please understand 
and let's figure this out together, I feel we're really close.

And yes, I believe removing such dangerous ALIGN() calls will be the last fix 
necessary, unless something other pops up.

Best regards,
Marek Vasut