| Message ID | 1639994084-25447-1-git-send-email-loic.poulain@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | clk: qcom: gcc-qcm2290: CLK_OPS_PARENT_ENABLE flag for rcg2 clocks | expand |
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 >
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
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
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
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, }, };
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(+)