[PATCH V2] clk: Add composite clock type

[PATCH V2] clk: Add composite clock type

[Prashant Gaikwad]

Not all clocks are required to be decomposed into basic clock
types but at the same time want to use the functionality
provided by these basic clock types instead of duplicating.

For example, Tegra SoC has ~100 clocks which can be decomposed
into Mux -> Div -> Gate clock types making the clock count to
~300. Also, parent change operation can not be performed on gate
clock which forces to use mux clock in driver if want to change
the parent.

Instead aggregate the basic clock types functionality into one
clock and just use this clock for all operations. This clock
type re-uses the functionality of basic clock types and not
limited to basic clock types but any hardware-specific
implementation.

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

Changes from V1:
- 2nd patch dropped as the concept is acked by Mike.
- Fixed comments from Stephen.

---
 drivers/clk/Makefile         |    1 +
 drivers/clk/clk-composite.c  |  208 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |   30 ++++++
 3 files changed, 239 insertions(+), 0 deletions(-)
 create mode 100644 drivers/clk/clk-composite.c

diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index ce77077..2287848 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_COMMON_CLK)	+= clk-fixed-factor.o
 obj-$(CONFIG_COMMON_CLK)	+= clk-fixed-rate.o
 obj-$(CONFIG_COMMON_CLK)	+= clk-gate.o
 obj-$(CONFIG_COMMON_CLK)	+= clk-mux.o
+obj-$(CONFIG_COMMON_CLK)	+= clk-composite.o
 
 # SoCs specific
 obj-$(CONFIG_ARCH_BCM2835)	+= clk-bcm2835.o
diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
new file mode 100644
index 0000000..5a6587f
--- /dev/null
+++ b/drivers/clk/clk-composite.c
@@ -0,0 +1,208 @@
+/*
+ * Copyright (c) 2013 NVIDIA CORPORATION.  All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+
+#define to_clk_composite(_hw) container_of(_hw, struct clk_composite, hw)
+
+static u8 clk_composite_get_parent(struct clk_hw *hw)
+{
+	struct clk_composite *composite = to_clk_composite(hw);
+	const struct clk_ops *mux_ops = composite->mux_ops;
+	struct clk_hw *mux_hw = composite->mux_hw;
+
+	mux_hw->clk = hw->clk;
+
+	return mux_ops->get_parent(mux_hw);
+}
+
+static int clk_composite_set_parent(struct clk_hw *hw, u8 index)
+{
+	struct clk_composite *composite = to_clk_composite(hw);
+	const struct clk_ops *mux_ops = composite->mux_ops;
+	struct clk_hw *mux_hw = composite->mux_hw;
+
+	mux_hw->clk = hw->clk;
+
+	return mux_ops->set_parent(mux_hw, index);
+}
+
+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;
+
+	div_hw->clk = hw->clk;
+
+	return div_ops->recalc_rate(div_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;
+
+	div_hw->clk = hw->clk;
+
+	return div_ops->round_rate(div_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;
+
+	div_hw->clk = hw->clk;
+
+	return div_ops->set_rate(div_hw, rate, parent_rate);
+}
+
+static int clk_composite_is_enabled(struct clk_hw *hw)
+{
+	struct clk_composite *composite = to_clk_composite(hw);
+	const struct clk_ops *gate_ops = composite->gate_ops;
+	struct clk_hw *gate_hw = composite->gate_hw;
+
+	gate_hw->clk = hw->clk;
+
+	return gate_ops->is_enabled(gate_hw);
+}
+
+static int clk_composite_enable(struct clk_hw *hw)
+{
+	struct clk_composite *composite = to_clk_composite(hw);
+	const struct clk_ops *gate_ops = composite->gate_ops;
+	struct clk_hw *gate_hw = composite->gate_hw;
+
+	gate_hw->clk = hw->clk;
+
+	return gate_ops->enable(gate_hw);
+}
+
+static void clk_composite_disable(struct clk_hw *hw)
+{
+	struct clk_composite *composite = to_clk_composite(hw);
+	const struct clk_ops *gate_ops = composite->gate_ops;
+	struct clk_hw *gate_hw = composite->gate_hw;
+
+	gate_hw->clk = hw->clk;
+
+	gate_ops->disable(gate_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 *gate_hw, const struct clk_ops *gate_ops,
+			unsigned long flags)
+{
+	struct clk *clk;
+	struct clk_init_data init;
+	struct clk_composite *composite;
+	struct clk_ops *clk_composite_ops;
+
+	composite = kzalloc(sizeof(*composite), GFP_KERNEL);
+	if (!composite) {
+		pr_err("%s: could not allocate composite clk\n", __func__);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	init.name = name;
+	init.flags = flags | CLK_IS_BASIC;
+	init.parent_names = parent_names;
+	init.num_parents = num_parents;
+
+	/* allocate the clock ops */
+	clk_composite_ops = kzalloc(sizeof(*clk_composite_ops), GFP_KERNEL);
+	if (!clk_composite_ops) {
+		pr_err("%s: could not allocate clk ops\n", __func__);
+		kfree(composite);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	if (mux_hw && mux_ops) {
+		if (!mux_ops->get_parent || !mux_ops->set_parent) {
+			clk = ERR_PTR(-EINVAL);
+			goto err;
+		}
+
+		composite->mux_hw = mux_hw;
+		composite->mux_ops = mux_ops;
+		clk_composite_ops->get_parent = clk_composite_get_parent;
+		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) {
+			clk = ERR_PTR(-EINVAL);
+			goto err;
+		}
+
+		composite->div_hw = div_hw;
+		composite->div_ops = div_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) {
+		if (!gate_ops->is_enabled || !gate_ops->enable ||
+		    !gate_ops->disable) {
+			clk = ERR_PTR(-EINVAL);
+			goto err;
+		}
+
+		composite->gate_hw = gate_hw;
+		composite->gate_ops = gate_ops;
+		clk_composite_ops->is_enabled = clk_composite_is_enabled;
+		clk_composite_ops->enable = clk_composite_enable;
+		clk_composite_ops->disable = clk_composite_disable;
+	}
+
+	init.ops = clk_composite_ops;
+	composite->hw.init = &init;
+
+	clk = clk_register(dev, &composite->hw);
+	if (IS_ERR(clk))
+		goto err;
+
+	if (composite->mux_hw)
+		composite->mux_hw->clk = clk;
+
+	if (composite->div_hw)
+		composite->div_hw->clk = clk;
+
+	if (composite->gate_hw)
+		composite->gate_hw->clk = clk;
+
+	return clk;
+
+err:
+	kfree(clk_composite_ops);
+	kfree(composite);
+	return clk;
+}
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 7f197d7..f1a36aa 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -325,6 +325,36 @@ struct clk *clk_register_fixed_factor(struct device *dev, const char *name,
 		const char *parent_name, unsigned long flags,
 		unsigned int mult, unsigned int div);
 
+/***
+ * 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_ops:	clock ops for mux
+ * @div_ops:	clock ops for divider
+ * @gate_ops:	clock ops for gate
+ */
+struct clk_composite {
+	struct clk_hw	hw;
+
+	struct clk_hw	*mux_hw;
+	struct clk_hw	*div_hw;
+	struct clk_hw	*gate_hw;
+
+	const struct clk_ops	*mux_ops;
+	const struct clk_ops	*div_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 *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.7.4.1

Re: [PATCH V2] clk: Add composite clock type

[Hiroshi Doyu]

Hi Prashant,

Prashant Gaikwad <pgaikwad@nvidia.com> wrote @ Mon, 4 Feb 2013 09:11:22 +0100:

> +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)
> +{
> +       struct clk *clk;
> +       struct clk_init_data init;
> +       struct clk_composite *composite;
> +       struct clk_ops *clk_composite_ops;
> +
> +       composite = kzalloc(sizeof(*composite), GFP_KERNEL);
> +       if (!composite) {
> +               pr_err("%s: could not allocate composite clk\n", __func__);
> +               return ERR_PTR(-ENOMEM);
> +       }
> +
> +       init.name = name;
> +       init.flags = flags | CLK_IS_BASIC;
> +       init.parent_names = parent_names;
> +       init.num_parents = num_parents;
> +
> +       /* allocate the clock ops */
> +       clk_composite_ops = kzalloc(sizeof(*clk_composite_ops), GFP_KERNEL);

The members of "clk_composite_ops" seems to be always assigned
statically. Istead of dynamically allocating/assigning, can't we just
have "clk_composite_ops" statically as below?

static struct clk_ops clk_composite_ops = {
	.get_parent = clk_composite_get_parent;
	.set_parent = clk_composite_set_parent;
	.recalc_rate = clk_composite_recalc_rate;
	.round_rate = clk_composite_round_rate;
	.set_rate = clk_composite_set_rate;
	.is_enabled = clk_composite_is_enabled;
	.enable = clk_composite_enable;
	.disable = clk_composite_disable;
};

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)
{
	.....

	init.ops = &clk_composite_ops;

Re: [PATCH V2] clk: Add composite clock type

[Prashant Gaikwad]

On Monday 04 February 2013 03:07 PM, Hiroshi Doyu wrote:
> Hi Prashant,
>
> Prashant Gaikwad <pgaikwad@nvidia.com> wrote @ Mon, 4 Feb 2013 09:11:22 +0100:
>
>> +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)
>> +{
>> +       struct clk *clk;
>> +       struct clk_init_data init;
>> +       struct clk_composite *composite;
>> +       struct clk_ops *clk_composite_ops;
>> +
>> +       composite = kzalloc(sizeof(*composite), GFP_KERNEL);
>> +       if (!composite) {
>> +               pr_err("%s: could not allocate composite clk\n", __func__);
>> +               return ERR_PTR(-ENOMEM);
>> +       }
>> +
>> +       init.name = name;
>> +       init.flags = flags | CLK_IS_BASIC;
>> +       init.parent_names = parent_names;
>> +       init.num_parents = num_parents;
>> +
>> +       /* allocate the clock ops */
>> +       clk_composite_ops = kzalloc(sizeof(*clk_composite_ops), GFP_KERNEL);
> The members of "clk_composite_ops" seems to be always assigned
> statically. Istead of dynamically allocating/assigning, can't we just
> have "clk_composite_ops" statically as below?
>
> static struct clk_ops clk_composite_ops = {
> 	.get_parent = clk_composite_get_parent;
> 	.set_parent = clk_composite_set_parent;
> 	.recalc_rate = clk_composite_recalc_rate;
> 	.round_rate = clk_composite_round_rate;
> 	.set_rate = clk_composite_set_rate;
> 	.is_enabled = clk_composite_is_enabled;
> 	.enable = clk_composite_enable;
> 	.disable = clk_composite_disable;
> };
>
> 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)
> {
> 	.....
>
> 	init.ops = &clk_composite_ops;

No, clk_ops depends on the clocks you are using. There could be a clock 
with mux and gate while another one with mux and div.

Re: [PATCH V2] clk: Add composite clock type

[Tomasz Figa]

Hi Prashant,

Thank you for your patch. Please see some comments inline.

On Monday 04 of February 2013 13:41:22 Prashant Gaikwad wrote:
> Not all clocks are required to be decomposed into basic clock
> types but at the same time want to use the functionality
> provided by these basic clock types instead of duplicating.
> 
> For example, Tegra SoC has ~100 clocks which can be decomposed
> into Mux -> Div -> Gate clock types making the clock count to
> ~300. Also, parent change operation can not be performed on gate
> clock which forces to use mux clock in driver if want to change
> the parent.
> 
> Instead aggregate the basic clock types functionality into one
> clock and just use this clock for all operations. This clock
> type re-uses the functionality of basic clock types and not
> limited to basic clock types but any hardware-specific
> implementation.
> 
> Signed-off-by: Prashant Gaikwad <pgaikwad@nvidia.com>
> ---
> 
> Changes from V1:
> - 2nd patch dropped as the concept is acked by Mike.
> - Fixed comments from Stephen.
> 
> ---
>  drivers/clk/Makefile         |    1 +
>  drivers/clk/clk-composite.c  |  208
> ++++++++++++++++++++++++++++++++++++++++++ include/linux/clk-provider.h
> |   30 ++++++
>  3 files changed, 239 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/clk/clk-composite.c
> 
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index ce77077..2287848 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_COMMON_CLK)	+= clk-fixed-factor.o
>  obj-$(CONFIG_COMMON_CLK)	+= clk-fixed-rate.o
>  obj-$(CONFIG_COMMON_CLK)	+= clk-gate.o
>  obj-$(CONFIG_COMMON_CLK)	+= clk-mux.o
> +obj-$(CONFIG_COMMON_CLK)	+= clk-composite.o
> 
>  # SoCs specific
>  obj-$(CONFIG_ARCH_BCM2835)	+= clk-bcm2835.o
> diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
> new file mode 100644
> index 0000000..5a6587f
> --- /dev/null
> +++ b/drivers/clk/clk-composite.c
> @@ -0,0 +1,208 @@
> +/*
> + * Copyright (c) 2013 NVIDIA CORPORATION.  All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> it + * under the terms and conditions of the GNU General Public
> License, + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but
> WITHOUT + * ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> General Public License for + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see
> <http://www.gnu.org/licenses/>. + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +
> +#define to_clk_composite(_hw) container_of(_hw, struct clk_composite,
> hw) +
> +static u8 clk_composite_get_parent(struct clk_hw *hw)
> +{
> +	struct clk_composite *composite = to_clk_composite(hw);
> +	const struct clk_ops *mux_ops = composite->mux_ops;
> +	struct clk_hw *mux_hw = composite->mux_hw;
> +
> +	mux_hw->clk = hw->clk;

Why is this needed? Looks like this filed is already being initialized in 
clk_register_composite.

> +
> +	return mux_ops->get_parent(mux_hw);
> +}
> +
> +static int clk_composite_set_parent(struct clk_hw *hw, u8 index)
> +{
> +	struct clk_composite *composite = to_clk_composite(hw);
> +	const struct clk_ops *mux_ops = composite->mux_ops;
> +	struct clk_hw *mux_hw = composite->mux_hw;
> +
> +	mux_hw->clk = hw->clk;

Ditto.

> +
> +	return mux_ops->set_parent(mux_hw, index);
> +}
> +
> +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;
> +
> +	div_hw->clk = hw->clk;

Ditto.

> +
> +	return div_ops->recalc_rate(div_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;
> +
> +	div_hw->clk = hw->clk;

Ditto.

> +
> +	return div_ops->round_rate(div_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;
> +
> +	div_hw->clk = hw->clk;

Ditto.

> +
> +	return div_ops->set_rate(div_hw, rate, parent_rate);
> +}
> +
> +static int clk_composite_is_enabled(struct clk_hw *hw)
> +{
> +	struct clk_composite *composite = to_clk_composite(hw);
> +	const struct clk_ops *gate_ops = composite->gate_ops;
> +	struct clk_hw *gate_hw = composite->gate_hw;
> +
> +	gate_hw->clk = hw->clk;

Ditto.

> +
> +	return gate_ops->is_enabled(gate_hw);
> +}
> +
> +static int clk_composite_enable(struct clk_hw *hw)
> +{
> +	struct clk_composite *composite = to_clk_composite(hw);
> +	const struct clk_ops *gate_ops = composite->gate_ops;
> +	struct clk_hw *gate_hw = composite->gate_hw;
> +
> +	gate_hw->clk = hw->clk;

Ditto.

> +
> +	return gate_ops->enable(gate_hw);
> +}
> +
> +static void clk_composite_disable(struct clk_hw *hw)
> +{
> +	struct clk_composite *composite = to_clk_composite(hw);
> +	const struct clk_ops *gate_ops = composite->gate_ops;
> +	struct clk_hw *gate_hw = composite->gate_hw;
> +
> +	gate_hw->clk = hw->clk;

Ditto.

> +
> +	gate_ops->disable(gate_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 *gate_hw, const struct clk_ops *gate_ops,
> +			unsigned long flags)
> +{
> +	struct clk *clk;
> +	struct clk_init_data init;
> +	struct clk_composite *composite;
> +	struct clk_ops *clk_composite_ops;
> +
> +	composite = kzalloc(sizeof(*composite), GFP_KERNEL);
> +	if (!composite) {
> +		pr_err("%s: could not allocate composite clk\n", __func__);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	init.name = name;
> +	init.flags = flags | CLK_IS_BASIC;
> +	init.parent_names = parent_names;
> +	init.num_parents = num_parents;
> +
> +	/* allocate the clock ops */
> +	clk_composite_ops = kzalloc(sizeof(*clk_composite_ops), GFP_KERNEL);
> +	if (!clk_composite_ops) {
> +		pr_err("%s: could not allocate clk ops\n", __func__);
> +		kfree(composite);
> +		return ERR_PTR(-ENOMEM);
> +	}

