Update pil driver with SMMU mapping for allowing authorised
memory access to ADSP firmware, by reading required memory
regions either from device tree file or from resource table
embedded in ADSP binary header.
Signed-off-by: Srinivasa Rao Mandadapu <[email protected]>
---
Changes since V4:
-- Split the code and add appropriate APIs for resource allocation and free.
-- Update adsp_unmap_smmu with missing free ops call.
-- Update normalizing length value in adsp_of_unmap_smmu.
Changes since V3:
-- Rename is_adsp_sb_needed to adsp_sandbox_needed.
-- Add smmu unmapping in error case and in adsp stop.
Changes since V2:
-- Replace platform_bus_type with adsp->dev->bus.
-- Use API of_parse_phandle_with_args() instead of of_parse_phandle_with_fixed_args().
-- Replace adsp->is_wpss with adsp->is_adsp.
-- Update error handling in adsp_start().
drivers/remoteproc/qcom_q6v5_adsp.c | 200 +++++++++++++++++++++++++++++++++++-
1 file changed, 198 insertions(+), 2 deletions(-)
diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c
index b0a63a0..d01c97e 100644
--- a/drivers/remoteproc/qcom_q6v5_adsp.c
+++ b/drivers/remoteproc/qcom_q6v5_adsp.c
@@ -9,6 +9,7 @@
#include <linux/firmware.h>
#include <linux/interrupt.h>
#include <linux/io.h>
+#include <linux/iommu.h>
#include <linux/iopoll.h>
#include <linux/kernel.h>
#include <linux/mfd/syscon.h>
@@ -48,6 +49,8 @@
#define LPASS_PWR_ON_REG 0x10
#define LPASS_HALTREQ_REG 0x0
+#define SID_MASK_DEFAULT 0xF
+
#define QDSP6SS_XO_CBCR 0x38
#define QDSP6SS_CORE_CBCR 0x20
#define QDSP6SS_SLEEP_CBCR 0x3c
@@ -78,7 +81,7 @@ struct adsp_pil_data {
struct qcom_adsp {
struct device *dev;
struct rproc *rproc;
-
+ struct iommu_domain *iommu_dom;
struct qcom_q6v5 q6v5;
struct clk *xo;
@@ -333,6 +336,185 @@ static int adsp_load(struct rproc *rproc, const struct firmware *fw)
return 0;
}
+static void adsp_of_unmap_smmu(struct iommu_domain *iommu_dom, const __be32 *prop, int len)
+{
+ unsigned long iova;
+ unsigned int mem_size;
+ int i;
+
+ len /= sizeof(__be32);
+ for (i = 0; i < len; i++) {
+ iova = be32_to_cpu(prop[i++]);
+ /* Skip Physical address*/
+ i++;
+ mem_size = be32_to_cpu(prop[i++]);
+ iommu_unmap(iommu_dom, iova, mem_size);
+ }
+}
+
+static void adsp_rproc_unmap_smmu(struct rproc *rproc, int len)
+{
+ struct fw_rsc_devmem *rsc_fw;
+ struct fw_rsc_hdr *hdr;
+ int offset;
+ int i;
+
+ for (i = 0; i < len; i++) {
+ offset = rproc->table_ptr->offset[i];
+ hdr = (void *)rproc->table_ptr + offset;
+ rsc_fw = (struct fw_rsc_devmem *)hdr + sizeof(*hdr);
+
+ iommu_unmap(rproc->domain, rsc_fw->da, rsc_fw->len);
+ }
+}
+
+static void adsp_unmap_smmu(struct rproc *rproc)
+{
+ struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
+ const __be32 *prop;
+ unsigned int len;
+
+ iommu_unmap(adsp->iommu_dom, adsp->mem_phys, adsp->mem_size);
+
+ prop = of_get_property(adsp->dev->of_node, "qcom,adsp-memory-regions", &len);
+ if (prop) {
+ adsp_of_unmap_smmu(adsp->iommu_dom, prop, len);
+ } else {
+ if (rproc->table_ptr)
+ adsp_rproc_unmap_smmu(rproc, rproc->table_ptr->num);
+ }
+
+ iommu_domain_free(adsp->iommu_dom);
+}
+
+static int adsp_of_map_smmu(struct iommu_domain *iommu_dom, const __be32 *prop, int len)
+{
+ unsigned long mem_phys;
+ unsigned long iova;
+ unsigned int mem_size;
+ unsigned int flag;
+ int access_level;
+ int ret;
+ int i;
+
+ len /= sizeof(__be32);
+ for (i = 0; i < len; i++) {
+ iova = be32_to_cpu(prop[i++]);
+ mem_phys = be32_to_cpu(prop[i++]);
+ mem_size = be32_to_cpu(prop[i++]);
+ access_level = be32_to_cpu(prop[i]);
+
+ if (access_level)
+ flag = IOMMU_READ | IOMMU_WRITE;
+ else
+ flag = IOMMU_READ;
+
+ ret = iommu_map(iommu_dom, iova, mem_phys, mem_size, flag);
+ if (ret) {
+ pr_err("failed to map addr = %p mem_size = %x\n", &(mem_phys), mem_size);
+ goto of_smmu_unmap;
+ }
+ }
+ return 0;
+of_smmu_unmap:
+ adsp_of_unmap_smmu(iommu_dom, prop, i);
+ return ret;
+}
+
+static int adsp_rproc_map_smmu(struct rproc *rproc, int len)
+{
+ struct fw_rsc_devmem *rsc_fw;
+ struct fw_rsc_hdr *hdr;
+ int offset;
+ int ret;
+ int i;
+
+ if (!rproc->table_ptr)
+ return 0;
+
+ for (i = 0; i < rproc->table_ptr->num; i++) {
+ offset = rproc->table_ptr->offset[i];
+ hdr = (void *)rproc->table_ptr + offset;
+ rsc_fw = (struct fw_rsc_devmem *)hdr + sizeof(*hdr);
+
+ ret = iommu_map(rproc->domain, rsc_fw->da, rsc_fw->pa,
+ rsc_fw->len, rsc_fw->flags);
+ if (ret) {
+ pr_err("failed to map addr = %x mem_size = %x\n", rsc_fw->pa, rsc_fw->len);
+ goto rproc_smmu_unmap;
+ }
+ }
+
+ return 0;
+
+rproc_smmu_unmap:
+ adsp_rproc_unmap_smmu(rproc, i);
+ return ret;
+}
+
+static int adsp_map_smmu(struct qcom_adsp *adsp, struct rproc *rproc)
+{
+ struct of_phandle_args args;
+ const __be32 *prop;
+ long long sid;
+ unsigned int len;
+ int ret;
+
+ ret = of_parse_phandle_with_args(adsp->dev->of_node, "iommus", "#iommu-cells", 0, &args);
+ if (ret < 0)
+ sid = -1;
+ else
+ sid = args.args[0] & SID_MASK_DEFAULT;
+
+ adsp->iommu_dom = iommu_domain_alloc(adsp->dev->bus);
+ if (!adsp->iommu_dom) {
+ dev_err(adsp->dev, "failed to allocate iommu domain\n");
+ ret = -ENOMEM;
+ goto domain_free;
+ }
+
+ ret = iommu_attach_device(adsp->iommu_dom, adsp->dev);
+ if (ret) {
+ dev_err(adsp->dev, "could not attach device ret = %d\n", ret);
+ ret = -EBUSY;
+ goto detach_device;
+ }
+
+ /* Add SID configuration for ADSP Firmware to SMMU */
+ adsp->mem_phys = adsp->mem_phys | (sid << 32);
+
+ ret = iommu_map(adsp->iommu_dom, adsp->mem_phys, adsp->mem_phys,
+ adsp->mem_size, IOMMU_READ | IOMMU_WRITE);
+ if (ret) {
+ dev_err(adsp->dev, "Unable to map ADSP Physical Memory\n");
+ goto sid_unmap;
+ }
+
+ prop = of_get_property(adsp->dev->of_node, "qcom,adsp-memory-regions", &len);
+ if (prop) {
+ ret = adsp_of_map_smmu(adsp->iommu_dom, prop, len);
+ if (ret) {
+ dev_err(adsp->dev, "Unable to map memory regions accessed by ADSP\n");
+ goto sid_unmap;
+ }
+ } else {
+ ret = adsp_rproc_map_smmu(rproc, len);
+ if (ret) {
+ dev_err(adsp->dev, "Unable to map memory regions accessed by ADSP\n");
+ goto sid_unmap;
+ }
+ }
+ return 0;
+
+sid_unmap:
+ iommu_unmap(adsp->iommu_dom, adsp->mem_phys, adsp->mem_size);
+detach_device:
+ iommu_domain_free(adsp->iommu_dom);
+domain_free:
+ return ret;
+}
+
+
static int adsp_start(struct rproc *rproc)
{
struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
@@ -343,9 +525,16 @@ static int adsp_start(struct rproc *rproc)
if (ret)
return ret;
+ if (adsp->adsp_sandbox_needed) {
+ ret = adsp_map_smmu(adsp, rproc);
+ if (ret) {
+ dev_err(adsp->dev, "ADSP smmu mapping failed\n");
+ goto disable_irqs;
+ }
+ }
ret = clk_prepare_enable(adsp->xo);
if (ret)
- goto disable_irqs;
+ goto adsp_smmu_unmap;
ret = qcom_rproc_pds_enable(adsp, adsp->proxy_pds,
adsp->proxy_pd_count);
@@ -401,6 +590,9 @@ static int adsp_start(struct rproc *rproc)
qcom_rproc_pds_disable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
disable_xo_clk:
clk_disable_unprepare(adsp->xo);
+adsp_smmu_unmap:
+ if (adsp->adsp_sandbox_needed)
+ adsp_unmap_smmu(rproc);
disable_irqs:
qcom_q6v5_unprepare(&adsp->q6v5);
@@ -429,6 +621,9 @@ static int adsp_stop(struct rproc *rproc)
if (ret)
dev_err(adsp->dev, "failed to shutdown: %d\n", ret);
+ if (adsp->adsp_sandbox_needed)
+ adsp_unmap_smmu(rproc);
+
handover = qcom_q6v5_unprepare(&adsp->q6v5);
if (handover)
qcom_adsp_pil_handover(&adsp->q6v5);
@@ -460,6 +655,7 @@ static const struct rproc_ops adsp_ops = {
.stop = adsp_stop,
.da_to_va = adsp_da_to_va,
.parse_fw = qcom_register_dump_segments,
+ .find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
.load = adsp_load,
.panic = adsp_panic,
};
--
2.7.4
Quoting Srinivasa Rao Mandadapu (2022-08-22 01:22:02)
> diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c
> index b0a63a0..d01c97e 100644
> --- a/drivers/remoteproc/qcom_q6v5_adsp.c
> +++ b/drivers/remoteproc/qcom_q6v5_adsp.c
> @@ -333,6 +336,185 @@ static int adsp_load(struct rproc *rproc, const struct firmware *fw)
> return 0;
> }
>
> +static void adsp_of_unmap_smmu(struct iommu_domain *iommu_dom, const __be32 *prop, int len)
> +{
> + unsigned long iova;
> + unsigned int mem_size;
> + int i;
> +
> + len /= sizeof(__be32);
> + for (i = 0; i < len; i++) {
> + iova = be32_to_cpu(prop[i++]);
> + /* Skip Physical address*/
> + i++;
> + mem_size = be32_to_cpu(prop[i++]);
> + iommu_unmap(iommu_dom, iova, mem_size);
> + }
> +}
> +
> +static void adsp_rproc_unmap_smmu(struct rproc *rproc, int len)
> +{
> + struct fw_rsc_devmem *rsc_fw;
> + struct fw_rsc_hdr *hdr;
> + int offset;
> + int i;
> +
> + for (i = 0; i < len; i++) {
> + offset = rproc->table_ptr->offset[i];
> + hdr = (void *)rproc->table_ptr + offset;
> + rsc_fw = (struct fw_rsc_devmem *)hdr + sizeof(*hdr);
> +
> + iommu_unmap(rproc->domain, rsc_fw->da, rsc_fw->len);
> + }
> +}
> +
> +static void adsp_unmap_smmu(struct rproc *rproc)
> +{
> + struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
> + const __be32 *prop;
> + unsigned int len;
> +
> + iommu_unmap(adsp->iommu_dom, adsp->mem_phys, adsp->mem_size);
> +
> + prop = of_get_property(adsp->dev->of_node, "qcom,adsp-memory-regions", &len);
> + if (prop) {
> + adsp_of_unmap_smmu(adsp->iommu_dom, prop, len);
> + } else {
> + if (rproc->table_ptr)
> + adsp_rproc_unmap_smmu(rproc, rproc->table_ptr->num);
> + }
> +
> + iommu_domain_free(adsp->iommu_dom);
> +}
> +
> +static int adsp_of_map_smmu(struct iommu_domain *iommu_dom, const __be32 *prop, int len)
> +{
> + unsigned long mem_phys;
> + unsigned long iova;
> + unsigned int mem_size;
> + unsigned int flag;
> + int access_level;
> + int ret;
> + int i;
> +
> + len /= sizeof(__be32);
> + for (i = 0; i < len; i++) {
> + iova = be32_to_cpu(prop[i++]);
> + mem_phys = be32_to_cpu(prop[i++]);
> + mem_size = be32_to_cpu(prop[i++]);
> + access_level = be32_to_cpu(prop[i]);
> +
> + if (access_level)
> + flag = IOMMU_READ | IOMMU_WRITE;
> + else
> + flag = IOMMU_READ;
> +
> + ret = iommu_map(iommu_dom, iova, mem_phys, mem_size, flag);
> + if (ret) {
> + pr_err("failed to map addr = %p mem_size = %x\n", &(mem_phys), mem_size);
Why can't this be dev_err()?
> + goto of_smmu_unmap;
> + }
> + }
> + return 0;
> +of_smmu_unmap:
> + adsp_of_unmap_smmu(iommu_dom, prop, i);
> + return ret;
> +}
> +
> +static int adsp_rproc_map_smmu(struct rproc *rproc, int len)
> +{
> + struct fw_rsc_devmem *rsc_fw;
const?
> + struct fw_rsc_hdr *hdr;
const?
> + int offset;
> + int ret;
> + int i;
> +
> + if (!rproc->table_ptr)
> + return 0;
> +
> + for (i = 0; i < rproc->table_ptr->num; i++) {
> + offset = rproc->table_ptr->offset[i];
> + hdr = (void *)rproc->table_ptr + offset;
> + rsc_fw = (struct fw_rsc_devmem *)hdr + sizeof(*hdr);
> +
> + ret = iommu_map(rproc->domain, rsc_fw->da, rsc_fw->pa,
> + rsc_fw->len, rsc_fw->flags);
> + if (ret) {
> + pr_err("failed to map addr = %x mem_size = %x\n", rsc_fw->pa, rsc_fw->len);
Why can't this be dev_err()?
> + goto rproc_smmu_unmap;
> + }
> + }
> +
> + return 0;
> +
> +rproc_smmu_unmap:
> + adsp_rproc_unmap_smmu(rproc, i);
Does i need to be incremented? And/or unmap should be done in reverse.
> + return ret;
> +}
> +
> +static int adsp_map_smmu(struct qcom_adsp *adsp, struct rproc *rproc)
> +{
> + struct of_phandle_args args;
> + const __be32 *prop;
> + long long sid;
> + unsigned int len;
> + int ret;
> +
> + ret = of_parse_phandle_with_args(adsp->dev->of_node, "iommus", "#iommu-cells", 0, &args);
> + if (ret < 0)
> + sid = -1;
Is it a good idea to set the sid to -1? Does that mean all stream IDs?
> + else
> + sid = args.args[0] & SID_MASK_DEFAULT;
> +
> + adsp->iommu_dom = iommu_domain_alloc(adsp->dev->bus);
> + if (!adsp->iommu_dom) {
> + dev_err(adsp->dev, "failed to allocate iommu domain\n");
> + ret = -ENOMEM;
> + goto domain_free;
> + }
> +
> + ret = iommu_attach_device(adsp->iommu_dom, adsp->dev);
> + if (ret) {
> + dev_err(adsp->dev, "could not attach device ret = %d\n", ret);
> + ret = -EBUSY;
Why do we overwrite the error value?
> + goto detach_device;
> + }
> +
> + /* Add SID configuration for ADSP Firmware to SMMU */
> + adsp->mem_phys = adsp->mem_phys | (sid << 32);
> +
> + ret = iommu_map(adsp->iommu_dom, adsp->mem_phys, adsp->mem_phys,
> + adsp->mem_size, IOMMU_READ | IOMMU_WRITE);
> + if (ret) {
> + dev_err(adsp->dev, "Unable to map ADSP Physical Memory\n");
> + goto sid_unmap;
> + }
> +
> + prop = of_get_property(adsp->dev->of_node, "qcom,adsp-memory-regions", &len);
I find it odd that we're encoding virtual addresses (iovas) into
devicetree. Presumably the physical address needs to be in DT as a
carveout, but after that I would think we're free to allocate the
segments from the carveout however we see fit and then program that into
the SMMU. Maybe DT can be a suggestion, but otherwise can it be ignored?
> + if (prop) {
> + ret = adsp_of_map_smmu(adsp->iommu_dom, prop, len);
> + if (ret) {
> + dev_err(adsp->dev, "Unable to map memory regions accessed by ADSP\n");
> + goto sid_unmap;
> + }
> + } else {
> + ret = adsp_rproc_map_smmu(rproc, len);
> + if (ret) {
> + dev_err(adsp->dev, "Unable to map memory regions accessed by ADSP\n");
Maybe this should be a different string in case it is confused with the
above print of the same string.
> + goto sid_unmap;
> + }
> + }
> + return 0;
> +
> +sid_unmap:
> + iommu_unmap(adsp->iommu_dom, adsp->mem_phys, adsp->mem_size);
> +detach_device:
> + iommu_domain_free(adsp->iommu_dom);
> +domain_free:
> + return ret;
> +}
> +
> +
> static int adsp_start(struct rproc *rproc)
> {
> struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
> @@ -343,9 +525,16 @@ static int adsp_start(struct rproc *rproc)
> if (ret)
> return ret;
>
> + if (adsp->adsp_sandbox_needed) {
> + ret = adsp_map_smmu(adsp, rproc);
> + if (ret) {
> + dev_err(adsp->dev, "ADSP smmu mapping failed\n");
> + goto disable_irqs;
> + }
> + }
Newline here please.
> ret = clk_prepare_enable(adsp->xo);
> if (ret)
> - goto disable_irqs;
> + goto adsp_smmu_unmap;
>
> ret = qcom_rproc_pds_enable(adsp, adsp->proxy_pds,
> adsp->proxy_pd_count);
On 8/23/2022 8:55 AM, Stephen Boyd wrote:
Thanks for your time and valuable suggestions!!!
> Quoting Srinivasa Rao Mandadapu (2022-08-22 01:22:02)
>> diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c
>> index b0a63a0..d01c97e 100644
>> --- a/drivers/remoteproc/qcom_q6v5_adsp.c
>> +++ b/drivers/remoteproc/qcom_q6v5_adsp.c
>> @@ -333,6 +336,185 @@ static int adsp_load(struct rproc *rproc, const struct firmware *fw)
>> return 0;
>> }
>>
>> +static void adsp_of_unmap_smmu(struct iommu_domain *iommu_dom, const __be32 *prop, int len)
>> +{
>> + unsigned long iova;
>> + unsigned int mem_size;
>> + int i;
>> +
>> + len /= sizeof(__be32);
>> + for (i = 0; i < len; i++) {
>> + iova = be32_to_cpu(prop[i++]);
>> + /* Skip Physical address*/
>> + i++;
>> + mem_size = be32_to_cpu(prop[i++]);
>> + iommu_unmap(iommu_dom, iova, mem_size);
>> + }
>> +}
>> +
>> +static void adsp_rproc_unmap_smmu(struct rproc *rproc, int len)
>> +{
>> + struct fw_rsc_devmem *rsc_fw;
>> + struct fw_rsc_hdr *hdr;
>> + int offset;
>> + int i;
>> +
>> + for (i = 0; i < len; i++) {
>> + offset = rproc->table_ptr->offset[i];
>> + hdr = (void *)rproc->table_ptr + offset;
>> + rsc_fw = (struct fw_rsc_devmem *)hdr + sizeof(*hdr);
>> +
>> + iommu_unmap(rproc->domain, rsc_fw->da, rsc_fw->len);
>> + }
>> +}
>> +
>> +static void adsp_unmap_smmu(struct rproc *rproc)
>> +{
>> + struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
>> + const __be32 *prop;
>> + unsigned int len;
>> +
>> + iommu_unmap(adsp->iommu_dom, adsp->mem_phys, adsp->mem_size);
>> +
>> + prop = of_get_property(adsp->dev->of_node, "qcom,adsp-memory-regions", &len);
>> + if (prop) {
>> + adsp_of_unmap_smmu(adsp->iommu_dom, prop, len);
>> + } else {
>> + if (rproc->table_ptr)
>> + adsp_rproc_unmap_smmu(rproc, rproc->table_ptr->num);
>> + }
>> +
>> + iommu_domain_free(adsp->iommu_dom);
>> +}
>> +
>> +static int adsp_of_map_smmu(struct iommu_domain *iommu_dom, const __be32 *prop, int len)
>> +{
>> + unsigned long mem_phys;
>> + unsigned long iova;
>> + unsigned int mem_size;
>> + unsigned int flag;
>> + int access_level;
>> + int ret;
>> + int i;
>> +
>> + len /= sizeof(__be32);
>> + for (i = 0; i < len; i++) {
>> + iova = be32_to_cpu(prop[i++]);
>> + mem_phys = be32_to_cpu(prop[i++]);
>> + mem_size = be32_to_cpu(prop[i++]);
>> + access_level = be32_to_cpu(prop[i]);
>> +
>> + if (access_level)
>> + flag = IOMMU_READ | IOMMU_WRITE;
>> + else
>> + flag = IOMMU_READ;
>> +
>> + ret = iommu_map(iommu_dom, iova, mem_phys, mem_size, flag);
>> + if (ret) {
>> + pr_err("failed to map addr = %p mem_size = %x\n", &(mem_phys), mem_size);
> Why can't this be dev_err()?
Actually, dev pointer is not available here, hence used pr_err.
>
>> + goto of_smmu_unmap;
>> + }
>> + }
>> + return 0;
>> +of_smmu_unmap:
>> + adsp_of_unmap_smmu(iommu_dom, prop, i);
>> + return ret;
>> +}
>> +
>> +static int adsp_rproc_map_smmu(struct rproc *rproc, int len)
>> +{
>> + struct fw_rsc_devmem *rsc_fw;
> const?
Okay. will update.
>
>> + struct fw_rsc_hdr *hdr;
> const?
Okay. Will update.
>
>> + int offset;
>> + int ret;
>> + int i;
>> +
>> + if (!rproc->table_ptr)
>> + return 0;
>> +
>> + for (i = 0; i < rproc->table_ptr->num; i++) {
>> + offset = rproc->table_ptr->offset[i];
>> + hdr = (void *)rproc->table_ptr + offset;
>> + rsc_fw = (struct fw_rsc_devmem *)hdr + sizeof(*hdr);
>> +
>> + ret = iommu_map(rproc->domain, rsc_fw->da, rsc_fw->pa,
>> + rsc_fw->len, rsc_fw->flags);
>> + if (ret) {
>> + pr_err("failed to map addr = %x mem_size = %x\n", rsc_fw->pa, rsc_fw->len);
> Why can't this be dev_err()?
Okay. Will change it.
>
>> + goto rproc_smmu_unmap;
>> + }
>> + }
>> +
>> + return 0;
>> +
>> +rproc_smmu_unmap:
>> + adsp_rproc_unmap_smmu(rproc, i);
> Does i need to be incremented? And/or unmap should be done in reverse.
Here i is the upper bound index in mapping failure case, hence it is
used as length. un-mapping is being done from starting till i value.
Please correct me if I am missing some thing.
>
>> + return ret;
>> +}
>> +
>> +static int adsp_map_smmu(struct qcom_adsp *adsp, struct rproc *rproc)
>> +{
>> + struct of_phandle_args args;
>> + const __be32 *prop;
>> + long long sid;
>> + unsigned int len;
>> + int ret;
>> +
>> + ret = of_parse_phandle_with_args(adsp->dev->of_node, "iommus", "#iommu-cells", 0, &args);
>> + if (ret < 0)
>> + sid = -1;
> Is it a good idea to set the sid to -1? Does that mean all stream IDs?
It seems, if sid is -1 iomap fails, because of alignment issues. Any I
will update with return in this case.
>
>> + else
>> + sid = args.args[0] & SID_MASK_DEFAULT;
>> +
>> + adsp->iommu_dom = iommu_domain_alloc(adsp->dev->bus);
>> + if (!adsp->iommu_dom) {
>> + dev_err(adsp->dev, "failed to allocate iommu domain\n");
>> + ret = -ENOMEM;
>> + goto domain_free;
>> + }
>> +
>> + ret = iommu_attach_device(adsp->iommu_dom, adsp->dev);
>> + if (ret) {
>> + dev_err(adsp->dev, "could not attach device ret = %d\n", ret);
>> + ret = -EBUSY;
> Why do we overwrite the error value?
It seems not required. Will remove it.
>
>> + goto detach_device;
>> + }
>> +
>> + /* Add SID configuration for ADSP Firmware to SMMU */
>> + adsp->mem_phys = adsp->mem_phys | (sid << 32);
>> +
>> + ret = iommu_map(adsp->iommu_dom, adsp->mem_phys, adsp->mem_phys,
>> + adsp->mem_size, IOMMU_READ | IOMMU_WRITE);
>> + if (ret) {
>> + dev_err(adsp->dev, "Unable to map ADSP Physical Memory\n");
>> + goto sid_unmap;
>> + }
>> +
>> + prop = of_get_property(adsp->dev->of_node, "qcom,adsp-memory-regions", &len);
> I find it odd that we're encoding virtual addresses (iovas) into
Actually from HLOS perspective, same IOVA and physical memory is being
used. Hence will remove virtual address field in DT.
> devicetree. Presumably the physical address needs to be in DT as a
> carveout, but after that I would think we're free to allocate the
Will try to carveout the physical addresses and handle it accordingly.
If this method is ignored I don't think we need to mention in DT in anyway.
> segments from the carveout however we see fit and then program that into
> the SMMU. Maybe DT can be a suggestion, but otherwise can it be ignored?
Yes, it seems it can be ignored. As it is the approach we did for
bringing up AudioReach solution, and used when ADSP binary is without
metadata section header info.
Will check internally and update accordingly.
Your opinion also helps please!!. Is it okay to keep it as backup option
with proper comment, such that this method can be used internally, with
raw ADSP binary in early stage bring-up scenarios?
>
>> + if (prop) {
>> + ret = adsp_of_map_smmu(adsp->iommu_dom, prop, len);
>> + if (ret) {
>> + dev_err(adsp->dev, "Unable to map memory regions accessed by ADSP\n");
>> + goto sid_unmap;
>> + }
>> + } else {
>> + ret = adsp_rproc_map_smmu(rproc, len);
>> + if (ret) {
>> + dev_err(adsp->dev, "Unable to map memory regions accessed by ADSP\n");
> Maybe this should be a different string in case it is confused with the
> above print of the same string.
Okay. Will modify the string.
>
>> + goto sid_unmap;
>> + }
>> + }
>> + return 0;
>> +
>> +sid_unmap:
>> + iommu_unmap(adsp->iommu_dom, adsp->mem_phys, adsp->mem_size);
>> +detach_device:
>> + iommu_domain_free(adsp->iommu_dom);
>> +domain_free:
>> + return ret;
>> +}
>> +
>> +
>> static int adsp_start(struct rproc *rproc)
>> {
>> struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
>> @@ -343,9 +525,16 @@ static int adsp_start(struct rproc *rproc)
>> if (ret)
>> return ret;
>>
>> + if (adsp->adsp_sandbox_needed) {
>> + ret = adsp_map_smmu(adsp, rproc);
>> + if (ret) {
>> + dev_err(adsp->dev, "ADSP smmu mapping failed\n");
>> + goto disable_irqs;
>> + }
>> + }
> Newline here please.
Okay.
>
>> ret = clk_prepare_enable(adsp->xo);
>> if (ret)
>> - goto disable_irqs;
>> + goto adsp_smmu_unmap;
>>
>> ret = qcom_rproc_pds_enable(adsp, adsp->proxy_pds,
>> adsp->proxy_pd_count);
On 8/25/2022 3:34 PM, Srinivasa Rao Mandadapu wrote:
>
> On 8/23/2022 8:55 AM, Stephen Boyd wrote:
> Thanks for your time and valuable suggestions!!!
>> Quoting Srinivasa Rao Mandadapu (2022-08-22 01:22:02)
>>> diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c
>>> b/drivers/remoteproc/qcom_q6v5_adsp.c
>>> index b0a63a0..d01c97e 100644
>>> --- a/drivers/remoteproc/qcom_q6v5_adsp.c
>>> +++ b/drivers/remoteproc/qcom_q6v5_adsp.c
>>> @@ -333,6 +336,185 @@ static int adsp_load(struct rproc *rproc,
>>> const struct firmware *fw)
>>> return 0;
>>> }
>>>
>>> +static void adsp_of_unmap_smmu(struct iommu_domain *iommu_dom,
>>> const __be32 *prop, int len)
>>> +{
>>> + unsigned long iova;
>>> + unsigned int mem_size;
>>> + int i;
>>> +
>>> + len /= sizeof(__be32);
>>> + for (i = 0; i < len; i++) {
>>> + iova = be32_to_cpu(prop[i++]);
>>> + /* Skip Physical address*/
>>> + i++;
>>> + mem_size = be32_to_cpu(prop[i++]);
>>> + iommu_unmap(iommu_dom, iova, mem_size);
>>> + }
>>> +}
>>> +
>>> +static void adsp_rproc_unmap_smmu(struct rproc *rproc, int len)
>>> +{
>>> + struct fw_rsc_devmem *rsc_fw;
>>> + struct fw_rsc_hdr *hdr;
>>> + int offset;
>>> + int i;
>>> +
>>> + for (i = 0; i < len; i++) {
>>> + offset = rproc->table_ptr->offset[i];
>>> + hdr = (void *)rproc->table_ptr + offset;
>>> + rsc_fw = (struct fw_rsc_devmem *)hdr + sizeof(*hdr);
>>> +
>>> + iommu_unmap(rproc->domain, rsc_fw->da, rsc_fw->len);
>>> + }
>>> +}
>>> +
>>> +static void adsp_unmap_smmu(struct rproc *rproc)
>>> +{
>>> + struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
>>> + const __be32 *prop;
>>> + unsigned int len;
>>> +
>>> + iommu_unmap(adsp->iommu_dom, adsp->mem_phys, adsp->mem_size);
>>> +
>>> + prop = of_get_property(adsp->dev->of_node,
>>> "qcom,adsp-memory-regions", &len);
>>> + if (prop) {
>>> + adsp_of_unmap_smmu(adsp->iommu_dom, prop, len);
>>> + } else {
>>> + if (rproc->table_ptr)
>>> + adsp_rproc_unmap_smmu(rproc,
>>> rproc->table_ptr->num);
>>> + }
>>> +
>>> + iommu_domain_free(adsp->iommu_dom);
>>> +}
>>> +
>>> +static int adsp_of_map_smmu(struct iommu_domain *iommu_dom, const
>>> __be32 *prop, int len)
>>> +{
>>> + unsigned long mem_phys;
>>> + unsigned long iova;
>>> + unsigned int mem_size;
>>> + unsigned int flag;
>>> + int access_level;
>>> + int ret;
>>> + int i;
>>> +
>>> + len /= sizeof(__be32);
>>> + for (i = 0; i < len; i++) {
>>> + iova = be32_to_cpu(prop[i++]);
>>> + mem_phys = be32_to_cpu(prop[i++]);
>>> + mem_size = be32_to_cpu(prop[i++]);
>>> + access_level = be32_to_cpu(prop[i]);
>>> +
>>> + if (access_level)
>>> + flag = IOMMU_READ | IOMMU_WRITE;
>>> + else
>>> + flag = IOMMU_READ;
>>> +
>>> + ret = iommu_map(iommu_dom, iova, mem_phys, mem_size,
>>> flag);
>>> + if (ret) {
>>> + pr_err("failed to map addr = %p mem_size =
>>> %x\n", &(mem_phys), mem_size);
>> Why can't this be dev_err()?
> Actually, dev pointer is not available here, hence used pr_err.
>>
>>> + goto of_smmu_unmap;
>>> + }
>>> + }
>>> + return 0;
>>> +of_smmu_unmap:
>>> + adsp_of_unmap_smmu(iommu_dom, prop, i);
>>> + return ret;
>>> +}
>>> +
>>> +static int adsp_rproc_map_smmu(struct rproc *rproc, int len)
>>> +{
>>> + struct fw_rsc_devmem *rsc_fw;
>> const?
> Okay. will update.
>>
>>> + struct fw_rsc_hdr *hdr;
>> const?
> Okay. Will update.
>>
>>> + int offset;
>>> + int ret;
>>> + int i;
>>> +
>>> + if (!rproc->table_ptr)
>>> + return 0;
>>> +
>>> + for (i = 0; i < rproc->table_ptr->num; i++) {
>>> + offset = rproc->table_ptr->offset[i];
>>> + hdr = (void *)rproc->table_ptr + offset;
>>> + rsc_fw = (struct fw_rsc_devmem *)hdr + sizeof(*hdr);
>>> +
>>> + ret = iommu_map(rproc->domain, rsc_fw->da, rsc_fw->pa,
>>> + rsc_fw->len, rsc_fw->flags);
>>> + if (ret) {
>>> + pr_err("failed to map addr = %x mem_size =
>>> %x\n", rsc_fw->pa, rsc_fw->len);
>> Why can't this be dev_err()?
> Okay. Will change it.
>>
>>> + goto rproc_smmu_unmap;
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> +
>>> +rproc_smmu_unmap:
>>> + adsp_rproc_unmap_smmu(rproc, i);
>> Does i need to be incremented? And/or unmap should be done in reverse.
>
> Here i is the upper bound index in mapping failure case, hence it is
> used as length. un-mapping is being done from starting till i value.
>
> Please correct me if I am missing some thing.
>
>>
>>> + return ret;
>>> +}
>>> +
>>> +static int adsp_map_smmu(struct qcom_adsp *adsp, struct rproc *rproc)
>>> +{
>>> + struct of_phandle_args args;
>>> + const __be32 *prop;
>>> + long long sid;
>>> + unsigned int len;
>>> + int ret;
>>> +
>>> + ret = of_parse_phandle_with_args(adsp->dev->of_node,
>>> "iommus", "#iommu-cells", 0, &args);
>>> + if (ret < 0)
>>> + sid = -1;
>> Is it a good idea to set the sid to -1? Does that mean all stream IDs?
> It seems, if sid is -1 iomap fails, because of alignment issues. Any I
> will update with return in this case.
>>
>>> + else
>>> + sid = args.args[0] & SID_MASK_DEFAULT;
>>> +
>>> + adsp->iommu_dom = iommu_domain_alloc(adsp->dev->bus);
>>> + if (!adsp->iommu_dom) {
>>> + dev_err(adsp->dev, "failed to allocate iommu
>>> domain\n");
>>> + ret = -ENOMEM;
>>> + goto domain_free;
>>> + }
>>> +
>>> + ret = iommu_attach_device(adsp->iommu_dom, adsp->dev);
>>> + if (ret) {
>>> + dev_err(adsp->dev, "could not attach device ret =
>>> %d\n", ret);
>>> + ret = -EBUSY;
>> Why do we overwrite the error value?
> It seems not required. Will remove it.
>>
>>> + goto detach_device;
>>> + }
>>> +
>>> + /* Add SID configuration for ADSP Firmware to SMMU */
>>> + adsp->mem_phys = adsp->mem_phys | (sid << 32);
>>> +
>>> + ret = iommu_map(adsp->iommu_dom, adsp->mem_phys,
>>> adsp->mem_phys,
>>> + adsp->mem_size, IOMMU_READ | IOMMU_WRITE);
>>> + if (ret) {
>>> + dev_err(adsp->dev, "Unable to map ADSP Physical
>>> Memory\n");
>>> + goto sid_unmap;
>>> + }
>>> +
>>> + prop = of_get_property(adsp->dev->of_node,
>>> "qcom,adsp-memory-regions", &len);
>> I find it odd that we're encoding virtual addresses (iovas) into
> Actually from HLOS perspective, same IOVA and physical memory is being
> used. Hence will remove virtual address field in DT.
>> devicetree. Presumably the physical address needs to be in DT as a
>> carveout, but after that I would think we're free to allocate the
> Will try to carveout the physical addresses and handle it accordingly.
> If this method is ignored I don't think we need to mention in DT in
> anyway.
>> segments from the carveout however we see fit and then program that into
>> the SMMU. Maybe DT can be a suggestion, but otherwise can it be ignored?
>
> Yes, it seems it can be ignored. As it is the approach we did for
> bringing up AudioReach solution, and used when ADSP binary is without
> metadata section header info.
>
> Will check internally and update accordingly.
>
> Your opinion also helps please!!. Is it okay to keep it as backup
> option with proper comment, such that this method can be used
> internally, with raw ADSP binary in early stage bring-up scenarios?
After internal discussions, decided to remove sandboxing using device
tree and explicitly doing in PIL driver.
Instead, decided to use rproc's parse_fw call back
"rproc_elf_load_rsc_table" function and achieve the sand boxing.
Will re post the patches with corresponding changes.
>
>>
>>> + if (prop) {
>>> + ret = adsp_of_map_smmu(adsp->iommu_dom, prop, len);
>>> + if (ret) {
>>> + dev_err(adsp->dev, "Unable to map memory
>>> regions accessed by ADSP\n");
>>> + goto sid_unmap;
>>> + }
>>> + } else {
>>> + ret = adsp_rproc_map_smmu(rproc, len);
>>> + if (ret) {
>>> + dev_err(adsp->dev, "Unable to map memory
>>> regions accessed by ADSP\n");
>> Maybe this should be a different string in case it is confused with the
>> above print of the same string.
> Okay. Will modify the string.
As explained above will remove both of the above methods.
>>
>>> + goto sid_unmap;
>>> + }
>>> + }
>>> + return 0;
>>> +
>>> +sid_unmap:
>>> + iommu_unmap(adsp->iommu_dom, adsp->mem_phys, adsp->mem_size);
>>> +detach_device:
>>> + iommu_domain_free(adsp->iommu_dom);
>>> +domain_free:
>>> + return ret;
>>> +}
>>> +
>>> +
>>> static int adsp_start(struct rproc *rproc)
>>> {
>>> struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
>>> @@ -343,9 +525,16 @@ static int adsp_start(struct rproc *rproc)
>>> if (ret)
>>> return ret;
>>>
>>> + if (adsp->adsp_sandbox_needed) {
>>> + ret = adsp_map_smmu(adsp, rproc);
>>> + if (ret) {
>>> + dev_err(adsp->dev, "ADSP smmu mapping
>>> failed\n");
>>> + goto disable_irqs;
>>> + }
>>> + }
>> Newline here please.
> Okay.
>>
>>> ret = clk_prepare_enable(adsp->xo);
>>> if (ret)
>>> - goto disable_irqs;
>>> + goto adsp_smmu_unmap;
>>>
>>> ret = qcom_rproc_pds_enable(adsp, adsp->proxy_pds,
>>> adsp->proxy_pd_count);