| Message ID | 1407330702-2926-1-git-send-email-ronald.wahl@raritan.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Ronald, On Wed, 6 Aug 2014 15:11:42 +0200 Ronald Wahl <ronald.wahl@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
Adding USB and Atmel Maintainers in Cc. On Thu, 7 Aug 2014 09:52:31 +0200 Boris BREZILLON <boris.brezillon@free-electrons.com> wrote: > Hi Ronald, > > On Wed, 6 Aug 2014 15:11:42 +0200 > Ronald Wahl <ronald.wahl@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 >
On 07.08.2014 09:59, Boris BREZILLON wrote: > On Thu, 7 Aug 2014 09:52:31 +0200 > Boris BREZILLON <boris.brezillon@free-electrons.com> wrote: >> On Wed, 6 Aug 2014 15:11:42 +0200 >> Ronald Wahl <ronald.wahl@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. So getting this right while saving power is probably not trivial and beyond my knowledge/experience with the udc code. It would be nice if someone with a deeper knowledge of the atmel udc code can get this right. My primary intention was to get the code working again. thx & greets, ron
On Thu, 07 Aug 2014 14:43:32 +0200 Ronald Wahl <ronald.wahl@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@free-electrons.com> wrote: > >> On Wed, 6 Aug 2014 15:11:42 +0200 > >> Ronald Wahl <ronald.wahl@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
Hi, On Wed, Aug 06, 2014 at 03:11:42PM +0200, Ronald Wahl 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). missing Signed-off-by. Also, please resend with linux-usb in the loop.
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. > 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
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. > > 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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, 13 Aug 2014 16:53:49 +0200 Alexandre Belloni <alexandre.belloni@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@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On 13/08/2014 17:20, Boris BREZILLON : > On Wed, 13 Aug 2014 16:53:49 +0200 > Alexandre Belloni <alexandre.belloni@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). I would recommend to keep it like that until we remove all need for the old clock implementation. Then we will be able to remove this test and simplify the code. In the meantime, it is stated explicitly and I find it more obvious than trying to hide this matter of fact. Bye, >>>> 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@lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >> > > >
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); } /* @@ -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)) { + retval = clk_prepare(udc->uclk); + if (retval) + goto fail1; + } + retval = clk_prepare(udc->fclk); + if (retval) + goto fail1a; + retval = clk_prepare_enable(udc->iclk); if (retval) - goto fail1; + goto fail1b; at91_udp_write(udc, AT91_UDP_TXVC, AT91_UDP_TXVC_TXVDIS); at91_udp_write(udc, AT91_UDP_IDR, 0xffffffff); /* Clear all pending interrupts - UDP may be used by bootloader. */ at91_udp_write(udc, AT91_UDP_ICR, 0xffffffff); - clk_disable_unprepare(udc->iclk); + clk_disable(udc->iclk); /* request UDC and maybe VBUS irqs */ udc->udp_irq = platform_get_irq(pdev, 0); @@ -1795,7 +1804,7 @@ static int at91udc_probe(struct platform_device *pdev) 0, driver_name, udc); if (retval < 0) { DBG("request irq %d failed\n", udc->udp_irq); - goto fail1; + goto fail1c; } if (gpio_is_valid(udc->board.vbus_pin)) { retval = gpio_request(udc->board.vbus_pin, "udc_vbus"); @@ -1848,6 +1857,13 @@ fail3: gpio_free(udc->board.vbus_pin); fail2: free_irq(udc->udp_irq, udc); +fail1c: + clk_unprepare(udc->iclk); +fail1b: + clk_unprepare(udc->fclk); +fail1a: + if (IS_ENABLED(CONFIG_COMMON_CLK)) + clk_unprepare(udc->uclk); fail1: if (IS_ENABLED(CONFIG_COMMON_CLK) && !IS_ERR(udc->uclk)) clk_put(udc->uclk); @@ -1896,6 +1912,11 @@ static int __exit at91udc_remove(struct platform_device *pdev) res = platform_get_resource(pdev, IORESOURCE_MEM, 0); release_mem_region(res->start, resource_size(res)); + if (IS_ENABLED(CONFIG_COMMON_CLK)) + clk_unprepare(udc->uclk); + clk_unprepare(udc->fclk); + clk_unprepare(udc->iclk); + clk_put(udc->iclk); clk_put(udc->fclk); if (IS_ENABLED(CONFIG_COMMON_CLK))