diff mbox

[v4,05/19] irqchip: add nps Internal and external irqchips

Message ID 1450228238-4499-6-git-send-email-noamc@ezchip.com
State Superseded
Headers show

Commit Message

Noam Camus Dec. 16, 2015, 1:10 a.m. UTC
From: Noam Camus <noamc@ezchip.com>

Adding EZchip NPS400 support.
NPS internal interrupts are internally handled at
Multi Thread Manager (MTM) that is signaled for deactivating
an interrupt.
External interrupts is handled also at Global Interrupt
Controller (GIC) e.g. serial and network devices.

Signed-off-by: Noam Camus <noamc@ezchip.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Marc Zyngier <marc.zyngier@arm.com>
---
 .../interrupt-controller/ezchip,nps400-ic.txt      |   17 +++
 drivers/irqchip/Makefile                           |    1 +
 drivers/irqchip/irq-eznps.c                        |  131 ++++++++++++++++++++
 3 files changed, 149 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ezchip,nps400-ic.txt
 create mode 100644 drivers/irqchip/irq-eznps.c

Comments

Marc Zyngier Dec. 16, 2015, 9:30 a.m. UTC | #1
On 16/12/15 01:10, Noam Camus wrote:
> From: Noam Camus <noamc@ezchip.com>
> 
> Adding EZchip NPS400 support.
> NPS internal interrupts are internally handled at
> Multi Thread Manager (MTM) that is signaled for deactivating
> an interrupt.
> External interrupts is handled also at Global Interrupt
> Controller (GIC) e.g. serial and network devices.
> 
> Signed-off-by: Noam Camus <noamc@ezchip.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  .../interrupt-controller/ezchip,nps400-ic.txt      |   17 +++
>  drivers/irqchip/Makefile                           |    1 +
>  drivers/irqchip/irq-eznps.c                        |  131 ++++++++++++++++++++
>  3 files changed, 149 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ezchip,nps400-ic.txt
>  create mode 100644 drivers/irqchip/irq-eznps.c
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/ezchip,nps400-ic.txt b/Documentation/devicetree/bindings/interrupt-controller/ezchip,nps400-ic.txt
> new file mode 100644
> index 0000000..888b2b9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/ezchip,nps400-ic.txt
> @@ -0,0 +1,17 @@
> +EZchip NPS Interrupt Controller
> +
> +Required properties:
> +
> +- compatible : should be "ezchip,nps400-ic"
> +- interrupt-controller : Identifies the node as an interrupt controller
> +- #interrupt-cells : Specifies the number of cells needed to encode an
> +  interrupt source. The value shall be 1.
> +
> +
> +Example:
> +
> +intc: interrupt-controller {
> +	compatible = "ezchip,nps400-ic";
> +	interrupt-controller;
> +	#interrupt-cells = <1>;
> +};
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 177f78f..b95b954 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -55,3 +55,4 @@ obj-$(CONFIG_RENESAS_H8S_INTC)		+= irq-renesas-h8s.o
>  obj-$(CONFIG_ARCH_SA1100)		+= irq-sa11x0.o
>  obj-$(CONFIG_INGENIC_IRQ)		+= irq-ingenic.o
>  obj-$(CONFIG_IMX_GPCV2)			+= irq-imx-gpcv2.o
> +obj-$(CONFIG_ARC_PLAT_EZNPS)		+= irq-eznps.o
> diff --git a/drivers/irqchip/irq-eznps.c b/drivers/irqchip/irq-eznps.c
> new file mode 100644
> index 0000000..fc6d59a
> --- /dev/null
> +++ b/drivers/irqchip/irq-eznps.c
> @@ -0,0 +1,131 @@
> +/*
> + * Copyright(c) 2015 EZchip Technologies.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * The full GNU General Public License is included in this distribution in
> + * the file called "COPYING".
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/irqdomain.h>
> +#include <linux/irqchip.h>
> +#include <asm/irq.h>
> +
> +/*
> + * NPS400 core includes a Interrupt Controller (IC) support.
> + * All cores can deactivate level irqs at first level control
> + * at cores mesh layer called MTM.
> + * For devices out side chip e.g. uart, network there is another
> + * level called Global Interrupt Manager (GIM).
> + * This second level can control level and edge interrupt.
> + *
> + * NOTE: AUX_IENABLE and CTOP_AUX_IACK are auxiliary registers
> + * with private HW copy per CPU.
> + */
> +
> +static void nps400_irq_mask(struct irq_data *data)
> +{
> +	unsigned int ienb;
> +
> +	ienb = read_aux_reg(AUX_IENABLE);
> +	ienb &= ~(1 << data->hwirq);
> +	write_aux_reg(AUX_IENABLE, ienb);
> +}
> +
> +static void nps400_irq_unmask(struct irq_data *data)
> +{
> +	unsigned int ienb;
> +
> +	ienb = read_aux_reg(AUX_IENABLE);
> +	ienb |= (1 << data->hwirq);
> +	write_aux_reg(AUX_IENABLE, ienb);
> +}
> +
> +static void nps400_irq_eoi_global(struct irq_data *data)
> +{
> +	write_aux_reg(CTOP_AUX_IACK, 1 << data->hwirq);
> +
> +	/* Don't ack before all device access is done */
> +	mb();
> +
> +	__asm__ __volatile__ (
> +	"       .word %0\n"
> +	:
> +	: "i"(CTOP_INST_RSPI_GIC_0_R12)
> +	: "memory");
> +}
> +
> +static void nps400_irq_eoi(struct irq_data *data)
> +{
> +	write_aux_reg(CTOP_AUX_IACK, 1 << data->hwirq);
> +}
> +
> +static struct irq_chip nps400_irq_chip_fasteoi = {
> +	.name		= "NPS400 IC Global",
> +	.irq_mask	= nps400_irq_mask,
> +	.irq_unmask	= nps400_irq_unmask,
> +	.irq_eoi	= nps400_irq_eoi_global,
> +};
> +
> +static struct irq_chip nps400_irq_chip_percpu = {
> +	.name		= "NPS400 IC",
> +	.irq_mask	= nps400_irq_mask,
> +	.irq_unmask	= nps400_irq_unmask,
> +	.irq_eoi	= nps400_irq_eoi,
> +};
> +
> +static int nps400_irq_map(struct irq_domain *d, unsigned int virq,
> +			  irq_hw_number_t hw)
> +{
> +	switch (hw) {
> +	case TIMER0_IRQ:
> +#if defined(CONFIG_SMP)
> +	case IPI_IRQ:
> +#endif
> +		irq_set_chip_and_handler(virq, &nps400_irq_chip_percpu,
> +					 handle_percpu_irq);
> +	break;
> +	default:
> +		irq_set_chip_and_handler(virq, &nps400_irq_chip_fasteoi,
> +					 handle_fasteoi_irq);
> +	break;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops nps400_irq_ops = {
> +	.xlate = irq_domain_xlate_onecell,
> +	.map = nps400_irq_map,
> +};
> +
> +static struct irq_domain *nps400_root_domain;
> +
> +static int __init nps400_of_init(struct device_node *node,
> +				 struct device_node *parent)
> +{
> +	if (parent)
> +		panic("DeviceTree incore ic not a root irq controller\n");
> +
> +	nps400_root_domain = irq_domain_add_linear(node, NR_CPU_IRQS,
> +						   &nps400_irq_ops, NULL);
> +
> +	if (!nps400_root_domain)
> +		panic("nps400 root irq domain not avail\n");
> +
> +	/* with this we don't need to export nps400_root_domain */
> +	irq_set_default_host(nps400_root_domain);

Why do you need this? Devices should have their interrupt-parent
pointing to this node, and irq_find_host should sort it out. I must be
missing some information (only being CC'd on this single patch).

> +
> +	return 0;
> +}
> +IRQCHIP_DECLARE(ezchip_nps400_ic, "ezchip,nps400-ic", nps400_of_init);
> 

Another thing I'm not seeing here is where is the interrupt actually
taken. This code only contains the EOI part, but not the ACK side, as
well as the reverse lookup hwirq -> irq). Where is that code?

Thanks,

	M.
Noam Camus Dec. 18, 2015, 10:37 a.m. UTC | #2
From: Marc Zyngier [mailto:marc.zyngier@arm.com] 
Sent: Wednesday, December 16, 2015 11:31 AM

>> +static int __init nps400_of_init(struct device_node *node,
>> +				 struct device_node *parent)
>> +{
>> +	if (parent)
>> +		panic("DeviceTree incore ic not a root irq controller\n");
>> +
>> +	nps400_root_domain = irq_domain_add_linear(node, NR_CPU_IRQS,
>> +						   &nps400_irq_ops, NULL);
>> +
>> +	if (!nps400_root_domain)
>> +		panic("nps400 root irq domain not avail\n");
>> +
>> +	/* with this we don't need to export nps400_root_domain */
>> +	irq_set_default_host(nps400_root_domain);

>Why do you need this? Devices should have their interrupt-parent pointing to this node, and irq_find_host should sort it >out. I must be missing some information (only being CC'd on this single patch).
Sorry, I will CC you by my next version, in the meantime please refer to:
https://lkml.org/lkml/2015/12/15/864

I need this for my per CPU irqs such timer and IPI which do not come from some external device but from CPUs.
For these IRQs I am calling to irq_create_mapping() from my platform at arch/arc and at that point I got no irqdomain and using irq_find_host() is not good since I got no device_node (at most I can have DT root).
Is there device_node for DT root? 
Please advise what to do?

>> +
>> +	return 0;
>> +}
>> +IRQCHIP_DECLARE(ezchip_nps400_ic, "ezchip,nps400-ic", 
>> +nps400_of_init);
>> 

>Another thing I'm not seeing here is where is the interrupt actually taken. This code only contains the EOI part, but not the ACK side, as well as the reverse lookup hwirq -> irq). Where is that code?

ACK is an optional handler and is not needed by my platform.
I will add comment that since my IRQs are EOI based I do not need an ACK.

I do not understand reverse lookup remark, where is it missing?
Could you point me to an example for such reverse lookup? 

Regards,
Noam
Marc Zyngier Dec. 18, 2015, 11:21 a.m. UTC | #3
On 18/12/15 10:37, Noam Camus wrote:
> From: Marc Zyngier [mailto:marc.zyngier@arm.com] 
> Sent: Wednesday, December 16, 2015 11:31 AM
> 
>>> +static int __init nps400_of_init(struct device_node *node,
>>> +				 struct device_node *parent)
>>> +{
>>> +	if (parent)
>>> +		panic("DeviceTree incore ic not a root irq controller\n");
>>> +
>>> +	nps400_root_domain = irq_domain_add_linear(node, NR_CPU_IRQS,
>>> +						   &nps400_irq_ops, NULL);
>>> +
>>> +	if (!nps400_root_domain)
>>> +		panic("nps400 root irq domain not avail\n");
>>> +
>>> +	/* with this we don't need to export nps400_root_domain */
>>> +	irq_set_default_host(nps400_root_domain);
> 
>> Why do you need this? Devices should have their interrupt-parent pointing to this node, and irq_find_host should sort it >out. I must be missing some information (only being CC'd on this single patch).
> Sorry, I will CC you by my next version, in the meantime please refer to:
> https://lkml.org/lkml/2015/12/15/864
> 
> I need this for my per CPU irqs such timer and IPI which do not come
> from some external device but from CPUs. For these IRQs I am calling
> to irq_create_mapping() from my platform at arch/arc and at that
> point I got no irqdomain and using irq_find_host() is not good since
> I got no device_node (at most I can have DT root).

That's a problem. You should never do that for your timer (doing a
request_irq will do the right thing, and that's what your timer driver
already does).

As for initializing your IPIs, they are usually outside of the IRQ
space, so you should handle them separately (and get your irqchip to
initialize them).

> Is there device_node for DT root? 
> Please advise what to do?
> 
>>> +
>>> +	return 0;
>>> +}
>>> +IRQCHIP_DECLARE(ezchip_nps400_ic, "ezchip,nps400-ic", 
>>> +nps400_of_init);
>>>
> 
>> Another thing I'm not seeing here is where is the interrupt actually taken. This code only contains the EOI part, but not the ACK side, as well as the reverse lookup hwirq -> irq). Where is that code?
> 
> ACK is an optional handler and is not needed by my platform.
> I will add comment that since my IRQs are EOI based I do not need an ACK.

What I'm talking about is not the irq_ack method that the irqchip can
provide, but the action your perform on your interrupt controller to
extract the hwirq number and find out what corresponding Linux interrupt
has fired.

> 
> I do not understand reverse lookup remark, where is it missing?
> Could you point me to an example for such reverse lookup? 

See for example:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-sun4i.c#n136

This is the function (sun4i_handle_irq) that is executed almost
immediately after the IRQ is asserted. The CPU reads the hwirq from the
interrupt controller, and use handle_domain_irq() to call the
corresponding handler (by doing a lookup in the domain).

I couldn't find out in your platform code where you are doing that.

Thanks,

	M.
Noam Camus Dec. 18, 2015, 2:29 p.m. UTC | #4
>From: Marc Zyngier [mailto:marc.zyngier@arm.com] 
>Sent: Friday, December 18, 2015 1:21 PM

>> I need this for my per CPU irqs such timer and IPI which do not come 
>> from some external device but from CPUs. For these IRQs I am calling 
>> to irq_create_mapping() from my platform at arch/arc and at that point 
>> I got no irqdomain and using irq_find_host() is not good since I got 
>> no device_node (at most I can have DT root).

>That's a problem. You should never do that for your timer (doing a request_irq will do the right thing, and that's what your timer driver already does).

Please be more specific, from all that I wrote what is the problem?
When I use request_irq() it fail without the mapping and mapping fail without a domain.
Never do what?
What should I do then?

>As for initializing your IPIs, they are usually outside of the IRQ space, so you should handle them separately (and get your irqchip to initialize them).
I am handling all my IRQs within same irqchip, which is the only one I have. So I am not sure what you expect here.
Please be more elaborate.

>> 
>> Another thing I'm not seeing here is where is the interrupt actually taken. This code only contains the EOI part, but not the ACK side, as well as the reverse lookup hwirq -> irq). Where is that code?
>> 
>> ACK is an optional handler and is not needed by my platform.
>> I will add comment that since my IRQs are EOI based I do not need an ACK.

>What I'm talking about is not the irq_ack method that the irqchip can provide, but the action your perform on your interrupt controller to extract the hwirq number and find out what corresponding Linux interrupt has fired.

>> 
>> I do not understand reverse lookup remark, where is it missing?
>> Could you point me to an example for such reverse lookup? 

>See for example:

>https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-sun4i.c#n136

>This is the function (sun4i_handle_irq) that is executed almost immediately after the IRQ is asserted. The CPU reads the hwirq from the interrupt controller, and use handle_domain_irq() to call the corresponding handler (by doing a lookup in the domain).

>I couldn't find out in your platform code where you are doing that.

OK, this is seem much like exclusively ARM stuff.
Note that I am working with ARC (seem alike) here and we do not define CONFIG_HANDLE_DOMAIN_IRQ and do not implement set_handle_irq().

So for ARC this reverse mapping is something we can leave without (maybe because we are kind of a legacy domain).

Note that this patch is part of a set I introduce to ARC as a new platform.
You can view my tree and the patch set at:
https://github.com/EZchip/linux/commits/EZ-arc-for-next-basic-reviewd-DO-NOT-COMMENT

-Noam
Marc Zyngier Dec. 18, 2015, 4:31 p.m. UTC | #5
On 18/12/15 14:29, Noam Camus wrote:
>> From: Marc Zyngier [mailto:marc.zyngier@arm.com] 
>> Sent: Friday, December 18, 2015 1:21 PM
> 
>>> I need this for my per CPU irqs such timer and IPI which do not come 
>>> from some external device but from CPUs. For these IRQs I am calling 
>>> to irq_create_mapping() from my platform at arch/arc and at that point 
>>> I got no irqdomain and using irq_find_host() is not good since I got 
>>> no device_node (at most I can have DT root).
> 
>> That's a problem. You should never do that for your timer (doing a request_irq will do the right thing, and that's what your timer driver already does).
> 
> Please be more specific, from all that I wrote what is the problem?

Calling irq_create_mapping out of the blue like you do it here:

+static void eznps_init_per_cpu(int cpu)
+{
+	/* Create mapping for all per cpu IRQs */
+	if (cpu == 0) {
+		irq_create_mapping(NULL, TIMER0_IRQ);
+		irq_create_mapping(NULL, IPI_IRQ);
+	}

is simply not acceptable.

> When I use request_irq() it fail without the mapping and mapping fail without a domain.

Grmbl...

That's not completely surprising:

+		timer {
+			compatible = "ezchip,nps400-timer";
+			clocks = <&sysclk>;
+			clock-names="sysclk";
+		};

Where is the interrupt?

> Never do what?
> What should I do then?

Maybe you should start by looking how the other architectures have
solved that exact problem at least half a dozen time.

> 
>> As for initializing your IPIs, they are usually outside of the IRQ
>> space, so you should handle them separately (and get your irqchip
>> to initialize them).
> I am handling all my IRQs within same irqchip, which is the only one 
> I have. So I am not sure what you expect here. Please be more 
> elaborate.

Do not create a mapping for IPIs. Full stop. Handle them independently
from your normal IRQs.

>>>
>>> Another thing I'm not seeing here is where is the interrupt actually taken. This code only contains the EOI part, but not the ACK side, as well as the reverse lookup hwirq -> irq). Where is that code?
>>>
>>> ACK is an optional handler and is not needed by my platform.
>>> I will add comment that since my IRQs are EOI based I do not need an ACK.
> 
>> What I'm talking about is not the irq_ack method that the irqchip can provide, but the action your perform on your interrupt controller to extract the hwirq number and find out what corresponding Linux interrupt has fired.
> 
>>>
>>> I do not understand reverse lookup remark, where is it missing?
>>> Could you point me to an example for such reverse lookup? 
> 
>> See for example:
> 
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-sun4i.c#n136
> 
>> This is the function (sun4i_handle_irq) that is executed almost immediately after the IRQ is asserted. The CPU reads the hwirq from the interrupt controller, and use handle_domain_irq() to call the corresponding handler (by doing a lookup in the domain).
> 
>> I couldn't find out in your platform code where you are doing that.
> 
> OK, this is seem much like exclusively ARM stuff.

No, this is not. Can you please stop looking at the surface of things
and start taking an interest in how things actually *work*? Almost
*nothing* in the interrupt handling code is architecture specific.

> Note that I am working with ARC (seem alike) here and we do not
> define CONFIG_HANDLE_DOMAIN_IRQ and do not implement
> set_handle_irq().
> 
> So for ARC this reverse mapping is something we can leave without
> (maybe because we are kind of a legacy domain).

Yeah, I just located the crap: arch_do_IRQ() happily takes a hwirq (the
vector number), and uses that as a Linux IRQ. This looks a lot like ARM
pre-DT, about 10 years ago.

Well, time to meet the 21st century. If you intend to use DT, please fix
your arch port. Otherwise, just hardcode everything in your platform and
don't pretend to support device tree.

Thanks,

	M.
Noam Camus Dec. 19, 2015, 9:30 a.m. UTC | #6
>From: Marc Zyngier [mailto:marc.zyngier@arm.com] 
>Sent: Friday, December 18, 2015 6:31 PM

>> Note that I am working with ARC (seem alike) here and we do not define 
>> CONFIG_HANDLE_DOMAIN_IRQ and do not implement set_handle_irq().
>> 
>> So for ARC this reverse mapping is something we can leave without 
>> (maybe because we are kind of a legacy domain).

>Yeah, I just located the crap: arch_do_IRQ() happily takes a hwirq (the vector number), and uses that as a Linux IRQ. This looks a lot like ARM pre-DT, about 10 years ago.

>Well, time to meet the 21st century. If you intend to use DT, please fix your arch port. Otherwise, just hardcode everything in your platform and don't pretend to support device tree.

Thank you very much. 
I have the complete picture now.

Will discuss with ARC maintainer and see what best way to do this change. 

-Noam
Vineet Gupta Dec. 30, 2015, 11:35 a.m. UTC | #7
On Friday 18 December 2015 10:01 PM, Marc Zyngier wrote:
> On 18/12/15 14:29, Noam Camus wrote:
>>> From: Marc Zyngier [mailto:marc.zyngier@arm.com] 
>>> Sent: Friday, December 18, 2015 1:21 PM
>>
>>>> I need this for my per CPU irqs such timer and IPI which do not come 
>>>> from some external device but from CPUs. For these IRQs I am calling 
>>>> to irq_create_mapping() from my platform at arch/arc and at that point 
>>>> I got no irqdomain and using irq_find_host() is not good since I got 
>>>> no device_node (at most I can have DT root).
>>
>>> That's a problem. You should never do that for your timer (doing a request_irq will do the right thing, and that's what your timer driver already does).
>>
>> Please be more specific, from all that I wrote what is the problem?
> 
> Calling irq_create_mapping out of the blue like you do it here:
> 
> +static void eznps_init_per_cpu(int cpu)
> +{
> +	/* Create mapping for all per cpu IRQs */
> +	if (cpu == 0) {
> +		irq_create_mapping(NULL, TIMER0_IRQ);
> +		irq_create_mapping(NULL, IPI_IRQ);
> +	}
> 
> is simply not acceptable.
> 
>> When I use request_irq() it fail without the mapping and mapping fail without a domain.
> 
> Grmbl...
> 
> That's not completely surprising:
> 
> +		timer {
> +			compatible = "ezchip,nps400-timer";
> +			clocks = <&sysclk>;
> +			clock-names="sysclk";
> +		};
> 
> Where is the interrupt?
> 
>> Never do what?
>> What should I do then?
> 
> Maybe you should start by looking how the other architectures have
> solved that exact problem at least half a dozen time.
> 
>>
>>> As for initializing your IPIs, they are usually outside of the IRQ
>>> space, so you should handle them separately (and get your irqchip
>>> to initialize them).
>> I am handling all my IRQs within same irqchip, which is the only one 
>> I have. So I am not sure what you expect here. Please be more 
>> elaborate.
> 
> Do not create a mapping for IPIs. Full stop. Handle them independently
> from your normal IRQs.
> 
>>>>
>>>> Another thing I'm not seeing here is where is the interrupt actually taken. This code only contains the EOI part, but not the ACK side, as well as the reverse lookup hwirq -> irq). Where is that code?
>>>>
>>>> ACK is an optional handler and is not needed by my platform.
>>>> I will add comment that since my IRQs are EOI based I do not need an ACK.
>>
>>> What I'm talking about is not the irq_ack method that the irqchip can provide, but the action your perform on your interrupt controller to extract the hwirq number and find out what corresponding Linux interrupt has fired.
>>
>>>>
>>>> I do not understand reverse lookup remark, where is it missing?
>>>> Could you point me to an example for such reverse lookup? 
>>
>>> See for example:
>>
>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-sun4i.c#n136
>>
>>> This is the function (sun4i_handle_irq) that is executed almost immediately after the IRQ is asserted. The CPU reads the hwirq from the interrupt controller, and use handle_domain_irq() to call the corresponding handler (by doing a lookup in the domain).
>>
>>> I couldn't find out in your platform code where you are doing that.
>>
>> OK, this is seem much like exclusively ARM stuff.
> 
> No, this is not. Can you please stop looking at the surface of things
> and start taking an interest in how things actually *work*? Almost
> *nothing* in the interrupt handling code is architecture specific.
> 
>> Note that I am working with ARC (seem alike) here and we do not
>> define CONFIG_HANDLE_DOMAIN_IRQ and do not implement
>> set_handle_irq().
>>
>> So for ARC this reverse mapping is something we can leave without
>> (maybe because we are kind of a legacy domain).
> 
> Yeah, I just located the crap: arch_do_IRQ() happily takes a hwirq (the
> vector number), and uses that as a Linux IRQ. This looks a lot like ARM
> pre-DT, about 10 years ago.
> 
> Well, time to meet the 21st century. If you intend to use DT, please fix
> your arch port. Otherwise, just hardcode everything in your platform and
> don't pretend to support device tree.

Hi Marc,

Taking your rant in a positive stride and I'm all up for making this as
nice/modern as possible. I don't have issues with enabling
CONFIG_HANDLE_DOMAIN_IRQ for ARC (although it might add a few cycles o/h to each ISR)

However currently (4.4-rcX) it is only enabled for arm/arm64/openrisc and from the
looks of it in drivers/irqchip, only ARM based SoCs use the handle_domain_irq()
calls by plugging into ARM top level handler.

Why is that not a problem for other arches like PPC/MIPS which also use DT
heavily. Or perhaps they are also subtly broken as ARC is !

What am I missing ?

Thx,
-Vineet
Vineet Gupta Jan. 12, 2016, 7 a.m. UTC | #8
Hi Marc,

On Wednesday 30 December 2015 05:05 PM, Vineet Gupta wrote:
> On Friday 18 December 2015 10:01 PM, Marc Zyngier wrote:
>> On 18/12/15 14:29, Noam Camus wrote:
>>>> From: Marc Zyngier [mailto:marc.zyngier@arm.com] 
>>>> Sent: Friday, December 18, 2015 1:21 PM
>>>
>>>>> I need this for my per CPU irqs such timer and IPI which do not come 
>>>>> from some external device but from CPUs. For these IRQs I am calling 
>>>>> to irq_create_mapping() from my platform at arch/arc and at that point 
>>>>> I got no irqdomain and using irq_find_host() is not good since I got 
>>>>> no device_node (at most I can have DT root).
>>>
>>>> That's a problem. You should never do that for your timer (doing a request_irq will do the right thing, and that's what your timer driver already does).
>>>
>>> Please be more specific, from all that I wrote what is the problem?
>>
>> Calling irq_create_mapping out of the blue like you do it here:
>>
>> +static void eznps_init_per_cpu(int cpu)
>> +{
>> +	/* Create mapping for all per cpu IRQs */
>> +	if (cpu == 0) {
>> +		irq_create_mapping(NULL, TIMER0_IRQ);
>> +		irq_create_mapping(NULL, IPI_IRQ);
>> +	}
>>
>> is simply not acceptable.
>>
>>> When I use request_irq() it fail without the mapping and mapping fail without a domain.
>>
>> Grmbl...
>>
>> That's not completely surprising:
>>
>> +		timer {
>> +			compatible = "ezchip,nps400-timer";
>> +			clocks = <&sysclk>;
>> +			clock-names="sysclk";
>> +		};
>>
>> Where is the interrupt?
>>
>>> Never do what?
>>> What should I do then?
>>
>> Maybe you should start by looking how the other architectures have
>> solved that exact problem at least half a dozen time.
>>
>>>
>>>> As for initializing your IPIs, they are usually outside of the IRQ
>>>> space, so you should handle them separately (and get your irqchip
>>>> to initialize them).
>>> I am handling all my IRQs within same irqchip, which is the only one 
>>> I have. So I am not sure what you expect here. Please be more 
>>> elaborate.
>>
>> Do not create a mapping for IPIs. Full stop. Handle them independently
>> from your normal IRQs.
>>
>>>>>
>>>>> Another thing I'm not seeing here is where is the interrupt actually taken. This code only contains the EOI part, but not the ACK side, as well as the reverse lookup hwirq -> irq). Where is that code?
>>>>>
>>>>> ACK is an optional handler and is not needed by my platform.
>>>>> I will add comment that since my IRQs are EOI based I do not need an ACK.
>>>
>>>> What I'm talking about is not the irq_ack method that the irqchip can provide, but the action your perform on your interrupt controller to extract the hwirq number and find out what corresponding Linux interrupt has fired.
>>>
>>>>>
>>>>> I do not understand reverse lookup remark, where is it missing?
>>>>> Could you point me to an example for such reverse lookup? 
>>>
>>>> See for example:
>>>
>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-sun4i.c#n136
>>>
>>>> This is the function (sun4i_handle_irq) that is executed almost immediately after the IRQ is asserted. The CPU reads the hwirq from the interrupt controller, and use handle_domain_irq() to call the corresponding handler (by doing a lookup in the domain).
>>>
>>>> I couldn't find out in your platform code where you are doing that.
>>>
>>> OK, this is seem much like exclusively ARM stuff.
>>
>> No, this is not. Can you please stop looking at the surface of things
>> and start taking an interest in how things actually *work*? Almost
>> *nothing* in the interrupt handling code is architecture specific.
>>
>>> Note that I am working with ARC (seem alike) here and we do not
>>> define CONFIG_HANDLE_DOMAIN_IRQ and do not implement
>>> set_handle_irq().
>>>
>>> So for ARC this reverse mapping is something we can leave without
>>> (maybe because we are kind of a legacy domain).
>>
>> Yeah, I just located the crap: arch_do_IRQ() happily takes a hwirq (the
>> vector number), and uses that as a Linux IRQ. This looks a lot like ARM
>> pre-DT, about 10 years ago.
>>
>> Well, time to meet the 21st century. If you intend to use DT, please fix
>> your arch port. Otherwise, just hardcode everything in your platform and
>> don't pretend to support device tree.
> 
> Hi Marc,
> 
> Taking your rant in a positive stride and I'm all up for making this as
> nice/modern as possible. I don't have issues with enabling
> CONFIG_HANDLE_DOMAIN_IRQ for ARC (although it might add a few cycles o/h to each ISR)
> 
> However currently (4.4-rcX) it is only enabled for arm/arm64/openrisc and from the
> looks of it in drivers/irqchip, only ARM based SoCs use the handle_domain_irq()
> calls by plugging into ARM top level handler.
> 
> Why is that not a problem for other arches like PPC/MIPS which also use DT
> heavily. Or perhaps they are also subtly broken as ARC is !
> 
> What am I missing ?

Marc, I hope you are back from holidays. When u get a chance could you please
respond to above.

Also I tool a stab at it anyways.
1. Enabled HANDLE_DOMAIN_IRQ
2. arch_do_IRQ() calls handle_domain_irq(NULL, hwirq, regs) since that code
doesn't know about domains.

It fails w/o irq_set_default_host() being called.

If we go back to sunxi example you quoted above, it relies on driver passing the
domain to handle_domain_irq(). IMHO it is simpler if we had the default domain.

So long story short, ARC can be made to use handle_domain_irq() w/o the song and
dance of registering another callback from irqchip code if we retained the default
domain setting.

-Vineet
Marc Zyngier Jan. 12, 2016, 8:48 a.m. UTC | #9
Hi Vineet,

Sorry I missed that one, it must have been caught in the mother of all
"mark as read" I did on coming back from holiday.

On 12/01/16 07:00, Vineet Gupta wrote:
> Hi Marc,
> 
> On Wednesday 30 December 2015 05:05 PM, Vineet Gupta wrote:
>> On Friday 18 December 2015 10:01 PM, Marc Zyngier wrote:
>>> On 18/12/15 14:29, Noam Camus wrote:
>>>>> From: Marc Zyngier [mailto:marc.zyngier@arm.com] 
>>>>> Sent: Friday, December 18, 2015 1:21 PM
>>>>
>>>>>> I need this for my per CPU irqs such timer and IPI which do not come 
>>>>>> from some external device but from CPUs. For these IRQs I am calling 
>>>>>> to irq_create_mapping() from my platform at arch/arc and at that point 
>>>>>> I got no irqdomain and using irq_find_host() is not good since I got 
>>>>>> no device_node (at most I can have DT root).
>>>>
>>>>> That's a problem. You should never do that for your timer (doing a request_irq will do the right thing, and that's what your timer driver already does).
>>>>
>>>> Please be more specific, from all that I wrote what is the problem?
>>>
>>> Calling irq_create_mapping out of the blue like you do it here:
>>>
>>> +static void eznps_init_per_cpu(int cpu)
>>> +{
>>> +	/* Create mapping for all per cpu IRQs */
>>> +	if (cpu == 0) {
>>> +		irq_create_mapping(NULL, TIMER0_IRQ);
>>> +		irq_create_mapping(NULL, IPI_IRQ);
>>> +	}
>>>
>>> is simply not acceptable.
>>>
>>>> When I use request_irq() it fail without the mapping and mapping fail without a domain.
>>>
>>> Grmbl...
>>>
>>> That's not completely surprising:
>>>
>>> +		timer {
>>> +			compatible = "ezchip,nps400-timer";
>>> +			clocks = <&sysclk>;
>>> +			clock-names="sysclk";
>>> +		};
>>>
>>> Where is the interrupt?
>>>
>>>> Never do what?
>>>> What should I do then?
>>>
>>> Maybe you should start by looking how the other architectures have
>>> solved that exact problem at least half a dozen time.
>>>
>>>>
>>>>> As for initializing your IPIs, they are usually outside of the IRQ
>>>>> space, so you should handle them separately (and get your irqchip
>>>>> to initialize them).
>>>> I am handling all my IRQs within same irqchip, which is the only one 
>>>> I have. So I am not sure what you expect here. Please be more 
>>>> elaborate.
>>>
>>> Do not create a mapping for IPIs. Full stop. Handle them independently
>>> from your normal IRQs.
>>>
>>>>>>
>>>>>> Another thing I'm not seeing here is where is the interrupt actually taken. This code only contains the EOI part, but not the ACK side, as well as the reverse lookup hwirq -> irq). Where is that code?
>>>>>>
>>>>>> ACK is an optional handler and is not needed by my platform.
>>>>>> I will add comment that since my IRQs are EOI based I do not need an ACK.
>>>>
>>>>> What I'm talking about is not the irq_ack method that the irqchip can provide, but the action your perform on your interrupt controller to extract the hwirq number and find out what corresponding Linux interrupt has fired.
>>>>
>>>>>>
>>>>>> I do not understand reverse lookup remark, where is it missing?
>>>>>> Could you point me to an example for such reverse lookup? 
>>>>
>>>>> See for example:
>>>>
>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-sun4i.c#n136
>>>>
>>>>> This is the function (sun4i_handle_irq) that is executed almost immediately after the IRQ is asserted. The CPU reads the hwirq from the interrupt controller, and use handle_domain_irq() to call the corresponding handler (by doing a lookup in the domain).
>>>>
>>>>> I couldn't find out in your platform code where you are doing that.
>>>>
>>>> OK, this is seem much like exclusively ARM stuff.
>>>
>>> No, this is not. Can you please stop looking at the surface of things
>>> and start taking an interest in how things actually *work*? Almost
>>> *nothing* in the interrupt handling code is architecture specific.
>>>
>>>> Note that I am working with ARC (seem alike) here and we do not
>>>> define CONFIG_HANDLE_DOMAIN_IRQ and do not implement
>>>> set_handle_irq().
>>>>
>>>> So for ARC this reverse mapping is something we can leave without
>>>> (maybe because we are kind of a legacy domain).
>>>
>>> Yeah, I just located the crap: arch_do_IRQ() happily takes a hwirq (the
>>> vector number), and uses that as a Linux IRQ. This looks a lot like ARM
>>> pre-DT, about 10 years ago.
>>>
>>> Well, time to meet the 21st century. If you intend to use DT, please fix
>>> your arch port. Otherwise, just hardcode everything in your platform and
>>> don't pretend to support device tree.
>>
>> Hi Marc,
>>
>> Taking your rant in a positive stride and I'm all up for making this as
>> nice/modern as possible. I don't have issues with enabling
>> CONFIG_HANDLE_DOMAIN_IRQ for ARC (although it might add a few cycles o/h to each ISR)
>>
>> However currently (4.4-rcX) it is only enabled for arm/arm64/openrisc and from the
>> looks of it in drivers/irqchip, only ARM based SoCs use the handle_domain_irq()
>> calls by plugging into ARM top level handler.
>>
>> Why is that not a problem for other arches like PPC/MIPS which also use DT
>> heavily. Or perhaps they are also subtly broken as ARC is !
>>
>> What am I missing ?

HANDLE_DOMAIN_IRQ is not mandatory at all - a number of architectures
had something open-coded in the past (with some drawbacks and/or bugs),
and this config option is just one of the ways to get it right.

MIPS/PPC perform the reverse lookup directly, without using this
infrastructure, and that would be a valid approach for ARC too.

> Marc, I hope you are back from holidays. When u get a chance could you please
> respond to above.
> 
> Also I tool a stab at it anyways.
> 1. Enabled HANDLE_DOMAIN_IRQ
> 2. arch_do_IRQ() calls handle_domain_irq(NULL, hwirq, regs) since that code
> doesn't know about domains.
> 
> It fails w/o irq_set_default_host() being called.

Well, that's expected. unless you use the underlying primitive
(__handle_domain_irq) and pass false as the parameter for lookup (see
handle_IRQ() in arch/arm/kernel/irq.c). This is what we use for "legacy"
platforms that do not support domains.

> If we go back to sunxi example you quoted above, it relies on driver passing the
> domain to handle_domain_irq(). IMHO it is simpler if we had the default domain.
> 
> So long story short, ARC can be made to use handle_domain_irq() w/o the song and
> dance of registering another callback from irqchip code if we retained the default
> domain setting.

This only works if you can guarantee that you will never have another
irqchip calling irq_set_default_host()... If your system is always
simple enough to guarantee that, why not.

Thanks,

	M.
Vineet Gupta Jan. 12, 2016, 9:12 a.m. UTC | #10
On Tuesday 12 January 2016 02:18 PM, Marc Zyngier wrote:
> Hi Vineet,
>
> Sorry I missed that one, it must have been caught in the mother of all
> "mark as read" I did on coming back from holiday.

That was expected - NP :-)

>
> On 12/01/16 07:00, Vineet Gupta wrote:
>> > Hi Marc,
>> > 
>> > On Wednesday 30 December 2015 05:05 PM, Vineet Gupta wrote:
>>> >> On Friday 18 December 2015 10:01 PM, Marc Zyngier wrote:
>>>> >>> On 18/12/15 14:29, Noam Camus wrote:
>>>>>> >>>>> From: Marc Zyngier [mailto:marc.zyngier@arm.com] 
>>>>>> >>>>> Sent: Friday, December 18, 2015 1:21 PM
>>>>> >>>>
>>>>>>> >>>>>> I need this for my per CPU irqs such timer and IPI which do not come 
>>>>>>> >>>>>> from some external device but from CPUs. For these IRQs I am calling 
>>>>>>> >>>>>> to irq_create_mapping() from my platform at arch/arc and at that point 
>>>>>>> >>>>>> I got no irqdomain and using irq_find_host() is not good since I got 
>>>>>>> >>>>>> no device_node (at most I can have DT root).
>>>>> >>>>
>>>>>> >>>>> That's a problem. You should never do that for your timer (doing a request_irq will do the right thing, and that's what your timer driver already does).
>>>>> >>>>
>>>>> >>>> Please be more specific, from all that I wrote what is the problem?
>>>> >>>
>>>> >>> Calling irq_create_mapping out of the blue like you do it here:
>>>> >>>
>>>> >>> +static void eznps_init_per_cpu(int cpu)
>>>> >>> +{
>>>> >>> +	/* Create mapping for all per cpu IRQs */
>>>> >>> +	if (cpu == 0) {
>>>> >>> +		irq_create_mapping(NULL, TIMER0_IRQ);
>>>> >>> +		irq_create_mapping(NULL, IPI_IRQ);
>>>> >>> +	}
>>>> >>>
>>>> >>> is simply not acceptable.
>>>> >>>
>>>>> >>>> When I use request_irq() it fail without the mapping and mapping fail without a domain.
>>>> >>>
>>>> >>> Grmbl...
>>>> >>>
>>>> >>> That's not completely surprising:
>>>> >>>
>>>> >>> +		timer {
>>>> >>> +			compatible = "ezchip,nps400-timer";
>>>> >>> +			clocks = <&sysclk>;
>>>> >>> +			clock-names="sysclk";
>>>> >>> +		};
>>>> >>>
>>>> >>> Where is the interrupt?
>>>> >>>
>>>>> >>>> Never do what?
>>>>> >>>> What should I do then?
>>>> >>>
>>>> >>> Maybe you should start by looking how the other architectures have
>>>> >>> solved that exact problem at least half a dozen time.
>>>> >>>
>>>>> >>>>
>>>>>> >>>>> As for initializing your IPIs, they are usually outside of the IRQ
>>>>>> >>>>> space, so you should handle them separately (and get your irqchip
>>>>>> >>>>> to initialize them).
>>>>> >>>> I am handling all my IRQs within same irqchip, which is the only one 
>>>>> >>>> I have. So I am not sure what you expect here. Please be more 
>>>>> >>>> elaborate.
>>>> >>>
>>>> >>> Do not create a mapping for IPIs. Full stop. Handle them independently
>>>> >>> from your normal IRQs.
>>>> >>>
>>>>>>> >>>>>>
>>>>>>> >>>>>> Another thing I'm not seeing here is where is the interrupt actually taken. This code only contains the EOI part, but not the ACK side, as well as the reverse lookup hwirq -> irq). Where is that code?
>>>>>>> >>>>>>
>>>>>>> >>>>>> ACK is an optional handler and is not needed by my platform.
>>>>>>> >>>>>> I will add comment that since my IRQs are EOI based I do not need an ACK.
>>>>> >>>>
>>>>>> >>>>> What I'm talking about is not the irq_ack method that the irqchip can provide, but the action your perform on your interrupt controller to extract the hwirq number and find out what corresponding Linux interrupt has fired.
>>>>> >>>>
>>>>>>> >>>>>>
>>>>>>> >>>>>> I do not understand reverse lookup remark, where is it missing?
>>>>>>> >>>>>> Could you point me to an example for such reverse lookup? 
>>>>> >>>>
>>>>>> >>>>> See for example:
>>>>> >>>>
>>>>>> >>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-sun4i.c#n136
>>>>> >>>>
>>>>>> >>>>> This is the function (sun4i_handle_irq) that is executed almost immediately after the IRQ is asserted. The CPU reads the hwirq from the interrupt controller, and use handle_domain_irq() to call the corresponding handler (by doing a lookup in the domain).
>>>>> >>>>
>>>>>> >>>>> I couldn't find out in your platform code where you are doing that.
>>>>> >>>>
>>>>> >>>> OK, this is seem much like exclusively ARM stuff.
>>>> >>>
>>>> >>> No, this is not. Can you please stop looking at the surface of things
>>>> >>> and start taking an interest in how things actually *work*? Almost
>>>> >>> *nothing* in the interrupt handling code is architecture specific.
>>>> >>>
>>>>> >>>> Note that I am working with ARC (seem alike) here and we do not
>>>>> >>>> define CONFIG_HANDLE_DOMAIN_IRQ and do not implement
>>>>> >>>> set_handle_irq().
>>>>> >>>>
>>>>> >>>> So for ARC this reverse mapping is something we can leave without
>>>>> >>>> (maybe because we are kind of a legacy domain).
>>>> >>>
>>>> >>> Yeah, I just located the crap: arch_do_IRQ() happily takes a hwirq (the
>>>> >>> vector number), and uses that as a Linux IRQ. This looks a lot like ARM
>>>> >>> pre-DT, about 10 years ago.
>>>> >>>
>>>> >>> Well, time to meet the 21st century. If you intend to use DT, please fix
>>>> >>> your arch port. Otherwise, just hardcode everything in your platform and
>>>> >>> don't pretend to support device tree.
>>> >>
>>> >> Hi Marc,
>>> >>
>>> >> Taking your rant in a positive stride and I'm all up for making this as
>>> >> nice/modern as possible. I don't have issues with enabling
>>> >> CONFIG_HANDLE_DOMAIN_IRQ for ARC (although it might add a few cycles o/h to each ISR)
>>> >>
>>> >> However currently (4.4-rcX) it is only enabled for arm/arm64/openrisc and from the
>>> >> looks of it in drivers/irqchip, only ARM based SoCs use the handle_domain_irq()
>>> >> calls by plugging into ARM top level handler.
>>> >>
>>> >> Why is that not a problem for other arches like PPC/MIPS which also use DT
>>> >> heavily. Or perhaps they are also subtly broken as ARC is !
>>> >>
>>> >> What am I missing ?
> HANDLE_DOMAIN_IRQ is not mandatory at all - a number of architectures
> had something open-coded in the past (with some drawbacks and/or bugs),
> and this config option is just one of the ways to get it right.
>
> MIPS/PPC perform the reverse lookup directly, without using this
> infrastructure, and that would be a valid approach for ARC too.

I'd rather use the generic infrastructure and improve it if needed !

>> > Marc, I hope you are back from holidays. When u get a chance could you please
>> > respond to above.
>> > 
>> > Also I tool a stab at it anyways.
>> > 1. Enabled HANDLE_DOMAIN_IRQ
>> > 2. arch_do_IRQ() calls handle_domain_irq(NULL, hwirq, regs) since that code
>> > doesn't know about domains.
>> > 
>> > It fails w/o irq_set_default_host() being called.
> Well, that's expected. unless you use the underlying primitive
> (__handle_domain_irq) and pass false as the parameter for lookup (see
> handle_IRQ() in arch/arm/kernel/irq.c). This is what we use for "legacy"
> platforms that do not support domains.

Right I saw that and that causes virq = hwirq - kind of defeats the purpose if we
were doing this on ARC.

>
>> > If we go back to sunxi example you quoted above, it relies on driver passing the
>> > domain to handle_domain_irq(). IMHO it is simpler if we had the default domain.
>> > 
>> > So long story short, ARC can be made to use handle_domain_irq() w/o the song and
>> > dance of registering another callback from irqchip code if we retained the default
>> > domain setting.
> This only works if you can guarantee that you will never have another
> irqchip calling irq_set_default_host()... If your system is always
> simple enough to guarantee that, why not.

ATM we can certainly assume that.

However with ARM approach two irqchips can still call set_handle_irq() and only
the first one succeeds (and others return silently). That seems wrong to me -
irq_xxx.c will still use the handler registered by say irq_sun41.c ?

Thx,
-Vineet
Marc Zyngier Jan. 12, 2016, 9:28 a.m. UTC | #11
On 12/01/16 09:12, Vineet Gupta wrote:

[...]

>> HANDLE_DOMAIN_IRQ is not mandatory at all - a number of architectures
>> had something open-coded in the past (with some drawbacks and/or bugs),
>> and this config option is just one of the ways to get it right.
>>
>> MIPS/PPC perform the reverse lookup directly, without using this
>> infrastructure, and that would be a valid approach for ARC too.
> 
> I'd rather use the generic infrastructure and improve it if needed !
> 
>>>> Marc, I hope you are back from holidays. When u get a chance could you please
>>>> respond to above.
>>>>
>>>> Also I tool a stab at it anyways.
>>>> 1. Enabled HANDLE_DOMAIN_IRQ
>>>> 2. arch_do_IRQ() calls handle_domain_irq(NULL, hwirq, regs) since that code
>>>> doesn't know about domains.
>>>>
>>>> It fails w/o irq_set_default_host() being called.
>> Well, that's expected. unless you use the underlying primitive
>> (__handle_domain_irq) and pass false as the parameter for lookup (see
>> handle_IRQ() in arch/arm/kernel/irq.c). This is what we use for "legacy"
>> platforms that do not support domains.
> 
> Right I saw that and that causes virq = hwirq - kind of defeats the purpose if we
> were doing this on ARC.

You can forget about this if you convert all your platforms to using
domains. But if you have to deal with a transition period (which, in the
ARM case, will last exactly forever because some platforms will never be
converted to DT), this comes in handy.

>>
>>>> If we go back to sunxi example you quoted above, it relies on driver passing the
>>>> domain to handle_domain_irq(). IMHO it is simpler if we had the default domain.
>>>>
>>>> So long story short, ARC can be made to use handle_domain_irq() w/o the song and
>>>> dance of registering another callback from irqchip code if we retained the default
>>>> domain setting.
>> This only works if you can guarantee that you will never have another
>> irqchip calling irq_set_default_host()... If your system is always
>> simple enough to guarantee that, why not.
> 
> ATM we can certainly assume that.
> 
> However with ARM approach two irqchips can still call set_handle_irq() and only
> the first one succeeds (and others return silently). That seems wrong to me -
> irq_xxx.c will still use the handler registered by say irq_sun41.c ?

Oh, this is by no mean foolproof. We do rely on irqchips installing
their primary handler only if they are truly the primary irqchip (they
do not have a parent). Anything else will fail badly.

So if you're confident that you can ensure that noone will set de
default domain twice, you'll be fine.

Thanks,

	M.
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/interrupt-controller/ezchip,nps400-ic.txt b/Documentation/devicetree/bindings/interrupt-controller/ezchip,nps400-ic.txt
new file mode 100644
index 0000000..888b2b9
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/ezchip,nps400-ic.txt
@@ -0,0 +1,17 @@ 
+EZchip NPS Interrupt Controller
+
+Required properties:
+
+- compatible : should be "ezchip,nps400-ic"
+- interrupt-controller : Identifies the node as an interrupt controller
+- #interrupt-cells : Specifies the number of cells needed to encode an
+  interrupt source. The value shall be 1.
+
+
+Example:
+
+intc: interrupt-controller {
+	compatible = "ezchip,nps400-ic";
+	interrupt-controller;
+	#interrupt-cells = <1>;
+};
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 177f78f..b95b954 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -55,3 +55,4 @@  obj-$(CONFIG_RENESAS_H8S_INTC)		+= irq-renesas-h8s.o
 obj-$(CONFIG_ARCH_SA1100)		+= irq-sa11x0.o
 obj-$(CONFIG_INGENIC_IRQ)		+= irq-ingenic.o
 obj-$(CONFIG_IMX_GPCV2)			+= irq-imx-gpcv2.o
+obj-$(CONFIG_ARC_PLAT_EZNPS)		+= irq-eznps.o
diff --git a/drivers/irqchip/irq-eznps.c b/drivers/irqchip/irq-eznps.c
new file mode 100644
index 0000000..fc6d59a
--- /dev/null
+++ b/drivers/irqchip/irq-eznps.c
@@ -0,0 +1,131 @@ 
+/*
+ * Copyright(c) 2015 EZchip Technologies.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * The full GNU General Public License is included in this distribution in
+ * the file called "COPYING".
+ */
+
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/irqdomain.h>
+#include <linux/irqchip.h>
+#include <asm/irq.h>
+
+/*
+ * NPS400 core includes a Interrupt Controller (IC) support.
+ * All cores can deactivate level irqs at first level control
+ * at cores mesh layer called MTM.
+ * For devices out side chip e.g. uart, network there is another
+ * level called Global Interrupt Manager (GIM).
+ * This second level can control level and edge interrupt.
+ *
+ * NOTE: AUX_IENABLE and CTOP_AUX_IACK are auxiliary registers
+ * with private HW copy per CPU.
+ */
+
+static void nps400_irq_mask(struct irq_data *data)
+{
+	unsigned int ienb;
+
+	ienb = read_aux_reg(AUX_IENABLE);
+	ienb &= ~(1 << data->hwirq);
+	write_aux_reg(AUX_IENABLE, ienb);
+}
+
+static void nps400_irq_unmask(struct irq_data *data)
+{
+	unsigned int ienb;
+
+	ienb = read_aux_reg(AUX_IENABLE);
+	ienb |= (1 << data->hwirq);
+	write_aux_reg(AUX_IENABLE, ienb);
+}
+
+static void nps400_irq_eoi_global(struct irq_data *data)
+{
+	write_aux_reg(CTOP_AUX_IACK, 1 << data->hwirq);
+
+	/* Don't ack before all device access is done */
+	mb();
+
+	__asm__ __volatile__ (
+	"       .word %0\n"
+	:
+	: "i"(CTOP_INST_RSPI_GIC_0_R12)
+	: "memory");
+}
+
+static void nps400_irq_eoi(struct irq_data *data)
+{
+	write_aux_reg(CTOP_AUX_IACK, 1 << data->hwirq);
+}
+
+static struct irq_chip nps400_irq_chip_fasteoi = {
+	.name		= "NPS400 IC Global",
+	.irq_mask	= nps400_irq_mask,
+	.irq_unmask	= nps400_irq_unmask,
+	.irq_eoi	= nps400_irq_eoi_global,
+};
+
+static struct irq_chip nps400_irq_chip_percpu = {
+	.name		= "NPS400 IC",
+	.irq_mask	= nps400_irq_mask,
+	.irq_unmask	= nps400_irq_unmask,
+	.irq_eoi	= nps400_irq_eoi,
+};
+
+static int nps400_irq_map(struct irq_domain *d, unsigned int virq,
+			  irq_hw_number_t hw)
+{
+	switch (hw) {
+	case TIMER0_IRQ:
+#if defined(CONFIG_SMP)
+	case IPI_IRQ:
+#endif
+		irq_set_chip_and_handler(virq, &nps400_irq_chip_percpu,
+					 handle_percpu_irq);
+	break;
+	default:
+		irq_set_chip_and_handler(virq, &nps400_irq_chip_fasteoi,
+					 handle_fasteoi_irq);
+	break;
+	}
+
+	return 0;
+}
+
+static const struct irq_domain_ops nps400_irq_ops = {
+	.xlate = irq_domain_xlate_onecell,
+	.map = nps400_irq_map,
+};
+
+static struct irq_domain *nps400_root_domain;
+
+static int __init nps400_of_init(struct device_node *node,
+				 struct device_node *parent)
+{
+	if (parent)
+		panic("DeviceTree incore ic not a root irq controller\n");
+
+	nps400_root_domain = irq_domain_add_linear(node, NR_CPU_IRQS,
+						   &nps400_irq_ops, NULL);
+
+	if (!nps400_root_domain)
+		panic("nps400 root irq domain not avail\n");
+
+	/* with this we don't need to export nps400_root_domain */
+	irq_set_default_host(nps400_root_domain);
+
+	return 0;
+}
+IRQCHIP_DECLARE(ezchip_nps400_ic, "ezchip,nps400-ic", nps400_of_init);