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.
--
Regards,
Shawn
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
