On Thu, 23 Jun 2022 at 18:14, Max Krummenacher <max.oss.09@xxxxxxxxx> wrote: > > Hi all > > On Thu, Jun 16, 2022 at 2:51 PM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote: > > > > On Thu, Jun 9, 2022 at 5:10 PM Max Krummenacher <max.oss.09@xxxxxxxxx> wrote: > > > > > This series adds a PM domain provider driver which enables/disables > > > a regulator to control its power state. > > > > Actually, we did this on the U8500 in 2011. > > > > IIRC this led to problems because we had to invent "atomic regulators" > > because regulators use kernel abstractions that assume slowpath > > (process context) and power domains does not, i.e. they execute in > > fastpath, such as an interrupt handler. This isn't entirely correct. The callbacks of a genpd, *may* execute in atomic context, but that depends on whether the GENPD_FLAG_IRQ_SAFE is set for it or not. Similar to what we have for runtime PM callbacks, with pm_runtime_irq_safe(). > > > > The atomic regulator was a subset of regulator that only handled > > regulators that would result in something like an atomic register write. > > > > In the end it was not worth trying to upstream this approach, and > > as I remember it, Ulf Hansson intended to let the power domains poke > > these registers directly, which was easier. (It's on Ulfs TODO list to > > actually implement this, hehe.) Yep, unfortunately I never got to the point. However, poking the registers directly from the genpd provider's on/off callbacks has never been my plan. Instead I would rather expect us to call into a Ux500 specific interface for the prcmu FW. Simply because it's not really a regulator and must not be modelled like it. Instead it is a voltage/frequency domain that is managed behind a FW interface. > > > > Yours, > > Linus Walleij > > Thanks for all the feedback. > > The approach taken with the patchset seems to be architecturally wrong > and as Linus pointed out has additionally the flaw that the regulator > would need to be controllable in atomic context which depending on > the regulator driver / configuration may not be fulfilled. See above. Perhaps GENPD_FLAG_IRQ_SAFE and pm_runtime_irq_safe() can help with this? Note that, power domains can be modelled with child/parents too, which perhaps can help around this too. > > So our plan is to explicitly handle a (shared) regulator in every > driver involved, adding that regulator capability for drivers not > already having one. Please don't! I have recently rejected a similar approach for Tegra platforms, which now have been converted into using the power domain approach. If it's a powerail that is shared between controllers (devices), used to keep their registers values for example, that should be modelled as a power domain. Moreover for power domains, we can support voltage/frequency (performance) scaling, which isn't really applicable to a plain regulator. However, if the actual powerrail fits well to be modelled as regulator, please go ahead. Although, in this case, the regulator must only be controlled behind a genpd provider's on/off callback, so you still need the power domain approach, rather than using the regulator in each driver and for each device. > > > The question which remains is on how one would model functionality > which by itself does not need a driver but would need regulator > support to switch its supply on in run state and off in suspend > states and poweroff, like for example a simple level shifter. Not sure I understand the question properly, but perhaps by adopting my proposal above this problem becomes easier to solve? > Any suggestions on this topic? Thanks. > > Cheers > Max Kind regards Uffe
