linux-kernel - Re: [PATCH v4 2/4] firmware: ti

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
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20241220115043.qhjroeqc4gl3pwtz@lcpd911>
Date: Fri, 20 Dec 2024 17:20:43 +0530
From: Dhruva Gole <d-gole@...com>
To: Markus Schneider-Pargmann <msp@...libre.com>
CC: Nishanth Menon <nm@...com>, Tero Kristo <kristo@...nel.org>,
        Santosh
 Shilimkar <ssantosh@...nel.org>,
        Vignesh Raghavendra <vigneshr@...com>, Rob
 Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor
 Dooley <conor+dt@...nel.org>, Anand Gadiyar <gadiyar@...com>,
        <linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        <devicetree@...r.kernel.org>, Vishal Mahaveer <vishalm@...com>,
        Kevin Hilman
	<khilman@...libre.com>
Subject: Re: [PATCH v4 2/4] firmware: ti_sci: Partial-IO support

Hi Markus,

On Dec 19, 2024 at 21:02:13 +0100, Markus Schneider-Pargmann wrote:
> Add support for Partial-IO poweroff. In Partial-IO pins of a few

Maybe add a comma after the In partial-IO, a few pins in this SOC can generate....

> 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 have the
> "poweroff" in the list of wakeup-source power 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@...libre.com>
> ---
>  drivers/firmware/ti_sci.c | 115 +++++++++++++++++++++++++++++++++++++++++++++-
>  drivers/firmware/ti_sci.h |   5 ++
>  2 files changed, 119 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c
> index ec0c54935ac0d667323d98b86ac9d288b73be6aa..693ac816f8ba3941a9156bd39524099ca476d712 100644
> --- a/drivers/firmware/ti_sci.c
> +++ b/drivers/firmware/ti_sci.c
> @@ -3746,6 +3746,100 @@ 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 tisci_enter_partial_io(struct ti_sci_info *info)

Isn't the function naming style in the driver ti_sci_XXX ? You're
missing one `_` I guess

> +{
> +	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");
> +
> +	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;
> +}
> +
> +static bool tisci_canuart_wakeup_enabled(struct ti_sci_info *info)

Add some documentation around this please.

> +{
> +	struct device_node *wakeup_node = NULL;
> +
> +	for (wakeup_node = of_find_node_with_property(NULL, "wakeup-source");
> +	     wakeup_node;
> +	     wakeup_node = of_find_node_with_property(wakeup_node, "wakeup-source")) {
> +		struct platform_device *pdev;
> +		int index;
> +
> +		index = of_property_match_string(wakeup_node, "wakeup-source", "poweroff");
> +		if (index < 0)
> +			continue;

Doesn't the fact that we're inside the for loop already ensure this is > 0?

> +
> +		pdev = of_find_device_by_node(wakeup_node);
> +		if (!pdev)
> +			break;

Same here? Would we otherwise be in the loop?
Just having a quick look here, I could be wrong, please just check once.



-- 
Best regards,
Dhruva Gole
Texas Instruments Incorporated