linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/6] qcom: add OCMEM support
@ 2019-06-16 13:29 Brian Masney
  2019-06-16 13:29 ` [PATCH 1/6] dt-bindings: soc: qcom: add On Chip MEMory (OCMEM) bindings Brian Masney
                   ` (5 more replies)
  0 siblings, 6 replies; 24+ messages in thread
From: Brian Masney @ 2019-06-16 13:29 UTC (permalink / raw)
  To: agross, david.brown, robdclark, sean, robh+dt
  Cc: bjorn.andersson, airlied, daniel, mark.rutland, jonathan,
	linux-arm-msm, linux-kernel, dri-devel, freedreno, devicetree

This patch series adds support for Qualcomm's On Chip MEMory (OCMEM)
that is needed in order to support some A3xx and A4xx based GPUs
upstream. This is based on Rob Clark's patch series that he submitted
in October 2015 and I am resubmitting updated patches with his
permission.

This was tested with the GPU on a LG Nexus 5 (hammerhead) phone and
this will work on other msm8974-based systems. For a summary of what
currently works upstream on the Nexus 5, see my status page at
https://masneyb.github.io/nexus-5-upstream/.

Brian Masney (3):
  dt-bindings: soc: qcom: add On Chip MEMory (OCMEM) bindings
  dt-bindings: display: msm: gmu: add optional ocmem property
  drm/msm/gpu: add ocmem init/cleanup functions

Rob Clark (3):
  firmware: qcom: scm: add support to restore secure config
  firmware: qcom: scm: add OCMEM lock/unlock interface
  soc: qcom: add OCMEM driver

 .../devicetree/bindings/display/msm/gmu.txt   |   4 +
 .../bindings/soc/qcom/qcom,ocmem.yaml         |  66 +++
 drivers/firmware/qcom_scm-32.c                |  56 +++
 drivers/firmware/qcom_scm-64.c                |  18 +
 drivers/firmware/qcom_scm.c                   |  63 +++
 drivers/firmware/qcom_scm.h                   |  15 +
 drivers/gpu/drm/msm/adreno/a3xx_gpu.c         |  33 +-
 drivers/gpu/drm/msm/adreno/a3xx_gpu.h         |   3 +-
 drivers/gpu/drm/msm/adreno/a4xx_gpu.c         |  30 +-
 drivers/gpu/drm/msm/adreno/a4xx_gpu.h         |   3 +-
 drivers/gpu/drm/msm/adreno/adreno_gpu.c       |  41 ++
 drivers/gpu/drm/msm/adreno/adreno_gpu.h       |  10 +
 drivers/soc/qcom/Kconfig                      |  10 +
 drivers/soc/qcom/Makefile                     |   1 +
 drivers/soc/qcom/ocmem.c                      | 402 ++++++++++++++++++
 drivers/soc/qcom/ocmem.xml.h                  |  86 ++++
 include/linux/qcom_scm.h                      |  28 ++
 include/soc/qcom/ocmem.h                      |  34 ++
 18 files changed, 857 insertions(+), 46 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,ocmem.yaml
 create mode 100644 drivers/soc/qcom/ocmem.c
 create mode 100644 drivers/soc/qcom/ocmem.xml.h
 create mode 100644 include/soc/qcom/ocmem.h

-- 
2.20.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 1/6] dt-bindings: soc: qcom: add On Chip MEMory (OCMEM) bindings
  2019-06-16 13:29 [PATCH 0/6] qcom: add OCMEM support Brian Masney
@ 2019-06-16 13:29 ` Brian Masney
  2019-06-16 17:43   ` Bjorn Andersson
  2019-06-17 14:29   ` Rob Herring
  2019-06-16 13:29 ` [PATCH 2/6] dt-bindings: display: msm: gmu: add optional ocmem property Brian Masney
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 24+ messages in thread
From: Brian Masney @ 2019-06-16 13:29 UTC (permalink / raw)
  To: agross, david.brown, robdclark, sean, robh+dt
  Cc: bjorn.andersson, airlied, daniel, mark.rutland, jonathan,
	linux-arm-msm, linux-kernel, dri-devel, freedreno, devicetree

Add device tree bindings for the On Chip Memory (OCMEM) that is present
on some Qualcomm Snapdragon SoCs.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 .../bindings/soc/qcom/qcom,ocmem.yaml         | 66 +++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,ocmem.yaml

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,ocmem.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,ocmem.yaml
new file mode 100644
index 000000000000..5e3ae6311a16
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,ocmem.yaml
@@ -0,0 +1,66 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/qcom/qcom,ocmem.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: On Chip Memory (OCMEM) that is present on some Qualcomm Snapdragon SoCs.
+
+maintainers:
+  - Brian Masney <masneyb@onstation.org>
+
+description: |
+  The On Chip Memory (OCMEM) allocator allows various clients to allocate memory
+  from OCMEM based on performance, latency and power requirements. This is
+  typically used by the GPU, camera/video, and audio components on some
+  Snapdragon SoCs.
+
+properties:
+  compatible:
+    const: qcom,ocmem-msm8974
+
+  reg:
+    items:
+      - description: Control registers
+      - description: OCMEM address range
+
+  reg-names:
+    items:
+      - const: ocmem_ctrl_physical
+      - const: ocmem_physical
+
+  clocks:
+    items:
+      - description: Core clock
+      - description: Interface clock
+
+  clock-names:
+    items:
+      - const: core
+      - const: iface
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - clocks
+  - clock-names
+
+examples:
+  - |
+      #include <dt-bindings/clock/qcom,rpmcc.h>
+      #include <dt-bindings/clock/qcom,mmcc-msm8974.h>
+
+      ocmem: ocmem@fdd00000 {
+        compatible = "qcom,ocmem-msm8974";
+
+        reg = <0xfdd00000 0x2000>,
+               <0xfec00000 0x180000>;
+        reg-names = "ocmem_ctrl_physical",
+                    "ocmem_physical";
+
+        clocks = <&rpmcc RPM_SMD_OCMEMGX_CLK>,
+                  <&mmcc OCMEMCX_OCMEMNOC_CLK>;
+        clock-names = "core",
+                      "iface";
+      };
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 2/6] dt-bindings: display: msm: gmu: add optional ocmem property
  2019-06-16 13:29 [PATCH 0/6] qcom: add OCMEM support Brian Masney
  2019-06-16 13:29 ` [PATCH 1/6] dt-bindings: soc: qcom: add On Chip MEMory (OCMEM) bindings Brian Masney
@ 2019-06-16 13:29 ` Brian Masney
  2019-06-19 18:06   ` [Freedreno] " Jordan Crouse
  2019-06-19 20:16   ` Rob Herring
  2019-06-16 13:29 ` [PATCH 3/6] firmware: qcom: scm: add support to restore secure config Brian Masney
                   ` (3 subsequent siblings)
  5 siblings, 2 replies; 24+ messages in thread
From: Brian Masney @ 2019-06-16 13:29 UTC (permalink / raw)
  To: agross, david.brown, robdclark, sean, robh+dt
  Cc: bjorn.andersson, airlied, daniel, mark.rutland, jonathan,
	linux-arm-msm, linux-kernel, dri-devel, freedreno, devicetree

Some A3xx and A4xx Adreno GPUs do not have GMEM inside the GPU core and
must use the On Chip MEMory (OCMEM) in order to be functional. Add the
optional ocmem property to the Adreno Graphics Management Unit bindings.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 Documentation/devicetree/bindings/display/msm/gmu.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/msm/gmu.txt b/Documentation/devicetree/bindings/display/msm/gmu.txt
index 90af5b0a56a9..c746b95e95d4 100644
--- a/Documentation/devicetree/bindings/display/msm/gmu.txt
+++ b/Documentation/devicetree/bindings/display/msm/gmu.txt
@@ -31,6 +31,10 @@ Required properties:
 - iommus: phandle to the adreno iommu
 - operating-points-v2: phandle to the OPP operating points
 
+Optional properties:
+- ocmem: phandle to the On Chip Memory (OCMEM) that's present on some Snapdragon
+         SoCs. See Documentation/devicetree/bindings/soc/qcom/qcom,ocmem.yaml.
+
 Example:
 
 / {
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 3/6] firmware: qcom: scm: add support to restore secure config
  2019-06-16 13:29 [PATCH 0/6] qcom: add OCMEM support Brian Masney
  2019-06-16 13:29 ` [PATCH 1/6] dt-bindings: soc: qcom: add On Chip MEMory (OCMEM) bindings Brian Masney
  2019-06-16 13:29 ` [PATCH 2/6] dt-bindings: display: msm: gmu: add optional ocmem property Brian Masney
@ 2019-06-16 13:29 ` Brian Masney
  2019-06-16 17:53   ` Bjorn Andersson
  2019-06-16 13:29 ` [PATCH 4/6] firmware: qcom: scm: add OCMEM lock/unlock interface Brian Masney
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 24+ messages in thread
From: Brian Masney @ 2019-06-16 13:29 UTC (permalink / raw)
  To: agross, david.brown, robdclark, sean, robh+dt
  Cc: bjorn.andersson, airlied, daniel, mark.rutland, jonathan,
	linux-arm-msm, linux-kernel, dri-devel, freedreno, devicetree

From: Rob Clark <robdclark@gmail.com>

Add support to restore the secure configuration that is needed by the
On Chip MEMory (OCMEM) that is present on some Snapdragon devices.

Signed-off-by: Rob Clark <robdclark@gmail.com>
[masneyb@onstation.org: ported to latest kernel; minor reformatting.]
Signed-off-by: Brian Masney <masneyb@onstation.org>
---
Rob's last version of this patch:
https://patchwork.kernel.org/patch/7340701/

 drivers/firmware/qcom_scm-32.c | 21 +++++++++++++++++++++
 drivers/firmware/qcom_scm-64.c |  6 ++++++
 drivers/firmware/qcom_scm.c    | 23 +++++++++++++++++++++++
 drivers/firmware/qcom_scm.h    |  6 ++++++
 include/linux/qcom_scm.h       | 13 +++++++++++++
 5 files changed, 69 insertions(+)

diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
index 215061c581e1..089b47124933 100644
--- a/drivers/firmware/qcom_scm-32.c
+++ b/drivers/firmware/qcom_scm-32.c
@@ -442,6 +442,27 @@ int __qcom_scm_hdcp_req(struct device *dev, struct qcom_scm_hdcp_req *req,
 		req, req_cnt * sizeof(*req), resp, sizeof(*resp));
 }
 
+int __qcom_scm_restore_sec_config(struct device *dev, u32 sec_id,
+				  u32 ctx_bank_num)
+{
+	struct msm_scm_sec_cfg {
+		__le32 id;
+		__le32 ctx_bank_num;
+	} cfg;
+	int ret, scm_ret = 0;
+
+	cfg.id = cpu_to_le32(sec_id);
+	cfg.ctx_bank_num = cpu_to_le32(sec_id);
+
+	ret = qcom_scm_call(dev, QCOM_SCM_MP_SVC, QCOM_SCM_MP_RESTORE_SEC_CFG,
+			    &cfg, sizeof(cfg), &scm_ret, sizeof(scm_ret));
+
+	if (ret || scm_ret)
+		return ret ? ret : -EINVAL;
+
+	return 0;
+}
+
 void __qcom_scm_init(void)
 {
 }
diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
index 91d5ad7cf58b..b6b78da7f9c9 100644
--- a/drivers/firmware/qcom_scm-64.c
+++ b/drivers/firmware/qcom_scm-64.c
@@ -241,6 +241,12 @@ int __qcom_scm_hdcp_req(struct device *dev, struct qcom_scm_hdcp_req *req,
 	return ret;
 }
 
+int __qcom_scm_restore_sec_config(struct device *dev, u32 sec_id,
+				  u32 ctx_bank_num)
+{
+	return -ENOTSUPP;
+}
+
 void __qcom_scm_init(void)
 {
 	u64 cmd;
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 2ddc118dba1b..5495ef994c5d 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -170,6 +170,29 @@ int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp)
 }
 EXPORT_SYMBOL(qcom_scm_hdcp_req);
 
+/**
+ * qcom_scm_restore_sec_config_available() - Check if secure environment
+ * supports restore security config interface.
+ *
+ * Return true if restore-cfg interface is supported, false if not.
+ */
+bool qcom_scm_restore_sec_config_available(void)
+{
+	return __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_MP_SVC,
+					    QCOM_SCM_MP_RESTORE_SEC_CFG);
+}
+EXPORT_SYMBOL(qcom_scm_restore_sec_config_available);
+
+/**
+ * qcom_scm_restore_sec_config() - call restore-cfg interface
+ */
+int qcom_scm_restore_sec_config(struct device *dev,
+				enum qcom_scm_sec_dev_id sec_id)
+{
+	return __qcom_scm_restore_sec_config(dev, sec_id, 0);
+}
+EXPORT_SYMBOL(qcom_scm_restore_sec_config);
+
 /**
  * qcom_scm_pas_supported() - Check if the peripheral authentication service is
  *			      available for the given peripherial
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index 99506bd873c0..bccc7d10c5c2 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -42,6 +42,12 @@ extern int __qcom_scm_hdcp_req(struct device *dev,
 
 extern void __qcom_scm_init(void);
 
+#define QCOM_SCM_MP_SVC			0xc
+#define QCOM_SCM_MP_RESTORE_SEC_CFG	0x2
+
+extern int __qcom_scm_restore_sec_config(struct device *dev, u32 sec_id,
+					 u32 ctx_bank_num);
+
 #define QCOM_SCM_SVC_PIL		0x2
 #define QCOM_SCM_PAS_INIT_IMAGE_CMD	0x1
 #define QCOM_SCM_PAS_MEM_SETUP_CMD	0x2
diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
index 3f12cc77fb58..b5c0afaca955 100644
--- a/include/linux/qcom_scm.h
+++ b/include/linux/qcom_scm.h
@@ -24,6 +24,16 @@ struct qcom_scm_vmperm {
 	int perm;
 };
 
+enum qcom_scm_sec_dev_id {
+	QCOM_SCM_MDSS_DEV_ID	= 1,
+	QCOM_SCM_OCMEM_DEV_ID	= 5,
+	QCOM_SCM_PCIE0_DEV_ID	= 11,
+	QCOM_SCM_PCIE1_DEV_ID	= 12,
+	QCOM_SCM_GFX_DEV_ID	= 18,
+	QCOM_SCM_UFS_DEV_ID	= 19,
+	QCOM_SCM_ICE_DEV_ID	= 20,
+};
+
 #define QCOM_SCM_VMID_HLOS       0x3
 #define QCOM_SCM_VMID_MSS_MSA    0xF
 #define QCOM_SCM_VMID_WLAN       0x18
@@ -41,6 +51,9 @@ extern bool qcom_scm_is_available(void);
 extern bool qcom_scm_hdcp_available(void);
 extern int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt,
 			     u32 *resp);
+extern bool qcom_scm_restore_sec_config_available(void);
+extern int qcom_scm_restore_sec_config(struct device *dev,
+				       enum qcom_scm_sec_dev_id sec_id);
 extern bool qcom_scm_pas_supported(u32 peripheral);
 extern int qcom_scm_pas_init_image(u32 peripheral, const void *metadata,
 				   size_t size);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 4/6] firmware: qcom: scm: add OCMEM lock/unlock interface
  2019-06-16 13:29 [PATCH 0/6] qcom: add OCMEM support Brian Masney
                   ` (2 preceding siblings ...)
  2019-06-16 13:29 ` [PATCH 3/6] firmware: qcom: scm: add support to restore secure config Brian Masney
@ 2019-06-16 13:29 ` Brian Masney
  2019-06-16 17:54   ` Bjorn Andersson
  2019-06-16 13:29 ` [PATCH 5/6] soc: qcom: add OCMEM driver Brian Masney
  2019-06-16 13:29 ` [PATCH 6/6] drm/msm/gpu: add ocmem init/cleanup functions Brian Masney
  5 siblings, 1 reply; 24+ messages in thread
From: Brian Masney @ 2019-06-16 13:29 UTC (permalink / raw)
  To: agross, david.brown, robdclark, sean, robh+dt
  Cc: bjorn.andersson, airlied, daniel, mark.rutland, jonathan,
	linux-arm-msm, linux-kernel, dri-devel, freedreno, devicetree

From: Rob Clark <robdclark@gmail.com>

Add support for the OCMEM lock/unlock interface that is needed by the
On Chip MEMory (OCMEM) that is present on some Snapdragon devices.

Signed-off-by: Rob Clark <robdclark@gmail.com>
[masneyb@onstation.org: ported to latest kernel; minor reformatting.]
Signed-off-by: Brian Masney <masneyb@onstation.org>
---
Rob's last version of this patch:
https://patchwork.kernel.org/patch/7340711/

 drivers/firmware/qcom_scm-32.c | 35 +++++++++++++++++++++++++++++
 drivers/firmware/qcom_scm-64.c | 12 ++++++++++
 drivers/firmware/qcom_scm.c    | 40 ++++++++++++++++++++++++++++++++++
 drivers/firmware/qcom_scm.h    |  9 ++++++++
 include/linux/qcom_scm.h       | 15 +++++++++++++
 5 files changed, 111 insertions(+)

diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
index 089b47124933..0100c82b9c00 100644
--- a/drivers/firmware/qcom_scm-32.c
+++ b/drivers/firmware/qcom_scm-32.c
@@ -463,6 +463,41 @@ int __qcom_scm_restore_sec_config(struct device *dev, u32 sec_id,
 	return 0;
 }
 
+int __qcom_scm_ocmem_lock(struct device *dev, u32 id, u32 offset, u32 size,
+			  u32 mode)
+{
+	struct ocmem_tz_lock {
+		__le32 id;
+		__le32 offset;
+		__le32 size;
+		__le32 mode;
+	} request;
+
+	request.id = cpu_to_le32(id);
+	request.offset = cpu_to_le32(offset);
+	request.size = cpu_to_le32(size);
+	request.mode = cpu_to_le32(mode);
+
+	return qcom_scm_call(dev, QCOM_SCM_OCMEM_SVC, QCOM_SCM_OCMEM_LOCK_CMD,
+			     &request, sizeof(request), NULL, 0);
+}
+
+int __qcom_scm_ocmem_unlock(struct device *dev, u32 id, u32 offset, u32 size)
+{
+	struct ocmem_tz_unlock {
+		__le32 id;
+		__le32 offset;
+		__le32 size;
+	} request;
+
+	request.id = cpu_to_le32(id);
+	request.offset = cpu_to_le32(offset);
+	request.size = cpu_to_le32(size);
+
+	return qcom_scm_call(dev, QCOM_SCM_OCMEM_SVC, QCOM_SCM_OCMEM_UNLOCK_CMD,
+			     &request, sizeof(request), NULL, 0);
+}
+
 void __qcom_scm_init(void)
 {
 }
diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
index b6b78da7f9c9..2674d6d3cdde 100644
--- a/drivers/firmware/qcom_scm-64.c
+++ b/drivers/firmware/qcom_scm-64.c
@@ -247,6 +247,18 @@ int __qcom_scm_restore_sec_config(struct device *dev, u32 sec_id,
 	return -ENOTSUPP;
 }
 
+int __qcom_scm_ocmem_lock(struct device *dev, uint32_t id, uint32_t offset,
+			  uint32_t size, uint32_t mode)
+{
+	return -ENOTSUPP;
+}
+
+int __qcom_scm_ocmem_unlock(struct device *dev, uint32_t id, uint32_t offset,
+			    uint32_t size)
+{
+	return -ENOTSUPP;
+}
+
 void __qcom_scm_init(void)
 {
 	u64 cmd;
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 5495ef994c5d..85afb54defd4 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -193,6 +193,46 @@ int qcom_scm_restore_sec_config(struct device *dev,
 }
 EXPORT_SYMBOL(qcom_scm_restore_sec_config);
 
+/**
+ * qcom_scm_ocmem_lock_available() - is OCMEM lock/unlock interface available
+ */
+bool qcom_scm_ocmem_lock_available(void)
+{
+	return __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_OCMEM_SVC,
+					    QCOM_SCM_OCMEM_LOCK_CMD);
+}
+EXPORT_SYMBOL(qcom_scm_ocmem_lock_available);
+
+/**
+ * qcom_scm_ocmem_lock() - call OCMEM lock interface to assign an OCMEM
+ * region to the specified initiator
+ *
+ * @id:     tz initiator id
+ * @offset: OCMEM offset
+ * @size:   OCMEM size
+ * @mode:   access mode (WIDE/NARROW)
+ */
+int qcom_scm_ocmem_lock(enum qcom_scm_ocmem_client id, u32 offset, u32 size,
+			u32 mode)
+{
+	return __qcom_scm_ocmem_lock(__scm->dev, id, offset, size, mode);
+}
+EXPORT_SYMBOL(qcom_scm_ocmem_lock);
+
+/**
+ * qcom_scm_ocmem_unlock() - call OCMEM unlock interface to release an OCMEM
+ * region from the specified initiator
+ *
+ * @id:     tz initiator id
+ * @offset: OCMEM offset
+ * @size:   OCMEM size
+ */
+int qcom_scm_ocmem_unlock(enum qcom_scm_ocmem_client id, u32 offset, u32 size)
+{
+	return __qcom_scm_ocmem_unlock(__scm->dev, id, offset, size);
+}
+EXPORT_SYMBOL(qcom_scm_ocmem_unlock);
+
 /**
  * qcom_scm_pas_supported() - Check if the peripheral authentication service is
  *			      available for the given peripherial
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index bccc7d10c5c2..387e3c4e33c5 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -48,6 +48,15 @@ extern void __qcom_scm_init(void);
 extern int __qcom_scm_restore_sec_config(struct device *dev, u32 sec_id,
 					 u32 ctx_bank_num);
 
+#define QCOM_SCM_OCMEM_SVC			0xf
+#define QCOM_SCM_OCMEM_LOCK_CMD		0x1
+#define QCOM_SCM_OCMEM_UNLOCK_CMD		0x2
+
+extern int __qcom_scm_ocmem_lock(struct device *dev, u32 id, u32 offset,
+				 u32 size, u32 mode);
+extern int __qcom_scm_ocmem_unlock(struct device *dev, u32 id, u32 offset,
+				   u32 size);
+
 #define QCOM_SCM_SVC_PIL		0x2
 #define QCOM_SCM_PAS_INIT_IMAGE_CMD	0x1
 #define QCOM_SCM_PAS_MEM_SETUP_CMD	0x2
diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
index b5c0afaca955..977c01aa524a 100644
--- a/include/linux/qcom_scm.h
+++ b/include/linux/qcom_scm.h
@@ -34,6 +34,16 @@ enum qcom_scm_sec_dev_id {
 	QCOM_SCM_ICE_DEV_ID	= 20,
 };
 
+enum qcom_scm_ocmem_client {
+	QCOM_SCM_OCMEM_UNUSED_ID = 0x0,
+	QCOM_SCM_OCMEM_GRAPHICS_ID,
+	QCOM_SCM_OCMEM_VIDEO_ID,
+	QCOM_SCM_OCMEM_LP_AUDIO_ID,
+	QCOM_SCM_OCMEM_SENSORS_ID,
+	QCOM_SCM_OCMEM_OTHER_OS_ID,
+	QCOM_SCM_OCMEM_DEBUG_ID,
+};
+
 #define QCOM_SCM_VMID_HLOS       0x3
 #define QCOM_SCM_VMID_MSS_MSA    0xF
 #define QCOM_SCM_VMID_WLAN       0x18
@@ -54,6 +64,11 @@ extern int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt,
 extern bool qcom_scm_restore_sec_config_available(void);
 extern int qcom_scm_restore_sec_config(struct device *dev,
 				       enum qcom_scm_sec_dev_id sec_id);
+extern bool qcom_scm_ocmem_lock_available(void);
+extern int qcom_scm_ocmem_lock(enum qcom_scm_ocmem_client id, u32 offset,
+			       u32 size, u32 mode);
+extern int qcom_scm_ocmem_unlock(enum qcom_scm_ocmem_client id, u32 offset,
+				 u32 size);
 extern bool qcom_scm_pas_supported(u32 peripheral);
 extern int qcom_scm_pas_init_image(u32 peripheral, const void *metadata,
 				   size_t size);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 5/6] soc: qcom: add OCMEM driver
  2019-06-16 13:29 [PATCH 0/6] qcom: add OCMEM support Brian Masney
                   ` (3 preceding siblings ...)
  2019-06-16 13:29 ` [PATCH 4/6] firmware: qcom: scm: add OCMEM lock/unlock interface Brian Masney
@ 2019-06-16 13:29 ` Brian Masney
  2019-06-16 17:41   ` Bjorn Andersson
  2019-06-16 13:29 ` [PATCH 6/6] drm/msm/gpu: add ocmem init/cleanup functions Brian Masney
  5 siblings, 1 reply; 24+ messages in thread
