[PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree
Tang Yuantian-B29983
B29983 at freescale.com
Fri Oct 11 14:18:18 EST 2013
Thanks for your review.
See my reply inline
> -----Original Message-----
> From: Mark Rutland [mailto:mark.rutland at arm.com]
> Sent: 2013年10月10日 星期四 18:04
> To: Tang Yuantian-B29983
> Cc: galak at kernel.crashing.org; linuxppc-dev at lists.ozlabs.org;
> devicetree at vger.kernel.org; Li Yang-Leo-R58472
> Subject: Re: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device
> tree
>
> On Wed, Oct 09, 2013 at 07:38:24AM +0100, Yuantian.Tang at freescale.com
> wrote:
> > From: Tang Yuantian <yuantian.tang at freescale.com>
> >
> > The following SoCs will be affected: p2041, p3041, p4080, p5020,
> > p5040, b4420, b4860, t4240
> >
> > Signed-off-by: Tang Yuantian <Yuantian.Tang at freescale.com>
> > Signed-off-by: Li Yang <leoli at freescale.com>
> > ---
> > 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
> ++++++++++++++++++++
> > 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 +++++++++++
> > arch/powerpc/boot/dts/fsl/p2041si-pre.dtsi | 4 +
> > arch/powerpc/boot/dts/fsl/p3041si-post.dtsi | 60 +++++++++++
> > arch/powerpc/boot/dts/fsl/p3041si-pre.dtsi | 4 +
> > arch/powerpc/boot/dts/fsl/p4080si-post.dtsi | 112
> +++++++++++++++++++++
> > arch/powerpc/boot/dts/fsl/p4080si-pre.dtsi | 8 ++
> > arch/powerpc/boot/dts/fsl/p5020si-post.dtsi | 42 ++++++++
> > arch/powerpc/boot/dts/fsl/p5020si-pre.dtsi | 2 +
> > arch/powerpc/boot/dts/fsl/p5040si-post.dtsi | 60 +++++++++++
> > arch/powerpc/boot/dts/fsl/p5040si-pre.dtsi | 4 +
> > arch/powerpc/boot/dts/fsl/t4240si-post.dtsi | 85
> ++++++++++++++++
> > 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-clock.txt
> > 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 external
> > +SYSCLK signal. The SYSCLK input (frequency) is multiplied using
> > +multiple phase locked loops (PLL) to create a variety of frequencies
> > +which can then be passed to a variety of internal logic, including
> > +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 clock
>
> While I can see that "fsl,<chip>-clockgen" might have a large set of
> strings that we may never deal with in th kernel, I'd prefer that the
> basic strings (i.e. all the "fsl,qoriq-clockgen-[1,2].x" variants) were
> 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.
> > +- reg: Offset and length of the clock register set
> > +- clock-frequency: Indicates input clock frequency of clock block.
> > + 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.
> > +
> > +Recommended properties:
> > +- #ddress-cells: Specifies the number of cells used to represent
> > + physical base addresses. Must be present if the device has
> > + sub-nodes and set to 1 if present
>
> Typo: #address-cells
>
> In the example it looks like the address cells of child nodes are offsets
> within the unit, rather than absolute physical addresses. Could the code
> not treat them as absolute addresses? Then we'd only need to document
> that #address-cells, #size-cells and ranges must be present and have
> values suitable for mapping child nodes into the address space of the
> parent.
>
OK, thanks.
> > +- #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
> > +
> > +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:
> > + - "fsl,qoriq-core-pll-[1,2].x": Indicates a core PLL clock
> device
> > + - "fsl,qoriq-core-mux-[1,2].x": Indicates a core multiplexer
> clock
> > + device; divided from the core PLL clock
>
> As above, I'd prefer a complete list of the basic strings we expect.
>
There is no list here, just *-mux-1.x and *-mux-2.x
What kind of list do you prefer?
> > + - "fixed-clock": From common clock binding; indicates output
> clock
> > + of oscillator
> > + - "fsl,qoriq-sysclk-[1,2].x": Indicates input system clock
>
> Here too.
>
> > +- #clock-cells: From common clock binding; indicates the number of
> > + output clock. 0 is for one output clock; 1 for more than one
> > +clock
>
> If a clock source has multiple outputs, what those outputs are and what
> values in clock-cells they correspond to should be described here.
>
There is no way to describe the values of multiple outputs here.
This property is the type of BOOL. See the clock-bindings.txt.
> > +
> > +Recommended properties:
> > +- clocks: Should be the phandle of input parent clock
> > +- clock-names: From common clock binding, indicates the clock name
>
> That description's a bit opaque.
>
> What's the name of the clock input on these units? That's what clock-
> names should contain, and that should be documented.
>
Is it necessary to document these names since they are totally used
by clock provider and clock consumer has no idea about them?
> Do we not _always_ need the parent clock?
>
Not for fixed-clock that its parent clock is oscillator :)
> If we have a clock do we need a clock-names entry for it?
>
Technically yes, but I don't bother to add it if the clock node has
only one clocks.
> > +- clock-output-names: From common clock binding, indicates the names
> of
> > + output clocks
> > +- reg: Should be the offset and length of clock block base address.
> > + The length should be 4.
> > +
> > +Example for clock block and clock provider:
> > +/ {
> > + clockgen: global-utilities at e1000 {
> > + compatible = "fsl,p5020-clockgen", "fsl,qoriq-clockgen-
> 1.0";
> > + reg = <0xe1000 0x1000>;
> > + clock-frequency = <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?
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > +
> > + sysclk: sysclk {
> > + #clock-cells = <0>;
> > + compatible = "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.
> > + clock-output-names = "sysclk";
> > + }
> > +
> > + pll0: pll0 at 800 {
> > + #clock-cells = <1>;
> > + reg = <0x800 0x4>;
> > + compatible = "fsl,qoriq-core-pll-1.0";
> > + clocks = <&sysclk>;
> > + clock-output-names = "pll0", "pll0-div2";
> > + };
> > +
> > + pll1: pll1 at 820 {
> > + #clock-cells = <1>;
> > + reg = <0x820 0x4>;
> > + compatible = "fsl,qoriq-core-pll-1.0";
> > + clocks = <&sysclk>;
> > + clock-output-names = "pll1", "pll1-div2";
> > + };
> > +
> > + mux0: mux0 at 0 {
> > + #clock-cells = <0>;
> > + reg = <0x0 0x4>;
> > + compatible = "fsl,qoriq-core-mux-1.0";
> > + clocks = <&pll0 0>, <&pll0 1>, <&pll1 0>,
> <&pll1 1>;
> > + clock-names = "pll0_0", "pll0_1", "pll1_0",
> "pll1_1";
> > + clock-output-names = "cmux0";
> > + };
> > +
> > + mux1: mux1 at 20 {
> > + #clock-cells = <0>;
> > + reg = <0x20 0x4>;
> > + compatible = "fsl,qoriq-core-mux-1.0";
> > + clocks = <&pll0 0>, <&pll0 1>, <&pll1 0>,
> <&pll1 1>;
> > + clock-names = "pll0_0", "pll0_1", "pll1_0",
> "pll1_1";
> > + clock-output-names = "cmux1";
>
> How does the mux choose which input clock to use at a point in time?
>
That is decided at runtime. Different input clock will lead to the different
Clock frequency that the CPUs work on.
Thanks,
Yuantian
> Cheers,
> Mark.
More information about the Linuxppc-dev
mailing list