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