Re: [PATCH v2 1/2] clk: qcom: dispcc-sc7180: Only park display clks at init

[Neil Armstrong <neil.armstrong@xxxxxxxxxx>]

On 28/08/2024 01:12, Stephen Boyd wrote:
> Amit Pundir reports that audio and USB-C host mode stops working on
> sm8550 if the gcc_usb30_prim_master_clk_src clk is registered and
> clk_rcg2_shared_init() parks it on XO.

Why does it change the dispcc-sc7180 in this case ?

Neil

> Partially revert commit 01a0a6cc8cfd ("clk: qcom: Park shared RCGs upon
> registration") by skipping the parking bit, and make that the default
> for shared RCGs, but keep the part where we cache the config register as
> that's still necessary to figure out the true parent of the clk at
> registration time. Move the logic from clk_rcg2_shared_init() to another
> clk_ops structure for use by the display clks on sc7180 to minimize the
> impact of that commit on other qcom SoCs that can't handle the parking
> part.
>
> Fixes: 01a0a6cc8cfd ("clk: qcom: Park shared RCGs upon registration")
> Cc: Konrad Dybcio <konradybcio@xxxxxxxxxx>
> Cc: Bjorn Andersson <andersson@xxxxxxxxxx>
> Cc: Taniya Das <quic_tdas@xxxxxxxxxxx>
> Cc: Neil Armstrong <neil.armstrong@xxxxxxxxxx>
> Reported-by: Amit Pundir <amit.pundir@xxxxxxxxxx>
> Closes: https://lore.kernel.org/CAMi1Hd1KQBE4kKUdAn8E5FV+BiKzuv+8FoyWQrrTHPDoYTuhgA@xxxxxxxxxxxxxx
> Signed-off-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> ---
>   drivers/clk/qcom/clk-rcg.h       |  1 +
>   drivers/clk/qcom/clk-rcg2.c      | 36 +++++++++++++++++++++++++++++---
>   drivers/clk/qcom/dispcc-sc7180.c |  8 +++----
>   3 files changed, 38 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h
> index d7414361e432..5f479a29d969 100644
> --- a/drivers/clk/qcom/clk-rcg.h
> +++ b/drivers/clk/qcom/clk-rcg.h
> @@ -198,6 +198,7 @@ extern const struct clk_ops clk_byte2_ops;
>   extern const struct clk_ops clk_pixel_ops;
>   extern const struct clk_ops clk_gfx3d_ops;
>   extern const struct clk_ops clk_rcg2_shared_ops;
> +extern const struct clk_ops clk_rcg2_shared_init_park_ops;
>   extern const struct clk_ops clk_dp_ops;
>   
>   struct clk_rcg_dfs_data {
> diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
> index 30b19bd39d08..5f0b729d7115 100644
> --- a/drivers/clk/qcom/clk-rcg2.c
> +++ b/drivers/clk/qcom/clk-rcg2.c
> @@ -1305,6 +1305,31 @@ clk_rcg2_shared_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
>   }
>   
>   static int clk_rcg2_shared_init(struct clk_hw *hw)
> +{
> +	struct clk_rcg2 *rcg = to_clk_rcg2(hw);
> +
> +	/*
> +	 * Cache the cfg so that the parent is properly mapped at registration.
> +	 */
> +	regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, &rcg->parked_cfg);
> +
> +	return 0;
> +}
> +
> +const struct clk_ops clk_rcg2_shared_ops = {
> +	.init = clk_rcg2_shared_init,
> +	.enable = clk_rcg2_shared_enable,
> +	.disable = clk_rcg2_shared_disable,
> +	.get_parent = clk_rcg2_shared_get_parent,
> +	.set_parent = clk_rcg2_shared_set_parent,
> +	.recalc_rate = clk_rcg2_shared_recalc_rate,
> +	.determine_rate = clk_rcg2_determine_rate,
> +	.set_rate = clk_rcg2_shared_set_rate,
> +	.set_rate_and_parent = clk_rcg2_shared_set_rate_and_parent,
> +};
> +EXPORT_SYMBOL_GPL(clk_rcg2_shared_ops);
> +
> +static int clk_rcg2_shared_init_park(struct clk_hw *hw)
>   {
>   	/*
>   	 * This does a few things:
> @@ -1335,8 +1360,13 @@ static int clk_rcg2_shared_init(struct clk_hw *hw)
>   	return 0;
>   }
>   
> -const struct clk_ops clk_rcg2_shared_ops = {
> -	.init = clk_rcg2_shared_init,
> +/*
> + * Like clk_rcg2_shared_ops but also park the clk at init to avoid the parent
> + * from being shutdown, getting the clk stuck when it is turned on
> + * automatically by the GDSC.
> + */
> +const struct clk_ops clk_rcg2_shared_init_park_ops = {
> +	.init = clk_rcg2_shared_init_park,
>   	.enable = clk_rcg2_shared_enable,
>   	.disable = clk_rcg2_shared_disable,
>   	.get_parent = clk_rcg2_shared_get_parent,
> @@ -1346,7 +1376,7 @@ const struct clk_ops clk_rcg2_shared_ops = {
>   	.set_rate = clk_rcg2_shared_set_rate,
>   	.set_rate_and_parent = clk_rcg2_shared_set_rate_and_parent,
>   };
> -EXPORT_SYMBOL_GPL(clk_rcg2_shared_ops);
> +EXPORT_SYMBOL_GPL(clk_rcg2_shared_init_park_ops);
>   
>   /* Common APIs to be used for DFS based RCGR */
>   static void clk_rcg2_dfs_populate_freq(struct clk_hw *hw, unsigned int l,
> diff --git a/drivers/clk/qcom/dispcc-sc7180.c b/drivers/clk/qcom/dispcc-sc7180.c
> index 4710247be530..068af819f23e 100644
> --- a/drivers/clk/qcom/dispcc-sc7180.c
> +++ b/drivers/clk/qcom/dispcc-sc7180.c
> @@ -154,7 +154,7 @@ static struct clk_rcg2 disp_cc_mdss_ahb_clk_src = {
>   		.parent_data = disp_cc_parent_data_4,
>   		.num_parents = ARRAY_SIZE(disp_cc_parent_data_4),
>   		.flags = CLK_SET_RATE_PARENT,
> -		.ops = &clk_rcg2_shared_ops,
> +		.ops = &clk_rcg2_shared_init_park_ops,
>   	},
>   };
>   
> @@ -263,7 +263,7 @@ static struct clk_rcg2 disp_cc_mdss_mdp_clk_src = {
>   		.name = "disp_cc_mdss_mdp_clk_src",
>   		.parent_data = disp_cc_parent_data_3,
>   		.num_parents = ARRAY_SIZE(disp_cc_parent_data_3),
> -		.ops = &clk_rcg2_shared_ops,
> +		.ops = &clk_rcg2_shared_init_park_ops,
>   	},
>   };
>   
> @@ -291,7 +291,7 @@ static struct clk_rcg2 disp_cc_mdss_rot_clk_src = {
>   		.name = "disp_cc_mdss_rot_clk_src",
>   		.parent_data = disp_cc_parent_data_3,
>   		.num_parents = ARRAY_SIZE(disp_cc_parent_data_3),
> -		.ops = &clk_rcg2_shared_ops,
> +		.ops = &clk_rcg2_shared_init_park_ops,
>   	},
>   };
>   
> @@ -305,7 +305,7 @@ static struct clk_rcg2 disp_cc_mdss_vsync_clk_src = {
>   		.name = "disp_cc_mdss_vsync_clk_src",
>   		.parent_data = disp_cc_parent_data_0,
>   		.num_parents = ARRAY_SIZE(disp_cc_parent_data_0),
> -		.ops = &clk_rcg2_shared_ops,
> +		.ops = &clk_rcg2_shared_init_park_ops,
>   	},
>   };
>