| Message ID | 1354726121-17190-7-git-send-email-mark.langsdorf@calxeda.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Wed, Dec 5, 2012 at 8:48 AM, Mark Langsdorf <mark.langsdorf@calxeda.com> wrote: > diff --git a/drivers/cpufreq/highbank-cpufreq.c b/drivers/cpufreq/highbank-cpufreq.c > new file mode 100644 > index 0000000..1f28fa6 > --- /dev/null > +++ b/drivers/cpufreq/highbank-cpufreq.c > @@ -0,0 +1,102 @@ Looks pretty good to me. Some tedious nitpicks and discussion below. <snip> > +static int hb_voltage_change(unsigned int freq) > +{ > + int i; > + u32 msg[7]; > + > + msg[0] = HB_CPUFREQ_CHANGE_NOTE; > + msg[1] = freq / 1000000; > + for (i = 2; i < 7; i++) > + msg[i] = 0; > + > + return pl320_ipc_transmit(msg); > +} > + > +static int hb_cpufreq_clk_notify(struct notifier_block *nb, > + unsigned long action, void *hclk) > +{ > + struct clk_notifier_data *clk_data = hclk; > + int i = 0; > + > + if (action == PRE_RATE_CHANGE) { > + if (clk_data->new_rate > clk_data->old_rate) > + while (hb_voltage_change(clk_data->new_rate)) > + if (i++ > 15) There are a few magic numbers here. How about something like: #define HB_VOLT_CHANGE_MAX_TRIES 15 Maybe do the same for the i2c message length? > + return NOTIFY_STOP; How about NOTIFY_BAD? It more clearly signals that an error has occurred. You could also return notifier_from_errno(-ETIMEDOUT) here if you prefer but that would only be for the sake of readability. clk_set_rate doesn't actually return the notifier error code in the event of a notifier abort. > + } else if (action == POST_RATE_CHANGE) { > + if (clk_data->new_rate < clk_data->old_rate) > + while (hb_voltage_change(clk_data->new_rate)) > + if (i++ > 15) > + break; Same as above. It is true that the clock framework does nothing with post-rate change notifier aborts but that might change in the future. > + } > + > + return NOTIFY_DONE; > +} > + > +static struct notifier_block hb_cpufreq_clk_nb = { > + .notifier_call = hb_cpufreq_clk_notify, > +}; > + Do you have any plans to convert your voltage change routine over to the regulator framework? Likewise do you plan to use the OPP library in the future? I can understand if you do not do that since your regulator/dvfs programming model makes things very simple for you. The reason I bring this up is that I did float a patch a while back for a generalized dvfs notifier handler. The prereqs for using it are 1) ccf, 2) regulator fwk, 3) opp definitions. Here is the patch: https://github.com/mturquette/linux/commit/05a280bbc0819a6858d73088a632666f0c7f68a4 And an example usage in the OMAP CPUfreq driver: https://github.com/mturquette/linux/commit/958f10bb98a293aa912e7eb9cd6edbdc51c1c04a I understand if this approach incurs too much software overhead for you but I wanted to throw it out there. It might working nicely in the cpufreq-cpu0 driver or some other "generic" CPUfreq driver for implementing DVFS. Regards, Mike -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/05/2012 12:49 PM, Mike Turquette wrote: > On Wed, Dec 5, 2012 at 8:48 AM, Mark Langsdorf > <mark.langsdorf@calxeda.com> wrote: >> diff --git a/drivers/cpufreq/highbank-cpufreq.c b/drivers/cpufreq/highbank-cpufreq.c >> new file mode 100644 >> index 0000000..1f28fa6 >> --- /dev/null >> +++ b/drivers/cpufreq/highbank-cpufreq.c >> @@ -0,0 +1,102 @@ > > Looks pretty good to me. Some tedious nitpicks and discussion below. > <snip> > >> +static int hb_voltage_change(unsigned int freq) >> +{ >> + int i; >> + u32 msg[7]; >> + >> + msg[0] = HB_CPUFREQ_CHANGE_NOTE; >> + msg[1] = freq / 1000000; >> + for (i = 2; i < 7; i++) >> + msg[i] = 0; >> + >> + return pl320_ipc_transmit(msg); >> +} >> + >> +static int hb_cpufreq_clk_notify(struct notifier_block *nb, >> + unsigned long action, void *hclk) >> +{ >> + struct clk_notifier_data *clk_data = hclk; >> + int i = 0; >> + >> + if (action == PRE_RATE_CHANGE) { >> + if (clk_data->new_rate > clk_data->old_rate) >> + while (hb_voltage_change(clk_data->new_rate)) >> + if (i++ > 15) > > There are a few magic numbers here. How about something like: > > #define HB_VOLT_CHANGE_MAX_TRIES 15 > > Maybe do the same for the i2c message length? Fixed. >> + return NOTIFY_STOP; > > How about NOTIFY_BAD? It more clearly signals that an error has occurred. > Same as above. It is true that the clock framework does nothing with > post-rate change notifier aborts but that might change in the future. Changed and added. >> + } >> + >> + return NOTIFY_DONE; >> +} >> + >> +static struct notifier_block hb_cpufreq_clk_nb = { >> + .notifier_call = hb_cpufreq_clk_notify, >> +}; >> + > > Do you have any plans to convert your voltage change routine over to > the regulator framework? Likewise do you plan to use the OPP library > in the future? I can understand if you do not do that since your > regulator/dvfs programming model makes things very simple for you. I looked at treating the ECME as a voltage regulator, but it was a very bad fit. The ECME has a certain amount of intelligence built into it and corporate plans are to treat voltage control as a black box. The current solution is actually nicely generic from my perspective. The clk notifiers guarantee we can make the voltage changes at the right time regardless of the underlying cpufreq driver implementation. I don't think we need more until we get into cpufreq QoS issues, and even then I'd want to stick with something like the current structure. --Mark Langsdorf Calxeda, Inc. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 05, 2012 at 10:48:41AM -0600, Mark Langsdorf wrote: > Highbank processors depend on the external ECME to perform voltage > management based on a requested frequency. Communication between the > A9 cores and the ECME happens over the pl320 IPC channel. > > Signed-off-by: Mark Langsdorf <mark.langsdorf@calxeda.com> > Cc: shawn.guo@linaro.org Reviewed-by: Shawn Guo <shawn.guo@linaro.org> -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/boot/dts/highbank.dts b/arch/arm/boot/dts/highbank.dts index 0c6fc34..7c4c27d 100644 --- a/arch/arm/boot/dts/highbank.dts +++ b/arch/arm/boot/dts/highbank.dts @@ -36,6 +36,16 @@ next-level-cache = <&L2>; clocks = <&a9pll>; clock-names = "cpu"; + operating-points = < + /* kHz ignored */ + 1300000 1000000 + 1200000 1000000 + 1100000 1000000 + 800000 1000000 + 400000 1000000 + 200000 1000000 + >; + clock-latency = <100000>; }; cpu@1 { diff --git a/arch/arm/mach-highbank/Kconfig b/arch/arm/mach-highbank/Kconfig index 2896881..b7862da 100644 --- a/arch/arm/mach-highbank/Kconfig +++ b/arch/arm/mach-highbank/Kconfig @@ -1,5 +1,7 @@ config ARCH_HIGHBANK bool "Calxeda ECX-1000 (Highbank)" if ARCH_MULTI_V7 + select ARCH_HAS_CPUFREQ + select ARCH_HAS_OPP select ARCH_WANT_OPTIONAL_GPIOLIB select ARM_AMBA select ARM_GIC diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm index 5961e64..7aaac9f 100644 --- a/drivers/cpufreq/Kconfig.arm +++ b/drivers/cpufreq/Kconfig.arm @@ -76,3 +76,19 @@ config ARM_EXYNOS5250_CPUFREQ help This adds the CPUFreq driver for Samsung EXYNOS5250 SoC. + +config ARM_HIGHBANK_CPUFREQ + tristate "Calxeda Highbank-based" + depends on ARCH_HIGHBANK + select CPU_FREQ_TABLE + select GENERIC_CPUFREQ_CPU0 + select PM_OPP + select REGULATOR + + default m + help + This adds the CPUFreq driver for Calxeda Highbank SoC + based boards. + + If in doubt, say N. + diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile index 1bc90e1..9e8f12a 100644 --- a/drivers/cpufreq/Makefile +++ b/drivers/cpufreq/Makefile @@ -50,6 +50,7 @@ obj-$(CONFIG_ARM_EXYNOS4210_CPUFREQ) += exynos4210-cpufreq.o obj-$(CONFIG_ARM_EXYNOS4X12_CPUFREQ) += exynos4x12-cpufreq.o obj-$(CONFIG_ARM_EXYNOS5250_CPUFREQ) += exynos5250-cpufreq.o obj-$(CONFIG_ARM_OMAP2PLUS_CPUFREQ) += omap-cpufreq.o +obj-$(CONFIG_ARM_HIGHBANK_CPUFREQ) += highbank-cpufreq.o ################################################################################## # PowerPC platform drivers diff --git a/drivers/cpufreq/highbank-cpufreq.c b/drivers/cpufreq/highbank-cpufreq.c new file mode 100644 index 0000000..1f28fa6 --- /dev/null +++ b/drivers/cpufreq/highbank-cpufreq.c @@ -0,0 +1,102 @@ +/* + * Copyright (C) 2012 Calxeda, Inc. + * + * 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. + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/clk.h> +#include <linux/cpu.h> +#include <linux/err.h> +#include <linux/of.h> +#include <linux/mailbox.h> + +#define HB_CPUFREQ_CHANGE_NOTE 0x80000001 + +static int hb_voltage_change(unsigned int freq) +{ + int i; + u32 msg[7]; + + msg[0] = HB_CPUFREQ_CHANGE_NOTE; + msg[1] = freq / 1000000; + for (i = 2; i < 7; i++) + msg[i] = 0; + + return pl320_ipc_transmit(msg); +} + +static int hb_cpufreq_clk_notify(struct notifier_block *nb, + unsigned long action, void *hclk) +{ + struct clk_notifier_data *clk_data = hclk; + int i = 0; + + if (action == PRE_RATE_CHANGE) { + if (clk_data->new_rate > clk_data->old_rate) + while (hb_voltage_change(clk_data->new_rate)) + if (i++ > 15) + return NOTIFY_STOP; + } else if (action == POST_RATE_CHANGE) { + if (clk_data->new_rate < clk_data->old_rate) + while (hb_voltage_change(clk_data->new_rate)) + if (i++ > 15) + break; + } + + return NOTIFY_DONE; +} + +static struct notifier_block hb_cpufreq_clk_nb = { + .notifier_call = hb_cpufreq_clk_notify, +}; + +static int hb_cpufreq_driver_init(void) +{ + struct device *cpu_dev; + struct clk *cpu_clk; + struct device_node *np; + int ret; + + np = of_find_node_by_path("/cpus/cpu@0"); + if (!np) { + pr_err("failed to find highbank cpufreq node\n"); + return -ENOENT; + } + + cpu_dev = get_cpu_device(0); + if (!cpu_dev) { + pr_err("failed to get highbank cpufreq device\n"); + ret = -ENODEV; + goto out_put_node; + } + + cpu_dev->of_node = np; + + cpu_clk = clk_get(cpu_dev, NULL); + if (IS_ERR(cpu_clk)) { + ret = PTR_ERR(cpu_clk); + pr_err("failed to get cpu0 clock: %d\n", ret); + goto out_put_node; + } + + ret = clk_notifier_register(cpu_clk, &hb_cpufreq_clk_nb); + if (ret) { + pr_err("failed to register clk notifier: %d\n", ret); + goto out_put_node; + } + +out_put_node: + of_node_put(np); + return ret; +} +late_initcall(hb_cpufreq_driver_init); + +MODULE_AUTHOR("Mark Langsdorf <mark.langsdorf@calxeda.com>"); +MODULE_DESCRIPTION("Calxeda Highbank cpufreq driver"); +MODULE_LICENSE("GPL");
Highbank processors depend on the external ECME to perform voltage management based on a requested frequency. Communication between the A9 cores and the ECME happens over the pl320 IPC channel. Signed-off-by: Mark Langsdorf <mark.langsdorf@calxeda.com> Cc: shawn.guo@linaro.org Cc: mturquette@linaro.org --- Changes from v7 Removed old attribution to cpufreq-cpu0. Added some description in the documentation. Made cpu_dev, cpu_clk into local variables. Removed __devinit. Removed some unneeded includes. Added a brace to clarify some nested if logic. Changes from v6 Removed devicetree bindings documentation. Restructured driver to use clk notifications. Core driver logic is now cpufreq-clk0. Changes from v5 Changed ipc_transmit() to pl320_ipc_transmit(). Changes from v4 Removed erroneous changes to arch/arm/Kconfig. Removed unnecessary changes to drivers/cpufreq/Kconfig.arm Alphabetized additions to arch/arm/mach-highbank/Kconfig Changed ipc call and header to match new ipc location in drivers/mailbox. Changes from v3 None. Changes from v2 Changed transition latency binding in code to match documentation. Changes from v1 Added highbank specific Kconfig changes. arch/arm/boot/dts/highbank.dts | 10 ++++ arch/arm/mach-highbank/Kconfig | 2 + drivers/cpufreq/Kconfig.arm | 16 ++++++ drivers/cpufreq/Makefile | 1 + drivers/cpufreq/highbank-cpufreq.c | 102 +++++++++++++++++++++++++++++++++++++ 5 files changed, 131 insertions(+) create mode 100644 drivers/cpufreq/highbank-cpufreq.c