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

[Imran Shaik <imran.shaik@oss.qualcomm.com>]

On 28-05-2026 07:12 pm, Dmitry Baryshkov wrote:
> On Thu, May 28, 2026 at 03:37:05PM +0530, Imran Shaik wrote:
>> The Qualcomm Shikra Display clock controller reuses the QCM2290 DISPCC,
>> but has minor differences. Update the parent data of mdss ahb/mdp clocks
>> accordingly to the hardware clock plan and correct the GDSC *_wait_val and
>> flags which are applicable for both QCM2290 and Shikra SoC, and add the
>> support for DSI1 PHY source.
>>
>> Signed-off-by: Imran Shaik <imran.shaik@oss.qualcomm.com>
>> ---
>>   drivers/clk/qcom/dispcc-qcm2290.c | 18 +++++++++++++-----
>>   1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/clk/qcom/dispcc-qcm2290.c b/drivers/clk/qcom/dispcc-qcm2290.c
>> index 6d88d067337fa132114b0d8666931b449f86de17..19c997f3fe9f197d2c252a9dd1e8169947200f5f 100644
>> --- a/drivers/clk/qcom/dispcc-qcm2290.c
>> +++ b/drivers/clk/qcom/dispcc-qcm2290.c
>> @@ -2,6 +2,7 @@
>>   /*
>>    * Copyright (c) 2020, The Linux Foundation. All rights reserved.
>>    * Copyright (c) 2021, Linaro Ltd.
>> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
>>    */
>>   
>>   #include <linux/clk-provider.h>
>> @@ -32,6 +33,8 @@ enum {
>>   	P_GPLL0_OUT_DIV,
>>   	P_GPLL0_OUT_MAIN,
>>   	P_SLEEP_CLK,
>> +	P_DSI1_PHY_PLL_OUT_BYTECLK,
>> +	P_DSI1_PHY_PLL_OUT_DSICLK,
>>   };
>>   
>>   static const struct pll_vco spark_vco[] = {
>> @@ -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 },
> 
> Why?
> 
>>   };
>>   
>>   static const struct clk_parent_data disp_cc_parent_data_2[] = {
>> @@ -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" },
> 
> Do you realize that this is an undocumented ABI chance?
> 
>>   };
>>   
>>   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" },
>>   };
>>   
>>   static const struct parent_map disp_cc_parent_map_5[] = {
>> @@ -153,8 +158,8 @@ static struct clk_regmap_div disp_cc_mdss_byte0_div_clk_src = {
>>   
>>   static const struct freq_tbl ftbl_disp_cc_mdss_ahb_clk_src[] = {
>>   	F(19200000, P_BI_TCXO_AO, 1, 0, 0),
>> -	F(37500000, P_GPLL0_OUT_DIV, 8, 0, 0),
>> -	F(75000000, P_GPLL0_OUT_DIV, 4, 0, 0),
>> +	F(37500000, P_GPLL0_OUT_MAIN, 8, 0, 0),
>> +	F(75000000, P_GPLL0_OUT_MAIN, 4, 0, 0),
> 
> Why? It's not mentioned in the commit message.
> 

As per the HW clock plan, the ahb(parent_*_2)/mdp(parent_*_3) clock RCG 
parent must be GPLL0_OUT_MAIN (gcc_disp_gpll0_div_clk_src). Updated them 
accordingly and mentioned the same in the commit text below:

"Update the parent data of mdss ahb/mdp clocks accordingly to the 
hardware clock plan"

Will move these to indices approach, and add in a separate commit as you 
mentioned in the other patch comment.

>>   	{ }
>>   };
>>   
>> @@ -450,11 +455,14 @@ static const struct qcom_reset_map disp_cc_qcm2290_resets[] = {
>>   
>>   static struct gdsc mdss_gdsc = {
>>   	.gdscr = 0x3000,
>> +	.en_rest_wait_val = 0x2,
>> +	.en_few_wait_val = 0x2,
>> +	.clk_dis_wait_val = 0xf,
>>   	.pd = {
>>   		.name = "mdss_gdsc",
>>   	},
>>   	.pwrsts = PWRSTS_OFF_ON,
>> -	.flags = HW_CTRL,
>> +	.flags = HW_CTRL_TRIGGER | POLL_CFG_GDSCR | RETAIN_FF_ENABLE,
> 
> And this also needs explanation.
> 

Sure, will add these GDSC fixes in separate commit in next series.

Thanks,
Imran

>>   };
>