Re: [PATCH 1/2] Add a common struct clk

[Sascha Hauer <s.hauer@pengutronix.de>]

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 |