Re: [RFC PATCH V2] acpi/irq: Add stacked IRQ domain support to PCI interrupt link

[Ard Biesheuvel]

On Tue, 17 Nov 2020 at 19:57, Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> Nit: please don't just make up random styles for the subject.  Run
> "git log --oneline" on the file and/or the directory and try to follow
> the existing convention.  Using random styles adds noise to the
> system.
>
> On Tue, Nov 17, 2020 at 09:42:14PM +0800, Chen Baozi wrote:
> > Some PCIe designs require software to do extra acknowledgements for
> > legacy INTx interrupts. If the driver is written only for device tree,
> > things are simple. In that case, a new driver can be written under
> > driver/pci/controller/ with a DT node of PCIe host written like:
> >
> >   pcie {
> >     ...
> >     interrupt-map = <0 0 0  1  &pcie_intc 0>,
> >                     <0 0 0  2  &pcie_intc 1>,
> >                     <0 0 0  3  &pcie_intc 2>,
> >                     <0 0 0  4  &pcie_intc 3>;
> >
> >     pcie_intc: legacy-interrupt-controller {
> >       interrupt-controller;
> >       #interrupt-cells = <1>;
> >       interrupt-parent = <&gic>;
> >       interrupts = <0 226 4>;
> >     };
> >   };
> >
> > Similar designs can be found on Aardvark, MediaTek Gen2 and Socionext
> > UniPhier PCIe controller at the moment. Essentially, those designs are
> > supported by inserting an extra interrupt controller between PCIe host
> > and GIC and parse the topology in a DT-based PCI controller driver.
>
> If I understand correctly, we previously ignored the Resource Source
> field of an Extended Interrupt Descriptor in the _PRS method of
> PNP0C0F PCI Interrupt Link devices, and this patch adds support for
> it.
>
> If that's true, this has nothing to do with DT, other than DT being
> another way to describe the same topology, and the above details
> really aren't relevant to this patch.
>
> > As we turn to ACPI, All the PCIe hosts are described the same ID of
> > "PNP0A03" and share driver/acpi/pci_root.c. It comes to be a problem
> > to make this kind of PCI INTx work under ACPI.
>
> s/All the PCIe/all the PCIe/
>
> But this paragraph should probably just go away in favor of something
> about implementing Resource Source support.
>
> > Therefore, we introduce an stacked IRQ domain support to PCI interrupt
> > link for ACPI. With this support, we can populate the ResourceSource
> > to refer to a device object that describes an interrupt controller.
> > That would allow us to refer to a dedicated driver which implements
> > the logic needed to manage the interrupt state. With this patch,
> > those PCI interrupt links can be supported by describing the INTx
> > in ACPI table as the following example:
>
> "Stacked IRQ domain" sounds like a detail of how you're implementing
> support for the Resource Source field for PCI Interrupt Links.
>
> I don't know what the dedicated driver refers to.  This *should* be
> all generic code the follows the ACPI spec (which is pretty sketchy in
> this area).  But I assume that there's no special driver needed for
> devices like \SB.IXIU, and the logic associated with the interrupt
> controller is in the AML associated with IXIU.  It would probably be
> useful to mention the relevant methods in the IXIU methods in the
> example below.
>

As I understand it, the intent is to provide a driver for \SB.IXIU
that acknowledges the legacy INTx interrupts in a SoC specific way,
and I don't see how AML could be involved here.

That also explains why the routines are exported to modules - the IXIU
driver could be modularized.


> From ACPI v6.3, Table 6-200, it looks like this patch should include
> changes to acpi_bus_osc_support() to advertise "Interrupt
> ResourceSource support".
>

I assume this covers all uses of ResourceSource, right? Not only in
the context if PCIe legacy interrupts?

