Re: [PATCH] remoteproc: mtk_scp: Construct FW path if firmware-name not present

[Matthias Brugger <matthias.bgg@gmail.com>]

On 11/09/2025 16:00, AngeloGioacchino Del Regno wrote:
> After a reply on the mailing lists [1] it emerged that the DT
> property "firmware-name" should not be relied on because of
> possible issues with firmware versions.
> For MediaTek SCP, there has never been any firmware version vs
> driver version desync issue but, regardless, the firmwares are
> always using the same name and they're always located in a path
> with a specific pattern.
> 
> Instead of unconditionally always relying on the firmware-name
> devicetree property to get a path to the SCP FW file, drivers
> should construct a name based on what firmware it knows and
> what hardware it is running on.
> 
> In order to do that, add a `scp_get_default_fw_path()` function
> that constructs the path and filename based on two of the infos
> that the driver can get:
>   1. The compatible string with the highest priority (so, the
>      first one at index 0); and
>   2. The type of SCP HW - single-core or multi-core.
> 
> This means that the default firmware path is generated as:
>   - Single core SCP: mediatek/(soc_model)/scp.img
>     for example:     mediatek/mt8183/scp.img;
> 
>   - Multi core SCP:  mediatek/(soc_model)/scp_c(core_number).img
>     for example:     mediatek/mt8188/scp_c0.img for Core 0, and
>                      mediatek/mt8188/scp_c1.img for Core 1.
> 

As we inventing a naming scheme here: if we decide that signle core FW is calle 
scp_c0.img we can get rid of some code.

> Note that the generated firmware path is being used only if the
> "firmware-name" devicetree property is not present in the SCP
> node or in the SCP Core node(s).
> 
> [1 - Reply regarding firmware-name property]
> Link: https://lore.kernel.org/all/7e8718b0-df78-44a6-a102-89529d6abcce@app.fastmail.com/
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>   drivers/remoteproc/mtk_scp.c | 64 ++++++++++++++++++++++++++++++++----
>   1 file changed, 58 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
> index 8206a1766481..80fcb4b053b3 100644
> --- a/drivers/remoteproc/mtk_scp.c
> +++ b/drivers/remoteproc/mtk_scp.c
> @@ -16,6 +16,7 @@
>   #include <linux/remoteproc.h>
>   #include <linux/remoteproc/mtk_scp.h>
>   #include <linux/rpmsg/mtk_rpmsg.h>
> +#include <linux/string.h>
>   
>   #include "mtk_common.h"
>   #include "remoteproc_internal.h"
> @@ -1093,22 +1094,73 @@ static void scp_remove_rpmsg_subdev(struct mtk_scp *scp)
>   	}
>   }
>   
> +/**
> + * scp_get_default_fw_path() - Get default SCP firmware path
> + * @dev:     SCP Device
> + * @core_id: SCP Core number
> + *
> + * This function generates a path based on the following format:
> + *     mediatek/(soc_model)/scp(_cX).img; for multi-core or
> + *     mediatek/(soc_model)/scp.img for single core SCP HW
> + *
> + * Return: A devm allocated string containing the full path to
> + *         a SCP firmware or an error pointer
> + */
> +static const char *scp_get_default_fw_path(struct device *dev, int core_id)
> +{
> +	struct device_node *np = core_id < 0 ? dev->of_node : dev->parent->of_node;
> +	char scp_fw_file[7] = "scp_cX";

We provide a string that we later overwrite. I'd prefer to have just the 
reservation without any 'artificial' string in it.

> +	const char *compatible, *soc;
> +	int ret;
> +
> +	/* Use only the first compatible string */
> +	ret = of_property_read_string_index(np, "compatible", 0, &compatible);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	/* If the compatible string's length is implausible bail out early */
> +	if (strlen(compatible) < strlen("mediatek,mtXXXX-scp"))

Seems like a double check of compatible. Why is dt-bindings for that not enough?

> +		return ERR_PTR(-EINVAL);
> +
> +	/* If the compatible string starts with "mediatek,mt" assume that it's ok */
> +	if (!str_has_prefix(compatible, "mediatek,mt"))

Same here.

> +		return ERR_PTR(-EINVAL);
> +
> +	if (core_id >= 0)
> +		ret = snprintf(scp_fw_file, ARRAY_SIZE(scp_fw_file), "scp_c%1d", core_id);
> +	else
> +		ret = snprintf(scp_fw_file, ARRAY_SIZE(scp_fw_file), "scp");
> +	if (ret <= 0)
> +		return ERR_PTR(ret);
> +
> +	soc = &compatible[strlen("mediatek,")];
> +
> +	return devm_kasprintf(dev, GFP_KERNEL, "mediatek/%.*s/%s.img",
> +			      (int)strlen("mtXXXX"), soc, scp_fw_file);
> +}
> +
>   static struct mtk_scp *scp_rproc_init(struct platform_device *pdev,
>   				      struct mtk_scp_of_cluster *scp_cluster,
> -				      const struct mtk_scp_of_data *of_data)
> +				      const struct mtk_scp_of_data *of_data,
> +				      int core_id)
>   {
>   	struct device *dev = &pdev->dev;
>   	struct device_node *np = dev->of_node;
>   	struct mtk_scp *scp;
>   	struct rproc *rproc;
>   	struct resource *res;
> -	const char *fw_name = "scp.img";
> +	const char *fw_name;
>   	int ret, i;
>   	const struct mtk_scp_sizes_data *scp_sizes;
>   
>   	ret = rproc_of_parse_firmware(dev, 0, &fw_name);
> -	if (ret < 0 && ret != -EINVAL)
> -		return ERR_PTR(ret);
> +	if (ret) {
> +		fw_name = scp_get_default_fw_path(dev, core_id);

Wouldn't it make more sense to encapsulate the whole fw_name retrival in one 
function, e.g. scp_get_fw_path.

> +		if (IS_ERR(fw_name)) {
> +			dev_err(dev, "Cannot get firmware path: %ld\n", PTR_ERR(fw_name));
> +			return ERR_CAST(fw_name);
> +		}
> +	}
>   
>   	rproc = devm_rproc_alloc(dev, np->name, &scp_ops, fw_name, sizeof(*scp));
>   	if (!rproc) {
> @@ -1212,7 +1264,7 @@ static int scp_add_single_core(struct platform_device *pdev,
>   	struct mtk_scp *scp;
>   	int ret;
>   
> -	scp = scp_rproc_init(pdev, scp_cluster, of_device_get_match_data(dev));
> +	scp = scp_rproc_init(pdev, scp_cluster, of_device_get_match_data(dev), -1);
>   	if (IS_ERR(scp))
>   		return PTR_ERR(scp);
>   
> @@ -1259,7 +1311,7 @@ static int scp_add_multi_core(struct platform_device *pdev,
>   			goto init_fail;
>   		}
>   
> -		scp = scp_rproc_init(cpdev, scp_cluster, cluster_of_data[core_id]);
> +		scp = scp_rproc_init(cpdev, scp_cluster, cluster_of_data[core_id], core_id);
>   		put_device(&cpdev->dev);
>   		if (IS_ERR(scp)) {
>   			ret = PTR_ERR(scp);