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

[Marek Vasut]

On 10/03/2017 02:18 PM, Dr. Philipp Tomsich wrote:
> Marek,
> 
>> On 3 Oct 2017, at 14:04, Marek Vasut <marex at denx.de> wrote:
>>
>> On 09/19/2017 01:15 PM, Faiz Abbas wrote:
>>> A flush of the cache is required before any DMA access can take place.
>>
>> You mean invalidation for inbound DMA, flush for outbound DMA, right ?
>>
>>> 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>
>>> ---
>>> drivers/usb/dwc3/ep0.c    | 7 ++++---
>>> drivers/usb/dwc3/gadget.c | 3 ++-
>>> 2 files changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>>> index e61d980..f3a17a1 100644
>>> --- a/drivers/usb/dwc3/ep0.c
>>> +++ b/drivers/usb/dwc3/ep0.c
>>> @@ -82,7 +82,7 @@ static int dwc3_ep0_start_trans(struct dwc3 *dwc, u8 epnum, dma_addr_t buf_dma,
>>> 				| DWC3_TRB_CTRL_LST);
>>>
>>> 	dwc3_flush_cache((uintptr_t)buf_dma, len);
>>> -	dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
>>> +	dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
>>>
>>> 	if (chain)
>>> 		return 0;
>>> @@ -790,7 +790,7 @@ static void dwc3_ep0_complete_data(struct dwc3 *dwc,
>>> 	if (!r)
>>> 		return;
>>>
>>> -	dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
>>> +	dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
>>
>> Why *2 ?
>>
>>> 	status = DWC3_TRB_SIZE_TRBSTS(trb->size);
>>> 	if (status == DWC3_TRBSTS_SETUP_PENDING) {
>>> @@ -821,7 +821,8 @@ static void dwc3_ep0_complete_data(struct dwc3 *dwc,
>>> 			ur->actual += transferred;
>>>
>>> 			trb++;
>>> -			dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
>>> +			dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr,
>>> +					 sizeof(*trb) * 2);
>>> 			length = trb->size & DWC3_TRB_SIZE_MASK;
>>>
>>> 			ep0->free_slot = 0;
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index e065c5a..895a5bc 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -2567,7 +2567,8 @@ int dwc3_gadget_init(struct dwc3 *dwc)
>>> 		goto err0;
>>> 	}
>>>
>>> -	dwc->ep0_trb = dma_alloc_coherent(sizeof(*dwc->ep0_trb) * 2,
>>> +	dwc->ep0_trb = dma_alloc_coherent(ROUND(sizeof(*dwc->ep0_trb) * 2,
>>> +						CACHELINE_SIZE),
>>
>> AFAIU dma_alloc_coherent() should mark the memory area uncachable .
> 
> We had this discussion a while back, when I submitted the fixes to make
> this driver work on the RK3399: dma_alloc_coherent only performs a
> memalign on ARM and ARM64:
> 
> See the following snippet in arch/arm/include/asm/dma-mapping.h:

Does that mean the code is wrong / the function name is misleading ?

> static inline void *dma_alloc_coherent(size_t len, unsigned long *handle)
> {
>         *handle = (unsigned long)memalign(ARCH_DMA_MINALIGN, len);
>         return (void *)*handle;
> }
[...]

-- 
Best regards,
Marek Vasut