From: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
To: Andrew Murray <andrew.murray@arm.com>
Cc: maz@kernel.org, linux-kernel@vger.kernel.org,
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
Florian Fainelli <f.fainelli@gmail.com>,
bcm-kernel-feedback-list@broadcom.com,
Eric Anholt <eric@anholt.net>, Stefan Wahren <wahrenst@gmx.net>,
james.quinlan@broadcom.com, mbrugger@suse.com,
phil@raspberrypi.org, jeremy.linton@arm.com,
Bjorn Helgaas <bhelgaas@google.com>,
linux-pci@vger.kernel.org, linux-rpi-kernel@lists.infradead.org,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 5/6] PCI: brcmstb: add MSI capability
Date: Thu, 21 Nov 2019 18:19:12 +0100 [thread overview]
Message-ID: <85c4a01d4991a8593a9c3b56cf04bff38cc110e5.camel@suse.de> (raw)
In-Reply-To: <20191121153842.GZ43905@e119886-lin.cambridge.arm.com>
[-- Attachment #1: Type: text/plain, Size: 3491 bytes --]
On Thu, 2019-11-21 at 15:38 +0000, Andrew Murray wrote:
> > #define PCIE_MISC_PCIE_STATUS_PCIE_PHYLINKUP_SHIFT 0x4
> > #define PCIE_MISC_PCIE_STATUS_PCIE_LINK_IN_L23_MASK 0x40
> > #define PCIE_MISC_PCIE_STATUS_PCIE_LINK_IN_L23_SHIFT 0x6
> > +#define PCIE_MISC_REVISION_MAJMIN_MASK 0xffff
> > +#define PCIE_MISC_REVISION_MAJMIN_SHIFT 0
>
> I don't think these two are used.
Yes, in brcm_pcie_setup(), when we grab the PCIe hw revision number.
[...]
> > +static struct msi_domain_info brcm_msi_domain_info = {
> > + /* TODO: Multi MSI is technically supported by the controller */
>
> As I understand we're not supporting MSI_FLAG_MULTI_PCI_MSI, not because there
> is no hardware capability, but because the current use-case (RPi EPs) have no
> need for it. It wouldn't be a stretch to add this support (compare your alloc
> implementation with nwl_irq_domain_alloc from pcie-xilinx-nwl.c) - though I
> appreciate the difficulity you may have with testing.
>
> I'd probably replace the TODO line with:
>
> /* Multi MSI is supported by the controller, but not by this driver */
I'll replace the comment for now.
I've already seen people who unsoldered the XHCI chip on the RPi4 to then add a
proper PCI connector. If someone shows up with such setup, I'll be happy to
work out the MultiMSI support.
[...]
> > + .flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> > + MSI_FLAG_PCI_MSIX),
>
> Why the MSIX flag if the commit message says "It does not add MSIX since that
> functionality is not in the HW." in the commit message?
That's plain wrong, sorry.
[...]
> > + .chip = &brcm_msi_irq_chip,
> > +};
> > +
> > +static void brcm_pcie_msi_isr(struct irq_desc *desc)
> > +{
> > + struct irq_chip *chip = irq_desc_get_chip(desc);
> > + unsigned long status, virq;
> > + struct brcm_msi *msi;
> > + u32 mask, bit, hwirq;
> > + struct device *dev;
> > +
> > + chained_irq_enter(chip, desc);
> > + msi = irq_desc_get_handler_data(desc);
> > + mask = msi->intr_legacy_mask;
>
> NIT: As you only use this variable once, you could get rid of it.
OK
[...]
> > +
> > +static int brcm_pcie_enable_msi(struct brcm_pcie *pcie)
> > +{
> > + struct brcm_msi *msi;
> > + int irq, ret;
> > + struct device *dev = pcie->dev;
> > +
> > + irq = irq_of_parse_and_map(dev->of_node, 1);
> > + if (irq <= 0) {
> > + dev_err(dev, "cannot map msi intr\n");
>
> NIT: I think we can spare a few more characters and replace intr with
> interrupt.
Of course.
[...]
> > + /*
> > + * We ideally want the MSI target address to be located in the 32bit
> > + * addressable memory area. Some devices might depend on it. This is
> > + * possible either when the inbound window is located above the lower
> > + * 4GB or when the inbound and outbound areas fit in the lower 4GB of
> > + * memory.
> > + */
> > + if (rc_bar2_offset >= SZ_4G || (rc_bar2_size + rc_bar2_offset) <= SZ_4G)
> > + pcie->msi_target_addr = BRCM_MSI_TARGET_ADDR_LT_4GB;
> > + else
> > + pcie->msi_target_addr = BRCM_MSI_TARGET_ADDR_GT_4GB;
> > +
>
> Can this above hunk me moved into brcm_pcie_enable_msi? You could then avoid
> having the pcie->msi_target_addr and just have a single msi->target_addr
> variable?
As it depends on rc_bar2_offset and rc_bar2_size it's not really possible
without having to store those values in exchange, which IMO amounts to
negative benefit.
Regards,
Nicolas
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2019-11-21 17:19 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-12 15:59 [PATCH v2 0/6] Raspberry Pi 4 PCIe support Nicolas Saenz Julienne
2019-11-12 15:59 ` [PATCH v2 1/6] linux/log2.h: Add roundup/rounddown_pow_two64() family of functions Nicolas Saenz Julienne
2019-11-19 11:13 ` Andrew Murray
2019-11-19 11:30 ` Nicolas Saenz Julienne
2019-11-19 12:43 ` Nicolas Saenz Julienne
2019-11-19 16:28 ` Andrew Murray
2019-11-19 16:55 ` Jason Gunthorpe
2019-11-19 17:00 ` Andrew Murray
2019-11-12 15:59 ` [PATCH v2 2/6] dt-bindings: PCI: Add bindings for brcmstb's PCIe device Nicolas Saenz Julienne
2019-11-18 21:23 ` Rob Herring
2019-11-19 9:35 ` Nicolas Saenz Julienne
2019-11-19 11:17 ` Andrew Murray
2019-11-19 11:28 ` Nicolas Saenz Julienne
2019-11-12 15:59 ` [PATCH v2 3/6] ARM: dts: bcm2711: Enable PCIe controller Nicolas Saenz Julienne
2019-11-12 15:59 ` [PATCH v2 4/6] PCI: brcmstb: add Broadcom STB PCIe host controller driver Nicolas Saenz Julienne
2019-11-19 16:25 ` Andrew Murray
2019-11-19 18:20 ` Jeremy Linton
2019-11-20 20:24 ` Nicolas Saenz Julienne
2019-11-19 18:34 ` Florian Fainelli
2019-11-21 12:16 ` Andrew Murray
2019-11-20 19:53 ` Nicolas Saenz Julienne
2019-11-21 12:03 ` Andrew Murray
2019-11-21 12:59 ` Nicolas Saenz Julienne
2019-11-21 15:44 ` Andrew Murray
2019-11-21 21:07 ` Jim Quinlan
2019-11-22 14:59 ` Robin Murphy
2019-11-21 13:26 ` Nicolas Saenz Julienne
2019-11-21 15:46 ` Andrew Murray
2019-11-12 15:59 ` [PATCH v2 5/6] PCI: brcmstb: add MSI capability Nicolas Saenz Julienne
2019-11-13 13:49 ` Marc Zyngier
2019-11-21 15:38 ` Andrew Murray
2019-11-21 17:19 ` Nicolas Saenz Julienne [this message]
2019-11-12 15:59 ` [PATCH v2 6/6] MAINTAINERS: Add brcmstb PCIe controller Nicolas Saenz Julienne
2019-11-19 11:18 ` [PATCH v2 0/6] Raspberry Pi 4 PCIe support Andrew Murray
2019-11-19 11:49 ` Nicolas Saenz Julienne
2019-11-21 12:18 ` Andrew Murray
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=85c4a01d4991a8593a9c3b56cf04bff38cc110e5.camel@suse.de \
--to=nsaenzjulienne@suse.de \
--cc=andrew.murray@arm.com \
--cc=bcm-kernel-feedback-list@broadcom.com \
--cc=bhelgaas@google.com \
--cc=eric@anholt.net \
--cc=f.fainelli@gmail.com \
--cc=james.quinlan@broadcom.com \
--cc=jeremy.linton@arm.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=maz@kernel.org \
--cc=mbrugger@suse.com \
--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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).