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

Groups > linux.kernel > #1502226 > unrolled thread

[Patch v5 04/12] irqchip: xilinx: 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 v5 04/12] irqchip: xilinx: Add support for parent intc Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com> - 2016-10-17 19:00 +0200
    Re: [Patch v5 04/12] irqchip: xilinx: Add support for parent intc Marc Zyngier <marc.zyngier@arm.com> - 2016-10-21 11:50 +0200
      Re: [Patch v5 04/12] irqchip: xilinx: Add support for parent intc Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com> - 2016-10-25 11:50 +0200
        Re: [Patch v5 04/12] irqchip: xilinx: Add support for parent intc Thomas Gleixner <tglx@linutronix.de> - 2016-10-25 13:00 +0200
          Re: [Patch v5 04/12] irqchip: xilinx: Add support for parent intc Marc Zyngier <marc.zyngier@arm.com> - 2016-10-25 17:00 +0200

#1502226 — [Patch v5 04/12] irqchip: xilinx: 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>

---
V4 -> V5
Rebased to v4.9-rc1
Missing curly braces

V3 -> V4
Clean up if/else when a parent is found
Pass irqchip structure to handler as data

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

V1 -> V2

No change
---
 drivers/irqchip/irq-xilinx-intc.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-xilinx-intc.c b/drivers/irqchip/irq-xilinx-intc.c
index 45e5154..dbf8b0c 100644
--- a/drivers/irqchip/irq-xilinx-intc.c
+++ b/drivers/irqchip/irq-xilinx-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 int xintc_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
 	.map = xintc_map,
 };
 
+static void xil_intc_irq_handler(struct irq_desc *desc)
+{
+	u32 pending;
+
+	do {
+		pending = xintc_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;
 
 	if (xintc_irqc) {
@@ -221,7 +234,16 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
 		goto err_alloc;
 	}
 
-	irq_set_default_host(root_domain);
+	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);
+	}
 
 	return 0;
 
-- 
1.9.1

[toc] | [next] | [standalone]

#1505729

On 17/10/16 17:52, 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>
> 
> ---
> V4 -> V5
> Rebased to v4.9-rc1
> Missing curly braces
> 
> V3 -> V4
> Clean up if/else when a parent is found
> Pass irqchip structure to handler as data
> 
> V2 -> V3
> Reused existing parent node instead of finding again.
> Cleanup up handler based on review
> 
> V1 -> V2
> 
> No change
> ---
>  drivers/irqchip/irq-xilinx-intc.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-xilinx-intc.c b/drivers/irqchip/irq-xilinx-intc.c
> index 45e5154..dbf8b0c 100644
> --- a/drivers/irqchip/irq-xilinx-intc.c
> +++ b/drivers/irqchip/irq-xilinx-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 int xintc_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
>  	.map = xintc_map,
>  };
>  
> +static void xil_intc_irq_handler(struct irq_desc *desc)
> +{
> +	u32 pending;
> +
> +	do {
> +		pending = xintc_get_irq();
> +		if (pending == -1U)
> +			break;
> +		generic_handle_irq(pending);
> +	} while (true);

This is missing the chained_irq_enter()/exit() calls, which will lead to
races or lockups on the root irqchip.

> +}
> +
>  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;
>  
>  	if (xintc_irqc) {
> @@ -221,7 +234,16 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
>  		goto err_alloc;
>  	}
>  
> -	irq_set_default_host(root_domain);
> +	if (parent) {
> +		irq = irq_of_parse_and_map(intc, 0);
> +		if (irq)
> +			irq_set_chained_handler_and_data(irq,
> +							 xil_intc_irq_handler,
> +							 irqc);
> +

Shouldn't you return an error if irq is zero?

> +	} else {
> +		irq_set_default_host(root_domain);
> +	}
>  
>  	return 0;
>  
> 

Thanks,

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

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

#1508128

Hi,

Thanks for the review.
Some comments in-line.

On 10/21/2016 10:48 AM, Marc Zyngier wrote:
> On 17/10/16 17:52, 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>
>>
>> ---
>> V4 -> V5
>> Rebased to v4.9-rc1
>> Missing curly braces
>>
>> V3 -> V4
>> Clean up if/else when a parent is found
>> Pass irqchip structure to handler as data
>>
>> V2 -> V3
>> Reused existing parent node instead of finding again.
>> Cleanup up handler based on review
>>
>> V1 -> V2
>>
>> No change
>> ---
>>  drivers/irqchip/irq-xilinx-intc.c | 26 ++++++++++++++++++++++++--
>>  1 file changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-xilinx-intc.c b/drivers/irqchip/irq-xilinx-intc.c
>> index 45e5154..dbf8b0c 100644
>> --- a/drivers/irqchip/irq-xilinx-intc.c
>> +++ b/drivers/irqchip/irq-xilinx-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 int xintc_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
>>  	.map = xintc_map,
>>  };
>>
>> +static void xil_intc_irq_handler(struct irq_desc *desc)
>> +{
>> +	u32 pending;
>> +
>> +	do {
>> +		pending = xintc_get_irq();
>> +		if (pending == -1U)
>> +			break;
>> +		generic_handle_irq(pending);
>> +	} while (true);
>
> This is missing the chained_irq_enter()/exit() calls, which will lead to
> races or lockups on the root irqchip.
>

I 'll fix it up in the next series.

>> +}
>> +
>>  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;
>>
>>  	if (xintc_irqc) {
>> @@ -221,7 +234,16 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
>>  		goto err_alloc;
>>  	}
>>
>> -	irq_set_default_host(root_domain);
>> +	if (parent) {
>> +		irq = irq_of_parse_and_map(intc, 0);
>> +		if (irq)
>> +			irq_set_chained_handler_and_data(irq,
>> +							 xil_intc_irq_handler,
>> +							 irqc);
>> +
>
> Shouldn't you return an error if irq is zero?
>

I'll add the following for the error case

	pr_err("%s: Parent exists but interrupts property not defined\n" , __func__);
	goto err_alloc;

Thanks
ZubairLK

>> +	} else {
>> +		irq_set_default_host(root_domain);
>> +	}
>>
>>  	return 0;
>>
>>
>
> Thanks,
>
> 	M.
>

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

#1508192

On Tue, 25 Oct 2016, Zubair Lutfullah Kakakhel wrote:
> On 10/21/2016 10:48 AM, Marc Zyngier wrote:
> > Shouldn't you return an error if irq is zero?
> > 
> 
> I'll add the following for the error case
> 
> 	pr_err("%s: Parent exists but interrupts property not defined\n" ,
> __func__);

Please do not use this silly __func__ stuff. It's not giving any value to
the printout.

Set a proper prefix for your pr_* stuff, so the string is prefixed with
'irq-xilinx:' or whatever you think is appropriate. Then the string itself
is good enough to find from which place this printk comes.

Thanks,

	tglx

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

#1508347

On 25/10/16 15:44, Sören Brinkmann wrote:
> On Tue, 2016-10-25 at 12:49:33 +0200, Thomas Gleixner wrote:
>> On Tue, 25 Oct 2016, Zubair Lutfullah Kakakhel wrote:
>>> On 10/21/2016 10:48 AM, Marc Zyngier wrote:
>>>> Shouldn't you return an error if irq is zero?
>>>>
>>>
>>> I'll add the following for the error case
>>>
>>> 	pr_err("%s: Parent exists but interrupts property not defined\n" ,
>>> __func__);
>>
>> Please do not use this silly __func__ stuff. It's not giving any value to
>> the printout.
>>
>> Set a proper prefix for your pr_* stuff, so the string is prefixed with
>> 'irq-xilinx:' or whatever you think is appropriate. Then the string itself
>> is good enough to find from which place this printk comes.
> 
> Haven't looked at the real code, but is there probably a way to get a
> struct device pointer and use dev_err?

You wish. Interrupt controllers (and timers) are brought up way before
the device model is available, hence no struct device.

I've started untangling that mess a couple of times, and always ran out
of available time (you start pulling the VFS, then the scheduler, the
creation of the first thread, and then things lock up because you need
to context switch and no timer is ready yet).

I may try to spend some time on it again while travelling to LPC...

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web