On Tue, Mar 08, 2011 at 11:44:48AM +0800, Shawn Guo wrote:
On Mon, Mar 07, 2011 at 10:48:10AM -0700, Grant Likely wrote:
On Mon, Mar 7, 2011 at 9:22 AM, Shawn Guo shawn.guo@linaro.org wrote:
The patch is to add all gpt, uart related dt clock nodes for babbage. It sticks to the clock name used in clock-mx51-mx53.c, so that everything gets consistent to Reference Manual. For example, the numbering in clock name usually starts from 1, while 'reg' property numbering starts from 0 to easy clock binding.
Besides the generally used clock bindings, the following properties are proposed in this patch.
- clock-alias
Like clock-outputs to reflect cl->dev_id, property clock-alias is defined to reflect cl->con_id.
This feels like leakage of Linux kernel implementation details getting encoded into the binding. There shouldn't be any need for a clock alias property. It should all just work to have multiple devices explicitly refer to the same clock node because the dt clock support patch gets first crack at resolving a struct clk pointer before clkdev does its lookup.
This is to make clk_get() work. Let's take a look at this example. i.MX28 integrates a amba-pl011 uart controller, and there are two places calling clk_get() with the same dev_id to get the different 'clk'.
static struct clk_lookup lookups[] = { /* for amba bus driver */ _REGISTER_CLOCK("duart", "apb_pclk", xbus_clk) /* for amba-pl011 driver */ _REGISTER_CLOCK("duart", NULL, uart_clk) ... };
- drivers/amba/bus.c - to get xbus_clk
static int amba_get_enable_pclk(struct amba_device *pcdev) { struct clk *pclk = clk_get(&pcdev->dev, "apb_pclk"); int ret;
pcdev->pclk = pclk; if (IS_ERR(pclk)) return PTR_ERR(pclk); ret = clk_enable(pclk); if (ret) clk_put(pclk); return ret;}
- drivers/tty/serial/amba-pl011.c - to get uart_clk
static int pl011_probe(struct amba_device *dev, struct amba_id *id) { ...
uap->clk = clk_get(&dev->dev, NULL); if (IS_ERR(uap->clk)) { ret = PTR_ERR(uap->clk); goto unmap; }... }
Will this be broken if we do not have an alias in dt clock to reflect con_id?
- clock-depend
The mxc 'struct clk' has the member 'secondary' to refer to the clock that the 'clk' has dependency on. This 'secondary' clock needs to be on whenever the 'clk' is set to on. This clock-depend property is defined to reflect this 'secondary' clock.
This is fine, but it is a Freescale specific binding. Each of the imx51 clock nodes should have compatible set to something like "fsl,imx51-clock" so that the OS can know that it should be using this binding.
I doubt this is Freescale clock only use case. But I will do what you suggest here anyway.
[...]
- pll1_main_clk: pll1_main {
- compatible = "clock";
As hinted on above, "clock" doesn't look like a good compatible property. It should specify the specific implementation of a clock device. ie. "fsl,imx51-clock". Even that example may be too generic if there are multiple types of clock controllers in the imx51 SoC.
We are implementing clock-mx51-mx53.c. Would it be better to use 'fsl,mx5-clock'? Or, we will have to scan 'fsl,imx51-clock' and 'fsl,imx53-clock'. Oh, i.MX50 is also coming.
I'm going to use 'fsl,mxc-clock', as the 'clk' is currently defined under plat-mxc. Let me know if anyone is uncomfortable with it.