| Message ID | 20190128115359.30039-1-vkoul@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] clk: qcom: clk-rcg2: Introduce a cfg offset for RCGs | expand |
Quoting Vinod Koul (2019-01-28 03:53:58) > diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h > index e5eca8a1abe4..f06783c20688 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; > * @parent_map: map from software's parent index to hardware's src_sel field > * @freq_tbl: frequency table > * @clkr: regmap clock handle > + * @cfg_off: defines the cfg register offset from the CMD_RCGR > * Please remove this extra line here. Also, shouldn't it say offset from CMD_RCGR + CFG_REG? > */ > struct clk_rcg2 { > @@ -150,6 +151,7 @@ struct clk_rcg2 { > const struct parent_map *parent_map; > const struct freq_tbl *freq_tbl; > struct clk_regmap clkr; > + u8 cfg_off; > }; > > #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 6e3bd195d012..106848e3313f 100644 > --- a/drivers/clk/qcom/clk-rcg2.c > +++ b/drivers/clk/qcom/clk-rcg2.c > @@ -74,7 +74,8 @@ static u8 clk_rcg2_get_parent(struct clk_hw *hw) > u32 cfg; > int i, ret; > > - ret = regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, &cfg); > + ret = regmap_read(rcg->clkr.regmap, > + rcg->cmd_rcgr + rcg->cfg_off + CFG_REG, &cfg); Maybe we should define CFG_REG as CFG_REG(rcg) and then do the math there? #define CFG_REG(rcg) (rcg)->cfg_off + 0x4 > if (ret) > goto err; > > @@ -263,17 +268,20 @@ static int __clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f) > if (rcg->mnd_width && f->n) { > mask = BIT(rcg->mnd_width) - 1; > ret = regmap_update_bits(rcg->clkr.regmap, > - rcg->cmd_rcgr + M_REG, mask, f->m); > + rcg->cmd_rcgr + rcg->cfg_off + M_REG, > + mask, f->m); > if (ret) > return ret; > > ret = regmap_update_bits(rcg->clkr.regmap, > - rcg->cmd_rcgr + N_REG, mask, ~(f->n - f->m)); > + rcg->cmd_rcgr + rcg->cfg_off + N_REG, > + mask, ~(f->n - f->m)); > if (ret) > return ret; > > ret = regmap_update_bits(rcg->clkr.regmap, > - rcg->cmd_rcgr + D_REG, mask, ~f->n); > + rcg->cmd_rcgr + rcg->cfg_off + D_REG, > + mask, ~f->n); Ah the MND registers also move. Wow that's so sad. Do a similar thing for all these too? #define D_REG(rcg) (rcg)->cfg_off + 0x8 etc... All just to make things fit on the same number of lines as before! We could also throw the rcg->cmd_rcgr part into the register named macros to make things even shorter. It was mostly OK when it was just adding the offset to the base, but now we have another offset so I think we can roll it all into the macro so that we can just read "regmap_read FOO_REG" and ignore the rest. > if (ret) > return ret; > }
On 29-01-19, 14:42, Stephen Boyd wrote: > Quoting Vinod Koul (2019-01-28 03:53:58) > > diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h > > index e5eca8a1abe4..f06783c20688 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; > > * @parent_map: map from software's parent index to hardware's src_sel field > > * @freq_tbl: frequency table > > * @clkr: regmap clock handle > > + * @cfg_off: defines the cfg register offset from the CMD_RCGR > > * > > Please remove this extra line here. Also, shouldn't it say offset from > CMD_RCGR + CFG_REG? Sure but I dont like to mix so will send that as a separate patch :) and will update the comment too. > > > > */ > > struct clk_rcg2 { > > @@ -150,6 +151,7 @@ struct clk_rcg2 { > > const struct parent_map *parent_map; > > const struct freq_tbl *freq_tbl; > > struct clk_regmap clkr; > > + u8 cfg_off; > > }; > > > > #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 6e3bd195d012..106848e3313f 100644 > > --- a/drivers/clk/qcom/clk-rcg2.c > > +++ b/drivers/clk/qcom/clk-rcg2.c > > @@ -74,7 +74,8 @@ static u8 clk_rcg2_get_parent(struct clk_hw *hw) > > u32 cfg; > > int i, ret; > > > > - ret = regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, &cfg); > > + ret = regmap_read(rcg->clkr.regmap, > > + rcg->cmd_rcgr + rcg->cfg_off + CFG_REG, &cfg); > > Maybe we should define CFG_REG as CFG_REG(rcg) and then do the math > there? > > #define CFG_REG(rcg) (rcg)->cfg_off + 0x4 Sure that looks better > > > if (ret) > > goto err; > > > > @@ -263,17 +268,20 @@ static int __clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f) > > if (rcg->mnd_width && f->n) { > > mask = BIT(rcg->mnd_width) - 1; > > ret = regmap_update_bits(rcg->clkr.regmap, > > - rcg->cmd_rcgr + M_REG, mask, f->m); > > + rcg->cmd_rcgr + rcg->cfg_off + M_REG, > > + mask, f->m); > > if (ret) > > return ret; > > > > ret = regmap_update_bits(rcg->clkr.regmap, > > - rcg->cmd_rcgr + N_REG, mask, ~(f->n - f->m)); > > + rcg->cmd_rcgr + rcg->cfg_off + N_REG, > > + mask, ~(f->n - f->m)); > > if (ret) > > return ret; > > > > ret = regmap_update_bits(rcg->clkr.regmap, > > - rcg->cmd_rcgr + D_REG, mask, ~f->n); > > + rcg->cmd_rcgr + rcg->cfg_off + D_REG, > > + mask, ~f->n); > > Ah the MND registers also move. Wow that's so sad. Do a similar thing > for all these too? > > #define D_REG(rcg) (rcg)->cfg_off + 0x8 > etc... > > All just to make things fit on the same number of lines as before! We > could also throw the rcg->cmd_rcgr part into the register named macros > to make things even shorter. It was mostly OK when it was just adding > the offset to the base, but now we have another offset so I think we can > roll it all into the macro so that we can just read "regmap_read > FOO_REG" and ignore the rest. Correct that will make it neater, will do so -- ~Vinod
diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h index e5eca8a1abe4..f06783c20688 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; * @parent_map: map from software's parent index to hardware's src_sel field * @freq_tbl: frequency table * @clkr: regmap clock handle + * @cfg_off: defines the cfg register offset from the CMD_RCGR * */ struct clk_rcg2 { @@ -150,6 +151,7 @@ struct clk_rcg2 { const struct parent_map *parent_map; const struct freq_tbl *freq_tbl; struct clk_regmap clkr; + u8 cfg_off; }; #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 6e3bd195d012..106848e3313f 100644 --- a/drivers/clk/qcom/clk-rcg2.c +++ b/drivers/clk/qcom/clk-rcg2.c @@ -74,7 +74,8 @@ static u8 clk_rcg2_get_parent(struct clk_hw *hw) u32 cfg; int i, ret; - ret = regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, &cfg); + ret = regmap_read(rcg->clkr.regmap, + rcg->cmd_rcgr + rcg->cfg_off + CFG_REG, &cfg); if (ret) goto err; @@ -123,7 +124,8 @@ static int clk_rcg2_set_parent(struct clk_hw *hw, u8 index) int ret; u32 cfg = rcg->parent_map[index].cfg << CFG_SRC_SEL_SHIFT; - ret = regmap_update_bits(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, + ret = regmap_update_bits(rcg->clkr.regmap, + rcg->cmd_rcgr + rcg->cfg_off + CFG_REG, CFG_SRC_SEL_MASK, cfg); if (ret) return ret; @@ -162,13 +164,16 @@ clk_rcg2_recalc_rate(struct clk_hw *hw, unsigned long parent_rate) 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->cmd_rcgr + CFG_REG, &cfg); + regmap_read(rcg->clkr.regmap, + rcg->cmd_rcgr + rcg->cfg_off + CFG_REG, &cfg); if (rcg->mnd_width) { mask = BIT(rcg->mnd_width) - 1; - regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + M_REG, &m); + regmap_read(rcg->clkr.regmap, + rcg->cmd_rcgr + rcg->cfg_off + M_REG, &m); m &= mask; - regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + N_REG, &n); + regmap_read(rcg->clkr.regmap, + rcg->cmd_rcgr + rcg->cfg_off + N_REG, &n); n = ~n; n &= mask; n += m; @@ -263,17 +268,20 @@ static int __clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f) if (rcg->mnd_width && f->n) { mask = BIT(rcg->mnd_width) - 1; ret = regmap_update_bits(rcg->clkr.regmap, - rcg->cmd_rcgr + M_REG, mask, f->m); + rcg->cmd_rcgr + rcg->cfg_off + M_REG, + mask, f->m); if (ret) return ret; ret = regmap_update_bits(rcg->clkr.regmap, - rcg->cmd_rcgr + N_REG, mask, ~(f->n - f->m)); + rcg->cmd_rcgr + rcg->cfg_off + N_REG, + mask, ~(f->n - f->m)); if (ret) return ret; ret = regmap_update_bits(rcg->clkr.regmap, - rcg->cmd_rcgr + D_REG, mask, ~f->n); + rcg->cmd_rcgr + rcg->cfg_off + D_REG, + mask, ~f->n); if (ret) return ret; } @@ -284,9 +292,9 @@ 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->cmd_rcgr + CFG_REG, - mask, cfg); + return regmap_update_bits(rcg->clkr.regmap, + rcg->cmd_rcgr + rcg->cfg_off + CFG_REG, + mask, cfg); } static int clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f)