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