[PATCH 0/6] usb: dwc3: Interrupt moderation

[PATCH 0/6] usb: dwc3: Interrupt moderation

[John Youn]

This patch series implements interrupt moderation and also uses it in
implementing a workaround for STAR 9000961433.


John Youn (6):
  usb: dwc3: Add a check for the DWC_usb3 core
  usb: dwc3: Add a function to check properties
  Documentation: devicetree: dwc3: Add interrupt moderation
  usb: dwc3: gadget: Write the event count in interrupt
  usb: dwc3: Implement interrupt moderation
  usb: dwc3: Workaround for irq mask issue

 Documentation/devicetree/bindings/usb/dwc3.txt |  1 +
 drivers/usb/dwc3/core.c                        | 89 ++++++++++++++++++--------
 drivers/usb/dwc3/core.h                        | 21 ++++++
 drivers/usb/dwc3/gadget.c                      | 20 +++++-
 4 files changed, 103 insertions(+), 28 deletions(-)

-- 
2.10.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/6] Documentation: devicetree: dwc3: Add interrupt moderation

[John Youn]

Add interrupt moderation interval binding for dwc3.

Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
---
 Documentation/devicetree/bindings/usb/dwc3.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
index e3e6983..17de9fc 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -53,6 +53,7 @@ Optional properties:
  - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
 	register for post-silicon frame length adjustment when the
 	fladj_30mhz_sdbnd signal is invalid or incorrect.
+ - snps,imod_interval: the interrupt moderation interval.
 
  - <DEPRECATED> tx-fifo-resize: determines if the FIFO *has* to be reallocated.
 
-- 
2.10.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/6] Documentation: devicetree: dwc3: Add interrupt moderation

[John Youn]

Add interrupt moderation interval binding for dwc3.

Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
---
 Documentation/devicetree/bindings/usb/dwc3.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
index e3e6983..17de9fc 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -53,6 +53,7 @@ Optional properties:
  - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
 	register for post-silicon frame length adjustment when the
 	fladj_30mhz_sdbnd signal is invalid or incorrect.
+ - snps,imod_interval: the interrupt moderation interval.
 
  - <DEPRECATED> tx-fifo-resize: determines if the FIFO *has* to be reallocated.
 
-- 
2.10.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Documentation: devicetree: dwc3: Add interrupt moderation

[Sergei Shtylyov]

Hello.

On 10/25/2016 10:42 PM, John Youn wrote:

> Add interrupt moderation interval binding for dwc3.
>
> Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/usb/dwc3.txt | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> index e3e6983..17de9fc 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -53,6 +53,7 @@ Optional properties:
>   - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
>  	register for post-silicon frame length adjustment when the
>  	fladj_30mhz_sdbnd signal is invalid or incorrect.
> + - snps,imod_interval: the interrupt moderation interval.

    Hyphens are preferred in the DT node/prop names.

[...]

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Documentation: devicetree: dwc3: Add interrupt moderation

[Mark Rutland]

On Tue, Oct 25, 2016 at 12:42:46PM -0700, John Youn wrote:
> Add interrupt moderation interval binding for dwc3.
> 
> Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/usb/dwc3.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> index e3e6983..17de9fc 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -53,6 +53,7 @@ Optional properties:
>   - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
>  	register for post-silicon frame length adjustment when the
>  	fladj_30mhz_sdbnd signal is invalid or incorrect.
> + - snps,imod_interval: the interrupt moderation interval.

As otherwise commented, s/_/-/

What is "interrupt moderation"? The cover mentions that this is to be
used for some kind of workaround, but it's not clear to me what this is,
and as such, whether it makes sense to describe it in this manner.

Thanks,
Mark.

>  
>   - <DEPRECATED> tx-fifo-resize: determines if the FIFO *has* to be reallocated.
>  
> -- 
> 2.10.0
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Documentation: devicetree: dwc3: Add interrupt moderation

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 976 bytes --]


Hi,

John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org> writes:
> Add interrupt moderation interval binding for dwc3.
>
> Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/usb/dwc3.txt | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> index e3e6983..17de9fc 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -53,6 +53,7 @@ Optional properties:
>   - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
>  	register for post-silicon frame length adjustment when the
>  	fladj_30mhz_sdbnd signal is invalid or incorrect.
> + - snps,imod_interval: the interrupt moderation interval.

on top of all other comments, what's the unit here? nanoseconds? clock cycles?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

Re: [PATCH 3/6] Documentation: devicetree: dwc3: Add interrupt moderation

[John Youn]

On 10/26/2016 3:22 AM, Sergei Shtylyov wrote:
> Hello.
> 
> On 10/25/2016 10:42 PM, John Youn wrote:
> 
>> Add interrupt moderation interval binding for dwc3.
>>
>> Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/usb/dwc3.txt | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>> index e3e6983..17de9fc 100644
>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>> @@ -53,6 +53,7 @@ Optional properties:
>>   - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
>>  	register for post-silicon frame length adjustment when the
>>  	fladj_30mhz_sdbnd signal is invalid or incorrect.
>> + - snps,imod_interval: the interrupt moderation interval.
> 
>     Hyphens are preferred in the DT node/prop names.
> 

Thanks will fix.

John


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Documentation: devicetree: dwc3: Add interrupt moderation

[John Youn]

On 10/26/2016 3:57 AM, Mark Rutland wrote:
> On Tue, Oct 25, 2016 at 12:42:46PM -0700, John Youn wrote:
>> Add interrupt moderation interval binding for dwc3.
>>
>> Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/usb/dwc3.txt | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>> index e3e6983..17de9fc 100644
>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>> @@ -53,6 +53,7 @@ Optional properties:
>>   - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
>>  	register for post-silicon frame length adjustment when the
>>  	fladj_30mhz_sdbnd signal is invalid or incorrect.
>> + - snps,imod_interval: the interrupt moderation interval.
> 
> As otherwise commented, s/_/-/

Ok, forgot about that.

> 
> What is "interrupt moderation"? The cover mentions that this is to be
> used for some kind of workaround, but it's not clear to me what this is,
> and as such, whether it makes sense to describe it in this manner.
> 

Interrupt moderation throttles the interrupt rate to be no faster than
a specified interval. It's an optional feature of the controller.

This series implements the feature and enables it as a workaround for
a particular version of the controller.

This binding enables and configures the feature with an interval value
which is specified as a number of 250 ns increments.

I'll update the binding text to something like this:

 - snps,imod-interval: the interrupt moderation interval, specified in
   250 ns increments from 1 to 0xffff.


Regards,
John
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Documentation: devicetree: dwc3: Add interrupt moderation

[John Youn]

On 10/27/2016 3:47 AM, Felipe Balbi wrote:
> 
> Hi,
> 
> John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org> writes:
>> Add interrupt moderation interval binding for dwc3.
>>
>> Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/usb/dwc3.txt | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>> index e3e6983..17de9fc 100644
>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>> @@ -53,6 +53,7 @@ Optional properties:
>>   - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
>>  	register for post-silicon frame length adjustment when the
>>  	fladj_30mhz_sdbnd signal is invalid or incorrect.
>> + - snps,imod_interval: the interrupt moderation interval.
> 
> on top of all other comments, what's the unit here? nanoseconds? clock cycles?
> 

Number of 250 ns intervals. I'll update the description to clarify.

Regards,
John

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Documentation: devicetree: dwc3: Add interrupt moderation

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1348 bytes --]


Hi,

John Youn <John.Youn-HKixBCOQz3hWk0Htik3J/w@public.gmane.org> writes:
> On 10/27/2016 3:47 AM, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org> writes:
>>> Add interrupt moderation interval binding for dwc3.
>>>
>>> Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
>>> ---
>>>  Documentation/devicetree/bindings/usb/dwc3.txt | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>>> index e3e6983..17de9fc 100644
>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>> @@ -53,6 +53,7 @@ Optional properties:
>>>   - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
>>>  	register for post-silicon frame length adjustment when the
>>>  	fladj_30mhz_sdbnd signal is invalid or incorrect.
>>> + - snps,imod_interval: the interrupt moderation interval.
>> 
>> on top of all other comments, what's the unit here? nanoseconds? clock cycles?
>> 
>
> Number of 250 ns intervals. I'll update the description to clarify.

it's probably better to add it in nanoseconds itself, then let driver
compute register value with DIV_ROUND_UP()

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

Re: [PATCH 3/6] Documentation: devicetree: dwc3: Add interrupt moderation

[Mark Rutland]

