* Re: [RFC PATCH V2] acpi/irq: Add stacked IRQ domain support to PCI interrupt link
2020-11-17 13:42 [RFC PATCH V2] acpi/irq: Add stacked IRQ domain support to PCI interrupt link Chen Baozi
@ 2020-11-17 18:57 ` Bjorn Helgaas
2020-11-18 10:37 ` Chen Baozi
2020-11-18 13:45 ` Ard Biesheuvel
2020-11-18 9:27 ` Marc Zyngier
2020-11-18 9:51 ` Lorenzo Pieralisi
2 siblings, 2 replies; 10+ messages in thread
From: Bjorn Helgaas @ 2020-11-17 18:57 UTC (permalink / raw)
To: Chen Baozi
Cc: Ard Biesheuvel, Hanjun Guo, Marc Zyngier, Bjorn Helgaas,
linux-acpi, linux-pci, linux-arm-kernel, linux-kernel,
Lorenzo Pieralisi
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.
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".
> 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@phytium.com.cn>
> ---
> 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
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [RFC PATCH V2] acpi/irq: Add stacked IRQ domain support to PCI interrupt link
2020-11-17 18:57 ` Bjorn Helgaas
@ 2020-11-18 10:37 ` Chen Baozi
2020-11-18 13:45 ` Ard Biesheuvel
1 sibling, 0 replies; 10+ messages in thread
From: Chen Baozi @ 2020-11-18 10:37 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Ard Biesheuvel, Guohanjun, Marc Zyngier, Bjorn Helgaas,
linux-acpi, linux-pci, linux-arm-kernel, linux-kernel,
Lorenzo Pieralisi
Hi Bjorn,
Thanks for reviewing. I’ll try to fix all existing issues and send a new
version later.
> On Nov 18, 2020, at 2:57 AM, Bjorn Helgaas <helgaas@kernel.org> 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:
>>
>> 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.
>
> 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".
According to my understanding, does it mean to add something like：
+ capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_INTR_RS_SUPPORT;
and check whether the platform supports usage of ResourceSource after
acpi_run_osc() returns successfully:
+ osc_sb_intr_rs_support_confirmed =
+ capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_INTR_RS_SUPPORT;
with this bool value, we can then determine if we would ignore the
ResourceSource field later.
Or we just advertise this capability from the OS side without introducing
the ‘osc_sb_intr_rs_support_confirmed’?
I am not certain about it because:
1) If we strictly flow the spec, which says “the platform will indicate to
OS whether ... If not set, the OS may choose to ignore the ResourceSource
parameter in the extended interrupt descirptor”, this means this capability
can be used to determine whether we would ignore to parse the field later.
2) On the other hand, Since the ResourceSource has already been used to
create hierarchical domain for platform device (introduced by 621dc2fdcea1)
and previous driver does not check this capability, I am not sure whether
it would break the existing platforms.
Fix me if I am wrong.
Cheers,
Chen Baozi.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH V2] acpi/irq: Add stacked IRQ domain support to PCI interrupt link
2020-11-17 18:57 ` Bjorn Helgaas
2020-11-18 10:37 ` Chen Baozi
@ 2020-11-18 13:45 ` Ard Biesheuvel
2020-11-18 13:50 ` Rafael J. Wysocki
1 sibling, 1 reply; 10+ messages in thread
From: Ard Biesheuvel @ 2020-11-18 13:45 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Chen Baozi, Hanjun Guo, Marc Zyngier, Bjorn Helgaas,
ACPI Devel Maling List, PCI, Linux ARM,
Linux Kernel Mailing List, Lorenzo Pieralisi
On Tue, 17 Nov 2020 at 19:57, Bjorn Helgaas <helgaas@kernel.org> 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@phytium.com.cn>
> > ---
> > 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
> >
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [RFC PATCH V2] acpi/irq: Add stacked IRQ domain support to PCI interrupt link
2020-11-18 13:45 ` Ard Biesheuvel
@ 2020-11-18 13:50 ` Rafael J. Wysocki
0 siblings, 0 replies; 10+ messages in thread
From: Rafael J. Wysocki @ 2020-11-18 13:50 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Bjorn Helgaas, Chen Baozi, Hanjun Guo, Marc Zyngier,
Bjorn Helgaas, ACPI Devel Maling List, PCI, Linux ARM,
Linux Kernel Mailing List, Lorenzo Pieralisi
On Wed, Nov 18, 2020 at 2:46 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Tue, 17 Nov 2020 at 19:57, Bjorn Helgaas <helgaas@kernel.org> 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.
OK, but every new symbol export requires an in-the-tree user or the
patch is basically not applicable.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH V2] acpi/irq: Add stacked IRQ domain support to PCI interrupt link
2020-11-17 13:42 [RFC PATCH V2] acpi/irq: Add stacked IRQ domain support to PCI interrupt link Chen Baozi
2020-11-17 18:57 ` Bjorn Helgaas
@ 2020-11-18 9:27 ` Marc Zyngier
2020-11-18 13:36 ` Chen Baozi
2020-11-18 9:51 ` Lorenzo Pieralisi
2 siblings, 1 reply; 10+ messages in thread
From: Marc Zyngier @ 2020-11-18 9:27 UTC (permalink / raw)
To: Chen Baozi
Cc: Ard Biesheuvel, Hanjun Guo, Bjorn Helgaas, linux-acpi, linux-pci,
linux-arm-kernel, linux-kernel, Lorenzo Pieralisi
Hi Chen,
On top of Bjorn's comments:
On 2020-11-17 13:42, 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.
> 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.
>
> 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:
>
> 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@phytium.com.cn>
> ---
> 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");
A fwnode_handle is not an irqchip. It's just an opaque identifier
for a HW block. Furthermore, there is no need to have both a WARN_ON()
and a pr_warn(). Please pick one.
I'd also suggest you rename domain_id to fwnode, which is the commonly
used idiom (yes, I know about the unfortunate precedent in
acpi_register_gsi()).
> + 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);
By the way, this is almost an exact duplicate of acpi_register_gsi().
You definitely want to make this code common.
> +
> /**
> * 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);
>
> /*
> * 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;
fwnode_handle is most definitely not an 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);
We have kstrdup() for this kind of things, as using rs->string_length to
allocate
the buffer and strcpy() to copy it feels... dangerous.
> + }
> 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)
Same remark about the naming.
> {
> 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);
> 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
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [RFC PATCH V2] acpi/irq: Add stacked IRQ domain support to PCI interrupt link
2020-11-18 9:27 ` Marc Zyngier
@ 2020-11-18 13:36 ` Chen Baozi
0 siblings, 0 replies; 10+ messages in thread
From: Chen Baozi @ 2020-11-18 13:36 UTC (permalink / raw)
To: Marc Zyngier
Cc: Lorenzo Pieralisi, linux-pci, linux-kernel, linux-acpi,
Guohanjun, Bjorn Helgaas, Ard Biesheuvel, linux-arm-kernel
Hi Marc,
> On Nov 18, 2020, at 5:27 PM, Marc Zyngier <maz@kernel.org> wrote:
>
> Hi Chen,
>
> On top of Bjorn's comments:
>
> On 2020-11-17 13:42, Chen Baozi wrote:
>>
>> ---
>> 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");
>
> A fwnode_handle is not an irqchip. It's just an opaque identifier
> for a HW block. Furthermore, there is no need to have both a WARN_ON()
> and a pr_warn(). Please pick one.
>
> I'd also suggest you rename domain_id to fwnode, which is the commonly
> used idiom (yes, I know about the unfortunate precedent in acpi_register_gsi()).
>
>> + 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);
>
> By the way, this is almost an exact duplicate of acpi_register_gsi().
> You definitely want to make this code common.
>
>> @@ -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);
>> /*
>> * 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;
>
> fwnode_handle is most definitely not an IRQ domain.
>
>> @@ -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);
>
> We have kstrdup() for this kind of things, as using rs->string_length to allocate
> the buffer and strcpy() to copy it feels... dangerous.
>
>> + }
>> break;
>> }
>> default:
>> @@ -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)
>
> Same remark about the naming.
Thanks. It is very helpful. I’ll fix it in next version.
Baozi.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH V2] acpi/irq: Add stacked IRQ domain support to PCI interrupt link
2020-11-17 13:42 [RFC PATCH V2] acpi/irq: Add stacked IRQ domain support to PCI interrupt link Chen Baozi
2020-11-17 18:57 ` Bjorn Helgaas
2020-11-18 9:27 ` Marc Zyngier
@ 2020-11-18 9:51 ` Lorenzo Pieralisi
2020-11-18 14:05 ` Chen Baozi
2 siblings, 1 reply; 10+ messages in thread
From: Lorenzo Pieralisi @ 2020-11-18 9:51 UTC (permalink / raw)
To: Chen Baozi
Cc: Ard Biesheuvel, Hanjun Guo, Marc Zyngier, Bjorn Helgaas,
linux-acpi, linux-pci, linux-arm-kernel, linux-kernel
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.
> 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.
In this respect this patch is a minor detail. The major detail is how
those host controllers are going to probe and initialize with ACPI and I
am against merging this patch stand alone with no user before
understanding what you really want to do with those host controller
drivers in the ACPI world.
Side note, there is ongoing work for a generic interrupt MUX:
https://bugzilla.tianocore.org/show_bug.cgi?id=2995
If we ever come to support those MUXes with ACPI that must be a
starting point, the binding above can be your first "user".
I still have reservations about bootstrapping the host controllers
you mentioned in platforms with no firmware support whatsoever for
PCI initialization (eg address decoders, link bring-up, etc. - the
ACPI host bridge model relies on FW to carry out that initialization)
with ACPI - I would like to see the whole picture first.
Lorenzo
> 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:
>
> 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@phytium.com.cn>
> ---
> 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");
> + 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);
> +
> /**
> * 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);
>
> /*
> * 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);
> 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
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [RFC PATCH V2] acpi/irq: Add stacked IRQ domain support to PCI interrupt link
2020-11-18 9:51 ` Lorenzo Pieralisi
@ 2020-11-18 14:05 ` Chen Baozi
2020-11-19 10:37 ` Lorenzo Pieralisi
0 siblings, 1 reply; 10+ messages in thread
From: Chen Baozi @ 2020-11-18 14:05 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Marc Zyngier, Guohanjun, linux-kernel, linux-acpi, linux-pci,
Bjorn Helgaas, Ard Biesheuvel, linux-arm-kernel
Hi Lorenzo,
> On Nov 18, 2020, at 5:51 PM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
>
> 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.
>> 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.
>
> In this respect this patch is a minor detail. The major detail is how
> those host controllers are going to probe and initialize with ACPI and I
> am against merging this patch stand alone with no user before
> understanding what you really want to do with those host controller
> drivers in the ACPI world.
>
> Side note, there is ongoing work for a generic interrupt MUX:
>
> https://bugzilla.tianocore.org/show_bug.cgi?id=2995
>
> If we ever come to support those MUXes with ACPI that must be a
> starting point, the binding above can be your first "user".
>
> I still have reservations about bootstrapping the host controllers
> you mentioned in platforms with no firmware support whatsoever for
> PCI initialization (eg address decoders, link bring-up, etc. - the
> ACPI host bridge model relies on FW to carry out that initialization)
> with ACPI - I would like to see the whole picture first.
Frankly, I’m also waiting for my first “user” to be announced at the moment,
so that I can make the whole picture clearer. And it is why I mark this
patch as an RFC.
Yes. I admit it is a little weird to add another interrupt controller
between the GIC and INTx device. But if it is not only about
initialization but also about hooking into the INTx processing (e.g.,
introduce an extra ack operation...), it seems we cannot only rely
on FW. I have looked for a FW solution without introducing a new
driver later but failed... I’m happy to be fixed if there is a pure
FW solution.
Thanks.
Baozi.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [RFC PATCH V2] acpi/irq: Add stacked IRQ domain support to PCI interrupt link
2020-11-18 14:05 ` Chen Baozi
@ 2020-11-19 10:37 ` Lorenzo Pieralisi
0 siblings, 0 replies; 10+ messages in thread
From: Lorenzo Pieralisi @ 2020-11-19 10:37 UTC (permalink / raw)
To: Chen Baozi
Cc: Marc Zyngier, Guohanjun, linux-kernel, linux-acpi, linux-pci,
Bjorn Helgaas, Ard Biesheuvel, linux-arm-kernel
On Wed, Nov 18, 2020 at 10:05:29PM +0800, Chen Baozi wrote:
> Hi Lorenzo,
>
> > On Nov 18, 2020, at 5:51 PM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> >
> > 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.
> >> 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.
> >
> > In this respect this patch is a minor detail. The major detail is how
> > those host controllers are going to probe and initialize with ACPI and I
> > am against merging this patch stand alone with no user before
> > understanding what you really want to do with those host controller
> > drivers in the ACPI world.
> >
> > Side note, there is ongoing work for a generic interrupt MUX:
> >
> > https://bugzilla.tianocore.org/show_bug.cgi?id=2995
> >
> > If we ever come to support those MUXes with ACPI that must be a
> > starting point, the binding above can be your first "user".
> >
> > I still have reservations about bootstrapping the host controllers
> > you mentioned in platforms with no firmware support whatsoever for
> > PCI initialization (eg address decoders, link bring-up, etc. - the
> > ACPI host bridge model relies on FW to carry out that initialization)
> > with ACPI - I would like to see the whole picture first.
>
> Frankly, I’m also waiting for my first “user” to be announced at the
> moment, so that I can make the whole picture clearer. And it is why I
> mark this patch as an RFC.
AFAIK none of the host controllers requiring this IRQ muxing is shipped
with a firmware stack that can bootstrap them with ACPI.
That's why I think this patch is a thought exercise, there is not much
to talk about.
> Yes. I admit it is a little weird to add another interrupt controller
> between the GIC and INTx device. But if it is not only about
> initialization but also about hooking into the INTx processing (e.g.,
> introduce an extra ack operation...), it seems we cannot only rely
> on FW. I have looked for a FW solution without introducing a new
> driver later but failed... I’m happy to be fixed if there is a pure
> FW solution.
I did not say that to solve the INTX muxing we can use FW. I said that
to probe the host controllers that require this muxing there must be
firmware in place (to allow probing them with ACPI) before we look at
solving the PCI INTX muxing issue.
We should not solve issues that don't exist ;-)
Thanks,
Lorenzo
^ permalink raw reply [flat|nested] 10+ messages in thread