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


Groups > linux.kernel > #1399157 > unrolled thread

[Patch v4 0/8] Qualcomm SCM Rework

Started byAndy Gross <andy.gross@linaro.org>
First post2016-05-11 16:20 +0200
Last post2016-05-12 09:10 +0200
Articles 8 — 3 participants

Back to article view | Back to linux.kernel


Contents

  [Patch v4 0/8] Qualcomm SCM Rework Andy Gross <andy.gross@linaro.org> - 2016-05-11 16:20 +0200
    [Patch v4 5/8] firmware: qcom: scm: Convert to streaming DMA APIS Andy Gross <andy.gross@linaro.org> - 2016-05-11 16:20 +0200
      Re: [Patch v4 5/8] firmware: qcom: scm: Convert to streaming DMA APIS Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-05-11 22:40 +0200
    [Patch v4 2/8] firmware: qcom: scm: Convert SCM to platform driver Andy Gross <andy.gross@linaro.org> - 2016-05-11 16:20 +0200
      Re: [Patch v4 2/8] firmware: qcom: scm: Convert SCM to platform  driver Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-05-11 22:00 +0200
    [Patch v4 1/8] dt/bindings: firmware: Add Qualcomm SCM binding Andy Gross <andy.gross@linaro.org> - 2016-05-11 16:30 +0200
      Re: [Patch v4 1/8] dt/bindings: firmware: Add Qualcomm SCM binding Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-05-11 21:50 +0200
      Re: [Patch v4 1/8] dt/bindings: firmware: Add Qualcomm SCM binding Stanimir Varbanov <stanimir.varbanov@linaro.org> - 2016-05-12 09:10 +0200

#1399157 — [Patch v4 0/8] Qualcomm SCM Rework

FromAndy Gross <andy.gross@linaro.org>
Date2016-05-11 16:20 +0200
Subject[Patch v4 0/8] Qualcomm SCM Rework
Message-ID<rxD9v-1n1-5@gated-at.bofh.it>
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 added in a patch to
convert the scm-32 to use DMA streaming APIs.

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.

Changes from v3:
  * Added compatibles for the specific platforms requiring one clock.
  * All other platforms will use the generic binding that requires 3 clocks.

Changes from v2:
  * Added use of streaming DMA APIs for scm-32.
  * Changed DT binding to simplify compats.
  * Removed Kconfig.platform change that selected QCOM_SCM for ARM64
  * Fixed misc comments
  * Changed init function to populate the device differently.
  * Fixed missing of_node_put

Changes from v1:
  * Changed binding to reflect proper firmware node usage
  * Added arch_initcall to populate the firmware device, if present
  * Fixed various review comments
  * Removed extraneous includes from SCM 64 file.

Andy Gross (7):
  dt/bindings: firmware: Add Qualcomm SCM binding
  firmware: qcom: scm: Convert SCM to platform driver
  firmware: qcom: scm: Use atomic SCM for cold boot
  firmware: qcom: scm: Generalize shared error map
  firmware: qcom: scm: Convert to streaming DMA APIS
  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      |  28 +++
 arch/arm/boot/dts/qcom-apq8084.dtsi                |   8 +
 arch/arm64/boot/dts/qcom/msm8916.dtsi              |   8 +
 drivers/firmware/qcom_scm-32.c                     | 234 +++++++++------------
 drivers/firmware/qcom_scm-64.c                     | 208 +++++++++++++++++-
 drivers/firmware/qcom_scm.c                        | 171 ++++++++++++++-
 drivers/firmware/qcom_scm.h                        |  31 ++-
 7 files changed, 526 insertions(+), 162 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/firmware/qcom,scm.txt

-- 
1.9.1

[toc] | [next] | [standalone]


#1399159 — [Patch v4 5/8] firmware: qcom: scm: Convert to streaming DMA APIS

FromAndy Gross <andy.gross@linaro.org>
Date2016-05-11 16:20 +0200
Subject[Patch v4 5/8] firmware: qcom: scm: Convert to streaming DMA APIS
Message-ID<rxD9w-1n1-19@gated-at.bofh.it>
In reply to#1399157
This patch converts the Qualcomm SCM driver to use the streaming DMA APIs
for communication buffers.

Signed-off-by: Andy Gross <andy.gross@linaro.org>
---
 drivers/firmware/qcom_scm-32.c | 152 +++++++++++++----------------------------
 drivers/firmware/qcom_scm.c    |   6 +-
 drivers/firmware/qcom_scm.h    |  10 +--
 3 files changed, 58 insertions(+), 110 deletions(-)

diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
index 4388d13..3e71aec 100644
--- a/drivers/firmware/qcom_scm-32.c
+++ b/drivers/firmware/qcom_scm-32.c
@@ -23,8 +23,7 @@
 #include <linux/errno.h>
 #include <linux/err.h>
 #include <linux/qcom_scm.h>
-
-#include <asm/cacheflush.h>
+#include <linux/dma-mapping.h>
 
 #include "qcom_scm.h"
 
@@ -97,44 +96,6 @@ struct qcom_scm_response {
 };
 
 /**
- * alloc_qcom_scm_command() - Allocate an SCM command
- * @cmd_size: size of the command buffer
- * @resp_size: size of the response buffer
- *
- * Allocate an SCM command, including enough room for the command
- * and response headers as well as the command and response buffers.
- *
- * Returns a valid &qcom_scm_command on success or %NULL if the allocation fails.
- */
-static struct qcom_scm_command *alloc_qcom_scm_command(size_t cmd_size, size_t resp_size)
-{
-	struct qcom_scm_command *cmd;
-	size_t len = sizeof(*cmd) + sizeof(struct qcom_scm_response) + cmd_size +
-		resp_size;
-	u32 offset;
-
-	cmd = kzalloc(PAGE_ALIGN(len), GFP_KERNEL);
-	if (cmd) {
-		cmd->len = cpu_to_le32(len);
-		offset = offsetof(struct qcom_scm_command, buf);
-		cmd->buf_offset = cpu_to_le32(offset);
-		cmd->resp_hdr_offset = cpu_to_le32(offset + cmd_size);
-	}
-	return cmd;
-}
-
-/**
- * free_qcom_scm_command() - Free an SCM command
- * @cmd: command to free
- *
- * Free an SCM command.
- */
-static inline void free_qcom_scm_command(struct qcom_scm_command *cmd)
-{
-	kfree(cmd);
-}
-
-/**
  * qcom_scm_command_to_response() - Get a pointer to a qcom_scm_response
  * @cmd: command
  *
@@ -168,7 +129,7 @@ static inline void *qcom_scm_get_response_buffer(const struct qcom_scm_response
 	return (void *)rsp + le32_to_cpu(rsp->buf_offset);
 }
 
-static u32 smc(u32 cmd_addr)
+static u32 smc(dma_addr_t cmd_addr)
 {
 	int context_id;
 	register u32 r0 asm("r0") = 1;
@@ -192,51 +153,15 @@ static u32 smc(u32 cmd_addr)
 	return r0;
 }
 
-static int __qcom_scm_call(const struct qcom_scm_command *cmd)
-{
-	int ret;
-	u32 cmd_addr = virt_to_phys(cmd);
-
-	/*
-	 * Flush the command buffer so that the secure world sees
-	 * the correct data.
-	 */
-	secure_flush_area(cmd, cmd->len);
-
-	ret = smc(cmd_addr);
-	if (ret < 0)
-		ret = qcom_scm_remap_error(ret);
-
-	return ret;
-}
-
-static void qcom_scm_inv_range(unsigned long start, unsigned long end)
-{
-	u32 cacheline_size, ctr;
-
-	asm volatile("mrc p15, 0, %0, c0, c0, 1" : "=r" (ctr));
-	cacheline_size = 4 << ((ctr >> 16) & 0xf);
-
-	start = round_down(start, cacheline_size);
-	end = round_up(end, cacheline_size);
-	outer_inv_range(start, end);
-	while (start < end) {
-		asm ("mcr p15, 0, %0, c7, c6, 1" : : "r" (start)
-		     : "memory");
-		start += cacheline_size;
-	}
-	dsb();
-	isb();
-}
-
 /**
  * qcom_scm_call() - Send an SCM command
- * @svc_id: service identifier
- * @cmd_id: command identifier
- * @cmd_buf: command buffer
- * @cmd_len: length of the command buffer
- * @resp_buf: response buffer
- * @resp_len: length of the response buffer
+ * @dev:	struct device
+ * @svc_id:	service identifier
+ * @cmd_id:	command identifier
+ * @cmd_buf:	command buffer
+ * @cmd_len:	length of the command buffer
+ * @resp_buf:	response buffer
+ * @resp_len:	length of the response buffer
  *
  * Sends a command to the SCM and waits for the command to finish processing.
  *
@@ -247,42 +172,60 @@ static void qcom_scm_inv_range(unsigned long start, unsigned long end)
  * and response buffers is taken care of by qcom_scm_call; however, callers are
  * responsible for any other cached buffers passed over to the secure world.
  */
-static int qcom_scm_call(u32 svc_id, u32 cmd_id, const void *cmd_buf,
-			size_t cmd_len, void *resp_buf, size_t resp_len)
+static int qcom_scm_call(struct device *dev, u32 svc_id, u32 cmd_id,
+			 const void *cmd_buf, size_t cmd_len, void *resp_buf,
+			 size_t resp_len)
 {
 	int ret;
 	struct qcom_scm_command *cmd;
 	struct qcom_scm_response *rsp;
-	unsigned long start, end;
+	size_t alloc_len = sizeof(*cmd) + sizeof(struct qcom_scm_response) +
+			   cmd_len + resp_len;
+	dma_addr_t cmd_phys;
+	u32 offset;
 
-	cmd = alloc_qcom_scm_command(cmd_len, resp_len);
+	cmd = kzalloc(PAGE_ALIGN(alloc_len), GFP_KERNEL);
 	if (!cmd)
 		return -ENOMEM;
 
+	cmd->len = cpu_to_le32(alloc_len);
+	offset = offsetof(struct qcom_scm_command, buf);
+	cmd->buf_offset = cpu_to_le32(offset);
+	cmd->resp_hdr_offset = cpu_to_le32(offset + cmd_len);
+
 	cmd->id = cpu_to_le32((svc_id << 10) | cmd_id);
 	if (cmd_buf)
 		memcpy(qcom_scm_get_command_buffer(cmd), cmd_buf, cmd_len);
 
+	cmd_phys = dma_map_single(dev, cmd, alloc_len, DMA_TO_DEVICE);
+	if (dma_mapping_error(dev, cmd_phys)) {
+		kfree(cmd);
+		return -ENOMEM;
+	}
+
 	mutex_lock(&qcom_scm_lock);
-	ret = __qcom_scm_call(cmd);
+	ret = smc(cmd_phys);
+	if (ret < 0)
+		ret = qcom_scm_remap_error(ret);
 	mutex_unlock(&qcom_scm_lock);
 	if (ret)
 		goto out;
 
 	rsp = qcom_scm_command_to_response(cmd);
-	start = (unsigned long)rsp;
 
 	do {
-		qcom_scm_inv_range(start, start + sizeof(*rsp));
+		dma_sync_single_for_cpu(dev, cmd_phys + cmd_len + offset,
+					sizeof(*rsp), DMA_FROM_DEVICE);
 	} while (!rsp->is_complete);
 
-	end = (unsigned long)qcom_scm_get_response_buffer(rsp) + resp_len;
-	qcom_scm_inv_range(start, end);
-
-	if (resp_buf)
+	if (resp_buf) {
+		dma_sync_single_for_cpu(dev, cmd_phys + alloc_len - resp_len,
+					resp_len, DMA_FROM_DEVICE);
 		memcpy(resp_buf, qcom_scm_get_response_buffer(rsp), resp_len);
+	}
 out:
-	free_qcom_scm_command(cmd);
+	dma_unmap_single(dev, cmd_phys, alloc_len, DMA_TO_DEVICE);
+	kfree(cmd);
 	return ret;
 }
 
@@ -437,7 +380,8 @@ int __qcom_scm_set_cold_boot_addr(void *entry, const cpumask_t *cpus)
  * Set the Linux entry point for the SCM to transfer control to when coming
  * out of a power down. CPU power down may be executed on cpuidle or hotplug.
  */
