diff mbox series

[v2,14/18] firmware: qcom_scm-32: Create common legacy atomic call

Message ID 1573593774-12539-15-git-send-email-eberman@codeaurora.org (mailing list archive)
State Superseded
Headers show
Series Restructure, improve target support for qcom_scm driver | expand

Commit Message

Elliot Berman Nov. 12, 2019, 9:22 p.m. UTC
Per [1], legacy calling convention supports up to 5 arguments and
3 return values. Create one function to support this combination.

[1]: https://source.codeaurora.org/quic/la/kernel/msm-4.9/tree/drivers/soc/qcom/scm.c?h=kernel.lnx.4.9.r28-rel#n1024

Signed-off-by: Elliot Berman <eberman@codeaurora.org>
---
 drivers/firmware/qcom_scm-32.c | 107 +++++++++++++++++++++++++----------------
 1 file changed, 66 insertions(+), 41 deletions(-)

Comments

Stephen Boyd Nov. 19, 2019, 10:11 p.m. UTC | #1
Quoting Elliot Berman (2019-11-12 13:22:50)
> Per [1], legacy calling convention supports up to 5 arguments and
> 3 return values. Create one function to support this combination.

And remove the other functions in its place?

It would be nice to have some motivation here in the commit text.

> 
> [1]: https://source.codeaurora.org/quic/la/kernel/msm-4.9/tree/drivers/soc/qcom/scm.c?h=kernel.lnx.4.9.r28-rel#n1024
> 
> diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
> index 913a77c..eca18e1 100644
> --- a/drivers/firmware/qcom_scm-32.c
> +++ b/drivers/firmware/qcom_scm-32.c
> @@ -252,6 +252,8 @@ static int qcom_scm_call(struct device *dev, struct qcom_scm_desc *desc)
>         return ret;
>  }
>  
> +#define LEGACY_ATOMIC_N_REG_ARGS       5
> +#define LEGACY_ATOMIC_FIRST_REG_IDX    2
>  #define LEGACY_CLASS_REGISTER          (0x2 << 8)
>  #define LEGACY_MASK_IRQS               BIT(5)
>  #define LEGACY_ATOMIC_ID(svc, cmd, n) \
> @@ -261,52 +263,34 @@ static int qcom_scm_call(struct device *dev, struct qcom_scm_desc *desc)
>                                 (n & 0xf))
>  
>  /**
> - * qcom_scm_call_atomic1() - Send an atomic SCM command with one argument
> - * @svc_id: service identifier
> - * @cmd_id: command identifier
> - * @arg1: first argument
> + * qcom_scm_call_atomic() - Send an atomic SCM command with up to 5 arguments
> + * and 3 return values
>   *

Please document arguments.

>   * This shall only be used with commands that are guaranteed to be
>   * uninterruptable, atomic and SMP safe.
>   */
> -static s32 qcom_scm_call_atomic1(u32 svc, u32 cmd, u32 arg1)
> +static int qcom_scm_call_atomic(struct qcom_scm_desc *desc)

Can desc be const?

>  {
>         int context_id;
>         struct arm_smccc_args smc = {0};
>         struct arm_smccc_res res;
> +       size_t i, arglen = desc->arginfo & 0xf;
>  
> -       smc.a[0] = LEGACY_ATOMIC_ID(svc, cmd, 1);
> -       smc.a[1] = (unsigned long)&context_id;
> -       smc.a[2] = arg1;
> -       arm_smccc_smc(smc.a[0], smc.a[1], smc.a[2], smc.a[3],
> -                     smc.a[4], smc.a[5], smc.a[6], smc.a[7], &res);
> +       BUG_ON(arglen > LEGACY_ATOMIC_N_REG_ARGS);
>  
> -       return res.a0;
> -}
> +       smc.a[0] = LEGACY_ATOMIC_ID(desc->svc, desc->cmd, arglen);
> +       smc.a[1] = (unsigned long)&context_id;
>  
[...]
>  int __qcom_scm_io_writel(struct device *dev, phys_addr_t addr, unsigned int val)
>  {
> -       return qcom_scm_call_atomic2(QCOM_SCM_SVC_IO, QCOM_SCM_IO_WRITE,
> -                                    addr, val);
> +       struct qcom_scm_desc desc = {
> +               .svc = QCOM_SCM_SVC_IO,
> +               .cmd = QCOM_SCM_IO_WRITE,
> +               .owner = ARM_SMCCC_OWNER_SIP,
> +       };
> +
> +       desc.args[0] = addr;
> +       desc.args[1] = val;
> +       desc.arginfo = QCOM_SCM_ARGS(2);
> +
> +       return qcom_scm_call_atomic(&desc);

So what is the benefit of this conversion? Now callers have to construct
a descriptor on the stack and call the function that would otherwise
accept some number of arguments. Are we going to be adding more register
based APIs? It would seem simpler to just have a similar interface that
smccc has that takes some fixed number of registers and then suffer the
few extra register moves of some random value like 0 when they're not
used by the secure world.
diff mbox series

Patch

diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
index 913a77c..eca18e1 100644
--- a/drivers/firmware/qcom_scm-32.c
+++ b/drivers/firmware/qcom_scm-32.c
@@ -252,6 +252,8 @@  static int qcom_scm_call(struct device *dev, struct qcom_scm_desc *desc)
 	return ret;
 }
 
