| Message ID | 1371554737-25319-12-git-send-email-ezequiel.garcia@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tuesday 18 June 2013, Ezequiel Garcia wrote: > + 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>; 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. > + pcie@1,0 { > + device_type = "pci"; > + assigned-addresses = <0x82000800 0 0x40000 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 = <0x82000000 1 0 0x82000000 0 0 1 0 0x81000000 1 0 0x81000000 0 0 0 0x10000>; > + 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 0x80000 0 0x2000>; > + reg = <0x1000 0 0 0 0>; > + #address-cells = <3>; > + #size-cells = <2>; > + #interrupt-cells = <1>; > + ranges; ranges = <0x82000000 2 0 0x82000000 0 0 1 0 0x81000000 2 0 0x81000000 0 0 0 0x10000>; > + 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
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 = > <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? > 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
On Tuesday 18 June 2013, Thomas Petazzoni wrote: > > 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. > > 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
Dear Arnd Bergmann, On Tue, 18 Jun 2013 19:18:58 +0200, Arnd Bergmann wrote: > > > 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? > > 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
On Tuesday 18 June 2013, Thomas Petazzoni wrote: > On Tue, 18 Jun 2013 19:18:58 +0200, Arnd Bergmann wrote: > > > > > 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? > > > > 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
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
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
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.
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
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 = <0xffff0001 0 0xd0000000 0x100000 + 0xffff0002 0 0xe0000000 0x8100000 MBUS_ID(0x01, 0xe0) 0 0xfff00000 0x100000>; 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 = <0xffff0001 0 0xd0000000 0x100000 + 0xffff0002 0 0xe0000000 0x8100000 MBUS_ID(0x01, 0xe0) 0 0xfff00000 0x100000>; + 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 = <200000000>; @@ -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 = <0xffff0001 0 0xd0000000 0x100000 + 0xffff0002 0 0xe0000000 0x8100000 MBUS_ID(0x01, 0xe0) 0 0xfff00000 0x100000>; 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 0xffffffff>; }; + pcie-controller { + compatible = "marvell,armada-370-pcie"; + status = "disabled"; + device_type = "pci"; + + #address-cells = <3>; + #size-cells = <2>; + + bus-range = <0x00 0xff>; + + 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>; + + pcie@1,0 { + device_type = "pci"; + assigned-addresses = <0x82000800 0 0x40000 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>; + 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 0x80000 0 0x2000>; + reg = <0x1000 0 0 0 0>; + #address-cells = <3>; + #size-cells = <2>; + #interrupt-cells = <1>; + ranges; + 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"; + }; + }; + internal-regs { system-controller@18200 { compatible = "marvell,armada-370-xp-system-controller"; @@ -176,54 +225,6 @@ 0x18304 0x4>; status = "okay"; }; - - pcie-controller { - compatible = "marvell,armada-370-pcie"; - status = "disabled"; - device_type = "pci"; - - #address-cells = <3>; - #size-cells = <2>; - - bus-range = <0x00 0xff>; - - ranges = <0x82000000 0 0x40000 0x40000 0 0x00002000 /* Port 0.0 registers */ - 0x82000000 0 0x80000 0x80000 0 0x00002000 /* Port 1.0 registers */ - 0x82000000 0 0xe0000000 0xe0000000 0 0x08000000 /* non-prefetchable memory */ - 0x81000000 0 0 0xe8000000 0 0x00100000>; /* downstream I/O */ - - pcie@1,0 { - device_type = "pci"; - assigned-addresses = <0x82000800 0 0x40000 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>; - 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 0x80000 0 0x2000>; - reg = <0x1000 0 0 0 0>; - #address-cells = <3>; - #size-cells = <2>; - #interrupt-cells = <1>; - ranges; - 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"; - }; - }; }; }; };
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.garcia@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(-)