| Message ID | 20220421031849.454626-1-bjorn.andersson@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v3] clk: qcom: rcg2: Cache CFG register updates for parked RCGs | expand |
Quoting Bjorn Andersson (2022-04-20 20:18:49) > As GDSCs are turned on and off some associated clocks are momentarily > enabled for house keeping purposes. For this, and similar, purposes the > "shared RCGs" will park the RCG on a source clock which is known to be > available. > When the RCG is parked, a safe clock source will be selected and > committed, then the original source would be written back and upon enable > the change back to the unparked source would be committed. > > But starting with SM8350 this fails, as the value in CFG is committed by > the GDSC handshake and without a ticking parent the GDSC enablement will > time out. > > This becomes a concrete problem if the runtime supended state of a > device includes disabling such rcg's parent clock. As the device > attempts to power up the domain again the rcg will fail to enable and > hence the GDSC enablement will fail, preventing the device from > returning from the suspended state. > > This can be seen in e.g. the display stack during probe on SM8350. > > To avoid this problem, the software needs to ensure that the RCG is > configured to a active parent clock while it is disabled. This is done > by caching the CFG register content while the shared RCG is parked on > this safe source. > > Writes to M, N and D registers are committed as they are requested. New > helpers for get_parent() and recalc_rate() are extracted from their > previous implementations and __clk_rcg2_configure() is modified to allow > it to operate on the cached value. > > Fixes: 7ef6f11887bd ("clk: qcom: Configure the RCGs to a safe source as needed") > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- Taniya, can you please review? > > Changes since v2: > - Explained the symptoms in commit message > - Reduced error in clk_rcg2_get_parent() to pr_debug() > - Reduced the size of the change in __clk_rcg2_configure() > - Polished comment in clk_rcg2_shared_set_rate() > - Use clk_hw_is_enabled() in clk_rcg2_shared_set_parent() > > drivers/clk/qcom/clk-rcg.h | 2 + > drivers/clk/qcom/clk-rcg2.c | 124 ++++++++++++++++++++++++++++-------- > 2 files changed, 100 insertions(+), 26 deletions(-) > > diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h > index 00cea508d49e..012e745794fd 100644 > --- a/drivers/clk/qcom/clk-rcg.h > +++ b/drivers/clk/qcom/clk-rcg.h > @@ -140,6 +140,7 @@ extern const struct clk_ops clk_dyn_rcg_ops; > * @freq_tbl: frequency table > * @clkr: regmap clock handle > * @cfg_off: defines the cfg register offset from the CMD_RCGR + CFG_REG > + * @parked_cfg: cached value of the CFG register for parked RCGs > */ > struct clk_rcg2 { > u32 cmd_rcgr; > @@ -150,6 +151,7 @@ struct clk_rcg2 { > const struct freq_tbl *freq_tbl; > struct clk_regmap clkr; > u8 cfg_off; > + u32 parked_cfg; > }; > > #define to_clk_rcg2(_hw) container_of(to_clk_regmap(_hw), struct clk_rcg2, clkr) > diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c > index f675fd969c4d..9d218b04aa49 100644 > --- a/drivers/clk/qcom/clk-rcg2.c > +++ b/drivers/clk/qcom/clk-rcg2.c > @@ -73,16 +73,11 @@ static int clk_rcg2_is_enabled(struct clk_hw *hw) > return (cmd & CMD_ROOT_OFF) == 0; > } > > -static u8 clk_rcg2_get_parent(struct clk_hw *hw) > +static u8 __clk_rcg2_get_parent(struct clk_hw *hw, u32 cfg) > { > struct clk_rcg2 *rcg = to_clk_rcg2(hw); > int num_parents = clk_hw_get_num_parents(hw); > - u32 cfg; > - int i, ret; > - > - ret = regmap_read(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), &cfg); > - if (ret) > - goto err; > + int i; > > cfg &= CFG_SRC_SEL_MASK; > cfg >>= CFG_SRC_SEL_SHIFT; > @@ -91,12 +86,27 @@ static u8 clk_rcg2_get_parent(struct clk_hw *hw) > if (cfg == rcg->parent_map[i].cfg) > return i; > > -err: > pr_debug("%s: Clock %s has invalid parent, using default.\n", > __func__, clk_hw_get_name(hw)); > return 0; > } > > +static u8 clk_rcg2_get_parent(struct clk_hw *hw) > +{ > + struct clk_rcg2 *rcg = to_clk_rcg2(hw); > + u32 cfg; > + int ret; > + > + ret = regmap_read(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), &cfg); > + if (ret) { > + pr_debug("%s: Unable to read CFG register for %s\n", > + __func__, clk_hw_get_name(hw)); > + return 0; > + } > + > + return __clk_rcg2_get_parent(hw, cfg); > +} > + > static int update_config(struct clk_rcg2 *rcg) > { > int count, ret; > @@ -163,12 +173,10 @@ calc_rate(unsigned long rate, u32 m, u32 n, u32 mode, u32 hid_div) > } > > static unsigned long > -clk_rcg2_recalc_rate(struct clk_hw *hw, unsigned long parent_rate) > +__clk_rcg2_recalc_rate(struct clk_hw *hw, unsigned long parent_rate, u32 cfg) > { > struct clk_rcg2 *rcg = to_clk_rcg2(hw); > - u32 cfg, hid_div, m = 0, n = 0, mode = 0, mask; > - > - regmap_read(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), &cfg); > + u32 hid_div, m = 0, n = 0, mode = 0, mask; > > if (rcg->mnd_width) { > mask = BIT(rcg->mnd_width) - 1; > @@ -189,6 +197,17 @@ clk_rcg2_recalc_rate(struct clk_hw *hw, unsigned long parent_rate) > return calc_rate(parent_rate, m, n, mode, hid_div); > } > > +static unsigned long > +clk_rcg2_recalc_rate(struct clk_hw *hw, unsigned long parent_rate) > +{ > + struct clk_rcg2 *rcg = to_clk_rcg2(hw); > + u32 cfg; > + > + regmap_read(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), &cfg); > + > + return __clk_rcg2_recalc_rate(hw, parent_rate, cfg); > +} > + > static int _freq_tbl_determine_rate(struct clk_hw *hw, const struct freq_tbl *f, > struct clk_rate_request *req, > enum freq_policy policy) > @@ -262,7 +281,8 @@ static int clk_rcg2_determine_floor_rate(struct clk_hw *hw, > return _freq_tbl_determine_rate(hw, rcg->freq_tbl, req, FLOOR); > } > > -static int __clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f) > +static int __clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f, > + u32 *_cfg) > { > u32 cfg, mask, d_val, not2d_val, n_minus_m; > struct clk_hw *hw = &rcg->clkr.hw; > @@ -304,15 +324,27 @@ static int __clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f) > cfg |= rcg->parent_map[index].cfg << CFG_SRC_SEL_SHIFT; > if (rcg->mnd_width && f->n && (f->m != f->n)) > cfg |= CFG_MODE_DUAL_EDGE; > - return regmap_update_bits(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), > - mask, cfg); > + > + *_cfg &= ~mask; > + *_cfg |= cfg; > + > + return 0; > } > > static int clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f) > { > + u32 cfg; > int ret; > > - ret = __clk_rcg2_configure(rcg, f); > + ret = regmap_read(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), &cfg); > + if (ret) > + return ret; > + > + ret = __clk_rcg2_configure(rcg, f, &cfg); > + if (ret) > + return ret; > + > + ret = regmap_write(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), cfg); > if (ret) > return ret; > > @@ -979,11 +1011,12 @@ static int clk_rcg2_shared_set_rate(struct clk_hw *hw, unsigned long rate, > return -EINVAL; > > /* > - * In case clock is disabled, update the CFG, M, N and D registers > - * and don't hit the update bit of CMD register. > + * In case clock is disabled, update the M, N and D registers, cache > + * the CFG value in parked_cfg and don't hit the update bit of CMD > + * register. > */ > if (!__clk_is_enabled(hw->clk)) > - return __clk_rcg2_configure(rcg, f); > + return __clk_rcg2_configure(rcg, f, &rcg->parked_cfg); > > return clk_rcg2_shared_force_enable_clear(hw, f); > } > @@ -1007,6 +1040,11 @@ static int clk_rcg2_shared_enable(struct clk_hw *hw) > if (ret) > return ret; > > + /* Write back the stored configuration corresponding to current rate */ > + ret = regmap_write(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, rcg->parked_cfg); > + if (ret) > + return ret; > + > ret = update_config(rcg); > if (ret) > return ret; > @@ -1017,13 +1055,12 @@ static int clk_rcg2_shared_enable(struct clk_hw *hw) > static void clk_rcg2_shared_disable(struct clk_hw *hw) > { > struct clk_rcg2 *rcg = to_clk_rcg2(hw); > - u32 cfg; > > /* > * Store current configuration as switching to safe source would clear > * the SRC and DIV of CFG register > */ > - regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, &cfg); > + regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, &rcg->parked_cfg); > > /* > * Park the RCG at a safe configuration - sourced off of safe source. > @@ -1041,17 +1078,52 @@ static void clk_rcg2_shared_disable(struct clk_hw *hw) > update_config(rcg); > > clk_rcg2_clear_force_enable(hw); > +} > > - /* Write back the stored configuration corresponding to current rate */ > - regmap_write(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, cfg); > +static u8 clk_rcg2_shared_get_parent(struct clk_hw *hw) > +{ > + struct clk_rcg2 *rcg = to_clk_rcg2(hw); > + > + /* If the shared rcg is parked used the cached cfg instead */ > + if (!__clk_is_enabled(hw->clk)) > + return __clk_rcg2_get_parent(hw, rcg->parked_cfg); > + > + return clk_rcg2_get_parent(hw); > +} > + > +static int clk_rcg2_shared_set_parent(struct clk_hw *hw, u8 index) > +{ > + struct clk_rcg2 *rcg = to_clk_rcg2(hw); > + > + /* If the shared rcg is parked only update the cached cfg */ > + if (!clk_hw_is_enabled(hw)) { > + rcg->parked_cfg &= ~CFG_SRC_SEL_MASK; > + rcg->parked_cfg |= rcg->parent_map[index].cfg << CFG_SRC_SEL_SHIFT; > + > + return 0; > + } > + > + return clk_rcg2_set_parent(hw, index); > +} > + > +static unsigned long > +clk_rcg2_shared_recalc_rate(struct clk_hw *hw, unsigned long parent_rate) > +{ > + struct clk_rcg2 *rcg = to_clk_rcg2(hw); > + > + /* If the shared rcg is parked used the cached cfg instead */ > + if (!__clk_is_enabled(hw->clk)) > + return __clk_rcg2_recalc_rate(hw, parent_rate, rcg->parked_cfg); > + > + return clk_rcg2_recalc_rate(hw, parent_rate); > } > > const struct clk_ops clk_rcg2_shared_ops = { > .enable = clk_rcg2_shared_enable, > .disable = clk_rcg2_shared_disable, > - .get_parent = clk_rcg2_get_parent, > - .set_parent = clk_rcg2_set_parent, > - .recalc_rate = clk_rcg2_recalc_rate, > + .get_parent = clk_rcg2_shared_get_parent, > + .set_parent = clk_rcg2_shared_set_parent, > + .recalc_rate = clk_rcg2_shared_recalc_rate, > .determine_rate = clk_rcg2_determine_rate, > .set_rate = clk_rcg2_shared_set_rate, > .set_rate_and_parent = clk_rcg2_shared_set_rate_and_parent, > -- > 2.35.1 >
Hello Bjorn, Few nit-picks. Thanks & Regards, Taniya Das. -----Original Message----- From: Stephen Boyd <sboyd@kernel.org> Sent: Friday, April 22, 2022 8:13 AM To: Amit Nischal <anischal@codeaurora.org>; bjorn.andersson@linaro.org; Michael Turquette <mturquette@baylibre.com>; Taniya Das (QUIC) <quic_tdas@quicinc.com> Cc: linux-arm-msm@vger.kernel.org; linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH v3] clk: qcom: rcg2: Cache CFG register updates for parked RCGs Quoting Bjorn Andersson (2022-04-20 20:18:49) > As GDSCs are turned on and off some associated clocks are momentarily > enabled for house keeping purposes. For this, and similar, purposes > the "shared RCGs" will park the RCG on a source clock which is known > to be available. > When the RCG is parked, a safe clock source will be selected and > committed, then the original source would be written back and upon > enable the change back to the unparked source would be committed. > > But starting with SM8350 this fails, as the value in CFG is committed > by the GDSC handshake and without a ticking parent the GDSC enablement > will time out. > > This becomes a concrete problem if the runtime supended state of a > device includes disabling such rcg's parent clock. As the device > attempts to power up the domain again the rcg will fail to enable and > hence the GDSC enablement will fail, preventing the device from > returning from the suspended state. > > This can be seen in e.g. the display stack during probe on SM8350. > > To avoid this problem, the software needs to ensure that the RCG is > configured to a active parent clock while it is disabled. This is done > by caching the CFG register content while the shared RCG is parked on > this safe source. > > Writes to M, N and D registers are committed as they are requested. > New helpers for get_parent() and recalc_rate() are extracted from > their previous implementations and __clk_rcg2_configure() is modified > to allow it to operate on the cached value. > > Fixes: 7ef6f11887bd ("clk: qcom: Configure the RCGs to a safe source > as needed") > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- Taniya, can you please review? > > Changes since v2: > - Explained the symptoms in commit message > - Reduced error in clk_rcg2_get_parent() to pr_debug() > - Reduced the size of the change in __clk_rcg2_configure() > - Polished comment in clk_rcg2_shared_set_rate() > - Use clk_hw_is_enabled() in clk_rcg2_shared_set_parent() > > drivers/clk/qcom/clk-rcg.h | 2 + > drivers/clk/qcom/clk-rcg2.c | 124 > ++++++++++++++++++++++++++++-------- > 2 files changed, 100 insertions(+), 26 deletions(-) > > diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h > index 00cea508d49e..012e745794fd 100644 > --- a/drivers/clk/qcom/clk-rcg.h > +++ b/drivers/clk/qcom/clk-rcg.h > @@ -140,6 +140,7 @@ extern const struct clk_ops clk_dyn_rcg_ops; > * @freq_tbl: frequency table > * @clkr: regmap clock handle > * @cfg_off: defines the cfg register offset from the CMD_RCGR + > CFG_REG > + * @parked_cfg: cached value of the CFG register for parked RCGs > */ > struct clk_rcg2 { > u32 cmd_rcgr; > @@ -150,6 +151,7 @@ struct clk_rcg2 { > const struct freq_tbl *freq_tbl; > struct clk_regmap clkr; > u8 cfg_off; > + u32 parked_cfg; > }; > > #define to_clk_rcg2(_hw) container_of(to_clk_regmap(_hw), struct > clk_rcg2, clkr) diff --git a/drivers/clk/qcom/clk-rcg2.c > b/drivers/clk/qcom/clk-rcg2.c index f675fd969c4d..9d218b04aa49 100644 > --- a/drivers/clk/qcom/clk-rcg2.c > +++ b/drivers/clk/qcom/clk-rcg2.c > @@ -73,16 +73,11 @@ static int clk_rcg2_is_enabled(struct clk_hw *hw) > return (cmd & CMD_ROOT_OFF) == 0; } > > -static u8 clk_rcg2_get_parent(struct clk_hw *hw) > +static u8 __clk_rcg2_get_parent(struct clk_hw *hw, u32 cfg) > { > struct clk_rcg2 *rcg = to_clk_rcg2(hw); > int num_parents = clk_hw_get_num_parents(hw); > - u32 cfg; > - int i, ret; > - > - ret = regmap_read(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), &cfg); > - if (ret) > - goto err; > + int i; > > cfg &= CFG_SRC_SEL_MASK; > cfg >>= CFG_SRC_SEL_SHIFT; > @@ -91,12 +86,27 @@ static u8 clk_rcg2_get_parent(struct clk_hw *hw) > if (cfg == rcg->parent_map[i].cfg) > return i; > > -err: > pr_debug("%s: Clock %s has invalid parent, using default.\n", > __func__, clk_hw_get_name(hw)); > return 0; > } > > +static u8 clk_rcg2_get_parent(struct clk_hw *hw) { > + struct clk_rcg2 *rcg = to_clk_rcg2(hw); > + u32 cfg; > + int ret; > + > + ret = regmap_read(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), &cfg); > + if (ret) { > + pr_debug("%s: Unable to read CFG register for %s\n", > + __func__, clk_hw_get_name(hw)); > + return 0; > + } > + > + return __clk_rcg2_get_parent(hw, cfg); } > + > static int update_config(struct clk_rcg2 *rcg) { > int count, ret; > @@ -163,12 +173,10 @@ calc_rate(unsigned long rate, u32 m, u32 n, u32 > mode, u32 hid_div) } > > static unsigned long > -clk_rcg2_recalc_rate(struct clk_hw *hw, unsigned long parent_rate) > +__clk_rcg2_recalc_rate(struct clk_hw *hw, unsigned long parent_rate, > +u32 cfg) > { > struct clk_rcg2 *rcg = to_clk_rcg2(hw); > - u32 cfg, hid_div, m = 0, n = 0, mode = 0, mask; > - > - regmap_read(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), &cfg); > + u32 hid_div, m = 0, n = 0, mode = 0, mask; > > if (rcg->mnd_width) { > mask = BIT(rcg->mnd_width) - 1; @@ -189,6 +197,17 @@ > clk_rcg2_recalc_rate(struct clk_hw *hw, unsigned long parent_rate) > return calc_rate(parent_rate, m, n, mode, hid_div); } > > +static unsigned long > +clk_rcg2_recalc_rate(struct clk_hw *hw, unsigned long parent_rate) { > + struct clk_rcg2 *rcg = to_clk_rcg2(hw); > + u32 cfg; > + > + regmap_read(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), &cfg); > + > + return __clk_rcg2_recalc_rate(hw, parent_rate, cfg); } > + > static int _freq_tbl_determine_rate(struct clk_hw *hw, const struct freq_tbl *f, > struct clk_rate_request *req, > enum freq_policy policy) @@ -262,7 > +281,8 @@ static int clk_rcg2_determine_floor_rate(struct clk_hw *hw, > return _freq_tbl_determine_rate(hw, rcg->freq_tbl, req, > FLOOR); } > > -static int __clk_rcg2_configure(struct clk_rcg2 *rcg, const struct > freq_tbl *f) > +static int __clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f, > + u32 *_cfg) > { > u32 cfg, mask, d_val, not2d_val, n_minus_m; > struct clk_hw *hw = &rcg->clkr.hw; @@ -304,15 +324,27 @@ > static int __clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f) > cfg |= rcg->parent_map[index].cfg << CFG_SRC_SEL_SHIFT; > if (rcg->mnd_width && f->n && (f->m != f->n)) > cfg |= CFG_MODE_DUAL_EDGE; > - return regmap_update_bits(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), > - mask, cfg); > + > + *_cfg &= ~mask; > + *_cfg |= cfg; > + > + return 0; > } > > static int clk_rcg2_configure(struct clk_rcg2 *rcg, const struct > freq_tbl *f) { > + u32 cfg; > int ret; > > - ret = __clk_rcg2_configure(rcg, f); > + ret = regmap_read(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), &cfg); > + if (ret) > + return ret; > + > + ret = __clk_rcg2_configure(rcg, f, &cfg); > + if (ret) > + return ret; > + > + ret = regmap_write(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), > + cfg); > if (ret) > return ret; > > @@ -979,11 +1011,12 @@ static int clk_rcg2_shared_set_rate(struct clk_hw *hw, unsigned long rate, > return -EINVAL; > > /* > - * In case clock is disabled, update the CFG, M, N and D registers > - * and don't hit the update bit of CMD register. > + * In case clock is disabled, update the M, N and D registers, cache > + * the CFG value in parked_cfg and don't hit the update bit of CMD > + * register. > */ > if (!__clk_is_enabled(hw->clk)) Can we use clk_hw_is_enabled(hw) instead? > - return __clk_rcg2_configure(rcg, f); > + return __clk_rcg2_configure(rcg, f, &rcg->parked_cfg); > > return clk_rcg2_shared_force_enable_clear(hw, f); } @@ > -1007,6 +1040,11 @@ static int clk_rcg2_shared_enable(struct clk_hw *hw) > if (ret) > return ret; > > + /* Write back the stored configuration corresponding to current rate */ > + ret = regmap_write(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, rcg->parked_cfg); > + if (ret) > + return ret; > + > ret = update_config(rcg); > if (ret) > return ret; > @@ -1017,13 +1055,12 @@ static int clk_rcg2_shared_enable(struct > clk_hw *hw) static void clk_rcg2_shared_disable(struct clk_hw *hw) { > struct clk_rcg2 *rcg = to_clk_rcg2(hw); > - u32 cfg; > > /* > * Store current configuration as switching to safe source would clear > * the SRC and DIV of CFG register > */ > - regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, &cfg); > + regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, > + &rcg->parked_cfg); > > /* > * Park the RCG at a safe configuration - sourced off of safe source. > @@ -1041,17 +1078,52 @@ static void clk_rcg2_shared_disable(struct clk_hw *hw) > update_config(rcg); > > clk_rcg2_clear_force_enable(hw); > +} > > - /* Write back the stored configuration corresponding to current rate */ > - regmap_write(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, cfg); > +static u8 clk_rcg2_shared_get_parent(struct clk_hw *hw) { > + struct clk_rcg2 *rcg = to_clk_rcg2(hw); > + > + /* If the shared rcg is parked used the cached cfg instead */ s/used/use > + if (!__clk_is_enabled(hw->clk)) Can we use clk_hw_is_enabled(hw)? > + return __clk_rcg2_get_parent(hw, rcg->parked_cfg); > + > + return clk_rcg2_get_parent(hw); } > + > +static int clk_rcg2_shared_set_parent(struct clk_hw *hw, u8 index) { > + struct clk_rcg2 *rcg = to_clk_rcg2(hw); > + > + /* If the shared rcg is parked only update the cached cfg */ > + if (!clk_hw_is_enabled(hw)) { > + rcg->parked_cfg &= ~CFG_SRC_SEL_MASK; > + rcg->parked_cfg |= rcg->parent_map[index].cfg << > + CFG_SRC_SEL_SHIFT; > + > + return 0; > + } > + > + return clk_rcg2_set_parent(hw, index); } > + > +static unsigned long > +clk_rcg2_shared_recalc_rate(struct clk_hw *hw, unsigned long > +parent_rate) { > + struct clk_rcg2 *rcg = to_clk_rcg2(hw); > + > + /* If the shared rcg is parked used the cached cfg instead */ s/used/use > + if (!__clk_is_enabled(hw->clk)) Can we use clk_hw_is_enabled(hw)? > + return __clk_rcg2_recalc_rate(hw, parent_rate, > + rcg->parked_cfg); > + > + return clk_rcg2_recalc_rate(hw, parent_rate); > } > > const struct clk_ops clk_rcg2_shared_ops = { > .enable = clk_rcg2_shared_enable, > .disable = clk_rcg2_shared_disable, > - .get_parent = clk_rcg2_get_parent, > - .set_parent = clk_rcg2_set_parent, > - .recalc_rate = clk_rcg2_recalc_rate, > + .get_parent = clk_rcg2_shared_get_parent, > + .set_parent = clk_rcg2_shared_set_parent, > + .recalc_rate = clk_rcg2_shared_recalc_rate, > .determine_rate = clk_rcg2_determine_rate, > .set_rate = clk_rcg2_shared_set_rate, > .set_rate_and_parent = clk_rcg2_shared_set_rate_and_parent, > -- > 2.35.1 >
diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h index 00cea508d49e..012e745794fd 100644 --- a/drivers/clk/qcom/clk-rcg.h +++ b/drivers/clk/qcom/clk-rcg.h @@ -140,6 +140,7 @@ extern const struct clk_ops clk_dyn_rcg_ops; * @freq_tbl: frequency table * @clkr: regmap clock handle * @cfg_off: defines the cfg register offset from the CMD_RCGR + CFG_REG + * @parked_cfg: cached value of the CFG register for parked RCGs */ struct clk_rcg2 { u32 cmd_rcgr; @@ -150,6 +151,7 @@ struct clk_rcg2 { const struct freq_tbl *freq_tbl; struct clk_regmap clkr; u8 cfg_off; + u32 parked_cfg; }; #define to_clk_rcg2(_hw) container_of(to_clk_regmap(_hw), struct clk_rcg2, clkr) diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c index f675fd969c4d..9d218b04aa49 100644 --- a/drivers/clk/qcom/clk-rcg2.c +++ b/drivers/clk/qcom/clk-rcg2.c @@ -73,16 +73,11 @@ static int clk_rcg2_is_enabled(struct clk_hw *hw) return (cmd & CMD_ROOT_OFF) == 0; } -static u8 clk_rcg2_get_parent(struct clk_hw *hw) +static u8 __clk_rcg2_get_parent(struct clk_hw *hw, u32 cfg) { struct clk_rcg2 *rcg = to_clk_rcg2(hw); int num_parents = clk_hw_get_num_parents(hw); - u32 cfg; - int i, ret; - - ret = regmap_read(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), &cfg); - if (ret) - goto err; + int i; cfg &= CFG_SRC_SEL_MASK; cfg >>= CFG_SRC_SEL_SHIFT; @@ -91,12 +86,27 @@ static u8 clk_rcg2_get_parent(struct clk_hw *hw) if (cfg == rcg->parent_map[i].cfg) return i; -err: pr_debug("%s: Clock %s has invalid parent, using default.\n", __func__, clk_hw_get_name(hw)); return 0; } +static u8 clk_rcg2_get_parent(struct clk_hw *hw) +{ + struct clk_rcg2 *rcg = to_clk_rcg2(hw); + u32 cfg; + int ret; + + ret = regmap_read(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), &cfg); + if (ret) { + pr_debug("%s: Unable to read CFG register for %s\n", + __func__, clk_hw_get_name(hw)); + return 0; + } + + return __clk_rcg2_get_parent(hw, cfg); +} + static int update_config(struct clk_rcg2 *rcg) { int count, ret; @@ -163,12 +173,10 @@ calc_rate(unsigned long rate, u32 m, u32 n, u32 mode, u32 hid_div) } static unsigned long -clk_rcg2_recalc_rate(struct clk_hw *hw, unsigned long parent_rate) +__clk_rcg2_recalc_rate(struct clk_hw *hw, unsigned long parent_rate, u32 cfg) { struct clk_rcg2 *rcg = to_clk_rcg2(hw); - u32 cfg, hid_div, m = 0, n = 0, mode = 0, mask; - - regmap_read(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), &cfg); + u32 hid_div, m = 0, n = 0, mode = 0, mask; if (rcg->mnd_width) { mask = BIT(rcg->mnd_width) - 1; @@ -189,6 +197,17 @@ clk_rcg2_recalc_rate(struct clk_hw *hw, unsigned long parent_rate) return calc_rate(parent_rate, m, n, mode, hid_div); } +static unsigned long +clk_rcg2_recalc_rate(struct clk_hw *hw, unsigned long parent_rate) +{ + struct clk_rcg2 *rcg = to_clk_rcg2(hw); + u32 cfg; + + regmap_read(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), &cfg); + + return __clk_rcg2_recalc_rate(hw, parent_rate, cfg); +} + static int _freq_tbl_determine_rate(struct clk_hw *hw, const struct freq_tbl *f, struct clk_rate_request *req, enum freq_policy policy) @@ -262,7 +281,8 @@ static int clk_rcg2_determine_floor_rate(struct clk_hw *hw, return _freq_tbl_determine_rate(hw, rcg->freq_tbl, req, FLOOR); } -static int __clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f) +static int __clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f, + u32 *_cfg) { u32 cfg, mask, d_val, not2d_val, n_minus_m; struct clk_hw *hw = &rcg->clkr.hw; @@ -304,15 +324,27 @@ static int __clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f) cfg |= rcg->parent_map[index].cfg << CFG_SRC_SEL_SHIFT; if (rcg->mnd_width && f->n && (f->m != f->n)) cfg |= CFG_MODE_DUAL_EDGE; - return regmap_update_bits(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), - mask, cfg); + + *_cfg &= ~mask; + *_cfg |= cfg; + + return 0; } static int clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f) { + u32 cfg; int ret; - ret = __clk_rcg2_configure(rcg, f); + ret = regmap_read(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), &cfg); + if (ret) + return ret; + + ret = __clk_rcg2_configure(rcg, f, &cfg); + if (ret) + return ret; + + ret = regmap_write(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), cfg); if (ret) return ret; @@ -979,11 +1011,12 @@ static int clk_rcg2_shared_set_rate(struct clk_hw *hw, unsigned long rate, return -EINVAL; /* - * In case clock is disabled, update the CFG, M, N and D registers - * and don't hit the update bit of CMD register. + * In case clock is disabled, update the M, N and D registers, cache + * the CFG value in parked_cfg and don't hit the update bit of CMD + * register. */ if (!__clk_is_enabled(hw->clk)) - return __clk_rcg2_configure(rcg, f); + return __clk_rcg2_configure(rcg, f, &rcg->parked_cfg); return clk_rcg2_shared_force_enable_clear(hw, f); } @@ -1007,6 +1040,11 @@ static int clk_rcg2_shared_enable(struct clk_hw *hw) if (ret) return ret; + /* Write back the stored configuration corresponding to current rate */ + ret = regmap_write(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, rcg->parked_cfg); + if (ret) + return ret; + ret = update_config(rcg); if (ret) return ret; @@ -1017,13 +1055,12 @@ static int clk_rcg2_shared_enable(struct clk_hw *hw) static void clk_rcg2_shared_disable(struct clk_hw *hw) { struct clk_rcg2 *rcg = to_clk_rcg2(hw); - u32 cfg; /* * Store current configuration as switching to safe source would clear * the SRC and DIV of CFG register */ - regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, &cfg); + regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, &rcg->parked_cfg); /* * Park the RCG at a safe configuration - sourced off of safe source. @@ -1041,17 +1078,52 @@ static void clk_rcg2_shared_disable(struct clk_hw *hw) update_config(rcg); clk_rcg2_clear_force_enable(hw); +} - /* Write back the stored configuration corresponding to current rate */ - regmap_write(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, cfg); +static u8 clk_rcg2_shared_get_parent(struct clk_hw *hw) +{ + struct clk_rcg2 *rcg = to_clk_rcg2(hw); + + /* If the shared rcg is parked used the cached cfg instead */ + if (!__clk_is_enabled(hw->clk)) + return __clk_rcg2_get_parent(hw, rcg->parked_cfg); + + return clk_rcg2_get_parent(hw); +} + +static int clk_rcg2_shared_set_parent(struct clk_hw *hw, u8 index) +{ + struct clk_rcg2 *rcg = to_clk_rcg2(hw); + + /* If the shared rcg is parked only update the cached cfg */ + if (!clk_hw_is_enabled(hw)) { + rcg->parked_cfg &= ~CFG_SRC_SEL_MASK; + rcg->parked_cfg |= rcg->parent_map[index].cfg << CFG_SRC_SEL_SHIFT; + + return 0; + } + + return clk_rcg2_set_parent(hw, index); +} + +static unsigned long +clk_rcg2_shared_recalc_rate(struct clk_hw *hw, unsigned long parent_rate) +{ + struct clk_rcg2 *rcg = to_clk_rcg2(hw); + + /* If the shared rcg is parked used the cached cfg instead */ + if (!__clk_is_enabled(hw->clk)) + return __clk_rcg2_recalc_rate(hw, parent_rate, rcg->parked_cfg); + + return clk_rcg2_recalc_rate(hw, parent_rate); } const struct clk_ops clk_rcg2_shared_ops = { .enable = clk_rcg2_shared_enable, .disable = clk_rcg2_shared_disable, - .get_parent = clk_rcg2_get_parent, - .set_parent = clk_rcg2_set_parent, - .recalc_rate = clk_rcg2_recalc_rate, + .get_parent = clk_rcg2_shared_get_parent, + .set_parent = clk_rcg2_shared_set_parent, + .recalc_rate = clk_rcg2_shared_recalc_rate, .determine_rate = clk_rcg2_determine_rate, .set_rate = clk_rcg2_shared_set_rate, .set_rate_and_parent = clk_rcg2_shared_set_rate_and_parent,
As GDSCs are turned on and off some associated clocks are momentarily enabled for house keeping purposes. For this, and similar, purposes the "shared RCGs" will park the RCG on a source clock which is known to be available. When the RCG is parked, a safe clock source will be selected and committed, then the original source would be written back and upon enable the change back to the unparked source would be committed. But starting with SM8350 this fails, as the value in CFG is committed by the GDSC handshake and without a ticking parent the GDSC enablement will time out. This becomes a concrete problem if the runtime supended state of a device includes disabling such rcg's parent clock. As the device attempts to power up the domain again the rcg will fail to enable and hence the GDSC enablement will fail, preventing the device from returning from the suspended state. This can be seen in e.g. the display stack during probe on SM8350. To avoid this problem, the software needs to ensure that the RCG is configured to a active parent clock while it is disabled. This is done by caching the CFG register content while the shared RCG is parked on this safe source. Writes to M, N and D registers are committed as they are requested. New helpers for get_parent() and recalc_rate() are extracted from their previous implementations and __clk_rcg2_configure() is modified to allow it to operate on the cached value. Fixes: 7ef6f11887bd ("clk: qcom: Configure the RCGs to a safe source as needed") Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- Changes since v2: - Explained the symptoms in commit message - Reduced error in clk_rcg2_get_parent() to pr_debug() - Reduced the size of the change in __clk_rcg2_configure() - Polished comment in clk_rcg2_shared_set_rate() - Use clk_hw_is_enabled() in clk_rcg2_shared_set_parent() drivers/clk/qcom/clk-rcg.h | 2 + drivers/clk/qcom/clk-rcg2.c | 124 ++++++++++++++++++++++++++++-------- 2 files changed, 100 insertions(+), 26 deletions(-)