+#define LEGACY_ATOMIC_N_REG_ARGS	5
+#define LEGACY_ATOMIC_FIRST_REG_IDX	2
 #define LEGACY_CLASS_REGISTER		(0x2 << 8)
 #define LEGACY_MASK_IRQS		BIT(5)
 #define LEGACY_ATOMIC_ID(svc, cmd, n) \
@@ -261,52 +263,34 @@  static int qcom_scm_call(struct device *dev, struct qcom_scm_desc *desc)
 				(n & 0xf))
 
 /**
- * qcom_scm_call_atomic1() - Send an atomic SCM command with one argument
- * @svc_id: service identifier
- * @cmd_id: command identifier
- * @arg1: first argument
+ * qcom_scm_call_atomic() - Send an atomic SCM command with up to 5 arguments
+ * and 3 return values
  *
  * This shall only be used with commands that are guaranteed to be
  * uninterruptable, atomic and SMP safe.
  */
-static s32 qcom_scm_call_atomic1(u32 svc, u32 cmd, u32 arg1)
+static int qcom_scm_call_atomic(struct qcom_scm_desc *desc)
 {
 	int context_id;
 	struct arm_smccc_args smc = {0};
 	struct arm_smccc_res res;
+	size_t i, arglen = desc->arginfo & 0xf;
 
-	smc.a[0] = LEGACY_ATOMIC_ID(svc, cmd, 1);
-	smc.a[1] = (unsigned long)&context_id;
-	smc.a[2] = arg1;
-	arm_smccc_smc(smc.a[0], smc.a[1], smc.a[2], smc.a[3],
-		      smc.a[4], smc.a[5], smc.a[6], smc.a[7], &res);
+	BUG_ON(arglen > LEGACY_ATOMIC_N_REG_ARGS);
 
-	return res.a0;
-}
+	smc.a[0] = LEGACY_ATOMIC_ID(desc->svc, desc->cmd, arglen);
+	smc.a[1] = (unsigned long)&context_id;
 
-/**
- * qcom_scm_call_atomic2() - Send an atomic SCM command with two arguments
- * @svc_id:	service identifier
- * @cmd_id:	command identifier
- * @arg1:	first argument
- * @arg2:	second argument
- *
- * This shall only be used with commands that are guaranteed to be
- * uninterruptable, atomic and SMP safe.
- */
-static s32 qcom_scm_call_atomic2(u32 svc, u32 cmd, u32 arg1, u32 arg2)
-{
-	int context_id;
-	struct arm_smccc_args smc = {0};
-	struct arm_smccc_res res;
+	for (i = 0; i < arglen; i++)
+		smc.a[i + LEGACY_ATOMIC_FIRST_REG_IDX] = desc->args[i];
 
-	smc.a[0] = LEGACY_ATOMIC_ID(svc, cmd, 2);
-	smc.a[1] = (unsigned long)&context_id;
-	smc.a[2] = arg1;
-	smc.a[3] = arg2;
 	arm_smccc_smc(smc.a[0], smc.a[1], smc.a[2], smc.a[3],
 		      smc.a[4], smc.a[5], smc.a[6], smc.a[7], &res);
 
+	desc->result[0] = res.a1;
+	desc->result[1] = res.a2;
+	desc->result[2] = res.a3;
+
 	return res.a0;
 }
 
@@ -328,6 +312,11 @@  int __qcom_scm_set_cold_boot_addr(void *entry, const cpumask_t *cpus)
 		QCOM_SCM_FLAG_COLDBOOT_CPU2,
 		QCOM_SCM_FLAG_COLDBOOT_CPU3,
 	};
