[U-Boot] [PATCH] usb: dwc3: gadget: make cache-maintenance on event buffers more robust

[Marek Vasut <marex@denx.de>]

On 04/04/2017 09:56 PM, Dr. Philipp Tomsich wrote:
> 
>> On 04 Apr 2017, at 21:01, Marek Vasut <marex@denx.de> wrote:
>>
>>> Good point on the “long”, especially as I just copied this from other occurrences and it’s consistently wrong throughout DWC3 in U-Boot:
>>
>> Hrm, I thought the driver was ported over from Linux, so is this broken
>> in Linux too ?
> 
> Apparently, the dwc3_flush_cache calls (and the function itself) have been
> introduced during the porting. There’s no explicit cache-maintenance in DWC3
> for Linux. 

OK

>>> I’ll revise all of these and make a patch-series out of this.
>>
>> Maybe you should check the Linux first and see if there are some fixes
>> already.
>>
>> Thanks
> 
> Given that this seems to have been introduced with the port to U-Boot, there’s
> no applicable fixes there.

OK

>>>>> 	return evt;
>>>>> }
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>>> index 1156662..61af71b 100644
>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>> @@ -2668,11 +2668,12 @@ void dwc3_gadget_uboot_handle_interrupt(struct dwc3 *dwc)
>>>>> 		int i;
>>>>> 		struct dwc3_event_buffer *evt;
>>>>>
>>>>> +		dwc3_thread_interrupt(0, dwc);
>>>>> +
>>>>> +		/* Clean + Invalidate the buffers after touching them */
>>>>> 		for (i = 0; i < dwc->num_event_buffers; i++) {
>>>>> 			evt = dwc->ev_buffs[i];
>>>>> 			dwc3_flush_cache((long)evt->buf, evt->length);
>>>>> 		}
>>>>> -
>>>>
>>>> This makes me wonder, don't you need to invalidate the event buffer
>>>> somewhere so that the new data would be fetched from RAM ?
>>>
>>> We flush the event buffer before leaving the function.
>>> So the cache line will not be present in the cache, when we enter this function again.
>>
>> Then shouldn't we invalidate it instead ? flush and invalidate are two
>> different things …
> 
> The DWC3 flush expands to a clean+invalidate. It is not wrong, as long as
> it is used as in my patch:
> a. before the first time data is expected to be written by the peripheral (i.e.
> before the peripheral is started)—to ensure that the cache line is not cached
> any longer…

So invalidate() is enough ?

> b. after the driver modifies any buffers (i.e. anything modified will be written
> back) and before it next reads the buffers expecting possibly changed data
> (i.e. invalidating).

So flush+invalidate ? Keep in mind this driver may not be used on
ARMv7/v8 only ...

-- 
Best regards,
Marek Vasut