lkml.org 
[lkml]   [2021]   [Mar]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v1 3/5] dt-bindings: arm: Add cpu-idle-states to Tegra194 CPU nodes
From
Date
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

\
 
 \ /
  Last update: 2021-03-15 20:27    [W:0.147 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site