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@xxxxxxxxxx>
> > 
> > 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