cpufreq support for Marvell Armada XP

In the Armada XP SMP support code, we are reading the clock frequency of the booting CPU, and use that to assign the same frequency to the other CPUs, and we do this while the clocks are disabled.

However, the CPU clocks are in fact never prepared/enabled, and to support cpufreq, we now have two code paths to change the frequency of the CPU clocks in the CPU clock driver: one when the clock is enabled (dynamic frequency scaling), one when the clock is disabled (adjusting the CPU frequency before starting the CPU). In order for this to work, the CPU clocks now have to be prepared and enabled after the initial synchronization of the clock frequencies is done, so that all future rate changes of the CPU clocks will trigger a dynamic frequency scaling transition.

Signed-off-by Thomas Petazzoni <[email protected]>

---
 arch/arm/mach-mvebu/platsmp.c | 1 +
 1 file changed, 1 insertion(+)

This commit adds the necessary code in the Marvell EBU PMSU driver to support dynamic frequency scaling. In essence, what this new code does is that it:

* registers the frequency operating points supported by the CPU;

* registers a clock notifier of the CPU clocks. The notifier function

listens to the newly introduced APPLY_RATE_CHANGE event, and uses

that to finalize the frequency transition by doing the part of the procedure that involves the PMSU;

* registers a platform device for the cpufreq-generic driver, which will take care of the CPU frequency transitions.

Signed-off-by Thomas Petazzoni <[email protected]>

---
 arch/arm/mach-mvebu/pmsu.c | 162 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mvebu-pmsu.h |  20 ++++++
 2 files changed, 182 insertions(+)
 create mode 100644 include/linux/mvebu-pmsu.h

arch/arm/mach-mvebu/pmsu.c +162 -0

index 53a55c8..db7d9ab 100644
@@ -18,20 +18,26 @@
 
 #define pr_fmt(fmt) "mvebu-pmsu: " fmt
 
+#include <linux/clk.h>
 #include <linux/cpu_pm.h>
+#include <linux/delay.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/of_address.h>
+#include <linux/of_device.h>
 #include <linux/io.h>
 #include <linux/platform_device.h>
+#include <linux/pm_opp.h>
 #include <linux/smp.h>
 #include <linux/resource.h>
+#include <linux/slab.h>
 #include <asm/cacheflush.h>
 #include <asm/cp15.h>
 #include <asm/smp_plat.h>
 #include <asm/suspend.h>
 #include <asm/tlbflush.h>
 #include "common.h"
+#include "armada-370-xp.h"
 
 static void __iomem *pmsu_mp_base;
 

@@ -57,6 +63,10 @@ static void __iomem *pmsu_mp_base;
 #define PMSU_STATUS_AND_MASK_IRQ_MASK          BIT(24)
 #define PMSU_STATUS_AND_MASK_FIQ_MASK          BIT(25)
 
+#define PMSU_EVENT_STATUS_AND_MASK(cpu)     ((cpu * 0x100) + 0x120)
+#define PMSU_EVENT_STATUS_AND_MASK_DFS_DONE        BIT(1)
+#define PMSU_EVENT_STATUS_AND_MASK_DFS_DONE_MASK   BIT(17)
+
 #define PMSU_BOOT_ADDR_REDIRECT_OFFSET(cpu) ((cpu * 0x100) + 0x124)
 
 /* PMSU fabric registers */

@@ -296,3 +306,155 @@ int __init armada_370_xp_cpu_pm_init(void)
 
 arch_initcall(armada_370_xp_cpu_pm_init);
 early_initcall(armada_370_xp_pmsu_init);
