[PATCH 0/4] clk: mvebu: fix clk init order

On 17/02/2014 16:44, Ezequiel Garcia wrote:

On Mon, Feb 17, 2014 at 04:28:41PM +0100, Gregory CLEMENT wrote:

quoted

On 17/02/2014 16:21, Ezequiel Garcia wrote:

quoted

On Mon, Feb 17, 2014 at 03:25:22PM +0100, Gregory CLEMENT wrote:

quoted

On 17/02/2014 15:13, Ezequiel Garcia wrote:

quoted

On Wed, Feb 05, 2014 at 01:34:57PM -0500, Jason Cooper wrote:

quoted

On Sat, Jan 25, 2014 at 07:19:06PM +0100, Sebastian Hesselbarth wrote:

quoted

This patch set fixes clk init order that went upside-down with
v3.14. I haven't really investigated what caused this, but I assume
it is related with DT node reordering by addresses.

Anyway, with v3.14 for MVEBU SoCs, the clock gating driver gets
registered before core clocks driver. Unfortunately, we cannot
return -EPROBE_DEFER in drivers initialized by clk_of_init. As the
init order for our drivers is always core clocks before clock gating,
we maintain init order ourselves by hooking CLK_OF_DECLARE to one
init function that will register core clocks before clock gating
driver.

This patch is based on pre-v3.14-rc1 mainline and should go in as
fixes for it. As we now send MVEBU clk pull-requests to Mike directly,
I suggest Jason picks it up as a topic branch.

The patches have been boot tested on Dove and compile-tested only
for Kirkwood, Armada 370 and XP.

Sebastian Hesselbarth (4):
  clk: mvebu: armada-370: maintain clock init order
  clk: mvebu: armada-xp: maintain clock init order
  clk: mvebu: dove: maintain clock init order
  clk: mvebu: kirkwood: maintain clock init order

 drivers/clk/mvebu/armada-370.c | 21 ++++++++++-----------
 drivers/clk/mvebu/armada-xp.c  | 20 +++++++++-----------
 drivers/clk/mvebu/dove.c       | 19 +++++++++----------
 drivers/clk/mvebu/kirkwood.c   | 34 ++++++++++++++++------------------
 4 files changed, 44 insertions(+), 50 deletions(-)

Whole series applied to mvebu/clk-fixes.

Are we still in time to consider Emilio's oneline proposal?
(Emilio: would you mind preparing a suitable patch against dove,
kirkwood, armada370/xp, so we can see the real thing?).

I am still strongly against this proposal because hard-coded the parent
clock name in the driver seems very wrong and moreover in some circumstances
(if there is no output-name, which is our default case) this proposal
just ignored the parent clock given by the device tree and this looked
more wrong.

So you're against the proposal as a permanent fix, *and* against the
proposal as a workaround fix?

Yes

quoted

quoted

quoted

Sebastian fix works perfect, and it easy to understand. However, it has
quite a large diffstat. When compared to Emilio's oneline proposal, it
seems to me it would be preferable, unless it's broken.

Workaround or not, the fact is this code will be in v3.14, so maybe we
can spend some time considering a cleaner option.

Before discussing the solution as compared to your for-v3.15 clock
registration order patch, I wanted to trigger some discussion around
replacing this big and intrusive workaround with Emilio's oneline fix.

Let's suppose we're considering them as workaround to live just one or
two releases. Wouldn't it be better to take the least instrusive?

The better solution is the one which doesn't add another regression and until
today I though we had an agreement to use the patch set from Sebastian. If
I remember well Jason had sent a pull request for it.

Right. If you think it adds a regression, then that's a perfectly valid reasons
for nacking.

However, I'd like to double-check we have such a regression. I guess you're
talking about the "tclk" name being hardcoded. IMHO, it's hardcoded in the
driver in the first place:

void __init mvebu_coreclk_setup(struct device_node *np,
				const struct coreclk_soc_desc *desc)
{
	const char *tclk_name = "tclk";
[..] 

Here it is just about giving a name to a clock. As in the device tree
we only refer to the clock by index, the name don't matter.

So it wouldn't be too nasty?

I think you also mentioned the hypothetical case where the name
is overloaded in the devicetree, so it's not "tclk", no? In that case,
Emilio's fix would break.

I don't think I mentioned this case. I mentioned that this "fix" will
ignore the device tree.

However, we don't have such situation in our 'canonical' devicetrees,
so if the devicetree is allowed to be change, it can also be
changed to add clock-output-name's so the name doesn't have to be
obtained after the clock is registered.

Which would solve it, no?

I really don't understand why you want introduce potential problem, just
to save a few line of code. A code that " works perfect, and it easy
to understand" as you wrote.




-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com