Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.112.34 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.112.34; helo=mail.nvidia.com;
Subject: Re: [PATCH v1 3/5] dt-bindings: arm: Add cpu-idle-states to Tegra194
 CPU nodes
From:   Sowjanya Komatineni <skomatineni@nvidia.com>
To:     Sudeep Holla <sudeep.holla@arm.com>
CC:     <thierry.reding@gmail.com>, <jonathanh@nvidia.com>,
        <daniel.lezcano@linaro.org>, <robh+dt@kernel.org>,
        <ksitaraman@nvidia.com>, <sanjayc@nvidia.com>,
        <linux-tegra@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-pm@vger.kernel.org>, <devicetree@vger.kernel.org>
References: <1614838092-30398-1-git-send-email-skomatineni@nvidia.com>
 <1614838092-30398-4-git-send-email-skomatineni@nvidia.com>
 <20210308043755.llvdsuz2jwvweovb@bogus>
 <4cebf482-a2f8-5a79-a2f6-4ccd7d31c6ad@nvidia.com>
 <20210311025138.o4ub4j2ss725zpv4@bogus>
 <b31d14ef-81d8-0480-805b-a3cb64404b12@nvidia.com>
 <08ac26c1-8257-4c6d-d274-595fee28a00f@nvidia.com>
 <4b21f4c7-19cd-fcea-dd1b-9203be60a523@nvidia.com>
Message-ID: <ed102a55-d584-3399-f7f4-e4fee378fb81@nvidia.com>
Date:   Mon, 15 Mar 2021 12:26:52 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <4b21f4c7-19cd-fcea-dd1b-9203be60a523@nvidia.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Mar 2021 19:25:57.3041
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: c15e9c3e-b4fd-4b92-f596-08d8e7e828bb
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.112.34];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: BN8NAM11FT050.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN1PR12MB2592
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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