Patchwork [RFC,1/3,v2] usb: dwc2: Add extcon support to dwc2 driver

login
register
mail settings
Submitter John Stultz
Date Dec. 6, 2016, 8:06 a.m.
Message ID <1481011582-7162-2-git-send-email-john.stultz@linaro.org>
Download mbox | patch
Permalink /patch/9462131/
State New
Headers show

Comments

John Stultz - Dec. 6, 2016, 8:06 a.m.
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(+)
John Youn - Dec. 6, 2016, 11:17 p.m.
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.

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.

>  		usb_phy: usbphy {
>  			compatible = "hisilicon,hi6220-usb-phy";
>  			#phy-cells = <0>;
> @@ -745,6 +755,7 @@
>  			phys = <&usb_phy>;
>  			phy-names = "usb2-phy";
>  			clocks = <&sys_ctrl HI6220_USBOTG_HCLK>;
> +			extcon = <&usb_vbus>, <&usb_id>;

You should document what should be set for "extcon" in
/Documentation/devicetree/bindings/usb/dwc2.txt. And make that a
separate commit before using the binding.

>  			clock-names = "otg";
>  			dr_mode = "otg";
>  			g-use-dma;
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 2a21a04..4cfce62 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -623,6 +623,13 @@ struct dwc2_hregs_backup {
>  	bool valid;
>  };
>  
> +struct dwc2_extcon {
> +	struct notifier_block	nb;
> +	struct extcon_dev	*extcon;
> +	int			state;
> +};
> +
> +

Don't use multiple blank lines. Please run "checkpatch.pl --strict"
and fix other issues it reports, if possible.

[snip]

> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index 8e1728b..2e4545f 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -46,6 +46,7 @@
>  #include <linux/phy/phy.h>
>  #include <linux/platform_data/s3c-hsotg.h>
>  #include <linux/reset.h>
> +#include <linux/extcon.h>
>  
>  #include <linux/usb/of.h>
>  
> @@ -543,6 +544,7 @@ static int dwc2_driver_probe(struct platform_device *dev)
>  	struct dwc2_core_params defparams;
>  	struct dwc2_hsotg *hsotg;
>  	struct resource *res;
> +	struct extcon_dev *ext_id, *ext_vbus;
>  	int retval;
>  
>  	match = of_match_device(dwc2_of_match_table, &dev->dev);
> @@ -620,6 +622,45 @@ static int dwc2_driver_probe(struct platform_device *dev)
>  	if (retval)
>  		goto error;
>  
> +	ext_id = ERR_PTR(-ENODEV);
> +	ext_vbus = ERR_PTR(-ENODEV);
> +	if (of_property_read_bool(dev->dev.of_node, "extcon")) {

You can omit this check since it's done in the api function.

> +		/* Each one of them is not mandatory */
> +		ext_vbus = extcon_get_edev_by_phandle(&dev->dev, 0);
> +		if (IS_ERR(ext_vbus) && PTR_ERR(ext_vbus) != -ENODEV)
> +			return PTR_ERR(ext_vbus);
> +
> +		ext_id = extcon_get_edev_by_phandle(&dev->dev, 1);
> +		if (IS_ERR(ext_id) && PTR_ERR(ext_id) != -ENODEV)
> +			return PTR_ERR(ext_id);
> +	}

Ensure when you initialize hsotg->extcon_vbus/id that they are either
valid and set or NULL so that you don't have to do IS_ERR() all the
time.

Regards,
John
John Stultz - Dec. 7, 2016, 12:04 a.m.
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).

> 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


