Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

[Felipe Balbi <balbi@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5926 bytes --]


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 ;-)

> 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.

> 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?

> +	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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]