Hi Miquel, On Tue, 21 Jan 2025 at 01:34, Miquel Raynal <[email protected]> wrote: > > 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).
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
