On 11/23/2018 03:55 PM, Geert Uytterhoeven 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?
Done!
>> +#define CPG_RPC_CKSTP BIT(8)
>> +#define CPG_RPC_DIV_4_3_MASK GENMASK(4, 3)
>
> Unused
I'd like to keep it for the sake of completeness...
>> +#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;
Done.
>> +};
>
>> +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()?
Frankly speaking, I'm not sure when I should set that flag...
[...]
>> +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() */
I still haven't found where this bit is tested... and I got an impression everybody
just blindly copy&pastes it...
>
>> + 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);
Done.
> Hmm, looks like I missed that during review of commit 381081ffc2948e1e
> ("clk: renesas: r8a77970: Add SD0H/SD0 clocks for SDHI"), too.
Want me to fix this?
[...]
> Gr{oetje,eeting}s,
>
> Geert
MBR, Sergei
