| Message ID | 1364314719-1049-11-git-send-email-thomas.petazzoni@free-electrons.com |
|---|---|
| State | Not Applicable |
| Headers | show |
On Tue, Mar 26, 2013 at 05:18:32PM +0100, Thomas Petazzoni wrote: > + pcie-controller { > + compatible = "marvell,armada-370-pcie"; > + status = "disabled"; > + device_type = "pci"; > + > + #address-cells = <3>; > + #size-cells = <2>; > + > + bus-range = <0x00 0xff>; > + > + reg = <0xd0040000 0x2000>, <0xd0080000 0x2000>; > + > + reg-names = "pcie0.0", "pcie1.0"; > + > + ranges = <0x82000000 0 0xe0000000 0xe0000000 0 0x08000000 /* non-prefetchable memory */ > + 0x81000000 0 0 0xe8000000 0 0x00100000>; /* downstream I/O */ > + > + pcie@1,0 { > + device_type = "pci"; > + reg = <0x0800 0 0 0 0>; > + #address-cells = <3>; > + #size-cells = <2>; Very Minor Nit: These two # fields are not strictly necessary > + #interrupt-cells = <1>; > + 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"; > + reg = <0x1000 0 0 0 0>; > + #address-cells = <3>; > + #size-cells = <2>; > + #interrupt-cells = <1>; > + 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"; > + }; > + }; > }; > }; This basically looks fine to me, however, I think it is valuable if you and Thierry could use the same method to pass per-port registers. I expect others are going to reference these bindings for future work, and one standard method is more clear than two. Thierry: Did you settle on using assigned-addresses? Can you share the final binding for your driver? Jingoo Han's driver for Exynos uses lots of per-port registers, so I'm inclined to think that assigned-addresses is the clearer way forward. This is a fairly minor comment. Would people be comfortable going in as-is with a small follow-up revision to the DT? Regards, Jason -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dear Jason Gunthorpe, On Tue, 26 Mar 2013 10:34:21 -0600, Jason Gunthorpe wrote: > On Tue, Mar 26, 2013 at 05:18:32PM +0100, Thomas Petazzoni wrote: > > > + pcie-controller { > > + compatible = "marvell,armada-370-pcie"; > > + status = "disabled"; > > + device_type = "pci"; > > + > > + #address-cells = <3>; > > + #size-cells = <2>; > > + > > + bus-range = <0x00 0xff>; > > + > > + reg = <0xd0040000 0x2000>, <0xd0080000 0x2000>; > > + > > + reg-names = "pcie0.0", "pcie1.0"; > > + > > + ranges = <0x82000000 0 0xe0000000 0xe0000000 0 0x08000000 /* non-prefetchable memory */ > > + 0x81000000 0 0 0xe8000000 0 0x00100000>; /* downstream I/O */ > > + > > + pcie@1,0 { > > + device_type = "pci"; > > + reg = <0x0800 0 0 0 0>; > > + #address-cells = <3>; > > + #size-cells = <2>; > > Very Minor Nit: These two # fields are not strictly necessary Ok. I'll wait to see if there are more comments on this patch set, especially on deciding between reg = <...> or assigned-addresses, and I'll repost an updated version. > This basically looks fine to me, however, I think it is valuable if > you and Thierry could use the same method to pass per-port registers. I > expect others are going to reference these bindings for future work, > and one standard method is more clear than two. > > Thierry: Did you settle on using assigned-addresses? Can you share the > final binding for your driver? > > Jingoo Han's driver for Exynos uses lots of per-port registers, so I'm > inclined to think that assigned-addresses is the clearer way forward. > > This is a fairly minor comment. Would people be comfortable going in as-is > with a small follow-up revision to the DT? I'm fine reworking the driver in one way or another. I really don't care much about how the registers addresses are represented in the DT, as long as it gets the job done. So, let's wait for Thierry's opinion so we can align our DT bindings. Thanks, Thomas
On Tue, Mar 26, 2013 at 10:34:21AM -0600, Jason Gunthorpe wrote: [...] > This basically looks fine to me, however, I think it is valuable if > you and Thierry could use the same method to pass per-port registers. I > expect others are going to reference these bindings for future work, > and one standard method is more clear than two. > > Thierry: Did you settle on using assigned-addresses? Can you share the > final binding for your driver? Yes, I have the final bindings ready and I was waiting for Andrew's new version of the ranges parsing patch before sending the next (and hopefully final) version of the series. He posted that patch now so I should have something ready soon. For now here's what I currently use for DT: pcie-controller { compatible = "nvidia,tegra20-pcie"; device_type = "pci"; reg = <0x80003000 0x00000800 /* PADS registers */ 0x80003800 0x00000200 /* AFI registers */ 0x90000000 0x10000000>; /* configuration space */ reg-names = "pads", "afi", "cs"; interrupts = <0 98 0x04 /* controller interrupt */ 0 99 0x04>; /* MSI interrupt */ interrupt-names = "intr", "msi"; bus-range = <0x00 0xff>; #address-cells = <3>; #size-cells = <2>; ranges = <0x82000000 0 0x80000000 0x80000000 0 0x00001000 /* port 0 registers */ 0x82000000 0 0x80001000 0x80001000 0 0x00001000 /* port 1 registers */ 0x81000000 0 0 0x82000000 0 0x00010000 /* downstream I/O */ 0x82000000 0 0xa0000000 0xa0000000 0 0x10000000 /* non-prefetchable memory */ 0xc2000000 0 0xb0000000 0xb0000000 0 0x10000000>; /* prefetchable memory */ clocks = <&tegra_car 70>, <&tegra_car 72>, <&tegra_car 74>, <&tegra_car 118>; clock-names = "pex", "afi", "pcie_xclk", "pll_e"; status = "disabled"; pci@1,0 { device_type = "pci"; assigned-addresses = <0x82000800 0 0x80000000 0 0x1000>; reg = <0x000800 0 0 0 0>; status = "disabled"; #address-cells = <3>; #size-cells = <2>; nvidia,num-lanes = <2>; }; pci@2,0 { device_type = "pci"; assigned-addresses = <0x82001000 0 0x80001000 0 0x1000>; reg = <0x001000 0 0 0 0>; status = "disabled"; #address-cells = <3>; #size-cells = <2>; nvidia,num-lanes = <2>; }; }; I think that has everything that we discussed previously. Thierry
On Tuesday 26 March 2013, Thierry Reding wrote: > pci@1,0 { > device_type = "pci"; > assigned-addresses = <0x82000800 0 0x80000000 0 0x1000>; > reg = <0x000800 0 0 0 0>; > status = "disabled"; > > #address-cells = <3>; > #size-cells = <2>; > > nvidia,num-lanes = <2>; > }; Shouldn't there be an empty ranges property in there as well? Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 26, 2013 at 08:50:12PM +0000, Arnd Bergmann wrote: > On Tuesday 26 March 2013, Thierry Reding wrote: > > pci@1,0 { > > device_type = "pci"; > > assigned-addresses = <0x82000800 0 0x80000000 0 0x1000>; > > reg = <0x000800 0 0 0 0>; > > status = "disabled"; > > > > #address-cells = <3>; > > #size-cells = <2>; > > > > nvidia,num-lanes = <2>; > > }; > > Shouldn't there be an empty ranges property in there as well? Technically yes, I think. Though it doesn't make a bit of a difference on Linux. Thierry
On Tuesday 26 March 2013, Thierry Reding wrote: > On Tue, Mar 26, 2013 at 08:50:12PM +0000, Arnd Bergmann wrote: > > On Tuesday 26 March 2013, Thierry Reding wrote: > > > pci@1,0 { > > > device_type = "pci"; > > > assigned-addresses = <0x82000800 0 0x80000000 0 0x1000>; > > > reg = <0x000800 0 0 0 0>; > > > status = "disabled"; > > > > > > #address-cells = <3>; > > > #size-cells = <2>; > > > > > > nvidia,num-lanes = <2>; > > > }; > > > > Shouldn't there be an empty ranges property in there as well? > > Technically yes, I think. Though it doesn't make a bit of a difference > on Linux. Only because we have a workaround for old Power Macintoshs that we should remove or at least disable on non-PowerPC systems. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thierry, On Tue, 26 Mar 2013 21:16:54 +0100, Thierry Reding wrote: > > Thierry: Did you settle on using assigned-addresses? Can you share the > > final binding for your driver? > > Yes, I have the final bindings ready and I was waiting for Andrew's new > version of the ranges parsing patch before sending the next (and > hopefully final) version of the series. He posted that patch now so I > should have something ready soon. I will also rebase my patch set on top of Andrew's latest version of the of/pci ranges parsing patch. > For now here's what I currently use for DT: > > pcie-controller { > compatible = "nvidia,tegra20-pcie"; > device_type = "pci"; > reg = <0x80003000 0x00000800 /* PADS registers */ > 0x80003800 0x00000200 /* AFI registers */ > 0x90000000 0x10000000>; /* configuration space */ > reg-names = "pads", "afi", "cs"; > interrupts = <0 98 0x04 /* controller interrupt */ > 0 99 0x04>; /* MSI interrupt */ > interrupt-names = "intr", "msi"; > > bus-range = <0x00 0xff>; > #address-cells = <3>; > #size-cells = <2>; > > ranges = <0x82000000 0 0x80000000 0x80000000 0 0x00001000 /* port 0 registers */ > 0x82000000 0 0x80001000 0x80001000 0 0x00001000 /* port 1 registers */ > 0x81000000 0 0 0x82000000 0 0x00010000 /* downstream I/O */ > 0x82000000 0 0xa0000000 0xa0000000 0 0x10000000 /* non-prefetchable memory */ > 0xc2000000 0 0xb0000000 0xb0000000 0 0x10000000>; /* prefetchable memory */ > > clocks = <&tegra_car 70>, <&tegra_car 72>, <&tegra_car 74>, > <&tegra_car 118>; > clock-names = "pex", "afi", "pcie_xclk", "pll_e"; > status = "disabled"; > > pci@1,0 { > device_type = "pci"; > assigned-addresses = <0x82000800 0 0x80000000 0 0x1000>; > reg = <0x000800 0 0 0 0>; > status = "disabled"; > > #address-cells = <3>; > #size-cells = <2>; > > nvidia,num-lanes = <2>; > }; > > pci@2,0 { > device_type = "pci"; > assigned-addresses = <0x82001000 0 0x80001000 0 0x1000>; > reg = <0x001000 0 0 0 0>; > status = "disabled"; > > #address-cells = <3>; > #size-cells = <2>; > > nvidia,num-lanes = <2>; > }; > }; > > I think that has everything that we discussed previously. Hum, ok. I must admit I'm rather confused as to how this would map to the Marvell PCIe case. What we're discussing is how to encode the MMIO registers area that corresponds to each PCIe interface. For now, I've used a single "reg" property in the parent node, with one entry per PCIe interface: pcie-controller { compatible = "marvell,armada-xp-pcie"; status = "disabled"; device_type = "pci"; #address-cells = <3>; #size-cells = <2>; msi-parent = <&msi>; bus-range = <0x00 0xff>; reg = <0xd0040000 0x2000>, <0xd0042000 0x2000>, <0xd0044000 0x2000>, <0xd0048000 0x2000>, <0xd004C000 0x2000>, <0xd0080000 0x2000>, <0xd0082000 0x2000>, <0xd0084000 0x2000>, <0xd0088000 0x2000>, <0xd008C000 0x2000>; reg-names = "pcie0.0", "pcie2.0", "pcie0.1", "pcie0.2", "pcie0.3", "pcie1.0", "pcie3.0", "pcie1.1", "pcie1.2", "pcie1.3"; ranges = <0x82000000 0 0xe0000000 0xe0000000 0 0x08000000 /* non-prefetchable memory */ 0x81000000 0 0 0xe8000000 0 0x00100000>; /* downstream I/O */ pcie@1,0 { device_type = "pci"; reg = <0x0800 0 0 0 0>; #address-cells = <3>; #size-cells = <2>; #interrupt-cells = <1>; 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"; reg = <0x1000 0 0 0 0>; #address-cells = <3>; #size-cells = <2>; #interrupt-cells = <1>; interrupt-map-mask = <0 0 0 0>; interrupt-map = <0 0 0 0 &mpic 59>; marvell,pcie-port = <0>; marvell,pcie-lane = <1>; clocks = <&gateclk 6>; status = "disabled"; }; and so now the suggestions is to do: pcie-controller { compatible = "marvell,armada-xp-pcie"; status = "disabled"; device_type = "pci"; #address-cells = <3>; #size-cells = <2>; msi-parent = <&msi>; bus-range = <0x00 0xff>; ranges = <0x82000000 0 0xd0040000 0xd0040000 0 0x00002000 /* Port 0.0 registers */ 0x82000000 0 0xd0042000 0xd0042000 0 0x00002000 /* Port 2.0 registers */ 0x82000000 0 0xd0044000 0xd0044000 0 0x00002000 /* Port 0.1 registers */ 0x82000000 0 0xd0048000 0xd0048000 0 0x00002000 /* Port 0.2 registers */ 0x82000000 0 0xd004c000 0xd004c000 0 0x00002000 /* Port 0.3 registers */ 0x82000000 0 0xd0080000 0xd0080000 0 0x00002000 /* Port 1.0 registers */ 0x82000000 0 0xd0082000 0xd0082000 0 0x00002000 /* Port 3.0 registers */ 0x82000000 0 0xd0084000 0xd0084000 0 0x00002000 /* Port 1.1 registers */ 0x82000000 0 0xd0088000 0xd0088000 0 0x00002000 /* Port 1.2 registers */ 0x82000000 0 0xd008c000 0xd008c000 0 0x00002000 /* Port 1.3 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 0xd0040000 0 0x2000>; reg = <0x0800 0 0 0 0>; #address-cells = <3>; #size-cells = <2>; #interrupt-cells = <1>; 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 = <0x82001000 0 0xd0042000 0 0x2000>; reg = <0x1000 0 0 0 0>; #address-cells = <3>; #size-cells = <2>; #interrupt-cells = <1>; interrupt-map-mask = <0 0 0 0>; interrupt-map = <0 0 0 0 &mpic 59>; marvell,pcie-port = <0>; marvell,pcie-lane = <1>; clocks = <&gateclk 6>; status = "disabled"; }; [...] }; Is this correct? Thierry, Jason, if you could confirm my understanding, that would be great. I could then rework and resend the patch set. Thanks! Thomas
On Tue, Mar 26, 2013 at 10:27:44PM +0100, Thomas Petazzoni wrote: > Is this correct? Thierry, Jason, if you could confirm my understanding, > that would be great. I could then rework and resend the patch set. It looked to me the same as what Thierry was doing, and Thierry's looked like it included everything from the big discussion on that subject. Seems good to go to me. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 26, 2013 at 10:27:44PM +0100, Thomas Petazzoni wrote: [...] > and so now the suggestions is to do: > > pcie-controller { > compatible = "marvell,armada-xp-pcie"; > status = "disabled"; > device_type = "pci"; > > #address-cells = <3>; > #size-cells = <2>; > > msi-parent = <&msi>; > bus-range = <0x00 0xff>; > > ranges = <0x82000000 0 0xd0040000 0xd0040000 0 0x00002000 /* Port 0.0 registers */ > 0x82000000 0 0xd0042000 0xd0042000 0 0x00002000 /* Port 2.0 registers */ > 0x82000000 0 0xd0044000 0xd0044000 0 0x00002000 /* Port 0.1 registers */ > 0x82000000 0 0xd0048000 0xd0048000 0 0x00002000 /* Port 0.2 registers */ > 0x82000000 0 0xd004c000 0xd004c000 0 0x00002000 /* Port 0.3 registers */ > 0x82000000 0 0xd0080000 0xd0080000 0 0x00002000 /* Port 1.0 registers */ > 0x82000000 0 0xd0082000 0xd0082000 0 0x00002000 /* Port 3.0 registers */ > 0x82000000 0 0xd0084000 0xd0084000 0 0x00002000 /* Port 1.1 registers */ > 0x82000000 0 0xd0088000 0xd0088000 0 0x00002000 /* Port 1.2 registers */ > 0x82000000 0 0xd008c000 0xd008c000 0 0x00002000 /* Port 1.3 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 0xd0040000 0 0x2000>; > reg = <0x0800 0 0 0 0>; > #address-cells = <3>; > #size-cells = <2>; > #interrupt-cells = <1>; > 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 = <0x82001000 0 0xd0042000 0 0x2000>; > reg = <0x1000 0 0 0 0>; > #address-cells = <3>; > #size-cells = <2>; > #interrupt-cells = <1>; > interrupt-map-mask = <0 0 0 0>; > interrupt-map = <0 0 0 0 &mpic 59>; > marvell,pcie-port = <0>; > marvell,pcie-lane = <1>; > clocks = <&gateclk 6>; > status = "disabled"; > }; > > [...] > > }; > > Is this correct? Thierry, Jason, if you could confirm my understanding, > that would be great. I could then rework and resend the patch set. Yes, that looks correct. And as Arnd mentioned the pci@x,y nodes should probably have an empty ranges property. Thierry
diff --git a/arch/arm/boot/dts/armada-370.dtsi b/arch/arm/boot/dts/armada-370.dtsi index 8188d13..b0468fd 100644 --- a/arch/arm/boot/dts/armada-370.dtsi +++ b/arch/arm/boot/dts/armada-370.dtsi @@ -153,5 +153,50 @@ clocks = <&coreclk 0>; }; + pcie-controller { + compatible = "marvell,armada-370-pcie"; + status = "disabled"; + device_type = "pci"; + + #address-cells = <3>; + #size-cells = <2>; + + bus-range = <0x00 0xff>; + + reg = <0xd0040000 0x2000>, <0xd0080000 0x2000>; + + reg-names = "pcie0.0", "pcie1.0"; + + ranges = <0x82000000 0 0xe0000000 0xe0000000 0 0x08000000 /* non-prefetchable memory */ + 0x81000000 0 0 0xe8000000 0 0x00100000>; /* downstream I/O */ + + pcie@1,0 { + device_type = "pci"; + reg = <0x0800 0 0 0 0>; + #address-cells = <3>; + #size-cells = <2>; + #interrupt-cells = <1>; + 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"; + reg = <0x1000 0 0 0 0>; + #address-cells = <3>; + #size-cells = <2>; + #interrupt-cells = <1>; + 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"; + }; + }; }; };
The Armada 370 SoC has two 1x PCIe 2.0 interfaces, so we add the necessary Device Tree informations to make these interfaces availabel. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- arch/arm/boot/dts/armada-370.dtsi | 45 +++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+)