[PATCH 0/8] Qualcomm SCM Rework

[PATCH 0/8] Qualcomm SCM Rework

[Andy Gross]

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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/8] Qualcomm SCM Rework

[Andy Gross]

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

[PATCH 0/8] Qualcomm SCM Rework

[Andy Gross]

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

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

[Andy Gross]

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

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

[Andy Gross]

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

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

[Andy Gross]

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

[PATCH 2/8] firmware: qcom: scm: Convert SCM to platform driver

[Andy Gross]

This patch converts the Qualcomm SCM firmware driver into a platform
driver.

Signed-off-by: Andy Gross <andy.gross@linaro.org>
---
 arch/arm64/Kconfig.platforms |   1 +
 drivers/firmware/qcom_scm.c  | 154 ++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 147 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index efa77c1..6f0876f 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -76,6 +76,7 @@ config ARCH_MVEBU
 config ARCH_QCOM
 	bool "Qualcomm Platforms"
 	select PINCTRL
+	select QCOM_SCM
 	help
 	  This enables support for the ARMv8 based Qualcomm chipsets.
 
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 45c008d..8e1eeb8 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -10,19 +10,68 @@
  * 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/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;
+
+	if (__scm->iface_clk) {
+		ret = clk_prepare_enable(__scm->iface_clk);
+		if (ret)
+			goto disable_core;
+	}
+
+	if (__scm->bus_clk) {
+		ret = clk_prepare_enable(__scm->bus_clk);
+		if (ret)
+			goto disable_iface;
+	}
+
+	return 0;
+
+disable_iface:
+	if (__scm->iface_clk)
+		clk_disable_unprepare(__scm->iface_clk);
+disable_core:
+	if (__scm->bus_clk)
+		clk_disable_unprepare(__scm->core_clk);
+bail:
+	return ret;
+}
+
+static void qcom_scm_clk_disable(void)
+{
+	clk_disable_unprepare(__scm->core_clk);
+	if (__scm->iface_clk)
+		clk_disable_unprepare(__scm->iface_clk);
+	if (__scm->bus_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,11 +121,17 @@ EXPORT_SYMBOL(qcom_scm_cpu_power_down);
  */
 bool qcom_scm_hdcp_available(void)
 {
-	int ret;
+	int ret = qcom_scm_clk_enable();
+
+	if (ret)
+		goto clk_err;
 
 	ret = __qcom_scm_is_call_available(QCOM_SCM_SVC_HDCP,
-		QCOM_SCM_CMD_HDCP);
+						QCOM_SCM_CMD_HDCP);
+
+	qcom_scm_clk_disable();
 
+clk_err:
 	return (ret > 0) ? true : false;
 }
 EXPORT_SYMBOL(qcom_scm_hdcp_available);
@@ -91,6 +146,89 @@ 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);
+
+/**
+ * qcom_scm_is_available() - Checks if SCM is available
+ */
+bool qcom_scm_is_available(void)
+{
+	return !!__scm;
+}
+EXPORT_SYMBOL(qcom_scm_is_available);
+
+static int qcom_scm_probe(struct platform_device *pdev)
+{
+	struct qcom_scm *scm;
+	long rate;
+	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-apq8064")) {
+		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 */
+	rate = clk_round_rate(scm->core_clk, INT_MAX);
+	ret = clk_set_rate(scm->core_clk, rate);
+	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-apq8084",},
+	{ .compatible = "qcom,scm-msm8916",},
+	{ .compatible = "qcom,scm-msm8974",},
+	{},
+};
+
+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,
+};
+
+builtin_platform_driver(qcom_scm_driver);
+MODULE_DESCRIPTION("Qualcomm SCM driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

[PATCH 2/8] firmware: qcom: scm: Convert SCM to platform driver

[Andy Gross]

This patch converts the Qualcomm SCM firmware driver into a platform
driver.

Signed-off-by: Andy Gross <andy.gross@linaro.org>
---
 arch/arm64/Kconfig.platforms |   1 +
 drivers/firmware/qcom_scm.c  | 154 ++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 147 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index efa77c1..6f0876f 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -76,6 +76,7 @@ config ARCH_MVEBU
 config ARCH_QCOM
 	bool "Qualcomm Platforms"
 	select PINCTRL
+	select QCOM_SCM
 	help
 	  This enables support for the ARMv8 based Qualcomm chipsets.
 
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 45c008d..8e1eeb8 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -10,19 +10,68 @@
  * 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/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;
+
+	if (__scm->iface_clk) {
+		ret = clk_prepare_enable(__scm->iface_clk);
+		if (ret)
+			goto disable_core;
+	}
+
+	if (__scm->bus_clk) {
+		ret = clk_prepare_enable(__scm->bus_clk);
+		if (ret)
+			goto disable_iface;
+	}
+
+	return 0;
+
+disable_iface:
+	if (__scm->iface_clk)
+		clk_disable_unprepare(__scm->iface_clk);
+disable_core:
+	if (__scm->bus_clk)
+		clk_disable_unprepare(__scm->core_clk);
+bail:
+	return ret;
+}
+
+static void qcom_scm_clk_disable(void)
+{
+	clk_disable_unprepare(__scm->core_clk);
+	if (__scm->iface_clk)
+		clk_disable_unprepare(__scm->iface_clk);
+	if (__scm->bus_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,11 +121,17 @@ EXPORT_SYMBOL(qcom_scm_cpu_power_down);
  */
 bool qcom_scm_hdcp_available(void)
 {
-	int ret;
+	int ret = qcom_scm_clk_enable();
+
+	if (ret)
+		goto clk_err;
 
 	ret = __qcom_scm_is_call_available(QCOM_SCM_SVC_HDCP,
-		QCOM_SCM_CMD_HDCP);
+						QCOM_SCM_CMD_HDCP);
+
+	qcom_scm_clk_disable();
 
+clk_err:
 	return (ret > 0) ? true : false;
 }
 EXPORT_SYMBOL(qcom_scm_hdcp_available);
@@ -91,6 +146,89 @@ 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);
+
+/**
+ * qcom_scm_is_available() - Checks if SCM is available
+ */
+bool qcom_scm_is_available(void)
+{
+	return !!__scm;
+}
+EXPORT_SYMBOL(qcom_scm_is_available);
+
+static int qcom_scm_probe(struct platform_device *pdev)
+{
+	struct qcom_scm *scm;
+	long rate;
+	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-apq8064")) {
+		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 */
+	rate = clk_round_rate(scm->core_clk, INT_MAX);
+	ret = clk_set_rate(scm->core_clk, rate);
+	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-apq8084",},
+	{ .compatible = "qcom,scm-msm8916",},
+	{ .compatible = "qcom,scm-msm8974",},
+	{},
+};
+
+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,
+};
+
+builtin_platform_driver(qcom_scm_driver);
+MODULE_DESCRIPTION("Qualcomm SCM driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

[PATCH 2/8] firmware: qcom: scm: Convert SCM to platform driver

[Andy Gross]

This patch converts the Qualcomm SCM firmware driver into a platform
driver.

Signed-off-by: Andy Gross <andy.gross@linaro.org>
---
 arch/arm64/Kconfig.platforms |   1 +
 drivers/firmware/qcom_scm.c  | 154 ++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 147 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index efa77c1..6f0876f 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -76,6 +76,7 @@ config ARCH_MVEBU
 config ARCH_QCOM
 	bool "Qualcomm Platforms"
 	select PINCTRL
+	select QCOM_SCM
 	help
 	  This enables support for the ARMv8 based Qualcomm chipsets.
 
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 45c008d..8e1eeb8 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -10,19 +10,68 @@
  * 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/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;
+
+	if (__scm->iface_clk) {
+		ret = clk_prepare_enable(__scm->iface_clk);
+		if (ret)
+			goto disable_core;
+	}
+
+	if (__scm->bus_clk) {
+		ret = clk_prepare_enable(__scm->bus_clk);
+		if (ret)
+			goto disable_iface;
+	}
+
+	return 0;
+
+disable_iface:
+	if (__scm->iface_clk)
+		clk_disable_unprepare(__scm->iface_clk);
+disable_core:
+	if (__scm->bus_clk)
+		clk_disable_unprepare(__scm->core_clk);
+bail:
+	return ret;
+}
+
+static void qcom_scm_clk_disable(void)
+{
+	clk_disable_unprepare(__scm->core_clk);
+	if (__scm->iface_clk)
+		clk_disable_unprepare(__scm->iface_clk);
+	if (__scm->bus_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,11 +121,17 @@ EXPORT_SYMBOL(qcom_scm_cpu_power_down);
  */
 bool qcom_scm_hdcp_available(void)
 {
-	int ret;
+	int ret = qcom_scm_clk_enable();
+
+	if (ret)
+		goto clk_err;
 
 	ret = __qcom_scm_is_call_available(QCOM_SCM_SVC_HDCP,
-		QCOM_SCM_CMD_HDCP);
+						QCOM_SCM_CMD_HDCP);
+
+	qcom_scm_clk_disable();
 
+clk_err:
 	return (ret > 0) ? true : false;
 }
 EXPORT_SYMBOL(qcom_scm_hdcp_available);
@@ -91,6 +146,89 @@ 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);
+
+/**
+ * qcom_scm_is_available() - Checks if SCM is available
+ */
+bool qcom_scm_is_available(void)
+{
+	return !!__scm;
+}
+EXPORT_SYMBOL(qcom_scm_is_available);
+
+static int qcom_scm_probe(struct platform_device *pdev)
+{
+	struct qcom_scm *scm;
+	long rate;
+	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-apq8064")) {
+		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 */
+	rate = clk_round_rate(scm->core_clk, INT_MAX);
+	ret = clk_set_rate(scm->core_clk, rate);
+	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-apq8084",},
+	{ .compatible = "qcom,scm-msm8916",},
+	{ .compatible = "qcom,scm-msm8974",},
+	{},
+};
+
+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,
+};
+
+builtin_platform_driver(qcom_scm_driver);
+MODULE_DESCRIPTION("Qualcomm SCM driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

[PATCH 3/8] firmware: qcom: scm: Generalize shared error map

[Andy Gross]

This patch moves the qcom_scm_remap_error function to the include file
where can be used by both the 32 and 64 bit versions of the code.

Signed-off-by: Andy Gross <agross@codeaurora.org>
Signed-off-by: Andy Gross <andy.gross@linaro.org>
---
 drivers/firmware/qcom_scm-32.c | 17 -----------------
 drivers/firmware/qcom_scm.h    | 16 ++++++++++++++++
 2 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
index 0883292..9e3dc2f 100644
--- a/drivers/firmware/qcom_scm-32.c
+++ b/drivers/firmware/qcom_scm-32.c
@@ -168,23 +168,6 @@ static inline void *qcom_scm_get_response_buffer(const struct qcom_scm_response
 	return (void *)rsp + le32_to_cpu(rsp->buf_offset);
 }
 
-static int qcom_scm_remap_error(int err)
-{
-	pr_err("qcom_scm_call failed with error code %d\n", err);
-	switch (err) {
-	case QCOM_SCM_ERROR:
-		return -EIO;
-	case QCOM_SCM_EINVAL_ADDR:
-	case QCOM_SCM_EINVAL_ARG:
-		return -EINVAL;
-	case QCOM_SCM_EOPNOTSUPP:
-		return -EOPNOTSUPP;
-	case QCOM_SCM_ENOMEM:
-		return -ENOMEM;
-	}
-	return -EINVAL;
-}
-
 static u32 smc(u32 cmd_addr)
 {
 	int context_id;
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index 2cce75c..7dcc733 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -44,4 +44,20 @@ extern int __qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt,
 #define QCOM_SCM_ERROR		-1
 #define QCOM_SCM_INTERRUPTED	1
 
+static inline int qcom_scm_remap_error(int err)
+{
+	switch (err) {
+	case QCOM_SCM_ERROR:
+		return -EIO;
+	case QCOM_SCM_EINVAL_ADDR:
+	case QCOM_SCM_EINVAL_ARG:
+		return -EINVAL;
+	case QCOM_SCM_EOPNOTSUPP:
+		return -EOPNOTSUPP;
+	case QCOM_SCM_ENOMEM:
+		return -ENOMEM;
+	}
+	return -EINVAL;
+}
+
 #endif
-- 
1.9.1

[PATCH 3/8] firmware: qcom: scm: Generalize shared error map

[Andy Gross]

This patch moves the qcom_scm_remap_error function to the include file
where can be used by both the 32 and 64 bit versions of the code.

Signed-off-by: Andy Gross <agross@codeaurora.org>
Signed-off-by: Andy Gross <andy.gross@linaro.org>
---
 drivers/firmware/qcom_scm-32.c | 17 -----------------
 drivers/firmware/qcom_scm.h    | 16 ++++++++++++++++
 2 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
index 0883292..9e3dc2f 100644
--- a/drivers/firmware/qcom_scm-32.c
+++ b/drivers/firmware/qcom_scm-32.c
@@ -168,23 +168,6 @@ static inline void *qcom_scm_get_response_buffer(const struct qcom_scm_response
 	return (void *)rsp + le32_to_cpu(rsp->buf_offset);
 }
 
-static int qcom_scm_remap_error(int err)
-{
-	pr_err("qcom_scm_call failed with error code %d\n", err);
-	switch (err) {
-	case QCOM_SCM_ERROR:
-		return -EIO;
-	case QCOM_SCM_EINVAL_ADDR:
-	case QCOM_SCM_EINVAL_ARG:
-		return -EINVAL;
-	case QCOM_SCM_EOPNOTSUPP:
-		return -EOPNOTSUPP;
-	case QCOM_SCM_ENOMEM:
-		return -ENOMEM;
-	}
-	return -EINVAL;
-}
-
 static u32 smc(u32 cmd_addr)
 {
 	int context_id;
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index 2cce75c..7dcc733 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -44,4 +44,20 @@ extern int __qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt,
 #define QCOM_SCM_ERROR		-1
 #define QCOM_SCM_INTERRUPTED	1
 
+static inline int qcom_scm_remap_error(int err)
+{
+	switch (err) {
+	case QCOM_SCM_ERROR:
+		return -EIO;
+	case QCOM_SCM_EINVAL_ADDR:
+	case QCOM_SCM_EINVAL_ARG:
+		return -EINVAL;
+	case QCOM_SCM_EOPNOTSUPP:
+		return -EOPNOTSUPP;
+	case QCOM_SCM_ENOMEM:
+		return -ENOMEM;
+	}
+	return -EINVAL;
+}
+
 #endif
-- 
1.9.1

[PATCH 3/8] firmware: qcom: scm: Generalize shared error map

[Andy Gross]

This patch moves the qcom_scm_remap_error function to the include file
where can be used by both the 32 and 64 bit versions of the code.

Signed-off-by: Andy Gross <agross@codeaurora.org>
Signed-off-by: Andy Gross <andy.gross@linaro.org>
---
 drivers/firmware/qcom_scm-32.c | 17 -----------------
 drivers/firmware/qcom_scm.h    | 16 ++++++++++++++++
 2 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
index 0883292..9e3dc2f 100644
--- a/drivers/firmware/qcom_scm-32.c
+++ b/drivers/firmware/qcom_scm-32.c
@@ -168,23 +168,6 @@ static inline void *qcom_scm_get_response_buffer(const struct qcom_scm_response
 	return (void *)rsp + le32_to_cpu(rsp->buf_offset);
 }
 
-static int qcom_scm_remap_error(int err)
-{
-	pr_err("qcom_scm_call failed with error code %d\n", err);
-	switch (err) {
-	case QCOM_SCM_ERROR:
-		return -EIO;
-	case QCOM_SCM_EINVAL_ADDR:
-	case QCOM_SCM_EINVAL_ARG:
-		return -EINVAL;
-	case QCOM_SCM_EOPNOTSUPP:
-		return -EOPNOTSUPP;
-	case QCOM_SCM_ENOMEM:
-		return -ENOMEM;
-	}
-	return -EINVAL;
-}
-
 static u32 smc(u32 cmd_addr)
 {
 	int context_id;
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index 2cce75c..7dcc733 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -44,4 +44,20 @@ extern int __qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt,
 #define QCOM_SCM_ERROR		-1
 #define QCOM_SCM_INTERRUPTED	1
 
+static inline int qcom_scm_remap_error(int err)
+{
+	switch (err) {
+	case QCOM_SCM_ERROR:
+		return -EIO;
+	case QCOM_SCM_EINVAL_ADDR:
+	case QCOM_SCM_EINVAL_ARG:
+		return -EINVAL;
+	case QCOM_SCM_EOPNOTSUPP:
+		return -EOPNOTSUPP;
+	case QCOM_SCM_ENOMEM:
+		return -ENOMEM;
+	}
+	return -EINVAL;
+}
+
 #endif
-- 
1.9.1

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

[Andy Gross]

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

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

[Andy Gross]

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

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

[Andy Gross]

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

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

[Andy Gross]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Andy Gross]

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

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

[Andy Gross]

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

[PATCH 6/8] firmware: qcom: scm: Add memory allocation API

[Andy Gross]

This patch adds APIs for the scm-32 and scm-64 to use for coherent memory
allocation.

Signed-off-by: Andy Gross <andy.gross@linaro.org>
---
 drivers/firmware/qcom_scm.c | 17 +++++++++++++++++
 drivers/firmware/qcom_scm.h |  4 ++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 7d7b12b..6e3defb 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -19,6 +19,7 @@
 #include <linux/qcom_scm.h>
 #include <linux/of.h>
 #include <linux/clk.h>
+#include <linux/dma-mapping.h>
 
 #include "qcom_scm.h"
 
@@ -171,6 +172,22 @@ static void qcom_scm_init(void)
 	__qcom_scm_init();
 }
 
+void *qcom_scm_alloc_buffer(size_t size, dma_addr_t *dma_addr,
+				      gfp_t gfp)
+{
+	if (__scm)
+		return dma_alloc_writecombine(__scm->dev, size, dma_addr, gfp);
+	else
+		return ERR_PTR(-ENODEV);
+}
+
+void qcom_scm_free_buffer(size_t size, void *cpu_addr,
+				 dma_addr_t dma_addr)
+{
+	if (__scm)
+		dma_free_writecombine(__scm->dev, size, cpu_addr, dma_addr);
+}
+
 static int qcom_scm_probe(struct platform_device *pdev)
 {
 	struct qcom_scm *scm;
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index d3f1f0a..33215b4 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -37,6 +37,10 @@ extern int __qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt,
 		u32 *resp);
 
 extern void __qcom_scm_init(void);
+extern void *qcom_scm_alloc_buffer(size_t size, dma_addr_t *dma_addr,
+				    gfp_t gfp);
+extern void qcom_scm_free_buffer(size_t size, void *virt_addr,
+				 dma_addr_t dma_addr);
 /* common error codes */
 #define QCOM_SCM_V2_EBUSY	-12
 #define QCOM_SCM_ENOMEM		-5
-- 
1.9.1

[PATCH 6/8] firmware: qcom: scm: Add memory allocation API

[Andy Gross]

This patch adds APIs for the scm-32 and scm-64 to use for coherent memory
allocation.

Signed-off-by: Andy Gross <andy.gross@linaro.org>
---
 drivers/firmware/qcom_scm.c | 17 +++++++++++++++++
 drivers/firmware/qcom_scm.h |  4 ++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 7d7b12b..6e3defb 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -19,6 +19,7 @@
 #include <linux/qcom_scm.h>
 #include <linux/of.h>
 #include <linux/clk.h>
+#include <linux/dma-mapping.h>
 
 #include "qcom_scm.h"
 
@@ -171,6 +172,22 @@ static void qcom_scm_init(void)
 	__qcom_scm_init();
 }
 
+void *qcom_scm_alloc_buffer(size_t size, dma_addr_t *dma_addr,
+				      gfp_t gfp)
+{
+	if (__scm)
+		return dma_alloc_writecombine(__scm->dev, size, dma_addr, gfp);
+	else
+		return ERR_PTR(-ENODEV);
+}
+
+void qcom_scm_free_buffer(size_t size, void *cpu_addr,
+				 dma_addr_t dma_addr)
+{
+	if (__scm)
+		dma_free_writecombine(__scm->dev, size, cpu_addr, dma_addr);
+}
+
 static int qcom_scm_probe(struct platform_device *pdev)
 {
 	struct qcom_scm *scm;
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index d3f1f0a..33215b4 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -37,6 +37,10 @@ extern int __qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt,
 		u32 *resp);
 
 extern void __qcom_scm_init(void);
+extern void *qcom_scm_alloc_buffer(size_t size, dma_addr_t *dma_addr,
+				    gfp_t gfp);
+extern void qcom_scm_free_buffer(size_t size, void *virt_addr,
+				 dma_addr_t dma_addr);
 /* common error codes */
 #define QCOM_SCM_V2_EBUSY	-12
 #define QCOM_SCM_ENOMEM		-5
-- 
1.9.1

[PATCH 7/8] dts: qcom: apq8084: Add SCM firmware node

[Andy Gross]

This patch adds the firmware node for the SCM

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

diff --git a/arch/arm/boot/dts/qcom-apq8084.dtsi b/arch/arm/boot/dts/qcom-apq8084.dtsi
index a33a09f..711b6fb 100644
--- a/arch/arm/boot/dts/qcom-apq8084.dtsi
+++ b/arch/arm/boot/dts/qcom-apq8084.dtsi
@@ -86,6 +86,16 @@
 		};
 	};
 
