[PATCH 0/5] irqchip/dra7: crossbar bug fixes

[PATCH 0/5] irqchip/dra7: crossbar bug fixes

[Sricharan R]

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(-)

-- 
1.7.9.5

[PATCH 1/5] irqchip: crossbar: dont use '0' to mark reserved interrupts

[Sricharan R]

From: Nishanth Menon <nm@ti.com>

Today '0' is actually reserved, but may not be the same in the future.

So, use a flag to mark the GIC interrupts that are reserved.

Signed-off-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Sricharan R <r.sricharan@ti.com>
---
 drivers/irqchip/irq-crossbar.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
index 3d15d16..20105bc 100644
--- a/drivers/irqchip/irq-crossbar.c
+++ b/drivers/irqchip/irq-crossbar.c
@@ -17,6 +17,7 @@
 #include <linux/irqchip/arm-gic.h>
 
 #define IRQ_FREE	-1
+#define IRQ_RESERVED	-2
 #define GIC_IRQ_START	32
 
 /*
@@ -139,7 +140,7 @@ static int __init crossbar_of_init(struct device_node *node)
 				pr_err("Invalid reserved entry\n");
 				goto err3;
 			}
-			cb->irq_map[entry] = 0;
+			cb->irq_map[entry] = IRQ_RESERVED;
 		}
 	}
 
@@ -170,7 +171,7 @@ static int __init crossbar_of_init(struct device_node *node)
 	 * reserved irqs. so find and store the offsets once.
 	 */
 	for (i = 0; i < max; i++) {
-		if (!cb->irq_map[i])
+		if (cb->irq_map[i] == IRQ_RESERVED)
 			continue;
 
 		cb->register_offsets[i] = reserved;
-- 
1.7.9.5

[PATCH 2/5] irqchip: crossbar: check for premapped crossbar before allocating

[Sricharan R]

From: Nishanth Menon <nm@ti.com>

If irq_of_parse_and_map is executed twice, the same crossbar is mapped to two
different GIC interrupts. This is completely undesirable. Instead, check
if the requested crossbar event is pre-allocated and provide that GIC
mapping back to caller if already allocated.

Signed-off-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Sricharan R <r.sricharan@ti.com>
---
 drivers/irqchip/irq-crossbar.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
index 20105bc..51d4b87 100644
--- a/drivers/irqchip/irq-crossbar.c
+++ b/drivers/irqchip/irq-crossbar.c
@@ -51,6 +51,17 @@ static inline void crossbar_writeb(int irq_no, int cb_no)
 	writeb(cb_no, cb->crossbar_base + cb->register_offsets[irq_no]);
 }
 
+static inline int get_prev_map_irq(int cb_no)
+{
+	int i;
+
+	for (i = 0; i < cb->int_max; i++)
+		if (cb->irq_map[i] == cb_no)
+			return i;
+
+	return -ENODEV;
+}
+
 static inline int allocate_free_irq(int cb_no)
 {
 	int i;
@@ -88,11 +99,16 @@ static int crossbar_domain_xlate(struct irq_domain *d,
 {
 	unsigned long ret;
 
+	ret = get_prev_map_irq(intspec[1]);
+	if (!IS_ERR_VALUE(ret))
+		goto found;
+
 	ret = allocate_free_irq(intspec[1]);
 
 	if (IS_ERR_VALUE(ret))
 		return ret;
 
+found:
 	*out_hwirq = ret + GIC_IRQ_START;
 	return 0;
 }
-- 
1.7.9.5

[PATCH 3/5] irqchip: crossbar: Skip some irqs from getting mapped to crossbar

[Sricharan R]

From: Nishanth Menon <nm@ti.com>

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 <nm@ti.com>
Signed-off-by: Sricharan R <r.sricharan@ti.com>
---
 drivers/irqchip/irq-crossbar.c |   47 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 43 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
index 51d4b87..847f6e3 100644
--- a/drivers/irqchip/irq-crossbar.c
+++ b/drivers/irqchip/irq-crossbar.c
@@ -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;
 }
-- 
1.7.9.5

[PATCH 3/5] irqchip: crossbar: Skip some irqs from getting mapped to crossbar

[Joel Fernandes]

On 05/05/2014 09:18 AM, Sricharan R wrote:
> From: Nishanth Menon <nm@ti.com>
> 
> 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.

thanks,

-Joel


> Signed-off-by: Nishanth Menon <nm@ti.com>
> Signed-off-by: Sricharan R <r.sricharan@ti.com>
> ---
>  drivers/irqchip/irq-crossbar.c |   47 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 43 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
> index 51d4b87..847f6e3 100644
> --- a/drivers/irqchip/irq-crossbar.c
> +++ b/drivers/irqchip/irq-crossbar.c
> @@ -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;
>  }
> 

