From: Marc Zyngier <maz@kernel.org> To: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> Cc: Andrew Murray <andrew.murray@arm.com>, <linux-pci@vger.kernel.org>, <devicetree@vger.kernel.org>, <bcm-kernel-feedback-list@broadcom.com>, <linux-rpi-kernel@lists.infradead.org>, <linux-arm-kernel@lists.infradead.org>, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>, Florian Fainelli <f.fainelli@gmail.com>, <mbrugger@suse.com>, <phil@raspberrypi.org>, <linux-kernel@vger.kernel.org>, <wahrenst@gmx.net>, <james.quinlan@broadcom.com>, Bjorn Helgaas <bhelgaas@google.com> Subject: Re: [PATCH 4/4] PCI: brcmstb: add MSI capability Date: Mon, 11 Nov 2019 14:38:33 +0109 [thread overview] Message-ID: <e12adb8d4f3be328318c8b911f4ba611@www.loen.fr> (raw) In-Reply-To: <86aeec16bc04d17372db5e33ffec0d5621973116.camel@suse.de> Hi Nicolas, On 2019-11-11 12:31, Nicolas Saenz Julienne wrote: > Hi Marc, > thanks for the review! > > On Thu, 2019-11-07 at 16:49 +0109, Marc Zyngier wrote: >> On 2019-11-06 22:54, Nicolas Saenz Julienne wrote: >> > From: Jim Quinlan <james.quinlan@broadcom.com> >> > >> > This commit adds MSI to the Broadcom STB PCIe host controller. It >> > does >> > not add MSIX since that functionality is not in the HW. The MSI >> > controller is physically located within the PCIe block, however, >> > there >> > is no reason why the MSI controller could not be moved elsewhere >> in >> > the future. >> > >> > Since the internal Brcmstb MSI controller is intertwined with the >> > PCIe >> > controller, it is not its own platform device but rather part of >> the >> > PCIe platform device. >> > >> > This is based on Jim's original submission[1] with some slight >> > changes >> > regarding how pcie->msi_target_addr is decided. >> > >> > [1] https://patchwork.kernel.org/patch/10605955/ >> > >> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> >> > Co-developed-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> >> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> >> > --- >> > drivers/pci/controller/Kconfig | 2 +- >> > drivers/pci/controller/pcie-brcmstb.c | 333 >> > +++++++++++++++++++++++++- >> > 2 files changed, 332 insertions(+), 3 deletions(-) [...] >> > +static struct msi_domain_info brcm_msi_domain_info = { >> > + .flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS | >> > + MSI_FLAG_PCI_MSIX), >> >> Is there a particular reason for not supporting MultiMSI? I won't >> miss >> it, but it might be worth documenting the restriction if the HW >> cannot >> support it (though I can't immediately see why). > > There is no actual restriction. As Jim tells me, there never was the > need for > it. If it's fine with you, we'll leave that as an enhancement for the > future, > specially since the RPi's XHCI device only uses one MSI interrupt. Sure, that's fine. But as soon as someone takes this SoC and sticks it on a different board (RPi CM4 anyone?), this will become a requirement (I thought MultiMSI dead 4 years ago, and have been proved wrong many times since). > >> > + .chip = &brcm_msi_irq_chip, >> > +}; >> > + >> > +static void brcm_pcie_msi_isr(struct irq_desc *desc) >> > +{ >> > + struct irq_chip *chip = irq_desc_get_chip(desc); >> > + struct brcm_msi *msi; >> > + unsigned long status, virq; >> > + u32 mask, bit, hwirq; >> > + struct device *dev; >> > + >> > + chained_irq_enter(chip, desc); >> > + msi = irq_desc_get_handler_data(desc); >> > + mask = msi->intr_legacy_mask; >> > + dev = msi->dev; >> > + >> > + while ((status = bcm_readl(msi->intr_base + STATUS) & mask)) { >> >> Is this loop really worth it? If, as I imagine, this register is at >> the >> end of a wet piece of string, this additional read (likely to return >> zero) >> will have a measurable latency impact... > > I think this one was cargo-culted, TBH this pattern is all over the > place. > Though, now that you point it out, I can't really provide a > justification for > it. Maybe Jim can contradict me here, but It's working fine without > it. I know this pattern is ultra common (hey, the GIC uses it), but I'm somehow doubtful of its benefit. On GICv3, not reading the status register again has given us a performance boost for most workloads. [...] >> > + /* >> > + * Make sure we are not masking MSIs. Note that MSIs can be >> > masked, >> > + * but that occurs on the PCIe EP device >> >> That's not a guarantee, specially with plain MultiMSI. I'm actually >> minded to move the masking to be purely local on the MSI controllers >> I maintain. > > Sorry, I'm a little lost here. The way I understand it after reset, > even with > multiMSI, on the EP side all vectors are umasked. So it would make > sense to do > the same on the controller. > > The way I see it, we want to avoid using this register anyway, as > with multiMSI > we'd only get function wide masking, which I guess is not all that > useful. Yeah, I wasn't 100% clear. Unless you have MSI-X, there is no guarantee to have a mask bit per MSI. Multi-MSI definitely has only this problem. My advice would be to let the PCI layer deal with enabling/disabling interrupts at the endpoint level, and let this driver manage the masking at its own level, using the MASK registers. Thanks, M. -- Jazz is not dead. It just smells funny...
WARNING: multiple messages have this Message-ID (diff)
From: Marc Zyngier <maz@kernel.org> To: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> Cc: devicetree@vger.kernel.org, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>, mbrugger@suse.com, linux-pci@vger.kernel.org, phil@raspberrypi.org, linux-kernel@vger.kernel.org, Florian Fainelli <f.fainelli@gmail.com>, bcm-kernel-feedback-list@broadcom.com, linux-rpi-kernel@lists.infradead.org, james.quinlan@broadcom.com, Bjorn Helgaas <bhelgaas@google.com>, Andrew Murray <andrew.murray@arm.com>, linux-arm-kernel@lists.infradead.org, wahrenst@gmx.net Subject: Re: [PATCH 4/4] PCI: brcmstb: add MSI capability Date: Mon, 11 Nov 2019 14:38:33 +0109 [thread overview] Message-ID: <e12adb8d4f3be328318c8b911f4ba611@www.loen.fr> (raw) In-Reply-To: <86aeec16bc04d17372db5e33ffec0d5621973116.camel@suse.de> Hi Nicolas, On 2019-11-11 12:31, Nicolas Saenz Julienne wrote: > Hi Marc, > thanks for the review! > > On Thu, 2019-11-07 at 16:49 +0109, Marc Zyngier wrote: >> On 2019-11-06 22:54, Nicolas Saenz Julienne wrote: >> > From: Jim Quinlan <james.quinlan@broadcom.com> >> > >> > This commit adds MSI to the Broadcom STB PCIe host controller. It >> > does >> > not add MSIX since that functionality is not in the HW. The MSI >> > controller is physically located within the PCIe block, however, >> > there >> > is no reason why the MSI controller could not be moved elsewhere >> in >> > the future. >> > >> > Since the internal Brcmstb MSI controller is intertwined with the >> > PCIe >> > controller, it is not its own platform device but rather part of >> the >> > PCIe platform device. >> > >> > This is based on Jim's original submission[1] with some slight >> > changes >> > regarding how pcie->msi_target_addr is decided. >> > >> > [1] https://patchwork.kernel.org/patch/10605955/ >> > >> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> >> > Co-developed-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> >> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> >> > --- >> > drivers/pci/controller/Kconfig | 2 +- >> > drivers/pci/controller/pcie-brcmstb.c | 333 >> > +++++++++++++++++++++++++- >> > 2 files changed, 332 insertions(+), 3 deletions(-) [...] >> > +static struct msi_domain_info brcm_msi_domain_info = { >> > + .flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS | >> > + MSI_FLAG_PCI_MSIX), >> >> Is there a particular reason for not supporting MultiMSI? I won't >> miss >> it, but it might be worth documenting the restriction if the HW >> cannot >> support it (though I can't immediately see why). > > There is no actual restriction. As Jim tells me, there never was the > need for > it. If it's fine with you, we'll leave that as an enhancement for the > future, > specially since the RPi's XHCI device only uses one MSI interrupt. Sure, that's fine. But as soon as someone takes this SoC and sticks it on a different board (RPi CM4 anyone?), this will become a requirement (I thought MultiMSI dead 4 years ago, and have been proved wrong many times since). > >> > + .chip = &brcm_msi_irq_chip, >> > +}; >> > + >> > +static void brcm_pcie_msi_isr(struct irq_desc *desc) >> > +{ >> > + struct irq_chip *chip = irq_desc_get_chip(desc); >> > + struct brcm_msi *msi; >> > + unsigned long status, virq; >> > + u32 mask, bit, hwirq; >> > + struct device *dev; >> > + >> > + chained_irq_enter(chip, desc); >> > + msi = irq_desc_get_handler_data(desc); >> > + mask = msi->intr_legacy_mask; >> > + dev = msi->dev; >> > + >> > + while ((status = bcm_readl(msi->intr_base + STATUS) & mask)) { >> >> Is this loop really worth it? If, as I imagine, this register is at >> the >> end of a wet piece of string, this additional read (likely to return >> zero) >> will have a measurable latency impact... > > I think this one was cargo-culted, TBH this pattern is all over the > place. > Though, now that you point it out, I can't really provide a > justification for > it. Maybe Jim can contradict me here, but It's working fine without > it. I know this pattern is ultra common (hey, the GIC uses it), but I'm somehow doubtful of its benefit. On GICv3, not reading the status register again has given us a performance boost for most workloads. [...] >> > + /* >> > + * Make sure we are not masking MSIs. Note that MSIs can be >> > masked, >> > + * but that occurs on the PCIe EP device >> >> That's not a guarantee, specially with plain MultiMSI. I'm actually >> minded to move the masking to be purely local on the MSI controllers >> I maintain. > > Sorry, I'm a little lost here. The way I understand it after reset, > even with > multiMSI, on the EP side all vectors are umasked. So it would make > sense to do > the same on the controller. > > The way I see it, we want to avoid using this register anyway, as > with multiMSI > we'd only get function wide masking, which I guess is not all that > useful. Yeah, I wasn't 100% clear. Unless you have MSI-X, there is no guarantee to have a mask bit per MSI. Multi-MSI definitely has only this problem. My advice would be to let the PCI layer deal with enabling/disabling interrupts at the endpoint level, and let this driver manage the masking at its own level, using the MASK registers. Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-11-11 13:29 UTC|newest] Thread overview: 62+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-11-06 21:45 [PATCH 0/4] Raspberry Pi 4 PCIe support Nicolas Saenz Julienne 2019-11-06 21:45 ` Nicolas Saenz Julienne 2019-11-06 21:45 ` [PATCH 1/4] dt-bindings: pci: add bindings for brcmstb's PCIe device Nicolas Saenz Julienne 2019-11-06 21:45 ` Nicolas Saenz Julienne 2019-11-07 10:32 ` Andrew Murray 2019-11-07 10:32 ` Andrew Murray 2019-11-07 10:53 ` Nicolas Saenz Julienne 2019-11-07 10:53 ` Nicolas Saenz Julienne 2019-11-13 4:15 ` Rob Herring 2019-11-13 4:15 ` Rob Herring 2019-11-14 13:15 ` Nicolas Saenz Julienne 2019-11-14 13:15 ` Nicolas Saenz Julienne 2019-11-06 21:45 ` [PATCH 2/4] ARM: dts: bcm2711: Enable PCIe controller Nicolas Saenz Julienne 2019-11-06 21:45 ` Nicolas Saenz Julienne 2019-11-07 10:37 ` Andrew Murray 2019-11-07 10:37 ` Andrew Murray 2019-11-12 9:18 ` Nicolas Saenz Julienne 2019-11-12 9:18 ` Nicolas Saenz Julienne 2019-11-07 17:44 ` Stefan Wahren 2019-11-07 17:44 ` Stefan Wahren 2019-11-07 18:24 ` Nicolas Saenz Julienne 2019-11-07 18:24 ` Nicolas Saenz Julienne 2019-11-06 21:45 ` [PATCH 3/4] PCI: brcmstb: add Broadcom STB PCIe host controller driver Nicolas Saenz Julienne 2019-11-06 21:45 ` Nicolas Saenz Julienne 2019-11-07 15:00 ` Andrew Murray 2019-11-07 15:00 ` Andrew Murray 2019-11-07 16:12 ` Jim Quinlan 2019-11-07 16:12 ` Jim Quinlan 2019-11-08 10:52 ` Andrew Murray 2019-11-08 10:52 ` Andrew Murray 2019-11-08 16:33 ` Jim Quinlan 2019-11-08 16:33 ` Jim Quinlan 2019-11-07 17:30 ` Nicolas Saenz Julienne 2019-11-07 17:30 ` Nicolas Saenz Julienne 2019-11-08 10:51 ` Andrew Murray 2019-11-08 10:51 ` Andrew Murray 2019-11-07 17:50 ` Stefan Wahren 2019-11-07 17:50 ` Stefan Wahren 2019-11-08 11:13 ` Nicolas Saenz Julienne 2019-11-08 11:13 ` Nicolas Saenz Julienne 2019-11-11 7:10 ` Jeremy Linton 2019-11-11 7:10 ` Jeremy Linton 2019-11-11 15:29 ` Nicolas Saenz Julienne 2019-11-11 15:29 ` Nicolas Saenz Julienne 2019-11-11 16:40 ` Florian Fainelli 2019-11-11 16:40 ` Florian Fainelli 2019-11-11 20:00 ` Jeremy Linton 2019-11-11 20:00 ` Jeremy Linton 2019-11-11 21:27 ` Florian Fainelli 2019-11-11 21:27 ` Florian Fainelli 2019-11-06 21:45 ` [PATCH 4/4] PCI: brcmstb: add MSI capability Nicolas Saenz Julienne 2019-11-06 21:45 ` Nicolas Saenz Julienne 2019-11-07 15:40 ` Marc Zyngier 2019-11-07 15:40 ` Marc Zyngier 2019-11-11 11:21 ` Nicolas Saenz Julienne 2019-11-11 11:21 ` Nicolas Saenz Julienne 2019-11-11 13:29 ` Marc Zyngier [this message] 2019-11-11 13:29 ` Marc Zyngier 2019-11-06 21:51 ` [PATCH 0/4] Raspberry Pi 4 PCIe support Florian Fainelli 2019-11-06 21:51 ` Florian Fainelli 2019-11-07 9:58 ` Nicolas Saenz Julienne 2019-11-07 9:58 ` Nicolas Saenz Julienne
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=e12adb8d4f3be328318c8b911f4ba611@www.loen.fr \ --to=maz@kernel.org \ --cc=andrew.murray@arm.com \ --cc=bcm-kernel-feedback-list@broadcom.com \ --cc=bhelgaas@google.com \ --cc=devicetree@vger.kernel.org \ --cc=f.fainelli@gmail.com \ --cc=james.quinlan@broadcom.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pci@vger.kernel.org \ --cc=linux-rpi-kernel@lists.infradead.org \ --cc=lorenzo.pieralisi@arm.com \ --cc=mbrugger@suse.com \ --cc=nsaenzjulienne@suse.de \ --cc=phil@raspberrypi.org \ --cc=wahrenst@gmx.net \ /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.