MBus device tree binding

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +8 -0

+Required properties:
+
+- compatible:	 Should be set to one of the following:
+		 marvell,armada370-mbus
+		 marvell,armadaxp-mbus
+
+- reg:		 Device's register space.
+		 Two entries are expected, see the examples below.

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +12 -0

+Each child device needs at least a 'ranges' property. If the child is avaiable
+(i.e. status not 'disabled'), then the MBus driver creates a decoding window
+for it. For instance, in the example below the BootROM child is specified:
+
+	soc {
+		compatible = "marvell,armada370-mbus", "simple-bus";
+		reg = <0xd0020000 0x100>, <0xd0020180 0x20>;
+		#address-cells = <2>;
+		#size-cells = <1>;
+
+		ranges = < ... /* other entries */
+			   0x011d0000 0 0 0xfff00000 0x100000>;

Using 0xffff0002 as a placeholder for the pcie translation is definitely better than 0xffff0000 as you had before, but let me ask again in case you missed it the last time (and sorry if I missed the answer):

Why not just put the actual translation here the way it happens for each of the PCIe ports? With the definition here, the PCIe driver actually has no way to figure out what settings the windows need to use!

Dear Arnd Bergmann,

Quoted 7 lines · Arnd Bergmann

Using 0xffff0002 as a placeholder for the pcie translation is definitely better than 0xffff0000 as you had before, but let me ask again in case you missed it the last time (and sorry if I missed the answer):

Why not just put the actual translation here the way it happens for each of the PCIe ports? With the definition here, the PCIe driver actually has no way to figure out what settings the windows need to use!

Come on Arnd, this is something we have already discussed countless times with you.

We cannot define translations for each PCIe port because we don't know in advance how much I/O and memory space each PCIe device will request. This is the reason why we have one global range for I/O space and one global space for memory space, that are given to the Linux PCI core, which then dynamically assigns sub-ranges for each PCIe device into those two global ranges.

Quoted 15 lines · Ezequiel Garcia

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +15 -0

+Required properties:
+
+- compatible:	 Should be set to one of the following:
+		 marvell,armada370-mbus
+		 marvell,armadaxp-mbus
+
+- reg:		 Device's register space.
+		 Two entries are expected, see the examples below.
+		 The first one controls the devices decoding window and
+		 the second one controls the SDRAM decoding window.
+
+- address-cells: Must be '2'. The first cell for the MBus ID encoding,
+                 the second cell for the address offset within the window.
+
+- size-cells:    Must be '1'.

Documentation/devicetree/bindings/bus/mvebu-mbus.txt line 21

Quoted 3 lines

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +3 -0

+
+- ranges:        Must be set up to provide a proper translation for each child.
+	         See the examples below.

You should explain here what the policy is regarding windows set up by the boot loader. Are the ranges in here required to be the ones that the boot loader has preconfigured, or are they the ones that the mbus driver is supposed to set up?

Quoted 18 lines

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +18 -0

+Each child device needs at least a 'ranges' property. If the child is avaiable
+(i.e. status not 'disabled'), then the MBus driver creates a decoding window
+for it. For instance, in the example below the BootROM child is specified:
+
+	soc {
+		compatible = "marvell,armada370-mbus", "simple-bus";
+		reg = <0xd0020000 0x100>, <0xd0020180 0x20>;
+		#address-cells = <2>;
+		#size-cells = <1>;
+
+		ranges = < ... /* other entries */
+			   0x011d0000 0 0 0xfff00000 0x100000>;
+
+		bootrom {
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0 0x011d0000 0 0x100000>;
+		};

Documentation/devicetree/bindings/bus/mvebu-mbus.txt line 55

Quoted 4 lines

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +4 -0

+
+		/* other children */
+		...
+	};

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.

Quoted 16 lines · Arnd Bergmann

Quoted 15 lines · Ezequiel Garcia

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +15 -0

+Required properties:
+
+- compatible:	 Should be set to one of the following:
+		 marvell,armada370-mbus
+		 marvell,armadaxp-mbus
+
+- reg:		 Device's register space.
+		 Two entries are expected, see the examples below.
+		 The first one controls the devices decoding window and
+		 the second one controls the SDRAM decoding window.
+
+- address-cells: Must be '2'. The first cell for the MBus ID encoding,
+                 the second cell for the address offset within the window.
+
+- size-cells:    Must be '1'.

Quoted 9 lines

Documentation/devicetree/bindings/bus/mvebu-mbus.txt line 21

Quoted 3 lines

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +3 -0

+
+- ranges:        Must be set up to provide a proper translation for each child.
+	         See the examples below.

You should explain here what the policy is regarding windows set up by the boot loader. Are the ranges in here required to be the ones that the boot loader has preconfigured, or are they the ones that the mbus driver is supposed to set up?

Actually, I might be confused but the kernel MBus driver is completely independent from the bootloader's -except in the internal register case, (because we don't touch that from where the bootloader left it).

What happens is that any decoding window that was setup by the bootloader, is wiped and completely new windows are allocated using the translations in the DT, as described by this binding.

This was the case from the start with the old MBus driver. FWIW, I think it's actually the best choice that can be made: it makes the kernel independent of the previous setting.

I know you've suggested differently in the past, but I'm not sure I understand what's the benefit in keeping the bootloaders configuration.

Quoted 18 lines

Quoted 18 lines

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +18 -0

+Each child device needs at least a 'ranges' property. If the child is avaiable
+(i.e. status not 'disabled'), then the MBus driver creates a decoding window
+for it. For instance, in the example below the BootROM child is specified:
+
+	soc {
+		compatible = "marvell,armada370-mbus", "simple-bus";
+		reg = <0xd0020000 0x100>, <0xd0020180 0x20>;
+		#address-cells = <2>;
+		#size-cells = <1>;
+
+		ranges = < ... /* other entries */
+			   0x011d0000 0 0 0xfff00000 0x100000>;
+
+		bootrom {
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0 0x011d0000 0 0x100000>;
+		};

Quoted 9 lines

Documentation/devicetree/bindings/bus/mvebu-mbus.txt line 55

Quoted 4 lines

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +4 -0

+
+		/* other children */
+		...
+	};

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.

Quoted 27 lines · Ezequiel Garcia

Quoted 16 lines · Arnd Bergmann

Quoted 15 lines · Ezequiel Garcia

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +15 -0

