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

[Boris BREZILLON]

On Wed, 13 Aug 2014 16:53:49 +0200
Alexandre Belloni <alexandre.belloni at free-electrons.com> wrote:

> Hi Mike,
> 
> On 11/08/2014 at 20:34:56 -0700, Mike Turquette wrote :
> > Quoting Ronald Wahl (2014-08-06 06:11:42)
> > > 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)) {
> > 
> > Why is this check necessary at all? Drivers shouldn't have to care at
> > all about the underlying clock framework implementation.
> > 
> 
> I believe it has been done because without the common clock framework,
> usb_clk is not defined.

Absolutely: uclk is only available when using the CCF. In the old at91
clock implementation the USB clock rate was configured during early boot
at registration time.
With the CCF implementation USB clk rate is no longer hardcoded at init
time and we have to configure it appropriately before using it (the
prepare and enable actions are useless though because uclk is the parent
of fclk, and thus will be prepared/enabled when fclk is
prepared/enabled).

What we could do here is test for CONFIG_COMMON_CLK_AT91 instead of
CONFIG_COMMON_CLK (so that there's no direct dependency on the CCF).

Another option is to implement determine_rate in the new at91 usb clk
implementation and then call clk_set_rate on fclk (without testing the
return code, because the old clk implementation will always return
-EINVAL).

> 
> > >                 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);
> > >  }
> > >  
> > >  /*
> > > @@ -1780,14 +1780,23 @@ static int at91udc_probe(struct platform_device *pdev)
> > >         }
> > >  
> > >         /* don't do anything until we have both gadget driver and VBUS */
> > > +       if (IS_ENABLED(CONFIG_COMMON_CLK)) {
> > 
> > Same question here. What does the clock framework implementation have to
> > do with uclk?
> > 
> > Regards,
> > Mike
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 



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