+Mathias
On 31/03/17 10:46, Felipe Balbi wrote:
>
> Hi,
>
> Roger Quadros <rogerq@xxxxxx> writes:
>>>>> Roger Quadros <rogerq@xxxxxx> 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@xxxxxx>
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@xxxxxx>
---
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(-)
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);
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;
}
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
