[PATCH v4 1/4] cpufreq: Add a cpufreq driver for Marvell Dove

On 4 December 2013 19:47, Andrew Lunn [off-list ref] 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. :)

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

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 :)