Re: [PATCH v2 5/6] PCI: brcmstb: add MSI capability

[Nicolas Saenz Julienne]

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: signature.asc
Description: This is a digitally signed message part