Thanks for your review. > -----Original Message----- > From: Mark Rutland [mailto:mark.rutland@xxxxxxx] > Sent: 2013年10月21日 星期一 17:15 > To: Tang Yuantian-B29983 > Cc: galak@xxxxxxxxxxxxxxxxxxx; linuxppc-dev@xxxxxxxxxxxxxxxx; > devicetree@xxxxxxxxxxxxxxx; Li Yang-Leo-R58472 > Subject: Re: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device > tree > > On Sat, Oct 12, 2013 at 04:40:06AM +0100, Tang Yuantian-B29983 wrote: > > 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. > > Won't this break existing users DTBs? > This binding and DT should be merged first. For some reasons, the driver has already been merged. > > > > > > > > > > > > + > > > > > > +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-*. > > This may be somewhat pedantic, but an element from the list plus fixed- > clock isn't one or more of the following. It's one of the following plus > fixed-clock. > > It may be better to explicitly state that the compatible list must > include one of the following, plus fixed-clock. > Since the qoriq-sysclk-* is not 100% compatible with fixed-clock(no clock-frequency property), I will remove fixed-clock. > > > > > > > > + - "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. > > OK. The note at the bottom cna go, but I'd still prefer an explicit list > with a description of each entry. > OK, will give a list for them. > > > > > > > > > > > > + - "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. > > No it does not act like boo, a nd only appears to if you do not consider > the general casel. In general #clock-cells could be any arbitrarily large > number, not just <0> or <1>. It represents the number of cells in a > clock-specifier, not simply that there is a clock-specifier. > > It's perfectly possible to have #clock-cells = <2>, or more. It' > perfectly possible to have a DT like the below (with some properties > omitted for > brevity): > > clk_2: some-clock { > compatible = "vendor,banked-clock"; > #clock-cells = <2>; > }; > > clk_5: other-clock { > #clock-cells = <5>; > }; > > clk_0: another-clock { > #clock-cells = <0>; > }; > > consumer { > clocks = <&clk5 0 17 53 91 0>, > <&clk_0>, > <&clk2 3 17>; > }; > > In all of these cases, the cells in the clock-specifier must mean > something. > They uniquely identify a clock output of the clock provider, and there > must be a way of mapping them to a particular clock. > > The meaning of the cells in the clock specifier should be specified. > Consider "vendor,banked-clock". It's binding could look something like: > > - compatible: should contain > * "vendor,banked-clock" for v1 banked clock designs > - #clock-cells: Should be <2> > * The first cell selects the internal clock bank, indexed [1,3] > * The second cell selects a clock within the bank, indexed [0,25] > > A clock might have a set of named outputs: > > - #clock-cells: should be <1>, a single cell which may be one of the > following: > * 0 - REFCLKOUT > * 1 - PLLCLKOUT > * 5 - LOWPWRCLKOUT > > A clock provider might have a contiguous (or discontiguous) set of clock > indexes: > > - #clock-cells: should be <1>. The clock index as per the documentation, > in the range [0,15]. > > I hope this clears up the confusion on what I am asking for. > I understand now. There could be a clock-cells with the value of more than 1. That is just rarely used. In my case, #clock-cells should be <1> and I will add a list of values. > > > > > 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? > > Take a look at Documentation/devicetree/bindings/clock/imx23-clock.txt, > which specifies each clock output by name. You can also take a look at > the tegra clock bindings, which define the set of clocks by reference to > a device tree header file. > Got it. > > > > > > > > > > > > + > > > > > > +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, > > Cheers. > > [...] > > > > > > > + #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. > > OK. Should we not place it there for future? We're not compatible with > fixed-clock if we don't fulfil the minimum requirements of the fixed- > clock binding... > > > > > > > > > > > > > + 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? > > We have clock-names for a consumer's names of its inputs, and clock- > output-names for a provider's names of it's outputs. > > Consider a PLL device which takes a clock input (REFCLK) and outputs a > higher frequency clock (OUTCLK). The PLL's specification only talks in > terms of REFCLK and OUTCLK, but these are not the names of the clocks as > they are output from the original clock, or provided to the final > consumer. > > Consider a sequence of these PLLs plugged together. The OUTCLK of one > feeds into the REFCLK of the next: > > fixed: fixed-clock { > compatible = "fixed-clock"; > clock-frequency = <50>; > }; > > pll_0: pll { > compatible = "vendor,pll"; > clocks = <&fixed>; > clock-names = "refclk"; > clock-output-names = "outclk"; > }; > > pll_1: another-pll { > compatible = "vendor,pll"; > clocks = <&pll_0>; > clock-names = "refclk"; > clock-output-names = "outclk"; > }; > > consumer { > compatible = "vendor,clock-consumer"; > clocks = <&pll_1>, > <&pll_0>; > clock-names = "halfclk", "fullclk"; }; > > In each case, clock-names describes the clock inputs from the view of the > PLL they are input to, not from the provider they came from, or the > consumer some outputs are going to. Giving the inputs names in this way > makes it possible to describe situations where onlya subset of clock > inputs are wired up -- we could just have "fullclk" on the final consumer, > with only pll_0 as an input, and the driver could figure out what to do. > > Does that help? > Yes, it is very helpful. I will name clock-names better. Regards, Yuantian ��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