On Thu, Oct 27, 2016 at 02:08:25PM -0700, John Youn wrote:
> On 10/26/2016 3:57 AM, Mark Rutland wrote:
> > On Tue, Oct 25, 2016 at 12:42:46PM -0700, John Youn wrote:
> >> Add interrupt moderation interval binding for dwc3.

> >> + - snps,imod_interval: the interrupt moderation interval.

> > What is "interrupt moderation"? The cover mentions that this is to be
> > used for some kind of workaround, but it's not clear to me what this is,
> > and as such, whether it makes sense to describe it in this manner.
> > 
> 
> Interrupt moderation throttles the interrupt rate to be no faster than
> a specified interval. It's an optional feature of the controller.

Ok.

> This series implements the feature and enables it as a workaround for
> a particular version of the controller.

... as a workaround for *what*? Is there a bug in that IP version, or an
integration issue? Does the problem vary per-board?

Generally, if there's a problem that needs to be worked around, we
describe the problem in the DT (perhaps implicitly in the compatible
string), and then the kernel chooses the workaround.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Documentation: devicetree: dwc3: Add interrupt moderation

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3397 bytes --]


Hi,

Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> writes:
> On Thu, Oct 27, 2016 at 02:08:25PM -0700, John Youn wrote:
>> On 10/26/2016 3:57 AM, Mark Rutland wrote:
>> > On Tue, Oct 25, 2016 at 12:42:46PM -0700, John Youn wrote:
>> >> Add interrupt moderation interval binding for dwc3.
>
>> >> + - snps,imod_interval: the interrupt moderation interval.
>
>> > What is "interrupt moderation"? The cover mentions that this is to be
>> > used for some kind of workaround, but it's not clear to me what this is,
>> > and as such, whether it makes sense to describe it in this manner.
>> > 
>> 
>> Interrupt moderation throttles the interrupt rate to be no faster than
>> a specified interval. It's an optional feature of the controller.
>
> Ok.
>
>> This series implements the feature and enables it as a workaround for
>> a particular version of the controller.
>
> ... as a workaround for *what*? Is there a bug in that IP version, or an

