Re: [PATCH 2/3] clk: qcom: Add SM8350 camera clock controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] clk: qcom: Lucid 5LPE PLLs in camcc-sm8350 use clk_trion_pll_configure()
- [Medium] clk: qcom: camcc-sm8350: duplicate cam_cc_pll2 parent in cam_cc_parent_data_1
--

commit 0c4b57cb98acf493d0d1fffca035f3d8996aea87
Author: Vladimir Zapolskiy <vz@kernel.org>

clk: qcom: Add SM8350 camera clock controller driver

This commit adds the camera clock controller (CAMCC) driver for the
Qualcomm SM8350 SoC, including six Lucid 5LPE PLLs, one Zonda 5LPE PLL,
clocks, resets, and GDSCs.

> diff --git a/drivers/clk/qcom/camcc-sm8350.c b/drivers/clk/qcom/camcc-sm8350.c
> new file mode 100644
> index 0000000000000..7881f1d2ab09e
> --- /dev/null
> +++ b/drivers/clk/qcom/camcc-sm8350.c
> @@ -0,0 +1,2650 @@

[ ... ]

> +static struct clk_alpha_pll cam_cc_pll0 = {
> +	.offset = 0x0,
> +	.config = &cam_cc_pll0_config,
> +	.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 = "cam_cc_pll0",
> +			.parent_data = &pll_parent_data_tcxo,
> +			.num_parents = 1,
> +			.ops = &clk_alpha_pll_lucid_5lpe_ops,
> +		},
> +	},
> +};

[ ... ]

> +static const struct parent_map cam_cc_parent_map_1[] = {
> +	{ P_BI_TCXO, 0 },
> +	{ P_CAM_CC_PLL2_OUT_AUX2, 3 },
> +	{ P_CAM_CC_PLL2_OUT_EARLY, 5 },
> +};
> +
> +static const struct clk_parent_data cam_cc_parent_data_1[] = {
> +	{ .index = DT_BI_TCXO },
> +	{ .hw = &cam_cc_pll2.clkr.hw },
> +	{ .hw = &cam_cc_pll2.clkr.hw },
> +};

[Severity: Medium]
Does having two entries in cam_cc_parent_data_1 point to the same
&cam_cc_pll2.clkr.hw cause clk_fetch_parent_index() to select the wrong
parent index for cam_cc_mclk0_clk_src through cam_cc_mclk5_clk_src?

In cam_cc_parent_map_1, index 1 is P_CAM_CC_PLL2_OUT_AUX2 (cfg = 3) and
index 2 is P_CAM_CC_PLL2_OUT_EARLY (cfg = 5), while ftbl_cam_cc_mclk0_clk_src
only uses P_BI_TCXO and P_CAM_CC_PLL2_OUT_EARLY:

static const struct freq_tbl ftbl_cam_cc_mclk0_clk_src[] = {
	F(19200000, P_BI_TCXO, 1, 0, 0),
	F(24000000, P_CAM_CC_PLL2_OUT_EARLY, 10, 1, 4),
	F(68571429, P_CAM_CC_PLL2_OUT_EARLY, 14, 0, 0),
	{ }
};

When clk_fetch_parent_index() in drivers/clk/clk.c looks up cam_cc_pll2, it
scans linearly from index 0:

drivers/clk/clk.c:clk_fetch_parent_index() {
	for (i = 0; i < core->num_parents; i++) {
		/* Found it first try! */
		if (core->parents[i].core == parent)
			return i;

		/* Something else is here, so keep looking */
		if (core->parents[i].core)
			continue;

		/* Maybe core hasn't been cached but the hw is all we know? */
		if (core->parents[i].hw) {
			if (core->parents[i].hw == parent->hw)
				break;

Even after _freq_tbl_determine_rate() populates core->parents[2].core via
clk_hw_get_parent_by_index(hw, 2), core->parents[1].core remains NULL and
core->parents[1].hw == parent->hw matches at index 1, caching
core->parents[1].core = parent and returning index 1 instead of index 2.

If clk_set_parent() is called on cam_cc_mclk0_clk_src..cam_cc_mclk5_clk_src
with cam_cc_pll2 (for example via assigned-clock-parents),
clk_rcg2_shared_set_parent() receives index 1 and programs
rcg->parent_map[1].cfg (3, P_CAM_CC_PLL2_OUT_AUX2) into CFG_SRC_SEL instead
of rcg->parent_map[2].cfg (5, P_CAM_CC_PLL2_OUT_EARLY).

Should P_CAM_CC_PLL2_OUT_AUX2 and the duplicate &cam_cc_pll2.clkr.hw entry
be removed from cam_cc_parent_map_1 and cam_cc_parent_data_1, as in
camcc-sm8150.c and camcc-sm8250.c?

[ ... ]

> +static struct clk_alpha_pll *cam_cc_sm8350_plls[] = {
> +	&cam_cc_pll0,
> +	&cam_cc_pll1,
> +	&cam_cc_pll2,
> +	&cam_cc_pll3,
> +	&cam_cc_pll4,
> +	&cam_cc_pll5,
> +	&cam_cc_pll6,
> +};
> +
> +static const u32 cam_cc_sm8350_critical_cbcrs[] = {
> +	0xc148, /* CAM_CC_GDSC_CLK */
> +};
> +
> +static const struct regmap_config cam_cc_sm8350_regmap_config = {
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +	.max_register = 0xf008,
> +	.fast_io = true,
> +};
> +
> +static const struct qcom_cc_driver_data cam_cc_sm8350_driver_data = {
> +	.alpha_plls = cam_cc_sm8350_plls,
> +	.num_alpha_plls = ARRAY_SIZE(cam_cc_sm8350_plls),
> +	.clk_cbcrs = cam_cc_sm8350_critical_cbcrs,
> +	.num_clk_cbcrs = ARRAY_SIZE(cam_cc_sm8350_critical_cbcrs),
> +};

[Severity: Medium]
Will passing cam_cc_sm8350_plls via cam_cc_sm8350_driver_data.alpha_plls
configure the six Lucid 5LPE PLLs (cam_cc_pll0, cam_cc_pll1, cam_cc_pll3,
cam_cc_pll4, cam_cc_pll5, and cam_cc_pll6) with clk_trion_pll_configure()
instead of clk_lucid_5lpe_pll_configure()?

During probe, the call chain is:

cam_cc_sm8350_probe()
  -> qcom_cc_probe()
    -> qcom_cc_really_probe()
      -> qcom_cc_clk_pll_configure()
        -> qcom_clk_alpha_pll_configure()

Those six PLLs use clk_alpha_pll_lucid_5lpe_ops and
clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_LUCID]. Because
CLK_ALPHA_PLL_TYPE_LUCID is aliased to CLK_ALPHA_PLL_TYPE_TRION in
drivers/clk/qcom/clk-alpha-pll.h, GET_PLL_TYPE(pll) in
qcom_clk_alpha_pll_configure() matches CLK_ALPHA_PLL_TYPE_TRION:

drivers/clk/qcom/clk-alpha-pll.c:qcom_clk_alpha_pll_configure() {
	...
	case CLK_ALPHA_PLL_TYPE_TRION:
		clk_trion_pll_configure(pll, regmap, pll->config);
		break;

clk_trion_pll_configure() sets PLL_UPDATE_BYPASS in PLL_MODE:

drivers/clk/qcom/clk-alpha-pll.c:clk_trion_pll_configure() {
	...
	regmap_update_bits(regmap, PLL_MODE(pll), PLL_UPDATE_BYPASS,
			   PLL_UPDATE_BYPASS);

As noted in commit 362be5cbaec2a6 ("clk: qcom: dispcc-sm8250: use special
function for Lucid 5LPE PLL"), Lucid 5LPE PLLs require
clk_lucid_5lpe_pll_configure(), which omits setting PLL_UPDATE_BYPASS.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104001704.2744766-1-vz@kernel.org?part=2