[PATCH 0/3] sunxi: unify main oscillator and its gate

[PATCH 0/3] sunxi: unify main oscillator and its gate

[Emilio López]

Hi Mike,

As discussed previously when introducing the sunxi driver, this patchset
unifies the main oscillator ("osc24M_fixed") and its gate ("osc24M")
into a single clock ("osc24M"). It does so by adding fixed-rate support
into the composite clock and then using it to implement the new clock.

The changes shouldn't require any modifications on current/future users 
of the composite clock, although I haven't tested any to make sure of 
it. @Prashant, can I get a tested-by from you if necessary?

Thanks,

Emilio

Emilio L?pez (3):
  clk: composite: Add fixed-rate support
  clk: sunxi: Unify oscillator clock
  ARM: sunxi: unify osc24M_fixed and osc24M

 arch/arm/boot/dts/sunxi.dtsi  |  8 +-----
 drivers/clk/clk-composite.c   | 57 ++++++++++++++++++++++++++++++++++++++++---
 drivers/clk/sunxi/clk-sunxi.c | 30 +++++++++++++++++------
 include/linux/clk-provider.h  |  6 +++++
 4 files changed, 83 insertions(+), 18 deletions(-)

-- 
1.8.2.1

[PATCH 1/3] clk: composite: Add fixed-rate support

[Emilio López]

This patchset adds fixed-rate support to the composite clock, allowing
us to register gatable oscillators.

Signed-off-by: Emilio L?pez <emilio@elopez.com.ar>
---
 drivers/clk/clk-composite.c  | 57 ++++++++++++++++++++++++++++++++++++++++----
 include/linux/clk-provider.h |  6 +++++
 2 files changed, 59 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
index 097dee4..5416e1d 100644
--- a/drivers/clk/clk-composite.c
+++ b/drivers/clk/clk-composite.c
@@ -43,8 +43,8 @@ static int clk_composite_set_parent(struct clk_hw *hw, u8 index)
 	return mux_ops->set_parent(mux_hw, index);
 }
 
-static unsigned long clk_composite_recalc_rate(struct clk_hw *hw,
-					    unsigned long parent_rate)
+static unsigned long clk_composite_recalc_rate_div(struct clk_hw *hw,
+						   unsigned long parent_rate)
 {
 	struct clk_composite *composite = to_clk_composite(hw);
 	const struct clk_ops *div_ops = composite->div_ops;
@@ -55,6 +55,18 @@ static unsigned long clk_composite_recalc_rate(struct clk_hw *hw,
 	return div_ops->recalc_rate(div_hw, parent_rate);
 }
 
+static unsigned long clk_composite_recalc_rate_fixed(struct clk_hw *hw,
+						     unsigned long parent_rate)
+{
+	struct clk_composite *composite = to_clk_composite(hw);
+	const struct clk_ops *fixed_ops = composite->fixed_ops;
+	struct clk_hw *fixed_hw = composite->fixed_hw;
+
+	fixed_hw->clk = hw->clk;
+
+	return fixed_ops->recalc_rate(fixed_hw, parent_rate);
+}
+
 static long clk_composite_round_rate(struct clk_hw *hw, unsigned long rate,
 				  unsigned long *prate)
 {
@@ -112,11 +124,12 @@ static void clk_composite_disable(struct clk_hw *hw)
 	gate_ops->disable(gate_hw);
 }
 
-struct clk *clk_register_composite(struct device *dev, const char *name,
+static struct clk *_clk_register_composite(struct device *dev, const char *name,
 			const char **parent_names, int num_parents,
 			struct clk_hw *mux_hw, const struct clk_ops *mux_ops,
 			struct clk_hw *div_hw, const struct clk_ops *div_ops,
 			struct clk_hw *gate_hw, const struct clk_ops *gate_ops,
+			struct clk_hw *fixed_hw, const struct clk_ops *fixed_ops,
 			unsigned long flags)
 {
 	struct clk *clk;
@@ -158,7 +171,7 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
 
 		composite->div_hw = div_hw;
 		composite->div_ops = div_ops;
-		clk_composite_ops->recalc_rate = clk_composite_recalc_rate;
+		clk_composite_ops->recalc_rate = clk_composite_recalc_rate_div;
 		clk_composite_ops->round_rate = clk_composite_round_rate;
 		clk_composite_ops->set_rate = clk_composite_set_rate;
 	}
@@ -177,6 +190,17 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
 		clk_composite_ops->disable = clk_composite_disable;
 	}
 
+	if (fixed_hw && fixed_ops) {
+		if (!fixed_ops->recalc_rate) {
+			clk = ERR_PTR(-EINVAL);
+			goto err;
+		}
+
+		composite->fixed_hw = fixed_hw;
+		composite->fixed_ops = fixed_ops;
+		clk_composite_ops->recalc_rate = clk_composite_recalc_rate_fixed;
+	}
+
 	init.ops = clk_composite_ops;
 	composite->hw.init = &init;
 
@@ -193,9 +217,34 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
 	if (composite->gate_hw)
 		composite->gate_hw->clk = clk;
 
+	if (composite->fixed_hw)
+		composite->fixed_hw->clk = clk;
+
 	return clk;
 
 err:
 	kfree(composite);
 	return clk;
 }
+
+struct clk *clk_register_composite(struct device *dev, const char *name,
+			const char **parent_names, int num_parents,
+			struct clk_hw *mux_hw, const struct clk_ops *mux_ops,
+			struct clk_hw *div_hw, const struct clk_ops *div_ops,
+			struct clk_hw *gate_hw, const struct clk_ops *gate_ops,
+			unsigned long flags)
+{
+	return _clk_register_composite(dev, name, parent_names, num_parents,
+				       mux_hw, mux_ops, div_hw, div_ops,
+				       gate_hw, gate_ops, NULL, NULL, flags);
+}
+
+struct clk *clk_register_gatable_osc(struct device *dev, const char *name,
+			struct clk_hw *fixed_hw, const struct clk_ops *fixed_ops,
+			struct clk_hw *gate_hw, const struct clk_ops *gate_ops,
+			unsigned long flags)
+{
+	return _clk_register_composite(dev, name, NULL, 0, NULL, NULL,
+				       NULL, NULL, gate_hw, gate_ops,
+				       fixed_hw, fixed_ops, flags);
+}
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 33e7e64..82dd006 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -343,10 +343,12 @@ struct clk_composite {
 	struct clk_hw	*mux_hw;
 	struct clk_hw	*div_hw;
 	struct clk_hw	*gate_hw;
+	struct clk_hw	*fixed_hw;
 
 	const struct clk_ops	*mux_ops;
 	const struct clk_ops	*div_ops;
 	const struct clk_ops	*gate_ops;
+	const struct clk_ops	*fixed_ops;
 };
 
 struct clk *clk_register_composite(struct device *dev, const char *name,
@@ -356,6 +358,10 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
 		struct clk_hw *gate_hw, const struct clk_ops *gate_ops,
 		unsigned long flags);
 
+struct clk *clk_register_gatable_osc(struct device *dev, const char *name,
+		struct clk_hw *fixed_hw, const struct clk_ops *fixed_ops,
+		struct clk_hw *gate_hw, const struct clk_ops *gate_ops,
+		unsigned long flags);
 /**
  * clk_register - allocate a new clock, register it and return an opaque cookie
  * @dev: device that is registering this clock
-- 
1.8.2.1

[PATCH 2/3] clk: sunxi: Unify oscillator clock

[Emilio López]

This commit uses the new fixed-rate support on the composite clock to
unify osc24M_fixed and osc24M clocks, so it matches the actual hardware.

Signed-off-by: Emilio L?pez <emilio@elopez.com.ar>
---
 drivers/clk/sunxi/clk-sunxi.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
index 0bb0eb4..e072bd8 100644
--- a/drivers/clk/sunxi/clk-sunxi.c
+++ b/drivers/clk/sunxi/clk-sunxi.c
@@ -33,16 +33,33 @@ static DEFINE_SPINLOCK(clk_lock);
 static void __init sunxi_osc_clk_setup(struct device_node *node)
 {
 	struct clk *clk;
+	struct clk_fixed_rate *fixed;
+	struct clk_gate *gate;
 	const char *clk_name = node->name;
-	const char *parent;
-	void *reg;
+	u32 rate;
 
-	reg = of_iomap(node, 0);
+	/* allocate fixed-rate and gate clock structs */
+	fixed = kzalloc(sizeof(struct clk_fixed_rate), GFP_KERNEL);
+	if (!fixed)
+		return;
+	gate = kzalloc(sizeof(struct clk_gate), GFP_KERNEL);
+	if (!gate) {
+		kfree(fixed);
+		return;
+	}
 
