[3/3] clk: shmobile: Add R8A7790 clocks support

Commit Message

Laurent Pinchart Oct. 29, 2013, 2:55 p.m. UTC

The R8A7790 has several clocks that are too custom to be supported in a
generic driver. Those clocks can be divided in two categories:

- Fixed rate clocks with multiplier and divisor set according to boot
  mode configuration

- Custom divider clocks with SoC-specific divider values

This driver supports both.

Signed-off-by: Laurent Pinchart <[e-mail]>
---
 .../bindings/clock/renesas,r8a7790-cpg-clocks.txt  |  26 +++
 drivers/clk/shmobile/Makefile                      |   1 +
 drivers/clk/shmobile/clk-r8a7790.c                 | 176 +++++++++++++++++++++
 include/dt-bindings/clock/r8a7790-clock.h          |  10 ++
 include/linux/clk/shmobile.h                       |  19 +++
 5 files changed, 232 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt
 create mode 100644 drivers/clk/shmobile/clk-r8a7790.c
 create mode 100644 include/linux/clk/shmobile.h

Comments

Kumar Gala Oct. 29, 2013, 11:56 p.m. UTC | #1

On Oct 29, 2013, at 9:55 AM, Laurent Pinchart wrote:

> The R8A7790 has several clocks that are too custom to be supported in a
> generic driver. Those clocks can be divided in two categories:
> 
> - Fixed rate clocks with multiplier and divisor set according to boot
>  mode configuration
> 
> - Custom divider clocks with SoC-specific divider values
> 
> This driver supports both.
> 
> Signed-off-by: Laurent Pinchart <[e-mail]>
> ---
> .../bindings/clock/renesas,r8a7790-cpg-clocks.txt  |  26 +++
> drivers/clk/shmobile/Makefile                      |   1 +
> drivers/clk/shmobile/clk-r8a7790.c                 | 176 +++++++++++++++++++++
> include/dt-bindings/clock/r8a7790-clock.h          |  10 ++
> include/linux/clk/shmobile.h                       |  19 +++
> 5 files changed, 232 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt
> create mode 100644 drivers/clk/shmobile/clk-r8a7790.c
> create mode 100644 include/linux/clk/shmobile.h
> 
> diff --git a/Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt b/Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt
> new file mode 100644
> index 0000000..d889917
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt
> @@ -0,0 +1,26 @@
> +* Renesas R8A7790 Clock Pulse Generator (CPG)
> +
> +The CPG generates core clocks for the R8A7790. It includes three PLLs and
> +several fixed ratio dividers.
> +
> +Required Properties:
> +
> +  - compatible: Must be "renesas,r8a7790-cpg-clocks"
> +  - reg: Base address and length of the memory resource used by the CPG
> +  - clocks: Reference to the parent clock
> +  - #clock-cells: Must be 1
> +  - clock-output-names: The name of the clocks, must be "main", "pll1",
> +    "pll3", "lb", "qspi", "sdh", "sd0", "sd1".
> +
> +
> +Example
> +-------
> +
> +	cpg_clocks: cpg_clocks {

cpg_clocks@e6150000

> +		compatible = "renesas,r8a7790-cpg-clocks";
> +		reg = <0 0xe6150000 0 0x1000>;
> +		clocks = <&extal_clk>;
> +		#clock-cells = <1>;
> +		clock-output-names = "main", "pll1", "pll3", "lb",
> +				     "qspi", "sdh", "sd0", "sd1";
> +	};

Other than minor nit, ack.

For the DT-Binding portion:

Acked-by: Kumar Gala <[e-mail]>

- k

Magnus Damm Nov. 5, 2013, 7:56 a.m. UTC | #2

On Tue, Oct 29, 2013 at 11:55 PM, Laurent Pinchart
<[e-mail]> wrote:
> The R8A7790 has several clocks that are too custom to be supported in a
> generic driver. Those clocks can be divided in two categories:
>
> - Fixed rate clocks with multiplier and divisor set according to boot
>   mode configuration
>
> - Custom divider clocks with SoC-specific divider values
>
> This driver supports both.
>
> Signed-off-by: Laurent Pinchart <[e-mail]>
> ---
>  .../bindings/clock/renesas,r8a7790-cpg-clocks.txt  |  26 +++
>  drivers/clk/shmobile/Makefile                      |   1 +
>  drivers/clk/shmobile/clk-r8a7790.c                 | 176 +++++++++++++++++++++
>  include/dt-bindings/clock/r8a7790-clock.h          |  10 ++
>  include/linux/clk/shmobile.h                       |  19 +++
>  5 files changed, 232 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt
>  create mode 100644 drivers/clk/shmobile/clk-r8a7790.c
>  create mode 100644 include/linux/clk/shmobile.h
>
> diff --git a/Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt b/Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt
> new file mode 100644
> index 0000000..d889917
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt
> @@ -0,0 +1,26 @@
> +* Renesas R8A7790 Clock Pulse Generator (CPG)
> +
> +The CPG generates core clocks for the R8A7790. It includes three PLLs and
> +several fixed ratio dividers.
> +
> +Required Properties:
> +
> +  - compatible: Must be "renesas,r8a7790-cpg-clocks"
> +  - reg: Base address and length of the memory resource used by the CPG
> +  - clocks: Reference to the parent clock
> +  - #clock-cells: Must be 1
> +  - clock-output-names: The name of the clocks, must be "main", "pll1",
> +    "pll3", "lb", "qspi", "sdh", "sd0", "sd1".
> +
> +
> +Example
> +-------
> +
> +       cpg_clocks: cpg_clocks {
> +               compatible = "renesas,r8a7790-cpg-clocks";
> +               reg = <0 0xe6150000 0 0x1000>;
> +               clocks = <&extal_clk>;
> +               #clock-cells = <1>;
> +               clock-output-names = "main", "pll1", "pll3", "lb",
> +                                    "qspi", "sdh", "sd0", "sd1";
> +       };
> diff --git a/drivers/clk/shmobile/Makefile b/drivers/clk/shmobile/Makefile
> index 3275c78..949f29e 100644
> --- a/drivers/clk/shmobile/Makefile
> +++ b/drivers/clk/shmobile/Makefile
> @@ -1,4 +1,5 @@
>  obj-$(CONFIG_ARCH_EMEV2)               += clk-emev2.o
> +obj-$(CONFIG_ARCH_R8A7790)             += clk-r8a7790.o
>  obj-$(CONFIG_ARCH_SHMOBILE_MULTI)      += clk-div6.o
>  obj-$(CONFIG_ARCH_SHMOBILE_MULTI)      += clk-mstp.o
>
> diff --git a/drivers/clk/shmobile/clk-r8a7790.c b/drivers/clk/shmobile/clk-r8a7790.c
> new file mode 100644
> index 0000000..2e76638
> --- /dev/null
> +++ b/drivers/clk/shmobile/clk-r8a7790.c
> @@ -0,0 +1,176 @@
> +/*
> + * r8a7790 Core CPG Clocks
> + *
> + * Copyright (C) 2013  Ideas On Board SPRL
> + *
> + * Contact: Laurent Pinchart <[e-mail]>
> + *
> + * 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/clkdev.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/spinlock.h>
> +
> +#include <dt-bindings/clock/r8a7790-clock.h>
> +
> +#define CPG_NUM_CLOCKS                 (R8A7790_CLK_SD1 + 1)
> +
> +struct r8a7790_cpg {
> +       struct clk_onecell_data data;
> +       spinlock_t lock;
> +       void __iomem *reg;
> +};
> +
> +/*
> + *   MD                EXTAL           PLL0    PLL1    PLL3
> + * 14 13 19    (MHz)           *1      *1
> + *---------------------------------------------------
> + * 0  0  0     15 x 1          x172/2  x208/2  x106
> + * 0  0  1     15 x 1          x172/2  x208/2  x88
> + * 0  1  0     20 x 1          x130/2  x156/2  x80
> + * 0  1  1     20 x 1          x130/2  x156/2  x66
> + * 1  0  0     26 / 2          x200/2  x240/2  x122
> + * 1  0  1     26 / 2          x200/2  x240/2  x102
> + * 1  1  0     30 / 2          x172/2  x208/2  x106
> + * 1  1  1     30 / 2          x172/2  x208/2  x88
> + *
> + * *1 :        Table 7.6 indicates VCO ouput (PLLx = VCO/2)
> + */
> +#define CPG_PLL_CONFIG_INDEX(md)       ((((md) & BIT(14)) >> 12) | \
> +                                        (((md) & BIT(13)) >> 12) | \
> +                                        (((md) & BIT(19)) >> 19))
> +struct cpg_pll_config {
> +       unsigned int extal_div;
> +       unsigned int pll1_mult;
> +       unsigned int pll3_mult;
> +};
> +
> +static const struct cpg_pll_config cpg_pll_configs[8] = {
> +       { 1, 208, 106 }, { 1, 208,  88 }, { 1, 156,  80 }, { 1, 156,  66 },
> +       { 2, 240, 122 }, { 2, 240, 102 }, { 2, 208, 106 }, { 2, 208,  88 },
> +};
> +
> +/* SDHI divisors */
> +static const struct clk_div_table cpg_sdh_div_table[] = {
> +       {  0,  2 }, {  1,  3 }, {  2,  4 }, {  3,  6 },
> +       {  4,  8 }, {  5, 12 }, {  6, 16 }, {  7, 18 },
> +       {  8, 24 }, { 10, 36 }, { 11, 48 }, {  0,  0 },
> +};
> +
> +static const struct clk_div_table cpg_sd01_div_table[] = {
> +       {  5, 12 }, {  6, 16 }, {  7, 18 }, {  8, 24 },
> +       { 10, 36 }, { 11, 48 }, { 12, 10 }, {  0,  0 },
> +};
> +
> +static u32 cpg_mode __initdata;
> +
> +#define CPG_SDCKCR                     0x00000074
> +
> +static void __init r8a7790_cpg_clocks_init(struct device_node *np)
> +{
> +       const struct cpg_pll_config *config;
> +       struct r8a7790_cpg *cpg;
> +       struct clk **clks;
> +       unsigned int i;
> +
> +       cpg = kzalloc(sizeof(*cpg), GFP_KERNEL);
> +       clks = kzalloc(CPG_NUM_CLOCKS * sizeof(*clks), GFP_KERNEL);
> +       if (cpg == NULL || clks == NULL) {
> +               kfree(clks);
> +               kfree(cpg);
> +               pr_err("%s: failed to allocate cpg\n", __func__);
> +               return;
> +       }
> +
> +       spin_lock_init(&cpg->lock);
> +
> +       cpg->data.clks = clks;
> +       cpg->data.clk_num = CPG_NUM_CLOCKS;
> +
> +       cpg->reg = of_iomap(np, 0);
> +       if (WARN_ON(cpg->reg == NULL)) {
> +               kfree(cpg->data.clks);
> +               kfree(cpg);
> +               return;
> +       }
> +
> +       config = &cpg_pll_configs[CPG_PLL_CONFIG_INDEX(cpg_mode)];
> +
> +       for (i = 0; i < CPG_NUM_CLOCKS; ++i) {
> +               const struct clk_div_table *table = NULL;
> +               const char *parent_name = "main";
> +               const char *name;
> +               unsigned int shift;
> +               unsigned int mult = 1;
> +               unsigned int div = 1;
> +               struct clk *clk;
> +
> +               of_property_read_string_index(np, "clock-output-names", i,
> +                                             &name);
> +
> +               switch (i) {
> +               case R8A7790_CLK_MAIN:
> +                       parent_name = of_clk_get_parent_name(np, 0);
> +                       div = config->extal_div;
> +                       break;
> +               case R8A7790_CLK_PLL1:
> +                       mult = config->pll1_mult / 2;
> +                       break;
> +               case R8A7790_CLK_PLL3:
> +                       mult = config->pll3_mult;
> +                       break;
> +               case R8A7790_CLK_LB:
> +                       div = cpg_mode & BIT(18) ? 36 : 24;
> +                       break;
> +               case R8A7790_CLK_QSPI:
> +                       div = (cpg_mode & (BIT(3) | BIT(2) | BIT(1))) == BIT(2)
> +                           ? 16 : 20;
> +                       break;
> +               case R8A7790_CLK_SDH:
> +                       table = cpg_sdh_div_table;
> +                       shift = 8;
> +                       break;
> +               case R8A7790_CLK_SD0:
> +                       table = cpg_sd01_div_table;
> +                       shift = 4;
> +                       break;
> +               case R8A7790_CLK_SD1:
> +                       table = cpg_sd01_div_table;
> +                       shift = 0;
> +                       break;
> +               }
> +
> +               if (!table)
> +                       clk = clk_register_fixed_factor(NULL, name, parent_name,
> +                                                       0, mult, div);
> +               else
> +                       clk = clk_register_divider_table(NULL, name,
> +                                                        parent_name, 0,
> +                                                        cpg->reg + CPG_SDCKCR,
> +                                                        shift, 4, 0, table,
> +                                                        &cpg->lock);
> +
> +               if (IS_ERR(clk))
> +                       pr_err("%s: failed to register %s %s clock (%ld)\n",
> +                              __func__, np->name, name, PTR_ERR(clk));
> +       }
> +
> +       of_clk_add_provider(np, of_clk_src_onecell_get, &cpg->data);
> +}
> +CLK_OF_DECLARE(r8a7790_cpg_clks, "renesas,r8a7790-cpg-clocks",
> +              r8a7790_cpg_clocks_init);
> +
> +void __init r8a7790_clocks_init(u32 mode)
> +{
> +       cpg_mode = mode;
> +
> +       of_clk_init(NULL);
> +}

Hi Laurent,

Thanks a lot for your efforts on this. In general I think it all looks
good, I have however one question regarding the "probe" interface
between the SoC code in arch/arm/mach-shmobile and drivers/clk. The
code above in r8a7790_clocks_init() looks a bit spaghetti-style to me,
perhaps it is possible to make it cleaner somehow?

I realize that you need some way to read out the mode pin setting, and
that is currently provided by the SoC code in arch/arm/mach-shmobile/.
Today it seems that you instead of letting the drivers/clk/ code call
SoC code to get the mode pin setting (and add a SoC link dependency)
you simply feed the settings to the clk code from the SoC code using
the parameter to r8a7790_clocks_init(). This direction seems good to
me. I'm however not too keen on the current symbol dependency in the
"other" direction.

If possible I'd like to keep the interface between the SoC code and
the driver code to "standard" driver model functions. For instance,
using of_clk_init(NULL) from the SoC code seems like a pretty good
standard interface - without any link time dependencies. So with the
current code, the r8a7790_clocks_init() function somehow goes against
this no-symbol-dependency preference that I happen to have. =)

Would it for instance be possible let the SoC code read out the mode
pin setting, and then pass the current setting using the argument of
of_clk_init() to select different dividers? That way the symbol
dependency goes away. Or maybe it becomes too verbose?

Let me know what you think. I would like to follow the same style for r8a7791.

Cheers,

/ magnus

Kuninori Morimoto Nov. 5, 2013, 8:52 a.m. UTC | #3

Hi Laurent

> +Required Properties:
> +
> +  - compatible: Must be "renesas,r8a7790-cpg-clocks"
> +  - reg: Base address and length of the memory resource used by the CPG
> +  - clocks: Reference to the parent clock
> +  - #clock-cells: Must be 1
> +  - clock-output-names: The name of the clocks, must be "main", "pll1",
> +    "pll3", "lb", "qspi", "sdh", "sd0", "sd1".
(snip)
> +	for (i = 0; i < CPG_NUM_CLOCKS; ++i) {
> +		const struct clk_div_table *table = NULL;
> +		const char *parent_name = "main";
> +		const char *name;
> +		unsigned int shift;
> +		unsigned int mult = 1;
> +		unsigned int div = 1;
> +		struct clk *clk;
> +
> +		of_property_read_string_index(np, "clock-output-names", i,
> +					      &name);
> +
> +		switch (i) {
> +		case R8A7790_CLK_MAIN:
> +			parent_name = of_clk_get_parent_name(np, 0);
> +			div = config->extal_div;
> +			break;
> +		case R8A7790_CLK_PLL1:
> +			mult = config->pll1_mult / 2;
> +			break;
> +		case R8A7790_CLK_PLL3:
> +			mult = config->pll3_mult;
> +			break;
> +		case R8A7790_CLK_LB:
> +			div = cpg_mode & BIT(18) ? 36 : 24;
> +			break;
> +		case R8A7790_CLK_QSPI:
> +			div = (cpg_mode & (BIT(3) | BIT(2) | BIT(1))) == BIT(2)
> +			    ? 16 : 20;
> +			break;
> +		case R8A7790_CLK_SDH:
> +			table = cpg_sdh_div_table;
> +			shift = 8;
> +			break;
> +		case R8A7790_CLK_SD0:
> +			table = cpg_sd01_div_table;
> +			shift = 4;
> +			break;
> +		case R8A7790_CLK_SD1:
> +			table = cpg_sd01_div_table;
> +			shift = 0;
> +			break;
> +		}

Is this clock-output-names realy "Required" property ?
The "name" and "order" seem fixed, then,
I guess it can simply use "name array" ?

> +static void __init r8a7790_cpg_clocks_init(struct device_node *np)
> +{
> +	const struct cpg_pll_config *config;
> +	struct r8a7790_cpg *cpg;
> +	struct clk **clks;
> +	unsigned int i;
> +
> +	cpg = kzalloc(sizeof(*cpg), GFP_KERNEL);
> +	clks = kzalloc(CPG_NUM_CLOCKS * sizeof(*clks), GFP_KERNEL);
> +	if (cpg == NULL || clks == NULL) {
> +		kfree(clks);
> +		kfree(cpg);
> +		pr_err("%s: failed to allocate cpg\n", __func__);
> +		return;
> +	}
> +
> +	spin_lock_init(&cpg->lock);
> +
> +	cpg->data.clks = clks;
> +	cpg->data.clk_num = CPG_NUM_CLOCKS;
> +
> +	cpg->reg = of_iomap(np, 0);
> +	if (WARN_ON(cpg->reg == NULL)) {
> +		kfree(cpg->data.clks);
> +		kfree(cpg);
> +		return;
> +	}

Above 2 error cases are doing same thing.
If can use goto xxx_error.

Best regards
---
Kuninori Morimoto

Laurent Pinchart Nov. 5, 2013, 11:47 p.m. UTC | #4

Hi Magnus,

(And a question for Mike below)

On Tuesday 05 November 2013 16:56:40 Magnus Damm wrote:
> On Tue, Oct 29, 2013 at 11:55 PM, Laurent Pinchart wrote:
> > The R8A7790 has several clocks that are too custom to be supported in a
> > generic driver. Those clocks can be divided in two categories:
> > 
> > - Fixed rate clocks with multiplier and divisor set according to boot
> >   mode configuration
> > 
> > - Custom divider clocks with SoC-specific divider values
> > 
> > This driver supports both.
> > 
> > Signed-off-by: Laurent Pinchart
> > <[e-mail]>
> > ---
> > 
> >  .../bindings/clock/renesas,r8a7790-cpg-clocks.txt  |  26 +++
> >  drivers/clk/shmobile/Makefile                      |   1 +
> >  drivers/clk/shmobile/clk-r8a7790.c                 | 176 ++++++++++++++++
> >  include/dt-bindings/clock/r8a7790-clock.h          |  10 ++
> >  include/linux/clk/shmobile.h                       |  19 +++
> >  5 files changed, 232 insertions(+)
> >  create mode 100644
> >  Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt
> >  create mode 100644 drivers/clk/shmobile/clk-r8a7790.c
> >  create mode 100644 include/linux/clk/shmobile.h
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt
> > b/Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt
> > new file mode 100644
> > index 0000000..d889917
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt
> > @@ -0,0 +1,26 @@
> > +* Renesas R8A7790 Clock Pulse Generator (CPG)
> > +
> > +The CPG generates core clocks for the R8A7790. It includes three PLLs and
> > +several fixed ratio dividers.
> > +
> > +Required Properties:
> > +
> > +  - compatible: Must be "renesas,r8a7790-cpg-clocks"
> > +  - reg: Base address and length of the memory resource used by the CPG
> > +  - clocks: Reference to the parent clock
> > +  - #clock-cells: Must be 1
> > +  - clock-output-names: The name of the clocks, must be "main", "pll1",
> > +    "pll3", "lb", "qspi", "sdh", "sd0", "sd1".
> > +
> > +
> > +Example
> > +-------
> > +
> > +       cpg_clocks: cpg_clocks {
> > +               compatible = "renesas,r8a7790-cpg-clocks";
> > +               reg = <0 0xe6150000 0 0x1000>;
> > +               clocks = <&extal_clk>;
> > +               #clock-cells = <1>;
> > +               clock-output-names = "main", "pll1", "pll3", "lb",
> > +                                    "qspi", "sdh", "sd0", "sd1";
> > +       };
> > diff --git a/drivers/clk/shmobile/Makefile b/drivers/clk/shmobile/Makefile
> > index 3275c78..949f29e 100644
> > --- a/drivers/clk/shmobile/Makefile
> > +++ b/drivers/clk/shmobile/Makefile
> > @@ -1,4 +1,5 @@
> >  obj-$(CONFIG_ARCH_EMEV2)               += clk-emev2.o
> > +obj-$(CONFIG_ARCH_R8A7790)             += clk-r8a7790.o
> >  obj-$(CONFIG_ARCH_SHMOBILE_MULTI)      += clk-div6.o
> >  obj-$(CONFIG_ARCH_SHMOBILE_MULTI)      += clk-mstp.o
> > diff --git a/drivers/clk/shmobile/clk-r8a7790.c
> > b/drivers/clk/shmobile/clk-r8a7790.c new file mode 100644
> > index 0000000..2e76638
> > --- /dev/null
> > +++ b/drivers/clk/shmobile/clk-r8a7790.c
> > @@ -0,0 +1,176 @@
> > +/*
> > + * r8a7790 Core CPG Clocks
> > + *
> > + * Copyright (C) 2013  Ideas On Board SPRL
> > + *
> > + * Contact: Laurent Pinchart <[e-mail]>
> > + *
> > + * 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/clkdev.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/spinlock.h>
> > +
> > +#include <dt-bindings/clock/r8a7790-clock.h>
> > +
> > +#define CPG_NUM_CLOCKS                 (R8A7790_CLK_SD1 + 1)
> > +
> > +struct r8a7790_cpg {
> > +       struct clk_onecell_data data;
> > +       spinlock_t lock;
> > +       void __iomem *reg;
> > +};
> > +
> > +/*
> > + *   MD                EXTAL           PLL0    PLL1    PLL3
> > + * 14 13 19    (MHz)           *1      *1
> > + *---------------------------------------------------
> > + * 0  0  0     15 x 1          x172/2  x208/2  x106
> > + * 0  0  1     15 x 1          x172/2  x208/2  x88
> > + * 0  1  0     20 x 1          x130/2  x156/2  x80
> > + * 0  1  1     20 x 1          x130/2  x156/2  x66
> > + * 1  0  0     26 / 2          x200/2  x240/2  x122
> > + * 1  0  1     26 / 2          x200/2  x240/2  x102
> > + * 1  1  0     30 / 2          x172/2  x208/2  x106
> > + * 1  1  1     30 / 2          x172/2  x208/2  x88
> > + *
> > + * *1 :        Table 7.6 indicates VCO ouput (PLLx = VCO/2)
> > + */
> > +#define CPG_PLL_CONFIG_INDEX(md)       ((((md) & BIT(14)) >> 12) | \
> > +                                        (((md) & BIT(13)) >> 12) | \
> > +                                        (((md) & BIT(19)) >> 19))
> > +struct cpg_pll_config {
> > +       unsigned int extal_div;
> > +       unsigned int pll1_mult;
> > +       unsigned int pll3_mult;
> > +};
> > +
> > +static const struct cpg_pll_config cpg_pll_configs[8] = {
> > +       { 1, 208, 106 }, { 1, 208,  88 }, { 1, 156,  80 }, { 1, 156,  66
> > },
> > +       { 2, 240, 122 }, { 2, 240, 102 }, { 2, 208, 106 }, { 2, 208,  88
> > },
> > +};
> > +
> > +/* SDHI divisors */
> > +static const struct clk_div_table cpg_sdh_div_table[] = {
> > +       {  0,  2 }, {  1,  3 }, {  2,  4 }, {  3,  6 },
> > +       {  4,  8 }, {  5, 12 }, {  6, 16 }, {  7, 18 },
> > +       {  8, 24 }, { 10, 36 }, { 11, 48 }, {  0,  0 },
> > +};
> > +
> > +static const struct clk_div_table cpg_sd01_div_table[] = {
> > +       {  5, 12 }, {  6, 16 }, {  7, 18 }, {  8, 24 },
> > +       { 10, 36 }, { 11, 48 }, { 12, 10 }, {  0,  0 },
> > +};
> > +
> > +static u32 cpg_mode __initdata;
> > +
> > +#define CPG_SDCKCR                     0x00000074
> > +
> > +static void __init r8a7790_cpg_clocks_init(struct device_node *np)
> > +{
> > +       const struct cpg_pll_config *config;
> > +       struct r8a7790_cpg *cpg;
> > +       struct clk **clks;
> > +       unsigned int i;
> > +
> > +       cpg = kzalloc(sizeof(*cpg), GFP_KERNEL);
> > +       clks = kzalloc(CPG_NUM_CLOCKS * sizeof(*clks), GFP_KERNEL);
> > +       if (cpg == NULL || clks == NULL) {
> > +               kfree(clks);
> > +               kfree(cpg);
> > +               pr_err("%s: failed to allocate cpg\n", __func__);
> > +               return;
> > +       }
> > +
> > +       spin_lock_init(&cpg->lock);
> > +
> > +       cpg->data.clks = clks;
> > +       cpg->data.clk_num = CPG_NUM_CLOCKS;
> > +
> > +       cpg->reg = of_iomap(np, 0);
> > +       if (WARN_ON(cpg->reg == NULL)) {
> > +               kfree(cpg->data.clks);
> > +               kfree(cpg);
> > +               return;
> > +       }
> > +
> > +       config = &cpg_pll_configs[CPG_PLL_CONFIG_INDEX(cpg_mode)];
> > +
> > +       for (i = 0; i < CPG_NUM_CLOCKS; ++i) {
> > +               const struct clk_div_table *table = NULL;
> > +               const char *parent_name = "main";
> > +               const char *name;
> > +               unsigned int shift;
> > +               unsigned int mult = 1;
> > +               unsigned int div = 1;
> > +               struct clk *clk;
> > +
> > +               of_property_read_string_index(np, "clock-output-names", i,
> > +                                             &name);
> > +
> > +               switch (i) {
> > +               case R8A7790_CLK_MAIN:
> > +                       parent_name = of_clk_get_parent_name(np, 0);
> > +                       div = config->extal_div;
> > +                       break;
> > +               case R8A7790_CLK_PLL1:
> > +                       mult = config->pll1_mult / 2;
> > +                       break;
> > +               case R8A7790_CLK_PLL3:
> > +                       mult = config->pll3_mult;
> > +                       break;
> > +               case R8A7790_CLK_LB:
> > +                       div = cpg_mode & BIT(18) ? 36 : 24;
> > +                       break;
> > +               case R8A7790_CLK_QSPI:
> > +                       div = (cpg_mode & (BIT(3) | BIT(2) | BIT(1))) ==
> > BIT(2) +                           ? 16 : 20;
> > +                       break;
> > +               case R8A7790_CLK_SDH:
> > +                       table = cpg_sdh_div_table;
> > +                       shift = 8;
> > +                       break;
> > +               case R8A7790_CLK_SD0:
> > +                       table = cpg_sd01_div_table;
> > +                       shift = 4;
> > +                       break;
> > +               case R8A7790_CLK_SD1:
> > +                       table = cpg_sd01_div_table;
> > +                       shift = 0;
> > +                       break;
> > +               }
> > +
> > +               if (!table)
> > +                       clk = clk_register_fixed_factor(NULL, name,
> > parent_name,
> > +                                                       0, mult, div);
> > +               else
> > +                       clk = clk_register_divider_table(NULL, name,
> > +                                                        parent_name, 0,
> > +                                                        cpg->reg +
> > CPG_SDCKCR,
> > +                                                        shift, 4, 0,
> > table,
> > +                                                        &cpg->lock);
> > +
> > +               if (IS_ERR(clk))
> > +                       pr_err("%s: failed to register %s %s clock
> > (%ld)\n",
> > +                              __func__, np->name, name, PTR_ERR(clk));
> > +       }
> > +
> > +       of_clk_add_provider(np, of_clk_src_onecell_get, &cpg->data);
> > +}
> > +CLK_OF_DECLARE(r8a7790_cpg_clks, "renesas,r8a7790-cpg-clocks",
> > +              r8a7790_cpg_clocks_init);
> > +
> > +void __init r8a7790_clocks_init(u32 mode)
> > +{
> > +       cpg_mode = mode;
> > +
> > +       of_clk_init(NULL);
> > +}
> 
> Hi Laurent,
> 
> Thanks a lot for your efforts on this. In general I think it all looks good,

Thank you.

> I have however one question regarding the "probe" interface between the SoC
> code in arch/arm/mach-shmobile and drivers/clk. The code above in
> r8a7790_clocks_init() looks a bit spaghetti-style to me, perhaps it is
> possible to make it cleaner somehow?

Isn't it just a loop ? :-) The clocks handled by this driver are "special", 
they need to be initialized manually.

> I realize that you need some way to read out the mode pin setting, and that
> is currently provided by the SoC code in arch/arm/mach-shmobile/. Today it
> seems that you instead of letting the drivers/clk/ code call SoC code to get
> the mode pin setting (and add a SoC link dependency) you simply feed the
> settings to the clk code from the SoC code using the parameter to
> r8a7790_clocks_init(). This direction seems good to me. I'm however not too
> keen on the current symbol dependency in the "other" direction.
> 
> If possible I'd like to keep the interface between the SoC code and the
> driver code to "standard" driver model functions. For instance, using
> of_clk_init(NULL) from the SoC code seems like a pretty good standard
> interface - without any link time dependencies. So with the current code,
> the r8a7790_clocks_init() function somehow goes against this no-symbol-
> dependency preference that I happen to have. =)
> 
> Would it for instance be possible let the SoC code read out the mode pin
> setting, and then pass the current setting using the argument of
> of_clk_init() to select different dividers? That way the symbol dependency
> goes away. Or maybe it becomes too verbose?

The of_clk_init() argument is an array of struct of_device_id that is used by 
the clock core to match device tree nodes. I don't really see how you would 
like to use it to pass the boot mode pins state to the driver.

There is currently no dedicated API (at least to my knowledge) to pass clock 
configuration information between arch/arm and drivers/clk. Here's a couple of 
methods that can be used.

- Call a drivers/clk function from platform code with the clock configuration 
information and store that information in a driver global variable for later 
use (this is the method currently used by this patch).

- Call a platform code function from driver code to read the configuration. 
This is pretty similar to the above, with a dependency in the other direction.

- Read the value directly from the hardware in the clock driver. This doesn't 
feel really right, as that's not the job of the clock driver. In a more 
general case this solution might not always be possible, as reading the 
configuration might require access to resources not available to drivers.

- Create a standard API for this purpose. It might be overengineering.

- Use AUXDATA filled by platform code.

There might be other solutions I haven't thought of. I went for the first 
method as there's already 8 other clock drivers that expose an initialization 
function to platform code. I might just have copied a mistake though.

Mike, do you have an opinion on this ? In a nutshell, the code clocks are 
fixed factor but their factor depend on a boot mode configuration. The 
configuration value is thus needed when instantiating the clocks. It can be 
read from a hardware register, outside of the clocks IP core.

> Let me know what you think. I would like to follow the same style for
> r8a7791.

Laurent Pinchart Nov. 5, 2013, 11:57 p.m. UTC | #5

Hi Morimoto-san,

