Hi Miquel, On Mon, 27 Jan 2025 at 10:11, Miquel Raynal <[email protected]> wrote: > > Hi Simon, > > On 25/01/2025 at 10:11:22 -07, Simon Glass <[email protected]> wrote: > > > Hi Miquel, > > > > On Fri, 24 Jan 2025 at 08:20, Miquel Raynal <[email protected]> > > 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. > > I don't think the SPL is a problem here? I consider broken a board file > with one main function calling enable() twice on the same domain and > disable() only once. Although I am fine implementing a "force power > off", I still consider this is not a correct approach. > > >> >> > 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? > > ->power_on() and ->power_off() perform a state transition, like you > would expect. But these callbacks are not supposed to be called > directly, they are typically called in the genpd_power_on/off() helpers > (through _genpd_power_on/off()), see below. > > >> >> 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. > > In Linux, we do not return -EALREADY if the refcount of a power domain > indicates it is already on when powering up: > https://elixir.bootlin.com/linux/v6.12.6/source/drivers/pmdomain/core.c#L922 > > However, while we do return -EBUSY when turning it off if there are children > still enabled, > (https://elixir.bootlin.com/linux/v6.12.6/source/drivers/pmdomain/core.c#L848) > this function is not exposed and there is not a single caller that > actually checks the return code. As a matter of fact, disabling the > power domains is mostly done asynchronously anyway in a work queue that > is supposed to "try" disabling the unused domains: > https://elixir.bootlin.com/linux/v6.12.6/source/drivers/pmdomain/core.c#L2890 > So in the end, consumers just do not care about the final status of the > domain, and they are unaware of it. > > Please note that most of the handling of the domains is abstracted > through Runtime PM handling where, again, the return value is never > forwarded to the device drivers. > > >> > 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 :-) > > I am okay having two levels, one returning statuses and the other one > hiding them. After some time if nobody complained nor used the low-level > one, we can certainly merge them. So I would propose something like: > > int power_domain_on_low_level() > { > //refcount handling, power_on() if needed > } > > static inline int power_domain_on(pd) { > int ret = power_domain_on_low_level(pd); > if (ret == -EBUSY || ret == -EALREADY) > ret = 0; > return ret; > } > > Would that work for you?
Yes, that's perfect. Regards, Simon