From: Brian Masney @ 2019-06-16 13:29 UTC (permalink / raw)
  To: agross, david.brown, robdclark, sean, robh+dt
  Cc: bjorn.andersson, airlied, daniel, mark.rutland, jonathan,
	linux-arm-msm, linux-kernel, dri-devel, freedreno, devicetree

From: Rob Clark <robdclark@gmail.com>

The OCMEM driver handles allocation and configuration of the On Chip
MEMory that is present on some Snapdragon SoCs.

Devices which have OCMEM do not have GMEM inside the GPU core, so the
GPU must instead use OCMEM to be functional. Since currently the GPU
is the only OCMEM user with an upstream driver, this is just a minimal
implementation sufficient for statically allocating to the GPU it's
chunk of OCMEM.

Signed-off-by: Rob Clark <robdclark@gmail.com>
Co-developed-by: Brian Masney <masneyb@onstation.org>
Signed-off-by: Brian Masney <masneyb@onstation.org>
---
Changes since Rob's last version of this patch:
https://patchwork.kernel.org/patch/7379801/
- reformatted driver to allow multiple instances
- updated logging of error paths during device probing
- remove unused psgsc_ctrl
- remove _clk from clock names
- propagate error code from devm_ioremap_resource()
- use device_get_match_data()
- SPDX license tags
- remove QCOM_SMD in Kconfig
- select ARCH_QCOM in Kconfig
- select QCOM_SCM in Kconfig
- longer description in Kconfig

 drivers/soc/qcom/Kconfig     |  10 +
 drivers/soc/qcom/Makefile    |   1 +
 drivers/soc/qcom/ocmem.c     | 402 +++++++++++++++++++++++++++++++++++
 drivers/soc/qcom/ocmem.xml.h |  86 ++++++++
 include/soc/qcom/ocmem.h     |  34 +++
 5 files changed, 533 insertions(+)
 create mode 100644 drivers/soc/qcom/ocmem.c
 create mode 100644 drivers/soc/qcom/ocmem.xml.h
 create mode 100644 include/soc/qcom/ocmem.h

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 880cf0290962..998d94d60a3c 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -62,6 +62,16 @@ config QCOM_MDT_LOADER
 	tristate
 	select QCOM_SCM
 
+config QCOM_OCMEM
+	tristate "Qualcomm On Chip Memory (OCMEM) driver"
+	depends on ARCH_QCOM
+	select QCOM_SCM
+	help
+          The On Chip Memory (OCMEM) allocator allows various clients to
+          allocate memory from OCMEM based on performance, latency and power
+          requirements. This is typically used by the GPU, camera/video, and
+          audio components on some Snapdragon SoCs.
+
 config QCOM_PM
 	bool "Qualcomm Power Management"
 	depends on ARCH_QCOM && !ARM64
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index ffe519b0cb66..76ac469f548c 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o
 obj-$(CONFIG_QCOM_GLINK_SSR) +=	glink_ssr.o
 obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
 obj-$(CONFIG_QCOM_MDT_LOADER)	+= mdt_loader.o
+obj-$(CONFIG_QCOM_OCMEM)	+= ocmem.o
 obj-$(CONFIG_QCOM_PM)	+=	spm.o
 obj-$(CONFIG_QCOM_QMI_HELPERS)	+= qmi_helpers.o
 qmi_helpers-y	+= qmi_encdec.o qmi_interface.o
diff --git a/drivers/soc/qcom/ocmem.c b/drivers/soc/qcom/ocmem.c
new file mode 100644
index 000000000000..5ebf5031b6c5
--- /dev/null
+++ b/drivers/soc/qcom/ocmem.c
@@ -0,0 +1,402 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2019 Brian Masney <masneyb@onstation.org>
+ * Copyright (C) 2015 Red Hat. Author: Rob Clark <robdclark@gmail.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/qcom_scm.h>
+#include <linux/sizes.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#include <soc/qcom/ocmem.h>
+#include "ocmem.xml.h"
+
+enum region_mode {
+	WIDE_MODE = 0x0,
+	THIN_MODE,
+	MODE_DEFAULT = WIDE_MODE,
+};
+
+struct ocmem_region {
+	bool interleaved;
+	enum region_mode mode;
+	unsigned int num_macros;
+	enum ocmem_macro_state macro_state[4];
+	unsigned long macro_size;
+	unsigned long region_size;
+};
+
+struct ocmem_config {
+	uint8_t num_regions;
+	uint32_t macro_size;
+};
+
+struct ocmem {
+	struct device *dev;
+	const struct ocmem_config *config;
+	struct resource *ocmem_mem;
+	struct clk *core_clk;
+	struct clk *iface_clk;
+	void __iomem *mmio;
+	unsigned int num_ports;
+	unsigned int num_macros;
+	bool interleaved;
+	struct ocmem_region *regions;
+};
+
+#define FIELD(val, name) (((val) & name ## __MASK) >> name ## __SHIFT)
+
+static inline void ocmem_write(struct ocmem *ocmem, u32 reg, u32 data)
+{
+	writel(data, ocmem->mmio + reg);
+}
+
+static inline u32 ocmem_read(struct ocmem *ocmem, u32 reg)
+{
+	return readl(ocmem->mmio + reg);
+}
+
+static int ocmem_clk_enable(struct ocmem *ocmem)
+{
+	int ret;
+
+	ret = clk_prepare_enable(ocmem->core_clk);
+	if (ret) {
+		dev_info(ocmem->dev, "Fail to enable core clk\n");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(ocmem->iface_clk);
+	if (ret) {
+		dev_info(ocmem->dev, "Fail to enable iface clk\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static void ocmem_clk_disable(struct ocmem *ocmem)
+{
+	clk_disable_unprepare(ocmem->iface_clk);
+	clk_disable_unprepare(ocmem->core_clk);
+}
+
+static int ocmem_dev_remove(struct platform_device *pdev)
+{
+	struct ocmem *ocmem = platform_get_drvdata(pdev);
+
+	ocmem_clk_disable(ocmem);
+
+	return 0;
+}
+
+static void update_ocmem(struct ocmem *ocmem)
+{
+	uint32_t region_mode_ctrl = 0x0;
+	unsigned int pos = 0, i = 0;
+
+	if (!qcom_scm_ocmem_lock_available()) {
+		for (i = 0; i < ocmem->config->num_regions; i++) {
+			struct ocmem_region *region = &ocmem->regions[i];
+
+			pos = i << 2;
+			if (region->mode == THIN_MODE)
+				region_mode_ctrl |= BIT(pos);
+		}
+
+		dev_dbg(ocmem->dev, "ocmem_region_mode_control %x\n",
+			region_mode_ctrl);
+		ocmem_write(ocmem, REG_OCMEM_REGION_MODE_CTL, region_mode_ctrl);
+	}
+
+	for (i = 0; i < ocmem->config->num_regions; i++) {
+		struct ocmem_region *region = &ocmem->regions[i];
+		u32 data;
+
+		data = OCMEM_PSGSC_CTL_MACRO0_MODE(region->macro_state[0]) |
+			OCMEM_PSGSC_CTL_MACRO1_MODE(region->macro_state[1]) |
+			OCMEM_PSGSC_CTL_MACRO2_MODE(region->macro_state[2]) |
+			OCMEM_PSGSC_CTL_MACRO3_MODE(region->macro_state[3]);
+
+		ocmem_write(ocmem, REG_OCMEM_PSGSC_CTL(i), data);
+	}
+}
+
+static unsigned long phys_to_offset(struct ocmem *ocmem,
+				    unsigned long addr)
+{
+	if (addr < ocmem->ocmem_mem->start || addr >= ocmem->ocmem_mem->end)
+		return 0;
+
+	return addr - ocmem->ocmem_mem->start;
+}
+
+static unsigned long device_address(struct ocmem *ocmem,
+				    enum ocmem_client client,
+				    unsigned long addr)
+{
+	/* TODO, gpu uses phys_to_offset, but others do not.. */
+	return phys_to_offset(ocmem, addr);
+}
+
+static void update_range(struct ocmem *ocmem, struct ocmem_buf *buf,
+			 enum ocmem_macro_state mstate, enum region_mode rmode)
+{
+	unsigned long offset = 0;
+	int i, j;
+
+	/*
+	 * TODO probably should assert somewhere that range is aligned
+	 * to macro boundaries..
+	 */
+
+	for (i = 0; i < ocmem->config->num_regions; i++) {
+		struct ocmem_region *region = &ocmem->regions[i];
+
+		if (buf->offset <= offset && offset < buf->offset + buf->len)
+			region->mode = rmode;
+
+		for (j = 0; j < region->num_macros; j++) {
+			if (buf->offset <= offset &&
+			    offset < buf->offset + buf->len)
+				region->macro_state[j] = mstate;
+
+			offset += region->macro_size;
+		}
+	}
+
+	update_ocmem(ocmem);
+}
+
+struct ocmem *of_get_ocmem(struct device *dev)
+{
+	struct platform_device *pdev;
+	struct device_node *devnode;
+
+	devnode = of_parse_phandle(dev->of_node, "ocmem", 0);
+	if (!devnode) {
+		dev_err(dev, "Cannot look up ocmem phandle\n");
+		return NULL;
+	}
+
+	pdev = of_find_device_by_node(devnode);
+	if (!pdev) {
+		dev_err(dev, "Cannot find device node %s\n", devnode->name);
+		return NULL;
+	}
+
+	return platform_get_drvdata(pdev);
+}
+EXPORT_SYMBOL(of_get_ocmem);
+
+struct ocmem_buf *ocmem_allocate(struct ocmem *ocmem, enum ocmem_client client,
+				 unsigned long size)
+{
+	struct ocmem_buf *buf;
+
+	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
+	if (!buf)
+		return ERR_PTR(-ENOMEM);
+
+	buf->offset = 0;
+	buf->addr = device_address(ocmem, client, buf->offset);
+	buf->len = size;
+
+	update_range(ocmem, buf, CORE_ON, WIDE_MODE);
+
+	if (qcom_scm_ocmem_lock_available()) {
+		int ret;
+
+		ret = qcom_scm_ocmem_lock(QCOM_SCM_OCMEM_GRAPHICS_ID,
+					  buf->offset, buf->len, WIDE_MODE);
+		if (ret)
+			dev_err(ocmem->dev, "could not lock: %d\n", ret);
+	} else {
+		if (client == OCMEM_GRAPHICS) {
+			ocmem_write(ocmem, REG_OCMEM_GFX_MPU_START,
+				    buf->offset);
+			ocmem_write(ocmem, REG_OCMEM_GFX_MPU_END,
+				    buf->offset + buf->len);
+		}
+	}
+
+	return buf;
+}
+EXPORT_SYMBOL(ocmem_allocate);
+
+void ocmem_free(struct ocmem *ocmem, enum ocmem_client client,
+		struct ocmem_buf *buf)
+{
+	update_range(ocmem, buf, CLK_OFF, MODE_DEFAULT);
+
+	if (qcom_scm_ocmem_lock_available()) {
+		int ret;
+
+		ret = qcom_scm_ocmem_unlock(QCOM_SCM_OCMEM_GRAPHICS_ID,
+					    buf->offset, buf->len);
+		if (ret)
+			dev_err(ocmem->dev, "could not unlock: %d\n", ret);
+	} else {
+		if (client == OCMEM_GRAPHICS) {
+			ocmem_write(ocmem, REG_OCMEM_GFX_MPU_START, 0x0);
+			ocmem_write(ocmem, REG_OCMEM_GFX_MPU_END, 0x0);
+		}
+	}
+
+	kfree(buf);
+}
+EXPORT_SYMBOL(ocmem_free);
+
+static int ocmem_dev_probe(struct platform_device *pdev)
+{
+	struct ocmem *ocmem;
+	uint32_t reg, num_banks, region_size;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	int i, j, ret;
+
+	if (!qcom_scm_is_available())
+		return -EPROBE_DEFER;
+
+	ocmem = devm_kzalloc(dev, sizeof(*ocmem), GFP_KERNEL);
+	if (!ocmem)
+		return -ENOMEM;
+
+	ocmem->dev = dev;
+	ocmem->config = device_get_match_data(dev);
+
+	ocmem->core_clk = devm_clk_get(dev, "core");
+	if (IS_ERR(ocmem->core_clk)) {
+		if (PTR_ERR(ocmem->core_clk) != -EPROBE_DEFER)
+			dev_err(dev, "Unable to get the core clock\n");
+
+		return PTR_ERR(ocmem->core_clk);
+	}
+
+	ocmem->iface_clk = devm_clk_get(dev, "iface");
+	if (IS_ERR(ocmem->iface_clk)) {
+		if (PTR_ERR(ocmem->iface_clk) != -EPROBE_DEFER)
+			dev_err(dev, "Unable to get the iface clock\n");
+
+		return PTR_ERR(ocmem->iface_clk);
+	}
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+					   "ocmem_ctrl_physical");
+	if (!res) {
+		dev_err(dev, "Could not get ocmem_ctrl_physical region\n");
+		return -ENXIO;
+	}
+
+	ocmem->mmio = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(ocmem->mmio)) {
+		dev_err(&pdev->dev,
+			"Failed to ioremap ocmem_ctrl_physical resource\n");
+		return PTR_ERR(ocmem->mmio);
+	}
+
+	ocmem->ocmem_mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+							"ocmem_physical");
+	if (!ocmem->ocmem_mem) {
+		dev_err(dev, "Could not get ocmem_physical region\n");
+		return -ENXIO;
+	}
+
+	/* The core clock is synchronous with graphics */
+	WARN_ON(clk_set_rate(ocmem->core_clk, 1000) < 0);
+
+	ret = ocmem_clk_enable(ocmem);
+	if (ret)
+		return ret;
+
+	if (qcom_scm_restore_sec_config_available()) {
+		dev_dbg(dev, "configuring scm\n");
+		ret = qcom_scm_restore_sec_config(&pdev->dev,
+						  QCOM_SCM_OCMEM_DEV_ID);
+		if (ret) {
+			dev_err(dev, "Could not enable secure configuration\n");
+			goto err_clk_disable;
+		}
+	}
+
+	reg = ocmem_read(ocmem, REG_OCMEM_HW_PROFILE);
+	ocmem->num_ports = FIELD(reg, OCMEM_HW_PROFILE_NUM_PORTS);
+	ocmem->num_macros = FIELD(reg, OCMEM_HW_PROFILE_NUM_MACROS);
+	ocmem->interleaved = !!(reg & OCMEM_HW_PROFILE_INTERLEAVING);
+
+	num_banks = ocmem->num_ports / 2;
+	region_size = ocmem->config->macro_size * num_banks;
+
+	dev_info(dev, "%u ports, %u regions, %u macros, %sinterleaved\n",
+		 ocmem->num_ports, ocmem->config->num_regions,
+		 ocmem->num_macros, ocmem->interleaved ? "" : "not ");
+
+	ocmem->regions = devm_kcalloc(dev, ocmem->config->num_regions,
+				      sizeof(struct ocmem_region), GFP_KERNEL);
+	if (!ocmem->regions) {
+		ret = -ENOMEM;
+		goto err_clk_disable;
+	}
+
+	for (i = 0; i < ocmem->config->num_regions; i++) {
+		struct ocmem_region *region = &ocmem->regions[i];
+
+		if (WARN_ON(num_banks > ARRAY_SIZE(region->macro_state))) {
+			ret = -EINVAL;
+			goto err_clk_disable;
+		}
+
+		region->mode = MODE_DEFAULT;
+		region->num_macros = num_banks;
+
+		if (i == (ocmem->config->num_regions - 1) &&
+		    reg & OCMEM_HW_PROFILE_LAST_REGN_HALFSIZE) {
+			region->macro_size = ocmem->config->macro_size / 2;
+			region->region_size = region_size / 2;
+		} else {
+			region->macro_size = ocmem->config->macro_size;
+			region->region_size = region_size;
+		}
+
+		for (j = 0; j < ARRAY_SIZE(region->macro_state); j++)
+			region->macro_state[j] = CLK_OFF;
+	}
+
+	platform_set_drvdata(pdev, ocmem);
+
+	return 0;
+
+err_clk_disable:
+	ocmem_clk_disable(ocmem);
+	return ret;
+}
+
+static const struct ocmem_config ocmem_8974_config = {
+	.num_regions = 3,
+	.macro_size = SZ_128K,
+};
+
+static const struct of_device_id ocmem_of_match[] = {
+	{ .compatible = "qcom,ocmem-msm8974", .data = &ocmem_8974_config },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(of, ocmem_of_match);
+
+static struct platform_driver ocmem_driver = {
+	.probe = ocmem_dev_probe,
+	.remove = ocmem_dev_remove,
+	.driver = {
+		.name = "ocmem",
+		.of_match_table = ocmem_of_match,
+	},
+};
+
+module_platform_driver(ocmem_driver);
diff --git a/drivers/soc/qcom/ocmem.xml.h b/drivers/soc/qcom/ocmem.xml.h
new file mode 100644
index 000000000000..b4bfb85d1e33
--- /dev/null
+++ b/drivers/soc/qcom/ocmem.xml.h
@@ -0,0 +1,86 @@
+/* SPDX-License-Identifier: MIT */
+
+#ifndef OCMEM_XML
+#define OCMEM_XML
+
+/* Autogenerated file, DO NOT EDIT manually!
+
+This file was generated by the rules-ng-ng headergen tool in this git repository:
+http://github.com/freedreno/envytools/
+git clone https://github.com/freedreno/envytools.git
+
+The rules-ng-ng source files this header was generated from are:
+- /home/robclark/src/freedreno/envytools/rnndb/adreno/ocmem.xml         (   1773 bytes, from 2015-09-24 17:30:00)
+
+Copyright (C) 2013-2015 by the following authors:
+- Rob Clark <robdclark@gmail.com> (robclark)
+*/
+
+enum ocmem_macro_state {
+	PASSTHROUGH = 0,
+	PERI_ON = 1,
+	CORE_ON = 2,
+	CLK_OFF = 4,
+};
+
+#define REG_OCMEM_HW_VERSION					0x00000000
+
+#define REG_OCMEM_HW_PROFILE					0x00000004
+#define OCMEM_HW_PROFILE_NUM_PORTS__MASK			0x0000000f
+#define OCMEM_HW_PROFILE_NUM_PORTS__SHIFT			0
+static inline uint32_t OCMEM_HW_PROFILE_NUM_PORTS(uint32_t val)
+{
+	return ((val) << OCMEM_HW_PROFILE_NUM_PORTS__SHIFT) & OCMEM_HW_PROFILE_NUM_PORTS__MASK;
+}
+#define OCMEM_HW_PROFILE_NUM_MACROS__MASK			0x00003f00
+#define OCMEM_HW_PROFILE_NUM_MACROS__SHIFT			8
+static inline uint32_t OCMEM_HW_PROFILE_NUM_MACROS(uint32_t val)
+{
+	return ((val) << OCMEM_HW_PROFILE_NUM_MACROS__SHIFT) & OCMEM_HW_PROFILE_NUM_MACROS__MASK;
+}
+#define OCMEM_HW_PROFILE_LAST_REGN_HALFSIZE			0x00010000
+#define OCMEM_HW_PROFILE_INTERLEAVING				0x00020000
+
+#define REG_OCMEM_GEN_STATUS					0x0000000c
+
+#define REG_OCMEM_PSGSC_STATUS					0x00000038
+
+static inline uint32_t REG_OCMEM_PSGSC(uint32_t i0) { return 0x0000003c + 0x1*i0; }
+
+static inline uint32_t REG_OCMEM_PSGSC_CTL(uint32_t i0) { return 0x0000003c + 0x1*i0; }
+#define OCMEM_PSGSC_CTL_MACRO0_MODE__MASK			0x00000007
+#define OCMEM_PSGSC_CTL_MACRO0_MODE__SHIFT			0
+static inline uint32_t OCMEM_PSGSC_CTL_MACRO0_MODE(enum ocmem_macro_state val)
+{
+	return ((val) << OCMEM_PSGSC_CTL_MACRO0_MODE__SHIFT) & OCMEM_PSGSC_CTL_MACRO0_MODE__MASK;
+}
+#define OCMEM_PSGSC_CTL_MACRO1_MODE__MASK			0x00000070
+#define OCMEM_PSGSC_CTL_MACRO1_MODE__SHIFT			4
+static inline uint32_t OCMEM_PSGSC_CTL_MACRO1_MODE(enum ocmem_macro_state val)
+{
+	return ((val) << OCMEM_PSGSC_CTL_MACRO1_MODE__SHIFT) & OCMEM_PSGSC_CTL_MACRO1_MODE__MASK;
+}
+#define OCMEM_PSGSC_CTL_MACRO2_MODE__MASK			0x00000700
+#define OCMEM_PSGSC_CTL_MACRO2_MODE__SHIFT			8
+static inline uint32_t OCMEM_PSGSC_CTL_MACRO2_MODE(enum ocmem_macro_state val)
+{
+	return ((val) << OCMEM_PSGSC_CTL_MACRO2_MODE__SHIFT) & OCMEM_PSGSC_CTL_MACRO2_MODE__MASK;
+}
+#define OCMEM_PSGSC_CTL_MACRO3_MODE__MASK			0x00007000
+#define OCMEM_PSGSC_CTL_MACRO3_MODE__SHIFT			12
+static inline uint32_t OCMEM_PSGSC_CTL_MACRO3_MODE(enum ocmem_macro_state val)
+{
+	return ((val) << OCMEM_PSGSC_CTL_MACRO3_MODE__SHIFT) & OCMEM_PSGSC_CTL_MACRO3_MODE__MASK;
+}
+
+#define REG_OCMEM_REGION_MODE_CTL				0x00001000
+#define OCMEM_REGION_MODE_CTL_REG0_THIN				0x00000001
+#define OCMEM_REGION_MODE_CTL_REG1_THIN				0x00000002
+#define OCMEM_REGION_MODE_CTL_REG2_THIN				0x00000004
+#define OCMEM_REGION_MODE_CTL_REG3_THIN				0x00000008
+
+#define REG_OCMEM_GFX_MPU_START					0x00001004
+
+#define REG_OCMEM_GFX_MPU_END					0x00001008
+
+#endif /* OCMEM_XML */
diff --git a/include/soc/qcom/ocmem.h b/include/soc/qcom/ocmem.h
new file mode 100644
index 000000000000..e56ce220096d
--- /dev/null
+++ b/include/soc/qcom/ocmem.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2015 Red Hat
+ * Author: Rob Clark <robdclark@gmail.com>
+ */
+
+#ifndef __OCMEM_H__
+#define __OCMEM_H__
+
+enum ocmem_client {
+	/* GMEM clients */
+	OCMEM_GRAPHICS = 0x0,
+	/*
+	 * TODO add more once ocmem_allocate() is clever enough to
+	 * deal with multiple clients.
+	 */
+	OCMEM_CLIENT_MAX,
+};
+
+struct ocmem;
+
+struct ocmem_buf {
+	unsigned long offset;
+	unsigned long addr;
+	unsigned long len;
+};
+
+struct ocmem *of_get_ocmem(struct device *dev);
+struct ocmem_buf *ocmem_allocate(struct ocmem *ocmem, enum ocmem_client client,
+				 unsigned long size);
+void ocmem_free(struct ocmem *ocmem, enum ocmem_client client,
+		struct ocmem_buf *buf);
+
+#endif /* __OCMEM_H__ */
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 6/6] drm/msm/gpu: add ocmem init/cleanup functions
  2019-06-16 13:29 [PATCH 0/6] qcom: add OCMEM support Brian Masney
                   ` (4 preceding siblings ...)
  2019-06-16 13:29 ` [PATCH 5/6] soc: qcom: add OCMEM driver Brian Masney
@ 2019-06-16 13:29 ` Brian Masney
  2019-06-16 18:06   ` Bjorn Andersson
  2019-06-19 18:15   ` [Freedreno] " Jordan Crouse
  5 siblings, 2 replies; 24+ messages in thread
From: Brian Masney @ 2019-06-16 13:29 UTC (permalink / raw)
  To: agross, david.brown, robdclark, sean, robh+dt
  Cc: bjorn.andersson, airlied, daniel, mark.rutland, jonathan,
	linux-arm-msm, linux-kernel, dri-devel, freedreno, devicetree

The files a3xx_gpu.c and a4xx_gpu.c have ifdefs for the OCMEM support
that was missing upstream. Add two new functions (adreno_gpu_ocmem_init
and adreno_gpu_ocmem_cleanup) that removes some duplicated code. We also
need to change the ifdef check for CONFIG_MSM_OCMEM to CONFIG_QCOM_OCMEM
now that OCMEM support is upstream.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 drivers/gpu/drm/msm/adreno/a3xx_gpu.c   | 33 +++++++-------------
 drivers/gpu/drm/msm/adreno/a3xx_gpu.h   |  3 +-
 drivers/gpu/drm/msm/adreno/a4xx_gpu.c   | 30 ++++++------------
 drivers/gpu/drm/msm/adreno/a4xx_gpu.h   |  3 +-
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 41 +++++++++++++++++++++++++
 drivers/gpu/drm/msm/adreno/adreno_gpu.h | 10 ++++++
 6 files changed, 74 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
index c3b4bc6e4155..72720bb2aca1 100644
--- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
@@ -17,10 +17,6 @@
  * this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#ifdef CONFIG_MSM_OCMEM
-#  include <mach/ocmem.h>
-#endif
-
 #include "a3xx_gpu.h"
 
 #define A3XX_INT0_MASK \
@@ -206,9 +202,9 @@ static int a3xx_hw_init(struct msm_gpu *gpu)
 		gpu_write(gpu, REG_A3XX_RBBM_GPR0_CTL, 0x00000000);
 
 	/* Set the OCMEM base address for A330, etc */
-	if (a3xx_gpu->ocmem_hdl) {
+	if (a3xx_gpu->ocmem.hdl) {
 		gpu_write(gpu, REG_A3XX_RB_GMEM_BASE_ADDR,
-			(unsigned int)(a3xx_gpu->ocmem_base >> 14));
+			(unsigned int)(a3xx_gpu->ocmem.base >> 14));
 	}
 
 	/* Turn on performance counters: */
@@ -329,10 +325,7 @@ static void a3xx_destroy(struct msm_gpu *gpu)
 
 	adreno_gpu_cleanup(adreno_gpu);
 
-#ifdef CONFIG_MSM_OCMEM
-	if (a3xx_gpu->ocmem_base)
-		ocmem_free(OCMEM_GRAPHICS, a3xx_gpu->ocmem_hdl);
-#endif
+	adreno_gpu_ocmem_cleanup(&a3xx_gpu->ocmem);
 
 	kfree(a3xx_gpu);
 }
@@ -507,17 +500,10 @@ struct msm_gpu *a3xx_gpu_init(struct drm_device *dev)
 
 	/* if needed, allocate gmem: */
 	if (adreno_is_a330(adreno_gpu)) {
-#ifdef CONFIG_MSM_OCMEM
-		/* TODO this is different/missing upstream: */
-		struct ocmem_buf *ocmem_hdl =
-				ocmem_allocate(OCMEM_GRAPHICS, adreno_gpu->gmem);
-
-		a3xx_gpu->ocmem_hdl = ocmem_hdl;
-		a3xx_gpu->ocmem_base = ocmem_hdl->addr;
-		adreno_gpu->gmem = ocmem_hdl->len;
-		DBG("using %dK of OCMEM at 0x%08x", adreno_gpu->gmem / 1024,
-				a3xx_gpu->ocmem_base);
-#endif
+		ret = adreno_gpu_ocmem_init(&adreno_gpu->base.pdev->dev,
+					    adreno_gpu, &a3xx_gpu->ocmem);
+		if (ret)
+			goto fail;
 	}
 
 	if (!gpu->aspace) {
@@ -530,11 +516,14 @@ struct msm_gpu *a3xx_gpu_init(struct drm_device *dev)
 		 */
 		DRM_DEV_ERROR(dev->dev, "No memory protection without IOMMU\n");
 		ret = -ENXIO;
-		goto fail;
+		goto fail_cleanup_ocmem;
 	}
 
 	return gpu;
 
+fail_cleanup_ocmem:
+	adreno_gpu_ocmem_cleanup(&a3xx_gpu->ocmem);
+
 fail:
 	if (a3xx_gpu)
 		a3xx_destroy(&a3xx_gpu->base.base);
diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.h b/drivers/gpu/drm/msm/adreno/a3xx_gpu.h
index ab60dc9e344e..727c34f38f9e 100644
--- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.h
@@ -30,8 +30,7 @@ struct a3xx_gpu {
 	struct adreno_gpu base;
 
 	/* if OCMEM is used for GMEM: */
-	uint32_t ocmem_base;
-	void *ocmem_hdl;
+	struct adreno_ocmem ocmem;
 };
 #define to_a3xx_gpu(x) container_of(x, struct a3xx_gpu, base)
 
diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
index ab2b752566d8..b8f825107796 100644
--- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
@@ -2,9 +2,6 @@
 /* Copyright (c) 2014 The Linux Foundation. All rights reserved.
  */
 #include "a4xx_gpu.h"
-#ifdef CONFIG_MSM_OCMEM
-#  include <soc/qcom/ocmem.h>
-#endif
 
 #define A4XX_INT0_MASK \
 	(A4XX_INT0_RBBM_AHB_ERROR |        \
@@ -188,7 +185,7 @@ static int a4xx_hw_init(struct msm_gpu *gpu)
 			(1 << 30) | 0xFFFF);
 
 	gpu_write(gpu, REG_A4XX_RB_GMEM_BASE_ADDR,
-			(unsigned int)(a4xx_gpu->ocmem_base >> 14));
+			(unsigned int)(a4xx_gpu->ocmem.base >> 14));
 
 	/* Turn on performance counters: */
 	gpu_write(gpu, REG_A4XX_RBBM_PERFCTR_CTL, 0x01);
@@ -318,10 +315,7 @@ static void a4xx_destroy(struct msm_gpu *gpu)
 
 	adreno_gpu_cleanup(adreno_gpu);
 
-#ifdef CONFIG_MSM_OCMEM
-	if (a4xx_gpu->ocmem_base)
-		ocmem_free(OCMEM_GRAPHICS, a4xx_gpu->ocmem_hdl);
-#endif
+	adreno_gpu_ocmem_cleanup(&a4xx_gpu->ocmem);
 
 	kfree(a4xx_gpu);
 }
@@ -578,17 +572,10 @@ struct msm_gpu *a4xx_gpu_init(struct drm_device *dev)
 
 	/* if needed, allocate gmem: */
 	if (adreno_is_a4xx(adreno_gpu)) {
-#ifdef CONFIG_MSM_OCMEM
-		/* TODO this is different/missing upstream: */
-		struct ocmem_buf *ocmem_hdl =
-				ocmem_allocate(OCMEM_GRAPHICS, adreno_gpu->gmem);
-
-		a4xx_gpu->ocmem_hdl = ocmem_hdl;
-		a4xx_gpu->ocmem_base = ocmem_hdl->addr;
-		adreno_gpu->gmem = ocmem_hdl->len;
-		DBG("using %dK of OCMEM at 0x%08x", adreno_gpu->gmem / 1024,
-				a4xx_gpu->ocmem_base);
-#endif
+		ret = adreno_gpu_ocmem_init(dev->dev, adreno_gpu,
+					    &a4xx_gpu->ocmem);
+		if (ret)
+			goto fail;
 	}
 
 	if (!gpu->aspace) {
@@ -601,11 +588,14 @@ struct msm_gpu *a4xx_gpu_init(struct drm_device *dev)
 		 */
 		DRM_DEV_ERROR(dev->dev, "No memory protection without IOMMU\n");
 		ret = -ENXIO;
-		goto fail;
+		goto fail_cleanup_ocmem;
 	}
 
 	return gpu;
 
+fail_cleanup_ocmem:
+	adreno_gpu_ocmem_cleanup(&a4xx_gpu->ocmem);
+
 fail:
 	if (a4xx_gpu)
 		a4xx_destroy(&a4xx_gpu->base.base);
diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.h b/drivers/gpu/drm/msm/adreno/a4xx_gpu.h
index d506311ee240..a01448cba2ea 100644
--- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.h
@@ -16,8 +16,7 @@ struct a4xx_gpu {
 	struct adreno_gpu base;
 
 	/* if OCMEM is used for GMEM: */
-	uint32_t ocmem_base;
-	void *ocmem_hdl;
+	struct adreno_ocmem ocmem;
 };
 #define to_a4xx_gpu(x) container_of(x, struct a4xx_gpu, base)
 
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 6f7f4114afcf..e0a9409c8a32 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -29,6 +29,10 @@
 #include "msm_gem.h"
 #include "msm_mmu.h"
 
+#ifdef CONFIG_QCOM_OCMEM
+#  include <soc/qcom/ocmem.h>
+#endif
+
 static bool zap_available = true;
 
 static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,
@@ -897,6 +901,43 @@ static int adreno_get_pwrlevels(struct device *dev,
 	return 0;
 }
 
+int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,
+			  struct adreno_ocmem *adreno_ocmem)
+{
+#ifdef CONFIG_QCOM_OCMEM
+	struct ocmem_buf *ocmem_hdl;
+	struct ocmem *ocmem;
+
+	ocmem = of_get_ocmem(dev);
+	if (!ocmem) {
+		/* This is an optional property so return success. */
+		return 0;
+	}
+
+	ocmem_hdl = ocmem_allocate(ocmem, OCMEM_GRAPHICS, adreno_gpu->gmem);
+	if (IS_ERR(ocmem_hdl))
+		return PTR_ERR(ocmem_hdl);
+
+	adreno_ocmem->ocmem = ocmem;
+	adreno_ocmem->base = ocmem_hdl->addr;
+	adreno_ocmem->hdl = ocmem_hdl;
+	adreno_gpu->gmem = ocmem_hdl->len;
+	DBG("using %dK of OCMEM at 0x%08x", adreno_gpu->gmem / 1024,
+	    adreno_ocmem->base);
+#endif
+
+	return 0;
+}
+
+void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *adreno_ocmem)
+{
+#ifdef CONFIG_QCOM_OCMEM
+	if (adreno_ocmem->base)
+		ocmem_free(adreno_ocmem->ocmem, OCMEM_GRAPHICS,
+			   adreno_ocmem->hdl);
+#endif
+}
+
 int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
 		struct adreno_gpu *adreno_gpu,
 		const struct adreno_gpu_funcs *funcs, int nr_rings)
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index 0925606ec9b5..1cd11570323b 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -136,6 +136,12 @@ struct adreno_gpu {
 };
 #define to_adreno_gpu(x) container_of(x, struct adreno_gpu, base)
 
+struct adreno_ocmem {
+	struct ocmem *ocmem;
+	uint32_t base;
+	void *hdl;
+};
+
 /* platform config data (ie. from DT, or pdata) */
 struct adreno_platform_config {
 	struct adreno_rev rev;
@@ -241,6 +247,10 @@ void adreno_dump(struct msm_gpu *gpu);
 void adreno_wait_ring(struct msm_ringbuffer *ring, uint32_t ndwords);
 struct msm_ringbuffer *adreno_active_ring(struct msm_gpu *gpu);
 
+int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,
+			  struct adreno_ocmem *ocmem);
+void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *ocmem);
+
 int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
 		struct adreno_gpu *gpu, const struct adreno_gpu_funcs *funcs,
 		int nr_rings);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH 5/6] soc: qcom: add OCMEM driver
  2019-06-16 13:29 ` [PATCH 5/6] soc: qcom: add OCMEM driver Brian Masney
@ 2019-06-16 17:41   ` Bjorn Andersson
  2019-06-18  2:02     ` Brian Masney
  0 siblings, 1 reply; 24+ messages in thread
From: Bjorn Andersson @ 2019-06-16 17:41 UTC (permalink / raw)
  To: Brian Masney
  Cc: agross, david.brown, robdclark, sean, robh+dt, airlied, daniel,
	mark.rutland, jonathan, linux-arm-msm, linux-kernel, dri-devel,
	freedreno, devicetree

On Sun 16 Jun 06:29 PDT 2019, Brian Masney wrote:

> From: Rob Clark <robdclark@gmail.com>
> 
> The OCMEM driver handles allocation and configuration of the On Chip
> MEMory that is present on some Snapdragon SoCs.
> 
> Devices which have OCMEM do not have GMEM inside the GPU core, so the
> GPU must instead use OCMEM to be functional. Since currently the GPU
> is the only OCMEM user with an upstream driver, this is just a minimal
> implementation sufficient for statically allocating to the GPU it's
> chunk of OCMEM.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> Co-developed-by: Brian Masney <masneyb@onstation.org>
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
> Changes since Rob's last version of this patch:
> https://patchwork.kernel.org/patch/7379801/
> - reformatted driver to allow multiple instances
> - updated logging of error paths during device probing
> - remove unused psgsc_ctrl
> - remove _clk from clock names
> - propagate error code from devm_ioremap_resource()
> - use device_get_match_data()
> - SPDX license tags
> - remove QCOM_SMD in Kconfig
> - select ARCH_QCOM in Kconfig
> - select QCOM_SCM in Kconfig
> - longer description in Kconfig
> 
>  drivers/soc/qcom/Kconfig     |  10 +
>  drivers/soc/qcom/Makefile    |   1 +
>  drivers/soc/qcom/ocmem.c     | 402 +++++++++++++++++++++++++++++++++++
>  drivers/soc/qcom/ocmem.xml.h |  86 ++++++++
>  include/soc/qcom/ocmem.h     |  34 +++
>  5 files changed, 533 insertions(+)
>  create mode 100644 drivers/soc/qcom/ocmem.c
>  create mode 100644 drivers/soc/qcom/ocmem.xml.h
>  create mode 100644 include/soc/qcom/ocmem.h
> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 880cf0290962..998d94d60a3c 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -62,6 +62,16 @@ config QCOM_MDT_LOADER
>  	tristate
>  	select QCOM_SCM
>  
> +config QCOM_OCMEM
> +	tristate "Qualcomm On Chip Memory (OCMEM) driver"
> +	depends on ARCH_QCOM
> +	select QCOM_SCM
> +	help
> +          The On Chip Memory (OCMEM) allocator allows various clients to
> +          allocate memory from OCMEM based on performance, latency and power
> +          requirements. This is typically used by the GPU, camera/video, and
> +          audio components on some Snapdragon SoCs.
> +
>  config QCOM_PM
>  	bool "Qualcomm Power Management"
>  	depends on ARCH_QCOM && !ARM64
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index ffe519b0cb66..76ac469f548c 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -5,6 +5,7 @@ obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o
>  obj-$(CONFIG_QCOM_GLINK_SSR) +=	glink_ssr.o
>  obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
>  obj-$(CONFIG_QCOM_MDT_LOADER)	+= mdt_loader.o
> +obj-$(CONFIG_QCOM_OCMEM)	+= ocmem.o
>  obj-$(CONFIG_QCOM_PM)	+=	spm.o
>  obj-$(CONFIG_QCOM_QMI_HELPERS)	+= qmi_helpers.o
>  qmi_helpers-y	+= qmi_encdec.o qmi_interface.o
> diff --git a/drivers/soc/qcom/ocmem.c b/drivers/soc/qcom/ocmem.c
> new file mode 100644
> index 000000000000..5ebf5031b6c5
> --- /dev/null
> +++ b/drivers/soc/qcom/ocmem.c
> @@ -0,0 +1,402 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2019 Brian Masney <masneyb@onstation.org>
> + * Copyright (C) 2015 Red Hat. Author: Rob Clark <robdclark@gmail.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/qcom_scm.h>
> +#include <linux/sizes.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +
> +#include <soc/qcom/ocmem.h>
> +#include "ocmem.xml.h"
> +
> +enum region_mode {
> +	WIDE_MODE = 0x0,
> +	THIN_MODE,
> +	MODE_DEFAULT = WIDE_MODE,
> +};
> +
> +struct ocmem_region {
> +	bool interleaved;
> +	enum region_mode mode;
> +	unsigned int num_macros;
> +	enum ocmem_macro_state macro_state[4];
> +	unsigned long macro_size;
> +	unsigned long region_size;
> +};
> +
> +struct ocmem_config {
> +	uint8_t num_regions;
> +	uint32_t macro_size;
> +};
> +
> +struct ocmem {
> +	struct device *dev;
> +	const struct ocmem_config *config;
> +	struct resource *ocmem_mem;
> +	struct clk *core_clk;
> +	struct clk *iface_clk;
> +	void __iomem *mmio;
> +	unsigned int num_ports;
> +	unsigned int num_macros;
> +	bool interleaved;
> +	struct ocmem_region *regions;
> +};
> +
> +#define FIELD(val, name) (((val) & name ## __MASK) >> name ## __SHIFT)

include/linux/bitfield.h has standard macros for this, please use that
instead.

> +
> +static inline void ocmem_write(struct ocmem *ocmem, u32 reg, u32 data)
> +{
> +	writel(data, ocmem->mmio + reg);
> +}
> +
> +static inline u32 ocmem_read(struct ocmem *ocmem, u32 reg)
> +{
> +	return readl(ocmem->mmio + reg);
> +}
> +
> +static int ocmem_clk_enable(struct ocmem *ocmem)
> +{
> +	int ret;
> +
> +	ret = clk_prepare_enable(ocmem->core_clk);

Use clk_bulk_* instead, it will reduce the amount of duplication in the
three places you poke at your clocks.

And with that I would suggest that you just inline these into probe and
remove.

> +	if (ret) {
> +		dev_info(ocmem->dev, "Fail to enable core clk\n");
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(ocmem->iface_clk);
> +	if (ret) {
> +		dev_info(ocmem->dev, "Fail to enable iface clk\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void ocmem_clk_disable(struct ocmem *ocmem)
> +{
> +	clk_disable_unprepare(ocmem->iface_clk);
> +	clk_disable_unprepare(ocmem->core_clk);
> +}
> +
> +static int ocmem_dev_remove(struct platform_device *pdev)

Please move this below probe().

> +{
> +	struct ocmem *ocmem = platform_get_drvdata(pdev);
> +
> +	ocmem_clk_disable(ocmem);
> +
> +	return 0;
> +}
> +
> +static void update_ocmem(struct ocmem *ocmem)
> +{
> +	uint32_t region_mode_ctrl = 0x0;
> +	unsigned int pos = 0, i = 0;

Both pos and i are initialized before use, no need to do it here.

> +
> +	if (!qcom_scm_ocmem_lock_available()) {
> +		for (i = 0; i < ocmem->config->num_regions; i++) {
> +			struct ocmem_region *region = &ocmem->regions[i];
> +
> +			pos = i << 2;

Just use i * 4 in the BIT() operation below.


But the generated macros has the "thin bits" as 1, 2, 4, 8; so shouldn't
this be BIT(i)?

> +			if (region->mode == THIN_MODE)
> +				region_mode_ctrl |= BIT(pos);
> +		}
> +
> +		dev_dbg(ocmem->dev, "ocmem_region_mode_control %x\n",
> +			region_mode_ctrl);
> +		ocmem_write(ocmem, REG_OCMEM_REGION_MODE_CTL, region_mode_ctrl);
> +	}
> +
> +	for (i = 0; i < ocmem->config->num_regions; i++) {
> +		struct ocmem_region *region = &ocmem->regions[i];
> +		u32 data;
> +
> +		data = OCMEM_PSGSC_CTL_MACRO0_MODE(region->macro_state[0]) |
> +			OCMEM_PSGSC_CTL_MACRO1_MODE(region->macro_state[1]) |
> +			OCMEM_PSGSC_CTL_MACRO2_MODE(region->macro_state[2]) |
> +			OCMEM_PSGSC_CTL_MACRO3_MODE(region->macro_state[3]);
> +
> +		ocmem_write(ocmem, REG_OCMEM_PSGSC_CTL(i), data);
> +	}
> +}
> +
> +static unsigned long phys_to_offset(struct ocmem *ocmem,
> +				    unsigned long addr)
> +{
> +	if (addr < ocmem->ocmem_mem->start || addr >= ocmem->ocmem_mem->end)
> +		return 0;
> +
> +	return addr - ocmem->ocmem_mem->start;
> +}
> +
> +static unsigned long device_address(struct ocmem *ocmem,
> +				    enum ocmem_client client,
> +				    unsigned long addr)
> +{
> +	/* TODO, gpu uses phys_to_offset, but others do not.. */

Perhaps WARN_ON(client != OCMEM_GRAPHICS) as well?

> +	return phys_to_offset(ocmem, addr);
> +}
> +
> +static void update_range(struct ocmem *ocmem, struct ocmem_buf *buf,
> +			 enum ocmem_macro_state mstate, enum region_mode rmode)
> +{
> +	unsigned long offset = 0;
> +	int i, j;
> +
> +	/*
> +	 * TODO probably should assert somewhere that range is aligned
> +	 * to macro boundaries..
> +	 */
> +
> +	for (i = 0; i < ocmem->config->num_regions; i++) {
> +		struct ocmem_region *region = &ocmem->regions[i];
> +
> +		if (buf->offset <= offset && offset < buf->offset + buf->len)
> +			region->mode = rmode;
> +
> +		for (j = 0; j < region->num_macros; j++) {
> +			if (buf->offset <= offset &&
> +			    offset < buf->offset + buf->len)
> +				region->macro_state[j] = mstate;
> +
> +			offset += region->macro_size;
> +		}
> +	}
> +
> +	update_ocmem(ocmem);
> +}
> +
> +struct ocmem *of_get_ocmem(struct device *dev)
> +{
> +	struct platform_device *pdev;
> +	struct device_node *devnode;
> +
> +	devnode = of_parse_phandle(dev->of_node, "ocmem", 0);
> +	if (!devnode) {
> +		dev_err(dev, "Cannot look up ocmem phandle\n");
> +		return NULL;

return ERR_PTR(-EINVAL);

> +	}
> +
> +	pdev = of_find_device_by_node(devnode);
> +	if (!pdev) {
> +		dev_err(dev, "Cannot find device node %s\n", devnode->name);
> +		return NULL;

return ERR_PTR(-EPROBE_DEFER)

> +	}
> +
> +	return platform_get_drvdata(pdev);
> +}
> +EXPORT_SYMBOL(of_get_ocmem);
> +
> +struct ocmem_buf *ocmem_allocate(struct ocmem *ocmem, enum ocmem_client client,
> +				 unsigned long size)
> +{
> +	struct ocmem_buf *buf;
> +
> +	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
> +	if (!buf)
> +		return ERR_PTR(-ENOMEM);
> +

This is a very simple allocator... It would be nice to add the minimal
functionality of making sure that you only return successfully if
there's not already a live allocation.

But at least you should add a comment here stating the "limitation" on
the algorithm.

> +	buf->offset = 0;
> +	buf->addr = device_address(ocmem, client, buf->offset);
> +	buf->len = size;
> +
> +	update_range(ocmem, buf, CORE_ON, WIDE_MODE);
> +
> +	if (qcom_scm_ocmem_lock_available()) {
> +		int ret;
> +
> +		ret = qcom_scm_ocmem_lock(QCOM_SCM_OCMEM_GRAPHICS_ID,
> +					  buf->offset, buf->len, WIDE_MODE);
> +		if (ret)
> +			dev_err(ocmem->dev, "could not lock: %d\n", ret);
> +	} else {
> +		if (client == OCMEM_GRAPHICS) {

Isn't the lock_available case also graphcis only? Perhaps it's worth
swapping the inner and outer blocks.

> +			ocmem_write(ocmem, REG_OCMEM_GFX_MPU_START,
> +				    buf->offset);
> +			ocmem_write(ocmem, REG_OCMEM_GFX_MPU_END,
> +				    buf->offset + buf->len);
> +		}

And it's probably good to warn and fail if the client isn't graphics.

> +	}
> +
> +	return buf;
> +}
> +EXPORT_SYMBOL(ocmem_allocate);
> +
> +void ocmem_free(struct ocmem *ocmem, enum ocmem_client client,
> +		struct ocmem_buf *buf)
> +{
> +	update_range(ocmem, buf, CLK_OFF, MODE_DEFAULT);
> +
> +	if (qcom_scm_ocmem_lock_available()) {
> +		int ret;
> +
> +		ret = qcom_scm_ocmem_unlock(QCOM_SCM_OCMEM_GRAPHICS_ID,
> +					    buf->offset, buf->len);
> +		if (ret)
> +			dev_err(ocmem->dev, "could not unlock: %d\n", ret);
> +	} else {
> +		if (client == OCMEM_GRAPHICS) {
> +			ocmem_write(ocmem, REG_OCMEM_GFX_MPU_START, 0x0);
> +			ocmem_write(ocmem, REG_OCMEM_GFX_MPU_END, 0x0);
> +		}
> +	}
> +
> +	kfree(buf);
> +}
> +EXPORT_SYMBOL(ocmem_free);
> +
> +static int ocmem_dev_probe(struct platform_device *pdev)
> +{
> +	struct ocmem *ocmem;
> +	uint32_t reg, num_banks, region_size;
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	int i, j, ret;
> +
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
> +	ocmem = devm_kzalloc(dev, sizeof(*ocmem), GFP_KERNEL);
> +	if (!ocmem)
> +		return -ENOMEM;
> +
> +	ocmem->dev = dev;
> +	ocmem->config = device_get_match_data(dev);
> +
> +	ocmem->core_clk = devm_clk_get(dev, "core");

devm_clk_bulk_get()

> +	if (IS_ERR(ocmem->core_clk)) {
> +		if (PTR_ERR(ocmem->core_clk) != -EPROBE_DEFER)
> +			dev_err(dev, "Unable to get the core clock\n");
> +
> +		return PTR_ERR(ocmem->core_clk);
> +	}
> +
> +	ocmem->iface_clk = devm_clk_get(dev, "iface");
> +	if (IS_ERR(ocmem->iface_clk)) {
> +		if (PTR_ERR(ocmem->iface_clk) != -EPROBE_DEFER)
> +			dev_err(dev, "Unable to get the iface clock\n");
> +
> +		return PTR_ERR(ocmem->iface_clk);
> +	}
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +					   "ocmem_ctrl_physical");

We know it's the "_physical", so drop that from the name.

> +	if (!res) {

It's idiomatic to ignore this check and rely on ioremap_resource() to
fail gracefully if passed NULL.

> +		dev_err(dev, "Could not get ocmem_ctrl_physical region\n");
> +		return -ENXIO;
> +	}
> +
> +	ocmem->mmio = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(ocmem->mmio)) {
> +		dev_err(&pdev->dev,
> +			"Failed to ioremap ocmem_ctrl_physical resource\n");
> +		return PTR_ERR(ocmem->mmio);
> +	}
> +
> +	ocmem->ocmem_mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +							"ocmem_physical");
> +	if (!ocmem->ocmem_mem) {
> +		dev_err(dev, "Could not get ocmem_physical region\n");
> +		return -ENXIO;
> +	}
> +
> +	/* The core clock is synchronous with graphics */
> +	WARN_ON(clk_set_rate(ocmem->core_clk, 1000) < 0);
> +
> +	ret = ocmem_clk_enable(ocmem);
> +	if (ret)
> +		return ret;
> +
> +	if (qcom_scm_restore_sec_config_available()) {
> +		dev_dbg(dev, "configuring scm\n");
> +		ret = qcom_scm_restore_sec_config(&pdev->dev,
> +						  QCOM_SCM_OCMEM_DEV_ID);
> +		if (ret) {
> +			dev_err(dev, "Could not enable secure configuration\n");
> +			goto err_clk_disable;
> +		}
> +	}
> +
> +	reg = ocmem_read(ocmem, REG_OCMEM_HW_PROFILE);
> +	ocmem->num_ports = FIELD(reg, OCMEM_HW_PROFILE_NUM_PORTS);
> +	ocmem->num_macros = FIELD(reg, OCMEM_HW_PROFILE_NUM_MACROS);
> +	ocmem->interleaved = !!(reg & OCMEM_HW_PROFILE_INTERLEAVING);
> +
> +	num_banks = ocmem->num_ports / 2;
> +	region_size = ocmem->config->macro_size * num_banks;
> +
> +	dev_info(dev, "%u ports, %u regions, %u macros, %sinterleaved\n",
> +		 ocmem->num_ports, ocmem->config->num_regions,
> +		 ocmem->num_macros, ocmem->interleaved ? "" : "not ");
> +
> +	ocmem->regions = devm_kcalloc(dev, ocmem->config->num_regions,
> +				      sizeof(struct ocmem_region), GFP_KERNEL);
> +	if (!ocmem->regions) {
> +		ret = -ENOMEM;
> +		goto err_clk_disable;
> +	}
> +
> +	for (i = 0; i < ocmem->config->num_regions; i++) {
> +		struct ocmem_region *region = &ocmem->regions[i];
> +
> +		if (WARN_ON(num_banks > ARRAY_SIZE(region->macro_state))) {
> +			ret = -EINVAL;
> +			goto err_clk_disable;
> +		}
> +
> +		region->mode = MODE_DEFAULT;
> +		region->num_macros = num_banks;
> +
> +		if (i == (ocmem->config->num_regions - 1) &&
> +		    reg & OCMEM_HW_PROFILE_LAST_REGN_HALFSIZE) {
> +			region->macro_size = ocmem->config->macro_size / 2;
> +			region->region_size = region_size / 2;
> +		} else {
> +			region->macro_size = ocmem->config->macro_size;
> +			region->region_size = region_size;
> +		}
> +
> +		for (j = 0; j < ARRAY_SIZE(region->macro_state); j++)
> +			region->macro_state[j] = CLK_OFF;
> +	}
> +
> +	platform_set_drvdata(pdev, ocmem);
> +
> +	return 0;
> +
> +err_clk_disable:
> +	ocmem_clk_disable(ocmem);
> +	return ret;
> +}
> +
> +static const struct ocmem_config ocmem_8974_config = {
> +	.num_regions = 3,
> +	.macro_size = SZ_128K,
> +};
> +
> +static const struct of_device_id ocmem_of_match[] = {
> +	{ .compatible = "qcom,ocmem-msm8974", .data = &ocmem_8974_config },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, ocmem_of_match);
> +
> +static struct platform_driver ocmem_driver = {
> +	.probe = ocmem_dev_probe,
> +	.remove = ocmem_dev_remove,
> +	.driver = {
> +		.name = "ocmem",
> +		.of_match_table = ocmem_of_match,
> +	},
> +};
> +
> +module_platform_driver(ocmem_driver);

MODULE_LICENSE()

> diff --git a/drivers/soc/qcom/ocmem.xml.h b/drivers/soc/qcom/ocmem.xml.h

I would prefer that these lived at the top of the c file, rather than
being generated.

> new file mode 100644
> index 000000000000..b4bfb85d1e33
> --- /dev/null
> +++ b/drivers/soc/qcom/ocmem.xml.h
> @@ -0,0 +1,86 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +#ifndef OCMEM_XML
> +#define OCMEM_XML
> +
> +/* Autogenerated file, DO NOT EDIT manually!
> +
> +This file was generated by the rules-ng-ng headergen tool in this git repository:
> +http://github.com/freedreno/envytools/
> +git clone https://github.com/freedreno/envytools.git
> +
> +The rules-ng-ng source files this header was generated from are:
> +- /home/robclark/src/freedreno/envytools/rnndb/adreno/ocmem.xml         (   1773 bytes, from 2015-09-24 17:30:00)
> +
> +Copyright (C) 2013-2015 by the following authors:
> +- Rob Clark <robdclark@gmail.com> (robclark)
> +*/
> +
> +enum ocmem_macro_state {
> +	PASSTHROUGH = 0,
> +	PERI_ON = 1,
> +	CORE_ON = 2,
> +	CLK_OFF = 4,
> +};
> +
> +#define REG_OCMEM_HW_VERSION					0x00000000
> +
> +#define REG_OCMEM_HW_PROFILE					0x00000004
> +#define OCMEM_HW_PROFILE_NUM_PORTS__MASK			0x0000000f
> +#define OCMEM_HW_PROFILE_NUM_PORTS__SHIFT			0
> +static inline uint32_t OCMEM_HW_PROFILE_NUM_PORTS(uint32_t val)
> +{
> +	return ((val) << OCMEM_HW_PROFILE_NUM_PORTS__SHIFT) & OCMEM_HW_PROFILE_NUM_PORTS__MASK;
> +}
> +#define OCMEM_HW_PROFILE_NUM_MACROS__MASK			0x00003f00
> +#define OCMEM_HW_PROFILE_NUM_MACROS__SHIFT			8
> +static inline uint32_t OCMEM_HW_PROFILE_NUM_MACROS(uint32_t val)
> +{
> +	return ((val) << OCMEM_HW_PROFILE_NUM_MACROS__SHIFT) & OCMEM_HW_PROFILE_NUM_MACROS__MASK;
> +}
> +#define OCMEM_HW_PROFILE_LAST_REGN_HALFSIZE			0x00010000
> +#define OCMEM_HW_PROFILE_INTERLEAVING				0x00020000
> +
> +#define REG_OCMEM_GEN_STATUS					0x0000000c
> +
> +#define REG_OCMEM_PSGSC_STATUS					0x00000038
> +
> +static inline uint32_t REG_OCMEM_PSGSC(uint32_t i0) { return 0x0000003c + 0x1*i0; }
> +
> +static inline uint32_t REG_OCMEM_PSGSC_CTL(uint32_t i0) { return 0x0000003c + 0x1*i0; }
> +#define OCMEM_PSGSC_CTL_MACRO0_MODE__MASK			0x00000007
> +#define OCMEM_PSGSC_CTL_MACRO0_MODE__SHIFT			0
> +static inline uint32_t OCMEM_PSGSC_CTL_MACRO0_MODE(enum ocmem_macro_state val)
> +{
> +	return ((val) << OCMEM_PSGSC_CTL_MACRO0_MODE__SHIFT) & OCMEM_PSGSC_CTL_MACRO0_MODE__MASK;
> +}
> +#define OCMEM_PSGSC_CTL_MACRO1_MODE__MASK			0x00000070
> +#define OCMEM_PSGSC_CTL_MACRO1_MODE__SHIFT			4
> +static inline uint32_t OCMEM_PSGSC_CTL_MACRO1_MODE(enum ocmem_macro_state val)
> +{
> +	return ((val) << OCMEM_PSGSC_CTL_MACRO1_MODE__SHIFT) & OCMEM_PSGSC_CTL_MACRO1_MODE__MASK;
> +}
> +#define OCMEM_PSGSC_CTL_MACRO2_MODE__MASK			0x00000700
> +#define OCMEM_PSGSC_CTL_MACRO2_MODE__SHIFT			8
> +static inline uint32_t OCMEM_PSGSC_CTL_MACRO2_MODE(enum ocmem_macro_state val)
> +{
> +	return ((val) << OCMEM_PSGSC_CTL_MACRO2_MODE__SHIFT) & OCMEM_PSGSC_CTL_MACRO2_MODE__MASK;
> +}
> +#define OCMEM_PSGSC_CTL_MACRO3_MODE__MASK			0x00007000
> +#define OCMEM_PSGSC_CTL_MACRO3_MODE__SHIFT			12
> +static inline uint32_t OCMEM_PSGSC_CTL_MACRO3_MODE(enum ocmem_macro_state val)
> +{
> +	return ((val) << OCMEM_PSGSC_CTL_MACRO3_MODE__SHIFT) & OCMEM_PSGSC_CTL_MACRO3_MODE__MASK;
> +}
> +
> +#define REG_OCMEM_REGION_MODE_CTL				0x00001000
> +#define OCMEM_REGION_MODE_CTL_REG0_THIN				0x00000001
> +#define OCMEM_REGION_MODE_CTL_REG1_THIN				0x00000002
> +#define OCMEM_REGION_MODE_CTL_REG2_THIN				0x00000004
> +#define OCMEM_REGION_MODE_CTL_REG3_THIN				0x00000008
> +
> +#define REG_OCMEM_GFX_MPU_START					0x00001004
> +
> +#define REG_OCMEM_GFX_MPU_END					0x00001008
> +
> +#endif /* OCMEM_XML */
> diff --git a/include/soc/qcom/ocmem.h b/include/soc/qcom/ocmem.h
> new file mode 100644
> index 000000000000..e56ce220096d
> --- /dev/null
> +++ b/include/soc/qcom/ocmem.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2015 Red Hat
> + * Author: Rob Clark <robdclark@gmail.com>
> + */
> +
> +#ifndef __OCMEM_H__
> +#define __OCMEM_H__
> +
> +enum ocmem_client {
> +	/* GMEM clients */
> +	OCMEM_GRAPHICS = 0x0,
> +	/*
> +	 * TODO add more once ocmem_allocate() is clever enough to
> +	 * deal with multiple clients.
> +	 */
> +	OCMEM_CLIENT_MAX,
> +};
> +
> +struct ocmem;
> +
> +struct ocmem_buf {
> +	unsigned long offset;
> +	unsigned long addr;
> +	unsigned long len;
> +};
> +
> +struct ocmem *of_get_ocmem(struct device *dev);
> +struct ocmem_buf *ocmem_allocate(struct ocmem *ocmem, enum ocmem_client client,
> +				 unsigned long size);
> +void ocmem_free(struct ocmem *ocmem, enum ocmem_client client,
> +		struct ocmem_buf *buf);
> +
> +#endif /* __OCMEM_H__ */
> -- 

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/6] dt-bindings: soc: qcom: add On Chip MEMory (OCMEM) bindings
  2019-06-16 13:29 ` [PATCH 1/6] dt-bindings: soc: qcom: add On Chip MEMory (OCMEM) bindings Brian Masney
@ 2019-06-16 17:43   ` Bjorn Andersson
  2019-06-17 14:29   ` Rob Herring
  1 sibling, 0 replies; 24+ messages in thread
From: Bjorn Andersson @ 2019-06-16 17:43 UTC (permalink / raw)
  To: Brian Masney
  Cc: agross, david.brown, robdclark, sean, robh+dt, airlied, daniel,
	mark.rutland, jonathan, linux-arm-msm, linux-kernel, dri-devel,
	freedreno, devicetree

On Sun 16 Jun 06:29 PDT 2019, Brian Masney wrote:

> Add device tree bindings for the On Chip Memory (OCMEM) that is present
> on some Qualcomm Snapdragon SoCs.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
>  .../bindings/soc/qcom/qcom,ocmem.yaml         | 66 +++++++++++++++++++
>  1 file changed, 66 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,ocmem.yaml
> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,ocmem.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,ocmem.yaml
> new file mode 100644
> index 000000000000..5e3ae6311a16
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,ocmem.yaml
> @@ -0,0 +1,66 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/qcom/qcom,ocmem.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: On Chip Memory (OCMEM) that is present on some Qualcomm Snapdragon SoCs.
> +
> +maintainers:
> +  - Brian Masney <masneyb@onstation.org>
> +
> +description: |
> +  The On Chip Memory (OCMEM) allocator allows various clients to allocate memory
> +  from OCMEM based on performance, latency and power requirements. This is
> +  typically used by the GPU, camera/video, and audio components on some
> +  Snapdragon SoCs.
> +
> +properties:
> +  compatible:
> +    const: qcom,ocmem-msm8974

qcom,msm8974-ocmem

> +
> +  reg:
> +    items:
> +      - description: Control registers
> +      - description: OCMEM address range
> +
> +  reg-names:
> +    items:
> +      - const: ocmem_ctrl_physical
> +      - const: ocmem_physical

Drop the "_physical" part, it's given by this being "reg".

Regards,
Bjorn

> +
> +  clocks:
> +    items:
> +      - description: Core clock
> +      - description: Interface clock
> +
> +  clock-names:
> +    items:
> +      - const: core
> +      - const: iface
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - clocks
> +  - clock-names
> +
> +examples:
> +  - |
> +      #include <dt-bindings/clock/qcom,rpmcc.h>
> +      #include <dt-bindings/clock/qcom,mmcc-msm8974.h>
> +
> +      ocmem: ocmem@fdd00000 {
> +        compatible = "qcom,ocmem-msm8974";
> +
> +        reg = <0xfdd00000 0x2000>,
> +               <0xfec00000 0x180000>;
> +        reg-names = "ocmem_ctrl_physical",
> +                    "ocmem_physical";
> +
> +        clocks = <&rpmcc RPM_SMD_OCMEMGX_CLK>,
> +                  <&mmcc OCMEMCX_OCMEMNOC_CLK>;
> +        clock-names = "core",
> +                      "iface";
> +      };
> -- 
> 2.20.1
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 3/6] firmware: qcom: scm: add support to restore secure config
  2019-06-16 13:29 ` [PATCH 3/6] firmware: qcom: scm: add support to restore secure config Brian Masney
@ 2019-06-16 17:53   ` Bjorn Andersson
  0 siblings, 0 replies; 24+ messages in thread
From: Bjorn Andersson @ 2019-06-16 17:53 UTC (permalink / raw)
  To: Brian Masney
  Cc: agross, david.brown, robdclark, sean, robh+dt, airlied, daniel,
	mark.rutland, jonathan, linux-arm-msm, linux-kernel, dri-devel,
	freedreno, devicetree

On Sun 16 Jun 06:29 PDT 2019, Brian Masney wrote:

> From: Rob Clark <robdclark@gmail.com>
> 
> Add support to restore the secure configuration that is needed by the
> On Chip MEMory (OCMEM) that is present on some Snapdragon devices.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> [masneyb@onstation.org: ported to latest kernel; minor reformatting.]
> Signed-off-by: Brian Masney <masneyb@onstation.org>

This went upstream for 64-bit with config abbreviated cfg, so please
implement __qcom_scm_restore_sec_cfg() for 32-bit and add the defines
instead.

Regards,
Bjorn

> ---
> Rob's last version of this patch:
> https://patchwork.kernel.org/patch/7340701/
> 
>  drivers/firmware/qcom_scm-32.c | 21 +++++++++++++++++++++
>  drivers/firmware/qcom_scm-64.c |  6 ++++++
>  drivers/firmware/qcom_scm.c    | 23 +++++++++++++++++++++++
>  drivers/firmware/qcom_scm.h    |  6 ++++++
>  include/linux/qcom_scm.h       | 13 +++++++++++++
>  5 files changed, 69 insertions(+)
> 
> diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
> index 215061c581e1..089b47124933 100644
> --- a/drivers/firmware/qcom_scm-32.c
> +++ b/drivers/firmware/qcom_scm-32.c
> @@ -442,6 +442,27 @@ int __qcom_scm_hdcp_req(struct device *dev, struct qcom_scm_hdcp_req *req,
>  		req, req_cnt * sizeof(*req), resp, sizeof(*resp));
>  }
>  
> +int __qcom_scm_restore_sec_config(struct device *dev, u32 sec_id,
> +				  u32 ctx_bank_num)
> +{
> +	struct msm_scm_sec_cfg {
> +		__le32 id;
> +		__le32 ctx_bank_num;
> +	} cfg;
> +	int ret, scm_ret = 0;
> +
> +	cfg.id = cpu_to_le32(sec_id);
> +	cfg.ctx_bank_num = cpu_to_le32(sec_id);
> +
> +	ret = qcom_scm_call(dev, QCOM_SCM_MP_SVC, QCOM_SCM_MP_RESTORE_SEC_CFG,
> +			    &cfg, sizeof(cfg), &scm_ret, sizeof(scm_ret));
> +
> +	if (ret || scm_ret)
> +		return ret ? ret : -EINVAL;
> +
> +	return 0;
> +}
> +
>  void __qcom_scm_init(void)
>  {
>  }
> diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
> index 91d5ad7cf58b..b6b78da7f9c9 100644
> --- a/drivers/firmware/qcom_scm-64.c
> +++ b/drivers/firmware/qcom_scm-64.c
> @@ -241,6 +241,12 @@ int __qcom_scm_hdcp_req(struct device *dev, struct qcom_scm_hdcp_req *req,
>  	return ret;
>  }
>  
> +int __qcom_scm_restore_sec_config(struct device *dev, u32 sec_id,
> +				  u32 ctx_bank_num)
> +{
> +	return -ENOTSUPP;
> +}
> +
>  void __qcom_scm_init(void)
>  {
>  	u64 cmd;
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 2ddc118dba1b..5495ef994c5d 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -170,6 +170,29 @@ int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp)
>  }
>  EXPORT_SYMBOL(qcom_scm_hdcp_req);
>  
> +/**
> + * qcom_scm_restore_sec_config_available() - Check if secure environment
> + * supports restore security config interface.
> + *
> + * Return true if restore-cfg interface is supported, false if not.
> + */
> +bool qcom_scm_restore_sec_config_available(void)
> +{
> +	return __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_MP_SVC,
> +					    QCOM_SCM_MP_RESTORE_SEC_CFG);
> +}
> +EXPORT_SYMBOL(qcom_scm_restore_sec_config_available);
> +
> +/**
> + * qcom_scm_restore_sec_config() - call restore-cfg interface
> + */
> +int qcom_scm_restore_sec_config(struct device *dev,
> +				enum qcom_scm_sec_dev_id sec_id)
> +{
> +	return __qcom_scm_restore_sec_config(dev, sec_id, 0);
> +}
> +EXPORT_SYMBOL(qcom_scm_restore_sec_config);
> +
>  /**
>   * qcom_scm_pas_supported() - Check if the peripheral authentication service is
>   *			      available for the given peripherial
> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
> index 99506bd873c0..bccc7d10c5c2 100644
> --- a/drivers/firmware/qcom_scm.h
> +++ b/drivers/firmware/qcom_scm.h
> @@ -42,6 +42,12 @@ extern int __qcom_scm_hdcp_req(struct device *dev,
>  
>  extern void __qcom_scm_init(void);
>  
> +#define QCOM_SCM_MP_SVC			0xc
> +#define QCOM_SCM_MP_RESTORE_SEC_CFG	0x2
> +
> +extern int __qcom_scm_restore_sec_config(struct device *dev, u32 sec_id,
> +					 u32 ctx_bank_num);
> +
>  #define QCOM_SCM_SVC_PIL		0x2
>  #define QCOM_SCM_PAS_INIT_IMAGE_CMD	0x1
>  #define QCOM_SCM_PAS_MEM_SETUP_CMD	0x2
> diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
> index 3f12cc77fb58..b5c0afaca955 100644
> --- a/include/linux/qcom_scm.h
> +++ b/include/linux/qcom_scm.h
> @@ -24,6 +24,16 @@ struct qcom_scm_vmperm {
>  	int perm;
>  };
>  
> +enum qcom_scm_sec_dev_id {
> +	QCOM_SCM_MDSS_DEV_ID	= 1,
> +	QCOM_SCM_OCMEM_DEV_ID	= 5,
> +	QCOM_SCM_PCIE0_DEV_ID	= 11,
> +	QCOM_SCM_PCIE1_DEV_ID	= 12,
> +	QCOM_SCM_GFX_DEV_ID	= 18,
> +	QCOM_SCM_UFS_DEV_ID	= 19,
> +	QCOM_SCM_ICE_DEV_ID	= 20,
> +};
> +
>  #define QCOM_SCM_VMID_HLOS       0x3
>  #define QCOM_SCM_VMID_MSS_MSA    0xF
>  #define QCOM_SCM_VMID_WLAN       0x18
> @@ -41,6 +51,9 @@ extern bool qcom_scm_is_available(void);
>  extern bool qcom_scm_hdcp_available(void);
>  extern int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt,
>  			     u32 *resp);
> +extern bool qcom_scm_restore_sec_config_available(void);
> +extern int qcom_scm_restore_sec_config(struct device *dev,
> +				       enum qcom_scm_sec_dev_id sec_id);
>  extern bool qcom_scm_pas_supported(u32 peripheral);
>  extern int qcom_scm_pas_init_image(u32 peripheral, const void *metadata,
>  				   size_t size);
> -- 
> 2.20.1
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/6] firmware: qcom: scm: add OCMEM lock/unlock interface
  2019-06-16 13:29 ` [PATCH 4/6] firmware: qcom: scm: add OCMEM lock/unlock interface Brian Masney
@ 2019-06-16 17:54   ` Bjorn Andersson
  0 siblings, 0 replies; 24+ messages in thread
From: Bjorn Andersson @ 2019-06-16 17:54 UTC (permalink / raw)
  To: Brian Masney
  Cc: agross, david.brown, robdclark, sean, robh+dt, airlied, daniel,
	mark.rutland, jonathan, linux-arm-msm, linux-kernel, dri-devel,
	freedreno, devicetree

On Sun 16 Jun 06:29 PDT 2019, Brian Masney wrote:

> From: Rob Clark <robdclark@gmail.com>
> 
> Add support for the OCMEM lock/unlock interface that is needed by the
> On Chip MEMory (OCMEM) that is present on some Snapdragon devices.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> [masneyb@onstation.org: ported to latest kernel; minor reformatting.]
> Signed-off-by: Brian Masney <masneyb@onstation.org>

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

> ---
> Rob's last version of this patch:
> https://patchwork.kernel.org/patch/7340711/
> 
>  drivers/firmware/qcom_scm-32.c | 35 +++++++++++++++++++++++++++++
>  drivers/firmware/qcom_scm-64.c | 12 ++++++++++
>  drivers/firmware/qcom_scm.c    | 40 ++++++++++++++++++++++++++++++++++
>  drivers/firmware/qcom_scm.h    |  9 ++++++++
>  include/linux/qcom_scm.h       | 15 +++++++++++++
>  5 files changed, 111 insertions(+)
> 
> diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
> index 089b47124933..0100c82b9c00 100644
> --- a/drivers/firmware/qcom_scm-32.c
> +++ b/drivers/firmware/qcom_scm-32.c
> @@ -463,6 +463,41 @@ int __qcom_scm_restore_sec_config(struct device *dev, u32 sec_id,
>  	return 0;
>  }
>  
> +int __qcom_scm_ocmem_lock(struct device *dev, u32 id, u32 offset, u32 size,
> +			  u32 mode)
> +{
> +	struct ocmem_tz_lock {
> +		__le32 id;
> +		__le32 offset;
> +		__le32 size;
> +		__le32 mode;
> +	} request;
> +
> +	request.id = cpu_to_le32(id);
> +	request.offset = cpu_to_le32(offset);
> +	request.size = cpu_to_le32(size);
> +	request.mode = cpu_to_le32(mode);
> +
> +	return qcom_scm_call(dev, QCOM_SCM_OCMEM_SVC, QCOM_SCM_OCMEM_LOCK_CMD,
> +			     &request, sizeof(request), NULL, 0);
> +}
> +
> +int __qcom_scm_ocmem_unlock(struct device *dev, u32 id, u32 offset, u32 size)
> +{
> +	struct ocmem_tz_unlock {
> +		__le32 id;
> +		__le32 offset;
> +		__le32 size;
> +	} request;
> +
> +	request.id = cpu_to_le32(id);
> +	request.offset = cpu_to_le32(offset);
> +	request.size = cpu_to_le32(size);
> +
> +	return qcom_scm_call(dev, QCOM_SCM_OCMEM_SVC, QCOM_SCM_OCMEM_UNLOCK_CMD,
> +			     &request, sizeof(request), NULL, 0);
> +}
> +
>  void __qcom_scm_init(void)
>  {
>  }
> diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
> index b6b78da7f9c9..2674d6d3cdde 100644
> --- a/drivers/firmware/qcom_scm-64.c
> +++ b/drivers/firmware/qcom_scm-64.c
> @@ -247,6 +247,18 @@ int __qcom_scm_restore_sec_config(struct device *dev, u32 sec_id,
>  	return -ENOTSUPP;
>  }
>  
> +int __qcom_scm_ocmem_lock(struct device *dev, uint32_t id, uint32_t offset,
> +			  uint32_t size, uint32_t mode)
> +{
> +	return -ENOTSUPP;
> +}
> +
> +int __qcom_scm_ocmem_unlock(struct device *dev, uint32_t id, uint32_t offset,
> +			    uint32_t size)
> +{
> +	return -ENOTSUPP;
> +}
> +
>  void __qcom_scm_init(void)
>  {
>  	u64 cmd;
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 5495ef994c5d..85afb54defd4 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -193,6 +193,46 @@ int qcom_scm_restore_sec_config(struct device *dev,
>  }
>  EXPORT_SYMBOL(qcom_scm_restore_sec_config);
>  
> +/**
> + * qcom_scm_ocmem_lock_available() - is OCMEM lock/unlock interface available
> + */
> +bool qcom_scm_ocmem_lock_available(void)
> +{
> +	return __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_OCMEM_SVC,
> +					    QCOM_SCM_OCMEM_LOCK_CMD);
> +}
> +EXPORT_SYMBOL(qcom_scm_ocmem_lock_available);
> +
> +/**
> + * qcom_scm_ocmem_lock() - call OCMEM lock interface to assign an OCMEM
> + * region to the specified initiator
> + *
> + * @id:     tz initiator id
> + * @offset: OCMEM offset
> + * @size:   OCMEM size
> + * @mode:   access mode (WIDE/NARROW)
> + */
> +int qcom_scm_ocmem_lock(enum qcom_scm_ocmem_client id, u32 offset, u32 size,
> +			u32 mode)
> +{
> +	return __qcom_scm_ocmem_lock(__scm->dev, id, offset, size, mode);
> +}
> +EXPORT_SYMBOL(qcom_scm_ocmem_lock);
> +
> +/**
> + * qcom_scm_ocmem_unlock() - call OCMEM unlock interface to release an OCMEM
> + * region from the specified initiator
> + *
> + * @id:     tz initiator id
> + * @offset: OCMEM offset
> + * @size:   OCMEM size
> + */
> +int qcom_scm_ocmem_unlock(enum qcom_scm_ocmem_client id, u32 offset, u32 size)
> +{
> +	return __qcom_scm_ocmem_unlock(__scm->dev, id, offset, size);
> +}
> +EXPORT_SYMBOL(qcom_scm_ocmem_unlock);
> +
>  /**
>   * qcom_scm_pas_supported() - Check if the peripheral authentication service is
>   *			      available for the given peripherial
> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
> index bccc7d10c5c2..387e3c4e33c5 100644
> --- a/drivers/firmware/qcom_scm.h
> +++ b/drivers/firmware/qcom_scm.h
> @@ -48,6 +48,15 @@ extern void __qcom_scm_init(void);
>  extern int __qcom_scm_restore_sec_config(struct device *dev, u32 sec_id,
>  					 u32 ctx_bank_num);
>  
> +#define QCOM_SCM_OCMEM_SVC			0xf
> +#define QCOM_SCM_OCMEM_LOCK_CMD		0x1
> +#define QCOM_SCM_OCMEM_UNLOCK_CMD		0x2
> +
> +extern int __qcom_scm_ocmem_lock(struct device *dev, u32 id, u32 offset,
> +				 u32 size, u32 mode);
> +extern int __qcom_scm_ocmem_unlock(struct device *dev, u32 id, u32 offset,
> +				   u32 size);
> +
>  #define QCOM_SCM_SVC_PIL		0x2
>  #define QCOM_SCM_PAS_INIT_IMAGE_CMD	0x1
>  #define QCOM_SCM_PAS_MEM_SETUP_CMD	0x2
> diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
> index b5c0afaca955..977c01aa524a 100644
> --- a/include/linux/qcom_scm.h
> +++ b/include/linux/qcom_scm.h
> @@ -34,6 +34,16 @@ enum qcom_scm_sec_dev_id {
>  	QCOM_SCM_ICE_DEV_ID	= 20,
>  };
>  
> +enum qcom_scm_ocmem_client {
> +	QCOM_SCM_OCMEM_UNUSED_ID = 0x0,
> +	QCOM_SCM_OCMEM_GRAPHICS_ID,
> +	QCOM_SCM_OCMEM_VIDEO_ID,
> +	QCOM_SCM_OCMEM_LP_AUDIO_ID,
> +	QCOM_SCM_OCMEM_SENSORS_ID,
> +	QCOM_SCM_OCMEM_OTHER_OS_ID,
> +	QCOM_SCM_OCMEM_DEBUG_ID,
> +};
> +
>  #define QCOM_SCM_VMID_HLOS       0x3
>  #define QCOM_SCM_VMID_MSS_MSA    0xF
>  #define QCOM_SCM_VMID_WLAN       0x18
> @@ -54,6 +64,11 @@ extern int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt,
>  extern bool qcom_scm_restore_sec_config_available(void);
>  extern int qcom_scm_restore_sec_config(struct device *dev,
>  				       enum qcom_scm_sec_dev_id sec_id);
> +extern bool qcom_scm_ocmem_lock_available(void);
> +extern int qcom_scm_ocmem_lock(enum qcom_scm_ocmem_client id, u32 offset,
> +			       u32 size, u32 mode);
> +extern int qcom_scm_ocmem_unlock(enum qcom_scm_ocmem_client id, u32 offset,
> +				 u32 size);
>  extern bool qcom_scm_pas_supported(u32 peripheral);
>  extern int qcom_scm_pas_init_image(u32 peripheral, const void *metadata,
>  				   size_t size);
> -- 
> 2.20.1
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 6/6] drm/msm/gpu: add ocmem init/cleanup functions
  2019-06-16 13:29 ` [PATCH 6/6] drm/msm/gpu: add ocmem init/cleanup functions Brian Masney
@ 2019-06-16 18:06   ` Bjorn Andersson
  2019-06-17  0:18     ` Brian Masney
  2019-06-19 18:15   ` [Freedreno] " Jordan Crouse
  1 sibling, 1 reply; 24+ messages in thread
From: Bjorn Andersson @ 2019-06-16 18:06 UTC (permalink / raw)
  To: Brian Masney
  Cc: agross, david.brown, robdclark, sean, robh+dt, airlied, daniel,
	mark.rutland, jonathan, linux-arm-msm, linux-kernel, dri-devel,
	freedreno, devicetree

On Sun 16 Jun 06:29 PDT 2019, Brian Masney wrote:

> The files a3xx_gpu.c and a4xx_gpu.c have ifdefs for the OCMEM support
> that was missing upstream. Add two new functions (adreno_gpu_ocmem_init
> and adreno_gpu_ocmem_cleanup) that removes some duplicated code. We also
> need to change the ifdef check for CONFIG_MSM_OCMEM to CONFIG_QCOM_OCMEM
> now that OCMEM support is upstream.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
>  drivers/gpu/drm/msm/adreno/a3xx_gpu.c   | 33 +++++++-------------
>  drivers/gpu/drm/msm/adreno/a3xx_gpu.h   |  3 +-
>  drivers/gpu/drm/msm/adreno/a4xx_gpu.c   | 30 ++++++------------
>  drivers/gpu/drm/msm/adreno/a4xx_gpu.h   |  3 +-
>  drivers/gpu/drm/msm/adreno/adreno_gpu.c | 41 +++++++++++++++++++++++++
>  drivers/gpu/drm/msm/adreno/adreno_gpu.h | 10 ++++++
>  6 files changed, 74 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> index c3b4bc6e4155..72720bb2aca1 100644
> --- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> @@ -17,10 +17,6 @@
>   * this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> -#ifdef CONFIG_MSM_OCMEM
> -#  include <mach/ocmem.h>
> -#endif
> -
>  #include "a3xx_gpu.h"
>  
>  #define A3XX_INT0_MASK \
> @@ -206,9 +202,9 @@ static int a3xx_hw_init(struct msm_gpu *gpu)
>  		gpu_write(gpu, REG_A3XX_RBBM_GPR0_CTL, 0x00000000);
>  
>  	/* Set the OCMEM base address for A330, etc */
> -	if (a3xx_gpu->ocmem_hdl) {
> +	if (a3xx_gpu->ocmem.hdl) {
>  		gpu_write(gpu, REG_A3XX_RB_GMEM_BASE_ADDR,
> -			(unsigned int)(a3xx_gpu->ocmem_base >> 14));
> +			(unsigned int)(a3xx_gpu->ocmem.base >> 14));

This blindly requires that the ocmem allocator will return entries
allocated to 16kB. Please ensure that a future implementation of the
actual ocmem allocator maintains this (comments? checks?). 

>  	}
>  
>  	/* Turn on performance counters: */
> @@ -329,10 +325,7 @@ static void a3xx_destroy(struct msm_gpu *gpu)
>  
>  	adreno_gpu_cleanup(adreno_gpu);
>  
> -#ifdef CONFIG_MSM_OCMEM
> -	if (a3xx_gpu->ocmem_base)
> -		ocmem_free(OCMEM_GRAPHICS, a3xx_gpu->ocmem_hdl);
> -#endif
> +	adreno_gpu_ocmem_cleanup(&a3xx_gpu->ocmem);
>  
>  	kfree(a3xx_gpu);
>  }
> @@ -507,17 +500,10 @@ struct msm_gpu *a3xx_gpu_init(struct drm_device *dev)
>  
>  	/* if needed, allocate gmem: */
>  	if (adreno_is_a330(adreno_gpu)) {
> -#ifdef CONFIG_MSM_OCMEM
> -		/* TODO this is different/missing upstream: */
> -		struct ocmem_buf *ocmem_hdl =
> -				ocmem_allocate(OCMEM_GRAPHICS, adreno_gpu->gmem);
> -
> -		a3xx_gpu->ocmem_hdl = ocmem_hdl;
> -		a3xx_gpu->ocmem_base = ocmem_hdl->addr;
> -		adreno_gpu->gmem = ocmem_hdl->len;
> -		DBG("using %dK of OCMEM at 0x%08x", adreno_gpu->gmem / 1024,
> -				a3xx_gpu->ocmem_base);
> -#endif
> +		ret = adreno_gpu_ocmem_init(&adreno_gpu->base.pdev->dev,
> +					    adreno_gpu, &a3xx_gpu->ocmem);
> +		if (ret)
> +			goto fail;
>  	}
>  
>  	if (!gpu->aspace) {
> @@ -530,11 +516,14 @@ struct msm_gpu *a3xx_gpu_init(struct drm_device *dev)
>  		 */
>  		DRM_DEV_ERROR(dev->dev, "No memory protection without IOMMU\n");
>  		ret = -ENXIO;
> -		goto fail;
> +		goto fail_cleanup_ocmem;
>  	}
>  
>  	return gpu;
>  
> +fail_cleanup_ocmem:
> +	adreno_gpu_ocmem_cleanup(&a3xx_gpu->ocmem);
> +
>  fail:
>  	if (a3xx_gpu)
>  		a3xx_destroy(&a3xx_gpu->base.base);
> diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.h b/drivers/gpu/drm/msm/adreno/a3xx_gpu.h
> index ab60dc9e344e..727c34f38f9e 100644
> --- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.h
> @@ -30,8 +30,7 @@ struct a3xx_gpu {
>  	struct adreno_gpu base;
>  
>  	/* if OCMEM is used for GMEM: */
> -	uint32_t ocmem_base;
> -	void *ocmem_hdl;
> +	struct adreno_ocmem ocmem;
>  };
>  #define to_a3xx_gpu(x) container_of(x, struct a3xx_gpu, base)
>  
> diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> index ab2b752566d8..b8f825107796 100644
> --- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> @@ -2,9 +2,6 @@
>  /* Copyright (c) 2014 The Linux Foundation. All rights reserved.
>   */
>  #include "a4xx_gpu.h"
> -#ifdef CONFIG_MSM_OCMEM
> -#  include <soc/qcom/ocmem.h>
> -#endif
>  
>  #define A4XX_INT0_MASK \
>  	(A4XX_INT0_RBBM_AHB_ERROR |        \
> @@ -188,7 +185,7 @@ static int a4xx_hw_init(struct msm_gpu *gpu)
>  			(1 << 30) | 0xFFFF);
>  
>  	gpu_write(gpu, REG_A4XX_RB_GMEM_BASE_ADDR,
> -			(unsigned int)(a4xx_gpu->ocmem_base >> 14));
> +			(unsigned int)(a4xx_gpu->ocmem.base >> 14));
>  
>  	/* Turn on performance counters: */
>  	gpu_write(gpu, REG_A4XX_RBBM_PERFCTR_CTL, 0x01);
> @@ -318,10 +315,7 @@ static void a4xx_destroy(struct msm_gpu *gpu)
>  
>  	adreno_gpu_cleanup(adreno_gpu);
>  
> -#ifdef CONFIG_MSM_OCMEM
> -	if (a4xx_gpu->ocmem_base)
> -		ocmem_free(OCMEM_GRAPHICS, a4xx_gpu->ocmem_hdl);
> -#endif
> +	adreno_gpu_ocmem_cleanup(&a4xx_gpu->ocmem);
>  
>  	kfree(a4xx_gpu);
>  }
> @@ -578,17 +572,10 @@ struct msm_gpu *a4xx_gpu_init(struct drm_device *dev)
>  
>  	/* if needed, allocate gmem: */
>  	if (adreno_is_a4xx(adreno_gpu)) {
> -#ifdef CONFIG_MSM_OCMEM
> -		/* TODO this is different/missing upstream: */
> -		struct ocmem_buf *ocmem_hdl =
> -				ocmem_allocate(OCMEM_GRAPHICS, adreno_gpu->gmem);
> -
> -		a4xx_gpu->ocmem_hdl = ocmem_hdl;
> -		a4xx_gpu->ocmem_base = ocmem_hdl->addr;
> -		adreno_gpu->gmem = ocmem_hdl->len;
> -		DBG("using %dK of OCMEM at 0x%08x", adreno_gpu->gmem / 1024,
> -				a4xx_gpu->ocmem_base);
> -#endif
> +		ret = adreno_gpu_ocmem_init(dev->dev, adreno_gpu,
> +					    &a4xx_gpu->ocmem);
> +		if (ret)
> +			goto fail;
>  	}
>  
>  	if (!gpu->aspace) {
> @@ -601,11 +588,14 @@ struct msm_gpu *a4xx_gpu_init(struct drm_device *dev)
>  		 */
>  		DRM_DEV_ERROR(dev->dev, "No memory protection without IOMMU\n");
>  		ret = -ENXIO;
> -		goto fail;
> +		goto fail_cleanup_ocmem;
>  	}
>  
>  	return gpu;
>  
> +fail_cleanup_ocmem:
> +	adreno_gpu_ocmem_cleanup(&a4xx_gpu->ocmem);
> +
>  fail:
>  	if (a4xx_gpu)
>  		a4xx_destroy(&a4xx_gpu->base.base);
> diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.h b/drivers/gpu/drm/msm/adreno/a4xx_gpu.h
> index d506311ee240..a01448cba2ea 100644
> --- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.h
> @@ -16,8 +16,7 @@ struct a4xx_gpu {
>  	struct adreno_gpu base;
>  
>  	/* if OCMEM is used for GMEM: */
> -	uint32_t ocmem_base;
> -	void *ocmem_hdl;
> +	struct adreno_ocmem ocmem;
>  };
>  #define to_a4xx_gpu(x) container_of(x, struct a4xx_gpu, base)
>  
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 6f7f4114afcf..e0a9409c8a32 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -29,6 +29,10 @@
>  #include "msm_gem.h"
>  #include "msm_mmu.h"
>  
> +#ifdef CONFIG_QCOM_OCMEM
> +#  include <soc/qcom/ocmem.h>
> +#endif

This file exists (after the previous patch), so no need to make its
inclusion conditional.

> +
>  static bool zap_available = true;
>  
>  static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,
> @@ -897,6 +901,43 @@ static int adreno_get_pwrlevels(struct device *dev,
>  	return 0;
>  }
>  
> +int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,
> +			  struct adreno_ocmem *adreno_ocmem)
> +{
> +#ifdef CONFIG_QCOM_OCMEM

No need to make this conditional.

> +	struct ocmem_buf *ocmem_hdl;
> +	struct ocmem *ocmem;
> +
> +	ocmem = of_get_ocmem(dev);
> +	if (!ocmem) {
> +		/* This is an optional property so return success. */
> +		return 0;
> +	}
> +
> +	ocmem_hdl = ocmem_allocate(ocmem, OCMEM_GRAPHICS, adreno_gpu->gmem);
> +	if (IS_ERR(ocmem_hdl))
> +		return PTR_ERR(ocmem_hdl);
> +
> +	adreno_ocmem->ocmem = ocmem;
> +	adreno_ocmem->base = ocmem_hdl->addr;
> +	adreno_ocmem->hdl = ocmem_hdl;
> +	adreno_gpu->gmem = ocmem_hdl->len;
> +	DBG("using %dK of OCMEM at 0x%08x", adreno_gpu->gmem / 1024,
> +	    adreno_ocmem->base);
> +#endif
> +
> +	return 0;
> +}
> +
> +void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *adreno_ocmem)
> +{
> +#ifdef CONFIG_QCOM_OCMEM
> +	if (adreno_ocmem->base)

It would be nice to have ocmem_free() accept NULL, similar to kfree() et
al.

> +		ocmem_free(adreno_ocmem->ocmem, OCMEM_GRAPHICS,
> +			   adreno_ocmem->hdl);
> +#endif
> +}
> +
>  int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>  		struct adreno_gpu *adreno_gpu,
>  		const struct adreno_gpu_funcs *funcs, int nr_rings)
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> index 0925606ec9b5..1cd11570323b 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> @@ -136,6 +136,12 @@ struct adreno_gpu {
>  };
>  #define to_adreno_gpu(x) container_of(x, struct adreno_gpu, base)
>  
> +struct adreno_ocmem {
> +	struct ocmem *ocmem;
> +	uint32_t base;

By ocmem being physically fixed this is sufficient, but unsigned long is
a nicer type for carrying memory addresses.

Regards,
Bjorn

> +	void *hdl;
> +};
> +
>  /* platform config data (ie. from DT, or pdata) */
>  struct adreno_platform_config {
>  	struct adreno_rev rev;
> @@ -241,6 +247,10 @@ void adreno_dump(struct msm_gpu *gpu);
>  void adreno_wait_ring(struct msm_ringbuffer *ring, uint32_t ndwords);
>  struct msm_ringbuffer *adreno_active_ring(struct msm_gpu *gpu);
>  
> +int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,
> +			  struct adreno_ocmem *ocmem);
> +void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *ocmem);
> +
>  int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>  		struct adreno_gpu *gpu, const struct adreno_gpu_funcs *funcs,
>  		int nr_rings);
> -- 
> 2.20.1
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 6/6] drm/msm/gpu: add ocmem init/cleanup functions
  2019-06-16 18:06   ` Bjorn Andersson
@ 2019-06-17  0:18     ` Brian Masney
  2019-06-17  3:43       ` Bjorn Andersson
  0 siblings, 1 reply; 24+ messages in thread
From: Brian Masney @ 2019-06-17  0:18 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: agross, david.brown, robdclark, sean, robh+dt, airlied, daniel,
	mark.rutland, jonathan, linux-arm-msm, linux-kernel, dri-devel,
	freedreno, devicetree

Hi Bjorn,

On Sun, Jun 16, 2019 at 11:06:33AM -0700, Bjorn Andersson wrote:
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > index 6f7f4114afcf..e0a9409c8a32 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > @@ -29,6 +29,10 @@
> >  #include "msm_gem.h"
> >  #include "msm_mmu.h"
> >  
> > +#ifdef CONFIG_QCOM_OCMEM
> > +#  include <soc/qcom/ocmem.h>
> > +#endif
> 
> This file exists (after the previous patch), so no need to make its
> inclusion conditional.
> 
> > +
> >  static bool zap_available = true;
> >  
> >  static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,
> > @@ -897,6 +901,43 @@ static int adreno_get_pwrlevels(struct device *dev,
> >  	return 0;
> >  }
> >  
> > +int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,
> > +			  struct adreno_ocmem *adreno_ocmem)
> > +{
> > +#ifdef CONFIG_QCOM_OCMEM
> 
> No need to make this conditional.

I have these #ifdefs for the a5xx and a6xx GPUs that don't have ocmem
in the SoC. Without the #ifdefs, those systems would need to have the
ocmem driver in their kernel.

Thanks for the quick review on the patch set!

Brian

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 6/6] drm/msm/gpu: add ocmem init/cleanup functions
  2019-06-17  0:18     ` Brian Masney
@ 2019-06-17  3:43       ` Bjorn Andersson
  0 siblings, 0 replies; 24+ messages in thread
