diff mbox

[v3,10/12] ARM: mvebu: Relocate Armada 370/XP DeviceBus device tree nodes

Message ID 1371554737-25319-11-git-send-email-ezequiel.garcia@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ezequiel Garcia June 18, 2013, 11:25 a.m. UTC 
Now that mbus has been added to the device tree, it's possible to
move the DeviceBus out of internal registers, placing it directly
below the mbus. This is a more accurate representation of the hardware.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/boot/dts/armada-370-xp.dtsi             | 94 +++++++++++++-----------
 arch/arm/boot/dts/armada-xp-gp.dts               | 60 +++++++--------
 arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts | 60 +++++++--------
 3 files changed, 110 insertions(+), 104 deletions(-)

Comments

Arnd Bergmann June 18, 2013, 4:16 p.m. UTC | #1 
On Tuesday 18 June 2013, Ezequiel Garcia wrote:
> 
> +               devbus-bootcs {
> +                       compatible = "marvell,mvebu-devbus";
> +                       reg = <0xffff0001 0x10400 0x8>;
> +                       ranges = <0 MBUS_ID(0x01, 0x2f) 0 0xffffffff>;
> +                       #address-cells = <1>;
> +                       #size-cells = <1>;
> +                       clocks = <&coreclk 0>;
> +                       status = "disabled";
> +               };

This is a violation of the binding as far as I can tell, since you don't specify ranges
to access the 0xffff0001 0x10400 address. However, as I explained in my comment for
the binding, I think you should clarify the binding and leave the implementation
as you have it here.

	Arnd
Ezequiel Garcia June 18, 2013, 10:09 p.m. UTC | #2 
On Tue, Jun 18, 2013 at 06:16:26PM +0200, Arnd Bergmann wrote:
> On Tuesday 18 June 2013, Ezequiel Garcia wrote:
> > 
> > +               devbus-bootcs {
> > +                       compatible = "marvell,mvebu-devbus";
> > +                       reg = <0xffff0001 0x10400 0x8>;
> > +                       ranges = <0 MBUS_ID(0x01, 0x2f) 0 0xffffffff>;
> > +                       #address-cells = <1>;
> > +                       #size-cells = <1>;
> > +                       clocks = <&coreclk 0>;
> > +                       status = "disabled";
> > +               };
> 
> This is a violation of the binding as far as I can tell, since you don't specify ranges
> to access the 0xffff0001 0x10400 address. However, as I explained in my comment for
> the binding, I think you should clarify the binding and leave the implementation
> as you have it here.
> 

Mmm... again I got lost here. Which 'ranges' you say I don't specify to
access the (formerly) 0xffff0001?

AFAIK, 'ranges' are only for children translation, which means I don't
need to specify a ranges for that in the devbus node, but in its parent,
right?

This ranges thing can be very tricky, so please correct me if I'm
mistaken.
Ezequiel Garcia June 18, 2013, 10:14 p.m. UTC | #3 
On Tue, Jun 18, 2013 at 07:09:29PM -0300, Ezequiel Garcia wrote:
> On Tue, Jun 18, 2013 at 06:16:26PM +0200, Arnd Bergmann wrote:
> > On Tuesday 18 June 2013, Ezequiel Garcia wrote:
> > > 
> > > +               devbus-bootcs {
> > > +                       compatible = "marvell,mvebu-devbus";
> > > +                       reg = <0xffff0001 0x10400 0x8>;
> > > +                       ranges = <0 MBUS_ID(0x01, 0x2f) 0 0xffffffff>;
> > > +                       #address-cells = <1>;
> > > +                       #size-cells = <1>;
> > > +                       clocks = <&coreclk 0>;
> > > +                       status = "disabled";
> > > +               };
> > 
> > This is a violation of the binding as far as I can tell, since you don't specify ranges
> > to access the 0xffff0001 0x10400 address. However, as I explained in my comment for
> > the binding, I think you should clarify the binding and leave the implementation
> > as you have it here.
> > 
> 
> Mmm... again I got lost here. Which 'ranges' you say I don't specify to
> access the (formerly) 0xffff0001?
> 

Oh, maybe you meant I'm not specifying an mbus-node ranges translation
in this same patch in this .dtsi file I'm modifying?

In that case, that's on purpose to avoid the nightmare involved in
mixing 'ranges' in per-board .dts files together with the ranges
declared in each included .dtsi.

