usb: dwc2: regression during boot on Raspberry Pi

[Stefan Wahren]

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;