Extending the Marvell ICU support

[Thomas Petazzoni]

Hello Marc,

Thanks for your feedback!

On Thu, 5 Apr 2018 09:54:13 +0100, Marc Zyngier wrote:

> > However, as explained above, having multiple msi-parents is currently
> > not supported. Should we add support for that ? Do you see a
> > different/better way of supporting our use case ?
> > 
> > It is worth mentioning that the ICU also support REIs, which work like
> > the SEIs: there is a single GIC interrupt (n°33), one register to raise
> > a REI (GICP_SET_REI) and two 32-bits registers (GICP_RECR0/GICP_RECR1)
> > to find which REI was raised. So we might have to support this in the
> > future as well. REI stands for RAM Error Interrupt.
> > 
> > As usual, I'm available on IRC to discuss this live. I'm sure there are
> > some bits of information that I forgot, and that will be needed to
> > fully understand what our situation is.  
> 
> To sum up the discussion we had yesterday on IRC:
> 
> Multiple MSI parents, despite being easy to express in DT, are a real
> pain in the kernel, because each device allocates its MSIs from a
> single, implicit name-space (its directly attached msi_domain). Having
> more than one means redesigning the whole generic MSI API to deal with
> multiple domains. It then raises the question of how you access a
> domain. How do you get a reference to it? Do we keep the additional
> domains at the device level, or somewhere else? What's the notion of a
> default MSI domain? This turns the whole logic upside down, and I'm not
> even adding ACPI to the mix...
> 
> I'm not saying this is impossible to achieve, but that's so disruptive
> that this may not be easy to achieve within a reasonable time frame.

Yes, it is indeed a significant change.

> On the other hand, wired interrupts are just as easy to express in DT,
> and because everything is explicit (domain, interrupt number,
> configuration), you can actually implement something that works both in
> DT and in the kernel.
> 
> What I'm suggesting is the following:
> 
> ap {
> 	[...]
> };
> 
> cp {
> 	interrupt-parent = <&icu>;
> 
> 	icu: interrupt-controller at ... {
> 		compatible = "marvell,cp110-icu";
> 		reg = <...>;
> 
> 		icu-nsr: icu-nsr {
> 			#interrupt-cells = <2>;
> 			interrupt-controller;
> 			msi-parent = <&gicp>;
> 		};
> 
> 		icu-sei: icu-sei {
> 			#interrupt-cells = <2>;
> 			interrupt-controller;
> 			msi-parent = <&gicp-sei>;
> 		};
> 	};
> 
> 	/* This device uses a NSR */
> 	rtc {
> 		reg = <...>;
> 		interrupts-extended = <&icu-nsr 77 IRQ_TYPE_LEVEL_HIGH>;
> 	};
> 
> 	some-other-device {
> 		reg = <...>;
> 		interrupts-extended = <&icu-sei 12 IRQ_TYPE_LEVEL_HIGH>;
> 	};
> };
> 
> which should make things work in a pretty obvious way.

So you're suggesting that the ICU driver registers multiple irq
domains, one for NSR, one for SEI, each having its own MSI parent,
correct ?

I of course haven't tried it, but it feels like something that should
work.

> Of course, the main issue is that it completely breaks DT compatibility.
> You can probably make a new kernel work with an old DT, but a new DT on
> an older kernel is just going to catch fire.

I don't think "a new DT on an older kernel" has ever been a
requirement, has it? The whole idea of DT ABI compatibility is that an
old DT shipped on a board continues to work with newer kernel versions.

> I guess that's the price to pay for HW that wasn't completely described
> on day-1.

Isn't "HW not completely described on day 1" the norm rather than the
exception ? But oh well, I won't re-open this whole DT backward
compatibility discussion.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com