+Required properties:
+
+- compatible:	 Should be set to one of the following:
+		 marvell,armada370-mbus
+		 marvell,armadaxp-mbus
+
+- reg:		 Device's register space.
+		 Two entries are expected, see the examples below.
+		 The first one controls the devices decoding window and
+		 the second one controls the SDRAM decoding window.
+
+- address-cells: Must be '2'. The first cell for the MBus ID encoding,
+                 the second cell for the address offset within the window.
+
+- size-cells:    Must be '1'.

Quoted 9 lines

Documentation/devicetree/bindings/bus/mvebu-mbus.txt line 21

Quoted 3 lines

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +3 -0

+
+- ranges:        Must be set up to provide a proper translation for each child.
+	         See the examples below.

You should explain here what the policy is regarding windows set up by the boot loader. Are the ranges in here required to be the ones that the boot loader has preconfigured, or are they the ones that the mbus driver is supposed to set up?

Quoted 3 lines

Actually, I might be confused but the kernel MBus driver is completely independent from the bootloader's -except in the internal register case, (because we don't touch that from where the bootloader left it).

Right, then state that clearly in the binding.

Quoted 10 lines

What happens is that any decoding window that was setup by the bootloader, is wiped and completely new windows are allocated using the translations in the DT, as described by this binding.

This was the case from the start with the old MBus driver. FWIW, I think it's actually the best choice that can be made: it makes the kernel independent of the previous setting.

I know you've suggested differently in the past, but I'm not sure I understand what's the benefit in keeping the bootloaders configuration.

The device tree /normally/ describes things that are either wired up in hardware or set up by the boot loader. Describing things that the boot loader may or may not have set up and that the kernel should set up but may ignore if it wants to is a bit fishy, but it seems that you have decided to do it that way. You should definitely document the fact that all ranges except the "internal-regs" are just suggestions and cannot be relied on to be present at boot time.

Quoted 11 lines

Quoted 10 lines

What happens is that any decoding window that was setup by the bootloader, is wiped and completely new windows are allocated using the translations in the DT, as described by this binding.

This was the case from the start with the old MBus driver. FWIW, I think it's actually the best choice that can be made: it makes the kernel independent of the previous setting.

I know you've suggested differently in the past, but I'm not sure I understand what's the benefit in keeping the bootloaders configuration.

Quoted 8 lines

The device tree normally describes things that are either wired up in hardware or set up by the boot loader. Describing things that the boot loader may or may not have set up and that the kernel should set up but may ignore if it wants to is a bit fishy, but it seems that you have decided to do it that way. You should definitely document the fact that all ranges except the "internal-regs" are just suggestions and cannot be relied on to be present at boot time.

Hold on! I've just noticed this, and I want to clarify something, just to avoid mis-interpretations. The binding is not saying "the windows described through this ranges are present at boot time".

Rather, it is "this binding will guarantee that the windows described in it will be present after the mbus allocates them".

Does it sound too fishy?

Quoted 20 lines · Ezequiel Garcia

Quoted 11 lines

Quoted 10 lines

What happens is that any decoding window that was setup by the bootloader, is wiped and completely new windows are allocated using the translations in the DT, as described by this binding.

This was the case from the start with the old MBus driver. FWIW, I think it's actually the best choice that can be made: it makes the kernel independent of the previous setting.

I know you've suggested differently in the past, but I'm not sure I understand what's the benefit in keeping the bootloaders configuration.

Quoted 8 lines

The device tree normally describes things that are either wired up in hardware or set up by the boot loader. Describing things that the boot loader may or may not have set up and that the kernel should set up but may ignore if it wants to is a bit fishy, but it seems that you have decided to do it that way. You should definitely document the fact that all ranges except the "internal-regs" are just suggestions and cannot be relied on to be present at boot time.

Quoted 8 lines

Hold on! I've just noticed this, and I want to clarify something, just to avoid mis-interpretations. The binding is not saying "the windows described through this ranges are present at boot time".

Rather, it is "this binding will guarantee that the windows described in it will be present after the mbus allocates them".

Does it sound too fishy?

I don't think it's a guarantee that the binding can make. The binding describes the interface between the hardware/firmware and the kernel, not an interface between one kernel driver and another.

You could instead write:

"The ranges property defines a set of mbus windows that are expected to be set by the operating system and that are guaranteed to be free of overlaps with one another or with the system memory ranges. Each entry in the property refers to exactly one window. If an operating system choses to use a different set of mbus windows, it must ensure that any address translations performed from downstream devices are adapted accordingly. The operating system may insert additional mbus windows that do not conflict with the ones listed in the ranges, e.g. for mapping PCIe devices. As a special case, the internal register window must be set up by the boot loader at the address listed in the ranges property, since the operating uses it to set up the other windows."

Quoted 36 lines · Arnd Bergmann

Quoted 20 lines · Ezequiel Garcia

Quoted 11 lines

Quoted 10 lines

What happens is that any decoding window that was setup by the bootloader, is wiped and completely new windows are allocated using the translations in the DT, as described by this binding.

This was the case from the start with the old MBus driver. FWIW, I think it's actually the best choice that can be made: it makes the kernel independent of the previous setting.

I know you've suggested differently in the past, but I'm not sure I understand what's the benefit in keeping the bootloaders configuration.

Quoted 8 lines

The device tree normally describes things that are either wired up in hardware or set up by the boot loader. Describing things that the boot loader may or may not have set up and that the kernel should set up but may ignore if it wants to is a bit fishy, but it seems that you have decided to do it that way. You should definitely document the fact that all ranges except the "internal-regs" are just suggestions and cannot be relied on to be present at boot time.

Quoted 8 lines

Hold on! I've just noticed this, and I want to clarify something, just to avoid mis-interpretations. The binding is not saying "the windows described through this ranges are present at boot time".

Rather, it is "this binding will guarantee that the windows described in it will be present after the mbus allocates them".

Does it sound too fishy?

I don't think it's a guarantee that the binding can make. The binding describes the interface between the hardware/firmware and the kernel, not an interface between one kernel driver and another.

You could instead write:

Quoted 11 lines

"The ranges property defines a set of mbus windows that are expected to be set by the operating system and that are guaranteed to be free of overlaps with one another or with the system memory ranges. Each entry in the property refers to exactly one window. If an operating system choses to use a different set of mbus windows, it must ensure that any address translations performed from downstream devices are adapted accordingly. The operating system may insert additional mbus windows that do not conflict with the ones listed in the ranges, e.g. for mapping PCIe devices. As a special case, the internal register window must be set up by the boot loader at the address listed in the ranges property, since the operating

Quoted 2 lines

uses it to set up the other windows."

Nice!

Shamelessly copy-pasted into the binding documentation.