you didn't receive the entire series, I guess. Here's last patch in the
series:

 This is a workaround for STAR 9000961433 which affects only version
 3.00a of the DWC_usb3 core. This prevents the controller interrupt from
 being masked while handling events. Enabling interrupt moderation allows
 us to work around this issue because once the GEVNTCOUNT.count is
 written the IRQ is immediately deasserted and won't be asserted again
 until GEVNTCOUNT.EHB is cleared.

 Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
 ---
  drivers/usb/dwc3/core.c | 12 ++++++++++++
  1 file changed, 12 insertions(+)

 diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
 index 6733838..7fa0832 100644
 --- a/drivers/usb/dwc3/core.c
 +++ b/drivers/usb/dwc3/core.c
 @@ -1050,6 +1050,18 @@ static void dwc3_check_params(struct dwc3 *dwc)
		 dwc->imod_interval = 0;
	 }

 +	/*
 +	 * Workaround for STAR 9000961433 which affects only version
 +	 * 3.00a of the DWC_usb3 core. This prevents the controller
 +	 * interrupt from being masked while handling events. IMOD
 +	 * allows us to work around this issue. Enable it for the
 +	 * affected version.
 +	 */
 +	if (!dwc->imod_interval &&
 +	    (dwc->revision == DWC3_REVISION_300A)) {
 +		dwc->imod_interval = 1;
 +	}
 +
	 /* Check the maximum_speed parameter */
	 switch (dwc->maximum_speed) {
	 case USB_SPEED_LOW:

> integration issue? Does the problem vary per-board?
>
> Generally, if there's a problem that needs to be worked around, we
> describe the problem in the DT (perhaps implicitly in the compatible
> string), and then the kernel chooses the workaround.

Regardless of the silicon erratum, interrupt moderation is a *feature*
of the IP, common to all instances since revision v3.00a (IIRC). John is
just using interrupt moderation in the context of implementing this
workaround. But the actual feature is valid also without the erratum.

Another thing to remember is that different applications (i.e. boards)
might want to moderate the interrupt for different periods. That's,
again, not related to the erratum at all.

We cannot use compatible to figure this one out. dwc3 does as much
runtime discovery as possible, but we cannot discover what is the
desired interrupt moderation interval for $this setup. It needs to be
passed in on a board-by-board basis.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

Re: [PATCH 3/6] Documentation: devicetree: dwc3: Add interrupt moderation

[Mark Rutland]

On Fri, Oct 28, 2016 at 01:30:07PM +0300, Felipe Balbi wrote:
> Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> writes:
> > On Thu, Oct 27, 2016 at 02:08:25PM -0700, John Youn wrote:
> >> On 10/26/2016 3:57 AM, Mark Rutland wrote:
> >> > On Tue, Oct 25, 2016 at 12:42:46PM -0700, John Youn wrote:
> >> >> Add interrupt moderation interval binding for dwc3.
> >
> >> >> + - snps,imod_interval: the interrupt moderation interval.

> >> This series implements the feature and enables it as a workaround for
> >> a particular version of the controller.
> >
> > ... as a workaround for *what*? Is there a bug in that IP version, or an
> 
> you didn't receive the entire series, I guess. Here's last patch in the
> series:

No, I did not. Thanks for forwarding this.

>  This is a workaround for STAR 9000961433 which affects only version
>  3.00a of the DWC_usb3 core. This prevents the controller interrupt from
>  being masked while handling events. Enabling interrupt moderation allows
>  us to work around this issue because once the GEVNTCOUNT.count is
>  written the IRQ is immediately deasserted and won't be asserted again
>  until GEVNTCOUNT.EHB is cleared.
> 
>  Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
>  ---
>   drivers/usb/dwc3/core.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
>  diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>  index 6733838..7fa0832 100644
>  --- a/drivers/usb/dwc3/core.c
>  +++ b/drivers/usb/dwc3/core.c
>  @@ -1050,6 +1050,18 @@ static void dwc3_check_params(struct dwc3 *dwc)
> 		 dwc->imod_interval = 0;
> 	 }
> 
>  +	/*
>  +	 * Workaround for STAR 9000961433 which affects only version
>  +	 * 3.00a of the DWC_usb3 core. This prevents the controller
>  +	 * interrupt from being masked while handling events. IMOD
>  +	 * allows us to work around this issue. Enable it for the
>  +	 * affected version.
>  +	 */
>  +	if (!dwc->imod_interval &&
>  +	    (dwc->revision == DWC3_REVISION_300A)) {
>  +		dwc->imod_interval = 1;
>  +	}
>  +
> 	 /* Check the maximum_speed parameter */
> 	 switch (dwc->maximum_speed) {
> 	 case USB_SPEED_LOW:
> 
> > integration issue? Does the problem vary per-board?
> >
> > Generally, if there's a problem that needs to be worked around, we
> > describe the problem in the DT (perhaps implicitly in the compatible
> > string), and then the kernel chooses the workaround.
> 
> Regardless of the silicon erratum, interrupt moderation is a *feature*
> of the IP, common to all instances since revision v3.00a (IIRC). John is
> just using interrupt moderation in the context of implementing this
> workaround. But the actual feature is valid also without the erratum.

Sure, I understand this.

> Another thing to remember is that different applications (i.e. boards)
> might want to moderate the interrupt for different periods. That's,
> again, not related to the erratum at all.

... again, the question is *why*?

If this varies per use-case, then it would be better to handle this
dynamically -- people can run wildly different use-cases on the same
hardware.

I'm not sure that it makes sense for this to be in the DT, though I may
have misunderstood.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Documentation: devicetree: dwc3: Add interrupt moderation

[John Youn]

On 10/28/2016 3:39 AM, Mark Rutland wrote:
> On Fri, Oct 28, 2016 at 01:30:07PM +0300, Felipe Balbi wrote:
>> Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> writes:
>>> On Thu, Oct 27, 2016 at 02:08:25PM -0700, John Youn wrote:
>>>> On 10/26/2016 3:57 AM, Mark Rutland wrote:
>>>>> On Tue, Oct 25, 2016 at 12:42:46PM -0700, John Youn wrote:
>>>>>> Add interrupt moderation interval binding for dwc3.
>>>
>>>>>> + - snps,imod_interval: the interrupt moderation interval.
> 
>>>> This series implements the feature and enables it as a workaround for
>>>> a particular version of the controller.
>>>
>>> ... as a workaround for *what*? Is there a bug in that IP version, or an
>>
>> you didn't receive the entire series, I guess. Here's last patch in the
>> series:
> 
> No, I did not. Thanks for forwarding this.
> 
>>  This is a workaround for STAR 9000961433 which affects only version
>>  3.00a of the DWC_usb3 core. This prevents the controller interrupt from
>>  being masked while handling events. Enabling interrupt moderation allows
>>  us to work around this issue because once the GEVNTCOUNT.count is
>>  written the IRQ is immediately deasserted and won't be asserted again
>>  until GEVNTCOUNT.EHB is cleared.
>>
>>  Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
>>  ---
>>   drivers/usb/dwc3/core.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>>  diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>  index 6733838..7fa0832 100644
>>  --- a/drivers/usb/dwc3/core.c
>>  +++ b/drivers/usb/dwc3/core.c
>>  @@ -1050,6 +1050,18 @@ static void dwc3_check_params(struct dwc3 *dwc)
>> 		 dwc->imod_interval = 0;
>> 	 }
>>
>>  +	/*
>>  +	 * Workaround for STAR 9000961433 which affects only version
>>  +	 * 3.00a of the DWC_usb3 core. This prevents the controller
>>  +	 * interrupt from being masked while handling events. IMOD
>>  +	 * allows us to work around this issue. Enable it for the
>>  +	 * affected version.
>>  +	 */
>>  +	if (!dwc->imod_interval &&
>>  +	    (dwc->revision == DWC3_REVISION_300A)) {
>>  +		dwc->imod_interval = 1;
>>  +	}
>>  +
>> 	 /* Check the maximum_speed parameter */
>> 	 switch (dwc->maximum_speed) {
>> 	 case USB_SPEED_LOW:
>>
>>> integration issue? Does the problem vary per-board?
>>>
>>> Generally, if there's a problem that needs to be worked around, we
>>> describe the problem in the DT (perhaps implicitly in the compatible
>>> string), and then the kernel chooses the workaround.
>>
>> Regardless of the silicon erratum, interrupt moderation is a *feature*
>> of the IP, common to all instances since revision v3.00a (IIRC). John is
>> just using interrupt moderation in the context of implementing this
>> workaround. But the actual feature is valid also without the erratum.
> 
> Sure, I understand this.
> 
>> Another thing to remember is that different applications (i.e. boards)
>> might want to moderate the interrupt for different periods. That's,
>> again, not related to the erratum at all.
> 
> ... again, the question is *why*?
> 
> If this varies per use-case, then it would be better to handle this
> dynamically -- people can run wildly different use-cases on the same
> hardware.

It could make sense either per use-case or per platform.

If we add a dynamic setting, would that be through sysfs?

> 
> I'm not sure that it makes sense for this to be in the DT, though I may
> have misunderstood.

I'm ok omitting the DT binding until it is needed by someone. Since we
currently only need it for the workaround, which is detected
automatically.

Felipe, are you ok with this?

Regards,
John
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Documentation: devicetree: dwc3: Add interrupt moderation

[John Youn]

On 10/28/2016 3:09 AM, Felipe Balbi wrote:
> 
> Hi,
> 
> John Youn <John.Youn-HKixBCOQz3hWk0Htik3J/w@public.gmane.org> writes:
>> On 10/27/2016 3:47 AM, Felipe Balbi wrote:
>>>
>>> Hi,
>>>
>>> John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org> writes:
>>>> Add interrupt moderation interval binding for dwc3.
>>>>
>>>> Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
>>>> ---
>>>>  Documentation/devicetree/bindings/usb/dwc3.txt | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>> index e3e6983..17de9fc 100644
>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>> @@ -53,6 +53,7 @@ Optional properties:
>>>>   - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
>>>>  	register for post-silicon frame length adjustment when the
>>>>  	fladj_30mhz_sdbnd signal is invalid or incorrect.
>>>> + - snps,imod_interval: the interrupt moderation interval.
>>>
>>> on top of all other comments, what's the unit here? nanoseconds? clock cycles?
>>>
>>
>> Number of 250 ns intervals. I'll update the description to clarify.
> 
> it's probably better to add it in nanoseconds itself, then let driver
> compute register value with DIV_ROUND_UP()
> 

I'm fine with it either way, but I think "increments of 250 ns" is
slightly cleaner in that it reflects the exact settings that are
possible and documented, and also fits neatly into a u16.

I assume you'd also like the internal variable dwc3->imod_interval
changed as well?

Regards,
John


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Documentation: devicetree: dwc3: Add interrupt moderation

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 430 bytes --]


