RE: [PATCH v2] PCI: imx6: Save and restore MSI control of RC in suspend and resume

[Hongxing Zhu <hongxing.zhu@nxp.com>]

> -----Original Message-----
> From: Lorenzo Pieralisi <lpieralisi@kernel.org>
> Sent: 2022年12月30日 23:06
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; l.stach@pengutronix.de;
> bhelgaas@google.com
> Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI control of RC in
> suspend and resume
> 
> On Thu, Dec 08, 2022 at 02:05:34PM +0800, Richard Zhu wrote:
> > The MSI Enable bit controls delivery of MSI interrupts from components
> > below the Root Port. This bit might lost during the suspend, should be
> > re-stored during resume.
> >
> > Save the MSI control during suspend, and restore it in resume.
> 
> I believe that what Lucas and Bjorn asked on v1 is still not answered.
> 
> The root port is a PCI device, why do we need to save and restore the MSI cap
> on top of what PCI core already does ? The RP should be enumerated as a PCI
> device and therefore I expect the MSI cap to be saved/restored in the
> suspend/resume execution.
> 
> I don't think there is anything iMX6 specific in this.
Hi Lorenzo:
Thanks for your comments.
Sorry to reply late, since I got a high fever in the past days.

Based on i.MX6QP SABRESD board and XHCI PCIe2USB3.0 device, the MSI cap
 save/restore of PCI core is not executed(dev->msi_enabled is zero)
 during my suspend/resume tests.

It seems that some device might shutdown msi when do the suspend operations.
> 
> Would you mind investigating it please ?
Sure, I did further investigation on i.MX6QP platform.
The MSI_EN bit of RC MSI capability would be cleared to zero, when
 PCIE_RESET(BIT29 of IOMUXC_GPR1) is toggled (assertion 1b'1,
 then de-assertion 1b'0).

Verification steps:
MSI_EN of RC is set to 1b'1 when system is boot up.
 ./memtool 1ffc050 1
0x01FFC050:  01017005

Toggle PCIe reset of i.MX6QP.
root@imx6qpdlsolox:~# ./memtool 20e0004=68691005
Writing 32-bit value 0x68691005 to address 0x020E0004
root@imx6qpdlsolox:~# ./memtool 20e0004=48691005
Writing 32-bit value 0x48691005 to address 0x020E0004

The MSI_EN bit of RC had been cleared to 1b'0.
./memtool 1ffc050 1
0x01FFC050:  01807005

This is why I used to reply to Bjorn the MSI_EN of RC is cleared when
 RESETs are toggled during the imx6_pcie_host_init() in
 imx6_pcie_resume_noirq() callback.

Best Regards
Richard Zhu
> 
> Lorenzo
> 
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> > Changes v1-->v2:
> > New create one save/restore function, used save the setting in suspend
> > and restore the configuration in resume.
> > v1
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> >
> hwork.kernel.org%2Fproject%2Flinux-pci%2Fpatch%2F1667289595-12440-1-g
> i
> >
> t-send-email-hongxing.zhu%40nxp.com%2F&data=05%7C01%7Chongxing.zhu
> %40n
> >
> xp.com%7C3aeb1d128f854dad1a5608daea77706d%7C686ea1d3bc2b4c6fa92
> cd99c5c
> >
> 301635%7C0%7C0%7C638080095954881374%7CUnknown%7CTWFpbGZsb3
> d8eyJWIjoiMC
> >
> 4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%
> 7C%7C%
> >
> 7C&sdata=V8yVvvpTKGoR1UyQP5HD2IdlSjJdznBeD12bdI67dEI%3D&reserved=
> 0
> >
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 1dde5c579edc..aa3096890c3b 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -76,6 +76,7 @@ struct imx6_pcie {
> >  	struct clk		*pcie;
> >  	struct clk		*pcie_aux;
> >  	struct regmap		*iomuxc_gpr;
> > +	u16			msi_ctrl;
> >  	u32			controller_id;
> >  	struct reset_control	*pciephy_reset;
> >  	struct reset_control	*apps_reset;
> > @@ -1042,6 +1043,26 @@ static void imx6_pcie_pm_turnoff(struct
> imx6_pcie *imx6_pcie)
> >  	usleep_range(1000, 10000);
> >  }
> >
> > +static void imx6_pcie_msi_save_restore(struct imx6_pcie *imx6_pcie,
> > +bool save) {
> > +	u8 offset;
> > +	u16 val;
> > +	struct dw_pcie *pci = imx6_pcie->pci;
> > +
> > +	if (pci_msi_enabled()) {
> > +		offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
> > +		if (save) {
> > +			val = dw_pcie_readw_dbi(pci, offset + PCI_MSI_FLAGS);
> > +			imx6_pcie->msi_ctrl = val;
> > +		} else {
> > +			dw_pcie_dbi_ro_wr_en(pci);
> > +			val = imx6_pcie->msi_ctrl;
> > +			dw_pcie_writew_dbi(pci, offset + PCI_MSI_FLAGS, val);
> > +			dw_pcie_dbi_ro_wr_dis(pci);
> > +		}
> > +	}
> > +}
> > +
> >  static int imx6_pcie_suspend_noirq(struct device *dev)  {
> >  	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev); @@ -1050,6
> > +1071,7 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
> >  	if (!(imx6_pcie->drvdata->flags &
> IMX6_PCIE_FLAG_SUPPORTS_SUSPEND))
> >  		return 0;
> >
> > +	imx6_pcie_msi_save_restore(imx6_pcie, true);
> >  	imx6_pcie_pm_turnoff(imx6_pcie);
> >  	imx6_pcie_stop_link(imx6_pcie->pci);
> >  	imx6_pcie_host_exit(pp);
> > @@ -1069,6 +1091,7 @@ static int imx6_pcie_resume_noirq(struct device
> *dev)
> >  	ret = imx6_pcie_host_init(pp);
> >  	if (ret)
> >  		return ret;
> > +	imx6_pcie_msi_save_restore(imx6_pcie, false);
> >  	dw_pcie_setup_rc(pp);
> >
> >  	if (imx6_pcie->link_is_up)
> > --
> > 2.25.1
> >