Re: [PATCH 2/3] ARM: dts: enable clock support for Broadcom NSP
From: Ray Jui
Date: Thu Nov 19 2015 - 11:25:28 EST
On 11/19/2015 7:48 AM, Jon Mason wrote:
On Wed, Nov 18, 2015 at 03:57:36PM -0800, Ray Jui wrote:
Would this patch merge properly with the other NSP DT clean up patch
+ I2C DT patch that you worked out internally but have not sent out?
I thought it's going to make the maintainers' life easier if you can
group DT changes per platform and send them out in the same series.
I also have some inline comments below.
On 11/18/2015 3:13 PM, Jon Mason wrote:
Replace current device tree dummy clocks with real clock support for
Broadcom Northstar Plus SoC
Signed-off-by: Jon Mason <jonmason@xxxxxxxxxxxx>
---
arch/arm/boot/dts/bcm-nsp.dtsi | 99 ++++++++++++++++++++++++++++++++----------
1 file changed, 75 insertions(+), 24 deletions(-)
diff --git a/arch/arm/boot/dts/bcm-nsp.dtsi b/arch/arm/boot/dts/bcm-nsp.dtsi
index 4bcdd28..f85a4f1 100644
--- a/arch/arm/boot/dts/bcm-nsp.dtsi
+++ b/arch/arm/boot/dts/bcm-nsp.dtsi
@@ -32,6 +32,7 @@
#include <dt-bindings/interrupt-controller/arm-gic.h>
#include <dt-bindings/interrupt-controller/irq.h>
+#include <dt-bindings/clock/bcm-nsp.h>
#include "skeleton.dtsi"
@@ -42,7 +43,7 @@
mpcore {
compatible = "simple-bus";
- ranges = <0x00000000 0x19020000 0x00003000>;
+ ranges = <0x00000000 0x19000000 0x00023000>;
Why does this have anything to do with clocks? Shouldn't it be a
separate patch?
No, this is correct (though the patch is a little odd to look at).
The a9pll starts at 0x19000000 instead of 0x19020000. So, everything
needs to be adjusted.
Okay.
#address-cells = <1>;
#size-cells = <1>;
@@ -58,32 +59,23 @@
};
};
- L2: l2-cache {
- compatible = "arm,pl310-cache";
- reg = <0x2000 0x1000>;
- cache-unified;
- cache-level = <2>;
- };
-
- gic: interrupt-controller@19021000 {
- compatible = "arm,cortex-a9-gic";
- #interrupt-cells = <3>;
- #address-cells = <0>;
- interrupt-controller;
- reg = <0x1000 0x1000>,
- <0x0100 0x100>;
+ a9pll: arm_clk@19000000 {
+ #clock-cells = <0>;
+ compatible = "brcm,nsp-armpll";
+ clocks = <&osc>;
+ reg = <0x00000 0x1000>;
};
timer@19020200 {
compatible = "arm,cortex-a9-global-timer";
- reg = <0x0200 0x100>;
+ reg = <0x20200 0x100>;
interrupts = <GIC_PPI 11 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&periph_clk>;
};
twd-timer@19020600 {
compatible = "arm,cortex-a9-twd-timer";
- reg = <0x0600 0x20>;
+ reg = <0x20600 0x20>;
interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(2) |
IRQ_TYPE_LEVEL_HIGH)>;
clocks = <&periph_clk>;
@@ -91,11 +83,27 @@
twd-watchdog@19020620 {
compatible = "arm,cortex-a9-twd-wdt";
- reg = <0x0620 0x20>;
+ reg = <0x20620 0x20>;
interrupts = <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(2) |
IRQ_TYPE_LEVEL_HIGH)>;
clocks = <&periph_clk>;
};
+
+ gic: interrupt-controller@19021000 {
+ compatible = "arm,cortex-a9-gic";
+ #interrupt-cells = <3>;
+ #address-cells = <0>;
+ interrupt-controller;
+ reg = <0x21000 0x1000>,
+ <0x20100 0x100>;
+ };
+
+ L2: l2-cache {
+ compatible = "arm,pl310-cache";
+ reg = <0x22000 0x1000>;
+ cache-unified;
+ cache-level = <2>;
+ };
From here and above, all labels are wrong. You are moving them into
a bus that has translated bus addresses, but you still use absolute
addresses for all labels.
Please fix all the labels.
And again, 1) Why is this change imbedded in a patch meant for
adding DT clock support according to the commit message; 2) how does
the dependency work with the other patches that you are about to
send out?
This was already discussed in the original series. See
http://www.spinics.net/lists/arm-kernel/msg451580.html
The discussion explains my first question. But what about my second
question? How does the dependency work with other NSP DT patches that
you have on hand? Will there be conflicts? If so, do you expect the
maintainers need to manually fix all the conflicts?
};
clocks {
@@ -103,10 +111,34 @@
#size-cells = <1>;
ranges;
- periph_clk: periph_clk {
+ osc: oscillator {
+ #clock-cells = <0>;
compatible = "fixed-clock";
+ clock-frequency = <25000000>;
+ };
+
+ iprocmed: iprocmed {
+ #clock-cells = <0>;
+ compatible = "fixed-factor-clock";
+ clocks = <&genpll BCM_NSP_GENPLL_IPROCFAST_CLK>;
+ clock-div = <2>;
+ clock-mult = <1>;
+ };
+
+ iprocslow: iprocslow {
+ #clock-cells = <0>;
+ compatible = "fixed-factor-clock";
+ clocks = <&genpll BCM_NSP_GENPLL_IPROCFAST_CLK>;
+ clock-div = <4>;
+ clock-mult = <1>;
+ };
+
+ periph_clk: periph_clk {
#clock-cells = <0>;
- clock-frequency = <500000000>;
+ compatible = "fixed-factor-clock";
+ clocks = <&a9pll>;
+ clock-div = <2>;
+ clock-mult = <1>;
};
};
@@ -118,17 +150,17 @@
uart0: serial@18000300 {
compatible = "ns16550a";
- reg = <0x0300 0x100>;
+ reg = <0x000300 0x100>;
interrupts = <GIC_SPI 85 IRQ_TYPE_LEVEL_HIGH>;
- clock-frequency = <62499840>;
+ clocks = <&osc>;
status = "disabled";
};
uart1: serial@18000400 {
compatible = "ns16550a";
- reg = <0x0400 0x100>;
+ reg = <0x000400 0x100>;
interrupts = <GIC_SPI 85 IRQ_TYPE_LEVEL_HIGH>;
- clock-frequency = <62499840>;
+ clocks = <&osc>;
status = "disabled";
};
@@ -217,5 +249,24 @@
brcm,nand-has-wp;
};
+
+ lcpll0: lcpll0@1803f100 {
wrong label
Please fix
+ #clock-cells = <1>;
+ compatible = "brcm,nsp-lcpll0";
+ reg = <0x03f100 0x14>;
+ clocks = <&osc>;
+ clock-output-names = "lcpll0", "pcie_phy", "sdio",
+ "ddr_phy";
+ };
+
+ genpll: genpll@1803f140 {
wrong label
Please fix
+ #clock-cells = <1>;
+ compatible = "brcm,nsp-genpll";
+ reg = <0x03f140 0x24>;
+ clocks = <&osc>;
+ clock-output-names = "genpll", "phy", "ethernetclk",
+ "usbclk", "iprocfast", "sata1",
+ "sata2";
+ };
};
};
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/