-int __qcom_scm_set_warm_boot_addr(void *entry, const cpumask_t *cpus)
+int __qcom_scm_set_warm_boot_addr(struct device *dev, void *entry,
+				  const cpumask_t *cpus)
 {
 	int ret;
 	int flags = 0;
@@ -463,7 +407,7 @@ int __qcom_scm_set_warm_boot_addr(void *entry, const cpumask_t *cpus)
 
 	cmd.addr = cpu_to_le32(virt_to_phys(entry));
 	cmd.flags = cpu_to_le32(flags);
-	ret = qcom_scm_call(QCOM_SCM_SVC_BOOT, QCOM_SCM_BOOT_ADDR,
+	ret = qcom_scm_call(dev, QCOM_SCM_SVC_BOOT, QCOM_SCM_BOOT_ADDR,
 			    &cmd, sizeof(cmd), NULL, 0);
 	if (!ret) {
 		for_each_cpu(cpu, cpus)
@@ -487,25 +431,27 @@ void __qcom_scm_cpu_power_down(u32 flags)
 			flags & QCOM_SCM_FLUSH_FLAG_MASK);
 }
 
-int __qcom_scm_is_call_available(u32 svc_id, u32 cmd_id)
+int __qcom_scm_is_call_available(struct device *dev, u32 svc_id, u32 cmd_id)
 {
 	int ret;
 	__le32 svc_cmd = cpu_to_le32((svc_id << 10) | cmd_id);
 	__le32 ret_val = 0;
 
-	ret = qcom_scm_call(QCOM_SCM_SVC_INFO, QCOM_IS_CALL_AVAIL_CMD, &svc_cmd,
-			sizeof(svc_cmd), &ret_val, sizeof(ret_val));
+	ret = qcom_scm_call(dev, QCOM_SCM_SVC_INFO, QCOM_IS_CALL_AVAIL_CMD,
+			    &svc_cmd, sizeof(svc_cmd), &ret_val,
+			    sizeof(ret_val));
 	if (ret)
 		return ret;
 
 	return le32_to_cpu(ret_val);
 }
 
-int __qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp)
+int __qcom_scm_hdcp_req(struct device *dev, struct qcom_scm_hdcp_req *req,
+			u32 req_cnt, u32 *resp)
 {
 	if (req_cnt > QCOM_SCM_HDCP_MAX_REQ_CNT)
 		return -ERANGE;
 
-	return qcom_scm_call(QCOM_SCM_SVC_HDCP, QCOM_SCM_CMD_HDCP,
+	return qcom_scm_call(dev, QCOM_SCM_SVC_HDCP, QCOM_SCM_CMD_HDCP,
 		req, req_cnt * sizeof(*req), resp, sizeof(*resp));
 }
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 4d6ae32..23ccec6 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -89,7 +89,7 @@ EXPORT_SYMBOL(qcom_scm_set_cold_boot_addr);
  */
 int qcom_scm_set_warm_boot_addr(void *entry, const cpumask_t *cpus)
 {
-	return __qcom_scm_set_warm_boot_addr(entry, cpus);
+	return __qcom_scm_set_warm_boot_addr(__scm->dev, entry, cpus);
 }
 EXPORT_SYMBOL(qcom_scm_set_warm_boot_addr);
 
@@ -119,7 +119,7 @@ bool qcom_scm_hdcp_available(void)
 	if (ret)
 		return ret;
 
-	ret = __qcom_scm_is_call_available(QCOM_SCM_SVC_HDCP,
+	ret = __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_HDCP,
 						QCOM_SCM_CMD_HDCP);
 
 	qcom_scm_clk_disable();
@@ -143,7 +143,7 @@ int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp)
 	if (ret)
 		return ret;
 
-	ret = __qcom_scm_hdcp_req(req, req_cnt, resp);
+	ret = __qcom_scm_hdcp_req(__scm->dev, req, req_cnt, resp);
 	qcom_scm_clk_disable();
 	return ret;
 }
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index 7dcc733..afe6676 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -19,7 +19,8 @@
 #define QCOM_SCM_FLAG_HLOS		0x01
 #define QCOM_SCM_FLAG_COLDBOOT_MC	0x02
 #define QCOM_SCM_FLAG_WARMBOOT_MC	0x04
-extern int __qcom_scm_set_warm_boot_addr(void *entry, const cpumask_t *cpus);
+extern int __qcom_scm_set_warm_boot_addr(struct device *dev, void *entry,
+		const cpumask_t *cpus);
 extern int __qcom_scm_set_cold_boot_addr(void *entry, const cpumask_t *cpus);
 
 #define QCOM_SCM_CMD_TERMINATE_PC	0x2
@@ -29,12 +30,13 @@ extern void __qcom_scm_cpu_power_down(u32 flags);
 
 #define QCOM_SCM_SVC_INFO		0x6
 #define QCOM_IS_CALL_AVAIL_CMD		0x1
-extern int __qcom_scm_is_call_available(u32 svc_id, u32 cmd_id);
+extern int __qcom_scm_is_call_available(struct device *dev, u32 svc_id,
+		u32 cmd_id);
 
 #define QCOM_SCM_SVC_HDCP		0x11
 #define QCOM_SCM_CMD_HDCP		0x01
