[U-Boot] [PATCH v2] usb: dwc3: Allocate and flush dwc->ep0_trb in a cache aligned manner

[Marek Vasut]

On 10/17/2017 01:10 PM, Faiz Abbas wrote:
> Hey,
> 
> On Tuesday 17 October 2017 03:31 PM, Marek Vasut wrote:
>> On 10/17/2017 07:25 AM, Faiz Abbas wrote:
>>>
>>>
>>> On Monday 16 October 2017 08:52 PM, Marek Vasut wrote:
>>>> On 10/16/2017 04:51 PM, Felipe Balbi wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> Faiz Abbas <faiz_abbas at ti.com> writes:
>>>>>>>>> Marek Vasut <marex at denx.de> writes:
>>>>>>>>>> On 10/16/2017 07:21 AM, Faiz Abbas wrote:
>>>>>>>>>>> A flush of the cache is required before any outbound DMA access can
>>>>>>>>>>> take place. The minimum size that can be flushed from the cache is
>>>>>>>>>>> one cache line size. Therefore, any buffer allocated for DMA should
>>>>>>>>>>> be in multiples of cache line size.
>>>>>>>>>>>
>>>>>>>>>>> Thus, allocate memory for ep0_trb in multiples of cache line size.
>>>>>>>>>>>
>>>>>>>>>>> Also, when local variable trb is assigned to dwc->ep0_trb[1] and used
>>>>>>>>>>> to flush cache, it leads to cache misaligned messages as only the base
>>>>>>>>>>> address dwc->ep0_trb is cache aligned.
>>>>>>>>>>>
>>>>>>>>>>> Therefore, flush cache using ep0_trb_addr which is always cache aligned.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Faiz Abbas <faiz_abbas at ti.com>
>>>>>>>>>>
>>>>>>>>>> SGTM, Felipe, can you review this please ?
>>>>>>>>>
>>>>>>>>> is cache maintenance done correctly in u-boot? Isn't the whole idea of a
>>>>>>>>> coherent memory area that is is non-cacheable, non-bufferable memory?
>>>>>>>>>
>>>>>>>>> Also, why isn't the API itself guaranteeing alignment requirements?
>>>>>>>>>
>>>>>>>> There is no support in u-boot to make a memory area non-cacheable.
>>>>>>>> This is the definition of dma_alloc_coherent()
>>>>>>>>
>>>>>>>> static inline void *dma_alloc_coherent(size_t len, unsigned long *handle)
>>>>>>>> {
>>>>>>>>         *handle = (unsigned long)memalign(ARCH_DMA_MINALIGN, len);
>>>>>>>>         return (void *)*handle;
>>>>>>>> }
>>>>>>>>
>>>>>>>> This driver is mostly copied from kernel (where dma_alloc_coherent() is
>>>>>>>> what you describe) and extra flush_cache functions are added because of
>>>>>>>> U-Boot's inability to allocate coherent memory.
>>>>>>>
>>>>>>> then that's what should be fixed. No?
>>>>>>>
>>>>>>
>>>>>> You're right but that sounds like a long-term feature which will affect
>>>>>> a huge part of u-boot. Until it is implemented, I guess this is the best
>>>>>> way to handle the issue.
>>>>>
>>>>> Not my call to make. I'll defer to Marek and Tom
>>>>>
>>>> We're deep in RC anyway, so feel free to prepare a fix for next MW .
>>>>
>>>
>>> Fix as in rebase same patch for next merge window?
>>
>> As in, add support for marking memory area noncachable and then use it
>> here. It shouldn't be hard, it's only about some MMU table attributes.
>>
> 
> dma_alloc_coherent() is used by many architectures (arm, x86, nios2,
> nds32). I can implement the feature in arm because I can test it but
> someone else needs to do it for the other architectures.

Sounds good.

-- 
Best regards,
Marek Vasut