Documentation/devicetree/bindings/bus/mvebu-mbus.txt line 85

Quoted 6 lines · Ezequiel Garcia

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +6 -0

+
+IIAA0000
+
+Where:
+  -- I = Marvell defined target ID for programmable windows
+  -- A = Marvell defined target attributes for programmable windows

I thought we agreed to something like:

SIAA0000

Where 'S' is the designator for the special items like PCI-E and internal-regs. 0 = normal target ids, 0xF = special ids.

The target is only 4 bits, the attr is 8, so a little doc update to clarify this should be enough, no need to change the DTs.

Quoted 12 lines · Jason Gunthorpe

Documentation/devicetree/bindings/bus/mvebu-mbus.txt line 85

Quoted 6 lines · Ezequiel Garcia

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +6 -0

+
+IIAA0000
+
+Where:
+  -- I = Marvell defined target ID for programmable windows
+  -- A = Marvell defined target attributes for programmable windows

I thought we agreed to something like:

SIAA0000

Quoted 5 lines

Where 'S' is the designator for the special items like PCI-E and internal-regs. 0 = normal target ids, 0xF = special ids.

The target is only 4 bits, the attr is 8, so a little doc update to clarify this should be enough, no need to change the DTs.

+1 for SIAA0000, as it allows to use MBUS_ID also for those fake windows. It makes it more readable IMHO.

Also allows you to have up to 40b offset, which might be important with LPAE enabled.

Quoted 2 lines · Sebastian Hesselbarth

Also allows you to have up to 40b offset, which might be important with LPAE enabled.

Not with the current generation I think, since the mbus windows are 32 bit only, but it would avoid having to come up with a new format for a potential future-generation mbus that has wider address.

Quoted 7 lines · Arnd Bergmann

Quoted 2 lines · Sebastian Hesselbarth

Also allows you to have up to 40b offset, which might be important with LPAE enabled.

Not with the current generation I think, since the mbus windows are 32 bit only, but it would avoid having to come up with a new format for a potential future-generation mbus that has wider address.

Yeah, I also recall Thomas or Gregory mention a 32b limitation in remap windows. But we don't need to waste addresses here

And even if SIAA0000 is a concern because there may be target id >15 someday, we can still have IIAASS00 instead of IIAA00SS. I guess LPAE will not rise above 40b quickly ;)

Quoted 6 lines · Sebastian Hesselbarth

Yeah, I also recall Thomas or Gregory mention a 32b limitation in remap windows. But we don't need to waste addresses here

And even if SIAA0000 is a concern because there may be target id>15 someday, we can still have IIAASS00 instead of IIAA00SS. I guess LPAE will not rise above 40b quickly ;)

S = 0 means 4 bit I, 8 bit A
S = F means special
S = 1 could mean 16 bit I, etc , etc

Yes, we could define things as 'SIAAoooo oooooooo'

But remember 'o' is the offset within the window, it is not related to LPAE.

To need> 32 bits 'o' you need to have windows> 4G in size. The only thing that could possibly do that is PCI-E, and it is all special anyhow..

The mbus top level ranges remap already supports>4G locations for the windows, even though current hardware cannot do that.

Quoted 3 lines

S = 0 means 4 bit I, 8 bit A
S = F means special
S = 1 could mean 16 bit I, etc , etc

S & 0x8 == 0x0 means 7b target
S & 0x8 == 0x8 means 7b special ?

Quoted 2 lines

The mbus top level ranges remap already supports>4G locations for the windows, even though current hardware cannot do that.

True. But as Arnd also mentioned, I don't think it will ever be a problem at all. Possibly there will never be any future SoC with mbus that will either allow >32b remap base addresses nor >4G size.

Quoted 6 lines · Sebastian Hesselbarth

Quoted 3 lines

S = 0 means 4 bit I, 8 bit A
S = F means special
S = 1 could mean 16 bit I, etc , etc

S& 0x8 == 0x0 means 7b target
S& 0x8 == 0x8 means 7b special ?

No need, special == mbus driver defined. There is no target ID.

The forms could be:

0IAA0000
FK000000
- K=0 -> internal regs
- K=1 -> PCI-E thingy
etc
1IIAA000 (future expansion example)

Quoted 6 lines

Quoted 2 lines

The mbus top level ranges remap already supports>4G locations for the windows, even though current hardware cannot do that.

True. But as Arnd also mentioned, I don't think it will ever be a problem at all. Possibly there will never be any future SoC with mbus that will either allow>32b remap base addresses nor>4G size.

Actually, I think the failure to allow> 32b remap is a mistake on Marvell's part. Linux needs as much low memory as possible, moving things above 4G gives you more low SDRAM.

So I'd hope to see 40bit addressing for MBUS windows in a future chip.

But again, that already works with what Ezequiel has..

To be very clear, the only issue with the 32 bit offset is if we need to describe an offset into a target in DT that is larger than 32 bits. The only use of offsets in DT is for internal regs. The need for a> 32 bit offset in DT does not exist with the current architecture.

Jason

Quoted 9 lines · Jason Gunthorpe

The forms could be:

0IAA0000
FK000000
- K=0 -> internal regs
- K=1 -> PCI-E thingy
etc
1IIAA000 (future expansion example)

Ok, got it. Any encoding is fine that allows to distinguish real remap windows and fake ones. I assumed that maybe someday there will be more than 4b target id so 0x80 as special case indicator leaves 7b of normal target id in the _current_ mapping.

Hi Sebastian,

You loose +1 internet points for dropping me from Cc ;-)

Quoted 11 lines · Sebastian Hesselbarth

Quoted 9 lines · Jason Gunthorpe

The forms could be:

0IAA0000
FK000000
- K=0 -> internal regs
- K=1 -> PCI-E thingy
etc
1IIAA000 (future expansion example)

Quoted 5 lines

Ok, got it. Any encoding is fine that allows to distinguish real remap windows and fake ones. I assumed that maybe someday there will be more than 4b target id so 0x80 as special case indicator leaves 7b of normal target id in the _current_ mapping.

I'm also wondering about why we not care about target IDs being more than 4 bits.

Jason: (I'm checking now but perhaps you know better than me):
Is there any MBus-architectural reason for you assuring the target ID will always be within 4-bits?

Quoted 25 lines · Ezequiel Garcia

Hi Sebastian,

You loose +1 internet points for dropping me from Cc ;-)

Quoted 11 lines · Sebastian Hesselbarth

Quoted 9 lines · Jason Gunthorpe

The forms could be:

0IAA0000
FK000000
- K=0 -> internal regs
- K=1 -> PCI-E thingy
etc
1IIAA000 (future expansion example)

Quoted 5 lines

Ok, got it. Any encoding is fine that allows to distinguish real remap windows and fake ones. I assumed that maybe someday there will be more than 4b target id so 0x80 as special case indicator leaves 7b of normal target id in the _current_ mapping.

I'm also wondering about why we not care about target IDs being more than 4 bits.

Quoted 3 lines

Jason: (I'm checking now but perhaps you know better than me):
Is there any MBus-architectural reason for you assuring the target ID will always be within 4-bits?

The manuals I have for the register set say 4 bits is allocated for mbus targets.. Are yours different?

If they change it then the window register layout changes and then the mbus driver probably need to change as well.

The address decoding window to access the BootROM should not be allocated programatically, but instead declared in the device tree.

Signed-off-by Ezequiel Garcia <[email protected]>

 arch/arm/mach-mvebu/platsmp.c | 1 -
 1 file changed, 1 deletion(-)

arch/arm/mach-mvebu/platsmp.c +1 -0

index 93f2f3a..d419fac 100644
+++ b/arch/arm/mach-mvebu/platsmp.c

Quoted 7 lines · Ezequiel Garcia

The address decoding window to access the BootROM should not be allocated programatically, but instead declared in the device tree.

Signed-off-by Ezequiel Garcia <[email protected]>

 arch/arm/mach-mvebu/platsmp.c | 1 -
 1 file changed, 1 deletion(-)

arch/arm/mach-mvebu/platsmp.c void __init armada_xp_smp_prepare_cpus(unsigned int max_cpus) · line 121

Quoted 9 lines

arch/arm/mach-mvebu/platsmp.c +1 -1

index 93f2f3a..d419fac 100644
+++ b/arch/arm/mach-mvebu/platsmp.c

@@ -118,7 +118,6 @@ void __init armada_xp_smp_prepare_cpus(unsigned int max_cpus)
 	set_secondary_cpus_clock();
 	flush_cache_all();
 	set_cpu_coherent(cpu_logical_map(smp_processor_id()), 0);
-	mvebu_mbus_add_window("bootrom", 0xfff00000, SZ_1M);
 }

I think some kind of test is needed here. As I understand it the SMP startup uses a trampoline in the boot rom and the boot rom *must* be mapped to 0xfff00000 ?

Verifying the DT is setup this way and aborting if it is not seems like a good idea..

Quoted 3 lines

I think some kind of test is needed here. As I understand it the SMP startup uses a trampoline in the boot rom and the boot rom *must* be mapped to 0xfff00000 ?

Yes, that's my understanding as well, but I will do some testing since it should be interesting...

Quoted 2 lines

Verifying the DT is setup this way and aborting if it is not seems like a good idea..

I have no problem doing that, but to me it sounds as it's the responsability of the one that writes the DT, no?

Maybe this is a requirement for this SoC, but not for another... so, why should the kernel *check* for that?

Quoted 6 lines · Ezequiel Garcia

Quoted 3 lines

I think some kind of test is needed here. As I understand it the SMP startup uses a trampoline in the boot rom and the boot rom *must* be mapped to 0xfff00000 ?

Yes, that's my understanding as well, but I will do some testing since it should be interesting...

If it is like the earlier chips you will also have the choice of something based at 0, there is a register bit that sets the reset address.

Quoted 2 lines

Quoted 2 lines

Verifying the DT is setup this way and aborting if it is not seems like a good idea..

Quoted 2 lines

I have no problem doing that, but to me it sounds as it's the responsability of the one that writes the DT, no?

Having the kernel enforce that the DT node is present and at the right location, I think, is helpful for the bootloader folks to ensure they write correct DTs.

Quoted 2 lines

Maybe this is a requirement for this SoC, but not for another... so, why should the kernel *check* for that?

The function was called armada_xp_smp_prepare_cpus - all Armada XP's will work like this..

Quoted 3 lines

Having the kernel enforce that the DT node is present and at the right location, I think, is helpful for the bootloader folks to ensure they write correct DTs.

Granted. But then I wonder... why do we bother to put the BootROM in the DT window if we're going to check for a fixed address it in any case?

Quoted 6 lines · Ezequiel Garcia

Quoted 3 lines

Having the kernel enforce that the DT node is present and at the right location, I think, is helpful for the bootloader folks to ensure they write correct DTs.

Granted. But then I wonder... why do we bother to put the BootROM in the DT window if we're going to check for a fixed address it in any case?

Code re-use in the mbus driver?

Maybe future SOCs in this family will have programmable SMP startup addresses?

Non-SMP systems don't need to map the boot rom at all?

Quoted 7 lines · Ezequiel Garcia

The address decoding window to access the BootROM should not be allocated programatically, but instead declared in the device tree.

Signed-off-by Ezequiel Garcia <[email protected]>

 arch/arm/mach-mvebu/platsmp.c | 1 -
 1 file changed, 1 deletion(-)

arch/arm/mach-mvebu/platsmp.c void __init armada_xp_smp_prepare_cpus(unsigned int max_cpus) · line 121

Quoted 9 lines

arch/arm/mach-mvebu/platsmp.c +1 -1

index 93f2f3a..d419fac 100644
+++ b/arch/arm/mach-mvebu/platsmp.c

@@ -118,7 +118,6 @@ void __init armada_xp_smp_prepare_cpus(unsigned int max_cpus)
 	set_secondary_cpus_clock();
 	flush_cache_all();
 	set_cpu_coherent(cpu_logical_map(smp_processor_id()), 0);
-	mvebu_mbus_add_window("bootrom", 0xfff00000, SZ_1M);
 }

I think some kind of test is needed here. As I understand it the SMP startup uses a trampoline in the boot rom and the boot rom *must* be mapped to 0xfff00000 ?

Verifying the DT is setup this way and aborting if it is not seems like a good idea..

Quoted 2 lines

Verifying the DT is setup this way and aborting if it is not seems like a good idea..

I agree it's a nice idea, but I'm not too sure how to accomplish this in a simple and generic way. There's nothing in the DT that allows you to know which of the ranges entries correspond to the BootROM, unless you go through each of the entries comparing against the known target ID and attribute.

You could also do a 'of_find_node_by_name(NULL, "bootrom");' but the binding no longer requires to even have any children for the window.

Quoted 8 lines · Ezequiel Garcia

Quoted 2 lines

Verifying the DT is setup this way and aborting if it is not seems like a good idea..

