Re: usb: dwc2: regression during boot on Raspberry Pi

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello,

On 2015-11-08 11:12, 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;



This change looks reasonable, I remember the similar issue was in s3c-hsotg driver
and it caused kernel ops on driver probe, when bootloader left dwc2 enabled.
The other way of solving it would be to add
irq_set_status_flags(irq, IRQ_NOAUTOEN);
before devm_request_irq() and then call enable_irq(irq) after
dwc2_lowlevel_hw_enable().

Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux