Re: [PATCH V2 15/33] xen/arm: Use hierarchical device tree to retrieve GIC information

[Julien Grall <julien.grall@linaro.org>]

On 05/08/2013 02:46 PM, Ian Campbell wrote:

> On Wed, 2013-05-08 at 03:33 +0100, Julien Grall wrote:
>> - Remove early parsing for GIC addresses
>> - Remove hard coded maintenance IRQ number
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> Changes in v2:
>>     - use the new function request_dt_irq
>> ---
>>  xen/arch/arm/gic.c            |   63 ++++++++++++++++++++++++++++-------------
>>  xen/common/device_tree.c      |   42 ---------------------------
>>  xen/include/xen/device_tree.h |    8 ------
>>  3 files changed, 43 insertions(+), 70 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 1efa9a3..34304b3 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -45,6 +45,7 @@ static struct {
>>      paddr_t hbase;       /* Address of virtual interface registers */
>>      paddr_t vbase;       /* Address of virtual cpu interface registers */
>>      unsigned int lines;  /* Number of interrupts (SPIs + PPIs + SGIs) */
>> +    struct dt_irq maintenance; /* IRQ maintenance */
>>      unsigned int cpus;
>>      spinlock_t lock;
>>  } gic;
>> @@ -352,28 +353,49 @@ int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
>>  /* Set up the GIC */
>>  void __init gic_init(void)
>>  {
>> +    struct dt_device_node *node;
>> +    int res;
>> +
>> +    node = dt_find_interrupt_controller("arm,cortex-a15-gic");
>> +    if ( !node )
>> +        panic("Unable to find compatible GIC in the device tree\n");
>> +
>> +    dt_device_set_used_by(node, DT_USED_BY_XEN);
>> +
>> +    res = dt_device_get_address(node, 0, &gic.dbase, NULL);
>> +    if ( res || !gic.dbase || (gic.dbase & ~PAGE_MASK) )
>> +        panic("GIC: Cannot find a valid address for the distributor\n");
>> +
>> +    res = dt_device_get_address(node, 1, &gic.cbase, NULL);
>> +    if ( res || !gic.cbase || (gic.cbase & ~PAGE_MASK) )
>> +        panic("GIC: Cannot find a valid address for the CPU\n");
>> +
>> +    res = dt_device_get_address(node, 2, &gic.hbase, NULL);
>> +    if ( res || !gic.hbase || (gic.hbase & ~PAGE_MASK) )
>> +        panic("GIC: Cannot find a valid address for the hypervisor\n");
>> +
>> +    res = dt_device_get_address(node, 3, &gic.vbase, NULL);
>> +    if ( res || !gic.vbase || (gic.vbase & ~PAGE_MASK) )
>> +        panic("GIC: Cannot find a valid address for the virtual CPU\n");
>> +
>> +    res = dt_device_get_irq(node, 0, &gic.maintenance);
>> +    if ( res )
>> +        panic("GIC: Cannot find the maintenance IRQ\n");
>> +
>> +    /* Set the GIC as the primary interrupt controller */
>> +    dt_interrupt_controller = node;
>> +
>> +    /* TODO: Add check on distributor, cpu size */
>> +
>>      printk("GIC initialization:\n"
>>                "        gic_dist_addr=%"PRIpaddr"\n"
>>                "        gic_cpu_addr=%"PRIpaddr"\n"
>>                "        gic_hyp_addr=%"PRIpaddr"\n"
>> -              "        gic_vcpu_addr=%"PRIpaddr"\n",
>> -              early_info.gic.gic_dist_addr, early_info.gic.gic_cpu_addr,
>> -              early_info.gic.gic_hyp_addr, early_info.gic.gic_vcpu_addr);
>> -    if ( !early_info.gic.gic_dist_addr ||
>> -         !early_info.gic.gic_cpu_addr ||
>> -         !early_info.gic.gic_hyp_addr ||
>> -         !early_info.gic.gic_vcpu_addr )
>> -        panic("the physical address of one of the GIC interfaces is missing\n");
>> -    if ( (early_info.gic.gic_dist_addr & ~PAGE_MASK) ||
>> -         (early_info.gic.gic_cpu_addr & ~PAGE_MASK) ||
>> -         (early_info.gic.gic_hyp_addr & ~PAGE_MASK) ||
>> -         (early_info.gic.gic_vcpu_addr & ~PAGE_MASK) )
>> -        panic("GIC interfaces not page aligned.\n");
> 
> Please can we keep this last check separate and after the print, this
> way we get to see why it failed (not present vs invalid).


I will fix it on the next patch series.

> 
> I'm ambivalent about keeping the !gic.Xbase separate or including it in
> the res check.


I don't see a good reason why the gic base address could not be 0. I
prefer to remove this check.

-- 
Julien