[v3,1/2] clk: qcom: camcc-sm6350: add pm_runtime support

  Make sure that we can enable and disable the power domains used for
camcc when the clocks are and aren't used.

Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
 drivers/clk/qcom/camcc-sm6350.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)
  

On Tue, 14 Feb 2023 at 13:01, Luca Weiss <luca.weiss@fairphone.com> wrote:
>
> Make sure that we can enable and disable the power domains used for
> camcc when the clocks are and aren't used.
>
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
>  drivers/clk/qcom/camcc-sm6350.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/qcom/camcc-sm6350.c b/drivers/clk/qcom/camcc-sm6350.c
> index acba9f99d960..fc5532e2ee5b 100644
> --- a/drivers/clk/qcom/camcc-sm6350.c
> +++ b/drivers/clk/qcom/camcc-sm6350.c
> @@ -7,6 +7,8 @@
>  #include <linux/clk-provider.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_clock.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/regmap.h>
>
>  #include <dt-bindings/clock/qcom,sm6350-camcc.h>
> @@ -1869,6 +1871,19 @@ MODULE_DEVICE_TABLE(of, camcc_sm6350_match_table);
>  static int camcc_sm6350_probe(struct platform_device *pdev)
>  {
>         struct regmap *regmap;
> +       int ret;
> +
> +       ret = devm_pm_runtime_enable(&pdev->dev);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = devm_pm_clk_create(&pdev->dev);
> +       if (ret < 0)
> +               return ret;

This makes me wonder, what is the use for the pm_clk in your case? The
driver doesn't seem to use of_pm_clk_add_clk(), of_pm_clk_add_clks()
or pm_clk_add_clk(). So pm_clk_suspend() and pm_clk_resume() do
nothing.

> +
> +       ret = pm_runtime_get(&pdev->dev);
> +       if (ret)
> +               return ret;
>
>         regmap = qcom_cc_map(pdev, &camcc_sm6350_desc);
>         if (IS_ERR(regmap))
> @@ -1879,14 +1894,22 @@ static int camcc_sm6350_probe(struct platform_device *pdev)
>         clk_agera_pll_configure(&camcc_pll2, regmap, &camcc_pll2_config);
>         clk_fabia_pll_configure(&camcc_pll3, regmap, &camcc_pll3_config);
>
> -       return qcom_cc_really_probe(pdev, &camcc_sm6350_desc, regmap);
> +       ret = qcom_cc_really_probe(pdev, &camcc_sm6350_desc, regmap);
> +       pm_runtime_put(&pdev->dev);
> +
> +       return ret;
>  }
>
> +static const struct dev_pm_ops camcc_pm_ops = {
> +       SET_RUNTIME_PM_OPS(pm_clk_suspend, pm_clk_resume, NULL)
> +};
> +
>  static struct platform_driver camcc_sm6350_driver = {
>         .probe = camcc_sm6350_probe,
>         .driver = {
>                 .name = "sm6350-camcc",
>                 .of_match_table = camcc_sm6350_match_table,
> +               .pm = &camcc_pm_ops,
>         },
>  };
>
>
> --
> 2.39.1
>
  

On Tue Feb 14, 2023 at 1:32 PM CET, Dmitry Baryshkov wrote:
> On Tue, 14 Feb 2023 at 13:01, Luca Weiss <luca.weiss@fairphone.com> wrote:
> >
> > Make sure that we can enable and disable the power domains used for
> > camcc when the clocks are and aren't used.
> >
> > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> > ---
> >  drivers/clk/qcom/camcc-sm6350.c | 25 ++++++++++++++++++++++++-
> >  1 file changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/qcom/camcc-sm6350.c b/drivers/clk/qcom/camcc-sm6350.c
> > index acba9f99d960..fc5532e2ee5b 100644
> > --- a/drivers/clk/qcom/camcc-sm6350.c
> > +++ b/drivers/clk/qcom/camcc-sm6350.c
> > @@ -7,6 +7,8 @@
> >  #include <linux/clk-provider.h>
> >  #include <linux/module.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/pm_clock.h>
> > +#include <linux/pm_runtime.h>
> >  #include <linux/regmap.h>
> >
> >  #include <dt-bindings/clock/qcom,sm6350-camcc.h>
> > @@ -1869,6 +1871,19 @@ MODULE_DEVICE_TABLE(of, camcc_sm6350_match_table);
> >  static int camcc_sm6350_probe(struct platform_device *pdev)
> >  {
> >         struct regmap *regmap;
> > +       int ret;
> > +
> > +       ret = devm_pm_runtime_enable(&pdev->dev);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       ret = devm_pm_clk_create(&pdev->dev);
> > +       if (ret < 0)
> > +               return ret;
>
> This makes me wonder, what is the use for the pm_clk in your case? The
> driver doesn't seem to use of_pm_clk_add_clk(), of_pm_clk_add_clks()
> or pm_clk_add_clk(). So pm_clk_suspend() and pm_clk_resume() do
> nothing.

You're right that we're not using any of these functions in the driver.
However still when camcc is not used, the associated power domain turns
off correctly so that part works as expected.

Honestly these lines have been copied from a different driver and I'm
not familiar enough with the pm_runtime APIs to know what to use here
without using the pm_clk* and pm_clk_suspend.

Basically we need, if any clock is being used in the driver, the
power-domain needs to be enabled as well, and if nothing is used the
power-domain can be disabled again.

Please advise.

Regards
Luca

>
> > +
> > +       ret = pm_runtime_get(&pdev->dev);
> > +       if (ret)
> > +               return ret;
> >
> >         regmap = qcom_cc_map(pdev, &camcc_sm6350_desc);
> >         if (IS_ERR(regmap))
> > @@ -1879,14 +1894,22 @@ static int camcc_sm6350_probe(struct platform_device *pdev)
> >         clk_agera_pll_configure(&camcc_pll2, regmap, &camcc_pll2_config);
> >         clk_fabia_pll_configure(&camcc_pll3, regmap, &camcc_pll3_config);
> >
> > -       return qcom_cc_really_probe(pdev, &camcc_sm6350_desc, regmap);
> > +       ret = qcom_cc_really_probe(pdev, &camcc_sm6350_desc, regmap);
> > +       pm_runtime_put(&pdev->dev);
> > +
> > +       return ret;
> >  }
> >
> > +static const struct dev_pm_ops camcc_pm_ops = {
> > +       SET_RUNTIME_PM_OPS(pm_clk_suspend, pm_clk_resume, NULL)
> > +};
> > +
> >  static struct platform_driver camcc_sm6350_driver = {
> >         .probe = camcc_sm6350_probe,
> >         .driver = {
> >                 .name = "sm6350-camcc",
> >                 .of_match_table = camcc_sm6350_match_table,
> > +               .pm = &camcc_pm_ops,
> >         },
> >  };
> >
> >
> > --
> > 2.39.1
> >
>
>
> -- 
> With best wishes
> Dmitry
  

