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

[Oleksandr <olekstysh@xxxxxxxxx>]

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.
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?

--
Regards,

Oleksandr Tyshchenko


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