[PATCH 3/5] irqchip: crossbar: Skip some irqs from getting mapped to crossbar

[Nishanth Menon]

On 14:24-20140508, Joel Fernandes wrote:
> On 05/05/2014 09:18 AM, Sricharan R wrote:
> > From: Nishanth Menon <nm@ti.com>
> > 
> > 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.
> 

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

[PATCH 3/5] irqchip: crossbar: Skip some irqs from getting mapped to crossbar

[Joel Fernandes]

On Thu, May 8, 2014 at 3:37 PM, Nishanth Menon <nm@ti.com> wrote:
> On 14:24-20140508, Joel Fernandes wrote:
>> On 05/05/2014 09:18 AM, Sricharan R wrote:
>> > From: Nishanth Menon <nm@ti.com>
>> >
>> > 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.
>>
>
> 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.

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...

thanks,

-Joel


>
> 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

[PATCH 3/5] irqchip: crossbar: Skip some irqs from getting mapped to crossbar

[Santosh Shilimkar]

On Thursday 08 May 2014 06:43 PM, Joel Fernandes wrote:
> On Thu, May 8, 2014 at 3:37 PM, Nishanth Menon <nm@ti.com> wrote:
>> On 14:24-20140508, Joel Fernandes wrote:
>>> On 05/05/2014 09:18 AM, Sricharan R wrote:
>>>> From: Nishanth Menon <nm@ti.com>
>>>>
>>>> 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.
>>>
>>
>> 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.
> 
> 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.
>
It doesn't because its not an irqchip. 
 
> 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...
> 
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.


Regards,
Santosh

[PATCH 3/5] irqchip: crossbar: Skip some irqs from getting mapped to crossbar

[Joel Fernandes]

On Thu, May 8, 2014 at 6:05 PM, Santosh Shilimkar
<santosh.shilimkar@ti.com> wrote:
[..]
>> 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...
>>
> 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.

Further, I don't think my comments are re-inventing anything because
the brand-new bugs that are being found now weren't found then and my
comments were more related to these bugs.

As for complexity, it appears there will be signficant hacks coming up
that are required to handle the corner cases by adding more lists and
dt-properties. So its already on a complicated path.. Even with such
hacks, Nishanth pointed out that the implementation can break. Its
already like a deck of cards in my opinion and complicates things for
everyone using it. You said you hadn't gone through the fixes in this
series, you should go over them and see the new lists added to skip
and reserve various things.

thanks,

  -Joel

[PATCH 3/5] irqchip: crossbar: Skip some irqs from getting mapped to crossbar

[Santosh Shilimkar]

On Thursday 08 May 2014 08:13 PM, Joel Fernandes wrote:
> On Thu, May 8, 2014 at 6:05 PM, Santosh Shilimkar
> <santosh.shilimkar@ti.com> wrote:
> [..]
>>> 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...
>>>
>> 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.
> 
Obviously you haven't read all the threads... Please read [1]. There
was a reason I said read *all* the threads. Because anyone who looks
at this hardware IP block thinks it can be irqchip. You are not the
first one who said that.

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

