From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933291AbdCaM7X (ORCPT ); Fri, 31 Mar 2017 08:59:23 -0400 Received: from mga02.intel.com ([134.134.136.20]:7220 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933094AbdCaM6g (ORCPT ); Fri, 31 Mar 2017 08:58:36 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,251,1486454400"; d="scan'208";a="82697918" From: Felipe Balbi To: Roger Quadros Cc: vivek.gautam@codeaurora.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, Mathias Nyman Subject: Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode In-Reply-To: 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> <87tw69irlf.fsf@linux.intel.com> Date: Fri, 31 Mar 2017 15:58:28 +0300 Message-ID: <87r31diowb.fsf@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, Roger Quadros writes: > On 31/03/17 15:00, Felipe Balbi wrote: >> >> Hi, >> >> Roger Quadros 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 >>> 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 >> >> 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