Re: [PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes
Arnd,
Going through this a couple questions came out...
On Tue, Jun 18, 2013 at 11:35:50PM +0200, Arnd Bergmann wrote:
On Tuesday 18 June 2013, Ezequiel Garcia wrote:
+
+ ranges =
+ 0x8200 0 0x40x0001 0x4
0 0x2000
+ 0x8200 0 0x80x0001 0x8
0 0x2000
+ 0x8200 0 0xe000 0x0002 0
0 0x0800
+ 0x8100 0 0 0x0002
0x800 0 0x0010;
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
0x8200 0 0xe000 0x0002 0
0xe000 0x0800
So far, so good.
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
- 0x0002 0 0xe000 0x810
+ 0x0002 0xe000 0xe000 0x810
... I obviously got something wrong, but I thought you said that
this change allowed to *not* have any mbus-node ranges translation.
so that mbus actually translates the right addresses. You could also
have the PCI memory space start at 0, which would mean
0x8200 0 0 0x0002 0 0 0x0800
and
0x0002 0 0xe000 0x810
Mmm.. and why is this option acceptable?
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.
diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 13a633b..aa674f4 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -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.
Thanks!
--
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes
On Tuesday 18 June 2013, Ezequiel Garcia wrote: 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. After the suggestions that Grant made about the of_bus, I think the fully dynamic model would actually be simpler than what you have here. You wouldn't actually have to dissect the ranges property at all, just keep the mapping table in memory for all devices that are in use, with a special case for the internal-regs. But I think that's fine, we can alway simplify the code later as long as the binding covers all cases. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes
On Wednesday 19 June 2013, Ezequiel Garcia wrote:
Arnd,
Going through this a couple questions came out...
On Tue, Jun 18, 2013 at 11:35:50PM +0200, Arnd Bergmann wrote:
On Tuesday 18 June 2013, Ezequiel Garcia wrote:
+
+ ranges =
+ 0x8200 0 0x40x0001
0x4 0 0x2000
+ 0x8200 0 0x80x0001
0x8 0 0x2000
+ 0x8200 0 0xe000 0x0002 0
0 0x0800
+ 0x8100 0 0 0x0002
0x800 0 0x0010;
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
0x8200 0 0xe000 0x0002 0
0xe000 0x0800
So far, so good.
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
- 0x0002 0 0xe000 0x810
+ 0x0002 0xe000 0xe000 0x810
... I obviously got something wrong, but I thought you said that
this change allowed to *not* have any mbus-node ranges translation.
I wasn't making a specific requirement here on whether you have that
in the mbus ranges or not, but if you put it in there, you have to adapt
the contents as above.
so that mbus actually translates the right addresses. You could also
have the PCI memory space start at 0, which would mean
0x8200 0 0 0x0002 0 0 0x0800
and
0x0002 0 0xe000 0x810
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.
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.
diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 13a633b..aa674f4 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -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.
This patch is needed only if you change the ranges to have the bus
aperture start at 0. However, if we pick a different way to specify
the aperture than putting it into both ranges properties, that point
is moot, since you wouldn't use of_pci_range_to_resource here
anyway.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes
On Wed, Jun 19, 2013 at 02:11:58PM +0200, Arnd Bergmann wrote: 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. Jason ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes
On Wednesday 19 June 2013, Jason Gunthorpe wrote: Today 18:53:48 On Wed, Jun 19, 2013 at 02:11:58PM +0200, Arnd Bergmann wrote: 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. Ah, good point. You are obviously right, it should definitely be a 1:1 mapping, anything else just creates a mess. I was working on a system like that before, it wasn't pretty (you have to provide separate dma_map_ops then). Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
[PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes
Now that mbus has been added to the device tree, it's possible to
move the PCIe nodes out of internal registers, placing it directly
below the mbus. This is a more accurate representation of the
hardware.
Signed-off-by: Ezequiel Garcia ezequiel.gar...@free-electrons.com
---
arch/arm/boot/dts/armada-370-db.dts | 1 +
arch/arm/boot/dts/armada-370-mirabox.dts | 33 +--
arch/arm/boot/dts/armada-370-rd.dts | 1 +
arch/arm/boot/dts/armada-370.dtsi| 97
4 files changed, 68 insertions(+), 64 deletions(-)
diff --git a/arch/arm/boot/dts/armada-370-db.dts
b/arch/arm/boot/dts/armada-370-db.dts
index c313968..6891343 100644
--- a/arch/arm/boot/dts/armada-370-db.dts
+++ b/arch/arm/boot/dts/armada-370-db.dts
@@ -31,6 +31,7 @@
soc {
ranges = 0x0001 0 0xd000 0x10
+ 0x0002 0 0xe000 0x810
MBUS_ID(0x01, 0xe0) 0 0xfff0 0x10;
internal-regs {
diff --git a/arch/arm/boot/dts/armada-370-mirabox.dts
b/arch/arm/boot/dts/armada-370-mirabox.dts
index abb1ccf..830727a 100644
--- a/arch/arm/boot/dts/armada-370-mirabox.dts
+++ b/arch/arm/boot/dts/armada-370-mirabox.dts
@@ -26,8 +26,25 @@
soc {
ranges = 0x0001 0 0xd000 0x10
+ 0x0002 0 0xe000 0x810
MBUS_ID(0x01, 0xe0) 0 0xfff0 0x10;
+ pcie-controller {
+ status = okay;
+
+ /* Internal mini-PCIe connector */
+ pcie@1,0 {
+ /* Port 0, Lane 0 */
+ status = okay;
+ };
+
+ /* Connected on the PCB to a USB 3.0 XHCI controller */
+ pcie@2,0 {
+ /* Port 1, Lane 0 */
+ status = okay;
+ };
+ };
+
internal-regs {
serial@12000 {
clock-frequency = 2;
@@ -122,22 +139,6 @@
reg = 0x25;
};
};
-
- pcie-controller {
- status = okay;
-
- /* Internal mini-PCIe connector */
- pcie@1,0 {
- /* Port 0, Lane 0 */
- status = okay;
- };
-
- /* Connected on the PCB to a USB 3.0 XHCI
controller */
- pcie@2,0 {
- /* Port 1, Lane 0 */
- status = okay;
- };
- };
};
};
};
diff --git a/arch/arm/boot/dts/armada-370-rd.dts
b/arch/arm/boot/dts/armada-370-rd.dts
index 9ae8bdc..132cf8e 100644
--- a/arch/arm/boot/dts/armada-370-rd.dts
+++ b/arch/arm/boot/dts/armada-370-rd.dts
@@ -29,6 +29,7 @@
soc {
ranges = 0x0001 0 0xd000 0x10
+ 0x0002 0 0xe000 0x810
MBUS_ID(0x01, 0xe0) 0 0xfff0 0x10;
internal-regs {
diff --git a/arch/arm/boot/dts/armada-370.dtsi
b/arch/arm/boot/dts/armada-370.dtsi
index c7f9971..1288a78 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -38,6 +38,55 @@
ranges = 0 MBUS_ID(0x01, 0xe0) 0 0x;
};
+ pcie-controller {
+ compatible = marvell,armada-370-pcie;
+ status = disabled;
+ device_type = pci;
+
+ #address-cells = 3;
+ #size-cells = 2;
+
+ bus-range = 0x00 0xff;
+
+ ranges =
+ 0x8200 0 0x40x0001 0x4 0
0x2000
+ 0x8200 0 0x80x0001 0x8 0
0x2000
+ 0x8200 0 0xe000 0x0002 0 0
0x0800
+ 0x8100 0 0 0x0002 0x800 0
0x0010;
+
+ pcie@1,0 {
+ device_type = pci;
+ assigned-addresses = 0x82000800 0 0x4 0
0x2000;
+ reg = 0x0800 0 0 0 0;
+ #address-cells = 3;
+ #size-cells = 2;
+ #interrupt-cells = 1;
+ ranges;
+ interrupt-map-mask = 0 0 0 0;
+
Re: [PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes
On Tuesday 18 June 2013, Ezequiel Garcia wrote:
+ ranges =
+ 0x8200 0 0x40x0001 0x4 0
0x2000
+ 0x8200 0 0x80x0001 0x8 0
0x2000
+ 0x8200 0 0xe000 0x0002 0
0 0x0800
+ 0x8100 0 0 0x0002 0x800
0 0x0010;
To clarify my earlier comment, I think it would be nicer to write this as
ranges =
0x8200 0 0x40x0001 0x4 0
0x2000
0x8200 0 0x80x0001 0x8 0
0x2000
0x8200 1 0 MBUS_ID(0x12, 0x34) 0 1 0
0x8200 2 0 MBUS_ID(0x13, 0x34) 0 1 0
0x8100 1 0 MBUS_ID(0x12, 0x35) 0 0 0x1;
0x8100 2 0 MBUS_ID(0x13, 0x35) 0 0 0x1;
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.
+ pcie@1,0 {
+ device_type = pci;
+ assigned-addresses = 0x82000800 0 0x4 0
0x2000;
+ reg = 0x0800 0 0 0 0;
+ #address-cells = 3;
+ #size-cells = 2;
+ #interrupt-cells = 1;
+ ranges;
Then you do a ranges property for each port with the high-order
address word equal to the port number:
ranges = 0x8200 1 0 0x8200 0 0 1 0
0x8100 1 0 0x8100 0 0 0 0x1;
+ interrupt-map-mask = 0 0 0 0;
+ interrupt-map = 0 0 0 0 mpic 58;
+ marvell,pcie-port = 0;
+ marvell,pcie-lane = 0;
+ clocks = gateclk 5;
+ status = disabled;
+ pcie@2,0 {
+ device_type = pci;
+ assigned-addresses = 0x82002800 0 0x8 0
0x2000;
+ reg = 0x1000 0 0 0 0;
+ #address-cells = 3;
+ #size-cells = 2;
+ #interrupt-cells = 1;
+ ranges;
ranges = 0x8200 2 0 0x8200 0 0 1 0
0x8100 2 0 0x8100 0 0 0 0x1;
+ interrupt-map-mask = 0 0 0 0;
+ interrupt-map = 0 0 0 0 mpic 62;
+ marvell,pcie-port = 1;
+ marvell,pcie-lane = 0;
+ clocks = gateclk 9;
+ status = disabled;
+ };
Does this make sense?
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes
Dear Arnd Bergmann, On Tue, 18 Jun 2013 18:29:35 +0200, Arnd Bergmann wrote: To clarify my earlier comment, I think it would be nicer to write this as ranges = 0x8200 0 0x40x0001 0x4 0 0x2000 0x8200 0 0x80x0001 0x8 0 0x2000 0x8200 1 0 MBUS_ID(0x12, 0x34) 0 1 0 0x8200 2 0 MBUS_ID(0x13, 0x34) 0 1 0 0x8100 1 0 MBUS_ID(0x12, 0x35) 0 0 0x1; 0x8100 2 0 MBUS_ID(0x13, 0x35) 0 0 0x1; 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? Does this make sense? Not at all. Please read once again the hundreds of e-mails we've exchanged about the need for dynamic windows for PCIe devices, which lead us to have the emulated PCI-to-PCI bridge stuff. I'm starting to be fed up to re-explain this to you over-and-over again. Best regards, Thomas -- Thomas Petazzoni, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes
Dear Arnd Bergmann, On Tue, 18 Jun 2013 19:18:58 +0200, Arnd Bergmann wrote: ranges = 0x8200 0 0x40x0001 0x4 0 0x2000 0x8200 0 0x80x0001 0x8 0 0x2000 0x8200 1 0 MBUS_ID(0x12, 0x34) 0 1 0 0x8200 2 0 MBUS_ID(0x13, 0x34) 0 1 0 0x8100 1 0 MBUS_ID(0x12, 0x35) 0 0 0x1; 0x8100 2 0 MBUS_ID(0x13, 0x35) 0 0 0x1; 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? 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? Thomas -- Thomas Petazzoni, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes
On Tuesday 18 June 2013, Thomas Petazzoni wrote: To clarify my earlier comment, I think it would be nicer to write this as ranges = 0x8200 0 0x40x0001 0x4 0 0x2000 0x8200 0 0x80x0001 0x8 0 0x2000 0x8200 1 0 MBUS_ID(0x12, 0x34) 0 1 0 0x8200 2 0 MBUS_ID(0x13, 0x34) 0 1 0 0x8100 1 0 MBUS_ID(0x12, 0x35) 0 0 0x1; 0x8100 2 0 MBUS_ID(0x13, 0x35) 0 0 0x1; 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? Where did I suggest static windows for PCIe devices? Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes
On Tuesday 18 June 2013, Thomas Petazzoni wrote: On Tue, 18 Jun 2013 19:18:58 +0200, Arnd Bergmann wrote: ranges = 0x8200 0 0x40x0001 0x4 0 0x2000 0x8200 0 0x80x0001 0x8 0 0x2000 0x8200 1 0 MBUS_ID(0x12, 0x34) 0 1 0 0x8200 2 0 MBUS_ID(0x13, 0x34) 0 1 0 0x8100 1 0 MBUS_ID(0x12, 0x35) 0 0 0x1; 0x8100 2 0 MBUS_ID(0x13, 0x35) 0 0 0x1; 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? 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. The main differences are: * It unifies the binding for the PCIe case and all other registers. There is no need to treat PCIe special in the binding this way, which is more future-proof and more consistent. * 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. * The binding actually allows the PCIe translation to be discontiguous, so if we want to improve the code in the future, we can fill all the holes in the mbus space with PCIe windows without changing the binding. * It separates hardware description (in the PCIe node) from policy (in the mbus node), just like we do for all the other mbus children. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes
On Tue, Jun 18, 2013 at 08:22:08PM +0200, Arnd Bergmann wrote: 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? 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. * 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.. Jason ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes
On Tuesday 18 June 2013, Jason Gunthorpe wrote: On Tue, Jun 18, 2013 at 08:22:08PM +0200, Arnd Bergmann wrote: 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? 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. Right. 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. I would still want to see the actual MBUS IDs in the ranges property of the pci-controller nodes. Right now, the pcie driver has to call into the mbus driver passing a hardcoded string for setting up the mapping, which is then used to look up the MBUS ID. This is rather inconsistent and we should have all that information in DT. Ideally we should also use it, to ensure it's correct but for 3.11 it would be enough to get the DT representation right. We can replace the string passing in 3.12. * 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. Yes, good point. However that is something we can figure out if it comes to that. With my suggestion of setting up the mbus windows in the of_bus xlate function, we would already create them at boot time when of_platform_populate gets called, so that wouldn't be a problem. 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. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes
On Tuesday 18 June 2013, Ezequiel Garcia wrote:
+
+ ranges =
+ 0x8200 0 0x40x0001 0x4 0
0x2000
+ 0x8200 0 0x80x0001 0x8 0
0x2000
+ 0x8200 0 0xe000 0x0002 0
0 0x0800
+ 0x8100 0 0 0x0002 0x800
0 0x0010;
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
0x8200 0 0xe000 0x0002 0 0xe000
0x0800
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
- 0x0002 0 0xe000 0x810
+ 0x0002 0xe000 0xe000 0x810
so that mbus actually translates the right addresses. You could also
have the PCI memory space start at 0, which would mean
0x8200 0 0 0x0002 0 0 0x0800
and
0x0002 0 0xe000 0x810
Note that the driver doesn't actually handle the generic case correctly, you
would
need to apply this patch
diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 13a633b..aa674f4 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -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
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes
On Tue, Jun 18, 2013 at 11:20:07PM +0200, Arnd Bergmann wrote: On Tuesday 18 June 2013, Jason Gunthorpe wrote: On Tue, Jun 18, 2013 at 08:22:08PM +0200, Arnd Bergmann wrote: 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? 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. Right. 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. Yes, I'll send a v4 soon, and I'll try to address this correctly, as you're suggesting. [...] 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. -- Ezequiel García, Free Electrons Embedded Linux, Kernel and Android Engineering http://free-electrons.com ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
