Re: [PATCH v2] phy: qcom-snps: Add runtime suspend and resume handlers
On 6/24/2020 5:21 AM, Vinod Koul wrote: > Hi Wesley, > > On 21-05-20, 18:50, Wesley Cheng wrote: >> Allow for the PHY to be put into a powered down state when possible. >> Add the required suspend and resume callbacks, which will determine >> what resources can be turned off depending on the cable status. >> >> Signed-off-by: Wesley Cheng >> >> --- >> Changes in v2: >> - Addressed checkpatch alignment/line length warnings. >> - Removed superfluous init in qcom_snps_hsphy_resume(). >> >> drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 100 >> ++ >> 1 file changed, 100 insertions(+) >> >> diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c >> b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c >> index 4d74045..0a4e77af 100644 >> --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c >> +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c >> @@ -76,7 +76,9 @@ >> * @iface_clk: phy interface clock >> * @phy_reset: phy reset control >> * @vregs: regulator supplies bulk data >> + * @suspended: PHY is in the suspended state >> * @phy_initialized: if PHY has been initialized correctly >> + * @mode: contains the current mode the PHY is in >> */ >> struct qcom_snps_hsphy { >> struct phy *phy; >> @@ -87,7 +89,9 @@ struct qcom_snps_hsphy { >> struct reset_control *phy_reset; >> struct regulator_bulk_data vregs[SNPS_HS_NUM_VREGS]; >> >> +bool suspended; >> bool phy_initialized; >> +enum phy_mode mode; >> }; >> >> static inline void qcom_snps_hsphy_write_mask(void __iomem *base, u32 >> offset, >> @@ -104,6 +108,84 @@ static inline void qcom_snps_hsphy_write_mask(void >> __iomem *base, u32 offset, >> readl_relaxed(base + offset); >> } >> >> +static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy) >> +{ >> +if (hsphy->suspended) >> +return 0; > > Am still not convinced why this would be called when we are already > suspended :) > Hi Vinod, OK, we can remove it for now, and if its really required further on, we can re-add. >> + >> +dev_dbg(>phy->dev, "Suspend QCOM SNPS PHY, mode:%d\n", >> +hsphy->mode); > > Remove debug artifacts here? > Sure, can do that. >> + >> +if (hsphy->mode == PHY_MODE_USB_HOST) { >> +/* Enable auto-resume to meet remote wakeup timing */ >> +qcom_snps_hsphy_write_mask(hsphy->base, >> +USB2_PHY_USB_PHY_HS_PHY_CTRL2, >> +USB2_AUTO_RESUME, >> +USB2_AUTO_RESUME); > > Lets align the lines above to opening brace please.. > If you run checkpatch with --strict option you would get this CHECK: > Alignment should match open parenthesis > OK, got it. Fixed a few other spacing warnings as well. >> +usleep_range(500, 1000); >> +qcom_snps_hsphy_write_mask(hsphy->base, >> +USB2_PHY_USB_PHY_HS_PHY_CTRL2, >> +0, USB2_AUTO_RESUME); >> +} >> + >> +clk_disable_unprepare(hsphy->cfg_ahb_clk); >> +hsphy->suspended = true; >> + >> +return 0; >> +} >> + >> +static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy) >> +{ >> +int ret; >> + >> +if (!hsphy->suspended) >> +return 0; >> + >> +dev_dbg(>phy->dev, "Resume QCOM SNPS PHY, mode:%d\n", >> +hsphy->mode); > > here as well > Done. >> + >> +ret = clk_prepare_enable(hsphy->cfg_ahb_clk); >> +if (ret) { >> +dev_err(>phy->dev, >> +"failed to enable cfg ahb clock, %d\n", ret); > > single line should be okay now :) > Yep! >> +return ret; >> +} >> + >> +hsphy->suspended = false; >> +return 0; >> +} >> + >> +static int __maybe_unused qcom_snps_hsphy_runtime_suspend(struct device >> *dev) >> +{ >> +struct qcom_snps_hsphy *hsphy = dev_get_drvdata(dev); >> + >> +if (!hsphy->phy_initialized) >> +return 0; >> + >> +qcom_snps_hsphy_suspend(hsphy); >> +return 0; >> +} >> + >> +static int __maybe_unused qcom_snps_hsphy_runtime_resume(struct device *dev) >> +{ >> +struct qcom_snps_hsphy *hsphy = dev_get_drvdata(dev); >> + >> +if (!hsphy->phy_initialized) >> +return 0; >> + >> +qcom_snps_hsphy_resume(hsphy); >> +return 0; >> +} >> + >> +static int qcom_snps_hsphy_set_mode(struct phy *phy, enum phy_mode mode, >> +int submode) >> +{ >> +struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy); >> + >> +hsphy->mode = mode; >> +return 0; >> +} >> + >> static int qcom_snps_hsphy_init(struct phy *phy) >> { >> struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy); >> @@ -175,6 +257,7 @@ static int qcom_snps_hsphy_init(struct phy *phy) >> UTMI_PHY_CMN_CTRL_OVERRIDE_EN, 0); >> >> hsphy->phy_initialized = true; >>
Re: [PATCH v2] phy: qcom-snps: Add runtime suspend and resume handlers
Hi Wesley, On 21-05-20, 18:50, Wesley Cheng wrote: > Allow for the PHY to be put into a powered down state when possible. > Add the required suspend and resume callbacks, which will determine > what resources can be turned off depending on the cable status. > > Signed-off-by: Wesley Cheng > > --- > Changes in v2: > - Addressed checkpatch alignment/line length warnings. > - Removed superfluous init in qcom_snps_hsphy_resume(). > > drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 100 > ++ > 1 file changed, 100 insertions(+) > > diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > index 4d74045..0a4e77af 100644 > --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > @@ -76,7 +76,9 @@ > * @iface_clk: phy interface clock > * @phy_reset: phy reset control > * @vregs: regulator supplies bulk data > + * @suspended: PHY is in the suspended state > * @phy_initialized: if PHY has been initialized correctly > + * @mode: contains the current mode the PHY is in > */ > struct qcom_snps_hsphy { > struct phy *phy; > @@ -87,7 +89,9 @@ struct qcom_snps_hsphy { > struct reset_control *phy_reset; > struct regulator_bulk_data vregs[SNPS_HS_NUM_VREGS]; > > + bool suspended; > bool phy_initialized; > + enum phy_mode mode; > }; > > static inline void qcom_snps_hsphy_write_mask(void __iomem *base, u32 offset, > @@ -104,6 +108,84 @@ static inline void qcom_snps_hsphy_write_mask(void > __iomem *base, u32 offset, > readl_relaxed(base + offset); > } > > +static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy) > +{ > + if (hsphy->suspended) > + return 0; Am still not convinced why this would be called when we are already suspended :) > + > + dev_dbg(>phy->dev, "Suspend QCOM SNPS PHY, mode:%d\n", > + hsphy->mode); Remove debug artifacts here? > + > + if (hsphy->mode == PHY_MODE_USB_HOST) { > + /* Enable auto-resume to meet remote wakeup timing */ > + qcom_snps_hsphy_write_mask(hsphy->base, > + USB2_PHY_USB_PHY_HS_PHY_CTRL2, > + USB2_AUTO_RESUME, > + USB2_AUTO_RESUME); Lets align the lines above to opening brace please.. If you run checkpatch with --strict option you would get this CHECK: Alignment should match open parenthesis > + usleep_range(500, 1000); > + qcom_snps_hsphy_write_mask(hsphy->base, > + USB2_PHY_USB_PHY_HS_PHY_CTRL2, > + 0, USB2_AUTO_RESUME); > + } > + > + clk_disable_unprepare(hsphy->cfg_ahb_clk); > + hsphy->suspended = true; > + > + return 0; > +} > + > +static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy) > +{ > + int ret; > + > + if (!hsphy->suspended) > + return 0; > + > + dev_dbg(>phy->dev, "Resume QCOM SNPS PHY, mode:%d\n", > + hsphy->mode); here as well > + > + ret = clk_prepare_enable(hsphy->cfg_ahb_clk); > + if (ret) { > + dev_err(>phy->dev, > + "failed to enable cfg ahb clock, %d\n", ret); single line should be okay now :) > + return ret; > + } > + > + hsphy->suspended = false; > + return 0; > +} > + > +static int __maybe_unused qcom_snps_hsphy_runtime_suspend(struct device *dev) > +{ > + struct qcom_snps_hsphy *hsphy = dev_get_drvdata(dev); > + > + if (!hsphy->phy_initialized) > + return 0; > + > + qcom_snps_hsphy_suspend(hsphy); > + return 0; > +} > + > +static int __maybe_unused qcom_snps_hsphy_runtime_resume(struct device *dev) > +{ > + struct qcom_snps_hsphy *hsphy = dev_get_drvdata(dev); > + > + if (!hsphy->phy_initialized) > + return 0; > + > + qcom_snps_hsphy_resume(hsphy); > + return 0; > +} > + > +static int qcom_snps_hsphy_set_mode(struct phy *phy, enum phy_mode mode, > + int submode) > +{ > + struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy); > + > + hsphy->mode = mode; > + return 0; > +} > + > static int qcom_snps_hsphy_init(struct phy *phy) > { > struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy); > @@ -175,6 +257,7 @@ static int qcom_snps_hsphy_init(struct phy *phy) > UTMI_PHY_CMN_CTRL_OVERRIDE_EN, 0); > > hsphy->phy_initialized = true; > + hsphy->suspended = false; > > return 0; > > @@ -201,6 +284,7 @@ static int qcom_snps_hsphy_exit(struct phy *phy) > static const struct phy_ops qcom_snps_hsphy_gen_ops = { > .init = qcom_snps_hsphy_init, > .exit = qcom_snps_hsphy_exit, > + .set_mode = qcom_snps_hsphy_set_mode, This should be a
[PATCH v2] phy: qcom-snps: Add runtime suspend and resume handlers
Allow for the PHY to be put into a powered down state when possible. Add the required suspend and resume callbacks, which will determine what resources can be turned off depending on the cable status. Signed-off-by: Wesley Cheng --- Changes in v2: - Addressed checkpatch alignment/line length warnings. - Removed superfluous init in qcom_snps_hsphy_resume(). drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 100 ++ 1 file changed, 100 insertions(+) diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c index 4d74045..0a4e77af 100644 --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c @@ -76,7 +76,9 @@ * @iface_clk: phy interface clock * @phy_reset: phy reset control * @vregs: regulator supplies bulk data + * @suspended: PHY is in the suspended state * @phy_initialized: if PHY has been initialized correctly + * @mode: contains the current mode the PHY is in */ struct qcom_snps_hsphy { struct phy *phy; @@ -87,7 +89,9 @@ struct qcom_snps_hsphy { struct reset_control *phy_reset; struct regulator_bulk_data vregs[SNPS_HS_NUM_VREGS]; + bool suspended; bool phy_initialized; + enum phy_mode mode; }; static inline void qcom_snps_hsphy_write_mask(void __iomem *base, u32 offset, @@ -104,6 +108,84 @@ static inline void qcom_snps_hsphy_write_mask(void __iomem *base, u32 offset, readl_relaxed(base + offset); } +static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy) +{ + if (hsphy->suspended) + return 0; + + dev_dbg(>phy->dev, "Suspend QCOM SNPS PHY, mode:%d\n", + hsphy->mode); + + if (hsphy->mode == PHY_MODE_USB_HOST) { + /* Enable auto-resume to meet remote wakeup timing */ + qcom_snps_hsphy_write_mask(hsphy->base, + USB2_PHY_USB_PHY_HS_PHY_CTRL2, + USB2_AUTO_RESUME, + USB2_AUTO_RESUME); + usleep_range(500, 1000); + qcom_snps_hsphy_write_mask(hsphy->base, + USB2_PHY_USB_PHY_HS_PHY_CTRL2, + 0, USB2_AUTO_RESUME); + } + + clk_disable_unprepare(hsphy->cfg_ahb_clk); + hsphy->suspended = true; + + return 0; +} + +static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy) +{ + int ret; + + if (!hsphy->suspended) + return 0; + + dev_dbg(>phy->dev, "Resume QCOM SNPS PHY, mode:%d\n", + hsphy->mode); + + ret = clk_prepare_enable(hsphy->cfg_ahb_clk); + if (ret) { + dev_err(>phy->dev, + "failed to enable cfg ahb clock, %d\n", ret); + return ret; + } + + hsphy->suspended = false; + return 0; +} + +static int __maybe_unused qcom_snps_hsphy_runtime_suspend(struct device *dev) +{ + struct qcom_snps_hsphy *hsphy = dev_get_drvdata(dev); + + if (!hsphy->phy_initialized) + return 0; + + qcom_snps_hsphy_suspend(hsphy); + return 0; +} + +static int __maybe_unused qcom_snps_hsphy_runtime_resume(struct device *dev) +{ + struct qcom_snps_hsphy *hsphy = dev_get_drvdata(dev); + + if (!hsphy->phy_initialized) + return 0; + + qcom_snps_hsphy_resume(hsphy); + return 0; +} + +static int qcom_snps_hsphy_set_mode(struct phy *phy, enum phy_mode mode, + int submode) +{ + struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy); + + hsphy->mode = mode; + return 0; +} + static int qcom_snps_hsphy_init(struct phy *phy) { struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy); @@ -175,6 +257,7 @@ static int qcom_snps_hsphy_init(struct phy *phy) UTMI_PHY_CMN_CTRL_OVERRIDE_EN, 0); hsphy->phy_initialized = true; + hsphy->suspended = false; return 0; @@ -201,6 +284,7 @@ static int qcom_snps_hsphy_exit(struct phy *phy) static const struct phy_ops qcom_snps_hsphy_gen_ops = { .init = qcom_snps_hsphy_init, .exit = qcom_snps_hsphy_exit, + .set_mode = qcom_snps_hsphy_set_mode, .owner = THIS_MODULE, }; @@ -212,6 +296,11 @@ static int qcom_snps_hsphy_exit(struct phy *phy) }; MODULE_DEVICE_TABLE(of, qcom_snps_hsphy_of_match_table); +static const struct dev_pm_ops qcom_snps_hsphy_pm_ops = { + SET_RUNTIME_PM_OPS(qcom_snps_hsphy_runtime_suspend, + qcom_snps_hsphy_runtime_resume, NULL) +}; + static int qcom_snps_hsphy_probe(struct platform_device *pdev) { struct device *dev = >dev; @@ -255,6 +344,14 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev)