[PATCH v3 0/2] Add pm_runtime support to SM6350 camcc

[PATCH v3 0/2] Add pm_runtime support to SM6350 camcc

[Luca Weiss]

As pointed out during patch review, we should make sure that we turn on
the CX power domain when camcc is in use, and also disable it (or remove
our vote on it) when camcc is not in use.

For this add pm_runtime support to the driver and stick the power-domain
in the devicetree.

Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
Changes in v3:
- Remove extra error print for qcom_cc_really_probe since driver core
  already prints the error anyways
- Fix From: and Sign-offs email address confusion, sorry about that...
- Link to v2: https://lore.kernel.org/r/20230213-sm6350-camcc-runtime_pm-v2-0-60a507bf3e68@z3ntu.xyz

Changes in v2:
- no change resend since I messed up To/CC in the initial submission
- Link to v1: https://lore.kernel.org/r/20230213-sm6350-camcc-runtime_pm-v1-0-761fc69f7a80@z3ntu.xyz

---
Luca Weiss (2):
      clk: qcom: camcc-sm6350: add pm_runtime support
      arm64: dts: qcom: sm6350: add power domain to camcc

 arch/arm64/boot/dts/qcom/sm6350.dtsi |  2 ++
 drivers/clk/qcom/camcc-sm6350.c      | 25 ++++++++++++++++++++++++-
 2 files changed, 26 insertions(+), 1 deletion(-)
---
base-commit: 09e41676e35ab06e4bce8870ea3bf1f191c3cb90
change-id: 20230213-sm6350-camcc-runtime_pm-639b75c3d4b1

Best regards,
-- 
Luca Weiss <luca.weiss@fairphone.com>

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

[Luca Weiss]

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;
+
+	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

[PATCH v3 2/2] arm64: dts: qcom: sm6350: add power domain to camcc

[Luca Weiss]

Add the CX power domain to the camcc node so the power domain gets
marked as in-use when camcc is used.

Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
 arch/arm64/boot/dts/qcom/sm6350.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi b/arch/arm64/boot/dts/qcom/sm6350.dtsi
index 1e1d366c92c1..62d6dcd8d1fe 100644
--- a/arch/arm64/boot/dts/qcom/sm6350.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi
@@ -1507,6 +1507,8 @@ camcc: clock-controller@ad00000 {
 			compatible = "qcom,sm6350-camcc";
 			reg = <0 0x0ad00000 0 0x16000>;
 			clocks = <&rpmhcc RPMH_CXO_CLK>;
+			power-domains = <&rpmhpd SM6350_CX>;
+			required-opps = <&rpmhpd_opp_low_svs>;
 			#clock-cells = <1>;
 			#reset-cells = <1>;
 			#power-domain-cells = <1>;

-- 
2.39.1

Re: [PATCH v3 2/2] arm64: dts: qcom: sm6350: add power domain to camcc

[Krzysztof Kozlowski]

On 14/02/2023 12:01, Luca Weiss wrote:
> Add the CX power domain to the camcc node so the power domain gets
> marked as in-use when camcc is used.
> 
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
>  arch/arm64/boot/dts/qcom/sm6350.dtsi | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> index 1e1d366c92c1..62d6dcd8d1fe 100644
> --- a/arch/arm64/boot/dts/qcom/sm6350.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> @@ -1507,6 +1507,8 @@ camcc: clock-controller@ad00000 {
>  			compatible = "qcom,sm6350-camcc";
>  			reg = <0 0x0ad00000 0 0x16000>;
>  			clocks = <&rpmhcc RPMH_CXO_CLK>;
> +			power-domains = <&rpmhpd SM6350_CX>;
> +			required-opps = <&rpmhpd_opp_low_svs>;

Does not look like you tested the DTS against bindings. Please run `make
dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
for instructions).

Best regards,
Krzysztof

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

[Dmitry Baryshkov]

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
>


-- 
With best wishes
Dmitry

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

[Luca Weiss]

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

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

[Dmitry Baryshkov]

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.

-- 
With best wishes
Dmitry

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

[Luca Weiss]

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