[PATCH] BUG: clk_find() misses some clocks

[PATCH] BUG: clk_find() misses some clocks

[Richard Genoud]

if a clock is declared like that:
CLKDEV_CON_DEV_ID("pioA", "fffff400.gpio", &pioAB_clk)
(so, dev_id AND con_id are defined)
and the driver looks for a dev_id (but no specific con_id) like that:
clock = clk_get(&pdev->dev, NULL);

The clock won't be found:

if (dev_id)
	best_possible += 2; /* dev_id != NULL */
if (con_id)
	best_possible += 1; /* con_id == NULL */
/* => so best possible == 2 */

list_for_each_entry(p, &clocks, node) {
	match = 0;
/*
 * checking p->dev_id = "fffff400.gpio" and p->con_id = "pioA"
 * against dev_id = "fffff400.gpio" and con_id = NULL
 */
	if (p->dev_id) { /* ok, p->dev_id exists */
	 /* and, we are lucky, there's a match ! \o/ */
		if (!dev_id || strcmp(p->dev_id, dev_id))
			continue;
		match += 2;
	}
/* so we have match == 2 */

	if (p->con_id) {
		/* p->con_id is not null */
		if (!con_id || strcmp(p->con_id, con_id))
		/*
		 * too bad !! con_id is null !
		 * even if we have best_possible == match
		 * our clock won't be selected
		 */
			continue;
		match += 1;
	}

	if (match > best_found) {
		cl = p;
		if (match != best_possible)
			best_found = match;
		else
			break;
	}
}

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
---
 drivers/clk/clkdev.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index d423c9b..e28b120 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -114,13 +114,13 @@ static struct clk_lookup *clk_find(const char *dev_id, const char *con_id)
 
 	list_for_each_entry(p, &clocks, node) {
 		match = 0;
-		if (p->dev_id) {
-			if (!dev_id || strcmp(p->dev_id, dev_id))
+		if (p->dev_id && dev_id) {
+			if (strcmp(p->dev_id, dev_id))
 				continue;
 			match += 2;
 		}
-		if (p->con_id) {
-			if (!con_id || strcmp(p->con_id, con_id))
+		if (p->con_id && con_id) {
+			if (strcmp(p->con_id, con_id))
 				continue;
 			match += 1;
 		}
-- 
1.7.2.5

Re: [PATCH] BUG: clk_find() misses some clocks

[Russell King - ARM Linux]

Please read the comments on the function:

 * Find the correct struct clk for the device and connection ID.
 * We do slightly fuzzy matching here:
 *  An entry with a NULL ID is assumed to be a wildcard.
 *  If an entry has a device ID, it must match
 *  If an entry has a connection ID, it must match
 * Then we take the most specific entry - with the following
 * order of precedence: dev+con > dev only > con only.

On Fri, Aug 17, 2012 at 11:47:23AM +0200, Richard Genoud wrote:
> if a clock is declared like that:
> CLKDEV_CON_DEV_ID("pioA", "fffff400.gpio", &pioAB_clk)

So you've declared it with a connection ID.  Therefore, as the comment
above says, "it must match" what the driver is asking for.

It's not a bug, this is done intentionally so that mismatches do not
occur.

Re: [PATCH] BUG: clk_find() misses some clocks

[Richard Genoud]

2012/8/17 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> Please read the comments on the function:
>
>  * Find the correct struct clk for the device and connection ID.
>  * We do slightly fuzzy matching here:
>  *  An entry with a NULL ID is assumed to be a wildcard.
>  *  If an entry has a device ID, it must match
>  *  If an entry has a connection ID, it must match
>  * Then we take the most specific entry - with the following
>  * order of precedence: dev+con > dev only > con only.
>
> On Fri, Aug 17, 2012 at 11:47:23AM +0200, Richard Genoud wrote:
>> if a clock is declared like that:
>> CLKDEV_CON_DEV_ID("pioA", "fffff400.gpio", &pioAB_clk)
>
> So you've declared it with a connection ID.  Therefore, as the comment
> above says, "it must match" what the driver is asking for.
>
> It's not a bug, this is done intentionally so that mismatches do not
> occur.
Ok, so I have to declare it like that :
CLKDEV_CON_DEV_ID(NULL, "fffff400.gpio", &pioAB_clk)

Got it !
Thanks.

Richard.

-- 
for me, ck means con kolivas and not calvin klein... does it mean I'm a geek ?

Re: [PATCH] BUG: clk_find() misses some clocks

[Russell King - ARM Linux]

On Fri, Aug 17, 2012 at 12:23:06PM +0200, Richard Genoud wrote:
> Ok, so I have to declare it like that :
> CLKDEV_CON_DEV_ID(NULL, "fffff400.gpio", &pioAB_clk)

Yes, because the wildcarding is in the clkdev tables, not in the driver.
If you think about what's going on, that's the way it has to be.  Remember
that it's the _device_ that defines what the connection name is (there's
the hint: it's the connection name on the device itself) and it isn't a
global clock name.

So, for example, if set of devices used in multiple different SoCs have a
clock input called "apbclk" and on some SoCs this clock is uncontrollable,
but on others it can be individually controlled, we can cater for this:

- the SoC where this clock is uncontrollable can create a dummy clock with
  a connection ID of "apbclk" and a NULL device name, which will match
  against any device asking for an "apbclk".
- the SoC where the clock is individually controllable would list the
  individual "apbclk" entries in the clkdev table, and clkdev will return
  the appropriate one.

In mixed environments, this also works, because we return the "best match"
from clkdev.

Same goes for devices where they have an "iclk" and an "fclk" (iow, two
clocks) - the connection ID can be wildcarded in the clkdev tables so that
the same clock can be returned for the same device where the driver asks
for the clocks individually - if that's what the SoC support desires.

It makes no sense for a driver to ask for "any clock on this device"
because drivers always want the clock for a particular purpose.  Hence
why NULL arguments to clk_get() don't act as wildcards.