On Tuesday 05 November 2013 00:52:29 Kuninori Morimoto wrote:
> Hi Laurent
> 
> > +Required Properties:
> > +
> > +  - compatible: Must be "renesas,r8a7790-cpg-clocks"
> > +  - reg: Base address and length of the memory resource used by the CPG
> > +  - clocks: Reference to the parent clock
> > +  - #clock-cells: Must be 1
> > +  - clock-output-names: The name of the clocks, must be "main", "pll1",
> > +    "pll3", "lb", "qspi", "sdh", "sd0", "sd1".
> 
> (snip)
> 
> > +	for (i = 0; i < CPG_NUM_CLOCKS; ++i) {
> > +		const struct clk_div_table *table = NULL;
> > +		const char *parent_name = "main";
> > +		const char *name;
> > +		unsigned int shift;
> > +		unsigned int mult = 1;
> > +		unsigned int div = 1;
> > +		struct clk *clk;
> > +
> > +		of_property_read_string_index(np, "clock-output-names", i,
> > +					      &name);
> > +
> > +		switch (i) {
> > +		case R8A7790_CLK_MAIN:
> > +			parent_name = of_clk_get_parent_name(np, 0);
> > +			div = config->extal_div;
> > +			break;
> > +		case R8A7790_CLK_PLL1:
> > +			mult = config->pll1_mult / 2;
> > +			break;
> > +		case R8A7790_CLK_PLL3:
> > +			mult = config->pll3_mult;
> > +			break;
> > +		case R8A7790_CLK_LB:
> > +			div = cpg_mode & BIT(18) ? 36 : 24;
> > +			break;
> > +		case R8A7790_CLK_QSPI:
> > +			div = (cpg_mode & (BIT(3) | BIT(2) | BIT(1))) == BIT(2)
> > +			    ? 16 : 20;
> > +			break;
> > +		case R8A7790_CLK_SDH:
> > +			table = cpg_sdh_div_table;
> > +			shift = 8;
> > +			break;
> > +		case R8A7790_CLK_SD0:
> > +			table = cpg_sd01_div_table;
> > +			shift = 4;
> > +			break;
> > +		case R8A7790_CLK_SD1:
> > +			table = cpg_sd01_div_table;
> > +			shift = 0;
> > +			break;
> > +		}
> 
> Is this clock-output-names realy "Required" property ?
> The "name" and "order" seem fixed, then,
> I guess it can simply use "name array" ?

The clock-output-names property is required by the of_clk_get_parent_name() 
function. The property is mandatory for all clocks that need to be referenced 
by name, which is the case of all non-leaf clocks on our platforms. We thus 
need it.

> > +static void __init r8a7790_cpg_clocks_init(struct device_node *np)
> > +{
> > +	const struct cpg_pll_config *config;
> > +	struct r8a7790_cpg *cpg;
> > +	struct clk **clks;
> > +	unsigned int i;
> > +
> > +	cpg = kzalloc(sizeof(*cpg), GFP_KERNEL);
> > +	clks = kzalloc(CPG_NUM_CLOCKS * sizeof(*clks), GFP_KERNEL);
> > +	if (cpg == NULL || clks == NULL) {
> > +		kfree(clks);
> > +		kfree(cpg);
> > +		pr_err("%s: failed to allocate cpg\n", __func__);
> > +		return;
> > +	}
> > +
> > +	spin_lock_init(&cpg->lock);
> > +
> > +	cpg->data.clks = clks;
> > +	cpg->data.clk_num = CPG_NUM_CLOCKS;
> > +
> > +	cpg->reg = of_iomap(np, 0);
> > +	if (WARN_ON(cpg->reg == NULL)) {
> > +		kfree(cpg->data.clks);
> > +		kfree(cpg);
> > +		return;
> > +	}
> 
> Above 2 error cases are doing same thing.
> If can use goto xxx_error.

Good point.

Given that a failure to allocate memory or ioremap registers will lead to a 
boot failure, I wonder whether we couldn't just remove the kfree() calls and 
leak memory. Is there a point in cleaning up behind us if the system will no 
boot due to missing core clocks anyway ?

Kuninori Morimoto Nov. 6, 2013, 12:54 a.m. UTC | #6

Hi Laurent

> > > +	for (i = 0; i < CPG_NUM_CLOCKS; ++i) {
> > > +		const struct clk_div_table *table = NULL;
> > > +		const char *parent_name = "main";
> > > +		const char *name;
> > > +		unsigned int shift;
> > > +		unsigned int mult = 1;
> > > +		unsigned int div = 1;
> > > +		struct clk *clk;
> > > +
> > > +		of_property_read_string_index(np, "clock-output-names", i,
> > > +					      &name);
> > > +
> > > +		switch (i) {
> > > +		case R8A7790_CLK_MAIN:
> > > +			parent_name = of_clk_get_parent_name(np, 0);
> > > +			div = config->extal_div;
> > > +			break;
> > > +		case R8A7790_CLK_PLL1:
> > > +			mult = config->pll1_mult / 2;
> > > +			break;
> > > +		case R8A7790_CLK_PLL3:
> > > +			mult = config->pll3_mult;
> > > +			break;
> > > +		case R8A7790_CLK_LB:
> > > +			div = cpg_mode & BIT(18) ? 36 : 24;
> > > +			break;
> > > +		case R8A7790_CLK_QSPI:
> > > +			div = (cpg_mode & (BIT(3) | BIT(2) | BIT(1))) == BIT(2)
> > > +			    ? 16 : 20;
> > > +			break;
> > > +		case R8A7790_CLK_SDH:
> > > +			table = cpg_sdh_div_table;
> > > +			shift = 8;
> > > +			break;
> > > +		case R8A7790_CLK_SD0:
> > > +			table = cpg_sd01_div_table;
> > > +			shift = 4;
> > > +			break;
> > > +		case R8A7790_CLK_SD1:
> > > +			table = cpg_sd01_div_table;
> > > +			shift = 0;
> > > +			break;
> > > +		}
> > 
> > Is this clock-output-names realy "Required" property ?
> > The "name" and "order" seem fixed, then,
> > I guess it can simply use "name array" ?
> 
> The clock-output-names property is required by the of_clk_get_parent_name() 
> function. The property is mandatory for all clocks that need to be referenced 
> by name, which is the case of all non-leaf clocks on our platforms. We thus 
> need it.

Please correct me if my understanding was wrong.
Does your "of_clk_get_parent_name()" means "case R8A7790_CLK_MAIN"'s one ?
If Yes, it is needed on "parent" clock side, not here ?
If No,  who need/call of_clk_get_parent_name() for this ?
does "qspi", "sdh", "sd0", "sd1" can be parent clock for some device ??

And, parent of main clock is fixed by MD pin settings.
SW can't exchange it.

Best regards
---
Kuninori Morimoto

Laurent Pinchart Nov. 6, 2013, 1 a.m. UTC | #7

Hi Morimoto-san,

On Tuesday 05 November 2013 16:54:31 Kuninori Morimoto wrote:
> Hi Laurent
> 
> > > > +	for (i = 0; i < CPG_NUM_CLOCKS; ++i) {
> > > > +		const struct clk_div_table *table = NULL;
> > > > +		const char *parent_name = "main";
> > > > +		const char *name;
> > > > +		unsigned int shift;
> > > > +		unsigned int mult = 1;
> > > > +		unsigned int div = 1;
> > > > +		struct clk *clk;
> > > > +
> > > > +		of_property_read_string_index(np, "clock-output-names", i,
> > > > +					      &name);
> > > > +
> > > > +		switch (i) {
> > > > +		case R8A7790_CLK_MAIN:
> > > > +			parent_name = of_clk_get_parent_name(np, 0);
> > > > +			div = config->extal_div;
> > > > +			break;
> > > > +		case R8A7790_CLK_PLL1:
> > > > +			mult = config->pll1_mult / 2;
> > > > +			break;
> > > > +		case R8A7790_CLK_PLL3:
> > > > +			mult = config->pll3_mult;
> > > > +			break;
> > > > +		case R8A7790_CLK_LB:
> > > > +			div = cpg_mode & BIT(18) ? 36 : 24;
> > > > +			break;
> > > > +		case R8A7790_CLK_QSPI:
> > > > +			div = (cpg_mode & (BIT(3) | BIT(2) | BIT(1))) == BIT(2)
> > > > +			    ? 16 : 20;
> > > > +			break;
> > > > +		case R8A7790_CLK_SDH:
> > > > +			table = cpg_sdh_div_table;
> > > > +			shift = 8;
> > > > +			break;
> > > > +		case R8A7790_CLK_SD0:
> > > > +			table = cpg_sd01_div_table;
> > > > +			shift = 4;
> > > > +			break;
> > > > +		case R8A7790_CLK_SD1:
> > > > +			table = cpg_sd01_div_table;
> > > > +			shift = 0;
> > > > +			break;
> > > > +		}
> > > 
> > > Is this clock-output-names realy "Required" property ?
> > > The "name" and "order" seem fixed, then,
> > > I guess it can simply use "name array" ?
> > 
> > The clock-output-names property is required by the
> > of_clk_get_parent_name()
> > function. The property is mandatory for all clocks that need to be
> > referenced by name, which is the case of all non-leaf clocks on our
> > platforms. We thus need it.
> 
> Please correct me if my understanding was wrong.
> Does your "of_clk_get_parent_name()" means "case R8A7790_CLK_MAIN"'s one ?
> If Yes, it is needed on "parent" clock side, not here ?
> If No,  who need/call of_clk_get_parent_name() for this ?
> does "qspi", "sdh", "sd0", "sd1" can be parent clock for some device ??

All those clocks are parents of other clocks (DIV6, MSTP or fixed-factor 
clocks). The DIV6, MSTP and fixed-factor clock drivers call 
of_clk_get_parent_name() to get the name of their parent clock, which is 
required to register the clocks with CCF. See of_fixed_factor_clk_setup() for 
instance.

> And, parent of main clock is fixed by MD pin settings.
> SW can't exchange it.

Kuninori Morimoto Nov. 6, 2013, 2:31 a.m. UTC | #8

Hi Laurent

> > Please correct me if my understanding was wrong.
> > Does your "of_clk_get_parent_name()" means "case R8A7790_CLK_MAIN"'s one ?
> > If Yes, it is needed on "parent" clock side, not here ?
> > If No,  who need/call of_clk_get_parent_name() for this ?
> > does "qspi", "sdh", "sd0", "sd1" can be parent clock for some device ??
> 
> All those clocks are parents of other clocks (DIV6, MSTP or fixed-factor 
> clocks). The DIV6, MSTP and fixed-factor clock drivers call 
> of_clk_get_parent_name() to get the name of their parent clock, which is 
> required to register the clocks with CCF. See of_fixed_factor_clk_setup() for 
> instance.

Ahh... OK, I understand.

Hmm...
I guesss We can use [1/3] and [2/3] patches for all Renesas SoC as generic driver,
but, all SoC needs clk/shmobile/clk-xxxx.c like [3/3].
Because it has SoC special divider values.
Is this correct ?

Now, your [1/3] patch (= for MSTP) added renesas special property
as "renesas,clock-indices".
Is it impossible to add renesas special property like "renesas,clock-custom-divider"
which can specify custom divider values in generic cpg driver ?
If we can have it, all Renesas SoC can use generic driver ?

Best regards
---
Kuninori Morimoto

Simon Horman Nov. 6, 2013, 7:18 a.m. UTC | #9

On Tue, Oct 29, 2013 at 03:55:11PM +0100, Laurent Pinchart wrote:
> The R8A7790 has several clocks that are too custom to be supported in a
> generic driver. Those clocks can be divided in two categories:
> 
> - Fixed rate clocks with multiplier and divisor set according to boot
>   mode configuration
> 
> - Custom divider clocks with SoC-specific divider values
> 
> This driver supports both.
> 
> Signed-off-by: Laurent Pinchart <[e-mail]>
> ---
>  .../bindings/clock/renesas,r8a7790-cpg-clocks.txt  |  26 +++
>  drivers/clk/shmobile/Makefile                      |   1 +
>  drivers/clk/shmobile/clk-r8a7790.c                 | 176 +++++++++++++++++++++
>  include/dt-bindings/clock/r8a7790-clock.h          |  10 ++
>  include/linux/clk/shmobile.h                       |  19 +++
>  5 files changed, 232 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt
>  create mode 100644 drivers/clk/shmobile/clk-r8a7790.c
>  create mode 100644 include/linux/clk/shmobile.h
> 
> diff --git a/Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt b/Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt
> new file mode 100644
> index 0000000..d889917
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt
> @@ -0,0 +1,26 @@
> +* Renesas R8A7790 Clock Pulse Generator (CPG)
> +
> +The CPG generates core clocks for the R8A7790. It includes three PLLs and
> +several fixed ratio dividers.
> +
> +Required Properties:
> +
> +  - compatible: Must be "renesas,r8a7790-cpg-clocks"
> +  - reg: Base address and length of the memory resource used by the CPG
> +  - clocks: Reference to the parent clock
> +  - #clock-cells: Must be 1
> +  - clock-output-names: The name of the clocks, must be "main", "pll1",

s/The name/The names/

> +    "pll3", "lb", "qspi", "sdh", "sd0", "sd1".
> +
> +
> +Example
> +-------
> +
> +	cpg_clocks: cpg_clocks {
> +		compatible = "renesas,r8a7790-cpg-clocks";
> +		reg = <0 0xe6150000 0 0x1000>;
> +		clocks = <&extal_clk>;
> +		#clock-cells = <1>;
> +		clock-output-names = "main", "pll1", "pll3", "lb",
> +				     "qspi", "sdh", "sd0", "sd1";
> +	};

I wonder if it makes sense to craft a more generic bindings document.

I am currently working on clock support for the r8a7779 (H1) based
on this patch series and I expect the bindings to end up being
more or less the same.

I expect there to be similar support to be added for many other renesas SoCs
over time.

> diff --git a/drivers/clk/shmobile/Makefile b/drivers/clk/shmobile/Makefile
> index 3275c78..949f29e 100644
> --- a/drivers/clk/shmobile/Makefile
> +++ b/drivers/clk/shmobile/Makefile
> @@ -1,4 +1,5 @@
>  obj-$(CONFIG_ARCH_EMEV2)		+= clk-emev2.o
> +obj-$(CONFIG_ARCH_R8A7790)		+= clk-r8a7790.o
>  obj-$(CONFIG_ARCH_SHMOBILE_MULTI)	+= clk-div6.o
>  obj-$(CONFIG_ARCH_SHMOBILE_MULTI)	+= clk-mstp.o
>  
> diff --git a/drivers/clk/shmobile/clk-r8a7790.c b/drivers/clk/shmobile/clk-r8a7790.c
> new file mode 100644
> index 0000000..2e76638
> --- /dev/null
> +++ b/drivers/clk/shmobile/clk-r8a7790.c
> @@ -0,0 +1,176 @@
> +/*
> + * r8a7790 Core CPG Clocks
> + *
> + * Copyright (C) 2013  Ideas On Board SPRL
> + *
> + * Contact: Laurent Pinchart <[e-mail]>
> + *
> + * 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/clkdev.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/spinlock.h>
> +
> +#include <dt-bindings/clock/r8a7790-clock.h>
> +
> +#define CPG_NUM_CLOCKS			(R8A7790_CLK_SD1 + 1)

Perhaps R8A7790_NUM_CLOCKS in r8a7790-clock.h would be nicer.

> +
> +struct r8a7790_cpg {
> +	struct clk_onecell_data data;
> +	spinlock_t lock;
> +	void __iomem *reg;
> +};
> +
> +/*
> + *   MD		EXTAL		PLL0	PLL1	PLL3
> + * 14 13 19	(MHz)		*1	*1
> + *---------------------------------------------------
> + * 0  0  0	15 x 1		x172/2	x208/2	x106
> + * 0  0  1	15 x 1		x172/2	x208/2	x88
> + * 0  1  0	20 x 1		x130/2	x156/2	x80
> + * 0  1  1	20 x 1		x130/2	x156/2	x66
> + * 1  0  0	26 / 2		x200/2	x240/2	x122
> + * 1  0  1	26 / 2		x200/2	x240/2	x102
> + * 1  1  0	30 / 2		x172/2	x208/2	x106
> + * 1  1  1	30 / 2		x172/2	x208/2	x88
> + *
> + * *1 :	Table 7.6 indicates VCO ouput (PLLx = VCO/2)
> + */
> +#define CPG_PLL_CONFIG_INDEX(md)	((((md) & BIT(14)) >> 12) | \
> +					 (((md) & BIT(13)) >> 12) | \
> +					 (((md) & BIT(19)) >> 19))
> +struct cpg_pll_config {
> +	unsigned int extal_div;
> +	unsigned int pll1_mult;
> +	unsigned int pll3_mult;
> +};
> +
> +static const struct cpg_pll_config cpg_pll_configs[8] = {
> +	{ 1, 208, 106 }, { 1, 208,  88 }, { 1, 156,  80 }, { 1, 156,  66 },
> +	{ 2, 240, 122 }, { 2, 240, 102 }, { 2, 208, 106 }, { 2, 208,  88 },
> +};
> +
> +/* SDHI divisors */
> +static const struct clk_div_table cpg_sdh_div_table[] = {
> +	{  0,  2 }, {  1,  3 }, {  2,  4 }, {  3,  6 },
> +	{  4,  8 }, {  5, 12 }, {  6, 16 }, {  7, 18 },
> +	{  8, 24 }, { 10, 36 }, { 11, 48 }, {  0,  0 },
> +};
> +
> +static const struct clk_div_table cpg_sd01_div_table[] = {
> +	{  5, 12 }, {  6, 16 }, {  7, 18 }, {  8, 24 },
> +	{ 10, 36 }, { 11, 48 }, { 12, 10 }, {  0,  0 },
> +};

Can any or all of the above three constants be __initconst?

> +
> +static u32 cpg_mode __initdata;
> +
> +#define CPG_SDCKCR			0x00000074
> +
> +static void __init r8a7790_cpg_clocks_init(struct device_node *np)
> +{
> +	const struct cpg_pll_config *config;
> +	struct r8a7790_cpg *cpg;
> +	struct clk **clks;
> +	unsigned int i;
> +
> +	cpg = kzalloc(sizeof(*cpg), GFP_KERNEL);

Nit: s/sizeof(*cpg)/sizeof *cpg/, sizeof is an operator.
> +	clks = kzalloc(CPG_NUM_CLOCKS * sizeof(*clks), GFP_KERNEL);
> +	if (cpg == NULL || clks == NULL) {
> +		kfree(clks);
> +		kfree(cpg);
> +		pr_err("%s: failed to allocate cpg\n", __func__);
> +		return;
> +	}
> +
> +	spin_lock_init(&cpg->lock);
> +
> +	cpg->data.clks = clks;
> +	cpg->data.clk_num = CPG_NUM_CLOCKS;
> +
> +	cpg->reg = of_iomap(np, 0);
> +	if (WARN_ON(cpg->reg == NULL)) {
> +		kfree(cpg->data.clks);
> +		kfree(cpg);
> +		return;
> +	}

Perhaps this error checking could be merged with that of cpg and clks?

> +
> +	config = &cpg_pll_configs[CPG_PLL_CONFIG_INDEX(cpg_mode)];
> +
> +	for (i = 0; i < CPG_NUM_CLOCKS; ++i) {
> +		const struct clk_div_table *table = NULL;
> +		const char *parent_name = "main";
> +		const char *name;
> +		unsigned int shift;
> +		unsigned int mult = 1;
> +		unsigned int div = 1;
> +		struct clk *clk;
> +
> +		of_property_read_string_index(np, "clock-output-names", i,
> +					      &name);
> +
> +		switch (i) {
> +		case R8A7790_CLK_MAIN:
> +			parent_name = of_clk_get_parent_name(np, 0);
> +			div = config->extal_div;
> +			break;
> +		case R8A7790_CLK_PLL1:
> +			mult = config->pll1_mult / 2;
> +			break;
> +		case R8A7790_CLK_PLL3:
> +			mult = config->pll3_mult;
> +			break;
> +		case R8A7790_CLK_LB:
> +			div = cpg_mode & BIT(18) ? 36 : 24;
> +			break;
> +		case R8A7790_CLK_QSPI:
> +			div = (cpg_mode & (BIT(3) | BIT(2) | BIT(1))) == BIT(2)
> +			    ? 16 : 20;
> +			break;
> +		case R8A7790_CLK_SDH:
> +			table = cpg_sdh_div_table;
> +			shift = 8;
> +			break;
> +		case R8A7790_CLK_SD0:
> +			table = cpg_sd01_div_table;
> +			shift = 4;
> +			break;
> +		case R8A7790_CLK_SD1:
> +			table = cpg_sd01_div_table;
> +			shift = 0;
> +			break;
> +		}

I wonder if it would be good to and skip the portions below if the switch
statement hits a default: case.

Also, if i was an enum type then I think the C compiler would warn
about any missing cases. That might be nice too.

> +
> +		if (!table)
> +			clk = clk_register_fixed_factor(NULL, name, parent_name,
> +							0, mult, div);
> +		else
> +			clk = clk_register_divider_table(NULL, name,
> +							 parent_name, 0,
> +							 cpg->reg + CPG_SDCKCR,
> +							 shift, 4, 0, table,
> +							 &cpg->lock);
> +
> +		if (IS_ERR(clk))
> +			pr_err("%s: failed to register %s %s clock (%ld)\n",
> +			       __func__, np->name, name, PTR_ERR(clk));
> +	}
> +
> +	of_clk_add_provider(np, of_clk_src_onecell_get, &cpg->data);
> +}
> +CLK_OF_DECLARE(r8a7790_cpg_clks, "renesas,r8a7790-cpg-clocks",
> +	       r8a7790_cpg_clocks_init);
> +
> +void __init r8a7790_clocks_init(u32 mode)
> +{
> +	cpg_mode = mode;
> +
> +	of_clk_init(NULL);
> +}
> diff --git a/include/dt-bindings/clock/r8a7790-clock.h b/include/dt-bindings/clock/r8a7790-clock.h
> index 19f2b48..f0ed742 100644
> --- a/include/dt-bindings/clock/r8a7790-clock.h
> +++ b/include/dt-bindings/clock/r8a7790-clock.h
> @@ -10,6 +10,16 @@
>  #ifndef __DT_BINDINGS_CLOCK_R8A7790_H__
>  #define __DT_BINDINGS_CLOCK_R8A7790_H__
>  
> +/* CPG */
> +#define R8A7790_CLK_MAIN		0
> +#define R8A7790_CLK_PLL1		1
> +#define R8A7790_CLK_PLL3		2
> +#define R8A7790_CLK_LB			3
> +#define R8A7790_CLK_QSPI		4
> +#define R8A7790_CLK_SDH			5
> +#define R8A7790_CLK_SD0			6
> +#define R8A7790_CLK_SD1			7
> +
>  /* MSTP1 */
>  #define R8A7790_CLK_CMT0		20
>  
> diff --git a/include/linux/clk/shmobile.h b/include/linux/clk/shmobile.h
> new file mode 100644
> index 0000000..b090855
> --- /dev/null
> +++ b/include/linux/clk/shmobile.h
> @@ -0,0 +1,19 @@
> +/*
> + * Copyright 2013 Ideas On Board SPRL
> + *
> + * Contact: Laurent Pinchart <[e-mail]>
> + *
> + * 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; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#ifndef __LINUX_CLK_SHMOBILE_H_
> +#define __LINUX_CLK_SHMOBILE_H_
> +
> +#include <linux/types.h>
> +
> +void r8a7790_clocks_init(u32 mode);
> +
> +#endif
> -- 
> 1.8.1.5
> 
> --
> 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 Nov. 6, 2013, 12:41 p.m. UTC | #10

Hi Morimoto-san,

On Tuesday 05 November 2013 18:31:31 Kuninori Morimoto wrote:
> Hi Laurent
> 
> > > Please correct me if my understanding was wrong.
> > > Does your "of_clk_get_parent_name()" means "case R8A7790_CLK_MAIN"'s one
> > > ?
> > > If Yes, it is needed on "parent" clock side, not here ?
> > > If No,  who need/call of_clk_get_parent_name() for this ?
> > > does "qspi", "sdh", "sd0", "sd1" can be parent clock for some device ??
> > 
> > All those clocks are parents of other clocks (DIV6, MSTP or fixed-factor
> > clocks). The DIV6, MSTP and fixed-factor clock drivers call
> > of_clk_get_parent_name() to get the name of their parent clock, which is
> > required to register the clocks with CCF. See of_fixed_factor_clk_setup()
> > for instance.
> 
> Ahh... OK, I understand.
> 
> Hmm...
> I guesss We can use [1/3] and [2/3] patches for all Renesas SoC as generic
> driver, but, all SoC needs clk/shmobile/clk-xxxx.c like [3/3].
> Because it has SoC special divider values.
> Is this correct ?

That's correct.

> Now, your [1/3] patch (= for MSTP) added renesas special property
> as "renesas,clock-indices".
> Is it impossible to add renesas special property like
> "renesas,clock-custom-divider" which can specify custom divider values in
> generic cpg driver ?
> If we can have it, all Renesas SoC can use generic driver ?

The clocks supported by the SoC CPG driver are special in the sense that 
they're not really standardized. Most of them are fixed-factor clocks with a 
factor that depends on boot mode pins. The others are DIV4 clocks that vary 
widely from SoC to SoC. Expressing all that information in DT would be much 
more complex than expressing it in C code, which is why I've decided to go for 
a per-SoC CPG driver, especially given that the amount of code isn't that 
large.

Laurent Pinchart Nov. 6, 2013, 12:56 p.m. UTC | #11

Hi Simon,

On Wednesday 06 November 2013 16:18:09 Simon Horman wrote:
> On Tue, Oct 29, 2013 at 03:55:11PM +0100, Laurent Pinchart wrote:
> > The R8A7790 has several clocks that are too custom to be supported in a
> > generic driver. Those clocks can be divided in two categories:
> > 
> > - Fixed rate clocks with multiplier and divisor set according to boot
> >   mode configuration
> > 
> > - Custom divider clocks with SoC-specific divider values
> > 
> > This driver supports both.
> > 
> > Signed-off-by: Laurent Pinchart
> > <[e-mail]>
> > ---
> > 
> >  .../bindings/clock/renesas,r8a7790-cpg-clocks.txt  |  26 +++
> >  drivers/clk/shmobile/Makefile                      |   1 +
> >  drivers/clk/shmobile/clk-r8a7790.c                 | 176 ++++++++++++++++
> >  include/dt-bindings/clock/r8a7790-clock.h          |  10 ++
> >  include/linux/clk/shmobile.h                       |  19 +++
> >  5 files changed, 232 insertions(+)
> >  create mode 100644
> >  Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt
> >  create mode 100644 drivers/clk/shmobile/clk-r8a7790.c
> >  create mode 100644 include/linux/clk/shmobile.h
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt
> > b/Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt
> > new file mode 100644
> > index 0000000..d889917
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt
> > @@ -0,0 +1,26 @@
> > +* Renesas R8A7790 Clock Pulse Generator (CPG)
> > +
> > +The CPG generates core clocks for the R8A7790. It includes three PLLs and
> > +several fixed ratio dividers.
> > +
> > +Required Properties:
> > +
> > +  - compatible: Must be "renesas,r8a7790-cpg-clocks"
> > +  - reg: Base address and length of the memory resource used by the CPG
> > +  - clocks: Reference to the parent clock
> > +  - #clock-cells: Must be 1
> > +  - clock-output-names: The name of the clocks, must be "main", "pll1",
> 
> s/The name/The names/

Will fix, thank you.

> > +    "pll3", "lb", "qspi", "sdh", "sd0", "sd1".
> > +
> > +
> > +Example
> > +-------
> > +
> > +	cpg_clocks: cpg_clocks {
> > +		compatible = "renesas,r8a7790-cpg-clocks";
> > +		reg = <0 0xe6150000 0 0x1000>;
> > +		clocks = <&extal_clk>;
> > +		#clock-cells = <1>;
> > +		clock-output-names = "main", "pll1", "pll3", "lb",
> > +				     "qspi", "sdh", "sd0", "sd1";
> > +	};
> 
> I wonder if it makes sense to craft a more generic bindings document.
> 
> I am currently working on clock support for the r8a7779 (H1) based
> on this patch series and I expect the bindings to end up being
> more or less the same.
> 
> I expect there to be similar support to be added for many other renesas SoCs
> over time.

That's a good idea. I'll gladly review patches :-)

> > diff --git a/drivers/clk/shmobile/Makefile b/drivers/clk/shmobile/Makefile
> > index 3275c78..949f29e 100644
> > --- a/drivers/clk/shmobile/Makefile
> > +++ b/drivers/clk/shmobile/Makefile
> > @@ -1,4 +1,5 @@
> >  obj-$(CONFIG_ARCH_EMEV2)		+= clk-emev2.o
> > +obj-$(CONFIG_ARCH_R8A7790)		+= clk-r8a7790.o
> >  obj-$(CONFIG_ARCH_SHMOBILE_MULTI)	+= clk-div6.o
> >  obj-$(CONFIG_ARCH_SHMOBILE_MULTI)	+= clk-mstp.o
> > diff --git a/drivers/clk/shmobile/clk-r8a7790.c
> > b/drivers/clk/shmobile/clk-r8a7790.c new file mode 100644
> > index 0000000..2e76638
> > --- /dev/null
> > +++ b/drivers/clk/shmobile/clk-r8a7790.c
> > @@ -0,0 +1,176 @@
> > +/*
> > + * r8a7790 Core CPG Clocks
> > + *
> > + * Copyright (C) 2013  Ideas On Board SPRL
> > + *
> > + * Contact: Laurent Pinchart <[e-mail]>
> > + *
> > + * 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/clkdev.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/spinlock.h>
> > +
> > +#include <dt-bindings/clock/r8a7790-clock.h>
> > +
> > +#define CPG_NUM_CLOCKS			(R8A7790_CLK_SD1 + 1)
> 
> Perhaps R8A7790_NUM_CLOCKS in r8a7790-clock.h would be nicer.

Good question. The dt-bindings/ headers are primarly meant to store macros 
used by .dts files and (possibly) shared with C code. Given that 
R8A7790_NUM_CLOCKS isn't used by .dts files I'm not sure it would belong to 
that header.

> > +
> > +struct r8a7790_cpg {
> > +	struct clk_onecell_data data;
> > +	spinlock_t lock;
> > +	void __iomem *reg;
> > +};
> > +
> > +/*
> > + *   MD		EXTAL		PLL0	PLL1	PLL3
> > + * 14 13 19	(MHz)		*1	*1
> > + *---------------------------------------------------
> > + * 0  0  0	15 x 1		x172/2	x208/2	x106
> > + * 0  0  1	15 x 1		x172/2	x208/2	x88
> > + * 0  1  0	20 x 1		x130/2	x156/2	x80
> > + * 0  1  1	20 x 1		x130/2	x156/2	x66
> > + * 1  0  0	26 / 2		x200/2	x240/2	x122
> > + * 1  0  1	26 / 2		x200/2	x240/2	x102
> > + * 1  1  0	30 / 2		x172/2	x208/2	x106
> > + * 1  1  1	30 / 2		x172/2	x208/2	x88
> > + *
> > + * *1 :	Table 7.6 indicates VCO ouput (PLLx = VCO/2)
> > + */
> > +#define CPG_PLL_CONFIG_INDEX(md)	((((md) & BIT(14)) >> 12) | \
> > +					 (((md) & BIT(13)) >> 12) | \
> > +					 (((md) & BIT(19)) >> 19))
> > +struct cpg_pll_config {
> > +	unsigned int extal_div;
> > +	unsigned int pll1_mult;
> > +	unsigned int pll3_mult;
> > +};
> > +
> > +static const struct cpg_pll_config cpg_pll_configs[8] = {
> > +	{ 1, 208, 106 }, { 1, 208,  88 }, { 1, 156,  80 }, { 1, 156,  66 },
> > +	{ 2, 240, 122 }, { 2, 240, 102 }, { 2, 208, 106 }, { 2, 208,  88 },
> > +};
> > +
> > +/* SDHI divisors */
> > +static const struct clk_div_table cpg_sdh_div_table[] = {
> > +	{  0,  2 }, {  1,  3 }, {  2,  4 }, {  3,  6 },
> > +	{  4,  8 }, {  5, 12 }, {  6, 16 }, {  7, 18 },
> > +	{  8, 24 }, { 10, 36 }, { 11, 48 }, {  0,  0 },
> > +};
> > +
> > +static const struct clk_div_table cpg_sd01_div_table[] = {
> > +	{  5, 12 }, {  6, 16 }, {  7, 18 }, {  8, 24 },
> > +	{ 10, 36 }, { 11, 48 }, { 12, 10 }, {  0,  0 },
> > +};
> 
> Can any or all of the above three constants be __initconst?

Not the clk_div_table arrays, as they're stored in the divider clock structure 
internally and used at runtime. The cpg_pll_configs array, however, can. I'll 
fix that.

> > +
> > +static u32 cpg_mode __initdata;
> > +
> > +#define CPG_SDCKCR			0x00000074
> > +
> > +static void __init r8a7790_cpg_clocks_init(struct device_node *np)
> > +{
> > +	const struct cpg_pll_config *config;
> > +	struct r8a7790_cpg *cpg;
> > +	struct clk **clks;
> > +	unsigned int i;
> > +
> > +	cpg = kzalloc(sizeof(*cpg), GFP_KERNEL);
> 
> Nit: s/sizeof(*cpg)/sizeof *cpg/, sizeof is an operator.

That's what I do in my non-kernel code, but the kernel coding style uses (). 
It took me a bit of time to get used to it :-)

> > +	clks = kzalloc(CPG_NUM_CLOCKS * sizeof(*clks), GFP_KERNEL);
> > +	if (cpg == NULL || clks == NULL) {
> > +		kfree(clks);
> > +		kfree(cpg);
> > +		pr_err("%s: failed to allocate cpg\n", __func__);
> > +		return;
> > +	}
> > +
> > +	spin_lock_init(&cpg->lock);
> > +
> > +	cpg->data.clks = clks;
> > +	cpg->data.clk_num = CPG_NUM_CLOCKS;
> > +
> > +	cpg->reg = of_iomap(np, 0);
> > +	if (WARN_ON(cpg->reg == NULL)) {
> > +		kfree(cpg->data.clks);
> > +		kfree(cpg);
> > +		return;
> > +	}
> 
> Perhaps this error checking could be merged with that of cpg and clks?

Morimoto-san has already pointed-out that issue. Here was my answer, your 
opinion would be appreciated.

Given that a failure to allocate memory or ioremap registers will lead to a 
boot failure, I wonder whether we couldn't just remove the kfree() calls and 
leak memory. Is there a point in cleaning up behind us if the system will no 
boot due to missing core clocks anyway ?

> > +
> > +	config = &cpg_pll_configs[CPG_PLL_CONFIG_INDEX(cpg_mode)];
> > +
> > +	for (i = 0; i < CPG_NUM_CLOCKS; ++i) {
> > +		const struct clk_div_table *table = NULL;
> > +		const char *parent_name = "main";
> > +		const char *name;
> > +		unsigned int shift;
> > +		unsigned int mult = 1;
> > +		unsigned int div = 1;
> > +		struct clk *clk;
> > +
> > +		of_property_read_string_index(np, "clock-output-names", i,
> > +					      &name);
> > +
> > +		switch (i) {
> > +		case R8A7790_CLK_MAIN:
> > +			parent_name = of_clk_get_parent_name(np, 0);
> > +			div = config->extal_div;
> > +			break;
> > +		case R8A7790_CLK_PLL1:
> > +			mult = config->pll1_mult / 2;
> > +			break;
> > +		case R8A7790_CLK_PLL3:
> > +			mult = config->pll3_mult;
> > +			break;
> > +		case R8A7790_CLK_LB:
> > +			div = cpg_mode & BIT(18) ? 36 : 24;
> > +			break;
> > +		case R8A7790_CLK_QSPI:
> > +			div = (cpg_mode & (BIT(3) | BIT(2) | BIT(1))) == BIT(2)
> > +			    ? 16 : 20;
> > +			break;
> > +		case R8A7790_CLK_SDH:
> > +			table = cpg_sdh_div_table;
> > +			shift = 8;
> > +			break;
> > +		case R8A7790_CLK_SD0:
> > +			table = cpg_sd01_div_table;
> > +			shift = 4;
> > +			break;
> > +		case R8A7790_CLK_SD1:
> > +			table = cpg_sd01_div_table;
> > +			shift = 0;
> > +			break;
> > +		}
> 
> I wonder if it would be good to and skip the portions below if the switch
> statement hits a default: case.

Yes, but that would be a bug in the driver, as all cases should be handled :-)

> Also, if i was an enum type then I think the C compiler would warn
> about any missing cases. That might be nice too.

It would be, but the DT compiler doesn't support enums, so we're stuck with 
#define's if we want to share them between .dts and C files.

> > +
> > +		if (!table)
> > +			clk = clk_register_fixed_factor(NULL, name, parent_name,
> > +							0, mult, div);
> > +		else
> > +			clk = clk_register_divider_table(NULL, name,
> > +							 parent_name, 0,
> > +							 cpg->reg + CPG_SDCKCR,
> > +							 shift, 4, 0, table,
> > +							 &cpg->lock);
> > +
> > +		if (IS_ERR(clk))
> > +			pr_err("%s: failed to register %s %s clock (%ld)\n",
> > +			       __func__, np->name, name, PTR_ERR(clk));
> > +	}
> > +
> > +	of_clk_add_provider(np, of_clk_src_onecell_get, &cpg->data);
> > +}
> > +CLK_OF_DECLARE(r8a7790_cpg_clks, "renesas,r8a7790-cpg-clocks",
> > +	       r8a7790_cpg_clocks_init);
> > +
> > +void __init r8a7790_clocks_init(u32 mode)
> > +{
> > +	cpg_mode = mode;
> > +
> > +	of_clk_init(NULL);
> > +}
> > diff --git a/include/dt-bindings/clock/r8a7790-clock.h
> > b/include/dt-bindings/clock/r8a7790-clock.h index 19f2b48..f0ed742 100644
> > --- a/include/dt-bindings/clock/r8a7790-clock.h
> > +++ b/include/dt-bindings/clock/r8a7790-clock.h
> > @@ -10,6 +10,16 @@
> > 
> >  #ifndef __DT_BINDINGS_CLOCK_R8A7790_H__
> >  #define __DT_BINDINGS_CLOCK_R8A7790_H__
> > 
> > +/* CPG */
> > +#define R8A7790_CLK_MAIN		0
> > +#define R8A7790_CLK_PLL1		1
> > +#define R8A7790_CLK_PLL3		2
> > +#define R8A7790_CLK_LB			3
> > +#define R8A7790_CLK_QSPI		4
> > +#define R8A7790_CLK_SDH			5
> > +#define R8A7790_CLK_SD0			6
> > +#define R8A7790_CLK_SD1			7
> > +
> > 
> >  /* MSTP1 */
> >  #define R8A7790_CLK_CMT0		20
> > 
> > diff --git a/include/linux/clk/shmobile.h b/include/linux/clk/shmobile.h
> > new file mode 100644
> > index 0000000..b090855
> > --- /dev/null
> > +++ b/include/linux/clk/shmobile.h
> > @@ -0,0 +1,19 @@
> > +/*
> > + * Copyright 2013 Ideas On Board SPRL
> > + *
> > + * Contact: Laurent Pinchart <[e-mail]>
> > + *
> > + * 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; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> > +
> > +#ifndef __LINUX_CLK_SHMOBILE_H_
> > +#define __LINUX_CLK_SHMOBILE_H_
> > +
> > +#include <linux/types.h>
> > +
> > +void r8a7790_clocks_init(u32 mode);
> > +
> > +#endif

Kuninori Morimoto Nov. 7, 2013, 3:22 a.m. UTC | #12

Hi Laurent

> > Now, your [1/3] patch (= for MSTP) added renesas special property
> > as "renesas,clock-indices".
> > Is it impossible to add renesas special property like
> > "renesas,clock-custom-divider" which can specify custom divider values in
> > generic cpg driver ?
> > If we can have it, all Renesas SoC can use generic driver ?
> 
> The clocks supported by the SoC CPG driver are special in the sense that 
> they're not really standardized. Most of them are fixed-factor clocks with a 
> factor that depends on boot mode pins. The others are DIV4 clocks that vary 
> widely from SoC to SoC. Expressing all that information in DT would be much 
> more complex than expressing it in C code, which is why I've decided to go for 
> a per-SoC CPG driver, especially given that the amount of code isn't that 
> large.

In my SuperH experience, CPG clock divier has same pattern.
In H2's SDCKCR register, SDHFC and SD0FC looks different.
But, the real different is only enabled bit area,
the value is same.
You can find same pattern on FRQCRB register too,
and, you can find same pattern on M2.

I guess FRQCRB/SDHFC can share code if it has mask.
This idea was used on R-Mobile series on div4.
(ex clock-sh73a0.c :: div4_clks)

Do you think we can use this idea on DT?
like this ?

	div4_sdh {
                compatible = "renesas,cpg-clocks-1/x";
                renesas,clock-divider-mask = 0xdff;
                ...
	}

Best regards
---
Kuninori Morimoto

Kuninori Morimoto Nov. 7, 2013, 7:20 a.m. UTC | #13

Hi Laurent again

> In my SuperH experience, CPG clock divier has same pattern.
> In H2's SDCKCR register, SDHFC and SD0FC looks different.
> But, the real different is only enabled bit area,
> the value is same.
> You can find same pattern on FRQCRB register too,
> and, you can find same pattern on M2.
> 
> I guess FRQCRB/SDHFC can share code if it has mask.
> This idea was used on R-Mobile series on div4.
> (ex clock-sh73a0.c :: div4_clks)

I talked it with Magnus, and I understand your headache.
I can agree about your approach now.

But, I guess H2 and M2 are very similar SoC.
and I don't want too many clock-${SoC} files.
Is it possible to share code like clk-gen2.c ?

Best regards
---
Kuninori Morimoto

Laurent Pinchart Nov. 7, 2013, 12:15 p.m. UTC | #14

Hi Morimoto-san,

On Wednesday 06 November 2013 23:20:32 Kuninori Morimoto wrote:
> Hi Laurent again
> 
> > In my SuperH experience, CPG clock divier has same pattern.
> > In H2's SDCKCR register, SDHFC and SD0FC looks different.
> > But, the real different is only enabled bit area,
> > the value is same.
> > You can find same pattern on FRQCRB register too,
> > and, you can find same pattern on M2.
> > 
> > I guess FRQCRB/SDHFC can share code if it has mask.
> > This idea was used on R-Mobile series on div4.
> > (ex clock-sh73a0.c :: div4_clks)
> 
> I talked it with Magnus, and I understand your headache.
> I can agree about your approach now.
> 
> But, I guess H2 and M2 are very similar SoC.
> and I don't want too many clock-${SoC} files.
> Is it possible to share code like clk-gen2.c ?

Sure, if the hardware is very similar, it makes sense to share code. I've had 
a quick look at the M2 documentation, and it seems to be indeed mostly 
identical to H2. Only a couple of clocks are missing, and a new GPU clock 
register is documented. As the GPU clock is mentioned in the H2 documentation 
as being programmable I would assume that the GPU clock register exists and is 
identical in H2 but has been forgotten in the documentation.

We should probably extend clock-r8a7790.c with M2 support and rename it to 
clock-rcar-gen2.c before pushing it to mainline. Would you like to submit a 
patch ? :-)

Kuninori Morimoto Nov. 8, 2013, 12:06 a.m. UTC | #15

Hi Laurent

> Sure, if the hardware is very similar, it makes sense to share code. I've had 
> a quick look at the M2 documentation, and it seems to be indeed mostly 
> identical to H2. Only a couple of clocks are missing, and a new GPU clock 
> register is documented. As the GPU clock is mentioned in the H2 documentation 
> as being programmable I would assume that the GPU clock register exists and is 
> identical in H2 but has been forgotten in the documentation.
> 
> We should probably extend clock-r8a7790.c with M2 support and rename it to 
> clock-rcar-gen2.c before pushing it to mainline. Would you like to submit a 
> patch ? :-)

Thank yor for your agreement.
I don't want to send "rename patch",
please consider it in your v2 patch if possible :P

Best regards
---
Kuninori Morimoto

Laurent Pinchart Nov. 8, 2013, 1 a.m. UTC | #16

Hi Morimoto-san,

On Thursday 07 November 2013 16:06:29 Kuninori Morimoto wrote:
> Hi Laurent
> 
> > Sure, if the hardware is very similar, it makes sense to share code. I've
> > had a quick look at the M2 documentation, and it seems to be indeed
> > mostly identical to H2. Only a couple of clocks are missing, and a new
> > GPU clock register is documented. As the GPU clock is mentioned in the H2
> > documentation as being programmable I would assume that the GPU clock
> > register exists and is identical in H2 but has been forgotten in the
> > documentation.
> > 
> > We should probably extend clock-r8a7790.c with M2 support and rename it to
> > clock-rcar-gen2.c before pushing it to mainline. Would you like to submit
> > a patch ? :-)
> 
> Thank yor for your agreement.
> I don't want to send "rename patch",
> please consider it in your v2 patch if possible :P

We also need to take into account that the two SoCs will have a different 
number of clocks. I'm thus inclined to replace the index-based switch 
statement by a similar construct that will match on the clock name instead. Do 
you have any opinion on that ?

Kuninori Morimoto Nov. 8, 2013, 6:02 a.m. UTC | #17

Hi Laurent

> > Thank yor for your agreement.
> > I don't want to send "rename patch",
> > please consider it in your v2 patch if possible :P
> 
> We also need to take into account that the two SoCs will have a different 
> number of clocks. I'm thus inclined to replace the index-based switch 
> statement by a similar construct that will match on the clock name instead. Do 
> you have any opinion on that ?

Of course sharing code is very nice, but, 
sometimes, it makes code difficult/dirty.

Now, I understand your headache (I have same headache :),
and, you and I are sharing same opinion I think.
So, I believe your decision.

Best regards
---
Kuninori Morimoto

Simon Horman Nov. 8, 2013, 6:34 a.m. UTC | #18

On Wed, Nov 06, 2013 at 01:56:18PM +0100, Laurent Pinchart wrote:
> Hi Simon,
> 
> On Wednesday 06 November 2013 16:18:09 Simon Horman wrote:
> > On Tue, Oct 29, 2013 at 03:55:11PM +0100, Laurent Pinchart wrote:
> > > The R8A7790 has several clocks that are too custom to be supported in a
> > > generic driver. Those clocks can be divided in two categories:
> > > 
> > > - Fixed rate clocks with multiplier and divisor set according to boot
> > >   mode configuration
> > > 
> > > - Custom divider clocks with SoC-specific divider values
> > > 
> > > This driver supports both.
> > > 
> > > Signed-off-by: Laurent Pinchart
> > > <[e-mail]>
> > > ---
> > > 
> > >  .../bindings/clock/renesas,r8a7790-cpg-clocks.txt  |  26 +++
> > >  drivers/clk/shmobile/Makefile                      |   1 +
> > >  drivers/clk/shmobile/clk-r8a7790.c                 | 176 ++++++++++++++++
> > >  include/dt-bindings/clock/r8a7790-clock.h          |  10 ++
> > >  include/linux/clk/shmobile.h                       |  19 +++
> > >  5 files changed, 232 insertions(+)
> > >  create mode 100644
> > >  Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt
> > >  create mode 100644 drivers/clk/shmobile/clk-r8a7790.c
> > >  create mode 100644 include/linux/clk/shmobile.h
> > > 
> > > diff --git
> > > a/Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt
> > > b/Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt
> > > new file mode 100644
> > > index 0000000..d889917
> > > --- /dev/null
> > > +++
> > > b/Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt
> > > @@ -0,0 +1,26 @@
> > > +* Renesas R8A7790 Clock Pulse Generator (CPG)
> > > +
> > > +The CPG generates core clocks for the R8A7790. It includes three PLLs and
> > > +several fixed ratio dividers.
> > > +
> > > +Required Properties:
> > > +
> > > +  - compatible: Must be "renesas,r8a7790-cpg-clocks"
> > > +  - reg: Base address and length of the memory resource used by the CPG
> > > +  - clocks: Reference to the parent clock
> > > +  - #clock-cells: Must be 1
> > > +  - clock-output-names: The name of the clocks, must be "main", "pll1",
> > 
> > s/The name/The names/
> 
> Will fix, thank you.
> 
> > > +    "pll3", "lb", "qspi", "sdh", "sd0", "sd1".
> > > +
> > > +
> > > +Example
> > > +-------
> > > +
> > > +	cpg_clocks: cpg_clocks {
> > > +		compatible = "renesas,r8a7790-cpg-clocks";
> > > +		reg = <0 0xe6150000 0 0x1000>;
> > > +		clocks = <&extal_clk>;
> > > +		#clock-cells = <1>;
> > > +		clock-output-names = "main", "pll1", "pll3", "lb",
> > > +				     "qspi", "sdh", "sd0", "sd1";
> > > +	};
> > 
> > I wonder if it makes sense to craft a more generic bindings document.
> > 
> > I am currently working on clock support for the r8a7779 (H1) based
> > on this patch series and I expect the bindings to end up being
> > more or less the same.
> > 
> > I expect there to be similar support to be added for many other renesas SoCs
> > over time.
> 
> That's a good idea. I'll gladly review patches :-)
> 
> > > diff --git a/drivers/clk/shmobile/Makefile b/drivers/clk/shmobile/Makefile
> > > index 3275c78..949f29e 100644
> > > --- a/drivers/clk/shmobile/Makefile
> > > +++ b/drivers/clk/shmobile/Makefile
> > > @@ -1,4 +1,5 @@
> > >  obj-$(CONFIG_ARCH_EMEV2)		+= clk-emev2.o
> > > +obj-$(CONFIG_ARCH_R8A7790)		+= clk-r8a7790.o
> > >  obj-$(CONFIG_ARCH_SHMOBILE_MULTI)	+= clk-div6.o
> > >  obj-$(CONFIG_ARCH_SHMOBILE_MULTI)	+= clk-mstp.o
> > > diff --git a/drivers/clk/shmobile/clk-r8a7790.c
> > > b/drivers/clk/shmobile/clk-r8a7790.c new file mode 100644
> > > index 0000000..2e76638
> > > --- /dev/null
> > > +++ b/drivers/clk/shmobile/clk-r8a7790.c
> > > @@ -0,0 +1,176 @@
> > > +/*
> > > + * r8a7790 Core CPG Clocks
> > > + *
> > > + * Copyright (C) 2013  Ideas On Board SPRL
> > > + *
> > > + * Contact: Laurent Pinchart <[e-mail]>
> > > + *
> > > + * 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/clkdev.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/spinlock.h>
> > > +
> > > +#include <dt-bindings/clock/r8a7790-clock.h>
> > > +
> > > +#define CPG_NUM_CLOCKS			(R8A7790_CLK_SD1 + 1)
> > 
> > Perhaps R8A7790_NUM_CLOCKS in r8a7790-clock.h would be nicer.
> 
> Good question. The dt-bindings/ headers are primarly meant to store macros 
> used by .dts files and (possibly) shared with C code. Given that 
> R8A7790_NUM_CLOCKS isn't used by .dts files I'm not sure it would belong to 
> that header.

That makes sense. Lets leave things as you have them.

> > > +
> > > +struct r8a7790_cpg {
> > > +	struct clk_onecell_data data;
> > > +	spinlock_t lock;
> > > +	void __iomem *reg;
> > > +};
> > > +
> > > +/*
> > > + *   MD		EXTAL		PLL0	PLL1	PLL3
> > > + * 14 13 19	(MHz)		*1	*1
> > > + *---------------------------------------------------
> > > + * 0  0  0	15 x 1		x172/2	x208/2	x106
> > > + * 0  0  1	15 x 1		x172/2	x208/2	x88
> > > + * 0  1  0	20 x 1		x130/2	x156/2	x80
> > > + * 0  1  1	20 x 1		x130/2	x156/2	x66
> > > + * 1  0  0	26 / 2		x200/2	x240/2	x122
> > > + * 1  0  1	26 / 2		x200/2	x240/2	x102
> > > + * 1  1  0	30 / 2		x172/2	x208/2	x106
> > > + * 1  1  1	30 / 2		x172/2	x208/2	x88
> > > + *
> > > + * *1 :	Table 7.6 indicates VCO ouput (PLLx = VCO/2)
> > > + */
> > > +#define CPG_PLL_CONFIG_INDEX(md)	((((md) & BIT(14)) >> 12) | \
> > > +					 (((md) & BIT(13)) >> 12) | \
> > > +					 (((md) & BIT(19)) >> 19))
> > > +struct cpg_pll_config {
> > > +	unsigned int extal_div;
> > > +	unsigned int pll1_mult;
> > > +	unsigned int pll3_mult;
> > > +};
> > > +
> > > +static const struct cpg_pll_config cpg_pll_configs[8] = {
> > > +	{ 1, 208, 106 }, { 1, 208,  88 }, { 1, 156,  80 }, { 1, 156,  66 },
> > > +	{ 2, 240, 122 }, { 2, 240, 102 }, { 2, 208, 106 }, { 2, 208,  88 },
> > > +};
> > > +
> > > +/* SDHI divisors */
> > > +static const struct clk_div_table cpg_sdh_div_table[] = {
> > > +	{  0,  2 }, {  1,  3 }, {  2,  4 }, {  3,  6 },
> > > +	{  4,  8 }, {  5, 12 }, {  6, 16 }, {  7, 18 },
> > > +	{  8, 24 }, { 10, 36 }, { 11, 48 }, {  0,  0 },
> > > +};
> > > +
> > > +static const struct clk_div_table cpg_sd01_div_table[] = {
> > > +	{  5, 12 }, {  6, 16 }, {  7, 18 }, {  8, 24 },
> > > +	{ 10, 36 }, { 11, 48 }, { 12, 10 }, {  0,  0 },
> > > +};
> > 
> > Can any or all of the above three constants be __initconst?
> 
> Not the clk_div_table arrays, as they're stored in the divider clock structure 
> internally and used at runtime. The cpg_pll_configs array, however, can. I'll 
> fix that.
> 
> > > +
> > > +static u32 cpg_mode __initdata;
> > > +
> > > +#define CPG_SDCKCR			0x00000074
> > > +
> > > +static void __init r8a7790_cpg_clocks_init(struct device_node *np)
> > > +{
> > > +	const struct cpg_pll_config *config;
> > > +	struct r8a7790_cpg *cpg;
> > > +	struct clk **clks;
> > > +	unsigned int i;
> > > +
> > > +	cpg = kzalloc(sizeof(*cpg), GFP_KERNEL);
> > 
> > Nit: s/sizeof(*cpg)/sizeof *cpg/, sizeof is an operator.
> 
> That's what I do in my non-kernel code, but the kernel coding style uses (). 
> It took me a bit of time to get used to it :-)
> 
> > > +	clks = kzalloc(CPG_NUM_CLOCKS * sizeof(*clks), GFP_KERNEL);
> > > +	if (cpg == NULL || clks == NULL) {
> > > +		kfree(clks);
> > > +		kfree(cpg);
> > > +		pr_err("%s: failed to allocate cpg\n", __func__);
> > > +		return;
> > > +	}
> > > +
> > > +	spin_lock_init(&cpg->lock);
> > > +
> > > +	cpg->data.clks = clks;
> > > +	cpg->data.clk_num = CPG_NUM_CLOCKS;
> > > +
> > > +	cpg->reg = of_iomap(np, 0);
> > > +	if (WARN_ON(cpg->reg == NULL)) {
> > > +		kfree(cpg->data.clks);
> > > +		kfree(cpg);
> > > +		return;
> > > +	}
> > 
> > Perhaps this error checking could be merged with that of cpg and clks?
> 
> Morimoto-san has already pointed-out that issue. Here was my answer, your 
> opinion would be appreciated.
> 
> Given that a failure to allocate memory or ioremap registers will lead to a 
> boot failure, I wonder whether we couldn't just remove the kfree() calls and 
> leak memory. Is there a point in cleaning up behind us if the system will no 
> boot due to missing core clocks anyway ?

