On Fri, Dec 06, 2019 at 03:26:16PM +0100, Ulf Hansson wrote: [...] > > You can merge it as it is but that's how things stand and adding > > a comment to the *code* would help understand its logic. > > Okay, how about adding a comment along the lines of this: > > "Using the deepest state for the CPU to trigger a potential selection > of a shared state for the domain, assumes the domain states are all > deeper states". Just this it should be fine (I trimmed it a bit). > > > So, unless I am missing your point, I think the above code does > > > exactly what you want, no? > > > > > > In regards to the "arbitrary choice" of what cpuidle state to use, > > > there are more details about why that is, in the changelog. > > > > > > > > > > > This inizialization though does not belong in here, it is done at driver > > > > level, it should not be done in this per-cpu path. IIUC the logic the > > > > enter pointer should only be overridden if and only if all cpus managed > > > > by the driver have a corresponding device associated. > > > > > > I think you have overlooked the fact that there are one cpuidle driver > > > registered per CPU. The above doesn't make sense to me, sorry. > > > > You are calling psci_dt_cpu_init_idle() for every possibile cpu. > > > > Every time psci_dt_attach_cpu() is called, we check dev and override > > the idle driver enter method. There is one driver, what I am saying > > is that it is not correct to check dev and override the enter pointer > > for *every* cpu that we try to attach to a power domain. This must > > be done once for all by checking that *all* devices could be attached > > to a power domain. > > Ah, now I think get your point. > > You want me to re-iterate through all the registered cpuidle drivers, > which means one per CPU - and then override the enter callback for > each of them, but only if all devices was successfully attached to a > PM domain. Is that correct? > > My only worries with this, is that we have already registered the > cpuidle drivers and I don't think it's a good idea to update the enter > callbacks, beyond that point. > > Perhaps another option is to track whether the first CPU gets attached > (and then update the enter callback), but after that require all the > remaining CPUs to be attached as well - else bail out with an error > code, failing to register all the driver instances. > > What do you think about that? I was confused - now we have one cpuidle driver per cpu so this comment was bogus from this perspective (I was still reasoning wit a *single* cpuidle driver across cpus. Apologies). Sudeep will follow up on this but please forget this specific comment - I was wrong. Thanks, Lorenzo
