[PATCH 0/5] DRM/MSM: Add support for MSM8956 and Adreno 510

[PATCH 0/5] DRM/MSM: Add support for MSM8956 and Adreno 510

[kholk11]

From: AngeloGioacchino Del Regno <kholk11@gmail.com>

This patch series enables support for MSM8956/76 and its Adreno 510
GPU on the current DRM driver.

The personal aim is to upstream MSM8956 as much as possible.

This code has been tested on two Sony phones featuring the Qualcomm
MSM8956 SoC.

Angelo G. Del Regno (5):
  drm/msm/mdp5: Add optional TBU and TBU_RT clocks
  drm/msm/mdp5: Add configuration for msm8x56
  drm/msm/dsi: Add configuration for 28nm PLL on family B
  drm/msm/dsi: Add configuration for 8x56
  drm/msm/adreno: Add support for Adreno 510 GPU

 .../devicetree/bindings/display/msm/dsi.txt   |  1 +
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c         | 87 +++++++++++++---
 drivers/gpu/drm/msm/adreno/a5xx_power.c       |  7 ++
 drivers/gpu/drm/msm/adreno/adreno_device.c    | 15 +++
 drivers/gpu/drm/msm/adreno/adreno_gpu.h       |  5 +
 drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c      | 99 +++++++++++++++++++
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c      | 10 ++
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h      |  2 +
 drivers/gpu/drm/msm/dsi/dsi_cfg.c             | 22 +++++
 drivers/gpu/drm/msm/dsi/dsi_cfg.h             |  1 +
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c         |  2 +
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.h         |  1 +
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c    | 18 ++++
 13 files changed, 258 insertions(+), 12 deletions(-)

-- 
2.21.0

[PATCH 1/5] drm/msm/mdp5: Add optional TBU and TBU_RT clocks

[kholk11]

From: "Angelo G. Del Regno" <kholk11@gmail.com>

Some SoCs, like MSM8956/8976 (and APQ variants), do feature these
clocks and we need to enable them in order to get the hardware to
properly work.

Signed-off-by: Angelo G. Del Regno <kholk11@gmail.com>
---
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 10 ++++++++++
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index fec6ef1ae3b9..23be9b95dd7e 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -293,6 +293,10 @@ int mdp5_disable(struct mdp5_kms *mdp5_kms)
 	mdp5_kms->enable_count--;
 	WARN_ON(mdp5_kms->enable_count < 0);
 
+	if (mdp5_kms->tbu_rt_clk)
+		clk_disable_unprepare(mdp5_kms->tbu_rt_clk);
+	if (mdp5_kms->tbu_clk)
+		clk_disable_unprepare(mdp5_kms->tbu_clk);
 	clk_disable_unprepare(mdp5_kms->ahb_clk);
 	clk_disable_unprepare(mdp5_kms->axi_clk);
 	clk_disable_unprepare(mdp5_kms->core_clk);
@@ -313,6 +317,10 @@ int mdp5_enable(struct mdp5_kms *mdp5_kms)
 	clk_prepare_enable(mdp5_kms->core_clk);
 	if (mdp5_kms->lut_clk)
 		clk_prepare_enable(mdp5_kms->lut_clk);
+	if (mdp5_kms->tbu_clk)
+		clk_prepare_enable(mdp5_kms->tbu_clk);
+	if (mdp5_kms->tbu_rt_clk)
+		clk_prepare_enable(mdp5_kms->tbu_rt_clk);
 
 	return 0;
 }
@@ -948,6 +956,8 @@ static int mdp5_init(struct platform_device *pdev, struct drm_device *dev)
 
 	/* optional clocks: */
 	get_clk(pdev, &mdp5_kms->lut_clk, "lut", false);
