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 = >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 = _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-omap" in
the body of a message to majord...@vger.kernel.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]

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

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 = >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 = _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-omap" in
the body of a message to majord...@vger.kernel.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]

-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-
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.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]

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


signature.asc
Description: Digital signature

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 
>> ---
>>  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 = >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 = _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, >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,
>> +   >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",
>> +   

[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 
---
 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 = >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 = _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, >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,
+  >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, >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, >otg_nb);
+extcon_fail:
+   extcon_unregister_notifier(dwc->edev, EXTCON_USB, >otg_nb);
+
+   return ret;
+}
+
+static void dwc3_drd_exit(struct dwc3 *dwc)
+{
+   usb_otg_unregister(dwc->dev);
+   extcon_unregister_notifier(dwc->edev, 

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

[Felipe Balbi]

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 
> ---
>  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 = >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 = _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, >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,
> +>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, >otg_config);
> + if (IS_ERR(dwc->fsm)) {
> + ret =