[PATCH v4 0/9] usb: dwc3: add dual-role support

[PATCH v4 0/9] usb: dwc3: add dual-role support

[Roger Quadros]

Hi,

This series adds dual role support to dwc3 controller driver.
Series depends on [1] for core OTG/dual-role support.

Patches are based on greg/usb-next.

Tested on am437x-gp-evm and dra7-evm after platform related patches are
applied. Those are available at [2].

[1] core OTG/DRD support - http://thread.gmane.org/gmane.linux.kernel/2025239
[2] platform support - http://thread.gmane.org/gmane.linux.usb.general/127898

cheers,
-roger

Felipe Balbi (1):
  usb: dwc3: core: Adapt to named interrupts

Roger Quadros (8):
  usb: dwc3: add dual-role support
  usb: dwc3: core.h: add some register definitions
  usb: dwc3: dwc3-omap: Make the wrapper interrupt shared
  usb: dwc3: core: make dual-role work with OTG irq
  usb: dwc3: save/restore OTG registers during suspend/resume
  usb: dwc3: gadget: Fix suspend/resume during dual-role mode
  usb: dwc3: core: Prevent otg events from disabling themselves
  usb: dwc3: core: don't break during suspend/resume while we're
    dual-role

 drivers/usb/dwc3/core.c          | 422 +++++++++++++++++++++++++++++++++++++--
 drivers/usb/dwc3/core.h          | 103 ++++++++++
 drivers/usb/dwc3/dwc3-omap.c     |   4 +-
 drivers/usb/dwc3/gadget.c        |   8 +-
 drivers/usb/dwc3/platform_data.h |   1 +
 5 files changed, 516 insertions(+), 22 deletions(-)

-- 
2.1.4

[PATCH v4 1/9] usb: dwc3: add dual-role support

[Roger Quadros]

Register with the USB OTG core. Since we don't support
OTG yet we just work as a dual-role device even
if device tree says "otg".

Use extcon framework to get VBUS/ID cable events and
kick the OTG state machine.

Signed-off-by: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
---
 drivers/usb/dwc3/core.c          | 174 ++++++++++++++++++++++++++++++++++++++-
 drivers/usb/dwc3/core.h          |   7 ++
 drivers/usb/dwc3/platform_data.h |   1 +
 3 files changed, 181 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 064123e..2e36a9b 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -704,6 +704,152 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
 	return 0;
 }
 
+/* --------------------- Dual-Role management ------------------------------- */
+
+static void dwc3_drd_fsm_sync(struct dwc3 *dwc)
+{
+	int id, vbus;
+
+	/* get ID */
+	id = extcon_get_cable_state(dwc->edev, "USB-HOST");
+	/* Host means ID == 0 */
+	id = !id;
+
+	/* get VBUS */
+	vbus = extcon_get_cable_state(dwc->edev, "USB");
+	dev_dbg(dwc->dev, "id %d vbus %d\n", id, vbus);
+
+	dwc->fsm->id = id;
+	dwc->fsm->b_sess_vld = vbus;
+	usb_otg_sync_inputs(dwc->fsm);
+}
+
+static int dwc3_drd_start_host(struct otg_fsm *fsm, int on)
+{
+	struct device *dev = usb_otg_fsm_to_dev(fsm);
+	struct dwc3 *dwc = dev_get_drvdata(dev);
+
+	dev_dbg(dwc->dev, "%s: %d\n", __func__, on);
+	if (on) {
+		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
+		/* start the HCD */
+		usb_otg_start_host(fsm, true);
+	} else {
+		/* stop the HCD */
+		usb_otg_start_host(fsm, false);
+	}
+
+	return 0;
+}
+
+static int dwc3_drd_start_gadget(struct otg_fsm *fsm, int on)
+{
+	struct device *dev = usb_otg_fsm_to_dev(fsm);
+	struct dwc3 *dwc = dev_get_drvdata(dev);
+
+	dev_dbg(dwc->dev, "%s: %d\n", __func__, on);
+	if (on) {
+		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
+		dwc3_event_buffers_setup(dwc);
+
+		/* start the UDC */
+		usb_otg_start_gadget(fsm, true);
+	} else {
+		/* stop the UDC */
+		usb_otg_start_gadget(fsm, false);
+	}
+
+	return 0;
+}
+
+static struct otg_fsm_ops dwc3_drd_ops = {
+	.start_host = dwc3_drd_start_host,
+	.start_gadget = dwc3_drd_start_gadget,
+};
+
+static int dwc3_drd_notifier(struct notifier_block *nb,
+			     unsigned long event, void *ptr)
+{
+	struct dwc3 *dwc = container_of(nb, struct dwc3, otg_nb);
+
+	dwc3_drd_fsm_sync(dwc);
+
+	return NOTIFY_DONE;
+}
+
+static int dwc3_drd_init(struct dwc3 *dwc)
+{
+	int ret, id, vbus;
+	struct usb_otg_caps *otgcaps = &dwc->otg_config.otg_caps;
+
+	otgcaps->otg_rev = 0;
+	otgcaps->hnp_support = false;
+	otgcaps->srp_support = false;
+	otgcaps->adp_support = false;
+	dwc->otg_config.fsm_ops = &dwc3_drd_ops;
+
+	if (!dwc->edev) {
+		dev_err(dwc->dev, "No extcon device found for OTG mode\n");
+		return -ENODEV;
+	}
+
+	dwc->otg_nb.notifier_call = dwc3_drd_notifier;
+	ret = extcon_register_notifier(dwc->edev, EXTCON_USB, &dwc->otg_nb);
+	if (ret < 0) {
+		dev_err(dwc->dev, "Couldn't register USB cable notifier\n");
+		return -ENODEV;
+	}
+
+	ret = extcon_register_notifier(dwc->edev, EXTCON_USB_HOST,
+				       &dwc->otg_nb);
+	if (ret < 0) {
+		dev_err(dwc->dev, "Couldn't register USB-HOST cable notifier\n");
+		ret = -ENODEV;
+		goto extcon_fail;
+	}
+
+	/* sanity check id & vbus states */
+	id = extcon_get_cable_state(dwc->edev, "USB-HOST");
+	vbus = extcon_get_cable_state(dwc->edev, "USB");
+	if (id < 0 || vbus < 0) {
+		dev_err(dwc->dev, "Invalid USB cable state. id %d, vbus %d\n",
+			id, vbus);
+		ret = -ENODEV;
+		goto fail;
+	}
+
+	/* register as DRD device with OTG core */
+	dwc->fsm = usb_otg_register(dwc->dev, &dwc->otg_config);
+	if (IS_ERR(dwc->fsm)) {
+		ret = PTR_ERR(dwc->fsm);
+		if (ret == -ENOTSUPP)
+			dev_err(dwc->dev, "CONFIG_USB_OTG needed for dual-role\n");
+		else
+			dev_err(dwc->dev, "Failed to register with OTG core\n");
+
+		goto fail;
+	}
+
+	dwc3_drd_fsm_sync(dwc);
+
+	return 0;
+fail:
+	extcon_unregister_notifier(dwc->edev, EXTCON_USB_HOST, &dwc->otg_nb);
+extcon_fail:
+	extcon_unregister_notifier(dwc->edev, EXTCON_USB, &dwc->otg_nb);
+
+	return ret;
+}
+
+static void dwc3_drd_exit(struct dwc3 *dwc)
+{
+	usb_otg_unregister(dwc->dev);
+	extcon_unregister_notifier(dwc->edev, EXTCON_USB_HOST, &dwc->otg_nb);
+	extcon_unregister_notifier(dwc->edev, EXTCON_USB, &dwc->otg_nb);
+}
+
+/* -------------------------------------------------------------------------- */
+
 static int dwc3_core_init_mode(struct dwc3 *dwc)
 {
 	struct device *dev = dwc->dev;
@@ -727,13 +873,21 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
 		}
 		break;
 	case USB_DR_MODE_OTG:
-		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
+		ret = dwc3_drd_init(dwc);
+		if (ret) {
+			dev_err(dev, "limiting to peripheral only\n");
+			dwc->dr_mode = USB_DR_MODE_PERIPHERAL;
+			dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
+			goto gadget_init;
+		}
+
 		ret = dwc3_host_init(dwc);
 		if (ret) {
 			dev_err(dev, "failed to initialize host\n");
 			return ret;
 		}
 
+gadget_init:
 		ret = dwc3_gadget_init(dwc);
 		if (ret) {
 			dev_err(dev, "failed to initialize gadget\n");
@@ -760,6 +914,7 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc)
 	case USB_DR_MODE_OTG:
 		dwc3_host_exit(dwc);
 		dwc3_gadget_exit(dwc);
+		dwc3_drd_exit(dwc);
 		break;
 	default:
 		/* do nothing */
@@ -843,6 +998,16 @@ static int dwc3_probe(struct platform_device *pdev)
 	hird_threshold = 12;
 
 	if (node) {
+		if (of_property_read_bool(node, "extcon"))
+			dwc->edev = extcon_get_edev_by_phandle(dev, 0);
+		else if (of_property_read_bool(dev->parent->of_node, "extcon"))
+			dwc->edev = extcon_get_edev_by_phandle(dev->parent, 0);
+
+		if (IS_ERR(dwc->edev)) {
+			dev_vdbg(dev, "couldn't get extcon device\n");
+			return -EPROBE_DEFER;
+		}
+
 		dwc->maximum_speed = of_usb_get_maximum_speed(node);
 		dwc->has_lpm_erratum = of_property_read_bool(node,
 				"snps,has-lpm-erratum");
@@ -887,6 +1052,13 @@ static int dwc3_probe(struct platform_device *pdev)
 		of_property_read_string(node, "snps,hsphy_interface",
 					&dwc->hsphy_interface);
 	} else if (pdata) {
+		if (pdata->extcon) {
+			dwc->edev = extcon_get_extcon_dev(pdata->extcon);
+			if (!dwc->edev) {
+				dev_vdbg(dev, "couldn't get extcon device\n");
+				return -EPROBE_DEFER;
+			}
+		}
 		dwc->maximum_speed = pdata->maximum_speed;
 		dwc->has_lpm_erratum = pdata->has_lpm_erratum;
 		if (pdata->lpm_nyet_threshold)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 0447788..5ca2b25 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -31,8 +31,10 @@
 #include <linux/usb/gadget.h>
 #include <linux/usb/otg.h>
 #include <linux/ulpi/interface.h>
+#include <linux/usb/otg-fsm.h>
 
 #include <linux/phy/phy.h>
+#include <linux/extcon.h>
 
 #define DWC3_MSG_MAX	500
 
@@ -753,6 +755,11 @@ struct dwc3 {
 
 	struct ulpi		*ulpi;
 
+	struct extcon_dev	*edev;	/* USB cable events ID & VBUS */
+	struct notifier_block	otg_nb;	/* notifier for USB cable events */
+	struct otg_fsm		*fsm;
+	struct usb_otg_config	otg_config;
+
 	void __iomem		*regs;
 	size_t			regs_size;
 
diff --git a/drivers/usb/dwc3/platform_data.h b/drivers/usb/dwc3/platform_data.h
index d3614ec..b3b245c 100644
--- a/drivers/usb/dwc3/platform_data.h
+++ b/drivers/usb/dwc3/platform_data.h
@@ -47,4 +47,5 @@ struct dwc3_platform_data {
 	unsigned tx_de_emphasis:2;
 
 	const char *hsphy_interface;
+	const char *extcon;	/* extcon name for USB cable events ID/VBUS */
 };
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/9] usb: dwc3: add dual-role support

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 8978 bytes --]

Hi,

On Wed, Sep 02, 2015 at 05:24:16PM +0300, Roger Quadros wrote:
> Register with the USB OTG core. Since we don't support
> OTG yet we just work as a dual-role device even
> if device tree says "otg".
> 
> Use extcon framework to get VBUS/ID cable events and
> kick the OTG state machine.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  drivers/usb/dwc3/core.c          | 174 ++++++++++++++++++++++++++++++++++++++-
>  drivers/usb/dwc3/core.h          |   7 ++
>  drivers/usb/dwc3/platform_data.h |   1 +
>  3 files changed, 181 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 064123e..2e36a9b 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -704,6 +704,152 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
>  	return 0;
>  }
>  
> +/* --------------------- Dual-Role management ------------------------------- */
> +
> +static void dwc3_drd_fsm_sync(struct dwc3 *dwc)
> +{
> +	int id, vbus;
> +
> +	/* get ID */
> +	id = extcon_get_cable_state(dwc->edev, "USB-HOST");
> +	/* Host means ID == 0 */
> +	id = !id;
> +
> +	/* get VBUS */
> +	vbus = extcon_get_cable_state(dwc->edev, "USB");
> +	dev_dbg(dwc->dev, "id %d vbus %d\n", id, vbus);

tracepoint please. We don't want this driver to use dev_(v)?db anymore.
Ditto to all others.

> +
> +	dwc->fsm->id = id;
> +	dwc->fsm->b_sess_vld = vbus;
> +	usb_otg_sync_inputs(dwc->fsm);
> +}
> +
> +static int dwc3_drd_start_host(struct otg_fsm *fsm, int on)
> +{
> +	struct device *dev = usb_otg_fsm_to_dev(fsm);
> +	struct dwc3 *dwc = dev_get_drvdata(dev);

how about adding a usb_otg_get_drvdata(fsm) ?

> +	dev_dbg(dwc->dev, "%s: %d\n", __func__, on);
> +	if (on) {
> +		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
> +		/* start the HCD */
> +		usb_otg_start_host(fsm, true);
> +	} else {
> +		/* stop the HCD */
> +		usb_otg_start_host(fsm, false);
> +	}

This can be simplified.

	if (on)
		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);

	usb_otg_start_host(fsm, on);

