Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 30 Dec 2022 16:06:24 +0100
From:   Lorenzo Pieralisi <lpieralisi@kernel.org>
To:     Richard Zhu <hongxing.zhu@nxp.com>, l.stach@pengutronix.de,
        bhelgaas@google.com
Cc:     linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, kernel@pengutronix.de,
        linux-imx@nxp.com
Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI control of RC in
 suspend and resume
Message-ID: <Y67+cH15frjqK2vA@lpieralisi>
References: <1670479534-22154-1-git-send-email-hongxing.zhu@nxp.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1670479534-22154-1-git-send-email-hongxing.zhu@nxp.com>
Precedence: bulk

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 <hongxing.zhu@nxp.com>
> ---
> Changes v1-->v2:
> New create one save/restore function, used save the setting in suspend and
> restore the configuration in resume.
> v1 https://patchwork.kernel.org/project/linux-pci/patch/1667289595-12440-1-git-send-email-hongxing.zhu@nxp.com/
> 
> ---
>  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
>