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?
> + 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.
> 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?
> +
> +#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
