Re: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree

From: Scott Wood <hidden>
Date: 2013-10-11 19:07:13
Also in: linux-devicetree

On Fri, 2013-10-11 at 10:25 +0100, Mark Rutland wrote:

On Fri, Oct 11, 2013 at 04:18:18AM +0100, Tang Yuantian-B29983 wrote:

quoted

Thanks for your review.
See my reply inline

quoted

-----Original Message-----
From: Mark Rutland [mailto:mark.rutland@arm.com]
Sent: 2013=E5=B9=B410=E6=9C=8810=E6=97=A5 =E6=98=9F=E6=9C=9F=E5=9B=9B=

 18:04

quoted

To: Tang Yuantian-B29983
Cc: galak@kernel.crashing.org; linuxppc-dev@lists.ozlabs.org;
devicetree@vger.kernel.org; Li Yang-Leo-R58472
Subject: Re: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in =

device

quoted

tree

On Wed, Oct 09, 2013 at 07:38:24AM +0100, Yuantian.Tang@freescale.c=

om

quoted

wrote:

quoted

From: Tang Yuantian <redacted>

The following SoCs will be affected: p2041, p3041, p4080, p5020,
p5040, b4420, b4860, t4240

Signed-off-by: Tang Yuantian <redacted>
Signed-off-by: Li Yang <redacted>
---
v5:
        - refine the binding document
        - update the compatible string
v4:
        - add binding document
        - update compatible string
        - update the reg property
v3:
        - fix typo
v2:
        - add t4240, b4420, b4860 support
        - remove pll/4 clock from p2041, p3041 and p5020 board

 .../devicetree/bindings/clock/corenet-clock.txt    | 111

++++++++++++++++++++

quoted

 arch/powerpc/boot/dts/fsl/b4420si-post.dtsi        |  35 +++++++
 arch/powerpc/boot/dts/fsl/b4420si-pre.dtsi         |   2 +
 arch/powerpc/boot/dts/fsl/b4860si-post.dtsi        |  35 +++++++
 arch/powerpc/boot/dts/fsl/b4860si-pre.dtsi         |   4 +
 arch/powerpc/boot/dts/fsl/p2041si-post.dtsi        |  60 +++++++=

++++

quoted

 arch/powerpc/boot/dts/fsl/p2041si-pre.dtsi         |   4 +
 arch/powerpc/boot/dts/fsl/p3041si-post.dtsi        |  60 +++++++=

++++

quoted

 arch/powerpc/boot/dts/fsl/p3041si-pre.dtsi         |   4 +
 arch/powerpc/boot/dts/fsl/p4080si-post.dtsi        | 112

+++++++++++++++++++++

quoted

 arch/powerpc/boot/dts/fsl/p4080si-pre.dtsi         |   8 ++
 arch/powerpc/boot/dts/fsl/p5020si-post.dtsi        |  42 +++++++=

quoted

 arch/powerpc/boot/dts/fsl/p5020si-pre.dtsi         |   2 +
 arch/powerpc/boot/dts/fsl/p5040si-post.dtsi        |  60 +++++++=

++++

quoted

 arch/powerpc/boot/dts/fsl/p5040si-pre.dtsi         |   4 +
 arch/powerpc/boot/dts/fsl/t4240si-post.dtsi        |  85

++++++++++++++++

quoted

 arch/powerpc/boot/dts/fsl/t4240si-pre.dtsi         |  12 +++
 17 files changed, 640 insertions(+)
 create mode 100644
Documentation/devicetree/bindings/clock/corenet-clock.txt

diff --git a/Documentation/devicetree/bindings/clock/corenet-cloc=

k.txt

quoted

b/Documentation/devicetree/bindings/clock/corenet-clock.txt
new file mode 100644
index 0000000..8efc62d

--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/corenet-clock.txt

@@ -0,0 +1,111 @@
+* Clock Block on Freescale CoreNet Platforms
+
+Freescale CoreNet chips take primary clocking input from the ext=

ernal

quoted

+SYSCLK signal. The SYSCLK input (frequency) is multiplied using
+multiple phase locked loops (PLL) to create a variety of frequen=

cies

quoted

+which can then be passed to a variety of internal logic, includi=

ng

quoted

+cores and peripheral IP blocks.
+Please refer to the Reference Manual for details.
+
+1. Clock Block Binding
+
+Required properties:
+- compatible: Should include one or more of the following:
+       - "fsl,<chip>-clockgen": for chip specific clock block
+       - "fsl,qoriq-clockgen-[1,2].x": for chassis 1.x and 2.x c=

lock

quoted

While I can see that "fsl,<chip>-clockgen" might have a large set o=

quoted

strings that we may never deal with in th kernel, I'd prefer that t=

he

quoted

basic strings (i.e. all the "fsl,qoriq-clockgen-[1,2].x" variants) =

were

quoted

listed explicitly here.

Given they only seem to be "fsl,qoriq-clockgen-1.0" and "fsl,qoriq-
clockgen-2.0" this shouldn't be too difficult to list and describe.

OK, I will list them all.

=20
Thanks.
=20

quoted

+- reg: Offset and length of the clock register set
+- clock-frequency: Indicates input clock frequency of clock bloc=

k.

quoted

+       Will be set by u-boot

Why does the fact this is set by u-boot matter to the binding?

OK, I will remove it.

quoted

+
+Recommended properties:
+- #ddress-cells: Specifies the number of cells used to represent
+       physical base addresses.  Must be present if the device h=

as

quoted

+       sub-nodes and set to 1 if present

Typo: #address-cells

In the example it looks like the address cells of child nodes are o=

ffsets

quoted

within the unit, rather than absolute physical addresses. Could the=

 code

quoted

not treat them as absolute addresses? Then we'd only need to docume=

nt

quoted

that #address-cells, #size-cells and ranges must be present and hav=

quoted

values suitable for mapping child nodes into the address space of t=

he

quoted

parent.

OK, thanks.

quoted

+- #size-cells: Specifies the number of cells used to represent
+       the size of an address. Must be present if the device has
+       sub-nodes and set to 1 if present

It's not really the size of an address, it's the size of a region
identified by a reg entry.

I think this can be simplified by my suggestion above.

Yes

=20
Aah, I see that this is already in use, so it's a bit late to change
this...
=20

quoted

+
+2. Clock Provider/Consumer Binding
+
+Most of the binding are from the common clock binding[1].
+ [1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : Should include one or more of the following:

=20
I didn't spot this earlier, but why "one or more"? are the 2.0 variants
backwards compatible with the 1.0 variants.
=20

quoted

+       - "fsl,qoriq-core-pll-[1,2].x": Indicates a core PLL cloc=

quoted

device

quoted

+       - "fsl,qoriq-core-mux-[1,2].x": Indicates a core multiple=

xer

quoted

clock

quoted

+               device; divided from the core PLL clock

As above, I'd prefer a complete list of the basic strings we expect=

quoted

There is no list here, just *-mux-1.x and *-mux-2.x
What kind of list do you prefer?

=20
Something like:
=20
- compatible: Should include at least one of:
    * "fsl,qoriq-core-pll-1.0" for core PLL clocks (v1.0)
    * "fsl,qoriq-core-pll-2.0" for core PLL clocks (v2.0)
    * "fsl,qoriq-core-mux-1.0" for core mux clocks (v1.0)
    * "fsl,qoriq-core-mux-2.0" for core mux clocks (v2.0)
  The *-2.0 variants are backwards compatible with the *-1.0 variants,
  so for compatiblity a *-1.0 variant string should be in the list.

I'm not sure that they're 100% compatible.  1.0 seems to have a "KILL"
bit in the PLL register that 2.0 doesn't have (unless it's a
documentation glitch).

quoted

+- clock-output-names: From common clock binding, indicates the n=

ames

quoted

of

quoted

+       output clocks
+- reg: Should be the offset and length of clock block base addre=

ss.

quoted

+       The length should be 4.
+
+Example for clock block and clock provider:
+/ {
+       clockgen: global-utilities@e1000 {
+               compatible =3D "fsl,p5020-clockgen", "fsl,qoriq-c=

lockgen-

quoted

1.0";

quoted

+               reg =3D <0xe1000 0x1000>;
+               clock-frequency =3D <0>;

That looks odd.

Yes, but it has already existed here before this patch.
Can I move it to sysclk clock node since it is not used yet?

=20
I hadn't realised there were DTS with this already. Why is there a cloc=

with clock-frequency =3D <0> at all? Surely that isn't useful...

The actual frequency is patched in by U-Boot -- and moving it to a
different node would break this.

quoted

+               #address-cells =3D <1>;
+               #size-cells =3D <1>;
+
+               sysclk: sysclk {
+                       #clock-cells =3D <0>;
+                       compatible =3D "fsl,qoriq-sysclk-1.0",
+ "fixed-clock";

We didn't mention in the binding that "fsl,qoriq-sysclk-1.0" was
compatible with "fixed-clock" and should have "fixed-clock" in the
compatible list...

OK, will fix it.

=20
Cheers. Also, doesn't a fixed-clock require a clock-frequency?

Why isn't the global-utilities node, that has the clock-frequency,
acting as the fixed-clock?  I thought that's what was in earlier
revisions...

If it absolutely must be a separate node for some reason, I suppose you
could remove the "fixed-clock" and have a tiny "driver" that looks up
the frequency in the parent node.  This would be an instance of a
non-"fixed-clock" that doesn't have a parent clock described in the
device tree, because the information comes from some other mechanism.

quoted

+               mux1: mux1@20 {
+                       #clock-cells =3D <0>;
+                       reg =3D <0x20 0x4>;
+                       compatible =3D "fsl,qoriq-core-mux-1.0";
+                       clocks =3D <&pll0 0>, <&pll0 1>, <&pll1 0=

quoted

<&pll1 1>;

quoted

+                       clock-names =3D "pll0_0", "pll0_1", "pll1=

_0",

quoted

quoted
"pll1_1";

=20
I didn't spot this last time, but the clock-names here seem to be the
names of the outputs from the provider, rather than the input names of
the consumer. This goes against the intended purpose of clock-names.

As far as "pll0", "pll1", etc. goes, that appears to be the input name.
It's all on one chip, so the virtual pins are documented based on what
they're connected to.

I'm not sure I understand the "_0"/"_1" part, though.  Doesn't each PLL
just have one output, which the consumer may choose to divide by 2 (or
in some cases 4)?  Why does that division need to be exposed in the
device tree as separate connections to the parent clock?

-Scott

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help