Message ID | 1478517877-23733-3-git-send-email-riteshh@codeaurora.org (mailing list archive) |
---|---|
State | Superseded, archived |
Delegated to: | Stephen Boyd |
Headers | show |
Hi Rajendra, [auto build test WARNING on ulf.hansson-mmc/next] [also build test WARNING on v4.9-rc4 next-20161028] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Ritesh-Harjani/mmc-sdhci-msm-Add-clk-rates-DDR-HS400-support/20161107-203031 base: https://git.linaro.org/people/ulf.hansson/mmc next config: i386-allmodconfig (attached as .config) compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce: # save the attached .config to linux build tree make ARCH=i386 All warnings (new ones prefixed by >>): drivers/clk/qcom/clk-rcg2.c: In function '_freq_tbl_determine_rate': >> drivers/clk/qcom/clk-rcg2.c:192:2: warning: switch condition has boolean value [-Wswitch-bool] switch (match) { ^~~~~~ drivers/clk/qcom/clk-rcg2.c: In function '__clk_rcg2_set_rate': drivers/clk/qcom/clk-rcg2.c:297:2: warning: switch condition has boolean value [-Wswitch-bool] switch (match) { ^~~~~~ coccinelle warnings: (new ones prefixed by >>) >> drivers/clk/qcom/clk-rcg2.c:306:2-3: Unneeded semicolon drivers/clk/qcom/clk-rcg2.c:201:2-3: Unneeded semicolon Please review and possibly fold the followup patch. vim +192 drivers/clk/qcom/clk-rcg2.c 186 { 187 unsigned long clk_flags, rate = req->rate; 188 struct clk_hw *p; 189 struct clk_rcg2 *rcg = to_clk_rcg2(hw); 190 int index; 191 > 192 switch (match) { 193 case FLOOR: 194 f = qcom_find_freq_floor(f, rate); 195 break; 196 case CEIL: 197 f = qcom_find_freq(f, rate); 198 break; 199 default: 200 return -EINVAL; 201 }; 202 203 if (!f) 204 return -EINVAL; 205 206 index = qcom_find_src_index(hw, rcg->parent_map, f->src); 207 if (index < 0) 208 return index; 209 210 clk_flags = clk_hw_get_flags(hw); 211 p = clk_hw_get_parent_by_index(hw, index); 212 if (clk_flags & CLK_SET_RATE_PARENT) { 213 if (f->pre_div) { 214 rate /= 2; 215 rate *= f->pre_div + 1; 216 } 217 218 if (f->n) { 219 u64 tmp = rate; 220 tmp = tmp * f->n; 221 do_div(tmp, f->m); 222 rate = tmp; 223 } 224 } else { 225 rate = clk_hw_get_rate(p); 226 } 227 req->best_parent_hw = p; 228 req->best_parent_rate = rate; 229 req->rate = f->freq; 230 231 return 0; 232 } 233 234 static int clk_rcg2_determine_rate(struct clk_hw *hw, 235 struct clk_rate_request *req) 236 { 237 struct clk_rcg2 *rcg = to_clk_rcg2(hw); 238 239 return _freq_tbl_determine_rate(hw, rcg->freq_tbl, req, CEIL); 240 } 241 242 static int clk_rcg2_determine_floor_rate(struct clk_hw *hw, 243 struct clk_rate_request *req) 244 { 245 struct clk_rcg2 *rcg = to_clk_rcg2(hw); 246 247 return _freq_tbl_determine_rate(hw, rcg->freq_tbl, req, FLOOR); 248 } 249 250 static int clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f) 251 { 252 u32 cfg, mask; 253 struct clk_hw *hw = &rcg->clkr.hw; 254 int ret, index = qcom_find_src_index(hw, rcg->parent_map, f->src); 255 256 if (index < 0) 257 return index; 258 259 if (rcg->mnd_width && f->n) { 260 mask = BIT(rcg->mnd_width) - 1; 261 ret = regmap_update_bits(rcg->clkr.regmap, 262 rcg->cmd_rcgr + M_REG, mask, f->m); 263 if (ret) 264 return ret; 265 266 ret = regmap_update_bits(rcg->clkr.regmap, 267 rcg->cmd_rcgr + N_REG, mask, ~(f->n - f->m)); 268 if (ret) 269 return ret; 270 271 ret = regmap_update_bits(rcg->clkr.regmap, 272 rcg->cmd_rcgr + D_REG, mask, ~f->n); 273 if (ret) 274 return ret; 275 } 276 277 mask = BIT(rcg->hid_width) - 1; 278 mask |= CFG_SRC_SEL_MASK | CFG_MODE_MASK; 279 cfg = f->pre_div << CFG_SRC_DIV_SHIFT; 280 cfg |= rcg->parent_map[index].cfg << CFG_SRC_SEL_SHIFT; 281 if (rcg->mnd_width && f->n && (f->m != f->n)) 282 cfg |= CFG_MODE_DUAL_EDGE; 283 ret = regmap_update_bits(rcg->clkr.regmap, 284 rcg->cmd_rcgr + CFG_REG, mask, cfg); 285 if (ret) 286 return ret; 287 288 return update_config(rcg); 289 } 290 291 static int __clk_rcg2_set_rate(struct clk_hw *hw, unsigned long rate, 292 bool match) 293 { 294 struct clk_rcg2 *rcg = to_clk_rcg2(hw); 295 const struct freq_tbl *f; 296 297 switch (match) { 298 case FLOOR: 299 f = qcom_find_freq_floor(rcg->freq_tbl, rate); 300 break; 301 case CEIL: 302 f = qcom_find_freq(rcg->freq_tbl, rate); 303 break; 304 default: 305 return -EINVAL; > 306 }; 307 308 if (!f) 309 return -EINVAL; --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 11/07, Ritesh Harjani wrote: > diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h > index b904c33..1b3e8d2 100644 > --- a/drivers/clk/qcom/clk-rcg.h > +++ b/drivers/clk/qcom/clk-rcg.h > @@ -173,6 +173,7 @@ struct clk_rcg2 { > #define to_clk_rcg2(_hw) container_of(to_clk_regmap(_hw), struct clk_rcg2, clkr) > > extern const struct clk_ops clk_rcg2_ops; > +extern const struct clk_ops clk_rcg2_floor_ops; > extern const struct clk_ops clk_rcg2_shared_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 a071bba..04433a6 100644 > --- a/drivers/clk/qcom/clk-rcg2.c > +++ b/drivers/clk/qcom/clk-rcg2.c > @@ -47,6 +47,11 @@ > #define N_REG 0xc > #define D_REG 0x10 > > +enum { > + FLOOR, > + CEIL, > +}; Give it a name. > + > static int clk_rcg2_is_enabled(struct clk_hw *hw) > { > struct clk_rcg2 *rcg = to_clk_rcg2(hw); > @@ -176,15 +181,25 @@ static int clk_rcg2_set_parent(struct clk_hw *hw, u8 index) > return calc_rate(parent_rate, m, n, mode, hid_div); > } > > -static int _freq_tbl_determine_rate(struct clk_hw *hw, > - const struct freq_tbl *f, struct clk_rate_request *req) > +static int _freq_tbl_determine_rate(struct clk_hw *hw, const struct freq_tbl *f, > + struct clk_rate_request *req, bool match) Use the enum please. Also name it something besides match. policy? > { > unsigned long clk_flags, rate = req->rate; > struct clk_hw *p; > struct clk_rcg2 *rcg = to_clk_rcg2(hw); > int index; > > - f = qcom_find_freq(f, rate); > + switch (match) { > + case FLOOR: > + f = qcom_find_freq_floor(f, rate); > + break; > + case CEIL: > + f = qcom_find_freq(f, rate); > + break; > + default: > + return -EINVAL; > + }; > + > if (!f) > return -EINVAL; > > diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c > index fffcbaf..cf6b87f 100644 > --- a/drivers/clk/qcom/common.c > +++ b/drivers/clk/qcom/common.c > @@ -46,6 +46,32 @@ struct freq_tbl *qcom_find_freq(const struct freq_tbl *f, unsigned long rate) > } > EXPORT_SYMBOL_GPL(qcom_find_freq); > > +const > +struct freq_tbl *qcom_find_freq_floor(const struct freq_tbl *f, We can't put const and struct on the same line? > + unsigned long rate) > +{ > + int size = 0; > + > + if (!f) > + return NULL; > + > + /* > + * The freq table has entries in the ascending order of frequencies > + * To find the floor for a given frequency, we need to do a reverse > + * lookup of the table > + */ > + for (; f->freq; f++, size++) > + ; > + > + for (f--; size; f--, size--) > + if (rate >= f->freq) > + return f; I don't understand why we can't do this while iterating through the table. We shouldn't need to size up the frequency table first. const struct freq_tbl *best = NULL; for ( ; f->freq; f++) { if (rate >= f->freq) best = f->freq; else break; } return best; > + > + /* could not find any rates lower than *rate* */ > + return NULL; > +} > +EXPORT_SYMBOL_GPL(qcom_find_freq_floor);
Hi Stephen, Thanks for the review. On 11/9/2016 4:32 AM, Stephen Boyd wrote: > On 11/07, Ritesh Harjani wrote: >> diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h >> index b904c33..1b3e8d2 100644 >> --- a/drivers/clk/qcom/clk-rcg.h >> +++ b/drivers/clk/qcom/clk-rcg.h >> @@ -173,6 +173,7 @@ struct clk_rcg2 { >> #define to_clk_rcg2(_hw) container_of(to_clk_regmap(_hw), struct clk_rcg2, clkr) >> >> extern const struct clk_ops clk_rcg2_ops; >> +extern const struct clk_ops clk_rcg2_floor_ops; >> extern const struct clk_ops clk_rcg2_shared_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 a071bba..04433a6 100644 >> --- a/drivers/clk/qcom/clk-rcg2.c >> +++ b/drivers/clk/qcom/clk-rcg2.c >> @@ -47,6 +47,11 @@ >> #define N_REG 0xc >> #define D_REG 0x10 >> >> +enum { >> + FLOOR, >> + CEIL, >> +}; > > Give it a name. Yes, sure. I will keep it as freq_policy. > >> + >> static int clk_rcg2_is_enabled(struct clk_hw *hw) >> { >> struct clk_rcg2 *rcg = to_clk_rcg2(hw); >> @@ -176,15 +181,25 @@ static int clk_rcg2_set_parent(struct clk_hw *hw, u8 index) >> return calc_rate(parent_rate, m, n, mode, hid_div); >> } >> >> -static int _freq_tbl_determine_rate(struct clk_hw *hw, >> - const struct freq_tbl *f, struct clk_rate_request *req) >> +static int _freq_tbl_determine_rate(struct clk_hw *hw, const struct freq_tbl *f, >> + struct clk_rate_request *req, bool match) > > Use the enum please. Also name it something besides match. > policy? Sure. (freq_policy) > >> { >> unsigned long clk_flags, rate = req->rate; >> struct clk_hw *p; >> struct clk_rcg2 *rcg = to_clk_rcg2(hw); >> int index; >> >> - f = qcom_find_freq(f, rate); >> + switch (match) { >> + case FLOOR: >> + f = qcom_find_freq_floor(f, rate); >> + break; >> + case CEIL: >> + f = qcom_find_freq(f, rate); >> + break; >> + default: >> + return -EINVAL; >> + }; >> + >> if (!f) >> return -EINVAL; >> >> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c >> index fffcbaf..cf6b87f 100644 >> --- a/drivers/clk/qcom/common.c >> +++ b/drivers/clk/qcom/common.c >> @@ -46,6 +46,32 @@ struct freq_tbl *qcom_find_freq(const struct freq_tbl *f, unsigned long rate) >> } >> EXPORT_SYMBOL_GPL(qcom_find_freq); >> >> +const >> +struct freq_tbl *qcom_find_freq_floor(const struct freq_tbl *f, > > We can't put const and struct on the same line? Ok sure. > >> + unsigned long rate) >> +{ >> + int size = 0; >> + >> + if (!f) >> + return NULL; >> + >> + /* >> + * The freq table has entries in the ascending order of frequencies >> + * To find the floor for a given frequency, we need to do a reverse >> + * lookup of the table >> + */ >> + for (; f->freq; f++, size++) >> + ; >> + >> + for (f--; size; f--, size--) >> + if (rate >= f->freq) >> + return f; > > I don't understand why we can't do this while iterating through > the table. We shouldn't need to size up the frequency table first. > > const struct freq_tbl *best = NULL; > > for ( ; f->freq; f++) { > if (rate >= f->freq) > best = f->freq; > else > break; > } > > return best; > Yes, will do above change. >> + >> + /* could not find any rates lower than *rate* */ > >> + return NULL; >> +} >> +EXPORT_SYMBOL_GPL(qcom_find_freq_floor); >
diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h index b904c33..1b3e8d2 100644 --- a/drivers/clk/qcom/clk-rcg.h +++ b/drivers/clk/qcom/clk-rcg.h @@ -173,6 +173,7 @@ struct clk_rcg2 { #define to_clk_rcg2(_hw) container_of(to_clk_regmap(_hw), struct clk_rcg2, clkr) extern const struct clk_ops clk_rcg2_ops; +extern const struct clk_ops clk_rcg2_floor_ops; extern const struct clk_ops clk_rcg2_shared_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 a071bba..04433a6 100644 --- a/drivers/clk/qcom/clk-rcg2.c +++ b/drivers/clk/qcom/clk-rcg2.c @@ -47,6 +47,11 @@ #define N_REG 0xc #define D_REG 0x10 +enum { + FLOOR, + CEIL, +}; + static int clk_rcg2_is_enabled(struct clk_hw *hw) { struct clk_rcg2 *rcg = to_clk_rcg2(hw); @@ -176,15 +181,25 @@ static int clk_rcg2_set_parent(struct clk_hw *hw, u8 index) return calc_rate(parent_rate, m, n, mode, hid_div); } -static int _freq_tbl_determine_rate(struct clk_hw *hw, - const struct freq_tbl *f, struct clk_rate_request *req) +static int _freq_tbl_determine_rate(struct clk_hw *hw, const struct freq_tbl *f, + struct clk_rate_request *req, bool match) { unsigned long clk_flags, rate = req->rate; struct clk_hw *p; struct clk_rcg2 *rcg = to_clk_rcg2(hw); int index; - f = qcom_find_freq(f, rate); + switch (match) { + case FLOOR: + f = qcom_find_freq_floor(f, rate); + break; + case CEIL: + f = qcom_find_freq(f, rate); + break; + default: + return -EINVAL; + }; + if (!f) return -EINVAL; @@ -221,7 +236,15 @@ static int clk_rcg2_determine_rate(struct clk_hw *hw, { struct clk_rcg2 *rcg = to_clk_rcg2(hw); - return _freq_tbl_determine_rate(hw, rcg->freq_tbl, req); + return _freq_tbl_determine_rate(hw, rcg->freq_tbl, req, CEIL); +} + +static int clk_rcg2_determine_floor_rate(struct clk_hw *hw, + struct clk_rate_request *req) +{ + struct clk_rcg2 *rcg = to_clk_rcg2(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) @@ -265,12 +288,23 @@ static int clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f) return update_config(rcg); } -static int __clk_rcg2_set_rate(struct clk_hw *hw, unsigned long rate) +static int __clk_rcg2_set_rate(struct clk_hw *hw, unsigned long rate, + bool match) { struct clk_rcg2 *rcg = to_clk_rcg2(hw); const struct freq_tbl *f; - f = qcom_find_freq(rcg->freq_tbl, rate); + switch (match) { + case FLOOR: + f = qcom_find_freq_floor(rcg->freq_tbl, rate); + break; + case CEIL: + f = qcom_find_freq(rcg->freq_tbl, rate); + break; + default: + return -EINVAL; + }; + if (!f) return -EINVAL; @@ -280,13 +314,25 @@ static int __clk_rcg2_set_rate(struct clk_hw *hw, unsigned long rate) static int clk_rcg2_set_rate(struct clk_hw *hw, unsigned long rate, unsigned long parent_rate) { - return __clk_rcg2_set_rate(hw, rate); + return __clk_rcg2_set_rate(hw, rate, CEIL); +} + +static int clk_rcg2_set_floor_rate(struct clk_hw *hw, unsigned long rate, + unsigned long parent_rate) +{ + return __clk_rcg2_set_rate(hw, rate, FLOOR); } static int clk_rcg2_set_rate_and_parent(struct clk_hw *hw, unsigned long rate, unsigned long parent_rate, u8 index) { - return __clk_rcg2_set_rate(hw, rate); + return __clk_rcg2_set_rate(hw, rate, CEIL); +} + +static int clk_rcg2_set_floor_rate_and_parent(struct clk_hw *hw, + unsigned long rate, unsigned long parent_rate, u8 index) +{ + return __clk_rcg2_set_rate(hw, rate, FLOOR); } const struct clk_ops clk_rcg2_ops = { @@ -300,6 +346,17 @@ static int clk_rcg2_set_rate_and_parent(struct clk_hw *hw, }; EXPORT_SYMBOL_GPL(clk_rcg2_ops); +const struct clk_ops clk_rcg2_floor_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_determine_floor_rate, + .set_rate = clk_rcg2_set_floor_rate, + .set_rate_and_parent = clk_rcg2_set_floor_rate_and_parent, +}; +EXPORT_SYMBOL_GPL(clk_rcg2_floor_ops); + static int clk_rcg2_shared_force_enable(struct clk_hw *hw, unsigned long rate) { struct clk_rcg2 *rcg = to_clk_rcg2(hw); @@ -323,7 +380,7 @@ static int clk_rcg2_shared_force_enable(struct clk_hw *hw, unsigned long rate) pr_err("%s: RCG did not turn on\n", name); /* set clock rate */ - ret = __clk_rcg2_set_rate(hw, rate); + ret = __clk_rcg2_set_rate(hw, rate, CEIL); if (ret) return ret; diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c index fffcbaf..cf6b87f 100644 --- a/drivers/clk/qcom/common.c +++ b/drivers/clk/qcom/common.c @@ -46,6 +46,32 @@ struct freq_tbl *qcom_find_freq(const struct freq_tbl *f, unsigned long rate) } EXPORT_SYMBOL_GPL(qcom_find_freq); +const +struct freq_tbl *qcom_find_freq_floor(const struct freq_tbl *f, + unsigned long rate) +{ + int size = 0; + + if (!f) + return NULL; + + /* + * The freq table has entries in the ascending order of frequencies + * To find the floor for a given frequency, we need to do a reverse + * lookup of the table + */ + for (; f->freq; f++, size++) + ; + + for (f--; size; f--, size--) + if (rate >= f->freq) + return f; + + /* could not find any rates lower than *rate* */ + return NULL; +} +EXPORT_SYMBOL_GPL(qcom_find_freq_floor); + int qcom_find_src_index(struct clk_hw *hw, const struct parent_map *map, u8 src) { int i, num_parents = clk_hw_get_num_parents(hw); diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h index ae9bdeb..76886a1 100644 --- a/drivers/clk/qcom/common.h +++ b/drivers/clk/qcom/common.h @@ -34,6 +34,8 @@ struct qcom_cc_desc { 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 int qcom_find_src_index(struct clk_hw *hw, const struct parent_map *map, u8 src);