irqchip/dra7: crossbar bug fixes

From: Nishanth Menon <[email protected]>

When, in the system due to varied reasons, interrupts might be unusable due to hardware behavior, but register maps do exist, then those interrupts should be skipped while mapping irq to crossbars.

Signed-off-by Nishanth Menon <[email protected]>

Signed-off-by Sricharan R <[email protected]>

---
 drivers/irqchip/irq-crossbar.c |   47 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 43 insertions(+), 4 deletions(-)

drivers/irqchip/irq-crossbar.c +43 -4

index 51d4b87..847f6e3 100644
@@ -13,11 +13,13 @@
 #include <linux/io.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
+#include <linux/of_device.h>
 #include <linux/slab.h>
 #include <linux/irqchip/arm-gic.h>
 
 #define IRQ_FREE	-1
 #define IRQ_RESERVED	-2
+#define IRQ_SKIP	-3
 #define GIC_IRQ_START	32
 
 /*

@@ -34,6 +36,16 @@ struct crossbar_device {
 	void (*write) (int, int);
 };
 
+/**
+ * struct crossbar_data: Platform specific data
+ * @irqs_unused: array of irqs that cannot be used because of hw erratas
+ * @size: size of the irqs_unused array
+ */
+struct crossbar_data {
+	const uint *irqs_unused;
+	const uint size;
+};
+
 static struct crossbar_device *cb;
 
 static inline void crossbar_writel(int irq_no, int cb_no)

@@ -119,10 +131,12 @@ const struct irq_domain_ops routable_irq_domain_ops = {
 	.xlate = crossbar_domain_xlate
 };
 
-static int __init crossbar_of_init(struct device_node *node)
+static int __init crossbar_of_init(struct device_node *node,
+				   const struct crossbar_data *data)
 {
 	int i, size, max, reserved = 0, entry;
 	const __be32 *irqsr;
+	const int *irqsk = NULL;
 
 	cb = kzalloc(sizeof(*cb), GFP_KERNEL);
 

@@ -194,6 +208,22 @@ static int __init crossbar_of_init(struct device_node *node)
 		reserved += size;
 	}
 
+	/* Skip the ones marked as unused */
+	if (data) {
+		irqsk = data->irqs_unused;
+		size = data->size;
+
+		for (i = 0; i < size; i++) {
+			entry = irqsk[i];
+
+			if (entry > max) {
+				pr_err("Invalid skip entry\n");
+				goto err3;
+			}
+			cb->irq_map[entry] = IRQ_SKIP;
+		}
+	}
+
 	register_routable_domain_ops(&routable_irq_domain_ops);
 	return 0;
 

@@ -208,18 +238,27 @@ err1:
 	return -ENOMEM;
 }
 
+/* irq number 10 cannot be used because of hw bug */
+int dra_irqs_unused[] = { 10 };
+struct crossbar_data cb_dra_data = { dra_irqs_unused,
+				     ARRAY_SIZE(dra_irqs_unused) };
+
 static const struct of_device_id crossbar_match[] __initconst = {
-	{ .compatible = "ti,irq-crossbar" },
+	{ .compatible = "ti,irq-crossbar", .data = &cb_dra_data },
 	{}
 };
 
 int __init irqcrossbar_init(void)
 {
 	struct device_node *np;
-	np = of_find_matching_node(NULL, crossbar_match);
+	const struct of_device_id *of_id;
+	const struct crossbar_data *cdata;
+
+	np = of_find_matching_node_and_match(NULL, crossbar_match, &of_id);
 	if (!np)
 		return -ENODEV;
 
-	crossbar_of_init(np);
+	cdata = of_id->data;
+	crossbar_of_init(np, cdata);
 	return 0;
 }

Quoted 6 lines · Sricharan R

From: Nishanth Menon <[email protected]>

When, in the system due to varied reasons, interrupts might be unusable due to hardware behavior, but register maps do exist, then those interrupts should be skipped while mapping irq to crossbars.

Just wondering, instead of hardcoding this data in the code, and introducing additional flags (IRQ_SKIP), why not just put these GIC IRQs in the ti,irq-reserved property in DTS for platforms where such IRQs are not usable. That way you're skipping these IRQs anyway.

Also that would avoid adding more hard coded data for future SoCs into the source for such IRQs that must be skipped, and also reduces LOC.

Quoted 8 lines · Joel Fernandes

Quoted 6 lines · Sricharan R

From: Nishanth Menon <[email protected]>

When, in the system due to varied reasons, interrupts might be unusable due to hardware behavior, but register maps do exist, then those interrupts should be skipped while mapping irq to crossbars.

Quoted 8 lines

Just wondering, instead of hardcoding this data in the code, and introducing additional flags (IRQ_SKIP), why not just put these GIC IRQs in the ti,irq-reserved property in DTS for platforms where such IRQs are not usable. That way you're skipping these IRQs anyway.

Also that would avoid adding more hard coded data for future SoCs into the source for such IRQs that must be skipped, and also reduces LOC.

Good question - lets try to explain the hardware a little here -> obviously a driver that cannot use the hardware is useless compared to reducing LOC count ;).. and apologies about the long reply..