> +
> +	return 0;
> +}
> +
> +static int dwc3_drd_start_gadget(struct otg_fsm *fsm, int on)
> +{
> +	struct device *dev = usb_otg_fsm_to_dev(fsm);
> +	struct dwc3 *dwc = dev_get_drvdata(dev);
> +
> +	dev_dbg(dwc->dev, "%s: %d\n", __func__, on);
> +	if (on) {
> +		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
> +		dwc3_event_buffers_setup(dwc);
> +
> +		/* start the UDC */
> +		usb_otg_start_gadget(fsm, true);
> +	} else {
> +		/* stop the UDC */
> +		usb_otg_start_gadget(fsm, false);
> +	}

likewise:

	if (on) {
		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
		dwc3_event_buffers_setup(dwc);
	}

	usb_otg_start_gadget(fsm, on);

> +	return 0;
> +}
> +
> +static struct otg_fsm_ops dwc3_drd_ops = {
> +	.start_host = dwc3_drd_start_host,
> +	.start_gadget = dwc3_drd_start_gadget,
> +};
> +
> +static int dwc3_drd_notifier(struct notifier_block *nb,
> +			     unsigned long event, void *ptr)
> +{
> +	struct dwc3 *dwc = container_of(nb, struct dwc3, otg_nb);
> +
> +	dwc3_drd_fsm_sync(dwc);
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static int dwc3_drd_init(struct dwc3 *dwc)
> +{
> +	int ret, id, vbus;
> +	struct usb_otg_caps *otgcaps = &dwc->otg_config.otg_caps;
> +
> +	otgcaps->otg_rev = 0;
> +	otgcaps->hnp_support = false;
> +	otgcaps->srp_support = false;
> +	otgcaps->adp_support = false;
> +	dwc->otg_config.fsm_ops = &dwc3_drd_ops;
> +
> +	if (!dwc->edev) {
> +		dev_err(dwc->dev, "No extcon device found for OTG mode\n");
> +		return -ENODEV;
> +	}
> +
> +	dwc->otg_nb.notifier_call = dwc3_drd_notifier;
> +	ret = extcon_register_notifier(dwc->edev, EXTCON_USB, &dwc->otg_nb);
> +	if (ret < 0) {
> +		dev_err(dwc->dev, "Couldn't register USB cable notifier\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = extcon_register_notifier(dwc->edev, EXTCON_USB_HOST,
> +				       &dwc->otg_nb);
> +	if (ret < 0) {
> +		dev_err(dwc->dev, "Couldn't register USB-HOST cable notifier\n");
> +		ret = -ENODEV;
> +		goto extcon_fail;
> +	}
> +
> +	/* sanity check id & vbus states */
> +	id = extcon_get_cable_state(dwc->edev, "USB-HOST");
> +	vbus = extcon_get_cable_state(dwc->edev, "USB");
> +	if (id < 0 || vbus < 0) {
> +		dev_err(dwc->dev, "Invalid USB cable state. id %d, vbus %d\n",
> +			id, vbus);
> +		ret = -ENODEV;
> +		goto fail;
> +	}
> +
> +	/* register as DRD device with OTG core */
> +	dwc->fsm = usb_otg_register(dwc->dev, &dwc->otg_config);
> +	if (IS_ERR(dwc->fsm)) {
> +		ret = PTR_ERR(dwc->fsm);
> +		if (ret == -ENOTSUPP)
> +			dev_err(dwc->dev, "CONFIG_USB_OTG needed for dual-role\n");
> +		else
> +			dev_err(dwc->dev, "Failed to register with OTG core\n");

do you need to cope with EPROBE_DEFER ?

> +
> +		goto fail;
> +	}
> +
> +	dwc3_drd_fsm_sync(dwc);
> +
> +	return 0;
> +fail:
> +	extcon_unregister_notifier(dwc->edev, EXTCON_USB_HOST, &dwc->otg_nb);
> +extcon_fail:
> +	extcon_unregister_notifier(dwc->edev, EXTCON_USB, &dwc->otg_nb);
> +
> +	return ret;
> +}
> +
> +static void dwc3_drd_exit(struct dwc3 *dwc)
> +{
> +	usb_otg_unregister(dwc->dev);
> +	extcon_unregister_notifier(dwc->edev, EXTCON_USB_HOST, &dwc->otg_nb);
> +	extcon_unregister_notifier(dwc->edev, EXTCON_USB, &dwc->otg_nb);
> +}
> +
> +/* -------------------------------------------------------------------------- */
> +
>  static int dwc3_core_init_mode(struct dwc3 *dwc)
>  {
>  	struct device *dev = dwc->dev;
> @@ -727,13 +873,21 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>  		}
>  		break;
>  	case USB_DR_MODE_OTG:
> -		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
> +		ret = dwc3_drd_init(dwc);
> +		if (ret) {
> +			dev_err(dev, "limiting to peripheral only\n");
> +			dwc->dr_mode = USB_DR_MODE_PERIPHERAL;
> +			dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
> +			goto gadget_init;
> +		}
> +
>  		ret = dwc3_host_init(dwc);
>  		if (ret) {
>  			dev_err(dev, "failed to initialize host\n");
>  			return ret;
>  		}
>  
> +gadget_init:
>  		ret = dwc3_gadget_init(dwc);
>  		if (ret) {
>  			dev_err(dev, "failed to initialize gadget\n");
> @@ -760,6 +914,7 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc)
>  	case USB_DR_MODE_OTG:
>  		dwc3_host_exit(dwc);
>  		dwc3_gadget_exit(dwc);
> +		dwc3_drd_exit(dwc);
>  		break;
>  	default:
>  		/* do nothing */
> @@ -843,6 +998,16 @@ static int dwc3_probe(struct platform_device *pdev)
>  	hird_threshold = 12;
>  
>  	if (node) {
> +		if (of_property_read_bool(node, "extcon"))
> +			dwc->edev = extcon_get_edev_by_phandle(dev, 0);
> +		else if (of_property_read_bool(dev->parent->of_node, "extcon"))
> +			dwc->edev = extcon_get_edev_by_phandle(dev->parent, 0);

why do you need to check the parent ? Why isn't that done on the glue
layer ?

> +
> +		if (IS_ERR(dwc->edev)) {
> +			dev_vdbg(dev, "couldn't get extcon device\n");

dev_err() ??

> +			return -EPROBE_DEFER;

this could make us reschedule probe forever.

> +		}
> +
>  		dwc->maximum_speed = of_usb_get_maximum_speed(node);
>  		dwc->has_lpm_erratum = of_property_read_bool(node,
>  				"snps,has-lpm-erratum");
> @@ -887,6 +1052,13 @@ static int dwc3_probe(struct platform_device *pdev)
>  		of_property_read_string(node, "snps,hsphy_interface",
>  					&dwc->hsphy_interface);
>  	} else if (pdata) {
> +		if (pdata->extcon) {
> +			dwc->edev = extcon_get_extcon_dev(pdata->extcon);
> +			if (!dwc->edev) {
> +				dev_vdbg(dev, "couldn't get extcon device\n");
> +				return -EPROBE_DEFER;

ditto

> +			}
> +		}
>  		dwc->maximum_speed = pdata->maximum_speed;
>  		dwc->has_lpm_erratum = pdata->has_lpm_erratum;
>  		if (pdata->lpm_nyet_threshold)
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 0447788..5ca2b25 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -31,8 +31,10 @@
>  #include <linux/usb/gadget.h>
>  #include <linux/usb/otg.h>
>  #include <linux/ulpi/interface.h>
> +#include <linux/usb/otg-fsm.h>
>  
>  #include <linux/phy/phy.h>
> +#include <linux/extcon.h>
>  
>  #define DWC3_MSG_MAX	500
>  
> @@ -753,6 +755,11 @@ struct dwc3 {
>  
>  	struct ulpi		*ulpi;
>  
> +	struct extcon_dev	*edev;	/* USB cable events ID & VBUS */
> +	struct notifier_block	otg_nb;	/* notifier for USB cable events */
> +	struct otg_fsm		*fsm;
> +	struct usb_otg_config	otg_config;
> +
>  	void __iomem		*regs;
>  	size_t			regs_size;
>  
> diff --git a/drivers/usb/dwc3/platform_data.h b/drivers/usb/dwc3/platform_data.h
> index d3614ec..b3b245c 100644
> --- a/drivers/usb/dwc3/platform_data.h
> +++ b/drivers/usb/dwc3/platform_data.h
> @@ -47,4 +47,5 @@ struct dwc3_platform_data {
>  	unsigned tx_de_emphasis:2;
>  
>  	const char *hsphy_interface;
> +	const char *extcon;	/* extcon name for USB cable events ID/VBUS */
>  };
> -- 
> 2.1.4
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 1/9] usb: dwc3: add dual-role support

[Roger Quadros]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 02/09/15 17:31, Felipe Balbi wrote:
> Hi,
> 
> On Wed, Sep 02, 2015 at 05:24:16PM +0300, Roger Quadros wrote:
>> Register with the USB OTG core. Since we don't support
>> OTG yet we just work as a dual-role device even
>> if device tree says "otg".
>>
>> Use extcon framework to get VBUS/ID cable events and
>> kick the OTG state machine.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>  drivers/usb/dwc3/core.c          | 174 ++++++++++++++++++++++++++++++++++++++-
>>  drivers/usb/dwc3/core.h          |   7 ++
>>  drivers/usb/dwc3/platform_data.h |   1 +
>>  3 files changed, 181 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 064123e..2e36a9b 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -704,6 +704,152 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
>>  	return 0;
>>  }
>>  
>> +/* --------------------- Dual-Role management ------------------------------- */
>> +
>> +static void dwc3_drd_fsm_sync(struct dwc3 *dwc)
>> +{
>> +	int id, vbus;
>> +
>> +	/* get ID */
>> +	id = extcon_get_cable_state(dwc->edev, "USB-HOST");
>> +	/* Host means ID == 0 */
>> +	id = !id;
>> +
>> +	/* get VBUS */
>> +	vbus = extcon_get_cable_state(dwc->edev, "USB");
>> +	dev_dbg(dwc->dev, "id %d vbus %d\n", id, vbus);
> 
> tracepoint please. We don't want this driver to use dev_(v)?db anymore.
> Ditto to all others.

OK.

> 
>> +
>> +	dwc->fsm->id = id;
>> +	dwc->fsm->b_sess_vld = vbus;
>> +	usb_otg_sync_inputs(dwc->fsm);
>> +}
>> +
>> +static int dwc3_drd_start_host(struct otg_fsm *fsm, int on)
>> +{
>> +	struct device *dev = usb_otg_fsm_to_dev(fsm);
>> +	struct dwc3 *dwc = dev_get_drvdata(dev);
> 
> how about adding a usb_otg_get_drvdata(fsm) ?

You meant for otg core? That can be done.

> 
>> +	dev_dbg(dwc->dev, "%s: %d\n", __func__, on);
>> +	if (on) {
>> +		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
>> +		/* start the HCD */
>> +		usb_otg_start_host(fsm, true);
>> +	} else {
>> +		/* stop the HCD */
>> +		usb_otg_start_host(fsm, false);
>> +	}
> 
> This can be simplified.
> 
> 	if (on)
> 		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
> 
> 	usb_otg_start_host(fsm, on);

Indeed.

> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int dwc3_drd_start_gadget(struct otg_fsm *fsm, int on)
>> +{
>> +	struct device *dev = usb_otg_fsm_to_dev(fsm);
>> +	struct dwc3 *dwc = dev_get_drvdata(dev);
>> +
>> +	dev_dbg(dwc->dev, "%s: %d\n", __func__, on);
>> +	if (on) {
>> +		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
>> +		dwc3_event_buffers_setup(dwc);
>> +
>> +		/* start the UDC */
>> +		usb_otg_start_gadget(fsm, true);
>> +	} else {
>> +		/* stop the UDC */
>> +		usb_otg_start_gadget(fsm, false);
>> +	}
> 
> likewise:
> 
> 	if (on) {
> 		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
> 		dwc3_event_buffers_setup(dwc);
> 	}
> 
> 	usb_otg_start_gadget(fsm, on);

OK.
> 
>> +	return 0;
>> +}
>> +
>> +static struct otg_fsm_ops dwc3_drd_ops = {
>> +	.start_host = dwc3_drd_start_host,
>> +	.start_gadget = dwc3_drd_start_gadget,
>> +};
>> +
>> +static int dwc3_drd_notifier(struct notifier_block *nb,
>> +			     unsigned long event, void *ptr)
>> +{
>> +	struct dwc3 *dwc = container_of(nb, struct dwc3, otg_nb);
>> +
>> +	dwc3_drd_fsm_sync(dwc);
>> +
>> +	return NOTIFY_DONE;
>> +}
>> +
>> +static int dwc3_drd_init(struct dwc3 *dwc)
>> +{
>> +	int ret, id, vbus;
>> +	struct usb_otg_caps *otgcaps = &dwc->otg_config.otg_caps;
>> +
>> +	otgcaps->otg_rev = 0;
>> +	otgcaps->hnp_support = false;
>> +	otgcaps->srp_support = false;
>> +	otgcaps->adp_support = false;
>> +	dwc->otg_config.fsm_ops = &dwc3_drd_ops;
>> +
>> +	if (!dwc->edev) {
>> +		dev_err(dwc->dev, "No extcon device found for OTG mode\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	dwc->otg_nb.notifier_call = dwc3_drd_notifier;
>> +	ret = extcon_register_notifier(dwc->edev, EXTCON_USB, &dwc->otg_nb);
>> +	if (ret < 0) {
>> +		dev_err(dwc->dev, "Couldn't register USB cable notifier\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	ret = extcon_register_notifier(dwc->edev, EXTCON_USB_HOST,
>> +				       &dwc->otg_nb);
>> +	if (ret < 0) {
>> +		dev_err(dwc->dev, "Couldn't register USB-HOST cable notifier\n");
>> +		ret = -ENODEV;
>> +		goto extcon_fail;
>> +	}
>> +
>> +	/* sanity check id & vbus states */
>> +	id = extcon_get_cable_state(dwc->edev, "USB-HOST");
>> +	vbus = extcon_get_cable_state(dwc->edev, "USB");
>> +	if (id < 0 || vbus < 0) {
>> +		dev_err(dwc->dev, "Invalid USB cable state. id %d, vbus %d\n",
>> +			id, vbus);
>> +		ret = -ENODEV;
>> +		goto fail;
>> +	}
>> +
>> +	/* register as DRD device with OTG core */
>> +	dwc->fsm = usb_otg_register(dwc->dev, &dwc->otg_config);
>> +	if (IS_ERR(dwc->fsm)) {
>> +		ret = PTR_ERR(dwc->fsm);
>> +		if (ret == -ENOTSUPP)
>> +			dev_err(dwc->dev, "CONFIG_USB_OTG needed for dual-role\n");
>> +		else
>> +			dev_err(dwc->dev, "Failed to register with OTG core\n");
> 
> do you need to cope with EPROBE_DEFER ?

Yes, we should.

> 
>> +
>> +		goto fail;
>> +	}
>> +
>> +	dwc3_drd_fsm_sync(dwc);
>> +
>> +	return 0;
>> +fail:
>> +	extcon_unregister_notifier(dwc->edev, EXTCON_USB_HOST, &dwc->otg_nb);
>> +extcon_fail:
>> +	extcon_unregister_notifier(dwc->edev, EXTCON_USB, &dwc->otg_nb);
>> +
>> +	return ret;
>> +}
>> +
>> +static void dwc3_drd_exit(struct dwc3 *dwc)
>> +{
>> +	usb_otg_unregister(dwc->dev);
>> +	extcon_unregister_notifier(dwc->edev, EXTCON_USB_HOST, &dwc->otg_nb);
>> +	extcon_unregister_notifier(dwc->edev, EXTCON_USB, &dwc->otg_nb);
>> +}
>> +
>> +/* -------------------------------------------------------------------------- */
>> +
>>  static int dwc3_core_init_mode(struct dwc3 *dwc)
>>  {
>>  	struct device *dev = dwc->dev;
>> @@ -727,13 +873,21 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>>  		}
>>  		break;
>>  	case USB_DR_MODE_OTG:
>> -		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
>> +		ret = dwc3_drd_init(dwc);
>> +		if (ret) {

So here if ret is -EPROBE_DEFER, we return.

>> +			dev_err(dev, "limiting to peripheral only\n");
>> +			dwc->dr_mode = USB_DR_MODE_PERIPHERAL;
>> +			dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
>> +			goto gadget_init;
>> +		}
>> +
>>  		ret = dwc3_host_init(dwc);
>>  		if (ret) {
>>  			dev_err(dev, "failed to initialize host\n");
>>  			return ret;
>>  		}
>>  
>> +gadget_init:
>>  		ret = dwc3_gadget_init(dwc);
>>  		if (ret) {
>>  			dev_err(dev, "failed to initialize gadget\n");
>> @@ -760,6 +914,7 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc)
>>  	case USB_DR_MODE_OTG:
>>  		dwc3_host_exit(dwc);
>>  		dwc3_gadget_exit(dwc);
>> +		dwc3_drd_exit(dwc);
>>  		break;
>>  	default:
>>  		/* do nothing */
>> @@ -843,6 +998,16 @@ static int dwc3_probe(struct platform_device *pdev)
>>  	hird_threshold = 12;
>>  
>>  	if (node) {
>> +		if (of_property_read_bool(node, "extcon"))
>> +			dwc->edev = extcon_get_edev_by_phandle(dev, 0);
>> +		else if (of_property_read_bool(dev->parent->of_node, "extcon"))
>> +			dwc->edev = extcon_get_edev_by_phandle(dev->parent, 0);
> 
> why do you need to check the parent ? Why isn't that done on the glue
> layer ?

On DRA7-evm, the extcon device is defined in the glue layer node. But
we need the device both at the glue layer and at the core layer.
We do get the extcon device in dwc3-omap.c

Any suggestion how to pass the extcon device from glue layer to core.c?
Or should I add the extcon property to dwc3 USB node as well in the DT?

> 
>> +
>> +		if (IS_ERR(dwc->edev)) {
>> +			dev_vdbg(dev, "couldn't get extcon device\n");
> 
> dev_err() ??

Is it ok to print it even in EPROBE_DEFER case?

> 
>> +			return -EPROBE_DEFER;
> 
> this could make us reschedule probe forever.

Good catch, we must return PTR_ERR(dwc->edev).

> 
>> +		}
>> +
>>  		dwc->maximum_speed = of_usb_get_maximum_speed(node);
>>  		dwc->has_lpm_erratum = of_property_read_bool(node,
>>  				"snps,has-lpm-erratum");
>> @@ -887,6 +1052,13 @@ static int dwc3_probe(struct platform_device *pdev)
>>  		of_property_read_string(node, "snps,hsphy_interface",
>>  					&dwc->hsphy_interface);
>>  	} else if (pdata) {
>> +		if (pdata->extcon) {
>> +			dwc->edev = extcon_get_extcon_dev(pdata->extcon);
>> +			if (!dwc->edev) {
>> +				dev_vdbg(dev, "couldn't get extcon device\n");
>> +				return -EPROBE_DEFER;
> 
> ditto

OK.

> 
>> +			}
>> +		}
>>  		dwc->maximum_speed = pdata->maximum_speed;
>>  		dwc->has_lpm_erratum = pdata->has_lpm_erratum;
>>  		if (pdata->lpm_nyet_threshold)
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 0447788..5ca2b25 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -31,8 +31,10 @@
>>  #include <linux/usb/gadget.h>
>>  #include <linux/usb/otg.h>
>>  #include <linux/ulpi/interface.h>
>> +#include <linux/usb/otg-fsm.h>
>>  
>>  #include <linux/phy/phy.h>
>> +#include <linux/extcon.h>
>>  
>>  #define DWC3_MSG_MAX	500
>>  
>> @@ -753,6 +755,11 @@ struct dwc3 {
>>  
>>  	struct ulpi		*ulpi;
>>  
>> +	struct extcon_dev	*edev;	/* USB cable events ID & VBUS */
>> +	struct notifier_block	otg_nb;	/* notifier for USB cable events */
>> +	struct otg_fsm		*fsm;
>> +	struct usb_otg_config	otg_config;
>> +
>>  	void __iomem		*regs;
>>  	size_t			regs_size;
>>  
>> diff --git a/drivers/usb/dwc3/platform_data.h b/drivers/usb/dwc3/platform_data.h
>> index d3614ec..b3b245c 100644
>> --- a/drivers/usb/dwc3/platform_data.h
>> +++ b/drivers/usb/dwc3/platform_data.h
>> @@ -47,4 +47,5 @@ struct dwc3_platform_data {
>>  	unsigned tx_de_emphasis:2;
>>  
>>  	const char *hsphy_interface;
>> +	const char *extcon;	/* extcon name for USB cable events ID/VBUS */
>>  };
>> -- 
>> 2.1.4
>>
> 

- --
cheers,
- -roger
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJV6DtcAAoJENJaa9O+djCTWYcP/j/nseHR49Z1I8ftXv8urDGl
mMjMTzhlMxB7CYCtiqGpQ8Dytr9R7Ahm1llUVC4v6vYC5csJW1IcKxp/LOnaYmOL
d6EVFCSGM5JdEN83JoPfz/BD8qR/pWMmgzAGo60CkESev3E+/KxtcTl8md8CyOJj
DimMFsAmMxiVD2uABaW64Y96sG/lr8+b2AOksa6P1Os5n3GylNNegNHLSVMfx0b8
6lTn5uOa70fFZJTQDfF5idzgTrIT9irvP/GZ6RY1d6xLat4Hos1SWCjrDCHfgvHF
z3z39uW336/dpB56eomUSzctq0gVzzxG8lOIuVPXAJWEmwlx2FbV3M6viul56AGH
vtgWEB2LloLZxuXz60lnj3y8MSccUfMii+KILKtJVVjD7TW1BziW31juq+QZs+T2
yTT3YWuJVXpzob9jbzAenyU2ogDoS3vNA9cMRGOdr8cKg33z70rU/mfPcKlQKgWN
Ngve174czk9ZZgrF+l2r/m3SOqSbTDRqkxarP+KFrzNEn/hFPprSx8Encqg7Xr0t
EzUNQC64LmTxpzJwXzRuMuCqcu+plhuvSK93ymU5LkNqe+Bv5WtJaqXuxRCQ9rrY
Mn34QMFaKLz+9mLRvwGEp70MRafy44eJ5qslyj+0glL9mOB3AV7nyAArzeEJgY4/
vyyf4sA7JD9OL6NpkgVh
=ZhiZ
-----END PGP SIGNATURE-----

Re: [PATCH v4 1/9] usb: dwc3: add dual-role support

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2325 bytes --]

Hi,

On Thu, Sep 03, 2015 at 03:21:48PM +0300, Roger Quadros wrote:
> >> +	dwc->fsm->id = id;
> >> +	dwc->fsm->b_sess_vld = vbus;
> >> +	usb_otg_sync_inputs(dwc->fsm);
> >> +}
> >> +
> >> +static int dwc3_drd_start_host(struct otg_fsm *fsm, int on)
> >> +{
> >> +	struct device *dev = usb_otg_fsm_to_dev(fsm);
> >> +	struct dwc3 *dwc = dev_get_drvdata(dev);
> > 
> > how about adding a usb_otg_get_drvdata(fsm) ?
> 
> You meant for otg core? That can be done.

yeah. BTW, I think otg core needs quite a few changes to become actually
useful. Currently it's just too much pointer ping-pong going back and
forth between phy, otg core, udc and hcd.

Also, I caught a ton of issues with it and suspend/resume. You might
want to fix them before adding more users to it.

It's also rather racy and that needs fixing too. On top of all that, I
think there's too much being added to UDC just to get Dual-Role, let's
see if we can improve that too.

> >> @@ -843,6 +998,16 @@ static int dwc3_probe(struct platform_device *pdev)
> >>  	hird_threshold = 12;
> >>  
> >>  	if (node) {
> >> +		if (of_property_read_bool(node, "extcon"))
> >> +			dwc->edev = extcon_get_edev_by_phandle(dev, 0);
> >> +		else if (of_property_read_bool(dev->parent->of_node, "extcon"))
> >> +			dwc->edev = extcon_get_edev_by_phandle(dev->parent, 0);
> > 
> > why do you need to check the parent ? Why isn't that done on the glue
> > layer ?
> 
> On DRA7-evm, the extcon device is defined in the glue layer node. But
> we need the device both at the glue layer and at the core layer.

why do you need extcon here ? Glue updates core via UTMI about the
states, right ? So you should get proper VBUS and ID status via OTG IRQ.
Is that not working ?

> We do get the extcon device in dwc3-omap.c
> 
> Any suggestion how to pass the extcon device from glue layer to core.c?
> Or should I add the extcon property to dwc3 USB node as well in the DT?

GPIO toggles
  dwc3-omap extcon event
    update status via UTMI STATUS register
      OTG IRQ on core
        Horray!

:-)

> >> +
> >> +		if (IS_ERR(dwc->edev)) {
> >> +			dev_vdbg(dev, "couldn't get extcon device\n");
> > 
> > dev_err() ??
> 
> Is it ok to print it even in EPROBE_DEFER case?

hmm, probably pointless, indeed.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 1/9] usb: dwc3: add dual-role support

[Roger Quadros]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Felipe,

On 03/09/15 18:44, Felipe Balbi wrote:
> Hi,
> 
> On Thu, Sep 03, 2015 at 03:21:48PM +0300, Roger Quadros wrote:
>>>> +	dwc->fsm->id = id;
>>>> +	dwc->fsm->b_sess_vld = vbus;
>>>> +	usb_otg_sync_inputs(dwc->fsm);
>>>> +}
>>>> +
>>>> +static int dwc3_drd_start_host(struct otg_fsm *fsm, int on)
>>>> +{
>>>> +	struct device *dev = usb_otg_fsm_to_dev(fsm);
>>>> +	struct dwc3 *dwc = dev_get_drvdata(dev);
>>>
>>> how about adding a usb_otg_get_drvdata(fsm) ?
>>
>> You meant for otg core? That can be done.
> 
> yeah. BTW, I think otg core needs quite a few changes to become actually
> useful. Currently it's just too much pointer ping-pong going back and
> forth between phy, otg core, udc and hcd.

Sure, any inputs for improvement appreciated.

> 
> Also, I caught a ton of issues with it and suspend/resume. You might
> want to fix them before adding more users to it.

OK.

> 
> It's also rather racy and that needs fixing too. On top of all that, I
> think there's too much being added to UDC just to get Dual-Role, let's
> see if we can improve that too.

Would appreciate if you could give all your inputs on the otg core thread
as early as you can :)

> 
>>>> @@ -843,6 +998,16 @@ static int dwc3_probe(struct platform_device *pdev)
>>>>  	hird_threshold = 12;
>>>>  
>>>>  	if (node) {
>>>> +		if (of_property_read_bool(node, "extcon"))
>>>> +			dwc->edev = extcon_get_edev_by_phandle(dev, 0);
>>>> +		else if (of_property_read_bool(dev->parent->of_node, "extcon"))
>>>> +			dwc->edev = extcon_get_edev_by_phandle(dev->parent, 0);
>>>
>>> why do you need to check the parent ? Why isn't that done on the glue
>>> layer ?
>>
>> On DRA7-evm, the extcon device is defined in the glue layer node. But
>> we need the device both at the glue layer and at the core layer.
> 
> why do you need extcon here ? Glue updates core via UTMI about the
> states, right ? So you should get proper VBUS and ID status via OTG IRQ.
> Is that not working ?

I didn't even expect that would work. Let me give that a try.

> 
>> We do get the extcon device in dwc3-omap.c
>>
>> Any suggestion how to pass the extcon device from glue layer to core.c?
>> Or should I add the extcon property to dwc3 USB node as well in the DT?
> 
> GPIO toggles
>   dwc3-omap extcon event
>     update status via UTMI STATUS register
>       OTG IRQ on core
>         Horray!
> 
> :-)

That's great. Thanks :)

> 
>>>> +
>>>> +		if (IS_ERR(dwc->edev)) {
>>>> +			dev_vdbg(dev, "couldn't get extcon device\n");
>>>
>>> dev_err() ??
>>
>> Is it ok to print it even in EPROBE_DEFER case?
> 
> hmm, probably pointless, indeed.
> 

cheers,
- -roger
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJV6V8UAAoJENJaa9O+djCToUwP/1SRrcUG/1KG54c1loxKFvYG
51x17TiVuQc68xtrp56HYJNRpZzZIwBqlfa3LU19bd/p6alYGaaY6jOqW+gFWWt8
tJd+7lyjVuCWkrJBQ6obK4yDPK4r6ZLcFlCTLGsfnd6SSBdsGxNrpcNCwz2rhhE/
uTPQdU4wvgxDFnGFVtTMhM+/ehtJtkKB4dppoFA5Vw86vsKinJ7o5EJMwho/PwJu
4D+mQVi17kQDBx1wkQPBxyVHD6RXfjMBLK+zg46T6lsg1eUodkv22Grf9Xy4i4wT
9Pr3g6SFnczkKiU1Bp7q4TV048SY0KedA7oe1U7K9B+hjHK4Fc2/vxtb3kGyNK7x
VPkN3NbvqSZalcgmdoDnv6VvU5NdnZUyKasVeJQDp9Fzaom9rUmNvic0ZLx9TOTP
4e4R2ovyCFnU1nODDUccYDInPiJ3EUo6D7CX3L7rfud5h6qAXcqC6vzZ3oHcSnBu
S1PXPTjLyk/a+den4gf41ReF5xKXzNocH21cXe7oFwKDaCJ9VgEn4+E5tX0vK778
KGHz3qFEQGkM6Eib0tEZEdHUEgeO8H99rNAELFNTrtS0FPZIEtQPHr2lmFDM/iYC
sEECc/HFP+LvmAzuJLA4XRUeNh6xo0K/ZDOtX1YOCCqnjy/UHNKvB4gUDzVaNti+
DB0wWXP+/A9Qz1PBK1dG
=rFBL
-----END PGP SIGNATURE-----

Re: [PATCH v4 1/9] usb: dwc3: add dual-role support

[Roger Quadros]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 04/09/15 12:06, Roger Quadros wrote:
> Felipe,
> 
> On 03/09/15 18:44, Felipe Balbi wrote:
>> Hi,
> 
>> On Thu, Sep 03, 2015 at 03:21:48PM +0300, Roger Quadros wrote:
>>>>> +	dwc->fsm->id = id;
>>>>> +	dwc->fsm->b_sess_vld = vbus;
>>>>> +	usb_otg_sync_inputs(dwc->fsm);
>>>>> +}
>>>>> +
>>>>> +static int dwc3_drd_start_host(struct otg_fsm *fsm, int on)
>>>>> +{
>>>>> +	struct device *dev = usb_otg_fsm_to_dev(fsm);
>>>>> +	struct dwc3 *dwc = dev_get_drvdata(dev);
>>>>
>>>> how about adding a usb_otg_get_drvdata(fsm) ?
>>>
>>> You meant for otg core? That can be done.
> 
>> yeah. BTW, I think otg core needs quite a few changes to become actually
>> useful. Currently it's just too much pointer ping-pong going back and
>> forth between phy, otg core, udc and hcd.
> 
> Sure, any inputs for improvement appreciated.
> 
> 
>> Also, I caught a ton of issues with it and suspend/resume. You might
>> want to fix them before adding more users to it.
> 
> OK.
> 
> 
>> It's also rather racy and that needs fixing too. On top of all that, I
>> think there's too much being added to UDC just to get Dual-Role, let's
>> see if we can improve that too.
> 
> Would appreciate if you could give all your inputs on the otg core thread
> as early as you can :)
> 
> 
>>>>> @@ -843,6 +998,16 @@ static int dwc3_probe(struct platform_device *pdev)
>>>>>  	hird_threshold = 12;
>>>>>  
>>>>>  	if (node) {
>>>>> +		if (of_property_read_bool(node, "extcon"))
>>>>> +			dwc->edev = extcon_get_edev_by_phandle(dev, 0);
>>>>> +		else if (of_property_read_bool(dev->parent->of_node, "extcon"))
>>>>> +			dwc->edev = extcon_get_edev_by_phandle(dev->parent, 0);
>>>>
>>>> why do you need to check the parent ? Why isn't that done on the glue
>>>> layer ?
>>>
>>> On DRA7-evm, the extcon device is defined in the glue layer node. But
>>> we need the device both at the glue layer and at the core layer.
> 
>> why do you need extcon here ? Glue updates core via UTMI about the
>> states, right ? So you should get proper VBUS and ID status via OTG IRQ.
>> Is that not working ?
> 
> I didn't even expect that would work. Let me give that a try.
> 
> 
>>> We do get the extcon device in dwc3-omap.c
>>>
>>> Any suggestion how to pass the extcon device from glue layer to core.c?
>>> Or should I add the extcon property to dwc3 USB node as well in the DT?
> 
>> GPIO toggles
>>   dwc3-omap extcon event
>>     update status via UTMI STATUS register
>>       OTG IRQ on core
>>         Horray!
> 
>> :-)
> 
> That's great. Thanks :)

This approach worked. Had to do the following change to the dwc3-omap glue
to make it work.

From: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
Date: Fri, 4 Sep 2015 15:13:59 +0300
Subject: [PATCH] usb: dwc3: omap: Pass VBUS and ID events transparently

Don't make any decisions regarding VBUS session based on ID
status. That is best left to the OTG core.

Pass ID and VBUS events independent of each other so that OTG
core knows exactly what to do.

This makes dual-role with extcon work with OTG irq on OMAP platforms.

Signed-off-by: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
- ---
 drivers/usb/dwc3/dwc3-omap.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
index b18f2a3..751feee 100644
- --- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -233,19 +233,14 @@ static void dwc3_omap_set_mailbox(struct dwc3_omap *omap,
 		}
 
 		val = dwc3_omap_read_utmi_ctrl(omap);
- -		val &= ~(USBOTGSS_UTMI_OTG_CTRL_IDDIG
- -				| USBOTGSS_UTMI_OTG_CTRL_VBUSVALID
- -				| USBOTGSS_UTMI_OTG_CTRL_SESSEND);
- -		val |= USBOTGSS_UTMI_OTG_CTRL_SESSVALID
- -				| USBOTGSS_UTMI_OTG_CTRL_POWERPRESENT;
+		val &= ~USBOTGSS_UTMI_OTG_CTRL_IDDIG;
 		dwc3_omap_write_utmi_ctrl(omap, val);
 		break;
 
 	case OMAP_DWC3_VBUS_VALID:
 		val = dwc3_omap_read_utmi_ctrl(omap);
 		val &= ~USBOTGSS_UTMI_OTG_CTRL_SESSEND;
- -		val |= USBOTGSS_UTMI_OTG_CTRL_IDDIG
- -				| USBOTGSS_UTMI_OTG_CTRL_VBUSVALID
+		val |= USBOTGSS_UTMI_OTG_CTRL_VBUSVALID
 				| USBOTGSS_UTMI_OTG_CTRL_SESSVALID
 				| USBOTGSS_UTMI_OTG_CTRL_POWERPRESENT;
 		dwc3_omap_write_utmi_ctrl(omap, val);
@@ -254,14 +249,16 @@ static void dwc3_omap_set_mailbox(struct dwc3_omap *omap,
 	case OMAP_DWC3_ID_FLOAT:
 		if (omap->vbus_reg)
 			regulator_disable(omap->vbus_reg);
+		val = dwc3_omap_read_utmi_ctrl(omap);
+		val |= USBOTGSS_UTMI_OTG_CTRL_IDDIG;
+		dwc3_omap_write_utmi_ctrl(omap, val);
 
 	case OMAP_DWC3_VBUS_OFF:
 		val = dwc3_omap_read_utmi_ctrl(omap);
 		val &= ~(USBOTGSS_UTMI_OTG_CTRL_SESSVALID
 				| USBOTGSS_UTMI_OTG_CTRL_VBUSVALID
 				| USBOTGSS_UTMI_OTG_CTRL_POWERPRESENT);
- -		val |= USBOTGSS_UTMI_OTG_CTRL_SESSEND
- -				| USBOTGSS_UTMI_OTG_CTRL_IDDIG;
+		val |= USBOTGSS_UTMI_OTG_CTRL_SESSEND;
 		dwc3_omap_write_utmi_ctrl(omap, val);
 		break;
 
- -- 
2.1.4
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJV7VwcAAoJENJaa9O+djCTlNEP/1ymxHQuTvTRupMB+WsGf/Md
DhgI9SAPNK8luElKYZMNIs7r+QyjoASubEmRqSjGrj7BtrXfLKwFNjktse+6x32l
ELjWgmMvU9BnO2vrrPXFafhoF42ywdqtB/3bVGfqTNubMZArI4QeXy8VWdQVPmTg
CoEl65Ta3nNyji0T0yXzklak7/cSJ1lU/RcBPQVJHc8FSmxD6EtM1U51QilojrtC
mtYfMzVpS1yTfvhnmwYLBip09eBTWRmgWPsQd7Y6uABJzFb02x6Q0Jd36SeaD6d8
jfowOa4FZ29mrUTi5tKGfHxgc2ZiD81vhFUdUJtgnlfc30rNEVNJDF8YtYEKHe5q
JQy7zCXRHX0XB+qwbs9RfFzDLNvSmot1cDuf78SPYbG/SR84txklBoTdA0rLKE82
xTU2l5ZPHJL6NULvte0KsR/OVwqtAnvB1xZ/o3HPrKfW7HBpfGkumVVOqE2bTRA0
5QrOrI9Txyjl+aAL4Tt15F8NM0w6m9mpbfJMXOY7gQM/2t3LbSfSeh8/P9sw3qxn
z0+tZ2cFXIm4o7zNtrKzXU3qWBvD7FDQ3m2Gev3WhrIUZMjrpVk5/6r8FXIJcPEs
46+vPD7BC48r7jAgqDA2rJRaq78PyUAkQ9oZssIqPZMPhS1ABtPFUshuxRJZd60A
ey0RU5H6TZDOHEQN0tW5
=+73W
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/9] usb: dwc3: add dual-role support

[Peter Chen]

On Wed, Sep 02, 2015 at 05:24:16PM +0300, Roger Quadros wrote:
> Register with the USB OTG core. Since we don't support
> OTG yet we just work as a dual-role device even
> if device tree says "otg".
> 
> +
> +static int dwc3_drd_init(struct dwc3 *dwc)
> +{
> +	int ret, id, vbus;
> +	struct usb_otg_caps *otgcaps = &dwc->otg_config.otg_caps;
> +
> +	otgcaps->otg_rev = 0;
> +	otgcaps->hnp_support = false;
> +	otgcaps->srp_support = false;
> +	otgcaps->adp_support = false;
> +	dwc->otg_config.fsm_ops = &dwc3_drd_ops;
> +
> +	if (!dwc->edev) {
> +		dev_err(dwc->dev, "No extcon device found for OTG mode\n");
> +		return -ENODEV;
> +	}
> +

Do All dwc3 platforms id/vbus need to get through extcon? Do the
SoCs have id/vbus pin?


-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/9] usb: dwc3: add dual-role support

[Roger Quadros]

Peter,

On 06/09/15 05:02, Peter Chen wrote:
> On Wed, Sep 02, 2015 at 05:24:16PM +0300, Roger Quadros wrote:
>> Register with the USB OTG core. Since we don't support
>> OTG yet we just work as a dual-role device even
>> if device tree says "otg".
>>
>> +
>> +static int dwc3_drd_init(struct dwc3 *dwc)
>> +{
>> +	int ret, id, vbus;
>> +	struct usb_otg_caps *otgcaps = &dwc->otg_config.otg_caps;
>> +
>> +	otgcaps->otg_rev = 0;
>> +	otgcaps->hnp_support = false;
>> +	otgcaps->srp_support = false;
>> +	otgcaps->adp_support = false;
>> +	dwc->otg_config.fsm_ops = &dwc3_drd_ops;
>> +
>> +	if (!dwc->edev) {
>> +		dev_err(dwc->dev, "No extcon device found for OTG mode\n");
>> +		return -ENODEV;
>> +	}
>> +
> 
> Do All dwc3 platforms id/vbus need to get through extcon? Do the
> SoCs have id/vbus pin?
> 
> 

Extcon access is in fact not needed from dwc3 driver and I will be getting
rid of this patch. We will support dual-role only via the OTG irq as in patch 5.

The way it works is that the OMAP glue layer dwc3-omap.c requests extcon device
and sets some mailbox register and this causes the VBUS/ID events to come over
OTG irq/status. So this patch is redundant.

The extcon device is not needed for all TI platforms. e.g. we need it for
DRA7 but not for AM437x.

cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 5/9] usb: dwc3: core: make dual-role work with OTG irq

[Roger Quadros]

If the ID pin event is not available over extcon
then we rely on the OTG controller to provide us ID and VBUS
information.

We still don't support any OTG features but just
dual-role operation.

Signed-off-by: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
---
 drivers/usb/dwc3/core.c | 217 ++++++++++++++++++++++++++++++++++++++++++++----
 drivers/usb/dwc3/core.h |   3 +
 2 files changed, 205 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 38b31df..632ee53 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -704,6 +704,63 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
 	return 0;
 }
 
+/* Get OTG events and sync it to OTG fsm */
+static void dwc3_otg_fsm_sync(struct dwc3 *dwc)
+{
+	u32 reg;
+	int id, vbus;
+
+	reg = dwc3_readl(dwc->regs, DWC3_OSTS);
+	dev_dbg(dwc->dev, "otgstatus 0x%x\n", reg);
+
+	id = !!(reg & DWC3_OSTS_CONIDSTS);
+	vbus = !!(reg & DWC3_OSTS_BSESVLD);
+
+	if (id != dwc->fsm->id || vbus != dwc->fsm->b_sess_vld) {
+		dev_dbg(dwc->dev, "id %d vbus %d\n", id, vbus);
+		dwc->fsm->id = id;
+		dwc->fsm->b_sess_vld = vbus;
+		usb_otg_sync_inputs(dwc->fsm);
+	}
+}
+
+static irqreturn_t dwc3_otg_thread_irq(int irq, void *_dwc)
+{
+	struct dwc3 *dwc = _dwc;
+	unsigned long flags;
+	irqreturn_t ret = IRQ_NONE;
+
+	spin_lock_irqsave(&dwc->lock, flags);
+	dwc3_otg_fsm_sync(dwc);
+	/* unmask interrupts */
+	dwc3_writel(dwc->regs, DWC3_OEVTEN, dwc->oevten);
+	spin_unlock_irqrestore(&dwc->lock, flags);
+
+	return ret;
+}
+
+static irqreturn_t dwc3_otg_irq(int irq, void *_dwc)
+{
+	struct dwc3 *dwc = _dwc;
+	irqreturn_t ret = IRQ_NONE;
+	u32 reg;
+
+	spin_lock(&dwc->lock);
+
+	reg = dwc3_readl(dwc->regs, DWC3_OEVT);
+	if (reg) {
+		dwc3_writel(dwc->regs, DWC3_OEVT, reg);
+		/* mask interrupts till processed */
+		dwc->oevten = dwc3_readl(dwc->regs, DWC3_OEVTEN);
+		dwc3_writel(dwc->regs, DWC3_OEVTEN, 0);
+		ret = IRQ_WAKE_THREAD;
+	}
+
+	spin_unlock(&dwc->lock);
+
+	return ret;
+}
+
 /* --------------------- Dual-Role management ------------------------------- */
 
 static void dwc3_drd_fsm_sync(struct dwc3 *dwc)
@@ -728,15 +785,44 @@ static int dwc3_drd_start_host(struct otg_fsm *fsm, int on)
 {
 	struct device *dev = usb_otg_fsm_to_dev(fsm);
 	struct dwc3 *dwc = dev_get_drvdata(dev);
+	u32 reg;
 
 	dev_dbg(dwc->dev, "%s: %d\n", __func__, on);
+	if (dwc->edev) {
+		if (on) {
+			dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
+			/* start the HCD */
+			usb_otg_start_host(fsm, true);
+		} else {
+			/* stop the HCD */
+			usb_otg_start_host(fsm, false);
+		}
+
+		return 0;
+	}
+
+	/* switch OTG core */
 	if (on) {
-		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
+		/* OCTL.PeriMode = 0 */
+		reg = dwc3_readl(dwc->regs, DWC3_OCTL);
+		reg &= ~DWC3_OCTL_PERIMODE;
+		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
+		/* unconditionally turn on VBUS */
+		reg |= DWC3_OCTL_PRTPWRCTL;
+		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
 		/* start the HCD */
 		usb_otg_start_host(fsm, true);
 	} else {
 		/* stop the HCD */
 		usb_otg_start_host(fsm, false);
+		/* turn off VBUS */
+		reg = dwc3_readl(dwc->regs, DWC3_OCTL);
+		reg &= ~DWC3_OCTL_PRTPWRCTL;
+		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
+		/* OCTL.PeriMode = 1 */
+		reg = dwc3_readl(dwc->regs, DWC3_OCTL);
+		reg |= DWC3_OCTL_PERIMODE;
+		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
 	}
 
 	return 0;
@@ -746,15 +832,48 @@ static int dwc3_drd_start_gadget(struct otg_fsm *fsm, int on)
 {
 	struct device *dev = usb_otg_fsm_to_dev(fsm);
 	struct dwc3 *dwc = dev_get_drvdata(dev);
+	u32 reg;
 
 	dev_dbg(dwc->dev, "%s: %d\n", __func__, on);
-	if (on) {
-		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
+	if (on)
 		dwc3_event_buffers_setup(dwc);
 
+	if (dwc->edev) {
+		if (on) {
+			dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
+			usb_otg_start_gadget(fsm, true);
+		} else {
+			usb_otg_start_gadget(fsm, false);
+		}
+
+		return 0;
+	}
+
+	/* switch OTG core */
+	if (on) {
+		/* OCTL.PeriMode = 1 */
+		reg = dwc3_readl(dwc->regs, DWC3_OCTL);
+		reg |= DWC3_OCTL_PERIMODE;
+		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
+		/* GUSB2PHYCFG0.SusPHY = 1 */
+		if (!dwc->dis_u2_susphy_quirk) {
+			reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
+			reg |= DWC3_GUSB2PHYCFG_SUSPHY;
+			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
+		}
 		/* start the UDC */
 		usb_otg_start_gadget(fsm, true);
 	} else {
+		/* GUSB2PHYCFG0.SusPHY=0 */
+		if (!dwc->dis_u2_susphy_quirk) {
+			reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
+			reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
+			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
+		}
+		/* OCTL.PeriMode = 1 */
+		reg = dwc3_readl(dwc->regs, DWC3_OCTL);
+		reg |= DWC3_OCTL_PERIMODE;
+		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
 		/* stop the UDC */
 		usb_otg_start_gadget(fsm, false);
 	}
@@ -777,10 +896,30 @@ static int dwc3_drd_notifier(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }
 
+static int dwc3_drd_register(struct dwc3 *dwc)
+{
+	int ret;
+
+	/* register parent as DRD device with OTG core */
+	dwc->fsm = usb_otg_register(dwc->dev, &dwc->otg_config);
+	if (IS_ERR(dwc->fsm)) {
+		ret = PTR_ERR(dwc->fsm);
+		if (ret == -ENOTSUPP)
+			dev_err(dwc->dev, "CONFIG_USB_OTG needed for dual-role\n");
+		else
+			dev_err(dwc->dev, "Failed to register with OTG core\n");
+
+		return ret;
+	}
+
+	return 0;
+}
+
 static int dwc3_drd_init(struct dwc3 *dwc)
 {
 	int ret, id, vbus;
 	struct usb_otg_caps *otgcaps = &dwc->otg_config.otg_caps;
+	u32 reg;
 
 	otgcaps->otg_rev = 0;
 	otgcaps->hnp_support = false;
@@ -788,9 +927,10 @@ static int dwc3_drd_init(struct dwc3 *dwc)
 	otgcaps->adp_support = false;
 	dwc->otg_config.fsm_ops = &dwc3_drd_ops;
 
+	/* If extcon device is not present we rely on OTG core for ID event */
 	if (!dwc->edev) {
-		dev_err(dwc->dev, "No extcon device found for OTG mode\n");
-		return -ENODEV;
+		dev_dbg(dwc->dev, "No extcon device found for OTG mode\n");
+		goto try_otg_core;
 	}
 
 	dwc->otg_nb.notifier_call = dwc3_drd_notifier;
@@ -818,17 +958,9 @@ static int dwc3_drd_init(struct dwc3 *dwc)
 		goto fail;
 	}
 
-	/* register as DRD device with OTG core */
-	dwc->fsm = usb_otg_register(dwc->dev, &dwc->otg_config);
-	if (IS_ERR(dwc->fsm)) {
-		ret = PTR_ERR(dwc->fsm);
-		if (ret == -ENOTSUPP)
-			dev_err(dwc->dev, "CONFIG_USB_OTG needed for dual-role\n");
-		else
-			dev_err(dwc->dev, "Failed to register with OTG core\n");
-
+	ret = dwc3_drd_register(dwc);
+	if (ret)
 		goto fail;
-	}
 
 	dwc3_drd_fsm_sync(dwc);
 
@@ -839,6 +971,61 @@ extcon_fail:
 	extcon_unregister_notifier(dwc->edev, EXTCON_USB, &dwc->otg_nb);
 
 	return ret;
+
+try_otg_core:
+	ret = dwc3_drd_register(dwc);
+	if (ret)
+		return ret;
+
+	/* disable all irqs */
+	dwc3_writel(dwc->regs, DWC3_OEVTEN, 0);
+	/* clear all events */
+	dwc3_writel(dwc->regs, DWC3_OEVT, ~0);
+
+	ret = request_threaded_irq(dwc->otg_irq, dwc3_otg_irq,
+				   dwc3_otg_thread_irq,
+				   IRQF_SHARED, "dwc3-otg", dwc);
+	if (ret) {
+		dev_err(dwc->dev, "failed to request irq #%d --> %d\n",
+			dwc->otg_irq, ret);
+		ret = -ENODEV;
+		goto error;
+	}
+
+	/* we need to set OTG to get events from OTG core */
+	dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
+	/* GUSB2PHYCFG0.SusPHY=0 */
+	if (!dwc->dis_u2_susphy_quirk) {
+		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
+		reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
+		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
+	}
+
+	/* Initialize OTG registers */
+	/*
+	 * Prevent host/device reset from resetting OTG core.
+	 * If we don't do this then xhci_reset (USBCMD.HCRST) will reset
+	 * the signal outputs sent to the PHY, the OTG FSM logic of the
+	 * core and also the resets to the VBUS filters inside the core.
+	 */
+	reg = DWC3_OCFG_SFTRSTMASK;
+	dwc3_writel(dwc->regs, DWC3_OCFG, reg);
+	/* Enable ID event interrupt */
+	dwc3_writel(dwc->regs, DWC3_OEVTEN, DWC3_OEVTEN_CONIDSTSCHNGEN |
+			DWC3_OEVTEN_BDEVVBUSCHNGE |
+			DWC3_OEVTEN_BDEVSESSVLDDETEN);
+	/* OCTL.PeriMode = 1 */
+	dwc3_writel(dwc->regs, DWC3_OCTL, DWC3_OCTL_PERIMODE);
+
+	dwc3_otg_fsm_sync(dwc);
+	usb_otg_sync_inputs(dwc->fsm);
+
+	return 0;
+
+error:
+	usb_otg_unregister(dwc->dev);
+
+	return ret;
 }
 
 static void dwc3_drd_exit(struct dwc3 *dwc)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index bd32cb2..129ef37 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -736,6 +736,7 @@ struct dwc3_scratchpad_array {
  * @gadget_driver: pointer to the gadget driver
  * @regs: base address for our registers
  * @regs_size: address space size
+ * @oevten: otg interrupt enable mask copy
  * @nr_scratch: number of scratch buffers
  * @num_event_buffers: calculated number of event buffers
  * @u1u2: only used on revisions <1.83a for workaround
@@ -858,6 +859,8 @@ struct dwc3 {
 	u32			dcfg;
 	u32			gctl;
 
+	u32			oevten;
+
 	u32			nr_scratch;
 	u32			num_event_buffers;
 	u32			u1u2;
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 5/9] usb: dwc3: core: make dual-role work with OTG irq

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 10608 bytes --]

Hi,

On Wed, Sep 02, 2015 at 05:24:20PM +0300, Roger Quadros wrote:
> If the ID pin event is not available over extcon
> then we rely on the OTG controller to provide us ID and VBUS
> information.
> 
> We still don't support any OTG features but just
> dual-role operation.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  drivers/usb/dwc3/core.c | 217 ++++++++++++++++++++++++++++++++++++++++++++----
>  drivers/usb/dwc3/core.h |   3 +
>  2 files changed, 205 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 38b31df..632ee53 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -704,6 +704,63 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
>  	return 0;
>  }
>  
> +/* Get OTG events and sync it to OTG fsm */
> +static void dwc3_otg_fsm_sync(struct dwc3 *dwc)
> +{
> +	u32 reg;
> +	int id, vbus;
> +
> +	reg = dwc3_readl(dwc->regs, DWC3_OSTS);
> +	dev_dbg(dwc->dev, "otgstatus 0x%x\n", reg);
> +
> +	id = !!(reg & DWC3_OSTS_CONIDSTS);
> +	vbus = !!(reg & DWC3_OSTS_BSESVLD);
> +
> +	if (id != dwc->fsm->id || vbus != dwc->fsm->b_sess_vld) {
> +		dev_dbg(dwc->dev, "id %d vbus %d\n", id, vbus);
> +		dwc->fsm->id = id;
> +		dwc->fsm->b_sess_vld = vbus;
> +		usb_otg_sync_inputs(dwc->fsm);
> +	}

this guy shouldn't try to filter events here. That's what the FSM should
be doing.

> +}
> +
> +static irqreturn_t dwc3_otg_thread_irq(int irq, void *_dwc)
> +{
> +	struct dwc3 *dwc = _dwc;
> +	unsigned long flags;
> +	irqreturn_t ret = IRQ_NONE;

this IRQ will be disabled pretty quickly. You *always* return IRQ_NONE

> +	spin_lock_irqsave(&dwc->lock, flags);

if you cache current OSTS in dwc3, you can use that instead and change
this to a spin_lock() instead of disabling IRQs here. This device's IRQs
are already masked anyway.

> +	dwc3_otg_fsm_sync(dwc);
> +	/* unmask interrupts */
> +	dwc3_writel(dwc->regs, DWC3_OEVTEN, dwc->oevten);
> +	spin_unlock_irqrestore(&dwc->lock, flags);
> +
> +	return ret;
> +}
> +
> +static irqreturn_t dwc3_otg_irq(int irq, void *_dwc)
> +{
> +	struct dwc3 *dwc = _dwc;
> +	irqreturn_t ret = IRQ_NONE;
> +	u32 reg;
> +
> +	spin_lock(&dwc->lock);

this seems unnecessary, we're already in hardirq with IRQs disabled.
What sort of race could we have ? (in fact, this also needs change in
dwc3/gadget.c).

> +
> +	reg = dwc3_readl(dwc->regs, DWC3_OEVT);
> +	if (reg) {
> +		dwc3_writel(dwc->regs, DWC3_OEVT, reg);
> +		/* mask interrupts till processed */
> +		dwc->oevten = dwc3_readl(dwc->regs, DWC3_OEVTEN);
> +		dwc3_writel(dwc->regs, DWC3_OEVTEN, 0);
> +		ret = IRQ_WAKE_THREAD;
> +	}
> +
> +	spin_unlock(&dwc->lock);
> +
> +	return ret;
> +}
> +
>  /* --------------------- Dual-Role management ------------------------------- */
>  
>  static void dwc3_drd_fsm_sync(struct dwc3 *dwc)
> @@ -728,15 +785,44 @@ static int dwc3_drd_start_host(struct otg_fsm *fsm, int on)
>  {
>  	struct device *dev = usb_otg_fsm_to_dev(fsm);
>  	struct dwc3 *dwc = dev_get_drvdata(dev);
> +	u32 reg;
>  
>  	dev_dbg(dwc->dev, "%s: %d\n", __func__, on);
> +	if (dwc->edev) {
> +		if (on) {
> +			dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
> +			/* start the HCD */
> +			usb_otg_start_host(fsm, true);
> +		} else {
> +			/* stop the HCD */
> +			usb_otg_start_host(fsm, false);
> +		}

		if (on)
			dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
		usb_otg_start_host(fsm, on);

> +
> +		return 0;
> +	}
> +
> +	/* switch OTG core */
>  	if (on) {
> -		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
> +		/* OCTL.PeriMode = 0 */
> +		reg = dwc3_readl(dwc->regs, DWC3_OCTL);
> +		reg &= ~DWC3_OCTL_PERIMODE;
> +		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
> +		/* unconditionally turn on VBUS */
> +		reg |= DWC3_OCTL_PRTPWRCTL;
> +		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
>  		/* start the HCD */
>  		usb_otg_start_host(fsm, true);
>  	} else {
>  		/* stop the HCD */
>  		usb_otg_start_host(fsm, false);
> +		/* turn off VBUS */
> +		reg = dwc3_readl(dwc->regs, DWC3_OCTL);
> +		reg &= ~DWC3_OCTL_PRTPWRCTL;
> +		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
> +		/* OCTL.PeriMode = 1 */
> +		reg = dwc3_readl(dwc->regs, DWC3_OCTL);
> +		reg |= DWC3_OCTL_PERIMODE;
> +		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
>  	}

it looks like you're not really following the fluxchart from SNPS
documentation, see Figure 11-4 on section 11.1.4.5

> @@ -746,15 +832,48 @@ static int dwc3_drd_start_gadget(struct otg_fsm *fsm, int on)
>  {
>  	struct device *dev = usb_otg_fsm_to_dev(fsm);
>  	struct dwc3 *dwc = dev_get_drvdata(dev);
> +	u32 reg;
>  
>  	dev_dbg(dwc->dev, "%s: %d\n", __func__, on);
> -	if (on) {
> -		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
> +	if (on)
>  		dwc3_event_buffers_setup(dwc);
>  
> +	if (dwc->edev) {
> +		if (on) {
> +			dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
> +			usb_otg_start_gadget(fsm, true);
> +		} else {
> +			usb_otg_start_gadget(fsm, false);
> +		}
> +
> +		return 0;
> +	}
> +
> +	/* switch OTG core */
> +	if (on) {
> +		/* OCTL.PeriMode = 1 */
> +		reg = dwc3_readl(dwc->regs, DWC3_OCTL);
> +		reg |= DWC3_OCTL_PERIMODE;
> +		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
> +		/* GUSB2PHYCFG0.SusPHY = 1 */
> +		if (!dwc->dis_u2_susphy_quirk) {
> +			reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> +			reg |= DWC3_GUSB2PHYCFG_SUSPHY;
> +			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
> +		}
>  		/* start the UDC */
>  		usb_otg_start_gadget(fsm, true);
>  	} else {
> +		/* GUSB2PHYCFG0.SusPHY=0 */
> +		if (!dwc->dis_u2_susphy_quirk) {
> +			reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> +			reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
> +			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
> +		}
> +		/* OCTL.PeriMode = 1 */
> +		reg = dwc3_readl(dwc->regs, DWC3_OCTL);
> +		reg |= DWC3_OCTL_PERIMODE;
> +		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
>  		/* stop the UDC */
>  		usb_otg_start_gadget(fsm, false);
>  	}
> @@ -777,10 +896,30 @@ static int dwc3_drd_notifier(struct notifier_block *nb,
>  	return NOTIFY_DONE;
>  }
>  
> +static int dwc3_drd_register(struct dwc3 *dwc)
> +{
> +	int ret;
> +
> +	/* register parent as DRD device with OTG core */
> +	dwc->fsm = usb_otg_register(dwc->dev, &dwc->otg_config);
> +	if (IS_ERR(dwc->fsm)) {
> +		ret = PTR_ERR(dwc->fsm);
> +		if (ret == -ENOTSUPP)
> +			dev_err(dwc->dev, "CONFIG_USB_OTG needed for dual-role\n");
> +		else
> +			dev_err(dwc->dev, "Failed to register with OTG core\n");
> +
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int dwc3_drd_init(struct dwc3 *dwc)
>  {
>  	int ret, id, vbus;
>  	struct usb_otg_caps *otgcaps = &dwc->otg_config.otg_caps;
> +	u32 reg;
>  
>  	otgcaps->otg_rev = 0;
>  	otgcaps->hnp_support = false;
> @@ -788,9 +927,10 @@ static int dwc3_drd_init(struct dwc3 *dwc)
>  	otgcaps->adp_support = false;
>  	dwc->otg_config.fsm_ops = &dwc3_drd_ops;
>  
> +	/* If extcon device is not present we rely on OTG core for ID event */
>  	if (!dwc->edev) {
> -		dev_err(dwc->dev, "No extcon device found for OTG mode\n");
> -		return -ENODEV;
> +		dev_dbg(dwc->dev, "No extcon device found for OTG mode\n");
> +		goto try_otg_core;
>  	}
>  
>  	dwc->otg_nb.notifier_call = dwc3_drd_notifier;
> @@ -818,17 +958,9 @@ static int dwc3_drd_init(struct dwc3 *dwc)
>  		goto fail;
>  	}
>  
> -	/* register as DRD device with OTG core */
> -	dwc->fsm = usb_otg_register(dwc->dev, &dwc->otg_config);
> -	if (IS_ERR(dwc->fsm)) {
> -		ret = PTR_ERR(dwc->fsm);
> -		if (ret == -ENOTSUPP)
> -			dev_err(dwc->dev, "CONFIG_USB_OTG needed for dual-role\n");
> -		else
> -			dev_err(dwc->dev, "Failed to register with OTG core\n");
> -
> +	ret = dwc3_drd_register(dwc);
> +	if (ret)
>  		goto fail;
> -	}
>  
>  	dwc3_drd_fsm_sync(dwc);
>  
> @@ -839,6 +971,61 @@ extcon_fail:
>  	extcon_unregister_notifier(dwc->edev, EXTCON_USB, &dwc->otg_nb);
>  
>  	return ret;
> +
> +try_otg_core:
> +	ret = dwc3_drd_register(dwc);
> +	if (ret)
> +		return ret;
> +
> +	/* disable all irqs */
> +	dwc3_writel(dwc->regs, DWC3_OEVTEN, 0);
> +	/* clear all events */
> +	dwc3_writel(dwc->regs, DWC3_OEVT, ~0);
> +
> +	ret = request_threaded_irq(dwc->otg_irq, dwc3_otg_irq,
> +				   dwc3_otg_thread_irq,
> +				   IRQF_SHARED, "dwc3-otg", dwc);
> +	if (ret) {
> +		dev_err(dwc->dev, "failed to request irq #%d --> %d\n",
> +			dwc->otg_irq, ret);
> +		ret = -ENODEV;
> +		goto error;
> +	}
> +
> +	/* we need to set OTG to get events from OTG core */
> +	dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
> +	/* GUSB2PHYCFG0.SusPHY=0 */
> +	if (!dwc->dis_u2_susphy_quirk) {
> +		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> +		reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
> +		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
> +	}
> +
> +	/* Initialize OTG registers */
> +	/*
> +	 * Prevent host/device reset from resetting OTG core.
> +	 * If we don't do this then xhci_reset (USBCMD.HCRST) will reset
> +	 * the signal outputs sent to the PHY, the OTG FSM logic of the
> +	 * core and also the resets to the VBUS filters inside the core.
> +	 */
> +	reg = DWC3_OCFG_SFTRSTMASK;
> +	dwc3_writel(dwc->regs, DWC3_OCFG, reg);
> +	/* Enable ID event interrupt */
> +	dwc3_writel(dwc->regs, DWC3_OEVTEN, DWC3_OEVTEN_CONIDSTSCHNGEN |
> +			DWC3_OEVTEN_BDEVVBUSCHNGE |
> +			DWC3_OEVTEN_BDEVSESSVLDDETEN);
> +	/* OCTL.PeriMode = 1 */
> +	dwc3_writel(dwc->regs, DWC3_OCTL, DWC3_OCTL_PERIMODE);
> +
> +	dwc3_otg_fsm_sync(dwc);
> +	usb_otg_sync_inputs(dwc->fsm);
> +
> +	return 0;
> +
> +error:
> +	usb_otg_unregister(dwc->dev);
> +
> +	return ret;
>  }
>  
>  static void dwc3_drd_exit(struct dwc3 *dwc)
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index bd32cb2..129ef37 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -736,6 +736,7 @@ struct dwc3_scratchpad_array {
>   * @gadget_driver: pointer to the gadget driver
>   * @regs: base address for our registers
>   * @regs_size: address space size
> + * @oevten: otg interrupt enable mask copy
>   * @nr_scratch: number of scratch buffers
>   * @num_event_buffers: calculated number of event buffers
>   * @u1u2: only used on revisions <1.83a for workaround
> @@ -858,6 +859,8 @@ struct dwc3 {
>  	u32			dcfg;
>  	u32			gctl;
>  
> +	u32			oevten;
> +
>  	u32			nr_scratch;
>  	u32			num_event_buffers;
>  	u32			u1u2;
> -- 
> 2.1.4
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 5/9] usb: dwc3: core: make dual-role work with OTG irq

[Roger Quadros]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 02/09/15 17:43, Felipe Balbi wrote:
> Hi,
> 
> On Wed, Sep 02, 2015 at 05:24:20PM +0300, Roger Quadros wrote:
>> If the ID pin event is not available over extcon
>> then we rely on the OTG controller to provide us ID and VBUS
>> information.
>>
>> We still don't support any OTG features but just
>> dual-role operation.
>>
>> Signed-off-by: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
>> ---
>>  drivers/usb/dwc3/core.c | 217 ++++++++++++++++++++++++++++++++++++++++++++----
>>  drivers/usb/dwc3/core.h |   3 +
>>  2 files changed, 205 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 38b31df..632ee53 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -704,6 +704,63 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
>>  	return 0;
>>  }
>>  
>> +/* Get OTG events and sync it to OTG fsm */
>> +static void dwc3_otg_fsm_sync(struct dwc3 *dwc)
>> +{
>> +	u32 reg;
>> +	int id, vbus;
>> +
>> +	reg = dwc3_readl(dwc->regs, DWC3_OSTS);
>> +	dev_dbg(dwc->dev, "otgstatus 0x%x\n", reg);
>> +
>> +	id = !!(reg & DWC3_OSTS_CONIDSTS);
>> +	vbus = !!(reg & DWC3_OSTS_BSESVLD);
>> +
>> +	if (id != dwc->fsm->id || vbus != dwc->fsm->b_sess_vld) {
>> +		dev_dbg(dwc->dev, "id %d vbus %d\n", id, vbus);
>> +		dwc->fsm->id = id;
>> +		dwc->fsm->b_sess_vld = vbus;
>> +		usb_otg_sync_inputs(dwc->fsm);
>> +	}
> 
> this guy shouldn't try to filter events here. That's what the FSM should
> be doing.

OK. I'll remove the if condition.

> 
>> +}
>> +
>> +static irqreturn_t dwc3_otg_thread_irq(int irq, void *_dwc)
>> +{
>> +	struct dwc3 *dwc = _dwc;
>> +	unsigned long flags;
>> +	irqreturn_t ret = IRQ_NONE;
> 
> this IRQ will be disabled pretty quickly. You *always* return IRQ_NONE
> 
>> +	spin_lock_irqsave(&dwc->lock, flags);
> 
> if you cache current OSTS in dwc3, you can use that instead and change
> this to a spin_lock() instead of disabling IRQs here. This device's IRQs
> are already masked anyway.

OK.

> 
>> +	dwc3_otg_fsm_sync(dwc);
>> +	/* unmask interrupts */
>> +	dwc3_writel(dwc->regs, DWC3_OEVTEN, dwc->oevten);
>> +	spin_unlock_irqrestore(&dwc->lock, flags);
>> +
>> +	return ret;
>> +}
>> +
>> +static irqreturn_t dwc3_otg_irq(int irq, void *_dwc)
>> +{
>> +	struct dwc3 *dwc = _dwc;
>> +	irqreturn_t ret = IRQ_NONE;
>> +	u32 reg;
>> +
>> +	spin_lock(&dwc->lock);
> 
> this seems unnecessary, we're already in hardirq with IRQs disabled.
> What sort of race could we have ? (in fact, this also needs change in
> dwc3/gadget.c).

You're right. Will fix at both places.
> 
>> +
>> +	reg = dwc3_readl(dwc->regs, DWC3_OEVT);
>> +	if (reg) {
>> +		dwc3_writel(dwc->regs, DWC3_OEVT, reg);
>> +		/* mask interrupts till processed */
>> +		dwc->oevten = dwc3_readl(dwc->regs, DWC3_OEVTEN);
>> +		dwc3_writel(dwc->regs, DWC3_OEVTEN, 0);
>> +		ret = IRQ_WAKE_THREAD;
>> +	}
>> +
>> +	spin_unlock(&dwc->lock);
>> +
>> +	return ret;
>> +}
>> +
>>  /* --------------------- Dual-Role management ------------------------------- */
>>  
>>  static void dwc3_drd_fsm_sync(struct dwc3 *dwc)
>> @@ -728,15 +785,44 @@ static int dwc3_drd_start_host(struct otg_fsm *fsm, int on)
>>  {
>>  	struct device *dev = usb_otg_fsm_to_dev(fsm);
>>  	struct dwc3 *dwc = dev_get_drvdata(dev);
>> +	u32 reg;
>>  
>>  	dev_dbg(dwc->dev, "%s: %d\n", __func__, on);
>> +	if (dwc->edev) {
>> +		if (on) {
>> +			dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
>> +			/* start the HCD */
>> +			usb_otg_start_host(fsm, true);
>> +		} else {
>> +			/* stop the HCD */
>> +			usb_otg_start_host(fsm, false);
>> +		}
> 
> 		if (on)
> 			dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
> 		usb_otg_start_host(fsm, on);
> 

OK.

>> +
>> +		return 0;
>> +	}
>> +
>> +	/* switch OTG core */
>>  	if (on) {
>> -		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
>> +		/* OCTL.PeriMode = 0 */
>> +		reg = dwc3_readl(dwc->regs, DWC3_OCTL);
>> +		reg &= ~DWC3_OCTL_PERIMODE;
>> +		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
>> +		/* unconditionally turn on VBUS */
>> +		reg |= DWC3_OCTL_PRTPWRCTL;
>> +		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
>>  		/* start the HCD */
>>  		usb_otg_start_host(fsm, true);
>>  	} else {
>>  		/* stop the HCD */
>>  		usb_otg_start_host(fsm, false);
>> +		/* turn off VBUS */
>> +		reg = dwc3_readl(dwc->regs, DWC3_OCTL);
>> +		reg &= ~DWC3_OCTL_PRTPWRCTL;
>> +		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
>> +		/* OCTL.PeriMode = 1 */
>> +		reg = dwc3_readl(dwc->regs, DWC3_OCTL);
>> +		reg |= DWC3_OCTL_PERIMODE;
>> +		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
>>  	}
> 
> it looks like you're not really following the fluxchart from SNPS
> documentation, see Figure 11-4 on section 11.1.4.5

Did you mean that I'm ignoring all OTG bits (HNP/SRP/ADP)?

> 
>> @@ -746,15 +832,48 @@ static int dwc3_drd_start_gadget(struct otg_fsm *fsm, int on)
>>  {
>>  	struct device *dev = usb_otg_fsm_to_dev(fsm);
>>  	struct dwc3 *dwc = dev_get_drvdata(dev);
>> +	u32 reg;
>>  
>>  	dev_dbg(dwc->dev, "%s: %d\n", __func__, on);
>> -	if (on) {
>> -		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
>> +	if (on)
>>  		dwc3_event_buffers_setup(dwc);
>>  
>> +	if (dwc->edev) {
>> +		if (on) {
>> +			dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
>> +			usb_otg_start_gadget(fsm, true);
>> +		} else {
>> +			usb_otg_start_gadget(fsm, false);
>> +		}
>> +
>> +		return 0;
>> +	}
>> +
>> +	/* switch OTG core */
>> +	if (on) {
>> +		/* OCTL.PeriMode = 1 */
>> +		reg = dwc3_readl(dwc->regs, DWC3_OCTL);
>> +		reg |= DWC3_OCTL_PERIMODE;
>> +		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
>> +		/* GUSB2PHYCFG0.SusPHY = 1 */
>> +		if (!dwc->dis_u2_susphy_quirk) {
>> +			reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
>> +			reg |= DWC3_GUSB2PHYCFG_SUSPHY;
>> +			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
>> +		}
>>  		/* start the UDC */
>>  		usb_otg_start_gadget(fsm, true);
>>  	} else {
>> +		/* GUSB2PHYCFG0.SusPHY=0 */
>> +		if (!dwc->dis_u2_susphy_quirk) {
>> +			reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
>> +			reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
>> +			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
>> +		}
>> +		/* OCTL.PeriMode = 1 */
>> +		reg = dwc3_readl(dwc->regs, DWC3_OCTL);
>> +		reg |= DWC3_OCTL_PERIMODE;
>> +		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
>>  		/* stop the UDC */
>>  		usb_otg_start_gadget(fsm, false);
>>  	}
>> @@ -777,10 +896,30 @@ static int dwc3_drd_notifier(struct notifier_block *nb,
>>  	return NOTIFY_DONE;
>>  }
>>  

- --
cheers,
- -roger
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJV6FCCAAoJENJaa9O+djCTbaYQAME771phZpgr2Xtj1ejnPE8H
Bl84Sam/gWjy4+mqCUw+mQaCuF8M24ExVugHypQ0fF+9w6UMNrDrg+g+kZtQusCt
BTFkvS7g/6LJHEJowIoRZc5y/5bhnLa4Udcw5pYdhZHG7yIUsTs98WePROdOPk6z
i6OXA/wPC9ZJxeavew42HDmNj2IjJppU7bLDo+uMj/vz35dElq/B5w5mAXhshJ9A
R2IbxDevP4SiBYPfx1uFYKO5v9YVHnB3wk+3i3MjKuwO2CqfAVjzt9qWpM1iNThx
hOh+9vOenvttn7WHXP0scZAdBjmp3kKRAlfSELaAowy79X/3QRseZ75yJA8/tz+y
GT0x69fDQDxu0ffC961CY8p0a0F3ByVAqXBmsrCXPj0KxfutOkB8xE1BXY6+oUg/
ciqe0geXabmD9mu+3z8AXWOsFBnyFzsgSa2Dx5CRJ4/w5jhYOIg9/l8GGDlP6p1R
kHfiGYC2OzyxM4IgKYvc5p/VbAA4Ub5aQsWBdMYahAbs+l1xQ7zUEALf5S8c0KHK
k8jJo+oo+ghWzm6ikMfn96Ko/0vQuKG+uZZtzBDVp0uHBEW135GmZV5PCOh89M2k
yMuZQnbcTpEaANvWYJDkH3Can6Afcuki/i9kOK8bDib4Exo3IBijZNsLzpUzeS0m
vnsEqLL9IDRJR54ibQOC
=5n96
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 5/9] usb: dwc3: core: make dual-role work with OTG irq

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1549 bytes --]

Hi,

On Thu, Sep 03, 2015 at 04:52:02PM +0300, Roger Quadros wrote:
> >>  	if (on) {
> >> -		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
> >> +		/* OCTL.PeriMode = 0 */
> >> +		reg = dwc3_readl(dwc->regs, DWC3_OCTL);
> >> +		reg &= ~DWC3_OCTL_PERIMODE;
> >> +		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
> >> +		/* unconditionally turn on VBUS */
> >> +		reg |= DWC3_OCTL_PRTPWRCTL;
> >> +		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
> >>  		/* start the HCD */
> >>  		usb_otg_start_host(fsm, true);
> >>  	} else {
> >>  		/* stop the HCD */
> >>  		usb_otg_start_host(fsm, false);
> >> +		/* turn off VBUS */
> >> +		reg = dwc3_readl(dwc->regs, DWC3_OCTL);
> >> +		reg &= ~DWC3_OCTL_PRTPWRCTL;
> >> +		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
> >> +		/* OCTL.PeriMode = 1 */
> >> +		reg = dwc3_readl(dwc->regs, DWC3_OCTL);
> >> +		reg |= DWC3_OCTL_PERIMODE;
> >> +		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
> >>  	}
> > 
> > it looks like you're not really following the fluxchart from SNPS
> > documentation, see Figure 11-4 on section 11.1.4.5
> 
> Did you mean that I'm ignoring all OTG bits (HNP/SRP/ADP)?

yes and no :-)  There's a rather complex flux chart which details how we
switch from host to peripheral and vice versa. We need to follow that to
the smallest details since that's what IP provider considers to be
correct. If we deviate from that we should have very strong reasons for
doing so and we also want big, fat, long comments in source code
detailing why and how we're deviating :-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 5/9] usb: dwc3: core: make dual-role work with OTG irq

[Roger Quadros]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 03/09/15 18:51, Felipe Balbi wrote:
> Hi,
> 
> On Thu, Sep 03, 2015 at 04:52:02PM +0300, Roger Quadros wrote:
>>>>  	if (on) {
>>>> -		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
>>>> +		/* OCTL.PeriMode = 0 */
>>>> +		reg = dwc3_readl(dwc->regs, DWC3_OCTL);
>>>> +		reg &= ~DWC3_OCTL_PERIMODE;
>>>> +		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
>>>> +		/* unconditionally turn on VBUS */
>>>> +		reg |= DWC3_OCTL_PRTPWRCTL;
>>>> +		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
>>>>  		/* start the HCD */
>>>>  		usb_otg_start_host(fsm, true);
>>>>  	} else {
>>>>  		/* stop the HCD */
>>>>  		usb_otg_start_host(fsm, false);
>>>> +		/* turn off VBUS */
>>>> +		reg = dwc3_readl(dwc->regs, DWC3_OCTL);
>>>> +		reg &= ~DWC3_OCTL_PRTPWRCTL;
>>>> +		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
>>>> +		/* OCTL.PeriMode = 1 */
>>>> +		reg = dwc3_readl(dwc->regs, DWC3_OCTL);
>>>> +		reg |= DWC3_OCTL_PERIMODE;
>>>> +		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
>>>>  	}
>>>
>>> it looks like you're not really following the fluxchart from SNPS
>>> documentation, see Figure 11-4 on section 11.1.4.5
>>
>> Did you mean that I'm ignoring all OTG bits (HNP/SRP/ADP)?
> 
> yes and no :-)  There's a rather complex flux chart which details how we
> switch from host to peripheral and vice versa. We need to follow that to
> the smallest details since that's what IP provider considers to be
> correct. If we deviate from that we should have very strong reasons for
> doing so and we also want big, fat, long comments in source code
> detailing why and how we're deviating :-)
> 
Understood. I'll update this accordingly.

- --
cheers,
- -roger
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJV6WC4AAoJENJaa9O+djCTWSEP/jq0jX2KJVjqKeSk60wyDIEH
AIGXpaupz4XCFRsLFHlEomAoWHEzmbdwQlcI5gyB4N6IgF6xfxX71AZnytw4k4ja
sFxQq/qNSPWa4yiEY9MFbIPuiRMFcLOB5VbemGpcZq4n31yaKlbLtHMDEC8zPmdY
yfnUz7raY2tID/+wOASAM+nDXVS6nFztWXCrz+4TxwDaH0dgwl9OAoD7nvTil3MT
UTuR34GbWcb9rCxJVYgneht1sLbkKGCkwjLRcqzhOajg87GuW38irqD9OOF+pLCm
srhYsntQyBJlvArDQKMZIyGEYHvjICWzJUo/IPA8DFS/qOrRLyrLvVHskb6pr9/y
KZL/1N+5Nbh0N0XGWKwxzVh69DqmQPuY32SN2r1xqCDzzZAnvrl9cz6ixsRq1JGK
9EvYNJlRWsdypfA8tdhPtCgXua+wYSLNewgHQSko10qeAgiaIU5/bQ/9Eb2Ys9QV
6wkhr0Gulf/hnMKXHupErhUsvSBJk2MGaaDIfUSt3mWMTRsthp2NedRE9li9bWW9
gSXcTxs8785Y7FgxFDvvqQ05rbGWNqfxkeq54ziMtH3mZ93inE7me1HbqL1jgEuh
NFrNsAKYc000syvnqunzl8uZMRMsICaY1aKC0riQTdpgagHgGMfadKtWXb8OdcVa
gICj2/o8MpPpv3FEniDz
=L39b
-----END PGP SIGNATURE-----

Re: [PATCH v4 5/9] usb: dwc3: core: make dual-role work with OTG irq

[Peter Chen]

On Wed, Sep 02, 2015 at 09:43:38AM -0500, Felipe Balbi wrote:
> Hi,
> 
> > +
> > +static irqreturn_t dwc3_otg_irq(int irq, void *_dwc)
> > +{
> > +	struct dwc3 *dwc = _dwc;
> > +	irqreturn_t ret = IRQ_NONE;
> > +	u32 reg;
> > +
> > +	spin_lock(&dwc->lock);
> 
> this seems unnecessary, we're already in hardirq with IRQs disabled.
> What sort of race could we have ? (in fact, this also needs change in
> dwc3/gadget.c).
> 

Is it possible the kernel process is accessing the content you will 
access?

-- 

Best Regards,
Peter Chen

Re: [PATCH v4 5/9] usb: dwc3: core: make dual-role work with OTG irq

[Roger Quadros]

On 06/09/15 05:20, Peter Chen wrote:
> On Wed, Sep 02, 2015 at 09:43:38AM -0500, Felipe Balbi wrote:
>> Hi,
>>
>>> +
>>> +static irqreturn_t dwc3_otg_irq(int irq, void *_dwc)
>>> +{
>>> +	struct dwc3 *dwc = _dwc;
>>> +	irqreturn_t ret = IRQ_NONE;
>>> +	u32 reg;
>>> +
>>> +	spin_lock(&dwc->lock);
>>
>> this seems unnecessary, we're already in hardirq with IRQs disabled.
>> What sort of race could we have ? (in fact, this also needs change in
>> dwc3/gadget.c).
>>
> 
> Is it possible the kernel process is accessing the content you will 
> access?
> 
When kernel process accesses the data we'll never reach here
as we're protecting it with spinlock_irqsave(), spinunlock_irqrestore().

cheers,
-roger

[PATCH v4 9/9] usb: dwc3: core: don't break during suspend/resume while we're dual-role

[Roger Quadros]

We can't rely just on dr_mode to decide if we're in host or gadget
mode when we're configured as otg/dual-role. So while dr_mode is
OTG, we find out from  the otg state machine if we're in host
or gadget mode and take the necessary actions during suspend/resume.

Also make sure that we disable OTG irq and events during system suspend
so that we don't lockup the system during system suspend/resume.

Signed-off-by: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
---
 drivers/usb/dwc3/core.c | 27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 654aebf..25891e3 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1455,18 +1455,15 @@ static int dwc3_suspend(struct device *dev)
 		dwc->octl = dwc3_readl(dwc->regs, DWC3_OCTL);
 		dwc->oevt = dwc3_readl(dwc->regs, DWC3_OEVT);
 		dwc->oevten = dwc3_readl(dwc->regs, DWC3_OEVTEN);
+		dwc3_writel(dwc->regs, DWC3_OEVTEN, 0);
+		disable_irq(dwc->otg_irq);
 	}
 
-	switch (dwc->dr_mode) {
-	case USB_DR_MODE_PERIPHERAL:
-	case USB_DR_MODE_OTG:
+	if (dwc->dr_mode == USB_DR_MODE_PERIPHERAL ||
+	    ((dwc->dr_mode == USB_DR_MODE_OTG) && (dwc->fsm->protocol == PROTO_GADGET)))
 		dwc3_gadget_suspend(dwc);
-		/* FALLTHROUGH */
-	case USB_DR_MODE_HOST:
-	default:
-		dwc3_event_buffers_cleanup(dwc);
-		break;
-	}
+
+	dwc3_event_buffers_cleanup(dwc);
 
 	dwc->gctl = dwc3_readl(dwc->regs, DWC3_GCTL);
 	spin_unlock_irqrestore(&dwc->lock, flags);
@@ -1506,18 +1503,12 @@ static int dwc3_resume(struct device *dev)
 		dwc3_writel(dwc->regs, DWC3_OCTL, dwc->octl);
 		dwc3_writel(dwc->regs, DWC3_OEVT, dwc->oevt);
 		dwc3_writel(dwc->regs, DWC3_OEVTEN, dwc->oevten);
+		enable_irq(dwc->otg_irq);
 	}
 
-	switch (dwc->dr_mode) {
-	case USB_DR_MODE_PERIPHERAL:
-	case USB_DR_MODE_OTG:
+	if (dwc->dr_mode == USB_DR_MODE_PERIPHERAL ||
+	    ((dwc->dr_mode == USB_DR_MODE_OTG) && (dwc->fsm->protocol == PROTO_GADGET)))
 		dwc3_gadget_resume(dwc);
-		/* FALLTHROUGH */
-	case USB_DR_MODE_HOST:
-	default:
-		/* do nothing */
-		break;
-	}
 
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 9/9] usb: dwc3: core: don't break during suspend/resume while we're dual-role

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2527 bytes --]

On Wed, Sep 02, 2015 at 05:24:24PM +0300, Roger Quadros wrote:
> We can't rely just on dr_mode to decide if we're in host or gadget
> mode when we're configured as otg/dual-role. So while dr_mode is
> OTG, we find out from  the otg state machine if we're in host
> or gadget mode and take the necessary actions during suspend/resume.
> 
> Also make sure that we disable OTG irq and events during system suspend
> so that we don't lockup the system during system suspend/resume.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  drivers/usb/dwc3/core.c | 27 +++++++++------------------
>  1 file changed, 9 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 654aebf..25891e3 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1455,18 +1455,15 @@ static int dwc3_suspend(struct device *dev)
>  		dwc->octl = dwc3_readl(dwc->regs, DWC3_OCTL);
>  		dwc->oevt = dwc3_readl(dwc->regs, DWC3_OEVT);
>  		dwc->oevten = dwc3_readl(dwc->regs, DWC3_OEVTEN);
> +		dwc3_writel(dwc->regs, DWC3_OEVTEN, 0);
> +		disable_irq(dwc->otg_irq);

you don't need disable_irq() here. In fact, it causes problems since
you're calling it with IRQs disabled.

>  	}
>  
> -	switch (dwc->dr_mode) {
> -	case USB_DR_MODE_PERIPHERAL:
> -	case USB_DR_MODE_OTG:
> +	if (dwc->dr_mode == USB_DR_MODE_PERIPHERAL ||
> +	    ((dwc->dr_mode == USB_DR_MODE_OTG) && (dwc->fsm->protocol == PROTO_GADGET)))
>  		dwc3_gadget_suspend(dwc);
> -		/* FALLTHROUGH */
> -	case USB_DR_MODE_HOST:
> -	default:
> -		dwc3_event_buffers_cleanup(dwc);
> -		break;
> -	}
> +
> +	dwc3_event_buffers_cleanup(dwc);
>  
>  	dwc->gctl = dwc3_readl(dwc->regs, DWC3_GCTL);
>  	spin_unlock_irqrestore(&dwc->lock, flags);
> @@ -1506,18 +1503,12 @@ static int dwc3_resume(struct device *dev)
>  		dwc3_writel(dwc->regs, DWC3_OCTL, dwc->octl);
>  		dwc3_writel(dwc->regs, DWC3_OEVT, dwc->oevt);
>  		dwc3_writel(dwc->regs, DWC3_OEVTEN, dwc->oevten);
> +		enable_irq(dwc->otg_irq);
>  	}
>  
> -	switch (dwc->dr_mode) {
> -	case USB_DR_MODE_PERIPHERAL:
> -	case USB_DR_MODE_OTG:
> +	if (dwc->dr_mode == USB_DR_MODE_PERIPHERAL ||
> +	    ((dwc->dr_mode == USB_DR_MODE_OTG) && (dwc->fsm->protocol == PROTO_GADGET)))
>  		dwc3_gadget_resume(dwc);
> -		/* FALLTHROUGH */
> -	case USB_DR_MODE_HOST:
> -	default:
> -		/* do nothing */
> -		break;
> -	}
>  
>  	spin_unlock_irqrestore(&dwc->lock, flags);
>  
> -- 
> 2.1.4
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 9/9] usb: dwc3: core: don't break during suspend/resume while we're dual-role

[Roger Quadros]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 02/09/15 17:48, Felipe Balbi wrote:
> On Wed, Sep 02, 2015 at 05:24:24PM +0300, Roger Quadros wrote:
>> We can't rely just on dr_mode to decide if we're in host or gadget
>> mode when we're configured as otg/dual-role. So while dr_mode is
>> OTG, we find out from  the otg state machine if we're in host
>> or gadget mode and take the necessary actions during suspend/resume.
>>
>> Also make sure that we disable OTG irq and events during system suspend
>> so that we don't lockup the system during system suspend/resume.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>  drivers/usb/dwc3/core.c | 27 +++++++++------------------
>>  1 file changed, 9 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 654aebf..25891e3 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -1455,18 +1455,15 @@ static int dwc3_suspend(struct device *dev)
>>  		dwc->octl = dwc3_readl(dwc->regs, DWC3_OCTL);
>>  		dwc->oevt = dwc3_readl(dwc->regs, DWC3_OEVT);
>>  		dwc->oevten = dwc3_readl(dwc->regs, DWC3_OEVTEN);
>> +		dwc3_writel(dwc->regs, DWC3_OEVTEN, 0);
>> +		disable_irq(dwc->otg_irq);
> 
> you don't need disable_irq() here. In fact, it causes problems since
> you're calling it with IRQs disabled.

OK. will remove it.

> 
>>  	}
>>  
>> -	switch (dwc->dr_mode) {
>> -	case USB_DR_MODE_PERIPHERAL:
>> -	case USB_DR_MODE_OTG:
>> +	if (dwc->dr_mode == USB_DR_MODE_PERIPHERAL ||
>> +	    ((dwc->dr_mode == USB_DR_MODE_OTG) && (dwc->fsm->protocol == PROTO_GADGET)))
>>  		dwc3_gadget_suspend(dwc);
>> -		/* FALLTHROUGH */
>> -	case USB_DR_MODE_HOST:
>> -	default:
>> -		dwc3_event_buffers_cleanup(dwc);
>> -		break;
>> -	}
>> +
>> +	dwc3_event_buffers_cleanup(dwc);
>>  
>>  	dwc->gctl = dwc3_readl(dwc->regs, DWC3_GCTL);
>>  	spin_unlock_irqrestore(&dwc->lock, flags);
>> @@ -1506,18 +1503,12 @@ static int dwc3_resume(struct device *dev)
>>  		dwc3_writel(dwc->regs, DWC3_OCTL, dwc->octl);
>>  		dwc3_writel(dwc->regs, DWC3_OEVT, dwc->oevt);
>>  		dwc3_writel(dwc->regs, DWC3_OEVTEN, dwc->oevten);
>> +		enable_irq(dwc->otg_irq);
>>  	}
>>  
>> -	switch (dwc->dr_mode) {
>> -	case USB_DR_MODE_PERIPHERAL:
>> -	case USB_DR_MODE_OTG:
>> +	if (dwc->dr_mode == USB_DR_MODE_PERIPHERAL ||
>> +	    ((dwc->dr_mode == USB_DR_MODE_OTG) && (dwc->fsm->protocol == PROTO_GADGET)))
>>  		dwc3_gadget_resume(dwc);
>> -		/* FALLTHROUGH */
>> -	case USB_DR_MODE_HOST:
>> -	default:
>> -		/* do nothing */
>> -		break;
>> -	}
>>  
>>  	spin_unlock_irqrestore(&dwc->lock, flags);
>>  
>> -- 
>> 2.1.4
>>
> 

- --
cheers,
- -roger
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJV6FLtAAoJENJaa9O+djCTxnMQANGOTEBjO8E9v32qfrgwL6X+
VyTeGBHhfwv7/u7xOM0im6tfXXaLtjj16mEY8fePC+oXO2Vv3RDxi0HTiF6sePnT
odt8nTOYGq7arPAKrtL8BK/8xyJRDjijXWGdQDAgPT1D5V2y8Ib4AbkM2j5IkL+/
75EGybhxIPNGNEV2eS/OBn2BPWSzn/r6rFMJiuYPY2yoPQqwG1ovdt+K+tvMybvZ
sYdQxu4UPN1z1pKplmCQxmPut9SyCqIAeHXdWGT6kJsleBsv2WNM1ZdM/y4zZmMw
gpyNOs+HYqdd/llfEYFrSRnSM2io2GyKZ73xM+DVfY8ot7Vf/h4x41Dz4V19jqtC
3IoyPzNb+inbsRKs0GhTw3yD9N8b7xRbVF+qZPIvDvn7QGLF2pY8cDiqZE1LiOs2
VpKBRoC0wdmdQ/bjRMt516jFmJiQ2RbR2SenGffIr+PrCb1EQ7Fmwtoya5FPgIz/
nMdL7g1MGHUzWvzXTYO+RH4bHV9cDL66qekOL7PR1yEyAAo09vLc+ds/4z85MGQ6
2JYKYb4Mdtv2EAOzIqNR3+FzcCawpZbhm8bBFz14Kj12pgjwl5A3o+fYaZ4Q07OK
ZfFRAUG4RKFNVcjb7hhiBPdN+mb34/50YreFcNj/MOYwklXwUAmgiTFH7cjO9SA7
XZpbfSRwT4Ec4WWqTeeR
=unNn
-----END PGP SIGNATURE-----

Re: [PATCH v4 9/9] usb: dwc3: core: don't break during suspend/resume while we're dual-role

[Sergei Shtylyov]

Hello.

On 09/02/2015 05:24 PM, Roger Quadros wrote:

> We can't rely just on dr_mode to decide if we're in host or gadget
> mode when we're configured as otg/dual-role. So while dr_mode is
> OTG, we find out from  the otg state machine if we're in host
> or gadget mode and take the necessary actions during suspend/resume.
>
> Also make sure that we disable OTG irq and events during system suspend
> so that we don't lockup the system during system suspend/resume.
>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>   drivers/usb/dwc3/core.c | 27 +++++++++------------------
>   1 file changed, 9 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 654aebf..25891e3 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1455,18 +1455,15 @@ static int dwc3_suspend(struct device *dev)
>   		dwc->octl = dwc3_readl(dwc->regs, DWC3_OCTL);
>   		dwc->oevt = dwc3_readl(dwc->regs, DWC3_OEVT);
>   		dwc->oevten = dwc3_readl(dwc->regs, DWC3_OEVTEN);
> +		dwc3_writel(dwc->regs, DWC3_OEVTEN, 0);
> +		disable_irq(dwc->otg_irq);
>   	}
>
> -	switch (dwc->dr_mode) {
> -	case USB_DR_MODE_PERIPHERAL:
> -	case USB_DR_MODE_OTG:
> +	if (dwc->dr_mode == USB_DR_MODE_PERIPHERAL ||
> +	    ((dwc->dr_mode == USB_DR_MODE_OTG) && (dwc->fsm->protocol == PROTO_GADGET)))

    Hm, you're not very consistent about your parens. :-)

[...]
> @@ -1506,18 +1503,12 @@ static int dwc3_resume(struct device *dev)
>   		dwc3_writel(dwc->regs, DWC3_OCTL, dwc->octl);
>   		dwc3_writel(dwc->regs, DWC3_OEVT, dwc->oevt);
>   		dwc3_writel(dwc->regs, DWC3_OEVTEN, dwc->oevten);
> +		enable_irq(dwc->otg_irq);
>   	}
>
> -	switch (dwc->dr_mode) {
> -	case USB_DR_MODE_PERIPHERAL:
> -	case USB_DR_MODE_OTG:
> +	if (dwc->dr_mode == USB_DR_MODE_PERIPHERAL ||
> +	    ((dwc->dr_mode == USB_DR_MODE_OTG) && (dwc->fsm->protocol == PROTO_GADGET)))

    Same here...

[...]

WBR, Sergei

Re: [PATCH v4 9/9] usb: dwc3: core: don't break during suspend/resume while we're dual-role

[Roger Quadros]

On 02/09/15 20:22, Sergei Shtylyov wrote:
> Hello.
> 
> On 09/02/2015 05:24 PM, Roger Quadros wrote:
> 
>> We can't rely just on dr_mode to decide if we're in host or gadget
>> mode when we're configured as otg/dual-role. So while dr_mode is
>> OTG, we find out from  the otg state machine if we're in host
>> or gadget mode and take the necessary actions during suspend/resume.
>>
>> Also make sure that we disable OTG irq and events during system suspend
>> so that we don't lockup the system during system suspend/resume.
>>
>> Signed-off-by: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
>> ---
>>   drivers/usb/dwc3/core.c | 27 +++++++++------------------
>>   1 file changed, 9 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 654aebf..25891e3 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -1455,18 +1455,15 @@ static int dwc3_suspend(struct device *dev)
>>           dwc->octl = dwc3_readl(dwc->regs, DWC3_OCTL);
>>           dwc->oevt = dwc3_readl(dwc->regs, DWC3_OEVT);
>>           dwc->oevten = dwc3_readl(dwc->regs, DWC3_OEVTEN);
>> +        dwc3_writel(dwc->regs, DWC3_OEVTEN, 0);
>> +        disable_irq(dwc->otg_irq);
>>       }
>>
>> -    switch (dwc->dr_mode) {
>> -    case USB_DR_MODE_PERIPHERAL:
>> -    case USB_DR_MODE_OTG:
>> +    if (dwc->dr_mode == USB_DR_MODE_PERIPHERAL ||
>> +        ((dwc->dr_mode == USB_DR_MODE_OTG) && (dwc->fsm->protocol == PROTO_GADGET)))
> 
>    Hm, you're not very consistent about your parens. :-)

You're right. Should be

if ((dwc->dr_mode == USB_DR_MODE_PERIPHERAL) ||
    ((dwc->dr_mode == USB_DR_MODE_OTG) && (dwc->fsm->protocol == PROTO_GADGET)))


> 
> [...]
>> @@ -1506,18 +1503,12 @@ static int dwc3_resume(struct device *dev)
>>           dwc3_writel(dwc->regs, DWC3_OCTL, dwc->octl);
>>           dwc3_writel(dwc->regs, DWC3_OEVT, dwc->oevt);
>>           dwc3_writel(dwc->regs, DWC3_OEVTEN, dwc->oevten);
>> +        enable_irq(dwc->otg_irq);
>>       }
>>
>> -    switch (dwc->dr_mode) {
>> -    case USB_DR_MODE_PERIPHERAL:
>> -    case USB_DR_MODE_OTG:
>> +    if (dwc->dr_mode == USB_DR_MODE_PERIPHERAL ||
>> +        ((dwc->dr_mode == USB_DR_MODE_OTG) && (dwc->fsm->protocol == PROTO_GADGET)))
> 
>    Same here...

--
cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 9/9] usb: dwc3: core: don't break during suspend/resume while we're dual-role

[Sergei Shtylyov]

Hello.

On 09/03/2015 05:01 PM, Roger Quadros wrote:

>>> We can't rely just on dr_mode to decide if we're in host or gadget
>>> mode when we're configured as otg/dual-role. So while dr_mode is
>>> OTG, we find out from  the otg state machine if we're in host
>>> or gadget mode and take the necessary actions during suspend/resume.
>>>
>>> Also make sure that we disable OTG irq and events during system suspend
>>> so that we don't lockup the system during system suspend/resume.
>>>
>>> Signed-off-by: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
>>> ---
>>>    drivers/usb/dwc3/core.c | 27 +++++++++------------------
>>>    1 file changed, 9 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index 654aebf..25891e3 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -1455,18 +1455,15 @@ static int dwc3_suspend(struct device *dev)
>>>            dwc->octl = dwc3_readl(dwc->regs, DWC3_OCTL);
>>>            dwc->oevt = dwc3_readl(dwc->regs, DWC3_OEVT);
>>>            dwc->oevten = dwc3_readl(dwc->regs, DWC3_OEVTEN);
>>> +        dwc3_writel(dwc->regs, DWC3_OEVTEN, 0);
>>> +        disable_irq(dwc->otg_irq);
>>>        }
>>>
>>> -    switch (dwc->dr_mode) {
>>> -    case USB_DR_MODE_PERIPHERAL:
>>> -    case USB_DR_MODE_OTG:
>>> +    if (dwc->dr_mode == USB_DR_MODE_PERIPHERAL ||
>>> +        ((dwc->dr_mode == USB_DR_MODE_OTG) && (dwc->fsm->protocol == PROTO_GADGET)))
>>
>>     Hm, you're not very consistent about your parens. :-)
>
> You're right. Should be
>
> if ((dwc->dr_mode == USB_DR_MODE_PERIPHERAL) ||
>      ((dwc->dr_mode == USB_DR_MODE_OTG) && (dwc->fsm->protocol == PROTO_GADGET)))

    Parens around == are useless, I'd prefer if you deleted them. But if you'd 
like to keep them, let it be so. :-)

> --
> cheers,
> -roger

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 9/9] usb: dwc3: core: don't break during suspend/resume while we're dual-role

[Roger Quadros]

On 03/09/15 17:05, Sergei Shtylyov wrote:
> Hello.
> 
> On 09/03/2015 05:01 PM, Roger Quadros wrote:
> 
>>>> We can't rely just on dr_mode to decide if we're in host or gadget
>>>> mode when we're configured as otg/dual-role. So while dr_mode is
>>>> OTG, we find out from  the otg state machine if we're in host
>>>> or gadget mode and take the necessary actions during suspend/resume.
>>>>
>>>> Also make sure that we disable OTG irq and events during system suspend
>>>> so that we don't lockup the system during system suspend/resume.
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>> ---
>>>>    drivers/usb/dwc3/core.c | 27 +++++++++------------------
>>>>    1 file changed, 9 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index 654aebf..25891e3 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>> @@ -1455,18 +1455,15 @@ static int dwc3_suspend(struct device *dev)
>>>>            dwc->octl = dwc3_readl(dwc->regs, DWC3_OCTL);
>>>>            dwc->oevt = dwc3_readl(dwc->regs, DWC3_OEVT);
>>>>            dwc->oevten = dwc3_readl(dwc->regs, DWC3_OEVTEN);
>>>> +        dwc3_writel(dwc->regs, DWC3_OEVTEN, 0);
>>>> +        disable_irq(dwc->otg_irq);
>>>>        }
>>>>
>>>> -    switch (dwc->dr_mode) {
>>>> -    case USB_DR_MODE_PERIPHERAL:
>>>> -    case USB_DR_MODE_OTG:
>>>> +    if (dwc->dr_mode == USB_DR_MODE_PERIPHERAL ||
>>>> +        ((dwc->dr_mode == USB_DR_MODE_OTG) && (dwc->fsm->protocol == PROTO_GADGET)))
>>>
>>>     Hm, you're not very consistent about your parens. :-)
>>
>> You're right. Should be
>>
>> if ((dwc->dr_mode == USB_DR_MODE_PERIPHERAL) ||
>>      ((dwc->dr_mode == USB_DR_MODE_OTG) && (dwc->fsm->protocol == PROTO_GADGET)))
> 
>    Parens around == are useless, I'd prefer if you deleted them. But if you'd like to keep them, let it be so. :-)

OK, how about this then?

if (dwc->dr_mode == USB_DR_MODE_PERIPHERAL ||
    (dwc->dr_mode == USB_DR_MODE_OTG && dwc->fsm->protocol == PROTO_GADGET))

cheers,
-roger

Re: [PATCH v4 9/9] usb: dwc3: core: don't break during suspend/resume while we're dual-role

[Sergei Shtylyov]

On 09/03/2015 05:10 PM, Roger Quadros wrote:

>>>>> We can't rely just on dr_mode to decide if we're in host or gadget
>>>>> mode when we're configured as otg/dual-role. So while dr_mode is
>>>>> OTG, we find out from  the otg state machine if we're in host
>>>>> or gadget mode and take the necessary actions during suspend/resume.
>>>>>
>>>>> Also make sure that we disable OTG irq and events during system suspend
>>>>> so that we don't lockup the system during system suspend/resume.
>>>>>
>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>> ---
>>>>>     drivers/usb/dwc3/core.c | 27 +++++++++------------------
>>>>>     1 file changed, 9 insertions(+), 18 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>> index 654aebf..25891e3 100644
>>>>> --- a/drivers/usb/dwc3/core.c
>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>> @@ -1455,18 +1455,15 @@ static int dwc3_suspend(struct device *dev)
>>>>>             dwc->octl = dwc3_readl(dwc->regs, DWC3_OCTL);
>>>>>             dwc->oevt = dwc3_readl(dwc->regs, DWC3_OEVT);
>>>>>             dwc->oevten = dwc3_readl(dwc->regs, DWC3_OEVTEN);
>>>>> +        dwc3_writel(dwc->regs, DWC3_OEVTEN, 0);
>>>>> +        disable_irq(dwc->otg_irq);
>>>>>         }
>>>>>
>>>>> -    switch (dwc->dr_mode) {
>>>>> -    case USB_DR_MODE_PERIPHERAL:
>>>>> -    case USB_DR_MODE_OTG:
>>>>> +    if (dwc->dr_mode == USB_DR_MODE_PERIPHERAL ||
>>>>> +        ((dwc->dr_mode == USB_DR_MODE_OTG) && (dwc->fsm->protocol == PROTO_GADGET)))
>>>>
>>>>      Hm, you're not very consistent about your parens. :-)
>>>
>>> You're right. Should be
>>>
>>> if ((dwc->dr_mode == USB_DR_MODE_PERIPHERAL) ||
>>>       ((dwc->dr_mode == USB_DR_MODE_OTG) && (dwc->fsm->protocol == PROTO_GADGET)))
>>
>>     Parens around == are useless, I'd prefer if you deleted them. But if you'd like to keep them, let it be so. :-)

> OK, how about this then?

> if (dwc->dr_mode == USB_DR_MODE_PERIPHERAL ||
>      (dwc->dr_mode == USB_DR_MODE_OTG && dwc->fsm->protocol == PROTO_GADGET))

    Strictly speaking, parens around && are also not needed but gcc may 
probably issue a warning without them. Not sure, your call.

> cheers,
> -roger

MBR, Sergei

[PATCH v4 2/9] usb: dwc3: core.h: add some register definitions

[Roger Quadros]

Add OTG and GHWPARAMS6 register definitions

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/dwc3/core.h | 82 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 5ca2b25..4b85330 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -173,6 +173,15 @@
 #define DWC3_GCTL_GBLHIBERNATIONEN	(1 << 1)
 #define DWC3_GCTL_DSBLCLKGTNG		(1 << 0)
 
+/* Global Status Register */
+#define DWC3_GSTS_OTG_IP	(1 << 10)
+#define DWC3_GSTS_BC_IP		(1 << 9)
+#define DWC3_GSTS_ADP_IP	(1 << 8)
+#define DWC3_GSTS_HOST_IP	(1 << 7)
+#define DWC3_GSTS_DEVICE_IP	(1 << 6)
+#define DWC3_GSTS_CSR_TIMEOUT	(1 << 5)
+#define DWC3_GSTS_BUS_ERR_ADDR_VLD	(1 << 4)
+
 /* Global USB2 PHY Configuration Register */
 #define DWC3_GUSB2PHYCFG_PHYSOFTRST	(1 << 31)
 #define DWC3_GUSB2PHYCFG_SUSPHY		(1 << 6)
@@ -234,6 +243,11 @@
 #define DWC3_MAX_HIBER_SCRATCHBUFS		15
 
 /* Global HWPARAMS6 Register */
+#define DWC3_GHWPARAMS6_BCSUPPORT		(1 << 14)
+#define DWC3_GHWPARAMS6_OTG3SUPPORT		(1 << 13)
+#define DWC3_GHWPARAMS6_ADPSUPPORT		(1 << 12)
+#define DWC3_GHWPARAMS6_HNPSUPPORT		(1 << 11)
+#define DWC3_GHWPARAMS6_SRPSUPPORT		(1 << 10)
 #define DWC3_GHWPARAMS6_EN_FPGA			(1 << 7)
 
 /* Device Configuration Register */
@@ -393,6 +407,74 @@
 #define DWC3_DEPCMD_TYPE_BULK		2
 #define DWC3_DEPCMD_TYPE_INTR		3
 
+/* OTG Configuration Register */
+#define DWC3_OCFG_DISPWRCUTTOFF		(1 << 5)
+#define DWC3_OCFG_HIBDISMASK		(1 << 4)
+#define DWC3_OCFG_SFTRSTMASK		(1 << 3)
+#define DWC3_OCFG_OTGVERSION		(1 << 2)
+#define DWC3_OCFG_HNPCAP		(1 << 1)
+#define DWC3_OCFG_SRPCAP		(1 << 0)
+
+/* OTG CTL Register */
+#define DWC3_OCTL_OTG3GOERR		(1 << 7)
+#define DWC3_OCTL_PERIMODE		(1 << 6)
+#define DWC3_OCTL_PRTPWRCTL		(1 << 5)
+#define DWC3_OCTL_HNPREQ		(1 << 4)
+#define DWC3_OCTL_SESREQ		(1 << 3)
+#define DWC3_OCTL_TERMSELIDPULSE	(1 << 2)
+#define DWC3_OCTL_DEVSETHNPEN		(1 << 1)
+#define DWC3_OCTL_HSTSETHNPEN		(1 << 0)
+
+/* OTG Event Register */
+#define DWC3_OEVT_DEVICEMODE		(1 << 31)
+#define DWC3_OEVT_XHCIRUNSTPSET		(1 << 27)
+#define DWC3_OEVT_DEVRUNSTPSET		(1 << 26)
+#define DWC3_OEVT_HIBENTRY		(1 << 25)
+#define DWC3_OEVT_CONIDSTSCHNG		(1 << 24)
+#define DWC3_OEVT_HRRCONFNOTIF		(1 << 23)
+#define DWC3_OEVT_HRRINITNOTIF		(1 << 22)
+#define DWC3_OEVT_ADEVIDLE		(1 << 21)
+#define DWC3_OEVT_ADEVBHOSTEND		(1 << 20)
+#define DWC3_OEVT_ADEVHOST		(1 << 19)
+#define DWC3_OEVT_ADEVHNPCHNG		(1 << 18)
+#define DWC3_OEVT_ADEVSRPDET		(1 << 17)
+#define DWC3_OEVT_ADEVSESSENDDET	(1 << 16)
+#define DWC3_OEVT_BDEVBHOSTEND		(1 << 11)
+#define DWC3_OEVT_BDEVHNPCHNG		(1 << 10)
+#define DWC3_OEVT_BDEVSESSVLDDET	(1 << 9)
+#define DWC3_OEVT_BDEVVBUSCHNG		(1 << 8)
+#define DWC3_OEVT_BSESSVLD		(1 << 3)
+#define DWC3_OEVT_HSTNEGSTS		(1 << 2)
+#define DWC3_OEVT_SESREQSTS		(1 << 1)
+#define DWC3_OEVT_ERROR			(1 << 0)
+
+/* OTG Event Enable Register */
+#define DWC3_OEVTEN_XHCIRUNSTPSETEN	(1 << 27)
+#define DWC3_OEVTEN_DEVRUNSTPSETEN	(1 << 26)
+#define DWC3_OEVTEN_HIBENTRYEN		(1 << 25)
+#define DWC3_OEVTEN_CONIDSTSCHNGEN	(1 << 24)
+#define DWC3_OEVTEN_HRRCONFNOTIFEN	(1 << 23)
+#define DWC3_OEVTEN_HRRINITNOTIFEN	(1 << 22)
+#define DWC3_OEVTEN_ADEVIDLEEN		(1 << 21)
+#define DWC3_OEVTEN_ADEVBHOSTENDEN	(1 << 20)
+#define DWC3_OEVTEN_ADEVHOSTEN		(1 << 19)
+#define DWC3_OEVTEN_ADEVHNPCHNGEN	(1 << 18)
+#define DWC3_OEVTEN_ADEVSRPDETEN	(1 << 17)
+#define DWC3_OEVTEN_ADEVSESSENDDETEN	(1 << 16)
+#define DWC3_OEVTEN_BDEVHOSTENDEN	(1 << 11)
+#define DWC3_OEVTEN_BDEVHNPCHNGEN	(1 << 10)
+#define DWC3_OEVTEN_BDEVSESSVLDDETEN	(1 << 9)
+#define DWC3_OEVTEN_BDEVVBUSCHNGE	(1 << 8)
+
+/* OTG Status Register */
+#define DWC3_OSTS_DEVRUNSTP		(1 << 13)
+#define DWC3_OSTS_XHCIRUNSTP		(1 << 12)
+#define DWC3_OSTS_PERIPHERALSTATE	(1 << 4)
+#define DWC3_OSTS_XHCIPRTPOWER		(1 << 3)
+#define DWC3_OSTS_BSESVLD		(1 << 2)
+#define DWC3_OSTS_VBUSVLD		(1 << 1)
+#define DWC3_OSTS_CONIDSTS		(1 << 0)
+
 /* Structures */
 
 struct dwc3_trb;
-- 
2.1.4

[PATCH v4 3/9] usb: dwc3: dwc3-omap: Make the wrapper interrupt shared

[Roger Quadros]

The wrapper interrupt is shared with OTG core so mark it IRQF_SHARED.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/dwc3/dwc3-omap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
index a5a1b7c..b18f2a3 100644
--- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -506,8 +506,8 @@ static int dwc3_omap_probe(struct platform_device *pdev)
 	reg = dwc3_omap_readl(omap->base, USBOTGSS_SYSCONFIG);
 	omap->dma_status = !!(reg & USBOTGSS_SYSCONFIG_DMADISABLE);
 
-	ret = devm_request_irq(dev, omap->irq, dwc3_omap_interrupt, 0,
-			"dwc3-omap", omap);
+	ret = devm_request_irq(dev, omap->irq, dwc3_omap_interrupt, IRQF_SHARED,
+			       "dwc3-omap", omap);
 	if (ret) {
 		dev_err(dev, "failed to request IRQ #%d --> %d\n",
 				omap->irq, ret);
-- 
2.1.4

Re: [PATCH v4 3/9] usb: dwc3: dwc3-omap: Make the wrapper interrupt shared

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1098 bytes --]

On Wed, Sep 02, 2015 at 05:24:18PM +0300, Roger Quadros wrote:
> The wrapper interrupt is shared with OTG core so mark it IRQF_SHARED.
> 
> Signed-off-by: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
> ---
>  drivers/usb/dwc3/dwc3-omap.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
> index a5a1b7c..b18f2a3 100644
> --- a/drivers/usb/dwc3/dwc3-omap.c
> +++ b/drivers/usb/dwc3/dwc3-omap.c
> @@ -506,8 +506,8 @@ static int dwc3_omap_probe(struct platform_device *pdev)
>  	reg = dwc3_omap_readl(omap->base, USBOTGSS_SYSCONFIG);
>  	omap->dma_status = !!(reg & USBOTGSS_SYSCONFIG_DMADISABLE);
>  
> -	ret = devm_request_irq(dev, omap->irq, dwc3_omap_interrupt, 0,
> -			"dwc3-omap", omap);
> +	ret = devm_request_irq(dev, omap->irq, dwc3_omap_interrupt, IRQF_SHARED,
> +			       "dwc3-omap", omap);

looks like only first line needs to change

>  	if (ret) {
>  		dev_err(dev, "failed to request IRQ #%d --> %d\n",
>  				omap->irq, ret);
> -- 
> 2.1.4
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v4 4/9] usb: dwc3: core: Adapt to named interrupts

[Roger Quadros]

From: Felipe Balbi <balbi@ti.com>

Add support to use interrupt names,

Following are the interrupt names

Peripheral Interrupt - peripheral
HOST Interrupt - host
OTG Interrupt - otg

[Roger Q]
- If any of these are missing we use the
first available IRQ resource so that we don't
break with older DTBs.

- Use gadget_irq in gadget driver.

Signed-off-by: Felipe Balbi <balbi@ti.com>
Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/dwc3/core.c   | 12 ++++++++++++
 drivers/usb/dwc3/core.h   |  7 +++++++
 drivers/usb/dwc3/gadget.c |  2 +-
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 2e36a9b..38b31df 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -958,6 +958,18 @@ static int dwc3_probe(struct platform_device *pdev)
 	dwc->xhci_resources[1].flags = res->flags;
 	dwc->xhci_resources[1].name = res->name;
 
+	dwc->otg_irq = platform_get_irq_byname(pdev, "otg");
+	if (dwc->otg_irq < 0)
+		dwc->otg_irq = res->start;
+
+	dwc->gadget_irq = platform_get_irq_byname(pdev, "peripheral");
+	if (dwc->gadget_irq < 0)
+		dwc->gadget_irq = res->start;
+
+	dwc->xhci_irq = platform_get_irq_byname(pdev, "host");
+	if (dwc->xhci_irq < 0)
+		dwc->xhci_irq = res->start;
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
 		dev_err(dev, "missing memory resource\n");
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 4b85330..bd32cb2 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -742,6 +742,9 @@ struct dwc3_scratchpad_array {
  * @maximum_speed: maximum speed requested (mainly for testing purposes)
  * @revision: revision register contents
  * @dr_mode: requested mode of operation
+ * @xhci_irq: IRQ number for XHCI IRQs
+ * @gadget_irq: IRQ number for Peripheral IRQs
+ * @otg_irq: IRQ number for OTG IRQs
  * @usb2_phy: pointer to USB2 PHY
  * @usb3_phy: pointer to USB3 PHY
  * @usb2_generic_phy: pointer to USB2 PHY
@@ -847,6 +850,10 @@ struct dwc3 {
 
 	enum usb_dr_mode	dr_mode;
 
+	int			gadget_irq;
+	int			xhci_irq;
+	int			otg_irq;
+
 	/* used for suspend/resume */
 	u32			dcfg;
 	u32			gctl;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 0c25704..1148ec8 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1533,7 +1533,7 @@ static int dwc3_gadget_start(struct usb_gadget *g,
 	int			irq;
 	u32			reg;
 
-	irq = platform_get_irq(to_platform_device(dwc->dev), 0);
+	irq = dwc->gadget_irq;
 	ret = request_threaded_irq(irq, dwc3_interrupt, dwc3_thread_interrupt,
 			IRQF_SHARED, "dwc3", dwc);
 	if (ret) {
-- 
2.1.4

Re: [PATCH v4 4/9] usb: dwc3: core: Adapt to named interrupts

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 9089 bytes --]

On Wed, Sep 02, 2015 at 05:24:19PM +0300, Roger Quadros wrote:
> From: Felipe Balbi <balbi@ti.com>
> 
> Add support to use interrupt names,
> 
> Following are the interrupt names
> 
> Peripheral Interrupt - peripheral
> HOST Interrupt - host
> OTG Interrupt - otg
> 
> [Roger Q]
> - If any of these are missing we use the
> first available IRQ resource so that we don't
> break with older DTBs.

this is what original commit did:

commit ecd5f71cfd663bcd4efd2f29824acd8b2ba9715d
Author: Felipe Balbi <balbi@ti.com>
Date:   Fri Jan 3 13:49:38 2014 -0600

    usb: dwc3: cleanup IRQ resources
    
    In order to make it easier for the driver to
    figure out which modes of operation it has,
    and because some dwc3 integrations have rather
    fuzzy IRQ routing, we now require three different
    IRQ numbers (peripheral, host, otg).
    
    In order to do that and maintain backwards compatibility,
    we still maintain support for the old IRQ resource name,
    but if you *really* want to have proper peripheral/host/otg
    support, you should make sure your resources are correct.
    
    Signed-off-by: Felipe Balbi <balbi@ti.com>

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 60580a01cdd2..1f01031873b7 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -556,10 +556,22 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
 static int dwc3_core_init_mode(struct dwc3 *dwc)
 {
 	struct device *dev = dwc->dev;
+	struct platform_device *pdev = to_platform_device(dev);
 	int ret;
 
 	switch (dwc->dr_mode) {
 	case USB_DR_MODE_PERIPHERAL:
+		dwc->gadget_irq = platform_get_irq_byname(pdev, "dwc3_peripheral");
+		if (dwc->gadget_irq < 0) {
+			dwc->gadget_irq = platform_get_irq_byname(pdev, "dwc_usb3");
+			if (dwc->gadget_irq < 0) {
+				dev_err(dev, "missing IRQ\n");
+				return dwc->gadget_irq;
+			} else {
+				dev_warn(dev, "dwc_usb3 resource is deprecated\n");
+			}
+		}
+
 		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
 		ret = dwc3_gadget_init(dwc);
 		if (ret) {
@@ -568,6 +580,22 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
 		}
 		break;
 	case USB_DR_MODE_HOST:
+		dwc->xhci_irq = platform_get_irq_byname(pdev, "dwc3_host");
+		if (dwc->xhci_irq < 0) {
+			dwc->xhci_irq = platform_get_irq_byname(pdev, "dwc_usb3");
+			if (dwc->xhci_irq < 0) {
+				dev_err(dev, "missing Host IRQ\n");
+				return dwc->xhci_irq;
+			} else {
+				dev_warn(dev, "dwc_usb3 resource is deprecated\n");
+			}
+
+			dwc->xhci_resources[1].start = dwc->xhci_irq;
+			dwc->xhci_resources[1].end = dwc->xhci_irq;
+			dwc->xhci_resources[1].flags = IORESOURCE_IRQ;
+			dwc->xhci_resources[1].name = "xhci";
+		}
+
 		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
 		ret = dwc3_host_init(dwc);
 		if (ret) {
@@ -576,6 +604,28 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
 		}
 		break;
 	case USB_DR_MODE_OTG:
+		dwc->gadget_irq = platform_get_irq_byname(pdev, "dwc3_peripheral");
+		if (dwc->gadget_irq < 0) {
+			dwc->gadget_irq = platform_get_irq_byname(pdev, "dwc_usb3");
+			if (dwc->gadget_irq < 0) {
+				dev_err(dev, "missing IRQ\n");
+				return dwc->gadget_irq;
+			} else {
+				dev_warn(dev, "dwc_usb3 resource is deprecated\n");
+			}
+
+			dwc->xhci_irq = dwc->gadget_irq;
+			dwc->otg_irq = dwc->gadget_irq;
+		} else {
+			dwc->xhci_irq = platform_get_irq_byname(pdev, "dwc3_host");
+			if (dwc->xhci_irq < 0) {
+				dev_err(dev, "missing Host IRQ\n");
+				return dwc->xhci_irq;
+			}
+
+			dwc->otg_irq = platform_get_irq_byname(pdev, "dwc3_otg");
+		}
+
 		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
 		ret = dwc3_otg_init(dwc);
 		if (ret) {
@@ -647,18 +697,6 @@ static int dwc3_probe(struct platform_device *pdev)
 	dwc->mem = mem;
 	dwc->dev = dev;
 
-	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-	if (!res) {
-		dev_err(dev, "missing IRQ\n");
-		return -ENODEV;
-	}
-	dwc->xhci_resources[1].start = res->start;
-	dwc->xhci_resources[1].end = res->end;
-	dwc->xhci_resources[1].flags = res->flags;
-	dwc->xhci_resources[1].name = res->name;
-
-	dwc->otg_irq = platform_get_irq_byname(pdev, "dwc3_otg");
-
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
 		dev_err(dev, "missing memory resource\n");
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 8e2e579b4b1e..0b02186fad6f 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -671,6 +671,8 @@ struct dwc3_scratchpad_array {
  * @maximum_speed: maximum speed requested (mainly for testing purposes)
  * @revision: revision register contents
  * @dr_mode: requested mode of operation
+ * @gadget_irq: IRQ number for Peripheral IRQs
+ * @xhci_irq: IRQ number for Host IRQs
  * @otg_irq: IRQ number for OTG IRQs
  * @usb2_phy: pointer to USB2 PHY
  * @usb3_phy: pointer to USB3 PHY
@@ -747,6 +749,8 @@ struct dwc3 {
 
 	enum usb_dr_mode	dr_mode;
 
+	int			gadget_irq;
+	int			xhci_irq;
 	int			otg_irq;
 
 	/* used for suspend/resume */
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index a8cf87b3de01..2a6e5155fc89 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1567,15 +1567,13 @@ static int dwc3_gadget_start(struct usb_gadget *g,
 	struct dwc3_ep		*dep;
 	unsigned long		flags;
 	int			ret = 0;
-	int			irq;
 	u32			reg;
 
-	irq = platform_get_irq(to_platform_device(dwc->dev), 0);
-	ret = request_threaded_irq(irq, dwc3_interrupt, dwc3_thread_interrupt,
-			IRQF_SHARED, "dwc3", dwc);
+	ret = request_threaded_irq(dwc->gadget_irq, dwc3_interrupt,
+			dwc3_thread_interrupt, IRQF_SHARED, "dwc3", dwc);
 	if (ret) {
 		dev_err(dwc->dev, "failed to request irq #%d --> %d\n",
-				irq, ret);
+				dwc->gadget_irq, ret);
 		goto err0;
 	}
 
@@ -1668,7 +1666,7 @@ err2:
 err1:
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
-	free_irq(irq, dwc);
+	free_irq(dwc->gadget_irq, dwc);
 
 err0:
 	return ret;
@@ -1679,7 +1677,6 @@ static int dwc3_gadget_stop(struct usb_gadget *g,
 {
 	struct dwc3		*dwc = gadget_to_dwc(g);
 	unsigned long		flags;
-	int			irq;
 
 	spin_lock_irqsave(&dwc->lock, flags);
 
@@ -1691,8 +1688,7 @@ static int dwc3_gadget_stop(struct usb_gadget *g,
 
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
-	irq = platform_get_irq(to_platform_device(dwc->dev), 0);
-	free_irq(irq, dwc);
+	free_irq(dwc->gadget_irq, dwc);
 
 	return 0;
 }

> 
> - Use gadget_irq in gadget driver.
> 
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  drivers/usb/dwc3/core.c   | 12 ++++++++++++
>  drivers/usb/dwc3/core.h   |  7 +++++++
>  drivers/usb/dwc3/gadget.c |  2 +-
>  3 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 2e36a9b..38b31df 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -958,6 +958,18 @@ static int dwc3_probe(struct platform_device *pdev)
>  	dwc->xhci_resources[1].flags = res->flags;
>  	dwc->xhci_resources[1].name = res->name;
>  
> +	dwc->otg_irq = platform_get_irq_byname(pdev, "otg");
> +	if (dwc->otg_irq < 0)
> +		dwc->otg_irq = res->start;
> +
> +	dwc->gadget_irq = platform_get_irq_byname(pdev, "peripheral");
> +	if (dwc->gadget_irq < 0)
> +		dwc->gadget_irq = res->start;
> +
> +	dwc->xhci_irq = platform_get_irq_byname(pdev, "host");
> +	if (dwc->xhci_irq < 0)
> +		dwc->xhci_irq = res->start;
> +
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (!res) {
>  		dev_err(dev, "missing memory resource\n");
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 4b85330..bd32cb2 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -742,6 +742,9 @@ struct dwc3_scratchpad_array {
>   * @maximum_speed: maximum speed requested (mainly for testing purposes)
>   * @revision: revision register contents
>   * @dr_mode: requested mode of operation
> + * @xhci_irq: IRQ number for XHCI IRQs
> + * @gadget_irq: IRQ number for Peripheral IRQs
> + * @otg_irq: IRQ number for OTG IRQs
>   * @usb2_phy: pointer to USB2 PHY
>   * @usb3_phy: pointer to USB3 PHY
>   * @usb2_generic_phy: pointer to USB2 PHY
> @@ -847,6 +850,10 @@ struct dwc3 {
>  
>  	enum usb_dr_mode	dr_mode;
>  
> +	int			gadget_irq;
> +	int			xhci_irq;
> +	int			otg_irq;
> +
>  	/* used for suspend/resume */
>  	u32			dcfg;
>  	u32			gctl;
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 0c25704..1148ec8 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1533,7 +1533,7 @@ static int dwc3_gadget_start(struct usb_gadget *g,
>  	int			irq;
>  	u32			reg;
>  
> -	irq = platform_get_irq(to_platform_device(dwc->dev), 0);
> +	irq = dwc->gadget_irq;
>  	ret = request_threaded_irq(irq, dwc3_interrupt, dwc3_thread_interrupt,
>  			IRQF_SHARED, "dwc3", dwc);
>  	if (ret) {
> -- 
> 2.1.4
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 4/9] usb: dwc3: core: Adapt to named interrupts

[Roger Quadros]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 02/09/15 17:34, Felipe Balbi wrote:
> On Wed, Sep 02, 2015 at 05:24:19PM +0300, Roger Quadros wrote:
>> From: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
>>
>> Add support to use interrupt names,
>>
>> Following are the interrupt names
>>
>> Peripheral Interrupt - peripheral
>> HOST Interrupt - host
>> OTG Interrupt - otg
>>
>> [Roger Q]
>> - If any of these are missing we use the
>> first available IRQ resource so that we don't
>> break with older DTBs.
> 
> this is what original commit did:
> 
> commit ecd5f71cfd663bcd4efd2f29824acd8b2ba9715d
> Author: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> Date:   Fri Jan 3 13:49:38 2014 -0600
> 
>     usb: dwc3: cleanup IRQ resources
>     
>     In order to make it easier for the driver to
>     figure out which modes of operation it has,
>     and because some dwc3 integrations have rather
>     fuzzy IRQ routing, we now require three different
>     IRQ numbers (peripheral, host, otg).
>     
>     In order to do that and maintain backwards compatibility,
>     we still maintain support for the old IRQ resource name,
>     but if you *really* want to have proper peripheral/host/otg
>     support, you should make sure your resources are correct.
>     
>     Signed-off-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>

This is better since we request the resource only if needed and bail out
if it is not present.

> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 60580a01cdd2..1f01031873b7 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -556,10 +556,22 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
>  static int dwc3_core_init_mode(struct dwc3 *dwc)
>  {
>  	struct device *dev = dwc->dev;
> +	struct platform_device *pdev = to_platform_device(dev);
>  	int ret;
>  
>  	switch (dwc->dr_mode) {
>  	case USB_DR_MODE_PERIPHERAL:
> +		dwc->gadget_irq = platform_get_irq_byname(pdev, "dwc3_peripheral");

Shall we just name it just "peripheral"?

> +		if (dwc->gadget_irq < 0) {
> +			dwc->gadget_irq = platform_get_irq_byname(pdev, "dwc_usb3");

How will this work? Currently we don't have a name for the IRQ in the DT.

> +			if (dwc->gadget_irq < 0) {
> +				dev_err(dev, "missing IRQ\n");
> +				return dwc->gadget_irq;
> +			} else {
> +				dev_warn(dev, "dwc_usb3 resource is deprecated\n");

Do we want to warn about legacy nodes?

> +			}
> +		}
> +
>  		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
>  		ret = dwc3_gadget_init(dwc);
>  		if (ret) {
> @@ -568,6 +580,22 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>  		}
>  		break;
>  	case USB_DR_MODE_HOST:
> +		dwc->xhci_irq = platform_get_irq_byname(pdev, "dwc3_host");
> +		if (dwc->xhci_irq < 0) {
> +			dwc->xhci_irq = platform_get_irq_byname(pdev, "dwc_usb3");
> +			if (dwc->xhci_irq < 0) {
> +				dev_err(dev, "missing Host IRQ\n");
> +				return dwc->xhci_irq;
> +			} else {
> +				dev_warn(dev, "dwc_usb3 resource is deprecated\n");
> +			}
> +
> +			dwc->xhci_resources[1].start = dwc->xhci_irq;
> +			dwc->xhci_resources[1].end = dwc->xhci_irq;
> +			dwc->xhci_resources[1].flags = IORESOURCE_IRQ;
> +			dwc->xhci_resources[1].name = "xhci";
> +		}
> +
>  		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
>  		ret = dwc3_host_init(dwc);
>  		if (ret) {
> @@ -576,6 +604,28 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>  		}
>  		break;
>  	case USB_DR_MODE_OTG:
> +		dwc->gadget_irq = platform_get_irq_byname(pdev, "dwc3_peripheral");
> +		if (dwc->gadget_irq < 0) {
> +			dwc->gadget_irq = platform_get_irq_byname(pdev, "dwc_usb3");
> +			if (dwc->gadget_irq < 0) {
> +				dev_err(dev, "missing IRQ\n");
> +				return dwc->gadget_irq;
> +			} else {
> +				dev_warn(dev, "dwc_usb3 resource is deprecated\n");
> +			}
> +
> +			dwc->xhci_irq = dwc->gadget_irq;
> +			dwc->otg_irq = dwc->gadget_irq;
> +		} else {
> +			dwc->xhci_irq = platform_get_irq_byname(pdev, "dwc3_host");
> +			if (dwc->xhci_irq < 0) {
> +				dev_err(dev, "missing Host IRQ\n");
> +				return dwc->xhci_irq;
> +			}
> +
> +			dwc->otg_irq = platform_get_irq_byname(pdev, "dwc3_otg");

need to check if error?

> +		}

Need to setup xhci_resource[1]?

> +
>  		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
>  		ret = dwc3_otg_init(dwc);
>  		if (ret) {
> @@ -647,18 +697,6 @@ static int dwc3_probe(struct platform_device *pdev)
>  	dwc->mem = mem;
>  	dwc->dev = dev;
>  
> -	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> -	if (!res) {
> -		dev_err(dev, "missing IRQ\n");
> -		return -ENODEV;
> -	}
> -	dwc->xhci_resources[1].start = res->start;
> -	dwc->xhci_resources[1].end = res->end;
> -	dwc->xhci_resources[1].flags = res->flags;
> -	dwc->xhci_resources[1].name = res->name;
> -
> -	dwc->otg_irq = platform_get_irq_byname(pdev, "dwc3_otg");
> -
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (!res) {
>  		dev_err(dev, "missing memory resource\n");
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 8e2e579b4b1e..0b02186fad6f 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -671,6 +671,8 @@ struct dwc3_scratchpad_array {
>   * @maximum_speed: maximum speed requested (mainly for testing purposes)
>   * @revision: revision register contents
>   * @dr_mode: requested mode of operation
> + * @gadget_irq: IRQ number for Peripheral IRQs
> + * @xhci_irq: IRQ number for Host IRQs
>   * @otg_irq: IRQ number for OTG IRQs
>   * @usb2_phy: pointer to USB2 PHY
>   * @usb3_phy: pointer to USB3 PHY
> @@ -747,6 +749,8 @@ struct dwc3 {
>  
>  	enum usb_dr_mode	dr_mode;
>  
> +	int			gadget_irq;
> +	int			xhci_irq;
>  	int			otg_irq;
>  
>  	/* used for suspend/resume */
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index a8cf87b3de01..2a6e5155fc89 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1567,15 +1567,13 @@ static int dwc3_gadget_start(struct usb_gadget *g,
>  	struct dwc3_ep		*dep;
>  	unsigned long		flags;
>  	int			ret = 0;
> -	int			irq;
>  	u32			reg;
>  
> -	irq = platform_get_irq(to_platform_device(dwc->dev), 0);
> -	ret = request_threaded_irq(irq, dwc3_interrupt, dwc3_thread_interrupt,
> -			IRQF_SHARED, "dwc3", dwc);
> +	ret = request_threaded_irq(dwc->gadget_irq, dwc3_interrupt,
> +			dwc3_thread_interrupt, IRQF_SHARED, "dwc3", dwc);
>  	if (ret) {
>  		dev_err(dwc->dev, "failed to request irq #%d --> %d\n",
> -				irq, ret);
> +				dwc->gadget_irq, ret);
>  		goto err0;
>  	}
>  
> @@ -1668,7 +1666,7 @@ err2:
>  err1:
>  	spin_unlock_irqrestore(&dwc->lock, flags);
>  
> -	free_irq(irq, dwc);
> +	free_irq(dwc->gadget_irq, dwc);
>  
>  err0:
>  	return ret;
> @@ -1679,7 +1677,6 @@ static int dwc3_gadget_stop(struct usb_gadget *g,
>  {
>  	struct dwc3		*dwc = gadget_to_dwc(g);
>  	unsigned long		flags;
> -	int			irq;
>  
>  	spin_lock_irqsave(&dwc->lock, flags);
>  
> @@ -1691,8 +1688,7 @@ static int dwc3_gadget_stop(struct usb_gadget *g,
>  
>  	spin_unlock_irqrestore(&dwc->lock, flags);
>  
> -	irq = platform_get_irq(to_platform_device(dwc->dev), 0);
> -	free_irq(irq, dwc);
> +	free_irq(dwc->gadget_irq, dwc);
>  
>  	return 0;
>  }
> 

- --
cheers,
- -roger
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJV6EEzAAoJENJaa9O+djCT2CYQALOJ+zhWZ1fw8z1FEHnouTT7
6riR0KkGkptuGiRCfUTZYNA+YSMLK2vaZbgH6YWqOrrgK2T45kEAsvoO66byN/2W
XistUWUDRR3X5vhxS4Jsc6euHU7+zKVioTog+qmfFgu/NUJrSUg2w4Q8IR/YGq29
7En5WjGSagd9EoSrS6q9GxYY5m/WOR2Nt/pqOvdtd+izJiRkKrYNfCBzv47majv7
Pdyh6VMBLGjLpQDzKJ+dC8pRzBKI5MpGPNsCFuCNGhjrS5KIv+nMnrtezNRHrU9m
BRzcaYjPj90+zF52BeYPsk+e+r4DqZrnsCw8JWAf/Z+dSiHDcllnNHFjmjyxEWVE
QSVKX6rzq2ADGlCjklEUiy/iFlVV4tF+BzdS+ykEHAT4H7F6h3uAnLCZIE31DEZ/
SL7AfBxzO9xp4hweiQfz8VfpjrI7GWv6kR2A8zWV2pPD5TPo0pSNghI+r0eK1mJ8
rt1IlQnaxqFxNJjYtdVpvppV/ATihqXX+6e3Zgr3rvr8nA/rMEQZ5ePid0tPh4yM
r4r5JAfrP0ROIkvHkQ/otEey2yK5YBMiQ0kqKWZAOztAZTsEeEP0zrJWKIJXtKt5
7P3kOTuZNko4INO7i+F3bb2zBL1ies/mKhdsuvu65QJi7KMDYvLfLdtHTudSwkfX
mtsHh+60Nc0pqqIqH12t
=M0TG
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 4/9] usb: dwc3: core: Adapt to named interrupts

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3950 bytes --]

Hi,

On Thu, Sep 03, 2015 at 03:46:43PM +0300, Roger Quadros wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
> 
> On 02/09/15 17:34, Felipe Balbi wrote:
> > On Wed, Sep 02, 2015 at 05:24:19PM +0300, Roger Quadros wrote:
> >> From: Felipe Balbi <balbi@ti.com>
> >>
> >> Add support to use interrupt names,
> >>
> >> Following are the interrupt names
> >>
> >> Peripheral Interrupt - peripheral
> >> HOST Interrupt - host
> >> OTG Interrupt - otg
> >>
> >> [Roger Q]
> >> - If any of these are missing we use the
> >> first available IRQ resource so that we don't
> >> break with older DTBs.
> > 
> > this is what original commit did:
> > 
> > commit ecd5f71cfd663bcd4efd2f29824acd8b2ba9715d
> > Author: Felipe Balbi <balbi@ti.com>
> > Date:   Fri Jan 3 13:49:38 2014 -0600
> > 
> >     usb: dwc3: cleanup IRQ resources
> >     
> >     In order to make it easier for the driver to
> >     figure out which modes of operation it has,
> >     and because some dwc3 integrations have rather
> >     fuzzy IRQ routing, we now require three different
> >     IRQ numbers (peripheral, host, otg).
> >     
> >     In order to do that and maintain backwards compatibility,
> >     we still maintain support for the old IRQ resource name,
> >     but if you *really* want to have proper peripheral/host/otg
> >     support, you should make sure your resources are correct.
> >     
> >     Signed-off-by: Felipe Balbi <balbi@ti.com>
> 
> This is better since we request the resource only if needed and bail out
> if it is not present.
> 
> > 
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 60580a01cdd2..1f01031873b7 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -556,10 +556,22 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
> >  static int dwc3_core_init_mode(struct dwc3 *dwc)
> >  {
> >  	struct device *dev = dwc->dev;
> > +	struct platform_device *pdev = to_platform_device(dev);
> >  	int ret;
> >  
> >  	switch (dwc->dr_mode) {
> >  	case USB_DR_MODE_PERIPHERAL:
> > +		dwc->gadget_irq = platform_get_irq_byname(pdev, "dwc3_peripheral");
> 
> Shall we just name it just "peripheral"?

sure, why not :-)

> > +		if (dwc->gadget_irq < 0) {
> > +			dwc->gadget_irq = platform_get_irq_byname(pdev, "dwc_usb3");
> 
> How will this work? Currently we don't have a name for the IRQ in the DT.

we can just add interrupt-names, right ? Or, the fallback could be what
is already done today: just fetch it by index.

> 
> > +			if (dwc->gadget_irq < 0) {
> > +				dev_err(dev, "missing IRQ\n");
> > +				return dwc->gadget_irq;
> > +			} else {
> > +				dev_warn(dev, "dwc_usb3 resource is deprecated\n");
> 
> Do we want to warn about legacy nodes?

Sure :-)

Now, do you want me to update it, or will you do it ? BTW, if you decide
to do it, we need to patch all users :-)

> > @@ -576,6 +604,28 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
> >  		}
> >  		break;
> >  	case USB_DR_MODE_OTG:
> > +		dwc->gadget_irq = platform_get_irq_byname(pdev, "dwc3_peripheral");
> > +		if (dwc->gadget_irq < 0) {
> > +			dwc->gadget_irq = platform_get_irq_byname(pdev, "dwc_usb3");
> > +			if (dwc->gadget_irq < 0) {
> > +				dev_err(dev, "missing IRQ\n");
> > +				return dwc->gadget_irq;
> > +			} else {
> > +				dev_warn(dev, "dwc_usb3 resource is deprecated\n");
> > +			}
> > +
> > +			dwc->xhci_irq = dwc->gadget_irq;
> > +			dwc->otg_irq = dwc->gadget_irq;
> > +		} else {
> > +			dwc->xhci_irq = platform_get_irq_byname(pdev, "dwc3_host");
> > +			if (dwc->xhci_irq < 0) {
> > +				dev_err(dev, "missing Host IRQ\n");
> > +				return dwc->xhci_irq;
> > +			}
> > +
> > +			dwc->otg_irq = platform_get_irq_byname(pdev, "dwc3_otg");
> 
> need to check if error?

right

> > +		}
> 
> Need to setup xhci_resource[1]?

isn't it done above ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 4/9] usb: dwc3: core: Adapt to named interrupts

[Roger Quadros]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Hi,

On 03/09/15 18:48, Felipe Balbi wrote:
> Hi,
> 
> On Thu, Sep 03, 2015 at 03:46:43PM +0300, Roger Quadros wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA256
>>
>> On 02/09/15 17:34, Felipe Balbi wrote:
>>> On Wed, Sep 02, 2015 at 05:24:19PM +0300, Roger Quadros wrote:
>>>> From: Felipe Balbi <balbi@ti.com>
>>>>
>>>> Add support to use interrupt names,
>>>>
>>>> Following are the interrupt names
>>>>
>>>> Peripheral Interrupt - peripheral
>>>> HOST Interrupt - host
>>>> OTG Interrupt - otg
>>>>
>>>> [Roger Q]
>>>> - If any of these are missing we use the
>>>> first available IRQ resource so that we don't
>>>> break with older DTBs.
>>>
>>> this is what original commit did:
>>>
>>> commit ecd5f71cfd663bcd4efd2f29824acd8b2ba9715d
>>> Author: Felipe Balbi <balbi@ti.com>
>>> Date:   Fri Jan 3 13:49:38 2014 -0600
>>>
>>>     usb: dwc3: cleanup IRQ resources
>>>     
>>>     In order to make it easier for the driver to
>>>     figure out which modes of operation it has,
>>>     and because some dwc3 integrations have rather
>>>     fuzzy IRQ routing, we now require three different
>>>     IRQ numbers (peripheral, host, otg).
>>>     
>>>     In order to do that and maintain backwards compatibility,
>>>     we still maintain support for the old IRQ resource name,
>>>     but if you *really* want to have proper peripheral/host/otg
>>>     support, you should make sure your resources are correct.
>>>     
>>>     Signed-off-by: Felipe Balbi <balbi@ti.com>
>>
>> This is better since we request the resource only if needed and bail out
>> if it is not present.
>>
>>>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index 60580a01cdd2..1f01031873b7 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -556,10 +556,22 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
>>>  static int dwc3_core_init_mode(struct dwc3 *dwc)
>>>  {
>>>  	struct device *dev = dwc->dev;
>>> +	struct platform_device *pdev = to_platform_device(dev);
>>>  	int ret;
>>>  
>>>  	switch (dwc->dr_mode) {
>>>  	case USB_DR_MODE_PERIPHERAL:
>>> +		dwc->gadget_irq = platform_get_irq_byname(pdev, "dwc3_peripheral");
>>
>> Shall we just name it just "peripheral"?
> 
> sure, why not :-)
> 
>>> +		if (dwc->gadget_irq < 0) {
>>> +			dwc->gadget_irq = platform_get_irq_byname(pdev, "dwc_usb3");
>>
>> How will this work? Currently we don't have a name for the IRQ in the DT.
> 
> we can just add interrupt-names, right ? Or, the fallback could be what
> is already done today: just fetch it by index.
> 
>>
>>> +			if (dwc->gadget_irq < 0) {
>>> +				dev_err(dev, "missing IRQ\n");
>>> +				return dwc->gadget_irq;
>>> +			} else {
>>> +				dev_warn(dev, "dwc_usb3 resource is deprecated\n");
>>
>> Do we want to warn about legacy nodes?
> 
> Sure :-)
> 
> Now, do you want me to update it, or will you do it ? BTW, if you decide
> to do it, we need to patch all users :-)

Would appreciate if you can do it. It is independent of dual-role support and can go in early.
BTW omap users are already using the named interrupt property.

> 
>>> @@ -576,6 +604,28 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>>>  		}
>>>  		break;
>>>  	case USB_DR_MODE_OTG:
>>> +		dwc->gadget_irq = platform_get_irq_byname(pdev, "dwc3_peripheral");
>>> +		if (dwc->gadget_irq < 0) {
>>> +			dwc->gadget_irq = platform_get_irq_byname(pdev, "dwc_usb3");
>>> +			if (dwc->gadget_irq < 0) {
>>> +				dev_err(dev, "missing IRQ\n");
>>> +				return dwc->gadget_irq;
>>> +			} else {
>>> +				dev_warn(dev, "dwc_usb3 resource is deprecated\n");
>>> +			}
>>> +
>>> +			dwc->xhci_irq = dwc->gadget_irq;
>>> +			dwc->otg_irq = dwc->gadget_irq;
>>> +		} else {
>>> +			dwc->xhci_irq = platform_get_irq_byname(pdev, "dwc3_host");
>>> +			if (dwc->xhci_irq < 0) {
>>> +				dev_err(dev, "missing Host IRQ\n");
>>> +				return dwc->xhci_irq;
>>> +			}
>>> +
>>> +			dwc->otg_irq = platform_get_irq_byname(pdev, "dwc3_otg");
>>
>> need to check if error?
> 
> right
> 
>>> +		}
>>
>> Need to setup xhci_resource[1]?
> 
> isn't it done above ?
> 

It is done in the USB_DR_MODE_HOST case, but that won't be executed for USB_DR_MODE_OTG case, no?
I'm talking about this part

+			dwc->xhci_resources[1].start = dwc->xhci_irq;
+			dwc->xhci_resources[1].end = dwc->xhci_irq;
+			dwc->xhci_resources[1].flags = IORESOURCE_IRQ;
+			dwc->xhci_resources[1].name = "xhci";

cheers,
- -roger
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJV6WBDAAoJENJaa9O+djCTXW4QAIx6ok9D/bGqz8F++KKdIJmQ
Kj7cedP1B4el/+OQ20n38g9U8HosrY5wtB9n0VdBc99bLGbBFMOheeamLQw8u6rU
WKT4zcXjbasDURE25eqmAu3Nc7EiXVebiu07GEVR82/zl2H6edSddaf9Bx/3+yzm
VV6gwIXBISbkTuTfw5Nsh9GR4+X3KTOE78Nygtv7MfbLpMrfbABGHhWnGpCRn5oO
SHMvZd1HUHFKb0Z2nKrBXV9yzayQtfjAjNjKn2rIxwirKdMAk96z+32Ql3lh06Aq
jxe+UK/KXH9+Cd25FFEQlazBxcd7gnbugS+MYroUW8dPSWsX0LjfIkzjDbwwfh12
DqLMhwa+ouCvJMjrU5ifRvtmVvNS5LUY3Em6ZQW+3nzXrFdiaAuzOTc8WeMGKL9T
Hl63yKsc1kGgtkpoxvMlNdkQABIN7B7mMlJTZ3GKLBcneRcWjc/nPmsCV+ASuukg
K5Cck/RVtB2KBhh8e1vrZIFJOVrtoB6T22MwJf3Lrw+uVxxprM9w9SbE1nibWU/G
7AebuHXtM08Hon999j4odQaAmKU1zS0EfSm6bdy6/7cTDfhicidkK4w3d9MxR9oo
kOTIE/ti3tppH94CiTN6fjT6I0Z9en6lEPnUv1X1pypbRKWCQw0o7/GxCrhkkbE/
eHH1noaEXEGyoTzmrlrW
=fDSz
-----END PGP SIGNATURE-----

[PATCH v4 6/9] usb: dwc3: save/restore OTG registers during suspend/resume

[Roger Quadros]

Without this we loose OTG controller register context and malfunction
after a system suspend-resume.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/dwc3/core.c | 17 +++++++++++++++++
 drivers/usb/dwc3/core.h |  6 +++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 632ee53..684010c 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -56,6 +56,7 @@ void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
 	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
 	reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));
 	reg |= DWC3_GCTL_PRTCAPDIR(mode);
+	dwc->current_mode = mode;
 	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
 }
 
@@ -1443,6 +1444,14 @@ static int dwc3_suspend(struct device *dev)
 
 	spin_lock_irqsave(&dwc->lock, flags);
 
+	/* Save OTG state only if we're really using it */
+	if (dwc->current_mode == DWC3_GCTL_PRTCAP_OTG) {
+		dwc->ocfg = dwc3_readl(dwc->regs, DWC3_OCFG);
+		dwc->octl = dwc3_readl(dwc->regs, DWC3_OCTL);
+		dwc->oevt = dwc3_readl(dwc->regs, DWC3_OEVT);
+		dwc->oevten = dwc3_readl(dwc->regs, DWC3_OEVTEN);
+	}
+
 	switch (dwc->dr_mode) {
 	case USB_DR_MODE_PERIPHERAL:
 	case USB_DR_MODE_OTG:
@@ -1486,6 +1495,14 @@ static int dwc3_resume(struct device *dev)
 	dwc3_event_buffers_setup(dwc);
 	dwc3_writel(dwc->regs, DWC3_GCTL, dwc->gctl);
 
+	/* Restore OTG state only if we're really using it */
+	if (dwc->current_mode == DWC3_GCTL_PRTCAP_OTG) {
+		dwc3_writel(dwc->regs, DWC3_OCFG, dwc->ocfg);
+		dwc3_writel(dwc->regs, DWC3_OCTL, dwc->octl);
+		dwc3_writel(dwc->regs, DWC3_OEVT, dwc->oevt);
+		dwc3_writel(dwc->regs, DWC3_OEVTEN, dwc->oevten);
+	}
+
 	switch (dwc->dr_mode) {
 	case USB_DR_MODE_PERIPHERAL:
 	case USB_DR_MODE_OTG:
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 129ef37..1115ce0 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -737,6 +737,7 @@ struct dwc3_scratchpad_array {
  * @regs: base address for our registers
  * @regs_size: address space size
  * @oevten: otg interrupt enable mask copy
+ * @current_mode: current mode of operation written to PRTCAPDIR
  * @nr_scratch: number of scratch buffers
  * @num_event_buffers: calculated number of event buffers
  * @u1u2: only used on revisions <1.83a for workaround
@@ -858,9 +859,12 @@ struct dwc3 {
 	/* used for suspend/resume */
 	u32			dcfg;
 	u32			gctl;
-
+	u32			ocfg;
+	u32			octl;
+	u32			oevt;
 	u32			oevten;
 
+	u32			current_mode;
 	u32			nr_scratch;
 	u32			num_event_buffers;
 	u32			u1u2;
-- 
2.1.4

Re: [PATCH v4 6/9] usb: dwc3: save/restore OTG registers during suspend/resume

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3002 bytes --]

On Wed, Sep 02, 2015 at 05:24:21PM +0300, Roger Quadros wrote:
> Without this we loose OTG controller register context and malfunction
> after a system suspend-resume.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  drivers/usb/dwc3/core.c | 17 +++++++++++++++++
>  drivers/usb/dwc3/core.h |  6 +++++-
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 632ee53..684010c 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -56,6 +56,7 @@ void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
>  	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
>  	reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));
>  	reg |= DWC3_GCTL_PRTCAPDIR(mode);
> +	dwc->current_mode = mode;
>  	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
>  }
>  
> @@ -1443,6 +1444,14 @@ static int dwc3_suspend(struct device *dev)
>  
>  	spin_lock_irqsave(&dwc->lock, flags);
>  
> +	/* Save OTG state only if we're really using it */
> +	if (dwc->current_mode == DWC3_GCTL_PRTCAP_OTG) {
> +		dwc->ocfg = dwc3_readl(dwc->regs, DWC3_OCFG);
> +		dwc->octl = dwc3_readl(dwc->regs, DWC3_OCTL);
> +		dwc->oevt = dwc3_readl(dwc->regs, DWC3_OEVT);

oevt is what you use to clear pending IRQs, which means that ...

> +		dwc->oevten = dwc3_readl(dwc->regs, DWC3_OEVTEN);
> +	}
> +
>  	switch (dwc->dr_mode) {
>  	case USB_DR_MODE_PERIPHERAL:
>  	case USB_DR_MODE_OTG:
> @@ -1486,6 +1495,14 @@ static int dwc3_resume(struct device *dev)
>  	dwc3_event_buffers_setup(dwc);
>  	dwc3_writel(dwc->regs, DWC3_GCTL, dwc->gctl);
>  
> +	/* Restore OTG state only if we're really using it */
> +	if (dwc->current_mode == DWC3_GCTL_PRTCAP_OTG) {
> +		dwc3_writel(dwc->regs, DWC3_OCFG, dwc->ocfg);
> +		dwc3_writel(dwc->regs, DWC3_OCTL, dwc->octl);
> +		dwc3_writel(dwc->regs, DWC3_OEVT, dwc->oevt);

... you could be clearing pending IRQs right here.

> +		dwc3_writel(dwc->regs, DWC3_OEVTEN, dwc->oevten);
> +	}
> +
>  	switch (dwc->dr_mode) {
>  	case USB_DR_MODE_PERIPHERAL:
>  	case USB_DR_MODE_OTG:
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 129ef37..1115ce0 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -737,6 +737,7 @@ struct dwc3_scratchpad_array {
>   * @regs: base address for our registers
>   * @regs_size: address space size
>   * @oevten: otg interrupt enable mask copy
> + * @current_mode: current mode of operation written to PRTCAPDIR
>   * @nr_scratch: number of scratch buffers
>   * @num_event_buffers: calculated number of event buffers
>   * @u1u2: only used on revisions <1.83a for workaround
> @@ -858,9 +859,12 @@ struct dwc3 {
>  	/* used for suspend/resume */
>  	u32			dcfg;
>  	u32			gctl;
> -
> +	u32			ocfg;
> +	u32			octl;
> +	u32			oevt;
>  	u32			oevten;
>  
> +	u32			current_mode;
>  	u32			nr_scratch;
>  	u32			num_event_buffers;
>  	u32			u1u2;
> -- 
> 2.1.4
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 6/9] usb: dwc3: save/restore OTG registers during suspend/resume

[Roger Quadros]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 02/09/15 17:44, Felipe Balbi wrote:
> On Wed, Sep 02, 2015 at 05:24:21PM +0300, Roger Quadros wrote:
>> Without this we loose OTG controller register context and malfunction
>> after a system suspend-resume.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>  drivers/usb/dwc3/core.c | 17 +++++++++++++++++
>>  drivers/usb/dwc3/core.h |  6 +++++-
>>  2 files changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 632ee53..684010c 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -56,6 +56,7 @@ void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
>>  	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
>>  	reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));
>>  	reg |= DWC3_GCTL_PRTCAPDIR(mode);
>> +	dwc->current_mode = mode;
>>  	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
>>  }
>>  
>> @@ -1443,6 +1444,14 @@ static int dwc3_suspend(struct device *dev)
>>  
>>  	spin_lock_irqsave(&dwc->lock, flags);
>>  
>> +	/* Save OTG state only if we're really using it */
>> +	if (dwc->current_mode == DWC3_GCTL_PRTCAP_OTG) {
>> +		dwc->ocfg = dwc3_readl(dwc->regs, DWC3_OCFG);
>> +		dwc->octl = dwc3_readl(dwc->regs, DWC3_OCTL);
>> +		dwc->oevt = dwc3_readl(dwc->regs, DWC3_OEVT);
> 
> oevt is what you use to clear pending IRQs, which means that ...
> 
>> +		dwc->oevten = dwc3_readl(dwc->regs, DWC3_OEVTEN);
>> +	}
>> +
>>  	switch (dwc->dr_mode) {
>>  	case USB_DR_MODE_PERIPHERAL:
>>  	case USB_DR_MODE_OTG:
>> @@ -1486,6 +1495,14 @@ static int dwc3_resume(struct device *dev)
>>  	dwc3_event_buffers_setup(dwc);
>>  	dwc3_writel(dwc->regs, DWC3_GCTL, dwc->gctl);
>>  
>> +	/* Restore OTG state only if we're really using it */
>> +	if (dwc->current_mode == DWC3_GCTL_PRTCAP_OTG) {
>> +		dwc3_writel(dwc->regs, DWC3_OCFG, dwc->ocfg);
>> +		dwc3_writel(dwc->regs, DWC3_OCTL, dwc->octl);
>> +		dwc3_writel(dwc->regs, DWC3_OEVT, dwc->oevt);
> 
> ... you could be clearing pending IRQs right here.

Good catch. So we can't really restore this register.
I'll remove this line.

> 
>> +		dwc3_writel(dwc->regs, DWC3_OEVTEN, dwc->oevten);
>> +	}
>> +
>>  	switch (dwc->dr_mode) {
>>  	case USB_DR_MODE_PERIPHERAL:
>>  	case USB_DR_MODE_OTG:
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 129ef37..1115ce0 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -737,6 +737,7 @@ struct dwc3_scratchpad_array {
>>   * @regs: base address for our registers
>>   * @regs_size: address space size
>>   * @oevten: otg interrupt enable mask copy
>> + * @current_mode: current mode of operation written to PRTCAPDIR
>>   * @nr_scratch: number of scratch buffers
>>   * @num_event_buffers: calculated number of event buffers
>>   * @u1u2: only used on revisions <1.83a for workaround
>> @@ -858,9 +859,12 @@ struct dwc3 {
>>  	/* used for suspend/resume */
>>  	u32			dcfg;
>>  	u32			gctl;
>> -
>> +	u32			ocfg;
>> +	u32			octl;
>> +	u32			oevt;
>>  	u32			oevten;
>>  
>> +	u32			current_mode;
>>  	u32			nr_scratch;
>>  	u32			num_event_buffers;
>>  	u32			u1u2;
>> -- 
>> 2.1.4
>>
> 

- --
cheers,
- -roger
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJV6FEGAAoJENJaa9O+djCTE7gP/0rlQJPL1R11k8hJCM842zjT
SSxfdNQ9kkG008D88As8nLodL79agvD/XBot7iIrTGVFMG/tzzkwQb71gZoCOogb
K0eMMnLNyzLRapNpOe1OJqI1NjO8Yx1o7heoaGkGYERR7m6dwz1LmSPYEtaaveVq
5gb5NbUKfSq0Cs6wM+3+U8CQzAPLf7eRSZwiORkzyn7cWiZDsYHr9Bn35NA24r5v
0YMQvmTp7K/7pFENiFHmOWbKPoY6G8ebkInBSMXQfHcLPtL86l9e+VrlJrIZzBqQ
SJyoQw8JFU9c2ojtKeG9AYn8fSbCQpbsuIFAZtCVy2rK4xMiITmaSsjHF5+KpBt2
cE7eahicYAP3edLqT0R0hYfiI+JrVuOdkiW+W8UWPHrGt/nELQjQxZtIkTOI1b9B
FIxnZbIwp0veEE+vqXiTQizUPdZag0qPG1PZ5Bpu0867vfX6khY2MCNeYTDatzHT
dz9RwWIDH3/K5K7TOug/VZEiqvLY3BYMrMK+W3FreGfagwvYvlLyojsHifrA1x9z
1+zAjkml1kIIP5Jlf3VOYdLsN8mgeMyCNfi5USvVFnzPITAjtwm9+ai7xxxCjSxA
QTvmhQRjAxIPWWZgVuc/Z5PdqgHKFKpCLxG8IE9bqGBQnNSKueZHEhfUCxfPNKV6
ZkkhIq/s6hm6S/YM4WcS
=hUag
-----END PGP SIGNATURE-----

[PATCH v4 7/9] usb: dwc3: gadget: Fix suspend/resume during dual-role mode

[Roger Quadros]

Gdget controller might not be always active during suspend/
resume when we are operating in dual-role/otg mode.
Check if we're active and only if we are then perform
necessary actions during suspend/resume.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/dwc3/gadget.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 1148ec8..ec6112d 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2792,6 +2792,9 @@ void dwc3_gadget_exit(struct dwc3 *dwc)
 
 int dwc3_gadget_suspend(struct dwc3 *dwc)
 {
+	if (!dwc->gadget_driver)
+		return 0;
+
 	if (dwc->pullups_connected) {
 		dwc3_gadget_disable_irq(dwc);
 		dwc3_gadget_run_stop(dwc, true, true);
@@ -2810,6 +2813,9 @@ int dwc3_gadget_resume(struct dwc3 *dwc)
 	struct dwc3_ep		*dep;
 	int			ret;
 
+	if (!dwc->gadget_driver)
+		return 0;
+
 	/* Start with SuperSpeed Default */
 	dwc3_gadget_ep0_desc.wMaxPacketSize = cpu_to_le16(512);
 
-- 
2.1.4

[PATCH v4 8/9] usb: dwc3: core: Prevent otg events from disabling themselves

[Roger Quadros]

There is a race happening during dwc3_drd_init() that causes
otg events to get disabled. This is what happens.

dwc3_otg_irq() happens immediately when PRTCAP is set to OTG,
even though OEVTEN is 0. This is because BIT 31 IRQ of
OEVT can't be disabled by OEVTEN.
We configure OEVTEN in dwc3_otg_init() but dwc3_otg_irq() has
already saved OEVTEN as 0 into dwc->oevten. So finally when
dwc3_irq_thread_irq() is called we save 0 into OEVTEN
thus disabling OTG irqs forever.

We fix this by disabling IRQs when configuring OEVTEN in
dwc3_otg_init().

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/dwc3/core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 684010c..654aebf 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -921,6 +921,7 @@ static int dwc3_drd_init(struct dwc3 *dwc)
 	int ret, id, vbus;
 	struct usb_otg_caps *otgcaps = &dwc->otg_config.otg_caps;
 	u32 reg;
+	unsigned long flags;
 
 	otgcaps->otg_rev = 0;
 	otgcaps->hnp_support = false;
@@ -993,6 +994,8 @@ try_otg_core:
 		goto error;
 	}
 
+	spin_lock_irqsave(&dwc->lock, flags);
+
 	/* we need to set OTG to get events from OTG core */
 	dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
 	/* GUSB2PHYCFG0.SusPHY=0 */
@@ -1018,6 +1021,8 @@ try_otg_core:
 	/* OCTL.PeriMode = 1 */
 	dwc3_writel(dwc->regs, DWC3_OCTL, DWC3_OCTL_PERIMODE);
 
+	spin_unlock_irqrestore(&dwc->lock, flags);
+
 	dwc3_otg_fsm_sync(dwc);
 	usb_otg_sync_inputs(dwc->fsm);
 
-- 
2.1.4

Re: [PATCH v4 8/9] usb: dwc3: core: Prevent otg events from disabling themselves

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1847 bytes --]

On Wed, Sep 02, 2015 at 05:24:23PM +0300, Roger Quadros wrote:
> There is a race happening during dwc3_drd_init() that causes
> otg events to get disabled. This is what happens.
> 
> dwc3_otg_irq() happens immediately when PRTCAP is set to OTG,
> even though OEVTEN is 0. This is because BIT 31 IRQ of
> OEVT can't be disabled by OEVTEN.
> We configure OEVTEN in dwc3_otg_init() but dwc3_otg_irq() has
> already saved OEVTEN as 0 into dwc->oevten. So finally when
> dwc3_irq_thread_irq() is called we save 0 into OEVTEN
> thus disabling OTG irqs forever.
> 
> We fix this by disabling IRQs when configuring OEVTEN in
> dwc3_otg_init().
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>

can't you just merge this patch into the one which introduced the bug to
start with ?

> ---
>  drivers/usb/dwc3/core.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 684010c..654aebf 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -921,6 +921,7 @@ static int dwc3_drd_init(struct dwc3 *dwc)
>  	int ret, id, vbus;
>  	struct usb_otg_caps *otgcaps = &dwc->otg_config.otg_caps;
>  	u32 reg;
> +	unsigned long flags;
>  
>  	otgcaps->otg_rev = 0;
>  	otgcaps->hnp_support = false;
> @@ -993,6 +994,8 @@ try_otg_core:
>  		goto error;
>  	}
>  
> +	spin_lock_irqsave(&dwc->lock, flags);
> +
>  	/* we need to set OTG to get events from OTG core */
>  	dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
>  	/* GUSB2PHYCFG0.SusPHY=0 */
> @@ -1018,6 +1021,8 @@ try_otg_core:
>  	/* OCTL.PeriMode = 1 */
>  	dwc3_writel(dwc->regs, DWC3_OCTL, DWC3_OCTL_PERIMODE);
>  
> +	spin_unlock_irqrestore(&dwc->lock, flags);
> +
>  	dwc3_otg_fsm_sync(dwc);
>  	usb_otg_sync_inputs(dwc->fsm);
>  
> -- 
> 2.1.4
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 8/9] usb: dwc3: core: Prevent otg events from disabling themselves

[Roger Quadros]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 02/09/15 17:47, Felipe Balbi wrote:
> On Wed, Sep 02, 2015 at 05:24:23PM +0300, Roger Quadros wrote:
>> There is a race happening during dwc3_drd_init() that causes
>> otg events to get disabled. This is what happens.
>>
>> dwc3_otg_irq() happens immediately when PRTCAP is set to OTG,
>> even though OEVTEN is 0. This is because BIT 31 IRQ of
>> OEVT can't be disabled by OEVTEN.
>> We configure OEVTEN in dwc3_otg_init() but dwc3_otg_irq() has
>> already saved OEVTEN as 0 into dwc->oevten. So finally when
>> dwc3_irq_thread_irq() is called we save 0 into OEVTEN
>> thus disabling OTG irqs forever.
>>
>> We fix this by disabling IRQs when configuring OEVTEN in
>> dwc3_otg_init().
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
> 
> can't you just merge this patch into the one which introduced the bug to
> start with ?

Yes, I'll do that.

> 
>> ---
>>  drivers/usb/dwc3/core.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 684010c..654aebf 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -921,6 +921,7 @@ static int dwc3_drd_init(struct dwc3 *dwc)
>>  	int ret, id, vbus;
>>  	struct usb_otg_caps *otgcaps = &dwc->otg_config.otg_caps;
>>  	u32 reg;
>> +	unsigned long flags;
>>  
>>  	otgcaps->otg_rev = 0;
>>  	otgcaps->hnp_support = false;
>> @@ -993,6 +994,8 @@ try_otg_core:
>>  		goto error;
>>  	}
>>  
>> +	spin_lock_irqsave(&dwc->lock, flags);
>> +
>>  	/* we need to set OTG to get events from OTG core */
>>  	dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
>>  	/* GUSB2PHYCFG0.SusPHY=0 */
>> @@ -1018,6 +1021,8 @@ try_otg_core:
>>  	/* OCTL.PeriMode = 1 */
>>  	dwc3_writel(dwc->regs, DWC3_OCTL, DWC3_OCTL_PERIMODE);
>>  
>> +	spin_unlock_irqrestore(&dwc->lock, flags);
>> +
>>  	dwc3_otg_fsm_sync(dwc);
>>  	usb_otg_sync_inputs(dwc->fsm);
>>  
>> -- 
>> 2.1.4
>>
> 
- --
cheers,
- -roger
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJV6FEnAAoJENJaa9O+djCTSjgQALL73OtdfKAWte2JFxsw1PVT
Xeh+ec7FJqNCKb2TwTSgnnWp6BxapbAlkK3kSwhsbgZvZLGtf3IC+xPHWNCs4dZs
uCs9oDPC/oAiYBnN3gYZxBFuhe1fsFEuS7aFIzrvKD/+tnXmd0IQ5Yu3k06VDXIM
jf/034Wep9AK44fyPy0sLKET1J6/UMvjxvUpEyxj9elbvs+GqzD9qR/GF6Ob8q6d
5f7y4ajMBCgyoC/wpIXDIJi7yqH1MUZq8bdC+7BKyc8Fz7kzVSpU2SL0BSRMU7Jn
Mzyx25WbWy1mXkfdgOjXtxx6MH5gV6jIIvqobyXXrK/e0z3kU6OeFprbubsOAvmC
fcgRkOrfr1O7/G5Gm7mhU5kLSZF6kvf6MoDPGCd/TbP4OgfYcPiTj5O020qDbRxZ
Hfuy/5Xk1oxR2aui1q9ycCWrpSA4L+B4fSpDXUznZw755BhhPnVeoYlXUfcb2sI+
YPNs6cLeX95I27AoMpK0wjMptmswaKcDmwv50mjK0kXgUcnVGs6z7YxSIJ2eAxPV
OpYLwVTfJiqoeJMscHvqoipwjlHqq4xS780SbzgZTDFOk2Ik7l0B6mNoL9CgaaXa
LVCBEywdWo+9os4b3JRrjDSqqdOMhBVaO0GvtiZZqfF7mOTmJHF/yFAODBw/CLXi
jAmBC/nM3M32CJphQQ1J
=z7cJ
-----END PGP SIGNATURE-----