I agree it's a nice idea, but I'm not too sure how to accomplish this in a simple and generic way. There's nothing in the DT that allows you to know which of the ranges entries correspond to the BootROM, unless you go through each of the entries comparing against the known target ID and attribute.

I think you need to have a defined compatible string for the bootrom, use one of the of_find.. functions to locate the node, then translate the regs to get a CPU address, ensure it is the right base and size..

I wasn't sure you wanted to panic(), to clip on available CPUs, or to just do a pr_warn / WARN(), so here's a piece of code:
(disclaimer: non-tested, non-compiled, etc.)

	node = of_find_compatible_node(NULL, NULL, "bootrom");

Quoted 3 lines · Ezequiel Garcia

I wasn't sure you wanted to panic(), to clip on available CPUs, or to just do a pr_warn / WARN(), so here's a piece of code:
(disclaimer: non-tested, non-compiled, etc.)

Up to you, but you know it won't work if it isn't right so continuing on to try and startup the CPUs is not ideal. panic is probably reasonable?

Quoted 1 line

	node = of_find_compatible_node(NULL, NULL, "bootrom");

"marvell,bootrom" ?

Now that mbus device tree binding has been introduced, remove the address decoding window management from this driver.
A suitable 'ranges' entry should be added to the devbus-compatible node in the device tree, as described by the mbus binding documentation.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 drivers/memory/mvebu-devbus.c | 64 ++-----------------------------------------
 1 file changed, 2 insertions(+), 62 deletions(-)

drivers/memory/mvebu-devbus.c +2 -62

index 978e8e3..94c9248 100644
@@ -208,16 +208,11 @@ static int mvebu_devbus_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *node = pdev->dev.of_node;
-	struct device_node *parent;
 	struct devbus *devbus;
 	struct resource *res;
 	struct clk *clk;
 	unsigned long rate;
-	const __be32 *ranges;
-	int err, cs;
-	int addr_cells, p_addr_cells, size_cells;
-	int ranges_len, tuple_len;
-	u32 base, size;
+	int err;
 
 	devbus = devm_kzalloc(&pdev->dev, sizeof(struct devbus), GFP_KERNEL);
 	if (!devbus)

@@ -248,68 +243,13 @@ static int mvebu_devbus_probe(struct platform_device *pdev)
 		return err;
 
 	/*
-	 * Allocate an address window for this device.
-	 * If the device probing fails, then we won't be able to
-	 * remove the allocated address decoding window.
-	 *
-	 * FIXME: This is only a temporary hack! We need to do this here
-	 * because we still don't have device tree bindings for mbus.
-	 * Once that support is added, we will declare these address windows
-	 * statically in the device tree, and remove the window configuration
-	 * from here.
-	 */
-
-	/*
-	 * Get the CS to choose the window string.
-	 * This is a bit hacky, but it will be removed once the
-	 * address windows are declared in the device tree.
-	 */
-	cs = (((unsigned long)devbus->base) % 0x400) / 8;
-
-	/*
-	 * Parse 'ranges' property to obtain a (base,size) window tuple.
-	 * This will be removed once the address windows
-	 * are declared in the device tree.
-	 */
-	parent = of_get_parent(node);
-	if (!parent)
-		return -EINVAL;
-
-	p_addr_cells = of_n_addr_cells(parent);
-	of_node_put(parent);
-
-	addr_cells = of_n_addr_cells(node);
-	size_cells = of_n_size_cells(node);
-	tuple_len = (p_addr_cells + addr_cells + size_cells) * sizeof(__be32);
-
-	ranges = of_get_property(node, "ranges", &ranges_len);
-	if (ranges == NULL || ranges_len != tuple_len)
-		return -EINVAL;
-
-	base = of_translate_address(node, ranges + addr_cells);
-	if (base == OF_BAD_ADDR)
-		return -EINVAL;
-	size = of_read_number(ranges + addr_cells + p_addr_cells, size_cells);
-
-	/*
-	 * Create an mbus address windows.
-	 * FIXME: Remove this, together with the above code, once the
-	 * address windows are declared in the device tree.
-	 */
-	err = mvebu_mbus_add_window(devbus_wins[cs], base, size);
-	if (err < 0)
-		return err;
-
-	/*
 	 * We need to create a child device explicitly from here to
 	 * guarantee that the child will be probed after the timing
 	 * parameters for the bus are written.
 	 */
 	err = of_platform_populate(node, NULL, NULL, dev);
-	if (err < 0) {
-		mvebu_mbus_del_window(base, size);
+	if (err < 0)
 		return err;
-	}
 
 	return 0;
 }

Signature · 1 line

1.8.1.5

Quoted 5 lines · Ezequiel Garcia

Now that mbus device tree binding has been introduced, remove the address decoding window management from this driver.
A suitable 'ranges' entry should be added to the devbus-compatible node in the device tree, as described by the mbus binding documentation.

Quoted 4 lines

Signed-off-by Ezequiel Garcia <[email protected]>

---
 drivers/memory/mvebu-devbus.c | 64 ++-----------------------------------------
 1 file changed, 2 insertions(+), 62 deletions(-)

Unfortunately, the patch adding mvebu-devbus is going through gregkh's tree. Either this patch needs to wait for v3.12, or ask Greg if he can take this one.

Dear Jason Cooper,

Quoted 11 lines · Jason Cooper

Quoted 9 lines · Ezequiel Garcia

Now that mbus device tree binding has been introduced, remove the address decoding window management from this driver.
A suitable 'ranges' entry should be added to the devbus-compatible node in the device tree, as described by the mbus binding documentation.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 drivers/memory/mvebu-devbus.c | 64 ++-----------------------------------------
 1 file changed, 2 insertions(+), 62 deletions(-)

Quoted 3 lines

Unfortunately, the patch adding mvebu-devbus is going through gregkh's tree. Either this patch needs to wait for v3.12, or ask Greg if he can take this one.

I believe that what we're looking for right now is a Acked-by from both the Marvell maintainers, Arnd Bergmann and Jason Gunthorpe. Once we get those Acked-by, we can figure out the merge strategy for this patch set.

Quoted 18 lines · Thomas Petazzoni

Dear Jason Cooper,

Quoted 14 lines · Jason Cooper

Quoted 9 lines · Ezequiel Garcia

Now that mbus device tree binding has been introduced, remove the address decoding window management from this driver.
A suitable 'ranges' entry should be added to the devbus-compatible node in the device tree, as described by the mbus binding documentation.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 drivers/memory/mvebu-devbus.c | 64 ++-----------------------------------------
 1 file changed, 2 insertions(+), 62 deletions(-)

