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....@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
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
