Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1385499 > unrolled thread

[PATCH 0/8] Qualcomm SCM Rework

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/8] Qualcomm SCM Rework Andy Gross <andy.gross@linaro.org> - 2016-04-23 00:20 +0200
    [PATCH 4/8] firmware: qcom: scm: Add support for ARM64 SoCs Andy Gross <andy.gross@linaro.org> - 2016-04-23 00:20 +0200
      Re: [PATCH 4/8] firmware: qcom: scm: Add support for ARM64 SoCs Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-04-23 01:50 +0200
        Re: [PATCH 4/8] firmware: qcom: scm: Add support for ARM64 SoCs Andy Gross <andy.gross@linaro.org> - 2016-04-23 07:00 +0200
          Re: [PATCH 4/8] firmware: qcom: scm: Add support for ARM64 SoCs Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-04-23 16:20 +0200
    [PATCH 8/8] arm64: dts: msm8916: Add SCM firmware node Andy Gross <andy.gross@linaro.org> - 2016-04-23 00:20 +0200
    [PATCH 5/8] firmware: qcom: scm: Use atomic SCM for cold boot Andy Gross <andy.gross@linaro.org> - 2016-04-23 00:20 +0200
      Re: [PATCH 5/8] firmware: qcom: scm: Use atomic SCM for cold boot Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-04-23 02:00 +0200
        Re: [PATCH 5/8] firmware: qcom: scm: Use atomic SCM for cold boot Andy Gross <andy.gross@linaro.org> - 2016-04-23 06:50 +0200
    [PATCH 1/8] dt/bindings: firmware: Add Qualcomm SCM binding Andy Gross <andy.gross@linaro.org> - 2016-04-23 00:30 +0200
      Re: [PATCH 1/8] dt/bindings: firmware: Add Qualcomm SCM binding Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-04-23 01:20 +0200
      Re: [PATCH 1/8] dt/bindings: firmware: Add Qualcomm SCM binding Stanimir Varbanov <stanimir.varbanov@linaro.org> - 2016-04-23 10:00 +0200
      Re: [PATCH 1/8] dt/bindings: firmware: Add Qualcomm SCM binding Rob Herring <robh@kernel.org> - 2016-04-23 19:00 +0200
        Re: [PATCH 1/8] dt/bindings: firmware: Add Qualcomm SCM binding Andy Gross <andy.gross@linaro.org> - 2016-04-23 19:40 +0200
          Re: [PATCH 1/8] dt/bindings: firmware: Add Qualcomm SCM binding Rob Herring <robh@kernel.org> - 2016-04-25 15:20 +0200

#1385499 — [PATCH 0/8] Qualcomm SCM Rework

The following set of patches does a bit of rework on the existing
Qualcomm SCM firmware.  The first couple of patches deals with turning
the current SCM into a platform driver.  The next couple are cleanups
that make adding the 64 support a little easier.

I took Kumar's 64 bit support patch and modified it to use the arm_smccc
calls.  This simplified things quite a bit.

Lastly, there are a few DT patches to add the firmware node for a couple of the
supported platforms.

Andy Gross (7):
  dt/bindings: firmware: Add Qualcomm SCM binding
  firmware: qcom: scm: Convert SCM to platform driver
  firmware: qcom: scm: Generalize shared error map
  firmware: qcom: scm: Use atomic SCM for cold boot
  firmware: qcom: scm: Add memory allocation API
  dts: qcom: apq8084: Add SCM firmware node
  arm64: dts: msm8916: Add SCM firmware node

Kumar Gala (1):
  firmware: qcom: scm: Add support for ARM64 SoCs

 .../devicetree/bindings/firmware/qcom,scm.txt      |  31 ++++
 arch/arm/boot/dts/qcom-apq8084.dtsi                |  10 ++
 arch/arm64/Kconfig.platforms                       |   1 +
 arch/arm64/boot/dts/qcom/msm8916.dtsi              |  10 ++
 drivers/firmware/qcom_scm-32.c                     |  61 ++++---
 drivers/firmware/qcom_scm-64.c                     | 194 ++++++++++++++++++++-
 drivers/firmware/qcom_scm.c                        | 178 ++++++++++++++++++-
 drivers/firmware/qcom_scm.h                        |  24 +++
 8 files changed, 468 insertions(+), 41 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/firmware/qcom,scm.txt

-- 
1.9.1

[toc] | [next] | [standalone]

#1385500 — [PATCH 4/8] firmware: qcom: scm: Add support for ARM64 SoCs

From: Kumar Gala <galak@codeaurora.org>

Add an implementation of the SCM interface that works on ARM64 SoCs.  This
is used by things like determine if we have HDCP support or not on the
system.

Signed-off-by: Kumar Gala <galak@codeaurora.org>
Signed-off-by: Andy Gross <andy.gross@linaro.org>
---
 drivers/firmware/qcom_scm-32.c |   4 +
 drivers/firmware/qcom_scm-64.c | 194 ++++++++++++++++++++++++++++++++++++++++-
 drivers/firmware/qcom_scm.c    |   7 ++
 drivers/firmware/qcom_scm.h    |   4 +
 4 files changed, 207 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
index 9e3dc2f..0d2a3f8 100644
--- a/drivers/firmware/qcom_scm-32.c
+++ b/drivers/firmware/qcom_scm-32.c
@@ -482,3 +482,7 @@ int __qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp)
 	return qcom_scm_call(QCOM_SCM_SVC_HDCP, QCOM_SCM_CMD_HDCP,
 		req, req_cnt * sizeof(*req), resp, sizeof(*resp));
 }
+
+void __qcom_scm_init(void)
+{
+}
diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
index bb6555f..86bec08 100644
--- a/drivers/firmware/qcom_scm-64.c
+++ b/drivers/firmware/qcom_scm-64.c
@@ -13,6 +13,142 @@
 #include <linux/io.h>
 #include <linux/errno.h>
 #include <linux/qcom_scm.h>
