Re: [PATCH v2 5/5] clk: qcom: add video clock controller driver for SM8250

From: Stephen Boyd
Date: Tue Sep 22 2020 - 14:46:45 EST


Quoting Jonathan Marek (2020-09-03 20:09:54)
> Add support for the video clock controller found on SM8250 based devices.
>
> Derived from the downstream driver.
>
> Signed-off-by: Jonathan Marek <jonathan@xxxxxxxx>
> ---
> drivers/clk/qcom/Kconfig | 9 +
> drivers/clk/qcom/Makefile | 1 +
> drivers/clk/qcom/videocc-sm8250.c | 518 ++++++++++++++++++++++++++++++
> 3 files changed, 528 insertions(+)
> create mode 100644 drivers/clk/qcom/videocc-sm8250.c
>
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index 40d7ee9886c9..95efa38211d5 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -453,6 +453,15 @@ config SM_VIDEOCC_8150
> Say Y if you want to support video devices and functionality such as
> video encode and decode.
>
> +config SM_VIDEOCC_8250
> + tristate "SM8250 Video Clock Controller"
> + select SDM_GCC_8250
> + select QCOM_GDSC
> + help
> + Support for the video clock controller on SM8250 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 6f4c580d2728..55fb20800b66 100644
> --- a/drivers/clk/qcom/Makefile
> +++ b/drivers/clk/qcom/Makefile
> @@ -69,6 +69,7 @@ obj-$(CONFIG_SM_GCC_8250) += gcc-sm8250.o
> obj-$(CONFIG_SM_GPUCC_8150) += gpucc-sm8150.o
> obj-$(CONFIG_SM_GPUCC_8250) += gpucc-sm8250.o
> obj-$(CONFIG_SM_VIDEOCC_8150) += videocc-sm8150.o
> +obj-$(CONFIG_SM_VIDEOCC_8250) += videocc-sm8250.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-sm8250.c b/drivers/clk/qcom/videocc-sm8250.c
> new file mode 100644
> index 000000000000..a814d10945c4
> --- /dev/null
> +++ b/drivers/clk/qcom/videocc-sm8250.c
> @@ -0,0 +1,518 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2018-2020, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include <dt-bindings/clock/qcom,videocc-sm8250.h>
> +
[...]
> +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 = &(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 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 = &(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_IS_CRITICAL | CLK_SET_RATE_PARENT,

Similar critical clk comment, see below.

> + .ops = &clk_rcg2_ops,
> + },
> +};
> +
> +static struct clk_branch video_cc_ahb_clk = {
> + .halt_reg = 0xe58,
> + .halt_check = BRANCH_HALT,
> + .clkr = {
> + .enable_reg = 0xe58,
> + .enable_mask = BIT(0),
> + .hw.init = &(struct clk_init_data){
> + .name = "video_cc_ahb_clk",
> + .parent_data = &(const struct clk_parent_data){
> + .hw = &video_cc_ahb_clk_src.clkr.hw,
> + },
> + .num_parents = 1,
> + .flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT,

Similar critical clk comment, see below.

> + .ops = &clk_branch2_ops,
> + },
> + },
> +};
> +
> +static struct clk_branch video_cc_mvs0_clk = {
> + .halt_reg = 0xd34,
> + .halt_check = BRANCH_HALT_SKIP, /* TODO: hw gated ? */

Is this resolved?

> + .clkr = {
> + .enable_reg = 0xd34,
> + .enable_mask = BIT(0),
> + .hw.init = &(struct clk_init_data){
> + .name = "video_cc_mvs0_clk",
> + .parent_data = &(const struct clk_parent_data){
> + .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_xo_clk = {
> + .halt_reg = 0xeec,
> + .halt_check = BRANCH_HALT,
> + .clkr = {
> + .enable_reg = 0xeec,
> + .enable_mask = BIT(0),
> + .hw.init = &(struct clk_init_data){
> + .name = "video_cc_xo_clk",
> + .parent_data = &(const struct clk_parent_data){
> + .hw = &video_cc_xo_clk_src.clkr.hw,
> + },
> + .num_parents = 1,
> + .flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT,

Please add a coment why it's critical. If no consumer of this clk exists
it's preferred to move the enabling to probe and not waste memory
modeling it in software.

> + .ops = &clk_branch2_ops,
> + },
> + },
> +};
> +