Re: [PATCH 1/3] clk: introduce omap_clk_associate

[Felipe Balbi <me@felipebalbi.com>]

On Sat, Oct 25, 2008 at 11:03:50PM +0300, Felipe Balbi wrote:
> Sorry,
> 
> forgot to add RFC since I didn't test these patches yet.
> 
> anyways, this is the new version of the omap_clk_associate()
> mechanism. The patch header will have to change, just saw it.
> 
> Paul, could you take a look at it, does it look better now ?
> 
> I dropped the third argument (function name) since we should
> be matching only against the consumer device, if the consumer
> device is correct, it doesn't matter which name we give to
> clk_get().

I tested this today and got some weird behavior on hsmmc.c. If I just
apply the patch, clk_enable() doesn't seem to work and we get a
"non-linefecth" exception.

when I put some printks on hsmmc.c, it seemed to help as there was no
BUG anymore, but instead clk_disable() was getting stuck.

Paul, do you have a clue about what could it be ?? I was wondering if
clk_get() was kinda depending on the delay created by running
list_for_each_entry() twice ?? I merged all of them into one only loop.

BTW, I changed one little thing in the patch:
INIT_LIST_HEAD() is done in clk_register().

> On Sat, Oct 25, 2008 at 10:55:46PM +0300, Felipe Balbi wrote:
> > From: Felipe Balbi <felipe.balbi@nokia.com>
> > 
> > Introduce a new mechanism to omap's clk implementation to
> > associate the device with its clock during platform_device
> > registration. Also gives the clock a function name (like
> > mmc_fck, uart_fck, ehci_fck, etc) so drivers won't have to
> > care about clk names any longer.
> > 
> > With this mechanism we can also be sure drivers won't be able
> > to clk_get the wrong clk (only true when we move all drivers
> > to this new mechanism and drop the old ones).
> > 
> > Clk's function names will be defined as they come necessary
> > but let's try to keep it as simple as possible.
> > 
> > Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
> > ---
> >  arch/arm/plat-omap/clock.c              |   47 ++++++++++++++++++++++++++++--
> >  arch/arm/plat-omap/include/mach/clock.h |    4 ++
> >  2 files changed, 47 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm/plat-omap/clock.c b/arch/arm/plat-omap/clock.c
> > index 5178701..083a3ff 100644
> > --- a/arch/arm/plat-omap/clock.c
> > +++ b/arch/arm/plat-omap/clock.c
> > @@ -44,6 +44,7 @@ static struct clk_functions *arch_clock;
> >  struct clk * clk_get(struct device *dev, const char *id)
> >  {
> >  	struct clk *p, *clk = ERR_PTR(-ENOENT);
> > +	struct device *d;
> >  	int idno;
> >  
> >  	if (dev == NULL || dev->bus != &platform_bus_type)
> > @@ -54,27 +55,65 @@ struct clk * clk_get(struct device *dev, const char *id)
> >  	mutex_lock(&clocks_mutex);
> >  
> >  	list_for_each_entry(p, &clocks, node) {
> > +		list_for_each_entry(d, &p->consumers, node) {
> > +			if (dev == d) {
> > +				clk = p;
> > +				break;
> > +			}
> > +		}
> > +
> >  		if (p->id == idno &&
> >  		    strcmp(id, p->name) == 0 && try_module_get(p->owner)) {
> >  			clk = p;
> > -			goto found;
> > +			break;
> >  		}
> > -	}
> >  
> > -	list_for_each_entry(p, &clocks, node) {
> >  		if (strcmp(id, p->name) == 0 && try_module_get(p->owner)) {
> >  			clk = p;
> >  			break;
> >  		}
> >  	}
> 
> here I merged both list_for_each_entry() in only one to make it a bit
> cheaper. Hope there's no problem with it.
> 
> >  
> > -found:
> >  	mutex_unlock(&clocks_mutex);
> >  
> >  	return clk;
> >  }
> >  EXPORT_SYMBOL(clk_get);
> >  
> > +/**
> > + * omap_clk_associate - associates a clock to its consumer device(s)
> > + * @id: clock id as defined in arch/arm/mach-omapX/clkxxxx.h
> > + * @dev: device pointer for the clock consumer
> > + *
> > + * This function attempts to simplify clock handling for device drivers
> > + * by letting struct clk know which device is the correct consumer for
> > + * that clock. By making this trick, we let drivers get the correct
> > + * clock without knowing about different clock names between omap
> > + * versions.
> > + */
> > +void __init omap_clk_associate(const char *id, struct device *dev)
> > +{
> > +	struct clk *clk = clk_get(NULL, id);
> > +
> > +	if (!clk || IS_ERR(clk)) {
> > +		pr_debug("%s: unable to get clock\n", __func__);
> > +		return;
> > +	}
> > +
> > +	if (!dev) {
> > +		pr_debug("%s: missing device to associate with\n", __func__);
> > +		clk_put(clk);
> > +		return;
> > +	}
> > +
> > +	if (list_empty(&clk->consumers))
> > +		INIT_LIST_HEAD(&clk->consumers);
> 
> this I need to test. I didn't know about other mechanism to see if
> clk->consumers wasn't initialized so I used list_empty(). I need to see
> if it won't break in runtime.
> 
> > +
> > +	/* add device to consumer list */
> > +	list_add(&dev->node, &clk->consumers);
> > +	clk_put(clk);
> > +};
> > +
> >  int clk_enable(struct clk *clk)
> >  {
> >  	unsigned long flags;
> > diff --git a/arch/arm/plat-omap/include/mach/clock.h b/arch/arm/plat-omap/include/mach/clock.h
> > index 9088925..1600a14 100644
> > --- a/arch/arm/plat-omap/include/mach/clock.h
> > +++ b/arch/arm/plat-omap/include/mach/clock.h
> > @@ -13,6 +13,8 @@
> >  #ifndef __ARCH_ARM_OMAP_CLOCK_H
> >  #define __ARCH_ARM_OMAP_CLOCK_H
> >  
> > +#include <linux/device.h>
> > +
> >  struct module;
> >  struct clk;
> >  struct clockdomain;
> > @@ -61,6 +63,7 @@ struct dpll_data {
> >  
> >  struct clk {
> >  	struct list_head	node;
> > +	struct list_head	consumers;
> >  	struct module		*owner;
> >  	const char		*name;
> >  	int			id;
> > @@ -116,6 +119,7 @@ struct clk_functions {
> >  
> >  extern unsigned int mpurate;
> >  
> > +extern void omap_clk_associate(const char *id, struct device *dev);
> >  extern int clk_init(struct clk_functions *custom_clocks);
> >  extern int clk_register(struct clk *clk);
> >  extern void clk_unregister(struct clk *clk);
> > -- 
> > 1.6.0.2.307.gc427
> 
> -- 
> balbi
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
balbi