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

On Mon, Mar 07, 2011 at 10:48:10AM -0700, Grant Likely wrote:

On Mon, Mar 7, 2011 at 9:22 AM, Shawn Guo [off-list ref] wrote:

quoted

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?

quoted

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

quoted

Signed-off-by: Shawn Guo <redacted>
---
 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