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

login
register
mail settings
Submitter Emilio López
Date April 9, 2013, 1:48 p.m.
Message ID <1365515284-32061-2-git-send-email-emilio@elopez.com.ar>
Download mbox | patch
Permalink /patch/235088/
State New
Headers show

Comments

Emilio López - April 9, 2013, 1:48 p.m.
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(-)
Gregory CLEMENT - April 9, 2013, 3:41 p.m.
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.
Emilio López - April 9, 2013, 4:04 p.m.
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?
Mike Turquette - April 10, 2013, 10:02 p.m.
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(-)

Patch

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