* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
2013-10-19 12:37 ` [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove Andrew Lunn
@ 2013-10-19 23:09 ` Luka Perkov
2013-10-20 8:42 ` Andrew Lunn
2013-10-21 10:42 ` Sebastian Hesselbarth
` (2 subsequent siblings)
3 siblings, 1 reply; 16+ messages in thread
From: Luka Perkov @ 2013-10-19 23:09 UTC (permalink / raw)
To: linux-arm-kernel
Hi Andrew,
On Sat, Oct 19, 2013 at 02:37:38PM +0200, Andrew Lunn wrote:
> The Marvell Dove SoC can run the CPU at two frequencies. The high
> frequencey is from a PLL, while the lower is the same as the DDR
> clock. Add a cpufreq driver to swap between these frequences.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
> drivers/cpufreq/Kconfig.arm | 7 ++
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/dove-cpufreq.c | 276 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 284 insertions(+)
> create mode 100644 drivers/cpufreq/dove-cpufreq.c
...
> --- /dev/null
> +++ b/drivers/cpufreq/dove-cpufreq.c
> @@ -0,0 +1,276 @@
> +/*
> + * dove_freq.c: cpufreq driver for the Marvell dove
> + *
> + * 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.
> + */
> +
> +#define DEBUG
It seems to me that this define is not used at all...
Luka
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
2013-10-19 23:09 ` Luka Perkov
@ 2013-10-20 8:42 ` Andrew Lunn
0 siblings, 0 replies; 16+ messages in thread
From: Andrew Lunn @ 2013-10-20 8:42 UTC (permalink / raw)
To: linux-arm-kernel
On Sun, Oct 20, 2013 at 01:09:08AM +0200, Luka Perkov wrote:
> Hi Andrew,
>
> On Sat, Oct 19, 2013 at 02:37:38PM +0200, Andrew Lunn wrote:
> > The Marvell Dove SoC can run the CPU at two frequencies. The high
> > frequencey is from a PLL, while the lower is the same as the DDR
> > clock. Add a cpufreq driver to swap between these frequences.
> >
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > ---
> > drivers/cpufreq/Kconfig.arm | 7 ++
> > drivers/cpufreq/Makefile | 1 +
> > drivers/cpufreq/dove-cpufreq.c | 276 +++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 284 insertions(+)
> > create mode 100644 drivers/cpufreq/dove-cpufreq.c
>
> ...
>
> > --- /dev/null
> > +++ b/drivers/cpufreq/dove-cpufreq.c
> > @@ -0,0 +1,276 @@
> > +/*
> > + * dove_freq.c: cpufreq driver for the Marvell dove
> > + *
> > + * 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.
> > + */
> > +
> > +#define DEBUG
>
> It seems to me that this define is not used at all...
Hi Luke
Correct. Its left over from my debugging. I will collect more comments
and then remove it in a v2 patchset.
Thanks
Andrew
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
2013-10-19 12:37 ` [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove Andrew Lunn
2013-10-19 23:09 ` Luka Perkov
@ 2013-10-21 10:42 ` Sebastian Hesselbarth
2013-10-21 15:42 ` Andrew Lunn
2013-10-22 9:01 ` Viresh Kumar
2013-10-22 10:19 ` Sudeep KarkadaNagesha
3 siblings, 1 reply; 16+ messages in thread
From: Sebastian Hesselbarth @ 2013-10-21 10:42 UTC (permalink / raw)
To: linux-arm-kernel
On 10/19/2013 01:37 PM, Andrew Lunn wrote:
> The Marvell Dove SoC can run the CPU at two frequencies. The high
> frequencey is from a PLL, while the lower is the same as the DDR
> clock. Add a cpufreq driver to swap between these frequences.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Andrew,
thanks for adding Dove cpufreq! I have some remarks below.
> ---
> drivers/cpufreq/Kconfig.arm | 7 ++
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/dove-cpufreq.c | 276 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 284 insertions(+)
> create mode 100644 drivers/cpufreq/dove-cpufreq.c
>
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 701ec95..3d77633 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -8,6 +8,13 @@ config ARM_BIG_LITTLE_CPUFREQ
> help
> This enables the Generic CPUfreq driver for ARM big.LITTLE platforms.
>
> +config ARM_DOVE_CPUFREQ
> + def_bool ARCH_DOVE && OF
> + select CPU_FREQ_TABLE
> + help
> + This adds the CPUFreq driver for Marvell Dove
> + SoCs.
> +
> config ARM_DT_BL_CPUFREQ
> tristate "Generic probing via DT for ARM big LITTLE CPUfreq driver"
> depends on ARM_BIG_LITTLE_CPUFREQ && OF
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index b7948bb..5956661 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -49,6 +49,7 @@ obj-$(CONFIG_ARM_DT_BL_CPUFREQ) += arm_big_little_dt.o
>
> obj-$(CONFIG_ARCH_DAVINCI_DA850) += davinci-cpufreq.o
> obj-$(CONFIG_UX500_SOC_DB8500) += dbx500-cpufreq.o
> +obj-$(CONFIG_ARM_DOVE_CPUFREQ) += dove-cpufreq.o
> obj-$(CONFIG_ARM_EXYNOS_CPUFREQ) += exynos-cpufreq.o
> obj-$(CONFIG_ARM_EXYNOS4210_CPUFREQ) += exynos4210-cpufreq.o
> obj-$(CONFIG_ARM_EXYNOS4X12_CPUFREQ) += exynos4x12-cpufreq.o
> diff --git a/drivers/cpufreq/dove-cpufreq.c b/drivers/cpufreq/dove-cpufreq.c
> new file mode 100644
> index 0000000..4730b05
> --- /dev/null
> +++ b/drivers/cpufreq/dove-cpufreq.c
> @@ -0,0 +1,276 @@
> +/*
> + * dove_freq.c: cpufreq driver for the Marvell dove
> + *
> + * 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.
> + */
> +
> +#define DEBUG
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/cpufreq.h>
> +#include <linux/interrupt.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/of_irq.h>
> +#include <asm/proc-fns.h>
> +
> +#define DFS_CR 0x00
> +#define DFS_EN BIT(0)
> +#define CPU_SLOW_EN BIT(1)
> +#define L2_RATIO_OFFS 9
> +#define L2_RATIO_MASK (0x3F << L2_RATIO_OFFS)
> +#define DFS_SR 0x04
> +#define CPU_SLOW_MODE_STTS BIT(1)
> +
> +/* PMU_CR */
> +#define MASK_FIQ BIT(28)
> +#define MASK_IRQ BIT(24) /* PMU_CR */
> +
> +/* CPU Clock Divider Control 0 Register */
> +#define DPRATIO_OFFS 24
> +#define DPRATIO_MASK (0x3F << DPRATIO_OFFS)
> +#define XPRATIO_OFFS 16
> +#define XPRATIO_MASK (0x3F << XPRATIO_OFFS)
> +
> +static struct priv
> +{
> + struct clk *cpu_clk;
> + struct clk *ddr_clk;
> + struct device *dev;
> + unsigned long dpratio;
> + unsigned long xpratio;
> + void __iomem *dfs;
> + void __iomem *pmu_cr;
> + void __iomem *pmu_clk_div;
> +} priv;
> +
> +#define STATE_CPU_FREQ 0x01
> +#define STATE_DDR_FREQ 0x02
> +
> +/*
> + * Dove 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 dove_freq_table[] = {
> + {STATE_CPU_FREQ, 0}, /* CPU uses cpuclk */
> + {STATE_DDR_FREQ, 0}, /* CPU uses ddrclk */
> + {0, CPUFREQ_TABLE_END},
> +};
> +
> +static unsigned int dove_cpufreq_get_cpu_frequency(unsigned int cpu)
> +{
> + unsigned long reg = readl_relaxed(priv.dfs + DFS_SR);
> +
> + if (reg & CPU_SLOW_MODE_STTS)
> + return dove_freq_table[1].frequency;
> + return dove_freq_table[0].frequency;
> +}
> +
> +static void dove_cpufreq_set_cpu_state(struct cpufreq_policy *policy,
> + unsigned int index)
> +{
> + struct cpufreq_freqs freqs;
> + unsigned int state = dove_freq_table[index].driver_data;
> + unsigned long reg, cr;
> +
> + freqs.old = dove_cpufreq_get_cpu_frequency(0);
> + freqs.new = dove_freq_table[index].frequency;
> +
> + cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE);
> +
> + if (freqs.old != freqs.new) {
> + local_irq_disable();
> +
> + /* Mask IRQ and FIQ to CPU */
> + cr = readl(priv.pmu_cr);
> + cr |= MASK_IRQ | MASK_FIQ;
> + writel(cr, priv.pmu_cr);
> +
> + /* Set/Clear the CPU_SLOW_EN bit */
> + reg = readl_relaxed(priv.dfs + DFS_CR);
> + reg &= ~L2_RATIO_MASK;
> +
> + switch (state) {
> + case STATE_CPU_FREQ:
> + reg |= priv.xpratio;
> + reg &= ~CPU_SLOW_EN;
> + break;
> + case STATE_DDR_FREQ:
> + reg |= (priv.dpratio | CPU_SLOW_EN);
Does slow mode really (only) depend on the value written into CPUL2CR?
Dove FS isn't that chatty about it and I cannot really test right now.
But reading it, I think there is two CPU PLL to {L2,DDR} ratios that you
can switch between by (de-)asserting CPU_SLOW_EN.
If so, I guess it would be better to set those ratios once in init and
take care of CPU_SLOW_EN here only.
> + break;
> + }
> +
> + /* Start the DFS process */
> + reg |= DFS_EN;
> +
> + writel(reg, priv.dfs + DFS_CR);
> +
> + /* Wait-for-Interrupt, while the hardware changes frequency */
> + cpu_do_idle();
> +
> + local_irq_enable();
> + }
> + cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
> +}
> +
> +static int dove_cpufreq_target(struct cpufreq_policy *policy,
> + unsigned int target_freq,
> + unsigned int relation)
> +{
> + unsigned int index = 0;
> +
> + if (cpufreq_frequency_table_target(policy, dove_freq_table,
> + target_freq, relation, &index))
> + return -EINVAL;
> +
> + dove_cpufreq_set_cpu_state(policy, index);
> +
> + return 0;
> +}
> +
> +static int dove_cpufreq_cpu_init(struct cpufreq_policy *policy)
> +{
> + return cpufreq_generic_init(policy, dove_freq_table, 5000);
> +}
> +
> +static irqreturn_t dove_cpufreq_irq(int irq, void *dev)
> +{
> + return IRQ_HANDLED;
> +}
> +
> +static struct cpufreq_driver dove_cpufreq_driver = {
> + .get = dove_cpufreq_get_cpu_frequency,
> + .verify = cpufreq_generic_frequency_table_verify,
> + .target = dove_cpufreq_target,
> + .init = dove_cpufreq_cpu_init,
> + .exit = cpufreq_generic_exit,
> + .name = "dove-cpufreq",
> + .attr = cpufreq_generic_attr,
> +};
> +
> +static int dove_cpufreq_probe(struct platform_device *pdev)
> +{
> + struct device_node *np;
> + struct resource *res;
> + int err;
> + int irq;
> +
> + priv.dev = &pdev->dev;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + priv.dfs = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(priv.dfs))
> + return PTR_ERR(priv.dfs);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + priv.pmu_cr = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(priv.pmu_cr))
Cleanup path for most of the resources here and below is kind of
missing. Also, maybe give names to the different areas and not depend
on ordering?
Sebastian
> + return PTR_ERR(priv.pmu_cr);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> + priv.pmu_clk_div = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(priv.pmu_clk_div))
> + return PTR_ERR(priv.pmu_clk_div);
> +
> + np = of_find_node_by_path("/cpus/cpu at 0");
> + if (!np)
> + return -ENODEV;
> +
> + priv.cpu_clk = of_clk_get_by_name(np, "cpu_clk");
> + 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);
> + dove_freq_table[0].frequency = clk_get_rate(priv.cpu_clk) / 1000;
> +
> + priv.ddr_clk = of_clk_get_by_name(np, "ddrclk");
> + 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);
> + dove_freq_table[1].frequency = clk_get_rate(priv.ddr_clk) / 1000;
> +
> + irq = irq_of_parse_and_map(np, 0);
> + if (!irq) {
> + dev_err(priv.dev, "irq_of_parse_and_map failed\n");
> + return 0;
> + }
> +
> + err = devm_request_irq(&pdev->dev, irq, dove_cpufreq_irq,
> + 0, "dove-cpufreq", NULL);
> + if (err) {
> + dev_err(&pdev->dev, "cannot assign irq %d, %d\n", irq, err);
> + return err;
> + }
> +
> + of_node_put(np);
> + np = NULL;
> +
> + /* Read the target ratio which should be the DDR ratio */
> + priv.dpratio = readl_relaxed(priv.pmu_clk_div);
> + priv.dpratio = (priv.dpratio & DPRATIO_MASK) >> DPRATIO_OFFS;
> + priv.dpratio = priv.dpratio << L2_RATIO_OFFS;
> +
> + /* Save L2 ratio at reset */
> + priv.xpratio = readl(priv.pmu_clk_div);
> + priv.xpratio = (priv.xpratio & XPRATIO_MASK) >> XPRATIO_OFFS;
> + priv.xpratio = priv.xpratio << L2_RATIO_OFFS;
> +
> + err = cpufreq_register_driver(&dove_cpufreq_driver);
> + if (!err)
> + return 0;
> +
> + dev_err(priv.dev, "Failed to register cpufreq driver");
> +
> + clk_disable_unprepare(priv.ddr_clk);
> +out_cpu:
> + clk_disable_unprepare(priv.cpu_clk);
> + of_node_put(np);
> +
> + return err;
> +}
> +
> +static int dove_cpufreq_remove(struct platform_device *pdev)
> +{
> + cpufreq_unregister_driver(&dove_cpufreq_driver);
> +
> + clk_disable_unprepare(priv.ddr_clk);
> + clk_disable_unprepare(priv.cpu_clk);
> +
> + return 0;
> +}
> +
> +static struct platform_driver dove_cpufreq_platform_driver = {
> + .probe = dove_cpufreq_probe,
> + .remove = dove_cpufreq_remove,
> + .driver = {
> + .name = "dove-cpufreq",
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +module_platform_driver(dove_cpufreq_platform_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Andrew Lunn <andrew@lunn.ch");
> +MODULE_DESCRIPTION("cpufreq driver for Marvell's dove CPU");
> +MODULE_ALIAS("platform:dove-cpufreq");
>
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
2013-10-21 10:42 ` Sebastian Hesselbarth
@ 2013-10-21 15:42 ` Andrew Lunn
2013-10-21 16:38 ` Sebastian Hesselbarth
0 siblings, 1 reply; 16+ messages in thread
From: Andrew Lunn @ 2013-10-21 15:42 UTC (permalink / raw)
To: linux-arm-kernel
> >+ if (freqs.old != freqs.new) {
> >+ local_irq_disable();
> >+
> >+ /* Mask IRQ and FIQ to CPU */
> >+ cr = readl(priv.pmu_cr);
> >+ cr |= MASK_IRQ | MASK_FIQ;
> >+ writel(cr, priv.pmu_cr);
> >+
> >+ /* Set/Clear the CPU_SLOW_EN bit */
> >+ reg = readl_relaxed(priv.dfs + DFS_CR);
> >+ reg &= ~L2_RATIO_MASK;
> >+
> >+ switch (state) {
> >+ case STATE_CPU_FREQ:
> >+ reg |= priv.xpratio;
> >+ reg &= ~CPU_SLOW_EN;
> >+ break;
> >+ case STATE_DDR_FREQ:
> >+ reg |= (priv.dpratio | CPU_SLOW_EN);
>
> Does slow mode really (only) depend on the value written into CPUL2CR?
> Dove FS isn't that chatty about it and I cannot really test right now.
> But reading it, I think there is two CPU PLL to {L2,DDR} ratios that you
> can switch between by (de-)asserting CPU_SLOW_EN.
Hi Sebastian
The lack of details in the datasheet was very annoying. I spent many
evenings looking at a frozen up cubox. In the end i had to risk eye
cancer and look at the Marvell vendor code. It always sets these
values on each transition, and it even calculated one of the ratios
each time the frequency changed. I never actually tried setting them
once and then leaving them alone. Worth a try once i have the hardware
available.
> >+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >+ priv.dfs = devm_ioremap_resource(&pdev->dev, res);
> >+ if (IS_ERR(priv.dfs))
> >+ return PTR_ERR(priv.dfs);
> >+
> >+ res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> >+ priv.pmu_cr = devm_ioremap_resource(&pdev->dev, res);
> >+ if (IS_ERR(priv.pmu_cr))
>
> Cleanup path for most of the resources here and below is kind of
> missing.
Well, i'm using devm_ioremap_resources(). The cleanup for that should
happen automagically. And platform_get_resource() is just a lookup
function.
> Also, maybe give names to the different areas and not depend
> on ordering?
Actually, they already have names, but you probably have not go to
that patch yet. So swapping to platform_get_resource_byname() would be
simple.
Thanks
Andrew
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
2013-10-21 15:42 ` Andrew Lunn
@ 2013-10-21 16:38 ` Sebastian Hesselbarth
0 siblings, 0 replies; 16+ messages in thread
From: Sebastian Hesselbarth @ 2013-10-21 16:38 UTC (permalink / raw)
To: linux-arm-kernel
On 10/21/2013 04:42 PM, Andrew Lunn wrote:
>>> + if (freqs.old != freqs.new) {
>>> + local_irq_disable();
>>> +
>>> + /* Mask IRQ and FIQ to CPU */
>>> + cr = readl(priv.pmu_cr);
>>> + cr |= MASK_IRQ | MASK_FIQ;
>>> + writel(cr, priv.pmu_cr);
>>> +
>>> + /* Set/Clear the CPU_SLOW_EN bit */
>>> + reg = readl_relaxed(priv.dfs + DFS_CR);
>>> + reg &= ~L2_RATIO_MASK;
>>> +
>>> + switch (state) {
>>> + case STATE_CPU_FREQ:
>>> + reg |= priv.xpratio;
>>> + reg &= ~CPU_SLOW_EN;
>>> + break;
>>> + case STATE_DDR_FREQ:
>>> + reg |= (priv.dpratio | CPU_SLOW_EN);
>>
>> Does slow mode really (only) depend on the value written into CPUL2CR?
>> Dove FS isn't that chatty about it and I cannot really test right now.
>> But reading it, I think there is two CPU PLL to {L2,DDR} ratios that you
>> can switch between by (de-)asserting CPU_SLOW_EN.
>
> The lack of details in the datasheet was very annoying. I spent many
> evenings looking at a frozen up cubox. In the end i had to risk eye
> cancer and look at the Marvell vendor code. It always sets these
> values on each transition, and it even calculated one of the ratios
> each time the frequency changed. I never actually tried setting them
> once and then leaving them alone. Worth a try once i have the hardware
> available.
I actually just tried it and realized that both xpratio and dpratio are
equal (=4). And I wonder that not setting l2 ratio hangs it, while not
setting ddr ratio is fine. Anyway, without proper documentation IMHO
your approach is as fine as it can get. We can have a closer look at
it, when we both have time/tools/space available.
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> + priv.dfs = devm_ioremap_resource(&pdev->dev, res);
>>> + if (IS_ERR(priv.dfs))
>>> + return PTR_ERR(priv.dfs);
>>> +
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>> + priv.pmu_cr = devm_ioremap_resource(&pdev->dev, res);
>>> + if (IS_ERR(priv.pmu_cr))
>>
>> Cleanup path for most of the resources here and below is kind of
>> missing.
>
> Well, i'm using devm_ioremap_resources(). The cleanup for that should
> happen automagically. And platform_get_resource() is just a lookup
> function.
True. But e.g. on the clock errors, you do not drop node or clk. No
clk_disable_unprepare on irq failures or if cpufreq_register_driver
fails.
>> Also, maybe give names to the different areas and not depend
>> on ordering?
>
> Actually, they already have names, but you probably have not go to
> that patch yet. So swapping to platform_get_resource_byname() would be
> simple.
I did flip over the non-DT resources and now (finally) realized, that
there is no specific DT node (You told me at least twice before).
Currently, I don't see why cpufreq shouldn't get its own node as this
adds dependency to legacy code we are trying to get rid of. But as you
already said, let's wait for summit results on DT future.
BTW, have another look at the resource names and remove the ':' (or
add it to all).
See you soon,
Sebastian
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
2013-10-19 12:37 ` [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove Andrew Lunn
2013-10-19 23:09 ` Luka Perkov
2013-10-21 10:42 ` Sebastian Hesselbarth
@ 2013-10-22 9:01 ` Viresh Kumar
2013-10-22 15:57 ` Andrew Lunn
2013-10-22 10:19 ` Sudeep KarkadaNagesha
3 siblings, 1 reply; 16+ messages in thread
From: Viresh Kumar @ 2013-10-22 9:01 UTC (permalink / raw)
To: linux-arm-kernel
On 19 October 2013 18:07, Andrew Lunn <andrew@lunn.ch> wrote:
> +config ARM_DOVE_CPUFREQ
> + def_bool ARCH_DOVE && OF
> + select CPU_FREQ_TABLE
Refer:
3bc28ab cpufreq: remove CONFIG_CPU_FREQ_TABLE
> + help
> + This adds the CPUFreq driver for Marvell Dove
> + SoCs.
Above can come in one line?
> +
> config ARM_DT_BL_CPUFREQ
> tristate "Generic probing via DT for ARM big LITTLE CPUfreq driver"
> depends on ARM_BIG_LITTLE_CPUFREQ && OF
> diff --git a/drivers/cpufreq/dove-cpufreq.c b/drivers/cpufreq/dove-cpufreq.c
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
why do you need this one?
> +#include <linux/cpufreq.h>
> +#include <linux/interrupt.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/of_irq.h>
Can you add them in ascending order please?
> +#include <asm/proc-fns.h>
> +
> +#define DFS_CR 0x00
> +#define DFS_EN BIT(0)
> +#define CPU_SLOW_EN BIT(1)
> +#define L2_RATIO_OFFS 9
> +#define L2_RATIO_MASK (0x3F << L2_RATIO_OFFS)
> +#define DFS_SR 0x04
> +#define CPU_SLOW_MODE_STTS BIT(1)
> +
> +/* PMU_CR */
> +#define MASK_FIQ BIT(28)
> +#define MASK_IRQ BIT(24) /* PMU_CR */
Use space after #define instead of tabs. And then use tabs
consistently after Macro name to keep macro values aligned.
> +/* CPU Clock Divider Control 0 Register */
> +#define DPRATIO_OFFS 24
> +#define DPRATIO_MASK (0x3F << DPRATIO_OFFS)
> +#define XPRATIO_OFFS 16
> +#define XPRATIO_MASK (0x3F << XPRATIO_OFFS)
Here as well.. aligning with earlier macro's might look better.
> +static struct priv
> +{
> + struct clk *cpu_clk;
> + struct clk *ddr_clk;
> + struct device *dev;
> + unsigned long dpratio;
> + unsigned long xpratio;
> + void __iomem *dfs;
> + void __iomem *pmu_cr;
> + void __iomem *pmu_clk_div;
> +} priv;
> +
> +#define STATE_CPU_FREQ 0x01
> +#define STATE_DDR_FREQ 0x02
> +
> +/*
> + * Dove 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 dove_freq_table[] = {
> + {STATE_CPU_FREQ, 0}, /* CPU uses cpuclk */
> + {STATE_DDR_FREQ, 0}, /* CPU uses ddrclk */
> + {0, CPUFREQ_TABLE_END},
> +};
> +
> +static unsigned int dove_cpufreq_get_cpu_frequency(unsigned int cpu)
> +{
> + unsigned long reg = readl_relaxed(priv.dfs + DFS_SR);
> +
> + if (reg & CPU_SLOW_MODE_STTS)
> + return dove_freq_table[1].frequency;
> + return dove_freq_table[0].frequency;
> +}
> +
> +static void dove_cpufreq_set_cpu_state(struct cpufreq_policy *policy,
> + unsigned int index)
> +{
> + struct cpufreq_freqs freqs;
> + unsigned int state = dove_freq_table[index].driver_data;
> + unsigned long reg, cr;
> +
> + freqs.old = dove_cpufreq_get_cpu_frequency(0);
> + freqs.new = dove_freq_table[index].frequency;
> +
> + cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE);
> +
> + if (freqs.old != freqs.new) {
If this is false, why should we start sending notifications?
> + local_irq_disable();
> +
> + /* Mask IRQ and FIQ to CPU */
> + cr = readl(priv.pmu_cr);
> + cr |= MASK_IRQ | MASK_FIQ;
> + writel(cr, priv.pmu_cr);
> +
> + /* Set/Clear the CPU_SLOW_EN bit */
> + reg = readl_relaxed(priv.dfs + DFS_CR);
> + reg &= ~L2_RATIO_MASK;
> +
> + switch (state) {
> + case STATE_CPU_FREQ:
> + reg |= priv.xpratio;
> + reg &= ~CPU_SLOW_EN;
> + break;
> + case STATE_DDR_FREQ:
> + reg |= (priv.dpratio | CPU_SLOW_EN);
> + break;
> + }
> +
> + /* Start the DFS process */
> + reg |= DFS_EN;
> +
> + writel(reg, priv.dfs + DFS_CR);
> +
> + /* Wait-for-Interrupt, while the hardware changes frequency */
> + cpu_do_idle();
> +
> + local_irq_enable();
> + }
> + cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
> +}
> +
> +static int dove_cpufreq_target(struct cpufreq_policy *policy,
> + unsigned int target_freq,
> + unsigned int relation)
there are few patches floating in making target light weight.. This part will
change significantly ones those are in..
> +{
> + unsigned int index = 0;
> +
> + if (cpufreq_frequency_table_target(policy, dove_freq_table,
> + target_freq, relation, &index))
> + return -EINVAL;
> +
> + dove_cpufreq_set_cpu_state(policy, index);
> +
> + return 0;
> +}
> +
> +static int dove_cpufreq_cpu_init(struct cpufreq_policy *policy)
> +{
> + return cpufreq_generic_init(policy, dove_freq_table, 5000);
> +}
> +
> +static irqreturn_t dove_cpufreq_irq(int irq, void *dev)
> +{
> + return IRQ_HANDLED;
> +}
what is this for?
> +static struct cpufreq_driver dove_cpufreq_driver = {
> + .get = dove_cpufreq_get_cpu_frequency,
> + .verify = cpufreq_generic_frequency_table_verify,
> + .target = dove_cpufreq_target,
> + .init = dove_cpufreq_cpu_init,
> + .exit = cpufreq_generic_exit,
> + .name = "dove-cpufreq",
> + .attr = cpufreq_generic_attr,
> +};
> +
> +static int dove_cpufreq_probe(struct platform_device *pdev)
> +{
> + struct device_node *np;
> + struct resource *res;
> + int err;
> + int irq;
Above two can be merged.
> + priv.dev = &pdev->dev;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + priv.dfs = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(priv.dfs))
> + return PTR_ERR(priv.dfs);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + priv.pmu_cr = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(priv.pmu_cr))
> + return PTR_ERR(priv.pmu_cr);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> + priv.pmu_clk_div = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(priv.pmu_clk_div))
> + return PTR_ERR(priv.pmu_clk_div);
> +
> + np = of_find_node_by_path("/cpus/cpu at 0");
> + if (!np)
> + return -ENODEV;
> +
> + priv.cpu_clk = of_clk_get_by_name(np, "cpu_clk");
> + 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);
this can fail..
> + dove_freq_table[0].frequency = clk_get_rate(priv.cpu_clk) / 1000;
> +
> + priv.ddr_clk = of_clk_get_by_name(np, "ddrclk");
> + 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);
and so can this..
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
2013-10-22 9:01 ` Viresh Kumar
@ 2013-10-22 15:57 ` Andrew Lunn
2013-10-23 4:29 ` Viresh Kumar
0 siblings, 1 reply; 16+ messages in thread
From: Andrew Lunn @ 2013-10-22 15:57 UTC (permalink / raw)
To: linux-arm-kernel
Hi Viresh
Thanks for your comments. I will include them in the next version.
> > +static irqreturn_t dove_cpufreq_irq(int irq, void *dev)
> > +{
> > + return IRQ_HANDLED;
> > +}
>
> what is this for?
The hardware raises an interrupt when the change is complete. This
interrupt is what brings the CPU out of the WFI. I don't know the
interrupt code too well, but i think it is necessary for the driver to
acknowledged it. The interrupt core code counts interrupts which
nobody acknowledges, and after 1000 are received, it disables the
interrupt. That would probably result in the machine locking solid,
never coming out of the WFI. So i'm playing it safe and handling the
interrupt.
> > +static int dove_cpufreq_probe(struct platform_device *pdev)
> > +{
> > + struct device_node *np;
> > + struct resource *res;
> > + int err;
> > + int irq;
>
> Above two can be merged.
Well, i was told the exact opposite by the USB serial maintainer :-)
I will use your coding style here, and his for USB code, and try to
keep everybody happy.
> > + priv.dev = &pdev->dev;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + priv.dfs = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(priv.dfs))
> > + return PTR_ERR(priv.dfs);
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > + priv.pmu_cr = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(priv.pmu_cr))
> > + return PTR_ERR(priv.pmu_cr);
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> > + priv.pmu_clk_div = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(priv.pmu_clk_div))
> > + return PTR_ERR(priv.pmu_clk_div);
> > +
> > + np = of_find_node_by_path("/cpus/cpu at 0");
> > + if (!np)
> > + return -ENODEV;
> > +
> > + priv.cpu_clk = of_clk_get_by_name(np, "cpu_clk");
> > + 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);
>
> this can fail..
In theory yes. In practice no. It is a fixed rate clock. prepare and
enable are actually NOPs for this type of clock. However the API
documentation explicitly states you need to call prepare and enable
before you can call clk_get_rate(). But i can add error checking if
you want.
>
> > + dove_freq_table[0].frequency = clk_get_rate(priv.cpu_clk) / 1000;
Andrew
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
2013-10-22 15:57 ` Andrew Lunn
@ 2013-10-23 4:29 ` Viresh Kumar
0 siblings, 0 replies; 16+ messages in thread
From: Viresh Kumar @ 2013-10-23 4:29 UTC (permalink / raw)
To: linux-arm-kernel
On 22 October 2013 21:27, Andrew Lunn <andrew@lunn.ch> wrote:
> The hardware raises an interrupt when the change is complete. This
> interrupt is what brings the CPU out of the WFI. I don't know the
> interrupt code too well, but i think it is necessary for the driver to
> acknowledged it. The interrupt core code counts interrupts which
> nobody acknowledges, and after 1000 are received, it disables the
> interrupt. That would probably result in the machine locking solid,
> never coming out of the WFI. So i'm playing it safe and handling the
> interrupt.
Theory looks correct AFAIK.. But it would be better to give it a try
on real hardware, hopefully you have that :)
Also, in case this is required, it would be better to document it a bit
over the routine..
> In theory yes. In practice no. It is a fixed rate clock. prepare and
> enable are actually NOPs for this type of clock. However the API
> documentation explicitly states you need to call prepare and enable
> before you can call clk_get_rate(). But i can add error checking if
> you want.
Leave it then..
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
2013-10-19 12:37 ` [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove Andrew Lunn
` (2 preceding siblings ...)
2013-10-22 9:01 ` Viresh Kumar
@ 2013-10-22 10:19 ` Sudeep KarkadaNagesha
3 siblings, 0 replies; 16+ messages in thread
From: Sudeep KarkadaNagesha @ 2013-10-22 10:19 UTC (permalink / raw)
To: linux-arm-kernel
On 19/10/13 13:37, Andrew Lunn wrote:
> The Marvell Dove SoC can run the CPU at two frequencies. The high
> frequencey is from a PLL, while the lower is the same as the DDR
> clock. Add a cpufreq driver to swap between these frequences.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
> drivers/cpufreq/Kconfig.arm | 7 ++
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/dove-cpufreq.c | 276 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 284 insertions(+)
> create mode 100644 drivers/cpufreq/dove-cpufreq.c
>
[snip]
> +static int dove_cpufreq_probe(struct platform_device *pdev)
> +{
> + struct device_node *np;
> + struct resource *res;
> + int err;
> + int irq;
> +
> + priv.dev = &pdev->dev;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + priv.dfs = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(priv.dfs))
> + return PTR_ERR(priv.dfs);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + priv.pmu_cr = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(priv.pmu_cr))
> + return PTR_ERR(priv.pmu_cr);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> + priv.pmu_clk_div = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(priv.pmu_clk_div))
> + return PTR_ERR(priv.pmu_clk_div);
> +
> + np = of_find_node_by_path("/cpus/cpu at 0");
> + if (!np)
> + return -ENODEV;
> +
You need not search for cpu nodes. When the cpu are registered, their of_node
gets initialised. So you can just use:
np = of_cpu_device_node_get(0);
However I think its better to just get:
cpu_dev = get_cpu_device(0);
as clk APIs can use cpu_dev
> + priv.cpu_clk = of_clk_get_by_name(np, "cpu_clk");
Now this can be
priv.cpu_clk = devm_clk_get(cpu_dev, "cpu_clk");
> + 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);
> + dove_freq_table[0].frequency = clk_get_rate(priv.cpu_clk) / 1000;
> +
> + priv.ddr_clk = of_clk_get_by_name(np, "ddrclk");
Similarly priv.ddr_clk = devm_clk_get(cpu_dev, "ddrclk");
> + 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);
> + dove_freq_table[1].frequency = clk_get_rate(priv.ddr_clk) / 1000;
> +
> + irq = irq_of_parse_and_map(np, 0);
Here you can use cpu_dev->of_node
Regards,
Sudeep
^ permalink raw reply [flat|nested] 16+ messages in thread