This series does some cleanups, fixes for handling two interrupts
getting mapped twice to same crossbar and provides support for
hardwired IRQ and crossbar definitions.
On certain platforms such as DRA7, SPIs 0, 1, 2, 3, 5, 6, 10,
131, 132, 133 are direct wired to hardware blocks bypassing
crossbar. This quirky implementation is *NOT* supposed to be the
expectation of crossbar hardware usage. This series adds support
to represent such hard-wired irqs through DT and avoid generic
allocation/programming of crossbar in the driver.
This way of supporting hard-wired irqs was a result of
the below discussions.
http://www.spinics.net/lists/arm-kernel/msg329946.html
Based on 3.15 mainline.
All the patches are available here
[email protected]:Sricharanti/sricharan.git crossbar_updates
The fixes series[1] earlier posted is merged in to this.
[1] http://www.spinics.net/lists/arm-kernel/msg328273.html
[V2] Merged the above series and rebased on 3.15 mainline
Nishanth Menon (16):
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
irqchip: crossbar: remove IS_ERR_VALUE check
irqchip: crossbar: fix sparse warnings
irqchip: crossbar: fix checkpatch warning
irqchip: crossbar: fix kerneldoc warning
irqchip: crossbar: fix memory leak incase of invalid entry
irqchip: crossbar: return proper error value
irqchip: crossbar: change the goto naming
irqchip: crossbar: introduce ti,max-crossbar-sources to identify
valid crossbar mapping
irqchip: crossbar: introduce centralized check for crossbar write
Documentation: dt: OMAP: crossbar: add description for interrupt
consumer
irqchip: crossbar allow for quirky hardware with direct hardwiring of
GIC
Rajendra Nayak (1):
irqchip: crossbar: DRA7: Fix unused crossbar list
Sricharan R (2):
irqchip: crossbar: set cb pointer to null in case of error
irqchip: crossbar: Add kerneldoc for crossbar_domain_unmap callback
.../devicetree/bindings/arm/omap/crossbar.txt | 27 +++
drivers/irqchip/irq-crossbar.c | 193 +++++++++++++++++---
2 files changed, 191 insertions(+), 29 deletions(-)
--
1.7.9.5
From: Nishanth Menon <[email protected]>
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 <[email protected]>
Signed-off-by: Sricharan R <[email protected]>
Signed-off-by: Tony Lindgren <[email protected]>
---
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
From: Nishanth Menon <[email protected]>
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 <[email protected]>
Signed-off-by: Sricharan R <[email protected]>
Signed-off-by: Tony Lindgren <[email protected]>
---
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
From: Nishanth Menon <[email protected]>
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 <[email protected]>
Signed-off-by: Sricharan R <[email protected]>
Signed-off-by: Tony Lindgren <[email protected]>
---
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
From: Nishanth Menon <[email protected]>
There is absolutely no need for crossbar driver to expose functions and
variables into global namespace. So make them all static
Fixes sparse warnings:
drivers/irqchip/irq-crossbar.c:129:29: warning: symbol 'routable_irq_domain_ops' was not declared. Should it be static?
drivers/irqchip/irq-crossbar.c:252:5: warning: symbol 'dra_irqs_unused' was not declared. Should it be static?
drivers/irqchip/irq-crossbar.c:253:22: warning: symbol 'cb_dra_data' was not declared. Should it be static?
drivers/irqchip/irq-crossbar.c:261:12: warning: symbol 'irqcrossbar_init' was not declared. Should it be static?
Signed-off-by: Nishanth Menon <[email protected]>
---
drivers/irqchip/irq-crossbar.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
index c5415ae..5da9d36 100644
--- a/drivers/irqchip/irq-crossbar.c
+++ b/drivers/irqchip/irq-crossbar.c
@@ -16,6 +16,7 @@
#include <linux/of_device.h>
#include <linux/slab.h>
#include <linux/irqchip/arm-gic.h>
+#include <linux/irqchip/irq-crossbar.h>
#define IRQ_FREE -1
#define IRQ_RESERVED -2
@@ -126,7 +127,7 @@ found:
return 0;
}
-const struct irq_domain_ops routable_irq_domain_ops = {
+static const struct irq_domain_ops routable_irq_domain_ops = {
.map = crossbar_domain_map,
.unmap = crossbar_domain_unmap,
.xlate = crossbar_domain_xlate
@@ -249,8 +250,8 @@ 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,
+static int dra_irqs_unused[] = { 10 };
+static struct crossbar_data cb_dra_data = { dra_irqs_unused,
ARRAY_SIZE(dra_irqs_unused), 0 };
static const struct of_device_id crossbar_match[] __initconst = {
--
1.7.9.5
From: Nishanth Menon <[email protected]>
When the provided unused skip list entry is greater than max irqs
possible, we go to err3, but we fail to free register_offsets,
should have returned to err4 instead which ensures that allocated
register_offsets are freed as well.
Signed-off-by: Nishanth Menon <[email protected]>
---
drivers/irqchip/irq-crossbar.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
index 42a2e62..fea3e5d 100644
--- a/drivers/irqchip/irq-crossbar.c
+++ b/drivers/irqchip/irq-crossbar.c
@@ -225,7 +225,7 @@ static int __init crossbar_of_init(struct device_node *node,
if (entry > max) {
pr_err("Invalid skip entry\n");
- goto err3;
+ goto err4;
}
cb->irq_map[entry] = IRQ_SKIP;
}
--
1.7.9.5
The crossbar_domain_map/unmap callbacks need not be called same number
of times for a particular irq. But still use counting is not needed
here, because unmap(irq) gets called when irq_desc(irq) is disposed.
After this the irq is anyways unusable and have to mapped again.
Adding the above kerneldoc for unmap callback clarity.
Signed-off-by: Sricharan R <[email protected]>
---
drivers/irqchip/irq-crossbar.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
index 5f3e75a..2a73a66 100644
--- a/drivers/irqchip/irq-crossbar.c
+++ b/drivers/irqchip/irq-crossbar.c
@@ -101,6 +101,16 @@ static int crossbar_domain_map(struct irq_domain *d, unsigned int irq,
return 0;
}
+/**
+ * crossbar_domain_unmap - unmap a crossbar<->irq connection
+ * @d: domain of irq to unmap
+ * @irq: virq number
+ *
+ * The map/unmap callbacks need not be called same number of times for
+ * a particular irq. But still use counting is not needed here, because
+ * unmap(irq) gets called when irq_desc(irq) is disposed. After this the
+ * irq is anyways unusuable and have to be mapped again.
+ */
static void crossbar_domain_unmap(struct irq_domain *d, unsigned int irq)
{
irq_hw_number_t hw = irq_get_irq_data(irq)->hwirq;
--
1.7.9.5
From: Nishanth Menon <[email protected]>
The current crossbar description does not include the description
required for the consumer of the crossbar, a.k.a devices whoes events
pass through the crossbar into the GIC interrupt controller.
So, provide documentation for the same.
Signed-off-by: Nishanth Menon <[email protected]>
---
.../devicetree/bindings/arm/omap/crossbar.txt | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/Documentation/devicetree/bindings/arm/omap/crossbar.txt b/Documentation/devicetree/bindings/arm/omap/crossbar.txt
index 6d2e2f5..816d11b 100644
--- a/Documentation/devicetree/bindings/arm/omap/crossbar.txt
+++ b/Documentation/devicetree/bindings/arm/omap/crossbar.txt
@@ -27,3 +27,20 @@ Examples:
ti,reg-size = <2>;
ti,irqs-reserved = <0 1 2 3 5 6 131 132 139 140>;
};
+
+Consumer:
+========
+See Documentation/devicetree/bindings/interrupt-controller/interrupts.txt and
+Documentation/devicetree/bindings/arm/gic.txt for further details.
+
+An interrupt consumer on an SoC using crossbar will use:
+ interrupts = <GIC_SPI request_number interrupt_level>
+request number shall be between 0 to that described by
+"ti,max-crossbar-sources"
+
+Example:
+ device_x@0x4a023000 {
+ /* Crossbar 8 used */
+ interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
+ ...
+ };
--
1.7.9.5
From: Nishanth Menon <[email protected]>
On certain platforms such as DRA7, SPIs 0, 1, 2, 3, 5, 6, 10, 131,
132, 133 are direct wired to hardware blocks bypassing crossbar.
This quirky implementation is *NOT* supposed to be the expectation
of crossbar hardware usage. However, these are already marked in our
description of the hardware with SKIP and RESERVED where appropriate.
Unfortunately, we need to be able to refer to these hardwired IRQs.
So, to request these, crossbar driver can use the existing information
from it's table that these SKIP/RESERVED maps are direct wired sources
and generic allocation/programming of crossbar should be avoided.
Signed-off-by: Nishanth Menon <[email protected]>
---
.../devicetree/bindings/arm/omap/crossbar.txt | 12 ++++++++++--
drivers/irqchip/irq-crossbar.c | 20 ++++++++++++++++++--
2 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/arm/omap/crossbar.txt b/Documentation/devicetree/bindings/arm/omap/crossbar.txt
index 816d11b..7476d9b 100644
--- a/Documentation/devicetree/bindings/arm/omap/crossbar.txt
+++ b/Documentation/devicetree/bindings/arm/omap/crossbar.txt
@@ -35,8 +35,10 @@ Documentation/devicetree/bindings/arm/gic.txt for further details.
An interrupt consumer on an SoC using crossbar will use:
interrupts = <GIC_SPI request_number interrupt_level>
-request number shall be between 0 to that described by
-"ti,max-crossbar-sources"
+When the request number is between 0 to that described by
+"ti,max-crossbar-sources", it is assumed to be a crossbar mapping. If the
+request_number is greater than "ti,max-crossbar-sources", then it is mapped as a
+quirky hardware mapping direct to GIC.
Example:
device_x@0x4a023000 {
@@ -44,3 +46,9 @@ Example:
interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
...
};
+
+ device_y@0x4a033000 {
+ /* Direct mapped GIC SPI 1 used */
+ interrupts = <GIC_SPI 401 IRQ_TYPE_LEVEL_HIGH>;
+ ...
+ };
diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
index 745ad0a..2b61bbb 100644
--- a/drivers/irqchip/irq-crossbar.c
+++ b/drivers/irqchip/irq-crossbar.c
@@ -98,8 +98,13 @@ static inline int allocate_free_irq(int cb_no)
static inline bool needs_crossbar_write(irq_hw_number_t hw)
{
- if (hw > GIC_IRQ_START)
- return true;
+ int cb_no;
+
+ if (hw > GIC_IRQ_START) {
+ cb_no = cb->irq_map[hw - GIC_IRQ_START];
+ if (cb_no != IRQ_RESERVED && cb_no != IRQ_SKIP)
+ return true;
+ }
return false;
}
@@ -139,8 +144,19 @@ static int crossbar_domain_xlate(struct irq_domain *d,
{
int ret;
int req_num = intspec[1];
+ int direct_map_num;
if (req_num >= cb->max_crossbar_sources) {
+ direct_map_num = req_num - cb->max_crossbar_sources;
+ if (direct_map_num < cb->int_max) {
+ ret = cb->irq_map[direct_map_num];
+ if (ret == IRQ_RESERVED || ret == IRQ_SKIP) {
+ /* We use the interrupt num as h/w irq num */
+ ret = direct_map_num;
+ goto found;
+ }
+ }
+
pr_err("%s: requested crossbar number %d > max %d\n",
__func__, req_num, cb->max_crossbar_sources);
return -EINVAL;
--
1.7.9.5
From: Nishanth Menon <[email protected]>
This is a basic check to ensure that crossbar register needs to be
written. This ensures that we have a common check which is used in
both map and unmap logic.
Signed-off-by: Nishanth Menon <[email protected]>
---
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 cf69c4d..745ad0a 100644
--- a/drivers/irqchip/irq-crossbar.c
+++ b/drivers/irqchip/irq-crossbar.c
@@ -96,10 +96,20 @@ static inline int allocate_free_irq(int cb_no)
return -ENODEV;
}
+static inline bool needs_crossbar_write(irq_hw_number_t hw)
+{
+ if (hw > GIC_IRQ_START)
+ return true;
+
+ return false;
+}
+
static int crossbar_domain_map(struct irq_domain *d, unsigned int irq,
irq_hw_number_t hw)
{
- cb->write(hw - GIC_IRQ_START, cb->irq_map[hw - GIC_IRQ_START]);
+ if (needs_crossbar_write(hw))
+ cb->write(hw - GIC_IRQ_START, cb->irq_map[hw - GIC_IRQ_START]);
+
return 0;
}
@@ -117,7 +127,7 @@ static void crossbar_domain_unmap(struct irq_domain *d, unsigned int irq)
{
irq_hw_number_t hw = irq_get_irq_data(irq)->hwirq;
- if (hw > GIC_IRQ_START)
+ if (needs_crossbar_write(hw))
cb->irq_map[hw - GIC_IRQ_START] = IRQ_FREE;
}
--
1.7.9.5
From: Nishanth Menon <[email protected]>
Using err1,2,3,4 etc makes it hard to ensure a new exit path in the
middle will not result in spurious changes, so rename the error paths
as per the function it does.
Signed-off-by: Nishanth Menon <[email protected]>
---
drivers/irqchip/irq-crossbar.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
index 524e6e9..cf0d79f 100644
--- a/drivers/irqchip/irq-crossbar.c
+++ b/drivers/irqchip/irq-crossbar.c
@@ -152,17 +152,17 @@ static int __init crossbar_of_init(struct device_node *node,
cb->crossbar_base = of_iomap(node, 0);
if (!cb->crossbar_base)
- goto err1;
+ goto err_cb;
of_property_read_u32(node, "ti,max-irqs", &max);
if (!max) {
pr_err("missing 'ti,max-irqs' property\n");
ret = -EINVAL;
- goto err2;
+ goto err_base;
}
cb->irq_map = kcalloc(max, sizeof(int), GFP_KERNEL);
if (!cb->irq_map)
- goto err2;
+ goto err_base;
cb->int_max = max;
@@ -181,7 +181,7 @@ static int __init crossbar_of_init(struct device_node *node,
if (entry > max) {
pr_err("Invalid reserved entry\n");
ret = -EINVAL;
- goto err3;
+ goto err_irq_map;
}
cb->irq_map[entry] = IRQ_RESERVED;
}
@@ -189,7 +189,7 @@ static int __init crossbar_of_init(struct device_node *node,
cb->register_offsets = kcalloc(max, sizeof(int), GFP_KERNEL);
if (!cb->register_offsets)
- goto err3;
+ goto err_irq_map;
of_property_read_u32(node, "ti,reg-size", &size);
@@ -206,7 +206,7 @@ static int __init crossbar_of_init(struct device_node *node,
default:
pr_err("Invalid reg-size property\n");
ret = -EINVAL;
- goto err4;
+ goto err_reg_offset;
break;
}
@@ -234,7 +234,7 @@ static int __init crossbar_of_init(struct device_node *node,
if (entry > max) {
pr_err("Invalid skip entry\n");
ret = -EINVAL;
- goto err4;
+ goto err_reg_offset;
}
cb->irq_map[entry] = IRQ_SKIP;
}
@@ -251,13 +251,13 @@ static int __init crossbar_of_init(struct device_node *node,
register_routable_domain_ops(&routable_irq_domain_ops);
return 0;
-err4:
+err_reg_offset:
kfree(cb->register_offsets);
-err3:
+err_irq_map:
kfree(cb->irq_map);
-err2:
+err_base:
iounmap(cb->crossbar_base);
-err1:
+err_cb:
kfree(cb);
return ret;
}
--
1.7.9.5
From: Nishanth Menon <[email protected]>
Currently we attempt to map any crossbar value to an IRQ, however,
this is not correct from hardware perspective. There is a max crossbar
event number upto which hardware supports. So describe the same in
device tree using 'ti,max-crossbar-sources' property and use it to
validate requests.
Signed-off-by: Nishanth Menon <[email protected]>
---
.../devicetree/bindings/arm/omap/crossbar.txt | 2 ++
drivers/irqchip/irq-crossbar.c | 21 ++++++++++++++++++--
2 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/arm/omap/crossbar.txt b/Documentation/devicetree/bindings/arm/omap/crossbar.txt
index fb88585..6d2e2f5 100644
--- a/Documentation/devicetree/bindings/arm/omap/crossbar.txt
+++ b/Documentation/devicetree/bindings/arm/omap/crossbar.txt
@@ -10,6 +10,7 @@ Required properties:
- compatible : Should be "ti,irq-crossbar"
- reg: Base address and the size of the crossbar registers.
- ti,max-irqs: Total number of irqs available at the interrupt controller.
+- ti,max-crossbar-sources: Maximum number of crossbar sources that can be routed.
- ti,reg-size: Size of a individual register in bytes. Every individual
register is assumed to be of same size. Valid sizes are 1, 2, 4.
- ti,irqs-reserved: List of the reserved irq lines that are not muxed using
@@ -22,6 +23,7 @@ Examples:
compatible = "ti,irq-crossbar";
reg = <0x4a002a48 0x130>;
ti,max-irqs = <160>;
+ ti,max-crossbar-sources = <400>;
ti,reg-size = <2>;
ti,irqs-reserved = <0 1 2 3 5 6 131 132 139 140>;
};
diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
index 2a73a66..cf69c4d 100644
--- a/drivers/irqchip/irq-crossbar.c
+++ b/drivers/irqchip/irq-crossbar.c
@@ -26,6 +26,7 @@
/**
* struct crossbar_device - crossbar device descriptio
* @int_max: maximum number of supported interrupts
+ * @max_crossbar_sources: Maximum number of crossbar sources
* @irq_map: array of interrupts to crossbar number mapping
* @crossbar_base: crossbar base address
* @register_offsets: offsets for each irq number
@@ -33,6 +34,7 @@
*/
struct crossbar_device {
uint int_max;
+ uint max_crossbar_sources;
uint *irq_map;
void __iomem *crossbar_base;
int *register_offsets;
@@ -126,12 +128,19 @@ static int crossbar_domain_xlate(struct irq_domain *d,
unsigned int *out_type)
{
int ret;
+ int req_num = intspec[1];
- ret = get_prev_map_irq(intspec[1]);
+ if (req_num >= cb->max_crossbar_sources) {
+ pr_err("%s: requested crossbar number %d > max %d\n",
+ __func__, req_num, cb->max_crossbar_sources);
+ return -EINVAL;
+ }
+
+ ret = get_prev_map_irq(req_num);
if (ret >= 0)
goto found;
- ret = allocate_free_irq(intspec[1]);
+ ret = allocate_free_irq(req_num);
if (ret < 0)
return ret;
@@ -164,6 +173,14 @@ static int __init crossbar_of_init(struct device_node *node,
if (!cb->crossbar_base)
goto err_cb;
+ of_property_read_u32(node, "ti,max-crossbar-sources",
+ &cb->max_crossbar_sources);
+ if (!cb->max_crossbar_sources) {
+ pr_err("missing 'ti,max-crossbar-sources' property\n");
+ ret = -EINVAL;
+ goto err_base;
+ }
+
of_property_read_u32(node, "ti,max-irqs", &max);
if (!max) {
pr_err("missing 'ti,max-irqs' property\n");
--
1.7.9.5
From: Nishanth Menon <[email protected]>
crossbar_of_init always returns -ENOMEM in case of errors, return proper
error results in case of failures.
Signed-off-by: Nishanth Menon <[email protected]>
---
drivers/irqchip/irq-crossbar.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
index fea3e5d..524e6e9 100644
--- a/drivers/irqchip/irq-crossbar.c
+++ b/drivers/irqchip/irq-crossbar.c
@@ -140,20 +140,26 @@ static 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, safe_map;
+ int i, size, max = 0, reserved = 0, entry, safe_map;
const __be32 *irqsr;
const int *irqsk = NULL;
+ int ret = -ENOMEM;
cb = kzalloc(sizeof(*cb), GFP_KERNEL);
if (!cb)
- return -ENOMEM;
+ return ret;
cb->crossbar_base = of_iomap(node, 0);
if (!cb->crossbar_base)
goto err1;
of_property_read_u32(node, "ti,max-irqs", &max);
+ if (!max) {
+ pr_err("missing 'ti,max-irqs' property\n");
+ ret = -EINVAL;
+ goto err2;
+ }
cb->irq_map = kcalloc(max, sizeof(int), GFP_KERNEL);
if (!cb->irq_map)
goto err2;
@@ -174,6 +180,7 @@ static int __init crossbar_of_init(struct device_node *node,
i, &entry);
if (entry > max) {
pr_err("Invalid reserved entry\n");
+ ret = -EINVAL;
goto err3;
}
cb->irq_map[entry] = IRQ_RESERVED;
@@ -198,6 +205,7 @@ static int __init crossbar_of_init(struct device_node *node,
break;
default:
pr_err("Invalid reg-size property\n");
+ ret = -EINVAL;
goto err4;
break;
}
@@ -225,6 +233,7 @@ static int __init crossbar_of_init(struct device_node *node,
if (entry > max) {
pr_err("Invalid skip entry\n");
+ ret = -EINVAL;
goto err4;
}
cb->irq_map[entry] = IRQ_SKIP;
@@ -250,7 +259,7 @@ err2:
iounmap(cb->crossbar_base);
err1:
kfree(cb);
- return -ENOMEM;
+ return ret;
}
/* irq number 10,133,139 and 140 cannot be used because of hw bug */
--
1.7.9.5
If crossbar_of_init returns with a error, then set the cb pointer
to null.
Signed-off-by: Sricharan R <[email protected]>
---
drivers/irqchip/irq-crossbar.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
index cf0d79f..5f3e75a 100644
--- a/drivers/irqchip/irq-crossbar.c
+++ b/drivers/irqchip/irq-crossbar.c
@@ -259,6 +259,8 @@ err_base:
iounmap(cb->crossbar_base);
err_cb:
kfree(cb);
+
+ cb = NULL;
return ret;
}
--
1.7.9.5
From: Nishanth Menon <[email protected]>
kernel doc style is wrong in code. fix it to squelch
kerneldoc warnings:
Warning(drivers/irqchip/irq-crossbar.c:27): missing initial short description on line:
* struct crossbar_device: crossbar device description
Info(drivers/irqchip/irq-crossbar.c:27): Scanning doc for struct
Warning(drivers/irqchip/irq-crossbar.c:39): No description found for parameter 'write'
Warning(drivers/irqchip/irq-crossbar.c:42): missing initial short description on line:
* struct crossbar_data: Platform specific data
Info(drivers/irqchip/irq-crossbar.c:42): Scanning doc for struct
Warning(drivers/irqchip/irq-crossbar.c:50): No description found for parameter 'safe_map'
4 warnings
Signed-off-by: Nishanth Menon <[email protected]>
---
drivers/irqchip/irq-crossbar.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
index 58790d4..7d4db07 100644
--- a/drivers/irqchip/irq-crossbar.c
+++ b/drivers/irqchip/irq-crossbar.c
@@ -23,11 +23,13 @@
#define IRQ_SKIP -3
#define GIC_IRQ_START 32
-/*
+/**
+ * struct crossbar_device - crossbar device descriptio
* @int_max: maximum number of supported interrupts
* @irq_map: array of interrupts to crossbar number mapping
* @crossbar_base: crossbar base address
* @register_offsets: offsets for each irq number
+ * @write: register write function pointer
*/
struct crossbar_device {
uint int_max;
@@ -39,9 +41,10 @@ struct crossbar_device {
};
/**
- * struct crossbar_data: Platform specific data
+ * 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
+ * @safe_map: safe value to write to crossbar register
*/
struct crossbar_data {
const uint *irqs_unused;
--
1.7.9.5
From: Rajendra Nayak <[email protected]>
On DRA7 compatible IRQ crossbar, IRQ 10 default mapped to L3_APP_IRQ,
IRQ133 is default mapped to NMI pin, 139 and 140 crossbars are
unused(not routed). Mark these as unused crossbar IRQs.
Technical Reference Manual documentation update expected
DRA72x-TRMINC00067 is being used to track the update.
Reported-by: Nishanth Menon <[email protected]>
Reported-by: Sricharan R <[email protected]>
Reported-by: Suman Anna <[email protected]>
Signed-off-by: Rajendra Nayak <[email protected]>
---
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 7d4db07..42a2e62 100644
--- a/drivers/irqchip/irq-crossbar.c
+++ b/drivers/irqchip/irq-crossbar.c
@@ -253,8 +253,8 @@ err1:
return -ENOMEM;
}
-/* irq number 10 cannot be used because of hw bug */
-static int dra_irqs_unused[] = { 10 };
+/* irq number 10,133,139 and 140 cannot be used because of hw bug */
+static int dra_irqs_unused[] = { 10 , 133, 139, 140 };
static struct crossbar_data cb_dra_data = { dra_irqs_unused,
ARRAY_SIZE(dra_irqs_unused), 0 };
--
1.7.9.5
From: Nishanth Menon <[email protected]>
remove un-necessary space in function pointer.
Fixes checkpatch warning:
WARNING: Unnecessary space before function pointer arguments
#37: FILE: drivers/irqchip/irq-crossbar.c:37:
+ void (*write) (int, int);
WARNING: Missing a blank line after declarations
+ int *register_offsets;
+ void (*write)(int, int);
WARNING: Prefer kcalloc over kzalloc with multiply
+ cb->irq_map = kzalloc(max * sizeof(int), GFP_KERNEL);
WARNING: Prefer kcalloc over kzalloc with multiply
+ cb->register_offsets = kzalloc(max * sizeof(int), GFP_KERNEL);
Signed-off-by: Nishanth Menon <[email protected]>
---
drivers/irqchip/irq-crossbar.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
index 5da9d36..58790d4 100644
--- a/drivers/irqchip/irq-crossbar.c
+++ b/drivers/irqchip/irq-crossbar.c
@@ -34,7 +34,8 @@ struct crossbar_device {
uint *irq_map;
void __iomem *crossbar_base;
int *register_offsets;
- void (*write) (int, int);
+
+ void (*write)(int, int);
};
/**
@@ -150,7 +151,7 @@ static int __init crossbar_of_init(struct device_node *node,
goto err1;
of_property_read_u32(node, "ti,max-irqs", &max);
- cb->irq_map = kzalloc(max * sizeof(int), GFP_KERNEL);
+ cb->irq_map = kcalloc(max, sizeof(int), GFP_KERNEL);
if (!cb->irq_map)
goto err2;
@@ -176,7 +177,7 @@ static int __init crossbar_of_init(struct device_node *node,
}
}
- cb->register_offsets = kzalloc(max * sizeof(int), GFP_KERNEL);
+ cb->register_offsets = kcalloc(max, sizeof(int), GFP_KERNEL);
if (!cb->register_offsets)
goto err3;
--
1.7.9.5
From: Nishanth Menon <[email protected]>
IS_ERR_VALUE makes sense only *if* there could be valid values in
negative error range. But in the cases that we do use it, there is no
such case. Just remove the same.
Signed-off-by: Nishanth Menon <[email protected]>
---
drivers/irqchip/irq-crossbar.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
index de021638..c5415ae 100644
--- a/drivers/irqchip/irq-crossbar.c
+++ b/drivers/irqchip/irq-crossbar.c
@@ -110,15 +110,15 @@ static int crossbar_domain_xlate(struct irq_domain *d,
unsigned long *out_hwirq,
unsigned int *out_type)
{
- unsigned long ret;
+ int ret;
ret = get_prev_map_irq(intspec[1]);
- if (!IS_ERR_VALUE(ret))
+ if (ret >= 0)
goto found;
ret = allocate_free_irq(intspec[1]);
- if (IS_ERR_VALUE(ret))
+ if (ret < 0)
return ret;
found:
--
1.7.9.5
From: Nishanth Menon <[email protected]>
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 <[email protected]>
Signed-off-by: Sricharan R <[email protected]>
Signed-off-by: Tony Lindgren <[email protected]>
---
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
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]>
Signed-off-by: Tony Lindgren <[email protected]>
---
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
On Thu, Jun 12, 2014 at 05:23:11PM +0530, Sricharan R wrote:
> 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]>
> Signed-off-by: Tony Lindgren <[email protected]>
Tony, have you applied these somewhere already?
> ---
> 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 },
> {}
> };
This is a bug in all implementations of this IP? Or, a specific
SoC's implementation? Would this be better expressed in the dts via a
property? Can we expect future implementations to be fixed?
thx,
Jason.
On Thu, Jun 12, 2014 at 05:23:13PM +0530, Sricharan R wrote:
> From: Nishanth Menon <[email protected]>
>
> 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.
Could you expand on this logic some more? What class of bugs are you
hoping to discover more easily?
thx,
Jason.
> Signed-off-by: Nishanth Menon <[email protected]>
> Signed-off-by: Sricharan R <[email protected]>
> Signed-off-by: Tony Lindgren <[email protected]>
> ---
> 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
>
On Thu, Jun 12, 2014 at 05:23:16PM +0530, Sricharan R wrote:
> From: Nishanth Menon <[email protected]>
>
> remove un-necessary space in function pointer.
>
> Fixes checkpatch warning:
> WARNING: Unnecessary space before function pointer arguments
> #37: FILE: drivers/irqchip/irq-crossbar.c:37:
> + void (*write) (int, int);
>
> WARNING: Missing a blank line after declarations
> + int *register_offsets;
> + void (*write)(int, int);
>
> WARNING: Prefer kcalloc over kzalloc with multiply
> + cb->irq_map = kzalloc(max * sizeof(int), GFP_KERNEL);
>
> WARNING: Prefer kcalloc over kzalloc with multiply
> + cb->register_offsets = kzalloc(max * sizeof(int), GFP_KERNEL);
>
> Signed-off-by: Nishanth Menon <[email protected]>
> ---
> drivers/irqchip/irq-crossbar.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
> index 5da9d36..58790d4 100644
> --- a/drivers/irqchip/irq-crossbar.c
> +++ b/drivers/irqchip/irq-crossbar.c
> @@ -34,7 +34,8 @@ struct crossbar_device {
> uint *irq_map;
> void __iomem *crossbar_base;
> int *register_offsets;
> - void (*write) (int, int);
> +
> + void (*write)(int, int);
The empty line here looks bogus to me. Did you re-run checkpatch after
fixing the unnecessary space to see if it still complained about having
a 'blank line after declarations'?
> };
>
> /**
> @@ -150,7 +151,7 @@ static int __init crossbar_of_init(struct device_node *node,
> goto err1;
>
> of_property_read_u32(node, "ti,max-irqs", &max);
> - cb->irq_map = kzalloc(max * sizeof(int), GFP_KERNEL);
> + cb->irq_map = kcalloc(max, sizeof(int), GFP_KERNEL);
> if (!cb->irq_map)
> goto err2;
>
> @@ -176,7 +177,7 @@ static int __init crossbar_of_init(struct device_node *node,
> }
> }
>
> - cb->register_offsets = kzalloc(max * sizeof(int), GFP_KERNEL);
> + cb->register_offsets = kcalloc(max, sizeof(int), GFP_KERNEL);
> if (!cb->register_offsets)
> goto err3;
I'm generally opposed to these sorts of checkpatch patches, especially
when they are just warnings. It's great for a new driver in the staging
tree, but it makes backporting future bugfixes that much harder when
drivers have been live in mainline.
If, in the future, you're changing code in this area, go ahead and
convert to kcalloc(), but I wouldn't do a separate patch for this kind
of thing.
Honestly, I would just drop this patch and not worry about it.
thx,
Jason.
On Thu, Jun 12, 2014 at 05:23:17PM +0530, Sricharan R wrote:
> From: Nishanth Menon <[email protected]>
>
> kernel doc style is wrong in code. fix it to squelch
> kerneldoc warnings:
I would re-word the above to mention that we need to add missing
properties for kerneldoc (@write, @safemap), and we're doing some
cleanup of harmless warnings while we are here.
> Warning(drivers/irqchip/irq-crossbar.c:27): missing initial short description on line:
> * struct crossbar_device: crossbar device description
> Info(drivers/irqchip/irq-crossbar.c:27): Scanning doc for struct
> Warning(drivers/irqchip/irq-crossbar.c:39): No description found for parameter 'write'
> Warning(drivers/irqchip/irq-crossbar.c:42): missing initial short description on line:
> * struct crossbar_data: Platform specific data
> Info(drivers/irqchip/irq-crossbar.c:42): Scanning doc for struct
> Warning(drivers/irqchip/irq-crossbar.c:50): No description found for parameter 'safe_map'
> 4 warnings
>
> Signed-off-by: Nishanth Menon <[email protected]>
> ---
> drivers/irqchip/irq-crossbar.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
> index 58790d4..7d4db07 100644
> --- a/drivers/irqchip/irq-crossbar.c
> +++ b/drivers/irqchip/irq-crossbar.c
> @@ -23,11 +23,13 @@
> #define IRQ_SKIP -3
> #define GIC_IRQ_START 32
>
> -/*
> +/**
> + * struct crossbar_device - crossbar device descriptio
s/descriptio/description/
thx,
Jason.
> * @int_max: maximum number of supported interrupts
> * @irq_map: array of interrupts to crossbar number mapping
> * @crossbar_base: crossbar base address
> * @register_offsets: offsets for each irq number
> + * @write: register write function pointer
> */
> struct crossbar_device {
> uint int_max;
> @@ -39,9 +41,10 @@ struct crossbar_device {
> };
>
> /**
> - * struct crossbar_data: Platform specific data
> + * 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
> + * @safe_map: safe value to write to crossbar register
> */
> struct crossbar_data {
> const uint *irqs_unused;
> --
> 1.7.9.5
>
On Thu, Jun 12, 2014 at 05:23:18PM +0530, Sricharan R wrote:
> From: Rajendra Nayak <[email protected]>
>
> On DRA7 compatible IRQ crossbar, IRQ 10 default mapped to L3_APP_IRQ,
> IRQ133 is default mapped to NMI pin, 139 and 140 crossbars are
> unused(not routed). Mark these as unused crossbar IRQs.
>
> Technical Reference Manual documentation update expected
> DRA72x-TRMINC00067 is being used to track the update.
>
> Reported-by: Nishanth Menon <[email protected]>
> Reported-by: Sricharan R <[email protected]>
> Reported-by: Suman Anna <[email protected]>
> Signed-off-by: Rajendra Nayak <[email protected]>
> ---
> 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 7d4db07..42a2e62 100644
> --- a/drivers/irqchip/irq-crossbar.c
> +++ b/drivers/irqchip/irq-crossbar.c
> @@ -253,8 +253,8 @@ err1:
> return -ENOMEM;
> }
>
> -/* irq number 10 cannot be used because of hw bug */
> -static int dra_irqs_unused[] = { 10 };
> +/* irq number 10,133,139 and 140 cannot be used because of hw bug */
> +static int dra_irqs_unused[] = { 10 , 133, 139, 140 };
see my earlier comment regarding a potential dts property.
thx,
Jason.
> static struct crossbar_data cb_dra_data = { dra_irqs_unused,
> ARRAY_SIZE(dra_irqs_unused), 0 };
>
> --
> 1.7.9.5
>
On Thu, Jun 12, 2014 at 05:23:19PM +0530, Sricharan R wrote:
> From: Nishanth Menon <[email protected]>
>
> When the provided unused skip list entry is greater than max irqs
> possible, we go to err3, but we fail to free register_offsets,
> should have returned to err4 instead which ensures that allocated
s/returned/jumped/
> register_offsets are freed as well.
>
> Signed-off-by: Nishanth Menon <[email protected]>
> ---
> drivers/irqchip/irq-crossbar.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
> index 42a2e62..fea3e5d 100644
> --- a/drivers/irqchip/irq-crossbar.c
> +++ b/drivers/irqchip/irq-crossbar.c
> @@ -225,7 +225,7 @@ static int __init crossbar_of_init(struct device_node *node,
>
> if (entry > max) {
> pr_err("Invalid skip entry\n");
> - goto err3;
> + goto err4;
This would be a good opportunity to reduce the possibility of future
errors. Please consider renaming err{1,2,3,4} to something more
recognizable while you are here.
thx,
Jason.
> }
> cb->irq_map[entry] = IRQ_SKIP;
> }
> --
> 1.7.9.5
>
Hi Jason,
On Thursday 12 June 2014 06:21 PM, Jason Cooper wrote:
> On Thu, Jun 12, 2014 at 05:23:11PM +0530, Sricharan R wrote:
>> 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]>
>> Signed-off-by: Tony Lindgren <[email protected]>
>
> Tony, have you applied these somewhere already?
>
>> ---
>> 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 },
>> {}
>> };
>
> This is a bug in all implementations of this IP? Or, a specific
> SoC's implementation? Would this be better expressed in the dts via a
> property? Can we expect future implementations to be fixed?
>
> thx,
>
> Jason.
Infact this and PATCH#10 should be merged. I will change that.
So in Socs's (2 so far) that do have a crossbar, some irqs are mapped
through a crossbar and some are directly wired to the irqchip.
These 'unused irqs' are those which are directly wired but they still
have a crossbar register. Their routing cannot be changed. So this
is not really expected usage of the crossbar hw ip. We initially thought
having a dts property separately for this, but took this path to avoid
loading the dts with additional bindings which may not be generic.
Regards,
Sricharan
Hi Jason,
On Thursday 12 June 2014 06:46 PM, Jason Cooper wrote:
> On Thu, Jun 12, 2014 at 05:23:18PM +0530, Sricharan R wrote:
>> From: Rajendra Nayak <[email protected]>
>>
>> On DRA7 compatible IRQ crossbar, IRQ 10 default mapped to L3_APP_IRQ,
>> IRQ133 is default mapped to NMI pin, 139 and 140 crossbars are
>> unused(not routed). Mark these as unused crossbar IRQs.
>>
>> Technical Reference Manual documentation update expected
>> DRA72x-TRMINC00067 is being used to track the update.
>>
>> Reported-by: Nishanth Menon <[email protected]>
>> Reported-by: Sricharan R <[email protected]>
>> Reported-by: Suman Anna <[email protected]>
>> Signed-off-by: Rajendra Nayak <[email protected]>
>> ---
>> 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 7d4db07..42a2e62 100644
>> --- a/drivers/irqchip/irq-crossbar.c
>> +++ b/drivers/irqchip/irq-crossbar.c
>> @@ -253,8 +253,8 @@ err1:
>> return -ENOMEM;
>> }
>>
>> -/* irq number 10 cannot be used because of hw bug */
>> -static int dra_irqs_unused[] = { 10 };
>> +/* irq number 10,133,139 and 140 cannot be used because of hw bug */
>> +static int dra_irqs_unused[] = { 10 , 133, 139, 140 };
>
> see my earlier comment regarding a potential dts property.
>
Ok, if dts would be the better approach then I can change this.
Regards,
Sricharan
Hi Jason,
On Thursday 12 June 2014 06:40 PM, Jason Cooper wrote:
> On Thu, Jun 12, 2014 at 05:23:16PM +0530, Sricharan R wrote:
>> From: Nishanth Menon <[email protected]>
>>
>> remove un-necessary space in function pointer.
>>
>> Fixes checkpatch warning:
>> WARNING: Unnecessary space before function pointer arguments
>> #37: FILE: drivers/irqchip/irq-crossbar.c:37:
>> + void (*write) (int, int);
>>
>> WARNING: Missing a blank line after declarations
>> + int *register_offsets;
>> + void (*write)(int, int);
>>
>> WARNING: Prefer kcalloc over kzalloc with multiply
>> + cb->irq_map = kzalloc(max * sizeof(int), GFP_KERNEL);
>>
>> WARNING: Prefer kcalloc over kzalloc with multiply
>> + cb->register_offsets = kzalloc(max * sizeof(int), GFP_KERNEL);
>>
>> Signed-off-by: Nishanth Menon <[email protected]>
>> ---
>> drivers/irqchip/irq-crossbar.c | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
>> index 5da9d36..58790d4 100644
>> --- a/drivers/irqchip/irq-crossbar.c
>> +++ b/drivers/irqchip/irq-crossbar.c
>> @@ -34,7 +34,8 @@ struct crossbar_device {
>> uint *irq_map;
>> void __iomem *crossbar_base;
>> int *register_offsets;
>> - void (*write) (int, int);
>> +
>> + void (*write)(int, int);
>
> The empty line here looks bogus to me. Did you re-run checkpatch after
> fixing the unnecessary space to see if it still complained about having
> a 'blank line after declarations'?
>
Yes, it still complains even after fixing unnecessary space.
WARNING: Missing a blank line after declarations
#37: FILE: ./drivers/irqchip/irq-crossbar.c:37:
+ int *register_offsets;
+ void (*write)(int, int);
>> };
>>
>> /**
>> @@ -150,7 +151,7 @@ static int __init crossbar_of_init(struct device_node *node,
>> goto err1;
>>
>> of_property_read_u32(node, "ti,max-irqs", &max);
>> - cb->irq_map = kzalloc(max * sizeof(int), GFP_KERNEL);
>> + cb->irq_map = kcalloc(max, sizeof(int), GFP_KERNEL);
>> if (!cb->irq_map)
>> goto err2;
>>
>> @@ -176,7 +177,7 @@ static int __init crossbar_of_init(struct device_node *node,
>> }
>> }
>>
>> - cb->register_offsets = kzalloc(max * sizeof(int), GFP_KERNEL);
>> + cb->register_offsets = kcalloc(max, sizeof(int), GFP_KERNEL);
>> if (!cb->register_offsets)
>> goto err3;
>
> I'm generally opposed to these sorts of checkpatch patches, especially
> when they are just warnings. It's great for a new driver in the staging
> tree, but it makes backporting future bugfixes that much harder when
> drivers have been live in mainline.
>
> If, in the future, you're changing code in this area, go ahead and
> convert to kcalloc(), but I wouldn't do a separate patch for this kind
> of thing.
>
> Honestly, I would just drop this patch and not worry about it.
>
Ok, but i just hope that there may not be real needs to make changes
for this driver in future. So if that's the case then it might be
better to fix it once for now.
Regards,
Sricharan
On Thu, Jun 12, 2014 at 05:23:20PM +0530, Sricharan R wrote:
> From: Nishanth Menon <[email protected]>
>
> crossbar_of_init always returns -ENOMEM in case of errors, return proper
> error results in case of failures.
Please describe what the other possible failures are and how the changes
are more correct.
>
> Signed-off-by: Nishanth Menon <[email protected]>
> ---
> drivers/irqchip/irq-crossbar.c | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
> index fea3e5d..524e6e9 100644
> --- a/drivers/irqchip/irq-crossbar.c
> +++ b/drivers/irqchip/irq-crossbar.c
> @@ -140,20 +140,26 @@ static 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, safe_map;
> + int i, size, max = 0, reserved = 0, entry, safe_map;
> const __be32 *irqsr;
> const int *irqsk = NULL;
> + int ret = -ENOMEM;
>
> cb = kzalloc(sizeof(*cb), GFP_KERNEL);
>
> if (!cb)
> - return -ENOMEM;
> + return ret;
>
> cb->crossbar_base = of_iomap(node, 0);
> if (!cb->crossbar_base)
> goto err1;
>
> of_property_read_u32(node, "ti,max-irqs", &max);
> + if (!max) {
> + pr_err("missing 'ti,max-irqs' property\n");
> + ret = -EINVAL;
> + goto err2;
> + }
> cb->irq_map = kcalloc(max, sizeof(int), GFP_KERNEL);
> if (!cb->irq_map)
> goto err2;
> @@ -174,6 +180,7 @@ static int __init crossbar_of_init(struct device_node *node,
> i, &entry);
> if (entry > max) {
> pr_err("Invalid reserved entry\n");
> + ret = -EINVAL;
> goto err3;
> }
> cb->irq_map[entry] = IRQ_RESERVED;
> @@ -198,6 +205,7 @@ static int __init crossbar_of_init(struct device_node *node,
> break;
> default:
> pr_err("Invalid reg-size property\n");
> + ret = -EINVAL;
> goto err4;
> break;
> }
> @@ -225,6 +233,7 @@ static int __init crossbar_of_init(struct device_node *node,
>
> if (entry > max) {
> pr_err("Invalid skip entry\n");
> + ret = -EINVAL;
> goto err4;
> }
> cb->irq_map[entry] = IRQ_SKIP;
> @@ -250,7 +259,7 @@ err2:
> iounmap(cb->crossbar_base);
> err1:
> kfree(cb);
> - return -ENOMEM;
> + return ret;
> }
So, where is the change to the successful initialization? Shouldn't it
now set ret = 0?
This whole function seems to be a hodge-podge of different error
handling styles, which led to the memory leak fixed earlier in this
series.
Please go over this function, choose sane labels, and consolidate _all_
returns to the end of the function.
thx,
Jason.
Hi Jason,
On Thursday 12 June 2014 06:50 PM, Jason Cooper wrote:
> On Thu, Jun 12, 2014 at 05:23:19PM +0530, Sricharan R wrote:
>> From: Nishanth Menon <[email protected]>
>>
>> When the provided unused skip list entry is greater than max irqs
>> possible, we go to err3, but we fail to free register_offsets,
>> should have returned to err4 instead which ensures that allocated
>
> s/returned/jumped/
ok, will correct.
>
>> register_offsets are freed as well.
>>
>> Signed-off-by: Nishanth Menon <[email protected]>
>> ---
>> drivers/irqchip/irq-crossbar.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
>> index 42a2e62..fea3e5d 100644
>> --- a/drivers/irqchip/irq-crossbar.c
>> +++ b/drivers/irqchip/irq-crossbar.c
>> @@ -225,7 +225,7 @@ static int __init crossbar_of_init(struct device_node *node,
>>
>> if (entry > max) {
>> pr_err("Invalid skip entry\n");
>> - goto err3;
>> + goto err4;
>
> This would be a good opportunity to reduce the possibility of future
> errors. Please consider renaming err{1,2,3,4} to something more
> recognizable while you are here.
>
Ok, this is done as a part of patch#13.
Regards,
Sricharan
On Thu, Jun 12, 2014 at 05:23:21PM +0530, Sricharan R wrote:
> From: Nishanth Menon <[email protected]>
>
> Using err1,2,3,4 etc makes it hard to ensure a new exit path in the
> middle will not result in spurious changes, so rename the error paths
> as per the function it does.
>
> Signed-off-by: Nishanth Menon <[email protected]>
> ---
> drivers/irqchip/irq-crossbar.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
> index 524e6e9..cf0d79f 100644
> --- a/drivers/irqchip/irq-crossbar.c
> +++ b/drivers/irqchip/irq-crossbar.c
> @@ -152,17 +152,17 @@ static int __init crossbar_of_init(struct device_node *node,
>
> cb->crossbar_base = of_iomap(node, 0);
> if (!cb->crossbar_base)
> - goto err1;
> + goto err_cb;
>
> of_property_read_u32(node, "ti,max-irqs", &max);
> if (!max) {
> pr_err("missing 'ti,max-irqs' property\n");
> ret = -EINVAL;
> - goto err2;
> + goto err_base;
> }
> cb->irq_map = kcalloc(max, sizeof(int), GFP_KERNEL);
> if (!cb->irq_map)
> - goto err2;
> + goto err_base;
>
> cb->int_max = max;
>
> @@ -181,7 +181,7 @@ static int __init crossbar_of_init(struct device_node *node,
> if (entry > max) {
> pr_err("Invalid reserved entry\n");
> ret = -EINVAL;
> - goto err3;
> + goto err_irq_map;
> }
> cb->irq_map[entry] = IRQ_RESERVED;
> }
> @@ -189,7 +189,7 @@ static int __init crossbar_of_init(struct device_node *node,
>
> cb->register_offsets = kcalloc(max, sizeof(int), GFP_KERNEL);
> if (!cb->register_offsets)
> - goto err3;
> + goto err_irq_map;
>
> of_property_read_u32(node, "ti,reg-size", &size);
>
> @@ -206,7 +206,7 @@ static int __init crossbar_of_init(struct device_node *node,
> default:
> pr_err("Invalid reg-size property\n");
> ret = -EINVAL;
> - goto err4;
> + goto err_reg_offset;
> break;
> }
>
> @@ -234,7 +234,7 @@ static int __init crossbar_of_init(struct device_node *node,
> if (entry > max) {
> pr_err("Invalid skip entry\n");
> ret = -EINVAL;
> - goto err4;
> + goto err_reg_offset;
> }
> cb->irq_map[entry] = IRQ_SKIP;
> }
> @@ -251,13 +251,13 @@ static int __init crossbar_of_init(struct device_node *node,
> register_routable_domain_ops(&routable_irq_domain_ops);
> return 0;
>
> -err4:
> +err_reg_offset:
> kfree(cb->register_offsets);
> -err3:
> +err_irq_map:
> kfree(cb->irq_map);
> -err2:
> +err_base:
> iounmap(cb->crossbar_base);
> -err1:
> +err_cb:
> kfree(cb);
> return ret;
Ahhh :-) Ignore my previous request.
thx,
Jason.
Hi Jason,
On Thursday 12 June 2014 06:44 PM, Jason Cooper wrote:
> On Thu, Jun 12, 2014 at 05:23:17PM +0530, Sricharan R wrote:
>> From: Nishanth Menon <[email protected]>
>>
>> kernel doc style is wrong in code. fix it to squelch
>> kerneldoc warnings:
>
> I would re-word the above to mention that we need to add missing
> properties for kerneldoc (@write, @safemap), and we're doing some
> cleanup of harmless warnings while we are here.
>
Ok, will do this.
>> Warning(drivers/irqchip/irq-crossbar.c:27): missing initial short description on line:
>> * struct crossbar_device: crossbar device description
>> Info(drivers/irqchip/irq-crossbar.c:27): Scanning doc for struct
>> Warning(drivers/irqchip/irq-crossbar.c:39): No description found for parameter 'write'
>> Warning(drivers/irqchip/irq-crossbar.c:42): missing initial short description on line:
>> * struct crossbar_data: Platform specific data
>> Info(drivers/irqchip/irq-crossbar.c:42): Scanning doc for struct
>> Warning(drivers/irqchip/irq-crossbar.c:50): No description found for parameter 'safe_map'
>> 4 warnings
>>
>> Signed-off-by: Nishanth Menon <[email protected]>
>> ---
>> drivers/irqchip/irq-crossbar.c | 7 +++++--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
>> index 58790d4..7d4db07 100644
>> --- a/drivers/irqchip/irq-crossbar.c
>> +++ b/drivers/irqchip/irq-crossbar.c
>> @@ -23,11 +23,13 @@
>> #define IRQ_SKIP -3
>> #define GIC_IRQ_START 32
>>
>> -/*
>> +/**
>> + * struct crossbar_device - crossbar device descriptio
>
> s/descriptio/description/
Ok, will correct.
Regards,
Sricharan
On Thu, Jun 12, 2014 at 05:23:23PM +0530, Sricharan R wrote:
> The crossbar_domain_map/unmap callbacks need not be called same number
> of times for a particular irq. But still use counting is not needed
> here, because unmap(irq) gets called when irq_desc(irq) is disposed.
> After this the irq is anyways unusable and have to mapped again.
There's no need to duplicate the code comment in the commit log ;-)
>
> Adding the above kerneldoc for unmap callback clarity.
>
> Signed-off-by: Sricharan R <[email protected]>
> ---
> drivers/irqchip/irq-crossbar.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
> index 5f3e75a..2a73a66 100644
> --- a/drivers/irqchip/irq-crossbar.c
> +++ b/drivers/irqchip/irq-crossbar.c
> @@ -101,6 +101,16 @@ static int crossbar_domain_map(struct irq_domain *d, unsigned int irq,
> return 0;
> }
>
> +/**
> + * crossbar_domain_unmap - unmap a crossbar<->irq connection
> + * @d: domain of irq to unmap
> + * @irq: virq number
> + *
> + * The map/unmap callbacks need not be called same number of times for
s/same/the same/
> + * a particular irq. But still use counting is not needed here, because
> + * unmap(irq) gets called when irq_desc(irq) is disposed. After this the
> + * irq is anyways unusuable and have to be mapped again.
Please re-word this. If I were new to the driver (I am ;-) ) I would
have no idea what this means (and I don't :( )
thx,
Jason.
> + */
> static void crossbar_domain_unmap(struct irq_domain *d, unsigned int irq)
> {
> irq_hw_number_t hw = irq_get_irq_data(irq)->hwirq;
> --
> 1.7.9.5
>
Hi Jason,
On Thursday 12 June 2014 07:19 PM, Jason Cooper wrote:
> On Thu, Jun 12, 2014 at 05:23:23PM +0530, Sricharan R wrote:
>> The crossbar_domain_map/unmap callbacks need not be called same number
>> of times for a particular irq. But still use counting is not needed
>> here, because unmap(irq) gets called when irq_desc(irq) is disposed.
>> After this the irq is anyways unusable and have to mapped again.
>
> There's no need to duplicate the code comment in the commit log ;-)
Ok, will change this.
>
>>
>> Adding the above kerneldoc for unmap callback clarity.
>>
>> Signed-off-by: Sricharan R <[email protected]>
>> ---
>> drivers/irqchip/irq-crossbar.c | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
>> index 5f3e75a..2a73a66 100644
>> --- a/drivers/irqchip/irq-crossbar.c
>> +++ b/drivers/irqchip/irq-crossbar.c
>> @@ -101,6 +101,16 @@ static int crossbar_domain_map(struct irq_domain *d, unsigned int irq,
>> return 0;
>> }
>>
>> +/**
>> + * crossbar_domain_unmap - unmap a crossbar<->irq connection
>> + * @d: domain of irq to unmap
>> + * @irq: virq number
>> + *
>> + * The map/unmap callbacks need not be called same number of times for
>
> s/same/the same/
ok.
>
>> + * a particular irq. But still use counting is not needed here, because
>> + * unmap(irq) gets called when irq_desc(irq) is disposed. After this the
>> + * irq is anyways unusuable and have to be mapped again.
>
> Please re-word this. If I were new to the driver (I am ;-) ) I would
> have no idea what this means (and I don't :( )
>
hmm, will reword it better then..
Regards,
Sricharan
On Thu, Jun 12, 2014 at 05:23:24PM +0530, Sricharan R wrote:
> From: Nishanth Menon <[email protected]>
>
> Currently we attempt to map any crossbar value to an IRQ, however,
> this is not correct from hardware perspective. There is a max crossbar
> event number upto which hardware supports. So describe the same in
> device tree using 'ti,max-crossbar-sources' property and use it to
> validate requests.
>
> Signed-off-by: Nishanth Menon <[email protected]>
> ---
> .../devicetree/bindings/arm/omap/crossbar.txt | 2 ++
> drivers/irqchip/irq-crossbar.c | 21 ++++++++++++++++++--
> 2 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/arm/omap/crossbar.txt b/Documentation/devicetree/bindings/arm/omap/crossbar.txt
> index fb88585..6d2e2f5 100644
> --- a/Documentation/devicetree/bindings/arm/omap/crossbar.txt
> +++ b/Documentation/devicetree/bindings/arm/omap/crossbar.txt
> @@ -10,6 +10,7 @@ Required properties:
> - compatible : Should be "ti,irq-crossbar"
> - reg: Base address and the size of the crossbar registers.
> - ti,max-irqs: Total number of irqs available at the interrupt controller.
> +- ti,max-crossbar-sources: Maximum number of crossbar sources that can be routed.
> - ti,reg-size: Size of a individual register in bytes. Every individual
> register is assumed to be of same size. Valid sizes are 1, 2, 4.
> - ti,irqs-reserved: List of the reserved irq lines that are not muxed using
> @@ -22,6 +23,7 @@ Examples:
> compatible = "ti,irq-crossbar";
> reg = <0x4a002a48 0x130>;
> ti,max-irqs = <160>;
> + ti,max-crossbar-sources = <400>;
> ti,reg-size = <2>;
> ti,irqs-reserved = <0 1 2 3 5 6 131 132 139 140>;
> };
> diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
> index 2a73a66..cf69c4d 100644
> --- a/drivers/irqchip/irq-crossbar.c
> +++ b/drivers/irqchip/irq-crossbar.c
> @@ -26,6 +26,7 @@
> /**
> * struct crossbar_device - crossbar device descriptio
> * @int_max: maximum number of supported interrupts
> + * @max_crossbar_sources: Maximum number of crossbar sources
> * @irq_map: array of interrupts to crossbar number mapping
> * @crossbar_base: crossbar base address
> * @register_offsets: offsets for each irq number
> @@ -33,6 +34,7 @@
> */
> struct crossbar_device {
> uint int_max;
> + uint max_crossbar_sources;
> uint *irq_map;
> void __iomem *crossbar_base;
> int *register_offsets;
> @@ -126,12 +128,19 @@ static int crossbar_domain_xlate(struct irq_domain *d,
> unsigned int *out_type)
> {
> int ret;
> + int req_num = intspec[1];
>
> - ret = get_prev_map_irq(intspec[1]);
> + if (req_num >= cb->max_crossbar_sources) {
> + pr_err("%s: requested crossbar number %d > max %d\n",
> + __func__, req_num, cb->max_crossbar_sources);
> + return -EINVAL;
> + }
> +
> + ret = get_prev_map_irq(req_num);
> if (ret >= 0)
> goto found;
>
> - ret = allocate_free_irq(intspec[1]);
> + ret = allocate_free_irq(req_num);
>
> if (ret < 0)
> return ret;
> @@ -164,6 +173,14 @@ static int __init crossbar_of_init(struct device_node *node,
> if (!cb->crossbar_base)
> goto err_cb;
>
> + of_property_read_u32(node, "ti,max-crossbar-sources",
> + &cb->max_crossbar_sources);
> + if (!cb->max_crossbar_sources) {
> + pr_err("missing 'ti,max-crossbar-sources' property\n");
> + ret = -EINVAL;
> + goto err_base;
> + }
This completely breaks all boards using old dtbs. Please set
max_crossbar_sources to a sane value (400) when the property is missing.
> +
> of_property_read_u32(node, "ti,max-irqs", &max);
> if (!max) {
> pr_err("missing 'ti,max-irqs' property\n");
I know this is context, but you may want to look at this property as
well and set it to a sane value instead of erroring out.
thx,
Jason.
* Jason Cooper <[email protected]> [140612 05:52]:
> On Thu, Jun 12, 2014 at 05:23:11PM +0530, Sricharan R wrote:
> > 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]>
> > Signed-off-by: Tony Lindgren <[email protected]>
>
> Tony, have you applied these somewhere already?
I think some of these I had applied into a branch ready for
merging but then it was discovered that further changes
were needed and the branch was dropped.
Sricharan, please remove my Signed-off-by from this series.
If I end up applying it for merging my scripts will add it
automatically.
Regards,
Tony
On Thu, Jun 12, 2014 at 06:57:15AM -0700, Tony Lindgren wrote:
> * Jason Cooper <[email protected]> [140612 05:52]:
> > On Thu, Jun 12, 2014 at 05:23:11PM +0530, Sricharan R wrote:
> > > 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]>
> > > Signed-off-by: Tony Lindgren <[email protected]>
> >
> > Tony, have you applied these somewhere already?
>
> I think some of these I had applied into a branch ready for
> merging but then it was discovered that further changes
> were needed and the branch was dropped.
Ok.
> Sricharan, please remove my Signed-off-by from this series.
> If I end up applying it for merging my scripts will add it
> automatically.
Do you have other changes outside of irqchip depending on this series?
If so, I can set up a topic branch for you guys to base off of.
Otherwise, I'll just apply them to irqchip/core when they're ready.
Also, Sricharan, when you respin this, please clearly identify (in the
comment section) those patches that need to be flagged for stable. It
would be helpful if they were the first patches in the series as well.
thx,
Jason.
On Thu, Jun 12, 2014 at 06:49:17PM +0530, Sricharan R wrote:
> Hi Jason,
>
> On Thursday 12 June 2014 06:21 PM, Jason Cooper wrote:
> > On Thu, Jun 12, 2014 at 05:23:11PM +0530, Sricharan R wrote:
> >> 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]>
> >> Signed-off-by: Tony Lindgren <[email protected]>
> >
> > Tony, have you applied these somewhere already?
> >
> >> ---
> >> 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 },
> >> {}
> >> };
> >
> > This is a bug in all implementations of this IP? Or, a specific
> > SoC's implementation? Would this be better expressed in the dts via a
> > property? Can we expect future implementations to be fixed?
> >
> > thx,
> >
> > Jason.
> Infact this and PATCH#10 should be merged. I will change that.
>
> So in Socs's (2 so far) that do have a crossbar, some irqs are mapped
> through a crossbar and some are directly wired to the irqchip.
> These 'unused irqs' are those which are directly wired but they still
> have a crossbar register. Their routing cannot be changed. So this
> is not really expected usage of the crossbar hw ip. We initially thought
> having a dts property separately for this, but took this path to avoid
> loading the dts with additional bindings which may not be generic.
How do you plan to handle future SoCs with this IP and possibly
different hard-wired irqs?
thx,
Jason.
On Thu, 2014-06-12 at 19:05 +0530, Sricharan R wrote:
> On Thursday 12 June 2014 06:40 PM, Jason Cooper wrote:
> > On Thu, Jun 12, 2014 at 05:23:16PM +0530, Sricharan R wrote:
> >> diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
[]
> >> @@ -34,7 +34,8 @@ struct crossbar_device {
> >> uint *irq_map;
> >> void __iomem *crossbar_base;
> >> int *register_offsets;
> >> - void (*write) (int, int);
> >> +
> >> + void (*write)(int, int);
> >
> > The empty line here looks bogus to me.
Good eye. It's unnecessary.
> Did you re-run checkpatch after
> > fixing the unnecessary space to see if it still complained about having
> > a 'blank line after declarations'?
> >
> Yes, it still complains even after fixing unnecessary space.
It's a checkpatch defect.
It's been fixed by:
https://lkml.org/lkml/2014/6/6/426
> > I'm generally opposed to these sorts of checkpatch patches, especially
> > when they are just warnings. It's great for a new driver in the staging
> > tree, but it makes backporting future bugfixes that much harder when
> > drivers have been live in mainline.
Blind adherence to checkpatch isn't always a great idea.
But bugfix backports haven't been much of an issue in
other subsystems with fairly active whitespace/style
changes.
Hey Joe,
On Thu, Jun 12, 2014 at 07:18:31AM -0700, Joe Perches wrote:
> On Thu, 2014-06-12 at 19:05 +0530, Sricharan R wrote:
> > On Thursday 12 June 2014 06:40 PM, Jason Cooper wrote:
> > > On Thu, Jun 12, 2014 at 05:23:16PM +0530, Sricharan R wrote:
> > >> diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
> []
> > >> @@ -34,7 +34,8 @@ struct crossbar_device {
> > >> uint *irq_map;
> > >> void __iomem *crossbar_base;
> > >> int *register_offsets;
> > >> - void (*write) (int, int);
> > >> +
> > >> + void (*write)(int, int);
> > >
> > > The empty line here looks bogus to me.
>
> Good eye. It's unnecessary.
>
> > > Did you re-run checkpatch after fixing the unnecessary space to
> > > see if it still complained about having a 'blank line after
> > > declarations'?
> > >
> > Yes, it still complains even after fixing unnecessary space.
>
> It's a checkpatch defect.
>
> It's been fixed by:
> https://lkml.org/lkml/2014/6/6/426
Ah, good to know.
> > > I'm generally opposed to these sorts of checkpatch patches, especially
> > > when they are just warnings. It's great for a new driver in the staging
> > > tree, but it makes backporting future bugfixes that much harder when
> > > drivers have been live in mainline.
>
> Blind adherence to checkpatch isn't always a great idea.
Agreed.
> But bugfix backports haven't been much of an issue in
> other subsystems with fairly active whitespace/style
> changes.
Most of the mvebu fixes we've had that failed to apply were generally
due to a large whitespace change (dts node shuffling, admittedly not
checkpatch-related). I've also frequently been stymied by code cleanups
when using git blame to find the commit introducing a regression.
So, my general rule is: If you're submitting a patch to make checkpatch
be quiet, re-assess the need. If you're making changes and you can fix
some checkpatch items while you're there, then do so.
There are certainly legitimate checkpatch-only patches, I just don't
think this is one qualifies.
thx,
Jason.
On Thu, 2014-06-12 at 11:32 -0400, Jason Cooper wrote:
Hi Jason.
> > But bugfix backports haven't been much of an issue in
> > other subsystems with fairly active whitespace/style
> > changes.
>
> Most of the mvebu fixes we've had that failed to apply were generally
> due to a large whitespace change (dts node shuffling, admittedly not
> checkpatch-related).
So not due to this.
> I've also frequently been stymied by code cleanups
> when using git blame to find the commit introducing a regression.
git blame -w can frequently help there.
> So, my general rule is: If you're submitting a patch to make checkpatch
> be quiet, re-assess the need. If you're making changes and you can fix
> some checkpatch items while you're there, then do so.
Decent rule.
> There are certainly legitimate checkpatch-only patches, I just don't
> think this is one qualifies.
Of course it's the maintainer's choice (and last I saw,
that's you) to ignore whatever doesn't fit the appropriate
vision for the code.
$ ./scripts/get_maintainer.pl -f drivers/irqchip/irq-crossbar.c
Thomas Gleixner <[email protected]> (maintainer:IRQCHIP DRIVERS)
Jason Cooper <[email protected]> (maintainer:IRQCHIP DRIVERS)
...
cheers, Joe
Hi Jason,
On Thursday 12 June 2014 06:26 PM, Jason Cooper wrote:
> On Thu, Jun 12, 2014 at 05:23:13PM +0530, Sricharan R wrote:
>> From: Nishanth Menon <[email protected]>
>>
>> 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.
>
> Could you expand on this logic some more? What class of bugs are you
> hoping to discover more easily?
>
class 1. address space errors -> example:
reg = <a size_b>
ti,max-irqs = is a wrong parameter
class 2: irq-reserved list - which decides which entries in the
address space is not actually wired in
class 3: wrong list of routable-irqs.
in general allocating from max to min tends to have benefits in
ensuring the different issues that may be present in dts is easily
caught at definition time, rather than at a later point in time.
Regards,
Sricharan
Hi Jason,
On Thursday 12 June 2014 09:35 PM, Joe Perches wrote:
> On Thu, 2014-06-12 at 11:32 -0400, Jason Cooper wrote:
>
> Hi Jason.
>
>>> But bugfix backports haven't been much of an issue in
>>> other subsystems with fairly active whitespace/style
>>> changes.
>>
>> Most of the mvebu fixes we've had that failed to apply were generally
>> due to a large whitespace change (dts node shuffling, admittedly not
>> checkpatch-related).
>
> So not due to this.
>
>> I've also frequently been stymied by code cleanups
>> when using git blame to find the commit introducing a regression.
>
> git blame -w can frequently help there.
>
>> So, my general rule is: If you're submitting a patch to make checkpatch
>> be quiet, re-assess the need. If you're making changes and you can fix
>> some checkpatch items while you're there, then do so.
>
> Decent rule.
>
>> There are certainly legitimate checkpatch-only patches, I just don't
>> think this is one qualifies.
>
> Of course it's the maintainer's choice (and last I saw,
> that's you) to ignore whatever doesn't fit the appropriate
> vision for the code.
>
> $ ./scripts/get_maintainer.pl -f drivers/irqchip/irq-crossbar.c
> Thomas Gleixner <[email protected]> (maintainer:IRQCHIP DRIVERS)
> Jason Cooper <[email protected]> (maintainer:IRQCHIP DRIVERS)
Ok, if this is not qualifying as a separate patch then i will merge
this with other patches in the series which touch them.
Regards,
Sricharan
On Thursday 12 June 2014 07:27 PM, Tony Lindgren wrote:
> * Jason Cooper <[email protected]> [140612 05:52]:
>> On Thu, Jun 12, 2014 at 05:23:11PM +0530, Sricharan R wrote:
>>> 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]>
>>> Signed-off-by: Tony Lindgren <[email protected]>
>>
>> Tony, have you applied these somewhere already?
>
> I think some of these I had applied into a branch ready for
> merging but then it was discovered that further changes
> were needed and the branch was dropped.
>
> Sricharan, please remove my Signed-off-by from this series.
> If I end up applying it for merging my scripts will add it
> automatically.
>
Ok, will remove it.
Regards,
Sricharan
Hi Jason,
On Thursday 12 June 2014 07:37 PM, Jason Cooper wrote:
> On Thu, Jun 12, 2014 at 06:49:17PM +0530, Sricharan R wrote:
>> Hi Jason,
>>
>> On Thursday 12 June 2014 06:21 PM, Jason Cooper wrote:
>>> On Thu, Jun 12, 2014 at 05:23:11PM +0530, Sricharan R wrote:
>>>> 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]>
>>>> Signed-off-by: Tony Lindgren <[email protected]>
>>>
>>> Tony, have you applied these somewhere already?
>>>
>>>> ---
>>>> 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 },
>>>> {}
>>>> };
>>>
>>> This is a bug in all implementations of this IP? Or, a specific
>>> SoC's implementation? Would this be better expressed in the dts via a
>>> property? Can we expect future implementations to be fixed?
>>>
>>> thx,
>>>
>>> Jason.
>> Infact this and PATCH#10 should be merged. I will change that.
>>
>> So in Socs's (2 so far) that do have a crossbar, some irqs are mapped
>> through a crossbar and some are directly wired to the irqchip.
>> These 'unused irqs' are those which are directly wired but they still
>> have a crossbar register. Their routing cannot be changed. So this
>> is not really expected usage of the crossbar hw ip. We initially thought
>> having a dts property separately for this, but took this path to avoid
>> loading the dts with additional bindings which may not be generic.
>
> How do you plan to handle future SoCs with this IP and possibly
> different hard-wired irqs?
Yes, that would require adding a new compatible in the above list and dts.
So if adding a new binding in the dts would be cleaner, then i will change
it that way.
Regards,
Sricharan
Hi Jason,
On Thursday 12 June 2014 07:35 PM, Jason Cooper wrote:
> On Thu, Jun 12, 2014 at 06:57:15AM -0700, Tony Lindgren wrote:
>> * Jason Cooper <[email protected]> [140612 05:52]:
>>> On Thu, Jun 12, 2014 at 05:23:11PM +0530, Sricharan R wrote:
>>>> 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]>
>>>> Signed-off-by: Tony Lindgren <[email protected]>
>>>
>>> Tony, have you applied these somewhere already?
>>
>> I think some of these I had applied into a branch ready for
>> merging but then it was discovered that further changes
>> were needed and the branch was dropped.
>
> Ok.
>
>> Sricharan, please remove my Signed-off-by from this series.
>> If I end up applying it for merging my scripts will add it
>> automatically.
>
> Do you have other changes outside of irqchip depending on this series?
> If so, I can set up a topic branch for you guys to base off of.
> Otherwise, I'll just apply them to irqchip/core when they're ready.
>
There are dts changes which are dependent upon this series.
http://www.spinics.net/lists/linux-omap/msg108116.html
> Also, Sricharan, when you respin this, please clearly identify (in the
> comment section) those patches that need to be flagged for stable. It
> would be helpful if they were the first patches in the series as well.
Ok, i will point this out clearly.
Regards,
Sricharan
Hi Jason,
On Thursday 12 June 2014 07:24 PM, Jason Cooper wrote:
> On Thu, Jun 12, 2014 at 05:23:24PM +0530, Sricharan R wrote:
>> From: Nishanth Menon <[email protected]>
>>
>> Currently we attempt to map any crossbar value to an IRQ, however,
>> this is not correct from hardware perspective. There is a max crossbar
>> event number upto which hardware supports. So describe the same in
>> device tree using 'ti,max-crossbar-sources' property and use it to
>> validate requests.
>>
>> Signed-off-by: Nishanth Menon <[email protected]>
>> ---
>> .../devicetree/bindings/arm/omap/crossbar.txt | 2 ++
>> drivers/irqchip/irq-crossbar.c | 21 ++++++++++++++++++--
>> 2 files changed, 21 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/omap/crossbar.txt b/Documentation/devicetree/bindings/arm/omap/crossbar.txt
>> index fb88585..6d2e2f5 100644
>> --- a/Documentation/devicetree/bindings/arm/omap/crossbar.txt
>> +++ b/Documentation/devicetree/bindings/arm/omap/crossbar.txt
>> @@ -10,6 +10,7 @@ Required properties:
>> - compatible : Should be "ti,irq-crossbar"
>> - reg: Base address and the size of the crossbar registers.
>> - ti,max-irqs: Total number of irqs available at the interrupt controller.
>> +- ti,max-crossbar-sources: Maximum number of crossbar sources that can be routed.
>> - ti,reg-size: Size of a individual register in bytes. Every individual
>> register is assumed to be of same size. Valid sizes are 1, 2, 4.
>> - ti,irqs-reserved: List of the reserved irq lines that are not muxed using
>> @@ -22,6 +23,7 @@ Examples:
>> compatible = "ti,irq-crossbar";
>> reg = <0x4a002a48 0x130>;
>> ti,max-irqs = <160>;
>> + ti,max-crossbar-sources = <400>;
>> ti,reg-size = <2>;
>> ti,irqs-reserved = <0 1 2 3 5 6 131 132 139 140>;
>> };
>> diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
>> index 2a73a66..cf69c4d 100644
>> --- a/drivers/irqchip/irq-crossbar.c
>> +++ b/drivers/irqchip/irq-crossbar.c
>> @@ -26,6 +26,7 @@
>> /**
>> * struct crossbar_device - crossbar device descriptio
>> * @int_max: maximum number of supported interrupts
>> + * @max_crossbar_sources: Maximum number of crossbar sources
>> * @irq_map: array of interrupts to crossbar number mapping
>> * @crossbar_base: crossbar base address
>> * @register_offsets: offsets for each irq number
>> @@ -33,6 +34,7 @@
>> */
>> struct crossbar_device {
>> uint int_max;
>> + uint max_crossbar_sources;
>> uint *irq_map;
>> void __iomem *crossbar_base;
>> int *register_offsets;
>> @@ -126,12 +128,19 @@ static int crossbar_domain_xlate(struct irq_domain *d,
>> unsigned int *out_type)
>> {
>> int ret;
>> + int req_num = intspec[1];
>>
>> - ret = get_prev_map_irq(intspec[1]);
>> + if (req_num >= cb->max_crossbar_sources) {
>> + pr_err("%s: requested crossbar number %d > max %d\n",
>> + __func__, req_num, cb->max_crossbar_sources);
>> + return -EINVAL;
>> + }
>> +
>> + ret = get_prev_map_irq(req_num);
>> if (ret >= 0)
>> goto found;
>>
>> - ret = allocate_free_irq(intspec[1]);
>> + ret = allocate_free_irq(req_num);
>>
>> if (ret < 0)
>> return ret;
>> @@ -164,6 +173,14 @@ static int __init crossbar_of_init(struct device_node *node,
>> if (!cb->crossbar_base)
>> goto err_cb;
>>
>> + of_property_read_u32(node, "ti,max-crossbar-sources",
>> + &cb->max_crossbar_sources);
>> + if (!cb->max_crossbar_sources) {
>> + pr_err("missing 'ti,max-crossbar-sources' property\n");
>> + ret = -EINVAL;
>> + goto err_base;
>> + }
>
> This completely breaks all boards using old dtbs. Please set
> max_crossbar_sources to a sane value (400) when the property is missing.
>
>> +
>> of_property_read_u32(node, "ti,max-irqs", &max);
>> if (!max) {
>> pr_err("missing 'ti,max-irqs' property\n");
>
> I know this is context, but you may want to look at this property as
> well and set it to a sane value instead of erroring out.
>
crossbar dts node itself is not there in any dts yet. So this is not applicable
for any old boards. Any future dts with crossbar node should have this property
defined.
Regards,
Sricharan
On Friday 13 June 2014 12:26 PM, Sricharan R wrote:
> Hi Jason,
>
> On Thursday 12 June 2014 07:35 PM, Jason Cooper wrote:
>> On Thu, Jun 12, 2014 at 06:57:15AM -0700, Tony Lindgren wrote:
>>> * Jason Cooper <[email protected]> [140612 05:52]:
>>>> On Thu, Jun 12, 2014 at 05:23:11PM +0530, Sricharan R wrote:
>>>>> 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]>
>>>>> Signed-off-by: Tony Lindgren <[email protected]>
>>>>
>>>> Tony, have you applied these somewhere already?
>>>
>>> I think some of these I had applied into a branch ready for
>>> merging but then it was discovered that further changes
>>> were needed and the branch was dropped.
>>
>> Ok.
>>
>>> Sricharan, please remove my Signed-off-by from this series.
>>> If I end up applying it for merging my scripts will add it
>>> automatically.
>>
>> Do you have other changes outside of irqchip depending on this series?
>> If so, I can set up a topic branch for you guys to base off of.
>> Otherwise, I'll just apply them to irqchip/core when they're ready.
>>
> There are dts changes which are dependent upon this series.
>
> http://www.spinics.net/lists/linux-omap/msg108116.html
>
>> Also, Sricharan, when you respin this, please clearly identify (in the
>> comment section) those patches that need to be flagged for stable. It
>> would be helpful if they were the first patches in the series as well.
>
> Ok, i will point this out clearly.
Infact since the dts node is not present in the older kernel (even now),
the driver itself is not used. So i feel there is nothing to be flagged
for stable as such.
Regards,
Sricharan
On Fri, Jun 13, 2014 at 12:26:10PM +0530, Sricharan R wrote:
> On Thursday 12 June 2014 07:35 PM, Jason Cooper wrote:
...
> > Do you have other changes outside of irqchip depending on this series?
> > If so, I can set up a topic branch for you guys to base off of.
> > Otherwise, I'll just apply them to irqchip/core when they're ready.
> >
> There are dts changes which are dependent upon this series.
>
> http://www.spinics.net/lists/linux-omap/msg108116.html
In general, dts changes shouldn't depend on code changes or vice-versa.
If they do, that's an indicator that we're breaking compatibility with
older dtbs.
Looking at the dra7.dtsi changes, we're redefining the interrupt
property, which can't be good. :(
Perhaps a better solution would be to add a property, say 'ti,cross-irq'
that is the exact same format as 'interrupts', but is used by the
crossbar driver?
I'm not convinced of this yet, I suspect we may not actually have a
dependency between the dtsi changes and the code changes. We would have
the ugly "if you have the crossbar node, 'interrupts' means X, if not it
means Y" in the binding docs. But the absence of the node prevents the
crossbar driver from re-interpreting the interrupts property.
Have you tried booting all the different scenarios? eg:
old dtb, new driver
new dtb, old driver
old dtb, old driver
new dtb, new driver
thx,
Jason.
On Fri, Jun 13, 2014 at 11:39:39AM +0530, Sricharan R wrote:
> Hi Jason,
>
> On Thursday 12 June 2014 06:26 PM, Jason Cooper wrote:
> > On Thu, Jun 12, 2014 at 05:23:13PM +0530, Sricharan R wrote:
> >> From: Nishanth Menon <[email protected]>
> >>
> >> 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.
> >
> > Could you expand on this logic some more? What class of bugs are you
> > hoping to discover more easily?
> >
>
> class 1. address space errors -> example:
> reg = <a size_b>
> ti,max-irqs = is a wrong parameter
>
> class 2: irq-reserved list - which decides which entries in the
> address space is not actually wired in
>
> class 3: wrong list of routable-irqs.
>
> in general allocating from max to min tends to have benefits in
> ensuring the different issues that may be present in dts is easily
> caught at definition time, rather than at a later point in time.
Perfect, thanks for clarifying. Please add this to the commit log when
you respin.
thx,
Jason.
On Fri, Jun 13, 2014 at 12:00:31PM +0530, Sricharan R wrote:
> Ok, if this is not qualifying as a separate patch then i will merge
> this with other patches in the series which touch them.
A good general rule of thumb is to just run checkpatch on the patches,
not the source files. This way, we prevent new warnings from being
introduced, and we can cleanup stuff in the immediate vicinity to code
we are already changing.
thx,
Jason.
On Fri, Jun 13, 2014 at 04:24:52PM +0530, Sricharan R wrote:
> On Thursday 12 June 2014 07:24 PM, Jason Cooper wrote:
> > On Thu, Jun 12, 2014 at 05:23:24PM +0530, Sricharan R wrote:
...
> >> + of_property_read_u32(node, "ti,max-crossbar-sources",
> >> + &cb->max_crossbar_sources);
> >> + if (!cb->max_crossbar_sources) {
> >> + pr_err("missing 'ti,max-crossbar-sources' property\n");
> >> + ret = -EINVAL;
> >> + goto err_base;
> >> + }
> >
> > This completely breaks all boards using old dtbs. Please set
> > max_crossbar_sources to a sane value (400) when the property is missing.
> >
> >> +
> >> of_property_read_u32(node, "ti,max-irqs", &max);
> >> if (!max) {
> >> pr_err("missing 'ti,max-irqs' property\n");
> >
> > I know this is context, but you may want to look at this property as
> > well and set it to a sane value instead of erroring out.
> >
> crossbar dts node itself is not there in any dts yet. So this is not applicable
> for any old boards. Any future dts with crossbar node should have this property
> defined.
Now that I see the dra7.dtsi changes, I fully agree.
thx,
Jason.
On Fri, Jun 13, 2014 at 12:07:49PM +0530, Sricharan R wrote:
> Hi Jason,
>
> On Thursday 12 June 2014 07:37 PM, Jason Cooper wrote:
> > On Thu, Jun 12, 2014 at 06:49:17PM +0530, Sricharan R wrote:
> >> Hi Jason,
> >>
> >> On Thursday 12 June 2014 06:21 PM, Jason Cooper wrote:
> >>> On Thu, Jun 12, 2014 at 05:23:11PM +0530, Sricharan R wrote:
> >>>> 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]>
> >>>> Signed-off-by: Tony Lindgren <[email protected]>
> >>>
> >>> Tony, have you applied these somewhere already?
> >>>
> >>>> ---
> >>>> 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 },
> >>>> {}
> >>>> };
> >>>
> >>> This is a bug in all implementations of this IP? Or, a specific
> >>> SoC's implementation? Would this be better expressed in the dts via a
> >>> property? Can we expect future implementations to be fixed?
> >>>
> >>> thx,
> >>>
> >>> Jason.
> >> Infact this and PATCH#10 should be merged. I will change that.
> >>
> >> So in Socs's (2 so far) that do have a crossbar, some irqs are mapped
> >> through a crossbar and some are directly wired to the irqchip.
> >> These 'unused irqs' are those which are directly wired but they still
> >> have a crossbar register. Their routing cannot be changed. So this
> >> is not really expected usage of the crossbar hw ip. We initially thought
> >> having a dts property separately for this, but took this path to avoid
> >> loading the dts with additional bindings which may not be generic.
> >
> > How do you plan to handle future SoCs with this IP and possibly
> > different hard-wired irqs?
> Yes, that would require adding a new compatible in the above list and dts.
> So if adding a new binding in the dts would be cleaner, then i will change
> it that way.
Yes, unless the DT maintainers have shot the idea down, I'd prefer to
see a separate property. With the method you currently have, we'll have
to change the compatible when the IP _hasn't_ changed, just because the
SoC was wired differently.
We could trigger on the SoC compatible and maintain a table, but that
seems overly hacky when the dt is supposed to describe the hardware.
thx,
Jason.
On Friday 13 June 2014 09:10 AM, Jason Cooper wrote:
> On Fri, Jun 13, 2014 at 12:26:10PM +0530, Sricharan R wrote:
>> On Thursday 12 June 2014 07:35 PM, Jason Cooper wrote:
> ...
>>> Do you have other changes outside of irqchip depending on this series?
>>> If so, I can set up a topic branch for you guys to base off of.
>>> Otherwise, I'll just apply them to irqchip/core when they're ready.
>>>
>> There are dts changes which are dependent upon this series.
>>
>> http://www.spinics.net/lists/linux-omap/msg108116.html
>
> In general, dts changes shouldn't depend on code changes or vice-versa.
> If they do, that's an indicator that we're breaking compatibility with
> older dtbs.
>
Thats true. The case with cross-bar though is the feature wasn't
completly supported so far before this series. Perhaps the the initial
bindings should have been marked unstable.
> Looking at the dra7.dtsi changes, we're redefining the interrupt
> property, which can't be good. :(
>
> Perhaps a better solution would be to add a property, say 'ti,cross-irq'
> that is the exact same format as 'interrupts', but is used by the
> crossbar driver?
>
We have gone over those earlier and it was agreed to re-use interrupt
properties and for special cases, define a cross-bar property to describe
it.
> I'm not convinced of this yet, I suspect we may not actually have a
> dependency between the dtsi changes and the code changes. We would have
> the ugly "if you have the crossbar node, 'interrupts' means X, if not it
> means Y" in the binding docs. But the absence of the node prevents the
> crossbar driver from re-interpreting the interrupts property.
>
In ideal cross-bar hardware you don't need the assumption "if you have the
crossbar node, 'interrupts' means X, if not it means Y"
It is purely because the cross-bar irq router hardware has few nasty
bugs which needs those special handling. And thats the reason, the
property was added.
> Have you tried booting all the different scenarios? eg:
>
> old dtb, new driver
> new dtb, old driver
> old dtb, old driver
> new dtb, new driver
>
Old driver wasn't complete as mentioned and hence the above
combinations becomes bit irrelevant.
Regards,
Santosh
On Fri, Jun 13, 2014 at 09:35:20AM -0400, Santosh Shilimkar wrote:
> On Friday 13 June 2014 09:10 AM, Jason Cooper wrote:
...
> > Have you tried booting all the different scenarios? eg:
> >
> > old dtb, new driver
> > new dtb, old driver
> > old dtb, old driver
> > new dtb, new driver
> >
> Old driver wasn't complete as mentioned and hence the above
> combinations becomes bit irrelevant.
Ahh, great! In that case, I think we should be good without the
dependency between the code changes and the dtsi changes.
thx,
Jason.
On Thu, Jun 12, 2014 at 05:23:08PM +0530, Sricharan R wrote:
> This series does some cleanups, fixes for handling two interrupts
> getting mapped twice to same crossbar and provides support for
> hardwired IRQ and crossbar definitions.
>
> On certain platforms such as DRA7, SPIs 0, 1, 2, 3, 5, 6, 10,
> 131, 132, 133 are direct wired to hardware blocks bypassing
> crossbar. This quirky implementation is *NOT* supposed to be the
> expectation of crossbar hardware usage. This series adds support
> to represent such hard-wired irqs through DT and avoid generic
> allocation/programming of crossbar in the driver.
>
> This way of supporting hard-wired irqs was a result of
> the below discussions.
> http://www.spinics.net/lists/arm-kernel/msg329946.html
>
> Based on 3.15 mainline.
>
> All the patches are available here
> [email protected]:Sricharanti/sricharan.git crossbar_updates
>
> The fixes series[1] earlier posted is merged in to this.
> [1] http://www.spinics.net/lists/arm-kernel/msg328273.html
>
> [V2] Merged the above series and rebased on 3.15 mainline
>
> Nishanth Menon (16):
> 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
> irqchip: crossbar: remove IS_ERR_VALUE check
> irqchip: crossbar: fix sparse warnings
> irqchip: crossbar: fix checkpatch warning
> irqchip: crossbar: fix kerneldoc warning
> irqchip: crossbar: fix memory leak incase of invalid entry
> irqchip: crossbar: return proper error value
> irqchip: crossbar: change the goto naming
> irqchip: crossbar: introduce ti,max-crossbar-sources to identify
> valid crossbar mapping
> irqchip: crossbar: introduce centralized check for crossbar write
> Documentation: dt: OMAP: crossbar: add description for interrupt
> consumer
> irqchip: crossbar allow for quirky hardware with direct hardwiring of
> GIC
>
> Rajendra Nayak (1):
> irqchip: crossbar: DRA7: Fix unused crossbar list
>
> Sricharan R (2):
> irqchip: crossbar: set cb pointer to null in case of error
> irqchip: crossbar: Add kerneldoc for crossbar_domain_unmap callback
Also, I just noticed this when I decided to look at the history of this
driver: Please format the subject lines like so:
irqchip: crossbar: Set cb pointer ...
^
|
\-- note the capitalization
thx,
Jason.
On Fri, 2014-06-13 at 11:01 -0400, Jason Cooper wrote:
> Please format the subject lines like so:
>
> irqchip: crossbar: Set cb pointer ...
> ^
> |
> \-- note the capitalization
I suggest you don't make this a rule and focus
on more important stuff instead.
Nobody is going to write tools that parse the
changelogs by "git log --format=%s" for case
sensitivity.
As is, it's about half/half.
$ git log --format=%s --no-merges drivers/irqchip/ | \
grep "^irqchip:" | cut -f3- -d":"
On Fri, Jun 13, 2014 at 09:14:34AM -0700, Joe Perches wrote:
> On Fri, 2014-06-13 at 11:01 -0400, Jason Cooper wrote:
> > Please format the subject lines like so:
> >
> > irqchip: crossbar: Set cb pointer ...
> > ^
> > |
> > \-- note the capitalization
>
> I suggest you don't make this a rule and focus
> on more important stuff instead.
It's not my rule. ;-)
> Nobody is going to write tools that parse the
> changelogs by "git log --format=%s" for case
> sensitivity.
>
> As is, it's about half/half.
>
> $ git log --format=%s --no-merges drivers/irqchip/ | \
> grep "^irqchip:" | cut -f3- -d":"
Yeah, we haven't been perfect in the past, and a lot of stuff was merged
without going though tglx's tree. That's why I was asked to help out.
Either I'm fixing up the subject line as I apply the patch, or, I kindly
ask the submitter to fix it up during a re-spin.
thx,
Jason.
On Fri, 2014-06-13 at 12:37 -0400, Jason Cooper wrote:
> On Fri, Jun 13, 2014 at 09:14:34AM -0700, Joe Perches wrote:
> > On Fri, 2014-06-13 at 11:01 -0400, Jason Cooper wrote:
> > > Please format the subject lines like so:
> > >
> > > irqchip: crossbar: Set cb pointer ...
> > > ^
> > > |
> > > \-- note the capitalization
> >
> > I suggest you don't make this a rule and focus
> > on more important stuff instead.
>
> It's not my rule. ;-)
Who's rule is it then?
Documentation/SubmittingPatches simply says:
The canonical patch subject line is:
Subject: [PATCH 001/123] subsystem: summary phrase
It doesn't say anything about capitalization.
On Fri, Jun 13, 2014 at 09:48:24AM -0700, Joe Perches wrote:
> On Fri, 2014-06-13 at 12:37 -0400, Jason Cooper wrote:
> > On Fri, Jun 13, 2014 at 09:14:34AM -0700, Joe Perches wrote:
> > > On Fri, 2014-06-13 at 11:01 -0400, Jason Cooper wrote:
> > > > Please format the subject lines like so:
> > > >
> > > > irqchip: crossbar: Set cb pointer ...
> > > > ^
> > > > |
> > > > \-- note the capitalization
> > >
> > > I suggest you don't make this a rule and focus
> > > on more important stuff instead.
> >
> > It's not my rule. ;-)
>
> Who's rule is it then?
Thomas'
thx,
Jason.
On Fri, 13 Jun 2014, Jason Cooper wrote:
> On Fri, Jun 13, 2014 at 09:48:24AM -0700, Joe Perches wrote:
> > On Fri, 2014-06-13 at 12:37 -0400, Jason Cooper wrote:
> > > On Fri, Jun 13, 2014 at 09:14:34AM -0700, Joe Perches wrote:
> > > > On Fri, 2014-06-13 at 11:01 -0400, Jason Cooper wrote:
> > > > > Please format the subject lines like so:
> > > > >
> > > > > irqchip: crossbar: Set cb pointer ...
> > > > > ^
> > > > > |
> > > > > \-- note the capitalization
> > > >
> > > > I suggest you don't make this a rule and focus
> > > > on more important stuff instead.
Says the one, who pesters people about typos in changelogs....
WTF? Jason is doing a great job in reviewing and he knows what to
concentrate on.
> > > It's not my rule. ;-)
> >
> > Who's rule is it then?
>
> Thomas'
Sentences start with an upper case letter. Our brain is trained on
that rule when parsing a line.
So for people who actually review patches by reading them instead of
running a spell checker, consistent formatting more important than
avoiding the random typo, which our brain just blends out in most of
the cases. Unfortunately also when the typo is in actual code :(
Thanks,
tglx
On Fri, 2014-06-13 at 22:38 +0200, Thomas Gleixner wrote:
> On Fri, 13 Jun 2014, Jason Cooper wrote:
> > On Fri, Jun 13, 2014 at 09:48:24AM -0700, Joe Perches wrote:
> > > On Fri, 2014-06-13 at 12:37 -0400, Jason Cooper wrote:
> > > > On Fri, Jun 13, 2014 at 09:14:34AM -0700, Joe Perches wrote:
> > > > > On Fri, 2014-06-13 at 11:01 -0400, Jason Cooper wrote:
> > > > > > Please format the subject lines like so:
> > > > > >
> > > > > > irqchip: crossbar: Set cb pointer ...
> > > > > > ^
> > > > > > |
> > > > > > \-- note the capitalization
> > > > >
> > > > > I suggest you don't make this a rule and focus
> > > > > on more important stuff instead.
[elided the bit that describes what a patch subject looks like]
Documentation/SubmittingPatches simply says:
The canonical patch subject line is:
Subject: [PATCH 001/123] subsystem: summary phrase
It doesn't say anything about capitalization.
> Sentences start with an upper case letter. Our brain is trained on
> that rule when parsing a line.
<shrug> I don't think patch subjects are sentences.
The docs call them phrases.
> So for people who actually review patches by reading them instead of
> running a spell checker, consistent formatting more important than
> avoiding the random typo, which our brain just blends out in most of
> the cases. Unfortunately also when the typo is in actual code :(
That part about the code is truth.
Anyway, how you spend your time is certainly up to you.
Do what makes you happy.
But if you want this specific form for your patches,
please just document it somewhere in the kernel tree.
I think that relatively commit log subjects are generally
easy to parse as-is and don't need more strictures.
I think it akin to british/american spelling uses and
I and i. I just don't care which people use.
I did propose a mechanism to nudge people when proposed
patch subjects don't fit some specific maintainer's idea
of proper.
https://lkml.org/lkml/2010/11/16/245
cheers, Joe