Hi,

John Youn <John.Youn-HKixBCOQz3hWk0Htik3J/w@public.gmane.org> writes:
>> I'm not sure that it makes sense for this to be in the DT, though I may
>> have misunderstood.
>
> I'm ok omitting the DT binding until it is needed by someone. Since we
> currently only need it for the workaround, which is detected
> automatically.
>
> Felipe, are you ok with this?

certainly. No problems whatsoever :-)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

Re: [PATCH 3/6] Documentation: devicetree: dwc3: Add interrupt moderation

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1800 bytes --]


Hi,

John Youn <John.Youn-HKixBCOQz3hWk0Htik3J/w@public.gmane.org> writes:
>>>> John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org> writes:
>>>>> Add interrupt moderation interval binding for dwc3.
>>>>>
>>>>> Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
>>>>> ---
>>>>>  Documentation/devicetree/bindings/usb/dwc3.txt | 1 +
>>>>>  1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> index e3e6983..17de9fc 100644
>>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> @@ -53,6 +53,7 @@ Optional properties:
>>>>>   - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
>>>>>  	register for post-silicon frame length adjustment when the
>>>>>  	fladj_30mhz_sdbnd signal is invalid or incorrect.
>>>>> + - snps,imod_interval: the interrupt moderation interval.
>>>>
>>>> on top of all other comments, what's the unit here? nanoseconds? clock cycles?
>>>>
>>>
>>> Number of 250 ns intervals. I'll update the description to clarify.
>> 
>> it's probably better to add it in nanoseconds itself, then let driver
>> compute register value with DIV_ROUND_UP()
>> 
>
> I'm fine with it either way, but I think "increments of 250 ns" is
> slightly cleaner in that it reflects the exact settings that are
> possible and documented, and also fits neatly into a u16.

