Re: [PATCH RFC 02/23] clk: qcom: add SM8650 LPASS core & audio clock controllers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] clk: qcom: missing pm_clk setup for iface clock in
  lpass_hm_core_probe()
- [High] clk: qcom: wrong DT clock index for BI_TCXO in
  lpass_core_cc_parent_data_ao_2
- [Medium] clk: qcom: missing driver unregister on error in SM8650 LPASS
  init functions
- [Medium] clk: qcom: wrong postdiv clk_ops and user_ctl_val for
  lpass_core_cc_dig_pll
- [Low] clk: qcom: dead code, unused declaration, and unwrapped pm_ops
  in SM8650 LPASS
--

commit b45bde29cb5ec941ad6de37d77c22a656fbda080
Author: Neil Armstrong <neil.armstrong@linaro.org>

clk: qcom: add SM8650 LPASS core & audio clock controllers

This commit adds clock and power domain controller drivers for the SM8650
LPASS HM, core, audio, and AON blocks when running without ADSP firmware.

> diff --git a/drivers/clk/qcom/lpassaudiocc-sm8650.c b/drivers/clk/qcom/lpassaudiocc-sm8650.c
> new file mode 100644
> index 0000000000000..636ce9a1b3451
> --- /dev/null
> +++ b/drivers/clk/qcom/lpassaudiocc-sm8650.c
[ ... ]
> +static struct clk_rcg2 lpass_aon_cc_ext_if1_clk_src = {
> +	.cmd_rcgr = 0xb004,
> +	.mnd_width = 16,
> +	.hid_width = 5,
> +	.parent_map = lpass_audio_cc_parent_map_0,
> +	.freq_tbl = ftbl_lpass_cc_ext_clk_src,
> +	.clkr.hw.init = &(const struct clk_init_data){
> +		.name = "lpass_aon_cc_ext_if1_clk_src",
> +		.parent_data = lpass_aon_cc_parent_data_0,
> +		.num_parents = ARRAY_SIZE(lpass_aon_cc_parent_data_0),
> +		.ops = &clk_rcg2_ops,
> +	},
> +};
[ ... ]
> +static struct clk_rcg2 lpass_aon_cc_tx_mclk_clk_src = {
> +	.cmd_rcgr = 0x1b004,
> +	.mnd_width = 0,
> +	.hid_width = 5,
> +	.parent_map = lpass_aon_cc_parent_map_0,
> +	.freq_tbl = ftbl_lpass_mclk_clk_src,
> +	.clkr.hw.init = &(const struct clk_init_data){
> +		.name = "lpass_aon_cc_tx_mclk_clk_src",
> +		.parent_data = lpass_aon_cc_parent_data_0,
> +		.num_parents = ARRAY_SIZE(lpass_aon_cc_parent_data_0),
> +		//.flags = CLK_OPS_PARENT_ENABLE,
> +		.ops = &clk_rcg2_ops,
> +	},
> +};
[ ... ]
> +static struct clk_branch lpass_aon_cc_tx_mclk_clk;
[ ... ]
> +static const struct dev_pm_ops lpass_audio_cc_sm8650_pm_ops = {
> +	SET_RUNTIME_PM_OPS(pm_clk_suspend, pm_clk_resume, NULL)
> +};
> +
> +static struct platform_driver lpass_audio_cc_sm8650_driver = {
> +	.probe = lpass_audio_cc_sm8650_probe,
> +	.driver = {
> +		.name = "lpass_audio_cc-sm8650",
> +		.of_match_table = lpass_audio_cc_sm8650_match_table,
> +		.pm = &lpass_audio_cc_sm8650_pm_ops,
> +	},
> +};

[Severity: Low]
This isn't a bug, but the `.pm` assignments in `lpass_audio_cc_sm8650_driver`,
`lpass_aon_cc_sm8650_driver`, and `lpass_hm_sm8650_driver` do not use
`pm_ptr()` (with `RUNTIME_PM_OPS()`), so the `dev_pm_ops` structures are not
dropped when PM is disabled.

