With the driver supporting multiple generations of hardware, some mold
has definitely grown over the code..
This series attempts to amend this situation a bit by commonizing some
code paths and fixing some bugs while at it.
Only tested on SM8250.
Definitely needs testing on:
- SDM845 with old bindings
- SDM845 with new bindings or 7180
- MSM8916
- MSM8996
Signed-off-by: Konrad Dybcio <[email protected]>
---
Changes in v3:
- Drop const within const patch
- Pick up tags
- Some stylistic fixes in kerneldoc
- Link to v2: https://lore.kernel.org/r/[email protected]
Changes in v2:
- Fix "set but unused" warning in "Drop cache properties in resource struct"
- Fix modular build with "Commonize vdec_get()"
- Rebase
- Test again on 8250, since nobody else tested other platforms since the last
submission (or at least hasn't reported that), I'm assuming nobody cares
- Needs to be tested atop [1] and similar, it's in latest -next already
- Link to v1: https://lore.kernel.org/r/[email protected]
[1] https://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git/commit/?h=for-next&id=d2cd22c9c384aa50c0b4530e842bd078427e6279
---
Konrad Dybcio (19):
media: venus: pm_helpers: Only set rate of the core clock in core_clks_enable
media: venus: pm_helpers: Rename core_clks_get to venus_clks_get
media: venus: pm_helpers: Add kerneldoc to venus_clks_get()
media: venus: core: Set OPP clkname in a common code path
media: venus: pm_helpers: Kill dead code
media: venus: pm_helpers: Move reset acquisition to common code
media: venus: core: Deduplicate OPP genpd names
media: venus: core: Get rid of vcodec_num
media: venus: core: Drop cache properties in resource struct
media: venus: core: Use GENMASK for dma_mask
media: venus: core: Remove cp_start
media: venus: pm_helpers: Commonize core_power
media: venus: pm_helpers: Remove pm_ops->core_put
media: venus: core: Define a pointer to core->res
media: venus: pm_helpers: Simplify vcodec clock handling
media: venus: pm_helpers: Commonize getting clocks and GenPDs
media: venus: pm_helpers: Commonize vdec_get()
media: venus: pm_helpers: Commonize venc_get()
media: venus: pm_helpers: Use reset_bulk API
drivers/media/platform/qcom/venus/core.c | 139 ++++-------
drivers/media/platform/qcom/venus/core.h | 22 +-
drivers/media/platform/qcom/venus/firmware.c | 3 +-
drivers/media/platform/qcom/venus/hfi_venus.c | 10 +-
drivers/media/platform/qcom/venus/pm_helpers.c | 323 +++++++++----------------
drivers/media/platform/qcom/venus/pm_helpers.h | 10 +-
drivers/media/platform/qcom/venus/vdec.c | 9 +-
drivers/media/platform/qcom/venus/venc.c | 9 +-
8 files changed, 191 insertions(+), 334 deletions(-)
---
base-commit: 26074e1be23143b2388cacb36166766c235feb7c
change-id: 20230911-topic-mars-e60bb2269411
Best regards,
--
Konrad Dybcio <[email protected]>
Currently the infrastructure is set up for vast expandability, but
it's far too complex for what is just 0-2 clocks. Categorize the
clocks and simplify their getting.
One notable change is that vcodec clocks are switched to use
devm_clk_get_optional, which will let us commonize the code further
while leaving the burden of figuring out which SoCs need codec-specific
clocks and which don't to the bindings checker.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/media/platform/qcom/venus/core.c | 18 ----
drivers/media/platform/qcom/venus/core.h | 13 ++-
drivers/media/platform/qcom/venus/pm_helpers.c | 129 +++++++++++++------------
3 files changed, 71 insertions(+), 89 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index e61aa863b7f7..1f4a86b1bd73 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -581,9 +581,6 @@ static const struct venus_resources msm8996_res = {
.reg_tbl_size = ARRAY_SIZE(msm8996_reg_preset),
.clks = {"core", "iface", "bus", "mbus" },
.clks_num = 4,
- .vcodec0_clks = { "core" },
- .vcodec1_clks = { "core" },
- .vcodec_clks_num = 1,
.max_load = 2563200,
.hfi_version = HFI_VERSION_3XX,
.dma_mask = (GENMASK(31, 30) | GENMASK(28, 26) | GENMASK(24, 22)) - 1,
@@ -636,9 +633,6 @@ static const struct venus_resources sdm660_res = {
.bw_tbl_dec_size = ARRAY_SIZE(sdm660_bw_table_dec),
.clks = {"core", "iface", "bus", "bus_throttle" },
.clks_num = 4,
- .vcodec0_clks = { "vcodec0_core" },
- .vcodec1_clks = { "vcodec0_core" },
- .vcodec_clks_num = 1,
.max_load = 1036800,
.hfi_version = HFI_VERSION_3XX,
.cp_size = 0x79000000,
@@ -680,9 +674,6 @@ static const struct venus_resources sdm845_res = {
.bw_tbl_dec_size = ARRAY_SIZE(sdm845_bw_table_dec),
.clks = {"core", "iface", "bus" },
.clks_num = 3,
- .vcodec0_clks = { "core", "bus" },
- .vcodec1_clks = { "core", "bus" },
- .vcodec_clks_num = 2,
.max_load = 3110400, /* 4096x2160@90 */
.hfi_version = HFI_VERSION_4XX,
.vpu_version = VPU_VERSION_AR50,
@@ -699,9 +690,6 @@ static const struct venus_resources sdm845_res_v2 = {
.bw_tbl_dec_size = ARRAY_SIZE(sdm845_bw_table_dec),
.clks = {"core", "iface", "bus" },
.clks_num = 3,
- .vcodec0_clks = { "vcodec0_core", "vcodec0_bus" },
- .vcodec1_clks = { "vcodec1_core", "vcodec1_bus" },
- .vcodec_clks_num = 2,
.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0", "vcodec1" },
.vcodec_pmdomains_num = 3,
.opp_pmdomain = pd_names_cx,
@@ -744,8 +732,6 @@ static const struct venus_resources sc7180_res = {
.bw_tbl_dec_size = ARRAY_SIZE(sc7180_bw_table_dec),
.clks = {"core", "iface", "bus" },
.clks_num = 3,
- .vcodec0_clks = { "vcodec0_core", "vcodec0_bus" },
- .vcodec_clks_num = 2,
.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
.vcodec_pmdomains_num = 2,
.opp_pmdomain = pd_names_cx,
@@ -796,8 +782,6 @@ static const struct venus_resources sm8250_res = {
.clks_num = 2,
.resets = { "bus", "core" },
.resets_num = 2,
- .vcodec0_clks = { "vcodec0_core" },
- .vcodec_clks_num = 1,
.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
.vcodec_pmdomains_num = 2,
.opp_pmdomain = pd_names_mx,
@@ -851,8 +835,6 @@ static const struct venus_resources sc7280_res = {
.ubwc_conf = &sc7280_ubwc_config,
.clks = {"core", "bus", "iface"},
.clks_num = 3,
- .vcodec0_clks = {"vcodec_core", "vcodec_bus"},
- .vcodec_clks_num = 2,
.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
.vcodec_pmdomains_num = 2,
.opp_pmdomain = pd_names_cx,
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 19908f028441..b4c41dc0f8c7 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -24,9 +24,10 @@
#define VDBGFW "VenusFW : "
#define VIDC_CLKS_NUM_MAX 4
-#define VIDC_VCODEC_CLKS_NUM_MAX 2
#define VIDC_RESETS_NUM_MAX 2
+#define MAX_NUM_VCODECS 2
+
extern int venus_fw_debug;
struct freq_tbl {
@@ -68,8 +69,6 @@ struct venus_resources {
const struct hfi_ubwc_config *ubwc_conf;
const char * const clks[VIDC_CLKS_NUM_MAX];
unsigned int clks_num;
- const char * const vcodec0_clks[VIDC_VCODEC_CLKS_NUM_MAX];
- const char * const vcodec1_clks[VIDC_VCODEC_CLKS_NUM_MAX];
unsigned int vcodec_clks_num;
const char **vcodec_pmdomains;
unsigned int vcodec_pmdomains_num;
@@ -123,8 +122,8 @@ struct venus_format {
* @aon_base: AON base address
* @irq: Venus irq
* @clks: an array of struct clk pointers
- * @vcodec0_clks: an array of vcodec0 struct clk pointers
- * @vcodec1_clks: an array of vcodec1 struct clk pointers
+ * @vcodec_core_clks: an array of codec core clk pointers
+ * @vcodec_bus_clks: an array of codec bus clk pointers
* @video_path: an interconnect handle to video to/from memory path
* @cpucfg_path: an interconnect handle to cpu configuration path
* @has_opp_table: does OPP table exist
@@ -176,8 +175,8 @@ struct venus_core {
void __iomem *aon_base;
int irq;
struct clk *clks[VIDC_CLKS_NUM_MAX];
- struct clk *vcodec0_clks[VIDC_VCODEC_CLKS_NUM_MAX];
- struct clk *vcodec1_clks[VIDC_VCODEC_CLKS_NUM_MAX];
+ struct clk *vcodec_core_clks[MAX_NUM_VCODECS];
+ struct clk *vcodec_bus_clks[MAX_NUM_VCODECS];
struct icc_path *video_path;
struct icc_path *cpucfg_path;
bool has_opp_table;
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index d717e150b34f..583153bbb74e 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -110,67 +110,74 @@ static void core_clks_disable(struct venus_core *core)
static int core_clks_set_rate(struct venus_core *core, unsigned long freq)
{
- int ret;
+ int i, ret;
ret = dev_pm_opp_set_rate(core->dev, freq);
if (ret)
return ret;
- ret = clk_set_rate(core->vcodec0_clks[0], freq);
- if (ret)
- return ret;
-
- ret = clk_set_rate(core->vcodec1_clks[0], freq);
- if (ret)
- return ret;
+ for (i = 0; i < MAX_NUM_VCODECS; i++) {
+ ret = clk_set_rate(core->vcodec_core_clks[i], freq);
+ if (ret)
+ return ret;
+ }
return 0;
}
-static int vcodec_clks_get(struct venus_core *core, struct device *dev,
- struct clk **clks, const char * const *id)
+static int vcodec_clks_get(struct venus_core *core, struct device *dev, u8 id)
{
- const struct venus_resources *res = core->res;
- unsigned int i;
+ char buf[13] = { 0 }; /* vcodecX_core\0 */
- for (i = 0; i < res->vcodec_clks_num; i++) {
- if (!id[i])
- continue;
- clks[i] = devm_clk_get(dev, id[i]);
- if (IS_ERR(clks[i]))
- return PTR_ERR(clks[i]);
+ /* Best we can do is 2 cores */
+ if (id > MAX_NUM_VCODECS - 1) {
+ dev_err(dev, "Got impossible vcodec id %u\n", id);
+ return -EINVAL;
+ };
+
+ snprintf(buf, sizeof(buf), "vcodec%u_core", id);
+
+ /* First try the non-legacy name */
+ core->vcodec_core_clks[id] = devm_clk_get_optional(dev, buf);
+ if (IS_ERR(core->vcodec_core_clks[id])) {
+ /* Try again, with the legacy name */
+ core->vcodec_core_clks[id] = devm_clk_get_optional(dev, "core");
+ if (IS_ERR(core->vcodec_core_clks[id]))
+ return PTR_ERR(core->vcodec_core_clks[id]);
+ }
+
+ memset(buf, 0, sizeof(buf));
+ snprintf(buf, sizeof(buf), "vcodec%u_bus", id);
+
+ core->vcodec_bus_clks[id] = devm_clk_get_optional(dev, buf);
+ if (IS_ERR(core->vcodec_bus_clks[id])) {
+ core->vcodec_bus_clks[id] = devm_clk_get_optional(dev, "bus");
+ if (IS_ERR(core->vcodec_bus_clks[id]))
+ return PTR_ERR(core->vcodec_bus_clks[id]);
}
return 0;
}
-static int vcodec_clks_enable(struct venus_core *core, struct clk **clks)
+static int vcodec_clks_enable(struct venus_core *core, u8 id)
{
- const struct venus_resources *res = core->res;
- unsigned int i;
int ret;
- for (i = 0; i < res->vcodec_clks_num; i++) {
- ret = clk_prepare_enable(clks[i]);
- if (ret)
- goto err;
- }
+ ret = clk_prepare_enable(core->vcodec_core_clks[id]);
+ if (ret)
+ return ret;
- return 0;
-err:
- while (i--)
- clk_disable_unprepare(clks[i]);
+ ret = clk_prepare_enable(core->vcodec_bus_clks[id]);
+ if (ret)
+ clk_disable_unprepare(core->vcodec_core_clks[id]);
return ret;
}
-static void vcodec_clks_disable(struct venus_core *core, struct clk **clks)
+static void vcodec_clks_disable(struct venus_core *core, u8 id)
{
- const struct venus_resources *res = core->res;
- unsigned int i = res->vcodec_clks_num;
-
- while (i--)
- clk_disable_unprepare(clks[i]);
+ clk_disable_unprepare(core->vcodec_bus_clks[id]);
+ clk_disable_unprepare(core->vcodec_core_clks[id]);
}
static u32 load_per_instance(struct venus_inst *inst)
@@ -343,8 +350,7 @@ static int vdec_get_v3(struct device *dev)
{
struct venus_core *core = dev_get_drvdata(dev);
- return vcodec_clks_get(core, dev, core->vcodec0_clks,
- core->res->vcodec0_clks);
+ return vcodec_clks_get(core, dev, 0);
}
static int vdec_power_v3(struct device *dev, int on)
@@ -355,9 +361,9 @@ static int vdec_power_v3(struct device *dev, int on)
vcodec_control_v3(core, VIDC_SESSION_TYPE_DEC, true);
if (on == POWER_ON)
- ret = vcodec_clks_enable(core, core->vcodec0_clks);
+ ret = vcodec_clks_enable(core, 0);
else
- vcodec_clks_disable(core, core->vcodec0_clks);
+ vcodec_clks_disable(core, 0);
vcodec_control_v3(core, VIDC_SESSION_TYPE_DEC, false);
@@ -368,8 +374,7 @@ static int venc_get_v3(struct device *dev)
{
struct venus_core *core = dev_get_drvdata(dev);
- return vcodec_clks_get(core, dev, core->vcodec1_clks,
- core->res->vcodec1_clks);
+ return vcodec_clks_get(core, dev, 1);
}
static int venc_power_v3(struct device *dev, int on)
@@ -380,9 +385,9 @@ static int venc_power_v3(struct device *dev, int on)
vcodec_control_v3(core, VIDC_SESSION_TYPE_ENC, true);
if (on == POWER_ON)
- ret = vcodec_clks_enable(core, core->vcodec1_clks);
+ ret = vcodec_clks_enable(core, 1);
else
- vcodec_clks_disable(core, core->vcodec1_clks);
+ vcodec_clks_disable(core, 1);
vcodec_control_v3(core, VIDC_SESSION_TYPE_ENC, false);
@@ -441,7 +446,7 @@ static int poweroff_coreid(struct venus_core *core, unsigned int coreid_mask)
if (ret)
return ret;
- vcodec_clks_disable(core, core->vcodec0_clks);
+ vcodec_clks_disable(core, 0);
ret = vcodec_control_v4(core, VIDC_CORE_ID_1, false);
if (ret)
@@ -457,7 +462,7 @@ static int poweroff_coreid(struct venus_core *core, unsigned int coreid_mask)
if (ret)
return ret;
- vcodec_clks_disable(core, core->vcodec1_clks);
+ vcodec_clks_disable(core, 1);
ret = vcodec_control_v4(core, VIDC_CORE_ID_2, false);
if (ret)
@@ -484,7 +489,7 @@ static int poweron_coreid(struct venus_core *core, unsigned int coreid_mask)
if (ret)
return ret;
- ret = vcodec_clks_enable(core, core->vcodec0_clks);
+ ret = vcodec_clks_enable(core, 0);
if (ret)
return ret;
@@ -502,7 +507,7 @@ static int poweron_coreid(struct venus_core *core, unsigned int coreid_mask)
if (ret)
return ret;
- ret = vcodec_clks_enable(core, core->vcodec1_clks);
+ ret = vcodec_clks_enable(core, 1);
if (ret)
return ret;
@@ -763,20 +768,18 @@ static int vdec_get_v4(struct device *dev)
if (!legacy_binding)
return 0;
- return vcodec_clks_get(core, dev, core->vcodec0_clks,
- core->res->vcodec0_clks);
+ return vcodec_clks_get(core, dev, 0);
}
static void vdec_put_v4(struct device *dev)
{
struct venus_core *core = dev_get_drvdata(dev);
- unsigned int i;
if (!legacy_binding)
return;
- for (i = 0; i < core->res->vcodec_clks_num; i++)
- core->vcodec0_clks[i] = NULL;
+ core->vcodec_core_clks[0] = NULL;
+ core->vcodec_bus_clks[0] = NULL;
}
static int vdec_power_v4(struct device *dev, int on)
@@ -792,9 +795,9 @@ static int vdec_power_v4(struct device *dev, int on)
return ret;
if (on == POWER_ON)
- ret = vcodec_clks_enable(core, core->vcodec0_clks);
+ ret = vcodec_clks_enable(core, 0);
else
- vcodec_clks_disable(core, core->vcodec0_clks);
+ vcodec_clks_disable(core, 0);
vcodec_control_v4(core, VIDC_CORE_ID_1, false);
@@ -808,20 +811,18 @@ static int venc_get_v4(struct device *dev)
if (!legacy_binding)
return 0;
- return vcodec_clks_get(core, dev, core->vcodec1_clks,
- core->res->vcodec1_clks);
+ return vcodec_clks_get(core, dev, 1);
}
static void venc_put_v4(struct device *dev)
{
struct venus_core *core = dev_get_drvdata(dev);
- unsigned int i;
if (!legacy_binding)
return;
- for (i = 0; i < core->res->vcodec_clks_num; i++)
- core->vcodec1_clks[i] = NULL;
+ core->vcodec_core_clks[1] = NULL;
+ core->vcodec_bus_clks[1] = NULL;
}
static int venc_power_v4(struct device *dev, int on)
@@ -837,9 +838,9 @@ static int venc_power_v4(struct device *dev, int on)
return ret;
if (on == POWER_ON)
- ret = vcodec_clks_enable(core, core->vcodec1_clks);
+ ret = vcodec_clks_enable(core, 1);
else
- vcodec_clks_disable(core, core->vcodec1_clks);
+ vcodec_clks_disable(core, 1);
vcodec_control_v4(core, VIDC_CORE_ID_2, false);
@@ -934,11 +935,11 @@ static int core_get_v4(struct venus_core *core)
dev_info(dev, "%s legacy binding\n", legacy_binding ? "" : "non");
- ret = vcodec_clks_get(core, dev, core->vcodec0_clks, res->vcodec0_clks);
+ ret = vcodec_clks_get(core, dev, 0);
if (ret)
return ret;
- ret = vcodec_clks_get(core, dev, core->vcodec1_clks, res->vcodec1_clks);
+ ret = vcodec_clks_get(core, dev, 1);
if (ret)
return ret;
--
2.44.0
There is no reason to keep reset_get code local to HFIv4/v6.
Move it to the common part.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/media/platform/qcom/venus/core.c | 9 ++++++++-
drivers/media/platform/qcom/venus/pm_helpers.c | 23 -----------------------
2 files changed, 8 insertions(+), 24 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 5ab3c414ec0f..0652065cb113 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -15,6 +15,7 @@
#include <linux/of_platform.h>
#include <linux/platform_device.h>
#include <linux/pm_opp.h>
+#include <linux/reset.h>
#include <linux/slab.h>
#include <linux/types.h>
#include <linux/pm_domain.h>
@@ -286,7 +287,7 @@ static int venus_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct venus_core *core;
- int ret;
+ int i, ret;
core = devm_kzalloc(dev, sizeof(*core), GFP_KERNEL);
if (!core)
@@ -324,6 +325,12 @@ static int venus_probe(struct platform_device *pdev)
if (ret)
return ret;
+ for (i = 0; i < core->res->resets_num; i++) {
+ core->resets[i] = devm_reset_control_get_exclusive(dev, core->res->resets[i]);
+ if (IS_ERR(core->resets[i]))
+ return PTR_ERR(core->resets[i]);
+ }
+
if (core->pm_ops->core_get) {
ret = core->pm_ops->core_get(core);
if (ret)
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 730c4b593cec..5b2a40a2f524 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -939,25 +939,6 @@ static int core_resets_reset(struct venus_core *core)
return ret;
}
-static int core_resets_get(struct venus_core *core)
-{
- struct device *dev = core->dev;
- const struct venus_resources *res = core->res;
- unsigned int i;
- int ret;
-
- for (i = 0; i < res->resets_num; i++) {
- core->resets[i] =
- devm_reset_control_get_exclusive(dev, res->resets[i]);
- if (IS_ERR(core->resets[i])) {
- ret = PTR_ERR(core->resets[i]);
- return ret;
- }
- }
-
- return 0;
-}
-
static int core_get_v4(struct venus_core *core)
{
struct device *dev = core->dev;
@@ -981,10 +962,6 @@ static int core_get_v4(struct venus_core *core)
if (ret)
return ret;
- ret = core_resets_get(core);
- if (ret)
- return ret;
-
if (legacy_binding)
return 0;
--
2.44.0
On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
> There is no reason to keep reset_get code local to HFIv4/v6.
>
> Move it to the common part.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/media/platform/qcom/venus/core.c | 9 ++++++++-
> drivers/media/platform/qcom/venus/pm_helpers.c | 23 -----------------------
> 2 files changed, 8 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 5ab3c414ec0f..0652065cb113 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -15,6 +15,7 @@
> #include <linux/of_platform.h>
> #include <linux/platform_device.h>
> #include <linux/pm_opp.h>
> +#include <linux/reset.h>
> #include <linux/slab.h>
> #include <linux/types.h>
> #include <linux/pm_domain.h>
> @@ -286,7 +287,7 @@ static int venus_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> struct venus_core *core;
> - int ret;
> + int i, ret;
>
> core = devm_kzalloc(dev, sizeof(*core), GFP_KERNEL);
> if (!core)
> @@ -324,6 +325,12 @@ static int venus_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> + for (i = 0; i < core->res->resets_num; i++) {
> + core->resets[i] = devm_reset_control_get_exclusive(dev, core->res->resets[i]);
> + if (IS_ERR(core->resets[i]))
> + return PTR_ERR(core->resets[i]);
> + }
> +
> if (core->pm_ops->core_get) {
> ret = core->pm_ops->core_get(core);
> if (ret)
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index 730c4b593cec..5b2a40a2f524 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -939,25 +939,6 @@ static int core_resets_reset(struct venus_core *core)
> return ret;
> }
>
> -static int core_resets_get(struct venus_core *core)
> -{
> - struct device *dev = core->dev;
> - const struct venus_resources *res = core->res;
> - unsigned int i;
> - int ret;
> -
> - for (i = 0; i < res->resets_num; i++) {
> - core->resets[i] =
> - devm_reset_control_get_exclusive(dev, res->resets[i]);
> - if (IS_ERR(core->resets[i])) {
> - ret = PTR_ERR(core->resets[i]);
> - return ret;
> - }
> - }
> -
> - return 0;
> -}
> -
> static int core_get_v4(struct venus_core *core)
> {
> struct device *dev = core->dev;
> @@ -981,10 +962,6 @@ static int core_get_v4(struct venus_core *core)
> if (ret)
> return ret;
>
> - ret = core_resets_get(core);
> - if (ret)
> - return ret;
> -
> if (legacy_binding)
> return 0;
>
>
Reviewed-by: Dikshita Agarwal <[email protected]>
Hi Konrad,
On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
> Currently the infrastructure is set up for vast expandability, but
> it's far too complex for what is just 0-2 clocks. Categorize the
> clocks and simplify their getting.
>
> One notable change is that vcodec clocks are switched to use
> devm_clk_get_optional, which will let us commonize the code further
> while leaving the burden of figuring out which SoCs need codec-specific
> clocks and which don't to the bindings checker.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/media/platform/qcom/venus/core.c | 18 ----
> drivers/media/platform/qcom/venus/core.h | 13 ++-
> drivers/media/platform/qcom/venus/pm_helpers.c | 129 +++++++++++++------------
> 3 files changed, 71 insertions(+), 89 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index e61aa863b7f7..1f4a86b1bd73 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -581,9 +581,6 @@ static const struct venus_resources msm8996_res = {
> .reg_tbl_size = ARRAY_SIZE(msm8996_reg_preset),
> .clks = {"core", "iface", "bus", "mbus" },
> .clks_num = 4,
> - .vcodec0_clks = { "core" },
> - .vcodec1_clks = { "core" },
> - .vcodec_clks_num = 1,
> .max_load = 2563200,
> .hfi_version = HFI_VERSION_3XX,
> .dma_mask = (GENMASK(31, 30) | GENMASK(28, 26) | GENMASK(24, 22)) - 1,
> @@ -636,9 +633,6 @@ static const struct venus_resources sdm660_res = {
> .bw_tbl_dec_size = ARRAY_SIZE(sdm660_bw_table_dec),
> .clks = {"core", "iface", "bus", "bus_throttle" },
> .clks_num = 4,
> - .vcodec0_clks = { "vcodec0_core" },
> - .vcodec1_clks = { "vcodec0_core" },
> - .vcodec_clks_num = 1,
> .max_load = 1036800,
> .hfi_version = HFI_VERSION_3XX,
> .cp_size = 0x79000000,
> @@ -680,9 +674,6 @@ static const struct venus_resources sdm845_res = {
> .bw_tbl_dec_size = ARRAY_SIZE(sdm845_bw_table_dec),
> .clks = {"core", "iface", "bus" },
> .clks_num = 3,
> - .vcodec0_clks = { "core", "bus" },
> - .vcodec1_clks = { "core", "bus" },
> - .vcodec_clks_num = 2,
> .max_load = 3110400, /* 4096x2160@90 */
> .hfi_version = HFI_VERSION_4XX,
> .vpu_version = VPU_VERSION_AR50,
> @@ -699,9 +690,6 @@ static const struct venus_resources sdm845_res_v2 = {
> .bw_tbl_dec_size = ARRAY_SIZE(sdm845_bw_table_dec),
> .clks = {"core", "iface", "bus" },
> .clks_num = 3,
> - .vcodec0_clks = { "vcodec0_core", "vcodec0_bus" },
> - .vcodec1_clks = { "vcodec1_core", "vcodec1_bus" },
> - .vcodec_clks_num = 2,
> .vcodec_pmdomains = (const char *[]) { "venus", "vcodec0", "vcodec1" },
> .vcodec_pmdomains_num = 3,
> .opp_pmdomain = pd_names_cx,
> @@ -744,8 +732,6 @@ static const struct venus_resources sc7180_res = {
> .bw_tbl_dec_size = ARRAY_SIZE(sc7180_bw_table_dec),
> .clks = {"core", "iface", "bus" },
> .clks_num = 3,
> - .vcodec0_clks = { "vcodec0_core", "vcodec0_bus" },
> - .vcodec_clks_num = 2,
> .vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
> .vcodec_pmdomains_num = 2,
> .opp_pmdomain = pd_names_cx,
> @@ -796,8 +782,6 @@ static const struct venus_resources sm8250_res = {
> .clks_num = 2,
> .resets = { "bus", "core" },
> .resets_num = 2,
> - .vcodec0_clks = { "vcodec0_core" },
> - .vcodec_clks_num = 1,
> .vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
> .vcodec_pmdomains_num = 2,
> .opp_pmdomain = pd_names_mx,
> @@ -851,8 +835,6 @@ static const struct venus_resources sc7280_res = {
> .ubwc_conf = &sc7280_ubwc_config,
> .clks = {"core", "bus", "iface"},
> .clks_num = 3,
> - .vcodec0_clks = {"vcodec_core", "vcodec_bus"},
> - .vcodec_clks_num = 2,
> .vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
> .vcodec_pmdomains_num = 2,
> .opp_pmdomain = pd_names_cx,
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 19908f028441..b4c41dc0f8c7 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -24,9 +24,10 @@
> #define VDBGFW "VenusFW : "
>
> #define VIDC_CLKS_NUM_MAX 4
> -#define VIDC_VCODEC_CLKS_NUM_MAX 2
> #define VIDC_RESETS_NUM_MAX 2
>
> +#define MAX_NUM_VCODECS 2
> +
> extern int venus_fw_debug;
>
> struct freq_tbl {
> @@ -68,8 +69,6 @@ struct venus_resources {
> const struct hfi_ubwc_config *ubwc_conf;
> const char * const clks[VIDC_CLKS_NUM_MAX];
> unsigned int clks_num;
> - const char * const vcodec0_clks[VIDC_VCODEC_CLKS_NUM_MAX];
> - const char * const vcodec1_clks[VIDC_VCODEC_CLKS_NUM_MAX];
> unsigned int vcodec_clks_num;
> const char **vcodec_pmdomains;
> unsigned int vcodec_pmdomains_num;
> @@ -123,8 +122,8 @@ struct venus_format {
> * @aon_base: AON base address
> * @irq: Venus irq
> * @clks: an array of struct clk pointers
> - * @vcodec0_clks: an array of vcodec0 struct clk pointers
> - * @vcodec1_clks: an array of vcodec1 struct clk pointers
> + * @vcodec_core_clks: an array of codec core clk pointers
> + * @vcodec_bus_clks: an array of codec bus clk pointers
> * @video_path: an interconnect handle to video to/from memory path
> * @cpucfg_path: an interconnect handle to cpu configuration path
> * @has_opp_table: does OPP table exist
> @@ -176,8 +175,8 @@ struct venus_core {
> void __iomem *aon_base;
> int irq;
> struct clk *clks[VIDC_CLKS_NUM_MAX];
> - struct clk *vcodec0_clks[VIDC_VCODEC_CLKS_NUM_MAX];
> - struct clk *vcodec1_clks[VIDC_VCODEC_CLKS_NUM_MAX];
> + struct clk *vcodec_core_clks[MAX_NUM_VCODECS];
> + struct clk *vcodec_bus_clks[MAX_NUM_VCODECS];
> struct icc_path *video_path;
> struct icc_path *cpucfg_path;
> bool has_opp_table;
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index d717e150b34f..583153bbb74e 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -110,67 +110,74 @@ static void core_clks_disable(struct venus_core *core)
>
> static int core_clks_set_rate(struct venus_core *core, unsigned long freq)
> {
> - int ret;
> + int i, ret;
>
> ret = dev_pm_opp_set_rate(core->dev, freq);
> if (ret)
> return ret;
>
> - ret = clk_set_rate(core->vcodec0_clks[0], freq);
> - if (ret)
> - return ret;
> -
> - ret = clk_set_rate(core->vcodec1_clks[0], freq);
> - if (ret)
> - return ret;
> + for (i = 0; i < MAX_NUM_VCODECS; i++) {
> + ret = clk_set_rate(core->vcodec_core_clks[i], freq);
> + if (ret)
> + return ret;
> + }
>
> return 0;
> }
>
> -static int vcodec_clks_get(struct venus_core *core, struct device *dev,
> - struct clk **clks, const char * const *id)
> +static int vcodec_clks_get(struct venus_core *core, struct device *dev, u8 id)
> {
> - const struct venus_resources *res = core->res;
> - unsigned int i;
> + char buf[13] = { 0 }; /* vcodecX_core\0 */
>
> - for (i = 0; i < res->vcodec_clks_num; i++) {
> - if (!id[i])
> - continue;
> - clks[i] = devm_clk_get(dev, id[i]);
> - if (IS_ERR(clks[i]))
> - return PTR_ERR(clks[i]);
> + /* Best we can do is 2 cores */
> + if (id > MAX_NUM_VCODECS - 1) {
> + dev_err(dev, "Got impossible vcodec id %u\n", id);
> + return -EINVAL;
> + };
> +
> + snprintf(buf, sizeof(buf), "vcodec%u_core", id);
> +
> + /* First try the non-legacy name */
> + core->vcodec_core_clks[id] = devm_clk_get_optional(dev, buf);
> + if (IS_ERR(core->vcodec_core_clks[id])) {
> + /* Try again, with the legacy name */
> + core->vcodec_core_clks[id] = devm_clk_get_optional(dev, "core");
> + if (IS_ERR(core->vcodec_core_clks[id]))
> + return PTR_ERR(core->vcodec_core_clks[id]);
> + }
> +
> + memset(buf, 0, sizeof(buf));
> + snprintf(buf, sizeof(buf), "vcodec%u_bus", id);
> +
> + core->vcodec_bus_clks[id] = devm_clk_get_optional(dev, buf);
> + if (IS_ERR(core->vcodec_bus_clks[id])) {
> + core->vcodec_bus_clks[id] = devm_clk_get_optional(dev, "bus");
> + if (IS_ERR(core->vcodec_bus_clks[id]))
> + return PTR_ERR(core->vcodec_bus_clks[id]);
> }
>
> return 0;
> }
>
> -static int vcodec_clks_enable(struct venus_core *core, struct clk **clks)
> +static int vcodec_clks_enable(struct venus_core *core, u8 id)
> {
> - const struct venus_resources *res = core->res;
> - unsigned int i;
> int ret;
>
> - for (i = 0; i < res->vcodec_clks_num; i++) {
> - ret = clk_prepare_enable(clks[i]);
> - if (ret)
> - goto err;
> - }
> + ret = clk_prepare_enable(core->vcodec_core_clks[id]);
> + if (ret)
> + return ret;
>
> - return 0;
> -err:
> - while (i--)
> - clk_disable_unprepare(clks[i]);
> + ret = clk_prepare_enable(core->vcodec_bus_clks[id]);
> + if (ret)
> + clk_disable_unprepare(core->vcodec_core_clks[id]);
>
> return ret;
> }
>
> -static void vcodec_clks_disable(struct venus_core *core, struct clk **clks)
> +static void vcodec_clks_disable(struct venus_core *core, u8 id)
> {
> - const struct venus_resources *res = core->res;
> - unsigned int i = res->vcodec_clks_num;
> -
> - while (i--)
> - clk_disable_unprepare(clks[i]);
> + clk_disable_unprepare(core->vcodec_bus_clks[id]);
> + clk_disable_unprepare(core->vcodec_core_clks[id]);
> }
>
> static u32 load_per_instance(struct venus_inst *inst)
> @@ -343,8 +350,7 @@ static int vdec_get_v3(struct device *dev)
> {
> struct venus_core *core = dev_get_drvdata(dev);
>
> - return vcodec_clks_get(core, dev, core->vcodec0_clks,
> - core->res->vcodec0_clks);
> + return vcodec_clks_get(core, dev, 0);
> }
>
> static int vdec_power_v3(struct device *dev, int on)
> @@ -355,9 +361,9 @@ static int vdec_power_v3(struct device *dev, int on)
> vcodec_control_v3(core, VIDC_SESSION_TYPE_DEC, true);
>
> if (on == POWER_ON)
> - ret = vcodec_clks_enable(core, core->vcodec0_clks);
> + ret = vcodec_clks_enable(core, 0);
> else
> - vcodec_clks_disable(core, core->vcodec0_clks);
> + vcodec_clks_disable(core, 0);
>
> vcodec_control_v3(core, VIDC_SESSION_TYPE_DEC, false);
>
> @@ -368,8 +374,7 @@ static int venc_get_v3(struct device *dev)
> {
> struct venus_core *core = dev_get_drvdata(dev);
>
> - return vcodec_clks_get(core, dev, core->vcodec1_clks,
> - core->res->vcodec1_clks);
> + return vcodec_clks_get(core, dev, 1);
> }
>
> static int venc_power_v3(struct device *dev, int on)
> @@ -380,9 +385,9 @@ static int venc_power_v3(struct device *dev, int on)
> vcodec_control_v3(core, VIDC_SESSION_TYPE_ENC, true);
>
> if (on == POWER_ON)
> - ret = vcodec_clks_enable(core, core->vcodec1_clks);
> + ret = vcodec_clks_enable(core, 1);
> else
> - vcodec_clks_disable(core, core->vcodec1_clks);
> + vcodec_clks_disable(core, 1);
>
> vcodec_control_v3(core, VIDC_SESSION_TYPE_ENC, false);
>
> @@ -441,7 +446,7 @@ static int poweroff_coreid(struct venus_core *core, unsigned int coreid_mask)
> if (ret)
> return ret;
>
> - vcodec_clks_disable(core, core->vcodec0_clks);
> + vcodec_clks_disable(core, 0);
>
> ret = vcodec_control_v4(core, VIDC_CORE_ID_1, false);
> if (ret)
> @@ -457,7 +462,7 @@ static int poweroff_coreid(struct venus_core *core, unsigned int coreid_mask)
> if (ret)
> return ret;
>
> - vcodec_clks_disable(core, core->vcodec1_clks);
> + vcodec_clks_disable(core, 1);
>
> ret = vcodec_control_v4(core, VIDC_CORE_ID_2, false);
> if (ret)
> @@ -484,7 +489,7 @@ static int poweron_coreid(struct venus_core *core, unsigned int coreid_mask)
> if (ret)
> return ret;
>
> - ret = vcodec_clks_enable(core, core->vcodec0_clks);
> + ret = vcodec_clks_enable(core, 0);
> if (ret)
> return ret;
>
> @@ -502,7 +507,7 @@ static int poweron_coreid(struct venus_core *core, unsigned int coreid_mask)
> if (ret)
> return ret;
>
> - ret = vcodec_clks_enable(core, core->vcodec1_clks);
> + ret = vcodec_clks_enable(core, 1);
> if (ret)
> return ret;
>
> @@ -763,20 +768,18 @@ static int vdec_get_v4(struct device *dev)
> if (!legacy_binding)
> return 0;
>
> - return vcodec_clks_get(core, dev, core->vcodec0_clks,
> - core->res->vcodec0_clks);
> + return vcodec_clks_get(core, dev, 0);
> }
>
> static void vdec_put_v4(struct device *dev)
> {
> struct venus_core *core = dev_get_drvdata(dev);
> - unsigned int i;
>
> if (!legacy_binding)
> return;
>
> - for (i = 0; i < core->res->vcodec_clks_num; i++)
> - core->vcodec0_clks[i] = NULL;
> + core->vcodec_core_clks[0] = NULL;
> + core->vcodec_bus_clks[0] = NULL;
> }
>
> static int vdec_power_v4(struct device *dev, int on)
> @@ -792,9 +795,9 @@ static int vdec_power_v4(struct device *dev, int on)
> return ret;
>
> if (on == POWER_ON)
> - ret = vcodec_clks_enable(core, core->vcodec0_clks);
> + ret = vcodec_clks_enable(core, 0);
> else
> - vcodec_clks_disable(core, core->vcodec0_clks);
> + vcodec_clks_disable(core, 0);
>
> vcodec_control_v4(core, VIDC_CORE_ID_1, false);
>
> @@ -808,20 +811,18 @@ static int venc_get_v4(struct device *dev)
> if (!legacy_binding)
> return 0;
>
> - return vcodec_clks_get(core, dev, core->vcodec1_clks,
> - core->res->vcodec1_clks);
> + return vcodec_clks_get(core, dev, 1);
> }
>
> static void venc_put_v4(struct device *dev)
> {
> struct venus_core *core = dev_get_drvdata(dev);
> - unsigned int i;
>
> if (!legacy_binding)
> return;
>
> - for (i = 0; i < core->res->vcodec_clks_num; i++)
> - core->vcodec1_clks[i] = NULL;
> + core->vcodec_core_clks[1] = NULL;
> + core->vcodec_bus_clks[1] = NULL;
> }
>
> static int venc_power_v4(struct device *dev, int on)
> @@ -837,9 +838,9 @@ static int venc_power_v4(struct device *dev, int on)
> return ret;
>
> if (on == POWER_ON)
> - ret = vcodec_clks_enable(core, core->vcodec1_clks);
> + ret = vcodec_clks_enable(core, 1);
> else
> - vcodec_clks_disable(core, core->vcodec1_clks);
> + vcodec_clks_disable(core, 1);
>
> vcodec_control_v4(core, VIDC_CORE_ID_2, false);
>
> @@ -934,11 +935,11 @@ static int core_get_v4(struct venus_core *core)
>
> dev_info(dev, "%s legacy binding\n", legacy_binding ? "" : "non");
>
> - ret = vcodec_clks_get(core, dev, core->vcodec0_clks, res->vcodec0_clks);
> + ret = vcodec_clks_get(core, dev, 0);
> if (ret)
> return ret;
>
> - ret = vcodec_clks_get(core, dev, core->vcodec1_clks, res->vcodec1_clks);
> + ret = vcodec_clks_get(core, dev, 1);
> if (ret)
> return ret;
>
>
It is better to keep the information about all clocks being used for a
particular SOC in one place, like its currently captured in resource
structure. Leaving core clocks in resources and moving vcodec clocks to
other places makes it less readable.
Also let's say, if in future, a new clock is introcued, we will need to
introduce a new array additional to vcodec_core, vcodec_bus.
Thanks,
Dikshita