lkml.org 
[lkml]   [2011]   [Jan]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/2] Add a common struct clk
On Mon, Jan 10, 2011 at 10:43:04AM +0800, Jeremy Kerr wrote:
> Hi Sascha,
>
> > I'm currently thinking about how to get the locking right with this
> > approach. In the current i.MX implementation we have a global lock which
> > protects the clock enable counter and also the register accesses in the
> > clock code. With the common struct clock we have a lock per clock which
> > only protects the enable counter, so we have to introduce a second lock
> > to protect the register accesses.
>
> Are the registers shared between clocks? If not, you can just use the existing
> per-clk lock. Otherwise it'd be reasonable to add a global register lock,
> protecting accesses to the shared register set (and *only* protecting these
> registers).

Yes, the registers are shared between clocks.

>
> > The problem comes with nested calls to for example clk_enable which
> > happens when the parent clock gets enabled. currently we do this with
> > clk->enable(clk->parent) which results in an unlocked clk_enable of the
> > parent. With common struct clk we would have to call
> > clk_enable(clk_get_parent(clk) which results in taking the lock a second
> > time.
> > Any ideas how to solve this?
>
> With the shared register lock, you just need to make sure that you don't
> recurse to the parent while holding the lock.
>
> For clocks with a shared register set, the general pattern would be something
> like:
>
> struct clk_foo {
> struct clk clk;
> u32 enable_reg;
> u32 enable_mask;
> struct clk *parent;
> };
>
> static DEFINE_SPINLOCK(clk_foo_register_lock);
>
> /* called with _clk->lock held */
> static int clk_foo_enable(struct clk *_clk)
> {
> struct clk_foo *clk = to_clk_foo(_clk);
> int reg, rc;
>
> /* enable parent - will acquire and release the parent's per-clk lock */
> rc = clk_enable(clk->parent);
> if (rc)
> return rc;
>
> /* do register update, under global register lock */
> spin_lock(&clk_foo_register_lock);
>
> reg = __raw_readl(clk->reg);
> __raw_writel(clk->reg, reg | clk->enable_mask);
>
> spin_unlock(&clk_foo_register_lock);
>
> return 0;
> }
>
> struct clk_foo_ops = {
> .enable = clk_foo_enable;
> [...]
> };

Ok, that should do it. Unfortunately this requires pushing the lock down
to the individual functions instead of taking it in some global place.

>
> However, because clk_mxc introduces its own set of abstractions, there may be
> some merging to do here. For my work on mx51, I've done a very basic port:
>
> * changed plat-mxc's struct clk to struct clk_mxc
> * embedded struct clk into struct clk_mxc (ie, making it use the common API)
> * separated some of the simpler clocks to separate types (eg clk_fixed,
> clk_pll, clk_ccgr).
>
> The goal here is to separate all of the clocks into their most basic types,
> leaving no clk_mxc clocks remaining, then the locking should be much simpler.

I am aware of your work. In fact, I already did some work upon this. See

git://git.pengutronix.de/git/imx/linux-2.6.git clk-common

and

git://git.pengutronix.de/git/imx/linux-2.6.git clk-common-wip

The first branch converts the whole i.MX architecture to clk-common.
The second branch converts i.MX51 into the basic building blocks. See
how i.MX51 clock support looks like after the conversion:

http://git.pengutronix.de/?p=imx/linux-2.6.git;a=blob;f=arch/arm/mach-mx5/clock-mx51-mx53.c;h=60c05c9b8916ebcb856f1e3f4e8c419f878c13a3;hb=refs/heads/clk-common-wip

The following shows the basic building blocks I used:

http://git.pengutronix.de/?p=imx/linux-2.6.git;a=commitdiff;h=b2f7ef29c8d56ef3574a5040890ea0edc14dae7f

This branch must be reworked because the correct locking is missing, but
the first branch should be ready for merging once your clk-common
patches are merged. I'll post the patches for review soon. I hope it's
clear soon that your clk-common patches get merged.

Sascha

--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |


\
 
 \ /
  Last update: 2011-01-10 11:43    [W:0.064 / U:0.240 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site