Also, `lpass_aon_cc_tx_mclk_clk_src` has a commented-out `.flags` line, the
forward declaration of `lpass_aon_cc_tx_mclk_clk` is unused because the
following clocks reference `lpass_aon_cc_tx_mclk_clk_src`,
`lpass_aon_cc_ext_if1_clk_src` uses `lpass_audio_cc_parent_map_0` instead of
`lpass_aon_cc_parent_map_0`, and `lpass_audio_cc_sm8650_probe()` and
`lpass_aon_cc_sm8650_probe()` duplicate the same `pm_clk` setup.

[ ... ]
> +static int __init lpass_audio_cc_sm8650_init(void)
> +{
> +	int ret;
> +
> +	ret = platform_driver_register(&lpass_aon_cc_sm8650_driver);
> +	if (ret)
> +		return ret;
> +
> +	return platform_driver_register(&lpass_audio_cc_sm8650_driver);
> +}
> +subsys_initcall(lpass_audio_cc_sm8650_init);

[Severity: Medium]
Does `lpass_audio_cc_sm8650_init()` leave `lpass_aon_cc_sm8650_driver`
registered if registering `lpass_audio_cc_sm8650_driver` fails?  When built as
a module, failing module init without calling
`platform_driver_unregister(&lpass_aon_cc_sm8650_driver)` leaves a freed
`platform_driver` structure linked on the platform bus list, causing a
use-after-free.  `lpass_core_cc_sm8650_init()` in `lpasscorecc-sm8650.c` has
the same issue with `lpass_hm_sm8650_driver`.

> diff --git a/drivers/clk/qcom/lpasscorecc-sm8650.c b/drivers/clk/qcom/lpasscorecc-sm8650.c
> new file mode 100644
> index 0000000000000..4f91ab0d772ec
> --- /dev/null
> +++ b/drivers/clk/qcom/lpasscorecc-sm8650.c
> @@ -0,0 +1,509 @@
[ ... ]
> +static const struct alpha_pll_config lpass_core_cc_dig_pll_config = {
> +	/* 614.4 MHz */
> +	.l = 0x20,
> +	.alpha = 0x0,
> +	.config_ctl_val = 0x20485699,
> +	.config_ctl_hi_val = 0x00182261,
> +	.config_ctl_hi1_val = 0x82aa299c,
> +	.test_ctl_val = 0x00000000,
> +	.test_ctl_hi_val = 0x00000003,
> +	.test_ctl_hi1_val = 0x00009000,
> +	.test_ctl_hi2_val = 0x00000034,
> +	.user_ctl_val = 0x00000000,
> +	.user_ctl_hi_val = 0x00000005,
> +};
[ ... ]
> +static struct clk_alpha_pll_postdiv lpass_core_cc_dig_pll_out_odd = {
> +	.offset = 0x0,
> +	.post_div_shift = 14,
> +	.post_div_table = post_div_table_lpass_core_cc_dig_pll_out_odd,
> +	.num_post_div = ARRAY_SIZE(post_div_table_lpass_core_cc_dig_pll_out_odd),
> +	.width = 4,
> +	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_LUCID_OLE],
> +	.clkr.hw.init = &(struct clk_init_data){
> +		.name = "lpass_core_cc_dig_pll_out_odd",
> +		.parent_hws = (const struct clk_hw*[]){
> +			&lpass_core_cc_dig_pll.clkr.hw,
> +		},
> +		.num_parents = 1,
> +		.flags = CLK_SET_RATE_PARENT,
> +		.ops = &clk_alpha_pll_postdiv_lucid_ops,
> +	},
> +};

[Severity: Medium]
Should `lpass_core_cc_dig_pll_out_odd` use `clk_alpha_pll_postdiv_lucid_ole_ops`
instead of `clk_alpha_pll_postdiv_lucid_ops`?

`lpass_core_cc_dig_pll` is a Lucid OLE PLL, where the FSM vote bit lives at
bit 25 of `user_ctl` (`clk_lucid_evo_pll_postdiv_set_rate()`), whereas
`clk_alpha_pll_postdiv_lucid_ops` checks bit 20 of `mode`
(`clk_alpha_pll_postdiv_fabia_set_rate()`).  In addition,
`lpass_core_cc_dig_pll_config` sets `.user_ctl_val = 0x00000000`, so
`clk_alpha_pll_write_config()` skips writing `user_ctl` and never programs the
odd post-divider or output enable bits during PLL configuration.

[ ... ]
> +static const struct parent_map lpass_core_cc_parent_map_2[] = {
> +	{ P_BI_TCXO, 0 },
> +	{ P_LPASS_CORE_CC_DIG_PLL_OUT_MAIN, 1 },
> +	{ P_LPASS_CORE_CC_DIG_PLL_OUT_MAIN_DIV_CLK_SRC, 2 },
> +};
> +
> +static const struct clk_parent_data lpass_core_cc_parent_data_ao_2[] = {
> +	{ .index = 1 },
> +	{ .hw = &lpass_core_cc_dig_pll.clkr.hw },
> +	{ .hw = &lpass_core_cc_dig_pll_out_main_div_clk_src.clkr.hw },
> +};
> +
> +static const struct freq_tbl ftbl_lpass_core_cc_core_clk_src[] = {
> +	F(19200000, P_BI_TCXO, 1, 0, 0),
> +	F(51200000, P_LPASS_CORE_CC_DIG_PLL_OUT_MAIN_DIV_CLK_SRC, 6, 0, 0),
> +	F(102400000, P_LPASS_CORE_CC_DIG_PLL_OUT_MAIN_DIV_CLK_SRC, 3, 0, 0),
> +	F(204800000, P_LPASS_CORE_CC_DIG_PLL_OUT_MAIN, 3, 0, 0),
> +	{ }
> +};
> +
> +static struct clk_rcg2 lpass_core_cc_core_clk_src = {
> +	.cmd_rcgr = 0x25000,
> +	.mnd_width = 8,
> +	.hid_width = 5,
> +	.parent_map = lpass_core_cc_parent_map_2,
> +	.freq_tbl = ftbl_lpass_core_cc_core_clk_src,
> +	.clkr.hw.init = &(const struct clk_init_data){
> +		.name = "lpass_core_cc_core_clk_src",
> +		.parent_data = lpass_core_cc_parent_data_ao_2,
> +		.num_parents = ARRAY_SIZE(lpass_core_cc_parent_data_ao_2),
> +		.ops = &clk_rcg2_shared_ops,
> +	},
> +};

[Severity: High]
Should `lpass_core_cc_parent_data_ao_2[0]` use `.index = 0` instead of
`.index = 1`?

`qcom,sm8650-lpasscorecc` only has a single DT input clock (`bi_tcxo` at index
0), as used by `lpass_core_cc_dig_pll` and `lpass_core_cc_parent_data_0`.
With `.index = 1`, parent 0 of `lpass_core_cc_core_clk_src` cannot be
resolved, so `lpass_core_cc_core_clk_src` and its child branches report rate 0
when parked on `bi_tcxo` via `clk_rcg2_shared_ops`, and requesting 19.2 MHz
fails with `-EINVAL` in `_freq_tbl_determine_rate()`.

[ ... ]
> +static int lpass_hm_core_probe(struct platform_device *pdev)
> +{
> +	return qcom_cc_probe(pdev, &lpass_core_hm_sm8650_desc);
> +}
> +
> +static const struct dev_pm_ops lpass_hm_sm8650_pm_ops = {
> +	SET_RUNTIME_PM_OPS(pm_clk_suspend, pm_clk_resume, NULL)
> +};

[Severity: High]
Does `lpass_hm_core_probe()` need to set up `pm_clk` and add the `"iface"`
clock before calling `qcom_cc_probe()`, like `lpass_audio_cc_sm8650_probe()`
and `lpass_aon_cc_sm8650_probe()` do?

`qcom_cc_really_probe()` with `.use_rpm = true` only calls
`devm_pm_runtime_enable()` and `pm_runtime_resume_and_get()`; it does not call
`devm_pm_clk_create()` or `pm_clk_add(&pdev->dev, "iface")`.  Without that
setup, `pm_clk_resume()` and `pm_clk_suspend()` in `lpass_hm_sm8650_pm_ops` do
nothing, leaving the LPASS HM interface clock unacquired and potentially gated
when `qcom_cc_really_probe()` and `gdsc_register()` access the HM registers.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-topic-sm8650-el2-v1-0-5183f6aab9fc@linaro.org?part=2