On Tue, Jul 30, 2024 at 07:28:01AM GMT, Nishanth Menon wrote:

On 10:00-20240729, Markus Schneider-Pargmann wrote:

quoted

Add support for Partial-IO poweroff. In Partial-IO pins of a few modules
can generate system wakeups while DDR memory is not powered resulting in
a fresh boot of the system. The modules that can be wakeup sources are
defined by the devicetree.

Only wakeup sources that are actually enabled by the user will be
considered as a an active wakeup source. If none of the wakeup sources
are enabled the system will do a normal poweroff. If at least one wakeup
source is enabled it will instead send a TI_SCI_MSG_PREPARE_SLEEP
message from the sys_off handler. Sending this message will result in an
immediate shutdown of the system. No execution is expected after this
point. The code will enter an infinite loop.

The wakeup source device nodes are gathered during probe. But they are
only resolved to the actual devices in the sys_off handler, if they
exist. If they do not exist, they are ignored.

A short documentation about Partial-IO can be found in section 6.2.4.5
of the TRM at
  https://www.ti.com/lit/pdf/spruiv7

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 drivers/firmware/ti_sci.c | 160 +++++++++++++++++++++++++++++++++-----
 drivers/firmware/ti_sci.h |  34 ++++++++
 2 files changed, 175 insertions(+), 19 deletions(-)

diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c
index 160968301b1f..ba2e56da0215 100644
--- a/drivers/firmware/ti_sci.c
+++ b/drivers/firmware/ti_sci.c

