* [PATCH v2 0/2] SM8350 VIDEOCC
@ 2023-04-14 11:26 Konrad Dybcio
2023-04-14 11:26 ` [PATCH v2 1/2] dt-bindings: clock: qcom,videocc: Add SM8350 Konrad Dybcio
2023-04-14 11:26 ` [PATCH v2 2/2] clk: qcom: Introduce SM8350 VIDEOCC Konrad Dybcio
0 siblings, 2 replies; 12+ messages in thread
From: Konrad Dybcio @ 2023-04-14 11:26 UTC (permalink / raw)
To: Bjorn Andersson, Andy Gross, Michael Turquette, Stephen Boyd,
Rob Herring, Krzysztof Kozlowski, Philipp Zabel, Taniya Das
Cc: Marijn Suijten, linux-arm-msm, linux-clk, devicetree,
linux-kernel, Konrad Dybcio
v1 -> v2:
- "){" -> ") {"
- subsys_initcall -> module_platform_driver
- constify lucid_5lpe_vco & .hw.init
- devm_add_action_or_reset -> devm_pm_runtime_enable
v1: https://lore.kernel.org/r/20230413-topic-lahaina_vidcc-v1-0-134f9b22a5b3@linaro.org
This serires brings support for SM8350 videocc and updates the
related dt-bindings.
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
Konrad Dybcio (2):
dt-bindings: clock: qcom,videocc: Add SM8350
clk: qcom: Introduce SM8350 VIDEOCC
.../devicetree/bindings/clock/qcom,videocc.yaml | 29 +-
drivers/clk/qcom/Kconfig | 9 +
drivers/clk/qcom/Makefile | 1 +
drivers/clk/qcom/videocc-sm8350.c | 557 +++++++++++++++++++++
include/dt-bindings/clock/qcom,sm8350-videocc.h | 35 ++
include/dt-bindings/reset/qcom,sm8350-videocc.h | 18 +
6 files changed, 648 insertions(+), 1 deletion(-)
---
base-commit: e3342532ecd39bbd9c2ab5b9001cec1589bc37e9
change-id: 20230413-topic-lahaina_vidcc-bcdabb475542
Best regards,
--
Konrad Dybcio <konrad.dybcio@linaro.org>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/2] dt-bindings: clock: qcom,videocc: Add SM8350
2023-04-14 11:26 [PATCH v2 0/2] SM8350 VIDEOCC Konrad Dybcio
@ 2023-04-14 11:26 ` Konrad Dybcio
2023-04-14 15:17 ` Krzysztof Kozlowski
2023-04-14 11:26 ` [PATCH v2 2/2] clk: qcom: Introduce SM8350 VIDEOCC Konrad Dybcio
1 sibling, 1 reply; 12+ messages in thread
From: Konrad Dybcio @ 2023-04-14 11:26 UTC (permalink / raw)
To: Bjorn Andersson, Andy Gross, Michael Turquette, Stephen Boyd,
Rob Herring, Krzysztof Kozlowski, Philipp Zabel, Taniya Das
Cc: Marijn Suijten, linux-arm-msm, linux-clk, devicetree,
linux-kernel, Konrad Dybcio
SM8350, like most recent higher-end chips has a separate clock
controller block just for the Venus IP. Document it.
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
.../devicetree/bindings/clock/qcom,videocc.yaml | 29 +++++++++++++++++-
include/dt-bindings/clock/qcom,sm8350-videocc.h | 35 ++++++++++++++++++++++
include/dt-bindings/reset/qcom,sm8350-videocc.h | 18 +++++++++++
3 files changed, 81 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/clock/qcom,videocc.yaml b/Documentation/devicetree/bindings/clock/qcom,videocc.yaml
index 2b07146161b4..6d892b0f2306 100644
--- a/Documentation/devicetree/bindings/clock/qcom,videocc.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,videocc.yaml
@@ -19,6 +19,8 @@ description: |
include/dt-bindings/clock/qcom,videocc-sdm845.h
include/dt-bindings/clock/qcom,videocc-sm8150.h
include/dt-bindings/clock/qcom,videocc-sm8250.h
+ include/dt-bindings/clock/qcom,videocc-sm8350.h
+ include/dt-bindings/reset/qcom,videocc-sm8350.h
properties:
compatible:
@@ -28,6 +30,7 @@ properties:
- qcom,sdm845-videocc
- qcom,sm8150-videocc
- qcom,sm8250-videocc
+ - qcom,sm8350-videocc
clocks:
minItems: 1
@@ -63,7 +66,6 @@ required:
- compatible
- reg
- clocks
- - clock-names
- '#clock-cells'
- '#reset-cells'
- '#power-domain-cells'
@@ -85,6 +87,9 @@ allOf:
items:
- const: bi_tcxo
+ required:
+ - clock-names
+
- if:
properties:
compatible:
@@ -101,6 +106,9 @@ allOf:
- const: bi_tcxo
- const: bi_tcxo_ao
+ required:
+ - clock-names
+
- if:
properties:
compatible:
@@ -119,6 +127,25 @@ allOf:
- const: bi_tcxo
- const: bi_tcxo_ao
+ required:
+ - clock-names
+
+ - if:
+ properties:
+ compatible:
+ enum:
+ - qcom,sm8350-videocc
+ then:
+ properties:
+ clocks:
+ items:
+ - description: Board XO source
+ - description: Board active XO source
+ - description: Board sleep clock
+
+ required:
+ - power-domains
+
additionalProperties: false
examples:
diff --git a/include/dt-bindings/clock/qcom,sm8350-videocc.h b/include/dt-bindings/clock/qcom,sm8350-videocc.h
new file mode 100644
index 000000000000..b6945a448676
--- /dev/null
+++ b/include/dt-bindings/clock/qcom,sm8350-videocc.h
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Copyright (c) 2019, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2023, Linaro Limited
+ */
+
+#ifndef _DT_BINDINGS_CLK_QCOM_VIDEO_CC_SM8350_H
+#define _DT_BINDINGS_CLK_QCOM_VIDEO_CC_SM8350_H
+
+/* Clocks */
+#define VIDEO_CC_AHB_CLK_SRC 0
+#define VIDEO_CC_MVS0_CLK 1
+#define VIDEO_CC_MVS0_CLK_SRC 2
+#define VIDEO_CC_MVS0_DIV_CLK_SRC 3
+#define VIDEO_CC_MVS0C_CLK 4
+#define VIDEO_CC_MVS0C_DIV2_DIV_CLK_SRC 5
+#define VIDEO_CC_MVS1_CLK 6
+#define VIDEO_CC_MVS1_CLK_SRC 7
+#define VIDEO_CC_MVS1_DIV2_CLK 8
+#define VIDEO_CC_MVS1_DIV_CLK_SRC 9
+#define VIDEO_CC_MVS1C_CLK 10
+#define VIDEO_CC_MVS1C_DIV2_DIV_CLK_SRC 11
+#define VIDEO_CC_SLEEP_CLK 12
+#define VIDEO_CC_SLEEP_CLK_SRC 13
+#define VIDEO_CC_XO_CLK_SRC 14
+#define VIDEO_PLL0 15
+#define VIDEO_PLL1 16
+
+/* GDSCs */
+#define MVS0C_GDSC 0
+#define MVS1C_GDSC 1
+#define MVS0_GDSC 2
+#define MVS1_GDSC 3
+
+#endif
diff --git a/include/dt-bindings/reset/qcom,sm8350-videocc.h b/include/dt-bindings/reset/qcom,sm8350-videocc.h
new file mode 100644
index 000000000000..df7a808720ee
--- /dev/null
+++ b/include/dt-bindings/reset/qcom,sm8350-videocc.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Copyright (c) 2019, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2023, Linaro Limited
+ */
+
+#ifndef _DT_BINDINGS_RESET_QCOM_VIDEO_CC_SM8350_H
+#define _DT_BINDINGS_RESET_QCOM_VIDEO_CC_SM8350_H
+
+#define CVP_VIDEO_CC_INTERFACE_BCR 0
+#define CVP_VIDEO_CC_MVS0_BCR 1
+#define VIDEO_CC_MVS0C_CLK_ARES 2
+#define CVP_VIDEO_CC_MVS0C_BCR 3
+#define CVP_VIDEO_CC_MVS1_BCR 4
+#define VIDEO_CC_MVS1C_CLK_ARES 5
+#define CVP_VIDEO_CC_MVS1C_BCR 6
+
+#endif
--
2.40.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/2] clk: qcom: Introduce SM8350 VIDEOCC
2023-04-14 11:26 [PATCH v2 0/2] SM8350 VIDEOCC Konrad Dybcio
2023-04-14 11:26 ` [PATCH v2 1/2] dt-bindings: clock: qcom,videocc: Add SM8350 Konrad Dybcio
@ 2023-04-14 11:26 ` Konrad Dybcio
2023-04-14 16:31 ` Dmitry Baryshkov
1 sibling, 1 reply; 12+ messages in thread
From: Konrad Dybcio @ 2023-04-14 11:26 UTC (permalink / raw)
To: Bjorn Andersson, Andy Gross, Michael Turquette, Stephen Boyd,
Rob Herring, Krzysztof Kozlowski, Philipp Zabel, Taniya Das
Cc: Marijn Suijten, linux-arm-msm, linux-clk, devicetree,
linux-kernel, Konrad Dybcio
Add support for the Video Clock Controller found on the SM8350 SoC.
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
drivers/clk/qcom/Kconfig | 9 +
drivers/clk/qcom/Makefile | 1 +
drivers/clk/qcom/videocc-sm8350.c | 557 ++++++++++++++++++++++++++++++++++++++
3 files changed, 567 insertions(+)
diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index d71c9d6036bb..dbb1dfcddb31 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -916,6 +916,15 @@ config SM_VIDEOCC_8250
Say Y if you want to support video devices and functionality such as
video encode and decode.
+config SM_VIDEOCC_8350
+ tristate "SM8350 Video Clock Controller"
+ select SM_GCC_8350
+ select QCOM_GDSC
+ help
+ Support for the video clock controller on SM8350 devices.
+ Say Y if you want to support video devices and functionality such as
+ video encode and decode.
+
config SPMI_PMIC_CLKDIV
tristate "SPMI PMIC clkdiv Support"
depends on SPMI || COMPILE_TEST
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index b54085e579a0..53290040523b 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -126,6 +126,7 @@ obj-$(CONFIG_SM_GPUCC_8350) += gpucc-sm8350.o
obj-$(CONFIG_SM_TCSRCC_8550) += tcsrcc-sm8550.o
obj-$(CONFIG_SM_VIDEOCC_8150) += videocc-sm8150.o
obj-$(CONFIG_SM_VIDEOCC_8250) += videocc-sm8250.o
+obj-$(CONFIG_SM_VIDEOCC_8350) += videocc-sm8350.o
obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o
obj-$(CONFIG_KPSS_XCC) += kpss-xcc.o
obj-$(CONFIG_QCOM_HFPLL) += hfpll.o
diff --git a/drivers/clk/qcom/videocc-sm8350.c b/drivers/clk/qcom/videocc-sm8350.c
new file mode 100644
index 000000000000..e0cf474a632d
--- /dev/null
+++ b/drivers/clk/qcom/videocc-sm8350.c
@@ -0,0 +1,557 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2019-2020, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2023, Linaro Limited
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm_clock.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+
+#include <dt-bindings/clock/qcom,sm8350-videocc.h>
+#include <dt-bindings/reset/qcom,sm8350-videocc.h>
+
+#include "clk-alpha-pll.h"
+#include "clk-branch.h"
+#include "clk-rcg.h"
+#include "clk-regmap.h"
+#include "clk-regmap-divider.h"
+#include "common.h"
+#include "reset.h"
+#include "gdsc.h"
+
+enum {
+ DT_BI_TCXO,
+ DT_BI_TCXO_AO,
+ DT_SLEEP_CLK,
+};
+
+enum {
+ P_BI_TCXO,
+ P_BI_TCXO_AO,
+ P_SLEEP_CLK,
+ P_VIDEO_PLL0_OUT_MAIN,
+ P_VIDEO_PLL1_OUT_MAIN,
+};
+
+static const struct pll_vco lucid_5lpe_vco[] = {
+ { 249600000, 1750000000, 0 },
+};
+
+static const struct alpha_pll_config video_pll0_config = {
+ .l = 0x25,
+ .alpha = 0x8000,
+ .config_ctl_val = 0x20485699,
+ .config_ctl_hi_val = 0x00002261,
+ .config_ctl_hi1_val = 0x2a9a699c,
+ .test_ctl_val = 0x00000000,
+ .test_ctl_hi_val = 0x00000000,
+ .test_ctl_hi1_val = 0x01800000,
+ .user_ctl_val = 0x00000000,
+ .user_ctl_hi_val = 0x00000805,
+ .user_ctl_hi1_val = 0x00000000,
+};
+
+static struct clk_alpha_pll video_pll0 = {
+ .offset = 0x42c,
+ .vco_table = lucid_5lpe_vco,
+ .num_vco = ARRAY_SIZE(lucid_5lpe_vco),
+ .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_LUCID],
+ .clkr = {
+ .hw.init = &(const struct clk_init_data) {
+ .name = "video_pll0",
+ .parent_data = &(const struct clk_parent_data){
+ .index = DT_BI_TCXO,
+ },
+ .num_parents = 1,
+ .ops = &clk_alpha_pll_lucid_5lpe_ops,
+ },
+ },
+};
+
+static const struct alpha_pll_config video_pll1_config = {
+ .l = 0x2b,
+ .alpha = 0xc000,
+ .config_ctl_val = 0x20485699,
+ .config_ctl_hi_val = 0x00002261,
+ .config_ctl_hi1_val = 0x2a9a699c,
+ .test_ctl_val = 0x00000000,
+ .test_ctl_hi_val = 0x00000000,
+ .test_ctl_hi1_val = 0x01800000,
+ .user_ctl_val = 0x00000000,
+ .user_ctl_hi_val = 0x00000805,
+ .user_ctl_hi1_val = 0x00000000,
+};
+
+static struct clk_alpha_pll video_pll1 = {
+ .offset = 0x7d0,
+ .vco_table = lucid_5lpe_vco,
+ .num_vco = ARRAY_SIZE(lucid_5lpe_vco),
+ .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_LUCID],
+ .clkr = {
+ .hw.init = &(const struct clk_init_data) {
+ .name = "video_pll1",
+ .parent_data = &(const struct clk_parent_data){
+ .index = DT_BI_TCXO,
+ },
+ .num_parents = 1,
+ .ops = &clk_alpha_pll_lucid_5lpe_ops,
+ },
+ },
+};
+
+static const struct parent_map video_cc_parent_map_0[] = {
+ { P_BI_TCXO_AO, 0 },
+};
+
+static const struct clk_parent_data video_cc_parent_data_0[] = {
+ { .index = DT_BI_TCXO_AO },
+};
+
+static const struct parent_map video_cc_parent_map_1[] = {
+ { P_BI_TCXO, 0 },
+ { P_VIDEO_PLL0_OUT_MAIN, 1 },
+};
+
+static const struct clk_parent_data video_cc_parent_data_1[] = {
+ { .index = DT_BI_TCXO },
+ { .hw = &video_pll0.clkr.hw },
+};
+
+static const struct parent_map video_cc_parent_map_2[] = {
+ { P_BI_TCXO, 0 },
+ { P_VIDEO_PLL1_OUT_MAIN, 1 },
+};
+
+static const struct clk_parent_data video_cc_parent_data_2[] = {
+ { .index = DT_BI_TCXO },
+ { .hw = &video_pll1.clkr.hw },
+};
+
+static const struct freq_tbl ftbl_video_cc_ahb_clk_src[] = {
+ F(19200000, P_BI_TCXO, 1, 0, 0),
+ { }
+};
+
+static struct clk_rcg2 video_cc_ahb_clk_src = {
+ .cmd_rcgr = 0xbd4,
+ .mnd_width = 0,
+ .hid_width = 5,
+ .parent_map = video_cc_parent_map_0,
+ .freq_tbl = ftbl_video_cc_ahb_clk_src,
+ .clkr.hw.init = &(const struct clk_init_data) {
+ .name = "video_cc_ahb_clk_src",
+ .parent_data = video_cc_parent_data_0,
+ .num_parents = ARRAY_SIZE(video_cc_parent_data_0),
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_rcg2_shared_ops,
+ },
+};
+
+static const struct freq_tbl ftbl_video_cc_mvs0_clk_src[] = {
+ F(720000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0),
+ F(1014000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0),
+ F(1098000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0),
+ F(1332000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0),
+ { }
+};
+
+static struct clk_rcg2 video_cc_mvs0_clk_src = {
+ .cmd_rcgr = 0xb94,
+ .mnd_width = 0,
+ .hid_width = 5,
+ .parent_map = video_cc_parent_map_1,
+ .freq_tbl = ftbl_video_cc_mvs0_clk_src,
+ .clkr.hw.init = &(const struct clk_init_data) {
+ .name = "video_cc_mvs0_clk_src",
+ .parent_data = video_cc_parent_data_1,
+ .num_parents = ARRAY_SIZE(video_cc_parent_data_1),
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_rcg2_shared_ops,
+ },
+};
+
+static const struct freq_tbl ftbl_video_cc_mvs1_clk_src[] = {
+ F(840000000, P_VIDEO_PLL1_OUT_MAIN, 1, 0, 0),
+ F(1098000000, P_VIDEO_PLL1_OUT_MAIN, 1, 0, 0),
+ F(1332000000, P_VIDEO_PLL1_OUT_MAIN, 1, 0, 0),
+ { }
+};
+
+static struct clk_rcg2 video_cc_mvs1_clk_src = {
+ .cmd_rcgr = 0xbb4,
+ .mnd_width = 0,
+ .hid_width = 5,
+ .parent_map = video_cc_parent_map_2,
+ .freq_tbl = ftbl_video_cc_mvs1_clk_src,
+ .clkr.hw.init = &(const struct clk_init_data) {
+ .name = "video_cc_mvs1_clk_src",
+ .parent_data = video_cc_parent_data_2,
+ .num_parents = ARRAY_SIZE(video_cc_parent_data_2),
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_rcg2_shared_ops,
+ },
+};
+
+static const struct freq_tbl ftbl_video_cc_sleep_clk_src[] = {
+ F(32000, P_SLEEP_CLK, 1, 0, 0),
+ { }
+};
+
+static struct clk_rcg2 video_cc_sleep_clk_src = {
+ .cmd_rcgr = 0xef0,
+ .mnd_width = 0,
+ .hid_width = 5,
+ .freq_tbl = ftbl_video_cc_sleep_clk_src,
+ .clkr.hw.init = &(const struct clk_init_data) {
+ .name = "video_cc_sleep_clk_src",
+ .parent_data = &(const struct clk_parent_data){
+ .index = DT_SLEEP_CLK,
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_rcg2_ops,
+ },
+};
+
+static struct clk_rcg2 video_cc_xo_clk_src = {
+ .cmd_rcgr = 0xecc,
+ .mnd_width = 0,
+ .hid_width = 5,
+ .parent_map = video_cc_parent_map_0,
+ .freq_tbl = ftbl_video_cc_ahb_clk_src,
+ .clkr.hw.init = &(const struct clk_init_data) {
+ .name = "video_cc_xo_clk_src",
+ .parent_data = video_cc_parent_data_0,
+ .num_parents = ARRAY_SIZE(video_cc_parent_data_0),
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_rcg2_ops,
+ },
+};
+
+static struct clk_regmap_div video_cc_mvs0_div_clk_src = {
+ .reg = 0xd54,
+ .shift = 0,
+ .width = 4,
+ .clkr.hw.init = &(const struct clk_init_data) {
+ .name = "video_cc_mvs0_div_clk_src",
+ .parent_hws = (const struct clk_hw*[]){
+ &video_cc_mvs0_clk_src.clkr.hw,
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_regmap_div_ro_ops,
+ },
+};
+
+static struct clk_regmap_div video_cc_mvs0c_div2_div_clk_src = {
+ .reg = 0xc54,
+ .shift = 0,
+ .width = 4,
+ .clkr.hw.init = &(const struct clk_init_data) {
+ .name = "video_cc_mvs0c_div2_div_clk_src",
+ .parent_hws = (const struct clk_hw*[]){
+ &video_cc_mvs0_clk_src.clkr.hw,
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_regmap_div_ro_ops,
+ },
+};
+
+static struct clk_regmap_div video_cc_mvs1_div_clk_src = {
+ .reg = 0xdd4,
+ .shift = 0,
+ .width = 4,
+ .clkr.hw.init = &(const struct clk_init_data) {
+ .name = "video_cc_mvs1_div_clk_src",
+ .parent_hws = (const struct clk_hw*[]){
+ &video_cc_mvs1_clk_src.clkr.hw,
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_regmap_div_ro_ops,
+ },
+};
+
+static struct clk_regmap_div video_cc_mvs1c_div2_div_clk_src = {
+ .reg = 0xcf4,
+ .shift = 0,
+ .width = 4,
+ .clkr.hw.init = &(const struct clk_init_data) {
+ .name = "video_cc_mvs1c_div2_div_clk_src",
+ .parent_hws = (const struct clk_hw*[]){
+ &video_cc_mvs1_clk_src.clkr.hw,
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_regmap_div_ro_ops,
+ },
+};
+
+static struct clk_branch video_cc_mvs0_clk = {
+ .halt_reg = 0xd34,
+ .halt_check = BRANCH_HALT_VOTED,
+ .hwcg_reg = 0xd34,
+ .hwcg_bit = 1,
+ .clkr = {
+ .enable_reg = 0xd34,
+ .enable_mask = BIT(0),
+ .hw.init = &(const struct clk_init_data) {
+ .name = "video_cc_mvs0_clk",
+ .parent_hws = (const struct clk_hw*[]){
+ &video_cc_mvs0_div_clk_src.clkr.hw,
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_branch2_ops,
+ },
+ },
+};
+
+static struct clk_branch video_cc_mvs0c_clk = {
+ .halt_reg = 0xc34,
+ .halt_check = BRANCH_HALT,
+ .clkr = {
+ .enable_reg = 0xc34,
+ .enable_mask = BIT(0),
+ .hw.init = &(const struct clk_init_data) {
+ .name = "video_cc_mvs0c_clk",
+ .parent_hws = (const struct clk_hw*[]){
+ &video_cc_mvs0c_div2_div_clk_src.clkr.hw,
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_branch2_ops,
+ },
+ },
+};
+
+static struct clk_branch video_cc_mvs1_clk = {
+ .halt_reg = 0xdb4,
+ .halt_check = BRANCH_HALT_VOTED,
+ .hwcg_reg = 0xdb4,
+ .hwcg_bit = 1,
+ .clkr = {
+ .enable_reg = 0xdb4,
+ .enable_mask = BIT(0),
+ .hw.init = &(const struct clk_init_data) {
+ .name = "video_cc_mvs1_clk",
+ .parent_hws = (const struct clk_hw*[]){
+ &video_cc_mvs1_div_clk_src.clkr.hw,
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_branch2_ops,
+ },
+ },
+};
+
+static struct clk_branch video_cc_mvs1_div2_clk = {
+ .halt_reg = 0xdf4,
+ .halt_check = BRANCH_HALT_VOTED,
+ .hwcg_reg = 0xdf4,
+ .hwcg_bit = 1,
+ .clkr = {
+ .enable_reg = 0xdf4,
+ .enable_mask = BIT(0),
+ .hw.init = &(const struct clk_init_data) {
+ .name = "video_cc_mvs1_div2_clk",
+ .parent_hws = (const struct clk_hw*[]){
+ &video_cc_mvs1c_div2_div_clk_src.clkr.hw,
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_branch2_ops,
+ },
+ },
+};
+
+static struct clk_branch video_cc_mvs1c_clk = {
+ .halt_reg = 0xcd4,
+ .halt_check = BRANCH_HALT,
+ .clkr = {
+ .enable_reg = 0xcd4,
+ .enable_mask = BIT(0),
+ .hw.init = &(const struct clk_init_data) {
+ .name = "video_cc_mvs1c_clk",
+ .parent_hws = (const struct clk_hw*[]){
+ &video_cc_mvs1c_div2_div_clk_src.clkr.hw,
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_branch2_ops,
+ },
+ },
+};
+
+static struct clk_branch video_cc_sleep_clk = {
+ .halt_reg = 0xf10,
+ .halt_check = BRANCH_HALT,
+ .clkr = {
+ .enable_reg = 0xf10,
+ .enable_mask = BIT(0),
+ .hw.init = &(const struct clk_init_data) {
+ .name = "video_cc_sleep_clk",
+ .parent_hws = (const struct clk_hw*[]){
+ &video_cc_sleep_clk_src.clkr.hw,
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_branch2_ops,
+ },
+ },
+};
+
+static struct gdsc mvs0c_gdsc = {
+ .gdscr = 0xbf8,
+ .pd = {
+ .name = "mvs0c_gdsc",
+ },
+ .flags = RETAIN_FF_ENABLE,
+ .pwrsts = PWRSTS_OFF_ON,
+};
+
+static struct gdsc mvs1c_gdsc = {
+ .gdscr = 0xc98,
+ .pd = {
+ .name = "mvs1c_gdsc",
+ },
+ .flags = RETAIN_FF_ENABLE,
+ .pwrsts = PWRSTS_OFF_ON,
+};
+
+static struct gdsc mvs0_gdsc = {
+ .gdscr = 0xd18,
+ .pd = {
+ .name = "mvs0_gdsc",
+ },
+ .flags = HW_CTRL | RETAIN_FF_ENABLE,
+ .pwrsts = PWRSTS_OFF_ON,
+};
+
+static struct gdsc mvs1_gdsc = {
+ .gdscr = 0xd98,
+ .pd = {
+ .name = "mvs1_gdsc",
+ },
+ .flags = HW_CTRL | RETAIN_FF_ENABLE,
+ .pwrsts = PWRSTS_OFF_ON,
+};
+
+static struct clk_regmap *video_cc_sm8350_clocks[] = {
+ [VIDEO_CC_AHB_CLK_SRC] = &video_cc_ahb_clk_src.clkr,
+ [VIDEO_CC_MVS0_CLK] = &video_cc_mvs0_clk.clkr,
+ [VIDEO_CC_MVS0_CLK_SRC] = &video_cc_mvs0_clk_src.clkr,
+ [VIDEO_CC_MVS0_DIV_CLK_SRC] = &video_cc_mvs0_div_clk_src.clkr,
+ [VIDEO_CC_MVS0C_CLK] = &video_cc_mvs0c_clk.clkr,
+ [VIDEO_CC_MVS0C_DIV2_DIV_CLK_SRC] = &video_cc_mvs0c_div2_div_clk_src.clkr,
+ [VIDEO_CC_MVS1_CLK] = &video_cc_mvs1_clk.clkr,
+ [VIDEO_CC_MVS1_CLK_SRC] = &video_cc_mvs1_clk_src.clkr,
+ [VIDEO_CC_MVS1_DIV2_CLK] = &video_cc_mvs1_div2_clk.clkr,
+ [VIDEO_CC_MVS1_DIV_CLK_SRC] = &video_cc_mvs1_div_clk_src.clkr,
+ [VIDEO_CC_MVS1C_CLK] = &video_cc_mvs1c_clk.clkr,
+ [VIDEO_CC_MVS1C_DIV2_DIV_CLK_SRC] = &video_cc_mvs1c_div2_div_clk_src.clkr,
+ [VIDEO_CC_SLEEP_CLK] = &video_cc_sleep_clk.clkr,
+ [VIDEO_CC_SLEEP_CLK_SRC] = &video_cc_sleep_clk_src.clkr,
+ [VIDEO_CC_XO_CLK_SRC] = &video_cc_xo_clk_src.clkr,
+ [VIDEO_PLL0] = &video_pll0.clkr,
+ [VIDEO_PLL1] = &video_pll1.clkr,
+};
+
+static const struct qcom_reset_map video_cc_sm8350_resets[] = {
+ [CVP_VIDEO_CC_INTERFACE_BCR] = { 0xe54 },
+ [CVP_VIDEO_CC_MVS0_BCR] = { 0xd14 },
+ [VIDEO_CC_MVS0C_CLK_ARES] = { 0xc34, 2 },
+ [CVP_VIDEO_CC_MVS0C_BCR] = { 0xbf4 },
+ [CVP_VIDEO_CC_MVS1_BCR] = { 0xd94 },
+ [VIDEO_CC_MVS1C_CLK_ARES] = { 0xcd4, 2 },
+ [CVP_VIDEO_CC_MVS1C_BCR] = { 0xc94 },
+};
+
+static struct gdsc *video_cc_sm8350_gdscs[] = {
+ [MVS0C_GDSC] = &mvs0c_gdsc,
+ [MVS1C_GDSC] = &mvs1c_gdsc,
+ [MVS0_GDSC] = &mvs0_gdsc,
+ [MVS1_GDSC] = &mvs1_gdsc,
+};
+
+static const struct regmap_config video_cc_sm8350_regmap_config = {
+ .reg_bits = 32,
+ .reg_stride = 4,
+ .val_bits = 32,
+ .max_register = 0x10000,
+ .fast_io = true,
+};
+
+static struct qcom_cc_desc video_cc_sm8350_desc = {
+ .config = &video_cc_sm8350_regmap_config,
+ .clks = video_cc_sm8350_clocks,
+ .num_clks = ARRAY_SIZE(video_cc_sm8350_clocks),
+ .resets = video_cc_sm8350_resets,
+ .num_resets = ARRAY_SIZE(video_cc_sm8350_resets),
+ .gdscs = video_cc_sm8350_gdscs,
+ .num_gdscs = ARRAY_SIZE(video_cc_sm8350_gdscs),
+};
+
+static int video_cc_sm8350_probe(struct platform_device *pdev)
+{
+ struct regmap *regmap;
+ int ret;
+
+ ret = devm_pm_runtime_enable(&pdev->dev);
+ if (ret)
+ return ret;
+
+ ret = pm_runtime_resume_and_get(&pdev->dev);
+ if (ret)
+ return ret;
+
+ regmap = qcom_cc_map(pdev, &video_cc_sm8350_desc);
+ if (IS_ERR(regmap)) {
+ pm_runtime_put(&pdev->dev);
+ return PTR_ERR(regmap);
+ };
+
+ clk_lucid_pll_configure(&video_pll0, regmap, &video_pll0_config);
+ clk_lucid_pll_configure(&video_pll1, regmap, &video_pll1_config);
+
+ /*
+ * Keep clocks always enabled:
+ * video_cc_ahb_clk
+ * video_cc_xo_clk
+ */
+ regmap_update_bits(regmap, 0xe58, BIT(0), BIT(0));
+ regmap_update_bits(regmap, 0xeec, BIT(0), BIT(0));
+
+ ret = qcom_cc_really_probe(pdev, &video_cc_sm8350_desc, regmap);
+ pm_runtime_put(&pdev->dev);
+
+ return ret;
+}
+
+static const struct dev_pm_ops video_cc_sm8350_pm_ops = {
+ SET_RUNTIME_PM_OPS(pm_clk_suspend, pm_clk_resume, NULL)
+};
+
+static const struct of_device_id video_cc_sm8350_match_table[] = {
+ { .compatible = "qcom,sm8350-videocc" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, video_cc_sm8350_match_table);
+
+static struct platform_driver video_cc_sm8350_driver = {
+ .probe = video_cc_sm8350_probe,
+ .driver = {
+ .name = "sm8350-videocc",
+ .of_match_table = video_cc_sm8350_match_table,
+ .pm = &video_cc_sm8350_pm_ops,
+ },
+};
+module_platform_driver(video_cc_sm8350_driver);
+
+MODULE_DESCRIPTION("QTI SM8350 VIDEOCC Driver");
+MODULE_LICENSE("GPL");
--
2.40.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: clock: qcom,videocc: Add SM8350
2023-04-14 11:26 ` [PATCH v2 1/2] dt-bindings: clock: qcom,videocc: Add SM8350 Konrad Dybcio
@ 2023-04-14 15:17 ` Krzysztof Kozlowski
2023-04-14 16:45 ` Dmitry Baryshkov
0 siblings, 1 reply; 12+ messages in thread
From: Krzysztof Kozlowski @ 2023-04-14 15:17 UTC (permalink / raw)
To: Konrad Dybcio, Bjorn Andersson, Andy Gross, Michael Turquette,
Stephen Boyd, Rob Herring, Krzysztof Kozlowski, Philipp Zabel,
Taniya Das
Cc: Marijn Suijten, linux-arm-msm, linux-clk, devicetree, linux-kernel
On 14/04/2023 13:26, Konrad Dybcio wrote:
> SM8350, like most recent higher-end chips has a separate clock
> controller block just for the Venus IP. Document it.
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
> .../devicetree/bindings/clock/qcom,videocc.yaml | 29 +++++++++++++
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] clk: qcom: Introduce SM8350 VIDEOCC
2023-04-14 11:26 ` [PATCH v2 2/2] clk: qcom: Introduce SM8350 VIDEOCC Konrad Dybcio
@ 2023-04-14 16:31 ` Dmitry Baryshkov
2023-04-14 17:48 ` Konrad Dybcio
0 siblings, 1 reply; 12+ messages in thread
From: Dmitry Baryshkov @ 2023-04-14 16:31 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Bjorn Andersson, Andy Gross, Michael Turquette, Stephen Boyd,
Rob Herring, Krzysztof Kozlowski, Philipp Zabel, Taniya Das,
Marijn Suijten, linux-arm-msm, linux-clk, devicetree,
linux-kernel
On Fri, 14 Apr 2023 at 14:26, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> Add support for the Video Clock Controller found on the SM8350 SoC.
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
> drivers/clk/qcom/Kconfig | 9 +
> drivers/clk/qcom/Makefile | 1 +
> drivers/clk/qcom/videocc-sm8350.c | 557 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 567 insertions(+)
>
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index d71c9d6036bb..dbb1dfcddb31 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -916,6 +916,15 @@ config SM_VIDEOCC_8250
> Say Y if you want to support video devices and functionality such as
> video encode and decode.
>
> +config SM_VIDEOCC_8350
> + tristate "SM8350 Video Clock Controller"
> + select SM_GCC_8350
> + select QCOM_GDSC
> + help
> + Support for the video clock controller on SM8350 devices.
> + Say Y if you want to support video devices and functionality such as
> + video encode and decode.
> +
> config SPMI_PMIC_CLKDIV
> tristate "SPMI PMIC clkdiv Support"
> depends on SPMI || COMPILE_TEST
> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> index b54085e579a0..53290040523b 100644
> --- a/drivers/clk/qcom/Makefile
> +++ b/drivers/clk/qcom/Makefile
> @@ -126,6 +126,7 @@ obj-$(CONFIG_SM_GPUCC_8350) += gpucc-sm8350.o
> obj-$(CONFIG_SM_TCSRCC_8550) += tcsrcc-sm8550.o
> obj-$(CONFIG_SM_VIDEOCC_8150) += videocc-sm8150.o
> obj-$(CONFIG_SM_VIDEOCC_8250) += videocc-sm8250.o
> +obj-$(CONFIG_SM_VIDEOCC_8350) += videocc-sm8350.o
> obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o
> obj-$(CONFIG_KPSS_XCC) += kpss-xcc.o
> obj-$(CONFIG_QCOM_HFPLL) += hfpll.o
> diff --git a/drivers/clk/qcom/videocc-sm8350.c b/drivers/clk/qcom/videocc-sm8350.c
> new file mode 100644
> index 000000000000..e0cf474a632d
> --- /dev/null
> +++ b/drivers/clk/qcom/videocc-sm8350.c
> @@ -0,0 +1,557 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2019-2020, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2023, Linaro Limited
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_clock.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +
> +#include <dt-bindings/clock/qcom,sm8350-videocc.h>
> +#include <dt-bindings/reset/qcom,sm8350-videocc.h>
> +
> +#include "clk-alpha-pll.h"
> +#include "clk-branch.h"
> +#include "clk-rcg.h"
> +#include "clk-regmap.h"
> +#include "clk-regmap-divider.h"
> +#include "common.h"
> +#include "reset.h"
> +#include "gdsc.h"
> +
> +enum {
> + DT_BI_TCXO,
> + DT_BI_TCXO_AO,
> + DT_SLEEP_CLK,
> +};
> +
> +enum {
> + P_BI_TCXO,
> + P_BI_TCXO_AO,
> + P_SLEEP_CLK,
> + P_VIDEO_PLL0_OUT_MAIN,
> + P_VIDEO_PLL1_OUT_MAIN,
> +};
> +
> +static const struct pll_vco lucid_5lpe_vco[] = {
> + { 249600000, 1750000000, 0 },
> +};
> +
> +static const struct alpha_pll_config video_pll0_config = {
> + .l = 0x25,
> + .alpha = 0x8000,
> + .config_ctl_val = 0x20485699,
> + .config_ctl_hi_val = 0x00002261,
> + .config_ctl_hi1_val = 0x2a9a699c,
> + .test_ctl_val = 0x00000000,
> + .test_ctl_hi_val = 0x00000000,
> + .test_ctl_hi1_val = 0x01800000,
> + .user_ctl_val = 0x00000000,
> + .user_ctl_hi_val = 0x00000805,
> + .user_ctl_hi1_val = 0x00000000,
> +};
> +
> +static struct clk_alpha_pll video_pll0 = {
> + .offset = 0x42c,
> + .vco_table = lucid_5lpe_vco,
> + .num_vco = ARRAY_SIZE(lucid_5lpe_vco),
> + .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_LUCID],
> + .clkr = {
> + .hw.init = &(const struct clk_init_data) {
> + .name = "video_pll0",
> + .parent_data = &(const struct clk_parent_data){
> + .index = DT_BI_TCXO,
> + },
> + .num_parents = 1,
> + .ops = &clk_alpha_pll_lucid_5lpe_ops,
> + },
> + },
> +};
> +
> +static const struct alpha_pll_config video_pll1_config = {
> + .l = 0x2b,
> + .alpha = 0xc000,
> + .config_ctl_val = 0x20485699,
> + .config_ctl_hi_val = 0x00002261,
> + .config_ctl_hi1_val = 0x2a9a699c,
> + .test_ctl_val = 0x00000000,
> + .test_ctl_hi_val = 0x00000000,
> + .test_ctl_hi1_val = 0x01800000,
> + .user_ctl_val = 0x00000000,
> + .user_ctl_hi_val = 0x00000805,
> + .user_ctl_hi1_val = 0x00000000,
> +};
> +
> +static struct clk_alpha_pll video_pll1 = {
> + .offset = 0x7d0,
> + .vco_table = lucid_5lpe_vco,
> + .num_vco = ARRAY_SIZE(lucid_5lpe_vco),
> + .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_LUCID],
> + .clkr = {
> + .hw.init = &(const struct clk_init_data) {
> + .name = "video_pll1",
> + .parent_data = &(const struct clk_parent_data){
> + .index = DT_BI_TCXO,
> + },
> + .num_parents = 1,
> + .ops = &clk_alpha_pll_lucid_5lpe_ops,
> + },
> + },
> +};
> +
> +static const struct parent_map video_cc_parent_map_0[] = {
> + { P_BI_TCXO_AO, 0 },
> +};
> +
> +static const struct clk_parent_data video_cc_parent_data_0[] = {
> + { .index = DT_BI_TCXO_AO },
> +};
> +
> +static const struct parent_map video_cc_parent_map_1[] = {
> + { P_BI_TCXO, 0 },
> + { P_VIDEO_PLL0_OUT_MAIN, 1 },
> +};
> +
> +static const struct clk_parent_data video_cc_parent_data_1[] = {
> + { .index = DT_BI_TCXO },
> + { .hw = &video_pll0.clkr.hw },
> +};
> +
> +static const struct parent_map video_cc_parent_map_2[] = {
> + { P_BI_TCXO, 0 },
> + { P_VIDEO_PLL1_OUT_MAIN, 1 },
> +};
> +
> +static const struct clk_parent_data video_cc_parent_data_2[] = {
> + { .index = DT_BI_TCXO },
> + { .hw = &video_pll1.clkr.hw },
> +};
> +
> +static const struct freq_tbl ftbl_video_cc_ahb_clk_src[] = {
> + F(19200000, P_BI_TCXO, 1, 0, 0),
> + { }
> +};
> +
> +static struct clk_rcg2 video_cc_ahb_clk_src = {
> + .cmd_rcgr = 0xbd4,
> + .mnd_width = 0,
> + .hid_width = 5,
> + .parent_map = video_cc_parent_map_0,
> + .freq_tbl = ftbl_video_cc_ahb_clk_src,
> + .clkr.hw.init = &(const struct clk_init_data) {
> + .name = "video_cc_ahb_clk_src",
> + .parent_data = video_cc_parent_data_0,
> + .num_parents = ARRAY_SIZE(video_cc_parent_data_0),
> + .flags = CLK_SET_RATE_PARENT,
> + .ops = &clk_rcg2_shared_ops,
> + },
> +};
Do we need this clock at all? We don't have the child
video_cc_ahb_clk, so potentially CCF can try disabling or modifying
this clock.
> +
> +static const struct freq_tbl ftbl_video_cc_mvs0_clk_src[] = {
> + F(720000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0),
> + F(1014000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0),
> + F(1098000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0),
> + F(1332000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0),
> + { }
> +};
> +
> +static struct clk_rcg2 video_cc_mvs0_clk_src = {
> + .cmd_rcgr = 0xb94,
> + .mnd_width = 0,
> + .hid_width = 5,
> + .parent_map = video_cc_parent_map_1,
> + .freq_tbl = ftbl_video_cc_mvs0_clk_src,
> + .clkr.hw.init = &(const struct clk_init_data) {
> + .name = "video_cc_mvs0_clk_src",
> + .parent_data = video_cc_parent_data_1,
> + .num_parents = ARRAY_SIZE(video_cc_parent_data_1),
> + .flags = CLK_SET_RATE_PARENT,
> + .ops = &clk_rcg2_shared_ops,
> + },
> +};
> +
> +static const struct freq_tbl ftbl_video_cc_mvs1_clk_src[] = {
> + F(840000000, P_VIDEO_PLL1_OUT_MAIN, 1, 0, 0),
> + F(1098000000, P_VIDEO_PLL1_OUT_MAIN, 1, 0, 0),
> + F(1332000000, P_VIDEO_PLL1_OUT_MAIN, 1, 0, 0),
> + { }
> +};
> +
> +static struct clk_rcg2 video_cc_mvs1_clk_src = {
> + .cmd_rcgr = 0xbb4,
> + .mnd_width = 0,
> + .hid_width = 5,
> + .parent_map = video_cc_parent_map_2,
> + .freq_tbl = ftbl_video_cc_mvs1_clk_src,
> + .clkr.hw.init = &(const struct clk_init_data) {
> + .name = "video_cc_mvs1_clk_src",
> + .parent_data = video_cc_parent_data_2,
> + .num_parents = ARRAY_SIZE(video_cc_parent_data_2),
> + .flags = CLK_SET_RATE_PARENT,
> + .ops = &clk_rcg2_shared_ops,
> + },
> +};
> +
> +static const struct freq_tbl ftbl_video_cc_sleep_clk_src[] = {
> + F(32000, P_SLEEP_CLK, 1, 0, 0),
> + { }
> +};
> +
> +static struct clk_rcg2 video_cc_sleep_clk_src = {
> + .cmd_rcgr = 0xef0,
> + .mnd_width = 0,
> + .hid_width = 5,
> + .freq_tbl = ftbl_video_cc_sleep_clk_src,
> + .clkr.hw.init = &(const struct clk_init_data) {
> + .name = "video_cc_sleep_clk_src",
> + .parent_data = &(const struct clk_parent_data){
> + .index = DT_SLEEP_CLK,
> + },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT,
> + .ops = &clk_rcg2_ops,
> + },
> +};
> +
> +static struct clk_rcg2 video_cc_xo_clk_src = {
> + .cmd_rcgr = 0xecc,
> + .mnd_width = 0,
> + .hid_width = 5,
> + .parent_map = video_cc_parent_map_0,
> + .freq_tbl = ftbl_video_cc_ahb_clk_src,
> + .clkr.hw.init = &(const struct clk_init_data) {
> + .name = "video_cc_xo_clk_src",
> + .parent_data = video_cc_parent_data_0,
> + .num_parents = ARRAY_SIZE(video_cc_parent_data_0),
> + .flags = CLK_SET_RATE_PARENT,
> + .ops = &clk_rcg2_ops,
> + },
> +};
> +
> +static struct clk_regmap_div video_cc_mvs0_div_clk_src = {
> + .reg = 0xd54,
> + .shift = 0,
> + .width = 4,
> + .clkr.hw.init = &(const struct clk_init_data) {
> + .name = "video_cc_mvs0_div_clk_src",
> + .parent_hws = (const struct clk_hw*[]){
> + &video_cc_mvs0_clk_src.clkr.hw,
> + },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT,
> + .ops = &clk_regmap_div_ro_ops,
> + },
> +};
> +
> +static struct clk_regmap_div video_cc_mvs0c_div2_div_clk_src = {
> + .reg = 0xc54,
> + .shift = 0,
> + .width = 4,
> + .clkr.hw.init = &(const struct clk_init_data) {
> + .name = "video_cc_mvs0c_div2_div_clk_src",
> + .parent_hws = (const struct clk_hw*[]){
> + &video_cc_mvs0_clk_src.clkr.hw,
> + },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT,
> + .ops = &clk_regmap_div_ro_ops,
> + },
> +};
> +
> +static struct clk_regmap_div video_cc_mvs1_div_clk_src = {
> + .reg = 0xdd4,
> + .shift = 0,
> + .width = 4,
> + .clkr.hw.init = &(const struct clk_init_data) {
> + .name = "video_cc_mvs1_div_clk_src",
> + .parent_hws = (const struct clk_hw*[]){
> + &video_cc_mvs1_clk_src.clkr.hw,
> + },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT,
> + .ops = &clk_regmap_div_ro_ops,
> + },
> +};
> +
> +static struct clk_regmap_div video_cc_mvs1c_div2_div_clk_src = {
> + .reg = 0xcf4,
> + .shift = 0,
> + .width = 4,
> + .clkr.hw.init = &(const struct clk_init_data) {
> + .name = "video_cc_mvs1c_div2_div_clk_src",
> + .parent_hws = (const struct clk_hw*[]){
> + &video_cc_mvs1_clk_src.clkr.hw,
> + },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT,
> + .ops = &clk_regmap_div_ro_ops,
> + },
> +};
> +
> +static struct clk_branch video_cc_mvs0_clk = {
> + .halt_reg = 0xd34,
> + .halt_check = BRANCH_HALT_VOTED,
> + .hwcg_reg = 0xd34,
> + .hwcg_bit = 1,
> + .clkr = {
> + .enable_reg = 0xd34,
> + .enable_mask = BIT(0),
> + .hw.init = &(const struct clk_init_data) {
> + .name = "video_cc_mvs0_clk",
> + .parent_hws = (const struct clk_hw*[]){
> + &video_cc_mvs0_div_clk_src.clkr.hw,
> + },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT,
> + .ops = &clk_branch2_ops,
> + },
> + },
> +};
> +
> +static struct clk_branch video_cc_mvs0c_clk = {
> + .halt_reg = 0xc34,
> + .halt_check = BRANCH_HALT,
> + .clkr = {
> + .enable_reg = 0xc34,
> + .enable_mask = BIT(0),
> + .hw.init = &(const struct clk_init_data) {
> + .name = "video_cc_mvs0c_clk",
> + .parent_hws = (const struct clk_hw*[]){
> + &video_cc_mvs0c_div2_div_clk_src.clkr.hw,
> + },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT,
> + .ops = &clk_branch2_ops,
> + },
> + },
> +};
> +
> +static struct clk_branch video_cc_mvs1_clk = {
> + .halt_reg = 0xdb4,
> + .halt_check = BRANCH_HALT_VOTED,
As a note, sm8250 has BRANCH_HALT here.
> + .hwcg_reg = 0xdb4,
> + .hwcg_bit = 1,
> + .clkr = {
> + .enable_reg = 0xdb4,
> + .enable_mask = BIT(0),
> + .hw.init = &(const struct clk_init_data) {
> + .name = "video_cc_mvs1_clk",
> + .parent_hws = (const struct clk_hw*[]){
> + &video_cc_mvs1_div_clk_src.clkr.hw,
> + },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT,
> + .ops = &clk_branch2_ops,
> + },
> + },
> +};
> +
> +static struct clk_branch video_cc_mvs1_div2_clk = {
> + .halt_reg = 0xdf4,
> + .halt_check = BRANCH_HALT_VOTED,
> + .hwcg_reg = 0xdf4,
> + .hwcg_bit = 1,
> + .clkr = {
> + .enable_reg = 0xdf4,
> + .enable_mask = BIT(0),
> + .hw.init = &(const struct clk_init_data) {
> + .name = "video_cc_mvs1_div2_clk",
> + .parent_hws = (const struct clk_hw*[]){
> + &video_cc_mvs1c_div2_div_clk_src.clkr.hw,
> + },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT,
> + .ops = &clk_branch2_ops,
> + },
> + },
> +};
> +
> +static struct clk_branch video_cc_mvs1c_clk = {
> + .halt_reg = 0xcd4,
> + .halt_check = BRANCH_HALT,
> + .clkr = {
> + .enable_reg = 0xcd4,
> + .enable_mask = BIT(0),
> + .hw.init = &(const struct clk_init_data) {
> + .name = "video_cc_mvs1c_clk",
> + .parent_hws = (const struct clk_hw*[]){
> + &video_cc_mvs1c_div2_div_clk_src.clkr.hw,
> + },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT,
> + .ops = &clk_branch2_ops,
> + },
> + },
> +};
> +
> +static struct clk_branch video_cc_sleep_clk = {
> + .halt_reg = 0xf10,
> + .halt_check = BRANCH_HALT,
> + .clkr = {
> + .enable_reg = 0xf10,
> + .enable_mask = BIT(0),
> + .hw.init = &(const struct clk_init_data) {
> + .name = "video_cc_sleep_clk",
> + .parent_hws = (const struct clk_hw*[]){
> + &video_cc_sleep_clk_src.clkr.hw,
> + },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT,
> + .ops = &clk_branch2_ops,
> + },
> + },
> +};
> +
> +static struct gdsc mvs0c_gdsc = {
> + .gdscr = 0xbf8,
> + .pd = {
> + .name = "mvs0c_gdsc",
> + },
> + .flags = RETAIN_FF_ENABLE,
> + .pwrsts = PWRSTS_OFF_ON,
> +};
> +
> +static struct gdsc mvs1c_gdsc = {
> + .gdscr = 0xc98,
> + .pd = {
> + .name = "mvs1c_gdsc",
> + },
> + .flags = RETAIN_FF_ENABLE,
> + .pwrsts = PWRSTS_OFF_ON,
> +};
> +
> +static struct gdsc mvs0_gdsc = {
> + .gdscr = 0xd18,
> + .pd = {
> + .name = "mvs0_gdsc",
> + },
> + .flags = HW_CTRL | RETAIN_FF_ENABLE,
> + .pwrsts = PWRSTS_OFF_ON,
> +};
> +
> +static struct gdsc mvs1_gdsc = {
> + .gdscr = 0xd98,
> + .pd = {
> + .name = "mvs1_gdsc",
> + },
> + .flags = HW_CTRL | RETAIN_FF_ENABLE,
> + .pwrsts = PWRSTS_OFF_ON,
> +};
> +
> +static struct clk_regmap *video_cc_sm8350_clocks[] = {
> + [VIDEO_CC_AHB_CLK_SRC] = &video_cc_ahb_clk_src.clkr,
> + [VIDEO_CC_MVS0_CLK] = &video_cc_mvs0_clk.clkr,
> + [VIDEO_CC_MVS0_CLK_SRC] = &video_cc_mvs0_clk_src.clkr,
> + [VIDEO_CC_MVS0_DIV_CLK_SRC] = &video_cc_mvs0_div_clk_src.clkr,
> + [VIDEO_CC_MVS0C_CLK] = &video_cc_mvs0c_clk.clkr,
> + [VIDEO_CC_MVS0C_DIV2_DIV_CLK_SRC] = &video_cc_mvs0c_div2_div_clk_src.clkr,
> + [VIDEO_CC_MVS1_CLK] = &video_cc_mvs1_clk.clkr,
> + [VIDEO_CC_MVS1_CLK_SRC] = &video_cc_mvs1_clk_src.clkr,
> + [VIDEO_CC_MVS1_DIV2_CLK] = &video_cc_mvs1_div2_clk.clkr,
> + [VIDEO_CC_MVS1_DIV_CLK_SRC] = &video_cc_mvs1_div_clk_src.clkr,
> + [VIDEO_CC_MVS1C_CLK] = &video_cc_mvs1c_clk.clkr,
> + [VIDEO_CC_MVS1C_DIV2_DIV_CLK_SRC] = &video_cc_mvs1c_div2_div_clk_src.clkr,
> + [VIDEO_CC_SLEEP_CLK] = &video_cc_sleep_clk.clkr,
> + [VIDEO_CC_SLEEP_CLK_SRC] = &video_cc_sleep_clk_src.clkr,
> + [VIDEO_CC_XO_CLK_SRC] = &video_cc_xo_clk_src.clkr,
> + [VIDEO_PLL0] = &video_pll0.clkr,
> + [VIDEO_PLL1] = &video_pll1.clkr,
> +};
> +
> +static const struct qcom_reset_map video_cc_sm8350_resets[] = {
> + [CVP_VIDEO_CC_INTERFACE_BCR] = { 0xe54 },
> + [CVP_VIDEO_CC_MVS0_BCR] = { 0xd14 },
Would it be better to use common VIDEO_CC prefix here (IOW:
VIDEO_CC_CVP_MVS0_BCR, VIDEO_CC_CVP_INTERFACE_BCR), etc.
> + [VIDEO_CC_MVS0C_CLK_ARES] = { 0xc34, 2 },
> + [CVP_VIDEO_CC_MVS0C_BCR] = { 0xbf4 },
> + [CVP_VIDEO_CC_MVS1_BCR] = { 0xd94 },
> + [VIDEO_CC_MVS1C_CLK_ARES] = { 0xcd4, 2 },
> + [CVP_VIDEO_CC_MVS1C_BCR] = { 0xc94 },
> +};
> +
> +static struct gdsc *video_cc_sm8350_gdscs[] = {
> + [MVS0C_GDSC] = &mvs0c_gdsc,
> + [MVS1C_GDSC] = &mvs1c_gdsc,
> + [MVS0_GDSC] = &mvs0_gdsc,
> + [MVS1_GDSC] = &mvs1_gdsc,
> +};
> +
> +static const struct regmap_config video_cc_sm8350_regmap_config = {
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> + .max_register = 0x10000,
> + .fast_io = true,
> +};
> +
> +static struct qcom_cc_desc video_cc_sm8350_desc = {
> + .config = &video_cc_sm8350_regmap_config,
> + .clks = video_cc_sm8350_clocks,
> + .num_clks = ARRAY_SIZE(video_cc_sm8350_clocks),
> + .resets = video_cc_sm8350_resets,
> + .num_resets = ARRAY_SIZE(video_cc_sm8350_resets),
> + .gdscs = video_cc_sm8350_gdscs,
> + .num_gdscs = ARRAY_SIZE(video_cc_sm8350_gdscs),
> +};
> +
> +static int video_cc_sm8350_probe(struct platform_device *pdev)
> +{
> + struct regmap *regmap;
> + int ret;
> +
> + ret = devm_pm_runtime_enable(&pdev->dev);
> + if (ret)
> + return ret;
> +
> + ret = pm_runtime_resume_and_get(&pdev->dev);
> + if (ret)
> + return ret;
> +
> + regmap = qcom_cc_map(pdev, &video_cc_sm8350_desc);
> + if (IS_ERR(regmap)) {
> + pm_runtime_put(&pdev->dev);
> + return PTR_ERR(regmap);
> + };
Extra semicolon
> +
> + clk_lucid_pll_configure(&video_pll0, regmap, &video_pll0_config);
> + clk_lucid_pll_configure(&video_pll1, regmap, &video_pll1_config);
> +
> + /*
> + * Keep clocks always enabled:
> + * video_cc_ahb_clk
> + * video_cc_xo_clk
> + */
> + regmap_update_bits(regmap, 0xe58, BIT(0), BIT(0));
> + regmap_update_bits(regmap, 0xeec, BIT(0), BIT(0));
> +
> + ret = qcom_cc_really_probe(pdev, &video_cc_sm8350_desc, regmap);
> + pm_runtime_put(&pdev->dev);
> +
> + return ret;
> +}
> +
> +static const struct dev_pm_ops video_cc_sm8350_pm_ops = {
> + SET_RUNTIME_PM_OPS(pm_clk_suspend, pm_clk_resume, NULL)
The driver doesn't use pm_clk at all. Are these PM_OPS correct?
> +};
> +
> +static const struct of_device_id video_cc_sm8350_match_table[] = {
> + { .compatible = "qcom,sm8350-videocc" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, video_cc_sm8350_match_table);
> +
> +static struct platform_driver video_cc_sm8350_driver = {
> + .probe = video_cc_sm8350_probe,
> + .driver = {
> + .name = "sm8350-videocc",
> + .of_match_table = video_cc_sm8350_match_table,
> + .pm = &video_cc_sm8350_pm_ops,
> + },
> +};
> +module_platform_driver(video_cc_sm8350_driver);
> +
> +MODULE_DESCRIPTION("QTI SM8350 VIDEOCC Driver");
> +MODULE_LICENSE("GPL");
>
> --
> 2.40.0
>
Generic note: the register layout follows closely sm8250. However the
existing differences probably do not warrant merging them.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: clock: qcom,videocc: Add SM8350
2023-04-14 15:17 ` Krzysztof Kozlowski
@ 2023-04-14 16:45 ` Dmitry Baryshkov
2023-04-14 20:32 ` Konrad Dybcio
2023-04-14 20:58 ` Krzysztof Kozlowski
0 siblings, 2 replies; 12+ messages in thread
From: Dmitry Baryshkov @ 2023-04-14 16:45 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Konrad Dybcio, Bjorn Andersson, Andy Gross, Michael Turquette,
Stephen Boyd, Rob Herring, Krzysztof Kozlowski, Philipp Zabel,
Taniya Das, Marijn Suijten, linux-arm-msm, linux-clk, devicetree,
linux-kernel
On Fri, 14 Apr 2023 at 18:18, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 14/04/2023 13:26, Konrad Dybcio wrote:
> > SM8350, like most recent higher-end chips has a separate clock
> > controller block just for the Venus IP. Document it.
> >
> > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> > ---
> > .../devicetree/bindings/clock/qcom,videocc.yaml | 29 +++++++++++++
>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Krzysztof, Konrad, would it make sense to split it into separate
bindings? After all, previous videocc bindings used clock-names, while
this one doesn't.
>
> Best regards,
> Krzysztof
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] clk: qcom: Introduce SM8350 VIDEOCC
2023-04-14 16:31 ` Dmitry Baryshkov
@ 2023-04-14 17:48 ` Konrad Dybcio
2023-04-14 20:54 ` Dmitry Baryshkov
0 siblings, 1 reply; 12+ messages in thread
From: Konrad Dybcio @ 2023-04-14 17:48 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Bjorn Andersson, Andy Gross, Michael Turquette, Stephen Boyd,
Rob Herring, Krzysztof Kozlowski, Philipp Zabel, Taniya Das,
Marijn Suijten, linux-arm-msm, linux-clk, devicetree,
linux-kernel
On 14.04.2023 18:31, Dmitry Baryshkov wrote:
> On Fri, 14 Apr 2023 at 14:26, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>> Add support for the Video Clock Controller found on the SM8350 SoC.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
[...]
>> +static struct clk_rcg2 video_cc_ahb_clk_src = {
>> + .cmd_rcgr = 0xbd4,
>> + .mnd_width = 0,
>> + .hid_width = 5,
>> + .parent_map = video_cc_parent_map_0,
>> + .freq_tbl = ftbl_video_cc_ahb_clk_src,
>> + .clkr.hw.init = &(const struct clk_init_data) {
>> + .name = "video_cc_ahb_clk_src",
>> + .parent_data = video_cc_parent_data_0,
>> + .num_parents = ARRAY_SIZE(video_cc_parent_data_0),
>> + .flags = CLK_SET_RATE_PARENT,
>> + .ops = &clk_rcg2_shared_ops,
>> + },
>> +};
>
> Do we need this clock at all? We don't have the child
> video_cc_ahb_clk, so potentially CCF can try disabling or modifying
> this clock.
Hm.. I see a few things:
1. downstream kona has it, upstream does not
2. it's shared so we never actually hard-shut it off..
2a. ..but it'd be good to ensure it's on when it's ready..
2b. ..but we never do anyway..
2c. ..but should we even? doesn't Venus govern it internally?
>
>> +
>> +static const struct freq_tbl ftbl_video_cc_mvs0_clk_src[] = {
>> + F(720000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0),
>> + F(1014000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0),
>> + F(1098000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0),
>> + F(1332000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0),
>> + { }
>> +};
>> +
[...]
>> +static struct clk_branch video_cc_mvs1_clk = {
>> + .halt_reg = 0xdb4,
>> + .halt_check = BRANCH_HALT_VOTED,
>
> As a note, sm8250 has BRANCH_HALT here.
No, it does on the div2 clk, and so do we:
[...]
>> +};
>> +
>> +static struct clk_branch video_cc_mvs1_div2_clk = {
>> + .halt_reg = 0xdf4,
>> + .halt_check = BRANCH_HALT_VOTED,
>> + .hwcg_reg = 0xdf4,
[...]
>> +
>> +static const struct qcom_reset_map video_cc_sm8350_resets[] = {
>> + [CVP_VIDEO_CC_INTERFACE_BCR] = { 0xe54 },
>> + [CVP_VIDEO_CC_MVS0_BCR] = { 0xd14 },
>
> Would it be better to use common VIDEO_CC prefix here (IOW:
> VIDEO_CC_CVP_MVS0_BCR, VIDEO_CC_CVP_INTERFACE_BCR), etc.
My best guess would be that the ones prefixed with CVP_
are actual INTF/INSTANCEn(CORE) reset lines whereas
the ones containing _CLK_ reset their clock sub-branches.
>
>> + [VIDEO_CC_MVS0C_CLK_ARES] = { 0xc34, 2 },
>> + [CVP_VIDEO_CC_MVS0C_BCR] = { 0xbf4 },
>> + [CVP_VIDEO_CC_MVS1_BCR] = { 0xd94 },
>> + [VIDEO_CC_MVS1C_CLK_ARES] = { 0xcd4, 2 },
>> + [CVP_VIDEO_CC_MVS1C_BCR] = { 0xc94 },
>> +};
[...]
>> + ret = pm_runtime_resume_and_get(&pdev->dev);
>> + if (ret)
>> + return ret;
>> +
>> + regmap = qcom_cc_map(pdev, &video_cc_sm8350_desc);
>> + if (IS_ERR(regmap)) {
>> + pm_runtime_put(&pdev->dev);
>> + return PTR_ERR(regmap);
>> + };
>
> Extra semicolon
Ooeh!
>
>> +
>> + clk_lucid_pll_configure(&video_pll0, regmap, &video_pll0_config);
>> + clk_lucid_pll_configure(&video_pll1, regmap, &video_pll1_config);
>> +
>> + /*
>> + * Keep clocks always enabled:
>> + * video_cc_ahb_clk
>> + * video_cc_xo_clk
>> + */
>> + regmap_update_bits(regmap, 0xe58, BIT(0), BIT(0));
>> + regmap_update_bits(regmap, 0xeec, BIT(0), BIT(0));
>> +
>> + ret = qcom_cc_really_probe(pdev, &video_cc_sm8350_desc, regmap);
>> + pm_runtime_put(&pdev->dev);
>> +
>> + return ret;
>> +}
>> +
>> +static const struct dev_pm_ops video_cc_sm8350_pm_ops = {
>> + SET_RUNTIME_PM_OPS(pm_clk_suspend, pm_clk_resume, NULL)
>
> The driver doesn't use pm_clk at all. Are these PM_OPS correct?
I'm unsure. I see the pm state changing in debugfs when the clocks are
(not) consumed. But let's continue our discussion about using pm_clks
for AHB.
>
>> +};
>> +
>> +static const struct of_device_id video_cc_sm8350_match_table[] = {
>> + { .compatible = "qcom,sm8350-videocc" },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(of, video_cc_sm8350_match_table);
>> +
>> +static struct platform_driver video_cc_sm8350_driver = {
>> + .probe = video_cc_sm8350_probe,
>> + .driver = {
>> + .name = "sm8350-videocc",
>> + .of_match_table = video_cc_sm8350_match_table,
>> + .pm = &video_cc_sm8350_pm_ops,
>> + },
>> +};
>> +module_platform_driver(video_cc_sm8350_driver);
>> +
>> +MODULE_DESCRIPTION("QTI SM8350 VIDEOCC Driver");
>> +MODULE_LICENSE("GPL");
>>
>> --
>> 2.40.0
>>
>
> Generic note: the register layout follows closely sm8250. However the
> existing differences probably do not warrant merging them.
No, I don't think merging any designs that are farther away
than 8150 and 8155 or 8992 and 8994 etc. is a good idea..
I don't want to ever look at something like dispcc-sm8[123]50.c
again!
Konrad
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: clock: qcom,videocc: Add SM8350
2023-04-14 16:45 ` Dmitry Baryshkov
@ 2023-04-14 20:32 ` Konrad Dybcio
2023-04-14 20:58 ` Krzysztof Kozlowski
1 sibling, 0 replies; 12+ messages in thread
From: Konrad Dybcio @ 2023-04-14 20:32 UTC (permalink / raw)
To: Dmitry Baryshkov, Krzysztof Kozlowski
Cc: Bjorn Andersson, Andy Gross, Michael Turquette, Stephen Boyd,
Rob Herring, Krzysztof Kozlowski, Philipp Zabel, Taniya Das,
Marijn Suijten, linux-arm-msm, linux-clk, devicetree,
linux-kernel
On 14.04.2023 18:45, Dmitry Baryshkov wrote:
> On Fri, 14 Apr 2023 at 18:18, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 14/04/2023 13:26, Konrad Dybcio wrote:
>>> SM8350, like most recent higher-end chips has a separate clock
>>> controller block just for the Venus IP. Document it.
>>>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
>>> .../devicetree/bindings/clock/qcom,videocc.yaml | 29 +++++++++++++
>>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
> Krzysztof, Konrad, would it make sense to split it into separate
> bindings? After all, previous videocc bindings used clock-names, while
> this one doesn't.
I'm fine with either of these. Your call, Krzysztof.
Konrad
>
>>
>> Best regards,
>> Krzysztof
>>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] clk: qcom: Introduce SM8350 VIDEOCC
2023-04-14 17:48 ` Konrad Dybcio
@ 2023-04-14 20:54 ` Dmitry Baryshkov
2023-04-17 18:10 ` Konrad Dybcio
0 siblings, 1 reply; 12+ messages in thread
From: Dmitry Baryshkov @ 2023-04-14 20:54 UTC (permalink / raw)
To: Konrad Dybcio, Taniya Das
Cc: Bjorn Andersson, Andy Gross, Michael Turquette, Stephen Boyd,
Rob Herring, Krzysztof Kozlowski, Philipp Zabel, Taniya Das,
Marijn Suijten, linux-arm-msm, linux-clk, devicetree,
linux-kernel
On Fri, 14 Apr 2023 at 20:48, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
>
>
> On 14.04.2023 18:31, Dmitry Baryshkov wrote:
> > On Fri, 14 Apr 2023 at 14:26, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> >>
> >> Add support for the Video Clock Controller found on the SM8350 SoC.
> >>
> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> >> ---
>
> [...]
>
> >> +static struct clk_rcg2 video_cc_ahb_clk_src = {
> >> + .cmd_rcgr = 0xbd4,
> >> + .mnd_width = 0,
> >> + .hid_width = 5,
> >> + .parent_map = video_cc_parent_map_0,
> >> + .freq_tbl = ftbl_video_cc_ahb_clk_src,
> >> + .clkr.hw.init = &(const struct clk_init_data) {
> >> + .name = "video_cc_ahb_clk_src",
> >> + .parent_data = video_cc_parent_data_0,
> >> + .num_parents = ARRAY_SIZE(video_cc_parent_data_0),
> >> + .flags = CLK_SET_RATE_PARENT,
> >> + .ops = &clk_rcg2_shared_ops,
> >> + },
> >> +};
> >
> > Do we need this clock at all? We don't have the child
> > video_cc_ahb_clk, so potentially CCF can try disabling or modifying
> > this clock.
> Hm.. I see a few things:
>
> 1. downstream kona has it, upstream does not
> 2. it's shared so we never actually hard-shut it off..
> 2a. ..but it'd be good to ensure it's on when it's ready..
> 2b. ..but we never do anyway..
> 2c. ..but should we even? doesn't Venus govern it internally?
>
>
> >
> >> +
> >> +static const struct freq_tbl ftbl_video_cc_mvs0_clk_src[] = {
> >> + F(720000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0),
> >> + F(1014000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0),
> >> + F(1098000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0),
> >> + F(1332000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0),
> >> + { }
> >> +};
> >> +
>
> [...]
>
> >> +static struct clk_branch video_cc_mvs1_clk = {
> >> + .halt_reg = 0xdb4,
> >> + .halt_check = BRANCH_HALT_VOTED,
> >
> > As a note, sm8250 has BRANCH_HALT here.
> No, it does on the div2 clk, and so do we:
Excuse me, I got confused by all the syllables. I was looking at the
video_cc_mvs1c_clk. On sm8250 it is _VOTED, in this patch it is not. I
can not say that either one of those is incorrect, but such a
difference looks a bit suspicious for me. Maybe Tanya or somebody else
can comment here.
> [...]
>
> >> +};
> >> +
> >> +static struct clk_branch video_cc_mvs1_div2_clk = {
> >> + .halt_reg = 0xdf4,
> >> + .halt_check = BRANCH_HALT_VOTED,
> >> + .hwcg_reg = 0xdf4,
>
> [...]
>
> >> +
> >> +static const struct qcom_reset_map video_cc_sm8350_resets[] = {
> >> + [CVP_VIDEO_CC_INTERFACE_BCR] = { 0xe54 },
> >> + [CVP_VIDEO_CC_MVS0_BCR] = { 0xd14 },
> >
> > Would it be better to use common VIDEO_CC prefix here (IOW:
> > VIDEO_CC_CVP_MVS0_BCR, VIDEO_CC_CVP_INTERFACE_BCR), etc.
> My best guess would be that the ones prefixed with CVP_
> are actual INTF/INSTANCEn(CORE) reset lines whereas
> the ones containing _CLK_ reset their clock sub-branches.
Note, again, on sm8250 all resets start with VIDEO_CC, even CVP ones.
I think we can follow that.
>
> >
> >> + [VIDEO_CC_MVS0C_CLK_ARES] = { 0xc34, 2 },
> >> + [CVP_VIDEO_CC_MVS0C_BCR] = { 0xbf4 },
> >> + [CVP_VIDEO_CC_MVS1_BCR] = { 0xd94 },
> >> + [VIDEO_CC_MVS1C_CLK_ARES] = { 0xcd4, 2 },
> >> + [CVP_VIDEO_CC_MVS1C_BCR] = { 0xc94 },
> >> +};
>
> [...]
>
> >> + ret = pm_runtime_resume_and_get(&pdev->dev);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + regmap = qcom_cc_map(pdev, &video_cc_sm8350_desc);
> >> + if (IS_ERR(regmap)) {
> >> + pm_runtime_put(&pdev->dev);
> >> + return PTR_ERR(regmap);
> >> + };
> >
> > Extra semicolon
> Ooeh!
>
> >
> >> +
> >> + clk_lucid_pll_configure(&video_pll0, regmap, &video_pll0_config);
> >> + clk_lucid_pll_configure(&video_pll1, regmap, &video_pll1_config);
> >> +
> >> + /*
> >> + * Keep clocks always enabled:
> >> + * video_cc_ahb_clk
> >> + * video_cc_xo_clk
> >> + */
> >> + regmap_update_bits(regmap, 0xe58, BIT(0), BIT(0));
> >> + regmap_update_bits(regmap, 0xeec, BIT(0), BIT(0));
> >> +
> >> + ret = qcom_cc_really_probe(pdev, &video_cc_sm8350_desc, regmap);
> >> + pm_runtime_put(&pdev->dev);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static const struct dev_pm_ops video_cc_sm8350_pm_ops = {
> >> + SET_RUNTIME_PM_OPS(pm_clk_suspend, pm_clk_resume, NULL)
> >
> > The driver doesn't use pm_clk at all. Are these PM_OPS correct?
> I'm unsure. I see the pm state changing in debugfs when the clocks are
> (not) consumed. But let's continue our discussion about using pm_clks
> for AHB.
Well, those are two separate questions. One is that w/o additional
pm_clk calls this string is useless (and should be removed). Another
on is a possible restructure of our cc drivers to use pm_clk for AHB
clocks (which would require adding more than that).
>
> >
> >> +};
> >> +
> >> +static const struct of_device_id video_cc_sm8350_match_table[] = {
> >> + { .compatible = "qcom,sm8350-videocc" },
> >> + { }
> >> +};
> >> +MODULE_DEVICE_TABLE(of, video_cc_sm8350_match_table);
> >> +
> >> +static struct platform_driver video_cc_sm8350_driver = {
> >> + .probe = video_cc_sm8350_probe,
> >> + .driver = {
> >> + .name = "sm8350-videocc",
> >> + .of_match_table = video_cc_sm8350_match_table,
> >> + .pm = &video_cc_sm8350_pm_ops,
> >> + },
> >> +};
> >> +module_platform_driver(video_cc_sm8350_driver);
> >> +
> >> +MODULE_DESCRIPTION("QTI SM8350 VIDEOCC Driver");
> >> +MODULE_LICENSE("GPL");
> >>
> >> --
> >> 2.40.0
> >>
> >
> > Generic note: the register layout follows closely sm8250. However the
> > existing differences probably do not warrant merging them.
> No, I don't think merging any designs that are farther away
> than 8150 and 8155 or 8992 and 8994 etc. is a good idea..
>
> I don't want to ever look at something like dispcc-sm8[123]50.c
> again!
Me too!
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: clock: qcom,videocc: Add SM8350
2023-04-14 16:45 ` Dmitry Baryshkov
2023-04-14 20:32 ` Konrad Dybcio
@ 2023-04-14 20:58 ` Krzysztof Kozlowski
1 sibling, 0 replies; 12+ messages in thread
From: Krzysztof Kozlowski @ 2023-04-14 20:58 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Konrad Dybcio, Bjorn Andersson, Andy Gross, Michael Turquette,
Stephen Boyd, Rob Herring, Krzysztof Kozlowski, Philipp Zabel,
Taniya Das, Marijn Suijten, linux-arm-msm, linux-clk, devicetree,
linux-kernel
On 14/04/2023 18:45, Dmitry Baryshkov wrote:
> On Fri, 14 Apr 2023 at 18:18, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 14/04/2023 13:26, Konrad Dybcio wrote:
>>> SM8350, like most recent higher-end chips has a separate clock
>>> controller block just for the Venus IP. Document it.
>>>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
>>> .../devicetree/bindings/clock/qcom,videocc.yaml | 29 +++++++++++++
>>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
> Krzysztof, Konrad, would it make sense to split it into separate
> bindings? After all, previous videocc bindings used clock-names, while
> this one doesn't.
Yes, makes sense. Otherwise I am afraid allOf:if:then will keep growing...
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] clk: qcom: Introduce SM8350 VIDEOCC
2023-04-14 20:54 ` Dmitry Baryshkov
@ 2023-04-17 18:10 ` Konrad Dybcio
2023-04-17 18:41 ` Dmitry Baryshkov
0 siblings, 1 reply; 12+ messages in thread
From: Konrad Dybcio @ 2023-04-17 18:10 UTC (permalink / raw)
To: Dmitry Baryshkov, Taniya Das
Cc: Bjorn Andersson, Andy Gross, Michael Turquette, Stephen Boyd,
Rob Herring, Krzysztof Kozlowski, Philipp Zabel, Taniya Das,
Marijn Suijten, linux-arm-msm, linux-clk, devicetree,
linux-kernel
On 14.04.2023 22:54, Dmitry Baryshkov wrote:
> On Fri, 14 Apr 2023 at 20:48, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>>
>>
>> On 14.04.2023 18:31, Dmitry Baryshkov wrote:
>>> On Fri, 14 Apr 2023 at 14:26, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>>>
>>>> Add support for the Video Clock Controller found on the SM8350 SoC.
>>>>
>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>> ---
>>
>> [...]
>>
>>>> +static struct clk_rcg2 video_cc_ahb_clk_src = {
>>>> + .cmd_rcgr = 0xbd4,
>>>> + .mnd_width = 0,
>>>> + .hid_width = 5,
>>>> + .parent_map = video_cc_parent_map_0,
>>>> + .freq_tbl = ftbl_video_cc_ahb_clk_src,
>>>> + .clkr.hw.init = &(const struct clk_init_data) {
>>>> + .name = "video_cc_ahb_clk_src",
>>>> + .parent_data = video_cc_parent_data_0,
>>>> + .num_parents = ARRAY_SIZE(video_cc_parent_data_0),
>>>> + .flags = CLK_SET_RATE_PARENT,
>>>> + .ops = &clk_rcg2_shared_ops,
>>>> + },
>>>> +};
>>>
>>> Do we need this clock at all? We don't have the child
>>> video_cc_ahb_clk, so potentially CCF can try disabling or modifying
>>> this clock.
>> Hm.. I see a few things:
>>
>> 1. downstream kona has it, upstream does not
>> 2. it's shared so we never actually hard-shut it off..
>> 2a. ..but it'd be good to ensure it's on when it's ready..
>> 2b. ..but we never do anyway..
>> 2c. ..but should we even? doesn't Venus govern it internally?
>>
>>
>>>
>>>> +
>>>> +static const struct freq_tbl ftbl_video_cc_mvs0_clk_src[] = {
>>>> + F(720000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0),
>>>> + F(1014000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0),
>>>> + F(1098000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0),
>>>> + F(1332000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0),
>>>> + { }
>>>> +};
>>>> +
>>
>> [...]
>>
>>>> +static struct clk_branch video_cc_mvs1_clk = {
>>>> + .halt_reg = 0xdb4,
>>>> + .halt_check = BRANCH_HALT_VOTED,
>>>
>>> As a note, sm8250 has BRANCH_HALT here.
>> No, it does on the div2 clk, and so do we:
>
> Excuse me, I got confused by all the syllables. I was looking at the
> video_cc_mvs1c_clk. On sm8250 it is _VOTED, in this patch it is not. I
> can not say that either one of those is incorrect, but such a
> difference looks a bit suspicious for me. Maybe Tanya or somebody else
> can comment here.
I'd say this could be a design decision, with div2 clocks being
treated differently, but it's how downstream does it on shipping
devices and while generally it's not a great thing to say, it seems
to be the "right enough" thing..
>
>> [...]
>>
>>>> +};
>>>> +
>>>> +static struct clk_branch video_cc_mvs1_div2_clk = {
>>>> + .halt_reg = 0xdf4,
>>>> + .halt_check = BRANCH_HALT_VOTED,
>>>> + .hwcg_reg = 0xdf4,
>>
>> [...]
>>
>>>> +
>>>> +static const struct qcom_reset_map video_cc_sm8350_resets[] = {
>>>> + [CVP_VIDEO_CC_INTERFACE_BCR] = { 0xe54 },
>>>> + [CVP_VIDEO_CC_MVS0_BCR] = { 0xd14 },
>>>
>>> Would it be better to use common VIDEO_CC prefix here (IOW:
>>> VIDEO_CC_CVP_MVS0_BCR, VIDEO_CC_CVP_INTERFACE_BCR), etc.
>> My best guess would be that the ones prefixed with CVP_
>> are actual INTF/INSTANCEn(CORE) reset lines whereas
>> the ones containing _CLK_ reset their clock sub-branches.
>
> Note, again, on sm8250 all resets start with VIDEO_CC, even CVP ones.
> I think we can follow that.
Or get rid of that, as it's always called with a phandle to videocc..
Thoughts?
>
>>
>>>
>>>> + [VIDEO_CC_MVS0C_CLK_ARES] = { 0xc34, 2 },
>>>> + [CVP_VIDEO_CC_MVS0C_BCR] = { 0xbf4 },
>>>> + [CVP_VIDEO_CC_MVS1_BCR] = { 0xd94 },
>>>> + [VIDEO_CC_MVS1C_CLK_ARES] = { 0xcd4, 2 },
>>>> + [CVP_VIDEO_CC_MVS1C_BCR] = { 0xc94 },
>>>> +};
>>
>> [...]
>>
>>>> + ret = pm_runtime_resume_and_get(&pdev->dev);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + regmap = qcom_cc_map(pdev, &video_cc_sm8350_desc);
>>>> + if (IS_ERR(regmap)) {
>>>> + pm_runtime_put(&pdev->dev);
>>>> + return PTR_ERR(regmap);
>>>> + };
>>>
>>> Extra semicolon
>> Ooeh!
>>
>>>
>>>> +
>>>> + clk_lucid_pll_configure(&video_pll0, regmap, &video_pll0_config);
>>>> + clk_lucid_pll_configure(&video_pll1, regmap, &video_pll1_config);
>>>> +
>>>> + /*
>>>> + * Keep clocks always enabled:
>>>> + * video_cc_ahb_clk
>>>> + * video_cc_xo_clk
>>>> + */
>>>> + regmap_update_bits(regmap, 0xe58, BIT(0), BIT(0));
>>>> + regmap_update_bits(regmap, 0xeec, BIT(0), BIT(0));
>>>> +
>>>> + ret = qcom_cc_really_probe(pdev, &video_cc_sm8350_desc, regmap);
>>>> + pm_runtime_put(&pdev->dev);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static const struct dev_pm_ops video_cc_sm8350_pm_ops = {
>>>> + SET_RUNTIME_PM_OPS(pm_clk_suspend, pm_clk_resume, NULL)
>>>
>>> The driver doesn't use pm_clk at all. Are these PM_OPS correct?
>> I'm unsure. I see the pm state changing in debugfs when the clocks are
>> (not) consumed. But let's continue our discussion about using pm_clks
>> for AHB.
>
> Well, those are two separate questions. One is that w/o additional
> pm_clk calls this string is useless (and should be removed). Another
> on is a possible restructure of our cc drivers to use pm_clk for AHB
> clocks (which would require adding more than that).
Right, I had an impression that you needed any sort of pm ops at
all to be registered with pm_genpd correctly, but that seems not to
be the case.. With that commented out, I still see "suspended" / "active"
and not "unsupported"..
Konrad
>
>
>>
>>>
>>>> +};
>>>> +
>>>> +static const struct of_device_id video_cc_sm8350_match_table[] = {
>>>> + { .compatible = "qcom,sm8350-videocc" },
>>>> + { }
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, video_cc_sm8350_match_table);
>>>> +
>>>> +static struct platform_driver video_cc_sm8350_driver = {
>>>> + .probe = video_cc_sm8350_probe,
>>>> + .driver = {
>>>> + .name = "sm8350-videocc",
>>>> + .of_match_table = video_cc_sm8350_match_table,
>>>> + .pm = &video_cc_sm8350_pm_ops,
>>>> + },
>>>> +};
>>>> +module_platform_driver(video_cc_sm8350_driver);
>>>> +
>>>> +MODULE_DESCRIPTION("QTI SM8350 VIDEOCC Driver");
>>>> +MODULE_LICENSE("GPL");
>>>>
>>>> --
>>>> 2.40.0
>>>>
>>>
>>> Generic note: the register layout follows closely sm8250. However the
>>> existing differences probably do not warrant merging them.
>> No, I don't think merging any designs that are farther away
>> than 8150 and 8155 or 8992 and 8994 etc. is a good idea..
>>
>> I don't want to ever look at something like dispcc-sm8[123]50.c
>> again!
>
> Me too!
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] clk: qcom: Introduce SM8350 VIDEOCC
2023-04-17 18:10 ` Konrad Dybcio
@ 2023-04-17 18:41 ` Dmitry Baryshkov
0 siblings, 0 replies; 12+ messages in thread
From: Dmitry Baryshkov @ 2023-04-17 18:41 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Taniya Das, Bjorn Andersson, Andy Gross, Michael Turquette,
Stephen Boyd, Rob Herring, Krzysztof Kozlowski, Philipp Zabel,
Marijn Suijten, linux-arm-msm, linux-clk, devicetree,
linux-kernel
On Mon, 17 Apr 2023 at 21:10, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
>
>
> On 14.04.2023 22:54, Dmitry Baryshkov wrote:
> > On Fri, 14 Apr 2023 at 20:48, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> >>
> >>
> >>
> >> On 14.04.2023 18:31, Dmitry Baryshkov wrote:
> >>> On Fri, 14 Apr 2023 at 14:26, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> >>>>
> >>>> Add support for the Video Clock Controller found on the SM8350 SoC.
> >>>>
> >>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> >>>> ---
> >>
> >> [...]
> >>
> >>>> +static struct clk_rcg2 video_cc_ahb_clk_src = {
> >>>> + .cmd_rcgr = 0xbd4,
> >>>> + .mnd_width = 0,
> >>>> + .hid_width = 5,
> >>>> + .parent_map = video_cc_parent_map_0,
> >>>> + .freq_tbl = ftbl_video_cc_ahb_clk_src,
> >>>> + .clkr.hw.init = &(const struct clk_init_data) {
> >>>> + .name = "video_cc_ahb_clk_src",
> >>>> + .parent_data = video_cc_parent_data_0,
> >>>> + .num_parents = ARRAY_SIZE(video_cc_parent_data_0),
> >>>> + .flags = CLK_SET_RATE_PARENT,
> >>>> + .ops = &clk_rcg2_shared_ops,
> >>>> + },
> >>>> +};
> >>>
> >>> Do we need this clock at all? We don't have the child
> >>> video_cc_ahb_clk, so potentially CCF can try disabling or modifying
> >>> this clock.
> >> Hm.. I see a few things:
> >>
> >> 1. downstream kona has it, upstream does not
> >> 2. it's shared so we never actually hard-shut it off..
> >> 2a. ..but it'd be good to ensure it's on when it's ready..
> >> 2b. ..but we never do anyway..
> >> 2c. ..but should we even? doesn't Venus govern it internally?
> >>
> >>
> >>>
> >>>> +
> >>>> +static const struct freq_tbl ftbl_video_cc_mvs0_clk_src[] = {
> >>>> + F(720000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0),
> >>>> + F(1014000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0),
> >>>> + F(1098000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0),
> >>>> + F(1332000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0),
> >>>> + { }
> >>>> +};
> >>>> +
> >>
> >> [...]
> >>
> >>>> +static struct clk_branch video_cc_mvs1_clk = {
> >>>> + .halt_reg = 0xdb4,
> >>>> + .halt_check = BRANCH_HALT_VOTED,
> >>>
> >>> As a note, sm8250 has BRANCH_HALT here.
> >> No, it does on the div2 clk, and so do we:
> >
> > Excuse me, I got confused by all the syllables. I was looking at the
> > video_cc_mvs1c_clk. On sm8250 it is _VOTED, in this patch it is not. I
> > can not say that either one of those is incorrect, but such a
> > difference looks a bit suspicious for me. Maybe Tanya or somebody else
> > can comment here.
> I'd say this could be a design decision, with div2 clocks being
> treated differently, but it's how downstream does it on shipping
> devices and while generally it's not a great thing to say, it seems
> to be the "right enough" thing..
Ack. Fair enough.
>
> >
> >> [...]
> >>
> >>>> +};
> >>>> +
> >>>> +static struct clk_branch video_cc_mvs1_div2_clk = {
> >>>> + .halt_reg = 0xdf4,
> >>>> + .halt_check = BRANCH_HALT_VOTED,
> >>>> + .hwcg_reg = 0xdf4,
> >>
> >> [...]
> >>
> >>>> +
> >>>> +static const struct qcom_reset_map video_cc_sm8350_resets[] = {
> >>>> + [CVP_VIDEO_CC_INTERFACE_BCR] = { 0xe54 },
> >>>> + [CVP_VIDEO_CC_MVS0_BCR] = { 0xd14 },
> >>>
> >>> Would it be better to use common VIDEO_CC prefix here (IOW:
> >>> VIDEO_CC_CVP_MVS0_BCR, VIDEO_CC_CVP_INTERFACE_BCR), etc.
> >> My best guess would be that the ones prefixed with CVP_
> >> are actual INTF/INSTANCEn(CORE) reset lines whereas
> >> the ones containing _CLK_ reset their clock sub-branches.
> >
> > Note, again, on sm8250 all resets start with VIDEO_CC, even CVP ones.
> > I think we can follow that.
> Or get rid of that, as it's always called with a phandle to videocc..
>
> Thoughts?
I'd say, switch to VIDEO_CC prefix, please. We can not drop the
prefix, as we risc getting conflicts otherwise.
>
> >
> >>
> >>>
> >>>> + [VIDEO_CC_MVS0C_CLK_ARES] = { 0xc34, 2 },
> >>>> + [CVP_VIDEO_CC_MVS0C_BCR] = { 0xbf4 },
> >>>> + [CVP_VIDEO_CC_MVS1_BCR] = { 0xd94 },
> >>>> + [VIDEO_CC_MVS1C_CLK_ARES] = { 0xcd4, 2 },
> >>>> + [CVP_VIDEO_CC_MVS1C_BCR] = { 0xc94 },
> >>>> +};
> >>
> >> [...]
> >>
> >>>> + ret = pm_runtime_resume_and_get(&pdev->dev);
> >>>> + if (ret)
> >>>> + return ret;
> >>>> +
> >>>> + regmap = qcom_cc_map(pdev, &video_cc_sm8350_desc);
> >>>> + if (IS_ERR(regmap)) {
> >>>> + pm_runtime_put(&pdev->dev);
> >>>> + return PTR_ERR(regmap);
> >>>> + };
> >>>
> >>> Extra semicolon
> >> Ooeh!
> >>
> >>>
> >>>> +
> >>>> + clk_lucid_pll_configure(&video_pll0, regmap, &video_pll0_config);
> >>>> + clk_lucid_pll_configure(&video_pll1, regmap, &video_pll1_config);
> >>>> +
> >>>> + /*
> >>>> + * Keep clocks always enabled:
> >>>> + * video_cc_ahb_clk
> >>>> + * video_cc_xo_clk
> >>>> + */
> >>>> + regmap_update_bits(regmap, 0xe58, BIT(0), BIT(0));
> >>>> + regmap_update_bits(regmap, 0xeec, BIT(0), BIT(0));
> >>>> +
> >>>> + ret = qcom_cc_really_probe(pdev, &video_cc_sm8350_desc, regmap);
> >>>> + pm_runtime_put(&pdev->dev);
> >>>> +
> >>>> + return ret;
> >>>> +}
> >>>> +
> >>>> +static const struct dev_pm_ops video_cc_sm8350_pm_ops = {
> >>>> + SET_RUNTIME_PM_OPS(pm_clk_suspend, pm_clk_resume, NULL)
> >>>
> >>> The driver doesn't use pm_clk at all. Are these PM_OPS correct?
> >> I'm unsure. I see the pm state changing in debugfs when the clocks are
> >> (not) consumed. But let's continue our discussion about using pm_clks
> >> for AHB.
> >
> > Well, those are two separate questions. One is that w/o additional
> > pm_clk calls this string is useless (and should be removed). Another
> > on is a possible restructure of our cc drivers to use pm_clk for AHB
> > clocks (which would require adding more than that).
> Right, I had an impression that you needed any sort of pm ops at
> all to be registered with pm_genpd correctly, but that seems not to
> be the case.. With that commented out, I still see "suspended" / "active"
> and not "unsupported"..
Let's just drop them for now.
>
> Konrad
> >
> >
> >>
> >>>
> >>>> +};
> >>>> +
> >>>> +static const struct of_device_id video_cc_sm8350_match_table[] = {
> >>>> + { .compatible = "qcom,sm8350-videocc" },
> >>>> + { }
> >>>> +};
> >>>> +MODULE_DEVICE_TABLE(of, video_cc_sm8350_match_table);
> >>>> +
> >>>> +static struct platform_driver video_cc_sm8350_driver = {
> >>>> + .probe = video_cc_sm8350_probe,
> >>>> + .driver = {
> >>>> + .name = "sm8350-videocc",
> >>>> + .of_match_table = video_cc_sm8350_match_table,
> >>>> + .pm = &video_cc_sm8350_pm_ops,
> >>>> + },
> >>>> +};
> >>>> +module_platform_driver(video_cc_sm8350_driver);
> >>>> +
> >>>> +MODULE_DESCRIPTION("QTI SM8350 VIDEOCC Driver");
> >>>> +MODULE_LICENSE("GPL");
> >>>>
> >>>> --
> >>>> 2.40.0
> >>>>
> >>>
> >>> Generic note: the register layout follows closely sm8250. However the
> >>> existing differences probably do not warrant merging them.
> >> No, I don't think merging any designs that are farther away
> >> than 8150 and 8155 or 8992 and 8994 etc. is a good idea..
> >>
> >> I don't want to ever look at something like dispcc-sm8[123]50.c
> >> again!
> >
> > Me too!
> >
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-04-17 18:41 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-14 11:26 [PATCH v2 0/2] SM8350 VIDEOCC Konrad Dybcio
2023-04-14 11:26 ` [PATCH v2 1/2] dt-bindings: clock: qcom,videocc: Add SM8350 Konrad Dybcio
2023-04-14 15:17 ` Krzysztof Kozlowski
2023-04-14 16:45 ` Dmitry Baryshkov
2023-04-14 20:32 ` Konrad Dybcio
2023-04-14 20:58 ` Krzysztof Kozlowski
2023-04-14 11:26 ` [PATCH v2 2/2] clk: qcom: Introduce SM8350 VIDEOCC Konrad Dybcio
2023-04-14 16:31 ` Dmitry Baryshkov
2023-04-14 17:48 ` Konrad Dybcio
2023-04-14 20:54 ` Dmitry Baryshkov
2023-04-17 18:10 ` Konrad Dybcio
2023-04-17 18:41 ` Dmitry Baryshkov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.