Basic understanding:
GIC has 160 SPIs and number of hardware block interrupt sources is around or more than 400. So, in comes crossbar - which is basically a mapper by allowing us to select an hardware block interrupt source (identified as crossbar_number or cb_no in code). So all we have to do is to write to a register in crossbar corresponding to GIC and viola, we now routed the interrupt source to a GIC interrupt of our choice. At least the Specification reads so.... until you drill down to the details.

A) You have 160 SPI GIC, and 152 crossbar registers. So, you have 8 GIC SPI interrupts that are hardwired. the reserved mapping basically marks these to indicate that we dont have registers. Example: 0 1 2 3 5
6 131 and 132
- Limitation today - if you want to use PMU for CPU0, SPI interrupt is 131, then if you define, in dts:
interrupts = <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>
driver assumes it is crossbar number 131(reserved), Similarly:
GIC CS_CTI_MPU_C0_IRQ (SPI 1) is ELM_IRQ (crossbar 1)
GIC CS_CTI_MPU_C1_IRQ (SPI 2) is EXT_SYS_IRQ_1 (crossbar 2) GIC MPU_CLUSTER_IRQ_AXIERR (SPI 3) is reserved (crossbar 3) GIC WD_TIMER_MPU_C0_IRQ_WARN (SPI 5) is L3_MAIN_IRQ_APP_ERR (crossbar 5) GIC WD_TIMER_MPU_C1_IRQ_WARN (SPI 6) is PRM_IRQ_MPU (crossbar 6) GIC MPU_CLUSTER_IRQ_PMU_C0 (SPI 131) is reserved (crossbar 131) GIC MPU_CLUSTER_IRQ_PMU_C1 (SPI 132) is reserved (crossbar 132)

As of today, we cannot differentiate in DTS if it is one of these "direct map" interrupts we are requesting or crossbar number we are requesting.

B) among the 152 cross bar registers, you have three sets:
B.1) The ones like Crossbar register 1 which maps to SPI4 - no problem -
you write the crossbar number you want to map, bingo, job done.
- The driver works brilliantly here. and this is true for 148 GIC SPIs.
B.2) The ones like 10 139 140 - these are interesting, because we have
crossbar registers corresponding to these, However writing anything to them has no impact - at least 10 is confirmed to have been hardwired to L3_APP_IRQ (but not documented), we are trying to get explanations for 139 and 140. - but there is strong indication based on testing performed that the registers are NOPs and GIC is hardwired in.

I had originally discovered 10, but only a day or so back did we understand what is going on, others we dont know yet.
B.3) 133 is a variation to B.2 - There is an magical efuse register
which controls if the GIC is hardwired or not. when the efuse bit is 0, it behaves like B.1(program and it works), but almost all silicon have it set to "hardwired mode" :(

The following you wont find in any TRM, and is based on tests performed during the last few days - primarily meant to illustrate this.

MPU Crossbar
152 registers

   +-------+         +------+
   |       |    +----+C1    |
   | PPI.. |    |    +------+
   | 0..32 |    | <--+C2    |
   |       |    |    +------+     +------------+
   +-------+    | +--+C5    |     |            +---+
   |  SPI1 |    | |  +------+   <-+ L3 APP IRQ |   |
   |       |    | |  |      |     ++-------+---+   |
   +-------+    | |  |      |      +-------+       |
   |  SPI3 |    | |  +------+      | CPU0  |       |
   |       |    | |  |      |      | PMU   +----+  |
   +-------+    | |  +------+      +-------+    |  |
   | SPI4  | <--+ |  |      |                   |  |
   |       |      |  |      |                   |  |
   +-------+      |  |      |    +---------+    |  |
   | SPI10 | <----+  |      |    | External|    |  |
+> |       |         |      |    | NMI     |    |  |
|  +-------+         +------+    +-+-----+-++   |  |
|  | SPI131|         |      |      +-----+  |   |  |
|  |       | <+      +------+      | Efuse  |   |  |
|  +-------+  |      |C126  | <--+-----+-+  |   |  |
|  | SPI133| <---+   +------+    +-----+    |   |  |
|  +-------+  |  | +-+C132  |    |CPU0 |    |   |  |
|  | SPI139| <-----+ +------+    |WDT  |    |   |  |
|  +-------+  |  | | +------+    +--+--+    |   |  |
|  | ..... |  |  | |                |       |   |  |
|  +-------+  |  | +----------------+       |   |  |
|  | SPI159|  |  |                          |   |  |
|  +-------+  |  +--------------------------+   |  |
|             |                                 |  |
|    GIC      +---------------------------------+  |
|  160 SPI                                         |
|                                                  |
+--------------------------------------------------+

So, to answer your question - I hope this explains skip and reserved. Now, we happily can handle case B.1 (148 SPI interrupts) - However,

The reason I requested this series to be blocked is:
a) We dont completely (yet) have explanation about hardware for B.2 139 and 140.
b) we definitely need to be able to request the interrupts of A, B.2, B.3 - and our framework as it stands right now fails.

NOTE:
obviously we claim dra7 compatibility. dra742 and 744 seem similar - but we dont have confirmation for the same yet. following device tree maintainer recommendations of having dts compatibility closely match with SoC behavior. yeah, we could make the driver too generic and move everything to dts.. but that does not seem to be the way we do things with dt.

--
Regards,
Nishanth Menon

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 22 lines · Nishanth Menon

Quoted 8 lines · Joel Fernandes

Quoted 6 lines · Sricharan R

From: Nishanth Menon <[email protected]>

When, in the system due to varied reasons, interrupts might be unusable due to hardware behavior, but register maps do exist, then those interrupts should be skipped while mapping irq to crossbars.

Quoted 8 lines

Just wondering, instead of hardcoding this data in the code, and introducing additional flags (IRQ_SKIP), why not just put these GIC IRQs in the ti,irq-reserved property in DTS for platforms where such IRQs are not usable. That way you're skipping these IRQs anyway.

Also that would avoid adding more hard coded data for future SoCs into the source for such IRQs that must be skipped, and also reduces LOC.

Good question - lets try to explain the hardware a little here -> obviously a driver that cannot use the hardware is useless compared to reducing LOC count ;).. and apologies about the long reply..

Quoted 8 lines

Basic understanding:
GIC has 160 SPIs and number of hardware block interrupt sources is around or more than 400. So, in comes crossbar - which is basically a mapper by allowing us to select an hardware block interrupt source (identified as crossbar_number or cb_no in code). So all we have to do is to write to a register in crossbar corresponding to GIC and viola, we now routed the interrupt source to a GIC interrupt of our choice. At least the Specification reads so.... until you drill down to the details.

Thanks for the long explanation and the diagrams!

Yes, I feel there is no other way and with so many HW bugs, I think it makes sense to make it a real irqchip driver.

Further since not everything goes through the crossbar and some are direct mapped like your diagram, the correct fix is probably making it an irqchip and doing the interrupt controller parenting correctly in DT.

That would take care of A), because users of such direct mapped interrupts will go through the GIC interrupt controller directly.

It will also take care of B), because if writing to cross bar has no effect for a particular IRQ, or if those IRQs are hard-wired to something, as you said, then that something should go through the GIC directly.

I can try to whip up something like this if it makes sense, let me know...

Quoted 13 lines

Further since not everything goes through the crossbar and some are direct mapped like your diagram, the correct fix is probably making it an irqchip and doing the interrupt controller parenting correctly in DT.

That would take care of A), because users of such direct mapped interrupts will go through the GIC interrupt controller directly.

It will also take care of B), because if writing to cross bar has no effect for a particular IRQ, or if those IRQs are hard-wired to something, as you said, then that something should go through the GIC directly.

Quoted 2 lines

I can try to whip up something like this if it makes sense, let me know...

I have been ignoring this series considering they were just fixes but you comments are like re-inventing wheel. Please read all the old threads and comments from Thomas and me on why we took approach and why it is not an irqchip. There is no need to complicate it further.

On Thu, May 8, 2014 at 6:05 PM, Santosh Shilimkar
<[email protected]> wrote:
[..]

Quoted 13 lines

Quoted 13 lines

Further since not everything goes through the crossbar and some are direct mapped like your diagram, the correct fix is probably making it an irqchip and doing the interrupt controller parenting correctly in DT.

That would take care of A), because users of such direct mapped interrupts will go through the GIC interrupt controller directly.

It will also take care of B), because if writing to cross bar has no effect for a particular IRQ, or if those IRQs are hard-wired to something, as you said, then that something should go through the GIC directly.

Quoted 7 lines

Quoted 2 lines

I can try to whip up something like this if it makes sense, let me know...

I have been ignoring this series considering they were just fixes but you comments are like re-inventing wheel. Please read all the old threads and comments from Thomas and me on why we took approach and why it is not an irqchip. There is no need to complicate it further.

Are you talking about the discussion on this thread?
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-August/194318.html

I didn't really get a sense that there was a common agreement that irqchip is not the way to go there. I can stand corrected if there was a common consensus that irqchip is not the right solution (with any specific comments why). There was a concern in the thread that making it irqchip doesn't help dma reuse the infrastructure, but that concern seems moot now that the driver is proposed to live in drivers/irqchip.

The concern was really not where the code resides but what the actual hardware is and how can it fit into Linux. The whole reason I was actually against irqhcip from beginning of crossbar series was the hardware is not irqchip rather just a router. Thomas the formally NAKed that approach on thread [1]. If there are bugs, doesn't mean we can make fit the hardware into some subsystem where it can't be described.

Regards,
Santosh
[1] https://lkml.org/lkml/2013/9/13/413

On Thu, May 8, 2014 at 7:25 PM, Santosh Shilimkar
<[email protected]> wrote:

Ok, thanks for pointing to the post.

Quoted 2 lines · Joel Fernandes

On Thu, May 8, 2014 at 7:25 PM, Santosh Shilimkar
<[email protected]> wrote:

[...]

Quoted 2 lines

Ok, thanks for pointing to the post.

