sanitizing crazy clock data files

From: Jeremy Kerr <[email protected]>
+ * @get: Called by the core clock code when a device driver acquires a
+ * clock via clk_get(). Optional.
+ *
+ * @put: Called by the core clock code when a devices driver releases a
+ * clock via clk_put(). Optional.

Quoted 6 lines · Sascha Hauer

From: Jeremy Kerr <[email protected]>
+ * @get: Called by the core clock code when a device driver acquires a
+ * clock via clk_get(). Optional.
+ *
+ * @put: Called by the core clock code when a devices driver releases a
+ * clock via clk_put(). Optional.

These callbacks are completely pointless. There are only two non empty implementations in tree:

One does a try_module_get(clk->owner), which should be done in generic code. The other does special clock enabling magic which wants to go to clk->prepare().

Thanks,

tglx

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

Hi Thomas,

Quoted 11 lines · Thomas Gleixner

Quoted 6 lines · Sascha Hauer

From: Jeremy Kerr <[email protected]>
+ * @get: Called by the core clock code when a device driver acquires a
+ * clock via clk_get(). Optional.
+ *
+ * @put: Called by the core clock code when a devices driver releases a
+ * clock via clk_put(). Optional.

These callbacks are completely pointless. There are only two non empty implementations in tree:

Quoted 3 lines

One does a try_module_get(clk->owner), which should be done in generic code. The other does special clock enabling magic which wants to go to clk->prepare().

You forget mach-u300 __clk_get. It updates some registers.
The ops get/put let arch clock driver have a chance to do special things at the time when a user begins to use the clock.

Quoted 17 lines · Richard Zhao

Hi Thomas,

Quoted 14 lines · Thomas Gleixner

Quoted 6 lines · Sascha Hauer

From: Jeremy Kerr <[email protected]>
+ * @get: Called by the core clock code when a device driver acquires a
+ * clock via clk_get(). Optional.
+ *
+ * @put: Called by the core clock code when a devices driver releases a
+ * clock via clk_put(). Optional.

These callbacks are completely pointless. There are only two non empty implementations in tree:

One does a try_module_get(clk->owner), which should be done in generic code. The other does special clock enabling magic which wants to go to clk->prepare().

Quoted 3 lines

You forget mach-u300 __clk_get. It updates some registers.
The ops get/put let arch clock driver have a chance to do special things at the time when a user begins to use the clock.

No I did not forget. It's exactly the code which wants to go into clk->prepare. Which you have to call _before_ doing anything with the clock, so there is no need for an extra callback.

Thanks,

tglx

Quoted 22 lines · Thomas Gleixner

Quoted 20 lines · Richard Zhao

Hi Thomas,

Quoted 8 lines · Thomas Gleixner

Quoted 6 lines · Sascha Hauer

From: Jeremy Kerr <[email protected]>
+ * @get: Called by the core clock code when a device driver acquires a
+ * clock via clk_get(). Optional.
+ *
+ * @put: Called by the core clock code when a devices driver releases a
+ * clock via clk_put(). Optional.

Quoted 6 lines

These callbacks are completely pointless. There are only two non empty implementations in tree:

One does a try_module_get(clk->owner), which should be done in generic code. The other does special clock enabling magic which wants to go to clk->prepare().

You forget mach-u300 __clk_get. It updates some registers.
The ops get/put let arch clock driver have a chance to do special things at the time when a user begins to use the clock.

Quoted 3 lines

No I did not forget. It's exactly the code which wants to go into clk->prepare. Which you have to call _before_ doing anything with the clock, so there is no need for an extra callback.

No. clk_prepare only have to be called before clk_enable, but not have to for other operations, set_rate/set_parent etc.

Quoted 26 lines · Richard Zhao

Quoted 25 lines · Thomas Gleixner

Quoted 14 lines · Richard Zhao

Hi Thomas,

Quoted 11 lines · Thomas Gleixner

Quoted 6 lines · Sascha Hauer

From: Jeremy Kerr <[email protected]>
+ * @get: Called by the core clock code when a device driver acquires a
+ * clock via clk_get(). Optional.
+ *
+ * @put: Called by the core clock code when a devices driver releases a
+ * clock via clk_put(). Optional.

These callbacks are completely pointless. There are only two non empty implementations in tree:

Quoted 6 lines

Quoted 3 lines

One does a try_module_get(clk->owner), which should be done in generic code. The other does special clock enabling magic which wants to go to clk->prepare().

You forget mach-u300 __clk_get. It updates some registers.
The ops get/put let arch clock driver have a chance to do special things at the time when a user begins to use the clock.

No I did not forget. It's exactly the code which wants to go into clk->prepare. Which you have to call _before_ doing anything with the clock, so there is no need for an extra callback.

Quoted 2 lines

No. clk_prepare only have to be called before clk_enable, but not have to for other operations, set_rate/set_parent etc.

And why do you need those registers set before you actually prepare/enable the clock? Just to set the rate register? You can store that information and set it on prepare. Before prepare/enable the value of the rate register is totally irrelevant.

The point of anything whats named get and put in the kernel is to obtain or drop a reference to an object.

Putting arbitrary initialization into such functions is a really bad hack.

And looking at that u300 code, it's borken:

    user1: clk_get("MCLK");
    	   clk_set_rate(.....);

    user2: clk_get("MCLK");

The second clk_get() call will overwrite what ?

The fact that you decided to put that into the absolutely wrong function is not an argument for keeping those callbacks.

The only real usecase for __clk_get() is the try_module_get()/module_put() code which will become part of the generic code. It does exaclty what get and put are meant for: dealing with refcounts.

Thanks,

tglx

Quoted 10 lines · Thomas Gleixner

Quoted 9 lines · Richard Zhao

Quoted 7 lines

Quoted 4 lines

One does a try_module_get(clk->owner), which should be done in generic code. The other does special clock enabling magic which wants to go to clk->prepare().

You forget mach-u300 __clk_get. It updates some registers.
The ops get/put let arch clock driver have a chance to do special things at the time when a user begins to use the clock.

Quoted 6 lines

Quoted 3 lines

No I did not forget. It's exactly the code which wants to go into clk->prepare. Which you have to call _before_ doing anything with the clock, so there is no need for an extra callback.

No. clk_prepare only have to be called before clk_enable, but not have to for other operations, set_rate/set_parent etc.

And why do you need those registers set before you actually prepare/enable the clock? Just to set the rate register? You can store that information and set it on prepare. Before prepare/enable the value of the rate register is totally irrelevant.

The point of anything whats named get and put in the kernel is to obtain or drop a reference to an object.

Putting arbitrary initialization into such functions is a really bad hack.

And looking at that u300 code, it's borken:

? ?user1: clk_get("MCLK");
? ? ? ? ? clk_set_rate(.....);

? ?user2: clk_get("MCLK");

The second clk_get() call will overwrite what ?

2011/4/23 Thomas Gleixner <[email protected]>:

Quoted 29 lines · Richard Zhao

Quoted 10 lines · Thomas Gleixner

Quoted 9 lines · Richard Zhao

Quoted 7 lines

Quoted 4 lines

One does a try_module_get(clk->owner), which should be done in generic code. The other does special clock enabling magic which wants to go to clk->prepare().

You forget mach-u300 __clk_get. It updates some registers.
The ops get/put let arch clock driver have a chance to do special things at the time when a user begins to use the clock.

Quoted 6 lines

Quoted 3 lines

No I did not forget. It's exactly the code which wants to go into clk->prepare. Which you have to call _before_ doing anything with the clock, so there is no need for an extra callback.

No. clk_prepare only have to be called before clk_enable, but not have to for other operations, set_rate/set_parent etc.

And why do you need those registers set before you actually prepare/enable the clock? Just to set the rate register? You can store that information and set it on prepare. Before prepare/enable the value of the rate register is totally irrelevant.

The point of anything whats named get and put in the kernel is to obtain or drop a reference to an object.

Putting arbitrary initialization into such functions is a really bad hack.

Quoted 8 lines

And looking at that u300 code, it's borken:

? ?user1: clk_get("MCLK");
? ? ? ? ? clk_set_rate(.....);

? ?user2: clk_get("MCLK");

The second clk_get() call will overwrite what ?

Heh yeah that is the case, and this code can just as well live in the ->prepare() function.

The clock that goes to "MCLK" and "MSPRO" is utilizing a common clock divider, which sounds backwards, but in practice this is because it's and MMC or Memory Stick Pro block clock, and you cannot pinmux in both in the ASIC, they are connected to the same pins and either function need its own support electronics. So naturally only one of them is ever active at the same time.

This should be *properly* reflected by creating the pin/padmux framework and have the clock prepare() call hook back into that, so that when you try to activate say "mclk" you need to mux in these pins and if the mspro is muxed in at that time you return -ENODEV and the clock code bails out.

We should just move it to prepare() and I'll fix it up if need be.

Linus Walleij

Quoted 6 lines · Sascha Hauer

From: Jeremy Kerr<[email protected]>
+ * @get: Called by the core clock code when a device driver acquires a
+ * clock via clk_get(). Optional.
+ *
+ * @put: Called by the core clock code when a devices driver releases a
+ * clock via clk_put(). Optional.

These callbacks are completely pointless. There are only two non empty implementations in tree:

One does a try_module_get(clk->owner), which should be done in generic code. The other does special clock enabling magic which wants to go to clk->prepare().

Quoted 11 lines · Thomas Gleixner

Quoted 6 lines · Sascha Hauer

From: Jeremy Kerr<[email protected]>
+ * @get: Called by the core clock code when a device driver acquires a
+ * clock via clk_get(). Optional.
+ *
+ * @put: Called by the core clock code when a devices driver releases a
+ * clock via clk_put(). Optional.

These callbacks are completely pointless. There are only two non empty implementations in tree:

Quoted 3 lines

One does a try_module_get(clk->owner), which should be done in generic code. The other does special clock enabling magic which wants to go to clk->prepare().

Aren't you kinda contradicting yourself? In the other thread you say that when we implement a generic framework, you will have to implement more than what a specific arch would need. This seems to be exactly one of those cases. What's wrong with it?

Quoted 6 lines · Sascha Hauer

From: Jeremy Kerr <[email protected]>
+ * @get: Called by the core clock code when a device driver acquires a
+ * clock via clk_get(). Optional.
+ *
+ * @put: Called by the core clock code when a devices driver releases a
+ * clock via clk_put(). Optional.

These callbacks are completely pointless. There are only two non empty implementations in tree:

One does a try_module_get(clk->owner), which should be done in generic code. The other does special clock enabling magic which wants to go to clk->prepare().

Quoted 11 lines · Thomas Gleixner

Quoted 6 lines · Sascha Hauer

From: Jeremy Kerr <[email protected]>
+ * @get: Called by the core clock code when a device driver acquires a
+ * clock via clk_get(). Optional.
+ *
+ * @put: Called by the core clock code when a devices driver releases a
+ * clock via clk_put(). Optional.

These callbacks are completely pointless. There are only two non empty implementations in tree:

Quoted 3 lines

One does a try_module_get(clk->owner), which should be done in generic code. The other does special clock enabling magic which wants to go to clk->prepare().

I disagree. Most clocks don't live in a module - there's only one platform which does at present. To force every clock to have an owner field is rediculous. We already know that the OMAP tree represents a

significant amount of data, and for every additional unnecessary word, it gobbles up another 4K of data space.

If we go down the route of separating clock stuff into bite-size pieces, OMAP is going to end up with yet more struct clks, which is going to add to the bloat even more.

Quoted 12 lines · Russell King - ARM Linux

Quoted 11 lines · Thomas Gleixner

Quoted 6 lines · Sascha Hauer

From: Jeremy Kerr <[email protected]>
+ * @get: Called by the core clock code when a device driver acquires a
+ * clock via clk_get(). Optional.
+ *
+ * @put: Called by the core clock code when a devices driver releases a
+ * clock via clk_put(). Optional.

These callbacks are completely pointless. There are only two non empty implementations in tree:

Quoted 7 lines

Quoted 3 lines

One does a try_module_get(clk->owner), which should be done in generic code. The other does special clock enabling magic which wants to go to clk->prepare().

I disagree. Most clocks don't live in a module - there's only one platform which does at present. To force every clock to have an owner field is rediculous. We already know that the OMAP tree represents a

So we trade an owner field (which can be NULL) versus two function pointers in the clk_ops struct, which are of course subject to be abused for all kind of crap which does not belong there at all.

Quoted 2 lines

significant amount of data, and for every additional unnecessary word, it gobbles up another 4K of data space.

What are we talking about?

arch/arm/mach-omap2/clock2420_data.c:117
arch/arm/mach-omap2/clock2430_data.c:126
arch/arm/mach-omap2/clock3xxx_data.c:235
arch/arm/mach-omap2/clock44xx_data.c:228

So even if we could build a common kernel for all these omaps, then adding owner to struct clk will consume ~2800 bytes.

Quoted 16 lines · Russell King - ARM Linux

Quoted 14 lines · Thomas Gleixner

Quoted 6 lines · Sascha Hauer

From: Jeremy Kerr <[email protected]>
+ * @get: Called by the core clock code when a device driver acquires a
+ * clock via clk_get(). Optional.
+ *
+ * @put: Called by the core clock code when a devices driver releases a
+ * clock via clk_put(). Optional.