Unfortunately, the patch adding mvebu-devbus is going through gregkh's tree. Either this patch needs to wait for v3.12, or ask Greg if he can take this one.

Quoted 4 lines

I believe that what we're looking for right now is a Acked-by from both the Marvell maintainers, Arnd Bergmann and Jason Gunthorpe. Once we get those Acked-by, we can figure out the merge strategy for this patch set.

Right. I was more looking to give Ezequiel a heads up

arch/arm/boot/dts/armada-370-xp.dtsi line 46

Quoted 10 lines · Ezequiel Garcia

arch/arm/boot/dts/armada-370-xp.dtsi +9 -0

+               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.

Quoted 12 lines · Arnd Bergmann

Quoted 10 lines · Ezequiel Garcia

arch/arm/boot/dts/armada-370-xp.dtsi +9 -0

+               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";
+               };

Quoted 5 lines

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?

Quoted 12 lines · Arnd Bergmann

arch/arm/boot/dts/armada-370-xp.dtsi line 46

Quoted 10 lines · Ezequiel Garcia

arch/arm/boot/dts/armada-370-xp.dtsi +9 -0

+               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";
+               };

Quoted 5 lines

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.

Quoted 4 lines

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.

arch/arm/boot/dts/armada-370.dtsi +4 -0

+                               interrupt-map-mask = <0 0 0 0>;
+                               interrupt-map = <0 0 0 0 &mpic 58>;
+                               marvell,pcie-port = <0>;
+                               marvell,pcie-lane = <0>;

To clarify my earlier comment, I think it would be nicer to write this as

                       ranges =
                              <0x82000000 0 0x40000    0xffff0001 0x40000 0 0x00002000
                               0x82000000 0 0x80000    0xffff0001 0x80000 0 0x00002000
                               0x82000000 1 0 MBUS_ID(0x12, 0x34) 0  1 0
                               0x82000000 2 0 MBUS_ID(0x13, 0x34) 0  1 0
                               0x81000000 1 0 MBUS_ID(0x12, 0x35) 0 0 0x10000;
                               0x81000000 2 0 MBUS_ID(0x13, 0x35) 0 0 0x10000>;

The MBUS_ID numbers above are made up since I don't know them, but this way you can describe how the entire 4GB MMIO address space of the PCI bus is mapped into the MBUS address space.

Does this make sense?

Quoted 10 lines

To clarify my earlier comment, I think it would be nicer to write this as

                       ranges =
                              <0x82000000 0 0x40000    0xffff0001 0x40000 0 0x00002000
                               0x82000000 0 0x80000    0xffff0001 0x80000 0 0x00002000
                               0x82000000 1 0 MBUS_ID(0x12, 0x34) 0  1 0
                               0x82000000 2 0 MBUS_ID(0x13, 0x34) 0  1 0
                               0x81000000 1 0 MBUS_ID(0x12, 0x35) 0 0 0x10000;
                               0x81000000 2 0 MBUS_ID(0x13, 0x35) 0 0 0x10000>;

Quoted 3 lines

The MBUS_ID numbers above are made up since I don't know them, but this way you can describe how the entire 4GB MMIO address space of the PCI bus is mapped into the MBUS address space.

This is NOT possible because we don't know in advance how much memory space and I/O space each PCIe device will require.

Arnd, we've discussed this at length with you while getting the PCIe driver merged, and we've explained this to you numerous times. Could you please understand that any of your proposal that suggests writing down static windows for PCIe devices will not work?

Quoted 15 lines

Quoted 11 lines

                       ranges =
                              <0x82000000 0 0x40000    0xffff0001 0x40000 0 0x00002000
                               0x82000000 0 0x80000    0xffff0001 0x80000 0 0x00002000
                               0x82000000 1 0 MBUS_ID(0x12, 0x34) 0  1 0
                               0x82000000 2 0 MBUS_ID(0x13, 0x34) 0  1 0
                               0x81000000 1 0 MBUS_ID(0x12, 0x35) 0 0 0x10000;
                               0x81000000 2 0 MBUS_ID(0x13, 0x35) 0 0 0x10000>;

The MBUS_ID numbers above are made up since I don't know them, but this way you can describe how the entire 4GB MMIO address space of the PCI bus is mapped into the MBUS address space.

This is NOT possible because we don't know in advance how much memory space and I/O space each PCIe device will require.

Quoted 4 lines

Arnd, we've discussed this at length with you while getting the PCIe driver merged, and we've explained this to you numerous times. Could you please understand that any of your proposal that suggests writing down static windows for PCIe devices will not work?

Where did I suggest static windows for PCIe devices?

Quoted 20 lines · Arnd Bergmann

Quoted 19 lines

Quoted 11 lines

                       ranges =
                              <0x82000000 0 0x40000    0xffff0001 0x40000 0 0x00002000
                               0x82000000 0 0x80000    0xffff0001 0x80000 0 0x00002000
                               0x82000000 1 0 MBUS_ID(0x12, 0x34) 0  1 0
                               0x82000000 2 0 MBUS_ID(0x13, 0x34) 0  1 0
                               0x81000000 1 0 MBUS_ID(0x12, 0x35) 0 0 0x10000;
                               0x81000000 2 0 MBUS_ID(0x13, 0x35) 0 0 0x10000>;

The MBUS_ID numbers above are made up since I don't know them, but this way you can describe how the entire 4GB MMIO address space of the PCI bus is mapped into the MBUS address space.

This is NOT possible because we don't know in advance how much memory space and I/O space each PCIe device will require.

Arnd, we've discussed this at length with you while getting the PCIe driver merged, and we've explained this to you numerous times. Could you please understand that any of your proposal that suggests writing down static windows for PCIe devices will not work?

Quoted 1 line

Where did I suggest static windows for PCIe devices?

Where does your new proposal buys us anything useful compared to the existing PCIe DT binding that has been discussed at length with you?

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Arnd, we've discussed this at length with you while getting the PCIe driver merged, and we've explained this to you numerous times. Could you please understand that any of your proposal that suggests writing down static windows for PCIe devices will not work?

Quoted 4 lines

Quoted 1 line

Where did I suggest static windows for PCIe devices?

Where does your new proposal buys us anything useful compared to the existing PCIe DT binding that has been discussed at length with you?

I'm pretty sure I explained the idea above originally and was ignored. Jason Gunthorpe might remember better, but I think he liked it when I originally proposed doing it this way.

* Since the host physical address for the PCIe memory space window is set up dynamically anyway, there is no reason to hardcode it in DT. We want it to be as large as possible, and this way the mbus driver can just pick the largest free area itself after setting up all the other mappings from the ranges property.

