Message-ID: <CAAsHzqttOoWOCAEmYaWjWmH4a--o0FXCRKfFC9S-+3KGiZVuHQ@mail.gmail.com>
Date:   Tue, 9 May 2017 15:55:37 -0700
From:   Khuong Dinh <kdinh@....com>
To:     Lorenzo Pieralisi <lorenzo.pieralisi@....com>
Cc:     Marc Zyngier <marc.zyngier@....com>, msalter@...hat.com,
        Bjorn Helgaas <bhelgaas@...gle.com>, linux-pci@...r.kernel.org,
        jcm@...hat.com, patches@....com, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, rjw@...ysocki.net,
        Duc Dang <dhdang@....com>
Subject: Re: [PATCH v2 pci] PCI/MSI: pci-xgene-msi: Enable MSI support in ACPI
 boot for X-Gene v1

Hi Lorenzo,

On Fri, May 5, 2017 at 9:51 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@....com> 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.

> 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 <marc.zyngier@....com> wrote:
>> > On 28/04/17 01:54, Khuong Dinh wrote:
>> >> From: Khuong Dinh <kdinh@....com>
>> >>
>> >> 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 <kdinh@....com>
>> >> Signed-off-by: Duc Dang <dhdang@....com>
>> >> Acked-by: Marc Zyngier <marc.zyngier@....com>
>> >> ---
>> >> 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 <linux/pci.h>
>> >>  #include <linux/platform_device.h>
>> >>  #include <linux/of_pci.h>
>> >> +#include <linux/acpi.h>
>> >>
>> >>  #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...
>>
>> --
>> CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is
>> for the sole use of the intended recipient(s) and contains information that
>> is confidential and proprietary to Applied Micro Circuits Corporation or
>> its subsidiaries. It is to be used solely for the purpose of furthering the
>> parties' business relationship. All unauthorized review, use, disclosure or
>> distribution is prohibited. If you are not the intended recipient, please
>> contact the sender by reply e-mail and destroy all copies of the original
>> message.

Powered by blists - more mailing lists