[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH 2/2] xen/device-tree: Add ability to handle nodes with interrupts-extended prop
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |