diff mbox series

[1/2] clk: qcom: clk-rcg2: Introduce a cfg offset for RCGs

Message ID 20190128115359.30039-1-vkoul@kernel.org (mailing list archive)
State New, archived
Headers show
Series [1/2] clk: qcom: clk-rcg2: Introduce a cfg offset for RCGs | expand

Commit Message

Vinod Koul Jan. 28, 2019, 11:53 a.m. UTC 
From: Taniya Das <tdas@codeaurora.org>

The RCG CFG/M/N/D register base could be at a different offset than
the CMD register, so introduce a cfg_offset to identify the offset
with respect to the CMD register.

Signed-off-by: Taniya Das <tdas@codeaurora.org>
Signed-off-by: Anu Ramanathan <anur@codeaurora.org>
Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/clk/qcom/clk-rcg.h  |  2 ++
 drivers/clk/qcom/clk-rcg2.c | 30 +++++++++++++++++++-----------
 2 files changed, 21 insertions(+), 11 deletions(-)

Comments

Stephen Boyd Jan. 29, 2019, 10:42 p.m. UTC | #1 
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;
>         }
Vinod Koul Jan. 30, 2019, 4:34 a.m. UTC | #2 
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
diff mbox series

Patch

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)