| Message ID | 20170320223614.1351-1-sboyd@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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.
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
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.
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
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.
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
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 */
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(-)