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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html