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

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Simon,

On 20/01/2025 at 12:21:04 -07, Simon Glass <sjg@chromium.org> wrote:

> Hi Miquel,
>
> On Mon, 20 Jan 2025 at 03:34, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>>
>> Hello Simon,
>>
>> >>  int power_domain_on(struct power_domain *power_domain)
>> >>  {
>>
>> ...
>>
>> >> +       if (priv->on_count++ > 0)
>> >> +               return 0;
>> >
>> > -EALREADY
>>
>> ...
>>
>> >>  int power_domain_off(struct power_domain *power_domain)
>> >>  {
>>
>> ...
>>
>> >> +       if (priv->on_count-- > 1)
>> >> +               return 0;
>> >
>> > -EBUSY
>> >
>> > See how the regulator uclass does it.
>>
>> I really does not understand why we would like to do that.
>>
>> It is perfectly normal operation to call power_domain_on/off() on the
>> same power domain several times in a row and there is no reason to
>> return an error code. It is quite the opposite, the main reason for
>> power domains is to act like shared regulators. Se while a regulator has
>> one user and doing the same action on it several times does not make
>> much sense and can be reported, that is not how power domains have been
>> thought about in the first place.
>
> I am not aware of any difference between these two subsystems.

Actually you're right on this point, the regulator API works like the
pmdomain one. I had a look: it always returns 0, no matter the state of
the regulator after the operation (as long as there is no error of
course).

> 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.

> 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).

> 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.

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?

Thanks,
Miquèl