Re: [PATCH 3/4] clk: renesas: rcar-gen3-cpg: add RPCD2 clock

[Geert Uytterhoeven]

Hi Sergei,

On Thu, Nov 22, 2018 at 7:43 PM Sergei Shtylyov
 wrote:
> Add the RPCD2 clock for the R-Car gen3 SoCs -- this clock is en/disabled
> via the RPCCKCR register on all the R-Car gen3 SoCs except V3M (R8A77970)
> and has a fixed divisor of 2 (applied to the RPC clock).
>
> 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
> @@ -524,6 +524,89 @@ static struct clk * __init cpg_rpc_clk_r
> return clk;
>  }
>
> +static int cpg_rpcd2_clock_enable(struct clk_hw *hw)
> +{
> +   struct rpc_clock *clock = to_rpc_clock(hw);
> +
> +   cpg_reg_modify(clock->reg, CPG_RPC_CKSTP2, 0);
> +
> +   return 0;
> +}
> +
> +static void cpg_rpcd2_clock_disable(struct clk_hw *hw)
> +{
> +   struct rpc_clock *clock = to_rpc_clock(hw);
> +
> +   cpg_reg_modify(clock->reg, 0, CPG_RPC_CKSTP2);
> +}
> +
> +static int cpg_rpcd2_clock_is_enabled(struct clk_hw *hw)
> +{
> +   struct rpc_clock *clock = to_rpc_clock(hw);
> +
> +   return !(readl(clock->reg) & CPG_RPC_CKSTP2);
> +}

As the above 3 functions are identical to their rpc_*() counterparts,
except for the bit touched, would it make sense to share them, e.g. by
storing the bit number in struct rpc_clock?

> +static long cpg_rpcd2_round_rate(struct clk_hw *hw, unsigned long rate,
> +unsigned long *parent_rate)
> +{
> +   return *parent_rate / 2;

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_rpcd2_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 = _rpcd2_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() */

Given RPCD2 is the combination of a gate and fixed-divider clock, would
it make sense to use clk_composite?

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

[PATCH 3/4] clk: renesas: rcar-gen3-cpg: add RPCD2 clock

[Sergei Shtylyov]

Add the RPCD2 clock for the R-Car gen3 SoCs -- this clock is en/disabled
via the RPCCKCR register on all the R-Car gen3 SoCs except V3M (R8A77970)
and has a fixed divisor of 2 (applied to the RPC clock).

Signed-off-by: Sergei Shtylyov 

---
 drivers/clk/renesas/rcar-gen3-cpg.c |   87 
 drivers/clk/renesas/rcar-gen3-cpg.h |1 
 2 files changed, 88 insertions(+)

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
@@ -524,6 +524,89 @@ static struct clk * __init cpg_rpc_clk_r
return clk;
 }
 
+static int cpg_rpcd2_clock_enable(struct clk_hw *hw)
+{
+   struct rpc_clock *clock = to_rpc_clock(hw);
+
+   cpg_reg_modify(clock->reg, CPG_RPC_CKSTP2, 0);
+
+   return 0;
+}
+
+static void cpg_rpcd2_clock_disable(struct clk_hw *hw)
+{
+   struct rpc_clock *clock = to_rpc_clock(hw);
+
+   cpg_reg_modify(clock->reg, 0, CPG_RPC_CKSTP2);
+}
+
+static int cpg_rpcd2_clock_is_enabled(struct clk_hw *hw)
+{
+   struct rpc_clock *clock = to_rpc_clock(hw);
+
+   return !(readl(clock->reg) & CPG_RPC_CKSTP2);
+}
+
+static unsigned long cpg_rpcd2_recalc_rate(struct clk_hw *hw,
+  unsigned long parent_rate)
+{
+   return parent_rate / 2;
+}
+
+static long cpg_rpcd2_round_rate(struct clk_hw *hw, unsigned long rate,
+unsigned long *parent_rate)
+{
+   return *parent_rate / 2;
+}
+
+static int cpg_rpcd2_set_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long parent_rate)
+{
+   /*
+* We must report success but we can do so unconditionally because
+* the round_rate() method returns values that ensure this call is
+* a nop.
+*/
+   return 0;
+}
+
+static const struct clk_ops cpg_rpcd2_clock_ops = {
+   .enable = cpg_rpcd2_clock_enable,
+   .disable = cpg_rpcd2_clock_disable,
+   .is_enabled = cpg_rpcd2_clock_is_enabled,
+   .recalc_rate = cpg_rpcd2_recalc_rate,
+   .round_rate = cpg_rpcd2_round_rate,
+   .set_rate = cpg_rpcd2_set_rate,
+};
+
+static struct clk * __init cpg_rpcd2_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 = _rpcd2_clock_ops;
+   init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT;
+   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);
+
+   return clk;
+}
+
 
 static const struct rcar_gen3_cpg_pll_config *cpg_pll_config __initdata;
 static unsigned int cpg_clk_extalr __initdata;
@@ -701,6 +784,10 @@ struct clk * __init rcar_gen3_cpg_clk_re
case CLK_TYPE_GEN3_RPC:
return cpg_rpc_clk_register(core, base, __clk_get_name(parent));
 
+   case CLK_TYPE_GEN3_RPCD2:
+   return cpg_rpcd2_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
@@ -24,6 +24,7 @@ enum rcar_gen3_clk_types {
CLK_TYPE_GEN3_OSC,  /* OSC EXTAL predivider and fixed divider */
CLK_TYPE_GEN3_RCKSEL,   /* Select parent/divider using RCKCR.CKSEL */
CLK_TYPE_GEN3_RPC,
+   CLK_TYPE_GEN3_RPCD2,
 
/* SoC specific definitions start here */
CLK_TYPE_GEN3_SOC_BASE,