These callbacks are completely pointless. There are only two non empty implementations in tree:

One does a try_module_get(clk->owner), which should be done in generic code. The other does special clock enabling magic which wants to go to clk->prepare().

Quoted 3 lines

I disagree. Most clocks don't live in a module - there's only one platform which does at present. To force every clock to have an owner field is rediculous. We already know that the OMAP tree represents a

So we trade an owner field (which can be NULL) versus two function pointers in the clk_ops struct, which are of course subject to be abused for all kind of crap which does not belong there at all.

Quoted 18 lines · Thomas Gleixner

Quoted 16 lines · Russell King - ARM Linux

Quoted 14 lines · Thomas Gleixner

Quoted 6 lines · Sascha Hauer

From: Jeremy Kerr <[email protected]>
+ * @get: Called by the core clock code when a device driver acquires a
+ * clock via clk_get(). Optional.
+ *
+ * @put: Called by the core clock code when a devices driver releases a
+ * clock via clk_put(). Optional.

These callbacks are completely pointless. There are only two non empty implementations in tree:

One does a try_module_get(clk->owner), which should be done in generic code. The other does special clock enabling magic which wants to go to clk->prepare().

Quoted 7 lines

Quoted 3 lines

I disagree. Most clocks don't live in a module - there's only one platform which does at present. To force every clock to have an owner field is rediculous. We already know that the OMAP tree represents a

So we trade an owner field (which can be NULL) versus two function pointers in the clk_ops struct, which are of course subject to be abused for all kind of crap which does not belong there at all.

And the current __clk_get/__clk_put stuff has been abused to hell and back hasn't it with just three implementations. While I agree with your sentiment, lets agree to address that _if_ it becomes a problem. At the moment, we have _enough_ problems to deal with, we don't need to invent new ones which really don't matter at this stage.

From: Jeremy Kerr <[email protected]>

We currently have ~21 definitions of struct clk in the ARM architecture, each defined on a per-platform basis. This makes it difficult to define platform- (or architecture-) independent clock sources without making assumptions about struct clk, and impossible to compile two platforms with different struct clks into a single image.

drivers/clk/clk.c new file +9 -0

+
+ ? ? ? if (!ret)
+ ? ? ? ? ? ? ? clk->enable_count++;
+ ? ? ? spin_unlock_irqrestore(&clk->enable_lock, flags);
+
+ ? ? ? return ret;
+}
+EXPORT_SYMBOL_GPL(clk_enable);
+

2011/4/15 Sascha Hauer <[email protected]>:

Quoted 7 lines

From: Jeremy Kerr <[email protected]>

We currently have ~21 definitions of struct clk in the ARM architecture, each defined on a per-platform basis. This makes it difficult to define platform- (or architecture-) independent clock sources without making assumptions about struct clk, and impossible to compile two platforms with different struct clks into a single image.

Sorry for this late comment, maybe already adressed:

Review

Quoted 10 lines

(quoted diff) +10 -0

