Re: [PATCH 1/5] PCI: intel-gw: Move interrupt enable to own function

[Manivannan Sadhasivam]

On Fri, Mar 27, 2026 at 09:18:49AM +0100, Florian Eckert wrote:
> Hello Mani,
> 
> On 2026-03-26 15:00, Manivannan Sadhasivam wrote:
> > On Tue, Mar 17, 2026 at 11:12:49AM +0100, Florian Eckert wrote:
> > > To improve the readability of the code, move the interrupt enable
> > > instructions to a separate function. That is already done for the
> > > disable interrupt instruction.
> > > 
> > > Signed-off-by: Florian Eckert <fe@xxxxxxxxxx>
> > > ---
> > >  drivers/pci/controller/dwc/pcie-intel-gw.c | 11 ++++++++---
> > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-intel-gw.c
> > > b/drivers/pci/controller/dwc/pcie-intel-gw.c
> > > index c21906eced61896c8a8307dbd6b72d229f9a5c5f..3a85bd0ef1b7f9414ce19fe56d82a78e34e9b648
> > > 100644
> > > --- a/drivers/pci/controller/dwc/pcie-intel-gw.c
> > > +++ b/drivers/pci/controller/dwc/pcie-intel-gw.c
> > > @@ -196,6 +196,13 @@ static void
> > > intel_pcie_device_rst_deassert(struct intel_pcie *pcie)
> > >  	gpiod_set_value_cansleep(pcie->reset_gpio, 0);
> > >  }
> > > 
> > > +static void intel_pcie_core_irq_enable(struct intel_pcie *pcie)
> > > +{
> > > +	pcie_app_wr(pcie, PCIE_APP_IRNEN, 0);
> > > +	pcie_app_wr(pcie, PCIE_APP_IRNCR, PCIE_APP_IRN_INT);
> > > +	pcie_app_wr(pcie, PCIE_APP_IRNEN, PCIE_APP_IRN_INT);
> > 
> > I'm confused. Previous code changed PCIE_APP_IRNEN register by writing
> > to
> > PCIE_APP_IRN_INT field. But this function is now changing PCIE_APP_IRNEN
> > and
> > PCIE_APP_IRNCR registers.
> 
> First, all pending interrupts are cleared and disabled, just as this is done
> in the disable function 'intel_pcie_core_irq_disable()' [1].
> 
> After that, all relevant interrupts are enabled. The `PCIE_APP_IRNEN`
> definition contains all the relevant interrupts that are of interest [2].
> 
> As I unfortunately don’t have any documentation for this IP core, I suspect
> that the intention is to set the IP core for interrupt handling to a
> specific
> state.
> 
> Perhaps the problem was that the IP core did not reinitialize the interrupt
> register properly after a power cycle. In my view, it can’t do any harm to
> switch the interrupt register off and then on again to set the Interrupts to
> a specific state. They do the same in their SDK [4]. Maxlinear is the only
> company that uses this IP core.
> 

Ok, fair enough. But you need to add the above info to the commit message
including the reference to driver snippet in Maxlinear repo.

- Mani

-- 
மணிவண்ணன் சதாசிவம்