Re: [PATCH] xen: get GIC addresses from DT

[David Vrabel <david.vrabel@citrix.com>]

On 30/11/12 12:25, Stefano Stabellini wrote:
> On Fri, 30 Nov 2012, David Vrabel wrote:
>> On 23/11/12 15:21, Stefano Stabellini wrote:
>>> Get the address of the GIC distributor, cpu, virtual and virtual cpu
>>> interfaces registers from device tree.
>>
>> The original intention of the early DTB parsing was to get the minimum
>> of info needed before the DTB could be translated into a more useful
>> form (similar to what Linux does).
>>
>> Is this something that is still being considered in the long term?  If
>> so, should the GIC be initialized from this translated form, instead of
>> using the early parsing?
> 
> Regardless of the final form of DT parsing in Xen, I think that CPU,
> memory and GIC are the three things that should be parsed early.

Ok.

>>> +bool_t device_tree_node_compatible(const void *fdt, int node, const char *match)
>>> +{
>>> +    int len, l;
>>> +    const void *prop;
>>> +
>>> +    prop = fdt_getprop(fdt, node, "compatible", &len);
>>> +    if ( prop == NULL )
>>> +        return 0;
>>> +
>>> +    while ( len > 0 ) {
>>> +        if ( !strncmp(prop, match, strlen(match)) )
>>> +            return 1;
>>> +        l = strlen(prop) + 1;
>>> +        prop += l;
>>> +        len -= l;
>>> +    }
>>
>> This doesn't look right.  Don't you need to split the prop string on
>> comma boundaries?
> 
> Nope, the compatible string to match is, for example, the entirety of
> "arm,cortex-a15-gic", or alternatively "arm,cortex-a9-gic". Trying to
> match just "cortex-a15-gic" would be incorrect.
> This is what Linux does too.

Yes, you're right. I was misrembering the format.

>>> +    cell = (const u32 *)prop->data;
>>> +    device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
>>
>> Is this needed?  This cell is reread below.
> 
> Yes, because device_tree_get_reg increments the cell pointer

Perhaps I wasn't clear.  The result of this device_tree_get_reg() is not
used anywhere, and the side effect of advancing cell is also not used.

>>> +
>>> +    cell = (const u32 *)prop->data;
>>> +    reg_cells = address_cells + size_cells;
>>> +    interfaces = fdt32_to_cpu(prop->len) / (reg_cells * sizeof(u32));

David