Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1272664 > unrolled thread
| Started by | Jon Mason <jonmason@broadcom.com> |
|---|---|
| First post | 2015-11-19 00:20 +0100 |
| Last post | 2015-11-19 17:30 +0100 |
| Articles | 5 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH 0/3] ARM: dts: add support for NS, NSP, and NS2 clocks Jon Mason <jonmason@broadcom.com> - 2015-11-19 00:20 +0100
[PATCH 2/3] ARM: dts: enable clock support for Broadcom NSP Jon Mason <jonmason@broadcom.com> - 2015-11-19 00:20 +0100
Re: [PATCH 2/3] ARM: dts: enable clock support for Broadcom NSP Ray Jui <rjui@broadcom.com> - 2015-11-19 01:00 +0100
Re: [PATCH 2/3] ARM: dts: enable clock support for Broadcom NSP Jon Mason <jonmason@broadcom.com> - 2015-11-19 16:50 +0100
Re: [PATCH 2/3] ARM: dts: enable clock support for Broadcom NSP Ray Jui <rjui@broadcom.com> - 2015-11-19 17:30 +0100
| From | Jon Mason <jonmason@broadcom.com> |
|---|---|
| Date | 2015-11-19 00:20 +0100 |
| Subject | [PATCH 0/3] ARM: dts: add support for NS, NSP, and NS2 clocks |
| Message-ID | <qwkb7-1kY-7@gated-at.bofh.it> |
This patch series adds device tree support for the Broadcom Northstar, Northstar Plus, and Northstar 2 clocks. Last sent as an RFC (see https://lkml.org/lkml/2015/10/13/882) due to the inability to merge because of the driver dependencies. Those necessary driver changes were merged into 4.4. All comments have been addressed and it is ready to be pulled in. Jon Mason (3): ARM: dts: enable clock support for BCM5301X ARM: dts: enable clock support for Broadcom NSP ARM64: dts: enable clock support for Broadcom NS2 arch/arm/boot/dts/bcm-nsp.dtsi | 99 ++++++++++++++++++++++++++--------- arch/arm/boot/dts/bcm5301x.dtsi | 92 ++++++++++++++++++++++++-------- arch/arm64/boot/dts/broadcom/ns2.dtsi | 80 +++++++++++++++++++++++++++- 3 files changed, 225 insertions(+), 46 deletions(-) -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Jon Mason <jonmason@broadcom.com> |
|---|---|
| Date | 2015-11-19 00:20 +0100 |
| Subject | [PATCH 2/3] ARM: dts: enable clock support for Broadcom NSP |
| Message-ID | <qwkb8-1kY-17@gated-at.bofh.it> |
| In reply to | #1272664 |
Replace current device tree dummy clocks with real clock support for
Broadcom Northstar Plus SoC
Signed-off-by: Jon Mason <jonmason@broadcom.com>
---
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>;
#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>;
+ };
};
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 {
+ #clock-cells = <1>;
+ compatible = "brcm,nsp-lcpll0";
+ reg = <0x03f100 0x14>;
+ clocks = <&osc>;
+ clock-output-names = "lcpll0", "pcie_phy", "sdio",
+ "ddr_phy";
+ };
+
+ genpll: genpll@1803f140 {
+ #clock-cells = <1>;
+ compatible = "brcm,nsp-genpll";
+ reg = <0x03f140 0x24>;
+ clocks = <&osc>;
+ clock-output-names = "genpll", "phy", "ethernetclk",
+ "usbclk", "iprocfast", "sata1",
+ "sata2";
+ };
};
};
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Ray Jui <rjui@broadcom.com> |
|---|---|
| Date | 2015-11-19 01:00 +0100 |
| Subject | Re: [PATCH 2/3] ARM: dts: enable clock support for Broadcom NSP |
| Message-ID | <qwkNQ-1An-11@gated-at.bofh.it> |
| In reply to | #1272665 |
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@broadcom.com>
> ---
> 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?
> #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.
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?
> };
>
> 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
> + #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
> + #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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Jon Mason <jonmason@broadcom.com> |
|---|---|
| Date | 2015-11-19 16:50 +0100 |
| Subject | Re: [PATCH 2/3] ARM: dts: enable clock support for Broadcom NSP |
| Message-ID | <qwzDc-2M9-39@gated-at.bofh.it> |
| In reply to | #1272691 |
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@broadcom.com>
> >---
> > 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.
>
> > #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.
>
> 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
>
>
> > };
> >
> > 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
>
> >+ #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
>
> >+ #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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Ray Jui <rjui@broadcom.com> |
|---|---|
| Date | 2015-11-19 17:30 +0100 |
| Subject | Re: [PATCH 2/3] ARM: dts: enable clock support for Broadcom NSP |
| Message-ID | <qwAfU-3gQ-11@gated-at.bofh.it> |
| In reply to | #1273239 |
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@broadcom.com>
>>> ---
>>> 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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web