| Message ID | 1411573068-12952-4-git-send-email-rric@kernel.org |
|---|---|
| State | Changes Requested |
| Headers | show |
On Wednesday 24 September 2014 17:37:45 Robert Richter wrote: > > + pcie0@0x8480,00000000 { The name should be pci, not pci0. > + compatible = "cavium,thunder-pcie"; > + device_type = "pci"; > + msi-parent = <&its>; > + bus-range = <0 255>; > + #size-cells = <2>; > + #address-cells = <3>; > + reg = <0x8480 0x00000000 0 0x10000000>; /* Configuration space */ > + ranges = <0x03000000 0x8010 0x00000000 0x8010 0x00000000 0x70 0x00000000>, /* mem ranges */ > + <0x03000000 0x8300 0x00000000 0x8300 0x00000000 0x80 0x00000000>, > + <0x03000000 0x87e0 0x00000000 0x87e0 0x00000000 0x01 0x00000000>; > + }; If you claim the entire 0-255 bus range, I think you should also specify a domain, otherwise it's not predictable which domain you get. The interrupt-map and interrupt-map-mask properties are required for PCI, otherwise you can't do LSI interrupts. If your hardware can support it, you should also list I/O space and prefetchable memory spaces. Can you explain why you have multiple non-prefetchable ranges? 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 Wed, Sep 24, 2014 at 9:36 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Wednesday 24 September 2014 17:37:45 Robert Richter wrote: >> >> + pcie0@0x8480,00000000 { > > The name should be pci, not pci0. Thanks, will change. > >> + compatible = "cavium,thunder-pcie"; >> + device_type = "pci"; >> + msi-parent = <&its>; >> + bus-range = <0 255>; >> + #size-cells = <2>; >> + #address-cells = <3>; >> + reg = <0x8480 0x00000000 0 0x10000000>; /* Configuration space */ >> + ranges = <0x03000000 0x8010 0x00000000 0x8010 0x00000000 0x70 0x00000000>, /* mem ranges */ >> + <0x03000000 0x8300 0x00000000 0x8300 0x00000000 0x80 0x00000000>, >> + <0x03000000 0x87e0 0x00000000 0x87e0 0x00000000 0x01 0x00000000>; >> + }; > > If you claim the entire 0-255 bus range, I think you should also > specify a domain, otherwise it's not predictable which domain you > get. > > The interrupt-map and interrupt-map-mask properties are required for PCI, > otherwise you can't do LSI interrupts. This PCI controller supports only MSIx interrupts which are edge triggered. > > If your hardware can support it, you should also list I/O space and prefetchable > memory spaces. Can you explain why you have multiple non-prefetchable ranges? Our hardware is an ECAM based host controller and doesn't support I/O and prefetchable memory spaces. All on-board PCI devices connected to this PCI controller have fixed resources and doesn't have to be allocated/reassigned. Some of these devices are SRIOV based. Kernel's SRIOV (pci/iov.c) is expecting 'resource->parent' hierarchy to be set, otherwise doesn't enable SRIOV device. So, here multiple non-prefetchable ranges of root bus aid in resource claiming and setting res->parent hierarchy. We do call "pci_claim_resource" in controller driver code. "[PATCH 1/6] pci, thunder: Add support for Thunder PCIe host controller." > > 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 -- 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 Wednesday 24 September 2014 23:34:04 Sunil Kovvuri wrote: > On Wed, Sep 24, 2014 at 9:36 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Wednesday 24 September 2014 17:37:45 Robert Richter wrote: > >> + compatible = "cavium,thunder-pcie"; > >> + device_type = "pci"; > >> + msi-parent = <&its>; > >> + bus-range = <0 255>; > >> + #size-cells = <2>; > >> + #address-cells = <3>; > >> + reg = <0x8480 0x00000000 0 0x10000000>; /* Configuration space */ > >> + ranges = <0x03000000 0x8010 0x00000000 0x8010 0x00000000 0x70 0x00000000>, /* mem ranges */ > >> + <0x03000000 0x8300 0x00000000 0x8300 0x00000000 0x80 0x00000000>, > >> + <0x03000000 0x87e0 0x00000000 0x87e0 0x00000000 0x01 0x00000000>; > >> + }; > > > > If you claim the entire 0-255 bus range, I think you should also > > specify a domain, otherwise it's not predictable which domain you > > get. > > > > The interrupt-map and interrupt-map-mask properties are required for PCI, > > otherwise you can't do LSI interrupts. > > This PCI controller supports only MSIx interrupts which are edge triggered. Interesting, so it's not PCI compliant then? I assume this will be fixed in the production version of the silicon, right? Having no support for interrupts mean that the majority of PCI device drivers will fail. > > If your hardware can support it, you should also list I/O space and prefetchable > > memory spaces. Can you explain why you have multiple non-prefetchable ranges? > > Our hardware is an ECAM based host controller and doesn't support I/O > and prefetchable memory spaces. > All on-board PCI devices connected to this PCI controller have fixed resources > and doesn't have to be allocated/reassigned. Some of these devices are > SRIOV based. I think you need to mark the ones that are nonrelocatable with flag 0x80000000, otherwise the PCI core might decide to reassign them. > Kernel's SRIOV (pci/iov.c) is expecting 'resource->parent' hierarchy > to be set, otherwise doesn't > enable SRIOV device. So, here multiple non-prefetchable ranges of root bus > aid in resource claiming and setting res->parent hierarchy. I don't understand. Isn't that just a bug in the code that you are working around with the DT. Have you tried fixing the code instead? > We do call "pci_claim_resource" in controller driver code. > "[PATCH 1/6] pci, thunder: Add support for Thunder PCIe host controller." My guess is that you are using the wrong interface here. Isn't the normal request_resource() in the host driver enough? 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 Thu, Sep 25, 2014 at 12:04 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Wednesday 24 September 2014 23:34:04 Sunil Kovvuri wrote: >> On Wed, Sep 24, 2014 at 9:36 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> > On Wednesday 24 September 2014 17:37:45 Robert Richter wrote: >> >> + compatible = "cavium,thunder-pcie"; >> >> + device_type = "pci"; >> >> + msi-parent = <&its>; >> >> + bus-range = <0 255>; >> >> + #size-cells = <2>; >> >> + #address-cells = <3>; >> >> + reg = <0x8480 0x00000000 0 0x10000000>; /* Configuration space */ >> >> + ranges = <0x03000000 0x8010 0x00000000 0x8010 0x00000000 0x70 0x00000000>, /* mem ranges */ >> >> + <0x03000000 0x8300 0x00000000 0x8300 0x00000000 0x80 0x00000000>, >> >> + <0x03000000 0x87e0 0x00000000 0x87e0 0x00000000 0x01 0x00000000>; >> >> + }; >> > >> > If you claim the entire 0-255 bus range, I think you should also >> > specify a domain, otherwise it's not predictable which domain you >> > get. >> > >> > The interrupt-map and interrupt-map-mask properties are required for PCI, >> > otherwise you can't do LSI interrupts. >> >> This PCI controller supports only MSIx interrupts which are edge triggered. > > Interesting, so it's not PCI compliant then? I assume this will be fixed > in the production version of the silicon, right? > > Having no support for interrupts mean that the majority of PCI device drivers > will fail. This controller is for on-board PCI devices and all of them do support MSIx interrupts. > >> > If your hardware can support it, you should also list I/O space and prefetchable >> > memory spaces. Can you explain why you have multiple non-prefetchable ranges? >> >> Our hardware is an ECAM based host controller and doesn't support I/O >> and prefetchable memory spaces. >> All on-board PCI devices connected to this PCI controller have fixed resources >> and doesn't have to be allocated/reassigned. Some of these devices are >> SRIOV based. > > I think you need to mark the ones that are nonrelocatable with flag > 0x80000000, otherwise the PCI core might decide to reassign them. Is this flag part of DT pci node properties ? I am using IORESOURCE_PCI_FIXED flag. Its there in other patches of the same series. > >> Kernel's SRIOV (pci/iov.c) is expecting 'resource->parent' hierarchy >> to be set, otherwise doesn't >> enable SRIOV device. So, here multiple non-prefetchable ranges of root bus >> aid in resource claiming and setting res->parent hierarchy. > > I don't understand. Isn't that just a bug in the code that you are working > around with the DT. Have you tried fixing the code instead? I tried but wasn't sure if its going to impact existing SRIOV devices. Will have a deeper look again . > >> We do call "pci_claim_resource" in controller driver code. >> "[PATCH 1/6] pci, thunder: Add support for Thunder PCIe host controller." > > My guess is that you are using the wrong interface here. Isn't the normal > request_resource() in the host driver enough? Isn't that host driver calls "request_resource" only for resources of root port. i.e requesting from iomem_resource/ioport_resource. Here i am referring to SRIOV devices enumerated upon scan of root port. > > 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 Thursday 25 September 2014 00:37:00 Sunil Kovvuri wrote: > On Thu, Sep 25, 2014 at 12:04 AM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Wednesday 24 September 2014 23:34:04 Sunil Kovvuri wrote: > >> On Wed, Sep 24, 2014 at 9:36 PM, Arnd Bergmann <arnd@arndb.de> wrote: > >> > On Wednesday 24 September 2014 17:37:45 Robert Richter wrote: > >> >> + compatible = "cavium,thunder-pcie"; > >> >> + device_type = "pci"; > >> >> + msi-parent = <&its>; > >> >> + bus-range = <0 255>; > >> >> + #size-cells = <2>; > >> >> + #address-cells = <3>; > >> >> + reg = <0x8480 0x00000000 0 0x10000000>; /* Configuration space */ > >> >> + ranges = <0x03000000 0x8010 0x00000000 0x8010 0x00000000 0x70 0x00000000>, /* mem ranges */ > >> >> + <0x03000000 0x8300 0x00000000 0x8300 0x00000000 0x80 0x00000000>, > >> >> + <0x03000000 0x87e0 0x00000000 0x87e0 0x00000000 0x01 0x00000000>; > >> >> + }; > >> > > >> > If you claim the entire 0-255 bus range, I think you should also > >> > specify a domain, otherwise it's not predictable which domain you > >> > get. > >> > > >> > The interrupt-map and interrupt-map-mask properties are required for PCI, > >> > otherwise you can't do LSI interrupts. > >> > >> This PCI controller supports only MSIx interrupts which are edge triggered. > > > > Interesting, so it's not PCI compliant then? I assume this will be fixed > > in the production version of the silicon, right? > > > > Having no support for interrupts mean that the majority of PCI device drivers > > will fail. > > This controller is for on-board PCI devices and all of them do support > MSIx interrupts. But in general, booting with "pci=nomsi" should still work, at least for debugging purposes. It's hard to believe the chip designers forgot to connect the one wire. You said that the PCI host is SBSA compliant, so it must according to section 8.4 have the legacy interrupts wired up to SPI lines in the GIC. > >> > If your hardware can support it, you should also list I/O space and prefetchable > >> > memory spaces. Can you explain why you have multiple non-prefetchable ranges? > >> > >> Our hardware is an ECAM based host controller and doesn't support I/O > >> and prefetchable memory spaces. > >> All on-board PCI devices connected to this PCI controller have fixed resources > >> and doesn't have to be allocated/reassigned. Some of these devices are > >> SRIOV based. > > > > I think you need to mark the ones that are nonrelocatable with flag > > 0x80000000, otherwise the PCI core might decide to reassign them. > > Is this flag part of DT pci node properties ? > I am using IORESOURCE_PCI_FIXED flag. Its there in other patches of > the same series. Ah, right. I checked the source code again and it seems that we don't handle this right at the moment. I think a range that has the nonrelocatable flag set should be used for IORESOURCE_PCI_FIXED mappings without any host specific code, but that needs to be implemented in common code. > >> Kernel's SRIOV (pci/iov.c) is expecting 'resource->parent' hierarchy > >> to be set, otherwise doesn't > >> enable SRIOV device. So, here multiple non-prefetchable ranges of root bus > >> aid in resource claiming and setting res->parent hierarchy. > > > > I don't understand. Isn't that just a bug in the code that you are working > > around with the DT. Have you tried fixing the code instead? > > I tried but wasn't sure if its going to impact existing SRIOV devices. > Will have a deeper look again . IIRC others have reported problems in the same area before, and that turned out to be a resource that was not registered correctly. What do you see in /proc/iomem without your workaround? > >> We do call "pci_claim_resource" in controller driver code. > >> "[PATCH 1/6] pci, thunder: Add support for Thunder PCIe host controller." > > > > My guess is that you are using the wrong interface here. Isn't the normal > > request_resource() in the host driver enough? > > Isn't that host driver calls "request_resource" only for resources of root port. > i.e requesting from iomem_resource/ioport_resource. Here i am referring > to SRIOV devices enumerated upon scan of root port. The device resources should be nested within the bus resources, as you would e.g. find on my PC system: dc000000-dcffffff : PCI Bus 0000:40 dcc00000-dcefffff : PCI Bus 0000:41 dcce0000-dccfffff : 0000:41:00.0 dcd00000-dcefffff : PCI Bus 0000:42 dcd00000-dcdfffff : PCI Bus 0000:45 dcdef800-dcdeffff : 0000:45:00.0 dcdef800-dcdeffff : ahci 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 Thu, Sep 25, 2014 at 1:31 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Thursday 25 September 2014 00:37:00 Sunil Kovvuri wrote: >> On Thu, Sep 25, 2014 at 12:04 AM, Arnd Bergmann <arnd@arndb.de> wrote: >> > On Wednesday 24 September 2014 23:34:04 Sunil Kovvuri wrote: >> >> All on-board PCI devices connected to this PCI controller have fixed resources >> >> and doesn't have to be allocated/reassigned. Some of these devices are >> >> SRIOV based. >> > >> > I think you need to mark the ones that are nonrelocatable with flag >> > 0x80000000, otherwise the PCI core might decide to reassign them. >> >> Is this flag part of DT pci node properties ? >> I am using IORESOURCE_PCI_FIXED flag. Its there in other patches of >> the same series. > > Ah, right. I checked the source code again and it seems that we don't handle > this right at the moment. I think a range that has the nonrelocatable > flag set should be used for IORESOURCE_PCI_FIXED mappings without any > host specific code, but that needs to be implemented in common code. What connection do you envision between nonrelocatable ranges and IORESOURCE_PCI_FIXED? I don't know what a nonrelocatable range is, but for IORESOURCE_PCI_FIXED, all I intend is that the PCI core should not try to assign a different address, e.g., because the BAR is read-only or because it's a legacy IDE/VGA/etc. range for which there is no BAR at all. Bjorn -- 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 Thursday 25 September 2014, Bjorn Helgaas wrote: > On Thu, Sep 25, 2014 at 1:31 AM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Thursday 25 September 2014 00:37:00 Sunil Kovvuri wrote: > >> On Thu, Sep 25, 2014 at 12:04 AM, Arnd Bergmann <arnd@arndb.de> wrote: > >> > On Wednesday 24 September 2014 23:34:04 Sunil Kovvuri wrote: > > >> >> All on-board PCI devices connected to this PCI controller have fixed resources > >> >> and doesn't have to be allocated/reassigned. Some of these devices are > >> >> SRIOV based. > >> > > >> > I think you need to mark the ones that are nonrelocatable with flag > >> > 0x80000000, otherwise the PCI core might decide to reassign them. > >> > >> Is this flag part of DT pci node properties ? > >> I am using IORESOURCE_PCI_FIXED flag. Its there in other patches of > >> the same series. > > > > Ah, right. I checked the source code again and it seems that we don't handle > > this right at the moment. I think a range that has the nonrelocatable > > flag set should be used for IORESOURCE_PCI_FIXED mappings without any > > host specific code, but that needs to be implemented in common code. > > What connection do you envision between nonrelocatable ranges and > IORESOURCE_PCI_FIXED? I don't know what a nonrelocatable range is, > but for IORESOURCE_PCI_FIXED, all I intend is that the PCI core should > not try to assign a different address, e.g., because the BAR is > read-only or because it's a legacy IDE/VGA/etc. range for which there > is no BAR at all. I think that is exactly the definition of the nonrelocatable flag in http://www.openfirmware.org/1275/bindings/pci/pci2_1.pdf The example given in section 11.1.2 is for a VGA device that has some relocatable BARs and some nonrelocatable BARs. 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 Thu, Sep 25, 2014 at 1:26 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Thursday 25 September 2014, Bjorn Helgaas wrote: >> On Thu, Sep 25, 2014 at 1:31 AM, Arnd Bergmann <arnd@arndb.de> wrote: >> > On Thursday 25 September 2014 00:37:00 Sunil Kovvuri wrote: >> >> On Thu, Sep 25, 2014 at 12:04 AM, Arnd Bergmann <arnd@arndb.de> wrote: >> >> > On Wednesday 24 September 2014 23:34:04 Sunil Kovvuri wrote: >> >> >> >> All on-board PCI devices connected to this PCI controller have fixed resources >> >> >> and doesn't have to be allocated/reassigned. Some of these devices are >> >> >> SRIOV based. >> >> > >> >> > I think you need to mark the ones that are nonrelocatable with flag >> >> > 0x80000000, otherwise the PCI core might decide to reassign them. >> >> >> >> Is this flag part of DT pci node properties ? >> >> I am using IORESOURCE_PCI_FIXED flag. Its there in other patches of >> >> the same series. >> > >> > Ah, right. I checked the source code again and it seems that we don't handle >> > this right at the moment. I think a range that has the nonrelocatable >> > flag set should be used for IORESOURCE_PCI_FIXED mappings without any >> > host specific code, but that needs to be implemented in common code. >> >> What connection do you envision between nonrelocatable ranges and >> IORESOURCE_PCI_FIXED? I don't know what a nonrelocatable range is, >> but for IORESOURCE_PCI_FIXED, all I intend is that the PCI core should >> not try to assign a different address, e.g., because the BAR is >> read-only or because it's a legacy IDE/VGA/etc. range for which there >> is no BAR at all. > > I think that is exactly the definition of the nonrelocatable flag > in http://www.openfirmware.org/1275/bindings/pci/pci2_1.pdf > > The example given in section 11.1.2 is for a VGA device that has some > relocatable BARs and some nonrelocatable BARs. OK. You said "a range that has the nonrelocatable flag set should be used for IORESOURCE_PCI_FIXED mappings." I thought you meant that the range was a bridge window, and somehow PCI_FIXED BARs should be put in that window. But maybe you meant that nonrelocatable ranges should have the IORESOURCE_PCI_FIXED bit set in their struct resources. That would mean we couldn't move the window, but we could put relocatable BARs inside the window. What needs to be implemented? Just the code that would set IORESOURCE_PCI_FIXED for nonrelocatable ranges? Bjorn -- 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 Thursday 25 September 2014, Bjorn Helgaas wrote: > OK. You said "a range that has the nonrelocatable flag set should be > used for IORESOURCE_PCI_FIXED mappings." I thought you meant that the > range was a bridge window, and somehow PCI_FIXED BARs should be put in > that window. > > But maybe you meant that nonrelocatable ranges should have the > IORESOURCE_PCI_FIXED bit set in their struct resources. That would > mean we couldn't move the window, but we could put relocatable BARs > inside the window. > > What needs to be implemented? Just the code that would set > IORESOURCE_PCI_FIXED for nonrelocatable ranges? It might be more complex than I thought. Let's see what the original hack does: +/* + * All PCIe devices in Thunder have fixed resources, shouldn't be reassigned. + * Also claim the device's valid resources to set 'res->parent' hierarchy. + */ +static void pci_dev_resource_fixup(struct pci_dev *dev) +{ + struct resource *res; + int resno; + + for (resno = 0; resno < PCI_NUM_RESOURCES; resno++) + dev->resource[resno].flags |= IORESOURCE_PCI_FIXED; + + for (resno = 0; resno < PCI_BRIDGE_RESOURCES; resno++) { + res = &dev->resource[resno]; + if (res->parent || !(res->flags & IORESOURCE_MEM)) + continue; + pci_claim_resource(dev, resno); + } +} +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CAVIUM, PCI_ANY_ID, + pci_dev_resource_fixup); This actually looks harmful, because it means that any kernel that contains the thunder host controller driver will do the above for any device made by Cavium, whether it's connected to this bridge or not. What I think we want instead is to mark any resource whose parent resource is IORESOURCE_PCI_FIXED to have the same flag, and mark the PCI host controller resources IORESOURCE_PCI_FIXED when the nonrelocatable flag is set, and that should all be done in core code, not in a driver fixup. The part that still looks weird is the pci_claim_resource() that Sunil mentioned. This is currently done for resources that do not have a parent, but AFAICT all PCI device resources should have a parent that connects it to the upstream bridge. 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 Thu, Sep 25, 2014 at 2:22 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Thursday 25 September 2014, Bjorn Helgaas wrote: >> OK. You said "a range that has the nonrelocatable flag set should be >> used for IORESOURCE_PCI_FIXED mappings." I thought you meant that the >> range was a bridge window, and somehow PCI_FIXED BARs should be put in >> that window. >> >> But maybe you meant that nonrelocatable ranges should have the >> IORESOURCE_PCI_FIXED bit set in their struct resources. That would >> mean we couldn't move the window, but we could put relocatable BARs >> inside the window. >> >> What needs to be implemented? Just the code that would set >> IORESOURCE_PCI_FIXED for nonrelocatable ranges? > > It might be more complex than I thought. Let's see what the original > hack does: > > +/* > + * All PCIe devices in Thunder have fixed resources, shouldn't be reassigned. > + * Also claim the device's valid resources to set 'res->parent' hierarchy. > + */ > +static void pci_dev_resource_fixup(struct pci_dev *dev) > +{ > + struct resource *res; > + int resno; > + > + for (resno = 0; resno < PCI_NUM_RESOURCES; resno++) > + dev->resource[resno].flags |= IORESOURCE_PCI_FIXED; > + > + for (resno = 0; resno < PCI_BRIDGE_RESOURCES; resno++) { > + res = &dev->resource[resno]; > + if (res->parent || !(res->flags & IORESOURCE_MEM)) > + continue; > + pci_claim_resource(dev, resno); > + } > +} > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CAVIUM, PCI_ANY_ID, > + pci_dev_resource_fixup); > > This actually looks harmful, because it means that any kernel that contains > the thunder host controller driver will do the above for any device made > by Cavium, whether it's connected to this bridge or not. I agree, that looks broken. > What I think we want instead is to mark any resource whose parent > resource is IORESOURCE_PCI_FIXED to have the same flag, and mark > the PCI host controller resources IORESOURCE_PCI_FIXED when the > nonrelocatable flag is set, and that should all be done in core > code, not in a driver fixup. The PCI host controller resources are host bridge windows, right? I don't think it matters whether they have IORESOURCE_PCI_FIXED set, because the bridge is not itself a PCI device, and PCI resource assignment treats the host bridge windows as fixed. That doesn't imply that the PCI device resources *inside* the windows need to be fixed, though. Regular BARs can be moved around inside the window. Sunil said "All on-board PCI devices connected to this PCI controller have fixed resources..." That sounds like these on-board PCI devices are non-compliant because their BARs don't work per spec. But that doesn't sound right, because we wouldn't be able to size them. > The part that still looks weird is the pci_claim_resource() that > Sunil mentioned. This is currently done for resources that do not > have a parent, but AFAICT all PCI device resources should have a > parent that connects it to the upstream bridge. Ideally the pci_claim_resource() would be in the core, not in arch code, but it's a bit of a hodge-podge right now. Bjorn -- 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, Sep 24, 2014 at 11:06 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Wednesday 24 September 2014 17:37:45 Robert Richter wrote: >> >> + pcie0@0x8480,00000000 { > > The name should be pci, not pci0. And the address should be "@848000000000". There was some confusion about when the comma should be used and it is only if you have things like PCI bus, device, function for the addressing. > >> + compatible = "cavium,thunder-pcie"; A bit too generic as many of the Cavium bindings have been. Rob -- 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
Thanks for pointing correction ROB. Will fix it. On Fri, Sep 26, 2014 at 11:56 PM, Rob Herring <robherring2@gmail.com> wrote: > On Wed, Sep 24, 2014 at 11:06 AM, Arnd Bergmann <arnd@arndb.de> wrote: >> On Wednesday 24 September 2014 17:37:45 Robert Richter wrote: >>> >>> + pcie0@0x8480,00000000 { >> >> The name should be pci, not pci0. > > And the address should be "@848000000000". There was some confusion > about when the comma should be used and it is only if you have things > like PCI bus, device, function for the addressing. > > >> >>> + compatible = "cavium,thunder-pcie"; > > A bit too generic as many of the Cavium bindings have been. > > Rob > -- > 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 -- 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 24.09.14 18:06:04, Arnd Bergmann wrote: > > + compatible = "cavium,thunder-pcie"; > > + device_type = "pci"; > > + msi-parent = <&its>; > > + bus-range = <0 255>; > > + #size-cells = <2>; > > + #address-cells = <3>; > > + reg = <0x8480 0x00000000 0 0x10000000>; /* Configuration space */ > > + ranges = <0x03000000 0x8010 0x00000000 0x8010 0x00000000 0x70 0x00000000>, /* mem ranges */ > > + <0x03000000 0x8300 0x00000000 0x8300 0x00000000 0x80 0x00000000>, > > + <0x03000000 0x87e0 0x00000000 0x87e0 0x00000000 0x01 0x00000000>; > > + }; > > If you claim the entire 0-255 bus range, I think you should also > specify a domain, otherwise it's not predictable which domain you > get. Liviu's code assigns a unique id to the domain if missing, see of_pci_get_domain_nr(). So I don't think we need to add a "pci-domain" property here. Liviu's DT implementation that assigns a unique number differs a bit from ACPI which states: "If _SEG [aka domain number] does not exist, OSPM assumes that all PCI bus segments are in PCI Segment Group 0." Maybe of_pci_get_domain_nr() should be similar to ACPI. If there are multiple root bridges, the "pci-domain" property could be forced instead. -Robert -- 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, Oct 07, 2014 at 03:27:44PM +0100, Robert Richter wrote: > On 24.09.14 18:06:04, Arnd Bergmann wrote: > > > + compatible = "cavium,thunder-pcie"; > > > + device_type = "pci"; > > > + msi-parent = <&its>; > > > + bus-range = <0 255>; > > > + #size-cells = <2>; > > > + #address-cells = <3>; > > > + reg = <0x8480 0x00000000 0 0x10000000>; /* Configuration space */ > > > + ranges = <0x03000000 0x8010 0x00000000 0x8010 0x00000000 0x70 0x00000000>, /* mem ranges */ > > > + <0x03000000 0x8300 0x00000000 0x8300 0x00000000 0x80 0x00000000>, > > > + <0x03000000 0x87e0 0x00000000 0x87e0 0x00000000 0x01 0x00000000>; > > > + }; > > > > If you claim the entire 0-255 bus range, I think you should also > > specify a domain, otherwise it's not predictable which domain you > > get. > > Liviu's code assigns a unique id to the domain if missing, see > of_pci_get_domain_nr(). So I don't think we need to add a "pci-domain" > property here. Not anymore! That function is gone in v12 onwards. What is in -next has a new function called of_get_pci_domain_nr() (slight name change) but that only gets the value set in the "linux,pci-domain" property of the device node. It is the choice of the host bridge driver to use that function or to use pci_get_new_domain_nr() which *does* generate an unique id every time it gets called. > > Liviu's DT implementation that assigns a unique number differs a bit > from ACPI which states: "If _SEG [aka domain number] does not exist, > OSPM assumes that all PCI bus segments are in PCI Segment Group 0." > > Maybe of_pci_get_domain_nr() should be similar to ACPI. If there are > multiple root bridges, the "pci-domain" property could be forced > instead. Indeed. But the enforcing is left as an exercise to the host bridge implementor for the moment. Best regards, Liviu > > -Robert >
On 07.10.14 16:01:49, Liviu Dudau wrote: > On Tue, Oct 07, 2014 at 03:27:44PM +0100, Robert Richter wrote: > > On 24.09.14 18:06:04, Arnd Bergmann wrote: > > > > + compatible = "cavium,thunder-pcie"; > > > > + device_type = "pci"; > > > > + msi-parent = <&its>; > > > > + bus-range = <0 255>; > > > > + #size-cells = <2>; > > > > + #address-cells = <3>; > > > > + reg = <0x8480 0x00000000 0 0x10000000>; /* Configuration space */ > > > > + ranges = <0x03000000 0x8010 0x00000000 0x8010 0x00000000 0x70 0x00000000>, /* mem ranges */ > > > > + <0x03000000 0x8300 0x00000000 0x8300 0x00000000 0x80 0x00000000>, > > > > + <0x03000000 0x87e0 0x00000000 0x87e0 0x00000000 0x01 0x00000000>; > > > > + }; > > > > > > If you claim the entire 0-255 bus range, I think you should also > > > specify a domain, otherwise it's not predictable which domain you > > > get. > > > > Liviu's code assigns a unique id to the domain if missing, see > > of_pci_get_domain_nr(). So I don't think we need to add a "pci-domain" > > property here. > > Not anymore! That function is gone in v12 onwards. What is in -next has > a new function called of_get_pci_domain_nr() (slight name change) but > that only gets the value set in the "linux,pci-domain" property of the > device node. It is the choice of the host bridge driver to use that > function or to use pci_get_new_domain_nr() which *does* generate an > unique id every time it gets called. I am quite confused a bit on which is the latest code base now. I was looking into Bjorn's pci/host-generic and there is a different implemetation: ---- /** * This function will try to obtain the host bridge domain number by * using of_alias_get_id() call with "pci-domain" as a stem. If that * fails, a local allocator will be used. The local allocator can * be requested to return a new domain_nr if the information is missing * from the device tree. * * @node: device tree node with the domain information * @allocate_if_missing: if DT lacks information about the domain nr, * allocate a new number. * * Returns the associated domain number from DT, or a new domain number * if DT information is missing and @allocate_if_missing is true. If * @allocate_if_missing is false then the last allocated domain number * will be returned. */ int of_pci_get_domain_nr(struct device_node *node, bool allocate_if_missing) { int domain; domain = atomic_read(&of_domain_nr); if (domain == -1) { /* first run, get max defined domain nr in device tree */ domain = of_get_max_pci_domain_nr(); /* then set the start value for allocator to be max + 1 */ atomic_set(&of_domain_nr, domain + 1); } domain = of_alias_get_id(node, "pci-domain"); if (domain == -ENODEV) { domain = atomic_read(&of_domain_nr); if (allocate_if_missing) atomic_inc(&of_domain_nr); } return domain; } EXPORT_SYMBOL_GPL(of_pci_get_domain_nr); ---- This differs also from v13. Please check. It would be good to have a stable code base to work with, so I would prefer incremental patches on top of Bjorn's pci/host-generic. > > Liviu's DT implementation that assigns a unique number differs a bit > > from ACPI which states: "If _SEG [aka domain number] does not exist, > > OSPM assumes that all PCI bus segments are in PCI Segment Group 0." > > > > Maybe of_pci_get_domain_nr() should be similar to ACPI. If there are > > multiple root bridges, the "pci-domain" property could be forced > > instead. > > Indeed. But the enforcing is left as an exercise to the host bridge > implementor for the moment. Right, can be added later. Thanks, -Robert -- 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, Oct 08, 2014 at 09:49:27AM +0100, Robert Richter wrote: > On 07.10.14 16:01:49, Liviu Dudau wrote: > > On Tue, Oct 07, 2014 at 03:27:44PM +0100, Robert Richter wrote: > > > On 24.09.14 18:06:04, Arnd Bergmann wrote: > > > > > + compatible = "cavium,thunder-pcie"; > > > > > + device_type = "pci"; > > > > > + msi-parent = <&its>; > > > > > + bus-range = <0 255>; > > > > > + #size-cells = <2>; > > > > > + #address-cells = <3>; > > > > > + reg = <0x8480 0x00000000 0 0x10000000>; /* Configuration space */ > > > > > + ranges = <0x03000000 0x8010 0x00000000 0x8010 0x00000000 0x70 0x00000000>, /* mem ranges */ > > > > > + <0x03000000 0x8300 0x00000000 0x8300 0x00000000 0x80 0x00000000>, > > > > > + <0x03000000 0x87e0 0x00000000 0x87e0 0x00000000 0x01 0x00000000>; > > > > > + }; > > > > > > > > If you claim the entire 0-255 bus range, I think you should also > > > > specify a domain, otherwise it's not predictable which domain you > > > > get. > > > > > > Liviu's code assigns a unique id to the domain if missing, see > > > of_pci_get_domain_nr(). So I don't think we need to add a "pci-domain" > > > property here. > > > > Not anymore! That function is gone in v12 onwards. What is in -next has > > a new function called of_get_pci_domain_nr() (slight name change) but > > that only gets the value set in the "linux,pci-domain" property of the > > device node. It is the choice of the host bridge driver to use that > > function or to use pci_get_new_domain_nr() which *does* generate an > > unique id every time it gets called. > > I am quite confused a bit on which is the latest code base now. I was > looking into Bjorn's pci/host-generic and there is a different > implemetation: > > ---- > /** > * This function will try to obtain the host bridge domain number by > * using of_alias_get_id() call with "pci-domain" as a stem. If that > * fails, a local allocator will be used. The local allocator can > * be requested to return a new domain_nr if the information is missing > * from the device tree. > * > * @node: device tree node with the domain information > * @allocate_if_missing: if DT lacks information about the domain nr, > * allocate a new number. > * > * Returns the associated domain number from DT, or a new domain number > * if DT information is missing and @allocate_if_missing is true. If > * @allocate_if_missing is false then the last allocated domain number > * will be returned. > */ > int of_pci_get_domain_nr(struct device_node *node, bool allocate_if_missing) > { > int domain; > > domain = atomic_read(&of_domain_nr); > if (domain == -1) { > /* first run, get max defined domain nr in device tree */ > domain = of_get_max_pci_domain_nr(); > /* then set the start value for allocator to be max + 1 */ > atomic_set(&of_domain_nr, domain + 1); > } > domain = of_alias_get_id(node, "pci-domain"); > if (domain == -ENODEV) { > domain = atomic_read(&of_domain_nr); > if (allocate_if_missing) > atomic_inc(&of_domain_nr); > } > > return domain; > } > EXPORT_SYMBOL_GPL(of_pci_get_domain_nr); > ---- > > This differs also from v13. Please check. I'm not sure what you are looking at. Commit 41e5c0f81d3e does look like my v13. Not sure why you are still seeing this v11 version. > > It would be good to have a stable code base to work with, so I would > prefer incremental patches on top of Bjorn's pci/host-generic. Agreed, but from what I can see from my side pci/host-generic and next have the same versions, so there should not be any confusion. Best regards, Liviu > > > > Liviu's DT implementation that assigns a unique number differs a bit > > > from ACPI which states: "If _SEG [aka domain number] does not exist, > > > OSPM assumes that all PCI bus segments are in PCI Segment Group 0." > > > > > > Maybe of_pci_get_domain_nr() should be similar to ACPI. If there are > > > multiple root bridges, the "pci-domain" property could be forced > > > instead. > > > > Indeed. But the enforcing is left as an exercise to the host bridge > > implementor for the moment. > > Right, can be added later. > > Thanks, > > -Robert > -- > 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 08.10.14 17:44:32, Liviu Dudau wrote: > On Wed, Oct 08, 2014 at 09:49:27AM +0100, Robert Richter wrote: > > On 07.10.14 16:01:49, Liviu Dudau wrote: > > I am quite confused a bit on which is the latest code base now. I was > > looking into Bjorn's pci/host-generic and there is a different > > implemetation: > > This differs also from v13. Please check. > I'm not sure what you are looking at. Commit 41e5c0f81d3e does look like > my v13. Not sure why you are still seeing this v11 version. Right, my bad. Sorry for the noise. -Robert -- 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
diff --git a/arch/arm64/boot/dts/thunder-88xx.dtsi b/arch/arm64/boot/dts/thunder-88xx.dtsi index 9cb7cf94284a..0b433b0e7af4 100644 --- a/arch/arm64/boot/dts/thunder-88xx.dtsi +++ b/arch/arm64/boot/dts/thunder-88xx.dtsi @@ -407,4 +407,53 @@ clock-names = "apb_pclk"; }; }; + + pcie0@0x8480,00000000 { + compatible = "cavium,thunder-pcie"; + device_type = "pci"; + msi-parent = <&its>; + bus-range = <0 255>; + #size-cells = <2>; + #address-cells = <3>; + reg = <0x8480 0x00000000 0 0x10000000>; /* Configuration space */ + ranges = <0x03000000 0x8010 0x00000000 0x8010 0x00000000 0x70 0x00000000>, /* mem ranges */ + <0x03000000 0x8300 0x00000000 0x8300 0x00000000 0x80 0x00000000>, + <0x03000000 0x87e0 0x00000000 0x87e0 0x00000000 0x01 0x00000000>; + }; + + pcie1@0x8490,00000000 { + compatible = "cavium,thunder-pcie"; + device_type = "pci"; + msi-parent = <&its>; + bus-range = <0 255>; + #size-cells = <2>; + #address-cells = <3>; + reg = <0x8490 0x00000000 0 0x10000000>; /* Configuration space */ + ranges = <0x03000000 0x8310 0x00000000 0x8310 0x00000000 0x00 0x10000000>, /* mem ranges */ + <0x03000000 0x8100 0x00000000 0x8100 0x00000000 0x80 0x00000000>; + }; + + pcie2@0x84a0,00000000 { + compatible = "cavium,thunder-pcie"; + device_type = "pci"; + msi-parent = <&its>; + bus-range = <0 255>; + #size-cells = <2>; + #address-cells = <3>; + reg = <0x84a0 0x00000000 0 0x10000000>; /* Configuration space */ + ranges = <0x03000000 0x8320 0x00000000 0x8320 0x00000000 0x00 0x10000000>, /* mem ranges */ + <0x03000000 0x8430 0x00000000 0x8430 0x00000000 0x01 0x00000000>; + }; + + pcie3@0x84b0,00000000 { + compatible = "cavium,thunder-pcie"; + device_type = "pci"; + msi-parent = <&its>; + bus-range = <0 255>; + #size-cells = <2>; + #address-cells = <3>; + reg = <0x84b0 0x00000000 0 0x10000000>; /* Configuration space */ + ranges = <0x03000000 0x8330 0x00000000 0x8330 0x00000000 0x00 0x10000000>, /* mem ranges */ + <0x03000000 0x8180 0x00000000 0x8180 0x00000000 0x80 0x00000000>; + }; };