[PATCH 0/4] Renesas R8A77980 CPG/MSSR RPC clock support

[PATCH 0/4] Renesas R8A77980 CPG/MSSR RPC clock support

[Sergei Shtylyov]

Hello!

Here's the set of 4 patches against the 'clk-renesas' branch of Geert Uytterhoeven's
'renesas-drivers.git' repo. We're adding support for the RPC clocks...

[1/4] clk: renesas: rcar-gen3-cpg: factor out cpg_reg_modify()
[2/4] clk: renesas: rcar-gen3-cpg: add RPC clock
[3/4] clk: renesas: rcar-gen3-cpg: add RPCD2 clock
[4/4] clk: renesas: r8a77980-cpg-mssr: add RPC clocks

MBR, Sergei

[PATCH 1/4] clk: renesas: rcar-gen3-cpg: factor out cpg_reg_modify()

[Sergei Shtylyov]

There's quite often repeated sequence of a CPG register read-modify-write,
so it seems worth factoring it out into a function -- this saves 68 bytes
of the object code already (AArch64 gcc 4.8.5) and will save more with the
next patches...

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
 drivers/clk/renesas/rcar-gen3-cpg.c |   37 ++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

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
@@ -30,6 +30,15 @@
 
 #define CPG_RCKCR_CKSEL	BIT(15)	/* RCLK Clock Source Select */
 
