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

[Boris BREZILLON]

On Thu, 07 Aug 2014 14:43:32 +0200
Ronald Wahl <ronald.wahl at raritan.com> wrote:

> On 07.08.2014 09:59, Boris BREZILLON wrote:
> > On Thu, 7 Aug 2014 09:52:31 +0200
> > Boris BREZILLON <boris.brezillon at free-electrons.com> wrote:
> >> 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 ?
> 
> Even with threaded interrupts things are still called while interrupts 
> are disabled by one or more layers of spin_lock_irqsave. There are also 
> different code paths from where clk_on() is called.

You're right (I only had a quick look at it). Let's fix this as a first
step and we'll figure out how to optimize power consumption later.
BTW, clk_set_rate can sleep too (AFAIK clk_enable and clk_disable
are the only one that can be called in atomic context).

Best Regards,

Boris

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