+Lorenzo Hi Sowjanya, Sorry for the delayed response. I am still in vacation 😉 On Thu, Mar 11, 2021 at 01:11:37PM -0800, Sowjanya Komatineni wrote: > > On 3/10/21 6:52 PM, Sudeep Holla wrote: > > On Mon, Mar 08, 2021 at 10:32:17AM -0800, Sowjanya Komatineni wrote: > > > On 3/7/21 8:37 PM, Sudeep Holla wrote: > > > > On Wed, Mar 03, 2021 at 10:08:10PM -0800, Sowjanya Komatineni wrote: > > > > > This patch adds cpu-idle-states and corresponding state nodes to > > > > > Tegra194 CPU in dt-binding document > > > > > > > > > I see that this platform has PSCI support. Can you care to explain why > > > > you need additional DT bindings and driver for PSCI based CPU suspend. > > > > Until the reasons are convincing, consider NACK from my side for this > > > > driver and DT bindings. You should be really using those bindings and > > > > the driver may be with minor changes there. > > > > > > > MCE firmware is in charge of state transition for Tegra194 carmel CPUs. > > > > > Sure, but I assume only TF-A talks to MCE and not any OSPM/Linux kernel. > > No. Tegra194 CPU idle driver works with MCE firmware running in background > so cpuidle kernel driver also talks to MCE firmware directly on state > information. If that is the case I wouldn't term this as PSCI compliant firmware and wouldn't attempt to use PSCI CPU idle driver. Now if we would what to allow non-PSCI idle driver for Arm64 is entirely different question that deserves a separate discussion IMO. > > > > > For run-time state transitions, need to provide state request along with its > > > residency time to MCE firmware which is running in the background. > > > > > Sounds similar to x86 mwait, perhaps we need to extend PSCI if we need > > to make this firmware PSCI compliant or just say it is not and implement > > completely independent implementation. I am not saying that is acceptable > > ATM but I prefer not to mix some implementation to make it look like > > PSCI compliant. > > > > > State min residency is updated into power_state value along with state id > > > that is passed to psci_cpu_suspend_enter > > > > > Sounds like a hack/workaround. I would prefer to standardise that. IIUC > > the power_state is more static and derived from DT. I don't like to > > overload that TBH. Need to check with authors of that binding. > > Passing state idle time to ATF along with state to enter is Tegra specific > as ATF firmware updates idle time to Tegra MCE firmware which will be used > for deciding on state transition along with other information and background > load. > So far we don't have any platform specific PSCI in OSPM and I prefer to keep it that way. > Not sure if this need to be standardized but will try to find alternate way > to update idle time without misusing power-state value. > Sure, we can always review and see if any alternatives are acceptable, but I am bit nervous to tie this as PSCI if it is not strictly spec compliant. > Will discuss on this internally and get back. > Thanks. > > > > > Also states cross-over idle times need to be provided to MCE firmware. > > > > > New requirements if this has to be PSCI compliant. > > Updating cross-over idle times from DT to MCE firmware directly from cpuidle > kernel driver with corresponding MCE ARI commands is again Tegra specific. > So all there are platform specific but static information you need from DT ? If so, what can't it be made part of TF-A and OSPM can avoid interfering with that info completely. My understanding was that OSPM provides runtime hints like x86 mwait. If that's not the case, I am failing to understand the need for OSPM to pass such static information from DT to the firmware. Why can't that be just part of the firmware to begin with ? > > > > > MCE firmware decides on state transition based on these inputs along with > > > its background work load. > > > What do you mean by this *"background work load"* ? -- Regards, Sudeep
