[PATCH] regmap: mmio: prepare/unprepare clk only when read/write

[PATCH] regmap: mmio: prepare/unprepare clk only when read/write

[peng.fan]

From: Peng Fan <peng.fan@nxp.com>

use clk_prepare at the very beginning will invoke pm runtime resume,
if the clk is binded with a power domain. This will cause runtime
power high. Let's use clk_prepare_enable and clk_disable_unprepare
when read/write to avoid the runtime power issue.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/base/regmap/regmap-mmio.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/base/regmap/regmap-mmio.c b/drivers/base/regmap/regmap-mmio.c
index af967d8f975e..a1ad7419c4a3 100644
--- a/drivers/base/regmap/regmap-mmio.c
+++ b/drivers/base/regmap/regmap-mmio.c
@@ -118,7 +118,7 @@ static int regmap_mmio_write(void *context, unsigned int reg, unsigned int val)
 	int ret;
 
 	if (!IS_ERR(ctx->clk)) {
-		ret = clk_enable(ctx->clk);
+		ret = clk_prepare_enable(ctx->clk);
 		if (ret < 0)
 			return ret;
 	}
@@ -126,7 +126,7 @@ static int regmap_mmio_write(void *context, unsigned int reg, unsigned int val)
 	ctx->reg_write(ctx, reg, val);
 
 	if (!IS_ERR(ctx->clk))
-		clk_disable(ctx->clk);
+		clk_disable_unprepare(ctx->clk);
 
 	return 0;
 }
@@ -175,7 +175,7 @@ static int regmap_mmio_read(void *context, unsigned int reg, unsigned int *val)
 	int ret;
 
 	if (!IS_ERR(ctx->clk)) {
-		ret = clk_enable(ctx->clk);
+		ret = clk_prepare_enable(ctx->clk);
 		if (ret < 0)
 			return ret;
 	}
@@ -183,7 +183,7 @@ static int regmap_mmio_read(void *context, unsigned int reg, unsigned int *val)
 	*val = ctx->reg_read(ctx, reg);
 
 	if (!IS_ERR(ctx->clk))
-		clk_disable(ctx->clk);
+		clk_disable_unprepare(ctx->clk);
 
 	return 0;
 }
@@ -193,7 +193,6 @@ static void regmap_mmio_free_context(void *context)
 	struct regmap_mmio_context *ctx = context;
 
 	if (!IS_ERR(ctx->clk)) {
-		clk_unprepare(ctx->clk);
 		if (!ctx->attached_clk)
 			clk_put(ctx->clk);
 	}
@@ -305,12 +304,6 @@ static struct regmap_mmio_context *regmap_mmio_gen_context(struct device *dev,
 		goto err_free;
 	}
 
-	ret = clk_prepare(ctx->clk);
-	if (ret < 0) {
-		clk_put(ctx->clk);
-		goto err_free;
-	}
-
 	return ctx;
 
 err_free:
@@ -361,7 +354,7 @@ int regmap_mmio_attach_clk(struct regmap *map, struct clk *clk)
 	ctx->clk = clk;
 	ctx->attached_clk = true;
 
-	return clk_prepare(ctx->clk);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(regmap_mmio_attach_clk);
 
@@ -369,8 +362,6 @@ void regmap_mmio_detach_clk(struct regmap *map)
 {
 	struct regmap_mmio_context *ctx = map->bus_context;
 
-	clk_unprepare(ctx->clk);
-
 	ctx->attached_clk = false;
 	ctx->clk = NULL;
 }
-- 
2.16.4

RE: [PATCH] regmap: mmio: prepare/unprepare clk only when read/write

[Aisheng Dong]

> From: Peng Fan <peng.fan@nxp.com>
> Sent: Thursday, April 23, 2020 1:47 PM
> 
> use clk_prepare at the very beginning will invoke pm runtime resume, if the clk is
> binded with a power domain. This will cause runtime power high. Let's use
> clk_prepare_enable and clk_disable_unprepare when read/write to avoid the
> runtime power issue.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>

I wonder you may can't do that because mmio bus is using spinlock.

Regards
Aisheng

> ---
>  drivers/base/regmap/regmap-mmio.c | 19 +++++--------------
>  1 file changed, 5 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/base/regmap/regmap-mmio.c
> b/drivers/base/regmap/regmap-mmio.c
> index af967d8f975e..a1ad7419c4a3 100644
> --- a/drivers/base/regmap/regmap-mmio.c
> +++ b/drivers/base/regmap/regmap-mmio.c
> @@ -118,7 +118,7 @@ static int regmap_mmio_write(void *context, unsigned
> int reg, unsigned int val)
>  	int ret;
> 
>  	if (!IS_ERR(ctx->clk)) {
> -		ret = clk_enable(ctx->clk);
> +		ret = clk_prepare_enable(ctx->clk);
>  		if (ret < 0)
>  			return ret;
>  	}
> @@ -126,7 +126,7 @@ static int regmap_mmio_write(void *context, unsigned
> int reg, unsigned int val)
>  	ctx->reg_write(ctx, reg, val);
> 
>  	if (!IS_ERR(ctx->clk))
> -		clk_disable(ctx->clk);
> +		clk_disable_unprepare(ctx->clk);
> 
>  	return 0;
>  }
> @@ -175,7 +175,7 @@ static int regmap_mmio_read(void *context, unsigned
> int reg, unsigned int *val)
>  	int ret;
> 
>  	if (!IS_ERR(ctx->clk)) {
> -		ret = clk_enable(ctx->clk);
> +		ret = clk_prepare_enable(ctx->clk);
>  		if (ret < 0)
>  			return ret;
>  	}
> @@ -183,7 +183,7 @@ static int regmap_mmio_read(void *context, unsigned
> int reg, unsigned int *val)
>  	*val = ctx->reg_read(ctx, reg);
> 
>  	if (!IS_ERR(ctx->clk))
> -		clk_disable(ctx->clk);
> +		clk_disable_unprepare(ctx->clk);
> 
>  	return 0;
>  }
> @@ -193,7 +193,6 @@ static void regmap_mmio_free_context(void *context)
>  	struct regmap_mmio_context *ctx = context;
> 
>  	if (!IS_ERR(ctx->clk)) {
> -		clk_unprepare(ctx->clk);
>  		if (!ctx->attached_clk)
>  			clk_put(ctx->clk);
>  	}
> @@ -305,12 +304,6 @@ static struct regmap_mmio_context
> *regmap_mmio_gen_context(struct device *dev,
>  		goto err_free;
>  	}
> 
> -	ret = clk_prepare(ctx->clk);
> -	if (ret < 0) {
> -		clk_put(ctx->clk);
> -		goto err_free;
> -	}
> -
>  	return ctx;
> 
>  err_free:
> @@ -361,7 +354,7 @@ int regmap_mmio_attach_clk(struct regmap *map,
> struct clk *clk)
>  	ctx->clk = clk;
>  	ctx->attached_clk = true;
> 
> -	return clk_prepare(ctx->clk);
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(regmap_mmio_attach_clk);
> 
> @@ -369,8 +362,6 @@ void regmap_mmio_detach_clk(struct regmap *map)
> {
>  	struct regmap_mmio_context *ctx = map->bus_context;
> 
> -	clk_unprepare(ctx->clk);
> -
>  	ctx->attached_clk = false;
>  	ctx->clk = NULL;
>  }
> --
> 2.16.4

Re: [PATCH] regmap: mmio: prepare/unprepare clk only when read/write

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 520 bytes --]

On Thu, Apr 23, 2020 at 01:46:31PM +0800, peng.fan@nxp.com wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> use clk_prepare at the very beginning will invoke pm runtime resume,
> if the clk is binded with a power domain. This will cause runtime
> power high. Let's use clk_prepare_enable and clk_disable_unprepare
> when read/write to avoid the runtime power issue.

This will mean that we're doing clk_prepare() during I/O which isn't
good since for MMIO regmaps we support I/O operations in atomic
contexts.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH] regmap: mmio: prepare/unprepare clk only when read/write

[Peng Fan]

> Subject: Re: [PATCH] regmap: mmio: prepare/unprepare clk only when
> read/write
> 
> On Thu, Apr 23, 2020 at 01:46:31PM +0800, peng.fan@nxp.com wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > use clk_prepare at the very beginning will invoke pm runtime resume,
> > if the clk is binded with a power domain. This will cause runtime
> > power high. Let's use clk_prepare_enable and clk_disable_unprepare
> > when read/write to avoid the runtime power issue.
> 
> This will mean that we're doing clk_prepare() during I/O which isn't good
> since for MMIO regmaps we support I/O operations in atomic contexts.

Ah, yes. Do you have any suggestions? If we use clk_prepare at the very
beginning, the power that binded to the clk device will be always on,
and cause power consumption high.

