Re: [PATCH v2 4/5] clk: qcom: dispcc-qcm2290: Add support for Qualcomm Shikra DISPCC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing parent clock for DSI1 byte clock breaks DSI1 display operation.
- [Low] Semantic mismatch and dead code regarding GPLL0 divided clock.
--

commit f4a6601cc643cfa42c191307206d5dca13a8299a
Author: Imran Shaik <imran.shaik@oss.qualcomm.com>

clk: qcom: dispcc-qcm2290: Add support for Qualcomm Shikra DISPCC

This commit updates the Qualcomm Shikra Display clock controller parent
data for mdss ahb/mdp clocks. It corrects the GDSC wait values and flags,
and introduces support for the DSI1 PHY source.

> diff --git a/drivers/clk/qcom/dispcc-qcm2290.c b/drivers/clk/qcom/dispcc-qcm2290.c
> index 6d88d067337fa..19c997f3fe9f1 100644
> --- a/drivers/clk/qcom/dispcc-qcm2290.c
> +++ b/drivers/clk/qcom/dispcc-qcm2290.c
[ ... ]
> @@ -84,7 +87,7 @@ static const struct clk_parent_data disp_cc_parent_data_1[] = {
>  
>  static const struct parent_map disp_cc_parent_map_2[] = {
>  	{ P_BI_TCXO_AO, 0 },
> -	{ P_GPLL0_OUT_DIV, 4 },
> +	{ P_GPLL0_OUT_MAIN, 4 },
>  };
>  
>  static const struct clk_parent_data disp_cc_parent_data_2[] = {

[Severity: Low]
Does this change create a mismatch between the enum and the clock source?

The map now uses P_GPLL0_OUT_MAIN, but disp_cc_parent_data_2 still references
gcc_disp_gpll0_div_clk_src. This change also appears to leave P_GPLL0_OUT_DIV
entirely unused in the codebase.

> @@ -101,17 +104,19 @@ static const struct parent_map disp_cc_parent_map_3[] = {
>  static const struct clk_parent_data disp_cc_parent_data_3[] = {
>  	{ .fw_name = "bi_tcxo" },
>  	{ .hw = &disp_cc_pll0.clkr.hw },
> -	{ .fw_name = "gcc_disp_gpll0_clk_src" },
> +	{ .fw_name = "gcc_disp_gpll0_div_clk_src" },
>  };

[Severity: Low]
Similar to the map above, disp_cc_parent_data_3 now points to the divided
clock source, but its corresponding map continues to use P_GPLL0_OUT_MAIN.

Is it intended for P_GPLL0_OUT_MAIN to represent the divided clock for
both multiplexers?

>  static const struct parent_map disp_cc_parent_map_4[] = {
>  	{ P_BI_TCXO, 0 },
>  	{ P_DSI0_PHY_PLL_OUT_DSICLK, 1 },
> +	{ P_DSI1_PHY_PLL_OUT_DSICLK, 2 },
>  };
>  
>  static const struct clk_parent_data disp_cc_parent_data_4[] = {
>  	{ .fw_name = "bi_tcxo" },
>  	{ .fw_name = "dsi0_phy_pll_out_dsiclk" },
> +	{ .fw_name = "dsi1_phy_pll_out_dsiclk" },
>  };

[Severity: High]
Does the DSI1 PHY source also require parent data for the byte clock?

The P_DSI1_PHY_PLL_OUT_DSICLK source is added to disp_cc_parent_map_4 and
disp_cc_parent_data_4 here. However, P_DSI1_PHY_PLL_OUT_BYTECLK was added
to the enum earlier in this patch but omitted from disp_cc_parent_map_0 and
disp_cc_parent_data_0.

Since a DSI interface structurally requires both a pixel clock and a
synchronously derived byte clock from the same active PHY, could this omission
prevent the hardware from configuring the byte and escape clocks to use the
DSI1 PHY, resulting in broken DSI1 functionality?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528-shikra-dispcc-gpucc-v2-0-953f246a0fbb@oss.qualcomm.com?part=4