From: Taniya Das <[email protected]>
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 <[email protected]>
---
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)) {
+ 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, 12 Jul 2023 at 04:49, Satya Priya Kakitapalli
<[email protected]> wrote:
>
> From: Taniya Das <[email protected]>
>
> 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 <[email protected]>
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
>
--
With best wishes
Dmitry
On Wed, Jul 12, 2023 at 07:18:12AM +0530, Satya Priya Kakitapalli wrote:
> From: Taniya Das <[email protected]>
>
> 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 <[email protected]>
>
> 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 <[email protected]>
> ---
> Resending this patch as there is no review for 2 months.
>
Thanks for bumping the discussion.
Regards,
Bjorn