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

Message ID 1299514932-13558-2-git-send-email-shawn.guo@linaro.org
State New
Headers show

Commit Message

Shawn Guo March 7, 2011, 4:22 p.m.
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.

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

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 arch/arm/boot/dts/babbage.dts |  162 +++++++++++++++++++++++++++++++++++++++--
 1 files changed, 156 insertions(+), 6 deletions(-)

Comments

Grant Likely March 7, 2011, 5:48 p.m. | #1
On Mon, Mar 7, 2011 at 9:22 AM, Shawn Guo <shawn.guo@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.

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

>
> Signed-off-by: Shawn Guo <shawn.guo@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@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.

> +                       reg = <0>;
> +                       clock-outputs = "pll1_main";
> +                       clock-source = <&osc_clk>;
> +               };
> +
> +               pll1_sw_clk: pll_switch@0 {
> +                       compatible = "clock";
> +                       reg = <0>;
> +                       clock-outputs = "pll1_sw";
> +                       clock-source = <&pll1_main_clk>;
> +               };
> +
> +               pll2_sw_clk: pll_switch@1 {
> +                       compatible = "clock";
> +                       reg = <1>;
> +                       clock-outputs = "pll2_sw";
> +                       clock-source = <&osc_clk>;
> +               };
> +
> +               pll3_sw_clk: pll_switch@2 {
> +                       compatible = "clock";
> +                       reg = <2>;
> +                       clock-outputs = "pll3_sw";
> +                       clock-source = <&osc_clk>;
> +               };
> +
> +               lp_apm_clk: lp_apm {
> +                       compatible = "clock";
> +                       clock-outputs = "lp_apm";
> +                       clock-source = <&osc_clk>;
> +               };
> +
> +               main_bus_clk: main_bus {
> +                       compatible = "clock";
> +                       clock-outputs = "main_bus";
> +                       clock-source = <&pll2_sw_clk>;
> +               };
> +
> +               ahb_clk: ahb {
> +                       compatible = "clock";
> +                       clock-outputs = "ahb";
> +                       clock-source = <&main_bus_clk>;
> +               };
> +
> +               ipg_clk: ipg {
> +                       compatible = "clock";
> +                       clock-outputs = "ipg";
> +                       clock-source = <&ahb_clk>;
> +               };
> +
> +               spba_clk: spba {
> +                       compatible = "clock";
> +                       clock-outputs = "spba";
> +                       clock-source = <&ipg_clk>;
> +               };
> +
> +               ahb_max_clk: ahb_max {
> +                       compatible = "clock";
> +                       clock-outputs = "ahb_max";
> +                       clock-source = <&ahb_clk>;
> +               };
> +
> +               aips_tz1_clk: aips_tz@0 {
> +                       compatible = "clock";
> +                       reg = <0>;
> +                       clock-outputs = "aips_tz1";
> +                       clock-source = <&ahb_clk>;
> +                       clock-depend = <&ahb_max_clk>;
> +               };
> +
> +               aips_tz2_clk: aips_tz@1 {
> +                       compatible = "clock";
> +                       reg = <1>;
> +                       clock-outputs = "aips_tz2";
> +                       clock-source = <&ahb_clk>;
> +                       clock-depend = <&ahb_max_clk>;
> +               };
> +
> +               gpt_ipg_clk: gpt_ipg {
> +                       compatible = "clock";
> +                       clock-outputs = "gpt_ipg";
> +                       clock-source = <&ipg_clk>;
> +               };
> +
> +               gpt_clk: gpt {
> +                       compatible = "clock";
> +                       clock-outputs = "gpt";
> +                       clock-source = <&ipg_clk>;
> +                       clock-depend = <&gpt_ipg_clk>;
> +               };
> +
> +               uart1_ipg_clk: uart_ipg@0 {
>                        compatible = "clock";
> +                       reg = <0>;
> +                       clock-outputs = "uart1_ipg";
> +                       clock-source = <&ipg_clk>;
> +                       clock-depend = <&aips_tz1_clk>;
> +               };
> +
> +               uart2_ipg_clk: uart_ipg@1 {
> +                       compatible = "clock";
> +                       reg = <1>;
> +                       clock-outputs = "uart2_ipg";
> +                       clock-source = <&ipg_clk>;
> +                       clock-depend = <&aips_tz1_clk>;
> +               };
> +
> +               uart3_ipg_clk: uart_ipg@2 {
> +                       compatible = "clock";
> +                       reg = <2>;
> +                       clock-outputs = "uart3_ipg";
> +                       clock-source = <&ipg_clk>;
> +                       clock-depend = <&spba_clk>;
> +               };
> +
> +               uart_root_clk: uart_root {
> +                       compatible = "clock";
> +                       clock-outputs = "uart_root";
> +                       clock-source = <&pll2_sw_clk>;
> +               };
> +
> +               uart1_clk: uart@0 {
> +                       compatible = "clock";
> +                       reg = <0>;
>                        clock-outputs = "imx-uart.0";
> +                       clock-source = <&uart_root_clk>;
>                };
>
> -               uart1_clk: uart@1 {
> +               uart2_clk: uart@1 {
>                        compatible = "clock";
> +                       reg = <1>;
>                        clock-outputs = "imx-uart.1";
> +                       clock-source = <&uart_root_clk>;
>                };
>
> -               uart2_clk: uart@2 {
> +               uart3_clk: uart@2 {
>                        compatible = "clock";
> +                       reg = <2>;
>                        clock-outputs = "imx-uart.2";
> +                       clock-source = <&uart_root_clk>;
>                };
>
>                fec_clk: fec@0 {
> @@ -67,7 +217,7 @@
>                        reg = <0xc000 0x1000>;
>                        interrupts = <0x21>;
>                        rts-cts;
> -                       uart-clock = <&uart2_clk>, "uart";
> +                       uart-clock = <&uart3_clk>, "uart";
>                };
>        };
>
> @@ -82,7 +232,7 @@
>                        reg = <0xbc000 0x1000>;
>                        interrupts = <0x1f>;
>                        rts-cts;
> -                       uart-clock = <&uart0_clk>, "uart";
> +                       uart-clock = <&uart1_clk>, "uart";
>                };
>
>                imx-uart@c0000 {
> @@ -90,7 +240,7 @@
>                        reg = <0xc0000 0x1000>;
>                        interrupts = <0x20>;
>                        rts-cts;
> -                       uart-clock = <&uart1_clk>, "uart";
> +                       uart-clock = <&uart2_clk>, "uart";
>                };
>        };
>
> --
> 1.7.1
>
>
> _______________________________________________
> linaro-dev mailing list
> linaro-dev@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev
>
Shawn Guo March 8, 2011, 3:44 a.m. | #2
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@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@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@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.
Jason Hui March 8, 2011, 6:56 a.m. | #3
Hi, Shawn,

