usb: gadget: at91_udc: move prepare clk into process context
Subsystems (2)
PATCH v2 1/1 usb: gadget: at91_udc: move prepare clk into process context
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(-)
drivers/usb/gadget/udc/at91_udc.c +30 -9
@@ -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 ))
Signature · 1 line
1.9.3
PATCH v2 1/1 usb: gadget: at91_udc: move prepare clk into process context
Quoted 3 lines · Ronald Wahl
Commit
7628083227b6bc4a7e33d7c381d7a4e558424b6badded clock preparation in interrupt context. This is not allowed as it might sleep. Move clock preparation into process context (at91udc_probe).
Add Boris and Felipe to Cc: ...
- ron
PATCH v2 1/1 usb: gadget: at91_udc: move prepare clk into process context
In reply to Ronald WahlHi Ronald,
Quoted 27 lines · On Wed, 6 Aug 2014 15:11:42 +0200 Ronald Wahl <[email protected]> wrote:
Commit
7628083227b6bc4a7e33d7c381d7a4e558424b6badded 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(-)drivers/usb/gadget/udc/at91_udc.c +3
-3@@ -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 ;
drivers/usb/gadget/udc/at91_udc.c static void clk_off(struct at91_udc *udc) · line 891
Quoted 10 lines
drivers/usb/gadget/udc/at91_udc.c +3
-3udc->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
Signature · 3 lines
Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
PATCH v2 1/1 usb: gadget: at91_udc: move prepare clk into process context
Adding USB and Atmel Maintainers in Cc.
Quoted 60 lines · On Thu, 7 Aug 2014 09:52:31 +0200 Boris BREZILLON…
Hi Ronald,
Quoted 27 lines · On Wed, 6 Aug 2014 15:11:42 +0200 Ronald Wahl <[email protected]> wrote:
Commit
7628083227b6bc4a7e33d7c381d7a4e558424b6badded 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(-)drivers/usb/gadget/udc/at91_udc.c +3
-3@@ -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 ;drivers/usb/gadget/udc/at91_udc.c static void clk_off(struct at91_udc *udc) · line 891
Quoted 10 lines
drivers/usb/gadget/udc/at91_udc.c +3
-3udc->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_unpreparecall.How about leaving the clk_on/off unchanged and use a threaded irq instead of a normal irq ?
Best Regards,
Boris
Signature · 3 lines
Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
PATCH v2 1/1 usb: gadget: at91_udc: move prepare clk into process context
Quoted 46 lines · Boris BREZILLON
Quoted 44 lines · On Thu, 7 Aug 2014 09:52:31 +0200 Boris BREZILLON…
Quoted 27 lines · On Wed, 6 Aug 2014 15:11:42 +0200 Ronald Wahl <[email protected]> wrote:
Commit
7628083227b6bc4a7e33d7c381d7a4e558424b6badded 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(-)drivers/usb/gadget/udc/at91_udc.c +3
-3@@ -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 ;drivers/usb/gadget/udc/at91_udc.c static void clk_off(struct at91_udc *udc) · line 891
Quoted 10 lines
drivers/usb/gadget/udc/at91_udc.c +3
-3udc->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.
Quoted 8 lines
Quoted 8 lines
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_unpreparecall.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
Signature · 5 lines
Ronald Wahl - ronald.wahl at raritan.com - Phone +49 375271349-0 Fax -99 Raritan Deutschland GmbH, Kornmarkt 7, 08056 Zwickau, Germany USt-IdNr. DE813094160, Steuer-Nr. 227/117/01749 Amtsgericht Chemnitz HRB 23605 Gesch?ftsf?hrung: Stuart Hopper, Ralf Ploenes
PATCH v2 1/1 usb: gadget: at91_udc: move prepare clk into process context
Quoted 53 lines · On Thu, 07 Aug 2014 14:43:32 +0200 Ronald Wahl <[email protected]> wrote:
Quoted 52 lines · Boris BREZILLON
Quoted 50 lines · On Thu, 7 Aug 2014 09:52:31 +0200 Boris BREZILLON…
Quoted 27 lines · On Wed, 6 Aug 2014 15:11:42 +0200 Ronald Wahl <[email protected]> wrote:
Commit
7628083227b6bc4a7e33d7c381d7a4e558424b6badded 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(-)drivers/usb/gadget/udc/at91_udc.c +3
-3@@ -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 ;drivers/usb/gadget/udc/at91_udc.c static void clk_off(struct at91_udc *udc) · line 891
Quoted 10 lines
drivers/usb/gadget/udc/at91_udc.c +3
-3udc->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_unpreparecall.
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
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 whereclk_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
Signature · 3 lines
Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
PATCH v2 1/1 usb: gadget: at91_udc: move prepare clk into process context
In reply to Ronald WahlHi,
Quoted 3 lines · Ronald Wahl
Commit
7628083227b6bc4a7e33d7c381d7a4e558424b6badded 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.
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140807/fbff6d10/attachment.sig>
PATCH v2 1/1 usb: gadget: at91_udc: move prepare clk into process context
In reply to Ronald WahlQuoting Ronald Wahl (2014-08-06 06:11:42)
Quoted 12 lines
Commit
7628083227b6bc4a7e33d7c381d7a4e558424b6badded 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(-)drivers/usb/gadget/udc/at91_udc.c +0
-0
drivers/usb/gadget/udc/at91_udc.c static void clk_on(struct at91_udc *udc) · line 873
Quoted 2 lines
drivers/usb/gadget/udc/at91_udc.c +0
-0@@ -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.
Quoted 26 lines
drivers/usb/gadget/udc/at91_udc.c +6
-6clk_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); }/*
drivers/usb/gadget/udc/at91_udc.c static int at91udc_probe(struct platform_device *pdev) · line 1783
Quoted 4 lines
drivers/usb/gadget/udc/at91_udc.c +1
-0@@ -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
PATCH v2 1/1 usb: gadget: at91_udc: move prepare clk into process context
Hi Mike,
Quoted 14 lines · Mike Turquette
Quoting Ronald Wahl (2014-08-06 06:11:42)
Quoted 13 lines
Commit
7628083227b6bc4a7e33d7c381d7a4e558424b6badded 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(-)drivers/usb/gadget/udc/at91_udc.c +0
-0@@ -872,10 +872,10 @@ static void clk_on(struct at91_udc *udc)
Quoted 5 lines
drivers/usb/gadget/udc/at91_udc.c static void clk_on(struct at91_udc *udc) · line 873
Quoted 1 line
drivers/usb/gadget/udc/at91_udc.c +0
-0if (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.
Quoted 41 lines
Quoted 26 lines
drivers/usb/gadget/udc/at91_udc.c +6
-6clk_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); }/* drivers/usb/gadget/udc/at91_udc.c static int at91udc_probe(struct platform_device *pdev) · line 1783
Quoted 4 lines
drivers/usb/gadget/udc/at91_udc.c +1
-0@@ -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
Signature · 3 lines
Alexandre Belloni, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
PATCH v2 1/1 usb: gadget: at91_udc: move prepare clk into process context
Quoted 17 lines · On Wed, 13 Aug 2014 16:53:49 +0200 Alexandre Belloni…
Hi Mike,
Quoted 14 lines · Mike Turquette
Quoting Ronald Wahl (2014-08-06 06:11:42)
Quoted 13 lines
Commit
7628083227b6bc4a7e33d7c381d7a4e558424b6badded 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(-)drivers/usb/gadget/udc/at91_udc.c +0
-0@@ -872,10 +872,10 @@ static void clk_on(struct at91_udc *udc)
Quoted 8 lines
Quoted 5 lines
drivers/usb/gadget/udc/at91_udc.c static void clk_on(struct at91_udc *udc) · line 873
Quoted 1 line
drivers/usb/gadget/udc/at91_udc.c +0
-0if (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_clkis 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).
Quoted 43 lines
Quoted 42 lines
Quoted 27 lines
drivers/usb/gadget/udc/at91_udc.c +6
-6clk_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); }/* drivers/usb/gadget/udc/at91_udc.c static int at91udc_probe(struct platform_device *pdev) · line 1783
Quoted 4 lines
drivers/usb/gadget/udc/at91_udc.c +1
-0@@ -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
Signature · 3 lines
Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
PATCH v2 1/1 usb: gadget: at91_udc: move prepare clk into process context
On 13/08/2014 17:20, Boris BREZILLON :
Quoted 38 lines
Quoted 17 lines · On Wed, 13 Aug 2014 16:53:49 +0200 Alexandre Belloni…
Hi Mike,
Quoted 14 lines · Mike Turquette
Quoting Ronald Wahl (2014-08-06 06:11:42)
Quoted 13 lines
Commit
7628083227b6bc4a7e33d7c381d7a4e558424b6badded 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(-)drivers/usb/gadget/udc/at91_udc.c +0
-0@@ -872,10 +872,10 @@ static void clk_on(struct at91_udc *udc)Quoted 8 lines
Quoted 5 lines
drivers/usb/gadget/udc/at91_udc.c static void clk_on(struct at91_udc *udc) · line 873
Quoted 1 line
drivers/usb/gadget/udc/at91_udc.c +0
-0if (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_clkis 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).
Quoted 7 lines
What we could do here is test for
CONFIG_COMMON_CLK_AT91instead ofCONFIG_COMMON_CLK(so that there's no direct dependency on the CCF).Another option is to implement
determine_ratein the new at91 usb clk implementation and then callclk_set_rateon 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,
Quoted 45 lines
Quoted 42 lines
Quoted 41 lines
Quoted 26 lines
drivers/usb/gadget/udc/at91_udc.c +6
-6clk_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); }/* drivers/usb/gadget/udc/at91_udc.c static int at91udc_probe(struct platform_device *pdev) · line 1783
Quoted 4 lines
drivers/usb/gadget/udc/at91_udc.c +1
-0@@ -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
Signature · 1 line
Nicolas Ferre