[PATCH V4 0/3] Add video clock controller driver for SM8450

[PATCH V4 0/3] Add video clock controller driver for SM8450

[Taniya Das]

Add bindings, driver and DT node for video clock controller on SM8450.

Taniya Das (3):
  dt-bindings: clock: qcom: Add SM8450 video clock controller
  clk: qcom: videocc-sm8450: Add video clock controller driver for
    SM8450
  arm64: dts: qcom: sm8450: Add video clock controller

 .../bindings/clock/qcom,sm8450-videocc.yaml   |  77 +++
 arch/arm64/boot/dts/qcom/sm8450.dtsi          |  12 +
 drivers/clk/qcom/Kconfig                      |   9 +
 drivers/clk/qcom/Makefile                     |   1 +
 drivers/clk/qcom/videocc-sm8450.c             | 461 ++++++++++++++++++
 .../dt-bindings/clock/qcom,sm8450-videocc.h   |  38 ++
 6 files changed, 598 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml
 create mode 100644 drivers/clk/qcom/videocc-sm8450.c
 create mode 100644 include/dt-bindings/clock/qcom,sm8450-videocc.h

-- 
2.17.1

[PATCH V4 1/3] dt-bindings: clock: qcom: Add SM8450 video clock controller

[Taniya Das]

Add device tree bindings for the video clock controller on Qualcomm
SM8450 platform.

Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes since V3:
 - None.

Changes since V2:
 - As per Stephen's comments drop clock-names to match how newer
   qcom clk bindings are being done.
 - Change the header file name as qcom,sm8450-videocc.h to match
   latest upstream header files.

Changes since V1:
 - Change the properties order to keep reg after the compatible
   property.

 .../bindings/clock/qcom,sm8450-videocc.yaml   | 77 +++++++++++++++++++
 .../dt-bindings/clock/qcom,sm8450-videocc.h   | 38 +++++++++
 2 files changed, 115 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml
 create mode 100644 include/dt-bindings/clock/qcom,sm8450-videocc.h

diff --git a/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml b/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml
new file mode 100644
index 000000000000..58e59065bb2a
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml
@@ -0,0 +1,77 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/qcom,sm8450-videocc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Video Clock & Reset Controller on SM8450
+
+maintainers:
+  - Taniya Das <quic_tdas@quicinc.com>
+
+description: |
+  Qualcomm video clock control module provides the clocks, resets and power
+  domains on SM8450.
+
+  See also:: include/dt-bindings/clock/qcom,videocc-sm8450.h
+
+properties:
+  compatible:
+    const: qcom,sm8450-videocc
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: Video AHB clock from GCC
+      - description: Board XO source
+
+  power-domains:
+    maxItems: 1
+    description:
+      MMCX power domain.
+
+  required-opps:
+    maxItems: 1
+    description:
+      A phandle to an OPP node describing required MMCX performance point.
+
+  '#clock-cells':
+    const: 1
+
+  '#reset-cells':
+    const: 1
+
+  '#power-domain-cells':
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - power-domains
+  - required-opps
+  - '#clock-cells'
+  - '#reset-cells'
+  - '#power-domain-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,gcc-sm8450.h>
+    #include <dt-bindings/clock/qcom,rpmh.h>
+    #include <dt-bindings/power/qcom-rpmpd.h>
+    videocc: clock-controller@aaf0000 {
+      compatible = "qcom,sm8450-videocc";
+      reg = <0x0aaf0000 0x10000>;
+      clocks = <&gcc GCC_VIDEO_AHB_CLK>,
+               <&rpmhcc RPMH_CXO_CLK>;
+      power-domains = <&rpmhpd SM8450_MMCX>;
+      required-opps = <&rpmhpd_opp_low_svs>;
+      #clock-cells = <1>;
+      #reset-cells = <1>;
+      #power-domain-cells = <1>;
+    };
+...
diff --git a/include/dt-bindings/clock/qcom,sm8450-videocc.h b/include/dt-bindings/clock/qcom,sm8450-videocc.h
new file mode 100644
index 000000000000..9d795adfe4eb
--- /dev/null
+++ b/include/dt-bindings/clock/qcom,sm8450-videocc.h
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#ifndef _DT_BINDINGS_CLK_QCOM_VIDEO_CC_SM8450_H
+#define _DT_BINDINGS_CLK_QCOM_VIDEO_CC_SM8450_H
+
+/* VIDEO_CC clocks */
+#define VIDEO_CC_MVS0_CLK					0
+#define VIDEO_CC_MVS0_CLK_SRC					1
+#define VIDEO_CC_MVS0_DIV_CLK_SRC				2
+#define VIDEO_CC_MVS0C_CLK					3
+#define VIDEO_CC_MVS0C_DIV2_DIV_CLK_SRC				4
+#define VIDEO_CC_MVS1_CLK					5
+#define VIDEO_CC_MVS1_CLK_SRC					6
+#define VIDEO_CC_MVS1_DIV_CLK_SRC				7
+#define VIDEO_CC_MVS1C_CLK					8
+#define VIDEO_CC_MVS1C_DIV2_DIV_CLK_SRC				9
+#define VIDEO_CC_PLL0						10
+#define VIDEO_CC_PLL1						11
+
+/* VIDEO_CC power domains */
+#define VIDEO_CC_MVS0C_GDSC					0
+#define VIDEO_CC_MVS0_GDSC					1
+#define VIDEO_CC_MVS1C_GDSC					2
+#define VIDEO_CC_MVS1_GDSC					3
+
+/* VIDEO_CC resets */
+#define CVP_VIDEO_CC_INTERFACE_BCR				0
+#define CVP_VIDEO_CC_MVS0_BCR					1
+#define CVP_VIDEO_CC_MVS0C_BCR					2
+#define CVP_VIDEO_CC_MVS1_BCR					3
+#define CVP_VIDEO_CC_MVS1C_BCR					4
+#define VIDEO_CC_MVS0C_CLK_ARES					5
+#define VIDEO_CC_MVS1C_CLK_ARES					6
+
+#endif
-- 
2.17.1

Re: [PATCH V4 1/3] dt-bindings: clock: qcom: Add SM8450 video clock controller

[Krzysztof Kozlowski]

On 09/05/2023 19:21, Taniya Das wrote:
> Add device tree bindings for the video clock controller on Qualcomm
> SM8450 platform.
> 
> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
> Changes since V3:
>  - None.
> 
> Changes since V2:
>  - As per Stephen's comments drop clock-names to match how newer
>    qcom clk bindings are being done.
>  - Change the header file name as qcom,sm8450-videocc.h to match
>    latest upstream header files.
> 
> Changes since V1:
>  - Change the properties order to keep reg after the compatible
>    property.
> 
>  .../bindings/clock/qcom,sm8450-videocc.yaml   | 77 +++++++++++++++++++
>  .../dt-bindings/clock/qcom,sm8450-videocc.h   | 38 +++++++++
>  2 files changed, 115 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml
>  create mode 100644 include/dt-bindings/clock/qcom,sm8450-videocc.h
> 
> diff --git a/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml b/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml
> new file mode 100644
> index 000000000000..58e59065bb2a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml
> @@ -0,0 +1,77 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/qcom,sm8450-videocc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Video Clock & Reset Controller on SM8450
> +
> +maintainers:
> +  - Taniya Das <quic_tdas@quicinc.com>
> +
> +description: |
> +  Qualcomm video clock control module provides the clocks, resets and power
> +  domains on SM8450.
> +
> +  See also:: include/dt-bindings/clock/qcom,videocc-sm8450.h
> +
> +properties:
> +  compatible:
> +    const: qcom,sm8450-videocc
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: Video AHB clock from GCC
> +      - description: Board XO source

Why the order is different than all other devices? Board XO is always first.


Best regards,
Krzysztof

Re: [PATCH V4 1/3] dt-bindings: clock: qcom: Add SM8450 video clock controller

[Taniya Das]

Thanks for the review.

On 5/10/2023 12:42 PM, Krzysztof Kozlowski wrote:
> On 09/05/2023 19:21, Taniya Das wrote:
>> Add device tree bindings for the video clock controller on Qualcomm
>> SM8450 platform.
>>
>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
>> Reviewed-by: Rob Herring <robh@kernel.org>
>> ---
>> Changes since V3:
>>   - None.
>>
>> Changes since V2:
>>   - As per Stephen's comments drop clock-names to match how newer
>>     qcom clk bindings are being done.
>>   - Change the header file name as qcom,sm8450-videocc.h to match
>>     latest upstream header files.
>>
>> Changes since V1:
>>   - Change the properties order to keep reg after the compatible
>>     property.
>>
>>   .../bindings/clock/qcom,sm8450-videocc.yaml   | 77 +++++++++++++++++++
>>   .../dt-bindings/clock/qcom,sm8450-videocc.h   | 38 +++++++++
>>   2 files changed, 115 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml
>>   create mode 100644 include/dt-bindings/clock/qcom,sm8450-videocc.h
>>
>> diff --git a/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml b/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml
>> new file mode 100644
>> index 000000000000..58e59065bb2a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml
>> @@ -0,0 +1,77 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/clock/qcom,sm8450-videocc.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm Video Clock & Reset Controller on SM8450
>> +
>> +maintainers:
>> +  - Taniya Das <quic_tdas@quicinc.com>
>> +
>> +description: |
>> +  Qualcomm video clock control module provides the clocks, resets and power
>> +  domains on SM8450.
>> +
>> +  See also:: include/dt-bindings/clock/qcom,videocc-sm8450.h
>> +
>> +properties:
>> +  compatible:
>> +    const: qcom,sm8450-videocc
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    items:
>> +      - description: Video AHB clock from GCC
>> +      - description: Board XO source
> 
> Why the order is different than all other devices? Board XO is always first.
> 
>

Yes, will be fixed in the next patch set.


> Best regards,
> Krzysztof
> 

-- 
Thanks & Regards,
Taniya Das.

[PATCH V4 2/3] clk: qcom: videocc-sm8450: Add video clock controller driver for SM8450

[Taniya Das]

Add support for the video clock controller driver for peripheral clock
clients to be able to request for video cc clocks.

Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
---
Changes since V3:
 - Use lower case hex.
 - Check the return value here and bail out early on failure in probe.

Changes since V2:
 - Update the header file name to match the latest upstream header
   files.

Changes since V1:
 - Use DT indices instead of fw_name.
 - Replace pm_runtime_enable with devm_pm_runtime_enable.
 - Change license to GPL from GPL V2.

 drivers/clk/qcom/Kconfig          |   9 +
 drivers/clk/qcom/Makefile         |   1 +
 drivers/clk/qcom/videocc-sm8450.c | 461 ++++++++++++++++++++++++++++++
 3 files changed, 471 insertions(+)
 create mode 100644 drivers/clk/qcom/videocc-sm8450.c

diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index 12be3e2371b3..927aa5983464 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -962,4 +962,13 @@ config CLK_GFM_LPASS_SM8250
 	  Support for the Glitch Free Mux (GFM) Low power audio
           subsystem (LPASS) clocks found on SM8250 SoCs.

+config SM_VIDEOCC_8450
+	tristate "SM8450 Video Clock Controller"
+	select SM_GCC_8450
+	select QCOM_GDSC
+	help
+	  Support for the video clock controller on Qualcomm Technologies, Inc.
+	  SM8450 devices.
+	  Say Y if you want to support video devices and functionality such as
+	  video encode/decode.
 endif
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index 9ff4c373ad95..1960ad8e8713 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -127,6 +127,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_8450) += videocc-sm8450.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-sm8450.c b/drivers/clk/qcom/videocc-sm8450.c
new file mode 100644
index 000000000000..b6d25cc6956b
--- /dev/null
+++ b/drivers/clk/qcom/videocc-sm8450.c
@@ -0,0 +1,461 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+
+#include <dt-bindings/clock/qcom,sm8450-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 "gdsc.h"
+#include "reset.h"
+
+enum {
+	DT_BI_TCXO,
+};
+
+enum {
+	P_BI_TCXO,
+	P_VIDEO_CC_PLL0_OUT_MAIN,
+	P_VIDEO_CC_PLL1_OUT_MAIN,
+};
+
+static const struct pll_vco lucid_evo_vco[] = {
+	{ 249600000, 2020000000, 0 },
+};
+
+static const struct alpha_pll_config video_cc_pll0_config = {
+	.l = 0x1e,
+	.alpha = 0x0,
+	.config_ctl_val = 0x20485699,
+	.config_ctl_hi_val = 0x00182261,
+	.config_ctl_hi1_val = 0x32aa299c,
+	.user_ctl_val = 0x00000000,
+	.user_ctl_hi_val = 0x00000805,
+};
+
+static struct clk_alpha_pll video_cc_pll0 = {
+	.offset = 0x0,
+	.vco_table = lucid_evo_vco,
+	.num_vco = ARRAY_SIZE(lucid_evo_vco),
+	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_LUCID_EVO],
+	.clkr = {
+		.hw.init = &(const struct clk_init_data) {
+			.name = "video_cc_pll0",
+			.parent_data = &(const struct clk_parent_data) {
+				.index = DT_BI_TCXO,
+			},
+			.num_parents = 1,
+			.ops = &clk_alpha_pll_lucid_evo_ops,
+		},
+	},
+};
+
+static const struct alpha_pll_config video_cc_pll1_config = {
+	.l = 0x2b,
+	.alpha = 0xc000,
+	.config_ctl_val = 0x20485699,
+	.config_ctl_hi_val = 0x00182261,
+	.config_ctl_hi1_val = 0x32aa299c,
+	.user_ctl_val = 0x00000000,
+	.user_ctl_hi_val = 0x00000805,
+};
+
+static struct clk_alpha_pll video_cc_pll1 = {
+	.offset = 0x1000,
+	.vco_table = lucid_evo_vco,
+	.num_vco = ARRAY_SIZE(lucid_evo_vco),
+	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_LUCID_EVO],
+	.clkr = {
+		.hw.init = &(const struct clk_init_data) {
+			.name = "video_cc_pll1",
+			.parent_data = &(const struct clk_parent_data) {
+				.index = DT_BI_TCXO,
+			},
+			.num_parents = 1,
+			.ops = &clk_alpha_pll_lucid_evo_ops,
+		},
+	},
+};
+
+static const struct parent_map video_cc_parent_map_0[] = {
+	{ P_BI_TCXO, 0 },
+	{ P_VIDEO_CC_PLL0_OUT_MAIN, 1 },
+};
+
+static const struct clk_parent_data video_cc_parent_data_0[] = {
+	{ .index = DT_BI_TCXO },
+	{ .hw = &video_cc_pll0.clkr.hw },
+};
+
+static const struct parent_map video_cc_parent_map_1[] = {
+	{ P_BI_TCXO, 0 },
+	{ P_VIDEO_CC_PLL1_OUT_MAIN, 1 },
+};
+
+static const struct clk_parent_data video_cc_parent_data_1[] = {
+	{ .index = DT_BI_TCXO },
+	{ .hw = &video_cc_pll1.clkr.hw },
+};
+
+static const struct freq_tbl ftbl_video_cc_mvs0_clk_src[] = {
+	F(576000000, P_VIDEO_CC_PLL0_OUT_MAIN, 1, 0, 0),
+	F(720000000, P_VIDEO_CC_PLL0_OUT_MAIN, 1, 0, 0),
+	F(1014000000, P_VIDEO_CC_PLL0_OUT_MAIN, 1, 0, 0),
+	F(1098000000, P_VIDEO_CC_PLL0_OUT_MAIN, 1, 0, 0),
+	F(1332000000, P_VIDEO_CC_PLL0_OUT_MAIN, 1, 0, 0),
+	{ }
+};
+
+static struct clk_rcg2 video_cc_mvs0_clk_src = {
+	.cmd_rcgr = 0x8000,
+	.mnd_width = 0,
+	.hid_width = 5,
+	.parent_map = video_cc_parent_map_0,
+	.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_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_mvs1_clk_src[] = {
+	F(840000000, P_VIDEO_CC_PLL1_OUT_MAIN, 1, 0, 0),
+	F(1050000000, P_VIDEO_CC_PLL1_OUT_MAIN, 1, 0, 0),
+	F(1350000000, P_VIDEO_CC_PLL1_OUT_MAIN, 1, 0, 0),
+	F(1500000000, P_VIDEO_CC_PLL1_OUT_MAIN, 1, 0, 0),
+	F(1650000000, P_VIDEO_CC_PLL1_OUT_MAIN, 1, 0, 0),
+	{ }
+};
+
+static struct clk_rcg2 video_cc_mvs1_clk_src = {
+	.cmd_rcgr = 0x8018,
+	.mnd_width = 0,
+	.hid_width = 5,
+	.parent_map = video_cc_parent_map_1,
+	.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_1,
+		.num_parents = ARRAY_SIZE(video_cc_parent_data_1),
+		.flags = CLK_SET_RATE_PARENT,
+		.ops = &clk_rcg2_shared_ops,
+	},
+};
+
+static struct clk_regmap_div video_cc_mvs0_div_clk_src = {
+	.reg = 0x80b8,
+	.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 = 0x806c,
+	.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 = 0x80dc,
+	.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 = 0x8094,
+	.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 = 0x80b0,
+	.halt_check = BRANCH_HALT_SKIP,
+	.hwcg_reg = 0x80b0,
+	.hwcg_bit = 1,
+	.clkr = {
+		.enable_reg = 0x80b0,
+		.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 = 0x8064,
+	.halt_check = BRANCH_HALT,
+	.clkr = {
+		.enable_reg = 0x8064,
+		.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 = 0x80d4,
+	.halt_check = BRANCH_HALT_SKIP,
+	.hwcg_reg = 0x80d4,
+	.hwcg_bit = 1,
+	.clkr = {
+		.enable_reg = 0x80d4,
+		.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_mvs1c_clk = {
+	.halt_reg = 0x808c,
+	.halt_check = BRANCH_HALT,
+	.clkr = {
+		.enable_reg = 0x808c,
+		.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 gdsc video_cc_mvs0c_gdsc = {
+	.gdscr = 0x804c,
+	.en_rest_wait_val = 0x2,
+	.en_few_wait_val = 0x2,
+	.clk_dis_wait_val = 0x6,
+	.pd = {
+		.name = "video_cc_mvs0c_gdsc",
+	},
+	.pwrsts = PWRSTS_OFF_ON,
+	.flags = RETAIN_FF_ENABLE,
+};
+
+static struct gdsc video_cc_mvs0_gdsc = {
+	.gdscr = 0x809c,
+	.en_rest_wait_val = 0x2,
+	.en_few_wait_val = 0x2,
+	.clk_dis_wait_val = 0x6,
+	.pd = {
+		.name = "video_cc_mvs0_gdsc",
+	},
+	.pwrsts = PWRSTS_OFF_ON,
+	.parent = &video_cc_mvs0c_gdsc.pd,
+	.flags = RETAIN_FF_ENABLE | HW_CTRL,
+};
+
+static struct gdsc video_cc_mvs1c_gdsc = {
+	.gdscr = 0x8074,
+	.en_rest_wait_val = 0x2,
+	.en_few_wait_val = 0x2,
+	.clk_dis_wait_val = 0x6,
+	.pd = {
+		.name = "video_cc_mvs1c_gdsc",
+	},
+	.pwrsts = PWRSTS_OFF_ON,
+	.flags = RETAIN_FF_ENABLE,
+};
+
+static struct gdsc video_cc_mvs1_gdsc = {
+	.gdscr = 0x80c0,
+	.en_rest_wait_val = 0x2,
+	.en_few_wait_val = 0x2,
+	.clk_dis_wait_val = 0x6,
+	.pd = {
+		.name = "video_cc_mvs1_gdsc",
+	},
+	.pwrsts = PWRSTS_OFF_ON,
+	.parent = &video_cc_mvs1c_gdsc.pd,
+	.flags = RETAIN_FF_ENABLE | HW_CTRL,
+};
+
+static struct clk_regmap *video_cc_sm8450_clocks[] = {
+	[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_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_PLL0] = &video_cc_pll0.clkr,
+	[VIDEO_CC_PLL1] = &video_cc_pll1.clkr,
+};
+
+static struct gdsc *video_cc_sm8450_gdscs[] = {
+	[VIDEO_CC_MVS0C_GDSC] = &video_cc_mvs0c_gdsc,
+	[VIDEO_CC_MVS0_GDSC] = &video_cc_mvs0_gdsc,
+	[VIDEO_CC_MVS1C_GDSC] = &video_cc_mvs1c_gdsc,
+	[VIDEO_CC_MVS1_GDSC] = &video_cc_mvs1_gdsc,
+};
+
+static const struct qcom_reset_map video_cc_sm8450_resets[] = {
+	[CVP_VIDEO_CC_INTERFACE_BCR] = { 0x80e0 },
+	[CVP_VIDEO_CC_MVS0_BCR] = { 0x8098 },
+	[CVP_VIDEO_CC_MVS0C_BCR] = { 0x8048 },
+	[CVP_VIDEO_CC_MVS1_BCR] = { 0x80bc },
+	[CVP_VIDEO_CC_MVS1C_BCR] = { 0x8070 },
+	[VIDEO_CC_MVS0C_CLK_ARES] = { 0x8064, 2 },
+	[VIDEO_CC_MVS1C_CLK_ARES] = { 0x808c, 2 },
+};
+
+static const struct regmap_config video_cc_sm8450_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.max_register = 0x9f4c,
+	.fast_io = true,
+};
+
+static struct qcom_cc_desc video_cc_sm8450_desc = {
+	.config = &video_cc_sm8450_regmap_config,
+	.clks = video_cc_sm8450_clocks,
+	.num_clks = ARRAY_SIZE(video_cc_sm8450_clocks),
+	.resets = video_cc_sm8450_resets,
+	.num_resets = ARRAY_SIZE(video_cc_sm8450_resets),
+	.gdscs = video_cc_sm8450_gdscs,
+	.num_gdscs = ARRAY_SIZE(video_cc_sm8450_gdscs),
+};
+
+static const struct of_device_id video_cc_sm8450_match_table[] = {
+	{ .compatible = "qcom,sm8450-videocc" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, video_cc_sm8450_match_table);
+
+static int video_cc_sm8450_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_sm8450_desc);
+	if (IS_ERR(regmap)) {
+		pm_runtime_put(&pdev->dev);
+		return PTR_ERR(regmap);
+	}
+
+	clk_lucid_evo_pll_configure(&video_cc_pll0, regmap, &video_cc_pll0_config);
+	clk_lucid_evo_pll_configure(&video_cc_pll1, regmap, &video_cc_pll1_config);
+
+	/*
+	 * Keep clocks always enabled:
+	 *	video_cc_ahb_clk
+	 *	video_cc_sleep_clk
+	 *	video_cc_xo_clk
+	 */
+	regmap_update_bits(regmap, 0x80e4, BIT(0), BIT(0));
+	regmap_update_bits(regmap, 0x8130, BIT(0), BIT(0));
+	regmap_update_bits(regmap, 0x8114, BIT(0), BIT(0));
+
+	ret = qcom_cc_really_probe(pdev, &video_cc_sm8450_desc, regmap);
+
+	pm_runtime_put(&pdev->dev);
+
+	return ret;
+}
+
+static struct platform_driver video_cc_sm8450_driver = {
+	.probe = video_cc_sm8450_probe,
+	.driver = {
+		.name = "video_cc-sm8450",
+		.of_match_table = video_cc_sm8450_match_table,
+	},
+};
+
+static int __init video_cc_sm8450_init(void)
+{
+	return platform_driver_register(&video_cc_sm8450_driver);
+}
+subsys_initcall(video_cc_sm8450_init);
+
+static void __exit video_cc_sm8450_exit(void)
+{
+	platform_driver_unregister(&video_cc_sm8450_driver);
+}
+module_exit(video_cc_sm8450_exit);
+
+MODULE_DESCRIPTION("QTI VIDEO_CC SM8450 Driver");
+MODULE_LICENSE("GPL");
--
2.17.1

Re: [PATCH V4 2/3] clk: qcom: videocc-sm8450: Add video clock controller driver for SM8450

[Dmitry Baryshkov]

On Tue, 9 May 2023 at 20:22, Taniya Das <quic_tdas@quicinc.com> wrote:
>
> Add support for the video clock controller driver for peripheral clock
> clients to be able to request for video cc clocks.
>
> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
> ---
> Changes since V3:
>  - Use lower case hex.
>  - Check the return value here and bail out early on failure in probe.
>
> Changes since V2:
>  - Update the header file name to match the latest upstream header
>    files.
>
> Changes since V1:
>  - Use DT indices instead of fw_name.
>  - Replace pm_runtime_enable with devm_pm_runtime_enable.
>  - Change license to GPL from GPL V2.
>
>  drivers/clk/qcom/Kconfig          |   9 +
>  drivers/clk/qcom/Makefile         |   1 +
>  drivers/clk/qcom/videocc-sm8450.c | 461 ++++++++++++++++++++++++++++++
>  3 files changed, 471 insertions(+)
>  create mode 100644 drivers/clk/qcom/videocc-sm8450.c

[skipped]


> +static const struct qcom_reset_map video_cc_sm8450_resets[] = {
> +       [CVP_VIDEO_CC_INTERFACE_BCR] = { 0x80e0 },
> +       [CVP_VIDEO_CC_MVS0_BCR] = { 0x8098 },
> +       [CVP_VIDEO_CC_MVS0C_BCR] = { 0x8048 },
> +       [CVP_VIDEO_CC_MVS1_BCR] = { 0x80bc },
> +       [CVP_VIDEO_CC_MVS1C_BCR] = { 0x8070 },

Can we have a common VIDEO_CC prefix here please?

> +       [VIDEO_CC_MVS0C_CLK_ARES] = { 0x8064, 2 },
> +       [VIDEO_CC_MVS1C_CLK_ARES] = { 0x808c, 2 },
> +};
> +
> +static const struct regmap_config video_cc_sm8450_regmap_config = {
> +       .reg_bits = 32,
> +       .reg_stride = 4,
> +       .val_bits = 32,
> +       .max_register = 0x9f4c,
> +       .fast_io = true,
> +};
> +
> +static struct qcom_cc_desc video_cc_sm8450_desc = {
> +       .config = &video_cc_sm8450_regmap_config,
> +       .clks = video_cc_sm8450_clocks,
> +       .num_clks = ARRAY_SIZE(video_cc_sm8450_clocks),
> +       .resets = video_cc_sm8450_resets,
> +       .num_resets = ARRAY_SIZE(video_cc_sm8450_resets),
> +       .gdscs = video_cc_sm8450_gdscs,
> +       .num_gdscs = ARRAY_SIZE(video_cc_sm8450_gdscs),
> +};
> +
> +static const struct of_device_id video_cc_sm8450_match_table[] = {
> +       { .compatible = "qcom,sm8450-videocc" },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, video_cc_sm8450_match_table);
> +
> +static int video_cc_sm8450_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_sm8450_desc);
> +       if (IS_ERR(regmap)) {
> +               pm_runtime_put(&pdev->dev);
> +               return PTR_ERR(regmap);
> +       }
> +
> +       clk_lucid_evo_pll_configure(&video_cc_pll0, regmap, &video_cc_pll0_config);
> +       clk_lucid_evo_pll_configure(&video_cc_pll1, regmap, &video_cc_pll1_config);
> +
> +       /*
> +        * Keep clocks always enabled:
> +        *      video_cc_ahb_clk
> +        *      video_cc_sleep_clk
> +        *      video_cc_xo_clk
> +        */
> +       regmap_update_bits(regmap, 0x80e4, BIT(0), BIT(0));
> +       regmap_update_bits(regmap, 0x8130, BIT(0), BIT(0));
> +       regmap_update_bits(regmap, 0x8114, BIT(0), BIT(0));
> +
> +       ret = qcom_cc_really_probe(pdev, &video_cc_sm8450_desc, regmap);
> +
> +       pm_runtime_put(&pdev->dev);
> +
> +       return ret;
> +}
> +
> +static struct platform_driver video_cc_sm8450_driver = {
> +       .probe = video_cc_sm8450_probe,
> +       .driver = {
> +               .name = "video_cc-sm8450",
> +               .of_match_table = video_cc_sm8450_match_table,
> +       },
> +};
> +
> +static int __init video_cc_sm8450_init(void)
> +{
> +       return platform_driver_register(&video_cc_sm8450_driver);
> +}
> +subsys_initcall(video_cc_sm8450_init);
> +
> +static void __exit video_cc_sm8450_exit(void)
> +{
> +       platform_driver_unregister(&video_cc_sm8450_driver);
> +}
> +module_exit(video_cc_sm8450_exit);

module_platform_driver() ?

> +
> +MODULE_DESCRIPTION("QTI VIDEO_CC SM8450 Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.17.1
>


-- 
With best wishes
Dmitry

Re: [PATCH V4 2/3] clk: qcom: videocc-sm8450: Add video clock controller driver for SM8450

[Taniya Das]

Hello Dmitry,

Thank you for your review.

On 5/10/2023 2:03 AM, Dmitry Baryshkov wrote:
> On Tue, 9 May 2023 at 20:22, Taniya Das <quic_tdas@quicinc.com> wrote:
>>
>> Add support for the video clock controller driver for peripheral clock
>> clients to be able to request for video cc clocks.
>>
>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
>> ---
>> Changes since V3:
>>   - Use lower case hex.
>>   - Check the return value here and bail out early on failure in probe.
>>
>> Changes since V2:
>>   - Update the header file name to match the latest upstream header
>>     files.
>>
>> Changes since V1:
>>   - Use DT indices instead of fw_name.
>>   - Replace pm_runtime_enable with devm_pm_runtime_enable.
>>   - Change license to GPL from GPL V2.
>>
>>   drivers/clk/qcom/Kconfig          |   9 +
>>   drivers/clk/qcom/Makefile         |   1 +
>>   drivers/clk/qcom/videocc-sm8450.c | 461 ++++++++++++++++++++++++++++++
>>   3 files changed, 471 insertions(+)
>>   create mode 100644 drivers/clk/qcom/videocc-sm8450.c
> 
> [skipped]
> 
> 
>> +static const struct qcom_reset_map video_cc_sm8450_resets[] = {
>> +       [CVP_VIDEO_CC_INTERFACE_BCR] = { 0x80e0 },
>> +       [CVP_VIDEO_CC_MVS0_BCR] = { 0x8098 },
>> +       [CVP_VIDEO_CC_MVS0C_BCR] = { 0x8048 },
>> +       [CVP_VIDEO_CC_MVS1_BCR] = { 0x80bc },
>> +       [CVP_VIDEO_CC_MVS1C_BCR] = { 0x8070 },
> 
> Can we have a common VIDEO_CC prefix here please?

The BCR names are coming from hardware plan and software interface, thus 
we would like to keep them intact.


> 
>> +       [VIDEO_CC_MVS0C_CLK_ARES] = { 0x8064, 2 },
>> +       [VIDEO_CC_MVS1C_CLK_ARES] = { 0x808c, 2 },
>> +};
>> +

The ARES resets are coming from VideoCC clocks(CBCR), hence the name 
starts with VIDEO_CC.

>> +static const struct regmap_config video_cc_sm8450_regmap_config = {
>> +       .reg_bits = 32,
>> +       .reg_stride = 4,
>> +       .val_bits = 32,
>> +       .max_register = 0x9f4c,
>> +       .fast_io = true,
>> +};
>> +
>> +static struct qcom_cc_desc video_cc_sm8450_desc = {
>> +       .config = &video_cc_sm8450_regmap_config,
>> +       .clks = video_cc_sm8450_clocks,
>> +       .num_clks = ARRAY_SIZE(video_cc_sm8450_clocks),
>> +       .resets = video_cc_sm8450_resets,
>> +       .num_resets = ARRAY_SIZE(video_cc_sm8450_resets),
>> +       .gdscs = video_cc_sm8450_gdscs,
>> +       .num_gdscs = ARRAY_SIZE(video_cc_sm8450_gdscs),
>> +};
>> +
>> +static const struct of_device_id video_cc_sm8450_match_table[] = {
>> +       { .compatible = "qcom,sm8450-videocc" },
>> +       { }
>> +};
>> +MODULE_DEVICE_TABLE(of, video_cc_sm8450_match_table);
>> +
>> +static int video_cc_sm8450_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_sm8450_desc);
>> +       if (IS_ERR(regmap)) {
>> +               pm_runtime_put(&pdev->dev);
>> +               return PTR_ERR(regmap);
>> +       }
>> +
>> +       clk_lucid_evo_pll_configure(&video_cc_pll0, regmap, &video_cc_pll0_config);
>> +       clk_lucid_evo_pll_configure(&video_cc_pll1, regmap, &video_cc_pll1_config);
>> +
>> +       /*
>> +        * Keep clocks always enabled:
>> +        *      video_cc_ahb_clk
>> +        *      video_cc_sleep_clk
>> +        *      video_cc_xo_clk
>> +        */
>> +       regmap_update_bits(regmap, 0x80e4, BIT(0), BIT(0));
>> +       regmap_update_bits(regmap, 0x8130, BIT(0), BIT(0));
>> +       regmap_update_bits(regmap, 0x8114, BIT(0), BIT(0));
>> +
>> +       ret = qcom_cc_really_probe(pdev, &video_cc_sm8450_desc, regmap);
>> +
>> +       pm_runtime_put(&pdev->dev);
>> +
>> +       return ret;
>> +}
>> +
>> +static struct platform_driver video_cc_sm8450_driver = {
>> +       .probe = video_cc_sm8450_probe,
>> +       .driver = {
>> +               .name = "video_cc-sm8450",
>> +               .of_match_table = video_cc_sm8450_match_table,
>> +       },
>> +};
>> +
>> +static int __init video_cc_sm8450_init(void)
>> +{
>> +       return platform_driver_register(&video_cc_sm8450_driver);
>> +}
>> +subsys_initcall(video_cc_sm8450_init);
>> +
>> +static void __exit video_cc_sm8450_exit(void)
>> +{
>> +       platform_driver_unregister(&video_cc_sm8450_driver);
>> +}
>> +module_exit(video_cc_sm8450_exit);
> 
> module_platform_driver() ?
> 

We would like to keep the clock drivers all probed at subsys_initcall. 
We could revisit and update as cleanup if we want to move them to module 
init.

>> +
>> +MODULE_DESCRIPTION("QTI VIDEO_CC SM8450 Driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.17.1
>>
> 
> 

-- 
Thanks & Regards,
Taniya Das.

Re: [PATCH V4 2/3] clk: qcom: videocc-sm8450: Add video clock controller driver for SM8450

[Dmitry Baryshkov]

On Fri, 19 May 2023 at 13:53, Taniya Das <quic_tdas@quicinc.com> wrote:
>
> Hello Dmitry,
>
> Thank you for your review.
>
> On 5/10/2023 2:03 AM, Dmitry Baryshkov wrote:
> > On Tue, 9 May 2023 at 20:22, Taniya Das <quic_tdas@quicinc.com> wrote:
> >>
> >> Add support for the video clock controller driver for peripheral clock
> >> clients to be able to request for video cc clocks.
> >>
> >> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
> >> ---
> >> Changes since V3:
> >>   - Use lower case hex.
> >>   - Check the return value here and bail out early on failure in probe.
> >>
> >> Changes since V2:
> >>   - Update the header file name to match the latest upstream header
> >>     files.
> >>
> >> Changes since V1:
> >>   - Use DT indices instead of fw_name.
> >>   - Replace pm_runtime_enable with devm_pm_runtime_enable.
> >>   - Change license to GPL from GPL V2.
> >>
> >>   drivers/clk/qcom/Kconfig          |   9 +
> >>   drivers/clk/qcom/Makefile         |   1 +
> >>   drivers/clk/qcom/videocc-sm8450.c | 461 ++++++++++++++++++++++++++++++
> >>   3 files changed, 471 insertions(+)
> >>   create mode 100644 drivers/clk/qcom/videocc-sm8450.c
> >
> > [skipped]
> >
> >
> >> +static const struct qcom_reset_map video_cc_sm8450_resets[] = {
> >> +       [CVP_VIDEO_CC_INTERFACE_BCR] = { 0x80e0 },
> >> +       [CVP_VIDEO_CC_MVS0_BCR] = { 0x8098 },
> >> +       [CVP_VIDEO_CC_MVS0C_BCR] = { 0x8048 },
> >> +       [CVP_VIDEO_CC_MVS1_BCR] = { 0x80bc },
> >> +       [CVP_VIDEO_CC_MVS1C_BCR] = { 0x8070 },
> >
> > Can we have a common VIDEO_CC prefix here please?
>
> The BCR names are coming from hardware plan and software interface, thus
> we would like to keep them intact.

We have had a similar discussion on the sm8350-videocc driver. While
keeping the hardware names is nice, name uniformity also should not be
neglected. It is much easier to follow and verify the patches if all
videocc resets start with VIDEO_CC name.

>
>
> >
> >> +       [VIDEO_CC_MVS0C_CLK_ARES] = { 0x8064, 2 },
> >> +       [VIDEO_CC_MVS1C_CLK_ARES] = { 0x808c, 2 },
> >> +};
> >> +
>
> The ARES resets are coming from VideoCC clocks(CBCR), hence the name
> starts with VIDEO_CC.
>
> >> +static const struct regmap_config video_cc_sm8450_regmap_config = {
> >> +       .reg_bits = 32,
> >> +       .reg_stride = 4,
> >> +       .val_bits = 32,
> >> +       .max_register = 0x9f4c,
> >> +       .fast_io = true,
> >> +};
> >> +
> >> +static struct qcom_cc_desc video_cc_sm8450_desc = {
> >> +       .config = &video_cc_sm8450_regmap_config,
> >> +       .clks = video_cc_sm8450_clocks,
> >> +       .num_clks = ARRAY_SIZE(video_cc_sm8450_clocks),
> >> +       .resets = video_cc_sm8450_resets,
> >> +       .num_resets = ARRAY_SIZE(video_cc_sm8450_resets),
> >> +       .gdscs = video_cc_sm8450_gdscs,
> >> +       .num_gdscs = ARRAY_SIZE(video_cc_sm8450_gdscs),
> >> +};
> >> +
> >> +static const struct of_device_id video_cc_sm8450_match_table[] = {
> >> +       { .compatible = "qcom,sm8450-videocc" },
> >> +       { }
> >> +};
> >> +MODULE_DEVICE_TABLE(of, video_cc_sm8450_match_table);
> >> +
> >> +static int video_cc_sm8450_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_sm8450_desc);
> >> +       if (IS_ERR(regmap)) {
> >> +               pm_runtime_put(&pdev->dev);
> >> +               return PTR_ERR(regmap);
> >> +       }
> >> +
> >> +       clk_lucid_evo_pll_configure(&video_cc_pll0, regmap, &video_cc_pll0_config);
> >> +       clk_lucid_evo_pll_configure(&video_cc_pll1, regmap, &video_cc_pll1_config);
> >> +
> >> +       /*
> >> +        * Keep clocks always enabled:
> >> +        *      video_cc_ahb_clk
> >> +        *      video_cc_sleep_clk
> >> +        *      video_cc_xo_clk
> >> +        */
> >> +       regmap_update_bits(regmap, 0x80e4, BIT(0), BIT(0));
> >> +       regmap_update_bits(regmap, 0x8130, BIT(0), BIT(0));
> >> +       regmap_update_bits(regmap, 0x8114, BIT(0), BIT(0));
> >> +
> >> +       ret = qcom_cc_really_probe(pdev, &video_cc_sm8450_desc, regmap);
> >> +
> >> +       pm_runtime_put(&pdev->dev);
> >> +
> >> +       return ret;
> >> +}
> >> +
> >> +static struct platform_driver video_cc_sm8450_driver = {
> >> +       .probe = video_cc_sm8450_probe,
> >> +       .driver = {
> >> +               .name = "video_cc-sm8450",
> >> +               .of_match_table = video_cc_sm8450_match_table,
> >> +       },
> >> +};
> >> +
> >> +static int __init video_cc_sm8450_init(void)
> >> +{
> >> +       return platform_driver_register(&video_cc_sm8450_driver);
> >> +}
> >> +subsys_initcall(video_cc_sm8450_init);
> >> +
> >> +static void __exit video_cc_sm8450_exit(void)
> >> +{
> >> +       platform_driver_unregister(&video_cc_sm8450_driver);
> >> +}
> >> +module_exit(video_cc_sm8450_exit);
> >
> > module_platform_driver() ?
> >
>
> We would like to keep the clock drivers all probed at subsys_initcall.
> We could revisit and update as cleanup if we want to move them to module
> init.

Any particular reason?

>
> >> +
> >> +MODULE_DESCRIPTION("QTI VIDEO_CC SM8450 Driver");
> >> +MODULE_LICENSE("GPL");
> >> --
> >> 2.17.1
> >>
> >
> >
>
> --
> Thanks & Regards,
> Taniya Das.



-- 
With best wishes
Dmitry

Re: [PATCH V4 2/3] clk: qcom: videocc-sm8450: Add video clock controller driver for SM8450

[Jagadeesh Kona]

Hi Dmitry,

Thanks for your review!

On 5/19/2023 6:19 PM, Dmitry Baryshkov wrote:
> On Fri, 19 May 2023 at 13:53, Taniya Das <quic_tdas@quicinc.com> wrote:
>>
>> Hello Dmitry,
>>
>> Thank you for your review.
>>
>> On 5/10/2023 2:03 AM, Dmitry Baryshkov wrote:
>>> On Tue, 9 May 2023 at 20:22, Taniya Das <quic_tdas@quicinc.com> wrote:
>>>>
>>>> Add support for the video clock controller driver for peripheral clock
>>>> clients to be able to request for video cc clocks.
>>>>
>>>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
>>>> ---
>>>> Changes since V3:
>>>>    - Use lower case hex.
>>>>    - Check the return value here and bail out early on failure in probe.
>>>>
>>>> Changes since V2:
>>>>    - Update the header file name to match the latest upstream header
>>>>      files.
>>>>
>>>> Changes since V1:
>>>>    - Use DT indices instead of fw_name.
>>>>    - Replace pm_runtime_enable with devm_pm_runtime_enable.
>>>>    - Change license to GPL from GPL V2.
>>>>
>>>>    drivers/clk/qcom/Kconfig          |   9 +
>>>>    drivers/clk/qcom/Makefile         |   1 +
>>>>    drivers/clk/qcom/videocc-sm8450.c | 461 ++++++++++++++++++++++++++++++
>>>>    3 files changed, 471 insertions(+)
>>>>    create mode 100644 drivers/clk/qcom/videocc-sm8450.c
>>>
>>> [skipped]
>>>
>>>
>>>> +static const struct qcom_reset_map video_cc_sm8450_resets[] = {
>>>> +       [CVP_VIDEO_CC_INTERFACE_BCR] = { 0x80e0 },
>>>> +       [CVP_VIDEO_CC_MVS0_BCR] = { 0x8098 },
>>>> +       [CVP_VIDEO_CC_MVS0C_BCR] = { 0x8048 },
>>>> +       [CVP_VIDEO_CC_MVS1_BCR] = { 0x80bc },
>>>> +       [CVP_VIDEO_CC_MVS1C_BCR] = { 0x8070 },
>>>
>>> Can we have a common VIDEO_CC prefix here please?
>>
>> The BCR names are coming from hardware plan and software interface, thus
>> we would like to keep them intact.
> 
> We have had a similar discussion on the sm8350-videocc driver. While
> keeping the hardware names is nice, name uniformity also should not be
> neglected. It is much easier to follow and verify the patches if all
> videocc resets start with VIDEO_CC name.
> 

As mentioned earlier, the BCR names are coming from hardware plan. 
Client drivers also use hardware plan to refer to these names. Hence we 
would like to keep these names aligned as per the hardware plan.

>>
>>
>>>
>>>> +       [VIDEO_CC_MVS0C_CLK_ARES] = { 0x8064, 2 },
>>>> +       [VIDEO_CC_MVS1C_CLK_ARES] = { 0x808c, 2 },
>>>> +};
>>>> +
>>
>> The ARES resets are coming from VideoCC clocks(CBCR), hence the name
>> starts with VIDEO_CC.
>>
>>>> +static const struct regmap_config video_cc_sm8450_regmap_config = {
>>>> +       .reg_bits = 32,
>>>> +       .reg_stride = 4,
>>>> +       .val_bits = 32,
>>>> +       .max_register = 0x9f4c,
>>>> +       .fast_io = true,
>>>> +};
>>>> +
>>>> +static struct qcom_cc_desc video_cc_sm8450_desc = {
>>>> +       .config = &video_cc_sm8450_regmap_config,
>>>> +       .clks = video_cc_sm8450_clocks,
>>>> +       .num_clks = ARRAY_SIZE(video_cc_sm8450_clocks),
>>>> +       .resets = video_cc_sm8450_resets,
>>>> +       .num_resets = ARRAY_SIZE(video_cc_sm8450_resets),
>>>> +       .gdscs = video_cc_sm8450_gdscs,
>>>> +       .num_gdscs = ARRAY_SIZE(video_cc_sm8450_gdscs),
>>>> +};
>>>> +
>>>> +static const struct of_device_id video_cc_sm8450_match_table[] = {
>>>> +       { .compatible = "qcom,sm8450-videocc" },
>>>> +       { }
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, video_cc_sm8450_match_table);
>>>> +
>>>> +static int video_cc_sm8450_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_sm8450_desc);
>>>> +       if (IS_ERR(regmap)) {
>>>> +               pm_runtime_put(&pdev->dev);
>>>> +               return PTR_ERR(regmap);
>>>> +       }
>>>> +
>>>> +       clk_lucid_evo_pll_configure(&video_cc_pll0, regmap, &video_cc_pll0_config);
>>>> +       clk_lucid_evo_pll_configure(&video_cc_pll1, regmap, &video_cc_pll1_config);
>>>> +
>>>> +       /*
>>>> +        * Keep clocks always enabled:
>>>> +        *      video_cc_ahb_clk
>>>> +        *      video_cc_sleep_clk
>>>> +        *      video_cc_xo_clk
>>>> +        */
>>>> +       regmap_update_bits(regmap, 0x80e4, BIT(0), BIT(0));
>>>> +       regmap_update_bits(regmap, 0x8130, BIT(0), BIT(0));
>>>> +       regmap_update_bits(regmap, 0x8114, BIT(0), BIT(0));
>>>> +
>>>> +       ret = qcom_cc_really_probe(pdev, &video_cc_sm8450_desc, regmap);
>>>> +
>>>> +       pm_runtime_put(&pdev->dev);
>>>> +
>>>> +       return ret;
>>>> +}
>>>> +
>>>> +static struct platform_driver video_cc_sm8450_driver = {
>>>> +       .probe = video_cc_sm8450_probe,
>>>> +       .driver = {
>>>> +               .name = "video_cc-sm8450",
>>>> +               .of_match_table = video_cc_sm8450_match_table,
>>>> +       },
>>>> +};
>>>> +
>>>> +static int __init video_cc_sm8450_init(void)
>>>> +{
>>>> +       return platform_driver_register(&video_cc_sm8450_driver);
>>>> +}
>>>> +subsys_initcall(video_cc_sm8450_init);
>>>> +
>>>> +static void __exit video_cc_sm8450_exit(void)
>>>> +{
>>>> +       platform_driver_unregister(&video_cc_sm8450_driver);
>>>> +}
>>>> +module_exit(video_cc_sm8450_exit);
>>>
>>> module_platform_driver() ?
>>>
>>
>> We would like to keep the clock drivers all probed at subsys_initcall.
>> We could revisit and update as cleanup if we want to move them to module
>> init.
> 
> Any particular reason?
> 

We need to evaluate and validate if module_initcall works for us in all 
the scenarios. We will revisit and post a cleanup patch once we conclude
module_initcall works for us in all scenarios.

Thanks & Regards,
Jagadeesh

[PATCH V4 3/3] arm64: dts: qcom: sm8450: Add video clock controller

[Taniya Das]

Add device node for video clock controller on Qualcomm SM8450 platform.

Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
---
Changes since V3:
 - None.

Changes since V2:
 - No changes.

Changes since V1:
 - No changes.

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

diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
index 595533aeafc4..00ff8efa53c7 100644
--- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
@@ -756,6 +756,18 @@
 				      "usb3_phy_wrapper_gcc_usb30_pipe_clk";
 		};
 
+		videocc: clock-controller@aaf0000 {
+			compatible = "qcom,sm8450-videocc";
+			reg = <0 0x0aaf0000 0 0x10000>;
+			clocks = <&rpmhcc RPMH_CXO_CLK>,
+				 <&gcc GCC_VIDEO_AHB_CLK>;
+			power-domains = <&rpmhpd SM8450_MMCX>;
+			required-opps = <&rpmhpd_opp_low_svs>;
+			#clock-cells = <1>;
+			#reset-cells = <1>;
+			#power-domain-cells = <1>;
+		};
+
 		gpi_dma2: dma-controller@800000 {
 			compatible = "qcom,sm8450-gpi-dma", "qcom,sm6350-gpi-dma";
 			#dma-cells = <3>;
-- 
2.17.1

Re: [PATCH V4 3/3] arm64: dts: qcom: sm8450: Add video clock controller

[Konrad Dybcio]

On 9.05.2023 19:21, Taniya Das wrote:
> Add device node for video clock controller on Qualcomm SM8450 platform.
> 
> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
> ---
> Changes since V3:
>  - None.
> 
> Changes since V2:
>  - No changes.
> 
> Changes since V1:
>  - No changes.
> 
>  arch/arm64/boot/dts/qcom/sm8450.dtsi | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> index 595533aeafc4..00ff8efa53c7 100644
> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> @@ -756,6 +756,18 @@
>  				      "usb3_phy_wrapper_gcc_usb30_pipe_clk";
>  		};
>  
> +		videocc: clock-controller@aaf0000 {
Nodes should be sorted by unit address.
This one belongs before cci@ac15000.

> +			compatible = "qcom,sm8450-videocc";
> +			reg = <0 0x0aaf0000 0 0x10000>;
> +			clocks = <&rpmhcc RPMH_CXO_CLK>,
> +				 <&gcc GCC_VIDEO_AHB_CLK>;
Older SoCs used to provide a vote on XO_A for videocc ahb_clk_src,
I'd assume that's now taken care of internally?

Konrad
> +			power-domains = <&rpmhpd SM8450_MMCX>;
> +			required-opps = <&rpmhpd_opp_low_svs>;
> +			#clock-cells = <1>;
> +			#reset-cells = <1>;
> +			#power-domain-cells = <1>;
> +		};
> +
>  		gpi_dma2: dma-controller@800000 {
>  			compatible = "qcom,sm8450-gpi-dma", "qcom,sm6350-gpi-dma";
>  			#dma-cells = <3>;

Re: [PATCH V4 3/3] arm64: dts: qcom: sm8450: Add video clock controller

[Taniya Das]

Thanks for the review.

On 5/10/2023 1:47 AM, Konrad Dybcio wrote:
> 
> 
> On 9.05.2023 19:21, Taniya Das wrote:
>> Add device node for video clock controller on Qualcomm SM8450 platform.
>>
>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
>> ---
>> Changes since V3:
>>   - None.
>>
>> Changes since V2:
>>   - No changes.
>>
>> Changes since V1:
>>   - No changes.
>>
>>   arch/arm64/boot/dts/qcom/sm8450.dtsi | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
>> index 595533aeafc4..00ff8efa53c7 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
>> @@ -756,6 +756,18 @@
>>   				      "usb3_phy_wrapper_gcc_usb30_pipe_clk";
>>   		};
>>   
>> +		videocc: clock-controller@aaf0000 {
> Nodes should be sorted by unit address.
> This one belongs before cci@ac15000.

Yes, my bad, will update in the next patchset.

> 
>> +			compatible = "qcom,sm8450-videocc";
>> +			reg = <0 0x0aaf0000 0 0x10000>;
>> +			clocks = <&rpmhcc RPMH_CXO_CLK>,
>> +				 <&gcc GCC_VIDEO_AHB_CLK>;
> Older SoCs used to provide a vote on XO_A for videocc ahb_clk_src,
> I'd assume that's now taken care of internally?
> 

Yes, it is taken care internally.

> Konrad
>> +			power-domains = <&rpmhpd SM8450_MMCX>;
>> +			required-opps = <&rpmhpd_opp_low_svs>;
>> +			#clock-cells = <1>;
>> +			#reset-cells = <1>;
>> +			#power-domain-cells = <1>;
>> +		};
>> +
>>   		gpi_dma2: dma-controller@800000 {
>>   			compatible = "qcom,sm8450-gpi-dma", "qcom,sm6350-gpi-dma";
>>   			#dma-cells = <3>;

-- 
Thanks & Regards,
Taniya Das.