[PATCH] PCI: dwc: Fix integrated MSI Receiver mask reg setting during resume

[PATCH] PCI: dwc: Fix integrated MSI Receiver mask reg setting during resume

[Jisheng Zhang]

If the host which makes use of the IP's integrated MSI Receiver losts
power during suspend, we call dw_pcie_setup_rc() to reinit the RC. But
dw_pcie_setup_rc() always set the pp->irq_mask[ctrl] as ~0, so the mask
register is always set as 0xffffffff incorrectly, thus the MSI can't
work after resume.

Fix this issue by moving pp->irq_mask[ctrl] initialization to
dw_pcie_host_init(), so we can correctly set the mask reg during both
boot and resume.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index f4755f3a03be..2fa86f32d964 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -362,6 +362,12 @@ int dw_pcie_host_init(struct pcie_port *pp)
 			if (ret < 0)
 				return ret;
 		} else if (pp->has_msi_ctrl) {
+			u32 ctrl, num_ctrls;
+
+			num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL;
+			for (ctrl = 0; ctrl < num_ctrls; ctrl++)
+				pp->irq_mask[ctrl] = ~0;
+
 			if (!pp->msi_irq) {
 				pp->msi_irq = platform_get_irq_byname_optional(pdev, "msi");
 				if (pp->msi_irq < 0) {
@@ -541,7 +547,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 
 		/* Initialize IRQ Status array */
 		for (ctrl = 0; ctrl < num_ctrls; ctrl++) {
-			pp->irq_mask[ctrl] = ~0;
 			dw_pcie_writel_dbi(pci, PCIE_MSI_INTR0_MASK +
 					    (ctrl * MSI_REG_CTRL_BLOCK_SIZE),
 					    pp->irq_mask[ctrl]);
-- 
2.34.1

RE: [PATCH] PCI: dwc: Fix integrated MSI Receiver mask reg setting during resume

[Hongxing Zhu]

> -----Original Message-----
> From: Jisheng Zhang <jszhang@kernel.org>
> Sent: Sunday, December 26, 2021 3:40 PM
> To: Jingoo Han <jingoohan1@gmail.com>; Gustavo Pimentel
> <gustavo.pimentel@synopsys.com>; Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com>; Rob Herring <robh@kernel.org>; Krzysztof
> Wilczyński <kw@linux.com>; Bjorn Helgaas <bhelgaas@google.com>
> Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH] PCI: dwc: Fix integrated MSI Receiver mask reg setting
> during resume
> 
> If the host which makes use of the IP's integrated MSI Receiver losts
> power during suspend, we call dw_pcie_setup_rc() to reinit the RC. But
> dw_pcie_setup_rc() always set the pp->irq_mask[ctrl] as ~0, so the mask
> register is always set as 0xffffffff incorrectly, thus the MSI can't work after
> resume.
> 
> Fix this issue by moving pp->irq_mask[ctrl] initialization to
> dw_pcie_host_init(), so we can correctly set the mask reg during both
> boot and resume.
> 
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
[Richard Zhu] Tested-by: Richard Zhu <hongxing.zhu@nxp.com>
Thanks.

Best Regards
Richard Zhu

> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
> b/drivers/pci/controller/dwc/pcie-designware-host.c
> index f4755f3a03be..2fa86f32d964 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -362,6 +362,12 @@ int dw_pcie_host_init(struct pcie_port *pp)
>  			if (ret < 0)
>  				return ret;
>  		} else if (pp->has_msi_ctrl) {
> +			u32 ctrl, num_ctrls;
> +
> +			num_ctrls = pp->num_vectors /
> MAX_MSI_IRQS_PER_CTRL;
> +			for (ctrl = 0; ctrl < num_ctrls; ctrl++)
> +				pp->irq_mask[ctrl] = ~0;
> +
>  			if (!pp->msi_irq) {
>  				pp->msi_irq =
> platform_get_irq_byname_optional(pdev, "msi");
>  				if (pp->msi_irq < 0) {
> @@ -541,7 +547,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
> 
>  		/* Initialize IRQ Status array */
>  		for (ctrl = 0; ctrl < num_ctrls; ctrl++) {
> -			pp->irq_mask[ctrl] = ~0;
>  			dw_pcie_writel_dbi(pci, PCIE_MSI_INTR0_MASK +
>  					    (ctrl * MSI_REG_CTRL_BLOCK_SIZE),
>  					    pp->irq_mask[ctrl]);
> --
> 2.34.1

Re: [PATCH] PCI: dwc: Fix integrated MSI Receiver mask reg setting during resume

[Jisheng Zhang]

On Sun, Dec 26, 2021 at 03:40:19PM +0800, Jisheng Zhang wrote:
> If the host which makes use of the IP's integrated MSI Receiver losts
> power during suspend, we call dw_pcie_setup_rc() to reinit the RC. But
> dw_pcie_setup_rc() always set the pp->irq_mask[ctrl] as ~0, so the mask
> register is always set as 0xffffffff incorrectly, thus the MSI can't
> work after resume.
> 
> Fix this issue by moving pp->irq_mask[ctrl] initialization to
> dw_pcie_host_init(), so we can correctly set the mask reg during both
> boot and resume.
> 
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>

Hi all,

This patch can still be applied to the latest linus tree. Do you want
me to rebase and send out a new version?

Without this patch, dwc host MSI interrupt(if use the IP's integrated
MSI receiver) can't work after resume. Could it be picked up as a fix
for v5.17?

Thanks

> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index f4755f3a03be..2fa86f32d964 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -362,6 +362,12 @@ int dw_pcie_host_init(struct pcie_port *pp)
>  			if (ret < 0)
>  				return ret;
>  		} else if (pp->has_msi_ctrl) {
> +			u32 ctrl, num_ctrls;
> +
> +			num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL;
> +			for (ctrl = 0; ctrl < num_ctrls; ctrl++)
> +				pp->irq_mask[ctrl] = ~0;
> +
>  			if (!pp->msi_irq) {
>  				pp->msi_irq = platform_get_irq_byname_optional(pdev, "msi");
>  				if (pp->msi_irq < 0) {
> @@ -541,7 +547,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>  
>  		/* Initialize IRQ Status array */
>  		for (ctrl = 0; ctrl < num_ctrls; ctrl++) {
> -			pp->irq_mask[ctrl] = ~0;
>  			dw_pcie_writel_dbi(pci, PCIE_MSI_INTR0_MASK +
>  					    (ctrl * MSI_REG_CTRL_BLOCK_SIZE),
>  					    pp->irq_mask[ctrl]);
> -- 
> 2.34.1
> 

Re: [PATCH] PCI: dwc: Fix integrated MSI Receiver mask reg setting during resume

[Lorenzo Pieralisi]

On Sun, Jan 23, 2022 at 08:02:42PM +0800, Jisheng Zhang wrote:
> On Sun, Dec 26, 2021 at 03:40:19PM +0800, Jisheng Zhang wrote:
> > If the host which makes use of the IP's integrated MSI Receiver losts
> > power during suspend, we call dw_pcie_setup_rc() to reinit the RC. But
> > dw_pcie_setup_rc() always set the pp->irq_mask[ctrl] as ~0, so the mask
> > register is always set as 0xffffffff incorrectly, thus the MSI can't
> > work after resume.
> > 
> > Fix this issue by moving pp->irq_mask[ctrl] initialization to
> > dw_pcie_host_init(), so we can correctly set the mask reg during both
> > boot and resume.
> > 
> > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> 
> Hi all,
> 
> This patch can still be applied to the latest linus tree. Do you want
> me to rebase and send out a new version?
> 
> Without this patch, dwc host MSI interrupt(if use the IP's integrated
> MSI receiver) can't work after resume. Could it be picked up as a fix
> for v5.17?

The tricky bit with this patch is that it is not clear what piece of
logic is lost on power down and what not. IIUC MSI interrupt controller
logic is kept so it does not need to be saved/restored (but in
dw_pcie_setup_rc() we overwrite PCIE_MSI_INTR0_ENABLE even if it
is not needed on resume - actually, it can even be destructive).

Maybe we need to write suspend/resume hooks for the dwc core instead
of moving code around to fix these bugs ?

Lorenzo

> 
> Thanks
> 
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-host.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index f4755f3a03be..2fa86f32d964 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -362,6 +362,12 @@ int dw_pcie_host_init(struct pcie_port *pp)
> >  			if (ret < 0)
> >  				return ret;
> >  		} else if (pp->has_msi_ctrl) {
> > +			u32 ctrl, num_ctrls;
> > +
> > +			num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL;
> > +			for (ctrl = 0; ctrl < num_ctrls; ctrl++)
> > +				pp->irq_mask[ctrl] = ~0;
> > +
> >  			if (!pp->msi_irq) {
> >  				pp->msi_irq = platform_get_irq_byname_optional(pdev, "msi");
> >  				if (pp->msi_irq < 0) {
> > @@ -541,7 +547,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
> >  
> >  		/* Initialize IRQ Status array */
> >  		for (ctrl = 0; ctrl < num_ctrls; ctrl++) {
> > -			pp->irq_mask[ctrl] = ~0;
> >  			dw_pcie_writel_dbi(pci, PCIE_MSI_INTR0_MASK +
> >  					    (ctrl * MSI_REG_CTRL_BLOCK_SIZE),
> >  					    pp->irq_mask[ctrl]);
> > -- 
> > 2.34.1
> > 

Re: [PATCH] PCI: dwc: Fix integrated MSI Receiver mask reg setting during resume

[Jisheng Zhang]

On Wed, Feb 23, 2022 at 11:46:22AM +0000, Lorenzo Pieralisi wrote:
> On Sun, Jan 23, 2022 at 08:02:42PM +0800, Jisheng Zhang wrote:
> > On Sun, Dec 26, 2021 at 03:40:19PM +0800, Jisheng Zhang wrote:
> > > If the host which makes use of the IP's integrated MSI Receiver losts
> > > power during suspend, we call dw_pcie_setup_rc() to reinit the RC. But
> > > dw_pcie_setup_rc() always set the pp->irq_mask[ctrl] as ~0, so the mask
> > > register is always set as 0xffffffff incorrectly, thus the MSI can't
> > > work after resume.
> > > 
> > > Fix this issue by moving pp->irq_mask[ctrl] initialization to
> > > dw_pcie_host_init(), so we can correctly set the mask reg during both
> > > boot and resume.
> > > 
> > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > 
> > Hi all,
> > 
> > This patch can still be applied to the latest linus tree. Do you want
> > me to rebase and send out a new version?
> > 
> > Without this patch, dwc host MSI interrupt(if use the IP's integrated
> > MSI receiver) can't work after resume. Could it be picked up as a fix
> > for v5.17?
> 
> The tricky bit with this patch is that it is not clear what piece of
> logic is lost on power down and what not. IIUC MSI interrupt controller
> logic is kept so it does not need to be saved/restored (but in

You may mean the external MSI interrupt controller case, but here the
focus is the integrated MSI Receiver in the IP. Normally, after
suspending to ram, the dwc IP would lost power, so the integrated MSI
Receiver also lost power.

> dw_pcie_setup_rc() we overwrite PCIE_MSI_INTR0_ENABLE even if it
> is not needed on resume - actually, it can even be destructive).
> 

For the integrated MSI Receiver case, since the entire IP power is lost,
so the PCIE_MSI_INTR0_MASK|ENABLE setting is lost, we need to resume the
mask to the one before suspending. For PCIE_MSI_INTR0_ENABLE register(s),
since it's always 0xffffffff, so current code is fine.

> Maybe we need to write suspend/resume hooks for the dwc core instead
> of moving code around to fix these bugs ?
> 

Even with suspend/resume hooks, we still need to fix the
PCIE_MSI_INTR0_MASK wrong setting with always ~0. After the fix, msi works
so we don't need suspend/resume hooks any more.

Thanks

Re: [PATCH] PCI: dwc: Fix integrated MSI Receiver mask reg setting during resume

[Lorenzo Pieralisi]

On Thu, Feb 24, 2022 at 12:06:09AM +0800, Jisheng Zhang wrote:
> On Wed, Feb 23, 2022 at 11:46:22AM +0000, Lorenzo Pieralisi wrote:
> > On Sun, Jan 23, 2022 at 08:02:42PM +0800, Jisheng Zhang wrote:
> > > On Sun, Dec 26, 2021 at 03:40:19PM +0800, Jisheng Zhang wrote:
> > > > If the host which makes use of the IP's integrated MSI Receiver losts
> > > > power during suspend, we call dw_pcie_setup_rc() to reinit the RC. But
> > > > dw_pcie_setup_rc() always set the pp->irq_mask[ctrl] as ~0, so the mask
> > > > register is always set as 0xffffffff incorrectly, thus the MSI can't
> > > > work after resume.
> > > > 
> > > > Fix this issue by moving pp->irq_mask[ctrl] initialization to
> > > > dw_pcie_host_init(), so we can correctly set the mask reg during both
> > > > boot and resume.
> > > > 
> > > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > > 
> > > Hi all,
> > > 
> > > This patch can still be applied to the latest linus tree. Do you want
> > > me to rebase and send out a new version?
> > > 
> > > Without this patch, dwc host MSI interrupt(if use the IP's integrated
> > > MSI receiver) can't work after resume. Could it be picked up as a fix
> > > for v5.17?
> > 
> > The tricky bit with this patch is that it is not clear what piece of
> > logic is lost on power down and what not. IIUC MSI interrupt controller
> > logic is kept so it does not need to be saved/restored (but in
> 
> You may mean the external MSI interrupt controller case, but here the
> focus is the integrated MSI Receiver in the IP. Normally, after
> suspending to ram, the dwc IP would lost power, so the integrated MSI
> Receiver also lost power.
> 
> > dw_pcie_setup_rc() we overwrite PCIE_MSI_INTR0_ENABLE even if it
> > is not needed on resume - actually, it can even be destructive).
> > 
> 
> For the integrated MSI Receiver case, since the entire IP power is lost,
> so the PCIE_MSI_INTR0_MASK|ENABLE setting is lost, we need to resume the
> mask to the one before suspending. For PCIE_MSI_INTR0_ENABLE register(s),
> since it's always 0xffffffff, so current code is fine.
> 
> > Maybe we need to write suspend/resume hooks for the dwc core instead
> > of moving code around to fix these bugs ?
> > 
> 
> Even with suspend/resume hooks, we still need to fix the
> PCIE_MSI_INTR0_MASK wrong setting with always ~0. After the fix, msi works
> so we don't need suspend/resume hooks any more.

I don't understand. The fix removes code that is writing into
PCIE_MSI_INTR0_MASK (in dw_pcie_setup_rc()). Where that register
content is restored on power up to the correct value then ?

I assume those registers when the IP loses power are reset
to their reset value, so something does not add up.

Lorenzo

Re: [PATCH] PCI: dwc: Fix integrated MSI Receiver mask reg setting during resume

[Jisheng Zhang]

On Thu, Feb 24, 2022 at 02:08:47PM +0000, Lorenzo Pieralisi wrote:
> On Thu, Feb 24, 2022 at 12:06:09AM +0800, Jisheng Zhang wrote:
> > On Wed, Feb 23, 2022 at 11:46:22AM +0000, Lorenzo Pieralisi wrote:
> > > On Sun, Jan 23, 2022 at 08:02:42PM +0800, Jisheng Zhang wrote:
> > > > On Sun, Dec 26, 2021 at 03:40:19PM +0800, Jisheng Zhang wrote:
> > > > > If the host which makes use of the IP's integrated MSI Receiver losts
> > > > > power during suspend, we call dw_pcie_setup_rc() to reinit the RC. But
> > > > > dw_pcie_setup_rc() always set the pp->irq_mask[ctrl] as ~0, so the mask
> > > > > register is always set as 0xffffffff incorrectly, thus the MSI can't
> > > > > work after resume.
> > > > > 
> > > > > Fix this issue by moving pp->irq_mask[ctrl] initialization to
> > > > > dw_pcie_host_init(), so we can correctly set the mask reg during both
> > > > > boot and resume.
> > > > > 
> > > > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > > > 
> > > > Hi all,
> > > > 
> > > > This patch can still be applied to the latest linus tree. Do you want
> > > > me to rebase and send out a new version?
> > > > 
> > > > Without this patch, dwc host MSI interrupt(if use the IP's integrated
> > > > MSI receiver) can't work after resume. Could it be picked up as a fix
> > > > for v5.17?
> > > 
> > > The tricky bit with this patch is that it is not clear what piece of
> > > logic is lost on power down and what not. IIUC MSI interrupt controller
> > > logic is kept so it does not need to be saved/restored (but in
> > 
> > You may mean the external MSI interrupt controller case, but here the
> > focus is the integrated MSI Receiver in the IP. Normally, after
> > suspending to ram, the dwc IP would lost power, so the integrated MSI
> > Receiver also lost power.
> > 
> > > dw_pcie_setup_rc() we overwrite PCIE_MSI_INTR0_ENABLE even if it
> > > is not needed on resume - actually, it can even be destructive).
> > > 
> > 
> > For the integrated MSI Receiver case, since the entire IP power is lost,
> > so the PCIE_MSI_INTR0_MASK|ENABLE setting is lost, we need to resume the
> > mask to the one before suspending. For PCIE_MSI_INTR0_ENABLE register(s),
> > since it's always 0xffffffff, so current code is fine.
> > 
> > > Maybe we need to write suspend/resume hooks for the dwc core instead
> > > of moving code around to fix these bugs ?
> > > 
> > 
> > Even with suspend/resume hooks, we still need to fix the
> > PCIE_MSI_INTR0_MASK wrong setting with always ~0. After the fix, msi works
> > so we don't need suspend/resume hooks any more.
> 
> I don't understand. The fix removes code that is writing into
> PCIE_MSI_INTR0_MASK (in dw_pcie_setup_rc()). Where that register

No, the patch just moves the irq_mask[] array setting from
dw_pcie_setup_rc() to dw_pcie_host_init(), the code which writes
irq_mask[] array into PCIE_MSI_INTR0_MASK is still kept there.

> content is restored on power up to the correct value then ?

the irq_mask[] array.

Re: [PATCH] PCI: dwc: Fix integrated MSI Receiver mask reg setting during resume

[Lorenzo Pieralisi]

On Sun, 26 Dec 2021 15:40:19 +0800, Jisheng Zhang wrote:
> If the host which makes use of the IP's integrated MSI Receiver losts
> power during suspend, we call dw_pcie_setup_rc() to reinit the RC. But
> dw_pcie_setup_rc() always set the pp->irq_mask[ctrl] as ~0, so the mask
> register is always set as 0xffffffff incorrectly, thus the MSI can't
> work after resume.
> 
> Fix this issue by moving pp->irq_mask[ctrl] initialization to
> dw_pcie_host_init(), so we can correctly set the mask reg during both
> boot and resume.
> 
> [...]

Applied to pci/dwc, thanks!

[1/1] PCI: dwc: Fix integrated MSI Receiver mask reg setting during resume
      https://git.kernel.org/lpieralisi/pci/c/84edd0090e

Thanks,
Lorenzo