* [RFC][PATCH 0/3 v2] Try to connect HiKey's usb extcon to dwc2 driver @ 2016-12-06 8:06 John Stultz 2016-12-06 8:06 ` [RFC][PATCH 1/3 v2] usb: dwc2: Add extcon support " John Stultz ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: John Stultz @ 2016-12-06 8:06 UTC (permalink / raw) To: lkml Cc: John Stultz, Wei Xu, Guodong Xu, Amit Pundir, Rob Herring, John Youn, Douglas Anderson, Chen Yu, Kishon Vijay Abraham I, Felipe Balbi, Greg Kroah-Hartman, linux-usb After feedback from Kishon and John, I've reworked my efforts to add extcon support to the phy-hi6220-usb driver and instead have added the extcon support to the dwc2 driver directly. This avoids odd interactions trying to wire the generic phy to the otg gadget structure to send proper connect/disconnect notifications to the hcd core. Since this is my first stab at moving it to the dwc2 driver, I suspect there is further improvements possible, so please let me know if there's anything folks would like to see changed. In this series, I also re-added an older patch to force port resuming when transitioning from host -> otg mode, as that was catching me as the bus would suspend if there were no devices plugged into the host ports and then would not work when replugging in the gadget cable. If there is a better way to handle this bus resuming logic in gadget mode, please let me know. Feedback and guidance would be greatly appreciated! thanks -john 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 Chen Yu (1): usb: dwc2: Force port resume on switching to device mode John Stultz (2): usb: dwc2: Add extcon support to dwc2 driver usb: dwc2: Avoid suspending if we're in gadget mode 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 | 34 +++++++++++++++++++++++++ drivers/usb/dwc2/platform.c | 41 +++++++++++++++++++++++++++++++ 5 files changed, 127 insertions(+) -- 2.7.4 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC][PATCH 1/3 v2] usb: dwc2: Add extcon support to dwc2 driver 2016-12-06 8:06 [RFC][PATCH 0/3 v2] Try to connect HiKey's usb extcon to dwc2 driver John Stultz @ 2016-12-06 8:06 ` John Stultz 2016-12-06 23:17 ` John Youn 2016-12-07 3:54 ` Chanwoo Choi 2016-12-06 8:06 ` [RFC][PATCH 2/3 v2] usb: dwc2: Avoid suspending if we're in gadget mode John Stultz 2016-12-06 8:06 ` [RFC][PATCH 3/3 v2] usb: dwc2: Force port resume on switching to device mode John Stultz 2 siblings, 2 replies; 11+ messages in thread From: John Stultz @ 2016-12-06 8:06 UTC (permalink / raw) To: lkml Cc: John Stultz, Wei Xu, Guodong Xu, Amit Pundir, Rob Herring, John Youn, Douglas Anderson, Chen Yu, Kishon Vijay Abraham I, Felipe Balbi, Greg Kroah-Hartman, linux-usb 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. -- 2.7.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [RFC][PATCH 1/3 v2] usb: dwc2: Add extcon support to dwc2 driver 2016-12-06 8:06 ` [RFC][PATCH 1/3 v2] usb: dwc2: Add extcon support " John Stultz @ 2016-12-06 23:17 ` John Youn 2016-12-07 0:04 ` John Stultz 2016-12-07 3:54 ` Chanwoo Choi 1 sibling, 1 reply; 11+ messages in thread From: John Youn @ 2016-12-06 23:17 UTC (permalink / raw) To: John Stultz, lkml Cc: Wei Xu, Guodong Xu, Amit Pundir, Rob Herring, John Youn, Douglas Anderson, Chen Yu, Kishon Vijay Abraham I, Felipe Balbi, Greg Kroah-Hartman, linux-usb 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][PATCH 1/3 v2] usb: dwc2: Add extcon support to dwc2 driver 2016-12-06 23:17 ` John Youn @ 2016-12-07 0:04 ` John Stultz 2016-12-07 0:26 ` John Youn 0 siblings, 1 reply; 11+ messages in thread From: John Stultz @ 2016-12-07 0:04 UTC (permalink / raw) To: John Youn Cc: lkml, Wei Xu, Guodong Xu, Amit Pundir, Rob Herring, Douglas Anderson, Chen Yu, Kishon Vijay Abraham I, Felipe Balbi, Greg Kroah-Hartman, linux-usb 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][PATCH 1/3 v2] usb: dwc2: Add extcon support to dwc2 driver 2016-12-07 0:04 ` John Stultz @ 2016-12-07 0:26 ` John Youn 2016-12-07 0:35 ` John Stultz 0 siblings, 1 reply; 11+ messages in thread From: John Youn @ 2016-12-07 0:26 UTC (permalink / raw) To: John Stultz, John Youn Cc: lkml, Wei Xu, Guodong Xu, Amit Pundir, Rob Herring, Douglas Anderson, Chen Yu, Kishon Vijay Abraham I, Felipe Balbi, Greg Kroah-Hartman, linux-usb 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][PATCH 1/3 v2] usb: dwc2: Add extcon support to dwc2 driver 2016-12-07 0:26 ` John Youn @ 2016-12-07 0:35 ` John Stultz 2016-12-07 1:44 ` John Stultz 0 siblings, 1 reply; 11+ messages in thread From: John Stultz @ 2016-12-07 0:35 UTC (permalink / raw) To: John Youn Cc: lkml, Wei Xu, Guodong Xu, Amit Pundir, Rob Herring, Douglas Anderson, Chen Yu, Kishon Vijay Abraham I, Felipe Balbi, Greg Kroah-Hartman, linux-usb 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][PATCH 1/3 v2] usb: dwc2: Add extcon support to dwc2 driver 2016-12-07 0:35 ` John Stultz @ 2016-12-07 1:44 ` John Stultz 0 siblings, 0 replies; 11+ messages in thread From: John Stultz @ 2016-12-07 1:44 UTC (permalink / raw) To: John Youn Cc: lkml, Wei Xu, Guodong Xu, Amit Pundir, Rob Herring, Douglas Anderson, Chen Yu, Kishon Vijay Abraham I, Felipe Balbi, Greg Kroah-Hartman, linux-usb 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][PATCH 1/3 v2] usb: dwc2: Add extcon support to dwc2 driver 2016-12-06 8:06 ` [RFC][PATCH 1/3 v2] usb: dwc2: Add extcon support " John Stultz 2016-12-06 23:17 ` John Youn @ 2016-12-07 3:54 ` Chanwoo Choi 2016-12-07 4:13 ` John Stultz 1 sibling, 1 reply; 11+ messages in thread From: Chanwoo Choi @ 2016-12-07 3:54 UTC (permalink / raw) To: John Stultz, lkml Cc: Wei Xu, Guodong Xu, Amit Pundir, Rob Herring, John Youn, Douglas Anderson, Chen Yu, Kishon Vijay Abraham I, Felipe Balbi, Greg Kroah-Hartman, linux-usb 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. > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][PATCH 1/3 v2] usb: dwc2: Add extcon support to dwc2 driver 2016-12-07 3:54 ` Chanwoo Choi @ 2016-12-07 4:13 ` John Stultz 0 siblings, 0 replies; 11+ messages in thread From: John Stultz @ 2016-12-07 4:13 UTC (permalink / raw) To: Chanwoo Choi Cc: lkml, Wei Xu, Guodong Xu, Amit Pundir, Rob Herring, John Youn, Douglas Anderson, Chen Yu, Kishon Vijay Abraham I, Felipe Balbi, Greg Kroah-Hartman, Linux USB List 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC][PATCH 2/3 v2] usb: dwc2: Avoid suspending if we're in gadget mode 2016-12-06 8:06 [RFC][PATCH 0/3 v2] Try to connect HiKey's usb extcon to dwc2 driver John Stultz 2016-12-06 8:06 ` [RFC][PATCH 1/3 v2] usb: dwc2: Add extcon support " John Stultz @ 2016-12-06 8:06 ` John Stultz 2016-12-06 8:06 ` [RFC][PATCH 3/3 v2] usb: dwc2: Force port resume on switching to device mode John Stultz 2 siblings, 0 replies; 11+ messages in thread From: John Stultz @ 2016-12-06 8:06 UTC (permalink / raw) To: lkml Cc: John Stultz, Wei Xu, Guodong Xu, Amit Pundir, Rob Herring, John Youn, Douglas Anderson, Chen Yu, Felipe Balbi, Greg Kroah-Hartman, linux-usb I've found when booting HiKey with the usb gadget cable attached if I then try to connect via adb, I get an infinite spew of: dwc2 f72c0000.usb: dwc2_hsotg_ep_sethalt(ep ffffffc0790ecb18 ep1out, 0) dwc2 f72c0000.usb: dwc2_hsotg_ep_sethalt(ep ffffffc0790eca18 ep1in, 0) It seems that the usb autosuspend is suspending the bus shortly after bootup when the gadget cable is attached. So when adbd then tries to use the device, it doesn't work and it then tries to restart it over and over via the ep_sethalt calls (via FUNCTIONFS_CLEAR_HALT ioctl). Chen Yu suggested this patch to avoid suspending if we're in device mode, and it avoids the problem. This doesn't remove the need for the previous patch, to resume the port when we switch to gadget mode from host mode. But it does seem to resolve the issue. 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: Felipe Balbi <felipe.balbi@linux.intel.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: linux-usb@vger.kernel.org Suggested-by: Chen Yu <chenyu56@huawei.com> Signed-off-by: John Stultz <john.stultz@linaro.org> --- drivers/usb/dwc2/hcd.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index 61eea70..75ddfa3 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -4386,6 +4386,9 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd) if (!HCD_HW_ACCESSIBLE(hcd)) goto unlock; + if (hsotg->op_state == OTG_STATE_B_PERIPHERAL) + goto unlock; + if (!hsotg->core_params->hibernation) goto skip_power_saving; -- 2.7.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [RFC][PATCH 3/3 v2] usb: dwc2: Force port resume on switching to device mode 2016-12-06 8:06 [RFC][PATCH 0/3 v2] Try to connect HiKey's usb extcon to dwc2 driver John Stultz 2016-12-06 8:06 ` [RFC][PATCH 1/3 v2] usb: dwc2: Add extcon support " John Stultz 2016-12-06 8:06 ` [RFC][PATCH 2/3 v2] usb: dwc2: Avoid suspending if we're in gadget mode John Stultz @ 2016-12-06 8:06 ` John Stultz 2 siblings, 0 replies; 11+ messages in thread From: John Stultz @ 2016-12-06 8:06 UTC (permalink / raw) To: lkml Cc: Chen Yu, Wei Xu, Guodong Xu, Amit Pundir, Rob Herring, John Youn, Douglas Anderson, Felipe Balbi, Greg Kroah-Hartman, linux-usb, John Stultz From: Chen Yu <chenyu56@huawei.com> We've seen failures when switching between host and gadget mode, which was diagnosed as being caused due to the bus being auto-suspended when we switched. So this patch forces a port resume when switching to device mode if the bus is suspended. 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: Felipe Balbi <felipe.balbi@linux.intel.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: linux-usb@vger.kernel.org Signed-off-by: Chen Yu <chenyu56@huawei.com> Signed-off-by: John Stultz <john.stultz@linaro.org> --- drivers/usb/dwc2/hcd.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index 75ddfa3..0221534 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -55,6 +55,9 @@ #include "core.h" #include "hcd.h" + +static void dwc2_port_resume(struct dwc2_hsotg *hsotg); + /* * ========================================================================= * Host Core Layer Functions @@ -3205,6 +3208,11 @@ static void dwc2_conn_id_status_change(struct work_struct *work) if (gotgctl & GOTGCTL_CONID_B) { /* Wait for switch to device mode */ dev_dbg(hsotg->dev, "connId B\n"); + if (hsotg->bus_suspended) { + dev_info(hsotg->dev, + "Do port resume before switching to device mode\n"); + dwc2_port_resume(hsotg); + } while (!dwc2_is_device_mode(hsotg)) { dev_info(hsotg->dev, "Waiting for Peripheral Mode, Mode=%s\n", -- 2.7.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-12-07 4:14 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-12-06 8:06 [RFC][PATCH 0/3 v2] Try to connect HiKey's usb extcon to dwc2 driver John Stultz 2016-12-06 8:06 ` [RFC][PATCH 1/3 v2] usb: dwc2: Add extcon support " John Stultz 2016-12-06 23:17 ` John Youn 2016-12-07 0:04 ` John Stultz 2016-12-07 0:26 ` John Youn 2016-12-07 0:35 ` John Stultz 2016-12-07 1:44 ` John Stultz 2016-12-07 3:54 ` Chanwoo Choi 2016-12-07 4:13 ` John Stultz 2016-12-06 8:06 ` [RFC][PATCH 2/3 v2] usb: dwc2: Avoid suspending if we're in gadget mode John Stultz 2016-12-06 8:06 ` [RFC][PATCH 3/3 v2] usb: dwc2: Force port resume on switching to device mode John Stultz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).