[PATCH 0/2] Add PWM clock support for bcm2835

[PATCH 0/2] Add PWM clock support for bcm2835

[Remi Pommarel]

Hi,

This patchset add support for pwm clock. At boot, this clock does not have a
default parent nor a default rate set. Thus we should be able to change its
parent to get this clock working. The current clock implementation is using a
mux to select the parent, but these clocks need to add a password (0x5a) in
higher register bits when changing parent. So a generic mux cannot be used
here.

The first patch fix the clock parent selection, while the second one is actually
adding the pwm clock registration.

Remi Pommarel (2):
  clk: bcm2835: Support for clock parent selection
  clk: bcm2835: Add PWM clock support

 arch/arm/boot/dts/bcm2835.dtsi      |   8 ++
 drivers/clk/bcm/clk-bcm2835.c       | 144 ++++++++++++++++++++++++------------
 include/dt-bindings/clock/bcm2835.h |   3 +-
 3 files changed, 108 insertions(+), 47 deletions(-)

-- 
2.0.1

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

[Remi Pommarel]

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

[PATCH 2/2] clk: bcm2835: Add PWM clock support

[Remi Pommarel]

Register the pwm clock for bcm2835.
This patch also adds the ability to set a clock default rate.

Signed-off-by: Remi Pommarel <repk@triplefau.lt>
---
 arch/arm/boot/dts/bcm2835.dtsi      |  8 ++++++++
 drivers/clk/bcm/clk-bcm2835.c       | 28 +++++++++++++++++++++++++++-
 include/dt-bindings/clock/bcm2835.h |  3 ++-
 3 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
index aef64de..0736de3 100644
--- a/arch/arm/boot/dts/bcm2835.dtsi
+++ b/arch/arm/boot/dts/bcm2835.dtsi
@@ -177,6 +177,14 @@
 			status = "disabled";
 		};
 
