| Message ID | 1387785725-24262-4-git-send-email-tinamdar@apm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Monday 23 December 2013, Tanmay Inamdar wrote: > This patch adds the bindings for X-Gene PCIe driver. The driver resides > under 'drivers/pci/host/pcie-xgene.c' file. > > Signed-off-by: Tanmay Inamdar <tinamdar@apm.com> > --- > .../devicetree/bindings/pci/xgene-pcie.txt | 43 ++++++++++++++++++++ > 1 file changed, 43 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pci/xgene-pcie.txt > > diff --git a/Documentation/devicetree/bindings/pci/xgene-pcie.txt b/Documentation/devicetree/bindings/pci/xgene-pcie.txt > new file mode 100644 > index 0000000..d92da4f > --- /dev/null > +++ b/Documentation/devicetree/bindings/pci/xgene-pcie.txt > @@ -0,0 +1,43 @@ > +* AppliedMicro X-Gene PCIe interface > + > +Required properties: > +- status: Either "ok" or "disabled". > +- device_type: set to "pci" > +- compatible: should contain "xgene,pcie" to identify the core. > +- reg: base addresses and lengths of the pcie controller configuration > + space register. > +- #address-cells: set to <3> > +- #size-cells: set to <2> > +- ranges: ranges for the PCI memory, I/O regions, config and MSI regions > +- #interrupt-cells: set to <1> > +- interrupt-map-mask and interrupt-map: standard PCI properties > + to define the mapping of the PCIe interface to interrupt > + numbers. > +- clocks: from common clock binding: handle to pci clock. > +- clock-names: from common clock binding. Should be "pcieclk". > + Better use an anonymous clock? The driver also seems to use a phy that is not defined here. > +Example: > + > +SoC specific DT Entry: > + pcie0: pcie@1f2b0000 { > + status = "disabled"; > + device_type = "pci"; > + compatible = "xgene,pcie"; > + #interrupt-cells = <1>; > + #size-cells = <2>; > + #address-cells = <3>; > + reg = < 0x00 0x1f2b0000 0x0 0x00010000>; > + ranges = <0x02000000 0x0 0x00000000 0xe0 0x00000000 0x0 0x10000000 /* mem*/ This is an awfully small memory space for a 64 bit machine. Is that a hardware bug you are working around? If not, please make it as large as you can to allow for arbitrary extension cards with large BARs, at least 4GB. Also, do you support no prefetchable memory? > + 0x01000000 0x0 0x80000000 0xe0 0x80000000 0x0 0x00010000 /* io */ I/O space at 0x80000000? That won't work with a lot of devices. Can you make it start at bus address 0? > + 0x00000000 0x0 0xd0000000 0xe0 0xd0000000 0x0 0x00200000 /* cfg */ config space is not normally in the ranges property, and I think you will need it in the pcie node itself as a 'reg' property so the code can access it. > + 0x00000000 0x0 0x79000000 0x00 0x79000000 0x0 0x00800000>; /* msi */ Same here. Also I suspect this is a separate irqchip that isn't actually part of the PCIe host. If they are separate devices, I'd strongly recommend modeling them as separate device-nodes in DT to make reuse easier. > + interrupt-map-mask = <0x0 0x0 0x0 0x7>; > + interrupt-map = <0x0 0x0 0x0 0x1 &gic 0x0 0xc2 0x1>; Only one IRQ for all devices? > + clocks = <&pcie0clk 0>; > + clock-names = "pcieclk" > + }; > + > +Board specific DT Entry: > + &pcie0 { > + status = "ok"; > + }; Arnd
On Fri, Jan 3, 2014 at 1:49 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Monday 23 December 2013, Tanmay Inamdar wrote: >> This patch adds the bindings for X-Gene PCIe driver. The driver resides >> under 'drivers/pci/host/pcie-xgene.c' file. >> >> Signed-off-by: Tanmay Inamdar <tinamdar@apm.com> >> --- >> .../devicetree/bindings/pci/xgene-pcie.txt | 43 ++++++++++++++++++++ >> 1 file changed, 43 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/pci/xgene-pcie.txt >> >> diff --git a/Documentation/devicetree/bindings/pci/xgene-pcie.txt b/Documentation/devicetree/bindings/pci/xgene-pcie.txt >> new file mode 100644 >> index 0000000..d92da4f >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pci/xgene-pcie.txt >> @@ -0,0 +1,43 @@ >> +* AppliedMicro X-Gene PCIe interface >> + >> +Required properties: >> +- status: Either "ok" or "disabled". >> +- device_type: set to "pci" >> +- compatible: should contain "xgene,pcie" to identify the core. >> +- reg: base addresses and lengths of the pcie controller configuration >> + space register. >> +- #address-cells: set to <3> >> +- #size-cells: set to <2> >> +- ranges: ranges for the PCI memory, I/O regions, config and MSI regions >> +- #interrupt-cells: set to <1> >> +- interrupt-map-mask and interrupt-map: standard PCI properties >> + to define the mapping of the PCIe interface to interrupt >> + numbers. >> +- clocks: from common clock binding: handle to pci clock. >> +- clock-names: from common clock binding. Should be "pcieclk". >> + > > Better use an anonymous clock? Sorry. Can you please elaborate? > > The driver also seems to use a phy that is not defined here. > >> +Example: >> + >> +SoC specific DT Entry: >> + pcie0: pcie@1f2b0000 { >> + status = "disabled"; >> + device_type = "pci"; >> + compatible = "xgene,pcie"; >> + #interrupt-cells = <1>; >> + #size-cells = <2>; >> + #address-cells = <3>; >> + reg = < 0x00 0x1f2b0000 0x0 0x00010000>; >> + ranges = <0x02000000 0x0 0x00000000 0xe0 0x00000000 0x0 0x10000000 /* mem*/ > > This is an awfully small memory space for a 64 bit machine. Is that a hardware bug you > are working around? If not, please make it as large as you can to allow for arbitrary > extension cards with large BARs, at least 4GB. HW does support a lot more than 256MB. I will change it. > > Also, do you support no prefetchable memory? HW has either IO or Memory regions for mapping device's memory space. There is no separate prefetchable memory space. > >> + 0x01000000 0x0 0x80000000 0xe0 0x80000000 0x0 0x00010000 /* io */ > > I/O space at 0x80000000? That won't work with a lot of devices. Can you make it start > at bus address 0? Ok. > >> + 0x00000000 0x0 0xd0000000 0xe0 0xd0000000 0x0 0x00200000 /* cfg */ > > config space is not normally in the ranges property, and I think you will need > it in the pcie node itself as a 'reg' property so the code can access it. pcie-designware.c does it that way. I just followed their implementation. > >> + 0x00000000 0x0 0x79000000 0x00 0x79000000 0x0 0x00800000>; /* msi */ > > Same here. Also I suspect this is a separate irqchip that isn't actually part > of the PCIe host. If they are separate devices, I'd strongly recommend modeling > them as separate device-nodes in DT to make reuse easier. You are right. However MSI irqchip needs PCIe core to map above mentioned space. I will remove it for now and will send it along with MSI patch. > >> + interrupt-map-mask = <0x0 0x0 0x0 0x7>; >> + interrupt-map = <0x0 0x0 0x0 0x1 &gic 0x0 0xc2 0x1>; > > Only one IRQ for all devices? The node represents a port. I believe that Linux framework uses only one of the legacy IRQs per port. Rest all remain unused. Hence I removed them. Please correct me if I am wrong. > >> + clocks = <&pcie0clk 0>; >> + clock-names = "pcieclk" >> + }; >> + >> +Board specific DT Entry: >> + &pcie0 { >> + status = "ok"; >> + }; > > Arnd
On Tuesday 07 January 2014, Tanmay Inamdar wrote: > On Fri, Jan 3, 2014 at 1:49 AM, Arnd Bergmann <arnd@arndb.de> wrote: > >> +Required properties: > >> +- status: Either "ok" or "disabled". > >> +- device_type: set to "pci" > >> +- compatible: should contain "xgene,pcie" to identify the core. > >> +- reg: base addresses and lengths of the pcie controller configuration > >> + space register. > >> +- #address-cells: set to <3> > >> +- #size-cells: set to <2> > >> +- ranges: ranges for the PCI memory, I/O regions, config and MSI regions > >> +- #interrupt-cells: set to <1> > >> +- interrupt-map-mask and interrupt-map: standard PCI properties > >> + to define the mapping of the PCIe interface to interrupt > >> + numbers. > >> +- clocks: from common clock binding: handle to pci clock. > >> +- clock-names: from common clock binding. Should be "pcieclk". > >> + > > > > Better use an anonymous clock? > > Sorry. Can you please elaborate? I mean drop the "clock-names" property. > >> +SoC specific DT Entry: > >> + pcie0: pcie@1f2b0000 { > >> + status = "disabled"; > >> + device_type = "pci"; > >> + compatible = "xgene,pcie"; > >> + #interrupt-cells = <1>; > >> + #size-cells = <2>; > >> + #address-cells = <3>; > >> + reg = < 0x00 0x1f2b0000 0x0 0x00010000>; > >> + ranges = <0x02000000 0x0 0x00000000 0xe0 0x00000000 0x0 0x10000000 /* mem*/ > > > > > > Also, do you support no prefetchable memory? > > HW has either IO or Memory regions for mapping device's memory space. > There is no separate prefetchable memory space. Are you sure the memory is non-prefetchable then? I would have expected 0x42000000 rather than 0x02000000, but I could be misremembering it. > > > >> + 0x00000000 0x0 0xd0000000 0xe0 0xd0000000 0x0 0x00200000 /* cfg */ > > > > config space is not normally in the ranges property, and I think you will need > > it in the pcie node itself as a 'reg' property so the code can access it. > > pcie-designware.c does it that way. I just followed their implementation. I don't remember what led to that, it still seems wrong and I can't find anything in the PCI binding for host bridges telling their config space this way. > >> + interrupt-map-mask = <0x0 0x0 0x0 0x7>; > >> + interrupt-map = <0x0 0x0 0x0 0x1 &gic 0x0 0xc2 0x1>; > > > > Only one IRQ for all devices? > > The node represents a port. I believe that Linux framework uses only > one of the legacy IRQs per port. Rest all remain unused. Hence I > removed them. Please correct me if I am wrong. Any PCI device can normally have four interrupts (IntA through IntD), which are traditionally separate pins on a PCI bus, but get emulated on PCIe. While it's not common for any normal device to use more than one IRQ, a bridge device will swizzle these (virtual) IRQ lines, so a device behind the bridge actually gets a different host IRQ. Arnd
On Tuesday 07 January 2014 16:35:01 Arnd Bergmann wrote: > > >> +SoC specific DT Entry: > > >> + pcie0: pcie@1f2b0000 { > > >> + status = "disabled"; > > >> + device_type = "pci"; > > >> + compatible = "xgene,pcie"; > > >> + #interrupt-cells = <1>; > > >> + #size-cells = <2>; > > >> + #address-cells = >; > > >> + reg = < 0x00 0x1f2b0000 0x0 0x00010000>; > > >> + ranges = <0x02000000 0x0 0x00000000 0xe0 0x00000000 0x0 0x10000000 /* mem*/ > > > > > > > > > Also, do you support no prefetchable memory? > > > > HW has either IO or Memory regions for mapping device's memory space. > > There is no separate prefetchable memory space. > > Are you sure the memory is non-prefetchable then? I would have expected > 0x42000000 rather than 0x02000000, but I could be misremembering it. Nevermind. I just checked and you are right: if you only have one memory range, it has to be non-prefetchable. Arnd
On Tue, Jan 07, 2014 at 04:35:01PM +0100, Arnd Bergmann wrote: > > >> + 0x00000000 0x0 0xd0000000 0xe0 0xd0000000 0x0 0x00200000 /* cfg */ > > > > > > config space is not normally in the ranges property, and I think you will need > > > it in the pcie node itself as a 'reg' property so the code can access it. > > > > pcie-designware.c does it that way. I just followed their implementation. > > I don't remember what led to that, it still seems wrong and I can't > find anything in the PCI binding for host bridges telling their > config space this way. When we discussed the mvebu PCI driver (which is, so far, the most throughly discussed PCI binding) it was concluded that the config space ranges like the above was OK only if it exactly described the standard ECAM layout. Idea being that standard/core code should be able to see that ranges, map the range and issue config accesses via the ECAM rules. > > >> + interrupt-map-mask = <0x0 0x0 0x0 0x7>; > > >> + interrupt-map = <0x0 0x0 0x0 0x1 &gic 0x0 0xc2 0x1>; > > > > > > Only one IRQ for all devices? > > > > The node represents a port. I believe that Linux framework uses only > > one of the legacy IRQs per port. Rest all remain unused. Hence I > > removed them. Please correct me if I am wrong. > > Any PCI device can normally have four interrupts (IntA through > IntD), which are traditionally separate pins on a PCI bus, but get > emulated on PCIe. While it's not common for any normal device to use > more than one IRQ, a bridge device will swizzle these (virtual) IRQ > lines, so a device behind the bridge actually gets a different host > IRQ. Agree, the binding should handle all four INTA,B,C,D assertions delivered to the port. If HW is able to decode the 4 ints into seperate Linux interrupt numbers then that should be described. If HW routes them all to a single number then interrupt-map-mask should be all 0. Arnd's point about swizzling effects the layout of the interrupt-map. When it is placed at the pcie-controller node level the map will incorporate one swizzle of the on-the-wire INTx messages. If the HW doesn't swizzle the INTx as the TLP passes through the bridge then it probably makes more sense to put the interrupt-map in the DT node of the bridge like mvebu does. Jason
On Tue, Jan 7, 2014 at 10:31 AM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: > On Tue, Jan 07, 2014 at 04:35:01PM +0100, Arnd Bergmann wrote: > >> > >> + 0x00000000 0x0 0xd0000000 0xe0 0xd0000000 0x0 0x00200000 /* cfg */ >> > > >> > > config space is not normally in the ranges property, and I think you will need >> > > it in the pcie node itself as a 'reg' property so the code can access it. >> > >> > pcie-designware.c does it that way. I just followed their implementation. >> >> I don't remember what led to that, it still seems wrong and I can't >> find anything in the PCI binding for host bridges telling their >> config space this way. > > When we discussed the mvebu PCI driver (which is, so far, the most > throughly discussed PCI binding) it was concluded that the config > space ranges like the above was OK only if it exactly described the > standard ECAM layout. > > Idea being that standard/core code should be able to see that ranges, > map the range and issue config accesses via the ECAM rules. Ok. Thanks. > >> > >> + interrupt-map-mask = <0x0 0x0 0x0 0x7>; >> > >> + interrupt-map = <0x0 0x0 0x0 0x1 &gic 0x0 0xc2 0x1>; >> > > >> > > Only one IRQ for all devices? >> > >> > The node represents a port. I believe that Linux framework uses only >> > one of the legacy IRQs per port. Rest all remain unused. Hence I >> > removed them. Please correct me if I am wrong. >> >> Any PCI device can normally have four interrupts (IntA through >> IntD), which are traditionally separate pins on a PCI bus, but get >> emulated on PCIe. While it's not common for any normal device to use >> more than one IRQ, a bridge device will swizzle these (virtual) IRQ >> lines, so a device behind the bridge actually gets a different host >> IRQ. > > Agree, the binding should handle all four INTA,B,C,D assertions > delivered to the port. > > If HW is able to decode the 4 ints into seperate Linux interrupt > numbers then that should be described. If HW routes them all to a > single number then interrupt-map-mask should be all 0. > > Arnd's point about swizzling effects the layout of the > interrupt-map. When it is placed at the pcie-controller node level the > map will incorporate one swizzle of the on-the-wire INTx messages. If > the HW doesn't swizzle the INTx as the TLP passes through the bridge > then it probably makes more sense to put the interrupt-map in the DT > node of the bridge like mvebu does. > Ok. > Jason
On Tue, Jan 7, 2014 at 7:35 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 07 January 2014, Tanmay Inamdar wrote: >> On Fri, Jan 3, 2014 at 1:49 AM, Arnd Bergmann <arnd@arndb.de> wrote: >> >> +Required properties: >> >> +- status: Either "ok" or "disabled". >> >> +- device_type: set to "pci" >> >> +- compatible: should contain "xgene,pcie" to identify the core. >> >> +- reg: base addresses and lengths of the pcie controller configuration >> >> + space register. >> >> +- #address-cells: set to <3> >> >> +- #size-cells: set to <2> >> >> +- ranges: ranges for the PCI memory, I/O regions, config and MSI regions >> >> +- #interrupt-cells: set to <1> >> >> +- interrupt-map-mask and interrupt-map: standard PCI properties >> >> + to define the mapping of the PCIe interface to interrupt >> >> + numbers. >> >> +- clocks: from common clock binding: handle to pci clock. >> >> +- clock-names: from common clock binding. Should be "pcieclk". >> >> + >> > >> > Better use an anonymous clock? >> >> Sorry. Can you please elaborate? > > I mean drop the "clock-names" property. > Did you mean clock-names in pcie-clock node or pcie node? I can drop clock-names from pcie clock node. However if I drop clock-names from pcie node, then clk_get call from pcie host driver would result in failure. Right? >> >> +SoC specific DT Entry: >> >> + pcie0: pcie@1f2b0000 { >> >> + status = "disabled"; >> >> + device_type = "pci"; >> >> + compatible = "xgene,pcie"; >> >> + #interrupt-cells = <1>; >> >> + #size-cells = <2>; >> >> + #address-cells = <3>; >> >> + reg = < 0x00 0x1f2b0000 0x0 0x00010000>; >> >> + ranges = <0x02000000 0x0 0x00000000 0xe0 0x00000000 0x0 0x10000000 /* mem*/ >> > >> > >> > Also, do you support no prefetchable memory? >> >> HW has either IO or Memory regions for mapping device's memory space. >> There is no separate prefetchable memory space. > > Are you sure the memory is non-prefetchable then? I would have expected > 0x42000000 rather than 0x02000000, but I could be misremembering it. > >> > >> >> + 0x00000000 0x0 0xd0000000 0xe0 0xd0000000 0x0 0x00200000 /* cfg */ >> > >> > config space is not normally in the ranges property, and I think you will need >> > it in the pcie node itself as a 'reg' property so the code can access it. >> >> pcie-designware.c does it that way. I just followed their implementation. > > I don't remember what led to that, it still seems wrong and I can't find anything > in the PCI binding for host bridges telling their config space this way. > >> >> + interrupt-map-mask = <0x0 0x0 0x0 0x7>; >> >> + interrupt-map = <0x0 0x0 0x0 0x1 &gic 0x0 0xc2 0x1>; >> > >> > Only one IRQ for all devices? >> >> The node represents a port. I believe that Linux framework uses only >> one of the legacy IRQs per port. Rest all remain unused. Hence I >> removed them. Please correct me if I am wrong. > > Any PCI device can normally have four interrupts (IntA through IntD), which are > traditionally separate pins on a PCI bus, but get emulated on PCIe. While it's > not common for any normal device to use more than one IRQ, a bridge device > will swizzle these (virtual) IRQ lines, so a device behind the bridge actually > gets a different host IRQ. > > Arnd
On Saturday 11 January 2014, Tanmay Inamdar wrote: > On Tue, Jan 7, 2014 at 7:35 AM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Tuesday 07 January 2014, Tanmay Inamdar wrote: > >> On Fri, Jan 3, 2014 at 1:49 AM, Arnd Bergmann <arnd@arndb.de> wrote: > >> > > >> > Better use an anonymous clock? > >> > >> Sorry. Can you please elaborate? > > > > I mean drop the "clock-names" property. > > > Did you mean clock-names in pcie-clock node or pcie node? I can drop > clock-names from pcie clock node. However if I drop clock-names from > pcie node, then clk_get call from pcie host driver would result in > failure. Right? I meant drop it from the pcie node, and change the clk_get call to pass NULL instead of the name, which will get the handle for the only clock provided. You only need clock-names if you have more than one clock in the device node and want to identify them. The pcie-clock node should not have a "clock-names" property at all, unless it has a "clocks" property as well and refers to its clock parent with it. I already noticed in another review that the xgene clocks get this part wrong and that should be fixed for all those clock provides, but it's unrelated to what I was talking about here. Arnd
diff --git a/Documentation/devicetree/bindings/pci/xgene-pcie.txt b/Documentation/devicetree/bindings/pci/xgene-pcie.txt new file mode 100644 index 0000000..d92da4f --- /dev/null +++ b/Documentation/devicetree/bindings/pci/xgene-pcie.txt @@ -0,0 +1,43 @@ +* AppliedMicro X-Gene PCIe interface + +Required properties: +- status: Either "ok" or "disabled". +- device_type: set to "pci" +- compatible: should contain "xgene,pcie" to identify the core. +- reg: base addresses and lengths of the pcie controller configuration + space register. +- #address-cells: set to <3> +- #size-cells: set to <2> +- ranges: ranges for the PCI memory, I/O regions, config and MSI regions +- #interrupt-cells: set to <1> +- interrupt-map-mask and interrupt-map: standard PCI properties + to define the mapping of the PCIe interface to interrupt + numbers. +- clocks: from common clock binding: handle to pci clock. +- clock-names: from common clock binding. Should be "pcieclk". + +Example: + +SoC specific DT Entry: + pcie0: pcie@1f2b0000 { + status = "disabled"; + device_type = "pci"; + compatible = "xgene,pcie"; + #interrupt-cells = <1>; + #size-cells = <2>; + #address-cells = <3>; + reg = < 0x00 0x1f2b0000 0x0 0x00010000>; + ranges = <0x02000000 0x0 0x00000000 0xe0 0x00000000 0x0 0x10000000 /* mem*/ + 0x01000000 0x0 0x80000000 0xe0 0x80000000 0x0 0x00010000 /* io */ + 0x00000000 0x0 0xd0000000 0xe0 0xd0000000 0x0 0x00200000 /* cfg */ + 0x00000000 0x0 0x79000000 0x00 0x79000000 0x0 0x00800000>; /* msi */ + interrupt-map-mask = <0x0 0x0 0x0 0x7>; + interrupt-map = <0x0 0x0 0x0 0x1 &gic 0x0 0xc2 0x1>; + clocks = <&pcie0clk 0>; + clock-names = "pcieclk" + }; + +Board specific DT Entry: + &pcie0 { + status = "ok"; + };
This patch adds the bindings for X-Gene PCIe driver. The driver resides under 'drivers/pci/host/pcie-xgene.c' file. Signed-off-by: Tanmay Inamdar <tinamdar@apm.com> --- .../devicetree/bindings/pci/xgene-pcie.txt | 43 ++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 Documentation/devicetree/bindings/pci/xgene-pcie.txt