[v5,02/05] clk: shmobile: Add Renesas R-Car Gen3 CPG support

Commit Message

Magnus Damm Aug. 31, 2015, 12:49 p.m. UTC

From: Gaku Inami <[e-mail]>

This V5 patch adds initial CPG support for R-Car Generation 3 and in
particular the R8A7795 SoC.

The R-Car Gen3 clock hardware has a register write protection feature that
needs to be shared between the CPG function needs to be shared between
the CPG and MSTP hardware somehow. So far this feature is simply ignored.

Signed-off-by: Gaku Inami <[e-mail]>
Signed-off-by: Kuninori Morimoto <[e-mail]>
Signed-off-by: Magnus Damm <[e-mail]>
---

 Changes since V4: (Magnus Damm <[e-mail]>)
 - Simplified clks array handling - thanks Geert!
 - Updated th DT binding documentation to reflect latest state
 - of_property_count_strings() -> of_property_count_u32_elems() fix

 Changes since V3: (Magnus Damm <[e-mail]>)
 - Reworked driver to incorporate most feedback from Stephen Boyd - thanks!!
 - Major things like syscon and driver model require more discussion.
 - Added hunk to build drivers/clk/shmobile if ARCH_RENESAS is set.

 Changes since V2: (Magnus Damm <[e-mail]>)
 - Reworked driver to rely on clock index instead of strings.
 - Dropped use of "clock-output-names".

 Earlier versions: Kuninori Morimoto <[e-mail]>
 Initial version: Gaku Inami <[e-mail]>

 Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt |   32 +
 drivers/clk/Makefile                                                     |    1 
 drivers/clk/shmobile/Makefile                                            |    1 
 drivers/clk/shmobile/clk-rcar-gen3.c                                     |  245 ++++++++++
 4 files changed, 279 insertions(+)

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Laurent Pinchart Sept. 1, 2015, 6 a.m. UTC | #1

Hi Magnus,

Thank you for the patch.

On Monday 31 August 2015 21:49:04 Magnus Damm wrote:
> From: Gaku Inami <[e-mail]>
> 
> This V5 patch adds initial CPG support for R-Car Generation 3 and in
> particular the R8A7795 SoC.
> 
> The R-Car Gen3 clock hardware has a register write protection feature that
> needs to be shared between the CPG function needs to be shared between
> the CPG and MSTP hardware somehow. So far this feature is simply ignored.
> 
> Signed-off-by: Gaku Inami <[e-mail]>
> Signed-off-by: Kuninori Morimoto <[e-mail]>
> Signed-off-by: Magnus Damm <[e-mail]>
> ---
> 
>  Changes since V4: (Magnus Damm <[e-mail]>)
>  - Simplified clks array handling - thanks Geert!
>  - Updated th DT binding documentation to reflect latest state
>  - of_property_count_strings() -> of_property_count_u32_elems() fix
> 
>  Changes since V3: (Magnus Damm <[e-mail]>)
>  - Reworked driver to incorporate most feedback from Stephen Boyd - thanks!!
>  - Major things like syscon and driver model require more discussion.
>  - Added hunk to build drivers/clk/shmobile if ARCH_RENESAS is set.
> 
>  Changes since V2: (Magnus Damm <[e-mail]>)
>  - Reworked driver to rely on clock index instead of strings.
>  - Dropped use of "clock-output-names".
> 
>  Earlier versions: Kuninori Morimoto <[e-mail]>
>  Initial version: Gaku Inami <[e-mail]>
> 
>  Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt | 
>  32 +
>  drivers/clk/Makefile                                     |    1
>  drivers/clk/shmobile/Makefile                            |    1
>  drivers/clk/shmobile/clk-rcar-gen3.c                     |  245 ++++++++++
>  4 files changed, 279 insertions(+)
> 
> --- /dev/null
> +++
> work/Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.t
> xt	2015-08-31 15:49:10.000000000 +0900 @@ -0,0 +1,32 @@
> +* Renesas R-Car Gen3 Clock Pulse Generator (CPG)
> +
> +The CPG generates core clocks for the R-Car Gen3 SoCs. It includes three
> PLLs
> +and several fixed ratio dividers.
> +
> +Required Properties:
> +
> +  - compatible: Must be one of
> +    - "renesas,r8a7795-cpg-clocks" for the r8a7795 CPG
> +    - "renesas,rcar-gen3-cpg-clocks" for the generic R-Car Gen3 CPG
> +
> +  - reg: Base address and length of the memory resource used by the CPG
> +
> +  - clocks: References to the parent clocks: first to the EXTAL clock
> +  - #clock-cells: Must be 1
> +  - clock-indices: Indices of the exported clocks

Do we actually need the clock-indices property, as CPG clocks are numbered 
sequentially ? It seems to me like we could drop the property, especially 
given that the number of clocks is hardcoded in the driver anyway.

