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

[Simon Glass]

Hi Miquel,

On Tue, 21 Jan 2025 at 01:34, Miquel Raynal <miquel.raynal at bootlin.com> wrote:
>
> Hi Simon,
>
> On 20/01/2025 at 12:21:04 -07, Simon Glass <sjg at chromium.org> wrote:
>
> > Hi Miquel,
> >
> > On Mon, 20 Jan 2025 at 03:34, Miquel Raynal <miquel.raynal at 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).

You can see dm_test_power_regulator_set_enable_if_allowed() for a test
that relates to this.

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

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

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

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

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.

Regards,
Simon