[PATCH v7 1/2] provide lock-less versions of clk_{enable|disable}

[PATCH v7 1/2] provide lock-less versions of clk_{enable|disable}

[Alexandre Bailon]

Rename __clk_{enable|disble} in davinci_clk_{enable|disable}.
davinci_clk_{enable|disable} is a lock-less version of
clk_{enable|disable}.
This is useful to recursively enable clock without doing recursive call
to clk_enable(), which would cause a recursive locking issue.
The lock-less version could be used by example by the usb20 phy clock,
that need to enable the usb20 clock before to start.

Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
Suggested-by: David Lechner <david@lechnology.com>
---
 arch/arm/mach-davinci/clock.c | 12 ++++++------
 arch/arm/mach-davinci/clock.h |  2 ++
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-davinci/clock.c b/arch/arm/mach-davinci/clock.c
index df42c93..f5dce9b 100644
--- a/arch/arm/mach-davinci/clock.c
+++ b/arch/arm/mach-davinci/clock.c
@@ -31,10 +31,10 @@ static LIST_HEAD(clocks);
 static DEFINE_MUTEX(clocks_mutex);
 static DEFINE_SPINLOCK(clockfw_lock);
 
-static void __clk_enable(struct clk *clk)
+void davinci_clk_enable(struct clk *clk)
 {
 	if (clk->parent)
-		__clk_enable(clk->parent);
+		davinci_clk_enable(clk->parent);
 	if (clk->usecount++ == 0) {
 		if (clk->flags & CLK_PSC)
 			davinci_psc_config(clk->domain, clk->gpsc, clk->lpsc,
@@ -44,7 +44,7 @@ static void __clk_enable(struct clk *clk)
 	}
 }
 
-static void __clk_disable(struct clk *clk)
+void davinci_clk_disable(struct clk *clk)
 {
 	if (WARN_ON(clk->usecount == 0))
 		return;
@@ -56,7 +56,7 @@ static void __clk_disable(struct clk *clk)
 			clk->clk_disable(clk);
 	}
 	if (clk->parent)
-		__clk_disable(clk->parent);
+		davinci_clk_disable(clk->parent);
 }
 
 int davinci_clk_reset(struct clk *clk, bool reset)
@@ -103,7 +103,7 @@ int clk_enable(struct clk *clk)
 		return -EINVAL;
 
 	spin_lock_irqsave(&clockfw_lock, flags);
-	__clk_enable(clk);
+	davinci_clk_enable(clk);
 	spin_unlock_irqrestore(&clockfw_lock, flags);
 
 	return 0;
@@ -118,7 +118,7 @@ void clk_disable(struct clk *clk)
 		return;
 
 	spin_lock_irqsave(&clockfw_lock, flags);
-	__clk_disable(clk);
+	davinci_clk_disable(clk);
 	spin_unlock_irqrestore(&clockfw_lock, flags);
 }
 EXPORT_SYMBOL(clk_disable);
diff --git a/arch/arm/mach-davinci/clock.h b/arch/arm/mach-davinci/clock.h
index e2a5437..fa2b837 100644
--- a/arch/arm/mach-davinci/clock.h
+++ b/arch/arm/mach-davinci/clock.h
@@ -132,6 +132,8 @@ int davinci_set_sysclk_rate(struct clk *clk, unsigned long rate);
 int davinci_set_refclk_rate(unsigned long rate);
 int davinci_simple_set_rate(struct clk *clk, unsigned long rate);
 int davinci_clk_reset(struct clk *clk, bool reset);
+void davinci_clk_enable(struct clk *clk);
+void davinci_clk_disable(struct clk *clk);
 
 extern struct platform_device davinci_wdt_device;
 extern void davinci_watchdog_reset(struct platform_device *);
-- 
2.7.3

[PATCH v7 2/2] ARM: davinci: da8xx: Fix sleeping function called from invalid context

[Alexandre Bailon]

Everytime the usb20 phy is enabled, there is a
"sleeping function called from invalid context" BUG.
In addition, there is a recursive locking happening
because of the recurse call to clk_enable().

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.
replace clk_prepare_enable() by davinci_clk_enable().

Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
Suggested-by: David Lechner <david@lechnology.com>
---
 arch/arm/mach-davinci/usb-da8xx.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/arch/arm/mach-davinci/usb-da8xx.c b/arch/arm/mach-davinci/usb-da8xx.c
