Add firmware bindings for Q6V5 MSS/PAS

Add optional "firmware-name" bindings for Q6V5 MSS and PAS based remoteprocs. For Q6V5 MSS/PAS the two/one relative firmware paths/path are to be listed respectively. Fallback to the default images for mba/modem for Q6V5 MSS or the default Hexagon image for Q6V5 PAS if the "firmware-name" binding is not present.

Signed-off-by Sibi Sankar <[email protected]>

---
 Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt | 6 ++++++
 Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt | 7 +++++++
 2 files changed, 13 insertions(+)

Add optional "firmware-name" bindings for Q6V5 MSS and PAS based remoteprocs. For Q6V5 MSS/PAS the two/one relative firmware paths/path are to be listed respectively. Fallback to the default images for mba/modem for Q6V5 MSS or the default Hexagon image for Q6V5 PAS if the "firmware-name" binding is not present.

Signed-off-by Sibi Sankar <[email protected]>

---
 Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt | 6 ++++++
 Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt | 7 +++++++
 2 files changed, 13 insertions(+)

Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt +0 -0

index 9c0cff3a5ed8..60ee0f73071a 100644

Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt +1 -0

+
 - clocks:
 	Usage: required
 	Value type: <prop-encoded-array>

Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt +7 -0

index 9ff5b0309417..3a99e7379d8c 100644
@@ -36,6 +36,13 @@ on the Qualcomm Hexagon core.
 	Value type: <stringlist>
 	Definition: must be "wdog", "fatal", "ready", "handover", "stop-ack"
 
+- firmware-name:
+	Usage: optional
+	Value type: <stringlist>
+	Definition: must list the relative firmware image paths for mba and
+		    modem. They are used for booting and authenticating the
+		    Hexagon core.
+
 - clocks:
 	Usage: required
 	Value type: <phandle>

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt on the Qualcomm ADSP Hexagon core. · line 34

Quoted 5 lines · Sibi Sankar

+- firmware-name:

Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt +4 -0

+	Usage: optional
+	Value type: <string>
+	Definition: must list the relative firmware image path for the
+		    Hexagon Core.

Relative to what? I still think it's a terrible idea that your driver looks for files at the top-level /lib/firmware/ directory, but now you're leaking this into the device tree. This should at a bare minimum be namespaced to something like the qcom/ sub-directory. But ideally, the driver would automatically be deriving a further sub-directory of qcom/ based on the chipset or something, and then the only thing you'd describe here is some kind of variant string -- something akin to ath10k's qcom,ath10k-calibration-variant (see Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt), which doesn't require a full path-name or any hierarchy.

Quoted 17 lines · Brian Norris

Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt on the Qualcomm ADSP Hexagon core. · line 34

Quoted 5 lines · Sibi Sankar

+- firmware-name:

Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt +4 -0

+	Usage: optional
+	Value type: <string>
+	Definition: must list the relative firmware image path for the
+		    Hexagon Core.

Relative to what? I still think it's a terrible idea that your driver looks for files at the top-level /lib/firmware/ directory, but now you're leaking this into the device tree. This should at a bare minimum be namespaced to something like the qcom/ sub-directory. But ideally, the driver would automatically be deriving a further sub-directory of qcom/ based on the chipset or something, and then the only thing you'd describe here is some kind of variant string -- something akin to ath10k's qcom,ath10k-calibration-variant (see Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt), which doesn't require a full path-name or any hierarchy.

Oh, I see Rob actually recommended this binding in v1, and it's (sort of) in use by a few other drivers. Is it really expected that we put arbitrary pathnames in device tree? None of the binding documentation seems very specific to me, and their implementations *do* allow arbitrary text. As it stands today, this is a great recipe for name collision -- e.g., how the driver today suggests "modem.XYZ" names; is Qualcomm really the only one out there making modems? :D

So my natural instinct is to avoid this. But if that's what everybody wants...

Quoted 27 lines · Brian Norris

Quoted 17 lines · Brian Norris

Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt on the Qualcomm ADSP Hexagon core. · line 34

Quoted 5 lines · Sibi Sankar

+- firmware-name:

Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt +4 -0

+	Usage: optional
+	Value type: <string>
+	Definition: must list the relative firmware image path for the
+		    Hexagon Core.

