Hi Sergei,
On Thu, Nov 22, 2018 at 7:41 PM Sergei Shtylyov
<sergei.shtylyov@xxxxxxxxxxxxxxxxxx> wrote:
> Add the RPC clock for the R-Car gen3 SoCs -- this clock is controlled by
> the RPCCKCR register on all the R-Car gen3 SoCs except V3M (R8A77970).
>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>
Thanks for your patch!
> --- renesas-drivers.orig/drivers/clk/renesas/rcar-gen3-cpg.c
> +++ renesas-drivers/drivers/clk/renesas/rcar-gen3-cpg.c
> @@ -409,6 +409,121 @@ free_clock:
> return clk;
> }
>
> +#define CPG_RPC_CKSTP2 BIT(9)
This bit is for RPCD2, so technically it should be part of patch 3/4.
Perhaps you can merge both patches, and absorb the non-SoC-specific
parts from patch 4/4?
> +#define CPG_RPC_CKSTP BIT(8)
> +#define CPG_RPC_DIV_4_3_MASK GENMASK(4, 3)
Unused
> +#define CPG_RPC_DIV_2_0_MASK GENMASK(2, 0)
> +
> +struct rpc_clock {
> + struct clk_hw hw;
> + void __iomem *reg;
As this register should be saved/restore during system suspend/resume,
you should add
struct cpg_simple_notifier csn;
> +};
> +static long cpg_rpc_clock_round_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long *parent_rate)
> +{
> + struct rpc_clock *clock = to_rpc_clock(hw);
> + unsigned int div = cpg_rpc_clock_calc_div(clock, rate, *parent_rate);
> +
> + return DIV_ROUND_CLOSEST(*parent_rate, div);
Given you set CLK_SET_RATE_PARENT, shouldn't you propagate up,
cfr. drivers/clk/clk-fixed-factor.c:clk_factor_round_rate()?
> +}
> +static struct clk * __init cpg_rpc_clk_register(const struct cpg_core_clk *core,
> + void __iomem *base,
> + const char *parent_name)
> +{
> + struct clk_init_data init;
> + struct rpc_clock *clock;
> + struct clk *clk;
> +
> + clock = kzalloc(sizeof(*clock), GFP_KERNEL);
> + if (!clock)
> + return ERR_PTR(-ENOMEM);
> +
> + init.name = core->name;
> + init.ops = &cpg_rpc_clock_ops;
> + init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT;
I don't think CLK_IS_BASIC is appropriate?
#define CLK_IS_BASIC BIT(5) /* Basic clk, can't do a to_clk_foo() */
> + init.parent_names = &parent_name;
> + init.num_parents = 1;
> +
> + clock->reg = base + CPG_RPCCKCR;
> + clock->hw.init = &init;
> +
> + clk = clk_register(NULL, &clock->hw);
> + if (IS_ERR(clk))
> + kfree(clock);
> +
For save/restore during system suspend/resume:
cpg_simple_notifier_register(notifiers, &clock->csn);
Hmm, looks like I missed that during review of commit 381081ffc2948e1e
("clk: renesas: r8a77970: Add SD0H/SD0 clocks for SDHI"), too.
> + return clk;
> +}
> +
>
> static const struct rcar_gen3_cpg_pll_config *cpg_pll_config __initdata;
> static unsigned int cpg_clk_extalr __initdata;
> @@ -583,6 +698,9 @@ struct clk * __init rcar_gen3_cpg_clk_re
> }
> break;
>
> + case CLK_TYPE_GEN3_RPC:
> + return cpg_rpc_clk_register(core, base, __clk_get_name(parent));
> +
> default:
> return ERR_PTR(-EINVAL);
> }
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
