[v1] PCI: intel-gw: Fixes to make the driver working again

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

Posted by Florian Eckert 2 weeks, 6 days ago

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@dev.tdt.de>
---
 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);
+}
+
 static void intel_pcie_core_irq_disable(struct intel_pcie *pcie)
 {
 	pcie_app_wr(pcie, PCIE_APP_IRNEN, 0);
@@ -317,9 +324,7 @@ static int intel_pcie_host_setup(struct intel_pcie *pcie)
 	if (ret)
 		goto app_init_err;
 
-	/* Enable integrated interrupts */
-	pcie_app_wr_mask(pcie, PCIE_APP_IRNEN, PCIE_APP_IRN_INT,
-			 PCIE_APP_IRN_INT);
+	intel_pcie_core_irq_enable(pcie);
 
 	return 0;
 

-- 
2.47.3

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

Posted by Manivannan Sadhasivam 1 week, 4 days ago

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@dev.tdt.de>
> ---
>  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.

- Mani

> +}
> +
>  static void intel_pcie_core_irq_disable(struct intel_pcie *pcie)
>  {
>  	pcie_app_wr(pcie, PCIE_APP_IRNEN, 0);
> @@ -317,9 +324,7 @@ static int intel_pcie_host_setup(struct intel_pcie *pcie)
>  	if (ret)
>  		goto app_init_err;
>  
> -	/* Enable integrated interrupts */
> -	pcie_app_wr_mask(pcie, PCIE_APP_IRNEN, PCIE_APP_IRN_INT,
> -			 PCIE_APP_IRN_INT);
> +	intel_pcie_core_irq_enable(pcie);
>  
>  	return 0;
>  
> 
> -- 
> 2.47.3
> 

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

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

Posted by Florian Eckert 1 week, 3 days ago

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@dev.tdt.de>
>> ---
>>  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.

- Florian

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-intel-gw.c?h=master#n199
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-intel-gw.c?h=master#n52
[3] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-intel-gw.c?h=master#n321
[4] 
https://github.com/maxlinear/linux/blob/updk_9.1.90/drivers/pci/controller/dwc/pcie-intel-gw.c#L431

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

Posted by Manivannan Sadhasivam 1 week, 3 days ago

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@dev.tdt.de>
> > > ---
> > >  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

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