| Message ID | 20230605184455.34832-3-athierry@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Clock fixes for qcom-snps-femto-v2 PHY driver | expand |
On Mon, Jun 05, 2023 at 02:44:54PM -0400, Adrien Thierry wrote: > Add the system sleep suspend and resume callbacks, reusing the same code > as the runtime suspend and resume callbacks. It would be nice (imo) to reference the downstream driver you used as a reference if you spin a v3. > > Signed-off-by: Adrien Thierry <athierry@redhat.com> > --- > I'm still a bit confused as to what the difference is between > suspend/resume PM ops and the struct usb_phy set_suspend() callback. > Please tell me if I should be populating the latter instead. My understanding is usb_phy is a legacy thing, and that the generic phys are the way to go (which this is). Looking at dwc3 for example, you can see it uses: phy_power_off(dwc->usb3_generic_phy); phy_power_off(dwc->usb2_generic_phy); usb_phy_set_suspend(dwc->usb3_phy, 1); usb_phy_set_suspend(dwc->usb2_phy, 1); so it handles both the generic phy (like this) and the usb_phy. This driver doesn't implement the power_off callback, but phy_power_off is also pm_runtime aware, so calling phy_power_off in a controller still achieves similar behavior (although I'm following right phy_init/phy_exit also are pm_runtime aware, so I think it would be at that point that the ops get called). This all assumes a user enables pm_runtime in sysfs for this driver. > > drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > index ce1d2f8b568a..378a5029f61e 100644 > --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > @@ -179,7 +179,7 @@ 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) > +static int qcom_snps_hsphy_do_suspend(struct qcom_snps_hsphy *hsphy) > { > dev_dbg(&hsphy->phy->dev, "Suspend QCOM SNPS PHY\n"); > > @@ -199,7 +199,7 @@ static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy) > return 0; > } > > -static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy) > +static int qcom_snps_hsphy_do_resume(struct qcom_snps_hsphy *hsphy) > { > int ret; > > @@ -214,25 +214,25 @@ static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy) > return 0; > } > > -static int __maybe_unused qcom_snps_hsphy_runtime_suspend(struct device *dev) > +static int __maybe_unused qcom_snps_hsphy_suspend(struct device *dev) > { > struct qcom_snps_hsphy *hsphy = dev_get_drvdata(dev); > > if (!hsphy->phy_initialized) > return 0; > > - qcom_snps_hsphy_suspend(hsphy); > + qcom_snps_hsphy_do_suspend(hsphy); Deserves its own patch, but can you return qcom_snps_hsphy_do_suspend/resume as part of your clean up in this series? > return 0; > } > > -static int __maybe_unused qcom_snps_hsphy_runtime_resume(struct device *dev) > +static int __maybe_unused qcom_snps_hsphy_resume(struct device *dev) > { > struct qcom_snps_hsphy *hsphy = dev_get_drvdata(dev); > > if (!hsphy->phy_initialized) > return 0; > > - qcom_snps_hsphy_resume(hsphy); > + qcom_snps_hsphy_do_resume(hsphy); > return 0; > } > > @@ -518,8 +518,10 @@ static const struct of_device_id qcom_snps_hsphy_of_match_table[] = { > 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) > + SET_RUNTIME_PM_OPS(qcom_snps_hsphy_suspend, > + qcom_snps_hsphy_resume, NULL) > + SET_SYSTEM_SLEEP_PM_OPS(qcom_snps_hsphy_suspend, > + qcom_snps_hsphy_resume) > }; > > static void qcom_snps_hsphy_override_param_update_val( > -- > 2.40.1 >
diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c index ce1d2f8b568a..378a5029f61e 100644 --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c @@ -179,7 +179,7 @@ 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) +static int qcom_snps_hsphy_do_suspend(struct qcom_snps_hsphy *hsphy) { dev_dbg(&hsphy->phy->dev, "Suspend QCOM SNPS PHY\n"); @@ -199,7 +199,7 @@ static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy) return 0; } -static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy) +static int qcom_snps_hsphy_do_resume(struct qcom_snps_hsphy *hsphy) { int ret; @@ -214,25 +214,25 @@ static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy) return 0; } -static int __maybe_unused qcom_snps_hsphy_runtime_suspend(struct device *dev) +static int __maybe_unused qcom_snps_hsphy_suspend(struct device *dev) { struct qcom_snps_hsphy *hsphy = dev_get_drvdata(dev); if (!hsphy->phy_initialized) return 0; - qcom_snps_hsphy_suspend(hsphy); + qcom_snps_hsphy_do_suspend(hsphy); return 0; } -static int __maybe_unused qcom_snps_hsphy_runtime_resume(struct device *dev) +static int __maybe_unused qcom_snps_hsphy_resume(struct device *dev) { struct qcom_snps_hsphy *hsphy = dev_get_drvdata(dev); if (!hsphy->phy_initialized) return 0; - qcom_snps_hsphy_resume(hsphy); + qcom_snps_hsphy_do_resume(hsphy); return 0; } @@ -518,8 +518,10 @@ static const struct of_device_id qcom_snps_hsphy_of_match_table[] = { 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) + SET_RUNTIME_PM_OPS(qcom_snps_hsphy_suspend, + qcom_snps_hsphy_resume, NULL) + SET_SYSTEM_SLEEP_PM_OPS(qcom_snps_hsphy_suspend, + qcom_snps_hsphy_resume) }; static void qcom_snps_hsphy_override_param_update_val(
Add the system sleep suspend and resume callbacks, reusing the same code as the runtime suspend and resume callbacks. Signed-off-by: Adrien Thierry <athierry@redhat.com> --- I'm still a bit confused as to what the difference is between suspend/resume PM ops and the struct usb_phy set_suspend() callback. Please tell me if I should be populating the latter instead. drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)