-	parent = of_clk_get_parent_name(node, 0);
+	if (of_property_read_u32(node, "clock-frequency", &rate))
+		return;
+
+	/* set up gate and fixed rate properties */
+	gate->reg = of_iomap(node, 0);
+	gate->bit_idx = SUNXI_OSC24M_GATE;
+	gate->lock = &clk_lock;
+	fixed->fixed_rate = rate;
 
-	clk = clk_register_gate(NULL, clk_name, parent, 0, reg,
-				SUNXI_OSC24M_GATE, 0, &clk_lock);
+	clk = clk_register_gatable_osc(NULL, clk_name, &fixed->hw,
+				       &clk_fixed_rate_ops, &gate->hw,
+				       &clk_gate_ops, CLK_IS_ROOT);
 
 	if (clk) {
 		of_clk_add_provider(node, of_clk_src_simple_get, clk);
@@ -380,7 +397,6 @@ static void __init sunxi_gates_clk_setup(struct device_node *node,
 
 /* Matches for of_clk_init */
 static const __initconst struct of_device_id clk_match[] = {
-	{.compatible = "fixed-clock", .data = of_fixed_clk_setup,},
 	{.compatible = "allwinner,sun4i-osc-clk", .data = sunxi_osc_clk_setup,},
 	{}
 };
-- 
1.8.2.1

[PATCH 3/3] ARM: sunxi: unify osc24M_fixed and osc24M

[Emilio López]

Now that the clock driver supports the gatable oscillator as one single
clock, drop osc24M_fixed and move the relevant properties to osc24M

Signed-off-by: Emilio L?pez <emilio@elopez.com.ar>
---
 arch/arm/boot/dts/sunxi.dtsi | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/arch/arm/boot/dts/sunxi.dtsi b/arch/arm/boot/dts/sunxi.dtsi
index a8d47e2..2309272 100644
--- a/arch/arm/boot/dts/sunxi.dtsi
+++ b/arch/arm/boot/dts/sunxi.dtsi
@@ -39,17 +39,11 @@
 			clock-frequency = <0>;
 		};
 
-		osc24M_fixed: osc24M_fixed {
-			#clock-cells = <0>;
-			compatible = "fixed-clock";
-			clock-frequency = <24000000>;
-		};
-
 		osc24M: osc24M at 01c20050 {
 			#clock-cells = <0>;
 			compatible = "allwinner,sun4i-osc-clk";
+			clock-frequency = <24000000>;
 			reg = <0x01c20050 0x4>;
-			clocks = <&osc24M_fixed>;
 		};
 
 		osc32k: osc32k {
-- 
1.8.2.1

[PATCH 0/3] sunxi: unify main oscillator and its gate

[Gregory CLEMENT]

Hi Emilio,

On 04/09/2013 03:48 PM, Emilio L?pez wrote:
> Hi Mike,
> 
> As discussed previously when introducing the sunxi driver, this patchset
> unifies the main oscillator ("osc24M_fixed") and its gate ("osc24M")
> into a single clock ("osc24M"). It does so by adding fixed-rate support
> into the composite clock and then using it to implement the new clock.
> 
> The changes shouldn't require any modifications on current/future users 
> of the composite clock, although I haven't tested any to make sure of 
> it. @Prashant, can I get a tested-by from you if necessary?

This patch set is a nice improvement, now we will have the best of the
two worlds: a 1-to-1 between the clock device in the device tree and the
real hardware clock on one side, and the reuse of existing driver clock
on the other side.

Could you indicate on which branch this patch set can be applied ? In
3.9-rc6 there is no file drivers/clk/clk-composite.c. I guess it should
be the something like clk-next or clk-for-3.10 from Mike's git tree.

Adding the branch where to apply the patch set is a good information to
add to the cover letter of a patch set.

Thanks,

> 
> Thanks,
> 
> Emilio
> 
> Emilio L??pez (3):
>   clk: composite: Add fixed-rate support
>   clk: sunxi: Unify oscillator clock
>   ARM: sunxi: unify osc24M_fixed and osc24M
> 
>  arch/arm/boot/dts/sunxi.dtsi  |  8 +-----
>  drivers/clk/clk-composite.c   | 57 ++++++++++++++++++++++++++++++++++++++++---
>  drivers/clk/sunxi/clk-sunxi.c | 30 +++++++++++++++++------
>  include/linux/clk-provider.h  |  6 +++++
>  4 files changed, 83 insertions(+), 18 deletions(-)
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/3] sunxi: unify main oscillator and its gate

[Emilio López]

Hi Gregory,

El 09/04/13 11:56, Gregory CLEMENT escribi?:
> Hi Emilio,
> 
> On 04/09/2013 03:48 PM, Emilio L?pez wrote:
>> Hi Mike,
>>
>> As discussed previously when introducing the sunxi driver, this patchset
>> unifies the main oscillator ("osc24M_fixed") and its gate ("osc24M")
>> into a single clock ("osc24M"). It does so by adding fixed-rate support
>> into the composite clock and then using it to implement the new clock.
>>
>> The changes shouldn't require any modifications on current/future users 
>> of the composite clock, although I haven't tested any to make sure of 
>> it. @Prashant, can I get a tested-by from you if necessary?
> 
> This patch set is a nice improvement, now we will have the best of the
> two worlds: a 1-to-1 between the clock device in the device tree and the
> real hardware clock on one side, and the reuse of existing driver clock
> on the other side.
> 
> Could you indicate on which branch this patch set can be applied ? In
> 3.9-rc6 there is no file drivers/clk/clk-composite.c. I guess it should
> be the something like clk-next or clk-for-3.10 from Mike's git tree.

Yes, this should be applied on Mike's clk-next / clk-for-3.10, and
Maxime's tree for the DT part. Mike's branches contain both the sunxi
driver and clk-composite.c.

> Adding the branch where to apply the patch set is a good information to
> add to the cover letter of a patch set.

Indeed, I always forget to do so :/

Thanks,

Emilio

[PATCH 1/3] clk: composite: Add fixed-rate support

[Gregory CLEMENT]

On 04/09/2013 03:48 PM, Emilio L?pez wrote:
> This patchset adds fixed-rate support to the composite clock, allowing
> us to register gatable oscillators.
> 
> Signed-off-by: Emilio L??pez <emilio@elopez.com.ar>
> ---
>  drivers/clk/clk-composite.c  | 57 ++++++++++++++++++++++++++++++++++++++++----
>  include/linux/clk-provider.h |  6 +++++
>  2 files changed, 59 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
> index 097dee4..5416e1d 100644
> --- a/drivers/clk/clk-composite.c
> +++ b/drivers/clk/clk-composite.c
> @@ -43,8 +43,8 @@ static int clk_composite_set_parent(struct clk_hw *hw, u8 index)
>  	return mux_ops->set_parent(mux_hw, index);
>  }
>  
> -static unsigned long clk_composite_recalc_rate(struct clk_hw *hw,
> -					    unsigned long parent_rate)
> +static unsigned long clk_composite_recalc_rate_div(struct clk_hw *hw,
> +						   unsigned long parent_rate)
>  {
>  	struct clk_composite *composite = to_clk_composite(hw);
>  	const struct clk_ops *div_ops = composite->div_ops;
> @@ -55,6 +55,18 @@ static unsigned long clk_composite_recalc_rate(struct clk_hw *hw,
>  	return div_ops->recalc_rate(div_hw, parent_rate);
>  }
>  
> +static unsigned long clk_composite_recalc_rate_fixed(struct clk_hw *hw,
> +						     unsigned long parent_rate)
> +{
> +	struct clk_composite *composite = to_clk_composite(hw);
> +	const struct clk_ops *fixed_ops = composite->fixed_ops;
> +	struct clk_hw *fixed_hw = composite->fixed_hw;
> +
> +	fixed_hw->clk = hw->clk;
> +
> +	return fixed_ops->recalc_rate(fixed_hw, parent_rate);
> +}
> +
>  static long clk_composite_round_rate(struct clk_hw *hw, unsigned long rate,
>  				  unsigned long *prate)
>  {
> @@ -112,11 +124,12 @@ static void clk_composite_disable(struct clk_hw *hw)
>  	gate_ops->disable(gate_hw);
>  }
>  
> -struct clk *clk_register_composite(struct device *dev, const char *name,
> +static struct clk *_clk_register_composite(struct device *dev, const char *name,
>  			const char **parent_names, int num_parents,
>  			struct clk_hw *mux_hw, const struct clk_ops *mux_ops,
>  			struct clk_hw *div_hw, const struct clk_ops *div_ops,
>  			struct clk_hw *gate_hw, const struct clk_ops *gate_ops,
> +			struct clk_hw *fixed_hw, const struct clk_ops *fixed_ops,
>  			unsigned long flags)
>  {
>  	struct clk *clk;
> @@ -158,7 +171,7 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
>  
>  		composite->div_hw = div_hw;
>  		composite->div_ops = div_ops;
> -		clk_composite_ops->recalc_rate = clk_composite_recalc_rate;
> +		clk_composite_ops->recalc_rate = clk_composite_recalc_rate_div;
>  		clk_composite_ops->round_rate = clk_composite_round_rate;
>  		clk_composite_ops->set_rate = clk_composite_set_rate;
>  	}
> @@ -177,6 +190,17 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
>  		clk_composite_ops->disable = clk_composite_disable;
>  	}
>  
> +	if (fixed_hw && fixed_ops) {
> +		if (!fixed_ops->recalc_rate) {
> +			clk = ERR_PTR(-EINVAL);
> +			goto err;
> +		}
> +
> +		composite->fixed_hw = fixed_hw;
> +		composite->fixed_ops = fixed_ops;
> +		clk_composite_ops->recalc_rate = clk_composite_recalc_rate_fixed;
> +	}
> +
>  	init.ops = clk_composite_ops;
>  	composite->hw.init = &init;
>  
> @@ -193,9 +217,34 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
>  	if (composite->gate_hw)
>  		composite->gate_hw->clk = clk;
>  
> +	if (composite->fixed_hw)
> +		composite->fixed_hw->clk = clk;
> +
>  	return clk;
>  
>  err:
>  	kfree(composite);
>  	return clk;
>  }
> +
> +struct clk *clk_register_composite(struct device *dev, const char *name,
> +			const char **parent_names, int num_parents,
> +			struct clk_hw *mux_hw, const struct clk_ops *mux_ops,
> +			struct clk_hw *div_hw, const struct clk_ops *div_ops,
> +			struct clk_hw *gate_hw, const struct clk_ops *gate_ops,
> +			unsigned long flags)
> +{
> +	return _clk_register_composite(dev, name, parent_names, num_parents,
> +				       mux_hw, mux_ops, div_hw, div_ops,
> +				       gate_hw, gate_ops, NULL, NULL, flags);
> +}
> +
> +struct clk *clk_register_gatable_osc(struct device *dev, const char *name,
> +			struct clk_hw *fixed_hw, const struct clk_ops *fixed_ops,
> +			struct clk_hw *gate_hw, const struct clk_ops *gate_ops,
> +			unsigned long flags)
> +{
> +	return _clk_register_composite(dev, name, NULL, 0, NULL, NULL,
> +				       NULL, NULL, gate_hw, gate_ops,
> +				       fixed_hw, fixed_ops, flags);
> +}

