From: Suravee Suthikulanit <suravee.suthikulpanit@amd.com> To: Marc Zyngier <marc.zyngier@arm.com> Cc: Mark Rutland <Mark.Rutland@arm.com>, "jason@lakedaemon.net" <jason@lakedaemon.net>, Pawel Moll <Pawel.Moll@arm.com>, Catalin Marinas <Catalin.Marinas@arm.com>, Will Deacon <Will.Deacon@arm.com>, "tglx@linutronix.de" <tglx@linutronix.de>, "Harish.Kasiviswanathan@amd.com" <Harish.Kasiviswanathan@amd.com>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>, "devicetree@vger.kernel.org" <devicetree@vger.kernel.org> Subject: Re: [PATCH 2/2 V4] irqchip: gicv2m: Add support for multiple MSI for ARM64 GICv2m Date: Fri, 15 Aug 2014 09:53:25 -0500 [thread overview] Message-ID: <53EE1EE5.3060103@amd.com> (raw) In-Reply-To: <87k369ri9k.fsf@approximate.cambridge.arm.com> On 8/15/2014 8:31 AM, Marc Zyngier wrote: > Hi Suravee, > >> +/* >> + * ARM64 function for seting up MSI irqs. >> + * Copied from driver/pci/msi.c: arch_setup_msi_irqs(). >> + */ >> +int arm64_setup_msi_irqs(struct pci_dev *dev, int nvec, int type) >> +{ >> + struct msi_desc *entry; >> + int ret; >> + >> + if (type == PCI_CAP_ID_MSI && nvec > 1) >> + return 1; >> + >> + list_for_each_entry(entry, &dev->msi_list, list) { >> + ret = arch_setup_msi_irq(dev, entry); >> + if (ret < 0) >> + return ret; >> + if (ret > 0) >> + return -ENOSPC; >> + } >> + >> + return 0; >> +} > > I'm going to reiterate what I said last time: Why do we need this? [Suravee] Marc, I understand what you described last time but I think there is one point that missing here. See below. > So far, we have two MSI-capable controllers on their way upstream: > GICv2m and GICv3. Both are perfectly capable of handling more than a > single MSI per device. [Suravee] I am aware of this. > So why should we cater for this? My gut feeling is that we should just > have: > > int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type) > { > struct msi_desc *entry; > int ret; > > /* > * So far, all our MSI controllers are capable of handling more > * than a single MSI per device. Should we encounter less > * capable devices, we'll consider doing something special for > * them. > */ > list_for_each_entry(entry, &dev->msi_list, list) { > ret = arch_setup_msi_irq(dev, entry); > if (ret < 0) > return ret; > if (ret > 0) > return -ENOSPC; > } > > return 0; > } > > and nothing else. Your driver should be able to retrieve the number of > MSI needed by the device, and allocate them. GICv3 manages it, and so > should GICv2m. > [Suravee] Multi-MSI and MSI-x are not the same. For MSI-X, you can treat each of the MSI separately since it MSI-X capability structure has a table specific for each one of them. For Multi-MSI, there is only one MSI capability structure which control all of them, and you need to program the "multiple-message enable" field with the encoding for "power-of-two", and therefore must be in contiguous range. Your logic above is what the standard MSI-x setup code is using. It is not handling of how many it can allocate all at once. As for sharing the logic b/w GICv2m and GICv3, unless they are sharing the same common data structure (e.g. the struct v2m which contans msi_chip), and the allocation function (e.g. generic gic_alloc_msi_irqs()), you pretty much need to do this separately since we need to walk the msi_chip back to its container structure. I am not saying this cannot be done, but we need to work out the detail together b/w GICv2m and GICv3. Thanks, Suravee
WARNING: multiple messages have this Message-ID (diff)
From: suravee.suthikulpanit@amd.com (Suravee Suthikulanit) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 2/2 V4] irqchip: gicv2m: Add support for multiple MSI for ARM64 GICv2m Date: Fri, 15 Aug 2014 09:53:25 -0500 [thread overview] Message-ID: <53EE1EE5.3060103@amd.com> (raw) In-Reply-To: <87k369ri9k.fsf@approximate.cambridge.arm.com> On 8/15/2014 8:31 AM, Marc Zyngier wrote: > Hi Suravee, > >> +/* >> + * ARM64 function for seting up MSI irqs. >> + * Copied from driver/pci/msi.c: arch_setup_msi_irqs(). >> + */ >> +int arm64_setup_msi_irqs(struct pci_dev *dev, int nvec, int type) >> +{ >> + struct msi_desc *entry; >> + int ret; >> + >> + if (type == PCI_CAP_ID_MSI && nvec > 1) >> + return 1; >> + >> + list_for_each_entry(entry, &dev->msi_list, list) { >> + ret = arch_setup_msi_irq(dev, entry); >> + if (ret < 0) >> + return ret; >> + if (ret > 0) >> + return -ENOSPC; >> + } >> + >> + return 0; >> +} > > I'm going to reiterate what I said last time: Why do we need this? [Suravee] Marc, I understand what you described last time but I think there is one point that missing here. See below. > So far, we have two MSI-capable controllers on their way upstream: > GICv2m and GICv3. Both are perfectly capable of handling more than a > single MSI per device. [Suravee] I am aware of this. > So why should we cater for this? My gut feeling is that we should just > have: > > int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type) > { > struct msi_desc *entry; > int ret; > > /* > * So far, all our MSI controllers are capable of handling more > * than a single MSI per device. Should we encounter less > * capable devices, we'll consider doing something special for > * them. > */ > list_for_each_entry(entry, &dev->msi_list, list) { > ret = arch_setup_msi_irq(dev, entry); > if (ret < 0) > return ret; > if (ret > 0) > return -ENOSPC; > } > > return 0; > } > > and nothing else. Your driver should be able to retrieve the number of > MSI needed by the device, and allocate them. GICv3 manages it, and so > should GICv2m. > [Suravee] Multi-MSI and MSI-x are not the same. For MSI-X, you can treat each of the MSI separately since it MSI-X capability structure has a table specific for each one of them. For Multi-MSI, there is only one MSI capability structure which control all of them, and you need to program the "multiple-message enable" field with the encoding for "power-of-two", and therefore must be in contiguous range. Your logic above is what the standard MSI-x setup code is using. It is not handling of how many it can allocate all at once. As for sharing the logic b/w GICv2m and GICv3, unless they are sharing the same common data structure (e.g. the struct v2m which contans msi_chip), and the allocation function (e.g. generic gic_alloc_msi_irqs()), you pretty much need to do this separately since we need to walk the msi_chip back to its container structure. I am not saying this cannot be done, but we need to work out the detail together b/w GICv2m and GICv3. Thanks, Suravee
next prev parent reply other threads:[~2014-08-15 14:53 UTC|newest] Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-08-13 15:00 [PATCH 0/2 V4] irqchip: gic: Introduce ARM GICv2m MSI(-X) support suravee.suthikulpanit 2014-08-13 15:00 ` suravee.suthikulpanit at amd.com 2014-08-13 15:00 ` suravee.suthikulpanit-5C7GfCeVMHo 2014-08-13 15:00 ` [PATCH 1/2 V4] irqchip: gic: Add supports for ARM GICv2m MSI(-X) suravee.suthikulpanit 2014-08-13 15:00 ` suravee.suthikulpanit at amd.com 2014-08-13 15:00 ` suravee.suthikulpanit 2014-08-14 2:56 ` Jingoo Han 2014-08-14 2:56 ` Jingoo Han 2014-08-28 9:15 ` Suravee Suthikulpanit 2014-08-28 9:15 ` Suravee Suthikulpanit 2014-08-28 9:15 ` Suravee Suthikulpanit 2014-08-14 17:55 ` Mark Rutland 2014-08-14 17:55 ` Mark Rutland 2014-08-14 17:55 ` Mark Rutland 2014-08-28 9:03 ` Suravee Suthikulpanit 2014-08-28 9:03 ` Suravee Suthikulpanit 2014-08-28 9:03 ` Suravee Suthikulpanit 2014-09-08 23:05 ` Suravee Suthikulpanit 2014-09-08 23:05 ` Suravee Suthikulpanit 2014-09-08 23:05 ` Suravee Suthikulpanit 2014-09-08 23:05 ` Suravee Suthikulpanit 2014-09-09 11:03 ` Mark Rutland 2014-09-09 11:03 ` Mark Rutland 2014-09-09 11:03 ` Mark Rutland 2014-08-15 14:03 ` Marc Zyngier 2014-08-15 14:03 ` Marc Zyngier 2014-08-15 14:03 ` Marc Zyngier 2014-08-15 14:03 ` Marc Zyngier 2014-08-28 8:59 ` Suravee Suthikulpanit 2014-08-28 8:59 ` Suravee Suthikulpanit 2014-08-28 8:59 ` Suravee Suthikulpanit 2014-09-05 16:15 ` Marc Zyngier 2014-09-05 16:15 ` Marc Zyngier 2014-09-05 16:15 ` Marc Zyngier 2014-09-05 16:15 ` Marc Zyngier 2014-08-13 15:00 ` [PATCH 2/2 V4] irqchip: gicv2m: Add support for multiple MSI for ARM64 GICv2m suravee.suthikulpanit 2014-08-13 15:00 ` suravee.suthikulpanit at amd.com 2014-08-13 15:00 ` suravee.suthikulpanit 2014-08-15 13:31 ` Marc Zyngier 2014-08-15 13:31 ` Marc Zyngier 2014-08-15 13:31 ` Marc Zyngier 2014-08-15 13:31 ` Marc Zyngier 2014-08-15 14:53 ` Suravee Suthikulanit [this message] 2014-08-15 14:53 ` Suravee Suthikulanit 2014-08-15 14:53 ` Suravee Suthikulanit 2014-08-15 15:08 ` Marc Zyngier 2014-08-15 15:08 ` Marc Zyngier 2014-08-15 15:08 ` Marc Zyngier 2014-08-15 15:08 ` Marc Zyngier
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=53EE1EE5.3060103@amd.com \ --to=suravee.suthikulpanit@amd.com \ --cc=Catalin.Marinas@arm.com \ --cc=Harish.Kasiviswanathan@amd.com \ --cc=Mark.Rutland@arm.com \ --cc=Pawel.Moll@arm.com \ --cc=Will.Deacon@arm.com \ --cc=devicetree@vger.kernel.org \ --cc=jason@lakedaemon.net \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-doc@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pci@vger.kernel.org \ --cc=marc.zyngier@arm.com \ --cc=tglx@linutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.