| Message ID | 20230712014812.3337992-1-quic_skakitap@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [RESEND] clk: qcom: rcg: Update rcg configuration before enabling it | expand |
On Wed, 12 Jul 2023 at 04:49, Satya Priya Kakitapalli <quic_skakitap@quicinc.com> wrote: > > From: Taniya Das <quic_tdas@quicinc.com> > > If rcg is in disabled state when clk_rcg2_shared_set_rate is called, the > new configuration is written to the configuration register but it won't be > effective in h/w yet because update bit won't be set if rcg is in disabled > state. Since the new configuration is not yet updated in h/w, dirty bit of > configuration register will be set in such case. Clear the dirty bit and > update the rcg to proper new configuration by setting the update bit before > enabling the rcg. > > Signed-off-by: Taniya Das <quic_tdas@quicinc.com> I think there should be your S-o-B too, as you are resending this patch. Next, should there be any Fixes tags? Probably this is a fix for 703db1f5da1e ? > --- > Resending this patch as there is no review for 2 months. > > drivers/clk/qcom/clk-rcg2.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c > index e22baf3a7112..b25635feb617 100644 > --- a/drivers/clk/qcom/clk-rcg2.c > +++ b/drivers/clk/qcom/clk-rcg2.c > @@ -982,7 +982,13 @@ static int clk_rcg2_set_force_enable(struct clk_hw *hw) > { > struct clk_rcg2 *rcg = to_clk_rcg2(hw); > const char *name = clk_hw_get_name(hw); > - int ret, count; > + int ret, count, val; > + > + if (!__clk_is_enabled(hw->clk)) { There is clk_hw_is_enabled(). Can you consider using it instead? Also, I'd like to point out the traditional source of controversy, confusion and troubles. The clk_rcg2_shared_ops does not have the is_enabled callback. Will this code work as expected in this case? > + regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + CMD_REG, &val); > + if (val & CMD_DIRTY_CFG) > + update_config(rcg); > + } > > ret = regmap_update_bits(rcg->clkr.regmap, rcg->cmd_rcgr + CMD_REG, > CMD_ROOT_EN, CMD_ROOT_EN); > -- > 2.25.1 >
On Wed, Jul 12, 2023 at 07:18:12AM +0530, Satya Priya Kakitapalli wrote: > From: Taniya Das <quic_tdas@quicinc.com> > > If rcg is in disabled state when clk_rcg2_shared_set_rate is called, the > new configuration is written to the configuration register but it won't be > effective in h/w yet because update bit won't be set if rcg is in disabled > state. Since the new configuration is not yet updated in h/w, dirty bit of > configuration register will be set in such case. Clear the dirty bit and > update the rcg to proper new configuration by setting the update bit before > enabling the rcg. > If I understand correctly you're saying that without this patch: devm_clk_get(); clk_set_rate(rate); clk_prepare_enable(); would look like it worked (i.e. clk_get_rate() would return rate), but in reality the clock is running at whatever the "default" rate is. That sounds like it could use a Fixes: tag if so! Thanks, Andrew
On Wed, Jul 12, 2023 at 07:18:12AM +0530, Satya Priya Kakitapalli wrote: > From: Taniya Das <quic_tdas@quicinc.com> > > If rcg is in disabled state when clk_rcg2_shared_set_rate is called, the > new configuration is written to the configuration register but it won't be > effective in h/w yet because update bit won't be set if rcg is in disabled > state. Does this take commit '703db1f5da1e ("clk: qcom: rcg2: Cache CFG register updates for parked RCGs")', which was merged in v5.19, into consideration? > Since the new configuration is not yet updated in h/w, dirty bit of > configuration register will be set in such case. Clear the dirty bit and > update the rcg to proper new configuration by setting the update bit before > enabling the rcg. > For a shared rcg2, which was updated while disabled, updates will be carried in the "parked_cfg" variable and the RCG_CFG will be stale so invoking update_config() should lead to exactly the problem you describe fixing here. Perhaps I'm missing something here, can you please confirm that this has been validated on a recent upstream kernel? > Signed-off-by: Taniya Das <quic_tdas@quicinc.com> > --- > Resending this patch as there is no review for 2 months. > Thanks for bumping the discussion. Regards, Bjorn
diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c index e22baf3a7112..b25635feb617 100644 --- a/drivers/clk/qcom/clk-rcg2.c +++ b/drivers/clk/qcom/clk-rcg2.c @@ -982,7 +982,13 @@ static int clk_rcg2_set_force_enable(struct clk_hw *hw) { struct clk_rcg2 *rcg = to_clk_rcg2(hw); const char *name = clk_hw_get_name(hw); - int ret, count; + int ret, count, val; + + if (!__clk_is_enabled(hw->clk)) { + regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + CMD_REG, &val); + if (val & CMD_DIRTY_CFG) + update_config(rcg); + } ret = regmap_update_bits(rcg->clkr.regmap, rcg->cmd_rcgr + CMD_REG, CMD_ROOT_EN, CMD_ROOT_EN);