Messages in this thread Patch in this message | | | Date | Thu, 28 Feb 2013 13:28:39 +0530 | From | Prashant Gaikwad <> | Subject | Re: [PATCH V2] clk: Add composite clock type |
| |
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
| |