| Message ID | 2135194.vFtAtbOFPb@diego (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Heiko, On Fri, Dec 18, 2015 at 10:30 AM, Heiko Stübner <heiko.stuebner@collabora.com> wrote: > In specific conditions (involving usb hubs) dwc2 devices can create a > lot of interrupts, even to the point of overwhelming devices running > at low frequencies. Some devices need to do special clock handling > at shutdown-time which may bring the system clock below the threshold > of being able to handle the dwc2 interrupts. Disabling dwc2-irqs > in a shutdown callbacks prevents reboots/poweroffs from getting stuck > in such cases. > > The hsotg struct already contains an unused irq element, so we can > just use it to store the irq number for the shutdown callback. > > Signed-off-by: Heiko Stuebner <heiko.stuebner@collabora.com> > --- > I'm also adapting the code on the clock-side to lessen the effects of > the slow clock (see clk: rockchip: only enter pll slow-mode directly > before reboots on rk3288), but this patch also fixes the issue of the > overwhelming irq-number in itself and may be interesting for other/future > platforms using the dwc2. > > > drivers/usb/dwc2/platform.c | 35 +++++++++++++++++++++++++++-------- > 1 file changed, 27 insertions(+), 8 deletions(-) > > diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c > index 39c1cbf..5510d07 100644 > --- a/drivers/usb/dwc2/platform.c > +++ b/drivers/usb/dwc2/platform.c > @@ -306,6 +306,25 @@ static int dwc2_driver_remove(struct platform_device *dev) > return 0; > } > > +/** > + * dwc2_driver_shutdown() - Called on device shutdown > + * > + * @dev: Platform device > + * > + * In specific conditions (involving usb hubs) dwc2 devices can create a > + * lot of interrupts, even to the point of overwhelming devices running > + * at low frequencies. Some devices need to do special clock handling > + * at shutdown-time which may bring the system clock below the threshold > + * of being able to handle the dwc2 interrupts. Disabling dwc2-irqs > + * prevents reboots/poweroffs from getting stuck in such cases. > + */ > +static void dwc2_driver_shutdown(struct platform_device *dev) > +{ > + struct dwc2_hsotg *hsotg = platform_get_drvdata(dev); > + > + disable_irq(hsotg->irq); In one other place I see dwc2 getting the IRQ from the USB HCD structure. That is: dwc2_hsotg_to_hcd(hsotg)->irq I wonder if that would be a good idea to do? Then a future patch could just remove the unused (and redundant) irq from the hsotg structure? Other than that this seems sane to me. -Doug
Hi Doug, Am Freitag, 18. Dezember 2015, 14:50:14 schrieb Doug Anderson: > On Fri, Dec 18, 2015 at 10:30 AM, Heiko Stübner > <heiko.stuebner@collabora.com> wrote: > > In specific conditions (involving usb hubs) dwc2 devices can create a > > lot of interrupts, even to the point of overwhelming devices running > > at low frequencies. Some devices need to do special clock handling > > at shutdown-time which may bring the system clock below the threshold > > of being able to handle the dwc2 interrupts. Disabling dwc2-irqs > > in a shutdown callbacks prevents reboots/poweroffs from getting stuck > > in such cases. > > > > The hsotg struct already contains an unused irq element, so we can > > just use it to store the irq number for the shutdown callback. > > > > Signed-off-by: Heiko Stuebner <heiko.stuebner@collabora.com> > > --- > > I'm also adapting the code on the clock-side to lessen the effects of > > the slow clock (see clk: rockchip: only enter pll slow-mode directly > > before reboots on rk3288), but this patch also fixes the issue of the > > overwhelming irq-number in itself and may be interesting for other/future > > platforms using the dwc2. > > > > drivers/usb/dwc2/platform.c | 35 +++++++++++++++++++++++++++-------- > > 1 file changed, 27 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c > > index 39c1cbf..5510d07 100644 > > --- a/drivers/usb/dwc2/platform.c > > +++ b/drivers/usb/dwc2/platform.c > > @@ -306,6 +306,25 @@ static int dwc2_driver_remove(struct platform_device > > *dev)> > > return 0; > > > > } > > > > +/** > > + * dwc2_driver_shutdown() - Called on device shutdown > > + * > > + * @dev: Platform device > > + * > > + * In specific conditions (involving usb hubs) dwc2 devices can create a > > + * lot of interrupts, even to the point of overwhelming devices running > > + * at low frequencies. Some devices need to do special clock handling > > + * at shutdown-time which may bring the system clock below the threshold > > + * of being able to handle the dwc2 interrupts. Disabling dwc2-irqs > > + * prevents reboots/poweroffs from getting stuck in such cases. > > + */ > > +static void dwc2_driver_shutdown(struct platform_device *dev) > > +{ > > + struct dwc2_hsotg *hsotg = platform_get_drvdata(dev); > > + > > + disable_irq(hsotg->irq); > > In one other place I see dwc2 getting the IRQ from the USB HCD > structure. That is: > > dwc2_hsotg_to_hcd(hsotg)->irq > > I wonder if that would be a good idea to do? Then a future patch > could just remove the unused (and redundant) irq from the hsotg > structure? The hcd-part as well as the gadget equivalent only gets enabled when the right dr-mode is set: if (hsotg->dr_mode != USB_DR_MODE_PERIPHERAL) { retval = dwc2_hcd_init(hsotg, hsotg->irq); ... Additionally the hcd/gadget part can also be compiled out, making that init function a stub. Also I think dwc2_hsotg_to_hcd is only defined in the hcd- scope and in the platform code you cannot really be sure if that is actually available - or would need to check for hcd-existence + gadget-existence as fallback. So I'd think accessing a generic irq-value might be preferrable :-) . Heiko
Heiko, On Fri, Dec 18, 2015 at 3:17 PM, Heiko Stübner <heiko.stuebner@collabora.com> wrote: > Hi Doug, > > Am Freitag, 18. Dezember 2015, 14:50:14 schrieb Doug Anderson: >> On Fri, Dec 18, 2015 at 10:30 AM, Heiko Stübner >> <heiko.stuebner@collabora.com> wrote: >> > In specific conditions (involving usb hubs) dwc2 devices can create a >> > lot of interrupts, even to the point of overwhelming devices running >> > at low frequencies. Some devices need to do special clock handling >> > at shutdown-time which may bring the system clock below the threshold >> > of being able to handle the dwc2 interrupts. Disabling dwc2-irqs >> > in a shutdown callbacks prevents reboots/poweroffs from getting stuck >> > in such cases. >> > >> > The hsotg struct already contains an unused irq element, so we can >> > just use it to store the irq number for the shutdown callback. >> > >> > Signed-off-by: Heiko Stuebner <heiko.stuebner@collabora.com> >> > --- >> > I'm also adapting the code on the clock-side to lessen the effects of >> > the slow clock (see clk: rockchip: only enter pll slow-mode directly >> > before reboots on rk3288), but this patch also fixes the issue of the >> > overwhelming irq-number in itself and may be interesting for other/future >> > platforms using the dwc2. >> > >> > drivers/usb/dwc2/platform.c | 35 +++++++++++++++++++++++++++-------- >> > 1 file changed, 27 insertions(+), 8 deletions(-) >> > >> > diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c >> > index 39c1cbf..5510d07 100644 >> > --- a/drivers/usb/dwc2/platform.c >> > +++ b/drivers/usb/dwc2/platform.c >> > @@ -306,6 +306,25 @@ static int dwc2_driver_remove(struct platform_device >> > *dev)> >> > return 0; >> > >> > } >> > >> > +/** >> > + * dwc2_driver_shutdown() - Called on device shutdown >> > + * >> > + * @dev: Platform device >> > + * >> > + * In specific conditions (involving usb hubs) dwc2 devices can create a >> > + * lot of interrupts, even to the point of overwhelming devices running >> > + * at low frequencies. Some devices need to do special clock handling >> > + * at shutdown-time which may bring the system clock below the threshold >> > + * of being able to handle the dwc2 interrupts. Disabling dwc2-irqs >> > + * prevents reboots/poweroffs from getting stuck in such cases. >> > + */ >> > +static void dwc2_driver_shutdown(struct platform_device *dev) >> > +{ >> > + struct dwc2_hsotg *hsotg = platform_get_drvdata(dev); >> > + >> > + disable_irq(hsotg->irq); >> >> In one other place I see dwc2 getting the IRQ from the USB HCD >> structure. That is: >> >> dwc2_hsotg_to_hcd(hsotg)->irq >> >> I wonder if that would be a good idea to do? Then a future patch >> could just remove the unused (and redundant) irq from the hsotg >> structure? > > The hcd-part as well as the gadget equivalent only gets enabled when the right > dr-mode is set: > if (hsotg->dr_mode != USB_DR_MODE_PERIPHERAL) { > retval = dwc2_hcd_init(hsotg, hsotg->irq); > ... > > Additionally the hcd/gadget part can also be compiled out, making that init > function a stub. Also I think dwc2_hsotg_to_hcd is only defined in the hcd- > scope and in the platform code you cannot really be sure if that is actually > available - or would need to check for hcd-existence + gadget-existence as > fallback. > > So I'd think accessing a generic irq-value might be preferrable :-) . Oh right. Duh. Now that I've made a fool of myself, you can decide if my reviewed-by is worth anything, but feel free to it. :-P Reviewed-by: Douglas Anderson <dianders@chromium.org>
On 12/18/2015 10:31 AM, Heiko Stübner wrote: > In specific conditions (involving usb hubs) dwc2 devices can create a > lot of interrupts, even to the point of overwhelming devices running > at low frequencies. Some devices need to do special clock handling > at shutdown-time which may bring the system clock below the threshold > of being able to handle the dwc2 interrupts. Disabling dwc2-irqs > in a shutdown callbacks prevents reboots/poweroffs from getting stuck > in such cases. > > The hsotg struct already contains an unused irq element, so we can > just use it to store the irq number for the shutdown callback. > > Signed-off-by: Heiko Stuebner <heiko.stuebner@collabora.com> > --- > I'm also adapting the code on the clock-side to lessen the effects of > the slow clock (see clk: rockchip: only enter pll slow-mode directly > before reboots on rk3288), but this patch also fixes the issue of the > overwhelming irq-number in itself and may be interesting for other/future > platforms using the dwc2. > > > drivers/usb/dwc2/platform.c | 35 +++++++++++++++++++++++++++-------- > 1 file changed, 27 insertions(+), 8 deletions(-) > > diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c > index 39c1cbf..5510d07 100644 > --- a/drivers/usb/dwc2/platform.c > +++ b/drivers/usb/dwc2/platform.c > @@ -306,6 +306,25 @@ static int dwc2_driver_remove(struct platform_device *dev) > return 0; > } > > +/** > + * dwc2_driver_shutdown() - Called on device shutdown > + * > + * @dev: Platform device > + * > + * In specific conditions (involving usb hubs) dwc2 devices can create a > + * lot of interrupts, even to the point of overwhelming devices running > + * at low frequencies. Some devices need to do special clock handling > + * at shutdown-time which may bring the system clock below the threshold > + * of being able to handle the dwc2 interrupts. Disabling dwc2-irqs > + * prevents reboots/poweroffs from getting stuck in such cases. > + */ > +static void dwc2_driver_shutdown(struct platform_device *dev) > +{ > + struct dwc2_hsotg *hsotg = platform_get_drvdata(dev); > + > + disable_irq(hsotg->irq); > +} > + > static const struct of_device_id dwc2_of_match_table[] = { > { .compatible = "brcm,bcm2835-usb", .data = ¶ms_bcm2835 }, > { .compatible = "rockchip,rk3066-usb", .data = ¶ms_rk3066 }, > @@ -335,7 +354,6 @@ static int dwc2_driver_probe(struct platform_device *dev) > struct dwc2_hsotg *hsotg; > struct resource *res; > int retval; > - int irq; > > match = of_match_device(dwc2_of_match_table, &dev->dev); > if (match && match->data) { > @@ -401,15 +419,15 @@ 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) { > + hsotg->irq = platform_get_irq(dev, 0); > + if (hsotg->irq < 0) { > dev_err(&dev->dev, "missing IRQ resource\n"); > - return irq; > + return hsotg->irq; > } > > dev_dbg(hsotg->dev, "registering common handler for irq%d\n", > - irq); > - retval = devm_request_irq(hsotg->dev, irq, > + hsotg->irq); > + retval = devm_request_irq(hsotg->dev, hsotg->irq, > dwc2_handle_common_intr, IRQF_SHARED, > dev_name(hsotg->dev), hsotg); > if (retval) > @@ -428,14 +446,14 @@ static int dwc2_driver_probe(struct platform_device *dev) > dwc2_set_parameters(hsotg, params); > > if (hsotg->dr_mode != USB_DR_MODE_HOST) { > - retval = dwc2_gadget_init(hsotg, irq); > + retval = dwc2_gadget_init(hsotg, hsotg->irq); > if (retval) > goto error; > hsotg->gadget_enabled = 1; > } > > if (hsotg->dr_mode != USB_DR_MODE_PERIPHERAL) { > - retval = dwc2_hcd_init(hsotg, irq); > + retval = dwc2_hcd_init(hsotg, hsotg->irq); > if (retval) { > if (hsotg->gadget_enabled) > dwc2_hsotg_remove(hsotg); > @@ -502,6 +520,7 @@ static struct platform_driver dwc2_platform_driver = { > }, > .probe = dwc2_driver_probe, > .remove = dwc2_driver_remove, > + .shutdown = dwc2_driver_shutdown, > }; > > module_platform_driver(dwc2_platform_driver); > Acked-by: John Youn <johnyoun@synopsys.com> John
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c index 39c1cbf..5510d07 100644 --- a/drivers/usb/dwc2/platform.c +++ b/drivers/usb/dwc2/platform.c @@ -306,6 +306,25 @@ static int dwc2_driver_remove(struct platform_device *dev) return 0; } +/** + * dwc2_driver_shutdown() - Called on device shutdown + * + * @dev: Platform device + * + * In specific conditions (involving usb hubs) dwc2 devices can create a + * lot of interrupts, even to the point of overwhelming devices running + * at low frequencies. Some devices need to do special clock handling + * at shutdown-time which may bring the system clock below the threshold + * of being able to handle the dwc2 interrupts. Disabling dwc2-irqs + * prevents reboots/poweroffs from getting stuck in such cases. + */ +static void dwc2_driver_shutdown(struct platform_device *dev) +{ + struct dwc2_hsotg *hsotg = platform_get_drvdata(dev); + + disable_irq(hsotg->irq); +} + static const struct of_device_id dwc2_of_match_table[] = { { .compatible = "brcm,bcm2835-usb", .data = ¶ms_bcm2835 }, { .compatible = "rockchip,rk3066-usb", .data = ¶ms_rk3066 }, @@ -335,7 +354,6 @@ static int dwc2_driver_probe(struct platform_device *dev) struct dwc2_hsotg *hsotg; struct resource *res; int retval; - int irq; match = of_match_device(dwc2_of_match_table, &dev->dev); if (match && match->data) { @@ -401,15 +419,15 @@ 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) { + hsotg->irq = platform_get_irq(dev, 0); + if (hsotg->irq < 0) { dev_err(&dev->dev, "missing IRQ resource\n"); - return irq; + return hsotg->irq; } dev_dbg(hsotg->dev, "registering common handler for irq%d\n", - irq); - retval = devm_request_irq(hsotg->dev, irq, + hsotg->irq); + retval = devm_request_irq(hsotg->dev, hsotg->irq, dwc2_handle_common_intr, IRQF_SHARED, dev_name(hsotg->dev), hsotg); if (retval) @@ -428,14 +446,14 @@ static int dwc2_driver_probe(struct platform_device *dev) dwc2_set_parameters(hsotg, params); if (hsotg->dr_mode != USB_DR_MODE_HOST) { - retval = dwc2_gadget_init(hsotg, irq); + retval = dwc2_gadget_init(hsotg, hsotg->irq); if (retval) goto error; hsotg->gadget_enabled = 1; } if (hsotg->dr_mode != USB_DR_MODE_PERIPHERAL) { - retval = dwc2_hcd_init(hsotg, irq); + retval = dwc2_hcd_init(hsotg, hsotg->irq); if (retval) { if (hsotg->gadget_enabled) dwc2_hsotg_remove(hsotg); @@ -502,6 +520,7 @@ static struct platform_driver dwc2_platform_driver = { }, .probe = dwc2_driver_probe, .remove = dwc2_driver_remove, + .shutdown = dwc2_driver_shutdown, }; module_platform_driver(dwc2_platform_driver);
In specific conditions (involving usb hubs) dwc2 devices can create a lot of interrupts, even to the point of overwhelming devices running at low frequencies. Some devices need to do special clock handling at shutdown-time which may bring the system clock below the threshold of being able to handle the dwc2 interrupts. Disabling dwc2-irqs in a shutdown callbacks prevents reboots/poweroffs from getting stuck in such cases. The hsotg struct already contains an unused irq element, so we can just use it to store the irq number for the shutdown callback. Signed-off-by: Heiko Stuebner <heiko.stuebner@collabora.com> --- I'm also adapting the code on the clock-side to lessen the effects of the slow clock (see clk: rockchip: only enter pll slow-mode directly before reboots on rk3288), but this patch also fixes the issue of the overwhelming irq-number in itself and may be interesting for other/future platforms using the dwc2. drivers/usb/dwc2/platform.c | 35 +++++++++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 8 deletions(-)