Re: [v8,5/7] PCI: mediatek-gen3: Add MSI support

[Marc Zyngier <maz@kernel.org>]

On Thu, 11 Mar 2021 09:47:45 +0000,
Jianjun Wang <jianjun.wang@mediatek.com> wrote:
> 
> On Wed, 2021-03-10 at 09:41 +0000, Marc Zyngier wrote:
> > On Wed, 10 Mar 2021 06:48:49 +0000,
> > Jianjun Wang <jianjun.wang@mediatek.com> wrote:
> > > > > @@ -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