[PATCH RFC 0/5] Dynamically add clk to clkdev per dt clock nodes

[PATCH RFC 0/5] Dynamically add clk to clkdev per dt clock nodes

[Shawn Guo]

Before translating all mx5 clocks from static adding way to dynamic
way, I would send this out for review and comment, so that I can turn
around in case that I'm on a wrong direction.

It's based on Jason's mx51-basic-dt-support patch set, and only adds
gpt and uart related clocks, but it's enough to get the system at
where Jason's patch can get.

 [PATCH 1/5] arm/dts: babbage: add gpt and uart related clock nodes
 [PATCH 2/5] arm/mxc: add clk members to ease dt clock support
 [PATCH 3/5] arm/dt: mx51: dynamically add gpt and uart related clocks per dt nodes
 [PATCH 4/5] arm/dt: mx5: change timer init function to dt clock way
 [PATCH 5/5] of/clock: eliminate function __of_clk_get_from_provider

 arch/arm/boot/dts/babbage.dts          |  162 ++++++++++++-
 arch/arm/mach-mx5/board-dt.c           |    9 +-
 arch/arm/mach-mx5/clock-mx51-mx53.c    |  436 +++++++++++++++++++++++++++++++-
 arch/arm/plat-mxc/include/mach/clock.h |    4 +
 drivers/of/clock.c                     |   23 +--
 drivers/tty/serial/imx.c               |   79 +++++-
 6 files changed, 661 insertions(+), 52 deletions(-)

Regards,
Shawn

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

[Shawn Guo]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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>;
+		};
+
+		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

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

[Grant Likely]

On Mon, Mar 7, 2011 at 9:22 AM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-cunTk1MwBs8s++Sfvej+rw@public.gmane.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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.

-- 
Regards,
Shawn

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

[Shawn Guo]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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.

-- 
Regards,
Shawn

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

[Shawn Guo]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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.

-- 
Regards,
Shawn

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

[Jason Hui]

Hi, Shawn,

On Tue, Mar 8, 2011 at 12:22 AM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-cunTk1MwBs8s++Sfvej+rw@public.gmane.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev
>

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

[Shawn Guo]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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 ---

-- 
Regards,
Shawn

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

[Jason Hui]

Hi, Shawn,

On Tue, Mar 8, 2011 at 3:07 PM, Shawn Guo <shawn.guo-KZfg59tc24xl57MIdRCFDg@public.gmane.org> 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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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
>
>

[PATCH 2/5] arm/mxc: add clk members to ease dt clock support

[Shawn Guo]

The 'rate' is added for fixed-clock support, while 'pll_base' is for
pll clock.  These two particular type of clocks are supposed to be
gracefully supported by the common clk api when it gets ready.

Signed-off-by: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm/plat-mxc/include/mach/clock.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-mxc/include/mach/clock.h b/arch/arm/plat-mxc/include/mach/clock.h
index 753a598..a29dc45 100644
--- a/arch/arm/plat-mxc/include/mach/clock.h
+++ b/arch/arm/plat-mxc/include/mach/clock.h
@@ -38,6 +38,10 @@ struct clk {
 	/* Register address for clock's enable/disable control. */
 	void __iomem *enable_reg;
 	u32 flags;
+	/* clock rate used by fixed-clock */
+	unsigned long rate;
+	/* base address of pll */
+	void __iomem *pll_base;
 	/* get the current clock rate (always a fresh value) */
 	unsigned long (*get_rate) (struct clk *);
 	/* Function ptr to set the clock to a new rate. The rate must match a
-- 
1.7.1

Re: [PATCH 2/5] arm/mxc: add clk members to ease dt clock support

[Grant Likely]

On Mon, Mar 7, 2011 at 9:22 AM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> The 'rate' is added for fixed-clock support, while 'pll_base' is for
> pll clock.  These two particular type of clocks are supposed to be
> gracefully supported by the common clk api when it gets ready.

How does the current imx clock code handle fixed and pll clocks?
Using the dt shouldn't require any special treatment in this regard.

g.

>
> Signed-off-by: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  arch/arm/plat-mxc/include/mach/clock.h |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/plat-mxc/include/mach/clock.h b/arch/arm/plat-mxc/include/mach/clock.h
> index 753a598..a29dc45 100644
> --- a/arch/arm/plat-mxc/include/mach/clock.h
> +++ b/arch/arm/plat-mxc/include/mach/clock.h
> @@ -38,6 +38,10 @@ struct clk {
>        /* Register address for clock's enable/disable control. */
>        void __iomem *enable_reg;
>        u32 flags;
> +       /* clock rate used by fixed-clock */
> +       unsigned long rate;
> +       /* base address of pll */
> +       void __iomem *pll_base;
>        /* get the current clock rate (always a fresh value) */
>        unsigned long (*get_rate) (struct clk *);
>        /* Function ptr to set the clock to a new rate. The rate must match a
> --
> 1.7.1
>
>
> _______________________________________________
> linaro-dev mailing list
> linaro-dev-cunTk1MwBs8s++Sfvej+rw@public.gmane.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 2/5] arm/mxc: add clk members to ease dt clock support

[Shawn Guo]

On Mon, Mar 07, 2011 at 10:53:37AM -0700, Grant Likely wrote:
> On Mon, Mar 7, 2011 at 9:22 AM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > The 'rate' is added for fixed-clock support, while 'pll_base' is for
> > pll clock.  These two particular type of clocks are supposed to be
> > gracefully supported by the common clk api when it gets ready.
> 
> How does the current imx clock code handle fixed and pll clocks?

For fixed-clock, the current code gets several variables holding the
rate and then return the rate from several get_rate functions.

static unsigned long external_high_reference, external_low_reference;
static unsigned long oscillator_reference, ckih2_reference;

static unsigned long get_high_reference_clock_rate(struct clk *clk)
{
        return external_high_reference;
}

static unsigned long get_low_reference_clock_rate(struct clk *clk)
{
        return external_low_reference;
}

static unsigned long get_oscillator_reference_clock_rate(struct clk *clk)
{
        return oscillator_reference;
}

static unsigned long get_ckih2_reference_clock_rate(struct clk *clk)
{
        return ckih2_reference;
}

With this new rate member added, all these can be consolidated into one.

For base address of pll, the current code uses the reference to clocks
statically defined to know which pll is the one.

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)
                return MX51_DPLL2_BASE;
        else if (pll == &pll3_sw_clk)
                return MX51_DPLL3_BASE;
        else
                BUG();

        return NULL;
#endif
}

static inline void __iomem *_get_pll_base(struct clk *pll)
{
        if (cpu_is_mx51())
                return _mx51_get_pll_base(pll);
        else
                return _mx53_get_pll_base(pll);
}

> Using the dt shouldn't require any special treatment in this regard.
> 
I would say these two members were added to make dt clock code simple
and good.

-- 
Regards,
Shawn

Re: [PATCH 2/5] arm/mxc: add clk members to ease dt clock support

[Shawn Guo]

On Tue, Mar 08, 2011 at 11:56:34AM +0800, Shawn Guo wrote:
> On Mon, Mar 07, 2011 at 10:53:37AM -0700, Grant Likely wrote:
> > On Mon, Mar 7, 2011 at 9:22 AM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > > The 'rate' is added for fixed-clock support, while 'pll_base' is for
> > > pll clock.  These two particular type of clocks are supposed to be
> > > gracefully supported by the common clk api when it gets ready.
> > 
> > How does the current imx clock code handle fixed and pll clocks?
> 
> For fixed-clock, the current code gets several variables holding the
> rate and then return the rate from several get_rate functions.
> 
> static unsigned long external_high_reference, external_low_reference;
> static unsigned long oscillator_reference, ckih2_reference;
> 
> static unsigned long get_high_reference_clock_rate(struct clk *clk)
> {
>         return external_high_reference;
> }
> 
> static unsigned long get_low_reference_clock_rate(struct clk *clk)
> {
>         return external_low_reference;
> }
> 
> static unsigned long get_oscillator_reference_clock_rate(struct clk *clk)
> {
>         return oscillator_reference;
> }
> 
> static unsigned long get_ckih2_reference_clock_rate(struct clk *clk)
> {
>         return ckih2_reference;
> }
> 
> With this new rate member added, all these can be consolidated into one.
> 
> For base address of pll, the current code uses the reference to clocks
> statically defined to know which pll is the one.
> 
I just noticed that the references to clocks statically created are 
being used in the current clock code widely, and I need to work around
it 'globally' anyway, so I will not add the new member 'pll_base'.

> 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)
>                 return MX51_DPLL2_BASE;
>         else if (pll == &pll3_sw_clk)
>                 return MX51_DPLL3_BASE;
>         else
>                 BUG();
> 
>         return NULL;
> #endif
> }
> 
> static inline void __iomem *_get_pll_base(struct clk *pll)
> {
>         if (cpu_is_mx51())
>                 return _mx51_get_pll_base(pll);
>         else
>                 return _mx53_get_pll_base(pll);
> }
> 
> > Using the dt shouldn't require any special treatment in this regard.
> > 
> I would say these two members were added to make dt clock code simple
> and good.

-- 
Regards,
Shawn

Re: [PATCH 2/5] arm/mxc: add clk members to ease dt clock support

[Grant Likely]

On Tue, Mar 08, 2011 at 11:56:34AM +0800, Shawn Guo wrote:
> On Mon, Mar 07, 2011 at 10:53:37AM -0700, Grant Likely wrote:
> > On Mon, Mar 7, 2011 at 9:22 AM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > > The 'rate' is added for fixed-clock support, while 'pll_base' is for
> > > pll clock.  These two particular type of clocks are supposed to be
> > > gracefully supported by the common clk api when it gets ready.
> > 
> > How does the current imx clock code handle fixed and pll clocks?
> 
> For fixed-clock, the current code gets several variables holding the
> rate and then return the rate from several get_rate functions.
> 
> static unsigned long external_high_reference, external_low_reference;
> static unsigned long oscillator_reference, ckih2_reference;
> 
> static unsigned long get_high_reference_clock_rate(struct clk *clk)
> {
>         return external_high_reference;
> }
> 
> static unsigned long get_low_reference_clock_rate(struct clk *clk)
> {
>         return external_low_reference;
> }
> 
> static unsigned long get_oscillator_reference_clock_rate(struct clk *clk)
> {
>         return oscillator_reference;
> }
> 
> static unsigned long get_ckih2_reference_clock_rate(struct clk *clk)
> {
>         return ckih2_reference;
> }
> 
> With this new rate member added, all these can be consolidated into one.
> 
> For base address of pll, the current code uses the reference to clocks
> statically defined to know which pll is the one.
> 
> 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)
>                 return MX51_DPLL2_BASE;
>         else if (pll == &pll3_sw_clk)
>                 return MX51_DPLL3_BASE;
>         else
>                 BUG();
> 
>         return NULL;
> #endif

Be careful about stuff like this.  Remember that enabling CONFIG_OF
must *not break* board support that does not use the device tree.  The
above #ifdef block will break existing users.

g.

Re: [PATCH 2/5] arm/mxc: add clk members to ease dt clock support

[Shawn Guo]

On Tue, Mar 15, 2011 at 01:41:01AM -0600, Grant Likely wrote:
> On Tue, Mar 08, 2011 at 11:56:34AM +0800, Shawn Guo wrote:
> > On Mon, Mar 07, 2011 at 10:53:37AM -0700, Grant Likely wrote:
> > > On Mon, Mar 7, 2011 at 9:22 AM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > > > The 'rate' is added for fixed-clock support, while 'pll_base' is for
> > > > pll clock.  These two particular type of clocks are supposed to be
> > > > gracefully supported by the common clk api when it gets ready.
> > > 
> > > How does the current imx clock code handle fixed and pll clocks?
> > 
> > For fixed-clock, the current code gets several variables holding the
> > rate and then return the rate from several get_rate functions.
> > 
> > static unsigned long external_high_reference, external_low_reference;
> > static unsigned long oscillator_reference, ckih2_reference;
> > 
> > static unsigned long get_high_reference_clock_rate(struct clk *clk)
> > {
> >         return external_high_reference;
> > }
> > 
> > static unsigned long get_low_reference_clock_rate(struct clk *clk)
> > {
> >         return external_low_reference;
> > }
> > 
> > static unsigned long get_oscillator_reference_clock_rate(struct clk *clk)
> > {
> >         return oscillator_reference;
> > }
> > 
> > static unsigned long get_ckih2_reference_clock_rate(struct clk *clk)
> > {
> >         return ckih2_reference;
> > }
> > 
> > With this new rate member added, all these can be consolidated into one.
> > 
> > For base address of pll, the current code uses the reference to clocks
> > statically defined to know which pll is the one.
> > 
> > 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)
> >                 return MX51_DPLL2_BASE;
> >         else if (pll == &pll3_sw_clk)
> >                 return MX51_DPLL3_BASE;
> >         else
> >                 BUG();
> > 
> >         return NULL;
> > #endif
> 
> Be careful about stuff like this.  Remember that enabling CONFIG_OF
> must *not break* board support that does not use the device tree.  The
> above #ifdef block will break existing users.
> 
Though the code has been killed in the latest version I just sent
yesterday I sent last night, I do not understand how it will break
the existing users.  The existing code is:

static inline void __iomem *_mx51_get_pll_base(struct clk *pll)
{
        if (pll == &pll1_main_clk)
                return MX51_DPLL1_BASE;
        else if (pll == &pll2_sw_clk)
                return MX51_DPLL2_BASE;
        else if (pll == &pll3_sw_clk)
                return MX51_DPLL3_BASE;
        else
                BUG();

        return NULL;
}

-- 
Regards,
Shawn

Re: [PATCH 2/5] arm/mxc: add clk members to ease dt clock support

[Grant Likely]

On Tue, Mar 15, 2011 at 03:50:29PM +0800, Shawn Guo wrote:
> On Tue, Mar 15, 2011 at 01:41:01AM -0600, Grant Likely wrote:
> > On Tue, Mar 08, 2011 at 11:56:34AM +0800, Shawn Guo wrote:
> > > On Mon, Mar 07, 2011 at 10:53:37AM -0700, Grant Likely wrote:
> > > > On Mon, Mar 7, 2011 at 9:22 AM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > > > > The 'rate' is added for fixed-clock support, while 'pll_base' is for
> > > > > pll clock.  These two particular type of clocks are supposed to be
> > > > > gracefully supported by the common clk api when it gets ready.
> > > > 
> > > > How does the current imx clock code handle fixed and pll clocks?
> > > 
> > > For fixed-clock, the current code gets several variables holding the
> > > rate and then return the rate from several get_rate functions.
> > > 
> > > static unsigned long external_high_reference, external_low_reference;
> > > static unsigned long oscillator_reference, ckih2_reference;
> > > 
> > > static unsigned long get_high_reference_clock_rate(struct clk *clk)
> > > {
> > >         return external_high_reference;
> > > }
> > > 
> > > static unsigned long get_low_reference_clock_rate(struct clk *clk)
> > > {
> > >         return external_low_reference;
> > > }
> > > 
> > > static unsigned long get_oscillator_reference_clock_rate(struct clk *clk)
> > > {
> > >         return oscillator_reference;
> > > }
> > > 
> > > static unsigned long get_ckih2_reference_clock_rate(struct clk *clk)
> > > {
> > >         return ckih2_reference;
> > > }
> > > 
> > > With this new rate member added, all these can be consolidated into one.
> > > 
> > > For base address of pll, the current code uses the reference to clocks
> > > statically defined to know which pll is the one.
> > > 
> > > 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)
> > >                 return MX51_DPLL2_BASE;
> > >         else if (pll == &pll3_sw_clk)
> > >                 return MX51_DPLL3_BASE;
> > >         else
> > >                 BUG();
> > > 
> > >         return NULL;
> > > #endif
> > 
> > Be careful about stuff like this.  Remember that enabling CONFIG_OF
> > must *not break* board support that does not use the device tree.  The
> > above #ifdef block will break existing users.
> > 
> Though the code has been killed in the latest version I just sent
> yesterday I sent last night, I do not understand how it will break
> the existing users.  The existing code is:
> 
> static inline void __iomem *_mx51_get_pll_base(struct clk *pll)
> {
>         if (pll == &pll1_main_clk)
>                 return MX51_DPLL1_BASE;
>         else if (pll == &pll2_sw_clk)
>                 return MX51_DPLL2_BASE;
>         else if (pll == &pll3_sw_clk)
>                 return MX51_DPLL3_BASE;
>         else
>                 BUG();
> 
>         return NULL;
> }

What you wrote wrapped the current implementation with #ifdef
CONFIG_OF ... #else [existing code] #endif.  That says to me that when
CONFIG_OF is enabled, the old code gets compiled out, which means the
function no longer works on non-dt platforms.

The goal is to support both dt and non-dt machines with a single
kernel image.

g.

> 
> -- 
> Regards,
> Shawn
> 

Re: [PATCH 2/5] arm/mxc: add clk members to ease dt clock support

[Shawn Guo]

On Tue, Mar 15, 2011 at 02:02:56AM -0600, Grant Likely wrote:
> On Tue, Mar 15, 2011 at 03:50:29PM +0800, Shawn Guo wrote:
> > On Tue, Mar 15, 2011 at 01:41:01AM -0600, Grant Likely wrote:
> > > On Tue, Mar 08, 2011 at 11:56:34AM +0800, Shawn Guo wrote:
> > > > On Mon, Mar 07, 2011 at 10:53:37AM -0700, Grant Likely wrote:
> > > > > On Mon, Mar 7, 2011 at 9:22 AM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > > > > > The 'rate' is added for fixed-clock support, while 'pll_base' is for
> > > > > > pll clock.  These two particular type of clocks are supposed to be
> > > > > > gracefully supported by the common clk api when it gets ready.
> > > > > 
> > > > > How does the current imx clock code handle fixed and pll clocks?
> > > > 
> > > > For fixed-clock, the current code gets several variables holding the
> > > > rate and then return the rate from several get_rate functions.
> > > > 
> > > > static unsigned long external_high_reference, external_low_reference;
> > > > static unsigned long oscillator_reference, ckih2_reference;
> > > > 
> > > > static unsigned long get_high_reference_clock_rate(struct clk *clk)
> > > > {
> > > >         return external_high_reference;
> > > > }
> > > > 
> > > > static unsigned long get_low_reference_clock_rate(struct clk *clk)
> > > > {
> > > >         return external_low_reference;
> > > > }
> > > > 
> > > > static unsigned long get_oscillator_reference_clock_rate(struct clk *clk)
> > > > {
> > > >         return oscillator_reference;
> > > > }
> > > > 
> > > > static unsigned long get_ckih2_reference_clock_rate(struct clk *clk)
> > > > {
> > > >         return ckih2_reference;
> > > > }
> > > > 
> > > > With this new rate member added, all these can be consolidated into one.
> > > > 
> > > > For base address of pll, the current code uses the reference to clocks
> > > > statically defined to know which pll is the one.
> > > > 
> > > > 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)
> > > >                 return MX51_DPLL2_BASE;
> > > >         else if (pll == &pll3_sw_clk)
> > > >                 return MX51_DPLL3_BASE;
> > > >         else
> > > >                 BUG();
> > > > 
> > > >         return NULL;
> > > > #endif
> > > 
> > > Be careful about stuff like this.  Remember that enabling CONFIG_OF
> > > must *not break* board support that does not use the device tree.  The
> > > above #ifdef block will break existing users.
> > > 
> > Though the code has been killed in the latest version I just sent
> > yesterday I sent last night, I do not understand how it will break
> > the existing users.  The existing code is:
> > 
> > static inline void __iomem *_mx51_get_pll_base(struct clk *pll)
> > {
> >         if (pll == &pll1_main_clk)
> >                 return MX51_DPLL1_BASE;
> >         else if (pll == &pll2_sw_clk)
> >                 return MX51_DPLL2_BASE;
> >         else if (pll == &pll3_sw_clk)
> >                 return MX51_DPLL3_BASE;
> >         else
> >                 BUG();
> > 
> >         return NULL;
> > }
> 
> What you wrote wrapped the current implementation with #ifdef
> CONFIG_OF ... #else [existing code] #endif.  That says to me that when
> CONFIG_OF is enabled, the old code gets compiled out, which means the
> function no longer works on non-dt platforms.
> 
> The goal is to support both dt and non-dt machines with a single
> kernel image.
> 
Ah, I missed this point.  Thanks.

-- 
Regards,
Shawn

[PATCH 3/5] arm/dt: mx51: dynamically add gpt and uart related clocks per dt nodes

[Shawn Guo]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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)
+
+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);
+		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");
+}
 
-		rc = of_clk_add_provider(node, mx5_dt_clk_get, clk);
-		if (rc)
-			pr_err("error adding fixed clk %s\n", node->name);
+static __init int mx5_add_uart_clk(struct device_node *node)
+{
+	const __be32 *reg;
+	int id;
+
+	ALLOC_CLK_LOOKUP();
+
+	reg = of_get_property(node, "reg", NULL);
+	if (!reg) {
+		ret = -ENOENT;
+		goto out_kfree;
+	}
+
+	id = be32_to_cpu(*reg);
+	if (id < 0 || id > 2) {
+		ret = -EINVAL;
+		goto out_kfree;
+	}
+
+	clk->id = id;
+	clk->parent = mx5_get_source_clk(node);
+	clk->secondary = mx5_get_depend_clk(node);
+	clk->enable = _clk_ccgr_enable;
+	clk->disable = _clk_ccgr_disable;
+	clk->enable_reg = MXC_CCM_CCGR1;
+
+	switch (id) {
+	case 0:
+		clk->enable_shift = MXC_CCM_CCGRx_CG4_OFFSET;
+		break;
+	case 1:
+		clk->enable_shift = MXC_CCM_CCGRx_CG6_OFFSET;
+		break;
+	case 2:
+		clk->enable_shift = MXC_CCM_CCGRx_CG8_OFFSET;
+	}
+
+	ADD_CLK_LOOKUP();
+}
+
+static __init int mx5_add_uart_root_clk(struct device_node *node)
+{
+	ALLOC_CLK_LOOKUP();
+
+	clk->get_rate = clk_uart_get_rate;
+	clk->set_parent = clk_uart_set_parent;
+
+	ADD_CLK_LOOKUP();
+}
+
+static __init int mx5_add_uart_ipg_clk(struct device_node *node)
+{
+	const __be32 *reg;
+	int id;
+
+	ALLOC_CLK_LOOKUP();
+
+	reg = of_get_property(node, "reg", NULL);
+	if (!reg) {
+		ret = -ENOENT;
+		goto out_kfree;
 	}
+
+	id = be32_to_cpu(*reg);
+	if (id < 0 || id > 2) {
+		ret = -EINVAL;
+		goto out_kfree;
+	}
+
+	clk->id = id;
+	clk->enable_reg = MXC_CCM_CCGR1;
+	clk->enable = _clk_ccgr_enable;
+	clk->disable = _clk_ccgr_disable;
+
+	switch (id) {
+	case 0:
+		clk->enable_shift = MXC_CCM_CCGRx_CG3_OFFSET;
+		break;
+	case 1:
+		clk->enable_shift = MXC_CCM_CCGRx_CG5_OFFSET;
+		break;
+	case 2:
+		clk->enable_shift = MXC_CCM_CCGRx_CG7_OFFSET;
+	}
+
+	ADD_CLK_LOOKUP();
+}
+
+static __init int mx5_add_gpt_clk(struct device_node *node)
+{
+	ALLOC_CLK_LOOKUP();
+
+	clk->enable_reg = MXC_CCM_CCGR2;
+	clk->enable_shift = MXC_CCM_CCGRx_CG9_OFFSET;
+	clk->enable = _clk_ccgr_enable;
+	clk->disable = _clk_ccgr_disable;
+
+	ADD_CLK_LOOKUP();
+}
+
+static __init int mx5_add_gpt_ipg_clk(struct device_node *node)
+{
+	ALLOC_CLK_LOOKUP();
+
+	clk->enable_reg = MXC_CCM_CCGR2;
+	clk->enable_shift = MXC_CCM_CCGRx_CG10_OFFSET;
+	clk->enable = _clk_ccgr_enable;
+	clk->disable = _clk_ccgr_disable;
+
+	ADD_CLK_LOOKUP();
+}
+
+static __init int mx5_add_aips_tz_clk(struct device_node *node)
+{
+	const __be32 *reg;
+	int id;
+
+	ALLOC_CLK_LOOKUP();
+
+	reg = of_get_property(node, "reg", NULL);
+	if (!reg) {
+		ret = -ENOENT;
+		goto out_kfree;
+	}
+
+	id = be32_to_cpu(*reg);
+	if (id < 0 || id > 1) {
+		ret = -EINVAL;
+		goto out_kfree;
+	}
+
+	clk->id = id;
+	clk->enable_reg = MXC_CCM_CCGR0;
+	clk->enable_shift = id ? MXC_CCM_CCGRx_CG12_OFFSET :
+				 MXC_CCM_CCGRx_CG13_OFFSET;
+	clk->enable = _clk_ccgr_enable;
+	clk->disable = _clk_ccgr_disable_inwait;
+
+	ADD_CLK_LOOKUP();
+}
+
+static __init int mx5_add_ahb_max_clk(struct device_node *node)
+{
+	ALLOC_CLK_LOOKUP();
+
+	clk->enable_reg = MXC_CCM_CCGR0;
+	clk->enable_shift = MXC_CCM_CCGRx_CG14_OFFSET;
+	clk->enable = _clk_max_enable;
+	clk->disable = _clk_max_disable;
+
+	ADD_CLK_LOOKUP();
+}
+
+static __init int mx5_add_spba_clk(struct device_node *node)
+{
+	ALLOC_CLK_LOOKUP();
+
+	clk->enable_reg = MXC_CCM_CCGR5;
+	clk->enable_shift = MXC_CCM_CCGRx_CG0_OFFSET;
+	clk->enable = _clk_ccgr_enable;
+	clk->disable = _clk_ccgr_disable;
+
+	ADD_CLK_LOOKUP();
+}
+
+static __init int mx5_add_ipg_clk(struct device_node *node)
+{
+	ALLOC_CLK_LOOKUP();
+
+	clk->get_rate = clk_ipg_get_rate;
+
+	ADD_CLK_LOOKUP();
+}
+
+static __init int mx5_add_ahb_clk(struct device_node *node)
+{
+	ALLOC_CLK_LOOKUP();
+
+	clk->get_rate = clk_ahb_get_rate;
+	clk->set_rate = _clk_ahb_set_rate;
+	clk->round_rate = _clk_ahb_round_rate;
+
+	ADD_CLK_LOOKUP();
+}
+
+static __init int mx5_add_main_bus_clk(struct device_node *node)
+{
+	ALLOC_CLK_LOOKUP();
+
+	clk->set_parent = _clk_main_bus_set_parent;
+
+	ADD_CLK_LOOKUP();
+}
+
+static __init int mx5_add_lp_apm_clk(struct device_node *node)
+{
+	ALLOC_CLK_LOOKUP();
+
+	clk->set_parent = _clk_lp_apm_set_parent;
+
+	ADD_CLK_LOOKUP();
+}
+
+static __init int mx5_add_pll_switch_clk(struct device_node *node)
+{
+	const __be32 *reg;
+	int id;
+
+	ALLOC_CLK_LOOKUP();
+
+	reg = of_get_property(node, "reg", NULL);
+	if (!reg) {
+		ret = -ENOENT;
+		goto out_kfree;
+	}
+
+	id = be32_to_cpu(*reg);
+	if (id < 0 || id > 2) {
+		ret = -EINVAL;
+		goto out_kfree;
+	}
+
+	clk->id = id;
+
+	switch (id) {
+	case 0:
+		clk->get_rate = clk_pll1_sw_get_rate;
+		clk->set_parent = _clk_pll1_sw_set_parent;
+		break;
+	case 1:
+		clk->get_rate = clk_pll_get_rate;
+		clk->set_rate = _clk_pll_set_rate;
+		clk->enable = _clk_pll_enable;
+		clk->disable = _clk_pll_disable;
+		clk->set_parent = _clk_pll2_sw_set_parent;
+		clk->pll_base = MX51_DPLL2_BASE;
+		break;
+	case 2:
+		clk->get_rate = clk_pll_get_rate;
+		clk->set_rate = _clk_pll_set_rate;
+		clk->enable = _clk_pll_enable;
+		clk->disable = _clk_pll_disable;
+		clk->pll_base = MX51_DPLL3_BASE;
+	}
+
+	ADD_CLK_LOOKUP();
+}
+
+static __init int mx5_add_pll1_main_clk(struct device_node *node)
+{
+	ALLOC_CLK_LOOKUP();
+
+	clk->get_rate = clk_pll_get_rate;
+	clk->enable = _clk_pll_enable;
+	clk->disable = _clk_pll_disable;
+	clk->pll_base = MX51_DPLL1_BASE;
+
+	ADD_CLK_LOOKUP();
+}
+
+static __init int mx5_dt_scan_clks(void)
+{
+	struct device_node *node;
+	int ret;
+
+	ret = mx5_scan_fixed_clks();
+	if (ret) {
+		pr_err("%s: fixed-clock failed %d\n", __func__, ret);
+		return ret;
+	}
+
+	for_each_compatible_node(node, NULL, "clock") {
+		if (!strcmp(node->name, "pll1_main"))
+			ret = mx5_add_pll1_main_clk(node);
+		else if (!strcmp(node->name, "pll_switch"))
+			ret = mx5_add_pll_switch_clk(node);
+		else if (!strcmp(node->name, "lp_apm"))
+			ret = mx5_add_lp_apm_clk(node);
+		else if (!strcmp(node->name, "main_bus"))
+			ret = mx5_add_main_bus_clk(node);
+		else if (!strcmp(node->name, "ahb"))
+			ret = mx5_add_ahb_clk(node);
+		else if (!strcmp(node->name, "ipg"))
+			ret = mx5_add_ipg_clk(node);
+		else if (!strcmp(node->name, "spba"))
+			ret = mx5_add_spba_clk(node);
+		else if (!strcmp(node->name, "ahb_max"))
+			ret = mx5_add_ahb_max_clk(node);
+		else if (!strcmp(node->name, "aips_tz"))
+			ret = mx5_add_aips_tz_clk(node);
+		else if (!strcmp(node->name, "gpt_ipg"))
+			ret = mx5_add_gpt_ipg_clk(node);
+		else if (!strcmp(node->name, "gpt"))
+			ret = mx5_add_gpt_clk(node);
+		else if (!strcmp(node->name, "uart_ipg"))
+			ret = mx5_add_uart_ipg_clk(node);
+		else if (!strcmp(node->name, "uart_root"))
+			ret = mx5_add_uart_root_clk(node);
+		else if (!strcmp(node->name, "uart"))
+			ret = mx5_add_uart_clk(node);
+		else
+			pr_warn("%s: unknown clock node %s\n",
+				__func__, node->name);
+
+		if (ret) {
+			pr_err("%s: clock %s failed %d\n",
+				__func__, node->name, ret);
+			break;
+		}
+	}
+
+	return ret;
 }
 
 void __init mx5_clk_dt_init(void)
-- 
1.7.1

Re: [PATCH 3/5] arm/dt: mx51: dynamically add gpt and uart related clocks per dt nodes

[Grant Likely]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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.

>  
> -		rc = of_clk_add_provider(node, mx5_dt_clk_get, clk);
> -		if (rc)
> -			pr_err("error adding fixed clk %s\n", node->name);
> +static __init int mx5_add_uart_clk(struct device_node *node)
> +{
> +	const __be32 *reg;
> +	int id;
> +
> +	ALLOC_CLK_LOOKUP();
> +
> +	reg = of_get_property(node, "reg", NULL);
> +	if (!reg) {
> +		ret = -ENOENT;
> +		goto out_kfree;
> +	}
> +
> +	id = be32_to_cpu(*reg);
> +	if (id < 0 || id > 2) {
> +		ret = -EINVAL;
> +		goto out_kfree;
> +	}
> +
> +	clk->id = id;
> +	clk->parent = mx5_get_source_clk(node);
> +	clk->secondary = mx5_get_depend_clk(node);
> +	clk->enable = _clk_ccgr_enable;
> +	clk->disable = _clk_ccgr_disable;
> +	clk->enable_reg = MXC_CCM_CCGR1;
> +
> +	switch (id) {
> +	case 0:
> +		clk->enable_shift = MXC_CCM_CCGRx_CG4_OFFSET;
> +		break;
> +	case 1:
> +		clk->enable_shift = MXC_CCM_CCGRx_CG6_OFFSET;
> +		break;
> +	case 2:
> +		clk->enable_shift = MXC_CCM_CCGRx_CG8_OFFSET;
> +	}
> +
> +	ADD_CLK_LOOKUP();
> +}
> +
> +static __init int mx5_add_uart_root_clk(struct device_node *node)
> +{
> +	ALLOC_CLK_LOOKUP();
> +
> +	clk->get_rate = clk_uart_get_rate;
> +	clk->set_parent = clk_uart_set_parent;
> +
> +	ADD_CLK_LOOKUP();
> +}
> +
> +static __init int mx5_add_uart_ipg_clk(struct device_node *node)
> +{
> +	const __be32 *reg;
> +	int id;
> +
> +	ALLOC_CLK_LOOKUP();
> +
> +	reg = of_get_property(node, "reg", NULL);
> +	if (!reg) {
> +		ret = -ENOENT;
> +		goto out_kfree;
>  	}
> +
> +	id = be32_to_cpu(*reg);
> +	if (id < 0 || id > 2) {
> +		ret = -EINVAL;
> +		goto out_kfree;
> +	}
> +
> +	clk->id = id;
> +	clk->enable_reg = MXC_CCM_CCGR1;
> +	clk->enable = _clk_ccgr_enable;
> +	clk->disable = _clk_ccgr_disable;
> +
> +	switch (id) {
> +	case 0:
> +		clk->enable_shift = MXC_CCM_CCGRx_CG3_OFFSET;
> +		break;
> +	case 1:
> +		clk->enable_shift = MXC_CCM_CCGRx_CG5_OFFSET;
> +		break;
> +	case 2:
> +		clk->enable_shift = MXC_CCM_CCGRx_CG7_OFFSET;
> +	}
> +
> +	ADD_CLK_LOOKUP();
> +}
> +
> +static __init int mx5_add_gpt_clk(struct device_node *node)
> +{
> +	ALLOC_CLK_LOOKUP();
> +
> +	clk->enable_reg = MXC_CCM_CCGR2;
> +	clk->enable_shift = MXC_CCM_CCGRx_CG9_OFFSET;
> +	clk->enable = _clk_ccgr_enable;
> +	clk->disable = _clk_ccgr_disable;
> +
> +	ADD_CLK_LOOKUP();
> +}
> +
> +static __init int mx5_add_gpt_ipg_clk(struct device_node *node)
> +{
> +	ALLOC_CLK_LOOKUP();
> +
> +	clk->enable_reg = MXC_CCM_CCGR2;
> +	clk->enable_shift = MXC_CCM_CCGRx_CG10_OFFSET;
> +	clk->enable = _clk_ccgr_enable;
> +	clk->disable = _clk_ccgr_disable;
> +
> +	ADD_CLK_LOOKUP();
> +}
> +
> +static __init int mx5_add_aips_tz_clk(struct device_node *node)
> +{
> +	const __be32 *reg;
> +	int id;
> +
> +	ALLOC_CLK_LOOKUP();
> +
> +	reg = of_get_property(node, "reg", NULL);
> +	if (!reg) {
> +		ret = -ENOENT;
> +		goto out_kfree;
> +	}
> +
> +	id = be32_to_cpu(*reg);
> +	if (id < 0 || id > 1) {
> +		ret = -EINVAL;
> +		goto out_kfree;
> +	}
> +
> +	clk->id = id;
> +	clk->enable_reg = MXC_CCM_CCGR0;
> +	clk->enable_shift = id ? MXC_CCM_CCGRx_CG12_OFFSET :
> +				 MXC_CCM_CCGRx_CG13_OFFSET;
> +	clk->enable = _clk_ccgr_enable;
> +	clk->disable = _clk_ccgr_disable_inwait;
> +
> +	ADD_CLK_LOOKUP();
> +}
> +
> +static __init int mx5_add_ahb_max_clk(struct device_node *node)
> +{
> +	ALLOC_CLK_LOOKUP();
> +
> +	clk->enable_reg = MXC_CCM_CCGR0;
> +	clk->enable_shift = MXC_CCM_CCGRx_CG14_OFFSET;
> +	clk->enable = _clk_max_enable;
> +	clk->disable = _clk_max_disable;
> +
> +	ADD_CLK_LOOKUP();
> +}
> +
> +static __init int mx5_add_spba_clk(struct device_node *node)
> +{
> +	ALLOC_CLK_LOOKUP();
> +
> +	clk->enable_reg = MXC_CCM_CCGR5;
> +	clk->enable_shift = MXC_CCM_CCGRx_CG0_OFFSET;
> +	clk->enable = _clk_ccgr_enable;
> +	clk->disable = _clk_ccgr_disable;
> +
> +	ADD_CLK_LOOKUP();
> +}
> +
> +static __init int mx5_add_ipg_clk(struct device_node *node)
> +{
> +	ALLOC_CLK_LOOKUP();
> +
> +	clk->get_rate = clk_ipg_get_rate;
> +
> +	ADD_CLK_LOOKUP();
> +}
> +
> +static __init int mx5_add_ahb_clk(struct device_node *node)
> +{
> +	ALLOC_CLK_LOOKUP();
> +
> +	clk->get_rate = clk_ahb_get_rate;
> +	clk->set_rate = _clk_ahb_set_rate;
> +	clk->round_rate = _clk_ahb_round_rate;
> +
> +	ADD_CLK_LOOKUP();
> +}
> +
> +static __init int mx5_add_main_bus_clk(struct device_node *node)
> +{
> +	ALLOC_CLK_LOOKUP();
> +
> +	clk->set_parent = _clk_main_bus_set_parent;
> +
> +	ADD_CLK_LOOKUP();
> +}
> +
> +static __init int mx5_add_lp_apm_clk(struct device_node *node)
> +{
> +	ALLOC_CLK_LOOKUP();
> +
> +	clk->set_parent = _clk_lp_apm_set_parent;
> +
> +	ADD_CLK_LOOKUP();
> +}
> +
> +static __init int mx5_add_pll_switch_clk(struct device_node *node)
> +{
> +	const __be32 *reg;
> +	int id;
> +
> +	ALLOC_CLK_LOOKUP();
> +
> +	reg = of_get_property(node, "reg", NULL);
> +	if (!reg) {
> +		ret = -ENOENT;
> +		goto out_kfree;
> +	}
> +
> +	id = be32_to_cpu(*reg);
> +	if (id < 0 || id > 2) {
> +		ret = -EINVAL;
> +		goto out_kfree;
> +	}
> +
> +	clk->id = id;
> +
> +	switch (id) {
> +	case 0:
> +		clk->get_rate = clk_pll1_sw_get_rate;
> +		clk->set_parent = _clk_pll1_sw_set_parent;
> +		break;
> +	case 1:
> +		clk->get_rate = clk_pll_get_rate;
> +		clk->set_rate = _clk_pll_set_rate;
> +		clk->enable = _clk_pll_enable;
> +		clk->disable = _clk_pll_disable;
> +		clk->set_parent = _clk_pll2_sw_set_parent;
> +		clk->pll_base = MX51_DPLL2_BASE;
> +		break;
> +	case 2:
> +		clk->get_rate = clk_pll_get_rate;
> +		clk->set_rate = _clk_pll_set_rate;
> +		clk->enable = _clk_pll_enable;
> +		clk->disable = _clk_pll_disable;
> +		clk->pll_base = MX51_DPLL3_BASE;
> +	}
> +
> +	ADD_CLK_LOOKUP();
> +}
> +
> +static __init int mx5_add_pll1_main_clk(struct device_node *node)
> +{
> +	ALLOC_CLK_LOOKUP();
> +
> +	clk->get_rate = clk_pll_get_rate;
> +	clk->enable = _clk_pll_enable;
> +	clk->disable = _clk_pll_disable;
> +	clk->pll_base = MX51_DPLL1_BASE;
> +
> +	ADD_CLK_LOOKUP();
> +}
> +
> +static __init int mx5_dt_scan_clks(void)
> +{
> +	struct device_node *node;
> +	int ret;
> +
> +	ret = mx5_scan_fixed_clks();
> +	if (ret) {
> +		pr_err("%s: fixed-clock failed %d\n", __func__, ret);
> +		return ret;
> +	}
> +
> +	for_each_compatible_node(node, NULL, "clock") {
> +		if (!strcmp(node->name, "pll1_main"))
> +			ret = mx5_add_pll1_main_clk(node);
> +		else if (!strcmp(node->name, "pll_switch"))
> +			ret = mx5_add_pll_switch_clk(node);
> +		else if (!strcmp(node->name, "lp_apm"))
> +			ret = mx5_add_lp_apm_clk(node);
> +		else if (!strcmp(node->name, "main_bus"))
> +			ret = mx5_add_main_bus_clk(node);
> +		else if (!strcmp(node->name, "ahb"))
> +			ret = mx5_add_ahb_clk(node);
> +		else if (!strcmp(node->name, "ipg"))
> +			ret = mx5_add_ipg_clk(node);
> +		else if (!strcmp(node->name, "spba"))
> +			ret = mx5_add_spba_clk(node);
> +		else if (!strcmp(node->name, "ahb_max"))
> +			ret = mx5_add_ahb_max_clk(node);
> +		else if (!strcmp(node->name, "aips_tz"))
> +			ret = mx5_add_aips_tz_clk(node);
> +		else if (!strcmp(node->name, "gpt_ipg"))
> +			ret = mx5_add_gpt_ipg_clk(node);
> +		else if (!strcmp(node->name, "gpt"))
> +			ret = mx5_add_gpt_clk(node);
> +		else if (!strcmp(node->name, "uart_ipg"))
> +			ret = mx5_add_uart_ipg_clk(node);
> +		else if (!strcmp(node->name, "uart_root"))
> +			ret = mx5_add_uart_root_clk(node);
> +		else if (!strcmp(node->name, "uart"))
> +			ret = mx5_add_uart_clk(node);

You can simplify this whole table is you take advantage of the .data
field in struct of_device_id, and use for_each_matching_node() to
search for relevant nodes.

> +		else
> +			pr_warn("%s: unknown clock node %s\n",
> +				__func__, node->name);
> +
> +		if (ret) {
> +			pr_err("%s: clock %s failed %d\n",
> +				__func__, node->name, ret);
> +			break;
> +		}
> +	}
> +
> +	return ret;
>  }
>  
>  void __init mx5_clk_dt_init(void)
> -- 
> 1.7.1
> 
> 
> _______________________________________________
> linaro-dev mailing list
> linaro-dev-cunTk1MwBs8s++Sfvej+rw@public.gmane.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [PATCH 3/5] arm/dt: mx51: dynamically add gpt and uart related clocks per dt nodes

[Shawn Guo]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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

Re: [PATCH 3/5] arm/dt: mx51: dynamically add gpt and uart related clocks per dt nodes

[Grant Likely]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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";
        };

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

g.

Re: [PATCH 3/5] arm/dt: mx51: dynamically add gpt and uart related clocks per dt nodes

[Shawn Guo]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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).
> 

[PATCH 4/5] arm/dt: mx5: change timer init function to dt clock way

[Shawn Guo]

The big deal here is that it removes the call to mx51_clocks_init,
where all the possible clocks are added to clkdev in static way.

Signed-off-by: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm/mach-mx5/board-dt.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-mx5/board-dt.c b/arch/arm/mach-mx5/board-dt.c
index 90593f5..6b2f11b 100644
--- a/arch/arm/mach-mx5/board-dt.c
+++ b/arch/arm/mach-mx5/board-dt.c
@@ -15,6 +15,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/of_platform.h>
 #include <linux/of_fdt.h>
+#include <linux/clk.h>
 
 #include <mach/common.h>
 #include <mach/hardware.h>
@@ -41,8 +42,14 @@ static void __init mx51_dt_board_init(void)
 
 static void __init mx51_dt_timer_init(void)
 {
-	mx51_clocks_init(32768, 24000000, 22579200, 0);
+	struct clk *clk;
+
 	mx5_clk_dt_init();
+
+	clk = clk_get_sys("gpt", NULL);
+	if (!IS_ERR(clk))
+		mxc_timer_init(clk, MX51_IO_ADDRESS(MX51_GPT1_BASE_ADDR),
+				MX51_MXC_INT_GPT);
 }
 
 static struct sys_timer mxc_timer = {
-- 
1.7.1

[PATCH 5/5] of/clock: eliminate function __of_clk_get_from_provider

[Shawn Guo]

With the platform clock support, the 'struct clk' should have been
associated with device_node->data.  So the use of function
__of_clk_get_from_provider can be eliminated.

Signed-off-by: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/of/clock.c |   23 ++---------------------
 1 files changed, 2 insertions(+), 21 deletions(-)

diff --git a/drivers/of/clock.c b/drivers/of/clock.c
index 7b5ea67..f124d0a 100644
--- a/drivers/of/clock.c
+++ b/drivers/of/clock.c
@@ -71,24 +71,6 @@ void of_clk_del_provider(struct device_node *np,
 	mutex_unlock(&of_clk_lock);
 }
 
-static struct clk *__of_clk_get_from_provider(struct device_node *np, const char *clk_output)
-{
-	struct of_clk_provider *provider;
-	struct clk *clk = NULL;
-
-	/* Check if we have such a provider in our array */
-	mutex_lock(&of_clk_lock);
-	list_for_each_entry(provider, &of_clk_providers, link) {
-		if (provider->node == np)
-			clk = provider->get(np, clk_output, provider->data);
-		if (clk)
-			break;
-	}
-	mutex_unlock(&of_clk_lock);
-
-	return clk;
-}
-
 struct clk *of_clk_get(struct device *dev, const char *id)
 {
 	struct device_node *provnode;
@@ -123,9 +105,8 @@ struct clk *of_clk_get(struct device *dev, const char *id)
 			__func__, prop_name, dev->of_node->full_name);
 		return NULL;
 	}
-	clk = __of_clk_get_from_provider(provnode, prop);
-	if (clk)
-		dev_dbg(dev, "Using clock from %s\n", provnode->full_name);
+
+	clk = provnode->data;
 
 	of_node_put(provnode);
 
-- 
1.7.1

[PATCH 0/5] add full platform dt clock support for mx51 babbage

[Shawn Guo]

This patch set is to add full platform dt clock support for mx51
babbage, based on Jason's basic-mx51-dt patch.  All mx51 non-dt clocks
in clock-mx51-mx53.c are translated to dt ones.

Shawn Guo (5):
      arm/dts: babbage: add all available clock nodes
      arm/mxc: add clk member 'rate' to ease dt fixed-clock support
      arm/dt: mx51: dynamically add clocks per dt nodes
      arm/dt: mx5: change timer init function to dt clock way
      of/clock: eliminate function __of_clk_get_from_provider

 arch/arm/boot/dts/babbage.dts          |  495 +++++++++++-
 arch/arm/mach-mx5/board-dt.c           |    9 +-
 arch/arm/mach-mx5/clock-mx51-mx53.c    | 1401 +++++++++++++++++++++++++++++++-
 arch/arm/plat-mxc/include/mach/clock.h |    4 +
 drivers/of/clock.c                     |   23 +-
 5 files changed, 1886 insertions(+), 46 deletions(-)