+static void cpg_reg_modify(void __iomem *reg, u32 clear, u32 set)
+{
+	u32 val = readl(reg);
+
+	val &= ~clear;
+	val |= set;
+	writel(val, reg);
+};
+
 struct cpg_simple_notifier {
 	struct notifier_block nb;
 	void __iomem *reg;
@@ -118,7 +127,6 @@ static int cpg_z_clk_set_rate(struct clk
 	struct cpg_z_clk *zclk = to_z_clk(hw);
 	unsigned int mult;
 	unsigned int i;
-	u32 val, kick;
 
 	/* Factor of 2 is for fixed divider */
 	mult = DIV_ROUND_CLOSEST_ULL(rate * 32ULL * 2, parent_rate);
@@ -127,17 +135,14 @@ static int cpg_z_clk_set_rate(struct clk
 	if (readl(zclk->kick_reg) & CPG_FRQCRB_KICK)
 		return -EBUSY;
 
-	val = readl(zclk->reg) & ~zclk->mask;
-	val |= ((32 - mult) << __ffs(zclk->mask)) & zclk->mask;
-	writel(val, zclk->reg);
+	cpg_reg_modify(zclk->reg, zclk->mask,
+		       ((32 - mult) << __ffs(zclk->mask)) & zclk->mask);
 
 	/*
 	 * Set KICK bit in FRQCRB to update hardware setting and wait for
 	 * clock change completion.
 	 */
-	kick = readl(zclk->kick_reg);
-	kick |= CPG_FRQCRB_KICK;
-	writel(kick, zclk->kick_reg);
+	cpg_reg_modify(zclk->kick_reg, 0, CPG_FRQCRB_KICK);
 
 	/*
 	 * Note: There is no HW information about the worst case latency.
@@ -262,12 +267,10 @@ static const struct sd_div_table cpg_sd_
 static int cpg_sd_clock_enable(struct clk_hw *hw)
 {
 	struct sd_clock *clock = to_sd_clock(hw);
-	u32 val = readl(clock->csn.reg);
-
-	val &= ~(CPG_SD_STP_MASK);
-	val |= clock->div_table[clock->cur_div_idx].val & CPG_SD_STP_MASK;
 
-	writel(val, clock->csn.reg);
+	cpg_reg_modify(clock->csn.reg, CPG_SD_STP_MASK,
+		       clock->div_table[clock->cur_div_idx].val &
+		       CPG_SD_STP_MASK);
 
 	return 0;
 }
@@ -276,7 +279,7 @@ static void cpg_sd_clock_disable(struct
 {
 	struct sd_clock *clock = to_sd_clock(hw);
 
-	writel(readl(clock->csn.reg) | CPG_SD_STP_MASK, clock->csn.reg);
+	cpg_reg_modify(clock->csn.reg, 0, CPG_SD_STP_MASK);
 }
 
 static int cpg_sd_clock_is_enabled(struct clk_hw *hw)
@@ -323,7 +326,6 @@ static int cpg_sd_clock_set_rate(struct
 {
 	struct sd_clock *clock = to_sd_clock(hw);
 	unsigned int div = cpg_sd_clock_calc_div(clock, rate, parent_rate);
-	u32 val;
 	unsigned int i;
 
 	for (i = 0; i < clock->div_num; i++)
@@ -335,10 +337,9 @@ static int cpg_sd_clock_set_rate(struct
 
 	clock->cur_div_idx = i;
 
-	val = readl(clock->csn.reg);
-	val &= ~(CPG_SD_STP_MASK | CPG_SD_FC_MASK);
-	val |= clock->div_table[i].val & (CPG_SD_STP_MASK | CPG_SD_FC_MASK);
-	writel(val, clock->csn.reg);
+	cpg_reg_modify(clock->csn.reg, CPG_SD_STP_MASK | CPG_SD_FC_MASK,
+		       clock->div_table[i].val &
+		       (CPG_SD_STP_MASK | CPG_SD_FC_MASK));
 
 	return 0;
 }

Re: [PATCH 1/4] clk: renesas: rcar-gen3-cpg: factor out cpg_reg_modify()

[Geert Uytterhoeven]

On Thu, Nov 22, 2018 at 7:39 PM Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> There's quite often repeated sequence of a CPG register read-modify-write,
> so it seems worth factoring it out into a function -- this saves 68 bytes
> of the object code already (AArch64 gcc 4.8.5) and will save more with the
> next patches...
>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

Re: [PATCH 1/4] clk: renesas: rcar-gen3-cpg: factor out cpg_reg_modify()

[Simon Horman]

On Thu, Nov 22, 2018 at 09:39:42PM +0300, Sergei Shtylyov wrote:
> There's quite often repeated sequence of a CPG register read-modify-write,
> so it seems worth factoring it out into a function -- this saves 68 bytes
> of the object code already (AArch64 gcc 4.8.5) and will save more with the
> next patches...
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

[PATCH 2/4] clk: renesas: rcar-gen3-cpg: add RPC clock

[Sergei Shtylyov]

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(+)

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,

Re: [PATCH 2/4] clk: renesas: rcar-gen3-cpg: add RPC clock

[Geert Uytterhoeven]

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

Re: [PATCH 2/4] clk: renesas: rcar-gen3-cpg: add RPC clock

[Sergei Shtylyov]

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

Re: [PATCH 2/4] clk: renesas: rcar-gen3-cpg: add RPC clock

[Geert Uytterhoeven]

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

-- 
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

[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 <sergei.shtylyov@cogentembedded.com>

---
 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 = &cpg_rpcd2_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;
@@ -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,

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
<sergei.shtylyov@cogentembedded.com> 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 <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
> @@ -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 = &cpg_rpcd2_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() */

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 -- 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

[PATCH 4/4] clk: renesas: r8a77980-cpg-mssr: add RPC clocks

[Sergei Shtylyov]

Add the RPCSRC internal clock on R-Car V3H (R8A77980) -- it's controlled
by the RPCCKCR.DIV[4:3] on all the R-Car gen3 SoCs except V3M (R8A77970)
but the encoding of this field is different between SoCs.

Add the RPC[D2] clocks (derived from this internal clock) and the RPC-IF
module clock as well...

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
 drivers/clk/renesas/r8a77980-cpg-mssr.c |   40 +++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

Index: renesas-drivers/drivers/clk/renesas/r8a77980-cpg-mssr.c
===================================================================
--- renesas-drivers.orig/drivers/clk/renesas/r8a77980-cpg-mssr.c
+++ renesas-drivers/drivers/clk/renesas/r8a77980-cpg-mssr.c
@@ -10,6 +10,7 @@
  * Copyright (C) 2015 Glider bvba
  */
 
+#include <linux/clk-provider.h>
 #include <linux/device.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
@@ -21,6 +22,10 @@
 #include "renesas-cpg-mssr.h"
 #include "rcar-gen3-cpg.h"
 
+enum r8a77980_clk_types {
+	CLK_TYPE_R8A77980_RPCSRC = CLK_TYPE_GEN3_SOC_BASE,
+};
+
 enum clk_ids {
 	/* Core Clock Outputs exported to DT */
 	LAST_DT_CORE_CLK = R8A77980_CLK_OSC,
@@ -41,12 +46,17 @@ enum clk_ids {
 	CLK_S2,
 	CLK_S3,
 	CLK_SDSRC,
+	CLK_RPCSRC,
 	CLK_OCO,
 
 	/* Module Clocks */
 	MOD_CLK_BASE
 };
 
+static const struct clk_div_table cpg_rpcsrc_div_table[] = {
+	{ 2, 5 }, { 3, 6 }, { 0, 0 },
+};
+
 static const struct cpg_core_clk r8a77980_core_clks[] __initconst = {
 	/* External Clock Inputs */
 	DEF_INPUT("extal",  CLK_EXTAL),
@@ -65,8 +75,14 @@ static const struct cpg_core_clk r8a7798
 	DEF_FIXED(".s2",	CLK_S2,		   CLK_PLL1_DIV2,  4, 1),
 	DEF_FIXED(".s3",	CLK_S3,		   CLK_PLL1_DIV2,  6, 1),
 	DEF_FIXED(".sdsrc",	CLK_SDSRC,	   CLK_PLL1_DIV2,  2, 1),
+	DEF_BASE(".rpcsrc",	CLK_RPCSRC, CLK_TYPE_R8A77980_RPCSRC, CLK_PLL1),
 	DEF_RATE(".oco",	CLK_OCO,           32768),
 
+	DEF_BASE("rpc",		R8A77980_CLK_RPC, CLK_TYPE_GEN3_RPC,
+		 CLK_RPCSRC),
+	DEF_BASE("rpcd2",	R8A77980_CLK_RPCD2, CLK_TYPE_GEN3_RPCD2,
+		 R8A77980_CLK_RPC),
+
 	/* Core Clock Outputs */
 	DEF_FIXED("ztr",	R8A77980_CLK_ZTR,   CLK_PLL1_DIV2,  6, 1),
 	DEF_FIXED("ztrd2",	R8A77980_CLK_ZTRD2, CLK_PLL1_DIV2, 12, 1),
@@ -164,6 +180,7 @@ static const struct mssr_mod_clk r8a7798
 	DEF_MOD("gpio1",		 911,	R8A77980_CLK_CP),
 	DEF_MOD("gpio0",		 912,	R8A77980_CLK_CP),
 	DEF_MOD("can-fd",		 914,	R8A77980_CLK_S3D2),
+	DEF_MOD("rpc-if",		 917,	R8A77980_CLK_RPC),
 	DEF_MOD("i2c4",			 927,	R8A77980_CLK_S0D6),
 	DEF_MOD("i2c3",			 928,	R8A77980_CLK_S0D6),
 	DEF_MOD("i2c2",			 929,	R8A77980_CLK_S3D2),
@@ -215,6 +232,27 @@ static int __init r8a77980_cpg_mssr_init
 	return rcar_gen3_cpg_init(cpg_pll_config, CLK_EXTALR, cpg_mode);
 }
 
+static struct clk * __init r8a77980_cpg_clk_register(struct device *dev,
+	const struct cpg_core_clk *core, const struct cpg_mssr_info *info,
+	struct clk **clks, void __iomem *base,
+	struct raw_notifier_head *notifiers)
+{
+	if (core->type == CLK_TYPE_R8A77980_RPCSRC) {
+		const struct clk *parent = clks[core->parent];
+
+		if (IS_ERR(parent))
+			return ERR_CAST(parent);
+
+		return clk_register_divider_table(NULL, core->name,
+						  __clk_get_name(parent), 0,
+						  base + CPG_RPCCKCR, 3, 2, 0,
+						  cpg_rpcsrc_div_table, NULL);
+	} else	{
+		return rcar_gen3_cpg_clk_register(dev, core, info, clks, base,
+						  notifiers);
+	}
+}
+
 const struct cpg_mssr_info r8a77980_cpg_mssr_info __initconst = {
 	/* Core Clocks */
 	.core_clks = r8a77980_core_clks,
@@ -233,5 +271,5 @@ const struct cpg_mssr_info r8a77980_cpg_
 
 	/* Callbacks */
 	.init = r8a77980_cpg_mssr_init,
-	.cpg_clk_register = rcar_gen3_cpg_clk_register,
+	.cpg_clk_register = r8a77980_cpg_clk_register,
 };

Re: [PATCH 4/4] clk: renesas: r8a77980-cpg-mssr: add RPC clocks

[Geert Uytterhoeven]

Hi Sergei,

Thanks for your patch!

On Thu, Nov 22, 2018 at 7:45 PM Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> Add the RPCSRC internal clock on R-Car V3H (R8A77980) -- it's controlled
> by the RPCCKCR.DIV[4:3] on all the R-Car gen3 SoCs except V3M (R8A77970)
> but the encoding of this field is different between SoCs.

Given the tables and encoding are the same on H3, M3-W, M3-N, and V3H,
I think it makes sense to move the common support to rcar-gen3-cpg.c.

Heck, you could even just select a different table on D3/E3 using
soc_device_match(), if only one encoding would not select a different parent
clock :-(

> Add the RPC[D2] clocks (derived from this internal clock) and the RPC-IF
> module clock as well...
>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

> --- renesas-drivers.orig/drivers/clk/renesas/r8a77980-cpg-mssr.c
> +++ renesas-drivers/drivers/clk/renesas/r8a77980-cpg-mssr.c

> @@ -21,6 +22,10 @@
>  #include "renesas-cpg-mssr.h"
>  #include "rcar-gen3-cpg.h"
>
> +enum r8a77980_clk_types {
> +       CLK_TYPE_R8A77980_RPCSRC = CLK_TYPE_GEN3_SOC_BASE,

Rename and move to rcar_gen3_clk_types?

> @@ -215,6 +232,27 @@ static int __init r8a77980_cpg_mssr_init
>         return rcar_gen3_cpg_init(cpg_pll_config, CLK_EXTALR, cpg_mode);
>  }
>
> +static struct clk * __init r8a77980_cpg_clk_register(struct device *dev,
> +       const struct cpg_core_clk *core, const struct cpg_mssr_info *info,
> +       struct clk **clks, void __iomem *base,
> +       struct raw_notifier_head *notifiers)
> +{
> +       if (core->type == CLK_TYPE_R8A77980_RPCSRC) {

I'd use a switch() statement here, for consistency with other drivers.

> +               const struct clk *parent = clks[core->parent];
> +
> +               if (IS_ERR(parent))
> +                       return ERR_CAST(parent);
> +
> +               return clk_register_divider_table(NULL, core->name,
> +                                                 __clk_get_name(parent), 0,
> +                                                 base + CPG_RPCCKCR, 3, 2, 0,
> +                                                 cpg_rpcsrc_div_table, NULL);

Don't you need a spinlock (last parameter, currently NULL)?
This needs to be synchronized with controlling the RPCD2 and RPC clocks,
as they operate on the same register.

However, that would deadlock, as enabling e.g. RPC-IF will enable
all parent clocks?

> +       } else  {
> +               return rcar_gen3_cpg_clk_register(dev, core, info, clks, base,
> +                                                 notifiers);
> +       }

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

Re: [PATCH 4/4] clk: renesas: r8a77980-cpg-mssr: add RPC clocks

[Sergei Shtylyov]

On 11/23/2018 03:59 PM, Geert Uytterhoeven wrote:

>> Add the RPCSRC internal clock on R-Car V3H (R8A77980) -- it's controlled
>> by the RPCCKCR.DIV[4:3] on all the R-Car gen3 SoCs except V3M (R8A77970)
>> but the encoding of this field is different between SoCs.
> 
> Given the tables and encoding are the same on H3, M3-W, M3-N, and V3H,
> I think it makes sense to move the common support to rcar-gen3-cpg.c.

   Done.

> Heck, you could even just select a different table on D3/E3 using
> soc_device_match(), if only one encoding would not select a different parent
> clock :-(

   Indeed...

>> Add the RPC[D2] clocks (derived from this internal clock) and the RPC-IF
>> module clock as well...
>>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
>> --- renesas-drivers.orig/drivers/clk/renesas/r8a77980-cpg-mssr.c
>> +++ renesas-drivers/drivers/clk/renesas/r8a77980-cpg-mssr.c
[...]
>> +               const struct clk *parent = clks[core->parent];
>> +
>> +               if (IS_ERR(parent))
>> +                       return ERR_CAST(parent);
>> +
>> +               return clk_register_divider_table(NULL, core->name,
>> +                                                 __clk_get_name(parent), 0,
>> +                                                 base + CPG_RPCCKCR, 3, 2, 0,
>> +                                                 cpg_rpcsrc_div_table, NULL);
> 
> Don't you need a spinlock (last parameter, currently NULL)?
> This needs to be synchronized with controlling the RPCD2 and RPC clocks,
> as they operate on the same register.

   Indeed. How about the RMW accesses to the other register? I'd like to place
the lock/unlock right in cpg_reg_modify()...

> However, that would deadlock, as enabling e.g. RPC-IF will enable
> all parent clocks?

  Could toy please elaborate?

>> +       } else  {
>> +               return rcar_gen3_cpg_clk_register(dev, core, info, clks, base,
>> +                                                 notifiers);
>> +       }
> 
> Gr{oetje,eeting}s,
> 
>                         Geert

MBR, Sergei

Re: [PATCH 4/4] clk: renesas: r8a77980-cpg-mssr: add RPC clocks

[Geert Uytterhoeven]

Hi Sergei,

On Tue, Nov 27, 2018 at 6:45 PM Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> On 11/23/2018 03:59 PM, Geert Uytterhoeven wrote:
> >> Add the RPCSRC internal clock on R-Car V3H (R8A77980) -- it's controlled
> >> by the RPCCKCR.DIV[4:3] on all the R-Car gen3 SoCs except V3M (R8A77970)
> >> but the encoding of this field is different between SoCs.

> >> --- renesas-drivers.orig/drivers/clk/renesas/r8a77980-cpg-mssr.c
> >> +++ renesas-drivers/drivers/clk/renesas/r8a77980-cpg-mssr.c
> [...]
> >> +               const struct clk *parent = clks[core->parent];
> >> +
> >> +               if (IS_ERR(parent))
> >> +                       return ERR_CAST(parent);
> >> +
> >> +               return clk_register_divider_table(NULL, core->name,
> >> +                                                 __clk_get_name(parent), 0,
> >> +                                                 base + CPG_RPCCKCR, 3, 2, 0,
> >> +                                                 cpg_rpcsrc_div_table, NULL);
> >
> > Don't you need a spinlock (last parameter, currently NULL)?
> > This needs to be synchronized with controlling the RPCD2 and RPC clocks,
> > as they operate on the same register.
>
>    Indeed. How about the RMW accesses to the other register? I'd like to place
> the lock/unlock right in cpg_reg_modify()...

Yes, RMW, too.

> > However, that would deadlock, as enabling e.g. RPC-IF will enable
> > all parent clocks?
>
>   Could toy please elaborate?

Sorry, I incorrectly assumed it would hold the lock while calling into the
parent, which would deadlock if using the same lock.
But as it only holds the lock for register access, it's fine.

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