By having only one mbus-node ranges property, per-board, it gets a bit
simpler.
Arnd Bergmann June 19, 2013, 12:03 p.m. UTC | #4 
On Wednesday 19 June 2013, Ezequiel Garcia wrote:
> On Tue, Jun 18, 2013 at 06:16:26PM +0200, Arnd Bergmann wrote:
> > On Tuesday 18 June 2013, Ezequiel Garcia wrote:
> > > 
> > > +               devbus-bootcs {
> > > +                       compatible = "marvell,mvebu-devbus";
> > > +                       reg = <0xffff0001 0x10400 0x8>;
> > > +                       ranges = <0 MBUS_ID(0x01, 0x2f) 0 0xffffffff>;
> > > +                       #address-cells = <1>;
> > > +                       #size-cells = <1>;
> > > +                       clocks = <&coreclk 0>;
> > > +                       status = "disabled";
> > > +               };
> > 
> > This is a violation of the binding as far as I can tell, since you don't specify ranges
> > to access the 0xffff0001 0x10400 address. However, as I explained in my comment for
> > the binding, I think you should clarify the binding and leave the implementation
> > as you have it here.
> > 
> 
> Mmm... again I got lost here. Which 'ranges' you say I don't specify to
> access the (formerly) 0xffff0001?
> 
> AFAIK, 'ranges' are only for children translation, which means I don't
> need to specify a ranges for that in the devbus node, but in its parent,
> right?
> 
> This ranges thing can be very tricky, so please correct me if I'm
> mistaken.

You already clarified that the binding was wrong. This was about the
part where you replied:

>> Do you really want to require the child to provide a "ranges" property?
>> I think this makes it more complicated to specify devices that belong
>> into the "internal-regs" category.
>> 
>
>No, this text is actually a left-over from the previous patchset, in
>current v3 patchset MBus children are not required to have any ranges.
>On the otherside, although you will need one except in the most trivial
>cases like for the BootROM.

With that change, everything above is ok.

	Arnd
diff mbox

Patch

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 4ae5eb5..0cd23ac 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -35,6 +35,56 @@ 
 		#size-cells = <1>;
 		interrupt-parent = <&mpic>;
 
+		devbus-bootcs {
+			compatible = "marvell,mvebu-devbus";
+			reg = <0xffff0001 0x10400 0x8>;
+			ranges = <0 MBUS_ID(0x01, 0x2f) 0 0xffffffff>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			clocks = <&coreclk 0>;
+			status = "disabled";
+		};
+
+		devbus-cs0 {
+			compatible = "marvell,mvebu-devbus";
+			reg = <0xffff0001 0x10408 0x8>;
+			ranges = <0 MBUS_ID(0x01, 0x3e) 0 0xffffffff>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			clocks = <&coreclk 0>;
+			status = "disabled";
+		};
+
+		devbus-cs1 {
+			compatible = "marvell,mvebu-devbus";
+			reg = <0xffff0001 0x10410 0x8>;
+			ranges = <0 MBUS_ID(0x01, 0x3d) 0 0xffffffff>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			clocks = <&coreclk 0>;
+			status = "disabled";
+		};
+
+		devbus-cs2 {
+			compatible = "marvell,mvebu-devbus";
+			reg = <0xffff0001 0x10418 0x8>;
+			ranges = <0 MBUS_ID(0x01, 0x3b) 0 0xffffffff>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			clocks = <&coreclk 0>;
+			status = "disabled";
+		};
+
+		devbus-cs3 {
+			compatible = "marvell,mvebu-devbus";
+			reg = <0xffff0001 0x10420 0x8>;
+			ranges = <0 MBUS_ID(0x01, 0x37) 0 0xffffffff>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			clocks = <&coreclk 0>;
+			status = "disabled";
+		};
+
 		internal-regs {
 			compatible = "simple-bus";
 			#address-cells = <1>;
@@ -181,50 +231,6 @@ 
 				status = "disabled";
 			};
 
-			devbus-bootcs@10400 {
-				compatible = "marvell,mvebu-devbus";
-				reg = <0x10400 0x8>;
-				#address-cells = <1>;
-				#size-cells = <1>;
-				clocks = <&coreclk 0>;
-				status = "disabled";
-			};
-
-			devbus-cs0@10408 {
-				compatible = "marvell,mvebu-devbus";
-				reg = <0x10408 0x8>;
-				#address-cells = <1>;
-				#size-cells = <1>;
-				clocks = <&coreclk 0>;
-				status = "disabled";
-			};
-
-			devbus-cs1@10410 {
-				compatible = "marvell,mvebu-devbus";
-				reg = <0x10410 0x8>;
-				#address-cells = <1>;
-				#size-cells = <1>;
-				clocks = <&coreclk 0>;
-				status = "disabled";
-			};
-
-			devbus-cs2@10418 {
-				compatible = "marvell,mvebu-devbus";
-				reg = <0x10418 0x8>;
-				#address-cells = <1>;
-				#size-cells = <1>;
-				clocks = <&coreclk 0>;
-				status = "disabled";
-			};
-
-			devbus-cs3@10420 {
-				compatible = "marvell,mvebu-devbus";
-				reg = <0x10420 0x8>;
-				#address-cells = <1>;
-				#size-cells = <1>;
-				clocks = <&coreclk 0>;
-				status = "disabled";
-			};
 		};
 	};
  };
diff --git a/arch/arm/boot/dts/armada-xp-gp.dts b/arch/arm/boot/dts/armada-xp-gp.dts
index 1f4daa7..3e14e82 100644
--- a/arch/arm/boot/dts/armada-xp-gp.dts
+++ b/arch/arm/boot/dts/armada-xp-gp.dts
@@ -40,7 +40,36 @@ 
 
 	soc {
 		ranges = <0xffff0001 0 0 0xd0000000 0x100000
-			  MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000>;
+			  MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000
+			  MBUS_ID(0x01, 0x2f) 0 0 0xf0000000 0x1000000>;
+
+		devbus-bootcs {
+			status = "okay";
+
+			/* Device Bus parameters are required */
+
+			/* Read parameters */
+			devbus,bus-width    = <8>;
+			devbus,turn-off-ps  = <60000>;
+			devbus,badr-skew-ps = <0>;
+			devbus,acc-first-ps = <124000>;
+			devbus,acc-next-ps  = <248000>;
+			devbus,rd-setup-ps  = <0>;
+			devbus,rd-hold-ps   = <0>;
+
+			/* Write parameters */
+			devbus,sync-enable = <0>;
+			devbus,wr-high-ps  = <60000>;
+			devbus,wr-low-ps   = <60000>;
+			devbus,ale-wr-ps   = <60000>;
+
+			/* NOR 16 MiB */
+			nor@0 {
+				compatible = "cfi-flash";
+				reg = <0 0x1000000>;
+				bank-width = <2>;
+			};
+		};
 
 		internal-regs {
 			serial@12000 {
@@ -116,35 +145,6 @@ 
 				};
 			};
 
-			devbus-bootcs@10400 {
-				status = "okay";
-				ranges = <0 0xf0000000 0x1000000>; /* @addr 0xf000000, size 0x1000000 */
-
-				/* Device Bus parameters are required */
-
-				/* Read parameters */
-				devbus,bus-width    = <8>;
-				devbus,turn-off-ps  = <60000>;
-				devbus,badr-skew-ps = <0>;
-				devbus,acc-first-ps = <124000>;
-				devbus,acc-next-ps  = <248000>;
-				devbus,rd-setup-ps  = <0>;
-				devbus,rd-hold-ps   = <0>;
-
-				/* Write parameters */
-				devbus,sync-enable = <0>;
-				devbus,wr-high-ps  = <60000>;
-				devbus,wr-low-ps   = <60000>;
-				devbus,ale-wr-ps   = <60000>;
-
-				/* NOR 16 MiB */
-				nor@0 {
-					compatible = "cfi-flash";
-					reg = <0 0x1000000>;
-					bank-width = <2>;
-				};
-			};
-
 			pcie-controller {
 				status = "okay";
 
diff --git a/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts b/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
index ea47196..490c423 100644
--- a/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
+++ b/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
@@ -28,7 +28,36 @@ 
 
 	soc {
 		ranges = <0xffff0001 0 0 0xd0000000 0x100000
-			  MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000>;
+			  MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000
+			  MBUS_ID(0x01, 0x2f) 0 0 0xf0000000 0x8000000>;
+
+		devbus-bootcs {
+			status = "okay";
+
+			/* Device Bus parameters are required */
+
+			/* Read parameters */
+			devbus,bus-width    = <8>;
+			devbus,turn-off-ps  = <60000>;
+			devbus,badr-skew-ps = <0>;
+			devbus,acc-first-ps = <124000>;
+			devbus,acc-next-ps  = <248000>;
+			devbus,rd-setup-ps  = <0>;
+			devbus,rd-hold-ps   = <0>;
+
+			/* Write parameters */
+			devbus,sync-enable = <0>;
+			devbus,wr-high-ps  = <60000>;
+			devbus,wr-low-ps   = <60000>;
+			devbus,ale-wr-ps   = <60000>;
+
+			/* NOR 128 MiB */
+			nor@0 {
+				compatible = "cfi-flash";
+				reg = <0 0x8000000>;
+				bank-width = <2>;
+			};
+		};
 
 		internal-regs {
 			serial@12000 {
@@ -144,35 +173,6 @@ 
 				status = "okay";
 			};
 
-			devbus-bootcs@10400 {
-				status = "okay";
-				ranges = <0 0xf0000000 0x8000000>; /* @addr 0xf000000, size 0x8000000 */
-
-				/* Device Bus parameters are required */
-
-				/* Read parameters */
-				devbus,bus-width    = <8>;
-				devbus,turn-off-ps  = <60000>;
-				devbus,badr-skew-ps = <0>;
-				devbus,acc-first-ps = <124000>;
-				devbus,acc-next-ps  = <248000>;
-				devbus,rd-setup-ps  = <0>;
-				devbus,rd-hold-ps   = <0>;
-
-				/* Write parameters */
-				devbus,sync-enable = <0>;
-				devbus,wr-high-ps  = <60000>;
-				devbus,wr-low-ps   = <60000>;
-				devbus,ale-wr-ps   = <60000>;
-
-				/* NOR 128 MiB */
-				nor@0 {
-					compatible = "cfi-flash";
-					reg = <0 0x8000000>;
-					bank-width = <2>;
-				};
-			};
-
 			pcie-controller {
 				status = "okay";
 				/* Internal mini-PCIe connector */