Message-ID: <748d6a49-3ee0-45af-bee3-fb40b98f94c4@gmail.com>
Date: Fri, 12 Sep 2025 11:51:28 +0200
From: Matthias Brugger <matthias.bgg@...il.com>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 linux-remoteproc@...r.kernel.org
Cc: arnd@...db.de, andersson@...nel.org, mathieu.poirier@...aro.org,
 wenst@...omium.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org,
 kernel@...labora.com
Subject: Re: [PATCH] remoteproc: mtk_scp: Construct FW path if firmware-name
 not present



On 12/09/2025 10:45, AngeloGioacchino Del Regno wrote:
> Il 12/09/25 09:01, Matthias Brugger ha scritto:
>>
>>
>> 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.
>>
> 
> Ohey!
> 
> No, well, we're not inventing a naming scheme... if you check in linux-firmware
> and in the current devicetrees, you'll see that the path adheres to what I wrote.
> 

Well I'm not able to find any *spc_c* firmware :)
Actually mt8188 has scp.img as the only file.

> As in - all of the single core SCP always had the firmware in path
> mediatek/mtXXXX/scp.img - and the dual core SCP has two firmwares.
> 
> The dual core one is a bit special in that the two cores are *almost* (but not
> fully) independent from each other (not entirely relevant to this discussion tho)
> and can load one firmware per core.
> 
> In short - in upstream, the only naming that we're inventing is the multicore SCP,
> but we're simply keeping the same name for the singlecore ones.
> 
> Even for multicore, I'm not really inventing that out of the blue - MediaTek are
> using that naming in downstream, so I'm just copying that.
> 

Which is no guarantee to be a good way to go ;)

Anyway I think the actual naming scheme just makes us add code for no buy-in. 
For me it would make more sense to fix the firmware naming in linux-firmware 
then "working around" that in kernel code.

> Btw... I really don't want to change the single core FW name to "scp_c0.img"
> because my plan is to get this merged and then cleanup the devicetrees for all
> MTK machines to *remove* the firmware-name property from the SCP node(s).
> 

OK, but that's independent. We could keep symlink in linux-firmware for backward 
compability, if needed (delta linux-firmware maintainer gets mad).

> firmware-name support in this driver is retained only for retrocompatibility
> with old DTs (and perhaps "very special" devices needing "very special" firmwares,
> of which none exist right now and hopefully we'll never see anything like that in
> the future).
> 
>>> 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@....fastmail.com/
>>> Signed-off-by: AngeloGioacchino Del Regno 
>>> <angelogioacchino.delregno@...labora.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.
>>
> 
> Yeah, this one is a leftover that I forgot to cleanup. I fully agree with you.
> 
> Will change that in v2.
> 
>>> +    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?
>>
> 
> It's more than that... (check below)
> 
>>> +        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.
>>
> 
> ....and it's because.... (check below)
> 
>>> +        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,")];
> 

Shouldn't we use strchr(compatible, ',') or similar here?

> ...I'd otherwise anyway have to check here, as this is a pointer to the middle of
> the compatible string, used below to extract "mtXXXX" (mt8195, mt1234 etc) from it.
> 
> Sure I get your point about bindings - but IMO those multi-purpose checks make the
> code robust, and will avoid exposure of random memory locations (and/or produce
> undefined behavior) in the event that the compatible string is shorter than needed.
> 
>>> +
>>> +    return devm_kasprintf(dev, GFP_KERNEL, "mediatek/%.*s/%s.img",
>>> +                  (int)strlen("mtXXXX"), soc, scp_fw_file);

I would have expected that there exists a function to extract a substring, but I 
didn't find any. Anyway, I think instead of hardcode the value we should search 
for '-' or use the remaining string as a whole. That would also fix the issue of 
a too short compatible string.

>>> +}
>>> +
>>>   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.
>>
> 
> Sorry, not a fan of that, I don't see the actual benefit, as in, (imo) it doesn't
> improve readability and it doesn't remove any duplication (as it's called only once
> in one single place).
> 
> But of course, I'm open to understand if I'm missing any point :-)
> 

My point would be to encapsulate the logic how to determine the fw_name in one 
function call. I think it improves readability because you look at the code and 
can say "OK here they somehow determine the fw_name" and only have to look into 
the function if you really care and skip over it otherwise.

Regards,
Matthias

> Cheers,
> Angelo
> 
>>> +        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);
>>
> 
> 

Powered by blists - more mailing lists