[PATCH v3 0/3] Use APIs in gdsc genpd to switch gdsc mode for venus v4 core

[PATCH v3 0/3] Use APIs in gdsc genpd to switch gdsc mode for venus v4 core

[Renjiang Han]

The Venus driver requires vcodec GDSC to be ON in SW mode for clock
operations and move it back to HW mode to gain power benefits. Earlier,
as there is no interface to switch the GDSC mode from GenPD framework,
the GDSC is moved to HW control mode as part of GDSC enable callback and
venus driver is writing to its POWER_CONTROL register to keep the GDSC ON
from SW whereever required. But the POWER_CONTROL register addresses are
not constant and can vary across the variants.

Also as per the HW recommendation, the GDSC mode switching needs to be
controlled from respective GDSC register and this is a uniform approach
across all the targets. Hence use dev_pm_genpd_set_hwmode() API which
controls GDSC mode switching using its respective GDSC register.

If only the clock patch or the venus driver patch is merged, the venus
driver will not work properly. Although it does not affect other system
functions, it is still recommended to wait until both the clock patch
and the venus driver patch are reviewed and passed, and then merge them
into the same release by their respective maintainers.

Validated this series on QCS615 and SC7180.

Signed-off-by: Renjiang Han <quic_renjiang@quicinc.com>
---
Changes in v3:
- 1. Update commit message.
- 2. Add a patch to clean up the dead code for the venus driver.
- 3. Remove vcodec_control_v4() function.
- 4. Directly call dev_pm_genpd_set_hwmode() without vcodec_control_v4().
- Link to v2: https://lore.kernel.org/r/20241223-switch_gdsc_mode-v2-0-eb5c96aee662@quicinc.com

Changes in v2:
- 1. Add the HW_CTRL_TRIGGER flag for the targets SM7150/SM8150 and SM8450
video GDSCs supporting movement between HW and SW mode of the GDSC.
(Suggested by Dmitry Baryshkov)
- 2. There is a dependency of the clock driver introducing the new flag
and the video driver adapting to this new API. Missing either the clock
and video driver could potentially break the video driver.
- Link to v1: https://lore.kernel.org/r/20241122-switch_gdsc_mode-v1-0-365f097ecbb0@quicinc.com

---
Renjiang Han (2):
      venus: pm_helpers: Use dev_pm_genpd_set_hwmode to switch GDSC mode on V4
      venus: pm_helpers: Remove dead code and simplify power management

Taniya Das (1):
      clk: qcom: videocc: Use HW_CTRL_TRIGGER flag for video GDSC's

 drivers/clk/qcom/videocc-sc7180.c              |  2 +-
 drivers/clk/qcom/videocc-sdm845.c              |  4 +-
 drivers/clk/qcom/videocc-sm7150.c              |  4 +-
 drivers/clk/qcom/videocc-sm8150.c              |  4 +-
 drivers/clk/qcom/videocc-sm8450.c              |  4 +-
 drivers/media/platform/qcom/venus/pm_helpers.c | 73 +++-----------------------
 6 files changed, 17 insertions(+), 74 deletions(-)
---
base-commit: 63b3ff03d91ae8f875fe8747c781a521f78cde17
change-id: 20250115-switch_gdsc_mode-a9c14fad9a36

Best regards,
-- 
Renjiang Han <quic_renjiang@quicinc.com>

[PATCH v3 1/3] clk: qcom: videocc: Use HW_CTRL_TRIGGER flag for video GDSC's

[Renjiang Han]

From: Taniya Das <quic_tdas@quicinc.com>

The video driver will be using the newly introduced
dev_pm_genpd_set_hwmode() API to switch the video GDSC to HW and SW
control modes at runtime.
Hence use HW_CTRL_TRIGGER flag instead of HW_CTRL for video GDSC's for
Qualcomm SoC SC7180, SDM845, SM7150, SM8150 and SM8450.

Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Renjiang Han <quic_renjiang@quicinc.com>
---
 drivers/clk/qcom/videocc-sc7180.c | 2 +-
 drivers/clk/qcom/videocc-sdm845.c | 4 ++--
 drivers/clk/qcom/videocc-sm7150.c | 4 ++--
 drivers/clk/qcom/videocc-sm8150.c | 4 ++--
 drivers/clk/qcom/videocc-sm8450.c | 4 ++--
 5 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/clk/qcom/videocc-sc7180.c b/drivers/clk/qcom/videocc-sc7180.c
index d7f84548039699ce6fdd7c0f6675c168d5eaf4c1..dd2441d6aa83bd7cff17deeb42f5d011c1e9b134 100644
--- a/drivers/clk/qcom/videocc-sc7180.c
+++ b/drivers/clk/qcom/videocc-sc7180.c
@@ -166,7 +166,7 @@ static struct gdsc vcodec0_gdsc = {
 	.pd = {
 		.name = "vcodec0_gdsc",
 	},
-	.flags = HW_CTRL,
+	.flags = HW_CTRL_TRIGGER,
 	.pwrsts = PWRSTS_OFF_ON,
 };
 