+#include <linux/cpumask.h>
+#include <linux/delay.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/qcom_scm.h>
+#include <linux/arm-smccc.h>
+
+#include <asm/cacheflush.h>
+#include <asm/compiler.h>
+#include <asm/smp_plat.h>
+
+#include "qcom_scm.h"
+
+#define QCOM_SCM_FNID(s, c) ((((s) & 0xFF) << 8) | ((c) & 0xFF))
+
+#define MAX_QCOM_SCM_ARGS 10
+#define MAX_QCOM_SCM_RETS 3
+
+#define QCOM_SCM_ARGS_IMPL(num, a, b, c, d, e, f, g, h, i, j, ...) (\
+			   (((a) & 0xff) << 4) | \
+			   (((b) & 0xff) << 6) | \
+			   (((c) & 0xff) << 8) | \
+			   (((d) & 0xff) << 10) | \
+			   (((e) & 0xff) << 12) | \
+			   (((f) & 0xff) << 14) | \
+			   (((g) & 0xff) << 16) | \
+			   (((h) & 0xff) << 18) | \
+			   (((i) & 0xff) << 20) | \
+			   (((j) & 0xff) << 22) | \
+			   (num & 0xffff))
+
+#define QCOM_SCM_ARGS(...) QCOM_SCM_ARGS_IMPL(__VA_ARGS__, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)
+
+/**
+ * struct qcom_scm_desc
+ * @arginfo: Metadata describing the arguments in args[]
+ * @args: The array of arguments for the secure syscall
+ * @res: The values returned by the secure syscall
+ * @extra_args_virt: The buffer containing extra arguments
+		   (that don't fit in available registers)
+ * @extra_args_phys: The physical address of the extra arguments
+ */
+struct qcom_scm_desc {
+	u32 arginfo;
+	u64 args[MAX_QCOM_SCM_ARGS];
+	struct arm_smccc_res res;
+
+	/* private */
+	void *extra_args_virt;
+	dma_addr_t extra_args_phys;
+	size_t alloc_size;
+};
+
+static u64 qcom_smccc_convention = -1;
+static DEFINE_MUTEX(qcom_scm_lock);
+
+#define QCOM_SCM_EBUSY_WAIT_MS 30
+#define QCOM_SCM_EBUSY_MAX_RETRY 20
+
+#define N_EXT_QCOM_SCM_ARGS 7
+#define FIRST_EXT_ARG_IDX 3
+#define N_REGISTER_ARGS (MAX_QCOM_SCM_ARGS - N_EXT_QCOM_SCM_ARGS + 1)
+
+/**
+ * qcom_scm_call() - Invoke a syscall in the secure world
+ * @svc_id: service identifier
+ * @cmd_id: command identifier
+ * @fn_id: The function ID for this syscall
+ * @desc: Descriptor structure containing arguments and return values
+ *
+ * Sends a command to the SCM and waits for the command to finish processing.
+ * This should *only* be called in pre-emptible context.
+ *
+*/
+static int qcom_scm_call(u32 svc_id, u32 cmd_id, struct qcom_scm_desc *desc)
+{
+	int arglen = desc->arginfo & 0xf;
+	int ret, retry_count = 0, i;
+	u32 fn_id = QCOM_SCM_FNID(svc_id, cmd_id);
+	u64 cmd, x5 = desc->args[FIRST_EXT_ARG_IDX];
+
+	if (unlikely(arglen > N_REGISTER_ARGS)) {
+		desc->alloc_size = N_EXT_QCOM_SCM_ARGS * sizeof(u64);
+		desc->extra_args_virt =
+			qcom_scm_alloc_buffer(desc->alloc_size,
+						 &desc->extra_args_phys,
+						 GFP_KERNEL);
+		if (!desc->extra_args_virt)
+			return qcom_scm_remap_error(-ENOMEM);
+
+		if (qcom_smccc_convention == ARM_SMCCC_SMC_32) {
+			u32 *args = desc->extra_args_virt;
+
+			for (i = 0; i < N_EXT_QCOM_SCM_ARGS; i++)
+				args[i] = desc->args[i + FIRST_EXT_ARG_IDX];
+		} else {
+			u64 *args = desc->extra_args_virt;
+
+			for (i = 0; i < N_EXT_QCOM_SCM_ARGS; i++)
+				args[i] = desc->args[i + FIRST_EXT_ARG_IDX];
+		}
+
+		x5 = desc->extra_args_phys;
+	}
+
+	do {
+		mutex_lock(&qcom_scm_lock);
+
+		cmd = ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL,
+					 qcom_smccc_convention,
+					 ARM_SMCCC_OWNER_SIP, fn_id);
+
+		do {
+			arm_smccc_smc(cmd, arglen, desc->args[0], desc->args[1],
+				      desc->args[2], x5, 0, 0, &desc->res);
+		} while (desc->res.a0 == QCOM_SCM_INTERRUPTED);
+
+		mutex_unlock(&qcom_scm_lock);
+
+		if (desc->res.a0 == QCOM_SCM_V2_EBUSY) {
+			if (retry_count++ > QCOM_SCM_EBUSY_MAX_RETRY)
+				break;
+			msleep(QCOM_SCM_EBUSY_WAIT_MS);
+		}
+	}  while (desc->res.a0 == QCOM_SCM_V2_EBUSY);
+
+	if (desc->extra_args_virt)
+		qcom_scm_free_buffer(desc->alloc_size, desc->extra_args_virt,
+				     desc->extra_args_phys);
+
+	if (desc->res.a0 < 0)
+		return qcom_scm_remap_error(ret);
+
+	return 0;
+}
 
 /**
  * qcom_scm_set_cold_boot_addr() - Set the cold boot address for cpus
@@ -50,14 +186,68 @@ int __qcom_scm_set_warm_boot_addr(void *entry, const cpumask_t *cpus)
  */
 void __qcom_scm_cpu_power_down(u32 flags)
 {
+	return;
 }
 
 int __qcom_scm_is_call_available(u32 svc_id, u32 cmd_id)
 {
-	return -ENOTSUPP;
+	int ret;
+	struct qcom_scm_desc desc = {0};
+
+	desc.arginfo = QCOM_SCM_ARGS(1);
+	desc.args[0] = QCOM_SCM_FNID(svc_id, cmd_id) |
+			(ARM_SMCCC_OWNER_SIP << ARM_SMCCC_OWNER_SHIFT);
+
+	ret = qcom_scm_call(QCOM_SCM_SVC_INFO, QCOM_IS_CALL_AVAIL_CMD,
+			    &desc);
+
+	if (ret)
+		return ret;
+
+	return desc.res.a1;
 }
 
 int __qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp)
 {
-	return -ENOTSUPP;
+	int ret;
+	struct qcom_scm_desc desc = {0};
+
+	if (req_cnt > QCOM_SCM_HDCP_MAX_REQ_CNT)
+		return -ERANGE;
+
+	desc.args[0] = req[0].addr;
+	desc.args[1] = req[0].val;
+	desc.args[2] = req[1].addr;
+	desc.args[3] = req[1].val;
+	desc.args[4] = req[2].addr;
+	desc.args[5] = req[2].val;
+	desc.args[6] = req[3].addr;
+	desc.args[7] = req[3].val;
+	desc.args[8] = req[4].addr;
+	desc.args[9] = req[4].val;
+	desc.arginfo = QCOM_SCM_ARGS(10);
+
+	ret = qcom_scm_call(QCOM_SCM_SVC_HDCP, QCOM_SCM_CMD_HDCP, &desc);
+	*resp = desc.res.a1;
+
+	return ret;
+}
+
+void __qcom_scm_init(void)
+{
+	u64 cmd;
+	struct arm_smccc_res res;
+	u32 function = QCOM_SCM_FNID(QCOM_SCM_SVC_INFO, QCOM_IS_CALL_AVAIL_CMD);
+
+	/* First try a SMC64 call */
+	cmd = ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, ARM_SMCCC_SMC_64,
+				 ARM_SMCCC_OWNER_SIP, function);
+
+	arm_smccc_smc(cmd, QCOM_SCM_ARGS(1), cmd & (~BIT(ARM_SMCCC_TYPE_SHIFT)),
+		      0, 0, 0, 0, 0, &res);
+
+	if (!res.a0 && res.a1)
+		qcom_smccc_convention = ARM_SMCCC_SMC_64;
+	else
+		qcom_smccc_convention = ARM_SMCCC_SMC_32;
 }
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 8e1eeb8..7d7b12b 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -166,6 +166,11 @@ bool qcom_scm_is_available(void)
 }
 EXPORT_SYMBOL(qcom_scm_is_available);
 