+	firmware {
+		compatible = "simple-bus";
+
+		scm {
+			compatible = "qcom,scm-apq8084";
+			clocks = <&gcc GCC_CE1_CLK> , <&gcc GCC_CE1_AXI_CLK>, <&gcc GCC_CE1_AHB_CLK>;
+			clock-names = "core", "bus", "iface";
+		};
+	};
+
 	cpu-pmu {
 		compatible = "qcom,krait-pmu";
 		interrupts = <1 7 0xf04>;
-- 
1.9.1

[PATCH 7/8] dts: qcom: apq8084: Add SCM firmware node

[Andy Gross]

This patch adds the firmware node for the SCM

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

diff --git a/arch/arm/boot/dts/qcom-apq8084.dtsi b/arch/arm/boot/dts/qcom-apq8084.dtsi
index a33a09f..711b6fb 100644
--- a/arch/arm/boot/dts/qcom-apq8084.dtsi
+++ b/arch/arm/boot/dts/qcom-apq8084.dtsi
@@ -86,6 +86,16 @@
 		};
 	};
 
+	firmware {
+		compatible = "simple-bus";
+
+		scm {
+			compatible = "qcom,scm-apq8084";
+			clocks = <&gcc GCC_CE1_CLK> , <&gcc GCC_CE1_AXI_CLK>, <&gcc GCC_CE1_AHB_CLK>;
+			clock-names = "core", "bus", "iface";
+		};
+	};
+
 	cpu-pmu {
 		compatible = "qcom,krait-pmu";
 		interrupts = <1 7 0xf04>;
-- 
1.9.1

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

[Andy Gross]

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

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

[Andy Gross]

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

Re: [PATCH 2/8] firmware: qcom: scm: Convert SCM to platform driver

[Bjorn Andersson]

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
[..]
> +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;
> +
> +	if (__scm->iface_clk) {

No need for this check, the clk api accepts NULL.

> +		ret = clk_prepare_enable(__scm->iface_clk);
> +		if (ret)
> +			goto disable_core;
> +	}
> +
> +	if (__scm->bus_clk) {

dito

> +		ret = clk_prepare_enable(__scm->bus_clk);
> +		if (ret)
> +			goto disable_iface;
> +	}
> +
> +	return 0;
> +
> +disable_iface:
> +	if (__scm->iface_clk)

dito

> +		clk_disable_unprepare(__scm->iface_clk);
> +disable_core:
> +	if (__scm->bus_clk)

and here

> +		clk_disable_unprepare(__scm->core_clk);
> +bail:
> +	return ret;
> +}
> +
> +static void qcom_scm_clk_disable(void)
> +{
> +	clk_disable_unprepare(__scm->core_clk);
> +	if (__scm->iface_clk)

and here

> +		clk_disable_unprepare(__scm->iface_clk);
> +	if (__scm->bus_clk)

and here

> +		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,11 +121,17 @@ EXPORT_SYMBOL(qcom_scm_cpu_power_down);
>   */
>  bool qcom_scm_hdcp_available(void)
>  {
> -	int ret;
> +	int ret = qcom_scm_clk_enable();
> +
> +	if (ret)
> +		goto clk_err;

Just return here.

>  
>  	ret = __qcom_scm_is_call_available(QCOM_SCM_SVC_HDCP,
> -		QCOM_SCM_CMD_HDCP);
> +						QCOM_SCM_CMD_HDCP);
> +
> +	qcom_scm_clk_disable();
>  
> +clk_err:
>  	return (ret > 0) ? true : false;

Unnecessary parenthesis.

>  }
>  EXPORT_SYMBOL(qcom_scm_hdcp_available);
> @@ -91,6 +146,89 @@ 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);
> +
> +/**
> + * qcom_scm_is_available() - Checks if SCM is available
> + */
> +bool qcom_scm_is_available(void)
> +{
> +	return !!__scm;
> +}
> +EXPORT_SYMBOL(qcom_scm_is_available);
> +
> +static int qcom_scm_probe(struct platform_device *pdev)
> +{
> +	struct qcom_scm *scm;
> +	long rate;
> +	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-apq8064")) {

Shouldn't this be reversed?

> +		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 */
> +	rate = clk_round_rate(scm->core_clk, INT_MAX);
> +	ret = clk_set_rate(scm->core_clk, rate);
> +	if (ret)
> +		return ret;
> +
> +	__scm = scm;
> +	__scm->dev = &pdev->dev;
> +
> +	return 0;
> +}
> +

