On 2021-06-25 04:23, Thokala, Srikanth wrote:
Hi Lorenzo,
-----Original Message-----
From: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
Sent: Monday, June 21, 2021 10:23 PM
To: Thokala, Srikanth <srikanth.thokala@xxxxxxxxx>; maz@xxxxxxxxxx
Cc: robh+dt@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx;
devicetree@xxxxxxxxxxxxxxx; andriy.shevchenko@xxxxxxxxxxxxxxx;
mgross@xxxxxxxxxxxxxxx; Raja Subramanian, Lakshmi Bai
<lakshmi.bai.raja.subramanian@xxxxxxxxx>; Sangannavar, Mallikarjunappa
<mallikarjunappa.sangannavar@xxxxxxxxx>; kw@xxxxxxxxx
Subject: Re: [PATCH v10 2/2] PCI: keembay: Add support for Intel Keem
Bay
[+Marc]
On Mon, Jun 07, 2021 at 09:10:44PM +0530, srikanth.thokala@xxxxxxxxx
wrote:
[...]
> +static void keembay_pcie_msi_irq_handler(struct irq_desc *desc)
> +{
> + struct keembay_pcie *pcie = irq_desc_get_handler_data(desc);
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + u32 val, mask, status;
> + struct pcie_port *pp;
> +
> + chained_irq_enter(chip, desc);
> +
> + pp = &pcie->pci.pp;
> + val = readl(pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS);
> + mask = readl(pcie->apb_base + PCIE_REGS_INTERRUPT_ENABLE);
> +
> + status = val & mask;
> +
> + if (status & MSI_CTRL_INT) {
> + dw_handle_msi_irq(pp);
> + writel(status, pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS);
> + }
> +
> + chained_irq_exit(chip, desc);
> +}
> +
> +static int keembay_pcie_setup_msi_irq(struct keembay_pcie *pcie)
> +{
> + struct dw_pcie *pci = &pcie->pci;
> + struct device *dev = pci->dev;
> + struct platform_device *pdev = to_platform_device(dev);
> + int irq;
> +
> + irq = platform_get_irq_byname(pdev, "pcie");
> + if (irq < 0)
> + return irq;
> +
> + irq_set_chained_handler_and_data(irq, keembay_pcie_msi_irq_handler,
> + pcie);
> +
Ok this is yet another DWC MSI incantation and given that Marc worked
hard consolidating them let's have a look before we merge it.
IIUC - this IP relies on the DWC logic to handle MSIs + custom
registers to detect a pending MSI IRQ because the logic in
dw_chained_msi_irq() is *not* enough so you have to register
a driver specific chained handler. This looks similar to the dra7xx
driver MSI handling but I am not entirely convinced it is needed.
I assume this code in keembay_pcie_msi_irq_handler() is required
owing to additional IP logic on top of the standard DWC IP, in
particular the PCIE_REGS_INTERRUPT_STATUS write to "clear" the
IRQ.
We need more insights before merging it so please provide them.
pp = &pcie->pci.pp;
val = readl(pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS);
mask = readl(pcie->apb_base + PCIE_REGS_INTERRUPT_ENABLE);
status = val & mask;
if (status & MSI_CTRL_INT) {
dw_handle_msi_irq(pp);
writel(status, pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS);
}
Yes, your understanding is correct.
Additional registers PCIE_REGS_INTERRUPT_ENABLE/_STATUS are provided
by IP to control the interrupts.
To receive MSI interrupts, SW must enable bit '8' of _ENABLE register.
And once a MSI is raised by the End point, bit '8' of _STATUS register
will be set and it needs to be cleared after servicing the interrupt.
What isn't clear here is whether the other bits that are written back
are significant and have a side effect. If only bit 8 is required,
shouldn't you *only* write this bit back?
Only you can know the answer to this, but it would be good if you
could actually document this deviation from the already wonky
DWC infrastructure.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
