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

[puneets <puneets@nvidia.com>]

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