Re: [Xen-devel] [RFC PATCH 2/2] xen/device-tree: Add ability to handle nodes with interrupts-extended prop

[Oleksandr <olekstysh@xxxxxxxxx>]

On 20.05.19 15:25, Julien Grall wrote:
> Hi,
Hi, Julien.


> On 02/05/2019 15:13, Oleksandr Tyshchenko wrote:
> > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> >
> > Xen expects to see "interrupts" property when parsing host
> > device-tree. But, there are cases when some device nodes contain
> > "interrupts-extended" property instead.
> >
> > The good example here is arch timer node for R-Car Gen3/Gen2 family,
> > which is mandatory device for Xen usage on ARM. And without ability
> > to handle such nodes, Xen fails to operate:
> Per the binding documentation [1], the interrupts-extend property 
> should only be used when a device has multiple interrupt parents. This 
> is not the case of the arch timer, so why is it used there?
> Don't get me wrong, I am fine with the idea of adding 
> "interrupts-extend". However, the commit message should give some 
> ground why a new property has been introduced/used over the current one.
Have just grepped, looks like, R-Car Gen2/Gen3 dtsi files are not the 
only single users of "interrupts-extended" property for a device with a 
single interrupt parent...
Unfortunately, I don't know the real reason, can guess only that for a 
device (with a single interrupt parent) outside "/soc" container the 
usage of single "interrupts-extended" property is more simpler/cleaner 
than usage of pairs ("interrupt-parent" + "interrupts").  Looks like, 
the patch "ARM: dts: r8a7790: add soc node" from this series [1] started 
using "interrupts-extended" property for ARCH timer node. I will mention 
that in patch description.

> > +    /* Try the new-style interrupts-extended first */
> > +    intnum = dt_count_phandle_with_args(device, "interrupts-extended",
> > +                                        "#interrupt-cells");
> > +    if ( intnum > 0 )
> IIUC dt_count_phandle_with_args, 0 would means the property is present 
> but doesn't contain any interrupts. I do agree this is a probably a 
> wrong device-tree, but technically I am not sure we should try to look 
> for "#interrupts" if intnum = 0.
agree, will return 0 if intnum == 0


> > +    {
> > +        dt_dprintk(" intnum=%d\n", intnum);
> You are re-using the exact same debug message as for "interrupts". So 
> it would be difficult for a developer to know exactly which path is 
> used. Could we print message regarding whether "interrupts-extended" 
> or "interrupts" is used?
I couldn't find where else the same debug message was used, could you, 
please, point me? But, I don't mind to add some indicator. For 
"interrupts-extended" path (newly added prints) I can add the 
corresponding prefix...

> > +        return intnum;
> > +    }
> > +
> >       /* Get the interrupts property */
> >       intspec = dt_get_property(device, "interrupts", &intlen);
> >       if ( intspec == NULL )
> > @@ -1420,10 +1430,29 @@ int dt_device_get_raw_irq(const struct 
> > dt_device_node *device,
> >       const __be32 *intspec, *tmp, *addr;
> >       u32 intsize, intlen;
> >       int res = -EINVAL;
> > +    struct dt_phandle_args args;
> > +    int i;
> >         dt_dprintk("dt_device_get_raw_irq: dev=%s, index=%u\n",
> >                  device->full_name, index);
> >   +    /* Get the reg property (if any) */
> > +    addr = dt_get_property(device, "reg", NULL);
> > +
> > +    /* Try the new-style interrupts-extended first */
> > +    res = dt_parse_phandle_with_args(device, "interrupts-extended",
> > +                                     "#interrupt-cells", index, &args);
> > +    if ( !res )
> I don't think the check is correct. dt_parse_phandle_with_args may 
> return a negative value in case of an error. So we likely want "res >= 
> 0" here.
I am not sure I understand your point correctly. Why do we need to check 
for res > 0 as well?
If index is not -1, then function will return either 0 (on success) or 
-ERR_XXX.

> > +    {
> > +        dt_dprintk(" intspec=%d intsize=%d\n", args.args[0], 
> > args.args_count);
> Same remark for the message here.
agree.


[1] https://www.spinics.net/lists/linux-renesas-soc/msg22539.html

--
Regards,

Oleksandr Tyshchenko


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel