Re: [PATCH 2/4] clk: renesas: rcar-gen3-cpg: add RPC clock
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
>
> 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 = _rpc_clock_ops;
>> + init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT;
>
> I don't think CLK_IS_BASIC is appropriate?
>
> #define CLK_IS_BASICBIT(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 it...
>
>> + init.parent_names = _name;
>> + init.num_parents = 1;
>> +
>> + clock->reg = base + CPG_RPCCKCR;
>> + clock->hw.init =
>> +
>> + clk = clk_register(NULL, >hw);
>> + if (IS_ERR(clk))
>> + kfree(clock);
>> +
>
> For save/restore during system suspend/resume:
>
> cpg_simple_notifier_register(notifiers, >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
Re: [PATCH 2/4] clk: renesas: rcar-gen3-cpg: add RPC clock
Hi Sergei,
On Thu, Nov 22, 2018 at 7:41 PM Sergei Shtylyov
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
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 = _rpc_clock_ops;
> + init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT;
I don't think CLK_IS_BASIC is appropriate?
#define CLK_IS_BASICBIT(5) /* Basic clk, can't do a to_clk_foo() */
> + init.parent_names = _name;
> + init.num_parents = 1;
> +
> + clock->reg = base + CPG_RPCCKCR;
> + clock->hw.init =
> +
> + clk = clk_register(NULL, >hw);
> + if (IS_ERR(clk))
> + kfree(clock);
> +
For save/restore during system suspend/resume:
cpg_simple_notifier_register(notifiers, >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 -- ge...@linux-m68k.org
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
