Re: [PATCH 2/4] PCI: mvebu: Implement support for interrupts on emulated bridge

["Pali Rohár" <pali@kernel.org>]

On Thursday 18 August 2022 21:51:21 Andrew Lunn wrote:
> > -static irqreturn_t mvebu_pcie_irq_handler(int irq, void *arg)
> > +static irqreturn_t mvebu_pcie_error_irq_handler(int irq, void *arg)
> > +{
> > +	struct mvebu_pcie_port *port = arg;
> > +	struct device *dev = &port->pcie->pdev->dev;
> > +	u32 cause, unmask, status;
> > +
> > +	cause = mvebu_readl(port, PCIE_INT_CAUSE_OFF);
> > +	unmask = mvebu_readl(port, PCIE_INT_UNMASK_OFF);
> > +	status = cause & unmask;
> > +
> > +	/* "error" interrupt handler does not process INTX interrupts */
> > +	status &= ~(PCIE_INT_INTX(0) | PCIE_INT_INTX(1) |
> > +		    PCIE_INT_INTX(2) | PCIE_INT_INTX(3));
> 
> Just for my understanding...
> 
> There are two interrupts

yes

> but the status information what those
> interrupts actually mean are all packed into one register?

yes

for masking individual interrupt events there is just one shared
register for both "intx" and "error" interrupt source.

and also there is also just one shared "cause" register which says which
individual interrupt events happened.

> I assume reading the clause register does not clear set bits?

yes, reading does not clear any interrupt event.

> Otherwise there
> would be a race condition.

> Are these actually level interrupts

yes

> and in order to clear them you need to poke some other register?

to clear individual interrupt event you have to write corresponding 1b
bit into that cause register.

so if interrupts events BIT(24), BIT(16) and BIT(17) happened and
BIT(24), BIT(25), BIT(26), BIT(27) and BIT(16) are unmasked then CPU
receives two interrupts (one for intx:24-27 and one for err:16). kernel
will call interrupt handlers for both intx and err (possible also in
parallel if it unmasked on different CPUs) and each handler just clears
events which process. So writing BIT(16) into cause register clears only
event 16 and all other (24-27, 17) are still active. And level interrupt
(the correct one intx or err) is then triggered again.

> > +	/*
> > +	 * Old DT bindings do not contain "error" interrupt
> > +	 * so do not fail probing driver when interrupt does not exist.
> > +	 */
> > +	port->error_irq = of_irq_get_byname(child, "error");
> > +	if (port->error_irq == -EPROBE_DEFER) {
> > +		ret = port->error_irq;
> > +		goto err;
> > +	}
> > +	if (port->error_irq <= 0) {
> > +		dev_warn(dev, "%s: interrupts on Root Port are unsupported, "
> 
> Maybe that should be "Error interrupts on Root..." ?
> 
>       Andrew

From PCIe Root Port point of view, it can be _any_ interrupt which can
PCIe Root Port (as PCI device) reports (it does not have to be PCIe AER
- error). Without this support PCIe Root Port cannot trigger or generate
any interrupt. Note that from PCIe point of view, this Marvell Root Port
never generates INTx interrupt. INTx interrupts are generated only by
endpoint devices. Marvell Root Port reports all events via those
_custom_ Marvell interrupts triggered by "error" source. So "error" here
means "Marvell error", not "PCIe error" (e.g. PCIe AER). For example
also PME events are reported via this "error" source. And PME is not
error.