[PATCH v6 2/3] clk: introduce the common clock framework

The common clock framework defines a common struct clk useful across most platforms as well as an implementation of the clk api that drivers can use safely for managing clocks.

The net result is consolidation of many different struct clk definitions and platform-specific clock framework implementations.

This patch introduces the common struct clk, struct clk_ops and an implementation of the well-known clock api in include/clk/clk.h. Platforms may define their own hardware-specific clock structure and their own clock operation callbacks, so long as it wraps an instance of struct clk_hw.

See Documentation/clk.txt for more details.

This patch is based on the work of Jeremy Kerr, which in turn was based on the work of Ben Herrenschmidt.

Now the question is whether you should provide a data structure which is explicitely used for static initialization and instead of having struct clk static you register the static initializer structure, which would be initdata. I don't think that anything needs clocks before the memory allocators are up and running. The clocks which are necessary to get that far have to be enabled in the boot loader anyway.

The static initialization question should not hold off this set from being merged, though settling it before growing users would be nice.

Otherwise this is a very well done infrastructure implementation! Thanks a lot Mike!

On Fri, Mar 09, 2012 at 11:54:23PM -0800, Mike Turquette wrote: > The common clock framework defines a common struct clk useful across > most platforms as well as an implementation of the clk api that drivers > can use safely for managing clocks.

You don't need a WARN_ON when you derefence clk->ops->round_rate anyway. Also, even when the current clock does not have a set_rate function it can still change its rate when the CLK_SET_RATE_PARENT is set.

Sorry Richard, that one slipped through the cracks. I'll publish a new version tomorrow with some of those fixes. Some of the others (such as multiple pre-rate change notifiers) I won't take in, but we can discuss more.

On Sun, Mar 11, 2012 at 4:34 AM, Sascha Hauer <s.hauer [at] pengutronix> wrote: > Hi Mike, > > I was about to give my tested-by when I decided to test the set_rate > function. Unfortunately this is broken for several reasons. I'll try > to come up with a fixup series later the day.

I haven't tested clk_set_rate since V4, but I also haven't changed the code appreciably. I'll retest on my end also.

The v6 Documentation/clk.txt states that .round_rate is mandatory for clocks that can adjust their rate, but I need to clarify this a bit more. Ideally we want to be able to call clk_set_rate on any clock and get a changed rate (if possible) by either adjusting that clocks rate direction (e.g. a PLL or an adjustable divider) or by propagating __clk_set_rate up the parents (assuming of course that CLK_SET_RATE_PARENT flag is set appropriately).

> Also, even when the current clock does not have a set_rate function it > can still change its rate when the CLK_SET_RATE_PARENT is set.

Correct. I'll clean this up and make the documentation a bit more verbose on when .set_rate/.round_rate/.recalc_rate are mandatory.

This depends on the .round_rate implementation, which I admit to having lost some sleep over. A clever .round_rate will request the "intermediate" rate for a clock when propagating a request to change the parent rate later on. Take for instance the following:

If we want child to run at 100MHz then the desirable configuration would be to have parent divide-by-1 and child divide-by-2. When we call,

clk_set_rate(child, 100MHz);

Its .round_rate should return 50MHz, and &parent_new_rate should be 200MHz. So 50MHz is an "intermediate" rate, but it gets us the divider we want. And in fact 50MHz reflects reality because that will be the rate of child until the parent propagation completes and we can adjust parent's dividers. (this is one reason why I prefer for pre-rate change notifiers to stack on top of each other).

So now that &parent_new_rate is > 0, __clk_set_rate will propagate the request up and parent's .round_rate will simply return 200MHz and leave it's own &parent_new_rate at 0. This will change from divide-by-2 to divide-by-1 and from this highest point in the tree we will propagate post-rate change notifiers downstream, as part of the recalc_rate tree walk.

I have tested this with OMAP4's CPUfreq driver and I think, while complicated, it is a sound way to approach the problem. Maybe the API can be cleaned up, if you have any suggestions.

