Re: [PATCH v7 2/2] firmware: ti_sci: Partial-IO support

["Markus Schneider-Pargmann" <msp@baylibre.com>]

[-- Attachment #1: Type: text/plain, Size: 4405 bytes --]

Hi Kendall,

On Tue Sep 23, 2025 at 7:49 PM CEST, Kendall Willis wrote:
> Hi Markus,
>
> On 11:19-20250812, Markus Schneider-Pargmann wrote:
>> Add support for Partial-IO poweroff. In Partial-IO pins of a few
>> hardware units can generate system wakeups while DDR memory is not
>> powered resulting in a fresh boot of the system. These hardware units in
>> the SoC are always powered so that some logic can detect pin activity.
>> 
>> If the system supports Partial-IO as described in the fw capabilities, a
>> sys_off handler is added. This sys_off handler decides if the poweroff
>> is executed by entering normal poweroff or Partial-IO instead. The
>> decision is made by checking if wakeup is enabled on all devices that
>> may wake up the SoC from Partial-IO.
>> 
>> The possible wakeup devices are found by checking which devices
>> reference a "Partial-IO" system state in the list of wakeup-source
>> system states. Only devices that are actually enabled by the user will
>> be considered as an active wakeup source. If none of the wakeup sources
>> is 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 wait for 5s and do an emergency_restart afterwards
>> if Partial-IO wasn't entered at that point.
>> 
>> 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 | 131 +++++++++++++++++++++++++++++++++++++++++++++-
>>  drivers/firmware/ti_sci.h |   5 ++
>>  2 files changed, 135 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c
>> index c187fb714b3a4c658d0593c844716d4b160e4fa9..fb6f3d7f7413917cf9534ba65f0a9786d1183c71 100644
>> --- a/drivers/firmware/ti_sci.c
>> +++ b/drivers/firmware/ti_sci.c
>> @@ -3750,6 +3750,116 @@ static const struct dev_pm_ops ti_sci_pm_ops = {
>>  #endif
>>  };
>>  
>> +/*
>> + * Enter Partial-IO, which disables everything including DDR with only a small
>> + * logic being active for wakeup.
>> + */
>> +static int ti_sci_enter_partial_io(struct ti_sci_info *info)
>> +{
>> +	struct ti_sci_msg_req_prepare_sleep *req;
>> +	struct ti_sci_xfer *xfer;
>> +	struct device *dev = info->dev;
>> +	int ret = 0;
>> +
>> +	xfer = ti_sci_get_one_xfer(info, TI_SCI_MSG_PREPARE_SLEEP,
>> +				   TI_SCI_FLAG_REQ_GENERIC_NORESPONSE,
>> +				   sizeof(*req), sizeof(struct ti_sci_msg_hdr));
>> +	if (IS_ERR(xfer)) {
>> +		ret = PTR_ERR(xfer);
>> +		dev_err(dev, "Message alloc failed(%d)\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	req = (struct ti_sci_msg_req_prepare_sleep *)xfer->xfer_buf;
>> +	req->mode = TISCI_MSG_VALUE_SLEEP_MODE_PARTIAL_IO;
>> +	req->ctx_lo = 0;
>> +	req->ctx_hi = 0;
>> +	req->debug_flags = 0;
>> +
>> +	dev_info(dev, "Entering Partial-IO because a powered wakeup-enabled device was found.\n");
>> +
>
> IMO this print should be in the ti_sci_sys_off_handler since that is
> where the check is to find the wakeup-enabled device.
>
>> +	ret = ti_sci_do_xfer(info, xfer);
>> +	if (ret) {
>> +		dev_err(dev, "Mbox send fail %d\n", ret);
>> +		goto fail;
>> +	}
>> +
>> +fail:
>> +	ti_sci_put_one_xfer(&info->minfo, xfer);
>> +
>> +	return ret;
>> +}
>> +
>
> Just something to think about: another way to implement this function
> would be to change ti_sci_cmd_prepare_sleep to add a parameter of flags
> for ti_sci_get_one_xfer in order to be able to choose if the
> TI_SCI_FLAG_REQ_GENERIC_NORESPONSE or TI_SCI_FLAG_REQ_ACK_ON_PROCESSED
> is used. This would make it so there's not duplicated code and
> ti_sci_cmd_prepare_sleep could be used to enter partial IO.

Thanks for the suggestion, I just tried your approach and I personally
prefer the code as it is. I needed two additional ifs to switch to a
different rx_message_size and handle or not handle a response. In the
end there is not much common code left. Also the pattern is quite common
between many of the functions doing transfers.

Thanks for your review, I fixed the rest.

Best
Markus

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 289 bytes --]