fix(psci): add optional pwr_domain_validate_suspend to plat_psci_ops_t

This patch adds a new optional member `pwr_domain_validate_suspend` to
the `plat_psci_ops_t` structure that allows a platform to optionally
perform platform specific validations in OS-initiated mode. This is
conditionally compiled into the build depending on the value of the
`PSCI_OS_INIT_MODE` build option.

In https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/17682,
the return type of the `pwr_domain_suspend` handler was updated from
`void` to `int` to allow a platform to optionally perform platform
specific validations in OS-initiated mode. However, when an error code
other than `PSCI_E_SUCCESS` is returned, the current exit path does not
undo the operations in `psci_suspend_to_pwrdown_start`, and as a result,
the system ends up in an unexpected state.

The fix in this patch prevents the need to undo the operations in
`psci_suspend_to_pwrdown_start`, by allowing the platform to first
perform any necessary platform specific validations before the PSCI
generic code proceeds to the point of no return where the CPU_SUSPEND
request is expected to complete successfully.

Change-Id: I05d92c7ea3f5364da09af630d44d78252185db20
Signed-off-by: Wing Li <wingers@google.com>

docs/design_documents/psci_osi_mode.rst

@@ -4,7 +4,7 @@ PSCI OS-initiated mode
 :Author: Maulik Shah & Wing Li
 :Organization: Qualcomm Innovation Center, Inc. & Google LLC
 :Contact: Maulik Shah <quic_mkshah@quicinc.com> & Wing Li <wingers@google.com>
-:Status: RFC
+:Status: Accepted
 
 .. contents:: Table of Contents
 
@@ -367,9 +367,11 @@ To add support for OS-initiated mode, the following changes are proposed:
     ``psci_validate_state_coordination``. If validation fails, propagate the
     error up the call stack.
 
-* Update the return type of the platform specific ``pwr_domain_suspend``
-  handler from ``void`` to ``int``, to allow the platform to optionally perform
-  validations based on hardware states.
+* Add a new optional member ``pwr_domain_validate_suspend`` to
+  ``plat_psci_ops_t`` to allow the platform to optionally perform validations
+  based on hardware states.
+
+* The platform specific ``pwr_domain_suspend`` handler remains unchanged.
 
 .. image:: ../resources/diagrams/psci-osi-mode.png
 

docs/porting-guide.rst

@@ -2818,6 +2818,17 @@ power down state where as it could be either power down, retention or run state
 for the higher power domain levels depending on the result of state
 coordination. The generic code expects the handler to succeed.
 
+plat_psci_ops.pwr_domain_validate_suspend() [optional]
+......................................................
+
+This is an optional function that is only compiled into the build if the build
+option ``PSCI_OS_INIT_MODE`` is enabled.
+
+If implemented, this function allows the platform to perform platform specific
+validations based on hardware states. The generic code expects this function to
+return PSCI_E_SUCCESS on success, or either PSCI_E_DENIED or
+PSCI_E_INVALID_PARAMS as appropriate for any invalid requests.
+
 plat_psci_ops.pwr_domain_suspend_pwrdown_early() [optional]
 ...........................................................
 
@@ -2876,10 +2887,6 @@ allocated in a special area if it cannot fit in the platform's global static
 data, for example in DRAM. The Distributor can then be powered down using an
 implementation-defined sequence.
 
-If the build option ``PSCI_OS_INIT_MODE`` is enabled, the generic code expects
-the platform to return PSCI_E_SUCCESS on success, or either PSCI_E_DENIED or
-PSCI_E_INVALID_PARAMS as appropriate for any invalid requests.
-
 plat_psci_ops.pwr_domain_pwr_down_wfi()
 .......................................
 

include/lib/psci/psci.h

