> Subject: Re: [PATCH v8 2/2] PCI: xilinx-cpm: Add Versal CPM Root Port driver
>
> On 2020-06-11 16:51, Bharat Kumar Gogada wrote:
>
> [...]
>
> >> > +/**
> >> > + * xilinx_cpm_pcie_init_port - Initialize hardware
> >> > + * @port: PCIe port information
> >> > + */
> >> > +static void xilinx_cpm_pcie_init_port(struct xilinx_cpm_pcie_port
> >> > *port)
> >> > +{
> >> > + if (cpm_pcie_link_up(port))
> >> > + dev_info(port->dev, "PCIe Link is UP\n");
> >> > + else
> >> > + dev_info(port->dev, "PCIe Link is DOWN\n");
> >> > +
> >> > + /* Disable all interrupts */
> >> > + pcie_write(port, ~XILINX_CPM_PCIE_IDR_ALL_MASK,
> >> > + XILINX_CPM_PCIE_REG_IMR);
> >> > +
> >> > + /* Clear pending interrupts */
> >> > + pcie_write(port, pcie_read(port, XILINX_CPM_PCIE_REG_IDR) &
> >> > + XILINX_CPM_PCIE_IMR_ALL_MASK,
> >> > + XILINX_CPM_PCIE_REG_IDR);
> >> > +
> >> > + /* Enable all interrupts */
> >> > + pcie_write(port, XILINX_CPM_PCIE_IMR_ALL_MASK,
> >> > + XILINX_CPM_PCIE_REG_IMR);
> >> > + pcie_write(port, XILINX_CPM_PCIE_IDRN_MASK,
> >> > + XILINX_CPM_PCIE_REG_IDRN_MASK);
> >>
> >> No. I've explained in the previous review why this was a terrible
> >> thing to do, and my patch got rid of it for a good reason.
> >>
> >> If the mask/unmask calls do not work, please explain what is wrong,
> >> and let's fix them. But we DO NOT enable interrupts outside of an
> >> irqchip callback, full stop.
> > The issue here is, we do not have any other register to enable
> > interrupts for above events, in order to see an interrupt assert for
> > these events, the respective mask bits shall be set to 1.
>
> I still disagree, because you're not explaining anything.
>
> We enable the interrupts as they are requested already (that's why we write
> to the these register in the respective mask/unmask callbacks). Why do you
> need to enable them ahead of the request?
HI Marc,
Yes agreed, this is not needed.
In xilinx_cpm_unmask_event_irq {
...
val |= d->hwirq; //which needs to be BIT(d->hwirq)
...
}
Did not notice this earlier that the required bit is not being set here.
Will fix it next patch.
Regards,
Bharat
