From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932993AbdCaLvp (ORCPT ); Fri, 31 Mar 2017 07:51:45 -0400 Received: from fllnx209.ext.ti.com ([198.47.19.16]:51816 "EHLO fllnx209.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932709AbdCaLvn (ORCPT ); Fri, 31 Mar 2017 07:51:43 -0400 Subject: Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode To: Felipe Balbi References: <1487250377-13653-1-git-send-email-rogerq@ti.com> <1487250377-13653-5-git-send-email-rogerq@ti.com> <87o9wlhd1j.fsf@linux.intel.com> <87d1d0l6f4.fsf@linux.intel.com> <8f7c0e29-2313-5d0a-ce5c-2c729c5f2b9d@ti.com> <87zig1j3bg.fsf@linux.intel.com> CC: , , , Mathias Nyman From: Roger Quadros Message-ID: Date: Fri, 31 Mar 2017 14:50:49 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <87zig1j3bg.fsf@linux.intel.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org +Mathias On 31/03/17 10:46, Felipe Balbi wrote: > > Hi, > > Roger Quadros writes: >>>>> Roger Quadros 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 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 --- 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