Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1473676 > unrolled thread

[Patch v3 03/11] irqchip: axi-intc: Add support for parent intc

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [Patch v3 03/11] irqchip: axi-intc: Add support for parent intc Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com> - 2016-08-31 18:40 +0200
    Re: [Patch v3 03/11] irqchip: axi-intc: Add support for parent intc Marc Zyngier <marc.zyngier@arm.com> - 2016-08-31 19:00 +0200
      Re: [Patch v3 03/11] irqchip: axi-intc: Add support for parent intc Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com> - 2016-09-01 13:10 +0200
        Re: [Patch v3 03/11] irqchip: axi-intc: Add support for parent intc Marc Zyngier <marc.zyngier@arm.com> - 2016-09-01 14:20 +0200
          Re: [Patch v3 03/11] irqchip: axi-intc: Add support for parent intc Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com> - 2016-09-01 16:00 +0200
            Re: [Patch v3 03/11] irqchip: axi-intc: Add support for parent intc Marc Zyngier <marc.zyngier@arm.com> - 2016-09-01 16:50 +0200

#1473676 — [Patch v3 03/11] irqchip: axi-intc: Add support for parent intc

The MIPS based xilfpga platform has the following IRQ structure

Peripherals --> xilinx_intcontroller -> mips_cpu_int controller

Add support for the driver to chain the irq handler

Signed-off-by: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>

---
V2 -> V3
Reused existing parent node instead of finding again.
Cleanup up handler based on review

V1 -> V2

No change
---
 drivers/irqchip/irq-axi-intc.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-axi-intc.c b/drivers/irqchip/irq-axi-intc.c
index cb69241..30bb084 100644
--- a/drivers/irqchip/irq-axi-intc.c
+++ b/drivers/irqchip/irq-axi-intc.c
@@ -15,6 +15,7 @@
 #include <linux/of_address.h>
 #include <linux/io.h>
 #include <linux/bug.h>
+#include <linux/of_irq.h>
 
 /* No one else should require these constants, so define them locally here. */
 #define ISR 0x00			/* Interrupt Status Register */
@@ -154,11 +155,23 @@ static const struct irq_domain_ops xintc_irq_domain_ops = {
 	.map = xintc_map,
 };
 
+static void xil_intc_irq_handler(struct irq_desc *desc)
+{
+	u32 pending;
+
+	do {
+		pending = get_irq();
+		if (pending == -1U)
+			break;
+		generic_handle_irq(pending);
+	} while (true);
+}
+
 static int __init xilinx_intc_of_init(struct device_node *intc,
 					     struct device_node *parent)
 {
 	u32 nr_irq;
-	int ret;
+	int ret, irq;
 	struct xintc_irq_chip *irqc;
 
 	irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
@@ -211,6 +224,15 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
 	root_domain = irq_domain_add_linear(intc, nr_irq, &xintc_irq_domain_ops,
 					    irqc);
 
+	if (parent) {
+		irq = irq_of_parse_and_map(intc, 0);
+		if (irq)
+			irq_set_chained_handler_and_data(irq,
+							 xil_intc_irq_handler,
+							 root_domain);
+
+	}
+
 	irq_set_default_host(root_domain);
 
 	return 0;
-- 
1.9.1

[toc] | [next] | [standalone]

#1473707

On 31/08/16 17:35, Zubair Lutfullah Kakakhel wrote:
> The MIPS based xilfpga platform has the following IRQ structure
> 
> Peripherals --> xilinx_intcontroller -> mips_cpu_int controller
> 
> Add support for the driver to chain the irq handler
> 
> Signed-off-by: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
> 
> ---
> V2 -> V3
> Reused existing parent node instead of finding again.
> Cleanup up handler based on review
> 
> V1 -> V2
> 
> No change
> ---
>  drivers/irqchip/irq-axi-intc.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-axi-intc.c b/drivers/irqchip/irq-axi-intc.c
> index cb69241..30bb084 100644
> --- a/drivers/irqchip/irq-axi-intc.c
> +++ b/drivers/irqchip/irq-axi-intc.c
> @@ -15,6 +15,7 @@
>  #include <linux/of_address.h>
>  #include <linux/io.h>
>  #include <linux/bug.h>
> +#include <linux/of_irq.h>
>  
>  /* No one else should require these constants, so define them locally here. */
>  #define ISR 0x00			/* Interrupt Status Register */
> @@ -154,11 +155,23 @@ static const struct irq_domain_ops xintc_irq_domain_ops = {
>  	.map = xintc_map,
>  };
>  
> +static void xil_intc_irq_handler(struct irq_desc *desc)
> +{
> +	u32 pending;
> +
> +	do {
> +		pending = get_irq();
> +		if (pending == -1U)
> +			break;
> +		generic_handle_irq(pending);
> +	} while (true);
> +}
> +
>  static int __init xilinx_intc_of_init(struct device_node *intc,
>  					     struct device_node *parent)
>  {
>  	u32 nr_irq;
> -	int ret;
> +	int ret, irq;
>  	struct xintc_irq_chip *irqc;
>  
>  	irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
> @@ -211,6 +224,15 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
>  	root_domain = irq_domain_add_linear(intc, nr_irq, &xintc_irq_domain_ops,
>  					    irqc);
>  
> +	if (parent) {
> +		irq = irq_of_parse_and_map(intc, 0);
> +		if (irq)
> +			irq_set_chained_handler_and_data(irq,
> +							 xil_intc_irq_handler,
> +							 root_domain);
> +
> +	}
> +
>  	irq_set_default_host(root_domain);
>  
>  	return 0;
> 

This doesn't seem right. You've now overridden the xintc_irqc pointer,
so I don't know how you can still process interrupts once you've
discovered a secondary interrupt controller. You've also allocated a
second root_domain, changed the default domain to point to the secondary
controller...

Have you tested this code? Or am I missing something obvious?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1474285

Hi,

Thanks for the review
Comments inline.

On 08/31/2016 05:57 PM, Marc Zyngier wrote:
> On 31/08/16 17:35, Zubair Lutfullah Kakakhel wrote:
>> The MIPS based xilfpga platform has the following IRQ structure
>>
>> Peripherals --> xilinx_intcontroller -> mips_cpu_int controller
>>
>> Add support for the driver to chain the irq handler
>>
>> Signed-off-by: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
>>
>> ---
>> V2 -> V3
>> Reused existing parent node instead of finding again.
>> Cleanup up handler based on review
>>
>> V1 -> V2
>>
>> No change
>> ---
>>  drivers/irqchip/irq-axi-intc.c | 24 +++++++++++++++++++++++-
>>  1 file changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-axi-intc.c b/drivers/irqchip/irq-axi-intc.c
>> index cb69241..30bb084 100644
>> --- a/drivers/irqchip/irq-axi-intc.c
>> +++ b/drivers/irqchip/irq-axi-intc.c
>> @@ -15,6 +15,7 @@
>>  #include <linux/of_address.h>
>>  #include <linux/io.h>
>>  #include <linux/bug.h>
>> +#include <linux/of_irq.h>
>>
>>  /* No one else should require these constants, so define them locally here. */
>>  #define ISR 0x00			/* Interrupt Status Register */
>> @@ -154,11 +155,23 @@ static const struct irq_domain_ops xintc_irq_domain_ops = {
>>  	.map = xintc_map,
>>  };
>>
>> +static void xil_intc_irq_handler(struct irq_desc *desc)
>> +{
>> +	u32 pending;
>> +
>> +	do {
>> +		pending = get_irq();
>> +		if (pending == -1U)
>> +			break;
>> +		generic_handle_irq(pending);
>> +	} while (true);
>> +}
>> +
>>  static int __init xilinx_intc_of_init(struct device_node *intc,
>>  					     struct device_node *parent)
>>  {
>>  	u32 nr_irq;
>> -	int ret;
>> +	int ret, irq;
>>  	struct xintc_irq_chip *irqc;
>>
>>  	irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
>> @@ -211,6 +224,15 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
>>  	root_domain = irq_domain_add_linear(intc, nr_irq, &xintc_irq_domain_ops,
>>  					    irqc);
>>
>> +	if (parent) {
>> +		irq = irq_of_parse_and_map(intc, 0);
>> +		if (irq)
>> +			irq_set_chained_handler_and_data(irq,
>> +							 xil_intc_irq_handler,
>> +							 root_domain);
>> +
>> +	}
>> +
>>  	irq_set_default_host(root_domain);
>>
>>  	return 0;
>>
>
> This doesn't seem right. You've now overridden the xintc_irqc pointer,
> so I don't know how you can still process interrupts once you've
> discovered a secondary interrupt controller. You've also allocated a
> second root_domain, changed the default domain to point to the secondary
> controller...
>
> Have you tested this code? Or am I missing something obvious?

Yes it works. I'll try to explain the platform setup a bit.
Perhaps that will make it clear about what I'm trying to do.

UART IRQ -> AXI INTC -> MIPS internal INTC -> CPU

MIPS Internal Interrupt controller in drivers/irqchip/irq-mips-cpu.c
uses irq_domain_add_legacy while AXI Intc uses irq_domain_add_linear

My aim was to set up a chained irq handler with least disturbance.

Hence the above code.

Your concerns are valid. The code is working because read/writes rely
on the static xintc_irqc in the file.
And the second root domain is also not breaking the platform because
the irq-mips-cpu.c uses irq_domain_add_legacy and doesn't use
irq_set_default_host.

# cat /proc/interrupts
            CPU0
   7:      43493      MIPS   7  timer
   8:         83  Xilinx INTC   1-level     eth0
   9:        417  Xilinx INTC   0-level     serial
  10:         15  Xilinx INTC   4-level     10a00000.i2c
ERR:          0
#

Given the above concerns. How about doing things this way?

	if (parent) {
		irq = irq_of_parse_and_map(intc, 0);
		if (irq)
			irq_set_chained_handler_and_data(irq,
							 xil_intc_irq_handler,
							 irqc);

	} else
		irq_set_default_host(root_domain);

default host is only set if no parent exists.
And the irqc pointer is passed as the data.

Thanks
ZubairLK

>
> Thanks,
>
> 	M.
>

[toc] | [prev] | [next] | [standalone]

#1474367

On 01/09/16 12:01, Zubair Lutfullah Kakakhel wrote:
> Hi,
> 
> Thanks for the review
> Comments inline.
> 
> On 08/31/2016 05:57 PM, Marc Zyngier wrote:
>> On 31/08/16 17:35, Zubair Lutfullah Kakakhel wrote:
>>> The MIPS based xilfpga platform has the following IRQ structure
>>>
>>> Peripherals --> xilinx_intcontroller -> mips_cpu_int controller
>>>
>>> Add support for the driver to chain the irq handler
>>>
>>> Signed-off-by: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
>>>
>>> ---
>>> V2 -> V3
>>> Reused existing parent node instead of finding again.
>>> Cleanup up handler based on review
>>>
>>> V1 -> V2
>>>
>>> No change
>>> ---
>>>  drivers/irqchip/irq-axi-intc.c | 24 +++++++++++++++++++++++-
>>>  1 file changed, 23 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/irqchip/irq-axi-intc.c b/drivers/irqchip/irq-axi-intc.c
>>> index cb69241..30bb084 100644
>>> --- a/drivers/irqchip/irq-axi-intc.c
>>> +++ b/drivers/irqchip/irq-axi-intc.c
>>> @@ -15,6 +15,7 @@
>>>  #include <linux/of_address.h>
>>>  #include <linux/io.h>
>>>  #include <linux/bug.h>
>>> +#include <linux/of_irq.h>
>>>
>>>  /* No one else should require these constants, so define them locally here. */
>>>  #define ISR 0x00			/* Interrupt Status Register */
>>> @@ -154,11 +155,23 @@ static const struct irq_domain_ops xintc_irq_domain_ops = {
>>>  	.map = xintc_map,
>>>  };
>>>
>>> +static void xil_intc_irq_handler(struct irq_desc *desc)
>>> +{
>>> +	u32 pending;
>>> +
>>> +	do {
>>> +		pending = get_irq();
>>> +		if (pending == -1U)
>>> +			break;
>>> +		generic_handle_irq(pending);
>>> +	} while (true);
>>> +}
>>> +
>>>  static int __init xilinx_intc_of_init(struct device_node *intc,
>>>  					     struct device_node *parent)
>>>  {
>>>  	u32 nr_irq;
>>> -	int ret;
>>> +	int ret, irq;
>>>  	struct xintc_irq_chip *irqc;
>>>
>>>  	irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
>>> @@ -211,6 +224,15 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
>>>  	root_domain = irq_domain_add_linear(intc, nr_irq, &xintc_irq_domain_ops,
>>>  					    irqc);
>>>
>>> +	if (parent) {
>>> +		irq = irq_of_parse_and_map(intc, 0);
>>> +		if (irq)
>>> +			irq_set_chained_handler_and_data(irq,
>>> +							 xil_intc_irq_handler,
>>> +							 root_domain);
>>> +
>>> +	}
>>> +
>>>  	irq_set_default_host(root_domain);
>>>
>>>  	return 0;
>>>
>>
>> This doesn't seem right. You've now overridden the xintc_irqc pointer,
>> so I don't know how you can still process interrupts once you've
>> discovered a secondary interrupt controller. You've also allocated a
>> second root_domain, changed the default domain to point to the secondary
>> controller...
>>
>> Have you tested this code? Or am I missing something obvious?
> 
> Yes it works. I'll try to explain the platform setup a bit.
> Perhaps that will make it clear about what I'm trying to do.
> 
> UART IRQ -> AXI INTC -> MIPS internal INTC -> CPU
> 
> MIPS Internal Interrupt controller in drivers/irqchip/irq-mips-cpu.c
> uses irq_domain_add_legacy while AXI Intc uses irq_domain_add_linear
> 
> My aim was to set up a chained irq handler with least disturbance.
> 
> Hence the above code.
> 
> Your concerns are valid. The code is working because read/writes rely
> on the static xintc_irqc in the file.
> And the second root domain is also not breaking the platform because
> the irq-mips-cpu.c uses irq_domain_add_legacy and doesn't use
> irq_set_default_host.
> 
> # cat /proc/interrupts
>             CPU0
>    7:      43493      MIPS   7  timer
>    8:         83  Xilinx INTC   1-level     eth0
>    9:        417  Xilinx INTC   0-level     serial
>   10:         15  Xilinx INTC   4-level     10a00000.i2c
> ERR:          0
> #
> 
> Given the above concerns. How about doing things this way?
> 
> 	if (parent) {
> 		irq = irq_of_parse_and_map(intc, 0);
> 		if (irq)
> 			irq_set_chained_handler_and_data(irq,
> 							 xil_intc_irq_handler,
> 							 irqc);
> 
> 	} else
> 		irq_set_default_host(root_domain);
> 
> default host is only set if no parent exists.
> And the irqc pointer is passed as the data.

But that still doesn't address the case I had in mind, which is when you
have *two* AXI-intc, one cascaded into the other. Is that something that
could be built? You should at least make sure that there is a big fat
warning if you don't want to support that case, because that will be
hell to debug.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1474409

Hi,

On 09/01/2016 01:14 PM, Marc Zyngier wrote:
> On 01/09/16 12:01, Zubair Lutfullah Kakakhel wrote:
>> Hi,
>>
>> Thanks for the review
>> Comments inline.
>>
>> On 08/31/2016 05:57 PM, Marc Zyngier wrote:
>>> On 31/08/16 17:35, Zubair Lutfullah Kakakhel wrote:
>>>> The MIPS based xilfpga platform has the following IRQ structure
>>>>
>>>> Peripherals --> xilinx_intcontroller -> mips_cpu_int controller
>>>>
>>>> Add support for the driver to chain the irq handler
>>>>
>>>> Signed-off-by: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
>>>>
>>>> ---
>>>> V2 -> V3
>>>> Reused existing parent node instead of finding again.
>>>> Cleanup up handler based on review
>>>>
>>>> V1 -> V2
>>>>
>>>> No change
>>>> ---
>>>>  drivers/irqchip/irq-axi-intc.c | 24 +++++++++++++++++++++++-
>>>>  1 file changed, 23 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/irqchip/irq-axi-intc.c b/drivers/irqchip/irq-axi-intc.c
>>>> index cb69241..30bb084 100644
>>>> --- a/drivers/irqchip/irq-axi-intc.c
>>>> +++ b/drivers/irqchip/irq-axi-intc.c
>>>> @@ -15,6 +15,7 @@
>>>>  #include <linux/of_address.h>
>>>>  #include <linux/io.h>
>>>>  #include <linux/bug.h>
>>>> +#include <linux/of_irq.h>
>>>>
>>>>  /* No one else should require these constants, so define them locally here. */
>>>>  #define ISR 0x00			/* Interrupt Status Register */
>>>> @@ -154,11 +155,23 @@ static const struct irq_domain_ops xintc_irq_domain_ops = {
>>>>  	.map = xintc_map,
>>>>  };
>>>>
>>>> +static void xil_intc_irq_handler(struct irq_desc *desc)
>>>> +{
>>>> +	u32 pending;
>>>> +
>>>> +	do {
>>>> +		pending = get_irq();
>>>> +		if (pending == -1U)
>>>> +			break;
>>>> +		generic_handle_irq(pending);
>>>> +	} while (true);
>>>> +}
>>>> +
>>>>  static int __init xilinx_intc_of_init(struct device_node *intc,
>>>>  					     struct device_node *parent)
>>>>  {
>>>>  	u32 nr_irq;
>>>> -	int ret;
>>>> +	int ret, irq;
>>>>  	struct xintc_irq_chip *irqc;
>>>>
>>>>  	irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
>>>> @@ -211,6 +224,15 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
>>>>  	root_domain = irq_domain_add_linear(intc, nr_irq, &xintc_irq_domain_ops,
>>>>  					    irqc);
>>>>
>>>> +	if (parent) {
>>>> +		irq = irq_of_parse_and_map(intc, 0);
>>>> +		if (irq)
>>>> +			irq_set_chained_handler_and_data(irq,
>>>> +							 xil_intc_irq_handler,
>>>> +							 root_domain);
>>>> +
>>>> +	}
>>>> +
>>>>  	irq_set_default_host(root_domain);
>>>>
>>>>  	return 0;
>>>>
>>>
>>> This doesn't seem right. You've now overridden the xintc_irqc pointer,
>>> so I don't know how you can still process interrupts once you've
>>> discovered a secondary interrupt controller. You've also allocated a
>>> second root_domain, changed the default domain to point to the secondary
>>> controller...
>>>
>>> Have you tested this code? Or am I missing something obvious?
>>
>> Yes it works. I'll try to explain the platform setup a bit.
>> Perhaps that will make it clear about what I'm trying to do.
>>
>> UART IRQ -> AXI INTC -> MIPS internal INTC -> CPU
>>
>> MIPS Internal Interrupt controller in drivers/irqchip/irq-mips-cpu.c
>> uses irq_domain_add_legacy while AXI Intc uses irq_domain_add_linear
>>
>> My aim was to set up a chained irq handler with least disturbance.
>>
>> Hence the above code.
>>
>> Your concerns are valid. The code is working because read/writes rely
>> on the static xintc_irqc in the file.
>> And the second root domain is also not breaking the platform because
>> the irq-mips-cpu.c uses irq_domain_add_legacy and doesn't use
>> irq_set_default_host.
>>
>> # cat /proc/interrupts
>>             CPU0
>>    7:      43493      MIPS   7  timer
>>    8:         83  Xilinx INTC   1-level     eth0
>>    9:        417  Xilinx INTC   0-level     serial
>>   10:         15  Xilinx INTC   4-level     10a00000.i2c
>> ERR:          0
>> #
>>
>> Given the above concerns. How about doing things this way?
>>
>> 	if (parent) {
>> 		irq = irq_of_parse_and_map(intc, 0);
>> 		if (irq)
>> 			irq_set_chained_handler_and_data(irq,
>> 							 xil_intc_irq_handler,
>> 							 irqc);
>>
>> 	} else
>> 		irq_set_default_host(root_domain);
>>
>> default host is only set if no parent exists.
>> And the irqc pointer is passed as the data.
>
> But that still doesn't address the case I had in mind, which is when you
> have *two* AXI-intc, one cascaded into the other. Is that something that
> could be built? You should at least make sure that there is a big fat
> warning if you don't want to support that case, because that will be
> hell to debug.

Oo. I didn't think of that one. tbh, I'm not sure if that is currently supported
because this driver came out of arch/microblaze. And it didn't have any code that
looked supportive of chained interrupt handling.

'Technically', it should be possible to synthesize two daisy chained axi interrupt
controllers on an FPGA. But I don't see it being supported before.

A warning would be nice. Any suggestions on the most suitable way?

Thanks,
ZubairLK

>
> Thanks,
>
> 	M.
>

[toc] | [prev] | [next] | [standalone]

#1474440

On 01/09/16 14:52, Zubair Lutfullah Kakakhel wrote:
> Hi,

[...]

>> But that still doesn't address the case I had in mind, which is when you
>> have *two* AXI-intc, one cascaded into the other. Is that something that
>> could be built? You should at least make sure that there is a big fat
>> warning if you don't want to support that case, because that will be
>> hell to debug.
> 
> Oo. I didn't think of that one. tbh, I'm not sure if that is currently supported
> because this driver came out of arch/microblaze. And it didn't have any code that
> looked supportive of chained interrupt handling.
> 
> 'Technically', it should be possible to synthesize two daisy chained axi interrupt
> controllers on an FPGA. But I don't see it being supported before.
> 
> A warning would be nice. Any suggestions on the most suitable way?

Check if you've already allocated a xintc_irqc. If so, abort the probing
early...

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web