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

Additionally, rename gic_acpi_preinit() to ic_acpi_preinit() as it is used
by ic_preinit(), while keeping it defined in architecture-specific as this
part is architecture-specific. In case if CONFIG_ACPI=n a stub for
ic_acpi_preinit() is provided. To declaration/defintion of ic_acpi_preint()
is added `inline` to deal with the compilation issue:
  error: 'ic_acpi_preinit' defined but not used [-Werror=unused-function]

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 ic 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 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               | 45 +------------------------------
 xen/arch/arm/setup.c             |  3 ++-
 xen/common/device.c              | 46 ++++++++++++++++++++++++++++++++
 xen/include/asm-generic/device.h | 10 +++++++
 4 files changed, 59 insertions(+), 45 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 3eaf670fd7..b4a1e769df 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -214,38 +214,8 @@ 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)
+void __init ic_acpi_preinit(void)
 {
     struct acpi_subtable_header *header;
     struct acpi_madt_generic_distributor *dist;
@@ -259,21 +229,8 @@ static void __init gic_acpi_preinit(void)
     if ( acpi_device_init(DEVICE_INTERRUPT_CONTROLLER, NULL, dist->version) )
         panic("Unable to find compatible GIC in the ACPI table\n");
 }
-#else
-static void __init gic_acpi_preinit(void) { }
 #endif
 
-/* Find the interrupt controller and set up the callback to translate
- * device tree or ACPI IRQ.
- */
-void __init gic_preinit(void)
-{
-    if ( acpi_disabled )
-        gic_dt_preinit();
-    else
-        gic_acpi_preinit();
-}
-
 /* Set up the GIC */
 void __init gic_init(void)
 {
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 71ebaa77ca..1ea7db0bd4 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -38,6 +38,7 @@
 #include <asm/page.h>
 #include <asm/static-evtchn.h>
 #include <asm/current.h>
+#include <asm/device.h>
 #include <asm/setup.h>
 #include <asm/gic.h>
 #include <asm/cpuerrata.h>
@@ -359,7 +360,7 @@ void asmlinkage __init start_xen(unsigned long fdt_paddr)
 
     preinit_xen_time();
 
-    gic_preinit();
+    ic_preinit();
 
     arm_uart_init();
     console_init_preirq();
diff --git a/xen/common/device.c b/xen/common/device.c
index 33e0d58f2f..f440d8be88 100644
--- a/xen/common/device.c
+++ b/xen/common/device.c
@@ -4,10 +4,14 @@
  *   xen/arch/arm/device.c
  */
 
+#include <xen/acpi.h>
 #include <xen/bug.h>
 #include <xen/device_tree.h>
 #include <xen/errno.h>
 #include <xen/init.h>
+#include <xen/kernel.h>
+#include <xen/lib.h>
+#include <xen/types.h>
 
 #include <asm/device.h>
 
@@ -56,6 +60,40 @@ enum device_class device_get_class(const struct dt_device_node *dev)
     return DEVICE_UNKNOWN;
 }
 
+static void __init ic_dt_preinit(void)
+{
+    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;
+
+        if ( !device_init(node, DEVICE_INTERRUPT_CONTROLLER, NULL) )
+        {
+            /* NOTE: Only one GIC is supported */
+            num_gics = 1;
+            break;
+        }
+    }
+
+    if ( !num_gics )
+        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);
+}
+
+#else /* !CONFIG_HAS_DEVICE_TREE */
+
+static void __init ic_dt_preinit(void) { }
+
 #endif
 
 #ifdef CONFIG_ACPI
@@ -80,3 +118,11 @@ int __init acpi_device_init(enum device_class class, const void *data, int class
 }
 
 #endif
+
+void __init ic_preinit(void)
+{
+    if ( acpi_disabled )
+        ic_dt_preinit();
+    else
+        ic_acpi_preinit();
+}
diff --git a/xen/include/asm-generic/device.h b/xen/include/asm-generic/device.h
index 1acd1ba1d8..0ce2bb3cdf 100644
--- a/xen/include/asm-generic/device.h
+++ b/xen/include/asm-generic/device.h
@@ -127,6 +127,16 @@ __section(".adev.info") = {                                         \
 
 #endif /* CONFIG_ACPI */
 
+void ic_preinit(void);
+
+#ifdef CONFIG_ACPI
+void ic_acpi_preinit(void);
+#else
+#include <xen/init.h>
+
+static inline void __init ic_acpi_preinit(void) { }
+#endif
+
 #endif /* __ASM_GENERIC_DEVICE_H__ */
 
 /*
-- 
2.47.0

On 30.10.2024 14:14, Oleksii Kurochko wrote:
> Introduce ic_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.
> 
> Additionally, rename gic_acpi_preinit() to ic_acpi_preinit() as it is used
> by ic_preinit(), while keeping it defined in architecture-specific as this
> part is architecture-specific. In case if CONFIG_ACPI=n a stub for
> ic_acpi_preinit() is provided. To declaration/defintion of ic_acpi_preint()
> is added `inline` to deal with the compilation issue:
>   error: 'ic_acpi_preinit' defined but not used [-Werror=unused-function]
> 
> 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 ic 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 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               | 45 +------------------------------
>  xen/arch/arm/setup.c             |  3 ++-
>  xen/common/device.c              | 46 ++++++++++++++++++++++++++++++++
>  xen/include/asm-generic/device.h | 10 +++++++
>  4 files changed, 59 insertions(+), 45 deletions(-)

Looks largely okay to me now, with a question and a nit at the bottom. The
question is mainly to Arm folks, where the code is coming from, and DT
maintainers: Is a file named device.c really an appropriate "home" for
IC-related code? If IC is a common thing in the DT world, would such code
maybe better live under common/device-tree/?

> --- a/xen/include/asm-generic/device.h
> +++ b/xen/include/asm-generic/device.h
> @@ -127,6 +127,16 @@ __section(".adev.info") = {                                         \
>  
>  #endif /* CONFIG_ACPI */
>  
> +void ic_preinit(void);
> +
> +#ifdef CONFIG_ACPI
> +void ic_acpi_preinit(void);
> +#else
> +#include <xen/init.h>
> +
> +static inline void __init ic_acpi_preinit(void) { }
> +#endif

I don't think there's a need to have __init on an empty inline stub. That'll
then also eliminate the need for the extra #include.

Jan

On 31/10/2024 10:34, Jan Beulich wrote:
> 
> 
> On 30.10.2024 14:14, Oleksii Kurochko wrote:
>> Introduce ic_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.
>>
>> Additionally, rename gic_acpi_preinit() to ic_acpi_preinit() as it is used
>> by ic_preinit(), while keeping it defined in architecture-specific as this
>> part is architecture-specific. In case if CONFIG_ACPI=n a stub for
>> ic_acpi_preinit() is provided. To declaration/defintion of ic_acpi_preint()
>> is added `inline` to deal with the compilation issue:
>>   error: 'ic_acpi_preinit' defined but not used [-Werror=unused-function]
>>
>> 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 ic 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 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               | 45 +------------------------------
>>  xen/arch/arm/setup.c             |  3 ++-
>>  xen/common/device.c              | 46 ++++++++++++++++++++++++++++++++
>>  xen/include/asm-generic/device.h | 10 +++++++
>>  4 files changed, 59 insertions(+), 45 deletions(-)
> 
> Looks largely okay to me now, with a question and a nit at the bottom. The
> question is mainly to Arm folks, where the code is coming from, and DT
> maintainers: Is a file named device.c really an appropriate "home" for
> IC-related code? If IC is a common thing in the DT world, would such code
> maybe better live under common/device-tree/?
I think we will have more interrupt related dt stuff common, so it would make sense to create
device-tree/irq.c or intc.c.

@Olku, can you please s/ic/intc/ ? It's much more common abbreviation for interrupt controller.
You can do e.g. grep -Rw "ic" vs grep -Rw "intc" in Linux Documentation/devicetree/bindings/interrupt-controller.

~Michal

On Thu, 2024-10-31 at 11:29 +0100, Michal Orzel wrote:
> 
> 
> On 31/10/2024 10:34, Jan Beulich wrote:
> > 
> > 
> > On 30.10.2024 14:14, Oleksii Kurochko wrote:
> > > Introduce ic_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.
> > > 
> > > Additionally, rename gic_acpi_preinit() to ic_acpi_preinit() as
> > > it is used
> > > by ic_preinit(), while keeping it defined in architecture-
> > > specific as this
> > > part is architecture-specific. In case if CONFIG_ACPI=n a stub
> > > for
> > > ic_acpi_preinit() is provided. To declaration/defintion of
> > > ic_acpi_preint()
> > > is added `inline` to deal with the compilation issue:
> > >   error: 'ic_acpi_preinit' defined but not used [-Werror=unused-
> > > function]
> > > 
> > > 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 ic 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 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               | 45 +-------------------------
> > > -----
> > >  xen/arch/arm/setup.c             |  3 ++-
> > >  xen/common/device.c              | 46
> > > ++++++++++++++++++++++++++++++++
> > >  xen/include/asm-generic/device.h | 10 +++++++
> > >  4 files changed, 59 insertions(+), 45 deletions(-)
> > 
> > Looks largely okay to me now, with a question and a nit at the
> > bottom. The
> > question is mainly to Arm folks, where the code is coming from, and
> > DT
> > maintainers: Is a file named device.c really an appropriate "home"
> > for
> > IC-related code? If IC is a common thing in the DT world, would
> > such code
> > maybe better live under common/device-tree/?
> I think we will have more interrupt related dt stuff common, so it
> would make sense to create
> device-tree/irq.c or intc.c.
What about the part of the code in common/device.c:
   void __init ic_preinit(void)
   {
       if ( acpi_disabled )
           ic_dt_preinit();
       else
           ic_acpi_preinit();
   }
   
Should it be also moved to device-tree/intc.c even ic_acpi_preinit() is
used?

~ Oleksii

On 31.10.2024 16:13, oleksii.kurochko@gmail.com wrote:
> On Thu, 2024-10-31 at 11:29 +0100, Michal Orzel wrote:
>>
>>
>> On 31/10/2024 10:34, Jan Beulich wrote:
>>>
>>>
>>> On 30.10.2024 14:14, Oleksii Kurochko wrote:
>>>> Introduce ic_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.
>>>>
>>>> Additionally, rename gic_acpi_preinit() to ic_acpi_preinit() as
>>>> it is used
>>>> by ic_preinit(), while keeping it defined in architecture-
>>>> specific as this
>>>> part is architecture-specific. In case if CONFIG_ACPI=n a stub
>>>> for
>>>> ic_acpi_preinit() is provided. To declaration/defintion of
>>>> ic_acpi_preint()
>>>> is added `inline` to deal with the compilation issue:
>>>>   error: 'ic_acpi_preinit' defined but not used [-Werror=unused-
>>>> function]
>>>>
>>>> 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 ic 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 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               | 45 +-------------------------
>>>> -----
>>>>  xen/arch/arm/setup.c             |  3 ++-
>>>>  xen/common/device.c              | 46
>>>> ++++++++++++++++++++++++++++++++
>>>>  xen/include/asm-generic/device.h | 10 +++++++
>>>>  4 files changed, 59 insertions(+), 45 deletions(-)
>>>
>>> Looks largely okay to me now, with a question and a nit at the
>>> bottom. The
>>> question is mainly to Arm folks, where the code is coming from, and
>>> DT
>>> maintainers: Is a file named device.c really an appropriate "home"
>>> for
>>> IC-related code? If IC is a common thing in the DT world, would
>>> such code
>>> maybe better live under common/device-tree/?
>> I think we will have more interrupt related dt stuff common, so it
>> would make sense to create
>> device-tree/irq.c or intc.c.
> What about the part of the code in common/device.c:
>    void __init ic_preinit(void)
>    {
>        if ( acpi_disabled )
>            ic_dt_preinit();
>        else
>            ic_acpi_preinit();
>    }
>    
> Should it be also moved to device-tree/intc.c even ic_acpi_preinit() is
> used?

Maybe best left to each arch? device.[ch] aren't a good home for this,
just like they aren't for the actual DT implementation.

Jan

On Thu, 2024-10-31 at 11:29 +0100, Michal Orzel wrote:
> 
> 
> On 31/10/2024 10:34, Jan Beulich wrote:
> > 
> > 
> > On 30.10.2024 14:14, Oleksii Kurochko wrote:
> > > Introduce ic_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.
> > > 
> > > Additionally, rename gic_acpi_preinit() to ic_acpi_preinit() as
> > > it is used
> > > by ic_preinit(), while keeping it defined in architecture-
> > > specific as this
> > > part is architecture-specific. In case if CONFIG_ACPI=n a stub
> > > for
> > > ic_acpi_preinit() is provided. To declaration/defintion of
> > > ic_acpi_preint()
> > > is added `inline` to deal with the compilation issue:
> > >   error: 'ic_acpi_preinit' defined but not used [-Werror=unused-
> > > function]
> > > 
> > > 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 ic 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 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               | 45 +-------------------------
> > > -----
> > >  xen/arch/arm/setup.c             |  3 ++-
> > >  xen/common/device.c              | 46
> > > ++++++++++++++++++++++++++++++++
> > >  xen/include/asm-generic/device.h | 10 +++++++
> > >  4 files changed, 59 insertions(+), 45 deletions(-)
> > 
> > Looks largely okay to me now, with a question and a nit at the
> > bottom. The
> > question is mainly to Arm folks, where the code is coming from, and
> > DT
> > maintainers: Is a file named device.c really an appropriate "home"
> > for
> > IC-related code? If IC is a common thing in the DT world, would
> > such code
> > maybe better live under common/device-tree/?
> I think we will have more interrupt related dt stuff common, so it
> would make sense to create
> device-tree/irq.c or intc.c.
> 
> @Olku, can you please s/ic/intc/ ? It's much more common abbreviation
> for interrupt controller.
> You can do e.g. grep -Rw "ic" vs grep -Rw "intc" in Linux
> Documentation/devicetree/bindings/interrupt-controller.
Sure, intc sounds good to me.

Thanks.

~ Oleksii