| Message ID | 1587662818-4461-1-git-send-email-wcheng@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | phy: qcom-snps: Add runtime suspend and resume handlers | expand |
On 23-04-20, 10:26, Wesley Cheng wrote: > +static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy) > +{ > + if (hsphy->suspended) > + return 0; > + > + dev_dbg(&hsphy->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); Kernel has a coding guideline where we try to "stick" to 80 char limit and is sometimes okay like debug logs. Above is not okay. Please fix it and run ./scripts/checkpatch.pl --strict on your patch and fix all errors. Warning and checks at your discretion using common sense. When in doubt do ask :) > + } > + > + clk_disable_unprepare(hsphy->cfg_ahb_clk); > + hsphy->suspended = true; why do you need to track this? > + > + return 0; > +} > + > +static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy) > +{ > + int ret = 0; superfluous init.. > static int qcom_snps_hsphy_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -251,6 +333,14 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev) > return ret; > } > > + pm_runtime_set_active(dev); > + pm_runtime_enable(dev); would it not make sense to enable this after pjy in initialized?
On 4/27/2020 9:59 AM, Vinod Koul wrote: > On 23-04-20, 10:26, Wesley Cheng wrote: > >> +static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy) >> +{ >> + if (hsphy->suspended) >> + return 0; >> + >> + dev_dbg(&hsphy->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); > > Kernel has a coding guideline where we try to "stick" to 80 char limit > and is sometimes okay like debug logs. Above is not okay. Please fix it > and run ./scripts/checkpatch.pl --strict on your patch and fix all > errors. Warning and checks at your discretion using common sense. When > in doubt do ask :) > Hi Vinod, Thanks for the input. I do run the checkpatch script before sending patches, and addressing the errors. However, seems this was tagged as a warning instead. Will keep in mind to address as many warnings as I can as well. >> + } >> + >> + clk_disable_unprepare(hsphy->cfg_ahb_clk); >> + hsphy->suspended = true; > > why do you need to track this? > More for debug purposes in case the RPM state becomes out of sync with the expected PHY state. We've seen some situations during PM suspend/resume testing where our RPM routines aren't executed, as PM will disable RPM briefly. It would be nice to be able to catch these situations after collecting our crash dumps. >> + >> + return 0; >> +} >> + >> +static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy) >> +{ >> + int ret = 0; > > superfluous init.. > Agreed. >> static int qcom_snps_hsphy_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> @@ -251,6 +333,14 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev) >> return ret; >> } >> >> + pm_runtime_set_active(dev); >> + pm_runtime_enable(dev); > > would it not make sense to enable this after pjy in initialized? > Not sure we want to put this in the phy_init() callback, as we can't guarantee how the driver registering the PHY will use it. It'll put the requirement of having to call phy_exit() for every phy_init() sequence, in order to avoid unbalanced disable_depth warnings from the RPM driver.
diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c index 20442a3..f48d877 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,77 @@ 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(&hsphy->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 = 0; + + if (!hsphy->suspended) + return 0; + + dev_dbg(&hsphy->phy->dev, "Resume QCOM SNPS PHY, mode = %d \n", hsphy->mode); + + ret = clk_prepare_enable(hsphy->cfg_ahb_clk); + if (ret) { + dev_err(&hsphy->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); @@ -171,6 +246,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; @@ -197,6 +273,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, }; @@ -208,6 +285,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 = &pdev->dev; @@ -251,6 +333,14 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev) return ret; } + pm_runtime_set_active(dev); + pm_runtime_enable(dev); + /* + * Prevent runtime pm from being ON by default. Users can enable + * it using power/control in sysfs. + */ + pm_runtime_forbid(dev); + generic_phy = devm_phy_create(dev, NULL, &qcom_snps_hsphy_gen_ops); if (IS_ERR(generic_phy)) { ret = PTR_ERR(generic_phy); @@ -265,6 +355,8 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev) phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate); if (!IS_ERR(phy_provider)) dev_dbg(dev, "Registered Qcom-SNPS HS phy\n"); + else + pm_runtime_disable(dev); return PTR_ERR_OR_ZERO(phy_provider); } @@ -273,6 +365,7 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev) .probe = qcom_snps_hsphy_probe, .driver = { .name = "qcom-snps-hs-femto-v2-phy", + .pm = &qcom_snps_hsphy_pm_ops, .of_match_table = qcom_snps_hsphy_of_match_table, }, };
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 <wcheng@codeaurora.org> --- Depends-on: phy: qcom-snps: Add SNPS USB PHY driver for QCOM based SOCs (https://patchwork.kernel.org/patch/11486171/) --- drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 93 +++++++++++++++++++++++++++ 1 file changed, 93 insertions(+)