+static void qcom_scm_init(void)
+{
+	__qcom_scm_init();
+}
+
 static int qcom_scm_probe(struct platform_device *pdev)
 {
 	struct qcom_scm *scm;
@@ -208,6 +213,8 @@ static int qcom_scm_probe(struct platform_device *pdev)
 	__scm = scm;
 	__scm->dev = &pdev->dev;
 
+	qcom_scm_init();
+
 	return 0;
 }
 
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index 7dcc733..d3f1f0a 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -36,7 +36,9 @@ extern int __qcom_scm_is_call_available(u32 svc_id, u32 cmd_id);
 extern int __qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt,
 		u32 *resp);
 
+extern void __qcom_scm_init(void);
 /* common error codes */
+#define QCOM_SCM_V2_EBUSY	-12
 #define QCOM_SCM_ENOMEM		-5
 #define QCOM_SCM_EOPNOTSUPP	-4
 #define QCOM_SCM_EINVAL_ADDR	-3
@@ -56,6 +58,8 @@ static inline int qcom_scm_remap_error(int err)
 		return -EOPNOTSUPP;
 	case QCOM_SCM_ENOMEM:
 		return -ENOMEM;
+	case QCOM_SCM_V2_EBUSY:
+		return err;
 	}
 	return -EINVAL;
 }
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1385538 — Re: [PATCH 4/8] firmware: qcom: scm: Add support for ARM64 SoCs

On Fri 22 Apr 15:17 PDT 2016, Andy Gross wrote:

[..]
> diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
[..]
> +
> +/**
> + * struct qcom_scm_desc
> + * @arginfo: Metadata describing the arguments in args[]
> + * @args: The array of arguments for the secure syscall
> + * @res: The values returned by the secure syscall
> + * @extra_args_virt: The buffer containing extra arguments
> +		   (that don't fit in available registers)
> + * @extra_args_phys: The physical address of the extra arguments

@alloc_size

> + */
> +struct qcom_scm_desc {
> +	u32 arginfo;
> +	u64 args[MAX_QCOM_SCM_ARGS];
> +	struct arm_smccc_res res;
> +
> +	/* private */
> +	void *extra_args_virt;
> +	dma_addr_t extra_args_phys;
> +	size_t alloc_size;
> +};
> +
> +static u64 qcom_smccc_convention = -1;
> +static DEFINE_MUTEX(qcom_scm_lock);
> +
> +#define QCOM_SCM_EBUSY_WAIT_MS 30
> +#define QCOM_SCM_EBUSY_MAX_RETRY 20
> +
> +#define N_EXT_QCOM_SCM_ARGS 7
> +#define FIRST_EXT_ARG_IDX 3
> +#define N_REGISTER_ARGS (MAX_QCOM_SCM_ARGS - N_EXT_QCOM_SCM_ARGS + 1)
> +
> +/**
> + * qcom_scm_call() - Invoke a syscall in the secure world
> + * @svc_id: service identifier
> + * @cmd_id: command identifier
> + * @fn_id: The function ID for this syscall
> + * @desc: Descriptor structure containing arguments and return values
> + *
> + * Sends a command to the SCM and waits for the command to finish processing.
> + * This should *only* be called in pre-emptible context.
> + *
> +*/

Extra empty line in comment and odd indentation.

> +static int qcom_scm_call(u32 svc_id, u32 cmd_id, struct qcom_scm_desc *desc)
> +{
> +	int arglen = desc->arginfo & 0xf;
> +	int ret, retry_count = 0, i;
> +	u32 fn_id = QCOM_SCM_FNID(svc_id, cmd_id);
> +	u64 cmd, x5 = desc->args[FIRST_EXT_ARG_IDX];
> +
> +	if (unlikely(arglen > N_REGISTER_ARGS)) {
> +		desc->alloc_size = N_EXT_QCOM_SCM_ARGS * sizeof(u64);
> +		desc->extra_args_virt =

alloc_size, extra_args_virt and extra_args_phys doesn't seem to outlive
this function, can't they be made local variable?

> +			qcom_scm_alloc_buffer(desc->alloc_size,
> +						 &desc->extra_args_phys,
> +						 GFP_KERNEL);
> +		if (!desc->extra_args_virt)
> +			return qcom_scm_remap_error(-ENOMEM);
> +
> +		if (qcom_smccc_convention == ARM_SMCCC_SMC_32) {
> +			u32 *args = desc->extra_args_virt;
> +
> +			for (i = 0; i < N_EXT_QCOM_SCM_ARGS; i++)
> +				args[i] = desc->args[i + FIRST_EXT_ARG_IDX];
> +		} else {
> +			u64 *args = desc->extra_args_virt;
> +
> +			for (i = 0; i < N_EXT_QCOM_SCM_ARGS; i++)
> +				args[i] = desc->args[i + FIRST_EXT_ARG_IDX];
> +		}
> +
> +		x5 = desc->extra_args_phys;
> +	}
> +
> +	do {
> +		mutex_lock(&qcom_scm_lock);
> +
> +		cmd = ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL,
> +					 qcom_smccc_convention,
> +					 ARM_SMCCC_OWNER_SIP, fn_id);
> +
> +		do {
> +			arm_smccc_smc(cmd, arglen, desc->args[0], desc->args[1],
> +				      desc->args[2], x5, 0, 0, &desc->res);
> +		} while (desc->res.a0 == QCOM_SCM_INTERRUPTED);
> +
> +		mutex_unlock(&qcom_scm_lock);
> +
> +		if (desc->res.a0 == QCOM_SCM_V2_EBUSY) {
> +			if (retry_count++ > QCOM_SCM_EBUSY_MAX_RETRY)
> +				break;
> +			msleep(QCOM_SCM_EBUSY_WAIT_MS);
> +		}
> +	}  while (desc->res.a0 == QCOM_SCM_V2_EBUSY);
> +
> +	if (desc->extra_args_virt)
> +		qcom_scm_free_buffer(desc->alloc_size, desc->extra_args_virt,
> +				     desc->extra_args_phys);
> +
> +	if (desc->res.a0 < 0)
> +		return qcom_scm_remap_error(ret);
> +
> +	return 0;
> +}
>  
>  /**
>   * qcom_scm_set_cold_boot_addr() - Set the cold boot address for cpus
> @@ -50,14 +186,68 @@ int __qcom_scm_set_warm_boot_addr(void *entry, const cpumask_t *cpus)
>   */
>  void __qcom_scm_cpu_power_down(u32 flags)
>  {
> +	return;

We can't have this empty?

>  }
>  
>  int __qcom_scm_is_call_available(u32 svc_id, u32 cmd_id)
>  {
> -	return -ENOTSUPP;
> +	int ret;
> +	struct qcom_scm_desc desc = {0};
> +
> +	desc.arginfo = QCOM_SCM_ARGS(1);
> +	desc.args[0] = QCOM_SCM_FNID(svc_id, cmd_id) |

Are we not playing the endian game om arm64?

> +			(ARM_SMCCC_OWNER_SIP << ARM_SMCCC_OWNER_SHIFT);
> +
> +	ret = qcom_scm_call(QCOM_SCM_SVC_INFO, QCOM_IS_CALL_AVAIL_CMD,
> +			    &desc);
> +
> +	if (ret)
> +		return ret;
> +
> +	return desc.res.a1;

We use the following construct elsewhere in scm:

	return ret ? : desc.res.a1;

>  }
>  
[..]
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 8e1eeb8..7d7b12b 100644
[..]
>  
> +static void qcom_scm_init(void)
> +{
> +	__qcom_scm_init();
> +}
> +
>  static int qcom_scm_probe(struct platform_device *pdev)
>  {
>  	struct qcom_scm *scm;
> @@ -208,6 +213,8 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  	__scm = scm;
>  	__scm->dev = &pdev->dev;
>  
> +	qcom_scm_init();
> +

Why don't you call __qcom_scm_init() directly here?

>  	return 0;
>  }
>  
> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
[..]
> +#define QCOM_SCM_V2_EBUSY	-12
>  #define QCOM_SCM_ENOMEM		-5
>  #define QCOM_SCM_EOPNOTSUPP	-4
>  #define QCOM_SCM_EINVAL_ADDR	-3
> @@ -56,6 +58,8 @@ static inline int qcom_scm_remap_error(int err)
>  		return -EOPNOTSUPP;
>  	case QCOM_SCM_ENOMEM:
>  		return -ENOMEM;
> +	case QCOM_SCM_V2_EBUSY:
> +		return err;

I don't think return -ENOMEM is the right thing to do here.

>  	}
>  	return -EINVAL;
>  }

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1385577 — Re: [PATCH 4/8] firmware: qcom: scm: Add support for ARM64 SoCs

On Fri, Apr 22, 2016 at 04:41:05PM -0700, Bjorn Andersson wrote:
> On Fri 22 Apr 15:17 PDT 2016, Andy Gross wrote:
> 
> [..]
> > diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
> [..]
> > +
> > +/**
> > + * struct qcom_scm_desc
> > + * @arginfo: Metadata describing the arguments in args[]
> > + * @args: The array of arguments for the secure syscall
> > + * @res: The values returned by the secure syscall
> > + * @extra_args_virt: The buffer containing extra arguments
> > +		   (that don't fit in available registers)
> > + * @extra_args_phys: The physical address of the extra arguments
> 
> @alloc_size

Will add that.

> > + */
> > +struct qcom_scm_desc {
> > +	u32 arginfo;
> > +	u64 args[MAX_QCOM_SCM_ARGS];
> > +	struct arm_smccc_res res;
> > +
> > +	/* private */
> > +	void *extra_args_virt;
> > +	dma_addr_t extra_args_phys;
> > +	size_t alloc_size;
> > +};
> > +
> > +static u64 qcom_smccc_convention = -1;
> > +static DEFINE_MUTEX(qcom_scm_lock);
> > +
> > +#define QCOM_SCM_EBUSY_WAIT_MS 30
> > +#define QCOM_SCM_EBUSY_MAX_RETRY 20
> > +
> > +#define N_EXT_QCOM_SCM_ARGS 7
> > +#define FIRST_EXT_ARG_IDX 3
> > +#define N_REGISTER_ARGS (MAX_QCOM_SCM_ARGS - N_EXT_QCOM_SCM_ARGS + 1)
> > +
> > +/**
> > + * qcom_scm_call() - Invoke a syscall in the secure world
> > + * @svc_id: service identifier
> > + * @cmd_id: command identifier
> > + * @fn_id: The function ID for this syscall
> > + * @desc: Descriptor structure containing arguments and return values
> > + *
> > + * Sends a command to the SCM and waits for the command to finish processing.
> > + * This should *only* be called in pre-emptible context.
> > + *
> > +*/
> 
> Extra empty line in comment and odd indentation.

oops.  I'll fix that up.

