| Message ID | 1530518264-6125-4-git-send-email-honghui.zhang@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
[+Rafael, Kevin, Ulf] On Mon, Jul 02, 2018 at 03:57:43PM +0800, honghui.zhang@mediatek.com wrote: > From: Honghui Zhang <honghui.zhang@mediatek.com> > > The MTCMOS of PCIe Host for MT2712 and MT7622 will be off when system > suspend, and all the internal 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> > Acked-by: Ryder Lee <ryder.lee@mediatek.com> > --- > drivers/pci/controller/pcie-mediatek.c | 67 ++++++++++++++++++++++++++++++++++ > 1 file changed, 67 insertions(+) > > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c > index 86918d4..175d7b6 100644 > --- a/drivers/pci/controller/pcie-mediatek.c > +++ b/drivers/pci/controller/pcie-mediatek.c > @@ -134,12 +134,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); > @@ -1197,12 +1199,75 @@ static int mtk_pcie_probe(struct platform_device *pdev) > return err; > } > > +static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev) > +{ > + struct mtk_pcie *pcie = dev_get_drvdata(dev); > + const struct mtk_pcie_soc *soc = pcie->soc; > + struct mtk_pcie_port *port; > + > + if (!soc->pm_support) > + return 0; > + > + if (list_empty(&pcie->ports)) > + return 0; > + > + list_for_each_entry(port, &pcie->ports, list) { > + clk_disable_unprepare(port->pipe_ck); > + clk_disable_unprepare(port->obff_ck); > + clk_disable_unprepare(port->axi_ck); > + clk_disable_unprepare(port->aux_ck); > + clk_disable_unprepare(port->ahb_ck); > + clk_disable_unprepare(port->sys_ck); > + phy_power_off(port->phy); > + phy_exit(port->phy); > + } > + > + mtk_pcie_subsys_powerdown(pcie); > + > + return 0; > +} > + > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev) > +{ > + struct mtk_pcie *pcie = dev_get_drvdata(dev); > + const struct mtk_pcie_soc *soc = pcie->soc; > + struct mtk_pcie_port *port, *tmp; > + > + if (!soc->pm_support) > + return 0; > + > + if (list_empty(&pcie->ports)) > + return 0; > + > + if (dev->pm_domain) { > + pm_runtime_enable(dev); > + pm_runtime_get_sync(dev); > + } Are these runtime PM calls needed/abused here ? Mind explaining the logic ? There is certainly an asymmetry with the suspend callback which made me suspicious, I am pretty certain Rafael/Kevin/Ulf can help me clarify so that we can make progress with this patch. Lorenzo > + > + clk_prepare_enable(pcie->free_ck); > + > + list_for_each_entry_safe(port, tmp, &pcie->ports, list) > + mtk_pcie_enable_port(port); > + > + /* In case of EP was removed while system suspend. */ > + if (list_empty(&pcie->ports)) > + mtk_pcie_subsys_powerdown(pcie); > + > + return 0; > +} > + > +static 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, > @@ -1210,6 +1275,7 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = { > > static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = { > .need_fix_class_id = true, > + .pm_support = true, > .ops = &mtk_pcie_ops_v2, > .startup = mtk_pcie_startup_port_v2, > .setup_irq = mtk_pcie_setup_irq, > @@ -1229,6 +1295,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 Tue, 2018-07-17 at 18:15 +0100, Lorenzo Pieralisi wrote: > [+Rafael, Kevin, Ulf] > > On Mon, Jul 02, 2018 at 03:57:43PM +0800, honghui.zhang@mediatek.com wrote: > > From: Honghui Zhang <honghui.zhang@mediatek.com> > > > > The MTCMOS of PCIe Host for MT2712 and MT7622 will be off when system > > suspend, and all the internal 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> > > Acked-by: Ryder Lee <ryder.lee@mediatek.com> > > --- > > drivers/pci/controller/pcie-mediatek.c | 67 ++++++++++++++++++++++++++++++++++ > > 1 file changed, 67 insertions(+) > > > > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c > > index 86918d4..175d7b6 100644 > > --- a/drivers/pci/controller/pcie-mediatek.c > > +++ b/drivers/pci/controller/pcie-mediatek.c > > @@ -134,12 +134,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); > > @@ -1197,12 +1199,75 @@ static int mtk_pcie_probe(struct platform_device *pdev) > > return err; > > } > > > > +static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev) > > +{ > > + struct mtk_pcie *pcie = dev_get_drvdata(dev); > > + const struct mtk_pcie_soc *soc = pcie->soc; > > + struct mtk_pcie_port *port; > > + > > + if (!soc->pm_support) > > + return 0; > > + > > + if (list_empty(&pcie->ports)) > > + return 0; > > + > > + list_for_each_entry(port, &pcie->ports, list) { > > + clk_disable_unprepare(port->pipe_ck); > > + clk_disable_unprepare(port->obff_ck); > > + clk_disable_unprepare(port->axi_ck); > > + clk_disable_unprepare(port->aux_ck); > > + clk_disable_unprepare(port->ahb_ck); > > + clk_disable_unprepare(port->sys_ck); > > + phy_power_off(port->phy); > > + phy_exit(port->phy); > > + } > > + > > + mtk_pcie_subsys_powerdown(pcie); > > + > > + return 0; > > +} > > + > > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev) > > +{ > > + struct mtk_pcie *pcie = dev_get_drvdata(dev); > > + const struct mtk_pcie_soc *soc = pcie->soc; > > + struct mtk_pcie_port *port, *tmp; > > + > > + if (!soc->pm_support) > > + return 0; > > + > > + if (list_empty(&pcie->ports)) > > + return 0; > > + > > + if (dev->pm_domain) { > > + pm_runtime_enable(dev); > > + pm_runtime_get_sync(dev); > > + } > > Are these runtime PM calls needed/abused here ? > > Mind explaining the logic ? > > There is certainly an asymmetry with the suspend callback which made me > suspicious, I am pretty certain Rafael/Kevin/Ulf can help me clarify so > that we can make progress with this patch. > > Lorenzo > Hi Lorenzo, thanks for your comments. Sorry I don't get you. I believe that in suspend callbacks the pm_runtime_put_sync and pm_runtime_disable should be called to gated the CMOS for this module, while the pm_rumtime_enable and pm_rumtime_get_sync should be called in resume callback. That's exactly this patch doing. But the pm_rumtime_put_sync and pm_runtime_disable functions was wrapped in the mtk_pcie_subsys_powerdown. I did not call mtk_pcie_subsys_powerup since it does not just wrapped pm_rumtime related functions but also do the platform_resource_get, devm_ioremap, and free_ck clock get which I do not needed in resume callback. Do you think it will be much clear if I abstract the platform_resource_get, devm_ioremap functions from mtk_pcie_subsys_powerup and put it to a new functions like mtk_pcie_subsys_resource_get, and then we may call the mtk_pcie_subsys_powerup in the resume function? thanks > > + > > + clk_prepare_enable(pcie->free_ck); > > + > > + list_for_each_entry_safe(port, tmp, &pcie->ports, list) > > + mtk_pcie_enable_port(port); > > + > > + /* In case of EP was removed while system suspend. */ > > + if (list_empty(&pcie->ports)) > > + mtk_pcie_subsys_powerdown(pcie); > > + > > + return 0; > > +} > > + > > +static 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, > > @@ -1210,6 +1275,7 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = { > > > > static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = { > > .need_fix_class_id = true, > > + .pm_support = true, > > .ops = &mtk_pcie_ops_v2, > > .startup = mtk_pcie_startup_port_v2, > > .setup_irq = mtk_pcie_setup_irq, > > @@ -1229,6 +1295,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, Jul 18, 2018 at 02:02:41PM +0800, Honghui Zhang wrote: <snip> > > > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev) > > > +{ > > > + struct mtk_pcie *pcie = dev_get_drvdata(dev); > > > + const struct mtk_pcie_soc *soc = pcie->soc; > > > + struct mtk_pcie_port *port, *tmp; > > > + > > > + if (!soc->pm_support) > > > + return 0; > > > + > > > + if (list_empty(&pcie->ports)) > > > + return 0; > > > + > > > + if (dev->pm_domain) { > > > + pm_runtime_enable(dev); > > > + pm_runtime_get_sync(dev); > > > + } > > > > Are these runtime PM calls needed/abused here ? > > > > Mind explaining the logic ? > > > > There is certainly an asymmetry with the suspend callback which made me > > suspicious, I am pretty certain Rafael/Kevin/Ulf can help me clarify so > > that we can make progress with this patch. > > > > Lorenzo > > > Hi Lorenzo, thanks for your comments. > Sorry I don't get you. > I believe that in suspend callbacks the pm_runtime_put_sync and > pm_runtime_disable should be called to gated the CMOS for this module, > while the pm_rumtime_enable and pm_rumtime_get_sync should be called > in resume callback. That's why I CC'ed Rafael, Kevin and Ulf, to answer this question thoroughly, I am not sure it is needed and that's the right way of doing it in system suspend callbacks. > That's exactly this patch doing. > But the pm_rumtime_put_sync and pm_runtime_disable functions was wrapped > in the mtk_pcie_subsys_powerdown. Ah, sorry, I missed that. > I did not call mtk_pcie_subsys_powerup since it does not just wrapped > pm_rumtime related functions but also do the platform_resource_get, > devm_ioremap, and free_ck clock get which I do not needed in resume > callback. > > Do you think it will be much clear if I abstract the > platform_resource_get, devm_ioremap functions from > mtk_pcie_subsys_powerup and put it to a new functions like > mtk_pcie_subsys_resource_get, and then we may call the > mtk_pcie_subsys_powerup in the resume function? I think so but let's wait first for feedback on whether those runtime PM calls are needed in the first place. Lorenzo
On Wed, 2018-07-18 at 10:49 +0100, Lorenzo Pieralisi wrote: > On Wed, Jul 18, 2018 at 02:02:41PM +0800, Honghui Zhang wrote: > > <snip> > > > > > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev) > > > > +{ > > > > + struct mtk_pcie *pcie = dev_get_drvdata(dev); > > > > + const struct mtk_pcie_soc *soc = pcie->soc; > > > > + struct mtk_pcie_port *port, *tmp; > > > > + > > > > + if (!soc->pm_support) > > > > + return 0; > > > > + > > > > + if (list_empty(&pcie->ports)) > > > > + return 0; > > > > + > > > > + if (dev->pm_domain) { > > > > + pm_runtime_enable(dev); > > > > + pm_runtime_get_sync(dev); > > > > + } > > > > > > Are these runtime PM calls needed/abused here ? > > > > > > Mind explaining the logic ? > > > > > > There is certainly an asymmetry with the suspend callback which made me > > > suspicious, I am pretty certain Rafael/Kevin/Ulf can help me clarify so > > > that we can make progress with this patch. > > > > > > Lorenzo > > > > > Hi Lorenzo, thanks for your comments. > > Sorry I don't get you. > > I believe that in suspend callbacks the pm_runtime_put_sync and > > pm_runtime_disable should be called to gated the CMOS for this module, > > while the pm_rumtime_enable and pm_rumtime_get_sync should be called > > in resume callback. > That's why I CC'ed Rafael, Kevin and Ulf, to answer this question > thoroughly, I am not sure it is needed and that's the right way > of doing it in system suspend callbacks. > hi, Rafael, Kevin and Ulf, after reading of the power related documents in Documents/power/, I'm still confused whether the runtime_pm callbacks should be called in system suspend callbacks. I believe that system suspend does not care about the device's CMOS status. And the device's CMOS status is controlled by runtime pm. That why I gated the CMOS through runtime pm in the system suspend callbacks. But I checked all existing system suspend callbacks and found there's no runtime pm was executed in system suspend callbacks. Does that means when system suspend, the system suspend framework does not care about the power consume? Or does it gated each device's CMOS in somewhere else? Or should I just remove the runtime pm callbacks in the system suspend flow? Could someone kindly give me some comments? Thanks in advance. > > That's exactly this patch doing. > > But the pm_rumtime_put_sync and pm_runtime_disable functions was wrapped > > in the mtk_pcie_subsys_powerdown. > > Ah, sorry, I missed that. > > > I did not call mtk_pcie_subsys_powerup since it does not just wrapped > > pm_rumtime related functions but also do the platform_resource_get, > > devm_ioremap, and free_ck clock get which I do not needed in resume > > callback. > > > > Do you think it will be much clear if I abstract the > > platform_resource_get, devm_ioremap functions from > > mtk_pcie_subsys_powerup and put it to a new functions like > > mtk_pcie_subsys_resource_get, and then we may call the > > mtk_pcie_subsys_powerup in the resume function? > > I think so but let's wait first for feedback on whether those > runtime PM calls are needed in the first place. > > Lorenzo
diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c index 86918d4..175d7b6 100644 --- a/drivers/pci/controller/pcie-mediatek.c +++ b/drivers/pci/controller/pcie-mediatek.c @@ -134,12 +134,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); @@ -1197,12 +1199,75 @@ static int mtk_pcie_probe(struct platform_device *pdev) return err; } +static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev) +{ + struct mtk_pcie *pcie = dev_get_drvdata(dev); + const struct mtk_pcie_soc *soc = pcie->soc; + struct mtk_pcie_port *port; + + if (!soc->pm_support) + return 0; + + if (list_empty(&pcie->ports)) + return 0; + + list_for_each_entry(port, &pcie->ports, list) { + clk_disable_unprepare(port->pipe_ck); + clk_disable_unprepare(port->obff_ck); + clk_disable_unprepare(port->axi_ck); + clk_disable_unprepare(port->aux_ck); + clk_disable_unprepare(port->ahb_ck); + clk_disable_unprepare(port->sys_ck); + phy_power_off(port->phy); + phy_exit(port->phy); + } + + mtk_pcie_subsys_powerdown(pcie); + + return 0; +} + +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev) +{ + struct mtk_pcie *pcie = dev_get_drvdata(dev); + const struct mtk_pcie_soc *soc = pcie->soc; + struct mtk_pcie_port *port, *tmp; + + if (!soc->pm_support) + return 0; + + if (list_empty(&pcie->ports)) + return 0; + + if (dev->pm_domain) { + pm_runtime_enable(dev); + pm_runtime_get_sync(dev); + } + + clk_prepare_enable(pcie->free_ck); + + list_for_each_entry_safe(port, tmp, &pcie->ports, list) + mtk_pcie_enable_port(port); + + /* In case of EP was removed while system suspend. */ + if (list_empty(&pcie->ports)) + mtk_pcie_subsys_powerdown(pcie); + + return 0; +} + +static 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, @@ -1210,6 +1275,7 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = { static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = { .need_fix_class_id = true, + .pm_support = true, .ops = &mtk_pcie_ops_v2, .startup = mtk_pcie_startup_port_v2, .setup_irq = mtk_pcie_setup_irq, @@ -1229,6 +1295,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);