PCI: mediatek: Add system pm support for MT2712

Message ID 1527647736-19986-1-git-send-email-honghui.zhang@mediatek.com
State New
Delegated to: Lorenzo Pieralisi
Headers show
Series
  • PCI: mediatek: Add system pm support for MT2712
Related show

Commit Message

Honghui Zhang May 30, 2018, 2:35 a.m.
From: Honghui Zhang <honghui.zhang@mediatek.com>

The MTCMOS of PCIe Host for MT2712 will be off when system suspend, and all
the internel control register will be reset after system resume. The PCIe
link should be re-established and the related control register values should
be re-set after system resume.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
CC: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/host/pcie-mediatek.c | 82 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

Comments

Ryder Lee May 31, 2018, 2:05 a.m. | #1
On Wed, 2018-05-30 at 10:35 +0800, honghui.zhang@mediatek.com wrote:
> From: Honghui Zhang <honghui.zhang@mediatek.com>
> 
> The MTCMOS of PCIe Host for MT2712 will be off when system suspend, and all
> the internel control register will be reset after system resume. The PCIe
> link should be re-established and the related control register values should
> be re-set after system resume.
> 
> Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
> CC: Ryder Lee <ryder.lee@mediatek.com>
> ---
>  drivers/pci/host/pcie-mediatek.c | 82 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 82 insertions(+)
> 
> diff --git a/drivers/pci/host/pcie-mediatek.c b/drivers/pci/host/pcie-mediatek.c
> index dabf1086..60f98d92 100644
> --- a/drivers/pci/host/pcie-mediatek.c
> +++ b/drivers/pci/host/pcie-mediatek.c
> @@ -132,12 +132,14 @@ struct mtk_pcie_port;
>  /**
>   * struct mtk_pcie_soc - differentiate between host generations
>   * @need_fix_class_id: whether this host's class ID needed to be fixed or not
> + * @pm_support: whether the host's MTCMOS will be off when suspend
>   * @ops: pointer to configuration access functions
>   * @startup: pointer to controller setting functions
>   * @setup_irq: pointer to initialize IRQ functions
>   */
>  struct mtk_pcie_soc {
>  	bool need_fix_class_id;
> +	bool pm_support;
>  	struct pci_ops *ops;
>  	int (*startup)(struct mtk_pcie_port *port);
>  	int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
> @@ -1179,12 +1181,91 @@ static int mtk_pcie_probe(struct platform_device *pdev)
>  	return err;
>  }
>  
> +static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
> +{
> +	struct platform_device *pdev;
> +	struct mtk_pcie *pcie;
> +	struct mtk_pcie_port *port;
> +	const struct mtk_pcie_soc *soc;
> +
> +	pdev = to_platform_device(dev);
> +	pcie = platform_get_drvdata(pdev);

How about this -
struct mtk_pcie *pcie = dev_get_drvdata(dev);

> +	soc = pcie->soc;
> +	if (!soc->pm_support)
> +		return 0;
> +
> +	list_for_each_entry(port, &pcie->ports, list) {
> +		clk_disable_unprepare(port->ahb_ck);
> +		clk_disable_unprepare(port->sys_ck);
> +		phy_power_off(port->phy);
> +	}
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
> +{
> +	struct platform_device *pdev;
> +	struct mtk_pcie *pcie;
> +	struct mtk_pcie_port *port;
> +	const struct mtk_pcie_soc *soc;
> +	int ret;
> +
> +	pdev = to_platform_device(dev);
> +	pcie = platform_get_drvdata(pdev);

struct mtk_pcie *pcie = dev_get_drvdata(dev);

> +	soc = pcie->soc;
> +	if (!soc->pm_support)
> +		return 0;
> +
> +	list_for_each_entry(port, &pcie->ports, list) {
> +		ret = phy_power_on(port->phy);
> +		if (ret) {
> +			dev_err(dev, "could not power on phy\n");
> +			return ret;
> +		}
> +		ret = clk_prepare_enable(port->sys_ck);
> +		if (ret) {
> +			dev_err(dev, "enable sys clock error\n");
> +			phy_power_off(port->phy);
> +			return ret;
> +		}
> +
> +		ret = clk_prepare_enable(port->ahb_ck);
> +		if (ret) {
> +			dev_err(dev, "enable ahb clock error\n");
> +			phy_power_off(port->phy);
> +			clk_disable_unprepare(port->sys_ck);
> +			return ret;
> +		}
> +
> +		ret = soc->startup(port);
> +		if (ret) {
> +			dev_err(dev, "pcie linkup failed\n");
> +			phy_power_off(port->phy);
> +			clk_disable_unprepare(port->sys_ck);
> +			clk_disable_unprepare(port->ahb_ck);
> +			return ret;
> +		}
> +
> +		if (IS_ENABLED(CONFIG_PCI_MSI))
> +			mtk_pcie_enable_msi(port);
> +	}
> +
> +	return 0;
> +}
> +
Honghui Zhang May 31, 2018, 2:19 a.m. | #2
On Thu, 2018-05-31 at 10:05 +0800, Ryder Lee wrote:
> On Wed, 2018-05-30 at 10:35 +0800, honghui.zhang@mediatek.com wrote:
> > From: Honghui Zhang <honghui.zhang@mediatek.com>
> > 
> > The MTCMOS of PCIe Host for MT2712 will be off when system suspend, and all
> > the internel control register will be reset after system resume. The PCIe
> > link should be re-established and the related control register values should
> > be re-set after system resume.
> > 
> > Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
> > CC: Ryder Lee <ryder.lee@mediatek.com>
> > ---
> >  drivers/pci/host/pcie-mediatek.c | 82 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 82 insertions(+)
> > 
> > diff --git a/drivers/pci/host/pcie-mediatek.c b/drivers/pci/host/pcie-mediatek.c
> > index dabf1086..60f98d92 100644
> > --- a/drivers/pci/host/pcie-mediatek.c
> > +++ b/drivers/pci/host/pcie-mediatek.c
> > @@ -132,12 +132,14 @@ struct mtk_pcie_port;
> >  /**
> >   * struct mtk_pcie_soc - differentiate between host generations
> >   * @need_fix_class_id: whether this host's class ID needed to be fixed or not
> > + * @pm_support: whether the host's MTCMOS will be off when suspend
> >   * @ops: pointer to configuration access functions
> >   * @startup: pointer to controller setting functions
> >   * @setup_irq: pointer to initialize IRQ functions
> >   */
> >  struct mtk_pcie_soc {
> >  	bool need_fix_class_id;
> > +	bool pm_support;
> >  	struct pci_ops *ops;
> >  	int (*startup)(struct mtk_pcie_port *port);
> >  	int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
> > @@ -1179,12 +1181,91 @@ static int mtk_pcie_probe(struct platform_device *pdev)
> >  	return err;
> >  }
> >  
> > +static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
> > +{
> > +	struct platform_device *pdev;
> > +	struct mtk_pcie *pcie;
> > +	struct mtk_pcie_port *port;
> > +	const struct mtk_pcie_soc *soc;
> > +
> > +	pdev = to_platform_device(dev);
> > +	pcie = platform_get_drvdata(pdev);
> 
> How about this -
> struct mtk_pcie *pcie = dev_get_drvdata(dev);

Yes, I forgot that, thanks very much for your reminder.

> 
> > +	soc = pcie->soc;
> > +	if (!soc->pm_support)
> > +		return 0;
> > +
> > +	list_for_each_entry(port, &pcie->ports, list) {
> > +		clk_disable_unprepare(port->ahb_ck);
> > +		clk_disable_unprepare(port->sys_ck);
> > +		phy_power_off(port->phy);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
> > +{
> > +	struct platform_device *pdev;
> > +	struct mtk_pcie *pcie;
> > +	struct mtk_pcie_port *port;
> > +	const struct mtk_pcie_soc *soc;
> > +	int ret;
> > +
> > +	pdev = to_platform_device(dev);
> > +	pcie = platform_get_drvdata(pdev);
> 
> struct mtk_pcie *pcie = dev_get_drvdata(dev);

thanks.

> 
> > +	soc = pcie->soc;
> > +	if (!soc->pm_support)
> > +		return 0;
> > +
> > +	list_for_each_entry(port, &pcie->ports, list) {
> > +		ret = phy_power_on(port->phy);
> > +		if (ret) {
> > +			dev_err(dev, "could not power on phy\n");
> > +			return ret;
> > +		}
> > +		ret = clk_prepare_enable(port->sys_ck);
> > +		if (ret) {
> > +			dev_err(dev, "enable sys clock error\n");
> > +			phy_power_off(port->phy);
> > +			return ret;
> > +		}
> > +
> > +		ret = clk_prepare_enable(port->ahb_ck);
> > +		if (ret) {
> > +			dev_err(dev, "enable ahb clock error\n");
> > +			phy_power_off(port->phy);
> > +			clk_disable_unprepare(port->sys_ck);
> > +			return ret;
> > +		}
> > +
> > +		ret = soc->startup(port);
> > +		if (ret) {
> > +			dev_err(dev, "pcie linkup failed\n");
> > +			phy_power_off(port->phy);
> > +			clk_disable_unprepare(port->sys_ck);
> > +			clk_disable_unprepare(port->ahb_ck);
> > +			return ret;
> > +		}
> > +
> > +		if (IS_ENABLED(CONFIG_PCI_MSI))
> > +			mtk_pcie_enable_msi(port);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> 
> 
>
Bjorn Helgaas May 31, 2018, 4:20 a.m. | #3
On Wed, May 30, 2018 at 10:35:36AM +0800, honghui.zhang@mediatek.com wrote:
> From: Honghui Zhang <honghui.zhang@mediatek.com>
> 
> The MTCMOS of PCIe Host for MT2712 will be off when system suspend, and all
> the internel control register will be reset after system resume. The PCIe
> link should be re-established and the related control register values should
> be re-set after system resume.
> 
> Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
> CC: Ryder Lee <ryder.lee@mediatek.com>
> ---
>  drivers/pci/host/pcie-mediatek.c | 82 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 82 insertions(+)
> 
> diff --git a/drivers/pci/host/pcie-mediatek.c b/drivers/pci/host/pcie-mediatek.c
> index dabf1086..60f98d92 100644
> --- a/drivers/pci/host/pcie-mediatek.c
> +++ b/drivers/pci/host/pcie-mediatek.c
> @@ -132,12 +132,14 @@ struct mtk_pcie_port;
>  /**
>   * struct mtk_pcie_soc - differentiate between host generations
>   * @need_fix_class_id: whether this host's class ID needed to be fixed or not
> + * @pm_support: whether the host's MTCMOS will be off when suspend
>   * @ops: pointer to configuration access functions
>   * @startup: pointer to controller setting functions
>   * @setup_irq: pointer to initialize IRQ functions
>   */
>  struct mtk_pcie_soc {
>  	bool need_fix_class_id;
> +	bool pm_support;
>  	struct pci_ops *ops;
>  	int (*startup)(struct mtk_pcie_port *port);
>  	int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
> @@ -1179,12 +1181,91 @@ static int mtk_pcie_probe(struct platform_device *pdev)
>  	return err;
>  }
>  
> +static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
> +{
> +	struct platform_device *pdev;
> +	struct mtk_pcie *pcie;
> +	struct mtk_pcie_port *port;
> +	const struct mtk_pcie_soc *soc;
> +
> +	pdev = to_platform_device(dev);
> +	pcie = platform_get_drvdata(pdev);
> +	soc = pcie->soc;
> +	if (!soc->pm_support)
> +		return 0;
> +
> +	list_for_each_entry(port, &pcie->ports, list) {
> +		clk_disable_unprepare(port->ahb_ck);
> +		clk_disable_unprepare(port->sys_ck);
> +		phy_power_off(port->phy);
> +	}
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)

I don't really like the __maybe_unused annotations.  Looking at the
other users of SET_NOIRQ_SYSTEM_SLEEP_PM_OPS, I think a small majority
of them wrap the function definitions in #ifdef CONFIG_PM_SLEEP
instead of using __maybe_unused.  That's also a bit ugly, but has the
advantage of truly omitting the definitions when they're not needed.
Honghui Zhang May 31, 2018, 7:37 a.m. | #4
On Wed, 2018-05-30 at 23:20 -0500, Bjorn Helgaas wrote:
> On Wed, May 30, 2018 at 10:35:36AM +0800, honghui.zhang@mediatek.com wrote:
> > From: Honghui Zhang <honghui.zhang@mediatek.com>
> > 
> > The MTCMOS of PCIe Host for MT2712 will be off when system suspend, and all
> > the internel control register will be reset after system resume. The PCIe
> > link should be re-established and the related control register values should
> > be re-set after system resume.
> > 
> > Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
> > CC: Ryder Lee <ryder.lee@mediatek.com>
> > ---
> >  drivers/pci/host/pcie-mediatek.c | 82 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 82 insertions(+)
> > 
> > diff --git a/drivers/pci/host/pcie-mediatek.c b/drivers/pci/host/pcie-mediatek.c
> > index dabf1086..60f98d92 100644
> > --- a/drivers/pci/host/pcie-mediatek.c
> > +++ b/drivers/pci/host/pcie-mediatek.c
> > @@ -132,12 +132,14 @@ struct mtk_pcie_port;
> >  /**
> >   * struct mtk_pcie_soc - differentiate between host generations
> >   * @need_fix_class_id: whether this host's class ID needed to be fixed or not
> > + * @pm_support: whether the host's MTCMOS will be off when suspend
> >   * @ops: pointer to configuration access functions
> >   * @startup: pointer to controller setting functions
> >   * @setup_irq: pointer to initialize IRQ functions
> >   */
> >  struct mtk_pcie_soc {
> >  	bool need_fix_class_id;
> > +	bool pm_support;
> >  	struct pci_ops *ops;
> >  	int (*startup)(struct mtk_pcie_port *port);
> >  	int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
> > @@ -1179,12 +1181,91 @@ static int mtk_pcie_probe(struct platform_device *pdev)
> >  	return err;
> >  }
> >  
> > +static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
> > +{
> > +	struct platform_device *pdev;
> > +	struct mtk_pcie *pcie;
> > +	struct mtk_pcie_port *port;
> > +	const struct mtk_pcie_soc *soc;
> > +
> > +	pdev = to_platform_device(dev);
> > +	pcie = platform_get_drvdata(pdev);
> > +	soc = pcie->soc;
> > +	if (!soc->pm_support)
> > +		return 0;
> > +
> > +	list_for_each_entry(port, &pcie->ports, list) {
> > +		clk_disable_unprepare(port->ahb_ck);
> > +		clk_disable_unprepare(port->sys_ck);
> > +		phy_power_off(port->phy);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
> 
> I don't really like the __maybe_unused annotations.  Looking at the
> other users of SET_NOIRQ_SYSTEM_SLEEP_PM_OPS, I think a small majority
> of them wrap the function definitions in #ifdef CONFIG_PM_SLEEP
> instead of using __maybe_unused.  That's also a bit ugly, but has the
> advantage of truly omitting the definitions when they're not needed.

Ok, I will follow your advise in the next version.

thanks.
kbuild test robot June 1, 2018, 12:42 a.m. | #5
Hi Honghui,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on pci/next]
[also build test WARNING on next-20180531]
[cannot apply to v4.17-rc7]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/honghui-zhang-mediatek-com/PCI-mediatek-Add-system-pm-support-for-MT2712/20180601-053217
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

   drivers/pci/host/pcie-mediatek.c:463:40: sparse: incorrect type in argument 1 (different address spaces) @@    expected void volatile *address @@    got void [novoid volatile *address @@
   drivers/pci/host/pcie-mediatek.c:463:40:    expected void volatile *address
   drivers/pci/host/pcie-mediatek.c:463:40:    got void [noderef] <asn:2>*
   drivers/pci/host/pcie-mediatek.c:586:44: sparse: incorrect type in argument 1 (different address spaces) @@    expected void volatile *address @@    got void [novoid volatile *address @@
   drivers/pci/host/pcie-mediatek.c:586:44:    expected void volatile *address
   drivers/pci/host/pcie-mediatek.c:586:44:    got void [noderef] <asn:2>*
>> drivers/pci/host/pcie-mediatek.c:1259:25: sparse: symbol 'mtk_pcie_pm_ops' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Patch

diff --git a/drivers/pci/host/pcie-mediatek.c b/drivers/pci/host/pcie-mediatek.c
index dabf1086..60f98d92 100644
--- a/drivers/pci/host/pcie-mediatek.c
+++ b/drivers/pci/host/pcie-mediatek.c
@@ -132,12 +132,14 @@  struct mtk_pcie_port;
 /**
  * struct mtk_pcie_soc - differentiate between host generations
  * @need_fix_class_id: whether this host's class ID needed to be fixed or not
+ * @pm_support: whether the host's MTCMOS will be off when suspend
  * @ops: pointer to configuration access functions
  * @startup: pointer to controller setting functions
  * @setup_irq: pointer to initialize IRQ functions
  */
 struct mtk_pcie_soc {
 	bool need_fix_class_id;
+	bool pm_support;
 	struct pci_ops *ops;
 	int (*startup)(struct mtk_pcie_port *port);
 	int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
@@ -1179,12 +1181,91 @@  static int mtk_pcie_probe(struct platform_device *pdev)
 	return err;
 }
 
+static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
+{
+	struct platform_device *pdev;
+	struct mtk_pcie *pcie;
+	struct mtk_pcie_port *port;
+	const struct mtk_pcie_soc *soc;
+
+	pdev = to_platform_device(dev);
+	pcie = platform_get_drvdata(pdev);
+	soc = pcie->soc;
+	if (!soc->pm_support)
+		return 0;
+
+	list_for_each_entry(port, &pcie->ports, list) {
+		clk_disable_unprepare(port->ahb_ck);
+		clk_disable_unprepare(port->sys_ck);
+		phy_power_off(port->phy);
+	}
+
+	return 0;
+}
+
+static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
+{
+	struct platform_device *pdev;
+	struct mtk_pcie *pcie;
+	struct mtk_pcie_port *port;
+	const struct mtk_pcie_soc *soc;
+	int ret;
+
+	pdev = to_platform_device(dev);
+	pcie = platform_get_drvdata(pdev);
+	soc = pcie->soc;
+	if (!soc->pm_support)
+		return 0;
+
+	list_for_each_entry(port, &pcie->ports, list) {
+		ret = phy_power_on(port->phy);
+		if (ret) {
+			dev_err(dev, "could not power on phy\n");
+			return ret;
+		}
+		ret = clk_prepare_enable(port->sys_ck);
+		if (ret) {
+			dev_err(dev, "enable sys clock error\n");
+			phy_power_off(port->phy);
+			return ret;
+		}
+
+		ret = clk_prepare_enable(port->ahb_ck);
+		if (ret) {
+			dev_err(dev, "enable ahb clock error\n");
+			phy_power_off(port->phy);
+			clk_disable_unprepare(port->sys_ck);
+			return ret;
+		}
+
+		ret = soc->startup(port);
+		if (ret) {
+			dev_err(dev, "pcie linkup failed\n");
+			phy_power_off(port->phy);
+			clk_disable_unprepare(port->sys_ck);
+			clk_disable_unprepare(port->ahb_ck);
+			return ret;
+		}
+
+		if (IS_ENABLED(CONFIG_PCI_MSI))
+			mtk_pcie_enable_msi(port);
+	}
+
+	return 0;
+}
+
+const struct dev_pm_ops mtk_pcie_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,
+				      mtk_pcie_resume_noirq)
+};
+
 static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
 	.ops = &mtk_pcie_ops,
 	.startup = mtk_pcie_startup_port,
 };
 
 static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
+	.pm_support = true,
 	.ops = &mtk_pcie_ops_v2,
 	.startup = mtk_pcie_startup_port_v2,
 	.setup_irq = mtk_pcie_setup_irq,
@@ -1211,6 +1292,7 @@  static struct platform_driver mtk_pcie_driver = {
 		.name = "mtk-pcie",
 		.of_match_table = mtk_pcie_ids,
 		.suppress_bind_attrs = true,
+		.pm = &mtk_pcie_pm_ops,
 	},
 };
 builtin_platform_driver(mtk_pcie_driver);