From: Gabriele Paoloni <gabriele.paoloni@huawei.com> To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>, Christopher Covington <cov@codeaurora.org> Cc: Tomasz Nowicki <tn@semihalf.com>, Duc Dang <dhdang@apm.com>, "liudongdong (C)" <liudongdong3@huawei.com>, Sinan Kaya <okaya@codeaurora.org>, Jeff Hugo <jhugo@codeaurora.org>, Jon Masters <jcm@redhat.com>, Mark Salter <msalter@redhat.com>, Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>, Jayachandran C <jchandra@broadcom.com>, David Daney <ddaney@caviumnetworks.com>, Robert Richter <robert.richter@caviumnetworks.com>, Hanjun Guo <hanjun.guo@linaro.org>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will.deacon@arm.com>, Bjorn Helgaas <bhelgaas@google.com>, Ganapatrao Kulkarni <gkulkarni@caviumnetworks.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org> Subject: RE: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller Date: Fri, 17 Jun 2016 08:01:43 +0000 [thread overview] Message-ID: <EE11001F9E5DDD47B7634E2F8A612F2E1F790346@lhreml507-mbx> (raw) In-Reply-To: <20160616174856.GB32388@red-moon> Hi Lorenzo and All > -----Original Message----- > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com] > Sent: 16 June 2016 18:49 > To: Christopher Covington > Cc: Tomasz Nowicki; Duc Dang; liudongdong (C); Sinan Kaya; Jeff Hugo; > Gabriele Paoloni; Jon Masters; Mark Salter; Suravee Suthikulpanit; > Jayachandran C; David Daney; Robert Richter; Hanjun Guo; linux-arm- > kernel@lists.infradead.org; Catalin Marinas; Will Deacon; Bjorn Helgaas; > Ganapatrao Kulkarni; linux-kernel@vger.kernel.org > Subject: Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling > for ACPI based PCI host controller > > On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote: > > From: Tomasz Nowicki <tn@semihalf.com> > > > > pci_generic_ecam_ops is used by default. Since there are platforms > > which have non-compliant ECAM space we need to overwrite these > > accessors prior to PCI buses enumeration. In order to do that > > we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that > > we can use proper PCI config space accessors and bus_shift. > > > > pci_generic_ecam_ops is still used for platforms free from quirks. > > > > Signed-off-by: Tomasz Nowicki <tn@semihalf.com> > > --- > > arch/arm64/kernel/pci.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c > > index 94cd43c..a891bda 100644 > > --- a/arch/arm64/kernel/pci.c > > +++ b/arch/arm64/kernel/pci.c > > @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root > *root) > > struct pci_config_window *cfg; > > struct resource cfgres; > > unsigned int bsz; > > + struct pci_ecam_ops *ops; > > > > /* Use address from _CBA if present, otherwise lookup MCFG */ > > if (!root->mcfg_addr) > > @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct > acpi_pci_root *root) > > return NULL; > > } > > > > - bsz = 1 << pci_generic_ecam_ops.bus_shift; > > + ops = pci_mcfg_get_ops(root); > > + bsz = 1 << ops->bus_shift; > > cfgres.start = root->mcfg_addr + bus_res->start * bsz; > > cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1; > > cfgres.flags = IORESOURCE_MEM; > > - cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res, > > - &pci_generic_ecam_ops); > > + cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res, ops); > > Arnd pointed this out already, I think that's the only pending question > here. > > pci_ecam_create() maps ECAM space for config regions retrieved from > the MCFG, which are *supposed* to be ECAM compliant. > > Do we think that's *always* correct/safe regardless of the kind > of quirk we are currently fixing up ? I didn't dig into the other vendors' quirk mechanism but I will quickly explain what we (would like to) do in HiSilicon Hip05/Hip06 SoCs. >From our perspective we use ECAM access mechanism for all the MCFG buses except the root ports. For the root ports we declare additional ACPI devices marked as "pnp0c02" (motherboard reserved resource) i.e.: Scope(_SB) { // PCIe Root bus Device (PCI1) { Name (_HID, "HISI0080") // PCI Express Root Bridge Name (_CID, "PNP0A03") // Compatible PCI Root Bridge Name(_SEG, 1) // Segment of this Root complex Name(_BBN, 64) // Base Bus Number Name(_CCA, 1) Method (_CRS, 0, Serialized) { // Root complex resources Name (RBUF, ResourceTemplate () { [...] ) }) // Name(RBUF) Return (RBUF) } // Method(_CRS) Device (RES1) { Name (_HID, "HISI0081") // HiSi PCIe RC config base address Name (_CID, "PNP0C02") // Motherboard reserved resource Name (_CRS, ResourceTemplate (){ Memory32Fixed (ReadWrite, 0xb0080000 , 0x10000) }) } [...] } // Device(PCI1) Therefore we declare a perfectly ECAM compliant MCFG and we "waste" the root ports addresses as in practice for the root ports we replace the MCFG address with the one retrieved from the ACPI device above. In terms of "safety" I think it should be ok as in practice we are reserving some addresses in MCFG that we do not use and the ones that are used are reserved as well in the ACPI namespace. >From a general perspective it seems to me that pci_mcfg_lookup can only parse mcfg entries that are declared according to the standard (i.e. you cannot declare a hacky mcfg table). Obviously the quirks allow the vendors to declare their own cfg_rd/wr function and therefore to do anything they want with the MCFG addresses. >From my perspective I think the easiest solution is to keep this quirk mechanism in place and then review vendor by vendor solution as they are pushed to the mailing list; if some vendors are abusing of some addresses/resources then they can be rejected... What do you think? Thanks Gab > > Or we do think that configuration space regions should come from > a different resource declared in the ACPI namespace if the regions > are not MCFG/ECAM compliant (ie config space is not defined through > MCFG at all - possibly through a _CRS method for a vendor specific > _HID under the PNP0A03 node ?) > > It might even be a choice we do not have anymore, but I think it > is important to make a decision and proceed accordingly. > > Comments appreciated. > > Thanks, > Lorenzo > > > if (IS_ERR(cfg)) { > > dev_err(&root->device->dev, "%04x:%pR error %ld mapping > ECAM\n", > > seg, bus_res, PTR_ERR(cfg)); > > -- > > Qualcomm Innovation Center, Inc. > > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > > a Linux Foundation Collaborative Project > >
WARNING: multiple messages have this Message-ID (diff)
From: gabriele.paoloni@huawei.com (Gabriele Paoloni) To: linux-arm-kernel@lists.infradead.org Subject: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller Date: Fri, 17 Jun 2016 08:01:43 +0000 [thread overview] Message-ID: <EE11001F9E5DDD47B7634E2F8A612F2E1F790346@lhreml507-mbx> (raw) In-Reply-To: <20160616174856.GB32388@red-moon> Hi Lorenzo and All > -----Original Message----- > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com] > Sent: 16 June 2016 18:49 > To: Christopher Covington > Cc: Tomasz Nowicki; Duc Dang; liudongdong (C); Sinan Kaya; Jeff Hugo; > Gabriele Paoloni; Jon Masters; Mark Salter; Suravee Suthikulpanit; > Jayachandran C; David Daney; Robert Richter; Hanjun Guo; linux-arm- > kernel at lists.infradead.org; Catalin Marinas; Will Deacon; Bjorn Helgaas; > Ganapatrao Kulkarni; linux-kernel at vger.kernel.org > Subject: Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling > for ACPI based PCI host controller > > On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote: > > From: Tomasz Nowicki <tn@semihalf.com> > > > > pci_generic_ecam_ops is used by default. Since there are platforms > > which have non-compliant ECAM space we need to overwrite these > > accessors prior to PCI buses enumeration. In order to do that > > we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that > > we can use proper PCI config space accessors and bus_shift. > > > > pci_generic_ecam_ops is still used for platforms free from quirks. > > > > Signed-off-by: Tomasz Nowicki <tn@semihalf.com> > > --- > > arch/arm64/kernel/pci.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c > > index 94cd43c..a891bda 100644 > > --- a/arch/arm64/kernel/pci.c > > +++ b/arch/arm64/kernel/pci.c > > @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root > *root) > > struct pci_config_window *cfg; > > struct resource cfgres; > > unsigned int bsz; > > + struct pci_ecam_ops *ops; > > > > /* Use address from _CBA if present, otherwise lookup MCFG */ > > if (!root->mcfg_addr) > > @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct > acpi_pci_root *root) > > return NULL; > > } > > > > - bsz = 1 << pci_generic_ecam_ops.bus_shift; > > + ops = pci_mcfg_get_ops(root); > > + bsz = 1 << ops->bus_shift; > > cfgres.start = root->mcfg_addr + bus_res->start * bsz; > > cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1; > > cfgres.flags = IORESOURCE_MEM; > > - cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res, > > - &pci_generic_ecam_ops); > > + cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res, ops); > > Arnd pointed this out already, I think that's the only pending question > here. > > pci_ecam_create() maps ECAM space for config regions retrieved from > the MCFG, which are *supposed* to be ECAM compliant. > > Do we think that's *always* correct/safe regardless of the kind > of quirk we are currently fixing up ? I didn't dig into the other vendors' quirk mechanism but I will quickly explain what we (would like to) do in HiSilicon Hip05/Hip06 SoCs. >From our perspective we use ECAM access mechanism for all the MCFG buses except the root ports. For the root ports we declare additional ACPI devices marked as "pnp0c02" (motherboard reserved resource) i.e.: Scope(_SB) { // PCIe Root bus Device (PCI1) { Name (_HID, "HISI0080") // PCI Express Root Bridge Name (_CID, "PNP0A03") // Compatible PCI Root Bridge Name(_SEG, 1) // Segment of this Root complex Name(_BBN, 64) // Base Bus Number Name(_CCA, 1) Method (_CRS, 0, Serialized) { // Root complex resources Name (RBUF, ResourceTemplate () { [...] ) }) // Name(RBUF) Return (RBUF) } // Method(_CRS) Device (RES1) { Name (_HID, "HISI0081") // HiSi PCIe RC config base address Name (_CID, "PNP0C02") // Motherboard reserved resource Name (_CRS, ResourceTemplate (){ Memory32Fixed (ReadWrite, 0xb0080000 , 0x10000) }) } [...] } // Device(PCI1) Therefore we declare a perfectly ECAM compliant MCFG and we "waste" the root ports addresses as in practice for the root ports we replace the MCFG address with the one retrieved from the ACPI device above. In terms of "safety" I think it should be ok as in practice we are reserving some addresses in MCFG that we do not use and the ones that are used are reserved as well in the ACPI namespace. >From a general perspective it seems to me that pci_mcfg_lookup can only parse mcfg entries that are declared according to the standard (i.e. you cannot declare a hacky mcfg table). Obviously the quirks allow the vendors to declare their own cfg_rd/wr function and therefore to do anything they want with the MCFG addresses. >From my perspective I think the easiest solution is to keep this quirk mechanism in place and then review vendor by vendor solution as they are pushed to the mailing list; if some vendors are abusing of some addresses/resources then they can be rejected... What do you think? Thanks Gab > > Or we do think that configuration space regions should come from > a different resource declared in the ACPI namespace if the regions > are not MCFG/ECAM compliant (ie config space is not defined through > MCFG at all - possibly through a _CRS method for a vendor specific > _HID under the PNP0A03 node ?) > > It might even be a choice we do not have anymore, but I think it > is important to make a decision and proceed accordingly. > > Comments appreciated. > > Thanks, > Lorenzo > > > if (IS_ERR(cfg)) { > > dev_err(&root->device->dev, "%04x:%pR error %ld mapping > ECAM\n", > > seg, bus_res, PTR_ERR(cfg)); > > -- > > Qualcomm Innovation Center, Inc. > > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > > a Linux Foundation Collaborative Project > >
next prev parent reply other threads:[~2016-06-17 8:02 UTC|newest] Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-06-15 15:34 [RFC PATCH v3 1/2] ACPI/PCI: Check platform specific ECAM quirks Christopher Covington 2016-06-15 15:34 ` Christopher Covington 2016-06-15 15:34 ` [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller Christopher Covington 2016-06-15 15:34 ` Christopher Covington 2016-06-16 17:48 ` Lorenzo Pieralisi 2016-06-16 17:48 ` Lorenzo Pieralisi 2016-06-17 8:01 ` Gabriele Paoloni [this message] 2016-06-17 8:01 ` Gabriele Paoloni 2016-06-17 14:13 ` Christopher Covington 2016-06-17 14:13 ` Christopher Covington 2016-06-16 17:10 ` [RFC PATCH v3 1/2] ACPI/PCI: Check platform specific ECAM quirks Lorenzo Pieralisi 2016-06-16 17:10 ` Lorenzo Pieralisi 2016-06-20 17:16 ` Lorenzo Pieralisi 2016-06-20 17:16 ` Lorenzo Pieralisi 2016-06-20 17:16 ` Lorenzo Pieralisi 2016-06-20 17:16 ` Lorenzo Pieralisi 2016-06-21 8:37 ` Ard Biesheuvel 2016-06-21 8:37 ` Ard Biesheuvel 2016-06-21 8:37 ` Ard Biesheuvel 2016-06-21 8:37 ` Ard Biesheuvel 2016-06-21 8:37 ` Ard Biesheuvel 2016-06-17 21:37 [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller Duc Dang 2016-06-20 9:42 ` Lorenzo Pieralisi 2016-06-20 9:42 ` Lorenzo Pieralisi 2016-06-20 17:17 ` Christopher Covington 2016-06-20 17:17 ` Christopher Covington 2016-06-20 19:12 ` Duc Dang 2016-06-20 19:12 ` Duc Dang 2016-06-21 8:42 ` Duc Dang 2016-06-21 8:42 ` Duc Dang 2016-06-21 9:26 ` Lorenzo Pieralisi 2016-06-21 9:26 ` Lorenzo Pieralisi 2016-06-21 9:29 ` Duc Dang 2016-06-21 9:29 ` Duc Dang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=EE11001F9E5DDD47B7634E2F8A612F2E1F790346@lhreml507-mbx \ --to=gabriele.paoloni@huawei.com \ --cc=Suravee.Suthikulpanit@amd.com \ --cc=bhelgaas@google.com \ --cc=catalin.marinas@arm.com \ --cc=cov@codeaurora.org \ --cc=ddaney@caviumnetworks.com \ --cc=dhdang@apm.com \ --cc=gkulkarni@caviumnetworks.com \ --cc=hanjun.guo@linaro.org \ --cc=jchandra@broadcom.com \ --cc=jcm@redhat.com \ --cc=jhugo@codeaurora.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=liudongdong3@huawei.com \ --cc=lorenzo.pieralisi@arm.com \ --cc=msalter@redhat.com \ --cc=okaya@codeaurora.org \ --cc=robert.richter@caviumnetworks.com \ --cc=tn@semihalf.com \ --cc=will.deacon@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.