+	struct qcom_scm_desc desc = {
+		.svc = QCOM_SCM_SVC_BOOT,
+		.cmd = QCOM_SCM_BOOT_SET_ADDR,
+		.owner = ARM_SMCCC_OWNER_SIP,
+	};
 
 	if (!cpus || (cpus && cpumask_empty(cpus)))
 		return -EINVAL;
@@ -339,8 +328,11 @@  int __qcom_scm_set_cold_boot_addr(void *entry, const cpumask_t *cpus)
 			set_cpu_present(cpu, false);
 	}
 
-	return qcom_scm_call_atomic2(QCOM_SCM_SVC_BOOT, QCOM_SCM_BOOT_SET_ADDR,
-				    flags, virt_to_phys(entry));
+	desc.args[0] = flags;
+	desc.args[1] = virt_to_phys(entry);
+	desc.arginfo = QCOM_SCM_ARGS(2);
+
+	return qcom_scm_call_atomic(&desc);
 }
 
 /**
@@ -399,8 +391,15 @@  int __qcom_scm_set_warm_boot_addr(struct device *dev, void *entry,
  */
 void __qcom_scm_cpu_power_down(u32 flags)
 {
-	qcom_scm_call_atomic1(QCOM_SCM_SVC_BOOT, QCOM_SCM_BOOT_TERMINATE_PC,
-			flags & QCOM_SCM_FLUSH_FLAG_MASK);
+	struct qcom_scm_desc desc = {
+		.svc = QCOM_SCM_SVC_BOOT,
+		.cmd = QCOM_SCM_BOOT_TERMINATE_PC,
+		.args[0] = flags & QCOM_SCM_FLUSH_FLAG_MASK,
+		.arginfo = QCOM_SCM_ARGS(1),
+		.owner = ARM_SMCCC_OWNER_SIP,
+	};
+
+	qcom_scm_call_atomic(&desc);
 }
 
 int __qcom_scm_is_call_available(struct device *dev, u32 svc_id, u32 cmd_id)
@@ -560,8 +559,17 @@  int __qcom_scm_pas_mss_reset(struct device *dev, bool reset)
 
 int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
 {
-	return qcom_scm_call_atomic2(QCOM_SCM_SVC_BOOT, QCOM_SCM_BOOT_SET_DLOAD_MODE,
-				     enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0, 0);
+	struct qcom_scm_desc desc = {
+		.svc = QCOM_SCM_SVC_BOOT,
+		.cmd = QCOM_SCM_BOOT_SET_DLOAD_MODE,
+		.owner = ARM_SMCCC_OWNER_SIP,
+	};
+
+	desc.args[0] = QCOM_SCM_BOOT_SET_DLOAD_MODE;
+	desc.args[1] = enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0;
+	desc.arginfo = QCOM_SCM_ARGS(2);
+
+	return qcom_scm_call_atomic(&desc);
 }
 
 int __qcom_scm_set_remote_state(struct device *dev, u32 state, u32 id)
@@ -610,18 +618,35 @@  int __qcom_scm_io_readl(struct device *dev, phys_addr_t addr,
 			unsigned int *val)
 {
 	int ret;
+	struct qcom_scm_desc desc = {
+		.svc = QCOM_SCM_SVC_IO,
+		.cmd = QCOM_SCM_IO_READ,
+		.owner = ARM_SMCCC_OWNER_SIP,
+	};
+
+	desc.args[0] = addr;
+	desc.arginfo = QCOM_SCM_ARGS(1);
 
-	ret = qcom_scm_call_atomic1(QCOM_SCM_SVC_IO, QCOM_SCM_IO_READ, addr);
+	ret = qcom_scm_call_atomic(&desc);
 	if (ret >= 0)
-		*val = ret;
+		*val = desc.result[0];
 
 	return ret < 0 ? ret : 0;
 }
 
 int __qcom_scm_io_writel(struct device *dev, phys_addr_t addr, unsigned int val)
 {
-	return qcom_scm_call_atomic2(QCOM_SCM_SVC_IO, QCOM_SCM_IO_WRITE,
-				     addr, val);
+	struct qcom_scm_desc desc = {
+		.svc = QCOM_SCM_SVC_IO,
+		.cmd = QCOM_SCM_IO_WRITE,
+		.owner = ARM_SMCCC_OWNER_SIP,
+	};
+
+	desc.args[0] = addr;
+	desc.args[1] = val;
+	desc.arginfo = QCOM_SCM_ARGS(2);
+
+	return qcom_scm_call_atomic(&desc);
 }
 
 int __qcom_scm_qsmmu500_wait_safe_toggle(struct device *dev, bool enable)