[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/2019 17:10, Oleksandr wrote:
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.
I don't think it is important to know why Renesas is using it. What matter is 
the property allows to describe in DT a device with interrupts coming from 
multiple interrupt controllers.
In other words, what I ask is explaining in the commit message what this 
property is used for and properly a pointer to the bindings helping the reviewer 
to find out what you speak about.

+    /* 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...
Sorry, I thought the message was duplicated. However, I still think a message 
telling which property is used would be useful.


+        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.
But I misread the code. Sorry for the noise.

Cheers,

--
Julien Grall

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.