[PATCH V4 0/4] irqchip: Support to set irq type for ACPI path

[PATCH V4 0/4] irqchip: Support to set irq type for ACPI path

[Jianmin Lv]

For ACPI path of pch-pic and liointc driver, setting irq
type is not supported yet, so the patch series add code
to implement it.

And a bug in translate callback of irqchip/loongson-pch-pic, which
is introduced by previous patch, is fixed.

GSI for legacy irqs of PCI devices are mapped in pch-pic domain, after
supporting setting_irq_type for pch-pic domain, we add the workaround
for LoongArch based PCI controller with high-level trigger intterrupt
so that the high-level trigger type is passed into acpi_register_gsi().
  
V1 -> V2
- Change comment information and fix a bug for DT path in patch[1].

V2 -> V3
- Separate original patch[1] to three patches[1][2][3].

V3 -> V4
- Improve commit log

Jianmin Lv (4):
  ACPI / PCI: fix LPIC IRQ model default PCI IRQ polarity
  irqchip/loongson-pch-pic: fix translate callback for DT path
  irqchip/loongson-pch-pic: Support to set IRQ type for ACPI path
  irqchip/loongson-liointc: Support to set IRQ type for ACPI path

 drivers/acpi/pci_irq.c                 |  6 ++++--
 drivers/irqchip/irq-loongson-liointc.c |  7 ++++++-
 drivers/irqchip/irq-loongson-pch-pic.c | 14 ++++++++++----
 3 files changed, 20 insertions(+), 7 deletions(-)

-- 
2.31.1

[PATCH V4 1/4] ACPI / PCI: fix LPIC IRQ model default PCI IRQ polarity

[Jianmin Lv]

On LoongArch ACPI based systems, the PCI devices (e.g. sata
controlers and PCI-to-to PCI bridge controlers) existed in
Loongson chipsets output high-level interrupt signal to the
interrupt controller they connected to, while the IRQs are
active low from the perspective of PCI(in 2.2.6. Interrupt
Pins, "Interrupts on PCI are optional and defined as level
sensitive, asserted low), which means that the interrupt
output of PCI devices plugged into PCI-to-to PCI bridges of
Loongson chipset will be also converted to high-level. So
high level triggered type is required to be passed to
acpi_register_gsi() when creating mappings for PCI devices.

Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
---
 drivers/acpi/pci_irq.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
