usb: dwc2: regression during boot on Raspberry Pi

[John Youn]

On 11/8/2015 2:13 AM, Stefan Wahren wrote:
> Hi Stephen,
> 
> Am 08.11.2015 um 06:06 schrieb Stephen Warren:
>> On 11/07/2015 05:16 PM, Stefan Wahren wrote:
>>> Hi,
>>> [...]
>>>    * phys (optional)
>>>    * phy-names (optional)
>>>
>>> So here are my questions:
>>>
>>> How to fix the kernel oops in dwc2 driver?
>>
>> Do you know exactly what causes the crash; you've bisected to the
>> specific commit, but I don't think mentioned why that commit causes an
>> issue.
> 
> The stacktrace in my initial mail shows a invalid paging request on 
> hsotg->regs in dwc2_is_controller_alive() which is called by 
> dwc2_handle_common_intr(). I must clarify the problem description. It's 
> reproducable on Raspberry Pi since 09a75e857790, but the real problem 
> seems to has been in the driver before. Looking at dwc2_driver_probe() 
> that the irq responsable for dwc2_handle_common_intr is requested BEFORE 
> hsotg->regs, hsotg->dr_mode and hsotg->lock are initialized.
> 
> I attached a patch which avoids this race by moving the request behind 
> dwc2_lowlevel_hw_init(). Not sure about that, but the oops disappear.
> 
>>
>>> Should we specify dr_mode = "host" for all Raspberry Pi variants in DT?
>>
>> It's optional so the driver must work without it. However, that value
>> seems appropriate, so you could certainly add it.
> 
> That's not the first platform, which shows me that dr_mode is only 
> optional for the case that the controller is really used for USB OTG. 
> AFAIK the OTG ID pin of the Raspberry PI A and B are tied to GND. But 
> the dwc2 driver doesn't know about that.
> 
>>
>>> Which clock should be referenced by USB host in DT?
>>>
>>> Do we need a USB PHY DT node and driver for bcm2835?
>>
>> I don't think there's a separate PHY module in bcm2835 is there?
>> Certainly there is no documentation, binding, or driver for it that I'm
>> aware of. I haven't checked the RPi Foundation downstream kernel though.
>>
> 
> I took a little deeper look into dwc2. According to 
> dwc2_lowlevel_hw_init() the USB PHY is required and clk is optional. I'm 
> confused :-(
> 
> Here is the draft for avoiding the oops:
> 
> --------------------------->8--------------------------------------------
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index 5859b0f..0e80087 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -341,20 +341,6 @@ static int dwc2_driver_probe(struct platform_device 
> *dev)
>   	if (retval)
>   		return retval;
> 
> -	irq = platform_get_irq(dev, 0);
> -	if (irq < 0) {
> -		dev_err(&dev->dev, "missing IRQ resource\n");
> -		return irq;
> -	}
> -
> -	dev_dbg(hsotg->dev, "registering common handler for irq%d\n",
> -		irq);
> -	retval = devm_request_irq(hsotg->dev, irq,
> -				  dwc2_handle_common_intr, IRQF_SHARED,
> -				  dev_name(hsotg->dev), hsotg);
> -	if (retval)
> -		return retval;
> -
>   	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
>   	hsotg->regs = devm_ioremap_resource(&dev->dev, res);
>   	if (IS_ERR(hsotg->regs))
> @@ -389,6 +375,20 @@ static int dwc2_driver_probe(struct platform_device 
> *dev)
> 
>   	dwc2_set_all_params(hsotg->core_params, -1);
> 
> +	irq = platform_get_irq(dev, 0);
> +	if (irq < 0) {
> +		dev_err(&dev->dev, "missing IRQ resource\n");
> +		return irq;
> +	}
> +
> +	dev_dbg(hsotg->dev, "registering common handler for irq%d\n",
> +		irq);
> +	retval = devm_request_irq(hsotg->dev, irq,
> +				  dwc2_handle_common_intr, IRQF_SHARED,
> +				  dev_name(hsotg->dev), hsotg);
> +	if (retval)
> +		return retval;
> +
>   	retval = dwc2_lowlevel_hw_enable(hsotg);
>   	if (retval)
>   		return retval;
> 
> 

Looks good to me. Could you submit a patch for this?

Regards,
John