On Sun, Mar 11, 2012 at 02:24:46PM -0700, Turquette, Mike wrote: > On Sun, Mar 11, 2012 at 4:34 AM, Sascha Hauer <s.hauer [at] pengutronix> wrote: > > Hi Mike, > > > > I was about to give my tested-by when I decided to test the set_rate > > function. Unfortunately this is broken for several reasons. I'll try > > to come up with a fixup series later the day. > > I haven't tested clk_set_rate since V4, but I also haven't changed the > code appreciably. I'll retest on my end also. > > > On Fri, Mar 09, 2012 at 11:54:23PM -0800, Mike Turquette wrote: > >> + /* find the new rate and see if parent rate should change too */ > >> + WARN_ON(!clk->ops->round_rate); > >> + > >> + new_rate = clk->ops->round_rate(clk->hw, rate, &parent_new_rate); > > > > You don't need a WARN_ON when you derefence clk->ops->round_rate anyway. > > Agreed that the WARN_ON should not be there. > > The v6 Documentation/clk.txt states that .round_rate is mandatory for > clocks that can adjust their rate, but I need to clarify this a bit > more. Ideally we want to be able to call clk_set_rate on any clock > and get a changed rate (if possible) by either adjusting that clocks > rate direction (e.g. a PLL or an adjustable divider) or by propagating > __clk_set_rate up the parents (assuming of course that > CLK_SET_RATE_PARENT flag is set appropriately). > > > Also, even when the current clock does not have a set_rate function it > > can still change its rate when the CLK_SET_RATE_PARENT is set. > > Correct. I'll clean this up and make the documentation a bit more > verbose on when .set_rate/.round_rate/.recalc_rate are mandatory. > > > > >> + > >> + /* NOTE: pre-rate change notifications will stack */ > >> + if (clk->notifier_count) > >> + ret = __clk_notify(clk, PRE_RATE_CHANGE, clk->rate, new_rate); > >> + > >> + if (ret == NOTIFY_BAD) > >> + return clk; > >> + > >> + /* speculate rate changes down the tree */ > >> + hlist_for_each_entry(child, tmp, &clk->children, child_node) { > >> + ret = __clk_speculate_rates(child, new_rate); > >> + if (ret == NOTIFY_BAD) > >> + return clk; > >> + } > >> + > >> + /* change the rate of this clk */ > >> + if (clk->ops->set_rate) > >> + ret = clk->ops->set_rate(clk->hw, new_rate); > > > > I don't know the reason why you change the child clock before the parent > > clock, but it cannot work since this clock will change its rate based on > > the old parent rate and not the new one. > > This depends on the .round_rate implementation, which I admit to > having lost some sleep over. A clever .round_rate will request the > "intermediate" rate for a clock when propagating a request to change > the parent rate later on. Take for instance the following: > > pll @ 200MHz (locked) > | > parent @ 100MHz (can divide by 1 or 2; currently divider is 2) > | > child @ 25MHz (can divide by 2 or 4; currently divider is 4) > > If we want child to run at 100MHz then the desirable configuration > would be to have parent divide-by-1 and child divide-by-2. When we > call, > > clk_set_rate(child, 100MHz); > > Its .round_rate should return 50MHz, and &parent_new_rate should be > 200MHz. So 50MHz is an "intermediate" rate, but it gets us the > divider we want. And in fact 50MHz reflects reality because that will > be the rate of child until the parent propagation completes and we can > adjust parent's dividers. (this is one reason why I prefer for > pre-rate change notifiers to stack on top of each other). > > So now that &parent_new_rate is > 0, __clk_set_rate will propagate the > request up and parent's .round_rate will simply return 200MHz and > leave it's own &parent_new_rate at 0. This will change from > divide-by-2 to divide-by-1 and from this highest point in the tree we > will propagate post-rate change notifiers downstream, as part of the > recalc_rate tree walk. > > I have tested this with OMAP4's CPUfreq driver and I think, while > complicated, it is a sound way to approach the problem. Maybe the API > can be cleaned up, if you have any suggestions.

I cannot see all implications this way will have. All this rate propagation is more complex than I thought it would be. I tried another approach on the weekend which basically does not try to do all in a single recursion but instead sets the rate in multiple steps:

1) call a function which calculates all new rates of affected clocks in a rate change and safes the value in a clk->new_rate field. This function returns the topmost clock which has to be changed. 2) starting from the topmost clock notify all clients. This walks the whole subtree even if a notfifier refuses the change. If necessary we can walk the whole subtree again to abort the change. 3) actually change rates starting from the topmost clocks and notify all clients on the way. I changed the set_rate callback to void. Instead of failing (what is failing in case of set_rate? The clock will still have some rate) I check for the result with clk_ops->recalc_rate.

In the end what's more important than the implementation details is that it actually works. I created a little test module which sets up two cascaded dividers, tries to change the rate at the output and checks the result. We might want to add something like this (and maybe similar tests for reparenting and other stuff) to the generic clock framework later. It's good to have something generic to test the framework with without depending on some particular SoC.

