Dear Ilya Yanok, > Dear Marek, > > 30.06.2012 23:27, Marek Vasut wrote: > >>> do { > >>> > >>> /* Invalidate dcache */ > >>> invalidate_dcache_range((uint32_t)&qh_list, > >>> > >>> (uint32_t)&qh_list + sizeof(struct QH)); > >>> > >>> invalidate_dcache_range((uint32_t)&qh, > >>> > >>> (uint32_t)&qh + sizeof(struct QH)); > >>> > >>> invalidate_dcache_range((uint32_t)qtd, > >>> > >>> (uint32_t)qtd + sizeof(qtd)); > >> > >> These guys are 32-byte aligned, right? So the assumption is that > >> cacheline is not greater than 32. Sounds like a bug to me (though could > >> be fixed rather easily with USB_MINALIGN introduced by Tom). > > > > Oooh, good catch indeed. Actually, look at the structures, even they have > > some weird alignment crap in them. Maybe that can also be dropped and > > the alignment shall be fixed properly. I'll have to research this more. > > I guess that's because they have to be both address and size aligned. > > >>> token = hc32_to_cpu(vtd->qt_token); > >>> if (!(token& 0x80)) > >>> > >>> break; > >>> > >>> WATCHDOG_RESET(); > >>> > >>> } while (get_timer(ts)< timeout); > >>> > >>> /* Invalidate the memory area occupied by buffer */ > >>> invalidate_dcache_range(((uint32_t)buffer& ~31), > >>> > >>> ((uint32_t)buffer& ~31) + roundup(length, 32)); > >> > >> That's worse. First of all, rounding is wrong: consider buffer=31, > >> length=32. But that's easy to fix too. The bad part is that with > >> writeback cache you can't just enlarge the range to cover the buffer and > >> fit alignment requirements -- this can potentially destroy some changes > >> that are in the same cache line but not yet stored to RAM. > > > > Correct, so we have multiple bugs in here that need to be squashed ASAP. > > > > Ilya, can you possibly submit a patch for this? > > Hm.. Fixing alignment for qh, qd and stuff is easy, I can surely submit > a patch for this.
Well ... partial patch is still better than none. > But I don't really know what to do this the last one: > if we are at this point there is no correct way out... We can increase > the range to cover the buffer or decrease it to be inside buffer -- but > both options are incorrect. Actually we should return error when > unaligned buffer is submitted in the first place... But this will likely > break a lot of systems... Probably put this check under if > (dcache_enabled())? Bounce buffer I guess ... with warning that you'll hit performance penalty becaue you're dumb and doing unaligned transfer. And for internal uboot stuff, we can align it. > > Regards, Ilya. Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot