[v2,4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

dra7 OTG core limits the host controller to USB2.0 (high-speed) mode
when we're operating in dual-role.

We work around that by bypassing the OTG core and reading the
extcon framework directly for ID/VBUS events.

Signed-off-by: Roger Quadros <rogerq@ti.com>

---
 Documentation/devicetree/bindings/usb/dwc3.txt |   2 +
 drivers/usb/dwc3/core.c                        | 169 ++++++++++++++++++++++++-
 drivers/usb/dwc3/core.h                        |   5 +
 3 files changed, 170 insertions(+), 6 deletions(-)

-- 
2.7.4

On 02/16/2017 06:36 PM, Roger Quadros wrote:
> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode

> when we're operating in dual-role.

>

> We work around that by bypassing the OTG core and reading the

> extcon framework directly for ID/VBUS events.

>

> Signed-off-by: Roger Quadros <rogerq@ti.com>

> ---

>   Documentation/devicetree/bindings/usb/dwc3.txt |   2 +

>   drivers/usb/dwc3/core.c                        | 169 ++++++++++++++++++++++++-

>   drivers/usb/dwc3/core.h                        |   5 +

>   3 files changed, 170 insertions(+), 6 deletions(-)

>

> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt

> index e3e6983..9955c0d 100644

> --- a/Documentation/devicetree/bindings/usb/dwc3.txt

> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt

> @@ -53,6 +53,8 @@ Optional properties:

>    - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ

>   	register for post-silicon frame length adjustment when the

>   	fladj_30mhz_sdbnd signal is invalid or incorrect.

> + - extcon: phandle to the USB connector extcon device. If present, extcon

> +	device will be used to get USB cable events instead of OTG controller.

>   

>    - <DEPRECATED> tx-fifo-resize: determines if the FIFO *has* to be reallocated.

>   

> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c

> index 619fa7c..b02d911 100644

> --- a/drivers/usb/dwc3/core.c

> +++ b/drivers/usb/dwc3/core.c


[snip]

> @@ -1587,6 +1727,14 @@ static int dwc3_probe(struct platform_device *pdev)

>   

>   	dwc3_get_properties(dwc);

>   

> +	if (dev->of_node) {

> +		if (of_property_read_bool(dev->of_node, "extcon"))

> +			dwc->edev = extcon_get_edev_by_phandle(dev, 0);


Don't we want separate edev's for vbus and id ?
One can have separate pins connected to them and in that case
we can't get the events out of one pin only.

> +

> +		if (IS_ERR(dwc->edev))

> +			return PTR_ERR(dwc->edev);


  Took me a while to get to this. :)

                 if (IS_ERR(dwc->edev)) {
                        ret = PTR_ERR(dwc->edev);
                        goto err0;
                 }

We want to reset the res->start back to its original offset.

Testing this series currently. Will get back with my results.


Regards
Vivek

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

On Thu, Feb 23, 2017 at 02:04:50PM +0530, Vivek Gautam wrote:
> 

> 

> On 02/16/2017 06:36 PM, Roger Quadros wrote:

> >dra7 OTG core limits the host controller to USB2.0 (high-speed) mode

> >when we're operating in dual-role.

> >

> >We work around that by bypassing the OTG core and reading the

> >extcon framework directly for ID/VBUS events.

> >

> >Signed-off-by: Roger Quadros <rogerq@ti.com>

> >---

> >  Documentation/devicetree/bindings/usb/dwc3.txt |   2 +

> >  drivers/usb/dwc3/core.c                        | 169 ++++++++++++++++++++++++-

> >  drivers/usb/dwc3/core.h                        |   5 +

> >  3 files changed, 170 insertions(+), 6 deletions(-)

> >

> >diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt

> >index e3e6983..9955c0d 100644

> >--- a/Documentation/devicetree/bindings/usb/dwc3.txt

> >+++ b/Documentation/devicetree/bindings/usb/dwc3.txt

> >@@ -53,6 +53,8 @@ Optional properties:

> >   - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ

> >  	register for post-silicon frame length adjustment when the

> >  	fladj_30mhz_sdbnd signal is invalid or incorrect.

> >+ - extcon: phandle to the USB connector extcon device. If present, extcon

> >+	device will be used to get USB cable events instead of OTG controller.

> >   - <DEPRECATED> tx-fifo-resize: determines if the FIFO *has* to be reallocated.

> >diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c

> >index 619fa7c..b02d911 100644

> >--- a/drivers/usb/dwc3/core.c

> >+++ b/drivers/usb/dwc3/core.c

> 

> [snip]

> 

> >@@ -1587,6 +1727,14 @@ static int dwc3_probe(struct platform_device *pdev)

> >  	dwc3_get_properties(dwc);

> >+	if (dev->of_node) {

> >+		if (of_property_read_bool(dev->of_node, "extcon"))

> >+			dwc->edev = extcon_get_edev_by_phandle(dev, 0);

> 

> Don't we want separate edev's for vbus and id ?

> One can have separate pins connected to them and in that case

> we can't get the events out of one pin only.

> 


Current extcon-usb-gpio driver supports id and vbus at the same time,
that means there are two optional gpios under one extcon node.

-- 

Best Regards,
Peter Chen

On 02/24/2017 06:27 AM, Peter Chen wrote:
> On Thu, Feb 23, 2017 at 02:04:50PM +0530, Vivek Gautam wrote:

>>

>> On 02/16/2017 06:36 PM, Roger Quadros wrote:

>>> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode

>>> when we're operating in dual-role.

>>>

>>> We work around that by bypassing the OTG core and reading the

>>> extcon framework directly for ID/VBUS events.

>>>

>>> Signed-off-by: Roger Quadros <rogerq@ti.com>

>>> ---

>>>   Documentation/devicetree/bindings/usb/dwc3.txt |   2 +

>>>   drivers/usb/dwc3/core.c                        | 169 ++++++++++++++++++++++++-

>>>   drivers/usb/dwc3/core.h                        |   5 +

>>>   3 files changed, 170 insertions(+), 6 deletions(-)

>>>

>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt

>>> index e3e6983..9955c0d 100644

>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt

>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt

>>> @@ -53,6 +53,8 @@ Optional properties:

>>>    - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ

>>>   	register for post-silicon frame length adjustment when the

>>>   	fladj_30mhz_sdbnd signal is invalid or incorrect.

>>> + - extcon: phandle to the USB connector extcon device. If present, extcon

>>> +	device will be used to get USB cable events instead of OTG controller.

>>>    - <DEPRECATED> tx-fifo-resize: determines if the FIFO *has* to be reallocated.

>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c

>>> index 619fa7c..b02d911 100644

>>> --- a/drivers/usb/dwc3/core.c

>>> +++ b/drivers/usb/dwc3/core.c

>> [snip]

>>

>>> @@ -1587,6 +1727,14 @@ static int dwc3_probe(struct platform_device *pdev)

>>>   	dwc3_get_properties(dwc);

>>> +	if (dev->of_node) {

>>> +		if (of_property_read_bool(dev->of_node, "extcon"))

>>> +			dwc->edev = extcon_get_edev_by_phandle(dev, 0);

>> Don't we want separate edev's for vbus and id ?

>> One can have separate pins connected to them and in that case

>> we can't get the events out of one pin only.

>>

> Current extcon-usb-gpio driver supports id and vbus at the same time,

> that means there are two optional gpios under one extcon node.

>


Right, and we would want to leverage that by providing couple
of phandles for vbus and id, and requesting the two in driver.


Regards
Vivek

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Hi Roger,

[snip]

>  /* dwc->lock must be held */

>  static void dwc3_otg_core_exit(struct dwc3 *dwc)

>  {

> +       if (dwc->edev)

> +               return;

> +

>         /* disable all otg irqs */

>         dwc3_otg_disable_events(dwc, DWC3_OTG_ALL_EVENTS);

>         /* clear all events */

> @@ -1286,6 +1364,57 @@ static int dwc3_drd_init(struct dwc3 *dwc)

>

>         INIT_WORK(&dwc->otg_work, dwc3_drd_work);

>

> +       /* If extcon device is present we don't rely on OTG core for ID event */

> +       if (dwc->edev) {

> +               int id, vbus;

> +

> +               dwc->edev_nb.notifier_call = dwc3_drd_notifier;

> +               ret = extcon_register_notifier(dwc->edev, EXTCON_USB,

> +                                              &dwc->edev_nb);


I recommend that you better to use the devm_extcon_register_notifier()

> +               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->edev_nb);


Ditto.

[snip]

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Hi,

2017-02-24 21:02 GMT+09:00 Roger Quadros <rogerq@ti.com>:
> +Chanwoo

>

> Hi Vivek,

>

> On 23/02/17 10:34, Vivek Gautam wrote:

>>

>>

>> On 02/16/2017 06:36 PM, Roger Quadros wrote:

>>> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode

>>> when we're operating in dual-role.

>>>

>>> We work around that by bypassing the OTG core and reading the

>>> extcon framework directly for ID/VBUS events.

>>>

>>> Signed-off-by: Roger Quadros <rogerq@ti.com>

>>> ---

>>>   Documentation/devicetree/bindings/usb/dwc3.txt |   2 +

>>>   drivers/usb/dwc3/core.c                        | 169 ++++++++++++++++++++++++-

>>>   drivers/usb/dwc3/core.h                        |   5 +

>>>   3 files changed, 170 insertions(+), 6 deletions(-)

>>>

>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt

>>> index e3e6983..9955c0d 100644

>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt

>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt

>>> @@ -53,6 +53,8 @@ Optional properties:

>>>    - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ

>>>       register for post-silicon frame length adjustment when the

>>>       fladj_30mhz_sdbnd signal is invalid or incorrect.

>>> + - extcon: phandle to the USB connector extcon device. If present, extcon

>>> +    device will be used to get USB cable events instead of OTG controller.

>>>      - <DEPRECATED> tx-fifo-resize: determines if the FIFO *has* to be reallocated.

>>>   diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c

>>> index 619fa7c..b02d911 100644

>>> --- a/drivers/usb/dwc3/core.c

>>> +++ b/drivers/usb/dwc3/core.c

>>

>> [snip]

>>

>>> @@ -1587,6 +1727,14 @@ static int dwc3_probe(struct platform_device *pdev)

>>>         dwc3_get_properties(dwc);

>>>   +    if (dev->of_node) {

>>> +        if (of_property_read_bool(dev->of_node, "extcon"))

>>> +            dwc->edev = extcon_get_edev_by_phandle(dev, 0);

>>

>> Don't we want separate edev's for vbus and id ?

>> One can have separate pins connected to them and in that case

>> we can't get the events out of one pin only.

>

> Is such kind of hardware really there? Ideally one extcon device

> represents one connector. So VBUS and ID events of a single USB

> port should come on one extcon device.

> If it doesn't then you need to fix your platforms extcon driver

> so that it does.

> Chanwoo can correct me if I'm wrong on this understanding.

>

> Currently, if VBUS and ID come on GPIOs the extcon-usb-gpio driver

> takes care of both.


Basically, one extcon device driver can support the multiple
external connectors if the detection h/w or port is same.

One extcon device with extcon-usb-gpio.c can support the detection
of both ID and VBUS.

But, if there is any issue of h/w schematic, we need to consider
how to use the extcon device.

>

>>

>>> +

>>> +        if (IS_ERR(dwc->edev))

>>> +            return PTR_ERR(dwc->edev);

>>

>>  Took me a while to get to this. :)

>>

>>                 if (IS_ERR(dwc->edev)) {

>>                        ret = PTR_ERR(dwc->edev);

>>                        goto err0;

>>                 }

>>

>> We want to reset the res->start back to its original offset.

>>

>> Testing this series currently. Will get back with my results.

>>

>>

> Thanks :)

>

> --

> cheers,

> -roger


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

2017-02-25 12:46 GMT+09:00 Chanwoo Choi <cwchoi00@gmail.com>:
> Hi,

>

> 2017-02-24 21:02 GMT+09:00 Roger Quadros <rogerq@ti.com>:

>> +Chanwoo

>>

>> Hi Vivek,

>>

>> On 23/02/17 10:34, Vivek Gautam wrote:

>>>

>>>

>>> On 02/16/2017 06:36 PM, Roger Quadros wrote:

>>>> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode

>>>> when we're operating in dual-role.

>>>>

>>>> We work around that by bypassing the OTG core and reading the

>>>> extcon framework directly for ID/VBUS events.

>>>>

>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>

>>>> ---

>>>>   Documentation/devicetree/bindings/usb/dwc3.txt |   2 +

>>>>   drivers/usb/dwc3/core.c                        | 169 ++++++++++++++++++++++++-

>>>>   drivers/usb/dwc3/core.h                        |   5 +

>>>>   3 files changed, 170 insertions(+), 6 deletions(-)

>>>>

>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt

>>>> index e3e6983..9955c0d 100644

>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt

>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt

>>>> @@ -53,6 +53,8 @@ Optional properties:

>>>>    - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ

>>>>       register for post-silicon frame length adjustment when the

>>>>       fladj_30mhz_sdbnd signal is invalid or incorrect.

>>>> + - extcon: phandle to the USB connector extcon device. If present, extcon

>>>> +    device will be used to get USB cable events instead of OTG controller.

>>>>      - <DEPRECATED> tx-fifo-resize: determines if the FIFO *has* to be reallocated.

>>>>   diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c

>>>> index 619fa7c..b02d911 100644

>>>> --- a/drivers/usb/dwc3/core.c

>>>> +++ b/drivers/usb/dwc3/core.c

>>>

>>> [snip]

>>>

>>>> @@ -1587,6 +1727,14 @@ static int dwc3_probe(struct platform_device *pdev)

>>>>         dwc3_get_properties(dwc);

>>>>   +    if (dev->of_node) {

>>>> +        if (of_property_read_bool(dev->of_node, "extcon"))

>>>> +            dwc->edev = extcon_get_edev_by_phandle(dev, 0);

>>>

>>> Don't we want separate edev's for vbus and id ?

>>> One can have separate pins connected to them and in that case

>>> we can't get the events out of one pin only.

>>

>> Is such kind of hardware really there? Ideally one extcon device

>> represents one connector. So VBUS and ID events of a single USB

>> port should come on one extcon device.

>> If it doesn't then you need to fix your platforms extcon driver

>> so that it does.

>> Chanwoo can correct me if I'm wrong on this understanding.

>>

>> Currently, if VBUS and ID come on GPIOs the extcon-usb-gpio driver

>> takes care of both.

>

> Basically, one extcon device driver can support the multiple

> external connectors if the detection h/w or port is same.

>

> One extcon device with extcon-usb-gpio.c can support the detection

> of both ID and VBUS.

>

> But, if there is any issue of h/w schematic, we need to consider

> how to use the extcon device.


Additionally, the extcon-usb-gpio.c use the two gpio pins
to detect both ID and VBUS pins. We can check it on documentation[1]
of extcon-usb-gpio driver.

[1] https://git.kernel.org/cgit/linux/kernel/git/chanwoo/extcon.git/tree/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

On 25/02/17 05:35, Chanwoo Choi wrote:
> Hi Roger,

> 

> [snip]

> 

>>  /* dwc->lock must be held */

>>  static void dwc3_otg_core_exit(struct dwc3 *dwc)

>>  {

>> +       if (dwc->edev)

>> +               return;

>> +

>>         /* disable all otg irqs */

>>         dwc3_otg_disable_events(dwc, DWC3_OTG_ALL_EVENTS);

>>         /* clear all events */

>> @@ -1286,6 +1364,57 @@ static int dwc3_drd_init(struct dwc3 *dwc)

>>

>>         INIT_WORK(&dwc->otg_work, dwc3_drd_work);

>>

>> +       /* If extcon device is present we don't rely on OTG core for ID event */

>> +       if (dwc->edev) {

>> +               int id, vbus;

>> +

>> +               dwc->edev_nb.notifier_call = dwc3_drd_notifier;

>> +               ret = extcon_register_notifier(dwc->edev, EXTCON_USB,

>> +                                              &dwc->edev_nb);

> 

> I recommend that you better to use the devm_extcon_register_notifier()


Got it.

> 

>> +               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->edev_nb);

> 

> Ditto.

> 

> [snip]

> 


-- 
cheers,
-roger

Hi,

Roger Quadros <rogerq@ti.com> writes:
> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode

> when we're operating in dual-role.


yeah, that's not a quirk. DRA7 supports OTGv2, not OTGv3. There was no
USB3 when OTGv2 was written.

DRA7 just shouldn't use OTG core altogether. In fact, this is the very
thing I've been saying for a long time. Make the simplest implementation
possible. The dead simple, does-one-thing-only sort of implementation.

All we need for Dual-Role (without OTG extras) is some input for ID and
VBUS, then we add/remove HCD/UDC conditionally and set PRTCAPDIR.

-- 
balbi

On 28/03/17 14:10, Felipe Balbi wrote:
> 

> Hi,

> 

> Roger Quadros <rogerq@ti.com> writes:

>> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode

>> when we're operating in dual-role.

> 

> yeah, that's not a quirk. DRA7 supports OTGv2, not OTGv3. There was no

> USB3 when OTGv2 was written.

> 

> DRA7 just shouldn't use OTG core altogether. In fact, this is the very

> thing I've been saying for a long time. Make the simplest implementation

> possible. The dead simple, does-one-thing-only sort of implementation.

> 

> All we need for Dual-Role (without OTG extras) is some input for ID and

> VBUS, then we add/remove HCD/UDC conditionally and set PRTCAPDIR.

> 


The catch is that on AM437x there is no way to get ID and VBUS events other
than the OTG controller so we have to rely on the OTG controller for that. :(

I agree on the simplicity part. Let's do what minimal is necessary first.

cheers,
-roger

Hi,

Roger Quadros <rogerq@ti.com> writes:
>> Roger Quadros <rogerq@ti.com> writes:

>>> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode

>>> when we're operating in dual-role.

>> 

>> yeah, that's not a quirk. DRA7 supports OTGv2, not OTGv3. There was no

>> USB3 when OTGv2 was written.

>> 

>> DRA7 just shouldn't use OTG core altogether. In fact, this is the very

>> thing I've been saying for a long time. Make the simplest implementation

>> possible. The dead simple, does-one-thing-only sort of implementation.

>> 

>> All we need for Dual-Role (without OTG extras) is some input for ID and

>> VBUS, then we add/remove HCD/UDC conditionally and set PRTCAPDIR.

>> 

>

> The catch is that on AM437x there is no way to get ID and VBUS events other

> than the OTG controller so we have to rely on the OTG controller for that. :(


okay, so AM437x can get OTG interrupts properly. That's fine. We can
still do everything we need using code that's already existing in dwc3
if we refactor it a bit and hook it up to the OTG IRQ handler.

Here's what we do:

* First we re-factor all necessary code around so the API for OTG/DRD
  is resumed to calling:

	dwc3_add_udc(dwc);
        dwc3_del_udc(dwc);
        dwc3_add_hcd(dwc);
        dwc3_del_hcd(dwc);

the semantics of these should be easy to understand and you can
implement each in their respective host.c/gadget.c files.

* Second step is to modify our dwc3_init_mode() (or whatever that
  function was called, sorry, didn't check) to make sure we have
  something like:

	case OTG:
        	dwc3_add_udc(dwc);
                break;

We should *not* add HCD in this case yet.

* After that we add otg.c (or drd.c, no preference) and make that call
  dwc3_add_udc(dwc) and, also, provide
  dwc3_add_otg(dwc)/dwc3_del_otg(dwc) calls. Then patch the switch
  statement above to:

	case OTG:
        	dwc3_add_otg(dwc);
                break;

Note that at this point, this is simply a direct replacement of
dwc3_add_udc() to dwc3_add_otg(). This should maintain current behavior
(which is starting with peripheral mode by default), but it should also
add support for OTG interrupts to change the mode (from an interrupt
thead)

	otg_isr()
        {

		/* don't forget to remove preivous mode if necessary */
        	if (perimode)
                	dwc3_add_udc(dwc);
                else
                	dwc3_add_hcd(dwc);
	}

* The next patch would be to choose default conditionally based on
  PERIMODE or whatever.

Of course, this is an oversimplified view of reality. You still need to
poke around at PRTCAPDIR, etc. But all this can, actually, be prototyped
using our "mode" debugfs file. Just make that call
dwc3_add/del_udc/hcd() apart from fiddling with PRTCAPDIR in GCTL.

Your first implementation could be just that. Refactoring what needs to
be refactored, then patching "mode" debugfs to work properly in that
case. Only add otg.c/drd.c after "mode" debugfs file is stable, because
then you know what needs to be taken into consideration.

Just to be clear, I'm not saying we should *ONLY* get the debugfs
interface for v4.12, I'm saying you should start with that and get that
stable and working properly (make an infinite loop constantly changing
modes and keep it running over the weekend) before you add support for
OTG interrupts, which could come in the same series ;-)

-- 
balbi

On 29/03/17 13:32, Felipe Balbi wrote:
> 

> Hi,

> 

> Roger Quadros <rogerq@ti.com> writes:

>>> Roger Quadros <rogerq@ti.com> writes:

>>>> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode

>>>> when we're operating in dual-role.

>>>

>>> yeah, that's not a quirk. DRA7 supports OTGv2, not OTGv3. There was no

>>> USB3 when OTGv2 was written.

>>>

>>> DRA7 just shouldn't use OTG core altogether. In fact, this is the very

>>> thing I've been saying for a long time. Make the simplest implementation

>>> possible. The dead simple, does-one-thing-only sort of implementation.

>>>

>>> All we need for Dual-Role (without OTG extras) is some input for ID and

>>> VBUS, then we add/remove HCD/UDC conditionally and set PRTCAPDIR.

>>>

>>

>> The catch is that on AM437x there is no way to get ID and VBUS events other

>> than the OTG controller so we have to rely on the OTG controller for that. :(

> 

> okay, so AM437x can get OTG interrupts properly. That's fine. We can

> still do everything we need using code that's already existing in dwc3

> if we refactor it a bit and hook it up to the OTG IRQ handler.

> 

> Here's what we do:

> 

> * First we re-factor all necessary code around so the API for OTG/DRD

>   is resumed to calling:

> 

> 	dwc3_add_udc(dwc);

>         dwc3_del_udc(dwc);

>         dwc3_add_hcd(dwc);

>         dwc3_del_hcd(dwc);

> 

> the semantics of these should be easy to understand and you can

> implement each in their respective host.c/gadget.c files.

> 

> * Second step is to modify our dwc3_init_mode() (or whatever that

>   function was called, sorry, didn't check) to make sure we have

>   something like:

> 

> 	case OTG:

>         	dwc3_add_udc(dwc);

>                 break;

> 

> We should *not* add HCD in this case yet.

> 

> * After that we add otg.c (or drd.c, no preference) and make that call

>   dwc3_add_udc(dwc) and, also, provide

>   dwc3_add_otg(dwc)/dwc3_del_otg(dwc) calls. Then patch the switch

>   statement above to:

> 

> 	case OTG:

>         	dwc3_add_otg(dwc);

>                 break;

> 

> Note that at this point, this is simply a direct replacement of

> dwc3_add_udc() to dwc3_add_otg(). This should maintain current behavior

> (which is starting with peripheral mode by default), but it should also

> add support for OTG interrupts to change the mode (from an interrupt

> thead)

> 

> 	otg_isr()

>         {

> 

> 		/* don't forget to remove preivous mode if necessary */

>         	if (perimode)

>                 	dwc3_add_udc(dwc);

>                 else

>                 	dwc3_add_hcd(dwc);

> 	}

> 

> * The next patch would be to choose default conditionally based on

>   PERIMODE or whatever.

> 

> Of course, this is an oversimplified view of reality. You still need to

> poke around at PRTCAPDIR, etc. But all this can, actually, be prototyped

> using our "mode" debugfs file. Just make that call

> dwc3_add/del_udc/hcd() apart from fiddling with PRTCAPDIR in GCTL.

> 

> Your first implementation could be just that. Refactoring what needs to

> be refactored, then patching "mode" debugfs to work properly in that

> case. Only add otg.c/drd.c after "mode" debugfs file is stable, because

> then you know what needs to be taken into consideration.

> 

> Just to be clear, I'm not saying we should *ONLY* get the debugfs

> interface for v4.12, I'm saying you should start with that and get that

> stable and working properly (make an infinite loop constantly changing

> modes and keep it running over the weekend) before you add support for

> OTG interrupts, which could come in the same series ;-)

> 


Agree with you. Moreover I could get rid of OTG controller related code
and have just debugfs and extcon implementation. We can add the OTG controller
bits later.

I agree with you on everything you said except using add/del_gadget_udc. :)
I've explained why we can't use del_gadget_udc in the other thread
but I'll explain it here again.

1) If we start in host role, usb_add_gadget_udc() won't be called. That means
no UDC and user can't load a gadget driver. Typical applications need to have
a gadget driver ready *before* the peripheral mode starts so that it can
enumerate immediately.

2) If we use usb_del_gadget_udc() when switching to host mode and
usb_add_gadget_udc() when switching back to peripheral mode, the previously
loaded gadget driver will not be assigned to this UDC. User has to unload
and reload the gadget driver.

3) All this becomes even more complex for configfs based gadget driver.

So using stop/start gadget is a much simpler solution really as UDC software
side of things remain unchanged and the gadget driver can persist between
role switches.

cheers,
-roger

Hi,

Roger Quadros <rogerq@ti.com> writes:
>> Roger Quadros <rogerq@ti.com> writes:

>>>> Roger Quadros <rogerq@ti.com> writes:

>>>>> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode

>>>>> when we're operating in dual-role.

>>>>

>>>> yeah, that's not a quirk. DRA7 supports OTGv2, not OTGv3. There was no

>>>> USB3 when OTGv2 was written.

>>>>

>>>> DRA7 just shouldn't use OTG core altogether. In fact, this is the very

>>>> thing I've been saying for a long time. Make the simplest implementation

>>>> possible. The dead simple, does-one-thing-only sort of implementation.

>>>>

>>>> All we need for Dual-Role (without OTG extras) is some input for ID and

>>>> VBUS, then we add/remove HCD/UDC conditionally and set PRTCAPDIR.

>>>>

>>>

>>> The catch is that on AM437x there is no way to get ID and VBUS events other

>>> than the OTG controller so we have to rely on the OTG controller for that. :(

>> 

>> okay, so AM437x can get OTG interrupts properly. That's fine. We can

>> still do everything we need using code that's already existing in dwc3

>> if we refactor it a bit and hook it up to the OTG IRQ handler.

>> 

>> Here's what we do:

>> 

>> * First we re-factor all necessary code around so the API for OTG/DRD

>>   is resumed to calling:

>> 

>> 	dwc3_add_udc(dwc);

>>         dwc3_del_udc(dwc);

>>         dwc3_add_hcd(dwc);

>>         dwc3_del_hcd(dwc);

>> 

>> the semantics of these should be easy to understand and you can

>> implement each in their respective host.c/gadget.c files.

>> 

>> * Second step is to modify our dwc3_init_mode() (or whatever that

>>   function was called, sorry, didn't check) to make sure we have

>>   something like:

>> 

>> 	case OTG:

>>         	dwc3_add_udc(dwc);

>>                 break;

>> 

>> We should *not* add HCD in this case yet.

>> 

>> * After that we add otg.c (or drd.c, no preference) and make that call

>>   dwc3_add_udc(dwc) and, also, provide

>>   dwc3_add_otg(dwc)/dwc3_del_otg(dwc) calls. Then patch the switch

>>   statement above to:

>> 

>> 	case OTG:

>>         	dwc3_add_otg(dwc);

>>                 break;

>> 

>> Note that at this point, this is simply a direct replacement of

>> dwc3_add_udc() to dwc3_add_otg(). This should maintain current behavior

>> (which is starting with peripheral mode by default), but it should also

>> add support for OTG interrupts to change the mode (from an interrupt

>> thead)

>> 

>> 	otg_isr()

>>         {

>> 

>> 		/* don't forget to remove preivous mode if necessary */

>>         	if (perimode)

>>                 	dwc3_add_udc(dwc);

>>                 else

>>                 	dwc3_add_hcd(dwc);

>> 	}

>> 

>> * The next patch would be to choose default conditionally based on

>>   PERIMODE or whatever.

>> 

>> Of course, this is an oversimplified view of reality. You still need to

>> poke around at PRTCAPDIR, etc. But all this can, actually, be prototyped

>> using our "mode" debugfs file. Just make that call

>> dwc3_add/del_udc/hcd() apart from fiddling with PRTCAPDIR in GCTL.

>> 

>> Your first implementation could be just that. Refactoring what needs to

>> be refactored, then patching "mode" debugfs to work properly in that

>> case. Only add otg.c/drd.c after "mode" debugfs file is stable, because

>> then you know what needs to be taken into consideration.

>> 

>> Just to be clear, I'm not saying we should *ONLY* get the debugfs

>> interface for v4.12, I'm saying you should start with that and get that

>> stable and working properly (make an infinite loop constantly changing

>> modes and keep it running over the weekend) before you add support for

>> OTG interrupts, which could come in the same series ;-)

>> 

>

> Agree with you. Moreover I could get rid of OTG controller related code

> and have just debugfs and extcon implementation. We can add the OTG controller

> bits later.

>

> I agree with you on everything you said except using add/del_gadget_udc. :)

> I've explained why we can't use del_gadget_udc in the other thread

> but I'll explain it here again.

>

> 1) If we start in host role, usb_add_gadget_udc() won't be called. That means

> no UDC and user can't load a gadget driver. Typical applications need to have

> a gadget driver ready *before* the peripheral mode starts so that it can

> enumerate immediately.


that has changed since you started writing this series :-) gadget
drivers are kept in pending list until a UDC is around. I'll get
information on that tomorrow, if you require.

> 2) If we use usb_del_gadget_udc() when switching to host mode and

> usb_add_gadget_udc() when switching back to peripheral mode, the previously

> loaded gadget driver will not be assigned to this UDC. User has to unload

> and reload the gadget driver.


that should not be the case anymore, if it is we have a bug in udc-core

> 3) All this becomes even more complex for configfs based gadget driver.

>

> So using stop/start gadget is a much simpler solution really as UDC software

> side of things remain unchanged and the gadget driver can persist between

> role switches.


I hadn't considered configfs, I'll try this out tomorrow as well.

-- 
balbi

On 29/03/17 16:21, Felipe Balbi wrote:
> 

> Hi,

> 

> Roger Quadros <rogerq@ti.com> writes:

>>> Roger Quadros <rogerq@ti.com> writes:

>>>>> Roger Quadros <rogerq@ti.com> writes:

>>>>>> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode

>>>>>> when we're operating in dual-role.

>>>>>

>>>>> yeah, that's not a quirk. DRA7 supports OTGv2, not OTGv3. There was no

>>>>> USB3 when OTGv2 was written.

>>>>>

>>>>> DRA7 just shouldn't use OTG core altogether. In fact, this is the very

>>>>> thing I've been saying for a long time. Make the simplest implementation

>>>>> possible. The dead simple, does-one-thing-only sort of implementation.

>>>>>

>>>>> All we need for Dual-Role (without OTG extras) is some input for ID and

>>>>> VBUS, then we add/remove HCD/UDC conditionally and set PRTCAPDIR.

>>>>>

>>>>

>>>> The catch is that on AM437x there is no way to get ID and VBUS events other

>>>> than the OTG controller so we have to rely on the OTG controller for that. :(

>>>

>>> okay, so AM437x can get OTG interrupts properly. That's fine. We can

>>> still do everything we need using code that's already existing in dwc3

>>> if we refactor it a bit and hook it up to the OTG IRQ handler.

>>>

>>> Here's what we do:

>>>

>>> * First we re-factor all necessary code around so the API for OTG/DRD

>>>   is resumed to calling:

>>>

>>> 	dwc3_add_udc(dwc);

>>>         dwc3_del_udc(dwc);

>>>         dwc3_add_hcd(dwc);

>>>         dwc3_del_hcd(dwc);

>>>

>>> the semantics of these should be easy to understand and you can

>>> implement each in their respective host.c/gadget.c files.

>>>

>>> * Second step is to modify our dwc3_init_mode() (or whatever that

>>>   function was called, sorry, didn't check) to make sure we have

>>>   something like:

>>>

>>> 	case OTG:

>>>         	dwc3_add_udc(dwc);

>>>                 break;

>>>

>>> We should *not* add HCD in this case yet.

>>>

>>> * After that we add otg.c (or drd.c, no preference) and make that call

>>>   dwc3_add_udc(dwc) and, also, provide

>>>   dwc3_add_otg(dwc)/dwc3_del_otg(dwc) calls. Then patch the switch

>>>   statement above to:

>>>

>>> 	case OTG:

>>>         	dwc3_add_otg(dwc);

>>>                 break;

>>>

>>> Note that at this point, this is simply a direct replacement of

>>> dwc3_add_udc() to dwc3_add_otg(). This should maintain current behavior

>>> (which is starting with peripheral mode by default), but it should also

>>> add support for OTG interrupts to change the mode (from an interrupt

>>> thead)

>>>

>>> 	otg_isr()

>>>         {

>>>

>>> 		/* don't forget to remove preivous mode if necessary */

>>>         	if (perimode)

>>>                 	dwc3_add_udc(dwc);

>>>                 else

>>>                 	dwc3_add_hcd(dwc);

>>> 	}

>>>

>>> * The next patch would be to choose default conditionally based on

>>>   PERIMODE or whatever.

>>>

>>> Of course, this is an oversimplified view of reality. You still need to

>>> poke around at PRTCAPDIR, etc. But all this can, actually, be prototyped

>>> using our "mode" debugfs file. Just make that call

>>> dwc3_add/del_udc/hcd() apart from fiddling with PRTCAPDIR in GCTL.

>>>

>>> Your first implementation could be just that. Refactoring what needs to

>>> be refactored, then patching "mode" debugfs to work properly in that

>>> case. Only add otg.c/drd.c after "mode" debugfs file is stable, because

>>> then you know what needs to be taken into consideration.

>>>

>>> Just to be clear, I'm not saying we should *ONLY* get the debugfs

>>> interface for v4.12, I'm saying you should start with that and get that

>>> stable and working properly (make an infinite loop constantly changing

>>> modes and keep it running over the weekend) before you add support for

>>> OTG interrupts, which could come in the same series ;-)

>>>

>>

>> Agree with you. Moreover I could get rid of OTG controller related code

>> and have just debugfs and extcon implementation. We can add the OTG controller

>> bits later.

>>

>> I agree with you on everything you said except using add/del_gadget_udc. :)

>> I've explained why we can't use del_gadget_udc in the other thread

>> but I'll explain it here again.

>>

>> 1) If we start in host role, usb_add_gadget_udc() won't be called. That means

>> no UDC and user can't load a gadget driver. Typical applications need to have

>> a gadget driver ready *before* the peripheral mode starts so that it can

>> enumerate immediately.

> 

> that has changed since you started writing this series :-) gadget

> drivers are kept in pending list until a UDC is around. I'll get

> information on that tomorrow, if you require.


"until a UDC is around" is the key point. If we never call usb_add_gadget_udc()
or we call usb_del_gadget_udc() then the UDC is not around right?
> 

>> 2) If we use usb_del_gadget_udc() when switching to host mode and

>> usb_add_gadget_udc() when switching back to peripheral mode, the previously

>> loaded gadget driver will not be assigned to this UDC. User has to unload

>> and reload the gadget driver.

> 

> that should not be the case anymore, if it is we have a bug in udc-core


OK. good to know.
> 

>> 3) All this becomes even more complex for configfs based gadget driver.

>>

>> So using stop/start gadget is a much simpler solution really as UDC software

>> side of things remain unchanged and the gadget driver can persist between

>> role switches.

> 

> I hadn't considered configfs, I'll try this out tomorrow as well.

> 

Al-right, thanks.

cheers,
-roger

Hi,

Roger Quadros <rogerq@ti.com> writes:
>>> 3) All this becomes even more complex for configfs based gadget driver.

>>>

>>> So using stop/start gadget is a much simpler solution really as UDC software

>>> side of things remain unchanged and the gadget driver can persist between

>>> role switches.

>> 

>> I hadn't considered configfs, I'll try this out tomorrow as well.

>> 

> Al-right, thanks.


just tested with g_zero.ko and a configfs-based f_mass_storage
gadget. All works fine.

-- 
balbi

On 30/03/17 12:32, Felipe Balbi wrote:
> 

> Hi,

> 

> Roger Quadros <rogerq@ti.com> writes:

>>>> 3) All this becomes even more complex for configfs based gadget driver.

>>>>

>>>> So using stop/start gadget is a much simpler solution really as UDC software

>>>> side of things remain unchanged and the gadget driver can persist between

>>>> role switches.

>>>

>>> I hadn't considered configfs, I'll try this out tomorrow as well.

>>>

>> Al-right, thanks.

> 

> just tested with g_zero.ko and a configfs-based f_mass_storage

> gadget. All works fine.

> 

Good to know. Thanks for the test.

cheers,
-roger

Hi,

On 29/03/17 13:32, Felipe Balbi wrote:
> 

> Hi,

> 

> Roger Quadros <rogerq@ti.com> writes:

>>> Roger Quadros <rogerq@ti.com> writes:

>>>> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode

>>>> when we're operating in dual-role.

>>>

>>> yeah, that's not a quirk. DRA7 supports OTGv2, not OTGv3. There was no

>>> USB3 when OTGv2 was written.

>>>

>>> DRA7 just shouldn't use OTG core altogether. In fact, this is the very

>>> thing I've been saying for a long time. Make the simplest implementation

>>> possible. The dead simple, does-one-thing-only sort of implementation.

>>>

>>> All we need for Dual-Role (without OTG extras) is some input for ID and

>>> VBUS, then we add/remove HCD/UDC conditionally and set PRTCAPDIR.

>>>

>>

>> The catch is that on AM437x there is no way to get ID and VBUS events other

>> than the OTG controller so we have to rely on the OTG controller for that. :(

> 

> okay, so AM437x can get OTG interrupts properly. That's fine. We can

> still do everything we need using code that's already existing in dwc3

> if we refactor it a bit and hook it up to the OTG IRQ handler.

> 

> Here's what we do:

> 

> * First we re-factor all necessary code around so the API for OTG/DRD

>   is resumed to calling:

> 

> 	dwc3_add_udc(dwc);

>         dwc3_del_udc(dwc);

>         dwc3_add_hcd(dwc);

>         dwc3_del_hcd(dwc);


Why do we need these new APIs? don't these suffice?
	dwc3_gadget_init(dwc);
	dwc3_gadget_exit(dwc);
	dwc3_host_init(dwc);
	dwc3_host_exit(dwc);
> 

> the semantics of these should be easy to understand and you can

> implement each in their respective host.c/gadget.c files.

> 

> * Second step is to modify our dwc3_init_mode() (or whatever that

>   function was called, sorry, didn't check) to make sure we have

>   something like:

> 

> 	case OTG:

>         	dwc3_add_udc(dwc);

>                 break;

> 

> We should *not* add HCD in this case yet.

> 

> * After that we add otg.c (or drd.c, no preference) and make that call

>   dwc3_add_udc(dwc) and, also, provide

>   dwc3_add_otg(dwc)/dwc3_del_otg(dwc) calls. Then patch the switch

>   statement above to:

> 

> 	case OTG:

>         	dwc3_add_otg(dwc);

>                 break;

> 

> Note that at this point, this is simply a direct replacement of

> dwc3_add_udc() to dwc3_add_otg(). This should maintain current behavior

> (which is starting with peripheral mode by default), but it should also

> add support for OTG interrupts to change the mode (from an interrupt

> thead)

> 

> 	otg_isr()

>         {

> 

> 		/* don't forget to remove preivous mode if necessary */

>         	if (perimode)

>                 	dwc3_add_udc(dwc);

>                 else

>                 	dwc3_add_hcd(dwc);

> 	}

> 

> * The next patch would be to choose default conditionally based on

>   PERIMODE or whatever.

> 

> Of course, this is an oversimplified view of reality. You still need to

> poke around at PRTCAPDIR, etc. But all this can, actually, be prototyped

> using our "mode" debugfs file. Just make that call

> dwc3_add/del_udc/hcd() apart from fiddling with PRTCAPDIR in GCTL.


We also need to ensure that system suspend/resume doesn't break.
Mainly if we suspend/resume with UDC removed.
> 

> Your first implementation could be just that. Refactoring what needs to

> be refactored, then patching "mode" debugfs to work properly in that

> case. Only add otg.c/drd.c after "mode" debugfs file is stable, because

> then you know what needs to be taken into consideration.

> 

> Just to be clear, I'm not saying we should *ONLY* get the debugfs

> interface for v4.12, I'm saying you should start with that and get that

> stable and working properly (make an infinite loop constantly changing

> modes and keep it running over the weekend) before you add support for

> OTG interrupts, which could come in the same series ;-)

> 


Just to clarify debugfs mode behaviour.

Currently it is just changing PRTCAPDIR. What we need to do is that if
dr_mode == "otg", then we call dwc3_host/gadget_init/exit() accordingly as well.

Does this make sense?

cheers,
-roger

Hi,

Roger Quadros <rogerq@ti.com> writes:
>>>> Roger Quadros <rogerq@ti.com> writes:

>>>>> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode

>>>>> when we're operating in dual-role.

>>>>

>>>> yeah, that's not a quirk. DRA7 supports OTGv2, not OTGv3. There was no

>>>> USB3 when OTGv2 was written.

>>>>

>>>> DRA7 just shouldn't use OTG core altogether. In fact, this is the very

>>>> thing I've been saying for a long time. Make the simplest implementation

>>>> possible. The dead simple, does-one-thing-only sort of implementation.

>>>>

>>>> All we need for Dual-Role (without OTG extras) is some input for ID and

>>>> VBUS, then we add/remove HCD/UDC conditionally and set PRTCAPDIR.

>>>>

>>>

>>> The catch is that on AM437x there is no way to get ID and VBUS events other

>>> than the OTG controller so we have to rely on the OTG controller for that. :(

>> 

>> okay, so AM437x can get OTG interrupts properly. That's fine. We can

>> still do everything we need using code that's already existing in dwc3

>> if we refactor it a bit and hook it up to the OTG IRQ handler.

>> 

>> Here's what we do:

>> 

>> * First we re-factor all necessary code around so the API for OTG/DRD

>>   is resumed to calling:

>> 

>> 	dwc3_add_udc(dwc);

>>         dwc3_del_udc(dwc);

>>         dwc3_add_hcd(dwc);

>>         dwc3_del_hcd(dwc);

>

> Why do we need these new APIs? don't these suffice?

> 	dwc3_gadget_init(dwc);

> 	dwc3_gadget_exit(dwc);

> 	dwc3_host_init(dwc);

> 	dwc3_host_exit(dwc);


well, if they do what we want, sure. They suffice.

>> the semantics of these should be easy to understand and you can

>> implement each in their respective host.c/gadget.c files.

>> 

>> * Second step is to modify our dwc3_init_mode() (or whatever that

>>   function was called, sorry, didn't check) to make sure we have

>>   something like:

>> 

>> 	case OTG:

>>         	dwc3_add_udc(dwc);

>>                 break;

>> 

>> We should *not* add HCD in this case yet.

>> 

>> * After that we add otg.c (or drd.c, no preference) and make that call

>>   dwc3_add_udc(dwc) and, also, provide

>>   dwc3_add_otg(dwc)/dwc3_del_otg(dwc) calls. Then patch the switch

>>   statement above to:

>> 

>> 	case OTG:

>>         	dwc3_add_otg(dwc);

>>                 break;

>> 

>> Note that at this point, this is simply a direct replacement of

>> dwc3_add_udc() to dwc3_add_otg(). This should maintain current behavior

>> (which is starting with peripheral mode by default), but it should also

>> add support for OTG interrupts to change the mode (from an interrupt

>> thead)

>> 

>> 	otg_isr()

>>         {

>> 

>> 		/* don't forget to remove preivous mode if necessary */

>>         	if (perimode)

>>                 	dwc3_add_udc(dwc);

>>                 else

>>                 	dwc3_add_hcd(dwc);

>> 	}

>> 

>> * The next patch would be to choose default conditionally based on

>>   PERIMODE or whatever.

>> 

>> Of course, this is an oversimplified view of reality. You still need to

>> poke around at PRTCAPDIR, etc. But all this can, actually, be prototyped

>> using our "mode" debugfs file. Just make that call

>> dwc3_add/del_udc/hcd() apart from fiddling with PRTCAPDIR in GCTL.

>

> We also need to ensure that system suspend/resume doesn't break.

> Mainly if we suspend/resume with UDC removed.


right, why would it break in that case? I'm missing something...

>> Your first implementation could be just that. Refactoring what needs to

>> be refactored, then patching "mode" debugfs to work properly in that

>> case. Only add otg.c/drd.c after "mode" debugfs file is stable, because

>> then you know what needs to be taken into consideration.

>> 

>> Just to be clear, I'm not saying we should *ONLY* get the debugfs

>> interface for v4.12, I'm saying you should start with that and get that

>> stable and working properly (make an infinite loop constantly changing

>> modes and keep it running over the weekend) before you add support for

>> OTG interrupts, which could come in the same series ;-)

>> 

>

> Just to clarify debugfs mode behaviour.

>

> Currently it is just changing PRTCAPDIR. What we need to do is that if

> dr_mode == "otg", then we call dwc3_host/gadget_init/exit() accordingly as well.

>

> Does this make sense?


it does.

-- 
balbi

+Mathias

On 31/03/17 10:46, Felipe Balbi wrote:
> 

> Hi,

> 

> Roger Quadros <rogerq@ti.com> writes:

>>>>> Roger Quadros <rogerq@ti.com> writes:

>>>>>> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode

>>>>>> when we're operating in dual-role.

>>>>>

>>>>> yeah, that's not a quirk. DRA7 supports OTGv2, not OTGv3. There was no

>>>>> USB3 when OTGv2 was written.

>>>>>

>>>>> DRA7 just shouldn't use OTG core altogether. In fact, this is the very

>>>>> thing I've been saying for a long time. Make the simplest implementation

>>>>> possible. The dead simple, does-one-thing-only sort of implementation.

>>>>>

>>>>> All we need for Dual-Role (without OTG extras) is some input for ID and

>>>>> VBUS, then we add/remove HCD/UDC conditionally and set PRTCAPDIR.

>>>>>

>>>>

>>>> The catch is that on AM437x there is no way to get ID and VBUS events other

>>>> than the OTG controller so we have to rely on the OTG controller for that. :(

>>>

>>> okay, so AM437x can get OTG interrupts properly. That's fine. We can

>>> still do everything we need using code that's already existing in dwc3

>>> if we refactor it a bit and hook it up to the OTG IRQ handler.

>>>

>>> Here's what we do:

>>>

>>> * First we re-factor all necessary code around so the API for OTG/DRD

>>>   is resumed to calling:

>>>

>>> 	dwc3_add_udc(dwc);

>>>         dwc3_del_udc(dwc);

>>>         dwc3_add_hcd(dwc);

>>>         dwc3_del_hcd(dwc);

>>

>> Why do we need these new APIs? don't these suffice?

>> 	dwc3_gadget_init(dwc);

>> 	dwc3_gadget_exit(dwc);

>> 	dwc3_host_init(dwc);

>> 	dwc3_host_exit(dwc);

> 

> well, if they do what we want, sure. They suffice.

> 

>>> the semantics of these should be easy to understand and you can

>>> implement each in their respective host.c/gadget.c files.

>>>

>>> * Second step is to modify our dwc3_init_mode() (or whatever that

>>>   function was called, sorry, didn't check) to make sure we have

>>>   something like:

>>>

>>> 	case OTG:

>>>         	dwc3_add_udc(dwc);

>>>                 break;

>>>

>>> We should *not* add HCD in this case yet.

>>>

>>> * After that we add otg.c (or drd.c, no preference) and make that call

>>>   dwc3_add_udc(dwc) and, also, provide

>>>   dwc3_add_otg(dwc)/dwc3_del_otg(dwc) calls. Then patch the switch

>>>   statement above to:

>>>

>>> 	case OTG:

>>>         	dwc3_add_otg(dwc);

>>>                 break;

>>>

>>> Note that at this point, this is simply a direct replacement of

>>> dwc3_add_udc() to dwc3_add_otg(). This should maintain current behavior

>>> (which is starting with peripheral mode by default), but it should also

>>> add support for OTG interrupts to change the mode (from an interrupt

>>> thead)

>>>

>>> 	otg_isr()

>>>         {

>>>

>>> 		/* don't forget to remove preivous mode if necessary */

>>>         	if (perimode)

>>>                 	dwc3_add_udc(dwc);

>>>                 else

>>>                 	dwc3_add_hcd(dwc);

>>> 	}

>>>

>>> * The next patch would be to choose default conditionally based on

>>>   PERIMODE or whatever.

>>>

>>> Of course, this is an oversimplified view of reality. You still need to

>>> poke around at PRTCAPDIR, etc. But all this can, actually, be prototyped

>>> using our "mode" debugfs file. Just make that call

>>> dwc3_add/del_udc/hcd() apart from fiddling with PRTCAPDIR in GCTL.

>>

>> We also need to ensure that system suspend/resume doesn't break.

>> Mainly if we suspend/resume with UDC removed.

> 

> right, why would it break in that case? I'm missing something...

> 

>>> Your first implementation could be just that. Refactoring what needs to

>>> be refactored, then patching "mode" debugfs to work properly in that

>>> case. Only add otg.c/drd.c after "mode" debugfs file is stable, because

>>> then you know what needs to be taken into consideration.

>>>

>>> Just to be clear, I'm not saying we should *ONLY* get the debugfs

>>> interface for v4.12, I'm saying you should start with that and get that

>>> stable and working properly (make an infinite loop constantly changing

>>> modes and keep it running over the weekend) before you add support for

>>> OTG interrupts, which could come in the same series ;-)

>>>

>>

>> Just to clarify debugfs mode behaviour.

>>

>> Currently it is just changing PRTCAPDIR. What we need to do is that if

>> dr_mode == "otg", then we call dwc3_host/gadget_init/exit() accordingly as well.

>>

>> Does this make sense?

> 

> it does.

> 


OK. Below is a patch that allows us to use debugfs/mode to do the role switch.
Switching from device to host worked fine but I get the following error when
switching from host to device.

https://hastebin.com/liluqosewe.xml

cheers,
-roger

---
From 50c49f18474b388d10533eb9f6d04f454fabf687 Mon Sep 17 00:00:00 2001
From: Roger Quadros <rogerq@ti.com>

Date: Fri, 31 Mar 2017 12:54:13 +0300
Subject: [PATCH] usb: dwc3: make role-switching work with debugfs/mode

If dr_mode == "otg", we start by default in PERIPHERAL mode.
Keep track of current role in "current_dr_role" whenever dwc3_set_mode()
is called.

When debugfs/mode is changed AND we're in dual-role mode,
handle the switch by stopping and starting the respective
host/gadget controllers.

Signed-off-by: Roger Quadros <rogerq@ti.com>

---
 drivers/usb/dwc3/core.c    | 38 +++++++++++++++++++++++------------
 drivers/usb/dwc3/core.h    |  2 ++
 drivers/usb/dwc3/debugfs.c | 49 +++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 70 insertions(+), 19 deletions(-)

-- 
2.7.4diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 369bab1..e2d36ba 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -108,6 +108,8 @@ void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
 	reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));
 	reg |= DWC3_GCTL_PRTCAPDIR(mode);
 	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
+
+	dwc->current_dr_role = mode;
 }
 
 u32 dwc3_core_fifo_space(struct dwc3_ep *dep, u8 type)
@@ -862,13 +864,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
 		}
 		break;
 	case USB_DR_MODE_OTG:
-		ret = dwc3_host_init(dwc);
-		if (ret) {
-			if (ret != -EPROBE_DEFER)
-				dev_err(dev, "failed to initialize host\n");
-			return ret;
-		}
-
+		/* start in peripheral role by default */
+		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
 		ret = dwc3_gadget_init(dwc);
 		if (ret) {
 			if (ret != -EPROBE_DEFER)
@@ -894,8 +891,11 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc)
 		dwc3_host_exit(dwc);
 		break;
 	case USB_DR_MODE_OTG:
-		dwc3_host_exit(dwc);
-		dwc3_gadget_exit(dwc);
+		/* role might have changed since start */
+		if (dwc->current_dr_role ==  DWC3_GCTL_PRTCAP_DEVICE)
+			dwc3_gadget_exit(dwc);
+		else if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)
+			dwc3_host_exit(dwc);
 		break;
 	default:
 		/* do nothing */
@@ -1209,11 +1209,18 @@ static int dwc3_suspend_common(struct dwc3 *dwc)
 
 	switch (dwc->dr_mode) {
 	case USB_DR_MODE_PERIPHERAL:
-	case USB_DR_MODE_OTG:
 		spin_lock_irqsave(&dwc->lock, flags);
 		dwc3_gadget_suspend(dwc);
 		spin_unlock_irqrestore(&dwc->lock, flags);
 		break;
+	case USB_DR_MODE_OTG:
+		/* gadget might not be always present */
+		if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE) {
+			spin_lock_irqsave(&dwc->lock, flags);
+			dwc3_gadget_suspend(dwc);
+			spin_unlock_irqrestore(&dwc->lock, flags);
+		}
+		break;
 	case USB_DR_MODE_HOST:
 	default:
 		/* do nothing */
@@ -1236,11 +1243,18 @@ static int dwc3_resume_common(struct dwc3 *dwc)
 
 	switch (dwc->dr_mode) {
 	case USB_DR_MODE_PERIPHERAL:
-	case USB_DR_MODE_OTG:
 		spin_lock_irqsave(&dwc->lock, flags);
 		dwc3_gadget_resume(dwc);
 		spin_unlock_irqrestore(&dwc->lock, flags);
-		/* FALLTHROUGH */
+		break;
+	case USB_DR_MODE_OTG:
+		/* gadget might not be always present */
+		if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE) {
+			spin_lock_irqsave(&dwc->lock, flags);
+			dwc3_gadget_resume(dwc);
+			spin_unlock_irqrestore(&dwc->lock, flags);
+		}
+		break;
 	case USB_DR_MODE_HOST:
 	default:
 		/* do nothing */
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 7ffdee5..f45ff44 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -785,6 +785,7 @@ struct dwc3_scratchpad_array {
  * @maximum_speed: maximum speed requested (mainly for testing purposes)
  * @revision: revision register contents
  * @dr_mode: requested mode of operation
+ * @current_dr_role: current role of operation when in dual-role mode
  * @hsphy_mode: UTMI phy mode, one of following:
  *		- USBPHY_INTERFACE_MODE_UTMI
  *		- USBPHY_INTERFACE_MODE_UTMIW
@@ -901,6 +902,7 @@ struct dwc3 {
 	size_t			regs_size;
 
 	enum usb_dr_mode	dr_mode;
+	u32			current_dr_role;
 	enum usb_phy_interface	hsphy_mode;
 
 	u32			fladj;
diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
index 31926dd..a101b14 100644
--- a/drivers/usb/dwc3/debugfs.c
+++ b/drivers/usb/dwc3/debugfs.c
@@ -327,19 +327,54 @@ static ssize_t dwc3_mode_write(struct file *file,
 		return -EFAULT;
 
 	if (!strncmp(buf, "host", 4))
-		mode |= DWC3_GCTL_PRTCAP_HOST;
+		mode = DWC3_GCTL_PRTCAP_HOST;
 
 	if (!strncmp(buf, "device", 6))
-		mode |= DWC3_GCTL_PRTCAP_DEVICE;
+		mode = DWC3_GCTL_PRTCAP_DEVICE;
 
 	if (!strncmp(buf, "otg", 3))
-		mode |= DWC3_GCTL_PRTCAP_OTG;
+		mode = DWC3_GCTL_PRTCAP_OTG;
 
-	if (mode) {
-		spin_lock_irqsave(&dwc->lock, flags);
-		dwc3_set_mode(dwc, mode);
-		spin_unlock_irqrestore(&dwc->lock, flags);
+	if (!mode)
+		return -EINVAL;
+
+	if (mode == dwc->current_dr_role)
+		goto exit;
+
+	/* prevent role switching if we're not dual-role */
+	if (dwc->dr_mode != USB_DR_MODE_OTG)
+		return -EINVAL;
+
+	/* stop old role */
+	if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)
+	switch (dwc->current_dr_role) {
+	case DWC3_GCTL_PRTCAP_HOST:
+		dwc3_host_exit(dwc);
+		break;
+	case DWC3_GCTL_PRTCAP_DEVICE:
+		dwc3_gadget_exit(dwc);
+		break;
+	default:
+		break;
+	}
+
+	/* switch PRTCAP mode. updates current_dr_role */
+	spin_lock_irqsave(&dwc->lock, flags);
+	dwc3_set_mode(dwc, mode);
+	spin_unlock_irqrestore(&dwc->lock, flags);
+
+	/* start new role */
+	switch (dwc->current_dr_role) {
+	case DWC3_GCTL_PRTCAP_HOST:
+		dwc3_host_init(dwc);
+		break;
+	case DWC3_GCTL_PRTCAP_DEVICE:
+		dwc3_gadget_init(dwc);
+		break;
+	default:
+		break;
 	}
+exit:
 	return count;
 }
 

Hi,

Roger Quadros <rogerq@ti.com> writes:
>>>> Your first implementation could be just that. Refactoring what needs to

>>>> be refactored, then patching "mode" debugfs to work properly in that

>>>> case. Only add otg.c/drd.c after "mode" debugfs file is stable, because

>>>> then you know what needs to be taken into consideration.

>>>>

>>>> Just to be clear, I'm not saying we should *ONLY* get the debugfs

>>>> interface for v4.12, I'm saying you should start with that and get that

>>>> stable and working properly (make an infinite loop constantly changing

>>>> modes and keep it running over the weekend) before you add support for

>>>> OTG interrupts, which could come in the same series ;-)

>>>>

>>>

>>> Just to clarify debugfs mode behaviour.

>>>

>>> Currently it is just changing PRTCAPDIR. What we need to do is that if

>>> dr_mode == "otg", then we call dwc3_host/gadget_init/exit() accordingly as well.

>>>

>>> Does this make sense?

>> 

>> it does.

>> 

>

> OK. Below is a patch that allows us to use debugfs/mode to do the role switch.

> Switching from device to host worked fine but I get the following error when

> switching from host to device.

>

> https://hastebin.com/liluqosewe.xml

>

> cheers,

> -roger

>

> ---

> From 50c49f18474b388d10533eb9f6d04f454fabf687 Mon Sep 17 00:00:00 2001

> From: Roger Quadros <rogerq@ti.com>

> Date: Fri, 31 Mar 2017 12:54:13 +0300

> Subject: [PATCH] usb: dwc3: make role-switching work with debugfs/mode

>

> If dr_mode == "otg", we start by default in PERIPHERAL mode.

> Keep track of current role in "current_dr_role" whenever dwc3_set_mode()

> is called.

>

> When debugfs/mode is changed AND we're in dual-role mode,

> handle the switch by stopping and starting the respective

> host/gadget controllers.

>

> Signed-off-by: Roger Quadros <rogerq@ti.com>


I'm assuming you also plan on breaking this down further ;-)

> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c

> index 369bab1..e2d36ba 100644

> --- a/drivers/usb/dwc3/core.c

> +++ b/drivers/usb/dwc3/core.c

> @@ -108,6 +108,8 @@ void dwc3_set_mode(struct dwc3 *dwc, u32 mode)

>  	reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));

>  	reg |= DWC3_GCTL_PRTCAPDIR(mode);

>  	dwc3_writel(dwc->regs, DWC3_GCTL, reg);

> +

> +	dwc->current_dr_role = mode;

>  }

>  

>  u32 dwc3_core_fifo_space(struct dwc3_ep *dep, u8 type)

> @@ -862,13 +864,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)

>  		}

>  		break;

>  	case USB_DR_MODE_OTG:

> -		ret = dwc3_host_init(dwc);

> -		if (ret) {

> -			if (ret != -EPROBE_DEFER)

> -				dev_err(dev, "failed to initialize host\n");

> -			return ret;

> -		}

> -

> +		/* start in peripheral role by default */

> +		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);

>  		ret = dwc3_gadget_init(dwc);

>  		if (ret) {

>  			if (ret != -EPROBE_DEFER)

> @@ -894,8 +891,11 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc)

>  		dwc3_host_exit(dwc);

>  		break;

>  	case USB_DR_MODE_OTG:

> -		dwc3_host_exit(dwc);

> -		dwc3_gadget_exit(dwc);

> +		/* role might have changed since start */

> +		if (dwc->current_dr_role ==  DWC3_GCTL_PRTCAP_DEVICE)

> +			dwc3_gadget_exit(dwc);

> +		else if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)

> +			dwc3_host_exit(dwc);


how about patching the respective exit/init functions with something
like:

if (dwc->current_dr_role != $my_expected_role)
	return 0;

then you can call them without any checks.

> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c

> index 31926dd..a101b14 100644

> --- a/drivers/usb/dwc3/debugfs.c

> +++ b/drivers/usb/dwc3/debugfs.c

> @@ -327,19 +327,54 @@ static ssize_t dwc3_mode_write(struct file *file,

>  		return -EFAULT;

>  

>  	if (!strncmp(buf, "host", 4))

> -		mode |= DWC3_GCTL_PRTCAP_HOST;

> +		mode = DWC3_GCTL_PRTCAP_HOST;

>  

>  	if (!strncmp(buf, "device", 6))

> -		mode |= DWC3_GCTL_PRTCAP_DEVICE;

> +		mode = DWC3_GCTL_PRTCAP_DEVICE;

>  

>  	if (!strncmp(buf, "otg", 3))

> -		mode |= DWC3_GCTL_PRTCAP_OTG;

> +		mode = DWC3_GCTL_PRTCAP_OTG;

>  

> -	if (mode) {

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

> -		dwc3_set_mode(dwc, mode);

> -		spin_unlock_irqrestore(&dwc->lock, flags);

> +	if (!mode)

> +		return -EINVAL;

> +

> +	if (mode == dwc->current_dr_role)

> +		goto exit;

> +

> +	/* prevent role switching if we're not dual-role */

> +	if (dwc->dr_mode != USB_DR_MODE_OTG)

> +		return -EINVAL;

> +

> +	/* stop old role */

> +	if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)


is this your bug? This switch statement only executes when we're in host
mode. This means that when you switch to peripheral, you don't exit
host. Then when you switch back from peripheral to host, you're going to
add the same platform_device again. We're going to have TWO xHCI
platform device with the exact same name. When you finally switch again
from host to device, then you have issues.

Can you confirm?

> +	switch (dwc->current_dr_role) {

> +	case DWC3_GCTL_PRTCAP_HOST:

> +		dwc3_host_exit(dwc);

> +		break;

> +	case DWC3_GCTL_PRTCAP_DEVICE:

> +		dwc3_gadget_exit(dwc);

> +		break;

> +	default:

> +		break;

> +	}

> +

> +	/* switch PRTCAP mode. updates current_dr_role */

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

> +	dwc3_set_mode(dwc, mode);

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

> +

> +	/* start new role */

> +	switch (dwc->current_dr_role) {

> +	case DWC3_GCTL_PRTCAP_HOST:

> +		dwc3_host_init(dwc);

> +		break;

> +	case DWC3_GCTL_PRTCAP_DEVICE:

> +		dwc3_gadget_init(dwc);

> +		break;

> +	default:

> +		break;

>  	}

> +exit:

>  	return count;

>  }

>  

> -- 

> 2.7.4

>

> --

> To unsubscribe from this list: send the line "unsubscribe linux-usb" in

> the body of a message to majordomo@vger.kernel.org

> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
balbi

On 31/03/17 15:00, Felipe Balbi wrote:
> 

> Hi,

> 

> Roger Quadros <rogerq@ti.com> writes:

>>>>> Your first implementation could be just that. Refactoring what needs to

>>>>> be refactored, then patching "mode" debugfs to work properly in that

>>>>> case. Only add otg.c/drd.c after "mode" debugfs file is stable, because

>>>>> then you know what needs to be taken into consideration.

>>>>>

>>>>> Just to be clear, I'm not saying we should *ONLY* get the debugfs

>>>>> interface for v4.12, I'm saying you should start with that and get that

>>>>> stable and working properly (make an infinite loop constantly changing

>>>>> modes and keep it running over the weekend) before you add support for

>>>>> OTG interrupts, which could come in the same series ;-)

>>>>>

>>>>

>>>> Just to clarify debugfs mode behaviour.

>>>>

>>>> Currently it is just changing PRTCAPDIR. What we need to do is that if

>>>> dr_mode == "otg", then we call dwc3_host/gadget_init/exit() accordingly as well.

>>>>

>>>> Does this make sense?

>>>

>>> it does.

>>>

>>

>> OK. Below is a patch that allows us to use debugfs/mode to do the role switch.

>> Switching from device to host worked fine but I get the following error when

>> switching from host to device.

>>

>> https://hastebin.com/liluqosewe.xml

>>

>> cheers,

>> -roger

>>

>> ---

>> From 50c49f18474b388d10533eb9f6d04f454fabf687 Mon Sep 17 00:00:00 2001

>> From: Roger Quadros <rogerq@ti.com>

>> Date: Fri, 31 Mar 2017 12:54:13 +0300

>> Subject: [PATCH] usb: dwc3: make role-switching work with debugfs/mode

>>

>> If dr_mode == "otg", we start by default in PERIPHERAL mode.

>> Keep track of current role in "current_dr_role" whenever dwc3_set_mode()

>> is called.

>>

>> When debugfs/mode is changed AND we're in dual-role mode,

>> handle the switch by stopping and starting the respective

>> host/gadget controllers.

>>

>> Signed-off-by: Roger Quadros <rogerq@ti.com>

> 

> I'm assuming you also plan on breaking this down further ;-)


Did you mean I must split this patch into smaller ones?

> 

>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c

>> index 369bab1..e2d36ba 100644

>> --- a/drivers/usb/dwc3/core.c

>> +++ b/drivers/usb/dwc3/core.c

>> @@ -108,6 +108,8 @@ void dwc3_set_mode(struct dwc3 *dwc, u32 mode)

>>  	reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));

>>  	reg |= DWC3_GCTL_PRTCAPDIR(mode);

>>  	dwc3_writel(dwc->regs, DWC3_GCTL, reg);

>> +

>> +	dwc->current_dr_role = mode;

>>  }

>>  

>>  u32 dwc3_core_fifo_space(struct dwc3_ep *dep, u8 type)

>> @@ -862,13 +864,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)

>>  		}

>>  		break;

>>  	case USB_DR_MODE_OTG:

>> -		ret = dwc3_host_init(dwc);

>> -		if (ret) {

>> -			if (ret != -EPROBE_DEFER)

>> -				dev_err(dev, "failed to initialize host\n");

>> -			return ret;

>> -		}

>> -

>> +		/* start in peripheral role by default */

>> +		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);

>>  		ret = dwc3_gadget_init(dwc);

>>  		if (ret) {

>>  			if (ret != -EPROBE_DEFER)

>> @@ -894,8 +891,11 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc)

>>  		dwc3_host_exit(dwc);

>>  		break;

>>  	case USB_DR_MODE_OTG:

>> -		dwc3_host_exit(dwc);

>> -		dwc3_gadget_exit(dwc);

>> +		/* role might have changed since start */

>> +		if (dwc->current_dr_role ==  DWC3_GCTL_PRTCAP_DEVICE)

>> +			dwc3_gadget_exit(dwc);

>> +		else if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)

>> +			dwc3_host_exit(dwc);

> 

> how about patching the respective exit/init functions with something

> like:

> 

> if (dwc->current_dr_role != $my_expected_role)

> 	return 0;

> 

> then you can call them without any checks.


OK.

> 

>> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c

>> index 31926dd..a101b14 100644

>> --- a/drivers/usb/dwc3/debugfs.c

>> +++ b/drivers/usb/dwc3/debugfs.c

>> @@ -327,19 +327,54 @@ static ssize_t dwc3_mode_write(struct file *file,

>>  		return -EFAULT;

>>  

>>  	if (!strncmp(buf, "host", 4))

>> -		mode |= DWC3_GCTL_PRTCAP_HOST;

>> +		mode = DWC3_GCTL_PRTCAP_HOST;

>>  

>>  	if (!strncmp(buf, "device", 6))

>> -		mode |= DWC3_GCTL_PRTCAP_DEVICE;

>> +		mode = DWC3_GCTL_PRTCAP_DEVICE;

>>  

>>  	if (!strncmp(buf, "otg", 3))

>> -		mode |= DWC3_GCTL_PRTCAP_OTG;

>> +		mode = DWC3_GCTL_PRTCAP_OTG;

>>  

>> -	if (mode) {

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

>> -		dwc3_set_mode(dwc, mode);

>> -		spin_unlock_irqrestore(&dwc->lock, flags);

>> +	if (!mode)

>> +		return -EINVAL;

>> +

>> +	if (mode == dwc->current_dr_role)

>> +		goto exit;

>> +

>> +	/* prevent role switching if we're not dual-role */

>> +	if (dwc->dr_mode != USB_DR_MODE_OTG)

>> +		return -EINVAL;

>> +

>> +	/* stop old role */

>> +	if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)

> 

> is this your bug? This switch statement only executes when we're in host

> mode. This means that when you switch to peripheral, you don't exit

> host. Then when you switch back from peripheral to host, you're going to

> add the same platform_device again. We're going to have TWO xHCI

> platform device with the exact same name. When you finally switch again

> from host to device, then you have issues.

> 

> Can you confirm?


That was a bug but I still see the issue although only when a mass storage
device was plugged in.

I see this other new issue when not using a mass storage device.

root@rockdesk:/sys/kernel/debug/48890000.usb# echo device > mode
[  218.226104] xhci-hcd xhci-hcd.1.auto: remove, state 4
[  218.231822] usb usb4: USB disconnect, device number 1
[  218.246973] xhci-hcd xhci-hcd.1.auto: USB bus 4 deregistered
[  218.252961] xhci-hcd xhci-hcd.1.auto: remove, state 4
[  218.258347] usb usb3: USB disconnect, device number 1
[  218.265858] xhci-hcd xhci-hcd.1.auto: USB bus 3 deregistered
[  218.274312] dwc3 48890000.usb: changing max_speed on rev 5533202a
[  218.282108] kobject (ed120208): tried to init an initialized object, something is seriously wrong.
[  218.291553] CPU: 1 PID: 2025 Comm: bash Not tainted 4.11.0-rc4-00004-g559b2c9 #1285
[  218.299590] Hardware name: Generic DRA74X (Flattened Device Tree)
[  218.306002] [<c01101ec>] (unwind_backtrace) from [<c010c328>] (show_stack+0x10/0x14)
[  218.314133] [<c010c328>] (show_stack) from [<c04b46b8>] (dump_stack+0xac/0xe0)
[  218.321716] [<c04b46b8>] (dump_stack) from [<c04b5dc8>] (kobject_init+0x78/0x94)
[  218.329484] [<c04b5dc8>] (kobject_init) from [<c0570c98>] (device_initialize+0x20/0xe4)
[  218.337891] [<c0570c98>] (device_initialize) from [<c0573280>] (device_register+0xc/0x18)
[  218.346502] [<c0573280>] (device_register) from [<bf298ce8>] (usb_add_gadget_udc_release+0x88/0x1e8 [udc_core])
[  218.357153] [<bf298ce8>] (usb_add_gadget_udc_release [udc_core]) from [<bf2c6d68>] (dwc3_gadget_init+0x278/0x6c0 [dwc3])
[  218.368603] [<bf2c6d68>] (dwc3_gadget_init [dwc3]) from [<bf2cab54>] (dwc3_mode_write+0x178/0x1c4 [dwc3])
[  218.378668] [<bf2cab54>] (dwc3_mode_write [dwc3]) from [<c04557c4>] (full_proxy_write+0x4c/0x64)
[  218.387897] [<c04557c4>] (full_proxy_write) from [<c02b29d8>] (__vfs_write+0x1c/0x114)
[  218.396206] [<c02b29d8>] (__vfs_write) from [<c02b421c>] (vfs_write+0xa0/0x168)
[  218.403877] [<c02b421c>] (vfs_write) from [<c02b50b4>] (SyS_write+0x44/0x9c)
[  218.411276] [<c02b50b4>] (SyS_write) from [<c0107880>] (ret_fast_syscall+0x0/0x1c)
[  218.419347] ------------[ cut here ]------------
[  218.424208] WARNING: CPU: 1 PID: 2025 at lib/kobject.c:597 kobject_get+0x48/0x58
[  218.432018] kobject: '(null)' (ed379018): is not initialized, yet kobject_get() is being called.
[  218.441272] Modules linked in: usb_f_ss_lb g_zero libcomposite xhci_plat_hcd xhci_hcd usbcore dwc3 udc_core evdev snd_soc_davinci_mcasp usb_common snd_soc_edma snd_soc_simple_card m25p80 snd_soc_tlv320aic3x e
[  218.488436] CPU: 1 PID: 2025 Comm: bash Not tainted 4.11.0-rc4-00004-g559b2c9 #1285
[  218.496470] Hardware name: Generic DRA74X (Flattened Device Tree)
[  218.502870] [<c01101ec>] (unwind_backtrace) from [<c010c328>] (show_stack+0x10/0x14)
[  218.510996] [<c010c328>] (show_stack) from [<c04b46b8>] (dump_stack+0xac/0xe0)
[  218.518579] [<c04b46b8>] (dump_stack) from [<c0136cf0>] (__warn+0xd8/0x104)
[  218.525895] [<c0136cf0>] (__warn) from [<c0136d50>] (warn_slowpath_fmt+0x34/0x44)
[  218.533756] [<c0136d50>] (warn_slowpath_fmt) from [<c04b5e38>] (kobject_get+0x48/0x58)
[  218.542065] [<c04b5e38>] (kobject_get) from [<c0800ef0>] (klist_add_tail+0x18/0x44)
[  218.550114] [<c0800ef0>] (klist_add_tail) from [<c05730dc>] (device_add+0x3d8/0x570)
[  218.558258] [<c05730dc>] (device_add) from [<bf298ce8>] (usb_add_gadget_udc_release+0x88/0x1e8 [udc_core])
[  218.568428] [<bf298ce8>] (usb_add_gadget_udc_release [udc_core]) from [<bf2c6d68>] (dwc3_gadget_init+0x278/0x6c0 [dwc3])
[  218.579872] [<bf2c6d68>] (dwc3_gadget_init [dwc3]) from [<bf2cab54>] (dwc3_mode_write+0x178/0x1c4 [dwc3])
[  218.589939] [<bf2cab54>] (dwc3_mode_write [dwc3]) from [<c04557c4>] (full_proxy_write+0x4c/0x64)
[  218.599165] [<c04557c4>] (full_proxy_write) from [<c02b29d8>] (__vfs_write+0x1c/0x114)
[  218.607480] [<c02b29d8>] (__vfs_write) from [<c02b421c>] (vfs_write+0xa0/0x168)
[  218.615147] [<c02b421c>] (vfs_write) from [<c02b50b4>] (SyS_write+0x44/0x9c)
[  218.622549] [<c02b50b4>] (SyS_write) from [<c0107880>] (ret_fast_syscall+0x0/0x1c)
[  218.630543] ---[ end trace 9b9aa5ff9aaa9cf9 ]---
[  218.635433] ------------[ cut here ]------------
[  218.640321] WARNING: CPU: 1 PID: 2025 at lib/refcount.c:114 kobject_get+0x24/0x58
[  218.648232] refcount_t: increment on 0; use-after-free.
[  218.653718] Modules linked in: usb_f_ss_lb g_zero libcomposite xhci_plat_hcd xhci_hcd usbcore dwc3 udc_core evdev snd_soc_davinci_mcasp usb_common snd_soc_edma snd_soc_simple_card m25p80 snd_soc_tlv320aic3x e
[  218.700873] CPU: 1 PID: 2025 Comm: bash Tainted: G        W       4.11.0-rc4-00004-g559b2c9 #1285
[  218.710180] Hardware name: Generic DRA74X (Flattened Device Tree)
[  218.716583] [<c01101ec>] (unwind_backtrace) from [<c010c328>] (show_stack+0x10/0x14)
[  218.724710] [<c010c328>] (show_stack) from [<c04b46b8>] (dump_stack+0xac/0xe0)
[  218.732296] [<c04b46b8>] (dump_stack) from [<c0136cf0>] (__warn+0xd8/0x104)
[  218.739605] [<c0136cf0>] (__warn) from [<c0136d50>] (warn_slowpath_fmt+0x34/0x44)
[  218.747467] [<c0136d50>] (warn_slowpath_fmt) from [<c04b5e14>] (kobject_get+0x24/0x58)
[  218.755778] [<c04b5e14>] (kobject_get) from [<c0800ef0>] (klist_add_tail+0x18/0x44)
[  218.763820] [<c0800ef0>] (klist_add_tail) from [<c05730dc>] (device_add+0x3d8/0x570)
[  218.771963] [<c05730dc>] (device_add) from [<bf298ce8>] (usb_add_gadget_udc_release+0x88/0x1e8 [udc_core])
[  218.782128] [<bf298ce8>] (usb_add_gadget_udc_release [udc_core]) from [<bf2c6d68>] (dwc3_gadget_init+0x278/0x6c0 [dwc3])
[  218.793573] [<bf2c6d68>] (dwc3_gadget_init [dwc3]) from [<bf2cab54>] (dwc3_mode_write+0x178/0x1c4 [dwc3])
[  218.803634] [<bf2cab54>] (dwc3_mode_write [dwc3]) from [<c04557c4>] (full_proxy_write+0x4c/0x64)
[  218.812862] [<c04557c4>] (full_proxy_write) from [<c02b29d8>] (__vfs_write+0x1c/0x114)
[  218.821166] [<c02b29d8>] (__vfs_write) from [<c02b421c>] (vfs_write+0xa0/0x168)
[  218.828848] [<c02b421c>] (vfs_write) from [<c02b50b4>] (SyS_write+0x44/0x9c)
[  218.836251] [<c02b50b4>] (SyS_write) from [<c0107880>] (ret_fast_syscall+0x0/0x1c)
[  218.844240] ---[ end trace 9b9aa5ff9aaa9cfa ]---
[  218.850118] zero gadget: Gadget Zero, version: Cinco de Mayo 2008
[  218.856622] zero gadget: zero ready
[  218.861206] omap_l3_noc 44000000.ocp: L3 application error: target 5 mod:1 (unclearable)
[  218.869779] omap_l3_noc 44000000.ocp: L3 debug error: target 5 mod:1 (unclearable)

> 

>> +	switch (dwc->current_dr_role) {

>> +	case DWC3_GCTL_PRTCAP_HOST:

>> +		dwc3_host_exit(dwc);

>> +		break;

>> +	case DWC3_GCTL_PRTCAP_DEVICE:

>> +		dwc3_gadget_exit(dwc);

>> +		break;

>> +	default:

>> +		break;

>> +	}

>> +

>> +	/* switch PRTCAP mode. updates current_dr_role */

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

>> +	dwc3_set_mode(dwc, mode);

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

>> +

>> +	/* start new role */

>> +	switch (dwc->current_dr_role) {

>> +	case DWC3_GCTL_PRTCAP_HOST:

>> +		dwc3_host_init(dwc);

>> +		break;

>> +	case DWC3_GCTL_PRTCAP_DEVICE:

>> +		dwc3_gadget_init(dwc);

>> +		break;

>> +	default:

>> +		break;

>>  	}

>> +exit:

>>  	return count;

>>  }

>>  

>> -- 

>> 2.7.4

>>

>> --

>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in

>> the body of a message to majordomo@vger.kernel.org

>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

> 


cheers,
-roger

Hi,

Roger Quadros <rogerq@ti.com> writes:
> On 31/03/17 15:00, Felipe Balbi wrote:

>> 

>> Hi,

>> 

>> Roger Quadros <rogerq@ti.com> writes:

>>>>>> Your first implementation could be just that. Refactoring what needs to

>>>>>> be refactored, then patching "mode" debugfs to work properly in that

>>>>>> case. Only add otg.c/drd.c after "mode" debugfs file is stable, because

>>>>>> then you know what needs to be taken into consideration.

>>>>>>

>>>>>> Just to be clear, I'm not saying we should *ONLY* get the debugfs

>>>>>> interface for v4.12, I'm saying you should start with that and get that

>>>>>> stable and working properly (make an infinite loop constantly changing

>>>>>> modes and keep it running over the weekend) before you add support for

>>>>>> OTG interrupts, which could come in the same series ;-)

>>>>>>

>>>>>

>>>>> Just to clarify debugfs mode behaviour.

>>>>>

>>>>> Currently it is just changing PRTCAPDIR. What we need to do is that if

>>>>> dr_mode == "otg", then we call dwc3_host/gadget_init/exit() accordingly as well.

>>>>>

>>>>> Does this make sense?

>>>>

>>>> it does.

>>>>

>>>

>>> OK. Below is a patch that allows us to use debugfs/mode to do the role switch.

>>> Switching from device to host worked fine but I get the following error when

>>> switching from host to device.

>>>

>>> https://hastebin.com/liluqosewe.xml

>>>

>>> cheers,

>>> -roger

>>>

>>> ---

>>> From 50c49f18474b388d10533eb9f6d04f454fabf687 Mon Sep 17 00:00:00 2001

>>> From: Roger Quadros <rogerq@ti.com>

>>> Date: Fri, 31 Mar 2017 12:54:13 +0300

>>> Subject: [PATCH] usb: dwc3: make role-switching work with debugfs/mode

>>>

>>> If dr_mode == "otg", we start by default in PERIPHERAL mode.

>>> Keep track of current role in "current_dr_role" whenever dwc3_set_mode()

>>> is called.

>>>

>>> When debugfs/mode is changed AND we're in dual-role mode,

>>> handle the switch by stopping and starting the respective

>>> host/gadget controllers.

>>>

>>> Signed-off-by: Roger Quadros <rogerq@ti.com>

>> 

>> I'm assuming you also plan on breaking this down further ;-)

>

> Did you mean I must split this patch into smaller ones?

>

>> 

>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c

>>> index 369bab1..e2d36ba 100644

>>> --- a/drivers/usb/dwc3/core.c

>>> +++ b/drivers/usb/dwc3/core.c

>>> @@ -108,6 +108,8 @@ void dwc3_set_mode(struct dwc3 *dwc, u32 mode)

>>>  	reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));

>>>  	reg |= DWC3_GCTL_PRTCAPDIR(mode);

>>>  	dwc3_writel(dwc->regs, DWC3_GCTL, reg);

>>> +

>>> +	dwc->current_dr_role = mode;

>>>  }

>>>  

>>>  u32 dwc3_core_fifo_space(struct dwc3_ep *dep, u8 type)

>>> @@ -862,13 +864,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)

>>>  		}

>>>  		break;

>>>  	case USB_DR_MODE_OTG:

>>> -		ret = dwc3_host_init(dwc);

>>> -		if (ret) {

>>> -			if (ret != -EPROBE_DEFER)

>>> -				dev_err(dev, "failed to initialize host\n");

>>> -			return ret;

>>> -		}

>>> -

>>> +		/* start in peripheral role by default */

>>> +		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);

>>>  		ret = dwc3_gadget_init(dwc);

>>>  		if (ret) {

>>>  			if (ret != -EPROBE_DEFER)

>>> @@ -894,8 +891,11 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc)

>>>  		dwc3_host_exit(dwc);

>>>  		break;

>>>  	case USB_DR_MODE_OTG:

>>> -		dwc3_host_exit(dwc);

>>> -		dwc3_gadget_exit(dwc);

>>> +		/* role might have changed since start */

>>> +		if (dwc->current_dr_role ==  DWC3_GCTL_PRTCAP_DEVICE)

>>> +			dwc3_gadget_exit(dwc);

>>> +		else if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)

>>> +			dwc3_host_exit(dwc);

>> 

>> how about patching the respective exit/init functions with something

>> like:

>> 

>> if (dwc->current_dr_role != $my_expected_role)

>> 	return 0;

>> 

>> then you can call them without any checks.

>

> OK.

>

>> 

>>> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c

>>> index 31926dd..a101b14 100644

>>> --- a/drivers/usb/dwc3/debugfs.c

>>> +++ b/drivers/usb/dwc3/debugfs.c

>>> @@ -327,19 +327,54 @@ static ssize_t dwc3_mode_write(struct file *file,

>>>  		return -EFAULT;

>>>  

>>>  	if (!strncmp(buf, "host", 4))

>>> -		mode |= DWC3_GCTL_PRTCAP_HOST;

>>> +		mode = DWC3_GCTL_PRTCAP_HOST;

>>>  

>>>  	if (!strncmp(buf, "device", 6))

>>> -		mode |= DWC3_GCTL_PRTCAP_DEVICE;

>>> +		mode = DWC3_GCTL_PRTCAP_DEVICE;

>>>  

>>>  	if (!strncmp(buf, "otg", 3))

>>> -		mode |= DWC3_GCTL_PRTCAP_OTG;

>>> +		mode = DWC3_GCTL_PRTCAP_OTG;

>>>  

>>> -	if (mode) {

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

>>> -		dwc3_set_mode(dwc, mode);

>>> -		spin_unlock_irqrestore(&dwc->lock, flags);

>>> +	if (!mode)

>>> +		return -EINVAL;

>>> +

>>> +	if (mode == dwc->current_dr_role)

>>> +		goto exit;

>>> +

>>> +	/* prevent role switching if we're not dual-role */

>>> +	if (dwc->dr_mode != USB_DR_MODE_OTG)

>>> +		return -EINVAL;

>>> +

>>> +	/* stop old role */

>>> +	if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)

>> 

>> is this your bug? This switch statement only executes when we're in host

>> mode. This means that when you switch to peripheral, you don't exit

>> host. Then when you switch back from peripheral to host, you're going to

>> add the same platform_device again. We're going to have TWO xHCI

>> platform device with the exact same name. When you finally switch again

>> from host to device, then you have issues.

>> 

>> Can you confirm?

>

> That was a bug but I still see the issue although only when a mass storage

> device was plugged in.

>

> I see this other new issue when not using a mass storage device.

>

> root@rockdesk:/sys/kernel/debug/48890000.usb# echo device > mode

> [  218.226104] xhci-hcd xhci-hcd.1.auto: remove, state 4

> [  218.231822] usb usb4: USB disconnect, device number 1

> [  218.246973] xhci-hcd xhci-hcd.1.auto: USB bus 4 deregistered

> [  218.252961] xhci-hcd xhci-hcd.1.auto: remove, state 4

> [  218.258347] usb usb3: USB disconnect, device number 1

> [  218.265858] xhci-hcd xhci-hcd.1.auto: USB bus 3 deregistered

> [  218.274312] dwc3 48890000.usb: changing max_speed on rev 5533202a

> [  218.282108] kobject (ed120208): tried to init an initialized object, something is seriously wrong.


kobj->state_initialized is left set. Need to find a clean way to clear
it. As a quick test, you could memset gadget->dev to zero from usb_del_gadget_udc()

-- 
balbi