[PATCH v2 0/2] Add firmware bindings for Q6V5 MSS/PAS

[PATCH v2 0/2] Add firmware bindings for Q6V5 MSS/PAS

[Sibi Sankar]

Q6V5 MSS on certain SoCs like SDM845 are capable of operating under
completely different configuration (like Non-Modem WLAN configuration)
depending on the firmware loaded without any change in boot sequence
of the Hexagon core. The patch series is ultimately aimed to avoid
multiple compatibles per SoC to just specify different upstreamed firmware
locations. This is achieved by using "firmware-name" binding to store
the relative path of mba/modem/pas firmware images.

remoteproc@4080000 { 
	...
	firmware-name = "qcom/sdm845/mss/mba.mbn",
			"qcom/sdm845/mss/modem.mdt";
	...
}

remoteproc@17300000 {
	...
	firmware-name = "qcom/sdm845/lpass/adsp.mdt";
	...
}

Suggested-by: Bjorn Andersson <bjorn.andersson@linaro.org>

v2:
  * Replace "qcom,firmware" with "firmware-name" as suggested
    by Rob
  * Include dt-bindings/parsing logic for PAS based remoteprocs 

Sibi Sankar (2):
  dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5
  remoteproc: qcom: Add support for parsing fw dt bindings

 .../bindings/remoteproc/qcom,adsp.txt         |  6 +++
 .../bindings/remoteproc/qcom,q6v5.txt         |  7 +++
 drivers/remoteproc/qcom_q6v5_mss.c            | 46 +++++++++++++++----
 drivers/remoteproc/qcom_q6v5_pas.c            | 11 ++++-
 4 files changed, 59 insertions(+), 11 deletions(-)

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

[PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5

[Sibi Sankar]

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 <sibis@codeaurora.org>
---
 Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt | 6 ++++++
 Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt | 7 +++++++
 2 files changed, 13 insertions(+)

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt b/Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt
index 9c0cff3a5ed8..60ee0f73071a 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt
@@ -27,6 +27,12 @@ on the Qualcomm ADSP Hexagon core.
 	Value type: <stringlist>
 	Definition: must be "wdog", "fatal", "ready", "handover", "stop-ack"
 
+- firmware-name:
+	Usage: optional
+	Value type: <string>
+	Definition: must list the relative firmware image path for the
+		    Hexagon Core.
+
 - clocks:
 	Usage: required
 	Value type: <prop-encoded-array>
diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
index 9ff5b0309417..3a99e7379d8c 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
@@ -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>
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 2/2] remoteproc: qcom: Add support for parsing fw dt bindings

[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 <sibis@codeaurora.org>
---
 drivers/remoteproc/qcom_q6v5_mss.c | 46 +++++++++++++++++++++++-------
 drivers/remoteproc/qcom_q6v5_pas.c | 11 ++++++-
 2 files changed, 46 insertions(+), 11 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index 01be7314e176..c75179006e24 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -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 */
@@ -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);
+			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);
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index b1e63fcd5fdf..141c7da29e9a 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -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;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5

[Rob Herring]

On Fri, 28 Dec 2018 10:18:18 +0530, Sibi Sankar wrote:
> 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 <sibis@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt | 6 ++++++
>  Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt | 7 +++++++
>  2 files changed, 13 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 2/2] remoteproc: qcom: Add support for parsing fw dt bindings

[Bjorn Andersson]

On Thu 27 Dec 20:48 PST 2018, Sibi Sankar wrote:

> 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 <sibis@codeaurora.org>

Thanks Sibi, just some minor style issues I would prefer to have fixed.

I picked patch 1, so no need to resend that.

> ---
>  drivers/remoteproc/qcom_q6v5_mss.c | 46 +++++++++++++++++++++++-------
>  drivers/remoteproc/qcom_q6v5_pas.c | 11 ++++++-
>  2 files changed, 46 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index 01be7314e176..c75179006e24 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -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);

Use fw_name here.

>  	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;
>  
>  		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);
> +			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);

Follow my lead and break the 80-char limit.

>  				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];

Use the fact that of_property_read_string_index() leaves the output
parameter untouched if it doesn't succeed in parsing the property.

I.e. do:

mba_image = desc->hexagon_mba_image;
ret = of_property_read_string_index(pdev->dev.of_node, "firmware-name",
				    0, &mba_image);
if (ret < 0 && ret != -EINVAL)
	fail();

and

qproc->hexagon_mdt_image = "modem.mdt";
ret = of_property_read_string_index(pdev->dev.of_node, "firmware-name",
				    1, &qproc->hexagon_mdt_image);
if (ret < 0 && ret != -EINVAL)
	fail();

> +
>  	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);
> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index b1e63fcd5fdf..141c7da29e9a 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -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;

As above, please use the fact that the last parameter of
of_property_read_string() doesn't modify the output if it fails.

> +
>  	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;

Regards,
Bjorn

Re: [PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5

[Brian Norris]

Hi Sibi,

On Fri, Dec 28, 2018 at 10:18:18AM +0530, Sibi Sankar wrote:
> 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 <sibis@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt | 6 ++++++
>  Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt | 7 +++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt b/Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt
> index 9c0cff3a5ed8..60ee0f73071a 100644
> --- a/Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt
> @@ -27,6 +27,12 @@ on the Qualcomm ADSP Hexagon core.
>  	Value type: <stringlist>
>  	Definition: must be "wdog", "fatal", "ready", "handover", "stop-ack"
>  
> +- firmware-name:
> +	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.

Brian

> +
>  - clocks:
>  	Usage: required
>  	Value type: <prop-encoded-array>
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> index 9ff5b0309417..3a99e7379d8c 100644
> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> @@ -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>
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH v2 2/2] remoteproc: qcom: Add support for parsing fw dt bindings

[Brian Norris]

On Fri, Dec 28, 2018 at 10:18:19AM +0530, Sibi Sankar wrote:
> 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 <sibis@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_mss.c | 46 +++++++++++++++++++++++-------
>  drivers/remoteproc/qcom_q6v5_pas.c | 11 ++++++-
>  2 files changed, 46 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index 01be7314e176..c75179006e24 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -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 */
> @@ -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

> +			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);
> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index b1e63fcd5fdf..141c7da29e9a 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -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;
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5

[Brian Norris]

On Thu, Jan 03, 2019 at 03:30:14PM -0800, Brian Norris wrote:
> On Fri, Dec 28, 2018 at 10:18:18AM +0530, Sibi Sankar wrote:
> > +- firmware-name:
> > +	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...

Brian