Regards,
Bjorn

[PATCH 2/8] firmware: qcom: scm: Convert SCM to platform driver

[Bjorn Andersson]

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
[..]
> +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;
> +
> +	if (__scm->iface_clk) {

No need for this check, the clk api accepts NULL.

> +		ret = clk_prepare_enable(__scm->iface_clk);
> +		if (ret)
> +			goto disable_core;
> +	}
> +
> +	if (__scm->bus_clk) {

dito

> +		ret = clk_prepare_enable(__scm->bus_clk);
> +		if (ret)
> +			goto disable_iface;
> +	}
> +
> +	return 0;
> +
> +disable_iface:
> +	if (__scm->iface_clk)

dito

> +		clk_disable_unprepare(__scm->iface_clk);
> +disable_core:
> +	if (__scm->bus_clk)

and here

> +		clk_disable_unprepare(__scm->core_clk);
> +bail:
> +	return ret;
> +}
> +
> +static void qcom_scm_clk_disable(void)
> +{
> +	clk_disable_unprepare(__scm->core_clk);
> +	if (__scm->iface_clk)

and here

> +		clk_disable_unprepare(__scm->iface_clk);
> +	if (__scm->bus_clk)

and here

> +		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,11 +121,17 @@ EXPORT_SYMBOL(qcom_scm_cpu_power_down);
>   */
>  bool qcom_scm_hdcp_available(void)
>  {
> -	int ret;
> +	int ret = qcom_scm_clk_enable();
> +
> +	if (ret)
> +		goto clk_err;

Just return here.

>  
>  	ret = __qcom_scm_is_call_available(QCOM_SCM_SVC_HDCP,
> -		QCOM_SCM_CMD_HDCP);
> +						QCOM_SCM_CMD_HDCP);
> +
> +	qcom_scm_clk_disable();
>  
> +clk_err:
>  	return (ret > 0) ? true : false;

Unnecessary parenthesis.

>  }
>  EXPORT_SYMBOL(qcom_scm_hdcp_available);
> @@ -91,6 +146,89 @@ 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);
> +
> +/**
> + * qcom_scm_is_available() - Checks if SCM is available
> + */
> +bool qcom_scm_is_available(void)
> +{
> +	return !!__scm;
> +}
> +EXPORT_SYMBOL(qcom_scm_is_available);
> +
> +static int qcom_scm_probe(struct platform_device *pdev)
> +{
> +	struct qcom_scm *scm;
> +	long rate;
> +	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-apq8064")) {

Shouldn't this be reversed?

> +		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 */
> +	rate = clk_round_rate(scm->core_clk, INT_MAX);
> +	ret = clk_set_rate(scm->core_clk, rate);
> +	if (ret)
> +		return ret;
> +
> +	__scm = scm;
> +	__scm->dev = &pdev->dev;
> +
> +	return 0;
> +}
> +

Regards,
Bjorn

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

[Bjorn Andersson]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Acked-by: Bjorn Andersson <bjorn.andersson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Bjorn Andersson]

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

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

[Bjorn Andersson]

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

Re: [PATCH 3/8] firmware: qcom: scm: Generalize shared error map

[Bjorn Andersson]

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

> This patch moves the qcom_scm_remap_error function to the include file
> where can be used by both the 32 and 64 bit versions of the code.
> 
> Signed-off-by: Andy Gross <agross-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Signed-off-by: Andy Gross <andy.gross-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Acked-by: Bjorn Andersson <bjorn.andersson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/8] firmware: qcom: scm: Generalize shared error map

[Bjorn Andersson]

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

> This patch moves the qcom_scm_remap_error function to the include file
> where can be used by both the 32 and 64 bit versions of the code.
> 
> Signed-off-by: Andy Gross <agross@codeaurora.org>
> Signed-off-by: Andy Gross <andy.gross@linaro.org>

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

Regards,
Bjorn

[PATCH 3/8] firmware: qcom: scm: Generalize shared error map

[Bjorn Andersson]

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

> This patch moves the qcom_scm_remap_error function to the include file
> where can be used by both the 32 and 64 bit versions of the code.
> 
> Signed-off-by: Andy Gross <agross@codeaurora.org>
> Signed-off-by: Andy Gross <andy.gross@linaro.org>

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