I don't know, I'll leave this to Mark and the other devicetree folks,
but I remember there was a preference of not passing register values via
devicetree. What if a following HW revision decides to change 250ns
increments to 125ns increments?

Mark?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

Re: [PATCH 3/6] Documentation: devicetree: dwc3: Add interrupt moderation

[Mark Rutland]

On Tue, Nov 01, 2016 at 01:18:17PM +0200, Felipe Balbi wrote:
> 
> Hi,
> 
> John Youn <John.Youn-HKixBCOQz3hWk0Htik3J/w@public.gmane.org> writes:
> >>>> John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org> writes:
> >>>>> Add interrupt moderation interval binding for dwc3.
> >>>>>
> >>>>> Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> >>>>> ---
> >>>>>  Documentation/devicetree/bindings/usb/dwc3.txt | 1 +
> >>>>>  1 file changed, 1 insertion(+)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> >>>>> index e3e6983..17de9fc 100644
> >>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> >>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> >>>>> @@ -53,6 +53,7 @@ Optional properties:
> >>>>>   - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
> >>>>>  	register for post-silicon frame length adjustment when the
> >>>>>  	fladj_30mhz_sdbnd signal is invalid or incorrect.
> >>>>> + - snps,imod_interval: the interrupt moderation interval.
> >>>>
> >>>> on top of all other comments, what's the unit here? nanoseconds? clock cycles?
> >>>>
> >>>
> >>> Number of 250 ns intervals. I'll update the description to clarify.
> >> 
> >> it's probably better to add it in nanoseconds itself, then let driver
> >> compute register value with DIV_ROUND_UP()
> >> 
> >
> > I'm fine with it either way, but I think "increments of 250 ns" is
> > slightly cleaner in that it reflects the exact settings that are
> > possible and documented, and also fits neatly into a u16.
> 
> I don't know, I'll leave this to Mark and the other devicetree folks,
> but I remember there was a preference of not passing register values via
> devicetree. What if a following HW revision decides to change 250ns
> increments to 125ns increments?
> 
> Mark?

Generally, directly human-readable units are preferable, with some
validation/reporting left to the driver for cases the HW cannot support.
In this case, some *-interval-ns property would be better.

That said, it sounds like we're all agreed that we don't need this
property for now. :)

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html