Re: [PATCH v2 2/5] mfd: lochnagar: Add support for the Cirrus Logic Lochnagar

[Richard Fitzgerald <rf@opensource.cirrus.com>]

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.
> 
>>>>> 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.
> 
>>>>> 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.
> 
>>> 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@kernel.org>
> 
> Thanks.
> 

If we're going to need to change regmap to get drivers that use regmap accepted into
MFD (at least without crippling them), can Lee or Mark please create a separate discussion
thread for that, and include the major contributors/users of regmap so Lee can air his
objections and proposals to a more appropriate group of people and we can get feedback
from the right people, and hopefully come to some sort of decision.