On Thu, 6 Apr 2023 at 11:26, Abel Vesa <abel.vesa@xxxxxxxxxx> wrote: > > On 23-04-05 16:11:18, Ulf Hansson wrote: > > Abel, Saravana, > > > > On Fri, 31 Mar 2023 at 06:59, Abel Vesa <abel.vesa@xxxxxxxxxx> wrote: > > > > > > On 23-03-30 12:50:44, Saravana Kannan wrote: > > > > On Thu, Mar 30, 2023 at 4:27 AM Abel Vesa <abel.vesa@xxxxxxxxxx> wrote: > > > > > > > > > > On 23-03-27 17:17:28, Saravana Kannan wrote: > > > > > > On Mon, Mar 27, 2023 at 12:38 PM Abel Vesa <abel.vesa@xxxxxxxxxx> wrote: > > > > > > > > > > > > > > There have been already a couple of tries to make the genpd "disable > > > > > > > unused" late initcall skip the powering off of domains that might be > > > > > > > needed until later on (i.e. until some consumer probes). The conclusion > > > > > > > was that the provider could return -EBUSY from the power_off callback > > > > > > > until the provider's sync state has been reached. This patch series tries > > > > > > > to provide a proof-of-concept that is working on Qualcomm platforms. > > > > > > > > > > > > I'm giving my thoughts in the cover letter instead of spreading it > > > > > > around all the patches so that there's context between the comments. > > > > > > > > > > > > 1) Why can't all the logic in this patch series be implemented at the > > > > > > framework level? And then allow the drivers to opt into this behavior > > > > > > by setting the sync_state() callback. > > > > > > > > > > > > That way, you can land it only for QC drivers by setting up > > > > > > sync_state() callback only for QC drivers, but actually have the same > > > > > > code function correctly for non-QC drivers too. And then once we have > > > > > > this functionality working properly for QC drivers for one kernel > > > > > > version (or two), we'll just have the framework set the device's > > > > > > driver's sync_state() if it doesn't have one already. > > > > > > > > > > I think Ulf has already NACK'ed that approach here: > > > > > [1] https://lore.kernel.org/lkml/CAPDyKFon35wcQ+5kx3QZb-awN_S_q8y1Sir-G+GoxkCvpN=iiA@xxxxxxxxxxxxxx/ > > > > > > > > I would have NACK'ed that too because that's an incomplete fix. As I > > > > said further below, the fix needs to be at the aggregation level where > > > > you aggregate all the current consumer requests. In there, you need to > > > > add in the "state at boot" input that gets cleared out after a > > > > sync_state() call is received for that power domain. > > > > > > > > > > So, just to make sure I understand your point. You would rather have the > > > genpd_power_off check if 'state at boot' is 'on' and return busy and > > > then clear then, via a generic genpd sync state you would mark 'state at > > > boot' as 'off' and queue up a power off request for each PD from there. > > > And as for 'state at boot' it would check the enable bit through > > > provider. > > > > > > Am I right so far? > > > > I am not sure I completely follow what you are suggesting here. > > Please have a look at this: > https://git.kernel.org/pub/scm/linux/kernel/git/abelvesa/linux.git/commit/?h=qcom/genpd/ignore_unused_until_sync_state&id=4f9e6140dfe77884012383f8ba2140cadb62ca4a > > Keep in mind that is WIP for now. Once I have something, I'll post it on > mailing list. Right now, there is a missing piece mentioned in that > commit message. I had a quick look and it seems rather promising, but I will have a closer look when you post it. > > > > > Although, let me point out that there is no requirement from the genpd > > API point of view, that the provider needs to be a driver. This means > > that the sync_state callback may not even be applicable for all genpd > > providers. > > Yes, I'm considering that case too. Good. > > > > > In other words, it looks to me that we may need some new genpd helper > > functions, no matter what. More importantly, it looks like we need an > > opt-in behaviour, unless we can figure out a common way for genpd to > > understand whether the sync_state thing is going to be applicable or > > not. Maybe Saravana has some ideas around this? > > > > Note that, I don't object to extending genpd to be more clever and to > > share common code, of course. We could, for example, make > > genpd_power_off() to bail out earlier, rather than calling the > > ->power_off() callback and waiting for it to return -EBUSY. Both of > > you have pointed this out to me, in some of the earlier > > replies/discussions too. > > The above link basically does this. I hope this is what Saravana has in > mind as well. Okay, let's see what Saravana thinks. Maybe it's easier to post an RFC, based upon the above and continue the discussion around that? Kind regards Uffe