Re: [PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5

[Bjorn Andersson]

On Thu 03 Jan 15:50 PST 2019, Brian Norris wrote:

> On Thu, Jan 03, 2019 at 03:30:14PM -0800, Brian Norris wrote:
> > On Fri, Dec 28, 2018 at 10:18:18AM +0530, Sibi Sankar wrote:
> > > +- firmware-name:
> > > +	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...
> 

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.

Regards,
Bjorn

Re: [PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5

[Brian Norris]

Hi Bjorn,

On Thu, Jan 03, 2019 at 04:01:45PM -0800, Bjorn Andersson wrote:
> On Thu 03 Jan 15:50 PST 2019, Brian Norris wrote:
> 
> > On Thu, Jan 03, 2019 at 03:30:14PM -0800, Brian Norris wrote:
> > > On Fri, Dec 28, 2018 at 10:18:18AM +0530, Sibi Sankar wrote:
> > > > +- firmware-name:
> > > > +	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...
> > 
> 
> 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 ;)

Brian

Re: [PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5

[Brian Norris]

Hi again,

On Thu, Jan 03, 2019 at 04:11:58PM -0800, Brian Norris wrote:
> On Thu, Jan 03, 2019 at 04:01:45PM -0800, Bjorn Andersson wrote:
> > 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 ;)

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

Re: [PATCH v2 2/2] remoteproc: qcom: Add support for parsing fw dt bindings

[Sibi Sankar]

Hi Brian,
Thanks for the review!

On 2019-01-04 05:14, Brian Norris wrote:
> On Fri, Dec 28, 2018 at 10:18:19AM +0530, Sibi Sankar wrote:
>> 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 <sibis@codeaurora.org>
>> ---
>>  drivers/remoteproc/qcom_q6v5_mss.c | 46 
>> +++++++++++++++++++++++-------
>>  drivers/remoteproc/qcom_q6v5_pas.c | 11 ++++++-
>>  2 files changed, 46 insertions(+), 11 deletions(-)
>> 
>> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c 
>> b/drivers/remoteproc/qcom_q6v5_mss.c
>> index 01be7314e176..c75179006e24 100644
>> --- a/drivers/remoteproc/qcom_q6v5_mss.c
>> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
>> @@ -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 */
>> @@ -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

we want to construct the names of the fw blobs
incrementally i.e xxx.xxx to xxx.bxx. Given
that we restrict the fw_name_len to be greater
than 4 at the beginning, I'll probably do
something like this.

/* Replace "xxx.xxx" with "xxx.bxx" */
sprintf(fw_name + fw_name_len - 3, "b%02d", i);

> 
>> +			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);
>> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c 
>> b/drivers/remoteproc/qcom_q6v5_pas.c
>> index b1e63fcd5fdf..141c7da29e9a 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pas.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
>> @@ -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;
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project
>> 

-- 
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5

[Sibi Sankar]

Hi Brian/Bjorn,
Thanks for the review!

On 2019-01-05 07:24, Brian Norris wrote:
> Hi again,
> 
> On Thu, Jan 03, 2019 at 04:11:58PM -0800, Brian Norris wrote:
>> On Thu, Jan 03, 2019 at 04:01:45PM -0800, Bjorn Andersson wrote:
>> > 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 ;)
> 
> 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.

Bjorn,
let me know how you want it implemented.
I am okay with either of the following:
* (variant tag based solution)
or
* (simply going ahead with what we have now).

> 
> Regards,
> Brian

-- 
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5

[Rob Herring]

On Fri, Jan 4, 2019 at 7:54 PM Brian Norris <briannorris@chromium.org> wrote:
>
> Hi again,
>
> On Thu, Jan 03, 2019 at 04:11:58PM -0800, Brian Norris wrote:
> > On Thu, Jan 03, 2019 at 04:01:45PM -0800, Bjorn Andersson wrote:
> > > 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 ;)
>
> 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'.

> 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.

Rob

Re: [PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5

[Brian Norris]

On Tue, Jan 08, 2019 at 09:22:30AM -0600, Rob Herring wrote:
> On Fri, Jan 4, 2019 at 7:54 PM Brian Norris <briannorris@chromium.org> wrote:
> > 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.*"

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.

> 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'.

OK. (I was surprised you accepted paths at all. But if we're going down
that road... *shrug*)

> > 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.

We already have reasons up-thread for why we can't get this exclusively
from compatible.

But if we were to partially construct and/or validate paths using
compatible, then the time to do that is now. As soon as this is merged
without such validation, then we're stuck.

Given the above, it seems like maybe the best we can do is put a
recommendation into a DT schema pattern.

Brian

Re: [PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5

[Rob Herring]

On Wed, Jan 9, 2019 at 3:55 PM Brian Norris <briannorris@chromium.org> wrote:
>
> On Tue, Jan 08, 2019 at 09:22:30AM -0600, Rob Herring wrote:
> > On Fri, Jan 4, 2019 at 7:54 PM Brian Norris <briannorris@chromium.org> wrote:
> > > 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.*"
>
> 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.

Yes, please! Initial support is in 5.0-rc.

I'm not requiring new bindings to be written as schemas just yet
though. We need to ring out any issues with early adopters.

Rob