-extern int __qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt,
-		u32 *resp);
+extern int __qcom_scm_hdcp_req(struct device *dev,
+		struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp);
 
 /* common error codes */
 #define QCOM_SCM_ENOMEM		-5
-- 
1.9.1

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


#1399497 — Re: [Patch v4 5/8] firmware: qcom: scm: Convert to streaming DMA APIS

FromBjorn Andersson <bjorn.andersson@linaro.org>
Date2016-05-11 22:40 +0200
SubjectRe: [Patch v4 5/8] firmware: qcom: scm: Convert to streaming DMA APIS
Message-ID<rxJ5g-7qx-7@gated-at.bofh.it>
In reply to#1399159
On Wed 11 May 07:15 PDT 2016, Andy Gross wrote:

> This patch converts the Qualcomm SCM driver to use the streaming DMA APIs
> for communication buffers.
> 

Some style issues in qcom_scm_call(), but functionality wise I think
this looks good (with the open question on packing of rsp and rps->buf).

> Signed-off-by: Andy Gross <andy.gross@linaro.org>
> ---
>  drivers/firmware/qcom_scm-32.c | 152 +++++++++++++----------------------------
>  drivers/firmware/qcom_scm.c    |   6 +-
>  drivers/firmware/qcom_scm.h    |  10 +--
>  3 files changed, 58 insertions(+), 110 deletions(-)
> 
> diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
[..]
>  /**
>   * qcom_scm_call() - Send an SCM command
> - * @svc_id: service identifier
> - * @cmd_id: command identifier
> - * @cmd_buf: command buffer
> - * @cmd_len: length of the command buffer
> - * @resp_buf: response buffer
> - * @resp_len: length of the response buffer
> + * @dev:	struct device
> + * @svc_id:	service identifier
> + * @cmd_id:	command identifier
> + * @cmd_buf:	command buffer
> + * @cmd_len:	length of the command buffer
> + * @resp_buf:	response buffer
> + * @resp_len:	length of the response buffer

Leaving the indentation alone clarifies that your changes is just the
addition of "dev".

>   *
>   * Sends a command to the SCM and waits for the command to finish processing.
>   *
> @@ -247,42 +172,60 @@ static void qcom_scm_inv_range(unsigned long start, unsigned long end)
>   * and response buffers is taken care of by qcom_scm_call; however, callers are
>   * responsible for any other cached buffers passed over to the secure world.
>   */
> -static int qcom_scm_call(u32 svc_id, u32 cmd_id, const void *cmd_buf,
> -			size_t cmd_len, void *resp_buf, size_t resp_len)
> +static int qcom_scm_call(struct device *dev, u32 svc_id, u32 cmd_id,
> +			 const void *cmd_buf, size_t cmd_len, void *resp_buf,
> +			 size_t resp_len)
>  {
>  	int ret;
>  	struct qcom_scm_command *cmd;
>  	struct qcom_scm_response *rsp;
> -	unsigned long start, end;
> +	size_t alloc_len = sizeof(*cmd) + sizeof(struct qcom_scm_response) +
> +			   cmd_len + resp_len;

This would be cleaner if written:

size_t alloc_len = sizeof(*cmd) + cmd_len + sizeof(*rsp) + resp_len;

> +	dma_addr_t cmd_phys;
> +	u32 offset;
>  
> -	cmd = alloc_qcom_scm_command(cmd_len, resp_len);
> +	cmd = kzalloc(PAGE_ALIGN(alloc_len), GFP_KERNEL);
>  	if (!cmd)
>  		return -ENOMEM;
>  
> +	cmd->len = cpu_to_le32(alloc_len);
> +	offset = offsetof(struct qcom_scm_command, buf);
> +	cmd->buf_offset = cpu_to_le32(offset);

Sure this is the offset of the buf in *cmd, but based on the rest of my
comments below I would have dropped the offset variable and just used
sizeof(*cmd).

> +	cmd->resp_hdr_offset = cpu_to_le32(offset + cmd_len);

But this would be more intuitive as sizeof(*cmd) + cmd_len;

> +
>  	cmd->id = cpu_to_le32((svc_id << 10) | cmd_id);
>  	if (cmd_buf)
>  		memcpy(qcom_scm_get_command_buffer(cmd), cmd_buf, cmd_len);

Somewhat unrelated, but we should probably just inline this helper in
favour of writing "cmd->buf".

>  
> +	cmd_phys = dma_map_single(dev, cmd, alloc_len, DMA_TO_DEVICE);

Unless there's a guarantee that the resp buffer is packed onto the
response we should probably map the entire page(s).

> +	if (dma_mapping_error(dev, cmd_phys)) {
> +		kfree(cmd);
> +		return -ENOMEM;
> +	}
> +
>  	mutex_lock(&qcom_scm_lock);
> -	ret = __qcom_scm_call(cmd);
> +	ret = smc(cmd_phys);
> +	if (ret < 0)
> +		ret = qcom_scm_remap_error(ret);
>  	mutex_unlock(&qcom_scm_lock);
>  	if (ret)
>  		goto out;
>  
>  	rsp = qcom_scm_command_to_response(cmd);
> -	start = (unsigned long)rsp;
>  
>  	do {
> -		qcom_scm_inv_range(start, start + sizeof(*rsp));
> +		dma_sync_single_for_cpu(dev, cmd_phys + cmd_len + offset,

Same here, would be easier to read if it was
  cmd_phys + sizeof(*cmd) + cmd_len;

Or even:
  cmd_phys + le32_to_cpu(cmd->resp_hdr_offset);

> +					sizeof(*rsp), DMA_FROM_DEVICE);
>  	} while (!rsp->is_complete);
>  
> -	end = (unsigned long)qcom_scm_get_response_buffer(rsp) + resp_len;
> -	qcom_scm_inv_range(start, end);
> -
> -	if (resp_buf)
> +	if (resp_buf) {
> +		dma_sync_single_for_cpu(dev, cmd_phys + alloc_len - resp_len,

Do we know that the firmware always set
  rsp->buf_offset = sizeof(*rsp)

Would be clearer (and safe from this assumption) if you have it as:
  cmd_phys + le32_to_cpu(cmd->resp_hdr_offset) + le32_to_cpu(rsp->buf_offset);

> +					resp_len, DMA_FROM_DEVICE);
>  		memcpy(resp_buf, qcom_scm_get_response_buffer(rsp), resp_len);

And as we just calculated that offset, we can drop the
qcom_scm_get_response_buffer() single use helper.

> +	}
>  out:
> -	free_qcom_scm_command(cmd);
> +	dma_unmap_single(dev, cmd_phys, alloc_len, DMA_TO_DEVICE);
> +	kfree(cmd);
>  	return ret;
>  }
>  

The rest looks good.

Regards,
Bjorn

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


#1399161 — [Patch v4 2/8] firmware: qcom: scm: Convert SCM to platform driver

FromAndy Gross <andy.gross@linaro.org>
Date2016-05-11 16:20 +0200
Subject[Patch v4 2/8] firmware: qcom: scm: Convert SCM to platform driver
Message-ID<rxD9w-1n1-25@gated-at.bofh.it>
In reply to#1399157
This patch converts the Qualcomm SCM firmware driver into a platform
driver.

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

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 45c008d..4d6ae32 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -10,19 +10,61 @@
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
- * 02110-1301, USA.
  */
-
+#include <linux/platform_device.h>
+#include <linux/module.h>
 #include <linux/cpumask.h>
 #include <linux/export.h>
 #include <linux/types.h>
 #include <linux/qcom_scm.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/clk.h>
 
 #include "qcom_scm.h"
 
+struct qcom_scm {
+	struct device *dev;
+	struct clk *core_clk;
+	struct clk *iface_clk;
+	struct clk *bus_clk;
+};
+
+static struct qcom_scm *__scm;
+
+static int qcom_scm_clk_enable(void)
+{
+	int ret;
+
+	ret = clk_prepare_enable(__scm->core_clk);
+	if (ret)
+		goto bail;
+
+	ret = clk_prepare_enable(__scm->iface_clk);
+	if (ret)
+		goto disable_core;
+
+	ret = clk_prepare_enable(__scm->bus_clk);
+	if (ret)
+		goto disable_iface;
+
+	return 0;
+
+disable_iface:
+	clk_disable_unprepare(__scm->iface_clk);
+disable_core:
+	clk_disable_unprepare(__scm->core_clk);
+bail:
+	return ret;
+}
+
+static void qcom_scm_clk_disable(void)
+{
+	clk_disable_unprepare(__scm->core_clk);
+	clk_disable_unprepare(__scm->iface_clk);
+	clk_disable_unprepare(__scm->bus_clk);
+}
+
 /**
  * qcom_scm_set_cold_boot_addr() - Set the cold boot address for cpus
  * @entry: Entry point function for the cpus
@@ -72,12 +114,17 @@ EXPORT_SYMBOL(qcom_scm_cpu_power_down);
  */
 bool qcom_scm_hdcp_available(void)
 {
-	int ret;
+	int ret = qcom_scm_clk_enable();
+
+	if (ret)
+		return ret;
 
 	ret = __qcom_scm_is_call_available(QCOM_SCM_SVC_HDCP,
-		QCOM_SCM_CMD_HDCP);
+						QCOM_SCM_CMD_HDCP);
+
+	qcom_scm_clk_disable();
 
