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

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

On Wed, 2018-06-27 at 19:45 +0300, Andy Shevchenko wrote:
> On Wed, Jun 27, 2018 at 12:21 PM,  <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.
> 
> >  struct mtk_pcie_soc {
> >         bool need_fix_class_id;
> 
> > +       bool pm_support;
> 
> Hmm... Do you really need this flag? Can't runtime PM API tell you this?

This host driver is both for MT2712, MT7622 and MT7623, but MT7623 do
not this this patch.
I only add this flag to identify whether we need to do the PM operation
for this SoC since I do not want to touch anything for MT7623.
 
> 
> >         struct pci_ops *ops;
> >         int (*startup)(struct mtk_pcie_port *port);
> >         int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
> > @@ -1195,12 +1197,76 @@ static int mtk_pcie_probe(struct platform_device *pdev)
> >         return err;
> >  }
> >
> > +#ifdef CONFIG_PM_SLEEP
> > +static int mtk_pcie_suspend_noirq(struct device *dev)
> 
> Perhaps __maybe_unused?

Ok, I'm a bit confused about this, if there's some discussion about
this, do you have the link of those discussion and kindly point put it
in this thread? Or I guess I can change this back to __maybe_unused if
there's no other comments about this.

> 
> > +{
> > +       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;
> 
> So, if the list is empty you are not suspending for the host?
> 

if the list was empty then it indicate that all the PCIe slot was not
connected with any EP device, and all the resource have been release in
probe time. So the host driver does not need to save or restore
anything.

> 
> > +
> > +       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 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;
> > +
> > +       if (!soc->pm_support)
> > +               return 0;
> > +
> 
> > +       if (list_empty(&pcie->ports))
> > +               return 0;
> 
> No runtime PM for this case?
> (It seems now I understand why in previous patch you have a similar check)
> 
I guess host driver does not need to resume anything if there's no EP
device was connected.

> > +
> > +       if (dev->pm_domain) {
> > +               pm_runtime_enable(dev);
> > +               pm_runtime_get_sync(dev);
> > +       }
> > +
> > +       clk_prepare_enable(pcie->free_ck);
> > +
> > +       list_for_each_entry(port, &pcie->ports, list)
> > +               mtk_pcie_enable_port(port);
> > +
> 
> > +       if (list_empty(&pcie->ports))
> 
> Hmm... How it would be true if you already bailed out above on the
> same condition?

Assuming that there's EP device connected before suspend, and the EP was
removed from the link while system suspend. It means that when enter
this function the list is not empty, but after the function of
mtk_pcie_enable_port was called, host driver found there's no EP device
was connected anymore, it will free this list and some other resources
in mtk_pcie_enable_port,(after mtk_pcie_enable_port was called, the list
is empty in this scenario) but leave the subsys power along, I guess we
need to take care of this scenario.
> 
> > +               mtk_pcie_subsys_powerdown(pcie);
> > +
> > +       return 0;
> > +}
> > +#endif
> > +
> > +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,
> 
> No update for .pm ?

I'm not get your point, I update the .pm callbacks in the
platform_driver struct, this SoC data just store the different
information for SoCs. Did I missed something?

> 
> > @@ -1208,6 +1274,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,
> > @@ -1227,6 +1294,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
> >
> 
> 
>