Re: [PATCH V1 16/29] xen/arm: Build DOM0 FDT by browsing the device tree structure

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

On 09/09/2013 12:33 PM, Ian Campbell wrote:
> On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
>>       if ( early_info.modules.nr_mods >= MOD_KERNEL &&
>>            early_info.modules.module[MOD_KERNEL].cmdline[0] )
>>           bootargs = &early_info.modules.module[MOD_KERNEL].cmdline[0];
>>
>> -    for ( prop = fdt_first_property_offset(fdt, node);
>> -          prop >= 0;
>> -          prop = fdt_next_property_offset(fdt, prop) )
>> +    for_each_property_of_node (np, pp)
>
> Is "of" here as in "the property of the node" or is it a stray Open
> Firmware from the Linux naming of these functions?
>
> Perhaps a dt_ prefix to match all the others?

Right. I will send a patch to rename for_each_property_of_node to 
dt_for_each_property_of_node.

>
>> -/* Returns the next node in fdt (starting from offset) which should be
>> - * passed through to dom0.
>> +/*
>> + * Helper to write an interrupts with the GIC format
>> + * This code is assuming the irq is an PPI.
>> + * TODO: Handle SPI
>>    */
>> -static int fdt_next_dom0_node(const void *fdt, int node,
>> -                              int *depth_out)
>> -{
>> -    int depth = *depth_out;
>> -
>> -    while ( (node = fdt_next_node(fdt, node, &depth)) &&
>> -            node >= 0 && depth >= 0 )
>> -    {
>> -        if ( depth >= DEVICE_TREE_MAX_DEPTH )
>> -            break;
>>
>> -        /* Skip /hypervisor/ node. We will inject our own. */
>> -        if ( fdt_node_check_compatible(fdt, node, "xen,xen" ) == 0 )
>> -        {
>> -            printk("Device-tree contains \"xen,xen\" node. Ignoring.\n");
>> -            continue;
>> -        }
>> +typedef __be32 interrupt_t[3];
>>
>> -        /* Skip multiboot subnodes */
>> -        if ( fdt_node_check_compatible(fdt, node,
>> -                                       "xen,multiboot-module" ) == 0 )
>> -            continue;
>> +static void set_interrupts(interrupt_t interrupt, unsigned int irq,
>> +                           unsigned int cpumask, unsigned int level)
>> +{
>> +    __be32 *cells = interrupt;
>
> This function and the interrupt_t type is only used for the
> event-channel ppi? I think gic_interrupt_t would be a better typedef
> name
> The function name is plural but I think it only handles one interrupt at
> a time, and it only handles PPIs?
>
> Unless there are other users of this code I think it could continue to
> be inside make_hypervisor_node, given that it is pretty particular
> already.

This function is used in another patch later. I will rename the 
different name in the next patch series.

>>
>> -        /* We've arrived at a node which dom0 is interested in. */
>> -        break;
>> -    }
>> +    BUG_ON(irq < 16 && irq >= 32);
>>
>> -    *depth_out = depth;
>> -    return node;
>> +    /* See linux Documentation/devictree/bindings/arm/gic.txt */
>
> devicetree.
>
>> +    dt_set_cell(&cells, 1, 1); /* is a PPI */
>> +    dt_set_cell(&cells, 1, irq - 16); /* PPIs start at 16 */
>> +    dt_set_cell(&cells, 1, (cpumask << 8) | level);
>>   }
>>
>> -static void make_hypervisor_node(void *fdt, int addrcells, int sizecells)
>> +static int make_hypervisor_node(void *fdt, const struct dt_device_node *parent)
>>   {
>>       const char compat[] =
>>           "xen,xen-"__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)"\0"
>>           "xen,xen";
>> -    u32 reg[4];
>> -    u32 intr[3];
>> -    u32 *cell;
>> +    __be32 reg[4];
>> +    interrupt_t intr[1];
>
> A single element array seems a bit pointless in this context.
>
>> @@ -463,21 +418,39 @@ static int handle_node(struct domain *d, const struct dt_device_node *np)
>>               return res;
>>       }
>>
>> +    /*
>> +     * The property "name" is used to have a different name on older FDT
>
> "is used" => "used"
>
>> +     * version. We want to keep the name retrieved during the tree
>> +     * structure creation, that is store in the node path.
>
> "stored"
>
> This comment is saying that the name of the name property used to be
> something else? What was it? Which version of FDT was that -- do we need
> to care?

Right, on older FDT version (< 0x10) each node has 2 different name:
   - the name just after FDT_BEGIN_NODE in the fdt which correspond to 
the "filename".
   - the name in property "name" which is a convenient name.

So we can't use the name field in device tree to retrieve the name to 
create the node.

For the FDT version, I don't know if we need to care. Linux pays 
attention to it in the device tree code.

-- 
Julien Grall