Re: [PATCH v2 3/4] usb: dwc3: add dual-role support

From: John Youn <John.Youn@synopsys.com>
To: Felipe Balbi <felipe.balbi@linux.intel.com>,
	Roger Quadros <rogerq@ti.com>,
	Mathias Nyman <mathias.nyman@linux.intel.com>,
	John Youn <John.Youn@synopsys.com>
Cc: "vivek.gautam@codeaurora.org" <vivek.gautam@codeaurora.org>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 3/4] usb: dwc3: add dual-role support
Date: Mon, 3 Apr 2017 05:31:39 +0000	[thread overview]
Message-ID: <2B3535C5ECE8B5419E3ECBE300772909021B4A488C@US01WEMBX2.internal.synopsys.com> (raw)
In-Reply-To: 874lybktcp.fsf@linux.intel.com

On 03/30/2017 02:27 AM, Felipe Balbi wrote:
>
> Hi
>
> Roger Quadros <rogerq@ti.com> writes:
>>>>> For something that simple, we wouldn't even need to use OTG FSM layer
>>>>> because that brings no benefit for such a simple requirement.
>>>>
>>>> no no. I think you got it wrong. I'm not using the OTG FSM layer at all :).
>>>
>>> what are all the otg_fsm mentions then? Also you have:
>>>
>>>  +	struct usb_otg		otg;
>>>  +	struct otg_fsm		otg_fsm;
>>>
>>
>> I'll get rid of them. They aren't really needed.
>> Now I see why you thought I was using the otg_fsm layer. :)
>
> fair enough
>
>>>>> Can you either confirm or refute the statement above?
>>>>
>>>> The real problem was that if host adapter was removed during a system suspend
>>>> then while resuming XHCI was locking up like below. This is probably because
>>>> we're trying to remove/Halt the HCD in the otg_irq_handler before XHCI driver has resumed?
>>>>
>>>> How can we ensure that we call dwc3_host_exit() only *after* XHCI driver has resumed?
>>>
>>> Well, xHCI is our child, so driver model should *already* be
>>> guaranteeing that, no? Specially since you're calling this from
>>> ->complete() which happens after ->resume() has been called for the
>>> entire tree. It's true, however, that dwc3's ->complete() will be called
>>> before xhci's ->complete(). Is this the problem you're pointing at? Or
>>> do you mean xHCI is runtime suspended (or runtime resuming) while you
>>> call dwc3_host_exit()? If that's the case, then there's a bug in xHCI
>>> itself.
>>
>> Yes dwc3->complete() being called before xhci->complete(), and so HCD being
>> removed before xhci->complete() causes the lockup.
>>
>> It could be a bug in xHCI driver as well.
>
> I see...
>
>>>> We need a way to mask the OTG events without loosing them while they are masked.
>>>> Do you know how that could be achieved?
>>>
>>> masking doesn't clear events. It just masks them. Look at gadget.c for
>>> how we do it. Basically, the code we have here is racy, really racy and
>>> will cause hard-to-debug lockups. Your code can only work with
>>> IRQF_ONESHOT, which we don't want to add back.
>>>
>>> In any case, to mask events, you set BIT 31 in GEVNTSIZ register. Just
>>> copy it from gadget.c ;-)
>>
>> Isn't GEVNTSIZ specific to device side? We're talking about the OTG block here.
>
> that's true, sorry.
>
>> Are you sure that setting bit 31 of GEVNTSIZ will mask OTG_irq? I don't think so.
>>
>> Note that OTG_IRQ is a separate IRQ line than PERIPHERAL_IRQ.
>
> man, there's really no way to mask OTG events. This can be bad :-)
>
> John, can you confirm if there's really no way of masking OTG events
> without loosing them?

Not sure. I'll have to verify.

If the IP version is 3.00a or higher you could experiment with using
interrupt moderation as a global mask.

John