Message-ID: <20130617174451.GF28497@quad.lixom.net>
Date:	Mon, 17 Jun 2013 10:44:51 -0700
From:	Olof Johansson <olof@...om.net>
To:	Lorenzo Pieralisi <lorenzo.pieralisi@....com>
Cc:	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	devicetree-discuss@...ts.ozlabs.org,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Pawel Moll <pawel.moll@....com>,
	Amit Kucheria <amit.kucheria@...aro.org>,
	Jon Medhurst <tixy@...aro.org>,
	Achin Gupta <achin.gupta@....com>,
	Sudeep KarkadaNagesha <Sudeep.KarkadaNagesha@....com>,
	Nicolas Pitre <nicolas.pitre@...aro.org>
Subject: Re: [RFC PATCH v4 2/2] drivers: mfd: vexpress: add Serial Power
 Controller (SPC) support

On Mon, Jun 17, 2013 at 04:51:09PM +0100, Lorenzo Pieralisi wrote:
> The TC2 versatile express core tile integrates a logic block that provides the
> interface between the dual cluster test-chip and the M3 microcontroller that
> carries out power management. The logic block, called Serial Power Controller
> (SPC), contains several memory mapped registers to control among other things
> low-power states, operating points and reset control.
> 
> This patch provides a driver that enables run-time control of features
> implemented by the SPC control logic.
> 
> The SPC control logic is required to be programmed very early in the boot
> process to reset secondary CPUs on the TC2 testchip, set-up jump addresses and
> wake-up IRQs for power management.
> Since the SPC logic is also used to control clocks and operating points,
> that have to be initialized early as well, the SPC interface consumers can not
> rely on early initcalls ordering, which is inconsistent, to wait for SPC
> initialization. Hence, in order to keep the components relying on the SPC
> coded in a sane way, the driver puts in place a synchronization scheme that
> allows kernel drivers to check if the SPC driver has been initialized and if
> not, to initialize it upon check.
> 
> A status variable is kept in memory so that loadable modules that require SPC
> interface (eg CPUfreq drivers) can still check the correct initialization and
> use the driver correctly after functions used at boot to init the driver are
> freed.
> 
> The driver also provides a bridge interface through the vexpress config
> infrastructure. Operations allowing to read/write operating points are
> made to go via the same interface as configuration transactions so that
> all requests to M3 are serialized.
> 
> Device tree bindings documentation for the SPC component is provided with
> the patchset.

Sorry, I got to think of this over the weekend and should have replied
before you had a chance to repost, but still:

Why is the operating point and frequency change code in this driver at all?
Usually, the MFD driver contains a shared method to access register space on
a multifunction device, but the actual operation of each subdevice is handled
by individual drivers in the regular locations.

So, in the case of operating points and requencies, that should be in
a cpufreq driver. And the clock setup should presumably be in a clk
framework driver if needed.

Then all that would be left here is the functionality for submitting the two
kinds of commands, and handling interrupts.

That'll trim down the driver to a point where I think you'll find it much
easier to get merged. :-)


[...]

> +struct ve_spc_drvdata {
> +	void __iomem *baseaddr;
> +	/* A15 processor MPIDR[15:8] bitfield */

A comment describing what the meaning is would be more useful, even if
less technically specific. Or maybe something like "Cluster ID of the
A15 cluster, from MPIDR[15:8]" or similar.

> +	u32 a15_clusid;


(I reserve the right to have more comments later but I think we want to discuss
the removal of frequency management code first. :-)


-Olof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists