diff mbox

[v4] ARM: davinci: da8xx: Fix sleeping function called from invalid context

Message ID 1480690380-13216-1-git-send-email-abailon@baylibre.com
State Superseded
Headers show

Commit Message

Alexandre Bailon Dec. 2, 2016, 2:53 p.m. UTC
Everytime the usb20 phy is enabled, there is a
"sleeping function called from invalid context" BUG.

clk_enable() from arch/arm/mach-davinci/clock.c uses spin_lock_irqsave()
before to invoke the callback usb20_phy_clk_enable().
usb20_phy_clk_enable() uses clk_get() and clk_enable_prepapre()
which may sleep.
Move clk_get() to da8xx_register_usb20_phy_clk() and
replace clk_prepare_enable() by clk_enable().

Signed-off-by: Alexandre Bailon <abailon@baylibre.com>

---
 arch/arm/mach-davinci/usb-da8xx.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

-- 
2.7.3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Comments

Sekhar Nori Dec. 5, 2016, 6:32 a.m. UTC | #1
Hi David,

On Monday 05 December 2016 09:14 AM, David Lechner wrote:
> I have just tried this patch with a bunch of kernel hacking options

> enabled. I am getting the message show at the end of this email. We

> still have the problem of nested spin locks due to nested calls to

> clk_enable(). So, really, we need to use __clk_enable() and

> __clk_disable() from arch/arm/mach-davinci/clock.c in

> usb20_phy_clk_enable() above.


Good catch. I noticed  that common clock framework uses a more elaborate
mechanism to allow nested clock API calls (see clk_prepare_lock()), but
we don't (yet) have that issue in mach-davinci since the recursive calls
are still in mach-davinci and dont need the exported API to be
recursively callable.

> Applying the following patch on top of your patch fixed the recursive

> lock message.


The patch looks okay to me, except couple of minor adjustments.

> 

> ---

> 

> diff --git a/arch/arm/mach-davinci/clock.c b/arch/arm/mach-davinci/clock.c

> index df42c93..4fba579 100644

> --- a/arch/arm/mach-davinci/clock.c

> +++ b/arch/arm/mach-davinci/clock.c

> @@ -31,7 +31,7 @@ static LIST_HEAD(clocks);

>  static DEFINE_MUTEX(clocks_mutex);

>  static DEFINE_SPINLOCK(clockfw_lock);

> 

> -static void __clk_enable(struct clk *clk)

> +void __clk_enable(struct clk *clk)


Now that this function is going to be used outside of this file, lets
drop the __ naming convention and call this davinci_clk_enable() in line
with how other davinci-local clock APIs are named. Same thing for the
disable counterpart.

Also, the making these function non-static should be a patch of its own.

Thanks,
Sekhar

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Alexandre Bailon Dec. 5, 2016, 10 a.m. UTC | #2
On 12/05/2016 04:44 AM, David Lechner wrote:
> On 12/02/2016 08:53 AM, Alexandre Bailon wrote:

>> Everytime the usb20 phy is enabled, there is a

>> "sleeping function called from invalid context" BUG.

>>

>> clk_enable() from arch/arm/mach-davinci/clock.c uses spin_lock_irqsave()

>> before to invoke the callback usb20_phy_clk_enable().

>> usb20_phy_clk_enable() uses clk_get() and clk_enable_prepapre()

>> which may sleep.

>> Move clk_get() to da8xx_register_usb20_phy_clk() and

>> replace clk_prepare_enable() by clk_enable().

>>

>> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>

>> ---

>>  arch/arm/mach-davinci/usb-da8xx.c | 28 +++++++++++++++-------------

>>  1 file changed, 15 insertions(+), 13 deletions(-)

>>

>> diff --git a/arch/arm/mach-davinci/usb-da8xx.c

>> b/arch/arm/mach-davinci/usb-da8xx.c

>> index b010e5f..704f506 100644

>> --- a/arch/arm/mach-davinci/usb-da8xx.c

>> +++ b/arch/arm/mach-davinci/usb-da8xx.c

>> @@ -22,6 +22,8 @@

>>  #define DA8XX_USB0_BASE        0x01e00000

>>  #define DA8XX_USB1_BASE        0x01e25000

>>

>> +static struct clk *usb20_clk;

>> +

