On Fri, Mar 01, 2019 at 01:38:43PM +0100, Geert Uytterhoeven wrote:
> Hi Simon,
>
> Thanks for your patch!
>
> On Thu, Feb 28, 2019 at 2:53 PM Simon Horman <horms+renesas@xxxxxxxxxxxx> wrote:
> > Parameterise the offset of the control register for
> > for Z and Z2 clocks.
>
> double "for"
>
> > This is in preparation for supporting the ZG clock on the R-Car E3
> > (r8a77990), D3 (r8a7795) and RZ/G2E (r8a774c0) SoCs which uses a different
>
> D3 (r8a77995) ... use
>
> > control register to existing support for Z and Z2 clocks.
> >
> > Signed-off-by: Simon Horman <horms+renesas@xxxxxxxxxxxx>
>
> > --- a/drivers/clk/renesas/r8a774a1-cpg-mssr.c
> > +++ b/drivers/clk/renesas/r8a774a1-cpg-mssr.c
> > @@ -71,8 +71,10 @@ static const struct cpg_core_clk r8a774a1_core_clks[] __initconst = {
> > DEF_GEN3_OSC(".r", CLK_RINT, CLK_EXTAL, 32),
> >
> > /* Core Clock Outputs */
> > - DEF_GEN3_Z("z", R8A774A1_CLK_Z, CLK_TYPE_GEN3_Z, CLK_PLL0, 2, 8),
> > - DEF_GEN3_Z("z2", R8A774A1_CLK_Z2, CLK_TYPE_GEN3_Z, CLK_PLL2, 2, 0),
>
> Given "[PATCH v5 0/6] clk: renesas: r8a77990, r8a774c0: Add Z2 clock"
> still used CLK_TYPE_GEN3_Z2, this patch does not apply.
> Have you forgotten to send out v6?
There was supposed to be a consolidation patch at the beginning
of this series. Sorry for omitting it.
>
> > + DEF_GEN3_Z("z", R8A774A1_CLK_Z, CLK_TYPE_GEN3_Z,
> > + CLK_PLL0, 2, CPG_FRQCRC, 8),
> > + DEF_GEN3_Z("z2", R8A774A1_CLK_Z2, CLK_TYPE_GEN3_Z,
> > + CLK_PLL2, 2, CPG_FRQCRC, 0),
> > DEF_FIXED("ztr", R8A774A1_CLK_ZTR, CLK_PLL1_DIV2, 6, 1),
> > DEF_FIXED("ztrd2", R8A774A1_CLK_ZTRD2, CLK_PLL1_DIV2, 12, 1),
> > DEF_FIXED("zt", R8A774A1_CLK_ZT, CLK_PLL1_DIV2, 4, 1),
>
> > --- a/drivers/clk/renesas/rcar-gen3-cpg.c
> > +++ b/drivers/clk/renesas/rcar-gen3-cpg.c
>
> > @@ -166,7 +165,7 @@ static const struct clk_ops cpg_z_clk_ops = {
> >
> > static struct clk * __init cpg_z_clk_register(const char *name,
> > const char *parent_name,
> > - void __iomem *reg,
> > + void __iomem *base,
> > unsigned int div,
> > unsigned int offset)
> > {
> > @@ -184,10 +183,11 @@ static struct clk * __init cpg_z_clk_register(const char *name,
> > init.parent_names = &parent_name;
> > init.num_parents = 1;
> >
> > - zclk->reg = reg + CPG_FRQCRC;
> > - zclk->kick_reg = reg + CPG_FRQCRB;
> > + zclk->reg = base + GEN3_Z_REG_OFFSET(offset);
> > + zclk->kick_reg = base + CPG_FRQCRB;
> > zclk->hw.init = &init;
> > - zclk->mask = GENMASK(offset + 4, offset);
> > + zclk->mask = GENMASK(GEN3_Z_BIT_OFFSET(offset) + 4,
> > + GEN3_Z_BIT_OFFSET(offset));
>
> I think the code would be easier to read if you would move the splitting
> of offset to the caller, and thus pass separate reg_offset and bit_offset
> parameters.
Sure, that sounds reasonable.
>
> > zclk->fixed_div = div; /* PLLVCO x 1/div x SYS-CPU divider */
> >
> > clk = clk_register(NULL, &zclk->hw);
> > diff --git a/drivers/clk/renesas/rcar-gen3-cpg.h b/drivers/clk/renesas/rcar-gen3-cpg.h
> > index a0535341b5da..02bf3785263c 100644
> > --- a/drivers/clk/renesas/rcar-gen3-cpg.h
> > +++ b/drivers/clk/renesas/rcar-gen3-cpg.h
> > @@ -48,8 +48,16 @@ enum rcar_gen3_clk_types {
> > DEF_BASE(_name, _id, CLK_TYPE_GEN3_RCKSEL, \
> > (_parent0) << 16 | (_parent1), .div = (_div0) << 16 | (_div1))
> >
> > -#define DEF_GEN3_Z(_name, _id, _type, _parent, _div, _offset) \
> > - DEF_BASE(_name, _id, _type, _parent, .div = _div, .offset = _offset)
> > +#define DEF_GEN3_Z_OFFSET(_reg_offset, _bit_offset) \
> > + ((_reg_offset) << 16 | (_bit_offset) )
> > +
> > +#define GEN3_Z_REG_OFFSET(_offset) ((_offset) >> 16)
> > +
> > +#define GEN3_Z_BIT_OFFSET(_offset) ((_offset) & 0xffff)
>
> Do you think these 3 (un)marshalling macros add much value, given they're
> used in a single place only?
Yes, they make things very clear to my mind.
But I'm open to an alternative if you have one in mind.
>
> > +
> > +#define DEF_GEN3_Z(_name, _id, _type, _parent, _div, _reg_offset, _bit_offset)\
> > + DEF_BASE(_name, _id, _type, _parent, .div = _div, \
> > + .offset = DEF_GEN3_Z_OFFSET(_reg_offset, _bit_offset))
> >
> > struct rcar_gen3_cpg_pll_config {
> > u8 extal_div;
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
>
