On Thu, Nov 14, 2013 at 04:41:41PM +0000, Sricharan R wrote:

Hi Mark,

On Thursday 14 November 2013 07:42 PM, Mark Rutland wrote:

quoted

On Thu, Nov 14, 2013 at 12:18:48PM +0000, Sricharan R wrote:

quoted

Some socs have a large number of interrupts requests to service
the needs of its many peripherals and subsystems. All of the
interrupt lines from the subsystems are not needed at the same
time, so they have to be muxed to the irq-controller appropriately.
In such places a interrupt controllers are preceded by an CROSSBAR
that provides flexibility in muxing the device requests to the controller
inputs.

This driver takes care a allocating a free irq and then configuring the
crossbar IP as a part of the mpu's irqchip callbacks. crossbar_init should
be called right before the irqchip_init, so that it is setup to handle the
irqchip callbacks.

Cc: Thomas Gleixner <redacted>
Cc: Linus Walleij <redacted>
Cc: Santosh Shilimkar <redacted>
Cc: Russell King <redacted>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Rajendra Nayak <redacted>
Cc: Marc Zyngier <redacted>
Cc: Grant Likely <redacted>
Cc: Rob Herring <redacted>
Signed-off-by: Sricharan R <redacted>
Acked-by: Kumar Gala <redacted> (for DT binding portion)
Acked-by: Santosh Shilimkar <redacted>
---
 [V2] Addressed Thomas Gleixner [off-list ref] comments
      and renamed the bindings as per Kumar Gala [off-list ref]
      comments.
 [V3] Changed static inline const to static inline int and removed
      unnecessary variable initialization as per
      Thomas Gleixner [off-list ref]. Updated commit tags
 [V4] Renamed crossbar_init as irqcrossbar_init as per
      Rajendra Nayak [off-list ref] suggestion.

 .../devicetree/bindings/arm/omap/crossbar.txt      |   27 +++
 drivers/irqchip/Kconfig                            |    8 +
 drivers/irqchip/Makefile                           |    1 +
 drivers/irqchip/irq-crossbar.c                     |  206 ++++++++++++++++++++
 include/linux/irqchip/irq-crossbar.h               |   11 ++
 5 files changed, 253 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/omap/crossbar.txt
 create mode 100644 drivers/irqchip/irq-crossbar.c
 create mode 100644 include/linux/irqchip/irq-crossbar.h

diff --git a/Documentation/devicetree/bindings/arm/omap/crossbar.txt b/Documentation/devicetree/bindings/arm/omap/crossbar.txt
new file mode 100644
index 0000000..fb88585
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/omap/crossbar.txt

@@ -0,0 +1,27 @@
+Some socs have a large number of interrupts requests to service
+the needs of its many peripherals and subsystems. All of the
+interrupt lines from the subsystems are not needed at the same
+time, so they have to be muxed to the irq-controller appropriately.
+In such places a interrupt controllers are preceded by an CROSSBAR
+that provides flexibility in muxing the device requests to the controller
+inputs.
+
+Required properties:
+- compatible : Should be "ti,irq-crossbar"
+- reg: Base address and the size of the crossbar registers.
+- ti,max-irqs: Total number of irqs available at the interrupt controller.
+- ti,reg-size: Size of a individual register in bytes. Every individual
+	    register is assumed to be of same size. Valid sizes are 1, 2, 4.
+- ti,irqs-reserved: List of the reserved irq lines that are not muxed using
+		 crossbar. These interrupt lines are reserved in the soc,
+		 so crossbar bar driver should not consider them as free
+		 lines.

The combination of the ti,max-irqs and ti,irqs-reserved properties seems
backwards to me. Why can we not describe the set of IRQs that _can_ be
used?

 Total set of irqs that are usable is max - reserved. Since reserved irqs
 are not continuous, we have to give the list. During the init we count
 the total number of reserved and get the usable one.

So why not describe the set of usable IRQs, rather than a set of IRQs
for which only some are usable then subtracting the set of unusable
IRQs?

It seems backwards to me to have a binding for a device describe
resources it doesn't have.

quoted

quoted

+
+Examples:
+		crossbar_mpu: @4a020000 {
+			compatible = "ti,irq-crossbar";
+			reg = <0x4a002a48 0x130>;
+			ti,max-irqs = <160>;
+			ti,reg-size = <2>;
+			ti,irqs-reserved = <0 1 2 3 5 6 131 132 139 140>;
+		};

[...]

quoted

+	/* Get and mark reserved irqs */
+	irqsr = of_get_property(node, "ti,irqs-reserved", &size);
+	if (irqsr) {
+		size /= sizeof(__be32);
+
+		for (i = 0; i < size; i++) {
+			entry = be32_to_cpup(irqsr + i);
+			if (entry > max) {
+				pr_err("Invalid reserved entry\n");
+				goto err3;
+			}
+			cb->irq_map[entry] = 0;
+		}
+	}

Don't deal with the raw DTB. Use of_property_read_u32_index.

 Ok, i will correct this.

quoted

quoted

+
+	cb->register_offsets = kzalloc(max * sizeof(int), GFP_KERNEL);
+	if (!cb->register_offsets)
+		goto err3;
+
+	of_property_read_u32(node, "ti,reg-size", &size);

If "ti,reg-size" isn't present, size is uninitialized. Please check the
return value of of_property_read_u32.

Ok, will correct this.

Cheers.

Mark.