>>               usb_phy: usbphy {
>>                       compatible = "hisilicon,hi6220-usb-phy";
>>                       #phy-cells = <0>;
>> @@ -745,6 +755,7 @@
>>                       phys = <&usb_phy>;
>>                       phy-names = "usb2-phy";
>>                       clocks = <&sys_ctrl HI6220_USBOTG_HCLK>;
>> +                     extcon = <&usb_vbus>, <&usb_id>;
>
> You should document what should be set for "extcon" in
> /Documentation/devicetree/bindings/usb/dwc2.txt. And make that a
> separate commit before using the binding.

Yes. I've already split it out, and added bindings documentation in my
tree. I included this here to make it easier to see what all was
involved w/o having to go through a bunch of patches.


>>                       clock-names = "otg";
>>                       dr_mode = "otg";
>>                       g-use-dma;
>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>> index 2a21a04..4cfce62 100644
>> --- a/drivers/usb/dwc2/core.h
>> +++ b/drivers/usb/dwc2/core.h
>> @@ -623,6 +623,13 @@ struct dwc2_hregs_backup {
>>       bool valid;
>>  };
>>
>> +struct dwc2_extcon {
>> +     struct notifier_block   nb;
>> +     struct extcon_dev       *extcon;
>> +     int                     state;
>> +};
>> +
>> +
>
> Don't use multiple blank lines. Please run "checkpatch.pl --strict"
> and fix other issues it reports, if possible.

Yes. Apologies. I noticed this once I sent it and fixed it up in my
tree (as well as a few other extra whitespace lines elsewhere).


> [snip]
>
>> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
>> index 8e1728b..2e4545f 100644
>> --- a/drivers/usb/dwc2/platform.c
>> +++ b/drivers/usb/dwc2/platform.c
>> @@ -46,6 +46,7 @@
>>  #include <linux/phy/phy.h>
>>  #include <linux/platform_data/s3c-hsotg.h>
>>  #include <linux/reset.h>
>> +#include <linux/extcon.h>
>>
>>  #include <linux/usb/of.h>
>>
>> @@ -543,6 +544,7 @@ static int dwc2_driver_probe(struct platform_device *dev)
>>       struct dwc2_core_params defparams;
>>       struct dwc2_hsotg *hsotg;
>>       struct resource *res;
>> +     struct extcon_dev *ext_id, *ext_vbus;
>>       int retval;
>>
>>       match = of_match_device(dwc2_of_match_table, &dev->dev);
>> @@ -620,6 +622,45 @@ static int dwc2_driver_probe(struct platform_device *dev)
>>       if (retval)
>>               goto error;
>>
>> +     ext_id = ERR_PTR(-ENODEV);
>> +     ext_vbus = ERR_PTR(-ENODEV);
>> +     if (of_property_read_bool(dev->dev.of_node, "extcon")) {
>
> You can omit this check since it's done in the api function.
>
>> +             /* Each one of them is not mandatory */
>> +             ext_vbus = extcon_get_edev_by_phandle(&dev->dev, 0);
>> +             if (IS_ERR(ext_vbus) && PTR_ERR(ext_vbus) != -ENODEV)
>> +                     return PTR_ERR(ext_vbus);
>> +
>> +             ext_id = extcon_get_edev_by_phandle(&dev->dev, 1);
>> +             if (IS_ERR(ext_id) && PTR_ERR(ext_id) != -ENODEV)
>> +                     return PTR_ERR(ext_id);
>> +     }
>
> Ensure when you initialize hsotg->extcon_vbus/id that they are either
> valid and set or NULL so that you don't have to do IS_ERR() all the
> time.

Will do! Thanks so much for the review!
-john
John Youn - Dec. 7, 2016, 12:26 a.m.
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
John Stultz - Dec. 7, 2016, 12:35 a.m.
On Tue, Dec 6, 2016 at 4:26 PM, John Youn <John.Youn@synopsys.com> wrote:
> 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:
>>> 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?

Well, when we see the WARN_ON, it doesn't connect into usb-gadget
mode. I had to unplug and re-plug the cable.
(The hack I linked to above avoids this, but I suspect its not correct).

Also Amit Pundir had mentioned earlier that the UDC sysfs state
doesn't get reported correctly since it doesn't register the
usb-gadget as unplugged until the cable is re-inserted.

> 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.

(Basically that and the two suspend fixes I sent along in this patchset :).

I'd be happy to test anything you're playing with.

thanks
-john
John Stultz - Dec. 7, 2016, 1:44 a.m.
On Tue, Dec 6, 2016 at 4:35 PM, John Stultz <john.stultz@linaro.org> wrote:
> On Tue, Dec 6, 2016 at 4:26 PM, John Youn <John.Youn@synopsys.com> wrote:
>> 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:
>>>> 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?
>
> Well, when we see the WARN_ON, it doesn't connect into usb-gadget
> mode. I had to unplug and re-plug the cable.
> (The hack I linked to above avoids this, but I suspect its not correct).
>
> Also Amit Pundir had mentioned earlier that the UDC sysfs state
> doesn't get reported correctly since it doesn't register the
> usb-gadget as unplugged until the cable is re-inserted.

Actually, this is still an outstanding issue, as
/sys/class/udc/f72c0000.usb/state seems to be stuck at "configured" no
matter the state of the cable, even with my patches. I'll need to look
further at the details there.

thanks
-john
Chanwoo Choi - Dec. 7, 2016, 3:54 a.m.
Hi John,

I give a some guide for extcon API.
This patch uses the deprecated extcon API (extcon_get_cable_state_).
So, I recommend that you better to use following extcon API:
- extcon_get_cable_state_()  -> extcon_get_state()
- extcon_register_notifier() -> devm_extcon_register_notifier()

Best Regards,
Chanwoo Choi

On 2016년 12월 06일 17:06, 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>;
> +		};
> +
>  		usb_phy: usbphy {
>  			compatible = "hisilicon,hi6220-usb-phy";
>  			#phy-cells = <0>;
> @@ -745,6 +755,7 @@
>  			phys = <&usb_phy>;
>  			phy-names = "usb2-phy";
>  			clocks = <&sys_ctrl HI6220_USBOTG_HCLK>;
> +			extcon = <&usb_vbus>, <&usb_id>;
>  			clock-names = "otg";
>  			dr_mode = "otg";
>  			g-use-dma;
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 2a21a04..4cfce62 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -623,6 +623,13 @@ struct dwc2_hregs_backup {
>  	bool valid;
>  };
>  
> +struct dwc2_extcon {
> +	struct notifier_block	nb;
> +	struct extcon_dev	*extcon;
> +	int			state;
> +};
> +
> +
>  /*
>   * Constants related to high speed periodic scheduling
>   *
> @@ -996,6 +1003,10 @@ struct dwc2_hsotg {
>  	u32 g_np_g_tx_fifo_sz;
>  	u32 g_tx_fifo_sz[MAX_EPS_CHANNELS];
>  #endif /* CONFIG_USB_DWC2_PERIPHERAL || CONFIG_USB_DWC2_DUAL_ROLE */
> +
> +	struct dwc2_extcon extcon_vbus;
> +	struct dwc2_extcon extcon_id;
> +	struct delayed_work extcon_work;
>  };
>  
>  /* Reasons for halting a host channel */
> @@ -1041,6 +1052,11 @@ extern void dwc2_flush_rx_fifo(struct dwc2_hsotg *hsotg);
>  extern void dwc2_enable_global_interrupts(struct dwc2_hsotg *hcd);
>  extern void dwc2_disable_global_interrupts(struct dwc2_hsotg *hcd);
>  
> +extern int dwc2_extcon_vbus_notifier(struct notifier_block *nb,
> +					unsigned long event, void *ptr);
> +extern int dwc2_extcon_id_notifier(struct notifier_block *nb,
> +					unsigned long event, void *ptr);
> +
>  /* This function should be called on every hardware interrupt. */
>  extern irqreturn_t dwc2_handle_common_intr(int irq, void *dev);
>  
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index d85c5c9..d4fa938 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -479,6 +479,31 @@ static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg)
>  	}
>  }
>  
> +
> +
> +int dwc2_extcon_vbus_notifier(struct notifier_block *nb,
> +					unsigned long event, void *ptr)
> +{
> +	struct dwc2_extcon *vbus = container_of(nb, struct dwc2_extcon, nb);
> +	struct dwc2_hsotg *hsotg = container_of(vbus, struct dwc2_hsotg,
> +						extcon_vbus);
> +
> +	schedule_delayed_work(&hsotg->extcon_work, msecs_to_jiffies(100));
> +	return NOTIFY_DONE;
> +}
> +
> +int dwc2_extcon_id_notifier(struct notifier_block *nb,
> +					unsigned long event, void *ptr)
> +{
> +	struct dwc2_extcon *id = container_of(nb, struct dwc2_extcon, nb);
> +	struct dwc2_hsotg *hsotg = container_of(id, struct dwc2_hsotg,
> +						extcon_id);
> +	schedule_delayed_work(&hsotg->extcon_work, msecs_to_jiffies(100));
> +
> +	return NOTIFY_DONE;
> +}
> +
> +
>  #define GINTMSK_COMMON	(GINTSTS_WKUPINT | GINTSTS_SESSREQINT |		\
>  			 GINTSTS_CONIDSTSCHNG | GINTSTS_OTGINT |	\
>  			 GINTSTS_MODEMIS | GINTSTS_DISCONNINT |		\
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index df5a065..61eea70 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -47,6 +47,7 @@
>  #include <linux/io.h>
>  #include <linux/slab.h>
>  #include <linux/usb.h>
> +#include <linux/extcon.h>
>  
>  #include <linux/usb/hcd.h>
>  #include <linux/usb/ch11.h>
> @@ -3246,6 +3247,26 @@ static void dwc2_conn_id_status_change(struct work_struct *work)
>  	}
>  }
>  
> +static void dwc2_hcd_extcon_func(struct work_struct *work)
> +{
> +	struct dwc2_hsotg *hsotg = container_of(work, struct dwc2_hsotg,
> +						extcon_work.work);
> +
> +	if (!IS_ERR(hsotg->extcon_vbus.extcon))
> +		hsotg->extcon_vbus.state =
> +			extcon_get_cable_state_(hsotg->extcon_vbus.extcon,
> +								EXTCON_USB);
> +	if (!IS_ERR(hsotg->extcon_id.extcon))
> +		hsotg->extcon_id.state =
> +			extcon_get_cable_state_(hsotg->extcon_id.extcon,
> +							EXTCON_USB_HOST);
> +
> +	if (hsotg->extcon_id.state)
> +		usb_gadget_vbus_connect(&hsotg->gadget);
> +	else
> +		usb_gadget_vbus_disconnect(&hsotg->gadget);
> +}
> +
>  static void dwc2_wakeup_detected(unsigned long data)
>  {
>  	struct dwc2_hsotg *hsotg = (struct dwc2_hsotg *)data;
> @@ -5085,6 +5106,8 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
>  	/* Initialize port reset work */
>  	INIT_DELAYED_WORK(&hsotg->reset_work, dwc2_hcd_reset_func);
>  
> +	INIT_DELAYED_WORK(&hsotg->extcon_work, dwc2_hcd_extcon_func);
> +
>  	/*
>  	 * Allocate space for storing data on status transactions. Normally no
>  	 * data is sent, but this space acts as a bit bucket. This must be
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index 8e1728b..2e4545f 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -46,6 +46,7 @@
>  #include <linux/phy/phy.h>
>  #include <linux/platform_data/s3c-hsotg.h>
>  #include <linux/reset.h>
> +#include <linux/extcon.h>
>  
>  #include <linux/usb/of.h>
>  
> @@ -543,6 +544,7 @@ static int dwc2_driver_probe(struct platform_device *dev)
>  	struct dwc2_core_params defparams;
>  	struct dwc2_hsotg *hsotg;
>  	struct resource *res;
> +	struct extcon_dev *ext_id, *ext_vbus;
>  	int retval;
>  
>  	match = of_match_device(dwc2_of_match_table, &dev->dev);
> @@ -620,6 +622,45 @@ static int dwc2_driver_probe(struct platform_device *dev)
>  	if (retval)
>  		goto error;
>  
> +	ext_id = ERR_PTR(-ENODEV);
> +	ext_vbus = ERR_PTR(-ENODEV);
> +	if (of_property_read_bool(dev->dev.of_node, "extcon")) {
> +		/* Each one of them is not mandatory */
> +		ext_vbus = extcon_get_edev_by_phandle(&dev->dev, 0);
> +		if (IS_ERR(ext_vbus) && PTR_ERR(ext_vbus) != -ENODEV)
> +			return PTR_ERR(ext_vbus);
> +
> +		ext_id = extcon_get_edev_by_phandle(&dev->dev, 1);
> +		if (IS_ERR(ext_id) && PTR_ERR(ext_id) != -ENODEV)
> +			return PTR_ERR(ext_id);
> +	}
> +
> +	hsotg->extcon_vbus.extcon = ext_vbus;
> +	if (!IS_ERR(ext_vbus)) {
> +		hsotg->extcon_vbus.nb.notifier_call = dwc2_extcon_vbus_notifier;
> +		retval = extcon_register_notifier(ext_vbus, EXTCON_USB,
> +							&hsotg->extcon_vbus.nb);
> +		if (retval < 0) {
> +			dev_err(&dev->dev, "register VBUS notifier failed\n");
> +			return retval;
> +		}
> +		hsotg->extcon_vbus.state = extcon_get_cable_state_(ext_vbus,
> +								EXTCON_USB);
> +	}
> +
> +	hsotg->extcon_id.extcon = ext_id;
> +	if (!IS_ERR(ext_id)) {
> +		hsotg->extcon_id.nb.notifier_call = dwc2_extcon_id_notifier;
> +		retval = extcon_register_notifier(ext_id, EXTCON_USB_HOST,
> +							&hsotg->extcon_id.nb);
> +		if (retval < 0) {
> +			dev_err(&dev->dev, "register ID notifier failed\n");
> +			return retval;
> +		}
> +		hsotg->extcon_id.state = extcon_get_cable_state_(ext_id,
> +							EXTCON_USB_HOST);
> +	}
> +
>  	/*
>  	 * Reset before dwc2_get_hwparams() then it could get power-on real
>  	 * reset value form registers.
>
John Stultz - Dec. 7, 2016, 4:13 a.m.
On Tue, Dec 6, 2016 at 7:54 PM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> Hi John,
>
> I give a some guide for extcon API.
> This patch uses the deprecated extcon API (extcon_get_cable_state_).
> So, I recommend that you better to use following extcon API:
> - extcon_get_cable_state_()  -> extcon_get_state()
> - extcon_register_notifier() -> devm_extcon_register_notifier()

Many thanks for the pointers! I really appreciate it! I'll rework the
driver accordingly (if JohnY doesn't conclude that the extcon support
here is unnecessary).

thanks
-john

Patch

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>;
+		};
+
 		usb_phy: usbphy {
 			compatible = "hisilicon,hi6220-usb-phy";
 			#phy-cells = <0>;
@@ -745,6 +755,7 @@ 
 			phys = <&usb_phy>;
 			phy-names = "usb2-phy";
 			clocks = <&sys_ctrl HI6220_USBOTG_HCLK>;
+			extcon = <&usb_vbus>, <&usb_id>;
 			clock-names = "otg";
 			dr_mode = "otg";
 			g-use-dma;
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 2a21a04..4cfce62 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -623,6 +623,13 @@  struct dwc2_hregs_backup {
 	bool valid;
 };
 
+struct dwc2_extcon {
+	struct notifier_block	nb;
+	struct extcon_dev	*extcon;
+	int			state;
+};
+
+
 /*
  * Constants related to high speed periodic scheduling
  *
@@ -996,6 +1003,10 @@  struct dwc2_hsotg {
 	u32 g_np_g_tx_fifo_sz;
 	u32 g_tx_fifo_sz[MAX_EPS_CHANNELS];
 #endif /* CONFIG_USB_DWC2_PERIPHERAL || CONFIG_USB_DWC2_DUAL_ROLE */