Yep - thanks Santosh for clarifying this. Now, we still have the issues that I pointed out in [1] - without resolving which, we should not enable crossbar for dra74x/72x.

A. taking example of PMU
interrupts = <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>
this wont work. instead the crossbar driver needs some sort of a hint to know that it should not map these on crossbar register instead assign GIC mapping directly.

I propose doing the following
#define GIC_CROSSBAR_PASSTHROUGH(irq_no) ((irq_no) | (0x1 << 31))

and dts will define the following:
interrupts = <GIC_SPI GIC_CROSSBAR_PASSTHROUGH(131) IRQ_TYPE_LEVEL_HIGH>

This will also work for the other cases (B.2, B.3)

For B.2: L3_APP_IRQ:
instead of:
interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>
we do:
interrupts = <GIC_SPI GIC_CROSSBAR_PASSTHROUGH(10) IRQ_TYPE_LEVEL_HIGH>

For B.3: NMI
interrupts = <GIC_SPI GIC_CROSSBAR_PASSTHROUGH(133) IRQ_TYPE_LEVEL_HIGH>

xlate is easy ->

drivers/irqchip/irq-crossbar.c +4 -0

index de021638..fd09ab4 100644
@@ -112,6 +112,10 @@ static int crossbar_domain_xlate(struct
 irq_domain *d,
 {
        unsigned long ret;
 
+       /* Check to see if direct GIC mapping is required */
+       if (intspec[1] & BIT(31))
+               return intspec[1] & ~BIT[31];
+
        ret = get_prev_map_irq(intspec[1]);
        if (!IS_ERR_VALUE(ret))

                goto found;

But then, crossbar_domain_map and crossbar_domain_unmap need hints as well to know that there is no corresponding crossbar registers. Have'nt thought through that yet. Looking to hear about opinions here.

Quoted 26 lines · Nishanth Menon

Quoted 2 lines · Joel Fernandes

On Thu, May 8, 2014 at 7:25 PM, Santosh Shilimkar
<[email protected]> wrote:

[...]

Quoted 2 lines

Ok, thanks for pointing to the post.

Yep - thanks Santosh for clarifying this. Now, we still have the issues that I pointed out in [1] - without resolving which, we should not enable crossbar for dra74x/72x.

A. taking example of PMU
interrupts = <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>
this wont work. instead the crossbar driver needs some sort of a hint to know that it should not map these on crossbar register instead assign GIC mapping directly.

I propose doing the following
#define GIC_CROSSBAR_PASSTHROUGH(irq_no) ((irq_no) | (0x1 << 31))

and dts will define the following:
interrupts = <GIC_SPI GIC_CROSSBAR_PASSTHROUGH(131) IRQ_TYPE_LEVEL_HIGH>

This will also work for the other cases (B.2, B.3)

Quoted 9 lines

For B.2: L3_APP_IRQ:
instead of:
interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>
we do:
interrupts = <GIC_SPI GIC_CROSSBAR_PASSTHROUGH(10) IRQ_TYPE_LEVEL_HIGH>

For B.3: NMI
interrupts = <GIC_SPI GIC_CROSSBAR_PASSTHROUGH(133) IRQ_TYPE_LEVEL_HIGH>

We can't do add a flag to generic interrupt controller flags since its very specific to cross-bar.

Quoted 20 lines

xlate is easy ->

drivers/irqchip/irq-crossbar.c +4 -0

index de021638..fd09ab4 100644
@@ -112,6 +112,10 @@ static int crossbar_domain_xlate(struct
 irq_domain *d,
 {
        unsigned long ret;
 
+       /* Check to see if direct GIC mapping is required */
+       if (intspec[1] & BIT(31))
+               return intspec[1] & ~BIT[31];
+
        ret = get_prev_map_irq(intspec[1]);
        if (!IS_ERR_VALUE(ret))

                goto found;

Quoted 5 lines

But then, crossbar_domain_map and crossbar_domain_unmap need hints as well to know that there is no corresponding crossbar registers. Have'nt thought through that yet. Looking to hear about opinions here.

May be we need additional property like reserved to take care of 1:1 map.

ti,irqs-direct-map = <131 132>;

Quoted 67 lines · Santosh Shilimkar

Quoted 26 lines · Nishanth Menon

Quoted 2 lines · Joel Fernandes

On Thu, May 8, 2014 at 7:25 PM, Santosh Shilimkar
<[email protected]> wrote:

[...]

Quoted 2 lines

Ok, thanks for pointing to the post.

Yep - thanks Santosh for clarifying this. Now, we still have the issues that I pointed out in [1] - without resolving which, we should not enable crossbar for dra74x/72x.

A. taking example of PMU
interrupts = <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>
this wont work. instead the crossbar driver needs some sort of a hint to know that it should not map these on crossbar register instead assign GIC mapping directly.

I propose doing the following
#define GIC_CROSSBAR_PASSTHROUGH(irq_no) ((irq_no) | (0x1 << 31))

and dts will define the following:
interrupts = <GIC_SPI GIC_CROSSBAR_PASSTHROUGH(131) IRQ_TYPE_LEVEL_HIGH>

This will also work for the other cases (B.2, B.3)

Quoted 9 lines

For B.2: L3_APP_IRQ:
instead of:
interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>
we do:
interrupts = <GIC_SPI GIC_CROSSBAR_PASSTHROUGH(10) IRQ_TYPE_LEVEL_HIGH>

For B.3: NMI
interrupts = <GIC_SPI GIC_CROSSBAR_PASSTHROUGH(133) IRQ_TYPE_LEVEL_HIGH>

We can't do add a flag to generic interrupt controller flags since its very specific to cross-bar.

Quoted 20 lines

xlate is easy ->

drivers/irqchip/irq-crossbar.c +4 -0

index de021638..fd09ab4 100644
@@ -112,6 +112,10 @@ static int crossbar_domain_xlate(struct
 irq_domain *d,
 {
        unsigned long ret;
 
+       /* Check to see if direct GIC mapping is required */
+       if (intspec[1] & BIT(31))
+               return intspec[1] & ~BIT[31];
+
        ret = get_prev_map_irq(intspec[1]);
        if (!IS_ERR_VALUE(ret))

                goto found;

Quoted 5 lines

But then, crossbar_domain_map and crossbar_domain_unmap need hints as well to know that there is no corresponding crossbar registers. Have'nt thought through that yet. Looking to hear about opinions here.

May be we need additional property like reserved to take care of 1:1 map.

Quoted 2 lines

ti,irqs-direct-map = <131 132>;

We already have equivalents for these -> reserved and skip. Problem is how does crossbar driver know the difference between direct maps and crossbar value?

6 is one of those reserved ones. dts for a device says:
interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>

Now, xlate gets intspec[1] = 6. 6 is valid crossbar number PRM_IRQ_MPU, however GIC 6 is mapped to WD_TIMER_MPU_C1_IRQ_WARN -> you need to be able to get a hint that this is direct mapping dts intended.

in the "6" example:

How do i get PRM_IRQ_MPU?
interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>

How do I get WD_TIMER_MPU_C1_IRQ_WARN?
interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH> ????? - that wont work as crossbar driver thinks it is crossbar 6 (PRM_IRQ_MPU)

Quoted 88 lines · Nishanth Menon

Quoted 67 lines · Santosh Shilimkar

Quoted 26 lines · Nishanth Menon

Quoted 2 lines · Joel Fernandes

On Thu, May 8, 2014 at 7:25 PM, Santosh Shilimkar
<[email protected]> wrote:

[...]

Quoted 2 lines

Ok, thanks for pointing to the post.

Yep - thanks Santosh for clarifying this. Now, we still have the issues that I pointed out in [1] - without resolving which, we should not enable crossbar for dra74x/72x.

A. taking example of PMU
interrupts = <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>
this wont work. instead the crossbar driver needs some sort of a hint to know that it should not map these on crossbar register instead assign GIC mapping directly.

I propose doing the following
#define GIC_CROSSBAR_PASSTHROUGH(irq_no) ((irq_no) | (0x1 << 31))

and dts will define the following:
interrupts = <GIC_SPI GIC_CROSSBAR_PASSTHROUGH(131) IRQ_TYPE_LEVEL_HIGH>

This will also work for the other cases (B.2, B.3)

Quoted 9 lines

For B.2: L3_APP_IRQ:
instead of:
interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>
we do:
interrupts = <GIC_SPI GIC_CROSSBAR_PASSTHROUGH(10) IRQ_TYPE_LEVEL_HIGH>

For B.3: NMI
interrupts = <GIC_SPI GIC_CROSSBAR_PASSTHROUGH(133) IRQ_TYPE_LEVEL_HIGH>

We can't do add a flag to generic interrupt controller flags since its very specific to cross-bar.

Quoted 20 lines

xlate is easy ->

drivers/irqchip/irq-crossbar.c +4 -0

index de021638..fd09ab4 100644
@@ -112,6 +112,10 @@ static int crossbar_domain_xlate(struct
 irq_domain *d,
 {
        unsigned long ret;
 
+       /* Check to see if direct GIC mapping is required */
+       if (intspec[1] & BIT(31))
+               return intspec[1] & ~BIT[31];
+
        ret = get_prev_map_irq(intspec[1]);
        if (!IS_ERR_VALUE(ret))

                goto found;

Quoted 5 lines

But then, crossbar_domain_map and crossbar_domain_unmap need hints as well to know that there is no corresponding crossbar registers. Have'nt thought through that yet. Looking to hear about opinions here.

May be we need additional property like reserved to take care of 1:1 map.

Quoted 2 lines

ti,irqs-direct-map = <131 132>;

We already have equivalents for these -> reserved and skip. Problem is how does crossbar driver know the difference between direct maps and crossbar value?

6 is one of those reserved ones. dts for a device says:
interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>

Now, xlate gets intspec[1] = 6. 6 is valid crossbar number PRM_IRQ_MPU, however GIC 6 is mapped to WD_TIMER_MPU_C1_IRQ_WARN -> you need to be able to get a hint that this is direct mapping dts intended.

in the "6" example:

How do i get PRM_IRQ_MPU?
interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>

Quoted 4 lines

How do I get WD_TIMER_MPU_C1_IRQ_WARN?
interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH> ????? - that wont work as crossbar driver thinks it is crossbar 6 (PRM_IRQ_MPU)

Looks like I am missing something. Is the issue because of SPI offset (32) which makes above confusion ?

Quoted 89 lines · Santosh Shilimkar

Quoted 88 lines · Nishanth Menon

Quoted 67 lines · Santosh Shilimkar

Quoted 26 lines · Nishanth Menon

Quoted 2 lines · Joel Fernandes

On Thu, May 8, 2014 at 7:25 PM, Santosh Shilimkar
<[email protected]> wrote:

[...]

Quoted 2 lines

Ok, thanks for pointing to the post.

Yep - thanks Santosh for clarifying this. Now, we still have the issues that I pointed out in [1] - without resolving which, we should not enable crossbar for dra74x/72x.

A. taking example of PMU
interrupts = <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>
this wont work. instead the crossbar driver needs some sort of a hint to know that it should not map these on crossbar register instead assign GIC mapping directly.

I propose doing the following
#define GIC_CROSSBAR_PASSTHROUGH(irq_no) ((irq_no) | (0x1 << 31))

and dts will define the following:
interrupts = <GIC_SPI GIC_CROSSBAR_PASSTHROUGH(131) IRQ_TYPE_LEVEL_HIGH>

This will also work for the other cases (B.2, B.3)

Quoted 9 lines

For B.2: L3_APP_IRQ:
instead of:
interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>
we do:
interrupts = <GIC_SPI GIC_CROSSBAR_PASSTHROUGH(10) IRQ_TYPE_LEVEL_HIGH>

For B.3: NMI
interrupts = <GIC_SPI GIC_CROSSBAR_PASSTHROUGH(133) IRQ_TYPE_LEVEL_HIGH>

We can't do add a flag to generic interrupt controller flags since its very specific to cross-bar.

Quoted 20 lines

xlate is easy ->

drivers/irqchip/irq-crossbar.c +4 -0

index de021638..fd09ab4 100644
@@ -112,6 +112,10 @@ static int crossbar_domain_xlate(struct
 irq_domain *d,
 {
        unsigned long ret;
 
+       /* Check to see if direct GIC mapping is required */
+       if (intspec[1] & BIT(31))
+               return intspec[1] & ~BIT[31];
+
        ret = get_prev_map_irq(intspec[1]);
        if (!IS_ERR_VALUE(ret))

                goto found;

Quoted 5 lines

But then, crossbar_domain_map and crossbar_domain_unmap need hints as well to know that there is no corresponding crossbar registers. Have'nt thought through that yet. Looking to hear about opinions here.

May be we need additional property like reserved to take care of 1:1 map.

Quoted 2 lines

ti,irqs-direct-map = <131 132>;

We already have equivalents for these -> reserved and skip. Problem is how does crossbar driver know the difference between direct maps and crossbar value?

6 is one of those reserved ones. dts for a device says:
interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>

Now, xlate gets intspec[1] = 6. 6 is valid crossbar number PRM_IRQ_MPU, however GIC 6 is mapped to WD_TIMER_MPU_C1_IRQ_WARN -> you need to be able to get a hint that this is direct mapping dts intended.

in the "6" example:

How do i get PRM_IRQ_MPU?
interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>

Quoted 6 lines

Quoted 4 lines

How do I get WD_TIMER_MPU_C1_IRQ_WARN?
interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH> ????? - that wont work as crossbar driver thinks it is crossbar 6 (PRM_IRQ_MPU)

Looks like I am missing something. Is the issue because of SPI offset (32) which makes above confusion ?

The way we modelled crossbar is that the irqchip is GIC and routable mapper is crossbar - which makes sense. now for every "interrupts" description we try to find a map using the crossbar driver as the irq framework rightly identifies that peripheral interrupts are routable and crossbar driver is the guy who knows how to map these to a proper GIC interrupt number. So far, we are good.

Now the trouble starts when our hardware description in dts assumes that every peripheral interrupt is a routable interrupt - as this thread describes - that is NOT the case.

Now the question is how do you differentiate?

interrupts = <GIC_SPI Number Level>

GIC_SPI is correct since we are attempting to describe the SPI interrupt (offset what ever it is, is a NOP from conceptual discussion).

Level is fine as well.

Number: what does this indicate? crossbar number is what we have assumed so far. however, then you loose the ability to describe interrupts on GIC SPI which dont have crossbar interrupts.

Question is how do we differentiate between the two?

Quoted 1 line · Santosh Shilimkar

On Friday 09 May 2014 09:36 AM, Nishanth Menon wrote:

Quoted 2 lines

Looks like I am missing something. Is the issue because of SPI offset (32) which makes above confusion ?

The way we modelled crossbar is that the irqchip is GIC and routable mapper is crossbar - which makes sense. now for every "interrupts" description we try to find a map using the crossbar driver as the irq framework rightly identifies that peripheral interrupts are routable and crossbar driver is the guy who knows how to map these to a proper GIC interrupt number. So far, we are good.

Now the trouble starts when our hardware description in dts assumes that every peripheral interrupt is a routable interrupt - as this thread describes - that is NOT the case.

Now the question is how do you differentiate?

interrupts = <GIC_SPI Number Level>

GIC_SPI is correct since we are attempting to describe the SPI interrupt (offset what ever it is, is a NOP from conceptual discussion).

Level is fine as well.

Number: what does this indicate? crossbar number is what we have assumed so far. however, then you loose the ability to describe interrupts on GIC SPI which dont have crossbar interrupts.

Question is how do we differentiate between the two?

Quoted 26 lines · Nishanth Menon

Quoted 2 lines · Joel Fernandes

On Thu, May 8, 2014 at 7:25 PM, Santosh Shilimkar
<[email protected]> wrote:

[...]

Quoted 2 lines

Ok, thanks for pointing to the post.

Yep - thanks Santosh for clarifying this. Now, we still have the issues that I pointed out in [1] - without resolving which, we should not enable crossbar for dra74x/72x.

A. taking example of PMU
interrupts = <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>
this wont work. instead the crossbar driver needs some sort of a hint to know that it should not map these on crossbar register instead assign GIC mapping directly.

I propose doing the following
#define GIC_CROSSBAR_PASSTHROUGH(irq_no) ((irq_no) | (0x1 << 31))

and dts will define the following:
interrupts = <GIC_SPI GIC_CROSSBAR_PASSTHROUGH(131) IRQ_TYPE_LEVEL_HIGH>

This will also work for the other cases (B.2, B.3)

Quoted 9 lines

For B.2: L3_APP_IRQ:
instead of:
interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>
we do:
interrupts = <GIC_SPI GIC_CROSSBAR_PASSTHROUGH(10) IRQ_TYPE_LEVEL_HIGH>

For B.3: NMI
interrupts = <GIC_SPI GIC_CROSSBAR_PASSTHROUGH(133) IRQ_TYPE_LEVEL_HIGH>

We can't do add a flag to generic interrupt controller flags since its very specific to cross-bar.

Quoted 20 lines

xlate is easy ->

drivers/irqchip/irq-crossbar.c +4 -0

index de021638..fd09ab4 100644
@@ -112,6 +112,10 @@ static int crossbar_domain_xlate(struct
 irq_domain *d,
 {
        unsigned long ret;
 
+       /* Check to see if direct GIC mapping is required */
+       if (intspec[1] & BIT(31))
+               return intspec[1] & ~BIT[31];
+
        ret = get_prev_map_irq(intspec[1]);
        if (!IS_ERR_VALUE(ret))

                goto found;

Quoted 5 lines

But then, crossbar_domain_map and crossbar_domain_unmap need hints as well to know that there is no corresponding crossbar registers. Have'nt thought through that yet. Looking to hear about opinions here.

May be we need additional property like reserved to take care of 1:1 map.

ti,irqs-direct-map = <131 132>;

Yep - thanks Santosh for clarifying this. Now, we still have the issues that I pointed out in [1] - without resolving which, we should not enable crossbar for dra74x/72x.

A. taking example of PMU
interrupts = <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>
this wont work. instead the crossbar driver needs some sort of a hint to know that it should not map these on crossbar register instead assign GIC mapping directly.

I propose doing the following
#define GIC_CROSSBAR_PASSTHROUGH(irq_no) ((irq_no) | (0x1 << 31))

and dts will define the following:
interrupts = <GIC_SPI GIC_CROSSBAR_PASSTHROUGH(131) IRQ_TYPE_LEVEL_HIGH>

This will also work for the other cases (B.2, B.3)

For B.2: L3_APP_IRQ:
instead of:
interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>
we do:
interrupts = <GIC_SPI GIC_CROSSBAR_PASSTHROUGH(10) IRQ_TYPE_LEVEL_HIGH>

For B.3: NMI
interrupts = <GIC_SPI GIC_CROSSBAR_PASSTHROUGH(133) IRQ_TYPE_LEVEL_HIGH>

xlate is easy ->

drivers/irqchip/irq-crossbar.c +0 -0

index de021638..fd09ab4 100644

Hi Nishanth,

On 05/09/2014 07:54 AM, Nishanth Menon wrote:
[..]

Quoted 10 lines

Yep - thanks Santosh for clarifying this. Now, we still have the issues that I pointed out in [1] - without resolving which, we should not enable crossbar for dra74x/72x.

A. taking example of PMU
interrupts = <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>
this wont work. instead the crossbar driver needs some sort of a hint to know that it should not map these on crossbar register instead assign GIC mapping directly.

Quoted 5 lines

I propose doing the following
#define GIC_CROSSBAR_PASSTHROUGH(irq_no) ((irq_no) | (0x1 << 31))

and dts will define the following:
interrupts = <GIC_SPI GIC_CROSSBAR_PASSTHROUGH(131) IRQ_TYPE_LEVEL_HIGH>

I would pick something smaller like GIC_SKIP_CROSSBAR.

Quoted 23 lines

This will also work for the other cases (B.2, B.3)

For B.2: L3_APP_IRQ:
instead of:
interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>
we do:
interrupts = <GIC_SPI GIC_CROSSBAR_PASSTHROUGH(10) IRQ_TYPE_LEVEL_HIGH>

For B.3: NMI
interrupts = <GIC_SPI GIC_CROSSBAR_PASSTHROUGH(133) IRQ_TYPE_LEVEL_HIGH>

xlate is easy ->

drivers/irqchip/irq-crossbar.c +0 -0

index de021638..fd09ab4 100644
@@ -112,6 +112,10 @@ static int crossbar_domain_xlate(struct
 irq_domain *d,
 {
        unsigned long ret;
 

drivers/irqchip/irq-crossbar.c static int crossbar_domain_xlate(struct · line 121

Quoted 6 lines

drivers/irqchip/irq-crossbar.c +4 -0

+       /* Check to see if direct GIC mapping is required */
+       if (intspec[1] & BIT(31))
+               return intspec[1] & ~BIT[31];
+
        ret = get_prev_map_irq(intspec[1]);
        if (!IS_ERR_VALUE(ret))

Sounds good, one problem I see here though is once you do the xlate, the information that the IRQ number is GIC cross bar is lost because you are 0'ing bit 31. Then how will map/unmap decide if it needs to be crossbar mapped/unmapped or GIC?

Hi Nishanth,

On 05/09/2014 07:54 AM, Nishanth Menon wrote:
[..]

Quoted 10 lines

Yep - thanks Santosh for clarifying this. Now, we still have the issues that I pointed out in [1] - without resolving which, we should not enable crossbar for dra74x/72x.

A. taking example of PMU
interrupts = <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>
this wont work. instead the crossbar driver needs some sort of a hint to know that it should not map these on crossbar register instead assign GIC mapping directly.

Quoted 5 lines

I propose doing the following
#define GIC_CROSSBAR_PASSTHROUGH(irq_no) ((irq_no) | (0x1 << 31))

and dts will define the following:
interrupts = <GIC_SPI GIC_CROSSBAR_PASSTHROUGH(131) IRQ_TYPE_LEVEL_HIGH>

I would pick something smaller like GIC_SKIP_CROSSBAR.

Quoted 19 lines

This will also work for the other cases (B.2, B.3)

For B.2: L3_APP_IRQ:
instead of:
interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>
we do:
interrupts = <GIC_SPI GIC_CROSSBAR_PASSTHROUGH(10) IRQ_TYPE_LEVEL_HIGH>

For B.3: NMI
interrupts = <GIC_SPI GIC_CROSSBAR_PASSTHROUGH(133) IRQ_TYPE_LEVEL_HIGH>

xlate is easy ->

drivers/irqchip/irq-crossbar.c +0 -0

index de021638..fd09ab4 100644

drivers/irqchip/irq-crossbar.c static int crossbar_domain_xlate(struct · line 121

Quoted 10 lines

drivers/irqchip/irq-crossbar.c +4 -0

@@ -112,6 +112,10 @@ static int crossbar_domain_xlate(struct
 irq_domain *d,
 {
        unsigned long ret;
 
+       /* Check to see if direct GIC mapping is required */
+       if (intspec[1] & BIT(31))
+               return intspec[1] & ~BIT[31];
+
        ret = get_prev_map_irq(intspec[1]);
        if (!IS_ERR_VALUE(ret))

Sounds good, one problem I see here though is once you do the xlate, the information that the IRQ number is GIC cross bar is lost because you are 0'ing bit 31. Then how will map/unmap decide if it needs to be crossbar mapped/unmapped or GIC?

Perhaps, the info in bit 31 should be stored somewhere and reused later during map time, or I am missing something.

These are fixes for the crossbar to handle two interrupts getting mapped twice to same crossbar and to skip some interrupts being used due to hardware bugs.

Nishanth Menon (5):
irqchip: crossbar: dont use '0' to mark reserved interrupts
irqchip: crossbar: check for premapped crossbar before allocating
irqchip: crossbar: Skip some irqs from getting mapped to crossbar
irqchip: crossbar: Initialise the crossbar with a safe value
irqchip: crossbar: Change allocation logic by reversing search for
free irqs

 drivers/irqchip/irq-crossbar.c |   82 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 74 insertions(+), 8 deletions(-)

Signature · 6 lines

1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

These are fixes for the crossbar to handle two interrupts getting mapped twice to same crossbar and to skip some interrupts being used due to hardware bugs.

Nishanth Menon (5):
irqchip: crossbar: dont use '0' to mark reserved interrupts
irqchip: crossbar: check for premapped crossbar before allocating
irqchip: crossbar: Skip some irqs from getting mapped to crossbar
irqchip: crossbar: Initialise the crossbar with a safe value
irqchip: crossbar: Change allocation logic by reversing search for
free irqs

 drivers/irqchip/irq-crossbar.c |   82 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 74 insertions(+), 8 deletions(-)