Messages in this thread | | | Subject | Re: [PATCH v1 3/5] dt-bindings: arm: Add cpu-idle-states to Tegra194 CPU nodes | From | Sowjanya Komatineni <> | Date | Mon, 15 Mar 2021 12:26:52 -0700 |
| |
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
| |