Thanks,
Peng.

Re: [PATCH] regmap: mmio: prepare/unprepare clk only when read/write

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 815 bytes --]

On Thu, Apr 23, 2020 at 10:51:36AM +0000, Peng Fan wrote:

> > This will mean that we're doing clk_prepare() during I/O which isn't good
> > since for MMIO regmaps we support I/O operations in atomic contexts.

> Ah, yes. Do you have any suggestions? If we use clk_prepare at the very
> beginning, the power that binded to the clk device will be always on,
> and cause power consumption high.

Not really in regmap itself - I think your best bet here is to move the
clock handling out of regmap and up into the driver.  regmap won't do
any I/O unless your driver calls it so this will work fine, this support
is only there in regmap as a convenience but if it's getting in the way
then you're probably going to be able to do a better job pushing this
into rutime PM in the driver or just having direct clock calls.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH] regmap: mmio: prepare/unprepare clk only when read/write

[Peng Fan]

> Subject: Re: [PATCH] regmap: mmio: prepare/unprepare clk only when
> read/write
> 
> On Thu, Apr 23, 2020 at 10:51:36AM +0000, Peng Fan wrote:
> 
> > > This will mean that we're doing clk_prepare() during I/O which isn't
> > > good since for MMIO regmaps we support I/O operations in atomic
> contexts.
> 
> > Ah, yes. Do you have any suggestions? If we use clk_prepare at the
> > very beginning, the power that binded to the clk device will be always
> > on, and cause power consumption high.
> 
> Not really in regmap itself - I think your best bet here is to move the clock
> handling out of regmap and up into the driver.  regmap won't do any I/O
> unless your driver calls it so this will work fine, this support is only there in
> regmap as a convenience but if it's getting in the way then you're probably
> going to be able to do a better job pushing this into rutime PM in the driver or
> just having direct clock calls.

If we not pass clk to regmap, accessing regmap registers will hang system with
Debugfs enabled.

If we pass clk to regmap, it will make the runtime pm power high, because
regmap mmio will call clk_prepare at the beginning.

That's why use clk_prepare_enable and clk_disable_unprepare.

Thinking about another direction, how about add clk_prepare and clk_unprepare
into debugfs ops open/close?

Thanks,
Peng.

Re: [PATCH] regmap: mmio: prepare/unprepare clk only when read/write

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 921 bytes --]

On Fri, Apr 24, 2020 at 01:27:29AM +0000, Peng Fan wrote:

> If we not pass clk to regmap, accessing regmap registers will hang system with
> Debugfs enabled.

If you're not using a cache then that'll be a problem, however there is
a flag runtime_pm in the regmap config which when set should cause the
device to be runtime PM enabled when it's accessed so if you do your
clock management in runtime PM it should still get enabled.  I *think*
that interacts OK with being in an atomic context but I can't say I've
verified.

> If we pass clk to regmap, it will make the runtime pm power high, because
> regmap mmio will call clk_prepare at the beginning.

> That's why use clk_prepare_enable and clk_disable_unprepare.

> Thinking about another direction, how about add clk_prepare and clk_unprepare
> into debugfs ops open/close?

Something still has to prepare the clocks for normal operation...  

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

regmap: mmio: lack of runtime_pm support for debugfs

[Brian Norris]

[[ Retitling from '[PATCH] regmap: mmio: prepare/unprepare clk only when
   read/write' ]]

Hi,

Sorry to bring up an old thread, but it appears there's an unresolved
problem in here that I'm also hitting:

On Fri, Apr 24, 2020 at 11:30:04AM +0100, Mark Brown wrote:
> On Fri, Apr 24, 2020 at 01:27:29AM +0000, Peng Fan wrote:
> 
> > If we not pass clk to regmap, accessing regmap registers will hang system with
> > Debugfs enabled.
> 
> If you're not using a cache then that'll be a problem, however there is
> a flag runtime_pm in the regmap config which when set should cause the
> device to be runtime PM enabled when it's accessed so if you do your
> clock management in runtime PM it should still get enabled.  I *think*
> that interacts OK with being in an atomic context but I can't say I've
> verified.

The only 'runtime_pm' flag I'm finding for regmap is for regmap_irq_chip
-- there isn't anything useful for other kinds of regmaps (like MMIO).

If this seems like an expected/useful feature, I'll look at adding it to
the generic 'struct regmap_config' / drivers/base/regmap/regmap.c.

This could be tricky in theory given the atomic context requirements,
but in reality, I think it would still be OK: this feature would really
be useful _only_ for otherwise-unregulated contexts, like debugfs
access (where we can sleep). For all non-debugfs accesses, we expect to
already be RPM_ACTIVE, because the driver should already be managing
runtime PM.

Brian

Re: regmap: mmio: lack of runtime_pm support for debugfs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1388 bytes --]

On Mon, Jan 24, 2022 at 03:50:23PM -0800, Brian Norris wrote:

> The only 'runtime_pm' flag I'm finding for regmap is for regmap_irq_chip
> -- there isn't anything useful for other kinds of regmaps (like MMIO).

> If this seems like an expected/useful feature, I'll look at adding it to
> the generic 'struct regmap_config' / drivers/base/regmap/regmap.c.

> This could be tricky in theory given the atomic context requirements,
> but in reality, I think it would still be OK: this feature would really
> be useful _only_ for otherwise-unregulated contexts, like debugfs
> access (where we can sleep). For all non-debugfs accesses, we expect to
> already be RPM_ACTIVE, because the driver should already be managing
> runtime PM.

Are you sure you wouldn't be better off with a cache here, or marking
the registers as precious so they don't get read (perhaps conditionally
to allow reading while the device is live)?  The general idea
expectation with the debugfs is that it shouldn't materially affect the
device, this would mean that it could cause the power to get bounced on
which feels like it might lead to surprises.  If you are sending a patch
adding support for this it should be debugfs specific, not a general
flag so it's clear that it's not going to do power management outside of
debugfs, as you say in the normal case it seems better for the driver to
do power management.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: regmap: mmio: lack of runtime_pm support for debugfs

[Brian Norris]

Hi Mark,

On Fri, Feb 4, 2022 at 11:02 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Jan 24, 2022 at 03:50:23PM -0800, Brian Norris wrote:
>
> > The only 'runtime_pm' flag I'm finding for regmap is for regmap_irq_chip
> > -- there isn't anything useful for other kinds of regmaps (like MMIO).
>
> > If this seems like an expected/useful feature, I'll look at adding it to
> > the generic 'struct regmap_config' / drivers/base/regmap/regmap.c.
>
> > This could be tricky in theory given the atomic context requirements,
> > but in reality, I think it would still be OK: this feature would really
> > be useful _only_ for otherwise-unregulated contexts, like debugfs
> > access (where we can sleep). For all non-debugfs accesses, we expect to
> > already be RPM_ACTIVE, because the driver should already be managing
> > runtime PM.
>
> Are you sure you wouldn't be better off with a cache here, or marking
> the registers as precious so they don't get read (perhaps conditionally
> to allow reading while the device is live)?

We do actually have a cache for the case I care about, but there's
also a debugfs file for bypassing that and...for some reason I'm
dealing with some diagnostic scripts that purposely toggle that. I'm
not sure how wise that is, but in general I like to reduce the number
of ways dumb user space can screw things up. I've even had to tell
people that recursively grepping through sysfs or debugfs is a bad
idea...

The precious_reg() callback is interesting. Maybe I could have that do
a quick pm_runtime check to make a decision. Thanks for the idea.

> The general idea
> expectation with the debugfs is that it shouldn't materially affect the
> device, this would mean that it could cause the power to get bounced on
> which feels like it might lead to surprises.

Yeah, good point.

> If you are sending a patch
> adding support for this it should be debugfs specific, not a general
> flag so it's clear that it's not going to do power management outside of
> debugfs, as you say in the normal case it seems better for the driver to
> do power management.

Another good point. I did poke around a bit on trying to do it
generically, and it looked I'd have to touch a lot of stuff to avoid
sleeping/atomic conflicts, while likely having no real material
benefit (for well-written uses) outside of debugfs.

Anyway, I'll probably just go with precious_reg() as suggested above.

Thanks a lot for the tips,
Brian

Re: regmap: mmio: lack of runtime_pm support for debugfs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1075 bytes --]

On Fri, Feb 04, 2022 at 11:21:51AM -0800, Brian Norris wrote:
> On Fri, Feb 4, 2022 at 11:02 AM Mark Brown <broonie@kernel.org> wrote:

> > Are you sure you wouldn't be better off with a cache here, or marking
> > the registers as precious so they don't get read (perhaps conditionally
> > to allow reading while the device is live)?

