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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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

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