| Message ID | 1386166641-7567-2-git-send-email-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 04, 2013 at 03:17:18PM +0100, 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> > Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > Sort header files > Remove unneeded header files > Notify only on change > Use get_cpu_device(0), devm_clk_get() > Use platform_get_resource_byname() > Error check clk_prepare_enable() > Comment the interrupt handler > > rebase onto 3.13-rc2 linux-next Is there something you need that prevents you from basing on v3.13-rc1? > use target_index > --- > drivers/cpufreq/Kconfig.arm | 5 + > drivers/cpufreq/Makefile | 1 + > drivers/cpufreq/dove-cpufreq.c | 259 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 265 insertions(+) > create mode 100644 drivers/cpufreq/dove-cpufreq.c Other than that, Acked-by: Jason Cooper <jason@lakedaemon.net> thx, Jason.
On Wed, Dec 04, 2013 at 02:17:18PM +0000, 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> > Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > Sort header files > Remove unneeded header files > Notify only on change > Use get_cpu_device(0), devm_clk_get() > Use platform_get_resource_byname() > Error check clk_prepare_enable() > Comment the interrupt handler > > rebase onto 3.13-rc2 linux-next > use target_index > --- > drivers/cpufreq/Kconfig.arm | 5 + > drivers/cpufreq/Makefile | 1 + > drivers/cpufreq/dove-cpufreq.c | 259 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 265 insertions(+) > create mode 100644 drivers/cpufreq/dove-cpufreq.c [...] > +static int dove_cpufreq_target(struct cpufreq_policy *policy, > + unsigned int index) > +{ > + unsigned int state = dove_freq_table[index].driver_data; > + unsigned long reg, cr; > + > + 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(); Just to check -- does the PMU automatically unmask IRQ and FIQ? [...] > +static int dove_cpufreq_probe(struct platform_device *pdev) > +{ > + struct device *cpu_dev; > + struct resource *res; > + int err, irq; > + > + memset(&priv, 0, sizeof(priv)); > + priv.dev = &pdev->dev; > + > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > + "cpufreq: DFS"); > + priv.dfs = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(priv.dfs)) > + return PTR_ERR(priv.dfs); > + > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > + "cpufreq: PMU CR"); > + 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_byname(pdev, IORESOURCE_MEM, > + "cpufreq: PMU Clk Div"); > + priv.pmu_clk_div = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(priv.pmu_clk_div)) > + return PTR_ERR(priv.pmu_clk_div); I'd very much prefer that the PMU were described in the DT, and the driver probed based on that. > + > + cpu_dev = get_cpu_device(0); > + > + priv.cpu_clk = devm_clk_get(cpu_dev, "cpu_clk"); > + if (IS_ERR(priv.cpu_clk)) { > + err = PTR_ERR(priv.cpu_clk); > + goto out; > + } I think it would be better to describe the clocks as being fed to the PMU than directly to the CPU -- the PMU selects the appropriate clock and feeds it to the CPU. Also, the clocks could be named better with respect to their logical function. One is the high-frequency default, and the other is a low-frequency option. They're both able to feed the CPU, and the fact that one also feeds the DDR isn't relevant to the CPU. Does the PMu also control the input to the DDR? > + > + err = clk_prepare_enable(priv.cpu_clk); > + if (err) > + goto out; > + > + dove_freq_table[0].frequency = clk_get_rate(priv.cpu_clk) / 1000; > + > + priv.ddr_clk = devm_clk_get(cpu_dev, "ddrclk"); > + if (IS_ERR(priv.ddr_clk)) { > + err = PTR_ERR(priv.ddr_clk); > + goto out; > + } > + > + err = clk_prepare_enable(priv.ddr_clk); > + if (err) > + goto out; > + > + dove_freq_table[1].frequency = clk_get_rate(priv.ddr_clk) / 1000; > + > + irq = irq_of_parse_and_map(cpu_dev->of_node, 0); > + if (!irq) { > + err = -ENXIO; > + goto out; > + } Similarly, the interrupt is a property of the PMU. Thanks, Mark.
On Wed, Dec 04, 2013 at 09:33:17AM -0500, Jason Cooper wrote: > On Wed, Dec 04, 2013 at 03:17:18PM +0100, 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> > > Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> > > --- > > Sort header files > > Remove unneeded header files > > Notify only on change > > Use get_cpu_device(0), devm_clk_get() > > Use platform_get_resource_byname() > > Error check clk_prepare_enable() > > Comment the interrupt handler > > > > rebase onto 3.13-rc2 linux-next > > Is there something you need that prevents you from basing on v3.13-rc1? It seems to be Rafael's policy that patches for pm need to be based on his linux-next branch. Viresh has also been making some major changes to all the cpufreq drivers to move code into the core and out of drivers. These changes are in rafaels linux-next, and i've made similar changes to the driver so it fits in. Andrew
On Wed, Dec 04, 2013 at 03:56:35PM +0100, Andrew Lunn wrote: > On Wed, Dec 04, 2013 at 09:33:17AM -0500, Jason Cooper wrote: > > On Wed, Dec 04, 2013 at 03:17:18PM +0100, 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> > > > Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> > > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> > > > --- > > > Sort header files > > > Remove unneeded header files > > > Notify only on change > > > Use get_cpu_device(0), devm_clk_get() > > > Use platform_get_resource_byname() > > > Error check clk_prepare_enable() > > > Comment the interrupt handler > > > > > > rebase onto 3.13-rc2 linux-next > > > > Is there something you need that prevents you from basing on v3.13-rc1? > > It seems to be Rafael's policy that patches for pm need to be based on > his linux-next branch. Viresh has also been making some major changes > to all the cpufreq drivers to move code into the core and out of > drivers. These changes are in rafaels linux-next, and i've made > similar changes to the driver so it fits in. Ah, ok. thx, Jason.
> > +static int dove_cpufreq_target(struct cpufreq_policy *policy, > > + unsigned int index) > > +{ > > + unsigned int state = dove_freq_table[index].driver_data; > > + unsigned long reg, cr; > > + > > + 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(); > > Just to check -- does the PMU automatically unmask IRQ and FIQ? The data sheet says so, yes. > > +static int dove_cpufreq_probe(struct platform_device *pdev) > > +{ > > + struct device *cpu_dev; > > + struct resource *res; > > + int err, irq; > > + > > + memset(&priv, 0, sizeof(priv)); > > + priv.dev = &pdev->dev; > > + > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > > + "cpufreq: DFS"); > > + priv.dfs = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(priv.dfs)) > > + return PTR_ERR(priv.dfs); > > + > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > > + "cpufreq: PMU CR"); > > + 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_byname(pdev, IORESOURCE_MEM, > > + "cpufreq: PMU Clk Div"); > > + priv.pmu_clk_div = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(priv.pmu_clk_div)) > > + return PTR_ERR(priv.pmu_clk_div); > > I'd very much prefer that the PMU were described in the DT, and the > driver probed based on that. Please see my other response. We can discuss that there. > > + > > + cpu_dev = get_cpu_device(0); > > + > > + priv.cpu_clk = devm_clk_get(cpu_dev, "cpu_clk"); > > + if (IS_ERR(priv.cpu_clk)) { > > + err = PTR_ERR(priv.cpu_clk); > > + goto out; > > + } > > I think it would be better to describe the clocks as being fed to the > PMU than directly to the CPU -- the PMU selects the appropriate clock > and feeds it to the CPU. The wording in the data sheet is not very clear: The PMU initiates new clock ratio values for the CPU subsystem clocks generation unit. So to me, the PMU is not the clock consumer, it just controls the clock generation unit. There is also a block diagram which shows the "CPU Subsystem ClockGen" as being external to the PMU. > Also, the clocks could be named better with respect to their logical > function. One is the high-frequency default, and the other is a > low-frequency option. They're both able to feed the CPU, and the fact > that one also feeds the DDR isn't relevant to the CPU. The data sheet uses the terms Turbo Speed and Slow Speed modes. So i could call them turbo and slow? However, the current names follow the kirkwood cpufreq driver. It also can swap between a fast clock and the DDR clock. So i thought keeping the drivers similar would help with the maintenance burden. Andrew
On Wed, Dec 04, 2013 at 03:27:26PM +0000, Andrew Lunn wrote: [...] > > > + > > > + cpu_dev = get_cpu_device(0); > > > + > > > + priv.cpu_clk = devm_clk_get(cpu_dev, "cpu_clk"); > > > + if (IS_ERR(priv.cpu_clk)) { > > > + err = PTR_ERR(priv.cpu_clk); > > > + goto out; > > > + } > > > > I think it would be better to describe the clocks as being fed to the > > PMU than directly to the CPU -- the PMU selects the appropriate clock > > and feeds it to the CPU. > > The wording in the data sheet is not very clear: > > The PMU initiates new clock ratio values for the CPU subsystem > clocks generation unit. > > So to me, the PMU is not the clock consumer, it just controls the > clock generation unit. There is also a block diagram which shows the > "CPU Subsystem ClockGen" as being external to the PMU. Hmm. That's somewhat arguable. From a block diagram I've found, the situation seems more like the ClockGen takes two clocks as input, and feeds the appropriate clock to the CPU. Logically, the CPU itself is a consumer of one clock. The ClockGen unit is controlled by a clock manager. The clock manager appears to be a component of the PMU itself, but I may have misunderstood. If that is the case, and the CPU Subsytem ClockGen is not controlled by anything else, then I don't see the problem with grouping that together logically with the clock manager (and therefore the PMU). That would imply describing the clock inputs on the PMU is fine. Have I missed something? One thing I'm worried about is adding lots of SoC-specific variations on data to generic nodes rather than doing that in a uniform fashion. I believe describing the clocks in the way you propose clashes with the cpufreq-cpu0 way of doing things (whereby the CPU has one clock input). If the ClockGen were modelled as a mux that allows switching between two rates, then that would make the description more uniform. > > > Also, the clocks could be named better with respect to their logical > > function. One is the high-frequency default, and the other is a > > low-frequency option. They're both able to feed the CPU, and the fact > > that one also feeds the DDR isn't relevant to the CPU. > > The data sheet uses the terms Turbo Speed and Slow Speed modes. So i > could call them turbo and slow? However, the current names follow the > kirkwood cpufreq driver. It also can swap between a fast clock and the > DDR clock. So i thought keeping the drivers similar would help with > the maintenance burden. I think given the above, the changes I'm asking for would already involve sufficient changes to make this concern irrelevant. However, I don't want to force an unmaintainable change. Mark.
On 4 December 2013 19:47, Andrew Lunn <andrew@lunn.ch> 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> > Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> I was sure about it and checked it again. I haven't Acked this patch until now. :) > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm > index ce52ed949249..af7c4947f218 100644 > --- a/drivers/cpufreq/Kconfig.arm > +++ b/drivers/cpufreq/Kconfig.arm > @@ -8,6 +8,11 @@ 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 > + help > + This adds the CPUFreq driver for Marvell Dove SoCs. Please add this one after DT_BL one.. I know you wanted to keep them in alphabetical order (probably we need to rename DT_BL), but keeping both BIG LITTLE options makes more sense. I will do the renaming later. > 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 > +static int dove_cpufreq_probe(struct platform_device *pdev) > +{ > + struct device *cpu_dev; > + struct resource *res; > + int err, irq; > + > + memset(&priv, 0, sizeof(priv)); > + priv.dev = &pdev->dev; > + > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > + "cpufreq: DFS"); > + priv.dfs = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(priv.dfs)) > + return PTR_ERR(priv.dfs); > + > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > + "cpufreq: PMU CR"); > + 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_byname(pdev, IORESOURCE_MEM, > + "cpufreq: PMU Clk Div"); > + priv.pmu_clk_div = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(priv.pmu_clk_div)) > + return PTR_ERR(priv.pmu_clk_div); > + > + cpu_dev = get_cpu_device(0); > + > + priv.cpu_clk = devm_clk_get(cpu_dev, "cpu_clk"); > + if (IS_ERR(priv.cpu_clk)) { > + err = PTR_ERR(priv.cpu_clk); > + goto out; > + } You are doing clk_disable_unprepare in out, and you haven't enabled them yet. So, just return error from here. > + err = clk_prepare_enable(priv.cpu_clk); > + if (err) > + goto out; same here > + dove_freq_table[0].frequency = clk_get_rate(priv.cpu_clk) / 1000; > + > + priv.ddr_clk = devm_clk_get(cpu_dev, "ddrclk"); > + if (IS_ERR(priv.ddr_clk)) { > + err = PTR_ERR(priv.ddr_clk); > + goto out; You need to do unprepare only for cpu clk here. > + } > + > + err = clk_prepare_enable(priv.ddr_clk); > + if (err) > + goto out; same here > + > + dove_freq_table[1].frequency = clk_get_rate(priv.ddr_clk) / 1000; Because you are using 0 and 1 in index for CPU and DDR freq at multiple places, probably makes sense to make Macros for them ? > + > + irq = irq_of_parse_and_map(cpu_dev->of_node, 0); > + if (!irq) { > + err = -ENXIO; > + goto out; > + } > + > + 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); > + goto out; > + } > + > + /* 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"); > + > +out: > + clk_disable_unprepare(priv.ddr_clk); > + clk_disable_unprepare(priv.cpu_clk); > + > + 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); Otherwise looks fine :)
This is an RFC where i attempt to address Mark Rutland's request for a PMU node in DT, and all clocks and interrupts required by the cpufreq driver are within this node. I would like to receive comments about this binding. This version does not address any other comments made to the code. I will fix those issues once the binding is mostly decided. Thanks Andrew Andrew Lunn (4): cpufreq: Add a cpufreq driver for Marvell Dove mvebu: Dove: Indicate the architecture supports cpufreq. mvebu: Dove: Enable cpufreq driver in defconfig mvebu: Dove: Add PMU node for cpufreq driver .../devicetree/bindings/cpufreq/cpufreq-dove.txt | 23 ++ arch/arm/Kconfig | 1 + arch/arm/boot/dts/dove.dtsi | 10 + arch/arm/configs/dove_defconfig | 2 + drivers/cpufreq/Kconfig.arm | 5 + drivers/cpufreq/Makefile | 1 + drivers/cpufreq/dove-cpufreq.c | 265 +++++++++++++++++++++ 7 files changed, 307 insertions(+) create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-dove.txt create mode 100644 drivers/cpufreq/dove-cpufreq.c
> > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm > > index ce52ed949249..af7c4947f218 100644 > > --- a/drivers/cpufreq/Kconfig.arm > > +++ b/drivers/cpufreq/Kconfig.arm > > @@ -8,6 +8,11 @@ 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 > > + help > > + This adds the CPUFreq driver for Marvell Dove SoCs. > > Please add this one after DT_BL one.. I know you wanted to keep > them in alphabetical order (probably we need to rename DT_BL), > but keeping both BIG LITTLE options makes more sense. > > I will do the renaming later. O.K, once the binding is agreed, i will make this change. > > 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 > > +static int dove_cpufreq_probe(struct platform_device *pdev) > > +{ > > + struct device *cpu_dev; > > + struct resource *res; > > + int err, irq; > > + > > + memset(&priv, 0, sizeof(priv)); > > + priv.dev = &pdev->dev; > > + > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > > + "cpufreq: DFS"); > > + priv.dfs = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(priv.dfs)) > > + return PTR_ERR(priv.dfs); > > + > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > > + "cpufreq: PMU CR"); > > + 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_byname(pdev, IORESOURCE_MEM, > > + "cpufreq: PMU Clk Div"); > > + priv.pmu_clk_div = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(priv.pmu_clk_div)) > > + return PTR_ERR(priv.pmu_clk_div); > > + > > + cpu_dev = get_cpu_device(0); > > + > > + priv.cpu_clk = devm_clk_get(cpu_dev, "cpu_clk"); > > + if (IS_ERR(priv.cpu_clk)) { > > + err = PTR_ERR(priv.cpu_clk); > > + goto out; > > + } > > You are doing clk_disable_unprepare in out, and you haven't enabled > them yet. So, just return error from here. You are missing part of the clk API, which makes this work, and keeps the code simple. NULL is a valid clock, and enable/disable and prepare/unprepare are NOP with a NULL clock. So the code after out: should mostly do the right thing. However what i do have wrong is not checking for IS_ERR() before clk_disable_unprepare(). I will fix that. > > + > > + dove_freq_table[1].frequency = clk_get_rate(priv.ddr_clk) / 1000; > > Because you are using 0 and 1 in index for CPU and DDR freq at multiple > places, probably makes sense to make Macros for them ? O.K. Thanks for the review, Andrew
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm index ce52ed949249..af7c4947f218 100644 --- a/drivers/cpufreq/Kconfig.arm +++ b/drivers/cpufreq/Kconfig.arm @@ -8,6 +8,11 @@ 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 + 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 74945652dd7a..cd72c2bbfd44 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 000000000000..7f7711ccccbe --- /dev/null +++ b/drivers/cpufreq/dove-cpufreq.c @@ -0,0 +1,259 @@ +/* + * 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. + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/clk.h> +#include <linux/cpu.h> +#include <linux/cpufreq.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/of.h> +#include <linux/of_irq.h> +#include <linux/platform_device.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 int dove_cpufreq_target(struct cpufreq_policy *policy, + unsigned int index) +{ + unsigned int state = dove_freq_table[index].driver_data; + unsigned long reg, cr; + + 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(); + + return 0; +} + +static int dove_cpufreq_cpu_init(struct cpufreq_policy *policy) +{ + return cpufreq_generic_init(policy, dove_freq_table, 5000); +} + +/* + * Handle the interrupt raised when the frequency change is + * complete. Without having an interrupt handler, the WFI will + * exit on the next timer tick, reducing performance. + */ +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_index = 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 *cpu_dev; + struct resource *res; + int err, irq; + + memset(&priv, 0, sizeof(priv)); + priv.dev = &pdev->dev; + + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, + "cpufreq: DFS"); + priv.dfs = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(priv.dfs)) + return PTR_ERR(priv.dfs); + + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, + "cpufreq: PMU CR"); + 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_byname(pdev, IORESOURCE_MEM, + "cpufreq: PMU Clk Div"); + priv.pmu_clk_div = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(priv.pmu_clk_div)) + return PTR_ERR(priv.pmu_clk_div); + + cpu_dev = get_cpu_device(0); + + priv.cpu_clk = devm_clk_get(cpu_dev, "cpu_clk"); + if (IS_ERR(priv.cpu_clk)) { + err = PTR_ERR(priv.cpu_clk); + goto out; + } + + err = clk_prepare_enable(priv.cpu_clk); + if (err) + goto out; + + dove_freq_table[0].frequency = clk_get_rate(priv.cpu_clk) / 1000; + + priv.ddr_clk = devm_clk_get(cpu_dev, "ddrclk"); + if (IS_ERR(priv.ddr_clk)) { + err = PTR_ERR(priv.ddr_clk); + goto out; + } + + err = clk_prepare_enable(priv.ddr_clk); + if (err) + goto out; + + dove_freq_table[1].frequency = clk_get_rate(priv.ddr_clk) / 1000; + + irq = irq_of_parse_and_map(cpu_dev->of_node, 0); + if (!irq) { + err = -ENXIO; + goto out; + } + + 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); + goto out; + } + + /* 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"); + +out: + clk_disable_unprepare(priv.ddr_clk); + clk_disable_unprepare(priv.cpu_clk); + + 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");