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

[John Youn <John.Youn-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>]

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