From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v4 1/6] xen: dt: add dt_for_each_irq_map helper Date: Fri, 26 Jun 2015 19:47:44 +0200 Message-ID: <558D9040.7070503@citrix.com> References: <1431084401.2660.440.camel@citrix.com> <1431084420-14372-1-git-send-email-ian.campbell@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1431084420-14372-1-git-send-email-ian.campbell@citrix.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 , xen-devel@lists.xen.org Cc: julien.grall@linaro.org, tim@xen.org, stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org Hi Ian, On 08/05/2015 13:26, Ian Campbell wrote: > This function iterates over a nodes interrupt-map property and calls a > callback for each interrupt. For now it only supplies the translated > IRQ since my use case has no need of e.g. child unit address. These > can be added as needed by any future users. > > This follows much the same logic as dt_irq_map_raw when parsing the > interrupt-map, but doesn't walk up the tree doing the actual > translation and it iterates over all entries instead of just looking > for the first match. > > I looked into refactoring dt_irq_map_raw but I couldn't find a way > which I was confident in, plus I was reluctant to diverge from the > Linux roots of this function any further. > > Signed-off-by: Ian Campbell Reviewed-by: Julien Grall > --- > v4: Pass a dt_irq not a dt_irq_raw to the callback. > --- > xen/common/device_tree.c | 148 +++++++++++++++++++++++++++++++++++++++++ > xen/include/xen/device_tree.h | 12 ++++ > 2 files changed, 160 insertions(+) > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c > index 02cae91..a2aeecd 100644 > --- a/xen/common/device_tree.c > +++ b/xen/common/device_tree.c > @@ -811,6 +811,154 @@ unsigned int dt_number_of_address(const struct dt_device_node *dev) > return (psize / onesize); > } > > +int dt_for_each_irq_map(const struct dt_device_node *dev, > + int (*cb)(const struct dt_device_node *, > + const struct dt_irq *, > + void *), > + void *data) > +{ > + const struct dt_device_node *ipar, *tnode, *old = NULL; > + const __be32 *tmp, *imap; > + u32 intsize = 1, addrsize, pintsize = 0, paddrsize = 0; > + u32 imaplen; > + int i, ret; > + > + struct dt_raw_irq dt_raw_irq; > + struct dt_irq dt_irq; > + > + dt_dprintk("%s: par=%s cb=%p data=%p\n", __func__, > + dev->full_name, cb, data); > + > + ipar = dev; > + > + /* First get the #interrupt-cells property of the current cursor > + * that tells us how to interpret the passed-in intspec. If there > + * is none, we are nice and just walk up the tree > + */ > + do { > + tmp = dt_get_property(ipar, "#interrupt-cells", NULL); > + if ( tmp != NULL ) > + { > + intsize = be32_to_cpu(*tmp); > + break; > + } > + tnode = ipar; > + ipar = dt_irq_find_parent(ipar); > + } while ( ipar ); This loop doesn't seem useful. AFAIU the spec, the PCI node (i.e your variable dev) will always have property #interrupt-cells. We will break directly. [..] > + if ( intsize > DT_MAX_IRQ_SPEC ) > + { > + dt_dprintk(" -> too many irq specifier cells\n"); > + goto fail; > + } [..] > + /* Parse interrupt-map */ > + while ( imaplen > (addrsize + intsize + 1) ) > + { > + /* skip child unit address and child interrupt specifier */ > + imap += addrsize + intsize; > + imaplen -= addrsize + intsize; > + > + /* Get the interrupt parent */ > + ipar = dt_find_node_by_phandle(be32_to_cpup(imap)); [..] > + /* Get #interrupt-cells and #address-cells of new > + * parent > + */ > + tmp = dt_get_property(ipar, "#interrupt-cells", NULL); > + if ( tmp == NULL ) > + { > + dt_dprintk(" -> parent lacks #interrupt-cells!\n"); > + goto fail; > + } > + pintsize = be32_to_cpu(*tmp); [..] > + dt_raw_irq.controller = ipar; > + dt_raw_irq.size = pintsize; Don't you need to check that pintsize is < DT_MAX_IRQ_SPEC? The previous "if ( ... > DT_MAX_IRQ_SPEC )" will likely be done on a different parent. For instance with the following incomplete DT (based on the apm-storm.dsi in Linux): pcie0 { #interrupt-cells = <1>; ... #interrupt-map = < 0x0 0x0 0x0 0x1 &gic ... > } The first ipar will point to pcie0 because it has a property "#interrupt-cells", while the second time ipar will point to the gic node. > + for ( i = 0; i < pintsize; i++ ) > + dt_raw_irq.specifier[i] = dt_read_number(imap + i, 1); > + > + ret = dt_irq_translate(&dt_raw_irq, &dt_irq); > + if ( ret < 0 ) The other caller of dt_irq_translate returns an error when ret is not 0. I would do the same here. Regards, -- Julien Grall