On 24-09-08 21:43:01, Dmitry Baryshkov wrote:
> On Sun, 8 Sept 2024 at 21:12, Abel Vesa <abel.vesa@xxxxxxxxxx> wrote:
> >
> > On 24-09-07 20:52:14, Dmitry Baryshkov wrote:
> > > On Sat, 7 Sept 2024 at 18:25, Abel Vesa <abel.vesa@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > ---
> > > > 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.
