[PATCH] arm: Kirkwood - Remove kirkwood_setup_wins and rely on the DT binding

[PATCH] arm: Kirkwood - Remove kirkwood_setup_wins and rely on the DT binding

[Jason Gunthorpe]

kirkwood_setup_wins is the last manual caller of mbus in kirkwood, don't
call it for DT boards and rely on the board having a mbus node with
a proper ranges property to setup these windows.

This makes the DT self consistent, since the physical address of the
NAND and CRYPTO are both referenced internally. The arbitary Linux
constants KIRKWOOD_NAND_MEM_PHYS_BASE and KIRKWOOD_SRAM_PHYS_BASE
no longer have to match the DT values.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 arch/arm/boot/dts/kirkwood-db-88f6281.dts              | 5 ++++-
 arch/arm/boot/dts/kirkwood-db-88f6282.dts              | 5 ++++-
 arch/arm/boot/dts/kirkwood-iconnect.dts                | 5 ++++-
 arch/arm/boot/dts/kirkwood-mplcec4.dts                 | 5 ++++-
 arch/arm/boot/dts/kirkwood-netgear_readynas_duo_v2.dts | 5 ++++-
 arch/arm/boot/dts/kirkwood-nsa310.dts                  | 5 ++++-
 arch/arm/boot/dts/kirkwood-ts219-6282.dts              | 5 ++++-
 arch/arm/mach-kirkwood/board-dt.c                      | 1 -
 8 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/arch/arm/boot/dts/kirkwood-db-88f6281.dts b/arch/arm/boot/dts/kirkwood-db-88f6281.dts
index 72c4b0a..0806ae6 100644
--- a/arch/arm/boot/dts/kirkwood-db-88f6281.dts
+++ b/arch/arm/boot/dts/kirkwood-db-88f6281.dts
@@ -19,7 +19,10 @@
 	compatible = "marvell,db-88f6281-bp", "marvell,kirkwood-88f6281", "marvell,kirkwood";
 
 	mbus {
-		ranges = <MBUS_ID(0xf0, 0x01) 0 0xf1000000 0x100000>;
+		ranges = <MBUS_ID(0xf0, 0x01) 0 0xf1000000 0x100000	/* internal-regs */
+			  MBUS_ID(0x01, 0x2f) 0 0xf4000000 0x10000	/* nand flash */
+			  MBUS_ID(0x03, 0x01) 0 0xf5000000 0x10000	/* crypto sram */
+			  >;
 		pcie-controller {
 			status = "okay";
 
diff --git a/arch/arm/boot/dts/kirkwood-db-88f6282.dts b/arch/arm/boot/dts/kirkwood-db-88f6282.dts
index 36c411d..8ecf3c4 100644
--- a/arch/arm/boot/dts/kirkwood-db-88f6282.dts
+++ b/arch/arm/boot/dts/kirkwood-db-88f6282.dts
@@ -19,7 +19,10 @@
 	compatible = "marvell,db-88f6282-bp", "marvell,kirkwood-88f6282", "marvell,kirkwood";
 
 	mbus {
-		ranges = <MBUS_ID(0xf0, 0x01) 0 0xf1000000 0x100000>;
+		ranges = <MBUS_ID(0xf0, 0x01) 0 0xf1000000 0x100000	/* internal-regs */
+			  MBUS_ID(0x01, 0x2f) 0 0xf4000000 0x10000	/* nand flash */
+			  MBUS_ID(0x03, 0x01) 0 0xf5000000 0x10000	/* crypto sram */
+			  >;
 		pcie-controller {
 			status = "okay";
 
diff --git a/arch/arm/boot/dts/kirkwood-iconnect.dts b/arch/arm/boot/dts/kirkwood-iconnect.dts
index 0323f01..2631733 100644
--- a/arch/arm/boot/dts/kirkwood-iconnect.dts
+++ b/arch/arm/boot/dts/kirkwood-iconnect.dts
@@ -19,7 +19,10 @@
 	};
 
 	mbus {
-		ranges = <MBUS_ID(0xf0, 0x01) 0 0xf1000000 0x100000>;
+		ranges = <MBUS_ID(0xf0, 0x01) 0 0xf1000000 0x100000	/* internal-regs */
+			  MBUS_ID(0x01, 0x2f) 0 0xf4000000 0x10000	/* nand flash */
+			  MBUS_ID(0x03, 0x01) 0 0xf5000000 0x10000	/* crypto sram */
+			  >;
 		pcie-controller {
 			status = "okay";
 
diff --git a/arch/arm/boot/dts/kirkwood-mplcec4.dts b/arch/arm/boot/dts/kirkwood-mplcec4.dts
index ce2b94b..fe445174 100644
--- a/arch/arm/boot/dts/kirkwood-mplcec4.dts
+++ b/arch/arm/boot/dts/kirkwood-mplcec4.dts
@@ -17,7 +17,10 @@
         };
 
 	mbus {
-		ranges = <MBUS_ID(0xf0, 0x01) 0 0xf1000000 0x100000>;
+		ranges = <MBUS_ID(0xf0, 0x01) 0 0xf1000000 0x100000	/* internal-regs */
+			  MBUS_ID(0x01, 0x2f) 0 0xf4000000 0x10000	/* nand flash */
+			  MBUS_ID(0x03, 0x01) 0 0xf5000000 0x10000	/* crypto sram */
+			  >;
 		pcie-controller {
 			status = "okay";
 
diff --git a/arch/arm/boot/dts/kirkwood-netgear_readynas_duo_v2.dts b/arch/arm/boot/dts/kirkwood-netgear_readynas_duo_v2.dts
index 874857e..37706f2 100644
--- a/arch/arm/boot/dts/kirkwood-netgear_readynas_duo_v2.dts
+++ b/arch/arm/boot/dts/kirkwood-netgear_readynas_duo_v2.dts
@@ -17,7 +17,10 @@
 	};
 
 	mbus {
-		ranges = <MBUS_ID(0xf0, 0x01) 0 0xf1000000 0x100000>;
+		ranges = <MBUS_ID(0xf0, 0x01) 0 0xf1000000 0x100000	/* internal-regs */
+			  MBUS_ID(0x01, 0x2f) 0 0xf4000000 0x10000	/* nand flash */
+			  MBUS_ID(0x03, 0x01) 0 0xf5000000 0x10000	/* crypto sram */
+			  >;
 		pcie-controller {
 			status = "okay";
 
diff --git a/arch/arm/boot/dts/kirkwood-nsa310.dts b/arch/arm/boot/dts/kirkwood-nsa310.dts
index 7aeae0c..c6d1edc 100644
--- a/arch/arm/boot/dts/kirkwood-nsa310.dts
+++ b/arch/arm/boot/dts/kirkwood-nsa310.dts
@@ -15,7 +15,10 @@
 	};
 
 	mbus {
-		ranges = <MBUS_ID(0xf0, 0x01) 0 0xf1000000 0x100000>;
+		ranges = <MBUS_ID(0xf0, 0x01) 0 0xf1000000 0x100000	/* internal-regs */
+			  MBUS_ID(0x01, 0x2f) 0 0xf4000000 0x10000	/* nand flash */
+			  MBUS_ID(0x03, 0x01) 0 0xf5000000 0x10000	/* crypto sram */
+			  >;
 		pcie-controller {
 			status = "okay";
 
diff --git a/arch/arm/boot/dts/kirkwood-ts219-6282.dts b/arch/arm/boot/dts/kirkwood-ts219-6282.dts
index 9efcd2d..94b82f9 100644
--- a/arch/arm/boot/dts/kirkwood-ts219-6282.dts
+++ b/arch/arm/boot/dts/kirkwood-ts219-6282.dts
@@ -6,7 +6,10 @@
 
 / {
 	mbus {
-		ranges = <MBUS_ID(0xf0, 0x01) 0 0xf1000000 0x100000>;
+		ranges = <MBUS_ID(0xf0, 0x01) 0 0xf1000000 0x100000	/* internal-regs */
+			  MBUS_ID(0x01, 0x2f) 0 0xf4000000 0x10000	/* nand flash */
+			  MBUS_ID(0x03, 0x01) 0 0xf5000000 0x10000	/* crypto sram */
+			  >;
 		pcie-controller {
 			status = "okay";
 
diff --git a/arch/arm/mach-kirkwood/board-dt.c b/arch/arm/mach-kirkwood/board-dt.c
index afc058b..41c8580 100644
--- a/arch/arm/mach-kirkwood/board-dt.c
+++ b/arch/arm/mach-kirkwood/board-dt.c
@@ -94,7 +94,6 @@ static void __init kirkwood_dt_init(void)
 	writel(readl(CPU_CONFIG) & ~CPU_CONFIG_ERROR_PROP, CPU_CONFIG);
 
 	BUG_ON(mvebu_mbus_dt_init());
-	kirkwood_setup_wins();
 
 	kirkwood_l2_init();
 
-- 
1.8.1.2

[PATCH] arm: Kirkwood - Remove kirkwood_setup_wins and rely on the DT binding

[Ezequiel Garcia]

Jason,

For some reason that I can't recall, the prefered style for commit logs is:

[PATCH] ARM: kirkwood: Remove kirkwood_setup_wins and rely on the DT
binding

(Yes, I know, it's just stupid nit...)

On Mon, Sep 16, 2013 at 04:47:43PM -0600, Jason Gunthorpe wrote:
> kirkwood_setup_wins is the last manual caller of mbus in kirkwood, don't
> call it for DT boards and rely on the board having a mbus node with
> a proper ranges property to setup these windows.
> 
> This makes the DT self consistent, since the physical address of the
> NAND and CRYPTO are both referenced internally. The arbitary Linux
> constants KIRKWOOD_NAND_MEM_PHYS_BASE and KIRKWOOD_SRAM_PHYS_BASE
> no longer have to match the DT values.
> 
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  arch/arm/boot/dts/kirkwood-db-88f6281.dts              | 5 ++++-
>  arch/arm/boot/dts/kirkwood-db-88f6282.dts              | 5 ++++-
>  arch/arm/boot/dts/kirkwood-iconnect.dts                | 5 ++++-
>  arch/arm/boot/dts/kirkwood-mplcec4.dts                 | 5 ++++-
>  arch/arm/boot/dts/kirkwood-netgear_readynas_duo_v2.dts | 5 ++++-
>  arch/arm/boot/dts/kirkwood-nsa310.dts                  | 5 ++++-
>  arch/arm/boot/dts/kirkwood-ts219-6282.dts              | 5 ++++-
>  arch/arm/mach-kirkwood/board-dt.c                      | 1 -
>  8 files changed, 28 insertions(+), 8 deletions(-)
> 

I'm trying to figure out why you're changing just these and not
all of the kirkwood device tree files, but I really can't.

Grep says these contain a nand node:

arch/arm/boot/dts/kirkwood-dockstar.dts
arch/arm/boot/dts/kirkwood-goflexnet.dts
arch/arm/boot/dts/kirkwood-guruplug-server-plus.dts
arch/arm/boot/dts/kirkwood-ib62x0.dts
arch/arm/boot/dts/kirkwood-iconnect.dts
arch/arm/boot/dts/kirkwood-iomega_ix2_200.dts
arch/arm/boot/dts/kirkwood-km_kirkwood.dts
arch/arm/boot/dts/kirkwood-mplcec4.dts
arch/arm/boot/dts/kirkwood-netgear_readynas_duo_v2.dts
arch/arm/boot/dts/kirkwood-nsa310.dts
arch/arm/boot/dts/kirkwood-openblocks_a6.dts
arch/arm/boot/dts/kirkwood-topkick.dts

Am I missing something obvious?
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH] arm: Kirkwood - Remove kirkwood_setup_wins and rely on the DT binding

[Jason Gunthorpe]

On Tue, Sep 17, 2013 at 10:32:07AM -0300, Ezequiel Garcia wrote:

> On Mon, Sep 16, 2013 at 04:47:43PM -0600, Jason Gunthorpe wrote:
> > kirkwood_setup_wins is the last manual caller of mbus in kirkwood, don't
> > call it for DT boards and rely on the board having a mbus node with
> > a proper ranges property to setup these windows.
> > 
> > This makes the DT self consistent, since the physical address of the
> > NAND and CRYPTO are both referenced internally. The arbitary Linux
> > constants KIRKWOOD_NAND_MEM_PHYS_BASE and KIRKWOOD_SRAM_PHYS_BASE
> > no longer have to match the DT values.
> > 
> > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> >  arch/arm/boot/dts/kirkwood-db-88f6281.dts              | 5 ++++-
> >  arch/arm/boot/dts/kirkwood-db-88f6282.dts              | 5 ++++-
> >  arch/arm/boot/dts/kirkwood-iconnect.dts                | 5 ++++-
> >  arch/arm/boot/dts/kirkwood-mplcec4.dts                 | 5 ++++-
> >  arch/arm/boot/dts/kirkwood-netgear_readynas_duo_v2.dts | 5 ++++-
> >  arch/arm/boot/dts/kirkwood-nsa310.dts                  | 5 ++++-
> >  arch/arm/boot/dts/kirkwood-ts219-6282.dts              | 5 ++++-
> >  arch/arm/mach-kirkwood/board-dt.c                      | 1 -
> >  8 files changed, 28 insertions(+), 8 deletions(-)
> > 
> 
> I'm trying to figure out why you're changing just these and not
> all of the kirkwood device tree files, but I really can't.

These were the ones that had a pre-existing mbus ranges line, which I
thought was the whole set of boards.

> Am I missing something obvious?

No, I didn't realize that you had only added mbus ranges to boards
that had PCI, every board needs a ranges for proper use of the
binding...

Since they are all the same I'll respin the patch and add the ranges to
the kirkwood.dtsi and remove it from all the board files, (which is
where I started, but then saw that board files had a ranges
already)

Jason

[PATCH] arm: Kirkwood - Remove kirkwood_setup_wins and rely on the DT binding

[Thomas Petazzoni]

Dear Jason Gunthorpe,

On Tue, 17 Sep 2013 09:36:19 -0600, Jason Gunthorpe wrote:

> Since they are all the same I'll respin the patch and add the ranges
> to the kirkwood.dtsi and remove it from all the board files, (which is
> where I started, but then saw that board files had a ranges
> already)

In Armada 370/XP land, we've decided to always put the ranges in the
per-board .dts file. The reason is that sometimes, a board needs to add
an additional range (like for a NOR device for example), and
unfortunately, the existing DT language doesn't allow "extending"
a 'ranges' property defined at the .dtsi level by an additional entry
added at the .dts level.

This has lead to problems in Armada 370/XP land, where we were
overloading the ranges property for some boards in the .dts, then added
a new range in the .dtsi, and forgot to propagate this change to
the .dts files. To avoid this, we've decided to have the ranges
property always in the .dts files.

A better solution would be to have a += operator in the DT language,
but until that exists, we thought that pushing the ranges property down
to the .dts file was the least horrible solution.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH] arm: Kirkwood - Remove kirkwood_setup_wins and rely on the DT binding

[Jason Gunthorpe]

On Tue, Sep 17, 2013 at 08:03:01PM +0200, Thomas Petazzoni wrote:
> Dear Jason Gunthorpe,
> 
> On Tue, 17 Sep 2013 09:36:19 -0600, Jason Gunthorpe wrote:
> 
> > Since they are all the same I'll respin the patch and add the ranges
> > to the kirkwood.dtsi and remove it from all the board files, (which is
> > where I started, but then saw that board files had a ranges
> > already)
> 
> In Armada 370/XP land, we've decided to always put the ranges in the
> per-board .dts file. The reason is that sometimes, a board needs to
> add

Yes, I recall you talking about this, that is why initially just went
adding to the existing mbus ranges in the board files.

However, is that really still the case? Now that PEX uses a difference
scheme it seems they all washed out to be the same.

All the armada-370 boards in mainline seem to be the same today.

armada-xp looks the same as well, except that armada-xp-axpwifiap.dts
is missing MBUS_ID(0x01, 0x2f), but it looks like it would be harmless
to add, even though it is not used.

> A better solution would be to have a += operator in the DT language,

Agree

> but until that exists, we thought that pushing the ranges property down
> to the .dts file was the least horrible solution.

I think we can get away with doing it the other way for kirkwood,
here are my reasons:
 - Kirkwood is mature now, the DT is basically complete, we shouldn't
   need to churn the ranges in the dtsi much, if at all.
 - There are 31 kirkwood dts files, and none of them need a ranges
   different from the default
 - Kirkwood has more than enough mbus windows, we don't need to be
   stingy with them
 - The board files were already sort of like this, but a big chunk
   of the 31 boards were missing ranges entirely.

Basically, no board file has a ranges, only the kirkwood.dtsi has a
ranges.

Jason

[PATCH] arm: Kirkwood - Remove kirkwood_setup_wins and rely on the DT binding

[Ezequiel Garcia]

On Tue, Sep 17, 2013 at 12:17:42PM -0600, Jason Gunthorpe wrote:
> On Tue, Sep 17, 2013 at 08:03:01PM +0200, Thomas Petazzoni wrote:
> > Dear Jason Gunthorpe,
> > 
> > On Tue, 17 Sep 2013 09:36:19 -0600, Jason Gunthorpe wrote:
> > 
> > > Since they are all the same I'll respin the patch and add the ranges
> > > to the kirkwood.dtsi and remove it from all the board files, (which is
> > > where I started, but then saw that board files had a ranges
> > > already)
> > 
> > In Armada 370/XP land, we've decided to always put the ranges in the
> > per-board .dts file. The reason is that sometimes, a board needs to
> > add
> 
> Yes, I recall you talking about this, that is why initially just went
> adding to the existing mbus ranges in the board files.
> 
> However, is that really still the case? Now that PEX uses a difference
> scheme it seems they all washed out to be the same.
> 
> All the armada-370 boards in mainline seem to be the same today.
> 
> armada-xp looks the same as well, except that armada-xp-axpwifiap.dts
> is missing MBUS_ID(0x01, 0x2f), but it looks like it would be harmless
> to add, even though it is not used.
> 
> > A better solution would be to have a += operator in the DT language,
> 
> Agree
> 
> > but until that exists, we thought that pushing the ranges property down
> > to the .dts file was the least horrible solution.
> 
> I think we can get away with doing it the other way for kirkwood,
> here are my reasons:
>  - Kirkwood is mature now, the DT is basically complete, we shouldn't
>    need to churn the ranges in the dtsi much, if at all.
>  - There are 31 kirkwood dts files, and none of them need a ranges
>    different from the default
>  - Kirkwood has more than enough mbus windows, we don't need to be
>    stingy with them
>  - The board files were already sort of like this, but a big chunk
>    of the 31 boards were missing ranges entirely.
> 
> Basically, no board file has a ranges, only the kirkwood.dtsi has a
> ranges.
> 

While I'm not advocating any of the alternatives, I think we could
add a comment *now* to all our device trees explaining how the ranges
property is *not* inheritable and that a child/board "dts" ranges
property will override the parent's completely.

Note that I'm not volunteering :)
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH] arm: Kirkwood - Remove kirkwood_setup_wins and rely on the DT binding

[Sebastian Hesselbarth]

On 09/17/2013 08:17 PM, Jason Gunthorpe wrote:
> On Tue, Sep 17, 2013 at 08:03:01PM +0200, Thomas Petazzoni wrote:
>> but until that exists, we thought that pushing the ranges property down
>> to the .dts file was the least horrible solution.
>
> I think we can get away with doing it the other way for kirkwood,
> here are my reasons:
>   - Kirkwood is mature now, the DT is basically complete, we shouldn't
>     need to churn the ranges in the dtsi much, if at all.
>   - There are 31 kirkwood dts files, and none of them need a ranges
>     different from the default
>   - Kirkwood has more than enough mbus windows, we don't need to be
>     stingy with them
>   - The board files were already sort of like this, but a big chunk
>     of the 31 boards were missing ranges entirely.
>
> Basically, no board file has a ranges, only the kirkwood.dtsi has a
> ranges.

Dove will also have its mbus ranges located in the SoC dtsi for the
same above reasons. The patch is already queued in mvebu/for-next.

Sebastian

[PATCH] arm: Kirkwood - Remove kirkwood_setup_wins and rely on the DT binding

[Thomas Petazzoni]

Dear Jason Gunthorpe,

On Tue, 17 Sep 2013 12:17:42 -0600, Jason Gunthorpe wrote:

> Yes, I recall you talking about this, that is why initially just went
> adding to the existing mbus ranges in the board files.
> 
> However, is that really still the case? Now that PEX uses a difference
> scheme it seems they all washed out to be the same.

No, some boards have a NOR flash that requires a dedicated window, some
do not have a NOR flash.

> > but until that exists, we thought that pushing the ranges property down
> > to the .dts file was the least horrible solution.
> 
> I think we can get away with doing it the other way for kirkwood,
> here are my reasons:
>  - Kirkwood is mature now, the DT is basically complete, we shouldn't
>    need to churn the ranges in the dtsi much, if at all.
>  - There are 31 kirkwood dts files, and none of them need a ranges
>    different from the default
>  - Kirkwood has more than enough mbus windows, we don't need to be
>    stingy with them
>  - The board files were already sort of like this, but a big chunk
>    of the 31 boards were missing ranges entirely.
> 
> Basically, no board file has a ranges, only the kirkwood.dtsi has a
> ranges.

Right, true that for Kirkwood this probably stands.

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com