On 11/11/2016 3:10 AM, Felipe Balbi wrote:
>
> Hi,
>
> John Youn <[email protected]> writes:
>> Currently GEVNTCOUNT is written in the threaded interrupt handler while
>> processing each event. This commit moves the GEVNTCOUNT write to the
>> hard IRQ. We then copy the events to a separate buffer for the event
>> handler to read from.
>>
>> This change is in preparation of working around an issue in core version
>> 3.00a where the interrupt cannot be de-asserted in the normal way.
>> However, if we enable interrupt moderation, we can also de-assert it by
>> writing to GEVNTCOUNT.
>>
>> Signed-off-by: John Youn <[email protected]>
>> ---
>> drivers/usb/dwc3/core.c | 4 ++++
>> drivers/usb/dwc3/core.h | 2 ++
>> drivers/usb/dwc3/gadget.c | 22 +++++++++++++---------
>> 3 files changed, 19 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index af42346..f0bb6df 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -251,6 +251,10 @@ static struct dwc3_event_buffer
>> *dwc3_alloc_one_event_buffer(struct dwc3 *dwc,
>>
>> evt->dwc = dwc;
>> evt->length = length;
>> + evt->cache = devm_kzalloc(dwc->dev, length, GFP_KERNEL);
>> + if (!evt->cache)
>> + return ERR_PTR(-ENOMEM);
>> +
>> evt->buf = dma_alloc_coherent(dwc->dev, length,
>> &evt->dma, GFP_KERNEL);
>> if (!evt->buf)
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 354de24..bf63756 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -472,6 +472,7 @@ struct dwc3_trb;
>> /**
>> * struct dwc3_event_buffer - Software event buffer representation
>> * @buf: _THE_ buffer
>> + * @cache: The buffer cache used in the threaded interrupt
>> * @length: size of this buffer
>> * @lpos: event offset
>> * @count: cache of last read event count register
>> @@ -481,6 +482,7 @@ struct dwc3_trb;
>> */
>> struct dwc3_event_buffer {
>> void *buf;
>> + void *cache;
>> unsigned length;
>> unsigned int lpos;
>> unsigned int count;
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 22ccc34..f07dd84 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2829,18 +2829,16 @@ static irqreturn_t dwc3_process_event_buf(struct
>> dwc3_event_buffer *evt)
>> {
>> struct dwc3 *dwc = evt->dwc;
>> irqreturn_t ret = IRQ_NONE;
>> - int left;
>> + int idx;
>> u32 reg;
>>
>> - left = evt->count;
>> -
>> if (!(evt->flags & DWC3_EVENT_PENDING))
>> return IRQ_NONE;
>>
>> - while (left > 0) {
>> + for (idx = 0; idx < evt->count; idx += 4) {
>
> why so many changes on the same patch? Why did you remove left and
> evt->lpos? You can still use both for traversing through your cache,
> right?
What I did was copy the received events from 'buf' to the beginning of
'cache'. So the handler just always traverses an array of 'count'
events from the beginning.
I could just do a single copy of the entire 'buf' each time. That
would be less changes.
>
>> union dwc3_event event;
>>
>> - event.raw = *(u32 *) (evt->buf + evt->lpos);
>> + event.raw = *(u32 *)(evt->cache + idx);
>>
>> dwc3_process_event_entry(dwc, &event);
>>
>> @@ -2853,10 +2851,6 @@ static irqreturn_t dwc3_process_event_buf(struct
>> dwc3_event_buffer *evt)
>> * boundary so I worry about that once we try to handle
>> * that.
>> */
>> - evt->lpos = (evt->lpos + 4) % DWC3_EVENT_BUFFERS_SIZE;
>> - left -= 4;
>> -
>> - dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), 4);
>> }
>>
>> evt->count = 0;
>> @@ -2889,6 +2883,7 @@ static irqreturn_t dwc3_check_event_buf(struct
>> dwc3_event_buffer *evt)
>> {
>> struct dwc3 *dwc = evt->dwc;
>> u32 count;
>> + u32 amount;
>
> please keep reverse xmas tree ordering ;-)
>
>> u32 reg;
>>
>> if (pm_runtime_suspended(dwc->dev)) {
>> @@ -2906,6 +2901,15 @@ static irqreturn_t dwc3_check_event_buf(struct
>> dwc3_event_buffer *evt)
>> evt->count = count;
>> evt->flags |= DWC3_EVENT_PENDING;
>>
>> + amount = min(count, DWC3_EVENT_BUFFERS_SIZE - evt->lpos);
>> + memcpy(evt->cache, evt->buf + evt->lpos, amount);
>> +
>> + if (amount < count)
>> + memcpy(evt->cache + amount, evt->buf, count - amount);
>> +
>> + evt->lpos = (evt->lpos + count) % DWC3_EVENT_BUFFERS_SIZE;
>
> evt->lpos is only for the driver, if you don't touch it here you can
> still use it on the bottom half handler.
Ok, I'll put it back in the BH.
>
>> + dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count);
>
> should you mask first just to make sure we don't have other interrupts
> as soon as we clear the ones we just read?
>
I don't think it matters since we are in interrupt context. But I can
move it to be the first thing here.
I'm out until Monday, so I won't be able to send a new version until
then.
Regards,
John
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
