public inbox for linux-pci@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] PCI: qcom: Enable MSI interrupts together with Link up if global IRQ is supported
@ 2024-09-30 13:44 Manivannan Sadhasivam
  2024-09-30 17:11 ` Bjorn Helgaas
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Manivannan Sadhasivam @ 2024-09-30 13:44 UTC (permalink / raw)
  To: lpieralisi, kw
  Cc: robh, bhelgaas, linux-pci, linux-arm-msm, linux-kernel,
	quic_qianyu, Manivannan Sadhasivam, Konrad Dybcio

Currently, if global IRQ is supported by the platform, only the Link up
interrupt is enabled in the PARF_INT_ALL_MASK register. But on some Qcom
platforms like SM8250, and X1E80100, MSIs are getting masked due to this.
They require enabling the MSI interrupt bits in the register to unmask
(enable) the MSIs.

Even though the MSI interrupt enable bits in PARF_INT_ALL_MASK are
described as 'diagnostic' interrupts in the internal documentation,
disabling them masks MSI on these platforms. Due to this, MSIs were not
reported to be received these platforms while supporting global IRQ.

So enable the MSI interrupts along with the Link up interrupt in the
PARF_INT_ALL_MASK register if global IRQ is supported. This ensures that
the MSIs continue to work and also the driver is able to catch the Link
up interrupt for enumerating endpoint devices.

Fixes: 4581403f6792 ("PCI: qcom: Enumerate endpoints based on Link up event in 'global_irq' interrupt")
Reported-by: Konrad Dybcio <konradybcio@kernel.org>
Closes: https://lore.kernel.org/linux-pci/9a692c98-eb0a-4d86-b642-ea655981ff53@kernel.org/
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index ef44a82be058..2b33d03ed054 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -133,6 +133,7 @@
 
 /* PARF_INT_ALL_{STATUS/CLEAR/MASK} register fields */
 #define PARF_INT_ALL_LINK_UP			BIT(13)
+#define PARF_INT_MSI_DEV_0_7			GENMASK(30, 23)
 
 /* PARF_NO_SNOOP_OVERIDE register fields */
 #define WR_NO_SNOOP_OVERIDE_EN			BIT(1)
@@ -1716,7 +1717,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 			goto err_host_deinit;
 		}
 
-		writel_relaxed(PARF_INT_ALL_LINK_UP, pcie->parf + PARF_INT_ALL_MASK);
+		writel_relaxed(PARF_INT_ALL_LINK_UP | PARF_INT_MSI_DEV_0_7,
+			       pcie->parf + PARF_INT_ALL_MASK);
 	}
 
 	qcom_pcie_icc_opp_update(pcie);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] PCI: qcom: Enable MSI interrupts together with Link up if global IRQ is supported
  2024-09-30 13:44 [PATCH] PCI: qcom: Enable MSI interrupts together with Link up if global IRQ is supported Manivannan Sadhasivam
@ 2024-09-30 17:11 ` Bjorn Helgaas
  2024-10-01  4:20   ` Manivannan Sadhasivam
  2024-10-02  1:10 ` Qiang Yu
  2024-10-04  9:23 ` Konrad Dybcio
  2 siblings, 1 reply; 7+ messages in thread
From: Bjorn Helgaas @ 2024-09-30 17:11 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: lpieralisi, kw, robh, bhelgaas, linux-pci, linux-arm-msm,
	linux-kernel, quic_qianyu, Konrad Dybcio

On Mon, Sep 30, 2024 at 07:14:09PM +0530, Manivannan Sadhasivam wrote:
> Currently, if global IRQ is supported by the platform, only the Link up
> interrupt is enabled in the PARF_INT_ALL_MASK register. But on some Qcom
> platforms like SM8250, and X1E80100, MSIs are getting masked due to this.
> They require enabling the MSI interrupt bits in the register to unmask
> (enable) the MSIs.

"global IRQ" is a very generic name.  If that's the official name, it
should at least be capitalized, e.g., "Global IRQ", to show that it is
a proper noun that refers to a specific IRQ.

> Even though the MSI interrupt enable bits in PARF_INT_ALL_MASK are
> described as 'diagnostic' interrupts in the internal documentation,
> disabling them masks MSI on these platforms. Due to this,

> MSIs were not
> reported to be received these platforms while supporting global IRQ.

I'm trying to parse "while supporting global IRQ."  We basically
support global IRQ by installing qcom_pcie_global_irq_thread(), but of
course the device doesn't see that, so I assume it would be more
informative to say that MSIs are masked by some register setting.

The patch suggests that MSIs are masked internally unless
PARF_INT_MSI_DEV_0_7 is set in PARF_INT_ALL_MASK.

Are you saying that prior to 4581403f6792, MSIs did work?  Does that
mean PARF_INT_MSI_DEV_0_7 was set by a bootloader or something, so
MSIs worked?  And then 4581403f6792 came along and implicitly cleared
PARF_INT_MSI_DEV_0_7, so MSIs were then masked?

> So enable the MSI interrupts along with the Link up interrupt in the
> PARF_INT_ALL_MASK register if global IRQ is supported. This ensures that
> the MSIs continue to work and also the driver is able to catch the Link
> up interrupt for enumerating endpoint devices.
> 
> Fixes: 4581403f6792 ("PCI: qcom: Enumerate endpoints based on Link up event in 'global_irq' interrupt")
> Reported-by: Konrad Dybcio <konradybcio@kernel.org>
> Closes: https://lore.kernel.org/linux-pci/9a692c98-eb0a-4d86-b642-ea655981ff53@kernel.org/
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index ef44a82be058..2b33d03ed054 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -133,6 +133,7 @@
>  
>  /* PARF_INT_ALL_{STATUS/CLEAR/MASK} register fields */
>  #define PARF_INT_ALL_LINK_UP			BIT(13)
> +#define PARF_INT_MSI_DEV_0_7			GENMASK(30, 23)
>  
>  /* PARF_NO_SNOOP_OVERIDE register fields */
>  #define WR_NO_SNOOP_OVERIDE_EN			BIT(1)
> @@ -1716,7 +1717,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  			goto err_host_deinit;
>  		}
>  
> -		writel_relaxed(PARF_INT_ALL_LINK_UP, pcie->parf + PARF_INT_ALL_MASK);
> +		writel_relaxed(PARF_INT_ALL_LINK_UP | PARF_INT_MSI_DEV_0_7,
> +			       pcie->parf + PARF_INT_ALL_MASK);
>  	}
>  
>  	qcom_pcie_icc_opp_update(pcie);
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] PCI: qcom: Enable MSI interrupts together with Link up if global IRQ is supported
  2024-09-30 17:11 ` Bjorn Helgaas
