Extending the Marvell ICU support

Extending the Marvell ICU support

[Thomas Petazzoni]

Hello Marc,

Back in June 2017, you helped me designing the ICU irqchip driver (in
drivers/irqchip/irq-mvebu-icu.c), and we now need to extend it to
support new functionality, and it would be useful to have your insight
on how to implement this new functionality.

Recap of what we have today
===========================

Marvell Armada 7K/8K are split in two parts: the AP side (with the CPU
core, GIC, and a few peripherals) and the CP side (with most
high-performance I/Os).

The GIC on the AP side has 2 ranges of 64 SPIs that are reserved from
interrupts coming from devices in the CP. The AP has a piece of
hardware that Marvell calls the GICP, which provides two registers to
set/clear an interrupt within those two ranges of 64 SPIs.

Each CP has an ICU unit, which turns the wired interrupts coming from
the devices inside the CP into message interrupts, that are signaled to
the AP by writing to the GICP registers, in the end triggering a SPI at
the GIC level.

In Linux, we have modeled the gicp as an MSI provider, and the ICU as a
MSI consumer.

Thanks to this, we handle all the NSR (Non Secured) interrupts from
devices in the CP.

What we need now
================

In addition to handling NSRs, the ICU also handles SEIs, which stands
for System Error Interrupt. Some devices in the CP raise SEIs instead
of NSRs.

On the AP side, 64 SEIs can be handled. Whenever one SEI is pending,
there is a single GIC interrupt that gets raised. Then 2 32-bit
registers called GICP_SECR0 and GICP_SECR1 can be used to find out
which SEI interrupt was raised.

Among those 64 SEIs, the first 21 are reserved for interrupts coming
from the AP, while the remaining ones are available for SEI interrupts
coming from the CPs. The ICU can be configured to write to a register
called GICP_SET_SEI register to raise a SEI.

Contrary to the NSR interrupts where there is a 1:1 mapping between one
ICU interrupt and one SPI interrupt in the GIC, for the SEI interrupts
there is a single GIC interrupt, and a de-multiplexing to find out
which specific SEI was raised.

The diagram at https://bootlin.com/~thomas/icu.pdf should hopefully
help understand a bit the situation. I hope you'll enjoy my drawing
skills :-)

Our question is how to model this in the irqchip/irqdomain world.

We were thinking of creating a gicp-sei Device Tree node, and a
corresponding irqchip driver. This irqchip would provide 21 wired
interrupts for the devices in the AP, and a MSI domain of 64-21
interrupts available for the CP.

Then, the ICU would have two msi-parents: the gicp used for NSR
interrupts, and the gicp-sei for the SEI interrupts. However, having
multiple MSI parents is currently not supported, and this is probably a
hint that we're going in the wrong direction.

Here is what the DT representation could look like:

ap {
	interrupt-parent = <&gic>;

	gic: interrupt-controller at ... { ... };

	gicp: interrupt-controller at ... {
		compatible = "marvell,ap806-gicp";
		reg = <0x3f0040 0x10>;
		marvell,spi-ranges = <64 64>, <288 64>;
		msi-controller;
	};

	gicp_sei: interrupt-controller at ... {
		compatible = "marvell,ap806-gicp-sei";
		reg = <...>;
		interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
		interrupt-controller;
		msi-controller;
	};

	device {
		... some random device in the AP that uses a SEI ...
		interrupt-extended = <&gicp_sei 12>;
	};
};

cp {
	interrupt-parent = <&icu>;

	icu: interrupt-controller at ... {
		compatible = "marvell,cp110-icu";
		reg = <...>;
		#interrupt-cells = <3>;
		interrupt-controller;
		msi-parent = <&gicp>, <&gicp_sei>;
	};

	/* This device uses a NSR */
	rtc {
		reg = <...>;
		interrupts = <ICU_GRP_NSR 77 IRQ_TYPE_LEVEL_HIGH>;
	};

	some-other-device {
		reg = <...>;
		interrupts = <ICU_GRP_SEI 12 IRQ_TYPE_LEVEL_HIGH>;
	};
};

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.

Thanks a lot in advance for your help!

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

Extending the Marvell ICU support

[Marc Zyngier]

Hi Thomas,

On 04/04/18 15:04, Thomas Petazzoni wrote:
> Hello Marc,
> 
> Back in June 2017, you helped me designing the ICU irqchip driver (in
> drivers/irqchip/irq-mvebu-icu.c), and we now need to extend it to
> support new functionality, and it would be useful to have your insight
> on how to implement this new functionality.
> 
> Recap of what we have today
> ===========================
> 
> Marvell Armada 7K/8K are split in two parts: the AP side (with the CPU
> core, GIC, and a few peripherals) and the CP side (with most
> high-performance I/Os).
> 
> The GIC on the AP side has 2 ranges of 64 SPIs that are reserved from
> interrupts coming from devices in the CP. The AP has a piece of
> hardware that Marvell calls the GICP, which provides two registers to
> set/clear an interrupt within those two ranges of 64 SPIs.
> 
> Each CP has an ICU unit, which turns the wired interrupts coming from
> the devices inside the CP into message interrupts, that are signaled to
> the AP by writing to the GICP registers, in the end triggering a SPI at
> the GIC level.
> 
> In Linux, we have modeled the gicp as an MSI provider, and the ICU as a
> MSI consumer.
> 
> Thanks to this, we handle all the NSR (Non Secured) interrupts from
> devices in the CP.
> 
> What we need now
> ================
> 
> In addition to handling NSRs, the ICU also handles SEIs, which stands
> for System Error Interrupt. Some devices in the CP raise SEIs instead
> of NSRs.
> 
> On the AP side, 64 SEIs can be handled. Whenever one SEI is pending,
> there is a single GIC interrupt that gets raised. Then 2 32-bit
> registers called GICP_SECR0 and GICP_SECR1 can be used to find out
> which SEI interrupt was raised.
> 
> Among those 64 SEIs, the first 21 are reserved for interrupts coming
> from the AP, while the remaining ones are available for SEI interrupts
> coming from the CPs. The ICU can be configured to write to a register
> called GICP_SET_SEI register to raise a SEI.
> 
> Contrary to the NSR interrupts where there is a 1:1 mapping between one
> ICU interrupt and one SPI interrupt in the GIC, for the SEI interrupts
> there is a single GIC interrupt, and a de-multiplexing to find out
> which specific SEI was raised.

That's pretty annoying. Having such a mix of different structures makes
you wonder what's going on in the head of the guy doing the system
integration...

> 
> The diagram at https://bootlin.com/~thomas/icu.pdf should hopefully
> help understand a bit the situation. I hope you'll enjoy my drawing
> skills :-)
> 
> Our question is how to model this in the irqchip/irqdomain world.
> 
> We were thinking of creating a gicp-sei Device Tree node, and a
> corresponding irqchip driver. This irqchip would provide 21 wired
> interrupts for the devices in the AP, and a MSI domain of 64-21
> interrupts available for the CP.
> 
> Then, the ICU would have two msi-parents: the gicp used for NSR
> interrupts, and the gicp-sei for the SEI interrupts. However, having
> multiple MSI parents is currently not supported, and this is probably a
> hint that we're going in the wrong direction.
> 
> Here is what the DT representation could look like:
> 
> ap {
> 	interrupt-parent = <&gic>;
> 
> 	gic: interrupt-controller at ... { ... };
> 
> 	gicp: interrupt-controller at ... {
> 		compatible = "marvell,ap806-gicp";
> 		reg = <0x3f0040 0x10>;
> 		marvell,spi-ranges = <64 64>, <288 64>;
> 		msi-controller;
> 	};
> 
> 	gicp_sei: interrupt-controller at ... {
> 		compatible = "marvell,ap806-gicp-sei";
> 		reg = <...>;
> 		interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
> 		interrupt-controller;
> 		msi-controller;
> 	};
> 
> 	device {
> 		... some random device in the AP that uses a SEI ...
> 		interrupt-extended = <&gicp_sei 12>;
> 	};
> };
> 
> cp {
> 	interrupt-parent = <&icu>;
> 
> 	icu: interrupt-controller at ... {
> 		compatible = "marvell,cp110-icu";
> 		reg = <...>;
> 		#interrupt-cells = <3>;
> 		interrupt-controller;
> 		msi-parent = <&gicp>, <&gicp_sei>;
> 	};
> 
> 	/* This device uses a NSR */
> 	rtc {
> 		reg = <...>;
> 		interrupts = <ICU_GRP_NSR 77 IRQ_TYPE_LEVEL_HIGH>;
> 	};
> 
> 	some-other-device {
> 		reg = <...>;
> 		interrupts = <ICU_GRP_SEI 12 IRQ_TYPE_LEVEL_HIGH>;
> 	};
> };
> 
> 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.

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.

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 guess that's the price to pay for HW that wasn't completely described
on day-1.

Hope this helps,

	M.
-- 
Jazz is not dead. It just smells funny...

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

