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

On Wed, Dec 04, 2013 at 03:27:26PM +0000, Andrew Lunn wrote:

[...]
  

quoted

quoted

+
+	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.

quoted

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.