> +
> +Example
> +-------
> +
> +	cpg_clocks: cpg_clocks@e6150000 {
> +		compatible = "renesas,r8a7795-cpg-clocks",
> +			     "renesas,rcar-gen3-cpg-clocks";
> +		reg = <0 0xe6150000 0 0x1000>;
> +		clocks = <&extal_clk>;
> +		#clock-cells = <1>;
> +                clock-indices = <
> +                             R8A7795_CLK_MAIN R8A7795_CLK_PLL0
> +                             R8A7795_CLK_PLL1 R8A7795_CLK_PLL2
> +                             R8A7795_CLK_PLL3 R8A7795_CLK_PLL4
> +                >;
> +	};
> --- 0001/drivers/clk/Makefile
> +++ work/drivers/clk/Makefile	2015-08-31 15:49:09.072366518 +0900
> @@ -67,6 +67,7 @@ obj-$(CONFIG_COMMON_CLK_QCOM)		+= qcom/
>  obj-$(CONFIG_ARCH_ROCKCHIP)		+= rockchip/
>  obj-$(CONFIG_COMMON_CLK_SAMSUNG)	+= samsung/
>  obj-$(CONFIG_ARCH_SHMOBILE_MULTI)	+= shmobile/
> +obj-$(CONFIG_ARCH_RENESAS)		+= shmobile/
>  obj-$(CONFIG_ARCH_SIRF)			+= sirf/
>  obj-$(CONFIG_ARCH_SOCFPGA)		+= socfpga/
>  obj-$(CONFIG_PLAT_SPEAR)		+= spear/
> --- 0006/drivers/clk/shmobile/Makefile
> +++ work/drivers/clk/shmobile/Makefile	2015-08-31 15:49:09.072366518 +0900
> @@ -8,4 +8,5 @@ obj-$(CONFIG_ARCH_R8A7790)		+= clk-rcar-
>  obj-$(CONFIG_ARCH_R8A7791)		+= clk-rcar-gen2.o clk-mstp.o clk-div6.o
>  obj-$(CONFIG_ARCH_R8A7793)		+= clk-rcar-gen2.o clk-mstp.o clk-div6.o
>  obj-$(CONFIG_ARCH_R8A7794)		+= clk-rcar-gen2.o clk-mstp.o clk-div6.o
> +obj-$(CONFIG_ARCH_R8A7795)		+= clk-rcar-gen3.o clk-mstp.o clk-div6.o
>  obj-$(CONFIG_ARCH_SH73A0)		+= clk-sh73a0.o clk-mstp.o clk-div6.o
> --- /dev/null
> +++ work/drivers/clk/shmobile/clk-rcar-gen3.c	2015-08-31 
21:10:01.102366518
> +0900 @@ -0,0 +1,245 @@
> +/*
> + * rcar_gen3 Core CPG Clocks
> + *
> + * Copyright (C) 2015 Renesas Electronics Corp.
> + *
> + * Based on rcar_gen2 Core CPG Clocks driver.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/clk/shmobile.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +#define RCAR_GEN3_CLK_MAIN	0
> +#define RCAR_GEN3_CLK_PLL0	1
> +#define RCAR_GEN3_CLK_PLL1	2
> +#define RCAR_GEN3_CLK_PLL2	3
> +#define RCAR_GEN3_CLK_PLL3	4
> +#define RCAR_GEN3_CLK_PLL4	5
> +#define RCAR_GEN3_CLK_NR	6
> +
> +static const char * const rcar_gen3_clk_names[RCAR_GEN3_CLK_NR] = {
> +	[RCAR_GEN3_CLK_MAIN] = "main",
> +	[RCAR_GEN3_CLK_PLL0] = "pll0",
> +	[RCAR_GEN3_CLK_PLL1] = "pll1",
> +	[RCAR_GEN3_CLK_PLL2] = "pll2",
> +	[RCAR_GEN3_CLK_PLL3] = "pll3",
> +	[RCAR_GEN3_CLK_PLL4] = "pll4",
> +};
> +
> +struct rcar_gen3_cpg {
> +	struct clk_onecell_data data;
> +	spinlock_t lock;
> +	void __iomem *reg;
> +	struct clk *clks[RCAR_GEN3_CLK_NR];
> +};
> +
> +#define CPG_PLL0CR	0x00d8
> +#define CPG_PLL2CR	0x002c
> +
> +/*
> + * common function
> + */
> +#define rcar_clk_readl(cpg, _reg) readl(cpg->reg + _reg)
> +
> +/*
> + * Reset register definitions.
> + */
> +#define MODEMR 0xe6160060
> +
> +static u32 rcar_gen3_read_mode_pins(void)
> +{
> +	static u32 mode;
> +	static bool mode_valid;
> +
> +	if (!mode_valid) {
> +		void __iomem *modemr = ioremap_nocache(MODEMR, 4);
> +
> +		BUG_ON(!modemr);
> +		mode = ioread32(modemr);
> +		iounmap(modemr);
> +		mode_valid = true;
> +	}
> +
> +	return mode;
> +}
> +
> +/* ------------------------------------------------------------------------
> + * CPG Clock Data
> + */
> +
> +/*
> + *   MD		EXTAL		PLL0	PLL1	PLL2	PLL3	PLL4
> + * 14 13 19 17	(MHz)		*1	*1	*1
> + *-------------------------------------------------------------------
> + * 0  0  0  0	16.66 x 1	x180/2	x192/2	x144/2	x192	x144
> + * 0  0  0  1	16.66 x 1	x180/2	x192/2	x144/2	x128	x144
> + * 0  0  1  0	Prohibited setting
> + * 0  0  1  1	16.66 x 1	x180/2	x192/2	x144/2	x192	x144
> + * 0  1  0  0	20    x 1	x150/2	x156/2	x120/2	x156	x120
> + * 0  1  0  1	20    x 1	x150/2	x156/2	x120/2	x106	x120
> + * 0  1  1  0	Prohibited setting
> + * 0  1  1  1	20    x 1	x150/2	x156/2	x120/2	x156	x120
> + * 1  0  0  0	25    x 1	x120/2	x128/2	x96/2	x128	x96
> + * 1  0  0  1	25    x 1	x120/2	x128/2	x96/2	x84	x96
> + * 1  0  1  0	Prohibited setting
> + * 1  0  1  1	25    x 1	x120/2	x128/2	x96/2	x128	x96
> + * 1  1  0  0	33.33 / 2	x180/2	x192/2	x144/2	x192	x144
> + * 1  1  0  1	33.33 / 2	x180/2	x192/2	x144/2	x128	x144
> + * 1  1  1  0	Prohibited setting
> + * 1  1  1  1	33.33 / 2	x180/2	x192/2	x144/2	x192	x144
> + *
> + * *1 : datasheet indicates VCO output (PLLx = VCO/2)

As explained in a separate e-mail there's a few clocks on R8A7795 that derive 
directly from PLL1 VCO. I thus wonder whether we shouldn't expose the PLL1 
clock as the VCO output and create VCO/2 using a fixed factor clock in DT.

> + */
> +#define CPG_PLL_CONFIG_INDEX(md)	((((md) & BIT(14)) >> 11) | \
> +					 (((md) & BIT(13)) >> 11) | \
> +					 (((md) & BIT(19)) >> 18) | \
> +					 (((md) & BIT(17)) >> 17))
> +struct cpg_pll_config {
> +	unsigned int extal_div;
> +	unsigned int pll1_mult;
> +	unsigned int pll3_mult;
> +	unsigned int pll4_mult;
> +};
> +
> +static const struct cpg_pll_config cpg_pll_configs[16] __initconst = {
> +/* EXTAL div	PLL1	PLL3	PLL4 */
> +	{ 1,	192,	192,	144, },
> +	{ 1,	192,	128,	144, },
> +	{ 0,	0,	0,	0,   }, /* Prohibited setting */
> +	{ 1,	192,	192,	144, },
> +	{ 1,	156,	156,	120, },
> +	{ 1,	156,	106,	120, },
> +	{ 0,	0,	0,	0,   }, /* Prohibited setting */
> +	{ 1,	156,	156,	120, },
> +	{ 1,	128,	128,	96,  },
> +	{ 1,	128,	84,	96,  },
> +	{ 0,	0,	0,	0,   }, /* Prohibited setting */
> +	{ 1,	128,	128,	96,  },
> +	{ 2,	192,	192,	144, },
> +	{ 2,	192,	128,	144, },
> +	{ 0,	0,	0,	0,   }, /* Prohibited setting */
> +	{ 2,	192,	192,	144, },
> +};
> +
> +/* ------------------------------------------------------------------------
> + * Initialization
> + */
> +
> +static struct clk * __init
> +rcar_gen3_cpg_register_clock(struct device_node *np, struct rcar_gen3_cpg
> *cpg,
> +			     const struct cpg_pll_config *config,
> +			     unsigned int gen3_clk)
> +{
> +	const char *parent_name = rcar_gen3_clk_names[RCAR_GEN3_CLK_MAIN];
> +	unsigned int mult = 1;
> +	unsigned int div = 1;
> +	u32 value;
> +
> +	switch (gen3_clk) {
> +	case RCAR_GEN3_CLK_MAIN:
> +		parent_name = of_clk_get_parent_name(np, 0);
> +		div = config->extal_div;
> +		break;
> +	case RCAR_GEN3_CLK_PLL0:
> +		/* PLL0 is a configurable multiplier clock. Register it as a
> +		 * fixed factor clock for now as there's no generic multiplier
> +		 * clock implementation and we currently have no need to change
> +		 * the multiplier value.
> +		 */
> +		value = rcar_clk_readl(cpg, CPG_PLL0CR);
> +		mult = ((value >> 24) & ((1 << 7) - 1)) + 1;

I'd find 0x3f more readable than (1 << 7) - 1 but that might just be me. Same 
comment for PLL2.

> +		break;
> +	case RCAR_GEN3_CLK_PLL1:
> +		mult = config->pll1_mult / 2;
> +		break;
> +	case RCAR_GEN3_CLK_PLL2:
> +		/* PLL2 is a configurable multiplier clock. Register it as a
> +		 * fixed factor clock for now as there's no generic multiplier
> +		 * clock implementation and we currently have no need to change
> +		 * the multiplier value.
> +		 */
> +		value = rcar_clk_readl(cpg, CPG_PLL2CR);
> +		mult = ((value >> 24) & ((1 << 7) - 1)) + 1;
> +		break;
> +	case RCAR_GEN3_CLK_PLL3:
> +		mult = config->pll3_mult;
> +		break;
> +	case RCAR_GEN3_CLK_PLL4:
> +		mult = config->pll4_mult;
> +		break;
> +	default:
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	return clk_register_fixed_factor(NULL, rcar_gen3_clk_names[gen3_clk],
> +					 parent_name, 0, mult, div);
> +}
> +
> +static void __init rcar_gen3_cpg_clocks_init(struct device_node *np)
> +{
> +	const struct cpg_pll_config *config;
> +	struct rcar_gen3_cpg *cpg;
> +	u32 cpg_mode;
> +	unsigned int i;
> +	int num_clks;
> +
> +	cpg_mode = rcar_gen3_read_mode_pins();
> +
> +	num_clks = of_property_count_u32_elems(np, "clock-indices");
> +	if (num_clks < 0) {
> +		pr_err("%s: failed to count clocks\n", __func__);
> +		return;
> +	}
> +
> +	cpg = kzalloc(sizeof(*cpg), GFP_KERNEL);
> +	if (!cpg)
> +		return;
> +
> +	spin_lock_init(&cpg->lock);

The spin lock is never used. I'd drop it for now, and add it back later 
when/if needed.

> +	cpg->data.clks = cpg->clks;
> +	cpg->data.clk_num = num_clks;
> +
> +	cpg->reg = of_iomap(np, 0);
> +	if (WARN_ON(cpg->reg == NULL))
> +		return;
> +
> +	config = &cpg_pll_configs[CPG_PLL_CONFIG_INDEX(cpg_mode)];
> +	if (!config->extal_div) {
> +		pr_err("%s: Prohibited setting (cpg_mode=0x%x)\n",
> +		       __func__, cpg_mode);
> +		return;
> +	}
> +
> +	for (i = 0; i < num_clks; ++i) {
> +		struct clk *clk;
> +		u32 idx;
> +		int ret;
> +
> +		ret = of_property_read_u32_index(np, "clock-indices", i, &idx);
> +		if (ret < 0)
> +			break;
> +
> +		clk = rcar_gen3_cpg_register_clock(np, cpg, config, idx);
> +		if (IS_ERR(clk))
> +			pr_err("%s: failed to register %s %u clock (%ld)\n",
> +			       __func__, np->name, idx, PTR_ERR(clk));
> +		else
> +			cpg->data.clks[idx] = clk;
> +	}
> +
> +	of_clk_add_provider(np, of_clk_src_onecell_get, &cpg->data);
> +}
> +CLK_OF_DECLARE(rcar_gen3_cpg_clks, "renesas,rcar-gen3-cpg-clocks",
> +	       rcar_gen3_cpg_clocks_init);

Magnus Damm Sept. 1, 2015, 10:59 a.m. UTC | #2

Hi Laurent,

On Tue, Sep 1, 2015 at 3:00 PM, Laurent Pinchart
<[e-mail]> wrote:
> Hi Magnus,
>
> Thank you for the patch.
>
> On Monday 31 August 2015 21:49:04 Magnus Damm wrote:
>> From: Gaku Inami <[e-mail]>
>>
>> This V5 patch adds initial CPG support for R-Car Generation 3 and in
>> particular the R8A7795 SoC.
>>
>> The R-Car Gen3 clock hardware has a register write protection feature that
>> needs to be shared between the CPG function needs to be shared between
>> the CPG and MSTP hardware somehow. So far this feature is simply ignored.
>>
>> Signed-off-by: Gaku Inami <[e-mail]>
>> Signed-off-by: Kuninori Morimoto <[e-mail]>
>> Signed-off-by: Magnus Damm <[e-mail]>
>> ---
>>
>>  Changes since V4: (Magnus Damm <[e-mail]>)
>>  - Simplified clks array handling - thanks Geert!
>>  - Updated th DT binding documentation to reflect latest state
>>  - of_property_count_strings() -> of_property_count_u32_elems() fix
>>
>>  Changes since V3: (Magnus Damm <[e-mail]>)
>>  - Reworked driver to incorporate most feedback from Stephen Boyd - thanks!!
>>  - Major things like syscon and driver model require more discussion.
>>  - Added hunk to build drivers/clk/shmobile if ARCH_RENESAS is set.
>>
>>  Changes since V2: (Magnus Damm <[e-mail]>)
>>  - Reworked driver to rely on clock index instead of strings.
>>  - Dropped use of "clock-output-names".
>>
>>  Earlier versions: Kuninori Morimoto <[e-mail]>
>>  Initial version: Gaku Inami <[e-mail]>
>>
>>  Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt |
>>  32 +
>>  drivers/clk/Makefile                                     |    1
>>  drivers/clk/shmobile/Makefile                            |    1
>>  drivers/clk/shmobile/clk-rcar-gen3.c                     |  245 ++++++++++
>>  4 files changed, 279 insertions(+)
>>
>> --- /dev/null
>> +++
>> work/Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.t
>> xt    2015-08-31 15:49:10.000000000 +0900 @@ -0,0 +1,32 @@
>> +* Renesas R-Car Gen3 Clock Pulse Generator (CPG)
>> +
>> +The CPG generates core clocks for the R-Car Gen3 SoCs. It includes three
>> PLLs
>> +and several fixed ratio dividers.
>> +
>> +Required Properties:
>> +
>> +  - compatible: Must be one of
>> +    - "renesas,r8a7795-cpg-clocks" for the r8a7795 CPG
>> +    - "renesas,rcar-gen3-cpg-clocks" for the generic R-Car Gen3 CPG
>> +
>> +  - reg: Base address and length of the memory resource used by the CPG
>> +
>> +  - clocks: References to the parent clocks: first to the EXTAL clock
>> +  - #clock-cells: Must be 1
>> +  - clock-indices: Indices of the exported clocks
>
> Do we actually need the clock-indices property, as CPG clocks are numbered
> sequentially ? It seems to me like we could drop the property, especially
> given that the number of clocks is hardcoded in the driver anyway.

There are two reasons why they are there:
1) I like to be able to search DT files to see which node that is
providing what clock.
2) When adding support for additional SoCs we may add new index to the
driver. New SoC may get sparsely populated index lists and old ones
will omit the recently added index.

>> +/*
>> + *   MD              EXTAL           PLL0    PLL1    PLL2    PLL3    PLL4
>> + * 14 13 19 17       (MHz)           *1      *1      *1
>> + *-------------------------------------------------------------------
>> + * 0  0  0  0        16.66 x 1       x180/2  x192/2  x144/2  x192    x144
>> + * 0  0  0  1        16.66 x 1       x180/2  x192/2  x144/2  x128    x144
>> + * 0  0  1  0        Prohibited setting
>> + * 0  0  1  1        16.66 x 1       x180/2  x192/2  x144/2  x192    x144
>> + * 0  1  0  0        20    x 1       x150/2  x156/2  x120/2  x156    x120
>> + * 0  1  0  1        20    x 1       x150/2  x156/2  x120/2  x106    x120
>> + * 0  1  1  0        Prohibited setting
>> + * 0  1  1  1        20    x 1       x150/2  x156/2  x120/2  x156    x120
>> + * 1  0  0  0        25    x 1       x120/2  x128/2  x96/2   x128    x96
>> + * 1  0  0  1        25    x 1       x120/2  x128/2  x96/2   x84     x96
>> + * 1  0  1  0        Prohibited setting
>> + * 1  0  1  1        25    x 1       x120/2  x128/2  x96/2   x128    x96
>> + * 1  1  0  0        33.33 / 2       x180/2  x192/2  x144/2  x192    x144
>> + * 1  1  0  1        33.33 / 2       x180/2  x192/2  x144/2  x128    x144
>> + * 1  1  1  0        Prohibited setting
>> + * 1  1  1  1        33.33 / 2       x180/2  x192/2  x144/2  x192    x144
>> + *
>> + * *1 : datasheet indicates VCO output (PLLx = VCO/2)
>
> As explained in a separate e-mail there's a few clocks on R8A7795 that derive
> directly from PLL1 VCO. I thus wonder whether we shouldn't expose the PLL1
> clock as the VCO output and create VCO/2 using a fixed factor clock in DT.

Do you think that would reduce complexity or simplify the code? If so
I think we should do it. Otherwise I think it makes sense to simply
follow the data sheet.

>> + */
>> +#define CPG_PLL_CONFIG_INDEX(md)     ((((md) & BIT(14)) >> 11) | \
>> +                                      (((md) & BIT(13)) >> 11) | \
>> +                                      (((md) & BIT(19)) >> 18) | \
>> +                                      (((md) & BIT(17)) >> 17))
>> +struct cpg_pll_config {
>> +     unsigned int extal_div;
>> +     unsigned int pll1_mult;
>> +     unsigned int pll3_mult;
>> +     unsigned int pll4_mult;
>> +};
>> +
>> +static const struct cpg_pll_config cpg_pll_configs[16] __initconst = {
>> +/* EXTAL div PLL1    PLL3    PLL4 */
>> +     { 1,    192,    192,    144, },
>> +     { 1,    192,    128,    144, },
>> +     { 0,    0,      0,      0,   }, /* Prohibited setting */
>> +     { 1,    192,    192,    144, },
>> +     { 1,    156,    156,    120, },
>> +     { 1,    156,    106,    120, },
>> +     { 0,    0,      0,      0,   }, /* Prohibited setting */
>> +     { 1,    156,    156,    120, },
>> +     { 1,    128,    128,    96,  },
>> +     { 1,    128,    84,     96,  },
>> +     { 0,    0,      0,      0,   }, /* Prohibited setting */
>> +     { 1,    128,    128,    96,  },
>> +     { 2,    192,    192,    144, },
>> +     { 2,    192,    128,    144, },
>> +     { 0,    0,      0,      0,   }, /* Prohibited setting */
>> +     { 2,    192,    192,    144, },
>> +};
>> +
>> +/* ------------------------------------------------------------------------
>> + * Initialization
>> + */
>> +
>> +static struct clk * __init
>> +rcar_gen3_cpg_register_clock(struct device_node *np, struct rcar_gen3_cpg
>> *cpg,
>> +                          const struct cpg_pll_config *config,
>> +                          unsigned int gen3_clk)
>> +{
>> +     const char *parent_name = rcar_gen3_clk_names[RCAR_GEN3_CLK_MAIN];
>> +     unsigned int mult = 1;
>> +     unsigned int div = 1;
>> +     u32 value;
>> +
>> +     switch (gen3_clk) {
>> +     case RCAR_GEN3_CLK_MAIN:
>> +             parent_name = of_clk_get_parent_name(np, 0);
>> +             div = config->extal_div;
>> +             break;
>> +     case RCAR_GEN3_CLK_PLL0:
>> +             /* PLL0 is a configurable multiplier clock. Register it as a
>> +              * fixed factor clock for now as there's no generic multiplier
>> +              * clock implementation and we currently have no need to change
>> +              * the multiplier value.
>> +              */
>> +             value = rcar_clk_readl(cpg, CPG_PLL0CR);
>> +             mult = ((value >> 24) & ((1 << 7) - 1)) + 1;
>
> I'd find 0x3f more readable than (1 << 7) - 1 but that might just be me. Same
> comment for PLL2.

I agree.

>> +             break;
>> +     case RCAR_GEN3_CLK_PLL1:
>> +             mult = config->pll1_mult / 2;
>> +             break;
>> +     case RCAR_GEN3_CLK_PLL2:
>> +             /* PLL2 is a configurable multiplier clock. Register it as a
>> +              * fixed factor clock for now as there's no generic multiplier
>> +              * clock implementation and we currently have no need to change
>> +              * the multiplier value.
>> +              */
>> +             value = rcar_clk_readl(cpg, CPG_PLL2CR);
>> +             mult = ((value >> 24) & ((1 << 7) - 1)) + 1;
>> +             break;
>> +     case RCAR_GEN3_CLK_PLL3:
>> +             mult = config->pll3_mult;
>> +             break;
>> +     case RCAR_GEN3_CLK_PLL4:
>> +             mult = config->pll4_mult;
>> +             break;
>> +     default:
>> +             return ERR_PTR(-EINVAL);
>> +     }
>> +
>> +     return clk_register_fixed_factor(NULL, rcar_gen3_clk_names[gen3_clk],
>> +                                      parent_name, 0, mult, div);
>> +}
>> +
>> +static void __init rcar_gen3_cpg_clocks_init(struct device_node *np)
>> +{
>> +     const struct cpg_pll_config *config;
>> +     struct rcar_gen3_cpg *cpg;
>> +     u32 cpg_mode;
>> +     unsigned int i;
>> +     int num_clks;
>> +
>> +     cpg_mode = rcar_gen3_read_mode_pins();
>> +
>> +     num_clks = of_property_count_u32_elems(np, "clock-indices");
>> +     if (num_clks < 0) {
>> +             pr_err("%s: failed to count clocks\n", __func__);
>> +             return;
>> +     }
>> +
>> +     cpg = kzalloc(sizeof(*cpg), GFP_KERNEL);
>> +     if (!cpg)
>> +             return;
>> +
>> +     spin_lock_init(&cpg->lock);
>
> The spin lock is never used. I'd drop it for now, and add it back later
> when/if needed.

Good point!

>> +     cpg->data.clks = cpg->clks;
>> +     cpg->data.clk_num = num_clks;
>> +
>> +     cpg->reg = of_iomap(np, 0);
>> +     if (WARN_ON(cpg->reg == NULL))
>> +             return;
>> +
>> +     config = &cpg_pll_configs[CPG_PLL_CONFIG_INDEX(cpg_mode)];
>> +     if (!config->extal_div) {
>> +             pr_err("%s: Prohibited setting (cpg_mode=0x%x)\n",
>> +                    __func__, cpg_mode);
>> +             return;
>> +     }
>> +
>> +     for (i = 0; i < num_clks; ++i) {
>> +             struct clk *clk;
>> +             u32 idx;
>> +             int ret;
>> +
>> +             ret = of_property_read_u32_index(np, "clock-indices", i, &idx);
>> +             if (ret < 0)
>> +                     break;
>> +
>> +             clk = rcar_gen3_cpg_register_clock(np, cpg, config, idx);
>> +             if (IS_ERR(clk))
>> +                     pr_err("%s: failed to register %s %u clock (%ld)\n",
>> +                            __func__, np->name, idx, PTR_ERR(clk));
>> +             else
>> +                     cpg->data.clks[idx] = clk;
>> +     }
>> +
>> +     of_clk_add_provider(np, of_clk_src_onecell_get, &cpg->data);
>> +}
>> +CLK_OF_DECLARE(rcar_gen3_cpg_clks, "renesas,rcar-gen3-cpg-clocks",
>> +            rcar_gen3_cpg_clocks_init);