> We do actually have a cache for the case I care about, but there's
> also a debugfs file for bypassing that and...for some reason I'm
> dealing with some diagnostic scripts that purposely toggle that. I'm
> not sure how wise that is, but in general I like to reduce the number
> of ways dumb user space can screw things up. I've even had to tell
> people that recursively grepping through sysfs or debugfs is a bad
> idea...

Are you sure this extra thing that bypasses the cache isn't an out of
tree patch?  It really doesn't sound like a good idea to have that, and
if people think they need it they probably have drivers doing something
quite unfortunate.  Or are you just looking at the upstream debugfs with
some volatile registers?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: regmap: mmio: lack of runtime_pm support for debugfs

[Brian Norris]

On Fri, Feb 4, 2022 at 11:41 AM Mark Brown <broonie@kernel.org> wrote:
> On Fri, Feb 04, 2022 at 11:21:51AM -0800, Brian Norris wrote:
> > We do actually have a cache for the case I care about, but there's
> > also a debugfs file for bypassing that and...for some reason I'm
> > dealing with some diagnostic scripts that purposely toggle that. I'm
> > not sure how wise that is, but in general I like to reduce the number
> > of ways dumb user space can screw things up. I've even had to tell
> > people that recursively grepping through sysfs or debugfs is a bad
> > idea...
>
> Are you sure this extra thing that bypasses the cache isn't an out of
> tree patch?  It really doesn't sound like a good idea to have that, and
> if people think they need it they probably have drivers doing something
> quite unfortunate.  Or are you just looking at the upstream debugfs with
> some volatile registers?

https://git.kernel.org/linus/d3dc5430d68fb91a62d971648170b34d46ab85bc
regmap: debugfs: Allow writes to cache state settings

And it's still there:
https://elixir.bootlin.com/linux/v5.16.5/source/drivers/base/regmap/regmap-debugfs.c#L521

Again, I didn't claim the *use* of that bypass in the aforementioned
diagnostic script was a good idea...but it's at least possible. I've
already carved out more exceptions so that script doesn't do bad
things on some systems, but maybe I'll poke the authors even more, to
see if they have a good reason for using the bypass.

Brian

Re: regmap: mmio: lack of runtime_pm support for debugfs

[Brian Norris]

After a bit more thinking:

On Fri, Feb 4, 2022 at 11:21 AM Brian Norris <briannorris@chromium.org> wrote:
> Anyway, I'll probably just go with precious_reg() as suggested above.

Unfortunately, precious_reg() wouldn't be quite safe either -- we
could do a single check for pm_runtime_active(), but we have no way of
knowing (guaranteeing) that it will remain active throughout the
remainder of the debugfs operation. For all we know, some other actor
could be *just* finishing with (and PM-suspending) our domain
immediately after the check. And we can't grab a reference, because we
don't have a callback for balancing that back out.

If I really want to solve this, I might have to go with adding a
debugfs-specific runtime_pm flag, and (to avoid too many side effects)
use that with pm_runtime_get_if_active() in regmap-debugfs.c.

Brian

Re: regmap: mmio: lack of runtime_pm support for debugfs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1423 bytes --]

On Fri, Feb 04, 2022 at 11:53:01AM -0800, Brian Norris wrote:
> On Fri, Feb 4, 2022 at 11:41 AM Mark Brown <broonie@kernel.org> wrote:
> > On Fri, Feb 04, 2022 at 11:21:51AM -0800, Brian Norris wrote:

> > > We do actually have a cache for the case I care about, but there's
> > > also a debugfs file for bypassing that and...for some reason I'm
> > > dealing with some diagnostic scripts that purposely toggle that. I'm

> > Are you sure this extra thing that bypasses the cache isn't an out of
> > tree patch?  It really doesn't sound like a good idea to have that, and
> > if people think they need it they probably have drivers doing something
> > quite unfortunate.  Or are you just looking at the upstream debugfs with
> > some volatile registers?

> Again, I didn't claim the *use* of that bypass in the aforementioned
> diagnostic script was a good idea...but it's at least possible. I've
> already carved out more exceptions so that script doesn't do bad
> things on some systems, but maybe I'll poke the authors even more, to
> see if they have a good reason for using the bypass.

So it's fiddling about with bypass mode by hand rather than
something that specifically shows you the underlying device.
That is also a terrible idea for routine use, if it's telling you
anything different from what the cache says the driver is buggy
and if the script causes that to happen then it itself is likely
to be the bug.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]