+		pwm: pwm@7e20c000 {
+			compatible = "brcm,bcm2835-pwm";
+			reg = <0x7e20c000 0x28>;
+			clocks = <&clocks BCM2835_CLOCK_PWM>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
 		usb@7e980000 {
 			compatible = "brcm,bcm2835-usb";
 			reg = <0x7e980000 0x10000>;
diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index 9469729..0647118 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -39,6 +39,7 @@
 
 #include <linux/clk-provider.h>
 #include <linux/clkdev.h>
+#include <linux/clk.h>
 #include <linux/clk/bcm2835.h>
 #include <linux/module.h>
 #include <linux/of.h>
@@ -625,6 +626,8 @@ struct bcm2835_clock_data {
 	const char *const *parents;
 	int num_mux_parents;
 
+	unsigned long dft_rate;
+
 	u32 ctl_reg;
 	u32 div_reg;
 
@@ -807,6 +810,17 @@ static const struct bcm2835_clock_data bcm2835_clock_emmc_data = {
 	.frac_bits = 8,
 };
 
+static const struct bcm2835_clock_data bcm2835_clock_pwm_data = {
+	.name = "pwm",
+	.num_mux_parents = ARRAY_SIZE(bcm2835_clock_per_parents),
+	.parents = bcm2835_clock_per_parents,
+	.dft_rate = 9600000,
+	.ctl_reg = CM_PWMCTL,
+	.div_reg = CM_PWMDIV,
+	.int_bits = 12,
+	.frac_bits = 12,
+};
+
 struct bcm2835_pll {
 	struct clk_hw hw;
 	struct bcm2835_cprman *cprman;
@@ -1440,6 +1454,7 @@ bcm2835_register_pll_divider(struct bcm2835_cprman *cprman,
 static struct clk *bcm2835_register_clock(struct bcm2835_cprman *cprman,
 					  const struct bcm2835_clock_data *data)
 {
+	struct clk *ret;
 	struct bcm2835_clock *clock;
 	struct clk_init_data init;
 	const char *parents[1 << CM_SRC_BITS];
@@ -1477,7 +1492,15 @@ static struct clk *bcm2835_register_clock(struct bcm2835_cprman *cprman,
 	clock->data = data;
 	clock->hw.init = &init;
 
-	return devm_clk_register(cprman->dev, &clock->hw);
+	ret = devm_clk_register(cprman->dev, &clock->hw);
+	if (IS_ERR(ret))
+		goto out;
+
+	if (data->dft_rate)
+		clk_set_rate(ret, data->dft_rate);
+
+out:
+	return ret;
 }
 
 static int bcm2835_clk_probe(struct platform_device *pdev)
@@ -1576,6 +1599,9 @@ static int bcm2835_clk_probe(struct platform_device *pdev)
 				  cprman->regs + CM_PERIICTL, CM_GATE_BIT,
 				  0, &cprman->regs_lock);
 
+	clks[BCM2835_CLOCK_PWM] =
+		bcm2835_register_clock(cprman, &bcm2835_clock_pwm_data);
+
 	return of_clk_add_provider(dev->of_node, of_clk_src_onecell_get,
 				   &cprman->onecell);
 }
diff --git a/include/dt-bindings/clock/bcm2835.h b/include/dt-bindings/clock/bcm2835.h
index d323efa..61f1d20 100644
--- a/include/dt-bindings/clock/bcm2835.h
+++ b/include/dt-bindings/clock/bcm2835.h
@@ -43,5 +43,6 @@
 #define BCM2835_CLOCK_TSENS		27
 #define BCM2835_CLOCK_EMMC		28
 #define BCM2835_CLOCK_PERI_IMAGE	29
+#define BCM2835_CLOCK_PWM		30
 
-#define BCM2835_CLOCK_COUNT		30
+#define BCM2835_CLOCK_COUNT		31
-- 
2.0.1

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

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 2764 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 2/2] clk: bcm2835: Add PWM clock support

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 4770 bytes --]

Remi Pommarel <repk@triplefau.lt> writes:

> Register the pwm clock for bcm2835.
> This patch also adds the ability to set a clock default rate.

I don't think we should be setting a default clock rate.  That should be
up to the thing that uses the clock.  If we need a standard rate set on
all Raspberry Pis, other than what is set at boot, then we could put it
in the bcm2835-pwm dt block (I think the "Assigned clock parents and
rates" part of clock-bindings.txt gives a way to do so).

> Signed-off-by: Remi Pommarel <repk@triplefau.lt>
> ---
>  arch/arm/boot/dts/bcm2835.dtsi      |  8 ++++++++
>  drivers/clk/bcm/clk-bcm2835.c       | 28 +++++++++++++++++++++++++++-
>  include/dt-bindings/clock/bcm2835.h |  3 ++-
>  3 files changed, 37 insertions(+), 2 deletions(-)

Submitting DT changes is super awkward.  You'd need to put the bcm2835.h
and driver change in one patch with this subject.  The clk maintainers
would pull that patch.  You'd then have a second patch that covers just
the .dtsi change, which I would pull once I had a stable branch to put
it onto that had the bcm2835.h change.

> diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
> index aef64de..0736de3 100644
> --- a/arch/arm/boot/dts/bcm2835.dtsi
> +++ b/arch/arm/boot/dts/bcm2835.dtsi
> @@ -177,6 +177,14 @@
>  			status = "disabled";
>  		};
>  
> +		pwm: pwm@7e20c000 {
> +			compatible = "brcm,bcm2835-pwm";
> +			reg = <0x7e20c000 0x28>;
> +			clocks = <&clocks BCM2835_CLOCK_PWM>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +		};
> +
>  		usb@7e980000 {
>  			compatible = "brcm,bcm2835-usb";
>  			reg = <0x7e980000 0x10000>;

> diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
> index 9469729..0647118 100644
> --- a/drivers/clk/bcm/clk-bcm2835.c
> +++ b/drivers/clk/bcm/clk-bcm2835.c
> @@ -39,6 +39,7 @@
>  
>  #include <linux/clk-provider.h>
>  #include <linux/clkdev.h>
> +#include <linux/clk.h>
>  #include <linux/clk/bcm2835.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> @@ -625,6 +626,8 @@ struct bcm2835_clock_data {
>  	const char *const *parents;
>  	int num_mux_parents;
>  
> +	unsigned long dft_rate;
> +
>  	u32 ctl_reg;
>  	u32 div_reg;
>  
> @@ -807,6 +810,17 @@ static const struct bcm2835_clock_data bcm2835_clock_emmc_data = {
>  	.frac_bits = 8,
>  };
>  
> +static const struct bcm2835_clock_data bcm2835_clock_pwm_data = {
> +	.name = "pwm",
> +	.num_mux_parents = ARRAY_SIZE(bcm2835_clock_per_parents),
> +	.parents = bcm2835_clock_per_parents,
> +	.dft_rate = 9600000,
> +	.ctl_reg = CM_PWMCTL,
> +	.div_reg = CM_PWMDIV,
> +	.int_bits = 12,
> +	.frac_bits = 12,
> +};
> +
>  struct bcm2835_pll {
>  	struct clk_hw hw;
>  	struct bcm2835_cprman *cprman;
> @@ -1440,6 +1454,7 @@ bcm2835_register_pll_divider(struct bcm2835_cprman *cprman,
>  static struct clk *bcm2835_register_clock(struct bcm2835_cprman *cprman,
>  					  const struct bcm2835_clock_data *data)
>  {
> +	struct clk *ret;
>  	struct bcm2835_clock *clock;
>  	struct clk_init_data init;
>  	const char *parents[1 << CM_SRC_BITS];
> @@ -1477,7 +1492,15 @@ static struct clk *bcm2835_register_clock(struct bcm2835_cprman *cprman,
>  	clock->data = data;
>  	clock->hw.init = &init;
>  
> -	return devm_clk_register(cprman->dev, &clock->hw);
> +	ret = devm_clk_register(cprman->dev, &clock->hw);
> +	if (IS_ERR(ret))
> +		goto out;
> +
> +	if (data->dft_rate)
> +		clk_set_rate(ret, data->dft_rate);
> +
> +out:
> +	return ret;
>  }
>  
>  static int bcm2835_clk_probe(struct platform_device *pdev)
> @@ -1576,6 +1599,9 @@ static int bcm2835_clk_probe(struct platform_device *pdev)
>  				  cprman->regs + CM_PERIICTL, CM_GATE_BIT,
>  				  0, &cprman->regs_lock);
>  
> +	clks[BCM2835_CLOCK_PWM] =
> +		bcm2835_register_clock(cprman, &bcm2835_clock_pwm_data);
> +
>  	return of_clk_add_provider(dev->of_node, of_clk_src_onecell_get,
>  				   &cprman->onecell);
>  }
> diff --git a/include/dt-bindings/clock/bcm2835.h b/include/dt-bindings/clock/bcm2835.h
> index d323efa..61f1d20 100644
> --- a/include/dt-bindings/clock/bcm2835.h
> +++ b/include/dt-bindings/clock/bcm2835.h
> @@ -43,5 +43,6 @@
>  #define BCM2835_CLOCK_TSENS		27
>  #define BCM2835_CLOCK_EMMC		28
>  #define BCM2835_CLOCK_PERI_IMAGE	29
> +#define BCM2835_CLOCK_PWM		30
>  
> -#define BCM2835_CLOCK_COUNT		30
> +#define BCM2835_CLOCK_COUNT		31
> -- 
> 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/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

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

[Remi Pommarel]

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

Re: [PATCH 2/2] clk: bcm2835: Add PWM clock support

[Remi Pommarel]

On Wed, Nov 04, 2015 at 06:17:38PM -0800, Eric Anholt wrote:
> Remi Pommarel <repk@triplefau.lt> writes:
> 
> > Register the pwm clock for bcm2835.
> > This patch also adds the ability to set a clock default rate.
> 
> I don't think we should be setting a default clock rate.  That should be
> up to the thing that uses the clock.  If we need a standard rate set on
> all Raspberry Pis, other than what is set at boot, then we could put it
> in the bcm2835-pwm dt block (I think the "Assigned clock parents and
> rates" part of clock-bindings.txt gives a way to do so).
> 

Oh I didn't know about assigned-clocks-{rates,parent}, it sure is a
better solution, thanks.

> > Signed-off-by: Remi Pommarel <repk@triplefau.lt>
> > ---
> >  arch/arm/boot/dts/bcm2835.dtsi      |  8 ++++++++
> >  drivers/clk/bcm/clk-bcm2835.c       | 28 +++++++++++++++++++++++++++-
> >  include/dt-bindings/clock/bcm2835.h |  3 ++-
> >  3 files changed, 37 insertions(+), 2 deletions(-)
> 
> Submitting DT changes is super awkward.  You'd need to put the bcm2835.h
> and driver change in one patch with this subject.  The clk maintainers
> would pull that patch.  You'd then have a second patch that covers just
> the .dtsi change, which I would pull once I had a stable branch to put
> it onto that had the bcm2835.h change.
> 

Ok will do, sorry about this.

[...]

Thanks.

-- 
Remi

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

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 1765 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]