All of lore.kernel.org
 help / color / mirror / Atom feed
From: Russell King - ARM Linux <linux@arm.linux.org.uk>
To: Paul Walmsley <paul@pwsan.com>
Cc: linux-arm-kernel@lists.arm.linux.org.uk,
	linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org,
	r-woodruff2@ti.com, Tony Lindgren <tony@atomide.com>
Subject: Re: [PATCH E 11/14] OMAP clock: track child clocks
Date: Sat, 14 Feb 2009 11:36:40 +0000	[thread overview]
Message-ID: <20090214113640.GB17965@n2100.arm.linux.org.uk> (raw)
In-Reply-To: <20090214112325.GA17965@n2100.arm.linux.org.uk>

On Sat, Feb 14, 2009 at 11:23:25AM +0000, Russell King - ARM Linux wrote:
> There's also a second issue - the comments before omap2_divisor_to_clksel()
> indicate that this function returns 0xffffffff on error.  Unfortunately,
> this is not so, it actually returns zero on error.  Moreover, we test
> the result of the function against ~0, so we'll never deal with the error
> case.  This really should be fixed so that we return the right value for
> the error case.  (Further comments on this in a follow up.)

The thing I don't like about this is that we have several functions looking
up clksel_rate entries, some of which return 0 on error and others ~0 on
error.

This is very prone to mistakes - and is probably why the original problem
has occurred.  I'd much prefer to fix the underlying confusion rather
than letting it persist, by making all these lookup functions return the
clksel_rate pointer or NULL.

Not only does that avoid the question about whether the function returns
0 or ~0 on error, but it also gets rid of the horrible return-through-pointer
style of _omap2_clksel_get_src_field() which itself is error prone.
(The kernel has had a few cases where this kind of thing has lead to
uninitialized use, so avoiding this where it's easy to do so makes
sense.)

One final point:

        if (parent_div > 0)
                clk->rate /= parent_div;

seems to be impossible to be false - the old code used div == 0 in the
tables as the end of table sentinel, and if it is encountered, 
_omap2_clksel_get_src_field (and the newer omap2_clksel_lookup_parent)
causes failure to occur.  Also, since parent_div is unsigned, the only
case where the above statement is false is when div == 0.  So, the code
can be further simplified to:

 	/* CLKSEL clocks follow their parents' rates, divided by a divisor */
 	clk->rate = new_parent->rate / clkr->div;

Agreed?

diff -u a/arch/arm/mach-omap2/clock.c b/arch/arm/mach-omap2/clock.c
--- a/arch/arm/mach-omap2/clock.c
+++ b/arch/arm/mach-omap2/clock.c
@@ -596,23 +596,24 @@
 }
 
 /**
- * omap2_clksel_to_divisor() - turn clksel field value into integer divider
+ * omap2_clksel_lookup_field() - lookup clksel_rate for clksel field value
  * @clk: OMAP struct clk to use
  * @field_val: register field value to find
  *
  * Given a struct clk of a rate-selectable clksel clock, and a register field
- * value to search for, find the corresponding clock divisor.  The register
+ * value to search for, find the corresponding clksel_rate entry.  The register
  * field value should be pre-masked and shifted down so the LSB is at bit 0
- * before calling.  Returns 0 on error
+ * before calling.  Returns NULL on error.
  */
-u32 omap2_clksel_to_divisor(struct clk *clk, u32 field_val)
+static const
+struct clksel_rate *omap2_clksel_lookup_field(struct clk *clk, u32 field_val)
 {
 	const struct clksel *clks;
 	const struct clksel_rate *clkr;
 
 	clks = omap2_get_clksel_by_parent(clk, clk->parent);
 	if (!clks)
-		return 0;
+		return NULL;
 
 	for (clkr = clks->rates; clkr->div; clkr++) {
 		if ((clkr->flags & cpu_mask) && (clkr->val == field_val))
@@ -623,22 +624,22 @@
 		printk(KERN_ERR "clock: Could not find fieldval %d for "
 		       "clock %s parent %s\n", field_val, clk->name,
 		       clk->parent->name);
-		return 0;
+		return NULL;
 	}
 
-	return clkr->div;
+	return clkr;
 }
 
 /**
- * omap2_divisor_to_clksel() - turn clksel integer divisor into a field value
+ * omap2_clksel_lookup_divisor() - lookup clksel_rate for integer divisor
  * @clk: OMAP struct clk to use
  * @div: integer divisor to search for
  *
- * Given a struct clk of a rate-selectable clksel clock, and a clock divisor,
- * find the corresponding register field value.  The return register value is
- * the value before left-shifting.  Returns ~0 on error
+ * Given a struct clk of a rate-selectable clksel clock and a clock divisor,
+ * find the corresponding clksel_rate entry.  Returns NULL on error.
  */
-u32 omap2_divisor_to_clksel(struct clk *clk, u32 div)
+static const
+struct clksel_rate *omap2_clksel_lookup_divisor(struct clk *clk, u32 div)
 {
 	const struct clksel *clks;
 	const struct clksel_rate *clkr;
@@ -648,7 +649,7 @@
 
 	clks = omap2_get_clksel_by_parent(clk, clk->parent);
 	if (!clks)
-		return ~0;
+		return NULL;
 
 	for (clkr = clks->rates; clkr->div; clkr++) {
 		if ((clkr->flags & cpu_mask) && (clkr->div == div))
@@ -659,10 +660,10 @@
 		printk(KERN_ERR "clock: Could not find divisor %d for "
 		       "clock %s parent %s\n", div, clk->name,
 		       clk->parent->name);
-		return ~0;
+		return NULL;
 	}
 
-	return clkr->val;
+	return clkr;
 }
 
 /**
@@ -673,6 +674,7 @@
  */
 u32 omap2_clksel_get_divisor(struct clk *clk)
 {
+	const struct clksel_rate *clkr;
 	u32 v;
 
 	if (!clk->clksel_mask)
@@ -681,11 +683,14 @@
 	v = __raw_readl(clk->clksel_reg) & clk->clksel_mask;
 	v >>= __ffs(clk->clksel_mask);
 
-	return omap2_clksel_to_divisor(clk, v);
+	clkr = omap2_clksel_lookup_field(clk, v);
+
+	return clkr ? clkr->val : 0;
 }
 
 int omap2_clksel_set_rate(struct clk *clk, unsigned long rate)
 {
+	const struct clksel_rate *clkr;
 	u32 v, field_val, validrate, new_div = 0;
 
 	if (!clk->clksel_mask)
@@ -695,17 +700,17 @@
 	if (validrate != rate)
 		return -EINVAL;
 
-	field_val = omap2_divisor_to_clksel(clk, new_div);
-	if (field_val == ~0)
+	clkr = omap2_clksel_lookup_divisor(clk, new_div);
+	if (!clkr)
 		return -EINVAL;
 
 	v = __raw_readl(clk->clksel_reg);
 	v &= ~clk->clksel_mask;
-	v |= field_val << __ffs(clk->clksel_mask);
+	v |= clkr->val << __ffs(clk->clksel_mask);
 	__raw_writel(v, clk->clksel_reg);
 	v = __raw_readl(clk->clksel_reg); /* OCP barrier */
 
-	clk->rate = clk->parent->rate / new_div;
+	clk->rate = clk->parent->rate / clkr->div;
 
 	_omap2xxx_clk_commit(clk);
 
@@ -733,18 +738,18 @@
 }
 
 /*
- * Converts encoded control register address into a full address
- * On error, the return value (parent_div) will be 0.
+ * Given a struct clk of a rate-selectable clksel clock and a parent clock,
+ * find the default clksel_rate entry.  Returns NULL on error.
  */
-static u32 _omap2_clksel_get_src_field(struct clk *src_clk, struct clk *clk,
-				       u32 *field_val)
+static const
+struct clksel_rate *omap2_clksel_lookup_parent(struct clk *clk, struct clk *parent)
 {
 	const struct clksel *clks;
 	const struct clksel_rate *clkr;
 
-	clks = omap2_get_clksel_by_parent(clk, src_clk);
+	clks = omap2_get_clksel_by_parent(clk, parent);
 	if (!clks)
-		return 0;
+		return NULL;
 
 	for (clkr = clks->rates; clkr->div; clkr++) {
 		if (clkr->flags & cpu_mask && clkr->flags & DEFAULT_RATE)
@@ -755,20 +760,19 @@
 		printk(KERN_ERR "clock: Could not find default rate for "
 		       "clock %s parent %s\n", clk->name,
 		       src_clk->parent->name);
-		return 0;
+		return NULL;
 	}
 
 	/* Should never happen.  Add a clksel mask to the struct clk. */
 	WARN_ON(clk->clksel_mask == 0);
 
-	*field_val = clkr->val;
-
-	return clkr->div;
+	return NULL;
 }
 
 int omap2_clk_set_parent(struct clk *clk, struct clk *new_parent)
 {
-	u32 field_val, v, parent_div;
+	const struct clksel_rate *clkr;
+	u32 v;
 
 	if (clk->flags & CONFIG_PARTICIPANT)
 		return -EINVAL;
@@ -776,8 +780,8 @@
 	if (!clk->clksel)
 		return -EINVAL;
 
-	parent_div = _omap2_clksel_get_src_field(new_parent, clk, &field_val);
-	if (!parent_div)
+	clkr = omap2_clksel_lookup_parent(clk, new_parent);
+	if (!clkr)
 		return -EINVAL;
 
 	if (clk->usecount > 0)
@@ -786,7 +790,7 @@
 	/* Set new source value (previous dividers if any in effect) */
 	v = __raw_readl(clk->clksel_reg);
 	v &= ~clk->clksel_mask;
-	v |= field_val << __ffs(clk->clksel_mask);
+	v |= clkr->val << __ffs(clk->clksel_mask);
 	__raw_writel(v, clk->clksel_reg);
 	v = __raw_readl(clk->clksel_reg);    /* OCP barrier */
 
@@ -800,8 +804,8 @@
 	/* CLKSEL clocks follow their parents' rates, divided by a divisor */
 	clk->rate = new_parent->rate;
 
-	if (parent_div > 0)
-		clk->rate /= parent_div;
+	if (clkr->div > 0)
+		clk->rate /= clkr->div;
 
 	pr_debug("clock: set parent of %s to %s (new rate %ld)\n",
 		 clk->name, clk->parent->name, clk->rate);
diff -u a/arch/arm/mach-omap2/clock.h b/arch/arm/mach-omap2/clock.h
--- a/arch/arm/mach-omap2/clock.h
+++ b/arch/arm/mach-omap2/clock.h
@@ -42,8 +42,6 @@ void omap2_init_clksel_parent(struct clk *clk);
 u32 omap2_clksel_get_divisor(struct clk *clk);
 u32 omap2_clksel_round_rate_div(struct clk *clk, unsigned long target_rate,
 				u32 *new_div);
-u32 omap2_clksel_to_divisor(struct clk *clk, u32 field_val);
-u32 omap2_divisor_to_clksel(struct clk *clk, u32 div);
 unsigned long omap2_fixed_divisor_recalc(struct clk *clk);
 long omap2_clksel_round_rate(struct clk *clk, unsigned long target_rate);
 int omap2_clksel_set_rate(struct clk *clk, unsigned long rate);

  reply	other threads:[~2009-02-14 11:37 UTC|newest]

Thread overview: 59+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-01-28 19:27 [PATCH E 00/14] OMAP clock, E of F: SDRAM fixes, clock optimization Paul Walmsley
2009-01-28 19:27 ` [PATCH E 01/14] OMAP2 SDRC: move mach-omap2/memory.h into include/asm-arm/arch-omap/sdrc.h Paul Walmsley
2009-01-28 19:27 ` [PATCH E 02/14] OMAP2 SDRC: rename memory.c to sdrc2xxx.c Paul Walmsley
2009-01-28 19:27 ` [PATCH E 03/14] OMAP2 SDRC: separate common OMAP2/3 code from OMAP2xxx code Paul Walmsley
2009-01-28 19:27 ` [PATCH E 04/14] OMAP2 SDRC: add SDRAM timing parameter infrastructure Paul Walmsley
2009-01-28 19:27 ` [PATCH E 05/14] OMAP3 clock: add omap3_core_dpll_m2_set_rate() Paul Walmsley
2009-01-28 19:27 ` [PATCH E 06/14] PM: OMAP3: Make sure clk_disable_unused() order is correct Paul Walmsley
2009-01-28 19:27 ` [PATCH E 07/14] OMAP2/3 clock: use standard set_rate fn in omap2_clk_arch_init() Paul Walmsley
2009-01-28 19:27 ` [PATCH E 08/14] OMAP clock: move rate recalc, propagation code up to plat-omap/clock.c Paul Walmsley
2009-01-29 17:41   ` Russell King - ARM Linux
2009-01-30  8:42     ` Paul Walmsley
2009-01-30  8:52       ` Russell King - ARM Linux
2009-01-30 14:23         ` Woodruff, Richard
2009-01-30 14:23           ` Woodruff, Richard
2009-01-31 11:40           ` Russell King - ARM Linux
2009-01-31 11:40             ` Russell King - ARM Linux
2009-02-03  8:42             ` Paul Walmsley
2009-02-03  8:42               ` Paul Walmsley
2009-02-03  9:45             ` Paul Walmsley
2009-02-03  9:45               ` Paul Walmsley
2009-02-02  7:13       ` Paul Walmsley
2009-02-03 13:18         ` Russell King - ARM Linux
2009-01-28 19:27 ` [PATCH E 09/14] OMAP2/3 clock: drop recalc function pointers from fixed rate clocks Paul Walmsley
2009-01-28 19:27 ` [PATCH E 10/14] OMAP clock: support "dry run" rate and parent changes Paul Walmsley
2009-02-08 13:17   ` Russell King - ARM Linux
2009-02-08 19:48     ` David Brownell
2009-02-11  7:53     ` Paul Walmsley
2009-02-08 15:53   ` Russell King - ARM Linux
2009-02-11  8:18     ` Paul Walmsley
2009-01-28 19:27 ` [PATCH E 11/14] OMAP clock: track child clocks Paul Walmsley
2009-01-29 15:14   ` Russell King - ARM Linux
2009-01-29 22:06     ` Russell King - ARM Linux
2009-01-30  8:35       ` Paul Walmsley
2009-02-02  4:57       ` Paul Walmsley
2009-02-09 14:11       ` Russell King - ARM Linux
2009-02-13  7:01         ` Paul Walmsley
2009-02-14 11:23           ` Russell King - ARM Linux
2009-02-14 11:36             ` Russell King - ARM Linux [this message]
2009-02-25  9:45               ` Paul Walmsley
2009-02-19 12:19             ` Russell King - ARM Linux
2009-02-20  0:50               ` Woodruff, Richard
2009-02-20  0:50                 ` Woodruff, Richard
2009-02-23 16:03                 ` Russell King - ARM Linux
2009-02-23 16:03                   ` Russell King - ARM Linux
2009-02-24 12:35                   ` Woodruff, Richard
2009-02-24 12:35                     ` Woodruff, Richard
2009-03-02 23:02                   ` Paul Walmsley
2009-03-02 23:02                     ` Paul Walmsley
2009-03-03 16:45                     ` Russell King - ARM Linux
2009-03-03 16:45                       ` Russell King - ARM Linux
2009-02-22 23:37             ` Paul Walmsley
2009-02-24  9:43               ` Russell King - ARM Linux
2009-01-29 19:52   ` Russell King - ARM Linux
2009-02-02  7:57     ` Paul Walmsley
2009-01-28 19:28 ` [PATCH E 12/14] OMAP clock: unnecessary clock flag removal fiesta Paul Walmsley
2009-02-23 15:50   ` Russell King - ARM Linux
2009-03-02 22:35     ` Paul Walmsley
2009-01-28 19:28 ` [PATCH E 13/14] OMAP2/3 clock: remove clk->owner Paul Walmsley
2009-01-28 19:28 ` [PATCH E 14/14] OMAP clock: rearrange clock.h structure order Paul Walmsley

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20090214113640.GB17965@n2100.arm.linux.org.uk \
    --to=linux@arm.linux.org.uk \
    --cc=linux-arm-kernel@lists.arm.linux.org.uk \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-omap@vger.kernel.org \
    --cc=paul@pwsan.com \
    --cc=r-woodruff2@ti.com \
    --cc=tony@atomide.com \
    --subject='Re: [PATCH E 11/14] OMAP clock: track child clocks' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.