That sounds reasonable to me. But if you go that way I think
you should put detail the decision in a comment in
r8a7790_cpg_clocks_init(). Otherwise it seems likely that someone
will come and clean up the memory leak at some later date.

> 
> > > +
> > > +	config = &cpg_pll_configs[CPG_PLL_CONFIG_INDEX(cpg_mode)];
> > > +
> > > +	for (i = 0; i < CPG_NUM_CLOCKS; ++i) {
> > > +		const struct clk_div_table *table = NULL;
> > > +		const char *parent_name = "main";
> > > +		const char *name;
> > > +		unsigned int shift;
> > > +		unsigned int mult = 1;
> > > +		unsigned int div = 1;
> > > +		struct clk *clk;
> > > +
> > > +		of_property_read_string_index(np, "clock-output-names", i,
> > > +					      &name);
> > > +
> > > +		switch (i) {
> > > +		case R8A7790_CLK_MAIN:
> > > +			parent_name = of_clk_get_parent_name(np, 0);
> > > +			div = config->extal_div;
> > > +			break;
> > > +		case R8A7790_CLK_PLL1:
> > > +			mult = config->pll1_mult / 2;
> > > +			break;
> > > +		case R8A7790_CLK_PLL3:
> > > +			mult = config->pll3_mult;
> > > +			break;
> > > +		case R8A7790_CLK_LB:
> > > +			div = cpg_mode & BIT(18) ? 36 : 24;
> > > +			break;
> > > +		case R8A7790_CLK_QSPI:
> > > +			div = (cpg_mode & (BIT(3) | BIT(2) | BIT(1))) == BIT(2)
> > > +			    ? 16 : 20;
> > > +			break;
> > > +		case R8A7790_CLK_SDH:
> > > +			table = cpg_sdh_div_table;
> > > +			shift = 8;
> > > +			break;
> > > +		case R8A7790_CLK_SD0:
> > > +			table = cpg_sd01_div_table;
> > > +			shift = 4;
> > > +			break;
> > > +		case R8A7790_CLK_SD1:
> > > +			table = cpg_sd01_div_table;
> > > +			shift = 0;
> > > +			break;
> > > +		}
> > 
> > I wonder if it would be good to and skip the portions below if the switch
> > statement hits a default: case.
> 
> Yes, but that would be a bug in the driver, as all cases should be handled :-)
> 
> > Also, if i was an enum type then I think the C compiler would warn
> > about any missing cases. That might be nice too.
> 
> It would be, but the DT compiler doesn't support enums, so we're stuck with 
> #define's if we want to share them between .dts and C files.

That makes sense. Lets leave things as you have them.

> 
> > > +
> > > +		if (!table)
> > > +			clk = clk_register_fixed_factor(NULL, name, parent_name,
> > > +							0, mult, div);
> > > +		else
> > > +			clk = clk_register_divider_table(NULL, name,
> > > +							 parent_name, 0,
> > > +							 cpg->reg + CPG_SDCKCR,
> > > +							 shift, 4, 0, table,
> > > +							 &cpg->lock);
> > > +
> > > +		if (IS_ERR(clk))
> > > +			pr_err("%s: failed to register %s %s clock (%ld)\n",
> > > +			       __func__, np->name, name, PTR_ERR(clk));
> > > +	}
> > > +
> > > +	of_clk_add_provider(np, of_clk_src_onecell_get, &cpg->data);
> > > +}
> > > +CLK_OF_DECLARE(r8a7790_cpg_clks, "renesas,r8a7790-cpg-clocks",
> > > +	       r8a7790_cpg_clocks_init);
> > > +
> > > +void __init r8a7790_clocks_init(u32 mode)
> > > +{
> > > +	cpg_mode = mode;
> > > +
> > > +	of_clk_init(NULL);
> > > +}
> > > diff --git a/include/dt-bindings/clock/r8a7790-clock.h
> > > b/include/dt-bindings/clock/r8a7790-clock.h index 19f2b48..f0ed742 100644
> > > --- a/include/dt-bindings/clock/r8a7790-clock.h
> > > +++ b/include/dt-bindings/clock/r8a7790-clock.h
> > > @@ -10,6 +10,16 @@
> > > 
> > >  #ifndef __DT_BINDINGS_CLOCK_R8A7790_H__
> > >  #define __DT_BINDINGS_CLOCK_R8A7790_H__
> > > 
> > > +/* CPG */
> > > +#define R8A7790_CLK_MAIN		0
> > > +#define R8A7790_CLK_PLL1		1
> > > +#define R8A7790_CLK_PLL3		2
> > > +#define R8A7790_CLK_LB			3
> > > +#define R8A7790_CLK_QSPI		4
> > > +#define R8A7790_CLK_SDH			5
> > > +#define R8A7790_CLK_SD0			6
> > > +#define R8A7790_CLK_SD1			7
> > > +
> > > 
> > >  /* MSTP1 */
> > >  #define R8A7790_CLK_CMT0		20
> > > 
> > > diff --git a/include/linux/clk/shmobile.h b/include/linux/clk/shmobile.h
> > > new file mode 100644
> > > index 0000000..b090855
> > > --- /dev/null
> > > +++ b/include/linux/clk/shmobile.h
> > > @@ -0,0 +1,19 @@
> > > +/*
> > > + * Copyright 2013 Ideas On Board SPRL
> > > + *
> > > + * Contact: Laurent Pinchart <[e-mail]>
> > > + *
> > > + * 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; either version 2 of the License, or
> > > + * (at your option) any later version.
> > > + */
> > > +
> > > +#ifndef __LINUX_CLK_SHMOBILE_H_
> > > +#define __LINUX_CLK_SHMOBILE_H_
> > > +
> > > +#include <linux/types.h>
> > > +
> > > +void r8a7790_clocks_init(u32 mode);
> > > +
> > > +#endif
> -- 
> Regards,
> 
> Laurent Pinchart
>

