Re: [PATCH] usb: dwc2: add shutdown callback to platform variant

["Heiko Stübner" <heiko.stuebner-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>]

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