Regards,
Bjorn

Re: [PATCH 6/8] firmware: qcom: scm: Add memory allocation API

[Bjorn Andersson]

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

> This patch adds APIs for the scm-32 and scm-64 to use for coherent memory
> allocation.
> 
> Signed-off-by: Andy Gross <andy.gross@linaro.org>

This patch must come before the ARM64 implementation.

> ---
>  drivers/firmware/qcom_scm.c | 17 +++++++++++++++++
>  drivers/firmware/qcom_scm.h |  4 ++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
[..]
> +
> +void qcom_scm_free_buffer(size_t size, void *cpu_addr,
> +				 dma_addr_t dma_addr)
> +{
> +	if (__scm)

This would be quite bad and the caller expects that the memory is
released when you return from here. This should also only happen if the
arch specific implementation is buggy, so just let it go BANG!

> +		dma_free_writecombine(__scm->dev, size, cpu_addr, dma_addr);
> +}
> +

Regards,
Bjorn

[PATCH 6/8] firmware: qcom: scm: Add memory allocation API

[Bjorn Andersson]

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

> This patch adds APIs for the scm-32 and scm-64 to use for coherent memory
> allocation.
> 
> Signed-off-by: Andy Gross <andy.gross@linaro.org>

This patch must come before the ARM64 implementation.

> ---
>  drivers/firmware/qcom_scm.c | 17 +++++++++++++++++
>  drivers/firmware/qcom_scm.h |  4 ++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
[..]
> +
> +void qcom_scm_free_buffer(size_t size, void *cpu_addr,
> +				 dma_addr_t dma_addr)
> +{
> +	if (__scm)

This would be quite bad and the caller expects that the memory is
released when you return from here. This should also only happen if the
arch specific implementation is buggy, so just let it go BANG!

> +		dma_free_writecombine(__scm->dev, size, cpu_addr, dma_addr);
> +}
> +

Regards,
Bjorn

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

[Bjorn Andersson]

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

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

[Bjorn Andersson]

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

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

[Bjorn Andersson]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Bjorn Andersson]

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

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

[Bjorn Andersson]

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

Re: [PATCH 7/8] dts: qcom: apq8084: Add SCM firmware node

[Bjorn Andersson]

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

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

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

Regards,
Bjorn

[PATCH 7/8] dts: qcom: apq8084: Add SCM firmware node

[Bjorn Andersson]

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

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

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

Regards,
Bjorn

Re: [PATCH 8/8] arm64: dts: msm8916: Add SCM firmware node

[Bjorn Andersson]

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

> This adds the devicetree node for the SCM firmware.
> 
> Signed-off-by: Andy Gross <andy.gross@linaro.org>

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

Regards,
Bjorn

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

[Bjorn Andersson]

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

> This adds the devicetree node for the SCM firmware.
> 
> Signed-off-by: Andy Gross <andy.gross@linaro.org>

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

Regards,
Bjorn

Re: [PATCH 2/8] firmware: qcom: scm: Convert SCM to platform driver

[Andy Gross]

On Fri, Apr 22, 2016 at 04:11:11PM -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
> [..]
> > +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;
> > +
> > +	if (__scm->iface_clk) {
> 
> No need for this check, the clk api accepts NULL.

Ah ok.  I wondered about that and then didn't check to see.

> > +		ret = clk_prepare_enable(__scm->iface_clk);
> > +		if (ret)
> > +			goto disable_core;
> > +	}
> > +
> > +	if (__scm->bus_clk) {
> 
> dito
> 
> > +		ret = clk_prepare_enable(__scm->bus_clk);
> > +		if (ret)
> > +			goto disable_iface;
> > +	}
> > +
> > +	return 0;
> > +
> > +disable_iface:
> > +	if (__scm->iface_clk)
> 
> dito
> 
> > +		clk_disable_unprepare(__scm->iface_clk);
> > +disable_core:
> > +	if (__scm->bus_clk)
> 
> and here
> 
> > +		clk_disable_unprepare(__scm->core_clk);
> > +bail:
> > +	return ret;
> > +}
> > +
> > +static void qcom_scm_clk_disable(void)
> > +{
> > +	clk_disable_unprepare(__scm->core_clk);
> > +	if (__scm->iface_clk)
> 
> and here
> 
> > +		clk_disable_unprepare(__scm->iface_clk);
> > +	if (__scm->bus_clk)
> 
> and here
> 
> > +		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,11 +121,17 @@ EXPORT_SYMBOL(qcom_scm_cpu_power_down);
> >   */
> >  bool qcom_scm_hdcp_available(void)
> >  {
> > -	int ret;
> > +	int ret = qcom_scm_clk_enable();
> > +
> > +	if (ret)
> > +		goto clk_err;
> 
> Just return here.

right.

> 
> >  
> >  	ret = __qcom_scm_is_call_available(QCOM_SCM_SVC_HDCP,
> > -		QCOM_SCM_CMD_HDCP);
> > +						QCOM_SCM_CMD_HDCP);
> > +
> > +	qcom_scm_clk_disable();
> >  
> > +clk_err:
> >  	return (ret > 0) ? true : false;
> 
> Unnecessary parenthesis.

will fix that.

> 
> >  }
> >  EXPORT_SYMBOL(qcom_scm_hdcp_available);
> > @@ -91,6 +146,89 @@ 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);
> > +
> > +/**
> > + * qcom_scm_is_available() - Checks if SCM is available
> > + */
> > +bool qcom_scm_is_available(void)
> > +{
> > +	return !!__scm;
> > +}
> > +EXPORT_SYMBOL(qcom_scm_is_available);
> > +
> > +static int qcom_scm_probe(struct platform_device *pdev)
> > +{
> > +	struct qcom_scm *scm;
> > +	long rate;
> > +	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-apq8064")) {
> 
> Shouldn't this be reversed?

Right, I inverted this.  Will fix.

> 
> > +		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 */
> > +	rate = clk_round_rate(scm->core_clk, INT_MAX);
> > +	ret = clk_set_rate(scm->core_clk, rate);
> > +	if (ret)
> > +		return ret;
> > +
> > +	__scm = scm;
> > +	__scm->dev = &pdev->dev;
> > +
> > +	return 0;
> > +}
> > +