>>  static struct platform_device da8xx_usb_phy = {

>>      .name        = "da8xx-usb-phy",

>>      .id        = -1,

>> @@ -158,21 +160,14 @@ int __init da8xx_register_usb_refclkin(int rate)

>>

>>  static void usb20_phy_clk_enable(struct clk *clk)

>>  {

>> -    struct clk *usb20_clk;

>>      int err;

>>      u32 val;

>>      u32 timeout = 500000; /* 500 msec */

>>

>>      val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));

>>

>> -    usb20_clk = clk_get(&da8xx_usb20_dev.dev, "usb20");

>> -    if (IS_ERR(usb20_clk)) {

>> -        pr_err("could not get usb20 clk: %ld\n", PTR_ERR(usb20_clk));

>> -        return;

>> -    }

>> -

>>      /* The USB 2.O PLL requires that the USB 2.O PSC is enabled as

>> well. */

>> -    err = clk_prepare_enable(usb20_clk);

>> +    err = clk_enable(usb20_clk);

>>      if (err) {

>>          pr_err("failed to enable usb20 clk: %d\n", err);

>>          clk_put(usb20_clk);

> 

> Need to remove clk_put() here.

I will do.
> 

>> @@ -197,8 +192,7 @@ static void usb20_phy_clk_enable(struct clk *clk)

>>

>>      pr_err("Timeout waiting for USB 2.0 PHY clock good\n");

>>  done:

>> -    clk_disable_unprepare(usb20_clk);

>> -    clk_put(usb20_clk);

>> +    clk_disable(usb20_clk);

>>  }

>>

>>  static void usb20_phy_clk_disable(struct clk *clk)

>> @@ -285,11 +279,19 @@ static struct clk_lookup usb20_phy_clk_lookup =

>>  int __init da8xx_register_usb20_phy_clk(bool use_usb_refclkin)

>>  {

>>      struct clk *parent;

>> -    int ret = 0;

>> +    int ret;

>> +

>> +    usb20_clk = clk_get(&da8xx_usb20_dev.dev, "usb20");

>> +    ret = PTR_ERR_OR_ZERO(usb20_clk);

>> +    if (ret)

>> +        return ret;

>>

>>      parent = clk_get(NULL, use_usb_refclkin ? "usb_refclkin" :

>> "pll0_aux");

>> -    if (IS_ERR(parent))

>> -        return PTR_ERR(parent);

>> +    ret = PTR_ERR_OR_ZERO(parent);

>> +    if (ret) {

>> +        clk_put(usb20_clk);

>> +        return ret;

>> +    }

>>

>>      usb20_phy_clk.parent = parent;

>>      ret = clk_register(&usb20_phy_clk);

>>

> 

> 

> I have just tried this patch with a bunch of kernel hacking options

> enabled. I am getting the message show at the end of this email. We

> still have the problem of nested spin locks due to nested calls to

> clk_enable(). So, really, we need to use __clk_enable() and

> __clk_disable() from arch/arm/mach-davinci/clock.c in

> usb20_phy_clk_enable() above.

> 

> Applying the following patch on top of your patch fixed the recursive

> lock message.

Good catch.

Thanks,
Alexandre
> 

> ---

> 

> diff --git a/arch/arm/mach-davinci/clock.c b/arch/arm/mach-davinci/clock.c

> index df42c93..4fba579 100644

> --- a/arch/arm/mach-davinci/clock.c

> +++ b/arch/arm/mach-davinci/clock.c

> @@ -31,7 +31,7 @@ static LIST_HEAD(clocks);

>  static DEFINE_MUTEX(clocks_mutex);

>  static DEFINE_SPINLOCK(clockfw_lock);

> 

> -static void __clk_enable(struct clk *clk)

> +void __clk_enable(struct clk *clk)

>  {

>         if (clk->parent)

>                 __clk_enable(clk->parent);

> @@ -44,7 +44,7 @@ static void __clk_enable(struct clk *clk)

>         }

>  }

> 

> -static void __clk_disable(struct clk *clk)

> +void __clk_disable(struct clk *clk)

>  {

>         if (WARN_ON(clk->usecount == 0))

>                 return;

> diff --git a/arch/arm/mach-davinci/clock.h b/arch/arm/mach-davinci/clock.h

> index e2a5437..8493242 100644

> --- a/arch/arm/mach-davinci/clock.h

> +++ b/arch/arm/mach-davinci/clock.h

> @@ -136,6 +136,9 @@ int davinci_clk_reset(struct clk *clk, bool reset);

>  extern struct platform_device davinci_wdt_device;

>  extern void davinci_watchdog_reset(struct platform_device *);

> 

> +void __clk_enable(struct clk *clk);

> +void __clk_disable(struct clk *clk);

> +

>  #endif

> 

>  #endif

> diff --git a/arch/arm/mach-davinci/usb-da8xx.c

> b/arch/arm/mach-davinci/usb-da8xx

> .c

> index 896d014..80ba0df 100644

> --- a/arch/arm/mach-davinci/usb-da8xx.c

> +++ b/arch/arm/mach-davinci/usb-da8xx.c

> @@ -160,19 +160,13 @@ int __init da8xx_register_usb_refclkin(int rate)

> 

>  static void usb20_phy_clk_enable(struct clk *clk)

>  {

> -       int err;

>         u32 val;

>         u32 timeout = 500000; /* 500 msec */

> 

>         val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));

> 

>         /* The USB 2.O PLL requires that the USB 2.O PSC is enabled as

> well. */

> -       err = clk_enable(usb20_clk);

> -       if (err) {

> -               pr_err("failed to enable usb20 clk: %d\n", err);

> -               clk_put(usb20_clk);

> -               return;

> -       }

> +       __clk_enable(usb20_clk);

> 