From: Bjorn Andersson @ 2019-06-17  3:43 UTC (permalink / raw)
  To: Brian Masney
  Cc: agross, david.brown, robdclark, sean, robh+dt, airlied, daniel,
	mark.rutland, jonathan, linux-arm-msm, linux-kernel, dri-devel,
	freedreno, devicetree

On Sun 16 Jun 17:18 PDT 2019, Brian Masney wrote:

> Hi Bjorn,
> 
> On Sun, Jun 16, 2019 at 11:06:33AM -0700, Bjorn Andersson wrote:
> > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > index 6f7f4114afcf..e0a9409c8a32 100644
> > > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > @@ -29,6 +29,10 @@
> > >  #include "msm_gem.h"
> > >  #include "msm_mmu.h"
> > >  
> > > +#ifdef CONFIG_QCOM_OCMEM
> > > +#  include <soc/qcom/ocmem.h>
> > > +#endif
> > 
> > This file exists (after the previous patch), so no need to make its
> > inclusion conditional.
> > 
> > > +
> > >  static bool zap_available = true;
> > >  
> > >  static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,
> > > @@ -897,6 +901,43 @@ static int adreno_get_pwrlevels(struct device *dev,
> > >  	return 0;
> > >  }
> > >  
> > > +int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,
> > > +			  struct adreno_ocmem *adreno_ocmem)
> > > +{
> > > +#ifdef CONFIG_QCOM_OCMEM
> > 
> > No need to make this conditional.
> 
> I have these #ifdefs for the a5xx and a6xx GPUs that don't have ocmem
> in the SoC. Without the #ifdefs, those systems would need to have the
> ocmem driver in their kernel.
> 

In order to provide the means for compiling a kernel for a[56]xx without
having to compile ocmem you should move these #ifdef to the ocmem
header file and provide static inline dummies for the case when it's
not.

(and use #if IS_ENABLED(CONFIG_FOO))

Don't forget to add
	depends on QCOM_OCMEM || QCOM_OCMEM=n

to the DRM_MSM config option, to allow the driver pair to be selected in
all possible ways.

> Thanks for the quick review on the patch set!
> 

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/6] dt-bindings: soc: qcom: add On Chip MEMory (OCMEM) bindings
  2019-06-16 13:29 ` [PATCH 1/6] dt-bindings: soc: qcom: add On Chip MEMory (OCMEM) bindings Brian Masney
  2019-06-16 17:43   ` Bjorn Andersson
@ 2019-06-17 14:29   ` Rob Herring
  1 sibling, 0 replies; 24+ messages in thread
From: Rob Herring @ 2019-06-17 14:29 UTC (permalink / raw)
  To: Brian Masney
  Cc: Andy Gross, David Brown, Rob Clark, Sean Paul, Bjorn Andersson,
	David Airlie, Daniel Vetter, Mark Rutland, Jonathan Marek,
	linux-arm-msm, linux-kernel, dri-devel, freedreno, devicetree

On Sun, Jun 16, 2019 at 7:29 AM Brian Masney <masneyb@onstation.org> wrote:
>
> Add device tree bindings for the On Chip Memory (OCMEM) that is present
> on some Qualcomm Snapdragon SoCs.
>
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
>  .../bindings/soc/qcom/qcom,ocmem.yaml         | 66 +++++++++++++++++++

.../bindings/sram/

>  1 file changed, 66 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,ocmem.yaml
>
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,ocmem.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,ocmem.yaml
> new file mode 100644
> index 000000000000..5e3ae6311a16
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,ocmem.yaml
> @@ -0,0 +1,66 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/qcom/qcom,ocmem.yaml#

schemas/sram/

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: On Chip Memory (OCMEM) that is present on some Qualcomm Snapdragon SoCs.
> +
> +maintainers:
> +  - Brian Masney <masneyb@onstation.org>
> +
> +description: |
> +  The On Chip Memory (OCMEM) allocator allows various clients to allocate memory

Is there something in the h/w that's an allocator? That's typically a
s/w thing that has nothing to do with h/w description.

> +  from OCMEM based on performance, latency and power requirements. This is
> +  typically used by the GPU, camera/video, and audio components on some
> +  Snapdragon SoCs.
> +
> +properties:
> +  compatible:
> +    const: qcom,ocmem-msm8974

What Bjorn said...

> +
> +  reg:
> +    items:
> +      - description: Control registers
> +      - description: OCMEM address range
> +
> +  reg-names:
> +    items:
> +      - const: ocmem_ctrl_physical
> +      - const: ocmem_physical

'ctrl' and 'mem' would be sufficient.

> +
> +  clocks:
> +    items:
> +      - description: Core clock
> +      - description: Interface clock
> +
> +  clock-names:
> +    items:
> +      - const: core
> +      - const: iface
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - clocks
> +  - clock-names
> +
> +examples:
> +  - |
> +      #include <dt-bindings/clock/qcom,rpmcc.h>
> +      #include <dt-bindings/clock/qcom,mmcc-msm8974.h>
> +
> +      ocmem: ocmem@fdd00000 {
> +        compatible = "qcom,ocmem-msm8974";
> +
> +        reg = <0xfdd00000 0x2000>,
> +               <0xfec00000 0x180000>;
> +        reg-names = "ocmem_ctrl_physical",
> +                    "ocmem_physical";
> +
> +        clocks = <&rpmcc RPM_SMD_OCMEMGX_CLK>,
> +                  <&mmcc OCMEMCX_OCMEMNOC_CLK>;
> +        clock-names = "core",
> +                      "iface";
> +      };
> --
> 2.20.1
>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 5/6] soc: qcom: add OCMEM driver
  2019-06-16 17:41   ` Bjorn Andersson
@ 2019-06-18  2:02     ` Brian Masney
  2019-06-18  2:29       ` Rob Clark
  0 siblings, 1 reply; 24+ messages in thread
From: Brian Masney @ 2019-06-18  2:02 UTC (permalink / raw)
  To: Bjorn Andersson, robdclark
  Cc: agross, david.brown, sean, robh+dt, airlied, daniel,
	mark.rutland, jonathan, linux-arm-msm, linux-kernel, dri-devel,
	freedreno, devicetree

Hi Rob Clark,

On Sun, Jun 16, 2019 at 10:41:06AM -0700, Bjorn Andersson wrote:
> > diff --git a/drivers/soc/qcom/ocmem.xml.h b/drivers/soc/qcom/ocmem.xml.h
> 
> I would prefer that these lived at the top of the c file, rather than
> being generated.

I think it would be nice to make this change as well.

Rob C: Your original file ocmem.xml.h was licensed under the MIT
license. I just wanted confirmation from you that it's OK to put
the contents of that file into ocmem.c which has the GPL 2.0 only
SPDX license tag. This will relicense the work. I imagine it's not
an issue but I just wanted to get confirmation so there is no
ambiguity regarding the licensing in the future.

Brian


> 
> > new file mode 100644
> > index 000000000000..b4bfb85d1e33
> > --- /dev/null
> > +++ b/drivers/soc/qcom/ocmem.xml.h
> > @@ -0,0 +1,86 @@
> > +/* SPDX-License-Identifier: MIT */
> > +
> > +#ifndef OCMEM_XML
> > +#define OCMEM_XML
> > +
> > +/* Autogenerated file, DO NOT EDIT manually!
> > +
> > +This file was generated by the rules-ng-ng headergen tool in this git repository:
> > +http://github.com/freedreno/envytools/
> > +git clone https://github.com/freedreno/envytools.git
> > +
> > +The rules-ng-ng source files this header was generated from are:
> > +- /home/robclark/src/freedreno/envytools/rnndb/adreno/ocmem.xml         (   1773 bytes, from 2015-09-24 17:30:00)
> > +
> > +Copyright (C) 2013-2015 by the following authors:
> > +- Rob Clark <robdclark@gmail.com> (robclark)
> > +*/

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 5/6] soc: qcom: add OCMEM driver
  2019-06-18  2:02     ` Brian Masney
@ 2019-06-18  2:29       ` Rob Clark
  0 siblings, 0 replies; 24+ messages in thread
From: Rob Clark @ 2019-06-18  2:29 UTC (permalink / raw)
  To: Brian Masney
  Cc: Bjorn Andersson, agross, David Brown, Sean Paul, Rob Herring,
	David Airlie, Daniel Vetter, Mark Rutland, Jonathan,
	linux-arm-msm, Linux Kernel Mailing List, dri-devel, freedreno,
	devicetree

On Mon, Jun 17, 2019 at 7:02 PM Brian Masney <masneyb@onstation.org> wrote:
>
> Hi Rob Clark,
>
> On Sun, Jun 16, 2019 at 10:41:06AM -0700, Bjorn Andersson wrote:
> > > diff --git a/drivers/soc/qcom/ocmem.xml.h b/drivers/soc/qcom/ocmem.xml.h
> >
> > I would prefer that these lived at the top of the c file, rather than
> > being generated.
>
> I think it would be nice to make this change as well.
>
> Rob C: Your original file ocmem.xml.h was licensed under the MIT
> license. I just wanted confirmation from you that it's OK to put
> the contents of that file into ocmem.c which has the GPL 2.0 only
> SPDX license tag. This will relicense the work. I imagine it's not
> an issue but I just wanted to get confirmation so there is no
> ambiguity regarding the licensing in the future.

fine by me.. I defaulted to generated headers since that is extremely
useful for gpu side of things (and userspace stuff defaults to MIT),
but probably overkill for ocmem which just has a handful of registers
(and no need for decoding userspace blob cmdstream dumps)

