| Message ID | 1614838092-30398-4-git-send-email-skomatineni@nvidia.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | Add cpuidle support for Tegra194 | expand |
On Wed, 03 Mar 2021 22:08:10 -0800, Sowjanya Komatineni wrote: > This patch adds cpu-idle-states and corresponding state nodes to > Tegra194 CPU in dt-binding document > > Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> > --- > .../bindings/arm/nvidia,tegra194-ccplex.yaml | 53 ++++++++++++++++++++++ > 1 file changed, 53 insertions(+) > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/arm/nvidia,tegra194-ccplex.example.dt.yaml: cpus: compatible: ['nvidia,tegra194-ccplex'] is not of type 'object' From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/arm/nvidia,tegra194-ccplex.yaml See https://patchwork.ozlabs.org/patch/1447108 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
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.
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. For run-time state transitions, need to provide state request along with its residency time to MCE firmware which is running in the background. State min residency is updated into power_state value along with state id that is passed to psci_cpu_suspend_enter Also states cross-over idle times need to be provided to MCE firmware. 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. Allowing cross-over threshold through DT allows users to vary idle time thresholds for state transitions based on different use-cases.
On 3/8/21 10:32 AM, 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. > > For run-time state transitions, need to provide state request along > with its residency time to MCE firmware which is running in the > background. > > State min residency is updated into power_state value along with state > id that is passed to psci_cpu_suspend_enter > > Also states cross-over idle times need to be provided to MCE firmware. > > 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. > > Allowing cross-over threshold through DT allows users to vary idle > time thresholds for state transitions based on different use-cases. > Hi Sudeep, Can you please let me know if you have any more concerns for having this Tegra specific cpuidle driver? Thanks Sowjanya
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. > 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. > Also states cross-over idle times need to be provided to MCE firmware. > New requirements if this has to be PSCI compliant. > 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. -- Regards, Sudeep
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
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
+Lorenzo Hi Sowjanya, Sorry for the delayed response. I am still in vacation
On Fri, Mar 12, 2021 at 02:27:30PM -0800, 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. > Yes that was my initial understanding, but your previous response was confusing. I should have read this first. > From PSCI specification I only see PSCI_STAT_RESIDENCY and > PSCI_STAT_COUNT optional functions for PSCI1.0/PSCI1.1 > Indeed, we don't have any support to pass run-time residency hints in PSCI today. > Can you please help add right people to explore on possible ways to add > PSCI function for passing corresponding state residency time to ATF? > Before we jump to implementation in TF-A we need to get agreement to get this added to the specification to support in OSPM/Linux. TF-A is just one implementation of PSCI and may not be only one. Formally you can raise any specification related queries to https://developer.arm.com/support or support@arm.com. I will ask the author of PSCI specification internally to take a look at this thread and chime in if they can. > 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 > Yes, I just asked the same question as response to your earlier email. Thanks for confirming that it can be moved out of OSPM/Linux and DT > With both of the above implementation changes Tegra194 driver will be > PSCI compliant. > We still need to get agreement on the specification front
On Mon, Mar 15, 2021 at 11:13:24AM -0700, 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. > Yes, I prefer you need to get this added in the PSCI specification. I have passed this thread to the author of the specification. > 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. > From previous emails, I gather these can be moved to firmware and need not be there in DT ? > As these are Tegra platform specific, Please comment if any other concerns > in having this configured by Tegra CPU Idle kernel driver. > I prefer not to have Tegra specific idle driver if we can get the necessary changes in PSCI spec. We must then have just one PSCI idle driver in the kernel. > Based on my understanding only above issue-1 is PSCI compliant related. > Please confirm. > Correct. -- Regards, Sudeep
On 3/16/21 12:18 AM, Sudeep Holla wrote: > On Mon, Mar 15, 2021 at 11:13:24AM -0700, 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. >> > Yes, I prefer you need to get this added in the PSCI specification. > I have passed this thread to the author of the specification. Thanks Sudeep. > >> 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. >> > From previous emails, I gather these can be moved to firmware and need not be > there in DT ? Yes we can move state thresholds programming from kernel driver to ATF but we still need to use DT properties for this to allow users to tweak for their use-cases. With DT access in ATF this can be done. But checking internally on having DTB access in ATF as currently we don't support that. > >> As these are Tegra platform specific, Please comment if any other concerns >> in having this configured by Tegra CPU Idle kernel driver. >> > I prefer not to have Tegra specific idle driver if we can get the necessary > changes in PSCI spec. We must then have just one PSCI idle driver in the > kernel. Agree by adding state residency run-time propagation along with power state to PSCI specification and moving state thresholds update to MCE from kernel driver to AT looks like we can use same PSCI cpu idle driver although we will be using state thresholds additional DT properties under cpu nodes which will be used by ATF firmware once we decide on no concerns to allow DTB access in ATF. >> Based on my understanding only above issue-1 is PSCI compliant related. >> Please confirm. >> > Correct. > > -- > Regards, > Sudeep
diff --git a/Documentation/devicetree/bindings/arm/nvidia,tegra194-ccplex.yaml b/Documentation/devicetree/bindings/arm/nvidia,tegra194-ccplex.yaml index c9675c4..e1a5005 100644 --- a/Documentation/devicetree/bindings/arm/nvidia,tegra194-ccplex.yaml +++ b/Documentation/devicetree/bindings/arm/nvidia,tegra194-ccplex.yaml @@ -30,6 +30,36 @@ properties: Specifies the bpmp node that needs to be queried to get operating point data for all CPUs. + cluster-deepest-power-state: + $ref: /schemas/types.yaml#/definitions/uint32 + description: CPU cluster deepest power state ID. + +patternProperties: + "^[a-z0-9]+$": + type: object + description: | + CPU core idle state nodes. + Refer to Documentation/devicetree/bindings/arm/idle-states.yaml + + properties: + compatible: + enum: + - nvidia,tegra194-cpuidle-core + + cpu_crossover_thresholds: + type: object + description: CPU idle states crossover threshold time in uSec. + + patternProperties: + "^[a-z0-9]+$": + type: object + + properties: + crossover_c1_c6: + $ref: /schemas/types.yaml#/definitions/uint32 + crossover_cc1_cc6: + $ref: /schemas/types.yaml#/definitions/uint32 + additionalProperties: true examples: @@ -39,12 +69,14 @@ examples: nvidia,bpmp = <&bpmp>; #address-cells = <1>; #size-cells = <0>; + cluster-deepest-power-state = <0x6>; cpu0_0: cpu@0 { compatible = "nvidia,tegra194-carmel"; device_type = "cpu"; reg = <0x0>; enable-method = "psci"; + cpu-idle-states = <&C6>; }; cpu0_1: cpu@1 { @@ -52,6 +84,7 @@ examples: device_type = "cpu"; reg = <0x001>; enable-method = "psci"; + cpu-idle-states = <&C6>; }; cpu1_0: cpu@100 { @@ -59,6 +92,7 @@ examples: device_type = "cpu"; reg = <0x100>; enable-method = "psci"; + cpu-idle-states = <&C6>; }; cpu1_1: cpu@101 { @@ -66,6 +100,25 @@ examples: device_type = "cpu"; reg = <0x101>; enable-method = "psci"; + cpu-idle-states = <&C6>; + }; + + cpu_core_power_states { + C6: c6 { + compatible = "nvidia,tegra194-cpuidle-core"; + idle-state-name = "CPU powergated, state retained"; + wakeup-latency-us = <2000>; + min-residency-us = <30000>; + arm,psci-suspend-param = <0x6>; + status = "okay"; + }; + }; + + cpu_crossover_thresholds { + thresholds { + crossover_c1_c6 = <30000>; + crossover_cc1_cc6 = <80000>; }; + }; }; ...
This patch adds cpu-idle-states and corresponding state nodes to Tegra194 CPU in dt-binding document Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> --- .../bindings/arm/nvidia,tegra194-ccplex.yaml | 53 ++++++++++++++++++++++ 1 file changed, 53 insertions(+)