[PATCH v1 0/4] drm/msm/a6xx: Add support for zap shader

[PATCH v1 0/4] drm/msm/a6xx: Add support for zap shader

[Jordan Crouse]

This patch series adds support for loading the zap shader on a6xx and using it
to get the GPU out of secure mode.

The Adreno a5xx and a6xx GPUs boot in "secure" mode which restricts the memory
the GPU is allowed to use. To get the GPU out of secure mode we need to write
to a register. However some bootloaders block access to this register and
require that the GPU instead perform a sequence to pull the GPU out of secure
mode. This sequence requires a special "zap" shader that will execute in
secure mode, clear out all the internal GPU settings and then transition to
in-secure mode.

This series adds support for loading and using the zap shader on a6xx assuming
that the shader exists and that the bootloader supports the secure mode. If any
part of the sequence fails then fall back to writing the register. If we get it
wrong, then writing to the register will trigger a protection mode error and
the system will go down.

The actual zap shader works almost identically to the one on 5xx outside of
a minor workaround for system resume. The first patch moves the a5xx specific
support to the generic adreno driver. The second patch add support for the
zap shader and the final two patches add the DT bindings and DT settings for
setting up the reserved memory that the shader requires.


Jordan Crouse (4):
  drm/msm/gpu: Move zap shader loading to adreno
  drm/msm/a6xx: Add zap shader load
  dt-bindings: drm/msm/gpu: Document a5xx / a6xx zap shader region
  arm64: dts: sdm845: Add zap shader region for GPU

 .../devicetree/bindings/display/msm/gpu.txt        |   7 ++
 arch/arm64/boot/dts/qcom/sdm845.dtsi               |  11 ++
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c              | 109 +-------------------
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c              |  38 ++++++-
 drivers/gpu/drm/msm/adreno/adreno_device.c         |   1 +
 drivers/gpu/drm/msm/adreno/adreno_gpu.c            | 113 +++++++++++++++++++++
 drivers/gpu/drm/msm/adreno/adreno_gpu.h            |   6 ++
 7 files changed, 176 insertions(+), 109 deletions(-)

-- 
2.7.4

[PATCH v1 1/4] drm/msm/gpu: Move zap shader loading to adreno

[Jordan Crouse]

a5xx and a6xx both share (mostly) the same code to load the zap shader and
bring the GPU out of secure mode. Move the formerly 5xx specific code to
adreno to make it available for a6xx too.

Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
---

 drivers/gpu/drm/msm/adreno/a5xx_gpu.c   | 109 +-----------------------------
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 113 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/msm/adreno/adreno_gpu.h |   6 ++
 3 files changed, 120 insertions(+), 108 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index d5f5e56..e5fcefa 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -15,9 +15,6 @@
 #include <linux/types.h>
 #include <linux/cpumask.h>
 #include <linux/qcom_scm.h>
-#include <linux/dma-mapping.h>
-#include <linux/of_address.h>
-#include <linux/soc/qcom/mdt_loader.h>
 #include <linux/pm_opp.h>
 #include <linux/nvmem-consumer.h>
 #include <linux/slab.h>
@@ -30,94 +27,6 @@ static void a5xx_dump(struct msm_gpu *gpu);
 
 #define GPU_PAS_ID 13
 
-static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname)
-{
-	struct device *dev = &gpu->pdev->dev;
-	const struct firmware *fw;
-	struct device_node *np;
-	struct resource r;
-	phys_addr_t mem_phys;
-	ssize_t mem_size;
-	void *mem_region = NULL;
-	int ret;
-
-	if (!IS_ENABLED(CONFIG_ARCH_QCOM))
-		return -EINVAL;
-
-	np = of_get_child_by_name(dev->of_node, "zap-shader");
-	if (!np)
-		return -ENODEV;
-
-	np = of_parse_phandle(np, "memory-region", 0);
-	if (!np)
-		return -EINVAL;
-
-	ret = of_address_to_resource(np, 0, &r);
-	if (ret)
-		return ret;
-
-	mem_phys = r.start;
-	mem_size = resource_size(&r);
-
-	/* Request the MDT file for the firmware */
-	fw = adreno_request_fw(to_adreno_gpu(gpu), fwname);
-	if (IS_ERR(fw)) {
-		DRM_DEV_ERROR(dev, "Unable to load %s\n", fwname);
-		return PTR_ERR(fw);
-	}
-
-	/* Figure out how much memory we need */
-	mem_size = qcom_mdt_get_size(fw);
-	if (mem_size < 0) {
-		ret = mem_size;
-		goto out;
-	}
-
-	/* Allocate memory for the firmware image */
-	mem_region = memremap(mem_phys, mem_size,  MEMREMAP_WC);
-	if (!mem_region) {
-		ret = -ENOMEM;
-		goto out;
-	}
-
-	/*
-	 * Load the rest of the MDT
-	 *
-	 * Note that we could be dealing with two different paths, since
-	 * with upstream linux-firmware it would be in a qcom/ subdir..
-	 * adreno_request_fw() handles this, but qcom_mdt_load() does
-	 * not.  But since we've already gotten thru adreno_request_fw()
-	 * we know which of the two cases it is:
-	 */
-	if (to_adreno_gpu(gpu)->fwloc == FW_LOCATION_LEGACY) {
-		ret = qcom_mdt_load(dev, fw, fwname, GPU_PAS_ID,
-				mem_region, mem_phys, mem_size, NULL);
-	} else {
-		char *newname;
-
-		newname = kasprintf(GFP_KERNEL, "qcom/%s", fwname);
-
-		ret = qcom_mdt_load(dev, fw, newname, GPU_PAS_ID,
-				mem_region, mem_phys, mem_size, NULL);
-		kfree(newname);
-	}
-	if (ret)
-		goto out;
-
-	/* Send the image to the secure world */
-	ret = qcom_scm_pas_auth_and_reset(GPU_PAS_ID);
-	if (ret)
-		DRM_DEV_ERROR(dev, "Unable to authorize the image\n");
-
-out:
-	if (mem_region)
-		memunmap(mem_region);
-
-	release_firmware(fw);
-
-	return ret;
-}
-
 static void a5xx_flush(struct msm_gpu *gpu, struct msm_ringbuffer *ring)
 {
 	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
@@ -563,8 +472,6 @@ static int a5xx_zap_shader_resume(struct msm_gpu *gpu)
 static int a5xx_zap_shader_init(struct msm_gpu *gpu)
 {
 	static bool loaded;
-	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
-	struct platform_device *pdev = gpu->pdev;
 	int ret;
 
 	/*
@@ -574,23 +481,9 @@ static int a5xx_zap_shader_init(struct msm_gpu *gpu)
 	if (loaded)
 		return a5xx_zap_shader_resume(gpu);
 
-	/* We need SCM to be able to load the firmware */
-	if (!qcom_scm_is_available()) {
-		DRM_DEV_ERROR(&pdev->dev, "SCM is not available\n");
-		return -EPROBE_DEFER;
-	}
-
-	/* Each GPU has a target specific zap shader firmware name to use */
-	if (!adreno_gpu->info->zapfw) {
-		DRM_DEV_ERROR(&pdev->dev,
-			"Zap shader firmware file not specified for this target\n");
-		return -ENODEV;
-	}
-
-	ret = zap_shader_load_mdt(gpu, adreno_gpu->info->zapfw);
+	ret = adreno_zap_shader_load(gpu, GPU_PAS_ID);
 
 	loaded = !ret;
-
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 2789847..a9eb58c 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -19,13 +19,126 @@
 
 #include <linux/ascii85.h>
 #include <linux/interconnect.h>
+#include <linux/qcom_scm.h>
 #include <linux/kernel.h>
+#include <linux/of_address.h>
 #include <linux/pm_opp.h>
 #include <linux/slab.h>
+#include <linux/soc/qcom/mdt_loader.h>
 #include "adreno_gpu.h"
 #include "msm_gem.h"
 #include "msm_mmu.h"
 
+static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,
+		u32 pasid)
+{
+	struct device *dev = &gpu->pdev->dev;
+	const struct firmware *fw;
+	struct device_node *np;
+	struct resource r;
+	phys_addr_t mem_phys;
+	ssize_t mem_size;
+	void *mem_region = NULL;
+	int ret;
+
+	if (!IS_ENABLED(CONFIG_ARCH_QCOM))
+		return -EINVAL;
+
+	np = of_get_child_by_name(dev->of_node, "zap-shader");
+	if (!np)
+		return -ENODEV;
+
+	np = of_parse_phandle(np, "memory-region", 0);
+	if (!np)
+		return -EINVAL;
+
+	ret = of_address_to_resource(np, 0, &r);
+	if (ret)
+		return ret;
+
+	mem_phys = r.start;
+	mem_size = resource_size(&r);
+
+	/* Request the MDT file for the firmware */
+	fw = adreno_request_fw(to_adreno_gpu(gpu), fwname);
+	if (IS_ERR(fw)) {
+		DRM_DEV_ERROR(dev, "Unable to load %s\n", fwname);
+		return PTR_ERR(fw);
+	}
+
+	/* Figure out how much memory we need */
+	mem_size = qcom_mdt_get_size(fw);
+	if (mem_size < 0) {
+		ret = mem_size;
+		goto out;
+	}
+
+	/* Allocate memory for the firmware image */
+	mem_region = memremap(mem_phys, mem_size,  MEMREMAP_WC);
+	if (!mem_region) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	/*
+	 * Load the rest of the MDT
+	 *
+	 * Note that we could be dealing with two different paths, since
+	 * with upstream linux-firmware it would be in a qcom/ subdir..
+	 * adreno_request_fw() handles this, but qcom_mdt_load() does
+	 * not.  But since we've already gotten through adreno_request_fw()
+	 * we know which of the two cases it is:
+	 */
+	if (to_adreno_gpu(gpu)->fwloc == FW_LOCATION_LEGACY) {
+		ret = qcom_mdt_load(dev, fw, fwname, pasid,
+				mem_region, mem_phys, mem_size, NULL);
+	} else {
+		char *newname;
+
+		newname = kasprintf(GFP_KERNEL, "qcom/%s", fwname);
+
+		ret = qcom_mdt_load(dev, fw, newname, pasid,
+				mem_region, mem_phys, mem_size, NULL);
+		kfree(newname);
+	}
+	if (ret)
+		goto out;
+
+	/* Send the image to the secure world */
+	ret = qcom_scm_pas_auth_and_reset(pasid);
+	if (ret)
+		DRM_DEV_ERROR(dev, "Unable to authorize the image\n");
+
+out:
+	if (mem_region)
+		memunmap(mem_region);
+
+	release_firmware(fw);
+
+	return ret;
+}
+
+int adreno_zap_shader_load(struct msm_gpu *gpu, u32 pasid)
+{
+	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
+	struct platform_device *pdev = gpu->pdev;
+
+	/* We need SCM to be able to load the firmware */
+	if (!qcom_scm_is_available()) {
+		DRM_DEV_ERROR(&pdev->dev, "SCM is not available\n");
+		return -EPROBE_DEFER;
+	}
+
+	/* Each GPU has a target specific zap shader firmware name to use */
+	if (!adreno_gpu->info->zapfw) {
+		DRM_DEV_ERROR(&pdev->dev,
+			"Zap shader firmware file not specified for this target\n");
+		return -ENODEV;
+	}
+
+	return zap_shader_load_mdt(gpu, adreno_gpu->info->zapfw, pasid);
+}
+
 int adreno_get_param(struct msm_gpu *gpu, uint32_t param, uint64_t *value)
 {
 	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index 5db459b..0925606e 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -252,6 +252,12 @@ void adreno_gpu_state_destroy(struct msm_gpu_state *state);
 int adreno_gpu_state_get(struct msm_gpu *gpu, struct msm_gpu_state *state);
 int adreno_gpu_state_put(struct msm_gpu_state *state);
 
+/*
+ * For a5xx and a6xx targets load the zap shader that is used to pull the GPU
+ * out of secure mode
+ */
+int adreno_zap_shader_load(struct msm_gpu *gpu, u32 pasid);
+
 /* ringbuffer helpers (the parts that are adreno specific) */
 
 static inline void
-- 
2.7.4

[PATCH v1 2/4] drm/msm/a6xx: Add zap shader load

[Jordan Crouse]

The a6xx GPU powers on in secure mode which restricts what memory it can
write to. To get out of secure mode the GPU driver can write to
REG_A6XX_RBBM_SECVID_TRUST_CNTL but on targets that are "secure" that
register region is blocked and writes will cause the system to go down.

For those targets we need to execute a special sequence that involves
loadinga special shader that clears the GPU registers and use a PM4
sequence to pull the GPU out of secure. Add support for loading the zap
shader and executing the secure sequence. For targets that do not support
SCM or the specific SCM sequence this should fail and we would fall back
to writing the register.

Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
---

 drivers/gpu/drm/msm/adreno/a6xx_gpu.c      | 38 +++++++++++++++++++++++++++++-
 drivers/gpu/drm/msm/adreno/adreno_device.c |  1 +
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index fefe773..5983e47 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -10,6 +10,8 @@
 
 #include <linux/devfreq.h>
 
+#define GPU_PAS_ID 13
+
 static inline bool _a6xx_check_idle(struct msm_gpu *gpu)
 {
 	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
@@ -343,6 +345,20 @@ static int a6xx_ucode_init(struct msm_gpu *gpu)
 	return 0;
 }
 
+static int a6xx_zap_shader_init(struct msm_gpu *gpu)
+{
+	static bool loaded;
+	int ret;
+
+	if (loaded)
+		return 0;
+
+	ret = adreno_zap_shader_load(gpu, GPU_PAS_ID);
+
+	loaded = !ret;
+	return ret;
+}
+
 #define A6XX_INT_MASK (A6XX_RBBM_INT_0_MASK_CP_AHB_ERROR | \
 	  A6XX_RBBM_INT_0_MASK_RBBM_ATB_ASYNCFIFO_OVERFLOW | \
 	  A6XX_RBBM_INT_0_MASK_CP_HW_ERROR | \
@@ -491,7 +507,27 @@ static int a6xx_hw_init(struct msm_gpu *gpu)
 	if (ret)
 		goto out;
 
-	gpu_write(gpu, REG_A6XX_RBBM_SECVID_TRUST_CNTL, 0x0);
+	/*
+	 * Try to load a zap shader into the secure world. If successful
+	 * we can use the CP to switch out of secure mode. If not then we
+	 * have no resource but to try to switch ourselves out manually. If we
+	 * guessed wrong then access to the RBBM_SECVID_TRUST_CNTL register will
+	 * be blocked and a permissions violation will soon follow.
+	 */
+	ret = a6xx_zap_shader_init(gpu);
+	if (!ret) {
+		OUT_PKT7(gpu->rb[0], CP_SET_SECURE_MODE, 1);
+		OUT_RING(gpu->rb[0], 0x00000000);
+
+		a6xx_flush(gpu, gpu->rb[0]);
+		if (!a6xx_idle(gpu, gpu->rb[0]))
+			return -EINVAL;
+	} else {
+		/* Print a warning so if we die, we know why */
+		dev_warn_once(gpu->dev->dev,
+			"Zap shader not enabled - using SECVID_TRUST_CNTL instead\n");
+		gpu_write(gpu, REG_A6XX_RBBM_SECVID_TRUST_CNTL, 0x0);
+	}
 
 out:
 	/*
diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
index 714ed65..ead5f6a 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -155,6 +155,7 @@ static const struct adreno_info gpulist[] = {
 		.gmem = SZ_1M,
 		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
 		.init = a6xx_gpu_init,
+		.zapfw = "a630_zap.mdt",
 	},
 };
 
-- 
2.7.4

[PATCH v1 3/4] dt-bindings: drm/msm/gpu: Document a5xx / a6xx zap shader region

[Jordan Crouse]

Describe the zap-shader node that defines a reserved memory region
to store the zap shader.

Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
---

 Documentation/devicetree/bindings/display/msm/gpu.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/msm/gpu.txt b/Documentation/devicetree/bindings/display/msm/gpu.txt
index aad1aef..1e6d870 100644
--- a/Documentation/devicetree/bindings/display/msm/gpu.txt
+++ b/Documentation/devicetree/bindings/display/msm/gpu.txt
@@ -25,6 +25,9 @@ Required properties:
 - qcom,gmu: For GMU attached devices a phandle to the GMU device that will
   control the power for the GPU. Applicable targets:
     - qcom,adreno-630.2
+- zap-shader: For a5xx and a6xx devices this node contains a memory-region that
+  points to reserved memory to store the zap shader that can be used to help
+  bring the GPU out of secure mode.
 
 Example 3xx/4xx/a5xx:
 
@@ -71,5 +74,9 @@ Example a6xx (with GMU):
 		operating-points-v2 = <&gpu_opp_table>;
 
 		qcom,gmu = <&gmu>;
+
+		zap-shader {
+			memory-region = <&zap_shader_region>;
+		};
 	};
 };
-- 
2.7.4

[PATCH v1 4/4] arm64: dts: sdm845: Add zap shader region for GPU

[Jordan Crouse]

Some Adreno GPU targets require a special zap shader to bring the GPU
out of secure mode. Define a region to allocate and store the zap
shader.

Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
---

 arch/arm64/boot/dts/qcom/sdm845.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index fed7a29..526456d 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -100,6 +100,11 @@
 			no-map;
 		};
 
+		zap_shader_region: memory@8c415000 {
+			reg = <0 0x8c415000 0 0x2000>;
+			no-map;
+		};
+
 		mpss_region: memory@8e000000 {
 			reg = <0 0x8e000000 0 0x7800000>;
 			no-map;
@@ -109,6 +114,8 @@
 			reg = <0 0x96500000 0 0x200000>;
 			no-map;
 		};
+
+
 	};
 
 	cpus {
@@ -1514,6 +1521,10 @@
 
 			qcom,gmu = <&gmu>;
 
+			zap-shader {
+				memory-region = <&zap_shader_region>;
+			};
+
 			gpu_opp_table: opp-table {
 				compatible = "operating-points-v2";
 
-- 
2.7.4

Re: [Freedreno] [PATCH v1 4/4] arm64: dts: sdm845: Add zap shader region for GPU

[Rob Clark]

On Tue, Mar 12, 2019 at 2:14 PM Jordan Crouse <jcrouse@codeaurora.org> wrote:
>
> Some Adreno GPU targets require a special zap shader to bring the GPU
> out of secure mode. Define a region to allocate and store the zap
> shader.
>
> Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> ---
>
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index fed7a29..526456d 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -100,6 +100,11 @@
>                         no-map;
>                 };
>
> +               zap_shader_region: memory@8c415000 {
> +                       reg = <0 0x8c415000 0 0x2000>;
> +                       no-map;
> +               };
> +
>                 mpss_region: memory@8e000000 {
>                         reg = <0 0x8e000000 0 0x7800000>;
>                         no-map;
> @@ -109,6 +114,8 @@
>                         reg = <0 0x96500000 0 0x200000>;
>                         no-map;
>                 };
> +
> +

I guess we don't really need this hunk..

other than that, the series lgtm

BR,
-R

>         };
>
>         cpus {
> @@ -1514,6 +1521,10 @@
>
>                         qcom,gmu = <&gmu>;
>
> +                       zap-shader {
> +                               memory-region = <&zap_shader_region>;
> +                       };
> +
>                         gpu_opp_table: opp-table {
>                                 compatible = "operating-points-v2";
>
> --
> 2.7.4
>
> _______________________________________________
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH v1 4/4] arm64: dts: sdm845: Add zap shader region for GPU

[Bjorn Andersson]

On Wed 13 Mar 08:42 PDT 2019, Rob Clark wrote:

> On Tue, Mar 12, 2019 at 2:14 PM Jordan Crouse <jcrouse@codeaurora.org> wrote:
> >
> > Some Adreno GPU targets require a special zap shader to bring the GPU
> > out of secure mode. Define a region to allocate and store the zap
> > shader.
> >
> > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> > ---
> >
> >  arch/arm64/boot/dts/qcom/sdm845.dtsi | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > index fed7a29..526456d 100644
> > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > @@ -100,6 +100,11 @@
> >                         no-map;
> >                 };
> >
> > +               zap_shader_region: memory@8c415000 {
> > +                       reg = <0 0x8c415000 0 0x2000>;
> > +                       no-map;
> > +               };
> > +
> >                 mpss_region: memory@8e000000 {
> >                         reg = <0 0x8e000000 0 0x7800000>;
> >                         no-map;
> > @@ -109,6 +114,8 @@
> >                         reg = <0 0x96500000 0 0x200000>;
> >                         no-map;
> >                 };
> > +
> > +
> 
> I guess we don't really need this hunk..
> 

I'll trim that as I pick up the patch.

> other than that, the series lgtm
> 

Will you pick up patch 1-3 and I'll pick patch 4 through Andy's tree?

Regards,
Bjorn

> BR,
> -R
> 
> >         };
> >
> >         cpus {
> > @@ -1514,6 +1521,10 @@
> >
> >                         qcom,gmu = <&gmu>;
> >
> > +                       zap-shader {
> > +                               memory-region = <&zap_shader_region>;
> > +                       };
> > +
> >                         gpu_opp_table: opp-table {
> >                                 compatible = "operating-points-v2";
> >
> > --
> > 2.7.4
> >
> > _______________________________________________
> > Freedreno mailing list
> > Freedreno@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH v1 4/4] arm64: dts: sdm845: Add zap shader region for GPU

[Rob Clark]

On Thu, Mar 14, 2019 at 1:20 AM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Wed 13 Mar 08:42 PDT 2019, Rob Clark wrote:
>
> > On Tue, Mar 12, 2019 at 2:14 PM Jordan Crouse <jcrouse@codeaurora.org> wrote:
> > >
> > > Some Adreno GPU targets require a special zap shader to bring the GPU
> > > out of secure mode. Define a region to allocate and store the zap
> > > shader.
> > >
> > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> > > ---
> > >
> > >  arch/arm64/boot/dts/qcom/sdm845.dtsi | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > index fed7a29..526456d 100644
> > > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > @@ -100,6 +100,11 @@
> > >                         no-map;
> > >                 };
> > >
> > > +               zap_shader_region: memory@8c415000 {
> > > +                       reg = <0 0x8c415000 0 0x2000>;
> > > +                       no-map;
> > > +               };
> > > +
> > >                 mpss_region: memory@8e000000 {
> > >                         reg = <0 0x8e000000 0 0x7800000>;
> > >                         no-map;
> > > @@ -109,6 +114,8 @@
> > >                         reg = <0 0x96500000 0 0x200000>;
> > >                         no-map;
> > >                 };
> > > +
> > > +
> >
> > I guess we don't really need this hunk..
> >
>
> I'll trim that as I pick up the patch.
>
> > other than that, the series lgtm
> >
>
> Will you pick up patch 1-3 and I'll pick patch 4 through Andy's tree?

yeah, sounds good.

BR,
-R

> Regards,
> Bjorn
>
> > BR,
> > -R
> >
> > >         };
> > >
> > >         cpus {
> > > @@ -1514,6 +1521,10 @@
> > >
> > >                         qcom,gmu = <&gmu>;
> > >
> > > +                       zap-shader {
> > > +                               memory-region = <&zap_shader_region>;
> > > +                       };
> > > +
> > >                         gpu_opp_table: opp-table {
> > >                                 compatible = "operating-points-v2";
> > >
> > > --
> > > 2.7.4
> > >
> > > _______________________________________________
> > > Freedreno mailing list
> > > Freedreno@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH v1 3/4] dt-bindings: drm/msm/gpu: Document a5xx / a6xx zap shader region

[Rob Herring]

On Tue, Mar 12, 2019 at 12:13:41PM -0600, Jordan Crouse wrote:
> Describe the zap-shader node that defines a reserved memory region
> to store the zap shader.
> 
> Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> ---
> 
>  Documentation/devicetree/bindings/display/msm/gpu.txt | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/msm/gpu.txt b/Documentation/devicetree/bindings/display/msm/gpu.txt
> index aad1aef..1e6d870 100644
> --- a/Documentation/devicetree/bindings/display/msm/gpu.txt
> +++ b/Documentation/devicetree/bindings/display/msm/gpu.txt
> @@ -25,6 +25,9 @@ Required properties:
>  - qcom,gmu: For GMU attached devices a phandle to the GMU device that will
>    control the power for the GPU. Applicable targets:
>      - qcom,adreno-630.2
> +- zap-shader: For a5xx and a6xx devices this node contains a memory-region that
> +  points to reserved memory to store the zap shader that can be used to help
> +  bring the GPU out of secure mode.

This is the properties section and zap-shader is not a property.

But why do you need a child node in the first place? Just add 
'memory-region' to the parent.

>  
>  Example 3xx/4xx/a5xx:
>  
> @@ -71,5 +74,9 @@ Example a6xx (with GMU):
>  		operating-points-v2 = <&gpu_opp_table>;
>  
>  		qcom,gmu = <&gmu>;
> +
> +		zap-shader {
> +			memory-region = <&zap_shader_region>;
> +		};
>  	};
>  };
> -- 
> 2.7.4
> 

Re: [PATCH v1 3/4] dt-bindings: drm/msm/gpu: Document a5xx / a6xx zap shader region

[Jordan Crouse]

On Thu, Mar 28, 2019 at 08:32:15AM -0500, Rob Herring wrote:
> On Tue, Mar 12, 2019 at 12:13:41PM -0600, Jordan Crouse wrote:
> > Describe the zap-shader node that defines a reserved memory region
> > to store the zap shader.
> > 
> > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> > ---
> > 
> >  Documentation/devicetree/bindings/display/msm/gpu.txt | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/display/msm/gpu.txt b/Documentation/devicetree/bindings/display/msm/gpu.txt
> > index aad1aef..1e6d870 100644
> > --- a/Documentation/devicetree/bindings/display/msm/gpu.txt
> > +++ b/Documentation/devicetree/bindings/display/msm/gpu.txt
> > @@ -25,6 +25,9 @@ Required properties:
> >  - qcom,gmu: For GMU attached devices a phandle to the GMU device that will
> >    control the power for the GPU. Applicable targets:
> >      - qcom,adreno-630.2
> > +- zap-shader: For a5xx and a6xx devices this node contains a memory-region that
> > +  points to reserved memory to store the zap shader that can be used to help
> > +  bring the GPU out of secure mode.
> 
> This is the properties section and zap-shader is not a property.

Thanks, I can fix that.

> But why do you need a child node in the first place? Just add 
> 'memory-region' to the parent.

Two reasons.  First, this memory is locked in the secure world once the MDT
loader is run and it isn't really intended for CPU access. If the parent device
tries to set up DMA operations thinking it has access to the memory it might not
work out very well.  Putting it in a child makes it clear that this is a special
chunk of memory for a special case.

The second reason is that not all target platforms require the zap shader, so it
would be nice to have it as a child node so that it could be removed on target
platforms that don't need it.

Jordan

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