+
+static void mvebu_pmsu_dfs_request_local(void *data)
+{
+       u32 reg;
+       u32 cpu = smp_processor_id();
+       unsigned long flags;
+
+       local_irq_save(flags);
+
+       /* Prepare to enter idle */
+       reg = readl(pmsu_mp_base + PMSU_STATUS_AND_MASK(cpu));
+       reg |= PMSU_STATUS_AND_MASK_CPU_IDLE_WAIT |
+              PMSU_STATUS_AND_MASK_IRQ_MASK     |
+              PMSU_STATUS_AND_MASK_FIQ_MASK;
+       writel(reg, pmsu_mp_base + PMSU_STATUS_AND_MASK(cpu));
+
+       /* Request the DFS transition */
+       reg = readl(pmsu_mp_base + PMSU_CONTROL_AND_CONFIG(cpu));
+       reg |= PMSU_CONTROL_AND_CONFIG_DFS_REQ;
+       writel(reg, pmsu_mp_base + PMSU_CONTROL_AND_CONFIG(cpu));
+
+       /* The fact of entering idle will trigger the DFS transition */
+       wfi();
+
+       /*
+        * We're back from idle, the DFS transition has completed,
+        * clear the idle wait indication.
+        */
+       reg = readl(pmsu_mp_base + PMSU_STATUS_AND_MASK(cpu));
+       reg &= ~PMSU_STATUS_AND_MASK_CPU_IDLE_WAIT;
+       writel(reg, pmsu_mp_base + PMSU_STATUS_AND_MASK(cpu));
+
+       local_irq_restore(flags);
+}
+
+int mvebu_pmsu_dfs_request(int cpu)
+{
+       unsigned long timeout;
+       int hwcpu = cpu_logical_map(cpu);
+       u32 reg;
+
+       /* Clear any previous DFS DONE event */
+       reg = readl(pmsu_mp_base + PMSU_EVENT_STATUS_AND_MASK(hwcpu));
+       reg &= ~PMSU_EVENT_STATUS_AND_MASK_DFS_DONE;
+       writel(reg, pmsu_mp_base + PMSU_EVENT_STATUS_AND_MASK(hwcpu));
+
+       /* Mask the DFS done interrupt, since we are going to poll */
+       reg = readl(pmsu_mp_base + PMSU_EVENT_STATUS_AND_MASK(hwcpu));
+       reg |= PMSU_EVENT_STATUS_AND_MASK_DFS_DONE_MASK;
+       writel(reg, pmsu_mp_base + PMSU_EVENT_STATUS_AND_MASK(hwcpu));
+
+       /* Trigger the DFS on the appropriate CPU */
+       smp_call_function_single(cpu, mvebu_pmsu_dfs_request_local,
+                                NULL, false);
+
+       /* Poll until the DFS done event is generated */
+       timeout = jiffies + HZ;
+       while (time_before(jiffies, timeout)) {
+               reg = readl(pmsu_mp_base + PMSU_EVENT_STATUS_AND_MASK(hwcpu));
+               if (reg & PMSU_EVENT_STATUS_AND_MASK_DFS_DONE)
+                       break;
+               udelay(10);
+       }
+
+       if (time_after(jiffies, timeout))
+               return -ETIME;
+
+       /* Restore the DFS mask to its original state */
+       reg = readl(pmsu_mp_base + PMSU_EVENT_STATUS_AND_MASK(hwcpu));
+       reg &= ~PMSU_EVENT_STATUS_AND_MASK_DFS_DONE_MASK;
+       writel(reg, pmsu_mp_base + PMSU_EVENT_STATUS_AND_MASK(hwcpu));
+
+       return 0;
+}
+
+static int __init armada_xp_pmsu_cpufreq_init(void)
+{
+       struct device_node *np;
+       struct resource res;
+       int ret, cpu;
+
+       if (!of_machine_is_compatible("marvell,armadaxp"))
+               return 0;
+
+       /*
+        * In order to have proper cpufreq handling, we need to ensure
+        * that the Device Tree description of the CPU clock includes
+        * the definition of the PMU DFS registers. If not, we do not
+        * register the clock notifier and the cpufreq driver. This
+        * piece of code is only for compatibility with old Device
+        * Trees.
+        */
+       np = of_find_compatible_node(NULL, NULL, "marvell,armada-xp-cpu-clock");
+       if (!np)
+               return 0;
+
+       ret = of_address_to_resource(np, 1, &res);
+       if (ret) {
+               pr_warn(FW_WARN "not enabling cpufreq, deprecated armada-xp-cpu-clock binding\n");
+               of_node_put(np);
+               return 0;
+       }
+
+       of_node_put(np);
+
+       /*
+        * For each CPU, this loop registers the operating points
+        * supported (which are the nominal CPU frequency and half of
+        * it), and registers the clock notifier that will take care
+        * of doing the PMSU part of a frequency transition.
+        */
+       for_each_possible_cpu(cpu) {
+               struct device *cpu_dev;
+               struct clk *clk;
+               int ret;
+
+               cpu_dev = get_cpu_device(cpu);
+               if (!cpu_dev) {
+                       pr_err("Cannot get CPU %d\n", cpu);
+                       continue;
+               }
+
+               clk = clk_get(cpu_dev, 0);
+               if (!clk) {
+                       pr_err("Cannot get clock for CPU %d\n", cpu);
+                       return -ENODEV;
+               }
+
+               /*
+                * In case of a failure of dev_pm_opp_add(), we don't
+                * bother with cleaning up the registered OPP (there's
+                * no function to do so), and simply cancel the
+                * registration of the cpufreq device.
+                */
+               ret = dev_pm_opp_add(cpu_dev, clk_get_rate(clk), 0);
+               if (ret) {
+                       clk_put(clk);
+                       return ret;
+               }
+
+               ret = dev_pm_opp_add(cpu_dev, clk_get_rate(clk) / 2, 0);
+               if (ret) {
+                       clk_put(clk);
+                       return ret;
+               }
+       }
+
+       platform_device_register_simple("cpufreq-generic", -1, NULL, 0);
+       return 0;
+}
+
+device_initcall(armada_xp_pmsu_cpufreq_init);

include/linux/mvebu-pmsu.h new file +20 -0

new file mode 100644
index 0000000..b918d07
@@ -0,0 +1,20 @@
+/*
+ * Copyright (C) 2012 Marvell
+ *
+ * Thomas Petazzoni <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#ifndef __MVEBU_PMSU_H__
+#define __MVEBU_PMSU_H__
+
+#ifdef CONFIG_MACH_MVEBU_V7
+int mvebu_pmsu_dfs_request(int cpu);
+#else
+static inline int mvebu_pmsu_dfs_request(int cpu) { return -ENODEV; }
+#endif
+
+#endif /* __MVEBU_PMSU_H__ */

Signature · 1 line

2.0.0

Quoted 3 lines

* registers the frequency operating points supported by the CPU;

* registers a clock notifier of the CPU clocks. The notifier function

Where does this code register a clock notifier callback?

Quoted 1 line

listens to the newly introduced APPLY_RATE_CHANGE event, and uses

I don't see APPLY_RATE_CHANGE referenced.

Dear Mike Turquette,

Quoted 9 lines · Mike Turquette

Quoted 3 lines

* registers the frequency operating points supported by the CPU;

* registers a clock notifier of the CPU clocks. The notifier function

Where does this code register a clock notifier callback?

Quoted 1 line

listens to the newly introduced APPLY_RATE_CHANGE event, and uses

I don't see APPLY_RATE_CHANGE referenced.

Yes, this is a mistake of the commit log, due to remains of the v2 of the patch series. Back in the v2, there was indeed a new clock notifier being used. But Stephen Boyd argued against that, and instead suggested to use a direct function call, which this v3 implements, as stated in the cover letter:

- As suggested by Stephen Boyd, instead of using a new clock notifier that somewhat "hides" the dependency of the clk-cpu clock driver on the PMSU, use a direct call from the clk-cpu driver to the PMSU driver.

The commit log of this commit was not adjusted consequently, and this is my fault. Jason, is it still time to change this commit log?

In order to support dynamic frequency scaling:

* the cpuclk Device Tree node needs to be updated to describe a second set of registers describing the PMU DFS registers.

