| Message ID | 1395270762-6055-3-git-send-email-tinamdar@apm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 19, 2014 at 6:12 PM, Tanmay Inamdar <tinamdar@apm.com> wrote: > This patch adds the device tree nodes for APM X-Gene PCIe controller and > PCIe clock interface. Since X-Gene SOC supports maximum 5 ports, 5 dts > nodes are added. [snip] > + pcie0: pcie@1f2b0000 { > + status = "disabled"; > + device_type = "pci"; > + compatible = "apm,xgene-storm-pcie", "apm,xgene-pcie"; > + #interrupt-cells = <1>; > + #size-cells = <2>; > + #address-cells = <3>; > + reg = < 0x00 0x1f2b0000 0x0 0x00010000 /* Controller registers */ > + 0xe0 0xd0000000 0x0 0x00200000>; /* PCI config space */ Where is the right place for config space? This binding has it here and others have it in ranges. Given that config space type is defined for ranges, I would think that is the right place. But Liviu's patches do not process config space entries in ranges. Perhaps we need a config space resource populated in the bridge struct. Rob > + reg-names = "csr", "cfg"; > + ranges = <0x01000000 0x00 0x00000000 0xe0 0x00000000 0x00 0x00010000 /* io */ > + 0x02000000 0x00 0x10000000 0xe0 0x10000000 0x00 0x80000000>; /* mem */
On Wed, Mar 26, 2014 at 02:28:42PM +0000, Rob Herring wrote: > On Wed, Mar 19, 2014 at 6:12 PM, Tanmay Inamdar <tinamdar@apm.com> wrote: > > This patch adds the device tree nodes for APM X-Gene PCIe controller and > > PCIe clock interface. Since X-Gene SOC supports maximum 5 ports, 5 dts > > nodes are added. > > [snip] > > > + pcie0: pcie@1f2b0000 { > > + status = "disabled"; > > + device_type = "pci"; > > + compatible = "apm,xgene-storm-pcie", "apm,xgene-pcie"; > > + #interrupt-cells = <1>; > > + #size-cells = <2>; > > + #address-cells = <3>; > > + reg = < 0x00 0x1f2b0000 0x0 0x00010000 /* Controller registers */ > > + 0xe0 0xd0000000 0x0 0x00200000>; /* PCI config space */ > > Where is the right place for config space? This binding has it here > and others have it in ranges. Given that config space type is defined > for ranges, I would think that is the right place. But Liviu's patches > do not process config space entries in ranges. Perhaps we need a > config space resource populated in the bridge struct. Rob, Have a look at the discussion between Will and Arnd on the subject of virtual PCI host controller for arm 32bit. My understanding is that config space is described via reg entries. See here: http://archive.arm.linux.org.uk/lurker/message/20140205.190947.b3c3e464.en.html Best regards, Liviu > > Rob > > > > + reg-names = "csr", "cfg"; > > + ranges = <0x01000000 0x00 0x00000000 0xe0 0x00000000 0x00 0x00010000 /* io */ > > + 0x02000000 0x00 0x10000000 0xe0 0x10000000 0x00 0x80000000>; /* mem */ >
On Wed, Mar 26, 2014 at 09:28:42AM -0500, Rob Herring wrote: > Where is the right place for config space? This binding has it here > and others have it in ranges. I think all the drivers in drivers/pci/host use 'reg', this was discussed in the dt-bindings list and AFAIK no new drivers have used ranges since it was brought up. > Given that config space type is defined for ranges, I would think > that is the right place. But Liviu's patches do not process config > space entries in ranges. Perhaps we need a config space resource > populated in the bridge struct. When we talked about this earlier on the DT bindings list the consensus seemed to be that configuration MMIO ranges should only be used if the underlying memory was exactly ECAM, and was not to be used for random configuration related register blocks. The rational being that generic code, upon seeing that ranges entry, could just go ahead and assume ECAM mapping. Jason
On Wed, Mar 26, 2014 at 09:28:42AM -0500, Rob Herring wrote: > On Wed, Mar 19, 2014 at 6:12 PM, Tanmay Inamdar <tinamdar@apm.com> wrote: > > This patch adds the device tree nodes for APM X-Gene PCIe controller and > > PCIe clock interface. Since X-Gene SOC supports maximum 5 ports, 5 dts > > nodes are added. > > [snip] > > > + pcie0: pcie@1f2b0000 { > > + status = "disabled"; > > + device_type = "pci"; > > + compatible = "apm,xgene-storm-pcie", "apm,xgene-pcie"; > > + #interrupt-cells = <1>; > > + #size-cells = <2>; > > + #address-cells = <3>; > > + reg = < 0x00 0x1f2b0000 0x0 0x00010000 /* Controller registers */ > > + 0xe0 0xd0000000 0x0 0x00200000>; /* PCI config space */ > Resurecting an old thread as this is relevant to what I'm doing at the moment: > Where is the right place for config space? This binding has it here > and others have it in ranges. Given that config space type is defined > for ranges, I would think that is the right place. But Liviu's patches > do not process config space entries in ranges. Perhaps we need a > config space resource populated in the bridge struct. I have found out that we cannot pasd the config ranges from the DT into the pci_host_bridge structure as the PCI framework doesn't have a resource type for config resources. Leaving the translation between range flags and resource type as is (filtered through the IORESOURCE_TYPE_BITS) will lead to a resource type of value zero, which is not recognised by any resource handling API so bridge configuration and bus scanning will barf. I'm looking for suggestions here, as Jason Gunthorpe suggested that we should be able to parse config ranges if they conform to the ECAM part of the PCI standard. Best regards, Liviu > > Rob > > > > + reg-names = "csr", "cfg"; > > + ranges = <0x01000000 0x00 0x00000000 0xe0 0x00000000 0x00 0x00010000 /* io */ > > + 0x02000000 0x00 0x10000000 0xe0 0x10000000 0x00 0x80000000>; /* mem */ > -- > 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 Wed, Apr 16, 2014 at 06:05:45PM +0100, Liviu Dudau wrote: > I have found out that we cannot pasd the config ranges from the DT into the > pci_host_bridge structure as the PCI framework doesn't have a resource type > for config resources. Leaving the translation between range flags and > resource type as is (filtered through the IORESOURCE_TYPE_BITS) will lead > to a resource type of value zero, which is not recognised by any resource > handling API so bridge configuration and bus scanning will barf. > > I'm looking for suggestions here, as Jason Gunthorpe suggested that we > should be able to parse config ranges if they conform to the ECAM part > of the PCI standard. The thinking here is the ranges should be well defined and general, it isn't a dumping ground for driver specific stuff. No spec says you can put config space into the ranges at all, nobody should be doing that today, obviously some cases were missed during review.. The comment about ECAM was intended as a general guidance on what config space in ranges could/should be used for. Right now config space shouldn't propagate out side any driver, so you can probably just filter it in your generic code, and make it very hard and obviously wrong for a driver to parse ranges for config space, so we don't get more usages. Jason
On Wed, Apr 16, 2014 at 03:21:04PM -0600, Jason Gunthorpe wrote: > On Wed, Apr 16, 2014 at 06:05:45PM +0100, Liviu Dudau wrote: > > > I have found out that we cannot pasd the config ranges from the DT into the > > pci_host_bridge structure as the PCI framework doesn't have a resource type > > for config resources. Leaving the translation between range flags and > > resource type as is (filtered through the IORESOURCE_TYPE_BITS) will lead > > to a resource type of value zero, which is not recognised by any resource > > handling API so bridge configuration and bus scanning will barf. > > > > I'm looking for suggestions here, as Jason Gunthorpe suggested that we > > should be able to parse config ranges if they conform to the ECAM part > > of the PCI standard. > > The thinking here is the ranges should be well defined and general, it > isn't a dumping ground for driver specific stuff. > > No spec says you can put config space into the ranges at all, nobody > should be doing that today, obviously some cases were missed during > review.. ePAPR documents allows that when ss == 00. > > The comment about ECAM was intended as a general guidance on what > config space in ranges could/should be used for. > > Right now config space shouldn't propagate out side any driver, so you > can probably just filter it in your generic code, and make it very hard > and obviously wrong for a driver to parse ranges for config space, so > we don't get more usages. OK, this goes slightly against your email from 26th March: "When we talked about this earlier on the DT bindings list the consensus seemed to be that configuration MMIO ranges should only be used if the underlying memory was exactly ECAM, and was not to be used for random configuration related register blocks. The rational being that generic code, upon seeing that ranges entry, could just go ahead and assume ECAM mapping." What I'm saying is that the only code that will see this ranges entry will be the parsing code as if we try to create a resource out of the range and add it to the host bridge structure (not driver) we will confuse the rest of the pci_host_bridge API. So we cannot do any ECAM accesses (yet?). Best regards, Liviu > > 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 Thu, Apr 17, 2014 at 01:20:42AM +0100, Liviu Dudau wrote: > > No spec says you can put config space into the ranges at all, nobody > > should be doing that today, obviously some cases were missed during > > review.. > > ePAPR documents allows that when ss == 00. Which do you mean? The 'PCI Bus Binding' spec has fairly specific language on how ranges should be used and interpreted, and it precludes doing anything meaningful with config space (like requiring b,d,f and r to be zeroed when doing compares against ranges, requiring the ranges to represent the bridge windows, etc). There is certainly room to invent something (like ECAM mapping) but nothing is specified in that document. The ePAPR document I have doesn't talk about PCI.. If you've found a document that defines how it works then that changes things.. ;) > > The comment about ECAM was intended as a general guidance on what > > config space in ranges could/should be used for. > > > > Right now config space shouldn't propagate out side any driver, so you > > can probably just filter it in your generic code, and make it very hard > > and obviously wrong for a driver to parse ranges for config space, so > > we don't get more usages. > > OK, this goes slightly against your email from 26th March: > > "When we talked about this earlier on the DT bindings list the > consensus seemed to be that configuration MMIO ranges should only be > used if the underlying memory was exactly ECAM, and was not to be used > for random configuration related register blocks. > > The rational being that generic code, upon seeing that ranges entry, > could just go ahead and assume ECAM mapping." > > What I'm saying is that the only code that will see this ranges entry will > be the parsing code as if we try to create a resource out of the range > and add it to the host bridge structure (not driver) we will confuse the > rest of the pci_host_bridge API. So we cannot do any ECAM accesses (yet?). Sorry if this seems unclear, what you quoted was from a specification standpoint - someday defining config space ranges to be the ECAM window makes the most sense. This is from the direction of precluding drivers from using it for random purposes. From a Linux standpoint, there is simply no infrastructure for generic config access outside the driver, so config space must remain contained in the driver, and shouldn't leak into the host bridge or other core structures. I think the shared code you are working on should simply ignore config ss ranges entirely, they have no defined meaning.. Regards, Jason
On Thu, Apr 17, 2014 at 02:24:34AM +0100, Jason Gunthorpe wrote: > On Thu, Apr 17, 2014 at 01:20:42AM +0100, Liviu Dudau wrote: > > > > No spec says you can put config space into the ranges at all, nobody > > > should be doing that today, obviously some cases were missed during > > > review.. > > > > ePAPR documents allows that when ss == 00. > > Which do you mean? The 'PCI Bus Binding' spec has fairly specific > language on how ranges should be used and interpreted, and it > precludes doing anything meaningful with config space (like requiring > b,d,f and r to be zeroed when doing compares against ranges, requiring > the ranges to represent the bridge windows, etc). > > There is certainly room to invent something (like ECAM mapping) but > nothing is specified in that document. On more carefull reading of the Power_ePAPR_APPROVED_v1.0.pdf document that I have I agree, there is no meaningful way of describing one's config ranges. > > The ePAPR document I have doesn't talk about PCI.. > > If you've found a document that defines how it works then that changes > things.. ;) > > > > The comment about ECAM was intended as a general guidance on what > > > config space in ranges could/should be used for. > > > > > > Right now config space shouldn't propagate out side any driver, so you > > > can probably just filter it in your generic code, and make it very hard > > > and obviously wrong for a driver to parse ranges for config space, so > > > we don't get more usages. > > > > OK, this goes slightly against your email from 26th March: > > > > "When we talked about this earlier on the DT bindings list the > > consensus seemed to be that configuration MMIO ranges should only be > > used if the underlying memory was exactly ECAM, and was not to be used > > for random configuration related register blocks. > > > > The rational being that generic code, upon seeing that ranges entry, > > could just go ahead and assume ECAM mapping." > > > > What I'm saying is that the only code that will see this ranges entry will > > be the parsing code as if we try to create a resource out of the range > > and add it to the host bridge structure (not driver) we will confuse the > > rest of the pci_host_bridge API. So we cannot do any ECAM accesses (yet?). > > Sorry if this seems unclear, what you quoted was from a specification > standpoint - someday defining config space ranges to be the ECAM > window makes the most sense. This is from the direction of precluding > drivers from using it for random purposes. > > From a Linux standpoint, there is simply no infrastructure for generic > config access outside the driver, so config space must remain > contained in the driver, and shouldn't leak into the host bridge or > other core structures. > > I think the shared code you are working on should simply ignore config > ss ranges entirely, they have no defined meaning.. Agree. Less things to code for is always better! Best regards, Liviu > > Regards, > Jason >
diff --git a/arch/arm64/boot/dts/apm-mustang.dts b/arch/arm64/boot/dts/apm-mustang.dts index 1247ca1..507b6c9 100644 --- a/arch/arm64/boot/dts/apm-mustang.dts +++ b/arch/arm64/boot/dts/apm-mustang.dts @@ -24,3 +24,11 @@ reg = < 0x1 0x00000000 0x0 0x80000000 >; /* Updated by bootloader */ }; }; + +&pcie0clk { + status = "ok"; +}; + +&pcie0 { + status = "ok"; +}; diff --git a/arch/arm64/boot/dts/apm-storm.dtsi b/arch/arm64/boot/dts/apm-storm.dtsi index d37d736..6011d25 100644 --- a/arch/arm64/boot/dts/apm-storm.dtsi +++ b/arch/arm64/boot/dts/apm-storm.dtsi @@ -176,6 +176,161 @@ reg-names = "csr-reg"; clock-output-names = "eth8clk"; }; + + pcie0clk: pcie0clk@1f2bc000 { + status = "disabled"; + compatible = "apm,xgene-device-clock"; + #clock-cells = <1>; + clocks = <&socplldiv2 0>; + reg = <0x0 0x1f2bc000 0x0 0x1000>; + reg-names = "csr-reg"; + clock-output-names = "pcie0clk"; + }; + + pcie1clk: pcie1clk@1f2cc000 { + status = "disabled"; + compatible = "apm,xgene-device-clock"; + #clock-cells = <1>; + clocks = <&socplldiv2 0>; + reg = <0x0 0x1f2cc000 0x0 0x1000>; + reg-names = "csr-reg"; + clock-output-names = "pcie1clk"; + }; + + pcie2clk: pcie2clk@1f2dc000 { + status = "disabled"; + compatible = "apm,xgene-device-clock"; + #clock-cells = <1>; + clocks = <&socplldiv2 0>; + reg = <0x0 0x1f2dc000 0x0 0x1000>; + reg-names = "csr-reg"; + clock-output-names = "pcie2clk"; + }; + + pcie3clk: pcie3clk@1f50c000 { + status = "disabled"; + compatible = "apm,xgene-device-clock"; + #clock-cells = <1>; + clocks = <&socplldiv2 0>; + reg = <0x0 0x1f50c000 0x0 0x1000>; + reg-names = "csr-reg"; + clock-output-names = "pcie3clk"; + }; + + pcie4clk: pcie4clk@1f51c000 { + status = "disabled"; + compatible = "apm,xgene-device-clock"; + #clock-cells = <1>; + clocks = <&socplldiv2 0>; + reg = <0x0 0x1f51c000 0x0 0x1000>; + reg-names = "csr-reg"; + clock-output-names = "pcie4clk"; + }; + }; + + pcie0: pcie@1f2b0000 { + status = "disabled"; + device_type = "pci"; + compatible = "apm,xgene-storm-pcie", "apm,xgene-pcie"; + #interrupt-cells = <1>; + #size-cells = <2>; + #address-cells = <3>; + reg = < 0x00 0x1f2b0000 0x0 0x00010000 /* Controller registers */ + 0xe0 0xd0000000 0x0 0x00200000>; /* PCI config space */ + reg-names = "csr", "cfg"; + ranges = <0x01000000 0x00 0x00000000 0xe0 0x00000000 0x00 0x00010000 /* io */ + 0x02000000 0x00 0x10000000 0xe0 0x10000000 0x00 0x80000000>; /* mem */ + dma-ranges = <0x42000000 0x40 0x00000000 0x40 0x00000000 0x40 0x00000000>; + interrupt-map-mask = <0x0 0x0 0x0 0x7>; + interrupt-map = <0x0 0x0 0x0 0x1 &gic 0x0 0xc2 0x1 + 0x0 0x0 0x0 0x2 &gic 0x0 0xc3 0x1 + 0x0 0x0 0x0 0x3 &gic 0x0 0xc4 0x1 + 0x0 0x0 0x0 0x4 &gic 0x0 0xc5 0x1>; + clocks = <&pcie0clk 0>; + }; + + pcie1: pcie@1f2c0000 { + status = "disabled"; + device_type = "pci"; + compatible = "apm,xgene-storm-pcie", "apm,xgene-pcie"; + #interrupt-cells = <1>; + #size-cells = <2>; + #address-cells = <3>; + reg = < 0x00 0x1f2c0000 0x0 0x00010000 /* Controller registers */ + 0xd0 0xd0000000 0x0 0x00200000>; /* PCI config space */ + reg-names = "csr", "cfg"; + ranges = <0x01000000 0x0 0x00000000 0xd0 0x00000000 0x00 0x00010000 /* io */ + 0x02000000 0x0 0x10000000 0xd0 0x10000000 0x00 0x80000000>; /* mem */ + dma-ranges = <0x42000000 0x40 0x00000000 0x40 0x00000000 0x40 0x00000000>; + interrupt-map-mask = <0x0 0x0 0x0 0x7>; + interrupt-map = <0x0 0x0 0x0 0x1 &gic 0x0 0xc8 0x1 + 0x0 0x0 0x0 0x2 &gic 0x0 0xc9 0x1 + 0x0 0x0 0x0 0x3 &gic 0x0 0xca 0x1 + 0x0 0x0 0x0 0x4 &gic 0x0 0xcb 0x1>; + clocks = <&pcie1clk 0>; + }; + + pcie2: pcie@1f2d0000 { + status = "disabled"; + device_type = "pci"; + compatible = "apm,xgene-storm-pcie", "apm,xgene-pcie"; + #interrupt-cells = <1>; + #size-cells = <2>; + #address-cells = <3>; + reg = < 0x00 0x1f2d0000 0x0 0x00010000 /* Controller registers */ + 0x90 0xd0000000 0x0 0x00200000>; /* PCI config space */ + reg-names = "csr", "cfg"; + ranges = <0x01000000 0x0 0x00000000 0x90 0x00000000 0x0 0x00010000 /* io */ + 0x02000000 0x0 0x10000000 0x90 0x10000000 0x0 0x80000000>; /* mem */ + dma-ranges = <0x42000000 0x40 0x00000000 0x40 0x00000000 0x40 0x00000000>; + interrupt-map-mask = <0x0 0x0 0x0 0x7>; + interrupt-map = <0x0 0x0 0x0 0x1 &gic 0x0 0xce 0x1 + 0x0 0x0 0x0 0x2 &gic 0x0 0xcf 0x1 + 0x0 0x0 0x0 0x3 &gic 0x0 0xd0 0x1 + 0x0 0x0 0x0 0x4 &gic 0x0 0xd1 0x1>; + clocks = <&pcie2clk 0>; + }; + + pcie3: pcie@1f500000 { + status = "disabled"; + device_type = "pci"; + compatible = "apm,xgene-storm-pcie", "apm,xgene-pcie"; + #interrupt-cells = <1>; + #size-cells = <2>; + #address-cells = <3>; + reg = < 0x00 0x1f500000 0x0 0x00010000 /* Controller registers */ + 0xa0 0xd0000000 0x0 0x00200000>; /* PCI config space */ + reg-names = "csr", "cfg"; + ranges = <0x01000000 0x0 0x00000000 0xa0 0x00000000 0x0 0x00010000 /* io */ + 0x02000000 0x0 0x10000000 0xa0 0x10000000 0x0 0x80000000>; /* mem */ + dma-ranges = <0x42000000 0x40 0x00000000 0x40 0x00000000 0x40 0x00000000>; + interrupt-map-mask = <0x0 0x0 0x0 0x7>; + interrupt-map = <0x0 0x0 0x0 0x1 &gic 0x0 0xd4 0x1 + 0x0 0x0 0x0 0x2 &gic 0x0 0xd5 0x1 + 0x0 0x0 0x0 0x3 &gic 0x0 0xd6 0x1 + 0x0 0x0 0x0 0x4 &gic 0x0 0xd7 0x1>; + clocks = <&pcie3clk 0>; + }; + + pcie4: pcie@1f510000 { + status = "disabled"; + device_type = "pci"; + compatible = "apm,xgene-storm-pcie", "apm,xgene-pcie"; + #interrupt-cells = <1>; + #size-cells = <2>; + #address-cells = <3>; + reg = < 0x00 0x1f510000 0x0 0x00010000 /* Controller registers */ + 0xc0 0xd0000000 0x0 0x00200000>; /* PCI config space */ + reg-names = "csr", "cfg"; + ranges = <0x01000000 0x0 0x00000000 0xc0 0x00000000 0x0 0x00010000 /* io */ + 0x02000000 0x0 0x10000000 0xc0 0x10000000 0x0 0x80000000>; /* mem */ + dma-ranges = <0x42000000 0x40 0x00000000 0x40 0x00000000 0x40 0x00000000>; + interrupt-map-mask = <0x0 0x0 0x0 0x7>; + interrupt-map = <0x0 0x0 0x0 0x1 &gic 0x0 0xda 0x1 + 0x0 0x0 0x0 0x2 &gic 0x0 0xdb 0x1 + 0x0 0x0 0x0 0x3 &gic 0x0 0xdc 0x1 + 0x0 0x0 0x0 0x4 &gic 0x0 0xdd 0x1>; + clocks = <&pcie4clk 0>; }; serial0: serial@1c020000 {
This patch adds the device tree nodes for APM X-Gene PCIe controller and PCIe clock interface. Since X-Gene SOC supports maximum 5 ports, 5 dts nodes are added. Signed-off-by: Tanmay Inamdar <tinamdar@apm.com> --- arch/arm64/boot/dts/apm-mustang.dts | 8 ++ arch/arm64/boot/dts/apm-storm.dtsi | 155 +++++++++++++++++++++++++++++++++++ 2 files changed, 163 insertions(+)