Thanks for reviewing!
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/8] firmware: qcom: scm: Convert SCM to platform driver

[Andy Gross]

On Fri, Apr 22, 2016 at 04:11:11PM -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
> [..]
> > +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;
> > +
> > +	if (__scm->iface_clk) {
> 
> No need for this check, the clk api accepts NULL.

Ah ok.  I wondered about that and then didn't check to see.

> > +		ret = clk_prepare_enable(__scm->iface_clk);
> > +		if (ret)
> > +			goto disable_core;
> > +	}
> > +
> > +	if (__scm->bus_clk) {
> 
> dito
> 
> > +		ret = clk_prepare_enable(__scm->bus_clk);
> > +		if (ret)
> > +			goto disable_iface;
> > +	}
> > +
> > +	return 0;
> > +
> > +disable_iface:
> > +	if (__scm->iface_clk)
> 
> dito
> 
> > +		clk_disable_unprepare(__scm->iface_clk);
> > +disable_core:
> > +	if (__scm->bus_clk)
> 
> and here
> 
> > +		clk_disable_unprepare(__scm->core_clk);
> > +bail:
> > +	return ret;
> > +}
> > +
> > +static void qcom_scm_clk_disable(void)
> > +{
> > +	clk_disable_unprepare(__scm->core_clk);
> > +	if (__scm->iface_clk)
> 
> and here
> 
> > +		clk_disable_unprepare(__scm->iface_clk);
> > +	if (__scm->bus_clk)
> 
> and here
> 
> > +		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,11 +121,17 @@ EXPORT_SYMBOL(qcom_scm_cpu_power_down);
> >   */
> >  bool qcom_scm_hdcp_available(void)
> >  {
> > -	int ret;
> > +	int ret = qcom_scm_clk_enable();
> > +
> > +	if (ret)
> > +		goto clk_err;
> 
> Just return here.

right.

> 
> >  
> >  	ret = __qcom_scm_is_call_available(QCOM_SCM_SVC_HDCP,
> > -		QCOM_SCM_CMD_HDCP);
> > +						QCOM_SCM_CMD_HDCP);
> > +
> > +	qcom_scm_clk_disable();
> >  
> > +clk_err:
> >  	return (ret > 0) ? true : false;
> 
> Unnecessary parenthesis.

will fix that.

> 
> >  }
> >  EXPORT_SYMBOL(qcom_scm_hdcp_available);
> > @@ -91,6 +146,89 @@ 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);
> > +
> > +/**
> > + * qcom_scm_is_available() - Checks if SCM is available
> > + */
> > +bool qcom_scm_is_available(void)
> > +{
> > +	return !!__scm;
> > +}
> > +EXPORT_SYMBOL(qcom_scm_is_available);
> > +
> > +static int qcom_scm_probe(struct platform_device *pdev)
> > +{
> > +	struct qcom_scm *scm;
> > +	long rate;
> > +	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-apq8064")) {
> 
> Shouldn't this be reversed?

Right, I inverted this.  Will fix.

> 
> > +		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 */
> > +	rate = clk_round_rate(scm->core_clk, INT_MAX);
> > +	ret = clk_set_rate(scm->core_clk, rate);
> > +	if (ret)
> > +		return ret;
> > +
> > +	__scm = scm;
> > +	__scm->dev = &pdev->dev;
> > +
> > +	return 0;
> > +}
> > +

Thanks for reviewing!

[PATCH 2/8] firmware: qcom: scm: Convert SCM to platform driver

[Andy Gross]