On Mon, Mar 12, 2012 at 4:51 AM, Sascha Hauer <s.hauer [at] pengutronix> wrote: > On Sun, Mar 11, 2012 at 02:24:46PM -0700, Turquette, Mike wrote: >> On Sun, Mar 11, 2012 at 4:34 AM, Sascha Hauer <s.hauer [at] pengutronix> wrote: >> > Hi Mike, >> > >> > I was about to give my tested-by when I decided to test the set_rate >> > function. Unfortunately this is broken for several reasons. I'll try >> > to come up with a fixup series later the day. >> >> I haven't tested clk_set_rate since V4, but I also haven't changed the >> code appreciably. I'll retest on my end also. >> >> > On Fri, Mar 09, 2012 at 11:54:23PM -0800, Mike Turquette wrote: >> >> + /* find the new rate and see if parent rate should change too */ >> >> + WARN_ON(!clk->ops->round_rate); >> >> + >> >> + new_rate = clk->ops->round_rate(clk->hw, rate, &parent_new_rate); >> > >> > You don't need a WARN_ON when you derefence clk->ops->round_rate anyway. >> >> Agreed that the WARN_ON should not be there. >> >> The v6 Documentation/clk.txt states that .round_rate is mandatory for >> clocks that can adjust their rate, but I need to clarify this a bit >> more. Ideally we want to be able to call clk_set_rate on any clock >> and get a changed rate (if possible) by either adjusting that clocks >> rate direction (e.g. a PLL or an adjustable divider) or by propagating >> __clk_set_rate up the parents (assuming of course that >> CLK_SET_RATE_PARENT flag is set appropriately). >> >> > Also, even when the current clock does not have a set_rate function it >> > can still change its rate when the CLK_SET_RATE_PARENT is set. >> >> Correct. I'll clean this up and make the documentation a bit more >> verbose on when .set_rate/.round_rate/.recalc_rate are mandatory. >> >> > >> >> + >> >> + /* NOTE: pre-rate change notifications will stack */ >> >> + if (clk->notifier_count) >> >> + ret = __clk_notify(clk, PRE_RATE_CHANGE, clk->rate, new_rate); >> >> + >> >> + if (ret == NOTIFY_BAD) >> >> + return clk; >> >> + >> >> + /* speculate rate changes down the tree */ >> >> + hlist_for_each_entry(child, tmp, &clk->children, child_node) { >> >> + ret = __clk_speculate_rates(child, new_rate); >> >> + if (ret == NOTIFY_BAD) >> >> + return clk; >> >> + } >> >> + >> >> + /* change the rate of this clk */ >> >> + if (clk->ops->set_rate) >> >> + ret = clk->ops->set_rate(clk->hw, new_rate); >> > >> > I don't know the reason why you change the child clock before the parent >> > clock, but it cannot work since this clock will change its rate based on >> > the old parent rate and not the new one. >> >> This depends on the .round_rate implementation, which I admit to >> having lost some sleep over. A clever .round_rate will request the >> "intermediate" rate for a clock when propagating a request to change >> the parent rate later on. Take for instance the following: >> >> pll @ 200MHz (locked) >> | >> parent @ 100MHz (can divide by 1 or 2; currently divider is 2) >> | >> child @ 25MHz (can divide by 2 or 4; currently divider is 4) >> >> If we want child to run at 100MHz then the desirable configuration >> would be to have parent divide-by-1 and child divide-by-2. When we >> call, >> >> clk_set_rate(child, 100MHz); >> >> Its .round_rate should return 50MHz, and &parent_new_rate should be >> 200MHz. So 50MHz is an "intermediate" rate, but it gets us the >> divider we want. And in fact 50MHz reflects reality because that will >> be the rate of child until the parent propagation completes and we can >> adjust parent's dividers. (this is one reason why I prefer for >> pre-rate change notifiers to stack on top of each other). >> >> So now that &parent_new_rate is > 0, __clk_set_rate will propagate the >> request up and parent's .round_rate will simply return 200MHz and >> leave it's own &parent_new_rate at 0. This will change from >> divide-by-2 to divide-by-1 and from this highest point in the tree we >> will propagate post-rate change notifiers downstream, as part of the >> recalc_rate tree walk. >> >> I have tested this with OMAP4's CPUfreq driver and I think, while >> complicated, it is a sound way to approach the problem. Maybe the API >> can be cleaned up, if you have any suggestions. > > I cannot see all implications this way will have. All this rate > propagation is more complex than I thought it would be.

Hi Sascha,

Yes it is very complicated. The solution I have now (recursive __clk_set_rate, clever .round_rate which requests parent rate) was not something I arrived at immediately.

I decided to validate the v6 patches more thoroughly today, based on your claim that clk_set_rate is broken and here is what I found:

