Re: [PATCH v4 01/33] xen/arm: Divide GIC initialization in 2 parts

[Ian Campbell <ian.campbell@citrix.com>]

Frediano, Zoltan,

On Thu, 2015-03-19 at 19:29 +0000, Julien Grall wrote:
>     Note that the HIP04 GIC driver has not been modified because I don't
>     have a platform where I can test my changes. Although, the code is
>     still building.
> 
>     I let the Hisilicon guys (Frediano and Zoltan) providing a suitable
>     patch for there platform.

Are you going to provide an update for the hip04 driver for this change?

>     Meanwhile, I think it can go upstream as it has been acked by both
>     Ian and Stefano.
> 
>     Changes in v4:
>         - Rebase on the latest staging (no functional changes)
>         - Add Ian and Stefano's ack
>         - Typo in the commit message
> 
>     Changes in v3:
>         - Patch was previously sent in a separate series [1]
>         - Reorder the function to avoid forward declaration
>         - Make gic-v3 driver compliant to the new interface
>         - Remove spurious field addition in gicv2 structure
> 
>     Changelog based on the separate series:
> 
>     Changes in v3:
>         - Patch added.
> 
>     [1] https://patches.linaro.org/33313/
> ---
>  xen/arch/arm/gic-v2.c     | 70 ++++++++++++++++++++++---------------------
>  xen/arch/arm/gic-v3.c     | 75 ++++++++++++++++++++++++-----------------------
>  xen/arch/arm/gic.c        | 16 ++++++++--
>  xen/arch/arm/setup.c      |  3 +-
>  xen/include/asm-arm/gic.h |  8 +++++
>  5 files changed, 100 insertions(+), 72 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 20cdbc9..3be4ad6 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -674,37 +674,10 @@ static hw_irq_controller gicv2_guest_irq_type = {
>      .set_affinity = gicv2_irq_set_affinity,
>  };
>  
> -const static struct gic_hw_operations gicv2_ops = {
> -    .info                = &gicv2_info,
> -    .secondary_init      = gicv2_secondary_cpu_init,
> -    .save_state          = gicv2_save_state,
> -    .restore_state       = gicv2_restore_state,
> -    .dump_state          = gicv2_dump_state,
> -    .gicv_setup          = gicv2v_setup,
> -    .gic_host_irq_type   = &gicv2_host_irq_type,
> -    .gic_guest_irq_type  = &gicv2_guest_irq_type,
> -    .eoi_irq             = gicv2_eoi_irq,
> -    .deactivate_irq      = gicv2_dir_irq,
> -    .read_irq            = gicv2_read_irq,
> -    .set_irq_properties  = gicv2_set_irq_properties,
> -    .send_SGI            = gicv2_send_SGI,
> -    .disable_interface   = gicv2_disable_interface,
> -    .update_lr           = gicv2_update_lr,
> -    .update_hcr_status   = gicv2_hcr_status,
> -    .clear_lr            = gicv2_clear_lr,
> -    .read_lr             = gicv2_read_lr,
> -    .write_lr            = gicv2_write_lr,
> -    .read_vmcr_priority  = gicv2_read_vmcr_priority,
> -    .read_apr            = gicv2_read_apr,
> -    .make_dt_node        = gicv2_make_dt_node,
> -};
> -
> -/* Set up the GIC */
> -static int __init gicv2_init(struct dt_device_node *node, const void *data)
> +static int __init gicv2_init(void)
>  {
>      int res;
> -
> -    dt_device_set_used_by(node, DOMID_XEN);
> +    const struct dt_device_node *node = gicv2_info.node;
>  
>      res = dt_device_get_address(node, 0, &gicv2.dbase, NULL);
>      if ( res || !gicv2.dbase || (gicv2.dbase & ~PAGE_MASK) )
> @@ -727,9 +700,6 @@ static int __init gicv2_init(struct dt_device_node *node, const void *data)
>          panic("GICv2: Cannot find the maintenance IRQ");
>      gicv2_info.maintenance_irq = res;
>  
> -    /* Set the GIC as the primary interrupt controller */
> -    dt_interrupt_controller = node;
> -
>      /* TODO: Add check on distributor, cpu size */
>  
>      printk("GICv2 initialization:\n"
> @@ -774,8 +744,42 @@ static int __init gicv2_init(struct dt_device_node *node, const void *data)
>  
>      spin_unlock(&gicv2.lock);
>  
> +    return 0;
> +}
> +
> +const static struct gic_hw_operations gicv2_ops = {
> +    .info                = &gicv2_info,
> +    .init                = gicv2_init,
> +    .secondary_init      = gicv2_secondary_cpu_init,
> +    .save_state          = gicv2_save_state,
> +    .restore_state       = gicv2_restore_state,
> +    .dump_state          = gicv2_dump_state,
> +    .gicv_setup          = gicv2v_setup,
> +    .gic_host_irq_type   = &gicv2_host_irq_type,
> +    .gic_guest_irq_type  = &gicv2_guest_irq_type,
> +    .eoi_irq             = gicv2_eoi_irq,
> +    .deactivate_irq      = gicv2_dir_irq,
> +    .read_irq            = gicv2_read_irq,
> +    .set_irq_properties  = gicv2_set_irq_properties,
> +    .send_SGI            = gicv2_send_SGI,
> +    .disable_interface   = gicv2_disable_interface,
> +    .update_lr           = gicv2_update_lr,
> +    .update_hcr_status   = gicv2_hcr_status,
> +    .clear_lr            = gicv2_clear_lr,
> +    .read_lr             = gicv2_read_lr,
> +    .write_lr            = gicv2_write_lr,
> +    .read_vmcr_priority  = gicv2_read_vmcr_priority,
> +    .read_apr            = gicv2_read_apr,
> +    .make_dt_node        = gicv2_make_dt_node,
> +};
> +
> +/* Set up the GIC */
> +static int __init gicv2_preinit(struct dt_device_node *node, const void *data)
> +{
>      gicv2_info.hw_version = GIC_V2;
> +    gicv2_info.node = node;
>      register_gic_ops(&gicv2_ops);
> +    dt_irq_xlate = gic_irq_xlate;
>  
>      return 0;
>  }
> @@ -788,7 +792,7 @@ static const struct dt_device_match gicv2_dt_match[] __initconst =
>  
>  DT_DEVICE_START(gicv2, "GICv2", DEVICE_GIC)
>          .dt_match = gicv2_dt_match,
> -        .init = gicv2_init,
> +        .init = gicv2_preinit,
>  DT_DEVICE_END
>  
>  /*
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index ab80670..48772f1 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1173,31 +1173,6 @@ static const hw_irq_controller gicv3_guest_irq_type = {
>      .set_affinity = gicv3_irq_set_affinity,
>  };
>  
> -static const struct gic_hw_operations gicv3_ops = {
> -    .info                = &gicv3_info,
> -    .save_state          = gicv3_save_state,
> -    .restore_state       = gicv3_restore_state,
> -    .dump_state          = gicv3_dump_state,
> -    .gicv_setup          = gicv_v3_init,
> -    .gic_host_irq_type   = &gicv3_host_irq_type,
> -    .gic_guest_irq_type  = &gicv3_guest_irq_type,
> -    .eoi_irq             = gicv3_eoi_irq,
> -    .deactivate_irq      = gicv3_dir_irq,
> -    .read_irq            = gicv3_read_irq,
> -    .set_irq_properties  = gicv3_set_irq_properties,
> -    .send_SGI            = gicv3_send_sgi,
> -    .disable_interface   = gicv3_disable_interface,
> -    .update_lr           = gicv3_update_lr,
> -    .update_hcr_status   = gicv3_hcr_status,
> -    .clear_lr            = gicv3_clear_lr,
> -    .read_lr             = gicv3_read_lr,
> -    .write_lr            = gicv3_write_lr,
> -    .read_vmcr_priority  = gicv3_read_vmcr_priority,
> -    .read_apr            = gicv3_read_apr,
> -    .secondary_init      = gicv3_secondary_cpu_init,
> -    .make_dt_node        = gicv3_make_dt_node,
> -};
> -
>  static int __init cmp_rdist(const void *a, const void *b)
>  {
>      const struct rdist_region *l = a, *r = a;
> @@ -1207,11 +1182,12 @@ static int __init cmp_rdist(const void *a, const void *b)
>  }
>  
>  /* Set up the GIC */
> -static int __init gicv3_init(struct dt_device_node *node, const void *data)
> +static int __init gicv3_init(void)
>  {
>      struct rdist_region *rdist_regs;
>      int res, i;
>      uint32_t reg;
> +    const struct dt_device_node *node = gicv3_info.node;
>  
>      if ( !cpu_has_gicv3 )
>      {
> @@ -1219,8 +1195,6 @@ static int __init gicv3_init(struct dt_device_node *node, const void *data)
>          return -ENODEV;
>      }
>  
> -    dt_device_set_used_by(node, DOMID_XEN);
> -
>      res = dt_device_get_address(node, 0, &gicv3.dbase, &gicv3.dbase_size);
>      if ( res || !gicv3.dbase )
>          panic("GICv3: Cannot find a valid distributor address");
> @@ -1274,9 +1248,6 @@ static int __init gicv3_init(struct dt_device_node *node, const void *data)
>          panic("GICv3: Cannot find the maintenance IRQ");
>      gicv3_info.maintenance_irq = res;
>  
> -    /* Set the GIC as the primary interrupt controller */
> -    dt_interrupt_controller = node;
> -
>      for ( i = 0; i < gicv3.rdist_count; i++ )
>      {
>          /* map dbase & rdist regions */
> @@ -1311,15 +1282,47 @@ static int __init gicv3_init(struct dt_device_node *node, const void *data)
>      res = gicv3_cpu_init();
>      gicv3_hyp_init();
>  
> -    gicv3_info.hw_version = GIC_V3;
> -    /* Register hw ops*/
> -    register_gic_ops(&gicv3_ops);
> -
>      spin_unlock(&gicv3.lock);
>  
>      return res;
>  }
>  
> +static const struct gic_hw_operations gicv3_ops = {
> +    .info                = &gicv3_info,
> +    .init                = gicv3_init,
> +    .save_state          = gicv3_save_state,
> +    .restore_state       = gicv3_restore_state,
> +    .dump_state          = gicv3_dump_state,
> +    .gicv_setup          = gicv_v3_init,
> +    .gic_host_irq_type   = &gicv3_host_irq_type,
> +    .gic_guest_irq_type  = &gicv3_guest_irq_type,
> +    .eoi_irq             = gicv3_eoi_irq,
> +    .deactivate_irq      = gicv3_dir_irq,
> +    .read_irq            = gicv3_read_irq,
> +    .set_irq_properties  = gicv3_set_irq_properties,
> +    .send_SGI            = gicv3_send_sgi,
> +    .disable_interface   = gicv3_disable_interface,
> +    .update_lr           = gicv3_update_lr,
> +    .update_hcr_status   = gicv3_hcr_status,
> +    .clear_lr            = gicv3_clear_lr,
> +    .read_lr             = gicv3_read_lr,
> +    .write_lr            = gicv3_write_lr,
> +    .read_vmcr_priority  = gicv3_read_vmcr_priority,
> +    .read_apr            = gicv3_read_apr,
> +    .secondary_init      = gicv3_secondary_cpu_init,
> +    .make_dt_node        = gicv3_make_dt_node,
> +};
> +
> +static int __init gicv3_preinit(struct dt_device_node *node, const void *data)
> +{
> +    gicv3_info.hw_version = GIC_V3;
> +    gicv3_info.node = node;
> +    register_gic_ops(&gicv3_ops);
> +    dt_irq_xlate = gic_irq_xlate;
> +
> +    return 0;
> +}
> +
>  static const struct dt_device_match gicv3_dt_match[] __initconst =
>  {
>      DT_MATCH_GIC_V3,
> @@ -1328,7 +1331,7 @@ static const struct dt_device_match gicv3_dt_match[] __initconst =
>  
>  DT_DEVICE_START(gicv3, "GICv3", DEVICE_GIC)
>          .dt_match = gicv3_dt_match,
> -        .init = gicv3_init,
> +        .init = gicv3_preinit,
>  DT_DEVICE_END
>  
>  /*
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 940fb8e..8e7f24b 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -163,8 +163,10 @@ int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
>      return 0;
>  }
>  
> -/* Set up the GIC */
> -void __init gic_init(void)
> +/* Find the interrupt controller and set up the callback to translate
> + * device tree IRQ.
> + */
> +void __init gic_preinit(void)
>  {
>      int rc;
>      struct dt_device_node *node;
> @@ -189,6 +191,16 @@ void __init gic_init(void)
>      if ( !num_gics )
>          panic("Unable to find compatible GIC in the device tree");
>  
> +    /* Set the GIC as the primary interrupt controller */
> +    dt_interrupt_controller = node;
> +    dt_device_set_used_by(node, DOMID_XEN);
> +}
> +
> +/* Set up the GIC */
> +void __init gic_init(void)
> +{
> +    if ( gic_hw_ops->init() )
> +        panic("Failed to initialize the GIC drivers");
>      /* Clear LR mask for cpu0 */
>      clear_cpu_lr_mask();
>  }
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index b905c9f..4f05f57 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -752,7 +752,6 @@ void __init start_xen(unsigned long boot_phys_offset,
>  
>      vm_init();
>      dt_unflatten_host_device_tree();
> -    dt_irq_xlate = gic_irq_xlate;
>  
>      init_IRQ();
>  
> @@ -760,6 +759,8 @@ void __init start_xen(unsigned long boot_phys_offset,
>  
>      preinit_xen_time();
>  
> +    gic_preinit();
> +
>      dt_uart_init();
>      console_init_preirq();
>      console_init_ring();
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 89a9b6f..b16f98e 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -232,6 +232,10 @@ extern void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq);
>  
>  /* Accept an interrupt from the GIC and dispatch its handler */
>  extern void gic_interrupt(struct cpu_user_regs *regs, int is_fiq);
> +/* Find the interrupt controller and set up the callback to translate
> + * device tree IRQ.
> + */
> +extern void gic_preinit(void);
>  /* Bring up the interrupt controller, and report # cpus attached */
>  extern void gic_init(void);
>  /* Bring up a secondary CPU's per-CPU GIC interface */
> @@ -284,11 +288,15 @@ struct gic_info {
>      uint8_t nr_lrs;
>      /* Maintenance irq number */
>      unsigned int maintenance_irq;
> +    /* Pointer to the device tree node representing the interrupt controller */
> +    const struct dt_device_node *node;
>  };
>  
>  struct gic_hw_operations {
>      /* Hold GIC HW information */
>      const struct gic_info *info;
> +    /* Initialize the GIC and the boot CPU */
> +    int (*init)(void);
>      /* Save GIC registers */
>      void (*save_state)(struct vcpu *);
>      /* Restore GIC registers */