Re: [RFC][PATCH 1/3 v2] usb: dwc2: Add extcon support to dwc2 driver

[John Youn <John.Youn@synopsys.com>]

On 12/6/2016 4:05 PM, John Stultz wrote:
> On Tue, Dec 6, 2016 at 3:17 PM, John Youn <John.Youn@synopsys.com> wrote:
>> On 12/6/2016 12:06 AM, John Stultz wrote:
>>> This patch wires up extcon support to the dwc2 driver
>>> so that devices that use a modern generic phy driver
>>> and don't use the usb-phy infrastructure can still
>>> signal connect/disconnect events.
>>>
>>> Cc: Wei Xu <xuwei5@hisilicon.com>
>>> Cc: Guodong Xu <guodong.xu@linaro.org>
>>> Cc: Amit Pundir <amit.pundir@linaro.org>
>>> Cc: Rob Herring <robh+dt@kernel.org>
>>> Cc: John Youn <johnyoun@synopsys.com>
>>> Cc: Douglas Anderson <dianders@chromium.org>
>>> Cc: Chen Yu <chenyu56@huawei.com>
>>> Cc: Kishon Vijay Abraham I <kishon@ti.com>
>>> Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Cc: linux-usb@vger.kernel.org
>>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>>> ---
>>> v2:
>>> * Move extcon logic from generic phy to dwc2 driver, as
>>>   suggested by Kishon.
>>> ---
>>>  arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 11 +++++++++
>>>  drivers/usb/dwc2/core.h                   | 16 ++++++++++++
>>>  drivers/usb/dwc2/core_intr.c              | 25 +++++++++++++++++++
>>>  drivers/usb/dwc2/hcd.c                    | 23 +++++++++++++++++
>>>  drivers/usb/dwc2/platform.c               | 41 +++++++++++++++++++++++++++++++
>>>  5 files changed, 116 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
>>> index 17839db..8a86a11 100644
>>> --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
>>> +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
>>> @@ -732,6 +732,16 @@
>>>                       regulator-always-on;
>>>               };
>>>
>>> +             usb_vbus: usb-vbus {
>>> +                     compatible = "linux,extcon-usb-gpio";
>>> +                     id-gpio = <&gpio2 6 1>;
>>> +             };
>>> +
>>> +             usb_id: usb-id {
>>> +                     compatible = "linux,extcon-usb-gpio";
>>> +                     id-gpio = <&gpio2 5 1>;
>>> +             };
>>> +
>>
>> So you are using extcon-usb-gpio driver to detect both the ID pin and
>> VBUS status correct? Do you need the VBUS one? It doesn't look like
>> you are using it.
> 
> Not at the moment. Apologies for my ignorance, I'm not totally
> familiar with the usage of the vbus vs id pins, so I'm somewhat
> following what I was seeing from other drivers. I know with a usb OTG
> to usb A adapter, you get the vbus signal but not the id signal, but I
> don't quite see what should be done differently in that case (as it
> seems to work ok).

Hi John,

The ID pin indicates which end of the cable is connected to the
controller port, determining whether it should take the role of an
A-device or B-device. If this is not visible to the controller (thus
the controller would not give CONNIDSTSCHNG interrupt), that is why
you would need to hook up the extcon module.

I'm thinking this shouldn't be needed for you since you can see this
interrupt.

> 
>> Also, do you really need this at all? Wasn't your system previously
>> able to detect the ID pin change correctly via the connection id
>> status change interrupt? This would only be needed if that were not
>> the case.
> 
> So it can be made work w/o this, but we needed other hacks because the
> usb-gadget disconnect logic never triggered when the cable was
> unplugged. The controller would jump over to host mode, then when we
> re-plugged in the usb-gadget cable, it would fail often as we never
> got a disconnect signal.  That's why earlier I was using this hack to
> force gadget disconnect before the reset was called:
> https://lkml.org/lkml/2016/10/20/26

Other than the triggering WARN_ON() in fifo init, is there any other
negative effects?

We are revisiting this fifo init code and I think the fifo init is not
necessary for USB_RESET purposes. This should get rid of a race
condition where the EP's are not disabled before attempting to
initialize their FIFO's. Which should get rid of the WARN_ON().

If this is the only issue, then this will probably resolve it.

Regards,
John