Re: [PATCH V2 RESEND 5/6] clk: qcom: camcc-sm8650: Add SM8650 camera clock controller driver

[Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>]

Hello Jagadeesh,

thank you for submitting the clock driver.

On 3/21/24 11:25, Jagadeesh Kona wrote:
> Add support for the camera clock controller for camera clients to
> be able to request for camcc clocks on SM8650 platform.
> 
> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>   drivers/clk/qcom/Kconfig        |    8 +
>   drivers/clk/qcom/Makefile       |    1 +
>   drivers/clk/qcom/camcc-sm8650.c | 3591 +++++++++++++++++++++++++++++++
>   3 files changed, 3600 insertions(+)
>   create mode 100644 drivers/clk/qcom/camcc-sm8650.c
> 
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index 8ab08e7b5b6c..6257f4a02ec4 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -826,6 +826,14 @@ config SM_CAMCC_8550
>   	  Support for the camera clock controller on SM8550 devices.
>   	  Say Y if you want to support camera devices and camera functionality.
>   
> +config SM_CAMCC_8650
> +	tristate "SM8650 Camera Clock Controller"
> +	depends on ARM64 || COMPILE_TEST
> +	select SM_GCC_8650
> +	help
> +	  Support for the camera clock controller on SM8650 devices.
> +	  Say Y if you want to support camera devices and camera functionality.
> +
>   config SM_DISPCC_6115
>   	tristate "SM6115 Display Clock Controller"
>   	depends on ARM64 || COMPILE_TEST
> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> index dec5b6db6860..28bffa1eb8dd 100644
> --- a/drivers/clk/qcom/Makefile
> +++ b/drivers/clk/qcom/Makefile
> @@ -109,6 +109,7 @@ obj-$(CONFIG_SM_CAMCC_6350) += camcc-sm6350.o
>   obj-$(CONFIG_SM_CAMCC_8250) += camcc-sm8250.o
>   obj-$(CONFIG_SM_CAMCC_8450) += camcc-sm8450.o
>   obj-$(CONFIG_SM_CAMCC_8550) += camcc-sm8550.o
> +obj-$(CONFIG_SM_CAMCC_8650) += camcc-sm8650.o
>   obj-$(CONFIG_SM_DISPCC_6115) += dispcc-sm6115.o
>   obj-$(CONFIG_SM_DISPCC_6125) += dispcc-sm6125.o
>   obj-$(CONFIG_SM_DISPCC_6350) += dispcc-sm6350.o
> diff --git a/drivers/clk/qcom/camcc-sm8650.c b/drivers/clk/qcom/camcc-sm8650.c
> new file mode 100644
> index 000000000000..1b28e086e519
> --- /dev/null
> +++ b/drivers/clk/qcom/camcc-sm8650.c
> @@ -0,0 +1,3591 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +
> +#include <dt-bindings/clock/qcom,sm8650-camcc.h>
> +
> +#include "clk-alpha-pll.h"
> +#include "clk-branch.h"
> +#include "clk-rcg.h"
> +#include "clk-regmap.h"
> +#include "common.h"
> +#include "gdsc.h"
> +#include "reset.h"
> +
> +enum {
> +	DT_IFACE,
> +	DT_BI_TCXO,
> +	DT_BI_TCXO_AO,
> +	DT_SLEEP_CLK,
> +};
> +
> +enum {
> +	P_BI_TCXO,
> +	P_BI_TCXO_AO,
> +	P_CAM_CC_PLL0_OUT_EVEN,
> +	P_CAM_CC_PLL0_OUT_MAIN,
> +	P_CAM_CC_PLL0_OUT_ODD,
> +	P_CAM_CC_PLL1_OUT_EVEN,
> +	P_CAM_CC_PLL2_OUT_EVEN,
> +	P_CAM_CC_PLL2_OUT_MAIN,
> +	P_CAM_CC_PLL3_OUT_EVEN,
> +	P_CAM_CC_PLL4_OUT_EVEN,
> +	P_CAM_CC_PLL5_OUT_EVEN,
> +	P_CAM_CC_PLL6_OUT_EVEN,
> +	P_CAM_CC_PLL7_OUT_EVEN,
> +	P_CAM_CC_PLL8_OUT_EVEN,
> +	P_CAM_CC_PLL9_OUT_EVEN,
> +	P_CAM_CC_PLL9_OUT_ODD,
> +	P_CAM_CC_PLL10_OUT_EVEN,
> +	P_SLEEP_CLK,
> +};
> +
> +static const struct pll_vco lucid_ole_vco[] = {
> +	{ 249600000, 2300000000, 0 },
> +};

I've noticed that a downstream Android kernel v6.1.25 defines this clock as

	static const struct pll_vco lucid_ole_vco[] = {
		{ 249600000, 2100000000, 0 },
	};

Do you know any particular reason why here the clock frequencies are different?

> +
> +static const struct pll_vco rivian_ole_vco[] = {
> +	{ 777000000, 1285000000, 0 },
> +};
> +

<snip>

> +static struct clk_rcg2 cam_cc_bps_clk_src = {
> +	.cmd_rcgr = 0x10050,
> +	.mnd_width = 0,
> +	.hid_width = 5,
> +	.parent_map = cam_cc_parent_map_2,
> +	.freq_tbl = ftbl_cam_cc_bps_clk_src,
> +	.clkr.hw.init = &(const struct clk_init_data) {
> +		.name = "cam_cc_bps_clk_src",
> +		.parent_data = cam_cc_parent_data_2,
> +		.num_parents = ARRAY_SIZE(cam_cc_parent_data_2),
> +		.flags = CLK_SET_RATE_PARENT,
> +		.ops = &clk_rcg2_shared_ops,
> +	},
> +};

Please let me ask after Dmitry about your rationale to select
&clk_rcg2_shared_ops here and below for all *_src clocks introduced in
the driver, I do remember you've did it in v1, could you please
elaborate it a bit more?

I have a concern that it's not possible to get an .is_enabled status
of the shared clocks, however at least in this particular case of
camcc clocks it seems to be technically possible.

It might indicate that there is an incompleteness in clk-rcg2.c driver
also, if it's really possible to get is_enabled runtime status at least
for some of the shared clocks.

> +
> +static const struct freq_tbl ftbl_cam_cc_camnoc_axi_rt_clk_src[] = {
> +	F(300000000, P_CAM_CC_PLL0_OUT_EVEN, 2, 0, 0),
> +	F(400000000, P_CAM_CC_PLL0_OUT_ODD, 1, 0, 0),
> +	{ }
> +};
> +

<snip>

Other than two my open questions above I don't see any issues with the
driver, if you be kind to provide the answers, please feel free to add
my

Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
Tested-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

--
Best wishes,
Vladimir