| Message ID | 5aa01cae-28ff-efb5-bf4d-1994760ecb79@cogentembedded.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Renesas R8A77980 CPG/MSSR RPC clock support | expand |
Hi Sergei, On Thu, Nov 22, 2018 at 7:41 PM Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> 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@cogentembedded.com> 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@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
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@cogentembedded.com> > > 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
Hi Sergei, On Tue, Nov 27, 2018 at 4:39 PM Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote: > 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@cogentembedded.com> > > > > Thanks for your patch! > > > >> --- renesas-drivers.orig/drivers/clk/renesas/rcar-gen3-cpg.c > >> +++ renesas-drivers/drivers/clk/renesas/rcar-gen3-cpg.c > >> + 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? Yes please. Thanks! Gr{oetje,eeting}s, Geert
Index: renesas-drivers/drivers/clk/renesas/rcar-gen3-cpg.c =================================================================== --- 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) +#define CPG_RPC_CKSTP BIT(8) +#define CPG_RPC_DIV_4_3_MASK GENMASK(4, 3) +#define CPG_RPC_DIV_2_0_MASK GENMASK(2, 0) + +struct rpc_clock { + struct clk_hw hw; + void __iomem *reg; +}; + +#define to_rpc_clock(_hw) container_of(_hw, struct rpc_clock, hw) + +static int cpg_rpc_clock_enable(struct clk_hw *hw) +{ + struct rpc_clock *clock = to_rpc_clock(hw); + + cpg_reg_modify(clock->reg, CPG_RPC_CKSTP, 0); + + return 0; +} + +static void cpg_rpc_clock_disable(struct clk_hw *hw) +{ + struct rpc_clock *clock = to_rpc_clock(hw); + + cpg_reg_modify(clock->reg, 0, CPG_RPC_CKSTP); +} + +static int cpg_rpc_clock_is_enabled(struct clk_hw *hw) +{ + struct rpc_clock *clock = to_rpc_clock(hw); + + return !(readl(clock->reg) & CPG_RPC_CKSTP); +} + +static unsigned long cpg_rpc_clock_recalc_rate(struct clk_hw *hw, + unsigned long parent_rate) +{ + struct rpc_clock *clock = to_rpc_clock(hw); + u32 div = (readl(clock->reg) & CPG_RPC_DIV_2_0_MASK) + 1; + + return DIV_ROUND_CLOSEST(parent_rate, div); +} + +static unsigned int cpg_rpc_clock_calc_div(struct rpc_clock *clock, + unsigned long rate, + unsigned long parent_rate) +{ + unsigned int div; + + if (!rate) + rate = 1; + + div = ALIGN(DIV_ROUND_CLOSEST(parent_rate, rate), 2); + + return clamp_t(unsigned int, div, 2, 8); +} + +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); +} + +static int cpg_rpc_clock_set_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); + + cpg_reg_modify(clock->reg, CPG_RPC_DIV_2_0_MASK, div - 1); + + return 0; +} + +static const struct clk_ops cpg_rpc_clock_ops = { + .enable = cpg_rpc_clock_enable, + .disable = cpg_rpc_clock_disable, + .is_enabled = cpg_rpc_clock_is_enabled, + .recalc_rate = cpg_rpc_clock_recalc_rate, + .round_rate = cpg_rpc_clock_round_rate, + .set_rate = cpg_rpc_clock_set_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; + 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); + + 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); } Index: renesas-drivers/drivers/clk/renesas/rcar-gen3-cpg.h =================================================================== --- renesas-drivers.orig/drivers/clk/renesas/rcar-gen3-cpg.h +++ renesas-drivers/drivers/clk/renesas/rcar-gen3-cpg.h @@ -23,6 +23,7 @@ enum rcar_gen3_clk_types { CLK_TYPE_GEN3_Z2, CLK_TYPE_GEN3_OSC, /* OSC EXTAL predivider and fixed divider */ CLK_TYPE_GEN3_RCKSEL, /* Select parent/divider using RCKCR.CKSEL */ + CLK_TYPE_GEN3_RPC, /* SoC specific definitions start here */ CLK_TYPE_GEN3_SOC_BASE, @@ -57,6 +58,7 @@ struct rcar_gen3_cpg_pll_config { u8 osc_prediv; }; +#define CPG_RPCCKCR 0x238 #define CPG_RCKCR 0x240 struct clk *rcar_gen3_cpg_clk_register(struct device *dev,
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@cogentembedded.com> --- drivers/clk/renesas/rcar-gen3-cpg.c | 118 ++++++++++++++++++++++++++++++++++++ drivers/clk/renesas/rcar-gen3-cpg.h | 2 2 files changed, 120 insertions(+)