Re: [PATCH] clk: qcom: gcc-qcm2290: CLK_OPS_PARENT_ENABLE flag for rcg2 clocks

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Mon 20 Dec 01:54 PST 2021, Loic Poulain wrote:

> When a rcg2 clock migrates to a new parent, both the old and new
> parent clocks must be enabled to complete the transition. This can
> be automatically performed by the clock core when a clock is flagged
> with CLK_OPS_PARENT_ENABLE.
> 
> Without this, we may hit rate update failures:
> gcc_sdcc2_apps_clk_src: rcg didn't update its configuration.
> WARNING: CPU: 1 PID: 82 at drivers/clk/qcom/clk-rcg2.c:122 update_config+0xe0/0xf0
> 
> Fixes: 496d1a13d405 ("clk: qcom: Add Global Clock Controller driver for QCM2290")
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> ---
>  drivers/clk/qcom/gcc-qcm2290.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/clk/qcom/gcc-qcm2290.c b/drivers/clk/qcom/gcc-qcm2290.c
> index b6fa7b8..9e1d88e 100644
> --- a/drivers/clk/qcom/gcc-qcm2290.c
> +++ b/drivers/clk/qcom/gcc-qcm2290.c
> @@ -674,6 +674,7 @@ static struct clk_rcg2 gcc_usb30_prim_mock_utmi_clk_src = {
>  		.name = "gcc_usb30_prim_mock_utmi_clk_src",
>  		.parent_data = gcc_parents_0,
>  		.num_parents = ARRAY_SIZE(gcc_parents_0),
> +		.flags = CLK_OPS_PARENT_ENABLE,

This seems like a correct fix for the obvious problem that we might end
up invoking clk_set_rate() and clk_set_parent() for these clocks while
their (and thereby themselves - in a software sense) are disabled.


However, clocks that downstream are marked "enable_safe_config", may in
addition be enabled by the hardware, behind out back. As such we must
ensure that they always have a valid configuration, we do this by
"parking" them on CXO whenever we're going to disable their parent
clocks.

Upstream we handle this by using the clk_rcg2_shared_ops, which will do
exactly this.


PS. Unfortunately these RCGs doesn't allow us to reliably implement
is_enabled() and as such clk_disable_unused() will skip parking the
unused clocks and continue to disable the PLLs feeding them (if they are
unused). I don't think this is a problem for the clocks you list here,
but we certainly have a problem with this in the dispcc. So this is
currently being discussed. For now it's expected that booting without
"clk_ignore_unused" is "broken".

Regards,
Bjorn

>  		.ops = &clk_rcg2_ops,
>  	},
>  };
> @@ -710,6 +711,7 @@ static struct clk_rcg2 gcc_camss_axi_clk_src = {
>  		.name = "gcc_camss_axi_clk_src",
>  		.parent_data = gcc_parents_4,
>  		.num_parents = ARRAY_SIZE(gcc_parents_4),
> +		.flags = CLK_OPS_PARENT_ENABLE,
>  		.ops = &clk_rcg2_ops,
>  	},
>  };
> @@ -730,6 +732,7 @@ static struct clk_rcg2 gcc_camss_cci_clk_src = {
>  		.name = "gcc_camss_cci_clk_src",
>  		.parent_data = gcc_parents_9,
>  		.num_parents = ARRAY_SIZE(gcc_parents_9),
> +		.flags = CLK_OPS_PARENT_ENABLE,
>  		.ops = &clk_rcg2_ops,
>  	},
>  };
> @@ -752,6 +755,7 @@ static struct clk_rcg2 gcc_camss_csi0phytimer_clk_src = {
>  		.name = "gcc_camss_csi0phytimer_clk_src",
>  		.parent_data = gcc_parents_5,
>  		.num_parents = ARRAY_SIZE(gcc_parents_5),
> +		.flags = CLK_OPS_PARENT_ENABLE,
>  		.ops = &clk_rcg2_ops,
>  	},
>  };
> @@ -766,6 +770,7 @@ static struct clk_rcg2 gcc_camss_csi1phytimer_clk_src = {
>  		.name = "gcc_camss_csi1phytimer_clk_src",
>  		.parent_data = gcc_parents_5,
>  		.num_parents = ARRAY_SIZE(gcc_parents_5),
> +		.flags = CLK_OPS_PARENT_ENABLE,
>  		.ops = &clk_rcg2_ops,
>  	},
>  };
> @@ -854,6 +859,7 @@ static struct clk_rcg2 gcc_camss_ope_ahb_clk_src = {
>  		.name = "gcc_camss_ope_ahb_clk_src",
>  		.parent_data = gcc_parents_6,
>  		.num_parents = ARRAY_SIZE(gcc_parents_6),
> +		.flags = CLK_OPS_PARENT_ENABLE,
>  		.ops = &clk_rcg2_ops,
>  	},
>  };
> @@ -912,6 +918,7 @@ static struct clk_rcg2 gcc_camss_tfe_0_clk_src = {
>  		.name = "gcc_camss_tfe_0_clk_src",
>  		.parent_data = gcc_parents_7,
>  		.num_parents = ARRAY_SIZE(gcc_parents_7),
> +		.flags = CLK_OPS_PARENT_ENABLE,
>  		.ops = &clk_rcg2_ops,
>  	},
>  };
> @@ -936,6 +943,7 @@ static struct clk_rcg2 gcc_camss_tfe_0_csid_clk_src = {
>  		.name = "gcc_camss_tfe_0_csid_clk_src",
>  		.parent_data = gcc_parents_8,
>  		.num_parents = ARRAY_SIZE(gcc_parents_8),
> +		.flags = CLK_OPS_PARENT_ENABLE,
>  		.ops = &clk_rcg2_ops,
>  	},
>  };
> @@ -950,6 +958,7 @@ static struct clk_rcg2 gcc_camss_tfe_1_clk_src = {
>  		.name = "gcc_camss_tfe_1_clk_src",
>  		.parent_data = gcc_parents_7,
>  		.num_parents = ARRAY_SIZE(gcc_parents_7),
> +		.flags = CLK_OPS_PARENT_ENABLE,
>  		.ops = &clk_rcg2_ops,
>  	},
>  };
> @@ -964,6 +973,7 @@ static struct clk_rcg2 gcc_camss_tfe_1_csid_clk_src = {
>  		.name = "gcc_camss_tfe_1_csid_clk_src",
>  		.parent_data = gcc_parents_8,
>  		.num_parents = ARRAY_SIZE(gcc_parents_8),
> +		.flags = CLK_OPS_PARENT_ENABLE,
>  		.ops = &clk_rcg2_ops,
>  	},
>  };
> @@ -1008,6 +1018,7 @@ static struct clk_rcg2 gcc_camss_top_ahb_clk_src = {
>  		.name = "gcc_camss_top_ahb_clk_src",
>  		.parent_data = gcc_parents_4,
>  		.num_parents = ARRAY_SIZE(gcc_parents_4),
> +		.flags = CLK_OPS_PARENT_ENABLE,
>  		.ops = &clk_rcg2_ops,
>  	},
>  };
> @@ -1030,6 +1041,7 @@ static struct clk_rcg2 gcc_gp1_clk_src = {
>  		.name = "gcc_gp1_clk_src",
>  		.parent_data = gcc_parents_2,
>  		.num_parents = ARRAY_SIZE(gcc_parents_2),
> +		.flags = CLK_OPS_PARENT_ENABLE,
>  		.ops = &clk_rcg2_ops,
>  	},
>  };
> @@ -1044,6 +1056,7 @@ static struct clk_rcg2 gcc_gp2_clk_src = {
>  		.name = "gcc_gp2_clk_src",
>  		.parent_data = gcc_parents_2,
>  		.num_parents = ARRAY_SIZE(gcc_parents_2),
> +		.flags = CLK_OPS_PARENT_ENABLE,
>  		.ops = &clk_rcg2_ops,
>  	},
>  };
> @@ -1058,6 +1071,7 @@ static struct clk_rcg2 gcc_gp3_clk_src = {
>  		.name = "gcc_gp3_clk_src",
>  		.parent_data = gcc_parents_2,
>  		.num_parents = ARRAY_SIZE(gcc_parents_2),
> +		.flags = CLK_OPS_PARENT_ENABLE,
>  		.ops = &clk_rcg2_ops,
>  	},
>  };
> @@ -1078,6 +1092,7 @@ static struct clk_rcg2 gcc_pdm2_clk_src = {
>  		.name = "gcc_pdm2_clk_src",
>  		.parent_data = gcc_parents_0,
>  		.num_parents = ARRAY_SIZE(gcc_parents_0),
> +		.flags = CLK_OPS_PARENT_ENABLE,
>  		.ops = &clk_rcg2_ops,
>  	},
>  };
> @@ -1220,6 +1235,7 @@ static struct clk_rcg2 gcc_sdcc1_apps_clk_src = {
>  		.name = "gcc_sdcc1_apps_clk_src",
>  		.parent_data = gcc_parents_1,
>  		.num_parents = ARRAY_SIZE(gcc_parents_1),
> +		.flags = CLK_OPS_PARENT_ENABLE,
>  		.ops = &clk_rcg2_floor_ops,
>  	},
>  };
> @@ -1243,6 +1259,7 @@ static struct clk_rcg2 gcc_sdcc1_ice_core_clk_src = {
>  		.name = "gcc_sdcc1_ice_core_clk_src",
>  		.parent_data = gcc_parents_0,
>  		.num_parents = ARRAY_SIZE(gcc_parents_0),
> +		.flags = CLK_OPS_PARENT_ENABLE,
>  		.ops = &clk_rcg2_ops,
>  	},
>  };
> @@ -1267,6 +1284,7 @@ static struct clk_rcg2 gcc_sdcc2_apps_clk_src = {
>  		.name = "gcc_sdcc2_apps_clk_src",
>  		.parent_data = gcc_parents_12,
>  		.num_parents = ARRAY_SIZE(gcc_parents_12),
> +		.flags = CLK_OPS_PARENT_ENABLE,
>  		.ops = &clk_rcg2_ops,
>  	},
>  };
> @@ -1289,6 +1307,7 @@ static struct clk_rcg2 gcc_usb30_prim_master_clk_src = {
>  		.name = "gcc_usb30_prim_master_clk_src",
>  		.parent_data = gcc_parents_0,
>  		.num_parents = ARRAY_SIZE(gcc_parents_0),
> +		.flags = CLK_OPS_PARENT_ENABLE,
>  		.ops = &clk_rcg2_ops,
>  	},
>  };
> @@ -1303,6 +1322,7 @@ static struct clk_rcg2 gcc_usb3_prim_phy_aux_clk_src = {
>  		.name = "gcc_usb3_prim_phy_aux_clk_src",
>  		.parent_data = gcc_parents_13,
>  		.num_parents = ARRAY_SIZE(gcc_parents_13),
> +		.flags = CLK_OPS_PARENT_ENABLE,
>  		.ops = &clk_rcg2_ops,
>  	},
>  };
> -- 
> 2.7.4
>