Thanks for your review. > > > > > > > +- 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 > > Aah, I see that this is already in use, so it's a bit late to change > this... > I will modify the driver anyway once the binding gets done.
> > > > > > + > > > > +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: > > I didn't spot this earlier, but why "one or more"? are the 2.0 variants > backwards compatible with the 1.0 variants. > One or more for fixed-clock which is compatible with qoriq-sysclk-*. > > > > + - "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? > > Something like: > > - 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. > See the comment by Scott wood. > > > > > > + - "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. > > Sorry? #clock-cells is most definitely _not_ a bool: > ACTs like bool. > 17: #clock-cells: Number of cells in a clock specifier; Typically 0 > for nodes > 18: with a single clock output and 1 for nodes with > multiple > 19: clock outputs. > > And neither are the clock-specifiers encoded with those cells. Consider: > > pll0: pll0@800 { > #clock-cells = <1>; > reg = <0x800 0x4>; > compatible = "fsl,qoriq-core-pll-1.0"; > clocks = <&sysclk>; > clock-output-names = "pll0", "pll0-div2"; > }; > > Here the value of the cells in a clock-specifier seem to be: > 0: pll0 > 1: pll0-div2 > > So in a consumer, the valid values of the cells in a clock-specifier > are: > > consumer: device { > compatible = "vendor,some-device"; > clocks = <&pll0 0>, /* pll0 */ > <&pll0 1>; /* pll0-div2 */ > }; > > There must be some meaning assigned to the values of the cells in the > clock-specifier (e.g. linear index of the clock output in the hardware). > It would be good to describe this, other clock bindings do. > Sorry, I still get what you mean. There is no INDEX at all. 0 is for one output, 1 for multiple output. Just like that. What the " other clock bindings" did you refer to? > > > > > > + > > > > +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? > > I'm not sure I follow -- clocks and clock-names are used by consumers. > They define which clocks are inputs to a consumer, and the names of the > clock inputs on the consumer: > > consumer@0xffff0000 { > reg = <0xffff0000 0x1000>; > compatible = "vendor,some-consumer"; > clocks = <&pl011 0>, > <&otherclock 43 22>, > <&pl011 1>; > clock-names = "apb_pclk", > "pixel_clk", > "scanout_clk"; > }; > > Here the set of clock-names would be defined in the binding of the > consumer, based on the clock input names in the IP documentation -- they > tell the consumer which clock inputs on the consumer the clocks described > in clocks are wired in to, and describe nothing about output of the > provider. Using clock-names allows the set of clocks on an IP to change > over revisions and for some clock inputs to be optional. > OK, I get it. Will name the clock-names better, and add it if missed. Thanks, > > > > > Do we not _always_ need the parent clock? > > > > > Not for fixed-clock that its parent clock is oscillator :) > > Certainly fixed-clock doesn't need any parent clock, but I guess PLLs and > muxes always do. > > > > > > 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. > > Ok. Where we only have one input clock, this doesn't need to be described. > > Do the mux clocks always take 3 input clocks (judging by the examples > they do)? > > > > > > > +- 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@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? > > I hadn't realised there were DTS with this already. Why is there a clock > with clock-frequency = <0> at all? Surely that isn't useful... > See comments by Scott Wood, Thanks you Scoot. > > > > > > + #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. > > Cheers. Also, doesn't a fixed-clock require a clock-frequency? > Yes, it should be. But we got the clock-frequency from parent node because There is a clock-frequency already there before this patch. > > > > > > + 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"; > > > > + }; > > > > + > > > > + 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_0", "pll0_1", > > > > + "pll1_0", > > > "pll1_1"; > > > > + clock-output-names = "cmux0"; > > > > + }; > > > > + > > > > + 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_0", "pll0_1", > > > > + "pll1_0", > > > "pll1_1"; > > 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. > I am confused here with provider/consumer because some nodes can both be consumer and provider. The clock-names are corresponding to clocks one by one, what's wrong with it? Regards, Yuantian > > > > + 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. > > Ok. > > Cheers, > Mark. _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev