Re: [v3] powerpc/mpc85xx: Update the clock device tree nodes
On Mon, 2013-08-26 at 21:49 -0500, Tang Yuantian-B29983 wrote:
+ };
+ pll1: pll1@820 {
+ #clock-cells = 1;
+ reg = 0x820;
+ compatible = fsl,core-pll-clock;
+ clocks = clockgen;
+ clock-output-names = pll1, pll1-div2, pll1-
div4;
+ };
Please leave a blank line between properties and nodes, and between
nodes.
OK, will add.
What does reg represent? Where is the binding for this?
The compatible is too vague.
Reg is register offset.
With no size?
No size is needed.
Yes, it is. Register blocks have size -- even if it's just a single
register.
It is too later to change since the clock driver is merged for months
although I sent this patch first.
It should not have gone in without an approved binding. It seems it went
in via Mike Turquette (why is a non-ARM-specific tree using linux-arm-
kernel as its list, BTW?). No ack from Ben, Kumar, or me is shown in the
commit.
The Linux common clock framework is not ARM specific. Any other arch can use
it.
Sure, it just seemed an odd choice of mailing list for something that
isn't ARM-specific.
In any case, you can preserve compatibility with existing trees without
using this compatible in new trees. The driver can check for both
compatibles, with a comment indicating that fsl,core-mux-clock is
deprecated and for compatibility only.
It is sub-clock node, is it really necessary to think about compatibility?
I think that's the node clockgen's responsibility.
It describes registers, so yes, you need to consider compatibility. A
clock provider is not responsible for figuring out how to program
devices that consume its clocks, nor should it make any assumptions
about such devices.
Besides, it is not too bad because other arch use the similar name.
I don't follow. This is a specific Freescale register interface, not a
general concept.
In any case, which similar names are you referring to? A search in
arch/arm/boot/dts for mux with clk or clock turns up
allwinner,sun4i-apb1-mux-clk which is much more specific than
fsl,core-mux-clock.
Ok, I will change the compatible string.
Do you think fsl,ppc-core-* is ok?
No. How about fsl,qoriq-chassis1-* (for e500mc/e5500) and
fsl,qoriq-chassis2-* (for e6500)?
-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [v3] powerpc/mpc85xx: Update the clock device tree nodes
OK, will update per your suggestions.
Thanks,
Yuantian
-Original Message-
From: Wood Scott-B07421
Sent: 2013年9月11日 星期三 5:47
To: Tang Yuantian-B29983
Cc: Wood Scott-B07421; ga...@kernel.crashing.org;
devicet...@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; Mike Turquette
Subject: Re: [v3] powerpc/mpc85xx: Update the clock device tree nodes
On Mon, 2013-08-26 at 21:49 -0500, Tang Yuantian-B29983 wrote:
+ };
+ pll1: pll1@820 {
+ #clock-cells = 1;
+ reg = 0x820;
+ compatible = fsl,core-pll-clock;
+ clocks = clockgen;
+ clock-output-names = pll1, pll1-div2, pll1-
div4;
+ };
Please leave a blank line between properties and nodes, and
between
nodes.
OK, will add.
What does reg represent? Where is the binding for this?
The compatible is too vague.
Reg is register offset.
With no size?
No size is needed.
Yes, it is. Register blocks have size -- even if it's just a single
register.
It is too later to change since the clock driver is merged for
months although I sent this patch first.
It should not have gone in without an approved binding. It seems it
went in via Mike Turquette (why is a non-ARM-specific tree using
linux-arm- kernel as its list, BTW?). No ack from Ben, Kumar, or me
is shown in the commit.
The Linux common clock framework is not ARM specific. Any other arch
can use it.
Sure, it just seemed an odd choice of mailing list for something that
isn't ARM-specific.
In any case, you can preserve compatibility with existing trees
without using this compatible in new trees. The driver can check
for both compatibles, with a comment indicating that
fsl,core-mux-clock is deprecated and for compatibility only.
It is sub-clock node, is it really necessary to think about
compatibility?
I think that's the node clockgen's responsibility.
It describes registers, so yes, you need to consider compatibility. A
clock provider is not responsible for figuring out how to program devices
that consume its clocks, nor should it make any assumptions about such
devices.
Besides, it is not too bad because other arch use the similar name.
I don't follow. This is a specific Freescale register interface,
not a general concept.
In any case, which similar names are you referring to? A search
in arch/arm/boot/dts for mux with clk or clock turns up
allwinner,sun4i-apb1-mux-clk which is much more specific than
fsl,core-mux-clock.
Ok, I will change the compatible string.
Do you think fsl,ppc-core-* is ok?
No. How about fsl,qoriq-chassis1-* (for e500mc/e5500) and fsl,qoriq-
chassis2-* (for e6500)?
-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [v3] powerpc/mpc85xx: Update the clock device tree nodes
On Sun, 2013-08-25 at 21:42 -0500, Tang Yuantian-B29983 wrote:
clockgen: global-utilities@e1000 {
- compatible = fsl,b4420-clockgen, fsl,qoriq-clockgen-2.0;
+ compatible = fsl,b4420-clockgen, fsl,qoriq-clockgen-2.0,
+fixed-clock;
+ clock-output-names = sysclk;
+ #clock-cells = 0;
Does U-Boot fill in clock-frequency here?
Yes, clock-frequency will be filled by uboot.
You suggested we'd better not add it here.
Right -- I was just making sure.
+ #address-cells = 1;
+ #size-cells = 0;
+ pll0: pll0@800 {
+ #clock-cells = 1;
+ reg = 0x800;
+ compatible = fsl,core-pll-clock;
+ clocks = clockgen;
+ clock-output-names = pll0, pll0-div2, pll0-div4;
+ };
+ pll1: pll1@820 {
+ #clock-cells = 1;
+ reg = 0x820;
+ compatible = fsl,core-pll-clock;
+ clocks = clockgen;
+ clock-output-names = pll1, pll1-div2, pll1-div4;
+ };
Please leave a blank line between properties and nodes, and between nodes.
OK, will add.
What does reg represent? Where is the binding for this?
The compatible is too vague.
Reg is register offset.
With no size?
I should have had a binding document.
About the compatible, you should pointed it out earlier in SDK review.
Sorry, it doesn't work that way. I don't know why I didn't notice this
stuff there -- the SDK review was probably rushed, with someone shouting
urgent. The SDK does not dictate what goes upstream. Device tree
bindings should go upstream first.
It is too later to change since the clock driver is merged for months
although
I sent this patch first.
It should not have gone in without an approved binding. It seems it
went in via Mike Turquette (why is a non-ARM-specific tree using
linux-arm-kernel as its list, BTW?). No ack from Ben, Kumar, or me is
shown in the commit.
In any case, you can preserve compatibility with existing trees without
using this compatible in new trees. The driver can check for both
compatibles, with a comment indicating that fsl,core-mux-clock is
deprecated and for compatibility only.
Besides, it is not too bad because other arch use the similar name.
I don't follow. This is a specific Freescale register interface, not a
general concept.
In any case, which similar names are you referring to? A search in
arch/arm/boot/dts for mux with clk or clock turns up
allwinner,sun4i-apb1-mux-clk which is much more specific than
fsl,core-mux-clock.
-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [v3] powerpc/mpc85xx: Update the clock device tree nodes
+ };
+ pll1: pll1@820 {
+ #clock-cells = 1;
+ reg = 0x820;
+ compatible = fsl,core-pll-clock;
+ clocks = clockgen;
+ clock-output-names = pll1, pll1-div2, pll1-
div4;
+ };
Please leave a blank line between properties and nodes, and between
nodes.
OK, will add.
What does reg represent? Where is the binding for this?
The compatible is too vague.
Reg is register offset.
With no size?
No size is needed.
I should have had a binding document.
About the compatible, you should pointed it out earlier in SDK review.
Sorry, it doesn't work that way. I don't know why I didn't notice this
stuff there -- the SDK review was probably rushed, with someone shouting
urgent. The SDK does not dictate what goes upstream. Device tree
bindings should go upstream first.
When I sent the patch v1, there is a binding document with it. But I missed
It in the patch v3, so when patch v3 got merged, the binding document didn't
get merged.
I will make the binding go upstream first next time.
It is too later to change since the clock driver is merged for months
although I sent this patch first.
It should not have gone in without an approved binding. It seems it went
in via Mike Turquette (why is a non-ARM-specific tree using linux-arm-
kernel as its list, BTW?). No ack from Ben, Kumar, or me is shown in the
commit.
The Linux common clock framework is not ARM specific. Any other arch can use it.
In fact, this clock driver is the first one that use common clk on PPC arch.
I will get the ack from you guys next time. I hope it doesn't make me wait too
long.
In any case, you can preserve compatibility with existing trees without
using this compatible in new trees. The driver can check for both
compatibles, with a comment indicating that fsl,core-mux-clock is
deprecated and for compatibility only.
It is sub-clock node, is it really necessary to think about compatibility?
I think that's the node clockgen's responsibility.
Besides, it is not too bad because other arch use the similar name.
I don't follow. This is a specific Freescale register interface, not a
general concept.
In any case, which similar names are you referring to? A search in
arch/arm/boot/dts for mux with clk or clock turns up
allwinner,sun4i-apb1-mux-clk which is much more specific than
fsl,core-mux-clock.
Ok, I will change the compatible string.
Do you think fsl,ppc-core-* is ok?
Regards,
Yuantian
-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [v3] powerpc/mpc85xx: Update the clock device tree nodes
clockgen: global-utilities@e1000 {
- compatible = fsl,b4420-clockgen, fsl,qoriq-clockgen-2.0;
+ compatible = fsl,b4420-clockgen, fsl,qoriq-clockgen-2.0,
+ fixed-clock;
+ clock-output-names = sysclk;
+ #clock-cells = 0;
Does U-Boot fill in clock-frequency here?
Yes, clock-frequency will be filled by uboot.
You suggested we'd better not add it here.
+ #address-cells = 1;
+ #size-cells = 0;
+ pll0: pll0@800 {
+ #clock-cells = 1;
+ reg = 0x800;
+ compatible = fsl,core-pll-clock;
+ clocks = clockgen;
+ clock-output-names = pll0, pll0-div2, pll0-div4;
+ };
+ pll1: pll1@820 {
+ #clock-cells = 1;
+ reg = 0x820;
+ compatible = fsl,core-pll-clock;
+ clocks = clockgen;
+ clock-output-names = pll1, pll1-div2, pll1-div4;
+ };
Please leave a blank line between properties and nodes, and between nodes.
OK, will add.
What does reg represent? Where is the binding for this?
The compatible is too vague.
Reg is register offset. I should have had a binding document.
About the compatible, you should pointed it out earlier in SDK review.
It is too later to change since the clock driver is merged for months although
I sent this patch first.
Besides, it is not too bad because other arch use the similar name.
Regards,
Yuantian
+ mux0: mux0@0 {
+ #clock-cells = 0;
+ reg = 0x0;
+ compatible = fsl,core-mux-clock;
+ clocks = pll0 0, pll0 1, pll0 2,
+pll1 0, pll1 1, pll1 2;
+ clock-names = pll0_0, pll0_1, pll0_2,
+ pll1_0, pll1_1, pll1_2;
+ clock-output-names = cmux0;
+ };
What does reg represent? Where is the binding for this?
The compatible is too vague.
-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [v3] powerpc/mpc85xx: Update the clock device tree nodes
On Thu, Jun 06, 2013 at 09:06:51AM +0800, tang yuantian 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
---
v3:
- fix typo
v2:
- add t4240, b4420, b4860 support
- remove pll/4 clock from p2041, p3041 and p5020 board
arch/powerpc/boot/dts/fsl/b4420si-post.dtsi | 32 -
arch/powerpc/boot/dts/fsl/b4420si-pre.dtsi | 2 +
arch/powerpc/boot/dts/fsl/b4860si-post.dtsi | 32 -
arch/powerpc/boot/dts/fsl/b4860si-pre.dtsi | 4 ++
arch/powerpc/boot/dts/fsl/p2041si-post.dtsi | 54 ++-
arch/powerpc/boot/dts/fsl/p2041si-pre.dtsi | 4 ++
arch/powerpc/boot/dts/fsl/p3041si-post.dtsi | 54 ++-
arch/powerpc/boot/dts/fsl/p3041si-pre.dtsi | 4 ++
arch/powerpc/boot/dts/fsl/p4080si-post.dtsi | 100
+++-
arch/powerpc/boot/dts/fsl/p4080si-pre.dtsi | 8 +++
arch/powerpc/boot/dts/fsl/p5020si-post.dtsi | 38 ++-
arch/powerpc/boot/dts/fsl/p5020si-pre.dtsi | 2 +
arch/powerpc/boot/dts/fsl/p5040si-post.dtsi | 54 ++-
arch/powerpc/boot/dts/fsl/p5040si-pre.dtsi | 4 ++
arch/powerpc/boot/dts/fsl/t4240si-post.dtsi | 77 -
arch/powerpc/boot/dts/fsl/t4240si-pre.dtsi | 12
16 files changed, 473 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/boot/dts/fsl/b4420si-post.dtsi
b/arch/powerpc/boot/dts/fsl/b4420si-post.dtsi
index 5a6615d..b69d6e5 100644
--- a/arch/powerpc/boot/dts/fsl/b4420si-post.dtsi
+++ b/arch/powerpc/boot/dts/fsl/b4420si-post.dtsi
@@ -85,7 +85,37 @@
};
clockgen: global-utilities@e1000 {
- compatible = fsl,b4420-clockgen, fsl,qoriq-clockgen-2.0;
+ compatible = fsl,b4420-clockgen, fsl,qoriq-clockgen-2.0,
+fixed-clock;
+ clock-output-names = sysclk;
+ #clock-cells = 0;
Does U-Boot fill in clock-frequency here?
+ #address-cells = 1;
+ #size-cells = 0;
+ pll0: pll0@800 {
+ #clock-cells = 1;
+ reg = 0x800;
+ compatible = fsl,core-pll-clock;
+ clocks = clockgen;
+ clock-output-names = pll0, pll0-div2, pll0-div4;
+ };
+ pll1: pll1@820 {
+ #clock-cells = 1;
+ reg = 0x820;
+ compatible = fsl,core-pll-clock;
+ clocks = clockgen;
+ clock-output-names = pll1, pll1-div2, pll1-div4;
+ };
Please leave a blank line between properties and nodes, and between
nodes.
What does reg represent? Where is the binding for this?
The compatible is too vague.
+ mux0: mux0@0 {
+ #clock-cells = 0;
+ reg = 0x0;
+ compatible = fsl,core-mux-clock;
+ clocks = pll0 0, pll0 1, pll0 2,
+ pll1 0, pll1 1, pll1 2;
+ clock-names = pll0_0, pll0_1, pll0_2,
+ pll1_0, pll1_1, pll1_2;
+ clock-output-names = cmux0;
+ };
What does reg represent? Where is the binding for this?
The compatible is too vague.
-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
