From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.3 required=3.0 tests=BUG6152_INVALID_DATE_TZ_ABSURD,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID, INVALID_DATE_TZ_ABSURD,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE, SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 78944C43331 for ; Mon, 11 Nov 2019 13:29:29 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 492172196E for ; Mon, 11 Nov 2019 13:29:29 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="Pf20RKBx" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 492172196E Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:Message-ID:References:In-Reply-To:From:Date: MIME-Version:Subject:To:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=z+6WkbCPBVIjofyq6FYZ8HWQeMzt7fgiX9LirjlfsFU=; b=Pf20RKBxwUBJVuZErybMyvkZ5 qUWt/Ua+2DuHvRsK0kA9bcrxOIRSwt1Si8juiY40CY+cd1N0VlfN3xHYcXr8lhcjY0eeCMsy409Md Y+9pZ1+hoszHUfi7/ALeTb+ZEZYPfBoqUJXPntPuKpYxP+XGfXVk9Cgarb4Bde51vJpkyeIVYsnRz NhQ+ywr/aNJOq/gmOgYTzXTHbfsdzD4d80CPnTC6hvF0tXaHaj2ULEPmkP1l5vwIf8p9/MOnpRwtT uVKZ4qXZnhZ7NOQeTcs8Jd7EDu1CGobWsZn7AarqaHvE1IjdbMPaNLODoTG+KAK6ViXttQE2r4iII lGkOC4uJA==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1iU9kw-00076l-Ki; Mon, 11 Nov 2019 13:29:22 +0000 Received: from inca-roads.misterjones.org ([213.251.177.50]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1iU9kr-00073Q-IS; Mon, 11 Nov 2019 13:29:19 +0000 Received: from www-data by cheepnis.misterjones.org with local (Exim 4.80) (envelope-from ) id 1iU9km-0001uY-H2; Mon, 11 Nov 2019 14:29:12 +0100 To: Nicolas Saenz Julienne Subject: Re: [PATCH 4/4] PCI: brcmstb: add MSI capability X-PHP-Originating-Script: 0:main.inc MIME-Version: 1.0 Date: Mon, 11 Nov 2019 14:38:33 +0109 From: Marc Zyngier In-Reply-To: <86aeec16bc04d17372db5e33ffec0d5621973116.camel@suse.de> References: <20191106214527.18736-1-nsaenzjulienne@suse.de> <20191106214527.18736-5-nsaenzjulienne@suse.de> <86aeec16bc04d17372db5e33ffec0d5621973116.camel@suse.de> Message-ID: X-Sender: maz@kernel.org User-Agent: Roundcube Webmail/0.7.2 X-SA-Exim-Connect-IP: X-SA-Exim-Rcpt-To: nsaenzjulienne@suse.de, 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@arm.com, f.fainelli@gmail.com, mbrugger@suse.com, phil@raspberrypi.org, linux-kernel@vger.kernel.org, wahrenst@gmx.net, james.quinlan@broadcom.com, bhelgaas@google.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on cheepnis.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20191111_052917_765062_A11A07B9 X-CRM114-Status: GOOD ( 24.97 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: devicetree@vger.kernel.org, Lorenzo Pieralisi , mbrugger@suse.com, linux-pci@vger.kernel.org, phil@raspberrypi.org, linux-kernel@vger.kernel.org, Florian Fainelli , bcm-kernel-feedback-list@broadcom.com, linux-rpi-kernel@lists.infradead.org, james.quinlan@broadcom.com, Bjorn Helgaas , Andrew Murray , linux-arm-kernel@lists.infradead.org, wahrenst@gmx.net Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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 >> > >> > 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 >> > Co-developed-by: Nicolas Saenz Julienne >> > Signed-off-by: Nicolas Saenz Julienne >> > --- >> > 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