[PATCH v2] usb: gadget: at91_udc: move prepare clk into process context

[Boris BREZILLON]

Adding USB and Atmel Maintainers in Cc.

On Thu, 7 Aug 2014 09:52:31 +0200
Boris BREZILLON <boris.brezillon at free-electrons.com> wrote:

> Hi Ronald,
> 
> On Wed,  6 Aug 2014 15:11:42 +0200
> Ronald Wahl <ronald.wahl at raritan.com> wrote:
> 
> > Commit 7628083227b6bc4a7e33d7c381d7a4e558424b6b added clock preparation in
> > interrupt context. This is not allowed as it might sleep. Move clock
> > preparation into process context (at91udc_probe).
> > ---
> >  drivers/usb/gadget/udc/at91_udc.c | 39 ++++++++++++++++++++++++++++++---------
> >  1 file changed, 30 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/usb/gadget/udc/at91_udc.c b/drivers/usb/gadget/udc/at91_udc.c
> > index cfd18bc..0b347a0 100644
> > --- a/drivers/usb/gadget/udc/at91_udc.c
> > +++ b/drivers/usb/gadget/udc/at91_udc.c
> > @@ -872,10 +872,10 @@ static void clk_on(struct at91_udc *udc)
> >  
> >  	if (IS_ENABLED(CONFIG_COMMON_CLK)) {
> >  		clk_set_rate(udc->uclk, 48000000);
> > -		clk_prepare_enable(udc->uclk);
> > +		clk_enable(udc->uclk);
> >  	}
> > -	clk_prepare_enable(udc->iclk);
> > -	clk_prepare_enable(udc->fclk);
> > +	clk_enable(udc->iclk);
> > +	clk_enable(udc->fclk);
> >  }
> >  
> >  static void clk_off(struct at91_udc *udc)
> > @@ -884,10 +884,10 @@ static void clk_off(struct at91_udc *udc)
> >  		return;
> >  	udc->clocked = 0;
> >  	udc->gadget.speed = USB_SPEED_UNKNOWN;
> > -	clk_disable_unprepare(udc->fclk);
> > -	clk_disable_unprepare(udc->iclk);
> > +	clk_disable(udc->fclk);
> > +	clk_disable(udc->iclk);
> >  	if (IS_ENABLED(CONFIG_COMMON_CLK))
> > -		clk_disable_unprepare(udc->uclk);
> > +		clk_disable(udc->uclk);
> >  }
> 
> As you stated prepare and unprepare should never be called in interrupt
> context. 
> 
> My concern here is that PLLB (which is often used as USB clock
> parent) will never be gated/disabled (because all this work is
> done in its unprepare method), and thus your power consumption will be
> higher (when entering suspend mode) than if you'd done a
> disable_unprepare call.
> 
> How about leaving the clk_on/off unchanged and use a threaded irq
> instead of a normal irq ?
> 
> 
> Best Regards,
> 
> Boris
> 



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com