From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754096Ab3BEKQS (ORCPT ); Tue, 5 Feb 2013 05:16:18 -0500 Received: from mailout3.samsung.com ([203.254.224.33]:49707 "EHLO mailout3.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751411Ab3BEKQD (ORCPT ); Tue, 5 Feb 2013 05:16:03 -0500 X-AuditID: cbfee61a-b7f7d6d000000f4e-17-5110dbe18437 From: Tomasz Figa To: linux-arm-kernel@lists.infradead.org Cc: Prashant Gaikwad , mturquette@linaro.org, sboyd@codeaurora.org, swarren@wwwdotorg.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH V2] clk: Add composite clock type Date: Tue, 05 Feb 2013 11:15:57 +0100 Message-id: <2204463.8bhhYZbxqK@amdc1227> Organization: Samsung Poland R&D Center User-Agent: KMail/4.9.5 (Linux/3.7.4-gentoo; KDE/4.9.5; x86_64; ; ) In-reply-to: <1359965482-29655-1-git-send-email-pgaikwad@nvidia.com> References: <1359965482-29655-1-git-send-email-pgaikwad@nvidia.com> MIME-version: 1.0 Content-transfer-encoding: 7Bit Content-type: text/plain; charset=us-ascii X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrOLMWRmVeSWpSXmKPExsVy+t9jQd2HtwUCDT5OFrG4vGsOmwOjx+dN cgGMUVw2Kak5mWWpRfp2CVwZzV92sRVcDa5omv2NuYFxqnMXIyeHhICJROOB80wQtpjEhXvr 2boYuTiEBKYzSty5/pkFwlnKJLF1wW1WkCo2ATWJzw2P2EBsEQENiSldj9lBipgF+hglnq3a xA6SEBYwlVh+bzsziM0ioCox5fUbFhCbV0BTYuK7SWBxfgF1iXfbnoKtFhVwlljY+xpsKKeA i8Shxw1g9UJA8RdfFzJD9ApK/Jh8DyzOLCAvsW//VFYIW0ti/c7jTBMYBWchKZuFpGwWkrIF jMyrGEVTC5ILipPScw31ihNzi0vz0vWS83M3MYKD85nUDsaVDRaHGAU4GJV4eH9c5g8UYk0s K67MPcQowcGsJML78QZQiDclsbIqtSg/vqg0J7X4EKM0B4uSOC/jqScBQgLpiSWp2ampBalF MFkmDk6pBsbm8wsuBOXM3LXdyOOy7Yl3ok9izs/WyF7bti39lxhX45E7yhob5vcbVBWHT/+8 xDalKPs7Q5Dz5ntrdomtjzFSqvB9bJKtyfVta1V/rPGJ1tUt5T4Ft1xju7fvTZ4hbHjH5jin /ImHAu+nHfyeZOQ3a5LYxmny0iKJ2xJXPlQsaf7+r0TmbagSS3FGoqEWc1FxIgDO2H0eSgIA AA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > > 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 > . + */ > + > +#include > +#include > +#include > +#include > + > +#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