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 <[email protected]>
Cc: Guodong Xu <[email protected]>
Cc: Amit Pundir <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: John Youn <[email protected]>
Cc: Douglas Anderson <[email protected]>
Cc: Chen Yu <[email protected]>
Cc: Kishon Vijay Abraham I <[email protected]>
Cc: Felipe Balbi <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
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
From: Chen Yu <[email protected]>
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 <[email protected]>
Cc: Guodong Xu <[email protected]>
Cc: Amit Pundir <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: John Youn <[email protected]>
Cc: Douglas Anderson <[email protected]>
Cc: Chen Yu <[email protected]>
Cc: Felipe Balbi <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
Signed-off-by: Chen Yu <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
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
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 <[email protected]>
Cc: Guodong Xu <[email protected]>
Cc: Amit Pundir <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: John Youn <[email protected]>
Cc: Douglas Anderson <[email protected]>
Cc: Chen Yu <[email protected]>
Cc: Kishon Vijay Abraham I <[email protected]>
Cc: Felipe Balbi <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
Signed-off-by: John Stultz <[email protected]>
---
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
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 <[email protected]>
Cc: Guodong Xu <[email protected]>
Cc: Amit Pundir <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: John Youn <[email protected]>
Cc: Douglas Anderson <[email protected]>
Cc: Chen Yu <[email protected]>
Cc: Felipe Balbi <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
Suggested-by: Chen Yu <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
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
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 <[email protected]>
> Cc: Guodong Xu <[email protected]>
> Cc: Amit Pundir <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: John Youn <[email protected]>
> Cc: Douglas Anderson <[email protected]>
> Cc: Chen Yu <[email protected]>
> Cc: Kishon Vijay Abraham I <[email protected]>
> Cc: Felipe Balbi <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: [email protected]
> Signed-off-by: John Stultz <[email protected]>
> ---
> 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
On Tue, Dec 6, 2016 at 3:17 PM, John Youn <[email protected]> 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 <[email protected]>
>> Cc: Guodong Xu <[email protected]>
>> Cc: Amit Pundir <[email protected]>
>> Cc: Rob Herring <[email protected]>
>> Cc: John Youn <[email protected]>
>> Cc: Douglas Anderson <[email protected]>
>> Cc: Chen Yu <[email protected]>
>> Cc: Kishon Vijay Abraham I <[email protected]>
>> Cc: Felipe Balbi <[email protected]>
>> Cc: Greg Kroah-Hartman <[email protected]>
>> Cc: [email protected]
>> Signed-off-by: John Stultz <[email protected]>
>> ---
>> 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
On 12/6/2016 4:05 PM, John Stultz wrote:
> On Tue, Dec 6, 2016 at 3:17 PM, John Youn <[email protected]> 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 <[email protected]>
>>> Cc: Guodong Xu <[email protected]>
>>> Cc: Amit Pundir <[email protected]>
>>> Cc: Rob Herring <[email protected]>
>>> Cc: John Youn <[email protected]>
>>> Cc: Douglas Anderson <[email protected]>
>>> Cc: Chen Yu <[email protected]>
>>> Cc: Kishon Vijay Abraham I <[email protected]>
>>> Cc: Felipe Balbi <[email protected]>
>>> Cc: Greg Kroah-Hartman <[email protected]>
>>> Cc: [email protected]
>>> Signed-off-by: John Stultz <[email protected]>
>>> ---
>>> 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
On Tue, Dec 6, 2016 at 4:26 PM, John Youn <[email protected]> wrote:
> On 12/6/2016 4:05 PM, John Stultz wrote:
>> On Tue, Dec 6, 2016 at 3:17 PM, John Youn <[email protected]> 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
On Tue, Dec 6, 2016 at 4:35 PM, John Stultz <[email protected]> wrote:
> On Tue, Dec 6, 2016 at 4:26 PM, John Youn <[email protected]> wrote:
>> On 12/6/2016 4:05 PM, John Stultz wrote:
>>> On Tue, Dec 6, 2016 at 3:17 PM, John Youn <[email protected]> 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
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 <[email protected]>
> Cc: Guodong Xu <[email protected]>
> Cc: Amit Pundir <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: John Youn <[email protected]>
> Cc: Douglas Anderson <[email protected]>
> Cc: Chen Yu <[email protected]>
> Cc: Kishon Vijay Abraham I <[email protected]>
> Cc: Felipe Balbi <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: [email protected]
> Signed-off-by: John Stultz <[email protected]>
> ---
> 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.
>
On Tue, Dec 6, 2016 at 7:54 PM, Chanwoo Choi <[email protected]> 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