index b010e5f..71f1e81 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,26 +160,13 @@ 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);
-	if (err) {
-		pr_err("failed to enable usb20 clk: %d\n", err);
-		clk_put(usb20_clk);
-		return;
-	}
+	davinci_clk_enable(usb20_clk);
 
 	/*
 	 * Turn on the USB 2.0 PHY, but just the PLL, and not OTG. The USB 1.1
@@ -197,8 +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_unprepare(usb20_clk);
-	clk_put(usb20_clk);
+	davinci_clk_disable(usb20_clk);
 }
 
 static void usb20_phy_clk_disable(struct clk *clk)
@@ -285,11 +273,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);
-- 
2.7.3

[PATCH v7 1/2] provide lock-less versions of clk_{enable|disable}

[Uwe Kleine-König]

Hello,

On Fri, Dec 09, 2016 at 05:59:32PM +0100, Alexandre Bailon wrote:
> Rename __clk_{enable|disble} in davinci_clk_{enable|disable}.
> davinci_clk_{enable|disable} is a lock-less version of
> clk_{enable|disable}.
> This is useful to recursively enable clock without doing recursive call
> to clk_enable(), which would cause a recursive locking issue.
> The lock-less version could be used by example by the usb20 phy clock,
> that need to enable the usb20 clock before to start.

I wouldn't call that lock-less. The difference is that the newly exposed
funcion requires the caller to already hold the lock. So maybe a better
name would be clk_enable_locked.

Would it be more sensible to convert davinci to common-clk?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH v7 1/2] provide lock-less versions of clk_{enable|disable}

[Sekhar Nori]

Hi Uwe,

On Saturday 10 December 2016 12:24 AM, Uwe Kleine-K?nig wrote:
> Hello,
> 
> On Fri, Dec 09, 2016 at 05:59:32PM +0100, Alexandre Bailon wrote:
>> Rename __clk_{enable|disble} in davinci_clk_{enable|disable}.
>> davinci_clk_{enable|disable} is a lock-less version of
>> clk_{enable|disable}.
>> This is useful to recursively enable clock without doing recursive call
>> to clk_enable(), which would cause a recursive locking issue.
>> The lock-less version could be used by example by the usb20 phy clock,
>> that need to enable the usb20 clock before to start.
> 
> I wouldn't call that lock-less. The difference is that the newly exposed
> funcion requires the caller to already hold the lock. So maybe a better

Right.

> name would be clk_enable_locked.

I am not sure the new name you propose is much clearer. Unless I am
missing an obvious naming pattern in kernel. Besides, I want to have the
"davinci_" prefix for consistency with how other mach-davinci internal
functions are named. FWIW, the equivalent function in common-clk is
called clk_core_enable().

> 
> Would it be more sensible to convert davinci to common-clk?

Yes, but there is no work going on on that and AFAIK, know one is
planning on working on it too. These patches are needed anyway since we
need to fix the existing issue on v4.10

Thanks,
Sekhar

[PATCH v7 1/2] provide lock-less versions of clk_{enable|disable}

[Alexandre Bailon]

On 12/12/2016 10:02 AM, Sekhar Nori wrote:
> Hi Uwe,
> 
> On Saturday 10 December 2016 12:24 AM, Uwe Kleine-K?nig wrote:
>> Hello,
>>
>> On Fri, Dec 09, 2016 at 05:59:32PM +0100, Alexandre Bailon wrote:
>>> Rename __clk_{enable|disble} in davinci_clk_{enable|disable}.
>>> davinci_clk_{enable|disable} is a lock-less version of
>>> clk_{enable|disable}.
>>> This is useful to recursively enable clock without doing recursive call
>>> to clk_enable(), which would cause a recursive locking issue.
>>> The lock-less version could be used by example by the usb20 phy clock,
>>> that need to enable the usb20 clock before to start.
>>
>> I wouldn't call that lock-less. The difference is that the newly exposed
>> funcion requires the caller to already hold the lock. So maybe a better
> 
> Right.
Is it enough to update the commit message or should I also update the
patch title?
If so, could you suggest one because I don't know how to describe it
shortly.

Thanks,
Alexandre

[PATCH v7 1/2] provide lock-less versions of clk_{enable|disable}

[David Lechner]

On 12/12/2016 04:27 AM, Alexandre Bailon wrote:
> On 12/12/2016 10:02 AM, Sekhar Nori wrote:
>> Hi Uwe,
>>
>> On Saturday 10 December 2016 12:24 AM, Uwe Kleine-K?nig wrote:
>>> Hello,
>>>
>>> On Fri, Dec 09, 2016 at 05:59:32PM +0100, Alexandre Bailon wrote:
>>>> Rename __clk_{enable|disble} in davinci_clk_{enable|disable}.
>>>> davinci_clk_{enable|disable} is a lock-less version of
>>>> clk_{enable|disable}.
>>>> This is useful to recursively enable clock without doing recursive call
>>>> to clk_enable(), which would cause a recursive locking issue.
>>>> The lock-less version could be used by example by the usb20 phy clock,
>>>> that need to enable the usb20 clock before to start.
>>>
>>> I wouldn't call that lock-less. The difference is that the newly exposed
>>> funcion requires the caller to already hold the lock. So maybe a better
>>
>> Right.
> Is it enough to update the commit message or should I also update the
> patch title?
> If so, could you suggest one because I don't know how to describe it
> shortly.

How about... "ARM: davinci: Make __clk_{enable,disable} functions public"





>
> Thanks,
> Alexandre
>

[PATCH v7 1/2] provide lock-less versions of clk_{enable|disable}

[Sekhar Nori]

On Saturday 17 December 2016 02:11 AM, David Lechner wrote:
> On 12/12/2016 04:27 AM, Alexandre Bailon wrote:
>> On 12/12/2016 10:02 AM, Sekhar Nori wrote:
>>> Hi Uwe,
>>>
>>> On Saturday 10 December 2016 12:24 AM, Uwe Kleine-K?nig wrote:
>>>> Hello,
>>>>
>>>> On Fri, Dec 09, 2016 at 05:59:32PM +0100, Alexandre Bailon wrote:
>>>>> Rename __clk_{enable|disble} in davinci_clk_{enable|disable}.
>>>>> davinci_clk_{enable|disable} is a lock-less version of
>>>>> clk_{enable|disable}.
>>>>> This is useful to recursively enable clock without doing recursive
>>>>> call
>>>>> to clk_enable(), which would cause a recursive locking issue.
>>>>> The lock-less version could be used by example by the usb20 phy clock,
>>>>> that need to enable the usb20 clock before to start.
>>>>
>>>> I wouldn't call that lock-less. The difference is that the newly
>>>> exposed
>>>> funcion requires the caller to already hold the lock. So maybe a better
>>>
>>> Right.
>> Is it enough to update the commit message or should I also update the
>> patch title?
>> If so, could you suggest one because I don't know how to describe it
>> shortly.
> 
> How about... "ARM: davinci: Make __clk_{enable,disable} functions public"

Looks fine to me, here is the updated subject line and commit text. No
need to resend just for this, I can adjust when applying.

"
ARM: davinci: Make __clk_{enable,disable} functions public

In some cases, there is a need to enable a clock as part of clock
enable callback of a different clock. For example, USB 2.0 PHY clock
enable requires USB 2.0 clock to be enabled. In this case, it is safe to
instead call  __clk_enable() since the clock framework lock is already
taken. calling clk_enable() causes recursive locking error.

A similar case arises in the clock disable path.

To enable such usage, make __clk_{enable,disable} functions publicly
available outside of clock.c. Also, call them
davinci_clk_{enable|disable} now to be consistent with how other
davinci-specific clock functions are named.

Note that these functions are not exported to drivers. They are meant
for usage in platform specific clock management code.
"

Thanks,
Sekhar

[PATCH v7 1/2] provide lock-less versions of clk_{enable|disable}

[Sekhar Nori]

On Sunday 18 December 2016 09:21 PM, Sekhar Nori wrote:
> ARM: davinci: Make __clk_{enable,disable} functions public
> 
> In some cases, there is a need to enable a clock as part of clock
> enable callback of a different clock. For example, USB 2.0 PHY clock
> enable requires USB 2.0 clock to be enabled. In this case, it is safe to
> instead call  __clk_enable() since the clock framework lock is already
> taken. calling clk_enable() causes recursive locking error.
> 
> A similar case arises in the clock disable path.
> 
> To enable such usage, make __clk_{enable,disable} functions publicly
> available outside of clock.c. Also, call them
> davinci_clk_{enable|disable} now to be consistent with how other
> davinci-specific clock functions are named.
> 
> Note that these functions are not exported to drivers. They are meant
> for usage in platform specific clock management code.

Applied to 'fixes' with above commit text.

Thanks,
Sekhar

[PATCH v7 2/2] ARM: davinci: da8xx: Fix sleeping function called from invalid context

[Sekhar Nori]

On Friday 09 December 2016 10:29 PM, Alexandre Bailon wrote:
> Everytime the usb20 phy is enabled, there is a
> "sleeping function called from invalid context" BUG.
> In addition, there is a recursive locking happening
> because of the recurse call to clk_enable().
> 
> 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.
> replace clk_prepare_enable() by davinci_clk_enable().
> 
> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
> Suggested-by: David Lechner <david@lechnology.com>

Applied to 'fixes' branch.

Thanks,
Sekhar