Message-ID: <x4y44ajcdi2y2dieaa6oohrptpzyiono3fruvwcdelmtzsh4ne@cgqxsz45ohcy>
Date: Tue, 30 Jul 2024 15:01:13 +0200
From: Markus Schneider-Pargmann <msp@...libre.com>
To: Nishanth Menon <nm@...com>
Cc: Tero Kristo <kristo@...nel.org>, 
	Santosh Shilimkar <ssantosh@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Vignesh Raghavendra <vigneshr@...com>, Vibhore Vardhan <vibhore@...com>, 
	Kevin Hilman <khilman@...libre.com>, Dhruva Gole <d-gole@...com>, 
	linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/6] firmware: ti_sci: Partial-IO support

On Tue, Jul 30, 2024 at 07:28:01AM GMT, Nishanth Menon wrote:
> On 10:00-20240729, Markus Schneider-Pargmann wrote:
> > 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@...libre.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?
> 
> >  
> >  	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;
> > +		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:
> 
> >  	/*
> >  	 * 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;
> >  }
> >  
> [...]
> 
> > +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?
> 
> > +
> > +	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.

> 
> > +		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

Powered by blists - more mailing lists