lkml.org 
[lkml]   [2017]   [Mar]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode
Date

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

\
 
 \ /
  Last update: 2017-03-31 15:03    [W:0.058 / U:1.276 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site