On Fri, Apr 22, 2016 at 04:11:11PM -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
> [..]
> > +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;
> > +
> > +	if (__scm->iface_clk) {
> 
> No need for this check, the clk api accepts NULL.

Ah ok.  I wondered about that and then didn't check to see.

> > +		ret = clk_prepare_enable(__scm->iface_clk);
> > +		if (ret)
> > +			goto disable_core;
> > +	}
> > +
> > +	if (__scm->bus_clk) {
> 
> dito
> 
> > +		ret = clk_prepare_enable(__scm->bus_clk);
> > +		if (ret)
> > +			goto disable_iface;
> > +	}
> > +
> > +	return 0;
> > +
> > +disable_iface:
> > +	if (__scm->iface_clk)
> 
> dito
> 
> > +		clk_disable_unprepare(__scm->iface_clk);
> > +disable_core:
> > +	if (__scm->bus_clk)
> 
> and here
> 
> > +		clk_disable_unprepare(__scm->core_clk);
> > +bail:
> > +	return ret;
> > +}
> > +
> > +static void qcom_scm_clk_disable(void)
> > +{
> > +	clk_disable_unprepare(__scm->core_clk);
> > +	if (__scm->iface_clk)
> 
> and here
> 
> > +		clk_disable_unprepare(__scm->iface_clk);
> > +	if (__scm->bus_clk)
> 
> and here
> 
> > +		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,11 +121,17 @@ EXPORT_SYMBOL(qcom_scm_cpu_power_down);
> >   */
> >  bool qcom_scm_hdcp_available(void)
> >  {
> > -	int ret;
> > +	int ret = qcom_scm_clk_enable();
> > +
> > +	if (ret)
> > +		goto clk_err;
> 
> Just return here.

right.

> 
> >  
> >  	ret = __qcom_scm_is_call_available(QCOM_SCM_SVC_HDCP,
> > -		QCOM_SCM_CMD_HDCP);
> > +						QCOM_SCM_CMD_HDCP);
> > +
> > +	qcom_scm_clk_disable();
> >  
> > +clk_err:
> >  	return (ret > 0) ? true : false;
> 
> Unnecessary parenthesis.

will fix that.

> 
> >  }
> >  EXPORT_SYMBOL(qcom_scm_hdcp_available);
> > @@ -91,6 +146,89 @@ 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);
> > +
> > +/**
> > + * qcom_scm_is_available() - Checks if SCM is available
> > + */
> > +bool qcom_scm_is_available(void)
> > +{
> > +	return !!__scm;
> > +}
> > +EXPORT_SYMBOL(qcom_scm_is_available);
> > +
> > +static int qcom_scm_probe(struct platform_device *pdev)
> > +{
> > +	struct qcom_scm *scm;
> > +	long rate;
> > +	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-apq8064")) {
> 
> Shouldn't this be reversed?

Right, I inverted this.  Will fix.

> 
> > +		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 */
> > +	rate = clk_round_rate(scm->core_clk, INT_MAX);
> > +	ret = clk_set_rate(scm->core_clk, rate);
> > +	if (ret)
> > +		return ret;
> > +
> > +	__scm = scm;
> > +	__scm->dev = &pdev->dev;
> > +
> > +	return 0;
> > +}
> > +

Thanks for reviewing!

Re: [PATCH 6/8] firmware: qcom: scm: Add memory allocation API

[Andy Gross]

On Fri, Apr 22, 2016 at 04:23:31PM -0700, Bjorn Andersson wrote:
> On Fri 22 Apr 15:17 PDT 2016, Andy Gross wrote:
> 
> > This patch adds APIs for the scm-32 and scm-64 to use for coherent memory
> > allocation.
> > 
> > Signed-off-by: Andy Gross <andy.gross@linaro.org>
> 
> This patch must come before the ARM64 implementation.

Oops I got my order a little mixed up.  I can shift it around or just redo the
commit message.  I am going to switch the scm-32 to use this as well.

> 
> > ---
> >  drivers/firmware/qcom_scm.c | 17 +++++++++++++++++
> >  drivers/firmware/qcom_scm.h |  4 ++++
> >  2 files changed, 21 insertions(+)
> > 
> > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> [..]
> > +
> > +void qcom_scm_free_buffer(size_t size, void *cpu_addr,
> > +				 dma_addr_t dma_addr)
> > +{
> > +	if (__scm)
> 
> This would be quite bad and the caller expects that the memory is
> released when you return from here. This should also only happen if the
> arch specific implementation is buggy, so just let it go BANG!

Agreed.

> 
> > +		dma_free_writecombine(__scm->dev, size, cpu_addr, dma_addr);
> > +}
> > +

Thanks for the review

[PATCH 6/8] firmware: qcom: scm: Add memory allocation API

[Andy Gross]

On Fri, Apr 22, 2016 at 04:23:31PM -0700, Bjorn Andersson wrote:
> On Fri 22 Apr 15:17 PDT 2016, Andy Gross wrote:
> 
> > This patch adds APIs for the scm-32 and scm-64 to use for coherent memory
> > allocation.
> > 
> > Signed-off-by: Andy Gross <andy.gross@linaro.org>
> 
> This patch must come before the ARM64 implementation.

Oops I got my order a little mixed up.  I can shift it around or just redo the
commit message.  I am going to switch the scm-32 to use this as well.

> 
> > ---
> >  drivers/firmware/qcom_scm.c | 17 +++++++++++++++++
> >  drivers/firmware/qcom_scm.h |  4 ++++
> >  2 files changed, 21 insertions(+)
> > 
> > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> [..]
> > +
> > +void qcom_scm_free_buffer(size_t size, void *cpu_addr,
> > +				 dma_addr_t dma_addr)
> > +{
> > +	if (__scm)
> 
> This would be quite bad and the caller expects that the memory is
> released when you return from here. This should also only happen if the
> arch specific implementation is buggy, so just let it go BANG!

Agreed.

> 
> > +		dma_free_writecombine(__scm->dev, size, cpu_addr, dma_addr);
> > +}
> > +

Thanks for the review

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

[Andy Gross]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Andy Gross]

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.

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

[Andy Gross]

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.

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

[Andy Gross]

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;
> >  }
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Andy Gross]

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;
> >  }

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

[Andy Gross]

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;
> >  }

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

[Stanimir Varbanov]

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

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

[Stanimir Varbanov]

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

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

[Bjorn Andersson]

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

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

[Bjorn Andersson]

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

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

[Rob Herring]

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

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

[Rob Herring]

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

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

[Rob Herring]

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

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

[Andy Gross]

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.

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

[Andy Gross]

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.

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

[Andy Gross]

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.

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

[Rob Herring]

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

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

[Rob Herring]

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

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

[Rob Herring]

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