RE: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree
Thanks for your review. -Original Message- From: Wood Scott-B07421 Sent: 2013年10月29日 星期二 11:26 To: Tang Yuantian-B29983 Cc: Wood Scott-B07421; Mark Rutland; devicet...@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; Li Yang-Leo-R58472 Subject: Re: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree On Sun, 2013-10-20 at 21:55 -0500, Tang Yuantian-B29983 wrote: I didn't see how your suggestion is a better matching. OSC PLL1 mux CPU | | |-- PLL2 --| As your suggestion, the clock tree looks like the above. In this case, the MUX driver will not know the divider details(/2, /4, or /3). When is there ever a /3? For T4, there is a /3, but it is used by PME not CPU. I think the MUX should act like switch which choose one of the input clock as a output clock. It should not CREATE clock(like PLL1/2, PLL1/4). The purpose of clock driver is to establish the clock tree. The clock tree will not be established in your suggestion because the divider is missing, we don't know where PLL/2 comes from. If you really like your proposal, it should be changed to this: OSC -- PLL1 - PLL1 /1 - MUX ---CPU ||___ PLL1 /2 ___| | | | PLL2 - PLL2 /2 ---| |___ PLL2/ 4 ___| (it is possible that PLLs have different divider). Do we actually have (or expect) a situation where the PLLs have different dividers, or even where the same bit setting in the MUX register means a different divider from one chip to another (within the same MUX compatible string)? If so, then I agree that we should go with your approach. For a specific chip, the dividers are same for each PLL. But CPU may use ONLY one of the dividers of a PLL. For example, on p4080, core0-3 may use PLL3/1, but do not use PLL3/2. (I didn't deal with this situation in driver either because there are limitations to use it). The way Freescale documents things in chip manuals rather than in block manuals, with little bits of information different in each chip manual, makes it hard to figure out this sort of thing. From the examples I looked at, it seemed pretty consistent that the low 2 bits of CLKSEL in the MUX were the log2 of the divider. Are there any chips that don't adhere to this? Your observation is correct until then. But there is no rule on this officially. Now I want to summary the pros and cons of your suggestion: The pros: 1. the device tree would be simpler. The cons: 1. cannot get the whole clock tree picture because dividers are hidden in driver. 2. no way to deal with the use case on p4080 3. need to deal with each chip-clockgen compatible string and pass a parameter to tell it what the divider is. 4. the log2 rule could be incorrect in future, say, we have a divider of /5. Any thoughts on this? Regards, Yuantian ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree
On Sun, 2013-10-20 at 21:55 -0500, Tang Yuantian-B29983 wrote: I didn't see how your suggestion is a better matching. OSC PLL1 mux CPU | | |-- PLL2 --| As your suggestion, the clock tree looks like the above. In this case, the MUX driver will not know the divider details(/2, /4, or /3). When is there ever a /3? I think the MUX should act like switch which choose one of the input clock as a output clock. It should not CREATE clock(like PLL1/2, PLL1/4). The purpose of clock driver is to establish the clock tree. The clock tree will not be established in your suggestion because the divider is missing, we don't know where PLL/2 comes from. If you really like your proposal, it should be changed to this: OSC -- PLL1 - PLL1 /1 - MUX ---CPU ||___ PLL1 /2 ___| | | | PLL2 - PLL2 /2 ---| |___ PLL2/ 4 ___| (it is possible that PLLs have different divider). Do we actually have (or expect) a situation where the PLLs have different dividers, or even where the same bit setting in the MUX register means a different divider from one chip to another (within the same MUX compatible string)? If so, then I agree that we should go with your approach. The way Freescale documents things in chip manuals rather than in block manuals, with little bits of information different in each chip manual, makes it hard to figure out this sort of thing. From the examples I looked at, it seemed pretty consistent that the low 2 bits of CLKSEL in the MUX were the log2 of the divider. Are there any chips that don't adhere to this? -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree
+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
+- reg: Offset and length of the clock register set
+- clock-frequency: Indicates input clock frequency of clock block.
+ Will be set by u-boot
+
+Recommended properties:
+- #ddress-cells: Specifies the number of cells used to represent
typo
Thanks, someone else already pointed out it. Will fix in next patch.
Regards,
Yuantian
+ 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
+
+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:
+ - 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
+ - fixed-clock: From common clock binding; indicates output clock
+ of oscillator
+ - fsl,qoriq-sysclk-[1,2].x: Indicates input system clock
+- #clock-cells: From common clock binding; indicates the number of
+ output clock. 0 is for one output clock; 1 for more than one clock
+
+Recommended properties:
+- clocks: Should be the phandle of input parent clock
+- clock-names: From common clock binding, indicates the clock name
+- 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.
Binding looks reasonable to me.
g.
+
+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;
+ #address-cells = 1;
+ #size-cells = 1;
+
+ sysclk: sysclk {
+ #clock-cells = 0;
+ compatible = fsl,qoriq-sysclk-1.0, fixed-clock;
+ 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;
+ clock-output-names = cmux1;
+ };
+ };
+ }
+
+Example for clock consumer:
+
+/ {
+ cpu0: PowerPC,e5500@0 {
+ ...
+ clocks = mux0;
+ ...
+ };
+ }
diff --git a/arch/powerpc/boot/dts/fsl/b4420si-post.dtsi
b/arch/powerpc/boot/dts/fsl/b4420si-post.dtsi
index 5a6615d..e910e82 100644
--- a/arch/powerpc/boot/dts/fsl/b4420si-post.dtsi
+++ b/arch/powerpc/boot/dts/fsl/b4420si-post.dtsi
@@ -86,6 +86,41 @@
clockgen: global-utilities@e1000 {
compatible = fsl,b4420-clockgen, fsl,qoriq-clockgen-2.0;
+ #address-cells = 1;
+ #size-cells = 1;
+
+ sysclk: sysclk {
+ #clock-cells = 0;
+ compatible = fsl,qoriq-sysclk-2.0, fixed-clock;
+ clock-output-names = sysclk;
+ }
+
+ pll0: pll0@800 {
+ #clock-cells = 1;
+ reg = 0x800 0x4;
+ compatible = fsl,qoriq-core-pll-2.0;
+ clocks = sysclk;
+ clock-output-names = pll0, pll0-div2, pll0-div4;
+ };
+
+
Re: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree
On Wed, 9 Oct 2013 14:38:24 +0800, 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 000..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
+- reg: Offset and length of the clock register set
+- clock-frequency: Indicates input clock frequency of clock block.
+ Will be set by u-boot
+
+Recommended properties:
+- #ddress-cells: Specifies the number of cells used to represent
typo
+ 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
+
+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:
+ - 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
+ - fixed-clock: From common clock binding; indicates output clock
+ of oscillator
+ - fsl,qoriq-sysclk-[1,2].x: Indicates input system clock
+- #clock-cells: From common clock binding; indicates the number of
+ output clock. 0 is for one output clock; 1 for more than one clock
+
+Recommended properties:
+- clocks: Should be the phandle of input parent clock
+- clock-names: From common clock binding, indicates the clock name
+- 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.
Binding looks reasonable to me.
g.
+
+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;
+ #address-cells = 1;
+ #size-cells = 1;
+
+ sysclk: sysclk {
+ #clock-cells = 0;
+ compatible = fsl,qoriq-sysclk-1.0, fixed-clock;
+
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?
+
+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.
+ - 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.
+ - 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
RE: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree
Thanks for your review. -Original Message- From: Mark Rutland [mailto:mark.rutl...@arm.com] Sent: 2013年10月21日 星期一 17:15 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 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
RE: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree
It's still selecting from multiple PLLs. I don't know whether divider module exists or not. If it exists, it should be part of PLL or between PLL and MUX. wherever it was, the device tree binding is appropriate. The device tree binding is unnecessarily complicated. The P3041RM shows exactly each PLL has 2 outputs which definitely have no divider at all. That diagram is a bit weird -- one of the outputs is used as is, and the other is split into 1/2 and 1/4. It doesn't really matter though; the end result is the same. We're describing the programming interface, not artwork choices in the manual. -Scott If the device tree needs to be modified, could you give me your suggestions? My suggestion is to have only one output per PLL node. The MUX node would have one input per PLL. Dividers would be handled internally to the MUX driver. -Scott I don't think your suggestion is constructive. Hmm? Your suggestion is on the premise of that the divider is part of MUX, And I think it should be part of PLL. MUX can't CREATE clock which PLL can. So my thought is more reasonable. If the device tree documents like what you said, it would have few help for driver. The reason is obvious that the driver is still going to deal with the divider detail which varies from platform to platform. I will document as it was unless you prove your suggestion. I can't follow this. My point is that my suggestion better matches the programming model, and is simpler. -Scott I didn't see how your suggestion is a better matching. OSC PLL1 mux CPU | | |-- PLL2 --| As your suggestion, the clock tree looks like the above. In this case, the MUX driver will not know the divider details(/2, /4, or /3). I think the MUX should act like switch which choose one of the input clock as a output clock. It should not CREATE clock(like PLL1/2, PLL1/4). The purpose of clock driver is to establish the clock tree. The clock tree will not be established in your suggestion because the divider is missing, we don't know where PLL/2 comes from. If you really like your proposal, it should be changed to this: OSC -- PLL1 - PLL1 /1 - MUX ---CPU ||___ PLL1 /2 ___| | | | PLL2 - PLL2 /2 ---| |___ PLL2/ 4 ___| (it is possible that PLLs have different divider). Regards, Yuantian ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree
On Thu, 2013-10-17 at 21:06 -0500, Tang Yuantian-B29983 wrote: On Wed, 2013-10-16 at 21:08 -0500, Tang Yuantian-B29983 wrote: That shows the dividers as being somewhere in between the PLL and the MUX. The MUX is where the divider is selected. There's nothing in the PLL's programming interface that relates to the dividers. As such it's simpler to model it as being part of the MUX. -Scott I don't know whether it is simpler, but modeling divider as being part of the MUX is your guess, right? If the divider is included in MUX, the MUX would not be called MUX. It's still selecting from multiple PLLs. I don't know whether divider module exists or not. If it exists, it should be part of PLL or between PLL and MUX. wherever it was, the device tree binding is appropriate. The device tree binding is unnecessarily complicated. The P3041RM shows exactly each PLL has 2 outputs which definitely have no divider at all. That diagram is a bit weird -- one of the outputs is used as is, and the other is split into 1/2 and 1/4. It doesn't really matter though; the end result is the same. We're describing the programming interface, not artwork choices in the manual. -Scott If the device tree needs to be modified, could you give me your suggestions? My suggestion is to have only one output per PLL node. The MUX node would have one input per PLL. Dividers would be handled internally to the MUX driver. -Scott I don't think your suggestion is constructive. Hmm? Your suggestion is on the premise of that the divider is part of MUX, And I think it should be part of PLL. MUX can't CREATE clock which PLL can. So my thought is more reasonable. If the device tree documents like what you said, it would have few help for driver. The reason is obvious that the driver is still going to deal with the divider detail which varies from platform to platform. I will document as it was unless you prove your suggestion. I can't follow this. My point is that my suggestion better matches the programming model, and is simpler. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree
On Wed, 2013-10-16 at 21:08 -0500, Tang Yuantian-B29983 wrote: That shows the dividers as being somewhere in between the PLL and the MUX. The MUX is where the divider is selected. There's nothing in the PLL's programming interface that relates to the dividers. As such it's simpler to model it as being part of the MUX. -Scott I don't know whether it is simpler, but modeling divider as being part of the MUX is your guess, right? If the divider is included in MUX, the MUX would not be called MUX. It's still selecting from multiple PLLs. I don't know whether divider module exists or not. If it exists, it should be part of PLL or between PLL and MUX. wherever it was, the device tree binding is appropriate. The device tree binding is unnecessarily complicated. The P3041RM shows exactly each PLL has 2 outputs which definitely have no divider at all. That diagram is a bit weird -- one of the outputs is used as is, and the other is split into 1/2 and 1/4. It doesn't really matter though; the end result is the same. We're describing the programming interface, not artwork choices in the manual. -Scott If the device tree needs to be modified, could you give me your suggestions? My suggestion is to have only one output per PLL node. The MUX node would have one input per PLL. Dividers would be handled internally to the MUX driver. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree
On Wed, 2013-10-16 at 21:08 -0500, Tang Yuantian-B29983 wrote: That shows the dividers as being somewhere in between the PLL and the MUX. The MUX is where the divider is selected. There's nothing in the PLL's programming interface that relates to the dividers. As such it's simpler to model it as being part of the MUX. -Scott I don't know whether it is simpler, but modeling divider as being part of the MUX is your guess, right? If the divider is included in MUX, the MUX would not be called MUX. It's still selecting from multiple PLLs. I don't know whether divider module exists or not. If it exists, it should be part of PLL or between PLL and MUX. wherever it was, the device tree binding is appropriate. The device tree binding is unnecessarily complicated. The P3041RM shows exactly each PLL has 2 outputs which definitely have no divider at all. That diagram is a bit weird -- one of the outputs is used as is, and the other is split into 1/2 and 1/4. It doesn't really matter though; the end result is the same. We're describing the programming interface, not artwork choices in the manual. -Scott If the device tree needs to be modified, could you give me your suggestions? My suggestion is to have only one output per PLL node. The MUX node would have one input per PLL. Dividers would be handled internally to the MUX driver. -Scott I don't think your suggestion is constructive. Your suggestion is on the premise of that the divider is part of MUX, And I think it should be part of PLL. MUX can't CREATE clock which PLL can. So my thought is more reasonable. If the device tree documents like what you said, it would have few help for driver. The reason is obvious that the driver is still going to deal with the divider detail which varies from platform to platform. I will document as it was unless you prove your suggestion. Thanks, Yuantian ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree
On Tue, 2013-10-15 at 21:57 -0500, Tang Yuantian-B29983 wrote: The device tree makes that quite clear. You chose to model it that way in the device tree; that doesn't make it clear that the hardware works that way or that it's a good way to model it. Each PLL has several output which MUX node can take from. Point out where in the hardware documentation it says this. What I see is a PLL that has one output, and a MUX register that can choose from multiple PLL and divider options. Take T4240 for example: see section 4.6.5.1 , (Page 141) in T4240RM Rev. D, 09/2012. That shows the dividers as being somewhere in between the PLL and the MUX. The MUX is where the divider is selected. There's nothing in the PLL's programming interface that relates to the dividers. As such it's simpler to model it as being part of the MUX. -Scott I don't know whether it is simpler, but modeling divider as being part of the MUX is your guess, right? If the divider is included in MUX, the MUX would not be called MUX. It's still selecting from multiple PLLs. I don't know whether divider module exists or not. If it exists, it should be part of PLL or between PLL and MUX. wherever it was, the device tree binding is appropriate. The device tree binding is unnecessarily complicated. The P3041RM shows exactly each PLL has 2 outputs which definitely have no divider at all. That diagram is a bit weird -- one of the outputs is used as is, and the other is split into 1/2 and 1/4. It doesn't really matter though; the end result is the same. We're describing the programming interface, not artwork choices in the manual. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree
That shows the dividers as being somewhere in between the PLL and the MUX. The MUX is where the divider is selected. There's nothing in the PLL's programming interface that relates to the dividers. As such it's simpler to model it as being part of the MUX. -Scott I don't know whether it is simpler, but modeling divider as being part of the MUX is your guess, right? If the divider is included in MUX, the MUX would not be called MUX. It's still selecting from multiple PLLs. I don't know whether divider module exists or not. If it exists, it should be part of PLL or between PLL and MUX. wherever it was, the device tree binding is appropriate. The device tree binding is unnecessarily complicated. The P3041RM shows exactly each PLL has 2 outputs which definitely have no divider at all. That diagram is a bit weird -- one of the outputs is used as is, and the other is split into 1/2 and 1/4. It doesn't really matter though; the end result is the same. We're describing the programming interface, not artwork choices in the manual. -Scott If the device tree needs to be modified, could you give me your suggestions? Regards, Yuantian ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree
On Mon, 2013-10-14 at 20:59 -0500, Tang Yuantian-B29983 wrote: -Original Message- From: Wood Scott-B07421 Sent: 2013年10月15日 星期二 6:13 To: Tang Yuantian-B29983 Cc: Wood Scott-B07421; Mark Rutland; devicet...@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; Li Yang-Leo-R58472 Subject: Re: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree On Fri, 2013-10-11 at 21:52 -0500, Tang Yuantian-B29983 wrote: Thanks for your review. -Original Message- From: Wood Scott-B07421 Sent: 2013年10月12日 星期六 3:07 To: Mark Rutland Cc: Tang Yuantian-B29983; devicet...@vger.kernel.org; linuxppc- d...@lists.ozlabs.org; Li Yang-Leo-R58472 Subject: Re: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree 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. You chose to model it that way in the device tree; that doesn't make it clear that the hardware works that way or that it's a good way to model it. Each PLL has several output which MUX node can take from. Point out where in the hardware documentation it says this. What I see is a PLL that has one output, and a MUX register that can choose from multiple PLL and divider options. Take T4240 for example: see section 4.6.5.1 , (Page 141) in T4240RM Rev. D, 09/2012. That shows the dividers as being somewhere in between the PLL and the MUX. The MUX is where the divider is selected. There's nothing in the PLL's programming interface that relates to the dividers. As such it's simpler to model it as being part of the MUX. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree
On Fri, 2013-10-11 at 21:52 -0500, Tang Yuantian-B29983 wrote: Thanks for your review. -Original Message- From: Wood Scott-B07421 Sent: 2013年10月12日 星期六 3:07 To: Mark Rutland Cc: Tang Yuantian-B29983; devicet...@vger.kernel.org; linuxppc- d...@lists.ozlabs.org; Li Yang-Leo-R58472 Subject: Re: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree 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. You chose to model it that way in the device tree; that doesn't make it clear that the hardware works that way or that it's a good way to model it. Each PLL has several output which MUX node can take from. Point out where in the hardware documentation it says this. What I see is a PLL that has one output, and a MUX register that can choose from multiple PLL and divider options. It is not a runtime decision. Hmm? It's a register you write to. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree
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 000..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
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.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 000..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
Re: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree
On Wed, 2013-10-09 at 14:38 +0800, yuantian.t...@freescale.com wrote:
From: Tang Yuantian yuantian.tang@frovider:
+/ {
+ clockgen: global-utilities@e1000 {
+ compatible = fsl,p5020-clockgen, fsl,qoriq-clockgen-1.0;
+ reg = 0xe1000 0x1000;
+ clock-frequency = 0;
+ #address-cells = 1;
+ #size-cells = 1;
+
+ sysclk: sysclk {
+ #clock-cells = 0;
+ compatible = fsl,qoriq-sysclk-1.0, fixed-clock;
+ 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;
+ };
Where is ranges in the global-utilities node?
-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree
Thanks for your review. -Original Message- From: Wood Scott-B07421 Sent: 2013年10月12日 星期六 3:07 To: Mark Rutland Cc: Tang Yuantian-B29983; devicet...@vger.kernel.org; linuxppc- d...@lists.ozlabs.org; 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.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 000..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
RE: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree
Thanks for your review.
-Original Message-
From: Wood Scott-B07421
Sent: 2013年10月12日 星期六 3:08
To: Tang Yuantian-B29983
Cc: ga...@kernel.crashing.org; devicet...@vger.kernel.org; linuxppc-
d...@lists.ozlabs.org
Subject: Re: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device
tree
On Wed, 2013-10-09 at 14:38 +0800, yuantian.t...@freescale.com wrote:
From: Tang Yuantian yuantian.tang@frovider:
+/ {
+ clockgen: global-utilities@e1000 {
+ compatible = fsl,p5020-clockgen, fsl,qoriq-clockgen-1.0;
+ reg = 0xe1000 0x1000;
+ clock-frequency = 0;
+ #address-cells = 1;
+ #size-cells = 1;
+
+ sysclk: sysclk {
+ #clock-cells = 0;
+ compatible = fsl,qoriq-sysclk-1.0, fixed-clock;
+ 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;
+ };
Where is ranges in the global-utilities node?
Will add it thanks.
Regards,
Yuantian
-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree
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
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 000..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. +- 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? + +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. +- #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. + +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: + - 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. + - fixed-clock: From common clock binding; indicates output clock + of oscillator
RE: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree
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 000..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. +- 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 + +2. Clock Provider/Consumer Binding + +Most of the binding are from the common
[PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree
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 000..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
+- reg: Offset and length of the clock register set
+- clock-frequency: Indicates input clock frequency of clock block.
+ Will be set by u-boot
+
+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
+- #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
+
+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:
+ - 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
+ - fixed-clock: From common clock binding; indicates output clock
+ of oscillator
+ - fsl,qoriq-sysclk-[1,2].x: Indicates input system clock
+- #clock-cells: From common clock binding; indicates the number of
+ output clock. 0 is for one output clock; 1 for more than one clock
+
+Recommended properties:
+- clocks: Should be the phandle of input parent clock
+- clock-names: From common clock binding, indicates the clock name
+- 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;
+ #address-cells = 1;
+ #size-cells = 1;
+
+ sysclk: sysclk {
+ #clock-cells = 0;
+ compatible = fsl,qoriq-sysclk-1.0, fixed-clock;
+ clock-output-names = sysclk;
+ }
+
+ pll0: pll0@800 {
+ #clock-cells = 1;
+ reg = 0x800