+	get_clk(pdev, &mdp5_kms->tbu_clk, "tbu", false);
+	get_clk(pdev, &mdp5_kms->tbu_rt_clk, "tbu_rt", false);
 
 	/* we need to set a default rate before enabling.  Set a safe
 	 * rate first, then figure out hw revision, and then set a
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h
index d1bf4fdfc815..128866742593 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h
@@ -53,6 +53,8 @@ struct mdp5_kms {
 	struct clk *ahb_clk;
 	struct clk *core_clk;
 	struct clk *lut_clk;
+	struct clk *tbu_clk;
+	struct clk *tbu_rt_clk;
 	struct clk *vsync_clk;
 
 	/*
-- 
2.21.0

Re: [Freedreno] [PATCH 1/5] drm/msm/mdp5: Add optional TBU and TBU_RT clocks

[Jeffrey Hugo]

On Sun, Sep 22, 2019 at 8:16 AM <kholk11@gmail.com> wrote:
>
> From: "Angelo G. Del Regno" <kholk11@gmail.com>
>
> Some SoCs, like MSM8956/8976 (and APQ variants), do feature these
> clocks and we need to enable them in order to get the hardware to
> properly work.
>
> Signed-off-by: Angelo G. Del Regno <kholk11@gmail.com>

I don't see these clocks documented in the mdp5 DT bindings document.
They need to be added in the DT first.

Re: [Freedreno] [PATCH 1/5] drm/msm/mdp5: Add optional TBU and TBU_RT clocks

[AngeloGioacchino Del Regno]

Il giorno lun 23 set 2019 alle ore 02:45 Jeffrey Hugo
<jeffrey.l.hugo@gmail.com> ha scritto:
>
> On Sun, Sep 22, 2019 at 8:16 AM <kholk11@gmail.com> wrote:
> >
> > From: "Angelo G. Del Regno" <kholk11@gmail.com>
> >
> > Some SoCs, like MSM8956/8976 (and APQ variants), do feature these
> > clocks and we need to enable them in order to get the hardware to
> > properly work.
> >
> > Signed-off-by: Angelo G. Del Regno <kholk11@gmail.com>
>
> I don't see these clocks documented in the mdp5 DT bindings document.
> They need to be added in the DT first.

I know, you're right... I've just noticed it. I'm sorry, I've
completely forgotten to
add them to the documentation.
I'll do that ASAP and resend.

[PATCH 2/5] drm/msm/mdp5: Add configuration for msm8x56

[kholk11]

From: "Angelo G. Del Regno" <kholk11@gmail.com>

Add the configuration entries for the MDP5 v1.11, found on MSM8956
and APQ8056.

Signed-off-by: Angelo G. Del Regno <kholk11@gmail.com>
---
 drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c | 99 ++++++++++++++++++++++++
 1 file changed, 99 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
index dd1daf0e305a..9ff44e7fc7c7 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
@@ -545,6 +545,104 @@ const struct mdp5_cfg_hw msm8x96_config = {
 	.max_clk = 412500000,
 };
 
+const struct mdp5_cfg_hw msm8x56_config = {
+	.name = "msm8x56",
+	.mdp = {
+		.count = 1,
+		.caps = MDP_CAP_SMP |
+			MDP_CAP_DSC |
+			MDP_CAP_SRC_SPLIT |
+			0,
+	},
+	.ctl = {
+		.count = 3,
+		.base = { 0x01000, 0x01200, 0x01400 },
+		.flush_hw_mask = 0xffffffff,
+	},
+	.smp = {
+		.mmb_count = 10,
+		.mmb_size = 10240,
+		.clients = {
+			[SSPP_VIG0] = 1, [SSPP_VIG1] = 9,
+			[SSPP_DMA0] = 4,
+			[SSPP_RGB0] = 7, [SSPP_RGB1] = 8,
+		},
+	},
+	.pipe_vig = {
+		.count = 2,
+		.base = { 0x04000, 0x06000 },
+		.caps = MDP_PIPE_CAP_HFLIP	|
+			MDP_PIPE_CAP_VFLIP	|
+			MDP_PIPE_CAP_SCALE	|
+			MDP_PIPE_CAP_CSC	|
+			MDP_PIPE_CAP_DECIMATION	|
+			MDP_PIPE_CAP_SW_PIX_EXT	|
+			0,
+	},
+	.pipe_rgb = {
+		.count = 2,
+		.base = { 0x14000, 0x16000 },
+		.caps = MDP_PIPE_CAP_HFLIP	|
+			MDP_PIPE_CAP_VFLIP	|
+			MDP_PIPE_CAP_DECIMATION	|
+			MDP_PIPE_CAP_SW_PIX_EXT	|
+			0,
+	},
+	.pipe_dma = {
+		.count = 1,
+		.base = { 0x24000 },
+		.caps = MDP_PIPE_CAP_HFLIP	|
+			MDP_PIPE_CAP_VFLIP	|
+			MDP_PIPE_CAP_SW_PIX_EXT	|
+			0,
+	},
+	.pipe_cursor = {
+		.count = 1,
+		.base = { 0x440DC },
+		.caps = MDP_PIPE_CAP_HFLIP	|
+			MDP_PIPE_CAP_VFLIP	|
+			MDP_PIPE_CAP_SW_PIX_EXT	|
+			MDP_PIPE_CAP_CURSOR	|
+			0,
+	},
+
+	.lm = {
+		.count = 2,
+		.base = { 0x44000, 0x45000 },
+		.instances = {
+				{ .id = 0, .pp = 0, .dspp = 0,
+				  .caps = MDP_LM_CAP_DISPLAY, },
+				{ .id = 1, .pp = -1, .dspp = -1,
+				  .caps = MDP_LM_CAP_WB },
+			     },
+		.nb_stages = 8,
+		.max_width = 2560,
+		.max_height = 0xFFFF,
+	},
+	.dspp = {
+		.count = 1,
+		.base = { 0x54000 },
+
+	},
+	.pp = {
+		.count = 3,
+		.base = { 0x70000, 0x70800, 0x72000 },
+	},
+	.dsc = {
+		.count = 2,
+		.base = { 0x80000, 0x80400 },
+	},
+	.intf = {
+		.base = { 0x6a000, 0x6a800, 0x6b000 },
+		.connect = {
+			[0] = INTF_DISABLED,
+			[1] = INTF_DSI,
+			[2] = INTF_DSI,
+		},
+	},
+	.max_clk = 360000000,
+};
+
 const struct mdp5_cfg_hw msm8917_config = {
 	.name = "msm8917",
 	.mdp = {
@@ -637,6 +735,7 @@ static const struct mdp5_cfg_handler cfg_handlers[] = {
 	{ .revision = 6, .config = { .hw = &msm8x16_config } },
 	{ .revision = 9, .config = { .hw = &msm8x94_config } },
 	{ .revision = 7, .config = { .hw = &msm8x96_config } },
+	{ .revision = 11, .config = { .hw = &msm8x56_config } },
 	{ .revision = 15, .config = { .hw = &msm8917_config } },
 };
 
-- 
2.21.0

[PATCH 3/5] drm/msm/dsi: Add configuration for 28nm PLL on family B

[kholk11]

From: "Angelo G. Del Regno" <kholk11@gmail.com>

The 28nm PLL has a different iospace on MSM/APQ family B SoCs:
add a new configuration and use it when the DT reports the
"qcom,dsi-phy-28nm-hpm-fam-b" compatible.

Signed-off-by: Angelo G. Del Regno <kholk11@gmail.com>
---
 .../devicetree/bindings/display/msm/dsi.txt    |  1 +
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c          |  2 ++
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.h          |  1 +
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c     | 18 ++++++++++++++++++
 4 files changed, 22 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt b/Documentation/devicetree/bindings/display/msm/dsi.txt
index af95586c898f..d3ba9ee22f38 100644
--- a/Documentation/devicetree/bindings/display/msm/dsi.txt
+++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
@@ -83,6 +83,7 @@ DSI PHY:
 Required properties:
 - compatible: Could be the following
   * "qcom,dsi-phy-28nm-hpm"
+  * "qcom,dsi-phy-28nm-hpm-fam-b"
   * "qcom,dsi-phy-28nm-lp"
   * "qcom,dsi-phy-20nm"
   * "qcom,dsi-phy-28nm-8960"
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
index 4097eca1b3ef..507c0146a305 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
@@ -481,6 +481,8 @@ static const struct of_device_id dsi_phy_dt_match[] = {
 #ifdef CONFIG_DRM_MSM_DSI_28NM_PHY
 	{ .compatible = "qcom,dsi-phy-28nm-hpm",
 	  .data = &dsi_phy_28nm_hpm_cfgs },
+	{ .compatible = "qcom,dsi-phy-28nm-hpm-fam-b",
+	  .data = &dsi_phy_28nm_hpm_famb_cfgs },
 	{ .compatible = "qcom,dsi-phy-28nm-lp",
 	  .data = &dsi_phy_28nm_lp_cfgs },
 #endif
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
index c4069ce6afe6..24b294ed3059 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
@@ -40,6 +40,7 @@ struct msm_dsi_phy_cfg {
 };
 
 extern const struct msm_dsi_phy_cfg dsi_phy_28nm_hpm_cfgs;
+extern const struct msm_dsi_phy_cfg dsi_phy_28nm_hpm_famb_cfgs;
 extern const struct msm_dsi_phy_cfg dsi_phy_28nm_lp_cfgs;
 extern const struct msm_dsi_phy_cfg dsi_phy_20nm_cfgs;
 extern const struct msm_dsi_phy_cfg dsi_phy_28nm_8960_cfgs;
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
index b3f678f6c2aa..3b9300545e16 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
@@ -142,6 +142,24 @@ const struct msm_dsi_phy_cfg dsi_phy_28nm_hpm_cfgs = {
 	.num_dsi_phy = 2,
 };
 
+const struct msm_dsi_phy_cfg dsi_phy_28nm_hpm_famb_cfgs = {
+	.type = MSM_DSI_PHY_28NM_HPM,
+	.src_pll_truthtable = { {true, true}, {false, true} },
+	.reg_cfg = {
+		.num = 1,
+		.regs = {
+			{"vddio", 100000, 100},
+		},
+	},
+	.ops = {
+		.enable = dsi_28nm_phy_enable,
+		.disable = dsi_28nm_phy_disable,
+		.init = msm_dsi_phy_init_common,
+	},
+	.io_start = { 0x1a94400, 0x1a94800 },
+	.num_dsi_phy = 2,
+};
+
 const struct msm_dsi_phy_cfg dsi_phy_28nm_lp_cfgs = {
 	.type = MSM_DSI_PHY_28NM_LP,
 	.src_pll_truthtable = { {true, true}, {true, true} },
-- 
2.21.0

[PATCH 4/5] drm/msm/dsi: Add configuration for 8x56

[kholk11]

From: "Angelo G. Del Regno" <kholk11@gmail.com>

MSM8956/APQ8056 has DSI version 3:10040002 (DSI 6G V1.4.2), featuring
two DSIs. It needs three clocks (mdp_core, iface, bus), one GDSC and
two vregs, VDDA at 1.2V and VDDIO at 1.8V.

Signed-off-by: Angelo G. Del Regno <kholk11@gmail.com>
---
 drivers/gpu/drm/msm/dsi/dsi_cfg.c | 22 ++++++++++++++++++++++
 drivers/gpu/drm/msm/dsi/dsi_cfg.h |  1 +
 2 files changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
index b7b7c1a9164a..d585ab7acde2 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
@@ -66,6 +66,26 @@ static const struct msm_dsi_config msm8916_dsi_cfg = {
 	.num_dsi = 1,
 };
 
+static const char * const dsi_8956_bus_clk_names[] = {
+	"mdp_core", "iface", "bus",
+};
+
+static const struct msm_dsi_config msm8956_dsi_cfg = {
+	.io_offset = DSI_6G_REG_SHIFT,
+	.reg_cfg = {
+		.num = 3,
+		.regs = {
+			{"gdsc", -1, -1},
+			{"vdda", 100000, 100},	/* 1.2 V */
+			{"vddio", 100000, 100},	/* 1.8 V */
+		},
+	},
+	.bus_clk_names = dsi_8956_bus_clk_names,
+	.num_bus_clks = ARRAY_SIZE(dsi_8956_bus_clk_names),
+	.io_start = { 0x1a94000, 0x1a96000 },
+	.num_dsi = 2,
+};
+
 static const struct msm_dsi_config msm8994_dsi_cfg = {
 	.io_offset = DSI_6G_REG_SHIFT,
 	.reg_cfg = {
@@ -197,6 +217,8 @@ static const struct msm_dsi_cfg_handler dsi_cfg_handlers[] = {
 		&msm8916_dsi_cfg, &msm_dsi_6g_host_ops},
 	{MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V1_4_1,
 		&msm8996_dsi_cfg, &msm_dsi_6g_host_ops},
+	{MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V1_4_2,
+		&msm8956_dsi_cfg, &msm_dsi_6g_host_ops},
 	{MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V2_2_0,
 		&msm8998_dsi_cfg, &msm_dsi_6g_v2_host_ops},
 	{MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V2_2_1,
diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.h b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
index e2b7a7dfbe49..50a37ceb6a25 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_cfg.h
+++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
@@ -17,6 +17,7 @@
 #define MSM_DSI_6G_VER_MINOR_V1_3	0x10030000
 #define MSM_DSI_6G_VER_MINOR_V1_3_1	0x10030001
 #define MSM_DSI_6G_VER_MINOR_V1_4_1	0x10040001
+#define MSM_DSI_6G_VER_MINOR_V1_4_2	0x10040002
 #define MSM_DSI_6G_VER_MINOR_V2_2_0	0x20000000
 #define MSM_DSI_6G_VER_MINOR_V2_2_1	0x20020001
 
-- 
2.21.0

[PATCH 5/5] drm/msm/adreno: Add support for Adreno 510 GPU

[kholk11]

From: "Angelo G. Del Regno" <kholk11@gmail.com>

The Adreno 510 GPU is a stripped version of the Adreno 5xx,
found in low-end SoCs like 8x56 and 8x76, which has 256K of
GMEM, with no GPMU nor ZAP.
Also, since the Adreno 5xx part of this driver seems to be
developed with high-end Adreno GPUs in mind, and since this
is a lower end one, add a comment making clear which GPUs
which support is not implemented yet is not using the GPMU
related hw init code, so that future developers will not go
crazy with that.

By the way, the lower end Adreno GPUs with no GPMU are:
A505/A506/A510 (no ZAP firmware)
A508/A509/A512 (with ZAP firmware)

Signed-off-by: Angelo G. Del Regno <kholk11@gmail.com>
---
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c      | 87 +++++++++++++++++++---
 drivers/gpu/drm/msm/adreno/a5xx_power.c    |  7 ++
 drivers/gpu/drm/msm/adreno/adreno_device.c | 15 ++++
 drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  5 ++
 4 files changed, 102 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index e9c55d1d6c04..c3814a65ba2d 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -353,6 +353,9 @@ static int a5xx_me_init(struct msm_gpu *gpu)
 		 * 2D mode 3 draw
 		 */
 		OUT_RING(ring, 0x0000000B);
+	} else if (adreno_is_a510(adreno_gpu)) {
+		/* Workaround for token and syncs */
+		OUT_RING(ring, 0x00000001);
 	} else {
 		/* No workarounds enabled */
 		OUT_RING(ring, 0x00000000);
@@ -502,6 +505,8 @@ static int a5xx_zap_shader_init(struct msm_gpu *gpu)
 static int a5xx_hw_init(struct msm_gpu *gpu)
 {
 	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
+	u32 meq_thresh, merciu_sz, roq_thresh_1, roq_thresh_2, eco_cntl;
+	u32 cur_eco_cnt;
 	int ret;
 
 	gpu_write(gpu, REG_A5XX_VBIF_ROUND_ROBIN_QOS_ARB, 0x00000003);
@@ -568,15 +573,31 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
 		0x00100000 + adreno_gpu->gmem - 1);
 	gpu_write(gpu, REG_A5XX_UCHE_GMEM_RANGE_MAX_HI, 0x00000000);
 
-	gpu_write(gpu, REG_A5XX_CP_MEQ_THRESHOLDS, 0x40);
-	if (adreno_is_a530(adreno_gpu))
-		gpu_write(gpu, REG_A5XX_CP_MERCIU_SIZE, 0x40);
+	/* Values for the majority of the models */
+	meq_thresh = 0x40;
+	merciu_sz = 0x40;
+	roq_thresh_2 = 0x80000060;
+	roq_thresh_1 = 0x40201B16;
+	eco_cntl = (0x400 << 11 | 0x300 << 22);
+
+	/* model specific overrides */
+	if (adreno_is_a510(adreno_gpu)) {
+		meq_thresh = 0x20;
+		merciu_sz = 0x20;
+		roq_thresh_2 = 0x40000030;
+		roq_thresh_1 = 0x20100D0A;
+		eco_cntl = (0x200 << 11 | 0x200 << 22);
+	}
+
 	if (adreno_is_a540(adreno_gpu))
-		gpu_write(gpu, REG_A5XX_CP_MERCIU_SIZE, 0x400);
-	gpu_write(gpu, REG_A5XX_CP_ROQ_THRESHOLDS_2, 0x80000060);
-	gpu_write(gpu, REG_A5XX_CP_ROQ_THRESHOLDS_1, 0x40201B16);
+		merciu_sz = 0x400;
+
+	gpu_write(gpu, REG_A5XX_CP_MEQ_THRESHOLDS, meq_thresh);
+	gpu_write(gpu, REG_A5XX_CP_MERCIU_SIZE, merciu_sz);
+	gpu_write(gpu, REG_A5XX_CP_ROQ_THRESHOLDS_2, roq_thresh_2);
+	gpu_write(gpu, REG_A5XX_CP_ROQ_THRESHOLDS_1, roq_thresh_1);
 
-	gpu_write(gpu, REG_A5XX_PC_DBG_ECO_CNTL, (0x400 << 11 | 0x300 << 22));
+	gpu_write(gpu, REG_A5XX_PC_DBG_ECO_CNTL, eco_cntl);
 
 	if (adreno_gpu->info->quirks & ADRENO_QUIRK_TWO_PASS_USE_WFI)
 		gpu_rmw(gpu, REG_A5XX_PC_DBG_ECO_CNTL, 0, (1 << 8));
@@ -589,6 +610,22 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
 	/* Enable ME/PFP split notification */
 	gpu_write(gpu, REG_A5XX_RBBM_AHB_CNTL1, 0xA6FFFFFF);
 
+	/*
+	 *  In A5x, CCU can send context_done event of a particular context to
+	 *  UCHE which ultimately reaches CP even when there is valid
+	 *  transaction of that context inside CCU. This can let CP to program
+	 *  config registers, which will make the "valid transaction" inside
+	 *  CCU to be interpreted differently. This can cause gpu fault. This
+	 *  bug is fixed in latest A510 revision. To enable this bug fix -
+	 *  bit[11] of RB_DBG_ECO_CNTL need to be set to 0, default is 1
+	 *  (disable). For older A510 version this bit is unused.
+	 */
+	if (adreno_is_a510(adreno_gpu)) {
+		cur_eco_cnt = gpu_read(gpu, REG_A5XX_RB_DBG_ECO_CNTL);
+		cur_eco_cnt &= ~(1 << 11);
+		gpu_write(gpu, REG_A5XX_RB_DBG_ECO_CNTL, cur_eco_cnt);
+	}
+
 	/* Enable HWCG */
 	a5xx_set_hwcg(gpu, true);
 
@@ -635,7 +672,7 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
 	/* UCHE */
 	gpu_write(gpu, REG_A5XX_CP_PROTECT(16), ADRENO_PROTECT_RW(0xE80, 16));
 
-	if (adreno_is_a530(adreno_gpu))
+	if (adreno_is_a530(adreno_gpu) || adreno_is_a510(adreno_gpu))
 		gpu_write(gpu, REG_A5XX_CP_PROTECT(17),
 			ADRENO_PROTECT_RW(0x10000, 0x8000));
 
@@ -679,7 +716,8 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
 
 	a5xx_preempt_hw_init(gpu);
 
-	a5xx_gpmu_ucode_init(gpu);
+	if (!adreno_is_a510(adreno_gpu))
+		a5xx_gpmu_ucode_init(gpu);
 
 	ret = a5xx_ucode_init(gpu);
 	if (ret)
@@ -712,12 +750,18 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
 	}
 
 	/*
-	 * Try to load a zap shader into the secure world. If successful
+	 * If the chip that we are using does support loading one, then
+	 * 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.
 	 */
+	if (adreno_is_a510(adreno_gpu)) {
+		gpu_write(gpu, REG_A5XX_RBBM_SECVID_TRUST_CNTL, 0x0);
+		goto skip_zap;
+	}
+
 	ret = a5xx_zap_shader_init(gpu);
 	if (!ret) {
 		OUT_PKT7(gpu->rb[0], CP_SET_SECURE_MODE, 1);
@@ -733,6 +777,7 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
 		gpu_write(gpu, REG_A5XX_RBBM_SECVID_TRUST_CNTL, 0x0);
 	}
 
+skip_zap:
 	/* Last step - yield the ringbuffer */
 	a5xx_preempt_start(gpu);
 
@@ -1066,6 +1111,7 @@ static void a5xx_dump(struct msm_gpu *gpu)
 
 static int a5xx_pm_resume(struct msm_gpu *gpu)
 {
+	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
 	int ret;
 
 	/* Turn on the core power */
@@ -1073,6 +1119,15 @@ static int a5xx_pm_resume(struct msm_gpu *gpu)
 	if (ret)
 		return ret;
 
+	if (adreno_is_a510(adreno_gpu)) {
+		/* Halt the sp_input_clk at HM level */
+		gpu_write(gpu, REG_A5XX_RBBM_CLOCK_CNTL, 0x00000055);
+		a5xx_set_hwcg(gpu, true);
+		/* Turn on sp_input_clk at HM level */
+		gpu_rmw(gpu, REG_A5XX_RBBM_CLOCK_CNTL, 0xFF, 0);
+		return 0;
+	}
+
 	/* Turn the RBCCU domain first to limit the chances of voltage droop */
 	gpu_write(gpu, REG_A5XX_GPMU_RBCCU_POWER_CNTL, 0x778000);
 
@@ -1101,9 +1156,17 @@ static int a5xx_pm_resume(struct msm_gpu *gpu)
 
 static int a5xx_pm_suspend(struct msm_gpu *gpu)
 {
+	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
+	u32 xin_halt_ctrl0_mask = 0xF;
+
+	/* A510 has 3 XIN ports in VBIF */
+	if (adreno_is_a510(adreno_gpu))
+		xin_halt_ctrl0_mask = 0x7;
+
 	/* Clear the VBIF pipe before shutting down */
-	gpu_write(gpu, REG_A5XX_VBIF_XIN_HALT_CTRL0, 0xF);
-	spin_until((gpu_read(gpu, REG_A5XX_VBIF_XIN_HALT_CTRL1) & 0xF) == 0xF);
+	gpu_write(gpu, REG_A5XX_VBIF_XIN_HALT_CTRL0, xin_halt_ctrl0_mask);
+	spin_until((gpu_read(gpu, REG_A5XX_VBIF_XIN_HALT_CTRL1) &
+				xin_halt_ctrl0_mask) == xin_halt_ctrl0_mask);
 
 	gpu_write(gpu, REG_A5XX_VBIF_XIN_HALT_CTRL0, 0);
 
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_power.c b/drivers/gpu/drm/msm/adreno/a5xx_power.c
index a3a06db675ba..58c374664c7f 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_power.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_power.c
@@ -297,6 +297,10 @@ int a5xx_power_init(struct msm_gpu *gpu)
 	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
 	int ret;
 
+	/* A505/A506/A510 (no ZAP) and A508/A509/A512 (w/ZAP) have no GPMU */
+	if (adreno_is_a510(adreno_gpu))
+		return 0;
+
 	/* Set up the limits management */
 	if (adreno_is_a530(adreno_gpu))
 		a530_lm_setup(gpu);
@@ -326,6 +330,9 @@ void a5xx_gpmu_ucode_init(struct msm_gpu *gpu)
 	unsigned int *data, *ptr, *cmds;
 	unsigned int cmds_size;
 
+	if (adreno_is_a510(adreno_gpu))
+		return;
+
 	if (a5xx_gpu->gpmu_bo)
 		return;
 
diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
index 40133a43960c..d0cd6bc0123b 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -114,6 +114,21 @@ static const struct adreno_info gpulist[] = {
 		.gmem  = (SZ_1M + SZ_512K),
 		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
 		.init  = a4xx_gpu_init,
+	}, {
+		.rev   = ADRENO_REV(5, 1, 0, ANY_ID),
+		.revn = 510,
+		.name = "A510",
+		.fw = {
+			[ADRENO_FW_PM4] = "a530_pm4.fw",
+			[ADRENO_FW_PFP] = "a530_pfp.fw",
+		},
+		.gmem = SZ_256K,
+		/*
+		 * Increase inactive period to 250 to avoid bouncing
+		 * the GDSC which appears to make it grumpy
+		 */
+		.inactive_period = 250,
+		.init = a5xx_gpu_init,
 	}, {
 		.rev = ADRENO_REV(5, 3, 0, 2),
 		.revn = 530,
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index c7441fb8313e..9f93916c8910 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -206,6 +206,11 @@ static inline int adreno_is_a430(struct adreno_gpu *gpu)
        return gpu->revn == 430;
 }
 
+static inline int adreno_is_a510(struct adreno_gpu *gpu)
+{
+	return gpu->revn == 510;
+}
+
 static inline int adreno_is_a530(struct adreno_gpu *gpu)
 {
 	return gpu->revn == 530;
-- 
2.21.0

Re: [PATCH 5/5] drm/msm/adreno: Add support for Adreno 510 GPU

[Jordan Crouse]

On Sat, Sep 21, 2019 at 12:04:39PM +0200, kholk11@gmail.com wrote:
> From: "Angelo G. Del Regno" <kholk11@gmail.com>
> 
> The Adreno 510 GPU is a stripped version of the Adreno 5xx,
> found in low-end SoCs like 8x56 and 8x76, which has 256K of
> GMEM, with no GPMU nor ZAP.
> Also, since the Adreno 5xx part of this driver seems to be
> developed with high-end Adreno GPUs in mind, and since this
> is a lower end one, add a comment making clear which GPUs
> which support is not implemented yet is not using the GPMU
> related hw init code, so that future developers will not go
> crazy with that.
> 
> By the way, the lower end Adreno GPUs with no GPMU are:
> A505/A506/A510 (no ZAP firmware)
> A508/A509/A512 (with ZAP firmware)

Thanks, just a few comments.  It is good to see some of these lower tier
parts start to make their way upstream.

> Signed-off-by: Angelo G. Del Regno <kholk11@gmail.com>
> ---
>  drivers/gpu/drm/msm/adreno/a5xx_gpu.c      | 87 +++++++++++++++++++---
>  drivers/gpu/drm/msm/adreno/a5xx_power.c    |  7 ++
>  drivers/gpu/drm/msm/adreno/adreno_device.c | 15 ++++
>  drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  5 ++
>  4 files changed, 102 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index e9c55d1d6c04..c3814a65ba2d 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -353,6 +353,9 @@ static int a5xx_me_init(struct msm_gpu *gpu)
>  		 * 2D mode 3 draw
>  		 */
>  		OUT_RING(ring, 0x0000000B);
> +	} else if (adreno_is_a510(adreno_gpu)) {
> +		/* Workaround for token and syncs */
> +		OUT_RING(ring, 0x00000001);
>  	} else {
>  		/* No workarounds enabled */
>  		OUT_RING(ring, 0x00000000);
> @@ -502,6 +505,8 @@ static int a5xx_zap_shader_init(struct msm_gpu *gpu)
>  static int a5xx_hw_init(struct msm_gpu *gpu)
>  {
>  	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> +	u32 meq_thresh, merciu_sz, roq_thresh_1, roq_thresh_2, eco_cntl;
> +	u32 cur_eco_cnt;
>  	int ret;
>  
>  	gpu_write(gpu, REG_A5XX_VBIF_ROUND_ROBIN_QOS_ARB, 0x00000003);
> @@ -568,15 +573,31 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
>  		0x00100000 + adreno_gpu->gmem - 1);
>  	gpu_write(gpu, REG_A5XX_UCHE_GMEM_RANGE_MAX_HI, 0x00000000);
>  
> -	gpu_write(gpu, REG_A5XX_CP_MEQ_THRESHOLDS, 0x40);
> -	if (adreno_is_a530(adreno_gpu))
> -		gpu_write(gpu, REG_A5XX_CP_MERCIU_SIZE, 0x40);
> +	/* Values for the majority of the models */
> +	meq_thresh = 0x40;
> +	merciu_sz = 0x40;
> +	roq_thresh_2 = 0x80000060;
> +	roq_thresh_1 = 0x40201B16;
> +	eco_cntl = (0x400 << 11 | 0x300 << 22);
> +
> +	/* model specific overrides */
> +	if (adreno_is_a510(adreno_gpu)) {
> +		meq_thresh = 0x20;
> +		merciu_sz = 0x20;
> +		roq_thresh_2 = 0x40000030;
> +		roq_thresh_1 = 0x20100D0A;
> +		eco_cntl = (0x200 << 11 | 0x200 << 22);
> +	}
> +
>  	if (adreno_is_a540(adreno_gpu))
> -		gpu_write(gpu, REG_A5XX_CP_MERCIU_SIZE, 0x400);
> -	gpu_write(gpu, REG_A5XX_CP_ROQ_THRESHOLDS_2, 0x80000060);
> -	gpu_write(gpu, REG_A5XX_CP_ROQ_THRESHOLDS_1, 0x40201B16);
> +		merciu_sz = 0x400;
> +
> +	gpu_write(gpu, REG_A5XX_CP_MEQ_THRESHOLDS, meq_thresh);
> +	gpu_write(gpu, REG_A5XX_CP_MERCIU_SIZE, merciu_sz);
> +	gpu_write(gpu, REG_A5XX_CP_ROQ_THRESHOLDS_2, roq_thresh_2);
> +	gpu_write(gpu, REG_A5XX_CP_ROQ_THRESHOLDS_1, roq_thresh_1);
>  
> -	gpu_write(gpu, REG_A5XX_PC_DBG_ECO_CNTL, (0x400 << 11 | 0x300 << 22));
> +	gpu_write(gpu, REG_A5XX_PC_DBG_ECO_CNTL, eco_cntl);

Personally, I am just fine with doing the direct register writes inside of
target specific if/else blocks instead of declaring variables and trying to
support "common" code.

>  
>  	if (adreno_gpu->info->quirks & ADRENO_QUIRK_TWO_PASS_USE_WFI)
>  		gpu_rmw(gpu, REG_A5XX_PC_DBG_ECO_CNTL, 0, (1 << 8));
> @@ -589,6 +610,22 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
>  	/* Enable ME/PFP split notification */
>  	gpu_write(gpu, REG_A5XX_RBBM_AHB_CNTL1, 0xA6FFFFFF);
>  
> +	/*
> +	 *  In A5x, CCU can send context_done event of a particular context to
> +	 *  UCHE which ultimately reaches CP even when there is valid
> +	 *  transaction of that context inside CCU. This can let CP to program
> +	 *  config registers, which will make the "valid transaction" inside
> +	 *  CCU to be interpreted differently. This can cause gpu fault. This
> +	 *  bug is fixed in latest A510 revision. To enable this bug fix -
> +	 *  bit[11] of RB_DBG_ECO_CNTL need to be set to 0, default is 1
> +	 *  (disable). For older A510 version this bit is unused.
> +	 */
> +	if (adreno_is_a510(adreno_gpu)) {
> +		cur_eco_cnt = gpu_read(gpu, REG_A5XX_RB_DBG_ECO_CNTL);
> +		cur_eco_cnt &= ~(1 << 11);
> +		gpu_write(gpu, REG_A5XX_RB_DBG_ECO_CNTL, cur_eco_cnt);

We have a gpu_rmw() function for this very purpose.

> +	}
> +
>  	/* Enable HWCG */
>  	a5xx_set_hwcg(gpu, true);
>  
> @@ -635,7 +672,7 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
>  	/* UCHE */
>  	gpu_write(gpu, REG_A5XX_CP_PROTECT(16), ADRENO_PROTECT_RW(0xE80, 16));
>  
> -	if (adreno_is_a530(adreno_gpu))
> +	if (adreno_is_a530(adreno_gpu) || adreno_is_a510(adreno_gpu))
>  		gpu_write(gpu, REG_A5XX_CP_PROTECT(17),
>  			ADRENO_PROTECT_RW(0x10000, 0x8000));
>  
> @@ -679,7 +716,8 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
>  
>  	a5xx_preempt_hw_init(gpu);
>  
> -	a5xx_gpmu_ucode_init(gpu);
> +	if (!adreno_is_a510(adreno_gpu))
> +		a5xx_gpmu_ucode_init(gpu);

This works for now, but if we start adding other targets without GPMU it could
get messier. If that happens, perhaps a flag or a quirk would be a better
identifier.

>  	ret = a5xx_ucode_init(gpu);
>  	if (ret)
> @@ -712,12 +750,18 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
>  	}
>  
>  	/*
> -	 * Try to load a zap shader into the secure world. If successful
> +	 * If the chip that we are using does support loading one, then
> +	 * 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.
>  	 */
> +	if (adreno_is_a510(adreno_gpu)) {
> +		gpu_write(gpu, REG_A5XX_RBBM_SECVID_TRUST_CNTL, 0x0);
> +		goto skip_zap;
> +	}
> +
>  	ret = a5xx_zap_shader_init(gpu);
>  	if (!ret) {
>  		OUT_PKT7(gpu->rb[0], CP_SET_SECURE_MODE, 1);
> @@ -733,6 +777,7 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
>  		gpu_write(gpu, REG_A5XX_RBBM_SECVID_TRUST_CNTL, 0x0);
>  	}
>  
> +skip_zap:
>  	/* Last step - yield the ringbuffer */
>  	a5xx_preempt_start(gpu);
>  
> @@ -1066,6 +1111,7 @@ static void a5xx_dump(struct msm_gpu *gpu)
>  
>  static int a5xx_pm_resume(struct msm_gpu *gpu)
>  {
> +	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>  	int ret;
>  
>  	/* Turn on the core power */
> @@ -1073,6 +1119,15 @@ static int a5xx_pm_resume(struct msm_gpu *gpu)
>  	if (ret)
>  		return ret;
>  
> +	if (adreno_is_a510(adreno_gpu)) {
> +		/* Halt the sp_input_clk at HM level */
> +		gpu_write(gpu, REG_A5XX_RBBM_CLOCK_CNTL, 0x00000055);
> +		a5xx_set_hwcg(gpu, true);
> +		/* Turn on sp_input_clk at HM level */
> +		gpu_rmw(gpu, REG_A5XX_RBBM_CLOCK_CNTL, 0xFF, 0);

Please use lower case hex.

> +		return 0;
> +	}
> +
>  	/* Turn the RBCCU domain first to limit the chances of voltage droop */
>  	gpu_write(gpu, REG_A5XX_GPMU_RBCCU_POWER_CNTL, 0x778000);
>  
> @@ -1101,9 +1156,17 @@ static int a5xx_pm_resume(struct msm_gpu *gpu)
>  
>  static int a5xx_pm_suspend(struct msm_gpu *gpu)
>  {
> +	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> +	u32 xin_halt_ctrl0_mask = 0xF;

You don't need a crazy name here - just mask should do.  Please use lower case
hex.
> +
> +	/* A510 has 3 XIN ports in VBIF */
> +	if (adreno_is_a510(adreno_gpu))
> +		xin_halt_ctrl0_mask = 0x7;
> +
>  	/* Clear the VBIF pipe before shutting down */
> -	gpu_write(gpu, REG_A5XX_VBIF_XIN_HALT_CTRL0, 0xF);
> -	spin_until((gpu_read(gpu, REG_A5XX_VBIF_XIN_HALT_CTRL1) & 0xF) == 0xF);
> +	gpu_write(gpu, REG_A5XX_VBIF_XIN_HALT_CTRL0, xin_halt_ctrl0_mask);
> +	spin_until((gpu_read(gpu, REG_A5XX_VBIF_XIN_HALT_CTRL1) &
> +				xin_halt_ctrl0_mask) == xin_halt_ctrl0_mask);
>  
>  	gpu_write(gpu, REG_A5XX_VBIF_XIN_HALT_CTRL0, 0);
>  
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_power.c b/drivers/gpu/drm/msm/adreno/a5xx_power.c
> index a3a06db675ba..58c374664c7f 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_power.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_power.c
> @@ -297,6 +297,10 @@ int a5xx_power_init(struct msm_gpu *gpu)
>  	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>  	int ret;
>  
> +/* A505/A506/A510 (no ZAP) and A508/A509/A512 (w/ZAP) have no GPMU */

This is true, but if we don't support A505/A506/A508/A509 yet, we shouldn't
mention them. Otherwise the developer could  get confused.

> +	if (adreno_is_a510(adreno_gpu))
> +		return 0;
> +
>  	/* Set up the limits management */
>  	if (adreno_is_a530(adreno_gpu))
>  		a530_lm_setup(gpu);
> @@ -326,6 +330,9 @@ void a5xx_gpmu_ucode_init(struct msm_gpu *gpu)
>  	unsigned int *data, *ptr, *cmds;
>  	unsigned int cmds_size;
>  
> +	if (adreno_is_a510(adreno_gpu))
> +		return;
> +
>  	if (a5xx_gpu->gpmu_bo)
>  		return;
>  
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> index 40133a43960c..d0cd6bc0123b 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> @@ -114,6 +114,21 @@ static const struct adreno_info gpulist[] = {
>  		.gmem  = (SZ_1M + SZ_512K),
>  		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
>  		.init  = a4xx_gpu_init,
> +	}, {
> +		.rev   = ADRENO_REV(5, 1, 0, ANY_ID),
> +		.revn = 510,
> +		.name = "A510",
> +		.fw = {
> +			[ADRENO_FW_PM4] = "a530_pm4.fw",
> +			[ADRENO_FW_PFP] = "a530_pfp.fw",
> +		},
> +		.gmem = SZ_256K,
> +		/*
> +		 * Increase inactive period to 250 to avoid bouncing
> +		 * the GDSC which appears to make it grumpy
> +		 */
> +		.inactive_period = 250,
> +		.init = a5xx_gpu_init,
>  	}, {
>  		.rev = ADRENO_REV(5, 3, 0, 2),
>  		.revn = 530,
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> index c7441fb8313e..9f93916c8910 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> @@ -206,6 +206,11 @@ static inline int adreno_is_a430(struct adreno_gpu *gpu)
>         return gpu->revn == 430;
>  }
>  
> +static inline int adreno_is_a510(struct adreno_gpu *gpu)
> +{
> +	return gpu->revn == 510;
> +}
> +
>  static inline int adreno_is_a530(struct adreno_gpu *gpu)
>  {
>  	return gpu->revn == 530;
> -- 
> 2.21.0
> 

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

Re: [PATCH 5/5] drm/msm/adreno: Add support for Adreno 510 GPU

[Rob Clark]

On Sat, Sep 21, 2019 at 3:04 AM <kholk11@gmail.com> wrote:
>
> From: "Angelo G. Del Regno" <kholk11@gmail.com>
>
> The Adreno 510 GPU is a stripped version of the Adreno 5xx,
> found in low-end SoCs like 8x56 and 8x76, which has 256K of
> GMEM, with no GPMU nor ZAP.
> Also, since the Adreno 5xx part of this driver seems to be
> developed with high-end Adreno GPUs in mind, and since this
> is a lower end one, add a comment making clear which GPUs
> which support is not implemented yet is not using the GPMU
> related hw init code, so that future developers will not go
> crazy with that.
>
> By the way, the lower end Adreno GPUs with no GPMU are:
> A505/A506/A510 (no ZAP firmware)
> A508/A509/A512 (with ZAP firmware)
>

Hi, thanks for the patch.. one comment below about zap firmware...
which is not completely to do with this patch, but is my thoughts on
how we should clean up zap handling

> Signed-off-by: Angelo G. Del Regno <kholk11@gmail.com>

[snip]

> @@ -679,7 +716,8 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
>
>         a5xx_preempt_hw_init(gpu);
>
> -       a5xx_gpmu_ucode_init(gpu);
> +       if (!adreno_is_a510(adreno_gpu))
> +               a5xx_gpmu_ucode_init(gpu);
>
>         ret = a5xx_ucode_init(gpu);
>         if (ret)
> @@ -712,12 +750,18 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
>         }
>
>         /*
> -        * Try to load a zap shader into the secure world. If successful
> +        * If the chip that we are using does support loading one, then
> +        * 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.
>          */
> +       if (adreno_is_a510(adreno_gpu)) {
> +               gpu_write(gpu, REG_A5XX_RBBM_SECVID_TRUST_CNTL, 0x0);
> +               goto skip_zap;
> +       }

This is something we need to cleanup on a6xx as well.  But it is
actually possible to have the same GPU with and without zap.  We have
this situation today with sdm845, for example.

What I'd like to do is rather than guess whether we can write
RBBM_SECVID_TRUST_CNTL or not (since that goes spectacularly wrong
when we guess incorrectly), is choose based on the presence of the
zap-shader child node in dtb.  (Currently a6xx tries to choose based
on whether zap firmware is present.. which we need to fix.)

Originally I was thinking we could keep the zap-shader node in the
SoC's "core" dtsi (ie. msm8996.dtsi, sdm845.dtsi, etc) and using
/delete-node/ in per-device dts files for devices without zap.. but
(AFAIU) the zap shader ends up being signed with a vendor key in most
cases, meaning that to have a "generic" (not device-specific) distro
image need to have different zap file names/paths for devices from
different vendors.  Given this, I think it makes more sense to move
the zap-shader node into a per-device (or at least, per-vendor) dts
file, ie. something like:

   /* sdm850-lenovo-yoga-c630.dts: */
  gpu {
     zap-shader {
        memory-region = <&gpu_mem>;
        zap-prefix = "LENOVO";
    };
  };

which would trigger the driver to try to load
/lib/firmware/qcom/LENOVO/a630_zap.mbn

(I'd like to, at least for devices that have ACPI/SMBIOS tables,
standardize on using the vendor name from SMBIOS tables as this
prefix.. so we have a way to construct the firmware path if we
eventually have ACPI boot support on the aarch64 laptops.)

BR,
-R

> +
>         ret = a5xx_zap_shader_init(gpu);
>         if (!ret) {
>                 OUT_PKT7(gpu->rb[0], CP_SET_SECURE_MODE, 1);
> @@ -733,6 +777,7 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
>                 gpu_write(gpu, REG_A5XX_RBBM_SECVID_TRUST_CNTL, 0x0);
>         }
>
> +skip_zap:
>         /* Last step - yield the ringbuffer */
>         a5xx_preempt_start(gpu);
>
> @@ -1066,6 +1111,7 @@ static void a5xx_dump(struct msm_gpu *gpu)

Re: [PATCH 5/5] drm/msm/adreno: Add support for Adreno 510 GPU

[AngeloGioacchino Del Regno]

Il giorno lun 23 set 2019 alle ore 18:37 Rob Clark
<robdclark@gmail.com> ha scritto:
>
> On Sat, Sep 21, 2019 at 3:04 AM <kholk11@gmail.com> wrote:
> >
> > From: "Angelo G. Del Regno" <kholk11@gmail.com>
> >
> > The Adreno 510 GPU is a stripped version of the Adreno 5xx,
> > found in low-end SoCs like 8x56 and 8x76, which has 256K of
> > GMEM, with no GPMU nor ZAP.
> > Also, since the Adreno 5xx part of this driver seems to be
> > developed with high-end Adreno GPUs in mind, and since this
> > is a lower end one, add a comment making clear which GPUs
> > which support is not implemented yet is not using the GPMU
> > related hw init code, so that future developers will not go
> > crazy with that.
> >
> > By the way, the lower end Adreno GPUs with no GPMU are:
> > A505/A506/A510 (no ZAP firmware)
> > A508/A509/A512 (with ZAP firmware)
> >
>
> Hi, thanks for the patch.. one comment below about zap firmware...
> which is not completely to do with this patch, but is my thoughts on
> how we should clean up zap handling
>
> > Signed-off-by: Angelo G. Del Regno <kholk11@gmail.com>
>
> [snip]
>
> > @@ -679,7 +716,8 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
> >
> >         a5xx_preempt_hw_init(gpu);
> >
> > -       a5xx_gpmu_ucode_init(gpu);
> > +       if (!adreno_is_a510(adreno_gpu))
> > +               a5xx_gpmu_ucode_init(gpu);
> >
> >         ret = a5xx_ucode_init(gpu);
> >         if (ret)
> > @@ -712,12 +750,18 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
> >         }
> >
> >         /*
> > -        * Try to load a zap shader into the secure world. If successful
> > +        * If the chip that we are using does support loading one, then
> > +        * 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.
> >          */
> > +       if (adreno_is_a510(adreno_gpu)) {
> > +               gpu_write(gpu, REG_A5XX_RBBM_SECVID_TRUST_CNTL, 0x0);
> > +               goto skip_zap;
> > +       }
>
> This is something we need to cleanup on a6xx as well.  But it is
> actually possible to have the same GPU with and without zap.  We have
> this situation today with sdm845, for example.
>
> What I'd like to do is rather than guess whether we can write
> RBBM_SECVID_TRUST_CNTL or not (since that goes spectacularly wrong
> when we guess incorrectly), is choose based on the presence of the
> zap-shader child node in dtb.  (Currently a6xx tries to choose based
> on whether zap firmware is present.. which we need to fix.)
>
> Originally I was thinking we could keep the zap-shader node in the
> SoC's "core" dtsi (ie. msm8996.dtsi, sdm845.dtsi, etc) and using
> /delete-node/ in per-device dts files for devices without zap.. but
> (AFAIU) the zap shader ends up being signed with a vendor key in most
> cases, meaning that to have a "generic" (not device-specific) distro
> image need to have different zap file names/paths for devices from
> different vendors.  Given this, I think it makes more sense to move
> the zap-shader node into a per-device (or at least, per-vendor) dts
> file, ie. something like:
>
>    /* sdm850-lenovo-yoga-c630.dts: */
>   gpu {
>      zap-shader {
>         memory-region = <&gpu_mem>;
>         zap-prefix = "LENOVO";
>     };
>   };
>
> which would trigger the driver to try to load
> /lib/firmware/qcom/LENOVO/a630_zap.mbn
>
> (I'd like to, at least for devices that have ACPI/SMBIOS tables,
> standardize on using the vendor name from SMBIOS tables as this
> prefix.. so we have a way to construct the firmware path if we
> eventually have ACPI boot support on the aarch64 laptops.)
>
> BR,
> -R
>
> > +
> >         ret = a5xx_zap_shader_init(gpu);
> >         if (!ret) {
> >                 OUT_PKT7(gpu->rb[0], CP_SET_SECURE_MODE, 1);
> > @@ -733,6 +777,7 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
> >                 gpu_write(gpu, REG_A5XX_RBBM_SECVID_TRUST_CNTL, 0x0);
> >         }
> >
> > +skip_zap:
> >         /* Last step - yield the ringbuffer */
> >         a5xx_preempt_start(gpu);
> >
> > @@ -1066,6 +1111,7 @@ static void a5xx_dump(struct msm_gpu *gpu)

Thanks to you for the review.
What I've documented there about the A5xx chips having ZAP and the ones not
having it, came out after a research in the downstream KGSL driver, where qcom
does this distinction based on just the Adreno model, which is the main reason
why I did it like that :)))

I am personally not aware of any A5xx chip having this behavior, so that's why I
didn't even try to think different about this patch. It just seemed to
be the right
thing to do...

Re: [PATCH 5/5] drm/msm/adreno: Add support for Adreno 510 GPU

[Rob Clark]

On Mon, Sep 23, 2019 at 10:27 AM AngeloGioacchino Del Regno
<kholk11@gmail.com> wrote:
>
> Il giorno lun 23 set 2019 alle ore 18:37 Rob Clark
> <robdclark@gmail.com> ha scritto:
> >
> > On Sat, Sep 21, 2019 at 3:04 AM <kholk11@gmail.com> wrote:
> > >
> > > From: "Angelo G. Del Regno" <kholk11@gmail.com>
> > >
> > > The Adreno 510 GPU is a stripped version of the Adreno 5xx,
> > > found in low-end SoCs like 8x56 and 8x76, which has 256K of
> > > GMEM, with no GPMU nor ZAP.
> > > Also, since the Adreno 5xx part of this driver seems to be
> > > developed with high-end Adreno GPUs in mind, and since this
> > > is a lower end one, add a comment making clear which GPUs
> > > which support is not implemented yet is not using the GPMU
> > > related hw init code, so that future developers will not go
> > > crazy with that.
> > >
> > > By the way, the lower end Adreno GPUs with no GPMU are:
> > > A505/A506/A510 (no ZAP firmware)
> > > A508/A509/A512 (with ZAP firmware)
> > >
> >
> > Hi, thanks for the patch.. one comment below about zap firmware...
> > which is not completely to do with this patch, but is my thoughts on
> > how we should clean up zap handling
> >
> > > Signed-off-by: Angelo G. Del Regno <kholk11@gmail.com>
> >
> > [snip]
> >
> > > @@ -679,7 +716,8 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
> > >
> > >         a5xx_preempt_hw_init(gpu);
> > >
> > > -       a5xx_gpmu_ucode_init(gpu);
> > > +       if (!adreno_is_a510(adreno_gpu))
> > > +               a5xx_gpmu_ucode_init(gpu);
> > >
> > >         ret = a5xx_ucode_init(gpu);
> > >         if (ret)
> > > @@ -712,12 +750,18 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
> > >         }
> > >
> > >         /*
> > > -        * Try to load a zap shader into the secure world. If successful
> > > +        * If the chip that we are using does support loading one, then
> > > +        * 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.
> > >          */
> > > +       if (adreno_is_a510(adreno_gpu)) {
> > > +               gpu_write(gpu, REG_A5XX_RBBM_SECVID_TRUST_CNTL, 0x0);
> > > +               goto skip_zap;
> > > +       }
> >
> > This is something we need to cleanup on a6xx as well.  But it is
> > actually possible to have the same GPU with and without zap.  We have
> > this situation today with sdm845, for example.
> >
> > What I'd like to do is rather than guess whether we can write
> > RBBM_SECVID_TRUST_CNTL or not (since that goes spectacularly wrong
> > when we guess incorrectly), is choose based on the presence of the
> > zap-shader child node in dtb.  (Currently a6xx tries to choose based
> > on whether zap firmware is present.. which we need to fix.)
> >
> > Originally I was thinking we could keep the zap-shader node in the
> > SoC's "core" dtsi (ie. msm8996.dtsi, sdm845.dtsi, etc) and using
> > /delete-node/ in per-device dts files for devices without zap.. but
> > (AFAIU) the zap shader ends up being signed with a vendor key in most
> > cases, meaning that to have a "generic" (not device-specific) distro
> > image need to have different zap file names/paths for devices from
> > different vendors.  Given this, I think it makes more sense to move
> > the zap-shader node into a per-device (or at least, per-vendor) dts
> > file, ie. something like:
> >
> >    /* sdm850-lenovo-yoga-c630.dts: */
> >   gpu {
> >      zap-shader {
> >         memory-region = <&gpu_mem>;
> >         zap-prefix = "LENOVO";
> >     };
> >   };
> >
> > which would trigger the driver to try to load
> > /lib/firmware/qcom/LENOVO/a630_zap.mbn
> >
> > (I'd like to, at least for devices that have ACPI/SMBIOS tables,
> > standardize on using the vendor name from SMBIOS tables as this
> > prefix.. so we have a way to construct the firmware path if we
> > eventually have ACPI boot support on the aarch64 laptops.)
> >
> > BR,
> > -R
> >
> > > +
> > >         ret = a5xx_zap_shader_init(gpu);
> > >         if (!ret) {
> > >                 OUT_PKT7(gpu->rb[0], CP_SET_SECURE_MODE, 1);
> > > @@ -733,6 +777,7 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
> > >                 gpu_write(gpu, REG_A5XX_RBBM_SECVID_TRUST_CNTL, 0x0);
> > >         }
> > >
> > > +skip_zap:
> > >         /* Last step - yield the ringbuffer */
> > >         a5xx_preempt_start(gpu);
> > >
> > > @@ -1066,6 +1111,7 @@ static void a5xx_dump(struct msm_gpu *gpu)
>
> Thanks to you for the review.
> What I've documented there about the A5xx chips having ZAP and the ones not
> having it, came out after a research in the downstream KGSL driver, where qcom
> does this distinction based on just the Adreno model, which is the main reason
> why I did it like that :)))
>
> I am personally not aware of any A5xx chip having this behavior, so that's why I
> didn't even try to think different about this patch. It just seemed to
> be the right
> thing to do...

