From mboxrd@z Thu Jan 1 00:00:00 1970 From: puneets Date: Fri, 9 Mar 2012 11:45:25 +0530 Subject: [U-Boot] [PATCH v8] usb: align buffers at cacheline In-Reply-To: <201203081512.45417.marex@denx.de> References: <4F54BEA9.6060302@boundarydevices.com> <201203072306.32769.marex@denx.de> <4F589650.2080902@nvidia.com> <201203081512.45417.marex@denx.de> Message-ID: <4F599FFD.5030702@nvidia.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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? Best regards, >>> Marek Vasut >> Thanx& Regards, >> Puneet >> >> --------------------------------------------------------------------------- >> -------- This email message is for the sole use of the intended >> recipient(s) and may contain confidential information. Any unauthorized >> review, use, disclosure or distribution is prohibited. If you are not the >> intended recipient, please contact the sender by reply email and destroy >> all copies of the original message. >> --------------------------------------------------------------------------- >> -------- > Best regards, > Marek Vasut Thanx & Regards, Puneet