Dear Mike Frysinger, > On Wednesday 07 March 2012 02:12:22 puneets wrote: > > On Tuesday 06 March 2012 08:37 AM, Mike Frysinger wrote: > > >> --- 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. > > > > 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. > > i think you missed my point. consider a func which has local vars like so: > int i; > char buf[1024]; > int k; > > and let's say you're running on a core that has a cache line size of 32 > bytes (which is fairly common). if you execute a data cache invalid insn, > the smallest region it can invalidate is 32 bytes. doesn't matter if you > only want to invalidate a buffer of 8 bytes ... everything else around it > gets invalidated as well. > > now, in the aforementioned stack, if it starts off aligned nicely at a 32 > byte boundary, the integer "i" will share a cache line with the first 28 > bytes of buffer "buf", and the integer "k" will share a cache line with > the last 4 bytes of the buffer "buf". (let's ignore what might or might > not happen based on gcc since this example can trivially be expanded to > structure layout.) > > the trouble is when you attempt to invalidate the contents of "buf". if > the cache is in writeback mode (which means you could have changes in the > cache which are not reflected in external RAM), then invalidating buf will > also discard values that might be in "i" or "k". this is why Simon put a > warning in the core data cache invalidate function. if the cache were in > writethrough mode (which also tends to be the default), then most likely > things would work fine and no one would notice. or if the data cache was > merely flushed, things would work, but at a decrease in performance: you'd > be flushing cache lines to external memory that you know will be > overwritten by a following transaction -- most likely DMA from a > peripheral such as the USB controller, and you'd be flushing objects that > the DMA wouldn't be touching, so they'd have to get refetched from > external RAM ("i" and "k" in my example above). > > simply rounding the address down to the start of the cache line and the > length up to a multiple of a cache line to keep the core code from issuing > the warning doesn't fix the problem i describe above. you actually get > the worst of both worlds -- silent runtime misbehavior when extra memory > gets invalidated. > > perhaps the warning in the core code could be dropped and all your changes > in fringe code obsoleted (such as these USB patches): when it detects that > an address is starting on an unaligned boundary, *flush* that line first, > and then let it be invalidated. accordingly, when the end length is on an > unaligned boundary, do the same flush-then-invalidate step. this should > also make things work without a (significant) loss in performance. if > anything, i suspect the overhead of doing runtime buffer size calculations > and manually aligning pointers (which is what ALLOC_CACHE_ALIGN_BUFFER > does) is a wash compared to partially flushing cache lines in the core ... > > Simon: what do you think of this last idea ? > -mike
Did we get anywhere with this? Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot