[v2] clk: ti: Add support for dm814x ADPLL

Commit Message

Tony Lindgren Dec. 11, 2015, 3:48 p.m. UTC

* Russell King - ARM Linux <[e-mail]> [151211 05:53]:
> On Thu, Dec 10, 2015 at 06:26:32PM -0800, Tony Lindgren wrote:
> > +	/* Released with kfree() by clkdev_drop() */
> > +	cl = kzalloc(sizeof(*cl), GFP_KERNEL);
> > +	if (!cl)
> > +		return -ENOMEM;
> > +
> > +	/* Use clkdev_add, clk_register_clkdev limits length to MAX_CON_ID */
> > +	cl->con_id = name;
> > +	cl->clk = clock;
> > +	cl->clk_hw = __clk_get_hw(clock);
> > +	clkdev_add(cl);
> > +	d->clocks[index].cl = cl;
> 
> NAK.  I've no idea why you're open-coding the clkdev internals (which
> seems to have been a historical habbit in OMAP code.)  Please stop
> doing this.
>
> You are provided with clkdev_alloc() which will allocate the structure
> and initialise it for you, and clkdev_add() which will add the allocated
> and initialised struct to the list of lookups.  Everything you're doing
> above can be done with clkdev_alloc() + clkdev_add() which have been
> there for a _very_ long time.  They're even documented (thanks for
> providing me with more proof that documentation is nothing but a waste
> of time. :))

I tried, but I was seeing mysterious silent failures for some clocks.

> Even better is clkdev_create() which eliminates the two step clkdev_alloc()
> and clkdev_add() process.
> 
> So, the whole of the above can be reduced down to:
> 
> 	cl = clkdev_create(clock, name, NULL);
> 	if (!cl)
> 		return -ENOMEM;
> 

There's a problem with MAX_CON_ID 16 hardcoded allocated name length.

In this case I have 13 instances of plls with 3 - 4 outputs each and I'd
like to use "481c5040.adpll.clkout" style naming starting with the instance
address. Also "adpll5.clkdcoldo" style naming would work,

Below is a patch we should probably apply as a band aid to produce a proper
warning.

Naturally we don't want to allocate longer names for all the clocks.. And
we already do have some const names coming from device tree we could use.

What if we optionally allow passing a name to clkdev and add some flag
that tells clkdev code not to attempt to free it? Or do you have better
ideas in mind to fix the issue?

Regards,

Tony

8< ----------------
From: Tony Lindgren <[e-mail]>
Date: Fri, 11 Dec 2015 07:28:36 -0800
Subject: [PATCH] clkdev: Make silent failures of clk_get() produce a proper
 warning

Otherwise we can see mysterious failures with some clocks where
the name is longer than MAX_CON_ID 16. This can easily happen with
clock names coming from device tree for example in the form of
"481c5040.adpll.clkout" or "adpll1.clkdcoldo".

Let's make things a bit easier to debug by adding a warning for
the clock name. Note that we don't want to error out here as the
string matching still probably does the right thing for most
clocks.

Also note that we should also remove the hardcoded name allocation
in clkdev by adding functions that allow passing a name to clkdev
optionally.

Signed-off-by: Tony Lindgren <[e-mail]>

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Russell King - ARM Linux Dec. 11, 2015, 4:14 p.m. UTC | #1

On Fri, Dec 11, 2015 at 07:48:54AM -0800, Tony Lindgren wrote:
> There's a problem with MAX_CON_ID 16 hardcoded allocated name length.

Absolutely right...

> In this case I have 13 instances of plls with 3 - 4 outputs each and I'd
> like to use "481c5040.adpll.clkout" style naming starting with the instance
> address. Also "adpll5.clkdcoldo" style naming would work,

Because it's a connection ID, not a clock _name_.  I see that we're still
making all the same mistakes with clocks that were made years ago, and
which lead down the path of amazingly complex drivers having conditional
clock names and the like.

Is there a reason why you can't split this into separate device and input
names, and use clk_get_sys() rather than passing NULL as the device to
clk_get().  This is exactly why we've ended up with clk_get_sys(), to
cater for the cases where there is no struct device to associate with the
connection ID: the idea behind clk_get_sys() is that you pass the device
name explicitly, along with the connection ID.

Tony Lindgren Dec. 11, 2015, 4:43 p.m. UTC | #2

* Russell King - ARM Linux <[e-mail]> [151211 08:16]:
> On Fri, Dec 11, 2015 at 07:48:54AM -0800, Tony Lindgren wrote:
> > There's a problem with MAX_CON_ID 16 hardcoded allocated name length.
> 
> Absolutely right...

Well adding the warning there allows people to at least see what's going
on.

> > In this case I have 13 instances of plls with 3 - 4 outputs each and I'd
> > like to use "481c5040.adpll.clkout" style naming starting with the instance
> > address. Also "adpll5.clkdcoldo" style naming would work,
> 
> Because it's a connection ID, not a clock _name_.  I see that we're still
> making all the same mistakes with clocks that were made years ago, and
> which lead down the path of amazingly complex drivers having conditional
> clock names and the like.

Yeah we certainly want to get out of the conditional name business.

> Is there a reason why you can't split this into separate device and input
> names, and use clk_get_sys() rather than passing NULL as the device to
> clk_get().  This is exactly why we've ended up with clk_get_sys(), to
> cater for the cases where there is no struct device to associate with the
> connection ID: the idea behind clk_get_sys() is that you pass the device
> name explicitly, along with the connection ID.

For most code yes, there's still a pile of shared legacy code relying on
the clock connection ID only.

So what do you envision people using for the connection ID in cases like
this? It needs to be unique, should relate to the documentation and fit
MAX_CON_ID. Something like "pll5.clkdcoldo" will still overflow for pll13
unless also the dot is left out :)

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -256,6 +256,10 @@  vclkdev_alloc(struct clk_hw *hw, const char *con_id, const char *dev_fmt,
 {
 	struct clk_lookup_alloc *cla;
 
+	if (strlen(con_id) > MAX_CON_ID)
+		pr_warn("%s length of %s > %i, clock lookup can fail\n",
+			__func__, con_id, MAX_CON_ID);
+
 	cla = __clkdev_alloc(sizeof(*cla));
 	if (!cla)
 		return NULL;