+
+	struct dwc2_extcon extcon_vbus;
+	struct dwc2_extcon extcon_id;
+	struct delayed_work extcon_work;
 };
 
 /* Reasons for halting a host channel */
@@ -1041,6 +1052,11 @@  extern void dwc2_flush_rx_fifo(struct dwc2_hsotg *hsotg);
 extern void dwc2_enable_global_interrupts(struct dwc2_hsotg *hcd);
 extern void dwc2_disable_global_interrupts(struct dwc2_hsotg *hcd);
 
+extern int dwc2_extcon_vbus_notifier(struct notifier_block *nb,
+					unsigned long event, void *ptr);
+extern int dwc2_extcon_id_notifier(struct notifier_block *nb,
+					unsigned long event, void *ptr);
+
 /* This function should be called on every hardware interrupt. */
 extern irqreturn_t dwc2_handle_common_intr(int irq, void *dev);
 
diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
index d85c5c9..d4fa938 100644
--- a/drivers/usb/dwc2/core_intr.c
+++ b/drivers/usb/dwc2/core_intr.c
@@ -479,6 +479,31 @@  static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg)
 	}
 }
 
+
+
+int dwc2_extcon_vbus_notifier(struct notifier_block *nb,
+					unsigned long event, void *ptr)
+{
+	struct dwc2_extcon *vbus = container_of(nb, struct dwc2_extcon, nb);
+	struct dwc2_hsotg *hsotg = container_of(vbus, struct dwc2_hsotg,
+						extcon_vbus);
+
+	schedule_delayed_work(&hsotg->extcon_work, msecs_to_jiffies(100));
+	return NOTIFY_DONE;
+}
+
+int dwc2_extcon_id_notifier(struct notifier_block *nb,
+					unsigned long event, void *ptr)
+{
+	struct dwc2_extcon *id = container_of(nb, struct dwc2_extcon, nb);
+	struct dwc2_hsotg *hsotg = container_of(id, struct dwc2_hsotg,
+						extcon_id);
+	schedule_delayed_work(&hsotg->extcon_work, msecs_to_jiffies(100));
+
+	return NOTIFY_DONE;
+}
+
+
 #define GINTMSK_COMMON	(GINTSTS_WKUPINT | GINTSTS_SESSREQINT |		\
 			 GINTSTS_CONIDSTSCHNG | GINTSTS_OTGINT |	\
 			 GINTSTS_MODEMIS | GINTSTS_DISCONNINT |		\
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index df5a065..61eea70 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -47,6 +47,7 @@ 
 #include <linux/io.h>
 #include <linux/slab.h>
 #include <linux/usb.h>
