[PATCH] irqchip/gic-v3: Support v2m frame backwards compatibility mode

[PATCH] irqchip/gic-v3: Support v2m frame backwards compatibility mode

[Stephen Boyd]

Some GIC configurations don't have an ITS or v2m frame, but they
want to support MSIs through the distributor's "v2m backwards
compatible" mode. This mode allows software written for the v2m
to treat the distributor as the only frame and support a limited
number of MSIs through a direct write to the same register offset
(0x40) as what exists in v2m.

Support this mode of operation by detecting if a gicv3 device
node has the "msi-controller" property, and then probe the v2m
frame code on top of the distributor base. Rely on existing v2m
DT properties to tell us about the number of SPIs and where they
start from because the GICD_TYPER register doesn't tell us this
information.

Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/irqchip/irq-gic-v2m.c          | 66 ++++++++++++++++++++++++----------
 drivers/irqchip/irq-gic-v3.c           |  4 +++
 include/linux/irqchip/arm-gic-common.h |  3 ++
 3 files changed, 54 insertions(+), 19 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v2m.c b/drivers/irqchip/irq-gic-v2m.c
index 863e073c6f7f..4e0d3493c510 100644
--- a/drivers/irqchip/irq-gic-v2m.c
+++ b/drivers/irqchip/irq-gic-v2m.c
@@ -26,6 +26,7 @@
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/irqchip/arm-gic.h>
+#include <linux/irqchip/arm-gic-common.h>
 
 /*
 * MSI_TYPER:
@@ -388,6 +389,36 @@ static struct of_device_id gicv2m_device_id[] = {
 	{},
 };
 
+static int __init giv2m_of_init_one(struct device_node *child, bool force)
+{
+	u32 spi_start = 0, nr_spis = 0;
+	struct resource res;
+	int ret;
+
+	if (!of_find_property(child, "msi-controller", NULL))
+		return force ? -EINVAL : 0;
+
+	ret = of_address_to_resource(child, 0, &res);
+	if (ret) {
+		pr_err("Failed to allocate v2m resource\n");
+		return ret;
+	}
+
+	if (!of_property_read_u32(child, "arm,msi-base-spi",
+				  &spi_start) &&
+	    !of_property_read_u32(child, "arm,msi-num-spis", &nr_spis))
+		if (!force)
+			pr_info("DT overriding V2M MSI_TYPER (base:%u, num:%u)\n",
+				spi_start, nr_spis);
+
+	if (force && !nr_spis) {
+		pr_err("Can't emulate v2m without num-spis\n");
+		return -EINVAL;
+	}
+
+	return gicv2m_init_one(&child->fwnode, spi_start, nr_spis, &res);
+}
+
 static int __init gicv2m_of_init(struct fwnode_handle *parent_handle,
 				 struct irq_domain *parent)
 {
@@ -397,25 +428,7 @@ static int __init gicv2m_of_init(struct fwnode_handle *parent_handle,
 
 	for (child = of_find_matching_node(node, gicv2m_device_id); child;
 	     child = of_find_matching_node(child, gicv2m_device_id)) {
-		u32 spi_start = 0, nr_spis = 0;
-		struct resource res;
-
-		if (!of_find_property(child, "msi-controller", NULL))
-			continue;
-
-		ret = of_address_to_resource(child, 0, &res);
-		if (ret) {
-			pr_err("Failed to allocate v2m resource.\n");
-			break;
-		}
-
-		if (!of_property_read_u32(child, "arm,msi-base-spi",
-					  &spi_start) &&
-		    !of_property_read_u32(child, "arm,msi-num-spis", &nr_spis))
-			pr_info("DT overriding V2M MSI_TYPER (base:%u, num:%u)\n",
-				spi_start, nr_spis);
-
-		ret = gicv2m_init_one(&child->fwnode, spi_start, nr_spis, &res);
+		ret = giv2m_of_init_one(child, false);
 		if (ret) {
 			of_node_put(child);
 			break;
@@ -518,6 +531,21 @@ static int __init gicv2m_acpi_init(struct irq_domain *parent)
 }
 #endif /* CONFIG_ACPI */
 
