Messages in this thread | | | Date | Mon, 10 Jan 2011 11:41:22 +0100 | From | Sascha Hauer <> | Subject | Re: [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 |
| |