| Message ID | 20240102-j7200-pcie-s2r-v1-11-84e55da52400@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add suspend to ram support for PCIe on J7200 | expand |
On Mon, Jan 15, 2024 at 05:14:52PM +0100, Thomas Richard wrote: > Add suspend and resume support. > The alread_configured flag is cleared during suspend stage to force the > phy initialization during the resume stage. s/alread_configured/already_configured/ Wrap to fill 75 columns. Add a blank line if you intend two paragraphs. I don't know whether there's a strong convention in drivers/phy, but I see several commit logs that capitalize "PHY". "Phy" is not a standard English word, so I think the capitalization makes it easier to read. Bjorn
Hi Thomas, On Mo, 2024-01-15 at 17:14 +0100, Thomas Richard wrote: > Add suspend and resume support. > The alread_configured flag is cleared during suspend stage to force the > phy initialization during the resume stage. > > Based on the work of Théo Lebrun <theo.lebrun@bootlin.com> > > Signed-off-by: Thomas Richard <thomas.richard@bootlin.com> > --- > drivers/phy/cadence/phy-cadence-torrent.c | 57 +++++++++++++++++++++++++++++++ > 1 file changed, 57 insertions(+) > > diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c > index 70413fca5776..31b2594e5942 100644 > --- a/drivers/phy/cadence/phy-cadence-torrent.c > +++ b/drivers/phy/cadence/phy-cadence-torrent.c > @@ -3006,6 +3006,62 @@ static void cdns_torrent_phy_remove(struct platform_device *pdev) [...] > +static int cdns_torrent_phy_resume_noirq(struct device *dev) > +{ > + struct cdns_torrent_phy *cdns_phy = dev_get_drvdata(dev); > + int node = cdns_phy->nsubnodes; > + int ret, i; > + > + ret = cdns_torrent_clk(cdns_phy); > + if (ret) > + goto clk_cleanup; > + > + /* Enable APB */ > + reset_control_deassert(cdns_phy->apb_rst); > + > + if (cdns_phy->nsubnodes > 1) { > + ret = cdns_torrent_phy_configure_multilink(cdns_phy); > + if (ret) > + goto put_lnk_rst; > + } > + > + return 0; > + > +put_lnk_rst: > + for (i = 0; i < node; i++) > + reset_control_put(cdns_phy->phys[i].lnk_rst); What is this intended to do? I expect this to explode in _remove, where the lnk_rst are put again. Should this be: reset_control_assert(cdns_phy->phys[i].lnk_rst); ? regards Philipp
Hello, On 1/16/24 19:22, Bjorn Helgaas wrote: > On Mon, Jan 15, 2024 at 05:14:52PM +0100, Thomas Richard wrote: >> Add suspend and resume support. >> The alread_configured flag is cleared during suspend stage to force the >> phy initialization during the resume stage. > > s/alread_configured/already_configured/ > > Wrap to fill 75 columns. Add a blank line if you intend two > paragraphs. > > I don't know whether there's a strong convention in drivers/phy, but I > see several commit logs that capitalize "PHY". "Phy" is not a > standard English word, so I think the capitalization makes it easier > to read. > Yes indeed, PHY is used in lots of commit logs. So I guess I can use it. Regards,
Hello Philipp, On 1/17/24 16:12, Philipp Zabel wrote: > Hi Thomas, > > On Mo, 2024-01-15 at 17:14 +0100, Thomas Richard wrote: >> Add suspend and resume support. >> The alread_configured flag is cleared during suspend stage to force the >> phy initialization during the resume stage. >> >> Based on the work of Théo Lebrun <theo.lebrun@bootlin.com> >> >> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com> >> --- >> drivers/phy/cadence/phy-cadence-torrent.c | 57 +++++++++++++++++++++++++++++++ >> 1 file changed, 57 insertions(+) >> >> diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c >> index 70413fca5776..31b2594e5942 100644 >> --- a/drivers/phy/cadence/phy-cadence-torrent.c >> +++ b/drivers/phy/cadence/phy-cadence-torrent.c >> @@ -3006,6 +3006,62 @@ static void cdns_torrent_phy_remove(struct platform_device *pdev) > [...] >> +static int cdns_torrent_phy_resume_noirq(struct device *dev) >> +{ >> + struct cdns_torrent_phy *cdns_phy = dev_get_drvdata(dev); >> + int node = cdns_phy->nsubnodes; >> + int ret, i; >> + >> + ret = cdns_torrent_clk(cdns_phy); >> + if (ret) >> + goto clk_cleanup; >> + >> + /* Enable APB */ >> + reset_control_deassert(cdns_phy->apb_rst); >> + >> + if (cdns_phy->nsubnodes > 1) { >> + ret = cdns_torrent_phy_configure_multilink(cdns_phy); >> + if (ret) >> + goto put_lnk_rst; >> + } >> + >> + return 0; >> + >> +put_lnk_rst: >> + for (i = 0; i < node; i++) >> + reset_control_put(cdns_phy->phys[i].lnk_rst); > > What is this intended to do? I expect this to explode in _remove, where > the lnk_rst are put again. Should this be: > > reset_control_assert(cdns_phy->phys[i].lnk_rst); > > ? Yes indeed, it's reset_control_assert instead of reset_control_put. Thanks for the review. Regards,
diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c index 70413fca5776..31b2594e5942 100644 --- a/drivers/phy/cadence/phy-cadence-torrent.c +++ b/drivers/phy/cadence/phy-cadence-torrent.c @@ -3006,6 +3006,62 @@ static void cdns_torrent_phy_remove(struct platform_device *pdev) cdns_torrent_clk_cleanup(cdns_phy); } +#ifdef CONFIG_PM +static int cdns_torrent_phy_suspend_noirq(struct device *dev) +{ + struct cdns_torrent_phy *cdns_phy = dev_get_drvdata(dev); + int i; + + reset_control_assert(cdns_phy->phy_rst); + reset_control_assert(cdns_phy->apb_rst); + for (i = 0; i < cdns_phy->nsubnodes; i++) + reset_control_assert(cdns_phy->phys[i].lnk_rst); + + if (!cdns_phy->already_configured) + clk_disable_unprepare(cdns_phy->clk); + else + cdns_phy->already_configured = 0; + + return 0; +} + +static int cdns_torrent_phy_resume_noirq(struct device *dev) +{ + struct cdns_torrent_phy *cdns_phy = dev_get_drvdata(dev); + int node = cdns_phy->nsubnodes; + int ret, i; + + ret = cdns_torrent_clk(cdns_phy); + if (ret) + goto clk_cleanup; + + /* Enable APB */ + reset_control_deassert(cdns_phy->apb_rst); + + if (cdns_phy->nsubnodes > 1) { + ret = cdns_torrent_phy_configure_multilink(cdns_phy); + if (ret) + goto put_lnk_rst; + } + + return 0; + +put_lnk_rst: + for (i = 0; i < node; i++) + reset_control_put(cdns_phy->phys[i].lnk_rst); + reset_control_assert(cdns_phy->apb_rst); + clk_disable_unprepare(cdns_phy->clk); +clk_cleanup: + cdns_torrent_clk_cleanup(cdns_phy); + return ret; +} + +static const struct dev_pm_ops cdns_torrent_phy_pm_ops = { + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(cdns_torrent_phy_suspend_noirq, + cdns_torrent_phy_resume_noirq) +}; +#endif + /* USB and DP link configuration */ static struct cdns_reg_pairs usb_dp_link_cmn_regs[] = { {0x0002, PHY_PLL_CFG}, @@ -4577,6 +4633,7 @@ static struct platform_driver cdns_torrent_phy_driver = { .driver = { .name = "cdns-torrent-phy", .of_match_table = cdns_torrent_phy_of_match, + .pm = pm_ptr(&cdns_torrent_phy_pm_ops), } }; module_platform_driver(cdns_torrent_phy_driver);
Add suspend and resume support. The alread_configured flag is cleared during suspend stage to force the phy initialization during the resume stage. Based on the work of Théo Lebrun <theo.lebrun@bootlin.com> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com> --- drivers/phy/cadence/phy-cadence-torrent.c | 57 +++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+)