-	return (ret > 0) ? true : false;
+	return ret > 0 ? true : false;
 }
 EXPORT_SYMBOL(qcom_scm_hdcp_available);
 
@@ -91,6 +138,106 @@ EXPORT_SYMBOL(qcom_scm_hdcp_available);
  */
 int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp)
 {
-	return __qcom_scm_hdcp_req(req, req_cnt, resp);
+	int ret = qcom_scm_clk_enable();
+
+	if (ret)
+		return ret;
+
+	ret = __qcom_scm_hdcp_req(req, req_cnt, resp);
+	qcom_scm_clk_disable();
+	return ret;
 }
 EXPORT_SYMBOL(qcom_scm_hdcp_req);
+
+static int qcom_scm_probe(struct platform_device *pdev)
+{
+	struct qcom_scm *scm;
+	int ret;
+
+	scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL);
+	if (!scm)
+		return -ENOMEM;
+
+	scm->core_clk = devm_clk_get(&pdev->dev, "core");
+	if (IS_ERR(scm->core_clk)) {
+		if (PTR_ERR(scm->core_clk) != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "failed to acquire core clk\n");
+		return PTR_ERR(scm->core_clk);
+	}
+
+	if (of_device_is_compatible(pdev->dev.of_node, "qcom,scm")) {
+		scm->iface_clk = devm_clk_get(&pdev->dev, "iface");
+		if (IS_ERR(scm->iface_clk)) {
+			if (PTR_ERR(scm->iface_clk) != -EPROBE_DEFER)
+				dev_err(&pdev->dev, "failed to acquire iface clk\n");
+			return PTR_ERR(scm->iface_clk);
+		}
+
+		scm->bus_clk = devm_clk_get(&pdev->dev, "bus");
+		if (IS_ERR(scm->bus_clk)) {
+			if (PTR_ERR(scm->bus_clk) != -EPROBE_DEFER)
+				dev_err(&pdev->dev, "failed to acquire bus clk\n");
+			return PTR_ERR(scm->bus_clk);
+		}
+	}
+
+	/* vote for max clk rate for highest performance */
+	ret = clk_set_rate(scm->core_clk, INT_MAX);
+	if (ret)
+		return ret;
+
+	__scm = scm;
+	__scm->dev = &pdev->dev;
+
+	return 0;
+}
+
+static const struct of_device_id qcom_scm_dt_match[] = {
+	{ .compatible = "qcom,scm-apq8064",},
+	{ .compatible = "qcom,scm-msm8660",},
+	{ .compatible = "qcom,scm-msm8960",},
+	{ .compatible = "qcom,scm",},
+	{}
+};
+
+MODULE_DEVICE_TABLE(of, qcom_scm_dt_match);
+
+static struct platform_driver qcom_scm_driver = {
+	.driver = {
+		.name	= "qcom_scm",
+		.of_match_table = qcom_scm_dt_match,
+	},
+	.probe = qcom_scm_probe,
+};
+
+static int __init qcom_scm_init(void)
+{
+	struct device_node *np, *parent;
+	int ret;
+
+	np = of_find_matching_node_and_match(NULL, qcom_scm_dt_match, NULL);
+
+	if (!np)
+		return -ENODEV;
+
+	parent = of_get_next_parent(np);
+	ret = of_platform_populate(parent, qcom_scm_dt_match, NULL, NULL);
+
+	of_node_put(parent);
+
+	if (ret)
+		return ret;
+
+	return platform_driver_register(&qcom_scm_driver);
+}
+
+arch_initcall(qcom_scm_init);
+
+static void __exit qcom_scm_exit(void)
+{
+	platform_driver_unregister(&qcom_scm_driver);
+}
+module_exit(qcom_scm_exit);
+
+MODULE_DESCRIPTION("Qualcomm SCM driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

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


#1399461 — Re: [Patch v4 2/8] firmware: qcom: scm: Convert SCM to platform driver

FromBjorn Andersson <bjorn.andersson@linaro.org>
Date2016-05-11 22:00 +0200
SubjectRe: [Patch v4 2/8] firmware: qcom: scm: Convert SCM to platform driver
Message-ID<rxIsy-6D9-15@gated-at.bofh.it>
In reply to#1399161
On Wed 11 May 07:15 PDT 2016, Andy Gross wrote:

> This patch converts the Qualcomm SCM firmware driver into a platform
> driver.
> 
> Signed-off-by: Andy Gross <andy.gross@linaro.org>

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

Regards,
Bjorn

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


#1399178 — [Patch v4 1/8] dt/bindings: firmware: Add Qualcomm SCM binding

FromAndy Gross <andy.gross@linaro.org>
Date2016-05-11 16:30 +0200
Subject[Patch v4 1/8] dt/bindings: firmware: Add Qualcomm SCM binding
Message-ID<rxDjd-1to-43@gated-at.bofh.it>
In reply to#1399157
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      | 28 ++++++++++++++++++++++
 1 file changed, 28 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..0c5f1ff
--- /dev/null
+++ b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
@@ -0,0 +1,28 @@
+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 platforms
+ * "qcom,scm-msm8660" for MSM8660 platforms
+ * "qcom,scm-msm8690" for MSM8690 platforms
+ * "qcom,scm" for later processors (MSM8916, APQ8084, MSM8974, etc)
+- clocks: One to three clocks may be required based on compatible.
+ * Only core clock required for "qcom,scm-apq8084", "qcom,scm-msm8660", and "qcom,scm-msm8960"
+ * Core, iface, and bus clocks required for "qcom,scm"
+- 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 {
+		scm {
+			compatible = "qcom,scm";
+			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]


#1399457 — Re: [Patch v4 1/8] dt/bindings: firmware: Add Qualcomm SCM binding

FromBjorn Andersson <bjorn.andersson@linaro.org>
Date2016-05-11 21:50 +0200
SubjectRe: [Patch v4 1/8] dt/bindings: firmware: Add Qualcomm SCM binding
Message-ID<rxIiR-6wb-13@gated-at.bofh.it>
In reply to#1399178
On Wed 11 May 07:15 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>
> ---
>  .../devicetree/bindings/firmware/qcom,scm.txt      | 28 ++++++++++++++++++++++
>  1 file changed, 28 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..0c5f1ff
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
> @@ -0,0 +1,28 @@
> +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 platforms
> + * "qcom,scm-msm8660" for MSM8660 platforms
> + * "qcom,scm-msm8690" for MSM8690 platforms
> + * "qcom,scm" for later processors (MSM8916, APQ8084, MSM8974, etc)
> +- clocks: One to three clocks may be required based on compatible.
> + * Only core clock required for "qcom,scm-apq8084", "qcom,scm-msm8660", and "qcom,scm-msm8960"
> + * Core, iface, and bus clocks required for "qcom,scm"
> +- 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.

I like this split of generic binding and the special cases, gives the
wanted clarity for the outliers without the added churn in the common
case.

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

Regards,
Bjorn

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


#1399701 — Re: [Patch v4 1/8] dt/bindings: firmware: Add Qualcomm SCM binding

FromStanimir Varbanov <stanimir.varbanov@linaro.org>
Date2016-05-12 09:10 +0200
SubjectRe: [Patch v4 1/8] dt/bindings: firmware: Add Qualcomm SCM binding
Message-ID<rxSUW-MD-11@gated-at.bofh.it>
In reply to#1399178
On 05/11/2016 05:15 PM, 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      | 28 ++++++++++++++++++++++
>  1 file changed, 28 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..0c5f1ff
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
> @@ -0,0 +1,28 @@
> +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 platforms
> + * "qcom,scm-msm8660" for MSM8660 platforms
> + * "qcom,scm-msm8690" for MSM8690 platforms
> + * "qcom,scm" for later processors (MSM8916, APQ8084, MSM8974, etc)
> +- clocks: One to three clocks may be required based on compatible.
> + * Only core clock required for "qcom,scm-apq8084", "qcom,scm-msm8660", and "qcom,scm-msm8960"
                                                ^^^^
probably this should be apq8064?

-- 
regards,
Stan

[toc] | [prev] | [standalone]


Back to top | Article view | linux.kernel


csiph-web