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

[Dmitry Baryshkov]

On Fri, May 29, 2026 at 02:53:45PM +0530, Imran Shaik wrote:
> 
> 
> 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@xxxxxxxxxxxxxxxx>
> > > ---
> > >   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.


Why are you trying to move to indices?

> > >   	{ }
> > >   };
> > > @@ -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.

With the proper justification, please.

> 
> Thanks,
> Imran
> 
> > >   };
> > 
> 

-- 
With best wishes
Dmitry