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

[Mark Rutland]

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 at lunn.ch>
> Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth at gmail.com>
> Acked-by: Viresh Kumar <viresh.kumar at 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.