Re: [PATCH v2 pci] PCI/MSI: pci-xgene-msi: Enable MSI support in ACPI boot for X-Gene v1

From: Khuong Dinh
Date: Wed Jun 14 2017 - 13:43:30 EST

On Wed, Jun 14, 2017 at 5:59 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@xxxxxxx> wrote:
> On Tue, Jun 13, 2017 at 01:56:44PM -0700, Khuong Dinh wrote:
>> Hi Lorenzo,
>>
>> On Thu, Jun 8, 2017 at 3:39 AM, Lorenzo Pieralisi
>> <lorenzo.pieralisi@xxxxxxx> wrote:
>> > On Tue, Jun 06, 2017 at 09:44:15AM -0700, Khuong Dinh wrote:
>> >> Hi Lorenzo,
>> >>
>> >> On Tue, May 9, 2017 at 3:55 PM, Khuong Dinh <kdinh@xxxxxxx> wrote:
>> >> > Hi Lorenzo,
>> >> >
>> >> > On Fri, May 5, 2017 at 9:51 AM, Lorenzo Pieralisi
>> >> > <lorenzo.pieralisi@xxxxxxx> 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 think that's a plaster as ineffective as reordering nodes, in short
>> > it is not a solution and you still rely on kernel link ordering, you
>> > can fiddle with ACPI tables as much as you want but that does not change.
>> >
>> >> 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.
>> >
>> > That's a tad vague to be frank, anyway it is pretty clear to me that we
>> > have hit a wall. In ACPI there is no way to describe probe dependencies
>> > like the one you have, it is as simple as that, and this MSI issue you
>> > are facing is just an example, there are more eg:
>> >
>> > https://www.spinics.net/lists/arm-kernel/msg585825.html
>> >
>> > At the end of the day the choice is simple either we accept (and we
>> > maintain because that's the problem) these hacks - and I am not willing
>> > to do that - or we find a way to solve this from a general perspective not
>> > as a point hack.
>> >
>> > I can have a look at solving the whole issue but it won't happen
>> > tomorrow.
>>
>> Thanks for helping to resolve this general ACPI dependence issue.
>> I look forward for a version to test with.
>>
>> Can we separate the general dependence patch from the X-Gene MSI ACPI
>> enable patch.
>> This original patch was to enable ACPI support for PCIe MSI.
>> The dependence issue can be resolved separately.
>> Can you help to pull in the patch.
>
> Ok, pragmatically the only sane thing you can do is:
>
> (1) Add an X-gene specific hook in drivers/acpi/scan.c (acpi_scan_init())
> that carries out fwnode registration (ie register the fwnode by
> searching the MSI HID object - this does not depend on ACPI device
> nodes order - you enforce the order by parsing the namespace before
> ACPI core does it, at that point in time ACPI objects are created).
> Not sure Rafael will be happy to see this code but that's the only
> solution that does not rely on ACPI device nodes ordering and kernel
> link ordering. You may achieve the same by extending the ACPI APD
> code (drivers/acpi/acpi_apd.c) whether that's the best way forward is
> to be seen.

Hi Rafael,
Do you have any idea about this suggestion?
Otherwise, I'll follow this Lorenzo's approach (1).

> (2) You can also add an xgene specific scan handler at arch_init_call()
> but given that PCI root bridge is managed through a scan handler too you
> would rely on ACPI devices nodes ordering in the DSDT. Let me explain
> something for the benefit of everyone reading this thread: I do not want
> X-gene ACPI tables to force the kernel to scan devices in any order
> whatsover because this would mean we will _never_ be able to change the
> scan order if we wanted to lest we trigger regressions. So this is
> a non-option in my opinion.
>
> (3) Last option is to register the MSI fwnode either in PCI ECAM quirk
> handling or in arm64 before probing the PCI root bus.
>
> I am not keen on (2) and (3) at all, so _if_ we have to find a solution
> to this problem (1) is the only option you have for the time being as
> far as I am concerned.

Thank you very much for your suggestions, Lorenzo.
If there's not any other idea, I'll follow your approach (1)

Best regards,
Khuong Dinh

> Lorenzo
>
>>
>> Best regards,
>> Khuong Dinh
>>
>> > Thanks,
>> > Lorenzo
>> >
>> >> 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 <marc.zyngier@xxxxxxx> wrote:
>> >> >>> > On 28/04/17 01:54, Khuong Dinh wrote:
>> >> >>> >> From: Khuong Dinh <kdinh@xxxxxxx>
>> >> >>> >>
>> >> >>> >> 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@xxxxxxx>
>> >> >>> >> Signed-off-by: Duc Dang <dhdang@xxxxxxx>
>> >> >>> >> Acked-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>> >> >>> >> ---
>> >> >>> >> 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...
>> >> >>>
>> >> >>> --