@ 2024-10-01  4:20   ` Manivannan Sadhasivam
  2024-10-01 21:19     ` Bjorn Helgaas
  0 siblings, 1 reply; 7+ messages in thread
From: Manivannan Sadhasivam @ 2024-10-01  4:20 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: lpieralisi, kw, robh, bhelgaas, linux-pci, linux-arm-msm,
	linux-kernel, quic_qianyu, Konrad Dybcio

On Mon, Sep 30, 2024 at 12:11:01PM -0500, Bjorn Helgaas wrote:
> On Mon, Sep 30, 2024 at 07:14:09PM +0530, Manivannan Sadhasivam wrote:
> > Currently, if global IRQ is supported by the platform, only the Link up
> > interrupt is enabled in the PARF_INT_ALL_MASK register. But on some Qcom
> > platforms like SM8250, and X1E80100, MSIs are getting masked due to this.
> > They require enabling the MSI interrupt bits in the register to unmask
> > (enable) the MSIs.
> 
> "global IRQ" is a very generic name.  If that's the official name, it
> should at least be capitalized, e.g., "Global IRQ", to show that it is
> a proper noun that refers to a specific IRQ.
> 

Sure.

> > Even though the MSI interrupt enable bits in PARF_INT_ALL_MASK are
> > described as 'diagnostic' interrupts in the internal documentation,
> > disabling them masks MSI on these platforms. Due to this,
> 
> > MSIs were not
> > reported to be received these platforms while supporting global IRQ.
> 
> I'm trying to parse "while supporting global IRQ."  We basically
> support global IRQ by installing qcom_pcie_global_irq_thread(), but of
> course the device doesn't see that, so I assume it would be more
> informative to say that MSIs are masked by some register setting.
> 

