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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 15/10/25 15:41, Arnd Bergmann ha scritto:
> On Wed, Oct 15, 2025, at 10:41, 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.
>>
>> 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>
> 
> This looks good to me, thanks for the changes!
> 
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> 

Thanks!

>> +
>> +	/* If the compatible string starts with "mediatek,mt" assume that it's ok */
>> +	if (!str_has_prefix(compatible, "mediatek,mt"))
>> +		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);
>> +
>> +	/* Not using strchr here, as strlen of a const gets optimized by compiler */
>> +	soc = &compatible[strlen("mediatek,")];
>> +
>> +	return devm_kasprintf(dev, GFP_KERNEL, "mediatek/%.*s/%s.img",
>> +			      (int)strlen("mtXXXX"), soc, scp_fw_file);
>> +}
> 
> This might have to eventually support version numbers, in case
> there are ever incompatible ABIs where updating the firmware requires
> a minimum kernel driver version.
> 
> Until there is a firmware file that needs it, this code is
> fine.
> 
> I'm not sure if you need to handle the case where there is
> a "firmware-name" property in DT, but the filesystem only
> contains the generated filenames, or when the generated
> firmware name refers to a newer version. It may be best to
> try both the generated names and the name from the dts
> file.

No, not really.

All of the platforms using firmware-name for the SCP are following the same
pattern as path... so this code autogenerates the path based on what it is
set already in DT as firmware-name where present.

The only exception is MT8188, where the firmware was named scp.img regardless
of the fact that it is dual-core and supports dual-firmware... that will need
a rename in linux-firmware. It's necessary evil, but caught just in time ;-)

P.S.: There is no MT8188 DT declaring firmware-name, so the firmware can
really just carelessly be renamed; doing so, no breakage/regression will
happen, and we again don't need to check for existance of both.


Cheers!
Angelo