BR,
-R


>
> Brian
>
>
> >
> > > new file mode 100644
> > > index 000000000000..b4bfb85d1e33
> > > --- /dev/null
> > > +++ b/drivers/soc/qcom/ocmem.xml.h
> > > @@ -0,0 +1,86 @@
> > > +/* SPDX-License-Identifier: MIT */
> > > +
> > > +#ifndef OCMEM_XML
> > > +#define OCMEM_XML
> > > +
> > > +/* Autogenerated file, DO NOT EDIT manually!
> > > +
> > > +This file was generated by the rules-ng-ng headergen tool in this git repository:
> > > +http://github.com/freedreno/envytools/
> > > +git clone https://github.com/freedreno/envytools.git
> > > +
> > > +The rules-ng-ng source files this header was generated from are:
> > > +- /home/robclark/src/freedreno/envytools/rnndb/adreno/ocmem.xml         (   1773 bytes, from 2015-09-24 17:30:00)
> > > +
> > > +Copyright (C) 2013-2015 by the following authors:
> > > +- Rob Clark <robdclark@gmail.com> (robclark)
> > > +*/

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Freedreno] [PATCH 2/6] dt-bindings: display: msm: gmu: add optional ocmem property
  2019-06-16 13:29 ` [PATCH 2/6] dt-bindings: display: msm: gmu: add optional ocmem property Brian Masney
@ 2019-06-19 18:06   ` Jordan Crouse
  2019-06-19 20:16   ` Rob Herring
  1 sibling, 0 replies; 24+ messages in thread
From: Jordan Crouse @ 2019-06-19 18:06 UTC (permalink / raw)
  To: Brian Masney
  Cc: agross, david.brown, robdclark, sean, robh+dt, mark.rutland,
	devicetree, jonathan, airlied, linux-arm-msm, linux-kernel,
	dri-devel, bjorn.andersson, daniel, freedreno

On Sun, Jun 16, 2019 at 09:29:26AM -0400, Brian Masney wrote:
> Some A3xx and A4xx Adreno GPUs do not have GMEM inside the GPU core and
> must use the On Chip MEMory (OCMEM) in order to be functional. Add the
> optional ocmem property to the Adreno Graphics Management Unit bindings.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
>  Documentation/devicetree/bindings/display/msm/gmu.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/msm/gmu.txt b/Documentation/devicetree/bindings/display/msm/gmu.txt
> index 90af5b0a56a9..c746b95e95d4 100644
> --- a/Documentation/devicetree/bindings/display/msm/gmu.txt
> +++ b/Documentation/devicetree/bindings/display/msm/gmu.txt
> @@ -31,6 +31,10 @@ Required properties:
>  - iommus: phandle to the adreno iommu
>  - operating-points-v2: phandle to the OPP operating points
>  
> +Optional properties:
> +- ocmem: phandle to the On Chip Memory (OCMEM) that's present on some Snapdragon
> +         SoCs. See Documentation/devicetree/bindings/soc/qcom/qcom,ocmem.yaml.
> +
>  Example:

You should add a full-fledged a4xx example here including a ocmem phandle.

Jordan

>  / {

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Freedreno] [PATCH 6/6] drm/msm/gpu: add ocmem init/cleanup functions
  2019-06-16 13:29 ` [PATCH 6/6] drm/msm/gpu: add ocmem init/cleanup functions Brian Masney
  2019-06-16 18:06   ` Bjorn Andersson
@ 2019-06-19 18:15   ` Jordan Crouse
  2019-06-19 18:21     ` Jordan Crouse
  1 sibling, 1 reply; 24+ messages in thread
From: Jordan Crouse @ 2019-06-19 18:15 UTC (permalink / raw)
  To: Brian Masney
  Cc: agross, david.brown, robdclark, sean, robh+dt, mark.rutland,
	devicetree, jonathan, airlied, linux-arm-msm, linux-kernel,
	dri-devel, bjorn.andersson, daniel, freedreno

On Sun, Jun 16, 2019 at 09:29:30AM -0400, Brian Masney wrote:
> The files a3xx_gpu.c and a4xx_gpu.c have ifdefs for the OCMEM support
> that was missing upstream. Add two new functions (adreno_gpu_ocmem_init
> and adreno_gpu_ocmem_cleanup) that removes some duplicated code. We also
> need to change the ifdef check for CONFIG_MSM_OCMEM to CONFIG_QCOM_OCMEM
> now that OCMEM support is upstream.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
>  drivers/gpu/drm/msm/adreno/a3xx_gpu.c   | 33 +++++++-------------
>  drivers/gpu/drm/msm/adreno/a3xx_gpu.h   |  3 +-
>  drivers/gpu/drm/msm/adreno/a4xx_gpu.c   | 30 ++++++------------
>  drivers/gpu/drm/msm/adreno/a4xx_gpu.h   |  3 +-
>  drivers/gpu/drm/msm/adreno/adreno_gpu.c | 41 +++++++++++++++++++++++++
>  drivers/gpu/drm/msm/adreno/adreno_gpu.h | 10 ++++++
>  6 files changed, 74 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> index c3b4bc6e4155..72720bb2aca1 100644
> --- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> @@ -17,10 +17,6 @@
>   * this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> -#ifdef CONFIG_MSM_OCMEM
> -#  include <mach/ocmem.h>
> -#endif
> -
>  #include "a3xx_gpu.h"
>  
>  #define A3XX_INT0_MASK \
> @@ -206,9 +202,9 @@ static int a3xx_hw_init(struct msm_gpu *gpu)
>  		gpu_write(gpu, REG_A3XX_RBBM_GPR0_CTL, 0x00000000);
>  
>  	/* Set the OCMEM base address for A330, etc */
> -	if (a3xx_gpu->ocmem_hdl) {
> +	if (a3xx_gpu->ocmem.hdl) {
>  		gpu_write(gpu, REG_A3XX_RB_GMEM_BASE_ADDR,
> -			(unsigned int)(a3xx_gpu->ocmem_base >> 14));
> +			(unsigned int)(a3xx_gpu->ocmem.base >> 14));
>  	}
>  
>  	/* Turn on performance counters: */
> @@ -329,10 +325,7 @@ static void a3xx_destroy(struct msm_gpu *gpu)
>  
>  	adreno_gpu_cleanup(adreno_gpu);
>  
> -#ifdef CONFIG_MSM_OCMEM
> -	if (a3xx_gpu->ocmem_base)
> -		ocmem_free(OCMEM_GRAPHICS, a3xx_gpu->ocmem_hdl);
> -#endif
> +	adreno_gpu_ocmem_cleanup(&a3xx_gpu->ocmem);
>  
>  	kfree(a3xx_gpu);
>  }
> @@ -507,17 +500,10 @@ struct msm_gpu *a3xx_gpu_init(struct drm_device *dev)
>  
>  	/* if needed, allocate gmem: */
>  	if (adreno_is_a330(adreno_gpu)) {
> -#ifdef CONFIG_MSM_OCMEM
> -		/* TODO this is different/missing upstream: */
> -		struct ocmem_buf *ocmem_hdl =
> -				ocmem_allocate(OCMEM_GRAPHICS, adreno_gpu->gmem);
> -
> -		a3xx_gpu->ocmem_hdl = ocmem_hdl;
> -		a3xx_gpu->ocmem_base = ocmem_hdl->addr;
> -		adreno_gpu->gmem = ocmem_hdl->len;
> -		DBG("using %dK of OCMEM at 0x%08x", adreno_gpu->gmem / 1024,
> -				a3xx_gpu->ocmem_base);
> -#endif
> +		ret = adreno_gpu_ocmem_init(&adreno_gpu->base.pdev->dev,
> +					    adreno_gpu, &a3xx_gpu->ocmem);
> +		if (ret)
> +			goto fail;
>  	}
>  
>  	if (!gpu->aspace) {
> @@ -530,11 +516,14 @@ struct msm_gpu *a3xx_gpu_init(struct drm_device *dev)
>  		 */
>  		DRM_DEV_ERROR(dev->dev, "No memory protection without IOMMU\n");
>  		ret = -ENXIO;
> -		goto fail;
> +		goto fail_cleanup_ocmem;
>  	}
>  
>  	return gpu;
>  
> +fail_cleanup_ocmem:
> +	adreno_gpu_ocmem_cleanup(&a3xx_gpu->ocmem);
> +
>  fail:
>  	if (a3xx_gpu)
>  		a3xx_destroy(&a3xx_gpu->base.base);

a3xx_destroy is going to have to be able to cleanup the ocmem anyway, so you
might as well stick it in there instead of having a second goto label.

> diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.h b/drivers/gpu/drm/msm/adreno/a3xx_gpu.h
> index ab60dc9e344e..727c34f38f9e 100644
> --- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.h
> @@ -30,8 +30,7 @@ struct a3xx_gpu {
>  	struct adreno_gpu base;
>  
>  	/* if OCMEM is used for GMEM: */
> -	uint32_t ocmem_base;
> -	void *ocmem_hdl;
> +	struct adreno_ocmem ocmem;
>  };
>  #define to_a3xx_gpu(x) container_of(x, struct a3xx_gpu, base)
>  
> diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> index ab2b752566d8..b8f825107796 100644
> --- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> @@ -2,9 +2,6 @@
>  /* Copyright (c) 2014 The Linux Foundation. All rights reserved.
>   */
>  #include "a4xx_gpu.h"
> -#ifdef CONFIG_MSM_OCMEM
> -#  include <soc/qcom/ocmem.h>
> -#endif
>  
>  #define A4XX_INT0_MASK \
>  	(A4XX_INT0_RBBM_AHB_ERROR |        \
> @@ -188,7 +185,7 @@ static int a4xx_hw_init(struct msm_gpu *gpu)
>  			(1 << 30) | 0xFFFF);
>  
>  	gpu_write(gpu, REG_A4XX_RB_GMEM_BASE_ADDR,
> -			(unsigned int)(a4xx_gpu->ocmem_base >> 14));
> +			(unsigned int)(a4xx_gpu->ocmem.base >> 14));
>  
>  	/* Turn on performance counters: */
>  	gpu_write(gpu, REG_A4XX_RBBM_PERFCTR_CTL, 0x01);
> @@ -318,10 +315,7 @@ static void a4xx_destroy(struct msm_gpu *gpu)
>  
>  	adreno_gpu_cleanup(adreno_gpu);
>  
> -#ifdef CONFIG_MSM_OCMEM
> -	if (a4xx_gpu->ocmem_base)
> -		ocmem_free(OCMEM_GRAPHICS, a4xx_gpu->ocmem_hdl);
> -#endif
> +	adreno_gpu_ocmem_cleanup(&a4xx_gpu->ocmem);
>  
>  	kfree(a4xx_gpu);
>  }
> @@ -578,17 +572,10 @@ struct msm_gpu *a4xx_gpu_init(struct drm_device *dev)
>  
>  	/* if needed, allocate gmem: */
>  	if (adreno_is_a4xx(adreno_gpu)) {
> -#ifdef CONFIG_MSM_OCMEM
> -		/* TODO this is different/missing upstream: */
> -		struct ocmem_buf *ocmem_hdl =
> -				ocmem_allocate(OCMEM_GRAPHICS, adreno_gpu->gmem);
> -
> -		a4xx_gpu->ocmem_hdl = ocmem_hdl;
> -		a4xx_gpu->ocmem_base = ocmem_hdl->addr;
> -		adreno_gpu->gmem = ocmem_hdl->len;
> -		DBG("using %dK of OCMEM at 0x%08x", adreno_gpu->gmem / 1024,
> -				a4xx_gpu->ocmem_base);
> -#endif
> +		ret = adreno_gpu_ocmem_init(dev->dev, adreno_gpu,
> +					    &a4xx_gpu->ocmem);
> +		if (ret)
> +			goto fail;
>  	}
>  
>  	if (!gpu->aspace) {
> @@ -601,11 +588,14 @@ struct msm_gpu *a4xx_gpu_init(struct drm_device *dev)
>  		 */
>  		DRM_DEV_ERROR(dev->dev, "No memory protection without IOMMU\n");
>  		ret = -ENXIO;
> -		goto fail;
> +		goto fail_cleanup_ocmem;
>  	}
>  
>  	return gpu;
>  
> +fail_cleanup_ocmem:
> +	adreno_gpu_ocmem_cleanup(&a4xx_gpu->ocmem);
> +
>  fail:
>  	if (a4xx_gpu)
>  		a4xx_destroy(&a4xx_gpu->base.base);

And same.

> diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.h b/drivers/gpu/drm/msm/adreno/a4xx_gpu.h
> index d506311ee240..a01448cba2ea 100644
> --- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.h
> @@ -16,8 +16,7 @@ struct a4xx_gpu {
>  	struct adreno_gpu base;
>  
>  	/* if OCMEM is used for GMEM: */
> -	uint32_t ocmem_base;
> -	void *ocmem_hdl;
> +	struct adreno_ocmem ocmem;
>  };
>  #define to_a4xx_gpu(x) container_of(x, struct a4xx_gpu, base)
>  
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 6f7f4114afcf..e0a9409c8a32 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -29,6 +29,10 @@
>  #include "msm_gem.h"
>  #include "msm_mmu.h"
>  
> +#ifdef CONFIG_QCOM_OCMEM

You won't need a ifdef here if the rest of the support is merged too.

> +#  include <soc/qcom/ocmem.h>
> +#endif
> +
>  static bool zap_available = true;
>  
>  static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,
> @@ -897,6 +901,43 @@ static int adreno_get_pwrlevels(struct device *dev,
>  	return 0;
>  }
>  
> +int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,
> +			  struct adreno_ocmem *adreno_ocmem)
> +{
> +#ifdef CONFIG_QCOM_OCMEM

Same. It should be assumed that the generic ocmem functions will be properly
stubbed. I suppose you have an argument for wanting to not compile in this
extra code if it isn't needed, but if so you'll want to #ifdef the entire
function(s) and use stubs in the include file instead.

> +	struct ocmem_buf *ocmem_hdl;
> +	struct ocmem *ocmem;
> +
> +	ocmem = of_get_ocmem(dev);
> +	if (!ocmem) {
> +		/* This is an optional property so return success. */
> +		return 0;
> +	}
> +
> +	ocmem_hdl = ocmem_allocate(ocmem, OCMEM_GRAPHICS, adreno_gpu->gmem);
> +	if (IS_ERR(ocmem_hdl))
> +		return PTR_ERR(ocmem_hdl);
> +
> +	adreno_ocmem->ocmem = ocmem;
> +	adreno_ocmem->base = ocmem_hdl->addr;
> +	adreno_ocmem->hdl = ocmem_hdl;
> +	adreno_gpu->gmem = ocmem_hdl->len;
> +	DBG("using %dK of OCMEM at 0x%08x", adreno_gpu->gmem / 1024,
> +	    adreno_ocmem->base);
> +#endif
> +
> +	return 0;
> +}
> +
> +void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *adreno_ocmem)
> +{
> +#ifdef CONFIG_QCOM_OCMEM
> +	if (adreno_ocmem->base)
> +		ocmem_free(adreno_ocmem->ocmem, OCMEM_GRAPHICS,
> +			   adreno_ocmem->hdl);
> +#endif
> +}
> +
>  int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>  		struct adreno_gpu *adreno_gpu,
>  		const struct adreno_gpu_funcs *funcs, int nr_rings)
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> index 0925606ec9b5..1cd11570323b 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> @@ -136,6 +136,12 @@ struct adreno_gpu {
>  };
>  #define to_adreno_gpu(x) container_of(x, struct adreno_gpu, base)
>  
> +struct adreno_ocmem {
> +	struct ocmem *ocmem;
> +	uint32_t base;
> +	void *hdl;
> +};
> +
>  /* platform config data (ie. from DT, or pdata) */
>  struct adreno_platform_config {
>  	struct adreno_rev rev;
> @@ -241,6 +247,10 @@ void adreno_dump(struct msm_gpu *gpu);
>  void adreno_wait_ring(struct msm_ringbuffer *ring, uint32_t ndwords);
>  struct msm_ringbuffer *adreno_active_ring(struct msm_gpu *gpu);
>  
> +int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,
> +			  struct adreno_ocmem *ocmem);
> +void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *ocmem);
> +
>  int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>  		struct adreno_gpu *gpu, const struct adreno_gpu_funcs *funcs,
>  		int nr_rings);
> -- 
> 2.20.1

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Freedreno] [PATCH 6/6] drm/msm/gpu: add ocmem init/cleanup functions
  2019-06-19 18:15   ` [Freedreno] " Jordan Crouse