@@ -319,13 +319,13 @@ typedef struct plat_psci_ops {
 	int (*pwr_domain_on)(u_register_t mpidr);
 	void (*pwr_domain_off)(const psci_power_state_t *target_state);
 	int (*pwr_domain_off_early)(const psci_power_state_t *target_state);
+#if PSCI_OS_INIT_MODE
+	int (*pwr_domain_validate_suspend)(
+				const psci_power_state_t *target_state);
+#endif
 	void (*pwr_domain_suspend_pwrdown_early)(
 				const psci_power_state_t *target_state);
-#if PSCI_OS_INIT_MODE
-	int (*pwr_domain_suspend)(const psci_power_state_t *target_state);
-#else
 	void (*pwr_domain_suspend)(const psci_power_state_t *target_state);
-#endif
 	void (*pwr_domain_on_finish)(const psci_power_state_t *target_state);
 	void (*pwr_domain_on_finish_late)(
 				const psci_power_state_t *target_state);

lib/psci/psci_common.c

@@ -451,8 +451,8 @@ void psci_get_target_local_pwr_states(unsigned int end_pwrlvl,
  * enter. This function will be called after coordination of requested power
  * states has been done for each power level.
  *****************************************************************************/
-static void psci_set_target_local_pwr_states(unsigned int end_pwrlvl,
-					const psci_power_state_t *target_state)
+void psci_set_target_local_pwr_states(unsigned int end_pwrlvl,
+				      const psci_power_state_t *target_state)
 {
 	unsigned int parent_idx, lvl;
 	const plat_local_state_t *pd_state = target_state->pwr_domain_state;
@@ -474,7 +474,6 @@ static void psci_set_target_local_pwr_states(unsigned int end_pwrlvl,
 	}
 }
 
-
 /*******************************************************************************
  * PSCI helper function to get the parent nodes corresponding to a cpu_index.
  ******************************************************************************/
@@ -595,9 +594,6 @@ void psci_do_state_coordination(unsigned int end_pwrlvl,
 		state_info->pwr_domain_state[lvl] = PSCI_LOCAL_STATE_RUN;
 
 	}
-
-	/* Update the target state in the power domain nodes */
-	psci_set_target_local_pwr_states(end_pwrlvl, state_info);
 }
 
 #if PSCI_OS_INIT_MODE
@@ -684,9 +680,6 @@ exit:
 		return rc;
 	}
 
-	/* Update the target state in the power domain nodes */
-	psci_set_target_local_pwr_states(end_pwrlvl, state_info);
-
 	return rc;
 }
 #endif

lib/psci/psci_off.c

@@ -104,6 +104,9 @@ int psci_do_cpu_off(unsigned int end_pwrlvl)
 	 */
 	psci_do_state_coordination(end_pwrlvl, &state_info);
 
+	/* Update the target state in the power domain nodes */
+	psci_set_target_local_pwr_states(end_pwrlvl, &state_info);
+
 #if ENABLE_PSCI_STAT
 	/* Update the last cpu for each level till end_pwrlvl */
 	psci_stats_update_pwr_down(end_pwrlvl, &state_info);

lib/psci/psci_private.h

@@ -298,6 +298,8 @@ void psci_restore_req_local_pwr_states(unsigned int cpu_idx,
 #endif
 void psci_get_target_local_pwr_states(unsigned int end_pwrlvl,
 				      psci_power_state_t *target_state);
+void psci_set_target_local_pwr_states(unsigned int end_pwrlvl,
+				      const psci_power_state_t *target_state);
 int psci_validate_entry_point(entry_point_info_t *ep,
 			uintptr_t entrypoint, u_register_t context_id);
 void psci_get_parent_pwr_domain_nodes(unsigned int cpu_idx,

lib/psci/psci_suspend.c

@@ -219,6 +219,19 @@ int psci_cpu_suspend_start(const entry_point_info_t *ep,
 	}
 #endif
 
+#if PSCI_OS_INIT_MODE
+	if (psci_plat_pm_ops->pwr_domain_validate_suspend != NULL) {
+		rc = psci_plat_pm_ops->pwr_domain_validate_suspend(state_info);
+		if (rc != PSCI_E_SUCCESS) {
+			skip_wfi = true;
+			goto exit;
+		}
+	}
+#endif
+
+	/* Update the target state in the power domain nodes */
+	psci_set_target_local_pwr_states(end_pwrlvl, state_info);
+
 #if ENABLE_PSCI_STAT
 	/* Update the last cpu for each level till end_pwrlvl */
 	psci_stats_update_pwr_down(end_pwrlvl, state_info);
@@ -234,15 +247,7 @@ int psci_cpu_suspend_start(const entry_point_info_t *ep,
 	 * program the power controller etc.
 	 */
 
-#if PSCI_OS_INIT_MODE
-	rc = psci_plat_pm_ops->pwr_domain_suspend(state_info);
-	if (rc != PSCI_E_SUCCESS) {
-		skip_wfi = true;
-		goto exit;
-	}
-#else
 	psci_plat_pm_ops->pwr_domain_suspend(state_info);
-#endif
 
 #if ENABLE_PSCI_STAT
 	plat_psci_stat_accounting_start(state_info);