[PATCH v4] xen/common: Move gic_dt_preinit() to common code

Oleksii Kurochko posted 1 patch 2 weeks, 5 days ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/54d0ff689e167b3f3137afea45307f762ce0a974.1732027385.git.oleksii.kurochko@gmail.com
There is a newer version of this series
xen/arch/arm/gic.c              | 32 +-----------------------------
xen/common/device-tree/Makefile |  1 +
xen/common/device-tree/intc.c   | 35 +++++++++++++++++++++++++++++++++
xen/include/xen/device_tree.h   |  6 ++++++
4 files changed, 43 insertions(+), 31 deletions(-)
create mode 100644 xen/common/device-tree/intc.c
[PATCH v4] xen/common: Move gic_dt_preinit() to common code
Posted by Oleksii Kurochko 2 weeks, 5 days ago
Introduce intc_dt_preinit() in the common codebase, as it is not
architecture-specific and can be reused by both PPC and RISC-V.
This function identifies the node with the interrupt-controller property
in the device tree and calls device_init() to handle architecture-specific
initialization of the interrupt controller.

Make minor adjustments compared to the original ARM implementation of
gic_dt_preinit():
 - Remove the local rc variable in gic_dt_preinit() since it is only used once.
 - Change the prefix from gic to intc to clarify that the function is not
   specific to ARM’s GIC, making it suitable for other architectures as well.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v4:
 - Add SPDX tag in intc.c
 - s/num_gics/num_intc
 - Update the comment: s/GIC/interrupt controller.
---
Changes in v3:
 - s/ic/intc.
 - Update the commit message.
 - Move intc_dt_preinit() to common/device-tree/intc.c.
 - Add declaration of intc_dt_preinit() in xen/device_tree.h.
 - Revert intc_preinit()-related changes and just back gic_preinit() in
   Arm's gic.c.
 - Revert ACPI-related changes.
---
Changes in v2:
 - Revert changes connected to moving of gic_acpi_preinit() to common code as
   it isn't really architecture indepent part.
 - Update the commit message.
 - Move stub of ic_acpi_preinit() to <asm-generic/device.h> for the case when
   CONFIG_ACPI=n.
---
 xen/arch/arm/gic.c              | 32 +-----------------------------
 xen/common/device-tree/Makefile |  1 +
 xen/common/device-tree/intc.c   | 35 +++++++++++++++++++++++++++++++++
 xen/include/xen/device_tree.h   |  6 ++++++
 4 files changed, 43 insertions(+), 31 deletions(-)
 create mode 100644 xen/common/device-tree/intc.c

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 3eaf670fd7..acf61a4de3 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -214,36 +214,6 @@ int gic_map_hwdom_extra_mappings(struct domain *d)
     return 0;
 }
 
