Message-ID: <CAJOA=zMsCuZgmp1Hm0dq=EVoX6Zj62XQVqR=6GzDntSD5HnHzg@mail.gmail.com>
Date:	Wed, 14 Mar 2012 13:47:56 -0700
From:	"Turquette, Mike" <mturquette@...com>
To:	Sascha Hauer <s.hauer@...gutronix.de>
Cc:	Russell King <linux@....linux.org.uk>,
	Andrew Lunn <andrew@...n.ch>, linaro-dev@...ts.linaro.org,
	Grant Likely <grant.likely@...retlab.ca>,
	Saravana Kannan <skannan@...eaurora.org>,
	Jeremy Kerr <jeremy.kerr@...onical.com>,
	Magnus Damm <magnus.damm@...il.com>,
	Deepak Saxena <dsaxena@...aro.org>,
	linux-arm-kernel@...ts.infradead.org,
	Arnd Bergman <arnd.bergmann@...aro.org>, patches@...aro.org,
	Rob Herring <rob.herring@...xeda.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Richard Zhao <richard.zhao@...aro.org>,
	Shawn Guo <shawn.guo@...escale.com>,
	Paul Walmsley <paul@...an.com>,
	Linus Walleij <linus.walleij@...ricsson.com>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	Stephen Boyd <sboyd@...eaurora.org>,
	linux-kernel@...r.kernel.org,
	Amit Kucheria <amit.kucheria@...aro.org>
Subject: Re: [PATCH v5 3/4] clk: introduce the common clock framework

On Wed, Mar 14, 2012 at 1:48 AM, Sascha Hauer <s.hauer@...gutronix.de> wrote:
> On Tue, Mar 13, 2012 at 04:43:57PM -0700, Turquette, Mike wrote:
>> On Tue, Mar 13, 2012 at 4:24 AM, Sascha Hauer <s.hauer@...gutronix.de> wrote:
>> > On Sat, Mar 03, 2012 at 12:29:00AM -0800, Mike Turquette wrote:
>> >> The common clock framework defines a common struct clk useful across
>> >> most platforms as well as an implementation of the clk api that drivers
>> >> can use safely for managing clocks.
>> >>
>> >> The net result is consolidation of many different struct clk definitions
>> >> and platform-specific clock framework implementations.
>> >>
>> >> This patch introduces the common struct clk, struct clk_ops and an
>> >> implementation of the well-known clock api in include/clk/clk.h.
>> >> Platforms may define their own hardware-specific clock structure and
>> >> their own clock operation callbacks, so long as it wraps an instance of
>> >> struct clk_hw.
>> >>
>> >> See Documentation/clk.txt for more details.
>> >>
>> >> This patch is based on the work of Jeremy Kerr, which in turn was based
>> >> on the work of Ben Herrenschmidt.
>> >>
>> >> Signed-off-by: Mike Turquette <mturquette@...aro.org>
>> >> Signed-off-by: Mike Turquette <mturquette@...com>
>> >> Cc: Jeremy Kerr <jeremy.kerr@...onical.com>
>> >> Cc: Thomas Gleixner <tglx@...utronix.de>
>> >> Cc: Arnd Bergman <arnd.bergmann@...aro.org>
>> >> Cc: Paul Walmsley <paul@...an.com>
>> >> Cc: Shawn Guo <shawn.guo@...escale.com>
>> >> Cc: Richard Zhao <richard.zhao@...aro.org>
>> >> Cc: Saravana Kannan <skannan@...eaurora.org>
>> >> Cc: Magnus Damm <magnus.damm@...il.com>
>> >> Cc: Rob Herring <rob.herring@...xeda.com>
>> >> Cc: Mark Brown <broonie@...nsource.wolfsonmicro.com>
>> >> Cc: Linus Walleij <linus.walleij@...ricsson.com>
>> >> Cc: Stephen Boyd <sboyd@...eaurora.org>
>> >> Cc: Amit Kucheria <amit.kucheria@...aro.org>
>> >> Cc: Deepak Saxena <dsaxena@...aro.org>
>> >> Cc: Grant Likely <grant.likely@...retlab.ca>
>> >> Cc: Andrew Lunn <andrew@...n.ch>
>> >> ---
>> >>  drivers/clk/Kconfig          |   28 +
>> >>  drivers/clk/Makefile         |    1 +
>> >>  drivers/clk/clk.c            | 1323 ++++++++++++++++++++++++++++++++++++++++++
>> >>  include/linux/clk-private.h  |   68 +++
>> >>  include/linux/clk-provider.h |  169 ++++++
>> >>  include/linux/clk.h          |   68 ++-
>> >>  6 files changed, 1652 insertions(+), 5 deletions(-)
>> >>  create mode 100644 drivers/clk/clk.c
>> >>  create mode 100644 include/linux/clk-private.h
>> >>  create mode 100644 include/linux/clk-provider.h
>> >>
>> >> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
>> >> index 3912576..18eb8c2 100644
>> >> --- a/drivers/clk/Kconfig
>> >> +++ b/drivers/clk/Kconfig
>> >> @@ -11,3 +11,31 @@ config HAVE_MACH_CLKDEV
>> >>
>> >>  config HAVE_CLK_PREPARE
>> >>       bool
>> >> +
>> >> +menuconfig COMMON_CLK
>> >> +     bool "Common Clock Framework"
>> >> +     select HAVE_CLK_PREPARE
>> >> +     ---help---
>> >> +       The common clock framework is a single definition of struct
>> >> +       clk, useful across many platforms, as well as an
>> >> +       implementation of the clock API in include/linux/clk.h.
>> >> +       Architectures utilizing the common struct clk should select
>> >> +       this automatically, but it may be necessary to manually select
>> >> +       this option for loadable modules requiring the common clock
>> >> +       framework.
>> >> +
>> >> +       If in doubt, say "N".
>> >> +
>> >> +if COMMON_CLK
>> >> +
>> >> +config COMMON_CLK_DEBUG
>> >> +     bool "DebugFS representation of clock tree"
>> >> +     depends on COMMON_CLK
>> >> +     ---help---
>> >> +       Creates a directory hierchy in debugfs for visualizing the clk
>> >> +       tree structure.  Each directory contains read-only members
>> >> +       that export information specific to that clk node: clk_rate,
>> >> +       clk_flags, clk_prepare_count, clk_enable_count &
>> >> +       clk_notifier_count.
>> >> +
>> >> +endif
>> >> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
>> >> index 07613fa..ff362c4 100644
>> >> --- a/drivers/clk/Makefile
>> >> +++ b/drivers/clk/Makefile
>> >> @@ -1,2 +1,3 @@
>> >>
>> >>  obj-$(CONFIG_CLKDEV_LOOKUP)  += clkdev.o
>> >> +obj-$(CONFIG_COMMON_CLK)     += clk.o
>> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> >> new file mode 100644
>> >> index 0000000..b979d74
>> >> --- /dev/null
>> >> +++ b/drivers/clk/clk.c
>> >> @@ -0,0 +1,1323 @@
>> >> +/*
>> >> + * Copyright (C) 2010-2011 Canonical Ltd <jeremy.kerr@...onical.com>
>> >> + * Copyright (C) 2011-2012 Linaro Ltd <mturquette@...aro.org>
>> >> + *
>> >> + * This program is free software; you can redistribute it and/or modify
>> >> + * it under the terms of the GNU General Public License version 2 as
>> >> + * published by the Free Software Foundation.
>> >> + *
>> >> + * Standard functionality for the common clock API.  See Documentation/clk.txt
>> >> + */
>> >> +
>> >> +#include <linux/clk-private.h>
>> >> +#include <linux/module.h>
>> >> +#include <linux/mutex.h>
>> >> +#include <linux/spinlock.h>
>> >> +#include <linux/err.h>
>> >> +#include <linux/list.h>
>> >> +#include <linux/slab.h>
>> >> +
>> >> +static DEFINE_SPINLOCK(enable_lock);
>> >> +static DEFINE_MUTEX(prepare_lock);
>> >> +
>> >> +static HLIST_HEAD(clk_root_list);
>> >> +static HLIST_HEAD(clk_orphan_list);
>> >> +static LIST_HEAD(clk_notifier_list);
>> >> +
>> >> +/***        debugfs support        ***/
>> >> +
>> >> +#ifdef CONFIG_COMMON_CLK_DEBUG
>> >> +#include <linux/debugfs.h>
>> >> +
>> >> +static struct dentry *rootdir;
>> >> +static struct dentry *orphandir;
>> >> +static int inited = 0;
>> >> +
>> >> +/* caller must hold prepare_lock */
>> >> +static int clk_debug_create_one(struct clk *clk, struct dentry *pdentry)
>> >> +{
>> >> +     struct dentry *d;
>> >> +     int ret = -ENOMEM;
>> >> +
>> >> +     if (!clk || !pdentry) {
>> >> +             ret = -EINVAL;
>> >> +             goto out;
>> >> +     }
>> >> +
>> >> +     d = debugfs_create_dir(clk->name, pdentry);
>> >> +     if (!d)
>> >> +             goto out;
>> >> +
>> >> +     clk->dentry = d;
>> >> +
>> >> +     d = debugfs_create_u32("clk_rate", S_IRUGO, clk->dentry,
>> >> +                     (u32 *)&clk->rate);
>> >> +     if (!d)
>> >> +             goto err_out;
>> >> +
>> >> +     d = debugfs_create_x32("clk_flags", S_IRUGO, clk->dentry,
>> >> +                     (u32 *)&clk->flags);
>> >> +     if (!d)
>> >> +             goto err_out;
>> >> +
>> >> +     d = debugfs_create_u32("clk_prepare_count", S_IRUGO, clk->dentry,
>> >> +                     (u32 *)&clk->prepare_count);
>> >> +     if (!d)
>> >> +             goto err_out;
>> >> +
>> >> +     d = debugfs_create_u32("clk_enable_count", S_IRUGO, clk->dentry,
>> >> +                     (u32 *)&clk->enable_count);
>> >> +     if (!d)
>> >> +             goto err_out;
>> >> +
>> >> +     d = debugfs_create_u32("clk_notifier_count", S_IRUGO, clk->dentry,
>> >> +                     (u32 *)&clk->notifier_count);
>> >> +     if (!d)
>> >> +             goto err_out;
>> >> +
>> >> +     ret = 0;
>> >> +     goto out;
>> >> +
>> >> +err_out:
>> >> +     debugfs_remove(clk->dentry);
>> >> +out:
>> >> +     return ret;
>> >> +}
>> >> +
>> >> +/* caller must hold prepare_lock */
>> >> +static int clk_debug_create_subtree(struct clk *clk, struct dentry *pdentry)
>> >> +{
>> >> +     struct clk *child;
>> >> +     struct hlist_node *tmp;
>> >> +     int ret = -EINVAL;;
>> >> +
>> >> +     if (!clk || !pdentry)
>> >> +             goto out;
>> >> +
>> >> +     ret = clk_debug_create_one(clk, pdentry);
>> >> +
>> >> +     if (ret)
>> >> +             goto out;
>> >> +
>> >> +     hlist_for_each_entry(child, tmp, &clk->children, child_node)
>> >> +             clk_debug_create_subtree(child, clk->dentry);
>> >> +
>> >> +     ret = 0;
>> >> +out:
>> >> +     return ret;
>> >> +}
>> >> +
>> >> +/**
>> >> + * clk_debug_register - add a clk node to the debugfs clk tree
>> >> + * @clk: the clk being added to the debugfs clk tree
>> >> + *
>> >> + * Dynamically adds a clk to the debugfs clk tree if debugfs has been
>> >> + * initialized.  Otherwise it bails out early since the debugfs clk tree
>> >> + * will be created lazily by clk_debug_init as part of a late_initcall.
>> >> + *
>> >> + * Caller must hold prepare_lock.  Only clk_init calls this function (so
>> >> + * far) so this is taken care.
>> >> + */
>> >> +static int clk_debug_register(struct clk *clk)
>> >> +{
>> >> +     struct clk *parent;
>> >> +     struct dentry *pdentry;
>> >> +     int ret = 0;
>> >> +
>> >> +     if (!inited)
>> >> +             goto out;
>> >> +
>> >> +     parent = clk->parent;
>> >> +
>> >> +     /*
>> >> +      * Check to see if a clk is a root clk.  Also check that it is
>> >> +      * safe to add this clk to debugfs
>> >> +      */
>> >> +     if (!parent)
>> >> +             if (clk->flags & CLK_IS_ROOT)
>> >> +                     pdentry = rootdir;
>> >> +             else
>> >> +                     pdentry = orphandir;
>> >> +     else
>> >> +             if (parent->dentry)
>> >> +                     pdentry = parent->dentry;
>> >> +             else
>> >> +                     goto out;
>> >> +
>> >> +     ret = clk_debug_create_subtree(clk, pdentry);
>> >> +
>> >> +out:
>> >> +     return ret;
>> >> +}
>> >> +
>> >> +/**
>> >> + * clk_debug_init - lazily create the debugfs clk tree visualization
>> >> + *
>> >> + * clks are often initialized very early during boot before memory can
>> >> + * be dynamically allocated and well before debugfs is setup.
>> >> + * clk_debug_init walks the clk tree hierarchy while holding
>> >> + * prepare_lock and creates the topology as part of a late_initcall,
>> >> + * thus insuring that clks initialized very early will still be
>> >> + * represented in the debugfs clk tree.  This function should only be
>> >> + * called once at boot-time, and all other clks added dynamically will
>> >> + * be done so with clk_debug_register.
>> >> + */
>> >> +static int __init clk_debug_init(void)
>> >> +{
>> >> +     struct clk *clk;
>> >> +     struct hlist_node *tmp;
>> >> +
>> >> +     rootdir = debugfs_create_dir("clk", NULL);
>> >> +
>> >> +     if (!rootdir)
>> >> +             return -ENOMEM;
>> >> +
>> >> +     orphandir = debugfs_create_dir("orphans", rootdir);
>> >> +
>> >> +     if (!orphandir)
>> >> +             return -ENOMEM;
>> >> +
>> >> +     mutex_lock(&prepare_lock);
>> >> +
>> >> +     hlist_for_each_entry(clk, tmp, &clk_root_list, child_node)
>> >> +             clk_debug_create_subtree(clk, rootdir);
>> >> +
>> >> +     hlist_for_each_entry(clk, tmp, &clk_orphan_list, child_node)
>> >> +             clk_debug_create_subtree(clk, orphandir);
>> >> +
>> >> +     inited = 1;
>> >> +
>> >> +     mutex_unlock(&prepare_lock);
>> >> +
>> >> +     return 0;
>> >> +}
>> >> +late_initcall(clk_debug_init);
>> >> +#else
>> >> +static inline int clk_debug_register(struct clk *clk) { return 0; }
>> >> +#endif /* CONFIG_COMMON_CLK_DEBUG */
>> >> +
>> >> +/***    helper functions   ***/
>> >> +
>> >> +inline const char *__clk_get_name(struct clk *clk)
>> >> +{
>> >> +     return !clk ? NULL : clk->name;
>> >> +}
>> >> +
>> >> +inline struct clk_hw *__clk_get_hw(struct clk *clk)
>> >> +{
>> >> +     return !clk ? NULL : clk->hw;
>> >> +}
>> >> +
>> >> +inline u8 __clk_get_num_parents(struct clk *clk)
>> >> +{
>> >> +     return !clk ? -EINVAL : clk->num_parents;
>> >> +}
>> >> +
>> >> +inline struct clk *__clk_get_parent(struct clk *clk)
>> >> +{
>> >> +     return !clk ? NULL : clk->parent;
>> >> +}
>> >> +
>> >> +inline unsigned long __clk_get_rate(struct clk *clk)
>> >> +{
>> >> +     return !clk ? -EINVAL : clk->rate;
>> >> +}
>> >> +
>> >> +inline unsigned long __clk_get_flags(struct clk *clk)
>> >> +{
>> >> +     return !clk ? -EINVAL : clk->flags;
>> >> +}
>> >> +
>> >> +static struct clk *__clk_lookup_subtree(const char *name, struct clk *clk)
>> >> +{
>> >> +     struct clk *child;
>> >> +     struct clk *ret;
>> >> +     struct hlist_node *tmp;
>> >> +
>> >> +     if (!strcmp(clk->name, name))
>> >> +             return clk;
>> >> +
>> >> +     hlist_for_each_entry(child, tmp, &clk->children, child_node) {
>> >> +             ret = __clk_lookup_subtree(name, child);
>> >> +             if (ret)
>> >> +                     return ret;
>> >> +     }
>> >> +
>> >> +     return NULL;
>> >> +}
>> >> +
>> >> +struct clk *__clk_lookup(const char *name)
>> >> +{
>> >> +     struct clk *root_clk;
>> >> +     struct clk *ret;
>> >> +     struct hlist_node *tmp;
>> >> +
>> >
>> > This should have a check for NULL pointers. NULL pointers can happen
>> > here if a mux has holes in it. In this case the parent name array would
>> > look like this:
>> >
>> > { "parentclk1", "parentclk2", NULL /* reserved */, "parentclk3" }
>> >
>> > Without checking for NULL pointers we dereference the NULL pointer here.
>>
>> I will add the check.  I had not considered having NULL as one of the
>> parent names.  It seems to me that the only time this would happen is
>> if we are wiring up discrete devices.  For instance:
>>
>> Device A has a fixed-rate clock named "dpll"
>> Device B has a mux clock named "leaf" with parent_names = { "32k",
>> "oscillator", "clkin" }
>>
>> On a given board, leaf's "pll" parent might map to the output of the
>> dpll clock.  I had considered that DT would step in here by creating
>> an intermediate clock with no functional clk_ops that would "connect"
>> these two clocks.  The benefit of this is that it is easier to match
>> up clock names when staring at some data sheets.  It is common for a
>> downstream device's data sheets to assign names to the external inputs
>> originating from some unknown device; unsurprisingly the data sheet
>> for the device supplying the external upstream clocks often has
>> different names for the output signals than the one in the downstream
>> device's data sheet.  Using the method described above it is easy for
>> a person with both data sheets to see how the devices are wired up
>> since all of the clock names are represented by nodes in the clock
>> tree.  In short, the final outcome would looking something like:
>>
>> dpll (device a's driver registered this clock)
>>   |
>> clkin (one of device b's possible parents)
>>   |    (it is a dummy clock from DT since it is board-level topology)
>>   |
>> leaf (device b's driver registered this clock)
>>
>> clkin would have a single parent named "dpll" in this case, and again,
>> it would have no meaningful clk_ops.
>>
>> A caveat to the above scenario is that parent_names shouldn't have
>> holes in it, but a null pointer check is reasonable anyways.
>>
>> What do you think?  Even if we allow for muxes to have holes in
>> parent_names we'll still have to solve these sort of device
>> integration issues and DT will likely play a roll here.  I'll roll in
>> the null pointer check regardless.
>>
>> Also, do you forsee needing hole in parent_names for any reason other
>> than described above?
>
> I need it only for the case where a some values in the mux are marked as
> reserved in the datasheet or we simply do not have the corresponding
> clock in our tree (yet). We could also say that NULL pointers are not
> allowed in parent arrays, but instead "orphan" or "dummy" should be
> used. Then __clk_init should check for NULL pointers to make this clear.

I've added a WARN in __clk_init if any entries in .parent_names are
NULL.  I think it best to populate it with "dummy", or maybe a
platform-specific name if it helps you during development.

Regards,
Mike

>
> Sascha
>
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists