Re: [PATCH] phy: qcom: edp: Add runtime PM support

[Abel Vesa]

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.