Hmm, this is what I mentioned in the above paragraph referencing
PARF_INT_ALL_MASK register. Is that not clear enough?

> The patch suggests that MSIs are masked internally unless
> PARF_INT_MSI_DEV_0_7 is set in PARF_INT_ALL_MASK.
> 
> Are you saying that prior to 4581403f6792, MSIs did work?  Does that
> mean PARF_INT_MSI_DEV_0_7 was set by a bootloader or something, so
> MSIs worked?  And then 4581403f6792 came along and implicitly cleared
> PARF_INT_MSI_DEV_0_7, so MSIs were then masked?
> 

Yeah. Those bits were enabled by default in hardware, but since they were
mentioned as 'diagnostic interrupts' in documentation, commit 4581403f6792
intentionally disabled them. But that results in MSIs getting masked in
*some* platforms.

- Mani

> > So enable the MSI interrupts along with the Link up interrupt in the
> > PARF_INT_ALL_MASK register if global IRQ is supported. This ensures that
> > the MSIs continue to work and also the driver is able to catch the Link
> > up interrupt for enumerating endpoint devices.
> > 
> > Fixes: 4581403f6792 ("PCI: qcom: Enumerate endpoints based on Link up event in 'global_irq' interrupt")
> > Reported-by: Konrad Dybcio <konradybcio@kernel.org>
> > Closes: https://lore.kernel.org/linux-pci/9a692c98-eb0a-4d86-b642-ea655981ff53@kernel.org/
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index ef44a82be058..2b33d03ed054 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -133,6 +133,7 @@
> >  
> >  /* PARF_INT_ALL_{STATUS/CLEAR/MASK} register fields */
> >  #define PARF_INT_ALL_LINK_UP			BIT(13)
> > +#define PARF_INT_MSI_DEV_0_7			GENMASK(30, 23)
> >  
> >  /* PARF_NO_SNOOP_OVERIDE register fields */
> >  #define WR_NO_SNOOP_OVERIDE_EN			BIT(1)
> > @@ -1716,7 +1717,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> >  			goto err_host_deinit;
> >  		}
> >  
> > -		writel_relaxed(PARF_INT_ALL_LINK_UP, pcie->parf + PARF_INT_ALL_MASK);
> > +		writel_relaxed(PARF_INT_ALL_LINK_UP | PARF_INT_MSI_DEV_0_7,
> > +			       pcie->parf + PARF_INT_ALL_MASK);
> >  	}
> >  
> >  	qcom_pcie_icc_opp_update(pcie);
> > -- 
> > 2.25.1
> > 

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] PCI: qcom: Enable MSI interrupts together with Link up if global IRQ is supported
  2024-10-01  4:20   ` Manivannan Sadhasivam
@ 2024-10-01 21:19     ` Bjorn Helgaas
  2024-10-03  4:57       ` Manivannan Sadhasivam
  0 siblings, 1 reply; 7+ messages in thread
From: Bjorn Helgaas @ 2024-10-01 21:19 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: lpieralisi, kw, robh, bhelgaas, linux-pci, linux-arm-msm,
	linux-kernel, quic_qianyu, Konrad Dybcio

On Tue, Oct 01, 2024 at 09:50:55AM +0530, Manivannan Sadhasivam wrote:
> On Mon, Sep 30, 2024 at 12:11:01PM -0500, Bjorn Helgaas wrote:
> > On Mon, Sep 30, 2024 at 07:14:09PM +0530, Manivannan Sadhasivam wrote:
> > > Currently, if global IRQ is supported by the platform, only the Link up
> > > interrupt is enabled in the PARF_INT_ALL_MASK register. But on some Qcom
> > > platforms like SM8250, and X1E80100, MSIs are getting masked due to this.
> > > They require enabling the MSI interrupt bits in the register to unmask
> > > (enable) the MSIs.
> > 
> > "global IRQ" is a very generic name.  If that's the official name, it
> > should at least be capitalized, e.g., "Global IRQ", to show that it is
> > a proper noun that refers to a specific IRQ.
> 
> Sure.
> 
> > > Even though the MSI interrupt enable bits in PARF_INT_ALL_MASK are
> > > described as 'diagnostic' interrupts in the internal documentation,
> > > disabling them masks MSI on these platforms. Due to this,
> > 
> > > MSIs were not
> > > reported to be received these platforms while supporting global IRQ.
> > 
> > I'm trying to parse "while supporting global IRQ."  We basically
> > support global IRQ by installing qcom_pcie_global_irq_thread(), but of
> > course the device doesn't see that, so I assume it would be more
> > informative to say that MSIs are masked by some register setting.
> 
> Hmm, this is what I mentioned in the above paragraph referencing
> PARF_INT_ALL_MASK register. Is that not clear enough?

It requires the knowledge that the MSI enable bits are set by
hardware, cleared by 4581403f6792, and set again here.  This will be
more accessible to non-qcom experts if that information is included
here.

> > The patch suggests that MSIs are masked internally unless
> > PARF_INT_MSI_DEV_0_7 is set in PARF_INT_ALL_MASK.
> > 
> > Are you saying that prior to 4581403f6792, MSIs did work?  Does that
> > mean PARF_INT_MSI_DEV_0_7 was set by a bootloader or something, so
> > MSIs worked?  And then 4581403f6792 came along and implicitly cleared
> > PARF_INT_MSI_DEV_0_7, so MSIs were then masked?
> 
> Yeah. Those bits were enabled by default in hardware, but since they were
> mentioned as 'diagnostic interrupts' in documentation, commit 4581403f6792
> intentionally disabled them. But that results in MSIs getting masked in
> *some* platforms.

Apparently the "*some* platforms" part is more qcom-expert knowledge?
There are other qcom platforms where MSIs are not disabled by
4581403f6792?  Information about which platforms are which also sounds
useful for future maintenance.

> > > So enable the MSI interrupts along with the Link up interrupt in the
> > > PARF_INT_ALL_MASK register if global IRQ is supported. This ensures that
> > > the MSIs continue to work and also the driver is able to catch the Link
> > > up interrupt for enumerating endpoint devices.
> > > 
> > > Fixes: 4581403f6792 ("PCI: qcom: Enumerate endpoints based on Link up event in 'global_irq' interrupt")
> > > Reported-by: Konrad Dybcio <konradybcio@kernel.org>
> > > Closes: https://lore.kernel.org/linux-pci/9a692c98-eb0a-4d86-b642-ea655981ff53@kernel.org/
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> > >  drivers/pci/controller/dwc/pcie-qcom.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > index ef44a82be058..2b33d03ed054 100644
> > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > @@ -133,6 +133,7 @@
> > >  
> > >  /* PARF_INT_ALL_{STATUS/CLEAR/MASK} register fields */
> > >  #define PARF_INT_ALL_LINK_UP			BIT(13)
> > > +#define PARF_INT_MSI_DEV_0_7			GENMASK(30, 23)
> > >  
> > >  /* PARF_NO_SNOOP_OVERIDE register fields */
> > >  #define WR_NO_SNOOP_OVERIDE_EN			BIT(1)
> > > @@ -1716,7 +1717,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> > >  			goto err_host_deinit;
> > >  		}
> > >  
> > > -		writel_relaxed(PARF_INT_ALL_LINK_UP, pcie->parf + PARF_INT_ALL_MASK);
> > > +		writel_relaxed(PARF_INT_ALL_LINK_UP | PARF_INT_MSI_DEV_0_7,
> > > +			       pcie->parf + PARF_INT_ALL_MASK);
> > >  	}
> > >  
> > >  	qcom_pcie_icc_opp_update(pcie);
> > > -- 
> > > 2.25.1
> > > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] PCI: qcom: Enable MSI interrupts together with Link up if global IRQ is supported
  2024-09-30 13:44 [PATCH] PCI: qcom: Enable MSI interrupts together with Link up if global IRQ is supported Manivannan Sadhasivam
  2024-09-30 17:11 ` Bjorn Helgaas
@ 2024-10-02  1:10 ` Qiang Yu
  2024-10-04  9:23 ` Konrad Dybcio
  2 siblings, 0 replies; 7+ messages in thread
From: Qiang Yu @ 2024-10-02  1:10 UTC (permalink / raw)
  To: Manivannan Sadhasivam, lpieralisi, kw
  Cc: robh, bhelgaas, linux-pci, linux-arm-msm, linux-kernel,
	Konrad Dybcio


On 9/30/2024 9:44 PM, Manivannan Sadhasivam wrote:
> Currently, if global IRQ is supported by the platform, only the Link up
> interrupt is enabled in the PARF_INT_ALL_MASK register. But on some Qcom
> platforms like SM8250, and X1E80100, MSIs are getting masked due to this.
> They require enabling the MSI interrupt bits in the register to unmask
> (enable) the MSIs.
>
> Even though the MSI interrupt enable bits in PARF_INT_ALL_MASK are
> described as 'diagnostic' interrupts in the internal documentation,
> disabling them masks MSI on these platforms. Due to this, MSIs were not
> reported to be received these platforms while supporting global IRQ.
>
> So enable the MSI interrupts along with the Link up interrupt in the
> PARF_INT_ALL_MASK register if global IRQ is supported. This ensures that
> the MSIs continue to work and also the driver is able to catch the Link
> up interrupt for enumerating endpoint devices.
>
> Fixes: 4581403f6792 ("PCI: qcom: Enumerate endpoints based on Link up event in 'global_irq' interrupt")
> Reported-by: Konrad Dybcio <konradybcio@kernel.org>
> Closes: https://lore.kernel.org/linux-pci/9a692c98-eb0a-4d86-b642-ea655981ff53@kernel.org/
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Reviewed-by: Qiang Yu <quic_qianyu@quicinc.com>

Thanks,
Qiang
> ---
>   drivers/pci/controller/dwc/pcie-qcom.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index ef44a82be058..2b33d03ed054 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -133,6 +133,7 @@
>   
>   /* PARF_INT_ALL_{STATUS/CLEAR/MASK} register fields */
>   #define PARF_INT_ALL_LINK_UP			BIT(13)
> +#define PARF_INT_MSI_DEV_0_7			GENMASK(30, 23)
>   
>   /* PARF_NO_SNOOP_OVERIDE register fields */
>   #define WR_NO_SNOOP_OVERIDE_EN			BIT(1)
> @@ -1716,7 +1717,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>   			goto err_host_deinit;
>   		}
>   
> -		writel_relaxed(PARF_INT_ALL_LINK_UP, pcie->parf + PARF_INT_ALL_MASK);
> +		writel_relaxed(PARF_INT_ALL_LINK_UP | PARF_INT_MSI_DEV_0_7,
> +			       pcie->parf + PARF_INT_ALL_MASK);
>   	}
>   
>   	qcom_pcie_icc_opp_update(pcie);

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] PCI: qcom: Enable MSI interrupts together with Link up if global IRQ is supported
  2024-10-01 21:19     ` Bjorn Helgaas
@ 2024-10-03  4:57       ` Manivannan Sadhasivam
  0 siblings, 0 replies; 7+ messages in thread
