[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <BANLkTimY1cxqaPG1T=_xKPustXR1GSmUKQ@mail.gmail.com>
Date: Tue, 24 May 2011 10:52:53 -0700
From: Colin Cross <ccross@...gle.com>
To: Richard Zhao <linuxzsc@...il.com>
Cc: Jeremy Kerr <jeremy.kerr@...onical.com>,
Thomas Gleixner <tglx@...utronix.de>,
lkml <linux-kernel@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, linux-sh@...r.kernel.org
Subject: Re: [PATCH 0/4] Add a generic struct clk
On Tue, May 24, 2011 at 10:22 AM, Richard Zhao <linuxzsc@...il.com> wrote:
> On Mon, May 23, 2011 at 04:12:24PM -0700, Colin Cross wrote:
>> On Fri, May 20, 2011 at 12:27 AM, Jeremy Kerr <jeremy.kerr@...onical.com> wrote:
>> > [This series was originally titled 'Add a common struct clk', but
>> > the goals have changed since that first set of patches. We're now aiming
>> > for a more complete generic clock infrastructure, rather than just
>> > abstracting struct clk]
>> >
>> > [This series still needs work, see the TODO section below]
>> >
>> > [Totally RFC at the moment]
>> >
>> > Hi all,
>> >
>> > These patches are an attempt to allow platforms to share clock code. At
>> > present, the definitions of 'struct clk' are local to platform code,
>> > which makes allocating and initialising cross-platform clock sources
>> > difficult, and makes it impossible to compile a single image containing
>> > support for two ARM platforms with different struct clks.
>> >
>> > The three patches are for the architecture-independent kernel code,
>> > introducing the common clk infrastructure. The changelog for the first
>> > patch includes details about the new clock definitions.
>> >
>> > The second patch implements clk_set_rate, and in doing so adds
>> > functionality to walk the clock tree in both directions.
>> >
>> > clk_set_parent is left unimplemented, see TODO below for why.
>> >
>> > The third and fourth patches provide some basic clocks (fixed-rate and
>> > gated), mostly to serve as an example of the hardware implementation.
>> > I'm intending to later provide similar base clocks for mux and divider
>> > hardware clocks.
>> >
>> > Many thanks to the following for their input:
>> > * Benjamin Herrenschmidt <benh@...nel.crashing.org>
>> > * Thomas Gleixner <tglx@...utronix.de>
>> > * Ben Dooks <ben-linux@...ff.org>
>> > * Baruch Siach <baruch@...s.co.il>
>> > * Russell King <linux@....linux.org.uk>
>> > * Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
>> > * Lorenzo Pieralisi <Lorenzo.Pieralisi@....com>
>> > * Vincent Guittot <vincent.guittot@...aro.org>
>> > * Sascha Hauer <s.hauer@...gutronix.de>
>> > * Ryan Mallon <ryan@...ewatersys.com>
>> > * Colin Cross <ccross@...gle.com>
>> > * Jassi Brar <jassisinghbrar@...il.com>
>> > * Saravana Kannan <skannan@...eaurora.org>
>>
>> I have a similar set of patches I was working on that handles a little
>> more of the common code than these. I can post them if you want, but
>> for now I'll just point out where I had different ideas, and not muddy
>> the waters with conflicting patches.
>>
>> > TODO:
>> >
>> > * Need to figure out the locking around clk_set_parent. Changing the in-kernel
>> > clock topology requires acquiring both the mutex (to prevent against races
>> > with clk_prepare, which may propagate to the parent clock) and the spinlock
>> > (to prevent the same race with clk_enable).
>> >
>> > However, we should also be changing the hardware clock topology in sync with
>> > the in-kernel clock topology, which would imply that both locks *also* need
>> > to be held while updating the parent in the hardware (ie, in
>> > clk_hw_ops->set_parent) too. However, I believe some platform clock
>> > implementations may require this callback to be able to sleep. Comments?
>>
>> This sequence is the best I could come up with without adding a
>> temporary 2nd parent:
>> 1. lock prepare mutex
> Maybe tell child clocks "I'm going to change clock rate, please stop work if needed"
>> 2. call prepare on the new parent if prepare_count > 0
>> 3. lock the enable spinlock
>> 4. call enable on the new parent if enable_count > 0
>> 5. change the parent pointer to the new parent
>> 6. unlock the spinlock
>> 7. call the set_parent callback
> Why do it need to sleep if it only set some hw registers?
Most implementations don't, and I would be fine with saying
clk_set_parent sleeps, but the set_parent op does not, but that
prevents clock chips on sleeping busses like i2c.
>> 8. lock the enable spinlock
>> 9. call disable on the old parent iff you called enable on the new
>> parent (enable_count may have changed)
>> 10. unlock the enable spinlock
>> 11. call unprepare on the old parent if prepare_count
> propagate rate here and also tell child clocks "rate changed already, change your
> parameters and go on to work".
Yes, propagate rate is needed.
>> 12. unlock prepare mutex
>>
>> The only possible problem I see is that if a clock starts disabled at
>> step 1., and clk_enable is called on it between steps 6 and 7,
>> clk_enable will return having enabled the new parent, but the clock is
>> still running off the old parent. As soon as the clock gets switched
>> to the new parent, the clock will be properly enabled. I don't see
>> this as a huge problem - calling clk_set_parent on a clock while it is
>> enabled may not even work without causing glitches on some platforms.
> some do be glitch free, especially for cpu clock parents.
>>
>> I guess the only way around it would be to store a temporary
>> "new_parent" pointer between steps 5 and 9, and have
>> clk_enable/disable operate on both the current parent and the new
>> parent. They would also need to refcount the extra enables separately
>> to undo on the old parent.
>>
>> > * tglx and I have been discussing different ways of passing clock information
>> > to the clock hardware implementation. I'm proposing providing a base 'struct
>> > clk_hw', which implementations subclass by wrapping in their own structure
>> > (or one of a set of simple 'library' structures, like clk_hw_mux or
>> > clk_hw_gate). The clk_hw base is passed as the first argument to all
>> > clk_hw_ops callbacks.
>> >
>> > tglx's plan is to create a separate struct clk_hwdata, which contains a
>> > union of base data structures for common clocks: div, mux, gate, etc. The
>> > ops callbacks are passed a clk_hw, plus a clk_hwdata, and most of the base
>> > hwdata fields are handled within the core clock code. This means less
>> > encapsulation of clock implementation logic, but more coverage of
>> > clock basics through the core code.
>>
>> I don't think they should be a union, I think there should be 3
>> separate private datas, and three sets of clock ops, for the three
>> different types of clock blocks: rate changers (dividers and plls),
>> muxes, and gates. These blocks are very often combined - a device
>> clock often has a mux and a divider, and clk_set_parent and
>> clk_set_rate on the same struct clk both need to work.
>>
>> > tglx, could you send some details about your approach? I'm aiming to refine
>> > this series with the good bits from each technique.
>> >
>> > * The clock registration code probably needs work. This is the domain
>> > of the platform/board maintainers, any wishes here?
> When clock init, data in struct clk may not be synced with hw registers. After
> enabled minimal needed clk (cpu, core bus etc), we need sync the whole tree,
> disable zero enable_count clocks, set correct .rate ... . The sync function
> is also common code, right? but not have to be called all times I think.
I believe each clock is synced with its hardware during clk_register
by calling the recalc_rate and get_parent callbacks.
> Thanks
> Richard
>> >
>> > Cheers,
>> >
>> >
>> > Jeremy
>> >
>> > --
>> >
>> > ---
>> > Jeremy Kerr (4):
>> > clk: Add a generic clock infrastructure
>> > clk: Implement clk_set_rate
>> > clk: Add fixed-rate clock
>> > clk: Add simple gated clock
>> >
>> > --
>> > 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/
>> >
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@...ts.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
--
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/
Powered by blists - more mailing lists