| Message ID | 20240102-j7200-pcie-s2r-v3-14-5c2e4a3fac1f@bootlin.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add suspend to ram support for PCIe on J7200 | expand |
On Thu, Feb 15, 2024 at 04:17:59PM +0100, Thomas Richard wrote: > Add suspend and resume support. > > The already_configured flag is cleared during the suspend stage to force > the PHY initialization during the resume stage. > Based on the work of Théo Lebrun <theo.lebrun@bootlin.com> SoB/Co-developed-by ? ... > +static int cdns_torrent_phy_suspend_noirq(struct device *dev) > +{ > + struct cdns_torrent_phy *cdns_phy = dev_get_drvdata(dev); > + int i; Why signed? > + 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) > + cdns_phy->already_configured = 0; > + else > + clk_disable_unprepare(cdns_phy->clk); > + > + 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; Ditto. > + 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_assert(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; > +}
On 2/15/24 16:46, Andy Shevchenko wrote: >> +static int cdns_torrent_phy_suspend_noirq(struct device *dev) >> +{ >> + struct cdns_torrent_phy *cdns_phy = dev_get_drvdata(dev); >> + int i; > > Why signed? In the for loop below, the i variable is compared to cdns_phy->nsubnodes, which is an int. https://elixir.bootlin.com/linux/latest/source/drivers/phy/cadence/phy-cadence-torrent.c#L360 > >> + 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) >> + cdns_phy->already_configured = 0; >> + else >> + clk_disable_unprepare(cdns_phy->clk); >> + >> + 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; > > Ditto> Same reason >> + 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_assert(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; >> +} >
On Do, 2024-02-15 at 16:17 +0100, Thomas Richard wrote: > Add suspend and resume support. > > The already_configured flag is cleared during the 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 | 54 +++++++++++++++++++++++++++++++ > 1 file changed, 54 insertions(+) > > diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c > index 52cadca4c07b..f8945a11e7ca 100644 > --- a/drivers/phy/cadence/phy-cadence-torrent.c > +++ b/drivers/phy/cadence/phy-cadence-torrent.c > @@ -3005,6 +3005,59 @@ static void cdns_torrent_phy_remove(struct platform_device *pdev) > cdns_torrent_clk_cleanup(cdns_phy); > } > > +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) > + cdns_phy->already_configured = 0; > + else > + clk_disable_unprepare(cdns_phy->clk); > + > + 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_assert(cdns_phy->phys[i].lnk_rst); The same cleanup is found in probe. Would it be cleaner to move this into cdns_torrent_phy_configure_multilink() instead of duplicating it here? > + reset_control_assert(cdns_phy->apb_rst); > + clk_disable_unprepare(cdns_phy->clk); > +clk_cleanup: > + cdns_torrent_clk_cleanup(cdns_phy); This calls of_clk_del_provider(), seems misplaced here. regards Philipp
On 2/21/24 14:09, Philipp Zabel wrote: > On Do, 2024-02-15 at 16:17 +0100, Thomas Richard wrote: >> Add suspend and resume support. >> >> The already_configured flag is cleared during the 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 | 54 +++++++++++++++++++++++++++++++ >> 1 file changed, 54 insertions(+) >> >> diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c >> index 52cadca4c07b..f8945a11e7ca 100644 >> --- a/drivers/phy/cadence/phy-cadence-torrent.c >> +++ b/drivers/phy/cadence/phy-cadence-torrent.c >> @@ -3005,6 +3005,59 @@ static void cdns_torrent_phy_remove(struct platform_device *pdev) >> cdns_torrent_clk_cleanup(cdns_phy); >> } >> >> +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) >> + cdns_phy->already_configured = 0; >> + else >> + clk_disable_unprepare(cdns_phy->clk); >> + >> + 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_assert(cdns_phy->phys[i].lnk_rst); > > The same cleanup is found in probe. Would it be cleaner to move this > into cdns_torrent_phy_configure_multilink() instead of duplicating it > here? Hello Philipp, Yes I could, but from my point of view, it would not be cleaner. This cleanup is called from many places in the probe: - https://elixir.bootlin.com/linux/v6.8-rc5/source/drivers/phy/cadence/phy-cadence-torrent.c#L2948 - https://elixir.bootlin.com/linux/v6.8-rc5/source/drivers/phy/cadence/phy-cadence-torrent.c#L2954 - https://elixir.bootlin.com/linux/v6.8-rc5/source/drivers/phy/cadence/phy-cadence-torrent.c#L2960 If I add this cleanup in cdns_torrent_phy_configure_multilink(), yes I could remove it from cdns_torrent_phy_resume_noirq(), but I should keep it in the probe. And I should modify the probe to jump to clk_cleanup if cdns_torrent_phy_configure_multilink() fails. > >> + reset_control_assert(cdns_phy->apb_rst); >> + clk_disable_unprepare(cdns_phy->clk); >> +clk_cleanup: >> + cdns_torrent_clk_cleanup(cdns_phy); > > This calls of_clk_del_provider(), seems misplaced here. Yes you're right, it's called in cdns_torrent_phy_remove(). So I should not call it in the resume callback, this will cause some issues during the remove. Regards,
On Mi, 2024-02-21 at 14:41 +0100, Thomas Richard wrote: > On 2/21/24 14:09, Philipp Zabel wrote: > > On Do, 2024-02-15 at 16:17 +0100, Thomas Richard wrote: > > > Add suspend and resume support. > > > > > > The already_configured flag is cleared during the 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 | 54 +++++++++++++++++++++++++++++++ > > > 1 file changed, 54 insertions(+) > > > > > > diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c > > > index 52cadca4c07b..f8945a11e7ca 100644 > > > --- a/drivers/phy/cadence/phy-cadence-torrent.c > > > +++ b/drivers/phy/cadence/phy-cadence-torrent.c > > > @@ -3005,6 +3005,59 @@ static void cdns_torrent_phy_remove(struct platform_device *pdev) > > > cdns_torrent_clk_cleanup(cdns_phy); > > > } > > > > > > +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) > > > + cdns_phy->already_configured = 0; > > > + else > > > + clk_disable_unprepare(cdns_phy->clk); > > > + > > > + 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_assert(cdns_phy->phys[i].lnk_rst); > > > > The same cleanup is found in probe. Would it be cleaner to move this > > into cdns_torrent_phy_configure_multilink() instead of duplicating it > > here? > > Hello Philipp, > > Yes I could, but from my point of view, it would not be cleaner. > This cleanup is called from many places in the probe: > - > https://elixir.bootlin.com/linux/v6.8-rc5/source/drivers/phy/cadence/phy-cadence-torrent.c#L2948 > - > https://elixir.bootlin.com/linux/v6.8-rc5/source/drivers/phy/cadence/phy-cadence-torrent.c#L2954 > - > https://elixir.bootlin.com/linux/v6.8-rc5/source/drivers/phy/cadence/phy-cadence-torrent.c#L2960 > > If I add this cleanup in cdns_torrent_phy_configure_multilink(), yes I > could remove it from cdns_torrent_phy_resume_noirq(), but I should keep > it in the probe. And I should modify the probe to jump to clk_cleanup if > cdns_torrent_phy_configure_multilink() fails. I see it now. If it can't be consolidated, it's not useful to move it around. > regards Philipp
diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c index 52cadca4c07b..f8945a11e7ca 100644 --- a/drivers/phy/cadence/phy-cadence-torrent.c +++ b/drivers/phy/cadence/phy-cadence-torrent.c @@ -3005,6 +3005,59 @@ static void cdns_torrent_phy_remove(struct platform_device *pdev) cdns_torrent_clk_cleanup(cdns_phy); } +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) + cdns_phy->already_configured = 0; + else + clk_disable_unprepare(cdns_phy->clk); + + 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_assert(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 DEFINE_NOIRQ_DEV_PM_OPS(cdns_torrent_phy_pm_ops, + cdns_torrent_phy_suspend_noirq, + cdns_torrent_phy_resume_noirq); + /* USB and DP link configuration */ static struct cdns_reg_pairs usb_dp_link_cmn_regs[] = { {0x0002, PHY_PLL_CFG}, @@ -4576,6 +4629,7 @@ static struct platform_driver cdns_torrent_phy_driver = { .driver = { .name = "cdns-torrent-phy", .of_match_table = cdns_torrent_phy_of_match, + .pm = pm_sleep_ptr(&cdns_torrent_phy_pm_ops), } }; module_platform_driver(cdns_torrent_phy_driver);
Add suspend and resume support. The already_configured flag is cleared during the 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 | 54 +++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+)