diff --git a/drivers/clk/qcom/videocc-sdm845.c b/drivers/clk/qcom/videocc-sdm845.c
index f77a0777947773dc8902c92098acff71b9b8f10f..6dedc80a8b3e18eca82c08a5bcd7e1fdc374d4b5 100644
--- a/drivers/clk/qcom/videocc-sdm845.c
+++ b/drivers/clk/qcom/videocc-sdm845.c
@@ -260,7 +260,7 @@ static struct gdsc vcodec0_gdsc = {
 	},
 	.cxcs = (unsigned int []){ 0x890, 0x930 },
 	.cxc_count = 2,
-	.flags = HW_CTRL | POLL_CFG_GDSCR,
+	.flags = HW_CTRL_TRIGGER | POLL_CFG_GDSCR,
 	.pwrsts = PWRSTS_OFF_ON,
 };
 
@@ -271,7 +271,7 @@ static struct gdsc vcodec1_gdsc = {
 	},
 	.cxcs = (unsigned int []){ 0x8d0, 0x950 },
 	.cxc_count = 2,
-	.flags = HW_CTRL | POLL_CFG_GDSCR,
+	.flags = HW_CTRL_TRIGGER | POLL_CFG_GDSCR,
 	.pwrsts = PWRSTS_OFF_ON,
 };
 
diff --git a/drivers/clk/qcom/videocc-sm7150.c b/drivers/clk/qcom/videocc-sm7150.c
index 14ef7f5617537363673662adc3910ddba8ea6a4f..b6912560ef9b7a84e7fd1d9924f5aac6967da780 100644
--- a/drivers/clk/qcom/videocc-sm7150.c
+++ b/drivers/clk/qcom/videocc-sm7150.c
@@ -271,7 +271,7 @@ static struct gdsc vcodec0_gdsc = {
 	},
 	.cxcs = (unsigned int []){ 0x890, 0x9ec },
 	.cxc_count = 2,
-	.flags = HW_CTRL | POLL_CFG_GDSCR,
+	.flags = HW_CTRL_TRIGGER | POLL_CFG_GDSCR,
 	.pwrsts = PWRSTS_OFF_ON,
 };
 
@@ -282,7 +282,7 @@ static struct gdsc vcodec1_gdsc = {
 	},
 	.cxcs = (unsigned int []){ 0x8d0, 0xa0c },
 	.cxc_count = 2,
-	.flags = HW_CTRL | POLL_CFG_GDSCR,
+	.flags = HW_CTRL_TRIGGER | POLL_CFG_GDSCR,
 	.pwrsts = PWRSTS_OFF_ON,
 };
 
diff --git a/drivers/clk/qcom/videocc-sm8150.c b/drivers/clk/qcom/videocc-sm8150.c
index daab3237eec19b727d34512d3a2ba1d7bd2743d6..3024f6fc89c8b374f2ef13debc283998cb136f6b 100644
--- a/drivers/clk/qcom/videocc-sm8150.c
+++ b/drivers/clk/qcom/videocc-sm8150.c
@@ -179,7 +179,7 @@ static struct gdsc vcodec0_gdsc = {
 	.pd = {
 		.name = "vcodec0_gdsc",
 	},
-	.flags = HW_CTRL,
+	.flags = HW_CTRL_TRIGGER,
 	.pwrsts = PWRSTS_OFF_ON,
 };
 
@@ -188,7 +188,7 @@ static struct gdsc vcodec1_gdsc = {
 	.pd = {
 		.name = "vcodec1_gdsc",
 	},
-	.flags = HW_CTRL,
+	.flags = HW_CTRL_TRIGGER,
 	.pwrsts = PWRSTS_OFF_ON,
 };
 static struct clk_regmap *video_cc_sm8150_clocks[] = {
diff --git a/drivers/clk/qcom/videocc-sm8450.c b/drivers/clk/qcom/videocc-sm8450.c
index f26c7eccb62e7eb8dbd022e2f01fa496eb570b3f..4cefcbbc020f201f19c75c20229415e0bdea2963 100644
--- a/drivers/clk/qcom/videocc-sm8450.c
+++ b/drivers/clk/qcom/videocc-sm8450.c
@@ -347,7 +347,7 @@ static struct gdsc video_cc_mvs0_gdsc = {
 	},
 	.pwrsts = PWRSTS_OFF_ON,
 	.parent = &video_cc_mvs0c_gdsc.pd,
-	.flags = RETAIN_FF_ENABLE | HW_CTRL,
+	.flags = HW_CTRL_TRIGGER | RETAIN_FF_ENABLE,
 };
 
 static struct gdsc video_cc_mvs1c_gdsc = {
@@ -372,7 +372,7 @@ static struct gdsc video_cc_mvs1_gdsc = {
 	},
 	.pwrsts = PWRSTS_OFF_ON,
 	.parent = &video_cc_mvs1c_gdsc.pd,
-	.flags = RETAIN_FF_ENABLE | HW_CTRL,
+	.flags = HW_CTRL_TRIGGER | RETAIN_FF_ENABLE,
 };
 
 static struct clk_regmap *video_cc_sm8450_clocks[] = {

-- 
2.34.1

[PATCH v3 2/3] venus: pm_helpers: Use dev_pm_genpd_set_hwmode to switch GDSC mode on V4

[Renjiang Han]

The POWER_CONTROL register addresses are not constant and can vary across
the variants. Also as per the HW recommendation, the GDSC mode switching
needs to be controlled from respective GDSC register and this is a uniform
approach across all the targets. Hence use dev_pm_genpd_set_hwmode() API
which controls GDSC mode switching using its respective GDSC register.

In venus v4 variants, the vcodec gdsc gets enabled in SW mode by default
with new HW_CTRL_TRIGGER flag and there is no need to switch it to SW mode
again after enable, hence add check to avoid switching gdsc to SW mode
again after gdsc enable. Similarly add check to avoid switching GDSC to HW
mode before disabling the GDSC, so GDSC gets enabled in SW mode in the
next enable.

Signed-off-by: Renjiang Han <quic_renjiang@quicinc.com>
---
 drivers/media/platform/qcom/venus/pm_helpers.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 33a5a659c0ada0ca97eebb5522c5f349f95c49c7..a2062b366d4aedba3eb5e4be456a005847eaec0b 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -412,7 +412,7 @@ static int vcodec_control_v4(struct venus_core *core, u32 coreid, bool enable)
 	u32 val;
 	int ret;
 
-	if (IS_V6(core))
+	if (IS_V6(core) || IS_V4(core))
 		return dev_pm_genpd_set_hwmode(core->pmdomains->pd_devs[coreid], !enable);
 	else if (coreid == VIDC_CORE_ID_1) {
 		ctrl = core->wrapper_base + WRAPPER_VCODEC0_MMCC_POWER_CONTROL;
@@ -450,7 +450,7 @@ static int poweroff_coreid(struct venus_core *core, unsigned int coreid_mask)
 
 		vcodec_clks_disable(core, core->vcodec0_clks);
 
-		if (!IS_V6(core)) {
+		if (!IS_V6(core) && !IS_V4(core)) {
 			ret = vcodec_control_v4(core, VIDC_CORE_ID_1, false);
 			if (ret)
 				return ret;
@@ -468,7 +468,7 @@ static int poweroff_coreid(struct venus_core *core, unsigned int coreid_mask)
 
 		vcodec_clks_disable(core, core->vcodec1_clks);
 
-		if (!IS_V6(core)) {
+		if (!IS_V6(core) && !IS_V4(core)) {
 			ret = vcodec_control_v4(core, VIDC_CORE_ID_2, false);
 			if (ret)
 				return ret;
@@ -491,7 +491,7 @@ static int poweron_coreid(struct venus_core *core, unsigned int coreid_mask)
 		if (ret < 0)
 			return ret;
 
-		if (!IS_V6(core)) {
+		if (!IS_V6(core) && !IS_V4(core)) {
 			ret = vcodec_control_v4(core, VIDC_CORE_ID_1, true);
 			if (ret)
 				return ret;
@@ -511,7 +511,7 @@ static int poweron_coreid(struct venus_core *core, unsigned int coreid_mask)
 		if (ret < 0)
 			return ret;
 
-		if (!IS_V6(core)) {
+		if (!IS_V6(core) && !IS_V4(core)) {
 			ret = vcodec_control_v4(core, VIDC_CORE_ID_2, true);
 			if (ret)
 				return ret;

-- 
2.34.1

[PATCH v3 3/3] venus: pm_helpers: Remove dead code and simplify power management

[Renjiang Han]

The functions poweron_coreid(), poweroff_coreid() and vcodec_control_v4()
are only used for v4 and v6. Remove the dead code by:
- Removing vcodec_control_v4() function
- Removing the check for !IS_V6(core) && !IS_V4(core) in poweron_coreid()
and poweroff_coreid()

Directly call dev_pm_genpd_set_hwmode() without vcodec_control_v4().

Signed-off-by: Renjiang Han <quic_renjiang@quicinc.com>
---
 drivers/media/platform/qcom/venus/pm_helpers.c | 73 +++-----------------------
 1 file changed, 8 insertions(+), 65 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index a2062b366d4aedba3eb5e4be456a005847eaec0b..846aa765edbb33df0b0c39bb463dd68c16ce1b68 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -406,74 +406,29 @@ static const struct venus_pm_ops pm_ops_v3 = {
 	.load_scale = load_scale_v1,
 };
 
-static int vcodec_control_v4(struct venus_core *core, u32 coreid, bool enable)
-{
-	void __iomem *ctrl, *stat;
-	u32 val;
-	int ret;
-
-	if (IS_V6(core) || IS_V4(core))
-		return dev_pm_genpd_set_hwmode(core->pmdomains->pd_devs[coreid], !enable);
-	else if (coreid == VIDC_CORE_ID_1) {
-		ctrl = core->wrapper_base + WRAPPER_VCODEC0_MMCC_POWER_CONTROL;
-		stat = core->wrapper_base + WRAPPER_VCODEC0_MMCC_POWER_STATUS;
-	} else {
-		ctrl = core->wrapper_base + WRAPPER_VCODEC1_MMCC_POWER_CONTROL;
-		stat = core->wrapper_base + WRAPPER_VCODEC1_MMCC_POWER_STATUS;
-	}
-
-	if (enable) {
-		writel(0, ctrl);
-
-		ret = readl_poll_timeout(stat, val, val & BIT(1), 1, 100);
-		if (ret)
-			return ret;
-	} else {
-		writel(1, ctrl);
-
-		ret = readl_poll_timeout(stat, val, !(val & BIT(1)), 1, 100);
-		if (ret)
-			return ret;
-	}
-
-	return 0;
-}
-
 static int poweroff_coreid(struct venus_core *core, unsigned int coreid_mask)
 {
 	int ret;
 
 	if (coreid_mask & VIDC_CORE_ID_1) {
-		ret = vcodec_control_v4(core, VIDC_CORE_ID_1, true);
+		ret = dev_pm_genpd_set_hwmode(core->pmdomains->pd_devs[VIDC_CORE_ID_1], false);
 		if (ret)
 			return ret;
 
 		vcodec_clks_disable(core, core->vcodec0_clks);
 
-		if (!IS_V6(core) && !IS_V4(core)) {
-			ret = vcodec_control_v4(core, VIDC_CORE_ID_1, false);
-			if (ret)
-				return ret;
-		}
-
 		ret = pm_runtime_put_sync(core->pmdomains->pd_devs[1]);
 		if (ret < 0)
 			return ret;
 	}
 
 	if (coreid_mask & VIDC_CORE_ID_2) {
-		ret = vcodec_control_v4(core, VIDC_CORE_ID_2, true);
+		ret = dev_pm_genpd_set_hwmode(core->pmdomains->pd_devs[VIDC_CORE_ID_2], false);
 		if (ret)
 			return ret;
 
 		vcodec_clks_disable(core, core->vcodec1_clks);
 
-		if (!IS_V6(core) && !IS_V4(core)) {
-			ret = vcodec_control_v4(core, VIDC_CORE_ID_2, false);
-			if (ret)
-				return ret;
-		}
-
 		ret = pm_runtime_put_sync(core->pmdomains->pd_devs[2]);
 		if (ret < 0)
 			return ret;
@@ -491,17 +446,11 @@ static int poweron_coreid(struct venus_core *core, unsigned int coreid_mask)
 		if (ret < 0)
 			return ret;
 
-		if (!IS_V6(core) && !IS_V4(core)) {
-			ret = vcodec_control_v4(core, VIDC_CORE_ID_1, true);
-			if (ret)
-				return ret;
-		}
-
 		ret = vcodec_clks_enable(core, core->vcodec0_clks);
 		if (ret)
 			return ret;
 
-		ret = vcodec_control_v4(core, VIDC_CORE_ID_1, false);
+		ret = dev_pm_genpd_set_hwmode(core->pmdomains->pd_devs[VIDC_CORE_ID_1], true);
 		if (ret < 0)
 			return ret;
 	}
@@ -511,17 +460,11 @@ static int poweron_coreid(struct venus_core *core, unsigned int coreid_mask)
 		if (ret < 0)
 			return ret;
 
-		if (!IS_V6(core) && !IS_V4(core)) {
-			ret = vcodec_control_v4(core, VIDC_CORE_ID_2, true);
-			if (ret)
-				return ret;
-		}
-
 		ret = vcodec_clks_enable(core, core->vcodec1_clks);
 		if (ret)
 			return ret;
 
-		ret = vcodec_control_v4(core, VIDC_CORE_ID_2, false);
+		ret = dev_pm_genpd_set_hwmode(core->pmdomains->pd_devs[VIDC_CORE_ID_2], true);
 		if (ret < 0)
 			return ret;
 	}
@@ -802,7 +745,7 @@ static int vdec_power_v4(struct device *dev, int on)
 	if (!legacy_binding)
 		return 0;
 
-	ret = vcodec_control_v4(core, VIDC_CORE_ID_1, true);
+	ret = dev_pm_genpd_set_hwmode(core->pmdomains->pd_devs[VIDC_CORE_ID_1], false);
 	if (ret)
 		return ret;
 
@@ -811,7 +754,7 @@ static int vdec_power_v4(struct device *dev, int on)
 	else
 		vcodec_clks_disable(core, core->vcodec0_clks);
 
-	vcodec_control_v4(core, VIDC_CORE_ID_1, false);
+	dev_pm_genpd_set_hwmode(core->pmdomains->pd_devs[VIDC_CORE_ID_1], true);
 
 	return ret;
 }
@@ -847,7 +790,7 @@ static int venc_power_v4(struct device *dev, int on)
 	if (!legacy_binding)
 		return 0;
 
-	ret = vcodec_control_v4(core, VIDC_CORE_ID_2, true);
+	ret = dev_pm_genpd_set_hwmode(core->pmdomains->pd_devs[VIDC_CORE_ID_2], false);
 	if (ret)
 		return ret;
 
@@ -856,7 +799,7 @@ static int venc_power_v4(struct device *dev, int on)
 	else
 		vcodec_clks_disable(core, core->vcodec1_clks);
 
-	vcodec_control_v4(core, VIDC_CORE_ID_2, false);
+	dev_pm_genpd_set_hwmode(core->pmdomains->pd_devs[VIDC_CORE_ID_2], true);
 
 	return ret;
 }

-- 
2.34.1

Re: [PATCH v3 0/3] Use APIs in gdsc genpd to switch gdsc mode for venus v4 core

[Dmitry Baryshkov]

On Wed, Jan 15, 2025 at 03:00:51PM +0530, Renjiang Han wrote:
> The Venus driver requires vcodec GDSC to be ON in SW mode for clock
> operations and move it back to HW mode to gain power benefits. Earlier,
> as there is no interface to switch the GDSC mode from GenPD framework,
> the GDSC is moved to HW control mode as part of GDSC enable callback and
> venus driver is writing to its POWER_CONTROL register to keep the GDSC ON
> from SW whereever required. But the POWER_CONTROL register addresses are
> not constant and can vary across the variants.
> 
> Also as per the HW recommendation, the GDSC mode switching needs to be
> controlled from respective GDSC register and this is a uniform approach
> across all the targets. Hence use dev_pm_genpd_set_hwmode() API which
> controls GDSC mode switching using its respective GDSC register.
> 
> If only the clock patch or the venus driver patch is merged, the venus
> driver will not work properly. Although it does not affect other system
> functions, it is still recommended to wait until both the clock patch
> and the venus driver patch are reviewed and passed, and then merge them
> into the same release by their respective maintainers.

NO! This will not work, as both -media and arm-soc branches will be
broken. Please read about the git-bisect before making such suggestions.

A proper plan would be:
- implement a function which allows one to check that hwmode is
  supported by the genpd driver
- Change Venus to use hwmode for those platforms if it is enabled
- Enable HWMODE support in the clock driver. Clearly identify that this
  patch should be merged together and after Venus changes if all
  maintainers agree with that
- Clean up now-dead code.

Doing it in any other way would result in the broken kernels.

> 
> Validated this series on QCS615 and SC7180.
> 
> Signed-off-by: Renjiang Han <quic_renjiang@quicinc.com>
> ---
> Changes in v3:
> - 1. Update commit message.
> - 2. Add a patch to clean up the dead code for the venus driver.
> - 3. Remove vcodec_control_v4() function.
> - 4. Directly call dev_pm_genpd_set_hwmode() without vcodec_control_v4().
> - Link to v2: https://lore.kernel.org/r/20241223-switch_gdsc_mode-v2-0-eb5c96aee662@quicinc.com
> 
> Changes in v2:
> - 1. Add the HW_CTRL_TRIGGER flag for the targets SM7150/SM8150 and SM8450
> video GDSCs supporting movement between HW and SW mode of the GDSC.
> (Suggested by Dmitry Baryshkov)
> - 2. There is a dependency of the clock driver introducing the new flag
> and the video driver adapting to this new API. Missing either the clock
> and video driver could potentially break the video driver.
> - Link to v1: https://lore.kernel.org/r/20241122-switch_gdsc_mode-v1-0-365f097ecbb0@quicinc.com
> 
> ---
> Renjiang Han (2):
>       venus: pm_helpers: Use dev_pm_genpd_set_hwmode to switch GDSC mode on V4
>       venus: pm_helpers: Remove dead code and simplify power management
> 
> Taniya Das (1):
>       clk: qcom: videocc: Use HW_CTRL_TRIGGER flag for video GDSC's
> 
>  drivers/clk/qcom/videocc-sc7180.c              |  2 +-
>  drivers/clk/qcom/videocc-sdm845.c              |  4 +-
>  drivers/clk/qcom/videocc-sm7150.c              |  4 +-
>  drivers/clk/qcom/videocc-sm8150.c              |  4 +-
>  drivers/clk/qcom/videocc-sm8450.c              |  4 +-
>  drivers/media/platform/qcom/venus/pm_helpers.c | 73 +++-----------------------
>  6 files changed, 17 insertions(+), 74 deletions(-)
> ---
> base-commit: 63b3ff03d91ae8f875fe8747c781a521f78cde17
> change-id: 20250115-switch_gdsc_mode-a9c14fad9a36
> 
> Best regards,
> -- 
> Renjiang Han <quic_renjiang@quicinc.com>
> 

-- 
With best wishes
Dmitry

Re: [PATCH v3 3/3] venus: pm_helpers: Remove dead code and simplify power management

[Dmitry Baryshkov]

On Wed, Jan 15, 2025 at 03:00:54PM +0530, Renjiang Han wrote:
> The functions poweron_coreid(), poweroff_coreid() and vcodec_control_v4()
> are only used for v4 and v6. Remove the dead code by:
> - Removing vcodec_control_v4() function
> - Removing the check for !IS_V6(core) && !IS_V4(core) in poweron_coreid()
> and poweroff_coreid()
> 
> Directly call dev_pm_genpd_set_hwmode() without vcodec_control_v4().
> 
> Signed-off-by: Renjiang Han <quic_renjiang@quicinc.com>
> ---
>  drivers/media/platform/qcom/venus/pm_helpers.c | 73 +++-----------------------
>  1 file changed, 8 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index a2062b366d4aedba3eb5e4be456a005847eaec0b..846aa765edbb33df0b0c39bb463dd68c16ce1b68 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -406,74 +406,29 @@ static const struct venus_pm_ops pm_ops_v3 = {
>  	.load_scale = load_scale_v1,
>  };
>  
> -static int vcodec_control_v4(struct venus_core *core, u32 coreid, bool enable)
> -{
> -	void __iomem *ctrl, *stat;
> -	u32 val;
> -	int ret;
> -
> -	if (IS_V6(core) || IS_V4(core))
> -		return dev_pm_genpd_set_hwmode(core->pmdomains->pd_devs[coreid], !enable);
> -	else if (coreid == VIDC_CORE_ID_1) {
> -		ctrl = core->wrapper_base + WRAPPER_VCODEC0_MMCC_POWER_CONTROL;
> -		stat = core->wrapper_base + WRAPPER_VCODEC0_MMCC_POWER_STATUS;
> -	} else {
> -		ctrl = core->wrapper_base + WRAPPER_VCODEC1_MMCC_POWER_CONTROL;
> -		stat = core->wrapper_base + WRAPPER_VCODEC1_MMCC_POWER_STATUS;
> -	}
> -
> -	if (enable) {
> -		writel(0, ctrl);
> -
> -		ret = readl_poll_timeout(stat, val, val & BIT(1), 1, 100);
> -		if (ret)
> -			return ret;
> -	} else {
> -		writel(1, ctrl);
> -
> -		ret = readl_poll_timeout(stat, val, !(val & BIT(1)), 1, 100);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	return 0;
> -}
> -
>  static int poweroff_coreid(struct venus_core *core, unsigned int coreid_mask)
>  {
>  	int ret;
>  
>  	if (coreid_mask & VIDC_CORE_ID_1) {
> -		ret = vcodec_control_v4(core, VIDC_CORE_ID_1, true);
> +		ret = dev_pm_genpd_set_hwmode(core->pmdomains->pd_devs[VIDC_CORE_ID_1], false);
>  		if (ret)
>  			return ret;
>  
>  		vcodec_clks_disable(core, core->vcodec0_clks);
>  
> -		if (!IS_V6(core) && !IS_V4(core)) {
> -			ret = vcodec_control_v4(core, VIDC_CORE_ID_1, false);
> -			if (ret)
> -				return ret;
> -		}
> -
>  		ret = pm_runtime_put_sync(core->pmdomains->pd_devs[1]);
>  		if (ret < 0)
>  			return ret;
>  	}
>  
>  	if (coreid_mask & VIDC_CORE_ID_2) {
> -		ret = vcodec_control_v4(core, VIDC_CORE_ID_2, true);
> +		ret = dev_pm_genpd_set_hwmode(core->pmdomains->pd_devs[VIDC_CORE_ID_2], false);
>  		if (ret)
>  			return ret;
>  
>  		vcodec_clks_disable(core, core->vcodec1_clks);
>  
> -		if (!IS_V6(core) && !IS_V4(core)) {
> -			ret = vcodec_control_v4(core, VIDC_CORE_ID_2, false);
> -			if (ret)
> -				return ret;
> -		}
> -
>  		ret = pm_runtime_put_sync(core->pmdomains->pd_devs[2]);
>  		if (ret < 0)
>  			return ret;
> @@ -491,17 +446,11 @@ static int poweron_coreid(struct venus_core *core, unsigned int coreid_mask)
>  		if (ret < 0)
>  			return ret;
>  
> -		if (!IS_V6(core) && !IS_V4(core)) {
> -			ret = vcodec_control_v4(core, VIDC_CORE_ID_1, true);
> -			if (ret)
> -				return ret;
> -		}
> -
>  		ret = vcodec_clks_enable(core, core->vcodec0_clks);
>  		if (ret)
>  			return ret;
>  
> -		ret = vcodec_control_v4(core, VIDC_CORE_ID_1, false);
> +		ret = dev_pm_genpd_set_hwmode(core->pmdomains->pd_devs[VIDC_CORE_ID_1], true);
>  		if (ret < 0)
>  			return ret;
>  	}
> @@ -511,17 +460,11 @@ static int poweron_coreid(struct venus_core *core, unsigned int coreid_mask)
>  		if (ret < 0)
>  			return ret;
>  
> -		if (!IS_V6(core) && !IS_V4(core)) {
> -			ret = vcodec_control_v4(core, VIDC_CORE_ID_2, true);
> -			if (ret)
> -				return ret;
> -		}
> -
>  		ret = vcodec_clks_enable(core, core->vcodec1_clks);
>  		if (ret)
>  			return ret;
>  
> -		ret = vcodec_control_v4(core, VIDC_CORE_ID_2, false);
> +		ret = dev_pm_genpd_set_hwmode(core->pmdomains->pd_devs[VIDC_CORE_ID_2], true);
>  		if (ret < 0)
>  			return ret;
>  	}
> @@ -802,7 +745,7 @@ static int vdec_power_v4(struct device *dev, int on)
>  	if (!legacy_binding)
>  		return 0;
>  
> -	ret = vcodec_control_v4(core, VIDC_CORE_ID_1, true);
> +	ret = dev_pm_genpd_set_hwmode(core->pmdomains->pd_devs[VIDC_CORE_ID_1], false);
>  	if (ret)
>  		return ret;
>  
> @@ -811,7 +754,7 @@ static int vdec_power_v4(struct device *dev, int on)
>  	else
>  		vcodec_clks_disable(core, core->vcodec0_clks);
>  
> -	vcodec_control_v4(core, VIDC_CORE_ID_1, false);
> +	dev_pm_genpd_set_hwmode(core->pmdomains->pd_devs[VIDC_CORE_ID_1], true);

Check for the error here.

>  
>  	return ret;
>  }
> @@ -847,7 +790,7 @@ static int venc_power_v4(struct device *dev, int on)
>  	if (!legacy_binding)
>  		return 0;
>  
> -	ret = vcodec_control_v4(core, VIDC_CORE_ID_2, true);
> +	ret = dev_pm_genpd_set_hwmode(core->pmdomains->pd_devs[VIDC_CORE_ID_2], false);
>  	if (ret)
>  		return ret;
>  
> @@ -856,7 +799,7 @@ static int venc_power_v4(struct device *dev, int on)
>  	else
>  		vcodec_clks_disable(core, core->vcodec1_clks);
>  
> -	vcodec_control_v4(core, VIDC_CORE_ID_2, false);
> +	dev_pm_genpd_set_hwmode(core->pmdomains->pd_devs[VIDC_CORE_ID_2], true);


Check for the error here.

>  
>  	return ret;
>  }
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v3 2/3] venus: pm_helpers: Use dev_pm_genpd_set_hwmode to switch GDSC mode on V4

[Bryan O'Donoghue]

On 15/01/2025 09:30, Renjiang Han wrote:
> The POWER_CONTROL register addresses are not constant and can vary across
> the variants. Also as per the HW recommendation, the GDSC mode switching
> needs to be controlled from respective GDSC register and this is a uniform
> approach across all the targets. Hence use dev_pm_genpd_set_hwmode() API
> which controls GDSC mode switching using its respective GDSC register.

This paragraph is difficult to read and understand. Try something a bit 
more like "Use dev_pm_genpd_set_hwmode() API to control GDSC mode 
switching. Do that because of reason x, y z"

"We are making this change because of reason a, b, c"

Basically just try to state the facts as clearly and concisely as possible.

---
bod

Re: [PATCH v3 2/3] venus: pm_helpers: Use dev_pm_genpd_set_hwmode to switch GDSC mode on V4

[Bjorn Andersson]

On Wed, Jan 15, 2025 at 12:02:31PM +0000, Bryan O'Donoghue wrote:
> On 15/01/2025 09:30, Renjiang Han wrote:
> > The POWER_CONTROL register addresses are not constant and can vary across
> > the variants. Also as per the HW recommendation, the GDSC mode switching
> > needs to be controlled from respective GDSC register and this is a uniform
> > approach across all the targets. Hence use dev_pm_genpd_set_hwmode() API
> > which controls GDSC mode switching using its respective GDSC register.
> 
> This paragraph is difficult to read and understand. Try something a bit more
> like "Use dev_pm_genpd_set_hwmode() API to control GDSC mode switching. Do
> that because of reason x, y z"

No, follow https://docs.kernel.org/process/submitting-patches.html#describe-your-changes

It says "describe your problem, then describe your solution". "Use "
isn't a good start of any problem description that I can think of.

Regards,
Bjorn

> 
> "We are making this change because of reason a, b, c"
> 
> Basically just try to state the facts as clearly and concisely as possible.
> 
> ---
> bod

Re: [PATCH v3 2/3] venus: pm_helpers: Use dev_pm_genpd_set_hwmode to switch GDSC mode on V4

[Bjorn Andersson]

On Wed, Jan 15, 2025 at 02:22:11PM -0600, Bjorn Andersson wrote:
> On Wed, Jan 15, 2025 at 12:02:31PM +0000, Bryan O'Donoghue wrote:
> > On 15/01/2025 09:30, Renjiang Han wrote:
> > > The POWER_CONTROL register addresses are not constant and can vary across
> > > the variants. Also as per the HW recommendation, the GDSC mode switching
> > > needs to be controlled from respective GDSC register and this is a uniform
> > > approach across all the targets. Hence use dev_pm_genpd_set_hwmode() API
> > > which controls GDSC mode switching using its respective GDSC register.
> > 
> > This paragraph is difficult to read and understand. Try something a bit more
> > like "Use dev_pm_genpd_set_hwmode() API to control GDSC mode switching. Do
> > that because of reason x, y z"
> 
> No, follow https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
> 
> It says "describe your problem, then describe your solution". "Use "
> isn't a good start of any problem description that I can think of.
> 

But just to clarify, I have no concern with your request for
improvements to the description of the problem being addressed. 

Regards,
Bjorn

Re: [PATCH v3 3/3] venus: pm_helpers: Remove dead code and simplify power management

[Renjiang Han]

On 1/15/2025 6:01 PM, Dmitry Baryshkov wrote:
>>   
>> -	vcodec_control_v4(core, VIDC_CORE_ID_2, false);
>> +	dev_pm_genpd_set_hwmode(core->pmdomains->pd_devs[VIDC_CORE_ID_2], true);
> Check for the error here.
Sure, thanks for pointing it out. I'll update it!
>
>>   
>>   	return ret;

-- 
Best Regards,
Renjiang

Re: [PATCH v3 0/3] Use APIs in gdsc genpd to switch gdsc mode for venus v4 core

[Renjiang Han]

On 1/15/2025 5:59 PM, Dmitry Baryshkov wrote:
>> If only the clock patch or the venus driver patch is merged, the venus
>> driver will not work properly. Although it does not affect other system
>> functions, it is still recommended to wait until both the clock patch
>> and the venus driver patch are reviewed and passed, and then merge them
>> into the same release by their respective maintainers.
> NO! This will not work, as both -media and arm-soc branches will be
> broken. Please read about the git-bisect before making such suggestions.
>
> A proper plan would be:
> - implement a function which allows one to check that hwmode is
>    supported by the genpd driver
> - Change Venus to use hwmode for those platforms if it is enabled
> - Enable HWMODE support in the clock driver. Clearly identify that this
>    patch should be merged together and after Venus changes if all
>    maintainers agree with that
> - Clean up now-dead code.
>
> Doing it in any other way would result in the broken kernels.
  Thanks for your comment. dev_pm_genpd_set_hwmode() has a check function.
  So I think I can do it like this to add compatibility in venus driver:
  - 1. the dev_pm_genpd_set_hwmode() is used to switch the GDSC mode by
  default.
  - 2. If it fails and dev_pm_genpd_set_hwmode() returns -EOPNOTSUPP, it
  means that the clock driver uses the HW_CTRL flag. At this time, the
  GDSC mode is switched by writing the POWER_CONTROL register.

  In addition, the merged code needs to follow this order.
  - 1. Add compatibility in venus driver.
  - 2. Use the HW_CTRL_TRIGGER flag in clock driver
  - 3. Remove the dead code in venus driver.

-- 
Best Regards,
Renjiang

Re: [PATCH v3 2/3] venus: pm_helpers: Use dev_pm_genpd_set_hwmode to switch GDSC mode on V4

[Renjiang Han]

On 1/16/2025 4:36 AM, Bjorn Andersson wrote:
> On Wed, Jan 15, 2025 at 02:22:11PM -0600, Bjorn Andersson wrote:
>> On Wed, Jan 15, 2025 at 12:02:31PM +0000, Bryan O'Donoghue wrote:
>>> On 15/01/2025 09:30, Renjiang Han wrote:
>>>> The POWER_CONTROL register addresses are not constant and can vary across
>>>> the variants. Also as per the HW recommendation, the GDSC mode switching
>>>> needs to be controlled from respective GDSC register and this is a uniform
>>>> approach across all the targets. Hence use dev_pm_genpd_set_hwmode() API
>>>> which controls GDSC mode switching using its respective GDSC register.
>>> This paragraph is difficult to read and understand. Try something a bit more
>>> like "Use dev_pm_genpd_set_hwmode() API to control GDSC mode switching. Do
>>> that because of reason x, y z"
>> No, follow https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
>>
>> It says "describe your problem, then describe your solution". "Use "
>> isn't a good start of any problem description that I can think of.
>>
> But just to clarify, I have no concern with your request for
> improvements to the description of the problem being addressed.
>
> Regards,
> Bjorn
Thanks Bjorn and Bryan. I'll update this message.

-- 
Best Regards,
Renjiang

Re: [PATCH v3 0/3] Use APIs in gdsc genpd to switch gdsc mode for venus v4 core

[Renjiang Han]

On 1/17/2025 12:17 PM, Renjiang Han wrote:
>
> On 1/15/2025 5:59 PM, Dmitry Baryshkov wrote:
>>> If only the clock patch or the venus driver patch is merged, the venus
>>> driver will not work properly. Although it does not affect other system
>>> functions, it is still recommended to wait until both the clock patch
>>> and the venus driver patch are reviewed and passed, and then merge them
>>> into the same release by their respective maintainers.
>> NO! This will not work, as both -media and arm-soc branches will be
>> broken. Please read about the git-bisect before making such suggestions.
>>
>> A proper plan would be:
>> - implement a function which allows one to check that hwmode is
>>    supported by the genpd driver
>> - Change Venus to use hwmode for those platforms if it is enabled
>> - Enable HWMODE support in the clock driver. Clearly identify that this
>>    patch should be merged together and after Venus changes if all
>>    maintainers agree with that
>> - Clean up now-dead code.
>>
>> Doing it in any other way would result in the broken kernels.
>  Thanks for your comment. dev_pm_genpd_set_hwmode() has a check function.
>  So I think I can do it like this to add compatibility in venus driver:
>  - 1. the dev_pm_genpd_set_hwmode() is used to switch the GDSC mode by
>  default.
>  - 2. If it fails and dev_pm_genpd_set_hwmode() returns -EOPNOTSUPP, it
>  means that the clock driver uses the HW_CTRL flag. At this time, the
>  GDSC mode is switched by writing the POWER_CONTROL register.
>
>  In addition, the merged code needs to follow this order.
>  - 1. Add compatibility in venus driver.
>  - 2. Use the HW_CTRL_TRIGGER flag in clock driver
>  - 3. Remove the dead code in venus driver.
>
  As internal discussion, in order to merge the patches in the correct order, we
  plan to submit two patch series.
  One patch series uses dev_pm_genpd_set_hwmode() to check if hwmode is supported,
  and the venus driver is compatible with the two methods of switching GDSC mode.
  Then the clock driver enables hwmode.
  The other patch series will clean up the dead code.
  Do you agree to submit two patch series or one?

-- 
Best Regards,
Renjiang