+int clk_enable(struct clk *clk)
+{
+ ? ? ? unsigned long flags;
+ ? ? ? int ret = 0;
+
+ ? ? ? WARN_ON(clk->prepare_count == 0);
+
+ ? ? ? spin_lock_irqsave(&clk->enable_lock, flags);
+ ? ? ? if (clk->enable_count == 0 && clk->ops->enable)
+ ? ? ? ? ? ? ? ret = clk->ops->enable(clk);

Here the spinlock is held requiring the clock implementation to be non-sleeping, something that has been discussed at no end, without addressing that eternal debate, please just drop the spinlock during the ->enable() call, what is it protecting during that time anyway...

we decided that the enable call shall be
non-sleeping for everything. ?That's why we introduced the prepare call for the sleeping part of the problem.

From: Jeremy Kerr<[email protected]>

We currently have ~21 definitions of struct clk in the ARM architecture, each defined on a per-platform basis. This makes it difficult to define platform- (or architecture-) independent clock sources without making assumptions about struct clk, and impossible to compile two platforms with different struct clks into a single image.

This change is an effort to unify struct clk where possible, by defining a common struct clk, containing a set of clock operations. Different clock implementations can set their own operations, and have a standard interface for generic code. The callback interface is exposed to the kernel proper, while the clock implementations only need to be seen by the platform internals.

This allows us to share clock code among platforms, and makes it possible to dynamically create clock devices in platform-independent code.

Platforms can enable the generic struct clock through CONFIG_USE_COMMON_STRUCT_CLK. In this case, the clock infrastructure consists of a common struct clk:

struct clk {
	const struct clk_ops	*ops;
	unsigned int		enable_count;
	unsigned int		prepare_count;
	spinlock_t		enable_lock;
	struct mutex		prepare_lock;
};

And a set of clock operations (defined per type of clock):

struct clk_ops {
	int             (*enable)(struct clk *);
	void            (*disable)(struct clk *);
	unsigned long   (*get_rate)(struct clk *);
	[...]
};

To define a hardware-specific clock, machine code can "subclass" the struct clock into a new struct (adding any device-specific data), and provide a set of operations:

struct clk_foo {
	struct clk	clk;
	void __iomem	*some_register;
};

struct clk_ops clk_foo_ops = {
.get_rate = clk_foo_get_rate,
};

The common clock definitions are based on a development patch from Ben Herrenschmidt<[email protected]>.

Signed-off-by Jeremy Kerr<[email protected]>

Signed-off-by Uwe Kleine-K?nig<[email protected]>

---
  drivers/clk/Kconfig  |    3 +
  drivers/clk/Makefile |    1 +
  drivers/clk/clk.c    |  132 ++++++++++++++++++++++++++++++++++++++++
  drivers/clk/clkdev.c |    7 ++
  include/linux/clk.h  |  164 +++++++++++++++++++++++++++++++++++++++++++++++---
  5 files changed, 298 insertions(+), 9 deletions(-)

create mode 100644 drivers/clk/clk.c

drivers/clk/Kconfig +3 -0

index 4168c88..6e3ae54 100644
@@ -2,3 +2,6 @@
  config CLKDEV_LOOKUP
  	bool
  	select HAVE_CLK
+
+config USE_COMMON_STRUCT_CLK
+	bool

drivers/clk/Makefile +1 -0

index 07613fa..a1a06d3 100644
@@ -1,2 +1,3 @@
 
  obj-$(CONFIG_CLKDEV_LOOKUP)	+= clkdev.o
+obj-$(CONFIG_USE_COMMON_STRUCT_CLK) += clk.o

drivers/clk/clk.c new file +7 -0

new file mode 100644
index 0000000..0bc9c6f
@@ -0,0 +1,132 @@
+/*
+ * Copyright (C) 2010-2011 Canonical Ltd<[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *

drivers/clk/clk.c line 17

Quoted 4 lines

drivers/clk/clk.c new file +4 -0

+int clk_prepare(struct clk *clk)
+{
+	int ret = 0;
+

Shouldn't all these functions have some clk pointer checks:

	if (IS_ERR_OR_NULL(clk))
		return -EINVAL;

Hi Rob,

Quoted 7 lines

drivers/clk/clk.c line 17

Quoted 4 lines

drivers/clk/clk.c new file +4 -0

+int clk_prepare(struct clk *clk)
+{
+	int ret = 0;
+

Shouldn't all these functions have some clk pointer checks:

Quoted 3 lines

	if (IS_ERR_OR_NULL(clk))
		return -EINVAL;

No, I'd prefer not to. The driver code should be responsible for passing a valid pointer here, by doing this check once (where the clock is initially acquired through clk_get), rather than the core code doing it on each function call.

I think you will find many examples in the kernel where that is not done by drivers.

Hi Rob,

Quoted 2 lines

I think you will find many examples in the kernel where that is not done by drivers.

Drivers should be checking the return value of clk_get - if they don't, it's a bug. This is the logical place to check, rather than before all clock API calls.

For cases where there is no clock provided for the device (but is a valid clock on some machines), the platform code should return a no-op clock from the clk_get call. This 'noop clock' would be a good contender for inclusion into the kernel-wide infrastructure, like clk_fixed.

Quoted 5 lines · Jeremy Kerr

Hi Rob,

Quoted 2 lines

I think you will find many examples in the kernel where that is not done by drivers.

Quoted 3 lines

Drivers should be checking the return value of clk_get - if they don't, it's a bug. This is the logical place to check, rather than before all clock API calls.

Maybe so, but it's common practice. Why not allow it, but add a warning? Or allow NULL, but not an error value.

Quoted 4 lines

For cases where there is no clock provided for the device (but is a valid clock on some machines), the platform code should return a no-op clock from the clk_get call. This 'noop clock' would be a good contender for inclusion into the kernel-wide infrastructure, like clk_fixed.

There could be cases where the driver wants to know if there is no clock.

Quoted 6 lines · Rob Herring

Quoted 5 lines · Jeremy Kerr

Hi Rob,

Quoted 2 lines

I think you will find many examples in the kernel where that is not done by drivers.

Quoted 6 lines

Quoted 3 lines

Drivers should be checking the return value of clk_get - if they don't, it's a bug. This is the logical place to check, rather than before all clock API calls.

Maybe so, but it's common practice. Why not allow it, but add a warning? Or allow NULL, but not an error value.

If drivers don't check the return value of clk_get() then they'll suffer kernel oopses. What's to say that if the driver doesn't check the return value from clk_get() that they'll bother checking the return value from any of the other clk_* API functions. Nothing.

So, checking for ERR values inside the clk_* functions is silly, and just means that buggy drivers continue with undiscovered bugs because they happen not to fail.

Quoted 7 lines

Quoted 4 lines

For cases where there is no clock provided for the device (but is a valid clock on some machines), the platform code should return a no-op clock from the clk_get call. This 'noop clock' would be a good contender for inclusion into the kernel-wide infrastructure, like clk_fixed.

There could be cases where the driver wants to know if there is no clock.

How exactly would a driver know that the clock it has is a no-op clock with it being a pointer to some random data structure? A better question would be in the case of a cpufreq driver, how would such a driver know that it's pointless trying to set the rate on a fixed rate clock?

There's soo many combinations here where you can say "if a clock has property X, then maybe the driver doesn't want to do Y". So, instead of trying to cover all the possibilities, take a step back and look at the bigger picture.

1. in a cpufreq driver, if it gets a clock, one would hope that the clock it has been provided with is adjustable - in other words, clk_set_rate() stands a chance of succeeding. If it fails, then it has the wrong clock.

2. if the clock it has does not support adjustments in this way, then there is a bug in the data involved in selecting that clock, and that's what's at fault. You find this out when you try to change its rate via clk_set_rate() which should fail.

3. (to head off something that I can see being proposed) no, I don't think we need a system of properties.

A cpufreq driver which obtains clks should be no different from any other driver. It should get a clk via clk_get(), and report errors if that fails. If clk_set_rate() fails, then it should report that error too. There's really nothing "special" here.

This does bring us to an interesting question though: should clk_set_rate() succeed or fail with a NULL clk? There is no clock to control, so my feeling is that it should fail, just like clk_get_rate() should return zero because the rate is meaningless. There is no rate to get and no rate to set.

Quoted 5 lines

This does bring us to an interesting question though: should clk_set_rate() succeed or fail with a NULL clk? There is no clock to control, so my feeling is that it should fail, just like clk_get_rate() should return zero because the rate is meaningless. There is no rate to get and no rate to set.

I think having NULL clocks return success on set_rate() would be more useful. The most common use case for NULL clocks is when a clocks is present in some soc/arch/mach but is not present in another. Instead of having the consumers having an "if" around every clk_* calls on that clk, they would get a NULL clk. If NULL clk starts failing for clk_set_rate(), I think the point of a NULL clk will get defeated.

Quoted 6 lines · Saravana Kannan

Quoted 5 lines

This does bring us to an interesting question though: should clk_set_rate() succeed or fail with a NULL clk? There is no clock to control, so my feeling is that it should fail, just like clk_get_rate() should return zero because the rate is meaningless. There is no rate to get and no rate to set.

Quoted 7 lines

I think having NULL clocks return success on set_rate() would be more useful. The most common use case for NULL clocks is when a clocks is present in some soc/arch/mach but is not present in another. Instead of having the consumers having an "if" around every clk_* calls on that clk, they would get a NULL clk. If NULL clk starts failing for clk_set_rate(), I think the point of a NULL clk will get defeated.

From my experience when a clock is present on one SoC but not on another
the actual rate does not matter, only enabling/disabling the clock for register accesses is important. When a driver sets a rate on a clock it does it on purpose and if this clock is not even present on other SoCs it looks like a bug or at least a very special case for me. So I think clk_set_rate for a NULL clock should fail.

Other than that it's not a good behaviour when clk_get_rate() returns 0 for clock which rate has been successfully set to another rate using clk_set_rate().

This does bring us to an interesting question though: should clk_set_rate() succeed or fail with a NULL clk? There is no clock to control, so my feeling is that it should fail, just like clk_get_rate() should return zero because the rate is meaningless. There is no rate to get and no rate to set.

Hi

Quoted 5 lines · Russell King - ARM Linux

This does bring us to an interesting question though: should clk_set_rate() succeed or fail with a NULL clk? There is no clock to control, so my feeling is that it should fail, just like clk_get_rate() should return zero because the rate is meaningless. There is no rate to get and no rate to set.

Returning an error would be my preference when a NULL or bogus clock pointer is passed to any clk_* operation, especially since those operations will need to dereference the clock pointer.

Hi Rob,

Quoted 2 lines

I think you will find many examples in the kernel where that is not done by drivers.

Drivers should be checking the return value of clk_get - if they don't, it's a bug. This is the logical place to check, rather than before all clock API calls.

For cases where there is no clock provided for the device (but is a valid clock on some machines), the platform code should return a no-op clock from the clk_get call. This 'noop clock' would be a good contender for inclusion into the kernel-wide infrastructure, like clk_fixed.

Quoted 4 lines · Jeremy Kerr

Hi Rob,

Quoted 2 lines

I think you will find many examples in the kernel where that is not done by drivers.

Quoted 8 lines

Drivers should be checking the return value of clk_get - if they don't, it's a bug. This is the logical place to check, rather than before all clock API calls.

For cases where there is no clock provided for the device (but is a valid clock on some machines), the platform code should return a no-op clock from the clk_get call. This 'noop clock' would be a good contender for inclusion into the kernel-wide infrastructure, like clk_fixed.

Having a general fixed clock is a good idea. There seem to be several 'dummy' clocks under various subarchs, and Stephen Boyd posted one for MSM on March 24.

Most of the MSM drivers check the clk_get and fail to probe the driver if the clock is unavailable.

NULL used to be a valid clock. This patch makes the common struct clk stuff work with this. Instead of crashing we return sane default values for this dummy clock, that is: can always be prepared, enabled, rate unkown and no set rate.

Hi Sascha,

Quoted 4 lines

NULL used to be a valid clock. This patch makes the common struct clk stuff work with this. Instead of crashing we return sane default values for this dummy clock, that is: can always be prepared, enabled, rate unkown and no set rate.

Although I'm not totally averse to this change, I've been trying to avoid allowing NULL clocks - it seems to be a little 'messy' to allows clocks that don't have any possible operations on them, and could hide error cases where clock pointers have not been correctly initialised.

However, I understand that this may be required for flexibility between machines, and in that case would make sense. Is that what you're intending this for?

NULL used to be a valid clock. This patch makes the common struct clk stuff work with this. Instead of crashing we return sane default values for this dummy clock, that is: can always be prepared, enabled, rate unkown and no set rate.

Signed-off-by Sascha Hauer <[email protected]>

Cc Jeremy Kerr <[email protected]>

---
 drivers/clk/clk.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

drivers/clk/clk.c +12 -0

index 0da0bb9..264c809 100644
@@ -15,6 +15,9 @@ int clk_prepare(struct clk *clk)
 {
 	int ret = 0;
 
+	if (!clk)
+		return 0;
+
 	mutex_lock(&clk->prepare_lock);
 	if (clk->prepare_count == 0 && clk->ops->prepare)
 		ret = clk->ops->prepare(clk);

@@ -29,6 +32,9 @@ EXPORT_SYMBOL_GPL(clk_prepare);
 
 void clk_unprepare(struct clk *clk)
 {
+	if (!clk)
+		return;
+
 	mutex_lock(&clk->prepare_lock);
 
 	WARN_ON(clk->prepare_count == 0);

@@ -47,6 +53,9 @@ int clk_enable(struct clk *clk)
 	unsigned long flags;
 	int ret = 0;
 
+	if (!clk)
+		return 0;
+
 	WARN_ON(clk->prepare_count == 0);
 
 	spin_lock_irqsave(&clk->enable_lock, flags);

@@ -65,6 +74,9 @@ void clk_disable(struct clk *clk)
 {
 	unsigned long flags;
 
+	if (!clk)
+		return;
+
 	spin_lock_irqsave(&clk->enable_lock, flags);
 
 	WARN_ON(clk->enable_count == 0);

A common case for clocks is that certain operations are not implemented and shall be passed through to the parent. Add convenience functions for this purpose

Signed-off-by Sascha Hauer <[email protected]>

Cc Jeremy Kerr <[email protected]>

---
 drivers/clk/clk.c   |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk.h |    9 ++++++
 2 files changed, 86 insertions(+), 0 deletions(-)

drivers/clk/clk.c +19 -0

index 264c809..7e2c182 100644
@@ -171,3 +171,80 @@ struct clk_ops clk_fixed_ops = {
 	.get_rate = clk_fixed_get_rate,
 };
 EXPORT_SYMBOL_GPL(clk_fixed_ops);
+
+int clk_parent_prepare(struct clk *clk)
+{
+	struct clk *parent = clk_get_parent(clk);
+
+	if (IS_ERR(parent))
+		return -ENOSYS;
+
+	return clk_prepare(parent);
+}
+EXPORT_SYMBOL_GPL(clk_parent_prepare);
+
+void clk_parent_unprepare(struct clk *clk)
+{
+	struct clk *parent = clk_get_parent(clk);
+
+	if (IS_ERR(parent))
+		return;
+

drivers/clk/clk.c +48 -0

+
+	return clk_enable(parent);
+}
+EXPORT_SYMBOL_GPL(clk_parent_enable);
+
+void clk_parent_disable(struct clk *clk)
+{
+	struct clk *parent = clk_get_parent(clk);
+
+	if (IS_ERR(parent))
+		return;
+
+	clk_disable(parent);
+}
+EXPORT_SYMBOL_GPL(clk_parent_disable);
+
+unsigned long clk_parent_get_rate(struct clk *clk)
+{
+	struct clk *parent = clk_get_parent(clk);
+
+	if (IS_ERR(parent))
+		return 0;
+
+	return clk_get_rate(parent);
+}
+EXPORT_SYMBOL_GPL(clk_parent_get_rate);
+
+long clk_parent_round_rate(struct clk *clk, unsigned long rate)
+{
+	struct clk *parent = clk_get_parent(clk);
+
+	if (IS_ERR(parent))
+		return -ENOSYS;
+
+	return clk_round_rate(parent, rate);
+}
+EXPORT_SYMBOL_GPL(clk_parent_round_rate);
+
+int clk_parent_set_rate(struct clk *clk, unsigned long rate)
+{
+	struct clk *parent = clk_get_parent(clk);
+
+	if (IS_ERR(parent))
+		return -ENOSYS;
+
+	return clk_set_rate(parent, rate);
+}
+EXPORT_SYMBOL_GPL(clk_parent_set_rate);

include/linux/clk.h +0 -0

index d2f0db0..d014341 100644
@@ -171,6 +171,15 @@ extern struct clk_ops clk_fixed_ops;
 #define DEFINE_CLK_FIXED(name, r) \
 	struct clk_fixed name = INIT_CLK_FIXED(name, r)

include/linux/clk.h +0 -0

 #else /* !CONFIG_USE_COMMON_STRUCT_CLK */
 
 /*

Signature · 7 lines

1.7.4.1


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

Quoted 42 lines · Sascha Hauer

A common case for clocks is that certain operations are not implemented and shall be passed through to the parent. Add convenience functions for this purpose

Signed-off-by Sascha Hauer <[email protected]>

Cc Jeremy Kerr <[email protected]>

---
 drivers/clk/clk.c   |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk.h |    9 ++++++
 2 files changed, 86 insertions(+), 0 deletions(-)

drivers/clk/clk.c +23 -0

index 264c809..7e2c182 100644
@@ -171,3 +171,80 @@ struct clk_ops clk_fixed_ops = {
 	.get_rate = clk_fixed_get_rate,
 };
 EXPORT_SYMBOL_GPL(clk_fixed_ops);
+
+int clk_parent_prepare(struct clk *clk)
+{
+	struct clk *parent = clk_get_parent(clk);
+
+	if (IS_ERR(parent))
+		return -ENOSYS;
+
+	return clk_prepare(parent);
+}
+EXPORT_SYMBOL_GPL(clk_parent_prepare);
+
+void clk_parent_unprepare(struct clk *clk)
+{
+	struct clk *parent = clk_get_parent(clk);
+
+	if (IS_ERR(parent))
+		return;
+
+	clk_unprepare(parent);
+}
+EXPORT_SYMBOL_GPL(clk_parent_unprepare);
+

drivers/clk/clk.c struct clk_ops clk_fixed_ops = { · line 202

Quoted 6 lines

drivers/clk/clk.c +6 -0

+int clk_parent_enable(struct clk *clk)
+{
+	struct clk *parent = clk_get_parent(clk);
+
+	if (IS_ERR(parent))
+		return 0;

Do you really want to return 0 here? IMHO something is fishy if a clock has .enable = clk_parent_enable but has no parent.

drivers/clk/clk.c struct clk_ops clk_fixed_ops = { · line 211

Quoted 10 lines

drivers/clk/clk.c +9 -0

+/* generic pass-through-to-parent functions */
+int clk_parent_prepare(struct clk *clk);
+void clk_parent_unprepare(struct clk *clk);
+int clk_parent_enable(struct clk *clk);
+void clk_parent_disable(struct clk *clk);
+unsigned long clk_parent_get_rate(struct clk *clk);
+long clk_parent_round_rate(struct clk *clk, unsigned long rate);
+int clk_parent_set_rate(struct clk *clk, unsigned long rate);
+

Technically they don't need to depend on USE_COMMON_STRUCT_CLK.

A common case for clocks is that certain operations are not implemented and shall be passed through to the parent. Add convenience functions for this purpose

Signed-off-by Sascha Hauer <[email protected]>

Cc Jeremy Kerr <[email protected]>

Quoted 7 lines · Sascha Hauer

A common case for clocks is that certain operations are not implemented and shall be passed through to the parent. Add convenience functions for this purpose

Signed-off-by Sascha Hauer <[email protected]>

Cc Jeremy Kerr <[email protected]>

I was hoping we could just build the parent handling into the core instead of having every clk_ops->enable call the parent ops.

Something like: if the clk_ops doesn't have a get_parent, clk_get_parent() would return NULL, and if the clk_ops doesn't have a set_parent, clk_set_parent() would do nothing.

Then clk_enable()/clk_disable() would call clk_get_parent() and then clk_enable() on that (whatever it is) before calling down into the ops->enable() function. Of course this means NULL is now the "no parent" value as opposed to ERR_PTR(-ENOSYS) but I'm willing to live with that since it makes things a bit simpler when you only have to check for NULL in the clk_enable()/clk_disable() path as opposed to NULL and IS_ERR.

I'm afraid this patch means every time the clk_ops wants to enable its parent (which is almost always the case), it has to do so explicitly when we really just want to concentrate on what enabling means for that clock (namely setting some bits in some registers). Thoughts?

Quoted 9 lines · Stephen Boyd

Quoted 7 lines · Sascha Hauer

A common case for clocks is that certain operations are not implemented and shall be passed through to the parent. Add convenience functions for this purpose

Signed-off-by Sascha Hauer <[email protected]>

Cc Jeremy Kerr <[email protected]>

Quoted 6 lines

I was hoping we could just build the parent handling into the core instead of having every clk_ops->enable call the parent ops.

Something like: if the clk_ops doesn't have a get_parent, clk_get_parent() would return NULL, and if the clk_ops doesn't have a set_parent, clk_set_parent() would do nothing.

That's the current behaviour (except that ERR_PTR(-ENOSYS) is returned instead of NULL).

Quoted 8 lines

Then clk_enable()/clk_disable() would call clk_get_parent() and then clk_enable() on that (whatever it is) before calling down into the ops->enable() function. Of course this means NULL is now the "no parent" value as opposed to ERR_PTR(-ENOSYS) but I'm willing to live with that since it makes things a bit simpler when you only have to check for NULL in the clk_enable()/clk_disable() path as opposed to NULL and IS_ERR.

Quoted 4 lines

I'm afraid this patch means every time the clk_ops wants to enable its parent (which is almost always the case), it has to do so explicitly when we really just want to concentrate on what enabling means for that clock (namely setting some bits in some registers). Thoughts?

+1 for letting the core enable/disable and prepare/unprepare the parent clocks. I scanned the different arm clock implementations and they all do it, except the ones which do not implement parents at all.

Hello,

Quoted 4 lines

+1 for letting the core enable/disable and prepare/unprepare the parent clocks. I scanned the different arm clock implementations and they all do it, except the ones which do not implement parents at all.

Then the question is if all do handle parents in the same way. (i.e. in enable do parent first, in disable do child first?)

If one layer of indirection is acceptable this can easily be accomplished (unless I oversee something):

	struct clk_with_parent {
		struct clk clk;
		const struct clk_ops *ops;
		struct clk *parent;
	}

Then the callbacks in clk_with_parent.clk would handle the parent and then call the respective callbacks in clk_with_parent.ops.

Quoted 3 lines

+1 for letting the core enable/disable and prepare/unprepare the parent clocks. I scanned the different arm clock implementations and they all do it, except the ones which do not implement parents at all.

Then the question is if all do handle parents in the same way. (i.e. in enable do parent first, in disable do child first?)

If one layer of indirection is acceptable this can easily be accomplished (unless I oversee something):

	struct clk_with_parent {
		struct clk clk;
		const struct clk_ops *ops;
		struct clk *parent;
	}

Then the callbacks in clk_with_parent.clk would handle the parent and then call the respective callbacks in clk_with_parent.ops.

Quoted 5 lines · Uwe Kleine-K?nig

Quoted 3 lines

+1 for letting the core enable/disable and prepare/unprepare the parent clocks. I scanned the different arm clock implementations and they all do it, except the ones which do not implement parents at all.

Then the question is if all do handle parents in the same way. (i.e. in enable do parent first, in disable do child first?)

That's not a question at all. Anything which does it the other way round is broken. Period.

Quoted 4 lines · Thomas Gleixner

Quoted 3 lines · Uwe Kleine-K?nig

Quoted 3 lines

+1 for letting the core enable/disable and prepare/unprepare the parent clocks. I scanned the different arm clock implementations and they all do it, except the ones which do not implement parents at all.

Quoted 5 lines

Quoted 2 lines

Then the question is if all do handle parents in the same way. (i.e. in enable do parent first, in disable do child first?)

That's not a question at all. Anything which does it the other way round is broken. Period.

I havn't checked, but I remember Jeremy not wanting to handle parents in the generic part because he saw some differences.

Signed-off-by Uwe Kleine-K?nig <[email protected]>

---
Hello,

only compile tested so far.

drivers/clk/clk.c +0 -0

index 264c809..7627815 100644

On Wed, 20 Apr 2011, Uwe Kleine-K?nig wrote:

Very useful changelog.

Quoted 5 lines

Signed-off-by Uwe Kleine-K?nig <[email protected]>

---
Hello,

only compile tested so far.

You are not listening at all, right?

Quoted 10 lines

drivers/clk/clk.c +1 -0

index 264c809..7627815 100644
@@ -14,10 +14,23 @@
 int clk_prepare(struct clk *clk)
 {
 	int ret = 0;
+	struct clk *parent = ERR_PTR(-ENOSYS);
 

drivers/clk/clk.c line 22

Quoted 4 lines

drivers/clk/clk.c +1 -0

 	if (!clk)
 		return 0;
 
+	if (clk->ops->flags & CLK_OPS_GENERIC_PARENT) {

Why the hell is this conditional? I told you that a proper abstraction

is for the sane cases which are the majority of the hardware and we don't code for the buggy HW case unless we have evidence that it exists. Once we have that evidence we can decide what to do - add a flag or a separate function.

drivers/clk/clk.c line 32

Quoted 10 lines

drivers/clk/clk.c +10 -0

+		parent = clk_get_parent(clk);
+
+		if (!IS_ERR(parent)) {
+			ret = clk_prepare(parent);
+			if (ret)
+				return ret;
+		} else if (PTR_ERR(parent) != -ENOSYS)
+			/* -ENOSYS means no parent and is OK */
+			return PTR_ERR(parent);
+	}

And this whole mess should be written:

    ret = clk_prepare(clk->parent);
    if (ret)
		return ret;

Which returns 0 when there is no parent and it also returns 0 when there is no prepare callback for the parent. Why the hell do we need all this ERRPTR checking mess and all this conditional crap ?

Quoted 3 lines · Thomas Gleixner

On Wed, 20 Apr 2011, Uwe Kleine-K?nig wrote:

Very useful changelog.

IMHO OK for a RFC patch.

Quoted 7 lines

And this whole mess should be written:

    ret = clk_prepare(clk->parent);
    if (ret)
		return ret;

Quoted 3 lines

Which returns 0 when there is no parent and it also returns 0 when there is no prepare callback for the parent. Why the hell do we need all this ERRPTR checking mess and all this conditional crap ?

struct clk has no parent member, there is only clk_get_parent(). If

there is no parent it returns ERR_PTR(-ENOSYS) and if you pass that to clk_prepare it tries to dereference it. So either it must not be called with an error pointer or clk_prepare et al. need adaption to handle

Quoted 5 lines · Uwe Kleine-K?nig

Quoted 3 lines · Thomas Gleixner

On Wed, 20 Apr 2011, Uwe Kleine-K?nig wrote:

Very useful changelog.

IMHO OK for a RFC patch.

Not at all.

Quoted 7 lines

Quoted 7 lines

And this whole mess should be written:

     ret = clk_prepare(clk->parent);
     if (ret)
		return ret;

Quoted 5 lines

Quoted 3 lines

Which returns 0 when there is no parent and it also returns 0 when there is no prepare callback for the parent. Why the hell do we need all this ERRPTR checking mess and all this conditional crap ?

struct clk has no parent member, there is only clk_get_parent(). If

Which is a complete failure to begin with. Why the heck do you need a callback to figure that out?

Because someone claimed, that you need a callback to make it safe from changing? Or what's the reason for this?

Quoted 4 lines

there is no parent it returns ERR_PTR(-ENOSYS) and if you pass that to clk_prepare it tries to dereference it. So either it must not be called with an error pointer or clk_prepare et al. need adaption to handle

The whole clk struct is an utter failure.

It's simply the least common denominator of all clk madness in the tree. Which results in a half documented data structure and a handful helper functions of which most of them are exported, though the functionality in them is less than the overhead of the EXPORT_SYMBOL.

That's not an abstraction and neither it's a framework. It's a half arsed conglomorate of primitives w/o the minimal documentation how those primitives should be used and why they are there in the first place.

This is new code and it should be aimed to cleanup things not to shuffle things around in a frenzy.

So what's wrong with that code:

1) struct clk is just a collection of function pointers and two locks

It lacks:

- a flag field for properties

- a field for the parent

- a field for the current clock rate

- a field for the base register

- a struct for the offsets of the most common registers relative to base

optionally a set of common register accessor functions like I did for the generic irq chip.

2) The "framework" API is just a set of low level primitive helper functions

