diff mbox series

core/interrupts: Clean up OPAL ICS node

Message ID 20190423062941.14217-1-oohall@gmail.com
State Under Review
Headers show
Series core/interrupts: Clean up OPAL ICS node | expand

Checks

Context Check Description
snowpatch_ozlabs/apply_patch success Successfully applied on branch master (d318cdb3863fcf92288528bfed3b6e435cf6f0ef)
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot fail Test snowpatch/job/snowpatch-skiboot on branch master
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco success Signed-off-by present

Commit Message

Oliver O'Halloran April 23, 2019, 6:29 a.m. UTC
For some reason that's lost to time the OPAL interrupt-controller source
device node has a unit address of 0. This causes dtc to emit warnings
since the node overlaps with the memory@0 node. There is also no real need
for the OPAL ICS node to even have a unit address since it has no
registers (reflected by the zero length in reg).

This patch reworks the OPAL ICS node so that it has no unit address
which results in a bare top-level node called "interrupt-controller."
While we're here get rid of the explicit device-tree traversal in
get_ics_phandle() and use a cached copy that is setup in add_ics_node().

Suggested-by: Maxim Polyakov <m.polyakov@yadro.com>
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
I had a look at the xics/xive code in linux and it doesn't seem that
either make any assumptions about the @0 being in the unit address.

Boot tested on a Tuleta and a Witherspoon without issues.
---
 core/interrupts.c    | 32 +++++++++++++++++++-------------
 include/interrupts.h |  2 +-
 2 files changed, 20 insertions(+), 14 deletions(-)

Comments

Maxim Polyakov May 14, 2019, 1:11 p.m. UTC | #1
23.04.2019 9:29, Oliver O'Halloran пишет:
> For some reason that's lost to time the OPAL interrupt-controller source
> device node has a unit address of 0. This causes dtc to emit warnings
> since the node overlaps with the memory@0 node. There is also no real need
> for the OPAL ICS node to even have a unit address since it has no
> registers (reflected by the zero length in reg).
> 
> This patch reworks the OPAL ICS node so that it has no unit address
> which results in a bare top-level node called "interrupt-controller."
> While we're here get rid of the explicit device-tree traversal in
> get_ics_phandle() and use a cached copy that is setup in add_ics_node().
> 
> Suggested-by: Maxim Polyakov <m.polyakov@yadro.com>
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> ---
> I had a look at the xics/xive code in linux and it doesn't seem that
> either make any assumptions about the @0 being in the unit address.
> 
> Boot tested on a Tuleta and a Witherspoon without issues.
> ---
>  core/interrupts.c    | 32 +++++++++++++++++++-------------
>  include/interrupts.h |  2 +-
>  2 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/core/interrupts.c b/core/interrupts.c
> index 5d7a68cd5eff..b4602d374ec3 100644
> --- a/core/interrupts.c
> +++ b/core/interrupts.c
> @@ -173,18 +173,27 @@ uint32_t get_psi_interrupt(uint32_t chip_id)
>  	return irq;
>  }
>  
> +static struct dt_node *ics_node;
>  
> -struct dt_node *add_ics_node(void)
> +void add_ics_node(void)
>  {
> -	struct dt_node *ics = dt_new_addr(dt_root, "interrupt-controller", 0);
> +	struct dt_node *ics;
>  	bool has_xive;
>  
> -	if (!ics)
> -		return NULL;
> +	/*
> +	 * This should never happen, but this code is ancient so maybe there's
> +	 * some obscure edge case it's covering
> +	 */
> +	ics = dt_new(dt_root, "interrupt-controller");
> +	if (!ics) {
> +		ics_node = dt_find_by_name(dt_root, "interrupt-controller");
> +		prlog(PR_DEBUG, "OPAL ICS node already exists?\n");
> +
> +		return;
> +	}
>  
>  	has_xive = proc_gen >= proc_gen_p9;
>  
> -	dt_add_property_cells(ics, "reg", 0, 0, 0, 0);
>  	dt_add_property_strings(ics, "compatible",
>  				has_xive ? "ibm,opal-xive-vc" : "IBM,ppc-xics",
>  				"IBM,opal-xics");
> @@ -194,19 +203,16 @@ struct dt_node *add_ics_node(void)
>  			       "PowerPC-Interrupt-Source-Controller");
>  	dt_add_property(ics, "interrupt-controller", NULL, 0);
>  
> -	return ics;
> +	ics_node = ics;
>  }
>  
>  uint32_t get_ics_phandle(void)
>  {
> -	struct dt_node *i;
>  
> -	for (i = dt_first(dt_root); i; i = dt_next(dt_root, i)) {
> -		if (streq(i->name, "interrupt-controller@0")) {
> -			return i->phandle;
> -		}
> -	}
> -	abort();
> +	if (!ics_node)
> +		abort();
> +
> +	return ics_node->phandle;
>  }
>  
>  void add_opal_interrupts(void)
> diff --git a/include/interrupts.h b/include/interrupts.h
> index 2c4fa7e92399..4a49bbaeb601 100644
> --- a/include/interrupts.h
> +++ b/include/interrupts.h
> @@ -312,7 +312,7 @@ extern void irq_for_each_source(void (*cb)(struct irq_source *, void *),
>  
>  extern uint32_t get_psi_interrupt(uint32_t chip_id);
>  
> -extern struct dt_node *add_ics_node(void);
> +extern void add_ics_node(void);
>  extern void add_opal_interrupts(void);
>  extern uint32_t get_ics_phandle(void);
>  
> 

Hi all. Any news about this patch?
diff mbox series

Patch

diff --git a/core/interrupts.c b/core/interrupts.c
index 5d7a68cd5eff..b4602d374ec3 100644
--- a/core/interrupts.c
+++ b/core/interrupts.c
@@ -173,18 +173,27 @@  uint32_t get_psi_interrupt(uint32_t chip_id)
 	return irq;
 }
 
+static struct dt_node *ics_node;
 
-struct dt_node *add_ics_node(void)
+void add_ics_node(void)
 {
-	struct dt_node *ics = dt_new_addr(dt_root, "interrupt-controller", 0);
+	struct dt_node *ics;
 	bool has_xive;
 
-	if (!ics)
-		return NULL;
+	/*
+	 * This should never happen, but this code is ancient so maybe there's
+	 * some obscure edge case it's covering
+	 */
+	ics = dt_new(dt_root, "interrupt-controller");
+	if (!ics) {
+		ics_node = dt_find_by_name(dt_root, "interrupt-controller");
+		prlog(PR_DEBUG, "OPAL ICS node already exists?\n");
+
+		return;
+	}
 
 	has_xive = proc_gen >= proc_gen_p9;
 
-	dt_add_property_cells(ics, "reg", 0, 0, 0, 0);
 	dt_add_property_strings(ics, "compatible",
 				has_xive ? "ibm,opal-xive-vc" : "IBM,ppc-xics",
 				"IBM,opal-xics");
@@ -194,19 +203,16 @@  struct dt_node *add_ics_node(void)
 			       "PowerPC-Interrupt-Source-Controller");
 	dt_add_property(ics, "interrupt-controller", NULL, 0);
 
-	return ics;
+	ics_node = ics;
 }
 
 uint32_t get_ics_phandle(void)
 {
-	struct dt_node *i;
 
-	for (i = dt_first(dt_root); i; i = dt_next(dt_root, i)) {
-		if (streq(i->name, "interrupt-controller@0")) {
-			return i->phandle;
-		}
-	}
-	abort();
+	if (!ics_node)
+		abort();
+
+	return ics_node->phandle;
 }
 
 void add_opal_interrupts(void)
diff --git a/include/interrupts.h b/include/interrupts.h
index 2c4fa7e92399..4a49bbaeb601 100644
--- a/include/interrupts.h
+++ b/include/interrupts.h
@@ -312,7 +312,7 @@  extern void irq_for_each_source(void (*cb)(struct irq_source *, void *),
 
 extern uint32_t get_psi_interrupt(uint32_t chip_id);
 
-extern struct dt_node *add_ics_node(void);
+extern void add_ics_node(void);
 extern void add_opal_interrupts(void);
 extern uint32_t get_ics_phandle(void);