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

[Sowjanya Komatineni <skomatineni@nvidia.com>]

Re-sending as it went out as HTML instead of plain text.


On 3/15/21 11:13 AM, Sowjanya Komatineni wrote:
>
> Hi Sudeep,
>
> I see you are one of the maintainer of PSCI driver. Please add any 
> other right persons if you think should also agree/comment.
>
> Can you please comment on below 2 items based on your feedback?
>
> 1. Can you please suggest on proper way of generalizing to pass state 
> residency time run-time along with state during state enter?
>
> Not sure if any other drivers need this but for Tegra as MCE firmware 
> is in-charge of states enter and decisions, passing run-time state 
> residency from kernel to ATF is required and agree on not using 
> power_state value for this which is against PSCI spec.
>
> 2. Regarding state thresholds, although state thresholds are policy 
> related in Tegra cpu idle perspective these thresholds are platform 
> specific based on use case and mainly for MCE firmware usage to decide 
> on state transitions for core and core clusters as well.
>
> As these are Tegra platform specific, Please comment if any other 
> concerns in having this configured by Tegra CPU Idle kernel driver.
>
> Based on my understanding only above issue-1 is PSCI compliant 
> related. Please confirm.
>
> Thanks
>
> Sowjanya
>
>
> On 3/12/21 2:27 PM, Sowjanya Komatineni wrote:
>>
>> Hi Sudeep,
>>
>> To make our driver PSCI compliant below are few updates to be done
>>
>> 1. Standardize passing residency time run-time thru PSCI to ATF.
>>
>>     From PSCI specification I only see PSCI_STAT_RESIDENCY and 
>> PSCI_STAT_COUNT optional functions for PSCI1.0/PSCI1.1
>>
>>    Can you please help add right people to explore on possible ways 
>> to add PSCI function for passing corresponding state residency time 
>> to ATF?
>>
>> 2. Tegra CPU Idle support definitely need to pass deepest cluster 
>> state and threshold to MCE firmware from DT and looks like we can 
>> move this implementation to ATF
>>
>>      With both of the above implementation changes Tegra194 driver 
>> will be PSCI compliant.
>>
>> Thanks
>>
>> Sowjanya
>>
>>
>> On 3/11/21 1:11 PM, 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.
>>>>
>>>>> 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