Re: [PATCH v2 3/5] usb: dwc3: gadget: Write the event count in interrupt
Hi, Janusz Dziedzic writes: >> John Youn writes: + dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count); + >> After that evt->buf[lpos, lpos + count] seems goes back to HW, so >> thread should not rely on this? >> >> Or I miss something? > > Hi, > > Yes, you're right. That's a possibility and to be safe we can cache > those. > > We're relying on the same mechanism that keeps the event buffer from > becoming full. Since it is just as (un)likely a possibility. That's > why you must size the event buffer appropriately taking into account > your system's latency, etc. > > And if the event buffer becomes full, that indicates something really > wrong happened in the controller. You shouldn't be getting 100's of > events at a time. > > But yes, we can address the overwriting issue. > > Either: > > 1) Cache all incoming events. Requires double the event buffer space. > > 2) Cache just one event and write back only '4' during hard >interrupt. Then in threaded interrupt read the one event from >cache, and process the remaining events from buffer as before. >Doesn't require a large cache, but a bit messier. > > Any other thoughts or ideas? do you really need to clear at least one event for this? >>> >>> Unfortunately yes. That's how the workaround works. We need to write >>> this during IMOD to de-assert the interrupt since the mask bit doesn't >>> work. >> >> okay. Then we should cache and clear a single event. >> > Cache all incoming events looks better for me, while we don't need > care of Vendor Test Event (12Bytes) here - and we will always handle That doesn't matter. If we write 4 to GEVNTCOUNT(0), then only 4 bytes will be cleared. We can still read the following 8 bytes of data from the event buffer itself. > this correctly and can add simple implementation for that in bottom > half. we can still handle this all correctly :-) -- balbi signature.asc Description: PGP signature
Re: [PATCH v2 3/5] usb: dwc3: gadget: Write the event count in interrupt
On 9 November 2016 at 09:05, Felipe Balbi wrote: > > Hi, > > John Youn writes: >>> + dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count); >>> + > After that evt->buf[lpos, lpos + count] seems goes back to HW, so > thread should not rely on this? > > Or I miss something? Hi, Yes, you're right. That's a possibility and to be safe we can cache those. We're relying on the same mechanism that keeps the event buffer from becoming full. Since it is just as (un)likely a possibility. That's why you must size the event buffer appropriately taking into account your system's latency, etc. And if the event buffer becomes full, that indicates something really wrong happened in the controller. You shouldn't be getting 100's of events at a time. But yes, we can address the overwriting issue. Either: 1) Cache all incoming events. Requires double the event buffer space. 2) Cache just one event and write back only '4' during hard interrupt. Then in threaded interrupt read the one event from cache, and process the remaining events from buffer as before. Doesn't require a large cache, but a bit messier. Any other thoughts or ideas? >>> >>> do you really need to clear at least one event for this? >>> >> >> Unfortunately yes. That's how the workaround works. We need to write >> this during IMOD to de-assert the interrupt since the mask bit doesn't >> work. > > okay. Then we should cache and clear a single event. > Cache all incoming events looks better for me, while we don't need care of Vendor Test Event (12Bytes) here - and we will always handle this correctly and can add simple implementation for that in bottom half. In case of single event cache, VTE handling will be much harder/ugly ... - while we have to check if cache 4 or 12 bytes. Anyway is the VTE case at all? We don't support this currently and don't have an issues ... BR Janusz >> We could do this only for 3.00a as well. > > if we do this only for 3.00a then the code will look odd :-) It > shouldn't cause any problems for any other revisions > > -- > balbi -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/5] usb: dwc3: gadget: Write the event count in interrupt
Hi, John Youn writes: >> + dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count); >> + After that evt->buf[lpos, lpos + count] seems goes back to HW, so thread should not rely on this? Or I miss something? >>> >>> Hi, >>> >>> Yes, you're right. That's a possibility and to be safe we can cache >>> those. >>> >>> We're relying on the same mechanism that keeps the event buffer from >>> becoming full. Since it is just as (un)likely a possibility. That's >>> why you must size the event buffer appropriately taking into account >>> your system's latency, etc. >>> >>> And if the event buffer becomes full, that indicates something really >>> wrong happened in the controller. You shouldn't be getting 100's of >>> events at a time. >>> >>> But yes, we can address the overwriting issue. >>> >>> Either: >>> >>> 1) Cache all incoming events. Requires double the event buffer space. >>> >>> 2) Cache just one event and write back only '4' during hard >>>interrupt. Then in threaded interrupt read the one event from >>>cache, and process the remaining events from buffer as before. >>>Doesn't require a large cache, but a bit messier. >>> >>> Any other thoughts or ideas? >> >> do you really need to clear at least one event for this? >> > > Unfortunately yes. That's how the workaround works. We need to write > this during IMOD to de-assert the interrupt since the mask bit doesn't > work. okay. Then we should cache and clear a single event. > We could do this only for 3.00a as well. if we do this only for 3.00a then the code will look odd :-) It shouldn't cause any problems for any other revisions -- balbi signature.asc Description: PGP signature
Re: [PATCH v2 3/5] usb: dwc3: gadget: Write the event count in interrupt
On 11/8/2016 6:09 AM, Felipe Balbi wrote: > > Hi, > > John Youn writes: >> On 11/4/2016 2:13 AM, Janusz Dziedzic wrote: >>> On 4 November 2016 at 07:41, Janusz Dziedzic >>> wrote: On 4 November 2016 at 02:31, John Youn wrote: > > Since we are saving the event count and handling the events in the > threaded interrupt handler, we can write and clear out the eventcount in > the hard interrupt handler itself. > > This behavior will be required for IP 3.00a cores that need to use > interrupt moderation as a workaround for an RTL issue were the interrupt > line cannot be masked between the hard/soft interrupt handler. > > Signed-off-by: John Youn > --- > drivers/usb/dwc3/gadget.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index a9c1d75..ac9eb39 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -2877,8 +2877,6 @@ static irqreturn_t dwc3_process_event_buf(struct > dwc3_event_buffer *evt) > */ > evt->lpos = (evt->lpos + 4) % DWC3_EVENT_BUFFERS_SIZE; > left -= 4; > - > - dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), 4); > } > > evt->count = 0; > @@ -2928,6 +2926,8 @@ static irqreturn_t dwc3_check_event_buf(struct > dwc3_event_buffer *evt) > evt->count = count; > evt->flags |= DWC3_EVENT_PENDING; > >>> Shouldn't you cache (somewhere): >>> u32 events[evt->buf + evt->lpos evt->buf + evt->lpos + count] >>> >>> before you will giveback this to HW (to be sure HW will not overwrite >>> this in case lot of events)? > > good point. That's why I wasn't clearing events until I actually > processed them :-) > > + dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count); > + >>> After that evt->buf[lpos, lpos + count] seems goes back to HW, so >>> thread should not rely on this? >>> >>> Or I miss something? >> >> Hi, >> >> Yes, you're right. That's a possibility and to be safe we can cache >> those. >> >> We're relying on the same mechanism that keeps the event buffer from >> becoming full. Since it is just as (un)likely a possibility. That's >> why you must size the event buffer appropriately taking into account >> your system's latency, etc. >> >> And if the event buffer becomes full, that indicates something really >> wrong happened in the controller. You shouldn't be getting 100's of >> events at a time. >> >> But yes, we can address the overwriting issue. >> >> Either: >> >> 1) Cache all incoming events. Requires double the event buffer space. >> >> 2) Cache just one event and write back only '4' during hard >>interrupt. Then in threaded interrupt read the one event from >>cache, and process the remaining events from buffer as before. >>Doesn't require a large cache, but a bit messier. >> >> Any other thoughts or ideas? > > do you really need to clear at least one event for this? > Unfortunately yes. That's how the workaround works. We need to write this during IMOD to de-assert the interrupt since the mask bit doesn't work. We could do this only for 3.00a as well. Regards, John -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/5] usb: dwc3: gadget: Write the event count in interrupt
Hi, John Youn writes: > On 11/4/2016 2:13 AM, Janusz Dziedzic wrote: >> On 4 November 2016 at 07:41, Janusz Dziedzic >> wrote: >>> On 4 November 2016 at 02:31, John Youn wrote: Since we are saving the event count and handling the events in the threaded interrupt handler, we can write and clear out the eventcount in the hard interrupt handler itself. This behavior will be required for IP 3.00a cores that need to use interrupt moderation as a workaround for an RTL issue were the interrupt line cannot be masked between the hard/soft interrupt handler. Signed-off-by: John Youn --- drivers/usb/dwc3/gadget.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index a9c1d75..ac9eb39 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2877,8 +2877,6 @@ static irqreturn_t dwc3_process_event_buf(struct dwc3_event_buffer *evt) */ evt->lpos = (evt->lpos + 4) % DWC3_EVENT_BUFFERS_SIZE; left -= 4; - - dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), 4); } evt->count = 0; @@ -2928,6 +2926,8 @@ static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt) evt->count = count; evt->flags |= DWC3_EVENT_PENDING; >> Shouldn't you cache (somewhere): >> u32 events[evt->buf + evt->lpos evt->buf + evt->lpos + count] >> >> before you will giveback this to HW (to be sure HW will not overwrite >> this in case lot of events)? good point. That's why I wasn't clearing events until I actually processed them :-) + dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count); + >> After that evt->buf[lpos, lpos + count] seems goes back to HW, so >> thread should not rely on this? >> >> Or I miss something? > > Hi, > > Yes, you're right. That's a possibility and to be safe we can cache > those. > > We're relying on the same mechanism that keeps the event buffer from > becoming full. Since it is just as (un)likely a possibility. That's > why you must size the event buffer appropriately taking into account > your system's latency, etc. > > And if the event buffer becomes full, that indicates something really > wrong happened in the controller. You shouldn't be getting 100's of > events at a time. > > But yes, we can address the overwriting issue. > > Either: > > 1) Cache all incoming events. Requires double the event buffer space. > > 2) Cache just one event and write back only '4' during hard >interrupt. Then in threaded interrupt read the one event from >cache, and process the remaining events from buffer as before. >Doesn't require a large cache, but a bit messier. > > Any other thoughts or ideas? do you really need to clear at least one event for this? -- balbi signature.asc Description: PGP signature
Re: [PATCH v2 3/5] usb: dwc3: gadget: Write the event count in interrupt
On 11/4/2016 2:13 AM, Janusz Dziedzic wrote: > On 4 November 2016 at 07:41, Janusz Dziedzic > wrote: >> On 4 November 2016 at 02:31, John Youn wrote: >>> >>> Since we are saving the event count and handling the events in the >>> threaded interrupt handler, we can write and clear out the eventcount in >>> the hard interrupt handler itself. >>> >>> This behavior will be required for IP 3.00a cores that need to use >>> interrupt moderation as a workaround for an RTL issue were the interrupt >>> line cannot be masked between the hard/soft interrupt handler. >>> >>> Signed-off-by: John Youn >>> --- >>> drivers/usb/dwc3/gadget.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>> index a9c1d75..ac9eb39 100644 >>> --- a/drivers/usb/dwc3/gadget.c >>> +++ b/drivers/usb/dwc3/gadget.c >>> @@ -2877,8 +2877,6 @@ static irqreturn_t dwc3_process_event_buf(struct >>> dwc3_event_buffer *evt) >>> */ >>> evt->lpos = (evt->lpos + 4) % DWC3_EVENT_BUFFERS_SIZE; >>> left -= 4; >>> - >>> - dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), 4); >>> } >>> >>> evt->count = 0; >>> @@ -2928,6 +2926,8 @@ static irqreturn_t dwc3_check_event_buf(struct >>> dwc3_event_buffer *evt) >>> evt->count = count; >>> evt->flags |= DWC3_EVENT_PENDING; >>> > Shouldn't you cache (somewhere): > u32 events[evt->buf + evt->lpos evt->buf + evt->lpos + count] > > before you will giveback this to HW (to be sure HW will not overwrite > this in case lot of events)? > >>> + dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count); >>> + > After that evt->buf[lpos, lpos + count] seems goes back to HW, so > thread should not rely on this? > > Or I miss something? Hi, Yes, you're right. That's a possibility and to be safe we can cache those. We're relying on the same mechanism that keeps the event buffer from becoming full. Since it is just as (un)likely a possibility. That's why you must size the event buffer appropriately taking into account your system's latency, etc. And if the event buffer becomes full, that indicates something really wrong happened in the controller. You shouldn't be getting 100's of events at a time. But yes, we can address the overwriting issue. Either: 1) Cache all incoming events. Requires double the event buffer space. 2) Cache just one event and write back only '4' during hard interrupt. Then in threaded interrupt read the one event from cache, and process the remaining events from buffer as before. Doesn't require a large cache, but a bit messier. Any other thoughts or ideas? Regards, John > >>> /* Mask interrupt */ >>> reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0)); >>> reg |= DWC3_GEVNTSIZ_INTMASK; >>> -- >> >> Hello, >> >> Are we sure this will work fine with 2.60a? >> >> Some time ago I have similar code (introduce event_pop) and move >> dwc3_write(dwc->regs, DWC3_GEVNTCOUNT(0), 4) >> before >> dwc3_process_event_entry() >> >> And have some issues ... >> Didn't work correctly in my case. >> >> BR >> Janusz > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/5] usb: dwc3: gadget: Write the event count in interrupt
On 4 November 2016 at 07:41, Janusz Dziedzic wrote: > On 4 November 2016 at 02:31, John Youn wrote: >> >> Since we are saving the event count and handling the events in the >> threaded interrupt handler, we can write and clear out the eventcount in >> the hard interrupt handler itself. >> >> This behavior will be required for IP 3.00a cores that need to use >> interrupt moderation as a workaround for an RTL issue were the interrupt >> line cannot be masked between the hard/soft interrupt handler. >> >> Signed-off-by: John Youn >> --- >> drivers/usb/dwc3/gadget.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index a9c1d75..ac9eb39 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -2877,8 +2877,6 @@ static irqreturn_t dwc3_process_event_buf(struct >> dwc3_event_buffer *evt) >> */ >> evt->lpos = (evt->lpos + 4) % DWC3_EVENT_BUFFERS_SIZE; >> left -= 4; >> - >> - dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), 4); >> } >> >> evt->count = 0; >> @@ -2928,6 +2926,8 @@ static irqreturn_t dwc3_check_event_buf(struct >> dwc3_event_buffer *evt) >> evt->count = count; >> evt->flags |= DWC3_EVENT_PENDING; >> Shouldn't you cache (somewhere): u32 events[evt->buf + evt->lpos evt->buf + evt->lpos + count] before you will giveback this to HW (to be sure HW will not overwrite this in case lot of events)? >> + dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count); >> + After that evt->buf[lpos, lpos + count] seems goes back to HW, so thread should not rely on this? Or I miss something? >> /* Mask interrupt */ >> reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0)); >> reg |= DWC3_GEVNTSIZ_INTMASK; >> -- > > Hello, > > Are we sure this will work fine with 2.60a? > > Some time ago I have similar code (introduce event_pop) and move > dwc3_write(dwc->regs, DWC3_GEVNTCOUNT(0), 4) > before > dwc3_process_event_entry() > > And have some issues ... > Didn't work correctly in my case. > > BR > Janusz -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/5] usb: dwc3: gadget: Write the event count in interrupt
On 4 November 2016 at 02:31, John Youn wrote: > > Since we are saving the event count and handling the events in the > threaded interrupt handler, we can write and clear out the eventcount in > the hard interrupt handler itself. > > This behavior will be required for IP 3.00a cores that need to use > interrupt moderation as a workaround for an RTL issue were the interrupt > line cannot be masked between the hard/soft interrupt handler. > > Signed-off-by: John Youn > --- > drivers/usb/dwc3/gadget.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index a9c1d75..ac9eb39 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -2877,8 +2877,6 @@ static irqreturn_t dwc3_process_event_buf(struct > dwc3_event_buffer *evt) > */ > evt->lpos = (evt->lpos + 4) % DWC3_EVENT_BUFFERS_SIZE; > left -= 4; > - > - dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), 4); > } > > evt->count = 0; > @@ -2928,6 +2926,8 @@ static irqreturn_t dwc3_check_event_buf(struct > dwc3_event_buffer *evt) > evt->count = count; > evt->flags |= DWC3_EVENT_PENDING; > > + dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count); > + > /* Mask interrupt */ > reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0)); > reg |= DWC3_GEVNTSIZ_INTMASK; > -- Hello, Are we sure this will work fine with 2.60a? Some time ago I have similar code (introduce event_pop) and move dwc3_write(dwc->regs, DWC3_GEVNTCOUNT(0), 4) before dwc3_process_event_entry() And have some issues ... Didn't work correctly in my case. BR Janusz -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/5] usb: dwc3: gadget: Write the event count in interrupt
Since we are saving the event count and handling the events in the threaded interrupt handler, we can write and clear out the eventcount in the hard interrupt handler itself. This behavior will be required for IP 3.00a cores that need to use interrupt moderation as a workaround for an RTL issue were the interrupt line cannot be masked between the hard/soft interrupt handler. Signed-off-by: John Youn --- drivers/usb/dwc3/gadget.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index a9c1d75..ac9eb39 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2877,8 +2877,6 @@ static irqreturn_t dwc3_process_event_buf(struct dwc3_event_buffer *evt) */ evt->lpos = (evt->lpos + 4) % DWC3_EVENT_BUFFERS_SIZE; left -= 4; - - dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), 4); } evt->count = 0; @@ -2928,6 +2926,8 @@ static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt) evt->count = count; evt->flags |= DWC3_EVENT_PENDING; + dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count); + /* Mask interrupt */ reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0)); reg |= DWC3_GEVNTSIZ_INTMASK; -- 2.10.0 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
