Re: [PATCH v8 2/2] PCI: xilinx-cpm: Add Versal CPM Root Port driver

[Marc Zyngier]

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?

        M.
--
Jazz is not dead. It just smells funny...