From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH V1 16/29] xen/arm: Build DOM0 FDT by browsing the device tree structure Date: Mon, 09 Sep 2013 13:26:46 +0100 Message-ID: <522DBE86.4070607@linaro.org> References: <1377701263-3319-1-git-send-email-julien.grall@linaro.org> <1377701263-3319-17-git-send-email-julien.grall@linaro.org> <1378726404.19967.67.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1378726404.19967.67.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: stefano.stabellini@eu.citrix.com, andre.przywara@linaro.org, patches@linaro.org, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.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