> > +static int qcom_scm_call(u32 svc_id, u32 cmd_id, struct qcom_scm_desc *desc)
> > +{
> > +	int arglen = desc->arginfo & 0xf;
> > +	int ret, retry_count = 0, i;
> > +	u32 fn_id = QCOM_SCM_FNID(svc_id, cmd_id);
> > +	u64 cmd, x5 = desc->args[FIRST_EXT_ARG_IDX];
> > +
> > +	if (unlikely(arglen > N_REGISTER_ARGS)) {
> > +		desc->alloc_size = N_EXT_QCOM_SCM_ARGS * sizeof(u64);
> > +		desc->extra_args_virt =
> 
> alloc_size, extra_args_virt and extra_args_phys doesn't seem to outlive
> this function, can't they be made local variable?

That is a good point.  I'll make them local.

> > +			qcom_scm_alloc_buffer(desc->alloc_size,
> > +						 &desc->extra_args_phys,
> > +						 GFP_KERNEL);
> > +		if (!desc->extra_args_virt)
> > +			return qcom_scm_remap_error(-ENOMEM);
> > +
> > +		if (qcom_smccc_convention == ARM_SMCCC_SMC_32) {
> > +			u32 *args = desc->extra_args_virt;
> > +
> > +			for (i = 0; i < N_EXT_QCOM_SCM_ARGS; i++)
> > +				args[i] = desc->args[i + FIRST_EXT_ARG_IDX];
> > +		} else {
> > +			u64 *args = desc->extra_args_virt;
> > +
> > +			for (i = 0; i < N_EXT_QCOM_SCM_ARGS; i++)
> > +				args[i] = desc->args[i + FIRST_EXT_ARG_IDX];
> > +		}
> > +
> > +		x5 = desc->extra_args_phys;
> > +	}
> > +
> > +	do {
> > +		mutex_lock(&qcom_scm_lock);
> > +
> > +		cmd = ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL,
> > +					 qcom_smccc_convention,
> > +					 ARM_SMCCC_OWNER_SIP, fn_id);
> > +
> > +		do {
> > +			arm_smccc_smc(cmd, arglen, desc->args[0], desc->args[1],
> > +				      desc->args[2], x5, 0, 0, &desc->res);
> > +		} while (desc->res.a0 == QCOM_SCM_INTERRUPTED);
> > +
> > +		mutex_unlock(&qcom_scm_lock);
> > +
> > +		if (desc->res.a0 == QCOM_SCM_V2_EBUSY) {
> > +			if (retry_count++ > QCOM_SCM_EBUSY_MAX_RETRY)
> > +				break;
> > +			msleep(QCOM_SCM_EBUSY_WAIT_MS);
> > +		}
> > +	}  while (desc->res.a0 == QCOM_SCM_V2_EBUSY);
> > +
> > +	if (desc->extra_args_virt)
> > +		qcom_scm_free_buffer(desc->alloc_size, desc->extra_args_virt,
> > +				     desc->extra_args_phys);
> > +
> > +	if (desc->res.a0 < 0)
> > +		return qcom_scm_remap_error(ret);
> > +
> > +	return 0;
> > +}
> >  
> >  /**
> >   * qcom_scm_set_cold_boot_addr() - Set the cold boot address for cpus
> > @@ -50,14 +186,68 @@ int __qcom_scm_set_warm_boot_addr(void *entry, const cpumask_t *cpus)
> >   */
> >  void __qcom_scm_cpu_power_down(u32 flags)
> >  {
> > +	return;
> 
> We can't have this empty?

OCD kicked in I think.  Yeah I'll make it empty.

> >  
> >  int __qcom_scm_is_call_available(u32 svc_id, u32 cmd_id)
> >  {
> > -	return -ENOTSUPP;
> > +	int ret;
> > +	struct qcom_scm_desc desc = {0};
> > +
> > +	desc.arginfo = QCOM_SCM_ARGS(1);
> > +	desc.args[0] = QCOM_SCM_FNID(svc_id, cmd_id) |
> 
> Are we not playing the endian game om arm64?

Actually yes.  This needs the le munging.

> > +			(ARM_SMCCC_OWNER_SIP << ARM_SMCCC_OWNER_SHIFT);
> > +
> > +	ret = qcom_scm_call(QCOM_SCM_SVC_INFO, QCOM_IS_CALL_AVAIL_CMD,
> > +			    &desc);
> > +
> > +	if (ret)
> > +		return ret;
> > +
> > +	return desc.res.a1;
> 
> We use the following construct elsewhere in scm:
> 
> 	return ret ? : desc.res.a1;

Will fix.

> 
> >  }
> >  
> [..]
> > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> > index 8e1eeb8..7d7b12b 100644
> [..]
> >  
> > +static void qcom_scm_init(void)
> > +{
> > +	__qcom_scm_init();
> > +}
> > +
> >  static int qcom_scm_probe(struct platform_device *pdev)
> >  {
> >  	struct qcom_scm *scm;
> > @@ -208,6 +213,8 @@ static int qcom_scm_probe(struct platform_device *pdev)
> >  	__scm = scm;
> >  	__scm->dev = &pdev->dev;
> >  
> > +	qcom_scm_init();
> > +
> 
> Why don't you call __qcom_scm_init() directly here?

Yeah that would save some stack ops.

As a side note, what do you think about just making the first transaction on the
scm-64 side do this init to figure out 32/64 calling convention?

That would eliminate this mess.

> >  	return 0;
> >  }
> >  
> > diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
> [..]
> > +#define QCOM_SCM_V2_EBUSY	-12
> >  #define QCOM_SCM_ENOMEM		-5
> >  #define QCOM_SCM_EOPNOTSUPP	-4
> >  #define QCOM_SCM_EINVAL_ADDR	-3
> > @@ -56,6 +58,8 @@ static inline int qcom_scm_remap_error(int err)
> >  		return -EOPNOTSUPP;
> >  	case QCOM_SCM_ENOMEM:
> >  		return -ENOMEM;
> > +	case QCOM_SCM_V2_EBUSY:
> > +		return err;
> 
> I don't think return -ENOMEM is the right thing to do here.

-EBUSY?

> >  	return -EINVAL;
> >  }

[toc] | [prev] | [next] | [standalone]

#1385679 — Re: [PATCH 4/8] firmware: qcom: scm: Add support for ARM64 SoCs

On Fri 22 Apr 21:52 PDT 2016, Andy Gross wrote:

> On Fri, Apr 22, 2016 at 04:41:05PM -0700, Bjorn Andersson wrote:
> > On Fri 22 Apr 15:17 PDT 2016, Andy Gross wrote:
[..]
> > > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> > > index 8e1eeb8..7d7b12b 100644
> > [..]
> > >  
> > > +static void qcom_scm_init(void)
> > > +{
> > > +	__qcom_scm_init();
> > > +}
> > > +
> > >  static int qcom_scm_probe(struct platform_device *pdev)
> > >  {
> > >  	struct qcom_scm *scm;
> > > @@ -208,6 +213,8 @@ static int qcom_scm_probe(struct platform_device *pdev)
> > >  	__scm = scm;
> > >  	__scm->dev = &pdev->dev;
> > >  
> > > +	qcom_scm_init();
> > > +
> > 
> > Why don't you call __qcom_scm_init() directly here?
> 
> Yeah that would save some stack ops.
> 
> As a side note, what do you think about just making the first transaction on the
> scm-64 side do this init to figure out 32/64 calling convention?
> 
> That would eliminate this mess.
> 

We will have quite a bunch of entry points in this API, so it will
probably be messier to have them all call some potential-init function.

Perhaps if it's possible to push it to the __qcom_scm_call{,_atomic}.
But I'm not sure we want those to be more complicated just to save this
one call...

> > >  	return 0;
> > >  }
> > >  
> > > diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
> > [..]
> > > +#define QCOM_SCM_V2_EBUSY	-12
> > >  #define QCOM_SCM_ENOMEM		-5
> > >  #define QCOM_SCM_EOPNOTSUPP	-4
> > >  #define QCOM_SCM_EINVAL_ADDR	-3
> > > @@ -56,6 +58,8 @@ static inline int qcom_scm_remap_error(int err)
> > >  		return -EOPNOTSUPP;
> > >  	case QCOM_SCM_ENOMEM:
> > >  		return -ENOMEM;
> > > +	case QCOM_SCM_V2_EBUSY:
> > > +		return err;
> > 
> > I don't think return -ENOMEM is the right thing to do here.
> 
> -EBUSY?
> 

That seems better.

> > >  	return -EINVAL;
> > >  }

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1385502 — [PATCH 8/8] arm64: dts: msm8916: Add SCM firmware node

This adds the devicetree node for the SCM firmware.

Signed-off-by: Andy Gross <andy.gross@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8916.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
index 9681200..d912cd7 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -122,6 +122,16 @@
 		hwlocks = <&tcsr_mutex 3>;
 	};
 
