Re: [PATCH 2/3] ARM: dts: enable clock support for Broadcom NSP

[Ray Jui <rjui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>]

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-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>> ---
>>>   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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html