| Message ID | 1481302773-14181-1-git-send-email-abailon@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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 >
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
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
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 *);
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(-)