I think you should remove all this chunk, and made your change in
clk_register_composite() instead of using _clk_register_composite(). I don't
think it is a good thing that each kind of composition was declared here.

The way you composed your clock should be done by each driver.

I understand that you didn't want to break the existing driver, but in this case
it is better to do the modification in the driver to follow the new API.
The change is pretty trivial and can be automated by using spatch aka semantic patch
aka coccinelle. But as this API is very new you could also do it by hand, however I
didn't find any occurrences of this function in the branches clk-next or clk-for-3.10.
I only found one occurrence in  the  in patch "clk: tegra30: Convert clk out to
composite clk" from Prashant Gaikwad. Unfortunately I didn't find a tree with this
commit applied.


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 1/3] clk: composite: Add fixed-rate support

[Emilio López]

Hi Gregory,

El 09/04/13 12:41, Gregory CLEMENT escribi?:
> On 04/09/2013 03:48 PM, Emilio L?pez wrote:
>> This patchset adds fixed-rate support to the composite clock, allowing
>> us to register gatable oscillators.
>>
>> Signed-off-by: Emilio L??pez <emilio@elopez.com.ar>
>> ---
>>  drivers/clk/clk-composite.c  | 57 ++++++++++++++++++++++++++++++++++++++++----
>>  include/linux/clk-provider.h |  6 +++++
>>  2 files changed, 59 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
>> index 097dee4..5416e1d 100644
>> --- a/drivers/clk/clk-composite.c
>> +++ b/drivers/clk/clk-composite.c
>> @@ -43,8 +43,8 @@ static int clk_composite_set_parent(struct clk_hw *hw, u8 index)
>>  	return mux_ops->set_parent(mux_hw, index);
>>  }
>>  
>> -static unsigned long clk_composite_recalc_rate(struct clk_hw *hw,
>> -					    unsigned long parent_rate)
>> +static unsigned long clk_composite_recalc_rate_div(struct clk_hw *hw,
>> +						   unsigned long parent_rate)
>>  {
>>  	struct clk_composite *composite = to_clk_composite(hw);
>>  	const struct clk_ops *div_ops = composite->div_ops;
>> @@ -55,6 +55,18 @@ static unsigned long clk_composite_recalc_rate(struct clk_hw *hw,
>>  	return div_ops->recalc_rate(div_hw, parent_rate);
>>  }
>>  
>> +static unsigned long clk_composite_recalc_rate_fixed(struct clk_hw *hw,
>> +						     unsigned long parent_rate)
>> +{
>> +	struct clk_composite *composite = to_clk_composite(hw);
>> +	const struct clk_ops *fixed_ops = composite->fixed_ops;
>> +	struct clk_hw *fixed_hw = composite->fixed_hw;
>> +
>> +	fixed_hw->clk = hw->clk;
>> +
>> +	return fixed_ops->recalc_rate(fixed_hw, parent_rate);
>> +}
>> +
>>  static long clk_composite_round_rate(struct clk_hw *hw, unsigned long rate,
>>  				  unsigned long *prate)
>>  {
>> @@ -112,11 +124,12 @@ static void clk_composite_disable(struct clk_hw *hw)
>>  	gate_ops->disable(gate_hw);
>>  }
>>  
>> -struct clk *clk_register_composite(struct device *dev, const char *name,
>> +static struct clk *_clk_register_composite(struct device *dev, const char *name,
>>  			const char **parent_names, int num_parents,
>>  			struct clk_hw *mux_hw, const struct clk_ops *mux_ops,
>>  			struct clk_hw *div_hw, const struct clk_ops *div_ops,
>>  			struct clk_hw *gate_hw, const struct clk_ops *gate_ops,
>> +			struct clk_hw *fixed_hw, const struct clk_ops *fixed_ops,
>>  			unsigned long flags)
>>  {
>>  	struct clk *clk;
>> @@ -158,7 +171,7 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
>>  
>>  		composite->div_hw = div_hw;
>>  		composite->div_ops = div_ops;
>> -		clk_composite_ops->recalc_rate = clk_composite_recalc_rate;
>> +		clk_composite_ops->recalc_rate = clk_composite_recalc_rate_div;
>>  		clk_composite_ops->round_rate = clk_composite_round_rate;
>>  		clk_composite_ops->set_rate = clk_composite_set_rate;
>>  	}
>> @@ -177,6 +190,17 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
>>  		clk_composite_ops->disable = clk_composite_disable;
>>  	}
>>  
>> +	if (fixed_hw && fixed_ops) {
>> +		if (!fixed_ops->recalc_rate) {
>> +			clk = ERR_PTR(-EINVAL);
>> +			goto err;
>> +		}
>> +
>> +		composite->fixed_hw = fixed_hw;
>> +		composite->fixed_ops = fixed_ops;
>> +		clk_composite_ops->recalc_rate = clk_composite_recalc_rate_fixed;
>> +	}
>> +
>>  	init.ops = clk_composite_ops;
>>  	composite->hw.init = &init;
>>  
>> @@ -193,9 +217,34 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
>>  	if (composite->gate_hw)
>>  		composite->gate_hw->clk = clk;
>>  
>> +	if (composite->fixed_hw)
>> +		composite->fixed_hw->clk = clk;
>> +
>>  	return clk;
>>  
>>  err:
>>  	kfree(composite);
>>  	return clk;
>>  }
>> +
>> +struct clk *clk_register_composite(struct device *dev, const char *name,
>> +			const char **parent_names, int num_parents,
>> +			struct clk_hw *mux_hw, const struct clk_ops *mux_ops,
>> +			struct clk_hw *div_hw, const struct clk_ops *div_ops,
>> +			struct clk_hw *gate_hw, const struct clk_ops *gate_ops,
>> +			unsigned long flags)
>> +{
>> +	return _clk_register_composite(dev, name, parent_names, num_parents,
>> +				       mux_hw, mux_ops, div_hw, div_ops,
>> +				       gate_hw, gate_ops, NULL, NULL, flags);
>> +}
>> +
>> +struct clk *clk_register_gatable_osc(struct device *dev, const char *name,
>> +			struct clk_hw *fixed_hw, const struct clk_ops *fixed_ops,
>> +			struct clk_hw *gate_hw, const struct clk_ops *gate_ops,
>> +			unsigned long flags)
>> +{
>> +	return _clk_register_composite(dev, name, NULL, 0, NULL, NULL,
>> +				       NULL, NULL, gate_hw, gate_ops,
>> +				       fixed_hw, fixed_ops, flags);
>> +}
> 
> I think you should remove all this chunk, and made your change in
> clk_register_composite() instead of using _clk_register_composite(). I don't
> think it is a good thing that each kind of composition was declared here.
> 
> The way you composed your clock should be done by each driver.
> 
> I understand that you didn't want to break the existing driver, but in this case
> it is better to do the modification in the driver to follow the new API.
> The change is pretty trivial and can be automated by using spatch aka semantic patch
> aka coccinelle. But as this API is very new you could also do it by hand, however I
> didn't find any occurrences of this function in the branches clk-next or clk-for-3.10.
> I only found one occurrence in  the  in patch "clk: tegra30: Convert clk out to
> composite clk" from Prashant Gaikwad. Unfortunately I didn't find a tree with this
> commit applied.

Originally I thought of doing it that way, but I couldn't find any
actual usage of clk-composite, so I played it safe and avoided breaking
the API. If there are no patches queued for 3.10 using it and Prashant
agrees, I will rename _clk_register_composite -> clk_register_composite
and drop the wrappers.

@Prashant: are there any patches queued for 3.10 using this API?

[PATCH 0/3] fixed-rate and fixed-factor clocks in composite clock

[Mike Turquette]

The series to add a gateable fixed-rate clock to the composite clock[1]
highlights some weaknesses in the composite clock's design.  This series
addresses those concerns (and hopefully supercedes that series) by allowing
more flexibility in the clock ops that can be registered for a composite clock,
namely by making .round_rate and .set_rate optional.

The last patch in this series is a respin of Emilio's changes to the
sunxi clock driver which makes use of the changes to the composite
clock.

Only compile tested, not run tested.

[1] http://article.gmane.org/gmane.linux.ports.arm.kernel/230004

Emilio L?pez (1):
  clk: sunxi: Unify oscillator clock

Mike Turquette (2):
  clk: composite: rename 'div' references to 'rate'
  clk: composite: allow fixed rates & fixed dividers

 drivers/clk/clk-composite.c   |   49 +++++++++++++++++++++++------------------
 drivers/clk/sunxi/clk-sunxi.c |   33 +++++++++++++++++++++------
 include/linux/clk-provider.h  |   14 ++++++------
 3 files changed, 60 insertions(+), 36 deletions(-)

-- 
1.7.10.4

[PATCH 1/3] clk: composite: rename 'div' references to 'rate'

[Mike Turquette]

Rename all div_hw and div_ops related variables and functions to use
rate_hw, rate_ops, etc.  This is to make the rate-change portion of the
composite clk implementation more generic.  A patch following this one
will allow for fixed-rate clocks to reuse this infrastructure.

Signed-off-by: Mike Turquette <mturquette@linaro.org>
Cc: Prashant Gaikwad <pgaikwad@nvidia.com>
Cc: Emilio L?pez <emilio@elopez.com.ar>
Cc: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/clk/clk-composite.c  |   40 ++++++++++++++++++++--------------------
 include/linux/clk-provider.h |   14 +++++++-------
 2 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
index 097dee4..6f4728c 100644
--- a/drivers/clk/clk-composite.c
+++ b/drivers/clk/clk-composite.c
@@ -47,36 +47,36 @@ static unsigned long clk_composite_recalc_rate(struct clk_hw *hw,
 					    unsigned long parent_rate)
 {
 	struct clk_composite *composite = to_clk_composite(hw);
-	const struct clk_ops *div_ops = composite->div_ops;
-	struct clk_hw *div_hw = composite->div_hw;
+	const struct clk_ops *rate_ops = composite->rate_ops;
+	struct clk_hw *rate_hw = composite->rate_hw;
 
-	div_hw->clk = hw->clk;
+	rate_hw->clk = hw->clk;
 
-	return div_ops->recalc_rate(div_hw, parent_rate);
+	return rate_ops->recalc_rate(rate_hw, parent_rate);
 }
 
 static long clk_composite_round_rate(struct clk_hw *hw, unsigned long rate,
 				  unsigned long *prate)
 {
 	struct clk_composite *composite = to_clk_composite(hw);
-	const struct clk_ops *div_ops = composite->div_ops;
-	struct clk_hw *div_hw = composite->div_hw;
+	const struct clk_ops *rate_ops = composite->rate_ops;
+	struct clk_hw *rate_hw = composite->rate_hw;
 
-	div_hw->clk = hw->clk;
+	rate_hw->clk = hw->clk;
 
-	return div_ops->round_rate(div_hw, rate, prate);
+	return rate_ops->round_rate(rate_hw, rate, prate);
 }
 
 static int clk_composite_set_rate(struct clk_hw *hw, unsigned long rate,
 			       unsigned long parent_rate)
 {
 	struct clk_composite *composite = to_clk_composite(hw);
-	const struct clk_ops *div_ops = composite->div_ops;
-	struct clk_hw *div_hw = composite->div_hw;
+	const struct clk_ops *rate_ops = composite->rate_ops;
+	struct clk_hw *rate_hw = composite->rate_hw;
 
-	div_hw->clk = hw->clk;
+	rate_hw->clk = hw->clk;
 
-	return div_ops->set_rate(div_hw, rate, parent_rate);
+	return rate_ops->set_rate(rate_hw, rate, parent_rate);
 }
 
 static int clk_composite_is_enabled(struct clk_hw *hw)
@@ -115,7 +115,7 @@ static void clk_composite_disable(struct clk_hw *hw)
 struct clk *clk_register_composite(struct device *dev, const char *name,
 			const char **parent_names, int num_parents,
 			struct clk_hw *mux_hw, const struct clk_ops *mux_ops,
-			struct clk_hw *div_hw, const struct clk_ops *div_ops,
+			struct clk_hw *rate_hw, const struct clk_ops *rate_ops,
 			struct clk_hw *gate_hw, const struct clk_ops *gate_ops,
 			unsigned long flags)
 {
@@ -149,15 +149,15 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
 		clk_composite_ops->set_parent = clk_composite_set_parent;
 	}
 