* the clock-latency property of the CPUs must be filled, otherwise the ondemand and conservative cpufreq governors refuse to work. The latency is high because the cost of a frequency transition is quite high on those CPUs.

Signed-off-by Thomas Petazzoni <[email protected]>

---
 arch/arm/boot/dts/armada-xp-mv78230.dtsi | 2 ++
 arch/arm/boot/dts/armada-xp-mv78260.dtsi | 2 ++
 arch/arm/boot/dts/armada-xp-mv78460.dtsi | 4 ++++
 arch/arm/boot/dts/armada-xp.dtsi         | 2 +-
 4 files changed, 9 insertions(+), 1 deletion(-)

Now that the Armada XP supports dynamic CPU frequency scaling, it makes sense to enable the cpufreq subsystem in mvebu_v7_defconfig, as well as the cpufreq-generic driver used on Armada XP.

Signed-off-by Thomas Petazzoni <[email protected]>

---
 arch/arm/configs/mvebu_v7_defconfig | 2 ++
 1 file changed, 2 insertions(+)

The cpufreq-generic driver is going to be used by more and more platforms to support dynamic frequency scaling of the CPU. This commit enables this driver by default in multi_v7_defconfig.

Signed-off-by Thomas Petazzoni <[email protected]>

---
 arch/arm/configs/multi_v7_defconfig | 1 +
 1 file changed, 1 insertion(+)

Mike, Viresh, Rafael, Jason, Gregory, Andrew, Sebastian,

Here is the third version of the patches adding cpufreq support for the Marvell Armada XP processor.

Changes since v2
================

- As suggested by Stephen Boyd, instead of using a new clock notifier that somewhat "hides" the dependency of the clk-cpu clock driver on the PMSU, use a direct call from the clk-cpu driver to the PMSU driver.

- Add a comment that explains why the OPP are not removed on failure in the PMSU code initializing the cpufreq logic, in answer to the review from Ezequiel Garcia.

Changes since v1
================

- Rework the patch series to use the generalized cpufreq-cpu0 (renamed cpufreq-generic) driver instead of having an Armada XP specific cpufreq driver. This was suggested by Viresh Kumar. Basically, it only involved adding a "clock-latency" property in the DT, and changing the PMSU code to register the two OPPs supported by each CPU, and registering the "cpufreq-generic" platform device instead of the "armadaxp-cpufreq" one.

Jason, this patch series is based on 3.16-rc3, but it applies fine even with mvebu/fixes and mvebu/soc merged (which contain some PMSU changes), so I haven't based the patch series on those branches. To _work_, the code needs the new cpufreq-generic driver which is pending in Viresh Kumar's tree for 3.17, but there is no build dependency.

Thanks,

Thomas

Thomas Petazzoni (7):
ARM: mvebu: ensure CPU clocks are enabled
ARM: mvebu: extend PMSU code to support dynamic frequency scaling
clk: mvebu: extend clk-cpu for dynamic frequency scaling
ARM: mvebu: update Armada XP DT for dynamic frequency scaling
ARM: mvebu: allow enabling of cpufreq on Armada XP
ARM: mvebu: update mvebu_v7_defconfig with cpufreq support
ARM: configs: add cpufreq-generic in multi_v7_defconfig

 .../devicetree/bindings/clock/mvebu-cpu-clock.txt  |   5 +-
 arch/arm/boot/dts/armada-xp-mv78230.dtsi           |   2 +
 arch/arm/boot/dts/armada-xp-mv78260.dtsi           |   2 +
 arch/arm/boot/dts/armada-xp-mv78460.dtsi           |   4 +
 arch/arm/boot/dts/armada-xp.dtsi                   |   2 +-
 arch/arm/configs/multi_v7_defconfig                |   1 +
 arch/arm/configs/mvebu_v7_defconfig                |   2 +
 arch/arm/mach-mvebu/Kconfig                        |   1 +
 arch/arm/mach-mvebu/platsmp.c                      |   1 +
 arch/arm/mach-mvebu/pmsu.c                         | 162 +++++++++++++++++++++
 drivers/clk/mvebu/clk-cpu.c                        |  80 +++++++++-
 include/linux/mvebu-pmsu.h                         |  20 +++
 12 files changed, 274 insertions(+), 8 deletions(-)
 create mode 100644 include/linux/mvebu-pmsu.h