It lacks:

- proper refcounting. clk_get() / clk_put() should do that at the framework level.

- the ability to propagate enable/disable/prepare/unprepare down through the parent tree

- proper mechanisms to sanity check clk_set_parent(), clk_set_rate() et. al.

This is not a implementation detail inside a specific clock. It's a problem which is common at least on the tree level:

rootclk
/ \
clk1 clk2
/ \
clk3 clk4
/
clk5

So now you want to change the rate of clk1. Can you do that?

No, but where is the sanity check which prevents that ?

In the clk1->set_rate() callback ?

Great, that's the wrong place.

So now you want to switch the parent of clk3 from clk1 to clk2. Can you do that?

No, but where is the sanity check which prevents that ?

In the clk3->set_parent() callback ?

Again, the wrong place.

And these are not problems of a particular clk implementation, these are general problems and those want to be addressed in a real framework.

It's debatable, whether you want to be able to change clocks which have active childs or not. If not the decision function is pretty simple. If yes, you need a list of child clks which you want to consult first before committing the change.

So unless you fix this stuff you should not even think about converting anything to this "framework". That's just waste of time and effort. Aside of declaring it stable and useful ....

The least thing which we need now are half baken "abstractions" which just shuffle code around for no value.

Quoted 7 lines · Thomas Gleixner

So what's wrong with that code:

1) struct clk is just a collection of function pointers and two locks

It lacks:

- a flag field for properties

Agree. I would very much like it.

Quoted 1 line

- a field for the parent

Agree.

Quoted 1 line

- a field for the current clock rate

Meh! Not a big deal. Some clocks don't have the concept of setting their rate since they share a parent with another unrelated clock (Ah, looks like you talk about with the tree diagram). So, it might be easier to have the rate inside each implementation if they need it. I shouldn't add too much code.

Quoted 1 line

- a field for the base register

Definitely no. It's completely useless for clocks whose registers don't all line up with fixed offsets from the base register. Now I will have to put one register here and the rest of the registers in the internal data?

Quoted 2 lines

- a struct for the offsets of the most common registers relative to base

Ok, you thought about rest of the registers. But, how can this fit in the common code? Each clock h/w implementation has totally different set of registers. Sometimes different even within the same SoC. This would be quite wasteful and doesn't even make sense to store at the top level. Also, storing offsets is a pain in the neck when the register space is not clean (happens more often than we would like :-().

Quoted 2 lines

optionally a set of common register accessor functions like I did for the generic irq chip.

Again, I don't see the point in having this in the common code. May be I'm missing something?

IMO, a better option instead of the base register and the offsets would be an option to have a priv_data pointer. I forgot the exact use case, but we thought that would have been helpful when we tried to port the msm clock driver in our tree on top of Jeremy's patches.

Quoted 7 lines

2) The "framework" API is just a set of low level primitive helper functions

It lacks:

- proper refcounting. clk_get() / clk_put() should do that at the framework level.

This has nothing to do with the patches Jeremy made. clk_get()/_put() is in clkdev. Also, I'm not sure if clk_get()/put() needs refcounting. That's like asking kalloc/kfree to have refcounting.

Quoted 2 lines

- the ability to propagate enable/disable/prepare/unprepare down through the parent tree

Agree. That would be nice. I think the main reason people were not pushing for it was to do things in manageable steps. It took a long time for people to agree on even the current framework Jeremy added.

Quoted 14 lines

- proper mechanisms to sanity check clk_set_parent(), clk_set_rate() et. al.

This is not a implementation detail inside a specific clock. It's a problem which is common at least on the tree level:

rootclk
/ \
clk1 clk2
/ \
clk3 clk4
/
clk5

Quoted 7 lines

So now you want to change the rate of clk1. Can you do that?

No, but where is the sanity check which prevents that ?

In the clk1->set_rate() callback ?

Great, that's the wrong place.

Ah! Nice to see that this bothers you too. This has been a point of contention with in our internal team too. I keep pushing back on requests to make child clock's set rate propagate up to the patent when the parent has two unrelated child clocks going to different devices.

IMO, the set rate should only work on the parent clock and if there really in a need to change the child clock rates, then the users of those child clocks will have to co-ordinate it amongst themselves. Although, our use case is a bit simpler in that most of the time the child clocks are direct branches (no dividers) of the parent.

I'm not sure how realistic/common your example of switching parents for clk3 is. May be it is -- I would interested in what people have to say.

So, between clk_set_divider(), clk_set_parent() and clk_set_rate(), I think we should be able to cover most clock trees as long as we don't propagate clk_set_rate() to parents with more than one children. In those case, the children should just implement clk_set_divider() (or not even that if there is no divider) and not allow clk_set_rate().

Quoted 3 lines

So unless you fix this stuff you should not even think about converting anything to this "framework". That's just waste of time and effort. Aside of declaring it stable and useful ....

I think you haven't seen all the repetition of refcounting and each mach's implementation of some variant of clock ops. The current patch set from Jeremy

will certainly help cut down some of that. If we get the "enable parent before you enable child, etc" in now, I don't think there will be much churn when we try to add code to enforce the tree restrictions you mention above.

Quoted 2 lines

The least thing which we need now are half baken "abstractions" which just shuffle code around for no value.

While a lot of your points are correct, I don't think the current patches from Jeremy are useless. May be I'm completely mistaken in assuming that you are referring to Jeremy's patches?

Btw, on a slight tangent, there is also the problem of clk_round_rate() not being sufficient when a driver is trying to work across different mach's. I think we need a clk_set_rate_range(min, ideal, max) but I can start a separate thread on that if you want. I talked about it a bit in another thread.

Quoted 6 lines · Saravana Kannan

Quoted 5 lines · Thomas Gleixner

2) The "framework" API is just a set of low level primitive helper functions

It lacks:

Quoted 6 lines

Quoted 2 lines

- proper refcounting. clk_get() / clk_put() should do that at the framework level.

This has nothing to do with the patches Jeremy made. clk_get()/_put() is in clkdev. Also, I'm not sure if clk_get()/put() needs refcounting. That's like asking kalloc/kfree to have refcounting.

Ok. I missed the clkdev part.

Depends, there is a lot of sane hardware out there (not necessarily in the ARM SoC world). We can do with a pointer if the need arises.

Quoted 5 lines

Quoted 2 lines

optionally a set of common register accessor functions like I did for the generic irq chip.

Again, I don't see the point in having this in the common code. May be I'm missing something?

See my RFC patch of a generic irq chip implementation and how much duplicated five line inline functions they removed.

Quoted 4 lines

IMO, a better option instead of the base register and the offsets would be an option to have a priv_data pointer. I forgot the exact use case, but we thought that would have been helpful when we tried to port the msm clock driver in our tree on top of Jeremy's patches.

It works either way, but we should try to comeup with a sensible common base struct for sane hardware.

Quoted 11 lines · Uwe Kleine-K?nig

Quoted 9 lines · Thomas Gleixner

And this whole mess should be written:

    ret = clk_prepare(clk->parent);
    if (ret)
		return ret;

Which returns 0 when there is no parent and it also returns 0 when there is no prepare callback for the parent. Why the hell do we need all this ERRPTR checking mess and all this conditional crap ?

Quoted 1 line

struct clk has no parent member, there is only clk_get_parent(). If

Which is a complete failure to begin with. Why the heck do you need a callback to figure that out?

Because someone claimed, that you need a callback to make it safe from changing? Or what's the reason for this?

Quoted 4 lines

there is no parent it returns ERR_PTR(-ENOSYS) and if you pass that to clk_prepare it tries to dereference it. So either it must not be called with an error pointer or clk_prepare et al. need adaption to handle

The whole clk struct is an utter failure.

It's simply the least common denominator of all clk madness in the tree. Which results in a half documented data structure and a handful helper functions of which most of them are exported, though the functionality in them is less than the overhead of the EXPORT_SYMBOL.

That's not an abstraction and neither it's a framework. It's a half arsed conglomorate of primitives w/o the minimal documentation how those primitives should be used and why they are there in the first place.

This is new code and it should be aimed to cleanup things not to shuffle things around in a frenzy.

