Message-ID: <DB9PR04MB84772F73D4AA1B49F87BECCF80B39@DB9PR04MB8477.eurprd04.prod.outlook.com>
Date:   Tue, 21 Jun 2022 18:16:50 +0000
From:   Aisheng Dong <aisheng.dong@....com>
To:     Mark Brown <broonie@...nel.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "dongas86@...il.com" <dongas86@...il.com>,
        "l.stach@...gutronix.de" <l.stach@...gutronix.de>,
        Peng Fan <peng.fan@....com>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>
Subject: RE: [PATCH RFC 1/2] regmap: add option to disable debugfs

> From: Mark Brown <broonie@...nel.org>
> Sent: Tuesday, June 21, 2022 11:32 PM
> 
> On Tue, Jun 21, 2022 at 02:56:58PM +0000, Aisheng Dong wrote:
> 
> > > so if we can't satisfy the read from the cache then we'll hit the
> > > cache_only check and return -EBUSY before we start trying to do any
> > > physical I/O.  The debugfs code will handle that gracefully,
> > > indicating that it couldn't get a value for the volatile register by
> > > showing all Xs for the value.  If none of the registers are cached
> > > then the file won't be terribly useful but it at least shouldn't cause any
> errors with accessing the device when it's powered down.
> 
> > That means we have to use cache_only mode to workaround the issue, right?
> > I saw that cache_only mode has to be explicated enable/disable by
> > driver, commonly used in device rpm in kernel right now.
> 
> Yes.
> 
> > However, things are a little bit complicated on i.MX that 1) imx
> > blkctl needs follow strict registers r/w flow interleaved with
> > handshakes with upstream gpc power operations and delay may be needed
> > between registers writing
> > 2) blkctl itself does not enable runtime pm, it simply call rpm to
> > resume upstream power domains devices and necessary clocks before r/w
> registers.
> 
> I'm not sure I fully understand the issue here?  If the driver can safely manage
> the hardware surely it can safely manage cache only mode too?  If there are
> duplicate resumes then cache only enable/disable is a boolean flag rather than
> refcounted so that shouldn't be a problem.
> 

I still can't see an easy and safe to way to do it.
What I'm wondering is whether it's worth to do it if need to introducing considerable
complexities in driver to overcome this issue if it can be simply disabled.
Anyway, I will try to investigate it.

> > Furthermore, current imx blkctl is a common driver used by many devices
> [1].
> > Introducing volatile registers and cache may bloat the driver a lot
> unnecessarily.
> 
> You don't actually need to have a cache to use cache only mode, if there are
> no cached registers then you'll just get -EBUSY on any access to the registers
> but that's hopefully fine since at the minute things will just fall over anyway.
> You shouldn't even need to flag registers as volatile if there's no cache since it's
> not really relevant without a cache.
> 

After a quick try initially, I found two issues:
1. There's a warning dump if using cache_only without cache
void regcache_cache_only(struct regmap *map, bool enable)
{
        map->lock(map->lock_arg);
        WARN_ON(map->cache_bypass && enable);
        ...
}
2. It seems _regmap_write() did not handle cache_only case well without cache.
It may still writes HW even for cache_only mode?

I guess we may need fix those two issues first before we can safely use it?

> > The simplest way for i.MX case may be just disabling debugfs as it
> > can't reflect the actually HW state without power. So we would wish the
> regmap core could provide an option to users.
> 
> The goal here is to describe the regmap itself so that there's less fragility as
> things change and we don't needlessly disable anything else that happens to
> be added to debugfs in the future due to having an overly generic flag, and we
> get the ability to directly catch access to the hardware that misses doing
> power management properly while we're at it.
> 
> We already have a way to describe it being unsafe to access the hardware, we
> may as well use it.
> 
> > And I noticed that syscon [2] seems have the same issue since the following
> commit:
> 
> > commit 9b947a13e7f6017f18470f665992dbae267852b3
> > Author: David Lechner <david@...hnology.com>
> > Date:   Mon Feb 19 15:43:02 2018 -0600
> 
> >     regmap: use debugfs even when no device
> 
> >     This registers regmaps with debugfs even when they do not have an
> >     associated device. For example, this is common for syscon regmaps.
> 
> That's a different thing, that's due to us naming the directory after the struct
> device but syscons being created before we have that struct device available.

Yes, but syscon has the same issue that the system may hang if dump registers
through debugfs without power on.
How would you suggest for this case as syscon is also a common driver?

Regards
Aisheng

Powered by blists - more mailing lists