1) clk_set_rate works. I pulled in the latest OMAP4 CPUfreq code into my common clk branch and it Just Worked. This is a dumb implementation involving no upwards parent propagation, and the clock changing is of type struct clk_hw_omap (relocking a PLL)

2) while I was at it I verified the rate change notifiers + clk_set_parent, which also work (I had not touched these since v4 and wanted to make sure nothing was broken)

Here is where things get interesting. I tried the same parent rate propagation via CPUfreq that I had done previously in the v4 series (http://article.gmane.org/gmane.linux.ports.arm.omap/68225), but this time it didn't work. The difference is that back in v4 all of my clocks in that propagation chain were struct clk_hw_omap, none of them were any of the basic clock types. Now in v6, one of the clocks in the chain is struct clk_divider. I started looking at the divider's .round_rate code and for the case where CLK_PARENT_SET_RATE flag is set the code doesn't make any sense. I spent some time today trying to fix struct clk_divider's .round_rate implementation and came to realize that there might not be a sane default for how such code should work. A sane default for the common divider that works correctly on OMAP may not be what you want on iMX.

To illustrate: if CLK_SET_RATE_PARENT flag is set for my clk_divider I'd really like to divide by 1 and pass the exact same rate that was requested for my clk_divider up to the parent. But the existing divider code tries to find the largest div for clk_divider and request a faster rate from the parent (well, I assume this is what it is supposed to do as the code doesn't quite get this right).

Do you have any ideas on this? Also, can you verify if this is what was failing for you, or maybe provide a log if the bug you mentioned exists elsewhere? An extreme solution to the problem would be for clk_divider to not support the CLK_SET_PARENT flag if we cannot agree on a sane default for this behavior. I like the idea of the divider choosing the smallest div possible and requesting the balance from the parent, but I need feedback from others on that point. For non-rate adjustable clocks (clk_gate in particular) it is very easy to continue supporting this flag as the .round_rate implementation should just be a "pass through" up to the parent. In fact this behavior is what some of my omap clocks are doing and that is why parent propagation for CPUfreq was working for me back in v4.

> I tried another > approach on the weekend which basically does not try to do all in a > single recursion but instead sets the rate in multiple steps: > > 1) call a function which calculates all new rates of affected clocks > in a rate change and safes the value in a clk->new_rate field. This > function returns the topmost clock which has to be changed. > 2) starting from the topmost clock notify all clients. This walks the > whole subtree even if a notfifier refuses the change. If necessary > we can walk the whole subtree again to abort the change. > 3) actually change rates starting from the topmost clocks and notify > all clients on the way. I changed the set_rate callback to void. > Instead of failing (what is failing in case of set_rate? The clock > will still have some rate) I check for the result with > clk_ops->recalc_rate.

I've considered something like this for "clock groups", especially when the set of clocks changing are all on the same device and the permutations are well known ahead of time. Is this working for you now?

> > In the end what's more important than the implementation details is that > it actually works. I created a little test module which sets up two > cascaded dividers, tries to change the rate at the output and checks the > result. We might want to add something like this (and maybe similar > tests for reparenting and other stuff) to the generic clock framework > later. It's good to have something generic to test the framework with > without depending on some particular SoC.

Agreed about the generic test. I'm not taking it into the current series, but we can grow the test cases and submit them later on.

