Re: [PATCH v5 5/7] PCI: qcom: Handle MSI IRQs properly

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 30/04/2022 01:47, Bjorn Helgaas wrote:
In subject, "Handle MSI IRQs properly" really doesn't tell us anything
useful.  The existing MSI support handles some MSI IRQs "properly," so
we should say something specific about the improvements here, like
"Handle multiple MSI groups" or "Handle MSIs routed to multiple GIC
interrupts" or "Handle split MSI IRQs" or similar.

On Sat, Apr 30, 2022 at 12:42:48AM +0300, Dmitry Baryshkov wrote:
On some of Qualcomm platforms each group of 32 MSI vectors is routed to the
separate GIC interrupt. Thus to receive higher MSI vectors properly,
add separate msi_host_init()/msi_host_deinit() handling additional host
IRQs.

+static void qcom_chained_msi_isr(struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	int irq = irq_desc_get_irq(desc);
+	struct pcie_port *pp;
+	int idx, pos;
+	unsigned long val;
+	u32 status, num_ctrls;
+	struct dw_pcie *pci;
+	struct qcom_pcie *pcie;
+
+	chained_irq_enter(chip, desc);
+
+	pp = irq_desc_get_handler_data(desc);
+	pci = to_dw_pcie_from_pp(pp);
+	pcie = to_qcom_pcie(pci);
+
+	/*
+	 * Unlike generic dw_handle_msi_irq we can determine, which group of
+	 * MSIs triggered the IRQ, so process just single group.

Parens and punctuation touch-up:

   Unlike generic dw_handle_msi_irq(), we can determine which group of
   MSIs triggered the IRQ, so process just that group.

+	 */
+	num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL;
+
+	for (idx = 0; idx < num_ctrls; idx++) {
+		if (pcie->msi_irqs[idx] == irq)
+			break;
+	}

Since this is basically an enhanced clone of dw_handle_msi_irq(), it
would be nice to use the same variable names ("i" instead of "idx")
so it's not gratuitously different.

Actually, I wonder if you could enhance dw_handle_msi_irq() slightly
so you could use it directly, e.g.,

     struct dw_pcie_host_ops {
       ...
       void (*msi_host_deinit)(struct pcie_port *pp);
  +    bool (*msi_in_block)(struct pcie_port *pp, int irq, int i);
     };

     dw_handle_msi_irq(...)
     {
       ...

       for (i = 0; i < num_ctrls; i++) {
  +      if (pp->ops->msi_in_block && !pp->ops->msi_in_block(pp, irq, i))
  +        continue;

	status = dw_pcie_readl_dbi(pci, PCIE_MSI_INTR0_STATUS ...);
	...

  +  bool qcom_pcie_msi_in_block(struct pcie_port *pp, int irq, int i)
  +  {
  +    ...
  +    pci = to_dw_pcie_from_pp(pp);
  +    pcie = to_qcom_pcie(pci);
  +
  +    if (pcie->msi_irqs[i] == irq)
  +      return true;
  +
  +    return false;
  +  }

Maybe that's more complicated than it's worth.

I think it will complicate the IRQ handler unnecessary. Just using a separate handler seems simpler.


+
+	if (WARN_ON_ONCE(unlikely(idx == num_ctrls)))
+		goto out;
+
+	status = dw_pcie_readl_dbi(pci, PCIE_MSI_INTR0_STATUS +
+				   (idx * MSI_REG_CTRL_BLOCK_SIZE));
+	if (!status)
+		goto out;
+
+	val = status;
+	pos = 0;
+	while ((pos = find_next_bit(&val, MAX_MSI_IRQS_PER_CTRL,
+				    pos)) != MAX_MSI_IRQS_PER_CTRL) {
+		generic_handle_domain_irq(pp->irq_domain,
+					  (idx * MAX_MSI_IRQS_PER_CTRL) +
+					  pos);
+		pos++;
+	}
+
+out:
+	chained_irq_exit(chip, desc);
+}


--
With best wishes
Dmitry



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux