Thanks for your review. > -----Original Message----- > From: Wood Scott-B07421 > Sent: 2013年10月12日 星期六 3:07 > To: Mark Rutland > Cc: Tang Yuantian-B29983; devicetree@xxxxxxxxxxxxxxx; linuxppc- > dev@xxxxxxxxxxxxxxxx; Li Yang-Leo-R58472 > Subject: Re: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device > tree > > 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: > > > Thanks for your review. > > > See my reply inline > > > > > > > -----Original Message----- > > > > From: Mark Rutland [mailto:mark.rutland@xxxxxxx] > > > > Sent: 2013年10月10日 星期四 18:04 > > > > 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 Wed, Oct 09, 2013 at 07:38:24AM +0100, > > > > Yuantian.Tang@xxxxxxxxxxxxx > > > > wrote: > > > > > From: Tang Yuantian <yuantian.tang@xxxxxxxxxxxxx> > > > > > > > > > > The following SoCs will be affected: p2041, p3041, p4080, p5020, > > > > > p5040, b4420, b4860, t4240 > > > > > > > > > > Signed-off-by: Tang Yuantian <Yuantian.Tang@xxxxxxxxxxxxx> > > > > > Signed-off-by: Li Yang <leoli@xxxxxxxxxxxxx> > > > > > --- > > > > > 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. > > > > Thanks. > > > > > > > > > > +- 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... > > > > > > > > > > + > > > > > +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. > > > > > > > + - "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. > > 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). > > > > > > +- 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... > > The actual frequency is patched in by U-Boot -- and moving it to a > different node would break this. > > > > > > + #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? > > 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... > As you pointed out before, we can't combine the global-utilities node and the fixed-clock node into one node, because these two nodes have different properties which are not compatible each other. > 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. > This tiny driver is still needed to get the clock frequency from parent node. That is what I am going to change with current driver. But we still need to add the fixed-clock node in order to provide clock source to PLL nodes. > > > > > + 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. > > 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? > The device tree makes that quite clear. Each PLL has several output which MUX node can take from. It is not a runtime decision. Regards, Yuantian > -Scott > ��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