So what's wrong with that code:

1) struct clk is just a collection of function pointers and two locks

It lacks:

- a flag field for properties

- a field for the parent

- a field for the current clock rate

- a field for the base register

- a struct for the offsets of the most common registers relative to base

optionally a set of common register accessor functions like I did for the generic irq chip.

2) The "framework" API is just a set of low level primitive helper functions

It lacks:

- proper refcounting. clk_get() / clk_put() should do that at the framework level.

- the ability to propagate enable/disable/prepare/unprepare down through the parent tree

- proper mechanisms to sanity check clk_set_parent(), clk_set_rate() et. al.

This is not a implementation detail inside a specific clock. It's a problem which is common at least on the tree level:

rootclk
/ \
clk1 clk2
/ \
clk3 clk4
/
clk5

So now you want to change the rate of clk1. Can you do that?

No, but where is the sanity check which prevents that ?

In the clk1->set_rate() callback ?

Great, that's the wrong place.

So now you want to switch the parent of clk3 from clk1 to clk2. Can you do that?

No, but where is the sanity check which prevents that ?

In the clk3->set_parent() callback ?

Again, the wrong place.

And these are not problems of a particular clk implementation, these are general problems and those want to be addressed in a real framework.

It's debatable, whether you want to be able to change clocks which have active childs or not. If not the decision function is pretty simple. If yes, you need a list of child clks which you want to consult first before committing the change.

So unless you fix this stuff you should not even think about converting anything to this "framework". That's just waste of time and effort. Aside of declaring it stable and useful ....

Quoted 5 lines

Which is a complete failure to begin with. Why the heck do you need a callback to figure that out?

Because someone claimed, that you need a callback to make it safe from changing? Or what's the reason for this?

If there were a pointer tracking the parent you still needed to program out the get_parent function anyhow to set the pointer initially. But I agree that in other situations having a pointer is more comfortable and saves ugly error handling e.g. in set_parent.

Quoted 3 lines

It lacks:

- a flag field for properties

Up to now there are no properties to distinguish. When the first comes up such a field is easily added, no?

Quoted 1 line

- a field for the parent

I can agree here, see above.

Quoted 1 line

- a field for the current clock rate

Then you need a mechanism to notify the children if the rate or parent of a clock changes. I assume you won't allow that for a clock that has more than one (enabled) consumer.

Knowing only very few platforms I
don't know if that is something you can forbid.

Quoted 1 line

- a field for the base register

This is not needed for all clocks (at least not a say void __iomem *). I think it's fine here to leave that to the platforms as it is now.

Quoted 2 lines

- a struct for the offsets of the most common registers relative to base

I'm not sure I understand you right here. If this is about not duplicating data/code for similar clocks, this works (IMHO) fine with the current approach (ie. sharing the ops pointer).

Quoted 2 lines

optionally a set of common register accessor functions like I did for the generic irq chip.

Agreed, this can (and should) come when the base API is fixed.

Quoted 5 lines

2) The "framework" API is just a set of low level primitive helper functions

It lacks:

Quoted 2 lines

- proper refcounting. clk_get() / clk_put() should do that at the framework level.

For most clocks it's not important if they are referenced somewhere. What should be done with that reference count at the framework level? The only thing I currently can come up with now is to module_get an optional owner. I'm not convinced this is worth to include in the framework. Still it should be possible to do, and it is.

Quoted 16 lines

- proper mechanisms to sanity check clk_set_parent(),

     clk_set_rate() et. al.

     This is not a implementation detail inside a specific clock. It's
     a problem which is common at least on the tree level:

                    rootclk
                 /          \
              clk1          clk2   
             /   \
           clk3  clk4
          /
         clk5

    So now you want to change the rate of clk1. Can you do that?

Quoted 1 line

No, but where is the sanity check which prevents that ?

I assume your ascii art should imply that clk3 and clk4 are enabled? What is the policy you suggest?
This was discussed at some point[1] with the result that the user of set_rate should be aware of the clk tree and the changes it introduces for now and that some policy could still be added later. Ditto for set_parent.

It's easy enough to implement such a policy on top of Jeremy's patches. I'm not able to see already now all the requirements of all users of the clk-API. So I agree with you that it probably isn't (yet) the perfect framework, but I think it's a good base to see these requirements.

Quoted 3 lines

So unless you fix this stuff you should not even think about converting anything to this "framework". That's just waste of time and effort. Aside of declaring it stable and useful ....

With stable I didn't intend to say something about the quality (though I still think it's a good thing), but that I won't rewrite the commits such that others can base their work on top of them.

Before your mail nobody spoke up against these patches and some people see a value in them and want to convert their platform to it. So

providing a branch seems sensible to me.

I don't say that this common struct clk is perfect already now and I expect it to change here and there. It allows to consolidate some code and so is a good thing. I think nobody disagrees that when having converted most or all platforms to it we're not done. It's just the first step. Maybe one of the next is to add a "class" of clocks that looks similar to this:

Quoted 4 lines · Uwe Kleine-K?nig

If there were a pointer tracking the parent you still needed to program out the get_parent function anyhow to set the pointer initially. But I agree that in other situations having a pointer is more comfortable and saves ugly error handling e.g. in set_parent.

That's nonsense. Why do you want a get_parent() function at all? parent is set up when the clock is established in the clock tree. And that's not done by some random driver. That's a property of the clock.

Quoted 4 lines

Quoted 1 line

- a field for the base register

This is not needed for all clocks (at least not a say void __iomem *). I think it's fine here to leave that to the platforms as it is now.

Crap. All clocks which are configurable in some way or can be enabled/disabled need access register(s) so you need a base address.

Quoted 10 lines · Thomas Gleixner

Quoted 4 lines · Uwe Kleine-K?nig

If there were a pointer tracking the parent you still needed to program out the get_parent function anyhow to set the pointer initially. But I agree that in other situations having a pointer is more comfortable and saves ugly error handling e.g. in set_parent.

That's nonsense. Why do you want a get_parent() function at all? parent is set up when the clock is established in the clock tree. And that's not done by some random driver. That's a property of the clock.

That's not universal - right now the drivers can and do know about this on some platforms. In some cases there will be requirements beyond the clock rate which mean that the driver will need to be able to tell the framework that, for example, it needs one clock to be in the same clock domain as another.

Quoted 12 lines · Mark Brown

Quoted 10 lines · Thomas Gleixner

Quoted 4 lines · Uwe Kleine-K?nig

If there were a pointer tracking the parent you still needed to program out the get_parent function anyhow to set the pointer initially. But I agree that in other situations having a pointer is more comfortable and saves ugly error handling e.g. in set_parent.

That's nonsense. Why do you want a get_parent() function at all? parent is set up when the clock is established in the clock tree. And that's not done by some random driver. That's a property of the clock.

Quoted 5 lines

That's not universal - right now the drivers can and do know about this on some platforms. In some cases there will be requirements beyond the clock rate which mean that the driver will need to be able to tell the framework that, for example, it needs one clock to be in the same clock domain as another.

Also there can be multiple parent clocks. An example are the timers on omaps where the parent clock can be either the 32KiHz clock or external source clock. ?his may need to be reprogrammed dynamically in some cases for advanced idle modes as one of the parent clocks may be shut down.

Quoted 19 lines · Tony Lindgren

Quoted 17 lines · Mark Brown

Quoted 10 lines · Thomas Gleixner

Quoted 4 lines · Uwe Kleine-K?nig

If there were a pointer tracking the parent you still needed to program out the get_parent function anyhow to set the pointer initially. But I agree that in other situations having a pointer is more comfortable and saves ugly error handling e.g. in set_parent.

That's nonsense. Why do you want a get_parent() function at all? parent is set up when the clock is established in the clock tree. And that's not done by some random driver. That's a property of the clock.

That's not universal - right now the drivers can and do know about this on some platforms. In some cases there will be requirements beyond the clock rate which mean that the driver will need to be able to tell the framework that, for example, it needs one clock to be in the same clock domain as another.

Quoted 4 lines

Also there can be multiple parent clocks. An example are the timers on omaps where the parent clock can be either the 32KiHz clock or external source clock. ?his may need to be reprogrammed dynamically in some cases for advanced idle modes as one of the parent clocks may be shut down.

If you need to propagate from bottom up, then you need a list of childs in struct clk. Right ?

Quoted 6 lines · Thomas Gleixner

Quoted 4 lines · Tony Lindgren

Also there can be multiple parent clocks. An example are the timers on omaps where the parent clock can be either the 32KiHz clock or external source clock. ?his may need to be reprogrammed dynamically in some cases for advanced idle modes as one of the parent clocks may be shut down.

Quoted 2 lines

If you need to propagate from bottom up, then you need a list of childs in struct clk. Right ?

Yes we need to be able to reprogram all children if the parent clock needs to be reprogrammed for cpufreq/DVFS.

Also a child may need to change the parent clock like I wrote above. However, it might be possible to deal with this by registering the same child for both parents and handle that directly in the driver for the special cases if that simplifies things.

In some clk_set_rate cases for a child the parent clock may need to be reprogrammed to provide the desired rate. I don't know how common this is, and this may be doable by requesting both parent and child in the driver if that simplifies things.

That's not reliably the case, some systems have some of the clock tree (esepcialy the core parts of the tree) controlled by a system management unit which isn't memory mapped and it's common for I2C or SPI connected chips to be involved to some extent too. One advantage we should be able to get from this framework is that it will be much easier to have standard drivers for off-SoC chips that could be shared.

Quoted 5 lines · Uwe Kleine-K?nig

Quoted 5 lines

Which is a complete failure to begin with. Why the heck do you need a callback to figure that out?

Because someone claimed, that you need a callback to make it safe from changing? Or what's the reason for this?

Quoted 4 lines

If there were a pointer tracking the parent you still needed to program out the get_parent function anyhow to set the pointer initially. But I agree that in other situations having a pointer is more comfortable and saves ugly error handling e.g. in set_parent.

That's nonsense. Why do you want a get_parent() function at all? parent is set up when the clock is established in the clock tree. And that's not done by some random driver. That's a property of the clock.

Quoted 11 lines · Thomas Gleixner

Quoted 9 lines · Uwe Kleine-K?nig

Quoted 5 lines

Which is a complete failure to begin with. Why the heck do you need a callback to figure that out?

Because someone claimed, that you need a callback to make it safe from changing? Or what's the reason for this?

If there were a pointer tracking the parent you still needed to program out the get_parent function anyhow to set the pointer initially. But I agree that in other situations having a pointer is more comfortable and saves ugly error handling e.g. in set_parent.

Quoted 3 lines

That's nonsense. Why do you want a get_parent() function at all? parent is set up when the clock is established in the clock tree. And that's not done by some random driver. That's a property of the clock.

It's *not* a fixed property of the clock. It's a runtime property.

If you change the rate of a clock, you may need to change its parent.

If you change the operating point of the device, (eg, sleep mode where the main system PLL gets turned off) you may need to change its parent to a slower clock to ensure wakeup timers continue to run but at a lower resolution.

Sure the API doesn't handle the runtime cases at all well, but that's not a reason to enforce a fixed tree structure on something which isn't a fixed tree structure.

Quoted 16 lines · Russell King - ARM Linux

Quoted 14 lines · Thomas Gleixner

Quoted 9 lines · Uwe Kleine-K?nig

Quoted 5 lines

Which is a complete failure to begin with. Why the heck do you need a callback to figure that out?

Because someone claimed, that you need a callback to make it safe from changing? Or what's the reason for this?

If there were a pointer tracking the parent you still needed to program out the get_parent function anyhow to set the pointer initially. But I agree that in other situations having a pointer is more comfortable and saves ugly error handling e.g. in set_parent.

That's nonsense. Why do you want a get_parent() function at all? parent is set up when the clock is established in the clock tree. And that's not done by some random driver. That's a property of the clock.

Quoted 1 line

It's *not* a fixed property of the clock. It's a runtime property.

I know that stuff can change and needs to be changed runtime. But this has to be done at the framework level and not at some random place in some random clk->ops->fn() implementation.

Quoted 18 lines · Thomas Gleixner

Quoted 16 lines · Russell King - ARM Linux

Quoted 14 lines · Thomas Gleixner

Quoted 9 lines · Uwe Kleine-K?nig

Quoted 5 lines

Which is a complete failure to begin with. Why the heck do you need a callback to figure that out?

Because someone claimed, that you need a callback to make it safe from changing? Or what's the reason for this?

If there were a pointer tracking the parent you still needed to program out the get_parent function anyhow to set the pointer initially. But I agree that in other situations having a pointer is more comfortable and saves ugly error handling e.g. in set_parent.

That's nonsense. Why do you want a get_parent() function at all? parent is set up when the clock is established in the clock tree. And that's not done by some random driver. That's a property of the clock.

Quoted 5 lines

Quoted 1 line

It's *not* a fixed property of the clock. It's a runtime property.

I know that stuff can change and needs to be changed runtime. But this has to be done at the framework level and not at some random place in some random clk->ops->fn() implementation.

Look, how do you _not_ do it at the clk->ops->fn() level when you have clocks which are pure muxes, and so are selected by a bitfield in a register vs other clocks which are muxes _and_ dividers combined which can't be separated. Do you _really_ want to teach the core clock framework about every type of platform quirk like that?