On Tue, Mar 8, 2011 at 12:22 AM, Shawn Guo <shawn.guo@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.
>
> * 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.
>
> Signed-off-by: Shawn Guo <shawn.guo@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@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";
> +                       reg = <0>;
> +                       clock-outputs = "pll1_main";
> +                       clock-source = <&osc_clk>;
> +               };
> +
> +               pll1_sw_clk: pll_switch@0 {
> +                       compatible = "clock";
> +                       reg = <0>;
> +                       clock-outputs = "pll1_sw";
> +                       clock-source = <&pll1_main_clk>;
> +               };
> +
> +               pll2_sw_clk: pll_switch@1 {
> +                       compatible = "clock";
> +                       reg = <1>;
> +                       clock-outputs = "pll2_sw";
> +                       clock-source = <&osc_clk>;
> +               };
>

It seems that you mis-used the reg property, it need fixed globally.

BR,
Jason

>
> --
> 1.7.1
>
>
> _______________________________________________
> linaro-dev mailing list
> linaro-dev@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev
>
Shawn Guo March 8, 2011, 7:07 a.m. | #4
On Tue, Mar 08, 2011 at 02:56:52PM +0800, Jason Hui wrote:
> Hi, Shawn,
> 
> On Tue, Mar 8, 2011 at 12:22 AM, Shawn Guo <shawn.guo@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.
> >
> > * 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.
> >
> > Signed-off-by: Shawn Guo <shawn.guo@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@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";
> > +                       reg = <0>;
> > +                       clock-outputs = "pll1_main";
> > +                       clock-source = <&osc_clk>;
> > +               };
> > +
> > +               pll1_sw_clk: pll_switch@0 {
> > +                       compatible = "clock";
> > +                       reg = <0>;
> > +                       clock-outputs = "pll1_sw";
> > +                       clock-source = <&pll1_main_clk>;
> > +               };
> > +
> > +               pll2_sw_clk: pll_switch@1 {
> > +                       compatible = "clock";
> > +                       reg = <1>;
> > +                       clock-outputs = "pll2_sw";
> > +                       clock-source = <&osc_clk>;
> > +               };
> >
> 
> It seems that you mis-used the reg property, it need fixed globally.
> 
I guessed it out from Grant's comment on your babbage.dts as below.

