| Message ID | 20240907-phy-qcom-edp-enable-runtime-pm-v1-1-8b9ee4210e1e@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | phy: qcom: edp: Add runtime PM support | expand |
On Sat, 7 Sept 2024 at 18:25, Abel Vesa <abel.vesa@linaro.org> wrote: > > Enable runtime PM support by adding proper ops which will handle the > clocks and regulators. These resources will now be handled on power_on and > power_off instead of init and exit PHY ops. Also enable these resources on > probe in order to balance out the disabling that is happening right after. > Prevent runtime PM from being ON by default as well. > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- > drivers/phy/qualcomm/phy-qcom-edp.c | 105 ++++++++++++++++++++++++++---------- > 1 file changed, 77 insertions(+), 28 deletions(-) > > diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c b/drivers/phy/qualcomm/phy-qcom-edp.c > index da2b32fb5b45..3affeef261bf 100644 > --- a/drivers/phy/qualcomm/phy-qcom-edp.c > +++ b/drivers/phy/qualcomm/phy-qcom-edp.c > @@ -192,14 +192,6 @@ static int qcom_edp_phy_init(struct phy *phy) > int ret; > u8 cfg8; > > - ret = regulator_bulk_enable(ARRAY_SIZE(edp->supplies), edp->supplies); > - if (ret) > - return ret; > - > - ret = clk_bulk_prepare_enable(ARRAY_SIZE(edp->clks), edp->clks); > - if (ret) > - goto out_disable_supplies; > - > writel(DP_PHY_PD_CTL_PWRDN | DP_PHY_PD_CTL_AUX_PWRDN | > DP_PHY_PD_CTL_PLL_PWRDN | DP_PHY_PD_CTL_DP_CLAMP_EN, > edp->edp + DP_PHY_PD_CTL); > @@ -246,11 +238,6 @@ static int qcom_edp_phy_init(struct phy *phy) > msleep(20); > > return 0; > - > -out_disable_supplies: > - regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies); > - > - return ret; > } > > static int qcom_edp_set_voltages(struct qcom_edp *edp, const struct phy_configure_opts_dp *dp_opts) > @@ -721,6 +708,8 @@ static int qcom_edp_phy_power_on(struct phy *phy) > u32 val; > u8 cfg1; > > + pm_runtime_get_sync(&phy->dev); > + > ret = edp->cfg->ver_ops->com_power_on(edp); > if (ret) > return ret; > @@ -841,6 +830,8 @@ static int qcom_edp_phy_power_off(struct phy *phy) > > writel(DP_PHY_PD_CTL_PSR_PWRDN, edp->edp + DP_PHY_PD_CTL); > > + pm_runtime_put(&phy->dev); > + > return 0; > } > > @@ -856,23 +847,12 @@ static int qcom_edp_phy_set_mode(struct phy *phy, enum phy_mode mode, int submod > return 0; > } > > -static int qcom_edp_phy_exit(struct phy *phy) > -{ > - struct qcom_edp *edp = phy_get_drvdata(phy); > - > - clk_bulk_disable_unprepare(ARRAY_SIZE(edp->clks), edp->clks); > - regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies); > - > - return 0; > -} > - > static const struct phy_ops qcom_edp_ops = { > .init = qcom_edp_phy_init, > .configure = qcom_edp_phy_configure, > .power_on = qcom_edp_phy_power_on, > .power_off = qcom_edp_phy_power_off, > .set_mode = qcom_edp_phy_set_mode, > - .exit = qcom_edp_phy_exit, > .owner = THIS_MODULE, > }; > > @@ -1036,6 +1016,32 @@ static int qcom_edp_clks_register(struct qcom_edp *edp, struct device_node *np) > return devm_of_clk_add_hw_provider(edp->dev, of_clk_hw_onecell_get, data); > } > > +static int __maybe_unused qcom_edp_runtime_suspend(struct device *dev) > +{ > + struct qcom_edp *edp = dev_get_drvdata(dev); > + > + dev_err(dev, "Suspending DP phy\n"); Debug leftovers? > + > + clk_bulk_disable_unprepare(ARRAY_SIZE(edp->clks), edp->clks); > + regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies); > + > + return 0; > +} > + > +static int __maybe_unused qcom_edp_runtime_resume(struct device *dev) > +{ > + struct qcom_edp *edp = dev_get_drvdata(dev); > + int ret; > + > + dev_err(dev, "Resuming DP phy\n"); Debug leftovers? > + > + ret = regulator_bulk_enable(ARRAY_SIZE(edp->supplies), edp->supplies); > + if (ret) > + return ret; > + > + return clk_bulk_prepare_enable(ARRAY_SIZE(edp->clks), edp->clks); Missing error handling > +} > + > static int qcom_edp_phy_probe(struct platform_device *pdev) > { > struct phy_provider *phy_provider; > @@ -1091,20 +1097,57 @@ static int qcom_edp_phy_probe(struct platform_device *pdev) > return ret; > } > > - ret = qcom_edp_clks_register(edp, pdev->dev.of_node); > - if (ret) > + ret = regulator_bulk_enable(ARRAY_SIZE(edp->supplies), edp->supplies); > + if (ret) { > + dev_err(dev, "failed to enable regulators, err=%d\n", ret); > return ret; > + } > + > + ret = clk_bulk_prepare_enable(ARRAY_SIZE(edp->clks), edp->clks); > + if (ret) { > + dev_err(dev, "failed to enable clocks, err=%d\n", ret); > + goto err_disable_regulators; > + } Please use pm_runtime_get_sync() instead(). > + > + ret = qcom_edp_clks_register(edp, pdev->dev.of_node); > + if (ret) { > + dev_err(dev, "failed to register PHY clocks, err=%d\n", ret); > + goto err_disable_clocks; > + } > > edp->phy = devm_phy_create(dev, pdev->dev.of_node, &qcom_edp_ops); > if (IS_ERR(edp->phy)) { > dev_err(dev, "failed to register phy\n"); > - return PTR_ERR(edp->phy); > + ret = PTR_ERR(edp->phy); > + goto err_disable_clocks; > } > > + pm_runtime_set_active(dev); > + ret = devm_pm_runtime_enable(dev); If this handles earlier, you don't need to call pm_runtime_set_active() manually > + if (ret) > + goto err_disable_clocks; > + /* > + * Prevent runtime pm from being ON by default. Users can enable > + * it using power/control in sysfs. why? > + */ > + pm_runtime_forbid(dev); > + > + dev_set_drvdata(dev, edp); > phy_set_drvdata(edp->phy, edp); > > phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate); > - return PTR_ERR_OR_ZERO(phy_provider); > + if (IS_ERR(phy_provider)) > + goto err_disable_clocks; > + > + return 0; > + > +err_disable_clocks: > + clk_bulk_disable_unprepare(ARRAY_SIZE(edp->clks), edp->clks); > + > +err_disable_regulators: > + regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies); Ideally this should be handled by pm_runtime. Or at least by pm_runtime_put(). > + > + return ret; > } > > static const struct of_device_id qcom_edp_phy_match_table[] = { > @@ -1117,10 +1160,16 @@ static const struct of_device_id qcom_edp_phy_match_table[] = { > }; > MODULE_DEVICE_TABLE(of, qcom_edp_phy_match_table); > > +static const struct dev_pm_ops qcom_edp_pm_ops = { > + SET_RUNTIME_PM_OPS(qcom_edp_runtime_suspend, > + qcom_edp_runtime_resume, NULL) > +}; > + > static struct platform_driver qcom_edp_phy_driver = { > .probe = qcom_edp_phy_probe, > .driver = { > .name = "qcom-edp-phy", > + .pm = &qcom_edp_pm_ops, > .of_match_table = qcom_edp_phy_match_table, > }, > }; > > --- > base-commit: 9aaeb87ce1e966169a57f53a02ba05b30880ffb8 > change-id: 20240907-phy-qcom-edp-enable-runtime-pm-6fad07af8947 > > Best regards, > -- > Abel Vesa <abel.vesa@linaro.org> >
On Sat, Sep 07, 2024 at 06:25:21PM GMT, Abel Vesa wrote: > Enable runtime PM support by adding proper ops which will handle the > clocks and regulators. These resources will now be handled on power_on and > power_off instead of init and exit PHY ops. Also enable these resources on > probe in order to balance out the disabling that is happening right after. Sounds good, I assume there's a good reason for doing this? Please provide a proper problem description, as defined in: https://docs.kernel.org/process/submitting-patches.html#describe-your-changes > Prevent runtime PM from being ON by default as well. > Why? > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- > drivers/phy/qualcomm/phy-qcom-edp.c | 105 ++++++++++++++++++++++++++---------- > 1 file changed, 77 insertions(+), 28 deletions(-) > > diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c b/drivers/phy/qualcomm/phy-qcom-edp.c > index da2b32fb5b45..3affeef261bf 100644 > --- a/drivers/phy/qualcomm/phy-qcom-edp.c > +++ b/drivers/phy/qualcomm/phy-qcom-edp.c > @@ -192,14 +192,6 @@ static int qcom_edp_phy_init(struct phy *phy) > int ret; > u8 cfg8; > > - ret = regulator_bulk_enable(ARRAY_SIZE(edp->supplies), edp->supplies); > - if (ret) > - return ret; > - > - ret = clk_bulk_prepare_enable(ARRAY_SIZE(edp->clks), edp->clks); > - if (ret) > - goto out_disable_supplies; > - > writel(DP_PHY_PD_CTL_PWRDN | DP_PHY_PD_CTL_AUX_PWRDN | > DP_PHY_PD_CTL_PLL_PWRDN | DP_PHY_PD_CTL_DP_CLAMP_EN, > edp->edp + DP_PHY_PD_CTL); > @@ -246,11 +238,6 @@ static int qcom_edp_phy_init(struct phy *phy) > msleep(20); > > return 0; > - > -out_disable_supplies: > - regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies); > - > - return ret; > } > > static int qcom_edp_set_voltages(struct qcom_edp *edp, const struct phy_configure_opts_dp *dp_opts) > @@ -721,6 +708,8 @@ static int qcom_edp_phy_power_on(struct phy *phy) > u32 val; > u8 cfg1; > > + pm_runtime_get_sync(&phy->dev); > + > ret = edp->cfg->ver_ops->com_power_on(edp); > if (ret) > return ret; > @@ -841,6 +830,8 @@ static int qcom_edp_phy_power_off(struct phy *phy) > > writel(DP_PHY_PD_CTL_PSR_PWRDN, edp->edp + DP_PHY_PD_CTL); > > + pm_runtime_put(&phy->dev); > + > return 0; > } > > @@ -856,23 +847,12 @@ static int qcom_edp_phy_set_mode(struct phy *phy, enum phy_mode mode, int submod > return 0; > } > > -static int qcom_edp_phy_exit(struct phy *phy) > -{ > - struct qcom_edp *edp = phy_get_drvdata(phy); > - > - clk_bulk_disable_unprepare(ARRAY_SIZE(edp->clks), edp->clks); > - regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies); > - > - return 0; > -} > - > static const struct phy_ops qcom_edp_ops = { > .init = qcom_edp_phy_init, > .configure = qcom_edp_phy_configure, > .power_on = qcom_edp_phy_power_on, > .power_off = qcom_edp_phy_power_off, > .set_mode = qcom_edp_phy_set_mode, > - .exit = qcom_edp_phy_exit, > .owner = THIS_MODULE, > }; > > @@ -1036,6 +1016,32 @@ static int qcom_edp_clks_register(struct qcom_edp *edp, struct device_node *np) > return devm_of_clk_add_hw_provider(edp->dev, of_clk_hw_onecell_get, data); > } > > +static int __maybe_unused qcom_edp_runtime_suspend(struct device *dev) > +{ > + struct qcom_edp *edp = dev_get_drvdata(dev); > + > + dev_err(dev, "Suspending DP phy\n"); > + > + clk_bulk_disable_unprepare(ARRAY_SIZE(edp->clks), edp->clks); > + regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies); > + > + return 0; > +} > + > +static int __maybe_unused qcom_edp_runtime_resume(struct device *dev) > +{ > + struct qcom_edp *edp = dev_get_drvdata(dev); > + int ret; > + > + dev_err(dev, "Resuming DP phy\n"); > + > + ret = regulator_bulk_enable(ARRAY_SIZE(edp->supplies), edp->supplies); > + if (ret) > + return ret; > + > + return clk_bulk_prepare_enable(ARRAY_SIZE(edp->clks), edp->clks); > +} > + > static int qcom_edp_phy_probe(struct platform_device *pdev) > { > struct phy_provider *phy_provider; > @@ -1091,20 +1097,57 @@ static int qcom_edp_phy_probe(struct platform_device *pdev) > return ret; > } > > - ret = qcom_edp_clks_register(edp, pdev->dev.of_node); > - if (ret) > + ret = regulator_bulk_enable(ARRAY_SIZE(edp->supplies), edp->supplies); > + if (ret) { > + dev_err(dev, "failed to enable regulators, err=%d\n", ret); > return ret; > + } > + > + ret = clk_bulk_prepare_enable(ARRAY_SIZE(edp->clks), edp->clks); > + if (ret) { > + dev_err(dev, "failed to enable clocks, err=%d\n", ret); > + goto err_disable_regulators; > + } > + > + ret = qcom_edp_clks_register(edp, pdev->dev.of_node); > + if (ret) { > + dev_err(dev, "failed to register PHY clocks, err=%d\n", ret); > + goto err_disable_clocks; > + } > > edp->phy = devm_phy_create(dev, pdev->dev.of_node, &qcom_edp_ops); > if (IS_ERR(edp->phy)) { > dev_err(dev, "failed to register phy\n"); > - return PTR_ERR(edp->phy); > + ret = PTR_ERR(edp->phy); > + goto err_disable_clocks; > } > > + pm_runtime_set_active(dev); > + ret = devm_pm_runtime_enable(dev); > + if (ret) > + goto err_disable_clocks; > + /* > + * Prevent runtime pm from being ON by default. Users can enable > + * it using power/control in sysfs. That is what this call do, please describe why it's done instead. Regards, Bjorn > + */ > + pm_runtime_forbid(dev); > + > + dev_set_drvdata(dev, edp); > phy_set_drvdata(edp->phy, edp); > > phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate); > - return PTR_ERR_OR_ZERO(phy_provider); > + if (IS_ERR(phy_provider)) > + goto err_disable_clocks; > + > + return 0; > + > +err_disable_clocks: > + clk_bulk_disable_unprepare(ARRAY_SIZE(edp->clks), edp->clks); > + > +err_disable_regulators: > + regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies); > + > + return ret; > } > > static const struct of_device_id qcom_edp_phy_match_table[] = { > @@ -1117,10 +1160,16 @@ static const struct of_device_id qcom_edp_phy_match_table[] = { > }; > MODULE_DEVICE_TABLE(of, qcom_edp_phy_match_table); > > +static const struct dev_pm_ops qcom_edp_pm_ops = { > + SET_RUNTIME_PM_OPS(qcom_edp_runtime_suspend, > + qcom_edp_runtime_resume, NULL) > +}; > + > static struct platform_driver qcom_edp_phy_driver = { > .probe = qcom_edp_phy_probe, > .driver = { > .name = "qcom-edp-phy", > + .pm = &qcom_edp_pm_ops, > .of_match_table = qcom_edp_phy_match_table, > }, > }; > > --- > base-commit: 9aaeb87ce1e966169a57f53a02ba05b30880ffb8 > change-id: 20240907-phy-qcom-edp-enable-runtime-pm-6fad07af8947 > > Best regards, > -- > Abel Vesa <abel.vesa@linaro.org> > >
On 24-09-07 20:52:14, Dmitry Baryshkov wrote: > On Sat, 7 Sept 2024 at 18:25, Abel Vesa <abel.vesa@linaro.org> wrote: > > > > Enable runtime PM support by adding proper ops which will handle the > > clocks and regulators. These resources will now be handled on power_on and > > power_off instead of init and exit PHY ops. Also enable these resources on > > probe in order to balance out the disabling that is happening right after. > > Prevent runtime PM from being ON by default as well. > > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > > --- > > drivers/phy/qualcomm/phy-qcom-edp.c | 105 ++++++++++++++++++++++++++---------- > > 1 file changed, 77 insertions(+), 28 deletions(-) > > > > diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c b/drivers/phy/qualcomm/phy-qcom-edp.c > > index da2b32fb5b45..3affeef261bf 100644 > > --- a/drivers/phy/qualcomm/phy-qcom-edp.c > > +++ b/drivers/phy/qualcomm/phy-qcom-edp.c > > @@ -192,14 +192,6 @@ static int qcom_edp_phy_init(struct phy *phy) > > int ret; > > u8 cfg8; > > > > - ret = regulator_bulk_enable(ARRAY_SIZE(edp->supplies), edp->supplies); > > - if (ret) > > - return ret; > > - > > - ret = clk_bulk_prepare_enable(ARRAY_SIZE(edp->clks), edp->clks); > > - if (ret) > > - goto out_disable_supplies; > > - > > writel(DP_PHY_PD_CTL_PWRDN | DP_PHY_PD_CTL_AUX_PWRDN | > > DP_PHY_PD_CTL_PLL_PWRDN | DP_PHY_PD_CTL_DP_CLAMP_EN, > > edp->edp + DP_PHY_PD_CTL); > > @@ -246,11 +238,6 @@ static int qcom_edp_phy_init(struct phy *phy) > > msleep(20); > > > > return 0; > > - > > -out_disable_supplies: > > - regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies); > > - > > - return ret; > > } > > > > static int qcom_edp_set_voltages(struct qcom_edp *edp, const struct phy_configure_opts_dp *dp_opts) > > @@ -721,6 +708,8 @@ static int qcom_edp_phy_power_on(struct phy *phy) > > u32 val; > > u8 cfg1; > > > > + pm_runtime_get_sync(&phy->dev); > > + > > ret = edp->cfg->ver_ops->com_power_on(edp); > > if (ret) > > return ret; > > @@ -841,6 +830,8 @@ static int qcom_edp_phy_power_off(struct phy *phy) > > > > writel(DP_PHY_PD_CTL_PSR_PWRDN, edp->edp + DP_PHY_PD_CTL); > > > > + pm_runtime_put(&phy->dev); > > + > > return 0; > > } > > > > @@ -856,23 +847,12 @@ static int qcom_edp_phy_set_mode(struct phy *phy, enum phy_mode mode, int submod > > return 0; > > } > > > > -static int qcom_edp_phy_exit(struct phy *phy) > > -{ > > - struct qcom_edp *edp = phy_get_drvdata(phy); > > - > > - clk_bulk_disable_unprepare(ARRAY_SIZE(edp->clks), edp->clks); > > - regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies); > > - > > - return 0; > > -} > > - > > static const struct phy_ops qcom_edp_ops = { > > .init = qcom_edp_phy_init, > > .configure = qcom_edp_phy_configure, > > .power_on = qcom_edp_phy_power_on, > > .power_off = qcom_edp_phy_power_off, > > .set_mode = qcom_edp_phy_set_mode, > > - .exit = qcom_edp_phy_exit, > > .owner = THIS_MODULE, > > }; > > > > @@ -1036,6 +1016,32 @@ static int qcom_edp_clks_register(struct qcom_edp *edp, struct device_node *np) > > return devm_of_clk_add_hw_provider(edp->dev, of_clk_hw_onecell_get, data); > > } > > > > +static int __maybe_unused qcom_edp_runtime_suspend(struct device *dev) > > +{ > > + struct qcom_edp *edp = dev_get_drvdata(dev); > > + > > + dev_err(dev, "Suspending DP phy\n"); > > Debug leftovers? Well, I should've made those dev_vdbg instead. > > > + > > + clk_bulk_disable_unprepare(ARRAY_SIZE(edp->clks), edp->clks); > > + regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies); > > + > > + return 0; > > +} > > + > > +static int __maybe_unused qcom_edp_runtime_resume(struct device *dev) > > +{ > > + struct qcom_edp *edp = dev_get_drvdata(dev); > > + int ret; > > + > > + dev_err(dev, "Resuming DP phy\n"); > > Debug leftovers? > See above. > > + > > + ret = regulator_bulk_enable(ARRAY_SIZE(edp->supplies), edp->supplies); > > + if (ret) > > + return ret; > > + > > + return clk_bulk_prepare_enable(ARRAY_SIZE(edp->clks), edp->clks); > > Missing error handling > Yep, will fix. > > +} > > + > > static int qcom_edp_phy_probe(struct platform_device *pdev) > > { > > struct phy_provider *phy_provider; > > @@ -1091,20 +1097,57 @@ static int qcom_edp_phy_probe(struct platform_device *pdev) > > return ret; > > } > > > > - ret = qcom_edp_clks_register(edp, pdev->dev.of_node); > > - if (ret) > > + ret = regulator_bulk_enable(ARRAY_SIZE(edp->supplies), edp->supplies); > > + if (ret) { > > + dev_err(dev, "failed to enable regulators, err=%d\n", ret); > > return ret; > > + } > > + > > + ret = clk_bulk_prepare_enable(ARRAY_SIZE(edp->clks), edp->clks); > > + if (ret) { > > + dev_err(dev, "failed to enable clocks, err=%d\n", ret); > > + goto err_disable_regulators; > > + } > > Please use pm_runtime_get_sync() instead(). > So let me explain how I thought this through first. This DP PHY is usually used on platforms where display is left enabled by the bootloader. So doing pm_runtime_get_sync would mean we increment the device's usage counter while it is known to be already enabled, even if genpd doesn't consider it so. So doing set_active instead would be more accurate. Now, for the regulator and clock generic frameworks, that seemed OK to do at the time. Now I can see that the same argument can be made for those as well. So I'm thinking maybe I just drop the enable here and don't do _get_sync, but rather rely on the resume being done on power on instead. > > + > > + ret = qcom_edp_clks_register(edp, pdev->dev.of_node); > > + if (ret) { > > + dev_err(dev, "failed to register PHY clocks, err=%d\n", ret); > > + goto err_disable_clocks; > > + } > > > > edp->phy = devm_phy_create(dev, pdev->dev.of_node, &qcom_edp_ops); > > if (IS_ERR(edp->phy)) { > > dev_err(dev, "failed to register phy\n"); > > - return PTR_ERR(edp->phy); > > + ret = PTR_ERR(edp->phy); > > + goto err_disable_clocks; > > } > > > > + pm_runtime_set_active(dev); > > + ret = devm_pm_runtime_enable(dev); > > If this handles earlier, you don't need to call pm_runtime_set_active() manually > Enable and set_active are two separate things. Maybe I'm misunderstanding your comment. > > + if (ret) > > + goto err_disable_clocks; > > + /* > > + * Prevent runtime pm from being ON by default. Users can enable > > + * it using power/control in sysfs. > > why? > OK, so this is a tricky one. If there is any platform out there that makes use of this PHY but the resources are not properly described, we might get in trouble. So I was thinking that maybe we don't risk that but let the user enable it via sysfs. That way, this patch will not break by default such platforms. Also, this would be in line with the rest of the other Qcom PHYs. > > + */ > > + pm_runtime_forbid(dev); > > + > > + dev_set_drvdata(dev, edp); > > phy_set_drvdata(edp->phy, edp); > > > > phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate); > > - return PTR_ERR_OR_ZERO(phy_provider); > > + if (IS_ERR(phy_provider)) > > + goto err_disable_clocks; > > + > > + return 0; > > + > > +err_disable_clocks: > > + clk_bulk_disable_unprepare(ARRAY_SIZE(edp->clks), edp->clks); > > + > > +err_disable_regulators: > > + regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies); > > Ideally this should be handled by pm_runtime. Or at least by pm_runtime_put(). Will drop entirely. Again, lets not enable anything on probe for now. > > > + > > + return ret; > > } > > > > static const struct of_device_id qcom_edp_phy_match_table[] = { > > @@ -1117,10 +1160,16 @@ static const struct of_device_id qcom_edp_phy_match_table[] = { > > }; > > MODULE_DEVICE_TABLE(of, qcom_edp_phy_match_table); > > > > +static const struct dev_pm_ops qcom_edp_pm_ops = { > > + SET_RUNTIME_PM_OPS(qcom_edp_runtime_suspend, > > + qcom_edp_runtime_resume, NULL) > > +}; > > + > > static struct platform_driver qcom_edp_phy_driver = { > > .probe = qcom_edp_phy_probe, > > .driver = { > > .name = "qcom-edp-phy", > > + .pm = &qcom_edp_pm_ops, > > .of_match_table = qcom_edp_phy_match_table, > > }, > > }; > > > > --- > > base-commit: 9aaeb87ce1e966169a57f53a02ba05b30880ffb8 > > change-id: 20240907-phy-qcom-edp-enable-runtime-pm-6fad07af8947 > > > > Best regards, > > -- > > Abel Vesa <abel.vesa@linaro.org> > > > > > -- > With best wishes > Dmitry
On 24-09-07 22:19:42, Bjorn Andersson wrote: > On Sat, Sep 07, 2024 at 06:25:21PM GMT, Abel Vesa wrote: > > Enable runtime PM support by adding proper ops which will handle the > > clocks and regulators. These resources will now be handled on power_on and > > power_off instead of init and exit PHY ops. Also enable these resources on > > probe in order to balance out the disabling that is happening right after. > > Sounds good, I assume there's a good reason for doing this? Replied to Dmitry's comment about this already, but will summarize here as well. Basically, this PHY is usually left enabled as part of display SS by the bootloader on most of the platforms it is used. My rationale here was initially that maybe incrementing device's usage counter would be wrong considering that it is already enabled. But then enabling clocks and regulators here would move the wrong logic to their generic framework. This I haven't thought through initially. So I decided to just drop the votes entirely on probe. The resources will be enabled on power_on via runtime PM get call. > > Please provide a proper problem description, as defined in: > https://docs.kernel.org/process/submitting-patches.html#describe-your-changes Yep, will describe the impact/necessity better in the next version. > > > Prevent runtime PM from being ON by default as well. > > > > Why? Also replied to Dmitry to his similar comment. Long story short, if any of the platforms that use this PHY have any missing/wrong resources voted for, it might render display broken on the first runtime suspend, if runtime PM is allowed by default. Plus, all other Qcom PHY drivers follow the same logic. Anyway, will drop in the next version. > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > > --- > > drivers/phy/qualcomm/phy-qcom-edp.c | 105 ++++++++++++++++++++++++++---------- > > 1 file changed, 77 insertions(+), 28 deletions(-) > > > > diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c b/drivers/phy/qualcomm/phy-qcom-edp.c > > index da2b32fb5b45..3affeef261bf 100644 > > --- a/drivers/phy/qualcomm/phy-qcom-edp.c > > +++ b/drivers/phy/qualcomm/phy-qcom-edp.c > > @@ -192,14 +192,6 @@ static int qcom_edp_phy_init(struct phy *phy) > > int ret; > > u8 cfg8; > > > > - ret = regulator_bulk_enable(ARRAY_SIZE(edp->supplies), edp->supplies); > > - if (ret) > > - return ret; > > - > > - ret = clk_bulk_prepare_enable(ARRAY_SIZE(edp->clks), edp->clks); > > - if (ret) > > - goto out_disable_supplies; > > - > > writel(DP_PHY_PD_CTL_PWRDN | DP_PHY_PD_CTL_AUX_PWRDN | > > DP_PHY_PD_CTL_PLL_PWRDN | DP_PHY_PD_CTL_DP_CLAMP_EN, > > edp->edp + DP_PHY_PD_CTL); > > @@ -246,11 +238,6 @@ static int qcom_edp_phy_init(struct phy *phy) > > msleep(20); > > > > return 0; > > - > > -out_disable_supplies: > > - regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies); > > - > > - return ret; > > } > > > > static int qcom_edp_set_voltages(struct qcom_edp *edp, const struct phy_configure_opts_dp *dp_opts) > > @@ -721,6 +708,8 @@ static int qcom_edp_phy_power_on(struct phy *phy) > > u32 val; > > u8 cfg1; > > > > + pm_runtime_get_sync(&phy->dev); > > + > > ret = edp->cfg->ver_ops->com_power_on(edp); > > if (ret) > > return ret; > > @@ -841,6 +830,8 @@ static int qcom_edp_phy_power_off(struct phy *phy) > > > > writel(DP_PHY_PD_CTL_PSR_PWRDN, edp->edp + DP_PHY_PD_CTL); > > > > + pm_runtime_put(&phy->dev); > > + > > return 0; > > } > > > > @@ -856,23 +847,12 @@ static int qcom_edp_phy_set_mode(struct phy *phy, enum phy_mode mode, int submod > > return 0; > > } > > > > -static int qcom_edp_phy_exit(struct phy *phy) > > -{ > > - struct qcom_edp *edp = phy_get_drvdata(phy); > > - > > - clk_bulk_disable_unprepare(ARRAY_SIZE(edp->clks), edp->clks); > > - regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies); > > - > > - return 0; > > -} > > - > > static const struct phy_ops qcom_edp_ops = { > > .init = qcom_edp_phy_init, > > .configure = qcom_edp_phy_configure, > > .power_on = qcom_edp_phy_power_on, > > .power_off = qcom_edp_phy_power_off, > > .set_mode = qcom_edp_phy_set_mode, > > - .exit = qcom_edp_phy_exit, > > .owner = THIS_MODULE, > > }; > > > > @@ -1036,6 +1016,32 @@ static int qcom_edp_clks_register(struct qcom_edp *edp, struct device_node *np) > > return devm_of_clk_add_hw_provider(edp->dev, of_clk_hw_onecell_get, data); > > } > > > > +static int __maybe_unused qcom_edp_runtime_suspend(struct device *dev) > > +{ > > + struct qcom_edp *edp = dev_get_drvdata(dev); > > + > > + dev_err(dev, "Suspending DP phy\n"); > > + > > + clk_bulk_disable_unprepare(ARRAY_SIZE(edp->clks), edp->clks); > > + regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies); > > + > > + return 0; > > +} > > + > > +static int __maybe_unused qcom_edp_runtime_resume(struct device *dev) > > +{ > > + struct qcom_edp *edp = dev_get_drvdata(dev); > > + int ret; > > + > > + dev_err(dev, "Resuming DP phy\n"); > > + > > + ret = regulator_bulk_enable(ARRAY_SIZE(edp->supplies), edp->supplies); > > + if (ret) > > + return ret; > > + > > + return clk_bulk_prepare_enable(ARRAY_SIZE(edp->clks), edp->clks); > > +} > > + > > static int qcom_edp_phy_probe(struct platform_device *pdev) > > { > > struct phy_provider *phy_provider; > > @@ -1091,20 +1097,57 @@ static int qcom_edp_phy_probe(struct platform_device *pdev) > > return ret; > > } > > > > - ret = qcom_edp_clks_register(edp, pdev->dev.of_node); > > - if (ret) > > + ret = regulator_bulk_enable(ARRAY_SIZE(edp->supplies), edp->supplies); > > + if (ret) { > > + dev_err(dev, "failed to enable regulators, err=%d\n", ret); > > return ret; > > + } > > + > > + ret = clk_bulk_prepare_enable(ARRAY_SIZE(edp->clks), edp->clks); > > + if (ret) { > > + dev_err(dev, "failed to enable clocks, err=%d\n", ret); > > + goto err_disable_regulators; > > + } > > + > > + ret = qcom_edp_clks_register(edp, pdev->dev.of_node); > > + if (ret) { > > + dev_err(dev, "failed to register PHY clocks, err=%d\n", ret); > > + goto err_disable_clocks; > > + } > > > > edp->phy = devm_phy_create(dev, pdev->dev.of_node, &qcom_edp_ops); > > if (IS_ERR(edp->phy)) { > > dev_err(dev, "failed to register phy\n"); > > - return PTR_ERR(edp->phy); > > + ret = PTR_ERR(edp->phy); > > + goto err_disable_clocks; > > } > > > > + pm_runtime_set_active(dev); > > + ret = devm_pm_runtime_enable(dev); > > + if (ret) > > + goto err_disable_clocks; > > + /* > > + * Prevent runtime pm from being ON by default. Users can enable > > + * it using power/control in sysfs. > > That is what this call do, please describe why it's done instead. Will drop, as mentioned above. > > Regards, > Bjorn > > > + */ > > + pm_runtime_forbid(dev); > > + > > + dev_set_drvdata(dev, edp); > > phy_set_drvdata(edp->phy, edp); > > > > phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate); > > - return PTR_ERR_OR_ZERO(phy_provider); > > + if (IS_ERR(phy_provider)) > > + goto err_disable_clocks; > > + > > + return 0; > > + > > +err_disable_clocks: > > + clk_bulk_disable_unprepare(ARRAY_SIZE(edp->clks), edp->clks); > > + > > +err_disable_regulators: > > + regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies); > > + > > + return ret; > > } > > > > static const struct of_device_id qcom_edp_phy_match_table[] = { > > @@ -1117,10 +1160,16 @@ static const struct of_device_id qcom_edp_phy_match_table[] = { > > }; > > MODULE_DEVICE_TABLE(of, qcom_edp_phy_match_table); > > > > +static const struct dev_pm_ops qcom_edp_pm_ops = { > > + SET_RUNTIME_PM_OPS(qcom_edp_runtime_suspend, > > + qcom_edp_runtime_resume, NULL) > > +}; > > + > > static struct platform_driver qcom_edp_phy_driver = { > > .probe = qcom_edp_phy_probe, > > .driver = { > > .name = "qcom-edp-phy", > > + .pm = &qcom_edp_pm_ops, > > .of_match_table = qcom_edp_phy_match_table, > > }, > > }; > > > > --- > > base-commit: 9aaeb87ce1e966169a57f53a02ba05b30880ffb8 > > change-id: 20240907-phy-qcom-edp-enable-runtime-pm-6fad07af8947 > > > > Best regards, > > -- > > Abel Vesa <abel.vesa@linaro.org> > > > >
On Sun, 8 Sept 2024 at 21:12, Abel Vesa <abel.vesa@linaro.org> wrote: > > On 24-09-07 20:52:14, Dmitry Baryshkov wrote: > > On Sat, 7 Sept 2024 at 18:25, Abel Vesa <abel.vesa@linaro.org> wrote: > > > > > > Enable runtime PM support by adding proper ops which will handle the > > > clocks and regulators. These resources will now be handled on power_on and > > > power_off instead of init and exit PHY ops. Also enable these resources on > > > probe in order to balance out the disabling that is happening right after. > > > Prevent runtime PM from being ON by default as well. > > > > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > > > --- > > > drivers/phy/qualcomm/phy-qcom-edp.c | 105 ++++++++++++++++++++++++++---------- > > > 1 file changed, 77 insertions(+), 28 deletions(-) > > > > > > diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c b/drivers/phy/qualcomm/phy-qcom-edp.c > > > index da2b32fb5b45..3affeef261bf 100644 > > > --- a/drivers/phy/qualcomm/phy-qcom-edp.c > > > +++ b/drivers/phy/qualcomm/phy-qcom-edp.c > > > @@ -192,14 +192,6 @@ static int qcom_edp_phy_init(struct phy *phy) > > > int ret; > > > u8 cfg8; > > > > > > - ret = regulator_bulk_enable(ARRAY_SIZE(edp->supplies), edp->supplies); > > > - if (ret) > > > - return ret; > > > - > > > - ret = clk_bulk_prepare_enable(ARRAY_SIZE(edp->clks), edp->clks); > > > - if (ret) > > > - goto out_disable_supplies; > > > - > > > writel(DP_PHY_PD_CTL_PWRDN | DP_PHY_PD_CTL_AUX_PWRDN | > > > DP_PHY_PD_CTL_PLL_PWRDN | DP_PHY_PD_CTL_DP_CLAMP_EN, > > > edp->edp + DP_PHY_PD_CTL); > > > @@ -246,11 +238,6 @@ static int qcom_edp_phy_init(struct phy *phy) > > > msleep(20); > > > > > > return 0; > > > - > > > -out_disable_supplies: > > > - regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies); > > > - > > > - return ret; > > > } > > > > > > static int qcom_edp_set_voltages(struct qcom_edp *edp, const struct phy_configure_opts_dp *dp_opts) > > > @@ -721,6 +708,8 @@ static int qcom_edp_phy_power_on(struct phy *phy) > > > u32 val; > > > u8 cfg1; > > > > > > + pm_runtime_get_sync(&phy->dev); > > > + > > > ret = edp->cfg->ver_ops->com_power_on(edp); > > > if (ret) > > > return ret; > > > @@ -841,6 +830,8 @@ static int qcom_edp_phy_power_off(struct phy *phy) > > > > > > writel(DP_PHY_PD_CTL_PSR_PWRDN, edp->edp + DP_PHY_PD_CTL); > > > > > > + pm_runtime_put(&phy->dev); > > > + > > > return 0; > > > } > > > > > > @@ -856,23 +847,12 @@ static int qcom_edp_phy_set_mode(struct phy *phy, enum phy_mode mode, int submod > > > return 0; > > > } > > > > > > -static int qcom_edp_phy_exit(struct phy *phy) > > > -{ > > > - struct qcom_edp *edp = phy_get_drvdata(phy); > > > - > > > - clk_bulk_disable_unprepare(ARRAY_SIZE(edp->clks), edp->clks); > > > - regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies); > > > - > > > - return 0; > > > -} > > > - > > > static const struct phy_ops qcom_edp_ops = { > > > .init = qcom_edp_phy_init, > > > .configure = qcom_edp_phy_configure, > > > .power_on = qcom_edp_phy_power_on, > > > .power_off = qcom_edp_phy_power_off, > > > .set_mode = qcom_edp_phy_set_mode, > > > - .exit = qcom_edp_phy_exit, > > > .owner = THIS_MODULE, > > > }; > > > > > > @@ -1036,6 +1016,32 @@ static int qcom_edp_clks_register(struct qcom_edp *edp, struct device_node *np) > > > return devm_of_clk_add_hw_provider(edp->dev, of_clk_hw_onecell_get, data); > > > } > > > > > > +static int __maybe_unused qcom_edp_runtime_suspend(struct device *dev) > > > +{ > > > + struct qcom_edp *edp = dev_get_drvdata(dev); > > > + > > > + dev_err(dev, "Suspending DP phy\n"); > > > > Debug leftovers? > > Well, I should've made those dev_vdbg instead. > > > > > > + > > > + clk_bulk_disable_unprepare(ARRAY_SIZE(edp->clks), edp->clks); > > > + regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies); > > > + > > > + return 0; > > > +} > > > + > > > +static int __maybe_unused qcom_edp_runtime_resume(struct device *dev) > > > +{ > > > + struct qcom_edp *edp = dev_get_drvdata(dev); > > > + int ret; > > > + > > > + dev_err(dev, "Resuming DP phy\n"); > > > > Debug leftovers? > > > > See above. > > > > + > > > + ret = regulator_bulk_enable(ARRAY_SIZE(edp->supplies), edp->supplies); > > > + if (ret) > > > + return ret; > > > + > > > + return clk_bulk_prepare_enable(ARRAY_SIZE(edp->clks), edp->clks); > > > > Missing error handling > > > > Yep, will fix. > > > > +} > > > + > > > static int qcom_edp_phy_probe(struct platform_device *pdev) > > > { > > > struct phy_provider *phy_provider; > > > @@ -1091,20 +1097,57 @@ static int qcom_edp_phy_probe(struct platform_device *pdev) > > > return ret; > > > } > > > > > > - ret = qcom_edp_clks_register(edp, pdev->dev.of_node); > > > - if (ret) > > > + ret = regulator_bulk_enable(ARRAY_SIZE(edp->supplies), edp->supplies); > > > + if (ret) { > > > + dev_err(dev, "failed to enable regulators, err=%d\n", ret); > > > return ret; > > > + } > > > + > > > + ret = clk_bulk_prepare_enable(ARRAY_SIZE(edp->clks), edp->clks); > > > + if (ret) { > > > + dev_err(dev, "failed to enable clocks, err=%d\n", ret); > > > + goto err_disable_regulators; > > > + } > > > > Please use pm_runtime_get_sync() instead(). > > > > So let me explain how I thought this through first. This DP PHY is > usually used on platforms where display is left enabled by the > bootloader. So doing pm_runtime_get_sync would mean we increment the > device's usage counter while it is known to be already enabled, even if > genpd doesn't consider it so. So doing set_active instead would be more > accurate. Now, for the regulator and clock generic frameworks, that > seemed OK to do at the time. Now I can see that the same argument can be > made for those as well. So I'm thinking maybe I just drop the enable > here and don't do _get_sync, but rather rely on the resume being done > on power on instead. Please don't rely on the bootloader. The device should be in the resumed state when registering clocks. Also devm_pm_runtime_enable() should also be called before, so that the CCF makes note of PM being enabled. > > > > + > > > + ret = qcom_edp_clks_register(edp, pdev->dev.of_node); > > > + if (ret) { > > > + dev_err(dev, "failed to register PHY clocks, err=%d\n", ret); > > > + goto err_disable_clocks; > > > + } > > > > > > edp->phy = devm_phy_create(dev, pdev->dev.of_node, &qcom_edp_ops); > > > if (IS_ERR(edp->phy)) { > > > dev_err(dev, "failed to register phy\n"); > > > - return PTR_ERR(edp->phy); > > > + ret = PTR_ERR(edp->phy); > > > + goto err_disable_clocks; > > > } > > > > > > + pm_runtime_set_active(dev); > > > + ret = devm_pm_runtime_enable(dev); > > > > If this handles earlier, you don't need to call pm_runtime_set_active() manually > > > > Enable and set_active are two separate things. Maybe I'm > misunderstanding your comment. Yeah, I wrote something strange here. I meant that if enable is called earlier and then if the device is resumed, there is no need to call set_active. > > > > + if (ret) > > > + goto err_disable_clocks; > > > + /* > > > + * Prevent runtime pm from being ON by default. Users can enable > > > + * it using power/control in sysfs. > > > > why? > > > > OK, so this is a tricky one. If there is any platform out there that > makes use of this PHY but the resources are not properly described, we > might get in trouble. So I was thinking that maybe we don't risk that > but let the user enable it via sysfs. That way, this patch will not > break by default such platforms. If the platform doesn't describe resources, it is broken, there is no need to support it. > > Also, this would be in line with the rest of the other Qcom PHYs. I think it's a bit of a cargo cult. Such code was added in 2018 and then it was c&p'ed since that time. > > > > + */ > > > + pm_runtime_forbid(dev); > > > + > > > + dev_set_drvdata(dev, edp); > > > phy_set_drvdata(edp->phy, edp); > > > > > > phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate); > > > - return PTR_ERR_OR_ZERO(phy_provider); > > > + if (IS_ERR(phy_provider)) > > > + goto err_disable_clocks; > > > + > > > + return 0; > > > + > > > +err_disable_clocks: > > > + clk_bulk_disable_unprepare(ARRAY_SIZE(edp->clks), edp->clks); > > > + > > > +err_disable_regulators: > > > + regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies); > > > > Ideally this should be handled by pm_runtime. Or at least by pm_runtime_put(). > > Will drop entirely. Again, lets not enable anything on probe for now. NAK. > > > > > > + > > > + return ret; > > > } > > > > > > static const struct of_device_id qcom_edp_phy_match_table[] = { > > > @@ -1117,10 +1160,16 @@ static const struct of_device_id qcom_edp_phy_match_table[] = { > > > }; > > > MODULE_DEVICE_TABLE(of, qcom_edp_phy_match_table); > > > > > > +static const struct dev_pm_ops qcom_edp_pm_ops = { > > > + SET_RUNTIME_PM_OPS(qcom_edp_runtime_suspend, > > > + qcom_edp_runtime_resume, NULL) > > > +}; > > > + > > > static struct platform_driver qcom_edp_phy_driver = { > > > .probe = qcom_edp_phy_probe, > > > .driver = { > > > .name = "qcom-edp-phy", > > > + .pm = &qcom_edp_pm_ops, > > > .of_match_table = qcom_edp_phy_match_table, > > > }, > > > }; > > > > > > --- > > > base-commit: 9aaeb87ce1e966169a57f53a02ba05b30880ffb8 > > > change-id: 20240907-phy-qcom-edp-enable-runtime-pm-6fad07af8947 > > > > > > Best regards, > > > -- > > > Abel Vesa <abel.vesa@linaro.org> > > > > > > > > > -- > > With best wishes > > Dmitry
On 24-09-08 21:43:01, Dmitry Baryshkov wrote: > On Sun, 8 Sept 2024 at 21:12, Abel Vesa <abel.vesa@linaro.org> wrote: > > > > On 24-09-07 20:52:14, Dmitry Baryshkov wrote: > > > On Sat, 7 Sept 2024 at 18:25, Abel Vesa <abel.vesa@linaro.org> wrote: > > > > > > > > Enable runtime PM support by adding proper ops which will handle the > > > > clocks and regulators. These resources will now be handled on power_on and > > > > power_off instead of init and exit PHY ops. Also enable these resources on > > > > probe in order to balance out the disabling that is happening right after. > > > > Prevent runtime PM from being ON by default as well. > > > > > > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > > > > --- > > > > drivers/phy/qualcomm/phy-qcom-edp.c | 105 ++++++++++++++++++++++++++---------- > > > > 1 file changed, 77 insertions(+), 28 deletions(-) > > > > > > > > diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c b/drivers/phy/qualcomm/phy-qcom-edp.c > > > > index da2b32fb5b45..3affeef261bf 100644 > > > > --- a/drivers/phy/qualcomm/phy-qcom-edp.c > > > > +++ b/drivers/phy/qualcomm/phy-qcom-edp.c [...] > > > > +} > > > > + > > > > static int qcom_edp_phy_probe(struct platform_device *pdev) > > > > { > > > > struct phy_provider *phy_provider; > > > > @@ -1091,20 +1097,57 @@ static int qcom_edp_phy_probe(struct platform_device *pdev) > > > > return ret; > > > > } > > > > > > > > - ret = qcom_edp_clks_register(edp, pdev->dev.of_node); > > > > - if (ret) > > > > + ret = regulator_bulk_enable(ARRAY_SIZE(edp->supplies), edp->supplies); > > > > + if (ret) { > > > > + dev_err(dev, "failed to enable regulators, err=%d\n", ret); > > > > return ret; > > > > + } > > > > + > > > > + ret = clk_bulk_prepare_enable(ARRAY_SIZE(edp->clks), edp->clks); > > > > + if (ret) { > > > > + dev_err(dev, "failed to enable clocks, err=%d\n", ret); > > > > + goto err_disable_regulators; > > > > + } > > > > > > Please use pm_runtime_get_sync() instead(). > > > > > > > So let me explain how I thought this through first. This DP PHY is > > usually used on platforms where display is left enabled by the > > bootloader. So doing pm_runtime_get_sync would mean we increment the > > device's usage counter while it is known to be already enabled, even if > > genpd doesn't consider it so. So doing set_active instead would be more > > accurate. Now, for the regulator and clock generic frameworks, that > > seemed OK to do at the time. Now I can see that the same argument can be > > made for those as well. So I'm thinking maybe I just drop the enable > > here and don't do _get_sync, but rather rely on the resume being done > > on power on instead. > > Please don't rely on the bootloader. The device should be in the > resumed state when registering clocks. Also devm_pm_runtime_enable() > should also be called before, so that the CCF makes note of PM being > enabled. Fair enough. Will do that. > > > > > > > + > > > > + ret = qcom_edp_clks_register(edp, pdev->dev.of_node); > > > > + if (ret) { > > > > + dev_err(dev, "failed to register PHY clocks, err=%d\n", ret); > > > > + goto err_disable_clocks; > > > > + } > > > > > > > > edp->phy = devm_phy_create(dev, pdev->dev.of_node, &qcom_edp_ops); > > > > if (IS_ERR(edp->phy)) { > > > > dev_err(dev, "failed to register phy\n"); > > > > - return PTR_ERR(edp->phy); > > > > + ret = PTR_ERR(edp->phy); > > > > + goto err_disable_clocks; > > > > } > > > > > > > > + pm_runtime_set_active(dev); > > > > + ret = devm_pm_runtime_enable(dev); > > > > > > If this handles earlier, you don't need to call pm_runtime_set_active() manually > > > > > > > Enable and set_active are two separate things. Maybe I'm > > misunderstanding your comment. > > Yeah, I wrote something strange here. I meant that if enable is called > earlier and then if the device is resumed, there is no need to call > set_active. I guess you meant "that if _get_sync is called earlier and then ...", right? Enable won't resume it. > > > > > > > + if (ret) > > > > + goto err_disable_clocks; > > > > + /* > > > > + * Prevent runtime pm from being ON by default. Users can enable > > > > + * it using power/control in sysfs. > > > > > > why? > > > > > > > OK, so this is a tricky one. If there is any platform out there that > > makes use of this PHY but the resources are not properly described, we > > might get in trouble. So I was thinking that maybe we don't risk that > > but let the user enable it via sysfs. That way, this patch will not > > break by default such platforms. > > If the platform doesn't describe resources, it is broken, there is no > need to support it. > > > > Also, this would be in line with the rest of the other Qcom PHYs. > > I think it's a bit of a cargo cult. Such code was added in 2018 and > then it was c&p'ed since that time. OK, will drop it. > > > > > > > + */ > > > > + pm_runtime_forbid(dev); > > > > + > > > > + dev_set_drvdata(dev, edp); > > > > phy_set_drvdata(edp->phy, edp); > > > > > > > > phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate); > > > > - return PTR_ERR_OR_ZERO(phy_provider); > > > > + if (IS_ERR(phy_provider)) > > > > + goto err_disable_clocks; > > > > + > > > > + return 0; > > > > + > > > > +err_disable_clocks: > > > > + clk_bulk_disable_unprepare(ARRAY_SIZE(edp->clks), edp->clks); > > > > + > > > > +err_disable_regulators: > > > > + regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies); > > > > > > Ideally this should be handled by pm_runtime. Or at least by pm_runtime_put(). > > > > Will drop entirely. Again, lets not enable anything on probe for now. > > NAK. OK, your argument above about the registering of clocks needing the PHY resumed makes sense. Will do _get_sync on probe.
On Sat, Sep 07, 2024 at 06:25:21PM +0300, Abel Vesa wrote: > Enable runtime PM support by adding proper ops which will handle the Avoid words like 'proper' here (what are non-proper runtime PM ops?). > clocks and regulators. These resources will now be handled on power_on and > power_off instead of init and exit PHY ops. No, this is simply a false claim and indicates that you haven't reviewed how PHY runtime PM works. Core will increment the usage count on init() and decrement it on exit(). > Also enable these resources on > probe in order to balance out the disabling that is happening right after. > Prevent runtime PM from being ON by default as well. And here you just regressed all current systems that do not have udev rules to enable runtime PM, and which will now be stuck with these resources always-on (e.g. during DPMS off and system suspend). In fact, you are even regressing systems that would enable runtime PM, as the runtime suspend callback would not currently be called when you enter system suspend so the regulators and clocks will be left on. This clearly hasn't been tested and analysed properly. > +static int __maybe_unused qcom_edp_runtime_suspend(struct device *dev) > +{ > + struct qcom_edp *edp = dev_get_drvdata(dev); > + > + dev_err(dev, "Suspending DP phy\n"); You forgot to drop your development printks (same below). Johan
On 24-09-18 12:05:59, Johan Hovold wrote: > On Sat, Sep 07, 2024 at 06:25:21PM +0300, Abel Vesa wrote: > > Enable runtime PM support by adding proper ops which will handle the > > Avoid words like 'proper' here (what are non-proper runtime PM ops?). Sure. > > > clocks and regulators. These resources will now be handled on power_on and > > power_off instead of init and exit PHY ops. > > No, this is simply a false claim and indicates that you haven't reviewed > how PHY runtime PM works. Core will increment the usage count on init() > and decrement it on exit(). Yeah, I guess the better argument here would be that the PHY needs regulators and clocks enabled. Anyway, ignore this version as it was already NACKed by Dmitry. > > > Also enable these resources on > > probe in order to balance out the disabling that is happening right after. > > Prevent runtime PM from being ON by default as well. > > And here you just regressed all current systems that do not have udev > rules to enable runtime PM, and which will now be stuck with these > resources always-on (e.g. during DPMS off and system suspend). > > In fact, you are even regressing systems that would enable runtime PM, > as the runtime suspend callback would not currently be called when you > enter system suspend so the regulators and clocks will be left on. > > This clearly hasn't been tested and analysed properly. > > > +static int __maybe_unused qcom_edp_runtime_suspend(struct device *dev) > > +{ > > + struct qcom_edp *edp = dev_get_drvdata(dev); > > + > > + dev_err(dev, "Suspending DP phy\n"); > > You forgot to drop your development printks (same below). > > Johan
On Wed, Sep 18, 2024 at 04:10:05PM +0300, Abel Vesa wrote: > On 24-09-18 12:05:59, Johan Hovold wrote: > > On Sat, Sep 07, 2024 at 06:25:21PM +0300, Abel Vesa wrote: > > > Enable runtime PM support by adding proper ops which will handle the > > > clocks and regulators. These resources will now be handled on power_on and > > > power_off instead of init and exit PHY ops. > > > > No, this is simply a false claim and indicates that you haven't reviewed > > how PHY runtime PM works. Core will increment the usage count on init() > > and decrement it on exit(). > > Yeah, I guess the better argument here would be that the PHY needs > regulators and clocks enabled No, that's already handled today so is clearly not a valid argument. > Anyway, ignore this version as it was already NACKed by Dmitry. No, my feedback is still valid, and you're bound to repeat the same mistakes over and over again unless you try to understand what I've been saying here. > > > Also enable these resources on > > > probe in order to balance out the disabling that is happening right after. > > > Prevent runtime PM from being ON by default as well. > > > > And here you just regressed all current systems that do not have udev > > rules to enable runtime PM, and which will now be stuck with these > > resources always-on (e.g. during DPMS off and system suspend). > > > > In fact, you are even regressing systems that would enable runtime PM, > > as the runtime suspend callback would not currently be called when you > > enter system suspend so the regulators and clocks will be left on. > > > > This clearly hasn't been tested and analysed properly. Johan
On 24-09-20 10:40:13, Johan Hovold wrote: > On Wed, Sep 18, 2024 at 04:10:05PM +0300, Abel Vesa wrote: > > On 24-09-18 12:05:59, Johan Hovold wrote: > > > On Sat, Sep 07, 2024 at 06:25:21PM +0300, Abel Vesa wrote: > > > > Enable runtime PM support by adding proper ops which will handle the > > > > > clocks and regulators. These resources will now be handled on power_on and > > > > power_off instead of init and exit PHY ops. > > > > > > No, this is simply a false claim and indicates that you haven't reviewed > > > how PHY runtime PM works. Core will increment the usage count on init() > > > and decrement it on exit(). > > > > Yeah, I guess the better argument here would be that the PHY needs > > regulators and clocks enabled > > No, that's already handled today so is clearly not a valid argument. I think we're saying the same thing here. I was trying to say that, as it is currently done and it's correct, the init() needs those resources enabled. > > > Anyway, ignore this version as it was already NACKed by Dmitry. > > No, my feedback is still valid, and you're bound to repeat the same > mistakes over and over again unless you try to understand what I've been > saying here. Duly noted. My reply wasn't trying to dismiss your feedback. Thanks for your feedback. > > > > > Also enable these resources on > > > > probe in order to balance out the disabling that is happening right after. > > > > Prevent runtime PM from being ON by default as well. > > > > > > And here you just regressed all current systems that do not have udev > > > rules to enable runtime PM, and which will now be stuck with these > > > resources always-on (e.g. during DPMS off and system suspend). > > > > > > In fact, you are even regressing systems that would enable runtime PM, > > > as the runtime suspend callback would not currently be called when you > > > enter system suspend so the regulators and clocks will be left on. > > > > > > This clearly hasn't been tested and analysed properly. > > Johan
diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c b/drivers/phy/qualcomm/phy-qcom-edp.c index da2b32fb5b45..3affeef261bf 100644 --- a/drivers/phy/qualcomm/phy-qcom-edp.c +++ b/drivers/phy/qualcomm/phy-qcom-edp.c @@ -192,14 +192,6 @@ static int qcom_edp_phy_init(struct phy *phy) int ret; u8 cfg8; - ret = regulator_bulk_enable(ARRAY_SIZE(edp->supplies), edp->supplies); - if (ret) - return ret; - - ret = clk_bulk_prepare_enable(ARRAY_SIZE(edp->clks), edp->clks); - if (ret) - goto out_disable_supplies; - writel(DP_PHY_PD_CTL_PWRDN | DP_PHY_PD_CTL_AUX_PWRDN | DP_PHY_PD_CTL_PLL_PWRDN | DP_PHY_PD_CTL_DP_CLAMP_EN, edp->edp + DP_PHY_PD_CTL); @@ -246,11 +238,6 @@ static int qcom_edp_phy_init(struct phy *phy) msleep(20); return 0; - -out_disable_supplies: - regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies); - - return ret; } static int qcom_edp_set_voltages(struct qcom_edp *edp, const struct phy_configure_opts_dp *dp_opts) @@ -721,6 +708,8 @@ static int qcom_edp_phy_power_on(struct phy *phy) u32 val; u8 cfg1; + pm_runtime_get_sync(&phy->dev); + ret = edp->cfg->ver_ops->com_power_on(edp); if (ret) return ret; @@ -841,6 +830,8 @@ static int qcom_edp_phy_power_off(struct phy *phy) writel(DP_PHY_PD_CTL_PSR_PWRDN, edp->edp + DP_PHY_PD_CTL); + pm_runtime_put(&phy->dev); + return 0; } @@ -856,23 +847,12 @@ static int qcom_edp_phy_set_mode(struct phy *phy, enum phy_mode mode, int submod return 0; } -static int qcom_edp_phy_exit(struct phy *phy) -{ - struct qcom_edp *edp = phy_get_drvdata(phy); - - clk_bulk_disable_unprepare(ARRAY_SIZE(edp->clks), edp->clks); - regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies); - - return 0; -} - static const struct phy_ops qcom_edp_ops = { .init = qcom_edp_phy_init, .configure = qcom_edp_phy_configure, .power_on = qcom_edp_phy_power_on, .power_off = qcom_edp_phy_power_off, .set_mode = qcom_edp_phy_set_mode, - .exit = qcom_edp_phy_exit, .owner = THIS_MODULE, }; @@ -1036,6 +1016,32 @@ static int qcom_edp_clks_register(struct qcom_edp *edp, struct device_node *np) return devm_of_clk_add_hw_provider(edp->dev, of_clk_hw_onecell_get, data); } +static int __maybe_unused qcom_edp_runtime_suspend(struct device *dev) +{ + struct qcom_edp *edp = dev_get_drvdata(dev); + + dev_err(dev, "Suspending DP phy\n"); + + clk_bulk_disable_unprepare(ARRAY_SIZE(edp->clks), edp->clks); + regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies); + + return 0; +} + +static int __maybe_unused qcom_edp_runtime_resume(struct device *dev) +{ + struct qcom_edp *edp = dev_get_drvdata(dev); + int ret; + + dev_err(dev, "Resuming DP phy\n"); + + ret = regulator_bulk_enable(ARRAY_SIZE(edp->supplies), edp->supplies); + if (ret) + return ret; + + return clk_bulk_prepare_enable(ARRAY_SIZE(edp->clks), edp->clks); +} + static int qcom_edp_phy_probe(struct platform_device *pdev) { struct phy_provider *phy_provider; @@ -1091,20 +1097,57 @@ static int qcom_edp_phy_probe(struct platform_device *pdev) return ret; } - ret = qcom_edp_clks_register(edp, pdev->dev.of_node); - if (ret) + ret = regulator_bulk_enable(ARRAY_SIZE(edp->supplies), edp->supplies); + if (ret) { + dev_err(dev, "failed to enable regulators, err=%d\n", ret); return ret; + } + + ret = clk_bulk_prepare_enable(ARRAY_SIZE(edp->clks), edp->clks); + if (ret) { + dev_err(dev, "failed to enable clocks, err=%d\n", ret); + goto err_disable_regulators; + } + + ret = qcom_edp_clks_register(edp, pdev->dev.of_node); + if (ret) { + dev_err(dev, "failed to register PHY clocks, err=%d\n", ret); + goto err_disable_clocks; + } edp->phy = devm_phy_create(dev, pdev->dev.of_node, &qcom_edp_ops); if (IS_ERR(edp->phy)) { dev_err(dev, "failed to register phy\n"); - return PTR_ERR(edp->phy); + ret = PTR_ERR(edp->phy); + goto err_disable_clocks; } + pm_runtime_set_active(dev); + ret = devm_pm_runtime_enable(dev); + if (ret) + goto err_disable_clocks; + /* + * Prevent runtime pm from being ON by default. Users can enable + * it using power/control in sysfs. + */ + pm_runtime_forbid(dev); + + dev_set_drvdata(dev, edp); phy_set_drvdata(edp->phy, edp); phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate); - return PTR_ERR_OR_ZERO(phy_provider); + if (IS_ERR(phy_provider)) + goto err_disable_clocks; + + return 0; + +err_disable_clocks: + clk_bulk_disable_unprepare(ARRAY_SIZE(edp->clks), edp->clks); + +err_disable_regulators: + regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies); + + return ret; } static const struct of_device_id qcom_edp_phy_match_table[] = { @@ -1117,10 +1160,16 @@ static const struct of_device_id qcom_edp_phy_match_table[] = { }; MODULE_DEVICE_TABLE(of, qcom_edp_phy_match_table); +static const struct dev_pm_ops qcom_edp_pm_ops = { + SET_RUNTIME_PM_OPS(qcom_edp_runtime_suspend, + qcom_edp_runtime_resume, NULL) +}; + static struct platform_driver qcom_edp_phy_driver = { .probe = qcom_edp_phy_probe, .driver = { .name = "qcom-edp-phy", + .pm = &qcom_edp_pm_ops, .of_match_table = qcom_edp_phy_match_table, }, };
Enable runtime PM support by adding proper ops which will handle the clocks and regulators. These resources will now be handled on power_on and power_off instead of init and exit PHY ops. Also enable these resources on probe in order to balance out the disabling that is happening right after. Prevent runtime PM from being ON by default as well. Signed-off-by: Abel Vesa <abel.vesa@linaro.org> --- drivers/phy/qualcomm/phy-qcom-edp.c | 105 ++++++++++++++++++++++++++---------- 1 file changed, 77 insertions(+), 28 deletions(-) --- base-commit: 9aaeb87ce1e966169a57f53a02ba05b30880ffb8 change-id: 20240907-phy-qcom-edp-enable-runtime-pm-6fad07af8947 Best regards,