[PATCH 3/5] irqchip: crossbar: Skip some irqs from getting mapped to crossbar

[Joel Fernandes]

On Thu, May 8, 2014 at 7:25 PM, Santosh Shilimkar
<santosh.shilimkar@ti.com> wrote:
[..]
> 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
>

Ok, thanks for pointing to the post.

[PATCH 3/5] irqchip: crossbar: Skip some irqs from getting mapped to crossbar

[Nishanth Menon]

On 05/08/2014 11:22 PM, Joel Fernandes wrote:
> On Thu, May 8, 2014 at 7:25 PM, Santosh Shilimkar
> <santosh.shilimkar@ti.com> wrote:
[...]
> 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 ->

diff --git a/drivers/irqchip/irq-crossbar.c
b/drivers/irqchip/irq-crossbar.c
index de021638..fd09ab4 100644
--- a/drivers/irqchip/irq-crossbar.c
+++ b/drivers/irqchip/irq-crossbar.c
@@ -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.



[1] http://marc.info/?l=linux-arm-kernel&m=139958155312421&w=2
-- 
Regards,
Nishanth Menon

[PATCH 3/5] irqchip: crossbar: Skip some irqs from getting mapped to crossbar

[Santosh Shilimkar]

On Friday 09 May 2014 08:54 AM, Nishanth Menon wrote:
> On 05/08/2014 11:22 PM, Joel Fernandes wrote:
>> On Thu, May 8, 2014 at 7:25 PM, Santosh Shilimkar
>> <santosh.shilimkar@ti.com> wrote:
> [...]
>> 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>
> 
We can't do add a flag to generic interrupt controller flags since its
very specific to cross-bar.

> xlate is easy ->
> 
> diff --git a/drivers/irqchip/irq-crossbar.c
> b/drivers/irqchip/irq-crossbar.c
> index de021638..fd09ab4 100644
> --- a/drivers/irqchip/irq-crossbar.c
> +++ b/drivers/irqchip/irq-crossbar.c
> @@ -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.
> 
> 
May be we need additional property like reserved to take care of 1:1
map.

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

[PATCH 3/5] irqchip: crossbar: Skip some irqs from getting mapped to crossbar

[Nishanth Menon]

On 05/09/2014 08:27 AM, Santosh Shilimkar wrote:
> On Friday 09 May 2014 08:54 AM, Nishanth Menon wrote:
>> On 05/08/2014 11:22 PM, Joel Fernandes wrote:
>>> On Thu, May 8, 2014 at 7:25 PM, Santosh Shilimkar
>>> <santosh.shilimkar@ti.com> wrote:
>> [...]
>>> 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>
>>
> We can't do add a flag to generic interrupt controller flags since its
> very specific to cross-bar.
> 
>> xlate is easy ->
>>
>> diff --git a/drivers/irqchip/irq-crossbar.c
>> b/drivers/irqchip/irq-crossbar.c
>> index de021638..fd09ab4 100644
>> --- a/drivers/irqchip/irq-crossbar.c
>> +++ b/drivers/irqchip/irq-crossbar.c
>> @@ -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.
>>
>>
> May be we need additional property like reserved to take care of 1:1
> map.
> 
> 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)

-- 
Regards,
Nishanth Menon

[PATCH 3/5] irqchip: crossbar: Skip some irqs from getting mapped to crossbar

[Santosh Shilimkar]

