Re: [PATCH V1 1/2] irqchip/loongson-pch-pic: Support to set irq type for ACPI path

From: Jianmin Lv
Date: Thu Sep 29 2022 - 09:35:16 EST



On 2022/9/29 下午5:44, Marc Zyngier wrote:
On Wed, 28 Sep 2022 22:35:17 -0400,
Jianmin Lv <lvjianmin@xxxxxxxxxxx> wrote:

On 2022/9/28 下午10:49, Marc Zyngier wrote:
On Mon, 15 Aug 2022 22:01:30 -0400,
Jianmin Lv <lvjianmin@xxxxxxxxxxx> wrote:

For ACPI path, the translate callback used IRQ_TYPE_NONE and ignored
the irq type in fwspec->param[1]. For supporting to set type for
irqs of the irqdomain, fwspec->param[1] should be used to get irq
type.

On Loongson platform, the irq trigger type of PCI devices is
high level, so high level triggered type is inputed to acpi_register_gsi
when create irq mapping for PCI devices.

Signed-off-by: Jianmin Lv <lvjianmin@xxxxxxxxxxx>
---
drivers/acpi/pci_irq.c | 3 ++-
drivers/irqchip/irq-loongson-pch-pic.c | 10 ++++++----
2 files changed, 8 insertions(+), 5 deletions(-)

$ ./scripts/get_maintainer.pl drivers/acpi/pci_irq.c
Bjorn Helgaas <bhelgaas@xxxxxxxxxx> (supporter:PCI SUBSYSTEM)
"Rafael J. Wysocki" <rafael@xxxxxxxxxx> (supporter:ACPI)
Len Brown <lenb@xxxxxxxxxx> (reviewer:ACPI)
linux-pci@xxxxxxxxxxxxxxx (open list:PCI SUBSYSTEM)
linux-acpi@xxxxxxxxxxxxxxx (open list:ACPI)
linux-kernel@xxxxxxxxxxxxxxx (open list)

How about you start Cc-ing some of the relevant people?

Ok, thanks, I'll cc relevant people list here.


diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
index 08e1577..34483b3 100644
--- a/drivers/acpi/pci_irq.c
+++ b/drivers/acpi/pci_irq.c
@@ -393,7 +393,8 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
* controller and must therefore be considered active high
* as default.
*/
- int polarity = acpi_irq_model == ACPI_IRQ_MODEL_GIC ?
+ int polarity = acpi_irq_model == ACPI_IRQ_MODEL_GIC ||
+ acpi_irq_model == ACPI_IRQ_MODEL_LPIC ?
ACPI_ACTIVE_HIGH : ACPI_ACTIVE_LOW;

The comment just above this only talks about ARM. Should it be
updated?

Ok, I'll update the comment.


Is this a limitation of the underlying interrupt controller?

It's the limitation that pci interrupt source of LoongArch only sends
high level trigger signal to interrupt controller(though, pci spec
requires asserted low).

Right, so this is the opposite problem ARM has.

But is it *always* intended to be built like this? Or is it a one-off
for this generation of Loongarch systems, to be fixed at a later time?


Yes, new generations will always keep this unchanged.



char *link = NULL;
char link_desc[16];
diff --git a/drivers/irqchip/irq-loongson-pch-pic.c b/drivers/irqchip/irq-loongson-pch-pic.c
index b6f1392..5067010 100644
--- a/drivers/irqchip/irq-loongson-pch-pic.c
+++ b/drivers/irqchip/irq-loongson-pch-pic.c
@@ -177,13 +177,15 @@ static int pch_pic_domain_translate(struct irq_domain *d,
if (fwspec->param_count < 1)
return -EINVAL;
- if (of_node) {
+ if (of_node)
*hwirq = fwspec->param[0] + priv->ht_vec_base;
- *type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
- } else {
+ else
*hwirq = fwspec->param[0] - priv->gsi_base;
+
+ if (fwspec->param_count > 1)
+ *type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
+ else
*type = IRQ_TYPE_NONE;

Isn't that a change in behaviour if of_node is non-NULL and
param_count==1?


It seems that current code here has bug that if fwspec->param_count==1
and of_node is non-null, fwspec->param[1] will be accessed, which is
introduced from previous patch(irqchip/loongson-pch-pic: Add ACPI init
support). Before the patch, for non-null of_node, translate
callback(use irq_domain_translate_twocell) will return -EINVAL if
fwspec->param_count < 2.

For ACPI path, fwspec->param_count can be 1 or 2.

So in this patch, I'll fix the bug and change the code as following:

if (fwspec->param_count < 1)
return -EINVAL;

if (of_node) {
if (fwspec->param_count < 2)
return -EINVAL;

*hwirq = fwspec->param[0] + priv->ht_vec_base;
*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
} else {
*hwirq = fwspec->param[0] - priv->gsi_base;

if (fwspec->param_count > 1)
*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
else
*type = IRQ_TYPE_NONE;
}


- }
return 0;
}

This irqchip change should probably be a separate patch.


As a separate patch, the input trigger type of pci devices will be low
level because of lacking of workaround to acpi_pci_irq_enable, which
will cause kernel hang, unless the patch of workaround to
acpi_pci_irq_enable is in front of this separated patch.

That seems like a sensible requirement, but I really want to
understand whether PCI Loongarch will *always* generate INTx as
ACTIVE_HIGH or not. Because if that is ever going to change, we will
need a different way to inform the irqchip about the polarity
inversion.


Above same. And in future, in case some generation use ACTIVE_LOW, I think we can use use *Source*(means link) with triggering and polarity property in pci route table of DSDT as following code to override ACTIVE_HIGH, rather than *Source index*(gsi).

if (entry->link)
gsi = acpi_pci_link_allocate_irq(entry->link,
entry->index,
&triggering, &polarity,
&link);


Because of a lot of machines outside have been shipped with firmware using *Source index*, for compatibility with such firmware, the workaround to acpi_pci_irq_enable is required.


M.