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....@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.

-- 
Regards,
Shawn

> 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).
> 


_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Reply via email to