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

[Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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.