Re: [PATCH V2] clk: Add composite clock type
From: Prashant Gaikwad
Date: Thu Feb 28 2013 - 02:59:02 EST
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@xxxxxxxxxx>
---
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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/