| Message ID | 1527647736-19986-1-git-send-email-honghui.zhang@mediatek.com |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: mediatek: Add system pm support for MT2712 | expand |
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; > +} > +
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; > > +} > > + > > >
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.
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.
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
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);