Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1262726 > unrolled thread

[PATCH 1/2] clk: bcm2835: Support for clock parent selection

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/2] clk: bcm2835: Support for clock parent selection Remi Pommarel <repk@triplefau.lt> - 2015-11-05 00:10 +0100
    Re: [PATCH 1/2] clk: bcm2835: Support for clock parent selection Eric Anholt <eric@anholt.net> - 2015-11-05 03:10 +0100
      Re: [PATCH 1/2] clk: bcm2835: Support for clock parent selection Remi Pommarel <repk@triplefau.lt> - 2015-11-05 20:00 +0100
        Re: [PATCH 1/2] clk: bcm2835: Support for clock parent selection Eric Anholt <eric@anholt.net> - 2015-11-09 17:40 +0100

#1262726 — [PATCH 1/2] clk: bcm2835: Support for clock parent selection

Some bcm2835 clocks used by hardware (like "PWM" or "H264") can have multiple
parents. These clocks divide the rate of one parent which can be selected by
setting the proper bits in their clock control register.

Previously all these parents where handled by a mux clock. But a mux clock
cannot be used because updating clock control register to select parent needs a
password to be xor'd with the parent index.

This patch get rid of mux clock and make these clocks handle their own parent,
allowing them to select the one to use.

Signed-off-by: Remi Pommarel <repk@triplefau.lt>
---
 drivers/clk/bcm/clk-bcm2835.c | 116 ++++++++++++++++++++++++++----------------
 1 file changed, 71 insertions(+), 45 deletions(-)

diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index 39bf582..9469729 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -1197,16 +1197,6 @@ static long bcm2835_clock_rate_from_divisor(struct bcm2835_clock *clock,
 	return temp;
 }
 
