Hi, Roger Quadros <rogerq@xxxxxx> writes: > On 31/03/17 15:00, Felipe Balbi wrote: >> >> Hi, >> >> Roger Quadros <rogerq@xxxxxx> 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@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> >> >> 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 -- 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
