On Thu, 11 Mar 2021 09:47:45 +0000,
Jianjun Wang [off-list ref] wrote:

On Wed, 2021-03-10 at 09:41 +0000, Marc Zyngier wrote:

quoted

On Wed, 10 Mar 2021 06:48:49 +0000,
Jianjun Wang [off-list ref] wrote:

quoted

quoted

quoted

@@ -408,6 +677,14 @@ static void mtk_pcie_irq_handler(struct irq_desc *desc)
 		generic_handle_irq(virq);
 	}
 
+	irq_bit = PCIE_MSI_SHIFT;
+	for_each_set_bit_from(irq_bit, &status, PCIE_MSI_SET_NUM +
+			      PCIE_MSI_SHIFT) {
+		mtk_pcie_msi_handler(port, irq_bit - PCIE_MSI_SHIFT);
+
+		writel_relaxed(BIT(irq_bit), port->base + PCIE_INT_STATUS_REG);

Isn't this write the same thing you have for EOI in the INTx case?
While I could understand your description in that case (this is a
resampling operation), I don't get what this does here. Either this is
also an EOI, but your initial description doesn't make sense, or it is
an Ack, and it should be moved to the right place.

Which one is it?

I think it should be an EOI which used to clear the interrupt status of
a single set in the PCIe intc field, maybe I should move it to the end
of the mtk_pcie_msi_handler() function.

I doubt this is an EOI. If, as I suspect, it instructs the HW to clear
the bit so that new pending bits can be recorded, it must take place
*before* the interrupt is handled, or you may lose MSIs in the
interval between the handling of the interrupt and the clearing of the
pending bit. To satisfy this requirement, this should be an ACK, which
is consistent with the way most MSI controllers such as this one work.

These bits are similar with the interrupt status of INTx, and the
interrupt status will remain until all the status of the corresponding
set are cleared. There is a while loop in mtk_pcie_msi_handler() which
is used to continuously polling and ACK the status of the MSI set, I
think the MSI may not be lose in this case.

Ah, is that the write to PCIE_MSI_SET_STATUS_OFFSET that you are
referring to? In that case, yes, I agree.

However, this write to PCIE_INT_STATUS_REG is more a property of the
mux interrupt and not one of the MSI interrupt. Given that you do not
represent that level as another level of chained controller, you might
as well leave it where it is...

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek