Re: [PATCH v2] xen/common: Move gic_preinit() to common code

[oleksii.kurochko@xxxxxxxxx]

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@xxxxxxxxx>
> > > ---
> > > 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