| Message ID | 20130916224743.GA18349@obsidianresearch.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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
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
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
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 :)
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
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
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();
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(-)