| Message ID | 20230427150717.20860-3-ansuelsmth@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | clk: qcom: clk-rcg2: introduce support for multiple conf for same freq | expand |
On Sat, May 27, 2023 at 06:11:16PM +0200, Konrad Dybcio wrote: > > > On 27.04.2023 17:07, Christian Marangi wrote: > > Some RCG frequency can be reached by multiple configuration. > > > > Add clk_rcg2_fm_ops ops to support these special RCG configurations. > > > > These alternative ops will select the frequency using a CEIL policy. > > > > When the correct frequency is found, the correct config is selected by > > calculating the final rate (by checking the defined parent and values > > in the config that is being checked) and deciding based on the one that > > is less different than the requested one. > > > > These check are skipped if there is just on config for the requested > > freq. > > > > qcom_find_freq_multi is added to search the freq with the new struct > > freq_multi_tbl. > > __clk_rcg2_select_conf is used to select the correct conf by simulating > > the final clock. > > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > --- > > drivers/clk/qcom/clk-rcg.h | 1 + > > drivers/clk/qcom/clk-rcg2.c | 152 ++++++++++++++++++++++++++++++++++++ > > drivers/clk/qcom/common.c | 18 +++++ > > drivers/clk/qcom/common.h | 2 + > > 4 files changed, 173 insertions(+) > > > > diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h > > index dc85b46b0d79..f8ec989ed3d9 100644 > > --- a/drivers/clk/qcom/clk-rcg.h > > +++ b/drivers/clk/qcom/clk-rcg.h > > @@ -188,6 +188,7 @@ struct clk_rcg2_gfx3d { > > > > extern const struct clk_ops clk_rcg2_ops; > > extern const struct clk_ops clk_rcg2_floor_ops; > > +extern const struct clk_ops clk_rcg2_fm_ops; > > extern const struct clk_ops clk_rcg2_mux_closest_ops; > > extern const struct clk_ops clk_edp_pixel_ops; > > extern const struct clk_ops clk_byte_ops; > > diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c > > index 76551534f10d..4f2fe012ef5f 100644 > > --- a/drivers/clk/qcom/clk-rcg2.c > > +++ b/drivers/clk/qcom/clk-rcg2.c > > @@ -266,6 +266,104 @@ static int _freq_tbl_determine_rate(struct clk_hw *hw, const struct freq_tbl *f, > > return 0; > > } > > > > +static const struct freq_conf * > > +__clk_rcg2_select_conf(struct clk_hw *hw, const struct freq_multi_tbl *f, > > + unsigned long req_rate) > > +{ > > + unsigned long best_rate = 0, parent_rate, rate; > > + const struct freq_conf *conf, *best_conf; > > + struct clk_rcg2 *rcg = to_clk_rcg2(hw); > > + struct clk_hw *p; > > + int index, i; > > + > > + /* Exit early if only one config is defined */ > > + if (f->num_confs == 1) > > + return f->confs; > > + > > + /* Search in each provided config the one that is near the wanted rate */ > > + for (i = 0, conf = f->confs; i < f->num_confs; i++, conf++) { > > + index = qcom_find_src_index(hw, rcg->parent_map, conf->src); > > + if (index < 0) > > + continue; > > + > > + p = clk_hw_get_parent_by_index(hw, index); > > + if (!p) > > + continue; > > + > > + parent_rate = clk_hw_get_rate(p); > > + rate = calc_rate(parent_rate, conf->n, conf->m, conf->n, conf->pre_div); > > + > > + if (rate == req_rate) { > > + best_conf = conf; > > + break; > > + } > > + > > + if (abs(req_rate - rate) < abs(best_rate - rate)) { > Shouldn't this be: > > if (abs(req_rate - rate) < abs(best_rate - req_rate) > > ? > > this way it'd say > > "if this iteration's rate is closer to the requested one than the > best one we've found yet, it's better" > Hi, thanks for the review! I wonder if even better would be something where we save the best rate diff and just compare that. rate_diff = abs(req_rate - rate) if (rate_diff < best_rate_diff) { best_rate_diff = rate_diff; best_conf = conf; } And best_rate_diff init to ULONG_MAX? > > + best_rate = rate; > > + best_conf = conf; > > + } > > + } > > + > > + /* > > + * Very unlikely. > > + * Force the first conf if we can't find a correct config. > > + */ > > + if (unlikely(i == f->num_confs)) > > + best_conf = f->confs; > Is that a supported scenario or would it be a device driver / clock > driver error? > It's to handle case for the 2 continue in the loop and arriving in a situation where best_conf was never set? Should we return a warning and an ERR_PTR? Idea was to provide a best effort selection. > > + > > + return best_conf; > > +} > > + > > +static int _freq_tbl_fm_determine_rate(struct clk_hw *hw, const struct freq_multi_tbl *f, > > + struct clk_rate_request *req) > > +{ > > + unsigned long clk_flags, rate = req->rate; > > + const struct freq_conf *conf; > > + struct clk_hw *p; > > + struct clk_rcg2 *rcg = to_clk_rcg2(hw); > swap lines 2, 3, 4 to 4, 2, 3 and you'll get a revers-Christmas-tree! > Thanks, didn't notice this. Will do in v5.
Quoting Christian Marangi (2023-05-29 05:34:57) > On Mon, May 29, 2023 at 02:12:23PM +0200, Konrad Dybcio wrote: > > On 28.05.2023 14:37, Christian Marangi wrote: > > > On Sat, May 27, 2023 at 06:11:16PM +0200, Konrad Dybcio wrote: > > >> On 27.04.2023 17:07, Christian Marangi wrote: > > >>> + * Force the first conf if we can't find a correct config. > > >>> + */ > > >>> + if (unlikely(i == f->num_confs)) > > >>> + best_conf = f->confs; > > >> Is that a supported scenario or would it be a device driver / clock > > >> driver error? > > >> > > > > > > It's to handle case for the 2 continue in the loop and arriving in a > > > situation where best_conf was never set? > > > > > > Should we return a warning and an ERR_PTR? Idea was to provide a best > > > effort selection. > > Hm.. I'm not sure what's the expected behavior here.. Stephen? > > > > I have this implementation rady, if you want I can send this revision > and discuss that in v5 directly. It's WARN and returning -EINVAL. I'd only have a WARN if you never expect to hit that case. Otherwise, it should return -EINVAL and not warn. At a quick glance it sounds like some sort of rounding policy, so just make sure the round_rate/determine_rate implementation agrees with what set_rate() will do and it should be good.
diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h index dc85b46b0d79..f8ec989ed3d9 100644 --- a/drivers/clk/qcom/clk-rcg.h +++ b/drivers/clk/qcom/clk-rcg.h @@ -188,6 +188,7 @@ struct clk_rcg2_gfx3d { extern const struct clk_ops clk_rcg2_ops; extern const struct clk_ops clk_rcg2_floor_ops; +extern const struct clk_ops clk_rcg2_fm_ops; extern const struct clk_ops clk_rcg2_mux_closest_ops; extern const struct clk_ops clk_edp_pixel_ops; extern const struct clk_ops clk_byte_ops; diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c index 76551534f10d..4f2fe012ef5f 100644 --- a/drivers/clk/qcom/clk-rcg2.c +++ b/drivers/clk/qcom/clk-rcg2.c @@ -266,6 +266,104 @@ static int _freq_tbl_determine_rate(struct clk_hw *hw, const struct freq_tbl *f, return 0; } +static const struct freq_conf * +__clk_rcg2_select_conf(struct clk_hw *hw, const struct freq_multi_tbl *f, + unsigned long req_rate) +{ + unsigned long best_rate = 0, parent_rate, rate; + const struct freq_conf *conf, *best_conf; + struct clk_rcg2 *rcg = to_clk_rcg2(hw); + struct clk_hw *p; + int index, i; + + /* Exit early if only one config is defined */ + if (f->num_confs == 1) + return f->confs; + + /* Search in each provided config the one that is near the wanted rate */ + for (i = 0, conf = f->confs; i < f->num_confs; i++, conf++) { + index = qcom_find_src_index(hw, rcg->parent_map, conf->src); + if (index < 0) + continue; + + p = clk_hw_get_parent_by_index(hw, index); + if (!p) + continue; + + parent_rate = clk_hw_get_rate(p); + rate = calc_rate(parent_rate, conf->n, conf->m, conf->n, conf->pre_div); + + if (rate == req_rate) { + best_conf = conf; + break; + } + + if (abs(req_rate - rate) < abs(best_rate - rate)) { + best_rate = rate; + best_conf = conf; + } + } + + /* + * Very unlikely. + * Force the first conf if we can't find a correct config. + */ + if (unlikely(i == f->num_confs)) + best_conf = f->confs; + + return best_conf; +} + +static int _freq_tbl_fm_determine_rate(struct clk_hw *hw, const struct freq_multi_tbl *f, + struct clk_rate_request *req) +{ + unsigned long clk_flags, rate = req->rate; + const struct freq_conf *conf; + struct clk_hw *p; + struct clk_rcg2 *rcg = to_clk_rcg2(hw); + int index; + + f = qcom_find_freq_multi(f, rate); + if (!f || !f->confs) + return -EINVAL; + + conf = __clk_rcg2_select_conf(hw, f, rate); + index = qcom_find_src_index(hw, rcg->parent_map, conf->src); + if (index < 0) + return index; + + clk_flags = clk_hw_get_flags(hw); + p = clk_hw_get_parent_by_index(hw, index); + if (!p) + return -EINVAL; + + if (clk_flags & CLK_SET_RATE_PARENT) { + rate = f->freq; + if (conf->pre_div) { + if (!rate) + rate = req->rate; + rate /= 2; + rate *= conf->pre_div + 1; + } + + if (conf->n) { + u64 tmp = rate; + + tmp = tmp * conf->n; + do_div(tmp, conf->m); + rate = tmp; + } + } else { + rate = clk_hw_get_rate(p); + } + + req->best_parent_hw = p; + req->best_parent_rate = rate; + req->rate = f->freq; + + return 0; +} + static int clk_rcg2_determine_rate(struct clk_hw *hw, struct clk_rate_request *req) { @@ -282,6 +380,14 @@ 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_fm_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) +{ + struct clk_rcg2 *rcg = to_clk_rcg2(hw); + + return _freq_tbl_fm_determine_rate(hw, rcg->freq_multi_tbl, req); +} + static int __clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f, u32 *_cfg) { @@ -375,6 +481,27 @@ static int __clk_rcg2_set_rate(struct clk_hw *hw, unsigned long rate, return clk_rcg2_configure(rcg, f); } +static int __clk_rcg2_fm_set_rate(struct clk_hw *hw, unsigned long rate) +{ + struct clk_rcg2 *rcg = to_clk_rcg2(hw); + const struct freq_multi_tbl *f; + const struct freq_conf *conf; + struct freq_tbl f_tbl; + + f = qcom_find_freq_multi(rcg->freq_multi_tbl, rate); + if (!f || !f->confs) + return -EINVAL; + + conf = __clk_rcg2_select_conf(hw, f, rate); + f_tbl.freq = f->freq; + f_tbl.src = conf->src; + f_tbl.pre_div = conf->pre_div; + f_tbl.m = conf->m; + f_tbl.n = conf->n; + + return clk_rcg2_configure(rcg, &f_tbl); +} + static int clk_rcg2_set_rate(struct clk_hw *hw, unsigned long rate, unsigned long parent_rate) { @@ -387,6 +514,12 @@ static int clk_rcg2_set_floor_rate(struct clk_hw *hw, unsigned long rate, return __clk_rcg2_set_rate(hw, rate, FLOOR); } +static int clk_rcg2_fm_set_rate(struct clk_hw *hw, unsigned long rate, + unsigned long parent_rate) +{ + return __clk_rcg2_fm_set_rate(hw, rate); +} + static int clk_rcg2_set_rate_and_parent(struct clk_hw *hw, unsigned long rate, unsigned long parent_rate, u8 index) { @@ -399,6 +532,12 @@ static int clk_rcg2_set_floor_rate_and_parent(struct clk_hw *hw, return __clk_rcg2_set_rate(hw, rate, FLOOR); } +static int clk_rcg2_fm_set_rate_and_parent(struct clk_hw *hw, + unsigned long rate, unsigned long parent_rate, u8 index) +{ + return __clk_rcg2_fm_set_rate(hw, rate); +} + static int clk_rcg2_get_duty_cycle(struct clk_hw *hw, struct clk_duty *duty) { struct clk_rcg2 *rcg = to_clk_rcg2(hw); @@ -509,6 +648,19 @@ const struct clk_ops clk_rcg2_floor_ops = { }; EXPORT_SYMBOL_GPL(clk_rcg2_floor_ops); +const struct clk_ops clk_rcg2_fm_ops = { + .is_enabled = clk_rcg2_is_enabled, + .get_parent = clk_rcg2_get_parent, + .set_parent = clk_rcg2_set_parent, + .recalc_rate = clk_rcg2_recalc_rate, + .determine_rate = clk_rcg2_fm_determine_rate, + .set_rate = clk_rcg2_fm_set_rate, + .set_rate_and_parent = clk_rcg2_fm_set_rate_and_parent, + .get_duty_cycle = clk_rcg2_get_duty_cycle, + .set_duty_cycle = clk_rcg2_set_duty_cycle, +}; +EXPORT_SYMBOL_GPL(clk_rcg2_fm_ops); + const struct clk_ops clk_rcg2_mux_closest_ops = { .determine_rate = __clk_mux_determine_rate_closest, .get_parent = clk_rcg2_get_parent, diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c index 75f09e6e057e..48f81e3a5e80 100644 --- a/drivers/clk/qcom/common.c +++ b/drivers/clk/qcom/common.c @@ -41,6 +41,24 @@ struct freq_tbl *qcom_find_freq(const struct freq_tbl *f, unsigned long rate) } EXPORT_SYMBOL_GPL(qcom_find_freq); +const struct freq_multi_tbl *qcom_find_freq_multi(const struct freq_multi_tbl *f, + unsigned long rate) +{ + if (!f) + return NULL; + + if (!f->freq) + return f; + + for (; f->freq; f++) + if (rate <= f->freq) + return f; + + /* Default to our fastest rate */ + return f - 1; +} +EXPORT_SYMBOL_GPL(qcom_find_freq_multi); + const struct freq_tbl *qcom_find_freq_floor(const struct freq_tbl *f, unsigned long rate) { diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h index 9c8f7b798d9f..2d4a8a837e6c 100644 --- a/drivers/clk/qcom/common.h +++ b/drivers/clk/qcom/common.h @@ -45,6 +45,8 @@ extern const struct freq_tbl *qcom_find_freq(const struct freq_tbl *f, unsigned long rate); extern const struct freq_tbl *qcom_find_freq_floor(const struct freq_tbl *f, unsigned long rate); +extern const struct freq_multi_tbl *qcom_find_freq_multi(const struct freq_multi_tbl *f, + unsigned long rate); extern void qcom_pll_set_fsm_mode(struct regmap *m, u32 reg, u8 bias_count, u8 lock_count); extern int qcom_find_src_index(struct clk_hw *hw, const struct parent_map *map,
Some RCG frequency can be reached by multiple configuration. Add clk_rcg2_fm_ops ops to support these special RCG configurations. These alternative ops will select the frequency using a CEIL policy. When the correct frequency is found, the correct config is selected by calculating the final rate (by checking the defined parent and values in the config that is being checked) and deciding based on the one that is less different than the requested one. These check are skipped if there is just on config for the requested freq. qcom_find_freq_multi is added to search the freq with the new struct freq_multi_tbl. __clk_rcg2_select_conf is used to select the correct conf by simulating the final clock. Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> --- drivers/clk/qcom/clk-rcg.h | 1 + drivers/clk/qcom/clk-rcg2.c | 152 ++++++++++++++++++++++++++++++++++++ drivers/clk/qcom/common.c | 18 +++++ drivers/clk/qcom/common.h | 2 + 4 files changed, 173 insertions(+)