That's the direction you're heading for by demanding that the core clk code does all this kind of crap. Crap in the core clock code, rather than a set of compartmentalized clock implementations for fixed rate clocks, muxes, dividers, dividers with muxes, plls, etc.

This are all well defined semantical problems and should be solved in common code rather than having different buggy implementations in each SoC clock "framework".

Quoted 3 lines · Thomas Gleixner

This are all well defined semantical problems and should be solved in common code rather than having different buggy implementations in each SoC clock "framework".

Why are you associating functional elements in drivers/clk with the SoC clock framework?

I think that's the root cause of this misunderstanding, and until you start realising that these building blocks are not part of the SoC implementation, we're going to continue arguing about this.

Lets look at a simple example (locking omitted)

clk_enable(clk) {
	if (clk->enable_count == 0) {
  	   res = clk->ops->enable(clk);
	   if (res)
	      return res;
        }
	enable_count++;
	return 0;
};   

So in each enable op you have:

*_enable(clk)

{
	res = clk_enable(clk_get_parent(clk));
	if (res)
		return res;

	res = hw_magic_enable(clk);
	if (res)
		clk_disable(clk_get_parent(clk));
	return res;
}

Instead of having at the core level:

clk_enable(clk)

{
	if (clk->enable_count == 0) {
		res = clk_enable(clk_get_parent(clk));
		if (res)
			return res;

		res = clk->ops->enable(clk);
		if (res) {
			clk_disable(clk_get_parent(clk));
			return res;
		}
	}
	clk->enable_count++;
	return 0;
}

Quoted 1 line · Thomas Gleixner

}

I'm going to give up discussing this with you - you have no desire to even understand what I'm saying. I don't see any point continuing this discussion any further.

Quoted 6 lines · Russell King - ARM Linux

Quoted 1 line · Thomas Gleixner

}

I'm going to give up discussing this with you - you have no desire to even understand what I'm saying. I don't see any point continuing this discussion any further.

Right, I have no interest in understanding what you want to do.

Quoted 4 lines · Thomas Gleixner

Quoted 3 lines · Russell King - ARM Linux

Quoted 1 line · Thomas Gleixner

}

Quoted 5 lines

Quoted 3 lines

I'm going to give up discussing this with you - you have no desire to even understand what I'm saying. I don't see any point continuing this discussion any further.

Right, I have no interest in understanding what you want to do.

And that's your basic problem. You don't even want to try and understand these arguments. So its pointless continuing the discussion.

Thanks for confirming that I've been wasting my time over the last couple of days.

Quoted 3 lines · Thomas Gleixner

This are all well defined semantical problems and should be solved in common code rather than having different buggy implementations in each SoC clock "framework".

Why are you associating functional elements in drivers/clk with the SoC clock framework?

I think that's the root cause of this misunderstanding, and until you start realising that these building blocks are not part of the SoC implementation, we're going to continue arguing about this.

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

Quoted 7 lines · Russell King - ARM Linux

Quoted 3 lines · Thomas Gleixner

This are all well defined semantical problems and should be solved in common code rather than having different buggy implementations in each SoC clock "framework".

Why are you associating functional elements in drivers/clk with the SoC clock framework?

i think i pretty much understand what thomas is after. but i have problems understanding, what you want here.

until this point, it looks like you only want the driver/clk to model the clock endpoints inside the devices. A device itself shouldnt be caring for clock parents or anything. its only concern is that a clock pin is fed with the right frequency, or off...

Quoted 4 lines

I think that's the root cause of this misunderstanding, and until you start realising that these building blocks are not part of the SoC implementation, we're going to continue arguing about this.

what you say now, pretty much sounds like what thomas wants. these building blocks would be objects derived from the clock baseclass, which thomas is trying to define. he doesnt seem to be concerned about the second more special layer yet.

i am quite puzzled, what exactly your fighting over :S

Quoted 27 lines · torbenh

Quoted 7 lines · Russell King - ARM Linux

Quoted 3 lines · Thomas Gleixner

This are all well defined semantical problems and should be solved in common code rather than having different buggy implementations in each SoC clock "framework".

Why are you associating functional elements in drivers/clk with the SoC clock framework?

i think i pretty much understand what thomas is after. but i have problems understanding, what you want here.

until this point, it looks like you only want the driver/clk to model the clock endpoints inside the devices. A device itself shouldnt be caring for clock parents or anything. its only concern is that a clock pin is fed with the right frequency, or off...

Quoted 4 lines

I think that's the root cause of this misunderstanding, and until you start realising that these building blocks are not part of the SoC implementation, we're going to continue arguing about this.

what you say now, pretty much sounds like what thomas wants. these building blocks would be objects derived from the clock baseclass, which thomas is trying to define. he doesnt seem to be concerned about the second more special layer yet.

Quoted 1 line

i am quite puzzled, what exactly your fighting over :S

Thomas wants a core framework that handles the clock tree, with each clock being the implementation of just the clock. Russell thinks this is impossible, and wants to stop at implementing building blocks for the standard clock types, and let each SoC put them into a tree.

I don't think anybody can argue that a shared clock tree framework, if it was possible to do well, would be a benefit. But there are a few problems that make abstracting the differences between SoCs very hard at that level.

After implementing the Tegra clock tree at least 10 times to solve different problems, there are two very hard problems to solve.

First is locking. Per-clock locking is hard because it requires choosing up front which way locking will go, usually locking the child, then locking the parent, then enabling the parent, and enabling the child. Any operation flow that requires traversing the tree from the parent to the child, for example changing the rate of the parent, causes lock inversion. Global locking is easy, but can be wasteful on platforms that have some very slow clocks (think external i2c clocks), and some very fast clocks (internal register writes). This is mitigated by the separation of clk_prepare and clk_enable locking - clk_enable can never sleep, so it can't be blocked for long. Locking at the root node of the tree would be nice, but most SoCs don't have a root node - they have a fast root clock and a slow root clock, and can switch the all or most of clock tree over to the slow clock in some modes.

The second problem is rate changing. Different platforms require wildly different strategies for changing clock rates. On Tegra, clk_set_rate will never change the parent, because we can generally set up the parent clocks to the correct frequencies for all the devices, and then device drivers can change their dividers to get the rate they need. The few cases where the parent needs to change (cpufreq and display pclk), the driver manually calls clk_set_parent. On other platforms, it is common to need to change the parent to get the necessary rate.

Automatically selecting parents in a generic way is impossible. If one DEV1 calls clk_set_rate and clk_enable, and the core framework responds by setting pll A to frequency X, and setting the parent of DEV1 to A, what happens when DEV2 calls clk_set_rate to a frequency that can only be satisfied by setting pll A to frequency Y? DEV1 can't be switched to another pll, because it is already enabled, and some platforms can't support glitchlessly changing a mux, even if the frequency doesn't change.

I still think a common implementation of the clock tree framework would be useful, even if not all platforms can use it. Allow the platforms that can to use it, providing only clock data, and maybe a few clock implementations if the building blocks are not sufficient, and allow those that can't to implement their own versions of the clk_* functions on top of the clock building blocks. Tegra already has a fairly generic implementation of a clock tree in arch/arm/mach-tegra/clock.c, moving it to the common clk struct and sharing it with other platforms wouldn't be any extra code, even if no other platforms could use it. And I bet at least OMAP could use it, from what I've seen.

Per-tree locking is a solvable problem, as long as a tree was considered as an ordered graph - two root nodes that feed into the same clocks would be considered part of the same tree. The core framework could look at all possible parents of a clock to determine what clock trees were truly independent, which would result in a single global lock on most platforms, but multiple locks on platforms that really had independent clock trees. Dynamically added clocks would cause problems with this solution.

I think automatic clock parent and rate setting would need to be delegated to a platform-specific layer, and I'm not sure how. Maybe allow platforms to override the set_rate ops in the clock tree building blocks, or a new op that can determine the new tree structure, and call the old set_rate op to set the registers.

Quoted 3 lines · Thomas Gleixner

This are all well defined semantical problems and should be solved in common code rather than having different buggy implementations in each SoC clock "framework".

Why are you associating functional elements in drivers/clk with the SoC clock framework?

I think that's the root cause of this misunderstanding, and until you start realising that these building blocks are not part of the SoC implementation, we're going to continue arguing about this.

I think Thomas mostly cares about having the tree handling done at the core code level. Russell's main point seems to be that the parent can change for reasons other than "clk_set_parent()" and hence we can have the core code manage all of the parent handling.

Quoted 1 line · Thomas Gleixner

- a field for the current clock rate

Then you need a mechanism to notify the children if the rate or parent of a clock changes. I assume you won't allow that for a clock that has more than one (enabled) consumer. Knowing only very few platforms I don't know if that is something you can forbid.

Quoted 3 lines · Uwe Kleine-K?nig

Quoted 1 line · Thomas Gleixner

- a field for the current clock rate

Quoted 4 lines

Then you need a mechanism to notify the children if the rate or parent of a clock changes. I assume you won't allow that for a clock that has more than one (enabled) consumer. Knowing only very few platforms I don't know if that is something you can forbid.

I suspect you need it - cpufreq style scaling of the core clocks (which many SoCs can do) involves pretty much the entire clock tree, though obviously the idea is that the effects get hidden at the edges.

Quoted 5 lines · Uwe Kleine-K?nig

Quoted 3 lines · Thomas Gleixner

On Wed, 20 Apr 2011, Uwe Kleine-K?nig wrote:

Very useful changelog.

IMHO OK for a RFC patch.

Not at all.

Quoted 7 lines

Quoted 7 lines

And this whole mess should be written:

    ret = clk_prepare(clk->parent);
    if (ret)
		return ret;

Quoted 5 lines

Quoted 3 lines

Which returns 0 when there is no parent and it also returns 0 when there is no prepare callback for the parent. Why the hell do we need all this ERRPTR checking mess and all this conditional crap ?

struct clk has no parent member, there is only clk_get_parent(). If

Which is a complete failure to begin with. Why the heck do you need a callback to figure that out?

Because someone claimed, that you need a callback to make it safe from changing? Or what's the reason for this?

Quoted 4 lines

there is no parent it returns ERR_PTR(-ENOSYS) and if you pass that to clk_prepare it tries to dereference it. So either it must not be called with an error pointer or clk_prepare et al. need adaption to handle

The whole clk struct is an utter failure.

It's simply the least common denominator of all clk madness in the tree. Which results in a half documented data structure and a handful helper functions of which most of them are exported, though the functionality in them is less than the overhead of the EXPORT_SYMBOL.

That's not an abstraction and neither it's a framework. It's a half arsed conglomorate of primitives w/o the minimal documentation how those primitives should be used and why they are there in the first place.

This is new code and it should be aimed to cleanup things not to shuffle things around in a frenzy.

So what's wrong with that code:

1) struct clk is just a collection of function pointers and two locks

It lacks:

- a flag field for properties

- a field for the parent
- a field for the current clock rate

- a field for the base register
- a struct for the offsets of the most common registers relative to base

The least thing which we need now are half baken "abstractions" which just shuffle code around for no value.

Quoted 2 lines · Thomas Gleixner

- a field for the parent
- a field for the current clock rate

Currently there is no possibility to bring the clock tree in sync with the hardware. clocks are not registered at all, they are just there. So there is no instance who could iterate over the tree in order to bring it in sync. Unless we create that (which I would like) there is no point in adding rate and parent fields.

Hm, thinking about this maybe we could add a CLK_IN_SYNC field which is initially cleared and gets set once the get_parent()/get_rate() callback has been called for a clock.

Quoted 3 lines

- a field for the base register
- a struct for the offsets of the most common registers relative to base

What's wrong with embedding struct clk into a more specific clock and access it with container_of()? It must be done anyway once a field is missing in struct clk, or we end up with a lot of fields in struct clk which are used in only few types of clocks.

Quoted 3 lines

The least thing which we need now are half baken "abstractions" which just shuffle code around for no value.

While I agree with several things you say even the half baken abstractions help to bring the problems into the light. What we have now is that all these problems are hidden in SoC specific abstractions which all have their own problems and bugs.

Quoted 10 lines · Sascha Hauer

Quoted 3 lines · Thomas Gleixner

- a field for the base register
- a struct for the offsets of the most common registers relative to base

What's wrong with embedding struct clk into a more specific clock and access it with container_of()? It must be done anyway once a field is missing in struct clk, or we end up with a lot of fields in struct clk which are used in only few types of clocks.

As I explained already, that's the whole purpose of frameworks. See clockevents, clocksource, genirq. Lots of stuff is only used by a very small subset, but it's better to have that in common code than growing warts at all ends.

Quoted 12 lines · Thomas Gleixner

Quoted 10 lines · Sascha Hauer

Quoted 3 lines · Thomas Gleixner

- a field for the base register
- a struct for the offsets of the most common registers relative to base

What's wrong with embedding struct clk into a more specific clock and access it with container_of()? It must be done anyway once a field is missing in struct clk, or we end up with a lot of fields in struct clk which are used in only few types of clocks.

Quoted 4 lines

As I explained already, that's the whole purpose of frameworks. See clockevents, clocksource, genirq. Lots of stuff is only used by a very small subset, but it's better to have that in common code than growing warts at all ends.

There's nothing intrinsic about the concept of a clock which means that it needs to be memory mapped or have a particular set of registers for us to work with. This sounds like something that ought to be factored out but it's not obvious to me that it should be part of the core struct.

Quoted 4 lines · Mark Brown

There's nothing intrinsic about the concept of a clock which means that it needs to be memory mapped or have a particular set of registers for us to work with. This sounds like something that ought to be factored out but it's not obvious to me that it should be part of the core struct.

Care to look over the various implementations and find out that a lot of them look like:

Just in about 100 variations of the theme. Which is basically the same as we have in irq chip implementations. And I showed how much of these things can be factored out when done right. And even if a particular clock does not implement enable/disable calls, then having regs.enable_reg around is not a problem.

Because that's what we implement via container_of now. And we can add such stuff which is obvious right away.

Quoted 6 lines · Thomas Gleixner

Quoted 4 lines · Mark Brown

There's nothing intrinsic about the concept of a clock which means that it needs to be memory mapped or have a particular set of registers for us to work with. This sounds like something that ought to be factored out but it's not obvious to me that it should be part of the core struct.

Quoted 2 lines

Care to look over the various implementations and find out that a lot of them look like:

Please bear in mind that due to the fact that we've currently got one clock implementation per subarchitecture anything off-SoC has some serious barriers to implementation in kernel. For the system management units you'd be looking at very new systems, this is a new development.

Quoted 6 lines

Just in about 100 variations of the theme. Which is basically the same as we have in irq chip implementations. And I showed how much of these things can be factored out when done right. And even if a particular clock does not implement enable/disable calls, then having regs.enable_reg around is not a problem.

This is why I said "This sounds like something that ought to be factored out but it's not obvious to me that it should be part of the core struct" above - I agree strongly that we should have a generic memory mapped implementation which factors out all the redundant code you'd get for these clocks.

Quoted 2 lines

Because that's what we implement via container_of now. And we can add such stuff which is obvious right away.

That's exactly the sort of thing I'm suggesting. You were asking for this to be part of struct clk itself, embedding the struct clk in a struct mm_clk (or whatever) and having standard ops for the mm_clk a much better abstraction.

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

Quoted 4 lines · Mark Brown

Please bear in mind that due to the fact that we've currently got one clock implementation per subarchitecture anything off-SoC has some serious barriers to implementation in kernel. For the system management units you'd be looking at very new systems, this is a new development.

How many arch have off-soc clocks? Where's the source?
At least for arm, nearly all is memory mapped clock registers. clk is free to override, we just need abstract the most common things.

Quoted 6 lines · Richard Zhao

Quoted 4 lines · Mark Brown

Please bear in mind that due to the fact that we've currently got one clock implementation per subarchitecture anything off-SoC has some serious barriers to implementation in kernel. For the system management units you'd be looking at very new systems, this is a new development.

Quoted 3 lines

How many arch have off-soc clocks? Where's the source?
At least for arm, nearly all is memory mapped clock registers. clk is free to override, we just need abstract the most common things.

Even for many ARMs it's common for the clock tree to be rooted in the PMIC (since it will always be on), and of course the full system clock tree will not just include the CPU but also any other digital components in the system.

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

Quoted 4 lines · Mark Brown

Even for many ARMs it's common for the clock tree to be rooted in the PMIC (since it will always be on), and of course the full system clock tree will not just include the CPU but also any other digital components in the system.

Yes, I see pmic or other on-board osc may be clock source. I just didn't realize anyone will put them to clk.

Care to look over the various implementations and find out that a lot of them look like:

static int _clk_enable(struct clk *clk)

{
        unsigned int reg;

        reg = __raw_readl(clk->enable_reg);
        reg |= 1 << clk->enable_shift;
        __raw_writel(reg, clk->enable_reg);

        return 0;
}

Just in about 100 variations of the theme. Which is basically the same as we have in irq chip implementations. And I showed how much of these things can be factored out when done right. And even if a particular clock does not implement enable/disable calls, then having regs.enable_reg around is not a problem.

Quoted 5 lines · Thomas Gleixner

Just in about 100 variations of the theme. Which is basically the same as we have in irq chip implementations. And I showed how much of these things can be factored out when done right. And even if a particular clock does not implement enable/disable calls, then having regs.enable_reg around is not a problem.

And you haven't noticed that with how things stand with what Jeremy's proposed, we _can_ abstract this kind of stuff.

Having read a number of your messages, I think you don't like it because you haven't been involved in creating it - I don't think there's anything much more specific than that to your protestations about it.

We _can_ (and I hope we do) factor out the 100 "clock gate" implementations into one instance - and I believe we can do that with Jeremy's proposal.

For those clocks which have an clock gate and a clock mux, we can create two clk structures, one being the clock gate container and one being the clock mux container, even if they're accessing the same register.

Shoving the enable crap into the core framework enforces that every clock has an gate attached to it, which is soo far departed from reality I've got to ask what universe you're living in.

};

If you start saying "a clock struct should contain enable bit, register address" then you're well on the path to creating a horrid mess like the above. And this is something we _must_ avoid.

So, Jeremy's done the right thing and created a base struct clk which is very much like a kobject, along with a set of methods to operate on it. Around that can be built things like fixed rate clocks, MUXes, gates, PLLs, and everything else which is required with minimal expense
- and all provided by core clk code. And we avoid crappy bloated structures like the thing above.

I tend to disagree. Yes, he created the minimal common level for a struct clk and the ops around it. But does that change anything about the status quo of duplicated functions walking the tree, duplicated functions to validate and round clock rates? The whole approach will keep the whole code mess in the SoCs the same as it is now and just enforce a common data structure and a dozen wrapper "API" functions around it.

So omap will simply have their bloat in a struct omap_clk() and every other SoC will do the same. The duplicated code, the locking mess etc. will all remain in totally incompatible ways. So what's the win?

Quoted 11 lines · Thomas Gleixner

I tend to disagree. Yes, he created the minimal common level for a struct clk and the ops around it. But does that change anything about the status quo of duplicated functions walking the tree, duplicated functions to validate and round clock rates? The whole approach will keep the whole code mess in the SoCs the same as it is now and just enforce a common data structure and a dozen wrapper "API" functions around it.

So omap will simply have their bloat in a struct omap_clk() and every other SoC will do the same. The duplicated code, the locking mess etc. will all remain in totally incompatible ways. So what's the win?

I disagree. With a 'divider+mux' clk, OMAP can move those clocks over. With a 'pll' clk, OMAP can move the PLL stuff over. With a simple enable/disable clk, OMAP can move those over. That's a significant portion of the OMAP clk mess converted over to smaller, much more well defined, self contained clk structures.

I don't see any reason with this why OMAP - or anyone else - should have their own platform_clk structure if we have the necessary building blocks, and provided there is sufficient review to ensure that those building blocks which are found to be lacking get fixed along the way.

As for the locking mess, that's what happens when you try to generalize stuff and end up with a finer locking scheme. The more locks you have, the more complex the locking rules become, and the more problems you encounter with potential deadlocks. And it seems that lockdep also starts to struggle too (as it operates on classes of locks not actual locks.)

The per clk locking is not going to work ever except you want to have the trylock dance if you need to propagate changes up the tree and down the tree. So yes, we need per tree locking and I can see the need for the mutex and the spinlock to protect the tree, but you really want to have the logic dealing with those locks and how they both protect the tree in common code and not in some random place.

Quoted 6 lines · Thomas Gleixner

The per clk locking is not going to work ever except you want to have the trylock dance if you need to propagate changes up the tree and down the tree. So yes, we need per tree locking and I can see the need for the mutex and the spinlock to protect the tree, but you really want to have the logic dealing with those locks and how they both protect the tree in common code and not in some random place.

rotfl. "per tree locking". That's farsical when you have muxes which select between different distinct trees. That _surely_ is the path to locking mayhem, because the child clocks can have their lock changed beneath them as the mux changes its parent.

I still think you're in a different universe.

Quoted 9 lines · Sascha Hauer

Quoted 2 lines · Thomas Gleixner

- a field for the parent
- a field for the current clock rate

Currently there is no possibility to bring the clock tree in sync with the hardware. clocks are not registered at all, they are just there. So there is no instance who could iterate over the tree in order to bring it in sync. Unless we create that (which I would like) there is no point in adding rate and parent fields.

And that's where the problem starts. What's the point of that current conglomorate of function pointers and half baken helper wrappers if we don't have answers for how to populate the clk tree, how to simplify stuff w/o the need of tons of callbacks which have to be implemented all over the place ?

Quoted 4 lines

Hm, thinking about this maybe we could add a CLK_IN_SYNC field which is initially cleared and gets set once the get_parent()/get_rate() callback has been called for a clock.

Quoted 8 lines

Quoted 3 lines

- a field for the base register
- a struct for the offsets of the most common registers relative to base

What's wrong with embedding struct clk into a more specific clock and access it with container_of()? It must be done anyway once a field is missing in struct clk, or we end up with a lot of fields in struct clk which are used in only few types of clocks.

As I explained already, that's the whole purpose of frameworks. See clockevents, clocksource, genirq. Lots of stuff is only used by a very small subset, but it's better to have that in common code than growing warts at all ends.

Quoted 4 lines

Quoted 3 lines

The least thing which we need now are half baken "abstractions" which just shuffle code around for no value.

Quoted 4 lines

While I agree with several things you say even the half baken abstractions help to bring the problems into the light. What we have now is that all these problems are hidden in SoC specific abstractions which all have their own problems and bugs.

And sadly enough the proposed solution keeps 90% of that crap where it is. So it's not a real improvement, just shuffling code around.

On Thu, Apr 21, 2011 at 11:21:53AM +0200, Thomas Gleixner wrote:
That's why I tried to sort out some common patterns (divider, gates, muxes) and put them into drivers/clk for other people to use it. It's enough to build a whole clock tree out of it (except plls and special stuff). All things you mentioned which are missing in the common clock stuff can be added without touching the i.MX specific parts. You want the framework (once it actually is one) to handle the parents? just move the parent handling out of drivers/clk/clk-[divider|mux|gate].c to drivers/clk/clk.c. Refcounting shall be fixed? do so in drivers/clk/clkdev.c.

I didn't say that the patches I posted shall be moved upstream as-is. I only wanted to show how a clock tree can look like when we sort out common patterns.

That brings in another question and this really needs to be answered
now: Locking

I'm pushing hard on this because I know that retrofitting proper locking schemes is going to be a nightmare.

You already have a lock nesting problem when you do parent propagation. With your code you do the parent propagation in the child callback under the prepare_mutex or the enable_lock.

This is going to end up in lockdep being quite unhappy unless you want tons of different lock classes for this stuff. i.e.

? ? clk_prepare(clk2)
? ? ? ?mutex_lock(clk2->mutex);
? ? ? ? ? clk->prepare(clk);
? ? ? ? ? ? ? ?clk_prepare(clk1)
? ? ? ? ? ? ? ? ? ?mutex_lock(clk1->mutex);

And you _CANNOT_ call clk1->prepare() from your clk2 prepare function as it would forgo the serialization and the prepare refcounting. And this kind of scheme is going to be even more problematic if you do bottom up propagation because you will run into lock inversion.

I seriously doubt that the fine grained per clock locking is going to work out at all.

How do you protect changes to the tree hierarchy against a traversal of a concurrent clk->prepare... propagation? Not at all, because you CANNOT. And you cannot use RCU for this either.

So what's the point of the per clk locks? I can't see one at all.

All those callbacks are not high frequency called functions, so if you want to make this a true framework which deals with tons of interesting stuff like propagation, reparenting and rate changes from bottom up then there is only one way to do that: global serialization.

There is no way around that. Everything else is going to be the locking hell and racy all over the place.

Quoted 2 lines · Thomas Gleixner

There is no way around that. Everything else is going to be the locking hell and racy all over the place.

Per clock locks are useful if you have some very slow clocks and some very fast clocks. PLLs may take 20 ms to lock, and preventing any other call to clk_prepare for that long could be problematic. The problem was worse before the clk_enable/clk_prepare split, maybe it's not necessary at all any more.

For Tegra, I solved my clock locking problems by (almost) always locking from child to parent, the normal case for clk_enable/clk_prepare. In general, you only need to lock parent and then child if you store the rate of the child in the child's clk struct and need to update it when the parent changes, or when traversing the tree for debugging output. I solved the first problem by storing the relation between the child and the parent in the child struct and locking from child to parent on clk_get_rate calls to calculate the rate, and the second by using a trylock loop over the list of clocks (see arch/arm/mach-tegra/clock.c).

Now that clk_prepare and clk_enable are split, the long-held locks (for plls, maybe dvfs?) are separated from the short-held locks for clk_enable/clk_disable, and the cost of global serialization is much more acceptable.

On Thu, 21 Apr 2011, Colin Cross wrote:

Yes, and that's exaclty what we don't want to have. You still need a global lock for doing the full tree acquisition and the only advantage which is left is that you can do parallel prepare/unprepare and everything else (parent settings, rate propagations) will need to do the locking dance.

Another thing I'm pondering is to provide the ability to maintain separate clk trees. So you can have individual domains which have their per clk tree locking. Would that make sense ?

Quoted 3 lines · Thomas Gleixner

Another thing I'm pondering is to provide the ability to maintain separate clk trees. So you can have individual domains which have their per clk tree locking. Would that make sense ?

Clock trees are likely to be linked somewhere - an external clock chip that drives the main clock input on the SoC, or an SoC clock output that drives a chip with an internal clock tree. Domains would have to be dynamically managed to ensure two clocks from different clock chips that become linked are moved under the same clock lock. Probably easier to keep a global lock.

Quoted 5 lines · Colin Cross

Quoted 3 lines · Thomas Gleixner

Another thing I'm pondering is to provide the ability to maintain separate clk trees. So you can have individual domains which have their per clk tree locking. Would that make sense ?

Quoted 6 lines

Clock trees are likely to be linked somewhere - an external clock chip that drives the main clock input on the SoC, or an SoC clock output that drives a chip with an internal clock tree. Domains would have to be dynamically managed to ensure two clocks from different clock chips that become linked are moved under the same clock lock. Probably easier to keep a global lock.

Ok. It's easy enough to split later if the need arises.

Quoted 15 lines · Thomas Gleixner

Quoted 12 lines · Colin Cross

Quoted 3 lines · Thomas Gleixner

Another thing I'm pondering is to provide the ability to maintain separate clk trees. So you can have individual domains which have their per clk tree locking. Would that make sense ?

Clock trees are likely to be linked somewhere - an external clock chip that drives the main clock input on the SoC, or an SoC clock output that drives a chip with an internal clock tree. Domains would have to be dynamically managed to ensure two clocks from different clock chips that become linked are moved under the same clock lock. Probably easier to keep a global lock.

Quoted 1 line

Ok. It's easy enough to split later if the need arises.

MSM certainly has the need for having independent clock trees. I would like support for this to be in from the start otherwise it's going to really screw clocks in one of the trees.

Quoted 16 lines · Saravana Kannan

Quoted 15 lines · Thomas Gleixner

Quoted 12 lines · Colin Cross

Quoted 3 lines · Thomas Gleixner

Another thing I'm pondering is to provide the ability to maintain separate clk trees. So you can have individual domains which have their per clk tree locking. Would that make sense ?

Clock trees are likely to be linked somewhere - an external clock chip that drives the main clock input on the SoC, or an SoC clock output that drives a chip with an internal clock tree. Domains would have to be dynamically managed to ensure two clocks from different clock chips that become linked are moved under the same clock lock. Probably easier to keep a global lock.

Quoted 5 lines

Quoted 1 line

Ok. It's easy enough to split later if the need arises.

MSM certainly has the need for having independent clock trees. I would like support for this to be in from the start otherwise it's going to really screw clocks in one of the trees.

Are they completely independent or do you need a global synchronization mechanism between those clock trees as well ?

Thanks,

tglx

They are completely independent. No synchronization is needed between these clock trees. It's basically a bunch of clocks controlled by another processor that has it's own set of PLLs, etc.

Quoted 10 lines · Sascha Hauer

On Thu, Apr 21, 2011 at 11:21:53AM +0200, Thomas Gleixner wrote:
That's why I tried to sort out some common patterns (divider, gates, muxes) and put them into drivers/clk for other people to use it. It's enough to build a whole clock tree out of it (except plls and special stuff). All things you mentioned which are missing in the common clock stuff can be added without touching the i.MX specific parts. You want the framework (once it actually is one) to handle the parents? just move the parent handling out of drivers/clk/clk-[divider|mux|gate].c to drivers/clk/clk.c. Refcounting shall be fixed? do so in drivers/clk/clkdev.c.

The point is that we want to do stuff like that now - BEFORE anyone starts using struct clk and the helper functions.

Quoted 3 lines

I didn't say that the patches I posted shall be moved upstream as-is. I only wanted to show how a clock tree can look like when we sort out common patterns.

We really want to get some sensible functionality for starting.

So what I can see now from the discussion is that we should at least add the following fields to struct clk:

        unsigned long  flags;
        unsigned long   rate;
        struct clk     *parent;
        unsigned int   users;

Add the parent propagation to the clk_enable disable prepare unprepare functions.

And we should add right away:

        struct hlist_head childs;
        struct hlist_node node;

Plus infrastructure to register clocks with the parent. Advanced things like bottom up propagation of clock rate changes is something we can do later, but we really want to make proper registration a required change for users of the new stuff.

That brings in another question and this really needs to be answered
now: Locking

I'm pushing hard on this because I know that retrofitting proper locking schemes is going to be a nightmare.

You already have a lock nesting problem when you do parent propagation. With your code you do the parent propagation in the child callback under the prepare_mutex or the enable_lock.

This is going to end up in lockdep being quite unhappy unless you want tons of different lock classes for this stuff. i.e.

      clk_prepare(clk2)
	mutex_lock(clk2->mutex);
	   clk->prepare(clk);
		clk_prepare(clk1)
		    mutex_lock(clk1->mutex);	

And you _CANNOT_ call clk1->prepare() from your clk2 prepare function as it would forgo the serialization and the prepare refcounting. And this kind of scheme is going to be even more problematic if you do bottom up propagation because you will run into lock inversion.

I seriously doubt that the fine grained per clock locking is going to work out at all.

How do you protect changes to the tree hierarchy against a traversal of a concurrent clk->prepare... propagation? Not at all, because you CANNOT. And you cannot use RCU for this either.

So what's the point of the per clk locks? I can't see one at all.

All those callbacks are not high frequency called functions, so if you want to make this a true framework which deals with tons of interesting stuff like propagation, reparenting and rate changes from bottom up then there is only one way to do that: global serialization.

There is no way around that. Everything else is going to be the locking hell and racy all over the place.

Quoted 2 lines · Thomas Gleixner

I seriously doubt that the fine grained per clock locking is going to work out at all.

One thing we did to handle the locking issue was to have a lock per "class" of clocks. For example, we had one lock for clocks controlled by writing to registers, one lock for voting clocks that just do aggregation and one lock for these extremely slow clocks. Since each class of clocks are generally implemented in their own file, having one lock per class was just a matter of declaring a static-global lock inside the file and grabbing it before you do any transaction.

As Colin said in his email, a global lock is less of an issue with the clk_prepare/unprepare() APIs now in place. But even among prepare/unprepare or enable/disable, some clocks might take much longer than others. For example, prepare for a clock might just need enabling a PLL, but for another clock it might need enabling a PLL and increasing some voltage. If the voltage control is going to go thru I2C, then it's going to be pretty bad for the other clock doing the prepare.

I think if we have the parent/child relationship captured in the generic framework, we might be able to get away with per clock locking if we always make sure we grab the locks in a top-down or bottom-up fashion. We could even

throw in some flags if we know that a clock will have a stable parent (one that doesn't change rates or need to lock children) to reduce how deep we have to traverse the tree.

Quoted 1 line · Uwe Kleine-K?nig

struct clk has no parent member, there is only clk_get_parent(). If

Which is a complete failure to begin with. Why the heck do you need a callback to figure that out?

Quoted 5 lines · Thomas Gleixner

Quoted 1 line · Uwe Kleine-K?nig

struct clk has no parent member, there is only clk_get_parent(). If

Which is a complete failure to begin with. Why the heck do you need a callback to figure that out?

Thomas, you're wrong. You're looking at things far too simply, because all you can see is a tree of clocks.

When you have MUXes, a clock can have one of many parents. So a single clk->parent doesn't hack it. That's one reason why clk_get_parent() exists, so that MUXes can return the appropriate parent for the current MUX setting.

You may wish to store the current setting in clk->parent, but I'd argue that's a recipe for things going wrong when the MUX settings differ from the cached clk->parent value. Instead, having clk_get_parent()s implementation return the _current_ parent and let MUXes do the right thing depending on hardware is much easier and more reliably correct.

Quoted 9 lines · Russell King - ARM Linux

Quoted 5 lines · Thomas Gleixner

Quoted 1 line · Uwe Kleine-K?nig

struct clk has no parent member, there is only clk_get_parent(). If

Which is a complete failure to begin with. Why the heck do you need a callback to figure that out?

Thomas, you're wrong. You're looking at things far too simply, because all you can see is a tree of clocks.

And that's what it is. Even if the clock can have sevaral possible parents, then there is only one active parent at a time. It's still a tree.

Quoted 4 lines

When you have MUXes, a clock can have one of many parents. So a single clk->parent doesn't hack it. That's one reason why clk_get_parent() exists, so that MUXes can return the appropriate parent for the current MUX setting.

That's only ever needed when you register a clk with the framework to decode the current setting from the hardware.

Quoted 5 lines

You may wish to store the current setting in clk->parent, but I'd argue that's a recipe for things going wrong when the MUX settings differ from the cached clk->parent value. Instead, having clk_get_parent()s implementation return the _current_ parent and let MUXes do the right thing depending on hardware is much easier and more reliably correct.

If something changes the clk parent in the hardware without going through common framework functions, which take care of updating clk->parent in the proper way, then the implementer got it wrong badly. That's the same thing as if you change the clock rate of your timekeeping clock behind the timekeeping core and then complain about timekeeping going awry.

Signed-off-by Uwe Kleine-K?nig <[email protected]>

---
Hello,

only compile tested so far.

Best regards
Uwe

 drivers/clk/clk.c   |   43 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk.h |    8 ++++++++
 2 files changed, 51 insertions(+), 0 deletions(-)

drivers/clk/clk.c +2 -0

index 264c809..7627815 100644
@@ -14,10 +14,23 @@
 int clk_prepare(struct clk *clk)
 {
 	int ret = 0;
+	struct clk *parent = ERR_PTR(-ENOSYS);
 
 	if (!clk)
 		return 0;
 
+	if (clk->ops->flags & CLK_OPS_GENERIC_PARENT) {

drivers/clk/clk.c +31 -0

+
 	mutex_lock(&clk->prepare_lock);
 	if (clk->prepare_count == 0 && clk->ops->prepare)
 		ret = clk->ops->prepare(clk);

@@ -26,6 +39,9 @@ int clk_prepare(struct clk *clk)
 		clk->prepare_count++;
 	mutex_unlock(&clk->prepare_lock);
 
+	if (ret && !IS_ERR(parent))
+		clk_unprepare(parent);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(clk_prepare);

@@ -45,6 +61,12 @@ void clk_unprepare(struct clk *clk)
 	}
 
 	mutex_unlock(&clk->prepare_lock);
+
+	if (clk->ops->flags & CLK_OPS_GENERIC_PARENT) {
+		struct clk *parent = clk_get_parent(clk);
+		if (!IS_ERR(parent))
+			clk_unprepare(parent);
+	}
 }
 EXPORT_SYMBOL_GPL(clk_unprepare);
 

@@ -52,10 +74,22 @@ int clk_enable(struct clk *clk)
 {
 	unsigned long flags;
 	int ret = 0;
+	struct clk *parent = ERR_PTR(-ENOSYS);
 
 	if (!clk)
 		return 0;
 
+	if (clk->ops->flags & CLK_OPS_GENERIC_PARENT) {
+		parent = clk_get_parent(clk);
+		if (!IS_ERR(parent)) {
+			ret = clk_enable(parent);
+			if (ret)
+				return ret;
+		} else if (PTR_ERR(parent) != -ENOSYS)
+			/* -ENOSYS means no parent and is OK */
+			return PTR_ERR(parent);
+	}
+
 	WARN_ON(clk->prepare_count == 0);
 
 	spin_lock_irqsave(&clk->enable_lock, flags);

@@ -66,6 +100,9 @@ int clk_enable(struct clk *clk)
 		clk->enable_count++;
 	spin_unlock_irqrestore(&clk->enable_lock, flags);
 
+	if (ret && !IS_ERR(parent))
+		clk_disable(parent);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(clk_enable);

@@ -85,6 +122,12 @@ void clk_disable(struct clk *clk)
 		clk->ops->disable(clk);
 
 	spin_unlock_irqrestore(&clk->enable_lock, flags);
+
+	if (clk->ops->flags & CLK_OPS_GENERIC_PARENT) {
+		struct clk *parent = clk_get_parent(clk);
+		if (!IS_ERR(parent))
+			clk_disable(parent);
+	}
 }
 EXPORT_SYMBOL_GPL(clk_disable);
 

include/linux/clk.h +8 -0

index d2f0db0..125e525 100644
@@ -62,6 +62,11 @@ struct clk {
 	.prepare_lock	= __MUTEX_INITIALIZER(name.prepare_lock),	\
 }
 
+/* bit definitions for struct clk_ops.flags */
+#define CLK_OPS_GENERIC_PARENT	1	/* automatically handle parent in
+					   enable, disable, prepare and
+					   unprepare functions */
+
 /**
  * struct clk_ops -  Callback operations for clocks; these are to be provided
  * by the clock implementation, and will be called by drivers through the clk_*

@@ -91,6 +96,8 @@ struct clk {
  * @put:	Called by the core clock code when a devices driver releases a
  *		clock via clk_put(). Optional.
  *
+ * @flags:	see above for possible values.
+ *
  * The clk_enable/clk_disable and clk_prepare/clk_unprepare pairs allow
  * implementations to split any work between atomic (enable) and sleepable
  * (prepare) contexts.  If a clock requires sleeping code to be turned on, this

@@ -119,6 +126,7 @@ struct clk_ops {
 	int		(*set_rate)(struct clk *, unsigned long);
 	int		(*set_parent)(struct clk *, struct clk *);
 	struct clk *	(*get_parent)(struct clk *);
+	unsigned int	flags;
 };
 
 /**

Signature · 7 lines

1.7.4.1


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