@ 2019-06-19 18:21     ` Jordan Crouse
  0 siblings, 0 replies; 24+ messages in thread
From: Jordan Crouse @ 2019-06-19 18:21 UTC (permalink / raw)
  To: Brian Masney, agross, david.brown, robdclark, sean, robh+dt,
	mark.rutland, devicetree, jonathan, airlied, linux-arm-msm,
	linux-kernel, dri-devel, bjorn.andersson, daniel, freedreno

On Wed, Jun 19, 2019 at 12:15:26PM -0600, Jordan Crouse wrote:
> On Sun, Jun 16, 2019 at 09:29:30AM -0400, Brian Masney wrote:
> > The files a3xx_gpu.c and a4xx_gpu.c have ifdefs for the OCMEM support
> > that was missing upstream. Add two new functions (adreno_gpu_ocmem_init
> > and adreno_gpu_ocmem_cleanup) that removes some duplicated code. We also
> > need to change the ifdef check for CONFIG_MSM_OCMEM to CONFIG_QCOM_OCMEM
> > now that OCMEM support is upstream.

Sorry for reviewing v1 when there was a v2 in flight. That will teach me to not
keep up on my email. I think you caught most of this, but a few things I still
saw.

<snip>

Jordan

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/6] dt-bindings: display: msm: gmu: add optional ocmem property
  2019-06-16 13:29 ` [PATCH 2/6] dt-bindings: display: msm: gmu: add optional ocmem property Brian Masney
  2019-06-19 18:06   ` [Freedreno] " Jordan Crouse
@ 2019-06-19 20:16   ` Rob Herring
  2019-06-19 20:21     ` Rob Clark
  1 sibling, 1 reply; 24+ messages in thread
From: Rob Herring @ 2019-06-19 20:16 UTC (permalink / raw)
  To: Brian Masney
  Cc: Andy Gross, David Brown, Rob Clark, Sean Paul, Bjorn Andersson,
	David Airlie, Daniel Vetter, Mark Rutland, Jonathan Marek,
	linux-arm-msm, linux-kernel, dri-devel, freedreno, devicetree

On Sun, Jun 16, 2019 at 7:29 AM Brian Masney <masneyb@onstation.org> wrote:
>
> Some A3xx and A4xx Adreno GPUs do not have GMEM inside the GPU core and
> must use the On Chip MEMory (OCMEM) in order to be functional. Add the
> optional ocmem property to the Adreno Graphics Management Unit bindings.
>
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
>  Documentation/devicetree/bindings/display/msm/gmu.txt | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/msm/gmu.txt b/Documentation/devicetree/bindings/display/msm/gmu.txt
> index 90af5b0a56a9..c746b95e95d4 100644
> --- a/Documentation/devicetree/bindings/display/msm/gmu.txt
> +++ b/Documentation/devicetree/bindings/display/msm/gmu.txt
> @@ -31,6 +31,10 @@ Required properties:
>  - iommus: phandle to the adreno iommu
>  - operating-points-v2: phandle to the OPP operating points
>
> +Optional properties:
> +- ocmem: phandle to the On Chip Memory (OCMEM) that's present on some Snapdragon
> +         SoCs. See Documentation/devicetree/bindings/soc/qcom/qcom,ocmem.yaml.

We already have a couple of similar properties. Lets standardize on
'sram' as that is what TI already uses.

Also, is the whole OCMEM allocated to the GMU? If not you should have
child nodes to subdivide the memory.

Rob

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/6] dt-bindings: display: msm: gmu: add optional ocmem property
  2019-06-19 20:16   ` Rob Herring
@ 2019-06-19 20:21     ` Rob Clark
  2019-06-21  2:14       ` Brian Masney
  0 siblings, 1 reply; 24+ messages in thread
From: Rob Clark @ 2019-06-19 20:21 UTC (permalink / raw)
  To: Rob Herring
  Cc: Brian Masney, Andy Gross, David Brown, Sean Paul,
	Bjorn Andersson, David Airlie, Daniel Vetter, Mark Rutland,
	Jonathan Marek, linux-arm-msm, linux-kernel, dri-devel,
	freedreno, devicetree

On Wed, Jun 19, 2019 at 1:17 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Sun, Jun 16, 2019 at 7:29 AM Brian Masney <masneyb@onstation.org> wrote:
> >
> > Some A3xx and A4xx Adreno GPUs do not have GMEM inside the GPU core and
> > must use the On Chip MEMory (OCMEM) in order to be functional. Add the
> > optional ocmem property to the Adreno Graphics Management Unit bindings.
> >
> > Signed-off-by: Brian Masney <masneyb@onstation.org>
> > ---
> >  Documentation/devicetree/bindings/display/msm/gmu.txt | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/display/msm/gmu.txt b/Documentation/devicetree/bindings/display/msm/gmu.txt
> > index 90af5b0a56a9..c746b95e95d4 100644
> > --- a/Documentation/devicetree/bindings/display/msm/gmu.txt
> > +++ b/Documentation/devicetree/bindings/display/msm/gmu.txt
> > @@ -31,6 +31,10 @@ Required properties:
> >  - iommus: phandle to the adreno iommu
> >  - operating-points-v2: phandle to the OPP operating points
> >
> > +Optional properties:
> > +- ocmem: phandle to the On Chip Memory (OCMEM) that's present on some Snapdragon
> > +         SoCs. See Documentation/devicetree/bindings/soc/qcom/qcom,ocmem.yaml.
>
> We already have a couple of similar properties. Lets standardize on
> 'sram' as that is what TI already uses.
>
> Also, is the whole OCMEM allocated to the GMU? If not you should have
> child nodes to subdivide the memory.
>

iirc, downstream a large chunk of OCMEM is statically allocated for
GPU.. the remainder is dynamically allocated for different use-cases.
The upstream driver Brian is proposing only handles the static
allocation case (and I don't think we have upstream support for the
various audio and video use-cases that used dynamic OCMEM allocation
downstream)

Although maybe we should still have a child node to separate the
statically and dynamically allocated parts?  I'm not sure what would
make the most sense..

BR,
-R

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/6] dt-bindings: display: msm: gmu: add optional ocmem property
  2019-06-19 20:21     ` Rob Clark
@ 2019-06-21  2:14       ` Brian Masney
  2019-06-22 23:28         ` Rob Clark
  0 siblings, 1 reply; 24+ messages in thread
From: Brian Masney @ 2019-06-21  2:14 UTC (permalink / raw)
  To: Rob Clark
  Cc: Rob Herring, Andy Gross, David Brown, Sean Paul, Bjorn Andersson,
	David Airlie, Daniel Vetter, Mark Rutland, Jonathan Marek,
	linux-arm-msm, linux-kernel, dri-devel, freedreno, devicetree

On Wed, Jun 19, 2019 at 01:21:20PM -0700, Rob Clark wrote:
> On Wed, Jun 19, 2019 at 1:17 PM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Sun, Jun 16, 2019 at 7:29 AM Brian Masney <masneyb@onstation.org> wrote:
> > >
> > > Some A3xx and A4xx Adreno GPUs do not have GMEM inside the GPU core and
> > > must use the On Chip MEMory (OCMEM) in order to be functional. Add the
> > > optional ocmem property to the Adreno Graphics Management Unit bindings.
> > >
> > > Signed-off-by: Brian Masney <masneyb@onstation.org>
> > > ---
> > >  Documentation/devicetree/bindings/display/msm/gmu.txt | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/display/msm/gmu.txt b/Documentation/devicetree/bindings/display/msm/gmu.txt
> > > index 90af5b0a56a9..c746b95e95d4 100644
> > > --- a/Documentation/devicetree/bindings/display/msm/gmu.txt
> > > +++ b/Documentation/devicetree/bindings/display/msm/gmu.txt
> > > @@ -31,6 +31,10 @@ Required properties:
> > >  - iommus: phandle to the adreno iommu
> > >  - operating-points-v2: phandle to the OPP operating points
> > >
> > > +Optional properties:
> > > +- ocmem: phandle to the On Chip Memory (OCMEM) that's present on some Snapdragon
> > > +         SoCs. See Documentation/devicetree/bindings/soc/qcom/qcom,ocmem.yaml.
> >
> > We already have a couple of similar properties. Lets standardize on
> > 'sram' as that is what TI already uses.
> >
> > Also, is the whole OCMEM allocated to the GMU? If not you should have
> > child nodes to subdivide the memory.
> >
> 
> iirc, downstream a large chunk of OCMEM is statically allocated for
> GPU.. the remainder is dynamically allocated for different use-cases.
> The upstream driver Brian is proposing only handles the static
> allocation case

It appears that the GPU expects to use a specific region of ocmem,
specifically starting at 0. The freedreno driver allocates 1MB of
ocmem on the Nexus 5 starting at ocmem address 0. As a test, I
changed the starting address to 0.5MB and kmscube shows only half the
cube, and four wide black bars across the screen:

https://www.flickr.com/photos/masneyb/48100534381/

> (and I don't think we have upstream support for the various audio and
> video use-cases that used dynamic OCMEM allocation downstream)

That's my understanding as well.

> Although maybe we should still have a child node to separate the
> statically and dynamically allocated parts?  I'm not sure what would
> make the most sense..

Given that the GPU is expecting a fixed address in ocmem, perhaps it
makes sense to have the child node. How about this based on the
sram/sram.txt bindings?

  ocmem: ocmem@fdd00000 {
    compatible = "qcom,msm8974-ocmem";

    reg = <0xfdd00000 0x2000>, <0xfec00000 0x180000>;
    reg-names = "ctrl", "mem";

    clocks = <&rpmcc RPM_SMD_OCMEMGX_CLK>, <&mmcc OCMEMCX_OCMEMNOC_CLK>;
    clock-names = "core", "iface";

    gmu-sram@0 {
      reg = <0x0 0x100000>;
      pool;
    };

    misc-sram@0 {
      reg = <0x100000 0x080000>;
      export;
    };
  };

I marked the misc pool as export since I've seen in the downstream ocmem
sources a reference to their closed libsensors that runs in userspace.

Looking at the sram bindings led me to the genalloc API
(Documentation/core-api/genalloc.rst). I wonder if this is the way that
this should be done?

Brian

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/6] dt-bindings: display: msm: gmu: add optional ocmem property
  2019-06-21  2:14       ` Brian Masney
@ 2019-06-22 23:28         ` Rob Clark
  0 siblings, 0 replies; 24+ messages in thread
From: Rob Clark @ 2019-06-22 23:28 UTC (permalink / raw)
  To: Brian Masney
  Cc: Rob Herring, Andy Gross, David Brown, Sean Paul, Bjorn Andersson,
	David Airlie, Daniel Vetter, Mark Rutland, Jonathan Marek,
	linux-arm-msm, linux-kernel, dri-devel, freedreno, devicetree

On Thu, Jun 20, 2019 at 7:14 PM Brian Masney <masneyb@onstation.org> wrote:
>
> On Wed, Jun 19, 2019 at 01:21:20PM -0700, Rob Clark wrote:
> > On Wed, Jun 19, 2019 at 1:17 PM Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Sun, Jun 16, 2019 at 7:29 AM Brian Masney <masneyb@onstation.org> wrote:
> > > >
> > > > Some A3xx and A4xx Adreno GPUs do not have GMEM inside the GPU core and
> > > > must use the On Chip MEMory (OCMEM) in order to be functional. Add the
> > > > optional ocmem property to the Adreno Graphics Management Unit bindings.
> > > >
> > > > Signed-off-by: Brian Masney <masneyb@onstation.org>
> > > > ---
> > > >  Documentation/devicetree/bindings/display/msm/gmu.txt | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/display/msm/gmu.txt b/Documentation/devicetree/bindings/display/msm/gmu.txt
> > > > index 90af5b0a56a9..c746b95e95d4 100644
> > > > --- a/Documentation/devicetree/bindings/display/msm/gmu.txt
> > > > +++ b/Documentation/devicetree/bindings/display/msm/gmu.txt
> > > > @@ -31,6 +31,10 @@ Required properties:
> > > >  - iommus: phandle to the adreno iommu
> > > >  - operating-points-v2: phandle to the OPP operating points
> > > >
> > > > +Optional properties:
> > > > +- ocmem: phandle to the On Chip Memory (OCMEM) that's present on some Snapdragon
> > > > +         SoCs. See Documentation/devicetree/bindings/soc/qcom/qcom,ocmem.yaml.
> > >
> > > We already have a couple of similar properties. Lets standardize on
> > > 'sram' as that is what TI already uses.
> > >
> > > Also, is the whole OCMEM allocated to the GMU? If not you should have
> > > child nodes to subdivide the memory.
> > >
> >
> > iirc, downstream a large chunk of OCMEM is statically allocated for
> > GPU.. the remainder is dynamically allocated for different use-cases.
> > The upstream driver Brian is proposing only handles the static
> > allocation case
>
> It appears that the GPU expects to use a specific region of ocmem,
> specifically starting at 0. The freedreno driver allocates 1MB of
> ocmem on the Nexus 5 starting at ocmem address 0. As a test, I
> changed the starting address to 0.5MB and kmscube shows only half the
> cube, and four wide black bars across the screen:
>
> https://www.flickr.com/photos/masneyb/48100534381/
>
> > (and I don't think we have upstream support for the various audio and
> > video use-cases that used dynamic OCMEM allocation downstream)
>
> That's my understanding as well.
>
> > Although maybe we should still have a child node to separate the
> > statically and dynamically allocated parts?  I'm not sure what would
> > make the most sense..
>
> Given that the GPU is expecting a fixed address in ocmem, perhaps it
> makes sense to have the child node. How about this based on the
> sram/sram.txt bindings?
>
>   ocmem: ocmem@fdd00000 {
>     compatible = "qcom,msm8974-ocmem";
>
>     reg = <0xfdd00000 0x2000>, <0xfec00000 0x180000>;
>     reg-names = "ctrl", "mem";
>
>     clocks = <&rpmcc RPM_SMD_OCMEMGX_CLK>, <&mmcc OCMEMCX_OCMEMNOC_CLK>;
>     clock-names = "core", "iface";
>
>     gmu-sram@0 {
>       reg = <0x0 0x100000>;
>       pool;
>     };
>
>     misc-sram@0 {
>       reg = <0x100000 0x080000>;
>       export;
>     };
>   };
>
> I marked the misc pool as export since I've seen in the downstream ocmem
> sources a reference to their closed libsensors that runs in userspace.
>
> Looking at the sram bindings led me to the genalloc API
> (Documentation/core-api/genalloc.rst). I wonder if this is the way that
> this should be done?

won't claim to be a dt expert, but this seems somewhat sane..  maybe
drop the export until a use-case comes along for that.. or even the
entire second child node?  I guess that comes down to what robher and
others prefer, I can't really speculate too much about the non-gpu
use-cases for ocmem (or if they'll ever be upstream)

BR,
-R

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2019-06-22 23:28 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-16 13:29 [PATCH 0/6] qcom: add OCMEM support Brian Masney
2019-06-16 13:29 ` [PATCH 1/6] dt-bindings: soc: qcom: add On Chip MEMory (OCMEM) bindings Brian Masney
2019-06-16 17:43   ` Bjorn Andersson
2019-06-17 14:29   ` Rob Herring
2019-06-16 13:29 ` [PATCH 2/6] dt-bindings: display: msm: gmu: add optional ocmem property Brian Masney
2019-06-19 18:06   ` [Freedreno] " Jordan Crouse
2019-06-19 20:16   ` Rob Herring
2019-06-19 20:21     ` Rob Clark
2019-06-21  2:14       ` Brian Masney
2019-06-22 23:28         ` Rob Clark
2019-06-16 13:29 ` [PATCH 3/6] firmware: qcom: scm: add support to restore secure config Brian Masney
2019-06-16 17:53   ` Bjorn Andersson
2019-06-16 13:29 ` [PATCH 4/6] firmware: qcom: scm: add OCMEM lock/unlock interface Brian Masney
2019-06-16 17:54   ` Bjorn Andersson
2019-06-16 13:29 ` [PATCH 5/6] soc: qcom: add OCMEM driver Brian Masney
2019-06-16 17:41   ` Bjorn Andersson
2019-06-18  2:02     ` Brian Masney
2019-06-18  2:29       ` Rob Clark
2019-06-16 13:29 ` [PATCH 6/6] drm/msm/gpu: add ocmem init/cleanup functions Brian Masney
2019-06-16 18:06   ` Bjorn Andersson
2019-06-17  0:18     ` Brian Masney
2019-06-17  3:43       ` Bjorn Andersson
2019-06-19 18:15   ` [Freedreno] " Jordan Crouse
2019-06-19 18:21     ` Jordan Crouse

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).