From: Honghui Zhang <[email protected]>
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 <[email protected]>
CC: Ryder Lee <[email protected]>
---
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)
+{
+ 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);
--
2.6.4
On Wed, 2018-05-30 at 10:35 +0800, [email protected] wrote:
> From: Honghui Zhang <[email protected]>
>
> 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 <[email protected]>
> CC: Ryder Lee <[email protected]>
> ---
> 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;
> +}
> +
On Thu, 2018-05-31 at 10:05 +0800, Ryder Lee wrote:
> On Wed, 2018-05-30 at 10:35 +0800, [email protected] wrote:
> > From: Honghui Zhang <[email protected]>
> >
> > 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 <[email protected]>
> > CC: Ryder Lee <[email protected]>
> > ---
> > 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;
> > +}
> > +
>
>
>
On Wed, May 30, 2018 at 10:35:36AM +0800, [email protected] wrote:
> From: Honghui Zhang <[email protected]>
>
> 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 <[email protected]>
> CC: Ryder Lee <[email protected]>
> ---
> 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.
On Wed, 2018-05-30 at 23:20 -0500, Bjorn Helgaas wrote:
> On Wed, May 30, 2018 at 10:35:36AM +0800, [email protected] wrote:
> > From: Honghui Zhang <[email protected]>
> >
> > 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 <[email protected]>
> > CC: Ryder Lee <[email protected]>
> > ---
> > 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.
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
Fixes: 24dc21cd1e9d ("PCI: mediatek: Add system pm support for MT2712")
Signed-off-by: kbuild test robot <[email protected]>
---
pcie-mediatek.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/host/pcie-mediatek.c b/drivers/pci/host/pcie-mediatek.c
index e3b16b0..e01cc66 100644
--- a/drivers/pci/host/pcie-mediatek.c
+++ b/drivers/pci/host/pcie-mediatek.c
@@ -1256,7 +1256,7 @@ static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
return 0;
}
-const struct dev_pm_ops mtk_pcie_pm_ops = {
+static const struct dev_pm_ops mtk_pcie_pm_ops = {
SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,
mtk_pcie_resume_noirq)
};