>         /*

>          * Turn on the USB 2.0 PHY, but just the PLL, and not OTG. The

> USB 1.1

> @@ -192,7 +186,7 @@ static void usb20_phy_clk_enable(struct clk *clk)

> 

>         pr_err("Timeout waiting for USB 2.0 PHY clock good\n");

>  done:

> -       clk_disable(usb20_clk);

> +       __clk_disable(usb20_clk);

>  }

> 

>  static void usb20_phy_clk_disable(struct clk *clk)

> 

> 

> 

> ---

> 

> 

> =============================================

> [ INFO: possible recursive locking detected ]

> 4.9.0-rc8-dlech-ev3-lms2012+ #22 Not tainted

> ---------------------------------------------

> swapper/1 is trying to acquire lock:

>  (

> clockfw_lock

> ){......}

> , at:

> [<c0014884>] clk_enable+0x24/0x50

> k:

>  (

> clockfw_lock

> ){......}

> , at:

> [<c0014884>] clk_enable+0x24/0x50

> ebug this:

>  Possible unsafe locking scenario:

>        CPU0

>        ----

>   lock(

> clockfw_lock

> );

>   lock(

> clockfw_lock

> );

>  May be due to missing lock nesting notation

> 4 locks held by swapper/1:

>  #0:

>  (

> &dev->mutex

> ){......}

> , at:

> [<c02bd60c>] __driver_attach+0x68/0xb4

>  #1:

>  (

> &dev->mutex

> ){......}

> , at:

> [<c02bd61c>] __driver_attach+0x78/0xb4

>  #2:

>  (

> &phy->mutex

> ){+.+...}

> , at:

> [<c025ee18>] phy_power_on+0x5c/0xe4

>  #3:

>  (

> clockfw_lock

> ){......}

> , at:

> [<c0014884>] clk_enable+0x24/0x50

> CPU: 0 PID: 1 Comm: swapper Not tainted 4.9.0-rc8-dlech-ev3-lms2012+ #22

> Hardware name: Generic DA850/OMAP-L138/AM18x

> Backtrace:

> [<c000d824>] (dump_backtrace) from [<c000dac4>] (show_stack+0x18/0x1c)

>  r7:c0e998a4 r6:c0820750 r5:c0820750 r4:c3838000

> [<c000daac>] (show_stack) from [<c02393dc>] (dump_stack+0x20/0x28)

> [<c02393bc>] (dump_stack) from [<c004bd7c>] (__lock_acquire+0x10f4/0x167c)

> [<c004ac88>] (__lock_acquire) from [<c004c6ac>] (lock_acquire+0x78/0x98)

>  r10:00000000 r9:c06a5ed4 r8:00000000 r7:00000001 r6:60000093 r5:00000000

>  r4:ffffe000

> [<c004c634>] (lock_acquire) from [<c04d8fac>]

> (_raw_spin_lock_irqsave+0x60/0x74)

>  r7:0000003b r6:c0014884 r5:80000093 r4:c06b09a0

> [<c04d8f4c>] (_raw_spin_lock_irqsave) from [<c0014884>]

> (clk_enable+0x24/0x50)

>  r6:c06f69f4 r5:0001ef02 r4:c06b3398

> [<c0014860>] (clk_enable) from [<c0015c74>]

> (usb20_phy_clk_enable+0x24/0xb8)

>  r5:0001ef02 r4:c06f69f0

> [<c0015c50>] (usb20_phy_clk_enable) from [<c0014858>]

> (__clk_enable+0x74/0x7c)

>  r9:c06a5ed4 r8:00000000 r7:0000003b r6:c3a6ca00 r5:80000013 r4:c06b7ee8

> [<c00147e4>] (__clk_enable) from [<c0014808>] (__clk_enable+0x24/0x7c)

>  r4:c06b8648

> [<c00147e4>] (__clk_enable) from [<c0014890>] (clk_enable+0x30/0x50)

>  r4:c06b8648

> [<c0014860>] (clk_enable) from [<c025f43c>]

> (da8xx_usb11_phy_power_on+0x30/0x80)

>  r5:c3a54050 r4:c06b8648

> [<c025f40c>] (da8xx_usb11_phy_power_on) from [<c025ee3c>]

> (phy_power_on+0x80/0xe4)

>  r5:c3a6c800 r4:fffffdf4

> [<c025edbc>] (phy_power_on) from [<c032243c>] (ohci_da8xx_enable+0x4c/0x80)

>  r7:0000003b r6:c3af6000 r5:c3af6000 r4:00000000

> [<c03223f0>] (ohci_da8xx_enable) from [<c03224f0>]

> (ohci_da8xx_reset+0x1c/0xd8)

>  r5:00000000 r4:c3af6000

> [<c03224d4>] (ohci_da8xx_reset) from [<c0309118>] (usb_add_hcd+0x314/0x834)

>  r7:0000003b r6:ffffffed r5:00000000 r4:c3af6000

> [<c0308e04>] (usb_add_hcd) from [<c032200c>] (ohci_da8xx_probe+0x14c/0x21c)

>  r9:c06a5ed4 r8:00000000 r7:c38bd010 r6:c38bd000 r5:c3af6000 r4:c38af900

> [<c0321ec0>] (ohci_da8xx_probe) from [<c02bec44>]

> (platform_drv_probe+0x40/0x8c)

>  r7:c06e610c r6:c06e610c r5:c38bd010 r4:c0321ec0

> [<c02bec04>] (platform_drv_probe) from [<c02bd438>]

> (driver_probe_device+0x138/0x2a4)

>  r7:c06e610c r6:c0e9db10 r5:00000000 r4:c38bd010

> [<c02bd300>] (driver_probe_device) from [<c02bd634>]

> (__driver_attach+0x90/0xb4)

>  r9:c06a5ed4 r8:c06f66e0 r7:c06e1250 r6:c06e610c r5:c38bd044 r4:c38bd010

> [<c02bd5a4>] (__driver_attach) from [<c02bba5c>]

> (bus_for_each_dev+0x74/0x98)

>  r7:c06e1250 r6:c02bd5a4 r5:c06e610c r4:00000000

> [<c02bb9e8>] (bus_for_each_dev) from [<c02bcf2c>] (driver_attach+0x20/0x28)

>  r6:c39a2300 r5:00000000 r4:c06e610c

> [<c02bcf0c>] (driver_attach) from [<c02bca80>] (bus_add_driver+0xd4/0x1f0)

> [<c02bc9ac>] (bus_add_driver) from [<c02bdcec>] (driver_register+0xa4/0xe8)

>  r7:c0698850 r6:00000000 r5:00000000 r4:c06e610c

> [<c02bdc48>] (driver_register) from [<c02bebac>]

> (__platform_driver_register+0x38/0x4c)

>  r5:00000000 r4:c06acce4

> [<c02beb74>] (__platform_driver_register) from [<c068da20>]

> (ohci_da8xx_init+0x64/0x90)

> [<c068d9bc>] (ohci_da8xx_init) from [<c00096c0>]

> (do_one_initcall+0xb0/0x168)

>  r5:c068d9bc r4:ffffe000

> [<c0009610>] (do_one_initcall) from [<c0676e14>]

> (kernel_init_freeable+0x110/0x1cc)

>  r9:c06a5ed4 r8:c06f66e0 r7:c0698850 r6:000000c1 r5:c06f66e0 r4:00000007

> [<c0676d04>] (kernel_init_freeable) from [<c04d37e4>]

> (kernel_init+0x10/0xf8)

>  r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c04d37d4 r4:00000000

> [<c04d37d4>] (kernel_init) from [<c000a2ac>] (ret_from_fork+0x14/0x28)

>  r5:c04d37d4 r4:00000000

> BUG: spinlock lockup suspected on CPU#0, swapper/1

>  lock: clockfw_lock+0x0/0x20, .magic: dead4ead, .owner: swapper/1,

> .owner_cpu: 0

> CPU: 0 PID: 1 Comm: swapper Not tainted 4.9.0-rc8-dlech-ev3-lms2012+ #22

> Hardware name: Generic DA850/OMAP-L138/AM18x

> Backtrace:

> [<c000d824>] (dump_backtrace) from [<c000dac4>] (show_stack+0x18/0x1c)

>  r7:00000000 r6:04605000 r5:c06b09a0 r4:c3838000

> [<c000daac>] (show_stack) from [<c02393dc>] (dump_stack+0x20/0x28)

> [<c02393bc>] (dump_stack) from [<c004f140>] (spin_dump+0x80/0x94)

> [<c004f0c0>] (spin_dump) from [<c004f2c4>] (do_raw_spin_lock+0xdc/0x11c)

>  r5:00000000 r4:c06b09a0

> [<c004f1e8>] (do_raw_spin_lock) from [<c04d8fb4>]

> (_raw_spin_lock_irqsave+0x68/0x74)

>  r10:00000000 r9:c06a5ed4 r8:00000000 r7:0000003b r6:c0014884 r5:80000093

>  r4:c06b09a0 r3:c3838000

> [<c04d8f4c>] (_raw_spin_lock_irqsave) from [<c0014884>]

> (clk_enable+0x24/0x50)

>  r6:c06f69f4 r5:0001ef02 r4:c06b3398

> [<c0014860>] (clk_enable) from [<c0015c74>]

> (usb20_phy_clk_enable+0x24/0xb8)

>  r5:0001ef02 r4:c06f69f0

> [<c0015c50>] (usb20_phy_clk_enable) from [<c0014858>]

> (__clk_enable+0x74/0x7c)

>  r9:c06a5ed4 r8:00000000 r7:0000003b r6:c3a6ca00 r5:80000013 r4:c06b7ee8

> [<c00147e4>] (__clk_enable) from [<c0014808>] (__clk_enable+0x24/0x7c)

>  r4:c06b8648

> [<c00147e4>] (__clk_enable) from [<c0014890>] (clk_enable+0x30/0x50)

>  r4:c06b8648

> [<c0014860>] (clk_enable) from [<c025f43c>]

> (da8xx_usb11_phy_power_on+0x30/0x80)

>  r5:c3a54050 r4:c06b8648

> [<c025f40c>] (da8xx_usb11_phy_power_on) from [<c025ee3c>]

> (phy_power_on+0x80/0xe4)

>  r5:c3a6c800 r4:fffffdf4

> [<c025edbc>] (phy_power_on) from [<c032243c>] (ohci_da8xx_enable+0x4c/0x80)

>  r7:0000003b r6:c3af6000 r5:c3af6000 r4:00000000

> [<c03223f0>] (ohci_da8xx_enable) from [<c03224f0>]

> (ohci_da8xx_reset+0x1c/0xd8)

>  r5:00000000 r4:c3af6000

> [<c03224d4>] (ohci_da8xx_reset) from [<c0309118>] (usb_add_hcd+0x314/0x834)

>  r7:0000003b r6:ffffffed r5:00000000 r4:c3af6000

> [<c0308e04>] (usb_add_hcd) from [<c032200c>] (ohci_da8xx_probe+0x14c/0x21c)

>  r9:c06a5ed4 r8:00000000 r7:c38bd010 r6:c38bd000 r5:c3af6000 r4:c38af900

> [<c0321ec0>] (ohci_da8xx_probe) from [<c02bec44>]

> (platform_drv_probe+0x40/0x8c)

>  r7:c06e610c r6:c06e610c r5:c38bd010 r4:c0321ec0

> [<c02bec04>] (platform_drv_probe) from [<c02bd438>]

> (driver_probe_device+0x138/0x2a4)

>  r7:c06e610c r6:c0e9db10 r5:00000000 r4:c38bd010

> [<c02bd300>] (driver_probe_device) from [<c02bd634>]

> (__driver_attach+0x90/0xb4)

>  r9:c06a5ed4 r8:c06f66e0 r7:c06e1250 r6:c06e610c r5:c38bd044 r4:c38bd010

> [<c02bd5a4>] (__driver_attach) from [<c02bba5c>]

> (bus_for_each_dev+0x74/0x98)

>  r7:c06e1250 r6:c02bd5a4 r5:c06e610c r4:00000000

> [<c02bb9e8>] (bus_for_each_dev) from [<c02bcf2c>] (driver_attach+0x20/0x28)

>  r6:c39a2300 r5:00000000 r4:c06e610c

> [<c02bcf0c>] (driver_attach) from [<c02bca80>] (bus_add_driver+0xd4/0x1f0)

> [<c02bc9ac>] (bus_add_driver) from [<c02bdcec>] (driver_register+0xa4/0xe8)

>  r7:c0698850 r6:00000000 r5:00000000 r4:c06e610c

> [<c02bdc48>] (driver_register) from [<c02bebac>]

> (__platform_driver_register+0x38/0x4c)

>  r5:00000000 r4:c06acce4

> [<c02beb74>] (__platform_driver_register) from [<c068da20>]

> (ohci_da8xx_init+0x64/0x90)

> [<c068d9bc>] (ohci_da8xx_init) from [<c00096c0>]

> (do_one_initcall+0xb0/0x168)

>  r5:c068d9bc r4:ffffe000

> [<c0009610>] (do_one_initcall) from [<c0676e14>]

> (kernel_init_freeable+0x110/0x1cc)

>  r9:c06a5ed4 r8:c06f66e0 r7:c0698850 r6:000000c1 r5:c06f66e0 r4:00000007

> [<c0676d04>] (kernel_init_freeable) from [<c04d37e4>]

> (kernel_init+0x10/0xf8)

>  r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c04d37d4 r4:00000000

> [<c04d37d4>] (kernel_init) from [<c000a2ac>] (ret_from_fork+0x14/0x28)

>  r5:c04d37d4 r4:00000000



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff mbox

Patch

diff --git a/arch/arm/mach-davinci/usb-da8xx.c b/arch/arm/mach-davinci/usb-da8xx.c
index b010e5f..704f506 100644
--- a/arch/arm/mach-davinci/usb-da8xx.c
+++ b/arch/arm/mach-davinci/usb-da8xx.c
@@ -22,6 +22,8 @@ 
 #define DA8XX_USB0_BASE		0x01e00000
 #define DA8XX_USB1_BASE		0x01e25000
 
+static struct clk *usb20_clk;
+
 static struct platform_device da8xx_usb_phy = {
 	.name		= "da8xx-usb-phy",
 	.id		= -1,
@@ -158,21 +160,14 @@  int __init da8xx_register_usb_refclkin(int rate)
 
 static void usb20_phy_clk_enable(struct clk *clk)
 {
-	struct clk *usb20_clk;
 	int err;
 	u32 val;
 	u32 timeout = 500000; /* 500 msec */
 
 	val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
 
