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

[Vinod Koul <vkoul@kernel.org>]

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