| Message ID | 20211226074019.2556-1-jszhang@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: dwc: Fix integrated MSI Receiver mask reg setting during resume | expand |
> -----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
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 >
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 > >
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
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
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.
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]);
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(-)