index 08e15774fb9f..ff30ceca2203 100644
--- a/drivers/acpi/pci_irq.c
+++ b/drivers/acpi/pci_irq.c
@@ -387,13 +387,15 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
 	u8 pin;
 	int triggering = ACPI_LEVEL_SENSITIVE;
 	/*
-	 * On ARM systems with the GIC interrupt model, level interrupts
+	 * On ARM systems with the GIC interrupt model, or LoongArch
+	 * systems with the LPIC interrupt model, level interrupts
 	 * are always polarity high by specification; PCI legacy
 	 * IRQs lines are inverted before reaching the interrupt
 	 * 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;
 	char *link = NULL;
 	char link_desc[16];
-- 
2.31.1

Re: [PATCH V4 1/4] ACPI / PCI: fix LPIC IRQ model default PCI IRQ polarity

[Bjorn Helgaas]

On Thu, Oct 20, 2022 at 04:22:02PM +0800, Jianmin Lv wrote:
> On LoongArch ACPI based systems, the PCI devices (e.g. sata
> controlers and PCI-to-to PCI bridge controlers) existed in
> Loongson chipsets output high-level interrupt signal to the
> interrupt controller they connected to,

I assume the active high behavior is hardware behavior that is
independent of the fact that you're using ACPI firmware on the
hardware.  If so, I would omit "ACPI based".

s/sata/SATA/
s/controlers/controllers/ (twice)
s/PCI-to-to PCI/PCI-to-PCI/
s/existed in/in/
s/they connected/they are connected/

> while the IRQs are
> active low from the perspective of PCI(in 2.2.6. Interrupt
> Pins, "Interrupts on PCI are optional and defined as level
> sensitive, asserted low),

I don't think you need this spec reference, since "asserted low" is
the standard thing that happens everywhere.  But if you do want it, it
needs to specify which spec it refers to, e.g., "Conventional PCI
r3.0, sec 2.2.6" so it's not confused with the PCIe spec.

The quote from the spec itself should be terminated with a close quote
("), i.e., 

  "Interrupts on PCI ... asserted low"

> which means that the interrupt
> output of PCI devices plugged into PCI-to-to PCI bridges of
> Loongson chipset will be also converted to high-level. So
> high level triggered type is required to be passed to
> acpi_register_gsi() when creating mappings for PCI devices.

This is the part where I was hoping for a reference to a spec that
talks about how PCI interrupts are inverted.  The inverter is the part
that's special here.

I see that ACPI r6.5, sec 5.2.12, mentions LPIC, but it doesn't
mention the inverter.  It has a lot more mentions of GIC, but also no
details about an inverter.  I suppose that would be in the GIC spec,
which I'm not familiar with.

The point is that one should be able to write this code from a spec,
without having to empirically discover the interrupt polarity.  What
spec tells you about using ACTIVE_HIGH here?

s/PCI-to-to PCI/PCI-to-PCI/ again

Rewrap the log to fill 75 columns like the rest of the history.

> Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
> ---
>  drivers/acpi/pci_irq.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
> index 08e15774fb9f..ff30ceca2203 100644
> --- a/drivers/acpi/pci_irq.c
> +++ b/drivers/acpi/pci_irq.c
> @@ -387,13 +387,15 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
>  	u8 pin;
>  	int triggering = ACPI_LEVEL_SENSITIVE;
>  	/*
> -	 * On ARM systems with the GIC interrupt model, level interrupts
> +	 * On ARM systems with the GIC interrupt model, or LoongArch
> +	 * systems with the LPIC interrupt model, level interrupts

Is "LoongArch" required in this comment?  Might the LPIC model be used
on non-LoongArch systems?

I see it follows the example of "ARM systems".  In my opinion, "ARM"
probably should be removed, too, because the code checks only for GIC
or LPIC; it doesn't check for ARM or LoongArch.

If GIC is restricted to ARM and LPIC is restricted to LoongArch,
that's fine, but that constraint should be expressed somewhere else
and doesn't need to be repeated here.

>  	 * are always polarity high by specification; PCI legacy
>  	 * IRQs lines are inverted before reaching the interrupt
>  	 * 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;
>  	char *link = NULL;
>  	char link_desc[16];
> -- 
> 2.31.1
> 

Re: [PATCH V4 1/4] ACPI / PCI: fix LPIC IRQ model default PCI IRQ polarity

[Jianmin Lv]

On 2022/10/21 上午12:47, Bjorn Helgaas wrote:
> On Thu, Oct 20, 2022 at 04:22:02PM +0800, Jianmin Lv wrote:
>> On LoongArch ACPI based systems, the PCI devices (e.g. sata
>> controlers and PCI-to-to PCI bridge controlers) existed in
>> Loongson chipsets output high-level interrupt signal to the
>> interrupt controller they connected to,
> 
> I assume the active high behavior is hardware behavior that is
> independent of the fact that you're using ACPI firmware on the
> hardware.  If so, I would omit "ACPI based".
> 
> s/sata/SATA/
> s/controlers/controllers/ (twice)
> s/PCI-to-to PCI/PCI-to-PCI/
> s/existed in/in/
> s/they connected/they are connected/
> 

Ok, thanks, I'll improve them again.

>> while the IRQs are
>> active low from the perspective of PCI(in 2.2.6. Interrupt
>> Pins, "Interrupts on PCI are optional and defined as level
>> sensitive, asserted low),
> 
> I don't think you need this spec reference, since "asserted low" is
> the standard thing that happens everywhere.  But if you do want it, it
> needs to specify which spec it refers to, e.g., "Conventional PCI
> r3.0, sec 2.2.6" so it's not confused with the PCIe spec.
> 
> The quote from the spec itself should be terminated with a close quote
> ("), i.e.,
> 
>    "Interrupts on PCI ... asserted low"
> 

Ok, thanks, I'll specify spec version with correct pattern.

>> which means that the interrupt
>> output of PCI devices plugged into PCI-to-to PCI bridges of
>> Loongson chipset will be also converted to high-level. So
>> high level triggered type is required to be passed to
>> acpi_register_gsi() when creating mappings for PCI devices.
> 
> This is the part where I was hoping for a reference to a spec that
> talks about how PCI interrupts are inverted.  The inverter is the part
> that's special here.
> 
> I see that ACPI r6.5, sec 5.2.12, mentions LPIC, but it doesn't
> mention the inverter.  It has a lot more mentions of GIC, but also no
> details about an inverter.  I suppose that would be in the GIC spec,
> which I'm not familiar with.
> 
> The point is that one should be able to write this code from a spec,
> without having to empirically discover the interrupt polarity.  What
> spec tells you about using ACTIVE_HIGH here?
> 
Yes, no mentions for the inverter in ACPI spec, the description about
device interrupt type can be found in Loongson chipset manual:

https://github.com/loongson/LoongArch-Documentation/blob/main/docs/Loongson-7A1000-usermanual-EN/interrupt-controller/device-interrupt-types.adoc

where the interrupts coming from interrupt source are level triggered 
and active high except some specific device such as AC97 DMA and GPIO.

> s/PCI-to-to PCI/PCI-to-PCI/ again
>
> Rewrap the log to fill 75 columns like the rest of the history.
> 
Ok, thanks.

>> Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
>> ---
>>   drivers/acpi/pci_irq.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
>> index 08e15774fb9f..ff30ceca2203 100644
>> --- a/drivers/acpi/pci_irq.c
>> +++ b/drivers/acpi/pci_irq.c
>> @@ -387,13 +387,15 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
>>   	u8 pin;
>>   	int triggering = ACPI_LEVEL_SENSITIVE;
>>   	/*
>> -	 * On ARM systems with the GIC interrupt model, level interrupts
>> +	 * On ARM systems with the GIC interrupt model, or LoongArch
>> +	 * systems with the LPIC interrupt model, level interrupts
> 
> Is "LoongArch" required in this comment?  Might the LPIC model be used
> on non-LoongArch systems?
> 
Just like GIC is restricted to ARM, and LPIC is restricted to LoongArch, 
as you mentioned below. So LPIC model will be not used on non-LoongArch 
systems.

> I see it follows the example of "ARM systems".  In my opinion, "ARM"
> probably should be removed, too, because the code checks only for GIC
> or LPIC; it doesn't check for ARM or LoongArch.
> 
> If GIC is restricted to ARM and LPIC is restricted to LoongArch,
> that's fine, but that constraint should be expressed somewhere else
> and doesn't need to be repeated here.
> 
Though the definition and constraints for GIC and LPIC are explicitly 
expressed in ACPI spec, to be clear, repeating the relation here only 
with short words maybe worthy so that people understand the workaround 
conveniently without having to referencing ACPI spec, right?

>>   	 * are always polarity high by specification; PCI legacy
>>   	 * IRQs lines are inverted before reaching the interrupt
>>   	 * 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;
>>   	char *link = NULL;
>>   	char link_desc[16];
>> -- 
>> 2.31.1
>>

Re: [PATCH V4 1/4] ACPI / PCI: fix LPIC IRQ model default PCI IRQ polarity

[Bjorn Helgaas]

On Fri, Oct 21, 2022 at 09:58:57AM +0800, Jianmin Lv wrote:
> On 2022/10/21 上午12:47, Bjorn Helgaas wrote:
> > On Thu, Oct 20, 2022 at 04:22:02PM +0800, Jianmin Lv wrote:
> > > On LoongArch ACPI based systems, the PCI devices (e.g. sata
> > > controlers and PCI-to-to PCI bridge controlers) existed in
> > > Loongson chipsets output high-level interrupt signal to the
> > > interrupt controller they connected to,

> > The point is that one should be able to write this code from a spec,
> > without having to empirically discover the interrupt polarity.  What
> > spec tells you about using ACTIVE_HIGH here?
> > 
> Yes, no mentions for the inverter in ACPI spec, the description about
> device interrupt type can be found in Loongson chipset manual:
> 
> https://github.com/loongson/LoongArch-Documentation/blob/main/docs/Loongson-7A1000-usermanual-EN/interrupt-controller/device-interrupt-types.adoc

That's the kind of reference I was looking for.  The link to HTML is
convenient in some ways, but since specs evolve over time and URLs are
ephemeral, I think a citation like "Loongson 7A1000 Bridge User Manual
v2.00, sec 5.3" is more likely to be useful far in the future.

Bjorn

Re: [PATCH V4 1/4] ACPI / PCI: fix LPIC IRQ model default PCI IRQ polarity

[Jianmin Lv]

On 2022/10/21 下午8:01, Bjorn Helgaas wrote:
> On Fri, Oct 21, 2022 at 09:58:57AM +0800, Jianmin Lv wrote:
>> On 2022/10/21 上午12:47, Bjorn Helgaas wrote:
>>> On Thu, Oct 20, 2022 at 04:22:02PM +0800, Jianmin Lv wrote:
>>>> On LoongArch ACPI based systems, the PCI devices (e.g. sata
>>>> controlers and PCI-to-to PCI bridge controlers) existed in
>>>> Loongson chipsets output high-level interrupt signal to the
>>>> interrupt controller they connected to,
> 
>>> The point is that one should be able to write this code from a spec,
>>> without having to empirically discover the interrupt polarity.  What
>>> spec tells you about using ACTIVE_HIGH here?
>>>
>> Yes, no mentions for the inverter in ACPI spec, the description about
>> device interrupt type can be found in Loongson chipset manual:
>>
>> https://github.com/loongson/LoongArch-Documentation/blob/main/docs/Loongson-7A1000-usermanual-EN/interrupt-controller/device-interrupt-types.adoc
> 
> That's the kind of reference I was looking for.  The link to HTML is
> convenient in some ways, but since specs evolve over time and URLs are
> ephemeral, I think a citation like "Loongson 7A1000 Bridge User Manual
> v2.00, sec 5.3" is more likely to be useful far in the future.
> 
Ok, good suggestion, thanks.

> Bjorn
> 

[PATCH V4 2/4] irqchip/loongson-pch-pic: fix translate callback for DT path

[Jianmin Lv]

In DT path of translate callback, if fwspec->param_count==1
and of_node is non-null, fwspec->param[1] will be accessed,
which is introduced from previous commit bcdd75c596c8
(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, so the check in the patch is added.

Fixes: bcdd75c596c8 ("irqchip/loongson-pch-pic: Add ACPI init support")
Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
---
 drivers/irqchip/irq-loongson-pch-pic.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/irqchip/irq-loongson-pch-pic.c b/drivers/irqchip/irq-loongson-pch-pic.c
index c01b9c257005..03493cda65a3 100644
--- a/drivers/irqchip/irq-loongson-pch-pic.c
+++ b/drivers/irqchip/irq-loongson-pch-pic.c
@@ -159,6 +159,9 @@ static int pch_pic_domain_translate(struct irq_domain *d,
 		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 {
-- 
2.31.1

[PATCH V4 3/4] irqchip/loongson-pch-pic: Support to set IRQ type for ACPI path

[Jianmin Lv]

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.

Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
---
 drivers/irqchip/irq-loongson-pch-pic.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/irq-loongson-pch-pic.c b/drivers/irqchip/irq-loongson-pch-pic.c
index 03493cda65a3..a26a3f59d4a5 100644
--- a/drivers/irqchip/irq-loongson-pch-pic.c
+++ b/drivers/irqchip/irq-loongson-pch-pic.c
@@ -155,9 +155,6 @@ static int pch_pic_domain_translate(struct irq_domain *d,
 	struct pch_pic *priv = d->host_data;
 	struct device_node *of_node = to_of_node(fwspec->fwnode);
 
-	if (fwspec->param_count < 1)
-		return -EINVAL;
-
 	if (of_node) {
 		if (fwspec->param_count < 2)
 			return -EINVAL;
@@ -165,8 +162,14 @@ static int pch_pic_domain_translate(struct irq_domain *d,
 		*hwirq = fwspec->param[0] + priv->ht_vec_base;
 		*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
 	} else {
+		if (fwspec->param_count < 1)
+			return -EINVAL;
+
 		*hwirq = fwspec->param[0] - priv->gsi_base;
-		*type = IRQ_TYPE_NONE;
+		if (fwspec->param_count > 1)
+			*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
+		else
+			*type = IRQ_TYPE_NONE;
 	}
 
 	return 0;
-- 
2.31.1

[PATCH V4 4/4] irqchip/loongson-liointc: Support to set IRQ type for ACPI path

[Jianmin Lv]

For ACPI path, the xlate callback used IRQ_TYPE_NONE and ignored
the IRQ type in intspec[1]. For supporting to set type for
IRQs of the irqdomain, intspec[1] should be used to get IRQ
type.

Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
---
 drivers/irqchip/irq-loongson-liointc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-loongson-liointc.c b/drivers/irqchip/irq-loongson-liointc.c
index 0da8716f8f24..838c8fa2d868 100644
--- a/drivers/irqchip/irq-loongson-liointc.c
+++ b/drivers/irqchip/irq-loongson-liointc.c
@@ -167,7 +167,12 @@ static int liointc_domain_xlate(struct irq_domain *d, struct device_node *ctrlr,
 	if (WARN_ON(intsize < 1))
 		return -EINVAL;
 	*out_hwirq = intspec[0] - GSI_MIN_CPU_IRQ;
-	*out_type = IRQ_TYPE_NONE;
+
+	if (intsize > 1)
+		*out_type = intspec[1] & IRQ_TYPE_SENSE_MASK;
+	else
+		*out_type = IRQ_TYPE_NONE;
+
 	return 0;
 }
 
-- 
2.31.1