On Tue, 7 Mar 2023 at 16:54, Luca Weiss <luca.weiss@fairphone.com> wrote:
>
> On Tue Feb 14, 2023 at 1:32 PM CET, Dmitry Baryshkov wrote:
> > On Tue, 14 Feb 2023 at 13:01, Luca Weiss <luca.weiss@fairphone.com> wrote:
> > >
> > > Make sure that we can enable and disable the power domains used for
> > > camcc when the clocks are and aren't used.
> > >
> > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> > > ---
> > >  drivers/clk/qcom/camcc-sm6350.c | 25 ++++++++++++++++++++++++-
> > >  1 file changed, 24 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/clk/qcom/camcc-sm6350.c b/drivers/clk/qcom/camcc-sm6350.c
> > > index acba9f99d960..fc5532e2ee5b 100644
> > > --- a/drivers/clk/qcom/camcc-sm6350.c
> > > +++ b/drivers/clk/qcom/camcc-sm6350.c
> > > @@ -7,6 +7,8 @@
> > >  #include <linux/clk-provider.h>
> > >  #include <linux/module.h>
> > >  #include <linux/platform_device.h>
> > > +#include <linux/pm_clock.h>
> > > +#include <linux/pm_runtime.h>
> > >  #include <linux/regmap.h>
> > >
> > >  #include <dt-bindings/clock/qcom,sm6350-camcc.h>
> > > @@ -1869,6 +1871,19 @@ MODULE_DEVICE_TABLE(of, camcc_sm6350_match_table);
> > >  static int camcc_sm6350_probe(struct platform_device *pdev)
> > >  {
> > >         struct regmap *regmap;
> > > +       int ret;
> > > +
> > > +       ret = devm_pm_runtime_enable(&pdev->dev);
> > > +       if (ret < 0)
> > > +               return ret;
> > > +
> > > +       ret = devm_pm_clk_create(&pdev->dev);
> > > +       if (ret < 0)
> > > +               return ret;
> >
> > This makes me wonder, what is the use for the pm_clk in your case? The
> > driver doesn't seem to use of_pm_clk_add_clk(), of_pm_clk_add_clks()
> > or pm_clk_add_clk(). So pm_clk_suspend() and pm_clk_resume() do
> > nothing.
>
> You're right that we're not using any of these functions in the driver.
> However still when camcc is not used, the associated power domain turns
> off correctly so that part works as expected.
>
> Honestly these lines have been copied from a different driver and I'm
> not familiar enough with the pm_runtime APIs to know what to use here
> without using the pm_clk* and pm_clk_suspend.

Please don't blindly C&P code.

>
> Basically we need, if any clock is being used in the driver, the
> power-domain needs to be enabled as well, and if nothing is used the
> power-domain can be disabled again.

Adding power-domains to the camcc node and calling
devm_pm_runtime_enable() should be enough. Please see how this is
managed for dispcc on sm8250.
  

On Tue Mar 7, 2023 at 4:06 PM CET, Dmitry Baryshkov wrote:
> On Tue, 7 Mar 2023 at 16:54, Luca Weiss <luca.weiss@fairphone.com> wrote:
> >
> > On Tue Feb 14, 2023 at 1:32 PM CET, Dmitry Baryshkov wrote:
> > > On Tue, 14 Feb 2023 at 13:01, Luca Weiss <luca.weiss@fairphone.com> wrote:
> > > >
> > > > Make sure that we can enable and disable the power domains used for
> > > > camcc when the clocks are and aren't used.
> > > >
> > > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> > > > ---
> > > >  drivers/clk/qcom/camcc-sm6350.c | 25 ++++++++++++++++++++++++-
> > > >  1 file changed, 24 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/clk/qcom/camcc-sm6350.c b/drivers/clk/qcom/camcc-sm6350.c
> > > > index acba9f99d960..fc5532e2ee5b 100644
> > > > --- a/drivers/clk/qcom/camcc-sm6350.c
> > > > +++ b/drivers/clk/qcom/camcc-sm6350.c
> > > > @@ -7,6 +7,8 @@
> > > >  #include <linux/clk-provider.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/platform_device.h>
> > > > +#include <linux/pm_clock.h>
> > > > +#include <linux/pm_runtime.h>
> > > >  #include <linux/regmap.h>
> > > >
> > > >  #include <dt-bindings/clock/qcom,sm6350-camcc.h>
> > > > @@ -1869,6 +1871,19 @@ MODULE_DEVICE_TABLE(of, camcc_sm6350_match_table);
> > > >  static int camcc_sm6350_probe(struct platform_device *pdev)
> > > >  {
> > > >         struct regmap *regmap;
> > > > +       int ret;
> > > > +
> > > > +       ret = devm_pm_runtime_enable(&pdev->dev);
> > > > +       if (ret < 0)
> > > > +               return ret;
> > > > +
> > > > +       ret = devm_pm_clk_create(&pdev->dev);
> > > > +       if (ret < 0)
> > > > +               return ret;
> > >
> > > This makes me wonder, what is the use for the pm_clk in your case? The
> > > driver doesn't seem to use of_pm_clk_add_clk(), of_pm_clk_add_clks()
> > > or pm_clk_add_clk(). So pm_clk_suspend() and pm_clk_resume() do
> > > nothing.
> >
> > You're right that we're not using any of these functions in the driver.
> > However still when camcc is not used, the associated power domain turns
> > off correctly so that part works as expected.
> >
> > Honestly these lines have been copied from a different driver and I'm
> > not familiar enough with the pm_runtime APIs to know what to use here
> > without using the pm_clk* and pm_clk_suspend.
>
> Please don't blindly C&P code.

I normally try to avoid this.. however pm_runtime is still quite a
mystery to me.

>
> >
> > Basically we need, if any clock is being used in the driver, the
> > power-domain needs to be enabled as well, and if nothing is used the
> > power-domain can be disabled again.
>
> Adding power-domains to the camcc node and calling
> devm_pm_runtime_enable() should be enough. Please see how this is
> managed for dispcc on sm8250.

Following that driver, so just using devm_pm_runtime_enable and
pm_runtime_resume_and_get doesn't seem to enable runtime_pm for the
device..

  $ cat /sys/devices/platform/soc@0/ad00000.clock-controller/power/runtime_status 
  unsupported

Also then I don't see the device in /sys/kernel/debug/pm_genpd/pm_genpd_summary

I'm guessing we still need to set the dev_pm_ops with something? Not
sure what to use here if it should just enable/disable the power domains
that are not handled by the driver directly. I also couldn't find any
example in existing code unfortuantely.

Any pointers?

Regards
Luca

>
> -- 
> With best wishes
> Dmitry