From: Manivannan Sadhasivam @ 2024-10-03  4:57 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: lpieralisi, kw, robh, bhelgaas, linux-pci, linux-arm-msm,
	linux-kernel, quic_qianyu, Konrad Dybcio

On Tue, Oct 01, 2024 at 04:19:57PM -0500, Bjorn Helgaas wrote:
> On Tue, Oct 01, 2024 at 09:50:55AM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Sep 30, 2024 at 12:11:01PM -0500, Bjorn Helgaas wrote:
> > > On Mon, Sep 30, 2024 at 07:14:09PM +0530, Manivannan Sadhasivam wrote:
> > > > Currently, if global IRQ is supported by the platform, only the Link up
> > > > interrupt is enabled in the PARF_INT_ALL_MASK register. But on some Qcom
> > > > platforms like SM8250, and X1E80100, MSIs are getting masked due to this.
> > > > They require enabling the MSI interrupt bits in the register to unmask
> > > > (enable) the MSIs.
> > > 
> > > "global IRQ" is a very generic name.  If that's the official name, it
> > > should at least be capitalized, e.g., "Global IRQ", to show that it is
> > > a proper noun that refers to a specific IRQ.
> > 
> > Sure.
> > 
> > > > Even though the MSI interrupt enable bits in PARF_INT_ALL_MASK are
> > > > described as 'diagnostic' interrupts in the internal documentation,
> > > > disabling them masks MSI on these platforms. Due to this,
> > > 
> > > > MSIs were not
> > > > reported to be received these platforms while supporting global IRQ.
> > > 
> > > I'm trying to parse "while supporting global IRQ."  We basically
> > > support global IRQ by installing qcom_pcie_global_irq_thread(), but of
> > > course the device doesn't see that, so I assume it would be more
> > > informative to say that MSIs are masked by some register setting.
> > 
> > Hmm, this is what I mentioned in the above paragraph referencing
> > PARF_INT_ALL_MASK register. Is that not clear enough?
> 
> It requires the knowledge that the MSI enable bits are set by
> hardware, cleared by 4581403f6792, and set again here.  This will be
> more accessible to non-qcom experts if that information is included
> here.
> 

Okay.

> > > The patch suggests that MSIs are masked internally unless
> > > PARF_INT_MSI_DEV_0_7 is set in PARF_INT_ALL_MASK.
> > > 
> > > Are you saying that prior to 4581403f6792, MSIs did work?  Does that
> > > mean PARF_INT_MSI_DEV_0_7 was set by a bootloader or something, so
> > > MSIs worked?  And then 4581403f6792 came along and implicitly cleared
> > > PARF_INT_MSI_DEV_0_7, so MSIs were then masked?
> > 
> > Yeah. Those bits were enabled by default in hardware, but since they were
> > mentioned as 'diagnostic interrupts' in documentation, commit 4581403f6792
> > intentionally disabled them. But that results in MSIs getting masked in
> > *some* platforms.
> 
> Apparently the "*some* platforms" part is more qcom-expert knowledge?

I already mentioned those platforms in the commit message 'SM8250 and X1E80100'.

> There are other qcom platforms where MSIs are not disabled by
> 4581403f6792?  Information about which platforms are which also sounds
> useful for future maintenance.
> 

Yeah, SM8450 is the one which I know so far. I will mention it explicitly.

- Mani

