Message-ID: <20140219162819.GA5000@localhost>
Date:	Wed, 19 Feb 2014 13:28:20 -0300
From:	Ezequiel Garcia <ezequiel.garcia@...e-electrons.com>
To:	Gregory CLEMENT <gregory.clement@...e-electrons.com>
Cc:	Emilio López <emilio@...pez.com.ar>,
	Jason Cooper <jason@...edaemon.net>,
	Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
	Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
	Andrew Lunn <andrew@...n.ch>,
	Mike Turquette <mturquette@...aro.org>,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 0/4] clk: mvebu: fix clk init order

On Tue, Feb 18, 2014 at 10:47:00AM +0100, Gregory CLEMENT wrote:
> On 17/02/2014 19:19, Ezequiel Garcia wrote:
> > On Mon, Feb 17, 2014 at 04:59:01PM +0100, Gregory CLEMENT wrote:
> > [..]
> >>>
> >>> 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.
> >>
> > 
> > Unless I'm really confused about what's the problem here, the *name* is
> > *all* that matters.
> > 
> > We're having a clock *registration* order issue (which is different from clock
> > enable). Let me try to explain the issue, in case it's still not clear.
> > 
> > I'll stick to the current specific problem but it can apply to any other
> > pair of parent/child.
> > 
> > Some of the mvebu clocks are registered as part of the "core" clock
> > group, modeled by the "marvell,armada-370-core-clock" compatible node.
> > 
> > Another group of mvebu clocks are registered as part of the "gating"
> > clock group, modeled by the "marvell,armada-370-gating-clock" compatible
> > node.
> > 
> > By default, all the gating clocks are child of the first registered core
> > clock. This clock is named "tclk" by default, and this name can be overloaded
> > in the devicetree.
> > 
> > So far, so good, right?
> > 
> > The issue we're trying to fix, arises from the mvebu_clk_gating_setup()
> > trying to get the name of this "tclk", as a registered clock.
> > 
> > In other words, the current code needs the tclk to be registered, for it
> > will ask his name like this:
> > 
> > 	const char *default_parent = NULL;
> > 
> > 	clk = of_clk_get(np, 0);
> > 	if (!IS_ERR(clk)) {
> > 		default_parent = __clk_get_name(clk);
> > 		clk_put(clk);
> > 	}
> > 
> > Once it gets the name, all goes smoothly. Notice how the clock is obtained
> > for the sole purpose of getting the name of it, which shows clearly it's the
> > *name* that matters.
> > 
> > The ordering issue happens when the gating clock group is probed, before
> > the core clock group. In that case, it's not possible to get the
> > "&coreclk 0" (which is wrongly assumed to be registered), and so it's
> > not possible to get the name.
> > 
> > So the root of the problem is that snippet above, which adds a
> > completely unneeded registration order requirement. Instead, we should
> > be looking for the names: we can hardcode the name or fetch it from the
> > devicetree (Emilio has posted patches for both).
> > 
> > I really don't see why we're not fixing this, instead of adding yet
> > another layer of complexity to the problem.
> 
> All this have already discussed in the previous emails. And even if Emilio
> denied introducing a regression, it was what the code did. See my example
> here:
> http://article.gmane.org/gmane.linux.kernel/1649439
> 
> Now as you both are really annoying with it, what would be an acceptable
> version would be the something like the patch attached. There will be still
> an issue if old dtb is used with recent kernel, but at least the user will
> be warned.
> 

The regression you're talking about is will happen if the user decides to set
an awkward parent as the *default* parent of the gate clock group.

It's just the *default* that's specified in the devicetree, not the actual parent,
since we've designed this so a particular clock parent can always be specified from
the array in the driver.

> This code only fix the Armada 370 case, a complete solution should modify
> the dtsi for Armada XP, Armada 375, Armada 38x, Kirkwood and Dove. It should
> also make the outputname mandatory for gate-clk for consistency.
> 

I think you're missing the point in discussion. If you propose a patch
for the mvebu clock driver, then I guess you admit the problem is
solvable in the driver.

So, the real questions are:

Do we want to enforce a clock registration order?

Is this a framework defect? Or are our mvebu clocks doing things wrong?

As I tried to explained in detailed above, I think the mvebu clocks are doing
really evil things by needing a registered clock, just so it can retrieve the
default clock *name*. It's not even the clock name, given we allow to
override such default name.

A clock never needed a parent clock to be registered to be able to
register itself, it can just use the parent's clock name.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
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