From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: [PATCH] xen: get GIC addresses from DT Date: Fri, 30 Nov 2012 13:15:34 +0000 Message-ID: <50B8B176.3070206@citrix.com> References: <50B897E1.70306@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini Cc: Anthony Perard , "xen-devel@lists.xensource.com" , Ian Campbell List-Id: xen-devel@lists.xenproject.org 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