Re: [PATCH DNM RFC 14/16] clk: qcom: Enable clk-dvfs on SM8650

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] clk: qcom: Unconditional GDSC required_pds breaks probe on older platforms
--

commit 5e8694e8d0d0380215d061d6b9b0bbabdb94e291
Author: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
One line subject: clk: qcom: Enable clk-dvfs on SM8650

This commit enables clock-dvfs infrastructure for SM8650 by filling in the
power requirements for SM8650 clocks. It also adds SM8650 support to the
shared dispcc, videocc, and camcc drivers.

> diff --git a/drivers/clk/qcom/dispcc-sm8550.c b/drivers/clk/qcom/dispcc-sm8550.c
> --- a/drivers/clk/qcom/dispcc-sm8550.c
> +++ b/drivers/clk/qcom/dispcc-sm8550.c
[ ... ]
> @@ -1629,6 +1778,8 @@ static struct gdsc mdss_gdsc = {
>  	},
>  	.pwrsts = PWRSTS_OFF_ON,
>  	.flags = POLL_CFG_GDSCR | HW_CTRL | RETAIN_FF_ENABLE,
> +	.required_pds = disp_cc_sm8650_block_pd_indices,
> +	.num_required_pds = ARRAY_SIZE(disp_cc_sm8650_block_pd_indices),
>  };

[Severity: High]
Does this unconditional assignment of SM8650-specific power domains to the
mdss_gdsc structure break the probe on older platforms like SM8550, SAR2130P,
and X1E80100?

Since the dispcc-sm8550.c driver is shared between these platforms, the older
platform descriptors (like disp_cc_sm8550_desc) do not set
num_dvfs_rail_voters. As a result, qcom_clk_dvfs_register() won't be called
during qcom_cc_really_probe().

When gdsc_register() attempts to register these shared GDSCs on the older
platforms, it evaluates num_required_pds > 0 and calls
gdsc_get_rail_voters(). This will then unconditionally delegate to
qcom_clk_dvfs_get_rail_voter(), which returns -EINVAL because the DVFS
subsystem was never initialized for them.

Could this cause the entire clock controller probe to fail on those older
platforms?

> diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c
> --- a/drivers/clk/qcom/videocc-sm8550.c
> +++ b/drivers/clk/qcom/videocc-sm8550.c
[ ... ]
> @@ -456,6 +514,8 @@ static struct gdsc video_cc_mvs0c_gdsc = {
>  	},
>  	.pwrsts = PWRSTS_OFF_ON,
>  	.flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE,
> +	.required_pds = video_cc_sm8650_block_pd_indices,
> +	.num_required_pds = ARRAY_SIZE(video_cc_sm8650_block_pd_indices),
>  };

[Severity: High]
Could the same probe breakage occur here for the video_cc_mvs0c_gdsc
structure in the shared videocc-sm8550.c driver?

The SM8650-specific requirements are added unconditionally to
video_cc_mvs0c_gdsc, which is used by SM8550 and X1E80100 as well. This
appears to introduce the same risk of failing the clock controller probe on
those platforms.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-topic-qc_clk_dvfs-v1-0-3041b788e9f7@oss.qualcomm.com?part=14