+	firmware {
+		compatible = "simple-bus";
+
+		scm {
+			compatible = "qcom,scm-msm8916";
+			clocks = <&gcc GCC_CRYPTO_CLK>, <&gcc GCC_CRYPTO_AXI_CLK>, <&gcc GCC_CRYPTO_AHB_CLK>;
+			clock-names = "core", "bus", "iface";
+		};
+	};
+
 	soc: soc {
 		#address-cells = <1>;
 		#size-cells = <1>;
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1385503 — [PATCH 5/8] firmware: qcom: scm: Use atomic SCM for cold boot

This patch changes the cold_set_boot_addr function to use atomic SCM
calls.  This removes the need for memory allocation and instead places
all arguments in registers.

Signed-off-by: Andy Gross <andy.gross@linaro.org>
---
 drivers/firmware/qcom_scm-32.c | 40 ++++++++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 14 deletions(-)

diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
index 0d2a3f8..f596091 100644
--- a/drivers/firmware/qcom_scm-32.c
+++ b/drivers/firmware/qcom_scm-32.c
@@ -294,34 +294,39 @@ out:
 				(n & 0xf))
 
 /**
- * qcom_scm_call_atomic1() - Send an atomic SCM command with one argument
+ * qcom_scm_call_atomic() - Send an atomic SCM command with one argument
  * @svc_id: service identifier
  * @cmd_id: command identifier
+ * @arglen: number of arguments
  * @arg1: first argument
+ * @arg2: second argument (optional - fill with 0 if unused)
  *
  * 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 s32 qcom_scm_call_atomic(u32 svc, u32 cmd, u32 arglen, u32 arg1,
+				u32 arg2)
 {
 	int context_id;
 
-	register u32 r0 asm("r0") = SCM_ATOMIC(svc, cmd, 1);
+	register u32 r0 asm("r0") = SCM_ATOMIC(svc, cmd, arglen);
 	register u32 r1 asm("r1") = (u32)&context_id;
 	register u32 r2 asm("r2") = arg1;
+	register u32 r3 asm("r3") = arg2;
 
 	asm volatile(
 			__asmeq("%0", "r0")
 			__asmeq("%1", "r0")
 			__asmeq("%2", "r1")
 			__asmeq("%3", "r2")
+			__asmeq("%4", "r3")
 #ifdef REQUIRES_SEC
 			".arch_extension sec\n"
 #endif
 			"smc    #0      @ switch to secure world\n"
 			: "=r" (r0)
-			: "r" (r0), "r" (r1), "r" (r2)
-			: "r3");
+			: "r" (r0), "r" (r1), "r" (r2), "r" (r3)
+			);
 	return r0;
 }
 
@@ -364,17 +369,24 @@ EXPORT_SYMBOL(qcom_scm_get_version);
 /*
  * Set the cold/warm boot address for one of the CPU cores.
  */
-static int qcom_scm_set_boot_addr(u32 addr, int flags)
+static int qcom_scm_set_boot_addr(u32 addr, int flags, bool do_atomic)
 {
 	struct {
 		__le32 flags;
 		__le32 addr;
 	} cmd;
 
-	cmd.addr = cpu_to_le32(addr);
-	cmd.flags = cpu_to_le32(flags);
-	return qcom_scm_call(QCOM_SCM_SVC_BOOT, QCOM_SCM_BOOT_ADDR,
-			&cmd, sizeof(cmd), NULL, 0);
+	if (do_atomic) {
+		return qcom_scm_call_atomic(QCOM_SCM_SVC_BOOT,
+					    QCOM_SCM_BOOT_ADDR, 2, flags, addr);
+	} else {
+
+		cmd.addr = cpu_to_le32(addr);
+		cmd.flags = cpu_to_le32(flags);
+
+		return qcom_scm_call(QCOM_SCM_SVC_BOOT, QCOM_SCM_BOOT_ADDR,
+				     &cmd, sizeof(cmd), NULL, 0);
+	}
 }
 
 /**
@@ -406,7 +418,7 @@ int __qcom_scm_set_cold_boot_addr(void *entry, const cpumask_t *cpus)
 			set_cpu_present(cpu, false);
 	}
 
-	return qcom_scm_set_boot_addr(virt_to_phys(entry), flags);
+	return qcom_scm_set_boot_addr(virt_to_phys(entry), flags, true);
 }
 
 /**
@@ -437,7 +449,7 @@ int __qcom_scm_set_warm_boot_addr(void *entry, const cpumask_t *cpus)
 	if (!flags)
 		return 0;
 
-	ret = qcom_scm_set_boot_addr(virt_to_phys(entry), flags);
+	ret = qcom_scm_set_boot_addr(virt_to_phys(entry), flags, false);
 	if (!ret) {
 		for_each_cpu(cpu, cpus)
 			qcom_scm_wb[cpu].entry = entry;
@@ -456,8 +468,8 @@ int __qcom_scm_set_warm_boot_addr(void *entry, const cpumask_t *cpus)
  */
 void __qcom_scm_cpu_power_down(u32 flags)
 {
-	qcom_scm_call_atomic1(QCOM_SCM_SVC_BOOT, QCOM_SCM_CMD_TERMINATE_PC,
-			flags & QCOM_SCM_FLUSH_FLAG_MASK);
+	qcom_scm_call_atomic(QCOM_SCM_SVC_BOOT, QCOM_SCM_CMD_TERMINATE_PC, 1,
+			flags & QCOM_SCM_FLUSH_FLAG_MASK, 0);
 }
 
 int __qcom_scm_is_call_available(u32 svc_id, u32 cmd_id)
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1385541 — Re: [PATCH 5/8] firmware: qcom: scm: Use atomic SCM for cold boot

On Fri 22 Apr 15:17 PDT 2016, Andy Gross wrote:

> This patch changes the cold_set_boot_addr function to use atomic SCM
> calls.  This removes the need for memory allocation and instead places
> all arguments in registers.
> 
> Signed-off-by: Andy Gross <andy.gross@linaro.org>
> ---
>  drivers/firmware/qcom_scm-32.c | 40 ++++++++++++++++++++++++++--------------
>  1 file changed, 26 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
[..]
>  /*
>   * Set the cold/warm boot address for one of the CPU cores.
>   */
> -static int qcom_scm_set_boot_addr(u32 addr, int flags)
> +static int qcom_scm_set_boot_addr(u32 addr, int flags, bool do_atomic)
>  {
>  	struct {
>  		__le32 flags;
>  		__le32 addr;
>  	} cmd;
>  
> -	cmd.addr = cpu_to_le32(addr);
> -	cmd.flags = cpu_to_le32(flags);
> -	return qcom_scm_call(QCOM_SCM_SVC_BOOT, QCOM_SCM_BOOT_ADDR,
> -			&cmd, sizeof(cmd), NULL, 0);
> +	if (do_atomic) {
> +		return qcom_scm_call_atomic(QCOM_SCM_SVC_BOOT,
> +					    QCOM_SCM_BOOT_ADDR, 2, flags, addr);
> +	} else {
> +
> +		cmd.addr = cpu_to_le32(addr);
> +		cmd.flags = cpu_to_le32(flags);
> +
> +		return qcom_scm_call(QCOM_SCM_SVC_BOOT, QCOM_SCM_BOOT_ADDR,
> +				     &cmd, sizeof(cmd), NULL, 0);
> +	}

I would prefer that you split this into two functions, rather than
hiding two functions bodies in one function.

Perhaps qcom_scm_set_boot_addr and qcom_scm_set_boot_addr_atomic?

>  }
>  

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1385574 — Re: [PATCH 5/8] firmware: qcom: scm: Use atomic SCM for cold boot