+#include <linux/extcon.h>
 
 #include <linux/usb/hcd.h>
 #include <linux/usb/ch11.h>
@@ -3246,6 +3247,26 @@  static void dwc2_conn_id_status_change(struct work_struct *work)
 	}
 }
 
+static void dwc2_hcd_extcon_func(struct work_struct *work)
+{
+	struct dwc2_hsotg *hsotg = container_of(work, struct dwc2_hsotg,
+						extcon_work.work);
+
+	if (!IS_ERR(hsotg->extcon_vbus.extcon))
+		hsotg->extcon_vbus.state =
+			extcon_get_cable_state_(hsotg->extcon_vbus.extcon,
+								EXTCON_USB);
+	if (!IS_ERR(hsotg->extcon_id.extcon))
+		hsotg->extcon_id.state =
+			extcon_get_cable_state_(hsotg->extcon_id.extcon,
+							EXTCON_USB_HOST);
+
+	if (hsotg->extcon_id.state)
+		usb_gadget_vbus_connect(&hsotg->gadget);
+	else
+		usb_gadget_vbus_disconnect(&hsotg->gadget);
+}
+
 static void dwc2_wakeup_detected(unsigned long data)
 {
 	struct dwc2_hsotg *hsotg = (struct dwc2_hsotg *)data;
@@ -5085,6 +5106,8 @@  int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
 	/* Initialize port reset work */
 	INIT_DELAYED_WORK(&hsotg->reset_work, dwc2_hcd_reset_func);
 
+	INIT_DELAYED_WORK(&hsotg->extcon_work, dwc2_hcd_extcon_func);
+
 	/*
 	 * Allocate space for storing data on status transactions. Normally no
 	 * data is sent, but this space acts as a bit bucket. This must be
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 8e1728b..2e4545f 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -46,6 +46,7 @@ 
 #include <linux/phy/phy.h>
 #include <linux/platform_data/s3c-hsotg.h>
 #include <linux/reset.h>
+#include <linux/extcon.h>
 
 #include <linux/usb/of.h>
 
@@ -543,6 +544,7 @@  static int dwc2_driver_probe(struct platform_device *dev)
 	struct dwc2_core_params defparams;
 	struct dwc2_hsotg *hsotg;
 	struct resource *res;
+	struct extcon_dev *ext_id, *ext_vbus;
 	int retval;
 
 	match = of_match_device(dwc2_of_match_table, &dev->dev);
@@ -620,6 +622,45 @@  static int dwc2_driver_probe(struct platform_device *dev)
 	if (retval)
 		goto error;
 
+	ext_id = ERR_PTR(-ENODEV);
+	ext_vbus = ERR_PTR(-ENODEV);
+	if (of_property_read_bool(dev->dev.of_node, "extcon")) {
+		/* Each one of them is not mandatory */
+		ext_vbus = extcon_get_edev_by_phandle(&dev->dev, 0);
+		if (IS_ERR(ext_vbus) && PTR_ERR(ext_vbus) != -ENODEV)
+			return PTR_ERR(ext_vbus);
+
+		ext_id = extcon_get_edev_by_phandle(&dev->dev, 1);
+		if (IS_ERR(ext_id) && PTR_ERR(ext_id) != -ENODEV)
+			return PTR_ERR(ext_id);
+	}
+
+	hsotg->extcon_vbus.extcon = ext_vbus;
+	if (!IS_ERR(ext_vbus)) {
+		hsotg->extcon_vbus.nb.notifier_call = dwc2_extcon_vbus_notifier;
+		retval = extcon_register_notifier(ext_vbus, EXTCON_USB,
+							&hsotg->extcon_vbus.nb);
+		if (retval < 0) {
+			dev_err(&dev->dev, "register VBUS notifier failed\n");
+			return retval;
+		}
+		hsotg->extcon_vbus.state = extcon_get_cable_state_(ext_vbus,
+								EXTCON_USB);
+	}
+
+	hsotg->extcon_id.extcon = ext_id;
+	if (!IS_ERR(ext_id)) {
+		hsotg->extcon_id.nb.notifier_call = dwc2_extcon_id_notifier;
+		retval = extcon_register_notifier(ext_id, EXTCON_USB_HOST,
+							&hsotg->extcon_id.nb);
+		if (retval < 0) {
+			dev_err(&dev->dev, "register ID notifier failed\n");
+			return retval;
+		}
+		hsotg->extcon_id.state = extcon_get_cable_state_(ext_id,
+							EXTCON_USB_HOST);
+	}
+
 	/*
 	 * Reset before dwc2_get_hwparams() then it could get power-on real
 	 * reset value form registers.