This Patch series fix the comment received on v2 patchset.
Major changes w.r.t. patchset v2 is as below.
1- move majority of hypervisor call related changes into scm driver
which were earlier in q6v5 rproc driver, which was not compatible
to existing design.
2- remove voting for proxy supplies till the time corner voting is supported.
Though testing of mss rproc driver is possible as cx and mx supplies are
in always power on domain.
3- Fix some other Minor comments.
This patchseries is based on Bjorns remoteproc git repository at:
git://github.com/andersson/remoteproc tags/rproc-v4.11
or to say this patchset is based on top of commit id 01625cc5e65f420dbea57f7de6b9552a0f807a84
in Bjorn's git repo.
Avaneesh Kumar Dwivedi (2):
soc: qcom: Add support of scm call for mss rproc to share access of
ddr
remoteproc: qcom: Enable mss rproc support on msm8996
.../devicetree/bindings/remoteproc/qcom,q6v5.txt | 1 +
drivers/firmware/qcom_scm-64.c | 25 ++
drivers/firmware/qcom_scm.c | 93 +++++++
drivers/firmware/qcom_scm.h | 3 +
drivers/remoteproc/qcom_q6v5_pil.c | 290 +++++++++++++++++++--
include/linux/qcom_scm.h | 14 +
6 files changed, 400 insertions(+), 26 deletions(-)
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
This patch add msm8996 mss reset sequence and other minor
changes for enabling mss rproc support.
Signed-off-by: Avaneesh Kumar Dwivedi <[email protected]>
---
.../devicetree/bindings/remoteproc/qcom,q6v5.txt | 1 +
drivers/remoteproc/qcom_q6v5_pil.c | 161 ++++++++++++++++++---
2 files changed, 138 insertions(+), 24 deletions(-)
diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
index 92347fe..f37b0da 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
@@ -10,6 +10,7 @@ on the Qualcomm Hexagon core.
"qcom,q6v5-pil",
"qcom,msm8916-mss-pil",
"qcom,msm8974-mss-pil"
+ "qcom,msm8996-mss-pil"
- reg:
Usage: required
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 62ad976..1676cd7 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -32,6 +32,7 @@
#include <linux/soc/qcom/mdt_loader.h>
#include <linux/soc/qcom/smem.h>
#include <linux/soc/qcom/smem_state.h>
+#include <linux/iopoll.h>
#include "remoteproc_internal.h"
#include "qcom_common.h"
@@ -64,6 +65,8 @@
#define QDSP6SS_RESET_REG 0x014
#define QDSP6SS_GFMUX_CTL_REG 0x020
#define QDSP6SS_PWR_CTL_REG 0x030
+#define QDSP6SS_MEM_PWR_CTL 0x0B0
+#define QDSP6SS_STRAP_ACC 0x110
/* AXI Halt Register Offsets */
#define AXI_HALTREQ_REG 0x0
@@ -92,6 +95,14 @@
#define QDSS_BHS_ON BIT(21)
#define QDSS_LDO_BYP BIT(22)
+/* QDSP6v56 parameters */
+#define QDSP6v56_LDO_BYP BIT(25)
+#define QDSP6v56_BHS_ON BIT(24)
+#define QDSP6v56_CLAMP_WL BIT(21)
+#define QDSP6v56_CLAMP_QMC_MEM BIT(22)
+#define HALT_CHECK_MAX_LOOPS 200
+#define QDSP6SS_XO_CBCR 0x0038
+#define QDSP6SS_ACC_OVERRIDE_VAL 0x20
struct reg_info {
struct regulator *reg;
int uV;
@@ -421,33 +432,97 @@ static int q6v5proc_reset(struct q6v5 *qproc)
{
u32 val;
int ret;
+ int i;
- /* Assert resets, stop core */
- val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
- val |= (Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE);
- writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
- /* Enable power block headswitch, and wait for it to stabilize */
- val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
- val |= QDSS_BHS_ON | QDSS_LDO_BYP;
- writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
- udelay(1);
-
- /*
- * Turn on memories. L2 banks should be done individually
- * to minimize inrush current.
- */
- val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
- val |= Q6SS_SLP_RET_N | Q6SS_L2TAG_SLP_NRET_N |
- Q6SS_ETB_SLP_NRET_N | Q6SS_L2DATA_STBY_N;
- writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
- val |= Q6SS_L2DATA_SLP_NRET_N_2;
- writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
- val |= Q6SS_L2DATA_SLP_NRET_N_1;
- writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
- val |= Q6SS_L2DATA_SLP_NRET_N_0;
- writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+ if (qproc->version == MSS_MSM8996) {
+ /* Override the ACC value if required */
+ writel(QDSP6SS_ACC_OVERRIDE_VAL,
+ qproc->reg_base + QDSP6SS_STRAP_ACC);
+
+ /* Assert resets, stop core */
+ val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
+ val |= Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE;
+ writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
+ /* BHS require xo cbcr to be enabled */
+ val = readl(qproc->reg_base + QDSP6SS_XO_CBCR);
+ val |= 0x1;
+ writel(val, qproc->reg_base + QDSP6SS_XO_CBCR);
+
+ /* Read CLKOFF bit to go low indicating CLK is enabled */
+ ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_XO_CBCR,
+ val, !(val & BIT(31)), 1, HALT_CHECK_MAX_LOOPS);
+ if (ret) {
+ dev_err(qproc->dev,
+ "xo cbcr enabling timed out (rc:%d)\n", ret);
+ return ret;
+ }
+ /* Enable power block headswitch and wait for it to stabilize */
+ val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+ val |= QDSP6v56_BHS_ON;
+ writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+ val |= readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+ udelay(1);
+
+ /* Put LDO in bypass mode */
+ val |= QDSP6v56_LDO_BYP;
+ writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+ /* Deassert QDSP6 compiler memory clamp */
+ val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+ val &= ~QDSP6v56_CLAMP_QMC_MEM;
+ writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+ /* Deassert memory peripheral sleep and L2 memory standby */
+ val |= Q6SS_L2DATA_STBY_N | Q6SS_SLP_RET_N;
+ writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+ /* Turn on L1, L2, ETB and JU memories 1 at a time */
+ val = readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
+ for (i = 19; i >= 0; i--) {
+ val |= BIT(i);
+ writel(val, qproc->reg_base +
+ QDSP6SS_MEM_PWR_CTL);
+ /*
+ * Read back value to ensure the write is done then
+ * wait for 1us for both memory peripheral and data
+ * array to turn on.
+ */
+ val |= readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
+ udelay(1);
+ }
+ /* Remove word line clamp */
+ val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+ val &= ~QDSP6v56_CLAMP_WL;
+ writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+ } else {
+ /* Assert resets, stop core */
+ val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
+ val |= Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE;
+ writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
+
+ /* Enable power block headswitch and wait for it to stabilize */
+ val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+ val |= QDSS_BHS_ON | QDSS_LDO_BYP;
+ writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+ val |= readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+ udelay(1);
+ /*
+ * Turn on memories. L2 banks should be done individually
+ * to minimize inrush current.
+ */
+ val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+ val |= Q6SS_SLP_RET_N | Q6SS_L2TAG_SLP_NRET_N |
+ Q6SS_ETB_SLP_NRET_N | Q6SS_L2DATA_STBY_N;
+ writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+ val |= Q6SS_L2DATA_SLP_NRET_N_2;
+ writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+ val |= Q6SS_L2DATA_SLP_NRET_N_1;
+ writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+ val |= Q6SS_L2DATA_SLP_NRET_N_0;
+ writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+ }
/* Remove IO clamp */
val &= ~Q6SS_CLAMP_IO;
writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
@@ -823,6 +898,7 @@ static int q6v5_stop(struct rproc *rproc)
{
struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
int ret;
+ u32 val;
qproc->running = false;
@@ -840,6 +916,15 @@ static int q6v5_stop(struct rproc *rproc)
q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
+ if (qproc->version == MSS_MSM8996) {
+ /*
+ * To avoid high MX current during LPASS/MSS restart.
+ */
+ val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+ val |= Q6SS_CLAMP_IO | QDSP6v56_CLAMP_WL |
+ QDSP6v56_CLAMP_QMC_MEM;
+ writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+ }
reset_control_assert(qproc->mss_restart);
q6v5_clk_disable(qproc->dev, qproc->active_clks,
qproc->active_clk_count);
@@ -1186,6 +1271,33 @@ static int q6v5_remove(struct platform_device *pdev)
return 0;
}
+static const struct rproc_hexagon_res msm8996_mss = {
+ .hexagon_mba_image = "mba.mbn",
+ .proxy_supply = (struct qcom_mss_reg_res[]) {
+ {}
+ },
+ .active_supply = (struct qcom_mss_reg_res[]) {
+ {},
+ {}
+ },
+ .proxy_clk_names = (char*[]){
+ "xo",
+ "pnoc",
+ "qdss",
+ NULL
+ },
+ .active_clk_names = (char*[]){
+ "iface",
+ "bus",
+ "mem",
+ "gpll0_mss_clk",
+ "snoc_axi_clk",
+ "mnoc_axi_clk",
+ NULL
+ },
+ .version = MSS_MSM8996,
+};
+
static const struct rproc_hexagon_res msm8916_mss = {
.hexagon_mba_image = "mba.mbn",
.proxy_supply = (struct qcom_mss_reg_res[]) {
@@ -1258,6 +1370,7 @@ static int q6v5_remove(struct platform_device *pdev)
{ .compatible = "qcom,q6v5-pil", .data = &msm8916_mss},
{ .compatible = "qcom,msm8916-mss-pil", .data = &msm8916_mss},
{ .compatible = "qcom,msm8974-mss-pil", .data = &msm8974_mss},
+ { .compatible = "qcom,msm8996-mss-pil", .data = &msm8996_mss},
{ },
};
MODULE_DEVICE_TABLE(of, q6v5_of_match);
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
This patch add scm call support to make hypervisor call to enable access
of fw regions in ddr to mss subsystem on arm-v8 arch soc's.
Signed-off-by: Avaneesh Kumar Dwivedi <[email protected]>
---
drivers/firmware/qcom_scm-64.c | 25 +++++++
drivers/firmware/qcom_scm.c | 93 ++++++++++++++++++++++++++
drivers/firmware/qcom_scm.h | 3 +
drivers/remoteproc/qcom_q6v5_pil.c | 129 ++++++++++++++++++++++++++++++++++++-
include/linux/qcom_scm.h | 14 ++++
5 files changed, 262 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
index 4a0f5ea..187fc00 100644
--- a/drivers/firmware/qcom_scm-64.c
+++ b/drivers/firmware/qcom_scm-64.c
@@ -358,3 +358,28 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool reset)
return ret ? : res.a1;
}
+
+int __qcom_scm_assign_mem(struct device *dev, struct vmid_detail vmid)
+{
+ int ret;
+ struct qcom_scm_desc desc = {0};
+ struct arm_smccc_res res;
+
+ desc.args[0] = vmid.fw_phy;
+ desc.args[1] = vmid.size_fw;
+ desc.args[2] = vmid.from_phy;
+ desc.args[3] = vmid.size_from;
+ desc.args[4] = vmid.to_phy;
+ desc.args[5] = vmid.size_to;
+ desc.args[6] = 0;
+
+ desc.arginfo = QCOM_SCM_ARGS(7, QCOM_SCM_RO, QCOM_SCM_VAL,
+ QCOM_SCM_RO, QCOM_SCM_VAL, QCOM_SCM_RO,
+ QCOM_SCM_VAL, QCOM_SCM_VAL);
+
+ ret = qcom_scm_call(dev, QCOM_SCM_SVC_MP,
+ QCOM_MEM_PROT_ASSIGN_ID,
+ &desc, &res);
+
+ return ret ? : res.a1;
+}
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 893f953ea..f137f34 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -42,6 +42,18 @@ struct qcom_scm {
static struct qcom_scm *__scm;
+struct dest_vm_and_perm_info {
+ __le32 vm;
+ __le32 perm;
+ __le32 *ctx;
+ __le32 ctx_size;
+};
+
+struct fw_region_info {
+ __le64 addr;
+ __le64 size;
+};
+
static int qcom_scm_clk_enable(void)
{
int ret;
@@ -292,6 +304,87 @@ int qcom_scm_pas_shutdown(u32 peripheral)
}
EXPORT_SYMBOL(qcom_scm_pas_shutdown);
+/**
+ * qcom_scm_assign_mem() - Allocate and fill vmid detail of old
+ * new owners of memory region for fw and metadata etc, Which is
+ * further passed to hypervisor, which does translate intermediate
+ * physical address used by subsystems.
+ * @vmid: structure with pointers and size detail of old and new
+ * owners vmid detail.
+ * Return 0 on success.
+ */
+int qcom_scm_assign_mem(struct vmid_detail vmid)
+{
+ unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
+ struct dest_vm_and_perm_info *to;
+ struct fw_region_info *fw_info;
+ __le64 fw_phy;
+ __le32 *from;
+ int ret = -ENOMEM;
+ int i;
+
+ from = dma_alloc_attrs(__scm->dev, vmid.size_from,
+ &vmid.from_phy, GFP_KERNEL, dma_attrs);
+ if (!from) {
+ dev_err(__scm->dev,
+ "failed to allocate buffer to pass source vmid detail\n");
+ return -ENOMEM;
+ }
+ to = dma_alloc_attrs(__scm->dev, vmid.size_to,
+ &vmid.to_phy, GFP_KERNEL, dma_attrs);
+ if (!to) {
+ dev_err(__scm->dev,
+ "failed to allocate buffer to pass destination vmid detail\n");
+ goto free_src_buff;
+ }
+ fw_info = dma_alloc_attrs(__scm->dev, sizeof(*fw_info),
+ &fw_phy, GFP_KERNEL, dma_attrs);
+ if (!fw_info) {
+ dev_err(__scm->dev,
+ "failed to allocate buffer to pass firmware detail\n");
+ goto free_dest_buff;
+ }
+
+ /* copy detail of original owner of ddr region */
+ /* in physically contigious buffer */
+ memcpy(from, vmid.from, vmid.size_from);
+
+ /* fill details of new owners of ddr region*/
+ /* in physically contigious buffer */
+ for (i = 0; i < (vmid.size_to / sizeof(__le32)); i++) {
+ to[i].vm = vmid.to[i];
+ to[i].perm = vmid.permission[i];
+ to[i].ctx = NULL;
+ to[i].ctx_size = 0;
+ }
+
+ /* copy detail of firmware region whose mapping need to be done */
+ /* in physically contigious buffer */
+ fw_info->addr = vmid.fw_phy;
+ fw_info->size = vmid.size_fw;
+
+ /* reuse fw_phy and size_fw members to fill address and size of */
+ /* fw_info buffer */
+ vmid.fw_phy = fw_phy;
+ vmid.size_to = sizeof(*to) * (vmid.size_to / sizeof(__le32));
+ vmid.size_fw = sizeof(*fw_info);
+ ret = __qcom_scm_assign_mem(__scm->dev, vmid);
+ if (!ret)
+ goto free_fw_buff;
+ return ret;
+free_fw_buff:
+ dma_free_attrs(__scm->dev, sizeof(*fw_info), fw_info,
+ fw_phy, dma_attrs);
+free_dest_buff:
+ dma_free_attrs(__scm->dev, vmid.size_to, to,
+ vmid.to_phy, dma_attrs);
+free_src_buff:
+ dma_free_attrs(__scm->dev, vmid.size_from, from,
+ vmid.from_phy, dma_attrs);
+ return ret;
+}
+EXPORT_SYMBOL(qcom_scm_assign_mem);
+
static int qcom_scm_pas_reset_assert(struct reset_controller_dev *rcdev,
unsigned long idx)
{
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index 3584b00..4665a11 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -55,6 +55,9 @@ extern int __qcom_scm_pas_mem_setup(struct device *dev, u32 peripheral,
extern int __qcom_scm_pas_auth_and_reset(struct device *dev, u32 peripheral);
extern int __qcom_scm_pas_shutdown(struct device *dev, u32 peripheral);
extern int __qcom_scm_pas_mss_reset(struct device *dev, bool reset);
+#define QCOM_SCM_SVC_MP 0xc
+#define QCOM_MEM_PROT_ASSIGN_ID 0x16
+extern int __qcom_scm_assign_mem(struct device *dev, struct vmid_detail vmid);
/* common error codes */
#define QCOM_SCM_V2_EBUSY -12
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 8fd697a..62ad976 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -110,6 +110,7 @@ struct rproc_hexagon_res {
struct qcom_mss_reg_res *active_supply;
char **proxy_clk_names;
char **active_clk_names;
+ int version;
};
struct q6v5 {
@@ -153,8 +154,28 @@ struct q6v5 {
size_t mpss_size;
struct qcom_rproc_subdev smd_subdev;
+ int version;
};
+enum {
+ MSS_MSM8916,
+ MSS_MSM8974,
+ MSS_MSM8996,
+};
+
+enum {
+ ASSIGN_ACCESS_MSA,
+ REMOVE_ACCESS_MSA,
+ ASSIGN_SHARED_ACCESS_MSA,
+ REMOVE_SHARED_ACCESS_MSA,
+};
+
+#define VMID_HLOS 0x3
+#define VMID_MSS_MSA 0xF
+#define PERM_READ 0x4
+#define PERM_WRITE 0x2
+#define PERM_EXEC 0x1
+#define PERM_RW (PERM_READ | PERM_WRITE)
static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
const struct qcom_mss_reg_res *reg_res)
{
@@ -288,6 +309,54 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,
return &table;
}
+int hyp_mem_access(int id, phys_addr_t addr, size_t size)
+{
+ struct vmid_detail vmid;
+ int ret;
+
+ switch (id) {
+ case ASSIGN_ACCESS_MSA:
+ vmid.from = (int[]) { VMID_HLOS };
+ vmid.to = (int[]) { VMID_MSS_MSA };
+ vmid.permission = (int[]) { PERM_READ | PERM_WRITE };
+ vmid.size_from = vmid.size_to = 1 * sizeof(__le32);
+ break;
+ case REMOVE_ACCESS_MSA:
+ vmid.from = (int[]) { VMID_MSS_MSA };
+ vmid.to = (int[]) { VMID_HLOS };
+ vmid.permission =
+ (int[]) { PERM_READ | PERM_WRITE | PERM_EXEC };
+ vmid.size_from = vmid.size_to = 1 * sizeof(__le32);
+ break;
+ case ASSIGN_SHARED_ACCESS_MSA:
+ vmid.from = (int[]) { VMID_HLOS };
+ vmid.to = (int[]) { VMID_HLOS, VMID_MSS_MSA };
+ vmid.permission = (int[]) { PERM_RW, PERM_RW };
+ vmid.size_from = 1 * sizeof(__le32);
+ vmid.size_to = 2 * sizeof(__le32);
+ break;
+ case REMOVE_SHARED_ACCESS_MSA:
+ vmid.from = (int[]) { VMID_HLOS, VMID_MSS_MSA };
+ vmid.to = (int[]) { VMID_HLOS };
+ vmid.permission =
+ (int[]) { PERM_READ | PERM_WRITE | PERM_EXEC };
+ vmid.size_from = 2 * sizeof(__le32);
+ vmid.size_to = 1 * sizeof(__le32);
+ break;
+ default:
+ break;
+ }
+
+ vmid.fw_phy = addr;
+ vmid.size_fw = size;
+ ret = qcom_scm_assign_mem(vmid);
+ if (ret)
+ pr_err("%s: Failed to assign memory protection, ret = %d\n",
+ __func__, ret);
+
+ return ret;
+}
+
static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
{
struct q6v5 *qproc = rproc->priv;
@@ -461,6 +530,15 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
memcpy(ptr, fw->data, fw->size);
+ /* Hypervisor mapping to access metadata by modem */
+ ret = qproc->version == MSS_MSM8996 ?
+ hyp_mem_access(ASSIGN_ACCESS_MSA, phys,
+ ALIGN(fw->size, SZ_4K)) : 0;
+ if (ret) {
+ dev_err(qproc->dev,
+ "Failed to assign metadata memory, ret - %d\n", ret);
+ return -ENOMEM;
+ }
writel(phys, qproc->rmb_base + RMB_PMI_META_DATA_REG);
writel(RMB_CMD_META_DATA_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
@@ -470,6 +548,13 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
else if (ret < 0)
dev_err(qproc->dev, "MPSS header authentication failed: %d\n", ret);
+ /* Metadata authentication done, remove modem access */
+ ret = qproc->version == MSS_MSM8996 ?
+ hyp_mem_access(REMOVE_ACCESS_MSA, phys,
+ ALIGN(fw->size, SZ_4K)) : 0;
+ if (ret)
+ dev_err(qproc->dev,
+ "Failed to reclaim metadata memory, ret - %d\n", ret);
dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs);
return ret < 0 ? ret : 0;
@@ -578,6 +663,16 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
size = readl(qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
if (!size) {
boot_addr = relocate ? qproc->mpss_phys : min_addr;
+ /* Hypervisor mapping of modem fw */
+ ret = qproc->version == MSS_MSM8996 ?
+ hyp_mem_access(ASSIGN_SHARED_ACCESS_MSA,
+ boot_addr, qproc->mpss_size) : 0;
+ if (ret) {
+ dev_err(qproc->dev,
+ "Failed to assign fw memory access, ret - %d\n",
+ ret);
+ return -ENOMEM;
+ }
writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG);
writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
}
@@ -636,6 +731,14 @@ static int q6v5_start(struct rproc *rproc)
goto assert_reset;
}
+ ret = qproc->version == MSS_MSM8996 ?
+ hyp_mem_access(ASSIGN_ACCESS_MSA, qproc->mba_phys,
+ qproc->mba_size) : 0;
+ if (ret) {
+ dev_err(qproc->dev,
+ "Failed to assign mba memory access, ret - %d\n", ret);
+ goto assert_reset;
+ }
writel(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG);
ret = q6v5proc_reset(qproc);
@@ -657,16 +760,22 @@ static int q6v5_start(struct rproc *rproc)
ret = q6v5_mpss_load(qproc);
if (ret)
- goto halt_axi_ports;
+ goto reclaim_mem;
ret = wait_for_completion_timeout(&qproc->start_done,
msecs_to_jiffies(5000));
if (ret == 0) {
dev_err(qproc->dev, "start timed out\n");
ret = -ETIMEDOUT;
- goto halt_axi_ports;
+ goto reclaim_mem;
}
+ ret = qproc->version == MSS_MSM8996 ?
+ hyp_mem_access(REMOVE_ACCESS_MSA, qproc->mba_phys,
+ qproc->mba_size) : 0;
+ if (ret)
+ dev_err(qproc->dev,
+ "Failed to reclaim mba memory, ret - %d\n", ret);
qproc->running = true;
q6v5_clk_disable(qproc->dev, qproc->proxy_clks,
@@ -676,7 +785,20 @@ static int q6v5_start(struct rproc *rproc)
return 0;
+reclaim_mem:
+ ret = qproc->version == MSS_MSM8996 ?
+ hyp_mem_access(REMOVE_SHARED_ACCESS_MSA,
+ qproc->mpss_phys, qproc->mpss_size) : 0;
+ if (ret)
+ dev_err(qproc->dev,
+ "Failed to reclaim fw memory, ret - %d\n", ret);
halt_axi_ports:
+ ret = qproc->version == MSS_MSM8996 ?
+ hyp_mem_access(REMOVE_ACCESS_MSA, qproc->mba_phys,
+ qproc->mba_size) : 0;
+ if (ret)
+ dev_err(qproc->dev,
+ "Failed to reclaim mba memory, ret - %d\n", ret);
q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
@@ -1015,6 +1137,7 @@ static int q6v5_probe(struct platform_device *pdev)
if (ret)
goto free_rproc;
+ qproc->version = desc->version;
ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt);
if (ret < 0)
goto free_rproc;
@@ -1090,6 +1213,7 @@ static int q6v5_remove(struct platform_device *pdev)
"mem",
NULL
},
+ .version = MSS_MSM8916,
};
static const struct rproc_hexagon_res msm8974_mss = {
@@ -1127,6 +1251,7 @@ static int q6v5_remove(struct platform_device *pdev)
"mem",
NULL
},
+ .version = MSS_MSM8974,
};
static const struct of_device_id q6v5_of_match[] = {
diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
index cc32ab8..cb0b7ee 100644
--- a/include/linux/qcom_scm.h
+++ b/include/linux/qcom_scm.h
@@ -23,6 +23,19 @@ struct qcom_scm_hdcp_req {
u32 val;
};
+struct vmid_detail {
+ __le32 *from;
+ __le32 *to;
+ __le32 *permission;
+ __le32 size_from;
+ __le32 size_to;
+ __le32 size_fw;
+ __le64 fw_phy;
+ __le64 from_phy;
+ __le64 to_phy;
+
+};
+
extern bool qcom_scm_is_available(void);
extern bool qcom_scm_hdcp_available(void);
@@ -36,6 +49,7 @@ extern int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr,
phys_addr_t size);
extern int qcom_scm_pas_auth_and_reset(u32 peripheral);
extern int qcom_scm_pas_shutdown(u32 peripheral);
+extern int qcom_scm_assign_mem(struct vmid_detail vmid);
#define QCOM_SCM_CPU_PWR_DOWN_L2_ON 0x0
#define QCOM_SCM_CPU_PWR_DOWN_L2_OFF 0x1
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
On 03/08, Avaneesh Kumar Dwivedi wrote:
> diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
> index 4a0f5ea..187fc00 100644
> --- a/drivers/firmware/qcom_scm-64.c
> +++ b/drivers/firmware/qcom_scm-64.c
> @@ -358,3 +358,28 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool reset)
>
> return ret ? : res.a1;
> }
> +
> +int __qcom_scm_assign_mem(struct device *dev, struct vmid_detail vmid)
Why are we passing a structure copy?
> +{
> + int ret;
> + struct qcom_scm_desc desc = {0};
> + struct arm_smccc_res res;
> +
> + desc.args[0] = vmid.fw_phy;
> + desc.args[1] = vmid.size_fw;
> + desc.args[2] = vmid.from_phy;
> + desc.args[3] = vmid.size_from;
> + desc.args[4] = vmid.to_phy;
> + desc.args[5] = vmid.size_to;
These should all cause sparse warnings because of incorrect
assignments. Given that these are all registers, I'm not sure why
the vmid_detail structure has __le32 in it.
> + desc.args[6] = 0;
> +
> + desc.arginfo = QCOM_SCM_ARGS(7, QCOM_SCM_RO, QCOM_SCM_VAL,
> + QCOM_SCM_RO, QCOM_SCM_VAL, QCOM_SCM_RO,
> + QCOM_SCM_VAL, QCOM_SCM_VAL);
> +
> + ret = qcom_scm_call(dev, QCOM_SCM_SVC_MP,
> + QCOM_MEM_PROT_ASSIGN_ID,
> + &desc, &res);
> +
> + return ret ? : res.a1;
> +}
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 893f953ea..f137f34 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -42,6 +42,18 @@ struct qcom_scm {
>
> static struct qcom_scm *__scm;
>
> +struct dest_vm_and_perm_info {
> + __le32 vm;
> + __le32 perm;
> + __le32 *ctx;
Drop the pointer? Just treat it like another number? Pointer is
really odd because it doesn't really make any sense what the
address of the pointer would be.
> + __le32 ctx_size;
> +};
> +
> +struct fw_region_info {
> + __le64 addr;
> + __le64 size;
> +};
> +
> static int qcom_scm_clk_enable(void)
> {
> int ret;
> @@ -292,6 +304,87 @@ int qcom_scm_pas_shutdown(u32 peripheral)
> }
> EXPORT_SYMBOL(qcom_scm_pas_shutdown);
>
> +/**
> + * qcom_scm_assign_mem() - Allocate and fill vmid detail of old
> + * new owners of memory region for fw and metadata etc, Which is
> + * further passed to hypervisor, which does translate intermediate
> + * physical address used by subsystems.
Maybe this can be the long description, but the short description
should be shorter.
> + * @vmid: structure with pointers and size detail of old and new
> + * owners vmid detail.
> + * Return 0 on success.
There's a standard syntax for return too. Look at kernel doc
howto please.
> + */
> +int qcom_scm_assign_mem(struct vmid_detail vmid)
Please no structure copy.
> +{
> + unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
> + struct dest_vm_and_perm_info *to;
> + struct fw_region_info *fw_info;
> + __le64 fw_phy;
> + __le32 *from;
> + int ret = -ENOMEM;
Not sure why we assign it. It gets overwritten with the first use.
> + int i;
> +
> + from = dma_alloc_attrs(__scm->dev, vmid.size_from,
> + &vmid.from_phy, GFP_KERNEL, dma_attrs);
> + if (!from) {
> + dev_err(__scm->dev,
> + "failed to allocate buffer to pass source vmid detail\n");
> + return -ENOMEM;
> + }
> + to = dma_alloc_attrs(__scm->dev, vmid.size_to,
> + &vmid.to_phy, GFP_KERNEL, dma_attrs);
> + if (!to) {
> + dev_err(__scm->dev,
> + "failed to allocate buffer to pass destination vmid detail\n");
> + goto free_src_buff;
> + }
> + fw_info = dma_alloc_attrs(__scm->dev, sizeof(*fw_info),
> + &fw_phy, GFP_KERNEL, dma_attrs);
Can we consolidate this into one allocation with the appropriate
size and then offset the different structures in it?
> + if (!fw_info) {
> + dev_err(__scm->dev,
> + "failed to allocate buffer to pass firmware detail\n");
> + goto free_dest_buff;
> + }
> +
> + /* copy detail of original owner of ddr region */
> + /* in physically contigious buffer */
> + memcpy(from, vmid.from, vmid.size_from);
> +
> + /* fill details of new owners of ddr region*/
> + /* in physically contigious buffer */
> + for (i = 0; i < (vmid.size_to / sizeof(__le32)); i++) {
> + to[i].vm = vmid.to[i];
> + to[i].perm = vmid.permission[i];
Here you want the cpu_to_le32() stuff. Please run sparse.
> + to[i].ctx = NULL;
> + to[i].ctx_size = 0;
> + }
> +
> + /* copy detail of firmware region whose mapping need to be done */
> + /* in physically contigious buffer */
/*
* Multi-line comments are like so.
*/
> + fw_info->addr = vmid.fw_phy;
> + fw_info->size = vmid.size_fw;
> +
> + /* reuse fw_phy and size_fw members to fill address and size of */
> + /* fw_info buffer */
> + vmid.fw_phy = fw_phy;
> + vmid.size_to = sizeof(*to) * (vmid.size_to / sizeof(__le32));
> + vmid.size_fw = sizeof(*fw_info);
> + ret = __qcom_scm_assign_mem(__scm->dev, vmid);
> + if (!ret)
> + goto free_fw_buff;
> + return ret;
We don't free dma on failure?
> +free_fw_buff:
> + dma_free_attrs(__scm->dev, sizeof(*fw_info), fw_info,
> + fw_phy, dma_attrs);
> +free_dest_buff:
> + dma_free_attrs(__scm->dev, vmid.size_to, to,
> + vmid.to_phy, dma_attrs);
> +free_src_buff:
> + dma_free_attrs(__scm->dev, vmid.size_from, from,
> + vmid.from_phy, dma_attrs);
> + return ret;
> +}
> +EXPORT_SYMBOL(qcom_scm_assign_mem);
> +
> static int qcom_scm_pas_reset_assert(struct reset_controller_dev *rcdev,
> unsigned long idx)
> {
> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
> index 3584b00..4665a11 100644
> --- a/drivers/firmware/qcom_scm.h
> +++ b/drivers/firmware/qcom_scm.h
> @@ -55,6 +55,9 @@ extern int __qcom_scm_pas_mem_setup(struct device *dev, u32 peripheral,
> extern int __qcom_scm_pas_auth_and_reset(struct device *dev, u32 peripheral);
> extern int __qcom_scm_pas_shutdown(struct device *dev, u32 peripheral);
> extern int __qcom_scm_pas_mss_reset(struct device *dev, bool reset);
> +#define QCOM_SCM_SVC_MP 0xc
This is already defined upstream?
> +#define QCOM_MEM_PROT_ASSIGN_ID 0x16
> +extern int __qcom_scm_assign_mem(struct device *dev, struct vmid_detail vmid);
>
> /* common error codes */
> #define QCOM_SCM_V2_EBUSY -12
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index 8fd697a..62ad976 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -288,6 +309,54 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,
> return &table;
> }
>
> +int hyp_mem_access(int id, phys_addr_t addr, size_t size)
static?
>
> static const struct of_device_id q6v5_of_match[] = {
> diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
> index cc32ab8..cb0b7ee 100644
> --- a/include/linux/qcom_scm.h
> +++ b/include/linux/qcom_scm.h
> @@ -23,6 +23,19 @@ struct qcom_scm_hdcp_req {
> u32 val;
> };
>
> +struct vmid_detail {
> + __le32 *from;
> + __le32 *to;
> + __le32 *permission;
> + __le32 size_from;
> + __le32 size_to;
> + __le32 size_fw;
> + __le64 fw_phy;
> + __le64 from_phy;
> + __le64 to_phy;
should the *_phy be phys_addr_t types?
Leave these all as u32/u64. Perhaps also move size_from/size_to
next to the arrays they're for. Also add some documentation so we
know what they're all about.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On Wed 08 Mar 10:03 PST 2017, Avaneesh Kumar Dwivedi wrote:
> This patch add scm call support to make hypervisor call to enable access
> of fw regions in ddr to mss subsystem on arm-v8 arch soc's.
>
> Signed-off-by: Avaneesh Kumar Dwivedi <[email protected]>
> ---
> drivers/firmware/qcom_scm-64.c | 25 +++++++
> drivers/firmware/qcom_scm.c | 93 ++++++++++++++++++++++++++
> drivers/firmware/qcom_scm.h | 3 +
> drivers/remoteproc/qcom_q6v5_pil.c | 129 ++++++++++++++++++++++++++++++++++++-
> include/linux/qcom_scm.h | 14 ++++
> 5 files changed, 262 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
> index 4a0f5ea..187fc00 100644
> --- a/drivers/firmware/qcom_scm-64.c
> +++ b/drivers/firmware/qcom_scm-64.c
> @@ -358,3 +358,28 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool reset)
>
> return ret ? : res.a1;
> }
> +
> +int __qcom_scm_assign_mem(struct device *dev, struct vmid_detail vmid)
Rather than packing these parameters up in a struct I think it's cleaner
to just pass them directly.
> +{
> + int ret;
> + struct qcom_scm_desc desc = {0};
> + struct arm_smccc_res res;
> +
> + desc.args[0] = vmid.fw_phy;
> + desc.args[1] = vmid.size_fw;
> + desc.args[2] = vmid.from_phy;
> + desc.args[3] = vmid.size_from;
> + desc.args[4] = vmid.to_phy;
> + desc.args[5] = vmid.size_to;
> + desc.args[6] = 0;
> +
> + desc.arginfo = QCOM_SCM_ARGS(7, QCOM_SCM_RO, QCOM_SCM_VAL,
> + QCOM_SCM_RO, QCOM_SCM_VAL, QCOM_SCM_RO,
> + QCOM_SCM_VAL, QCOM_SCM_VAL);
> +
> + ret = qcom_scm_call(dev, QCOM_SCM_SVC_MP,
> + QCOM_MEM_PROT_ASSIGN_ID,
> + &desc, &res);
> +
> + return ret ? : res.a1;
If I understand the downstream code we only care about "ret" here; being
zero on success or negative on error.
> +}
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 893f953ea..f137f34 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -42,6 +42,18 @@ struct qcom_scm {
>
> static struct qcom_scm *__scm;
>
> +struct dest_vm_and_perm_info {
> + __le32 vm;
> + __le32 perm;
> + __le32 *ctx;
Please be explicit about the fact that this is 64 bit.
> + __le32 ctx_size;
> +};
This should be __packed
> +
> +struct fw_region_info {
> + __le64 addr;
> + __le64 size;
> +};
> +
> static int qcom_scm_clk_enable(void)
> {
> int ret;
> @@ -292,6 +304,87 @@ int qcom_scm_pas_shutdown(u32 peripheral)
> }
> EXPORT_SYMBOL(qcom_scm_pas_shutdown);
>
> +/**
> + * qcom_scm_assign_mem() - Allocate and fill vmid detail of old
> + * new owners of memory region for fw and metadata etc, Which is
> + * further passed to hypervisor, which does translate intermediate
> + * physical address used by subsystems.
> + * @vmid: structure with pointers and size detail of old and new
> + * owners vmid detail.
> + * Return 0 on success.
> + */
> +int qcom_scm_assign_mem(struct vmid_detail vmid)
After a long discussion with Stephen I now think that I understand
what's actually going on here.
So this function will request TZ to remove all permissions for the
memory region in the tables specified in "from" and then for each vmid
in "to" it will set up the specified "permission".
So the "to" and "permissions" are actually a tuple, rather than
independent lists of values. So I think this should be exposed in the
prototype, as a list of <vmid, permission> entries.
To make the function prototype more convenient I think you should turn
"from" into a bitmap (e.g. BIT(VMID_HLOS) | BIT(VMID_MSS_MSA)).
If you then make the function, on success, return "to" as a bitmap the
caller can simply store that in a state variable and pass it as "from"
in the next call.
So you would have:
struct qcom_scm_mem_perm new_perms[] = {
{ VMID_HLOS, PERM_READ },
{ VMID_MSS_MSA, PREM_READ | PERM_WRITE },
};
current_perms = qcom_scm_assign_mem(ptr, size, current_perms, new_perms, 2);
And I believe something like "curr_perm" and "new_perm" are even better
names than "from" and "to"
> +{
> + unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
> + struct dest_vm_and_perm_info *to;
> + struct fw_region_info *fw_info;
> + __le64 fw_phy;
> + __le32 *from;
> + int ret = -ENOMEM;
> + int i;
> +
> + from = dma_alloc_attrs(__scm->dev, vmid.size_from,
> + &vmid.from_phy, GFP_KERNEL, dma_attrs);
> + if (!from) {
> + dev_err(__scm->dev,
> + "failed to allocate buffer to pass source vmid detail\n");
> + return -ENOMEM;
> + }
> + to = dma_alloc_attrs(__scm->dev, vmid.size_to,
> + &vmid.to_phy, GFP_KERNEL, dma_attrs);
> + if (!to) {
> + dev_err(__scm->dev,
> + "failed to allocate buffer to pass destination vmid detail\n");
> + goto free_src_buff;
> + }
> + fw_info = dma_alloc_attrs(__scm->dev, sizeof(*fw_info),
> + &fw_phy, GFP_KERNEL, dma_attrs);
> + if (!fw_info) {
> + dev_err(__scm->dev,
> + "failed to allocate buffer to pass firmware detail\n");
> + goto free_dest_buff;
> + }
You should be able to allocate these in one chunk and you should be able
to just use dma_alloc_coherent().
> +
> + /* copy detail of original owner of ddr region */
> + /* in physically contigious buffer */
> + memcpy(from, vmid.from, vmid.size_from);
With "from" as a bitmap see hweight_long() to figure out the size of
"from".
> +
> + /* fill details of new owners of ddr region*/
> + /* in physically contigious buffer */
> + for (i = 0; i < (vmid.size_to / sizeof(__le32)); i++) {
Better pass "number of entries in 'to'" and use standard types (such as
int or unsigned int) for "to" and "permission" until this point.
> + to[i].vm = vmid.to[i];
> + to[i].perm = vmid.permission[i];
> + to[i].ctx = NULL;
> + to[i].ctx_size = 0;
> + }
> +
> + /* copy detail of firmware region whose mapping need to be done */
> + /* in physically contigious buffer */
> + fw_info->addr = vmid.fw_phy;
> + fw_info->size = vmid.size_fw;
fw_info is a confusing name for "a list of memory regions".
And you need a cpu_to_le32() somewhere, better keep the in-kernel API
pass standard types (such as phys_addr_t) and then convert to the
"hardware specific" type here.
> +
> + /* reuse fw_phy and size_fw members to fill address and size of */
> + /* fw_info buffer */
Please don't try to be "clever" and reuse things when writing
kernel-code, non-obvious reuse of variables or struct members often end
up causing someone else to introduce bugs in your code later.
> + vmid.fw_phy = fw_phy;
> + vmid.size_to = sizeof(*to) * (vmid.size_to / sizeof(__le32));
> + vmid.size_fw = sizeof(*fw_info);
> + ret = __qcom_scm_assign_mem(__scm->dev, vmid);
> + if (!ret)
> + goto free_fw_buff;
> + return ret;
> +free_fw_buff:
> + dma_free_attrs(__scm->dev, sizeof(*fw_info), fw_info,
> + fw_phy, dma_attrs);
> +free_dest_buff:
> + dma_free_attrs(__scm->dev, vmid.size_to, to,
> + vmid.to_phy, dma_attrs);
> +free_src_buff:
> + dma_free_attrs(__scm->dev, vmid.size_from, from,
> + vmid.from_phy, dma_attrs);
> + return ret;
> +}
> +EXPORT_SYMBOL(qcom_scm_assign_mem);
> +
> static int qcom_scm_pas_reset_assert(struct reset_controller_dev *rcdev,
> unsigned long idx)
> {
> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
> index 3584b00..4665a11 100644
> --- a/drivers/firmware/qcom_scm.h
> +++ b/drivers/firmware/qcom_scm.h
> @@ -55,6 +55,9 @@ extern int __qcom_scm_pas_mem_setup(struct device *dev, u32 peripheral,
> extern int __qcom_scm_pas_auth_and_reset(struct device *dev, u32 peripheral);
> extern int __qcom_scm_pas_shutdown(struct device *dev, u32 peripheral);
> extern int __qcom_scm_pas_mss_reset(struct device *dev, bool reset);
> +#define QCOM_SCM_SVC_MP 0xc
> +#define QCOM_MEM_PROT_ASSIGN_ID 0x16
> +extern int __qcom_scm_assign_mem(struct device *dev, struct vmid_detail vmid);
>
> /* common error codes */
> #define QCOM_SCM_V2_EBUSY -12
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index 8fd697a..62ad976 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
Please split the patch in a scm-patch and a q6v5 patch.
> @@ -110,6 +110,7 @@ struct rproc_hexagon_res {
> struct qcom_mss_reg_res *active_supply;
> char **proxy_clk_names;
> char **active_clk_names;
> + int version;
This will result in multi-line conditionals checking if we should do
memory protection for platform x, y, z or not. Add a "bool
need_mem_protect;" instead.
> };
>
> struct q6v5 {
> @@ -153,8 +154,28 @@ struct q6v5 {
> size_t mpss_size;
>
> struct qcom_rproc_subdev smd_subdev;
> + int version;
> };
>
> +enum {
> + MSS_MSM8916,
> + MSS_MSM8974,
> + MSS_MSM8996,
> +};
> +
> +enum {
> + ASSIGN_ACCESS_MSA,
> + REMOVE_ACCESS_MSA,
> + ASSIGN_SHARED_ACCESS_MSA,
> + REMOVE_SHARED_ACCESS_MSA,
> +};
> +
> +#define VMID_HLOS 0x3
> +#define VMID_MSS_MSA 0xF
> +#define PERM_READ 0x4
> +#define PERM_WRITE 0x2
> +#define PERM_EXEC 0x1
> +#define PERM_RW (PERM_READ | PERM_WRITE)
These belongs in the SCM API instead. (Prefix them appropriately)
> static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
> const struct qcom_mss_reg_res *reg_res)
> {
> @@ -288,6 +309,54 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,
> return &table;
> }
>
> +int hyp_mem_access(int id, phys_addr_t addr, size_t size)
I much prefer the downstream split and naming of this function, rather
than using a switch to implement 4 different functions in one please
split it up.
> +{
> + struct vmid_detail vmid;
> + int ret;
> +
Pass the struct q6v5 here and return successfully here if we're not
supposed to do memory protection. It does clean up the calling code.
> + switch (id) {
> + case ASSIGN_ACCESS_MSA:
> + vmid.from = (int[]) { VMID_HLOS };
> + vmid.to = (int[]) { VMID_MSS_MSA };
> + vmid.permission = (int[]) { PERM_READ | PERM_WRITE };
> + vmid.size_from = vmid.size_to = 1 * sizeof(__le32);
> + break;
> + case REMOVE_ACCESS_MSA:
> + vmid.from = (int[]) { VMID_MSS_MSA };
> + vmid.to = (int[]) { VMID_HLOS };
> + vmid.permission =
> + (int[]) { PERM_READ | PERM_WRITE | PERM_EXEC };
> + vmid.size_from = vmid.size_to = 1 * sizeof(__le32);
> + break;
> + case ASSIGN_SHARED_ACCESS_MSA:
> + vmid.from = (int[]) { VMID_HLOS };
> + vmid.to = (int[]) { VMID_HLOS, VMID_MSS_MSA };
> + vmid.permission = (int[]) { PERM_RW, PERM_RW };
> + vmid.size_from = 1 * sizeof(__le32);
> + vmid.size_to = 2 * sizeof(__le32);
> + break;
> + case REMOVE_SHARED_ACCESS_MSA:
> + vmid.from = (int[]) { VMID_HLOS, VMID_MSS_MSA };
> + vmid.to = (int[]) { VMID_HLOS };
> + vmid.permission =
> + (int[]) { PERM_READ | PERM_WRITE | PERM_EXEC };
> + vmid.size_from = 2 * sizeof(__le32);
> + vmid.size_to = 1 * sizeof(__le32);
> + break;
> + default:
> + break;
> + }
> +
> + vmid.fw_phy = addr;
> + vmid.size_fw = size;
> + ret = qcom_scm_assign_mem(vmid);
> + if (ret)
> + pr_err("%s: Failed to assign memory protection, ret = %d\n",
> + __func__, ret);
> +
> + return ret;
> +}
> +
> static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
> {
> struct q6v5 *qproc = rproc->priv;
> @@ -461,6 +530,15 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
>
> memcpy(ptr, fw->data, fw->size);
>
> + /* Hypervisor mapping to access metadata by modem */
> + ret = qproc->version == MSS_MSM8996 ?
To prove the point above, imagine just in the next year when this
becomes:
ret = (qproc->version == MSS_MSM8996 ||
qproc->version == MSS_MSM8998 ||
qproc->version == MSS_NEXT_THING) ?
hyp_mem_access(ASSIGN_ACCESS_MSA, phys, ALIGN(fw->size, SZ_4K)) :
0;
Or in 5 years...
> + hyp_mem_access(ASSIGN_ACCESS_MSA, phys,
> + ALIGN(fw->size, SZ_4K)) : 0;
> + if (ret) {
> + dev_err(qproc->dev,
> + "Failed to assign metadata memory, ret - %d\n", ret);
Rather than having this print after each call to hyp_mem_access() move
the message into that function.
> + return -ENOMEM;
And don't forget to clean up the allocation before returning.
> + }
> writel(phys, qproc->rmb_base + RMB_PMI_META_DATA_REG);
> writel(RMB_CMD_META_DATA_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
>
> @@ -470,6 +548,13 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
> else if (ret < 0)
> dev_err(qproc->dev, "MPSS header authentication failed: %d\n", ret);
>
> + /* Metadata authentication done, remove modem access */
> + ret = qproc->version == MSS_MSM8996 ?
> + hyp_mem_access(REMOVE_ACCESS_MSA, phys,
> + ALIGN(fw->size, SZ_4K)) : 0;
You use ALIGN(fw->size, SZ_4K) numerous times in this function, use a
variable and make the allocation take this as well to make it explicit
that we're dealing with the same memory chunk size all over.
> + if (ret)
> + dev_err(qproc->dev,
> + "Failed to reclaim metadata memory, ret - %d\n", ret);
> dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs);
>
> return ret < 0 ? ret : 0;
> @@ -578,6 +663,16 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
> size = readl(qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
> if (!size) {
> boot_addr = relocate ? qproc->mpss_phys : min_addr;
> + /* Hypervisor mapping of modem fw */
> + ret = qproc->version == MSS_MSM8996 ?
> + hyp_mem_access(ASSIGN_SHARED_ACCESS_MSA,
> + boot_addr, qproc->mpss_size) : 0;
The mpss is loaded somewhere in qproc->mpss_region, potentially at some
offset (which would be reflected in boot_addr).
But if boot_addr > qproc->mpss_region the region to hand out is no
longer mpss_size large, it's "mpss_size - (boot_addr - mpss_phys)", so
you might give the mpss permission to trash the memory after the
mpss_region.
Better hand out qproc->mpss_phys of size qproc->mpss_size.
And as far as I can tell the downstream driver load the segments and
then make the MSS sole owner of the memory region. Do note that it's
perfectly fine to refactor code to make things better fit a natural and
easy to follow flow of ownership here!
> + if (ret) {
> + dev_err(qproc->dev,
> + "Failed to assign fw memory access, ret - %d\n",
> + ret);
> + return -ENOMEM;
> + }
> writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG);
> writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
> }
> @@ -636,6 +731,14 @@ static int q6v5_start(struct rproc *rproc)
> goto assert_reset;
> }
>
> + ret = qproc->version == MSS_MSM8996 ?
> + hyp_mem_access(ASSIGN_ACCESS_MSA, qproc->mba_phys,
> + qproc->mba_size) : 0;
> + if (ret) {
> + dev_err(qproc->dev,
> + "Failed to assign mba memory access, ret - %d\n", ret);
> + goto assert_reset;
> + }
> writel(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG);
>
> ret = q6v5proc_reset(qproc);
> @@ -657,16 +760,22 @@ static int q6v5_start(struct rproc *rproc)
>
> ret = q6v5_mpss_load(qproc);
> if (ret)
> - goto halt_axi_ports;
> + goto reclaim_mem;
>
> ret = wait_for_completion_timeout(&qproc->start_done,
> msecs_to_jiffies(5000));
> if (ret == 0) {
> dev_err(qproc->dev, "start timed out\n");
> ret = -ETIMEDOUT;
> - goto halt_axi_ports;
> + goto reclaim_mem;
> }
>
> + ret = qproc->version == MSS_MSM8996 ?
> + hyp_mem_access(REMOVE_ACCESS_MSA, qproc->mba_phys,
> + qproc->mba_size) : 0;
> + if (ret)
> + dev_err(qproc->dev,
> + "Failed to reclaim mba memory, ret - %d\n", ret);
> qproc->running = true;
>
> q6v5_clk_disable(qproc->dev, qproc->proxy_clks,
> @@ -676,7 +785,20 @@ static int q6v5_start(struct rproc *rproc)
>
On success q6v5_start() will leave Linus as sole owner of mba_phys and
mpss_phys (or boot_addr) will have shared ownership. rproc_stop() should
make sure to reclaim the sole ownership of mpss_phys again, so that
subsequent calls to rproc_start() will find the memory protection in an
appropriate state.
> return 0;
>
> +reclaim_mem:
> + ret = qproc->version == MSS_MSM8996 ?
> + hyp_mem_access(REMOVE_SHARED_ACCESS_MSA,
> + qproc->mpss_phys, qproc->mpss_size) : 0;
> + if (ret)
> + dev_err(qproc->dev,
> + "Failed to reclaim fw memory, ret - %d\n", ret);
> halt_axi_ports:
> + ret = qproc->version == MSS_MSM8996 ?
> + hyp_mem_access(REMOVE_ACCESS_MSA, qproc->mba_phys,
> + qproc->mba_size) : 0;
> + if (ret)
> + dev_err(qproc->dev,
> + "Failed to reclaim mba memory, ret - %d\n", ret);
Isn't it possible that the remote processor is still executing the MBA
firmware up until we halt the axi ports and assert the reset? We don't
want to remove permission until we know the CPU is stopped and it wont
fault on us.
> q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
> q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
> q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
> @@ -1015,6 +1137,7 @@ static int q6v5_probe(struct platform_device *pdev)
> if (ret)
> goto free_rproc;
>
> + qproc->version = desc->version;
> ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt);
> if (ret < 0)
> goto free_rproc;
> @@ -1090,6 +1213,7 @@ static int q6v5_remove(struct platform_device *pdev)
> "mem",
> NULL
> },
> + .version = MSS_MSM8916,
> };
>
> static const struct rproc_hexagon_res msm8974_mss = {
> @@ -1127,6 +1251,7 @@ static int q6v5_remove(struct platform_device *pdev)
> "mem",
> NULL
> },
> + .version = MSS_MSM8974,
> };
>
> static const struct of_device_id q6v5_of_match[] = {
> diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
> index cc32ab8..cb0b7ee 100644
> --- a/include/linux/qcom_scm.h
> +++ b/include/linux/qcom_scm.h
> @@ -23,6 +23,19 @@ struct qcom_scm_hdcp_req {
> u32 val;
> };
>
> +struct vmid_detail {
Whenever you add structs to a public API, make sure to add some
kerneldoc.
And as this is a kernel-API I would like to see standard kernel-types
for things.
> + __le32 *from;
> + __le32 *to;
> + __le32 *permission;
> + __le32 size_from;
> + __le32 size_to;
> + __le32 size_fw;
> + __le64 fw_phy;
> + __le64 from_phy;
> + __le64 to_phy;
> +
> +};
> +
Regards,
Bjorn
Hi Bjorn/Boyd,
thanks for comments and suggestion, beg apology for delayed reply as i
got busy with internal issues.
please see inline response.
On 4/6/2017 10:13 AM, Bjorn Andersson wrote:
> On Wed 08 Mar 10:03 PST 2017, Avaneesh Kumar Dwivedi wrote:
>
>> This patch add scm call support to make hypervisor call to enable access
>> of fw regions in ddr to mss subsystem on arm-v8 arch soc's.
>>
>> Signed-off-by: Avaneesh Kumar Dwivedi <[email protected]>
>> ---
>> drivers/firmware/qcom_scm-64.c | 25 +++++++
>> drivers/firmware/qcom_scm.c | 93 ++++++++++++++++++++++++++
>> drivers/firmware/qcom_scm.h | 3 +
>> drivers/remoteproc/qcom_q6v5_pil.c | 129 ++++++++++++++++++++++++++++++++++++-
>> include/linux/qcom_scm.h | 14 ++++
>> 5 files changed, 262 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
>> index 4a0f5ea..187fc00 100644
>> --- a/drivers/firmware/qcom_scm-64.c
>> +++ b/drivers/firmware/qcom_scm-64.c
>> @@ -358,3 +358,28 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool reset)
>>
>> return ret ? : res.a1;
>> }
>> +
>> +int __qcom_scm_assign_mem(struct device *dev, struct vmid_detail vmid)
> Rather than packing these parameters up in a struct I think it's cleaner
> to just pass them directly.
passing so many parameters directly is not seen good practice specially
when number of parameters to pass are many.
that is why i used struct copy, i did not use reference of struct since
values passed were not going to be modified.
but will surely look if i can work with direct passing of params.
>
>> +{
>> + int ret;
>> + struct qcom_scm_desc desc = {0};
>> + struct arm_smccc_res res;
>> +
>> + desc.args[0] = vmid.fw_phy;
>> + desc.args[1] = vmid.size_fw;
>> + desc.args[2] = vmid.from_phy;
>> + desc.args[3] = vmid.size_from;
>> + desc.args[4] = vmid.to_phy;
>> + desc.args[5] = vmid.size_to;
>> + desc.args[6] = 0;
>> +
>> + desc.arginfo = QCOM_SCM_ARGS(7, QCOM_SCM_RO, QCOM_SCM_VAL,
>> + QCOM_SCM_RO, QCOM_SCM_VAL, QCOM_SCM_RO,
>> + QCOM_SCM_VAL, QCOM_SCM_VAL);
>> +
>> + ret = qcom_scm_call(dev, QCOM_SCM_SVC_MP,
>> + QCOM_MEM_PROT_ASSIGN_ID,
>> + &desc, &res);
>> +
>> + return ret ? : res.a1;
> If I understand the downstream code we only care about "ret" here; being
> zero on success or negative on error.
sure, will check.
>> +}
>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>> index 893f953ea..f137f34 100644
>> --- a/drivers/firmware/qcom_scm.c
>> +++ b/drivers/firmware/qcom_scm.c
>> @@ -42,6 +42,18 @@ struct qcom_scm {
>>
>> static struct qcom_scm *__scm;
>>
>> +struct dest_vm_and_perm_info {
>> + __le32 vm;
>> + __le32 perm;
>> + __le32 *ctx;
> Please be explicit about the fact that this is 64 bit.
sorry i am not sure why they need to be 64 bit variable?
>
>> + __le32 ctx_size;
>> +};
> This should be __packed
Ok.
>
>> +
>> +struct fw_region_info {
>> + __le64 addr;
>> + __le64 size;
>> +};
>> +
>> static int qcom_scm_clk_enable(void)
>> {
>> int ret;
>> @@ -292,6 +304,87 @@ int qcom_scm_pas_shutdown(u32 peripheral)
>> }
>> EXPORT_SYMBOL(qcom_scm_pas_shutdown);
>>
>> +/**
>> + * qcom_scm_assign_mem() - Allocate and fill vmid detail of old
>> + * new owners of memory region for fw and metadata etc, Which is
>> + * further passed to hypervisor, which does translate intermediate
>> + * physical address used by subsystems.
>> + * @vmid: structure with pointers and size detail of old and new
>> + * owners vmid detail.
>> + * Return 0 on success.
>> + */
>> +int qcom_scm_assign_mem(struct vmid_detail vmid)
> After a long discussion with Stephen I now think that I understand
> what's actually going on here.
>
> So this function will request TZ to remove all permissions for the
> memory region in the tables specified in "from" and then for each vmid
> in "to" it will set up the specified "permission".
>
> So the "to" and "permissions" are actually a tuple, rather than
> independent lists of values. So I think this should be exposed in the
> prototype, as a list of <vmid, permission> entries.
>
> To make the function prototype more convenient I think you should turn
> "from" into a bitmap (e.g. BIT(VMID_HLOS) | BIT(VMID_MSS_MSA)).
i am not able to fully comprehend advantage of turning "from" into bitmap.
moreover when i read your below suggestion, which suggest to use "struct
qcom_scm_mem_perm" as below then i get further confused what i will do
with bitmap?
>
> If you then make the function, on success, return "to" as a bitmap the
> caller can simply store that in a state variable and pass it as "from"
> in the next call.
>
> So you would have:
>
> struct qcom_scm_mem_perm new_perms[] = {
> { VMID_HLOS, PERM_READ },
> { VMID_MSS_MSA, PREM_READ | PERM_WRITE },
> };
>
> current_perms = qcom_scm_assign_mem(ptr, size, current_perms, new_perms, 2);
This looks OK, will try to incorporate this idea.
>
>
> And I believe something like "curr_perm" and "new_perm" are even better
> names than "from" and "to"
ok.
>
>> +{
>> + unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
>> + struct dest_vm_and_perm_info *to;
>> + struct fw_region_info *fw_info;
>> + __le64 fw_phy;
>> + __le32 *from;
>> + int ret = -ENOMEM;
>> + int i;
>> +
>> + from = dma_alloc_attrs(__scm->dev, vmid.size_from,
>> + &vmid.from_phy, GFP_KERNEL, dma_attrs);
>> + if (!from) {
>> + dev_err(__scm->dev,
>> + "failed to allocate buffer to pass source vmid detail\n");
>> + return -ENOMEM;
>> + }
>> + to = dma_alloc_attrs(__scm->dev, vmid.size_to,
>> + &vmid.to_phy, GFP_KERNEL, dma_attrs);
>> + if (!to) {
>> + dev_err(__scm->dev,
>> + "failed to allocate buffer to pass destination vmid detail\n");
>> + goto free_src_buff;
>> + }
>> + fw_info = dma_alloc_attrs(__scm->dev, sizeof(*fw_info),
>> + &fw_phy, GFP_KERNEL, dma_attrs);
>> + if (!fw_info) {
>> + dev_err(__scm->dev,
>> + "failed to allocate buffer to pass firmware detail\n");
>> + goto free_dest_buff;
>> + }
> You should be able to allocate these in one chunk and you should be able
> to just use dma_alloc_coherent().
OK.
>
>> +
>> + /* copy detail of original owner of ddr region */
>> + /* in physically contigious buffer */
>> + memcpy(from, vmid.from, vmid.size_from);
> With "from" as a bitmap see hweight_long() to figure out the size of
> "from".
hweight_long() returns number of bits set in bitmap? is this correct understanding?
>
>> +
>> + /* fill details of new owners of ddr region*/
>> + /* in physically contigious buffer */
>> + for (i = 0; i < (vmid.size_to / sizeof(__le32)); i++) {
> Better pass "number of entries in 'to'" and use standard types (such as
> int or unsigned int) for "to" and "permission" until this point.
OK.
>
>> + to[i].vm = vmid.to[i];
>> + to[i].perm = vmid.permission[i];
>> + to[i].ctx = NULL;
>> + to[i].ctx_size = 0;
>> + }
>> +
>> + /* copy detail of firmware region whose mapping need to be done */
>> + /* in physically contigious buffer */
>> + fw_info->addr = vmid.fw_phy;
>> + fw_info->size = vmid.size_fw;
> fw_info is a confusing name for "a list of memory regions".
ok will try to use appropriate name.
>
> And you need a cpu_to_le32() somewhere, better keep the in-kernel API
> pass standard types (such as phys_addr_t) and then convert to the
> "hardware specific" type here.
ok.
>
>> +
>> + /* reuse fw_phy and size_fw members to fill address and size of */
>> + /* fw_info buffer */
> Please don't try to be "clever" and reuse things when writing
> kernel-code, non-obvious reuse of variables or struct members often end
> up causing someone else to introduce bugs in your code later.
ok.
>
>> + vmid.fw_phy = fw_phy;
>> + vmid.size_to = sizeof(*to) * (vmid.size_to / sizeof(__le32));
>> + vmid.size_fw = sizeof(*fw_info);
>> + ret = __qcom_scm_assign_mem(__scm->dev, vmid);
>> + if (!ret)
>> + goto free_fw_buff;
>> + return ret;
>> +free_fw_buff:
>> + dma_free_attrs(__scm->dev, sizeof(*fw_info), fw_info,
>> + fw_phy, dma_attrs);
>> +free_dest_buff:
>> + dma_free_attrs(__scm->dev, vmid.size_to, to,
>> + vmid.to_phy, dma_attrs);
>> +free_src_buff:
>> + dma_free_attrs(__scm->dev, vmid.size_from, from,
>> + vmid.from_phy, dma_attrs);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(qcom_scm_assign_mem);
>> +
>> static int qcom_scm_pas_reset_assert(struct reset_controller_dev *rcdev,
>> unsigned long idx)
>> {
>> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
>> index 3584b00..4665a11 100644
>> --- a/drivers/firmware/qcom_scm.h
>> +++ b/drivers/firmware/qcom_scm.h
>> @@ -55,6 +55,9 @@ extern int __qcom_scm_pas_mem_setup(struct device *dev, u32 peripheral,
>> extern int __qcom_scm_pas_auth_and_reset(struct device *dev, u32 peripheral);
>> extern int __qcom_scm_pas_shutdown(struct device *dev, u32 peripheral);
>> extern int __qcom_scm_pas_mss_reset(struct device *dev, bool reset);
>> +#define QCOM_SCM_SVC_MP 0xc
>> +#define QCOM_MEM_PROT_ASSIGN_ID 0x16
>> +extern int __qcom_scm_assign_mem(struct device *dev, struct vmid_detail vmid);
>>
>> /* common error codes */
>> #define QCOM_SCM_V2_EBUSY -12
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>> index 8fd697a..62ad976 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pil.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> Please split the patch in a scm-patch and a q6v5 patch.
ok.
>
>> @@ -110,6 +110,7 @@ struct rproc_hexagon_res {
>> struct qcom_mss_reg_res *active_supply;
>> char **proxy_clk_names;
>> char **active_clk_names;
>> + int version;
> This will result in multi-line conditionals checking if we should do
> memory protection for platform x, y, z or not. Add a "bool
> need_mem_protect;" instead.
ok.
>
>> };
>>
>> struct q6v5 {
>> @@ -153,8 +154,28 @@ struct q6v5 {
>> size_t mpss_size;
>>
>> struct qcom_rproc_subdev smd_subdev;
>> + int version;
>> };
>>
>> +enum {
>> + MSS_MSM8916,
>> + MSS_MSM8974,
>> + MSS_MSM8996,
>> +};
>> +
>> +enum {
>> + ASSIGN_ACCESS_MSA,
>> + REMOVE_ACCESS_MSA,
>> + ASSIGN_SHARED_ACCESS_MSA,
>> + REMOVE_SHARED_ACCESS_MSA,
>> +};
>> +
>> +#define VMID_HLOS 0x3
>> +#define VMID_MSS_MSA 0xF
>> +#define PERM_READ 0x4
>> +#define PERM_WRITE 0x2
>> +#define PERM_EXEC 0x1
>> +#define PERM_RW (PERM_READ | PERM_WRITE)
> These belongs in the SCM API instead. (Prefix them appropriately)
Do you mean something like SCM_VMID_HLOS?
>
>> static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
>> const struct qcom_mss_reg_res *reg_res)
>> {
>> @@ -288,6 +309,54 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,
>> return &table;
>> }
>>
>> +int hyp_mem_access(int id, phys_addr_t addr, size_t size)
> I much prefer the downstream split and naming of this function, rather
> than using a switch to implement 4 different functions in one please
> split it up.
so you mean i need to have seprate function for each "int id"? in switch
case?
just for sake of minimum code churn i tried to work with single function.
>
>> +{
>> + struct vmid_detail vmid;
>> + int ret;
>> +
> Pass the struct q6v5 here and return successfully here if we're not
> supposed to do memory protection. It does clean up the calling code.
OK.
>
>> + switch (id) {
>> + case ASSIGN_ACCESS_MSA:
>> + vmid.from = (int[]) { VMID_HLOS };
>> + vmid.to = (int[]) { VMID_MSS_MSA };
>> + vmid.permission = (int[]) { PERM_READ | PERM_WRITE };
>> + vmid.size_from = vmid.size_to = 1 * sizeof(__le32);
>> + break;
>> + case REMOVE_ACCESS_MSA:
>> + vmid.from = (int[]) { VMID_MSS_MSA };
>> + vmid.to = (int[]) { VMID_HLOS };
>> + vmid.permission =
>> + (int[]) { PERM_READ | PERM_WRITE | PERM_EXEC };
>> + vmid.size_from = vmid.size_to = 1 * sizeof(__le32);
>> + break;
>> + case ASSIGN_SHARED_ACCESS_MSA:
>> + vmid.from = (int[]) { VMID_HLOS };
>> + vmid.to = (int[]) { VMID_HLOS, VMID_MSS_MSA };
>> + vmid.permission = (int[]) { PERM_RW, PERM_RW };
>> + vmid.size_from = 1 * sizeof(__le32);
>> + vmid.size_to = 2 * sizeof(__le32);
>> + break;
>> + case REMOVE_SHARED_ACCESS_MSA:
>> + vmid.from = (int[]) { VMID_HLOS, VMID_MSS_MSA };
>> + vmid.to = (int[]) { VMID_HLOS };
>> + vmid.permission =
>> + (int[]) { PERM_READ | PERM_WRITE | PERM_EXEC };
>> + vmid.size_from = 2 * sizeof(__le32);
>> + vmid.size_to = 1 * sizeof(__le32);
>> + break;
>> + default:
>> + break;
>> + }
>> +
>> + vmid.fw_phy = addr;
>> + vmid.size_fw = size;
>> + ret = qcom_scm_assign_mem(vmid);
>> + if (ret)
>> + pr_err("%s: Failed to assign memory protection, ret = %d\n",
>> + __func__, ret);
>> +
>> + return ret;
>> +}
>> +
>> static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
>> {
>> struct q6v5 *qproc = rproc->priv;
>> @@ -461,6 +530,15 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
>>
>> memcpy(ptr, fw->data, fw->size);
>>
>> + /* Hypervisor mapping to access metadata by modem */
>> + ret = qproc->version == MSS_MSM8996 ?
> To prove the point above, imagine just in the next year when this
I agree.
> becomes:
>
> ret = (qproc->version == MSS_MSM8996 ||
> qproc->version == MSS_MSM8998 ||
> qproc->version == MSS_NEXT_THING) ?
> hyp_mem_access(ASSIGN_ACCESS_MSA, phys, ALIGN(fw->size, SZ_4K)) :
> 0;
>
> Or in 5 years...
>
>> + hyp_mem_access(ASSIGN_ACCESS_MSA, phys,
>> + ALIGN(fw->size, SZ_4K)) : 0;
>> + if (ret) {
>> + dev_err(qproc->dev,
>> + "Failed to assign metadata memory, ret - %d\n", ret);
> Rather than having this print after each call to hyp_mem_access() move
> the message into that function.
Ok.
>
>> + return -ENOMEM;
> And don't forget to clean up the allocation before returning.
Ok.
>
>> + }
>> writel(phys, qproc->rmb_base + RMB_PMI_META_DATA_REG);
>> writel(RMB_CMD_META_DATA_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
>>
>> @@ -470,6 +548,13 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
>> else if (ret < 0)
>> dev_err(qproc->dev, "MPSS header authentication failed: %d\n", ret);
>>
>> + /* Metadata authentication done, remove modem access */
>> + ret = qproc->version == MSS_MSM8996 ?
>> + hyp_mem_access(REMOVE_ACCESS_MSA, phys,
>> + ALIGN(fw->size, SZ_4K)) : 0;
> You use ALIGN(fw->size, SZ_4K) numerous times in this function, use a
> variable and make the allocation take this as well to make it explicit
> that we're dealing with the same memory chunk size all over.
OK.
>
>> + if (ret)
>> + dev_err(qproc->dev,
>> + "Failed to reclaim metadata memory, ret - %d\n", ret);
>> dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs);
>>
>> return ret < 0 ? ret : 0;
>> @@ -578,6 +663,16 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>> size = readl(qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
>> if (!size) {
>> boot_addr = relocate ? qproc->mpss_phys : min_addr;
>> + /* Hypervisor mapping of modem fw */
>> + ret = qproc->version == MSS_MSM8996 ?
>> + hyp_mem_access(ASSIGN_SHARED_ACCESS_MSA,
>> + boot_addr, qproc->mpss_size) : 0;
> The mpss is loaded somewhere in qproc->mpss_region, potentially at some
> offset (which would be reflected in boot_addr).
>
> But if boot_addr > qproc->mpss_region the region to hand out is no
> longer mpss_size large, it's "mpss_size - (boot_addr - mpss_phys)", so
> you might give the mpss permission to trash the memory after the
> mpss_region.
>
> Better hand out qproc->mpss_phys of size qproc->mpss_size.
OK.
>
>
> And as far as I can tell the downstream driver load the segments and
> then make the MSS sole owner of the memory region. Do note that it's
> perfectly fine to refactor code to make things better fit a natural and
> easy to follow flow of ownership here!
Not very clear what to do?
>
>> + if (ret) {
>> + dev_err(qproc->dev,
>> + "Failed to assign fw memory access, ret - %d\n",
>> + ret);
>> + return -ENOMEM;
>> + }
>> writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG);
>> writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
>> }
>> @@ -636,6 +731,14 @@ static int q6v5_start(struct rproc *rproc)
>> goto assert_reset;
>> }
>>
>> + ret = qproc->version == MSS_MSM8996 ?
>> + hyp_mem_access(ASSIGN_ACCESS_MSA, qproc->mba_phys,
>> + qproc->mba_size) : 0;
>> + if (ret) {
>> + dev_err(qproc->dev,
>> + "Failed to assign mba memory access, ret - %d\n", ret);
>> + goto assert_reset;
>> + }
>> writel(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG);
>>
>> ret = q6v5proc_reset(qproc);
>> @@ -657,16 +760,22 @@ static int q6v5_start(struct rproc *rproc)
>>
>> ret = q6v5_mpss_load(qproc);
>> if (ret)
>> - goto halt_axi_ports;
>> + goto reclaim_mem;
>>
>> ret = wait_for_completion_timeout(&qproc->start_done,
>> msecs_to_jiffies(5000));
>> if (ret == 0) {
>> dev_err(qproc->dev, "start timed out\n");
>> ret = -ETIMEDOUT;
>> - goto halt_axi_ports;
>> + goto reclaim_mem;
>> }
>>
>> + ret = qproc->version == MSS_MSM8996 ?
>> + hyp_mem_access(REMOVE_ACCESS_MSA, qproc->mba_phys,
>> + qproc->mba_size) : 0;
>> + if (ret)
>> + dev_err(qproc->dev,
>> + "Failed to reclaim mba memory, ret - %d\n", ret);
>> qproc->running = true;
>>
>> q6v5_clk_disable(qproc->dev, qproc->proxy_clks,
>> @@ -676,7 +785,20 @@ static int q6v5_start(struct rproc *rproc)
>>
> On success q6v5_start() will leave Linus as sole owner of mba_phys and
> mpss_phys (or boot_addr) will have shared ownership. rproc_stop() should
> make sure to reclaim the sole ownership of mpss_phys again, so that
> subsequent calls to rproc_start() will find the memory protection in an
> appropriate state.
Sure!
>
>> return 0;
>>
>> +reclaim_mem:
>> + ret = qproc->version == MSS_MSM8996 ?
>> + hyp_mem_access(REMOVE_SHARED_ACCESS_MSA,
>> + qproc->mpss_phys, qproc->mpss_size) : 0;
>> + if (ret)
>> + dev_err(qproc->dev,
>> + "Failed to reclaim fw memory, ret - %d\n", ret);
>> halt_axi_ports:
>> + ret = qproc->version == MSS_MSM8996 ?
>> + hyp_mem_access(REMOVE_ACCESS_MSA, qproc->mba_phys,
>> + qproc->mba_size) : 0;
>> + if (ret)
>> + dev_err(qproc->dev,
>> + "Failed to reclaim mba memory, ret - %d\n", ret);
> Isn't it possible that the remote processor is still executing the MBA
> firmware up until we halt the axi ports and assert the reset? We don't
> want to remove permission until we know the CPU is stopped and it wont
> fault on us.
Will check.
>
>> q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
>> q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
>> q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
>> @@ -1015,6 +1137,7 @@ static int q6v5_probe(struct platform_device *pdev)
>> if (ret)
>> goto free_rproc;
>>
>> + qproc->version = desc->version;
>> ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt);
>> if (ret < 0)
>> goto free_rproc;
>> @@ -1090,6 +1213,7 @@ static int q6v5_remove(struct platform_device *pdev)
>> "mem",
>> NULL
>> },
>> + .version = MSS_MSM8916,
>> };
>>
>> static const struct rproc_hexagon_res msm8974_mss = {
>> @@ -1127,6 +1251,7 @@ static int q6v5_remove(struct platform_device *pdev)
>> "mem",
>> NULL
>> },
>> + .version = MSS_MSM8974,
>> };
>>
>> static const struct of_device_id q6v5_of_match[] = {
>> diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
>> index cc32ab8..cb0b7ee 100644
>> --- a/include/linux/qcom_scm.h
>> +++ b/include/linux/qcom_scm.h
>> @@ -23,6 +23,19 @@ struct qcom_scm_hdcp_req {
>> u32 val;
>> };
>>
>> +struct vmid_detail {
> Whenever you add structs to a public API, make sure to add some
> kerneldoc.
Will check if where is it appropriate to provide documentation.
>
> And as this is a kernel-API I would like to see standard kernel-types
> for things.
OK.
>
>> + __le32 *from;
>> + __le32 *to;
>> + __le32 *permission;
>> + __le32 size_from;
>> + __le32 size_to;
>> + __le32 size_fw;
>> + __le64 fw_phy;
>> + __le64 from_phy;
>> + __le64 to_phy;
>> +
>> +};
>> +
> Regards,
> Bjorn
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
On 4/6/2017 5:14 AM, Stephen Boyd wrote:
> On 03/08, Avaneesh Kumar Dwivedi wrote:
>> diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
>> index 4a0f5ea..187fc00 100644
>> --- a/drivers/firmware/qcom_scm-64.c
>> +++ b/drivers/firmware/qcom_scm-64.c
>> @@ -358,3 +358,28 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool reset)
>>
>> return ret ? : res.a1;
>> }
>> +
>> +int __qcom_scm_assign_mem(struct device *dev, struct vmid_detail vmid)
> Why are we passing a structure copy?
I did not use struct pointer cause i am not modifying any of the passed
value, do you think i should use struct pointer than pass by value?
>
>> +{
>> + int ret;
>> + struct qcom_scm_desc desc = {0};
>> + struct arm_smccc_res res;
>> +
>> + desc.args[0] = vmid.fw_phy;
>> + desc.args[1] = vmid.size_fw;
>> + desc.args[2] = vmid.from_phy;
>> + desc.args[3] = vmid.size_from;
>> + desc.args[4] = vmid.to_phy;
>> + desc.args[5] = vmid.size_to;
> These should all cause sparse warnings because of incorrect
> assignments. Given that these are all registers, I'm not sure why
> the vmid_detail structure has __le32 in it.
will fix.
>
>> + desc.args[6] = 0;
>> +
>> + desc.arginfo = QCOM_SCM_ARGS(7, QCOM_SCM_RO, QCOM_SCM_VAL,
>> + QCOM_SCM_RO, QCOM_SCM_VAL, QCOM_SCM_RO,
>> + QCOM_SCM_VAL, QCOM_SCM_VAL);
>> +
>> + ret = qcom_scm_call(dev, QCOM_SCM_SVC_MP,
>> + QCOM_MEM_PROT_ASSIGN_ID,
>> + &desc, &res);
>> +
>> + return ret ? : res.a1;
>> +}
>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>> index 893f953ea..f137f34 100644
>> --- a/drivers/firmware/qcom_scm.c
>> +++ b/drivers/firmware/qcom_scm.c
>> @@ -42,6 +42,18 @@ struct qcom_scm {
>>
>> static struct qcom_scm *__scm;
>>
>> +struct dest_vm_and_perm_info {
>> + __le32 vm;
>> + __le32 perm;
>> + __le32 *ctx;
> Drop the pointer? Just treat it like another number? Pointer is
> really odd because it doesn't really make any sense what the
> address of the pointer would be.
Downstream this is pointer though unused, that is why kept same will
check and change.
>
>> + __le32 ctx_size;
>> +};
>> +
>> +struct fw_region_info {
>> + __le64 addr;
>> + __le64 size;
>> +};
>> +
>> static int qcom_scm_clk_enable(void)
>> {
>> int ret;
>> @@ -292,6 +304,87 @@ int qcom_scm_pas_shutdown(u32 peripheral)
>> }
>> EXPORT_SYMBOL(qcom_scm_pas_shutdown);
>>
>> +/**
>> + * qcom_scm_assign_mem() - Allocate and fill vmid detail of old
>> + * new owners of memory region for fw and metadata etc, Which is
>> + * further passed to hypervisor, which does translate intermediate
>> + * physical address used by subsystems.
> Maybe this can be the long description, but the short description
> should be shorter.
OK :(
>
>> + * @vmid: structure with pointers and size detail of old and new
>> + * owners vmid detail.
>> + * Return 0 on success.
> There's a standard syntax for return too. Look at kernel doc
> howto please.
OK.
>
>> + */
>> +int qcom_scm_assign_mem(struct vmid_detail vmid)
> Please no structure copy.
should struct pointer be OK, or direct argument passing?
>
>> +{
>> + unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
>> + struct dest_vm_and_perm_info *to;
>> + struct fw_region_info *fw_info;
>> + __le64 fw_phy;
>> + __le32 *from;
>> + int ret = -ENOMEM;
> Not sure why we assign it. It gets overwritten with the first use.
Yes will correct.
>
>> + int i;
>> +
>> + from = dma_alloc_attrs(__scm->dev, vmid.size_from,
>> + &vmid.from_phy, GFP_KERNEL, dma_attrs);
>> + if (!from) {
>> + dev_err(__scm->dev,
>> + "failed to allocate buffer to pass source vmid detail\n");
>> + return -ENOMEM;
>> + }
>> + to = dma_alloc_attrs(__scm->dev, vmid.size_to,
>> + &vmid.to_phy, GFP_KERNEL, dma_attrs);
>> + if (!to) {
>> + dev_err(__scm->dev,
>> + "failed to allocate buffer to pass destination vmid detail\n");
>> + goto free_src_buff;
>> + }
>> + fw_info = dma_alloc_attrs(__scm->dev, sizeof(*fw_info),
>> + &fw_phy, GFP_KERNEL, dma_attrs);
> Can we consolidate this into one allocation with the appropriate
> size and then offset the different structures in it?
OK.
>> + if (!fw_info) {
>> + dev_err(__scm->dev,
>> + "failed to allocate buffer to pass firmware detail\n");
>> + goto free_dest_buff;
>> + }
>> +
>> + /* copy detail of original owner of ddr region */
>> + /* in physically contigious buffer */
>> + memcpy(from, vmid.from, vmid.size_from);
>> +
>> + /* fill details of new owners of ddr region*/
>> + /* in physically contigious buffer */
>> + for (i = 0; i < (vmid.size_to / sizeof(__le32)); i++) {
>> + to[i].vm = vmid.to[i];
>> + to[i].perm = vmid.permission[i];
> Here you want the cpu_to_le32() stuff. Please run sparse.
OK, last time i tried running sparse but seems some environment problem,
it did not gave any warning.
>
>> + to[i].ctx = NULL;
>> + to[i].ctx_size = 0;
>> + }
>> +
>> + /* copy detail of firmware region whose mapping need to be done */
>> + /* in physically contigious buffer */
> /*
> * Multi-line comments are like so.
> */
Will change.
>
>> + fw_info->addr = vmid.fw_phy;
>> + fw_info->size = vmid.size_fw;
>> +
>> + /* reuse fw_phy and size_fw members to fill address and size of */
>> + /* fw_info buffer */
>> + vmid.fw_phy = fw_phy;
>> + vmid.size_to = sizeof(*to) * (vmid.size_to / sizeof(__le32));
>> + vmid.size_fw = sizeof(*fw_info);
>> + ret = __qcom_scm_assign_mem(__scm->dev, vmid);
>> + if (!ret)
>> + goto free_fw_buff;
>> + return ret;
> We don't free dma on failure?
Did not get, isnt i am freeing all dma allocs on failure?
>
>> +free_fw_buff:
>> + dma_free_attrs(__scm->dev, sizeof(*fw_info), fw_info,
>> + fw_phy, dma_attrs);
>> +free_dest_buff:
>> + dma_free_attrs(__scm->dev, vmid.size_to, to,
>> + vmid.to_phy, dma_attrs);
>> +free_src_buff:
>> + dma_free_attrs(__scm->dev, vmid.size_from, from,
>> + vmid.from_phy, dma_attrs);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(qcom_scm_assign_mem);
>> +
>> static int qcom_scm_pas_reset_assert(struct reset_controller_dev *rcdev,
>> unsigned long idx)
>> {
>> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
>> index 3584b00..4665a11 100644
>> --- a/drivers/firmware/qcom_scm.h
>> +++ b/drivers/firmware/qcom_scm.h
>> @@ -55,6 +55,9 @@ extern int __qcom_scm_pas_mem_setup(struct device *dev, u32 peripheral,
>> extern int __qcom_scm_pas_auth_and_reset(struct device *dev, u32 peripheral);
>> extern int __qcom_scm_pas_shutdown(struct device *dev, u32 peripheral);
>> extern int __qcom_scm_pas_mss_reset(struct device *dev, bool reset);
>> +#define QCOM_SCM_SVC_MP 0xc
> This is already defined upstream?
Will check, but i thought its not there.
>
>> +#define QCOM_MEM_PROT_ASSIGN_ID 0x16
>> +extern int __qcom_scm_assign_mem(struct device *dev, struct vmid_detail vmid);
>>
>> /* common error codes */
>> #define QCOM_SCM_V2_EBUSY -12
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>> index 8fd697a..62ad976 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pil.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
>> @@ -288,6 +309,54 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,
>> return &table;
>> }
>>
>> +int hyp_mem_access(int id, phys_addr_t addr, size_t size)
> static?
Yes, will correct.
>
>>
>> static const struct of_device_id q6v5_of_match[] = {
>> diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
>> index cc32ab8..cb0b7ee 100644
>> --- a/include/linux/qcom_scm.h
>> +++ b/include/linux/qcom_scm.h
>> @@ -23,6 +23,19 @@ struct qcom_scm_hdcp_req {
>> u32 val;
>> };
>>
>> +struct vmid_detail {
>> + __le32 *from;
>> + __le32 *to;
>> + __le32 *permission;
>> + __le32 size_from;
>> + __le32 size_to;
>> + __le32 size_fw;
>> + __le64 fw_phy;
>> + __le64 from_phy;
>> + __le64 to_phy;
> should the *_phy be phys_addr_t types?
>
> Leave these all as u32/u64. Perhaps also move size_from/size_to
> next to the arrays they're for. Also add some documentation so we
> know what they're all about.
OK.
>
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
On Fri 14 Apr 06:54 PDT 2017, Dwivedi, Avaneesh Kumar (avani) wrote:
> On 4/6/2017 10:13 AM, Bjorn Andersson wrote:
> > On Wed 08 Mar 10:03 PST 2017, Avaneesh Kumar Dwivedi wrote:
[..]
> > > +int __qcom_scm_assign_mem(struct device *dev, struct vmid_detail vmid)
> > Rather than packing these parameters up in a struct I think it's cleaner
> > to just pass them directly.
> passing so many parameters directly is not seen good practice specially when
> number of parameters to pass are many.
> that is why i used struct copy, i did not use reference of struct since
> values passed were not going to be modified.
> but will surely look if i can work with direct passing of params.
It's a good rule of thumb, but in this case you have 1 caller and 2
implementations (don't forget to stub the 32-bit one), so the overhead
of having a struct just to pass these parameters probably is not worth
it.
And the fact that some of the members have slightly different meanings
here compared to the other API is a bigger issue.
> >
> > > +{
> > > + int ret;
> > > + struct qcom_scm_desc desc = {0};
> > > + struct arm_smccc_res res;
> > > +
> > > + desc.args[0] = vmid.fw_phy;
> > > + desc.args[1] = vmid.size_fw;
> > > + desc.args[2] = vmid.from_phy;
> > > + desc.args[3] = vmid.size_from;
> > > + desc.args[4] = vmid.to_phy;
> > > + desc.args[5] = vmid.size_to;
> > > + desc.args[6] = 0;
> > > +
[..]
> > > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> > > index 893f953ea..f137f34 100644
> > > --- a/drivers/firmware/qcom_scm.c
> > > +++ b/drivers/firmware/qcom_scm.c
> > > @@ -42,6 +42,18 @@ struct qcom_scm {
> > > static struct qcom_scm *__scm;
> > > +struct dest_vm_and_perm_info {
> > > + __le32 vm;
> > > + __le32 perm;
> > > + __le32 *ctx;
> > Please be explicit about the fact that this is 64 bit.
> sorry i am not sure why they need to be 64 bit variable?
__le32 says that elements are 32-bits, * says this is a pointer to such
elements and as we are on a 64-bit system this member is 64-bit.
This is a binary interface between the Linux kernel and the TrustZone
code, so the struct must be encoded in exactly the same way on both
sides.
As Stephen suggested, making this __le64 would make this way more
obvious - and endian safe.
> >
> > > + __le32 ctx_size;
> > > +};
[..]
> > > +int qcom_scm_assign_mem(struct vmid_detail vmid)
> > After a long discussion with Stephen I now think that I understand
> > what's actually going on here.
> >
> > So this function will request TZ to remove all permissions for the
> > memory region in the tables specified in "from" and then for each vmid
> > in "to" it will set up the specified "permission".
> >
> > So the "to" and "permissions" are actually a tuple, rather than
> > independent lists of values. So I think this should be exposed in the
> > prototype, as a list of <vmid, permission> entries.
> >
> > To make the function prototype more convenient I think you should turn
> > "from" into a bitmap (e.g. BIT(VMID_HLOS) | BIT(VMID_MSS_MSA)).
> i am not able to fully comprehend advantage of turning "from" into bitmap.
> moreover when i read your below suggestion, which suggest to use "struct
> qcom_scm_mem_perm" as below then i get further confused what i will do with
> bitmap?
Every time you call this function you have to pass a list of "from" that
corresponds to the "to" of the last (successful) call to this function.
In your code this means that the "from" is either 1 or 2 elements and as
you can see in your own code there is no convenient way of doing this in
C - having an array of 2 elements and claiming that it is either 1 or 2
entries long is not very clean.
Further more, in the remoteproc driver you currently rely on the fact
that you "know", in each code path, what the last "to" was and just hard
code "from". But if you make this call return the next "from" as a
bitmap (much more convenient than a variable length array) we can store
this in the driver for each memory region and I believe the calling code
will be cleaner.
> >
> > If you then make the function, on success, return "to" as a bitmap the
> > caller can simply store that in a state variable and pass it as "from"
> > in the next call.
> >
> > So you would have:
> >
> > struct qcom_scm_mem_perm new_perms[] = {
> > { VMID_HLOS, PERM_READ },
> > { VMID_MSS_MSA, PREM_READ | PERM_WRITE },
> > };
> > current_perms = qcom_scm_assign_mem(ptr, size, current_perms, new_perms, 2);
> This looks OK, will try to incorporate this idea.
> >
> >
> > And I believe something like "curr_perm" and "new_perm" are even better
> > names than "from" and "to"
> ok.
> >
[..]
> > > +
> > > + /* copy detail of original owner of ddr region */
> > > + /* in physically contigious buffer */
> > > + memcpy(from, vmid.from, vmid.size_from);
> > With "from" as a bitmap see hweight_long() to figure out the size of
> > "from".
>
> hweight_long() returns number of bits set in bitmap? is this correct understanding?
>
Correct.
[..]
> > > diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
[..]
> > > +
> > > +#define VMID_HLOS 0x3
> > > +#define VMID_MSS_MSA 0xF
> > > +#define PERM_READ 0x4
> > > +#define PERM_WRITE 0x2
> > > +#define PERM_EXEC 0x1
> > > +#define PERM_RW (PERM_READ | PERM_WRITE)
> > These belongs in the SCM API instead. (Prefix them appropriately)
> Do you mean something like SCM_VMID_HLOS?
This is part of the qcom-scm API, other defines there are of the form
QCOM_SCM_* so QCOM_SCM_VMID_HLOS should be ok.
> >
> > > static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
> > > const struct qcom_mss_reg_res *reg_res)
> > > {
> > > @@ -288,6 +309,54 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,
> > > return &table;
> > > }
> > > +int hyp_mem_access(int id, phys_addr_t addr, size_t size)
> > I much prefer the downstream split and naming of this function, rather
> > than using a switch to implement 4 different functions in one please
> > split it up.
> so you mean i need to have seprate function for each "int id"? in switch
> case?
Yes.
> just for sake of minimum code churn i tried to work with single function.
>
In terms of C-code you have 1 function, but conceptually you have 4 - so
any reasoning about what this function does fully depends on "id" as
well.
[..]
> >
> >
> > And as far as I can tell the downstream driver load the segments and
> > then make the MSS sole owner of the memory region. Do note that it's
> > perfectly fine to refactor code to make things better fit a natural and
> > easy to follow flow of ownership here!
> Not very clear what to do?
In order to hand over ownership completely we can no longer verify the
segments as we load them - we will have to do this in two steps.
So I believe this code has to be refactored to make this in two separate
steps - with the latter done after we transmit the ownership of the
memory to the remote.
Regards,
Bjorn
On Fri 14 Apr 07:01 PDT 2017, Dwivedi, Avaneesh Kumar (avani) wrote:
> On 4/6/2017 5:14 AM, Stephen Boyd wrote:
> > On 03/08, Avaneesh Kumar Dwivedi wrote:
[..]
> > > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> > > index 893f953ea..f137f34 100644
> > > --- a/drivers/firmware/qcom_scm.c
> > > +++ b/drivers/firmware/qcom_scm.c
> > > @@ -42,6 +42,18 @@ struct qcom_scm {
> > > static struct qcom_scm *__scm;
> > > +struct dest_vm_and_perm_info {
> > > + __le32 vm;
> > > + __le32 perm;
> > > + __le32 *ctx;
> > Drop the pointer? Just treat it like another number? Pointer is
> > really odd because it doesn't really make any sense what the
> > address of the pointer would be.
> Downstream this is pointer though unused, that is why kept same will check
> and change.
The problem is that the size of a pointer depends on which platform
you're on. Spelling out __le32 here is then just deceiving.
> >
> > > + __le32 ctx_size;
> > > +};
[..]
> > > +int qcom_scm_assign_mem(struct vmid_detail vmid)
[..]
> > > + ret = __qcom_scm_assign_mem(__scm->dev, vmid);
> > > + if (!ret)
> > > + goto free_fw_buff;
> > > + return ret;
> > We don't free dma on failure?
> Did not get, isnt i am freeing all dma allocs on failure?
In the event that __qcom_scm_assign_mem() returns non-zero your not
jumping to free_fw_buff and just returning "ret" without freeing
the memory.
[..]
> > > diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
> > > index 3584b00..4665a11 100644
> > > --- a/drivers/firmware/qcom_scm.h
> > > +++ b/drivers/firmware/qcom_scm.h
> > > @@ -55,6 +55,9 @@ extern int __qcom_scm_pas_mem_setup(struct device *dev, u32 peripheral,
> > > extern int __qcom_scm_pas_auth_and_reset(struct device *dev, u32 peripheral);
> > > extern int __qcom_scm_pas_shutdown(struct device *dev, u32 peripheral);
> > > extern int __qcom_scm_pas_mss_reset(struct device *dev, bool reset);
> > > +#define QCOM_SCM_SVC_MP 0xc
> > This is already defined upstream?
> Will check, but i thought its not there.
We use the qcom_scm-api to abstract these facts, so this is an internal
constant - as such it goes in drivers/firmware/qcom_scm.h - and it's
already defined.
Regards,
Bjorn