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

["André Draszik" <andre.draszik@linaro.org>]

On Mon, 2026-01-19 at 15:00 +0000, André Draszik wrote:
> Hi Krzysztof,
> 
> On Sat, 2026-01-17 at 20:25 +0100, Krzysztof Kozlowski wrote:
> > 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?
> 
> The CMU driver originally used syscon_regmap_lookup_by_phandle() for sysreg,
> which does clk_get() and clk_prepare(), and then implicitly and for each
> register access clk_enable()/disable() for the clock specified in DT for the
> (sysreg) node.
> 
> This commit changes it to using device_node_to_regmap(), which does nothing
> with the clock (or any other resources). I opted to have the CMU driver still
> do a clk_get(), so that the relationship is still visible, e.g. in
> $debugfs/clk/clk_summary.
> 
> > > 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.
> 
> Similarly, to how other drivers (like UFS and peric/USI) also poke in
> the sysreg region in a similar way, the CMU related parts of sysreg are
> handled in this driver, though, as only the CMU driver knows how to
> configure them based on the gating (auto vs. manual).
> 
> No separate driver was sysreg was added when this code was introduced.
> The clk_get() used to be part of syscon_regmap_lookup_by_phandle(), and
> I chose to keep only that, as per above.
> 
> I propose to add an actual sysreg driver once the need arises, then
> this can also move, but for now keeping it here seems straight forward.
> 
> Is that OK with you?

Forgot to say, I can of course remove all clock related code (clk_get() etc).

What do you think? Is that acceptable?


CHeers,
Andre'