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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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?

> -/* 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.

>  
> -        /* 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?

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel