| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <58c2b614-1a56-413c-977d-42d6b8b8d2b5@ti.com>
Date: Mon, 3 Nov 2025 08:10:04 -0600
From: Andrew Davis <afd@...com>
To: Markus Schneider-Pargmann <msp@...libre.com>, Nishanth Menon <nm@...com>,
Tero Kristo <kristo@...nel.org>, Santosh Shilimkar <ssantosh@...nel.org>
CC: Vishal Mahaveer <vishalm@...com>, Kevin Hilman <khilman@...libre.com>,
Dhruva Gole <d-gole@...com>, Sebin Francis <sebin.francis@...com>, "Kendall
Willis" <k-willis@...com>, Akashdeep Kaur <a-kaur@...com>,
<linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v9 3/3] firmware: ti_sci: Partial-IO support
On 11/3/25 4:31 AM, Markus Schneider-Pargmann wrote:
> On Thu Oct 30, 2025 at 3:24 PM CET, Andrew Davis wrote:
>> On 10/30/25 4:26 AM, Markus Schneider-Pargmann (TI.com) 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 (TI.com) <msp@...libre.com>
>>> ---
>>> drivers/firmware/ti_sci.c | 132 +++++++++++++++++++++++++++++++++++++++++++++-
>>> drivers/firmware/ti_sci.h | 5 ++
>>> 2 files changed, 136 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c
>>> index 4db84a92a517b0aa7bb8d47e809d9848a16e2cc4..f2922fccfbe748a436cb9aa0a8c8e5f48db02ef9 100644
>>> --- a/drivers/firmware/ti_sci.c
>>> +++ b/drivers/firmware/ti_sci.c
>>> @@ -6,6 +6,7 @@
>>> * Nishanth Menon
>>> */
>>>
>>> +#include "linux/dev_printk.h"
>>> #define pr_fmt(fmt) "%s: " fmt, __func__
>>>
>>> #include <linux/bitmap.h>
>>> @@ -3663,6 +3664,116 @@ devm_ti_sci_get_resource(const struct ti_sci_handle *handle, struct device *dev,
>>> }
>>> EXPORT_SYMBOL_GPL(devm_ti_sci_get_resource);
>>>
>>> +/*
>>> + * 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;
>>
>> This whole function is almost identical to ti_sci_cmd_prepare_sleep() other
>> than you use a different mode here, which this different mode can be passed
>> into ti_sci_cmd_prepare_sleep() just the same. Only other difference would
>> be the NORESPONSE flag which you could just check "mode" passed in and
>> use the right flag for the mode.
>
> I thought this to be nicer as it avoided ifs, but as Kendall and you
> both requested this, I changed it for the next version.
>
>>
>>> + req->ctx_lo = 0;
>>> + req->ctx_hi = 0;
>>> + req->debug_flags = 0;
>>> +
>>> + 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;
>>> +}
>>> +
>>> +/*
>>> + * Iterate all device nodes that have a wakeup-source property and check if one
>>> + * of the possible phandles points to a Partial-IO system state. If it
>>> + * does resolve the device node to an actual device and check if wakeup is
>>> + * enabled.
>>> + */
>>> +static bool ti_sci_partial_io_wakeup_enabled(struct ti_sci_info *info)
>>> +{
>>> + struct device_node *wakeup_node = NULL;
>>> +
>>> + for_each_node_with_property(wakeup_node, "wakeup-source") {
>>> + struct of_phandle_iterator it;
>>> + int err;
>>> +
>>> + of_for_each_phandle(&it, err, wakeup_node, "wakeup-source", NULL, 0) {
>>> + struct platform_device *pdev;
>>> + bool may_wakeup;
>>> +
>>> + /*
>>> + * Continue if idle-state-name is not off-wake. Return
>>> + * value is the index of the string which should be 0 if
>>> + * off-wake is present.
>>> + */
>>> + if (of_property_match_string(it.node, "idle-state-name", "off-wake"))
>>> + continue;
>>> +
>>> + pdev = of_find_device_by_node(wakeup_node);
>>> + if (!pdev)
>>> + continue;
>>> +
>>> + may_wakeup = device_may_wakeup(&pdev->dev);
>>> + put_device(&pdev->dev);
>>> +
>>> + if (may_wakeup) {
>>> + dev_dbg(info->dev, "%pOF identified as wakeup source for Partial-IO\n",
>>> + wakeup_node);
>>> + of_node_put(it.node);
>>> + of_node_put(wakeup_node);
>>> + return true;
>>> + }
>>> + }
>>> + }
>>> +
>>> + return false;
>>> +}
>>> +
>>> +static int ti_sci_sys_off_handler(struct sys_off_data *data)
>>> +{
>>> + struct ti_sci_info *info = data->cb_data;
>>> + bool enter_partial_io = ti_sci_partial_io_wakeup_enabled(info);
>>> + int ret;
>>> +
>>> + if (!enter_partial_io)
>>> + return NOTIFY_DONE;
>>> +
>>> + dev_info(info->dev, "Entering Partial-IO because a powered wakeup-enabled device was found.\n");
>>> +
>>> + ret = ti_sci_enter_partial_io(info);
>>> +
>>
>> No need for newline here.
>>
>>> + if (ret) {
>>> + dev_err(info->dev,
>>> + "Failed to enter Partial-IO %pe, trying to do an emergency restart\n",
>>> + ERR_PTR(ret));
>>
>> Why cast this int to a pointer before printing it out?
>
> I am casting this to an error pointer to get the resolution of the error
> value to a symbolic name with %pe. This will print 'EBUSY' etc.
>
Ah I see, but why not use errname() directly? I see dev_err_probe() also goes though
a ERR_PTR() cast, so maybe there is some reason I'm not seeing, so I'm fine either way.
Thanks,
Andrew
> Thanks!
>
> Best
> Markus
Powered by blists - more mailing lists