@@ -99,6 +99,9 @@ struct ti_sci_desc {
  * @node:	list head
  * @host_id:	Host ID
  * @users:	Number of users of this instance
+ * @nr_wakeup_sources: Number of device nodes in wakeup_source_nodes
+ * @wakeup_source_nodes: Array of all device_nodes listed as wakeup sources in
+ *			 the devicetree
  */
 struct ti_sci_info {
 	struct device *dev;

@@ -116,6 +119,9 @@ struct ti_sci_info {
 	u8 host_id;
 	/* protected by ti_sci_list_mutex */
 	int users;
+
+	int nr_wakeup_sources;
+	struct device_node **wakeup_source_nodes;
 };
 
 #define cl_to_ti_sci_info(c)	container_of(c, struct ti_sci_info, cl)

@@ -392,10 +398,13 @@ static void ti_sci_put_one_xfer(struct ti_sci_xfers_info *minfo,
 static inline int ti_sci_do_xfer(struct ti_sci_info *info,
 				 struct ti_sci_xfer *xfer)
 {
+	struct ti_sci_msg_hdr *hdr = (struct ti_sci_msg_hdr *)xfer->tx_message.buf;
 	int ret;
 	int timeout;
 	struct device *dev = info->dev;
 	bool done_state = true;
+	bool response_expected = !!(hdr->flags & (TI_SCI_FLAG_REQ_ACK_ON_PROCESSED |
+						  TI_SCI_FLAG_REQ_ACK_ON_RECEIVED));

I think a separate patch to introduce a no_response expected patch would
make sense on which we build tisci_sys_off_handler in the next patch?

quoted

 
 	ret = mbox_send_message(info->chan_tx, &xfer->tx_message);
 	if (ret < 0)

@@ -403,25 +412,27 @@ static inline int ti_sci_do_xfer(struct ti_sci_info *info,
 
 	ret = 0;
 
-	if (system_state <= SYSTEM_RUNNING) {
-		/* And we wait for the response. */
-		timeout = msecs_to_jiffies(info->desc->max_rx_timeout_ms);
-		if (!wait_for_completion_timeout(&xfer->done, timeout))
-			ret = -ETIMEDOUT;
-	} else {
-		/*
-		 * If we are !running, we cannot use wait_for_completion_timeout
-		 * during noirq phase, so we must manually poll the completion.
-		 */
-		ret = read_poll_timeout_atomic(try_wait_for_completion, done_state,
-					       done_state, 1,
-					       info->desc->max_rx_timeout_ms * 1000,
-					       false, &xfer->done);
-	}
+	if (response_expected) {

	How about a goto?

Yes, thanks, looks cleaner.

if (!response_expected)
	goto no_response;

quoted

+		if (system_state <= SYSTEM_RUNNING) {
+			/* And we wait for the response. */
+			timeout = msecs_to_jiffies(info->desc->max_rx_timeout_ms);
+			if (!wait_for_completion_timeout(&xfer->done, timeout))
+				ret = -ETIMEDOUT;
+		} else {
+			/*
+			 * If we are !running, we cannot use wait_for_completion_timeout
+			 * during noirq phase, so we must manually poll the completion.
+			 */
+			ret = read_poll_timeout_atomic(try_wait_for_completion, done_state,
+						       done_state, 1,
+						       info->desc->max_rx_timeout_ms * 1000,
+						       false, &xfer->done);
+		}
 
-	if (ret == -ETIMEDOUT)
-		dev_err(dev, "Mbox timedout in resp(caller: %pS)\n",
-			(void *)_RET_IP_);
+		if (ret == -ETIMEDOUT)
+			dev_err(dev, "Mbox timedout in resp(caller: %pS)\n",
+				(void *)_RET_IP_);
+	}
 

no_response:

quoted

 	/*
 	 * NOTE: we might prefer not to need the mailbox ticker to manage the

@@ -3262,6 +3273,82 @@ static int tisci_reboot_handler(struct sys_off_data *data)
 	return NOTIFY_BAD;
 }
 

[...]

quoted

+static int tisci_sys_off_handler(struct sys_off_data *data)
+{
+	struct ti_sci_info *info = data->cb_data;
+	int i;
+	int ret;
+	bool enter_partial_io = false;
+
+	for (i = 0; i != info->nr_wakeup_sources; ++i) {
+		struct platform_device *pdev =
+			of_find_device_by_node(info->wakeup_source_nodes[i]);
+
+		if (!pdev)
+			continue;
+
+		if (device_may_wakeup(&pdev->dev)) {
+			dev_dbg(info->dev, "%pOFp identified as wakeup source\n",
+				info->wakeup_source_nodes[i]);
+			enter_partial_io = true;
+		}
+	}
+
+	if (!enter_partial_io)
+		return NOTIFY_DONE;
+
+	ret = tisci_enter_partial_io(info);
+
+	if (ret) {
+		dev_err(info->dev,
+			"Failed to enter Partial-IO %pe, trying to do an emergency restart\n",
+			ERR_PTR(ret));
+		emergency_restart();
+	}
+
+	while (1);

Why not fall through OR go through emergency_restart (since there is
no fall through for shutdown path) if it acks, but actually fails to
enter LPM state after a dt described or a default timeout period?

quoted

+
+	return NOTIFY_DONE;
+}
+
 /* Description for K2G */
 static const struct ti_sci_desc ti_sci_pmmc_k2g_desc = {
 	.default_host_id = 2,

@@ -3398,6 +3485,35 @@ static int ti_sci_probe(struct platform_device *pdev)
 		goto out;
 	}
 
+	if (of_property_read_bool(dev->of_node, "ti,partial-io-wakeup-sources")) {

You should probably check on TISCI_MSG_QUERY_FW_CAPS[1] if
Partial IO on low power mode is supported as well? if there is a
mismatch, report so?

I actually have another series in my queue that introduces this check. I
just implemented this check for Partial-IO yesterday in the patch that
introduces fw capabilities. If you like I can switch these series
around.

quoted

+		info->nr_wakeup_sources =
+			of_count_phandle_with_args(dev->of_node,
+						   "ti,partial-io-wakeup-sources",
+						   NULL);
+		info->wakeup_source_nodes =
+			devm_kzalloc(dev, sizeof(*info->wakeup_source_nodes),
+				     GFP_KERNEL);
+
+		for (i = 0; i != info->nr_wakeup_sources; ++i) {
+			struct device_node *devnode =
+				of_parse_phandle(dev->of_node,
+						 "ti,partial-io-wakeup-sources",
+						 i);
+			info->wakeup_source_nodes[i] = devnode;

Curious: Don't we need to maintain reference counting for the devnode
if CONFIG_OF_DYNAMIC?

In case you mean I missed of_node_put(), yes, I did, thank you. I added
it in a ti_sci_remove().

Best
Markus