Thanks for your comments!!

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Geert Uytterhoeven Sept. 1, 2015, 11:36 a.m. UTC | #3

Hi Magnus,

On Tue, Sep 1, 2015 at 12:59 PM, Magnus Damm <[e-mail]> wrote:
>>> +/*
>>> + *   MD              EXTAL           PLL0    PLL1    PLL2    PLL3    PLL4
>>> + * 14 13 19 17       (MHz)           *1      *1      *1
>>> + *-------------------------------------------------------------------
>>> + * 0  0  0  0        16.66 x 1       x180/2  x192/2  x144/2  x192    x144
>>> + * 0  0  0  1        16.66 x 1       x180/2  x192/2  x144/2  x128    x144
>>> + * 0  0  1  0        Prohibited setting
>>> + * 0  0  1  1        16.66 x 1       x180/2  x192/2  x144/2  x192    x144
>>> + * 0  1  0  0        20    x 1       x150/2  x156/2  x120/2  x156    x120
>>> + * 0  1  0  1        20    x 1       x150/2  x156/2  x120/2  x106    x120
>>> + * 0  1  1  0        Prohibited setting
>>> + * 0  1  1  1        20    x 1       x150/2  x156/2  x120/2  x156    x120
>>> + * 1  0  0  0        25    x 1       x120/2  x128/2  x96/2   x128    x96
>>> + * 1  0  0  1        25    x 1       x120/2  x128/2  x96/2   x84     x96
>>> + * 1  0  1  0        Prohibited setting
>>> + * 1  0  1  1        25    x 1       x120/2  x128/2  x96/2   x128    x96
>>> + * 1  1  0  0        33.33 / 2       x180/2  x192/2  x144/2  x192    x144
>>> + * 1  1  0  1        33.33 / 2       x180/2  x192/2  x144/2  x128    x144
>>> + * 1  1  1  0        Prohibited setting
>>> + * 1  1  1  1        33.33 / 2       x180/2  x192/2  x144/2  x192    x144
>>> + *
>>> + * *1 : datasheet indicates VCO output (PLLx = VCO/2)
>>
>> As explained in a separate e-mail there's a few clocks on R8A7795 that derive
>> directly from PLL1 VCO. I thus wonder whether we shouldn't expose the PLL1
>> clock as the VCO output and create VCO/2 using a fixed factor clock in DT.
>
> Do you think that would reduce complexity or simplify the code? If so
> I think we should do it. Otherwise I think it makes sense to simply
> follow the data sheet.

It would avoid having to apply a multiplier of two to the RPC clock.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Laurent Pinchart Sept. 1, 2015, 11:51 a.m. UTC | #4

On Tuesday 01 September 2015 13:36:28 Geert Uytterhoeven wrote:
> On Tue, Sep 1, 2015 at 12:59 PM, Magnus Damm <[e-mail]> wrote:
> >>> +/*
> >>> + *   MD              EXTAL           PLL0    PLL1    PLL2    PLL3   
> >>> PLL4
> >>> + * 14 13 19 17       (MHz)           *1      *1      *1
> >>> + *-------------------------------------------------------------------
> >>> + * 0  0  0  0        16.66 x 1       x180/2  x192/2  x144/2  x192   
> >>> x144
> >>> + * 0  0  0  1        16.66 x 1       x180/2  x192/2  x144/2  x128   
> >>> x144
> >>> + * 0  0  1  0        Prohibited setting
> >>> + * 0  0  1  1        16.66 x 1       x180/2  x192/2  x144/2  x192   
> >>> x144
> >>> + * 0  1  0  0        20    x 1       x150/2  x156/2  x120/2  x156   
> >>> x120
> >>> + * 0  1  0  1        20    x 1       x150/2  x156/2  x120/2  x106   
> >>> x120
> >>> + * 0  1  1  0        Prohibited setting
> >>> + * 0  1  1  1        20    x 1       x150/2  x156/2  x120/2  x156   
> >>> x120
> >>> + * 1  0  0  0        25    x 1       x120/2  x128/2  x96/2   x128   
> >>> x96
> >>> + * 1  0  0  1        25    x 1       x120/2  x128/2  x96/2   x84    
> >>> x96
> >>> + * 1  0  1  0        Prohibited setting
> >>> + * 1  0  1  1        25    x 1       x120/2  x128/2  x96/2   x128   
> >>> x96
> >>> + * 1  1  0  0        33.33 / 2       x180/2  x192/2  x144/2  x192   
> >>> x144
> >>> + * 1  1  0  1        33.33 / 2       x180/2  x192/2  x144/2  x128   
> >>> x144
> >>> + * 1  1  1  0        Prohibited setting
> >>> + * 1  1  1  1        33.33 / 2       x180/2  x192/2  x144/2  x192   
> >>> x144
> >>> + *
> >>> + * *1 : datasheet indicates VCO output (PLLx = VCO/2)
> >> 
> >> As explained in a separate e-mail there's a few clocks on R8A7795 that
> >> derive directly from PLL1 VCO. I thus wonder whether we shouldn't expose
> >> the PLL1 clock as the VCO output and create VCO/2 using a fixed factor
> >> clock in DT.
> >
> > Do you think that would reduce complexity or simplify the code? If so
> > I think we should do it. Otherwise I think it makes sense to simply
> > follow the data sheet.
> 
> It would avoid having to apply a multiplier of two to the RPC clock.

And it would match the hardware clock tree topology. Applying a multiplier to 
the RPC clock is a hack I can live with though, so I'll let you decide which 
option is best, but in general matching the hardware seems a good idea to me.

Laurent Pinchart Sept. 1, 2015, 12:30 p.m. UTC | #5

Hi Magnus,

On Tuesday 01 September 2015 19:59:01 Magnus Damm wrote:
> On Tue, Sep 1, 2015 at 3:00 PM, Laurent Pinchart wrote:
> > On Monday 31 August 2015 21:49:04 Magnus Damm wrote:
> >> From: Gaku Inami <[e-mail]>
> >> 
> >> This V5 patch adds initial CPG support for R-Car Generation 3 and in
> >> particular the R8A7795 SoC.
> >> 
> >> The R-Car Gen3 clock hardware has a register write protection feature
> >> that needs to be shared between the CPG function needs to be shared
> >> between the CPG and MSTP hardware somehow. So far this feature is simply
> >> ignored.
> >> 
> >> Signed-off-by: Gaku Inami <[e-mail]>
> >> Signed-off-by: Kuninori Morimoto <[e-mail]>
> >> Signed-off-by: Magnus Damm <[e-mail]>
> >> ---
> >> 
> >>  Changes since V4: (Magnus Damm <[e-mail]>)
> >>  - Simplified clks array handling - thanks Geert!
> >>  - Updated th DT binding documentation to reflect latest state
> >>  - of_property_count_strings() -> of_property_count_u32_elems() fix
> >>  
> >>  Changes since V3: (Magnus Damm <[e-mail]>)
> >>  - Reworked driver to incorporate most feedback from Stephen Boyd -
> >>  thanks!!
> >>  - Major things like syscon and driver model require more discussion.
> >>  - Added hunk to build drivers/clk/shmobile if ARCH_RENESAS is set.
> >>  
> >>  Changes since V2: (Magnus Damm <[e-mail]>)
> >>  - Reworked driver to rely on clock index instead of strings.
> >>  - Dropped use of "clock-output-names".
> >>  
> >>  Earlier versions: Kuninori Morimoto <[e-mail]>
> >>  Initial version: Gaku Inami <[e-mail]>
> >>  
> >>  Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt
> >>  |   32 +
> >>  drivers/clk/Makefile                                |    1
> >>  drivers/clk/shmobile/Makefile                       |    1
> >>  drivers/clk/shmobile/clk-rcar-gen3.c                |  245 ++++++++++
> >>  4 files changed, 279 insertions(+)
> >> 
> >> --- /dev/null
> >> +++
> >> work/Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks
> >> .txt
> >> 2015-08-31 15:49:10.000000000 +0900 @@ -0,0 +1,32 @@
> >> +* Renesas R-Car Gen3 Clock Pulse Generator (CPG)
> >> +
> >> +The CPG generates core clocks for the R-Car Gen3 SoCs. It includes three
> >> PLLs
> >> +and several fixed ratio dividers.
> >> +
> >> +Required Properties:
> >> +
> >> +  - compatible: Must be one of
> >> +    - "renesas,r8a7795-cpg-clocks" for the r8a7795 CPG
> >> +    - "renesas,rcar-gen3-cpg-clocks" for the generic R-Car Gen3 CPG
> >> +
> >> +  - reg: Base address and length of the memory resource used by the CPG
> >> +
> >> +  - clocks: References to the parent clocks: first to the EXTAL clock
> >> +  - #clock-cells: Must be 1
> >> +  - clock-indices: Indices of the exported clocks
> > 
> > Do we actually need the clock-indices property, as CPG clocks are numbered
> > sequentially ? It seems to me like we could drop the property, especially
> > given that the number of clocks is hardcoded in the driver anyway.
> 
> There are two reasons why they are there:
> 1) I like to be able to search DT files to see which node that is
> providing what clock.

The clocks are listed in the CPG section of include/dt-bindings/clock/r8a7795-
clock.h so that should already give a hint. Additionally I don't think you'll 
search the CPG core clocks very often in the DT sources as they're usually not 
used directly by devices :-)

> 2) When adding support for additional SoCs we may add new index to the
> driver. New SoC may get sparsely populated index lists and old ones
> will omit the recently added index.

If the CPG core differs between SoCs shouldn't that be handled by the DT 
compatibility string ?

> >> +/*
> >> + *   MD              EXTAL           PLL0    PLL1    PLL2    PLL3   
> >> PLL4
> >> + * 14 13 19 17       (MHz)           *1      *1      *1
> >> + *-------------------------------------------------------------------
> >> + * 0  0  0  0        16.66 x 1       x180/2  x192/2  x144/2  x192   
> >> x144
> >> + * 0  0  0  1        16.66 x 1       x180/2  x192/2  x144/2  x128   
> >> x144
> >> + * 0  0  1  0        Prohibited setting
> >> + * 0  0  1  1        16.66 x 1       x180/2  x192/2  x144/2  x192   
> >> x144
> >> + * 0  1  0  0        20    x 1       x150/2  x156/2  x120/2  x156   
> >> x120
> >> + * 0  1  0  1        20    x 1       x150/2  x156/2  x120/2  x106   
> >> x120
> >> + * 0  1  1  0        Prohibited setting
> >> + * 0  1  1  1        20    x 1       x150/2  x156/2  x120/2  x156   
> >> x120
> >> + * 1  0  0  0        25    x 1       x120/2  x128/2  x96/2   x128    x96
> >> + * 1  0  0  1        25    x 1       x120/2  x128/2  x96/2   x84     x96
> >> + * 1  0  1  0        Prohibited setting
> >> + * 1  0  1  1        25    x 1       x120/2  x128/2  x96/2   x128    x96
> >> + * 1  1  0  0        33.33 / 2       x180/2  x192/2  x144/2  x192   
> >> x144
> >> + * 1  1  0  1        33.33 / 2       x180/2  x192/2  x144/2  x128   
> >> x144
> >> + * 1  1  1  0        Prohibited setting
> >> + * 1  1  1  1        33.33 / 2       x180/2  x192/2  x144/2  x192   
> >> x144
> >> + *
> >> + * *1 : datasheet indicates VCO output (PLLx = VCO/2)
> > 
> > As explained in a separate e-mail there's a few clocks on R8A7795 that
> > derive directly from PLL1 VCO. I thus wonder whether we shouldn't expose
> > the PLL1 clock as the VCO output and create VCO/2 using a fixed factor
> > clock in DT.
>
> Do you think that would reduce complexity or simplify the code? If so
> I think we should do it. Otherwise I think it makes sense to simply
> follow the data sheet.

I've commented on this in a reply to Geert's e-mail.

Geert Uytterhoeven Sept. 1, 2015, 2:23 p.m. UTC | #6

Hi Magnus,

On Mon, Aug 31, 2015 at 2:49 PM, Magnus Damm <[e-mail]> wrote:
> --- /dev/null
> +++ work/Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt       2015-08-31 15:49:10.000000000 +0900
> @@ -0,0 +1,32 @@

> +       cpg_clocks: cpg_clocks@e6150000 {
> +               compatible = "renesas,r8a7795-cpg-clocks",
> +                            "renesas,rcar-gen3-cpg-clocks";
> +               reg = <0 0xe6150000 0 0x1000>;
> +               clocks = <&extal_clk>;
> +               #clock-cells = <1>;
> +                clock-indices = <
> +                             R8A7795_CLK_MAIN R8A7795_CLK_PLL0
> +                             R8A7795_CLK_PLL1 R8A7795_CLK_PLL2
> +                             R8A7795_CLK_PLL3 R8A7795_CLK_PLL4
> +                >;

The new "clock-indices" hunk uses spaces insteads of TABs for indentation.

> +       };

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Magnus Damm Sept. 2, 2015, 2:27 a.m. UTC | #7

Hi Laurent,

On Tue, Sep 1, 2015 at 9:30 PM, Laurent Pinchart
<[e-mail]> wrote:
> Hi Magnus,
>
> On Tuesday 01 September 2015 19:59:01 Magnus Damm wrote:
>> On Tue, Sep 1, 2015 at 3:00 PM, Laurent Pinchart wrote:
>> > On Monday 31 August 2015 21:49:04 Magnus Damm wrote:
>> >> From: Gaku Inami <[e-mail]>
>> >>
>> >> This V5 patch adds initial CPG support for R-Car Generation 3 and in
>> >> particular the R8A7795 SoC.
>> >>
>> >> The R-Car Gen3 clock hardware has a register write protection feature
>> >> that needs to be shared between the CPG function needs to be shared
>> >> between the CPG and MSTP hardware somehow. So far this feature is simply
>> >> ignored.
>> >>
>> >> Signed-off-by: Gaku Inami <[e-mail]>
>> >> Signed-off-by: Kuninori Morimoto <[e-mail]>
>> >> Signed-off-by: Magnus Damm <[e-mail]>
>> >> ---
>> >>
>> >>  Changes since V4: (Magnus Damm <[e-mail]>)
>> >>  - Simplified clks array handling - thanks Geert!
>> >>  - Updated th DT binding documentation to reflect latest state
>> >>  - of_property_count_strings() -> of_property_count_u32_elems() fix
>> >>
>> >>  Changes since V3: (Magnus Damm <[e-mail]>)
>> >>  - Reworked driver to incorporate most feedback from Stephen Boyd -
>> >>  thanks!!
>> >>  - Major things like syscon and driver model require more discussion.
>> >>  - Added hunk to build drivers/clk/shmobile if ARCH_RENESAS is set.
>> >>
>> >>  Changes since V2: (Magnus Damm <[e-mail]>)
>> >>  - Reworked driver to rely on clock index instead of strings.
>> >>  - Dropped use of "clock-output-names".
>> >>
>> >>  Earlier versions: Kuninori Morimoto <[e-mail]>
>> >>  Initial version: Gaku Inami <[e-mail]>
>> >>
>> >>  Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt
>> >>  |   32 +
>> >>  drivers/clk/Makefile                                |    1
>> >>  drivers/clk/shmobile/Makefile                       |    1
>> >>  drivers/clk/shmobile/clk-rcar-gen3.c                |  245 ++++++++++
>> >>  4 files changed, 279 insertions(+)
>> >>
>> >> --- /dev/null
>> >> +++
>> >> work/Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks
>> >> .txt
>> >> 2015-08-31 15:49:10.000000000 +0900 @@ -0,0 +1,32 @@
>> >> +* Renesas R-Car Gen3 Clock Pulse Generator (CPG)
>> >> +
>> >> +The CPG generates core clocks for the R-Car Gen3 SoCs. It includes three
>> >> PLLs
>> >> +and several fixed ratio dividers.
>> >> +
>> >> +Required Properties:
>> >> +
>> >> +  - compatible: Must be one of
>> >> +    - "renesas,r8a7795-cpg-clocks" for the r8a7795 CPG
>> >> +    - "renesas,rcar-gen3-cpg-clocks" for the generic R-Car Gen3 CPG
>> >> +
>> >> +  - reg: Base address and length of the memory resource used by the CPG
>> >> +
>> >> +  - clocks: References to the parent clocks: first to the EXTAL clock
>> >> +  - #clock-cells: Must be 1
>> >> +  - clock-indices: Indices of the exported clocks
>> >
>> > Do we actually need the clock-indices property, as CPG clocks are numbered
>> > sequentially ? It seems to me like we could drop the property, especially
>> > given that the number of clocks is hardcoded in the driver anyway.
>>
>> There are two reasons why they are there:
>> 1) I like to be able to search DT files to see which node that is
>> providing what clock.
>
> The clocks are listed in the CPG section of include/dt-bindings/clock/r8a7795-
> clock.h so that should already give a hint. Additionally I don't think you'll
> search the CPG core clocks very often in the DT sources as they're usually not
> used directly by devices :-)

You are right that they may not be searched very frequently. But at
least the CPG is consistent with MSTP with this implementation. =)

>> 2) When adding support for additional SoCs we may add new index to the
>> driver. New SoC may get sparsely populated index lists and old ones
>> will omit the recently added index.
>
> If the CPG core differs between SoCs shouldn't that be handled by the DT
> compatibility string ?

You know that I personally always prefered an incremental approach
with little speculation in DT. =)

So using SoC specific compat strings over generic ones would in
general be my preferred choice. I suspect the initial developer of
this patch simply followed the same style as Gen2 with a generic
compat string.

Does this mean that you for the r8a7795 CPG prefer SoC specific compat
string to begin with over a more generic one? If we go for SoC
specific compat string then perhaps we also should change file names?

Thanks,

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Laurent Pinchart Sept. 2, 2015, 5:21 a.m. UTC | #8

Hi Magnus,

On Wednesday 02 September 2015 11:27:27 Magnus Damm wrote:
> On Tue, Sep 1, 2015 at 9:30 PM, Laurent Pinchart wrote:
> > On Tuesday 01 September 2015 19:59:01 Magnus Damm wrote:
> >> On Tue, Sep 1, 2015 at 3:00 PM, Laurent Pinchart wrote:
> >>> On Monday 31 August 2015 21:49:04 Magnus Damm wrote:
> >>>> From: Gaku Inami <[e-mail]>
> >>>> 
> >>>> This V5 patch adds initial CPG support for R-Car Generation 3 and in
> >>>> particular the R8A7795 SoC.
> >>>> 
> >>>> The R-Car Gen3 clock hardware has a register write protection feature
> >>>> that needs to be shared between the CPG function needs to be shared
> >>>> between the CPG and MSTP hardware somehow. So far this feature is
> >>>> simply ignored.
> >>>> 
> >>>> Signed-off-by: Gaku Inami <[e-mail]>
> >>>> Signed-off-by: Kuninori Morimoto <[e-mail]>
> >>>> Signed-off-by: Magnus Damm <[e-mail]>
> >>>> ---
> >>>> 
> >>>>  Changes since V4: (Magnus Damm <[e-mail]>)
> >>>>  - Simplified clks array handling - thanks Geert!
> >>>>  - Updated th DT binding documentation to reflect latest state
> >>>>  - of_property_count_strings() -> of_property_count_u32_elems() fix
> >>>>  
> >>>>  Changes since V3: (Magnus Damm <[e-mail]>)
> >>>>  - Reworked driver to incorporate most feedback from Stephen Boyd -
> >>>>  thanks!!
> >>>>  - Major things like syscon and driver model require more discussion.
> >>>>  - Added hunk to build drivers/clk/shmobile if ARCH_RENESAS is set.
> >>>>  
> >>>>  Changes since V2: (Magnus Damm <[e-mail]>)
> >>>>  - Reworked driver to rely on clock index instead of strings.
> >>>>  - Dropped use of "clock-output-names".
> >>>>  
> >>>>  Earlier versions: Kuninori Morimoto
> >>>>  <[e-mail]>
> >>>>  Initial version: Gaku Inami <[e-mail]>
> >>>>  
> >>>>  Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.
> >>>>  txt  |   32 +
> >>>>  drivers/clk/Makefile                                |    1
> >>>>  drivers/clk/shmobile/Makefile                       |    1
> >>>>  drivers/clk/shmobile/clk-rcar-gen3.c                |  245 ++++++++++
> >>>>  4 files changed, 279 insertions(+)
> >>>> 
> >>>> --- /dev/null
> >>>> +++
> >>>> work/Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clo
> >>>> cks.txt
> >>>> 2015-08-31 15:49:10.000000000 +0900 @@ -0,0 +1,32 @@
> >>>> +* Renesas R-Car Gen3 Clock Pulse Generator (CPG)
> >>>> +
> >>>> +The CPG generates core clocks for the R-Car Gen3 SoCs. It includes
> >>>> three PLLs
> >>>> +and several fixed ratio dividers.
> >>>> +
> >>>> +Required Properties:
> >>>> +
> >>>> +  - compatible: Must be one of
> >>>> +    - "renesas,r8a7795-cpg-clocks" for the r8a7795 CPG
> >>>> +    - "renesas,rcar-gen3-cpg-clocks" for the generic R-Car Gen3 CPG
> >>>> +
> >>>> +  - reg: Base address and length of the memory resource used by the
> >>>> CPG
> >>>> +
> >>>> +  - clocks: References to the parent clocks: first to the EXTAL clock
> >>>> +  - #clock-cells: Must be 1
> >>>> +  - clock-indices: Indices of the exported clocks
> >>> 
> >>> Do we actually need the clock-indices property, as CPG clocks are
> >>> numbered sequentially ? It seems to me like we could drop the property,
> >>> especially given that the number of clocks is hardcoded in the driver
> >>> anyway.
> >> 
> >> There are two reasons why they are there:
> >> 1) I like to be able to search DT files to see which node that is
> >> providing what clock.
> > 
> > The clocks are listed in the CPG section of
> > include/dt-bindings/clock/r8a7795- clock.h so that should already give a
> > hint. Additionally I don't think you'll search the CPG core clocks very
> > often in the DT sources as they're usually not used directly by devices
> > :-)
> 
> You are right that they may not be searched very frequently. But at
> least the CPG is consistent with MSTP with this implementation. =)
> 
> >> 2) When adding support for additional SoCs we may add new index to the
> >> driver. New SoC may get sparsely populated index lists and old ones
> >> will omit the recently added index.
> > 
> > If the CPG core differs between SoCs shouldn't that be handled by the DT
> > compatibility string ?
> 
> You know that I personally always prefered an incremental approach
> with little speculation in DT. =)
> 
> So using SoC specific compat strings over generic ones would in
> general be my preferred choice. I suspect the initial developer of
> this patch simply followed the same style as Gen2 with a generic
> compat string.
> 
> Does this mean that you for the r8a7795 CPG prefer SoC specific compat
> string to begin with over a more generic one? If we go for SoC
> specific compat string then perhaps we also should change file names?

We use an SoC-specific compat string already so we won't be stuck in the 
future. I would keep matching on the generic compatible string for now until 
the unlikely future event that a Gen3 SoC would have a different CPG. I don't 
think there's too much point in making DT bindings (and the corresponding 
code) more complex than necessary to plan for an unlikely future hardware 
change that we know nothing about at this point anyway.

Magnus Damm Sept. 2, 2015, 8:24 a.m. UTC | #9

Hi Laurent,

On Wed, Sep 2, 2015 at 2:21 PM, Laurent Pinchart
<[e-mail]> wrote:
> Hi Magnus,
>
> On Wednesday 02 September 2015 11:27:27 Magnus Damm wrote:
>> On Tue, Sep 1, 2015 at 9:30 PM, Laurent Pinchart wrote:
>> > On Tuesday 01 September 2015 19:59:01 Magnus Damm wrote:
>> >> On Tue, Sep 1, 2015 at 3:00 PM, Laurent Pinchart wrote:
>> >>> On Monday 31 August 2015 21:49:04 Magnus Damm wrote:
>> >>>> From: Gaku Inami <[e-mail]>
>> >>>>
>> >>>> This V5 patch adds initial CPG support for R-Car Generation 3 and in
>> >>>> particular the R8A7795 SoC.
>> >>>>
>> >>>> The R-Car Gen3 clock hardware has a register write protection feature
>> >>>> that needs to be shared between the CPG function needs to be shared
>> >>>> between the CPG and MSTP hardware somehow. So far this feature is
>> >>>> simply ignored.
>> >>>>
>> >>>> Signed-off-by: Gaku Inami <[e-mail]>
>> >>>> Signed-off-by: Kuninori Morimoto <[e-mail]>
>> >>>> Signed-off-by: Magnus Damm <[e-mail]>
>> >>>> ---
>> >>>>
>> >>>>  Changes since V4: (Magnus Damm <[e-mail]>)
>> >>>>  - Simplified clks array handling - thanks Geert!
>> >>>>  - Updated th DT binding documentation to reflect latest state
>> >>>>  - of_property_count_strings() -> of_property_count_u32_elems() fix
>> >>>>
>> >>>>  Changes since V3: (Magnus Damm <[e-mail]>)
>> >>>>  - Reworked driver to incorporate most feedback from Stephen Boyd -
>> >>>>  thanks!!
>> >>>>  - Major things like syscon and driver model require more discussion.
>> >>>>  - Added hunk to build drivers/clk/shmobile if ARCH_RENESAS is set.
>> >>>>
>> >>>>  Changes since V2: (Magnus Damm <[e-mail]>)
>> >>>>  - Reworked driver to rely on clock index instead of strings.
>> >>>>  - Dropped use of "clock-output-names".
>> >>>>
>> >>>>  Earlier versions: Kuninori Morimoto
>> >>>>  <[e-mail]>
>> >>>>  Initial version: Gaku Inami <[e-mail]>
>> >>>>
>> >>>>  Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.
>> >>>>  txt  |   32 +
>> >>>>  drivers/clk/Makefile                                |    1
>> >>>>  drivers/clk/shmobile/Makefile                       |    1
>> >>>>  drivers/clk/shmobile/clk-rcar-gen3.c                |  245 ++++++++++
>> >>>>  4 files changed, 279 insertions(+)
>> >>>>
>> >>>> --- /dev/null
>> >>>> +++
>> >>>> work/Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clo
>> >>>> cks.txt
>> >>>> 2015-08-31 15:49:10.000000000 +0900 @@ -0,0 +1,32 @@
>> >>>> +* Renesas R-Car Gen3 Clock Pulse Generator (CPG)
>> >>>> +
>> >>>> +The CPG generates core clocks for the R-Car Gen3 SoCs. It includes
>> >>>> three PLLs
>> >>>> +and several fixed ratio dividers.
>> >>>> +
>> >>>> +Required Properties:
>> >>>> +
>> >>>> +  - compatible: Must be one of
>> >>>> +    - "renesas,r8a7795-cpg-clocks" for the r8a7795 CPG
>> >>>> +    - "renesas,rcar-gen3-cpg-clocks" for the generic R-Car Gen3 CPG
>> >>>> +
>> >>>> +  - reg: Base address and length of the memory resource used by the
>> >>>> CPG
>> >>>> +
>> >>>> +  - clocks: References to the parent clocks: first to the EXTAL clock
>> >>>> +  - #clock-cells: Must be 1
>> >>>> +  - clock-indices: Indices of the exported clocks
>> >>>
>> >>> Do we actually need the clock-indices property, as CPG clocks are
>> >>> numbered sequentially ? It seems to me like we could drop the property,
>> >>> especially given that the number of clocks is hardcoded in the driver
>> >>> anyway.
>> >>
>> >> There are two reasons why they are there:
>> >> 1) I like to be able to search DT files to see which node that is
>> >> providing what clock.
>> >
>> > The clocks are listed in the CPG section of
>> > include/dt-bindings/clock/r8a7795- clock.h so that should already give a
>> > hint. Additionally I don't think you'll search the CPG core clocks very
>> > often in the DT sources as they're usually not used directly by devices
>> > :-)
>>
>> You are right that they may not be searched very frequently. But at
>> least the CPG is consistent with MSTP with this implementation. =)
>>
>> >> 2) When adding support for additional SoCs we may add new index to the
>> >> driver. New SoC may get sparsely populated index lists and old ones
>> >> will omit the recently added index.
>> >
>> > If the CPG core differs between SoCs shouldn't that be handled by the DT
>> > compatibility string ?
>>
>> You know that I personally always prefered an incremental approach
>> with little speculation in DT. =)
>>
>> So using SoC specific compat strings over generic ones would in
>> general be my preferred choice. I suspect the initial developer of
>> this patch simply followed the same style as Gen2 with a generic
>> compat string.
>>
>> Does this mean that you for the r8a7795 CPG prefer SoC specific compat
>> string to begin with over a more generic one? If we go for SoC
>> specific compat string then perhaps we also should change file names?
>
> We use an SoC-specific compat string already so we won't be stuck in the
> future. I would keep matching on the generic compatible string for now until
> the unlikely future event that a Gen3 SoC would have a different CPG. I don't
> think there's too much point in making DT bindings (and the corresponding
> code) more complex than necessary to plan for an unlikely future hardware
> change that we know nothing about at this point anyway.

Totally agree!

So we discussed this over chat a bit more and the following
observations/agreements were made:

- Magnus clarified that the clock-indices used in current Gen3 CPG
version were selected over clock-output-names based on feedback from
CCF maintainers that clock-output-names were best to be dropped. Both
Laurent and Magnus agreed that getting rid of the clock-output-names
in DT must be a good thing - especially if the CCF maintainers prefer
that.

- Removing clock-output-names from DT does however not work as-is,
some CCF core changes may be needed. We need to work together with the
CCF maintainers to resolve this.

- We did a quick study of CPG on Gen2 and noticed that r8a7790 and
r8a7791 core clocks are not identical.

- On Gen2 using a generic DT compat string with possibility to add
clocks seemed to work well when adding r8a7791 support as second SoC
after r8a7790.

- Using clock-indices on Gen3 to allow adding new clocks over time
with generic DT compat string match in the driver to begin with seems
like a good way forward.

- The current style of using SoC-specific and generic DT compat
strings in DT documentation and DTS together with generic DT compat
string in driver (with opt-in SoC specific matcing if needed) works
well for most on-chip devices/drivers and CPG is no exception.

- If a future SoC has some incompatibility when it comes to core
clocks then we can chose to deal with this either via a DT compat
string check or extending the number of clocks using a clock-index.
How to handle this will most likely be a case-by-case desicion.

- Sharing clock index #defines for DT ABI through a header file is
desired but it is OK to handle it as an incremental feature patch.

The above basically means that the current driver is in pretty OK
shape, but we still need to figure out how to adjust the CCF core code
to allow operation without clock-output-names in DT.

Hope this sums up the current situation!

Cheers,

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

--- /dev/null
+++ work/Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt	2015-08-31 15:49:10.000000000 +0900
@@ -0,0 +1,32 @@ 
+* Renesas R-Car Gen3 Clock Pulse Generator (CPG)
+
+The CPG generates core clocks for the R-Car Gen3 SoCs. It includes three PLLs
+and several fixed ratio dividers.
+
+Required Properties:
+
+  - compatible: Must be one of
+    - "renesas,r8a7795-cpg-clocks" for the r8a7795 CPG
+    - "renesas,rcar-gen3-cpg-clocks" for the generic R-Car Gen3 CPG
+
+  - reg: Base address and length of the memory resource used by the CPG
+
+  - clocks: References to the parent clocks: first to the EXTAL clock
+  - #clock-cells: Must be 1
+  - clock-indices: Indices of the exported clocks
+
+Example
+-------
+
+	cpg_clocks: cpg_clocks@e6150000 {
+		compatible = "renesas,r8a7795-cpg-clocks",
+			     "renesas,rcar-gen3-cpg-clocks";
+		reg = <0 0xe6150000 0 0x1000>;
+		clocks = <&extal_clk>;
+		#clock-cells = <1>;
+                clock-indices = <
+                             R8A7795_CLK_MAIN R8A7795_CLK_PLL0
+                             R8A7795_CLK_PLL1 R8A7795_CLK_PLL2
+                             R8A7795_CLK_PLL3 R8A7795_CLK_PLL4
+                >;
+	};
--- 0001/drivers/clk/Makefile
+++ work/drivers/clk/Makefile	2015-08-31 15:49:09.072366518 +0900
@@ -67,6 +67,7 @@  obj-$(CONFIG_COMMON_CLK_QCOM)		+= qcom/
 obj-$(CONFIG_ARCH_ROCKCHIP)		+= rockchip/
 obj-$(CONFIG_COMMON_CLK_SAMSUNG)	+= samsung/
 obj-$(CONFIG_ARCH_SHMOBILE_MULTI)	+= shmobile/
+obj-$(CONFIG_ARCH_RENESAS)		+= shmobile/
 obj-$(CONFIG_ARCH_SIRF)			+= sirf/
 obj-$(CONFIG_ARCH_SOCFPGA)		+= socfpga/
 obj-$(CONFIG_PLAT_SPEAR)		+= spear/
--- 0006/drivers/clk/shmobile/Makefile
+++ work/drivers/clk/shmobile/Makefile	2015-08-31 15:49:09.072366518 +0900
@@ -8,4 +8,5 @@  obj-$(CONFIG_ARCH_R8A7790)		+= clk-rcar-
 obj-$(CONFIG_ARCH_R8A7791)		+= clk-rcar-gen2.o clk-mstp.o clk-div6.o
 obj-$(CONFIG_ARCH_R8A7793)		+= clk-rcar-gen2.o clk-mstp.o clk-div6.o
 obj-$(CONFIG_ARCH_R8A7794)		+= clk-rcar-gen2.o clk-mstp.o clk-div6.o
+obj-$(CONFIG_ARCH_R8A7795)		+= clk-rcar-gen3.o clk-mstp.o clk-div6.o
 obj-$(CONFIG_ARCH_SH73A0)		+= clk-sh73a0.o clk-mstp.o clk-div6.o
--- /dev/null
+++ work/drivers/clk/shmobile/clk-rcar-gen3.c	2015-08-31 21:10:01.102366518 +0900
@@ -0,0 +1,245 @@ 
+/*
+ * rcar_gen3 Core CPG Clocks
+ *
+ * Copyright (C) 2015 Renesas Electronics Corp.
+ *
+ * Based on rcar_gen2 Core CPG Clocks driver.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/clk/shmobile.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+#define RCAR_GEN3_CLK_MAIN	0
+#define RCAR_GEN3_CLK_PLL0	1
+#define RCAR_GEN3_CLK_PLL1	2
+#define RCAR_GEN3_CLK_PLL2	3
+#define RCAR_GEN3_CLK_PLL3	4
+#define RCAR_GEN3_CLK_PLL4	5
+#define RCAR_GEN3_CLK_NR	6
+
+static const char * const rcar_gen3_clk_names[RCAR_GEN3_CLK_NR] = {
+	[RCAR_GEN3_CLK_MAIN] = "main",
+	[RCAR_GEN3_CLK_PLL0] = "pll0",
+	[RCAR_GEN3_CLK_PLL1] = "pll1",
+	[RCAR_GEN3_CLK_PLL2] = "pll2",
+	[RCAR_GEN3_CLK_PLL3] = "pll3",
+	[RCAR_GEN3_CLK_PLL4] = "pll4",
+};
+
+struct rcar_gen3_cpg {
+	struct clk_onecell_data data;
+	spinlock_t lock;
+	void __iomem *reg;
+	struct clk *clks[RCAR_GEN3_CLK_NR];
+};
+
+#define CPG_PLL0CR	0x00d8
+#define CPG_PLL2CR	0x002c
+
+/*
+ * common function
+ */
+#define rcar_clk_readl(cpg, _reg) readl(cpg->reg + _reg)
+
+/*
+ * Reset register definitions.
+ */
+#define MODEMR 0xe6160060
+
+static u32 rcar_gen3_read_mode_pins(void)
+{
+	static u32 mode;
+	static bool mode_valid;
+
+	if (!mode_valid) {
+		void __iomem *modemr = ioremap_nocache(MODEMR, 4);
+
+		BUG_ON(!modemr);
+		mode = ioread32(modemr);
+		iounmap(modemr);
+		mode_valid = true;
+	}
+
+	return mode;
+}
+
+/* -----------------------------------------------------------------------------
+ * CPG Clock Data
+ */
+
+/*
+ *   MD		EXTAL		PLL0	PLL1	PLL2	PLL3	PLL4
+ * 14 13 19 17	(MHz)		*1	*1	*1
+ *-------------------------------------------------------------------
+ * 0  0  0  0	16.66 x 1	x180/2	x192/2	x144/2	x192	x144
+ * 0  0  0  1	16.66 x 1	x180/2	x192/2	x144/2	x128	x144
+ * 0  0  1  0	Prohibited setting
+ * 0  0  1  1	16.66 x 1	x180/2	x192/2	x144/2	x192	x144
+ * 0  1  0  0	20    x 1	x150/2	x156/2	x120/2	x156	x120
+ * 0  1  0  1	20    x 1	x150/2	x156/2	x120/2	x106	x120
+ * 0  1  1  0	Prohibited setting
+ * 0  1  1  1	20    x 1	x150/2	x156/2	x120/2	x156	x120
+ * 1  0  0  0	25    x 1	x120/2	x128/2	x96/2	x128	x96
+ * 1  0  0  1	25    x 1	x120/2	x128/2	x96/2	x84	x96
+ * 1  0  1  0	Prohibited setting
+ * 1  0  1  1	25    x 1	x120/2	x128/2	x96/2	x128	x96
+ * 1  1  0  0	33.33 / 2	x180/2	x192/2	x144/2	x192	x144
+ * 1  1  0  1	33.33 / 2	x180/2	x192/2	x144/2	x128	x144
+ * 1  1  1  0	Prohibited setting
+ * 1  1  1  1	33.33 / 2	x180/2	x192/2	x144/2	x192	x144
+ *
+ * *1 : datasheet indicates VCO output (PLLx = VCO/2)
+ *
+ */
+#define CPG_PLL_CONFIG_INDEX(md)	((((md) & BIT(14)) >> 11) | \
+					 (((md) & BIT(13)) >> 11) | \
+					 (((md) & BIT(19)) >> 18) | \
+					 (((md) & BIT(17)) >> 17))
+struct cpg_pll_config {
+	unsigned int extal_div;
+	unsigned int pll1_mult;
+	unsigned int pll3_mult;
+	unsigned int pll4_mult;
+};
+
+static const struct cpg_pll_config cpg_pll_configs[16] __initconst = {
+/* EXTAL div	PLL1	PLL3	PLL4 */
+	{ 1,	192,	192,	144, },
+	{ 1,	192,	128,	144, },
+	{ 0,	0,	0,	0,   }, /* Prohibited setting */
+	{ 1,	192,	192,	144, },
+	{ 1,	156,	156,	120, },
+	{ 1,	156,	106,	120, },
+	{ 0,	0,	0,	0,   }, /* Prohibited setting */
+	{ 1,	156,	156,	120, },
+	{ 1,	128,	128,	96,  },
+	{ 1,	128,	84,	96,  },
+	{ 0,	0,	0,	0,   }, /* Prohibited setting */
+	{ 1,	128,	128,	96,  },
+	{ 2,	192,	192,	144, },
+	{ 2,	192,	128,	144, },
+	{ 0,	0,	0,	0,   }, /* Prohibited setting */
+	{ 2,	192,	192,	144, },
+};
+
+/* -----------------------------------------------------------------------------
+ * Initialization
+ */
+
+static struct clk * __init
+rcar_gen3_cpg_register_clock(struct device_node *np, struct rcar_gen3_cpg *cpg,
+			     const struct cpg_pll_config *config,
+			     unsigned int gen3_clk)
+{
+	const char *parent_name = rcar_gen3_clk_names[RCAR_GEN3_CLK_MAIN];
+	unsigned int mult = 1;
+	unsigned int div = 1;
+	u32 value;
+
+	switch (gen3_clk) {
+	case RCAR_GEN3_CLK_MAIN:
+		parent_name = of_clk_get_parent_name(np, 0);
+		div = config->extal_div;
+		break;
+	case RCAR_GEN3_CLK_PLL0:
+		/* PLL0 is a configurable multiplier clock. Register it as a
+		 * fixed factor clock for now as there's no generic multiplier
+		 * clock implementation and we currently have no need to change
+		 * the multiplier value.
+		 */
+		value = rcar_clk_readl(cpg, CPG_PLL0CR);
+		mult = ((value >> 24) & ((1 << 7) - 1)) + 1;
+		break;
+	case RCAR_GEN3_CLK_PLL1:
+		mult = config->pll1_mult / 2;
+		break;
+	case RCAR_GEN3_CLK_PLL2:
+		/* PLL2 is a configurable multiplier clock. Register it as a
+		 * fixed factor clock for now as there's no generic multiplier
+		 * clock implementation and we currently have no need to change
+		 * the multiplier value.
+		 */
+		value = rcar_clk_readl(cpg, CPG_PLL2CR);
+		mult = ((value >> 24) & ((1 << 7) - 1)) + 1;
+		break;
+	case RCAR_GEN3_CLK_PLL3:
+		mult = config->pll3_mult;
+		break;
+	case RCAR_GEN3_CLK_PLL4:
+		mult = config->pll4_mult;
+		break;
+	default:
+		return ERR_PTR(-EINVAL);
+	}
+
+	return clk_register_fixed_factor(NULL, rcar_gen3_clk_names[gen3_clk],
+					 parent_name, 0, mult, div);
+}
+
+static void __init rcar_gen3_cpg_clocks_init(struct device_node *np)
+{
+	const struct cpg_pll_config *config;
+	struct rcar_gen3_cpg *cpg;
+	u32 cpg_mode;
+	unsigned int i;
+	int num_clks;
+
+	cpg_mode = rcar_gen3_read_mode_pins();
+
+	num_clks = of_property_count_u32_elems(np, "clock-indices");
+	if (num_clks < 0) {
+		pr_err("%s: failed to count clocks\n", __func__);
+		return;
+	}
+
+	cpg = kzalloc(sizeof(*cpg), GFP_KERNEL);
+	if (!cpg)
+		return;
+
+	spin_lock_init(&cpg->lock);
+
+	cpg->data.clks = cpg->clks;
+	cpg->data.clk_num = num_clks;
+
+	cpg->reg = of_iomap(np, 0);
+	if (WARN_ON(cpg->reg == NULL))
+		return;
+
+	config = &cpg_pll_configs[CPG_PLL_CONFIG_INDEX(cpg_mode)];
+	if (!config->extal_div) {
+		pr_err("%s: Prohibited setting (cpg_mode=0x%x)\n",
+		       __func__, cpg_mode);
+		return;
+	}
+
+	for (i = 0; i < num_clks; ++i) {
+		struct clk *clk;
+		u32 idx;
+		int ret;
+
+		ret = of_property_read_u32_index(np, "clock-indices", i, &idx);
+		if (ret < 0)
+			break;
+
+		clk = rcar_gen3_cpg_register_clock(np, cpg, config, idx);
+		if (IS_ERR(clk))
+			pr_err("%s: failed to register %s %u clock (%ld)\n",
+			       __func__, np->name, idx, PTR_ERR(clk));
+		else
+			cpg->data.clks[idx] = clk;
+	}
+
+	of_clk_add_provider(np, of_clk_src_onecell_get, &cpg->data);
+}
+CLK_OF_DECLARE(rcar_gen3_cpg_clks, "renesas,rcar-gen3-cpg-clocks",
+	       rcar_gen3_cpg_clocks_init);