| Message ID | 1556806436-26283-3-git-send-email-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add ability to handle nodes with interrupts-extended property | expand |
Hi, On 02/05/2019 15:13, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > 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. > > (XEN) **************************************** > (XEN) Panic on CPU 0: > (XEN) Timer: Unable to retrieve IRQ 0 from the device tree > (XEN) **************************************** > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > --- > xen/common/device_tree.c | 32 +++++++++++++++++++++++++++++--- > 1 file changed, 29 insertions(+), 3 deletions(-) > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c > index 65862b5..00ada6e 100644 > --- a/xen/common/device_tree.c > +++ b/xen/common/device_tree.c > @@ -987,9 +987,19 @@ unsigned int dt_number_of_irq(const struct dt_device_node *device) > const struct dt_device_node *p; > const __be32 *intspec, *tmp; > u32 intsize, intlen; > + int intnum; > > dt_dprintk("dt_irq_number: dev=%s\n", device->full_name); > > + /* 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. > + { > + 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? > + 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. > + { > + dt_dprintk(" intspec=%d intsize=%d\n", args.args[0], args.args_count); Same remark for the message here. > + > + for ( i = 0; i < args.args_count; i++ ) > + args.args[i] = cpu_to_be32(args.args[i]); > + > + return dt_irq_map_raw(args.np, args.args, args.args_count, > + addr, out_irq); > + } > + > /* Get the interrupts property */ > intspec = dt_get_property(device, "interrupts", &intlen); > if ( intspec == NULL ) > @@ -1432,9 +1461,6 @@ int dt_device_get_raw_irq(const struct dt_device_node *device, > > dt_dprintk(" intspec=%d intlen=%d\n", be32_to_cpup(intspec), intlen); > > - /* Get the reg property (if any) */ > - addr = dt_get_property(device, "reg", NULL); > - > /* Look for the interrupt parent. */ > p = dt_irq_find_parent(device); > if ( p == NULL ) > Cheers, [1] linux/Documentation/devicetree/bindings/interrupt-controller
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@epam.com> >> >> 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
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@epam.com> >>> >>> 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,
Hi, Julien >> >> >>> >>> On 02/05/2019 15:13, Oleksandr Tyshchenko wrote: >>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>> >>>> 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. OK. Sounds reasonable. Will add an information regarding the property itself with link. Should I retain the original sentences (regarding ARCH timer on R-Car uses it, etc) as well? >> >> >>> >>>> + { >>>> + 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. Just to clarify: should I add for the newly added messages ("interrupts-extended" path) only? Or I should modify existing messages for "interrupts" path also?
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c index 65862b5..00ada6e 100644 --- a/xen/common/device_tree.c +++ b/xen/common/device_tree.c @@ -987,9 +987,19 @@ unsigned int dt_number_of_irq(const struct dt_device_node *device) const struct dt_device_node *p; const __be32 *intspec, *tmp; u32 intsize, intlen; + int intnum; dt_dprintk("dt_irq_number: dev=%s\n", device->full_name); + /* Try the new-style interrupts-extended first */ + intnum = dt_count_phandle_with_args(device, "interrupts-extended", + "#interrupt-cells"); + if ( intnum > 0 ) + { + dt_dprintk(" intnum=%d\n", intnum); + 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 ) + { + dt_dprintk(" intspec=%d intsize=%d\n", args.args[0], args.args_count); + + for ( i = 0; i < args.args_count; i++ ) + args.args[i] = cpu_to_be32(args.args[i]); + + return dt_irq_map_raw(args.np, args.args, args.args_count, + addr, out_irq); + } + /* Get the interrupts property */ intspec = dt_get_property(device, "interrupts", &intlen); if ( intspec == NULL ) @@ -1432,9 +1461,6 @@ int dt_device_get_raw_irq(const struct dt_device_node *device, dt_dprintk(" intspec=%d intlen=%d\n", be32_to_cpup(intspec), intlen); - /* Get the reg property (if any) */ - addr = dt_get_property(device, "reg", NULL); - /* Look for the interrupt parent. */ p = dt_irq_find_parent(device); if ( p == NULL )