| Message ID | 1472661352-11983-4-git-send-email-Zubair.Kakakhel@imgtec.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
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. >
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.
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. >
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.
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;
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(-)