Sekhar Nori <[email protected]> writes:
> Currently, the spinlock in DaVinci clock framework is being
> used to:
>
> 1) Protect clock structure variables usecount and rate against
> concurrent modification.
>
> 2) Protect against simultaneous PSC enables/disables ie.
> serialize davinci_psc_config().
>
> 3) Serialize clk_set_rate():
> i. Prevent simultaneous setting of clock rates
> ii. Ensure clock list remains sane during rate
> propagation (also in clk_set_parent).
>
> Remove the spinlock usage in clock framework by:
>
> 1) Making clock rate and usecount as atomic variables.
>
> 2) Making davinci_psc_config() protect itself instead of
> relying on serialization by caller.
>
> 3) (i) Allowing the clk->set_rate to serialize itself. There
> should be no need to serialize all clock rate settings.
> Currently the only user of rate setting is cpufreq driver on
> DA850. cpufreq naturally serializes the calls to set CPU rate.
> Also, there appears no need to lock IRQs during CPU rate
> transtitions. If required, IRQs can be locked in the actual
> set_rate function.
>
> 3) (ii) Use the mutex already in place for serialzing clock list
> manipulation for serializing clock rate propagation as well.
>
> Apart from the general benefit of reducing locking granurlarity,
> the main motivation behind this change is to enable usage of
> clock framework for off-chip clock synthesizers. One such
> synthesizer, CDCE949, is present on DM6467 EVM. Access to the
> synthesizer happens through I2C bus - accessing which can lead to
> CPU sleep. Having IRQs locked in clk_set_rate prevents the
> clk->set_rate function from sleeping.
>
> Signed-off-by: Sekhar Nori <[email protected]>
I like this cleanup a lot. Applying patches 1-3 to davinci git and queued for
2.6.34.
Kevin
> ---
> arch/arm/mach-davinci/board-dm646x-evm.c | 4 +-
> arch/arm/mach-davinci/clock.c | 74 ++++++++++++-----------------
> arch/arm/mach-davinci/clock.h | 4 +-
> arch/arm/mach-davinci/da830.c | 2 +-
> arch/arm/mach-davinci/da850.c | 4 +-
> arch/arm/mach-davinci/dm355.c | 4 +-
> arch/arm/mach-davinci/dm365.c | 4 +-
> arch/arm/mach-davinci/dm644x.c | 8 ++--
> arch/arm/mach-davinci/dm646x.c | 8 ++--
> arch/arm/mach-davinci/psc.c | 6 ++
> 10 files changed, 56 insertions(+), 62 deletions(-)
>
> diff --git a/arch/arm/mach-davinci/board-dm646x-evm.c
> b/arch/arm/mach-davinci/board-dm646x-evm.c
> index 542bfdb..6ff3411 100644
> --- a/arch/arm/mach-davinci/board-dm646x-evm.c
> +++ b/arch/arm/mach-davinci/board-dm646x-evm.c
> @@ -722,9 +722,9 @@ static __init void davinci_dm646x_evm_irq_init(void)
> void __init dm646x_board_setup_refclk(struct clk *clk)
> {
> if (machine_is_davinci_dm6467tevm())
> - clk->rate = DM6467T_EVM_REF_FREQ;
> + atomic_set(&clk->rate, DM6467T_EVM_REF_FREQ);
> else
> - clk->rate = DM646X_EVM_REF_FREQ;
> + atomic_set(&clk->rate, DM646X_EVM_REF_FREQ);
> }
>
> MACHINE_START(DAVINCI_DM6467_EVM, "DaVinci DM646x EVM")
> diff --git a/arch/arm/mach-davinci/clock.c b/arch/arm/mach-davinci/clock.c
> index baece65..df884c8 100644
> --- a/arch/arm/mach-davinci/clock.c
> +++ b/arch/arm/mach-davinci/clock.c
> @@ -28,7 +28,6 @@
>
> static LIST_HEAD(clocks);
> static DEFINE_MUTEX(clocks_mutex);
> -static DEFINE_SPINLOCK(clockfw_lock);
>
> static unsigned psc_domain(struct clk *clk)
> {
> @@ -41,15 +40,16 @@ static void __clk_enable(struct clk *clk)
> {
> if (clk->parent)
> __clk_enable(clk->parent);
> - if (clk->usecount++ == 0 && (clk->flags & CLK_PSC))
> + if (atomic_read(&clk->usecount) == 0 && (clk->flags & CLK_PSC))
> davinci_psc_config(psc_domain(clk), clk->gpsc, clk->lpsc, 1);
> + atomic_inc(&clk->usecount);
> }
>
> static void __clk_disable(struct clk *clk)
> {
> - if (WARN_ON(clk->usecount == 0))
> + if (WARN_ON(atomic_read(&clk->usecount) == 0))
> return;
> - if (--clk->usecount == 0 && !(clk->flags & CLK_PLL))
> + if (atomic_dec_and_test(&clk->usecount) && !(clk->flags & CLK_PLL))
> davinci_psc_config(psc_domain(clk), clk->gpsc, clk->lpsc, 0);
> if (clk->parent)
> __clk_disable(clk->parent);
> @@ -57,14 +57,10 @@ static void __clk_disable(struct clk *clk)
>
> int clk_enable(struct clk *clk)
> {
> - unsigned long flags;
> -
> if (clk == NULL || IS_ERR(clk))
> return -EINVAL;
>
> - spin_lock_irqsave(&clockfw_lock, flags);
> __clk_enable(clk);
> - spin_unlock_irqrestore(&clockfw_lock, flags);
>
> return 0;
> }
> @@ -72,14 +68,10 @@ EXPORT_SYMBOL(clk_enable);
>
> void clk_disable(struct clk *clk)
> {
> - unsigned long flags;
> -
> if (clk == NULL || IS_ERR(clk))
> return;
>
> - spin_lock_irqsave(&clockfw_lock, flags);
> __clk_disable(clk);
> - spin_unlock_irqrestore(&clockfw_lock, flags);
> }
> EXPORT_SYMBOL(clk_disable);
>
> @@ -88,7 +80,7 @@ unsigned long clk_get_rate(struct clk *clk)
> if (clk == NULL || IS_ERR(clk))
> return -EINVAL;
>
> - return clk->rate;
> + return atomic_read(&clk->rate);
> }
> EXPORT_SYMBOL(clk_get_rate);
>
> @@ -100,7 +92,7 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
> if (clk->round_rate)
> return clk->round_rate(clk, rate);
>
> - return clk->rate;
> + return atomic_read(&clk->rate);
> }
> EXPORT_SYMBOL(clk_round_rate);
>
> @@ -111,28 +103,27 @@ static void propagate_rate(struct clk *root)
>
> list_for_each_entry(clk, &root->children, childnode) {
> if (clk->recalc)
> - clk->rate = clk->recalc(clk);
> + atomic_set(&clk->rate, clk->recalc(clk));
> propagate_rate(clk);
> }
> }
>
> int clk_set_rate(struct clk *clk, unsigned long rate)
> {
> - unsigned long flags;
> int ret = -EINVAL;
>
> if (clk == NULL || IS_ERR(clk))
> return ret;
>
> - spin_lock_irqsave(&clockfw_lock, flags);
> if (clk->set_rate)
> ret = clk->set_rate(clk, rate);
> if (ret == 0) {
> if (clk->recalc)
> - clk->rate = clk->recalc(clk);
> + atomic_set(&clk->rate, clk->recalc(clk));
> + mutex_lock(&clocks_mutex);
> propagate_rate(clk);
> + mutex_unlock(&clocks_mutex);
> }
> - spin_unlock_irqrestore(&clockfw_lock, flags);
>
> return ret;
> }
> @@ -140,26 +131,22 @@ EXPORT_SYMBOL(clk_set_rate);
>
> int clk_set_parent(struct clk *clk, struct clk *parent)
> {
> - unsigned long flags;
> -
> if (clk == NULL || IS_ERR(clk))
> return -EINVAL;
>
> /* Cannot change parent on enabled clock */
> - if (WARN_ON(clk->usecount))
> + if (WARN_ON(atomic_read(&clk->usecount)))
> return -EINVAL;
>
> mutex_lock(&clocks_mutex);
> clk->parent = parent;
> list_del_init(&clk->childnode);
> list_add(&clk->childnode, &clk->parent->children);
> - mutex_unlock(&clocks_mutex);
>
> - spin_lock_irqsave(&clockfw_lock, flags);
> if (clk->recalc)
> - clk->rate = clk->recalc(clk);
> + atomic_set(&clk->rate, clk->recalc(clk));
> propagate_rate(clk);
> - spin_unlock_irqrestore(&clockfw_lock, flags);
> + mutex_unlock(&clocks_mutex);
>
> return 0;
> }
> @@ -170,7 +157,7 @@ int clk_register(struct clk *clk)
> if (clk == NULL || IS_ERR(clk))
> return -EINVAL;
>
> - if (WARN(clk->parent && !clk->parent->rate,
> + if (WARN(clk->parent && !atomic_read(&clk->parent->rate),
> "CLK: %s parent %s has no rate!\n",
> clk->name, clk->parent->name))
> return -EINVAL;
> @@ -184,16 +171,16 @@ int clk_register(struct clk *clk)
> mutex_unlock(&clocks_mutex);
>
> /* If rate is already set, use it */
> - if (clk->rate)
> + if (atomic_read(&clk->rate))
> return 0;
>
> /* Else, see if there is a way to calculate it */
> if (clk->recalc)
> - clk->rate = clk->recalc(clk);
> + atomic_set(&clk->rate, clk->recalc(clk));
>
> /* Otherwise, default to parent rate */
> else if (clk->parent)
> - clk->rate = clk->parent->rate;
> + atomic_set(&clk->rate, atomic_read(&clk->parent->rate));
>
> return 0;
> }
> @@ -219,9 +206,9 @@ static int __init clk_disable_unused(void)
> {
> struct clk *ck;
>
> - spin_lock_irq(&clockfw_lock);
> + mutex_lock(&clocks_mutex);
> list_for_each_entry(ck, &clocks, node) {
> - if (ck->usecount > 0)
> + if (atomic_read(&ck->usecount) > 0)
> continue;
> if (!(ck->flags & CLK_PSC))
> continue;
> @@ -233,7 +220,7 @@ static int __init clk_disable_unused(void)
> pr_info("Clocks: disable unused %s\n", ck->name);
> davinci_psc_config(psc_domain(ck), ck->gpsc, ck->lpsc, 0);
> }
> - spin_unlock_irq(&clockfw_lock);
> + mutex_unlock(&clocks_mutex);
>
> return 0;
> }
> @@ -244,7 +231,7 @@ static unsigned long clk_sysclk_recalc(struct clk *clk)
> {
> u32 v, plldiv;
> struct pll_data *pll;
> - unsigned long rate = clk->rate;
> + unsigned long rate = atomic_read(&clk->rate);
>
> /* If this is the PLL base clock, no more calculations needed */
> if (clk->pll_data)
> @@ -253,7 +240,7 @@ static unsigned long clk_sysclk_recalc(struct clk *clk)
> if (WARN_ON(!clk->parent))
> return rate;
>
> - rate = clk->parent->rate;
> + rate = atomic_read(&clk->parent->rate);
>
> /* Otherwise, the parent must be a PLL */
> if (WARN_ON(!clk->parent->pll_data))
> @@ -281,9 +268,9 @@ static unsigned long clk_sysclk_recalc(struct clk *clk)
> static unsigned long clk_leafclk_recalc(struct clk *clk)
> {
> if (WARN_ON(!clk->parent))
> - return clk->rate;
> + return atomic_read(&clk->rate);
>
> - return clk->parent->rate;
> + return atomic_read(&clk->parent->rate);
> }
>
> static unsigned long clk_pllclk_recalc(struct clk *clk)
> @@ -291,11 +278,11 @@ static unsigned long clk_pllclk_recalc(struct clk *clk)
> u32 ctrl, mult = 1, prediv = 1, postdiv = 1;
> u8 bypass;
> struct pll_data *pll = clk->pll_data;
> - unsigned long rate = clk->rate;
> + unsigned long rate = atomic_read(&clk->rate);
>
> pll->base = IO_ADDRESS(pll->phys_base);
> ctrl = __raw_readl(pll->base + PLLCTL);
> - rate = pll->input_rate = clk->parent->rate;
> + rate = pll->input_rate = atomic_read(&clk->parent->rate);
>
> if (ctrl & PLLCTL_PLLEN) {
> bypass = 0;
> @@ -333,8 +320,8 @@ static unsigned long clk_pllclk_recalc(struct clk *clk)
> rate /= postdiv;
> }
>
> - pr_debug("PLL%d: input = %lu MHz [ ",
> - pll->num, clk->parent->rate / 1000000);
> + pr_debug("PLL%d: input = %u MHz [ ",
> + pll->num, atomic_read(&clk->parent->rate) / 1000000);
> if (bypass)
> pr_debug("bypass ");
> if (prediv > 1)
> @@ -452,7 +439,7 @@ int __init davinci_clk_init(struct davinci_clk *clocks)
> }
>
> if (clk->recalc)
> - clk->rate = clk->recalc(clk);
> + atomic_set(&clk->rate, clk->recalc(clk));
>
> if (clk->lpsc)
> clk->flags |= CLK_PSC;
> @@ -514,7 +501,8 @@ dump_clock(struct seq_file *s, unsigned nest, struct clk
> *parent)
> min(i, (unsigned)(sizeof(buf) - 1 - nest)));
>
> seq_printf(s, "%s users=%2d %-3s %9ld Hz\n",
> - buf, parent->usecount, state, clk_get_rate(parent));
> + buf, atomic_read(&parent->usecount), state,
> + clk_get_rate(parent));
> /* REVISIT show device associations too */
>
> /* cost is now small, but not linear... */
> diff --git a/arch/arm/mach-davinci/clock.h b/arch/arm/mach-davinci/clock.h
> index c92d77a..796a568 100644
> --- a/arch/arm/mach-davinci/clock.h
> +++ b/arch/arm/mach-davinci/clock.h
> @@ -67,8 +67,8 @@ struct clk {
> struct list_head node;
> struct module *owner;
> const char *name;
> - unsigned long rate;
> - u8 usecount;
> + atomic_t rate;
> + atomic_t usecount;
> u8 lpsc;
> u8 gpsc;
> u32 flags;
> diff --git a/arch/arm/mach-davinci/da830.c b/arch/arm/mach-davinci/da830.c
> index b22b5cf..d1961d2 100644
> --- a/arch/arm/mach-davinci/da830.c
> +++ b/arch/arm/mach-davinci/da830.c
> @@ -43,7 +43,7 @@ static struct pll_data pll0_data = {
>
> static struct clk ref_clk = {
> .name = "ref_clk",
> - .rate = DA830_REF_FREQ,
> + .rate = ATOMIC_INIT(DA830_REF_FREQ),
> };
>
> static struct clk pll0_clk = {
> diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
> index 717806c..62f8c22 100644
> --- a/arch/arm/mach-davinci/da850.c
> +++ b/arch/arm/mach-davinci/da850.c
> @@ -54,7 +54,7 @@ static struct pll_data pll0_data = {
>
> static struct clk ref_clk = {
> .name = "ref_clk",
> - .rate = DA850_REF_FREQ,
> + .rate = ATOMIC_INIT(DA850_REF_FREQ),
> };
>
> static struct clk pll0_clk = {
> @@ -1024,7 +1024,7 @@ static int da850_set_pll0rate(struct clk *clk, unsigned
> long armrate)
>
> static int da850_round_armrate(struct clk *clk, unsigned long rate)
> {
> - return clk->rate;
> + return atomic_read(&clk->rate);
> }
> #endif
>
> diff --git a/arch/arm/mach-davinci/dm355.c b/arch/arm/mach-davinci/dm355.c
> index dedf4d4..2244e8c 100644
> --- a/arch/arm/mach-davinci/dm355.c
> +++ b/arch/arm/mach-davinci/dm355.c
> @@ -55,7 +55,7 @@ static struct pll_data pll2_data = {
> static struct clk ref_clk = {
> .name = "ref_clk",
> /* FIXME -- crystal rate is board-specific */
> - .rate = DM355_REF_FREQ,
> + .rate = ATOMIC_INIT(DM355_REF_FREQ),
> };
>
> static struct clk pll1_clk = {
> @@ -314,7 +314,7 @@ static struct clk timer2_clk = {
> .name = "timer2",
> .parent = &pll1_aux_clk,
> .lpsc = DAVINCI_LPSC_TIMER2,
> - .usecount = 1, /* REVISIT: why cant' this be disabled? */
> + .usecount = ATOMIC_INIT(1), /* REVISIT: why cant' this be disabled? */
> };
>
> static struct clk timer3_clk = {
> diff --git a/arch/arm/mach-davinci/dm365.c b/arch/arm/mach-davinci/dm365.c
> index 2ec619e..cc3bae4 100644
> --- a/arch/arm/mach-davinci/dm365.c
> +++ b/arch/arm/mach-davinci/dm365.c
> @@ -52,7 +52,7 @@ static struct pll_data pll2_data = {
>
> static struct clk ref_clk = {
> .name = "ref_clk",
> - .rate = DM365_REF_FREQ,
> + .rate = ATOMIC_INIT(DM365_REF_FREQ),
> };
>
> static struct clk pll1_clk = {
> @@ -358,7 +358,7 @@ static struct clk timer2_clk = {
> .name = "timer2",
> .parent = &pll1_aux_clk,
> .lpsc = DAVINCI_LPSC_TIMER2,
> - .usecount = 1,
> + .usecount = ATOMIC_INIT(1),
> };
>
> static struct clk timer3_clk = {
> diff --git a/arch/arm/mach-davinci/dm644x.c b/arch/arm/mach-davinci/dm644x.c
> index 2cd0081..e65e29e 100644
> --- a/arch/arm/mach-davinci/dm644x.c
> +++ b/arch/arm/mach-davinci/dm644x.c
> @@ -47,7 +47,7 @@ static struct pll_data pll2_data = {
>
> static struct clk ref_clk = {
> .name = "ref_clk",
> - .rate = DM644X_REF_FREQ,
> + .rate = ATOMIC_INIT(DM644X_REF_FREQ),
> };
>
> static struct clk pll1_clk = {
> @@ -131,7 +131,7 @@ static struct clk dsp_clk = {
> .parent = &pll1_sysclk1,
> .lpsc = DAVINCI_LPSC_GEM,
> .flags = PSC_DSP,
> - .usecount = 1, /* REVISIT how to disable? */
> + .usecount = ATOMIC_INIT(1), /* REVISIT how to disable? */
> };
>
> static struct clk arm_clk = {
> @@ -146,7 +146,7 @@ static struct clk vicp_clk = {
> .parent = &pll1_sysclk2,
> .lpsc = DAVINCI_LPSC_IMCOP,
> .flags = PSC_DSP,
> - .usecount = 1, /* REVISIT how to disable? */
> + .usecount = ATOMIC_INIT(1), /* REVISIT how to disable? */
> };
>
> static struct clk vpss_master_clk = {
> @@ -274,7 +274,7 @@ static struct clk timer2_clk = {
> .name = "timer2",
> .parent = &pll1_aux_clk,
> .lpsc = DAVINCI_LPSC_TIMER2,
> - .usecount = 1, /* REVISIT: why cant' this be disabled? */
> + .usecount = ATOMIC_INIT(1), /* REVISIT: why cant' this be disabled? */
> };
>
> struct davinci_clk dm644x_clks[] = {
> diff --git a/arch/arm/mach-davinci/dm646x.c b/arch/arm/mach-davinci/dm646x.c
> index 515d3ed..6f80616 100644
> --- a/arch/arm/mach-davinci/dm646x.c
> +++ b/arch/arm/mach-davinci/dm646x.c
> @@ -60,7 +60,7 @@ static struct clk ref_clk = {
>
> static struct clk aux_clkin = {
> .name = "aux_clkin",
> - .rate = DM646X_AUX_FREQ,
> + .rate = ATOMIC_INIT(DM646X_AUX_FREQ),
> };
>
> static struct clk pll1_clk = {
> @@ -158,7 +158,7 @@ static struct clk dsp_clk = {
> .parent = &pll1_sysclk1,
> .lpsc = DM646X_LPSC_C64X_CPU,
> .flags = PSC_DSP,
> - .usecount = 1, /* REVISIT how to disable? */
> + .usecount = ATOMIC_INIT(1), /* REVISIT how to disable? */
> };
>
> static struct clk arm_clk = {
> @@ -262,14 +262,14 @@ static struct clk pwm0_clk = {
> .name = "pwm0",
> .parent = &pll1_sysclk3,
> .lpsc = DM646X_LPSC_PWM0,
> - .usecount = 1, /* REVIST: disabling hangs system */
> + .usecount = ATOMIC_INIT(1), /* REVIST: disabling hangs system */
> };
>
> static struct clk pwm1_clk = {
> .name = "pwm1",
> .parent = &pll1_sysclk3,
> .lpsc = DM646X_LPSC_PWM1,
> - .usecount = 1, /* REVIST: disabling hangs system */
> + .usecount = ATOMIC_INIT(1), /* REVIST: disabling hangs system */
> };
>
> static struct clk timer0_clk = {
> diff --git a/arch/arm/mach-davinci/psc.c b/arch/arm/mach-davinci/psc.c
> index 04a3cb7..a9a4d08 100644
> --- a/arch/arm/mach-davinci/psc.c
> +++ b/arch/arm/mach-davinci/psc.c
> @@ -21,6 +21,7 @@
> #include <linux/kernel.h>
> #include <linux/init.h>
> #include <linux/io.h>
> +#include <linux/spinlock.h>
>
> #include <mach/cputype.h>
> #include <mach/psc.h>
> @@ -64,6 +65,9 @@ void davinci_psc_config(unsigned int domain, unsigned int
> ctlr,
> void __iomem *psc_base;
> struct davinci_soc_info *soc_info = &davinci_soc_info;
> u32 next_state = enable ? 0x3 : 0x2; /* 0x3 enables, 0x2 disables */
> + /* Protect against simultaneous enable/disable of PSCs */
> + DEFINE_SPINLOCK(lock);
> + unsigned long flags;
>
> if (!soc_info->psc_bases || (ctlr >= soc_info->psc_bases_num)) {
> pr_warning("PSC: Bad psc data: 0x%x[%d]\n",
> @@ -73,6 +77,7 @@ void davinci_psc_config(unsigned int domain, unsigned int
> ctlr,
>
> psc_base = soc_info->psc_bases[ctlr];
>
> + spin_lock_irqsave(&lock, flags);
> mdctl = __raw_readl(psc_base + MDCTL + 4 * id);
> mdctl &= ~MDSTAT_STATE_MASK;
> mdctl |= next_state;
> @@ -111,4 +116,5 @@ void davinci_psc_config(unsigned int domain, unsigned int
> ctlr,
> do {
> mdstat = __raw_readl(psc_base + MDSTAT + 4 * id);
> } while (!((mdstat & MDSTAT_STATE_MASK) == next_state));
> + spin_unlock_irqrestore(&lock, flags);
> }
> --
> 1.6.2.4
>
> _______________________________________________
> Davinci-linux-open-source mailing list
> [email protected]
> http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
