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

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

[Loic Poulain]

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,
 		.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

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

[Bjorn Andersson]

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
> 

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

[Loic Poulain]

Hi Bjorn,

On Tue, 21 Dec 2021 at 00:40, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> 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.

Ok, thanks for the explanation, so we actually need both
clk_rcg2_shared_ops and CLK_OPS_PARENT_ENABLE, the former parking the
clock under always-on CXO (safe source), allowing hardware to toggle it
on its own. The latter allows safe parent switching by enabling the
new parent clock before update (and old parent, but we don't care
since we are actually parked on CXO) . Is that correct?

> 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".

Ok, understood thanks.


Loic

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

[Bjorn Andersson]

On Tue 21 Dec 02:50 PST 2021, Loic Poulain wrote:

> Hi Bjorn,
> 
> On Tue, 21 Dec 2021 at 00:40, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > 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.
> 
> Ok, thanks for the explanation, so we actually need both
> clk_rcg2_shared_ops and CLK_OPS_PARENT_ENABLE, the former parking the
> clock under always-on CXO (safe source), allowing hardware to toggle it
> on its own. The latter allows safe parent switching by enabling the
> new parent clock before update (and old parent, but we don't care
> since we are actually parked on CXO) . Is that correct?
> 

If a clock is parked and we get a request to change its rate, then the
old parent doesn't matter (as you say). But as we are done with the
set_rate the new parent will be turned off, as such as soon as we've
changed the parent we must park the RCG again.

So for parked shared_ops RCGs we simply remember the new configuration
of the set_rate until it's time to enable the RCG again.

As such I don't think that CLK_OPS_PARENT_ENABLE adds any value (for the
shared_ops RCGs), only a bit of overhead.


That said, if I read the code correctly don't think we properly handles
a clk_set_parent() of a parked shared RCGs today...

Regards,
Bjorn

> > 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".
> 
> Ok, understood thanks.
> 
> 
> Loic

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

[Loic Poulain]

Hi Bjorn,

On Wed, 22 Dec 2021 at 01:20, Bjorn Andersson
<bjorn.andersson@linaro.org> 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.
> >
> > Ok, thanks for the explanation, so we actually need both
> > clk_rcg2_shared_ops and CLK_OPS_PARENT_ENABLE, the former parking the
> > clock under always-on CXO (safe source), allowing hardware to toggle it
> > on its own. The latter allows safe parent switching by enabling the
> > new parent clock before update (and old parent, but we don't care
> > since we are actually parked on CXO) . Is that correct?
> >
>
> If a clock is parked and we get a request to change its rate, then the
> old parent doesn't matter (as you say). But as we are done with the
> set_rate the new parent will be turned off, as such as soon as we've
> changed the parent we must park the RCG again.
>
> So for parked shared_ops RCGs we simply remember the new configuration
> of the set_rate until it's time to enable the RCG again.
>
> As such I don't think that CLK_OPS_PARENT_ENABLE adds any value (for the
> shared_ops RCGs), only a bit of overhead.
>
>
> That said, if I read the code correctly don't think we properly handles
> a clk_set_parent() of a parked shared RCGs today...

Indeed, but most of the time, clk_rcg2_set_rate_and_parent is used,
and seems correctly implemented for shared case. But in case of e.g.
assigned-parent from dts, the clk_set_parent may indeed fail. So we
need a dedicated clk_rcg2_shared_set_parent function, maybe something
like:

+static int clk_rcg2_shared_set_parent(struct clk_hw *hw, u8 index)
+{
+       struct clk_rcg2 *rcg = to_clk_rcg2(hw);
+       int ret;
+       u32 cfg = rcg->parent_map[index].cfg << CFG_SRC_SEL_SHIFT;
+
+       /*
+        * In case clock is disabled, update the CFG and don't hit the
+        * update bit of CMD register, which will be done in next enable().
+        */
+       ret = regmap_update_bits(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg),
+                                CFG_SRC_SEL_MASK, cfg);
+       if (ret || !__clk_is_enabled(hw->clk))
+               return ret;
+
+       return update_config(rcg);
+}

Regards,
Loic