Patch

diff --git a/Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt b/Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt
new file mode 100644
index 0000000..d889917
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt
@@ -0,0 +1,26 @@ 
+* Renesas R8A7790 Clock Pulse Generator (CPG)
+
+The CPG generates core clocks for the R8A7790. It includes three PLLs and
+several fixed ratio dividers.
+
+Required Properties:
+
+  - compatible: Must be "renesas,r8a7790-cpg-clocks"
+  - reg: Base address and length of the memory resource used by the CPG
+  - clocks: Reference to the parent clock
+  - #clock-cells: Must be 1
+  - clock-output-names: The name of the clocks, must be "main", "pll1",
+    "pll3", "lb", "qspi", "sdh", "sd0", "sd1".
+
+
+Example
+-------
+
+	cpg_clocks: cpg_clocks {
+		compatible = "renesas,r8a7790-cpg-clocks";
+		reg = <0 0xe6150000 0 0x1000>;
+		clocks = <&extal_clk>;
+		#clock-cells = <1>;
+		clock-output-names = "main", "pll1", "pll3", "lb",
+				     "qspi", "sdh", "sd0", "sd1";
+	};
diff --git a/drivers/clk/shmobile/Makefile b/drivers/clk/shmobile/Makefile
index 3275c78..949f29e 100644
--- a/drivers/clk/shmobile/Makefile
+++ b/drivers/clk/shmobile/Makefile
@@ -1,4 +1,5 @@ 
 obj-$(CONFIG_ARCH_EMEV2)		+= clk-emev2.o
