Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On 2014/11/7 0:34, Marc Zyngier wrote: > Hi Thomas, > > On 06/11/14 10:42, Thomas Gleixner wrote: >> On Thu, 6 Nov 2014, Thomas Gleixner wrote: >>> On Wed, 5 Nov 2014, Suravee Suthikulanit wrote: On 11/5/2014 6:05 PM, Suravee Suthikulanit wrote: > - Overall, it seems that msi_domain_alloc() could be quite different > across architectures. Would it be possible to declare this function as > weak, and allow arch to override (similar to arch_setup_msi_irq)? Actually, declaring "msi_domain_ops" as non-static, and allow other code to override the .alloc and .free? >>> >>> Why do you want to do that? >> >> I know why. Because you want to spare a level of hierarchy. But thats >> wrong simply because MSI itself is an interrupt chip at the device >> level. >> >> [ MSI ] ---> [ GIC-MSI ] ---> [ GIC ] >> >> So the MSI level only cares about the allocation of the virq >> space. GIC-MSI allocates out of the bitmap which handles the hard >> wired range of MSI capable GIC interrupts and GIC handles the >> underlying functionality. >> >> And this makes a lot of sense, if you think about interrupt >> remapping. If ARM ever grows that you simply insert it into the chain: >> >> [ MSI ] ---> [ Remap] ---> [ GIC-MSI ] ---> [ GIC ] > > I think ARM has reached that stage with the ITS block in GICv3: > - Each device gets programmed with a set of "event IDs" ranging from 0 > to N-1, with N being the number of MSI vectors used by the device > - the ITS uses both the device ID (basically the PCI requester ID) and > the event ID to parse a set of software-managed tables (think page > tables for interrupts). > > The x86 remapping thing looks quite similar to that, by reading a couple > of pages from the VT-D document. > > So the way I understand the layout (and please correct me if I'm wrong, > which is certainly the case) is that the MSI domain is entirely generic, > allocates the virq, uses Remap as a parent, and uses > irq_chip_compose_msi_msg to call into the parent and generate whatever > goes into the MSI message. Hi Marc, It works exactly in this way:) > > I'm still struggling a bit to see how the remapping layer can access the > requester ID. x86 uses the irq_alloc_info to store that (the result of > an msi_get_hwirq call), but we don't have an equivalent structure on > arm/arm64. irq_alloc_info is newly introduced for hierarchy irqdomain on x86. Regards! Gerry > > I'll try to hack something with my current ITS driver and come back with > the results. > > Thanks, > > M. > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
Hi Thomas, On 06/11/14 10:42, Thomas Gleixner wrote: > On Thu, 6 Nov 2014, Thomas Gleixner wrote: >> On Wed, 5 Nov 2014, Suravee Suthikulanit wrote: >>> On 11/5/2014 6:05 PM, Suravee Suthikulanit wrote: - Overall, it seems that msi_domain_alloc() could be quite different across architectures. Would it be possible to declare this function as weak, and allow arch to override (similar to arch_setup_msi_irq)? >>> >>> Actually, declaring "msi_domain_ops" as non-static, and allow other code to >>> override the .alloc and .free? >> >> Why do you want to do that? > > I know why. Because you want to spare a level of hierarchy. But thats > wrong simply because MSI itself is an interrupt chip at the device > level. > > [ MSI ] ---> [ GIC-MSI ] ---> [ GIC ] > > So the MSI level only cares about the allocation of the virq > space. GIC-MSI allocates out of the bitmap which handles the hard > wired range of MSI capable GIC interrupts and GIC handles the > underlying functionality. > > And this makes a lot of sense, if you think about interrupt > remapping. If ARM ever grows that you simply insert it into the chain: > > [ MSI ] ---> [ Remap] ---> [ GIC-MSI ] ---> [ GIC ] I think ARM has reached that stage with the ITS block in GICv3: - Each device gets programmed with a set of "event IDs" ranging from 0 to N-1, with N being the number of MSI vectors used by the device - the ITS uses both the device ID (basically the PCI requester ID) and the event ID to parse a set of software-managed tables (think page tables for interrupts). The x86 remapping thing looks quite similar to that, by reading a couple of pages from the VT-D document. So the way I understand the layout (and please correct me if I'm wrong, which is certainly the case) is that the MSI domain is entirely generic, allocates the virq, uses Remap as a parent, and uses irq_chip_compose_msi_msg to call into the parent and generate whatever goes into the MSI message. I'm still struggling a bit to see how the remapping layer can access the requester ID. x86 uses the irq_alloc_info to store that (the result of an msi_get_hwirq call), but we don't have an equivalent structure on arm/arm64. I'll try to hack something with my current ITS driver and come back with the results. Thanks, M. -- Jazz is not dead. It just smells funny... -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On Thu, 6 Nov 2014, Thomas Gleixner wrote: > On Wed, 5 Nov 2014, Suravee Suthikulanit wrote: > > On 11/5/2014 6:05 PM, Suravee Suthikulanit wrote: > > > - Overall, it seems that msi_domain_alloc() could be quite different > > > across architectures. Would it be possible to declare this function as > > > weak, and allow arch to override (similar to arch_setup_msi_irq)? > > > > Actually, declaring "msi_domain_ops" as non-static, and allow other code to > > override the .alloc and .free? > > Why do you want to do that? I know why. Because you want to spare a level of hierarchy. But thats wrong simply because MSI itself is an interrupt chip at the device level. [ MSI ] ---> [ GIC-MSI ] ---> [ GIC ] So the MSI level only cares about the allocation of the virq space. GIC-MSI allocates out of the bitmap which handles the hard wired range of MSI capable GIC interrupts and GIC handles the underlying functionality. And this makes a lot of sense, if you think about interrupt remapping. If ARM ever grows that you simply insert it into the chain: [ MSI ] ---> [ Remap] ---> [ GIC-MSI ] ---> [ GIC ] If you look at Jiangs x86 implementation it does exactly that. [ MSI ] ---> [ Vector ] [ MSI ] ---> [ Remap ] ---> [ Vector ] And because ARM has this intermediate layer of GIC-MSI you need to represent it in the hierarchy whether you like it or not. If you'd try to bolt the GIC-MSI magic into the MSI layer itself, then interrupt remapping would never work. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
Hi Thomas, On 06/11/14 10:42, Thomas Gleixner wrote: On Thu, 6 Nov 2014, Thomas Gleixner wrote: On Wed, 5 Nov 2014, Suravee Suthikulanit wrote: On 11/5/2014 6:05 PM, Suravee Suthikulanit wrote: - Overall, it seems that msi_domain_alloc() could be quite different across architectures. Would it be possible to declare this function as weak, and allow arch to override (similar to arch_setup_msi_irq)? Actually, declaring msi_domain_ops as non-static, and allow other code to override the .alloc and .free? Why do you want to do that? I know why. Because you want to spare a level of hierarchy. But thats wrong simply because MSI itself is an interrupt chip at the device level. [ MSI ] --- [ GIC-MSI ] --- [ GIC ] So the MSI level only cares about the allocation of the virq space. GIC-MSI allocates out of the bitmap which handles the hard wired range of MSI capable GIC interrupts and GIC handles the underlying functionality. And this makes a lot of sense, if you think about interrupt remapping. If ARM ever grows that you simply insert it into the chain: [ MSI ] --- [ Remap] --- [ GIC-MSI ] --- [ GIC ] I think ARM has reached that stage with the ITS block in GICv3: - Each device gets programmed with a set of event IDs ranging from 0 to N-1, with N being the number of MSI vectors used by the device - the ITS uses both the device ID (basically the PCI requester ID) and the event ID to parse a set of software-managed tables (think page tables for interrupts). The x86 remapping thing looks quite similar to that, by reading a couple of pages from the VT-D document. So the way I understand the layout (and please correct me if I'm wrong, which is certainly the case) is that the MSI domain is entirely generic, allocates the virq, uses Remap as a parent, and uses irq_chip_compose_msi_msg to call into the parent and generate whatever goes into the MSI message. I'm still struggling a bit to see how the remapping layer can access the requester ID. x86 uses the irq_alloc_info to store that (the result of an msi_get_hwirq call), but we don't have an equivalent structure on arm/arm64. I'll try to hack something with my current ITS driver and come back with the results. Thanks, M. -- Jazz is not dead. It just smells funny... -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On 2014/11/7 0:34, Marc Zyngier wrote: Hi Thomas, On 06/11/14 10:42, Thomas Gleixner wrote: On Thu, 6 Nov 2014, Thomas Gleixner wrote: On Wed, 5 Nov 2014, Suravee Suthikulanit wrote: On 11/5/2014 6:05 PM, Suravee Suthikulanit wrote: - Overall, it seems that msi_domain_alloc() could be quite different across architectures. Would it be possible to declare this function as weak, and allow arch to override (similar to arch_setup_msi_irq)? Actually, declaring msi_domain_ops as non-static, and allow other code to override the .alloc and .free? Why do you want to do that? I know why. Because you want to spare a level of hierarchy. But thats wrong simply because MSI itself is an interrupt chip at the device level. [ MSI ] --- [ GIC-MSI ] --- [ GIC ] So the MSI level only cares about the allocation of the virq space. GIC-MSI allocates out of the bitmap which handles the hard wired range of MSI capable GIC interrupts and GIC handles the underlying functionality. And this makes a lot of sense, if you think about interrupt remapping. If ARM ever grows that you simply insert it into the chain: [ MSI ] --- [ Remap] --- [ GIC-MSI ] --- [ GIC ] I think ARM has reached that stage with the ITS block in GICv3: - Each device gets programmed with a set of event IDs ranging from 0 to N-1, with N being the number of MSI vectors used by the device - the ITS uses both the device ID (basically the PCI requester ID) and the event ID to parse a set of software-managed tables (think page tables for interrupts). The x86 remapping thing looks quite similar to that, by reading a couple of pages from the VT-D document. So the way I understand the layout (and please correct me if I'm wrong, which is certainly the case) is that the MSI domain is entirely generic, allocates the virq, uses Remap as a parent, and uses irq_chip_compose_msi_msg to call into the parent and generate whatever goes into the MSI message. Hi Marc, It works exactly in this way:) I'm still struggling a bit to see how the remapping layer can access the requester ID. x86 uses the irq_alloc_info to store that (the result of an msi_get_hwirq call), but we don't have an equivalent structure on arm/arm64. irq_alloc_info is newly introduced for hierarchy irqdomain on x86. Regards! Gerry I'll try to hack something with my current ITS driver and come back with the results. Thanks, M. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On Thu, 6 Nov 2014, Thomas Gleixner wrote: On Wed, 5 Nov 2014, Suravee Suthikulanit wrote: On 11/5/2014 6:05 PM, Suravee Suthikulanit wrote: - Overall, it seems that msi_domain_alloc() could be quite different across architectures. Would it be possible to declare this function as weak, and allow arch to override (similar to arch_setup_msi_irq)? Actually, declaring msi_domain_ops as non-static, and allow other code to override the .alloc and .free? Why do you want to do that? I know why. Because you want to spare a level of hierarchy. But thats wrong simply because MSI itself is an interrupt chip at the device level. [ MSI ] --- [ GIC-MSI ] --- [ GIC ] So the MSI level only cares about the allocation of the virq space. GIC-MSI allocates out of the bitmap which handles the hard wired range of MSI capable GIC interrupts and GIC handles the underlying functionality. And this makes a lot of sense, if you think about interrupt remapping. If ARM ever grows that you simply insert it into the chain: [ MSI ] --- [ Remap] --- [ GIC-MSI ] --- [ GIC ] If you look at Jiangs x86 implementation it does exactly that. [ MSI ] --- [ Vector ] [ MSI ] --- [ Remap ] --- [ Vector ] And because ARM has this intermediate layer of GIC-MSI you need to represent it in the hierarchy whether you like it or not. If you'd try to bolt the GIC-MSI magic into the MSI layer itself, then interrupt remapping would never work. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On Wed, 5 Nov 2014, Suravee Suthikulanit wrote: > On 11/5/2014 6:05 PM, Suravee Suthikulanit wrote: > > - Overall, it seems that msi_domain_alloc() could be quite different > > across architectures. Would it be possible to declare this function as > > weak, and allow arch to override (similar to arch_setup_msi_irq)? > > Actually, declaring "msi_domain_ops" as non-static, and allow other code to > override the .alloc and .free? Why do you want to do that? Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On 11/5/2014 6:05 PM, Suravee Suthikulanit wrote: - Overall, it seems that msi_domain_alloc() could be quite different across architectures. Would it be possible to declare this function as weak, and allow arch to override (similar to arch_setup_msi_irq)? Actually, declaring "msi_domain_ops" as non-static, and allow other code to override the .alloc and .free? Thanks, Suravee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On 11/4/2014 7:01 AM, Jiang Liu wrote: Hi Suravee, You may build a two level hierarchy irqdomains. Use the utilities in this thread http://www.spinics.net/lists/arm-kernel/msg374722.html to build an MSI irqdomain to manage MSI controllers in PCI devices. And build another irqdomain to manage SPI allocation in GICv2. That is: MSI irqdomain (program MSI registers) --> GIV irqdomain (manage SPIs in GICv2 controller) Regards! Gerry Gerry, I try out your patch from the link above, and I have a couple questions/issues. 1. In the drivers/pci/msi.c: msi_irq_domain_alloc_irqs(), it seems that the hwirq comes from msi_get_hwirq(dev, msidesc). In GICv2m, hwirq for MSI is fixed over a specific range. This might require arch-specific callback. 2. In msi_domain_activate, why "if (!irq_data->chip_data)"? 3. In, msi_domain_alloc(): - There should be a way to specify other types of irq handler besides the "handle_edge_irq". In case of GIC, it needs handle_fasteoi_irq. - When calling irq_domain_set_hwirq_and_chip(), you are passing "(void *)(long)i" for the "void *chip_data" parameter. What is this used for, and where? Shouldn't this be pointing to arch-specific data structure? - The code is calling irq_domain_alloc_irqs_parent before the loop, which calls irq_domain_set_hwirq_and_chip() and __irq_set_handler. Shouldn't the order be switched? - Overall, it seems that msi_domain_alloc() could be quite different across architectures. Would it be possible to declare this function as weak, and allow arch to override (similar to arch_setup_msi_irq)? Thanks, Suravee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On 11/4/2014 7:01 AM, Jiang Liu wrote: Hi Suravee, You may build a two level hierarchy irqdomains. Use the utilities in this thread http://www.spinics.net/lists/arm-kernel/msg374722.html to build an MSI irqdomain to manage MSI controllers in PCI devices. And build another irqdomain to manage SPI allocation in GICv2. That is: MSI irqdomain (program MSI registers) -- GIV irqdomain (manage SPIs in GICv2 controller) Regards! Gerry Gerry, I try out your patch from the link above, and I have a couple questions/issues. 1. In the drivers/pci/msi.c: msi_irq_domain_alloc_irqs(), it seems that the hwirq comes from msi_get_hwirq(dev, msidesc). In GICv2m, hwirq for MSI is fixed over a specific range. This might require arch-specific callback. 2. In msi_domain_activate, why if (!irq_data-chip_data)? 3. In, msi_domain_alloc(): - There should be a way to specify other types of irq handler besides the handle_edge_irq. In case of GIC, it needs handle_fasteoi_irq. - When calling irq_domain_set_hwirq_and_chip(), you are passing (void *)(long)i for the void *chip_data parameter. What is this used for, and where? Shouldn't this be pointing to arch-specific data structure? - The code is calling irq_domain_alloc_irqs_parent before the loop, which calls irq_domain_set_hwirq_and_chip() and __irq_set_handler. Shouldn't the order be switched? - Overall, it seems that msi_domain_alloc() could be quite different across architectures. Would it be possible to declare this function as weak, and allow arch to override (similar to arch_setup_msi_irq)? Thanks, Suravee -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On 11/5/2014 6:05 PM, Suravee Suthikulanit wrote: - Overall, it seems that msi_domain_alloc() could be quite different across architectures. Would it be possible to declare this function as weak, and allow arch to override (similar to arch_setup_msi_irq)? Actually, declaring msi_domain_ops as non-static, and allow other code to override the .alloc and .free? Thanks, Suravee -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On Wed, 5 Nov 2014, Suravee Suthikulanit wrote: On 11/5/2014 6:05 PM, Suravee Suthikulanit wrote: - Overall, it seems that msi_domain_alloc() could be quite different across architectures. Would it be possible to declare this function as weak, and allow arch to override (similar to arch_setup_msi_irq)? Actually, declaring msi_domain_ops as non-static, and allow other code to override the .alloc and .free? Why do you want to do that? Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On 04/11/14 14:20, Suravee Suthikulpanit wrote: > > > On 11/4/14 04:06, Thomas Gleixner wrote: >> On Mon, 3 Nov 2014, Suravee Suthikulanit wrote: >>> On 11/3/2014 4:51 PM, Thomas Gleixner wrote: On Mon, 3 Nov 2014, suravee.suthikulpa...@amd.com wrote: > + irq_domain_set_hwirq_and_chip(v2m->domain, virq, hwirq, > + _chip, v2m); > + > + irq_set_msi_desc(hwirq, desc); > + irq_set_irq_type(hwirq, IRQ_TYPE_EDGE_RISING); Sure both calls work perfectly fine as long as virq == hwirq, right? >>> >>> I was running into an issue when calling the irq_domain_alloc_irq_parent(), >>> it >>> requires of_phandle_args pointer to be passed in. However, this does not >>> work >>> for GICv2m since it does not have interrupt information in the device tree. >>> So, I decided at first to use direct (virq == hwirq) mapping, which >>> simplifies >>> the code a bit, but might not be ideal solution, as you pointed out. >> >> It's not only far from ideal. It's not a solution at all. Simply >> because there is no guarantee for virq == hwirq. >> >>> An alternative would be to create a temporary struct of_phandle_args, and >>> populate it with the interrupt information for the requested MSI. Then pass >>> it >>> to: >>>--> irq_domain_alloc_irq_parent >>> |--> gic_irq_domain_alloc >>> |--> gic_irq_domain_xlate >>> |--> gic_irq_domain_map >>> >>> However, this would still not be ideal if we want to support ACPI. Another >> >> Neither device tree nor ACPI has anything to do with MSI interrupts at >> runtime. >> >> All they do is to tell that there is a MSI controller and where the >> registers are and in the worst case fixups for a borked MSI_TYPER >> register. >> >> So either the TYPER reg or DT/ACPI gives you a fixed hwirq range which >> is reserved for MSI. And that's all you need, right? >> > Right, I get that part. Figuring out the fixed hwirq range for MSI is > not the point I am trying to make here. > >> [...] >> All you need is to pick one hwirq out of the existing fixed range and >> associate it to a newly allocated virq. That's the only information >> the underlying gic domain has to know about, because it needs to >> translate from the hwirq to the virq in the low level entry handler >> gic_handle_irq(). > > And that's what I am trying to do here except that GIC is expecting that > information to be passed to it via irq_domain_alloc_irqs(..., args) > where args is struct of_phandle_args (e.g. in the kernel/irqdomain.c: > irq_create_of_mapping). This works fine when specifying interrupt from > DT, but that is not always the case. > > Currently, I can just create a fake of_phandle_args just to pass the > hwirq information to GIC. > > --> gicv2m_setup_msi_irq() > |struct of_phandle_args phan; > |phan.np = NULL; > |phan.args_count = 3; > |phan.args[0] = 0; > |phan.args[1] = hwirq - 32; > |phan.args[2] = IRQ_TYPE_EDGE_RISING; > |--> irq_domain_alloc_irqs(d, 1, NUMA_NO_NODE, ); >|--> gicv2m_domain_alloc(d, virq, nr_irqs, arg) > |--> irq_domain_alloc_irqs_parent(d, virq, nr_irqs, arg); > > I am trying to figure out what would be a common data structure for this > purpose that would work for both Dt and non-DT case (e.g. GICv2m MSI). > Unless you think this is ok. I think of_phandle_args is the only thing we can use, whether this is DT or this other non-DT thing. This is how we represent a GIC HW interrupt outside of the GIC itself. Of course, this creates a dependency between the different domains (a child domain *must* know how the parent domain represents its interrupt). Things like the v2m widget are completely fused with the GIC anyway, so it makes sense. Things like generic secondary irqchips don't fit that model, but they can continue to use the existing framework. Thanks, M. -- Jazz is not dead. It just smells funny... -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On 11/4/14 07:01, Jiang Liu wrote: Hi Suravee, You may build a two level hierarchy irqdomains. Use the utilities in this thread http://www.spinics.net/lists/arm-kernel/msg374722.html to build an MSI irqdomain to manage MSI controllers in PCI devices. And build another irqdomain to manage SPI allocation in GICv2. That is: MSI irqdomain (program MSI registers) --> GIV irqdomain (manage SPIs in GICv2 controller) That's great. I'll look at this patch in and make use of it to create to MSI domain. Thanks, Suravee Regards! Gerry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On Tue, 4 Nov 2014, Suravee Suthikulpanit wrote: > And that's what I am trying to do here except that GIC is expecting that > information to be passed to it via irq_domain_alloc_irqs(..., args) where args > is struct of_phandle_args (e.g. in the kernel/irqdomain.c: > irq_create_of_mapping). This works fine when specifying interrupt from DT, but > that is not always the case. > > Currently, I can just create a fake of_phandle_args just to pass the hwirq > information to GIC. > > --> gicv2m_setup_msi_irq() > |struct of_phandle_args phan; > |phan.np = NULL; > |phan.args_count = 3; > |phan.args[0] = 0; > |phan.args[1] = hwirq - 32; > |phan.args[2] = IRQ_TYPE_EDGE_RISING; > |--> irq_domain_alloc_irqs(d, 1, NUMA_NO_NODE, ); > |--> gicv2m_domain_alloc(d, virq, nr_irqs, arg) > |--> irq_domain_alloc_irqs_parent(d, virq, nr_irqs, arg); > > I am trying to figure out what would be a common data structure for this > purpose that would work for both Dt and non-DT case (e.g. GICv2m MSI). Unless > you think this is ok. You need to sort that out with Marc. It needs to be done in a way which is usable for the other potential use cases of stacked domains on top of GIC. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On 11/4/14 04:06, Thomas Gleixner wrote: On Mon, 3 Nov 2014, Suravee Suthikulanit wrote: On 11/3/2014 4:51 PM, Thomas Gleixner wrote: On Mon, 3 Nov 2014, suravee.suthikulpa...@amd.com wrote: + irq_domain_set_hwirq_and_chip(v2m->domain, virq, hwirq, + _chip, v2m); + + irq_set_msi_desc(hwirq, desc); + irq_set_irq_type(hwirq, IRQ_TYPE_EDGE_RISING); Sure both calls work perfectly fine as long as virq == hwirq, right? I was running into an issue when calling the irq_domain_alloc_irq_parent(), it requires of_phandle_args pointer to be passed in. However, this does not work for GICv2m since it does not have interrupt information in the device tree. So, I decided at first to use direct (virq == hwirq) mapping, which simplifies the code a bit, but might not be ideal solution, as you pointed out. It's not only far from ideal. It's not a solution at all. Simply because there is no guarantee for virq == hwirq. An alternative would be to create a temporary struct of_phandle_args, and populate it with the interrupt information for the requested MSI. Then pass it to: --> irq_domain_alloc_irq_parent |--> gic_irq_domain_alloc |--> gic_irq_domain_xlate |--> gic_irq_domain_map However, this would still not be ideal if we want to support ACPI. Another Neither device tree nor ACPI has anything to do with MSI interrupts at runtime. All they do is to tell that there is a MSI controller and where the registers are and in the worst case fixups for a borked MSI_TYPER register. So either the TYPER reg or DT/ACPI gives you a fixed hwirq range which is reserved for MSI. And that's all you need, right? Right, I get that part. Figuring out the fixed hwirq range for MSI is not the point I am trying to make here. [...] All you need is to pick one hwirq out of the existing fixed range and associate it to a newly allocated virq. That's the only information the underlying gic domain has to know about, because it needs to translate from the hwirq to the virq in the low level entry handler gic_handle_irq(). And that's what I am trying to do here except that GIC is expecting that information to be passed to it via irq_domain_alloc_irqs(..., args) where args is struct of_phandle_args (e.g. in the kernel/irqdomain.c: irq_create_of_mapping). This works fine when specifying interrupt from DT, but that is not always the case. Currently, I can just create a fake of_phandle_args just to pass the hwirq information to GIC. --> gicv2m_setup_msi_irq() |struct of_phandle_args phan; |phan.np = NULL; |phan.args_count = 3; |phan.args[0] = 0; |phan.args[1] = hwirq - 32; |phan.args[2] = IRQ_TYPE_EDGE_RISING; |--> irq_domain_alloc_irqs(d, 1, NUMA_NO_NODE, ); |--> gicv2m_domain_alloc(d, virq, nr_irqs, arg) |--> irq_domain_alloc_irqs_parent(d, virq, nr_irqs, arg); I am trying to figure out what would be a common data structure for this purpose that would work for both Dt and non-DT case (e.g. GICv2m MSI). Unless you think this is ok. Thanks, Suravee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
Hi Suravee,
You may build a two level hierarchy irqdomains. Use the
utilities in this thread
http://www.spinics.net/lists/arm-kernel/msg374722.html to build an MSI
irqdomain to manage MSI controllers
in PCI devices. And build another irqdomain to manage SPI allocation
in GICv2.
That is: MSI irqdomain (program MSI registers) -->
GIV irqdomain (manage SPIs in GICv2 controller)
Regards!
Gerry
On 2014/11/4 6:16, suravee.suthikulpa...@amd.com wrote:
> From: Suravee Suthikulpanit
>
> ARM GICv2m specification extends GICv2 to support MSI(-X) with
> a new set of register frame. This patch introduces support for
> the non-secure GICv2m register frame. Currently, GICV2m is available
> in certain version of GIC-400.
>
> The patch introduces a new property in ARM gic binding, the v2m subnode.
> It is optional.
>
> Cc: Marc Zyngier
> Cc: Thomas Gleixner
> Cc: Mark Rutland
> Cc: Jason Cooper
> Cc: Will Deacon
> Cc: Catalin Marinas
> Signed-off-by: Suravee Suthikulpanit
> ---
> Documentation/devicetree/bindings/arm/gic.txt | 53
> arch/arm64/Kconfig| 1 +
> drivers/irqchip/Kconfig | 5 +
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-gic-v2m.c | 340
> ++
> drivers/irqchip/irq-gic-v2m.h | 6 +
> drivers/irqchip/irq-gic.c | 23 +-
> 7 files changed, 425 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/arm/gic.txt
> b/Documentation/devicetree/bindings/arm/gic.txt
> index c7d2fa1..ebf976a 100644
> --- a/Documentation/devicetree/bindings/arm/gic.txt
> +++ b/Documentation/devicetree/bindings/arm/gic.txt
> @@ -96,3 +96,56 @@ Example:
> <0x2c006000 0x2000>;
> interrupts = <1 9 0xf04>;
> };
> +
> +
> +* GICv2m extension for MSI/MSI-x support (Optional)
> +
> +Certain revisions of GIC-400 supports MSI/MSI-x via V2M register frame(s).
> +This is enabled by specifying v2m sub-node(s).
> +
> +Required properties:
> +
> +- compatible: The value here should contain "arm,gic-v2m-frame".
> +
> +- msi-controller: Identifies the node as an MSI controller.
> +
> +- reg : GICv2m MSI interface register base and size
> +
> +Optional properties:
> +
> +- arm,msi-base-spi : When the MSI_TYPER register contains an incorrect
> + value, this property should contain the SPI base of
> + the MSI frame, overriding the HW value.
> +
> +- arm,msi-num-spis : When the MSI_TYPER register contains an incorrect
> + value, this property should contain the number of
> + SPIs assigned to the frame, overriding the HW value.
> +
> +Example:
> +
> + interrupt-controller@e1101000 {
> + compatible = "arm,gic-400";
> + #interrupt-cells = <3>;
> + #address-cells = <2>;
> + #size-cells = <2>;
> + interrupt-controller;
> + interrupts = <1 8 0xf04>;
> + ranges = <0 0 0 0xe110 0 0x10>;
> + reg = <0x0 0xe111 0 0x01000>,
> + <0x0 0xe112f000 0 0x02000>,
> + <0x0 0xe114 0 0x1>,
> + <0x0 0xe116 0 0x1>;
> + v2m0: v2m@0x8000 {
> + compatible = "arm,gic-v2m-frame";
> + msi-controller;
> + reg = <0x0 0x8 0 0x1000>;
> + };
> +
> +
> +
> + v2mN: v2m@0x9000 {
> + compatible = "arm,gic-v2m-frame";
> + msi-controller;
> + reg = <0x0 0x9 0 0x1000>;
> + };
> + };
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index cde2f72..cbcde2d 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -12,6 +12,7 @@ config ARM64
> select ARM_ARCH_TIMER
> select ARM_GIC
> select AUDIT_ARCH_COMPAT_GENERIC
> + select ARM_GIC_V2M
> select ARM_GIC_V3
> select BUILDTIME_EXTABLE_SORT
> select CLONE_BACKWARDS
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 2a48e0a..39ce065 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -8,6 +8,11 @@ config ARM_GIC
> select IRQ_DOMAIN_HIERARCHY
> select MULTI_IRQ_HANDLER
>
> +config ARM_GIC_V2M
> + bool
> + depends on ARM_GIC
> + depends on PCI && PCI_MSI
> +
> config GIC_NON_BANKED
> bool
>
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 73052ba..3bda951 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_ARCH_SUNXI)+= irq-sun4i.o
> obj-$(CONFIG_ARCH_SUNXI) += irq-sunxi-nmi.o
> obj-$(CONFIG_ARCH_SPEAR3XX) += spear-shirq.o
> obj-$(CONFIG_ARM_GIC)
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On Mon, 3 Nov 2014, Suravee Suthikulanit wrote: > On 11/3/2014 4:51 PM, Thomas Gleixner wrote: > > On Mon, 3 Nov 2014, suravee.suthikulpa...@amd.com wrote: > > > + irq_domain_set_hwirq_and_chip(v2m->domain, virq, hwirq, > > > + _chip, v2m); > > > + > > > + irq_set_msi_desc(hwirq, desc); > > > + irq_set_irq_type(hwirq, IRQ_TYPE_EDGE_RISING); > > > > Sure both calls work perfectly fine as long as virq == hwirq, right? > > I was running into an issue when calling the irq_domain_alloc_irq_parent(), it > requires of_phandle_args pointer to be passed in. However, this does not work > for GICv2m since it does not have interrupt information in the device tree. > So, I decided at first to use direct (virq == hwirq) mapping, which simplifies > the code a bit, but might not be ideal solution, as you pointed out. It's not only far from ideal. It's not a solution at all. Simply because there is no guarantee for virq == hwirq. > An alternative would be to create a temporary struct of_phandle_args, and > populate it with the interrupt information for the requested MSI. Then pass it > to: > --> irq_domain_alloc_irq_parent >|--> gic_irq_domain_alloc > |--> gic_irq_domain_xlate > |--> gic_irq_domain_map > > However, this would still not be ideal if we want to support ACPI. Another Neither device tree nor ACPI has anything to do with MSI interrupts at runtime. All they do is to tell that there is a MSI controller and where the registers are and in the worst case fixups for a borked MSI_TYPER register. So either the TYPER reg or DT/ACPI gives you a fixed hwirq range which is reserved for MSI. And that's all you need, right? The MSI interrupt itself has no DT/ACPI information to use at allocation time simply because you CANNOT decribe a MSI device interrupt in DT/ACPI by any means. And you do not need any DT/ACPI information at that point. All you need is to pick one hwirq out of the existing fixed range and associate it to a newly allocated virq. That's the only information the underlying gic domain has to know about, because it needs to translate from the hwirq to the virq in the low level entry handler gic_handle_irq(). > alternative would be coming up with a dedicate structure to be used here. I > noticed on X86, it uses struct irq_alloc_info. May be that's what we also need > here. It's a x86 concept to transport X86 specific information in order to avoid duplicated code all over the place. And x86 MSI support is a completely different beast than the thing you are dealing with. x86 has no concept of a fixed hwirq range for MSI. So no, just picking random stuff from random MSI implementations does not help at all. > > [...] > > I do not care at all how YOU waste your time. But I care very much > > about the fact that YOU are wasting MY precious time by exposing me to > > your patch trainwrecks. > > I don't intend to waste yours or anybody's precious time. Sorry if it takes a > couple iterations to work out the issues. Also, I will try to put more comment > in my code to make it more clear. Let me know what works best for you to work > out the issues. By not sending obviously broken and half thought out patches in the first place. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On Mon, 3 Nov 2014, Suravee Suthikulanit wrote: On 11/3/2014 4:51 PM, Thomas Gleixner wrote: On Mon, 3 Nov 2014, suravee.suthikulpa...@amd.com wrote: + irq_domain_set_hwirq_and_chip(v2m-domain, virq, hwirq, + v2m_chip, v2m); + + irq_set_msi_desc(hwirq, desc); + irq_set_irq_type(hwirq, IRQ_TYPE_EDGE_RISING); Sure both calls work perfectly fine as long as virq == hwirq, right? I was running into an issue when calling the irq_domain_alloc_irq_parent(), it requires of_phandle_args pointer to be passed in. However, this does not work for GICv2m since it does not have interrupt information in the device tree. So, I decided at first to use direct (virq == hwirq) mapping, which simplifies the code a bit, but might not be ideal solution, as you pointed out. It's not only far from ideal. It's not a solution at all. Simply because there is no guarantee for virq == hwirq. An alternative would be to create a temporary struct of_phandle_args, and populate it with the interrupt information for the requested MSI. Then pass it to: -- irq_domain_alloc_irq_parent |-- gic_irq_domain_alloc |-- gic_irq_domain_xlate |-- gic_irq_domain_map However, this would still not be ideal if we want to support ACPI. Another Neither device tree nor ACPI has anything to do with MSI interrupts at runtime. All they do is to tell that there is a MSI controller and where the registers are and in the worst case fixups for a borked MSI_TYPER register. So either the TYPER reg or DT/ACPI gives you a fixed hwirq range which is reserved for MSI. And that's all you need, right? The MSI interrupt itself has no DT/ACPI information to use at allocation time simply because you CANNOT decribe a MSI device interrupt in DT/ACPI by any means. And you do not need any DT/ACPI information at that point. All you need is to pick one hwirq out of the existing fixed range and associate it to a newly allocated virq. That's the only information the underlying gic domain has to know about, because it needs to translate from the hwirq to the virq in the low level entry handler gic_handle_irq(). alternative would be coming up with a dedicate structure to be used here. I noticed on X86, it uses struct irq_alloc_info. May be that's what we also need here. It's a x86 concept to transport X86 specific information in order to avoid duplicated code all over the place. And x86 MSI support is a completely different beast than the thing you are dealing with. x86 has no concept of a fixed hwirq range for MSI. So no, just picking random stuff from random MSI implementations does not help at all. [...] I do not care at all how YOU waste your time. But I care very much about the fact that YOU are wasting MY precious time by exposing me to your patch trainwrecks. I don't intend to waste yours or anybody's precious time. Sorry if it takes a couple iterations to work out the issues. Also, I will try to put more comment in my code to make it more clear. Let me know what works best for you to work out the issues. By not sending obviously broken and half thought out patches in the first place. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
Hi Suravee,
You may build a two level hierarchy irqdomains. Use the
utilities in this thread
http://www.spinics.net/lists/arm-kernel/msg374722.html to build an MSI
irqdomain to manage MSI controllers
in PCI devices. And build another irqdomain to manage SPI allocation
in GICv2.
That is: MSI irqdomain (program MSI registers) --
GIV irqdomain (manage SPIs in GICv2 controller)
Regards!
Gerry
On 2014/11/4 6:16, suravee.suthikulpa...@amd.com wrote:
From: Suravee Suthikulpanit suravee.suthikulpa...@amd.com
ARM GICv2m specification extends GICv2 to support MSI(-X) with
a new set of register frame. This patch introduces support for
the non-secure GICv2m register frame. Currently, GICV2m is available
in certain version of GIC-400.
The patch introduces a new property in ARM gic binding, the v2m subnode.
It is optional.
Cc: Marc Zyngier marc.zyng...@arm.com
Cc: Thomas Gleixner t...@linutronix.de
Cc: Mark Rutland mark.rutl...@arm.com
Cc: Jason Cooper ja...@lakedaemon.net
Cc: Will Deacon will.dea...@arm.com
Cc: Catalin Marinas catalin.mari...@arm.com
Signed-off-by: Suravee Suthikulpanit suravee.suthikulpa...@amd.com
---
Documentation/devicetree/bindings/arm/gic.txt | 53
arch/arm64/Kconfig| 1 +
drivers/irqchip/Kconfig | 5 +
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-gic-v2m.c | 340
++
drivers/irqchip/irq-gic-v2m.h | 6 +
drivers/irqchip/irq-gic.c | 23 +-
7 files changed, 425 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/arm/gic.txt
b/Documentation/devicetree/bindings/arm/gic.txt
index c7d2fa1..ebf976a 100644
--- a/Documentation/devicetree/bindings/arm/gic.txt
+++ b/Documentation/devicetree/bindings/arm/gic.txt
@@ -96,3 +96,56 @@ Example:
0x2c006000 0x2000;
interrupts = 1 9 0xf04;
};
+
+
+* GICv2m extension for MSI/MSI-x support (Optional)
+
+Certain revisions of GIC-400 supports MSI/MSI-x via V2M register frame(s).
+This is enabled by specifying v2m sub-node(s).
+
+Required properties:
+
+- compatible: The value here should contain arm,gic-v2m-frame.
+
+- msi-controller: Identifies the node as an MSI controller.
+
+- reg : GICv2m MSI interface register base and size
+
+Optional properties:
+
+- arm,msi-base-spi : When the MSI_TYPER register contains an incorrect
+ value, this property should contain the SPI base of
+ the MSI frame, overriding the HW value.
+
+- arm,msi-num-spis : When the MSI_TYPER register contains an incorrect
+ value, this property should contain the number of
+ SPIs assigned to the frame, overriding the HW value.
+
+Example:
+
+ interrupt-controller@e1101000 {
+ compatible = arm,gic-400;
+ #interrupt-cells = 3;
+ #address-cells = 2;
+ #size-cells = 2;
+ interrupt-controller;
+ interrupts = 1 8 0xf04;
+ ranges = 0 0 0 0xe110 0 0x10;
+ reg = 0x0 0xe111 0 0x01000,
+ 0x0 0xe112f000 0 0x02000,
+ 0x0 0xe114 0 0x1,
+ 0x0 0xe116 0 0x1;
+ v2m0: v2m@0x8000 {
+ compatible = arm,gic-v2m-frame;
+ msi-controller;
+ reg = 0x0 0x8 0 0x1000;
+ };
+
+
+
+ v2mN: v2m@0x9000 {
+ compatible = arm,gic-v2m-frame;
+ msi-controller;
+ reg = 0x0 0x9 0 0x1000;
+ };
+ };
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index cde2f72..cbcde2d 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -12,6 +12,7 @@ config ARM64
select ARM_ARCH_TIMER
select ARM_GIC
select AUDIT_ARCH_COMPAT_GENERIC
+ select ARM_GIC_V2M
select ARM_GIC_V3
select BUILDTIME_EXTABLE_SORT
select CLONE_BACKWARDS
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 2a48e0a..39ce065 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -8,6 +8,11 @@ config ARM_GIC
select IRQ_DOMAIN_HIERARCHY
select MULTI_IRQ_HANDLER
+config ARM_GIC_V2M
+ bool
+ depends on ARM_GIC
+ depends on PCI PCI_MSI
+
config GIC_NON_BANKED
bool
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 73052ba..3bda951 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_ARCH_SUNXI)+= irq-sun4i.o
obj-$(CONFIG_ARCH_SUNXI) += irq-sunxi-nmi.o
obj-$(CONFIG_ARCH_SPEAR3XX) += spear-shirq.o
obj-$(CONFIG_ARM_GIC)
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On 11/4/14 04:06, Thomas Gleixner wrote: On Mon, 3 Nov 2014, Suravee Suthikulanit wrote: On 11/3/2014 4:51 PM, Thomas Gleixner wrote: On Mon, 3 Nov 2014, suravee.suthikulpa...@amd.com wrote: + irq_domain_set_hwirq_and_chip(v2m-domain, virq, hwirq, + v2m_chip, v2m); + + irq_set_msi_desc(hwirq, desc); + irq_set_irq_type(hwirq, IRQ_TYPE_EDGE_RISING); Sure both calls work perfectly fine as long as virq == hwirq, right? I was running into an issue when calling the irq_domain_alloc_irq_parent(), it requires of_phandle_args pointer to be passed in. However, this does not work for GICv2m since it does not have interrupt information in the device tree. So, I decided at first to use direct (virq == hwirq) mapping, which simplifies the code a bit, but might not be ideal solution, as you pointed out. It's not only far from ideal. It's not a solution at all. Simply because there is no guarantee for virq == hwirq. An alternative would be to create a temporary struct of_phandle_args, and populate it with the interrupt information for the requested MSI. Then pass it to: -- irq_domain_alloc_irq_parent |-- gic_irq_domain_alloc |-- gic_irq_domain_xlate |-- gic_irq_domain_map However, this would still not be ideal if we want to support ACPI. Another Neither device tree nor ACPI has anything to do with MSI interrupts at runtime. All they do is to tell that there is a MSI controller and where the registers are and in the worst case fixups for a borked MSI_TYPER register. So either the TYPER reg or DT/ACPI gives you a fixed hwirq range which is reserved for MSI. And that's all you need, right? Right, I get that part. Figuring out the fixed hwirq range for MSI is not the point I am trying to make here. [...] All you need is to pick one hwirq out of the existing fixed range and associate it to a newly allocated virq. That's the only information the underlying gic domain has to know about, because it needs to translate from the hwirq to the virq in the low level entry handler gic_handle_irq(). And that's what I am trying to do here except that GIC is expecting that information to be passed to it via irq_domain_alloc_irqs(..., args) where args is struct of_phandle_args (e.g. in the kernel/irqdomain.c: irq_create_of_mapping). This works fine when specifying interrupt from DT, but that is not always the case. Currently, I can just create a fake of_phandle_args just to pass the hwirq information to GIC. -- gicv2m_setup_msi_irq() |struct of_phandle_args phan; |phan.np = NULL; |phan.args_count = 3; |phan.args[0] = 0; |phan.args[1] = hwirq - 32; |phan.args[2] = IRQ_TYPE_EDGE_RISING; |-- irq_domain_alloc_irqs(d, 1, NUMA_NO_NODE, phan); |-- gicv2m_domain_alloc(d, virq, nr_irqs, arg) |-- irq_domain_alloc_irqs_parent(d, virq, nr_irqs, arg); I am trying to figure out what would be a common data structure for this purpose that would work for both Dt and non-DT case (e.g. GICv2m MSI). Unless you think this is ok. Thanks, Suravee -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On Tue, 4 Nov 2014, Suravee Suthikulpanit wrote: And that's what I am trying to do here except that GIC is expecting that information to be passed to it via irq_domain_alloc_irqs(..., args) where args is struct of_phandle_args (e.g. in the kernel/irqdomain.c: irq_create_of_mapping). This works fine when specifying interrupt from DT, but that is not always the case. Currently, I can just create a fake of_phandle_args just to pass the hwirq information to GIC. -- gicv2m_setup_msi_irq() |struct of_phandle_args phan; |phan.np = NULL; |phan.args_count = 3; |phan.args[0] = 0; |phan.args[1] = hwirq - 32; |phan.args[2] = IRQ_TYPE_EDGE_RISING; |-- irq_domain_alloc_irqs(d, 1, NUMA_NO_NODE, phan); |-- gicv2m_domain_alloc(d, virq, nr_irqs, arg) |-- irq_domain_alloc_irqs_parent(d, virq, nr_irqs, arg); I am trying to figure out what would be a common data structure for this purpose that would work for both Dt and non-DT case (e.g. GICv2m MSI). Unless you think this is ok. You need to sort that out with Marc. It needs to be done in a way which is usable for the other potential use cases of stacked domains on top of GIC. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On 11/4/14 07:01, Jiang Liu wrote: Hi Suravee, You may build a two level hierarchy irqdomains. Use the utilities in this thread http://www.spinics.net/lists/arm-kernel/msg374722.html to build an MSI irqdomain to manage MSI controllers in PCI devices. And build another irqdomain to manage SPI allocation in GICv2. That is: MSI irqdomain (program MSI registers) -- GIV irqdomain (manage SPIs in GICv2 controller) That's great. I'll look at this patch in and make use of it to create to MSI domain. Thanks, Suravee Regards! Gerry -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On 04/11/14 14:20, Suravee Suthikulpanit wrote: On 11/4/14 04:06, Thomas Gleixner wrote: On Mon, 3 Nov 2014, Suravee Suthikulanit wrote: On 11/3/2014 4:51 PM, Thomas Gleixner wrote: On Mon, 3 Nov 2014, suravee.suthikulpa...@amd.com wrote: + irq_domain_set_hwirq_and_chip(v2m-domain, virq, hwirq, + v2m_chip, v2m); + + irq_set_msi_desc(hwirq, desc); + irq_set_irq_type(hwirq, IRQ_TYPE_EDGE_RISING); Sure both calls work perfectly fine as long as virq == hwirq, right? I was running into an issue when calling the irq_domain_alloc_irq_parent(), it requires of_phandle_args pointer to be passed in. However, this does not work for GICv2m since it does not have interrupt information in the device tree. So, I decided at first to use direct (virq == hwirq) mapping, which simplifies the code a bit, but might not be ideal solution, as you pointed out. It's not only far from ideal. It's not a solution at all. Simply because there is no guarantee for virq == hwirq. An alternative would be to create a temporary struct of_phandle_args, and populate it with the interrupt information for the requested MSI. Then pass it to: -- irq_domain_alloc_irq_parent |-- gic_irq_domain_alloc |-- gic_irq_domain_xlate |-- gic_irq_domain_map However, this would still not be ideal if we want to support ACPI. Another Neither device tree nor ACPI has anything to do with MSI interrupts at runtime. All they do is to tell that there is a MSI controller and where the registers are and in the worst case fixups for a borked MSI_TYPER register. So either the TYPER reg or DT/ACPI gives you a fixed hwirq range which is reserved for MSI. And that's all you need, right? Right, I get that part. Figuring out the fixed hwirq range for MSI is not the point I am trying to make here. [...] All you need is to pick one hwirq out of the existing fixed range and associate it to a newly allocated virq. That's the only information the underlying gic domain has to know about, because it needs to translate from the hwirq to the virq in the low level entry handler gic_handle_irq(). And that's what I am trying to do here except that GIC is expecting that information to be passed to it via irq_domain_alloc_irqs(..., args) where args is struct of_phandle_args (e.g. in the kernel/irqdomain.c: irq_create_of_mapping). This works fine when specifying interrupt from DT, but that is not always the case. Currently, I can just create a fake of_phandle_args just to pass the hwirq information to GIC. -- gicv2m_setup_msi_irq() |struct of_phandle_args phan; |phan.np = NULL; |phan.args_count = 3; |phan.args[0] = 0; |phan.args[1] = hwirq - 32; |phan.args[2] = IRQ_TYPE_EDGE_RISING; |-- irq_domain_alloc_irqs(d, 1, NUMA_NO_NODE, phan); |-- gicv2m_domain_alloc(d, virq, nr_irqs, arg) |-- irq_domain_alloc_irqs_parent(d, virq, nr_irqs, arg); I am trying to figure out what would be a common data structure for this purpose that would work for both Dt and non-DT case (e.g. GICv2m MSI). Unless you think this is ok. I think of_phandle_args is the only thing we can use, whether this is DT or this other non-DT thing. This is how we represent a GIC HW interrupt outside of the GIC itself. Of course, this creates a dependency between the different domains (a child domain *must* know how the parent domain represents its interrupt). Things like the v2m widget are completely fused with the GIC anyway, so it makes sense. Things like generic secondary irqchips don't fit that model, but they can continue to use the existing framework. Thanks, M. -- Jazz is not dead. It just smells funny... -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On 11/3/2014 4:51 PM, Thomas Gleixner wrote:
On Mon, 3 Nov 2014, suravee.suthikulpa...@amd.com wrote:
+static void gicv2m_teardown_msi_irq(struct msi_chip *chip, unsigned int irq)
+{
+ int pos;
+ struct v2m_data *v2m = container_of(chip, struct v2m_data, msi_chip);
+
+ spin_lock(>msi_cnt_lock);
Why do you need an extra lock here? Is that stuff not serialized from
the msi_chip layer already?
If not, why don't we have the serialization there instead of forcing
every callback to implement its own?
From the following call paths:
|--> pci_enable_msi_range
|--> msi_capability_init
|--> arch_setup_msi_irqs
|--> arch_setup_msi_irq
and
|--> pci_enable_msix
|--> msix_capability_init
|--> arch_setup_msi_irqs
|--> arch_setup_msi_irq
It serialize when a PCI device driver tries to allocate multiple
interrupts. However, AFAICT, it would not serialize the allocation when
multiple drivers trying to setup MSI irqs at the same time. I needed
that to protect the bitmap structure. I also noticed the same in other
drivers as well.
I can look into this more to see where would be a good point.
+ pos = irq - v2m->spi_start;
So this assumes that @irq is the hwirq number, right? How does the
calling function know about that? It should only have knowledge about
the virq number if I'm not missing something.
And if I'm missing something, then that msi_chip stuff is seriously
broken.
It works this way because of the direct mapping (as you noticed). But I
am planning to change that. See below.
+ if (pos >= 0 && pos < v2m->nr_spis)
So you simply avoid the clear bitmap instead of yelling loudly about
being called with completely wrong data?
I'll provide appropriate warnings.
I would not be surprised if that is related to my question above.
Not quite sure which of the above questions.
+ spin_lock(>msi_cnt_lock);
+ offset = bitmap_find_free_region(v2m->bm, v2m->nr_spis, 0);
+ spin_unlock(>msi_cnt_lock);
+ if (offset < 0)
+ return offset;
+
+ hwirq = v2m->spi_start + offset;
+ virq = __irq_domain_alloc_irqs(v2m->domain, hwirq,
+ 1, NUMA_NO_NODE, v2m, true);
+ if (virq < 0) {
+ gicv2m_teardown_msi_irq(chip, hwirq);
+ return virq;
+ }
+
+ irq_domain_set_hwirq_and_chip(v2m->domain, virq, hwirq,
+ _chip, v2m);
+
+ irq_set_msi_desc(hwirq, desc);
+ irq_set_irq_type(hwirq, IRQ_TYPE_EDGE_RISING);
Sure both calls work perfectly fine as long as virq == hwirq, right?
I was running into an issue when calling the
irq_domain_alloc_irq_parent(), it requires of_phandle_args pointer to be
passed in. However, this does not work for GICv2m since it does not have
interrupt information in the device tree. So, I decided at first to use
direct (virq == hwirq) mapping, which simplifies the code a bit, but
might not be ideal solution, as you pointed out.
An alternative would be to create a temporary struct of_phandle_args,
and populate it with the interrupt information for the requested MSI.
Then pass it to:
--> irq_domain_alloc_irq_parent
|--> gic_irq_domain_alloc
|--> gic_irq_domain_xlate
|--> gic_irq_domain_map
However, this would still not be ideal if we want to support ACPI.
Another alternative would be coming up with a dedicate structure to be
used here. I noticed on X86, it uses struct irq_alloc_info. May be
that's what we also need here.
[...]
I do not care at all how YOU waste your time. But I care very much
about the fact that YOU are wasting MY precious time by exposing me to
your patch trainwrecks.
I don't intend to waste yours or anybody's precious time. Sorry if it
takes a couple iterations to work out the issues. Also, I will try to
put more comment in my code to make it more clear. Let me know what
works best for you to work out the issues.
Thanks,
Suravee
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On Mon, 3 Nov 2014, suravee.suthikulpa...@amd.com wrote:
> +static void gicv2m_teardown_msi_irq(struct msi_chip *chip, unsigned int irq)
> +{
> + int pos;
> + struct v2m_data *v2m = container_of(chip, struct v2m_data, msi_chip);
> +
> + spin_lock(>msi_cnt_lock);
Why do you need an extra lock here? Is that stuff not serialized from
the msi_chip layer already?
If not, why don't we have the serialization there instead of forcing
every callback to implement its own?
> + pos = irq - v2m->spi_start;
So this assumes that @irq is the hwirq number, right? How does the
calling function know about that? It should only have knowledge about
the virq number if I'm not missing something.
And if I'm missing something, then that msi_chip stuff is seriously
broken.
> + if (pos >= 0 && pos < v2m->nr_spis)
So you simply avoid the clear bitmap instead of yelling loudly about
being called with completely wrong data?
I would not be surprised if that is related to my question above.
> + bitmap_clear(v2m->bm, pos, 1);
> +static int gicv2m_setup_msi_irq(struct msi_chip *chip, struct pci_dev *pdev,
> + struct msi_desc *desc)
> +{
> + int hwirq, virq, offset;
> + struct v2m_data *v2m = container_of(chip, struct v2m_data, msi_chip);
> +
> + if (!desc)
> + return -EINVAL;
Why on earth does every callback of msi_chip have to check for this?
> + spin_lock(>msi_cnt_lock);
> + offset = bitmap_find_free_region(v2m->bm, v2m->nr_spis, 0);
> + spin_unlock(>msi_cnt_lock);
> + if (offset < 0)
> + return offset;
> +
> + hwirq = v2m->spi_start + offset;
> + virq = __irq_domain_alloc_irqs(v2m->domain, hwirq,
> +1, NUMA_NO_NODE, v2m, true);
> + if (virq < 0) {
> + gicv2m_teardown_msi_irq(chip, hwirq);
> + return virq;
> + }
> +
> + irq_domain_set_hwirq_and_chip(v2m->domain, virq, hwirq,
> + _chip, v2m);
> +
> + irq_set_msi_desc(hwirq, desc);
> + irq_set_irq_type(hwirq, IRQ_TYPE_EDGE_RISING);
Sure both calls work perfectly fine as long as virq == hwirq, right?
> + return 0;
Q: How does this populate virq to the caller?
A: Not at all
Q: How does the caller know which virq this function assigned?
A: Not at all
Q: How does the device driver know which virq to request?
A: Not at all
Q: Was this patch ever properly tested?
A: Not at all.
I do not care at all how YOU waste your time. But I care very much
about the fact that YOU are wasting MY precious time by exposing me to
your patch trainwrecks.
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
From: Suravee Suthikulpanit
ARM GICv2m specification extends GICv2 to support MSI(-X) with
a new set of register frame. This patch introduces support for
the non-secure GICv2m register frame. Currently, GICV2m is available
in certain version of GIC-400.
The patch introduces a new property in ARM gic binding, the v2m subnode.
It is optional.
Cc: Marc Zyngier
Cc: Thomas Gleixner
Cc: Mark Rutland
Cc: Jason Cooper
Cc: Will Deacon
Cc: Catalin Marinas
Signed-off-by: Suravee Suthikulpanit
---
Documentation/devicetree/bindings/arm/gic.txt | 53
arch/arm64/Kconfig| 1 +
drivers/irqchip/Kconfig | 5 +
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-gic-v2m.c | 340 ++
drivers/irqchip/irq-gic-v2m.h | 6 +
drivers/irqchip/irq-gic.c | 23 +-
7 files changed, 425 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/arm/gic.txt
b/Documentation/devicetree/bindings/arm/gic.txt
index c7d2fa1..ebf976a 100644
--- a/Documentation/devicetree/bindings/arm/gic.txt
+++ b/Documentation/devicetree/bindings/arm/gic.txt
@@ -96,3 +96,56 @@ Example:
<0x2c006000 0x2000>;
interrupts = <1 9 0xf04>;
};
+
+
+* GICv2m extension for MSI/MSI-x support (Optional)
+
+Certain revisions of GIC-400 supports MSI/MSI-x via V2M register frame(s).
+This is enabled by specifying v2m sub-node(s).
+
+Required properties:
+
+- compatible: The value here should contain "arm,gic-v2m-frame".
+
+- msi-controller: Identifies the node as an MSI controller.
+
+- reg : GICv2m MSI interface register base and size
+
+Optional properties:
+
+- arm,msi-base-spi : When the MSI_TYPER register contains an incorrect
+ value, this property should contain the SPI base of
+ the MSI frame, overriding the HW value.
+
+- arm,msi-num-spis : When the MSI_TYPER register contains an incorrect
+ value, this property should contain the number of
+ SPIs assigned to the frame, overriding the HW value.
+
+Example:
+
+ interrupt-controller@e1101000 {
+ compatible = "arm,gic-400";
+ #interrupt-cells = <3>;
+ #address-cells = <2>;
+ #size-cells = <2>;
+ interrupt-controller;
+ interrupts = <1 8 0xf04>;
+ ranges = <0 0 0 0xe110 0 0x10>;
+ reg = <0x0 0xe111 0 0x01000>,
+ <0x0 0xe112f000 0 0x02000>,
+ <0x0 0xe114 0 0x1>,
+ <0x0 0xe116 0 0x1>;
+ v2m0: v2m@0x8000 {
+ compatible = "arm,gic-v2m-frame";
+ msi-controller;
+ reg = <0x0 0x8 0 0x1000>;
+ };
+
+
+
+ v2mN: v2m@0x9000 {
+ compatible = "arm,gic-v2m-frame";
+ msi-controller;
+ reg = <0x0 0x9 0 0x1000>;
+ };
+ };
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index cde2f72..cbcde2d 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -12,6 +12,7 @@ config ARM64
select ARM_ARCH_TIMER
select ARM_GIC
select AUDIT_ARCH_COMPAT_GENERIC
+ select ARM_GIC_V2M
select ARM_GIC_V3
select BUILDTIME_EXTABLE_SORT
select CLONE_BACKWARDS
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 2a48e0a..39ce065 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -8,6 +8,11 @@ config ARM_GIC
select IRQ_DOMAIN_HIERARCHY
select MULTI_IRQ_HANDLER
+config ARM_GIC_V2M
+ bool
+ depends on ARM_GIC
+ depends on PCI && PCI_MSI
+
config GIC_NON_BANKED
bool
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 73052ba..3bda951 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_ARCH_SUNXI) += irq-sun4i.o
obj-$(CONFIG_ARCH_SUNXI) += irq-sunxi-nmi.o
obj-$(CONFIG_ARCH_SPEAR3XX)+= spear-shirq.o
obj-$(CONFIG_ARM_GIC) += irq-gic.o irq-gic-common.o
+obj-$(CONFIG_ARM_GIC_V2M) += irq-gic-v2m.o
obj-$(CONFIG_ARM_GIC_V3) += irq-gic-v3.o irq-gic-common.o
obj-$(CONFIG_ARM_NVIC) += irq-nvic.o
obj-$(CONFIG_ARM_VIC) += irq-vic.o
diff --git a/drivers/irqchip/irq-gic-v2m.c b/drivers/irqchip/irq-gic-v2m.c
new file mode 100644
index 000..fd8d51a
--- /dev/null
+++ b/drivers/irqchip/irq-gic-v2m.c
@@ -0,0 +1,340 @@
+/*
+ * ARM GIC v2m MSI(-X) support
+ * Support for Message Signaled Interrupts for systems that
+ * implement ARM Generic Interrupt Controller: GICv2m.
+ *
+ * Copyright (C) 2014
[V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
From: Suravee Suthikulpanit suravee.suthikulpa...@amd.com
ARM GICv2m specification extends GICv2 to support MSI(-X) with
a new set of register frame. This patch introduces support for
the non-secure GICv2m register frame. Currently, GICV2m is available
in certain version of GIC-400.
The patch introduces a new property in ARM gic binding, the v2m subnode.
It is optional.
Cc: Marc Zyngier marc.zyng...@arm.com
Cc: Thomas Gleixner t...@linutronix.de
Cc: Mark Rutland mark.rutl...@arm.com
Cc: Jason Cooper ja...@lakedaemon.net
Cc: Will Deacon will.dea...@arm.com
Cc: Catalin Marinas catalin.mari...@arm.com
Signed-off-by: Suravee Suthikulpanit suravee.suthikulpa...@amd.com
---
Documentation/devicetree/bindings/arm/gic.txt | 53
arch/arm64/Kconfig| 1 +
drivers/irqchip/Kconfig | 5 +
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-gic-v2m.c | 340 ++
drivers/irqchip/irq-gic-v2m.h | 6 +
drivers/irqchip/irq-gic.c | 23 +-
7 files changed, 425 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/arm/gic.txt
b/Documentation/devicetree/bindings/arm/gic.txt
index c7d2fa1..ebf976a 100644
--- a/Documentation/devicetree/bindings/arm/gic.txt
+++ b/Documentation/devicetree/bindings/arm/gic.txt
@@ -96,3 +96,56 @@ Example:
0x2c006000 0x2000;
interrupts = 1 9 0xf04;
};
+
+
+* GICv2m extension for MSI/MSI-x support (Optional)
+
+Certain revisions of GIC-400 supports MSI/MSI-x via V2M register frame(s).
+This is enabled by specifying v2m sub-node(s).
+
+Required properties:
+
+- compatible: The value here should contain arm,gic-v2m-frame.
+
+- msi-controller: Identifies the node as an MSI controller.
+
+- reg : GICv2m MSI interface register base and size
+
+Optional properties:
+
+- arm,msi-base-spi : When the MSI_TYPER register contains an incorrect
+ value, this property should contain the SPI base of
+ the MSI frame, overriding the HW value.
+
+- arm,msi-num-spis : When the MSI_TYPER register contains an incorrect
+ value, this property should contain the number of
+ SPIs assigned to the frame, overriding the HW value.
+
+Example:
+
+ interrupt-controller@e1101000 {
+ compatible = arm,gic-400;
+ #interrupt-cells = 3;
+ #address-cells = 2;
+ #size-cells = 2;
+ interrupt-controller;
+ interrupts = 1 8 0xf04;
+ ranges = 0 0 0 0xe110 0 0x10;
+ reg = 0x0 0xe111 0 0x01000,
+ 0x0 0xe112f000 0 0x02000,
+ 0x0 0xe114 0 0x1,
+ 0x0 0xe116 0 0x1;
+ v2m0: v2m@0x8000 {
+ compatible = arm,gic-v2m-frame;
+ msi-controller;
+ reg = 0x0 0x8 0 0x1000;
+ };
+
+
+
+ v2mN: v2m@0x9000 {
+ compatible = arm,gic-v2m-frame;
+ msi-controller;
+ reg = 0x0 0x9 0 0x1000;
+ };
+ };
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index cde2f72..cbcde2d 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -12,6 +12,7 @@ config ARM64
select ARM_ARCH_TIMER
select ARM_GIC
select AUDIT_ARCH_COMPAT_GENERIC
+ select ARM_GIC_V2M
select ARM_GIC_V3
select BUILDTIME_EXTABLE_SORT
select CLONE_BACKWARDS
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 2a48e0a..39ce065 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -8,6 +8,11 @@ config ARM_GIC
select IRQ_DOMAIN_HIERARCHY
select MULTI_IRQ_HANDLER
+config ARM_GIC_V2M
+ bool
+ depends on ARM_GIC
+ depends on PCI PCI_MSI
+
config GIC_NON_BANKED
bool
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 73052ba..3bda951 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_ARCH_SUNXI) += irq-sun4i.o
obj-$(CONFIG_ARCH_SUNXI) += irq-sunxi-nmi.o
obj-$(CONFIG_ARCH_SPEAR3XX)+= spear-shirq.o
obj-$(CONFIG_ARM_GIC) += irq-gic.o irq-gic-common.o
+obj-$(CONFIG_ARM_GIC_V2M) += irq-gic-v2m.o
obj-$(CONFIG_ARM_GIC_V3) += irq-gic-v3.o irq-gic-common.o
obj-$(CONFIG_ARM_NVIC) += irq-nvic.o
obj-$(CONFIG_ARM_VIC) += irq-vic.o
diff --git a/drivers/irqchip/irq-gic-v2m.c b/drivers/irqchip/irq-gic-v2m.c
new file mode 100644
index 000..fd8d51a
--- /dev/null
+++ b/drivers/irqchip/irq-gic-v2m.c
@@ -0,0 +1,340 @@
+/*
+ * ARM GIC v2m MSI(-X) support
+
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On Mon, 3 Nov 2014, suravee.suthikulpa...@amd.com wrote:
+static void gicv2m_teardown_msi_irq(struct msi_chip *chip, unsigned int irq)
+{
+ int pos;
+ struct v2m_data *v2m = container_of(chip, struct v2m_data, msi_chip);
+
+ spin_lock(v2m-msi_cnt_lock);
Why do you need an extra lock here? Is that stuff not serialized from
the msi_chip layer already?
If not, why don't we have the serialization there instead of forcing
every callback to implement its own?
+ pos = irq - v2m-spi_start;
So this assumes that @irq is the hwirq number, right? How does the
calling function know about that? It should only have knowledge about
the virq number if I'm not missing something.
And if I'm missing something, then that msi_chip stuff is seriously
broken.
+ if (pos = 0 pos v2m-nr_spis)
So you simply avoid the clear bitmap instead of yelling loudly about
being called with completely wrong data?
I would not be surprised if that is related to my question above.
+ bitmap_clear(v2m-bm, pos, 1);
+static int gicv2m_setup_msi_irq(struct msi_chip *chip, struct pci_dev *pdev,
+ struct msi_desc *desc)
+{
+ int hwirq, virq, offset;
+ struct v2m_data *v2m = container_of(chip, struct v2m_data, msi_chip);
+
+ if (!desc)
+ return -EINVAL;
Why on earth does every callback of msi_chip have to check for this?
+ spin_lock(v2m-msi_cnt_lock);
+ offset = bitmap_find_free_region(v2m-bm, v2m-nr_spis, 0);
+ spin_unlock(v2m-msi_cnt_lock);
+ if (offset 0)
+ return offset;
+
+ hwirq = v2m-spi_start + offset;
+ virq = __irq_domain_alloc_irqs(v2m-domain, hwirq,
+1, NUMA_NO_NODE, v2m, true);
+ if (virq 0) {
+ gicv2m_teardown_msi_irq(chip, hwirq);
+ return virq;
+ }
+
+ irq_domain_set_hwirq_and_chip(v2m-domain, virq, hwirq,
+ v2m_chip, v2m);
+
+ irq_set_msi_desc(hwirq, desc);
+ irq_set_irq_type(hwirq, IRQ_TYPE_EDGE_RISING);
Sure both calls work perfectly fine as long as virq == hwirq, right?
+ return 0;
Q: How does this populate virq to the caller?
A: Not at all
Q: How does the caller know which virq this function assigned?
A: Not at all
Q: How does the device driver know which virq to request?
A: Not at all
Q: Was this patch ever properly tested?
A: Not at all.
I do not care at all how YOU waste your time. But I care very much
about the fact that YOU are wasting MY precious time by exposing me to
your patch trainwrecks.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On 11/3/2014 4:51 PM, Thomas Gleixner wrote:
On Mon, 3 Nov 2014, suravee.suthikulpa...@amd.com wrote:
+static void gicv2m_teardown_msi_irq(struct msi_chip *chip, unsigned int irq)
+{
+ int pos;
+ struct v2m_data *v2m = container_of(chip, struct v2m_data, msi_chip);
+
+ spin_lock(v2m-msi_cnt_lock);
Why do you need an extra lock here? Is that stuff not serialized from
the msi_chip layer already?
If not, why don't we have the serialization there instead of forcing
every callback to implement its own?
From the following call paths:
|-- pci_enable_msi_range
|-- msi_capability_init
|-- arch_setup_msi_irqs
|-- arch_setup_msi_irq
and
|-- pci_enable_msix
|-- msix_capability_init
|-- arch_setup_msi_irqs
|-- arch_setup_msi_irq
It serialize when a PCI device driver tries to allocate multiple
interrupts. However, AFAICT, it would not serialize the allocation when
multiple drivers trying to setup MSI irqs at the same time. I needed
that to protect the bitmap structure. I also noticed the same in other
drivers as well.
I can look into this more to see where would be a good point.
+ pos = irq - v2m-spi_start;
So this assumes that @irq is the hwirq number, right? How does the
calling function know about that? It should only have knowledge about
the virq number if I'm not missing something.
And if I'm missing something, then that msi_chip stuff is seriously
broken.
It works this way because of the direct mapping (as you noticed). But I
am planning to change that. See below.
+ if (pos = 0 pos v2m-nr_spis)
So you simply avoid the clear bitmap instead of yelling loudly about
being called with completely wrong data?
I'll provide appropriate warnings.
I would not be surprised if that is related to my question above.
Not quite sure which of the above questions.
+ spin_lock(v2m-msi_cnt_lock);
+ offset = bitmap_find_free_region(v2m-bm, v2m-nr_spis, 0);
+ spin_unlock(v2m-msi_cnt_lock);
+ if (offset 0)
+ return offset;
+
+ hwirq = v2m-spi_start + offset;
+ virq = __irq_domain_alloc_irqs(v2m-domain, hwirq,
+ 1, NUMA_NO_NODE, v2m, true);
+ if (virq 0) {
+ gicv2m_teardown_msi_irq(chip, hwirq);
+ return virq;
+ }
+
+ irq_domain_set_hwirq_and_chip(v2m-domain, virq, hwirq,
+ v2m_chip, v2m);
+
+ irq_set_msi_desc(hwirq, desc);
+ irq_set_irq_type(hwirq, IRQ_TYPE_EDGE_RISING);
Sure both calls work perfectly fine as long as virq == hwirq, right?
I was running into an issue when calling the
irq_domain_alloc_irq_parent(), it requires of_phandle_args pointer to be
passed in. However, this does not work for GICv2m since it does not have
interrupt information in the device tree. So, I decided at first to use
direct (virq == hwirq) mapping, which simplifies the code a bit, but
might not be ideal solution, as you pointed out.
An alternative would be to create a temporary struct of_phandle_args,
and populate it with the interrupt information for the requested MSI.
Then pass it to:
-- irq_domain_alloc_irq_parent
|-- gic_irq_domain_alloc
|-- gic_irq_domain_xlate
|-- gic_irq_domain_map
However, this would still not be ideal if we want to support ACPI.
Another alternative would be coming up with a dedicate structure to be
used here. I noticed on X86, it uses struct irq_alloc_info. May be
that's what we also need here.
[...]
I do not care at all how YOU waste your time. But I care very much
about the fact that YOU are wasting MY precious time by exposing me to
your patch trainwrecks.
I don't intend to waste yours or anybody's precious time. Sorry if it
takes a couple iterations to work out the issues. Also, I will try to
put more comment in my code to make it more clear. Let me know what
works best for you to work out the issues.
Thanks,
Suravee
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
