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

[Simon Glass]

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.

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

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

Regards,
Simon