Re: [PATCH v1 3/5] dt-bindings: arm: Add cpu-idle-states to Tegra194 CPU nodes

[Sowjanya Komatineni <skomatineni@nvidia.com>]

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.
>
>> 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.

Not sure if this need to be standardized but will try to find alternate 
way to update idle time without misusing power-state value.

Will discuss on this internally and get back.

>
>> 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.

>
>> MCE firmware decides on state transition based on these inputs along with
>> its background work load.
>>
>> So, Tegra specific CPU idle driver is required mainly to provide cross-over
>> thresholds from DT and run time idle state information to MCE firmware
>> through Tegra MCE communication APIs.
>>
> I am worried if different vendors will come up with different custom
> solution for this. We need to either standardise this is Linux/DT or
> in PSCI.
>
>> Allowing cross-over threshold through DT allows users to vary idle time
>> thresholds for state transitions based on different use-cases.
>>
> Sounds like policy and not platform specific to be in DT, but I will leave
> that to DT maintainers.

cross-over idle times are based on supported CPU core and cluster states 
and updating these from DT to Tegra MCE firmware running in the 
background is Tegra specific.

>
> --
> Regards,
> Sudeep