> >   Device (IXIU) {
> >     ...
> >   }
> >
> >   Device(LINKA) {
> >     Name(_HID, EISAID("PNP0C0F"))
> >     Name(_PRS, ResourceTemplate(){
> >       Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive, 0, "\\SB.IXIU")
> >         { 60 }
> >     })
> >     ...
> >   }
> >
> >   Device(PCI0) {
> >     ...
> >     Name(_PRT, Package{
> >       Package{ 0x0000FFFF, 0, LINKA, 0 }
> >       ...
> >     })
> >   }
> >
> > Signed-off-by: Chen Baozi <chenbaozi@xxxxxxxxxxxxxx>
> > ---
> >  drivers/acpi/irq.c          | 22 +++++++++++++++++++++-
> >  drivers/acpi/pci_irq.c      |  6 ++++--
> >  drivers/acpi/pci_link.c     | 17 +++++++++++++++--
> >  include/acpi/acpi_drivers.h |  2 +-
> >  include/linux/acpi.h        |  4 ++++
> >  5 files changed, 45 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/acpi/irq.c b/drivers/acpi/irq.c
> > index e209081d644b..e78a44815c44 100644
> > --- a/drivers/acpi/irq.c
> > +++ b/drivers/acpi/irq.c
> > @@ -81,6 +81,25 @@ void acpi_unregister_gsi(u32 gsi)
> >  }
> >  EXPORT_SYMBOL_GPL(acpi_unregister_gsi);
> >
> > +int acpi_register_irq(struct device *dev, u32 irq, int trigger,
> > +                   int polarity, struct fwnode_handle *domain_id)
> > +{
> > +     struct irq_fwspec fwspec;
> > +
> > +     if (WARN_ON(!domain_id)) {
> > +             pr_warn("GSI: No registered irqchip, giving up\n");
>
> This message could contain more information, e.g., by using
> dev_warn(), including the irq value, etc.
>
> > +             return -EINVAL;
> > +     }
> > +
> > +     fwspec.fwnode = domain_id;
> > +     fwspec.param[0] = irq;
> > +     fwspec.param[1] = acpi_dev_get_irq_type(trigger, polarity);
> > +     fwspec.param_count = 2;
> > +
> > +     return irq_create_fwspec_mapping(&fwspec);
> > +}
> > +EXPORT_SYMBOL_GPL(acpi_register_irq);
>
> Why does this need to be exported?  You only call it from
> acpi_pci_irq_enable(), which cannot be a module.
>
> >  /**
> >   * acpi_get_irq_source_fwhandle() - Retrieve fwhandle from IRQ resource source.
> >   * @source: acpi_resource_source to use for the lookup.
> > @@ -92,7 +111,7 @@ EXPORT_SYMBOL_GPL(acpi_unregister_gsi);
> >   * Return:
> >   * The referenced device fwhandle or NULL on failure
> >   */
> > -static struct fwnode_handle *
> > +struct fwnode_handle *
> >  acpi_get_irq_source_fwhandle(const struct acpi_resource_source *source)
> >  {
> >       struct fwnode_handle *result;
> > @@ -115,6 +134,7 @@ acpi_get_irq_source_fwhandle(const struct acpi_resource_source *source)
> >       acpi_bus_put_acpi_device(device);
> >       return result;
> >  }
> > +EXPORT_SYMBOL_GPL(acpi_get_irq_source_fwhandle);
>
> This doesn't look like it needs to be exported either.
>
> >  /*
> >   * Context for the resource walk used to lookup IRQ resources.
> > diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
> > index 14ee631cb7cf..19296d70c95c 100644
> > --- a/drivers/acpi/pci_irq.c
> > +++ b/drivers/acpi/pci_irq.c
> > @@ -410,6 +410,7 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
> >       char *link = NULL;
> >       char link_desc[16];
> >       int rc;
> > +     struct fwnode_handle *irq_domain;
> >
> >       pin = dev->pin;
> >       if (!pin) {
> > @@ -438,7 +439,8 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
> >                       gsi = acpi_pci_link_allocate_irq(entry->link,
> >                                                        entry->index,
> >                                                        &triggering, &polarity,
> > -                                                      &link);
> > +                                                      &link,
> > +                                                      &irq_domain);
> >               else
> >                       gsi = entry->index;
> >       } else
> > @@ -462,7 +464,7 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
> >               return 0;
> >       }
> >
> > -     rc = acpi_register_gsi(&dev->dev, gsi, triggering, polarity);
> > +     rc = acpi_register_irq(&dev->dev, gsi, triggering, polarity, irq_domain);
> >       if (rc < 0) {
> >               dev_warn(&dev->dev, "PCI INT %c: failed to register GSI\n",
> >                        pin_name(pin));
> > diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> > index fb4c5632a232..219a644d739a 100644
> > --- a/drivers/acpi/pci_link.c
> > +++ b/drivers/acpi/pci_link.c
> > @@ -59,6 +59,7 @@ struct acpi_pci_link_irq {
> >       u8 resource_type;
> >       u8 possible_count;
> >       u32 possible[ACPI_PCI_LINK_MAX_POSSIBLE];
> > +     struct acpi_resource_source resource_source;
> >       u8 initialized:1;
> >       u8 reserved:7;
> >  };
> > @@ -120,6 +121,8 @@ static acpi_status acpi_pci_link_check_possible(struct acpi_resource *resource,
> >               {
> >                       struct acpi_resource_extended_irq *p =
> >                           &resource->data.extended_irq;
> > +                     struct acpi_resource_source *rs =
> > +                         &link->irq.resource_source;
> >                       if (!p || !p->interrupt_count) {
> >                               printk(KERN_WARNING PREFIX
> >                                             "Blank _PRS EXT IRQ resource\n");
> > @@ -140,6 +143,12 @@ static acpi_status acpi_pci_link_check_possible(struct acpi_resource *resource,
> >                       link->irq.triggering = p->triggering;
> >                       link->irq.polarity = p->polarity;
> >                       link->irq.resource_type = ACPI_RESOURCE_TYPE_EXTENDED_IRQ;
> > +                     if (p->resource_source.string_length) {
> > +                             rs->index = p->resource_source.index;
> > +                             rs->string_length = p->resource_source.string_length;
> > +                             rs->string_ptr = kmalloc(rs->string_length, GFP_KERNEL);
> > +                             strcpy(rs->string_ptr, p->resource_source.string_ptr);
> > +                     }
> >                       break;
> >               }
> >       default:
> > @@ -326,7 +335,8 @@ static int acpi_pci_link_set(struct acpi_pci_link *link, int irq)
> >                       resource->res.data.extended_irq.shareable = ACPI_SHARED;
> >               resource->res.data.extended_irq.interrupt_count = 1;
> >               resource->res.data.extended_irq.interrupts[0] = irq;
> > -             /* ignore resource_source, it's optional */
> > +             resource->res.data.extended_irq.resource_source =
> > +                     link->irq.resource_source;
> >               break;
> >       default:
> >               printk(KERN_ERR PREFIX "Invalid Resource_type %d\n", link->irq.resource_type);
> > @@ -612,7 +622,7 @@ static int acpi_pci_link_allocate(struct acpi_pci_link *link)
> >   * failure: return -1
> >   */
> >  int acpi_pci_link_allocate_irq(acpi_handle handle, int index, int *triggering,
> > -                            int *polarity, char **name)
> > +                            int *polarity, char **name, struct fwnode_handle **irq_domain)
> >  {
> >       int result;
> >       struct acpi_device *device;
> > @@ -656,6 +666,9 @@ int acpi_pci_link_allocate_irq(acpi_handle handle, int index, int *triggering,
> >               *polarity = link->irq.polarity;
> >       if (name)
> >               *name = acpi_device_bid(link->device);
> > +     if (irq_domain)
> > +             *irq_domain = acpi_get_irq_source_fwhandle(&link->irq.resource_source);
> > +
> >       ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> >                         "Link %s is referenced\n",
> >                         acpi_device_bid(link->device)));
> > diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
> > index 5eb175933a5b..6ff1ea76d476 100644
> > --- a/include/acpi/acpi_drivers.h
> > +++ b/include/acpi/acpi_drivers.h
> > @@ -68,7 +68,7 @@
> >
> >  int acpi_irq_penalty_init(void);
> >  int acpi_pci_link_allocate_irq(acpi_handle handle, int index, int *triggering,
> > -                            int *polarity, char **name);
> > +                            int *polarity, char **name, struct fwnode_handle **irq_domain);
> >  int acpi_pci_link_free_irq(acpi_handle handle);
> >
> >  /* ACPI PCI Device Binding (pci_bind.c) */
> > diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> > index 39263c6b52e1..5f1d7d3192fb 100644
> > --- a/include/linux/acpi.h
> > +++ b/include/linux/acpi.h
> > @@ -324,6 +324,8 @@ extern int sbf_port;
> >  extern unsigned long acpi_realmode_flags;
> >
> >  int acpi_register_gsi (struct device *dev, u32 gsi, int triggering, int polarity);
> > +int acpi_register_irq(struct device *dev, u32 gsi, int trigger,
> > +                   int polarity, struct fwnode_handle *domain_id);
>
> It looks like this declaration should be in drivers/acpi/internal.h,
> since it's only used inside drivers/acpi/.
>
> >  int acpi_gsi_to_irq (u32 gsi, unsigned int *irq);
> >  int acpi_isa_irq_to_gsi (unsigned isa_irq, u32 *gsi);
> >
> > @@ -336,6 +338,8 @@ struct irq_domain *acpi_irq_create_hierarchy(unsigned int flags,
> >                                            const struct irq_domain_ops *ops,
> >                                            void *host_data);
> >
> > +struct fwnode_handle *acpi_get_irq_source_fwhandle(const struct acpi_resource_source *source);
> > +
> >  #ifdef CONFIG_X86_IO_APIC
> >  extern int acpi_get_override_irq(u32 gsi, int *trigger, int *polarity);
> >  #else
> > --
> > 2.28.0
> >