Re: [PATCH v2 3/3] xen: arm: handle PCI DT node ranges and interrupt-map properties

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

On 16/03/15 16:22, Ian Campbell wrote:
>>> +    if ( dt_device_type_is_equal(dev, "pci") )
>>> +        return map_pci_device_ranges(d, dev, ranges, len);
>>> +
>>> +    printk("Cannot handle ranges for non-PCI device %s type %s\n",
>>> +           dt_node_name(dev), dev->type);
>>> +
>>
>> Is the printk really necessary? It will a spurious log on platform where
>> ranges is not empty (midway, arndale, foundation model...).
> 
> If the ranges is present and non-empty it's not impossible that we need
> to be doing something with it. I'd rather try and figure out how to
> whitelist such nodes, perhaps they lack a dev_type completely?

Why would we compute the range? Any usable MMIO should be describe the
child.

We have to compute the ranges for PCI because we want to map everything
at boot time and the PCI devices are not discoverable via the device tree...

AFAICT the parsing of the range is only required when the child are not
discoverable via device tree. So the lack of device_type is not a good
check.

IHMO all those stuff could be removed by doing the parsing in
PHYSDEVOP_pci_device_add.

Anyway this is a different approach which require Xen PCI support in
Linux...

>>> +static int map_device_interrupts(struct domain *d, const struct dt_device_node *dev)
>>> +{
>>> +
>>> +    if ( !dt_property_read_bool(dev, "interrupt-map") )
>>
>> It's strange to use dt_property_read_bool here and dt_get_property above
>> to check the emptiness.
>>
>> I prefer the dt_get_property version which is less confusing.
>>
>> Anyway, why do you need to check interrupt-map. Can't your new helper
>> deal with empty property?
> 
> It can, but the code below would log for any non-pci device, which is
> undesirable if there is no interrupt-map present.

It would be undesirable when interrupt-map is present for non-pci device...

We are able to go further down and map the interrupt so you will add
more worrying log on normal platform.

It's like the ranges, if we try to compute interrupt-map on non-pci we
may have some trouble and route IRQ we should not.

>>
>>> +        return 0; /* No interrupt map to handle */
>>> +
>>> +    if ( dt_device_type_is_equal(dev, "pci") )
>>> +        return dt_for_each_irq_map(dev, &map_interrupt_to_domain, d);
>>> +
>>> +    printk("Cannot handle interrupt-map for non-PCI device %s type %s\n",
>>> +           dt_node_name(dev), dev->type);
>>> +
>>> +    /* Lets not worry for now... */
>>> +    return 0;
>>> +}
>>> +
>>> +
>>>  /* Map the device in the domain */
>>>  static int map_device(struct domain *d, struct dt_device_node *dev)
>>>  {
>>> @@ -1025,6 +1173,14 @@ static int map_device(struct domain *d, struct dt_device_node *dev)
>>>          }
>>>      }
>>>  
>>> +    res = map_device_ranges(d, dev);
>>> +    if ( res )
>>> +        return res;
>>> +
>>> +    res = map_device_interrupts(d, dev);
>>
>> The name of the function doesn't make much sense. We already map the
>> interrupt above (see platform get_irq).
> 
> child_interrupt perhaps?

Sounds better. Although, with my point above those 2 functions is PCI
related.

Regards,

-- 
Julien Grall