[3/5] arm/dt: mx51: dynamically add gpt and uart related clocks per dt nodes

This patch is to change the static clock creating and registering to
the dynamic way, which scans dt clock nodes, associate clk with
device_node, and then add them to clkdev accordingly.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 arch/arm/mach-mx5/clock-mx51-mx53.c |  436 +++++++++++++++++++++++++++++++++--
 1 files changed, 422 insertions(+), 14 deletions(-)

On Tue, Mar 08, 2011 at 12:22:10AM +0800, Shawn Guo wrote:
> This patch is to change the static clock creating and registering to
> the dynamic way, which scans dt clock nodes, associate clk with
> device_node, and then add them to clkdev accordingly.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  arch/arm/mach-mx5/clock-mx51-mx53.c |  436 +++++++++++++++++++++++++++++++++--
>  1 files changed, 422 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm/mach-mx5/clock-mx51-mx53.c b/arch/arm/mach-mx5/clock-mx51-mx53.c
> index dedb7f9..1940171 100644
> --- a/arch/arm/mach-mx5/clock-mx51-mx53.c
> +++ b/arch/arm/mach-mx5/clock-mx51-mx53.c
> @@ -135,6 +135,9 @@ static inline u32 _get_mux(struct clk *parent, struct clk *m0,
>  
>  static inline void __iomem *_mx51_get_pll_base(struct clk *pll)
>  {
> +#ifdef CONFIG_OF
> +	return pll->pll_base;
> +#else
>  	if (pll == &pll1_main_clk)
>  		return MX51_DPLL1_BASE;
>  	else if (pll == &pll2_sw_clk)
> @@ -145,6 +148,7 @@ static inline void __iomem *_mx51_get_pll_base(struct clk *pll)
>  		BUG();
>  
>  	return NULL;
> +#endif
>  }
>  
>  static inline void __iomem *_mx53_get_pll_base(struct clk *pll)
> @@ -1439,33 +1443,437 @@ int __init mx53_clocks_init(unsigned long ckil, unsigned long osc,
>  	return 0;
>  }
>  
> +/*
> + * Dynamically create and register clks per dt nodes
> + */
>  #ifdef CONFIG_OF
> -static struct clk *mx5_dt_clk_get(struct device_node *np,
> -					const char *output_id, void *data)
> +
> +#define ALLOC_CLK_LOOKUP()						\
> +	struct clk_lookup *cl;						\
> +	struct clk *clk;						\
> +	int ret;							\
> +									\
> +	do {								\
> +		cl = kzalloc(sizeof(*cl) + sizeof(*clk), GFP_KERNEL);	\
> +		if (!cl)						\
> +			return -ENOMEM;					\
> +		clk = (struct clk *) (cl + 1);				\
> +									\
> +		clk->parent = mx5_get_source_clk(node);			\
> +		clk->secondary = mx5_get_source_clk(node);		\
> +	} while (0)
> +
> +#define ADD_CLK_LOOKUP()						\
> +	do {								\
> +		node->data = clk;					\
> +		cl->dev_id = of_get_property(node,			\
> +				"clock-outputs", NULL);			\
> +		cl->con_id = of_get_property(node,			\
> +				"clock-alias", NULL);			\
> +		if (!cl->dev_id && !cl->con_id) {			\
> +			ret = -EINVAL;					\
> +			goto out_kfree;					\
> +		}							\
> +		cl->clk = clk;						\
> +		clkdev_add(cl);						\
> +									\
> +		return 0;						\
> +									\
> +	out_kfree:							\
> +		kfree(cl);						\
> +		return ret;						\
> +	} while (0)

Yikes!  Doing this as a macro will be a nightmare for debugging.
This needs refactoring into functions.

> +
> +static unsigned long get_fixed_clk_rate(struct clk *clk)
>  {
> -	return data;
> +	return clk->rate;
>  }
>  
> -static __init void mx5_dt_scan_clks(void)
> +static __init int mx5_scan_fixed_clks(void)
>  {
>  	struct device_node *node;
> +	struct clk_lookup *cl;
>  	struct clk *clk;
> -	const char *id;
> -	int rc;
> +	const __be32 *rate;
> +	int ret = 0;
>  
> -	for_each_compatible_node(node, NULL, "clock") {
> -		id = of_get_property(node, "clock-outputs", NULL);
> -		if (!id)
> +	for_each_compatible_node(node, NULL, "fixed-clock") {
> +		cl = kzalloc(sizeof(*cl) + sizeof(*clk), GFP_KERNEL);
> +		if (!cl) {
> +			ret = -ENOMEM;
> +			break;
> +		}
> +		clk = (struct clk *) (cl + 1);
> +
> +		rate = of_get_property(node, "clock-frequency", NULL);
> +		if (!rate) {
> +			kfree(cl);
>  			continue;
> +		}
> +		clk->rate = be32_to_cpu(*rate);
> +		clk->get_rate = get_fixed_clk_rate;
> +
> +		node->data = clk;
>  
> -		clk = clk_get_sys(id, NULL);
> -		if (IS_ERR(clk))
> +		cl->dev_id = of_get_property(node, "clock-outputs", NULL);
> +		cl->con_id = of_get_property(node, "clock-alias", NULL);

As discussed briefly earlier, clock-alias looks like it is encoding
Linux-specific implementation details into the device tree, and it
shouldn't be necessary when explicit links to clock providers are
supplied in the device tree.

> +		if (!cl->dev_id && !cl->con_id) {
> +			kfree(cl);
>  			continue;
> +		}
> +		cl->clk = clk;
> +		clkdev_add(cl);
> +	}
> +
> +	return ret;
> +}
> +
> +static struct clk *mx5_prop_name_to_clk(struct device_node *node,
> +		const char *prop_name)
> +{
> +	struct device_node *provnode;
> +	struct clk *clk;
> +	const void *prop;
> +	u32 provhandle;
> +
> +	prop = of_get_property(node, prop_name, NULL);
> +	if (!prop)
> +		return NULL;
> +	provhandle = be32_to_cpup(prop);
> +
> +	provnode = of_find_node_by_phandle(provhandle);
> +	if (!provnode)
> +		return NULL;
> +
> +	clk = provnode->data;
> +
> +	of_node_put(provnode);
> +
> +	return clk;
> +}
> +
> +static inline struct clk *mx5_get_source_clk(struct device_node *node)
> +{
> +	return mx5_prop_name_to_clk(node, "clock-source");
> +}
> +
> +static inline struct clk *mx5_get_depend_clk(struct device_node *node)
> +{
> +	return mx5_prop_name_to_clk(node, "clock-depend");
> +}

Ditto here.  'clock-depend' seems to be Linux specifc.  I need to look
at the usage model for these properties.

>  
> -		rc = of_clk_add_provider(node, mx5_dt_clk_get, clk);
> -		if (rc)
> -			pr_err("error adding fixed clk %s\n", node->name);
> +static __init int mx5_add_uart_clk(struct device_node *node)
> +{
> +	const __be32 *reg;
> +	int id;
> +
> +	ALLOC_CLK_LOOKUP();
> +
> +	reg = of_get_property(node, "reg", NULL);
> +	if (!reg) {
> +		ret = -ENOENT;
> +		goto out_kfree;
> +	}
> +
> +	id = be32_to_cpu(*reg);
> +	if (id < 0 || id > 2) {
> +		ret = -EINVAL;
> +		goto out_kfree;
> +	}
> +
> +	clk->id = id;
> +	clk->parent = mx5_get_source_clk(node);
> +	clk->secondary = mx5_get_depend_clk(node);
> +	clk->enable = _clk_ccgr_enable;
> +	clk->disable = _clk_ccgr_disable;
> +	clk->enable_reg = MXC_CCM_CCGR1;
> +
> +	switch (id) {
> +	case 0:
> +		clk->enable_shift = MXC_CCM_CCGRx_CG4_OFFSET;
> +		break;
> +	case 1:
> +		clk->enable_shift = MXC_CCM_CCGRx_CG6_OFFSET;
> +		break;
> +	case 2:
> +		clk->enable_shift = MXC_CCM_CCGRx_CG8_OFFSET;
> +	}
> +
> +	ADD_CLK_LOOKUP();
> +}
> +
> +static __init int mx5_add_uart_root_clk(struct device_node *node)
> +{
> +	ALLOC_CLK_LOOKUP();
> +
> +	clk->get_rate = clk_uart_get_rate;
> +	clk->set_parent = clk_uart_set_parent;
> +
> +	ADD_CLK_LOOKUP();
> +}
> +
> +static __init int mx5_add_uart_ipg_clk(struct device_node *node)
> +{
> +	const __be32 *reg;
> +	int id;
> +
> +	ALLOC_CLK_LOOKUP();
> +
> +	reg = of_get_property(node, "reg", NULL);
> +	if (!reg) {
> +		ret = -ENOENT;
> +		goto out_kfree;
>  	}
> +
> +	id = be32_to_cpu(*reg);
> +	if (id < 0 || id > 2) {
> +		ret = -EINVAL;
> +		goto out_kfree;
> +	}
> +
> +	clk->id = id;
> +	clk->enable_reg = MXC_CCM_CCGR1;
> +	clk->enable = _clk_ccgr_enable;
> +	clk->disable = _clk_ccgr_disable;
> +
> +	switch (id) {
> +	case 0:
> +		clk->enable_shift = MXC_CCM_CCGRx_CG3_OFFSET;
> +		break;
> +	case 1:
> +		clk->enable_shift = MXC_CCM_CCGRx_CG5_OFFSET;
> +		break;
> +	case 2:
> +		clk->enable_shift = MXC_CCM_CCGRx_CG7_OFFSET;
> +	}
> +
> +	ADD_CLK_LOOKUP();
> +}
> +
> +static __init int mx5_add_gpt_clk(struct device_node *node)
> +{
> +	ALLOC_CLK_LOOKUP();
> +
> +	clk->enable_reg = MXC_CCM_CCGR2;
> +	clk->enable_shift = MXC_CCM_CCGRx_CG9_OFFSET;
> +	clk->enable = _clk_ccgr_enable;
> +	clk->disable = _clk_ccgr_disable;
> +
> +	ADD_CLK_LOOKUP();
> +}
> +
> +static __init int mx5_add_gpt_ipg_clk(struct device_node *node)
> +{
> +	ALLOC_CLK_LOOKUP();
> +
> +	clk->enable_reg = MXC_CCM_CCGR2;
> +	clk->enable_shift = MXC_CCM_CCGRx_CG10_OFFSET;
> +	clk->enable = _clk_ccgr_enable;
> +	clk->disable = _clk_ccgr_disable;
> +
> +	ADD_CLK_LOOKUP();
> +}
> +
> +static __init int mx5_add_aips_tz_clk(struct device_node *node)
> +{
> +	const __be32 *reg;
> +	int id;
> +
> +	ALLOC_CLK_LOOKUP();
> +
> +	reg = of_get_property(node, "reg", NULL);
> +	if (!reg) {
> +		ret = -ENOENT;
> +		goto out_kfree;
> +	}
> +
> +	id = be32_to_cpu(*reg);
> +	if (id < 0 || id > 1) {
> +		ret = -EINVAL;
> +		goto out_kfree;
> +	}
> +
> +	clk->id = id;
> +	clk->enable_reg = MXC_CCM_CCGR0;
> +	clk->enable_shift = id ? MXC_CCM_CCGRx_CG12_OFFSET :
> +				 MXC_CCM_CCGRx_CG13_OFFSET;
> +	clk->enable = _clk_ccgr_enable;
> +	clk->disable = _clk_ccgr_disable_inwait;
> +
> +	ADD_CLK_LOOKUP();
> +}
> +
> +static __init int mx5_add_ahb_max_clk(struct device_node *node)
> +{
> +	ALLOC_CLK_LOOKUP();
> +
> +	clk->enable_reg = MXC_CCM_CCGR0;
> +	clk->enable_shift = MXC_CCM_CCGRx_CG14_OFFSET;
> +	clk->enable = _clk_max_enable;
> +	clk->disable = _clk_max_disable;
> +
> +	ADD_CLK_LOOKUP();
> +}
> +
> +static __init int mx5_add_spba_clk(struct device_node *node)
> +{
> +	ALLOC_CLK_LOOKUP();
> +
> +	clk->enable_reg = MXC_CCM_CCGR5;
> +	clk->enable_shift = MXC_CCM_CCGRx_CG0_OFFSET;
> +	clk->enable = _clk_ccgr_enable;
> +	clk->disable = _clk_ccgr_disable;
> +
> +	ADD_CLK_LOOKUP();
> +}
> +
> +static __init int mx5_add_ipg_clk(struct device_node *node)
> +{
> +	ALLOC_CLK_LOOKUP();
> +
> +	clk->get_rate = clk_ipg_get_rate;
> +
> +	ADD_CLK_LOOKUP();
> +}
> +
> +static __init int mx5_add_ahb_clk(struct device_node *node)
> +{
> +	ALLOC_CLK_LOOKUP();
> +
> +	clk->get_rate = clk_ahb_get_rate;
> +	clk->set_rate = _clk_ahb_set_rate;
> +	clk->round_rate = _clk_ahb_round_rate;
> +
> +	ADD_CLK_LOOKUP();
> +}
> +
> +static __init int mx5_add_main_bus_clk(struct device_node *node)
> +{
> +	ALLOC_CLK_LOOKUP();
> +
> +	clk->set_parent = _clk_main_bus_set_parent;
> +
> +	ADD_CLK_LOOKUP();
> +}
> +
> +static __init int mx5_add_lp_apm_clk(struct device_node *node)
> +{
> +	ALLOC_CLK_LOOKUP();
> +
> +	clk->set_parent = _clk_lp_apm_set_parent;
> +
> +	ADD_CLK_LOOKUP();
> +}
> +
> +static __init int mx5_add_pll_switch_clk(struct device_node *node)
> +{
> +	const __be32 *reg;
> +	int id;
> +
> +	ALLOC_CLK_LOOKUP();
> +
> +	reg = of_get_property(node, "reg", NULL);
> +	if (!reg) {
> +		ret = -ENOENT;
> +		goto out_kfree;
> +	}
> +
> +	id = be32_to_cpu(*reg);
> +	if (id < 0 || id > 2) {
> +		ret = -EINVAL;
> +		goto out_kfree;
> +	}
> +
> +	clk->id = id;
> +
> +	switch (id) {
> +	case 0:
> +		clk->get_rate = clk_pll1_sw_get_rate;
> +		clk->set_parent = _clk_pll1_sw_set_parent;
> +		break;
> +	case 1:
> +		clk->get_rate = clk_pll_get_rate;
> +		clk->set_rate = _clk_pll_set_rate;
> +		clk->enable = _clk_pll_enable;
> +		clk->disable = _clk_pll_disable;
> +		clk->set_parent = _clk_pll2_sw_set_parent;
> +		clk->pll_base = MX51_DPLL2_BASE;
> +		break;
> +	case 2:
> +		clk->get_rate = clk_pll_get_rate;
> +		clk->set_rate = _clk_pll_set_rate;
> +		clk->enable = _clk_pll_enable;
> +		clk->disable = _clk_pll_disable;
> +		clk->pll_base = MX51_DPLL3_BASE;
> +	}
> +
> +	ADD_CLK_LOOKUP();
> +}
> +
> +static __init int mx5_add_pll1_main_clk(struct device_node *node)
> +{
> +	ALLOC_CLK_LOOKUP();
> +
> +	clk->get_rate = clk_pll_get_rate;
> +	clk->enable = _clk_pll_enable;
> +	clk->disable = _clk_pll_disable;
> +	clk->pll_base = MX51_DPLL1_BASE;
> +
> +	ADD_CLK_LOOKUP();
> +}
> +
> +static __init int mx5_dt_scan_clks(void)
> +{
> +	struct device_node *node;
> +	int ret;
> +
> +	ret = mx5_scan_fixed_clks();
> +	if (ret) {
> +		pr_err("%s: fixed-clock failed %d\n", __func__, ret);
> +		return ret;
> +	}
> +
> +	for_each_compatible_node(node, NULL, "clock") {
> +		if (!strcmp(node->name, "pll1_main"))
> +			ret = mx5_add_pll1_main_clk(node);
> +		else if (!strcmp(node->name, "pll_switch"))
> +			ret = mx5_add_pll_switch_clk(node);
> +		else if (!strcmp(node->name, "lp_apm"))
> +			ret = mx5_add_lp_apm_clk(node);
> +		else if (!strcmp(node->name, "main_bus"))
> +			ret = mx5_add_main_bus_clk(node);
> +		else if (!strcmp(node->name, "ahb"))
> +			ret = mx5_add_ahb_clk(node);
> +		else if (!strcmp(node->name, "ipg"))
> +			ret = mx5_add_ipg_clk(node);
> +		else if (!strcmp(node->name, "spba"))
> +			ret = mx5_add_spba_clk(node);
> +		else if (!strcmp(node->name, "ahb_max"))
> +			ret = mx5_add_ahb_max_clk(node);
> +		else if (!strcmp(node->name, "aips_tz"))
> +			ret = mx5_add_aips_tz_clk(node);
> +		else if (!strcmp(node->name, "gpt_ipg"))
> +			ret = mx5_add_gpt_ipg_clk(node);
> +		else if (!strcmp(node->name, "gpt"))
> +			ret = mx5_add_gpt_clk(node);
> +		else if (!strcmp(node->name, "uart_ipg"))
> +			ret = mx5_add_uart_ipg_clk(node);
> +		else if (!strcmp(node->name, "uart_root"))
> +			ret = mx5_add_uart_root_clk(node);
> +		else if (!strcmp(node->name, "uart"))
> +			ret = mx5_add_uart_clk(node);

You can simplify this whole table is you take advantage of the .data
field in struct of_device_id, and use for_each_matching_node() to
search for relevant nodes.

> +		else
> +			pr_warn("%s: unknown clock node %s\n",
> +				__func__, node->name);
> +
> +		if (ret) {
> +			pr_err("%s: clock %s failed %d\n",
> +				__func__, node->name, ret);
> +			break;
> +		}
> +	}
> +
> +	return ret;
>  }
>  
>  void __init mx5_clk_dt_init(void)
> -- 
> 1.7.1
> 
> 
> _______________________________________________
> linaro-dev mailing list
> linaro-dev@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev

On Tue, Mar 15, 2011 at 01:37:31AM -0600, Grant Likely wrote:
> On Tue, Mar 08, 2011 at 12:22:10AM +0800, Shawn Guo wrote:
> > This patch is to change the static clock creating and registering to
> > the dynamic way, which scans dt clock nodes, associate clk with
> > device_node, and then add them to clkdev accordingly.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> >  arch/arm/mach-mx5/clock-mx51-mx53.c |  436 +++++++++++++++++++++++++++++++++--
> >  1 files changed, 422 insertions(+), 14 deletions(-)
> > 
> > diff --git a/arch/arm/mach-mx5/clock-mx51-mx53.c b/arch/arm/mach-mx5/clock-mx51-mx53.c
> > index dedb7f9..1940171 100644
> > --- a/arch/arm/mach-mx5/clock-mx51-mx53.c
> > +++ b/arch/arm/mach-mx5/clock-mx51-mx53.c
> > @@ -135,6 +135,9 @@ static inline u32 _get_mux(struct clk *parent, struct clk *m0,
> >  
> >  static inline void __iomem *_mx51_get_pll_base(struct clk *pll)
> >  {
> > +#ifdef CONFIG_OF
> > +	return pll->pll_base;
> > +#else
> >  	if (pll == &pll1_main_clk)
> >  		return MX51_DPLL1_BASE;
> >  	else if (pll == &pll2_sw_clk)
> > @@ -145,6 +148,7 @@ static inline void __iomem *_mx51_get_pll_base(struct clk *pll)
> >  		BUG();
> >  
> >  	return NULL;
> > +#endif
> >  }
> >  
> >  static inline void __iomem *_mx53_get_pll_base(struct clk *pll)
> > @@ -1439,33 +1443,437 @@ int __init mx53_clocks_init(unsigned long ckil, unsigned long osc,
> >  	return 0;
> >  }
> >  
> > +/*
> > + * Dynamically create and register clks per dt nodes
> > + */
> >  #ifdef CONFIG_OF
> > -static struct clk *mx5_dt_clk_get(struct device_node *np,
> > -					const char *output_id, void *data)
> > +
> > +#define ALLOC_CLK_LOOKUP()						\
> > +	struct clk_lookup *cl;						\
> > +	struct clk *clk;						\
> > +	int ret;							\
> > +									\
> > +	do {								\
> > +		cl = kzalloc(sizeof(*cl) + sizeof(*clk), GFP_KERNEL);	\
> > +		if (!cl)						\
> > +			return -ENOMEM;					\
> > +		clk = (struct clk *) (cl + 1);				\
> > +									\
> > +		clk->parent = mx5_get_source_clk(node);			\
> > +		clk->secondary = mx5_get_source_clk(node);		\
> > +	} while (0)
> > +
> > +#define ADD_CLK_LOOKUP()						\
> > +	do {								\
> > +		node->data = clk;					\
> > +		cl->dev_id = of_get_property(node,			\
> > +				"clock-outputs", NULL);			\
> > +		cl->con_id = of_get_property(node,			\
> > +				"clock-alias", NULL);			\
> > +		if (!cl->dev_id && !cl->con_id) {			\
> > +			ret = -EINVAL;					\
> > +			goto out_kfree;					\
> > +		}							\
> > +		cl->clk = clk;						\
> > +		clkdev_add(cl);						\
> > +									\
> > +		return 0;						\
> > +									\
> > +	out_kfree:							\
> > +		kfree(cl);						\
> > +		return ret;						\
> > +	} while (0)
> 
> Yikes!  Doing this as a macro will be a nightmare for debugging.
> This needs refactoring into functions.
> 
> > +
> > +static unsigned long get_fixed_clk_rate(struct clk *clk)
> >  {
> > -	return data;
> > +	return clk->rate;
> >  }
> >  
> > -static __init void mx5_dt_scan_clks(void)
> > +static __init int mx5_scan_fixed_clks(void)
> >  {
> >  	struct device_node *node;
> > +	struct clk_lookup *cl;
> >  	struct clk *clk;
> > -	const char *id;
> > -	int rc;
> > +	const __be32 *rate;
> > +	int ret = 0;
> >  
> > -	for_each_compatible_node(node, NULL, "clock") {
> > -		id = of_get_property(node, "clock-outputs", NULL);
> > -		if (!id)
> > +	for_each_compatible_node(node, NULL, "fixed-clock") {
> > +		cl = kzalloc(sizeof(*cl) + sizeof(*clk), GFP_KERNEL);
> > +		if (!cl) {
> > +			ret = -ENOMEM;
> > +			break;
> > +		}
> > +		clk = (struct clk *) (cl + 1);
> > +
> > +		rate = of_get_property(node, "clock-frequency", NULL);
> > +		if (!rate) {
> > +			kfree(cl);
> >  			continue;
> > +		}
> > +		clk->rate = be32_to_cpu(*rate);
> > +		clk->get_rate = get_fixed_clk_rate;
> > +
> > +		node->data = clk;
> >  
> > -		clk = clk_get_sys(id, NULL);
> > -		if (IS_ERR(clk))
> > +		cl->dev_id = of_get_property(node, "clock-outputs", NULL);
> > +		cl->con_id = of_get_property(node, "clock-alias", NULL);
> 
> As discussed briefly earlier, clock-alias looks like it is encoding
> Linux-specific implementation details into the device tree, and it
> shouldn't be necessary when explicit links to clock providers are
> supplied in the device tree.
> 
> > +		if (!cl->dev_id && !cl->con_id) {
> > +			kfree(cl);
> >  			continue;
> > +		}
> > +		cl->clk = clk;
> > +		clkdev_add(cl);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static struct clk *mx5_prop_name_to_clk(struct device_node *node,
> > +		const char *prop_name)
> > +{
> > +	struct device_node *provnode;
> > +	struct clk *clk;
> > +	const void *prop;
> > +	u32 provhandle;
> > +
> > +	prop = of_get_property(node, prop_name, NULL);
> > +	if (!prop)
> > +		return NULL;
> > +	provhandle = be32_to_cpup(prop);
> > +
> > +	provnode = of_find_node_by_phandle(provhandle);
> > +	if (!provnode)
> > +		return NULL;
> > +
> > +	clk = provnode->data;
> > +
> > +	of_node_put(provnode);
> > +
> > +	return clk;
> > +}
> > +
> > +static inline struct clk *mx5_get_source_clk(struct device_node *node)
> > +{
> > +	return mx5_prop_name_to_clk(node, "clock-source");
> > +}
> > +
> > +static inline struct clk *mx5_get_depend_clk(struct device_node *node)
> > +{
> > +	return mx5_prop_name_to_clk(node, "clock-depend");
> > +}
> 
> Ditto here.  'clock-depend' seems to be Linux specifc.  I need to look
> at the usage model for these properties.
> 
This is not Linux but hardware specific.  Let's look at the eSDHC1
example below.

                esdhc1_clk: esdhc@0 {
                        compatible = "fsl,mxc-clock";
                        reg = <0>;
                        clock-outputs = "sdhci-esdhc-imx.0";
                        clock-source = <&pll2_sw_clk>;
                        clock-depend = <&esdhc1_ipg_clk>;
                };


We have esdhc1_clk added to clkdev standing for the clock for hardware
block eSDHC1.  This clock is actually the serial clock of eSDHC1,
while eSDHC1 internal working clock esdhc1_ipg_clk has also to be on
to get the block functional.

On Wed, Mar 16, 2011 at 08:04:56PM +0800, Shawn Guo wrote:
> On Tue, Mar 15, 2011 at 01:37:31AM -0600, Grant Likely wrote:
> > On Tue, Mar 08, 2011 at 12:22:10AM +0800, Shawn Guo wrote:
> > > This patch is to change the static clock creating and registering to
> > > the dynamic way, which scans dt clock nodes, associate clk with
> > > device_node, and then add them to clkdev accordingly.
> > > 
> > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > ---
> > >  arch/arm/mach-mx5/clock-mx51-mx53.c |  436 +++++++++++++++++++++++++++++++++--
> > >  1 files changed, 422 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/arch/arm/mach-mx5/clock-mx51-mx53.c b/arch/arm/mach-mx5/clock-mx51-mx53.c
> > > index dedb7f9..1940171 100644
> > > --- a/arch/arm/mach-mx5/clock-mx51-mx53.c
> > > +++ b/arch/arm/mach-mx5/clock-mx51-mx53.c
> > > @@ -135,6 +135,9 @@ static inline u32 _get_mux(struct clk *parent, struct clk *m0,
> > >  
> > >  static inline void __iomem *_mx51_get_pll_base(struct clk *pll)
> > >  {
> > > +#ifdef CONFIG_OF
> > > +	return pll->pll_base;
> > > +#else
> > >  	if (pll == &pll1_main_clk)
> > >  		return MX51_DPLL1_BASE;
> > >  	else if (pll == &pll2_sw_clk)
> > > @@ -145,6 +148,7 @@ static inline void __iomem *_mx51_get_pll_base(struct clk *pll)
> > >  		BUG();
> > >  
> > >  	return NULL;
> > > +#endif
> > >  }
> > >  
> > >  static inline void __iomem *_mx53_get_pll_base(struct clk *pll)
> > > @@ -1439,33 +1443,437 @@ int __init mx53_clocks_init(unsigned long ckil, unsigned long osc,
> > >  	return 0;
> > >  }
> > >  
> > > +/*
> > > + * Dynamically create and register clks per dt nodes
> > > + */
> > >  #ifdef CONFIG_OF
> > > -static struct clk *mx5_dt_clk_get(struct device_node *np,
> > > -					const char *output_id, void *data)
> > > +
> > > +#define ALLOC_CLK_LOOKUP()						\
> > > +	struct clk_lookup *cl;						\
> > > +	struct clk *clk;						\
> > > +	int ret;							\
> > > +									\
> > > +	do {								\
> > > +		cl = kzalloc(sizeof(*cl) + sizeof(*clk), GFP_KERNEL);	\
> > > +		if (!cl)						\
> > > +			return -ENOMEM;					\
> > > +		clk = (struct clk *) (cl + 1);				\
> > > +									\
> > > +		clk->parent = mx5_get_source_clk(node);			\
> > > +		clk->secondary = mx5_get_source_clk(node);		\
> > > +	} while (0)
> > > +
> > > +#define ADD_CLK_LOOKUP()						\
> > > +	do {								\
> > > +		node->data = clk;					\
> > > +		cl->dev_id = of_get_property(node,			\
> > > +				"clock-outputs", NULL);			\
> > > +		cl->con_id = of_get_property(node,			\
> > > +				"clock-alias", NULL);			\
> > > +		if (!cl->dev_id && !cl->con_id) {			\
> > > +			ret = -EINVAL;					\
> > > +			goto out_kfree;					\
> > > +		}							\
> > > +		cl->clk = clk;						\
> > > +		clkdev_add(cl);						\
> > > +									\
> > > +		return 0;						\
> > > +									\
> > > +	out_kfree:							\
> > > +		kfree(cl);						\
> > > +		return ret;						\
> > > +	} while (0)
> > 
> > Yikes!  Doing this as a macro will be a nightmare for debugging.
> > This needs refactoring into functions.
> > 
> > > +
> > > +static unsigned long get_fixed_clk_rate(struct clk *clk)
> > >  {
> > > -	return data;
> > > +	return clk->rate;
> > >  }
> > >  
> > > -static __init void mx5_dt_scan_clks(void)
> > > +static __init int mx5_scan_fixed_clks(void)
> > >  {
> > >  	struct device_node *node;
> > > +	struct clk_lookup *cl;
> > >  	struct clk *clk;
> > > -	const char *id;
> > > -	int rc;
> > > +	const __be32 *rate;
> > > +	int ret = 0;
> > >  
> > > -	for_each_compatible_node(node, NULL, "clock") {
> > > -		id = of_get_property(node, "clock-outputs", NULL);
> > > -		if (!id)
> > > +	for_each_compatible_node(node, NULL, "fixed-clock") {
> > > +		cl = kzalloc(sizeof(*cl) + sizeof(*clk), GFP_KERNEL);
> > > +		if (!cl) {
> > > +			ret = -ENOMEM;
> > > +			break;
> > > +		}
> > > +		clk = (struct clk *) (cl + 1);
> > > +
> > > +		rate = of_get_property(node, "clock-frequency", NULL);
> > > +		if (!rate) {
> > > +			kfree(cl);
> > >  			continue;
> > > +		}
> > > +		clk->rate = be32_to_cpu(*rate);
> > > +		clk->get_rate = get_fixed_clk_rate;
> > > +
> > > +		node->data = clk;
> > >  
> > > -		clk = clk_get_sys(id, NULL);
> > > -		if (IS_ERR(clk))
> > > +		cl->dev_id = of_get_property(node, "clock-outputs", NULL);
> > > +		cl->con_id = of_get_property(node, "clock-alias", NULL);
> > 
> > As discussed briefly earlier, clock-alias looks like it is encoding
> > Linux-specific implementation details into the device tree, and it
> > shouldn't be necessary when explicit links to clock providers are
> > supplied in the device tree.
> > 
> > > +		if (!cl->dev_id && !cl->con_id) {
> > > +			kfree(cl);
> > >  			continue;
> > > +		}
> > > +		cl->clk = clk;
> > > +		clkdev_add(cl);
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static struct clk *mx5_prop_name_to_clk(struct device_node *node,
> > > +		const char *prop_name)
> > > +{
> > > +	struct device_node *provnode;
> > > +	struct clk *clk;
> > > +	const void *prop;
> > > +	u32 provhandle;
> > > +
> > > +	prop = of_get_property(node, prop_name, NULL);
> > > +	if (!prop)
> > > +		return NULL;
> > > +	provhandle = be32_to_cpup(prop);
> > > +
> > > +	provnode = of_find_node_by_phandle(provhandle);
> > > +	if (!provnode)
> > > +		return NULL;
> > > +
> > > +	clk = provnode->data;
> > > +
> > > +	of_node_put(provnode);
> > > +
> > > +	return clk;
> > > +}
> > > +
> > > +static inline struct clk *mx5_get_source_clk(struct device_node *node)
> > > +{
> > > +	return mx5_prop_name_to_clk(node, "clock-source");
> > > +}
> > > +
> > > +static inline struct clk *mx5_get_depend_clk(struct device_node *node)
> > > +{
> > > +	return mx5_prop_name_to_clk(node, "clock-depend");
> > > +}
> > 
> > Ditto here.  'clock-depend' seems to be Linux specifc.  I need to look
> > at the usage model for these properties.
> > 
> This is not Linux but hardware specific.  Let's look at the eSDHC1
> example below.
> 
>                 esdhc1_clk: esdhc@0 {
>                         compatible = "fsl,mxc-clock";
>                         reg = <0>;
>                         clock-outputs = "sdhci-esdhc-imx.0";
>                         clock-source = <&pll2_sw_clk>;
>                         clock-depend = <&esdhc1_ipg_clk>;
>                 };
> 
> 
> We have esdhc1_clk added to clkdev standing for the clock for hardware
> block eSDHC1.  This clock is actually the serial clock of eSDHC1,
> while eSDHC1 internal working clock esdhc1_ipg_clk has also to be on
> to get the block functional.

Actually, part of what I think is throwing me off here is that this
node is only using half the clock binding.  A single node can be both
a clock provider and a clock consumer, which will often be the case
for clock controllers like this.  So in this case, it is using the
correct "clock-outputs" property to declare the clocks that it
provides, but it isn't using the *-clock binding to reference the
clocks that it needs.  This really should be something like:

        esdhc1_clk: esdhc@0 {
                compatible = "fsl,mxc-clock";
                reg = <0>;
                clock-outputs = "sdhci-esdhc-imx.0";
                src-clock = <&pll2_sw_clk>, "sw-clk";
                ipg-clock = <&esdhc1_ipg_clk>, "ipg-clk";
        };

Also remember that a single clock node can provide multiple clock
outputs.  I don't know if this is a factor for imx51, but if it is
then you should layout the clock nodes to replicate the actual clock
hardware topology in the hardware (as opposed to the software layout
that Linux is currently using).

g.

On Thu, Mar 17, 2011 at 02:47:49PM -0600, Grant Likely wrote:
> On Wed, Mar 16, 2011 at 08:04:56PM +0800, Shawn Guo wrote:
> > On Tue, Mar 15, 2011 at 01:37:31AM -0600, Grant Likely wrote:
> > > On Tue, Mar 08, 2011 at 12:22:10AM +0800, Shawn Guo wrote:
> > > > This patch is to change the static clock creating and registering to
> > > > the dynamic way, which scans dt clock nodes, associate clk with
> > > > device_node, and then add them to clkdev accordingly.
> > > > 
> > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > > ---
> > > >  arch/arm/mach-mx5/clock-mx51-mx53.c |  436 +++++++++++++++++++++++++++++++++--
> > > >  1 files changed, 422 insertions(+), 14 deletions(-)
> > > > 
> > > > diff --git a/arch/arm/mach-mx5/clock-mx51-mx53.c b/arch/arm/mach-mx5/clock-mx51-mx53.c
> > > > index dedb7f9..1940171 100644
> > > > --- a/arch/arm/mach-mx5/clock-mx51-mx53.c
> > > > +++ b/arch/arm/mach-mx5/clock-mx51-mx53.c
> > > > @@ -135,6 +135,9 @@ static inline u32 _get_mux(struct clk *parent, struct clk *m0,
> > > >  
> > > >  static inline void __iomem *_mx51_get_pll_base(struct clk *pll)
> > > >  {
> > > > +#ifdef CONFIG_OF
> > > > +	return pll->pll_base;
> > > > +#else
> > > >  	if (pll == &pll1_main_clk)
> > > >  		return MX51_DPLL1_BASE;
> > > >  	else if (pll == &pll2_sw_clk)
> > > > @@ -145,6 +148,7 @@ static inline void __iomem *_mx51_get_pll_base(struct clk *pll)
> > > >  		BUG();
> > > >  
> > > >  	return NULL;
> > > > +#endif
> > > >  }
> > > >  
> > > >  static inline void __iomem *_mx53_get_pll_base(struct clk *pll)
> > > > @@ -1439,33 +1443,437 @@ int __init mx53_clocks_init(unsigned long ckil, unsigned long osc,
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +/*
> > > > + * Dynamically create and register clks per dt nodes
> > > > + */
> > > >  #ifdef CONFIG_OF
> > > > -static struct clk *mx5_dt_clk_get(struct device_node *np,
> > > > -					const char *output_id, void *data)
> > > > +
> > > > +#define ALLOC_CLK_LOOKUP()						\
> > > > +	struct clk_lookup *cl;						\
> > > > +	struct clk *clk;						\
> > > > +	int ret;							\
> > > > +									\
> > > > +	do {								\
> > > > +		cl = kzalloc(sizeof(*cl) + sizeof(*clk), GFP_KERNEL);	\
> > > > +		if (!cl)						\
> > > > +			return -ENOMEM;					\
> > > > +		clk = (struct clk *) (cl + 1);				\
> > > > +									\
> > > > +		clk->parent = mx5_get_source_clk(node);			\
> > > > +		clk->secondary = mx5_get_source_clk(node);		\
> > > > +	} while (0)
> > > > +
> > > > +#define ADD_CLK_LOOKUP()						\
> > > > +	do {								\
> > > > +		node->data = clk;					\
> > > > +		cl->dev_id = of_get_property(node,			\
> > > > +				"clock-outputs", NULL);			\
> > > > +		cl->con_id = of_get_property(node,			\
> > > > +				"clock-alias", NULL);			\
> > > > +		if (!cl->dev_id && !cl->con_id) {			\
> > > > +			ret = -EINVAL;					\
> > > > +			goto out_kfree;					\
> > > > +		}							\
> > > > +		cl->clk = clk;						\
> > > > +		clkdev_add(cl);						\
> > > > +									\
> > > > +		return 0;						\
> > > > +									\
> > > > +	out_kfree:							\
> > > > +		kfree(cl);						\
> > > > +		return ret;						\
> > > > +	} while (0)
> > > 
> > > Yikes!  Doing this as a macro will be a nightmare for debugging.
> > > This needs refactoring into functions.
> > > 
> > > > +
> > > > +static unsigned long get_fixed_clk_rate(struct clk *clk)
> > > >  {
> > > > -	return data;
> > > > +	return clk->rate;
> > > >  }
> > > >  
> > > > -static __init void mx5_dt_scan_clks(void)
> > > > +static __init int mx5_scan_fixed_clks(void)
> > > >  {
> > > >  	struct device_node *node;
> > > > +	struct clk_lookup *cl;
> > > >  	struct clk *clk;
> > > > -	const char *id;
> > > > -	int rc;
> > > > +	const __be32 *rate;
> > > > +	int ret = 0;
> > > >  
> > > > -	for_each_compatible_node(node, NULL, "clock") {
> > > > -		id = of_get_property(node, "clock-outputs", NULL);
> > > > -		if (!id)
> > > > +	for_each_compatible_node(node, NULL, "fixed-clock") {
> > > > +		cl = kzalloc(sizeof(*cl) + sizeof(*clk), GFP_KERNEL);
> > > > +		if (!cl) {
> > > > +			ret = -ENOMEM;
> > > > +			break;
> > > > +		}
> > > > +		clk = (struct clk *) (cl + 1);
> > > > +
> > > > +		rate = of_get_property(node, "clock-frequency", NULL);
> > > > +		if (!rate) {
> > > > +			kfree(cl);
> > > >  			continue;
> > > > +		}
> > > > +		clk->rate = be32_to_cpu(*rate);
> > > > +		clk->get_rate = get_fixed_clk_rate;
> > > > +
> > > > +		node->data = clk;
> > > >  
> > > > -		clk = clk_get_sys(id, NULL);
> > > > -		if (IS_ERR(clk))
> > > > +		cl->dev_id = of_get_property(node, "clock-outputs", NULL);
> > > > +		cl->con_id = of_get_property(node, "clock-alias", NULL);
> > > 
> > > As discussed briefly earlier, clock-alias looks like it is encoding
> > > Linux-specific implementation details into the device tree, and it
> > > shouldn't be necessary when explicit links to clock providers are
> > > supplied in the device tree.
> > > 
> > > > +		if (!cl->dev_id && !cl->con_id) {
> > > > +			kfree(cl);
> > > >  			continue;
> > > > +		}
> > > > +		cl->clk = clk;
> > > > +		clkdev_add(cl);
> > > > +	}
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static struct clk *mx5_prop_name_to_clk(struct device_node *node,
> > > > +		const char *prop_name)
> > > > +{
> > > > +	struct device_node *provnode;
> > > > +	struct clk *clk;
> > > > +	const void *prop;
> > > > +	u32 provhandle;
> > > > +
> > > > +	prop = of_get_property(node, prop_name, NULL);
> > > > +	if (!prop)
> > > > +		return NULL;
> > > > +	provhandle = be32_to_cpup(prop);
> > > > +
> > > > +	provnode = of_find_node_by_phandle(provhandle);
> > > > +	if (!provnode)
> > > > +		return NULL;
> > > > +
> > > > +	clk = provnode->data;
> > > > +
> > > > +	of_node_put(provnode);
> > > > +
> > > > +	return clk;
> > > > +}
> > > > +
> > > > +static inline struct clk *mx5_get_source_clk(struct device_node *node)
> > > > +{
> > > > +	return mx5_prop_name_to_clk(node, "clock-source");
> > > > +}
> > > > +
> > > > +static inline struct clk *mx5_get_depend_clk(struct device_node *node)
> > > > +{
> > > > +	return mx5_prop_name_to_clk(node, "clock-depend");
> > > > +}
> > > 
> > > Ditto here.  'clock-depend' seems to be Linux specifc.  I need to look
> > > at the usage model for these properties.
> > > 
> > This is not Linux but hardware specific.  Let's look at the eSDHC1
> > example below.
> > 
> >                 esdhc1_clk: esdhc@0 {
> >                         compatible = "fsl,mxc-clock";
> >                         reg = <0>;
> >                         clock-outputs = "sdhci-esdhc-imx.0";
> >                         clock-source = <&pll2_sw_clk>;
> >                         clock-depend = <&esdhc1_ipg_clk>;
> >                 };
> > 
> > 
> > We have esdhc1_clk added to clkdev standing for the clock for hardware
> > block eSDHC1.  This clock is actually the serial clock of eSDHC1,
> > while eSDHC1 internal working clock esdhc1_ipg_clk has also to be on
> > to get the block functional.
> 
> Actually, part of what I think is throwing me off here is that this
> node is only using half the clock binding.  A single node can be both
> a clock provider and a clock consumer, which will often be the case
> for clock controllers like this.  So in this case, it is using the
> correct "clock-outputs" property to declare the clocks that it
> provides, but it isn't using the *-clock binding to reference the
> clocks that it needs.  This really should be something like:
> 
>         esdhc1_clk: esdhc@0 {
>                 compatible = "fsl,mxc-clock";
>                 reg = <0>;
>                 clock-outputs = "sdhci-esdhc-imx.0";
>                 src-clock = <&pll2_sw_clk>, "sw-clk";
>                 ipg-clock = <&esdhc1_ipg_clk>, "ipg-clk";

The name 'ipg-clock' is too specific to be a property naming which
should be generic.  So I would have something like:

                  src-clock = <&pll2_sw_clk>, "sw-clk";
                  dep-clock = <&esdhc1_ipg_clk>, "ipg-clk";

>         };
> 
> Also remember that a single clock node can provide multiple clock
> outputs.  I don't know if this is a factor for imx51, but if it is

I do not see this is a factor for imx51 so far.