Re: [PATCH v2 2/5] mfd: lochnagar: Add support for the Cirrus Logic Lochnagar
From: Richard Fitzgerald
Date: Mon Oct 29 2018 - 07:53:10 EST
On 29/10/18 11:04, Lee Jones wrote:
On Fri, 26 Oct 2018, Mark Brown wrote:
On Fri, Oct 26, 2018 at 09:00:51AM +0100, Lee Jones wrote:
On Thu, 25 Oct 2018, Richard Fitzgerald wrote:
Largely the point. How long do you think it would take to populate the
cache if you had to read thousands of registers over I2C? Boot time matters.
Deferring it until it's touched can create various unpredictable and
annoying behaviour later, for example if a lot of cache entries are
written while the chip is asleep and the initial values weren't known
then a suspend/resume cannot filter out writes that are setting the
register to its default (which regmap does to avoid unnecessary bus traffic).
So the resume could have a large amount of unnecessary overhead writing
registers to a value they already have or reading the initial values of
those registers.
One more register read when initially writing to a register and one
more when resuming doesn't sound like a vast amount of over-head.
Especially on resume extra register I/O really adds up - people really
care how long their system takes to come back from suspend, and how
quickly individual devices come back. For devices that are on slow
buses like I2C this means that every register operation counts. Boot
can be similarly pressured of course, though it's a less frequent issue
for these devices.
Not sure what you think I was suggesting above. If the default values
are actually non-zero that's fine - we'll either leave them as they
are (if they are never changed, in which case Regmap doesn't even need
to know about them), document only those (non-zero) ones or wait until
they are read for the first time, then populate the cache.
You can't assume that the device is in power on reset state unless the
driver reset it itself which may or may not be a good idea or even
possible, sometimes it's what you want but at other times even if it's
possible it can cause user visible disruption during the boot process
which is undesirable.
Setting up the cache manually also sounds like a vector for potential
failure. At least if you were to cache dynamically on first write
(either on start-up or after sleep) then the actual value will be
cached, rather than what a piece of C code says it should be.
Even where there's no problem getting the hardware into a clean state it
can rapidly get very, very expensive to do this with larger register
sets on slow buses, and at the framework level we can't assume that
readback support is even present on the device (the earliest versions of
cache support were written to support such devices). Some of the
userspaces that regmap devices get used with end up wanting to apply a
bunch of configuration at startup, if we can cut down on the amount of
I/O that's involved in doing that it can help them quite a bit. You
also get userspaces that want to enumerate device state at startup,
that's a bit easier to change in userspace but it's not an unreasonable
thing to want to do and can also get very I/O heavy.
There is some potential for errors to be introduced but equally these
tables can be both generated and verified mechanically, tasks that are
particularly straightforward for the device vendors to do. There are
also potential risks in doing this at runtime if we didn't get the
device reset, if we don't accurately mark the volatile registers as
volatile or if there's otherwise bugs in the code.
Precisely my point. Lochnagar is a small device yet it's required to
submit hundreds of lines of Regmap tables. Imagine what that would
look like for a large device.
There's no *requirement* to provide the data even if you're using the
cache (and the cache support is entirely optional), there's just costs
to not providing it in terms of what features you can get from the
regmap API and the performance of the system. Not every device is going
to be bothered by those costs, many devices don't provide all of the
data they could.
So what do we do in the case where, due to the size of the device, the
amount of lines required by these tables go from crazy to grotesque?
I'm not clear to me that Lochnagar will particularly benefit from
providing the cache defaults but it sounds like you've raised concerns
about other devices which would, and it seems clear that the readability
information is very useful for this device if there's registers that
it's unsafe to try to read from.
Any reduction in lines would be a good thing. Charles, could you
please define what specific benefits you gain from providing providing
the pre-cache data please? With a particular emphasis on whether the
trade-off is justified.
Why so much concern over the number of source lines of a data table?
If we were talking about removing lines of executable code to make it
more efficient - yes, that's a good thing.
Worrying about the number of lines of a data table, I don't see the
imperative for that.
Since this seems to be a significant part of your objection it would help
if you could tell us WHY you object so much to lines of tables.
Even if it is absolutely critical that you have to supply these to
Regmap up-front, instead of on first use/read, why can't you just
supply the oddball non-zero ones?
That doesn't work, we need to know both if the register has a default
value and what that value is - there's no great value in only supplying
the defaults for registers with non-zero values.
All registers have a default value. Why can't we assume that if a
register is writable and a default value was omitted then the default
is zero?
Ah wait! As I was typing the above, I just had a thought. We don't
actually provide a list of writable registers do we? Only a the
ability to verify if one is writable (presumably) before a write.
That's frustrating!
If you aren't happy with the regmap subsystem you could send some
patches to change it to what you would be happy with (and patch the ~1300
drivers that use it)
That may well happen. This is the pre-patch discussion.
Apologies that it had to happen on your submission, but this is that
alerted me to the issue(s).
The regmap cache API has been this way since it was introduced in 2011
FWIW, we did add range based support later which is purely data driven.
Utilising range support here would certainly help IMHO.
Like any kernel subsystem it has an API that we have to obey to be able to
use it.
Again, this isn't about Lochnagar.
I think from the perspective of Richard and Charles who are just trying
to get their driver merged this is something of an abstract distinction.
If the driver were merged and this discussion were happening separately
their perspective would most likely be different.
Charles has already mentioned that he'd take a look at the current
*use* (not changing the API, but the way in which Lochnagar
*uses/consumes* it). Actions which would be most welcomed.
Maybe Charles will find an alternative that doesn't affect performance/functionality
or is an acceptable tradeoff. But still, it seems an odd maintainer requirement
"please use this other subsystem less effectively to make _my_ subsystem have fewer
lines of source".
The API is obscene and needs a re-work IMHO.
I really hope we do not really have to list every register, but *if we
absolutely must*, let's do it once:
REG_ADDRESS, WRV, INITIAL_VALUE
There is the option to specify range based access tables instead of a
function, for a lot of devices this is a nice, easy way to specify
things that makes more sense so we support it. We don't combine the
tables because they're range based and if there is 100% overlap you can
always just point at the same table.
We allow the functions partly because it lets people handle weird cases
but also because it turned out when I looked at this that a lot of the
time the compiler output for switch statements was pretty efficient with
sparse register maps and it makes the code incredibly simple, much
simpler than trying to parse access tables into a more efficient data
structure and IIRC more compact too. It's possible that those tradeoffs
have changed since but at the time there was a class of devices where it
wasn't clear that putting more effort in would result in substantially
better outcomes.
To re-iterate, regmap is a standard kernel subsystem. It's not owned by Cirrus,
so it's not our responsibility if you don't like it. Mark Brown is the maintainer.
Sounds very much like you are saying, "it's not Cirrus' fault,
therefore it is not my problem"? Which is a terrible attitude.
I think from the perspective of Charles and Richard this is sounding an
awful lot like you want them (or someone else) to rewrite a very widely
used kernel API before they can get their driver merged. To me that
would be a completely disproportionate amount of effort for their goal
but unfortunately people do get asked to do things like that so it's not
an unreasonable fear for them to have.
I would see that as an unreasonable request.
To be clear, that is *not* what I am asking.
It sounded that way, so I apologize if that wasn't what you meant. It just
read as if you thought we were the owners (or only users) of regmap so we
can just go and change it as per your suggestion and then resend this patch.
Remember that the Linux kernel is an open community. Anyone should be
free to discuss any relevant issue. If we decide to take this
off-line, which is likely, then so be it. In the mean time, as a
contributor to this community project, it's absolutely your
responsibly to help discuss and potentially solve wider issues than
just your lines of code.
It's also a community of people with differing amounts of ability to
contribute, due to things like time, energy and so on. Not everyone can
work on everything they want to, let alone other things people ask them
to look at.
I'm not asking for code submission. Merely contributing to this
discussion, or simply allowing it to happen on the back of one of
their submission is enough.
Denouncing all responsibility and a lack of care is not okay.
As above, if one subsystem owner doesn't like another subsystem then those
subsystem owners should talk to each other and sort something out. It shouldn't
block patches that are just trying to use the subsystem as it currently exists
in the kernel.
Again, no one is blocking this patch.
This driver was submitted for review/discussion. We are discussing.
I kind of said this above but just to be clear this driver seems to me
like an idiomatic user of the regmap API as it is today. My guess is
that we could probably loose the defaults tables and not suffer too much
but equally well they don't hurt from a regmap point of view.
Perhaps Charles could elaborate on whether this is possible or not?
Reviwed-by: Mark Brown <broonie@xxxxxxxxxx>
Thanks.