Hi Simon, On 20/01/2025 at 12:21:04 -07, Simon Glass <[email protected]> wrote:
> Hi Miquel, > > On Mon, 20 Jan 2025 at 03:34, Miquel Raynal <[email protected]> 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