Quoted 5 lines · Arnd Bergmann

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Arnd, we've discussed this at length with you while getting the PCIe driver merged, and we've explained this to you numerous times. Could you please understand that any of your proposal that suggests writing down static windows for PCIe devices will not work?

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

Where did I suggest static windows for PCIe devices?

Where does your new proposal buys us anything useful compared to the existing PCIe DT binding that has been discussed at length with you?

I'm pretty sure I explained the idea above originally and was ignored. Jason Gunthorpe might remember better, but I think he liked it when I originally proposed doing it this way.

I remember it took a bit to understand your proposal, but I thought it could work, but I admit I forget all the little details now :(

Ah, if I can just rephrase simply - the notion was to move the determination of the aperture to use dynmic allocation and then restructure the ranges around the mbus target, since they no longer need to encode the aperture.

My concern: dynamically sizing the aperture is hard. There are three apertures that need to be picked, and the PCI core code has no support for dynamic apertures. Getting the aperture from the DT is a functional compromise.

Quoted 5 lines

* Since the host physical address for the PCIe memory space window is set up dynamically anyway, there is no reason to hardcode it in DT. We want it to be as large as possible, and this way the mbus driver can just pick the largest free area itself after setting up all the other mappings from the ranges property.

This seems to get really complicated if the mbus driver is ever required to support dynamic mappings.. If PCI-E claims all memory and then you modprobe something it could fail.

IMHO, I go back to my original thoughts. There is no real need for any of this to be dynamic, we can use the values in the DT, presumably set by the bootloader and things will work well.

The added complexity and failure modes for dynamic is simply not worth it..

Quoted 16 lines · Jason Gunthorpe

Quoted 5 lines · Arnd Bergmann

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Arnd, we've discussed this at length with you while getting the PCIe driver merged, and we've explained this to you numerous times. Could you please understand that any of your proposal that suggests writing down static windows for PCIe devices will not work?

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

Where did I suggest static windows for PCIe devices?

Where does your new proposal buys us anything useful compared to the existing PCIe DT binding that has been discussed at length with you?

I'm pretty sure I explained the idea above originally and was ignored. Jason Gunthorpe might remember better, but I think he liked it when I originally proposed doing it this way.

Quoted 7 lines

I remember it took a bit to understand your proposal, but I thought it could work, but I admit I forget all the little details now :(

Ah, if I can just rephrase simply - the notion was to move the determination of the aperture to use dynmic allocation and then restructure the ranges around the mbus target, since they no longer need to encode the aperture.

Right.

Quoted 4 lines

My concern: dynamically sizing the aperture is hard. There are three apertures that need to be picked, and the PCI core code has no support for dynamic apertures. Getting the aperture from the DT is a functional compromise.

After some discussion on IRC with Ezequiel, I think it's best to leave the aperture listed in DT but say in the binding that the OS may override it.

Quoted 6 lines

IMHO, I go back to my original thoughts. There is no real need for any of this to be dynamic, we can use the values in the DT, presumably set by the bootloader and things will work well.

The added complexity and failure modes for dynamic is simply not worth it..

I don't think it's too hard to be prepared for fully dynamic operation. As Grant said in his comment on v2, the real complexity comes from the fact that we are mixing dynamic and static configuration here, and the PCIe configuration is inherently dynamic.

The change I'm proposing would just mean the DT representation reflects the dynamic nature of the PCIe windows.

Although I'd like the binding to take this into account, for there's no point in restricting it -a priori- I can't see any advantage on doing fully dynamic window configuration on devices that are fixed in the first place. It sounds like bloating the whole thing without a strong need.

Review

Quoted 6 lines · Ezequiel Garcia

(quoted diff) +6 -0

+
+                       ranges =
+                              <0x82000000 0 0x40000    0xffff0001 0x40000 0 0x00002000
+                               0x82000000 0 0x80000    0xffff0001 0x80000 0 0x00002000
+                               0x82000000 0 0xe0000000 0xffff0002 0          0 0x08000000
+                               0x81000000 0 0          0xffff0002 0x8000000  0 0x00100000>;

As pointed out on IRC, this is not a good representation of the memory space, since it requires a non-zero sys->mem_offset, and it conflicts with the straight mapping I suggested.

I think it should be

                               0x82000000 0 0xe0000000 0xffff0002 0  0xe0000000 0x08000000

so that mbus actually translates the right addresses. You could also have the PCI memory space start at 0, which would mean

                               0x82000000 0 0 0xffff0002 0  0 0x08000000

and

                              0xffff0002 0 0xe0000000 0x8100000

Note that the driver doesn't actually handle the generic case correctly, you would need to apply this patch

drivers/pci/host/pci-mvebu.c +1 -0

index 13a633b..aa674f4 100644
@@ -790,6 +790,7 @@ static int __init mvebu_pcie_probe(struct platform_device *pdev)
 		}
 		if (restype == IORESOURCE_MEM) {
 			of_pci_range_to_resource(&range, np, &pcie->mem);
+			sys->mem_offset = range.cpu_addr - range.pci_addr;
 			pcie->mem.name = "MEM";
 		}
 	}

to deal with the generic case where the bus address is different from the CPU address.

Arnd,

Going through this a couple questions came out...

Quoted 8 lines · Arnd Bergmann

Review

Quoted 6 lines · Ezequiel Garcia

(quoted diff) +6 -0

+
+                       ranges =
+                              <0x82000000 0 0x40000    0xffff0001 0x40000 0 0x00002000
+                               0x82000000 0 0x80000    0xffff0001 0x80000 0 0x00002000
+                               0x82000000 0 0xe0000000 0xffff0002 0          0 0x08000000
+                               0x81000000 0 0          0xffff0002 0x8000000  0 0x00100000>;

Quoted 7 lines

As pointed out on IRC, this is not a good representation of the memory space, since it requires a non-zero sys->mem_offset, and it conflicts with the straight mapping I suggested.

I think it should be

                               0x82000000 0 0xe0000000 0xffff0002 0  0xe0000000 0x08000000

So far, so good.

Quoted 5 lines

if we want to encode the aperture in the ranges property here, i.e. have a 1:1 mapping between PCI memory space and MBUS space, and in mbus, you need the corresponding

Review

Quoted 2 lines

(quoted diff) +1 -1

-                           0xffff0002 0 0xe0000000 0x8100000
+                           0xffff0002 0xe0000000 0xe0000000 0x8100000

... I obviously got something wrong, but I thought you said that this change allowed to *not* have any mbus-node ranges translation.