-	if (div_hw && div_ops) {
-		if (!div_ops->recalc_rate || !div_ops->round_rate ||
-		    !div_ops->set_rate) {
+	if (rate_hw && rate_ops) {
+		if (!rate_ops->recalc_rate || !rate_ops->round_rate ||
+		    !rate_ops->set_rate) {
 			clk = ERR_PTR(-EINVAL);
 			goto err;
 		}
 
-		composite->div_hw = div_hw;
-		composite->div_ops = div_ops;
+		composite->rate_hw = rate_hw;
+		composite->rate_ops = rate_ops;
 		clk_composite_ops->recalc_rate = clk_composite_recalc_rate;
 		clk_composite_ops->round_rate = clk_composite_round_rate;
 		clk_composite_ops->set_rate = clk_composite_set_rate;
@@ -187,8 +187,8 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
 	if (composite->mux_hw)
 		composite->mux_hw->clk = clk;
 
-	if (composite->div_hw)
-		composite->div_hw->clk = clk;
+	if (composite->rate_hw)
+		composite->rate_hw->clk = clk;
 
 	if (composite->gate_hw)
 		composite->gate_hw->clk = clk;
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 9fdfae7..f5ba8c5 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -352,11 +352,11 @@ struct clk *clk_register_fixed_factor(struct device *dev, const char *name,
  * struct clk_composite - aggregate clock of mux, divider and gate clocks
  *
  * @hw:		handle between common and hardware-specific interfaces
- * @mux_hw:	handle between composite and hardware-specifix mux clock
- * @div_hw:	handle between composite and hardware-specifix divider clock
- * @gate_hw:	handle between composite and hardware-specifix gate clock
+ * @mux_hw:	handle between composite and hardware-specific mux clock
+ * @rate_hw:	handle between composite and hardware-specific rateider clock
+ * @gate_hw:	handle between composite and hardware-specific gate clock
  * @mux_ops:	clock ops for mux
- * @div_ops:	clock ops for divider
+ * @rate_ops:	clock ops for rateider
  * @gate_ops:	clock ops for gate
  */
 struct clk_composite {
@@ -364,18 +364,18 @@ struct clk_composite {
 	struct clk_ops	ops;
 
 	struct clk_hw	*mux_hw;
-	struct clk_hw	*div_hw;
+	struct clk_hw	*rate_hw;
 	struct clk_hw	*gate_hw;
 
 	const struct clk_ops	*mux_ops;
-	const struct clk_ops	*div_ops;
+	const struct clk_ops	*rate_ops;
 	const struct clk_ops	*gate_ops;
 };
 
 struct clk *clk_register_composite(struct device *dev, const char *name,
 		const char **parent_names, int num_parents,
 		struct clk_hw *mux_hw, const struct clk_ops *mux_ops,
-		struct clk_hw *div_hw, const struct clk_ops *div_ops,
+		struct clk_hw *rate_hw, const struct clk_ops *rate_ops,
 		struct clk_hw *gate_hw, const struct clk_ops *gate_ops,
 		unsigned long flags);
 
-- 
1.7.10.4

[PATCH 2/3] clk: composite: allow fixed rates & fixed dividers

[Mike Turquette]

The composite clock assumes that any clock implementing the .recalc_rate
callback will also implement .round_rate and .set_rate.  This is not
always true; the basic fixed-rate clock will only implement .recalc_rate
and a fixed-divider clock may choose to implement .recalc_rate and
.round_rate but not .set_rate.

Fix this by conditionally registering .round_rate and .set_rate
callbacks based on the rate_ops passed in to clk_composite_register.

Signed-off-by: Mike Turquette <mturquette@linaro.org>
Cc: Prashant Gaikwad <pgaikwad@nvidia.com>
Cc: Emilio L?pez <emilio@elopez.com.ar>
Cc: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/clk/clk-composite.c |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
index 6f4728c..2779fb3 100644
--- a/drivers/clk/clk-composite.c
+++ b/drivers/clk/clk-composite.c
@@ -150,17 +150,22 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
 	}
 
 	if (rate_hw && rate_ops) {
-		if (!rate_ops->recalc_rate || !rate_ops->round_rate ||
-		    !rate_ops->set_rate) {
+		if (!rate_ops->recalc_rate) {
 			clk = ERR_PTR(-EINVAL);
 			goto err;
 		}
 
+		/* .round_rate is a prerequisite for .set_rate */
+		if (rate_ops->round_rate) {
+			clk_composite_ops->round_rate = clk_composite_round_rate;
+			if (rate_ops->set_rate) {
+				clk_composite_ops->set_rate = clk_composite_set_rate;
+			}
+		}
+
 		composite->rate_hw = rate_hw;
 		composite->rate_ops = rate_ops;
 		clk_composite_ops->recalc_rate = clk_composite_recalc_rate;
-		clk_composite_ops->round_rate = clk_composite_round_rate;
-		clk_composite_ops->set_rate = clk_composite_set_rate;
 	}
 
 	if (gate_hw && gate_ops) {
-- 
1.7.10.4

[PATCH v2 3/3] clk: sunxi: Unify oscillator clock

[Mike Turquette]

From: Emilio L?pez <emilio@elopez.com.ar>

This commit uses the new fixed-rate support on the composite clock to
unify osc24M_fixed and osc24M clocks, so it matches the actual hardware.

Signed-off-by: Emilio L?pez <emilio@elopez.com.ar>
Signed-off-by: Mike Turquette <mturquette@linaro.org>
[mturquette at linaro.org: replace clk_register_gatable_osc with a call to
clk_register_composite]
---
Emilio,

I haven't been able to test these changes at run-time.  Can you see if
they are valid for solving your issue on sunxi?

 drivers/clk/sunxi/clk-sunxi.c |   33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
index 0bb0eb4..8492ad1 100644
--- a/drivers/clk/sunxi/clk-sunxi.c
+++ b/drivers/clk/sunxi/clk-sunxi.c
@@ -33,16 +33,36 @@ static DEFINE_SPINLOCK(clk_lock);
 static void __init sunxi_osc_clk_setup(struct device_node *node)
 {
 	struct clk *clk;
+	struct clk_fixed_rate *fixed;
+	struct clk_gate *gate;
 	const char *clk_name = node->name;
-	const char *parent;
-	void *reg;
+	u32 rate;
 
-	reg = of_iomap(node, 0);
+	/* allocate fixed-rate and gate clock structs */
+	fixed = kzalloc(sizeof(struct clk_fixed_rate), GFP_KERNEL);
+	if (!fixed)
+		return;
+	gate = kzalloc(sizeof(struct clk_gate), GFP_KERNEL);
+	if (!gate) {
+		kfree(fixed);
+		return;
+	}
 
-	parent = of_clk_get_parent_name(node, 0);
+	if (of_property_read_u32(node, "clock-frequency", &rate))
+		return;
+
+	/* set up gate and fixed rate properties */
+	gate->reg = of_iomap(node, 0);
+	gate->bit_idx = SUNXI_OSC24M_GATE;
+	gate->lock = &clk_lock;
+	fixed->fixed_rate = rate;
 
-	clk = clk_register_gate(NULL, clk_name, parent, 0, reg,
-				SUNXI_OSC24M_GATE, 0, &clk_lock);
+	clk = clk_register_composite(NULL, clk_name,
+			NULL, 0,
+			NULL, NULL,
+			&fixed->hw, &clk_fixed_rate_ops,
+			&gate->hw, &clk_gate_ops,
+			CLK_IS_ROOT);
 
 	if (clk) {
 		of_clk_add_provider(node, of_clk_src_simple_get, clk);
@@ -380,7 +400,6 @@ static void __init sunxi_gates_clk_setup(struct device_node *node,
 
 /* Matches for of_clk_init */
 static const __initconst struct of_device_id clk_match[] = {
-	{.compatible = "fixed-clock", .data = of_fixed_clk_setup,},
 	{.compatible = "allwinner,sun4i-osc-clk", .data = sunxi_osc_clk_setup,},
 	{}
 };
-- 
1.7.10.4

[PATCH v2 3/3] clk: sunxi: Unify oscillator clock

[Emilio López]

Hi Mike,

El 10/04/13 19:02, Mike Turquette escribi?:
> From: Emilio L?pez <emilio@elopez.com.ar>
> 
> This commit uses the new fixed-rate support on the composite clock to
> unify osc24M_fixed and osc24M clocks, so it matches the actual hardware.
> 
> Signed-off-by: Emilio L?pez <emilio@elopez.com.ar>
> Signed-off-by: Mike Turquette <mturquette@linaro.org>
> [mturquette at linaro.org: replace clk_register_gatable_osc with a call to
> clk_register_composite]
> ---
> Emilio,
> 
> I haven't been able to test these changes at run-time.  Can you see if
> they are valid for solving your issue on sunxi?

I tested the three patches you sent + 3/3 from the original series which
is the DT part.

$ git log --oneline -n 4
b63f51e ARM: sunxi: unify osc24M_fixed and osc24M
7f86473 clk: sunxi: Unify oscillator clock
78c8aaf clk: composite: allow fixed rates & fixed dividers
2d52a44 clk: composite: rename 'div' references to 'rate'

It seems to be working fine as far as I can tell, so

Tested-by: Emilio L?pez <emilio@elopez.com.ar>

Thanks,

Emilio

[PATCH 1/3] clk: composite: rename 'div' references to 'rate'

[Prashant Gaikwad]

On Thursday 11 April 2013 03:32 AM, Mike Turquette wrote:
> Rename all div_hw and div_ops related variables and functions to use
> rate_hw, rate_ops, etc.  This is to make the rate-change portion of the
> composite clk implementation more generic.  A patch following this one
> will allow for fixed-rate clocks to reuse this infrastructure.
>
> Signed-off-by: Mike Turquette <mturquette@linaro.org>
> Cc: Prashant Gaikwad <pgaikwad@nvidia.com>
> Cc: Emilio L?pez <emilio@elopez.com.ar>
> Cc: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>   drivers/clk/clk-composite.c  |   40 ++++++++++++++++++++--------------------
>   include/linux/clk-provider.h |   14 +++++++-------
>   2 files changed, 27 insertions(+), 27 deletions(-)
>
<snip>

>   
>   	if (composite->gate_hw)
>   		composite->gate_hw->clk = clk;
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 9fdfae7..f5ba8c5 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -352,11 +352,11 @@ struct clk *clk_register_fixed_factor(struct device *dev, const char *name,
>    * struct clk_composite - aggregate clock of mux, divider and gate clocks
>    *
>    * @hw:		handle between common and hardware-specific interfaces
> - * @mux_hw:	handle between composite and hardware-specifix mux clock
> - * @div_hw:	handle between composite and hardware-specifix divider clock
> - * @gate_hw:	handle between composite and hardware-specifix gate clock
> + * @mux_hw:	handle between composite and hardware-specific mux clock
> + * @rate_hw:	handle between composite and hardware-specific rateider clock

Small nitpick s/rateider/rate

Reviewed-by: Prashant Gaikwad <pgaikwad@nvidia.com>

> + * @gate_hw:	handle between composite and hardware-specific gate clock
>    * @mux_ops:	clock ops for mux
> - * @div_ops:	clock ops for divider
> + * @rate_ops:	clock ops for rateider
>    * @gate_ops:	clock ops for gate
>    */
>   struct clk_composite {
> @@ -364,18 +364,18 @@ struct clk_composite {
>   	struct clk_ops	ops;
>   
>   	struct clk_hw	*mux_hw;
> -	struct clk_hw	*div_hw;
> +	struct clk_hw	*rate_hw;
>   	struct clk_hw	*gate_hw;
>   
>   	const struct clk_ops	*mux_ops;
> -	const struct clk_ops	*div_ops;
> +	const struct clk_ops	*rate_ops;
>   	const struct clk_ops	*gate_ops;
>   };
>   
>   struct clk *clk_register_composite(struct device *dev, const char *name,
>   		const char **parent_names, int num_parents,
>   		struct clk_hw *mux_hw, const struct clk_ops *mux_ops,
> -		struct clk_hw *div_hw, const struct clk_ops *div_ops,
> +		struct clk_hw *rate_hw, const struct clk_ops *rate_ops,
>   		struct clk_hw *gate_hw, const struct clk_ops *gate_ops,
>   		unsigned long flags);
>   

[PATCH 2/3] clk: composite: allow fixed rates & fixed dividers

[Prashant Gaikwad]

On Thursday 11 April 2013 03:32 AM, Mike Turquette wrote:
> The composite clock assumes that any clock implementing the .recalc_rate
> callback will also implement .round_rate and .set_rate.  This is not
> always true; the basic fixed-rate clock will only implement .recalc_rate
> and a fixed-divider clock may choose to implement .recalc_rate and
> .round_rate but not .set_rate.
>
> Fix this by conditionally registering .round_rate and .set_rate
> callbacks based on the rate_ops passed in to clk_composite_register.
>
> Signed-off-by: Mike Turquette <mturquette@linaro.org>
> Cc: Prashant Gaikwad <pgaikwad@nvidia.com>
> Cc: Emilio L?pez <emilio@elopez.com.ar>
> Cc: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>   drivers/clk/clk-composite.c |   13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
> index 6f4728c..2779fb3 100644
> --- a/drivers/clk/clk-composite.c
> +++ b/drivers/clk/clk-composite.c
> @@ -150,17 +150,22 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
>   	}
>   
>   	if (rate_hw && rate_ops) {
> -		if (!rate_ops->recalc_rate || !rate_ops->round_rate ||
> -		    !rate_ops->set_rate) {
> +		if (!rate_ops->recalc_rate) {
>   			clk = ERR_PTR(-EINVAL);
>   			goto err;
>   		}
>   
> +		/* .round_rate is a prerequisite for .set_rate */
> +		if (rate_ops->round_rate) {
> +			clk_composite_ops->round_rate = clk_composite_round_rate;
> +			if (rate_ops->set_rate) {
> +				clk_composite_ops->set_rate = clk_composite_set_rate;
> +			}
> +		}
> +

What if rate_ops has only recalc_rate and set_rate? clk_composite_ops 
will have only recalc_rate and it will pass the sanity check in clk_init 
and caller of clk_register_composite will expect set_rate to work.
We should check here if rate_ops has set_rate but not round_rate then 
return error.

>   		composite->rate_hw = rate_hw;
>   		composite->rate_ops = rate_ops;
>   		clk_composite_ops->recalc_rate = clk_composite_recalc_rate;
> -		clk_composite_ops->round_rate = clk_composite_round_rate;
> -		clk_composite_ops->set_rate = clk_composite_set_rate;
>   	}
>   
>   	if (gate_hw && gate_ops) {

[PATCH 2/3] clk: composite: allow fixed rates & fixed dividers

[Mike Turquette]

Quoting Prashant Gaikwad (2013-04-11 04:47:50)
> On Thursday 11 April 2013 03:32 AM, Mike Turquette wrote:
> > The composite clock assumes that any clock implementing the .recalc_rate
> > callback will also implement .round_rate and .set_rate.  This is not
> > always true; the basic fixed-rate clock will only implement .recalc_rate
> > and a fixed-divider clock may choose to implement .recalc_rate and
> > .round_rate but not .set_rate.
> >
> > Fix this by conditionally registering .round_rate and .set_rate
> > callbacks based on the rate_ops passed in to clk_composite_register.
> >
> > Signed-off-by: Mike Turquette <mturquette@linaro.org>
> > Cc: Prashant Gaikwad <pgaikwad@nvidia.com>
> > Cc: Emilio L?pez <emilio@elopez.com.ar>
> > Cc: Gregory CLEMENT <gregory.clement@free-electrons.com>
> > ---
> >   drivers/clk/clk-composite.c |   13 +++++++++----
> >   1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
> > index 6f4728c..2779fb3 100644
> > --- a/drivers/clk/clk-composite.c
> > +++ b/drivers/clk/clk-composite.c
> > @@ -150,17 +150,22 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
> >       }
> >   
> >       if (rate_hw && rate_ops) {
> > -             if (!rate_ops->recalc_rate || !rate_ops->round_rate ||
> > -                 !rate_ops->set_rate) {
> > +             if (!rate_ops->recalc_rate) {
> >                       clk = ERR_PTR(-EINVAL);
> >                       goto err;
> >               }
> >   
> > +             /* .round_rate is a prerequisite for .set_rate */
> > +             if (rate_ops->round_rate) {
> > +                     clk_composite_ops->round_rate = clk_composite_round_rate;
> > +                     if (rate_ops->set_rate) {
> > +                             clk_composite_ops->set_rate = clk_composite_set_rate;
> > +                     }
> > +             }
> > +
> 
> What if rate_ops has only recalc_rate and set_rate? clk_composite_ops 
> will have only recalc_rate and it will pass the sanity check in clk_init 
> and caller of clk_register_composite will expect set_rate to work.
> We should check here if rate_ops has set_rate but not round_rate then 
> return error.
> 

I think I will instead put a WARN here instead of completely bailing out
of the clock registration.

Thanks,
Mike

> >               composite->rate_hw = rate_hw;
> >               composite->rate_ops = rate_ops;
> >               clk_composite_ops->recalc_rate = clk_composite_recalc_rate;
> > -             clk_composite_ops->round_rate = clk_composite_round_rate;
> > -             clk_composite_ops->set_rate = clk_composite_set_rate;
> >       }
> >   
> >       if (gate_hw && gate_ops) {

[PATCH 1/3] clk: composite: rename 'div' references to 'rate'

[Mike Turquette]

Quoting Prashant Gaikwad (2013-04-11 04:40:55)
> On Thursday 11 April 2013 03:32 AM, Mike Turquette wrote:
> > Rename all div_hw and div_ops related variables and functions to use
> > rate_hw, rate_ops, etc.  This is to make the rate-change portion of the
> > composite clk implementation more generic.  A patch following this one
> > will allow for fixed-rate clocks to reuse this infrastructure.
> >
> > Signed-off-by: Mike Turquette <mturquette@linaro.org>
> > Cc: Prashant Gaikwad <pgaikwad@nvidia.com>
> > Cc: Emilio L?pez <emilio@elopez.com.ar>
> > Cc: Gregory CLEMENT <gregory.clement@free-electrons.com>
> > ---
> >   drivers/clk/clk-composite.c  |   40 ++++++++++++++++++++--------------------
> >   include/linux/clk-provider.h |   14 +++++++-------
> >   2 files changed, 27 insertions(+), 27 deletions(-)
> >
> <snip>
> 
> >   
> >       if (composite->gate_hw)
> >               composite->gate_hw->clk = clk;
> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > index 9fdfae7..f5ba8c5 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -352,11 +352,11 @@ struct clk *clk_register_fixed_factor(struct device *dev, const char *name,
> >    * struct clk_composite - aggregate clock of mux, divider and gate clocks
> >    *
> >    * @hw:             handle between common and hardware-specific interfaces
> > - * @mux_hw:  handle between composite and hardware-specifix mux clock
> > - * @div_hw:  handle between composite and hardware-specifix divider clock
> > - * @gate_hw: handle between composite and hardware-specifix gate clock
> > + * @mux_hw:  handle between composite and hardware-specific mux clock
> > + * @rate_hw: handle between composite and hardware-specific rateider clock
> 
> Small nitpick s/rateider/rate
> 
> Reviewed-by: Prashant Gaikwad <pgaikwad@nvidia.com>
> 

Good catch!

Thanks,
Mike

> > + * @gate_hw: handle between composite and hardware-specific gate clock
> >    * @mux_ops:        clock ops for mux
> > - * @div_ops: clock ops for divider
> > + * @rate_ops:        clock ops for rateider
> >    * @gate_ops:       clock ops for gate
> >    */
> >   struct clk_composite {
> > @@ -364,18 +364,18 @@ struct clk_composite {
> >       struct clk_ops  ops;
> >   
> >       struct clk_hw   *mux_hw;
> > -     struct clk_hw   *div_hw;
> > +     struct clk_hw   *rate_hw;
> >       struct clk_hw   *gate_hw;
> >   
> >       const struct clk_ops    *mux_ops;
> > -     const struct clk_ops    *div_ops;
> > +     const struct clk_ops    *rate_ops;
> >       const struct clk_ops    *gate_ops;
> >   };
> >   
> >   struct clk *clk_register_composite(struct device *dev, const char *name,
> >               const char **parent_names, int num_parents,
> >               struct clk_hw *mux_hw, const struct clk_ops *mux_ops,
> > -             struct clk_hw *div_hw, const struct clk_ops *div_ops,
> > +             struct clk_hw *rate_hw, const struct clk_ops *rate_ops,
> >               struct clk_hw *gate_hw, const struct clk_ops *gate_ops,
> >               unsigned long flags);
> >

[PATCH v2 1/2] clk: composite: rename 'div' references to 'rate'

[Mike Turquette]

Rename all div_hw and div_ops related variables and functions to use
rate_hw, rate_ops, etc.  This is to make the rate-change portion of the
composite clk implementation more generic.  A patch following this one
will allow for fixed-rate clocks to reuse this infrastructure.

Signed-off-by: Mike Turquette <mturquette@linaro.org>
Reviewed-by: Prashant Gaikwad <pgaikwad@nvidia.com>
Tested-by: Emilio L?pez <emilio@elopez.com.ar>
Cc: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
Changes since v1:
* fixed regex typo in kerneldoc

 drivers/clk/clk-composite.c  |   40 ++++++++++++++++++++--------------------
 include/linux/clk-provider.h |   14 +++++++-------
 2 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
index 097dee4..6f4728c 100644
--- a/drivers/clk/clk-composite.c
+++ b/drivers/clk/clk-composite.c
@@ -47,36 +47,36 @@ static unsigned long clk_composite_recalc_rate(struct clk_hw *hw,
 					    unsigned long parent_rate)
 {
 	struct clk_composite *composite = to_clk_composite(hw);
-	const struct clk_ops *div_ops = composite->div_ops;
-	struct clk_hw *div_hw = composite->div_hw;
+	const struct clk_ops *rate_ops = composite->rate_ops;
+	struct clk_hw *rate_hw = composite->rate_hw;
 
-	div_hw->clk = hw->clk;
+	rate_hw->clk = hw->clk;
 
-	return div_ops->recalc_rate(div_hw, parent_rate);
+	return rate_ops->recalc_rate(rate_hw, parent_rate);
 }
 
 static long clk_composite_round_rate(struct clk_hw *hw, unsigned long rate,
 				  unsigned long *prate)
 {
 	struct clk_composite *composite = to_clk_composite(hw);
-	const struct clk_ops *div_ops = composite->div_ops;
-	struct clk_hw *div_hw = composite->div_hw;
+	const struct clk_ops *rate_ops = composite->rate_ops;
+	struct clk_hw *rate_hw = composite->rate_hw;
 
-	div_hw->clk = hw->clk;
+	rate_hw->clk = hw->clk;
 
-	return div_ops->round_rate(div_hw, rate, prate);
+	return rate_ops->round_rate(rate_hw, rate, prate);
 }
 
 static int clk_composite_set_rate(struct clk_hw *hw, unsigned long rate,
 			       unsigned long parent_rate)
 {
 	struct clk_composite *composite = to_clk_composite(hw);
-	const struct clk_ops *div_ops = composite->div_ops;
-	struct clk_hw *div_hw = composite->div_hw;
+	const struct clk_ops *rate_ops = composite->rate_ops;
+	struct clk_hw *rate_hw = composite->rate_hw;
 
-	div_hw->clk = hw->clk;
+	rate_hw->clk = hw->clk;
 
-	return div_ops->set_rate(div_hw, rate, parent_rate);
+	return rate_ops->set_rate(rate_hw, rate, parent_rate);
 }
 
 static int clk_composite_is_enabled(struct clk_hw *hw)
@@ -115,7 +115,7 @@ static void clk_composite_disable(struct clk_hw *hw)
 struct clk *clk_register_composite(struct device *dev, const char *name,
 			const char **parent_names, int num_parents,
 			struct clk_hw *mux_hw, const struct clk_ops *mux_ops,
-			struct clk_hw *div_hw, const struct clk_ops *div_ops,
+			struct clk_hw *rate_hw, const struct clk_ops *rate_ops,
 			struct clk_hw *gate_hw, const struct clk_ops *gate_ops,
 			unsigned long flags)
 {
@@ -149,15 +149,15 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
 		clk_composite_ops->set_parent = clk_composite_set_parent;
 	}
 
-	if (div_hw && div_ops) {
-		if (!div_ops->recalc_rate || !div_ops->round_rate ||
-		    !div_ops->set_rate) {
+	if (rate_hw && rate_ops) {
+		if (!rate_ops->recalc_rate || !rate_ops->round_rate ||
+		    !rate_ops->set_rate) {
 			clk = ERR_PTR(-EINVAL);
 			goto err;
 		}
 
-		composite->div_hw = div_hw;
-		composite->div_ops = div_ops;
+		composite->rate_hw = rate_hw;
+		composite->rate_ops = rate_ops;
 		clk_composite_ops->recalc_rate = clk_composite_recalc_rate;
 		clk_composite_ops->round_rate = clk_composite_round_rate;
 		clk_composite_ops->set_rate = clk_composite_set_rate;
@@ -187,8 +187,8 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
 	if (composite->mux_hw)
 		composite->mux_hw->clk = clk;
 
-	if (composite->div_hw)
-		composite->div_hw->clk = clk;
+	if (composite->rate_hw)
+		composite->rate_hw->clk = clk;
 
 	if (composite->gate_hw)
 		composite->gate_hw->clk = clk;
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 9fdfae7..766711a 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -352,11 +352,11 @@ struct clk *clk_register_fixed_factor(struct device *dev, const char *name,
  * struct clk_composite - aggregate clock of mux, divider and gate clocks
  *
  * @hw:		handle between common and hardware-specific interfaces
- * @mux_hw:	handle between composite and hardware-specifix mux clock
- * @div_hw:	handle between composite and hardware-specifix divider clock
- * @gate_hw:	handle between composite and hardware-specifix gate clock
+ * @mux_hw:	handle between composite and hardware-specific mux clock
+ * @rate_hw:	handle between composite and hardware-specific rate clock
+ * @gate_hw:	handle between composite and hardware-specific gate clock
  * @mux_ops:	clock ops for mux
- * @div_ops:	clock ops for divider
+ * @rate_ops:	clock ops for rate
  * @gate_ops:	clock ops for gate
  */
 struct clk_composite {
@@ -364,18 +364,18 @@ struct clk_composite {
 	struct clk_ops	ops;
 
 	struct clk_hw	*mux_hw;
-	struct clk_hw	*div_hw;
+	struct clk_hw	*rate_hw;
 	struct clk_hw	*gate_hw;
 
 	const struct clk_ops	*mux_ops;
-	const struct clk_ops	*div_ops;
+	const struct clk_ops	*rate_ops;
 	const struct clk_ops	*gate_ops;
 };
 
 struct clk *clk_register_composite(struct device *dev, const char *name,
 		const char **parent_names, int num_parents,
 		struct clk_hw *mux_hw, const struct clk_ops *mux_ops,
-		struct clk_hw *div_hw, const struct clk_ops *div_ops,
+		struct clk_hw *rate_hw, const struct clk_ops *rate_ops,
 		struct clk_hw *gate_hw, const struct clk_ops *gate_ops,
 		unsigned long flags);
 
-- 
1.7.10.4

[PATCH v2 2/2] clk: composite: allow fixed rates & fixed dividers

[Mike Turquette]

The composite clock assumes that any clock implementing the .recalc_rate
callback will also implement .round_rate and .set_rate.  This is not
always true; the basic fixed-rate clock will only implement .recalc_rate
and a fixed-divider clock may choose to implement .recalc_rate and
.round_rate but not .set_rate.

Fix this by conditionally registering .round_rate and .set_rate
callbacks based on the rate_ops passed in to clk_composite_register.

Signed-off-by: Mike Turquette <mturquette@linaro.org>
Cc: Prashant Gaikwad <pgaikwad@nvidia.com>
Tested-by: Emilio L?pez <emilio@elopez.com.ar>
Cc: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
Changes since v1:
* added WARN if set_rate is present without round_rate

 drivers/clk/clk-composite.c |   17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
index 6f4728c..a33f46f 100644
--- a/drivers/clk/clk-composite.c
+++ b/drivers/clk/clk-composite.c
@@ -150,17 +150,26 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
 	}
 
 	if (rate_hw && rate_ops) {
-		if (!rate_ops->recalc_rate || !rate_ops->round_rate ||
-		    !rate_ops->set_rate) {
+		if (!rate_ops->recalc_rate) {
 			clk = ERR_PTR(-EINVAL);
 			goto err;
 		}
 
+		/* .round_rate is a prerequisite for .set_rate */
+		if (rate_ops->round_rate) {
+			clk_composite_ops->round_rate = clk_composite_round_rate;
+			if (rate_ops->set_rate) {
+				clk_composite_ops->set_rate = clk_composite_set_rate;
+			}
+		} else {
+			WARN(rate_ops->set_rate,
+				"%s: missing round_rate op is required\n",
+				__func__);
+		}
+
 		composite->rate_hw = rate_hw;
 		composite->rate_ops = rate_ops;
 		clk_composite_ops->recalc_rate = clk_composite_recalc_rate;
-		clk_composite_ops->round_rate = clk_composite_round_rate;
-		clk_composite_ops->set_rate = clk_composite_set_rate;
 	}
 
 	if (gate_hw && gate_ops) {
-- 
1.7.10.4

[PATCH v2 3/3] clk: sunxi: Unify oscillator clock

[Mike Turquette]

Quoting Emilio L?pez (2013-04-10 17:07:21)
> Hi Mike,
> 
> El 10/04/13 19:02, Mike Turquette escribi?:
> > From: Emilio L?pez <emilio@elopez.com.ar>
> > 
> > This commit uses the new fixed-rate support on the composite clock to
> > unify osc24M_fixed and osc24M clocks, so it matches the actual hardware.
> > 
> > Signed-off-by: Emilio L?pez <emilio@elopez.com.ar>
> > Signed-off-by: Mike Turquette <mturquette@linaro.org>
> > [mturquette at linaro.org: replace clk_register_gatable_osc with a call to
> > clk_register_composite]
> > ---
> > Emilio,
> > 
> > I haven't been able to test these changes at run-time.  Can you see if
> > they are valid for solving your issue on sunxi?
> 
> I tested the three patches you sent + 3/3 from the original series which
> is the DT part.
> 
> $ git log --oneline -n 4
> b63f51e ARM: sunxi: unify osc24M_fixed and osc24M
> 7f86473 clk: sunxi: Unify oscillator clock
> 78c8aaf clk: composite: allow fixed rates & fixed dividers
> 2d52a44 clk: composite: rename 'div' references to 'rate'
> 
> It seems to be working fine as far as I can tell, so
> 
> Tested-by: Emilio L?pez <emilio@elopez.com.ar>
> 

I've applied all three patches to clk-next.  I assume the DT patch will
go through Maxime's tree.

Regards,
Mike

> Thanks,
> 
> Emilio

[PATCH 3/3] ARM: sunxi: unify osc24M_fixed and osc24M

[Maxime Ripard]

Hi,

Le 09/04/2013 15:48, Emilio L?pez a ?crit :
> Now that the clock driver supports the gatable oscillator as one single
> clock, drop osc24M_fixed and move the relevant properties to osc24M
> 
> Signed-off-by: Emilio L?pez <emilio@elopez.com.ar>

Rebased and applied in my dt-for-3.10 branch.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com