Date:   Tue, 21 Jun 2022 16:31:40 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Aisheng Dong <aisheng.dong@....com>
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

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.

> 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.

> 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.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists