[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240826144600.phsed6horodp3jlg@unhappy>
Date: Mon, 26 Aug 2024 09:46:00 -0500
From: Nishanth Menon <nm@...com>
To: Kevin Hilman <khilman@...libre.com>
CC: Tero Kristo <kristo@...nel.org>, Santosh Shilimkar <ssantosh@...nel.org>,
<linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
Vibhore Vardhan <vibhore@...com>, Dhruva Gole <d-gole@...com>,
Akashdeep Kaur
<a-kaur@...com>,
Markus Schneider-Pargmann <msp@...libre.com>
Subject: Re: [PATCH v10 1/4] firmware: ti_sci: Add support for querying the
firmware caps
On 08:39-20240814, Kevin Hilman wrote:
> From: Georgi Vlaev <g-vlaev@...com>
>
Mostly minor nits below:
> Add support for the TISCI_MSG_QUERY_FW_CAPS message, used to retrieve
> the firmware capabilities of the currently running system firmware. The
> message belongs to the TISCI general core message API [1] and is
> available in SysFW version 08.04.03 and above. Currently, the message is
> supported on devices with split architecture of the system firmware (DM
> + TIFS) like AM62x. Old revisions or not yet supported platforms will
> NACK this request.
>
> We're using this message locally in ti_sci.c to get the low power
> featutes of the FW/SoC. As there's no other kernel consumers yet, this
s/featutes/features/ ?
> is not added to struct ti_sci_core_ops.
>
> Sysfw version >= 10.00.04 support LPM_DM_MANAGED capability [2], where
> Device Mgr firmware now manages which low power mode is chosen. Going
> forward, this is the default configuration supported for TI AM62 family
> of devices. The state chosen by the DM can be influenced by sending
> constraints using the new LPM constraint APIs.
>
> [1] https://software-dl.ti.com/tisci/esd/latest/2_tisci_msgs/general/core.html
> [2] https://downloads.ti.com/tisci/esd/latest/2_tisci_msgs/general/core.html#tisci-msg-query-fw-caps
While both the links are valid,
https://software-dl.ti.com/tisci/esd/latest/index.html has been
used in documentation, so we should stay consistent on the domain name
here.
>
> Signed-off-by: Georgi Vlaev <g-vlaev@...com>
> [vibhore@...com: Support for LPM_DM_MANAGED mode]
> Signed-off-by: Vibhore Vardhan <vibhore@...com>
> Signed-off-by: Markus Schneider-Pargmann <msp@...libre.com>
> Signed-off-by: Kevin Hilman <khilman@...libre.com>
> ---
> drivers/firmware/ti_sci.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> drivers/firmware/ti_sci.h | 22 ++++++++++++++++++++++
[...]
>
> +/**
> + * ti_sci_msg_cmd_query_fw_caps() - Get the FW/SoC capabilities
> + * @handle: Pointer to TI SCI handle
> + * @fw_caps: Each bit in fw_caps indicating one FW/SOC capability
> + *
> + * Return: 0 if all went well, else returns appropriate error value.
> + */
> +static int ti_sci_msg_cmd_query_fw_caps(const struct ti_sci_handle *handle,
> + u64 *fw_caps)
> +{
> + struct ti_sci_info *info;
> + struct ti_sci_xfer *xfer;
> + struct ti_sci_msg_resp_query_fw_caps *resp;
> + struct device *dev;
> + int ret = 0;
> +
> + if (IS_ERR(handle))
> + return PTR_ERR(handle);
> + if (!handle)
> + return -EINVAL;
> +
> + info = handle_to_ti_sci_info(handle);
> + dev = info->dev;
> +
> + xfer = ti_sci_get_one_xfer(info, TI_SCI_MSG_QUERY_FW_CAPS,
> + TI_SCI_FLAG_REQ_ACK_ON_PROCESSED,
> + sizeof(struct ti_sci_msg_hdr),
> + sizeof(*resp));
> + if (IS_ERR(xfer)) {
> + ret = PTR_ERR(xfer);
> + dev_err(dev, "Message alloc failed(%d)\n", ret);
> + return ret;
> + }
> +
> + ret = ti_sci_do_xfer(info, xfer);
> + if (ret) {
> + dev_err(dev, "Mbox send fail %d\n", ret);
> + goto fail;
> + }
> +
> + resp = (struct ti_sci_msg_resp_query_fw_caps *)xfer->xfer_buf;
> +
> + if (!ti_sci_is_response_ack(resp)) {
Add a dev_err here and indicating failure to detect caps?
> + ret = -ENODEV;
> + goto fail;
> + }
> +
> + if (fw_caps)
> + *fw_caps = resp->fw_caps;
> +
> +fail:
> + ti_sci_put_one_xfer(&info->minfo, xfer);
> +
> + return ret;
> +}
> +
> static int ti_sci_cmd_core_reboot(const struct ti_sci_handle *handle)
> {
> struct ti_sci_info *info;
> @@ -3390,6 +3449,14 @@ static int ti_sci_probe(struct platform_device *pdev)
> goto out;
> }
>
> + /*
> + * Check if the firmware supports any optional low power modes.
> + * Old revisions of TIFS (< 08.04) will NACK the request.
Move this comment as part of the function documentation
> + */
> + ret = ti_sci_msg_cmd_query_fw_caps(&info->handle, &info->fw_caps);
> + if (ret || !(info->fw_caps & MSG_FLAG_CAPS_GENERIC))
ret can be set for various reasons including older firmware that may not
support fw_caps - just checking against fw_caps might suffice for the
dev_debug ?
> + pr_debug("Unable to detect LPM capabilities in fw_caps\n");
Please use dev_dbg
Just 2 cents: adding a dev_dbg with details on what matches we got
might be helpful for products in the field?
[...]
--
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
Powered by blists - more mailing lists