On Fri, Apr 22, 2016 at 04:50:05PM -0700, Bjorn Andersson wrote:
> On Fri 22 Apr 15:17 PDT 2016, Andy Gross wrote:
> 
> > This patch changes the cold_set_boot_addr function to use atomic SCM
> > calls.  This removes the need for memory allocation and instead places
> > all arguments in registers.
> > 
> > Signed-off-by: Andy Gross <andy.gross@linaro.org>
> > ---
> >  drivers/firmware/qcom_scm-32.c | 40 ++++++++++++++++++++++++++--------------
> >  1 file changed, 26 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
> [..]
> >  /*
> >   * Set the cold/warm boot address for one of the CPU cores.
> >   */
> > -static int qcom_scm_set_boot_addr(u32 addr, int flags)
> > +static int qcom_scm_set_boot_addr(u32 addr, int flags, bool do_atomic)
> >  {
> >  	struct {
> >  		__le32 flags;
> >  		__le32 addr;
> >  	} cmd;
> >  
> > -	cmd.addr = cpu_to_le32(addr);
> > -	cmd.flags = cpu_to_le32(flags);
> > -	return qcom_scm_call(QCOM_SCM_SVC_BOOT, QCOM_SCM_BOOT_ADDR,
> > -			&cmd, sizeof(cmd), NULL, 0);
> > +	if (do_atomic) {
> > +		return qcom_scm_call_atomic(QCOM_SCM_SVC_BOOT,
> > +					    QCOM_SCM_BOOT_ADDR, 2, flags, addr);
> > +	} else {
> > +
> > +		cmd.addr = cpu_to_le32(addr);
> > +		cmd.flags = cpu_to_le32(flags);
> > +
> > +		return qcom_scm_call(QCOM_SCM_SVC_BOOT, QCOM_SCM_BOOT_ADDR,
> > +				     &cmd, sizeof(cmd), NULL, 0);
> > +	}
> 
> I would prefer that you split this into two functions, rather than
> hiding two functions bodies in one function.
> 
> Perhaps qcom_scm_set_boot_addr and qcom_scm_set_boot_addr_atomic?

Fair enough.  It does get a little contrived when you start throwing the extra
options in there.

[toc] | [prev] | [next] | [standalone]

#1385512 — [PATCH 1/8] dt/bindings: firmware: Add Qualcomm SCM binding

This patch adds the device tree support for the Qualcomm SCM firmware.

Signed-off-by: Andy Gross <andy.gross@linaro.org>
---
 .../devicetree/bindings/firmware/qcom,scm.txt      | 31 ++++++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/firmware/qcom,scm.txt

diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.txt b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
new file mode 100644
index 0000000..57b9b3a
--- /dev/null
+++ b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
@@ -0,0 +1,31 @@
+QCOM Secure Channel Manager (SCM)
+
+Qualcomm processors include an interface to communicate to the secure firmware.
+This interface allows for clients to request different types of actions.  These
+can include CPU power up/down, HDCP requests, loading of firmware, and other
+assorted actions.
+
+Required properties:
+- compatible: must contain one of the following:
+ * "qcom,scm-apq8064" for APQ8064
+ * "qcom,scm-apq8084" for MSM8084
+ * "qcom,scm-msm8916" for MSM8916
+ * "qcom,scm-msm8974" for MSM8974
+- clocks: One to three clocks may be required based on compatible.
+ * Only core clock required for "qcom,scm-apq8064"
+ * Core, iface, and bus clocks required for all other compatibles.
+- clock-names: Must contain "core" for the core clock, "iface" for the interface
+  clock and "bus" for the bus clock per the requirements of the compatible.
+
+Example for MSM8916:
+
+	firmware {
+		compatible = "simple-bus";
+
+		scm {
+			compatible = "qcom,scm-msm8916";
+			clocks = <&gcc GCC_CRYPTO_CLK> , <&gcc GCC_CRYPTO_AXI_CLK>, <&gcc GCC_CRYPTO_AHB_CLK>;
+			clock-names = "core", "bus", "iface";
+		};
+	};
+
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1385525 — Re: [PATCH 1/8] dt/bindings: firmware: Add Qualcomm SCM binding

On Fri 22 Apr 15:17 PDT 2016, Andy Gross wrote:

> This patch adds the device tree support for the Qualcomm SCM firmware.
> 
> Signed-off-by: Andy Gross <andy.gross@linaro.org>

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

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1385593 — Re: [PATCH 1/8] dt/bindings: firmware: Add Qualcomm SCM binding

Hi Andy,

On 04/23/2016 01:17 AM, Andy Gross wrote:
> This patch adds the device tree support for the Qualcomm SCM firmware.
> 
> Signed-off-by: Andy Gross <andy.gross@linaro.org>
> ---
>  .../devicetree/bindings/firmware/qcom,scm.txt      | 31 ++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/firmware/qcom,scm.txt
> 
> diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.txt b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
> new file mode 100644
> index 0000000..57b9b3a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
> @@ -0,0 +1,31 @@
> +QCOM Secure Channel Manager (SCM)
> +
> +Qualcomm processors include an interface to communicate to the secure firmware.
> +This interface allows for clients to request different types of actions.  These
> +can include CPU power up/down, HDCP requests, loading of firmware, and other
> +assorted actions.
> +
> +Required properties:
> +- compatible: must contain one of the following:
> + * "qcom,scm-apq8064" for APQ8064
> + * "qcom,scm-apq8084" for MSM8084

s/MSM8084/APQ8084

regards,
Stan

[toc] | [prev] | [next] | [standalone]

#1385685 — Re: [PATCH 1/8] dt/bindings: firmware: Add Qualcomm SCM binding

On Fri, Apr 22, 2016 at 5:17 PM, Andy Gross <andy.gross@linaro.org> wrote:
> This patch adds the device tree support for the Qualcomm SCM firmware.
>
> Signed-off-by: Andy Gross <andy.gross@linaro.org>
> ---
>  .../devicetree/bindings/firmware/qcom,scm.txt      | 31 ++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/firmware/qcom,scm.txt
>
> diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.txt b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
> new file mode 100644
> index 0000000..57b9b3a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
> @@ -0,0 +1,31 @@
> +QCOM Secure Channel Manager (SCM)
> +
> +Qualcomm processors include an interface to communicate to the secure firmware.
> +This interface allows for clients to request different types of actions.  These
> +can include CPU power up/down, HDCP requests, loading of firmware, and other
> +assorted actions.
> +
> +Required properties:
> +- compatible: must contain one of the following:
> + * "qcom,scm-apq8064" for APQ8064
> + * "qcom,scm-apq8084" for MSM8084
> + * "qcom,scm-msm8916" for MSM8916
> + * "qcom,scm-msm8974" for MSM8974
> +- clocks: One to three clocks may be required based on compatible.
> + * Only core clock required for "qcom,scm-apq8064"
> + * Core, iface, and bus clocks required for all other compatibles.
> +- clock-names: Must contain "core" for the core clock, "iface" for the interface
> +  clock and "bus" for the bus clock per the requirements of the compatible.
> +
> +Example for MSM8916:
> +
> +       firmware {
> +               compatible = "simple-bus";

Firmware is a bus? Really? Let's not put hacks in the DT just so you
get automatic probing.

> +
> +               scm {
> +                       compatible = "qcom,scm-msm8916";
> +                       clocks = <&gcc GCC_CRYPTO_CLK> , <&gcc GCC_CRYPTO_AXI_CLK>, <&gcc GCC_CRYPTO_AHB_CLK>;
> +                       clock-names = "core", "bus", "iface";

Generally, /firmware defines an interface to firmware. I don't think
clocks belong here. This implies that non-secure world can turn off
clocks to secure world?

Rob

[toc] | [prev] | [next] | [standalone]

#1385696 — Re: [PATCH 1/8] dt/bindings: firmware: Add Qualcomm SCM binding

On Sat, Apr 23, 2016 at 11:56:50AM -0500, Rob Herring wrote:
> On Fri, Apr 22, 2016 at 5:17 PM, Andy Gross <andy.gross@linaro.org> wrote:
> > This patch adds the device tree support for the Qualcomm SCM firmware.
> >
> > Signed-off-by: Andy Gross <andy.gross@linaro.org>
> > ---
> >  .../devicetree/bindings/firmware/qcom,scm.txt      | 31 ++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/firmware/qcom,scm.txt
> >
> > diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.txt b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
> > new file mode 100644
> > index 0000000..57b9b3a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
> > @@ -0,0 +1,31 @@
> > +QCOM Secure Channel Manager (SCM)
> > +
> > +Qualcomm processors include an interface to communicate to the secure firmware.
> > +This interface allows for clients to request different types of actions.  These
> > +can include CPU power up/down, HDCP requests, loading of firmware, and other
> > +assorted actions.
> > +
> > +Required properties:
> > +- compatible: must contain one of the following:
> > + * "qcom,scm-apq8064" for APQ8064
> > + * "qcom,scm-apq8084" for MSM8084
> > + * "qcom,scm-msm8916" for MSM8916
> > + * "qcom,scm-msm8974" for MSM8974
> > +- clocks: One to three clocks may be required based on compatible.
> > + * Only core clock required for "qcom,scm-apq8064"
> > + * Core, iface, and bus clocks required for all other compatibles.
> > +- clock-names: Must contain "core" for the core clock, "iface" for the interface
> > +  clock and "bus" for the bus clock per the requirements of the compatible.
> > +
> > +Example for MSM8916:
> > +
> > +       firmware {
> > +               compatible = "simple-bus";
> 
> Firmware is a bus? Really? Let's not put hacks in the DT just so you
> get automatic probing.

So something like:

        firmware {
                compatible = "qcom,scm-apq8084";
                clocks = <&gcc GCC_CE1_CLK> , <&gcc GCC_CE1_AXI_CLK>, <&gcc GCC_CE1_AHB_CLK>;
                clock-names = "core", "bus", "iface";
        };

Seems to work fine.

> > +
> > +               scm {
> > +                       compatible = "qcom,scm-msm8916";
> > +                       clocks = <&gcc GCC_CRYPTO_CLK> , <&gcc GCC_CRYPTO_AXI_CLK>, <&gcc GCC_CRYPTO_AHB_CLK>;
> > +                       clock-names = "core", "bus", "iface";
> 
> Generally, /firmware defines an interface to firmware. I don't think
> clocks belong here. This implies that non-secure world can turn off
> clocks to secure world?

The caller into the SCM is on the hook for making sure the clocks are turned on.
The firmware people decided to not manage the clocks.  In a perfect world, they
would turn on their own clocks and it would all be self contained.  Sadly, it
isn't going to change.

The alternative is every device that makes scm calls needs to manage the clocks
for the firmware.

[toc] | [prev] | [next] | [standalone]

#1386409 — Re: [PATCH 1/8] dt/bindings: firmware: Add Qualcomm SCM binding

On Sat, Apr 23, 2016 at 12:33:51PM -0500, Andy Gross wrote:
> On Sat, Apr 23, 2016 at 11:56:50AM -0500, Rob Herring wrote:
> > On Fri, Apr 22, 2016 at 5:17 PM, Andy Gross <andy.gross@linaro.org> wrote:
> > > This patch adds the device tree support for the Qualcomm SCM firmware.
> > >
> > > Signed-off-by: Andy Gross <andy.gross@linaro.org>
> > > ---
> > >  .../devicetree/bindings/firmware/qcom,scm.txt      | 31 ++++++++++++++++++++++
> > >  1 file changed, 31 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/firmware/qcom,scm.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.txt b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
> > > new file mode 100644
> > > index 0000000..57b9b3a
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
> > > @@ -0,0 +1,31 @@
> > > +QCOM Secure Channel Manager (SCM)
> > > +
> > > +Qualcomm processors include an interface to communicate to the secure firmware.
> > > +This interface allows for clients to request different types of actions.  These
> > > +can include CPU power up/down, HDCP requests, loading of firmware, and other
> > > +assorted actions.
> > > +
> > > +Required properties:
> > > +- compatible: must contain one of the following:
> > > + * "qcom,scm-apq8064" for APQ8064
> > > + * "qcom,scm-apq8084" for MSM8084
> > > + * "qcom,scm-msm8916" for MSM8916
> > > + * "qcom,scm-msm8974" for MSM8974
> > > +- clocks: One to three clocks may be required based on compatible.
> > > + * Only core clock required for "qcom,scm-apq8064"
> > > + * Core, iface, and bus clocks required for all other compatibles.
> > > +- clock-names: Must contain "core" for the core clock, "iface" for the interface
> > > +  clock and "bus" for the bus clock per the requirements of the compatible.
> > > +
> > > +Example for MSM8916:
> > > +
> > > +       firmware {
> > > +               compatible = "simple-bus";
> > 
> > Firmware is a bus? Really? Let's not put hacks in the DT just so you
> > get automatic probing.
> 
> So something like:
> 
>         firmware {
>                 compatible = "qcom,scm-apq8084";
>                 clocks = <&gcc GCC_CE1_CLK> , <&gcc GCC_CE1_AXI_CLK>, <&gcc GCC_CE1_AHB_CLK>;
>                 clock-names = "core", "bus", "iface";
>         };
> 
> Seems to work fine.

Yes, because the top level nodes are probed. But then you can't have any 
other firmware nodes. You are going to have to call of_platform_populate 
on the /firmware node or create the device yourself. If there are other 
users of /firmware needing to probe, then we can perhaps do it 
generically.


> > > +
> > > +               scm {
> > > +                       compatible = "qcom,scm-msm8916";
> > > +                       clocks = <&gcc GCC_CRYPTO_CLK> , <&gcc GCC_CRYPTO_AXI_CLK>, <&gcc GCC_CRYPTO_AHB_CLK>;
> > > +                       clock-names = "core", "bus", "iface";
> > 
> > Generally, /firmware defines an interface to firmware. I don't think
> > clocks belong here. This implies that non-secure world can turn off
> > clocks to secure world?
> 
> The caller into the SCM is on the hook for making sure the clocks are turned on.
> The firmware people decided to not manage the clocks.  In a perfect world, they
> would turn on their own clocks and it would all be self contained.  Sadly, it
> isn't going to change.

Okay. Seems like a security problem to me though.

Rob

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web