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

PATCH Applied Jisheng Zhang 3 participants 8 messages Dec 26, 2021 – Mar 2, 2022

Subsystems (3)

PCI PCI native host bridge and endpoint drivers PCI driver for synopsys DesignWare

lore

Share & tools

lore

Jisheng Zhang

author

2021-12-26 07:47 UTC #0 lore raw

PATCH 1/1 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 <[email protected]>

---
 drivers/pci/controller/dwc/pcie-designware-host.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drivers/pci/controller/dwc/pcie-designware-host.c +6 -1

index f4755f3a03be..2fa86f32d964 100644
@@ -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]);

Signature · 1 line

2.34.1

Hongxing Zhu

2022-01-10 03:21 UTC #1 lore raw

Quoted 17 lines

-----Original Message-----
From: Jisheng Zhang <[email protected]>
Sent: Sunday, December 26, 2021 3:40 PM
To: Jingoo Han <[email protected]>; Gustavo Pimentel <[email protected]>; Lorenzo Pieralisi <[email protected]>; Rob Herring <[email protected]>; Krzysztof Wilczyński <[email protected]>; Bjorn Helgaas <[email protected]>
Cc: [email protected]; [email protected]
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.

Quoted 5 lines

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 <[email protected]>

[Richard Zhu] Tested-by: Richard Zhu <[email protected]> Thanks.

Best Regards
Richard Zhu

Quoted 34 lines

---
 drivers/pci/controller/dwc/pcie-designware-host.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drivers/pci/controller/dwc/pcie-designware-host.c +6 -0

index f4755f3a03be..2fa86f32d964 100644
@@ -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");

drivers/pci/controller/dwc/pcie-designware-host.c +0 -2

 				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

Jisheng Zhang

author

2022-01-23 12:10 UTC #2 lore raw

In reply to Jisheng Zhang

Quoted 11 lines · 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 <[email protected]>

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

Quoted 32 lines

---
 drivers/pci/controller/dwc/pcie-designware-host.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drivers/pci/controller/dwc/pcie-designware-host.c +6 -1

index f4755f3a03be..2fa86f32d964 100644
@@ -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]);

Signature · 1 line

2.34.1

Lorenzo Pieralisi

2022-02-23 11:46 UTC #3 lore raw

Quoted 13 lines · Jisheng Zhang

Quoted 11 lines · 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 <[email protected]>

Quoted 8 lines

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

Quoted 35 lines

Thanks

Quoted 32 lines

---
 drivers/pci/controller/dwc/pcie-designware-host.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drivers/pci/controller/dwc/pcie-designware-host.c +6 -1

index f4755f3a03be..2fa86f32d964 100644
@@ -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]);

Signature · 1 line

2.34.1

Jisheng Zhang

author

2022-02-23 16:14 UTC #4 lore raw

Quoted 19 lines · Lorenzo Pieralisi

Quoted 18 lines · Jisheng Zhang
Quoted 11 lines · 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 <[email protected]>
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?

Quoted 7 lines

Quoted 3 lines
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.

Quoted 3 lines

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.

Quoted 3 lines

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

Lorenzo Pieralisi

2022-02-24 14:08 UTC #5 lore raw

Quoted 42 lines · Jisheng Zhang

Quoted 19 lines · Lorenzo Pieralisi
Quoted 18 lines · Jisheng Zhang
Quoted 11 lines · 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 <[email protected]>
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?
Quoted 7 lines
Quoted 3 lines
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.
Quoted 3 lines
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.

Quoted 7 lines

Quoted 3 lines
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

Jisheng Zhang

author

2022-02-26 09:23 UTC #6 lore raw

Quoted 47 lines · Lorenzo Pieralisi

Quoted 46 lines · Jisheng Zhang
Quoted 19 lines · Lorenzo Pieralisi
Quoted 18 lines · Jisheng Zhang
Quoted 11 lines · 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 <[email protected]>
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?
Quoted 7 lines
Quoted 3 lines
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.
Quoted 3 lines
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.
Quoted 3 lines
Maybe we need to write suspend/resume hooks for the dwc core instead of moving code around to fix these bugs ?

Quoted 6 lines

Quoted 3 lines
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.

Quoted 1 line

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

the irq_mask[] array.

Lorenzo Pieralisi

Applied

2022-03-02 10:37 UTC #7 lore raw

In reply to Jisheng Zhang

Quoted 11 lines · 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.
[...]

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