On Friday 09 May 2014 09:36 AM, Nishanth Menon wrote:
> On 05/09/2014 08:27 AM, Santosh Shilimkar wrote:
>> On Friday 09 May 2014 08:54 AM, Nishanth Menon wrote:
>>> On 05/08/2014 11:22 PM, Joel Fernandes wrote:
>>>> On Thu, May 8, 2014 at 7:25 PM, Santosh Shilimkar
>>>> <santosh.shilimkar@ti.com> wrote:
>>> [...]
>>>> 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>
>>>
>> We can't do add a flag to generic interrupt controller flags since its
>> very specific to cross-bar.
>>
>>> xlate is easy ->
>>>
>>> diff --git a/drivers/irqchip/irq-crossbar.c
>>> b/drivers/irqchip/irq-crossbar.c
>>> index de021638..fd09ab4 100644
>>> --- a/drivers/irqchip/irq-crossbar.c
>>> +++ b/drivers/irqchip/irq-crossbar.c
>>> @@ -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.
>>>
>>>
>> May be we need additional property like reserved to take care of 1:1
>> map.
>>
>> 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)
> 
Looks like I am missing something. Is the issue because of SPI offset (32)
which makes above confusion ?

[PATCH 3/5] irqchip: crossbar: Skip some irqs from getting mapped to crossbar

[Nishanth Menon]

On 05/09/2014 08:45 AM, Santosh Shilimkar wrote:
> On Friday 09 May 2014 09:36 AM, Nishanth Menon wrote:
>> On 05/09/2014 08:27 AM, Santosh Shilimkar wrote:
>>> On Friday 09 May 2014 08:54 AM, Nishanth Menon wrote:
>>>> On 05/08/2014 11:22 PM, Joel Fernandes wrote:
>>>>> On Thu, May 8, 2014 at 7:25 PM, Santosh Shilimkar
>>>>> <santosh.shilimkar@ti.com> wrote:
>>>> [...]
>>>>> 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>
>>>>
>>> We can't do add a flag to generic interrupt controller flags since its
>>> very specific to cross-bar.
>>>
>>>> xlate is easy ->
>>>>
>>>> diff --git a/drivers/irqchip/irq-crossbar.c
>>>> b/drivers/irqchip/irq-crossbar.c
>>>> index de021638..fd09ab4 100644
>>>> --- a/drivers/irqchip/irq-crossbar.c
>>>> +++ b/drivers/irqchip/irq-crossbar.c
>>>> @@ -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.
>>>>
>>>>
>>> May be we need additional property like reserved to take care of 1:1
>>> map.
>>>
>>> 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)
>>
> 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?



-- 
Regards,
Nishanth Menon

[PATCH 3/5] irqchip: crossbar: Skip some irqs from getting mapped to crossbar

[Joel Fernandes]

On 05/09/2014 09:00 AM, Nishanth Menon wrote:
> On 05/09/2014 08:45 AM, Santosh Shilimkar wrote:
>> On Friday 09 May 2014 09:36 AM, Nishanth Menon wrote:
>>> On 05/09/2014 08:27 AM, Santosh Shilimkar wrote:
>>>> On Friday 09 May 2014 08:54 AM, Nishanth Menon wrote:
>>>>> On 05/08/2014 11:22 PM, Joel Fernandes wrote:
>>>>>> On Thu, May 8, 2014 at 7:25 PM, Santosh Shilimkar
>>>>>> <santosh.shilimkar@ti.com> wrote:
>>>>> [...]
>>>>>> 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>
>>>>>
>>>> We can't do add a flag to generic interrupt controller flags since its
>>>> very specific to cross-bar.
>>>>
>>>>> xlate is easy ->
>>>>>
>>>>> diff --git a/drivers/irqchip/irq-crossbar.c
>>>>> b/drivers/irqchip/irq-crossbar.c
>>>>> index de021638..fd09ab4 100644
>>>>> --- a/drivers/irqchip/irq-crossbar.c
>>>>> +++ b/drivers/irqchip/irq-crossbar.c
>>>>> @@ -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.
>>>>>
>>>>>
>>>> May be we need additional property like reserved to take care of 1:1
>>>> map.
>>>>
>>>> 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)
>>>
>> 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?
> 

IMO even the direct-mapped ones can go come from a list, trouble is with
the buggy hardware hard-coded mappings which map some random crossbar to
some random GIC and can't be changed.

Only way to handle this is with a sort of an associative map in DT, not
a list. But that's far more overkill than anyone could imagine, and the
suggestion to set BIT 31 is cleaner without needing any lists at all,
while hiding all the complexity within the crossbar driver. After xlate,
the IRQ number is corrected, so the flag disappears, and the info that
its DIRECT can be stored to retrieved later during map/unmap.

thanks,

-Joel

[PATCH 3/5] irqchip: crossbar: Skip some irqs from getting mapped to crossbar

[Santosh Shilimkar]

On Friday 09 May 2014 10:00 AM, Nishanth Menon wrote:
> On 05/09/2014 08:45 AM, Santosh Shilimkar wrote:
>> On Friday 09 May 2014 09:36 AM, Nishanth Menon wrote:
[..]

>> 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?
> 
Updating the thread about an off-list discussion on $subject.
Nishant understood the how he was indirectly changing the meaning
of IRQ bindings and why its a bad idea. The point which
didn't came out clearly on the list was also the limited set
of route-able IRQ registers at cross-bar which is utter non-sense
and broken hardware. And since we are patching up the cross-bar
hardware bugs, it should be done such a way that the cross-bar
device tree bindings reflect that.

Here is the way forward we decided...
Add additional/optional properties to cross-bar binding
so that you can represent IRQs which can't be routed by
cross-bar. Something like 'MAX_ROUTABLE_IRQS'.

Then on the cleint modules which has the shortcoming, you just
add MAX_ROUTABLE_IRQS to those IRQs. That way cross-bar
driver fails to route them and just returns the offset
IRQline thinking it is already hard wired.

As a proof of concept I suggest you to just create a bidnig
patch with example usage of such case in the Documentation
and send it for review.

Copy Device Tree folks and Thomas to see if they are ok with it
or can suggest some better way to deal with the issue.

Thanks for the patience and explaining the issue repeatedly.

regards,
Santosh

[PATCH 3/5] irqchip: crossbar: Skip some irqs from getting mapped to crossbar

[Joel Fernandes]

On 05/09/2014 08:27 AM, Santosh Shilimkar wrote:
> On Friday 09 May 2014 08:54 AM, Nishanth Menon wrote:
>> On 05/08/2014 11:22 PM, Joel Fernandes wrote:
>>> On Thu, May 8, 2014 at 7:25 PM, Santosh Shilimkar
>>> <santosh.shilimkar@ti.com> wrote:
>> [...]
>>> 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>
>>
> We can't do add a flag to generic interrupt controller flags since its
> very specific to cross-bar.
> 
>> xlate is easy ->
>>
>> diff --git a/drivers/irqchip/irq-crossbar.c
>> b/drivers/irqchip/irq-crossbar.c
>> index de021638..fd09ab4 100644
>> --- a/drivers/irqchip/irq-crossbar.c
>> +++ b/drivers/irqchip/irq-crossbar.c
>> @@ -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.
>>
>>
> May be we need additional property like reserved to take care of 1:1
> map.
> 
> ti,irqs-direct-map = <131 132>;
> 

That wont work for cases where the cross bar has hardware bugs as
Nishanth pointed in his example.

For such cases, you need to differentiate between a "direct-map" GIC IRQ
and a regular working logical cross-bar number.

For example, what if IRQ no 10 has a crossbar bug? Does this mean you
will also make crossbar number (not IRQ no, logical cross bar no) also
suffer?

The way to fix this is like Nishanth pointed by introducing some
encoding or flag:

For someone using the logical cross bar, they would say:
interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>

and in the same dts, for someone else who is using the direct-mapped
buggy IRQ 10, they would use the the PASSTHROUGH flag:
interrupts = <GIC_SPI GIC_CROSSBAR_PASSTHROUGH(10) IRQ_TYPE_LEVEL_HIGH>

