| Message ID | 20230213-sm6350-camcc-runtime_pm-v3-1-d35e0d833cc4@fairphone.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add pm_runtime support to SM6350 camcc | expand |
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
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, }, };
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(-)