> > +++ b/Documentation/devicetree/bindings/clock/corenet-clock.txt
> > @@ -0,0 +1,123 @@
> > +* 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:
>
> When you say "one or more", I assume you mean of the specific clock block
> strings, and then a chassis string?
>
> If so, it might be better to say smoething like:
>
> - compatible: Should contain a specific clock block compatible string
> and a single chassis clock compatible string.
>
> Clock block strings include:
> * "fsl,p2041-clockgen"
> * "fsl,p3041-clockgen"
>
> Chassis clock strings include:
> * "fsl,qoriq-clockgen-1.0": for chassis 1.0 clocks
> * "fsl,qoriq-clockgen-2.0": for chassis 2.0 clock
>
Sounds better.
> > + * "fsl,<chip>-clockgen": for chip specific clock block, here chip
> > + could be but not limited to one of the: p2041, p3041, p4080,
> > + p5020, p5040, t4240, b4420, b4860
> > + * "fsl,qoriq-clockgen-1.0": for chassis 1.0 clocks
> > + * "fsl,qoriq-clockgen-2.0": for chassis 2.0 clocks
> > + Notes that "fsl,qoriq-clockgen-1.0" and "fsl,qoriq-clockgen-2.0"
> > + cannot be included simultaneously.
> > +- reg: Offset and length of the clock register set
> > +
> > +Recommended properties:
> > +- ranges: Allows valid translation between child's address space and
> > + parent's. Must be present if the device has sub-nodes.
> > +- #address-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
> > +- #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
>
> Is there any particular reason these _must_ be 1?
>
They are one u32 variable long and no other options, so, they should be set to 1.
> > +
> > +2. Clock Provider/Consumer Binding
> > +
> > +Most of the bindings are from the common clock binding[1].
> > + [1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> > +
> > +Required properties:
> > +- compatible : Should include one of the following:
> > + * "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)
> > + * "fsl,qoriq-sysclk-1.0": for input system clock (v1.0)
> > + * "fsl,qoriq-sysclk-2.0": for input system clock (v2.0)
> > +- #clock-cells: From common clock binding; indicates the number of
> > + output clock. 0 is for "fsl,qoriq-sysclk-[1,2].0" and
> > + "fsl,qoriq-core-mux-[1,2].0"; 1 for "fsl,qoriq-core-pll-[1,2].0".
>
> Nit: #clock-cells defines the number of cells in a clock-specifier, not
> the number of output clocks. How about:
>
> - #clock-cells: The number of cells in a clock-specifier. Should be <0>
> for "fsl,qoriq-sysclk-[1,2].0" clocks, or <1> for
> "fsl,qoriq-core-pll-[1,2].0" clocks.
>
> > + If #clock-cells has a value of 1, its clock consumer should specify
> > + the desired clock by having the clock ID in its "clocks" phandle
> > + cell. The following is a full list IDs:
>
> The ID is in the clock-specifier, not the phandle. How about the
> following instead:
>
> For "fsl,qoriq-core-pll-[1,2].0" clocks, the single clock-specifier cell
> may take the following values:
>
OK, thanks.
> > + * 0 - equal to the PLL frequency
> > + * 1 - equal to the PLL frequency divided by 2
> > + * 2 - equal to the PLL frequency divided by 4
> > +
> > +Recommended properties:
> > +- clocks: Should be the phandle of input parent clock
> > +- clock-names: From common clock binding, indicates the clock name
>
> Is this not well defined and common across all of the clocks? The set of
> inputs they have must be well known, and if it isn't then this property
> isn't all that useful.
>
> > +- clock-output-names: From common clock binding, indicates the names
> of
> > + output clocks
>
> Similarly, I'd expect that there might be well defined output names.
>
The clock-output-names and clock-names vary both from nodes to nodes and
from platforms to platforms. There is no general rules to define it.
I think the *-names properties should be added easily by following the example below.
> > +- 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@e1000 {
> > + compatible = "fsl,p5020-clockgen", "fsl,qoriq-clockgen-1.0";
> > + ranges = <0x0 0xe1000 0x1000>;
> > + reg = <0xe1000 0x1000>;
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > +
> > + sysclk: sysclk {
> > + #clock-cells = <0>;
> > + compatible = "fsl,qoriq-sysclk-1.0";
> > + clock-output-names = "sysclk";
> > + }
> > +
> > + pll0: pll0@800 {
> > + #clock-cells = <1>;
> > + reg = <0x800 0x4>;
> > + compatible = "fsl,qoriq-core-pll-1.0";
> > + clocks = <&sysclk>;
> > + clock-output-names = "pll0", "pll0-div2";
> > + };
> > +
> > + pll1: pll1@820 {
> > + #clock-cells = <1>;
> > + reg = <0x820 0x4>;
> > + compatible = "fsl,qoriq-core-pll-1.0";
> > + clocks = <&sysclk>;
> > + clock-output-names = "pll1", "pll1-div2";
> > + };
>
> The binding documentation for clock-output-names describes clock-output-
> names as the name of the clock output signals. I understand this as
> meaning name of the output with respect to the clock, rather than a
> global namespace, but I may be mistaken there. Mike?
>
> Given that I'd expect pll0 and pll1 to have the same set of clock-output-
> names, (perhaps "pll", "pll-div2") as they seem to be instances of the
> same HW block. As far as I am aware the precise instance shouldn't affect
> the name (unless the documentation for the HW have these logical output
> names?).
>
These names will be used to register clock in driver, so it should be unique globally.
So...
> > +
> > + mux0: mux0@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", "pll0-div2", "pll1", "pll1-div2";
> > + clock-output-names = "cmux0";
> > + };
>
> I have a similar worry here with regards to the clock-names, but I guess
> it doesn't really matter here if the documentation doesn't provide any
> better names for the inputs to the mux.
>
> However for the output name I'd expect just "cmux".
>
Same as above, cmux is not unique if we have multiple MUXes.
Thanks,
Yuantian
> Thanks,
> Mark,
>
> > +
> > + mux1: mux1@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", "pll0-div2", "pll1", "pll1-div2";
> > + clock-output-names = "cmux1";
> > + };
> > + };
> > + }
> > +
> > +Example for clock consumer:
> > +
> > +/ {
> > + cpu0: PowerPC,e5500@0 {
> > + ...
> > + clocks = <&mux0>;
> > + ...
> > + };
> > + }
> > --
> > 1.8.0
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe devicetree"
> > in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo
> > info at http://vger.kernel.org/majordomo-info.html
> >
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
