Re: [PATCH 3/3] clk: samsung: allow runtime PM with auto clock gating

[Krzysztof Kozlowski <krzk@kernel.org>]

On 09/01/2026 18:27, André Draszik wrote:
> When automatic clock gating is enabled, runtime PM (RPM) isn't entered
> even if enabled for a CMU if a sysreg clock exists and is provided by
> this CMU (as is generally the case).
> 
> The reason is that this driver acquires a CMU's sysreg registers using
> syscon_regmap_lookup_by_phandle() which ends up preparing the sysreg
> clock. Given the sysreg clock is provided by this CMU, this CMU's usage
> count is therefore bumped and RPM can not be entered as this CMU never
> becomes idle.
> 
> Switch to using device_node_to_regmap() which doesn't handle resources
> (the clock), leaving the CMU's usage count unaffected.
> 
> Note1: sysreg clock handling is completely removed with this commit

I miss where do you remove in this commit the sysreg clock handling?

> because sysreg register access is only required during suspend/resume.
> In the runtime suspend case, we would have to enable the clock to read
> the registers, but we can not do that as that would cause a resume of
> this driver which is not allowed. This is not a problem because we
> would only need to handle the clock manually if automatic clock gating
> wasn't enabled in the first. This code is only relevant if automatic
> clock gating is enabled, though.
> 
> Fixes: 298fac4f4b96 ("clk: samsung: Implement automatic clock gating mode for CMUs")
> Signed-off-by: André Draszik <andre.draszik@linaro.org>
> ---
>  drivers/clk/samsung/clk.c | 92 +++++++++++++++++++++++++++++++++++------------
>  drivers/clk/samsung/clk.h |  2 ++
>  2 files changed, 71 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
> index 9f68f079fd552f8dfb6898dbfb47dec0e84c626c..6515df81fcbc79b90f5262843e67575f6a4e0dda 100644
> --- a/drivers/clk/samsung/clk.c
> +++ b/drivers/clk/samsung/clk.c
> @@ -9,11 +9,13 @@
>   */
>  
>  #include <linux/slab.h>
> +#include <linux/clk.h>
>  #include <linux/clkdev.h>
>  #include <linux/clk-provider.h>
>  #include <linux/io.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/mod_devicetable.h>
> +#include <linux/of.h>
>  #include <linux/of_address.h>
>  #include <linux/regmap.h>
>  #include <linux/syscore_ops.h>
> @@ -489,6 +491,50 @@ void __init samsung_cmu_register_clocks(struct samsung_clk_provider *ctx,
>  		samsung_clk_register_cpu(ctx, cmu->cpu_clks, cmu->nr_cpu_clks);
>  }
>  
> +static int samsung_get_sysreg_regmap(struct device_node *np,
> +				     struct samsung_clk_provider *ctx)
> +{
> +	struct device_node *sysreg_np;
> +	struct clk *sysreg_clk;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	sysreg_np = of_parse_phandle(np, "samsung,sysreg", 0);
> +	if (!sysreg_np)
> +		return -ENODEV;
> +
> +	sysreg_clk = of_clk_get(sysreg_np, 0);

I don't think you should be poking clock of some other device. This
clearly breaks encapsulation. What sysreg is needs to do to access
registers, is only business of sysreg. No other drivers should
re-implement this.

> +	if (IS_ERR(sysreg_clk)) {
> +		ret = PTR_ERR(sysreg_clk);
> +		/* clock is optional */
> +		if (ret != -ENOENT) {
> +			pr_warn("%pOF: Unable to get sysreg clock: %d\n", np,
> +				ret);
> +			goto put_sysreg_np;
> +		}
> +		sysreg_clk = NULL;
> +	}


Best regards,
Krzysztof