Relative to what? I still think it's a terrible idea that your driver looks for files at the top-level /lib/firmware/ directory, but now you're leaking this into the device tree. This should at a bare minimum be namespaced to something like the qcom/ sub-directory. But ideally, the driver would automatically be deriving a further sub-directory of qcom/ based on the chipset or something, and then the only thing you'd describe here is some kind of variant string -- something akin to ath10k's qcom,ath10k-calibration-variant (see Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt), which doesn't require a full path-name or any hierarchy.

Oh, I see Rob actually recommended this binding in v1, and it's (sort of) in use by a few other drivers. Is it really expected that we put arbitrary pathnames in device tree? None of the binding documentation seems very specific to me, and their implementations *do* allow arbitrary text. As it stands today, this is a great recipe for name collision -- e.g., how the driver today suggests "modem.XYZ" names; is Qualcomm really the only one out there making modems? :D

Quoted 3 lines

So my natural instinct is to avoid this. But if that's what everybody wants...

I share your concern about this, but I came to suggest this as the driver cares about platforms but the firmware is (often?) device/product-specific.

E.g. we will serve the MTP and Pixel 3 with the qcom,sdm845-adsp-pas compatible, but they are unlikely to run the same adsp firmware. This allows the individual dtb to specify which firmware the driver should use.

Quoted 8 lines · Bjorn Andersson

I share your concern about this, but I came to suggest this as the driver cares about platforms but the firmware is (often?) device/product-specific.

E.g. we will serve the MTP and Pixel 3 with the qcom,sdm845-adsp-pas compatible, but they are unlikely to run the same adsp firmware. This allows the individual dtb to specify which firmware the driver should use.

I understand this, but that still doesn't mean we should be suggesting each DTB to clutter the top-level firmware search path, especially since lazy people will probably just use "modem.mdt" and similar. That means you no longer can ship the same rootfs that supports both QCOM and <other> modems, if <other> modem also uses the same lazy format.

It seems like a much better practice to at least enforce a particular prefix to things. e.g., the driver could assume:

qcom/sdm845-adsp-pas/ (or if you must, just qcom/)

and your DTB only gets to add .../<your-string-here> to that path.

In case it isn't clear: I think it's also severely misguided that the existing driver gets away with lines like

	request_firmware(&fw, "modem.mdt", ...);

today ;)

Hi again,

Quoted 21 lines · Brian Norris

Quoted 8 lines · Bjorn Andersson

I share your concern about this, but I came to suggest this as the driver cares about platforms but the firmware is (often?) device/product-specific.

E.g. we will serve the MTP and Pixel 3 with the qcom,sdm845-adsp-pas compatible, but they are unlikely to run the same adsp firmware. This allows the individual dtb to specify which firmware the driver should use.

I understand this, but that still doesn't mean we should be suggesting each DTB to clutter the top-level firmware search path, especially since lazy people will probably just use "modem.mdt" and similar. That means you no longer can ship the same rootfs that supports both QCOM and <other> modems, if <other> modem also uses the same lazy format.

It seems like a much better practice to at least enforce a particular prefix to things. e.g., the driver could assume:

qcom/sdm845-adsp-pas/ (or if you must, just qcom/)

Quoted 8 lines

and your DTB only gets to add .../<your-string-here> to that path.

In case it isn't clear: I think it's also severely misguided that the existing driver gets away with lines like

	request_firmware(&fw, "modem.mdt", ...);

today ;)

To add to my thoughts, since I think maybe Sibi was a little unclear of my thoughts:

One of my primary concerns with the existing approach is that it's basically a complete free-for-all. We should have some minimal standards (enforced in code) such that our DTB can never point us at something like /lib/firmware/<other-vendor>/foo.bin (or /lib/firmware/modem.mdt; or lots of other bad examples). This could probably be done simply by always prefixing 'qcom/' (I don't remember -- does request_firmware() follow '..'? e.g., 'firmware-name = "../bar/foo.bin"'.)

As a bonus: it would be very nice if we can provide a little more structure by default, and avoid arbitrary hierarchy in the DTS. That's where I brought up ath10k's "variant" as an example; if we can use 'compatible' to capture most of this particular Hexagon core's properties, then we only leave a single level of variability to the DTS.

But I might be off-base with the "bonus" paragraph. So I'd also be somewhat happy with something much less ambitious, like just a built-in prefix ('qcom/').

