[PATCH 3/5] arm/dt: mx51: dynamically add gpt and uart related clocks per dt nodes
Shawn Guo
shawn.guo at freescale.com
Sat Mar 19 03:35:39 EST 2011
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 at 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 at 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 at 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).
>
More information about the devicetree-discuss
mailing list