[PATCH 1/5] arm/dts: babbage: add gpt and uart related clock nodes

[Shawn Guo]

On Mon, Mar 07, 2011 at 10:48:10AM -0700, Grant Likely wrote:
> On Mon, Mar 7, 2011 at 9:22 AM, Shawn Guo <shawn.guo at linaro.org> wrote:
> > The patch is to add all gpt, uart related dt clock nodes for babbage.
> > It sticks to the clock name used in clock-mx51-mx53.c, so that
> > everything gets consistent to Reference Manual.  For example, the
> > numbering in clock name usually starts from 1, while 'reg' property
> > numbering starts from 0 to easy clock binding.
> >
> > Besides the generally used clock bindings, the following properties
> > are proposed in this patch.
> >
> > * clock-alias
> > Like clock-outputs to reflect cl->dev_id, property clock-alias is
> > defined to reflect cl->con_id.
> 
> This feels like leakage of Linux kernel implementation details getting
> encoded into the binding.  There shouldn't be any need for a clock
> alias property.  It should all just work to have multiple devices
> explicitly refer to the same clock node because the dt clock support
> patch gets first crack at resolving a struct clk pointer before clkdev
> does its lookup.
> 
This is to make clk_get() work.  Let's take a look at this example.
i.MX28 integrates a amba-pl011 uart controller, and there are two
places calling clk_get() with the same dev_id to get the different
'clk'.

static struct clk_lookup lookups[] = {
        /* for amba bus driver */
        _REGISTER_CLOCK("duart", "apb_pclk", xbus_clk)
        /* for amba-pl011 driver */
        _REGISTER_CLOCK("duart", NULL, uart_clk)
	...
};

* drivers/amba/bus.c - to get xbus_clk
static int amba_get_enable_pclk(struct amba_device *pcdev)
{
        struct clk *pclk = clk_get(&pcdev->dev, "apb_pclk");
        int ret;

        pcdev->pclk = pclk;

        if (IS_ERR(pclk))
                return PTR_ERR(pclk);

        ret = clk_enable(pclk);
        if (ret)
                clk_put(pclk);

        return ret;
}

* drivers/tty/serial/amba-pl011.c - to get uart_clk
static int pl011_probe(struct amba_device *dev, struct amba_id *id)
{
	...

        uap->clk = clk_get(&dev->dev, NULL);
        if (IS_ERR(uap->clk)) {
                ret = PTR_ERR(uap->clk);
                goto unmap;
        }

	...
}

Will this be broken if we do not have an alias in dt clock to reflect
con_id?

> >
> > * clock-depend
> > The mxc 'struct clk' has the member 'secondary' to refer to the clock
> > that the 'clk' has dependency on.  This 'secondary' clock needs to be
> > on whenever the 'clk' is set to on.  This clock-depend property is
> > defined to reflect this 'secondary' clock.
> 
> This is fine, but it is a Freescale specific binding.  Each of the
> imx51 clock nodes should have compatible set to something like
> "fsl,imx51-clock" so that the OS can know that it should be using this
> binding.
> 
I doubt this is Freescale clock only use case.  But I will do what you
suggest here anyway.

Oh, I forgot another new property, clock-source, which is to reflect
the parent clock.  This should be very common one, but not sure if
the naming is proper. The naming 'clock-provider' should not be the
one, I guess.

> >
> > Signed-off-by: Shawn Guo <shawn.guo at linaro.org>
> > ---
> >  arch/arm/boot/dts/babbage.dts |  162 +++++++++++++++++++++++++++++++++++++++--
> >  1 files changed, 156 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/babbage.dts b/arch/arm/boot/dts/babbage.dts
> > index 46a3071..1774cec 100644
> > --- a/arch/arm/boot/dts/babbage.dts
> > +++ b/arch/arm/boot/dts/babbage.dts
> > @@ -35,19 +35,169 @@
> >                #address-cells = <1>;
> >                #size-cells = <0>;
> >
> > -               uart0_clk: uart at 0 {
> > +               ckil_clk: clkil {
> > +                       compatible = "fixed-clock";
> > +                       #frequency-cells = <1>;
> > +                       clock-outputs = "clil";
> > +                       clock-frequency = <32768>;
> > +               };
> > +
> > +               ckih_clk: ckih {
> > +                       compatible = "fixed-clock";
> > +                       #frequency-cells = <1>;
> > +                       clock-outputs = "ckih";
> > +                       clock-frequency = <22579200>;
> > +               };
> > +
> > +               osc_clk: soc {
> > +                       compatible = "fixed-clock";
> > +                       #frequency-cells = <1>;
> > +                       clock-outputs = "osc";
> > +                       clock-frequency = <24000000>;
> > +               };
> > +
> > +               pll1_main_clk: pll1_main {
> > +                       compatible = "clock";
> 
> As hinted on above, "clock" doesn't look like a good compatible
> property.  It should specify the specific implementation of a clock
> device.  ie. "fsl,imx51-clock".  Even that example may be too generic
> if there are multiple types of clock controllers in the imx51 SoC.
> 
We are implementing clock-mx51-mx53.c.  Would it be better to use
'fsl,mx5-clock'?  Or, we will have to scan 'fsl,imx51-clock' and
'fsl,imx53-clock'.  Oh, i.MX50 is also coming.

-- 
Regards,
Shawn