And you can also just ignore my thoughts entirely (and I'll be even less happy), since Rob did already provide his Reviewed-by ;) I mostly wanted to give food for thought, in the hopes that something in here would help improve this a bit.

Regards,
Brian

Hi again,

Quoted 21 lines · Brian Norris

Quoted 8 lines · Bjorn Andersson

I share your concern about this, but I came to suggest this as the driver cares about platforms but the firmware is (often?) device/product-specific.

E.g. we will serve the MTP and Pixel 3 with the qcom,sdm845-adsp-pas compatible, but they are unlikely to run the same adsp firmware. This allows the individual dtb to specify which firmware the driver should use.

I understand this, but that still doesn't mean we should be suggesting each DTB to clutter the top-level firmware search path, especially since lazy people will probably just use "modem.mdt" and similar. That means you no longer can ship the same rootfs that supports both QCOM and <other> modems, if <other> modem also uses the same lazy format.

It seems like a much better practice to at least enforce a particular prefix to things. e.g., the driver could assume:

qcom/sdm845-adsp-pas/ (or if you must, just qcom/)

Quoted 8 lines

and your DTB only gets to add .../<your-string-here> to that path.

In case it isn't clear: I think it's also severely misguided that the existing driver gets away with lines like

      request_firmware(&fw, "modem.mdt", ...);

today ;)

To add to my thoughts, since I think maybe Sibi was a little unclear of my thoughts:

One of my primary concerns with the existing approach is that it's basically a complete free-for-all. We should have some minimal standards (enforced in code) such that our DTB can never point us at something like /lib/firmware/<other-vendor>/foo.bin (or /lib/firmware/modem.mdt; or lots of other bad examples). This could probably be done simply by always prefixing 'qcom/' (I don't remember -- does request_firmware() follow '..'? e.g., 'firmware-name = "../bar/foo.bin"'.)

As a bonus: it would be very nice if we can provide a little more structure by default, and avoid arbitrary hierarchy in the DTS. That's where I brought up ath10k's "variant" as an example; if we can use 'compatible' to capture most of this particular Hexagon core's properties, then we only leave a single level of variability to the DTS.

Quoted 10 lines · Brian Norris

To add to my thoughts, since I think maybe Sibi was a little unclear of my thoughts:

One of my primary concerns with the existing approach is that it's basically a complete free-for-all. We should have some minimal standards (enforced in code) such that our DTB can never point us at something like /lib/firmware/<other-vendor>/foo.bin (or /lib/firmware/modem.mdt; or lots of other bad examples). This could probably be done simply by always prefixing 'qcom/' (I don't remember -- does request_firmware() follow '..'? e.g., 'firmware-name = "../bar/foo.bin"'.)

We can write a schema to enforce some of this:

firmware-name:
pattern: "^\w.*"

And you can have a device specific schema to enforce a subdir and/or filename(s).

I tend to think we should not put part of the path in drivers. No real good reason other than we already allow that for other users of 'firmware-name'.

Quoted 5 lines

As a bonus: it would be very nice if we can provide a little more structure by default, and avoid arbitrary hierarchy in the DTS. That's where I brought up ath10k's "variant" as an example; if we can use 'compatible' to capture most of this particular Hexagon core's properties, then we only leave a single level of variability to the DTS.

Some bindings use compatible to determine/construct the firmware name. If you want to restrict things, then that's probably how you should do it IMO.

Quoted 12 lines · Rob Herring

Quoted 10 lines · Brian Norris

To add to my thoughts, since I think maybe Sibi was a little unclear of my thoughts:

One of my primary concerns with the existing approach is that it's basically a complete free-for-all. We should have some minimal standards (enforced in code) such that our DTB can never point us at something like /lib/firmware/<other-vendor>/foo.bin (or /lib/firmware/modem.mdt; or lots of other bad examples). This could probably be done simply by always prefixing 'qcom/' (I don't remember -- does request_firmware() follow '..'? e.g., 'firmware-name = "../bar/foo.bin"'.)

Quoted 4 lines

We can write a schema to enforce some of this:

firmware-name:
pattern: "^\w.*"

Are DT schemas ready to use/enforce? Or would this currently just be a suggestion? I'm out of the loop. But I guess that would be interesting.

Add support for parsing "firmware-name" dt bindings which specifies the relative paths of mba/modem/pas image as strings. Fallback to the default paths for mba/modem/pas image on -EINVAL.

Signed-off-by Sibi Sankar <[email protected]>