-static long bcm2835_clock_round_rate(struct clk_hw *hw,
-				     unsigned long rate,
-				     unsigned long *parent_rate)
-{
-	struct bcm2835_clock *clock = bcm2835_clock_from_hw(hw);
-	u32 div = bcm2835_clock_choose_div(hw, rate, *parent_rate);
-
-	return bcm2835_clock_rate_from_divisor(clock, *parent_rate, div);
-}
-
 static unsigned long bcm2835_clock_get_rate(struct clk_hw *hw,
 					    unsigned long parent_rate)
 {
@@ -1278,13 +1268,69 @@ static int bcm2835_clock_set_rate(struct clk_hw *hw,
 	return 0;
 }
 
+static int bcm2835_clock_determine_source(struct clk_hw *hw,
+		struct clk_rate_request *req)
+{
+	struct clk_hw *parent, *best_parent = NULL;
+	struct clk_rate_request parent_req;
+	unsigned long prate, best_rate = ULONG_MAX;
+	size_t i;
+
+	/*
+	 * Select parent clock that has the closest but higher rate
+	 */
+	for (i = 0; i < clk_hw_get_num_parents(hw); ++i) {
+		parent = clk_hw_get_parent_by_index(hw, i);
+		if (!parent)
+			continue;
+		parent_req = *req;
+		prate = clk_hw_get_rate(parent);
+		if (prate < best_rate && prate >= req->rate) {
+			best_parent = parent;
+			best_rate = prate;
+		}
+	}
+
+	if (!best_parent)
+		return -EINVAL;
+
+	req->best_parent_hw = best_parent;
+	req->best_parent_rate = best_rate;
+
+	return 0;
+}
+
+static int bcm2835_clock_set_parent(struct clk_hw *hw, u8 index)
+{
+	struct bcm2835_clock *clock = bcm2835_clock_from_hw(hw);
+	struct bcm2835_cprman *cprman = clock->cprman;
+	const struct bcm2835_clock_data *data = clock->data;
+	u8 src = (index << CM_SRC_SHIFT) & CM_SRC_MASK;
+
+	cprman_write(cprman, data->ctl_reg, src);
+	return 0;
+}
+
+static u8 bcm2835_clock_get_parent(struct clk_hw *hw)
+{
+	struct bcm2835_clock *clock = bcm2835_clock_from_hw(hw);
+	struct bcm2835_cprman *cprman = clock->cprman;
+	const struct bcm2835_clock_data *data = clock->data;
+	u32 src = cprman_read(cprman, data->ctl_reg);
+
+	return (src & CM_SRC_MASK) >> CM_SRC_SHIFT;
+}
+
+
 static const struct clk_ops bcm2835_clock_clk_ops = {
 	.is_prepared = bcm2835_clock_is_on,
 	.prepare = bcm2835_clock_on,
 	.unprepare = bcm2835_clock_off,
 	.recalc_rate = bcm2835_clock_get_rate,
 	.set_rate = bcm2835_clock_set_rate,
-	.round_rate = bcm2835_clock_round_rate,
+	.determine_rate = bcm2835_clock_determine_source,
+	.set_parent = bcm2835_clock_set_parent,
+	.get_parent = bcm2835_clock_get_parent,
 };
 
 static int bcm2835_vpu_clock_is_on(struct clk_hw *hw)
@@ -1300,7 +1346,9 @@ static const struct clk_ops bcm2835_vpu_clock_clk_ops = {
 	.is_prepared = bcm2835_vpu_clock_is_on,
 	.recalc_rate = bcm2835_clock_get_rate,
 	.set_rate = bcm2835_clock_set_rate,
-	.round_rate = bcm2835_clock_round_rate,
+	.determine_rate = bcm2835_clock_determine_source,
+	.set_parent = bcm2835_clock_set_parent,
+	.get_parent = bcm2835_clock_get_parent,
 };
 
 static struct clk *bcm2835_register_pll(struct bcm2835_cprman *cprman,
@@ -1394,45 +1442,23 @@ static struct clk *bcm2835_register_clock(struct bcm2835_cprman *cprman,
 {
 	struct bcm2835_clock *clock;
 	struct clk_init_data init;
-	const char *parent;
+	const char *parents[1 << CM_SRC_BITS];
+	size_t i;
 
 	/*
-	 * Most of the clock generators have a mux field, so we
-	 * instantiate a generic mux as our parent to handle it.
+	 * Replace our "xosc" references with the oscillator's
+	 * actual name.
 	 */
-	if (data->num_mux_parents) {
-		const char *parents[1 << CM_SRC_BITS];
-		int i;
-
-		parent = devm_kasprintf(cprman->dev, GFP_KERNEL,
-					"mux_%s", data->name);
-		if (!parent)
-			return NULL;
-
-		/*
-		 * Replace our "xosc" references with the oscillator's
-		 * actual name.
-		 */
-		for (i = 0; i < data->num_mux_parents; i++) {
-			if (strcmp(data->parents[i], "xosc") == 0)
-				parents[i] = cprman->osc_name;
-			else
-				parents[i] = data->parents[i];
-		}
-
-		clk_register_mux(cprman->dev, parent,
-				 parents, data->num_mux_parents,
-				 CLK_SET_RATE_PARENT,
-				 cprman->regs + data->ctl_reg,
-				 CM_SRC_SHIFT, CM_SRC_BITS,
-				 0, &cprman->regs_lock);
-	} else {
-		parent = data->parents[0];
+	for (i = 0; i < data->num_mux_parents; i++) {
+		if (strcmp(data->parents[i], "xosc") == 0)
+			parents[i] = cprman->osc_name;
+		else
+			parents[i] = data->parents[i];
 	}
 
 	memset(&init, 0, sizeof(init));
-	init.parent_names = &parent;
-	init.num_parents = 1;
+	init.parent_names = parents;
+	init.num_parents = data->num_mux_parents;
 	init.name = data->name;
 	init.flags = CLK_IGNORE_UNUSED;
 
-- 
2.0.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1262823

[Multipart message — attachments visible in raw view] — view raw

Remi Pommarel <repk@triplefau.lt> writes:

> Some bcm2835 clocks used by hardware (like "PWM" or "H264") can have multiple
> parents. These clocks divide the rate of one parent which can be selected by
> setting the proper bits in their clock control register.
>
> Previously all these parents where handled by a mux clock. But a mux clock
> cannot be used because updating clock control register to select parent needs a
> password to be xor'd with the parent index.

Good point.  I previously was doing parent detection from muxes
manually, then simplified to using the generic mux later.  I didn't have
any clocks I wanted to change mux on, so I missed this requirement.

It looks like there's not too much work to folding the muxing back into
the driver, so it seems like you have a good plan.

> -static long bcm2835_clock_round_rate(struct clk_hw *hw,
> -				     unsigned long rate,
> -				     unsigned long *parent_rate)
> -{
> -	struct bcm2835_clock *clock = bcm2835_clock_from_hw(hw);
> -	u32 div = bcm2835_clock_choose_div(hw, rate, *parent_rate);
> -
> -	return bcm2835_clock_rate_from_divisor(clock, *parent_rate, div);
> -}
> -
>  static unsigned long bcm2835_clock_get_rate(struct clk_hw *hw,
>  					    unsigned long parent_rate)
>  {
> @@ -1278,13 +1268,69 @@ static int bcm2835_clock_set_rate(struct clk_hw *hw,
>  	return 0;
>  }
>  
> +static int bcm2835_clock_determine_source(struct clk_hw *hw,
> +		struct clk_rate_request *req)
> +{
> +	struct clk_hw *parent, *best_parent = NULL;
> +	struct clk_rate_request parent_req;
> +	unsigned long prate, best_rate = ULONG_MAX;
> +	size_t i;
> +
> +	/*
> +	 * Select parent clock that has the closest but higher rate
> +	 */
> +	for (i = 0; i < clk_hw_get_num_parents(hw); ++i) {
> +		parent = clk_hw_get_parent_by_index(hw, i);
> +		if (!parent)
> +			continue;
> +		parent_req = *req;
> +		prate = clk_hw_get_rate(parent);
> +		if (prate < best_rate && prate >= req->rate) {
> +			best_parent = parent;
> +			best_rate = prate;
> +		}
> +	}
> +
> +	if (!best_parent)
> +		return -EINVAL;
> +
> +	req->best_parent_hw = best_parent;
> +	req->best_parent_rate = best_rate;
> +
> +	return 0;
> +}

It looks like you've dropped the use of the divisor off of the PLL
channel when setting a rate.  That seems bad for all the other clocks in
the system, and a feature we couldn't lose.

Also, you're choosing the lowest but higher rate, while
mux_is_better_rate() chooses the highest but lower rate (which seems
much safer).  What led to that choice?

Also, if we're going to have this function, I think it should be called
"bcm2835_clock_determine_rate" to match the method name.

The parent get/setting looks good, though.

[toc] | [prev] | [next] | [standalone]

#1263486

Hi,

On Wed, Nov 04, 2015 at 06:03:31PM -0800, Eric Anholt wrote:

[...]

> 
> It looks like you've dropped the use of the divisor off of the PLL
> channel when setting a rate.  That seems bad for all the other clocks in
> the system, and a feature we couldn't lose.

Sorry, but I'm not sure to understand your point here. Are you afraid
that clocks such as PWM, H264, etc, have lost the ability to divide the
rate from the PLL or oscillator clock they cosume as source ?

If so, I think it's ok. If I'm not wrong here, clk_set_rate() first
calls clk->determinate_rate() then calls clk->set_rate(). This patch
makes bcm2835_clock_determine_source() to only select the parent to use
and does not set the clock's rate itself. The clock's rate is set later
on when bcm2835_clock_set_parent() is called.

bcm2835_clock_set_parent() still divides the parent rate so we are not
loosing this feature here.

> 
> Also, you're choosing the lowest but higher rate, while
> mux_is_better_rate() chooses the highest but lower rate (which seems
> much safer).  What led to that choice?

bcm2835_clock_determine_source() does not choose the rate for the clock
itself but only selects the parent to use as source that will be divided
later on. So I'm choosing a parent that has higher rate because I want
to divide this rate in bcm2835_clock_set_parent().

> 
> Also, if we're going to have this function, I think it should be called
> "bcm2835_clock_determine_rate" to match the method name.
> 

I have called it this way because this function only select the
parent/source to use for the PWM clock and is not really determinating
the clock's rate.


On second thought, I see that doing things this way can be confusing,
and is not a really safe way to choose a clock's rate.

It would probably be better to have bcm2835_clock_determine_source()
selects the parent by choosing the one that provides the rate which,
after being divided, generates the highest but lower rate out of the
PWM clock itself.

Moreover, if you agree with the above modification I see no reason to
not call it "bcm2835_clock_determine_rate"


Thanks

-- 
Remi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1265840

[Multipart message — attachments visible in raw view] — view raw

Remi Pommarel <repk@triplefau.lt> writes:

> Hi,
>
> On Wed, Nov 04, 2015 at 06:03:31PM -0800, Eric Anholt wrote:
>
> [...]
>
>> 
>> It looks like you've dropped the use of the divisor off of the PLL
>> channel when setting a rate.  That seems bad for all the other clocks in
>> the system, and a feature we couldn't lose.
>
> Sorry, but I'm not sure to understand your point here. Are you afraid
> that clocks such as PWM, H264, etc, have lost the ability to divide the
> rate from the PLL or oscillator clock they cosume as source ?
>
> If so, I think it's ok. If I'm not wrong here, clk_set_rate() first
> calls clk->determinate_rate() then calls clk->set_rate(). This patch
> makes bcm2835_clock_determine_source() to only select the parent to use
> and does not set the clock's rate itself. The clock's rate is set later
> on when bcm2835_clock_set_parent() is called.
>
> bcm2835_clock_set_parent() still divides the parent rate so we are not
> loosing this feature here.

I see.  You're leaving req->rate as-is, so that it gets passed back in
on the set_rate() call.  Since you've chosen only a parent with a
rate greater than ours, we know we'll be able to divide into it.

This has the downside that anything using the min/max rate clamping
doesn't get to know before setting that we might be out of bounds.

> It would probably be better to have bcm2835_clock_determine_source()
> selects the parent by choosing the one that provides the rate which,
> after being divided, generates the highest but lower rate out of the
> PWM clock itself.
>
> Moreover, if you agree with the above modification I see no reason to
> not call it "bcm2835_clock_determine_rate"

This sounds like what the function should be doing.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web