Extending the Marvell ICU support

[Marc Zyngier]

On 05/04/18 10:04, Thomas Petazzoni wrote:
> 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 ?

Yes, that's the idea. The alternative would have been to have a single
GICP domain and to route everything there, but the fact that SEIs are
multiplexed entirely kills that prospect. Blame the HW folks.

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

In that case, you can probably achieve "old DT with new kernel" at the
cost of checking the size of the interrupt specifier in your translate
method.

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

It probably is. I'm not going to enter the debate on DT compatibility
either, because all the FW descriptions we have are absolutely pathetic
in that regard.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Extending the Marvell ICU support

[Thomas Petazzoni]

Hello,

On Thu, 5 Apr 2018 10:27:24 +0100, Marc Zyngier wrote:

> > 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 ?  
> 
> Yes, that's the idea. The alternative would have been to have a single
> GICP domain and to route everything there, but the fact that SEIs are
> multiplexed entirely kills that prospect. Blame the HW folks.

A quick question: the ICU in total can handle N wired interrupts, and
it has N registers to configure how those interrupts are routed to the
AP. Therefore, we have a global set of N wired interrupts, each can be
either a NSR, a SEI, a REI, etc.

Currently, the ICU driver registers a single irq domain of
ICU_MAX_IRQS. If we move to your solution with sub-nodes, we will have
to register several irq domains. Is it OK if each of them exposes
ICU_MAX_IRQS, even if in practice you won't be able to use both the
interrupt icu_nsr[2] and the interrupt icu_sei[2] ?

So we will expose 2 * ICU_MAX_IRQS, but in practice only a total of
ICU_MAX_IRQS can be used. We would probably use a bitmap or something
like that in the driver to know which ICU is already used, and which is
still available. Indeed, a configuration such as:

	dev1 {
		interrupt-extended = <&icu_nsr 2 IRQ_TYPE_LEVEL_HIGH>;
	}

	dev2 {
		interrupt-extended = <&icu_sei 2 IRQ_TYPE_LEVEL_HIGH>;
	}

is not valid, because the ICU n?2 can only be either a NSR *or* a SEI.

Does that make sense ?

> > 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.  
> 
> In that case, you can probably achieve "old DT with new kernel" at the
> cost of checking the size of the interrupt specifier in your translate
> method.

Yep, I think backward compatibility can be achieved. It's going to
clutter the ICU driver implementation with dead code that is never
tested, but that's the whole point of DT backward compatibility.

Best regards,

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

Extending the Marvell ICU support

[Marc Zyngier]

On 05/04/18 10:51, Thomas Petazzoni wrote:
> Hello,
> 
> On Thu, 5 Apr 2018 10:27:24 +0100, Marc Zyngier wrote:
> 
>>> 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 ?  
>>
>> Yes, that's the idea. The alternative would have been to have a single
>> GICP domain and to route everything there, but the fact that SEIs are
>> multiplexed entirely kills that prospect. Blame the HW folks.
> 
> A quick question: the ICU in total can handle N wired interrupts, and
> it has N registers to configure how those interrupts are routed to the
> AP. Therefore, we have a global set of N wired interrupts, each can be
> either a NSR, a SEI, a REI, etc.
> 
> Currently, the ICU driver registers a single irq domain of
> ICU_MAX_IRQS. If we move to your solution with sub-nodes, we will have
> to register several irq domains. Is it OK if each of them exposes
> ICU_MAX_IRQS, even if in practice you won't be able to use both the
> interrupt icu_nsr[2] and the interrupt icu_sei[2] ?
> 
> So we will expose 2 * ICU_MAX_IRQS, but in practice only a total of
> ICU_MAX_IRQS can be used. We would probably use a bitmap or something
> like that in the driver to know which ICU is already used, and which is
> still available. Indeed, a configuration such as:
> 
> 	dev1 {
> 		interrupt-extended = <&icu_nsr 2 IRQ_TYPE_LEVEL_HIGH>;
> 	}
> 
> 	dev2 {
> 		interrupt-extended = <&icu_sei 2 IRQ_TYPE_LEVEL_HIGH>;
> 	}
> 
> is not valid, because the ICU n?2 can only be either a NSR *or* a SEI.
> 
> Does that make sense ?

It makes perfect sense, and I think that's fine. In order to avoid
always wasting ICU_MAX_IRQS entries, you could change the type of irq
domain you create from linear to tree. You pay a very small additional
hit on lookup, but that is probably immaterial.

You could benchmark it and decide for yourself.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...