---
 drivers/remoteproc/qcom_q6v5_mss.c | 46 +++++++++++++++++++++++-------
 drivers/remoteproc/qcom_q6v5_pas.c | 11 ++++++-
 2 files changed, 46 insertions(+), 11 deletions(-)

drivers/remoteproc/qcom_q6v5_mss.c +3 -1

index 01be7314e176..c75179006e24 100644
@@ -188,6 +188,7 @@ struct q6v5 {
 	bool has_alt_reset;
 	int mpss_perm;
 	int mba_perm;
+	const char *hexagon_mdt_image;
 	int version;
 };
 

@@ -860,17 +861,27 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
 	phys_addr_t min_addr = PHYS_ADDR_MAX;
 	phys_addr_t max_addr = 0;
 	bool relocate = false;
-	char seg_name[10];
+	char *fw_name;
+	size_t fw_name_len;
 	ssize_t offset;
 	size_t size = 0;
 	void *ptr;
 	int ret;
 	int i;
 

drivers/remoteproc/qcom_q6v5_mss.c +3 -2

 	if (ret < 0) {
-		dev_err(qproc->dev, "unable to load modem.mdt\n");
-		return ret;
+		dev_err(qproc->dev, "unable to load %s\n",
+			qproc->hexagon_mdt_image);
+		goto out;
 	}
 
 	/* Initialize the RMB validator */

@@ -918,10 +929,12 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
 		ptr = qproc->mpss_region + offset;
 

drivers/remoteproc/qcom_q6v5_mss.c +7 -2

 				goto release_firmware;
 			}
 

@@ -960,6 +973,8 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
 
 release_firmware:
 	release_firmware(fw);
+out:
+	kfree(fw_name);
 
 	return ret < 0 ? ret : 0;
 }

@@ -1075,9 +1090,10 @@ static int qcom_q6v5_register_dump_segments(struct rproc *rproc,
 	unsigned long i;
 	int ret;
 
-	ret = request_firmware(&fw, "modem.mdt", qproc->dev);
+	ret = request_firmware(&fw, qproc->hexagon_mdt_image, qproc->dev);
 	if (ret < 0) {
-		dev_err(qproc->dev, "unable to load modem.mdt\n");
+		dev_err(qproc->dev, "unable to load %s\n",
+			qproc->hexagon_mdt_image);
 		return ret;
 	}
 

@@ -1253,6 +1269,8 @@ static int q6v5_probe(struct platform_device *pdev)
 	const struct rproc_hexagon_res *desc;
 	struct q6v5 *qproc;
 	struct rproc *rproc;
+	const char *mba_image;
+	const char *fw_name[2];
 	int ret;
 
 	desc = of_device_get_match_data(&pdev->dev);

drivers/remoteproc/qcom_q6v5_mss.c +3 -1

+
 	rproc = rproc_alloc(&pdev->dev, pdev->name, &q6v5_ops,
-			    desc->hexagon_mba_image, sizeof(*qproc));
+			    mba_image, sizeof(*qproc));
 	if (!rproc) {
 		dev_err(&pdev->dev, "failed to allocate rproc\n");
 		return -ENOMEM;

@@ -1272,6 +1297,7 @@ static int q6v5_probe(struct platform_device *pdev)
 	qproc = (struct q6v5 *)rproc->priv;
 	qproc->dev = &pdev->dev;
 	qproc->rproc = rproc;
+	qproc->hexagon_mdt_image = (ret != 2) ? "modem.mdt" : fw_name[1];
 	platform_set_drvdata(pdev, qproc);
 
 	ret = q6v5_init_mem(qproc, pdev);

drivers/remoteproc/qcom_q6v5_pas.c +2 -0

index b1e63fcd5fdf..141c7da29e9a 100644
@@ -258,6 +258,8 @@ static int adsp_probe(struct platform_device *pdev)
 	const struct adsp_data *desc;
 	struct qcom_adsp *adsp;
 	struct rproc *rproc;
+	const char *fw_name;
+	const char *of_fw_name;
 	int ret;
 
 	desc = of_device_get_match_data(&pdev->dev);

Add support for parsing "firmware-name" dt bindings which specifies the relative paths of mba/modem/pas image as strings. Fallback to the default paths for mba/modem/pas image on -EINVAL.

Signed-off-by Sibi Sankar <[email protected]>

---
 drivers/remoteproc/qcom_q6v5_mss.c | 46 +++++++++++++++++++++++-------
 drivers/remoteproc/qcom_q6v5_pas.c | 11 ++++++-
 2 files changed, 46 insertions(+), 11 deletions(-)

drivers/remoteproc/qcom_q6v5_mss.c +15 -4

index 01be7314e176..c75179006e24 100644
@@ -188,6 +188,7 @@ struct q6v5 {
 	bool has_alt_reset;
 	int mpss_perm;
 	int mba_perm;
+	const char *hexagon_mdt_image;
 	int version;
 };
 

@@ -860,17 +861,27 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
 	phys_addr_t min_addr = PHYS_ADDR_MAX;
 	phys_addr_t max_addr = 0;
 	bool relocate = false;
-	char seg_name[10];
+	char *fw_name;
+	size_t fw_name_len;
 	ssize_t offset;
 	size_t size = 0;
 	void *ptr;
 	int ret;
 	int i;
 
-	ret = request_firmware(&fw, "modem.mdt", qproc->dev);
+	fw_name_len = strlen(qproc->hexagon_mdt_image);
+	if (fw_name_len <= 4)
+		return -EINVAL;
+
+	fw_name = kstrdup(qproc->hexagon_mdt_image, GFP_KERNEL);
+	if (!fw_name)
+		return -ENOMEM;
+
+	ret = request_firmware(&fw, qproc->hexagon_mdt_image, qproc->dev);
 	if (ret < 0) {
-		dev_err(qproc->dev, "unable to load modem.mdt\n");
-		return ret;
+		dev_err(qproc->dev, "unable to load %s\n",
+			qproc->hexagon_mdt_image);
+		goto out;
 	}
 
 	/* Initialize the RMB validator */

drivers/remoteproc/qcom_q6v5_mss.c +19 -4

+			ret = request_firmware(&seg_fw, fw_name, qproc->dev);
 			if (ret) {
-				dev_err(qproc->dev, "failed to load %s\n", seg_name);
+				dev_err(qproc->dev, "failed to load %s\n",
+					fw_name);
 				goto release_firmware;
 			}
 

@@ -960,6 +973,8 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
 
 release_firmware:
 	release_firmware(fw);
+out:
+	kfree(fw_name);
 
 	return ret < 0 ? ret : 0;
 }