On Mon, Mar 12, 2012 at 08:16:36PM -0700, Turquette, Mike wrote: > On Mon, Mar 12, 2012 at 4:51 AM, Sascha Hauer <s.hauer [at] pengutronix> wrote: > > On Sun, Mar 11, 2012 at 02:24:46PM -0700, Turquette, Mike wrote: > >> On Sun, Mar 11, 2012 at 4:34 AM, Sascha Hauer <s.hauer [at] pengutronix> wrote: > >> > Hi Mike, > >> > > >> > I was about to give my tested-by when I decided to test the set_rate > >> > function. Unfortunately this is broken for several reasons. I'll try > >> > to come up with a fixup series later the day. > >> > >> I haven't tested clk_set_rate since V4, but I also haven't changed the > >> code appreciably. I'll retest on my end also. > >> > >> > On Fri, Mar 09, 2012 at 11:54:23PM -0800, Mike Turquette wrote: > >> >> + /* find the new rate and see if parent rate should change too */ > >> >> + WARN_ON(!clk->ops->round_rate); > >> >> + > >> >> + new_rate = clk->ops->round_rate(clk->hw, rate, &parent_new_rate); > >> > > >> > You don't need a WARN_ON when you derefence clk->ops->round_rate anyway. > >> > >> Agreed that the WARN_ON should not be there. > >> > >> The v6 Documentation/clk.txt states that .round_rate is mandatory for > >> clocks that can adjust their rate, but I need to clarify this a bit > >> more. Ideally we want to be able to call clk_set_rate on any clock > >> and get a changed rate (if possible) by either adjusting that clocks > >> rate direction (e.g. a PLL or an adjustable divider) or by propagating > >> __clk_set_rate up the parents (assuming of course that > >> CLK_SET_RATE_PARENT flag is set appropriately). > >> > >> > Also, even when the current clock does not have a set_rate function it > >> > can still change its rate when the CLK_SET_RATE_PARENT is set. > >> > >> Correct. I'll clean this up and make the documentation a bit more > >> verbose on when .set_rate/.round_rate/.recalc_rate are mandatory. > >> > >> > > >> >> + > >> >> + /* NOTE: pre-rate change notifications will stack */ > >> >> + if (clk->notifier_count) > >> >> + ret = __clk_notify(clk, PRE_RATE_CHANGE, clk->rate, new_rate); > >> >> + > >> >> + if (ret == NOTIFY_BAD) > >> >> + return clk; > >> >> + > >> >> + /* speculate rate changes down the tree */ > >> >> + hlist_for_each_entry(child, tmp, &clk->children, child_node) { > >> >> + ret = __clk_speculate_rates(child, new_rate); > >> >> + if (ret == NOTIFY_BAD) > >> >> + return clk; > >> >> + } > >> >> + > >> >> + /* change the rate of this clk */ > >> >> + if (clk->ops->set_rate) > >> >> + ret = clk->ops->set_rate(clk->hw, new_rate); > >> > > >> > I don't know the reason why you change the child clock before the parent > >> > clock, but it cannot work since this clock will change its rate based on > >> > the old parent rate and not the new one. > >> > >> This depends on the .round_rate implementation, which I admit to > >> having lost some sleep over. A clever .round_rate will request the > >> "intermediate" rate for a clock when propagating a request to change > >> the parent rate later on. Take for instance the following: > >> > >> pll @ 200MHz (locked) > >> | > >> parent @ 100MHz (can divide by 1 or 2; currently divider is 2) > >> | > >> child @ 25MHz (can divide by 2 or 4; currently divider is 4) > >> > >> If we want child to run at 100MHz then the desirable configuration > >> would be to have parent divide-by-1 and child divide-by-2. When we > >> call, > >> > >> clk_set_rate(child, 100MHz); > >> > >> Its .round_rate should return 50MHz, and &parent_new_rate should be > >> 200MHz. So 50MHz is an "intermediate" rate, but it gets us the > >> divider we want. And in fact 50MHz reflects reality because that will > >> be the rate of child until the parent propagation completes and we can > >> adjust parent's dividers. (this is one reason why I prefer for > >> pre-rate change notifiers to stack on top of each other). > >> > >> So now that &parent_new_rate is > 0, __clk_set_rate will propagate the > >> request up and parent's .round_rate will simply return 200MHz and > >> leave it's own &parent_new_rate at 0. This will change from > >> divide-by-2 to divide-by-1 and from this highest point in the tree we > >> will propagate post-rate change notifiers downstream, as part of the > >> recalc_rate tree walk. > >> > >> I have tested this with OMAP4's CPUfreq driver and I think, while > >> complicated, it is a sound way to approach the problem. Maybe the API > >> can be cleaned up, if you have any suggestions. > > > > I cannot see all implications this way will have. All this rate > > propagation is more complex than I thought it would be. > > Hi Sascha, > > Yes it is very complicated. The solution I have now (recursive > __clk_set_rate, clever .round_rate which requests parent rate) was not > something I arrived at immediately. > > I decided to validate the v6 patches more thoroughly today, based on > your claim that clk_set_rate is broken and here is what I found: > > 1) clk_set_rate works. I pulled in the latest OMAP4 CPUfreq code into > my common clk branch and it Just Worked. This is a dumb > implementation involving no upwards parent propagation, and the clock > changing is of type struct clk_hw_omap (relocking a PLL) > > 2) while I was at it I verified the rate change notifiers + > clk_set_parent, which also work (I had not touched these since v4 and > wanted to make sure nothing was broken) > > Here is where things get interesting. I tried the same parent rate > propagation via CPUfreq that I had done previously in the v4 series > (http://article.gmane.org/gmane.linux.ports.arm.omap/68225), but this > time it didn't work. The difference is that back in v4 all of my > clocks in that propagation chain were struct clk_hw_omap, none of them > were any of the basic clock types. Now in v6, one of the clocks in > the chain is struct clk_divider. I started looking at the divider's > .round_rate code and for the case where CLK_PARENT_SET_RATE flag is > set the code doesn't make any sense. I spent some time today trying > to fix struct clk_divider's .round_rate implementation and came to > realize that there might not be a sane default for how such code > should work. A sane default for the common divider that works > correctly on OMAP may not be what you want on iMX. > > To illustrate: if CLK_SET_RATE_PARENT flag is set for my clk_divider > I'd really like to divide by 1 and pass the exact same rate that was > requested for my clk_divider up to the parent.

I don't understand this. If you use the CLK_SET_RATE_PARENT to force a divider value to 1 then why do you register a divider at all?

> But the existing > divider code tries to find the largest div for clk_divider and request > a faster rate from the parent (well, I assume this is what it is > supposed to do as the code doesn't quite get this right).

That's indeed different assumptions. When I set CLK_SET_RATE_PARENT I want to say to the divider that it is allowed to change the parent rate. So what it does (or should do) is to loop around its divider values and tries to round the parent rate to (rate * div). This way it can find the best divider values for this divider and the parent divider to get the wanted rate. This was done (almost) correctly in the version of the divider I posted to the list, but indeed the current version is broken.

> > Do you have any ideas on this? Also, can you verify if this is what > was failing for you, or maybe provide a log if the bug you mentioned > exists elsewhere? An extreme solution to the problem would be for > clk_divider to not support the CLK_SET_PARENT flag if we cannot agree > on a sane default for this behavior. I like the idea of the divider > choosing the smallest div possible and requesting the balance from the > parent, but I need feedback from others on that point.

As said, the divider tries to get the *best* value for itself and the parent divider (if exists), not the smallest. I think the current divider implements something between what I thought it should should do and what you thought it should do.

> For non-rate > adjustable clocks (clk_gate in particular) it is very easy to continue > supporting this flag as the .round_rate implementation should just be > a "pass through" up to the parent. In fact this behavior is what some > of my omap clocks are doing and that is why parent propagation for > CPUfreq was working for me back in v4. > > > I tried another > > approach on the weekend which basically does not try to do all in a > > single recursion but instead sets the rate in multiple steps: > > > > 1) call a function which calculates all new rates of affected clocks > > in a rate change and safes the value in a clk->new_rate field. This > > function returns the topmost clock which has to be changed. > > 2) starting from the topmost clock notify all clients. This walks the > > whole subtree even if a notfifier refuses the change. If necessary > > we can walk the whole subtree again to abort the change. > > 3) actually change rates starting from the topmost clocks and notify > > all clients on the way. I changed the set_rate callback to void. > > Instead of failing (what is failing in case of set_rate? The clock > > will still have some rate) I check for the result with > > clk_ops->recalc_rate. > > I've considered something like this for "clock groups", especially > when the set of clocks changing are all on the same device and the > permutations are well known ahead of time. Is this working for you > now?

The way described above works for me now, see this branch:

git://git.pengutronix.de/git/imx/linux-2.6.git v3.3-rc6-clkv6-fixup

You may not necessarily like it as it changes quite a lot in the rate changing code.

For the clock groups I plan to drop them at least for i.MX as the concept of clk groups seems somewhat broken. What we are grouping together are really different clocks and the driver should know about it and could even make use of it. We may not want to have more clocks in the driver though and may need something handling this on the bus layer, but that's another topic. The clock framework seems to be the wrong place for it.

> > > > > In the end what's more important than the implementation details is that > > it actually works. I created a little test module which sets up two > > cascaded dividers, tries to change the rate at the output and checks the > > result. We might want to add something like this (and maybe similar > > tests for reparenting and other stuff) to the generic clock framework > > later. It's good to have something generic to test the framework with > > without depending on some particular SoC. > > Agreed about the generic test. I'm not taking it into the current > series, but we can grow the test cases and submit them later on.

Have you tried running it? It's not i.MX specific. I attached an updated version of the test. It fails with several NULL pointer exceptions, mostly because it has the CLK_SET_RATE_PARENT flag set for a divider whose parent is a fixed clock. If I don't set this flag no more NULL pointer exceptions occur, but the rate is not correct because the divider has to use DIV_ROUND_UP in its calc_best_div function. Otherwise it always returns a higher frequency if parentrate/rate is not an integer.

Apart from these little details we are discussing about here I really like your patches and can't wait to see them mainline. Thanks for working on this.

On Tue, Mar 13, 2012 at 5:05 AM, Sascha Hauer <s.hauer [at] pengutronix> wrote: > On Mon, Mar 12, 2012 at 08:16:36PM -0700, Turquette, Mike wrote: >> On Mon, Mar 12, 2012 at 4:51 AM, Sascha Hauer <s.hauer [at] pengutronix> wrote: >> > I tried another >> > approach on the weekend which basically does not try to do all in a >> > single recursion but instead sets the rate in multiple steps: >> > >> > 1) call a function which calculates all new rates of affected clocks >> > in a rate change and safes the value in a clk->new_rate field. This >> > function returns the topmost clock which has to be changed. >> > 2) starting from the topmost clock notify all clients. This walks the >> > whole subtree even if a notfifier refuses the change. If necessary >> > we can walk the whole subtree again to abort the change. >> > 3) actually change rates starting from the topmost clocks and notify >> > all clients on the way. I changed the set_rate callback to void. >> > Instead of failing (what is failing in case of set_rate? The clock >> > will still have some rate) I check for the result with >> > clk_ops->recalc_rate. > > The way described above works for me now, see this branch: > > git://git.pengutronix.de/git/imx/linux-2.6.git v3.3-rc6-clkv6-fixup > > You may not necessarily like it as it changes quite a lot in the rate > changing code.

I tried that code and I really like it! It is much more readable and feels less "fragile" than the previous recursive __clk_set_rate. I did quite a bit of testing with this code today. One of the tests looks like this:

The new code did a fine job arbitrating rates for the PLL and the intermediate divider even if I put weird constraints on the PLL. For instance if I artificially limited it to a minimum of 600MHz and then ran clk_set_rate(dummy, 300MHz) it would lock at 600MHz and set clk_divider to divide-by-2. Setting to 600MHz or more set the divider back to 1 and relocked the PLL appropriately. Pretty cool.

I also tested the notifiers with this code and they seem to function properly. I'll take this code in for v7. Thanks a lot for this helpful contribution.

I did find that MULT_ROUND_UP caused trouble for my PLL's round_rate implementation. Maybe my PLL code is fragile but a quick fix was to make sure that we send the exact value we want to the round_rate code. I also feel this is more correct. Let me know what you think:

On Wed, Mar 14, 2012 at 05:51:48PM -0700, Turquette, Mike wrote: > On Tue, Mar 13, 2012 at 5:05 AM, Sascha Hauer <s.hauer [at] pengutronix> wrote: > > On Mon, Mar 12, 2012 at 08:16:36PM -0700, Turquette, Mike wrote: > >> On Mon, Mar 12, 2012 at 4:51 AM, Sascha Hauer <s.hauer [at] pengutronix> wrote: > >> > I tried another > >> > approach on the weekend which basically does not try to do all in a > >> > single recursion but instead sets the rate in multiple steps: > >> > > >> > 1) call a function which calculates all new rates of affected clocks > >> > in a rate change and safes the value in a clk->new_rate field. This > >> > function returns the topmost clock which has to be changed. > >> > 2) starting from the topmost clock notify all clients. This walks the > >> > whole subtree even if a notfifier refuses the change. If necessary > >> > we can walk the whole subtree again to abort the change. > >> > 3) actually change rates starting from the topmost clocks and notify > >> > all clients on the way. I changed the set_rate callback to void. > >> > Instead of failing (what is failing in case of set_rate? The clock > >> > will still have some rate) I check for the result with > >> > clk_ops->recalc_rate. > > > > The way described above works for me now, see this branch: > > > > git://git.pengutronix.de/git/imx/linux-2.6.git v3.3-rc6-clkv6-fixup > > > > You may not necessarily like it as it changes quite a lot in the rate > > changing code. > > I tried that code and I really like it! It is much more readable and > feels less "fragile" than the previous recursive __clk_set_rate. I > did quite a bit of testing with this code today. One of the tests > looks like this: > > pll (adjustable to anything) > | > clk_divider (5 bits wide) > | > dummy (no clk_ops) > > The new code did a fine job arbitrating rates for the PLL and the > intermediate divider even if I put weird constraints on the PLL. For > instance if I artificially limited it to a minimum of 600MHz and then > ran clk_set_rate(dummy, 300MHz) it would lock at 600MHz and set > clk_divider to divide-by-2. Setting to 600MHz or more set the divider > back to 1 and relocked the PLL appropriately. Pretty cool. > > I also tested the notifiers with this code and they seem to function > properly. I'll take this code in for v7. Thanks a lot for this > helpful contribution. > > I did find that MULT_ROUND_UP caused trouble for my PLL's round_rate > implementation. Maybe my PLL code is fragile but a quick fix was to > make sure that we send the exact value we want to the round_rate code. > I also feel this is more correct. Let me know what you think: > > 8<--------------------------------------------------------------- > > commit 189fecedb175d0366759246c4192f45b0bc39a50 > Author: Mike Turquette <mturquette [at] linaro> > Date: Wed Mar 14 17:29:51 2012 -0700 > > clk-divider.c: round the actual rate we care about > > diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c > index 86ca9cd..06ef4a0 100644 > --- a/drivers/clk/clk-divider.c > +++ b/drivers/clk/clk-divider.c > @@ -47,12 +47,6 @@ static unsigned long clk_divider_recalc_rate(struct > clk_hw *hw, > } > EXPORT_SYMBOL_GPL(clk_divider_recalc_rate); > > -/* > - * The reverse of DIV_ROUND_UP: The maximum number which > - * divided by m is r > - */ > -#define MULT_ROUND_UP(r, m) ((r) * (m) + (m) - 1) > - > static int clk_divider_bestdiv(struct clk_hw *hw, unsigned long rate, > unsigned long *best_parent_rate) > { > @@ -84,9 +78,9 @@ static int clk_divider_bestdiv(struct clk_hw *hw, > unsigned long rate, > > for (i = 1; i <= maxdiv; i++) { > parent_rate = __clk_round_rate(__clk_get_parent(hw->clk), > - MULT_ROUND_UP(rate, i)); > + (rate * i));

I think MULT_ROUND_UP is the right thing to use here (not sure if this is a good name though) Consider we want to have an output rate of 33Hz. Now acceptable input rates for a divider value of 3 would be 99, 100 and 101Hz, so we have to call round_rate for the parent with 101Hz which includes 100 and 99Hz.

If you have problems with your PLL than most likely because it does something different on clk_round_rate than it does in clk_set_rate, for example clk_round_rate(10000) returns 10000, but clk_set_rate then sets the rate 9999 due to some rounding error. Being consistent between round_rate and set_rate is very important for this mechanism to work properly. It did cost me some nerves to get it right for the divider (and even more nerves to figure out why it is correct the way it works)

On Thu, Mar 15, 2012 at 2:43 AM, Sascha Hauer <s.hauer [at] pengutronix> wrote: > On Wed, Mar 14, 2012 at 05:51:48PM -0700, Turquette, Mike wrote: >> @@ -84,9 +78,9 @@ static int clk_divider_bestdiv(struct clk_hw *hw, >> unsigned long rate, >> >> for (i = 1; i <= maxdiv; i++) { >> parent_rate = __clk_round_rate(__clk_get_parent(hw->clk), >> - MULT_ROUND_UP(rate, i)); >> + (rate * i)); > > I think MULT_ROUND_UP is the right thing to use here (not sure if this > is a good name though) > Consider we want to have an output rate of 33Hz. Now acceptable input > rates for a divider value of 3 would be 99, 100 and 101Hz, so we have > to call round_rate for the parent with 101Hz which includes 100 and > 99Hz.

We're back to the point Rob brought up about .round_rate rounding up or down. We really need a least-upper-bounds or greatest-lower-bounds flag, similar to how CPUfreq selects target frequencies today. I'm freezing features for this patchset now, so I'll keep your MULT_ROUND_UP approach and some day I'll fix .round_rate for good.

> If you have problems with your PLL than most likely because it does > something different on clk_round_rate than it does in clk_set_rate, > for example clk_round_rate(10000) returns 10000, but clk_set_rate then > sets the rate 9999 due to some rounding error. Being consistent between > round_rate and set_rate is very important for this mechanism to work > properly. It did cost me some nerves to get it right for the divider > (and even more nerves to figure out why it is correct the way it works)

What I'd like to do is request the *exact* frequency I want when passing in a rate to my PLLs .round_rate. Due to the way that my MN dividers are calculated, and due to some jitter avoidance code, it is bad for me to request 600000001 Hz when I really want 600MHz exactly. Anyways I fixed this up on my end enough to work with MULT_ROUND_UP, so that can stay for the immediate future.

MULT_ROUND_UP yields multiples of 'rate' plus an incrementing value (m - 1). Without that incrementing value added to the rate passed into __clk_round_rate the for loop above will always max out the divider. To illustrate:

The rate we want is 300MHz. Without MULT_ROUND_UP the for loop will start yielding these combinations:

These all yield the desired 300MHz for our divider so it just keeps going until you max out the divider and requests some crazy rate for the parent. On most hardware that I am aware of it is desirable to keep the divider as low as possible so the change to the conditional prevents us from overwriting best every single time while keeping the divider low. So yes, it is an optimization, but it is also quite necessary without MULT_ROUND_UP.

Anyways I've kept MULT_ROUND_UP exactly as-is with the exception that I've added that small optimization to keep dividers low. I hope there are no objections to that.