In this case, a "direct-map" list as you suggested doesn't work.

thanks,

-Joel

[PATCH 3/5] irqchip: crossbar: Skip some irqs from getting mapped to crossbar

[Joel Fernandes]

Hi Nishanth,

On 05/09/2014 07:54 AM, Nishanth Menon wrote:
[..]
> 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>

I would pick something smaller like GIC_SKIP_CROSSBAR.

> 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 ->
> 
> diff --git a/drivers/irqchip/irq-crossbar.c
> b/drivers/irqchip/irq-crossbar.c
> index de021638..fd09ab4 100644
> --- a/drivers/irqchip/irq-crossbar.c
> +++ b/drivers/irqchip/irq-crossbar.c
> @@ -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.

thanks,

-Joel

[PATCH 3/5] irqchip: crossbar: Skip some irqs from getting mapped to crossbar

[Joel Fernandes]

On 05/09/2014 08:36 AM, Joel Fernandes wrote:
> Hi Nishanth,
> 
> On 05/09/2014 07:54 AM, Nishanth Menon wrote:
> [..]
>> 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>
> 
> I would pick something smaller like GIC_SKIP_CROSSBAR.
> 
>> 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 ->
>>
>> diff --git a/drivers/irqchip/irq-crossbar.c
>> b/drivers/irqchip/irq-crossbar.c
>> index de021638..fd09ab4 100644
>> --- a/drivers/irqchip/irq-crossbar.c
>> +++ b/drivers/irqchip/irq-crossbar.c
>> @@ -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?
> 

Correcting myself here, I meant "IRQ number is GIC cross bar skipped".

[PATCH 3/5] irqchip: crossbar: Skip some irqs from getting mapped to crossbar

[Nishanth Menon]

On 05/09/2014 08:36 AM, Joel Fernandes wrote:
> Hi Nishanth,
> 
> On 05/09/2014 07:54 AM, Nishanth Menon wrote:
> [..]
>> 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>
> 
> I would pick something smaller like GIC_SKIP_CROSSBAR.
> 
>> 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 ->
>>
>> diff --git a/drivers/irqchip/irq-crossbar.c
>> b/drivers/irqchip/irq-crossbar.c
>> index de021638..fd09ab4 100644
>> --- a/drivers/irqchip/irq-crossbar.c
>> +++ b/drivers/irqchip/irq-crossbar.c
>> @@ -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.

no, you did not miss anything -> I did mention in my mail precisely
that "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."

Lets discuss hardware description problem(dts) first and then solve
the driver problem next.

-- 
Regards,
Nishanth Menon

[PATCH 4/5] irqchip: crossbar: Initialise the crossbar with a safe value

[Sricharan R]

From: Nishanth Menon <nm@ti.com>

Since crossbar is s/w configurable, the initial settings of the
crossbar cannot be assumed to be sane. This implies that:
a) On initialization all un-reserved crossbars must be initialized to
   a known 'safe' value.
b) When unmapping the interrupt, the safe value must be written to
   ensure that the crossbar mapping matches with interrupt controller
   usage.

So provide a safe value in the compatible data to map if
'0' is not safe for the platform and use it during init and unmap

Signed-off-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Sricharan R <r.sricharan@ti.com>
---
 drivers/irqchip/irq-crossbar.c |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
