[PATCH v3 05/13] power-domain: Add refcounting

[Simon Glass]

Hi Miquel,

On Mon, 27 Jan 2025 at 10:11, Miquel Raynal <miquel.raynal at bootlin.com> wrote:
>
> Hi Simon,
>
> On 25/01/2025 at 10:11:22 -07, Simon Glass <sjg at chromium.org> wrote:
>
> > Hi Miquel,
> >
> > On Fri, 24 Jan 2025 at 08:20, Miquel Raynal <miquel.raynal at bootlin.com> wrote:
> >>
> >> Hi Simon,
> >>
> >> >> > If we
> >> >> > want a power domain to actually turn off, how many times do we need to
> >> >> > call power_domain_off()?
> >> >>
> >> >> We do not? It is why I add refcounting, if a power domain has 2
> >> >> consumers, each consumer says whether it needs the power domain to be on
> >> >> or off and the core will handle, but in no case they should force it's
> >> >> state.
> >> >
> >> > This is a bootloader, so we do sometimes need to force something off,
> >> > or on.
> >>
> >> This, I do understand.
> >>
> >> >> > The function silently does nothing in many
> >> >> > cases, so it is not deterministic.
> >> >>
> >> >> It is fully deterministic, as long as consumers call power_on/off
> >> >> evenly (and this is already what we request in U-Boot).
> >> >
> >> > But I foresee people setting up power domains in board code, not
> >> > drivers. My concern is that this is hiding the real state.
> >>
> >> Setting up power domains in board code is drawback. It is legacy
> >> behaviour, people should switch to the device model (ie. using a proper
> >> DT description) and stop messing around with board files. It's been like
> >> 5 years since U-Boot forced people to transition, I wouldn't feel bad if
> >> these boards were no longer behaving like expected (mind there are very
> >> little chances this will break anything, as the kernel is supposed to
> >> re-init these domains anyway).
> >
> > While I agree with you, the message is not fully landing, certainly
> > not in SPL where there are code-size constraints.
>
> I don't think the SPL is a problem here? I consider broken a board file
> with one main function calling enable() twice on the same domain and
> disable() only once. Although I am fine implementing a "force power
> off", I still consider this is not a correct approach.
>
> >> >> > In the case where we *actually*
> >> >> > want to turn the power domain off, we are at a loss as to what code to
> >> >> > write.
> >> >> >
> >> >> >> Hence, I do not agree with returning error codes in these situations,
> >> >> >> they are misleading and they would have to be ignored anyway.
> >> >> >>
> >> >> >
> >> >> > How about creating a power_domain_off_if_allowed() or
> >> >> > power_domain_soft_off/on() or power_domain_req_off (for request)?
> >> >>
> >> >> The power domain logic has a hardware reality in many SoCs but is also a
> >> >> software concept that is shared with Linux. Changing the semantics in
> >> >> U-Boot would be very misleading and if my understanding is correct, this
> >> >> approach would be new.
> >> >
> >> > Yes this is quite a strong argument. Can you point me to the
> >> > equivalent API in Linux? I see pm_domain but not the same as U-Boot
> >>
> >> Yes, pmdomain (former genpd if I get it correctly), how are they
> >> different? From my point of view it is the same. The same devices are
> >> supported. So why would we want the API to be different?
> >
> > But can you please point me to the API? I am seeing struct
> > generic_pm_domain, which is uncommented, so it isn't clear what the
> > power_off() and power_on() functions actually do. Perhaps they work
> > differently from what you think?
>
> ->power_on() and ->power_off() perform a state transition, like you
> would expect. But these callbacks are not supposed to be called
> directly, they are typically called in the genpd_power_on/off() helpers
> (through _genpd_power_on/off()), see below.
>
> >> >> If you really want a way to track the state of the power domain,
> >> >> however, I can maybe add a helper returning its state, ie:
> >> >>
> >> >>          bool power_domain_is_on(domain)
> >> >>
> >> >> Would that work?
> >> >
> >> > Not really. We should have an API which returns -EALREADY or -EBUSY
> >> > based on the ref counts.
>
> In Linux, we do not return -EALREADY if the refcount of a power domain
> indicates it is already on when powering up:
> https://elixir.bootlin.com/linux/v6.12.6/source/drivers/pmdomain/core.c#L922
>
> However, while we do return -EBUSY when turning it off if there are children
> still enabled,
> (https://elixir.bootlin.com/linux/v6.12.6/source/drivers/pmdomain/core.c#L848)
> this function is not exposed and there is not a single caller that
> actually checks the return code. As a matter of fact, disabling the
> power domains is mostly done asynchronously anyway in a work queue that
> is supposed to "try" disabling the unused domains:
> https://elixir.bootlin.com/linux/v6.12.6/source/drivers/pmdomain/core.c#L2890
> So in the end, consumers just do not care about the final status of the
> domain, and they are unaware of it.
>
> Please note that most of the handling of the domains is abstracted
> through Runtime PM handling where, again, the return value is never
> forwarded to the device drivers.
>
> >> > But yes, we can change the naming, so that
> >> > this 'deterministic' ones have a different name, with the current
> >> > names used for your ref-counting one. Then the ref-counting one is a
> >> > thin layer on top of the deterministic one.
> >>
> >> Honestly, I am not convinced, but I will anyway assume we need a way to
> >> force a domain off. There is no need to force a power domain on however,
> >> as after power_domain_on(), the power domain cannot be off (except error
> >> condition of course).
> >>
> >> So I'll add a helper to force power off. It will be called
> >> power_domain_off_force() and be preceded with a comment stating that
> >> this is not the standard approach, to guide people towards the
> >> refcounted helper instead.
> >>
> >> Would this address your request?
> >
> > I believe it would be better to have a 'low-level' function which
> > handles the refcounting and returns  -EALREADY or -EBUSY if it does
> > nothing, with your 'Linux' functions sitting above that. You can put
> > any comment you like on the low-level function.
> >
> > The idea of 'forcing' something just seems odd to me. I imagine people
> > would call that just to be sure :-)
>
> I am okay having two levels, one returning statuses and the other one
> hiding them. After some time if nobody complained nor used the low-level
> one, we can certainly merge them. So I would propose something like:
>
> int power_domain_on_low_level()
> {
>         //refcount handling, power_on() if needed
> }
>
> static inline int power_domain_on(pd) {
>         int ret = power_domain_on_low_level(pd);
>         if (ret == -EBUSY || ret == -EALREADY)
>                 ret = 0;
>         return ret;
> }
>
> Would that work for you?

Yes, that's perfect.

Regards,
Simon