From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751483AbdFFQoS (ORCPT ); Tue, 6 Jun 2017 12:44:18 -0400 Received: from mail-pg0-f54.google.com ([74.125.83.54]:35925 "EHLO mail-pg0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751291AbdFFQoQ (ORCPT ); Tue, 6 Jun 2017 12:44:16 -0400 MIME-Version: 1.0 In-Reply-To: References: <1493340847-25720-1-git-send-email-kdinh@apm.com> <20170505165119.GA8614@red-moon> From: Khuong Dinh Date: Tue, 6 Jun 2017 09:44:15 -0700 Message-ID: Subject: Re: [PATCH v2 pci] PCI/MSI: pci-xgene-msi: Enable MSI support in ACPI boot for X-Gene v1 To: Lorenzo Pieralisi Cc: Marc Zyngier , msalter@redhat.com, Bjorn Helgaas , linux-pci@vger.kernel.org, jcm@redhat.com, patches@apm.com, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, rjw@rjwysocki.net, Duc Dang Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id v56GiMNM008493 Hi Lorenzo, On Tue, May 9, 2017 at 3:55 PM, Khuong Dinh wrote: > Hi Lorenzo, > > On Fri, May 5, 2017 at 9:51 AM, Lorenzo Pieralisi > wrote: >> On Thu, May 04, 2017 at 05:36:06PM -0700, Khuong Dinh wrote: >>> Hi Marc, >>> There's no explicit dependency between pcie driver and msi >>> controller. The current solution that we did is relying on the >>> node ordering in BIOS. ACPI 5.0 introduced _DEP method to assign a >>> higher priority in start ordering. This method could be applied in >>> case of msi and pcie are the same level of subsys_init (in ACPI >>> boot). However, PCIe driver has not supported for this dependency >>> check yet. How do you think about this solution. >> >> First off, you can't post emails with confidentiality notices on >> mailing lists so remove it from now onwards. > > Fixed > >> Secondly, I commented on this before, so you know what my opinion is. > > I got your opinion. I'll implement as your suggestion. > Regarding to your previous suggestion to add a hook walking the ACPI namespace before acpi_bus_scan() in acpi_scan_init() to make MSI controllers must be probed before PCI. I have a concern that the MSI namespace “ _SB.MSIX” is not a unique name and how can we walk the ACPI namespace and use this name to make MSI probed before PCI. May you have little bit more information for this or do you have another suggestion for this? There’s another solution which makes this simpler and I’d like to ask your opinion about this. The solution is to make an hierarchy between MSI and PCI nodes as below: Device(\_SB.MSIX) { Device(\_SB.PCI0) Device(\_SB.PCI1) …… } In other word, MSIX node is a parent node of PCI nodes in ACPI table. In this sense, there’s an explicit dependency between MSI and PCI, MSI controller must be probed before PCI and it will guarantee not breaking next kernel releases. How do you think about this solution. I also tried to use _DEP method to describe the dependency of PCIe nodes, but it looks that it does not work as PCI and MSI are handed by acpi_bus_scan and still having issue when we re-probe PCI. Thanks, Khuong Dinh >> Finally, please execute this command on the vmlinux that "works" >> for you: >> >> nm vmlinux | grep -E '__initcall_(xgene_pcie_msi_init|acpi_init)' > > $ nm vmlinux | grep -E '__initcall_(xgene_pcie_msi_init|acpi_init)' > ffff000008dab2d8 t __initcall_acpi_init4 > ffff000008dab2c8 t __initcall_xgene_pcie_msi_init4 > > Best regards, > Khuong Dinh > >> Even by ordering devices in the ACPI tables (that I abhor) I still do >> not understand how this works (I mean without relying on kernel link >> order to ensure that the MSI driver is probed before PCI devices are >> enumerated in acpi_init()). >> >> Thanks, >> Lorenzo >> >>> Best regards, >>> Khuong >>> >>> On Fri, Apr 28, 2017 at 2:27 AM, Marc Zyngier wrote: >>> > On 28/04/17 01:54, Khuong Dinh wrote: >>> >> From: Khuong Dinh >>> >> >>> >> This patch makes pci-xgene-msi driver ACPI-aware and provides >>> >> MSI capability for X-Gene v1 PCIe controllers in ACPI boot mode. >>> >> >>> >> Signed-off-by: Khuong Dinh >>> >> Signed-off-by: Duc Dang >>> >> Acked-by: Marc Zyngier >>> >> --- >>> >> v2: >>> >> - Verify with BIOS version 3.06.25 and 3.07.09 >>> >> v1: >>> >> - Initial version >>> >> --- >>> >> drivers/pci/host/pci-xgene-msi.c | 35 ++++++++++++++++++++++++++++++++--- >>> >> 1 files changed, 32 insertions(+), 3 deletions(-) >>> >> >>> >> diff --git a/drivers/pci/host/pci-xgene-msi.c b/drivers/pci/host/pci-xgene-msi.c >>> >> index f1b633b..00aaa3d 100644 >>> >> --- a/drivers/pci/host/pci-xgene-msi.c >>> >> +++ b/drivers/pci/host/pci-xgene-msi.c >>> >> @@ -24,6 +24,7 @@ >>> >> #include >>> >> #include >>> >> #include >>> >> +#include >>> >> >>> >> #define MSI_IR0 0x000000 >>> >> #define MSI_INT0 0x800000 >>> >> @@ -39,7 +40,7 @@ struct xgene_msi_group { >>> >> }; >>> >> >>> >> struct xgene_msi { >>> >> - struct device_node *node; >>> >> + struct fwnode_handle *fwnode; >>> >> struct irq_domain *inner_domain; >>> >> struct irq_domain *msi_domain; >>> >> u64 msi_addr; >>> >> @@ -249,6 +250,13 @@ static void xgene_irq_domain_free(struct irq_domain *domain, >>> >> .free = xgene_irq_domain_free, >>> >> }; >>> >> >>> >> +#ifdef CONFIG_ACPI >>> >> +static struct fwnode_handle *xgene_msi_get_fwnode(struct device *dev) >>> >> +{ >>> >> + return xgene_msi_ctrl.fwnode; >>> >> +} >>> >> +#endif >>> >> + >>> >> static int xgene_allocate_domains(struct xgene_msi *msi) >>> >> { >>> >> msi->inner_domain = irq_domain_add_linear(NULL, NR_MSI_VEC, >>> >> @@ -256,7 +264,7 @@ static int xgene_allocate_domains(struct xgene_msi *msi) >>> >> if (!msi->inner_domain) >>> >> return -ENOMEM; >>> >> >>> >> - msi->msi_domain = pci_msi_create_irq_domain(of_node_to_fwnode(msi->node), >>> >> + msi->msi_domain = pci_msi_create_irq_domain(msi->fwnode, >>> >> &xgene_msi_domain_info, >>> >> msi->inner_domain); >>> >> >>> >> @@ -265,6 +273,9 @@ static int xgene_allocate_domains(struct xgene_msi *msi) >>> >> return -ENOMEM; >>> >> } >>> >> >>> >> +#ifdef CONFIG_ACPI >>> >> + pci_msi_register_fwnode_provider(&xgene_msi_get_fwnode); >>> >> +#endif >>> >> return 0; >>> >> } >>> >> >>> >> @@ -449,6 +460,13 @@ static int xgene_msi_hwirq_free(unsigned int cpu) >>> >> {}, >>> >> }; >>> >> >>> >> +#ifdef CONFIG_ACPI >>> >> +static const struct acpi_device_id xgene_msi_acpi_ids[] = { >>> >> + {"APMC0D0E", 0}, >>> >> + { }, >>> >> +}; >>> >> +#endif >>> >> + >>> >> static int xgene_msi_probe(struct platform_device *pdev) >>> >> { >>> >> struct resource *res; >>> >> @@ -469,7 +487,17 @@ static int xgene_msi_probe(struct platform_device *pdev) >>> >> goto error; >>> >> } >>> >> xgene_msi->msi_addr = res->start; >>> >> - xgene_msi->node = pdev->dev.of_node; >>> >> + >>> >> + xgene_msi->fwnode = of_node_to_fwnode(pdev->dev.of_node); >>> >> + if (!xgene_msi->fwnode) { >>> >> + xgene_msi->fwnode = irq_domain_alloc_fwnode(NULL); >>> > >>> > Please provide something other than NULL, such as the base address if >>> > the device. That's quite useful for debugging. >>> > >>> >> + if (!xgene_msi->fwnode) { >>> >> + dev_err(&pdev->dev, "Failed to create fwnode\n"); >>> >> + rc = ENOMEM; >>> >> + goto error; >>> >> + } >>> >> + } >>> >> + >>> >> xgene_msi->num_cpus = num_possible_cpus(); >>> >> >>> >> rc = xgene_msi_init_allocator(xgene_msi); >>> >> @@ -540,6 +568,7 @@ static int xgene_msi_probe(struct platform_device *pdev) >>> >> .driver = { >>> >> .name = "xgene-msi", >>> >> .of_match_table = xgene_msi_match_table, >>> >> + .acpi_match_table = ACPI_PTR(xgene_msi_acpi_ids), >>> >> }, >>> >> .probe = xgene_msi_probe, >>> >> .remove = xgene_msi_remove, >>> >> >>> > >>> > The code is trivial, but relies on the MSI controller to probe before >>> > the PCI bus. What enforces this? How is it making sure that this is not >>> > going to break in the next kernel release? As far as I can tell, there >>> > is no explicit dependency between the two, making it the whole thing >>> > extremely fragile. >>> > >>> > Thanks, >>> > >>> > M. >>> > -- >>> > Jazz is not dead. It just smells funny... >>> >>> -- From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1493340847-25720-1-git-send-email-kdinh@apm.com> <20170505165119.GA8614@red-moon> From: Khuong Dinh Date: Tue, 6 Jun 2017 09:44:15 -0700 Message-ID: Subject: Re: [PATCH v2 pci] PCI/MSI: pci-xgene-msi: Enable MSI support in ACPI boot for X-Gene v1 To: Lorenzo Pieralisi List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: rjw@rjwysocki.net, Duc Dang , Marc Zyngier , linux-pci@vger.kernel.org, msalter@redhat.com, patches@apm.com, linux-kernel@vger.kernel.org, jcm@redhat.com, Bjorn Helgaas , linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="utf-8" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+bjorn=helgaas.com@lists.infradead.org List-ID: SGkgTG9yZW56bywKCk9uIFR1ZSwgTWF5IDksIDIwMTcgYXQgMzo1NSBQTSwgS2h1b25nIERpbmgg PGtkaW5oQGFwbS5jb20+IHdyb3RlOgo+IEhpIExvcmVuem8sCj4KPiBPbiBGcmksIE1heSA1LCAy MDE3IGF0IDk6NTEgQU0sIExvcmVuem8gUGllcmFsaXNpCj4gPGxvcmVuem8ucGllcmFsaXNpQGFy bS5jb20+IHdyb3RlOgo+PiBPbiBUaHUsIE1heSAwNCwgMjAxNyBhdCAwNTozNjowNlBNIC0wNzAw LCBLaHVvbmcgRGluaCB3cm90ZToKPj4+IEhpIE1hcmMsCj4+PiBUaGVyZSdzIG5vIGV4cGxpY2l0 IGRlcGVuZGVuY3kgYmV0d2VlbiBwY2llIGRyaXZlciBhbmQgbXNpCj4+PiBjb250cm9sbGVyLiAg VGhlIGN1cnJlbnQgc29sdXRpb24gdGhhdCB3ZSBkaWQgaXMgcmVseWluZyBvbiB0aGUKPj4+IG5v ZGUgb3JkZXJpbmcgaW4gQklPUy4gIEFDUEkgNS4wIGludHJvZHVjZWQgX0RFUCBtZXRob2QgdG8g YXNzaWduIGEKPj4+IGhpZ2hlciBwcmlvcml0eSBpbiBzdGFydCBvcmRlcmluZy4gIFRoaXMgbWV0 aG9kIGNvdWxkIGJlIGFwcGxpZWQgaW4KPj4+IGNhc2Ugb2YgbXNpIGFuZCBwY2llIGFyZSB0aGUg c2FtZSBsZXZlbCBvZiBzdWJzeXNfaW5pdCAoaW4gQUNQSQo+Pj4gYm9vdCkuICBIb3dldmVyLCBQ Q0llIGRyaXZlciBoYXMgbm90IHN1cHBvcnRlZCBmb3IgdGhpcyBkZXBlbmRlbmN5Cj4+PiBjaGVj ayB5ZXQuICBIb3cgZG8geW91IHRoaW5rIGFib3V0IHRoaXMgc29sdXRpb24uCj4+Cj4+IEZpcnN0 IG9mZiwgeW91IGNhbid0IHBvc3QgZW1haWxzIHdpdGggY29uZmlkZW50aWFsaXR5IG5vdGljZXMg b24KPj4gbWFpbGluZyBsaXN0cyBzbyByZW1vdmUgaXQgZnJvbSBub3cgb253YXJkcy4KPgo+IEZp eGVkCj4KPj4gU2Vjb25kbHksIEkgY29tbWVudGVkIG9uIHRoaXMgYmVmb3JlLCBzbyB5b3Uga25v dyB3aGF0IG15IG9waW5pb24gaXMuCj4KPiBJIGdvdCB5b3VyIG9waW5pb24uIEknbGwgaW1wbGVt ZW50IGFzIHlvdXIgc3VnZ2VzdGlvbi4KPgoKICBSZWdhcmRpbmcgdG8geW91ciBwcmV2aW91cyBz dWdnZXN0aW9uIHRvIGFkZCBhIGhvb2sgd2Fsa2luZyB0aGUgQUNQSQpuYW1lc3BhY2UgYmVmb3Jl IGFjcGlfYnVzX3NjYW4oKSBpbiBhY3BpX3NjYW5faW5pdCgpIHRvIG1ha2UgTVNJCmNvbnRyb2xs ZXJzIG11c3QgYmUgcHJvYmVkIGJlZm9yZSBQQ0kuCiAgSSBoYXZlIGEgY29uY2VybiB0aGF0IHRo ZSBNU0kgbmFtZXNwYWNlIOKAnCBfU0IuTVNJWOKAnSBpcyBub3QgYSB1bmlxdWUKbmFtZSBhbmQg aG93IGNhbiB3ZSB3YWxrIHRoZSBBQ1BJIG5hbWVzcGFjZSBhbmQgdXNlIHRoaXMgbmFtZSB0byBt YWtlCk1TSSBwcm9iZWQgYmVmb3JlIFBDSS4KICBNYXkgeW91IGhhdmUgbGl0dGxlIGJpdCBtb3Jl IGluZm9ybWF0aW9uIGZvciB0aGlzIG9yIGRvIHlvdSBoYXZlCmFub3RoZXIgc3VnZ2VzdGlvbiBm b3IgdGhpcz8KCiAgIFRoZXJl4oCZcyBhbm90aGVyIHNvbHV0aW9uIHdoaWNoIG1ha2VzIHRoaXMg c2ltcGxlciBhbmQgSeKAmWQgbGlrZSB0bwphc2sgeW91ciBvcGluaW9uIGFib3V0IHRoaXMuCiAg IFRoZSBzb2x1dGlvbiBpcyB0byBtYWtlIGFuIGhpZXJhcmNoeSBiZXR3ZWVuIE1TSSBhbmQgUENJ IG5vZGVzICBhcyBiZWxvdzoKRGV2aWNlKFxfU0IuTVNJWCkgewogICBEZXZpY2UoXF9TQi5QQ0kw KQogICBEZXZpY2UoXF9TQi5QQ0kxKQogICDigKbigKYKfQogIEluIG90aGVyIHdvcmQsIE1TSVgg bm9kZSBpcyBhIHBhcmVudCBub2RlIG9mIFBDSSBub2RlcyBpbiBBQ1BJIHRhYmxlLgogIEluIHRo aXMgc2Vuc2UsIHRoZXJl4oCZcyBhbiBleHBsaWNpdCBkZXBlbmRlbmN5IGJldHdlZW4gTVNJIGFu ZCBQQ0ksCk1TSSBjb250cm9sbGVyIG11c3QgYmUgcHJvYmVkIGJlZm9yZSBQQ0kgYW5kIGl0IHdp bGwgZ3VhcmFudGVlIG5vdApicmVha2luZyBuZXh0IGtlcm5lbCByZWxlYXNlcy4KICBIb3cgZG8g eW91IHRoaW5rIGFib3V0IHRoaXMgc29sdXRpb24uCgogSSBhbHNvIHRyaWVkIHRvIHVzZSBfREVQ IG1ldGhvZCB0byBkZXNjcmliZSB0aGUgZGVwZW5kZW5jeSBvZiBQQ0llCm5vZGVzLCBidXQgaXQg bG9va3MgdGhhdCBpdCBkb2VzIG5vdCB3b3JrIGFzIFBDSSBhbmQgTVNJIGFyZSBoYW5kZWQgYnkK IGFjcGlfYnVzX3NjYW4gYW5kIHN0aWxsIGhhdmluZyBpc3N1ZSB3aGVuIHdlIHJlLXByb2JlIFBD SS4KClRoYW5rcywKS2h1b25nIERpbmgKCj4+IEZpbmFsbHksIHBsZWFzZSBleGVjdXRlIHRoaXMg Y29tbWFuZCBvbiB0aGUgdm1saW51eCB0aGF0ICJ3b3JrcyIKPj4gZm9yIHlvdToKPj4KPj4gbm0g dm1saW51eCB8IGdyZXAgLUUgJ19faW5pdGNhbGxfKHhnZW5lX3BjaWVfbXNpX2luaXR8YWNwaV9p bml0KScKPgo+ICQgbm0gdm1saW51eCB8IGdyZXAgLUUgJ19faW5pdGNhbGxfKHhnZW5lX3BjaWVf bXNpX2luaXR8YWNwaV9pbml0KScKPiBmZmZmMDAwMDA4ZGFiMmQ4IHQgX19pbml0Y2FsbF9hY3Bp X2luaXQ0Cj4gZmZmZjAwMDAwOGRhYjJjOCB0IF9faW5pdGNhbGxfeGdlbmVfcGNpZV9tc2lfaW5p dDQKPgo+IEJlc3QgcmVnYXJkcywKPiBLaHVvbmcgRGluaAo+Cj4+IEV2ZW4gYnkgb3JkZXJpbmcg ZGV2aWNlcyBpbiB0aGUgQUNQSSB0YWJsZXMgKHRoYXQgSSBhYmhvcikgSSBzdGlsbCBkbwo+PiBu b3QgdW5kZXJzdGFuZCBob3cgdGhpcyB3b3JrcyAoSSBtZWFuIHdpdGhvdXQgcmVseWluZyBvbiBr ZXJuZWwgbGluawo+PiBvcmRlciB0byBlbnN1cmUgdGhhdCB0aGUgTVNJIGRyaXZlciBpcyBwcm9i ZWQgYmVmb3JlIFBDSSBkZXZpY2VzIGFyZQo+PiBlbnVtZXJhdGVkIGluIGFjcGlfaW5pdCgpKS4K Pj4KPj4gVGhhbmtzLAo+PiBMb3JlbnpvCj4+Cj4+PiBCZXN0IHJlZ2FyZHMsCj4+PiBLaHVvbmcK Pj4+Cj4+PiBPbiBGcmksIEFwciAyOCwgMjAxNyBhdCAyOjI3IEFNLCBNYXJjIFp5bmdpZXIgPG1h cmMuenluZ2llckBhcm0uY29tPiB3cm90ZToKPj4+ID4gT24gMjgvMDQvMTcgMDE6NTQsIEtodW9u ZyBEaW5oIHdyb3RlOgo+Pj4gPj4gRnJvbTogS2h1b25nIERpbmggPGtkaW5oQGFwbS5jb20+Cj4+ PiA+Pgo+Pj4gPj4gVGhpcyBwYXRjaCBtYWtlcyBwY2kteGdlbmUtbXNpIGRyaXZlciBBQ1BJLWF3 YXJlIGFuZCBwcm92aWRlcwo+Pj4gPj4gTVNJIGNhcGFiaWxpdHkgZm9yIFgtR2VuZSB2MSBQQ0ll IGNvbnRyb2xsZXJzIGluIEFDUEkgYm9vdCBtb2RlLgo+Pj4gPj4KPj4+ID4+IFNpZ25lZC1vZmYt Ynk6IEtodW9uZyBEaW5oIDxrZGluaEBhcG0uY29tPgo+Pj4gPj4gU2lnbmVkLW9mZi1ieTogRHVj IERhbmcgPGRoZGFuZ0BhcG0uY29tPgo+Pj4gPj4gQWNrZWQtYnk6IE1hcmMgWnluZ2llciA8bWFy Yy56eW5naWVyQGFybS5jb20+Cj4+PiA+PiAtLS0KPj4+ID4+IHYyOgo+Pj4gPj4gIC0gVmVyaWZ5 IHdpdGggQklPUyB2ZXJzaW9uIDMuMDYuMjUgYW5kIDMuMDcuMDkKPj4+ID4+IHYxOgo+Pj4gPj4g IC0gSW5pdGlhbCB2ZXJzaW9uCj4+PiA+PiAtLS0KPj4+ID4+ICBkcml2ZXJzL3BjaS9ob3N0L3Bj aS14Z2VuZS1tc2kuYyB8ICAgMzUgKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKystLS0K Pj4+ID4+ICAxIGZpbGVzIGNoYW5nZWQsIDMyIGluc2VydGlvbnMoKyksIDMgZGVsZXRpb25zKC0p Cj4+PiA+Pgo+Pj4gPj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvcGNpL2hvc3QvcGNpLXhnZW5lLW1z aS5jIGIvZHJpdmVycy9wY2kvaG9zdC9wY2kteGdlbmUtbXNpLmMKPj4+ID4+IGluZGV4IGYxYjYz M2IuLjAwYWFhM2QgMTAwNjQ0Cj4+PiA+PiAtLS0gYS9kcml2ZXJzL3BjaS9ob3N0L3BjaS14Z2Vu ZS1tc2kuYwo+Pj4gPj4gKysrIGIvZHJpdmVycy9wY2kvaG9zdC9wY2kteGdlbmUtbXNpLmMKPj4+ ID4+IEBAIC0yNCw2ICsyNCw3IEBACj4+PiA+PiAgI2luY2x1ZGUgPGxpbnV4L3BjaS5oPgo+Pj4g Pj4gICNpbmNsdWRlIDxsaW51eC9wbGF0Zm9ybV9kZXZpY2UuaD4KPj4+ID4+ICAjaW5jbHVkZSA8 bGludXgvb2ZfcGNpLmg+Cj4+PiA+PiArI2luY2x1ZGUgPGxpbnV4L2FjcGkuaD4KPj4+ID4+Cj4+ PiA+PiAgI2RlZmluZSBNU0lfSVIwICAgICAgICAgICAgICAgICAgICAgIDB4MDAwMDAwCj4+PiA+ PiAgI2RlZmluZSBNU0lfSU5UMCAgICAgICAgICAgICAweDgwMDAwMAo+Pj4gPj4gQEAgLTM5LDcg KzQwLDcgQEAgc3RydWN0IHhnZW5lX21zaV9ncm91cCB7Cj4+PiA+PiAgfTsKPj4+ID4+Cj4+PiA+ PiAgc3RydWN0IHhnZW5lX21zaSB7Cj4+PiA+PiAtICAgICBzdHJ1Y3QgZGV2aWNlX25vZGUgICAg ICAqbm9kZTsKPj4+ID4+ICsgICAgIHN0cnVjdCBmd25vZGVfaGFuZGxlICAgICpmd25vZGU7Cj4+ PiA+PiAgICAgICBzdHJ1Y3QgaXJxX2RvbWFpbiAgICAgICAqaW5uZXJfZG9tYWluOwo+Pj4gPj4g ICAgICAgc3RydWN0IGlycV9kb21haW4gICAgICAgKm1zaV9kb21haW47Cj4+PiA+PiAgICAgICB1 NjQgICAgICAgICAgICAgICAgICAgICBtc2lfYWRkcjsKPj4+ID4+IEBAIC0yNDksNiArMjUwLDEz IEBAIHN0YXRpYyB2b2lkIHhnZW5lX2lycV9kb21haW5fZnJlZShzdHJ1Y3QgaXJxX2RvbWFpbiAq ZG9tYWluLAo+Pj4gPj4gICAgICAgLmZyZWUgICA9IHhnZW5lX2lycV9kb21haW5fZnJlZSwKPj4+ ID4+ICB9Owo+Pj4gPj4KPj4+ID4+ICsjaWZkZWYgQ09ORklHX0FDUEkKPj4+ID4+ICtzdGF0aWMg c3RydWN0IGZ3bm9kZV9oYW5kbGUgKnhnZW5lX21zaV9nZXRfZndub2RlKHN0cnVjdCBkZXZpY2Ug KmRldikKPj4+ID4+ICt7Cj4+PiA+PiArICAgICByZXR1cm4geGdlbmVfbXNpX2N0cmwuZndub2Rl Owo+Pj4gPj4gK30KPj4+ID4+ICsjZW5kaWYKPj4+ID4+ICsKPj4+ID4+ICBzdGF0aWMgaW50IHhn ZW5lX2FsbG9jYXRlX2RvbWFpbnMoc3RydWN0IHhnZW5lX21zaSAqbXNpKQo+Pj4gPj4gIHsKPj4+ ID4+ICAgICAgIG1zaS0+aW5uZXJfZG9tYWluID0gaXJxX2RvbWFpbl9hZGRfbGluZWFyKE5VTEws IE5SX01TSV9WRUMsCj4+PiA+PiBAQCAtMjU2LDcgKzI2NCw3IEBAIHN0YXRpYyBpbnQgeGdlbmVf YWxsb2NhdGVfZG9tYWlucyhzdHJ1Y3QgeGdlbmVfbXNpICptc2kpCj4+PiA+PiAgICAgICBpZiAo IW1zaS0+aW5uZXJfZG9tYWluKQo+Pj4gPj4gICAgICAgICAgICAgICByZXR1cm4gLUVOT01FTTsK Pj4+ID4+Cj4+PiA+PiAtICAgICBtc2ktPm1zaV9kb21haW4gPSBwY2lfbXNpX2NyZWF0ZV9pcnFf ZG9tYWluKG9mX25vZGVfdG9fZndub2RlKG1zaS0+bm9kZSksCj4+PiA+PiArICAgICBtc2ktPm1z aV9kb21haW4gPSBwY2lfbXNpX2NyZWF0ZV9pcnFfZG9tYWluKG1zaS0+Zndub2RlLAo+Pj4gPj4g ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAmeGdlbmVf bXNpX2RvbWFpbl9pbmZvLAo+Pj4gPj4gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICBtc2ktPmlubmVyX2RvbWFpbik7Cj4+PiA+Pgo+Pj4gPj4gQEAgLTI2 NSw2ICsyNzMsOSBAQCBzdGF0aWMgaW50IHhnZW5lX2FsbG9jYXRlX2RvbWFpbnMoc3RydWN0IHhn ZW5lX21zaSAqbXNpKQo+Pj4gPj4gICAgICAgICAgICAgICByZXR1cm4gLUVOT01FTTsKPj4+ID4+ ICAgICAgIH0KPj4+ID4+Cj4+PiA+PiArI2lmZGVmIENPTkZJR19BQ1BJCj4+PiA+PiArICAgICBw Y2lfbXNpX3JlZ2lzdGVyX2Z3bm9kZV9wcm92aWRlcigmeGdlbmVfbXNpX2dldF9md25vZGUpOwo+ Pj4gPj4gKyNlbmRpZgo+Pj4gPj4gICAgICAgcmV0dXJuIDA7Cj4+PiA+PiAgfQo+Pj4gPj4KPj4+ ID4+IEBAIC00NDksNiArNDYwLDEzIEBAIHN0YXRpYyBpbnQgeGdlbmVfbXNpX2h3aXJxX2ZyZWUo dW5zaWduZWQgaW50IGNwdSkKPj4+ID4+ICAgICAgIHt9LAo+Pj4gPj4gIH07Cj4+PiA+Pgo+Pj4g Pj4gKyNpZmRlZiBDT05GSUdfQUNQSQo+Pj4gPj4gK3N0YXRpYyBjb25zdCBzdHJ1Y3QgYWNwaV9k ZXZpY2VfaWQgeGdlbmVfbXNpX2FjcGlfaWRzW10gPSB7Cj4+PiA+PiArICAgICB7IkFQTUMwRDBF IiwgMH0sCj4+PiA+PiArICAgICB7IH0sCj4+PiA+PiArfTsKPj4+ID4+ICsjZW5kaWYKPj4+ID4+ ICsKPj4+ID4+ICBzdGF0aWMgaW50IHhnZW5lX21zaV9wcm9iZShzdHJ1Y3QgcGxhdGZvcm1fZGV2 aWNlICpwZGV2KQo+Pj4gPj4gIHsKPj4+ID4+ICAgICAgIHN0cnVjdCByZXNvdXJjZSAqcmVzOwo+ Pj4gPj4gQEAgLTQ2OSw3ICs0ODcsMTcgQEAgc3RhdGljIGludCB4Z2VuZV9tc2lfcHJvYmUoc3Ry dWN0IHBsYXRmb3JtX2RldmljZSAqcGRldikKPj4+ID4+ICAgICAgICAgICAgICAgZ290byBlcnJv cjsKPj4+ID4+ICAgICAgIH0KPj4+ID4+ICAgICAgIHhnZW5lX21zaS0+bXNpX2FkZHIgPSByZXMt PnN0YXJ0Owo+Pj4gPj4gLSAgICAgeGdlbmVfbXNpLT5ub2RlID0gcGRldi0+ZGV2Lm9mX25vZGU7 Cj4+PiA+PiArCj4+PiA+PiArICAgICB4Z2VuZV9tc2ktPmZ3bm9kZSA9IG9mX25vZGVfdG9fZndu b2RlKHBkZXYtPmRldi5vZl9ub2RlKTsKPj4+ID4+ICsgICAgIGlmICgheGdlbmVfbXNpLT5md25v ZGUpIHsKPj4+ID4+ICsgICAgICAgICAgICAgeGdlbmVfbXNpLT5md25vZGUgPSBpcnFfZG9tYWlu X2FsbG9jX2Z3bm9kZShOVUxMKTsKPj4+ID4KPj4+ID4gUGxlYXNlIHByb3ZpZGUgc29tZXRoaW5n IG90aGVyIHRoYW4gTlVMTCwgc3VjaCBhcyB0aGUgYmFzZSBhZGRyZXNzIGlmCj4+PiA+IHRoZSBk ZXZpY2UuIFRoYXQncyBxdWl0ZSB1c2VmdWwgZm9yIGRlYnVnZ2luZy4KPj4+ID4KPj4+ID4+ICsg ICAgICAgICAgICAgaWYgKCF4Z2VuZV9tc2ktPmZ3bm9kZSkgewo+Pj4gPj4gKyAgICAgICAgICAg ICAgICAgICAgIGRldl9lcnIoJnBkZXYtPmRldiwgIkZhaWxlZCB0byBjcmVhdGUgZndub2RlXG4i KTsKPj4+ID4+ICsgICAgICAgICAgICAgICAgICAgICByYyA9IEVOT01FTTsKPj4+ID4+ICsgICAg ICAgICAgICAgICAgICAgICBnb3RvIGVycm9yOwo+Pj4gPj4gKyAgICAgICAgICAgICB9Cj4+PiA+ PiArICAgICB9Cj4+PiA+PiArCj4+PiA+PiAgICAgICB4Z2VuZV9tc2ktPm51bV9jcHVzID0gbnVt X3Bvc3NpYmxlX2NwdXMoKTsKPj4+ID4+Cj4+PiA+PiAgICAgICByYyA9IHhnZW5lX21zaV9pbml0 X2FsbG9jYXRvcih4Z2VuZV9tc2kpOwo+Pj4gPj4gQEAgLTU0MCw2ICs1NjgsNyBAQCBzdGF0aWMg aW50IHhnZW5lX21zaV9wcm9iZShzdHJ1Y3QgcGxhdGZvcm1fZGV2aWNlICpwZGV2KQo+Pj4gPj4g ICAgICAgLmRyaXZlciA9IHsKPj4+ID4+ICAgICAgICAgICAgICAgLm5hbWUgPSAieGdlbmUtbXNp IiwKPj4+ID4+ICAgICAgICAgICAgICAgLm9mX21hdGNoX3RhYmxlID0geGdlbmVfbXNpX21hdGNo X3RhYmxlLAo+Pj4gPj4gKyAgICAgICAgICAgICAuYWNwaV9tYXRjaF90YWJsZSA9IEFDUElfUFRS KHhnZW5lX21zaV9hY3BpX2lkcyksCj4+PiA+PiAgICAgICB9LAo+Pj4gPj4gICAgICAgLnByb2Jl ID0geGdlbmVfbXNpX3Byb2JlLAo+Pj4gPj4gICAgICAgLnJlbW92ZSA9IHhnZW5lX21zaV9yZW1v dmUsCj4+PiA+Pgo+Pj4gPgo+Pj4gPiBUaGUgY29kZSBpcyB0cml2aWFsLCBidXQgcmVsaWVzIG9u IHRoZSBNU0kgY29udHJvbGxlciB0byBwcm9iZSBiZWZvcmUKPj4+ID4gdGhlIFBDSSBidXMuIFdo YXQgZW5mb3JjZXMgdGhpcz8gSG93IGlzIGl0IG1ha2luZyBzdXJlIHRoYXQgdGhpcyBpcyBub3QK Pj4+ID4gZ29pbmcgdG8gYnJlYWsgaW4gdGhlIG5leHQga2VybmVsIHJlbGVhc2U/IEFzIGZhciBh cyBJIGNhbiB0ZWxsLCB0aGVyZQo+Pj4gPiBpcyBubyBleHBsaWNpdCBkZXBlbmRlbmN5IGJldHdl ZW4gdGhlIHR3bywgbWFraW5nIGl0IHRoZSB3aG9sZSB0aGluZwo+Pj4gPiBleHRyZW1lbHkgZnJh Z2lsZS4KPj4+ID4KPj4+ID4gVGhhbmtzLAo+Pj4gPgo+Pj4gPiAgICAgICAgIE0uCj4+PiA+IC0t Cj4+PiA+IEphenogaXMgbm90IGRlYWQuIEl0IGp1c3Qgc21lbGxzIGZ1bm55Li4uCj4+Pgo+Pj4g LS0KCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmxpbnV4 LWFybS1rZXJuZWwgbWFpbGluZyBsaXN0CmxpbnV4LWFybS1rZXJuZWxAbGlzdHMuaW5mcmFkZWFk Lm9yZwpodHRwOi8vbGlzdHMuaW5mcmFkZWFkLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2xpbnV4LWFy bS1rZXJuZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 From: kdinh@apm.com (Khuong Dinh) Date: Tue, 6 Jun 2017 09:44:15 -0700 Subject: [PATCH v2 pci] PCI/MSI: pci-xgene-msi: Enable MSI support in ACPI boot for X-Gene v1 In-Reply-To: References: <1493340847-25720-1-git-send-email-kdinh@apm.com> <20170505165119.GA8614@red-moon> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Lorenzo, On Tue, May 9, 2017 at 3:55 PM, Khuong Dinh wrote: > Hi Lorenzo, > > On Fri, May 5, 2017 at 9:51 AM, Lorenzo Pieralisi > wrote: >> On Thu, May 04, 2017 at 05:36:06PM -0700, Khuong Dinh wrote: >>> Hi Marc, >>> There's no explicit dependency between pcie driver and msi >>> controller. The current solution that we did is relying on the >>> node ordering in BIOS. ACPI 5.0 introduced _DEP method to assign a >>> higher priority in start ordering. This method could be applied in >>> case of msi and pcie are the same level of subsys_init (in ACPI >>> boot). However, PCIe driver has not supported for this dependency >>> check yet. How do you think about this solution. >> >> First off, you can't post emails with confidentiality notices on >> mailing lists so remove it from now onwards. > > Fixed > >> Secondly, I commented on this before, so you know what my opinion is. > > I got your opinion. I'll implement as your suggestion. > Regarding to your previous suggestion to add a hook walking the ACPI namespace before acpi_bus_scan() in acpi_scan_init() to make MSI controllers must be probed before PCI. I have a concern that the MSI namespace ? _SB.MSIX? is not a unique name and how can we walk the ACPI namespace and use this name to make MSI probed before PCI. May you have little bit more information for this or do you have another suggestion for this? There?s another solution which makes this simpler and I?d like to ask your opinion about this. The solution is to make an hierarchy between MSI and PCI nodes as below: Device(\_SB.MSIX) { Device(\_SB.PCI0) Device(\_SB.PCI1) ?? } In other word, MSIX node is a parent node of PCI nodes in ACPI table. In this sense, there?s an explicit dependency between MSI and PCI, MSI controller must be probed before PCI and it will guarantee not breaking next kernel releases. How do you think about this solution. I also tried to use _DEP method to describe the dependency of PCIe nodes, but it looks that it does not work as PCI and MSI are handed by acpi_bus_scan and still having issue when we re-probe PCI. Thanks, Khuong Dinh >> Finally, please execute this command on the vmlinux that "works" >> for you: >> >> nm vmlinux | grep -E '__initcall_(xgene_pcie_msi_init|acpi_init)' > > $ nm vmlinux | grep -E '__initcall_(xgene_pcie_msi_init|acpi_init)' > ffff000008dab2d8 t __initcall_acpi_init4 > ffff000008dab2c8 t __initcall_xgene_pcie_msi_init4 > > Best regards, > Khuong Dinh > >> Even by ordering devices in the ACPI tables (that I abhor) I still do >> not understand how this works (I mean without relying on kernel link >> order to ensure that the MSI driver is probed before PCI devices are >> enumerated in acpi_init()). >> >> Thanks, >> Lorenzo >> >>> Best regards, >>> Khuong >>> >>> On Fri, Apr 28, 2017 at 2:27 AM, Marc Zyngier wrote: >>> > On 28/04/17 01:54, Khuong Dinh wrote: >>> >> From: Khuong Dinh >>> >> >>> >> This patch makes pci-xgene-msi driver ACPI-aware and provides >>> >> MSI capability for X-Gene v1 PCIe controllers in ACPI boot mode. >>> >> >>> >> Signed-off-by: Khuong Dinh >>> >> Signed-off-by: Duc Dang >>> >> Acked-by: Marc Zyngier >>> >> --- >>> >> v2: >>> >> - Verify with BIOS version 3.06.25 and 3.07.09 >>> >> v1: >>> >> - Initial version >>> >> --- >>> >> drivers/pci/host/pci-xgene-msi.c | 35 ++++++++++++++++++++++++++++++++--- >>> >> 1 files changed, 32 insertions(+), 3 deletions(-) >>> >> >>> >> diff --git a/drivers/pci/host/pci-xgene-msi.c b/drivers/pci/host/pci-xgene-msi.c >>> >> index f1b633b..00aaa3d 100644 >>> >> --- a/drivers/pci/host/pci-xgene-msi.c >>> >> +++ b/drivers/pci/host/pci-xgene-msi.c >>> >> @@ -24,6 +24,7 @@ >>> >> #include >>> >> #include >>> >> #include >>> >> +#include >>> >> >>> >> #define MSI_IR0 0x000000 >>> >> #define MSI_INT0 0x800000 >>> >> @@ -39,7 +40,7 @@ struct xgene_msi_group { >>> >> }; >>> >> >>> >> struct xgene_msi { >>> >> - struct device_node *node; >>> >> + struct fwnode_handle *fwnode; >>> >> struct irq_domain *inner_domain; >>> >> struct irq_domain *msi_domain; >>> >> u64 msi_addr; >>> >> @@ -249,6 +250,13 @@ static void xgene_irq_domain_free(struct irq_domain *domain, >>> >> .free = xgene_irq_domain_free, >>> >> }; >>> >> >>> >> +#ifdef CONFIG_ACPI >>> >> +static struct fwnode_handle *xgene_msi_get_fwnode(struct device *dev) >>> >> +{ >>> >> + return xgene_msi_ctrl.fwnode; >>> >> +} >>> >> +#endif >>> >> + >>> >> static int xgene_allocate_domains(struct xgene_msi *msi) >>> >> { >>> >> msi->inner_domain = irq_domain_add_linear(NULL, NR_MSI_VEC, >>> >> @@ -256,7 +264,7 @@ static int xgene_allocate_domains(struct xgene_msi *msi) >>> >> if (!msi->inner_domain) >>> >> return -ENOMEM; >>> >> >>> >> - msi->msi_domain = pci_msi_create_irq_domain(of_node_to_fwnode(msi->node), >>> >> + msi->msi_domain = pci_msi_create_irq_domain(msi->fwnode, >>> >> &xgene_msi_domain_info, >>> >> msi->inner_domain); >>> >> >>> >> @@ -265,6 +273,9 @@ static int xgene_allocate_domains(struct xgene_msi *msi) >>> >> return -ENOMEM; >>> >> } >>> >> >>> >> +#ifdef CONFIG_ACPI >>> >> + pci_msi_register_fwnode_provider(&xgene_msi_get_fwnode); >>> >> +#endif >>> >> return 0; >>> >> } >>> >> >>> >> @@ -449,6 +460,13 @@ static int xgene_msi_hwirq_free(unsigned int cpu) >>> >> {}, >>> >> }; >>> >> >>> >> +#ifdef CONFIG_ACPI >>> >> +static const struct acpi_device_id xgene_msi_acpi_ids[] = { >>> >> + {"APMC0D0E", 0}, >>> >> + { }, >>> >> +}; >>> >> +#endif >>> >> + >>> >> static int xgene_msi_probe(struct platform_device *pdev) >>> >> { >>> >> struct resource *res; >>> >> @@ -469,7 +487,17 @@ static int xgene_msi_probe(struct platform_device *pdev) >>> >> goto error; >>> >> } >>> >> xgene_msi->msi_addr = res->start; >>> >> - xgene_msi->node = pdev->dev.of_node; >>> >> + >>> >> + xgene_msi->fwnode = of_node_to_fwnode(pdev->dev.of_node); >>> >> + if (!xgene_msi->fwnode) { >>> >> + xgene_msi->fwnode = irq_domain_alloc_fwnode(NULL); >>> > >>> > Please provide something other than NULL, such as the base address if >>> > the device. That's quite useful for debugging. >>> > >>> >> + if (!xgene_msi->fwnode) { >>> >> + dev_err(&pdev->dev, "Failed to create fwnode\n"); >>> >> + rc = ENOMEM; >>> >> + goto error; >>> >> + } >>> >> + } >>> >> + >>> >> xgene_msi->num_cpus = num_possible_cpus(); >>> >> >>> >> rc = xgene_msi_init_allocator(xgene_msi); >>> >> @@ -540,6 +568,7 @@ static int xgene_msi_probe(struct platform_device *pdev) >>> >> .driver = { >>> >> .name = "xgene-msi", >>> >> .of_match_table = xgene_msi_match_table, >>> >> + .acpi_match_table = ACPI_PTR(xgene_msi_acpi_ids), >>> >> }, >>> >> .probe = xgene_msi_probe, >>> >> .remove = xgene_msi_remove, >>> >> >>> > >>> > The code is trivial, but relies on the MSI controller to probe before >>> > the PCI bus. What enforces this? How is it making sure that this is not >>> > going to break in the next kernel release? As far as I can tell, there >>> > is no explicit dependency between the two, making it the whole thing >>> > extremely fragile. >>> > >>> > Thanks, >>> > >>> > M. >>> > -- >>> > Jazz is not dead. It just smells funny... >>> >>> --