usb: dwc2: regression during boot on Raspberry Pi

[Marek Szyprowski]

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