+obj-$(CONFIG_ARCH_R8A7790)		+= clk-r8a7790.o
 obj-$(CONFIG_ARCH_SHMOBILE_MULTI)	+= clk-div6.o
 obj-$(CONFIG_ARCH_SHMOBILE_MULTI)	+= clk-mstp.o
 
diff --git a/drivers/clk/shmobile/clk-r8a7790.c b/drivers/clk/shmobile/clk-r8a7790.c
new file mode 100644
index 0000000..2e76638
--- /dev/null
+++ b/drivers/clk/shmobile/clk-r8a7790.c
@@ -0,0 +1,176 @@ 
+/*
+ * r8a7790 Core CPG Clocks
+ *
+ * Copyright (C) 2013  Ideas On Board SPRL
+ *
+ * Contact: Laurent Pinchart <[e-mail]>
+ *
+ * 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/clkdev.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/spinlock.h>
+
+#include <dt-bindings/clock/r8a7790-clock.h>
+
+#define CPG_NUM_CLOCKS			(R8A7790_CLK_SD1 + 1)
+
+struct r8a7790_cpg {
+	struct clk_onecell_data data;
+	spinlock_t lock;
+	void __iomem *reg;
+};
+
+/*
+ *   MD		EXTAL		PLL0	PLL1	PLL3
+ * 14 13 19	(MHz)		*1	*1
+ *---------------------------------------------------
+ * 0  0  0	15 x 1		x172/2	x208/2	x106
+ * 0  0  1	15 x 1		x172/2	x208/2	x88
+ * 0  1  0	20 x 1		x130/2	x156/2	x80
+ * 0  1  1	20 x 1		x130/2	x156/2	x66
+ * 1  0  0	26 / 2		x200/2	x240/2	x122
+ * 1  0  1	26 / 2		x200/2	x240/2	x102
+ * 1  1  0	30 / 2		x172/2	x208/2	x106
+ * 1  1  1	30 / 2		x172/2	x208/2	x88
+ *
+ * *1 :	Table 7.6 indicates VCO ouput (PLLx = VCO/2)
+ */
+#define CPG_PLL_CONFIG_INDEX(md)	((((md) & BIT(14)) >> 12) | \
+					 (((md) & BIT(13)) >> 12) | \
+					 (((md) & BIT(19)) >> 19))
+struct cpg_pll_config {
+	unsigned int extal_div;
+	unsigned int pll1_mult;
+	unsigned int pll3_mult;
+};
+
+static const struct cpg_pll_config cpg_pll_configs[8] = {
+	{ 1, 208, 106 }, { 1, 208,  88 }, { 1, 156,  80 }, { 1, 156,  66 },
+	{ 2, 240, 122 }, { 2, 240, 102 }, { 2, 208, 106 }, { 2, 208,  88 },
+};
+
+/* SDHI divisors */
+static const struct clk_div_table cpg_sdh_div_table[] = {
+	{  0,  2 }, {  1,  3 }, {  2,  4 }, {  3,  6 },
+	{  4,  8 }, {  5, 12 }, {  6, 16 }, {  7, 18 },
+	{  8, 24 }, { 10, 36 }, { 11, 48 }, {  0,  0 },
+};
+
+static const struct clk_div_table cpg_sd01_div_table[] = {
+	{  5, 12 }, {  6, 16 }, {  7, 18 }, {  8, 24 },
+	{ 10, 36 }, { 11, 48 }, { 12, 10 }, {  0,  0 },
+};
+
+static u32 cpg_mode __initdata;
+
+#define CPG_SDCKCR			0x00000074
+
+static void __init r8a7790_cpg_clocks_init(struct device_node *np)
+{
+	const struct cpg_pll_config *config;
+	struct r8a7790_cpg *cpg;
+	struct clk **clks;
+	unsigned int i;
+
+	cpg = kzalloc(sizeof(*cpg), GFP_KERNEL);
+	clks = kzalloc(CPG_NUM_CLOCKS * sizeof(*clks), GFP_KERNEL);
+	if (cpg == NULL || clks == NULL) {
+		kfree(clks);
+		kfree(cpg);
+		pr_err("%s: failed to allocate cpg\n", __func__);
+		return;
+	}
+
+	spin_lock_init(&cpg->lock);
+
+	cpg->data.clks = clks;
+	cpg->data.clk_num = CPG_NUM_CLOCKS;
+
+	cpg->reg = of_iomap(np, 0);
+	if (WARN_ON(cpg->reg == NULL)) {
+		kfree(cpg->data.clks);
+		kfree(cpg);
+		return;
+	}
+
+	config = &cpg_pll_configs[CPG_PLL_CONFIG_INDEX(cpg_mode)];
+
+	for (i = 0; i < CPG_NUM_CLOCKS; ++i) {
+		const struct clk_div_table *table = NULL;
+		const char *parent_name = "main";
+		const char *name;
+		unsigned int shift;
+		unsigned int mult = 1;
+		unsigned int div = 1;
+		struct clk *clk;
+
+		of_property_read_string_index(np, "clock-output-names", i,
+					      &name);
+
+		switch (i) {
+		case R8A7790_CLK_MAIN:
+			parent_name = of_clk_get_parent_name(np, 0);
+			div = config->extal_div;
+			break;
+		case R8A7790_CLK_PLL1:
+			mult = config->pll1_mult / 2;
+			break;
+		case R8A7790_CLK_PLL3:
+			mult = config->pll3_mult;
+			break;
+		case R8A7790_CLK_LB:
+			div = cpg_mode & BIT(18) ? 36 : 24;
+			break;
+		case R8A7790_CLK_QSPI:
+			div = (cpg_mode & (BIT(3) | BIT(2) | BIT(1))) == BIT(2)
+			    ? 16 : 20;
+			break;
+		case R8A7790_CLK_SDH:
+			table = cpg_sdh_div_table;
+			shift = 8;
+			break;
+		case R8A7790_CLK_SD0:
+			table = cpg_sd01_div_table;
+			shift = 4;
+			break;
+		case R8A7790_CLK_SD1:
+			table = cpg_sd01_div_table;
+			shift = 0;
+			break;
+		}
+
+		if (!table)
+			clk = clk_register_fixed_factor(NULL, name, parent_name,
+							0, mult, div);
+		else
+			clk = clk_register_divider_table(NULL, name,
+							 parent_name, 0,
+							 cpg->reg + CPG_SDCKCR,
+							 shift, 4, 0, table,
+							 &cpg->lock);
+
+		if (IS_ERR(clk))
+			pr_err("%s: failed to register %s %s clock (%ld)\n",
+			       __func__, np->name, name, PTR_ERR(clk));
+	}
+
+	of_clk_add_provider(np, of_clk_src_onecell_get, &cpg->data);
+}
+CLK_OF_DECLARE(r8a7790_cpg_clks, "renesas,r8a7790-cpg-clocks",
+	       r8a7790_cpg_clocks_init);
+
+void __init r8a7790_clocks_init(u32 mode)
+{
+	cpg_mode = mode;
+
+	of_clk_init(NULL);
+}
diff --git a/include/dt-bindings/clock/r8a7790-clock.h b/include/dt-bindings/clock/r8a7790-clock.h
index 19f2b48..f0ed742 100644
--- a/include/dt-bindings/clock/r8a7790-clock.h
+++ b/include/dt-bindings/clock/r8a7790-clock.h
@@ -10,6 +10,16 @@ 
 #ifndef __DT_BINDINGS_CLOCK_R8A7790_H__
 #define __DT_BINDINGS_CLOCK_R8A7790_H__
 
+/* CPG */
+#define R8A7790_CLK_MAIN		0
+#define R8A7790_CLK_PLL1		1
+#define R8A7790_CLK_PLL3		2
+#define R8A7790_CLK_LB			3
+#define R8A7790_CLK_QSPI		4
+#define R8A7790_CLK_SDH			5
+#define R8A7790_CLK_SD0			6
+#define R8A7790_CLK_SD1			7
+
 /* MSTP1 */
 #define R8A7790_CLK_CMT0		20
 
diff --git a/include/linux/clk/shmobile.h b/include/linux/clk/shmobile.h
new file mode 100644
index 0000000..b090855
--- /dev/null
+++ b/include/linux/clk/shmobile.h
@@ -0,0 +1,19 @@ 
+/*
+ * Copyright 2013 Ideas On Board SPRL
+ *
+ * Contact: Laurent Pinchart <[e-mail]>
+ *
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __LINUX_CLK_SHMOBILE_H_
+#define __LINUX_CLK_SHMOBILE_H_
+
+#include <linux/types.h>
+
+void r8a7790_clocks_init(u32 mode);
+
+#endif