+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:

quoted

On Mon, Mar 08, 2021 at 10:32:17AM -0800, Sowjanya Komatineni wrote:

quoted

On 3/7/21 8:37 PM, Sudeep Holla wrote:

quoted

On Wed, Mar 03, 2021 at 10:08:10PM -0800, Sowjanya Komatineni wrote:

quoted

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.

quoted

quoted

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.

quoted

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.

quoted

quoted

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 ?

quoted

quoted

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