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

[Miquel Raynal]

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?

Thanks,
Miquèl