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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20110110104122.GI12078@pengutronix.de>
Date:	Mon, 10 Jan 2011 11:41:22 +0100
From:	Sascha Hauer <s.hauer@...gutronix.de>
To:	Jeremy Kerr <jeremy.kerr@...onical.com>
Cc:	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	Ben Herrenchmidt <benh@...nel.crashing.org>,
	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>
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 |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/