Maybe it would be better to embed this struct clk_ops inside struct 
clk_composite? This would allow to get rid of this allocation.

> +
> +	if (mux_hw && mux_ops) {
> +		if (!mux_ops->get_parent || !mux_ops->set_parent) {
> +			clk = ERR_PTR(-EINVAL);
> +			goto err;
> +		}
> +
> +		composite->mux_hw = mux_hw;
> +		composite->mux_ops = mux_ops;
> +		clk_composite_ops->get_parent = clk_composite_get_parent;
> +		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) {
> +			clk = ERR_PTR(-EINVAL);
> +			goto err;
> +		}
> +
> +		composite->div_hw = div_hw;
> +		composite->div_ops = div_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) {
> +		if (!gate_ops->is_enabled || !gate_ops->enable ||
> +		    !gate_ops->disable) {
> +			clk = ERR_PTR(-EINVAL);
> +			goto err;
> +		}
> +
> +		composite->gate_hw = gate_hw;
> +		composite->gate_ops = gate_ops;
> +		clk_composite_ops->is_enabled = clk_composite_is_enabled;
> +		clk_composite_ops->enable = clk_composite_enable;
> +		clk_composite_ops->disable = clk_composite_disable;
> +	}
> +
> +	init.ops = clk_composite_ops;
> +	composite->hw.init = &init;
> +
> +	clk = clk_register(dev, &composite->hw);
> +	if (IS_ERR(clk))
> +		goto err;
> +
> +	if (composite->mux_hw)
> +		composite->mux_hw->clk = clk;
> +
> +	if (composite->div_hw)
> +		composite->div_hw->clk = clk;
> +
> +	if (composite->gate_hw)
> +		composite->gate_hw->clk = clk;
> +
> +	return clk;
> +
> +err:
> +	kfree(clk_composite_ops);
> +	kfree(composite);
> +	return clk;
> +}
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 7f197d7..f1a36aa 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -325,6 +325,36 @@ struct clk *clk_register_fixed_factor(struct device
> *dev, const char *name, const char *parent_name, unsigned long flags,
>  		unsigned int mult, unsigned int div);
> 
> +/***
> + * 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_ops:	clock ops for mux
> + * @div_ops:	clock ops for divider
> + * @gate_ops:	clock ops for gate
> + */
> +struct clk_composite {
> +	struct clk_hw	hw;

As I suggested above:

	struct clk_ops	ops;

> +
> +	struct clk_hw	*mux_hw;
> +	struct clk_hw	*div_hw;
> +	struct clk_hw	*gate_hw;
> +
> +	const struct clk_ops	*mux_ops;
> +	const struct clk_ops	*div_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 *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

Best regards,
-- 
Tomasz Figa
Samsung Poland R&D Center
SW Solution Development, Linux Platform

Re: [PATCH V2] clk: Add composite clock type

[Hiroshi Doyu]

Prashant Gaikwad <pgaikwad@nvidia.com> wrote @ Tue, 5 Feb 2013 09:33:41 +0100:

> > The members of "clk_composite_ops" seems to be always assigned
> > statically. Istead of dynamically allocating/assigning, can't we just
> > have "clk_composite_ops" statically as below?
> >
> > static struct clk_ops clk_composite_ops = {
> > 	.get_parent = clk_composite_get_parent;
> > 	.set_parent = clk_composite_set_parent;
> > 	.recalc_rate = clk_composite_recalc_rate;
> > 	.round_rate = clk_composite_round_rate;
> > 	.set_rate = clk_composite_set_rate;
> > 	.is_enabled = clk_composite_is_enabled;
> > 	.enable = clk_composite_enable;
> > 	.disable = clk_composite_disable;
> > };
> >
> > 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)
> > {
> > 	.....
> >
> > 	init.ops = &clk_composite_ops;
> 
> No, clk_ops depends on the clocks you are using. There could be a clock 
> with mux and gate while another one with mux and div.

You are right. What about the following? We don't have to have similar
copy of clk_composite_ops for each instances.

diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
index f30fb4b..8f88805 100644
--- a/drivers/clk/clk-composite.c
+++ b/drivers/clk/clk-composite.c
@@ -27,6 +27,9 @@ static u8 clk_composite_get_parent(struct clk_hw *hw)
        const struct clk_ops *mux_ops = composite->mux_ops;
        struct clk_hw *mux_hw = composite->mux_hw;
 
+       if (!mux_hw->clk)
+	       return -EINVAL;
+
        mux_hw->clk = hw->clk;
 
        return mux_ops->get_parent(mux_hw);
@@ -38,6 +41,9 @@ static int clk_composite_set_parent(struct clk_hw *hw, u8 index)
        const struct clk_ops *mux_ops = composite->mux_ops;
        struct clk_hw *mux_hw = composite->mux_hw;
 
+       if (!mux_hw->clk)
+	       return -EINVAL;
+
        mux_hw->clk = hw->clk;
 
        return mux_ops->set_parent(mux_hw, index);
@@ -50,6 +56,9 @@ static unsigned long clk_composite_recalc_rate(struct clk_hw *hw,
        const struct clk_ops *div_ops = composite->div_ops;
        struct clk_hw *div_hw = composite->div_hw;
 
+       if (!div_hw->clk)
+	       return -EINVAL;
+
        div_hw->clk = hw->clk;
 
        return div_ops->recalc_rate(div_hw, parent_rate);
@@ -62,6 +71,9 @@ static long clk_composite_round_rate(struct clk_hw *hw, unsigned long rate,
        const struct clk_ops *div_ops = composite->div_ops;
        struct clk_hw *div_hw = composite->div_hw;
 
+       if (!div_hw->clk)
+	       return -EINVAL;
+
        div_hw->clk = hw->clk;
 
        return div_ops->round_rate(div_hw, rate, prate);
@@ -74,6 +86,9 @@ static int clk_composite_set_rate(struct clk_hw *hw, unsigned long rate,
        const struct clk_ops *div_ops = composite->div_ops;
        struct clk_hw *div_hw = composite->div_hw;
 
+       if (!div_hw->clk)
+	       return -EINVAL;
+
        div_hw->clk = hw->clk;
 
        return div_ops->set_rate(div_hw, rate, parent_rate);
@@ -85,6 +100,9 @@ static int clk_composite_is_enabled(struct clk_hw *hw)
        const struct clk_ops *gate_ops = composite->gate_ops;
        struct clk_hw *gate_hw = composite->gate_hw;
 
+       if (!gate_hw->clk)
+	       return -EINVAL;
+
        gate_hw->clk = hw->clk;
 
        return gate_ops->is_enabled(gate_hw);
@@ -96,6 +114,9 @@ static int clk_composite_enable(struct clk_hw *hw)
        const struct clk_ops *gate_ops = composite->gate_ops;
        struct clk_hw *gate_hw = composite->gate_hw;
 
+       if (!gate_hw->clk)
+	       return -EINVAL;
+
        gate_hw->clk = hw->clk;
 
        return gate_ops->enable(gate_hw);
@@ -107,11 +128,25 @@ static void clk_composite_disable(struct clk_hw *hw)
        const struct clk_ops *gate_ops = composite->gate_ops;
        struct clk_hw *gate_hw = composite->gate_hw;
 
+       if (!gate_hw->clk)
+	       return -EINVAL;
+
        gate_hw->clk = hw->clk;
 
        gate_ops->disable(gate_hw);
 }
 
+static struct clk_ops clk_composite_ops = {
+	.get_parent = clk_composite_get_parent,
+	.set_parent = clk_composite_set_parent,
+	.recalc_rate = clk_composite_recalc_rate,
+	.round_rate = clk_composite_round_rate,
+	.set_rate = clk_composite_set_rate,
+	.is_enabled = clk_composite_is_enabled,
+	.enable = clk_composite_enable,
+	.disable = clk_composite_disable,
+};
+
 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,
@@ -135,14 +170,6 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
        init.parent_names = parent_names;
        init.num_parents = num_parents;
 
-       /* allocate the clock ops */
-       clk_composite_ops = kzalloc(sizeof(*clk_composite_ops), GFP_KERNEL);
-       if (!clk_composite_ops) {
-               pr_err("%s: could not allocate clk ops\n", __func__);
-               kfree(composite);
-               return ERR_PTR(-ENOMEM);
-       }
-
        if (mux_hw && mux_ops) {
                if (!mux_ops->get_parent || !mux_ops->set_parent) {
                        clk = ERR_PTR(-EINVAL);
@@ -151,8 +178,6 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
 
                composite->mux_hw = mux_hw;
                composite->mux_ops = mux_ops;
-               clk_composite_ops->get_parent = clk_composite_get_parent;
-               clk_composite_ops->set_parent = clk_composite_set_parent;
        }
 
        if (div_hw && div_ops) {
@@ -164,9 +189,6 @@ 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->round_rate = clk_composite_round_rate;
-               clk_composite_ops->set_rate = clk_composite_set_rate;
        }
 
        if (gate_hw && gate_ops) {
@@ -178,12 +200,9 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
 
                composite->gate_hw = gate_hw;
                composite->gate_ops = gate_ops;
-               clk_composite_ops->is_enabled = clk_composite_is_enabled;
-               clk_composite_ops->enable = clk_composite_enable;
-               clk_composite_ops->disable = clk_composite_disable;
        }
 
-       init.ops = clk_composite_ops;
+       init.ops = &clk_composite_ops;
        composite->hw.init = &init;
 
        clk = clk_register(dev, &composite->hw);
@@ -202,7 +221,6 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
        return clk;
 
 err:
-       kfree(clk_composite_ops);
        kfree(composite);
        return clk;
 }

Re: [PATCH V2] clk: Add composite clock type

[Tomasz Figa]

On Tuesday 05 of February 2013 11:22:52 Hiroshi Doyu wrote:
> Prashant Gaikwad <pgaikwad@nvidia.com> wrote @ Tue, 5 Feb 2013 09:33:41 
+0100:
> > > The members of "clk_composite_ops" seems to be always assigned
> > > statically. Istead of dynamically allocating/assigning, can't we
> > > just
> > > have "clk_composite_ops" statically as below?
> > > 
> > > static struct clk_ops clk_composite_ops = {
> > > 
> > > 	.get_parent = clk_composite_get_parent;
> > > 	.set_parent = clk_composite_set_parent;
> > > 	.recalc_rate = clk_composite_recalc_rate;
> > > 	.round_rate = clk_composite_round_rate;
> > > 	.set_rate = clk_composite_set_rate;
> > > 	.is_enabled = clk_composite_is_enabled;
> > > 	.enable = clk_composite_enable;
> > > 	.disable = clk_composite_disable;
> > > 
> > > };
> > > 
> > > 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)
> > > 
> > > {
> > > 
> > > 	.....
> > > 	
> > > 	init.ops = &clk_composite_ops;
> > 
> > No, clk_ops depends on the clocks you are using. There could be a
> > clock
> > with mux and gate while another one with mux and div.
> 
> You are right. What about the following? We don't have to have similar
> copy of clk_composite_ops for each instances.
> 
> diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
> index f30fb4b..8f88805 100644
> --- a/drivers/clk/clk-composite.c
> +++ b/drivers/clk/clk-composite.c
> @@ -27,6 +27,9 @@ static u8 clk_composite_get_parent(struct clk_hw *hw)
>         const struct clk_ops *mux_ops = composite->mux_ops;
>         struct clk_hw *mux_hw = composite->mux_hw;
> 
> +       if (!mux_hw->clk)
> +	       return -EINVAL;
> +

Or maybe even:

		if (!mux_ops)

This would be more self-explanatory and save one dereference.

Best regards,
-- 
Tomasz Figa
Samsung Poland R&D Center
SW Solution Development, Linux Platform

Re: [PATCH V2] clk: Add composite clock type

[Hiroshi Doyu]

Prashant Gaikwad <pgaikwad@nvidia.com> wrote @ Mon, 4 Feb 2013 09:11:22 +0100:
...
If you want to consider the consistency for the other tegra
clk_register(), the following comment can be added although this is a
common function.

+	/* Data in .init is copied by clk_register(), so stack variable OK */
> +       composite->hw.init = &init;
> +
> +       clk = clk_register(dev, &composite->hw);

Re: [PATCH V2] clk: Add composite clock type

[Russell King - ARM Linux]

On Tue, Feb 05, 2013 at 11:22:52AM +0100, Hiroshi Doyu wrote:
> diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
> index f30fb4b..8f88805 100644
> --- a/drivers/clk/clk-composite.c
> +++ b/drivers/clk/clk-composite.c
> @@ -27,6 +27,9 @@ static u8 clk_composite_get_parent(struct clk_hw *hw)
>         const struct clk_ops *mux_ops = composite->mux_ops;
>         struct clk_hw *mux_hw = composite->mux_hw;
>  
> +       if (!mux_hw->clk)
> +	       return -EINVAL;
> +
>         mux_hw->clk = hw->clk;

That just looks totally wrong.

Firstly, according to the hunk, this function has the prototype:

static u8 clk_composite_get_parent(struct clk_hw *hw)

What do you think is the effect of passing -EINVAL back as a 'u8' ?

Secondly, the whole "check mux_hw->clk for NULL, and then overwrite it"
looks really really really wrong.  If it's already set, then why does it
need to be changed?  If it isn't set, why do you need to error out?

Thirdly, why is a function called "get_parent" modifying anything (isn't
it supposed to be _get_ing something?

Re: [PATCH V2] clk: Add composite clock type

[Prashant Gaikwad]

On Tuesday 05 February 2013 03:52 PM, Hiroshi Doyu wrote:
> Prashant Gaikwad <pgaikwad@nvidia.com> wrote @ Tue, 5 Feb 2013 09:33:41 +0100:
>
>>> The members of "clk_composite_ops" seems to be always assigned
>>> statically. Istead of dynamically allocating/assigning, can't we just
>>> have "clk_composite_ops" statically as below?
>>>
>>> static struct clk_ops clk_composite_ops = {
>>> 	.get_parent = clk_composite_get_parent;
>>> 	.set_parent = clk_composite_set_parent;
>>> 	.recalc_rate = clk_composite_recalc_rate;
>>> 	.round_rate = clk_composite_round_rate;
>>> 	.set_rate = clk_composite_set_rate;
>>> 	.is_enabled = clk_composite_is_enabled;
>>> 	.enable = clk_composite_enable;
>>> 	.disable = clk_composite_disable;
>>> };
>>>
>>> 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)
>>> {
>>> 	.....
>>>
>>> 	init.ops = &clk_composite_ops;
>> No, clk_ops depends on the clocks you are using. There could be a clock
>> with mux and gate while another one with mux and div.
> You are right. What about the following? We don't have to have similar
> copy of clk_composite_ops for each instances.

Clock framework takes decision depending on the ops availability and it 
does not know if the clock is mux or gate.

For example,

                 if (clk->ops->enable) {
                         ret = clk->ops->enable(clk->hw);
                         if (ret) {
                                 __clk_disable(clk->parent);
                                 return ret;
                         }
                 }

in above case if clk_composite does not have gate clock then as per your 
suggestion if it returns error value then it will fail and it is wrong.

Hence clock ops are populated depending on the clock types.

> diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
> index f30fb4b..8f88805 100644
> --- a/drivers/clk/clk-composite.c
> +++ b/drivers/clk/clk-composite.c
> @@ -27,6 +27,9 @@ static u8 clk_composite_get_parent(struct clk_hw *hw)
>          const struct clk_ops *mux_ops = composite->mux_ops;
>          struct clk_hw *mux_hw = composite->mux_hw;
>   
> +       if (!mux_hw->clk)
> +	       return -EINVAL;
> +
>          mux_hw->clk = hw->clk;
>   

It is wrong.

>          return mux_ops->get_parent(mux_hw);
> @@ -38,6 +41,9 @@ static int clk_composite_set_parent(struct clk_hw *hw, u8 index)
>          const struct clk_ops *mux_ops = composite->mux_ops;
>          struct clk_hw *mux_hw = composite->mux_hw;
>   
> +       if (!mux_hw->clk)
> +	       return -EINVAL;
> +
>          mux_hw->clk = hw->clk;
>   
>          return mux_ops->set_parent(mux_hw, index);
> @@ -50,6 +56,9 @@ static unsigned long clk_composite_recalc_rate(struct clk_hw *hw,
>          const struct clk_ops *div_ops = composite->div_ops;
>          struct clk_hw *div_hw = composite->div_hw;
>   
> +       if (!div_hw->clk)
> +	       return -EINVAL;
> +
>          div_hw->clk = hw->clk;
>   
>          return div_ops->recalc_rate(div_hw, parent_rate);
> @@ -62,6 +71,9 @@ static long clk_composite_round_rate(struct clk_hw *hw, unsigned long rate,
>          const struct clk_ops *div_ops = composite->div_ops;
>          struct clk_hw *div_hw = composite->div_hw;
>   
> +       if (!div_hw->clk)
> +	       return -EINVAL;
> +
>          div_hw->clk = hw->clk;
>   
>          return div_ops->round_rate(div_hw, rate, prate);
> @@ -74,6 +86,9 @@ static int clk_composite_set_rate(struct clk_hw *hw, unsigned long rate,
>          const struct clk_ops *div_ops = composite->div_ops;
>          struct clk_hw *div_hw = composite->div_hw;
>   
> +       if (!div_hw->clk)
> +	       return -EINVAL;
> +
>          div_hw->clk = hw->clk;
>   
>          return div_ops->set_rate(div_hw, rate, parent_rate);
> @@ -85,6 +100,9 @@ static int clk_composite_is_enabled(struct clk_hw *hw)
>          const struct clk_ops *gate_ops = composite->gate_ops;
>          struct clk_hw *gate_hw = composite->gate_hw;
>   
> +       if (!gate_hw->clk)
> +	       return -EINVAL;
> +
>          gate_hw->clk = hw->clk;
>   
>          return gate_ops->is_enabled(gate_hw);
> @@ -96,6 +114,9 @@ static int clk_composite_enable(struct clk_hw *hw)
>          const struct clk_ops *gate_ops = composite->gate_ops;
>          struct clk_hw *gate_hw = composite->gate_hw;
>   
> +       if (!gate_hw->clk)
> +	       return -EINVAL;
> +
>          gate_hw->clk = hw->clk;
>   
>          return gate_ops->enable(gate_hw);
> @@ -107,11 +128,25 @@ static void clk_composite_disable(struct clk_hw *hw)
>          const struct clk_ops *gate_ops = composite->gate_ops;
>          struct clk_hw *gate_hw = composite->gate_hw;
>   
> +       if (!gate_hw->clk)
> +	       return -EINVAL;
> +
>          gate_hw->clk = hw->clk;
>   
>          gate_ops->disable(gate_hw);
>   }
>   
> +static struct clk_ops clk_composite_ops = {
> +	.get_parent = clk_composite_get_parent,
> +	.set_parent = clk_composite_set_parent,
> +	.recalc_rate = clk_composite_recalc_rate,
> +	.round_rate = clk_composite_round_rate,
> +	.set_rate = clk_composite_set_rate,
> +	.is_enabled = clk_composite_is_enabled,
> +	.enable = clk_composite_enable,
> +	.disable = clk_composite_disable,
> +};
> +
>   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,
> @@ -135,14 +170,6 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
>          init.parent_names = parent_names;
>          init.num_parents = num_parents;
>   
> -       /* allocate the clock ops */
> -       clk_composite_ops = kzalloc(sizeof(*clk_composite_ops), GFP_KERNEL);
> -       if (!clk_composite_ops) {
> -               pr_err("%s: could not allocate clk ops\n", __func__);
> -               kfree(composite);
> -               return ERR_PTR(-ENOMEM);
> -       }
> -
>          if (mux_hw && mux_ops) {
>                  if (!mux_ops->get_parent || !mux_ops->set_parent) {
>                          clk = ERR_PTR(-EINVAL);
> @@ -151,8 +178,6 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
>   
>                  composite->mux_hw = mux_hw;
>                  composite->mux_ops = mux_ops;
> -               clk_composite_ops->get_parent = clk_composite_get_parent;
> -               clk_composite_ops->set_parent = clk_composite_set_parent;
>          }
>   
>          if (div_hw && div_ops) {
> @@ -164,9 +189,6 @@ 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->round_rate = clk_composite_round_rate;
> -               clk_composite_ops->set_rate = clk_composite_set_rate;
>          }
>   
>          if (gate_hw && gate_ops) {
> @@ -178,12 +200,9 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
>   
>                  composite->gate_hw = gate_hw;
>                  composite->gate_ops = gate_ops;
> -               clk_composite_ops->is_enabled = clk_composite_is_enabled;
> -               clk_composite_ops->enable = clk_composite_enable;
> -               clk_composite_ops->disable = clk_composite_disable;
>          }
>   
> -       init.ops = clk_composite_ops;
> +       init.ops = &clk_composite_ops;
>          composite->hw.init = &init;
>   
>          clk = clk_register(dev, &composite->hw);
> @@ -202,7 +221,6 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
>          return clk;
>   
>   err:
> -       kfree(clk_composite_ops);
>          kfree(composite);
>          return clk;
>   }

Re: [PATCH V2] clk: Add composite clock type

[Prashant Gaikwad]

On Tuesday 05 February 2013 03:45 PM, Tomasz Figa wrote:
> Hi Prashant,
>
> Thank you for your patch. Please see some comments inline.
>
> On Monday 04 of February 2013 13:41:22 Prashant Gaikwad wrote:
>> Not all clocks are required to be decomposed into basic clock
>> types but at the same time want to use the functionality
>> provided by these basic clock types instead of duplicating.
>>
>> For example, Tegra SoC has ~100 clocks which can be decomposed
>> into Mux -> Div -> Gate clock types making the clock count to
>> ~300. Also, parent change operation can not be performed on gate
>> clock which forces to use mux clock in driver if want to change
>> the parent.
>>
>> Instead aggregate the basic clock types functionality into one
>> clock and just use this clock for all operations. This clock
>> type re-uses the functionality of basic clock types and not
>> limited to basic clock types but any hardware-specific
>> implementation.
>>
>> Signed-off-by: Prashant Gaikwad <pgaikwad@nvidia.com>
>> ---
>>
>> Changes from V1:
>> - 2nd patch dropped as the concept is acked by Mike.
>> - Fixed comments from Stephen.
>>
>> ---
>>   drivers/clk/Makefile         |    1 +
>>   drivers/clk/clk-composite.c  |  208
>> ++++++++++++++++++++++++++++++++++++++++++ include/linux/clk-provider.h
>> |   30 ++++++
>>   3 files changed, 239 insertions(+), 0 deletions(-)
>>   create mode 100644 drivers/clk/clk-composite.c
>>
>> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
>> index ce77077..2287848 100644
>> --- a/drivers/clk/Makefile
>> +++ b/drivers/clk/Makefile
>> @@ -7,6 +7,7 @@ obj-$(CONFIG_COMMON_CLK)      += clk-fixed-factor.o
>>   obj-$(CONFIG_COMMON_CLK)     += clk-fixed-rate.o
>>   obj-$(CONFIG_COMMON_CLK)     += clk-gate.o
>>   obj-$(CONFIG_COMMON_CLK)     += clk-mux.o
>> +obj-$(CONFIG_COMMON_CLK)     += clk-composite.o
>>
>>   # SoCs specific
>>   obj-$(CONFIG_ARCH_BCM2835)   += clk-bcm2835.o
>> diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
>> new file mode 100644
>> index 0000000..5a6587f
>> --- /dev/null
>> +++ b/drivers/clk/clk-composite.c
>> @@ -0,0 +1,208 @@
>> +/*
>> + * Copyright (c) 2013 NVIDIA CORPORATION.  All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> it + * under the terms and conditions of the GNU General Public
>> License, + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but
>> WITHOUT + * ANY WARRANTY; without even the implied warranty of
>> MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> General Public License for + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see
>> <http://www.gnu.org/licenses/>. + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/err.h>
>> +#include <linux/slab.h>
>> +
>> +#define to_clk_composite(_hw) container_of(_hw, struct clk_composite,
>> hw) +
>> +static u8 clk_composite_get_parent(struct clk_hw *hw)
>> +{
>> +     struct clk_composite *composite = to_clk_composite(hw);
>> +     const struct clk_ops *mux_ops = composite->mux_ops;
>> +     struct clk_hw *mux_hw = composite->mux_hw;
>> +
>> +     mux_hw->clk = hw->clk;
> Why is this needed? Looks like this filed is already being initialized in
> clk_register_composite.

Some ops will get called during clk_init where this clk is not populated 
hence doing here. I have done it for all ops to make sure that any 
future change in clock framework don't break this clock.
Now, why duplicate it in clk_register_composite? It is possible that 
none of these ops get called in clk_init.
For example, recalc_rate is called during init and this ops is supported 
by div clock type, but what if div clock is not added.

I hope this explains the need.

>> +
>> +     return mux_ops->get_parent(mux_hw);
>> +}
>> +
>> +static int clk_composite_set_parent(struct clk_hw *hw, u8 index)
>> +{
>> +     struct clk_composite *composite = to_clk_composite(hw);
>> +     const struct clk_ops *mux_ops = composite->mux_ops;
>> +     struct clk_hw *mux_hw = composite->mux_hw;
>> +
>> +     mux_hw->clk = hw->clk;
> Ditto.
>
>> +
>> +     return mux_ops->set_parent(mux_hw, index);
>> +}
>> +
>> +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;
>> +
>> +     div_hw->clk = hw->clk;
> Ditto.
>
>> +
>> +     return div_ops->recalc_rate(div_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;
>> +
>> +     div_hw->clk = hw->clk;
> Ditto.
>
>> +
>> +     return div_ops->round_rate(div_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;
>> +
>> +     div_hw->clk = hw->clk;
> Ditto.
>
>> +
>> +     return div_ops->set_rate(div_hw, rate, parent_rate);
>> +}
>> +
>> +static int clk_composite_is_enabled(struct clk_hw *hw)
>> +{
>> +     struct clk_composite *composite = to_clk_composite(hw);
>> +     const struct clk_ops *gate_ops = composite->gate_ops;
>> +     struct clk_hw *gate_hw = composite->gate_hw;
>> +
>> +     gate_hw->clk = hw->clk;
> Ditto.
>
>> +
>> +     return gate_ops->is_enabled(gate_hw);
>> +}
>> +
>> +static int clk_composite_enable(struct clk_hw *hw)
>> +{
>> +     struct clk_composite *composite = to_clk_composite(hw);
>> +     const struct clk_ops *gate_ops = composite->gate_ops;
>> +     struct clk_hw *gate_hw = composite->gate_hw;
>> +
>> +     gate_hw->clk = hw->clk;
> Ditto.
>
>> +
>> +     return gate_ops->enable(gate_hw);
>> +}
>> +
>> +static void clk_composite_disable(struct clk_hw *hw)
>> +{
>> +     struct clk_composite *composite = to_clk_composite(hw);
>> +     const struct clk_ops *gate_ops = composite->gate_ops;
>> +     struct clk_hw *gate_hw = composite->gate_hw;
>> +
>> +     gate_hw->clk = hw->clk;
> Ditto.
>
>> +
>> +     gate_ops->disable(gate_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 *gate_hw, const struct clk_ops *gate_ops,
>> +                     unsigned long flags)
>> +{
>> +     struct clk *clk;
>> +     struct clk_init_data init;
>> +     struct clk_composite *composite;
>> +     struct clk_ops *clk_composite_ops;
>> +
>> +     composite = kzalloc(sizeof(*composite), GFP_KERNEL);
>> +     if (!composite) {
>> +             pr_err("%s: could not allocate composite clk\n", __func__);
>> +             return ERR_PTR(-ENOMEM);
>> +     }
>> +
>> +     init.name = name;
>> +     init.flags = flags | CLK_IS_BASIC;
>> +     init.parent_names = parent_names;
>> +     init.num_parents = num_parents;
>> +
>> +     /* allocate the clock ops */
>> +     clk_composite_ops = kzalloc(sizeof(*clk_composite_ops), GFP_KERNEL);
>> +     if (!clk_composite_ops) {
>> +             pr_err("%s: could not allocate clk ops\n", __func__);
>> +             kfree(composite);
>> +             return ERR_PTR(-ENOMEM);
>> +     }
> Maybe it would be better to embed this struct clk_ops inside struct
> clk_composite? This would allow to get rid of this allocation.
>
>> +
>> +     if (mux_hw && mux_ops) {
>> +             if (!mux_ops->get_parent || !mux_ops->set_parent) {
>> +                     clk = ERR_PTR(-EINVAL);
>> +                     goto err;
>> +             }
>> +
>> +             composite->mux_hw = mux_hw;
>> +             composite->mux_ops = mux_ops;
>> +             clk_composite_ops->get_parent = clk_composite_get_parent;
>> +             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) {
>> +                     clk = ERR_PTR(-EINVAL);
>> +                     goto err;
>> +             }
>> +
>> +             composite->div_hw = div_hw;
>> +             composite->div_ops = div_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) {
>> +             if (!gate_ops->is_enabled || !gate_ops->enable ||
>> +                 !gate_ops->disable) {
>> +                     clk = ERR_PTR(-EINVAL);
>> +                     goto err;
>> +             }
>> +
>> +             composite->gate_hw = gate_hw;
>> +             composite->gate_ops = gate_ops;
>> +             clk_composite_ops->is_enabled = clk_composite_is_enabled;
>> +             clk_composite_ops->enable = clk_composite_enable;
>> +             clk_composite_ops->disable = clk_composite_disable;
>> +     }
>> +
>> +     init.ops = clk_composite_ops;
>> +     composite->hw.init = &init;
>> +
>> +     clk = clk_register(dev, &composite->hw);
>> +     if (IS_ERR(clk))
>> +             goto err;
>> +
>> +     if (composite->mux_hw)
>> +             composite->mux_hw->clk = clk;
>> +
>> +     if (composite->div_hw)
>> +             composite->div_hw->clk = clk;
>> +
>> +     if (composite->gate_hw)
>> +             composite->gate_hw->clk = clk;
>> +
>> +     return clk;
>> +
>> +err:
>> +     kfree(clk_composite_ops);
>> +     kfree(composite);
>> +     return clk;
>> +}
>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
>> index 7f197d7..f1a36aa 100644
>> --- a/include/linux/clk-provider.h
>> +++ b/include/linux/clk-provider.h
>> @@ -325,6 +325,36 @@ struct clk *clk_register_fixed_factor(struct device
>> *dev, const char *name, const char *parent_name, unsigned long flags,
>>                unsigned int mult, unsigned int div);
>>
>> +/***
>> + * 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_ops:   clock ops for mux
>> + * @div_ops: clock ops for divider
>> + * @gate_ops:        clock ops for gate
>> + */
>> +struct clk_composite {
>> +     struct clk_hw   hw;
> As I suggested above:
>
>          struct clk_ops  ops;
>
>> +
>> +     struct clk_hw   *mux_hw;
>> +     struct clk_hw   *div_hw;
>> +     struct clk_hw   *gate_hw;
>> +
>> +     const struct clk_ops    *mux_ops;
>> +     const struct clk_ops    *div_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 *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
> Best regards,
> --
> Tomasz Figa
> Samsung Poland R&D Center
> SW Solution Development, Linux Platform
>

Re: [PATCH V2] clk: Add composite clock type

[Hiroshi Doyu]

Prashant Gaikwad <pgaikwad@nvidia.com> wrote @ Wed, 6 Feb 2013 03:55:00 +0100:

> >> No, clk_ops depends on the clocks you are using. There could be a clock
> >> with mux and gate while another one with mux and div.
> > You are right. What about the following? We don't have to have similar
> > copy of clk_composite_ops for each instances.
> 
> Clock framework takes decision depending on the ops availability and it 
> does not know if the clock is mux or gate.
> 
> For example,
> 
>                  if (clk->ops->enable) {
>                          ret = clk->ops->enable(clk->hw);
>                          if (ret) {
>                                  __clk_disable(clk->parent);
>                                  return ret;
>                          }
>                  }
> 
> in above case if clk_composite does not have gate clock then as per your 
> suggestion if it returns error value then it will fail and it is wrong.

Ok, now I understand. Thank you for explanation.

We always need to allocate clk_composite_ops for each clk_composite,
right? If so what about having "struct clk_ops ops" in "struct
clk_composite"?

diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
index f30fb4b..5240e24 100644
--- a/drivers/clk/clk-composite.c
+++ b/drivers/clk/clk-composite.c
@@ -129,20 +129,13 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
                pr_err("%s: could not allocate composite clk\n", __func__);
                return ERR_PTR(-ENOMEM);
        }
+       clk_composite_ops = &composite->ops;
 
        init.name = name;
        init.flags = flags | CLK_IS_BASIC;
        init.parent_names = parent_names;
        init.num_parents = num_parents;
 
-       /* allocate the clock ops */
-       clk_composite_ops = kzalloc(sizeof(*clk_composite_ops), GFP_KERNEL);
-       if (!clk_composite_ops) {
-               pr_err("%s: could not allocate clk ops\n", __func__);
-               kfree(composite);
-               return ERR_PTR(-ENOMEM);
-       }
-
        if (mux_hw && mux_ops) {
                if (!mux_ops->get_parent || !mux_ops->set_parent) {
                        clk = ERR_PTR(-EINVAL);
@@ -202,7 +195,6 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
        return clk;
 
 err:
-       kfree(clk_composite_ops);
        kfree(composite);
        return clk;
 }
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index f0ac818..bb5d36a 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -346,6 +346,8 @@ struct clk_composite {
        const struct clk_ops    *mux_ops;
        const struct clk_ops    *div_ops;
        const struct clk_ops    *gate_ops;
+
+       const struct clk_ops    ops;
 };
 
 struct clk *clk_register_composite(struct device *dev, const char *name,


> > diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
> > index f30fb4b..8f88805 100644
> > --- a/drivers/clk/clk-composite.c
> > +++ b/drivers/clk/clk-composite.c
> > @@ -27,6 +27,9 @@ static u8 clk_composite_get_parent(struct clk_hw *hw)
> >          const struct clk_ops *mux_ops = composite->mux_ops;
> >          struct clk_hw *mux_hw = composite->mux_hw;
> >   
> > +       if (!mux_hw->clk)
> > +	       return -EINVAL;
> > +
> >          mux_hw->clk = hw->clk;
>
> It is wrong.

Will the above "mux_hw->clk = hw->clk" be removed from the original?

Re: [PATCH V2] clk: Add composite clock type

[Prashant Gaikwad]

On Wednesday 06 February 2013 11:40 AM, Hiroshi Doyu wrote:
> Prashant Gaikwad <pgaikwad@nvidia.com> wrote @ Wed, 6 Feb 2013 03:55:00 +0100:
>
>>>> No, clk_ops depends on the clocks you are using. There could be a clock
>>>> with mux and gate while another one with mux and div.
>>> You are right. What about the following? We don't have to have similar
>>> copy of clk_composite_ops for each instances.
>> Clock framework takes decision depending on the ops availability and it
>> does not know if the clock is mux or gate.
>>
>> For example,
>>
>>                   if (clk->ops->enable) {
>>                           ret = clk->ops->enable(clk->hw);
>>                           if (ret) {
>>                                   __clk_disable(clk->parent);
>>                                   return ret;
>>                           }
>>                   }
>>
>> in above case if clk_composite does not have gate clock then as per your
>> suggestion if it returns error value then it will fail and it is wrong.
> Ok, now I understand. Thank you for explanation.
>
> We always need to allocate clk_composite_ops for each clk_composite,
> right? If so what about having "struct clk_ops ops" in "struct
> clk_composite"?
>
> diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
> index f30fb4b..5240e24 100644
> --- a/drivers/clk/clk-composite.c
> +++ b/drivers/clk/clk-composite.c
> @@ -129,20 +129,13 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
>                  pr_err("%s: could not allocate composite clk\n", __func__);
>                  return ERR_PTR(-ENOMEM);
>          }
> +       clk_composite_ops = &composite->ops;
>   
>          init.name = name;
>          init.flags = flags | CLK_IS_BASIC;
>          init.parent_names = parent_names;
>          init.num_parents = num_parents;
>   
> -       /* allocate the clock ops */
> -       clk_composite_ops = kzalloc(sizeof(*clk_composite_ops), GFP_KERNEL);
> -       if (!clk_composite_ops) {
> -               pr_err("%s: could not allocate clk ops\n", __func__);
> -               kfree(composite);
> -               return ERR_PTR(-ENOMEM);
> -       }
> -
>          if (mux_hw && mux_ops) {
>                  if (!mux_ops->get_parent || !mux_ops->set_parent) {
>                          clk = ERR_PTR(-EINVAL);
> @@ -202,7 +195,6 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
>          return clk;
>   
>   err:
> -       kfree(clk_composite_ops);
>          kfree(composite);
>          return clk;
>   }
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index f0ac818..bb5d36a 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -346,6 +346,8 @@ struct clk_composite {
>          const struct clk_ops    *mux_ops;
>          const struct clk_ops    *div_ops;
>          const struct clk_ops    *gate_ops;
> +
> +       const struct clk_ops    ops;
>   };
>   
>   struct clk *clk_register_composite(struct device *dev, const char *name,

This will work, but there is no harm in allocating dynamically. What is 
preferred?

>
>>> diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
>>> index f30fb4b..8f88805 100644
>>> --- a/drivers/clk/clk-composite.c
>>> +++ b/drivers/clk/clk-composite.c
>>> @@ -27,6 +27,9 @@ static u8 clk_composite_get_parent(struct clk_hw *hw)
>>>           const struct clk_ops *mux_ops = composite->mux_ops;
>>>           struct clk_hw *mux_hw = composite->mux_hw;
>>>    
>>> +       if (!mux_hw->clk)
>>> +	       return -EINVAL;
>>> +
>>>           mux_hw->clk = hw->clk;
>> It is wrong.
> Will the above "mux_hw->clk = hw->clk" be removed from the original?

Re: [PATCH V2] clk: Add composite clock type

[Hiroshi Doyu]

Prashant Gaikwad <pgaikwad@nvidia.com> wrote @ Wed, 6 Feb 2013 10:52:54 +0100:

> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > index f0ac818..bb5d36a 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -346,6 +346,8 @@ struct clk_composite {
> >          const struct clk_ops    *mux_ops;
> >          const struct clk_ops    *div_ops;
> >          const struct clk_ops    *gate_ops;
> > +
> > +       const struct clk_ops    ops;
> >   };
> >   
> >   struct clk *clk_register_composite(struct device *dev, const char *name,
> 
> This will work, but there is no harm in allocating dynamically. What is 
> preferred?

If we've already know that this "ops" is necessary per "struct
clk_composite" in advance, there's no point to allocate
"clk_composite" and "ops" separately.

Re: [PATCH V2] clk: Add composite clock type

[Tomasz Figa]

On Wednesday 06 of February 2013 15:22:54 Prashant Gaikwad wrote:
> On Wednesday 06 February 2013 11:40 AM, Hiroshi Doyu wrote:
> > Prashant Gaikwad <pgaikwad@nvidia.com> wrote @ Wed, 6 Feb 2013 
03:55:00 +0100:
> >>>> No, clk_ops depends on the clocks you are using. There could be a
> >>>> clock
> >>>> with mux and gate while another one with mux and div.
> >>> 
> >>> You are right. What about the following? We don't have to have
> >>> similar
> >>> copy of clk_composite_ops for each instances.
> >> 
> >> Clock framework takes decision depending on the ops availability and
> >> it
> >> does not know if the clock is mux or gate.
> >> 
> >> For example,
> >> 
> >>                   if (clk->ops->enable) {
> >>                   
> >>                           ret = clk->ops->enable(clk->hw);
> >>                           if (ret) {
> >>                           
> >>                                   __clk_disable(clk->parent);
> >>                                   return ret;
> >>                           
> >>                           }
> >>                   
> >>                   }
> >> 
> >> in above case if clk_composite does not have gate clock then as per
> >> your suggestion if it returns error value then it will fail and it
> >> is wrong.> 
> > Ok, now I understand. Thank you for explanation.
> > 
> > We always need to allocate clk_composite_ops for each clk_composite,
> > right? If so what about having "struct clk_ops ops" in "struct
> > clk_composite"?
> > 
> > diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
> > index f30fb4b..5240e24 100644
> > --- a/drivers/clk/clk-composite.c
> > +++ b/drivers/clk/clk-composite.c
> > @@ -129,20 +129,13 @@ struct clk *clk_register_composite(struct device
> > *dev, const char *name,> 
> >                  pr_err("%s: could not allocate composite clk\n",
> >                  __func__);
> >                  return ERR_PTR(-ENOMEM);
> >          
> >          }
> > 
> > +       clk_composite_ops = &composite->ops;
> > 
> >          init.name = name;
> >          init.flags = flags | CLK_IS_BASIC;
> >          init.parent_names = parent_names;
> >          init.num_parents = num_parents;
> > 
> > -       /* allocate the clock ops */
> > -       clk_composite_ops = kzalloc(sizeof(*clk_composite_ops),
> > GFP_KERNEL); -       if (!clk_composite_ops) {
> > -               pr_err("%s: could not allocate clk ops\n", __func__);
> > -               kfree(composite);
> > -               return ERR_PTR(-ENOMEM);
> > -       }
> > -
> > 
> >          if (mux_hw && mux_ops) {
> >          
> >                  if (!mux_ops->get_parent || !mux_ops->set_parent) {
> >                  
> >                          clk = ERR_PTR(-EINVAL);
> > 
> > @@ -202,7 +195,6 @@ struct clk *clk_register_composite(struct device
> > *dev, const char *name,> 
> >          return clk;
> >   
> >   err:
> > -       kfree(clk_composite_ops);
> > 
> >          kfree(composite);
> >          return clk;
> >   
> >   }
> > 
> > diff --git a/include/linux/clk-provider.h
> > b/include/linux/clk-provider.h index f0ac818..bb5d36a 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -346,6 +346,8 @@ struct clk_composite {
> > 
> >          const struct clk_ops    *mux_ops;
> >          const struct clk_ops    *div_ops;
> >          const struct clk_ops    *gate_ops;
> > 
> > +
> > +       const struct clk_ops    ops;
> > 
> >   };
> >   
> >   struct clk *clk_register_composite(struct device *dev, const char
> >   *name,
> This will work, but there is no harm in allocating dynamically. What is
> preferred?

IMHO it is always better to allocate one bigger structure than several 
smaller if they are always needed together and one cannot exist without 
others.

Best regards,
-- 
Tomasz Figa
Samsung Poland R&D Center
SW Solution Development, Linux Platform

Re: [PATCH V2] clk: Add composite clock type

[Tomasz Figa]

On Wednesday 06 of February 2013 08:34:32 Prashant Gaikwad wrote:
> On Tuesday 05 February 2013 03:45 PM, Tomasz Figa wrote:
> > Hi Prashant,
> > 
> > Thank you for your patch. Please see some comments inline.
> > 
> > On Monday 04 of February 2013 13:41:22 Prashant Gaikwad wrote:
> >> Not all clocks are required to be decomposed into basic clock
> >> types but at the same time want to use the functionality
> >> provided by these basic clock types instead of duplicating.
> >> 
> >> For example, Tegra SoC has ~100 clocks which can be decomposed
> >> into Mux -> Div -> Gate clock types making the clock count to
> >> ~300. Also, parent change operation can not be performed on gate
> >> clock which forces to use mux clock in driver if want to change
> >> the parent.
> >> 
> >> Instead aggregate the basic clock types functionality into one
> >> clock and just use this clock for all operations. This clock
> >> type re-uses the functionality of basic clock types and not
> >> limited to basic clock types but any hardware-specific
> >> implementation.
> >> 
> >> Signed-off-by: Prashant Gaikwad <pgaikwad@nvidia.com>
> >> ---
> >> 
> >> Changes from V1:
> >> - 2nd patch dropped as the concept is acked by Mike.
> >> - Fixed comments from Stephen.
> >> 
> >> ---
> >> 
> >>   drivers/clk/Makefile         |    1 +
> >>   drivers/clk/clk-composite.c  |  208
> >> 
> >> ++++++++++++++++++++++++++++++++++++++++++
> >> include/linux/clk-provider.h
> >> 
> >> |   30 ++++++
> >>   
> >>   3 files changed, 239 insertions(+), 0 deletions(-)
> >>   create mode 100644 drivers/clk/clk-composite.c
> >> 
> >> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> >> index ce77077..2287848 100644
> >> --- a/drivers/clk/Makefile
> >> +++ b/drivers/clk/Makefile
> >> @@ -7,6 +7,7 @@ obj-$(CONFIG_COMMON_CLK)      += clk-fixed-factor.o
> >> 
> >>   obj-$(CONFIG_COMMON_CLK)     += clk-fixed-rate.o
> >>   obj-$(CONFIG_COMMON_CLK)     += clk-gate.o
> >>   obj-$(CONFIG_COMMON_CLK)     += clk-mux.o
> >> 
> >> +obj-$(CONFIG_COMMON_CLK)     += clk-composite.o
> >> 
> >>   # SoCs specific
> >>   obj-$(CONFIG_ARCH_BCM2835)   += clk-bcm2835.o
> >> 
> >> diff --git a/drivers/clk/clk-composite.c
> >> b/drivers/clk/clk-composite.c
> >> new file mode 100644
> >> index 0000000..5a6587f
> >> --- /dev/null
> >> +++ b/drivers/clk/clk-composite.c
> >> @@ -0,0 +1,208 @@
> >> +/*
> >> + * Copyright (c) 2013 NVIDIA CORPORATION.  All rights reserved.
> >> + *
> >> + * This program is free software; you can redistribute it and/or
> >> modify it + * under the terms and conditions of the GNU General
> >> Public License, + * version 2, as published by the Free Software
> >> Foundation. + *
> >> + * This program is distributed in the hope it will be useful, but
> >> WITHOUT + * ANY WARRANTY; without even the implied warranty of
> >> MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >> General Public License for + * more details.
> >> + *
> >> + * You should have received a copy of the GNU General Public License
> >> + * along with this program.  If not, see
> >> <http://www.gnu.org/licenses/>. + */
> >> +
> >> +#include <linux/clk.h>
> >> +#include <linux/clk-provider.h>
> >> +#include <linux/err.h>
> >> +#include <linux/slab.h>
> >> +
> >> +#define to_clk_composite(_hw) container_of(_hw, struct
> >> clk_composite,
> >> hw) +
> >> +static u8 clk_composite_get_parent(struct clk_hw *hw)
> >> +{
> >> +     struct clk_composite *composite = to_clk_composite(hw);
> >> +     const struct clk_ops *mux_ops = composite->mux_ops;
> >> +     struct clk_hw *mux_hw = composite->mux_hw;
> >> +
> >> +     mux_hw->clk = hw->clk;
> > 
> > Why is this needed? Looks like this filed is already being initialized
> > in clk_register_composite.
> 
> Some ops will get called during clk_init where this clk is not populated
> hence doing here. I have done it for all ops to make sure that any
> future change in clock framework don't break this clock.
> Now, why duplicate it in clk_register_composite? It is possible that
> none of these ops get called in clk_init.
> For example, recalc_rate is called during init and this ops is supported
> by div clock type, but what if div clock is not added.
> 
> I hope this explains the need.
> 

Sorry, I don't understand your explanation.

I have asked why mux_hw->clk field has to be reinitialized in all the ops.

In other words, is it even possible that this clk pointer changes since 
calling clk_register from clk_register_composite and if yes, why?

Best regards,
-- 
Tomasz Figa
Samsung Poland R&D Center
SW Solution Development, Linux Platform

> >> +
> >> +     return mux_ops->get_parent(mux_hw);
> >> +}
> >> +
> >> +static int clk_composite_set_parent(struct clk_hw *hw, u8 index)
> >> +{
> >> +     struct clk_composite *composite = to_clk_composite(hw);
> >> +     const struct clk_ops *mux_ops = composite->mux_ops;
> >> +     struct clk_hw *mux_hw = composite->mux_hw;
> >> +
> >> +     mux_hw->clk = hw->clk;
> > 
> > Ditto.
> > 
> >> +
> >> +     return mux_ops->set_parent(mux_hw, index);
> >> +}
> >> +
> >> +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;
> >> +
> >> +     div_hw->clk = hw->clk;
> > 
> > Ditto.
> > 
> >> +
> >> +     return div_ops->recalc_rate(div_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;
> >> +
> >> +     div_hw->clk = hw->clk;
> > 
> > Ditto.
> > 
> >> +
> >> +     return div_ops->round_rate(div_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;
> >> +
> >> +     div_hw->clk = hw->clk;
> > 
> > Ditto.
> > 
> >> +
> >> +     return div_ops->set_rate(div_hw, rate, parent_rate);
> >> +}
> >> +
> >> +static int clk_composite_is_enabled(struct clk_hw *hw)
> >> +{
> >> +     struct clk_composite *composite = to_clk_composite(hw);
> >> +     const struct clk_ops *gate_ops = composite->gate_ops;
> >> +     struct clk_hw *gate_hw = composite->gate_hw;
> >> +
> >> +     gate_hw->clk = hw->clk;
> > 
> > Ditto.
> > 
> >> +
> >> +     return gate_ops->is_enabled(gate_hw);
> >> +}
> >> +
> >> +static int clk_composite_enable(struct clk_hw *hw)
> >> +{
> >> +     struct clk_composite *composite = to_clk_composite(hw);
> >> +     const struct clk_ops *gate_ops = composite->gate_ops;
> >> +     struct clk_hw *gate_hw = composite->gate_hw;
> >> +
> >> +     gate_hw->clk = hw->clk;
> > 
> > Ditto.
> > 
> >> +
> >> +     return gate_ops->enable(gate_hw);
> >> +}
> >> +
> >> +static void clk_composite_disable(struct clk_hw *hw)
> >> +{
> >> +     struct clk_composite *composite = to_clk_composite(hw);
> >> +     const struct clk_ops *gate_ops = composite->gate_ops;
> >> +     struct clk_hw *gate_hw = composite->gate_hw;
> >> +
> >> +     gate_hw->clk = hw->clk;
> > 
> > Ditto.
> > 
> >> +
> >> +     gate_ops->disable(gate_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 *gate_hw, const struct clk_ops *gate_ops, +                  
> >>   unsigned long flags)
> >> +{
> >> +     struct clk *clk;
> >> +     struct clk_init_data init;
> >> +     struct clk_composite *composite;
> >> +     struct clk_ops *clk_composite_ops;
> >> +
> >> +     composite = kzalloc(sizeof(*composite), GFP_KERNEL);
> >> +     if (!composite) {
> >> +             pr_err("%s: could not allocate composite clk\n",
> >> __func__); +             return ERR_PTR(-ENOMEM);
> >> +     }
> >> +
> >> +     init.name = name;
> >> +     init.flags = flags | CLK_IS_BASIC;
> >> +     init.parent_names = parent_names;
> >> +     init.num_parents = num_parents;
> >> +
> >> +     /* allocate the clock ops */
> >> +     clk_composite_ops = kzalloc(sizeof(*clk_composite_ops),
> >> GFP_KERNEL); +     if (!clk_composite_ops) {
> >> +             pr_err("%s: could not allocate clk ops\n", __func__);
> >> +             kfree(composite);
> >> +             return ERR_PTR(-ENOMEM);
> >> +     }
> > 
> > Maybe it would be better to embed this struct clk_ops inside struct
> > clk_composite? This would allow to get rid of this allocation.
> > 
> >> +
> >> +     if (mux_hw && mux_ops) {
> >> +             if (!mux_ops->get_parent || !mux_ops->set_parent) {
> >> +                     clk = ERR_PTR(-EINVAL);
> >> +                     goto err;
> >> +             }
> >> +
> >> +             composite->mux_hw = mux_hw;
> >> +             composite->mux_ops = mux_ops;
> >> +             clk_composite_ops->get_parent =
> >> clk_composite_get_parent;
> >> +             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) {
> >> +                     clk = ERR_PTR(-EINVAL);
> >> +                     goto err;
> >> +             }
> >> +
> >> +             composite->div_hw = div_hw;
> >> +             composite->div_ops = div_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) {
> >> +             if (!gate_ops->is_enabled || !gate_ops->enable ||
> >> +                 !gate_ops->disable) {
> >> +                     clk = ERR_PTR(-EINVAL);
> >> +                     goto err;
> >> +             }
> >> +
> >> +             composite->gate_hw = gate_hw;
> >> +             composite->gate_ops = gate_ops;
> >> +             clk_composite_ops->is_enabled =
> >> clk_composite_is_enabled;
> >> +             clk_composite_ops->enable = clk_composite_enable;
> >> +             clk_composite_ops->disable = clk_composite_disable;
> >> +     }
> >> +
> >> +     init.ops = clk_composite_ops;
> >> +     composite->hw.init = &init;
> >> +
> >> +     clk = clk_register(dev, &composite->hw);
> >> +     if (IS_ERR(clk))
> >> +             goto err;
> >> +
> >> +     if (composite->mux_hw)
> >> +             composite->mux_hw->clk = clk;
> >> +
> >> +     if (composite->div_hw)
> >> +             composite->div_hw->clk = clk;
> >> +
> >> +     if (composite->gate_hw)
> >> +             composite->gate_hw->clk = clk;
> >> +
> >> +     return clk;
> >> +
> >> +err:
> >> +     kfree(clk_composite_ops);
> >> +     kfree(composite);
> >> +     return clk;
> >> +}
> >> diff --git a/include/linux/clk-provider.h
> >> b/include/linux/clk-provider.h index 7f197d7..f1a36aa 100644
> >> --- a/include/linux/clk-provider.h
> >> +++ b/include/linux/clk-provider.h
> >> @@ -325,6 +325,36 @@ struct clk *clk_register_fixed_factor(struct
> >> device *dev, const char *name, const char *parent_name, unsigned
> >> long flags,>> 
> >>                unsigned int mult, unsigned int div);
> >> 
> >> +/***
> >> + * 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_ops:   clock ops
> >> for mux
> >> + * @div_ops: clock ops for divider
> >> + * @gate_ops:        clock ops for gate
> >> + */
> >> +struct clk_composite {
> >> +     struct clk_hw   hw;
> > 
> > As I suggested above:
> >          struct clk_ops  ops;
> >> 
> >> +
> >> +     struct clk_hw   *mux_hw;
> >> +     struct clk_hw   *div_hw;
> >> +     struct clk_hw   *gate_hw;
> >> +
> >> +     const struct clk_ops    *mux_ops;
> >> +     const struct clk_ops    *div_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 *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
> > 
> > Best regards,
> > --
> > Tomasz Figa
> > Samsung Poland R&D Center
> > SW Solution Development, Linux Platform
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2] clk: Add composite clock type

[Prashant Gaikwad]

On Wednesday 06 February 2013 03:36 PM, Tomasz Figa wrote:
> On Wednesday 06 of February 2013 08:34:32 Prashant Gaikwad wrote:
>> On Tuesday 05 February 2013 03:45 PM, Tomasz Figa wrote:
>>> Hi Prashant,
>>>
>>> Thank you for your patch. Please see some comments inline.
>>>
>>> On Monday 04 of February 2013 13:41:22 Prashant Gaikwad wrote:
>>>> Not all clocks are required to be decomposed into basic clock
>>>> types but at the same time want to use the functionality
>>>> provided by these basic clock types instead of duplicating.
>>>>
>>>> For example, Tegra SoC has ~100 clocks which can be decomposed
>>>> into Mux -> Div -> Gate clock types making the clock count to
>>>> ~300. Also, parent change operation can not be performed on gate
>>>> clock which forces to use mux clock in driver if want to change
>>>> the parent.
>>>>
>>>> Instead aggregate the basic clock types functionality into one
>>>> clock and just use this clock for all operations. This clock
>>>> type re-uses the functionality of basic clock types and not
>>>> limited to basic clock types but any hardware-specific
>>>> implementation.
>>>>
>>>> Signed-off-by: Prashant Gaikwad <pgaikwad@nvidia.com>
>>>> ---
>>>>
>>>> Changes from V1:
>>>> - 2nd patch dropped as the concept is acked by Mike.
>>>> - Fixed comments from Stephen.
>>>>
>>>> ---
>>>>
>>>>    drivers/clk/Makefile         |    1 +
>>>>    drivers/clk/clk-composite.c  |  208
>>>>
>>>> ++++++++++++++++++++++++++++++++++++++++++
>>>> include/linux/clk-provider.h
>>>>
>>>> |   30 ++++++
>>>>
>>>>    3 files changed, 239 insertions(+), 0 deletions(-)
>>>>    create mode 100644 drivers/clk/clk-composite.c
>>>>
>>>> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
>>>> index ce77077..2287848 100644
>>>> --- a/drivers/clk/Makefile
>>>> +++ b/drivers/clk/Makefile
>>>> @@ -7,6 +7,7 @@ obj-$(CONFIG_COMMON_CLK)      += clk-fixed-factor.o
>>>>
>>>>    obj-$(CONFIG_COMMON_CLK)     += clk-fixed-rate.o
>>>>    obj-$(CONFIG_COMMON_CLK)     += clk-gate.o
>>>>    obj-$(CONFIG_COMMON_CLK)     += clk-mux.o
>>>>
>>>> +obj-$(CONFIG_COMMON_CLK)     += clk-composite.o
>>>>
>>>>    # SoCs specific
>>>>    obj-$(CONFIG_ARCH_BCM2835)   += clk-bcm2835.o
>>>>
>>>> diff --git a/drivers/clk/clk-composite.c
>>>> b/drivers/clk/clk-composite.c
>>>> new file mode 100644
>>>> index 0000000..5a6587f
>>>> --- /dev/null
>>>> +++ b/drivers/clk/clk-composite.c
>>>> @@ -0,0 +1,208 @@
>>>> +/*
>>>> + * Copyright (c) 2013 NVIDIA CORPORATION.  All rights reserved.
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or
>>>> modify it + * under the terms and conditions of the GNU General
>>>> Public License, + * version 2, as published by the Free Software
>>>> Foundation. + *
>>>> + * This program is distributed in the hope it will be useful, but
>>>> WITHOUT + * ANY WARRANTY; without even the implied warranty of
>>>> MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>>> General Public License for + * more details.
>>>> + *
>>>> + * You should have received a copy of the GNU General Public License
>>>> + * along with this program.  If not, see
>>>> <http://www.gnu.org/licenses/>. + */
>>>> +
>>>> +#include <linux/clk.h>
>>>> +#include <linux/clk-provider.h>
>>>> +#include <linux/err.h>
>>>> +#include <linux/slab.h>
>>>> +
>>>> +#define to_clk_composite(_hw) container_of(_hw, struct
>>>> clk_composite,
>>>> hw) +
>>>> +static u8 clk_composite_get_parent(struct clk_hw *hw)
>>>> +{
>>>> +     struct clk_composite *composite = to_clk_composite(hw);
>>>> +     const struct clk_ops *mux_ops = composite->mux_ops;
>>>> +     struct clk_hw *mux_hw = composite->mux_hw;
>>>> +
>>>> +     mux_hw->clk = hw->clk;
>>> Why is this needed? Looks like this filed is already being initialized
>>> in clk_register_composite.
>> Some ops will get called during clk_init where this clk is not populated
>> hence doing here. I have done it for all ops to make sure that any
>> future change in clock framework don't break this clock.
>> Now, why duplicate it in clk_register_composite? It is possible that
>> none of these ops get called in clk_init.
>> For example, recalc_rate is called during init and this ops is supported
>> by div clock type, but what if div clock is not added.
>>
>> I hope this explains the need.
>>
> Sorry, I don't understand your explanation.
>
> I have asked why mux_hw->clk field has to be reinitialized in all the ops.
>
> In other words, is it even possible that this clk pointer changes since
> calling clk_register from clk_register_composite and if yes, why?

Tomasz,

calling sequence is as

clk_register(struct clk_hw *hw)
     clk_init(struct clk_hw *hw)
         .
         .
         hw->clk = clk;
         clk->ops.recalc_rate(hw) => clk_composite_recalc_rate(hw) => 
composite->div_ops.recalc_rate(div_hw) => clk_divider_recalc_rate(hw)

Now if clk_divider_recalc_rate tries to access clk from hw then it will 
get NULL since this is not assigned yet and that is what I am doing in 
clk_composite_recalc_rate.

I am doing it in all ops because I can not assume which one will get 
called first and always. If in future something changes the calling 
sequence in ccf framework then it will break this clock.

> Best regards,
> --
> Tomasz Figa
> Samsung Poland R&D Center
> SW Solution Development, Linux Platform
>
>>>> +
>>>> +     return mux_ops->get_parent(mux_hw);
>>>> +}
>>>> +
>>>> +static int clk_composite_set_parent(struct clk_hw *hw, u8 index)
>>>> +{
>>>> +     struct clk_composite *composite = to_clk_composite(hw);
>>>> +     const struct clk_ops *mux_ops = composite->mux_ops;
>>>> +     struct clk_hw *mux_hw = composite->mux_hw;
>>>> +
>>>> +     mux_hw->clk = hw->clk;
>>> Ditto.
>>>
>>>> +
>>>> +     return mux_ops->set_parent(mux_hw, index);
>>>> +}
>>>> +
>>>> +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;
>>>> +
>>>> +     div_hw->clk = hw->clk;
>>> Ditto.
>>>
>>>> +
>>>> +     return div_ops->recalc_rate(div_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;
>>>> +
>>>> +     div_hw->clk = hw->clk;
>>> Ditto.
>>>
>>>> +
>>>> +     return div_ops->round_rate(div_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;
>>>> +
>>>> +     div_hw->clk = hw->clk;
>>> Ditto.
>>>
>>>> +
>>>> +     return div_ops->set_rate(div_hw, rate, parent_rate);
>>>> +}
>>>> +
>>>> +static int clk_composite_is_enabled(struct clk_hw *hw)
>>>> +{
>>>> +     struct clk_composite *composite = to_clk_composite(hw);
>>>> +     const struct clk_ops *gate_ops = composite->gate_ops;
>>>> +     struct clk_hw *gate_hw = composite->gate_hw;
>>>> +
>>>> +     gate_hw->clk = hw->clk;
>>> Ditto.
>>>
>>>> +
>>>> +     return gate_ops->is_enabled(gate_hw);
>>>> +}
>>>> +
>>>> +static int clk_composite_enable(struct clk_hw *hw)
>>>> +{
>>>> +     struct clk_composite *composite = to_clk_composite(hw);
>>>> +     const struct clk_ops *gate_ops = composite->gate_ops;
>>>> +     struct clk_hw *gate_hw = composite->gate_hw;
>>>> +
>>>> +     gate_hw->clk = hw->clk;
>>> Ditto.
>>>
>>>> +
>>>> +     return gate_ops->enable(gate_hw);
>>>> +}
>>>> +
>>>> +static void clk_composite_disable(struct clk_hw *hw)
>>>> +{
>>>> +     struct clk_composite *composite = to_clk_composite(hw);
>>>> +     const struct clk_ops *gate_ops = composite->gate_ops;
>>>> +     struct clk_hw *gate_hw = composite->gate_hw;
>>>> +
>>>> +     gate_hw->clk = hw->clk;
>>> Ditto.
>>>
>>>> +
>>>> +     gate_ops->disable(gate_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 *gate_hw, const struct clk_ops *gate_ops, +
>>>>    unsigned long flags)
>>>> +{
>>>> +     struct clk *clk;
>>>> +     struct clk_init_data init;
>>>> +     struct clk_composite *composite;
>>>> +     struct clk_ops *clk_composite_ops;
>>>> +
>>>> +     composite = kzalloc(sizeof(*composite), GFP_KERNEL);
>>>> +     if (!composite) {
>>>> +             pr_err("%s: could not allocate composite clk\n",
>>>> __func__); +             return ERR_PTR(-ENOMEM);
>>>> +     }
>>>> +
>>>> +     init.name = name;
>>>> +     init.flags = flags | CLK_IS_BASIC;
>>>> +     init.parent_names = parent_names;
>>>> +     init.num_parents = num_parents;
>>>> +
>>>> +     /* allocate the clock ops */
>>>> +     clk_composite_ops = kzalloc(sizeof(*clk_composite_ops),
>>>> GFP_KERNEL); +     if (!clk_composite_ops) {
>>>> +             pr_err("%s: could not allocate clk ops\n", __func__);
>>>> +             kfree(composite);
>>>> +             return ERR_PTR(-ENOMEM);
>>>> +     }
>>> Maybe it would be better to embed this struct clk_ops inside struct
>>> clk_composite? This would allow to get rid of this allocation.
>>>
>>>> +
>>>> +     if (mux_hw && mux_ops) {
>>>> +             if (!mux_ops->get_parent || !mux_ops->set_parent) {
>>>> +                     clk = ERR_PTR(-EINVAL);
>>>> +                     goto err;
>>>> +             }
>>>> +
>>>> +             composite->mux_hw = mux_hw;
>>>> +             composite->mux_ops = mux_ops;
>>>> +             clk_composite_ops->get_parent =
>>>> clk_composite_get_parent;
>>>> +             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) {
>>>> +                     clk = ERR_PTR(-EINVAL);
>>>> +                     goto err;
>>>> +             }
>>>> +
>>>> +             composite->div_hw = div_hw;
>>>> +             composite->div_ops = div_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) {
>>>> +             if (!gate_ops->is_enabled || !gate_ops->enable ||
>>>> +                 !gate_ops->disable) {
>>>> +                     clk = ERR_PTR(-EINVAL);
>>>> +                     goto err;
>>>> +             }
>>>> +
>>>> +             composite->gate_hw = gate_hw;
>>>> +             composite->gate_ops = gate_ops;
>>>> +             clk_composite_ops->is_enabled =
>>>> clk_composite_is_enabled;
>>>> +             clk_composite_ops->enable = clk_composite_enable;
>>>> +             clk_composite_ops->disable = clk_composite_disable;
>>>> +     }
>>>> +
>>>> +     init.ops = clk_composite_ops;
>>>> +     composite->hw.init = &init;
>>>> +
>>>> +     clk = clk_register(dev, &composite->hw);
>>>> +     if (IS_ERR(clk))
>>>> +             goto err;
>>>> +
>>>> +     if (composite->mux_hw)
>>>> +             composite->mux_hw->clk = clk;
>>>> +
>>>> +     if (composite->div_hw)
>>>> +             composite->div_hw->clk = clk;
>>>> +
>>>> +     if (composite->gate_hw)
>>>> +             composite->gate_hw->clk = clk;
>>>> +
>>>> +     return clk;
>>>> +
>>>> +err:
>>>> +     kfree(clk_composite_ops);
>>>> +     kfree(composite);
>>>> +     return clk;
>>>> +}
>>>> diff --git a/include/linux/clk-provider.h
>>>> b/include/linux/clk-provider.h index 7f197d7..f1a36aa 100644
>>>> --- a/include/linux/clk-provider.h
>>>> +++ b/include/linux/clk-provider.h
>>>> @@ -325,6 +325,36 @@ struct clk *clk_register_fixed_factor(struct
>>>> device *dev, const char *name, const char *parent_name, unsigned
>>>> long flags,>>
>>>>                 unsigned int mult, unsigned int div);
>>>>
>>>> +/***
>>>> + * 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_ops:   clock ops
>>>> for mux
>>>> + * @div_ops: clock ops for divider
>>>> + * @gate_ops:        clock ops for gate
>>>> + */
>>>> +struct clk_composite {
>>>> +     struct clk_hw   hw;
>>> As I suggested above:
>>>           struct clk_ops  ops;
>>>> +
>>>> +     struct clk_hw   *mux_hw;
>>>> +     struct clk_hw   *div_hw;
>>>> +     struct clk_hw   *gate_hw;
>>>> +
>>>> +     const struct clk_ops    *mux_ops;
>>>> +     const struct clk_ops    *div_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 *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
>>> Best regards,
>>> --
>>> Tomasz Figa
>>> Samsung Poland R&D Center
>>> SW Solution Development, Linux Platform
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2] clk: Add composite clock type

[Stephen Warren]

On 02/28/2013 12:58 AM, Prashant Gaikwad wrote:
> On Wednesday 06 February 2013 03:36 PM, Tomasz Figa wrote:
>> On Wednesday 06 of February 2013 08:34:32 Prashant Gaikwad wrote:
>>> On Tuesday 05 February 2013 03:45 PM, Tomasz Figa wrote:
>>>> Hi Prashant,
>>>>
>>>> Thank you for your patch. Please see some comments inline.
>>>>
>>>> On Monday 04 of February 2013 13:41:22 Prashant Gaikwad wrote:
>>>>> Not all clocks are required to be decomposed into basic clock
>>>>> types but at the same time want to use the functionality
>>>>> provided by these basic clock types instead of duplicating.
>>>>>
>>>>> For example, Tegra SoC has ~100 clocks which can be decomposed
>>>>> into Mux -> Div -> Gate clock types making the clock count to
>>>>> ~300. Also, parent change operation can not be performed on gate
>>>>> clock which forces to use mux clock in driver if want to change
>>>>> the parent.
>>>>>
>>>>> Instead aggregate the basic clock types functionality into one
>>>>> clock and just use this clock for all operations. This clock
>>>>> type re-uses the functionality of basic clock types and not
>>>>> limited to basic clock types but any hardware-specific
>>>>> implementation.

>>>>> diff --git a/drivers/clk/clk-composite.c

>>>>> +static u8 clk_composite_get_parent(struct clk_hw *hw)
>>>>> +{
>>>>> +     struct clk_composite *composite = to_clk_composite(hw);
>>>>> +     const struct clk_ops *mux_ops = composite->mux_ops;
>>>>> +     struct clk_hw *mux_hw = composite->mux_hw;
>>>>> +
>>>>> +     mux_hw->clk = hw->clk;
>>>>
>>>> Why is this needed? Looks like this filed is already being initialized
>>>> in clk_register_composite.
>>>
>>> Some ops will get called during clk_init where this clk is not populated
>>> hence doing here. I have done it for all ops to make sure that any
>>> future change in clock framework don't break this clock.
>>> Now, why duplicate it in clk_register_composite? It is possible that
>>> none of these ops get called in clk_init.
>>> For example, recalc_rate is called during init and this ops is supported
>>> by div clock type, but what if div clock is not added.
>>>
>>> I hope this explains the need.
>>
>> Sorry, I don't understand your explanation.
>>
>> I have asked why mux_hw->clk field has to be reinitialized in all the
>> ops.
>>
>> In other words, is it even possible that this clk pointer changes since
>> calling clk_register from clk_register_composite and if yes, why?
> 
> Tomasz,
> 
> calling sequence is as
> 
> clk_register(struct clk_hw *hw)
>     clk_init(struct clk_hw *hw)
>         .
>         .
>         hw->clk = clk;
>         clk->ops.recalc_rate(hw) => clk_composite_recalc_rate(hw) =>
> composite->div_ops.recalc_rate(div_hw) => clk_divider_recalc_rate(hw)
> 
> Now if clk_divider_recalc_rate tries to access clk from hw then it will
> get NULL since this is not assigned yet and that is what I am doing in
> clk_composite_recalc_rate.
> 
> I am doing it in all ops because I can not assume which one will get
> called first and always. If in future something changes the calling
> sequence in ccf framework then it will break this clock.

Surely the CCF core should be taking care of this as part of
clk_register() or clk_init()?

Re: [PATCH V2] clk: Add composite clock type

[Tomasz Figa]

Hi Prashant,

On Thursday 28 of February 2013 11:20:31 Stephen Warren wrote:
> On 02/28/2013 12:58 AM, Prashant Gaikwad wrote:
> > On Wednesday 06 February 2013 03:36 PM, Tomasz Figa wrote:
> >> On Wednesday 06 of February 2013 08:34:32 Prashant Gaikwad wrote:
> >>> On Tuesday 05 February 2013 03:45 PM, Tomasz Figa wrote:
> >>>> Hi Prashant,
> >>>> 
> >>>> Thank you for your patch. Please see some comments inline.
> >>>> 
> >>>> On Monday 04 of February 2013 13:41:22 Prashant Gaikwad wrote:
> >>>>> Not all clocks are required to be decomposed into basic clock
> >>>>> types but at the same time want to use the functionality
> >>>>> provided by these basic clock types instead of duplicating.
> >>>>> 
> >>>>> For example, Tegra SoC has ~100 clocks which can be decomposed
> >>>>> into Mux -> Div -> Gate clock types making the clock count to
> >>>>> ~300. Also, parent change operation can not be performed on gate
> >>>>> clock which forces to use mux clock in driver if want to change
> >>>>> the parent.
> >>>>> 
> >>>>> Instead aggregate the basic clock types functionality into one
> >>>>> clock and just use this clock for all operations. This clock
> >>>>> type re-uses the functionality of basic clock types and not
> >>>>> limited to basic clock types but any hardware-specific
> >>>>> implementation.
> >>>>> 
> >>>>> diff --git a/drivers/clk/clk-composite.c
> >>>>> 
> >>>>> +static u8 clk_composite_get_parent(struct clk_hw *hw)
> >>>>> +{
> >>>>> +     struct clk_composite *composite = to_clk_composite(hw);
> >>>>> +     const struct clk_ops *mux_ops = composite->mux_ops;
> >>>>> +     struct clk_hw *mux_hw = composite->mux_hw;
> >>>>> +
> >>>>> +     mux_hw->clk = hw->clk;
> >>>> 
> >>>> Why is this needed? Looks like this filed is already being initialized
> >>>> in clk_register_composite.
> >>> 
> >>> Some ops will get called during clk_init where this clk is not populated
> >>> hence doing here. I have done it for all ops to make sure that any
> >>> future change in clock framework don't break this clock.
> >>> Now, why duplicate it in clk_register_composite? It is possible that
> >>> none of these ops get called in clk_init.
> >>> For example, recalc_rate is called during init and this ops is supported
> >>> by div clock type, but what if div clock is not added.
> >>> 
> >>> I hope this explains the need.
> >> 
> >> Sorry, I don't understand your explanation.
> >> 
> >> I have asked why mux_hw->clk field has to be reinitialized in all the
> >> ops.
> >> 
> >> In other words, is it even possible that this clk pointer changes since
> >> calling clk_register from clk_register_composite and if yes, why?
> > 
> > Tomasz,
> > 
> > calling sequence is as
> > 
> > clk_register(struct clk_hw *hw)
> > 
> >     clk_init(struct clk_hw *hw)
> >     
> >         .
> >         .
> >         hw->clk = clk;
> >         clk->ops.recalc_rate(hw) => clk_composite_recalc_rate(hw) =>
> > 
> > composite->div_ops.recalc_rate(div_hw) => clk_divider_recalc_rate(hw)
> > 
> > Now if clk_divider_recalc_rate tries to access clk from hw then it will
> > get NULL since this is not assigned yet and that is what I am doing in
> > clk_composite_recalc_rate.
> > 
> > I am doing it in all ops because I can not assume which one will get
> > called first and always. If in future something changes the calling
> > sequence in ccf framework then it will break this clock.
> 
> Surely the CCF core should be taking care of this as part of
> clk_register() or clk_init()?

Any news on this? It would be nice if this patch could be merged soon, because 
we'd like to rework Exynos clock code to use composite clocks before merge 
window, to have that merged for 3.10.

If you don't have time to work on this, would you mind if I made any necessary 
fixes, added my sign-off next to yours and posted next version myself?

Best regards,
-- 
Tomasz Figa
Samsung Poland R&D Center
SW Solution Development, Kernel and System Framework

Re: [PATCH V2] clk: Add composite clock type

[Prashant Gaikwad]

On Wednesday 13 March 2013 10:00 PM, Tomasz Figa wrote:
> Hi Prashant,
>
> On Thursday 28 of February 2013 11:20:31 Stephen Warren wrote:
>> On 02/28/2013 12:58 AM, Prashant Gaikwad wrote:
>>> On Wednesday 06 February 2013 03:36 PM, Tomasz Figa wrote:
>>>> On Wednesday 06 of February 2013 08:34:32 Prashant Gaikwad wrote:
>>>>> On Tuesday 05 February 2013 03:45 PM, Tomasz Figa wrote:
>>>>>> Hi Prashant,
>>>>>>
>>>>>> Thank you for your patch. Please see some comments inline.
>>>>>>
>>>>>> On Monday 04 of February 2013 13:41:22 Prashant Gaikwad wrote:
>>>>>>> Not all clocks are required to be decomposed into basic clock
>>>>>>> types but at the same time want to use the functionality
>>>>>>> provided by these basic clock types instead of duplicating.
>>>>>>>
>>>>>>> For example, Tegra SoC has ~100 clocks which can be decomposed
>>>>>>> into Mux -> Div -> Gate clock types making the clock count to
>>>>>>> ~300. Also, parent change operation can not be performed on gate
>>>>>>> clock which forces to use mux clock in driver if want to change
>>>>>>> the parent.
>>>>>>>
>>>>>>> Instead aggregate the basic clock types functionality into one
>>>>>>> clock and just use this clock for all operations. This clock
>>>>>>> type re-uses the functionality of basic clock types and not
>>>>>>> limited to basic clock types but any hardware-specific
>>>>>>> implementation.
>>>>>>>
>>>>>>> diff --git a/drivers/clk/clk-composite.c
>>>>>>>
>>>>>>> +static u8 clk_composite_get_parent(struct clk_hw *hw)
>>>>>>> +{
>>>>>>> +     struct clk_composite *composite = to_clk_composite(hw);
>>>>>>> +     const struct clk_ops *mux_ops = composite->mux_ops;
>>>>>>> +     struct clk_hw *mux_hw = composite->mux_hw;
>>>>>>> +
>>>>>>> +     mux_hw->clk = hw->clk;
>>>>>> Why is this needed? Looks like this filed is already being initialized
>>>>>> in clk_register_composite.
>>>>> Some ops will get called during clk_init where this clk is not populated
>>>>> hence doing here. I have done it for all ops to make sure that any
>>>>> future change in clock framework don't break this clock.
>>>>> Now, why duplicate it in clk_register_composite? It is possible that
>>>>> none of these ops get called in clk_init.
>>>>> For example, recalc_rate is called during init and this ops is supported
>>>>> by div clock type, but what if div clock is not added.
>>>>>
>>>>> I hope this explains the need.
>>>> Sorry, I don't understand your explanation.
>>>>
>>>> I have asked why mux_hw->clk field has to be reinitialized in all the
>>>> ops.
>>>>
>>>> In other words, is it even possible that this clk pointer changes since
>>>> calling clk_register from clk_register_composite and if yes, why?
>>> Tomasz,
>>>
>>> calling sequence is as
>>>
>>> clk_register(struct clk_hw *hw)
>>>
>>>      clk_init(struct clk_hw *hw)
>>>      
>>>          .
>>>          .
>>>          hw->clk = clk;
>>>          clk->ops.recalc_rate(hw) => clk_composite_recalc_rate(hw) =>
>>>
>>> composite->div_ops.recalc_rate(div_hw) => clk_divider_recalc_rate(hw)
>>>
>>> Now if clk_divider_recalc_rate tries to access clk from hw then it will
>>> get NULL since this is not assigned yet and that is what I am doing in
>>> clk_composite_recalc_rate.
>>>
>>> I am doing it in all ops because I can not assume which one will get
>>> called first and always. If in future something changes the calling
>>> sequence in ccf framework then it will break this clock.
>> Surely the CCF core should be taking care of this as part of
>> clk_register() or clk_init()?
> Any news on this? It would be nice if this patch could be merged soon, because
> we'd like to rework Exynos clock code to use composite clocks before merge
> window, to have that merged for 3.10.
>
> If you don't have time to work on this, would you mind if I made any necessary
> fixes, added my sign-off next to yours and posted next version myself?

Tomasz,

Sorry for delayed reply. I will send the updated patch in next 2-3 days. 
After that if any rework required you can make the changes and add your 
sign-off.

Thanks for the patience!!

Regards,
PrashantG

> Best regards,