| Message ID | 1485773044-31489-3-git-send-email-akdwived@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 01/30, Avaneesh Kumar Dwivedi wrote: > This patch add hypervisor call support for second stage translation from > mss remoteproc driver, this is required so that modem on msm8996 which is > based on armv8 architecture can access DDR region where modem firmware > are loaded. > > Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org> > --- Most of this should be combined with patch 1 from this series. > drivers/remoteproc/qcom_q6v5_pil.c | 202 ++++++++++++++++++++++++++++++++++++- > 1 file changed, 198 insertions(+), 4 deletions(-) > > diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c > index e5edefa..35eee68 100644 > --- a/drivers/remoteproc/qcom_q6v5_pil.c > +++ b/drivers/remoteproc/qcom_q6v5_pil.c > @@ -93,6 +93,23 @@ > #define QDSS_BHS_ON BIT(21) > #define QDSS_LDO_BYP BIT(22) > > +struct dest_vm_and_perm_info { > + __le32 vm; > + __le32 perm; > + __le32 *ctx; > + __le32 ctx_size; > +}; > + > +struct mem_prot_info { > + __le64 addr; > + __le64 size; > +}; > + > +struct scm_desc { > + __le32 arginfo; > + __le64 args[10]; > +}; These are all firmware specific things that should live in scm code, not PIL code. > + > struct reg_info { > struct regulator *reg; > int uV; > @@ -111,6 +128,7 @@ struct rproc_hexagon_res { > struct qcom_mss_reg_res active_supply[2]; > char **proxy_clk_names; > char **active_clk_names; > + int version; > }; > > struct q6v5 { > @@ -152,8 +170,29 @@ struct q6v5 { > phys_addr_t mpss_reloc; > void *mpss_region; > size_t mpss_size; > + int version; > + int protection_cmd; > +}; > + > +enum { > + MSS_MSM8916, > + MSS_MSM8974, > + MSS_MSM8996, > }; > > +enum { > + ASSIG_ACCESS_MSA, > + REMOV_ACCESS_MSA, > + SHARE_ACCESS_MSA, > + REMOV_SHARE_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 (0x4 | 0x2) Please USE PERM_READ | PERM_WRITE here instead. > static int q6v5_regulator_init(struct device *dev, struct reg_info *regs, > const struct qcom_mss_reg_res *reg_res) > { > @@ -273,12 +312,123 @@ static void q6v5_clk_disable(struct device *dev, > clk_disable_unprepare(clks[i]); > } > > +int hyp_mem_access(struct q6v5 *qproc, phys_addr_t addr, size_t size) This could be the scm API for now. But instead of taking qproc, it would take the protection_cmd variable (which looks sort of like a state machine BTW). To be more generic, perhaps it should take the src vmids + num src vmids, dst vmids + num dst vmids, and protection flags (which looks to be same size as dst vmid array). If we can get the right SCM API here everything else should fall into place. > +{ > + unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS; > + struct dest_vm_and_perm_info *dest_info; > + struct mem_prot_info *mem_prot_info; > + struct scm_desc desc = {0}; > + __le32 *source_vm_copy; > + __le64 mem_prot_phy; > + int dest_count = 1; > + int src_count = 1; > + __le32 *perm = {0}; > + __le32 *dest = {0}; > + __le32 *src = {0}; src/dst seem like pretty confusing names. It makes it sound like a memcpy operation. Perhaps from/to is better? Or current/next. > + __le64 phys_src; > + __le64 phy_dest; > + int ret; > + int i; > + > + if (qproc->version != MSS_MSM8996) > + return 0; > + > + switch (qproc->protection_cmd) { > + case ASSIG_ACCESS_MSA: { > + src = (int[2]) {VMID_HLOS, 0}; > + dest = (int[2]) {VMID_MSS_MSA, 0}; > + perm = (int[2]) {PERM_READ | PERM_WRITE, 0}; Why have two element arrays when we're only using the first element? Relying on default src_count of 1 is very confusing. > + break; > + } > + case REMOV_ACCESS_MSA: { > + src = (int[2]) {VMID_MSS_MSA, 0}; > + dest = (int[2]) {VMID_HLOS, 0}; > + perm = (int[2]) {PERM_READ | PERM_WRITE | PERM_EXEC, 0}; Same here. > + break; > + } > + case SHARE_ACCESS_MSA: { > + src = (int[2]) {VMID_HLOS, 0}; > + dest = (int[2]) {VMID_HLOS, VMID_MSS_MSA}; > + perm = (int[2]) {PERM_RW, PERM_RW}; Please add spaces around curly braces like { this } > + dest_count = 2; > + break; > + } > + case REMOV_SHARE_MSA: { And write REMOVE_SHARE_MSA, ASSIGN_ACCESS_MSA, etc. instead of dropping letters. > + src = (int[2]) {VMID_HLOS, VMID_MSS_MSA}; > + dest = (int[2]) {VMID_HLOS, 0}; > + perm = (int[2]) {PERM_READ | PERM_WRITE | PERM_EXEC, 0}; > + src_count = 2; > + break; > + } > + default: { And also drop curly braces around case statements. > + break; > + } > + } > + > + source_vm_copy = dma_alloc_attrs(qproc->dev, This should really allocate from the scm->dev instead of qproc. We don't know if qproc has the same DMA attributes that the firmware has, but we know that we're trying to relay information to the firmware here, not the qproc. > + src_count*sizeof(*source_vm_copy), &phys_src, > + GFP_KERNEL, dma_attrs); > + if (!source_vm_copy) { > + dev_err(qproc->dev, > + "failed to allocate buffer to pass source vmid detail\n"); > + return -ENOMEM; > + } > + memcpy(source_vm_copy, src, sizeof(*source_vm_copy) * src_count); > + > + dest_info = dma_alloc_attrs(qproc->dev, > + dest_count*sizeof(*dest_info), &phy_dest, > + GFP_KERNEL, dma_attrs); > + if (!dest_info) { > + dev_err(qproc->dev, > + "failed to allocate buffer to pass destination vmid detail\n"); > + return -ENOMEM; > + } > + for (i = 0; i < dest_count; i++) { > + dest_info[i].vm = dest[i]; > + dest_info[i].perm = perm[i]; Needs to do a cpu_to_le32() somewhere. Please run sparse. > + dest_info[i].ctx = NULL; > + dest_info[i].ctx_size = 0; > + } Perhaps we should just allocate these first (one or two elements isn't a big difference) and then fill the details in directly. > + > + mem_prot_info = dma_alloc_attrs(qproc->dev, > + sizeof(*mem_prot_info), &mem_prot_phy, > + GFP_KERNEL, dma_attrs); > + if (!dest_info) { > + dev_err(qproc->dev, > + "failed to allocate buffer to pass protected mem detail\n"); > + return -ENOMEM; > + } > + mem_prot_info->addr = addr; > + mem_prot_info->size = size; > + > + desc.args[0] = mem_prot_phy; > + desc.args[1] = sizeof(*mem_prot_info); > + desc.args[2] = phys_src; > + desc.args[3] = sizeof(*source_vm_copy) * src_count; > + desc.args[4] = phy_dest; > + desc.args[5] = dest_count*sizeof(*dest_info); Please add spaces around '*'. Run checkpatch. > + desc.args[6] = 0; > + > + ret = qcom_scm_assign_mem(&desc); > + if (ret) > + pr_info("%s: Failed to assign memory protection, ret = %d\n", pr_err? dev_err? > + __func__, ret); > + > + /* SCM call has returned free up buffers passed to secure domain */ > + dma_free_attrs(qproc->dev, src_count*sizeof(*source_vm_copy), > + source_vm_copy, phys_src, dma_attrs); > + dma_free_attrs(qproc->dev, dest_count*sizeof(*dest_info), > + dest_info, phy_dest, dma_attrs); > + dma_free_attrs(qproc->dev, sizeof(*mem_prot_info), mem_prot_info, > + mem_prot_phy, dma_attrs); > + return ret; > +} > + > static int q6v5_load(struct rproc *rproc, const struct firmware *fw) > { > struct q6v5 *qproc = rproc->priv; > > memcpy(qproc->mba_region, fw->data, fw->size); > - Please remove this patch noise. > return 0; > }
On 2/28/2017 12:46 PM, Stephen Boyd wrote: > On 01/30, Avaneesh Kumar Dwivedi wrote: >> This patch add hypervisor call support for second stage translation from >> mss remoteproc driver, this is required so that modem on msm8996 which is >> based on armv8 architecture can access DDR region where modem firmware >> are loaded. >> >> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org> >> --- > Most of this should be combined with patch 1 from this series. OK. > >> drivers/remoteproc/qcom_q6v5_pil.c | 202 ++++++++++++++++++++++++++++++++++++- >> 1 file changed, 198 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c >> index e5edefa..35eee68 100644 >> --- a/drivers/remoteproc/qcom_q6v5_pil.c >> +++ b/drivers/remoteproc/qcom_q6v5_pil.c >> @@ -93,6 +93,23 @@ >> #define QDSS_BHS_ON BIT(21) >> #define QDSS_LDO_BYP BIT(22) >> >> +struct dest_vm_and_perm_info { >> + __le32 vm; >> + __le32 perm; >> + __le32 *ctx; >> + __le32 ctx_size; >> +}; >> + >> +struct mem_prot_info { >> + __le64 addr; >> + __le64 size; >> +}; >> + >> +struct scm_desc { >> + __le32 arginfo; >> + __le64 args[10]; >> +}; > These are all firmware specific things that should live in scm > code, not PIL code. OK. > >> + >> struct reg_info { >> struct regulator *reg; >> int uV; >> @@ -111,6 +128,7 @@ struct rproc_hexagon_res { >> struct qcom_mss_reg_res active_supply[2]; >> char **proxy_clk_names; >> char **active_clk_names; >> + int version; >> }; >> >> struct q6v5 { >> @@ -152,8 +170,29 @@ struct q6v5 { >> phys_addr_t mpss_reloc; >> void *mpss_region; >> size_t mpss_size; >> + int version; >> + int protection_cmd; >> +}; >> + >> +enum { >> + MSS_MSM8916, >> + MSS_MSM8974, >> + MSS_MSM8996, >> }; >> >> +enum { >> + ASSIG_ACCESS_MSA, >> + REMOV_ACCESS_MSA, >> + SHARE_ACCESS_MSA, >> + REMOV_SHARE_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 (0x4 | 0x2) > Please USE PERM_READ | PERM_WRITE here instead. OK. > >> static int q6v5_regulator_init(struct device *dev, struct reg_info *regs, >> const struct qcom_mss_reg_res *reg_res) >> { >> @@ -273,12 +312,123 @@ static void q6v5_clk_disable(struct device *dev, >> clk_disable_unprepare(clks[i]); >> } >> >> +int hyp_mem_access(struct q6v5 *qproc, phys_addr_t addr, size_t size) > This could be the scm API for now. But instead of taking qproc, > it would take the protection_cmd variable (which looks sort of > like a state machine BTW). To be more generic, perhaps it should > take the src vmids + num src vmids, dst vmids + num dst vmids, > and protection flags (which looks to be same size as dst vmid > array). If we can get the right SCM API here everything else > should fall into place. Will analyses and modify as per suggestion. > >> +{ >> + unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS; >> + struct dest_vm_and_perm_info *dest_info; >> + struct mem_prot_info *mem_prot_info; >> + struct scm_desc desc = {0}; >> + __le32 *source_vm_copy; >> + __le64 mem_prot_phy; >> + int dest_count = 1; >> + int src_count = 1; >> + __le32 *perm = {0}; >> + __le32 *dest = {0}; >> + __le32 *src = {0}; > src/dst seem like pretty confusing names. It makes it sound like > a memcpy operation. Perhaps from/to is better? Or current/next. OK > >> + __le64 phys_src; >> + __le64 phy_dest; >> + int ret; >> + int i; >> + >> + if (qproc->version != MSS_MSM8996) >> + return 0; >> + >> + switch (qproc->protection_cmd) { >> + case ASSIG_ACCESS_MSA: { >> + src = (int[2]) {VMID_HLOS, 0}; >> + dest = (int[2]) {VMID_MSS_MSA, 0}; >> + perm = (int[2]) {PERM_READ | PERM_WRITE, 0}; > Why have two element arrays when we're only using the first > element? Relying on default src_count of 1 is very confusing. in some cases we initialize two elements. > >> + break; >> + } >> + case REMOV_ACCESS_MSA: { >> + src = (int[2]) {VMID_MSS_MSA, 0}; >> + dest = (int[2]) {VMID_HLOS, 0}; >> + perm = (int[2]) {PERM_READ | PERM_WRITE | PERM_EXEC, 0}; > Same here. OK. > >> + break; >> + } >> + case SHARE_ACCESS_MSA: { >> + src = (int[2]) {VMID_HLOS, 0}; >> + dest = (int[2]) {VMID_HLOS, VMID_MSS_MSA}; >> + perm = (int[2]) {PERM_RW, PERM_RW}; > Please add spaces around curly braces like { this } OK. > >> + dest_count = 2; >> + break; >> + } >> + case REMOV_SHARE_MSA: { > And write REMOVE_SHARE_MSA, ASSIGN_ACCESS_MSA, etc. instead of > dropping letters. OK. > >> + src = (int[2]) {VMID_HLOS, VMID_MSS_MSA}; >> + dest = (int[2]) {VMID_HLOS, 0}; >> + perm = (int[2]) {PERM_READ | PERM_WRITE | PERM_EXEC, 0}; >> + src_count = 2; >> + break; >> + } >> + default: { > And also drop curly braces around case statements. OK. > >> + break; >> + } >> + } >> + >> + source_vm_copy = dma_alloc_attrs(qproc->dev, > This should really allocate from the scm->dev instead of qproc. > We don't know if qproc has the same DMA attributes that the > firmware has, but we know that we're trying to relay information > to the firmware here, not the qproc. OK, agree. > >> + src_count*sizeof(*source_vm_copy), &phys_src, >> + GFP_KERNEL, dma_attrs); >> + if (!source_vm_copy) { >> + dev_err(qproc->dev, >> + "failed to allocate buffer to pass source vmid detail\n"); >> + return -ENOMEM; >> + } >> + memcpy(source_vm_copy, src, sizeof(*source_vm_copy) * src_count); >> + >> + dest_info = dma_alloc_attrs(qproc->dev, >> + dest_count*sizeof(*dest_info), &phy_dest, >> + GFP_KERNEL, dma_attrs); >> + if (!dest_info) { >> + dev_err(qproc->dev, >> + "failed to allocate buffer to pass destination vmid detail\n"); >> + return -ENOMEM; >> + } >> + for (i = 0; i < dest_count; i++) { >> + dest_info[i].vm = dest[i]; >> + dest_info[i].perm = perm[i]; > Needs to do a cpu_to_le32() somewhere. Please run sparse. I understand that byte reordering needed but not sure what is sparse will check and do it. > >> + dest_info[i].ctx = NULL; >> + dest_info[i].ctx_size = 0; >> + } > Perhaps we should just allocate these first (one or two elements > isn't a big difference) and then fill the details in directly. Not very clear what is ask here? > >> + >> + mem_prot_info = dma_alloc_attrs(qproc->dev, >> + sizeof(*mem_prot_info), &mem_prot_phy, >> + GFP_KERNEL, dma_attrs); >> + if (!dest_info) { >> + dev_err(qproc->dev, >> + "failed to allocate buffer to pass protected mem detail\n"); >> + return -ENOMEM; >> + } >> + mem_prot_info->addr = addr; >> + mem_prot_info->size = size; >> + >> + desc.args[0] = mem_prot_phy; >> + desc.args[1] = sizeof(*mem_prot_info); >> + desc.args[2] = phys_src; >> + desc.args[3] = sizeof(*source_vm_copy) * src_count; >> + desc.args[4] = phy_dest; >> + desc.args[5] = dest_count*sizeof(*dest_info); > Please add spaces around '*'. Run checkpatch. OK. > >> + desc.args[6] = 0; >> + >> + ret = qcom_scm_assign_mem(&desc); >> + if (ret) >> + pr_info("%s: Failed to assign memory protection, ret = %d\n", > pr_err? dev_err? Will correct. > >> + __func__, ret); >> + >> + /* SCM call has returned free up buffers passed to secure domain */ >> + dma_free_attrs(qproc->dev, src_count*sizeof(*source_vm_copy), >> + source_vm_copy, phys_src, dma_attrs); >> + dma_free_attrs(qproc->dev, dest_count*sizeof(*dest_info), >> + dest_info, phy_dest, dma_attrs); >> + dma_free_attrs(qproc->dev, sizeof(*mem_prot_info), mem_prot_info, >> + mem_prot_phy, dma_attrs); >> + return ret; >> +} >> + >> static int q6v5_load(struct rproc *rproc, const struct firmware *fw) >> { >> struct q6v5 *qproc = rproc->priv; >> >> memcpy(qproc->mba_region, fw->data, fw->size); >> - > Please remove this patch noise. OK. > >> return 0; >> }
On 03/03, Dwivedi, Avaneesh Kumar (avani) wrote: > On 2/28/2017 12:46 PM, Stephen Boyd wrote: > >On 01/30, Avaneesh Kumar Dwivedi wrote: > >>+ dest_info[i].vm = dest[i]; > >>+ dest_info[i].perm = perm[i]; > >Needs to do a cpu_to_le32() somewhere. Please run sparse. > I understand that byte reordering needed but not sure what is sparse > will check and do it. http://git.kernel.org/cgit/devel/sparse/sparse.git/ > > > >>+ dest_info[i].ctx = NULL; > >>+ dest_info[i].ctx_size = 0; > >>+ } > >Perhaps we should just allocate these first (one or two elements > >isn't a big difference) and then fill the details in directly. > Not very clear what is ask here? > > I'm saying we could fill in dest_info in the case statement instead of here in an up to 2 times loop.
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c index e5edefa..35eee68 100644 --- a/drivers/remoteproc/qcom_q6v5_pil.c +++ b/drivers/remoteproc/qcom_q6v5_pil.c @@ -93,6 +93,23 @@ #define QDSS_BHS_ON BIT(21) #define QDSS_LDO_BYP BIT(22) +struct dest_vm_and_perm_info { + __le32 vm; + __le32 perm; + __le32 *ctx; + __le32 ctx_size; +}; + +struct mem_prot_info { + __le64 addr; + __le64 size; +}; + +struct scm_desc { + __le32 arginfo; + __le64 args[10]; +}; + struct reg_info { struct regulator *reg; int uV; @@ -111,6 +128,7 @@ struct rproc_hexagon_res { struct qcom_mss_reg_res active_supply[2]; char **proxy_clk_names; char **active_clk_names; + int version; }; struct q6v5 { @@ -152,8 +170,29 @@ struct q6v5 { phys_addr_t mpss_reloc; void *mpss_region; size_t mpss_size; + int version; + int protection_cmd; +}; + +enum { + MSS_MSM8916, + MSS_MSM8974, + MSS_MSM8996, }; +enum { + ASSIG_ACCESS_MSA, + REMOV_ACCESS_MSA, + SHARE_ACCESS_MSA, + REMOV_SHARE_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 (0x4 | 0x2) static int q6v5_regulator_init(struct device *dev, struct reg_info *regs, const struct qcom_mss_reg_res *reg_res) { @@ -273,12 +312,123 @@ static void q6v5_clk_disable(struct device *dev, clk_disable_unprepare(clks[i]); } +int hyp_mem_access(struct q6v5 *qproc, phys_addr_t addr, size_t size) +{ + unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS; + struct dest_vm_and_perm_info *dest_info; + struct mem_prot_info *mem_prot_info; + struct scm_desc desc = {0}; + __le32 *source_vm_copy; + __le64 mem_prot_phy; + int dest_count = 1; + int src_count = 1; + __le32 *perm = {0}; + __le32 *dest = {0}; + __le32 *src = {0}; + __le64 phys_src; + __le64 phy_dest; + int ret; + int i; + + if (qproc->version != MSS_MSM8996) + return 0; + + switch (qproc->protection_cmd) { + case ASSIG_ACCESS_MSA: { + src = (int[2]) {VMID_HLOS, 0}; + dest = (int[2]) {VMID_MSS_MSA, 0}; + perm = (int[2]) {PERM_READ | PERM_WRITE, 0}; + break; + } + case REMOV_ACCESS_MSA: { + src = (int[2]) {VMID_MSS_MSA, 0}; + dest = (int[2]) {VMID_HLOS, 0}; + perm = (int[2]) {PERM_READ | PERM_WRITE | PERM_EXEC, 0}; + break; + } + case SHARE_ACCESS_MSA: { + src = (int[2]) {VMID_HLOS, 0}; + dest = (int[2]) {VMID_HLOS, VMID_MSS_MSA}; + perm = (int[2]) {PERM_RW, PERM_RW}; + dest_count = 2; + break; + } + case REMOV_SHARE_MSA: { + src = (int[2]) {VMID_HLOS, VMID_MSS_MSA}; + dest = (int[2]) {VMID_HLOS, 0}; + perm = (int[2]) {PERM_READ | PERM_WRITE | PERM_EXEC, 0}; + src_count = 2; + break; + } + default: { + break; + } + } + + source_vm_copy = dma_alloc_attrs(qproc->dev, + src_count*sizeof(*source_vm_copy), &phys_src, + GFP_KERNEL, dma_attrs); + if (!source_vm_copy) { + dev_err(qproc->dev, + "failed to allocate buffer to pass source vmid detail\n"); + return -ENOMEM; + } + memcpy(source_vm_copy, src, sizeof(*source_vm_copy) * src_count); + + dest_info = dma_alloc_attrs(qproc->dev, + dest_count*sizeof(*dest_info), &phy_dest, + GFP_KERNEL, dma_attrs); + if (!dest_info) { + dev_err(qproc->dev, + "failed to allocate buffer to pass destination vmid detail\n"); + return -ENOMEM; + } + for (i = 0; i < dest_count; i++) { + dest_info[i].vm = dest[i]; + dest_info[i].perm = perm[i]; + dest_info[i].ctx = NULL; + dest_info[i].ctx_size = 0; + } + + mem_prot_info = dma_alloc_attrs(qproc->dev, + sizeof(*mem_prot_info), &mem_prot_phy, + GFP_KERNEL, dma_attrs); + if (!dest_info) { + dev_err(qproc->dev, + "failed to allocate buffer to pass protected mem detail\n"); + return -ENOMEM; + } + mem_prot_info->addr = addr; + mem_prot_info->size = size; + + desc.args[0] = mem_prot_phy; + desc.args[1] = sizeof(*mem_prot_info); + desc.args[2] = phys_src; + desc.args[3] = sizeof(*source_vm_copy) * src_count; + desc.args[4] = phy_dest; + desc.args[5] = dest_count*sizeof(*dest_info); + desc.args[6] = 0; + + ret = qcom_scm_assign_mem(&desc); + if (ret) + pr_info("%s: Failed to assign memory protection, ret = %d\n", + __func__, ret); + + /* SCM call has returned free up buffers passed to secure domain */ + dma_free_attrs(qproc->dev, src_count*sizeof(*source_vm_copy), + source_vm_copy, phys_src, dma_attrs); + dma_free_attrs(qproc->dev, dest_count*sizeof(*dest_info), + dest_info, phy_dest, dma_attrs); + dma_free_attrs(qproc->dev, sizeof(*mem_prot_info), mem_prot_info, + mem_prot_phy, dma_attrs); + return ret; +} + static int q6v5_load(struct rproc *rproc, const struct firmware *fw) { struct q6v5 *qproc = rproc->priv; memcpy(qproc->mba_region, fw->data, fw->size); - return 0; } @@ -446,6 +596,14 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) memcpy(ptr, fw->data, fw->size); + /* Hypervisor support to access metadata by modem */ + qproc->protection_cmd = ASSIG_ACCESS_MSA; + ret = hyp_mem_access(qproc, phys, ALIGN(fw->size, SZ_4K)); + if (ret) { + dev_err(qproc->dev, "Failed to assign 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); @@ -454,7 +612,11 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) dev_err(qproc->dev, "MPSS header authentication timed out\n"); else if (ret < 0) dev_err(qproc->dev, "MPSS header authentication failed: %d\n", ret); - + /* Metadata authentication done, remove modem access */ + qproc->protection_cmd = REMOV_ACCESS_MSA; + ret = hyp_mem_access(qproc, phys, ALIGN(fw->size, SZ_4K)); + if (ret) + dev_err(qproc->dev, "Failed to assign memory, ret - %d\n", ret); dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs); return ret < 0 ? ret : 0; @@ -483,6 +645,13 @@ static int q6v5_mpss_validate(struct q6v5 *qproc, const struct firmware *fw) else boot_addr = fw_addr; + /* Hypervisor support to modem to access modem fw */ + qproc->protection_cmd = SHARE_ACCESS_MSA; + ret = hyp_mem_access(qproc, boot_addr, qproc->mpss_size); + if (ret) { + dev_err(qproc->dev, "Failed to assign memory, ret - %d\n", ret); + return -ENOMEM; + } ehdr = (struct elf32_hdr *)fw->data; phdrs = (struct elf32_phdr *)(ehdr + 1); for (i = 0; i < ehdr->e_phnum; i++, phdr++) { @@ -595,6 +764,12 @@ static int q6v5_start(struct rproc *rproc) goto assert_reset; } + qproc->protection_cmd = ASSIG_ACCESS_MSA; + ret = hyp_mem_access(qproc, qproc->mba_phys, qproc->mba_size); + if (ret) { + dev_err(qproc->dev, "Failed to assign memory, ret - %d\n", ret); + goto assert_reset; + } writel(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG); ret = q6v5proc_reset(qproc); @@ -616,16 +791,21 @@ 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; } + qproc->protection_cmd = REMOV_ACCESS_MSA; + ret = hyp_mem_access(qproc, qproc->mba_phys, qproc->mba_size); + if (ret) + dev_err(qproc->dev, + "Failed to reclaim memory, ret - %d\n", ret); qproc->running = true; q6v5_clk_disable(qproc->dev, qproc->proxy_clks, @@ -634,7 +814,18 @@ static int q6v5_start(struct rproc *rproc) qproc->proxy_reg_count); return 0; +reclaim_mem: + qproc->protection_cmd = REMOV_SHARE_MSA; + ret = hyp_mem_access(qproc, qproc->mpss_phys, qproc->mpss_size); + if (ret) + dev_err(qproc->dev, + "Failed to reclaim memory, ret - %d\n", ret); halt_axi_ports: + qproc->protection_cmd = REMOV_ACCESS_MSA; + ret = hyp_mem_access(qproc, qproc->mba_phys, qproc->mba_size); + if (ret) + dev_err(qproc->dev, + "Failed to reclaim 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); @@ -977,6 +1168,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; @@ -1056,6 +1248,7 @@ static int q6v5_remove(struct platform_device *pdev) "mem", NULL }, + .version = MSS_MSM8916, }; static const struct rproc_hexagon_res msm8974_mss = { @@ -1093,6 +1286,7 @@ static int q6v5_remove(struct platform_device *pdev) "mem", NULL }, + .version = MSS_MSM8974, }; static const struct of_device_id q6v5_of_match[] = { { .compatible = "qcom,q6v5-pil", .data = &msm8916_mss},
This patch add hypervisor call support for second stage translation from mss remoteproc driver, this is required so that modem on msm8996 which is based on armv8 architecture can access DDR region where modem firmware are loaded. Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org> --- drivers/remoteproc/qcom_q6v5_pil.c | 202 ++++++++++++++++++++++++++++++++++++- 1 file changed, 198 insertions(+), 4 deletions(-)