Re: [PATCH v5 3/4] clk: introduce the common clock framework

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

On 03/07/2012 01:20 PM, Turquette, Mike wrote:
On Tue, Mar 6, 2012 at 11:00 AM, Sascha Hauer<s.hauer@xxxxxxxxxxxxxx>  wrote:
On Mon, Mar 05, 2012 at 12:03:15PM -0800, Turquette, Mike wrote:
On Sun, Mar 4, 2012 at 11:38 PM, Sascha Hauer<s.hauer@xxxxxxxxxxxxxx>  wrote:
On Sun, Mar 04, 2012 at 04:12:21PM -0800, Turquette, Mike wrote:

I believe this patch already does what you suggest, but I might be
missing your point.

In include/linux/clk-private.h you expose struct clk outside the core.
This has to be done to make static initializers possible. There is a big
warning in this file that it must not be included from files implementing
struct clk_ops. You can simply avoid this warning by declaring struct clk
with only a single member:

include/linux/clk.h:

struct clk {
        struct clk_internal *internal;
};

This way everybody knows struct clk (thus can embed it in their static
initializers), but doesn't know anything about the internal members. Now
in drivers/clk/clk.c you declare struct clk_internal exactly like struct
clk was declared before:

struct clk_internal {
        const char              *name;
        const struct clk_ops    *ops;
        struct clk_hw           *hw;
        struct clk              *parent;
        char                    **parent_names;
        struct clk              **parents;
        u8                      num_parents;
        unsigned long           rate;
        unsigned long           flags;
        unsigned int            enable_count;
        unsigned int            prepare_count;
        struct hlist_head       children;
        struct hlist_node       child_node;
        unsigned int            notifier_count;
#ifdef CONFIG_COMMON_CLK_DEBUG
        struct dentry           *dentry;
#endif
};

An instance of struct clk_internal will be allocated in
__clk_init/clk_register. Now the private data stays completely inside
the core and noone can abuse it.

Hi Sascha,

I see the disconnect here.  For OMAP (and possibly other platforms) at
least some clock data is necessary during early boot, before the
regular allocation methods are available (timers for instance).

We had this problem on i.MX aswell. It turned out that the timer clock
is the only clock that is needed so early. We solved this by moving the
clock init to the system timer init function.

When you say "mov[ed] the clock init to the system timer init
function" do you mean that you statically allocated struct clk and
used the clk framework api, or instead you just did some direct
register writes to initialize things properly?

I meant that on i.MX we do the clock tree initialization when kmalloc is
available, see the attached patch for omap4 based on your branch which
does the same for Omap. The first clock you need is the one for the
timer, so you can initialize the clocktree at the beginning of
time_init() and don't need statically initialized clocks anymore.


Well, the file is work in progress, you probably fix this before sending
it out, but I bet people will include clk-private.h and nobody else
notices it.

clock44xx_data.c does not violate that rule.  None of the logic that
implements ops for those clocks is present clock44xx_data.c.

Indeed, I missed that. It only has the ops but not the individual
functions.

All of
the code in that file is simply initialization and registration of
OMAP4 clocks.  Many of the clocks are basic clock types (divider,
multiplexer and fixed-rate are used in that file) with protected code
drivers/clk/clk-*.c and the remaining clocks are of the struct
clk_hw_omap variety, which has code spread across several files:

arch/arm/mach-omap2/clock.c
arch/arm/mach-omap2/clock.h
arch/arm/mach-omap2/clkt_dpll.c
arch/arm/mach-omap2/clkt_clksel.c
arch/arm/mach-omap2/dpll3xxx.c
arch/arm/mach-omap2/dpll4xxx.c

All of the above files include linux/clk-provider.h, not
linux/clk-private.h.  That code makes heavy use of the
__clk_get_whatever helpers and shows how a platform might honor the
layer of separation between struct clk and stuct clk_ops/struct
clk_foo.  You are correct that the code is a work-in-progress, but
there are no layering violations that I can see.

I also think we are talking past each other to some degree.  One point
I would like to make (and maybe you already know this from code
review) is that it is unnecessary to have pointers to your parent
struct clk*'s when either initializing or registering your clocks.  In
fact the existing clk_register_foo functions don't even allow you to
pass in parent pointers and rely wholly on string name matching.  I
just wanted to point that out in case it went unnoticed, as it is a
new way of doing things from the previous series and was born out of
Thomas' review of V4 and multi-parent handling.  This also keeps
device-tree in mind where we might not know the struct clk *pointer at
compile time for "connecting" discrete devices.

Yes, I've seen this and I really like it. Also the change that
multiplexers return an index to an array instead of the parent
clock is very nice.

Sascha


8<-----------------------------------------------------

ARM omap4: move clocktree init to timer init time so we don't need static clocks

Signed-off-by: Sascha Hauer<s.hauer@xxxxxxxxxxxxxx>


Hi Mike,

I already took a quick look at the v7 series, but I thought this thread has more relevant context for my response. So, responding here.

I'm with Sascha on creating a clk_internal/clk_initializer and removing clk_hw. You had asked about the benefits of his suggestion in one of the earlier threads. I'm sure I have told some of these reasons I don't like clk_hw, but repeating my points here for others to chime in.

I used to be a huge proponent for using macros for clocks in our internal tree. All the clocks were constructed using macros (you will see it in the history of tree we publish in CAF). They quickly became unreadable when you have several hundreds of clocks. The biggest problem is that you can't quickly look at a clock's macro and figure out what the register offset or bit mask or shift value is. Our eyes/brains aren't meant for quickly parsing the commas and finding the n-th field or even remembering what the n-th field of each macro corresponds to. If it's actually broken out as fields in a struct, it's much easier to read. So, long story short, I think the well-intentioned helper macros will make code quite unreadable.

I also don't want to maintain such helper macros for the platform specific clock types that I will have to create for MSM if I were to go with macros.

If I choose not to use the helper macros, then I need to define three separate declarations for each clock. That looks messy when you have 100s of clocks.

With clk_initializer, we also don't have to keep changing the clk_init/clk_register APIs whenever we add any other optional fields in the future. You just add it to clk_initializer and if people care for those fields, they can add it to the static/dynamic clocks.

Whether we use a macro or not, the current approach also adds twice the the no. of symbols in the debug info and increases the size of the kernel. When we go for a single ARM kernel, this is going to be noticeable. I agree that the point about debuginfo size increase might not matter to most.

Hi Sascha,

Thanks for the example code.  This is in fact something I had
considered doing before, but it is a bit complicated for my platform.

We set up all of the OMAP clocks with __clk_init because this is a
dependency of OMAP's hwmod code which models and interacts with
specific resources for hardware modules, such as clocks.  There are
other dependencies such as our clockdomain code, which needs the
clocks to be fully initialized.  Some aspects of these layers get
init'd before we can allocate memory.

I know there is a version of OMAP hwmod in upstream, but is this dependency of fully initialized clock struct a requirement for the upstream version of hwmod or an internal OMAP tree? I will explain the reason for this question further down.

Admittedly I think that the OMAP code could migrate some of these bits
to a lazy-registration model, specifically the hwmod object instances,
but that requires an awful lot of refactoring for a fairly large stack
of platform code.  This might be something to achieve in the future
but for now we *need* initialisation to be fully static.

When we work on moving clocks to device tree, wouldn't you face the same problem? You will have to dynamically create most of the clocks in that case too.

Assuming that some day OMAP code can be refactored to allow for lazy
(or at least initcall-based) registration of clocks then perhaps your
suggestion can take root.  Which leads me to this question: are there
any other platforms out there that require the level of expose to
struct clk present in this patchset?  OMAP does, for now, but if that
changes then I need to know if others require this as well.

MSM platform doesn't need exposure to clk_internal or the corresponding fields in clk. We too have the timer issue, but I would rather deal with the timer separately (using register writes from the timer code) than ugly up the 200+ clocks we have on each SoC.

Getting to suggesting a solution, how about we go with something similar to Sasha's suggestion:
* Put clk_inititalizer in clk-provider.h
* clk-provider.h has a forward declaration to clk (struct clk;)
* Put struct clk and struct clk_internal in a clk-private.h or whatever.
* Add a struct *clk field to struct clk_initializer (this is temporary).


For platforms that don't have the static init limitations you mention, they can do something like this:

#include <linux/clk-provider.h>

struct clk_complex {
	struct clk_initializer i;
	u32 foo;
	u32 bar;
	u8 baz;
};

struct clk_complex mmc1_clk {
	foo = 5,
	bar = 10,
	baz = 3,
	. i = {
		.name = mmc,
		.ops = &complex_ops,
	}
};

clk_register(NULL, &mmc1_clk.i);

But platforms that have the limitations you mention can do something like:

#include <clk-provider.h>
#include <clk-private.h>

struct clk __mmc1_clk;

struct clk_complex mmc1_clk {
	foo = 5,
	bar = 10,
	baz = 3,
	. i = {
		.name = mmc,
		.ops = &complex_ops,
		.clk = &__mmc1_clk;
	}
};

clk_init(NULL, &mmc1_clk.i);

I guess another way to look at this is: move all the inputs params in clk_register() to clk_hw and rename clk_hw to clk_initializer.

With the above solutions, those that don't have the OMAP-like limitations can have their code look much cleaner, and OMAP can still continue to work. After the initial conversion of all platform to the common clock framework, we don't allow anyone to ever include clk-private.h. We could even add it to check_patch.pl. Then once everyone fixes their static initialization limitations, we can delete the clk field from clk_initializer and move struct clk to clk.c.

Thoughts?

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


[Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [eCos]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [PDAs]     [Linux]     [Linux MIPS]     [Yosemite Campsites]     [Photos]

Add to Google Follow linuxarm on Twitter