Regards,
Shawn

[PATCH 5/5] of/clock: eliminate function __of_clk_get_from_provider

[Shawn Guo]

With the platform clock support, the 'struct clk' should have been
associated with device_node->data.  So the use of function
__of_clk_get_from_provider can be eliminated.

Signed-off-by: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/of/clock.c |   23 ++---------------------
 1 files changed, 2 insertions(+), 21 deletions(-)

diff --git a/drivers/of/clock.c b/drivers/of/clock.c
index 7b5ea67..f124d0a 100644
--- a/drivers/of/clock.c
+++ b/drivers/of/clock.c
@@ -71,24 +71,6 @@ void of_clk_del_provider(struct device_node *np,
 	mutex_unlock(&of_clk_lock);
 }
 
-static struct clk *__of_clk_get_from_provider(struct device_node *np, const char *clk_output)
-{
-	struct of_clk_provider *provider;
-	struct clk *clk = NULL;
-
-	/* Check if we have such a provider in our array */
-	mutex_lock(&of_clk_lock);
-	list_for_each_entry(provider, &of_clk_providers, link) {
-		if (provider->node == np)
-			clk = provider->get(np, clk_output, provider->data);
-		if (clk)
-			break;
-	}
-	mutex_unlock(&of_clk_lock);
-
-	return clk;
-}
-
 struct clk *of_clk_get(struct device *dev, const char *id)
 {
 	struct device_node *provnode;
@@ -123,9 +105,8 @@ struct clk *of_clk_get(struct device *dev, const char *id)
 			__func__, prop_name, dev->of_node->full_name);
 		return NULL;
 	}
