| Message ID | 1299514932-13558-2-git-send-email-shawn.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
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. > > * 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. > > Signed-off-by: Shawn Guo <shawn.guo@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. > + reg = <0>; > + clock-outputs = "pll1_main"; > + clock-source = <&osc_clk>; > + }; > + > + pll1_sw_clk: pll_switch@0 { > + compatible = "clock"; > + reg = <0>; > + clock-outputs = "pll1_sw"; > + clock-source = <&pll1_main_clk>; > + }; > + > + pll2_sw_clk: pll_switch@1 { > + compatible = "clock"; > + reg = <1>; > + clock-outputs = "pll2_sw"; > + clock-source = <&osc_clk>; > + }; > + > + pll3_sw_clk: pll_switch@2 { > + compatible = "clock"; > + reg = <2>; > + clock-outputs = "pll3_sw"; > + clock-source = <&osc_clk>; > + }; > + > + lp_apm_clk: lp_apm { > + compatible = "clock"; > + clock-outputs = "lp_apm"; > + clock-source = <&osc_clk>; > + }; > + > + main_bus_clk: main_bus { > + compatible = "clock"; > + clock-outputs = "main_bus"; > + clock-source = <&pll2_sw_clk>; > + }; > + > + ahb_clk: ahb { > + compatible = "clock"; > + clock-outputs = "ahb"; > + clock-source = <&main_bus_clk>; > + }; > + > + ipg_clk: ipg { > + compatible = "clock"; > + clock-outputs = "ipg"; > + clock-source = <&ahb_clk>; > + }; > + > + spba_clk: spba { > + compatible = "clock"; > + clock-outputs = "spba"; > + clock-source = <&ipg_clk>; > + }; > + > + ahb_max_clk: ahb_max { > + compatible = "clock"; > + clock-outputs = "ahb_max"; > + clock-source = <&ahb_clk>; > + }; > + > + aips_tz1_clk: aips_tz@0 { > + compatible = "clock"; > + reg = <0>; > + clock-outputs = "aips_tz1"; > + clock-source = <&ahb_clk>; > + clock-depend = <&ahb_max_clk>; > + }; > + > + aips_tz2_clk: aips_tz@1 { > + compatible = "clock"; > + reg = <1>; > + clock-outputs = "aips_tz2"; > + clock-source = <&ahb_clk>; > + clock-depend = <&ahb_max_clk>; > + }; > + > + gpt_ipg_clk: gpt_ipg { > + compatible = "clock"; > + clock-outputs = "gpt_ipg"; > + clock-source = <&ipg_clk>; > + }; > + > + gpt_clk: gpt { > + compatible = "clock"; > + clock-outputs = "gpt"; > + clock-source = <&ipg_clk>; > + clock-depend = <&gpt_ipg_clk>; > + }; > + > + uart1_ipg_clk: uart_ipg@0 { > compatible = "clock"; > + reg = <0>; > + clock-outputs = "uart1_ipg"; > + clock-source = <&ipg_clk>; > + clock-depend = <&aips_tz1_clk>; > + }; > + > + uart2_ipg_clk: uart_ipg@1 { > + compatible = "clock"; > + reg = <1>; > + clock-outputs = "uart2_ipg"; > + clock-source = <&ipg_clk>; > + clock-depend = <&aips_tz1_clk>; > + }; > + > + uart3_ipg_clk: uart_ipg@2 { > + compatible = "clock"; > + reg = <2>; > + clock-outputs = "uart3_ipg"; > + clock-source = <&ipg_clk>; > + clock-depend = <&spba_clk>; > + }; > + > + uart_root_clk: uart_root { > + compatible = "clock"; > + clock-outputs = "uart_root"; > + clock-source = <&pll2_sw_clk>; > + }; > + > + uart1_clk: uart@0 { > + compatible = "clock"; > + reg = <0>; > clock-outputs = "imx-uart.0"; > + clock-source = <&uart_root_clk>; > }; > > - uart1_clk: uart@1 { > + uart2_clk: uart@1 { > compatible = "clock"; > + reg = <1>; > clock-outputs = "imx-uart.1"; > + clock-source = <&uart_root_clk>; > }; > > - uart2_clk: uart@2 { > + uart3_clk: uart@2 { > compatible = "clock"; > + reg = <2>; > clock-outputs = "imx-uart.2"; > + clock-source = <&uart_root_clk>; > }; > > fec_clk: fec@0 { > @@ -67,7 +217,7 @@ > reg = <0xc000 0x1000>; > interrupts = <0x21>; > rts-cts; > - uart-clock = <&uart2_clk>, "uart"; > + uart-clock = <&uart3_clk>, "uart"; > }; > }; > > @@ -82,7 +232,7 @@ > reg = <0xbc000 0x1000>; > interrupts = <0x1f>; > rts-cts; > - uart-clock = <&uart0_clk>, "uart"; > + uart-clock = <&uart1_clk>, "uart"; > }; > > imx-uart@c0000 { > @@ -90,7 +240,7 @@ > reg = <0xc0000 0x1000>; > interrupts = <0x20>; > rts-cts; > - uart-clock = <&uart1_clk>, "uart"; > + uart-clock = <&uart2_clk>, "uart"; > }; > }; > > -- > 1.7.1 > > > _______________________________________________ > linaro-dev mailing list > linaro-dev@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/linaro-dev >
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. 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.guo@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.
Hi, Shawn, On Tue, Mar 8, 2011 at 12: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. > > * 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. > > Signed-off-by: Shawn Guo <shawn.guo@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"; > + reg = <0>; > + clock-outputs = "pll1_main"; > + clock-source = <&osc_clk>; > + }; > + > + pll1_sw_clk: pll_switch@0 { > + compatible = "clock"; > + reg = <0>; > + clock-outputs = "pll1_sw"; > + clock-source = <&pll1_main_clk>; > + }; > + > + pll2_sw_clk: pll_switch@1 { > + compatible = "clock"; > + reg = <1>; > + clock-outputs = "pll2_sw"; > + clock-source = <&osc_clk>; > + }; > It seems that you mis-used the reg property, it need fixed globally. BR, Jason > > -- > 1.7.1 > > > _______________________________________________ > linaro-dev mailing list > linaro-dev@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/linaro-dev >
On Tue, Mar 08, 2011 at 02:56:52PM +0800, Jason Hui wrote: > Hi, Shawn, > > On Tue, Mar 8, 2011 at 12: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. > > > > * 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. > > > > Signed-off-by: Shawn Guo <shawn.guo@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"; > > + reg = <0>; > > + clock-outputs = "pll1_main"; > > + clock-source = <&osc_clk>; > > + }; > > + > > + pll1_sw_clk: pll_switch@0 { > > + compatible = "clock"; > > + reg = <0>; > > + clock-outputs = "pll1_sw"; > > + clock-source = <&pll1_main_clk>; > > + }; > > + > > + pll2_sw_clk: pll_switch@1 { > > + compatible = "clock"; > > + reg = <1>; > > + clock-outputs = "pll2_sw"; > > + clock-source = <&osc_clk>; > > + }; > > > > It seems that you mis-used the reg property, it need fixed globally. > I guessed it out from Grant's comment on your babbage.dts as below. --- quote begin --- >> + uart_clk0: uart@0 { @0 should only be specified if the node has a 'reg = <0>' property. In this case it doesn't so either 'reg' should be added, or '@0' should be removed. --- quote end ---
Hi, Shawn, On Tue, Mar 8, 2011 at 3:07 PM, Shawn Guo <shawn.guo@freescale.com> wrote: > On Tue, Mar 08, 2011 at 02:56:52PM +0800, Jason Hui wrote: >> Hi, Shawn, >> >> On Tue, Mar 8, 2011 at 12: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. >> > >> > * 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. >> > >> > Signed-off-by: Shawn Guo <shawn.guo@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"; >> > + reg = <0>; >> > + clock-outputs = "pll1_main"; >> > + clock-source = <&osc_clk>; >> > + }; >> > + >> > + pll1_sw_clk: pll_switch@0 { >> > + compatible = "clock"; >> > + reg = <0>; >> > + clock-outputs = "pll1_sw"; >> > + clock-source = <&pll1_main_clk>; >> > + }; >> > + >> > + pll2_sw_clk: pll_switch@1 { >> > + compatible = "clock"; >> > + reg = <1>; >> > + clock-outputs = "pll2_sw"; >> > + clock-source = <&osc_clk>; >> > + }; >> > >> >> It seems that you mis-used the reg property, it need fixed globally. >> > I guessed it out from Grant's comment on your babbage.dts as below. I don't understand clearly. Can we have the this usage, grant? reg=<1> in this case, it will be decoded as clk->id finally. pll2_sw_clk: pll_switch@1 { >> > + compatible = "clock"; >> > + reg = <1>; >> > + clock-outputs = "pll2_sw"; >> > + clock-source = <&osc_clk>; >> > + }; I just want to raise the problems. According to ePAPR, 2.3.6 reg Property: reg Value type: <prop-encoded-array> encoded as arbitrary number of (address,length) pairs. Description: The reg property describes the address and length of a device’s memory mapped register space within its parent’s address space. The value is a <prop-encoded-array>, composed of an arbitrary number of pairs of address and length, <address, length>. The number of <u32> cells required to specify the address and length are bus-specific and are specified by the #address-cells and #size-cells properties in the parent of the device node. If the parent node specifies a value of 0 for #size-cells, the length field in the value of reg shall be omitted. Example: Suppose a device within a system-on-a-chip had two blocks of registers—a 32-byte block at offset 0x3000 in the SOC and a 256-byte block at offset 0xFE00. The reg property would be encoded as follows (assuming #address-cells and #size-cells values of 1): reg = <0x3000 0x20 0xFE00 0x100>; > --- quote end --- > > -- > Regards, > Shawn > >
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.
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? > Sorry. The argument above is invalid, as neither dev_id nor con_id will be used to find the 'clk' when DT clock code applies. So, yes, property clock-alias is not needed.
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"; + reg = <0>; + clock-outputs = "pll1_main"; + clock-source = <&osc_clk>; + }; + + pll1_sw_clk: pll_switch@0 { + compatible = "clock"; + reg = <0>; + clock-outputs = "pll1_sw"; + clock-source = <&pll1_main_clk>; + }; + + pll2_sw_clk: pll_switch@1 { + compatible = "clock"; + reg = <1>; + clock-outputs = "pll2_sw"; + clock-source = <&osc_clk>; + }; + + pll3_sw_clk: pll_switch@2 { + compatible = "clock"; + reg = <2>; + clock-outputs = "pll3_sw"; + clock-source = <&osc_clk>; + }; + + lp_apm_clk: lp_apm { + compatible = "clock"; + clock-outputs = "lp_apm"; + clock-source = <&osc_clk>; + }; + + main_bus_clk: main_bus { + compatible = "clock"; + clock-outputs = "main_bus"; + clock-source = <&pll2_sw_clk>; + }; + + ahb_clk: ahb { + compatible = "clock"; + clock-outputs = "ahb"; + clock-source = <&main_bus_clk>; + }; + + ipg_clk: ipg { + compatible = "clock"; + clock-outputs = "ipg"; + clock-source = <&ahb_clk>; + }; + + spba_clk: spba { + compatible = "clock"; + clock-outputs = "spba"; + clock-source = <&ipg_clk>; + }; + + ahb_max_clk: ahb_max { + compatible = "clock"; + clock-outputs = "ahb_max"; + clock-source = <&ahb_clk>; + }; + + aips_tz1_clk: aips_tz@0 { + compatible = "clock"; + reg = <0>; + clock-outputs = "aips_tz1"; + clock-source = <&ahb_clk>; + clock-depend = <&ahb_max_clk>; + }; + + aips_tz2_clk: aips_tz@1 { + compatible = "clock"; + reg = <1>; + clock-outputs = "aips_tz2"; + clock-source = <&ahb_clk>; + clock-depend = <&ahb_max_clk>; + }; + + gpt_ipg_clk: gpt_ipg { + compatible = "clock"; + clock-outputs = "gpt_ipg"; + clock-source = <&ipg_clk>; + }; + + gpt_clk: gpt { + compatible = "clock"; + clock-outputs = "gpt"; + clock-source = <&ipg_clk>; + clock-depend = <&gpt_ipg_clk>; + }; + + uart1_ipg_clk: uart_ipg@0 { compatible = "clock"; + reg = <0>; + clock-outputs = "uart1_ipg"; + clock-source = <&ipg_clk>; + clock-depend = <&aips_tz1_clk>; + }; + + uart2_ipg_clk: uart_ipg@1 { + compatible = "clock"; + reg = <1>; + clock-outputs = "uart2_ipg"; + clock-source = <&ipg_clk>; + clock-depend = <&aips_tz1_clk>; + }; + + uart3_ipg_clk: uart_ipg@2 { + compatible = "clock"; + reg = <2>; + clock-outputs = "uart3_ipg"; + clock-source = <&ipg_clk>; + clock-depend = <&spba_clk>; + }; + + uart_root_clk: uart_root { + compatible = "clock"; + clock-outputs = "uart_root"; + clock-source = <&pll2_sw_clk>; + }; + + uart1_clk: uart@0 { + compatible = "clock"; + reg = <0>; clock-outputs = "imx-uart.0"; + clock-source = <&uart_root_clk>; }; - uart1_clk: uart@1 { + uart2_clk: uart@1 { compatible = "clock"; + reg = <1>; clock-outputs = "imx-uart.1"; + clock-source = <&uart_root_clk>; }; - uart2_clk: uart@2 { + uart3_clk: uart@2 { compatible = "clock"; + reg = <2>; clock-outputs = "imx-uart.2"; + clock-source = <&uart_root_clk>; }; fec_clk: fec@0 { @@ -67,7 +217,7 @@ reg = <0xc000 0x1000>; interrupts = <0x21>; rts-cts; - uart-clock = <&uart2_clk>, "uart"; + uart-clock = <&uart3_clk>, "uart"; }; }; @@ -82,7 +232,7 @@ reg = <0xbc000 0x1000>; interrupts = <0x1f>; rts-cts; - uart-clock = <&uart0_clk>, "uart"; + uart-clock = <&uart1_clk>, "uart"; }; imx-uart@c0000 { @@ -90,7 +240,7 @@ reg = <0xc0000 0x1000>; interrupts = <0x20>; rts-cts; - uart-clock = <&uart1_clk>, "uart"; + uart-clock = <&uart2_clk>, "uart"; }; };
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. * 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. Signed-off-by: Shawn Guo <shawn.guo@linaro.org> --- arch/arm/boot/dts/babbage.dts | 162 +++++++++++++++++++++++++++++++++++++++-- 1 files changed, 156 insertions(+), 6 deletions(-)