-	usb20_clk = clk_get(&da8xx_usb20_dev.dev, "usb20");
-	if (IS_ERR(usb20_clk)) {
-		pr_err("could not get usb20 clk: %ld\n", PTR_ERR(usb20_clk));
-		return;
-	}
-
 	/* The USB 2.O PLL requires that the USB 2.O PSC is enabled as well. */
-	err = clk_prepare_enable(usb20_clk);
+	err = clk_enable(usb20_clk);
 	if (err) {
 		pr_err("failed to enable usb20 clk: %d\n", err);
 		clk_put(usb20_clk);
@@ -197,8 +192,7 @@  static void usb20_phy_clk_enable(struct clk *clk)
 
 	pr_err("Timeout waiting for USB 2.0 PHY clock good\n");
 done:
-	clk_disable_unprepare(usb20_clk);
-	clk_put(usb20_clk);
+	clk_disable(usb20_clk);
 }
 
 static void usb20_phy_clk_disable(struct clk *clk)
@@ -285,11 +279,19 @@  static struct clk_lookup usb20_phy_clk_lookup =
 int __init da8xx_register_usb20_phy_clk(bool use_usb_refclkin)
 {
 	struct clk *parent;
-	int ret = 0;
+	int ret;
+
+	usb20_clk = clk_get(&da8xx_usb20_dev.dev, "usb20");
+	ret = PTR_ERR_OR_ZERO(usb20_clk);
+	if (ret)
+		return ret;
 
 	parent = clk_get(NULL, use_usb_refclkin ? "usb_refclkin" : "pll0_aux");
-	if (IS_ERR(parent))
-		return PTR_ERR(parent);
+	ret = PTR_ERR_OR_ZERO(parent);
+	if (ret) {
+		clk_put(usb20_clk);
+		return ret;
+	}
 
 	usb20_phy_clk.parent = parent;
 	ret = clk_register(&usb20_phy_clk);