Re: [PATCH v10 1/4] firmware: ti_sci: Add support for querying the firmware caps

[Nishanth Menon <nm@ti.com>]

On 08:39-20240814, Kevin Hilman wrote:
> From: Georgi Vlaev <g-vlaev@ti.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@ti.com>
> [vibhore@ti.com: Support for LPM_DM_MANAGED mode]
> Signed-off-by: Vibhore Vardhan <vibhore@ti.com>
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> Signed-off-by: Kevin Hilman <khilman@baylibre.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