On Thu, Jul 21, 2022 at 06:26:44PM -0700, Douglas Anderson wrote: > Looking through the current users of regulator_set_load(), all but one > or two basically follow the pattern: > 1. Get all the regulators. > 2. Go through each regulator and call regulator_set_load() on it with > some value that's sufficient to get the regulator to run in "HPM" > mode. > 3. Enable / disable the regulator as needed. Sure... I guess these devices are just hoping that their idle currents are near enough to zero to be safe. Having refreshed my mind about how all this works that's basically the assumption that Linux is making here, that any idle currents will be low enough to be satisified by the lowest power state of any practical regulator and we don't need to keep track of them. > Specifically it should be noted that many devices don't really have a > need for the low power mode of regulators. Unfortunately the current What exactly do you mean by "a need for the low power mode of regulators"? > state of the world means that all drivers are cluttered with tables of > loads and extra cruft at init time to tell the regulator framework > about said loads. We're only talking about a few lines of code here, and we could make a bulk helper to set loads if this is a serious issue. You could even just add some more values in the bulk struct and have it be pure data for the drivers, regulator_bulk_get() could see a load and set it. I think this may actually be the bulk of what's concerning you here? > There are lots of ways we could combine this "base load" with a load > that the driver requests. We could: > - Add them. > - Take the maximum (assume that the "base" is the min). > - Only use the "base" load if the driver didn't request one. This feels a bit mangled, I think largely because you're using the term "base load" to refer to something which I *think* you want to be the peak load the device might cause when it's actively doing stuff which is just very confusing. If this were being specified per device I'd expect that to be the idle rather than active current with a name like that. That's just naming, but it does suggest that if we were to put this in DT it should be more like the pinctrl equivalents and done as an array of currents with matching array of names. > I have chosen the third option here. If the driver knows better then > it can override. Note that the driver can't override to "0". To do > that it would just disable the regulator. I don't like the idea of having platform constraints which we ignore, this goes against the general safety principal of the regulator API which is that we won't touch the device state unless given explicit permission to do so by the platform description. The general idea is to ensure that we can't do anything that might damage the platform without approval from the system integrator. > NOTE: if we want to keep old binary dtb files working with new kernels > then we'd still have to keep existing crufty regulator_set_load() in > drivers, but hopefully we can stop adding new instances of this, and > perhaps eventually people will agree to deprecate old binary dtb files > and we can get rid of all the extra code. To be perfectly honest I'm not sure I see any great advantage here. It's moving the performance numbers from the driver into DT which makes them ABI which makes them harder to update, and the peak load is going to be vulnerable to changes due to driver changes - if we do some performance work and manage to start driving a device harder the numbers specified in the DT may become wrong. In general it seems safer to not put things into ABI if we don't have to, and a substantial proportion of things that might use regulators are off-SoC devices which don't generally want or need DT fragments to describe them so we end up having to duplicate things in multiple DTs which isn't ideal. What's big push to put this in DT rather than just make the code pattern easier to use? I'm not totally against it but I'm having a hard time getting enthusiastic about it. I think the underlying thing here (and the reason you're seeing this a lot with Qualcomm device specifically) is that Qualcomm have a firmware interface to specify loads and wanted to implement selection of a low power mode when things are idle in the way that Linux does with the get_optimum_mode() interface. Either they actually have some systems where they were able to select different modes when things are running or they don't have the thing we have where they discount loads from consumers that have removed their enable vote, either way for those platforms what's going on currently does seem like a sensible way of setting things up. Other platforms don't bother, I think that's likely to be some combination of not caring about anything finer grained than system suspend (which especially for Android is totally fair) and modern regulators being much more able to to dynamically adapt to load than those the code was written for.
Attachment:
signature.asc
Description: PGP signature
