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.rutl...@arm.com]
> > > Sent: 2013年10月10日 星期四 18:04
> > > To: Tang Yuantian-B29983
> > > Cc: ga...@kernel.crashing.org; linuxppc-dev@lists.ozlabs.org;
> > > devicet...@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.t...@freescale.com
> > > wrote:
> > > > From: Tang Yuantian <yuantian.t...@freescale.com>
> > > >
> > > > The following SoCs will be affected: p2041, p3041, p4080, p5020,
> > > > p5040, b4420, b4860, t4240
> > > >
> > > > Signed-off-by: Tang Yuantian <yuantian.t...@freescale.com>
> > > > Signed-off-by: Li Yang <le...@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.
>
> 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...
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.
> > > > + 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?
-Scott
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
