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
