Re: [PATCH v4 3/6] clk: introduce the common clock framework
From: Saravana Kannan
Date: Thu Jan 12 2012 - 20:19:25 EST
On 01/12/2012 04:48 PM, Rob Herring wrote:
On 01/12/2012 06:04 PM, Saravana Kannan wrote:
On 01/04/2012 08:07 PM, Turquette, Mike wrote:
On Wed, Jan 4, 2012 at 6:11 PM, Rob Herring<robherring2@xxxxxxxxx>
wrote:
On 01/04/2012 07:01 PM, Turquette, Mike wrote:
On Wed, Jan 4, 2012 at 6:32 AM, Rob Herring<robherring2@xxxxxxxxx>
wrote:
On 01/03/2012 08:15 PM, Richard Zhao wrote:
On Fri, Dec 16, 2011 at 04:45:48PM -0800, Turquette, Mike wrote:
On Wed, Dec 14, 2011 at 5:18 AM, Thomas
Gleixner<tglx@xxxxxxxxxxxxx> wrote:
On Tue, 13 Dec 2011, Mike Turquette wrote:
snip
+/**
+ * clk_init - initialize the data structures in a struct clk
+ * @dev: device initializing this clk, placeholder for now
+ * @clk: clk being initialized
+ *
+ * Initializes the lists in struct clk, queries the hardware
for the
+ * parent and rate and sets them both. Adds the clk to the
sysfs tree
+ * topology.
+ *
+ * Caller must populate clk->name and clk->flags before calling
I'm not too happy about this construct. That leaves struct clk
and its
members exposed to the world instead of making it a real opaque
cookie. I know from my own painful experience, that this will
lead to
random fiddling in that data structure in drivers and arch code
just
because the core code has a shortcoming.
Why can't we make struct clk a real cookie and confine the data
structure to the core code ?
That would change the init call to something like:
struct clk *clk_init(struct device *dev, const struct clk_hw *hw,
struct clk *parent)
And have:
struct clk_hw {
struct clk_hw_ops *ops;
const char *name;
unsigned long flags;
};
Implementers can do:
struct my_clk_hw {
struct clk_hw hw;
mydata;
};
And then change the clk ops callbacks to take struct clk_hw * as an
argument.
We have to define static clocks before we adopt DT binding.
If clk is opaque and allocate memory in clk core, it'll make hard
to define static clocks. And register/init will pass a long parameter
list.
DT is not a prerequisite for having dynamically created clocks. You
can
make clock init dynamic without DT.
Agreed.
What data goes in struct clk vs. struct clk_hw could change over time.
So perhaps we can start with some data in clk_hw and plan to move
it to
struct clk later. Even if almost everything ends up in clk_hw
initially,
at least the structure is in place to have common, core-only data
separate from platform data.
What is the point of this?
To have a way forward. It would be nice to have a clk infrastructure
before I retire...
Haha, agreed.
The original clk_hw was defined simply as:
struct clk_hw {
struct clk *clk;
};
It's only purpose in life was as a handle for navigation between the
opaque struct clk and the hardware-specific struct my_clk_hw. struct
clk_hw is defined in clk.h and everyone can see it. If we're suddenly
OK putting clk data in this structure then why bother with an opaque
struct clk at all?
What is the actual data you need to be static and accessible to the
platform code? A ptr to parent clk is the main thing I've seen for
static initialization. So make the parent ptr be struct clk_hw* and
allow the platforms to access.
To answer your question on what data we're trying to expose: platform
code commonly needs the parent pointer and the clk rate (and by
extension, the rate of the parent). For debug/error prints it is also
nice to have the clk name. Generic clk flags are also conceivably
something that platform code might want.
I agree with the need to have the parent and flags from a static init
perspective. There's not really a good reason the others can't be
accessed thru accessors though.
I'd like to spin the question around: if we're OK exposing some stuff
(in your example above, the parent pointer), then what clk data are
you trying to hide?
Well, everything from drivers which the current clk implementations do
hide. Catching abuse in with drivers coming in from all different trees
and lists will be impossible.
For platform code it is more fuzzy. I don't think platform code should
be allowed to muck with prepare/enable counts for example.
So there is a clear dichotomy: drivers shouldn't be exposed to any of
it and platform code should be exposed to some of it.
How about a drivers/clk/clk-private.h which will define struct clk and
must only be included by clk drivers in drivers/clk/*?
This establishes a bright line between those things which are allowed
to know the details of struct clk and those that are not: namely that
clk drivers in drivers/clk/ may use '#include "clk-private.h"'.
Obviously struct clk is opaque to the rest of the kernel (in the same
way it has been prior to the common clk patches) and there is no need
for struct clk_hw anymore. Also helper functions are no longer needed
for clock driver code, which I think *is* a manageable set of code to
review. Also clk drivers must live in drivers/clk/ for this to work
(without a big ugly path in someone's #include directive somewhere).
While the original clk_hw suggestion was well intentioned, it just
forces too many unnecessary dereferences and indirection. It also
prevents static init of some fields as others have mentioned. Overall,
it made the MSM clock code a mess when I tried to convert it to the
common clock framework during Linaro Connect Q4 2011.
The current off-tree MSM clock code uses a very similar approach to what
the original patches that Jeremy sent out did. When Mike sent out the
patches that removed clk_hw, the MSM code was much clearer and easier to
convert to the common clock framework.
The clk-private.h suggestion by Mike is reasonable seems like a good
compromise. It support the idea of not letting the world peek into the
clock struct (I want this too) while letting the clock driver use the
struct without jumping through hoops. It has my vote (not the whole
patch series, but the idea of having clk driver/framework specific stuff
in clk-private.h). I really hope we move ahead with this.
I'm fine with this approach. We're certainly no worse off as platforms
today have full access. However, it not me that has to be convinced.
Sorry for not being clear. My previous mail was a general comment to the
community and not directed specifically at you Rob.
My suggestion was to build into the data structures at least the option
to have core only and core+platform data. Maybe the core only part is
mostly empty at first. This at least shows some intent to hide some of
the data. Which fields give you pain not having access to them? So far
this mainly seems to be parent and rate.
It's been a while, but if I'm not mistaken, it was messy to statically
initialize the parent field and to tie up the clock specific struct
(say, clk_fixed) to the generic clock struct (clk) without having to
define each of them separately.
With the clk-private.h approach, I could do something like this:
static struct fixed_clk cxo_clk = {
.reg = 0x12345678
.c = {
.dbg_name = "cxo_clk",
.ops = &clk_ops_cxo,
CLK_INIT(cxo_clk.c),
},
}
And without it, it would look something like:
struct clk _cxo_clk;
static struct msm_fixed_clk cxo_clk = {
.reg = 0x12345678
.c.clk = &_cxo_clk,
};
struct clk _cxo_clk = {
.name = "cxo_clk",
.ops = &clk_ops_cxo,
.hw = &cxo_clk.c
};
As you can see, I now have to give a name for a struct that I don't
really care about after init and pollute the name space. It's also
clumsy since both the structs try to reference each other and I have to
use forward declaration for every single clock I try to statically
initialize.
I will also need to do several pointer derefs instead of using
container_of(), etc which is more efficient and less confusing than
multiple levels of pointer deref.
Thanks,
Saravana
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/