-static void __init gic_dt_preinit(void)
-{
-    int rc;
-    struct dt_device_node *node;
-    uint8_t num_gics = 0;
-
-    dt_for_each_device_node( dt_host, node )
-    {
-        if ( !dt_get_property(node, "interrupt-controller", NULL) )
-            continue;
-
-        if ( !dt_get_parent(node) )
-            continue;
-
-        rc = device_init(node, DEVICE_INTERRUPT_CONTROLLER, NULL);
-        if ( !rc )
-        {
-            /* NOTE: Only one GIC is supported */
-            num_gics = 1;
-            break;
-        }
-    }
-    if ( !num_gics )
-        panic("Unable to find compatible GIC in the device tree\n");
-
-    /* Set the GIC as the primary interrupt controller */
-    dt_interrupt_controller = node;
-    dt_device_set_used_by(node, DOMID_XEN);
-}
-
 #ifdef CONFIG_ACPI
 static void __init gic_acpi_preinit(void)
 {
@@ -269,7 +239,7 @@ static void __init gic_acpi_preinit(void) { }
 void __init gic_preinit(void)
 {
     if ( acpi_disabled )
-        gic_dt_preinit();
+        intc_dt_preinit();
     else
         gic_acpi_preinit();
 }
diff --git a/xen/common/device-tree/Makefile b/xen/common/device-tree/Makefile
index 58052d074e..7c549be38a 100644
--- a/xen/common/device-tree/Makefile
+++ b/xen/common/device-tree/Makefile
@@ -2,3 +2,4 @@ obj-y += bootfdt.init.o
 obj-y += bootinfo.init.o
 obj-y += device-tree.o
 obj-$(CONFIG_OVERLAY_DTB) += dt-overlay.o
+obj-y += intc.o
diff --git a/xen/common/device-tree/intc.c b/xen/common/device-tree/intc.c
new file mode 100644
index 0000000000..d2bcbc2d5e
--- /dev/null
+++ b/xen/common/device-tree/intc.c
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include <xen/device_tree.h>
+#include <xen/init.h>
+#include <xen/lib.h>
+
+void __init intc_dt_preinit(void)
+{
+    struct dt_device_node *node;
+    uint8_t num_intc = 0;
+
+    dt_for_each_device_node( dt_host, node )
+    {
+        if ( !dt_get_property(node, "interrupt-controller", NULL) )
+            continue;
+
+        if ( !dt_get_parent(node) )
+            continue;
+
+        if ( !device_init(node, DEVICE_INTERRUPT_CONTROLLER, NULL) )
+        {
+            /* NOTE: Only one interrupt controller is supported */
+            num_intc = 1;
+            break;
+        }
+    }
+
+    if ( !num_intc )
+        panic("Unable to find compatible interrupt contoller"
+              "in the device tree\n");
+
+    /* Set the interrupt controller as the primary interrupt controller */
+    dt_interrupt_controller = node;
+    dt_device_set_used_by(node, DOMID_XEN);
+}
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index e6287305a7..33d70b9594 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -238,6 +238,12 @@ extern rwlock_t dt_host_lock;
 struct dt_device_node *
 dt_find_interrupt_controller(const struct dt_device_match *matches);
 
+#ifdef CONFIG_HAS_DEVICE_TREE
+void intc_dt_preinit(void);
+#else
+static inline void intc_dt_preinit(void) { }
+#endif
+
 #define dt_prop_cmp(s1, s2) strcmp((s1), (s2))
 #define dt_node_cmp(s1, s2) strcasecmp((s1), (s2))
 #define dt_compat_cmp(s1, s2) strcasecmp((s1), (s2))
-- 
2.47.0


Re: [PATCH v4] xen/common: Move gic_dt_preinit() to common code
Posted by Michal Orzel 2 weeks, 3 days ago

On 19/11/2024 15:56, Oleksii Kurochko wrote:
> 
> 
> Introduce intc_dt_preinit() in the common codebase, as it is not
> architecture-specific and can be reused by both PPC and RISC-V.
> This function identifies the node with the interrupt-controller property
> in the device tree and calls device_init() to handle architecture-specific
> initialization of the interrupt controller.
> 
> Make minor adjustments compared to the original ARM implementation of
> gic_dt_preinit():
>  - Remove the local rc variable in gic_dt_preinit() since it is only used once.
>  - Change the prefix from gic to intc to clarify that the function is not
>    specific to ARM’s GIC, making it suitable for other architectures as well.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> Changes in v4:
>  - Add SPDX tag in intc.c
>  - s/num_gics/num_intc
>  - Update the comment: s/GIC/interrupt controller.
> ---
> Changes in v3:
>  - s/ic/intc.
>  - Update the commit message.
>  - Move intc_dt_preinit() to common/device-tree/intc.c.
>  - Add declaration of intc_dt_preinit() in xen/device_tree.h.
>  - Revert intc_preinit()-related changes and just back gic_preinit() in
>    Arm's gic.c.
>  - Revert ACPI-related changes.
> ---
> Changes in v2:
>  - Revert changes connected to moving of gic_acpi_preinit() to common code as
>    it isn't really architecture indepent part.
>  - Update the commit message.
>  - Move stub of ic_acpi_preinit() to <asm-generic/device.h> for the case when
>    CONFIG_ACPI=n.
> ---
>  xen/arch/arm/gic.c              | 32 +-----------------------------
>  xen/common/device-tree/Makefile |  1 +
>  xen/common/device-tree/intc.c   | 35 +++++++++++++++++++++++++++++++++
>  xen/include/xen/device_tree.h   |  6 ++++++
>  4 files changed, 43 insertions(+), 31 deletions(-)
>  create mode 100644 xen/common/device-tree/intc.c
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 3eaf670fd7..acf61a4de3 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -214,36 +214,6 @@ int gic_map_hwdom_extra_mappings(struct domain *d)
>      return 0;
>  }
> 
> -static void __init gic_dt_preinit(void)
> -{
> -    int rc;
> -    struct dt_device_node *node;
> -    uint8_t num_gics = 0;
> -
> -    dt_for_each_device_node( dt_host, node )
> -    {
> -        if ( !dt_get_property(node, "interrupt-controller", NULL) )
> -            continue;
> -
> -        if ( !dt_get_parent(node) )
> -            continue;
> -
> -        rc = device_init(node, DEVICE_INTERRUPT_CONTROLLER, NULL);
> -        if ( !rc )
> -        {
> -            /* NOTE: Only one GIC is supported */
> -            num_gics = 1;
> -            break;
> -        }
> -    }
> -    if ( !num_gics )
> -        panic("Unable to find compatible GIC in the device tree\n");
> -
> -    /* Set the GIC as the primary interrupt controller */
> -    dt_interrupt_controller = node;
> -    dt_device_set_used_by(node, DOMID_XEN);
> -}
> -
>  #ifdef CONFIG_ACPI
>  static void __init gic_acpi_preinit(void)
>  {
> @@ -269,7 +239,7 @@ static void __init gic_acpi_preinit(void) { }
>  void __init gic_preinit(void)
>  {
>      if ( acpi_disabled )
> -        gic_dt_preinit();
> +        intc_dt_preinit();
>      else
>          gic_acpi_preinit();
>  }
> diff --git a/xen/common/device-tree/Makefile b/xen/common/device-tree/Makefile
> index 58052d074e..7c549be38a 100644
> --- a/xen/common/device-tree/Makefile
> +++ b/xen/common/device-tree/Makefile
> @@ -2,3 +2,4 @@ obj-y += bootfdt.init.o
>  obj-y += bootinfo.init.o
>  obj-y += device-tree.o
>  obj-$(CONFIG_OVERLAY_DTB) += dt-overlay.o
> +obj-y += intc.o
> diff --git a/xen/common/device-tree/intc.c b/xen/common/device-tree/intc.c
> new file mode 100644
> index 0000000000..d2bcbc2d5e
> --- /dev/null
> +++ b/xen/common/device-tree/intc.c
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include <xen/device_tree.h>
> +#include <xen/init.h>
> +#include <xen/lib.h>
> +
> +void __init intc_dt_preinit(void)
> +{
> +    struct dt_device_node *node;
> +    uint8_t num_intc = 0;
> +
> +    dt_for_each_device_node( dt_host, node )
> +    {
> +        if ( !dt_get_property(node, "interrupt-controller", NULL) )
> +            continue;
> +
> +        if ( !dt_get_parent(node) )
> +            continue;
> +
> +        if ( !device_init(node, DEVICE_INTERRUPT_CONTROLLER, NULL) )
> +        {
> +            /* NOTE: Only one interrupt controller is supported */
> +            num_intc = 1;
> +            break;
> +        }
> +    }
> +
> +    if ( !num_intc )
> +        panic("Unable to find compatible interrupt contoller"
> +              "in the device tree\n");
Don't split printk messages. Also the split is incorrect as it'll result in "contollerin" (i.e. no space in between).
Also s/contoller/controller/
 
> +
> +    /* Set the interrupt controller as the primary interrupt controller */
> +    dt_interrupt_controller = node;
> +    dt_device_set_used_by(node, DOMID_XEN);
> +}
Missing EMACS block at the end of file.

> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index e6287305a7..33d70b9594 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -238,6 +238,12 @@ extern rwlock_t dt_host_lock;
>  struct dt_device_node *
>  dt_find_interrupt_controller(const struct dt_device_match *matches);
> 
> +#ifdef CONFIG_HAS_DEVICE_TREE
> +void intc_dt_preinit(void);
> +#else
> +static inline void intc_dt_preinit(void) { }
> +#endif
Is it really needed to provide the stub and guards? Other DT related functions in this header are not
protected and AFAICT the inclusion of this header only works if CONFIG_HAS_DEVICE_TREE=y.

~Michal

Re: [PATCH v4] xen/common: Move gic_dt_preinit() to common code
Posted by oleksii.kurochko@gmail.com 2 weeks, 2 days ago
On Thu, 2024-11-21 at 11:27 +0100, Michal Orzel wrote:
> 
> 
> On 19/11/2024 15:56, Oleksii Kurochko wrote:
> > 
> > 
> > Introduce intc_dt_preinit() in the common codebase, as it is not
> > architecture-specific and can be reused by both PPC and RISC-V.
> > This function identifies the node with the interrupt-controller
> > property
> > in the device tree and calls device_init() to handle architecture-
> > specific
> > initialization of the interrupt controller.
> > 
> > Make minor adjustments compared to the original ARM implementation
> > of
> > gic_dt_preinit():
> >  - Remove the local rc variable in gic_dt_preinit() since it is
> > only used once.
> >  - Change the prefix from gic to intc to clarify that the function
> > is not
> >    specific to ARM’s GIC, making it suitable for other
> > architectures as well.
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> > Changes in v4:
> >  - Add SPDX tag in intc.c
> >  - s/num_gics/num_intc
> >  - Update the comment: s/GIC/interrupt controller.
> > ---
> > Changes in v3:
> >  - s/ic/intc.
> >  - Update the commit message.
> >  - Move intc_dt_preinit() to common/device-tree/intc.c.
> >  - Add declaration of intc_dt_preinit() in xen/device_tree.h.
> >  - Revert intc_preinit()-related changes and just back
> > gic_preinit() in
> >    Arm's gic.c.
> >  - Revert ACPI-related changes.
> > ---
> > Changes in v2:
> >  - Revert changes connected to moving of gic_acpi_preinit() to
> > common code as
> >    it isn't really architecture indepent part.
> >  - Update the commit message.
> >  - Move stub of ic_acpi_preinit() to <asm-generic/device.h> for the
> > case when
> >    CONFIG_ACPI=n.
> > ---
> >  xen/arch/arm/gic.c              | 32 +----------------------------
> > -
> >  xen/common/device-tree/Makefile |  1 +
> >  xen/common/device-tree/intc.c   | 35
> > +++++++++++++++++++++++++++++++++
> >  xen/include/xen/device_tree.h   |  6 ++++++
> >  4 files changed, 43 insertions(+), 31 deletions(-)
> >  create mode 100644 xen/common/device-tree/intc.c
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 3eaf670fd7..acf61a4de3 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -214,36 +214,6 @@ int gic_map_hwdom_extra_mappings(struct domain
> > *d)
> >      return 0;
> >  }
> > 
> > -static void __init gic_dt_preinit(void)
> > -{
> > -    int rc;
> > -    struct dt_device_node *node;
> > -    uint8_t num_gics = 0;
> > -
> > -    dt_for_each_device_node( dt_host, node )
> > -    {
> > -        if ( !dt_get_property(node, "interrupt-controller", NULL)
> > )
> > -            continue;
> > -
> > -        if ( !dt_get_parent(node) )
> > -            continue;
> > -
> > -        rc = device_init(node, DEVICE_INTERRUPT_CONTROLLER, NULL);
> > -        if ( !rc )
> > -        {
> > -            /* NOTE: Only one GIC is supported */
> > -            num_gics = 1;
> > -            break;
> > -        }
> > -    }
> > -    if ( !num_gics )
> > -        panic("Unable to find compatible GIC in the device
> > tree\n");
> > -
> > -    /* Set the GIC as the primary interrupt controller */
> > -    dt_interrupt_controller = node;
> > -    dt_device_set_used_by(node, DOMID_XEN);
> > -}
> > -
> >  #ifdef CONFIG_ACPI
> >  static void __init gic_acpi_preinit(void)
> >  {
> > @@ -269,7 +239,7 @@ static void __init gic_acpi_preinit(void) { }
> >  void __init gic_preinit(void)
> >  {
> >      if ( acpi_disabled )
> > -        gic_dt_preinit();
> > +        intc_dt_preinit();
> >      else
> >          gic_acpi_preinit();
> >  }
> > diff --git a/xen/common/device-tree/Makefile b/xen/common/device-
> > tree/Makefile
> > index 58052d074e..7c549be38a 100644
> > --- a/xen/common/device-tree/Makefile
> > +++ b/xen/common/device-tree/Makefile
> > @@ -2,3 +2,4 @@ obj-y += bootfdt.init.o
> >  obj-y += bootinfo.init.o
> >  obj-y += device-tree.o
> >  obj-$(CONFIG_OVERLAY_DTB) += dt-overlay.o
> > +obj-y += intc.o
> > diff --git a/xen/common/device-tree/intc.c b/xen/common/device-
> > tree/intc.c
> > new file mode 100644
> > index 0000000000..d2bcbc2d5e
> > --- /dev/null
> > +++ b/xen/common/device-tree/intc.c
> > @@ -0,0 +1,35 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#include <xen/device_tree.h>
> > +#include <xen/init.h>
> > +#include <xen/lib.h>
> > +
> > +void __init intc_dt_preinit(void)
> > +{
> > +    struct dt_device_node *node;
> > +    uint8_t num_intc = 0;
> > +
> > +    dt_for_each_device_node( dt_host, node )
> > +    {
> > +        if ( !dt_get_property(node, "interrupt-controller", NULL)
> > )
> > +            continue;
> > +
> > +        if ( !dt_get_parent(node) )
> > +            continue;
> > +
> > +        if ( !device_init(node, DEVICE_INTERRUPT_CONTROLLER, NULL)
> > )
> > +        {
> > +            /* NOTE: Only one interrupt controller is supported */
> > +            num_intc = 1;
> > +            break;
> > +        }
> > +    }
> > +
> > +    if ( !num_intc )
> > +        panic("Unable to find compatible interrupt contoller"
> > +              "in the device tree\n");
> Don't split printk messages. Also the split is incorrect as it'll
> result in "contollerin" (i.e. no space in between).
> Also s/contoller/controller/
>  
> > +
> > +    /* Set the interrupt controller as the primary interrupt
> > controller */
> > +    dt_interrupt_controller = node;
> > +    dt_device_set_used_by(node, DOMID_XEN);
> > +}
> Missing EMACS block at the end of file.
> 
> > diff --git a/xen/include/xen/device_tree.h
> > b/xen/include/xen/device_tree.h
> > index e6287305a7..33d70b9594 100644
> > --- a/xen/include/xen/device_tree.h
> > +++ b/xen/include/xen/device_tree.h
> > @@ -238,6 +238,12 @@ extern rwlock_t dt_host_lock;
> >  struct dt_device_node *
> >  dt_find_interrupt_controller(const struct dt_device_match
> > *matches);
> > 
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> > +void intc_dt_preinit(void);
> > +#else
> > +static inline void intc_dt_preinit(void) { }
> > +#endif
> Is it really needed to provide the stub and guards? Other DT related
> functions in this header are not
> protected and AFAICT the inclusion of this header only works if
> CONFIG_HAS_DEVICE_TREE=y.
I think you are right and we can really drop stub and guards. I'll send
a new patch version applying this and the comments above.

Thanks.

~ Oleksii