I'm not aware of the case where a fw (hyp/tz) difference means you
don't have zap shader on any a5xx device.  But seeing it on a6xx made
me realize that this is really more about the fw than the hw.  Which
is something that I didn't realize when the a5xx zap support initially
landed.

I think what I'd do for now is, instead of if (adreno_is_XYZ() ||
adreno_is_ABC() || ...) { skip_zap }, I'd change that to if
(!has_zap_node)..  and for the dts files we should start adding the
zap node in device specific .dts files.

Also I suppose we might need to slightly change where we load the zap
fw, ie. we can skip trying to load zap fw in the !has_zap_node case.

BR,
-R

Re: [PATCH 5/5] drm/msm/adreno: Add support for Adreno 510 GPU

[AngeloGioacchino Del Regno]

Il giorno lun 23 set 2019 alle ore 20:23 Rob Clark
<robdclark@gmail.com> ha scritto:
>
> On Mon, Sep 23, 2019 at 10:27 AM AngeloGioacchino Del Regno
> <kholk11@gmail.com> wrote:
> >
> > Il giorno lun 23 set 2019 alle ore 18:37 Rob Clark
> > <robdclark@gmail.com> ha scritto:
> > >
> > > On Sat, Sep 21, 2019 at 3:04 AM <kholk11@gmail.com> wrote:
> > > >
> > > > From: "Angelo G. Del Regno" <kholk11@gmail.com>
> > > >
> > > > The Adreno 510 GPU is a stripped version of the Adreno 5xx,
> > > > found in low-end SoCs like 8x56 and 8x76, which has 256K of
> > > > GMEM, with no GPMU nor ZAP.
> > > > Also, since the Adreno 5xx part of this driver seems to be
> > > > developed with high-end Adreno GPUs in mind, and since this
> > > > is a lower end one, add a comment making clear which GPUs
> > > > which support is not implemented yet is not using the GPMU
> > > > related hw init code, so that future developers will not go
> > > > crazy with that.
> > > >
> > > > By the way, the lower end Adreno GPUs with no GPMU are:
> > > > A505/A506/A510 (no ZAP firmware)
> > > > A508/A509/A512 (with ZAP firmware)
> > > >
> > >
> > > Hi, thanks for the patch.. one comment below about zap firmware...
> > > which is not completely to do with this patch, but is my thoughts on
> > > how we should clean up zap handling
> > >
> > > > Signed-off-by: Angelo G. Del Regno <kholk11@gmail.com>
> > >
> > > [snip]
> > >
> > > > @@ -679,7 +716,8 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
> > > >
> > > >         a5xx_preempt_hw_init(gpu);
> > > >
> > > > -       a5xx_gpmu_ucode_init(gpu);
> > > > +       if (!adreno_is_a510(adreno_gpu))
> > > > +               a5xx_gpmu_ucode_init(gpu);
> > > >
> > > >         ret = a5xx_ucode_init(gpu);
> > > >         if (ret)
> > > > @@ -712,12 +750,18 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
> > > >         }
> > > >
> > > >         /*
> > > > -        * Try to load a zap shader into the secure world. If successful
> > > > +        * If the chip that we are using does support loading one, then
> > > > +        * 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.
> > > >          */
> > > > +       if (adreno_is_a510(adreno_gpu)) {
> > > > +               gpu_write(gpu, REG_A5XX_RBBM_SECVID_TRUST_CNTL, 0x0);
> > > > +               goto skip_zap;
> > > > +       }
> > >
> > > This is something we need to cleanup on a6xx as well.  But it is
> > > actually possible to have the same GPU with and without zap.  We have
> > > this situation today with sdm845, for example.
> > >
> > > What I'd like to do is rather than guess whether we can write
> > > RBBM_SECVID_TRUST_CNTL or not (since that goes spectacularly wrong
> > > when we guess incorrectly), is choose based on the presence of the
> > > zap-shader child node in dtb.  (Currently a6xx tries to choose based
> > > on whether zap firmware is present.. which we need to fix.)
> > >
> > > Originally I was thinking we could keep the zap-shader node in the
> > > SoC's "core" dtsi (ie. msm8996.dtsi, sdm845.dtsi, etc) and using
> > > /delete-node/ in per-device dts files for devices without zap.. but
> > > (AFAIU) the zap shader ends up being signed with a vendor key in most
> > > cases, meaning that to have a "generic" (not device-specific) distro
> > > image need to have different zap file names/paths for devices from
> > > different vendors.  Given this, I think it makes more sense to move
> > > the zap-shader node into a per-device (or at least, per-vendor) dts
> > > file, ie. something like:
> > >
> > >    /* sdm850-lenovo-yoga-c630.dts: */
> > >   gpu {
> > >      zap-shader {
> > >         memory-region = <&gpu_mem>;
> > >         zap-prefix = "LENOVO";
> > >     };
> > >   };
> > >
> > > which would trigger the driver to try to load
> > > /lib/firmware/qcom/LENOVO/a630_zap.mbn
> > >
> > > (I'd like to, at least for devices that have ACPI/SMBIOS tables,
> > > standardize on using the vendor name from SMBIOS tables as this
> > > prefix.. so we have a way to construct the firmware path if we
> > > eventually have ACPI boot support on the aarch64 laptops.)
> > >
> > > BR,
> > > -R
> > >
> > > > +
> > > >         ret = a5xx_zap_shader_init(gpu);
> > > >         if (!ret) {
> > > >                 OUT_PKT7(gpu->rb[0], CP_SET_SECURE_MODE, 1);
> > > > @@ -733,6 +777,7 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
> > > >                 gpu_write(gpu, REG_A5XX_RBBM_SECVID_TRUST_CNTL, 0x0);
> > > >         }
> > > >
> > > > +skip_zap:
> > > >         /* Last step - yield the ringbuffer */
> > > >         a5xx_preempt_start(gpu);
> > > >
> > > > @@ -1066,6 +1111,7 @@ static void a5xx_dump(struct msm_gpu *gpu)
> >
> > Thanks to you for the review.
> > What I've documented there about the A5xx chips having ZAP and the ones not
> > having it, came out after a research in the downstream KGSL driver, where qcom
> > does this distinction based on just the Adreno model, which is the main reason
> > why I did it like that :)))
> >
> > I am personally not aware of any A5xx chip having this behavior, so that's why I
> > didn't even try to think different about this patch. It just seemed to
> > be the right
> > thing to do...
>
> I'm not aware of the case where a fw (hyp/tz) difference means you
> don't have zap shader on any a5xx device.  But seeing it on a6xx made
> me realize that this is really more about the fw than the hw.  Which
> is something that I didn't realize when the a5xx zap support initially
> landed.
>
> I think what I'd do for now is, instead of if (adreno_is_XYZ() ||
> adreno_is_ABC() || ...) { skip_zap }, I'd change that to if
> (!has_zap_node)..  and for the dts files we should start adding the
> zap node in device specific .dts files.
>
> Also I suppose we might need to slightly change where we load the zap
> fw, ie. we can skip trying to load zap fw in the !has_zap_node case.
>
> BR,
> -R

Okay, there's no problem and yeah, I agree with you, better be safe than sorry
and better keep it clean from the very beginning.
I'll send a v2 with all the proposed changes to this patch from you
and Jordan ASAP.