Freescale MPIC h/w may not support all interrupt sources reported
by hardware or "last-interrupt-source" or platform. On these platforms
a misconfigured device tree that assigns one of the reserved
interrupts leaves a non-functioning system without warning.
First Patch just moves the last-irq calculation logic to a function,
Second patch reworks same logic, While I feel that device-tree should
get precedence over platform provided last-irq, but in this series
I have not changed this logic.
Third and fourth patch add non-contiguous interrupt sources support
Fifth patch enables this for P2020RDB-PC for now.
Bharat Bhushan (5):
powerpc/mpic: move last irq logic to function
powerpc/mpic: Rework last source irq calculation logic
powerpc/mpic: Add support for non-contiguous irq ranges
powerpc/mpic: Boot print supported interrupt ranges
powerpc/fsl: Add supported-irq-ranges for P2020
.../devicetree/bindings/powerpc/fsl/mpic.txt | 8 +
arch/powerpc/boot/dts/fsl/p2020si-post.dtsi | 3 +
arch/powerpc/include/asm/mpic.h | 9 +
arch/powerpc/platforms/85xx/mpc85xx_rdb.c | 5 +
arch/powerpc/sysdev/mpic.c | 184 ++++++++++++++++++---
5 files changed, 182 insertions(+), 27 deletions(-)
--
1.9.3
MPIC on NXP (Freescale) P2020 supports following irq
ranges:
> 0 - 11 (External interrupt)
> 16 - 79 (Internal interrupt)
> 176 - 183 (Messaging interrupt)
> 224 - 231 (Shared message signaled interrupt)
We have to remove "irq_count" from platform code as platform
is given precedence over device-tree, while I think device-tree
should have precedence.
Signed-off-by: Bharat Bhushan <[email protected]>
---
arch/powerpc/boot/dts/fsl/p2020si-post.dtsi | 3 +++
arch/powerpc/platforms/85xx/mpc85xx_rdb.c | 5 +++++
2 files changed, 8 insertions(+)
diff --git a/arch/powerpc/boot/dts/fsl/p2020si-post.dtsi b/arch/powerpc/boot/dts/fsl/p2020si-post.dtsi
index 884e01b..08e266b 100644
--- a/arch/powerpc/boot/dts/fsl/p2020si-post.dtsi
+++ b/arch/powerpc/boot/dts/fsl/p2020si-post.dtsi
@@ -192,6 +192,9 @@
/include/ "pq3-sec3.1-0.dtsi"
/include/ "pq3-mpic.dtsi"
/include/ "pq3-mpic-timer-B.dtsi"
+ pic@40000 {
+ supported-irq-ranges = <0 11 16 79 176 183 224 231>;
+ };
global-utilities@e0000 {
compatible = "fsl,p2020-guts";
diff --git a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
index 1006950..49ff348 100644
--- a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
+++ b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
@@ -57,6 +57,11 @@ void __init mpc85xx_rdb_pic_init(void)
MPIC_BIG_ENDIAN |
MPIC_SINGLE_DEST_CPU,
0, 256, " OpenPIC ");
+ } else if (of_machine_is_compatible("fsl,P2020RDB-PC")) {
+ mpic = mpic_alloc(NULL, 0,
+ MPIC_BIG_ENDIAN |
+ MPIC_SINGLE_DEST_CPU,
+ 0, 0, " OpenPIC ");
} else {
mpic = mpic_alloc(NULL, 0,
MPIC_BIG_ENDIAN |
--
1.9.3
Freescale MPIC h/w may not support all interrupt sources reported
by hardware, "last-interrupt-source" or platform. On these platforms
a misconfigured device tree that assigns one of the reserved
interrupts leaves a non-functioning system without warning.
This patch adds "supported-irq-ranges" property in device tree to
provide the range of supported source of interrupts. If a reserved
interrupt used then it will not be programming h/w, which it does
currently, and through warning.
Signed-off-by: Bharat Bhushan <[email protected]>
---
.../devicetree/bindings/powerpc/fsl/mpic.txt | 8 ++
arch/powerpc/include/asm/mpic.h | 9 ++
arch/powerpc/sysdev/mpic.c | 113 +++++++++++++++++++--
3 files changed, 121 insertions(+), 9 deletions(-)
diff --git a/Documentation/devicetree/bindings/powerpc/fsl/mpic.txt b/Documentation/devicetree/bindings/powerpc/fsl/mpic.txt
index dc57446..bd6da54 100644
--- a/Documentation/devicetree/bindings/powerpc/fsl/mpic.txt
+++ b/Documentation/devicetree/bindings/powerpc/fsl/mpic.txt
@@ -77,6 +77,14 @@ PROPERTIES
in the global feature registers. If specified, this field will
override the value read from MPIC_GREG_FEATURE_LAST_SRC.
+ - supported-irq-ranges
+ Usage: optional
+ Value type: <prop-encoded-array>
+ Definition: This encodes arbitrary number of start-irq and end-irq
+ pairs, both including. Interrupt source supported by an MPIC
+ may not be contigous, in that case this property will be used
+ to pass supported source of interrupt ranges.
+
INTERRUPT SPECIFIER DEFINITION
Interrupt specifiers consists of 4 cells encoded as
diff --git a/arch/powerpc/include/asm/mpic.h b/arch/powerpc/include/asm/mpic.h
index fad8ddd..4080c98 100644
--- a/arch/powerpc/include/asm/mpic.h
+++ b/arch/powerpc/include/asm/mpic.h
@@ -252,6 +252,11 @@ struct mpic_irq_save {
#endif
};
+struct mpic_irq_range {
+ u32 start_irq;
+ u32 end_irq;
+};
+
/* The instance data of a given MPIC */
struct mpic
{
@@ -281,6 +286,10 @@ struct mpic
/* Number of sources */
unsigned int num_sources;
+ /* Supported source ranges */
+ unsigned int num_ranges;
+ struct mpic_irq_range *irq_ranges;
+
/* vector numbers used for internal sources (ipi/timers) */
unsigned int ipi_vecs[4];
unsigned int timer_vecs[8];
diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
index d503887..cbf3a51 100644
--- a/arch/powerpc/sysdev/mpic.c
+++ b/arch/powerpc/sysdev/mpic.c
@@ -155,6 +155,23 @@ struct bus_type mpic_subsys = {
#endif /* CONFIG_MPIC_WEIRD */
+static int mpic_irq_source_invalid(struct mpic *mpic, unsigned int irq)
+{
+ int i;
+
+ for (i = 0; i < mpic->num_ranges; i++) {
+ if ((irq >= mpic->irq_ranges[i].start_irq) &&
+ (irq <= mpic->irq_ranges[i].end_irq))
+ return 0;
+ }
+
+ /* if not supported irq-ranges then check for num_sources */
+ if (!mpic->num_ranges && irq < mpic->num_sources)
+ return 0;
+
+ return -EINVAL;
+}
+
static inline unsigned int mpic_processor_id(struct mpic *mpic)
{
unsigned int cpu = 0;
@@ -873,8 +890,10 @@ int mpic_set_irq_type(struct irq_data *d, unsigned int flow_type)
DBG("mpic: set_irq_type(mpic:@%p,virq:%d,src:0x%x,type:0x%x)\n",
mpic, d->irq, src, flow_type);
- if (src >= mpic->num_sources)
+ if (mpic_irq_source_invalid(mpic, src)) {
+ WARN(1, "mpic: Reserved IRQ source %d\n", src);
return -EINVAL;
+ }
vold = mpic_irq_read(src, MPIC_INFO(IRQ_VECTOR_PRI));
@@ -933,8 +952,10 @@ void mpic_set_vector(unsigned int virq, unsigned int vector)
DBG("mpic: set_vector(mpic:@%p,virq:%d,src:%d,vector:0x%x)\n",
mpic, virq, src, vector);
- if (src >= mpic->num_sources)
+ if (mpic_irq_source_invalid(mpic, src)) {
+ WARN(1, "mpic: Reserved IRQ source %d\n", src);
return;
+ }
vecpri = mpic_irq_read(src, MPIC_INFO(IRQ_VECTOR_PRI));
vecpri = vecpri & ~MPIC_INFO(VECPRI_VECTOR_MASK);
@@ -950,8 +971,10 @@ static void mpic_set_destination(unsigned int virq, unsigned int cpuid)
DBG("mpic: set_destination(mpic:@%p,virq:%d,src:%d,cpuid:0x%x)\n",
mpic, virq, src, cpuid);
- if (src >= mpic->num_sources)
+ if (mpic_irq_source_invalid(mpic, src)) {
+ WARN(1, "mpic: Reserved IRQ source %d\n", src);
return;
+ }
mpic_irq_write(src, MPIC_INFO(IRQ_DESTINATION), 1 << cpuid);
}
@@ -1038,7 +1061,7 @@ static int mpic_host_map(struct irq_domain *h, unsigned int virq,
if (mpic_map_error_int(mpic, virq, hw))
return 0;
- if (hw >= mpic->num_sources) {
+ if (mpic_irq_source_invalid(mpic, hw)) {
pr_warn("mpic: Mapping of source 0x%x failed, source out of range !\n",
(unsigned int)hw);
return -EINVAL;
@@ -1210,6 +1233,52 @@ u32 fsl_mpic_primary_get_version(void)
return 0;
}
+static u32 mpic_last_irq_from_ranges(struct mpic *mpic)
+{
+ int i;
+ u32 last_irq = 0;
+
+ for (i = 0; i < mpic->num_ranges; i++)
+ if (last_irq < mpic->irq_ranges[i].end_irq)
+ last_irq = mpic->irq_ranges[i].end_irq;
+
+ return last_irq;
+}
+
+static int __init mpic_init_irq_ranges(struct mpic *mpic)
+{
+ const u32 *irq_ranges;
+ u32 len, count;
+ int i;
+
+ irq_ranges = of_get_property(mpic->node, "supported-irq-ranges", &len);
+ if (irq_ranges == NULL) {
+ pr_info("%s : supported-irq-ranges not found in mpic(%p)\n",
+ __func__, mpic->node);
+ return -1;
+ }
+
+ if (len % (2 * sizeof(u32)) != 0) {
+ pr_info("%s : incorrect irq ranges in mpic(%p)\n",
+ __func__, mpic->node);
+ return -1;
+ }
+
+ count = len / (2 * sizeof(u32));
+ mpic->irq_ranges = kcalloc(count, sizeof(struct mpic_irq_range),
+ GFP_KERNEL);
+ if (mpic->irq_ranges == NULL)
+ return -1;
+
+ mpic->num_ranges = count;
+ for (i = 0; i < count; i++) {
+ mpic->irq_ranges[i].start_irq = *irq_ranges++;
+ mpic->irq_ranges[i].end_irq = *irq_ranges++;
+ }
+
+ return 0;
+}
+
static int mpic_get_last_irq_source(struct mpic *mpic,
unsigned int irq_count,
unsigned int isu_size)
@@ -1219,14 +1288,18 @@ static int mpic_get_last_irq_source(struct mpic *mpic,
/* Current priority order for getting last irq:
* 1) irq_count from platform
- * 2) "last-interrupt-source" from device tree
- * 3) isu_size from platform
- * 4) MPIC h/w GREG_FEATURE_0 register
+ * 2) "supported-irq-ranges" from device tree
+ * 3) "last-interrupt-source" from device tree
+ * 4) isu_size from platform
+ * 5) MPIC h/w GREG_FEATURE_0 register
*/
if (irq_count)
return (irq_count - 1);
+ if (!mpic_init_irq_ranges(mpic))
+ return mpic_last_irq_from_ranges(mpic);
+
if (!of_property_read_u32(mpic->node, "last-interrupt-source",
&last_irq)) {
return last_irq;
@@ -1632,6 +1705,10 @@ void __init mpic_init(struct mpic *mpic)
u32 vecpri = MPIC_VECPRI_MASK | i |
(8 << MPIC_VECPRI_PRIORITY_SHIFT);
+ /* Skip if source irq not valid */
+ if (mpic_irq_source_invalid(mpic, i))
+ continue;
+
/* check if protected */
if (mpic->protected && test_bit(i, mpic->protected))
continue;
@@ -1732,9 +1809,14 @@ void mpic_setup_this_cpu(void)
* values of irq_desc[].affinity in irq.c.
*/
if (distribute_irqs && !(mpic->flags & MPIC_SINGLE_DEST_CPU)) {
- for (i = 0; i < mpic->num_sources ; i++)
+ for (i = 0; i < mpic->num_sources ; i++) {
+ /* Skip if irq source is not valid */
+ if (mpic_irq_source_invalid(mpic, i))
+ continue;
+
mpic_irq_write(i, MPIC_INFO(IRQ_DESTINATION),
mpic_irq_read(i, MPIC_INFO(IRQ_DESTINATION)) | msk);
+ }
}
/* Set current processor priority to 0 */
@@ -1772,9 +1854,14 @@ void mpic_teardown_this_cpu(int secondary)
raw_spin_lock_irqsave(&mpic_lock, flags);
/* let the mpic know we don't want intrs. */
- for (i = 0; i < mpic->num_sources ; i++)
+ for (i = 0; i < mpic->num_sources ; i++) {
+ /* Skip if irq not valid */
+ if (mpic_irq_source_invalid(mpic, i))
+ continue;
+
mpic_irq_write(i, MPIC_INFO(IRQ_DESTINATION),
mpic_irq_read(i, MPIC_INFO(IRQ_DESTINATION)) & ~msk);
+ }
/* Set current processor priority to max */
mpic_cpu_write(MPIC_INFO(CPU_CURRENT_TASK_PRI), 0xf);
@@ -1958,6 +2045,10 @@ static void mpic_suspend_one(struct mpic *mpic)
int i;
for (i = 0; i < mpic->num_sources; i++) {
+ /* Skip if irq source not valid */
+ if (mpic_irq_source_invalid(mpic, i))
+ continue;
+
mpic->save_data[i].vecprio =
mpic_irq_read(i, MPIC_INFO(IRQ_VECTOR_PRI));
mpic->save_data[i].dest =
@@ -1982,6 +2073,10 @@ static void mpic_resume_one(struct mpic *mpic)
int i;
for (i = 0; i < mpic->num_sources; i++) {
+ /* Skip if irq source not valid */
+ if (mpic_irq_source_invalid(mpic, i))
+ continue;
+
mpic_irq_write(i, MPIC_INFO(IRQ_VECTOR_PRI),
mpic->save_data[i].vecprio);
mpic_irq_write(i, MPIC_INFO(IRQ_DESTINATION),
--
1.9.3
As mpic can have non-contiguous source of interrupt range,
print same during boot.
Signed-off-by: Bharat Bhushan <[email protected]>
---
arch/powerpc/sysdev/mpic.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
index cbf3a51..8df248f 100644
--- a/arch/powerpc/sysdev/mpic.c
+++ b/arch/powerpc/sysdev/mpic.c
@@ -155,6 +155,21 @@ struct bus_type mpic_subsys = {
#endif /* CONFIG_MPIC_WEIRD */
+static void mpic_show_irq_ranges(struct mpic *mpic)
+{
+ int i;
+
+ pr_info("mpic: Initializing for %d sources\n", mpic->num_sources);
+
+ if (mpic->num_ranges) {
+ pr_info(" Supported source of interrupt ranges\n");
+ for (i = 0; i < mpic->num_ranges; i++)
+ pr_info(" > %d - %d\n", mpic->irq_ranges[i].start_irq,
+ mpic->irq_ranges[i].end_irq);
+
+ }
+}
+
static int mpic_irq_source_invalid(struct mpic *mpic, unsigned int irq)
{
int i;
@@ -1646,8 +1661,7 @@ void __init mpic_init(struct mpic *mpic)
int num_timers = 4;
BUG_ON(mpic->num_sources == 0);
-
- printk(KERN_INFO "mpic: Initializing for %d sources\n", mpic->num_sources);
+ mpic_show_irq_ranges(mpic);
/* Set current processor priority to max */
mpic_cpu_write(MPIC_INFO(CPU_CURRENT_TASK_PRI), 0xf);
--
1.9.3
This function just moves the last-irq calculation
logic to a function, while no change in logic.
Signed-off-by: Bharat Bhushan <[email protected]>
---
arch/powerpc/sysdev/mpic.c | 52 +++++++++++++++++++++++++++++-----------------
1 file changed, 33 insertions(+), 19 deletions(-)
diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
index 353b439..b6803bc 100644
--- a/arch/powerpc/sysdev/mpic.c
+++ b/arch/powerpc/sysdev/mpic.c
@@ -1210,6 +1210,36 @@ u32 fsl_mpic_primary_get_version(void)
return 0;
}
+static int mpic_get_last_irq_source(struct mpic *mpic,
+ unsigned int irq_count,
+ unsigned int isu_size)
+{
+ u32 last_irq;
+ u32 greg_feature;
+
+ /*
+ * Read feature register. For non-ISU MPICs, num sources as well. On
+ * ISU MPICs, sources are counted as ISUs are added
+ */
+ greg_feature = mpic_read(mpic->gregs, MPIC_INFO(GREG_FEATURE_0));
+
+ /*
+ * By default, the last source number comes from the MPIC, but the
+ * device-tree and board support code can override it on buggy hw.
+ * If we get passed an isu_size (multi-isu MPIC) then we use that
+ * as a default instead of the value read from the HW.
+ */
+ last_irq = (greg_feature & MPIC_GREG_FEATURE_LAST_SRC_MASK)
+ >> MPIC_GREG_FEATURE_LAST_SRC_SHIFT;
+ if (isu_size)
+ last_irq = isu_size * MPIC_MAX_ISU - 1;
+ of_property_read_u32(mpic->node, "last-interrupt-source", &last_irq);
+ if (irq_count)
+ last_irq = irq_count - 1;
+
+ return last_irq;
+}
+
struct mpic * __init mpic_alloc(struct device_node *node,
phys_addr_t phys_addr,
unsigned int flags,
@@ -1451,25 +1481,7 @@ struct mpic * __init mpic_alloc(struct device_node *node,
0x1000);
}
- /*
- * Read feature register. For non-ISU MPICs, num sources as well. On
- * ISU MPICs, sources are counted as ISUs are added
- */
- greg_feature = mpic_read(mpic->gregs, MPIC_INFO(GREG_FEATURE_0));
-
- /*
- * By default, the last source number comes from the MPIC, but the
- * device-tree and board support code can override it on buggy hw.
- * If we get passed an isu_size (multi-isu MPIC) then we use that
- * as a default instead of the value read from the HW.
- */
- last_irq = (greg_feature & MPIC_GREG_FEATURE_LAST_SRC_MASK)
- >> MPIC_GREG_FEATURE_LAST_SRC_SHIFT;
- if (isu_size)
- last_irq = isu_size * MPIC_MAX_ISU - 1;
- of_property_read_u32(mpic->node, "last-interrupt-source", &last_irq);
- if (irq_count)
- last_irq = irq_count - 1;
+ last_irq = mpic_get_last_irq_source(mpic, irq_count, isu_size);
/* Initialize main ISU if none provided */
if (!isu_size) {
@@ -1495,6 +1507,8 @@ struct mpic * __init mpic_alloc(struct device_node *node,
if (mpic->irqhost == NULL)
return NULL;
+ greg_feature = mpic_read(mpic->gregs, MPIC_INFO(GREG_FEATURE_0));
+
/* Display version */
switch (greg_feature & MPIC_GREG_FEATURE_VERSION_MASK) {
case 1:
--
1.9.3
Last irq calculation logic uses below priority order:
1) irq_count from platform
2) "last-interrupt-source" from device tree
3) isu_size from platform
4) MPIC h/w GREG_FEATURE_0 register
This patch reworks the last irq calculation logic but
functionality and priority order are same as before.
Signed-off-by: Bharat Bhushan <[email protected]>
---
arch/powerpc/sysdev/mpic.c | 31 +++++++++++++++++++------------
1 file changed, 19 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
index b6803bc..d503887 100644
--- a/arch/powerpc/sysdev/mpic.c
+++ b/arch/powerpc/sysdev/mpic.c
@@ -1217,25 +1217,32 @@ static int mpic_get_last_irq_source(struct mpic *mpic,
u32 last_irq;
u32 greg_feature;
+ /* Current priority order for getting last irq:
+ * 1) irq_count from platform
+ * 2) "last-interrupt-source" from device tree
+ * 3) isu_size from platform
+ * 4) MPIC h/w GREG_FEATURE_0 register
+ */
+
+ if (irq_count)
+ return (irq_count - 1);
+
+ if (!of_property_read_u32(mpic->node, "last-interrupt-source",
+ &last_irq)) {
+ return last_irq;
+ }
+
+ if (isu_size)
+ return (isu_size * MPIC_MAX_ISU - 1);
+
/*
- * Read feature register. For non-ISU MPICs, num sources as well. On
+ * Read feature register. For non-ISU MPICs, num sources as well. On
* ISU MPICs, sources are counted as ISUs are added
*/
greg_feature = mpic_read(mpic->gregs, MPIC_INFO(GREG_FEATURE_0));
- /*
- * By default, the last source number comes from the MPIC, but the
- * device-tree and board support code can override it on buggy hw.
- * If we get passed an isu_size (multi-isu MPIC) then we use that
- * as a default instead of the value read from the HW.
- */
last_irq = (greg_feature & MPIC_GREG_FEATURE_LAST_SRC_MASK)
>> MPIC_GREG_FEATURE_LAST_SRC_SHIFT;
- if (isu_size)
- last_irq = isu_size * MPIC_MAX_ISU - 1;
- of_property_read_u32(mpic->node, "last-interrupt-source", &last_irq);
- if (irq_count)
- last_irq = irq_count - 1;
return last_irq;
}
--
1.9.3
On Fri, Jul 27, 2018 at 03:17:59PM +0530, Bharat Bhushan wrote:
> Freescale MPIC h/w may not support all interrupt sources reported
> by hardware, "last-interrupt-source" or platform. On these platforms
> a misconfigured device tree that assigns one of the reserved
> interrupts leaves a non-functioning system without warning.
There are lots of ways to misconfigure DTs. I don't think this is
special and needs a property. We've had some interrupt mask or valid
properties in the past, but generally don't accept those.
>
> This patch adds "supported-irq-ranges" property in device tree to
> provide the range of supported source of interrupts. If a reserved
> interrupt used then it will not be programming h/w, which it does
> currently, and through warning.
>
> Signed-off-by: Bharat Bhushan <[email protected]>
> ---
> .../devicetree/bindings/powerpc/fsl/mpic.txt | 8 ++
> arch/powerpc/include/asm/mpic.h | 9 ++
> arch/powerpc/sysdev/mpic.c | 113 +++++++++++++++++++--
> 3 files changed, 121 insertions(+), 9 deletions(-)
On Tue, 2018-08-07 at 12:09 -0600, Rob Herring wrote:
> On Fri, Jul 27, 2018 at 03:17:59PM +0530, Bharat Bhushan wrote:
> > Freescale MPIC h/w may not support all interrupt sources reported
> > by hardware, "last-interrupt-source" or platform. On these platforms
> > a misconfigured device tree that assigns one of the reserved
> > interrupts leaves a non-functioning system without warning.
>
> There are lots of ways to misconfigure DTs. I don't think this is
> special and needs a property.
Yeah, the system will be just as non-functioning if you specify a valid-but-
wrong-for-the-device interrupt number.
> We've had some interrupt mask or valid
> properties in the past, but generally don't accept those.
FWIW, some of them like protected-sources and mpic-msgr-receive-mask aren't
for detecting errors, but are for partitioning (though the former is obsolete
with pic-no-reset).
-Scott
On Fri, 2018-07-27 at 15:18 +0530, Bharat Bhushan wrote:
> MPIC on NXP (Freescale) P2020 supports following irq
> ranges:
> > 0 - 11 (External interrupt)
> > 16 - 79 (Internal interrupt)
> > 176 - 183 (Messaging interrupt)
> > 224 - 231 (Shared message signaled interrupt)
Why don't you convert to the 4-cell interrupt specifiers that make dealing
with these ranges less error-prone?
> diff --git a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
> b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
> index 1006950..49ff348 100644
> --- a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
> +++ b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
> @@ -57,6 +57,11 @@ void __init mpc85xx_rdb_pic_init(void)
> MPIC_BIG_ENDIAN |
> MPIC_SINGLE_DEST_CPU,
> 0, 256, " OpenPIC ");
> + } else if (of_machine_is_compatible("fsl,P2020RDB-PC")) {
> + mpic = mpic_alloc(NULL, 0,
> + MPIC_BIG_ENDIAN |
> + MPIC_SINGLE_DEST_CPU,
> + 0, 0, " OpenPIC ");
> } else {
> mpic = mpic_alloc(NULL, 0,
> MPIC_BIG_ENDIAN |
I don't think we want to grow a list of every single revision of every board
in these platform files.
-Scott
> -----Original Message-----
> From: Scott Wood [mailto:[email protected]]
> Sent: Wednesday, August 8, 2018 2:34 AM
> To: Rob Herring <[email protected]>; Bharat Bhushan
> <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [RFC 3/5] powerpc/mpic: Add support for non-contiguous irq
> ranges
>
> On Tue, 2018-08-07 at 12:09 -0600, Rob Herring wrote:
> > On Fri, Jul 27, 2018 at 03:17:59PM +0530, Bharat Bhushan wrote:
> > > Freescale MPIC h/w may not support all interrupt sources reported by
> > > hardware, "last-interrupt-source" or platform. On these platforms a
> > > misconfigured device tree that assigns one of the reserved
> > > interrupts leaves a non-functioning system without warning.
> >
> > There are lots of ways to misconfigure DTs. I don't think this is
> > special and needs a property.
>
> Yeah, the system will be just as non-functioning if you specify a valid-but-
> wrong-for-the-device interrupt number.
Some is one additional benefits of this changes, MPIC have reserved regions for un-supported interrupts and read/writes to these reserved regions seams have no effect.
MPIC driver reads/writes to the reserved regions during init/uninit and save/restore state.
Let me know if it make sense to have these changes for mentioned reasons.
Thanks
-Bharat
>
> > We've had some interrupt mask or valid properties in the past, but
> > generally don't accept those.
>
> FWIW, some of them like protected-sources and mpic-msgr-receive-mask
> aren't for detecting errors, but are for partitioning (though the former is
> obsolete with pic-no-reset).
>
> -Scott
> -----Original Message-----
> From: Scott Wood [mailto:[email protected]]
> Sent: Wednesday, August 8, 2018 2:44 AM
> To: Bharat Bhushan <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [RFC 5/5] powerpc/fsl: Add supported-irq-ranges for P2020
>
> On Fri, 2018-07-27 at 15:18 +0530, Bharat Bhushan wrote:
> > MPIC on NXP (Freescale) P2020 supports following irq
> > ranges:
> > > 0 - 11 (External interrupt)
> > > 16 - 79 (Internal interrupt)
> > > 176 - 183 (Messaging interrupt)
> > > 224 - 231 (Shared message signaled interrupt)
>
> Why don't you convert to the 4-cell interrupt specifiers that make dealing
> with these ranges less error-prone?
Ok , will do if we agree to have this series as per comment on other patch.
>
> > diff --git a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
> > b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
> > index 1006950..49ff348 100644
> > --- a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
> > +++ b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
> > @@ -57,6 +57,11 @@ void __init mpc85xx_rdb_pic_init(void)
> > MPIC_BIG_ENDIAN |
> > MPIC_SINGLE_DEST_CPU,
> > 0, 256, " OpenPIC ");
> > + } else if (of_machine_is_compatible("fsl,P2020RDB-PC")) {
> > + mpic = mpic_alloc(NULL, 0,
> > + MPIC_BIG_ENDIAN |
> > + MPIC_SINGLE_DEST_CPU,
> > + 0, 0, " OpenPIC ");
> > } else {
> > mpic = mpic_alloc(NULL, 0,
> > MPIC_BIG_ENDIAN |
>
> I don't think we want to grow a list of every single revision of every board in
> these platform files.
One other confusing observation I have is that "irq_count" from platform code is given precedence over "last-interrupt-source" in device-tree.
Should not device-tree should have precedence otherwise there is no point using " last-interrupt-source" if platform code passes "irq_count" in mpic_alloc().
Thanks
-Bharat
>
> -Scott
On Wed, 2018-08-08 at 03:37 +0000, Bharat Bhushan wrote:
> > -----Original Message-----
> > From: Scott Wood [mailto:[email protected]]
> > Sent: Wednesday, August 8, 2018 2:34 AM
> > To: Rob Herring <[email protected]>; Bharat Bhushan
> > <[email protected]>
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected]; linux-
> > [email protected]; [email protected];
> > [email protected]; [email protected]
> > Subject: Re: [RFC 3/5] powerpc/mpic: Add support for non-contiguous irq
> > ranges
> >
> > On Tue, 2018-08-07 at 12:09 -0600, Rob Herring wrote:
> > > On Fri, Jul 27, 2018 at 03:17:59PM +0530, Bharat Bhushan wrote:
> > > > Freescale MPIC h/w may not support all interrupt sources reported by
> > > > hardware, "last-interrupt-source" or platform. On these platforms a
> > > > misconfigured device tree that assigns one of the reserved
> > > > interrupts leaves a non-functioning system without warning.
> > >
> > > There are lots of ways to misconfigure DTs. I don't think this is
> > > special and needs a property.
> >
> > Yeah, the system will be just as non-functioning if you specify a valid-
> > but-
> > wrong-for-the-device interrupt number.
>
> Some is one additional benefits of this changes, MPIC have reserved regions
> for un-supported interrupts and read/writes to these reserved regions seams
> have no effect.
> MPIC driver reads/writes to the reserved regions during init/uninit and
> save/restore state.
>
> Let me know if it make sense to have these changes for mentioned reasons.
The driver has been doing this forever with no ill effect. What is the
motivation for this change?
-Scott
> -----Original Message-----
> From: Scott Wood [mailto:[email protected]]
> Sent: Wednesday, August 8, 2018 11:21 AM
> To: Bharat Bhushan <[email protected]>; Rob Herring
> <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [RFC 3/5] powerpc/mpic: Add support for non-contiguous irq
> ranges
>
> On Wed, 2018-08-08 at 03:37 +0000, Bharat Bhushan wrote:
> > > -----Original Message-----
> > > From: Scott Wood [mailto:[email protected]]
> > > Sent: Wednesday, August 8, 2018 2:34 AM
> > > To: Rob Herring <[email protected]>; Bharat Bhushan
> > > <[email protected]>
> > > Cc: [email protected]; [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected]; linux-
> > > [email protected]; [email protected];
> > > [email protected]; [email protected]
> > > Subject: Re: [RFC 3/5] powerpc/mpic: Add support for non-contiguous
> > > irq ranges
> > >
> > > On Tue, 2018-08-07 at 12:09 -0600, Rob Herring wrote:
> > > > On Fri, Jul 27, 2018 at 03:17:59PM +0530, Bharat Bhushan wrote:
> > > > > Freescale MPIC h/w may not support all interrupt sources
> > > > > reported by hardware, "last-interrupt-source" or platform. On
> > > > > these platforms a misconfigured device tree that assigns one of
> > > > > the reserved interrupts leaves a non-functioning system without
> warning.
> > > >
> > > > There are lots of ways to misconfigure DTs. I don't think this is
> > > > special and needs a property.
> > >
> > > Yeah, the system will be just as non-functioning if you specify a
> > > valid-
> > > but-
> > > wrong-for-the-device interrupt number.
> >
> > Some is one additional benefits of this changes, MPIC have reserved
> > regions for un-supported interrupts and read/writes to these reserved
> > regions seams have no effect.
> > MPIC driver reads/writes to the reserved regions during init/uninit
> > and save/restore state.
> >
> > Let me know if it make sense to have these changes for mentioned
> reasons.
>
> The driver has been doing this forever with no ill effect.
Yes, there are no issue reported
> What is the motivation for this change?
On Simulation model I see warning when accessing the reserved region, So this patch is just an effort to improve.
Thanks
-Bharat
>
> -Scott
On Wed, 2018-08-08 at 03:44 +0000, Bharat Bhushan wrote:
> > -----Original Message-----
> > From: Scott Wood [mailto:[email protected]]
> > Sent: Wednesday, August 8, 2018 2:44 AM
> > To: Bharat Bhushan <[email protected]>;
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected]; linux-
> > [email protected]
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]
> > Subject: Re: [RFC 5/5] powerpc/fsl: Add supported-irq-ranges for P2020
> >
> > On Fri, 2018-07-27 at 15:18 +0530, Bharat Bhushan wrote:
> > > MPIC on NXP (Freescale) P2020 supports following irq
> > > ranges:
> > > > 0 - 11 (External interrupt)
> > > > 16 - 79 (Internal interrupt)
> > > > 176 - 183 (Messaging interrupt)
> > > > 224 - 231 (Shared message signaled interrupt)
> >
> > Why don't you convert to the 4-cell interrupt specifiers that make dealing
> > with these ranges less error-prone?
>
> Ok , will do if we agree to have this series as per comment on other patch.
If you're concerned with errors, this would be a good things to do regardless.
Actually, it seems that p2020si-post.dtsi already uses 4-cell interrupts.
What is motivating this patchset? Is there something wrong in the existing
dts files?
>
> >
> > > diff --git a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
> > > b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
> > > index 1006950..49ff348 100644
> > > --- a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
> > > +++ b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
> > > @@ -57,6 +57,11 @@ void __init mpc85xx_rdb_pic_init(void)
> > > MPIC_BIG_ENDIAN |
> > > MPIC_SINGLE_DEST_CPU,
> > > 0, 256, " OpenPIC ");
> > > + } else if (of_machine_is_compatible("fsl,P2020RDB-PC")) {
> > > + mpic = mpic_alloc(NULL, 0,
> > > + MPIC_BIG_ENDIAN |
> > > + MPIC_SINGLE_DEST_CPU,
> > > + 0, 0, " OpenPIC ");
> > > } else {
> > > mpic = mpic_alloc(NULL, 0,
> > > MPIC_BIG_ENDIAN |
> >
> > I don't think we want to grow a list of every single revision of every
> > board in
> > these platform files.
>
> One other confusing observation I have is that "irq_count" from platform
> code is given precedence over "last-interrupt-source" in device-tree.
> Should not device-tree should have precedence otherwise there is no point
> using " last-interrupt-source" if platform code passes "irq_count" in
> mpic_alloc().
Maybe, though I don't think it matters much given that last-interrupt-source
was only added to avoid having to pass irq_count in platform code.
-Scott
> -----Original Message-----
> From: Scott Wood [mailto:[email protected]]
> Sent: Wednesday, August 8, 2018 11:26 AM
> To: Bharat Bhushan <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [RFC 5/5] powerpc/fsl: Add supported-irq-ranges for P2020
>
> On Wed, 2018-08-08 at 03:44 +0000, Bharat Bhushan wrote:
> > > -----Original Message-----
> > > From: Scott Wood [mailto:[email protected]]
> > > Sent: Wednesday, August 8, 2018 2:44 AM
> > > To: Bharat Bhushan <[email protected]>;
> > > [email protected]; [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected]; linux-
> > > [email protected]
> > > Cc: [email protected]; [email protected];
> > > [email protected]; [email protected]
> > > Subject: Re: [RFC 5/5] powerpc/fsl: Add supported-irq-ranges for
> > > P2020
> > >
> > > On Fri, 2018-07-27 at 15:18 +0530, Bharat Bhushan wrote:
> > > > MPIC on NXP (Freescale) P2020 supports following irq
> > > > ranges:
> > > > > 0 - 11 (External interrupt)
> > > > > 16 - 79 (Internal interrupt)
> > > > > 176 - 183 (Messaging interrupt)
> > > > > 224 - 231 (Shared message signaled interrupt)
> > >
> > > Why don't you convert to the 4-cell interrupt specifiers that make
> > > dealing with these ranges less error-prone?
> >
> > Ok , will do if we agree to have this series as per comment on other patch.
>
> If you're concerned with errors, this would be a good things to do regardless.
> Actually, it seems that p2020si-post.dtsi already uses 4-cell interrupts.
>
> What is motivating this patchset? Is there something wrong in the existing
> dts files?
There is no error in device tree. Main motivation is to improve code for following reasons:
- While code study it was found that if a reserved irq-number used then there are no check in driver. irq will be configured as correct and interrupt will never fire.
- Warnings were observed on development platform (simulator) when read/write to reserved MPIC reason during init.
>
>
> >
> > >
> > > > diff --git a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
> > > > b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
> > > > index 1006950..49ff348 100644
> > > > --- a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
> > > > +++ b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
> > > > @@ -57,6 +57,11 @@ void __init mpc85xx_rdb_pic_init(void)
> > > > MPIC_BIG_ENDIAN |
> > > > MPIC_SINGLE_DEST_CPU,
> > > > 0, 256, " OpenPIC ");
> > > > + } else if (of_machine_is_compatible("fsl,P2020RDB-PC")) {
> > > > + mpic = mpic_alloc(NULL, 0,
> > > > + MPIC_BIG_ENDIAN |
> > > > + MPIC_SINGLE_DEST_CPU,
> > > > + 0, 0, " OpenPIC ");
> > > > } else {
> > > > mpic = mpic_alloc(NULL, 0,
> > > > MPIC_BIG_ENDIAN |
> > >
> > > I don't think we want to grow a list of every single revision of
> > > every board in these platform files.
> >
> > One other confusing observation I have is that "irq_count" from
> > platform code is given precedence over "last-interrupt-source" in device-
> tree.
> > Should not device-tree should have precedence otherwise there is no
> > point using " last-interrupt-source" if platform code passes
> > "irq_count" in mpic_alloc().
>
> Maybe, though I don't think it matters much given that last-interrupt-source
> was only added to avoid having to pass irq_count in platform code.
Thanks for clarifying;
My understanding was that "last-interrupt-source" added to ensure that we can over-ride value passed from platform code. In that case we do not need to change code and can control from device tree.
Thanks
-Bharat
>
> -Scott
On Wed, 2018-08-08 at 06:28 +0000, Bharat Bhushan wrote:
> > -----Original Message-----
> > From: Scott Wood [mailto:[email protected]]
> > Sent: Wednesday, August 8, 2018 11:26 AM
> > To: Bharat Bhushan <[email protected]>;
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected]; linux-
> > [email protected]
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]
> > Subject: Re: [RFC 5/5] powerpc/fsl: Add supported-irq-ranges for P2020
> >
> > On Wed, 2018-08-08 at 03:44 +0000, Bharat Bhushan wrote:
> > > > -----Original Message-----
> > > > From: Scott Wood [mailto:[email protected]]
> > > > Sent: Wednesday, August 8, 2018 2:44 AM
> > > > To: Bharat Bhushan <[email protected]>;
> > > > [email protected]; [email protected]; [email protected];
> > > > [email protected]; [email protected];
> > > > [email protected]; [email protected];
> > > > [email protected]; [email protected]; linux-
> > > > [email protected]
> > > > Cc: [email protected]; [email protected];
> > > > [email protected]; [email protected]
> > > > Subject: Re: [RFC 5/5] powerpc/fsl: Add supported-irq-ranges for
> > > > P2020
> > > >
> > > > On Fri, 2018-07-27 at 15:18 +0530, Bharat Bhushan wrote:
> > > > > MPIC on NXP (Freescale) P2020 supports following irq
> > > > > ranges:
> > > > > > 0 - 11 (External interrupt)
> > > > > > 16 - 79 (Internal interrupt)
> > > > > > 176 - 183 (Messaging interrupt)
> > > > > > 224 - 231 (Shared message signaled interrupt)
> > > >
> > > > Why don't you convert to the 4-cell interrupt specifiers that make
> > > > dealing with these ranges less error-prone?
> > >
> > > Ok , will do if we agree to have this series as per comment on other
> > > patch.
> >
> > If you're concerned with errors, this would be a good things to do
> > regardless.
> > Actually, it seems that p2020si-post.dtsi already uses 4-cell interrupts.
> >
> > What is motivating this patchset? Is there something wrong in the
> > existing
> > dts files?
>
> There is no error in device tree. Main motivation is to improve code for
> following reasons:
> - While code study it was found that if a reserved irq-number used then
> there are no check in driver. irq will be configured as correct and
> interrupt will never fire.
Again, a wrong interrupt number won't fire, whether an interrupt by that
number exists or not. I wouldn't mind a sanity check in the driver if the
programming model made it properly discoverable, but I don't think it's worth
messing with device trees just for this (and even less so given that there
don't seem to be new chips coming out that this would be relevant for).
> > > One other confusing observation I have is that "irq_count" from
> > > platform code is given precedence over "last-interrupt-source" in
> > > device-
> >
> > tree.
> > > Should not device-tree should have precedence otherwise there is no
> > > point using " last-interrupt-source" if platform code passes
> > > "irq_count" in mpic_alloc().
> >
> > Maybe, though I don't think it matters much given that last-interrupt-
> > source
> > was only added to avoid having to pass irq_count in platform code.
>
> Thanks for clarifying;
>
> My understanding was that "last-interrupt-source" added to ensure that we
> can over-ride value passed from platform code. In that case we do not need
> to change code and can control from device tree.
The changelog says, "To avoid needing to write custom board-specific code to
detect that scenario, allow it to be easily overridden in the device-tree,"
where "it" means the value provided by hardware. The goal was to pass in 256
without board code in the kernel, not to override the 256.
-Scott
> -----Original Message-----
> From: Scott Wood [mailto:[email protected]]
> Sent: Wednesday, August 8, 2018 11:27 PM
> To: Bharat Bhushan <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [RFC 5/5] powerpc/fsl: Add supported-irq-ranges for P2020
>
> On Wed, 2018-08-08 at 06:28 +0000, Bharat Bhushan wrote:
> > > -----Original Message-----
> > > From: Scott Wood [mailto:[email protected]]
> > > Sent: Wednesday, August 8, 2018 11:26 AM
> > > To: Bharat Bhushan <[email protected]>;
> > > [email protected]; [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected]; linux-
> > > [email protected]
> > > Cc: [email protected]; [email protected];
> > > [email protected]; [email protected]
> > > Subject: Re: [RFC 5/5] powerpc/fsl: Add supported-irq-ranges for
> > > P2020
> > >
> > > On Wed, 2018-08-08 at 03:44 +0000, Bharat Bhushan wrote:
> > > > > -----Original Message-----
> > > > > From: Scott Wood [mailto:[email protected]]
> > > > > Sent: Wednesday, August 8, 2018 2:44 AM
> > > > > To: Bharat Bhushan <[email protected]>;
> > > > > [email protected]; [email protected]; [email protected];
> > > > > [email protected]; [email protected];
> > > > > [email protected]; [email protected];
> > > > > [email protected]; [email protected];
> > > > > linux- [email protected]
> > > > > Cc: [email protected]; [email protected];
> > > > > [email protected]; [email protected]
> > > > > Subject: Re: [RFC 5/5] powerpc/fsl: Add supported-irq-ranges for
> > > > > P2020
> > > > >
> > > > > On Fri, 2018-07-27 at 15:18 +0530, Bharat Bhushan wrote:
> > > > > > MPIC on NXP (Freescale) P2020 supports following irq
> > > > > > ranges:
> > > > > > > 0 - 11 (External interrupt)
> > > > > > > 16 - 79 (Internal interrupt)
> > > > > > > 176 - 183 (Messaging interrupt)
> > > > > > > 224 - 231 (Shared message signaled interrupt)
> > > > >
> > > > > Why don't you convert to the 4-cell interrupt specifiers that
> > > > > make dealing with these ranges less error-prone?
> > > >
> > > > Ok , will do if we agree to have this series as per comment on
> > > > other patch.
> > >
> > > If you're concerned with errors, this would be a good things to do
> > > regardless.
> > > Actually, it seems that p2020si-post.dtsi already uses 4-cell interrupts.
> > >
> > > What is motivating this patchset? Is there something wrong in the
> > > existing dts files?
> >
> > There is no error in device tree. Main motivation is to improve code
> > for following reasons:
> > - While code study it was found that if a reserved irq-number used
> > then there are no check in driver. irq will be configured as correct
> > and interrupt will never fire.
>
> Again, a wrong interrupt number won't fire, whether an interrupt by that
> number exists or not. I wouldn't mind a sanity check in the driver if the
> programming model made it properly discoverable, but I don't think it's
> worth messing with device trees just for this (and even less so given that
> there don't seem to be new chips coming out that this would be relevant
> for).
Fair enough, we can use MPIC version to define supported interrupts ranges. Will that be acceptable.
Thanks
-Bharat
>
> > > > One other confusing observation I have is that "irq_count" from
> > > > platform code is given precedence over "last-interrupt-source" in
> > > > device-
> > >
> > > tree.
> > > > Should not device-tree should have precedence otherwise there is
> > > > no point using " last-interrupt-source" if platform code passes
> > > > "irq_count" in mpic_alloc().
> > >
> > > Maybe, though I don't think it matters much given that
> > > last-interrupt- source was only added to avoid having to pass
> > > irq_count in platform code.
> >
> > Thanks for clarifying;
> >
> > My understanding was that "last-interrupt-source" added to ensure that
> > we can over-ride value passed from platform code. In that case we do
> > not need to change code and can control from device tree.
>
> The changelog says, "To avoid needing to write custom board-specific code
> to detect that scenario, allow it to be easily overridden in the device-tree,"
> where "it" means the value provided by hardware. The goal was to pass in
> 256 without board code in the kernel, not to override the 256.
>
> -Scott
On Thu, 2018-08-09 at 03:28 +0000, Bharat Bhushan wrote:
> > -----Original Message-----
> > From: Scott Wood [mailto:[email protected]]
> > Sent: Wednesday, August 8, 2018 11:27 PM
> > To: Bharat Bhushan <[email protected]>;
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected]; linux-
> > [email protected]
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]
> > Subject: Re: [RFC 5/5] powerpc/fsl: Add supported-irq-ranges for P2020
> >
> > On Wed, 2018-08-08 at 06:28 +0000, Bharat Bhushan wrote:
> > > > -----Original Message-----
> > > > From: Scott Wood [mailto:[email protected]]
> > > > Sent: Wednesday, August 8, 2018 11:26 AM
> > > > To: Bharat Bhushan <[email protected]>;
> > > > [email protected]; [email protected]; [email protected];
> > > > [email protected]; [email protected];
> > > > [email protected]; [email protected];
> > > > [email protected]; [email protected]; linux-
> > > > [email protected]
> > > > Cc: [email protected]; [email protected];
> > > > [email protected]; [email protected]
> > > > Subject: Re: [RFC 5/5] powerpc/fsl: Add supported-irq-ranges for
> > > > P2020
> > > >
> > > > On Wed, 2018-08-08 at 03:44 +0000, Bharat Bhushan wrote:
> > > > > > -----Original Message-----
> > > > > > From: Scott Wood [mailto:[email protected]]
> > > > > > Sent: Wednesday, August 8, 2018 2:44 AM
> > > > > > To: Bharat Bhushan <[email protected]>;
> > > > > > [email protected]; [email protected]; [email protected];
> > > > > > [email protected]; [email protected];
> > > > > > [email protected]; [email protected];
> > > > > > [email protected]; [email protected];
> > > > > > linux- [email protected]
> > > > > > Cc: [email protected]; [email protected];
> > > > > > [email protected]; [email protected]
> > > > > > Subject: Re: [RFC 5/5] powerpc/fsl: Add supported-irq-ranges for
> > > > > > P2020
> > > > > >
> > > > > > On Fri, 2018-07-27 at 15:18 +0530, Bharat Bhushan wrote:
> > > > > > > MPIC on NXP (Freescale) P2020 supports following irq
> > > > > > > ranges:
> > > > > > > > 0 - 11 (External interrupt)
> > > > > > > > 16 - 79 (Internal interrupt)
> > > > > > > > 176 - 183 (Messaging interrupt)
> > > > > > > > 224 - 231 (Shared message signaled interrupt)
> > > > > >
> > > > > > Why don't you convert to the 4-cell interrupt specifiers that
> > > > > > make dealing with these ranges less error-prone?
> > > > >
> > > > > Ok , will do if we agree to have this series as per comment on
> > > > > other patch.
> > > >
> > > > If you're concerned with errors, this would be a good things to do
> > > > regardless.
> > > > Actually, it seems that p2020si-post.dtsi already uses 4-cell
> > > > interrupts.
> > > >
> > > > What is motivating this patchset? Is there something wrong in the
> > > > existing dts files?
> > >
> > > There is no error in device tree. Main motivation is to improve code
> > > for following reasons:
> > > - While code study it was found that if a reserved irq-number used
> > > then there are no check in driver. irq will be configured as correct
> > > and interrupt will never fire.
> >
> > Again, a wrong interrupt number won't fire, whether an interrupt by that
> > number exists or not. I wouldn't mind a sanity check in the driver if the
> > programming model made it properly discoverable, but I don't think it's
> > worth messing with device trees just for this (and even less so given that
> > there don't seem to be new chips coming out that this would be relevant
> > for).
>
> Fair enough, we can use MPIC version to define supported interrupts ranges.
> Will that be acceptable.
It's better than device tree changes but I'm not convinced it's worthwhile
just to suppress some simulator warnings. If the warnings really bother you,
you can use pic-no-reset in the device tree (assuming this isn't some new chip
that you want to make sure doesn't fall over when the usual mpic init happens)
and/or convince the hardware people to make the interface properly
discoverable including discontiguous regions (if there *is* some new chip I
haven't heard about).
-Scott
> -----Original Message-----
> From: Scott Wood [mailto:[email protected]]
> Sent: Thursday, August 9, 2018 11:42 AM
> To: Bharat Bhushan <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [RFC 5/5] powerpc/fsl: Add supported-irq-ranges for P2020
>
> On Thu, 2018-08-09 at 03:28 +0000, Bharat Bhushan wrote:
> > > -----Original Message-----
> > > From: Scott Wood [mailto:[email protected]]
> > > Sent: Wednesday, August 8, 2018 11:27 PM
> > > To: Bharat Bhushan <[email protected]>;
> > > [email protected]; [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected]; linux-
> > > [email protected]
> > > Cc: [email protected]; [email protected];
> > > [email protected]; [email protected]
> > > Subject: Re: [RFC 5/5] powerpc/fsl: Add supported-irq-ranges for
> > > P2020
> > >
> > > On Wed, 2018-08-08 at 06:28 +0000, Bharat Bhushan wrote:
> > > > > -----Original Message-----
> > > > > From: Scott Wood [mailto:[email protected]]
> > > > > Sent: Wednesday, August 8, 2018 11:26 AM
> > > > > To: Bharat Bhushan <[email protected]>;
> > > > > [email protected]; [email protected]; [email protected];
> > > > > [email protected]; [email protected];
> > > > > [email protected]; [email protected];
> > > > > [email protected]; [email protected];
> > > > > linux- [email protected]
> > > > > Cc: [email protected]; [email protected];
> > > > > [email protected]; [email protected]
> > > > > Subject: Re: [RFC 5/5] powerpc/fsl: Add supported-irq-ranges for
> > > > > P2020
> > > > >
> > > > > On Wed, 2018-08-08 at 03:44 +0000, Bharat Bhushan wrote:
> > > > > > > -----Original Message-----
> > > > > > > From: Scott Wood [mailto:[email protected]]
> > > > > > > Sent: Wednesday, August 8, 2018 2:44 AM
> > > > > > > To: Bharat Bhushan <[email protected]>;
> > > > > > > [email protected]; [email protected];
> > > > > > > [email protected]; [email protected];
> > > > > > > [email protected]; [email protected];
> > > > > > > [email protected]; [email protected];
> > > > > > > [email protected];
> > > > > > > linux- [email protected]
> > > > > > > Cc: [email protected]; [email protected];
> > > > > > > [email protected]; [email protected]
> > > > > > > Subject: Re: [RFC 5/5] powerpc/fsl: Add supported-irq-ranges
> > > > > > > for
> > > > > > > P2020
> > > > > > >
> > > > > > > On Fri, 2018-07-27 at 15:18 +0530, Bharat Bhushan wrote:
> > > > > > > > MPIC on NXP (Freescale) P2020 supports following irq
> > > > > > > > ranges:
> > > > > > > > > 0 - 11 (External interrupt)
> > > > > > > > > 16 - 79 (Internal interrupt)
> > > > > > > > > 176 - 183 (Messaging interrupt)
> > > > > > > > > 224 - 231 (Shared message signaled interrupt)
> > > > > > >
> > > > > > > Why don't you convert to the 4-cell interrupt specifiers
> > > > > > > that make dealing with these ranges less error-prone?
> > > > > >
> > > > > > Ok , will do if we agree to have this series as per comment on
> > > > > > other patch.
> > > > >
> > > > > If you're concerned with errors, this would be a good things to
> > > > > do regardless.
> > > > > Actually, it seems that p2020si-post.dtsi already uses 4-cell
> > > > > interrupts.
> > > > >
> > > > > What is motivating this patchset? Is there something wrong in
> > > > > the existing dts files?
> > > >
> > > > There is no error in device tree. Main motivation is to improve
> > > > code for following reasons:
> > > > - While code study it was found that if a reserved irq-number
> > > > used then there are no check in driver. irq will be configured as
> > > > correct and interrupt will never fire.
> > >
> > > Again, a wrong interrupt number won't fire, whether an interrupt by
> > > that number exists or not. I wouldn't mind a sanity check in the
> > > driver if the programming model made it properly discoverable, but I
> > > don't think it's worth messing with device trees just for this (and
> > > even less so given that there don't seem to be new chips coming out
> > > that this would be relevant for).
> >
> > Fair enough, we can use MPIC version to define supported interrupts
> ranges.
> > Will that be acceptable.
>
> It's better than device tree changes but I'm not convinced it's worthwhile just
> to suppress some simulator warnings.
> If the warnings really bother you, you
> can use pic-no-reset in the device tree (assuming this isn't some new chip
> that you want to make sure doesn't fall over when the usual mpic init
> happens) and/or convince the hardware people to make the interface
> properly discoverable including discontiguous regions (if there *is* some
> new chip I haven't heard about).
There is new chip under development based on e200, which uses same mpic as in older PowerPC versions.
This patch was not just about suppressing the warning but warning was the observation that reserved region is getting accessed during init/uninit/save/restore. This patch was just an minor improvement to avoid these accesses. We will drop this series if this improvement is not convincing.
Thanks
-Bharat
>
> -Scott