irqchip: add support for SMP irq router

This adds support for a second-gen irq router/controller present on some Sigma Designs chips.

Signed-off-by Sebastian Frias <[email protected]>

This is a RFC because I have a few doubts:
1) I had to unroll irq_of_parse_and_map() in order to get the HW IRQ declared in the device tree so that I can associate it with a given domain.

2) I'm not sure about the DT specification, in particular, the use of child nodes to declare different domains, but it works

3) I'm calling this an irq router to somehow highlight the fact that it is not a simple interrupt controller. Indeed it does not latch the IRQ lines by itself, and does not supports edge detection.

4) Do I have to do something more to handle the affinity stuff?

5) checkpatch.pl reports warnings, etc. but I guess it's ok for now since it is a RFC :-)

Please feel free to comment and suggest improvements.

 .../sigma,smp87xx-irqrouter.txt                    |  69 +++

 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-tango_v2.c                     | 594 +++++++++++++++++++++

drivers/irqchip/irq-tango_v2.c new file +33 -0

+static void tango_irqchip_mask_irq(struct irq_data *data)
+{
+	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
+	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
+	struct tango_irqrouter *irqrouter = irqrouter_output->context;
+	u32 hwirq = data->hwirq;
+	u32 offset = IRQ_TO_OFFSET(hwirq);
+	u32 value = tango_readl(irqrouter, offset);
+	u32 hwirq_reg_index = hwirq / 32;
+	u32 hwirq_bit_index = hwirq % 32;
+
+	DBGLOG("%s: mask hwirq(in) %d : current regvalue 0x%x (routed to hwirq(out) %d)\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)),
+	       hwirq, value, irqrouter_output->hwirq);
+
+	//mask cache
+	irqrouter->irq_mask[hwirq_reg_index] &= ~(1 << hwirq_bit_index);
+
+	value &= ~(IRQ_ROUTER_ENABLE_MASK);
+	tango_writel(irqrouter, offset, value);
+}
+
+static void tango_irqchip_unmask_irq(struct irq_data *data)
+{
+	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
+	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
+	struct tango_irqrouter *irqrouter = irqrouter_output->context;
+	u32 hwirq = data->hwirq;
+	u32 offset = IRQ_TO_OFFSET(hwirq);
+	u32 value = tango_readl(irqrouter, offset);
+	u32 hwirq_reg_index = hwirq / 32;
+	u32 hwirq_bit_index = hwirq % 32;
+

drivers/irqchip/irq-tango_v2.c new file +21 -0

+static int __init tango_of_irq_init(struct device_node *node,
+				    struct device_node *parent)
+{
+	struct tango_irqrouter *irqrouter;
+	struct device_node *child;
+	void __iomem *base;
+	u32 input_count, output_count, i;
+
+	base = of_iomap(node, 0);
+	if (!base) {
+		DBGERR("%s: failed to map combiner registers\n", node->name);
+		return -ENXIO;
+	}
+
+	of_property_read_u32(node, "inputs", &input_count);
+	if (!input_count) {
+		DBGERR("%s: missing 'inputs' property\n", node->name);
+		return -EINVAL;
+	}
+
+	of_property_read_u32(node, "outputs", &output_count);

In the patch subject, do you mean SMP as in Symmetric Multi Processor?

Is that the address you intend to submit with?

The "core" topic is over my head, so I'll just discuss the color of the bike shed.

Quoted 2 lines

 .../sigma,smp87xx-irqrouter.txt                    |  69 +++

In the *actual* submission, we can't use a wildcard like smp87xx we'll have to use an actual part number.

Quoted 3 lines

 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-tango_v2.c                     | 594 +++++++++++++++++++++

Likewise, I don't like the "_v2" suffix, it's too generic. Actual submission should use something more specific.

Documentation/devicetree/bindings/interrupt-controller/sigma,smp87xx-irqrouter.txt line 6

Quoted 9 lines

+++ b/Documentation/devicetree/bindings/interrupt-controller/sigma,smp87xx-irqrouter.txt

Documentation/devicetree/bindings/interrupt-controller/sigma,smp87xx-irqrouter.txt new file +6 -0

@@ -0,0 +1,69 @@
+* Sigma Designs Interrupt Router
+
+This module can route N IRQ inputs into M IRQ outputs, with N>M.
+For instance N=128, M=24.
+
+Note however that the HW does not latches the IRQ lines, so devices

                                     ^^^^^^^

"does not latch"

Also, you hard-code the fact that N/32 = 4 with the status_i variables.

Would it make sense to use for loops?
(instead of unrolling the loops)

e.g. for (i = 0; i < N/4; ++i) { ... }

Documentation/devicetree/bindings/interrupt-controller/sigma,smp87xx-irqrouter.txt line 36

Quoted 4 lines

Documentation/devicetree/bindings/interrupt-controller/sigma,smp87xx-irqrouter.txt new file +3 -0

+Optional properties:
+- interrupt-parent: pHandle of the parent interrupt controller, if not
+  inherited from the parent node.

I'm not sure this is what "optional" means.

From what I've seen in the documentation for other DT bindings, this is the meaning of "optional".

Documentation/devicetree/bindings/interrupt-controller/sigma,smp87xx-irqrouter.txt line 47

Quoted 5 lines

Documentation/devicetree/bindings/interrupt-controller/sigma,smp87xx-irqrouter.txt new file +4 -0

+The following example declares a irqrouter with 128 inputs and 24 outputs,
+with registers @ 0x6F800 and connected to the GIC.
+The two child nodes define two irqdomains, one connected to GIC input 2
+(hwirq=2, level=high), and ther other connected to GIC input 3 (hwirq=3,

                              ^^^^

drivers/irqchip/irq-tango_v2.c line 92

Quoted 2 lines

drivers/irqchip/irq-tango_v2.c new file +2 -0

+#define ROUTER_INPUTS  (128)
+#define ROUTER_OUTPUTS (24)

Parentheses are unnecessary around constants.

drivers/irqchip/irq-tango_v2.c line 95

Quoted 3 lines

drivers/irqchip/irq-tango_v2.c new file +2 -0

+#define IRQ_ROUTER_ENABLE_MASK (BIT(31))
+#define IRQ_ROUTER_INVERT_MASK (BIT(16))

Parentheses already provided in BIT macro.

drivers/irqchip/irq-tango_v2.c line 100

Quoted 5 lines

drivers/irqchip/irq-tango_v2.c new file +4 -0

+#define READ_SYS_IRQ_GROUP0 (0x420)
+#define READ_SYS_IRQ_GROUP1 (0x424)
+#define READ_SYS_IRQ_GROUP2 (0x428)
+#define READ_SYS_IRQ_GROUP3 (0x42C)

If a for loop were used, we'd only need to
#define SYSTEM_IRQ 0x420

  for (i = 0; i < N/4; ++i) {
    status_i = readl(base + SYSTEM_IRQ + i*4);

drivers/irqchip/irq-tango_v2.c line 107

Quoted 5 lines

drivers/irqchip/irq-tango_v2.c new file +5 -0

+#if 0
+#define SHORT_OR_FULL_NAME full_name
+#else
+#define SHORT_OR_FULL_NAME name
+#endif

Just pick one?

drivers/irqchip/irq-tango_v2.c line 109

Quoted 2 lines

drivers/irqchip/irq-tango_v2.c new file +1 -0

+#define NODE_NAME(__node__) (__node__ ? __node__->SHORT_OR_FULL_NAME : "<no-node>")

Is it possible for a node to not have a name?

I also think prefixing/postfixing macro parameters with underscores is positively fugly.

From my point of view, it makes a clear difference between macro parameters and rest of the code.

drivers/irqchip/irq-tango_v2.c line 133

Quoted 5 lines

drivers/irqchip/irq-tango_v2.c new file +5 -0

+/*
+  context for the driver
+*/
+struct tango_irqrouter {
+	raw_spinlock_t lock;

Is this lock really needed?

Is tango_irqdomain_handle_cascade_irq() expected to be called concurrently on multiple cores?

Even then, is it necessary to lock, in order to read 4 MMIO regs?

drivers/irqchip/irq-tango_v2.c line 141

Quoted 8 lines

drivers/irqchip/irq-tango_v2.c new file +8 -0

+	struct device_node *node;
+	void __iomem *base;
+	u32 input_count;
+	u32 output_count;
+	u32 irq_mask[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
+	u32 irq_invert_mask[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
+	struct tango_irqrouter_output output[ROUTER_OUTPUTS];
+};

Hmmm, if the driver were truly "parameterizable", I guess we should dynamically allocate the arrays.

Quoted 16 lines

drivers/irqchip/irq-tango_v2.c new file +16 -0

+static void tango_irqchip_unmask_irq(struct irq_data *data)
+{
+	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
+	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
+	struct tango_irqrouter *irqrouter = irqrouter_output->context;
+	u32 hwirq = data->hwirq;
+	u32 offset = IRQ_TO_OFFSET(hwirq);
+	u32 value = tango_readl(irqrouter, offset);
+	u32 hwirq_reg_index = hwirq / 32;
+	u32 hwirq_bit_index = hwirq % 32;
+
+	DBGLOG("%s: unmask hwirq(in) %d : current regvalue 0x%x (routed to hwirq(out) %d)\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)),
+	       hwirq, value, irqrouter_output->hwirq);
+
+	//unmask cache

drivers/irqchip/irq-tango_v2.c line 234

Quoted 5 lines

drivers/irqchip/irq-tango_v2.c new file +5 -0

+	irqrouter->irq_mask[hwirq_reg_index] |= (1 << hwirq_bit_index);
+
+	value |= IRQ_ROUTER_ENABLE_MASK;
+	tango_writel(irqrouter, offset, value);
+}

There might be an opportunity to factorize the two functions, and have mask/unmask call such "helper" with the proper args.

drivers/irqchip/irq-tango_v2.c line 564

Quoted 6 lines

drivers/irqchip/irq-tango_v2.c new file +6 -0

+
+	if ((input_count != ROUTER_INPUTS)
+	    || (output_count != ROUTER_OUTPUTS)) {
+		DBGERR("%s: input/output count mismatch", node->name);
+		return -EINVAL;
+	}

So the driver is not intended to be parameterized? (or perhaps only in a follow-up?)

On 07/04/2016 02:11 PM, Mason wrote:

Quoted 4 lines

Quoted 1 line

 .../sigma,smp87xx-irqrouter.txt                    |  69 +++

In the *actual* submission, we can't use a wildcard like smp87xx we'll have to use an actual part number.

Are you sure?
That would hinder genericity.
Actually I wanted to call it "sigma,smp-irqrouter.txt" (or "sigma,smp,irqrouter.txt").

To me there's no need to link the compatible string of a given HW module with that of the chip name the module it is embedded into. For example, the generic USB3 driver is "generic-xhci". While this module is not generic to be embedded in chips from different manufacturers, it is supposed to be generic within Sigma, and multiple Sigma chips (with potentially different denominations) can use it.

Quoted 1 line

Quoted 5 lines

Quoted 2 lines

 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-tango_v2.c                     | 594 +++++++++++++++++++++

Likewise, I don't like the "_v2" suffix, it's too generic. Actual submission should use something more specific.

Well, the other driver is irq-tango.c that is generic as well. I prefer versioning, as it is unrelated with the actual chip name.

Quoted 9 lines · Sebastian Frias

Quoted 6 lines · Mason

Quoted 1 line · Sebastian Frias

 .../sigma,smp87xx-irqrouter.txt                    |  69 +++

In the *actual* submission, we can't use a wildcard like smp87xx we'll have to use an actual part number.

Quoted 3 lines

Are you sure?
That would hinder genericity.
Actually I wanted to call it "sigma,smp-irqrouter.txt" (or "sigma,smp,irqrouter.txt").

sigma,smp-irqrouter.txt should be fine. The devicetree maintainers should yelp if they want something different.

Quoted 14 lines

To me there's no need to link the compatible string of a given HW module with that of the chip name the module it is embedded into. For example, the generic USB3 driver is "generic-xhci". While this module is not generic to be embedded in chips from different manufacturers, it is supposed to be generic within Sigma, and multiple Sigma chips (with potentially different denominations) can use it.

Quoted 6 lines

Quoted 3 lines

 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-tango_v2.c                     | 594 +++++++++++++++++++++

Likewise, I don't like the "_v2" suffix, it's too generic. Actual submission should use something more specific.

Quoted 2 lines

Well, the other driver is irq-tango.c that is generic as well. I prefer versioning, as it is unrelated with the actual chip name.

Is there a name, similar to 'tango', for this version of the IP? Something that would spark recognition for someone looking for "the damn driver for this XYZ irqchip I have". If not, irq-tango_v2.c is fine.

Quoted 13 lines · Jason Cooper

Quoted 11 lines · Sebastian Frias

Quoted 6 lines · Mason

Quoted 1 line · Sebastian Frias

 .../sigma,smp87xx-irqrouter.txt                    |  69 +++

In the *actual* submission, we can't use a wildcard like smp87xx we'll have to use an actual part number.

Are you sure?
That would hinder genericity.
Actually I wanted to call it "sigma,smp-irqrouter.txt" (or "sigma,smp,irqrouter.txt").

Quoted 2 lines

sigma,smp-irqrouter.txt should be fine. The devicetree maintainers should yelp if they want something different.

Personally, I don't like "smp" because it's too easy to confuse that for "symmetric multi-processor".

Come to think of it, I'm not sure the *name* of the file documenting a binding is as important to DT maintainers as the compatible string.

Quoted 2 lines

Come to think of it, I'm not sure the *name* of the file documenting a binding is as important to DT maintainers as the compatible string.

Correct. devicetee compatible strings need to be as specific as possible.

In a series of compatible IP blocks, the string should refer to the first version in the series, e.g. sigma,smp8710 for a series of compatible IP blocks like 8710, 8712, 8715, 8724. If an 8751 came along with a different register layout or some other incompatibility, then a new string would be sigma,smp8751. So,

8710 uses "sigma,smp8710"
8712 uses "sigma,smp8710"
8715 uses "sigma,smp8710"
8724 uses "sigma,smp8710"
8751 uses "sigma,smp8751"
8754 uses "sigma,smp8751"

Quoted 5 lines · Jason Cooper

Quoted 2 lines

Come to think of it, I'm not sure the *name* of the file documenting a binding is as important to DT maintainers as the compatible string.

Correct. devicetee compatible strings need to be as specific as possible.

Specific with respect to what thing? To the HW module they are describing (USB, IRQ controller, etc.) or to the chip the HW module it is embedded into?

Quoted 6 lines

In a series of compatible IP blocks, the string should refer to the first version in the series, e.g. sigma,smp8710 for a series of compatible IP blocks like 8710, 8712, 8715, 8724. If an 8751 came along with a different register layout or some other incompatibility, then a new string would be sigma,smp8751. So,

Quoted 7 lines

8710 uses "sigma,smp8710"
8712 uses "sigma,smp8710"
8715 uses "sigma,smp8710"
8724 uses "sigma,smp8710"
8751 uses "sigma,smp8751"
8754 uses "sigma,smp8751"

But this is not consistent with the strings for generic drivers, like "generic-xhci", etc.

A SoC is composed of several HW modules, some are shared among different manufacturers (i.e.: "generic-xhci"), and some are shared among different product lines of the same manufacturer (i.e.: "sigma,smp,irqrouter").

And the DT for a given chip should describe the collection of HW modules that make up the SoC, regardless of what chip introduced them or what other chip uses it. Why would that be relevant anyway?

By that reasoning, I also think that drivers/net/ethernet/aurora/nb8800.c should have had a string like "aurora,nb8800,sigma" or something, to specify that it is a NB8800 from Aurora integrated in a Sigma platform.

That way it can behave as vanilla NB8800 when the string is "aurora,nb8800" and it can adapt its behaviour to Sigma's platform when a different string (like "aurora,nb8800,sigma") is used in the DT.

Later if Sigma modified the nb8800 HW, it could be "aurora,nb8800,sigma-v2". The same way, if later Sigma used a different Aurora HW module, the DT would say "aurora,nb2000,sigma" (to signal that another driver is required)

Instead, drivers/net/ethernet/aurora/nb8800.c has "sigma,smp8642-ethernet" string, which ties it to a particular chip, and I don't see how does that convey version about the module or other relevant information; Plus it is confusing if the same module is then embedded on a SMP8756 chip.

Would you mind explaining how does that contrasts with the logic used by DT naming conventions?

Hi Sebastian,

Quoted 7 lines · Sebastian Frias

Quoted 5 lines · Jason Cooper

Quoted 2 lines

Come to think of it, I'm not sure the *name* of the file documenting a binding is as important to DT maintainers as the compatible string.

Correct. devicetee compatible strings need to be as specific as possible.

Quoted 3 lines

Specific with respect to what thing? To the HW module they are describing (USB, IRQ controller, etc.) or to the chip the HW module it is embedded into?

The compatible string uniquely identifies an interface between an IP block and the software (the devicetree binding). We use the most specific model number or name we can for that IP block when we create the binding and the compatible string.

If future SoCs come out and the IP block contained within, regardless of identifier, is compatible with the existing binding, then we can reuse it. This is what I was trying to show with my little chart quoted below.

For ethernet and other major blocks it's easy because they have their own model numbers and such. For the smaller blocks, like irqchips, we have to use the model number or unique name of the SoC we first found it in.

Quoted 3 lines

A SoC is composed of several HW modules, some are shared among different manufacturers (i.e.: "generic-xhci"), and some are shared among different product lines of the same manufacturer (i.e.: "sigma,smp,irqrouter").

Yes, that's why it's critical to be specific. We want to reuse drivers where it makes sense. We can if the interface is the same.

Quoted 4 lines

And the DT for a given chip should describe the collection of HW modules that make up the SoC, regardless of what chip introduced them or what other chip uses it. Why would that be relevant anyway?

Because the first time we see a new IP block, we can't predict the future. We don't know where it's going to be reused. We don't know when it's going to be changed, or how. So we mark it as specifically as we can based on the information we have when we first encounter it.

When a new version comes out, we see if the interface is still compatible. If it is, there's nothing to do. If it changed, we see if we can address it by adding a property to the existing binding. Failing that, we create a new compatible string to indicate a new interface.

Quoted 4 lines

By that reasoning, I also think that drivers/net/ethernet/aurora/nb8800.c should have had a string like "aurora,nb8800,sigma" or something, to specify that it is a NB8800 from Aurora integrated in a Sigma platform.

The fact that it is in the Sigma dts file tells this already. Based on the above, did the interface change when adding it to the sigma platform? Can the binding be reused?

Quoted 4 lines

That way it can behave as vanilla NB8800 when the string is "aurora,nb8800" and it can adapt its behaviour to Sigma's platform when a different string (like "aurora,nb8800,sigma") is used in the DT.

I would have to look at the specifics of "adapt its behaviour" in order to determine wether a new compatible is warranted, or maybe just add a property to the binding.

Quoted 4 lines

Later if Sigma modified the nb8800 HW, it could be "aurora,nb8800,sigma-v2". The same way, if later Sigma used a different Aurora HW module, the DT would say "aurora,nb2000,sigma" (to signal that another driver is required)

The compatible string is important, but we try not to overload it. There are many flexible ways we can indicate properties or quirks in the binding.

Quoted 5 lines

Instead, drivers/net/ethernet/aurora/nb8800.c has "sigma,smp8642-ethernet" string, which ties it to a particular chip, and I don't see how does that convey version about the module or other relevant information; Plus it is confusing if the same module is then embedded on a SMP8756 chip.

Sorry, it's just understood that the compatible string means they have the same binding/interface.

Quoted 3 lines

Would you mind explaining how does that contrasts with the logic used by DT naming conventions?

I think you may be looking at DT the wrong way :-P It's not an inventory of components for the end user to look at. It a list of hardware interfaces and how they're attached; for the OS to interpret and use.

Quoted 12 lines · Jason Cooper

Quoted 10 lines · Sebastian Frias

Quoted 5 lines · Jason Cooper

Quoted 2 lines

Come to think of it, I'm not sure the *name* of the file documenting a binding is as important to DT maintainers as the compatible string.

Correct. devicetee compatible strings need to be as specific as possible.

Specific with respect to what thing? To the HW module they are describing (USB, IRQ controller, etc.) or to the chip the HW module it is embedded into?

Quoted 4 lines

The compatible string uniquely identifies an interface between an IP block and the software (the devicetree binding). We use the most specific model number or name we can for that IP block when we create the binding and the compatible string.

Ok, so we agree that the string identifies the HW block itself, and not the chip in which it is embedded into.

Quoted 6 lines

Quoted 3 lines

A SoC is composed of several HW modules, some are shared among different manufacturers (i.e.: "generic-xhci"), and some are shared among different product lines of the same manufacturer (i.e.: "sigma,smp,irqrouter").

Yes, that's why it's critical to be specific. We want to reuse drivers where it makes sense. We can if the interface is the same.

Ok, so is "sigma,smp,irqrouter" good or not?

Hey Sebastian, Mason,

* Please fix mailer to wrap text at a sane length. I've re-wrapped and trimmed.

Quoted 1 line · Sebastian Frias

On 07/04/2016 02:11 PM, Mason wrote:

...

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

 .../sigma,smp87xx-irqrouter.txt                    |  69 +++

In the *actual* submission, we can't use a wildcard like smp87xx we'll have to use an actual part number.

Are you sure?
That would hinder genericity.
Actually I wanted to call it "sigma,smp-irqrouter.txt" (or "sigma,smp,irqrouter.txt").

sigma,smp-irqrouter.txt should be fine. The devicetree maintainers should yelp if they want something different.

Quoted 14 lines

To me there's no need to link the compatible string of a given HW module with that of the chip name the module it is embedded into. For example, the generic USB3 driver is "generic-xhci". While this module is not generic to be embedded in chips from different manufacturers, it is supposed to be generic within Sigma, and multiple Sigma chips (with potentially different denominations) can use it.

Quoted 6 lines

Quoted 3 lines

 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-tango_v2.c                     | 594 +++++++++++++++++++++

Likewise, I don't like the "_v2" suffix, it's too generic. Actual submission should use something more specific.

Quoted 2 lines

Well, the other driver is irq-tango.c that is generic as well. I prefer versioning, as it is unrelated with the actual chip name.

Is there a name, similar to 'tango', for this version of the IP? Something that would spark recognition for someone looking for "the damn driver for this XYZ irqchip I have". If not, irq-tango_v2.c is fine.

Quoted 32 lines · Jason Cooper

Quoted 1 line · Sebastian Frias

On 07/04/2016 02:11 PM, Mason wrote:

...

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

 .../sigma,smp87xx-irqrouter.txt                    |  69 +++

In the *actual* submission, we can't use a wildcard like smp87xx we'll have to use an actual part number.

Are you sure?
That would hinder genericity.
Actually I wanted to call it "sigma,smp-irqrouter.txt" (or "sigma,smp,irqrouter.txt").

sigma,smp-irqrouter.txt should be fine. The devicetree maintainers should yelp if they want something different.

Quoted 16 lines

To me there's no need to link the compatible string of a given HW module with that of the chip name the module it is embedded into. For example, the generic USB3 driver is "generic-xhci". While this module is not generic to be embedded in chips from different manufacturers, it is supposed to be generic within Sigma, and multiple Sigma chips (with potentially different denominations) can use it.

Quoted 6 lines

Quoted 3 lines

 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-tango_v2.c                     | 594 +++++++++++++++++++++

Likewise, I don't like the "_v2" suffix, it's too generic. Actual submission should use something more specific.

Well, the other driver is irq-tango.c that is generic as well. I prefer versioning, as it is unrelated with the actual chip name.

Quoted 4 lines

Is there a name, similar to 'tango', for this version of the IP? Something that would spark recognition for someone looking for "the damn driver for this XYZ irqchip I have". If not, irq-tango_v2.c is fine.

Thanks for your comments.
So, aside from some naming issues, do you think the driver is ok?

Quoted 3 lines

Thanks for your comments.
So, aside from some naming issues, do you think the driver is ok?

Well, it's going to be few days before I can really dig in to this. Until then, what I can say I see is that it looks like you're using devicetree to tell Linux how to lay out the irq domains. That's not right :(

The devicetree should *only* describe the hardware. Would *BSD be able to use the description in the dtb effectively?

iiuc, I think irq-crossbar.c may be a similar enough in task to give you an idea or two.

Hi Jason,

Quoted 8 lines · Jason Cooper

Quoted 3 lines

Thanks for your comments.
So, aside from some naming issues, do you think the driver is ok?

Well, it's going to be few days before I can really dig in to this. Until then, what I can say I see is that it looks like you're using devicetree to tell Linux how to lay out the irq domains. That's not right :(

Ok, so that replies my questions 1 and 2, thanks.

Quoted 6 lines

The devicetree should *only* describe the hardware. Would *BSD be able to use the description in the dtb effectively?

iiuc, I think irq-crossbar.c may be a similar enough in task to give you an idea or two.

I already did something like that, you can see it here:

https://marc.info/?l=linux-kernel&m=146592235919308&w=2

the problem with that code is that it cannot handle more than 24 IRQs (the number of outputs of the router), because they are not being shared.

Maybe I need a sort of hybrid approach by reintroducing part of "irq-crossbar.c" code to replace the irq domain layout that is currently being done using DT properties ?

However, I have not seen any examples of how to describe, using the DT, an association between a device HW irq, and the GIC hwirq where it goes to, nor how to express in the DT that multiple devices should share a given GIC hwirq.
Basically, when a device requests the IRQ specified in its DT, I need:
- to know which GIC hwirq line should I route it to (or the GIC to tell me which one it expects)

Quoted 11 lines

I already did something like that, you can see it here:

https://marc.info/?l=linux-kernel&m=146592235919308&w=2

the problem with that code is that it cannot handle more than 24 IRQs (the number of outputs of the router), because they are not being shared.

Maybe I need a sort of hybrid approach by reintroducing part of "irq-crossbar.c" code to replace the irq domain layout that is currently being done using DT properties ?

Quoted 7 lines

However, I have not seen any examples of how to describe, using the DT, an association between a device HW irq, and the GIC hwirq where it goes to, nor how to express in the DT that multiple devices should share a given GIC hwirq.
Basically, when a device requests the IRQ specified in its DT, I need:
- to know which GIC hwirq line should I route it to (or the GIC to tell me which one it expects)

You really don't need to describe this. The configuration that is applied to your router in entirely under software control,

and none of
that should appear in the DT. You could decide to mux all the interrupts to a single one, or decide that the 23 first interrupts you discover get their own private line to the GIC and that everything else is muxed.

So given that this is completely defined by software, it has no place in DT.

There may not be an example of such an interrupt controller in the tree, but this doesn't look too hard to implement.

Hi Marc,

Quoted 19 lines · Marc Zyngier

Quoted 18 lines

I already did something like that, you can see it here:

https://marc.info/?l=linux-kernel&m=146592235919308&w=2

the problem with that code is that it cannot handle more than 24 IRQs (the number of outputs of the router), because they are not being shared.

Maybe I need a sort of hybrid approach by reintroducing part of "irq-crossbar.c" code to replace the irq domain layout that is currently being done using DT properties ?

However, I have not seen any examples of how to describe, using the DT, an association between a device HW irq, and the GIC hwirq where it goes to, nor how to express in the DT that multiple devices should share a given GIC hwirq.
Basically, when a device requests the IRQ specified in its DT, I need:
- to know which GIC hwirq line should I route it to (or the GIC to tell me which one it expects)

Quoted 2 lines

You really don't need to describe this. The configuration that is applied to your router in entirely under software control,

With "entirely under software control" do you mean this driver's code?

Quoted 6 lines

and none of
that should appear in the DT. You could decide to mux all the interrupts to a single one, or decide that the 23 first interrupts you discover get their own private line to the GIC and that everything else is muxed.

Quoted 2 lines

So given that this is completely defined by software, it has no place in DT.

I think I'm missing something, what is the difference between the domains described by nodes in the DT for irq-tango.c (arch/arm/boot/dts/tango4-common.dtsi) and the DT from my RFC?

Alternatively, the previous DT (arch/arm/boot/dts/tango4-common.dtsi) allowed IRQ sharing to be specified in the DT, is that wrong?

Quoted 2 lines

There may not be an example of such an interrupt controller in the tree, but this doesn't look too hard to implement.

Well, if you the domains should not be described in the DT and that they should be somehow hardcoded into the drivers' code, it should not be hard indeed.

Quoted 2 lines · Sebastian Frias

Well, if you the domains should not be described in the DT and that they should be somehow hardcoded into the drivers' code, it should not be hard indeed.

Hardcoded? No way. You simply implement a route allocator in your driver, assigning them as needed. And yes, if you have more than 24 interrupts, they get muxed.

Quoted 4 lines · Marc Zyngier

Quoted 2 lines · Sebastian Frias

Well, if you the domains should not be described in the DT and that they should be somehow hardcoded into the drivers' code, it should not be hard indeed.

Quoted 3 lines

Hardcoded? No way. You simply implement a route allocator in your driver, assigning them as needed. And yes, if you have more than 24 interrupts, they get muxed.

There is one caveat though. Under some circumstances (think RT) you want to configure which interrupts get muxed and which not. We really should have that option, but yes for anything which has less than 24 autorouting is the way to go.

Quoted 9 lines · Thomas Gleixner

Quoted 3 lines · Marc Zyngier

Hardcoded? No way. You simply implement a route allocator in your driver, assigning them as needed. And yes, if you have more than 24 interrupts, they get muxed.

There is one caveat though. Under some circumstances (think RT) you want to configure which interrupts get muxed and which not. We really should have that option, but yes for anything which has less than 24 autorouting is the way to go.

Good point. I can see two possibilities for that:

- either we describe this DT with some form of hint, indicating what are the inputs that can be muxed to a single output. Easy, but the DT guys are going to throw rocks at me for being Linux-specific.

- or we have a way to express QoS in the irq subsystem, and a driver can request an interrupt with a "make it fast" flag. Of course, everybody and his dog are going to ask for it, and we're back to square one.

Do we have a way to detect which interrupt is more likely to be sensitive to muxing? My hunch is that if it is requested with IRQF_SHARED, then it is effectively muxable. Thoughts?

Quoted 11 lines · Marc Zyngier

Quoted 9 lines · Thomas Gleixner

Quoted 3 lines · Marc Zyngier

Hardcoded? No way. You simply implement a route allocator in your driver, assigning them as needed. And yes, if you have more than 24 interrupts, they get muxed.

There is one caveat though. Under some circumstances (think RT) you want to configure which interrupts get muxed and which not. We really should have that option, but yes for anything which has less than 24 autorouting is the way to go.

Quoted 5 lines

Good point. I can see two possibilities for that:

- either we describe this DT with some form of hint, indicating what are the inputs that can be muxed to a single output. Easy, but the DT guys are going to throw rocks at me for being Linux-specific.

That's not necessarily Linux specific. The problem arises with any other OS as well.

Quoted 3 lines

- or we have a way to express QoS in the irq subsystem, and a driver can request an interrupt with a "make it fast" flag. Of course, everybody and his dog are going to ask for it, and we're back to square one.

That and the driver does not know about the particular application scenario/system configuration.

Quoted 3 lines

Do we have a way to detect which interrupt is more likely to be sensitive to muxing? My hunch is that if it is requested with IRQF_SHARED, then it is effectively muxable. Thoughts?

That's too late. request_irq happens _after_ the interrupt is set up and the routing established.

Hi,

Quoted 28 lines · Thomas Gleixner

Quoted 11 lines · Marc Zyngier

Quoted 9 lines · Thomas Gleixner

Quoted 3 lines · Marc Zyngier

Hardcoded? No way. You simply implement a route allocator in your driver, assigning them as needed. And yes, if you have more than 24 interrupts, they get muxed.

There is one caveat though. Under some circumstances (think RT) you want to configure which interrupts get muxed and which not. We really should have that option, but yes for anything which has less than 24 autorouting is the way to go.

Quoted 5 lines

Good point. I can see two possibilities for that:

- either we describe this DT with some form of hint, indicating what are the inputs that can be muxed to a single output. Easy, but the DT guys are going to throw rocks at me for being Linux-specific.

That's not necessarily Linux specific. The problem arises with any other OS as well.

Quoted 3 lines

- or we have a way to express QoS in the irq subsystem, and a driver can request an interrupt with a "make it fast" flag. Of course, everybody and his dog are going to ask for it, and we're back to square one.

That and the driver does not know about the particular application scenario/system configuration.

Quoted 7 lines

Quoted 3 lines

Do we have a way to detect which interrupt is more likely to be sensitive to muxing? My hunch is that if it is requested with IRQF_SHARED, then it is effectively muxable. Thoughts?

That's too late. request_irq happens _after_ the interrupt is set up and the routing established.

What about using 3 values for the interrupt description like the GIC does? When connecting to the GIC we say "interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;" If devices using this driver (the one from the RFC) requested the interrupt like:
"interrupts = <0 38 IRQ_TYPE_LEVEL_HIGH>;"
"interrupts = <2 27 IRQ_TYPE_LEVEL_HIGH>;"
etc.
with the first field being the "group", then the driver could create a domain for the device's IRQ (or associate it to an existing one if it has already been created). It would thus serve as a hint on how to create domains and how to share IRQs into the same line (domain).

I guess I can get such information from the .translate and .alloc callbacks from a newly created domain hierarchy attached to the GIC, right?

Quoted 11 lines · Marc Zyngier

Quoted 9 lines · Thomas Gleixner

Quoted 3 lines · Marc Zyngier

Hardcoded? No way. You simply implement a route allocator in your driver, assigning them as needed. And yes, if you have more than 24 interrupts, they get muxed.

There is one caveat though. Under some circumstances (think RT) you want to configure which interrupts get muxed and which not. We really should have that option, but yes for anything which has less than 24 autorouting is the way to go.

Quoted 5 lines

Good point. I can see two possibilities for that:

- either we describe this DT with some form of hint, indicating what are the inputs that can be muxed to a single output. Easy, but the DT guys are going to throw rocks at me for being Linux-specific.

That's not necessarily Linux specific. The problem arises with any other OS as well.

Quoted 4 lines

Quoted 2 lines

You really don't need to describe this. The configuration that is applied to your router in entirely under software control,

With "entirely under software control" do you mean this driver's code?

Yes.

Quoted 10 lines

Quoted 9 lines

and none of
that should appear in the DT. You could decide to mux all the interrupts to a single one, or decide that the 23 first interrupts you discover get their own private line to the GIC and that everything else is muxed.

So given that this is completely defined by software, it has no place in DT.

Quoted 3 lines

I think I'm missing something, what is the difference between the domains described by nodes in the DT for irq-tango.c (arch/arm/boot/dts/tango4-common.dtsi) and the DT from my RFC?

The fundamental difference is that with your new fancy controller, you can decide what is going where, while the previous one is completely set in stone (the output line is a direct function of the input line).

Quoted 4 lines

Quoted 3 lines

I think I'm missing something, what is the difference between the domains described by nodes in the DT for irq-tango.c (arch/arm/boot/dts/tango4-common.dtsi) and the DT from my RFC?

Quoted 3 lines

The fundamental difference is that with your new fancy controller, you can decide what is going where, while the previous one is completely set in stone (the output line is a direct function of the input line).

I think that's where part the misunderstanding comes from. IMHO the output line is not a direct function of the input line. Any of the 64 IRQ lines entering the "old controller" (irq-tango.c) can be routed to any of its 3 outputs.

The only thing fixed is which GIC input is connected to those 3 outputs, ie:
GIC inputs 2, 3 and 4.

In the the "new controller" (irq-tango_v2.c, this RFC), any of 128 IRQ lines can be routed to any of 24 outputs, connected to GIC inputs 0...23.

In a nutshell:
- "old controller": routes [0...N] => GIC inputs [2...4]
- "new controller": routes [0...M] => GIC inputs [0...23]

So, when we think about it, if the "new DT" specified 24 domains, it would be equivalent of the "old DT" with 3 domains, right?

That's why it seemed more or less natural to keep describing the domains in the DT, the main reason for that being that it allowed the user to specify the IRQ sharing in the DT, and this is precisely the key point of this.

So, putting aside routing considerations and the discussion above, I think a simpler question is: if the domains should not be described in the DT, how can we define the IRQ sharing in the DT?

Quoted 4 lines

I think that's where part the misunderstanding comes from. IMHO the output line is not a direct function of the input line. Any of the 64 IRQ lines entering the "old controller" (irq-tango.c) can be routed to any of its 3 outputs.

Then the current DT binding isn't properly describing the HW.

Quoted 6 lines

In a nutshell:
- "old controller": routes [0...N] => GIC inputs [2...4]
- "new controller": routes [0...M] => GIC inputs [0...23]

So, when we think about it, if the "new DT" specified 24 domains, it would be equivalent of the "old DT" with 3 domains, right?

Indeed, but I consider the "old" binding to be rather misleading. It should have been described as a router too, rather than hardcoding things in DT. Granted, it doesn't matter much when you only have 3 possible output lines. But with 24 outputs, that becomes much more relevant.

Quoted 3 lines

So, putting aside routing considerations and the discussion above, I think a simpler question is: if the domains should not be described in the DT, how can we define the IRQ sharing in the DT?

You could have a set of sub-nodes saying something like this:

	mux-hint0 {
		inputs = <1 45 127>;
	}

	mux-hint1 {
		inputs = <2 33>;
	}

(or maybe you can have that as direct properties, but you get the idea). Here, you have two output pins dedicated to muxed interrupts (assuming they are all level interrupts), and the last 22 can be freely allocated as direct routes.

Hi Marc,

Quoted 6 lines · Marc Zyngier

Quoted 4 lines

I think that's where part the misunderstanding comes from. IMHO the output line is not a direct function of the input line. Any of the 64 IRQ lines entering the "old controller" (irq-tango.c) can be routed to any of its 3 outputs.

Then the current DT binding isn't properly describing the HW.

Ok, thanks, so it is not a good example then.

Quoted 11 lines

Quoted 6 lines

In a nutshell:
- "old controller": routes [0...N] => GIC inputs [2...4]
- "new controller": routes [0...M] => GIC inputs [0...23]

So, when we think about it, if the "new DT" specified 24 domains, it would be equivalent of the "old DT" with 3 domains, right?

Indeed, but I consider the "old" binding to be rather misleading. It should have been described as a router too, rather than hardcoding things in DT. Granted, it doesn't matter much when you only have 3 possible output lines. But with 24 outputs, that becomes much more relevant.

I see.

Quoted 14 lines

Quoted 3 lines

So, putting aside routing considerations and the discussion above, I think a simpler question is: if the domains should not be described in the DT, how can we define the IRQ sharing in the DT?

You could have a set of sub-nodes saying something like this:

	mux-hint0 {
		inputs = <1 45 127>;
	}

	mux-hint1 {
		inputs = <2 33>;
	}

Quoted 5 lines

(or maybe you can have that as direct properties, but you get the idea). Here, you have two output pins dedicated to muxed interrupts (assuming they are all level interrupts), and the last 22 can be freely allocated as direct routes.

Ok, I'll try to do that.
So, aside from the DT issues (that is, that it is describing domains), would it be ok to create a domain for each of the outputs?

Because I was looking at:
- Documentation/devicetree/bindings/interrupt-controller/samsung,exynos4210-combiner.txt
- drivers/irqchip/exynos-combiner.c
- arch/arm/boot/dts/exynos4210.dtsi

and what I see is that the DT basically list all outputs [0...15] connected to the parent interrupt controller, although the driver does not creates separate domains, just one. Then it attaches a chained handler for each of the outputs. On the .map callback it attaches a irqchip to the domain.

There is also:
- Documentation/devicetree/bindings/arm/omap/crossbar.txt
- drivers/irqchip/irq-crossbar.c
- arch/arm/boot/dts/dra7.dtsi

This one creates a domain hierarchy linked to the parent domain and uses irq_domain_alloc_irqs_parent() and irq_domain_set_hwirq_and_chip() to attach a irqchip to the domain on the .alloc callback.

Both use a single domain, as opposed to irq-tango.c which creates 3 domains. Right now irq-tango_v2.c is supposed to create one domain per output (if so the DT says)
Are there guidelines regarding that?

drivers/irqchip/irq-tango_v4.c new file +22 -0

+
+#define BITMASK_VECTOR_SIZE(__count__) (__count__ / 32)
+#define IRQ_TO_OFFSET(__hwirq__) (__hwirq__ * 4)
+
+struct tango_irqrouter;
+
+/*
+ * Maintains the mapping between a Linux virq and a hwirq
+ * on the parent controller.
+ * It is used by tango_irqdomain_map() or tango_irqdomain_hierarchy_alloc()
+ * to setup the route between input IRQ and output IRQ
+ */
+struct tango_irqrouter_output {
+	struct tango_irqrouter *context;
+
+	u32 domain_id;
+
+	u32 hwirq;
+	u32 hwirq_level;
+	u32 virq;
+
+	int shared_count;

drivers/irqchip/irq-tango_v4.c new file +8 -0

+}
+
+
+static inline void tango_writel(struct tango_irqrouter *irqrouter,
+				int reg,
+				u32 val)
+{
+	/*DBGLOG("w[0x%08x + 0x%08x = 0x%08x] = 0x%08x\n",

drivers/irqchip/irq-tango_v4.c new file +41 -0

+		irqrouter->swirq_mask |= (1 << swirq_bit_index);
+		value |= (1 << swirq_bit_index);
+	} else {
+		/* mask swirq */
+		irqrouter->swirq_mask &= ~(1 << swirq_bit_index);
+		value &= ~(1 << swirq_bit_index);
+	}
+
+	tango_writel(irqrouter, offset, value);
+}
+
+
+static inline void tango_set_hwirq_enable(struct tango_irqrouter *irqrouter,
+					  int hwirq,
+					  bool enable)
+{
+	u32 offset = IRQ_TO_OFFSET(hwirq);
+	u32 value = tango_readl(irqrouter, offset);
+	u32 hwirq_reg_index = hwirq / 32;
+	u32 hwirq_bit_index = hwirq % 32;
+	u32 *enable_mask = &(irqrouter->irq_mask[hwirq_reg_index]);
+
+#if 1
+	DBGLOG("%smask hwirq(in) %d : current regvalue 0x%x\n",
+	       enable ? "un":"",
+	       hwirq, value);
+#endif
+
+	if (enable) {
+		/* unmask irq */
+		*enable_mask |= (1 << hwirq_bit_index);
+		value |= IRQ_ROUTER_ENABLE_MASK;
+	} else {
+		/* mask irq */
+		*enable_mask &= ~(1 << hwirq_bit_index);
+		value &= ~(IRQ_ROUTER_ENABLE_MASK);
+	}
+
+	tango_writel(irqrouter, offset, value);
+}
+

drivers/irqchip/irq-tango_v4.c new file +39 -0

+}
+
+
+static inline void tango_set_hwirq_inversion(struct tango_irqrouter *irqrouter,
+					     int hwirq,
+					     bool invert)
+{
+	u32 offset = IRQ_TO_OFFSET(hwirq);
+	u32 value = tango_readl(irqrouter, offset);
+	u32 hwirq_reg_index = hwirq / 32;
+	u32 hwirq_bit_index = hwirq % 32;
+	u32 *invert_mask = &(irqrouter->irq_invert_mask[hwirq_reg_index]);
+
+	if (invert) {
+		*invert_mask |= (1 << hwirq_bit_index);
+		value |= IRQ_ROUTER_INVERT_MASK;
+	} else {
+		*invert_mask &= ~(1 << hwirq_bit_index);
+		value &= ~(IRQ_ROUTER_INVERT_MASK);
+	}
+
+	DBGLOG("hwirq(in) %d %s inverted\n", hwirq, invert ? "":"not");
+
+	tango_writel(irqrouter, offset, value);
+}
+
+
+static inline void tango_set_swirq_route(struct tango_irqrouter *irqrouter,
+					 int swirq_in,
+					 int irq_out)
+{
+	u32 swirq_reg_index = swirq_in / 4;
+	u32 swirq_bit_index = (swirq_in % 4) * 8;
+	u32 mask = ~(0x1f << swirq_bit_index);
+	u32 offset = SWIRQ_MAP_GROUP0 + (swirq_reg_index * 4);
+	u32 value = tango_readl(irqrouter, offset);
+
+	DBGLOG("ri %d, bi %d, mask 0x%x, offset 0x%x, val 0x%x\n",
+	       swirq_reg_index,

drivers/irqchip/irq-tango_v4.c new file +9 -0

+		value |= ((irq_out & 0x1f) << swirq_bit_index);
+
+	tango_writel(irqrouter, offset, value);
+}
+
+
+static inline void tango_set_hwirq_route(struct tango_irqrouter *irqrouter,
+					 int irq_in,
+					 int irq_out)

drivers/irqchip/irq-tango_v4.c new file +6 -0

+		return -EINVAL;
+	}
+
+	return 0;
+}
+

drivers/irqchip/irq-tango_v4.c new file +11 -0

+	tango_set_irq_enable(irqrouter, hwirq_in, 0);
+}
 
+#ifdef CONFIG_SMP
+static int tango_irqchip_set_irq_affinity(struct irq_data *data,
+					  const struct cpumask *mask_val,
+					  bool force)
+{
+	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
+	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
+	struct irq_chip *parent_chip = irq_get_chip(irqrouter_output->virq);
+	struct irq_data *parent_data = irq_get_irq_data(irqrouter_output->virq);

drivers/irqchip/irq-tango_v4.c new file +14 -0

+}
+
+static void tango_irqdomain_handle_cascade_irq(struct irq_desc *desc)
+{
+	struct irq_domain *domain = irq_desc_get_handler_data(desc);
+	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
+	struct tango_irqrouter *irqrouter = irqrouter_output->context;
+	struct irq_chip *host_chip = irq_desc_get_chip(desc);
+	u32 i, status;
+	u32 swirq_status, irq_status[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
+
+#if 0
+	DBGLOG("%s:%s(0x%p): irqrouter_output 0x%p, hwirq(out) %d\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain,

+ * @hwirq: HW IRQ of the device requesting an IRQ
+ * if hwirq > inputs it is a SW IRQ
+ * @virq: Linux IRQ (associated to the domain) to be given to the device
+ * @domain: IRQ domain (from the domain, we get the irqrouter_output
+ * in order to know to which output we need to route hwirq to)

+ */

drivers/irqchip/irq-tango_v4.c new file +32 -0

+static int tango_irqdomain_map(struct irq_domain *domain,
+			       unsigned int virq,
+			       irq_hw_number_t hwirq)
+{
+	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
+	struct tango_irqrouter *irqrouter = irqrouter_output->context;
+
+	DBGLOG("%s:%s(0x%p): hwirq(in) %d := virq %d, and route "
+	       "hwirq(in) %d => hwirq(out) %d (virq %d)\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)),
+	       domain->name,
+	       domain,
+	       (u32)hwirq,
+	       virq,
+	       (u32)hwirq,
+	       irqrouter_output->hwirq,
+	       irqrouter_output->virq);
+
+	if (hwirq >= (irqrouter->input_count + irqrouter->swirq_count))
+		DBGERR("%s: Invalid hwirq(in) %d >= %d + %d\n",
+		       NODE_NAME(irq_domain_get_of_node(domain)),
+		       (u32)hwirq,
+		       irqrouter->input_count,
+		       irqrouter->swirq_count);
+	else if (hwirq >= irqrouter->input_count)
+		DBGLOG("Map swirq %ld\n", hwirq - irqrouter->input_count);
+
+	irq_set_chip_and_handler(virq,
+				 &tango_irq_chip_shared_ops,
+				 handle_level_irq);
+	irq_set_chip_data(virq, domain);
+	irq_set_probe(virq);

+ * @domain: a registered domain
+ * @fwspec: an IRQ specification. This callback is used to translate the
+ * parameters given as irq_fwspec into a HW IRQ and Type values.

+ * @out_hwirq:
+ * @out_type:

+ */

drivers/irqchip/irq-tango_v4.c new file +13 -0

+static int tango_irqdomain_translate(struct irq_domain *domain,
+				     struct irq_fwspec *fwspec,
+				     unsigned long *out_hwirq,
+				     unsigned int *out_type)
+{
+	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
+	struct tango_irqrouter *irqrouter = irqrouter_output->context;
+	irq_hw_number_t irq;
+	u32 domain_id, type;
+	int err;
+
+	DBGLOG("%s:%s(0x%p): argc %d for hwirq(out) %d\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)),

drivers/irqchip/irq-tango_v4.c new file +7 -0

+	if (err < 0) {
+		DBGWARN("Failed to parse fwspec\n");
+		return err;
+	}
+
+	switch (domain_id) {
+	case SIGMA_HWIRQ:

drivers/irqchip/irq-tango_v4.c new file +8 -0

+		break;
+	};
+
+	*out_hwirq = irq;
+	*out_type  = type & IRQ_TYPE_SENSE_MASK;
+
+	DBGLOG("hwirq %d type 0x%x\n", (u32)*out_hwirq, (u32)*out_type);
+

+ * In practice it will return non-zero if the irq_fwspec matches one of the
+ * IRQs shared within the given domain.
+ * @bus_token: a bus token
+ */

drivers/irqchip/irq-tango_v4.c new file +29 -0

+static int tango_irqdomain_select(struct irq_domain *domain,
+				  struct irq_fwspec *fwspec,
+				  enum irq_domain_bus_token bus_token)
+{
+	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
+	struct tango_irqrouter *irqrouter = irqrouter_output->context;
+	irq_hw_number_t irq;
+	u32 domain_id, type;
+	int err;
+
+	DBGLOG("%s:%s(0x%p): argc %d, 0x%p, bus 0x%x\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain,
+	       fwspec->param_count, fwspec->fwnode, bus_token);
+
+	DBGLOG("router 0x%p, output 0x%p\n", irqrouter, irqrouter_output);
+
+	err = tango_parse_fwspec(domain, fwspec, &domain_id, &irq, &type);
+	if (err < 0)
+		return 0;
+
+	switch (domain_id) {
+	case SIGMA_HWIRQ:
+		DBGLOG("Request is for SIGMA_HWIRQ\n");
+		break;
+	case SIGMA_SWIRQ:
+		DBGLOG("Request is for SIGMA_SWIRQ\n");
+		break;
+	default:
+		DBGLOG("Request is for domain ID 0x%x (we are 0x%x)\n",

drivers/irqchip/irq-tango_v4.c new file +77 -0

+		 */
+		if (irqrouter_output->shared_count <= 0) {
+			DBGLOG("Not shared IRQ line?\n");
+			return 0;
+		}
+
+		for (i = 0; i < irqrouter_output->shared_count; i++) {
+			if (irq == irqrouter_output->shared_irqs[i]) {
+				DBGLOG("Match: IRQ %lu\n", irq);
+				return 1;
+			}
+		}
+	} else {
+		/* Otherwise, check if the domain_id given matches
+		 * the one assigned to this output
+		 */
+		if (domain_id == irqrouter_output->domain_id) {
+			DBGLOG("Match: Domain ID %d\n", domain_id);
+			return 1;
+		}
+	}
+
+	return 0;
+}
+
+
+/* 'irqrouter' handling callbacks
+ * Used for 'direct' IRQs, i.e.: IRQs that are directly routed to the GIC
+ * This driver does not dispatch the IRQs, the GIC does.
+ */
+
+
+static void tango_irqrouter_mask_irq(struct irq_data *data)
+{
+	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
+	struct tango_irqrouter *irqrouter = domain->host_data;
+	int hwirq_in = (int)data->hwirq;
+
+	DBGLOG("%s:%s(0x%p) hwirq(in) %d\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)),
+	       domain->name,
+	       domain,
+	       hwirq_in);
+
+	tango_set_irq_enable(irqrouter, hwirq_in, 0);
+
+	irq_chip_mask_parent(data);
+}
+
+
+static void tango_irqrouter_unmask_irq(struct irq_data *data)
+{
+	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
+	struct tango_irqrouter *irqrouter = domain->host_data;
+	int hwirq_in = (int)data->hwirq;
+
+	DBGLOG("%s:%s(0x%p) hwirq(in) %d\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)),
+	       domain->name,
+	       domain,
+	       hwirq_in);
+
+	tango_set_irq_enable(irqrouter, hwirq_in, 1);
+
+	irq_chip_unmask_parent(data);
+}
+
+
+static int tango_irqrouter_set_irq_type(struct irq_data *data,
+					unsigned int type)
+{
+	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
+	struct tango_irqrouter_output *irqrouter_output = NULL;
+	struct tango_irqrouter *irqrouter = domain->host_data;
+	int hwirq_in = (int)data->hwirq;
+	u32 parent_type;
+

drivers/irqchip/irq-tango_v4.c line 25

Quoted 4 lines · Sebastian Frias

drivers/irqchip/irq-tango_v4.c new file +4 -0

+
+#define DBGERR(__format, ...)	panic("[%s:%d] %s(): " __format,	\
+				      __FILE__, __LINE__,		\
+				      __func__, ##__VA_ARGS__)

Please get rid of this macro mess. Use the functions (panic, ...) directly. There is no value for this file, line, func crappola. Think about proper texts which can be grepped for.

Review

Quoted 3 lines

(quoted diff) +2 -0

+
+#define DBGWARN(__format, ...)	pr_err("[%s:%d] %s(): " __format,	\

Very consistent. WARN == err ....

drivers/irqchip/irq-tango_v4.c line 32

Quoted 5 lines

drivers/irqchip/irq-tango_v4.c new file +5 -0

+				       __FILE__, __LINE__,		\
+				       __func__, ##__VA_ARGS__)
+
+
+#if 0

Don't even think about posting code with "#if 0" or such in it.

drivers/irqchip/irq-tango_v4.c line 38

Quoted 6 lines

drivers/irqchip/irq-tango_v4.c new file +6 -0

+#define DBGLOG(__format, ...)   pr_info("[%s:%d] %s(): " __format,	\
+					__FILE__, __LINE__,		\
+					__func__, ##__VA_ARGS__)
+#else
+#define DBGLOG(__format, ...) do {} while (0)
+#endif

pr_debug() is there for a reason.

drivers/irqchip/irq-tango_v4.c line 139

Quoted 6 lines

drivers/irqchip/irq-tango_v4.c new file +5 -0

+#if 0
+#define SHORT_OR_FULL_NAME full_name
+#else
+#define SHORT_OR_FULL_NAME name
+#endif

Use one and be done with it. Really.

drivers/irqchip/irq-tango_v4.c line 142

Quoted 4 lines

drivers/irqchip/irq-tango_v4.c new file +3 -0

+
+#define NODE_NAME(__node__) (__node__ ? __node__->SHORT_OR_FULL_NAME :	\
+			     "<no-node>")

Sigh. pr_xxx("%s", NULL) prints <null>. That's really sufficient for your debug/error handling.

Quoted 23 lines

drivers/irqchip/irq-tango_v4.c new file +22 -0

+
+#define BITMASK_VECTOR_SIZE(__count__) (__count__ / 32)
+#define IRQ_TO_OFFSET(__hwirq__) (__hwirq__ * 4)
+
+struct tango_irqrouter;
+
+/*
+ * Maintains the mapping between a Linux virq and a hwirq
+ * on the parent controller.
+ * It is used by tango_irqdomain_map() or tango_irqdomain_hierarchy_alloc()
+ * to setup the route between input IRQ and output IRQ
+ */
+struct tango_irqrouter_output {
+	struct tango_irqrouter *context;
+
+	u32 domain_id;
+
+	u32 hwirq;
+	u32 hwirq_level;
+	u32 virq;
+
+	int shared_count;

drivers/irqchip/irq-tango_v4.c line 174

Quoted 10 lines

drivers/irqchip/irq-tango_v4.c new file +10 -0

+	int *shared_irqs;
+};
+
+/*
+ * Context for the driver
+ */
+struct tango_irqrouter {
+	raw_spinlock_t lock;
+	struct device_node *node;
+	void __iomem *base;

Plase align struct members proper

	raw_spinlock_t		lock;
	struct device_node	*node;
	void __iomem		*base;

drivers/irqchip/irq-tango_v4.c line 178

Quoted 5 lines

drivers/irqchip/irq-tango_v4.c new file +4 -0

+
+	int input_count;
+	u32 irq_mask[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
+	u32 irq_invert_mask[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];

You only ever use BITMASK_VECTOR_SIZE(ROUTER_INPUTS). Why can't you simply do:

#define ROUTER_INPUTs_MASK_SIZE (ROUTER__INPUTS / 32) ????

drivers/irqchip/irq-tango_v4.c line 349

Quoted 8 lines

drivers/irqchip/irq-tango_v4.c new file +7 -0

+static inline u32 tango_readl(struct tango_irqrouter *irqrouter,
+			      int reg)
+{
+	u32 val = readl_relaxed(irqrouter->base + reg);
+	/*DBGLOG("r[0x%08x + 0x%08x = 0x%08x] = 0x%08x\n",
+	  irqrouter->base, reg, irqrouter->base + reg, val);*/
+	return val;

Please get rid of this debug nonsense.

Quoted 13 lines

drivers/irqchip/irq-tango_v4.c new file +12 -0

+}
+
+
+static inline void tango_writel(struct tango_irqrouter *irqrouter,
+				int reg,
+				u32 val)
+{
+	/*DBGLOG("w[0x%08x + 0x%08x = 0x%08x] = 0x%08x\n",
+	  irqrouter->base, reg, irqrouter->base + reg, val);*/
+	writel_relaxed(val, irqrouter->base + reg);
+}
+

drivers/irqchip/irq-tango_v4.c line 367

Quoted 6 lines

drivers/irqchip/irq-tango_v4.c new file +6 -0

+
+static inline void tango_set_swirq_enable(struct tango_irqrouter *irqrouter,
+					  int swirq,
+					  bool enable)
+{
+	u32 offset = SWIRQ_ENABLE;

If you name that varible 'reg' then it becomes obvious what it does.

drivers/irqchip/irq-tango_v4.c line 378

Quoted 12 lines

drivers/irqchip/irq-tango_v4.c new file +11 -0

+	u32 value = tango_readl(irqrouter, offset);
+	u32 swirq_bit_index = swirq % SWIRQ_COUNT;
+
+#if 1
+	DBGLOG("%smask swirq(in) %d : current regvalue 0x%x\n",
+	       enable ? "un":"",
+	       swirq, value);
+#endif
+
+	if (enable) {
+		/* unmask swirq */

The whole code lacks comments, but here you document the obvious ....

Quoted 46 lines

drivers/irqchip/irq-tango_v4.c new file +45 -0

+		irqrouter->swirq_mask |= (1 << swirq_bit_index);
+		value |= (1 << swirq_bit_index);
+	} else {
+		/* mask swirq */
+		irqrouter->swirq_mask &= ~(1 << swirq_bit_index);
+		value &= ~(1 << swirq_bit_index);
+	}
+
+	tango_writel(irqrouter, offset, value);
+}
+
+
+static inline void tango_set_hwirq_enable(struct tango_irqrouter *irqrouter,
+					  int hwirq,
+					  bool enable)
+{
+	u32 offset = IRQ_TO_OFFSET(hwirq);
+	u32 value = tango_readl(irqrouter, offset);
+	u32 hwirq_reg_index = hwirq / 32;
+	u32 hwirq_bit_index = hwirq % 32;
+	u32 *enable_mask = &(irqrouter->irq_mask[hwirq_reg_index]);
+
+#if 1
+	DBGLOG("%smask hwirq(in) %d : current regvalue 0x%x\n",
+	       enable ? "un":"",
+	       hwirq, value);
+#endif
+
+	if (enable) {
+		/* unmask irq */
+		*enable_mask |= (1 << hwirq_bit_index);
+		value |= IRQ_ROUTER_ENABLE_MASK;
+	} else {
+		/* mask irq */
+		*enable_mask &= ~(1 << hwirq_bit_index);
+		value &= ~(IRQ_ROUTER_ENABLE_MASK);
+	}
+
+	tango_writel(irqrouter, offset, value);
+}
+
+
+static inline void tango_set_swirq_inversion(struct tango_irqrouter *irqrouter,
+					     int swirq,
+					     bool invert)

drivers/irqchip/irq-tango_v4.c line 429

Quoted 6 lines

drivers/irqchip/irq-tango_v4.c new file +6 -0

+{
+
+	DBGLOG("swirq(in) %d %s inverted\n", swirq, invert ? "":"not");
+
+	if (invert)
+		DBGERR("SW IRQs cannot be inverted!\n");

Groan.

Quoted 44 lines

drivers/irqchip/irq-tango_v4.c new file +43 -0

+}
+
+
+static inline void tango_set_hwirq_inversion(struct tango_irqrouter *irqrouter,
+					     int hwirq,
+					     bool invert)
+{
+	u32 offset = IRQ_TO_OFFSET(hwirq);
+	u32 value = tango_readl(irqrouter, offset);
+	u32 hwirq_reg_index = hwirq / 32;
+	u32 hwirq_bit_index = hwirq % 32;
+	u32 *invert_mask = &(irqrouter->irq_invert_mask[hwirq_reg_index]);
+
+	if (invert) {
+		*invert_mask |= (1 << hwirq_bit_index);
+		value |= IRQ_ROUTER_INVERT_MASK;
+	} else {
+		*invert_mask &= ~(1 << hwirq_bit_index);
+		value &= ~(IRQ_ROUTER_INVERT_MASK);
+	}
+
+	DBGLOG("hwirq(in) %d %s inverted\n", hwirq, invert ? "":"not");
+
+	tango_writel(irqrouter, offset, value);
+}
+
+
+static inline void tango_set_swirq_route(struct tango_irqrouter *irqrouter,
+					 int swirq_in,
+					 int irq_out)
+{
+	u32 swirq_reg_index = swirq_in / 4;
+	u32 swirq_bit_index = (swirq_in % 4) * 8;
+	u32 mask = ~(0x1f << swirq_bit_index);
+	u32 offset = SWIRQ_MAP_GROUP0 + (swirq_reg_index * 4);
+	u32 value = tango_readl(irqrouter, offset);
+
+	DBGLOG("ri %d, bi %d, mask 0x%x, offset 0x%x, val 0x%x\n",
+	       swirq_reg_index,
+	       swirq_bit_index,
+	       mask,
+	       offset,
+	       value);

drivers/irqchip/irq-tango_v4.c line 478

Quoted 6 lines

drivers/irqchip/irq-tango_v4.c new file +6 -0

+
+	DBGLOG("route swirq %d => hwirq(out) %d\n", swirq_in, irq_out);
+
+	value &= mask;
+
+	if (irq_out < 0) {

This logic is utterly confusing, really. What's a negative interrupt number?

drivers/irqchip/irq-tango_v4.c line 481

Quoted 4 lines

drivers/irqchip/irq-tango_v4.c new file +3 -0

+		tango_set_irq_enable(irqrouter,
+				     swirq_in + irqrouter->input_count,
+				     0);

Now you write back 'value' with the bit for this irq masked. Is that correct?

Review

Quoted 2 lines

(quoted diff) +1 -0

+	} else

  } else {

please

Quoted 14 lines

drivers/irqchip/irq-tango_v4.c new file +13 -0

+		value |= ((irq_out & 0x1f) << swirq_bit_index);
+
+	tango_writel(irqrouter, offset, value);
+}
+
+
+static inline void tango_set_hwirq_route(struct tango_irqrouter *irqrouter,
+					 int irq_in,
+					 int irq_out)
+{
+	u32 offset = IRQ_TO_OFFSET(irq_in);
+	u32 value;
+

drivers/irqchip/irq-tango_v4.c line 501

Quoted 6 lines

drivers/irqchip/irq-tango_v4.c new file +6 -0

+	DBGLOG("route hwirq(in) %d => hwirq(out) %d\n", irq_in, irq_out);
+
+	if (irq_out < 0) {
+		tango_set_irq_enable(irqrouter,
+				     irq_in,
+				     0);

This fits in a single line.

drivers/irqchip/irq-tango_v4.c line 502

Quoted 2 lines

drivers/irqchip/irq-tango_v4.c new file +1 -0

+		value = 0;

Please document why you are setting the value to 0 here.

Quoted 4 lines

drivers/irqchip/irq-tango_v4.c new file +3 -0

+	} else
+		value = (irq_out & 0x1f);
+

drivers/irqchip/irq-tango_v4.c line 515

Quoted 10 lines

drivers/irqchip/irq-tango_v4.c new file +10 -0

+	tango_writel(irqrouter, offset, value);
+}
+
+
+static inline int tango_set_irq_enable(struct tango_irqrouter *irqrouter,
+				       int irq,
+				       bool enable)
+{
+	if (irq >= (irqrouter->input_count + irqrouter->swirq_count))
+		return -EINVAL;

How can that happen? Paranoia programming?

drivers/irqchip/irq-tango_v4.c line 516

Quoted 2 lines

drivers/irqchip/irq-tango_v4.c new file +1 -0

+	else if (irq >= irqrouter->input_count)

That "else" is pointless. You return above.

drivers/irqchip/irq-tango_v4.c line 523

Quoted 8 lines

drivers/irqchip/irq-tango_v4.c new file +7 -0

+		tango_set_swirq_enable(irqrouter,
+				       irq - irqrouter->input_count,
+				       enable);
+	else
+		tango_set_hwirq_enable(irqrouter,
+				       irq,
+				       enable);

This can be written way simpler.

     	if (irq >= irqrouter->input_count)
	   	irq -= irqrouter->input_count;
	tango_set_hwirq_enable(irqrouter, irq, enable);

Hmm?

Quoted 10 lines

drivers/irqchip/irq-tango_v4.c new file +9 -0

+static int tango_set_irq_type(struct tango_irqrouter *irqrouter,
+			      int hwirq_in,
+			      u32 type,
+			      u32 parent_type)
+{
+	int err;
+
+	if (parent_type & (type & IRQ_TYPE_SENSE_MASK))
+		/* same polarity */

drivers/irqchip/irq-tango_v4.c line 576

Quoted 4 lines

drivers/irqchip/irq-tango_v4.c new file +4 -0

+		err = tango_set_irq_inversion(irqrouter, hwirq_in, 0);
+	else
+		/* invert polarity */
+		err = tango_set_irq_inversion(irqrouter, hwirq_in, 1);

  	bool invert = parent_type & (type & IRQ_TYPE_SENSE_MASK);

  	err = tango_set_irq_inversion(irqrouter, hwirq_in, invert);	

Quoted 5 lines

drivers/irqchip/irq-tango_v4.c new file +5 -0

+
+	if (err < 0) {
+		DBGWARN("Failed to setup IRQ %d polarity\n", hwirq_in);
+		return err;
+	}

drivers/irqchip/irq-tango_v4.c line 586

Quoted 5 lines

drivers/irqchip/irq-tango_v4.c new file +5 -0

+
+	switch (type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_RISING:
+	case IRQ_TYPE_EDGE_FALLING:
+		DBGERR("Does not support edge triggers\n");

This really sucks. In fact you panic here while the code suggests that you print a DEBUG error and continue .....

drivers/irqchip/irq-tango_v4.c line 589

Quoted 3 lines

drivers/irqchip/irq-tango_v4.c new file +3 -0

+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		break;

You can spare that break.

drivers/irqchip/irq-tango_v4.c line 594

Quoted 6 lines

drivers/irqchip/irq-tango_v4.c new file +5 -0

+	case IRQ_TYPE_LEVEL_LOW:
+		break;
+	default:
+		DBGWARN("Invalid trigger mode 0x%x for hwirq(in) %d\n",
+			type, hwirq_in);

So here you continue with an error code, but above for EDGE you panic. I really don't get that logic.

Quoted 11 lines

drivers/irqchip/irq-tango_v4.c new file +10 -0

+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+
+static int tango_get_output_for_hwirq(struct tango_irqrouter *irqrouter,
+				      int hwirq_in,
+				      struct tango_irqrouter_output **out_val)

drivers/irqchip/irq-tango_v4.c line 610

Quoted 6 lines

drivers/irqchip/irq-tango_v4.c new file +6 -0

+{
+	struct tango_irqrouter_output *irqrouter_output;
+	int i;
+
+	if (!out_val)
+		return -EINVAL;

This is utter crap. Look at the call site:

drivers/irqchip/irq-tango_v4.c line 1075

Quoted 6 lines

drivers/irqchip/irq-tango_v4.c new file +5 -0

+	struct tango_irqrouter_output *irqrouter_output = NULL;
+
+	tango_get_output_for_hwirq(irqrouter, hwirq_in, &irqrouter_output);
+	if (!irqrouter_output)
+		goto handle_parent;

So out_val CANNOT be NULL.

What's the purpose of your return values if you
ignore them?

drivers/irqchip/irq-tango_v4.c line 612

Quoted 3 lines

drivers/irqchip/irq-tango_v4.c new file +2 -0

+
+	/* Get the irqrouter_output for the hwirq */

This is another really helpful comment.

drivers/irqchip/irq-tango_v4.c line 623

Quoted 12 lines

drivers/irqchip/irq-tango_v4.c new file +11 -0

+	for (i = 0; i < irqrouter->output_count; i++) {
+		int j;
+
+		irqrouter_output = &(irqrouter->output[i]);
+
+		for (j = 0; j < irqrouter_output->shared_count; j++) {
+			if (hwirq_in == irqrouter_output->shared_irqs[j])
+				goto found_router_output;
+		}
+	}
+	if (i == irqrouter->output_count) {

What other reason would be to get here than this?

drivers/irqchip/irq-tango_v4.c line 632

Quoted 10 lines

drivers/irqchip/irq-tango_v4.c new file +9 -0

+		DBGWARN("Couldn't find hwirq mapping\n");
+		return -ENODEV;
+	}
+
+found_router_output:
+
+	*out_val = irqrouter_output;
+	return 0;
+}

This whole thing can be condensed to:

static struct tango_irqrouter_output * tango_get_output_for_hwirq(struct tango_irqrouter *irqr, int hwirq_in)

{
	struct tango_irqrouter_output *rout;
	int i, j;

	/* Scan the outputs for a matching hwirq number */
	for (i = 0, rout = irqr->output; i < irqr->output_count; i++, rout++) {
		for (j = 0; j < rout->shared_count; j++) {
			if (rout->shared_irqs[j] == hwirq_in)
			   	return rout;
		}
	}
	return NULL;
}

And the call site would become:

    	struct tango_irqrouter_output *rout;

	rout = tango_get_output_for_hwirq(irqrouter, hwirq_in);
	if (!rout)
		goto handle_parent;

Hmm?

drivers/irqchip/irq-tango_v4.c line 262

Quoted 6 lines

drivers/irqchip/irq-tango_v4.c new file +5 -0

+/* 'irqchip' handling callbacks
+ * Used for 'shared' IRQs, i.e.: IRQs that share a GIC input
+ * This driver performs the IRQ dispatch based on the flags
+ */
+

Multiline comments have this format:

/*
* First line
* ....
* Last line
*/

drivers/irqchip/irq-tango_v4.c line 685

Quoted 7 lines

drivers/irqchip/irq-tango_v4.c new file +6 -0

+static void tango_irqchip_mask_irq(struct irq_data *data)
+{
+	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
+	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
+	struct tango_irqrouter *irqrouter = irqrouter_output->context;
+	int hwirq_in = (int)data->hwirq;

Huch? What's that silly typecast for? Why can't you use u32 for hwirq_in?

Quoted 13 lines

drivers/irqchip/irq-tango_v4.c new file +11 -0

+	tango_set_irq_enable(irqrouter, hwirq_in, 0);
+}
 
+#ifdef CONFIG_SMP
+static int tango_irqchip_set_irq_affinity(struct irq_data *data,
+					  const struct cpumask *mask_val,
+					  bool force)
+{
+	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
+	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
+	struct irq_chip *parent_chip = irq_get_chip(irqrouter_output->virq);
+	struct irq_data *parent_data = irq_get_irq_data(irqrouter_output->virq);

drivers/irqchip/irq-tango_v4.c line 743

Quoted 10 lines

drivers/irqchip/irq-tango_v4.c new file +10 -0

+
+	DBGLOG("%s:%s(0x%p)\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain);
+
+	if (parent_chip && parent_chip->irq_set_affinity)
+		return parent_chip->irq_set_affinity(parent_data,
+						     mask_val,
+						     force);
+	else
+		return -EINVAL;

You really can spare identation levels by writing it a bit more clever:

    	if (!parent || !parent->irq_set_affinity)
	   	 return -EINVAL;

	return parent->irq_set_affinity(parent, mask, force);

You might notice that I shortened the variable names and it still is entirely clear what the code does.

Aside of that mask_val is a clear misnomer.

drivers/irqchip/irq-tango_v4.c line 754

Quoted 12 lines

drivers/irqchip/irq-tango_v4.c new file +11 -0

+}
+#endif
+
+
+static inline u32 tango_dispatch_irqs(struct irq_domain *domain,
+				      struct irq_desc *desc,
+				      u32 status,
+				      int base)
+{
+	u32 hwirq;
+	u32 virq;

Please put same types onto a single line

Quoted 4 lines

drivers/irqchip/irq-tango_v4.c new file +3 -0

+
+	while (status) {
+		hwirq = __ffs(status);

drivers/irqchip/irq-tango_v4.c line 767

Quoted 10 lines

drivers/irqchip/irq-tango_v4.c new file +10 -0

+		virq = irq_find_mapping(domain, base + hwirq);
+		if (unlikely(!virq))
+			handle_bad_irq(desc);
+		else
+			generic_handle_irq(virq);
+
+		status &= ~BIT(hwirq);
+	}
+
+	return status;

Huch? status is guaranteed to be 0 here. So what's the point of this return value.

Quoted 19 lines

drivers/irqchip/irq-tango_v4.c new file +18 -0

+}
+
+static void tango_irqdomain_handle_cascade_irq(struct irq_desc *desc)
+{
+	struct irq_domain *domain = irq_desc_get_handler_data(desc);
+	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
+	struct tango_irqrouter *irqrouter = irqrouter_output->context;
+	struct irq_chip *host_chip = irq_desc_get_chip(desc);
+	u32 i, status;
+	u32 swirq_status, irq_status[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
+
+#if 0
+	DBGLOG("%s:%s(0x%p): irqrouter_output 0x%p, hwirq(out) %d\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain,
+	       irqrouter_output, irqrouter_output->hwirq);
+#endif
+
+	chained_irq_enter(host_chip, desc);

drivers/irqchip/irq-tango_v4.c line 792

Quoted 6 lines

drivers/irqchip/irq-tango_v4.c new file +6 -0

+
+	raw_spin_lock(&(irqrouter->lock));
+	swirq_status = tango_readl(irqrouter, READ_SWIRQ_STATUS);
+	for (i = 0; i < BITMASK_VECTOR_SIZE(ROUTER_INPUTS); i++)
+		irq_status[i] = tango_readl(irqrouter,
+					    READ_SYS_IRQ_GROUP0 + i*4);

i * 4 please

drivers/irqchip/irq-tango_v4.c line 795

Quoted 4 lines

drivers/irqchip/irq-tango_v4.c new file +3 -0

+	raw_spin_unlock(&(irqrouter->lock));
+
+	/* HW irqs */

And that comment tells us what?

drivers/irqchip/irq-tango_v4.c line 806

Quoted 11 lines

drivers/irqchip/irq-tango_v4.c new file +11 -0

+	for (i = 0; i < BITMASK_VECTOR_SIZE(ROUTER_INPUTS); i++) {
+#if 0
+		DBGLOG("%d: 0x%08x (en 0x%08x, inv 0x%08x)\n",
+		       i,
+		       irq_status[i],
+		       irqrouter->irq_mask[0],
+		       irqrouter->irq_invert_mask[0]);
+#endif
+
+#define HANDLE_INVERTED_LINES(__irqstatus__, __x__) ((((~__irqstatus__) & irqrouter->irq_invert_mask[__x__]) & irqrouter->irq_mask[__x__]) | __irqstatus__)
+#define HANDLE_EN_AND_INV_MASKS(__irqstatus__, __y__) (HANDLE_INVERTED_LINES(__irqstatus__, __y__) & irqrouter->irq_mask[__y__])

What the hell does this unparseable macro mess? What's so wrong with a proper inline function?

drivers/irqchip/irq-tango_v4.c line 810

Quoted 5 lines

drivers/irqchip/irq-tango_v4.c new file +4 -0

+
+		irq_status[i] = HANDLE_EN_AND_INV_MASKS(irq_status[i], i);
+		status = tango_dispatch_irqs(domain, desc, irq_status[i], i*32);
+		if (status & irq_status[i])

Can you pretty please explain how this can happen? It can't. Because you clear every single bit of status in tango_dispatch_irqs(). See above.

Please clean up that unholy mess of nonsensical checks and debug code.

drivers/irqchip/irq-tango_v4.c line 819

Quoted 9 lines

drivers/irqchip/irq-tango_v4.c new file +9 -0

+			DBGERR("%s: %d unhandled IRQs (as a mask) 0x%x\n",
+			       NODE_NAME(irq_domain_get_of_node(domain)),
+			       i,
+			       status & irq_status[i]);
+	}
+
+	/* SW irqs */
+	swirq_status &= irqrouter->swirq_mask;
+	status = tango_dispatch_irqs(domain, desc, swirq_status, 128);

128 is the magic number pulled out of thin air or what?

drivers/irqchip/irq-tango_v4.c line 823

Quoted 5 lines

drivers/irqchip/irq-tango_v4.c new file +4 -0

+	if (status & swirq_status)
+		DBGERR("%s: Unhandled IRQs (as a mask) 0x%x\n",
+		       NODE_NAME(irq_domain_get_of_node(domain)),
+		       status & swirq_status);

See above.

drivers/irqchip/irq-tango_v4.c line 832

Quoted 9 lines

drivers/irqchip/irq-tango_v4.c new file +9 -0

+
+	chained_irq_exit(host_chip, desc);
+}
+
+
+/**
+ * tango_irqdomain_map - route a hwirq(in) to a hwirq(out).
+ * NOTE: The hwirq(out) must have been already allocated and enabled on
+ * the parent controller.

Please put notes AFTER the argument descriptions

Quoted 6 lines

+ * @hwirq: HW IRQ of the device requesting an IRQ
+ * if hwirq > inputs it is a SW IRQ
+ * @virq: Linux IRQ (associated to the domain) to be given to the device
+ * @domain: IRQ domain (from the domain, we get the irqrouter_output
+ * in order to know to which output we need to route hwirq to)

And if you spend a few seconds to align this proper, then it becomes suddenly readable.

* @hwirq: HW IRQ of the device requesting an IRQ
* if hwirq > inputs it is a SW IRQ
* @virq: Linux IRQ (associated to the domain) to be given to the device
* @domain: IRQ domain (from the domain, we get the irqrouter_output
* in order to know to which output we need to route hwirq to)

Hmm?

Quoted 34 lines

+ */

drivers/irqchip/irq-tango_v4.c new file +32 -0

+static int tango_irqdomain_map(struct irq_domain *domain,
+			       unsigned int virq,
+			       irq_hw_number_t hwirq)
+{
+	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
+	struct tango_irqrouter *irqrouter = irqrouter_output->context;
+
+	DBGLOG("%s:%s(0x%p): hwirq(in) %d := virq %d, and route "
+	       "hwirq(in) %d => hwirq(out) %d (virq %d)\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)),
+	       domain->name,
+	       domain,
+	       (u32)hwirq,
+	       virq,
+	       (u32)hwirq,
+	       irqrouter_output->hwirq,
+	       irqrouter_output->virq);
+
+	if (hwirq >= (irqrouter->input_count + irqrouter->swirq_count))
+		DBGERR("%s: Invalid hwirq(in) %d >= %d + %d\n",
+		       NODE_NAME(irq_domain_get_of_node(domain)),
+		       (u32)hwirq,
+		       irqrouter->input_count,
+		       irqrouter->swirq_count);
+	else if (hwirq >= irqrouter->input_count)
+		DBGLOG("Map swirq %ld\n", hwirq - irqrouter->input_count);
+
+	irq_set_chip_and_handler(virq,
+				 &tango_irq_chip_shared_ops,
+				 handle_level_irq);
+	irq_set_chip_data(virq, domain);
+	irq_set_probe(virq);

drivers/irqchip/irq-tango_v4.c line 880

Quoted 10 lines

drivers/irqchip/irq-tango_v4.c new file +10 -0

+
+	tango_set_irq_route(irqrouter, hwirq, irqrouter_output->hwirq);
+
+	return 0;
+}
+
+
+/**
+ * tango_irqdomain_translate - used to select the domain for a given
+ * irq_fwspec

It hardly selects the domain. @domain is handed in as a argument. And please get rid of that silly 'used to'. That just adds characters and no information.

- Select the router section for a firmware spec

Hmm?

Quoted 4 lines

+ * @domain: a registered domain
+ * @fwspec: an IRQ specification. This callback is used to translate the
+ * parameters given as irq_fwspec into a HW IRQ and Type values.

fwspec is a callback?

Quoted 3 lines

+ * @out_hwirq:
+ * @out_type:

Missing docs.....

Quoted 21 lines

+ */

drivers/irqchip/irq-tango_v4.c new file +19 -0

+static int tango_irqdomain_translate(struct irq_domain *domain,
+				     struct irq_fwspec *fwspec,
+				     unsigned long *out_hwirq,
+				     unsigned int *out_type)
+{
+	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
+	struct tango_irqrouter *irqrouter = irqrouter_output->context;
+	irq_hw_number_t irq;
+	u32 domain_id, type;
+	int err;
+
+	DBGLOG("%s:%s(0x%p): argc %d for hwirq(out) %d\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)),
+	       domain->name,
+	       domain,
+	       fwspec->param_count,
+	       irqrouter_output->hwirq);
+
+	err = tango_parse_fwspec(domain,

drivers/irqchip/irq-tango_v4.c line 909

Quoted 4 lines

drivers/irqchip/irq-tango_v4.c new file +4 -0

+				 fwspec,
+				 &domain_id,
+				 &irq,
+				 &type);

Please use the full 80 characters. Your patch does not become more valuable when you inflate the line count. It's more valuable when it is easy to read and understand.

Quoted 14 lines

drivers/irqchip/irq-tango_v4.c new file +13 -0

+	if (err < 0) {
+		DBGWARN("Failed to parse fwspec\n");
+		return err;
+	}
+
+	switch (domain_id) {
+	case SIGMA_HWIRQ:
+		DBGLOG("Request is for SIGMA_HWIRQ\n");
+		break;
+	case SIGMA_SWIRQ:
+		DBGLOG("Request is for SIGMA_SWIRQ\n");
+		irq += irqrouter->input_count;
+		break;

drivers/irqchip/irq-tango_v4.c line 926

Quoted 4 lines

drivers/irqchip/irq-tango_v4.c new file +4 -0

+	default:
+		DBGLOG("Request is for domain ID 0x%x (we are 0x%x)\n",
+		       domain_id,
+		       irqrouter_output->domain_id);

Huch what? You only ever handle HW and SW irqs and now you happily proceed if the id is something else. Consistent error handling is optional and can be replaced by random debug prints, right?

Quoted 13 lines

drivers/irqchip/irq-tango_v4.c new file +12 -0

+		break;
+	};
+
+	*out_hwirq = irq;
+	*out_type  = type & IRQ_TYPE_SENSE_MASK;
+
+	DBGLOG("hwirq %d type 0x%x\n", (u32)*out_hwirq, (u32)*out_type);
+
+	return 0;
+}
+
+

Quoted 6 lines

+/**
+ * tango_irqdomain_select - used to select the domain for a given irq_fwspec
+ * @domain: a registered domain
+ * @fwspec: an IRQ specification. This callback should return zero if the
+ * irq_fwspec does not belong to the given domain. If it does, it should
+ * return non-zero.

Callback????

Quoted 40 lines

+ * In practice it will return non-zero if the irq_fwspec matches one of the
+ * IRQs shared within the given domain.
+ * @bus_token: a bus token
+ */

drivers/irqchip/irq-tango_v4.c new file +35 -0

+static int tango_irqdomain_select(struct irq_domain *domain,
+				  struct irq_fwspec *fwspec,
+				  enum irq_domain_bus_token bus_token)
+{
+	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
+	struct tango_irqrouter *irqrouter = irqrouter_output->context;
+	irq_hw_number_t irq;
+	u32 domain_id, type;
+	int err;
+
+	DBGLOG("%s:%s(0x%p): argc %d, 0x%p, bus 0x%x\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain,
+	       fwspec->param_count, fwspec->fwnode, bus_token);
+
+	DBGLOG("router 0x%p, output 0x%p\n", irqrouter, irqrouter_output);
+
+	err = tango_parse_fwspec(domain, fwspec, &domain_id, &irq, &type);
+	if (err < 0)
+		return 0;
+
+	switch (domain_id) {
+	case SIGMA_HWIRQ:
+		DBGLOG("Request is for SIGMA_HWIRQ\n");
+		break;
+	case SIGMA_SWIRQ:
+		DBGLOG("Request is for SIGMA_SWIRQ\n");
+		break;
+	default:
+		DBGLOG("Request is for domain ID 0x%x (we are 0x%x)\n",
+		       domain_id,
+		       irqrouter_output->domain_id);
+		break;
+	};
+
+	if (!irqrouter->implicit_groups) {