index 847f6e3..287d3ce 100644
--- a/drivers/irqchip/irq-crossbar.c
+++ b/drivers/irqchip/irq-crossbar.c
@@ -44,6 +44,7 @@ struct crossbar_device {
 struct crossbar_data {
 	const uint *irqs_unused;
 	const uint size;
+	const uint safe_map;
 };
 
 static struct crossbar_device *cb;
@@ -134,7 +135,7 @@ const struct irq_domain_ops routable_irq_domain_ops = {
 static int __init crossbar_of_init(struct device_node *node,
 				   const struct crossbar_data *data)
 {
-	int i, size, max, reserved = 0, entry;
+	int i, size, max, reserved = 0, entry, safe_map;
 	const __be32 *irqsr;
 	const int *irqsk = NULL;
 
@@ -212,6 +213,7 @@ static int __init crossbar_of_init(struct device_node *node,
 	if (data) {
 		irqsk = data->irqs_unused;
 		size = data->size;
+		safe_map = data->safe_map;
 
 		for (i = 0; i < size; i++) {
 			entry = irqsk[i];
@@ -222,6 +224,14 @@ static int __init crossbar_of_init(struct device_node *node,
 			}
 			cb->irq_map[entry] = IRQ_SKIP;
 		}
+
+		for (i = 0; i < max; i++) {
+			if (cb->irq_map[i] == IRQ_RESERVED ||
+			    cb->irq_map[i] == IRQ_SKIP)
+				continue;
+
+			cb->write(i, safe_map);
+		}
 	}
 
 	register_routable_domain_ops(&routable_irq_domain_ops);
@@ -241,7 +251,7 @@ err1:
 /* 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) };
+				     ARRAY_SIZE(dra_irqs_unused), 0 };
 
 static const struct of_device_id crossbar_match[] __initconst = {
 	{ .compatible = "ti,irq-crossbar", .data = &cb_dra_data },
-- 
1.7.9.5

[PATCH 5/5] irqchip: crossbar: Change allocation logic by reversing search for free irqs

[Sricharan R]

From: Nishanth Menon <nm@ti.com>

Reverse the search algorithm to ensure that address mapping and IRQ
allocation logics are proper. This can open up new bugs which are
easily fixable rather than wait till allocation logic approaches
the limit to find new bugs.

Signed-off-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Sricharan R <r.sricharan@ti.com>
---
 drivers/irqchip/irq-crossbar.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
index 287d3ce..de021638 100644
--- a/drivers/irqchip/irq-crossbar.c
+++ b/drivers/irqchip/irq-crossbar.c
@@ -68,7 +68,7 @@ static inline int get_prev_map_irq(int cb_no)
 {
 	int i;
 
-	for (i = 0; i < cb->int_max; i++)
+	for (i = cb->int_max - 1; i >= 0; i--)
 		if (cb->irq_map[i] == cb_no)
 			return i;
 
@@ -79,7 +79,7 @@ static inline int allocate_free_irq(int cb_no)
 {
 	int i;
 
-	for (i = 0; i < cb->int_max; i++) {
+	for (i = cb->int_max - 1; i >= 0; i--) {
 		if (cb->irq_map[i] == IRQ_FREE) {
 			cb->irq_map[i] = cb_no;
 			return i;
-- 
1.7.9.5

[PATCH 0/5] irqchip/dra7: crossbar bug fixes

[Darren Etheridge]

Sricharan R <r.sricharan@ti.com> wrote on Mon [2014-May-05 19:48:42 +0530]:
> 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
> 
Looks good!

I tested this patch series (plus the necessary dts changes) on a
DRA7xx-EVM with the work in progress VIP driver which needs a crossbar
mapping otherwise it won't get any interrupts (using 3.15-rc4 kernel).
It boots cleanly and interrupts are firing for VIP, so everything
working for me.

Without this patch series I can't even get the DRA7xx EVM to boot with
IRQ crossbar support enabled so this is a big improvement.

Tested-by: Darren Etheridge <detheridge@ti.com>


>  drivers/irqchip/irq-crossbar.c |   82 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 74 insertions(+), 8 deletions(-)
> 
> -- 
> 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

[PATCH 0/5] irqchip/dra7: crossbar bug fixes

[Tony Lindgren]

* Sricharan R <r.sricharan@ti.com> [140505 07:20]:
> 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(-)

Thanks applying all into omap-for-v3.16/crossbar.

Tony