+int __init gicv2m_init_gicv3(struct fwnode_handle *handle,
+			     struct irq_domain *parent)
+{
+	int ret;
+	struct device_node *node = to_of_node(handle);
+
+	ret = giv2m_of_init_one(node, true);
+	if (!ret)
+		ret = gicv2m_allocate_domains(parent);
+	if (ret)
+		gicv2m_teardown();
+
+	return ret;
+}
+
 int __init gicv2m_init(struct fwnode_handle *parent_handle,
 		       struct irq_domain *parent)
 {
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 6c65eb917db6..9051d889c23e 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -1170,6 +1170,10 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
 
 	gic_populate_ppi_partitions(node);
 	gic_of_setup_kvm_info(node);
+
+	if (IS_ENABLED(CONFIG_ARM_GIC_V2M))
+		gicv2m_init_gicv3(&node->fwnode, gic_data.domain);
+
 	return 0;
 
 out_unmap_rdist:
diff --git a/include/linux/irqchip/arm-gic-common.h b/include/linux/irqchip/arm-gic-common.h
index c647b0547bcd..43207a0cbd39 100644
--- a/include/linux/irqchip/arm-gic-common.h
+++ b/include/linux/irqchip/arm-gic-common.h
@@ -31,4 +31,7 @@ struct gic_kvm_info {
 
 const struct gic_kvm_info *gic_get_kvm_info(void);
 
+int gicv2m_init_gicv3(struct fwnode_handle *parent_handle,
+		      struct irq_domain *parent);
+
 #endif /* __LINUX_IRQCHIP_ARM_GIC_COMMON_H */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] irqchip/gic-v3: Support v2m frame backwards compatibility mode

[Marc Zyngier]

On 20/03/17 22:36, Stephen Boyd wrote:
> Some GIC configurations don't have an ITS or v2m frame, but they
> want to support MSIs through the distributor's "v2m backwards
> compatible" mode. This mode allows software written for the v2m
> to treat the distributor as the only frame and support a limited
> number of MSIs through a direct write to the same register offset
> (0x40) as what exists in v2m.
> 
> Support this mode of operation by detecting if a gicv3 device
> node has the "msi-controller" property, and then probe the v2m
> frame code on top of the distributor base. Rely on existing v2m
> DT properties to tell us about the number of SPIs and where they
> start from because the GICD_TYPER register doesn't tell us this
> information.

Hi Stephen,

The whole idea behind this GICD_SETSPI_NSR is to offer a way to signal
SPIs using memory transaction, even allowing level interrupts (in
combinaison with the GICD_CLRSPI_NSR at offset 0x48). This is *not* a
GICv2m at all - only the offset is the same.

The reasoning is that firmware would program the various devices with
the GICD_{CLR,SET}SPI_NSR addresses and the payload, and simply describe
this as an SPI in the device tree. Another reason for doing so is that
while we can always twist the DT to express anything, this cannot be
described in ACPI at all.

Also, as you noticed, there is no provision in the architecture to
describe the range of message-based SPIs, because any SPI can be
signalled through that mechanism. It makes it impossible to distinguish
SPIs that are statically allocated (because it is a real wire) from
those that can be dynamically allocated (because it is just a number).

You end-up having to describe the range of SPIs that can be generated
through messages at least on a per SoC basis, and maybe on a per board
basis. Not to mention that you're still only describing half of the
capability of the HW (what about level interrupts?).

If we really want to support this kind of thing, I'd like to see level
interrupts supported as a first class citizen in our generic MSI
infrastructure, and then the GICv3 message-based SPIs as an client of
that infrastructure.

Thanks,

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

Re: [PATCH] irqchip/gic-v3: Support v2m frame backwards compatibility mode

[Thomas Petazzoni]

Hello Marc, Hello Stephen,

On Tue, 21 Mar 2017 09:43:24 +0000, Marc Zyngier wrote:

> The whole idea behind this GICD_SETSPI_NSR is to offer a way to signal
> SPIs using memory transaction, even allowing level interrupts (in
> combinaison with the GICD_CLRSPI_NSR at offset 0x48). This is *not* a
> GICv2m at all - only the offset is the same.
> 
> The reasoning is that firmware would program the various devices with
> the GICD_{CLR,SET}SPI_NSR addresses and the payload, and simply describe
> this as an SPI in the device tree. Another reason for doing so is that
> while we can always twist the DT to express anything, this cannot be
> described in ACPI at all.
> 
> Also, as you noticed, there is no provision in the architecture to
> describe the range of message-based SPIs, because any SPI can be
> signalled through that mechanism. It makes it impossible to distinguish
> SPIs that are statically allocated (because it is a real wire) from
> those that can be dynamically allocated (because it is just a number).
> 
> You end-up having to describe the range of SPIs that can be generated
> through messages at least on a per SoC basis, and maybe on a per board
> basis. Not to mention that you're still only describing half of the
> capability of the HW (what about level interrupts?).
> 
> If we really want to support this kind of thing, I'd like to see level
> interrupts supported as a first class citizen in our generic MSI
> infrastructure, and then the GICv3 message-based SPIs as an client of
> that infrastructure.

We are trying to support a platform that has a GICv3, and that also
uses level-triggered interrupts through GICD_SETSPI_NSR and
GICD_CLRSPI_NSR. Therefore, I'm also interested in seeing this
functionality of the GICv3 exposed as an MSI controller.

In the current Marvell Armada 7K/8K, we have a unit called the ICU
that turns wired level interrupts on one side of the chip into MSIs,
signaled to the GIC through a special unit called GICP, which allowed
to trigger SPIs in the GIC-400 by doing memory writes. See
irq-mvebu-icu.c and irq-mvebu-gicp.c for the two sides of the story
(MSI consumer and MSI provider). We have one hack between those two
drivers: because those interrupts are level-triggered, we need the
addresses of two registers, while 'struct msi_msg' only allows to pass
one address, assuming MSIs are edge-triggered.

In the upcoming Armada 8KP, we have a GICv3, which has built-in support
for memory-triggered SPIs, thanks to the GICD_SETSPI_NSR and
GICD_CLRSPI_NSR, and the ICU will directly use this GICv3
functionality. We would therefore very much like to have this GICv3
feature provided as a MSI controller, which as Marc said would require
supporting level-triggered MSIs.

Marc, let me know how we can collaborate on this topic. I'm able to
either test some preliminary patches, or work on such patches if
necessary (preferably with some initial directions).

Thanks!

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

Re: [PATCH] irqchip/gic-v3: Support v2m frame backwards compatibility mode

[Marc Zyngier]

Hi Thomas,

On 10/04/18 16:01, Thomas Petazzoni wrote:
> Hello Marc, Hello Stephen,
> 
> On Tue, 21 Mar 2017 09:43:24 +0000, Marc Zyngier wrote:
> 
>> The whole idea behind this GICD_SETSPI_NSR is to offer a way to signal
>> SPIs using memory transaction, even allowing level interrupts (in
>> combinaison with the GICD_CLRSPI_NSR at offset 0x48). This is *not* a
>> GICv2m at all - only the offset is the same.
>>
>> The reasoning is that firmware would program the various devices with
>> the GICD_{CLR,SET}SPI_NSR addresses and the payload, and simply describe
>> this as an SPI in the device tree. Another reason for doing so is that
>> while we can always twist the DT to express anything, this cannot be
>> described in ACPI at all.
>>
>> Also, as you noticed, there is no provision in the architecture to
>> describe the range of message-based SPIs, because any SPI can be
>> signalled through that mechanism. It makes it impossible to distinguish
>> SPIs that are statically allocated (because it is a real wire) from
>> those that can be dynamically allocated (because it is just a number).
>>
>> You end-up having to describe the range of SPIs that can be generated
>> through messages at least on a per SoC basis, and maybe on a per board
>> basis. Not to mention that you're still only describing half of the
>> capability of the HW (what about level interrupts?).
>>
>> If we really want to support this kind of thing, I'd like to see level
>> interrupts supported as a first class citizen in our generic MSI
>> infrastructure, and then the GICv3 message-based SPIs as an client of
>> that infrastructure.
> 
> We are trying to support a platform that has a GICv3, and that also
> uses level-triggered interrupts through GICD_SETSPI_NSR and
> GICD_CLRSPI_NSR. Therefore, I'm also interested in seeing this
> functionality of the GICv3 exposed as an MSI controller.
> 
> In the current Marvell Armada 7K/8K, we have a unit called the ICU
> that turns wired level interrupts on one side of the chip into MSIs,
> signaled to the GIC through a special unit called GICP, which allowed
> to trigger SPIs in the GIC-400 by doing memory writes. See
> irq-mvebu-icu.c and irq-mvebu-gicp.c for the two sides of the story
> (MSI consumer and MSI provider). We have one hack between those two
> drivers: because those interrupts are level-triggered, we need the
> addresses of two registers, while 'struct msi_msg' only allows to pass
> one address, assuming MSIs are edge-triggered.

So effectively, the GICP/GIC400 combination is a poor-man GICv3 MBI
(Message Based Interrupt -- we love overloaded acronyms) implementation.

> In the upcoming Armada 8KP, we have a GICv3, which has built-in support
> for memory-triggered SPIs, thanks to the GICD_SETSPI_NSR and
> GICD_CLRSPI_NSR, and the ICU will directly use this GICv3
> functionality. We would therefore very much like to have this GICv3
> feature provided as a MSI controller, which as Marc said would require
> supporting level-triggered MSIs.
> 
> Marc, let me know how we can collaborate on this topic. I'm able to
> either test some preliminary patches, or work on such patches if
> necessary (preferably with some initial directions).

I have a vague idea how to support this. Given that level-triggered MSIs
have to be platform MSIs (because it is just madness otherwise), we can
probably store an extra message in the struct platform_msi_desc for the
"lower the line" write. On activation, you'd get two callbacks, probably
with a flag of some sort to indicate whether this is for the rising or
falling edge. The thing I'm unclear about so far is how to let the
generic MSI layer know that we're dealing with such an interrupt without
make a total mess of everything. It is probably done by marking the
interrupt level triggered, but there are some corner cases.

And if that works, the PCI stuff will come for free (it is just a matter
of implementing a new irqdomain on top of the base GICv3 one).

I'll try to spend some time on it in the coming couple of weeks, but
will have to rely on you for the testing (as I don't have much in the
way of HW).

Thanks,

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

Re: [PATCH] irqchip/gic-v3: Support v2m frame backwards compatibility mode

[Thomas Petazzoni]

Hello,

Thanks for your feedback!

On Tue, 10 Apr 2018 16:23:00 +0100, Marc Zyngier wrote:

> > In the current Marvell Armada 7K/8K, we have a unit called the ICU
> > that turns wired level interrupts on one side of the chip into MSIs,
> > signaled to the GIC through a special unit called GICP, which allowed
> > to trigger SPIs in the GIC-400 by doing memory writes. See
> > irq-mvebu-icu.c and irq-mvebu-gicp.c for the two sides of the story
> > (MSI consumer and MSI provider). We have one hack between those two
> > drivers: because those interrupts are level-triggered, we need the
> > addresses of two registers, while 'struct msi_msg' only allows to pass
> > one address, assuming MSIs are edge-triggered.  
> 
> So effectively, the GICP/GIC400 combination is a poor-man GICv3 MBI
> (Message Based Interrupt -- we love overloaded acronyms) implementation.

Yes, that's what it is. I thought it was already clear for you, since
you already spent a great deal of time working with me on ICU/GICP back
when it was submitted and merged (and thank you for that!).

> > Marc, let me know how we can collaborate on this topic. I'm able to
> > either test some preliminary patches, or work on such patches if
> > necessary (preferably with some initial directions).  
> 
> I have a vague idea how to support this. Given that level-triggered MSIs
> have to be platform MSIs (because it is just madness otherwise), we can
> probably store an extra message in the struct platform_msi_desc for the
> "lower the line" write.

Is there a problem with extending the msi_msg structure with an
additional address ? It could be transparent for existing users of
msi_msg, who would continue to use just address_lo/address_hi/data,
while users needing level-triggered MSIs would use the new fields in
addition to the existing ones. But perhaps I'm missing something.

> On activation, you'd get two callbacks, probably with a flag of some
> sort to indicate whether this is for the rising or falling edge.

What you call "activation" is when ->write_msg() gets called on the MSI
consumer side, to configure its HW so that it knows how to trigger its
MSIs ?

> The thing I'm unclear about so far is how to let the generic MSI layer
> know that we're dealing with such an interrupt without make a total
> mess of everything. It is probably done by marking the interrupt
> level triggered, but there are some corner cases.

Certainly me not fully understand the generic MSI layer, but why does
it need to be aware of the interrupt being level vs. edge ?

> And if that works, the PCI stuff will come for free (it is just a
> matter of implementing a new irqdomain on top of the base GICv3 one).

I've lost you here :)

> I'll try to spend some time on it in the coming couple of weeks, but
> will have to rely on you for the testing (as I don't have much in the
> way of HW).

I can definitely make some tests, I have actual HW to test this.

Best regards,

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

Re: [PATCH] irqchip/gic-v3: Support v2m frame backwards compatibility mode

[Marc Zyngier]

On 10/04/18 16:41, Thomas Petazzoni wrote:
> Hello,
> 
> Thanks for your feedback!
> 
> On Tue, 10 Apr 2018 16:23:00 +0100, Marc Zyngier wrote:
> 
>>> In the current Marvell Armada 7K/8K, we have a unit called the ICU
>>> that turns wired level interrupts on one side of the chip into MSIs,
>>> signaled to the GIC through a special unit called GICP, which allowed
>>> to trigger SPIs in the GIC-400 by doing memory writes. See
>>> irq-mvebu-icu.c and irq-mvebu-gicp.c for the two sides of the story
>>> (MSI consumer and MSI provider). We have one hack between those two
>>> drivers: because those interrupts are level-triggered, we need the
>>> addresses of two registers, while 'struct msi_msg' only allows to pass
>>> one address, assuming MSIs are edge-triggered.  
>>
>> So effectively, the GICP/GIC400 combination is a poor-man GICv3 MBI
>> (Message Based Interrupt -- we love overloaded acronyms) implementation.
> 
> Yes, that's what it is. I thought it was already clear for you, since
> you already spent a great deal of time working with me on ICU/GICP back
> when it was submitted and merged (and thank you for that!).

I was just trying to give some context here for those who don't really
follow the nightmarish state that we deal with... ;-)

> 
>>> Marc, let me know how we can collaborate on this topic. I'm able to
>>> either test some preliminary patches, or work on such patches if
>>> necessary (preferably with some initial directions).  
>>
>> I have a vague idea how to support this. Given that level-triggered MSIs
>> have to be platform MSIs (because it is just madness otherwise), we can
>> probably store an extra message in the struct platform_msi_desc for the
>> "lower the line" write.
> 
> Is there a problem with extending the msi_msg structure with an
> additional address ? It could be transparent for existing users of
> msi_msg, who would continue to use just address_lo/address_hi/data,
> while users needing level-triggered MSIs would use the new fields in
> addition to the existing ones. But perhaps I'm missing something.

At least two reasons:

- I don't want to put an extra overhead on everyone else, as about 99.9%
of the msi_msg users are sane (read: PCI), and I'm quite happy to put
the overhead on the [expletive] crazy designs.

- The fact that GICv3 requires a different address and the same data is
an implementation detail. Let's say that I invent a new interrupt
controller that requires bit 31 of the data to indicate whether this is
a set or a clear, and uses the same address. Now your scheme doesn't work.

So I really want a different message altogether.

> 
>> On activation, you'd get two callbacks, probably with a flag of some
>> sort to indicate whether this is for the rising or falling edge.
> 
> What you call "activation" is when ->write_msg() gets called on the MSI
> consumer side, to configure its HW so that it knows how to trigger its
> MSIs ?

The write_msi is indeed part of the activation, together with
compose_msg. That's the phase where we actually commit the HW resources,
and plumb everything together.

> 
>> The thing I'm unclear about so far is how to let the generic MSI layer
>> know that we're dealing with such an interrupt without make a total
>> mess of everything. It is probably done by marking the interrupt
>> level triggered, but there are some corner cases.
> 
> Certainly me not fully understand the generic MSI layer, but why does
> it need to be aware of the interrupt being level vs. edge ?

See the above reasoning about the two messages. If you notice that the
MSI is level, you know that you'll need a second message for the clear.

> 
>> And if that works, the PCI stuff will come for free (it is just a
>> matter of implementing a new irqdomain on top of the base GICv3 one).
> 
> I've lost you here :)

Same as usual. GICv3 already implements a domain for all the interrupts
it services, and we just need to bolt an MBI-specific domain on top (the
equivalent of your GICP). At that stage, we can create the usual
platform and PCI MSI domains that are used by the drivers.

> 
>> I'll try to spend some time on it in the coming couple of weeks, but
>> will have to rely on you for the testing (as I don't have much in the
>> way of HW).
> 
> I can definitely make some tests, I have actual HW to test this.

Cool, thanks.

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

Re: [PATCH] irqchip/gic-v3: Support v2m frame backwards compatibility mode

[Marc Zyngier]

On 10/04/18 16:23, Marc Zyngier wrote:

> I have a vague idea how to support this. Given that level-triggered MSIs
> have to be platform MSIs (because it is just madness otherwise), we can
> probably store an extra message in the struct platform_msi_desc for the
> "lower the line" write. On activation, you'd get two callbacks, probably
> with a flag of some sort to indicate whether this is for the rising or
> falling edge.

Actually, we can get away with a single call and no extra storage if we do
something like below, and check the trigger in the backends:

diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index 2a8571f72b17..85408be6d752 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -89,13 +89,13 @@ int msi_domain_set_affinity(struct irq_data *irq_data,
 			    const struct cpumask *mask, bool force)
 {
 	struct irq_data *parent = irq_data->parent_data;
-	struct msi_msg msg;
+	struct msi_msg msg[2];
 	int ret;
 
 	ret = parent->chip->irq_set_affinity(parent, mask, force);
 	if (ret >= 0 && ret != IRQ_SET_MASK_OK_DONE) {
-		BUG_ON(irq_chip_compose_msi_msg(irq_data, &msg));
-		irq_chip_write_msi_msg(irq_data, &msg);
+		BUG_ON(irq_chip_compose_msi_msg(irq_data, msg));
+		irq_chip_write_msi_msg(irq_data, msg);
 	}
 
 	return ret;
@@ -104,20 +104,20 @@ int msi_domain_set_affinity(struct irq_data *irq_data,
 static int msi_domain_activate(struct irq_domain *domain,
 			       struct irq_data *irq_data, bool early)
 {
-	struct msi_msg msg;
+	struct msi_msg msg[2];
 
-	BUG_ON(irq_chip_compose_msi_msg(irq_data, &msg));
-	irq_chip_write_msi_msg(irq_data, &msg);
+	BUG_ON(irq_chip_compose_msi_msg(irq_data, msg));
+	irq_chip_write_msi_msg(irq_data, msg);
 	return 0;
 }
 
 static void msi_domain_deactivate(struct irq_domain *domain,
 				  struct irq_data *irq_data)
 {
-	struct msi_msg msg;
+	struct msi_msg msg[2];
 
-	memset(&msg, 0, sizeof(msg));
-	irq_chip_write_msi_msg(irq_data, &msg);
+	memset(msg, 0, sizeof(msg));
+	irq_chip_write_msi_msg(irq_data, msg);
 }
 
 static int msi_domain_alloc(struct irq_domain *domain, unsigned int virq,

Is it disgusting? You bet. Does it work? Probably.

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

Re: [PATCH] irqchip/gic-v3: Support v2m frame backwards compatibility mode

[Stephen Boyd]

Quoting Marc Zyngier (2018-04-10 08:23:00)
> On 10/04/18 16:01, Thomas Petazzoni wrote:
> 
> > In the upcoming Armada 8KP, we have a GICv3, which has built-in support
> > for memory-triggered SPIs, thanks to the GICD_SETSPI_NSR and
> > GICD_CLRSPI_NSR, and the ICU will directly use this GICv3
> > functionality. We would therefore very much like to have this GICv3
> > feature provided as a MSI controller, which as Marc said would require
> > supporting level-triggered MSIs.
> > 
> > Marc, let me know how we can collaborate on this topic. I'm able to
> > either test some preliminary patches, or work on such patches if
> > necessary (preferably with some initial directions).
> 
> I have a vague idea how to support this. Given that level-triggered MSIs
> have to be platform MSIs (because it is just madness otherwise), we can
> probably store an extra message in the struct platform_msi_desc for the
> "lower the line" write. On activation, you'd get two callbacks, probably
> with a flag of some sort to indicate whether this is for the rising or
> falling edge. The thing I'm unclear about so far is how to let the
> generic MSI layer know that we're dealing with such an interrupt without
> make a total mess of everything. It is probably done by marking the
> interrupt level triggered, but there are some corner cases.
> 
> And if that works, the PCI stuff will come for free (it is just a matter
> of implementing a new irqdomain on top of the base GICv3 one).
> 
> I'll try to spend some time on it in the coming couple of weeks, but
> will have to rely on you for the testing (as I don't have much in the
> way of HW).

I resent these patches late last year[1]. On the HW I had at the time we
were trying to support PCIe devices and we didn't need level interrupts.
I thought Marc had agreed to accept the patches without the level
interrupt support as long as we described the range of interrupts that
were supported but that doesn't seem to be the case anymore.

Anyway, this is mostly an FYI that I don't have the hardware to test
with anymore and I'm not going to keep sending patches on this topic.
Srini should have some hardware to test whatever solution you come up
with.

[1] http://lkml.kernel.org/r/20171127102408.6631-1-sboyd@codeaurora.org

Re: [PATCH] irqchip/gic-v3: Support v2m frame backwards compatibility mode

[Marc Zyngier]

Hi Stephen,

On 10/04/18 19:17, Stephen Boyd wrote:
> Quoting Marc Zyngier (2018-04-10 08:23:00)
>> On 10/04/18 16:01, Thomas Petazzoni wrote:
>>
>>> In the upcoming Armada 8KP, we have a GICv3, which has built-in support
>>> for memory-triggered SPIs, thanks to the GICD_SETSPI_NSR and
>>> GICD_CLRSPI_NSR, and the ICU will directly use this GICv3
>>> functionality. We would therefore very much like to have this GICv3
>>> feature provided as a MSI controller, which as Marc said would require
>>> supporting level-triggered MSIs.
>>>
>>> Marc, let me know how we can collaborate on this topic. I'm able to
>>> either test some preliminary patches, or work on such patches if
>>> necessary (preferably with some initial directions).
>>
>> I have a vague idea how to support this. Given that level-triggered MSIs
>> have to be platform MSIs (because it is just madness otherwise), we can
>> probably store an extra message in the struct platform_msi_desc for the
>> "lower the line" write. On activation, you'd get two callbacks, probably
>> with a flag of some sort to indicate whether this is for the rising or
>> falling edge. The thing I'm unclear about so far is how to let the
>> generic MSI layer know that we're dealing with such an interrupt without
>> make a total mess of everything. It is probably done by marking the
>> interrupt level triggered, but there are some corner cases.
>>
>> And if that works, the PCI stuff will come for free (it is just a matter
>> of implementing a new irqdomain on top of the base GICv3 one).
>>
>> I'll try to spend some time on it in the coming couple of weeks, but
>> will have to rely on you for the testing (as I don't have much in the
>> way of HW).
> 
> I resent these patches late last year[1]. On the HW I had at the time we
> were trying to support PCIe devices and we didn't need level interrupts.
> I thought Marc had agreed to accept the patches without the level
> interrupt support as long as we described the range of interrupts that
> were supported but that doesn't seem to be the case anymore.

My suggestion was to add the same level of functionality to the GICv3
driver, and not to reuse the GICv2m driver. This would have allowed us
to enable level interrupts at a later time. Obviously, I wasn't clear
enough about what I wanted to see.

So this time, we'll do it the other way around: plug in level
interrupts, and PCI/MSI will (mostly) appear for free.

Thanks,

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

Re: [PATCH] irqchip/gic-v3: Support v2m frame backwards compatibility mode

[Srinivas Kandagatla]

On 10/04/18 19:17, Stephen Boyd wrote:
> Anyway, this is mostly an FYI that I don't have the hardware to test
> with anymore and I'm not going to keep sending patches on this topic.
> Srini should have some hardware to test whatever solution you come up
> with.
Yep, I can test PCI MSI side of it with DragonBoard 820c.

thanks,
srini