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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
