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
