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

Commit Message

Hongxing Zhu Dec. 8, 2022, 6:05 a.m. UTC

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.

Signed-off-by: Richard Zhu <[e-mail]>
---
Changes v1-->v2:
New create one save/restore function, used save the setting in suspend and
restore the configuration in resume.
v1 https://patchwork.kernel.org/project/linux-pci/patch/[e-mail]/

---
 drivers/pci/controller/dwc/pci-imx6.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

Comments

Lorenzo Pieralisi Dec. 30, 2022, 3:06 p.m. UTC | #1

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.

Would you mind investigating it please ?

Lorenzo

> Signed-off-by: Richard Zhu <[e-mail]>
> ---
> Changes v1-->v2:
> New create one save/restore function, used save the setting in suspend and
> restore the configuration in resume.
> v1 https://patchwork.kernel.org/project/linux-pci/patch/[e-mail]/
> 
> ---
>  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
>

Hongxing Zhu Jan. 9, 2023, 2:08 a.m. UTC | #2

> -----Original Message-----
> From: Lorenzo Pieralisi <[e-mail]>
> Sent: 2022年12月30日 23:06
> To: Hongxing Zhu <[e-mail]>; [e-mail];
> [e-mail]
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; dl-linux-imx
> <[e-mail]>
> 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 <[e-mail]>
> > ---
> > 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
> >

Lorenzo Pieralisi March 10, 2023, 4:13 p.m. UTC | #3

On Mon, Jan 09, 2023 at 02:08:06AM +0000, Hongxing Zhu wrote:
> > -----Original Message-----
> > From: Lorenzo Pieralisi <[e-mail]>
> > Sent: 2022年12月30日 23:06
> > To: Hongxing Zhu <[e-mail]>; [e-mail];
> > [e-mail]
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; dl-linux-imx
> > <[e-mail]>
> > 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.

I still do not understand. The register you are saving/restoring in the
RC is not the root port Message control field in the root port MSI 
capability, it is a separate register that controls the root complex
MSI forwarding, is that correct ?

The root port MSI capability does not control the root complex
forwarding of MSIs TLPs.

So the bits you are saving and restoring IIUC should be MMIO space in
the root complex, dressed as an MSI capability, that has nothing to
do with the root port MSI capability.

Is that correct ?

Thanks,
Lorenzo
> 
> 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 <[e-mail]>
> > > ---
> > > 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
> > >
> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hongxing Zhu March 13, 2023, 2:50 a.m. UTC | #4

> -----Original Message-----
> From: Lorenzo Pieralisi <[e-mail]>
> Sent: 2023年3月11日 0:14
> To: Hongxing Zhu <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; dl-linux-imx
> <[e-mail]>
> Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI control of RC in
> suspend and resume
> 
> On Mon, Jan 09, 2023 at 02:08:06AM +0000, Hongxing Zhu wrote:
> > > -----Original Message-----
> > > From: Lorenzo Pieralisi <[e-mail]>
> > > Sent: 2022年12月30日 23:06
> > > To: Hongxing Zhu <[e-mail]>; [e-mail];
> > > [e-mail]
> > > Cc: [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; dl-linux-imx
> > > <[e-mail]>
> > > 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.
> 
> I still do not understand. The register you are saving/restoring in the RC is not
> the root port Message control field in the root port MSI capability, it is a
> separate register that controls the root complex MSI forwarding, is that
> correct ?
> 
> The root port MSI capability does not control the root complex forwarding of
> MSIs TLPs.
> 
> So the bits you are saving and restoring IIUC should be MMIO space in the
> root complex, dressed as an MSI capability, that has nothing to do with the
> root port MSI capability.
> 
> Is that correct ?
Hi Lorenzo:
Thanks for your reply.
It's not a separate register.
The bit I manipulated is the MSI Enable bit of the Message Control Register
 for MSI (Offset 02h) contained in the MSI-capability of Root Complex.
In addition, on i.MX6, the MSI Enable bit controls delivery of MSI
 interrupts from components below the Root Port.
So, set MSI Enable in imx6q-pcie to let the MSI from downstream
 components works.

Best Regards
Richard Zhu

> 
> Thanks,
> Lorenzo
> >
> > 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 <[e-mail]>
> > > > ---
> > > > 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%2F
> > > >
> patc%2F&data=05%7C01%7Chongxing.zhu%40nxp.com%7C24971d8de9b54b
> 0b10
> > > >
> ad08db2182774d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6
> 38140
> > > >
> 616456052078%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> QIjoiV
> > > >
> 2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=vE
> tRxL
> > > > BVi5lYmpwTNZfafMms3263LZXodneLChjEaOM%3D&reserved=0
> > > >
> > >
> 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%7C686ea1d3bc2b4c6fa9
> 2
> > > 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
> > > >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > [e-mail]
> > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists
> > .infradead.org%2Fmailman%2Flistinfo%2Flinux-arm-kernel&data=05%7C01
> %7C
> >
> hongxing.zhu%40nxp.com%7C24971d8de9b54b0b10ad08db2182774d%7C68
> 6ea1d3bc
> >
> 2b4c6fa92cd99c5c301635%7C0%7C0%7C638140616456052078%7CUnknow
> n%7CTWFpbG
> >
> Zsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%
> >
> 3D%7C3000%7C%7C%7C&sdata=JR6JKVNQGiZtxYbdD%2B4P9u7qgSMVGqQ
> PdN1CpN%2BrV
> > Ik%3D&reserved=0

Bjorn Helgaas March 13, 2023, 5:49 p.m. UTC | #5

On Mon, Mar 13, 2023 at 02:50:31AM +0000, Hongxing Zhu wrote:
> > -----Original Message-----
> > From: Lorenzo Pieralisi <[e-mail]>
> > Sent: 2023年3月11日 0:14
> > To: Hongxing Zhu <[e-mail]>
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; dl-linux-imx
> > <[e-mail]>
> > Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI control of RC in
> > suspend and resume
> > 
> > On Mon, Jan 09, 2023 at 02:08:06AM +0000, Hongxing Zhu wrote:
> > > > -----Original Message-----
> > > > From: Lorenzo Pieralisi <[e-mail]>
> > > > Sent: 2022年12月30日 23:06
> > > > To: Hongxing Zhu <[e-mail]>; [e-mail];
> > > > [e-mail]
> > > > Cc: [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail]; dl-linux-imx
> > > > <[e-mail]>
> > > > 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.
> > 
> > I still do not understand. The register you are saving/restoring in the RC is not
> > the root port Message control field in the root port MSI capability, it is a
> > separate register that controls the root complex MSI forwarding, is that
> > correct ?
> > 
> > The root port MSI capability does not control the root complex forwarding of
> > MSIs TLPs.
> > 
> > So the bits you are saving and restoring IIUC should be MMIO space in the
> > root complex, dressed as an MSI capability, that has nothing to do with the
> > root port MSI capability.
> > 
> > Is that correct ?
>
> It's not a separate register.
>
> The bit I manipulated is the MSI Enable bit of the Message Control
> Register for MSI (Offset 02h) contained in the MSI-capability of
> Root Complex.
>
> In addition, on i.MX6, the MSI Enable bit controls delivery of MSI
> interrupts from components below the Root Port.
>
> So, set MSI Enable in imx6q-pcie to let the MSI from downstream
> components works.

My confusion is about this "MSI Capability" found by
"dw_pcie_find_capability(pci, PCI_CAP_ID_MSI)" in your patch.

The i.MX6 manual might refer to that as an "MSI Capability" but as far
as I know, the PCIe base spec doesn't document a Root Complex MSI
Capability.

I don't think it's the same as the one documented in PCIe r6.0, sec
7.7.2.  I think it's different because:

  (1) I *think* "pci" here refers to the RC, not to a Root Port.

  (2) The semantics are different.  The MSI-X Enable bit in 7.7.2 only
  determines whether the Function itself is permitted to use MSI-X.
  It has nothing to do with devices *below* a Root Port can use MSI-X.
  It also has nothing to do with whether a Root Port can forward MSI
  transactions from those downstream devices.

This part of my confusion could be easily resolved via a comment.

I do have a follow-on question, though: the patch seems to enable
MSI-related functionality using a register in the DesignWare IP, not
something in the i.MX6-specific IP.  If that's true, why don't other
DesignWare-based drivers need something similar?

> > > 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 <[e-mail]>
> > > > > ---
> > > > > 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%2F
> > > > >
> > patc%2F&data=05%7C01%7Chongxing.zhu%40nxp.com%7C24971d8de9b54b
> > 0b10
> > > > >
> > ad08db2182774d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6
> > 38140
> > > > >
> > 616456052078%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> > QIjoiV
> > > > >
> > 2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=vE
> > tRxL
> > > > > BVi5lYmpwTNZfafMms3263LZXodneLChjEaOM%3D&reserved=0
> > > > >
> > > >
> > 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%7C686ea1d3bc2b4c6fa9
> > 2
> > > > 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

Hongxing Zhu March 14, 2023, 3:24 a.m. UTC | #6

> -----Original Message-----
> From: Bjorn Helgaas <[e-mail]>
> Sent: 2023年3月14日 1:49
> To: Hongxing Zhu <[e-mail]>
> Cc: Lorenzo Pieralisi <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; dl-linux-imx <[e-mail]>
> Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI control of RC in
> suspend and resume
> 
> On Mon, Mar 13, 2023 at 02:50:31AM +0000, Hongxing Zhu wrote:
> > > -----Original Message-----
> > > From: Lorenzo Pieralisi <[e-mail]>
> > > Sent: 2023年3月11日 0:14
> > > To: Hongxing Zhu <[e-mail]>
> > > Cc: [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; dl-linux-imx
> > > <[e-mail]>
> > > Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI control of
> > > RC in suspend and resume
> > >
> > > On Mon, Jan 09, 2023 at 02:08:06AM +0000, Hongxing Zhu wrote:
> > > > > -----Original Message-----
> > > > > From: Lorenzo Pieralisi <[e-mail]>
> > > > > Sent: 2022年12月30日 23:06
> > > > > To: Hongxing Zhu <[e-mail]>; [e-mail];
> > > > > [e-mail]
> > > > > Cc: [e-mail];
> > > > > [e-mail];
> > > > > [e-mail]; [e-mail];
> > > > > dl-linux-imx <[e-mail]>
> > > > > 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.
> > >
> > > I still do not understand. The register you are saving/restoring in
> > > the RC is not the root port Message control field in the root port
> > > MSI capability, it is a separate register that controls the root
> > > complex MSI forwarding, is that correct ?
> > >
> > > The root port MSI capability does not control the root complex
> > > forwarding of MSIs TLPs.
> > >
> > > So the bits you are saving and restoring IIUC should be MMIO space
> > > in the root complex, dressed as an MSI capability, that has nothing
> > > to do with the root port MSI capability.
> > >
> > > Is that correct ?
> >
> > It's not a separate register.
> >
> > The bit I manipulated is the MSI Enable bit of the Message Control
> > Register for MSI (Offset 02h) contained in the MSI-capability of Root
> > Complex.
> >
> > In addition, on i.MX6, the MSI Enable bit controls delivery of MSI
> > interrupts from components below the Root Port.
> >
> > So, set MSI Enable in imx6q-pcie to let the MSI from downstream
> > components works.
> 
> My confusion is about this "MSI Capability" found by
> "dw_pcie_find_capability(pci, PCI_CAP_ID_MSI)" in your patch.
> 
> The i.MX6 manual might refer to that as an "MSI Capability" but as far as I
> know, the PCIe base spec doesn't document a Root Complex MSI Capability.
> 
> I don't think it's the same as the one documented in PCIe r6.0, sec 7.7.2.  I
> think it's different because:
> 
>   (1) I *think* "pci" here refers to the RC, not to a Root Port.
> 
>   (2) The semantics are different.  The MSI-X Enable bit in 7.7.2 only
>   determines whether the Function itself is permitted to use MSI-X.
>   It has nothing to do with devices *below* a Root Port can use MSI-X.
>   It also has nothing to do with whether a Root Port can forward MSI
>   transactions from those downstream devices.
> 
> This part of my confusion could be easily resolved via a comment.
> 
> I do have a follow-on question, though: the patch seems to enable
> MSI-related functionality using a register in the DesignWare IP, not something
> in the i.MX6-specific IP.  If that's true, why don't other DesignWare-based
> drivers need something similar?
Hi Bjorn:
Thanks a lot for you reply.
This behavior is specific for i.MX PCIe.
i.MX PCIe designer use this MSI_EN bit to control the MSI trigger when
 integrate Design Ware PCIe IP.
So, the other DesignWare-base PCIe driver doesn't need this beahvior.

Best Regards
Richard Zhu
> 
> > > > 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 <[e-mail]>
> > > > > > ---
> > > > > > 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%2
> > > > > > F%2F
> > > > > >
> > >
> patc%2F&data=05%7C01%7Chongxing.zhu%40nxp.com%7C24971d8de9b54b
> > > 0b10
> > > > > >
> > >
> ad08db2182774d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6
> > > 38140
> > > > > >
> > >
> 616456052078%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> > > QIjoiV
> > > > > >
> > >
> 2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=vE
> > > tRxL
> > > > > > BVi5lYmpwTNZfafMms3263LZXodneLChjEaOM%3D&reserved=0
> > > > > >
> > > > >
> > >
> 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%7C686ea1d3bc2b4c6fa9
> > > 2
> > > > > 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

Lorenzo Pieralisi March 14, 2023, 10:27 a.m. UTC | #7

On Tue, Mar 14, 2023 at 03:24:28AM +0000, Hongxing Zhu wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <[e-mail]>
> > Sent: 2023年3月14日 1:49
> > To: Hongxing Zhu <[e-mail]>
> > Cc: Lorenzo Pieralisi <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; dl-linux-imx <[e-mail]>
> > Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI control of RC in
> > suspend and resume
> > 
> > On Mon, Mar 13, 2023 at 02:50:31AM +0000, Hongxing Zhu wrote:
> > > > -----Original Message-----
> > > > From: Lorenzo Pieralisi <[e-mail]>
> > > > Sent: 2023年3月11日 0:14
> > > > To: Hongxing Zhu <[e-mail]>
> > > > Cc: [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail]; dl-linux-imx
> > > > <[e-mail]>
> > > > Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI control of
> > > > RC in suspend and resume
> > > >
> > > > On Mon, Jan 09, 2023 at 02:08:06AM +0000, Hongxing Zhu wrote:
> > > > > > -----Original Message-----
> > > > > > From: Lorenzo Pieralisi <[e-mail]>
> > > > > > Sent: 2022年12月30日 23:06
> > > > > > To: Hongxing Zhu <[e-mail]>; [e-mail];
> > > > > > [e-mail]
> > > > > > Cc: [e-mail];
> > > > > > [e-mail];
> > > > > > [e-mail]; [e-mail];
> > > > > > dl-linux-imx <[e-mail]>
> > > > > > 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.
> > > >
> > > > I still do not understand. The register you are saving/restoring in
> > > > the RC is not the root port Message control field in the root port
> > > > MSI capability, it is a separate register that controls the root
> > > > complex MSI forwarding, is that correct ?
> > > >
> > > > The root port MSI capability does not control the root complex
> > > > forwarding of MSIs TLPs.
> > > >
> > > > So the bits you are saving and restoring IIUC should be MMIO space
> > > > in the root complex, dressed as an MSI capability, that has nothing
> > > > to do with the root port MSI capability.
> > > >
> > > > Is that correct ?
> > >
> > > It's not a separate register.
> > >
> > > The bit I manipulated is the MSI Enable bit of the Message Control
> > > Register for MSI (Offset 02h) contained in the MSI-capability of Root
> > > Complex.
> > >
> > > In addition, on i.MX6, the MSI Enable bit controls delivery of MSI
> > > interrupts from components below the Root Port.
> > >
> > > So, set MSI Enable in imx6q-pcie to let the MSI from downstream
> > > components works.
> > 
> > My confusion is about this "MSI Capability" found by
> > "dw_pcie_find_capability(pci, PCI_CAP_ID_MSI)" in your patch.
> > 
> > The i.MX6 manual might refer to that as an "MSI Capability" but as far as I
> > know, the PCIe base spec doesn't document a Root Complex MSI Capability.
> > 
> > I don't think it's the same as the one documented in PCIe r6.0, sec 7.7.2.  I
> > think it's different because:
> > 
> >   (1) I *think* "pci" here refers to the RC, not to a Root Port.
> > 
> >   (2) The semantics are different.  The MSI-X Enable bit in 7.7.2 only
> >   determines whether the Function itself is permitted to use MSI-X.
> >   It has nothing to do with devices *below* a Root Port can use MSI-X.
> >   It also has nothing to do with whether a Root Port can forward MSI
> >   transactions from those downstream devices.
> > 
> > This part of my confusion could be easily resolved via a comment.
> > 
> > I do have a follow-on question, though: the patch seems to enable
> > MSI-related functionality using a register in the DesignWare IP, not something
> > in the i.MX6-specific IP.  If that's true, why don't other DesignWare-based
> > drivers need something similar?
> Hi Bjorn:
> Thanks a lot for you reply.
> This behavior is specific for i.MX PCIe.

Which behaviour ? It can't be the root port MSI capability, that would
be a HW bug (ie disabling root port MSIs would imply disabling MSIs for all
downstream components).

> i.MX PCIe designer use this MSI_EN bit to control the MSI trigger when
>  integrate Design Ware PCIe IP.

Fair enough but that can't be the MSI Enable bit in the Root Port MSI
capability "Message Control" field I am afraid.

It is what Bjorn mentioned quite clearly, a root complex configuration
register dressed as an MSI capability, the root complex is not a PCI
device; either that or that's an HW bug.

Lorenzo

> So, the other DesignWare-base PCIe driver doesn't need this beahvior.
> 
> Best Regards
> Richard Zhu
> > 
> > > > > 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 <[e-mail]>
> > > > > > > ---
> > > > > > > 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%2
> > > > > > > F%2F
> > > > > > >
> > > >
> > patc%2F&data=05%7C01%7Chongxing.zhu%40nxp.com%7C24971d8de9b54b
> > > > 0b10
> > > > > > >
> > > >
> > ad08db2182774d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6
> > > > 38140
> > > > > > >
> > > >
> > 616456052078%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> > > > QIjoiV
> > > > > > >
> > > >
> > 2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=vE
> > > > tRxL
> > > > > > > BVi5lYmpwTNZfafMms3263LZXodneLChjEaOM%3D&reserved=0
> > > > > > >
> > > > > >
> > > >
> > 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%7C686ea1d3bc2b4c6fa9
> > > > 2
> > > > > > 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

Hongxing Zhu March 16, 2023, 7:37 a.m. UTC | #8

> -----Original Message-----
> From: Lorenzo Pieralisi <[e-mail]>
> Sent: 2023年3月14日 18:28
> To: Hongxing Zhu <[e-mail]>
> Cc: Bjorn Helgaas <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; dl-linux-imx <[e-mail]>
> Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI control of RC in
> suspend and resume
> 
> On Tue, Mar 14, 2023 at 03:24:28AM +0000, Hongxing Zhu wrote:
> > > -----Original Message-----
> > > From: Bjorn Helgaas <[e-mail]>
> > > Sent: 2023年3月14日 1:49
> > > To: Hongxing Zhu <[e-mail]>
> > > Cc: Lorenzo Pieralisi <[e-mail]>;
> > > [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; dl-linux-imx
> > > <[e-mail]>
> > > Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI control of
> > > RC in suspend and resume
> > >
> > > On Mon, Mar 13, 2023 at 02:50:31AM +0000, Hongxing Zhu wrote:
> > > > > -----Original Message-----
> > > > > From: Lorenzo Pieralisi <[e-mail]>
> > > > > Sent: 2023年3月11日 0:14
> > > > > To: Hongxing Zhu <[e-mail]>
> > > > > Cc: [e-mail]; [e-mail];
> > > > > [e-mail]; [e-mail];
> > > > > [e-mail]; [e-mail];
> > > > > dl-linux-imx <[e-mail]>
> > > > > Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI control
> > > > > of RC in suspend and resume
> > > > >
> > > > > On Mon, Jan 09, 2023 at 02:08:06AM +0000, Hongxing Zhu wrote:
> > > > > > > -----Original Message-----
> > > > > > > From: Lorenzo Pieralisi <[e-mail]>
> > > > > > > Sent: 2022年12月30日 23:06
> > > > > > > To: Hongxing Zhu <[e-mail]>;
> > > > > > > [e-mail]; [e-mail]
> > > > > > > Cc: [e-mail];
> > > > > > > [e-mail];
> > > > > > > [e-mail]; [e-mail];
> > > > > > > dl-linux-imx <[e-mail]>
> > > > > > > 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.
> > > > >
> > > > > I still do not understand. The register you are saving/restoring
> > > > > in the RC is not the root port Message control field in the root
> > > > > port MSI capability, it is a separate register that controls the
> > > > > root complex MSI forwarding, is that correct ?
> > > > >
> > > > > The root port MSI capability does not control the root complex
> > > > > forwarding of MSIs TLPs.
> > > > >
> > > > > So the bits you are saving and restoring IIUC should be MMIO
> > > > > space in the root complex, dressed as an MSI capability, that
> > > > > has nothing to do with the root port MSI capability.
> > > > >
> > > > > Is that correct ?
> > > >
> > > > It's not a separate register.
> > > >
> > > > The bit I manipulated is the MSI Enable bit of the Message Control
> > > > Register for MSI (Offset 02h) contained in the MSI-capability of
> > > > Root Complex.
> > > >
> > > > In addition, on i.MX6, the MSI Enable bit controls delivery of MSI
> > > > interrupts from components below the Root Port.
> > > >
> > > > So, set MSI Enable in imx6q-pcie to let the MSI from downstream
> > > > components works.
> > >
> > > My confusion is about this "MSI Capability" found by
> > > "dw_pcie_find_capability(pci, PCI_CAP_ID_MSI)" in your patch.
> > >
> > > The i.MX6 manual might refer to that as an "MSI Capability" but as
> > > far as I know, the PCIe base spec doesn't document a Root Complex MSI
> Capability.
> > >
> > > I don't think it's the same as the one documented in PCIe r6.0, sec
> > > 7.7.2.  I think it's different because:
> > >
> > >   (1) I *think* "pci" here refers to the RC, not to a Root Port.
> > >
> > >   (2) The semantics are different.  The MSI-X Enable bit in 7.7.2 only
> > >   determines whether the Function itself is permitted to use MSI-X.
> > >   It has nothing to do with devices *below* a Root Port can use MSI-X.
> > >   It also has nothing to do with whether a Root Port can forward MSI
> > >   transactions from those downstream devices.
> > >
> > > This part of my confusion could be easily resolved via a comment.
> > >
> > > I do have a follow-on question, though: the patch seems to enable
> > > MSI-related functionality using a register in the DesignWare IP, not
> > > something in the i.MX6-specific IP.  If that's true, why don't other
> > > DesignWare-based drivers need something similar?
> > Hi Bjorn:
> > Thanks a lot for you reply.
> > This behavior is specific for i.MX PCIe.
> 
> Which behaviour ? It can't be the root port MSI capability, that would be a HW
> bug (ie disabling root port MSIs would imply disabling MSIs for all downstream
> components).
> 

i.MX PCIe designer use this MSI_EN bit to control the MSI trigger when integrate
 Design Ware PCIe IP.
Without the MSI_EN bit assertion (1b'1), the devices below this RC can't trigger
 the MSI successfully.
Yes, you're right. It should not be the root port MSI capability. 

> > i.MX PCIe designer use this MSI_EN bit to control the MSI trigger when
> > integrate Design Ware PCIe IP.
> 
> Fair enough but that can't be the MSI Enable bit in the Root Port MSI
> capability "Message Control" field I am afraid.
> 
> It is what Bjorn mentioned quite clearly, a root complex configuration register
> dressed as an MSI capability, the root complex is not a PCI device; either that
> or that's an HW bug.
Yes, it is. I agree with you. Had report this situation to the design team.
Hope to correct this bug in HW design if it's possible.
Thanks.

Best Regards
Richard Zhu
> 
> Lorenzo
> 
> > So, the other DesignWare-base PCIe driver doesn't need this beahvior.
> >
> > Best Regards
> > Richard Zhu
> > >
> > > > > > 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 <[e-mail]>
> > > > > > > > ---
> > > > > > > > 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%2
> > > > > > > > F%2F
> > > > > > > >
> > > > >
> > >
> patc%2F&data=05%7C01%7Chongxing.zhu%40nxp.com%7C24971d8de9b54b
> > > > > 0b10
> > > > > > > >
> > > > >
> > >
> ad08db2182774d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6
> > > > > 38140
> > > > > > > >
> > > > >
> > >
> 616456052078%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> > > > > QIjoiV
> > > > > > > >
> > > > >
> > >
> 2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=vE
> > > > > tRxL
> > > > > > > > BVi5lYmpwTNZfafMms3263LZXodneLChjEaOM%3D&reserved=0
> > > > > > > >
> > > > > > >
> > > > >
> > >
> 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%7C686ea1d3bc2b4c6fa9
> > > > > 2
> > > > > > > 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

Lorenzo Pieralisi March 16, 2023, 8:10 a.m. UTC | #9

On Thu, Mar 16, 2023 at 07:37:41AM +0000, Hongxing Zhu wrote:

[...]

> > > > > It's not a separate register.
> > > > >
> > > > > The bit I manipulated is the MSI Enable bit of the Message Control
> > > > > Register for MSI (Offset 02h) contained in the MSI-capability of
> > > > > Root Complex.
> > > > >
> > > > > In addition, on i.MX6, the MSI Enable bit controls delivery of MSI
> > > > > interrupts from components below the Root Port.
> > > > >
> > > > > So, set MSI Enable in imx6q-pcie to let the MSI from downstream
> > > > > components works.
> > > >
> > > > My confusion is about this "MSI Capability" found by
> > > > "dw_pcie_find_capability(pci, PCI_CAP_ID_MSI)" in your patch.
> > > >
> > > > The i.MX6 manual might refer to that as an "MSI Capability" but as
> > > > far as I know, the PCIe base spec doesn't document a Root Complex MSI
> > Capability.
> > > >
> > > > I don't think it's the same as the one documented in PCIe r6.0, sec
> > > > 7.7.2.  I think it's different because:
> > > >
> > > >   (1) I *think* "pci" here refers to the RC, not to a Root Port.
> > > >
> > > >   (2) The semantics are different.  The MSI-X Enable bit in 7.7.2 only
> > > >   determines whether the Function itself is permitted to use MSI-X.
> > > >   It has nothing to do with devices *below* a Root Port can use MSI-X.
> > > >   It also has nothing to do with whether a Root Port can forward MSI
> > > >   transactions from those downstream devices.
> > > >
> > > > This part of my confusion could be easily resolved via a comment.
> > > >
> > > > I do have a follow-on question, though: the patch seems to enable
> > > > MSI-related functionality using a register in the DesignWare IP, not
> > > > something in the i.MX6-specific IP.  If that's true, why don't other
> > > > DesignWare-based drivers need something similar?
> > > Hi Bjorn:
> > > Thanks a lot for you reply.
> > > This behavior is specific for i.MX PCIe.
> > 
> > Which behaviour ? It can't be the root port MSI capability, that would be a HW
> > bug (ie disabling root port MSIs would imply disabling MSIs for all downstream
> > components).
> > 
> 
> i.MX PCIe designer use this MSI_EN bit to control the MSI trigger when integrate
>  Design Ware PCIe IP.
> Without the MSI_EN bit assertion (1b'1), the devices below this RC can't trigger
>  the MSI successfully.
> Yes, you're right. It should not be the root port MSI capability. 

The question is, it is or it is not the root port MSI capability ?

If it is, that's a HW bug.

If it is not there is nothing to do and this patch can be merged.

> > > i.MX PCIe designer use this MSI_EN bit to control the MSI trigger when
> > > integrate Design Ware PCIe IP.
> > 
> > Fair enough but that can't be the MSI Enable bit in the Root Port MSI
> > capability "Message Control" field I am afraid.
> > 
> > It is what Bjorn mentioned quite clearly, a root complex configuration register
> > dressed as an MSI capability, the root complex is not a PCI device; either that
> > or that's an HW bug.
> Yes, it is. I agree with you. Had report this situation to the design team.
> Hope to correct this bug in HW design if it's possible.

I don't understand if it is a HW bug or not, see above. I think it
is legitimate to have MMIO register space that *looks* like an MSI
capability for the root complex to control delivery of MSI interrupts,
as long as it is not the actual root port MSI capability, in the
root port PCI config space in which case this would be a HW bug from
what you are reporting.

Please clarify, thank you very much.

Lorenzo

> Thanks.
> 
> Best Regards
> Richard Zhu
> > 
> > Lorenzo
> > 
> > > So, the other DesignWare-base PCIe driver doesn't need this beahvior.
> > >
> > > Best Regards
> > > Richard Zhu
> > > >
> > > > > > > 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 <[e-mail]>
> > > > > > > > > ---
> > > > > > > > > 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%2
> > > > > > > > > F%2F
> > > > > > > > >
> > > > > >
> > > >
> > patc%2F&data=05%7C01%7Chongxing.zhu%40nxp.com%7C24971d8de9b54b
> > > > > > 0b10
> > > > > > > > >
> > > > > >
> > > >
> > ad08db2182774d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6
> > > > > > 38140
> > > > > > > > >
> > > > > >
> > > >
> > 616456052078%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> > > > > > QIjoiV
> > > > > > > > >
> > > > > >
> > > >
> > 2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=vE
> > > > > > tRxL
> > > > > > > > > BVi5lYmpwTNZfafMms3263LZXodneLChjEaOM%3D&reserved=0
> > > > > > > > >
> > > > > > > >
> > > > > >
> > > >
> > 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%7C686ea1d3bc2b4c6fa9
> > > > > > 2
> > > > > > > > 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

Hongxing Zhu March 17, 2023, 7:38 a.m. UTC | #10

> -----Original Message-----
> From: Lorenzo Pieralisi <[e-mail]>
> Sent: 2023年3月16日 16:11
> To: Hongxing Zhu <[e-mail]>
> Cc: Bjorn Helgaas <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; dl-linux-imx <[e-mail]>
> Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI control of RC in
> suspend and resume
> 
> On Thu, Mar 16, 2023 at 07:37:41AM +0000, Hongxing Zhu wrote:
> 
> [...]
> 
> > > > > > It's not a separate register.
> > > > > >
> > > > > > The bit I manipulated is the MSI Enable bit of the Message
> > > > > > Control Register for MSI (Offset 02h) contained in the
> > > > > > MSI-capability of Root Complex.
> > > > > >
> > > > > > In addition, on i.MX6, the MSI Enable bit controls delivery of
> > > > > > MSI interrupts from components below the Root Port.
> > > > > >
> > > > > > So, set MSI Enable in imx6q-pcie to let the MSI from
> > > > > > downstream components works.
> > > > >
> > > > > My confusion is about this "MSI Capability" found by
> > > > > "dw_pcie_find_capability(pci, PCI_CAP_ID_MSI)" in your patch.
> > > > >
> > > > > The i.MX6 manual might refer to that as an "MSI Capability" but
> > > > > as far as I know, the PCIe base spec doesn't document a Root
> > > > > Complex MSI
> > > Capability.
> > > > >
> > > > > I don't think it's the same as the one documented in PCIe r6.0,
> > > > > sec 7.7.2.  I think it's different because:
> > > > >
> > > > >   (1) I *think* "pci" here refers to the RC, not to a Root Port.
> > > > >
> > > > >   (2) The semantics are different.  The MSI-X Enable bit in 7.7.2 only
> > > > >   determines whether the Function itself is permitted to use MSI-X.
> > > > >   It has nothing to do with devices *below* a Root Port can use
> MSI-X.
> > > > >   It also has nothing to do with whether a Root Port can forward MSI
> > > > >   transactions from those downstream devices.
> > > > >
> > > > > This part of my confusion could be easily resolved via a comment.
> > > > >
> > > > > I do have a follow-on question, though: the patch seems to
> > > > > enable MSI-related functionality using a register in the
> > > > > DesignWare IP, not something in the i.MX6-specific IP.  If
> > > > > that's true, why don't other DesignWare-based drivers need something
> similar?
> > > > Hi Bjorn:
> > > > Thanks a lot for you reply.
> > > > This behavior is specific for i.MX PCIe.
> > >
> > > Which behaviour ? It can't be the root port MSI capability, that
> > > would be a HW bug (ie disabling root port MSIs would imply disabling
> > > MSIs for all downstream components).
> > >
> >
> > i.MX PCIe designer use this MSI_EN bit to control the MSI trigger when
> > integrate  Design Ware PCIe IP.
> > Without the MSI_EN bit assertion (1b'1), the devices below this RC
> > can't trigger  the MSI successfully.
> > Yes, you're right. It should not be the root port MSI capability.
> 
> The question is, it is or it is not the root port MSI capability ?
> 
> If it is, that's a HW bug.
> 
> If it is not there is nothing to do and this patch can be merged.
Hi Lorenzo:
Thanks for your reply.
I think it is not the root port MSI capability actually.
Refer to my understands, designer just use the msi_en bit to control the
 delivery of MSI interrupts from components below the Root Port.
> 
> > > > i.MX PCIe designer use this MSI_EN bit to control the MSI trigger
> > > > when integrate Design Ware PCIe IP.
> > >
> > > Fair enough but that can't be the MSI Enable bit in the Root Port
> > > MSI capability "Message Control" field I am afraid.
> > >
> > > It is what Bjorn mentioned quite clearly, a root complex
> > > configuration register dressed as an MSI capability, the root
> > > complex is not a PCI device; either that or that's an HW bug.
> > Yes, it is. I agree with you. Had report this situation to the design team.
> > Hope to correct this bug in HW design if it's possible.
> 
> I don't understand if it is a HW bug or not, see above. I think it is legitimate to
> have MMIO register space that *looks* like an MSI capability for the root
> complex to control delivery of MSI interrupts, as long as it is not the actual
> root port MSI capability, in the root port PCI config space in which case this
> would be a HW bug from what you are reporting.
I just provide the following suggestions.
- Root complex shouldn't have the MSI capability refer to the PCIe Spec
  7.7.1 chapter.
- Root port MSIs should not imply disabling MSIs for all downstream components.

Thanks.
Best Regards
Richard
> 
> Please clarify, thank you very much.
> 
> Lorenzo
> 
> > Thanks.
> >
> > Best Regards
> > Richard Zhu
> > >
> > > Lorenzo
> > >
> > > > So, the other DesignWare-base PCIe driver doesn't need this beahvior.
> > > >
> > > > Best Regards
> > > > Richard Zhu
> > > > >
> > > > > > > > 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 <[e-mail]>
> > > > > > > > > > ---
> > > > > > > > > > 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=ht
> > > > > > > > > > tps%
> > > > > > > > > > 3A%2
> > > > > > > > > > F%2F
> > > > > > > > > >
> > > > > > >
> > > > >
> > >
> patc%2F&data=05%7C01%7Chongxing.zhu%40nxp.com%7C24971d8de9b54b
> > > > > > > 0b10
> > > > > > > > > >
> > > > > > >
> > > > >
> > >
> ad08db2182774d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6
> > > > > > > 38140
> > > > > > > > > >
> > > > > > >
> > > > >
> > >
> 616456052078%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> > > > > > > QIjoiV
> > > > > > > > > >
> > > > > > >
> > > > >
> > >
> 2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=vE
> > > > > > > tRxL
> > > > > > > > > >
> BVi5lYmpwTNZfafMms3263LZXodneLChjEaOM%3D&reserved=0
> > > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > >
> > >
> 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%7C686ea1d3bc2b4c6fa9
> > > > > > > 2
> > > > > > > > > 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

Bjorn Helgaas March 17, 2023, 10:24 p.m. UTC | #11

On Fri, Mar 17, 2023 at 07:38:02AM +0000, Hongxing Zhu wrote:
> > -----Original Message-----
> > From: Lorenzo Pieralisi <[e-mail]>
> > Sent: 2023年3月16日 16:11
> > To: Hongxing Zhu <[e-mail]>
> > Cc: Bjorn Helgaas <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; dl-linux-imx <[e-mail]>
> > Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI control of RC in
> > suspend and resume
> > 
> > On Thu, Mar 16, 2023 at 07:37:41AM +0000, Hongxing Zhu wrote:
> > 
> > [...]
> > 
> > > > > > > It's not a separate register.
> > > > > > >
> > > > > > > The bit I manipulated is the MSI Enable bit of the Message
> > > > > > > Control Register for MSI (Offset 02h) contained in the
> > > > > > > MSI-capability of Root Complex.
> > > > > > >
> > > > > > > In addition, on i.MX6, the MSI Enable bit controls delivery of
> > > > > > > MSI interrupts from components below the Root Port.
> > > > > > >
> > > > > > > So, set MSI Enable in imx6q-pcie to let the MSI from
> > > > > > > downstream components works.
> > > > > >
> > > > > > My confusion is about this "MSI Capability" found by
> > > > > > "dw_pcie_find_capability(pci, PCI_CAP_ID_MSI)" in your patch.
> > > > > >
> > > > > > The i.MX6 manual might refer to that as an "MSI Capability" but
> > > > > > as far as I know, the PCIe base spec doesn't document a Root
> > > > > > Complex MSI
> > > > Capability.
> > > > > >
> > > > > > I don't think it's the same as the one documented in PCIe r6.0,
> > > > > > sec 7.7.2.  I think it's different because:
> > > > > >
> > > > > >   (1) I *think* "pci" here refers to the RC, not to a Root Port.
> > > > > >
> > > > > >   (2) The semantics are different.  The MSI-X Enable bit in 7.7.2 only
> > > > > >   determines whether the Function itself is permitted to use MSI-X.
> > > > > >   It has nothing to do with devices *below* a Root Port can use
> > MSI-X.
> > > > > >   It also has nothing to do with whether a Root Port can forward MSI
> > > > > >   transactions from those downstream devices.
> > > > > >
> > > > > > This part of my confusion could be easily resolved via a comment.
> > > > > >
> > > > > > I do have a follow-on question, though: the patch seems to
> > > > > > enable MSI-related functionality using a register in the
> > > > > > DesignWare IP, not something in the i.MX6-specific IP.  If
> > > > > > that's true, why don't other DesignWare-based drivers need something
> > similar?
> > > > > Hi Bjorn:
> > > > > Thanks a lot for you reply.
> > > > > This behavior is specific for i.MX PCIe.
> > > >
> > > > Which behaviour ? It can't be the root port MSI capability, that
> > > > would be a HW bug (ie disabling root port MSIs would imply disabling
> > > > MSIs for all downstream components).
> > > >
> > >
> > > i.MX PCIe designer use this MSI_EN bit to control the MSI trigger when
> > > integrate  Design Ware PCIe IP.
> > > Without the MSI_EN bit assertion (1b'1), the devices below this RC
> > > can't trigger  the MSI successfully.
> > > Yes, you're right. It should not be the root port MSI capability.
> > 
> > The question is, it is or it is not the root port MSI capability ?
> > 
> > If it is, that's a HW bug.
> > 
> > If it is not there is nothing to do and this patch can be merged.
> Hi Lorenzo:
> Thanks for your reply.
> I think it is not the root port MSI capability actually.
> Refer to my understands, designer just use the msi_en bit to control the
>  delivery of MSI interrupts from components below the Root Port.
> > 
> > > > > i.MX PCIe designer use this MSI_EN bit to control the MSI trigger
> > > > > when integrate Design Ware PCIe IP.
> > > >
> > > > Fair enough but that can't be the MSI Enable bit in the Root Port
> > > > MSI capability "Message Control" field I am afraid.
> > > >
> > > > It is what Bjorn mentioned quite clearly, a root complex
> > > > configuration register dressed as an MSI capability, the root
> > > > complex is not a PCI device; either that or that's an HW bug.
> > > Yes, it is. I agree with you. Had report this situation to the design team.
> > > Hope to correct this bug in HW design if it's possible.
> > 
> > I don't understand if it is a HW bug or not, see above. I think it is legitimate to
> > have MMIO register space that *looks* like an MSI capability for the root
> > complex to control delivery of MSI interrupts, as long as it is not the actual
> > root port MSI capability, in the root port PCI config space in which case this
> > would be a HW bug from what you are reporting.
> I just provide the following suggestions.
> - Root complex shouldn't have the MSI capability refer to the PCIe Spec
>   7.7.1 chapter.
> - Root port MSIs should not imply disabling MSIs for all downstream components.

I think this is all a lot of confusion, mostly on my part, sorry about
that.

Root Complex configuration and behavior is not specified by the PCIe
spec, so that's completely up to the i.MX designer.  It's fine for the
Root Complex to have an MSI Capability, and it's fine for that
capability to enable/disable the RC fielding of MSI MemWr transactions
from downstream devices and triggering MSI interrupts.

It's also fine for the RC MSI Capability to be identified with a
Capability ID of 0x5, although it is slightly confusing to use
PCI_CAP_ID_MSI to find it.  It's also slightly confusing to use the
PCI_MSI_FLAGS offset into the RC MSI Capability.

Using the PCI_CAP_ID_MSI, PCI_MSI_FLAGS, and PCI_MSI_FLAGS_ENABLE
macros suggests to the reader that this RC MSI capability is the same
as the the MSI Capability defined by PCIe r6.0, sec 7.7.1.  Obviously
it is *not* the same, because we're talking about a *Root Complex*
capability, while the sec 7.7.1 capability can only appear on PCIe
functions (Root Ports, Endpoints, Switch Ports, etc).

I suggest a comment to the effect that this is a Root Complex MSI
Capability, not the MSI Capability defined by PCIe r6.0, sec 7.7.1.

Possibly even add new #defines in pci-imx6.c with different names,
even though the values happen to be the same as the PCI_MSI_*
#defines.  That would be a convenient place to put a comment about
what they are.

Bjorn

Hongxing Zhu March 20, 2023, 7:02 a.m. UTC | #12

> -----Original Message-----
> From: Bjorn Helgaas <[e-mail]>
> Sent: 2023年3月18日 6:25
> To: Hongxing Zhu <[e-mail]>
> Cc: Lorenzo Pieralisi <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; dl-linux-imx <[e-mail]>
> Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI control of RC in suspend
> and resume
> 
> On Fri, Mar 17, 2023 at 07:38:02AM +0000, Hongxing Zhu wrote:
> > > -----Original Message-----
> > > From: Lorenzo Pieralisi <[e-mail]>
> > > Sent: 2023年3月16日 16:11
> > > To: Hongxing Zhu <[e-mail]>
> > > Cc: Bjorn Helgaas <[e-mail]>; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; dl-linux-imx <[e-mail]>
> > > Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI control of
> > > RC in suspend and resume
> > >
> > > On Thu, Mar 16, 2023 at 07:37:41AM +0000, Hongxing Zhu wrote:
> > >
> > > [...]
> > >
> > > > > > > > It's not a separate register.
> > > > > > > >
> > > > > > > > The bit I manipulated is the MSI Enable bit of the Message
> > > > > > > > Control Register for MSI (Offset 02h) contained in the
> > > > > > > > MSI-capability of Root Complex.
> > > > > > > >
> > > > > > > > In addition, on i.MX6, the MSI Enable bit controls
> > > > > > > > delivery of MSI interrupts from components below the Root Port.
> > > > > > > >
> > > > > > > > So, set MSI Enable in imx6q-pcie to let the MSI from
> > > > > > > > downstream components works.
> > > > > > >
> > > > > > > My confusion is about this "MSI Capability" found by
> > > > > > > "dw_pcie_find_capability(pci, PCI_CAP_ID_MSI)" in your patch.
> > > > > > >
> > > > > > > The i.MX6 manual might refer to that as an "MSI Capability"
> > > > > > > but as far as I know, the PCIe base spec doesn't document a
> > > > > > > Root Complex MSI
> > > > > Capability.
> > > > > > >
> > > > > > > I don't think it's the same as the one documented in PCIe
> > > > > > > r6.0, sec 7.7.2.  I think it's different because:
> > > > > > >
> > > > > > >   (1) I *think* "pci" here refers to the RC, not to a Root Port.
> > > > > > >
> > > > > > >   (2) The semantics are different.  The MSI-X Enable bit in 7.7.2 only
> > > > > > >   determines whether the Function itself is permitted to use MSI-X.
> > > > > > >   It has nothing to do with devices *below* a Root Port can
> > > > > > > use
> > > MSI-X.
> > > > > > >   It also has nothing to do with whether a Root Port can forward MSI
> > > > > > >   transactions from those downstream devices.
> > > > > > >
> > > > > > > This part of my confusion could be easily resolved via a comment.
> > > > > > >
> > > > > > > I do have a follow-on question, though: the patch seems to
> > > > > > > enable MSI-related functionality using a register in the
> > > > > > > DesignWare IP, not something in the i.MX6-specific IP.  If
> > > > > > > that's true, why don't other DesignWare-based drivers need
> > > > > > > something
> > > similar?
> > > > > > Hi Bjorn:
> > > > > > Thanks a lot for you reply.
> > > > > > This behavior is specific for i.MX PCIe.
> > > > >
> > > > > Which behaviour ? It can't be the root port MSI capability, that
> > > > > would be a HW bug (ie disabling root port MSIs would imply
> > > > > disabling MSIs for all downstream components).
> > > > >
> > > >
> > > > i.MX PCIe designer use this MSI_EN bit to control the MSI trigger
> > > > when integrate  Design Ware PCIe IP.
> > > > Without the MSI_EN bit assertion (1b'1), the devices below this RC
> > > > can't trigger  the MSI successfully.
> > > > Yes, you're right. It should not be the root port MSI capability.
> > >
> > > The question is, it is or it is not the root port MSI capability ?
> > >
> > > If it is, that's a HW bug.
> > >
> > > If it is not there is nothing to do and this patch can be merged.
> > Hi Lorenzo:
> > Thanks for your reply.
> > I think it is not the root port MSI capability actually.
> > Refer to my understands, designer just use the msi_en bit to control
> > the  delivery of MSI interrupts from components below the Root Port.
> > >
> > > > > > i.MX PCIe designer use this MSI_EN bit to control the MSI
> > > > > > trigger when integrate Design Ware PCIe IP.
> > > > >
> > > > > Fair enough but that can't be the MSI Enable bit in the Root
> > > > > Port MSI capability "Message Control" field I am afraid.
> > > > >
> > > > > It is what Bjorn mentioned quite clearly, a root complex
> > > > > configuration register dressed as an MSI capability, the root
> > > > > complex is not a PCI device; either that or that's an HW bug.
> > > > Yes, it is. I agree with you. Had report this situation to the design team.
> > > > Hope to correct this bug in HW design if it's possible.
> > >
> > > I don't understand if it is a HW bug or not, see above. I think it
> > > is legitimate to have MMIO register space that *looks* like an MSI
> > > capability for the root complex to control delivery of MSI
> > > interrupts, as long as it is not the actual root port MSI
> > > capability, in the root port PCI config space in which case this would be a HW
> bug from what you are reporting.
> > I just provide the following suggestions.
> > - Root complex shouldn't have the MSI capability refer to the PCIe Spec
> >   7.7.1 chapter.
> > - Root port MSIs should not imply disabling MSIs for all downstream
> components.
> 
> I think this is all a lot of confusion, mostly on my part, sorry about that.
> 
> Root Complex configuration and behavior is not specified by the PCIe spec, so
> that's completely up to the i.MX designer.  It's fine for the Root Complex to have
> an MSI Capability, and it's fine for that capability to enable/disable the RC fielding
> of MSI MemWr transactions from downstream devices and triggering MSI
> interrupts.
> 
> It's also fine for the RC MSI Capability to be identified with a Capability ID of 0x5,
> although it is slightly confusing to use PCI_CAP_ID_MSI to find it.  It's also
> slightly confusing to use the PCI_MSI_FLAGS offset into the RC MSI Capability.
> 
> Using the PCI_CAP_ID_MSI, PCI_MSI_FLAGS, and PCI_MSI_FLAGS_ENABLE
> macros suggests to the reader that this RC MSI capability is the same as the the
> MSI Capability defined by PCIe r6.0, sec 7.7.1.  Obviously it is *not* the same,
> because we're talking about a *Root Complex* capability, while the sec 7.7.1
> capability can only appear on PCIe functions (Root Ports, Endpoints, Switch Ports,
> etc).
> 
> I suggest a comment to the effect that this is a Root Complex MSI Capability, not
> the MSI Capability defined by PCIe r6.0, sec 7.7.1.
> 
> Possibly even add new #defines in pci-imx6.c with different names, even though
> the values happen to be the same as the PCI_MSI_* #defines.  That would be a
> convenient place to put a comment about what they are.
Hi Bjorn:
Thanks a lot for your dispelling doubts.
How about to add the following comments in the new add function to clarify it?

--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1036,6 +1036,18 @@ static void pci_imx_set_msi_en(struct dw_pcie *pci)
        u8 offset;
        u16 val;

+       /*
+        * When i.MX DM PCIe controller is configured as RC mode, it has one
+        * MSI Capability Structure, although PCIe r6.0, sec 7.7.1 doesn't
+        * specify the MSI Capability Structures for Root Complex.
+        *
+        * The MSI_EN bit of MSI control register contained in this MSI-CAP
+        * is used control the MSI delivery of MSI interrupts from components
+        * below the Root Port.
+        *
+        * Find it by PCI_CAP_ID_MSI here, and assert the MSI_EN bit to allow
+        * the MSI delivery below the Root Port, if the PCI MSI is enabled.
+        */
        if (pci_msi_enabled()) {
                offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
                dw_pcie_dbi_ro_wr_en(pci);
Best Regards
Richard Zhu
> 
> Bjorn

Lorenzo Pieralisi March 24, 2023, 3:59 p.m. UTC | #13

On Mon, Mar 20, 2023 at 07:02:35AM +0000, Hongxing Zhu wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <[e-mail]>
> > Sent: 2023年3月18日 6:25
> > To: Hongxing Zhu <[e-mail]>
> > Cc: Lorenzo Pieralisi <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; dl-linux-imx <[e-mail]>
> > Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI control of RC in suspend
> > and resume
> > 
> > On Fri, Mar 17, 2023 at 07:38:02AM +0000, Hongxing Zhu wrote:
> > > > -----Original Message-----
> > > > From: Lorenzo Pieralisi <[e-mail]>
> > > > Sent: 2023年3月16日 16:11
> > > > To: Hongxing Zhu <[e-mail]>
> > > > Cc: Bjorn Helgaas <[e-mail]>; [e-mail];
> > > > [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail];
> > > > [e-mail]; dl-linux-imx <[e-mail]>
> > > > Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI control of
> > > > RC in suspend and resume
> > > >
> > > > On Thu, Mar 16, 2023 at 07:37:41AM +0000, Hongxing Zhu wrote:
> > > >
> > > > [...]
> > > >
> > > > > > > > > It's not a separate register.
> > > > > > > > >
> > > > > > > > > The bit I manipulated is the MSI Enable bit of the Message
> > > > > > > > > Control Register for MSI (Offset 02h) contained in the
> > > > > > > > > MSI-capability of Root Complex.
> > > > > > > > >
> > > > > > > > > In addition, on i.MX6, the MSI Enable bit controls
> > > > > > > > > delivery of MSI interrupts from components below the Root Port.
> > > > > > > > >
> > > > > > > > > So, set MSI Enable in imx6q-pcie to let the MSI from
> > > > > > > > > downstream components works.
> > > > > > > >
> > > > > > > > My confusion is about this "MSI Capability" found by
> > > > > > > > "dw_pcie_find_capability(pci, PCI_CAP_ID_MSI)" in your patch.
> > > > > > > >
> > > > > > > > The i.MX6 manual might refer to that as an "MSI Capability"
> > > > > > > > but as far as I know, the PCIe base spec doesn't document a
> > > > > > > > Root Complex MSI
> > > > > > Capability.
> > > > > > > >
> > > > > > > > I don't think it's the same as the one documented in PCIe
> > > > > > > > r6.0, sec 7.7.2.  I think it's different because:
> > > > > > > >
> > > > > > > >   (1) I *think* "pci" here refers to the RC, not to a Root Port.
> > > > > > > >
> > > > > > > >   (2) The semantics are different.  The MSI-X Enable bit in 7.7.2 only
> > > > > > > >   determines whether the Function itself is permitted to use MSI-X.
> > > > > > > >   It has nothing to do with devices *below* a Root Port can
> > > > > > > > use
> > > > MSI-X.
> > > > > > > >   It also has nothing to do with whether a Root Port can forward MSI
> > > > > > > >   transactions from those downstream devices.
> > > > > > > >
> > > > > > > > This part of my confusion could be easily resolved via a comment.
> > > > > > > >
> > > > > > > > I do have a follow-on question, though: the patch seems to
> > > > > > > > enable MSI-related functionality using a register in the
> > > > > > > > DesignWare IP, not something in the i.MX6-specific IP.  If
> > > > > > > > that's true, why don't other DesignWare-based drivers need
> > > > > > > > something
> > > > similar?
> > > > > > > Hi Bjorn:
> > > > > > > Thanks a lot for you reply.
> > > > > > > This behavior is specific for i.MX PCIe.
> > > > > >
> > > > > > Which behaviour ? It can't be the root port MSI capability, that
> > > > > > would be a HW bug (ie disabling root port MSIs would imply
> > > > > > disabling MSIs for all downstream components).
> > > > > >
> > > > >
> > > > > i.MX PCIe designer use this MSI_EN bit to control the MSI trigger
> > > > > when integrate  Design Ware PCIe IP.
> > > > > Without the MSI_EN bit assertion (1b'1), the devices below this RC
> > > > > can't trigger  the MSI successfully.
> > > > > Yes, you're right. It should not be the root port MSI capability.
> > > >
> > > > The question is, it is or it is not the root port MSI capability ?
> > > >
> > > > If it is, that's a HW bug.
> > > >
> > > > If it is not there is nothing to do and this patch can be merged.
> > > Hi Lorenzo:
> > > Thanks for your reply.
> > > I think it is not the root port MSI capability actually.
> > > Refer to my understands, designer just use the msi_en bit to control
> > > the  delivery of MSI interrupts from components below the Root Port.
> > > >
> > > > > > > i.MX PCIe designer use this MSI_EN bit to control the MSI
> > > > > > > trigger when integrate Design Ware PCIe IP.
> > > > > >
> > > > > > Fair enough but that can't be the MSI Enable bit in the Root
> > > > > > Port MSI capability "Message Control" field I am afraid.
> > > > > >
> > > > > > It is what Bjorn mentioned quite clearly, a root complex
> > > > > > configuration register dressed as an MSI capability, the root
> > > > > > complex is not a PCI device; either that or that's an HW bug.
> > > > > Yes, it is. I agree with you. Had report this situation to the design team.
> > > > > Hope to correct this bug in HW design if it's possible.
> > > >
> > > > I don't understand if it is a HW bug or not, see above. I think it
> > > > is legitimate to have MMIO register space that *looks* like an MSI
> > > > capability for the root complex to control delivery of MSI
> > > > interrupts, as long as it is not the actual root port MSI
> > > > capability, in the root port PCI config space in which case this would be a HW
> > bug from what you are reporting.
> > > I just provide the following suggestions.
> > > - Root complex shouldn't have the MSI capability refer to the PCIe Spec
> > >   7.7.1 chapter.
> > > - Root port MSIs should not imply disabling MSIs for all downstream
> > components.
> > 
> > I think this is all a lot of confusion, mostly on my part, sorry about that.
> > 
> > Root Complex configuration and behavior is not specified by the PCIe spec, so
> > that's completely up to the i.MX designer.  It's fine for the Root Complex to have
> > an MSI Capability, and it's fine for that capability to enable/disable the RC fielding
> > of MSI MemWr transactions from downstream devices and triggering MSI
> > interrupts.
> > 
> > It's also fine for the RC MSI Capability to be identified with a Capability ID of 0x5,
> > although it is slightly confusing to use PCI_CAP_ID_MSI to find it.  It's also
> > slightly confusing to use the PCI_MSI_FLAGS offset into the RC MSI Capability.
> > 
> > Using the PCI_CAP_ID_MSI, PCI_MSI_FLAGS, and PCI_MSI_FLAGS_ENABLE
> > macros suggests to the reader that this RC MSI capability is the same as the the
> > MSI Capability defined by PCIe r6.0, sec 7.7.1.  Obviously it is *not* the same,
> > because we're talking about a *Root Complex* capability, while the sec 7.7.1
> > capability can only appear on PCIe functions (Root Ports, Endpoints, Switch Ports,
> > etc).
> > 
> > I suggest a comment to the effect that this is a Root Complex MSI Capability, not
> > the MSI Capability defined by PCIe r6.0, sec 7.7.1.
> > 
> > Possibly even add new #defines in pci-imx6.c with different names, even though
> > the values happen to be the same as the PCI_MSI_* #defines.  That would be a
> > convenient place to put a comment about what they are.
> Hi Bjorn:
> Thanks a lot for your dispelling doubts.
> How about to add the following comments in the new add function to clarify it?
> 
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -1036,6 +1036,18 @@ static void pci_imx_set_msi_en(struct dw_pcie *pci)
>         u8 offset;
>         u16 val;
> 
> +       /*
> +        * When i.MX DM PCIe controller is configured as RC mode, it has one
> +        * MSI Capability Structure, although PCIe r6.0, sec 7.7.1 doesn't
> +        * specify the MSI Capability Structures for Root Complex.

That's because a PCI root complex is not a PCI device (and this is not
an MSI capability, which lives in PCI config space).

I will reword it (and the commit log with it) and merge it in the coming
weeks for v6.4

Thanks,
Lorenzo

> +        *
> +        * The MSI_EN bit of MSI control register contained in this MSI-CAP
> +        * is used control the MSI delivery of MSI interrupts from components
> +        * below the Root Port.
> +        *
> +        * Find it by PCI_CAP_ID_MSI here, and assert the MSI_EN bit to allow
> +        * the MSI delivery below the Root Port, if the PCI MSI is enabled.
> +        */
>         if (pci_msi_enabled()) {
>                 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
>                 dw_pcie_dbi_ro_wr_en(pci);
> Best Regards
> Richard Zhu
> > 
> > Bjorn

Hongxing Zhu March 27, 2023, 12:22 a.m. UTC | #14

> -----Original Message-----
> From: Lorenzo Pieralisi <[e-mail]>
> Sent: 2023年3月24日 23:59
> To: Hongxing Zhu <[e-mail]>
> Cc: Bjorn Helgaas <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; dl-linux-imx <[e-mail]>
> Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI control of RC in suspend
> and resume
> 
> On Mon, Mar 20, 2023 at 07:02:35AM +0000, Hongxing Zhu wrote:
> > > -----Original Message-----
> > > From: Bjorn Helgaas <[e-mail]>
> > > Sent: 2023年3月18日 6:25
> > > To: Hongxing Zhu <[e-mail]>
> > > Cc: Lorenzo Pieralisi <[e-mail]>;
> > > [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; dl-linux-imx
> > > <[e-mail]>
> > > Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI control of
> > > RC in suspend and resume
> > >
> > > On Fri, Mar 17, 2023 at 07:38:02AM +0000, Hongxing Zhu wrote:
> > > > > -----Original Message-----
> > > > > From: Lorenzo Pieralisi <[e-mail]>
> > > > > Sent: 2023年3月16日 16:11
> > > > > To: Hongxing Zhu <[e-mail]>
> > > > > Cc: Bjorn Helgaas <[e-mail]>; [e-mail];
> > > > > [e-mail]; [e-mail];
> > > > > [e-mail];
> > > > > [e-mail]; [e-mail];
> > > > > dl-linux-imx <[e-mail]>
> > > > > Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI control
> > > > > of RC in suspend and resume
> > > > >
> > > > > On Thu, Mar 16, 2023 at 07:37:41AM +0000, Hongxing Zhu wrote:
> > > > >
> > > > > [...]
> > > > >
> > > > > > > > > > It's not a separate register.
> > > > > > > > > >
> > > > > > > > > > The bit I manipulated is the MSI Enable bit of the
> > > > > > > > > > Message Control Register for MSI (Offset 02h)
> > > > > > > > > > contained in the MSI-capability of Root Complex.
> > > > > > > > > >
> > > > > > > > > > In addition, on i.MX6, the MSI Enable bit controls
> > > > > > > > > > delivery of MSI interrupts from components below the Root Port.
> > > > > > > > > >
> > > > > > > > > > So, set MSI Enable in imx6q-pcie to let the MSI from
> > > > > > > > > > downstream components works.
> > > > > > > > >
> > > > > > > > > My confusion is about this "MSI Capability" found by
> > > > > > > > > "dw_pcie_find_capability(pci, PCI_CAP_ID_MSI)" in your patch.
> > > > > > > > >
> > > > > > > > > The i.MX6 manual might refer to that as an "MSI Capability"
> > > > > > > > > but as far as I know, the PCIe base spec doesn't
> > > > > > > > > document a Root Complex MSI
> > > > > > > Capability.
> > > > > > > > >
> > > > > > > > > I don't think it's the same as the one documented in
> > > > > > > > > PCIe r6.0, sec 7.7.2.  I think it's different because:
> > > > > > > > >
> > > > > > > > >   (1) I *think* "pci" here refers to the RC, not to a Root Port.
> > > > > > > > >
> > > > > > > > >   (2) The semantics are different.  The MSI-X Enable bit in 7.7.2
> only
> > > > > > > > >   determines whether the Function itself is permitted to use
> MSI-X.
> > > > > > > > >   It has nothing to do with devices *below* a Root Port
> > > > > > > > > can use
> > > > > MSI-X.
> > > > > > > > >   It also has nothing to do with whether a Root Port can forward
> MSI
> > > > > > > > >   transactions from those downstream devices.
> > > > > > > > >
> > > > > > > > > This part of my confusion could be easily resolved via a comment.
> > > > > > > > >
> > > > > > > > > I do have a follow-on question, though: the patch seems
> > > > > > > > > to enable MSI-related functionality using a register in
> > > > > > > > > the DesignWare IP, not something in the i.MX6-specific
> > > > > > > > > IP.  If that's true, why don't other DesignWare-based
> > > > > > > > > drivers need something
> > > > > similar?
> > > > > > > > Hi Bjorn:
> > > > > > > > Thanks a lot for you reply.
> > > > > > > > This behavior is specific for i.MX PCIe.
> > > > > > >
> > > > > > > Which behaviour ? It can't be the root port MSI capability,
> > > > > > > that would be a HW bug (ie disabling root port MSIs would
> > > > > > > imply disabling MSIs for all downstream components).
> > > > > > >
> > > > > >
> > > > > > i.MX PCIe designer use this MSI_EN bit to control the MSI
> > > > > > trigger when integrate  Design Ware PCIe IP.
> > > > > > Without the MSI_EN bit assertion (1b'1), the devices below
> > > > > > this RC can't trigger  the MSI successfully.
> > > > > > Yes, you're right. It should not be the root port MSI capability.
> > > > >
> > > > > The question is, it is or it is not the root port MSI capability ?
> > > > >
> > > > > If it is, that's a HW bug.
> > > > >
> > > > > If it is not there is nothing to do and this patch can be merged.
> > > > Hi Lorenzo:
> > > > Thanks for your reply.
> > > > I think it is not the root port MSI capability actually.
> > > > Refer to my understands, designer just use the msi_en bit to
> > > > control the  delivery of MSI interrupts from components below the Root
> Port.
> > > > >
> > > > > > > > i.MX PCIe designer use this MSI_EN bit to control the MSI
> > > > > > > > trigger when integrate Design Ware PCIe IP.
> > > > > > >
> > > > > > > Fair enough but that can't be the MSI Enable bit in the Root
> > > > > > > Port MSI capability "Message Control" field I am afraid.
> > > > > > >
> > > > > > > It is what Bjorn mentioned quite clearly, a root complex
> > > > > > > configuration register dressed as an MSI capability, the
> > > > > > > root complex is not a PCI device; either that or that's an HW bug.
> > > > > > Yes, it is. I agree with you. Had report this situation to the design team.
> > > > > > Hope to correct this bug in HW design if it's possible.
> > > > >
> > > > > I don't understand if it is a HW bug or not, see above. I think
> > > > > it is legitimate to have MMIO register space that *looks* like
> > > > > an MSI capability for the root complex to control delivery of
> > > > > MSI interrupts, as long as it is not the actual root port MSI
> > > > > capability, in the root port PCI config space in which case this
> > > > > would be a HW
> > > bug from what you are reporting.
> > > > I just provide the following suggestions.
> > > > - Root complex shouldn't have the MSI capability refer to the PCIe Spec
> > > >   7.7.1 chapter.
> > > > - Root port MSIs should not imply disabling MSIs for all
> > > > downstream
> > > components.
> > >
> > > I think this is all a lot of confusion, mostly on my part, sorry about that.
> > >
> > > Root Complex configuration and behavior is not specified by the PCIe
> > > spec, so that's completely up to the i.MX designer.  It's fine for
> > > the Root Complex to have an MSI Capability, and it's fine for that
> > > capability to enable/disable the RC fielding of MSI MemWr
> > > transactions from downstream devices and triggering MSI interrupts.
> > >
> > > It's also fine for the RC MSI Capability to be identified with a
> > > Capability ID of 0x5, although it is slightly confusing to use
> > > PCI_CAP_ID_MSI to find it.  It's also slightly confusing to use the
> PCI_MSI_FLAGS offset into the RC MSI Capability.
> > >
> > > Using the PCI_CAP_ID_MSI, PCI_MSI_FLAGS, and PCI_MSI_FLAGS_ENABLE
> > > macros suggests to the reader that this RC MSI capability is the
> > > same as the the MSI Capability defined by PCIe r6.0, sec 7.7.1.
> > > Obviously it is *not* the same, because we're talking about a *Root
> > > Complex* capability, while the sec 7.7.1 capability can only appear
> > > on PCIe functions (Root Ports, Endpoints, Switch Ports, etc).
> > >
> > > I suggest a comment to the effect that this is a Root Complex MSI
> > > Capability, not the MSI Capability defined by PCIe r6.0, sec 7.7.1.
> > >
> > > Possibly even add new #defines in pci-imx6.c with different names,
> > > even though the values happen to be the same as the PCI_MSI_*
> > > #defines.  That would be a convenient place to put a comment about what
> they are.
> > Hi Bjorn:
> > Thanks a lot for your dispelling doubts.
> > How about to add the following comments in the new add function to clarify it?
> >
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -1036,6 +1036,18 @@ static void pci_imx_set_msi_en(struct dw_pcie
> *pci)
> >         u8 offset;
> >         u16 val;
> >
> > +       /*
> > +        * When i.MX DM PCIe controller is configured as RC mode, it has one
> > +        * MSI Capability Structure, although PCIe r6.0, sec 7.7.1 doesn't
> > +        * specify the MSI Capability Structures for Root Complex.
> 
> That's because a PCI root complex is not a PCI device (and this is not an MSI
> capability, which lives in PCI config space).
> 
> I will reword it (and the commit log with it) and merge it in the coming weeks for
> v6.4
Hi Lorenzo:
Thanks a lot for your kindly help.

Best Regards
Richard Zhu
> 
> Thanks,
> Lorenzo
> 
> > +        *
> > +        * The MSI_EN bit of MSI control register contained in this MSI-CAP
> > +        * is used control the MSI delivery of MSI interrupts from components
> > +        * below the Root Port.
> > +        *
> > +        * Find it by PCI_CAP_ID_MSI here, and assert the MSI_EN bit to
> allow
> > +        * the MSI delivery below the Root Port, if the PCI MSI is enabled.
> > +        */
> >         if (pci_msi_enabled()) {
> >                 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
> >                 dw_pcie_dbi_ro_wr_en(pci); Best Regards Richard Zhu
> > >
> > > Bjorn

Lorenzo Pieralisi April 5, 2023, 3:55 p.m. UTC | #15

[+Cc Sergey]

On Mon, Mar 27, 2023 at 12:22:04AM +0000, Hongxing Zhu wrote:
> > -----Original Message-----
> > From: Lorenzo Pieralisi <[e-mail]>
> > Sent: 2023年3月24日 23:59
> > To: Hongxing Zhu <[e-mail]>
> > Cc: Bjorn Helgaas <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; dl-linux-imx <[e-mail]>
> > Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI control of RC in suspend
> > and resume
> > 
> > On Mon, Mar 20, 2023 at 07:02:35AM +0000, Hongxing Zhu wrote:
> > > > -----Original Message-----
> > > > From: Bjorn Helgaas <[e-mail]>
> > > > Sent: 2023年3月18日 6:25
> > > > To: Hongxing Zhu <[e-mail]>
> > > > Cc: Lorenzo Pieralisi <[e-mail]>;
> > > > [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail]; dl-linux-imx
> > > > <[e-mail]>
> > > > Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI control of
> > > > RC in suspend and resume
> > > >
> > > > On Fri, Mar 17, 2023 at 07:38:02AM +0000, Hongxing Zhu wrote:
> > > > > > -----Original Message-----
> > > > > > From: Lorenzo Pieralisi <[e-mail]>
> > > > > > Sent: 2023年3月16日 16:11
> > > > > > To: Hongxing Zhu <[e-mail]>
> > > > > > Cc: Bjorn Helgaas <[e-mail]>; [e-mail];
> > > > > > [e-mail]; [e-mail];
> > > > > > [e-mail];
> > > > > > [e-mail]; [e-mail];
> > > > > > dl-linux-imx <[e-mail]>
> > > > > > Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI control
> > > > > > of RC in suspend and resume
> > > > > >
> > > > > > On Thu, Mar 16, 2023 at 07:37:41AM +0000, Hongxing Zhu wrote:
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > > > > > It's not a separate register.
> > > > > > > > > > >
> > > > > > > > > > > The bit I manipulated is the MSI Enable bit of the
> > > > > > > > > > > Message Control Register for MSI (Offset 02h)
> > > > > > > > > > > contained in the MSI-capability of Root Complex.
> > > > > > > > > > >
> > > > > > > > > > > In addition, on i.MX6, the MSI Enable bit controls
> > > > > > > > > > > delivery of MSI interrupts from components below the Root Port.
> > > > > > > > > > >
> > > > > > > > > > > So, set MSI Enable in imx6q-pcie to let the MSI from
> > > > > > > > > > > downstream components works.
> > > > > > > > > >
> > > > > > > > > > My confusion is about this "MSI Capability" found by
> > > > > > > > > > "dw_pcie_find_capability(pci, PCI_CAP_ID_MSI)" in your patch.
> > > > > > > > > >
> > > > > > > > > > The i.MX6 manual might refer to that as an "MSI Capability"
> > > > > > > > > > but as far as I know, the PCIe base spec doesn't
> > > > > > > > > > document a Root Complex MSI
> > > > > > > > Capability.
> > > > > > > > > >
> > > > > > > > > > I don't think it's the same as the one documented in
> > > > > > > > > > PCIe r6.0, sec 7.7.2.  I think it's different because:
> > > > > > > > > >
> > > > > > > > > >   (1) I *think* "pci" here refers to the RC, not to a Root Port.
> > > > > > > > > >
> > > > > > > > > >   (2) The semantics are different.  The MSI-X Enable bit in 7.7.2
> > only
> > > > > > > > > >   determines whether the Function itself is permitted to use
> > MSI-X.
> > > > > > > > > >   It has nothing to do with devices *below* a Root Port
> > > > > > > > > > can use
> > > > > > MSI-X.
> > > > > > > > > >   It also has nothing to do with whether a Root Port can forward
> > MSI
> > > > > > > > > >   transactions from those downstream devices.
> > > > > > > > > >
> > > > > > > > > > This part of my confusion could be easily resolved via a comment.
> > > > > > > > > >
> > > > > > > > > > I do have a follow-on question, though: the patch seems
> > > > > > > > > > to enable MSI-related functionality using a register in
> > > > > > > > > > the DesignWare IP, not something in the i.MX6-specific
> > > > > > > > > > IP.  If that's true, why don't other DesignWare-based
> > > > > > > > > > drivers need something
> > > > > > similar?
> > > > > > > > > Hi Bjorn:
> > > > > > > > > Thanks a lot for you reply.
> > > > > > > > > This behavior is specific for i.MX PCIe.
> > > > > > > >
> > > > > > > > Which behaviour ? It can't be the root port MSI capability,
> > > > > > > > that would be a HW bug (ie disabling root port MSIs would
> > > > > > > > imply disabling MSIs for all downstream components).
> > > > > > > >
> > > > > > >
> > > > > > > i.MX PCIe designer use this MSI_EN bit to control the MSI
> > > > > > > trigger when integrate  Design Ware PCIe IP.
> > > > > > > Without the MSI_EN bit assertion (1b'1), the devices below
> > > > > > > this RC can't trigger  the MSI successfully.
> > > > > > > Yes, you're right. It should not be the root port MSI capability.
> > > > > >
> > > > > > The question is, it is or it is not the root port MSI capability ?
> > > > > >
> > > > > > If it is, that's a HW bug.
> > > > > >
> > > > > > If it is not there is nothing to do and this patch can be merged.
> > > > > Hi Lorenzo:
> > > > > Thanks for your reply.
> > > > > I think it is not the root port MSI capability actually.
> > > > > Refer to my understands, designer just use the msi_en bit to
> > > > > control the  delivery of MSI interrupts from components below the Root
> > Port.
> > > > > >
> > > > > > > > > i.MX PCIe designer use this MSI_EN bit to control the MSI
> > > > > > > > > trigger when integrate Design Ware PCIe IP.
> > > > > > > >
> > > > > > > > Fair enough but that can't be the MSI Enable bit in the Root
> > > > > > > > Port MSI capability "Message Control" field I am afraid.
> > > > > > > >
> > > > > > > > It is what Bjorn mentioned quite clearly, a root complex
> > > > > > > > configuration register dressed as an MSI capability, the
> > > > > > > > root complex is not a PCI device; either that or that's an HW bug.
> > > > > > > Yes, it is. I agree with you. Had report this situation to the design team.
> > > > > > > Hope to correct this bug in HW design if it's possible.
> > > > > >
> > > > > > I don't understand if it is a HW bug or not, see above. I think
> > > > > > it is legitimate to have MMIO register space that *looks* like
> > > > > > an MSI capability for the root complex to control delivery of
> > > > > > MSI interrupts, as long as it is not the actual root port MSI
> > > > > > capability, in the root port PCI config space in which case this
> > > > > > would be a HW
> > > > bug from what you are reporting.
> > > > > I just provide the following suggestions.
> > > > > - Root complex shouldn't have the MSI capability refer to the PCIe Spec
> > > > >   7.7.1 chapter.
> > > > > - Root port MSIs should not imply disabling MSIs for all
> > > > > downstream
> > > > components.
> > > >
> > > > I think this is all a lot of confusion, mostly on my part, sorry about that.
> > > >
> > > > Root Complex configuration and behavior is not specified by the PCIe
> > > > spec, so that's completely up to the i.MX designer.  It's fine for
> > > > the Root Complex to have an MSI Capability, and it's fine for that
> > > > capability to enable/disable the RC fielding of MSI MemWr
> > > > transactions from downstream devices and triggering MSI interrupts.
> > > >
> > > > It's also fine for the RC MSI Capability to be identified with a
> > > > Capability ID of 0x5, although it is slightly confusing to use
> > > > PCI_CAP_ID_MSI to find it.  It's also slightly confusing to use the
> > PCI_MSI_FLAGS offset into the RC MSI Capability.
> > > >
> > > > Using the PCI_CAP_ID_MSI, PCI_MSI_FLAGS, and PCI_MSI_FLAGS_ENABLE
> > > > macros suggests to the reader that this RC MSI capability is the
> > > > same as the the MSI Capability defined by PCIe r6.0, sec 7.7.1.
> > > > Obviously it is *not* the same, because we're talking about a *Root
> > > > Complex* capability, while the sec 7.7.1 capability can only appear
> > > > on PCIe functions (Root Ports, Endpoints, Switch Ports, etc).
> > > >
> > > > I suggest a comment to the effect that this is a Root Complex MSI
> > > > Capability, not the MSI Capability defined by PCIe r6.0, sec 7.7.1.
> > > >
> > > > Possibly even add new #defines in pci-imx6.c with different names,
> > > > even though the values happen to be the same as the PCI_MSI_*
> > > > #defines.  That would be a convenient place to put a comment about what
> > they are.
> > > Hi Bjorn:
> > > Thanks a lot for your dispelling doubts.
> > > How about to add the following comments in the new add function to clarify it?
> > >
> > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > @@ -1036,6 +1036,18 @@ static void pci_imx_set_msi_en(struct dw_pcie
> > *pci)
> > >         u8 offset;
> > >         u16 val;
> > >
> > > +       /*
> > > +        * When i.MX DM PCIe controller is configured as RC mode, it has one
> > > +        * MSI Capability Structure, although PCIe r6.0, sec 7.7.1 doesn't
> > > +        * specify the MSI Capability Structures for Root Complex.
> > 
> > That's because a PCI root complex is not a PCI device (and this is not an MSI
> > capability, which lives in PCI config space).
> > 
> > I will reword it (and the commit log with it) and merge it in the coming weeks for
> > v6.4
> Hi Lorenzo:
> Thanks a lot for your kindly help.

I am getting back to this since I am still not convinced and I want to
understand this once for all.

We do use dw_pcie_find_capability() in most DWC drivers to find and
peek/poke at eg PCI express capability of the *Root port* (?),

eg dw_pcie_wait_for_link()

so I assume that for iMX6 dw_pcie_find_capability() does just the same,
which would mean that we are poking the "Message Control" field of the
Root port MSI capability.

Either that (which would mean that iMX6 has a HW bug because the RP
Message Control field does not control the delivery of MSIs from
endpoints but just for the root port itself ) or all DWC controllers
modelled the root complex MMIO space as a set of PCI/PCIe capabilities
that are NOT necessarily mappable to PCI specifications defined ones.

Can anyone please shed some light on this ? I don't have DWC HW, we need
to know before merging this code.

Thanks,
Lorenzo

> 
> Best Regards
> Richard Zhu
> > 
> > Thanks,
> > Lorenzo
> > 
> > > +        *
> > > +        * The MSI_EN bit of MSI control register contained in this MSI-CAP
> > > +        * is used control the MSI delivery of MSI interrupts from components
> > > +        * below the Root Port.
> > > +        *
> > > +        * Find it by PCI_CAP_ID_MSI here, and assert the MSI_EN bit to
> > allow
> > > +        * the MSI delivery below the Root Port, if the PCI MSI is enabled.
> > > +        */
> > >         if (pci_msi_enabled()) {
> > >                 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
> > >                 dw_pcie_dbi_ro_wr_en(pci); Best Regards Richard Zhu
> > > >
> > > > Bjorn

Hongxing Zhu April 10, 2023, 6:48 a.m. UTC | #16

> -----Original Message-----
> From: Lorenzo Pieralisi <[e-mail]>
> Sent: 2023年4月5日 23:56
> To: Hongxing Zhu <[e-mail]>
> Cc: Bjorn Helgaas <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; dl-linux-imx <[e-mail]>; Serge Semin
> <[e-mail]>
> Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI control of RC in suspend
> and resume
> 
> [+Cc Sergey]
> 
> On Mon, Mar 27, 2023 at 12:22:04AM +0000, Hongxing Zhu wrote:
> > > -----Original Message-----
> > > From: Lorenzo Pieralisi <[e-mail]>
> > > Sent: 2023年3月24日 23:59
> > > To: Hongxing Zhu <[e-mail]>
> > > Cc: Bjorn Helgaas <[e-mail]>; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; dl-linux-imx <[e-mail]>
> > > Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI control of
> > > RC in suspend and resume
> > >
> > > On Mon, Mar 20, 2023 at 07:02:35AM +0000, Hongxing Zhu wrote:
> > > > > -----Original Message-----
> > > > > From: Bjorn Helgaas <[e-mail]>
> > > > > Sent: 2023年3月18日 6:25
> > > > > To: Hongxing Zhu <[e-mail]>
> > > > > Cc: Lorenzo Pieralisi <[e-mail]>;
> > > > > [e-mail]; [e-mail];
> > > > > [e-mail]; [e-mail];
> > > > > [e-mail]; [e-mail];
> > > > > dl-linux-imx <[e-mail]>
> > > > > Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI control
> > > > > of RC in suspend and resume
> > > > >
> > > > > On Fri, Mar 17, 2023 at 07:38:02AM +0000, Hongxing Zhu wrote:
> > > > > > > -----Original Message-----
> > > > > > > From: Lorenzo Pieralisi <[e-mail]>
> > > > > > > Sent: 2023年3月16日 16:11
> > > > > > > To: Hongxing Zhu <[e-mail]>
> > > > > > > Cc: Bjorn Helgaas <[e-mail]>;
> > > > > > > [e-mail]; [e-mail];
> > > > > > > [e-mail];
> > > > > > > [e-mail];
> > > > > > > [e-mail]; [e-mail];
> > > > > > > dl-linux-imx <[e-mail]>
> > > > > > > Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI
> > > > > > > control of RC in suspend and resume
> > > > > > >
> > > > > > > On Thu, Mar 16, 2023 at 07:37:41AM +0000, Hongxing Zhu wrote:
> > > > > > >
> > > > > > > [...]
> > > > > > >
> > > > > > > > > > > > It's not a separate register.
> > > > > > > > > > > >
> > > > > > > > > > > > The bit I manipulated is the MSI Enable bit of the
> > > > > > > > > > > > Message Control Register for MSI (Offset 02h)
> > > > > > > > > > > > contained in the MSI-capability of Root Complex.
> > > > > > > > > > > >
> > > > > > > > > > > > In addition, on i.MX6, the MSI Enable bit controls
> > > > > > > > > > > > delivery of MSI interrupts from components below the Root
> Port.
> > > > > > > > > > > >
> > > > > > > > > > > > So, set MSI Enable in imx6q-pcie to let the MSI
> > > > > > > > > > > > from downstream components works.
> > > > > > > > > > >
> > > > > > > > > > > My confusion is about this "MSI Capability" found by
> > > > > > > > > > > "dw_pcie_find_capability(pci, PCI_CAP_ID_MSI)" in your patch.
> > > > > > > > > > >
> > > > > > > > > > > The i.MX6 manual might refer to that as an "MSI Capability"
> > > > > > > > > > > but as far as I know, the PCIe base spec doesn't
> > > > > > > > > > > document a Root Complex MSI
> > > > > > > > > Capability.
> > > > > > > > > > >
> > > > > > > > > > > I don't think it's the same as the one documented in
> > > > > > > > > > > PCIe r6.0, sec 7.7.2.  I think it's different because:
> > > > > > > > > > >
> > > > > > > > > > >   (1) I *think* "pci" here refers to the RC, not to a Root Port.
> > > > > > > > > > >
> > > > > > > > > > >   (2) The semantics are different.  The MSI-X Enable
> > > > > > > > > > > bit in 7.7.2
> > > only
> > > > > > > > > > >   determines whether the Function itself is
> > > > > > > > > > > permitted to use
> > > MSI-X.
> > > > > > > > > > >   It has nothing to do with devices *below* a Root
> > > > > > > > > > > Port can use
> > > > > > > MSI-X.
> > > > > > > > > > >   It also has nothing to do with whether a Root Port
> > > > > > > > > > > can forward
> > > MSI
> > > > > > > > > > >   transactions from those downstream devices.
> > > > > > > > > > >
> > > > > > > > > > > This part of my confusion could be easily resolved via a
> comment.
> > > > > > > > > > >
> > > > > > > > > > > I do have a follow-on question, though: the patch
> > > > > > > > > > > seems to enable MSI-related functionality using a
> > > > > > > > > > > register in the DesignWare IP, not something in the
> > > > > > > > > > > i.MX6-specific IP.  If that's true, why don't other
> > > > > > > > > > > DesignWare-based drivers need something
> > > > > > > similar?
> > > > > > > > > > Hi Bjorn:
> > > > > > > > > > Thanks a lot for you reply.
> > > > > > > > > > This behavior is specific for i.MX PCIe.
> > > > > > > > >
> > > > > > > > > Which behaviour ? It can't be the root port MSI
> > > > > > > > > capability, that would be a HW bug (ie disabling root
> > > > > > > > > port MSIs would imply disabling MSIs for all downstream
> components).
> > > > > > > > >
> > > > > > > >
> > > > > > > > i.MX PCIe designer use this MSI_EN bit to control the MSI
> > > > > > > > trigger when integrate  Design Ware PCIe IP.
> > > > > > > > Without the MSI_EN bit assertion (1b'1), the devices below
> > > > > > > > this RC can't trigger  the MSI successfully.
> > > > > > > > Yes, you're right. It should not be the root port MSI capability.
> > > > > > >
> > > > > > > The question is, it is or it is not the root port MSI capability ?
> > > > > > >
> > > > > > > If it is, that's a HW bug.
> > > > > > >
> > > > > > > If it is not there is nothing to do and this patch can be merged.
> > > > > > Hi Lorenzo:
> > > > > > Thanks for your reply.
> > > > > > I think it is not the root port MSI capability actually.
> > > > > > Refer to my understands, designer just use the msi_en bit to
> > > > > > control the  delivery of MSI interrupts from components below
> > > > > > the Root
> > > Port.
> > > > > > >
> > > > > > > > > > i.MX PCIe designer use this MSI_EN bit to control the
> > > > > > > > > > MSI trigger when integrate Design Ware PCIe IP.
> > > > > > > > >
> > > > > > > > > Fair enough but that can't be the MSI Enable bit in the
> > > > > > > > > Root Port MSI capability "Message Control" field I am afraid.
> > > > > > > > >
> > > > > > > > > It is what Bjorn mentioned quite clearly, a root complex
> > > > > > > > > configuration register dressed as an MSI capability, the
> > > > > > > > > root complex is not a PCI device; either that or that's an HW bug.
> > > > > > > > Yes, it is. I agree with you. Had report this situation to the design
> team.
> > > > > > > > Hope to correct this bug in HW design if it's possible.
> > > > > > >
> > > > > > > I don't understand if it is a HW bug or not, see above. I
> > > > > > > think it is legitimate to have MMIO register space that
> > > > > > > *looks* like an MSI capability for the root complex to
> > > > > > > control delivery of MSI interrupts, as long as it is not the
> > > > > > > actual root port MSI capability, in the root port PCI config
> > > > > > > space in which case this would be a HW
> > > > > bug from what you are reporting.
> > > > > > I just provide the following suggestions.
> > > > > > - Root complex shouldn't have the MSI capability refer to the PCIe Spec
> > > > > >   7.7.1 chapter.
> > > > > > - Root port MSIs should not imply disabling MSIs for all
> > > > > > downstream
> > > > > components.
> > > > >
> > > > > I think this is all a lot of confusion, mostly on my part, sorry about that.
> > > > >
> > > > > Root Complex configuration and behavior is not specified by the
> > > > > PCIe spec, so that's completely up to the i.MX designer.  It's
> > > > > fine for the Root Complex to have an MSI Capability, and it's
> > > > > fine for that capability to enable/disable the RC fielding of
> > > > > MSI MemWr transactions from downstream devices and triggering MSI
> interrupts.
> > > > >
> > > > > It's also fine for the RC MSI Capability to be identified with a
> > > > > Capability ID of 0x5, although it is slightly confusing to use
> > > > > PCI_CAP_ID_MSI to find it.  It's also slightly confusing to use
> > > > > the
> > > PCI_MSI_FLAGS offset into the RC MSI Capability.
> > > > >
> > > > > Using the PCI_CAP_ID_MSI, PCI_MSI_FLAGS, and
> > > > > PCI_MSI_FLAGS_ENABLE macros suggests to the reader that this RC
> > > > > MSI capability is the same as the the MSI Capability defined by PCIe r6.0,
> sec 7.7.1.
> > > > > Obviously it is *not* the same, because we're talking about a
> > > > > *Root
> > > > > Complex* capability, while the sec 7.7.1 capability can only
> > > > > appear on PCIe functions (Root Ports, Endpoints, Switch Ports, etc).
> > > > >
> > > > > I suggest a comment to the effect that this is a Root Complex
> > > > > MSI Capability, not the MSI Capability defined by PCIe r6.0, sec 7.7.1.
> > > > >
> > > > > Possibly even add new #defines in pci-imx6.c with different
> > > > > names, even though the values happen to be the same as the
> > > > > PCI_MSI_* #defines.  That would be a convenient place to put a
> > > > > comment about what
> > > they are.
> > > > Hi Bjorn:
> > > > Thanks a lot for your dispelling doubts.
> > > > How about to add the following comments in the new add function to
> clarify it?
> > > >
> > > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > > @@ -1036,6 +1036,18 @@ static void pci_imx_set_msi_en(struct
> > > > dw_pcie
> > > *pci)
> > > >         u8 offset;
> > > >         u16 val;
> > > >
> > > > +       /*
> > > > +        * When i.MX DM PCIe controller is configured as RC mode, it has
> one
> > > > +        * MSI Capability Structure, although PCIe r6.0, sec 7.7.1 doesn't
> > > > +        * specify the MSI Capability Structures for Root Complex.
> > >
> > > That's because a PCI root complex is not a PCI device (and this is
> > > not an MSI capability, which lives in PCI config space).
> > >
> > > I will reword it (and the commit log with it) and merge it in the
> > > coming weeks for
> > > v6.4
> > Hi Lorenzo:
> > Thanks a lot for your kindly help.
> 
> I am getting back to this since I am still not convinced and I want to understand
> this once for all.
> 
> We do use dw_pcie_find_capability() in most DWC drivers to find and peek/poke
> at eg PCI express capability of the *Root port* (?),
> 
> eg dw_pcie_wait_for_link()
> 
> so I assume that for iMX6 dw_pcie_find_capability() does just the same, which
> would mean that we are poking the "Message Control" field of the Root port MSI
> capability.
> 
> Either that (which would mean that iMX6 has a HW bug because the RP Message
> Control field does not control the delivery of MSIs from endpoints but just for the
> root port itself ) or all DWC controllers modelled the root complex MMIO space as
> a set of PCI/PCIe capabilities that are NOT necessarily mappable to PCI
> specifications defined ones.
> 
> Can anyone please shed some light on this ? I don't have DWC HW, we need to
> know before merging this code.
Hi Lorenzo:
Regarding my understanding, DWC HW has the PCI/PCIe capability map when
 it works in RC mode and Spec doesn’t specify these Caps for host controller.
And, there are comments describe these callbacks already in pcie-designware.c.
...
/*
 * These interfaces resemble the pci_find_*capability() interfaces, but these
 * are for configuring host controllers, which are bridges *to* PCI devices but
 * are not PCI devices themselves.
 */
static u8 __dw_pcie_find_next_cap(struct dw_pcie *pci, u8 cap_ptr,
                                  u8 cap)
...


Best Regards
Richard Zhu
> 
> Thanks,
> Lorenzo
> 
> >
> > Best Regards
> > Richard Zhu
> > >
> > > Thanks,
> > > Lorenzo
> > >
> > > > +        *
> > > > +        * The MSI_EN bit of MSI control register contained in this
> MSI-CAP
> > > > +        * is used control the MSI delivery of MSI interrupts from
> components
> > > > +        * below the Root Port.
> > > > +        *
> > > > +        * Find it by PCI_CAP_ID_MSI here, and assert the MSI_EN
> > > > + bit to
> > > allow
> > > > +        * the MSI delivery below the Root Port, if the PCI MSI is enabled.
> > > > +        */
> > > >         if (pci_msi_enabled()) {
> > > >                 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
> > > >                 dw_pcie_dbi_ro_wr_en(pci); Best Regards Richard
> > > > Zhu
> > > > >
> > > > > Bjorn

Serge Semin April 11, 2023, 2:36 a.m. UTC | #17

On Wed, Apr 05, 2023 at 05:55:48PM +0200, Lorenzo Pieralisi wrote:
> [+Cc Sergey]
> 

Hi Lorenzo

[nip]

> > > 
> > > That's because a PCI root complex is not a PCI device (and this is not an MSI
> > > capability, which lives in PCI config space).
> > > 
> > > I will reword it (and the commit log with it) and merge it in the coming weeks for
> > > v6.4
> > Hi Lorenzo:
> > Thanks a lot for your kindly help.
> 
> I am getting back to this since I am still not convinced and I want to
> understand this once for all.
> 
> We do use dw_pcie_find_capability() in most DWC drivers to find and
> peek/poke at eg PCI express capability of the *Root port* (?),
> 
> eg dw_pcie_wait_for_link()
> 
> so I assume that for iMX6 dw_pcie_find_capability() does just the same,
> which would mean that we are poking the "Message Control" field of the
> Root port MSI capability.

Absolutely right. Besides of the standard PCIe config space which is
filled with the various PCIe capability registers, the DW PCIe Root
Port has the "Port Logic" CSR space with the DW PCIe-specific
settings: GENx Link control and lanes tuning stuff, iMSI-Rx engine,
iATU/eDMA, etc. Some of the capabilities are utilized by their direct
functionality (like the PCI Express capability) meanwhile some are
left unused even though they can be activated during the IP-core
synthesize stage. I don't know why the HW designers left them being
optionally enabled and then available for some setups, DW PCIe RC
HW-manual doesn't have their functionality description. The MSI
capability is one of such PCIe capabilities. It's described in the DW
PCIe RC registers space HW-manual and the DW PCIe RC IP-core
parameters, but the rest of the manual text has nothing about it.

Also note even though the DW PCIe RC HW-manual has _RC_ in the name it
actually describes the PCIe Root Port with the attention remark: "The
Synopsys PCI Express IP does not implement a full root complex."

So from what I can see there can be several reasons of such
configuration:
1. It simplifies the dual-mode device design for the HW-engineers.
Though IMO Synopsys at least should have dropped the unused
capabilities from the RP HW-manual.
2. Synopsys may imply that these capabilities can be used somehow
for the vendor-specific true Root Complex designs. Though they don't
specifically say about that. Instead there is a note: "To implement a
full root complex, you must add your application logic (connected to
the application client interface or AXI interface) and other support
logic (including clock and reset generation)."

> 
> Either that (which would mean that iMX6 has a HW bug because the RP
> Message Control field does not control the delivery of MSIs from
> endpoints but just for the root port itself )

I wouldn't call it a HW bug seeing the MSI capability registers are
actually unused by the RP controllers. So it looks more like the
iMX6-specific feature. See a short explanation in the commit
75cb8d20c112 ("PCI: imx: Enable MSI from downstream components").

Normally MSI event delivery to the CPU doesn't depend on the actual
MSI capability CSRs state in DW PCIe RPs. Instead the MSI is
configured via the Port Logic iMSI-RX registers. That's what is done
in the DW PCIe host driver (see pcie-designware-host.c).

> or all DWC controllers
> modelled the root complex MMIO space as a set of PCI/PCIe capabilities
> that are NOT necessarily mappable to PCI specifications defined ones.

Basically you are right though as I noted above Synopsys doesn't
provide a complete RC solution. It's just a single RP with the MMIO
(DBI) CSRs looking as the PCIe TYPE1 config space.

-Serge(y)

> 
> Can anyone please shed some light on this ? I don't have DWC HW, we need
> to know before merging this code.
> 
> Thanks,
> Lorenzo
> 
> > 
> > Best Regards
> > Richard Zhu
> > > 
> > > Thanks,
> > > Lorenzo
> > > 
> > > > +        *
> > > > +        * The MSI_EN bit of MSI control register contained in this MSI-CAP
> > > > +        * is used control the MSI delivery of MSI interrupts from components
> > > > +        * below the Root Port.
> > > > +        *
> > > > +        * Find it by PCI_CAP_ID_MSI here, and assert the MSI_EN bit to
> > > allow
> > > > +        * the MSI delivery below the Root Port, if the PCI MSI is enabled.
> > > > +        */
> > > >         if (pci_msi_enabled()) {
> > > >                 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
> > > >                 dw_pcie_dbi_ro_wr_en(pci); Best Regards Richard Zhu
> > > > >
> > > > > Bjorn
>

Lorenzo Pieralisi June 19, 2023, 9:07 a.m. UTC | #18

On Mon, Apr 10, 2023 at 06:48:48AM +0000, Hongxing Zhu wrote:

[...]

> > I am getting back to this since I am still not convinced and I want to understand
> > this once for all.
> > 
> > We do use dw_pcie_find_capability() in most DWC drivers to find and peek/poke
> > at eg PCI express capability of the *Root port* (?),
> > 
> > eg dw_pcie_wait_for_link()
> > 
> > so I assume that for iMX6 dw_pcie_find_capability() does just the same, which
> > would mean that we are poking the "Message Control" field of the Root port MSI
> > capability.
> > 
> > Either that (which would mean that iMX6 has a HW bug because the RP Message
> > Control field does not control the delivery of MSIs from endpoints but just for the
> > root port itself ) or all DWC controllers modelled the root complex MMIO space as
> > a set of PCI/PCIe capabilities that are NOT necessarily mappable to PCI
> > specifications defined ones.
> > 
> > Can anyone please shed some light on this ? I don't have DWC HW, we need to
> > know before merging this code.
> Hi Lorenzo:
> Regarding my understanding, DWC HW has the PCI/PCIe capability map when
>  it works in RC mode and Spec doesn’t specify these Caps for host controller.
> And, there are comments describe these callbacks already in pcie-designware.c.
> ...
> /*
>  * These interfaces resemble the pci_find_*capability() interfaces, but these
>  * are for configuring host controllers, which are bridges *to* PCI devices but
>  * are not PCI devices themselves.
>  */
> static u8 __dw_pcie_find_next_cap(struct dw_pcie *pci, u8 cap_ptr,
>                                   u8 cap)
> ...
> 
> 

I still believe this is an integration bug, more so after reading the
commit Serge pointed out:

75cb8d20c112 ("PCI: imx: Enable MSI from downstream components").

The commit above implies that if you have CONFIG_PCIEPORTBUS enabled,
you would not need to set the MSI enable bit explicitly because that's
done by the port driver while requesting RP services.

This means that it is _seen_ by the PCI core as a capability register
and it also means that if you have CONFIG_PCIEPORTBUS enabled and that
the port driver disables MSIs, all downstream MSIs are disabled, not
only the RP ones (as it should be according to the PCI specs).

So, this is a HW bug I am afraid - I will merge this patch but AFAICS
the HW integration bug is there regardless, however we slice it.

Thanks,
Lorenzo

Lorenzo Pieralisi June 19, 2023, 9:38 a.m. UTC | #19

On Thu, 08 Dec 2022 14:05:34 +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.
> 
> 
> [...]

Applied to controller/dwc (commit log rewritten - please check), thanks!

[1/1] PCI: imx6: Save and restore MSI control of RC in suspend and resume
      https://git.kernel.org/pci/pci/c/3bbc3c72c4b8

Thanks,
Lorenzo

Patch

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)