> > > > So enable the MSI interrupts along with the Link up interrupt in the
> > > > PARF_INT_ALL_MASK register if global IRQ is supported. This ensures that
> > > > the MSIs continue to work and also the driver is able to catch the Link
> > > > up interrupt for enumerating endpoint devices.
> > > > 
> > > > Fixes: 4581403f6792 ("PCI: qcom: Enumerate endpoints based on Link up event in 'global_irq' interrupt")
> > > > Reported-by: Konrad Dybcio <konradybcio@kernel.org>
> > > > Closes: https://lore.kernel.org/linux-pci/9a692c98-eb0a-4d86-b642-ea655981ff53@kernel.org/
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > ---
> > > >  drivers/pci/controller/dwc/pcie-qcom.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > index ef44a82be058..2b33d03ed054 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > @@ -133,6 +133,7 @@
> > > >  
> > > >  /* PARF_INT_ALL_{STATUS/CLEAR/MASK} register fields */
> > > >  #define PARF_INT_ALL_LINK_UP			BIT(13)
> > > > +#define PARF_INT_MSI_DEV_0_7			GENMASK(30, 23)
> > > >  
> > > >  /* PARF_NO_SNOOP_OVERIDE register fields */
> > > >  #define WR_NO_SNOOP_OVERIDE_EN			BIT(1)
> > > > @@ -1716,7 +1717,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> > > >  			goto err_host_deinit;
> > > >  		}
> > > >  
> > > > -		writel_relaxed(PARF_INT_ALL_LINK_UP, pcie->parf + PARF_INT_ALL_MASK);
> > > > +		writel_relaxed(PARF_INT_ALL_LINK_UP | PARF_INT_MSI_DEV_0_7,
> > > > +			       pcie->parf + PARF_INT_ALL_MASK);
> > > >  	}
> > > >  
> > > >  	qcom_pcie_icc_opp_update(pcie);
> > > > -- 
> > > > 2.25.1
> > > > 
> > 
> > -- 
> > மணிவண்ணன் சதாசிவம்

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] PCI: qcom: Enable MSI interrupts together with Link up if global IRQ is supported
  2024-09-30 13:44 [PATCH] PCI: qcom: Enable MSI interrupts together with Link up if global IRQ is supported Manivannan Sadhasivam
  2024-09-30 17:11 ` Bjorn Helgaas
  2024-10-02  1:10 ` Qiang Yu
@ 2024-10-04  9:23 ` Konrad Dybcio
  2 siblings, 0 replies; 7+ messages in thread
From: Konrad Dybcio @ 2024-10-04  9:23 UTC (permalink / raw)
  To: Manivannan Sadhasivam, lpieralisi, kw
  Cc: robh, bhelgaas, linux-pci, linux-arm-msm, linux-kernel,
	quic_qianyu, Konrad Dybcio

On 30.09.2024 3:44 PM, Manivannan Sadhasivam wrote:
> Currently, if global IRQ is supported by the platform, only the Link up
> interrupt is enabled in the PARF_INT_ALL_MASK register. But on some Qcom
> platforms like SM8250, and X1E80100, MSIs are getting masked due to this.
> They require enabling the MSI interrupt bits in the register to unmask
> (enable) the MSIs.
> 
> Even though the MSI interrupt enable bits in PARF_INT_ALL_MASK are
> described as 'diagnostic' interrupts in the internal documentation,
> disabling them masks MSI on these platforms. Due to this, MSIs were not
> reported to be received these platforms while supporting global IRQ.
> 
> So enable the MSI interrupts along with the Link up interrupt in the
> PARF_INT_ALL_MASK register if global IRQ is supported. This ensures that
> the MSIs continue to work and also the driver is able to catch the Link
> up interrupt for enumerating endpoint devices.
> 
> Fixes: 4581403f6792 ("PCI: qcom: Enumerate endpoints based on Link up event in 'global_irq' interrupt")
> Reported-by: Konrad Dybcio <konradybcio@kernel.org>
> Closes: https://lore.kernel.org/linux-pci/9a692c98-eb0a-4d86-b642-ea655981ff53@kernel.org/
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---

Tested-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> # SL7

Konrad

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2024-10-04  9:23 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-30 13:44 [PATCH] PCI: qcom: Enable MSI interrupts together with Link up if global IRQ is supported Manivannan Sadhasivam
2024-09-30 17:11 ` Bjorn Helgaas
2024-10-01  4:20   ` Manivannan Sadhasivam
2024-10-01 21:19     ` Bjorn Helgaas
2024-10-03  4:57       ` Manivannan Sadhasivam
2024-10-02  1:10 ` Qiang Yu
2024-10-04  9:23 ` Konrad Dybcio

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox