| Message ID | 1359215039-2848-2-git-send-email-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andrew, Few more comments from my side :( On 26 January 2013 21:13, Andrew Lunn <andrew@lunn.ch> wrote: > diff --git a/drivers/cpufreq/kirkwood-cpufreq.c b/drivers/cpufreq/kirkwood-cpufreq.c > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/clk.h> > +#include <linux/cpufreq.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/io.h> > +#include <asm/proc-fns.h> > + > +#define CPU_SW_INT_BLK BIT(28) > + > + > +#include <linux/clk-private.h> Any specific reason for keeping it together with other #includes? > +static void kirkwood_cpufreq_set_cpu_state(unsigned int index) > +{ > + Can remove extra blank line. > + if (freqs.old != freqs.new) { > + local_irq_disable(); > + > + /* Disable interrupts to the CPU */ > + reg = readl_relaxed(priv.base); > + reg |= CPU_SW_INT_BLK; > + writel(reg, priv.base); Any specific reason for calling writel() instead of writel_relaxed()? relaxed one would take less time and would be much more efficient. same for all other writel's too. > +}; > + > +static int kirkwood_cpufreq_verify(struct cpufreq_policy *policy) > +{ > + return cpufreq_frequency_table_verify(policy, &kirkwood_freq_table[0]); return cpufreq_frequency_table_verify(policy, kirkwood_freq_table); ?? > +} > + > +static int kirkwood_cpufreq_target(struct cpufreq_policy *policy, > + unsigned int target_freq, > + unsigned int relation) > +{ > + unsigned int index = 0; > + > + if (cpufreq_frequency_table_target(policy, kirkwood_freq_table, > + target_freq, relation, &index)) > + return -EINVAL; > + > + kirkwood_cpufreq_set_cpu_state(index); This routine does have error cases, like: wrong value of "state" variable, so returning int from it might make sense. Though i believe state can't be anything else then STATE_CPU_FREQ or STATE_DDR_FREQ. In which case, switch must be modified? > + return 0; > +} > + > +/* > + * Module init and exit code > + */ Can be converted to a single line comment if you want. :) > +static int kirkwood_cpufreq_cpu_exit(struct cpufreq_policy *policy) > +{ > + cpufreq_frequency_table_put_attr(policy->cpu); > + return 0; > +} Hmm.. Exit is normally called in two cases. Either cpufreq driver is unregistered or cpu is removed. In that case i am not sure if this routine makes sense? As we have a uniprocessor SoC here. @Rafael? > +static struct freq_attr *kirkwood_cpufreq_attr[] = { > + &cpufreq_freq_attr_scaling_available_freqs, > + NULL, > +}; > + > + Can remove extra blank line.
On Sun, Jan 27, 2013 at 09:21:42AM +0530, Viresh Kumar wrote: > Hi Andrew, > > Few more comments from my side :( No problems... > > On 26 January 2013 21:13, Andrew Lunn <andrew@lunn.ch> wrote: > > diff --git a/drivers/cpufreq/kirkwood-cpufreq.c b/drivers/cpufreq/kirkwood-cpufreq.c > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/clk.h> > > +#include <linux/cpufreq.h> > > +#include <linux/of.h> > > +#include <linux/platform_device.h> > > +#include <linux/io.h> > > +#include <asm/proc-fns.h> > > + > > +#define CPU_SW_INT_BLK BIT(28) > > + > > + > > +#include <linux/clk-private.h> > > Any specific reason for keeping it together with other #includes? Ah, yes, there is a good reason. I added this in for debugging purposes. I needed to look inside the clk cookie, which required this include file. But because it was for debug only, to be removed later, i kept it separate, easier to find and remove. I just forgot to remove it :-( > > + if (freqs.old != freqs.new) { > > + local_irq_disable(); > > + > > + /* Disable interrupts to the CPU */ > > + reg = readl_relaxed(priv.base); > > + reg |= CPU_SW_INT_BLK; > > + writel(reg, priv.base); > > +static int kirkwood_cpufreq_target(struct cpufreq_policy *policy, > > + unsigned int target_freq, > > + unsigned int relation) > > +{ > > + unsigned int index = 0; > > + > > + if (cpufreq_frequency_table_target(policy, kirkwood_freq_table, > > + target_freq, relation, &index)) > > + return -EINVAL; > > + > > + kirkwood_cpufreq_set_cpu_state(index); > > This routine does have error cases, like: wrong value of "state" > variable, so returning int from it might make sense. Though i believe > state can't be anything else then STATE_CPU_FREQ or STATE_DDR_FREQ. > In which case, switch must be modified? As you say, state cannot be anything else that the two expected values. I've just been taught that switch statements should always have a default clause. I also thought that a BUG() is too strong, no need to kill the machine. It is better to spam the kernel log so it gets noticed. I can swap to a WARN_ON(1). I don't really think this is an error that needs to be reported back to the framework. Its an implementation problem, not a runtime problem. So i would prefer to keep to void. > > +static int kirkwood_cpufreq_cpu_exit(struct cpufreq_policy *policy) > > +{ > > + cpufreq_frequency_table_put_attr(policy->cpu); > > + return 0; > > +} > > Hmm.. Exit is normally called in two cases. Either cpufreq driver is > unregistered > or cpu is removed. In that case i am not sure if this routine makes sense? As > we have a uniprocessor SoC here. The current Kconfig entry does not allow it to be compiled as a module. But the code does allow for it. With the current trend of making the ARM kernel multiplatform, its likely that cpufreq drivers will become modular so that only the appropriate driver gets loaded in a multiplatform kernel. The question then becomes, is it O.K. not being able to unload the module? Andrew
On 27 January 2013 14:29, Andrew Lunn <andrew@lunn.ch> wrote: > On Sun, Jan 27, 2013 at 09:21:42AM +0530, Viresh Kumar wrote: >> > + kirkwood_cpufreq_set_cpu_state(index); >> >> This routine does have error cases, like: wrong value of "state" >> variable, so returning int from it might make sense. Though i believe >> state can't be anything else then STATE_CPU_FREQ or STATE_DDR_FREQ. >> In which case, switch must be modified? > > As you say, state cannot be anything else that the two expected > values. I've just been taught that switch statements should always > have a default clause. I also thought that a BUG() is too strong, no > need to kill the machine. It is better to spam the kernel log so it > gets noticed. I can swap to a WARN_ON(1). I don't really think this is > an error that needs to be reported back to the framework. Its an > implementation problem, not a runtime problem. So i would prefer to > keep to void. Hmm. What i meant to say was, you just need an if,else instead of switch and you really don't have to take care of anything else than these two values. As that can't happen. Even the WARN_ON(). Its useless. Just believe on what is returned from cpufreq_frequency_table_target(). You really don't need to validate index returned from it, but only the return value (which you are already doing) >> > +static int kirkwood_cpufreq_cpu_exit(struct cpufreq_policy *policy) >> > +{ >> > + cpufreq_frequency_table_put_attr(policy->cpu); >> > + return 0; >> > +} >> >> Hmm.. Exit is normally called in two cases. Either cpufreq driver is >> unregistered >> or cpu is removed. In that case i am not sure if this routine makes sense? As >> we have a uniprocessor SoC here. > > The current Kconfig entry does not allow it to be compiled as a > module. But the code does allow for it. With the current trend of > making the ARM kernel multiplatform, its likely that cpufreq drivers > will become modular so that only the appropriate driver gets loaded in > a multiplatform kernel. The question then becomes, is it O.K. not > being able to unload the module? You got me wrong. What i wanted to say is, even if you have it as module and "rmmod" it, then also calling this routine wouldn't make any difference. Then i went into the code to see the effect. So, this will set freq_table for a cpu as NULL which will be returned by calling cpufreq_frequency_get_table(). And i now feel we must have this routine because once cpufreq driver isn't there for a cpu, we must not return any table for it. :)
diff --git a/drivers/clk/mvebu/clk-gating-ctrl.c b/drivers/clk/mvebu/clk-gating-ctrl.c index 8fa5408..ebf141d 100644 --- a/drivers/clk/mvebu/clk-gating-ctrl.c +++ b/drivers/clk/mvebu/clk-gating-ctrl.c @@ -193,6 +193,7 @@ static const struct mvebu_soc_descr __initconst kirkwood_gating_descr[] = { { "runit", NULL, 7 }, { "xor0", NULL, 8 }, { "audio", NULL, 9 }, + { "powersave", "cpuclk", 11 }, { "sata0", NULL, 14 }, { "sata1", NULL, 15 }, { "xor1", NULL, 16 }, diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm index a0b3661..80c8229 100644 --- a/drivers/cpufreq/Kconfig.arm +++ b/drivers/cpufreq/Kconfig.arm @@ -77,6 +77,12 @@ config ARM_EXYNOS5250_CPUFREQ This adds the CPUFreq driver for Samsung EXYNOS5250 SoC. +config ARM_KIRKWOOD_CPUFREQ + def_bool ARCH_KIRKWOOD && OF + help + This adds the CPUFreq driver for Marvell Kirkwood + SoCs. + config ARM_SPEAR_CPUFREQ bool "SPEAr CPUFreq support" depends on PLAT_SPEAR diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile index fadc4d4..39a0ffe 100644 --- a/drivers/cpufreq/Makefile +++ b/drivers/cpufreq/Makefile @@ -50,6 +50,7 @@ obj-$(CONFIG_ARM_EXYNOS_CPUFREQ) += exynos-cpufreq.o 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_KIRKWOOD_CPUFREQ) += kirkwood-cpufreq.o obj-$(CONFIG_ARM_OMAP2PLUS_CPUFREQ) += omap-cpufreq.o obj-$(CONFIG_ARM_SPEAR_CPUFREQ) += spear-cpufreq.o diff --git a/drivers/cpufreq/kirkwood-cpufreq.c b/drivers/cpufreq/kirkwood-cpufreq.c new file mode 100644 index 0000000..300d01d --- /dev/null +++ b/drivers/cpufreq/kirkwood-cpufreq.c @@ -0,0 +1,271 @@ +/* + * kirkwood_freq.c: cpufreq driver for the Marvell kirkwood + * + * Copyright (C) 2013 Andrew Lunn <andrew@lunn.ch> + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version + * 2 of the License, or (at your option) any later version. + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/clk.h> +#include <linux/cpufreq.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/io.h> +#include <asm/proc-fns.h> + +#define CPU_SW_INT_BLK BIT(28) + + +#include <linux/clk-private.h> + +static struct priv +{ + struct clk *cpu_clk; + struct clk *ddr_clk; + struct clk *powersave_clk; + struct device *dev; + void __iomem *base; +} priv; + +#define STATE_CPU_FREQ 0x01 +#define STATE_DDR_FREQ 0x02 + +/* + * Kirkwood can swap the clock to the CPU between two clocks: + * + * - cpu clk + * - ddr clk + * + * The frequencies are set at runtime before registering this * + * table. + */ +static struct cpufreq_frequency_table kirkwood_freq_table[] = { + {STATE_CPU_FREQ, 0}, /* CPU uses cpuclk */ + {STATE_DDR_FREQ, 0}, /* CPU uses ddrclk */ + {0, CPUFREQ_TABLE_END}, +}; + +static unsigned int kirkwood_cpufreq_get_cpu_frequency(unsigned int cpu) +{ + if (__clk_is_enabled(priv.powersave_clk)) + return kirkwood_freq_table[1].frequency; + return kirkwood_freq_table[0].frequency; +} + +static void kirkwood_cpufreq_set_cpu_state(unsigned int index) +{ + + struct cpufreq_freqs freqs; + unsigned int state = kirkwood_freq_table[index].index; + unsigned long reg; + + freqs.old = kirkwood_cpufreq_get_cpu_frequency(0); + freqs.new = kirkwood_freq_table[index].frequency; + freqs.cpu = 0; /* Kirkwood is UP */ + + cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE); + + dev_dbg(priv.dev, "Attempting to set frequency to %i KHz\n", + kirkwood_freq_table[index].frequency); + dev_dbg(priv.dev, "old frequency was %i KHz\n", + kirkwood_cpufreq_get_cpu_frequency(0)); + + if (freqs.old != freqs.new) { + local_irq_disable(); + + /* Disable interrupts to the CPU */ + reg = readl_relaxed(priv.base); + reg |= CPU_SW_INT_BLK; + writel(reg, priv.base); + + switch (state) { + case STATE_CPU_FREQ: + clk_disable(priv.powersave_clk); + break; + case STATE_DDR_FREQ: + clk_enable(priv.powersave_clk); + break; + default: + dev_err(priv.dev, "Unexpected cpufreq state"); + } + + /* Wait-for-Interrupt, which the hardware changes frequency */ + cpu_do_idle(); + + /* Enable interrupts to the CPU */ + reg = readl_relaxed(priv.base); + reg &= ~CPU_SW_INT_BLK; + writel(reg, priv.base); + + local_irq_enable(); + } + cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE); +}; + +static int kirkwood_cpufreq_verify(struct cpufreq_policy *policy) +{ + return cpufreq_frequency_table_verify(policy, &kirkwood_freq_table[0]); +} + +static int kirkwood_cpufreq_target(struct cpufreq_policy *policy, + unsigned int target_freq, + unsigned int relation) +{ + unsigned int index = 0; + + if (cpufreq_frequency_table_target(policy, kirkwood_freq_table, + target_freq, relation, &index)) + return -EINVAL; + + kirkwood_cpufreq_set_cpu_state(index); + + return 0; +} + +/* + * Module init and exit code + */ +static int kirkwood_cpufreq_cpu_init(struct cpufreq_policy *policy) +{ + int result; + + /* cpuinfo and default policy values */ + policy->cpuinfo.transition_latency = 5000; /* 5uS */ + policy->cur = kirkwood_cpufreq_get_cpu_frequency(0); + + result = cpufreq_frequency_table_cpuinfo(policy, kirkwood_freq_table); + if (result) + return result; + + cpufreq_frequency_table_get_attr(kirkwood_freq_table, policy->cpu); + + return 0; +} + +static int kirkwood_cpufreq_cpu_exit(struct cpufreq_policy *policy) +{ + cpufreq_frequency_table_put_attr(policy->cpu); + return 0; +} + +static struct freq_attr *kirkwood_cpufreq_attr[] = { + &cpufreq_freq_attr_scaling_available_freqs, + NULL, +}; + + +static struct cpufreq_driver kirkwood_cpufreq_driver = { + .get = kirkwood_cpufreq_get_cpu_frequency, + .verify = kirkwood_cpufreq_verify, + .target = kirkwood_cpufreq_target, + .init = kirkwood_cpufreq_cpu_init, + .exit = kirkwood_cpufreq_cpu_exit, + .name = "kirkwood_freq", + .owner = THIS_MODULE, + .attr = kirkwood_cpufreq_attr, +}; + +static int kirkwood_cpufreq_probe(struct platform_device *pdev) +{ + struct device_node *np = of_find_compatible_node( + NULL, NULL, "marvell,kirkwood-core-clock"); + + struct of_phandle_args clkspec; + struct resource *res; + int err; + + priv.dev = &pdev->dev; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) { + dev_err(&pdev->dev, "Cannot get memory resource\n"); + return -ENODEV; + } + priv.base = devm_request_and_ioremap(&pdev->dev, res); + if (!priv.base) { + dev_err(&pdev->dev, "Cannot ioremap\n"); + return -ENOMEM; + } + + clkspec.np = np; + clkspec.args_count = 1; + clkspec.args[0] = 1; + + priv.cpu_clk = of_clk_get_from_provider(&clkspec); + if (IS_ERR(priv.cpu_clk)) { + dev_err(priv.dev, "Unable to get cpuclk"); + return PTR_ERR(priv.cpu_clk); + } + + clk_prepare_enable(priv.cpu_clk); + kirkwood_freq_table[0].frequency = clk_get_rate(priv.cpu_clk) / 1000; + + clkspec.args[0] = 3; + priv.ddr_clk = of_clk_get_from_provider(&clkspec); + if (IS_ERR(priv.ddr_clk)) { + dev_err(priv.dev, "Unable to get ddrclk"); + err = PTR_ERR(priv.ddr_clk); + goto out_cpu; + } + + clk_prepare_enable(priv.ddr_clk); + kirkwood_freq_table[1].frequency = clk_get_rate(priv.ddr_clk) / 1000; + + np = of_find_compatible_node(NULL, NULL, + "marvell,kirkwood-gating-clock"); + clkspec.np = np; + clkspec.args[0] = 11; + priv.powersave_clk = of_clk_get_from_provider(&clkspec); + if (IS_ERR(priv.powersave_clk)) { + dev_err(priv.dev, "Unable to get powersave"); + err = PTR_ERR(priv.powersave_clk); + goto out_ddr; + } + clk_prepare(priv.powersave_clk); + + err = cpufreq_register_driver(&kirkwood_cpufreq_driver); + if (!err) + return 0; + dev_err(priv.dev, "Failed to register cpufreq driver"); + + clk_disable_unprepare(priv.powersave_clk); +out_ddr: + clk_disable_unprepare(priv.ddr_clk); +out_cpu: + clk_disable_unprepare(priv.cpu_clk); + + return err; +} + + +static int kirkwood_cpufreq_remove(struct platform_device *pdev) +{ + cpufreq_unregister_driver(&kirkwood_cpufreq_driver); + + clk_disable_unprepare(priv.powersave_clk); + clk_disable_unprepare(priv.ddr_clk); + clk_disable_unprepare(priv.cpu_clk); + + return 0; +} + +static struct platform_driver kirkwood_cpufreq_platform_driver = { + .probe = kirkwood_cpufreq_probe, + .remove = kirkwood_cpufreq_remove, + .driver = { + .name = "kirkwood-cpufreq", + .owner = THIS_MODULE, + }, +}; + +module_platform_driver(kirkwood_cpufreq_platform_driver); + +MODULE_LICENSE("GPL v2"); +MODULE_AUTHOR("Andrew Lunn <andrew@lunn.ch"); +MODULE_DESCRIPTION("cpufreq driver for Marvell's kirkwood CPU"); +MODULE_ALIAS("platform:kirkwood-cpufreq");
The Marvell Kirkwood SoCs have simple cpufreq support in hardware. The CPU can either use the a high speed cpu clock, or the slower DDR clock. Add a driver to swap between these two clock sources. Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- drivers/clk/mvebu/clk-gating-ctrl.c | 1 + drivers/cpufreq/Kconfig.arm | 6 + drivers/cpufreq/Makefile | 1 + drivers/cpufreq/kirkwood-cpufreq.c | 271 +++++++++++++++++++++++++++++++++++ 4 files changed, 279 insertions(+) create mode 100644 drivers/cpufreq/kirkwood-cpufreq.c