| Message ID | 1404744702-32010-2-git-send-email-thomas.petazzoni@free-electrons.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 07/07/14 07:51, Thomas Petazzoni wrote: > The current clk_set_rate() logic allows notifiers to be notified of > three different events: > > * PRE_RATE_CHANGE: before the clock driver ->set_rate() function is > called. > * ABORT_RATE_CHANGE: called if the rate change failed > * POST_RATE_CHANGE: called after the rate change has been > successfully done, but after ->recalc_rate() has been called, and > only if the rate of the clock has actually changed. > > This commit adds an additional APPLY_RATE_CHANGE, which we require on > Armada XP to implement dynamic frequency scaling of the CPU > clocks. The problem is as follows. > > On Armada XP, there are three hardware blocks involved in the dynamic > frequency scaling of the CPU clocks: > > - The CPU clocks hardware block itself, whose registers are already > "managed" by drivers/clk/mvebu/clk-cpu.c (compatible string: > marvell,armada-xp-cpu-clock). The driver currently only supports > changing the rate of the CPU clock when the clock is off (i.e > before we boot the secondary CPUs). > > - The PMU DFS registers, which are used to configure the target > frequency for a dynamic rate change. Those registers are relatively > well isolated from other PMU registers, so they can also be > "managed" by the drivers/clk/mvebu/clk-cpu.c. > > - The PMSU registers, which are used to actually trigger the dynamic > frequency change procedure, which consists in programming a bunch > of PMSU registers, entering the idle state on the CPU we want to > change the frequency, and then again reprogram a bunch of PMSU > registers. > > The procedure to change the frequency is: > > 1/ Reset some clocks using the CPU clocks hardware block. > 2/ Define the target frequency using the PMU DFS registers. > 3/ Do the actual frequency change using the PMSU registers. > > However, the PMSU registers are already "managed" by a driver in > arch/arm/mach-mvebu/pmsu.c, and the code there is needed for a wide > variety of power management activities: booting of secondary CPUs, CPU > hotplug, cpuidle, cpufreq, and soon suspend/resume. The same registers > in the PMSU are used for several of those activities. > > So, what we need to do is to have steps (1) and (2) above done in the > CPU clocks driver, and then step (3) done through a clk notifier. > > However, the current POST_RATE_CHANGE notifier is called too late > (after ->recalc_rate) and only if the rate has changed. So it works > fine for a pure notification of a frequency change, but it doesn't > work if the notified code is actually involved in the frequency > change, which is exactly the case we have here. Until the sequence > that uses the PMSU registers has been executed, the rate of the CPU > clock has not changed. > > In order to solve this problem, we propose to add an APPLY_RATE_CHANGE > notifier event, which gets called right after ->set_rate(), but before > ->recalc_rate(), and therefore regardless of whether there was an > actualy frequency change or not. Is there any reason why we can't call the pmsu code (part #3) directly from the cpu clock driver? It seems like if we just called the .set_rate() op we wouldn't actually have changed the clock's rate. That doesn't seem very intuitive and it really makes the code flow hard to follow.
Dear Stephen Boyd, On Mon, 07 Jul 2014 16:44:18 -0700, Stephen Boyd wrote: > > In order to solve this problem, we propose to add an APPLY_RATE_CHANGE > > notifier event, which gets called right after ->set_rate(), but before > > ->recalc_rate(), and therefore regardless of whether there was an > > actualy frequency change or not. > > Is there any reason why we can't call the pmsu code (part #3) directly > from the cpu clock driver? It seems like if we just called the > .set_rate() op we wouldn't actually have changed the clock's rate. That > doesn't seem very intuitive and it really makes the code flow hard to > follow. Right, but what solution would you propose to achieve that? These days, a direct call from drivers/ code to arch/arm/mach-<foo>/ code is frowned upon, no? (The code handling the PMSU is in arch/arm/mach-mvebu/pmsu.c). Thanks, Thomas
On 07/08/14 00:15, Thomas Petazzoni wrote: > Dear Stephen Boyd, > > On Mon, 07 Jul 2014 16:44:18 -0700, Stephen Boyd wrote: > >>> In order to solve this problem, we propose to add an APPLY_RATE_CHANGE >>> notifier event, which gets called right after ->set_rate(), but before >>> ->recalc_rate(), and therefore regardless of whether there was an >>> actualy frequency change or not. >> Is there any reason why we can't call the pmsu code (part #3) directly >> from the cpu clock driver? It seems like if we just called the >> .set_rate() op we wouldn't actually have changed the clock's rate. That >> doesn't seem very intuitive and it really makes the code flow hard to >> follow. > Right, but what solution would you propose to achieve that? These days, > a direct call from drivers/ code to arch/arm/mach-<foo>/ code is > frowned upon, no? (The code handling the PMSU is in > arch/arm/mach-mvebu/pmsu.c). > I don't see a problem with having an include file in include/linux/ for this, maybe others do though. If it actually is a problem then perhaps moving the pmsu.c code into drivers/ is the right solution.
Dear Stephen Boyd, On Tue, 08 Jul 2014 13:18:54 -0700, Stephen Boyd wrote: > >>> In order to solve this problem, we propose to add an APPLY_RATE_CHANGE > >>> notifier event, which gets called right after ->set_rate(), but before > >>> ->recalc_rate(), and therefore regardless of whether there was an > >>> actualy frequency change or not. > >> Is there any reason why we can't call the pmsu code (part #3) directly > >> from the cpu clock driver? It seems like if we just called the > >> .set_rate() op we wouldn't actually have changed the clock's rate. That > >> doesn't seem very intuitive and it really makes the code flow hard to > >> follow. > > Right, but what solution would you propose to achieve that? These days, > > a direct call from drivers/ code to arch/arm/mach-<foo>/ code is > > frowned upon, no? (The code handling the PMSU is in > > arch/arm/mach-mvebu/pmsu.c). > > I don't see a problem with having an include file in include/linux/ for > this, maybe others do though. I'm fine with that as well, but I believe one of the policy was to avoid having too much drivers/ stuff call into arch/arm/ stuff. > If it actually is a problem then perhaps > moving the pmsu.c code into drivers/ is the right solution. Yes, but where would it belong? The PMSU hardware block is tightly linked to SMP initialization (which means it should be up and running very early), CPU hotplug, cpuidle, cpufreq and suspend/resume. It means that it interacts with a lot of different subsystems. Maybe the solution of adding a direct call from drivers/clk/mvebu/clk-cpu.c to arch/arm/mach-mvebu/pmsu.c is the easiest and most explicit solution. Thanks, Thomas
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 8b73ede..afa559b 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -1525,6 +1525,9 @@ static void clk_change_rate(struct clk *clk) if (!skip_set_rate && clk->ops->set_rate) clk->ops->set_rate(clk->hw, clk->new_rate, best_parent_rate); + if (clk->notifier_count) + __clk_notify(clk, APPLY_RATE_CHANGE, old_rate, clk->new_rate); + clk->rate = clk_recalc(clk, best_parent_rate); if (clk->notifier_count && old_rate != clk->rate) diff --git a/include/linux/clk.h b/include/linux/clk.h index fb5e097..9ea9f5f 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -39,10 +39,14 @@ struct clk; * POST_RATE_CHANGE - called after the clk rate change has successfully * completed. Callbacks must always return NOTIFY_DONE or NOTIFY_OK. * + * APPLY_RATE_CHANGE - called right after calling ->set_rate(), but + * before recalculating the rate. + * */ #define PRE_RATE_CHANGE BIT(0) #define POST_RATE_CHANGE BIT(1) #define ABORT_RATE_CHANGE BIT(2) +#define APPLY_RATE_CHANGE BIT(3) /** * struct clk_notifier - associate a clk with a notifier
The current clk_set_rate() logic allows notifiers to be notified of three different events: * PRE_RATE_CHANGE: before the clock driver ->set_rate() function is called. * ABORT_RATE_CHANGE: called if the rate change failed * POST_RATE_CHANGE: called after the rate change has been successfully done, but after ->recalc_rate() has been called, and only if the rate of the clock has actually changed. This commit adds an additional APPLY_RATE_CHANGE, which we require on Armada XP to implement dynamic frequency scaling of the CPU clocks. The problem is as follows. On Armada XP, there are three hardware blocks involved in the dynamic frequency scaling of the CPU clocks: - The CPU clocks hardware block itself, whose registers are already "managed" by drivers/clk/mvebu/clk-cpu.c (compatible string: marvell,armada-xp-cpu-clock). The driver currently only supports changing the rate of the CPU clock when the clock is off (i.e before we boot the secondary CPUs). - The PMU DFS registers, which are used to configure the target frequency for a dynamic rate change. Those registers are relatively well isolated from other PMU registers, so they can also be "managed" by the drivers/clk/mvebu/clk-cpu.c. - The PMSU registers, which are used to actually trigger the dynamic frequency change procedure, which consists in programming a bunch of PMSU registers, entering the idle state on the CPU we want to change the frequency, and then again reprogram a bunch of PMSU registers. The procedure to change the frequency is: 1/ Reset some clocks using the CPU clocks hardware block. 2/ Define the target frequency using the PMU DFS registers. 3/ Do the actual frequency change using the PMSU registers. However, the PMSU registers are already "managed" by a driver in arch/arm/mach-mvebu/pmsu.c, and the code there is needed for a wide variety of power management activities: booting of secondary CPUs, CPU hotplug, cpuidle, cpufreq, and soon suspend/resume. The same registers in the PMSU are used for several of those activities. So, what we need to do is to have steps (1) and (2) above done in the CPU clocks driver, and then step (3) done through a clk notifier. However, the current POST_RATE_CHANGE notifier is called too late (after ->recalc_rate) and only if the rate has changed. So it works fine for a pure notification of a frequency change, but it doesn't work if the notified code is actually involved in the frequency change, which is exactly the case we have here. Until the sequence that uses the PMSU registers has been executed, the rate of the CPU clock has not changed. In order to solve this problem, we propose to add an APPLY_RATE_CHANGE notifier event, which gets called right after ->set_rate(), but before ->recalc_rate(), and therefore regardless of whether there was an actualy frequency change or not. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- drivers/clk/clk.c | 3 +++ include/linux/clk.h | 4 ++++ 2 files changed, 7 insertions(+)