Re: [PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[Honghui Zhang <honghui.zhang@mediatek.com>]

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
> >