Quoted 8 lines

so that mbus actually translates the right addresses. You could also have the PCI memory space start at 0, which would mean

                               0x82000000 0 0 0xffff0002 0  0 0x08000000

and

                              0xffff0002 0 0xe0000000 0x8100000

Mmm.. and why is this option acceptable?

Quoted 4 lines

Note that the driver doesn't actually handle the generic case correctly, you would need to apply this patch

This patch does not apply. I tried to understand what you did, but I'm confused by the fact I don't need to apply any patch to make it work on my boxes, using your proposed ranges translations.

Quoted 13 lines

drivers/pci/host/pci-mvebu.c +1 -0

index 13a633b..aa674f4 100644
@@ -790,6 +790,7 @@ static int __init mvebu_pcie_probe(struct platform_device *pdev)
 		}
 		if (restype == IORESOURCE_MEM) {
 			of_pci_range_to_resource(&range, np, &pcie->mem);
+			sys->mem_offset = range.cpu_addr - range.pci_addr;
 			pcie->mem.name = "MEM";
 		}
 	}

Quoted 2 lines

to deal with the generic case where the bus address is different from the CPU address.

Quoted 1 line

Mmm.. and why is this option acceptable?

As I explained on IRC, there is no requirement to pick a specific bus aperture. The only two sensible choices are to make the bus address the same as the CPU address, or to make the bus address start at 0, which is what this does.

Today 18:53:48

Quoted 6 lines · Arnd Bergmann

Quoted 1 line

Mmm.. and why is this option acceptable?

As I explained on IRC, there is no requirement to pick a specific bus aperture. The only two sensible choices are to make the bus address the same as the CPU address, or to make the bus address start at 0, which is what this does.

PCI bus addresses must not alias other addresess in the system or you'll get weirdness. For instance DMA initiated from the PCI bus at address 0, intended to read from SDRAM at 0 must not be claimed by another device on the PCI bus. IMHO, a 1:1 mapping between PCI and CPU is strongly preferred. Any other configuration will need some additional techniques to avoid aliasing.

In the example below there's an extract of a device tree showing how the internal-regs and pcie nodes can be represented:

	#define MBUS_ID(target,attributes) (((target) << 24) | ((attributes) << 16))

	soc {
		compatible = "marvell,armadaxp-mbus";
		reg = <0 0xd0020000 0 0x100>, <0 0xd0020180 0 0x20>;

		ranges = <0xffff0001 0 0 0xd0000000 0x100000   /* internal-regs */
			  0xffff0000 0 0 0xe0000000 0x8100000  /* pcie */
			  MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000
			  MBUS_ID(0x01, 0x2f) 0 0 0xf0000000 0x8000000>;

		pcie-controller {
			compatible = "marvell,armada-xp-pcie";
			status = "okay";
			device_type = "pci";

			#address-cells = <3>;
			#size-cells = <2>;

			ranges =
			       <0x82000000 0 0x40000 0xffff0001 0x40000 0 0x00002000   /* Port 0.0 registers */

				0x82000000 0 0x42000 0xffff0001 0x42000 0 0x00002000   /* Port 2.0 registers */
				0x82000000 0 0x44000 0xffff0001 0x44000 0 0x00002000   /* Port 0.1 registers */
				0x82000000 0 0x48000 0xffff0001 0x48000 0 0x00002000   /* Port 0.2 registers */
				0x82000000 0 0x4c000 0xffff0001 0x4c000 0 0x00002000   /* Port 0.3 registers */
				0x82000000 0 0x80000 0xffff0001 0x80000 0 0x00002000   /* Port 1.0 registers */
				0x82000000 0 0x82000 0xffff0001 0x82000 0 0x00002000   /* Port 3.0 registers */
				0x82000000 0 0xe0000000 0xffff0000 0 0 0x08000000   /* non-prefetchable memory */
				0x81000000 0 0 0xffff0000 0x8000000 0 0x00100000>; /* downstream I/O */

Changes from v2:
* Replaced the PCIe mapping with 0xffff0002, to avoid using a representation that might correspond to a possible window id.

Quoted 9 lines · Ezequiel Garcia

In the example below there's an extract of a device tree showing how the internal-regs and pcie nodes can be represented:

	#define MBUS_ID(target,attributes) (((target) << 24) | ((attributes) << 16))

	soc {
		compatible = "marvell,armadaxp-mbus";
		reg = <0 0xd0020000 0 0x100>, <0 0xd0020180 0 0x20>;

Quoted 4 lines

		ranges = <0xffff0001 0 0 0xd0000000 0x100000   /* internal-regs */
			  0xffff0000 0 0 0xe0000000 0x8100000  /* pcie */
			  MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000
			  MBUS_ID(0x01, 0x2f) 0 0 0xf0000000 0x8000000>;

Ezequiel,

you should update PCIE above (and possibly anywhere else) too.

Quoted 10 lines

		pcie-controller {
			compatible = "marvell,armada-xp-pcie";
			status = "okay";
			device_type = "pci";

			#address-cells = <3>;
			#size-cells = <2>;

			ranges =
			       <0x82000000 0 0x40000 0xffff0001 0x40000 0 0x00002000   /* Port 0.0 registers */

Quoted 8 lines

				0x82000000 0 0x42000 0xffff0001 0x42000 0 0x00002000   /* Port 2.0 registers */
				0x82000000 0 0x44000 0xffff0001 0x44000 0 0x00002000   /* Port 0.1 registers */
				0x82000000 0 0x48000 0xffff0001 0x48000 0 0x00002000   /* Port 0.2 registers */
				0x82000000 0 0x4c000 0xffff0001 0x4c000 0 0x00002000   /* Port 0.3 registers */
				0x82000000 0 0x80000 0xffff0001 0x80000 0 0x00002000   /* Port 1.0 registers */
				0x82000000 0 0x82000 0xffff0001 0x82000 0 0x00002000   /* Port 3.0 registers */
				0x82000000 0 0xe0000000 0xffff0000 0 0 0x08000000   /* non-prefetchable memory */
				0x81000000 0 0 0xffff0000 0x8000000 0 0x00100000>; /* downstream I/O */

Here for example.

Quoted 3 lines

Changes from v2:
* Replaced the PCIe mapping with 0xffff0002, to avoid using a representation that might correspond to a possible window id.

Out of curiosity, how will these fake mappings play with LPAE? If you extend possible address space to >32b the lower bits may belong to valid addresses.
Maybe you can (already do?) ignore that because of the 32b address restriction for internal registers IIRC Thomas mentioned?