[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 at linaro.org> wrote:
> > > The patch is to add all gpt, uart related dt clock nodes for babbage.
> > > It sticks to the clock name used in clock-mx51-mx53.c, so that
> > > everything gets consistent to Reference Manual.  For example, the
> > > numbering in clock name usually starts from 1, while 'reg' property
> > > numbering starts from 0 to easy clock binding.
> > >
> > > Besides the generally used clock bindings, the following properties
> > > are proposed in this patch.
> > >
> > > * clock-alias
> > > Like clock-outputs to reflect cl->dev_id, property clock-alias is
> > > defined to reflect cl->con_id.
> > 
> > This feels like leakage of Linux kernel implementation details getting
> > encoded into the binding.  There shouldn't be any need for a clock
> > alias property.  It should all just work to have multiple devices
> > explicitly refer to the same clock node because the dt clock support
> > patch gets first crack at resolving a struct clk pointer before clkdev
> > does its lookup.
> > 
> This is to make clk_get() work.  Let's take a look at this example.
> i.MX28 integrates a amba-pl011 uart controller, and there are two
> places calling clk_get() with the same dev_id to get the different
> 'clk'.
> 
> static struct clk_lookup lookups[] = {
>         /* for amba bus driver */
>         _REGISTER_CLOCK("duart", "apb_pclk", xbus_clk)
>         /* for amba-pl011 driver */
>         _REGISTER_CLOCK("duart", NULL, uart_clk)
> 	...
> };
> 
> * drivers/amba/bus.c - to get xbus_clk
> static int amba_get_enable_pclk(struct amba_device *pcdev)
> {
>         struct clk *pclk = clk_get(&pcdev->dev, "apb_pclk");
>         int ret;
> 
>         pcdev->pclk = pclk;
> 
>         if (IS_ERR(pclk))
>                 return PTR_ERR(pclk);
> 
>         ret = clk_enable(pclk);
>         if (ret)
>                 clk_put(pclk);
> 
>         return ret;
> }
> 
> * drivers/tty/serial/amba-pl011.c - to get uart_clk
> static int pl011_probe(struct amba_device *dev, struct amba_id *id)
> {
> 	...
> 
>         uap->clk = clk_get(&dev->dev, NULL);
>         if (IS_ERR(uap->clk)) {
>                 ret = PTR_ERR(uap->clk);
>                 goto unmap;
>         }
> 
> 	...
> }
> 
> Will this be broken if we do not have an alias in dt clock to reflect
> con_id?
> 
> > >
> > > * clock-depend
> > > The mxc 'struct clk' has the member 'secondary' to refer to the clock
> > > that the 'clk' has dependency on.  This 'secondary' clock needs to be
> > > on whenever the 'clk' is set to on.  This clock-depend property is
> > > defined to reflect this 'secondary' clock.
> > 
> > This is fine, but it is a Freescale specific binding.  Each of the
> > imx51 clock nodes should have compatible set to something like
> > "fsl,imx51-clock" so that the OS can know that it should be using this
> > binding.
> > 
> I doubt this is Freescale clock only use case.  But I will do what you
> suggest here anyway.
> 
[...]
> > > +               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