--- quote begin ---
>> +               uart_clk0: uart@0 {

@0 should only be specified if the node has a 'reg = <0>' property.
In this case it doesn't so either 'reg' should be added, or '@0'
should be removed.
--- quote end ---
Jason Hui March 8, 2011, 7:27 a.m. | #5
Hi, Shawn,

On Tue, Mar 8, 2011 at 3:07 PM, Shawn Guo <shawn.guo@freescale.com> wrote:
> On Tue, Mar 08, 2011 at 02:56:52PM +0800, Jason Hui wrote:
>> Hi, Shawn,
>>
>> On Tue, Mar 8, 2011 at 12:22 AM, Shawn Guo <shawn.guo@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.
>> >
>> > * 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.
>> >
>> > Signed-off-by: Shawn Guo <shawn.guo@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@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";
>> > +                       reg = <0>;
>> > +                       clock-outputs = "pll1_main";
>> > +                       clock-source = <&osc_clk>;
>> > +               };
>> > +
>> > +               pll1_sw_clk: pll_switch@0 {
>> > +                       compatible = "clock";
>> > +                       reg = <0>;
>> > +                       clock-outputs = "pll1_sw";
>> > +                       clock-source = <&pll1_main_clk>;
>> > +               };
>> > +
>> > +               pll2_sw_clk: pll_switch@1 {
>> > +                       compatible = "clock";
>> > +                       reg = <1>;
>> > +                       clock-outputs = "pll2_sw";
>> > +                       clock-source = <&osc_clk>;
>> > +               };
>> >
>>
>> It seems that you mis-used the reg property, it need fixed globally.
>>
> I guessed it out from Grant's comment on your babbage.dts as below.

I don't understand clearly. Can we have the this usage, grant? reg=<1>
in this case, it will  be
decoded as clk->id finally.

pll2_sw_clk: pll_switch@1 {
>> > +                       compatible = "clock";
>> > +                       reg = <1>;
>> > +                       clock-outputs = "pll2_sw";
>> > +                       clock-source = <&osc_clk>;
>> > +               };

I just want to raise the problems. According to ePAPR,
2.3.6 reg
Property: reg
Value type: <prop-encoded-array> encoded as arbitrary number of
(address,length) pairs.
Description:
The reg property describes the address and length of a device’s memory
mapped register
space within its parent’s address space. The value is a
<prop-encoded-array>, composed of
an arbitrary number of pairs of address and length, <address, length>.
The number of <u32> cells required to specify the address and length
are bus-specific and are
specified by the #address-cells and #size-cells properties in the
parent of the device node. If
the parent node specifies a value of 0 for #size-cells, the length
field in the value of reg shall
be omitted.
Example:
Suppose a device within a system-on-a-chip had two blocks of
registers—a 32-byte block at
offset 0x3000 in the SOC and a 256-byte block at offset 0xFE00. The
reg property would be
encoded as follows (assuming #address-cells and #size-cells values of 1):
reg = <0x3000 0x20 0xFE00 0x100>;

> --- quote end ---
>
> --
> Regards,
> Shawn
>
>
Shawn Guo March 12, 2011, 5:55 a.m. | #6
On Tue, Mar 08, 2011 at 11:44:48AM +0800, Shawn Guo wrote:
> 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@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.
> 
[...]
> > > +               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.
> 
I'm going to use 'fsl,mxc-clock', as the 'clk' is currently defined
under plat-mxc.  Let me know if anyone is uncomfortable with it.
Shawn Guo March 13, 2011, 8:08 a.m. | #7
On Tue, Mar 08, 2011 at 11:44:48AM +0800, Shawn Guo wrote:
> 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@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?
> 
Sorry.  The argument above is invalid, as neither dev_id nor con_id
will be used to find the 'clk' when DT clock code applies.  So, yes,
property clock-alias is not needed.

Patch

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@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";
+			reg = <0>;
+			clock-outputs = "pll1_main";
+			clock-source = <&osc_clk>;
+		};
+
+		pll1_sw_clk: pll_switch@0 {
+			compatible = "clock";
+			reg = <0>;
+			clock-outputs = "pll1_sw";
+			clock-source = <&pll1_main_clk>;
+		};
+
+		pll2_sw_clk: pll_switch@1 {
+			compatible = "clock";
+			reg = <1>;
+			clock-outputs = "pll2_sw";
+			clock-source = <&osc_clk>;
+		};
+
+		pll3_sw_clk: pll_switch@2 {
+			compatible = "clock";
+			reg = <2>;
+			clock-outputs = "pll3_sw";
+			clock-source = <&osc_clk>;
+		};
+
+		lp_apm_clk: lp_apm {
+			compatible = "clock";
+			clock-outputs = "lp_apm";
+			clock-source = <&osc_clk>;
+		};
+
+		main_bus_clk: main_bus {
+			compatible = "clock";
+			clock-outputs = "main_bus";
+			clock-source = <&pll2_sw_clk>;
+		};
+
+		ahb_clk: ahb {
+			compatible = "clock";
+			clock-outputs = "ahb";
+			clock-source = <&main_bus_clk>;
+		};
+
+		ipg_clk: ipg {
+			compatible = "clock";
+			clock-outputs = "ipg";
+			clock-source = <&ahb_clk>;
+		};
+
+		spba_clk: spba {
+			compatible = "clock";
+			clock-outputs = "spba";
+			clock-source = <&ipg_clk>;
+		};
+
+		ahb_max_clk: ahb_max {
+			compatible = "clock";
+			clock-outputs = "ahb_max";
+			clock-source = <&ahb_clk>;
+		};
+
+		aips_tz1_clk: aips_tz@0 {
+			compatible = "clock";
+			reg = <0>;
+			clock-outputs = "aips_tz1";
+			clock-source = <&ahb_clk>;
+			clock-depend = <&ahb_max_clk>;
+		};
+
+		aips_tz2_clk: aips_tz@1 {
+			compatible = "clock";
+			reg = <1>;
+			clock-outputs = "aips_tz2";
+			clock-source = <&ahb_clk>;
+			clock-depend = <&ahb_max_clk>;
+		};
+
+		gpt_ipg_clk: gpt_ipg {
+			compatible = "clock";
+			clock-outputs = "gpt_ipg";
+			clock-source = <&ipg_clk>;
+		};
+
+		gpt_clk: gpt {
+			compatible = "clock";
+			clock-outputs = "gpt";
+			clock-source = <&ipg_clk>;
+			clock-depend = <&gpt_ipg_clk>;
+		};
+
+		uart1_ipg_clk: uart_ipg@0 {
 			compatible = "clock";
+			reg = <0>;
+			clock-outputs = "uart1_ipg";
+			clock-source = <&ipg_clk>;
+			clock-depend = <&aips_tz1_clk>;
+		};
+
+		uart2_ipg_clk: uart_ipg@1 {
+			compatible = "clock";
+			reg = <1>;
+			clock-outputs = "uart2_ipg";
+			clock-source = <&ipg_clk>;
+			clock-depend = <&aips_tz1_clk>;
+		};
+
+		uart3_ipg_clk: uart_ipg@2 {
+			compatible = "clock";
+			reg = <2>;
+			clock-outputs = "uart3_ipg";
+			clock-source = <&ipg_clk>;
+			clock-depend = <&spba_clk>;
+		};
+
+		uart_root_clk: uart_root {
+			compatible = "clock";
+			clock-outputs = "uart_root";
+			clock-source = <&pll2_sw_clk>;
+		};
+
+		uart1_clk: uart@0 {
+			compatible = "clock";
+			reg = <0>;
 			clock-outputs = "imx-uart.0";
+			clock-source = <&uart_root_clk>;
 		};
 
-		uart1_clk: uart@1 {
+		uart2_clk: uart@1 {
 			compatible = "clock";
+			reg = <1>;
 			clock-outputs = "imx-uart.1";
+			clock-source = <&uart_root_clk>;
 		};
 
-		uart2_clk: uart@2 {
+		uart3_clk: uart@2 {
 			compatible = "clock";
+			reg = <2>;
 			clock-outputs = "imx-uart.2";
+			clock-source = <&uart_root_clk>;
 		};
 
 		fec_clk: fec@0 {
@@ -67,7 +217,7 @@ 
 			reg = <0xc000 0x1000>;
 			interrupts = <0x21>;
 			rts-cts;
-			uart-clock = <&uart2_clk>, "uart";
+			uart-clock = <&uart3_clk>, "uart";
 		};
 	};
 
@@ -82,7 +232,7 @@ 
 			reg = <0xbc000 0x1000>;
 			interrupts = <0x1f>;
 			rts-cts;
-			uart-clock = <&uart0_clk>, "uart";
+			uart-clock = <&uart1_clk>, "uart";
 		};
 
 		imx-uart@c0000 {
@@ -90,7 +240,7 @@ 
 			reg = <0xc0000 0x1000>;
 			interrupts = <0x20>;
 			rts-cts;
-			uart-clock = <&uart1_clk>, "uart";
+			uart-clock = <&uart2_clk>, "uart";
 		};
 	};