@@ -1075,9 +1090,10 @@ static int qcom_q6v5_register_dump_segments(struct rproc *rproc,
 	unsigned long i;
 	int ret;
 
-	ret = request_firmware(&fw, "modem.mdt", qproc->dev);
+	ret = request_firmware(&fw, qproc->hexagon_mdt_image, qproc->dev);
 	if (ret < 0) {
-		dev_err(qproc->dev, "unable to load modem.mdt\n");
+		dev_err(qproc->dev, "unable to load %s\n",
+			qproc->hexagon_mdt_image);
 		return ret;
 	}
 

@@ -1253,6 +1269,8 @@ static int q6v5_probe(struct platform_device *pdev)
 	const struct rproc_hexagon_res *desc;
 	struct q6v5 *qproc;
 	struct rproc *rproc;
+	const char *mba_image;
+	const char *fw_name[2];
 	int ret;
 
 	desc = of_device_get_match_data(&pdev->dev);

@@ -1262,8 +1280,15 @@ static int q6v5_probe(struct platform_device *pdev)
 	if (desc->need_mem_protection && !qcom_scm_is_available())
 		return -EPROBE_DEFER;
 
+	ret = of_property_read_string_array(pdev->dev.of_node, "firmware-name",
+					    fw_name, 2);
+	if (ret != -EINVAL && ret != 2)
+		return ret > 0 ? -EINVAL : ret;
+
+	mba_image = (ret != 2) ? desc->hexagon_mba_image : fw_name[0];
+
 	rproc = rproc_alloc(&pdev->dev, pdev->name, &q6v5_ops,
-			    desc->hexagon_mba_image, sizeof(*qproc));
+			    mba_image, sizeof(*qproc));
 	if (!rproc) {
 		dev_err(&pdev->dev, "failed to allocate rproc\n");
 		return -ENOMEM;

@@ -1272,6 +1297,7 @@ static int q6v5_probe(struct platform_device *pdev)
 	qproc = (struct q6v5 *)rproc->priv;
 	qproc->dev = &pdev->dev;
 	qproc->rproc = rproc;
+	qproc->hexagon_mdt_image = (ret != 2) ? "modem.mdt" : fw_name[1];
 	platform_set_drvdata(pdev, qproc);
 
 	ret = q6v5_init_mem(qproc, pdev);

drivers/remoteproc/qcom_q6v5_pas.c +10 -1

index b1e63fcd5fdf..141c7da29e9a 100644
@@ -258,6 +258,8 @@ static int adsp_probe(struct platform_device *pdev)
 	const struct adsp_data *desc;
 	struct qcom_adsp *adsp;
 	struct rproc *rproc;
+	const char *fw_name;
+	const char *of_fw_name;
 	int ret;
 
 	desc = of_device_get_match_data(&pdev->dev);

@@ -267,8 +269,15 @@ static int adsp_probe(struct platform_device *pdev)
 	if (!qcom_scm_is_available())
 		return -EPROBE_DEFER;
 
+	ret = of_property_read_string(pdev->dev.of_node, "firmware-name",
+				      &of_fw_name);
+	if (ret && ret != -EINVAL)
+		return ret;
+
+	fw_name = ret ? desc->firmware_name : of_fw_name;
+
 	rproc = rproc_alloc(&pdev->dev, pdev->name, &adsp_ops,
-			    desc->firmware_name, sizeof(*adsp));
+			    fw_name, sizeof(*adsp));
 	if (!rproc) {
 		dev_err(&pdev->dev, "unable to allocate remoteproc\n");
 		return -ENOMEM;

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Quoted 56 lines · Sibi Sankar

Add support for parsing "firmware-name" dt bindings which specifies the relative paths of mba/modem/pas image as strings. Fallback to the default paths for mba/modem/pas image on -EINVAL.

Signed-off-by Sibi Sankar <[email protected]>

---
 drivers/remoteproc/qcom_q6v5_mss.c | 46 
+++++++++++++++++++++++-------
 drivers/remoteproc/qcom_q6v5_pas.c | 11 ++++++-
 2 files changed, 46 insertions(+), 11 deletions(-)

drivers/remoteproc/qcom_q6v5_mss.c +15 -4

index 01be7314e176..c75179006e24 100644
@@ -188,6 +188,7 @@ struct q6v5 {
 	bool has_alt_reset;
 	int mpss_perm;
 	int mba_perm;
+	const char *hexagon_mdt_image;
 	int version;
 };
 

@@ -860,17 +861,27 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
 	phys_addr_t min_addr = PHYS_ADDR_MAX;
 	phys_addr_t max_addr = 0;
 	bool relocate = false;
-	char seg_name[10];
+	char *fw_name;
+	size_t fw_name_len;
 	ssize_t offset;
 	size_t size = 0;
 	void *ptr;
 	int ret;
 	int i;
 
-	ret = request_firmware(&fw, "modem.mdt", qproc->dev);
+	fw_name_len = strlen(qproc->hexagon_mdt_image);
+	if (fw_name_len <= 4)
+		return -EINVAL;
+
+	fw_name = kstrdup(qproc->hexagon_mdt_image, GFP_KERNEL);
+	if (!fw_name)
+		return -ENOMEM;
+
+	ret = request_firmware(&fw, qproc->hexagon_mdt_image, qproc->dev);
 	if (ret < 0) {
-		dev_err(qproc->dev, "unable to load modem.mdt\n");
-		return ret;
+		dev_err(qproc->dev, "unable to load %s\n",
+			qproc->hexagon_mdt_image);
+		goto out;
 	}
 
 	/* Initialize the RMB validator */

drivers/remoteproc/qcom_q6v5_mss.c static int q6v5_mpss_load(struct q6v5 *qproc) · line 933

Quoted 7 lines

drivers/remoteproc/qcom_q6v5_mss.c +2 -2

@@ -918,10 +929,12 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
 		ptr = qproc->mpss_region + offset;
 
 		if (phdr->p_filesz) {
-			snprintf(seg_name, sizeof(seg_name), "modem.b%02d", i);
-			ret = request_firmware(&seg_fw, seg_name, qproc->dev);
+			snprintf(fw_name + fw_name_len - 3, fw_name_len,
+				 "b%02d", i);

So, you're assuming that 'fw_name' ends in '.XXX' (for some 3-char value of 'XXX')? Seems a bit odd. But if you really want this, it feels like you should enforce this, and either comment on what you're doing or else use a proper computation that makes it clear (e.g., strlen("bin")).

Brian

Quoted 106 lines

drivers/remoteproc/qcom_q6v5_mss.c +10 -3

+			ret = request_firmware(&seg_fw, fw_name, qproc->dev);
 			if (ret) {
-				dev_err(qproc->dev, "failed to load %s\n", seg_name);
+				dev_err(qproc->dev, "failed to load %s\n",
+					fw_name);
 				goto release_firmware;
 			}
 

@@ -960,6 +973,8 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
 
 release_firmware:
 	release_firmware(fw);
+out:
+	kfree(fw_name);
 
 	return ret < 0 ? ret : 0;
 }

@@ -1075,9 +1090,10 @@ static int 
 qcom_q6v5_register_dump_segments(struct rproc *rproc,
 	unsigned long i;
 	int ret;
 
-	ret = request_firmware(&fw, "modem.mdt", qproc->dev);
+	ret = request_firmware(&fw, qproc->hexagon_mdt_image, qproc->dev);
 	if (ret < 0) {
-		dev_err(qproc->dev, "unable to load modem.mdt\n");
+		dev_err(qproc->dev, "unable to load %s\n",
+			qproc->hexagon_mdt_image);
 		return ret;
 	}

@@ -1253,6 +1269,8 @@ static int q6v5_probe(struct platform_device 
 *pdev)
 	const struct rproc_hexagon_res *desc;
 	struct q6v5 *qproc;
 	struct rproc *rproc;
+	const char *mba_image;
+	const char *fw_name[2];
 	int ret;
 

drivers/remoteproc/qcom_q6v5_mss.c +8 -1

 	desc = of_device_get_match_data(&pdev->dev);

@@ -1262,8 +1280,15 @@ static int q6v5_probe(struct platform_device 
 *pdev)
 	if (desc->need_mem_protection && !qcom_scm_is_available())
 		return -EPROBE_DEFER;
 
+	ret = of_property_read_string_array(pdev->dev.of_node, 
 "firmware-name",
+					    fw_name, 2);
+	if (ret != -EINVAL && ret != 2)
+		return ret > 0 ? -EINVAL : ret;
+
+	mba_image = (ret != 2) ? desc->hexagon_mba_image : fw_name[0];
+
 	rproc = rproc_alloc(&pdev->dev, pdev->name, &q6v5_ops,
-			    desc->hexagon_mba_image, sizeof(*qproc));
+			    mba_image, sizeof(*qproc));
 	if (!rproc) {

drivers/remoteproc/qcom_q6v5_mss.c +1 -0

 		dev_err(&pdev->dev, "failed to allocate rproc\n");
 		return -ENOMEM;

@@ -1272,6 +1297,7 @@ static int q6v5_probe(struct platform_device 
 *pdev)
 	qproc = (struct q6v5 *)rproc->priv;
 	qproc->dev = &pdev->dev;
 	qproc->rproc = rproc;
+	qproc->hexagon_mdt_image = (ret != 2) ? "modem.mdt" : fw_name[1];
 	platform_set_drvdata(pdev, qproc);
 

drivers/remoteproc/qcom_q6v5_mss.c +0 -0

 	ret = q6v5_init_mem(qproc, pdev);

drivers/remoteproc/qcom_q6v5_pas.c +2 -0

index b1e63fcd5fdf..141c7da29e9a 100644
@@ -258,6 +258,8 @@ static int adsp_probe(struct platform_device 
 *pdev)
 	const struct adsp_data *desc;
 	struct qcom_adsp *adsp;
 	struct rproc *rproc;
+	const char *fw_name;
+	const char *of_fw_name;
 	int ret;
 

drivers/remoteproc/qcom_q6v5_pas.c +8 -1

 	desc = of_device_get_match_data(&pdev->dev);

@@ -267,8 +269,15 @@ static int adsp_probe(struct platform_device 
 *pdev)
 	if (!qcom_scm_is_available())
 		return -EPROBE_DEFER;
 
+	ret = of_property_read_string(pdev->dev.of_node, "firmware-name",
+				      &of_fw_name);
+	if (ret && ret != -EINVAL)
+		return ret;
+
+	fw_name = ret ? desc->firmware_name : of_fw_name;
+
 	rproc = rproc_alloc(&pdev->dev, pdev->name, &adsp_ops,
-			    desc->firmware_name, sizeof(*adsp));
+			    fw_name, sizeof(*adsp));
 	if (!rproc) {
 		dev_err(&pdev->dev, "unable to allocate remoteproc\n");

return -ENOMEM;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project