Mike, Viresh, Rafael, Jason, Gregory, Andrew, Sebastian,

Here is the third version of the patches adding cpufreq support for the Marvell Armada XP processor.

Changes since v2
================

- As suggested by Stephen Boyd, instead of using a new clock notifier that somewhat "hides" the dependency of the clk-cpu clock driver on the PMSU, use a direct call from the clk-cpu driver to the PMSU driver.

- Add a comment that explains why the OPP are not removed on failure in the PMSU code initializing the cpufreq logic, in answer to the review from Ezequiel Garcia.

Changes since v1
================

- Rework the patch series to use the generalized cpufreq-cpu0 (renamed cpufreq-generic) driver instead of having an Armada XP specific cpufreq driver. This was suggested by Viresh Kumar. Basically, it only involved adding a "clock-latency" property in the DT, and changing the PMSU code to register the two OPPs supported by each CPU, and registering the "cpufreq-generic" platform device instead of the "armadaxp-cpufreq" one.

Jason, this patch series is based on 3.16-rc3, but it applies fine even with mvebu/fixes and mvebu/soc merged (which contain some PMSU changes), so I haven't based the patch series on those branches. To _work_, the code needs the new cpufreq-generic driver which is pending in Viresh Kumar's tree for 3.17, but there is no build dependency.

Thanks,

Thomas

Thomas Petazzoni (7):
ARM: mvebu: ensure CPU clocks are enabled
ARM: mvebu: extend PMSU code to support dynamic frequency scaling
clk: mvebu: extend clk-cpu for dynamic frequency scaling
ARM: mvebu: update Armada XP DT for dynamic frequency scaling
ARM: mvebu: allow enabling of cpufreq on Armada XP
ARM: mvebu: update mvebu_v7_defconfig with cpufreq support
ARM: configs: add cpufreq-generic in multi_v7_defconfig

 .../devicetree/bindings/clock/mvebu-cpu-clock.txt  |   5 +-
 arch/arm/boot/dts/armada-xp-mv78230.dtsi           |   2 +
 arch/arm/boot/dts/armada-xp-mv78260.dtsi           |   2 +
 arch/arm/boot/dts/armada-xp-mv78460.dtsi           |   4 +
 arch/arm/boot/dts/armada-xp.dtsi                   |   2 +-
 arch/arm/configs/multi_v7_defconfig                |   1 +
 arch/arm/configs/mvebu_v7_defconfig                |   2 +
 arch/arm/mach-mvebu/Kconfig                        |   1 +
 arch/arm/mach-mvebu/platsmp.c                      |   1 +
 arch/arm/mach-mvebu/pmsu.c                         | 162 +++++++++++++++++++++
 drivers/clk/mvebu/clk-cpu.c                        |  80 +++++++++-
 include/linux/mvebu-pmsu.h                         |  20 +++
 12 files changed, 274 insertions(+), 8 deletions(-)
 create mode 100644 include/linux/mvebu-pmsu.h

Viresh, Jason,

So, what do we do with this patch series, which depends on the cpufreq-generic driver? Has there been any solution found for 3.17 ?

Jason, in any case, I'd like the following patches to be merged for 3.17, regardless of what happens with the cpufreq driver:

ARM: mvebu: ensure CPU clocks are enabled
ARM: mvebu: extend PMSU code to support dynamic frequency scaling
clk: mvebu: extend clk-cpu for dynamic frequency scaling

One patch should be split:

ARM: mvebu: update Armada XP DT for dynamic frequency scaling

-> In this patch, the addition of clock-latency is related to the
cpufreq generic DT binding, so I think we shouldn't merge that. But on the other hand, this patch also adds the new registers for the Armada XP CPU clock, which is used by "clk: mvebu: extend clk-cpu for dynamic frequency scaling".

The patch:

ARM: mvebu: allow enabling of cpufreq on Armada XP

can be dropped, since ARCH_HAS_CPUFREQ has been removed.

The other patches are defconfig changes, which are meaningless without the cpufreq-generic driver.

Jason, what do you think about me sending a new version of the patch series, which will have two clearly separated set of patches:

1/ A first set of patches that can be applied regardless of what
happens on the cpufreq driver side. Getting it merged will not bring cpufreq support, but it will add the foundations needed to support it.

2/ A second set of patches that use the cpufreq-generic driver, which
might get applied of the cpufreq maintainers find a solution in time for 3.17. If not, then I'll re-adapt them for 3.18.

What do you think?

Quoted 1 line

What do you think?

Let's wait and see what -rc1 looks like and take action based on that.

So, what do we do with this patch series, which depends on the cpufreq-generic driver? Has there been any solution found for 3.17 ?

(quoted diff) +7 -0

+static int clk_cpu_set_rate(struct clk_hw *hwclk, unsigned long rate,
+                           unsigned long parent_rate)
+{
+       if (__clk_is_enabled(hwclk->clk))
+               return clk_cpu_on_set_rate(hwclk, rate, parent_rate);
+       else
+               return clk_cpu_off_set_rate(hwclk, rate, parent_rate);

This is racy. You don't hold the clk_enable lock so it could be enable between the conditional check and executing clk_cpu_on_set_rate.

How do you ensure that secondary CPU clocks are not enabled/disabled when changing rates?

Hello,

(Not sure why this e-mail comes with me as the From:, but anyway.)

Quoted 10 lines · Thomas Petazzoni

(quoted diff) +7 -0

+static int clk_cpu_set_rate(struct clk_hw *hwclk, unsigned long rate,
+                           unsigned long parent_rate)
+{
+       if (__clk_is_enabled(hwclk->clk))
+               return clk_cpu_on_set_rate(hwclk, rate, parent_rate);
+       else
+               return clk_cpu_off_set_rate(hwclk, rate, parent_rate);

This is racy. You don't hold the clk_enable lock so it could be enable between the conditional check and executing clk_cpu_on_set_rate.

Right.

Quoted 2 lines

How do you ensure that secondary CPU clocks are not enabled/disabled when changing rates?

In practice, this currently cannot happen: we enable the secondary CPU clocks before starting the secondary CPUs, and we never ever disable or re-enable again those clocks. So with the present code, I believe there is no problem. Even when we do CPU hotplug, we don't turn off the CPU clocks, simply because they cannot be turned off: the enable/disable state is only used here as an indication so that the clock driver knows which frequency change strategy it should apply.

But you're anyway right, I'll send a followup patch adding the lock. Would that be OK for you?

Thanks,

Thomas

Signature · 3 lines

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Quoted 16 lines

Quoted 2 lines

This is racy. You don't hold the clk_enable lock so it could be enable between the conditional check and executing clk_cpu_on_set_rate.

Right.

Quoted 2 lines

How do you ensure that secondary CPU clocks are not enabled/disabled when changing rates?

In practice, this currently cannot happen: we enable the secondary CPU clocks before starting the secondary CPUs, and we never ever disable or re-enable again those clocks. So with the present code, I believe there is no problem. Even when we do CPU hotplug, we don't turn off the CPU clocks, simply because they cannot be turned off: the enable/disable state is only used here as an indication so that the clock driver knows which frequency change strategy it should apply.

Quoted 2 lines

But you're anyway right, I'll send a followup patch adding the lock. Would that be OK for you?

Sounds good. Can you also fix up the changelog in patch #2? After that I am happy with this series. I guess Jason will take it in and send it for his PR?

The commit log of this commit was not adjusted consequently, and this is my fault. Jason, is it still time to change this commit log?