-	clk = __of_clk_get_from_provider(provnode, prop);
-	if (clk)
-		dev_dbg(dev, "Using clock from %s\n", provnode->full_name);
+
+	clk = provnode->data;
 
 	of_node_put(provnode);
 
-- 
1.7.1

Re: [PATCH 5/5] of/clock: eliminate function __of_clk_get_from_provider

[Grant Likely]

On Mon, Mar 14, 2011 at 09:18:42PM +0800, Shawn Guo wrote:
> With the platform clock support, the 'struct clk' should have been
> associated with device_node->data.  So the use of function
> __of_clk_get_from_provider can be eliminated.
> 
> Signed-off-by: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/of/clock.c |   23 ++---------------------
>  1 files changed, 2 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/of/clock.c b/drivers/of/clock.c
> index 7b5ea67..f124d0a 100644
> --- a/drivers/of/clock.c
> +++ b/drivers/of/clock.c
> @@ -71,24 +71,6 @@ void of_clk_del_provider(struct device_node *np,
>  	mutex_unlock(&of_clk_lock);
>  }
>  
> -static struct clk *__of_clk_get_from_provider(struct device_node *np, const char *clk_output)
> -{
> -	struct of_clk_provider *provider;
> -	struct clk *clk = NULL;
> -
> -	/* Check if we have such a provider in our array */
> -	mutex_lock(&of_clk_lock);
> -	list_for_each_entry(provider, &of_clk_providers, link) {
> -		if (provider->node == np)
> -			clk = provider->get(np, clk_output, provider->data);
> -		if (clk)
> -			break;
> -	}
> -	mutex_unlock(&of_clk_lock);
> -
> -	return clk;
> -}
> -
>  struct clk *of_clk_get(struct device *dev, const char *id)
>  {
>  	struct device_node *provnode;
> @@ -123,9 +105,8 @@ struct clk *of_clk_get(struct device *dev, const char *id)
>  			__func__, prop_name, dev->of_node->full_name);
>  		return NULL;
>  	}
> -	clk = __of_clk_get_from_provider(provnode, prop);
> -	if (clk)
> -		dev_dbg(dev, "Using clock from %s\n", provnode->full_name);
> +
> +	clk = provnode->data;

Where is the device_node->data pointer getting set?

In general the ->data pointer of struct device_node should be avoided.
There are no strong rules about its usage which means there is a very
real risk that another driver or subsystem will try to use it for a
different purpose.  

Iterating over the whole device tree is safer, and it really isn't
very expensive.  If you really want to store the struct_clk pointer in
the device node, then it would be better to add a struct clk * field
to struct device_node.

g.

Re: [PATCH 5/5] of/clock: eliminate function __of_clk_get_from_provider

[Shawn Guo]

On Tue, Mar 15, 2011 at 01:54:05AM -0600, Grant Likely wrote:
> On Mon, Mar 14, 2011 at 09:18:42PM +0800, Shawn Guo wrote:
> > With the platform clock support, the 'struct clk' should have been
> > associated with device_node->data.  So the use of function
> > __of_clk_get_from_provider can be eliminated.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > ---
> >  drivers/of/clock.c |   23 ++---------------------
> >  1 files changed, 2 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/of/clock.c b/drivers/of/clock.c
> > index 7b5ea67..f124d0a 100644
> > --- a/drivers/of/clock.c
> > +++ b/drivers/of/clock.c
> > @@ -71,24 +71,6 @@ void of_clk_del_provider(struct device_node *np,
> >  	mutex_unlock(&of_clk_lock);
> >  }
> >  
> > -static struct clk *__of_clk_get_from_provider(struct device_node *np, const char *clk_output)
> > -{
> > -	struct of_clk_provider *provider;
> > -	struct clk *clk = NULL;
> > -
> > -	/* Check if we have such a provider in our array */
> > -	mutex_lock(&of_clk_lock);
> > -	list_for_each_entry(provider, &of_clk_providers, link) {
> > -		if (provider->node == np)
> > -			clk = provider->get(np, clk_output, provider->data);
> > -		if (clk)
> > -			break;
> > -	}
> > -	mutex_unlock(&of_clk_lock);
> > -
> > -	return clk;
> > -}
> > -
> >  struct clk *of_clk_get(struct device *dev, const char *id)
> >  {
> >  	struct device_node *provnode;
> > @@ -123,9 +105,8 @@ struct clk *of_clk_get(struct device *dev, const char *id)
> >  			__func__, prop_name, dev->of_node->full_name);
> >  		return NULL;
> >  	}
> > -	clk = __of_clk_get_from_provider(provnode, prop);
> > -	if (clk)
> > -		dev_dbg(dev, "Using clock from %s\n", provnode->full_name);
> > +
> > +	clk = provnode->data;
> 
> Where is the device_node->data pointer getting set?
> 
+#define ADD_CLK_LOOKUP()						\
+	do {								\
+		node->data = clk;					\
		^^^^^^^^^^^^^^^^^
+									\
+		cl->dev_id = dev_id;					\
+		cl->clk = clk;						\
+		clkdev_add(cl);						\
+									\
+		return 0;						\
+									\
+	out_kfree:							\
+		kfree(cl);						\
+		return ret;						\
+	} while (0)

> In general the ->data pointer of struct device_node should be avoided.
> There are no strong rules about its usage which means there is a very
> real risk that another driver or subsystem will try to use it for a
> different purpose.  
> 
> Iterating over the whole device tree is safer, and it really isn't
> very expensive.  If you really want to store the struct_clk pointer in
> the device node, then it would be better to add a struct clk * field
> to struct device_node.
> 

-- 
Regards,
Shawn

Re: [PATCH 5/5] of/clock: eliminate function __of_clk_get_from_provider

[Grant Likely]

On Tue, Mar 15, 2011 at 03:59:16PM +0800, Shawn Guo wrote:
> On Tue, Mar 15, 2011 at 01:54:05AM -0600, Grant Likely wrote:
> > On Mon, Mar 14, 2011 at 09:18:42PM +0800, Shawn Guo wrote:
> > > With the platform clock support, the 'struct clk' should have been
> > > associated with device_node->data.  So the use of function
> > > __of_clk_get_from_provider can be eliminated.
> > > 
> > > Signed-off-by: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > > ---
> > >  drivers/of/clock.c |   23 ++---------------------
> > >  1 files changed, 2 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/drivers/of/clock.c b/drivers/of/clock.c
> > > index 7b5ea67..f124d0a 100644
> > > --- a/drivers/of/clock.c
> > > +++ b/drivers/of/clock.c
> > > @@ -71,24 +71,6 @@ void of_clk_del_provider(struct device_node *np,
> > >  	mutex_unlock(&of_clk_lock);
> > >  }
> > >  
> > > -static struct clk *__of_clk_get_from_provider(struct device_node *np, const char *clk_output)
> > > -{
> > > -	struct of_clk_provider *provider;
> > > -	struct clk *clk = NULL;
> > > -
> > > -	/* Check if we have such a provider in our array */
> > > -	mutex_lock(&of_clk_lock);
> > > -	list_for_each_entry(provider, &of_clk_providers, link) {
> > > -		if (provider->node == np)
> > > -			clk = provider->get(np, clk_output, provider->data);
> > > -		if (clk)
> > > -			break;
> > > -	}
> > > -	mutex_unlock(&of_clk_lock);
> > > -
> > > -	return clk;
> > > -}
> > > -
> > >  struct clk *of_clk_get(struct device *dev, const char *id)
> > >  {
> > >  	struct device_node *provnode;
> > > @@ -123,9 +105,8 @@ struct clk *of_clk_get(struct device *dev, const char *id)
> > >  			__func__, prop_name, dev->of_node->full_name);
> > >  		return NULL;
> > >  	}
> > > -	clk = __of_clk_get_from_provider(provnode, prop);
> > > -	if (clk)
> > > -		dev_dbg(dev, "Using clock from %s\n", provnode->full_name);
> > > +
> > > +	clk = provnode->data;
> > 
> > Where is the device_node->data pointer getting set?
> > 
> +#define ADD_CLK_LOOKUP()						\
> +	do {								\
> +		node->data = clk;					\
> 		^^^^^^^^^^^^^^^^^
> +									\
> +		cl->dev_id = dev_id;					\
> +		cl->clk = clk;						\
> +		clkdev_add(cl);						\
> +									\
> +		return 0;						\
> +									\
> +	out_kfree:							\
> +		kfree(cl);						\
> +		return ret;						\
> +	} while (0)

Yeah... don't do that!  :-)

g.

Re: [PATCH 5/5] of/clock: eliminate function __of_clk_get_from_provider

[Shawn Guo]

On Tue, Mar 15, 2011 at 01:54:05AM -0600, Grant Likely wrote:
> On Mon, Mar 14, 2011 at 09:18:42PM +0800, Shawn Guo wrote:
> > With the platform clock support, the 'struct clk' should have been
> > associated with device_node->data.  So the use of function
> > __of_clk_get_from_provider can be eliminated.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > ---
> >  drivers/of/clock.c |   23 ++---------------------
> >  1 files changed, 2 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/of/clock.c b/drivers/of/clock.c
> > index 7b5ea67..f124d0a 100644
> > --- a/drivers/of/clock.c
> > +++ b/drivers/of/clock.c
> > @@ -71,24 +71,6 @@ void of_clk_del_provider(struct device_node *np,
> >  	mutex_unlock(&of_clk_lock);
> >  }
> >  
> > -static struct clk *__of_clk_get_from_provider(struct device_node *np, const char *clk_output)
> > -{
> > -	struct of_clk_provider *provider;
> > -	struct clk *clk = NULL;
> > -
> > -	/* Check if we have such a provider in our array */
> > -	mutex_lock(&of_clk_lock);
> > -	list_for_each_entry(provider, &of_clk_providers, link) {
> > -		if (provider->node == np)
> > -			clk = provider->get(np, clk_output, provider->data);
> > -		if (clk)
> > -			break;
> > -	}
> > -	mutex_unlock(&of_clk_lock);
> > -
> > -	return clk;
> > -}
> > -
> >  struct clk *of_clk_get(struct device *dev, const char *id)
> >  {
> >  	struct device_node *provnode;
> > @@ -123,9 +105,8 @@ struct clk *of_clk_get(struct device *dev, const char *id)
> >  			__func__, prop_name, dev->of_node->full_name);
> >  		return NULL;
> >  	}
> > -	clk = __of_clk_get_from_provider(provnode, prop);
> > -	if (clk)
> > -		dev_dbg(dev, "Using clock from %s\n", provnode->full_name);
> > +
> > +	clk = provnode->data;
> 
> Where is the device_node->data pointer getting set?
> 
> In general the ->data pointer of struct device_node should be avoided.
> There are no strong rules about its usage which means there is a very
> real risk that another driver or subsystem will try to use it for a
> different purpose.  
> 
> Iterating over the whole device tree is safer, and it really isn't
> very expensive.  If you really want to store the struct_clk pointer in

I do not understand how we can get the pointer to the 'struct clk'
from device tree.  The clock code reads nodes from device tree and
creates 'struct clk' dynamically.  If we do not save the pointer
somewhere, the pointer will get lost outside the clock code.  How
can we possibly get it back from device tree later?

> the device node, then it would be better to add a struct clk * field
> to struct device_node.
> 

-- 
Regards,
Shawn