| Message ID | 20160921223805.21652-1-cov@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 21, 2016 at 06:38:05PM -0400, Christopher Covington wrote: > The Qualcomm Technologies QDF2432 SoC does not support accesses smaller > than 32 bits to the PCI configuration space. Register the appropriate > quirk. > > Signed-off-by: Christopher Covington <cov@codeaurora.org> Hi Christopher, Can you rebase this against v4.9-rc1? It no longer applies to my tree. Note that this hardware is not spec-compliant since it doesn't support sub-32 bit config writes. I just proposed a patch to warn about that [1], so if/when we merge that patch and this one, you'll start seeing those warnings. [1] http://lkml.kernel.org/r/20161031213902.6340.96123.stgit@bhelgaas-glaptop.roam.corp.google.com > --- > drivers/acpi/pci_mcfg.c | 8 ++++++++ > drivers/pci/ecam.c | 10 ++++++++++ > include/linux/pci-ecam.h | 1 + > 3 files changed, 19 insertions(+) > > diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c > index 245b79f..212334f 100644 > --- a/drivers/acpi/pci_mcfg.c > +++ b/drivers/acpi/pci_mcfg.c > @@ -96,6 +96,14 @@ static struct mcfg_fixup mcfg_quirks[] = { > THUNDER_ECAM_MCFG(2, 12), > THUNDER_ECAM_MCFG(2, 13), > #endif > + { "QCOM ", "QDF2432 ", 1, 0, MCFG_BUS_ANY, &pci_32b_ops }, > + { "QCOM ", "QDF2432 ", 1, 1, MCFG_BUS_ANY, &pci_32b_ops }, > + { "QCOM ", "QDF2432 ", 1, 2, MCFG_BUS_ANY, &pci_32b_ops }, > + { "QCOM ", "QDF2432 ", 1, 3, MCFG_BUS_ANY, &pci_32b_ops }, > + { "QCOM ", "QDF2432 ", 1, 4, MCFG_BUS_ANY, &pci_32b_ops }, > + { "QCOM ", "QDF2432 ", 1, 5, MCFG_BUS_ANY, &pci_32b_ops }, > + { "QCOM ", "QDF2432 ", 1, 6, MCFG_BUS_ANY, &pci_32b_ops }, > + { "QCOM ", "QDF2432 ", 1, 7, MCFG_BUS_ANY, &pci_32b_ops }, > }; > > static char mcfg_oem_id[ACPI_OEM_ID_SIZE]; > diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c > index 43ed08d..c3b3063 100644 > --- a/drivers/pci/ecam.c > +++ b/drivers/pci/ecam.c > @@ -162,3 +162,13 @@ struct pci_ecam_ops pci_generic_ecam_ops = { > .write = pci_generic_config_write, > } > }; > + > +/* ops for 32 bit config space access quirk */ > +struct pci_ecam_ops pci_32b_ops = { > + .bus_shift = 20, > + .pci_ops = { > + .map_bus = pci_ecam_map_bus, > + .read = pci_generic_config_read32, > + .write = pci_generic_config_write32, > + } > +}; > diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h > index 35f0e81..a6cffb8 100644 > --- a/include/linux/pci-ecam.h > +++ b/include/linux/pci-ecam.h > @@ -65,6 +65,7 @@ extern struct pci_ecam_ops pci_thunder_pem_ops; > #ifdef CONFIG_PCI_HOST_THUNDER_ECAM > extern struct pci_ecam_ops pci_thunder_ecam_ops; > #endif > +extern struct pci_ecam_ops pci_32b_ops; > > #ifdef CONFIG_PCI_HOST_GENERIC > /* for DT-based PCI controllers that support ECAM */ > -- > Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm > Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora > Forum, a Linux Foundation Collaborative Project. >
Hi Bjorn, On 2016-10-31 15:48, Bjorn Helgaas wrote: > On Wed, Sep 21, 2016 at 06:38:05PM -0400, Christopher Covington wrote: >> The Qualcomm Technologies QDF2432 SoC does not support accesses >> smaller >> than 32 bits to the PCI configuration space. Register the appropriate >> quirk. >> >> Signed-off-by: Christopher Covington <cov@codeaurora.org> > > Hi Christopher, > > Can you rebase this against v4.9-rc1? It no longer applies to my tree. I apologize for not being clearer. This patch depends on: PCI/ACPI: Extend pci_mcfg_lookup() responsibilities PCI/ACPI: Check platform-specific ECAM quirks These patches from Tomasz Nowicki were previously in your pci/ecam-v6 branch, but that seems to have come and gone. How would you like to proceed? > Note that this hardware is not spec-compliant since it doesn't support > sub-32 bit config writes. I just proposed a patch to warn about that > [1], so if/when we merge that patch and this one, you'll start seeing > those warnings. > > [1] > http://lkml.kernel.org/r/20161031213902.6340.96123.stgit@bhelgaas-glaptop.roam.corp.google.com That looks great, thank you. The earlier PCI HDL and SoC vendors can be made aware of such problems, the better. Thanks, Cov
On Tue, Nov 01, 2016 at 07:06:31AM -0600, cov@codeaurora.org wrote: > Hi Bjorn, > > On 2016-10-31 15:48, Bjorn Helgaas wrote: > >On Wed, Sep 21, 2016 at 06:38:05PM -0400, Christopher Covington wrote: > >>The Qualcomm Technologies QDF2432 SoC does not support accesses > >>smaller > >>than 32 bits to the PCI configuration space. Register the appropriate > >>quirk. > >> > >>Signed-off-by: Christopher Covington <cov@codeaurora.org> > > > >Hi Christopher, > > > >Can you rebase this against v4.9-rc1? It no longer applies to my tree. > > I apologize for not being clearer. This patch depends on: > > PCI/ACPI: Extend pci_mcfg_lookup() responsibilities > PCI/ACPI: Check platform-specific ECAM quirks > > These patches from Tomasz Nowicki were previously in your pci/ecam-v6 > branch, but that seems to have come and gone. How would you like to > proceed? Oh yes, that's right, I forgot that connection. I'm afraid I kind of dropped the ball on that thread, so I went back and read through it again. I *think* the current state is: - I'm OK with the first two patches that add the quirk infrastructure. - My issue with the last three patches that add ThunderX quirks is that there's no generic description of the ECAM address space. So if I understand correctly, your Qualcomm patch depends only on the first two patches. Then the question is how the Qualcomm ECAM address space is described. Your quirk overrides the default pci_generic_ecam_ops with the &pci_32b_ops, but it doesn't touch the address space part, so I assume the bus ranges and corresponding address space in your MCFG is correct. So far, so good. Is there also an ACPI device that contains that space in _CRS? I think we concluded that the standard solution is to describe this with a PNP0C02 device. Would you mind opening a bugzilla at bugzilla.kernel.org and attaching the dmesg log, /proc/iomem, and maybe a DSDT dump? I'd like to have something to point at to say "if you need an MCFG quirk, you need the MCFG bit and *also* these other related ACPI device bits, and here's how it should be done." Bjorn
Hi Bjorn, On 11/2/2016 12:08 PM, Bjorn Helgaas wrote: > On Tue, Nov 01, 2016 at 07:06:31AM -0600, cov@codeaurora.org wrote: >> Hi Bjorn, >> >> On 2016-10-31 15:48, Bjorn Helgaas wrote: >>> On Wed, Sep 21, 2016 at 06:38:05PM -0400, Christopher Covington wrote: >>>> The Qualcomm Technologies QDF2432 SoC does not support accesses >>>> smaller >>>> than 32 bits to the PCI configuration space. Register the appropriate >>>> quirk. >>>> >>>> Signed-off-by: Christopher Covington <cov@codeaurora.org> >>> >>> Hi Christopher, >>> >>> Can you rebase this against v4.9-rc1? It no longer applies to my tree. >> >> I apologize for not being clearer. This patch depends on: >> >> PCI/ACPI: Extend pci_mcfg_lookup() responsibilities >> PCI/ACPI: Check platform-specific ECAM quirks >> >> These patches from Tomasz Nowicki were previously in your pci/ecam-v6 >> branch, but that seems to have come and gone. How would you like to >> proceed? > > Oh yes, that's right, I forgot that connection. I'm afraid I kind of > dropped the ball on that thread, so I went back and read through it > again. > > I *think* the current state is: > > - I'm OK with the first two patches that add the quirk > infrastructure. > > - My issue with the last three patches that add ThunderX quirks is > that there's no generic description of the ECAM address space. > > So if I understand correctly, your Qualcomm patch depends only on the > first two patches. > > Then the question is how the Qualcomm ECAM address space is described. > Your quirk overrides the default pci_generic_ecam_ops with the > &pci_32b_ops, but it doesn't touch the address space part, so I assume > the bus ranges and corresponding address space in your MCFG is > correct. So far, so good. Qualcomm ECAM space includes both the root port and the endpoint address space with a single contiguous 256 MB address space described in MCFG table. There is no need to describe additional resources like PNP0C02. The only thing we missed was 8/16 bits access support on the root port. That's why, we need Cov's patch. > > Is there also an ACPI device that contains that space in _CRS? I > think we concluded that the standard solution is to describe this with > a PNP0C02 device. > > Would you mind opening a bugzilla at bugzilla.kernel.org and attaching > the dmesg log, /proc/iomem, and maybe a DSDT dump? I'd like to have > something to point at to say "if you need an MCFG quirk, you need the > MCFG bit and *also* these other related ACPI device bits, and here's > how it should be done." > > Bjorn >
On Wed, Nov 02, 2016 at 11:08:20AM -0500, Bjorn Helgaas wrote: > On Tue, Nov 01, 2016 at 07:06:31AM -0600, cov@codeaurora.org wrote: > > Hi Bjorn, > > > > On 2016-10-31 15:48, Bjorn Helgaas wrote: > > >On Wed, Sep 21, 2016 at 06:38:05PM -0400, Christopher Covington wrote: > > >>The Qualcomm Technologies QDF2432 SoC does not support accesses > > >>smaller > > >>than 32 bits to the PCI configuration space. Register the appropriate > > >>quirk. > > >> > > >>Signed-off-by: Christopher Covington <cov@codeaurora.org> > > > > > >Hi Christopher, > > > > > >Can you rebase this against v4.9-rc1? It no longer applies to my tree. > > > > I apologize for not being clearer. This patch depends on: > > > > PCI/ACPI: Extend pci_mcfg_lookup() responsibilities > > PCI/ACPI: Check platform-specific ECAM quirks > > > > These patches from Tomasz Nowicki were previously in your pci/ecam-v6 > > branch, but that seems to have come and gone. How would you like to > > proceed? > > Oh yes, that's right, I forgot that connection. I'm afraid I kind of > dropped the ball on that thread, so I went back and read through it > again. > > I *think* the current state is: > > - I'm OK with the first two patches that add the quirk > infrastructure. > > - My issue with the last three patches that add ThunderX quirks is > that there's no generic description of the ECAM address space. > > So if I understand correctly, your Qualcomm patch depends only on the > first two patches. I put those first two patches and yours on pci/ecam-v6 and pushed it again, so you can check it out. Bjorn
On Wed, Nov 02, 2016 at 12:36:16PM -0400, Sinan Kaya wrote: > Hi Bjorn, > > On 11/2/2016 12:08 PM, Bjorn Helgaas wrote: > > On Tue, Nov 01, 2016 at 07:06:31AM -0600, cov@codeaurora.org wrote: > >> Hi Bjorn, > >> > >> On 2016-10-31 15:48, Bjorn Helgaas wrote: > >>> On Wed, Sep 21, 2016 at 06:38:05PM -0400, Christopher Covington wrote: > >>>> The Qualcomm Technologies QDF2432 SoC does not support accesses > >>>> smaller > >>>> than 32 bits to the PCI configuration space. Register the appropriate > >>>> quirk. > >>>> > >>>> Signed-off-by: Christopher Covington <cov@codeaurora.org> > >>> > >>> Hi Christopher, > >>> > >>> Can you rebase this against v4.9-rc1? It no longer applies to my tree. > >> > >> I apologize for not being clearer. This patch depends on: > >> > >> PCI/ACPI: Extend pci_mcfg_lookup() responsibilities > >> PCI/ACPI: Check platform-specific ECAM quirks > >> > >> These patches from Tomasz Nowicki were previously in your pci/ecam-v6 > >> branch, but that seems to have come and gone. How would you like to > >> proceed? > > > > Oh yes, that's right, I forgot that connection. I'm afraid I kind of > > dropped the ball on that thread, so I went back and read through it > > again. > > > > I *think* the current state is: > > > > - I'm OK with the first two patches that add the quirk > > infrastructure. > > > > - My issue with the last three patches that add ThunderX quirks is > > that there's no generic description of the ECAM address space. > > > > So if I understand correctly, your Qualcomm patch depends only on the > > first two patches. > > > > Then the question is how the Qualcomm ECAM address space is described. > > Your quirk overrides the default pci_generic_ecam_ops with the > > &pci_32b_ops, but it doesn't touch the address space part, so I assume > > the bus ranges and corresponding address space in your MCFG is > > correct. So far, so good. > > Qualcomm ECAM space includes both the root port and the endpoint address > space with a single contiguous 256 MB address space described in MCFG table. > There is no need to describe additional resources like PNP0C02. This is the crucial point I have failed to communicate clearly: the PNP0C02 resource is *always* required, even if the MCFG is correct. The reason is that MCFG is a PCI-specific table, and it should be possible to boot a kernel with no PCI support. That kernel will not look at the MCFG. The PCI hardware will still be present and will still consume the ECAM space, so the OS must be able to discover that the ECAM space is not available for other devices. The usual way to for the OS to discover that would be via the _CRS of a PNP0A03 or PNP0A08 host bridge device. _CRS is what I mean by a "generic" way to describe this address space, because the ACPI core can interpret _CRS for all ACPI devices, even if the kernel doesn't contain drivers for all of those devices. It turns out that we can't use the _CRS of host bridges because of the Producer/Consumer bit screwup [1]. So the fallback is to include the ECAM space in the _CRS of a PNP0C02 device. This is what the PCI Firmware spec r3.0, Table 4-2, footnote 2 is talking about. Bjorn [1] The original ACPI spec intent was that Consumer resources would be space like ECAM that is consumed directly by the bridge, and Producer resources would be the windows forwarded down to PCI. But BIOSes didn't use the Producer/Consumer bit consistently, so we have to assume that all resources in host bridge _CRS are windows, which leaves us no way to describe the Consumer resources.
On 11/3/2016 10:00 AM, Bjorn Helgaas wrote: > On Wed, Nov 02, 2016 at 12:36:16PM -0400, Sinan Kaya wrote: >> Hi Bjorn, >> >> On 11/2/2016 12:08 PM, Bjorn Helgaas wrote: >>> On Tue, Nov 01, 2016 at 07:06:31AM -0600, cov@codeaurora.org wrote: >>>> Hi Bjorn, >>>> >>>> On 2016-10-31 15:48, Bjorn Helgaas wrote: >>>>> On Wed, Sep 21, 2016 at 06:38:05PM -0400, Christopher Covington wrote: >>>>>> The Qualcomm Technologies QDF2432 SoC does not support accesses >>>>>> smaller >>>>>> than 32 bits to the PCI configuration space. Register the appropriate >>>>>> quirk. >>>>>> >>>>>> Signed-off-by: Christopher Covington <cov@codeaurora.org> >>>>> >>>>> Hi Christopher, >>>>> >>>>> Can you rebase this against v4.9-rc1? It no longer applies to my tree. >>>> >>>> I apologize for not being clearer. This patch depends on: >>>> >>>> PCI/ACPI: Extend pci_mcfg_lookup() responsibilities >>>> PCI/ACPI: Check platform-specific ECAM quirks >>>> >>>> These patches from Tomasz Nowicki were previously in your pci/ecam-v6 >>>> branch, but that seems to have come and gone. How would you like to >>>> proceed? >>> >>> Oh yes, that's right, I forgot that connection. I'm afraid I kind of >>> dropped the ball on that thread, so I went back and read through it >>> again. >>> >>> I *think* the current state is: >>> >>> - I'm OK with the first two patches that add the quirk >>> infrastructure. >>> >>> - My issue with the last three patches that add ThunderX quirks is >>> that there's no generic description of the ECAM address space. >>> >>> So if I understand correctly, your Qualcomm patch depends only on the >>> first two patches. >>> >>> Then the question is how the Qualcomm ECAM address space is described. >>> Your quirk overrides the default pci_generic_ecam_ops with the >>> &pci_32b_ops, but it doesn't touch the address space part, so I assume >>> the bus ranges and corresponding address space in your MCFG is >>> correct. So far, so good. >> >> Qualcomm ECAM space includes both the root port and the endpoint address >> space with a single contiguous 256 MB address space described in MCFG table. >> There is no need to describe additional resources like PNP0C02. > > This is the crucial point I have failed to communicate clearly: the > PNP0C02 resource is *always* required, even if the MCFG is correct. > Interesting... It looks like there is a lot of lessons learnt here from history. I think this requirement is only true if your system DDR space and PCIe space overlaps in the memory map. I understand that Intel systems allow sharing of these two memory ranges. An OS could potentially reclaim this address range. If there is no overlap and PCI is not enabled, there can't be any SW entity to reclaim this space. Did I miss something? > The reason is that MCFG is a PCI-specific table, and it should be > possible to boot a kernel with no PCI support. That kernel will not > look at the MCFG. The PCI hardware will still be present and will > still consume the ECAM space, so the OS must be able to discover that > the ECAM space is not available for other devices. > > The usual way to for the OS to discover that would be via the _CRS of > a PNP0A03 or PNP0A08 host bridge device. _CRS is what I mean by a > "generic" way to describe this address space, because the ACPI core > can interpret _CRS for all ACPI devices, even if the kernel doesn't > contain drivers for all of those devices. > > It turns out that we can't use the _CRS of host bridges because of the > Producer/Consumer bit screwup [1]. So the fallback is to include the > ECAM space in the _CRS of a PNP0C02 device. This is what the PCI > Firmware spec r3.0, Table 4-2, footnote 2 is talking about. > > Bjorn > > [1] The original ACPI spec intent was that Consumer resources would be > space like ECAM that is consumed directly by the bridge, and Producer > resources would be the windows forwarded down to PCI. But BIOSes > didn't use the Producer/Consumer bit consistently, so we have to > assume that all resources in host bridge _CRS are windows, which > leaves us no way to describe the Consumer resources. > -- > 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 11/3/2016 12:58 PM, Sinan Kaya wrote: >> > This is the crucial point I have failed to communicate clearly: the >> > PNP0C02 resource is *always* required, even if the MCFG is correct. >> > > Interesting... > > It looks like there is a lot of lessons learnt here from history. > > I think this requirement is only true if your system DDR space and PCIe > space overlaps in the memory map. I understand that Intel systems allow > sharing of these two memory ranges. An OS could potentially reclaim this > address range. > > If there is no overlap and PCI is not enabled, there can't be any SW entity > to reclaim this space. > > Did I miss something? > For protection, it makes sense to reserve this range. I'm trying to understand who would claim this range.
On Thu, Nov 03, 2016 at 12:58:10PM -0400, Sinan Kaya wrote: > > On 11/3/2016 10:00 AM, Bjorn Helgaas wrote: > > On Wed, Nov 02, 2016 at 12:36:16PM -0400, Sinan Kaya wrote: > >> Hi Bjorn, > >> > >> On 11/2/2016 12:08 PM, Bjorn Helgaas wrote: > >>> On Tue, Nov 01, 2016 at 07:06:31AM -0600, cov@codeaurora.org wrote: > >>>> Hi Bjorn, > >>>> > >>>> On 2016-10-31 15:48, Bjorn Helgaas wrote: > >>>>> On Wed, Sep 21, 2016 at 06:38:05PM -0400, Christopher Covington wrote: > >>>>>> The Qualcomm Technologies QDF2432 SoC does not support accesses > >>>>>> smaller > >>>>>> than 32 bits to the PCI configuration space. Register the appropriate > >>>>>> quirk. > >>>>>> > >>>>>> Signed-off-by: Christopher Covington <cov@codeaurora.org> > >>>>> > >>>>> Hi Christopher, > >>>>> > >>>>> Can you rebase this against v4.9-rc1? It no longer applies to my tree. > >>>> > >>>> I apologize for not being clearer. This patch depends on: > >>>> > >>>> PCI/ACPI: Extend pci_mcfg_lookup() responsibilities > >>>> PCI/ACPI: Check platform-specific ECAM quirks > >>>> > >>>> These patches from Tomasz Nowicki were previously in your pci/ecam-v6 > >>>> branch, but that seems to have come and gone. How would you like to > >>>> proceed? > >>> > >>> Oh yes, that's right, I forgot that connection. I'm afraid I kind of > >>> dropped the ball on that thread, so I went back and read through it > >>> again. > >>> > >>> I *think* the current state is: > >>> > >>> - I'm OK with the first two patches that add the quirk > >>> infrastructure. > >>> > >>> - My issue with the last three patches that add ThunderX quirks is > >>> that there's no generic description of the ECAM address space. > >>> > >>> So if I understand correctly, your Qualcomm patch depends only on the > >>> first two patches. > >>> > >>> Then the question is how the Qualcomm ECAM address space is described. > >>> Your quirk overrides the default pci_generic_ecam_ops with the > >>> &pci_32b_ops, but it doesn't touch the address space part, so I assume > >>> the bus ranges and corresponding address space in your MCFG is > >>> correct. So far, so good. > >> > >> Qualcomm ECAM space includes both the root port and the endpoint address > >> space with a single contiguous 256 MB address space described in MCFG table. > >> There is no need to describe additional resources like PNP0C02. > > > > This is the crucial point I have failed to communicate clearly: the > > PNP0C02 resource is *always* required, even if the MCFG is correct. > > > > Interesting... > > It looks like there is a lot of lessons learnt here from history. > > I think this requirement is only true if your system DDR space and PCIe > space overlaps in the memory map. I understand that Intel systems allow > sharing of these two memory ranges. An OS could potentially reclaim this > address range. > > If there is no overlap and PCI is not enabled, there can't be any SW entity > to reclaim this space. No, this isn't really anything to do with DDR/PCIe overlaps. This is just a fundamental part of the ACPI model: the firmware should communicate all address space usage to the OS either via ACPI or via standard self-describing mechanisms like PCI BARs. You can argue that this isn't "necessary", but that's an assumption based on your knowledge of this particular system, and we don't want the OS to have to make that assumption. For example, ACPI allows the hot-addition of new ACPI devices, and we may have to assign address space for them, and we don't want to collide with existing devices. Bjorn
On 11/3/2016 4:43 PM, Bjorn Helgaas wrote: > On Thu, Nov 03, 2016 at 12:58:10PM -0400, Sinan Kaya wrote: >> >> On 11/3/2016 10:00 AM, Bjorn Helgaas wrote: >>> On Wed, Nov 02, 2016 at 12:36:16PM -0400, Sinan Kaya wrote: >>>> Hi Bjorn, >>>> >>>> On 11/2/2016 12:08 PM, Bjorn Helgaas wrote: >>>>> On Tue, Nov 01, 2016 at 07:06:31AM -0600, cov@codeaurora.org wrote: >>>>>> Hi Bjorn, >>>>>> >>>>>> On 2016-10-31 15:48, Bjorn Helgaas wrote: >>>>>>> On Wed, Sep 21, 2016 at 06:38:05PM -0400, Christopher Covington wrote: >>>>>>>> The Qualcomm Technologies QDF2432 SoC does not support accesses >>>>>>>> smaller >>>>>>>> than 32 bits to the PCI configuration space. Register the appropriate >>>>>>>> quirk. >>>>>>>> >>>>>>>> Signed-off-by: Christopher Covington <cov@codeaurora.org> >>>>>>> >>>>>>> Hi Christopher, >>>>>>> >>>>>>> Can you rebase this against v4.9-rc1? It no longer applies to my tree. >>>>>> >>>>>> I apologize for not being clearer. This patch depends on: >>>>>> >>>>>> PCI/ACPI: Extend pci_mcfg_lookup() responsibilities >>>>>> PCI/ACPI: Check platform-specific ECAM quirks >>>>>> >>>>>> These patches from Tomasz Nowicki were previously in your pci/ecam-v6 >>>>>> branch, but that seems to have come and gone. How would you like to >>>>>> proceed? >>>>> >>>>> Oh yes, that's right, I forgot that connection. I'm afraid I kind of >>>>> dropped the ball on that thread, so I went back and read through it >>>>> again. >>>>> >>>>> I *think* the current state is: >>>>> >>>>> - I'm OK with the first two patches that add the quirk >>>>> infrastructure. >>>>> >>>>> - My issue with the last three patches that add ThunderX quirks is >>>>> that there's no generic description of the ECAM address space. >>>>> >>>>> So if I understand correctly, your Qualcomm patch depends only on the >>>>> first two patches. >>>>> >>>>> Then the question is how the Qualcomm ECAM address space is described. >>>>> Your quirk overrides the default pci_generic_ecam_ops with the >>>>> &pci_32b_ops, but it doesn't touch the address space part, so I assume >>>>> the bus ranges and corresponding address space in your MCFG is >>>>> correct. So far, so good. >>>> >>>> Qualcomm ECAM space includes both the root port and the endpoint address >>>> space with a single contiguous 256 MB address space described in MCFG table. >>>> There is no need to describe additional resources like PNP0C02. >>> >>> This is the crucial point I have failed to communicate clearly: the >>> PNP0C02 resource is *always* required, even if the MCFG is correct. >>> >> >> Interesting... >> >> It looks like there is a lot of lessons learnt here from history. >> >> I think this requirement is only true if your system DDR space and PCIe >> space overlaps in the memory map. I understand that Intel systems allow >> sharing of these two memory ranges. An OS could potentially reclaim this >> address range. >> >> If there is no overlap and PCI is not enabled, there can't be any SW entity >> to reclaim this space. > > No, this isn't really anything to do with DDR/PCIe overlaps. This is > just a fundamental part of the ACPI model: the firmware should > communicate all address space usage to the OS either via ACPI or via > standard self-describing mechanisms like PCI BARs. > > You can argue that this isn't "necessary", but that's an assumption > based on your knowledge of this particular system, and we don't want > the OS to have to make that assumption. For example, ACPI allows the > hot-addition of new ACPI devices, and we may have to assign address > space for them, and we don't want to collide with existing devices. Thanks for the description. > > 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 >
Hi Bjorn, On 11/02/2016 12:08 PM, Bjorn Helgaas wrote: > On Tue, Nov 01, 2016 at 07:06:31AM -0600, cov@codeaurora.org wrote: >> Hi Bjorn, >> >> On 2016-10-31 15:48, Bjorn Helgaas wrote: >>> On Wed, Sep 21, 2016 at 06:38:05PM -0400, Christopher Covington wrote: >>>> The Qualcomm Technologies QDF2432 SoC does not support accesses >>>> smaller >>>> than 32 bits to the PCI configuration space. Register the appropriate >>>> quirk. >>>> >>>> Signed-off-by: Christopher Covington <cov@codeaurora.org> >>> >>> Hi Christopher, >>> >>> Can you rebase this against v4.9-rc1? It no longer applies to my tree. >> >> I apologize for not being clearer. This patch depends on: >> >> PCI/ACPI: Extend pci_mcfg_lookup() responsibilities >> PCI/ACPI: Check platform-specific ECAM quirks >> >> These patches from Tomasz Nowicki were previously in your pci/ecam-v6 >> branch, but that seems to have come and gone. How would you like to >> proceed? > > Oh yes, that's right, I forgot that connection. I'm afraid I kind of > dropped the ball on that thread, so I went back and read through it > again. > > I *think* the current state is: > > - I'm OK with the first two patches that add the quirk > infrastructure. > > - My issue with the last three patches that add ThunderX quirks is > that there's no generic description of the ECAM address space. > > So if I understand correctly, your Qualcomm patch depends only on the > first two patches. > > Then the question is how the Qualcomm ECAM address space is described. > Your quirk overrides the default pci_generic_ecam_ops with the > &pci_32b_ops, but it doesn't touch the address space part, so I assume > the bus ranges and corresponding address space in your MCFG is > correct. So far, so good. > > Is there also an ACPI device that contains that space in _CRS? I > think we concluded that the standard solution is to describe this with > a PNP0C02 device. > > Would you mind opening a bugzilla at bugzilla.kernel.org and attaching > the dmesg log, /proc/iomem, and maybe a DSDT dump? I'd like to have > something to point at to say "if you need an MCFG quirk, you need the > MCFG bit and *also* these other related ACPI device bits, and here's > how it should be done." We're working to add the PNP0C02 resource to future firmware, but it's not in the current firmware. Are dmesg and /proc/iomem from the current firmware interesting or should we wait for the update to file? Thanks, Cov
On Wed, Nov 09, 2016 at 02:25:56PM -0500, Christopher Covington wrote: > Hi Bjorn, > > On 11/02/2016 12:08 PM, Bjorn Helgaas wrote: > > On Tue, Nov 01, 2016 at 07:06:31AM -0600, cov@codeaurora.org wrote: > >> Hi Bjorn, > >> > >> On 2016-10-31 15:48, Bjorn Helgaas wrote: > >>> On Wed, Sep 21, 2016 at 06:38:05PM -0400, Christopher Covington wrote: > >>>> The Qualcomm Technologies QDF2432 SoC does not support accesses > >>>> smaller > >>>> than 32 bits to the PCI configuration space. Register the appropriate > >>>> quirk. > >>>> > >>>> Signed-off-by: Christopher Covington <cov@codeaurora.org> > >>> > >>> Hi Christopher, > >>> > >>> Can you rebase this against v4.9-rc1? It no longer applies to my tree. > >> > >> I apologize for not being clearer. This patch depends on: > >> > >> PCI/ACPI: Extend pci_mcfg_lookup() responsibilities > >> PCI/ACPI: Check platform-specific ECAM quirks > >> > >> These patches from Tomasz Nowicki were previously in your pci/ecam-v6 > >> branch, but that seems to have come and gone. How would you like to > >> proceed? > > > > Oh yes, that's right, I forgot that connection. I'm afraid I kind of > > dropped the ball on that thread, so I went back and read through it > > again. > > > > I *think* the current state is: > > > > - I'm OK with the first two patches that add the quirk > > infrastructure. > > > > - My issue with the last three patches that add ThunderX quirks is > > that there's no generic description of the ECAM address space. > > > > So if I understand correctly, your Qualcomm patch depends only on the > > first two patches. > > > > Then the question is how the Qualcomm ECAM address space is described. > > Your quirk overrides the default pci_generic_ecam_ops with the > > &pci_32b_ops, but it doesn't touch the address space part, so I assume > > the bus ranges and corresponding address space in your MCFG is > > correct. So far, so good. > > > > Is there also an ACPI device that contains that space in _CRS? I > > think we concluded that the standard solution is to describe this with > > a PNP0C02 device. > > > > Would you mind opening a bugzilla at bugzilla.kernel.org and attaching > > the dmesg log, /proc/iomem, and maybe a DSDT dump? I'd like to have > > something to point at to say "if you need an MCFG quirk, you need the > > MCFG bit and *also* these other related ACPI device bits, and here's > > how it should be done." > > We're working to add the PNP0C02 resource to future firmware, but it's > not in the current firmware. Are dmesg and /proc/iomem from the > current firmware interesting or should we wait for the update to file? Note that the ECAM space is not the only thing that should be described via these PNP0C02 devices. *All* non-enumerable resources should be described by the _CRS method of some ACPI device. Here's a sample from my laptop: PCI: MMCONFIG for domain 0000 [bus 00-3f] at [mem 0xf8000000-0xfbffffff] (base 0xf8000000) system 00:01: [io 0x1800-0x189f] could not be reserved system 00:01: [io 0x0800-0x087f] has been reserved system 00:01: [io 0x0880-0x08ff] has been reserved system 00:01: [io 0x0900-0x097f] has been reserved system 00:01: [io 0x0980-0x09ff] has been reserved system 00:01: [io 0x0a00-0x0a7f] has been reserved system 00:01: [io 0x0a80-0x0aff] has been reserved system 00:01: [io 0x0b00-0x0b7f] has been reserved system 00:01: [io 0x0b80-0x0bff] has been reserved system 00:01: [io 0x15e0-0x15ef] has been reserved system 00:01: [io 0x1600-0x167f] has been reserved system 00:01: [io 0x1640-0x165f] has been reserved system 00:01: [mem 0xf8000000-0xfbffffff] could not be reserved system 00:01: [mem 0xfed10000-0xfed13fff] has been reserved system 00:01: [mem 0xfed18000-0xfed18fff] has been reserved system 00:01: [mem 0xfed19000-0xfed19fff] has been reserved system 00:01: [mem 0xfeb00000-0xfebfffff] has been reserved system 00:01: [mem 0xfed20000-0xfed3ffff] has been reserved system 00:01: [mem 0xfed90000-0xfed93fff] has been reserved system 00:01: [mem 0xf7fe0000-0xf7ffffff] has been reserved system 00:01: Plug and Play ACPI device, IDs PNP0c02 (active) Do you have firmware in the field that may not get updated? If so, I'd like to see the whole solution for that firmware, including the MCFG quirk (which tells the PCI core where the ECAM region is) and whatever PNP0C02 quirk you figure out to actually reserve the region. I proposed a PNP0C02 quirk to Duc along these lines of the below. I don't actually know if it's feasible, but it didn't look as bad as I expected, so I'd kind of like somebody to try it out. I think you would have to call this via a DMI hook (do you have DMI on arm64?), maybe from pnp_init() or similar. struct pnp_protocol pnpquirk_protocol = { .name = "Plug and Play Quirks", }; void quirk() { struct pnp_dev *dev; struct resource res; ret = pnp_register_protocol(&pnpquirk_protocol); if (ret) return; dev = pnp_alloc_dev(&pnpquirk_protocol, 0, "PNP0C02"); if (!dev) return; res.start = XX; /* ECAM start */ res.end = YY; /* ECAM end */ res.flags = IORESOURCE_MEM; pnp_add_resource(dev, &res); dev->active = 1; pnp_add_device(dev); dev_info(&dev->dev, "fabricated device to reserve ECAM space %pR\n", &res); } Bjorn
Hi Bjorn, On 9 November 2016 at 20:06, Bjorn Helgaas <helgaas@kernel.org> wrote: > On Wed, Nov 09, 2016 at 02:25:56PM -0500, Christopher Covington wrote: >> Hi Bjorn, >> [...] >> >> We're working to add the PNP0C02 resource to future firmware, but it's >> not in the current firmware. Are dmesg and /proc/iomem from the >> current firmware interesting or should we wait for the update to file? > > Note that the ECAM space is not the only thing that should be > described via these PNP0C02 devices. *All* non-enumerable resources > should be described by the _CRS method of some ACPI device. Here's a > sample from my laptop: > > PCI: MMCONFIG for domain 0000 [bus 00-3f] at [mem 0xf8000000-0xfbffffff] (base 0xf8000000) > system 00:01: [io 0x1800-0x189f] could not be reserved > system 00:01: [io 0x0800-0x087f] has been reserved > system 00:01: [io 0x0880-0x08ff] has been reserved > system 00:01: [io 0x0900-0x097f] has been reserved > system 00:01: [io 0x0980-0x09ff] has been reserved > system 00:01: [io 0x0a00-0x0a7f] has been reserved > system 00:01: [io 0x0a80-0x0aff] has been reserved > system 00:01: [io 0x0b00-0x0b7f] has been reserved > system 00:01: [io 0x0b80-0x0bff] has been reserved > system 00:01: [io 0x15e0-0x15ef] has been reserved > system 00:01: [io 0x1600-0x167f] has been reserved > system 00:01: [io 0x1640-0x165f] has been reserved > system 00:01: [mem 0xf8000000-0xfbffffff] could not be reserved > system 00:01: [mem 0xfed10000-0xfed13fff] has been reserved > system 00:01: [mem 0xfed18000-0xfed18fff] has been reserved > system 00:01: [mem 0xfed19000-0xfed19fff] has been reserved > system 00:01: [mem 0xfeb00000-0xfebfffff] has been reserved > system 00:01: [mem 0xfed20000-0xfed3ffff] has been reserved > system 00:01: [mem 0xfed90000-0xfed93fff] has been reserved > system 00:01: [mem 0xf7fe0000-0xf7ffffff] has been reserved > system 00:01: Plug and Play ACPI device, IDs PNP0c02 (active) > > Do you have firmware in the field that may not get updated? If so, > I'd like to see the whole solution for that firmware, including the > MCFG quirk (which tells the PCI core where the ECAM region is) and > whatever PNP0C02 quirk you figure out to actually reserve the region. > > I proposed a PNP0C02 quirk to Duc along these lines of the below. I > don't actually know if it's feasible, but it didn't look as bad as I > expected, so I'd kind of like somebody to try it out. I think you > would have to call this via a DMI hook (do you have DMI on arm64?), > maybe from pnp_init() or similar. > We do have SMBIOS/DMI on arm64, but we have been successful so far not to rely on it for quirks, and we'd very much like to keep it that way. Since this ACPI _CRS method has nothing to do with SMBIOS/DMI, surely there is a better way to wire up the reservation code to the information exposed by ACPI?
On Wed, Nov 09, 2016 at 08:29:23PM +0000, Ard Biesheuvel wrote: > Hi Bjorn, > > On 9 November 2016 at 20:06, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Wed, Nov 09, 2016 at 02:25:56PM -0500, Christopher Covington wrote: > >> Hi Bjorn, > >> > [...] > >> > >> We're working to add the PNP0C02 resource to future firmware, but it's > >> not in the current firmware. Are dmesg and /proc/iomem from the > >> current firmware interesting or should we wait for the update to file? > > > > Note that the ECAM space is not the only thing that should be > > described via these PNP0C02 devices. *All* non-enumerable resources > > should be described by the _CRS method of some ACPI device. Here's a > > sample from my laptop: > > > > PCI: MMCONFIG for domain 0000 [bus 00-3f] at [mem 0xf8000000-0xfbffffff] (base 0xf8000000) > > system 00:01: [io 0x1800-0x189f] could not be reserved > > system 00:01: [io 0x0800-0x087f] has been reserved > > system 00:01: [io 0x0880-0x08ff] has been reserved > > system 00:01: [io 0x0900-0x097f] has been reserved > > system 00:01: [io 0x0980-0x09ff] has been reserved > > system 00:01: [io 0x0a00-0x0a7f] has been reserved > > system 00:01: [io 0x0a80-0x0aff] has been reserved > > system 00:01: [io 0x0b00-0x0b7f] has been reserved > > system 00:01: [io 0x0b80-0x0bff] has been reserved > > system 00:01: [io 0x15e0-0x15ef] has been reserved > > system 00:01: [io 0x1600-0x167f] has been reserved > > system 00:01: [io 0x1640-0x165f] has been reserved > > system 00:01: [mem 0xf8000000-0xfbffffff] could not be reserved > > system 00:01: [mem 0xfed10000-0xfed13fff] has been reserved > > system 00:01: [mem 0xfed18000-0xfed18fff] has been reserved > > system 00:01: [mem 0xfed19000-0xfed19fff] has been reserved > > system 00:01: [mem 0xfeb00000-0xfebfffff] has been reserved > > system 00:01: [mem 0xfed20000-0xfed3ffff] has been reserved > > system 00:01: [mem 0xfed90000-0xfed93fff] has been reserved > > system 00:01: [mem 0xf7fe0000-0xf7ffffff] has been reserved > > system 00:01: Plug and Play ACPI device, IDs PNP0c02 (active) > > > > Do you have firmware in the field that may not get updated? If so, > > I'd like to see the whole solution for that firmware, including the > > MCFG quirk (which tells the PCI core where the ECAM region is) and > > whatever PNP0C02 quirk you figure out to actually reserve the region. > > > > I proposed a PNP0C02 quirk to Duc along these lines of the below. I > > don't actually know if it's feasible, but it didn't look as bad as I > > expected, so I'd kind of like somebody to try it out. I think you > > would have to call this via a DMI hook (do you have DMI on arm64?), > > maybe from pnp_init() or similar. > > We do have SMBIOS/DMI on arm64, but we have been successful so far not > to rely on it for quirks, and we'd very much like to keep it that way. > > Since this ACPI _CRS method has nothing to do with SMBIOS/DMI, surely > there is a better way to wire up the reservation code to the > information exposed by ACPI? I'm open to other ways, feel free to propose one :) If you do a quirk, you need some way to identify the machine/firmware combination, because you don't want to apply the quirk on every machine. You're trying to work around a firmware issue, so you probably want something tied to the firmware version. On x86, that's typically done with DMI. Bjorn
On 10 November 2016 at 06:49, Bjorn Helgaas <helgaas@kernel.org> wrote: > On Wed, Nov 09, 2016 at 08:29:23PM +0000, Ard Biesheuvel wrote: >> Hi Bjorn, >> >> On 9 November 2016 at 20:06, Bjorn Helgaas <helgaas@kernel.org> wrote: >> > On Wed, Nov 09, 2016 at 02:25:56PM -0500, Christopher Covington wrote: >> >> Hi Bjorn, >> >> >> [...] >> >> >> >> We're working to add the PNP0C02 resource to future firmware, but it's >> >> not in the current firmware. Are dmesg and /proc/iomem from the >> >> current firmware interesting or should we wait for the update to file? >> > >> > Note that the ECAM space is not the only thing that should be >> > described via these PNP0C02 devices. *All* non-enumerable resources >> > should be described by the _CRS method of some ACPI device. Here's a >> > sample from my laptop: >> > >> > PCI: MMCONFIG for domain 0000 [bus 00-3f] at [mem 0xf8000000-0xfbffffff] (base 0xf8000000) >> > system 00:01: [io 0x1800-0x189f] could not be reserved >> > system 00:01: [io 0x0800-0x087f] has been reserved >> > system 00:01: [io 0x0880-0x08ff] has been reserved >> > system 00:01: [io 0x0900-0x097f] has been reserved >> > system 00:01: [io 0x0980-0x09ff] has been reserved >> > system 00:01: [io 0x0a00-0x0a7f] has been reserved >> > system 00:01: [io 0x0a80-0x0aff] has been reserved >> > system 00:01: [io 0x0b00-0x0b7f] has been reserved >> > system 00:01: [io 0x0b80-0x0bff] has been reserved >> > system 00:01: [io 0x15e0-0x15ef] has been reserved >> > system 00:01: [io 0x1600-0x167f] has been reserved >> > system 00:01: [io 0x1640-0x165f] has been reserved >> > system 00:01: [mem 0xf8000000-0xfbffffff] could not be reserved >> > system 00:01: [mem 0xfed10000-0xfed13fff] has been reserved >> > system 00:01: [mem 0xfed18000-0xfed18fff] has been reserved >> > system 00:01: [mem 0xfed19000-0xfed19fff] has been reserved >> > system 00:01: [mem 0xfeb00000-0xfebfffff] has been reserved >> > system 00:01: [mem 0xfed20000-0xfed3ffff] has been reserved >> > system 00:01: [mem 0xfed90000-0xfed93fff] has been reserved >> > system 00:01: [mem 0xf7fe0000-0xf7ffffff] has been reserved >> > system 00:01: Plug and Play ACPI device, IDs PNP0c02 (active) >> > >> > Do you have firmware in the field that may not get updated? If so, >> > I'd like to see the whole solution for that firmware, including the >> > MCFG quirk (which tells the PCI core where the ECAM region is) and >> > whatever PNP0C02 quirk you figure out to actually reserve the region. >> > >> > I proposed a PNP0C02 quirk to Duc along these lines of the below. I >> > don't actually know if it's feasible, but it didn't look as bad as I >> > expected, so I'd kind of like somebody to try it out. I think you >> > would have to call this via a DMI hook (do you have DMI on arm64?), >> > maybe from pnp_init() or similar. >> >> We do have SMBIOS/DMI on arm64, but we have been successful so far not >> to rely on it for quirks, and we'd very much like to keep it that way. >> >> Since this ACPI _CRS method has nothing to do with SMBIOS/DMI, surely >> there is a better way to wire up the reservation code to the >> information exposed by ACPI? > > I'm open to other ways, feel free to propose one :) > > If you do a quirk, you need some way to identify the machine/firmware > combination, because you don't want to apply the quirk on every > machine. You're trying to work around a firmware issue, so you > probably want something tied to the firmware version. On x86, that's > typically done with DMI. > I think I misunderstood the purpose of the example: that should only be necessary if the _CRS methods are missing from the firmware, right? If we update the firmware to cover all non-enumerable resources by such a method, we shouldn't need any such quirks at all IIUC
On Thu, Nov 10, 2016 at 06:25:16PM +0800, Ard Biesheuvel wrote: > On 10 November 2016 at 06:49, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Wed, Nov 09, 2016 at 08:29:23PM +0000, Ard Biesheuvel wrote: > >> Hi Bjorn, > >> > >> On 9 November 2016 at 20:06, Bjorn Helgaas <helgaas@kernel.org> wrote: > >> > On Wed, Nov 09, 2016 at 02:25:56PM -0500, Christopher Covington wrote: > >> >> Hi Bjorn, > >> >> > >> [...] > >> >> > >> >> We're working to add the PNP0C02 resource to future firmware, but it's > >> >> not in the current firmware. Are dmesg and /proc/iomem from the > >> >> current firmware interesting or should we wait for the update to file? > >> > > >> > Note that the ECAM space is not the only thing that should be > >> > described via these PNP0C02 devices. *All* non-enumerable resources > >> > should be described by the _CRS method of some ACPI device. Here's a > >> > sample from my laptop: > >> > > >> > PCI: MMCONFIG for domain 0000 [bus 00-3f] at [mem 0xf8000000-0xfbffffff] (base 0xf8000000) > >> > system 00:01: [io 0x1800-0x189f] could not be reserved > >> > system 00:01: [io 0x0800-0x087f] has been reserved > >> > system 00:01: [io 0x0880-0x08ff] has been reserved > >> > system 00:01: [io 0x0900-0x097f] has been reserved > >> > system 00:01: [io 0x0980-0x09ff] has been reserved > >> > system 00:01: [io 0x0a00-0x0a7f] has been reserved > >> > system 00:01: [io 0x0a80-0x0aff] has been reserved > >> > system 00:01: [io 0x0b00-0x0b7f] has been reserved > >> > system 00:01: [io 0x0b80-0x0bff] has been reserved > >> > system 00:01: [io 0x15e0-0x15ef] has been reserved > >> > system 00:01: [io 0x1600-0x167f] has been reserved > >> > system 00:01: [io 0x1640-0x165f] has been reserved > >> > system 00:01: [mem 0xf8000000-0xfbffffff] could not be reserved > >> > system 00:01: [mem 0xfed10000-0xfed13fff] has been reserved > >> > system 00:01: [mem 0xfed18000-0xfed18fff] has been reserved > >> > system 00:01: [mem 0xfed19000-0xfed19fff] has been reserved > >> > system 00:01: [mem 0xfeb00000-0xfebfffff] has been reserved > >> > system 00:01: [mem 0xfed20000-0xfed3ffff] has been reserved > >> > system 00:01: [mem 0xfed90000-0xfed93fff] has been reserved > >> > system 00:01: [mem 0xf7fe0000-0xf7ffffff] has been reserved > >> > system 00:01: Plug and Play ACPI device, IDs PNP0c02 (active) > >> > > >> > Do you have firmware in the field that may not get updated? If so, > >> > I'd like to see the whole solution for that firmware, including the > >> > MCFG quirk (which tells the PCI core where the ECAM region is) and > >> > whatever PNP0C02 quirk you figure out to actually reserve the region. > >> > > >> > I proposed a PNP0C02 quirk to Duc along these lines of the below. I > >> > don't actually know if it's feasible, but it didn't look as bad as I > >> > expected, so I'd kind of like somebody to try it out. I think you > >> > would have to call this via a DMI hook (do you have DMI on arm64?), > >> > maybe from pnp_init() or similar. > >> > >> We do have SMBIOS/DMI on arm64, but we have been successful so far not > >> to rely on it for quirks, and we'd very much like to keep it that way. > >> > >> Since this ACPI _CRS method has nothing to do with SMBIOS/DMI, surely > >> there is a better way to wire up the reservation code to the > >> information exposed by ACPI? > > > > I'm open to other ways, feel free to propose one :) > > > > If you do a quirk, you need some way to identify the machine/firmware > > combination, because you don't want to apply the quirk on every > > machine. You're trying to work around a firmware issue, so you > > probably want something tied to the firmware version. On x86, that's > > typically done with DMI. > > > > I think I misunderstood the purpose of the example: that should only > be necessary if the _CRS methods are missing from the firmware, right? > If we update the firmware to cover all non-enumerable resources by > such a method, we shouldn't need any such quirks at all IIUC Yes that's correct you need a quirk to fabricate a PNP0c02 motherboard resource if it is not present in FW. Lorenzo
On Thu, Nov 10, 2016 at 06:25:16PM +0800, Ard Biesheuvel wrote: > On 10 November 2016 at 06:49, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Wed, Nov 09, 2016 at 08:29:23PM +0000, Ard Biesheuvel wrote: > >> Hi Bjorn, > >> > >> On 9 November 2016 at 20:06, Bjorn Helgaas <helgaas@kernel.org> wrote: > >> > On Wed, Nov 09, 2016 at 02:25:56PM -0500, Christopher Covington wrote: > >> >> Hi Bjorn, > >> >> > >> [...] > >> >> > >> >> We're working to add the PNP0C02 resource to future firmware, but it's > >> >> not in the current firmware. Are dmesg and /proc/iomem from the > >> >> current firmware interesting or should we wait for the update to file? > >> > > >> > Note that the ECAM space is not the only thing that should be > >> > described via these PNP0C02 devices. *All* non-enumerable resources > >> > should be described by the _CRS method of some ACPI device. Here's a > >> > sample from my laptop: > >> > > >> > PCI: MMCONFIG for domain 0000 [bus 00-3f] at [mem 0xf8000000-0xfbffffff] (base 0xf8000000) > >> > system 00:01: [io 0x1800-0x189f] could not be reserved > >> > system 00:01: [io 0x0800-0x087f] has been reserved > >> > system 00:01: [io 0x0880-0x08ff] has been reserved > >> > system 00:01: [io 0x0900-0x097f] has been reserved > >> > system 00:01: [io 0x0980-0x09ff] has been reserved > >> > system 00:01: [io 0x0a00-0x0a7f] has been reserved > >> > system 00:01: [io 0x0a80-0x0aff] has been reserved > >> > system 00:01: [io 0x0b00-0x0b7f] has been reserved > >> > system 00:01: [io 0x0b80-0x0bff] has been reserved > >> > system 00:01: [io 0x15e0-0x15ef] has been reserved > >> > system 00:01: [io 0x1600-0x167f] has been reserved > >> > system 00:01: [io 0x1640-0x165f] has been reserved > >> > system 00:01: [mem 0xf8000000-0xfbffffff] could not be reserved > >> > system 00:01: [mem 0xfed10000-0xfed13fff] has been reserved > >> > system 00:01: [mem 0xfed18000-0xfed18fff] has been reserved > >> > system 00:01: [mem 0xfed19000-0xfed19fff] has been reserved > >> > system 00:01: [mem 0xfeb00000-0xfebfffff] has been reserved > >> > system 00:01: [mem 0xfed20000-0xfed3ffff] has been reserved > >> > system 00:01: [mem 0xfed90000-0xfed93fff] has been reserved > >> > system 00:01: [mem 0xf7fe0000-0xf7ffffff] has been reserved > >> > system 00:01: Plug and Play ACPI device, IDs PNP0c02 (active) > >> > > >> > Do you have firmware in the field that may not get updated? If so, > >> > I'd like to see the whole solution for that firmware, including the > >> > MCFG quirk (which tells the PCI core where the ECAM region is) and > >> > whatever PNP0C02 quirk you figure out to actually reserve the region. > >> > > >> > I proposed a PNP0C02 quirk to Duc along these lines of the below. I > >> > don't actually know if it's feasible, but it didn't look as bad as I > >> > expected, so I'd kind of like somebody to try it out. I think you > >> > would have to call this via a DMI hook (do you have DMI on arm64?), > >> > maybe from pnp_init() or similar. > >> > >> We do have SMBIOS/DMI on arm64, but we have been successful so far not > >> to rely on it for quirks, and we'd very much like to keep it that way. > >> > >> Since this ACPI _CRS method has nothing to do with SMBIOS/DMI, surely > >> there is a better way to wire up the reservation code to the > >> information exposed by ACPI? > > > > I'm open to other ways, feel free to propose one :) > > > > If you do a quirk, you need some way to identify the machine/firmware > > combination, because you don't want to apply the quirk on every > > machine. You're trying to work around a firmware issue, so you > > probably want something tied to the firmware version. On x86, that's > > typically done with DMI. > > > > I think I misunderstood the purpose of the example: that should only > be necessary if the _CRS methods are missing from the firmware, right? > If we update the firmware to cover all non-enumerable resources by > such a method, we shouldn't need any such quirks at all IIUC Right: if the firmware provides a PNP0C02 device with _CRS that includes the ECAM area, we don't need any PNP/ACPI quirks. We will still need the MCFG quirks since the hardware doesn't fully support ECAM. For the PNP/ACPI quirks, there are two interesting cases: 1) Firmware provides a PNP0C02 device, but its _CRS doesn't include the ECAM space, and 2) Firmware doesn't provide a PNP0C02 device at all. For case 1, we could consider adding the ECAM space to the existing device. This is essentially what quirk_amd_mmconfig_area() does. For case 2, we would have to fabricate the PNP0C02 device itself, then add the ECAM space to it. I don't think there's any existing code that does this, so this is what the example I proposed in this thread does. One could argue that it might be cleaner to use case 2 instead of the case 1 approach because it avoids mucking with an ACPI device from firmware. For devices that support _SRS, case 1 might break things because _CRS and _SRS are supposed to use the same resource descriptor buffer, and if we add resources the firmware doesn't know about, I don't think we'll encode the _SRS buffer correctly. But this is only a theoretical risk because we basically never use _SRS today. In either case, there has to be a mechanism to do the quirk only on the machine/firmware that needs it, of course. Bjorn
On 11/03/2016 10:00 AM, Bjorn Helgaas wrote: > It turns out that we can't use the _CRS of host bridges because of the > Producer/Consumer bit screwup [1]. So the fallback is to include the > ECAM space in the _CRS of a PNP0C02 device. This is what the PCI > Firmware spec r3.0, Table 4-2, footnote 2 is talking about. > > Bjorn > > [1] The original ACPI spec intent was that Consumer resources would be > space like ECAM that is consumed directly by the bridge, and Producer > resources would be the windows forwarded down to PCI. But BIOSes > didn't use the Producer/Consumer bit consistently, so we have to > assume that all resources in host bridge _CRS are windows, which > leaves us no way to describe the Consumer resources. Aside - and now I realize you'd called this out as recently as last month. Alas the HPE m400 I reference on the other thread about the APM quirks doesn't have the motherboard resource entry so we're stuck with exactly the situation you describe above there. Jon.
On 11/10/2016 12:42 PM, Bjorn Helgaas wrote: > For the PNP/ACPI quirks, there are two interesting cases: > > 1) Firmware provides a PNP0C02 device, but its _CRS doesn't include > the ECAM space, and > > 2) Firmware doesn't provide a PNP0C02 device at all. > > For case 1, we could consider adding the ECAM space to the existing > device. This is essentially what quirk_amd_mmconfig_area() does. > > For case 2, we would have to fabricate the PNP0C02 device itself, then > add the ECAM space to it. I don't think there's any existing code > that does this, so this is what the example I proposed in this thread > does. (this isn't QCOM/QDT specific) We'll go scrub for examples where there are systems missing the motherboard resource and get firmware fixed. As an example, I know that HPE ProLiant m400 (Moonshot) will need to be updated. It would probably be easier to just get the firmware fixed to add this than to introduce the first DMI quirk for this one. Ard and others very reasonably want to avoid DMI quirks on arm64. I take responsibility for being the guilty party that wrote SMBIOS/DMI into the SBBR originally as a means of keeping this failsafe for the future and because "that's what x86 does, so people will expect it". But we'll save that for a nasty situation further down the road. We are still working on getting vendors (other than QCOM and HPE, who have had this right since the beginning) to release firmware other than version "1.0" every time. That's always a good start ;) Jon.
diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c index 245b79f..212334f 100644 --- a/drivers/acpi/pci_mcfg.c +++ b/drivers/acpi/pci_mcfg.c @@ -96,6 +96,14 @@ static struct mcfg_fixup mcfg_quirks[] = { THUNDER_ECAM_MCFG(2, 12), THUNDER_ECAM_MCFG(2, 13), #endif + { "QCOM ", "QDF2432 ", 1, 0, MCFG_BUS_ANY, &pci_32b_ops }, + { "QCOM ", "QDF2432 ", 1, 1, MCFG_BUS_ANY, &pci_32b_ops }, + { "QCOM ", "QDF2432 ", 1, 2, MCFG_BUS_ANY, &pci_32b_ops }, + { "QCOM ", "QDF2432 ", 1, 3, MCFG_BUS_ANY, &pci_32b_ops }, + { "QCOM ", "QDF2432 ", 1, 4, MCFG_BUS_ANY, &pci_32b_ops }, + { "QCOM ", "QDF2432 ", 1, 5, MCFG_BUS_ANY, &pci_32b_ops }, + { "QCOM ", "QDF2432 ", 1, 6, MCFG_BUS_ANY, &pci_32b_ops }, + { "QCOM ", "QDF2432 ", 1, 7, MCFG_BUS_ANY, &pci_32b_ops }, }; static char mcfg_oem_id[ACPI_OEM_ID_SIZE]; diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c index 43ed08d..c3b3063 100644 --- a/drivers/pci/ecam.c +++ b/drivers/pci/ecam.c @@ -162,3 +162,13 @@ struct pci_ecam_ops pci_generic_ecam_ops = { .write = pci_generic_config_write, } }; + +/* ops for 32 bit config space access quirk */ +struct pci_ecam_ops pci_32b_ops = { + .bus_shift = 20, + .pci_ops = { + .map_bus = pci_ecam_map_bus, + .read = pci_generic_config_read32, + .write = pci_generic_config_write32, + } +}; diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h index 35f0e81..a6cffb8 100644 --- a/include/linux/pci-ecam.h +++ b/include/linux/pci-ecam.h @@ -65,6 +65,7 @@ extern struct pci_ecam_ops pci_thunder_pem_ops; #ifdef CONFIG_PCI_HOST_THUNDER_ECAM extern struct pci_ecam_ops pci_thunder_ecam_ops; #endif +extern struct pci_ecam_ops pci_32b_ops; #ifdef CONFIG_PCI_HOST_GENERIC /* for DT-based PCI controllers that support ECAM */
The Qualcomm Technologies QDF2432 SoC does not support accesses smaller than 32 bits to the PCI configuration space. Register the appropriate quirk. Signed-off-by: Christopher Covington <cov@codeaurora.org> --- drivers/acpi/pci_mcfg.c | 8 ++++++++ drivers/pci/ecam.c | 10 ++++++++++ include/linux/pci-ecam.h | 1 + 3 files changed, 19 insertions(+)