[PATCH 0/3] Renesas R8A7790 Common Clock Framework support

[PATCH 0/3] Renesas R8A7790 Common Clock Framework support

[Laurent Pinchart]

Hello,

This patch set adds CCF drivers for the clocks found in the Renesas R-Car
R8A7790 (H2) SoC.

The patches are pretty self-explanatory and described in their respective
commit message. The R8A7790 datasheet is unfortunately not publicly available.

The code is available in my git tree at

	git://linuxtv.org/pinchartl/fbdev.git clocks/ccf/r8a7790

The branch is based on a merge of renesas-devel-20131016, v3.12-rc7 and
multiarch drivers and clk-prepare patches I've posted earlier today. A full
DT description of the R8A7790 clocks is available in the clocks/ccf/lager
branch of the same repository (see "ARM: shmobile: r8a7790: Add clocks" and
"ARM: shmobile: r8a7790: Reference clocks" patches).

Laurent Pinchart (3):
  clk: shmobile: Add DIV6 clock support
  clk: shmobile: Add MSTP clock support
  clk: shmobile: Add R8A7790 clocks support

 .../bindings/clock/renesas,cpg-div6-clocks.txt     |  27 +++
 .../bindings/clock/renesas,cpg-mstp-clocks.txt     |  47 +++++
 .../bindings/clock/renesas,r8a7790-cpg-clocks.txt  |  26 +++
 drivers/clk/shmobile/Makefile                      |   6 +-
 drivers/clk/shmobile/clk-div6.c                    | 185 +++++++++++++++++
 drivers/clk/shmobile/clk-mstp.c                    | 229 +++++++++++++++++++++
 drivers/clk/shmobile/clk-r8a7790.c                 | 176 ++++++++++++++++
 include/dt-bindings/clock/r8a7790-clock.h          |  66 ++++++
 include/linux/clk/shmobile.h                       |  19 ++
 9 files changed, 780 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
 create mode 100644 Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
 create mode 100644 Documentation/devicetree/bindings/clock/renesas,r8a7790-cpg-clocks.txt
 create mode 100644 drivers/clk/shmobile/clk-div6.c
 create mode 100644 drivers/clk/shmobile/clk-mstp.c
 create mode 100644 drivers/clk/shmobile/clk-r8a7790.c
 create mode 100644 include/dt-bindings/clock/r8a7790-clock.h
 create mode 100644 include/linux/clk/shmobile.h

-- 
Regards,

Laurent Pinchart

[PATCH 1/3] clk: shmobile: Add DIV6 clock support

[Laurent Pinchart]

DIV6 clocks are divider gate clocks controlled through a single
register. The divider is expressed on 6 bits, hence the name, and can
take values from 1/1 to 1/64.

Those clocks are found on Renesas ARM SoCs.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 .../bindings/clock/renesas,cpg-div6-clocks.txt     |  27 +++
 drivers/clk/shmobile/Makefile                      |   4 +-
 drivers/clk/shmobile/clk-div6.c                    | 185 +++++++++++++++++++++
 3 files changed, 215 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
 create mode 100644 drivers/clk/shmobile/clk-div6.c

diff --git a/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt b/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
new file mode 100644
index 0000000..8036f3c
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
@@ -0,0 +1,27 @@
+* Renesas CPG DIV6 Clock
+
+The CPG DIV6 clocks are variable factor clocks provided by the Clock Pulse
+Generator (CPG). They clock input is divided by a configurable factor from 1
+to 64.
+
+Required Properties:
+
+  - compatible: Must be one of the following
+    - "renesas,r8a7790-div6-clock" for R8A7790 (R-Car H2) DIV6 clocks
+    - "renesas,cpg-div6-clock" for generic DIV6 clocks
+  - reg: Base address and length of the memory resource used by the DIV6 clock
+  - clocks: Reference to the parent clock
+  - #clock-cells: Must be 0
+  - clock-output-name: The name of the clock as a free-form string
+
+
+Example
+-------
+
+	sd2_clk: sd2_clk {
+		compatible = "renesas,r8a7790-div6-clock", "renesas,cpg-div6-clock";
+		reg = <0 0xe6150078 0 4>;
+		clocks = <&pll1_div2_clk>;
+		#clock-cells = <0>;
+		clock-output-names = "sd2";
+	};
diff --git a/drivers/clk/shmobile/Makefile b/drivers/clk/shmobile/Makefile
index 2240730..d5e14e0 100644
--- a/drivers/clk/shmobile/Makefile
+++ b/drivers/clk/shmobile/Makefile
@@ -1,3 +1,5 @@
-obj-$(CONFIG_ARCH_EMEV2)	+= clk-emev2.o
+obj-$(CONFIG_ARCH_EMEV2)		+= clk-emev2.o
+obj-$(CONFIG_ARCH_SHMOBILE_MULTI)	+= clk-div6.o
+
 # for emply built-in.o
 obj-n	:= dummy
diff --git a/drivers/clk/shmobile/clk-div6.c b/drivers/clk/shmobile/clk-div6.c
new file mode 100644
index 0000000..aac4756
--- /dev/null
+++ b/drivers/clk/shmobile/clk-div6.c
@@ -0,0 +1,185 @@
+/*
+ * r8a7790 Common Clock Framework support
+ *
+ * Copyright (C) 2013  Renesas Solutions Corp.
+ *
+ * Contact: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+ *
+ * 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/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+
+#define CPG_DIV6_CKSTP		BIT(8)
+#define CPG_DIV6_DIV(d)		((d) & 0x3f)
+#define CPG_DIV6_DIV_MASK	0x3f
+
+/**
+ * struct div6_clock - MSTP gating clock
+ * @hw: handle between common and hardware-specific interfaces
+ * @reg: IO-remapped register
+ * @div: divisor value (1-64)
+ */
+struct div6_clock {
+	struct clk_hw hw;
+	void __iomem *reg;
+	unsigned int div;
+};
+
+#define to_div6_clock(_hw) container_of(_hw, struct div6_clock, hw)
+
+static int cpg_div6_clock_enable(struct clk_hw *hw)
+{
+	struct div6_clock *clock = to_div6_clock(hw);
+
+	clk_writel(CPG_DIV6_DIV(clock->div - 1), clock->reg);
+
+	return 0;
+}
+
+static void cpg_div6_clock_disable(struct clk_hw *hw)
+{
+	struct div6_clock *clock = to_div6_clock(hw);
+
+	/* DIV6 clocks require the divisor field to be non-zero when stopping
+	 * the clock.
+	 */
+	clk_writel(CPG_DIV6_CKSTP | CPG_DIV6_DIV(CPG_DIV6_DIV_MASK),
+		   clock->reg);
+}
+
+static int cpg_div6_clock_is_enabled(struct clk_hw *hw)
+{
+	struct div6_clock *clock = to_div6_clock(hw);
+
+	return !(clk_readl(clock->reg) & CPG_DIV6_CKSTP);
+}
+
+static unsigned long cpg_div6_clock_recalc_rate(struct clk_hw *hw,
+						unsigned long parent_rate)
+{
+	struct div6_clock *clock = to_div6_clock(hw);
+	unsigned int div = (clk_readl(clock->reg) & CPG_DIV6_DIV_MASK) + 1;
+
+	return parent_rate / div;
+}
+
+static unsigned int cpg_div6_clock_calc_div(unsigned long rate,
+					    unsigned long parent_rate)
+{
+	unsigned int div;
+
+	div = DIV_ROUND_CLOSEST(parent_rate, rate);
+	return clamp_t(unsigned int, div, 1, 64);
+}
+
+static long cpg_div6_clock_round_rate(struct clk_hw *hw, unsigned long rate,
+				      unsigned long *parent_rate)
+{
+	unsigned int div = cpg_div6_clock_calc_div(rate, *parent_rate);
+
+	return *parent_rate / div;
+}
+
+static int cpg_div6_clock_set_rate(struct clk_hw *hw, unsigned long rate,
+				   unsigned long parent_rate)
+{
+	struct div6_clock *clock = to_div6_clock(hw);
+	unsigned int div = cpg_div6_clock_calc_div(rate, parent_rate);
+
+	clock->div = div;
+
+	/* Only program the new divisor if the clock isn't stopped. */
+	if (!(clk_readl(clock->reg) & CPG_DIV6_CKSTP))
+		clk_writel(CPG_DIV6_DIV(clock->div - 1), clock->reg);
+
+	return 0;
+}
+
+static const struct clk_ops cpg_div6_clock_ops = {
+	.enable = cpg_div6_clock_enable,
+	.disable = cpg_div6_clock_disable,
+	.is_enabled = cpg_div6_clock_is_enabled,
+	.recalc_rate = cpg_div6_clock_recalc_rate,
+	.round_rate = cpg_div6_clock_round_rate,
+	.set_rate = cpg_div6_clock_set_rate,
+};
+
+static void __init cpg_div6_clock_init(struct device_node *np)
+{
+	struct clk_init_data init;
+	struct div6_clock *clock;
+	const char *parent_name;
+	const char *name;
+	struct clk *clk;
+	int ret;
+
+	clock = kzalloc(sizeof(*clock), GFP_KERNEL);
+	if (!clock) {
+		pr_err("%s: failed to allocate %s DIV6 clock\n",
+		       __func__, np->name);
+		return;
+	}
+
+	/* Remap the clock register and read the divisor. Disabling the
+	 * clock overwrites the divisor, so we need to cache its value for the
+	 * enable operation.
+	 */
+	clock->reg = of_iomap(np, 0);
+	if (clock->reg == NULL) {
+		pr_err("%s: failed to map %s DIV6 clock register\n",
+		       __func__, np->name);
+		goto error;
+	}
+
+	clock->div = (clk_readl(clock->reg) & CPG_DIV6_DIV_MASK) + 1;
+
+	/* Parse the DT properties. */
+	ret = of_property_read_string(np, "clock-output-names", &name);
+	if (ret < 0) {
+		pr_err("%s: failed to get %s DIV6 clock output name\n",
+		       __func__, np->name);
+		goto error;
+	}
+
+	parent_name = of_clk_get_parent_name(np, 0);
+	if (parent_name == NULL) {
+		pr_err("%s: failed to get %s DIV6 clock parent name\n",
+		       __func__, np->name);
+		goto error;
+	}
+
+	/* Register the clock. */
+	init.name = name;
+	init.ops = &cpg_div6_clock_ops;
+	init.flags = CLK_IS_BASIC;
+	init.parent_names = &parent_name;
+	init.num_parents = 1;
+
+	clock->hw.init = &init;
+
+	clk = clk_register(NULL, &clock->hw);
+	if (IS_ERR(clk)) {
+		pr_err("%s: failed to register %s DIV6 clock (%ld)\n",
+		       __func__, np->name, PTR_ERR(clk));
+		goto error;
+	}
+
+	of_clk_add_provider(np, of_clk_src_simple_get, clk);
+
+	return;
+
+error:
+	if (clock->reg)
+		iounmap(clock->reg);
+	kfree(clock);
+}
+CLK_OF_DECLARE(cpg_div6_clk, "renesas,cpg-div6-clock", cpg_div6_clock_init);
-- 
1.8.1.5

Re: [PATCH 1/3] clk: shmobile: Add DIV6 clock support

[Kumar Gala]

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

> DIV6 clocks are divider gate clocks controlled through a single
> register. The divider is expressed on 6 bits, hence the name, and can
> take values from 1/1 to 1/64.
> 
> Those clocks are found on Renesas ARM SoCs.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> .../bindings/clock/renesas,cpg-div6-clocks.txt     |  27 +++
> drivers/clk/shmobile/Makefile                      |   4 +-
> drivers/clk/shmobile/clk-div6.c                    | 185 +++++++++++++++++++++
> 3 files changed, 215 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
> create mode 100644 drivers/clk/shmobile/clk-div6.c
> 
> diff --git a/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt b/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
> new file mode 100644
> index 0000000..8036f3c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
> @@ -0,0 +1,27 @@
> +* Renesas CPG DIV6 Clock
> +
> +The CPG DIV6 clocks are variable factor clocks provided by the Clock Pulse
> +Generator (CPG). They clock input is divided by a configurable factor from 1
> +to 64.
> +
> +Required Properties:
> +
> +  - compatible: Must be one of the following
> +    - "renesas,r8a7790-div6-clock" for R8A7790 (R-Car H2) DIV6 clocks
> +    - "renesas,cpg-div6-clock" for generic DIV6 clocks
> +  - reg: Base address and length of the memory resource used by the DIV6 clock
> +  - clocks: Reference to the parent clock
> +  - #clock-cells: Must be 0
> +  - clock-output-name: The name of the clock as a free-form string

Is this suppose to be 'clock-output-names' (missing a 's')?

> +
> +
> +Example
> +-------
> +
> +	sd2_clk: sd2_clk {

example should be sd2_clk@e6150078

> +		compatible = "renesas,r8a7790-div6-clock", "renesas,cpg-div6-clock";
> +		reg = <0 0xe6150078 0 4>;

curious, do you really have a second register at 0?

> +		clocks = <&pll1_div2_clk>;
> +		#clock-cells = <0>;
> +		clock-output-names = "sd2";
> +	};

- k


-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH 1/3] clk: shmobile: Add DIV6 clock support

[Laurent Pinchart]

Hi Kumar,

Thank you for the review.

On Tuesday 29 October 2013 18:33:00 Kumar Gala wrote:
> On Oct 29, 2013, at 9:55 AM, Laurent Pinchart wrote:
> > DIV6 clocks are divider gate clocks controlled through a single
> > register. The divider is expressed on 6 bits, hence the name, and can
> > take values from 1/1 to 1/64.
> > 
> > Those clocks are found on Renesas ARM SoCs.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > .../bindings/clock/renesas,cpg-div6-clocks.txt     |  27 +++
> > drivers/clk/shmobile/Makefile                      |   4 +-
> > drivers/clk/shmobile/clk-div6.c                    | 185 +++++++++++++++++
> > 3 files changed, 215 insertions(+), 1 deletion(-)
> > create mode 100644
> > Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
> > create mode 100644 drivers/clk/shmobile/clk-div6.c
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
> > b/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt new
> > file mode 100644
> > index 0000000..8036f3c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
> > @@ -0,0 +1,27 @@
> > +* Renesas CPG DIV6 Clock
> > +
> > +The CPG DIV6 clocks are variable factor clocks provided by the Clock
> > Pulse
> > +Generator (CPG). They clock input is divided by a configurable factor
> > from 1
> > +to 64.
> > +
> > +Required Properties:
> > +
> > +  - compatible: Must be one of the following
> > +    - "renesas,r8a7790-div6-clock" for R8A7790 (R-Car H2) DIV6 clocks
> > +    - "renesas,cpg-div6-clock" for generic DIV6 clocks
> > +  - reg: Base address and length of the memory resource used by the DIV6
> > clock
> > +  - clocks: Reference to the parent clock
> > +  - #clock-cells: Must be 0
> > +  - clock-output-name: The name of the clock as a free-form string
> 
> Is this suppose to be 'clock-output-names' (missing a 's')?

Yes it is, my bad. I'll fix that.

> > +
> > +
> > +Example
> > +-------
> > +
> > +	sd2_clk: sd2_clk {
> 
> example should be sd2_clk@e6150078

You're absolutely right.

> > +		compatible = "renesas,r8a7790-div6-clock", "renesas,cpg-div6-
clock";
> > +		reg = <0 0xe6150078 0 4>;
> 
> curious, do you really have a second register at 0?

No, but the r8a7790 has #address-cells and #size-cells set to 2.

> > +		clocks = <&pll1_div2_clk>;
> > +		#clock-cells = <0>;
> > +		clock-output-names = "sd2";
> > +	};

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] clk: shmobile: Add DIV6 clock support

[Kumar Gala]

On Oct 29, 2013, at 6:54 PM, Laurent Pinchart wrote:

> Hi Kumar,
> 
> Thank you for the review.
> 
> On Tuesday 29 October 2013 18:33:00 Kumar Gala wrote:
>> On Oct 29, 2013, at 9:55 AM, Laurent Pinchart wrote:
>>> DIV6 clocks are divider gate clocks controlled through a single
>>> register. The divider is expressed on 6 bits, hence the name, and can
>>> take values from 1/1 to 1/64.
>>> 
>>> Those clocks are found on Renesas ARM SoCs.
>>> 
>>> Signed-off-by: Laurent Pinchart
>>> <laurent.pinchart+renesas@ideasonboard.com>
>>> ---
>>> .../bindings/clock/renesas,cpg-div6-clocks.txt     |  27 +++
>>> drivers/clk/shmobile/Makefile                      |   4 +-
>>> drivers/clk/shmobile/clk-div6.c                    | 185 +++++++++++++++++
>>> 3 files changed, 215 insertions(+), 1 deletion(-)
>>> create mode 100644
>>> Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
>>> create mode 100644 drivers/clk/shmobile/clk-div6.c
>>> 
>>> diff --git
>>> a/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
>>> b/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt new
>>> file mode 100644
>>> index 0000000..8036f3c
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
>>> @@ -0,0 +1,27 @@
>>> +* Renesas CPG DIV6 Clock
>>> +
>>> +The CPG DIV6 clocks are variable factor clocks provided by the Clock
>>> Pulse
>>> +Generator (CPG). They clock input is divided by a configurable factor
>>> from 1
>>> +to 64.
>>> +
>>> +Required Properties:
>>> +
>>> +  - compatible: Must be one of the following
>>> +    - "renesas,r8a7790-div6-clock" for R8A7790 (R-Car H2) DIV6 clocks
>>> +    - "renesas,cpg-div6-clock" for generic DIV6 clocks
>>> +  - reg: Base address and length of the memory resource used by the DIV6
>>> clock
>>> +  - clocks: Reference to the parent clock
>>> +  - #clock-cells: Must be 0
>>> +  - clock-output-name: The name of the clock as a free-form string
>> 
>> Is this suppose to be 'clock-output-names' (missing a 's')?
> 
> Yes it is, my bad. I'll fix that.
> 
>>> +
>>> +
>>> +Example
>>> +-------
>>> +
>>> +	sd2_clk: sd2_clk {
>> 
>> example should be sd2_clk@e6150078
> 
> You're absolutely right.
> 
>>> +		compatible = "renesas,r8a7790-div6-clock", "renesas,cpg-div6-
> clock";
>>> +		reg = <0 0xe6150078 0 4>;
>> 
>> curious, do you really have a second register at 0?
> 
> No, but the r8a7790 has #address-cells and #size-cells set to 2.

Oh, duh.  Its been a while since I've seen a reg with cells being 2 ;)

> 
>>> +		clocks = <&pll1_div2_clk>;
>>> +		#clock-cells = <0>;
>>> +		clock-output-names = "sd2";
>>> +	};

- k

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH 2/3] clk: shmobile: Add MSTP clock support

[Laurent Pinchart]

MSTP clocks are gate clocks controlled through a register that handles
up to 32 clocks. The register is often sparsely populated.

Those clocks are found on Renesas ARM SoCs.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 .../bindings/clock/renesas,cpg-mstp-clocks.txt     |  47 +++++
 drivers/clk/shmobile/Makefile                      |   1 +
 drivers/clk/shmobile/clk-mstp.c                    | 229 +++++++++++++++++++++
 include/dt-bindings/clock/r8a7790-clock.h          |  56 +++++
 4 files changed, 333 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
 create mode 100644 drivers/clk/shmobile/clk-mstp.c
 create mode 100644 include/dt-bindings/clock/r8a7790-clock.h

diff --git a/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt b/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
new file mode 100644
index 0000000..b3a1ce0
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
@@ -0,0 +1,47 @@
+* Renesas R8A7790 MSTP Clocks
+
+The CPG can gate SoC device clocks. The gates are organized in groups of up to
+32 gates.
+
+This device tree binding describes a single 32 gate clocks group per node.
+Clocks are referenced by user nodes by the MSTP node phandle and the clock
+index in the group, from 0 to 31.
+
+Required Properties:
+
+  - compatible: Must be one of the following
+    - "renesas,r8a7790-mstp-clocks" for R8A7790 (R-Car H2) MSTP gate clocks
+    - "renesas,cpg-mstp-clock" for generic MSTP gate clocks
+  - reg: Base address and length of the memory resource used by the MSTP
+    clocks
+  - clocks: Reference to the parent clocks
+  - #clock-cells: Must be 1
+  - clock-output-names: The name of the clocks as free-form strings
+  - renesas,indices: Index of the gate clocks (0 to 31)
+
+The clocks, clock-output-names and renesas,indices properties contain one
+entry per gate. The MSTP groups are sparsely populated. Unimplemented gates
+must not be declared.
+
+
+Example
+-------
+
+	#include <dt-bindings/clock/r8a7790-clock.h>
+
+	mstp3_clks: mstp3_clks {
+		compatible = "renesas,r8a7790-mstp-clocks", "renesas,cpg-mstp-clocks";
+		reg = <0 0xe615013c 0 4>, <0 0xe6150048 0 4>;
+		clocks = <&cp_clk>, <&mmc1_clk>, <&sd3_clk>, <&sd2_clk>,
+			 <&cpg_clocks R8A7790_CLK_SD1>, <&cpg_clocks R8A7790_CLK_SD0>,
+			 <&mmc0_clk>;
+		#clock-cells = <1>;
+		clock-output-names =
+			"tpu0", "mmcif1", "sdhi3", "sdhi2",
+			 "sdhi1", "sdhi0", "mmcif0";
+		renesas,clock-indices = <
+			R8A7790_CLK_TPU0 R8A7790_CLK_MMCIF1 R8A7790_CLK_SDHI3
+			R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1 R8A7790_CLK_SDHI0
+			R8A7790_CLK_MMCIF0
+		>;
+	};
diff --git a/drivers/clk/shmobile/Makefile b/drivers/clk/shmobile/Makefile
index d5e14e0..3275c78 100644
--- a/drivers/clk/shmobile/Makefile
+++ b/drivers/clk/shmobile/Makefile
@@ -1,5 +1,6 @@
 obj-$(CONFIG_ARCH_EMEV2)		+= clk-emev2.o
 obj-$(CONFIG_ARCH_SHMOBILE_MULTI)	+= clk-div6.o
+obj-$(CONFIG_ARCH_SHMOBILE_MULTI)	+= clk-mstp.o
 
 # for emply built-in.o
 obj-n	:= dummy
diff --git a/drivers/clk/shmobile/clk-mstp.c b/drivers/clk/shmobile/clk-mstp.c
new file mode 100644
index 0000000..e576b60
--- /dev/null
+++ b/drivers/clk/shmobile/clk-mstp.c
@@ -0,0 +1,229 @@
+/*
+ * R-Car MSTP clocks
+ *
+ * Copyright (C) 2013 Ideas On Board SPRL
+ *
+ * Contact: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+ *
+ * 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/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/spinlock.h>
+
+/*
+ * MSTP clocks. We can't use standard gate clocks as we need to poll on the
+ * status register when enabling the clock.
+ */
+
+#define MSTP_MAX_CLOCKS		32
+
+/**
+ * struct mstp_clock_group - MSTP gating clocks group
+ *
+ * @data: clocks in this group
+ * @smstpcr: module stop control register
+ * @mstpsr: module stop status register (optional)
+ * @lock: protects writes to SMSTPCR
+ */
+struct mstp_clock_group {
+	struct clk_onecell_data data;
+	void __iomem *smstpcr;
+	void __iomem *mstpsr;
+	spinlock_t lock;
+};
+
+/**
+ * struct mstp_clock - MSTP gating clock
+ * @hw: handle between common and hardware-specific interfaces
+ * @bit_index: control bit index
+ * @group: MSTP clocks group
+ */
+struct mstp_clock {
+	struct clk_hw hw;
+	u32 bit_index;
+	struct mstp_clock_group *group;
+};
+
+#define to_mstp_clock(_hw) container_of(_hw, struct mstp_clock, hw)
+
+static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool enable)
+{
+	struct mstp_clock *clock = to_mstp_clock(hw);
+	struct mstp_clock_group *group = clock->group;
+	u32 bitmask = BIT(clock->bit_index);
+	unsigned long flags;
+	unsigned int i;
+	u32 value;
+
+	spin_lock_irqsave(&group->lock, flags);
+
+	value = clk_readl(group->smstpcr);
+	if (enable)
+		value &= ~bitmask;
+	else
+		value |= bitmask;
+	clk_writel(value, group->smstpcr);
+
+	spin_unlock_irqrestore(&group->lock, flags);
+
+	if (!enable || !group->mstpsr)
+		return 0;
+
+	for (i = 1000; i > 0; --i) {
+		if (!(clk_readl(group->mstpsr) & bitmask))
+			break;
+		cpu_relax();
+	}
+
+	if (!i) {
+		pr_err("%s: failed to enable %p[%d]\n", __func__,
+		       group->smstpcr, clock->bit_index);
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
+static int cpg_mstp_clock_enable(struct clk_hw *hw)
+{
+	return cpg_mstp_clock_endisable(hw, true);
+}
+
+static void cpg_mstp_clock_disable(struct clk_hw *hw)
+{
+	cpg_mstp_clock_endisable(hw, false);
+}
+
+static int cpg_mstp_clock_is_enabled(struct clk_hw *hw)
+{
+	struct mstp_clock *clock = to_mstp_clock(hw);
+	struct mstp_clock_group *group = clock->group;
+	u32 value;
+
+	if (group->mstpsr)
+		value = clk_readl(group->mstpsr);
+	else
+		value = clk_readl(group->smstpcr);
+
+	return !!(value & BIT(clock->bit_index));
+}
+
+static const struct clk_ops cpg_mstp_clock_ops = {
+	.enable = cpg_mstp_clock_enable,
+	.disable = cpg_mstp_clock_disable,
+	.is_enabled = cpg_mstp_clock_is_enabled,
+};
+
+static struct clk * __init
+cpg_mstp_clock_register(const char *name, const char *parent_name,
+			unsigned int index, struct mstp_clock_group *group)
+{
+	struct clk_init_data init;
+	struct mstp_clock *clock;
+	struct clk *clk;
+
+	clock = kzalloc(sizeof(*clock), GFP_KERNEL);
+	if (!clock) {
+		pr_err("%s: failed to allocate MSTP clock.\n", __func__);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	init.name = name;
+	init.ops = &cpg_mstp_clock_ops;
+	init.flags = CLK_IS_BASIC;
+	init.parent_names = &parent_name;
+	init.num_parents = 1;
+
+	clock->bit_index = index;
+	clock->group = group;
+	clock->hw.init = &init;
+
+	clk = clk_register(NULL, &clock->hw);
+
+	if (IS_ERR(clk))
+		kfree(clock);
+
+	return clk;
+}
+
+static void __init cpg_mstp_clocks_init(struct device_node *np)
+{
+	struct mstp_clock_group *group;
+	struct clk **clks;
+	unsigned int i;
+
+	group = kzalloc(sizeof(*group), GFP_KERNEL);
+	clks = kzalloc(MSTP_MAX_CLOCKS * sizeof(*clks), GFP_KERNEL);
+	if (group == NULL || clks == NULL) {
+		kfree(group);
+		kfree(clks);
+		pr_err("%s: failed to allocate group\n", __func__);
+		return;
+	}
+
+	spin_lock_init(&group->lock);
+	group->data.clks = clks;
+
+	group->smstpcr = of_iomap(np, 0);
+	group->mstpsr = of_iomap(np, 1);
+
+	if (group->smstpcr == NULL) {
+		pr_err("%s: failed to remap SMSTPCR\n", __func__);
+		kfree(group);
+		kfree(clks);
+		return;
+	}
+
+	for (i = 0; i < MSTP_MAX_CLOCKS; ++i) {
+		const char *parent_name;
+		const char *name;
+		u32 clkidx;
+		int ret;
+
+		/* Skip clocks with no name. */
+		ret = of_property_read_string_index(np, "clock-output-names",
+						    i, &name);
+		if (ret < 0 || strlen(name) == 0)
+			continue;
+
+		parent_name = of_clk_get_parent_name(np, i);
+		ret = of_property_read_u32_index(np, "renesas,clock-indices", i,
+						 &clkidx);
+		if (parent_name == NULL || ret < 0)
+			break;
+
+		if (clkidx >= MSTP_MAX_CLOCKS) {
+			pr_err("%s: invalid clock %s %s index %u)\n",
+			       __func__, np->name, name, clkidx);
+			continue;
+		}
+
+		clks[clkidx] = cpg_mstp_clock_register(name, parent_name, i,
+						       group);
+		if (!IS_ERR(clks[clkidx])) {
+			group->data.clk_num = max(group->data.clk_num, clkidx);
+			/*
+			 * Register a clkdev to let board code retrieve the
+			 * clock by name and register aliases for non-DT
+			 * devices.
+			 *
+			 * FIXME: Remove this when all devices that require a
+			 * clock will be instantiated from DT.
+			 */
+			clk_register_clkdev(clks[clkidx], name, NULL);
+		} else {
+			pr_err("%s: failed to register %s %s clock (%ld)\n",
+			       __func__, np->name, name, PTR_ERR(clks[clkidx]));
+		}
+	}
+
+	of_clk_add_provider(np, of_clk_src_onecell_get, &group->data);
+}
+CLK_OF_DECLARE(cpg_mstp_clks, "renesas,cpg-mstp-clocks", cpg_mstp_clocks_init);
diff --git a/include/dt-bindings/clock/r8a7790-clock.h b/include/dt-bindings/clock/r8a7790-clock.h
new file mode 100644
index 0000000..19f2b48
--- /dev/null
+++ b/include/dt-bindings/clock/r8a7790-clock.h
@@ -0,0 +1,56 @@
+/*
+ * Copyright 2013 Ideas On Board SPRL
+ *
+ * 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 __DT_BINDINGS_CLOCK_R8A7790_H__
+#define __DT_BINDINGS_CLOCK_R8A7790_H__
+
+/* MSTP1 */
+#define R8A7790_CLK_CMT0		20
+
+/* MSTP2 */
+#define R8A7790_CLK_SCIFA2		2
+#define R8A7790_CLK_SCIFA1		3
+#define R8A7790_CLK_SCIFA0		4
+#define R8A7790_CLK_SCIFB0		6
+#define R8A7790_CLK_SCIFB1		7
+#define R8A7790_CLK_SCIFB2		16
+
+/* MSTP3 */
+#define R8A7790_CLK_TPU0		4
+#define R8A7790_CLK_MMCIF1		5
+#define R8A7790_CLK_SDHI3		11
+#define R8A7790_CLK_SDHI2		12
+#define R8A7790_CLK_SDHI1		13
+#define R8A7790_CLK_SDHI0		14
+#define R8A7790_CLK_MMCIF0		15
+
+/* MSTP5 */
+#define R8A7790_CLK_THERMAL		22
+
+/* MSTP7 */
+#define R8A7790_CLK_HSCIF1		16
+#define R8A7790_CLK_HSCIF0		17
+#define R8A7790_CLK_SCIF1		20
+#define R8A7790_CLK_SCIF0		21
+#define R8A7790_CLK_DU2			22
+#define R8A7790_CLK_DU1			23
+#define R8A7790_CLK_DU0			24
+#define R8A7790_CLK_LVDS1		25
+#define R8A7790_CLK_LVDS0		26
+
+/* MSTP8 */
+#define R8A7790_CLK_ETHER		13
+
+/* MSTP9 */
+#define R8A7790_CLK_I2C3		28
+#define R8A7790_CLK_I2C2		29
+#define R8A7790_CLK_I2C1		30
+#define R8A7790_CLK_I2C0		31
+
+#endif /* __DT_BINDINGS_CLOCK_R8A7790_H__ */
-- 
1.8.1.5

Re: [PATCH 2/3] clk: shmobile: Add MSTP clock support

[Kumar Gala]

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

> MSTP clocks are gate clocks controlled through a register that handles
> up to 32 clocks. The register is often sparsely populated.
> 
> Those clocks are found on Renesas ARM SoCs.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> .../bindings/clock/renesas,cpg-mstp-clocks.txt     |  47 +++++
> drivers/clk/shmobile/Makefile                      |   1 +
> drivers/clk/shmobile/clk-mstp.c                    | 229 +++++++++++++++++++++
> include/dt-bindings/clock/r8a7790-clock.h          |  56 +++++
> 4 files changed, 333 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
> create mode 100644 drivers/clk/shmobile/clk-mstp.c
> create mode 100644 include/dt-bindings/clock/r8a7790-clock.h
> 
> diff --git a/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt b/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
> new file mode 100644
> index 0000000..b3a1ce0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
> @@ -0,0 +1,47 @@
> +* Renesas R8A7790 MSTP Clocks

can we spell out MSTP once in the heading?

> +
> +The CPG can gate SoC device clocks. The gates are organized in groups of up to
> +32 gates.
> +
> +This device tree binding describes a single 32 gate clocks group per node.
> +Clocks are referenced by user nodes by the MSTP node phandle and the clock
> +index in the group, from 0 to 31.
> +
> +Required Properties:
> +
> +  - compatible: Must be one of the following
> +    - "renesas,r8a7790-mstp-clocks" for R8A7790 (R-Car H2) MSTP gate clocks
> +    - "renesas,cpg-mstp-clock" for generic MSTP gate clocks
> +  - reg: Base address and length of the memory resource used by the MSTP
> +    clocks
> +  - clocks: Reference to the parent clocks
> +  - #clock-cells: Must be 1
> +  - clock-output-names: The name of the clocks as free-form strings
> +  - renesas,indices: Index of the gate clocks (0 to 31)

Index of the gate clock into what?

> +
> +The clocks, clock-output-names and renesas,indices properties contain one
> +entry per gate. The MSTP groups are sparsely populated. Unimplemented gates

per gate clock. (?)

> +must not be declared.
> +
> +
> +Example
> +-------
> +
> +	#include <dt-bindings/clock/r8a7790-clock.h>
> +
> +	mstp3_clks: mstp3_clks {
> +		compatible = "renesas,r8a7790-mstp-clocks", "renesas,cpg-mstp-clocks";
> +		reg = <0 0xe615013c 0 4>, <0 0xe6150048 0 4>;
> +		clocks = <&cp_clk>, <&mmc1_clk>, <&sd3_clk>, <&sd2_clk>,
> +			 <&cpg_clocks R8A7790_CLK_SD1>, <&cpg_clocks R8A7790_CLK_SD0>,
> +			 <&mmc0_clk>;
> +		#clock-cells = <1>;
> +		clock-output-names =
> +			"tpu0", "mmcif1", "sdhi3", "sdhi2",
> +			 "sdhi1", "sdhi0", "mmcif0";
> +		renesas,clock-indices = <
> +			R8A7790_CLK_TPU0 R8A7790_CLK_MMCIF1 R8A7790_CLK_SDHI3
> +			R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1 R8A7790_CLK_SDHI0
> +			R8A7790_CLK_MMCIF0
> +		>;
> +	};

- k

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH 2/3] clk: shmobile: Add MSTP clock support

[Laurent Pinchart]

Hi Kumar,

Thank you for the review.

On Tuesday 29 October 2013 18:36:06 Kumar Gala wrote:
> On Oct 29, 2013, at 9:55 AM, Laurent Pinchart wrote:
> > MSTP clocks are gate clocks controlled through a register that handles
> > up to 32 clocks. The register is often sparsely populated.
> > 
> > Those clocks are found on Renesas ARM SoCs.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > .../bindings/clock/renesas,cpg-mstp-clocks.txt     |  47 +++++
> > drivers/clk/shmobile/Makefile                      |   1 +
> > drivers/clk/shmobile/clk-mstp.c                    | 229 +++++++++++++++++
> > include/dt-bindings/clock/r8a7790-clock.h          |  56 +++++
> > 4 files changed, 333 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
> > create mode 100644 drivers/clk/shmobile/clk-mstp.c
> > create mode 100644 include/dt-bindings/clock/r8a7790-clock.h
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
> > b/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt new
> > file mode 100644
> > index 0000000..b3a1ce0
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
> > @@ -0,0 +1,47 @@
> > +* Renesas R8A7790 MSTP Clocks
> 
> can we spell out MSTP once in the heading?

Sure thing. It stands for Module Stop.

> > +
> > +The CPG can gate SoC device clocks. The gates are organized in groups of
> > up to
> > +32 gates.
> > +
> > +This device tree binding describes a single 32 gate clocks group per
> > node.
> > +Clocks are referenced by user nodes by the MSTP node phandle and the
> > clock
> > +index in the group, from 0 to 31.
> > +
> > +Required Properties:
> > +
> > +  - compatible: Must be one of the following
> > +    - "renesas,r8a7790-mstp-clocks" for R8A7790 (R-Car H2) MSTP gate
> > clocks
> > +    - "renesas,cpg-mstp-clock" for generic MSTP gate clocks
> > +  - reg: Base address and length of the memory resource used by the MSTP
> > +    clocks
> > +  - clocks: Reference to the parent clocks
> > +  - #clock-cells: Must be 1
> > +  - clock-output-names: The name of the clocks as free-form strings
> > +  - renesas,indices: Index of the gate clocks (0 to 31)
> 
> Index of the gate clock into what?

Into the 32 gates clock group. Groups have 32 entries with a 32-bit register 
that controls 32 gate clocks. The groups (and thus registers) are sparsly 
populated, this property lists the indices for the register bits corresponding 
to the clocks.

Would

  - renesas,indices: Indices of the gate clocks into the group (0 to 31)

be explicit enough ?

> > +
> > +The clocks, clock-output-names and renesas,indices properties contain one
> > +entry per gate. The MSTP groups are sparsely populated. Unimplemented
> > gates
> per gate clock. (?)

I'll change that.

> > +must not be declared.
> > +
> > +
> > +Example
> > +-------
> > +
> > +	#include <dt-bindings/clock/r8a7790-clock.h>
> > +
> > +	mstp3_clks: mstp3_clks {

mstp3_clks@e615013c I suppose.

> > +		compatible = "renesas,r8a7790-mstp-clocks", "renesas,cpg-mstp-
clocks";
> > +		reg = <0 0xe615013c 0 4>, <0 0xe6150048 0 4>;
> > +		clocks = <&cp_clk>, <&mmc1_clk>, <&sd3_clk>, <&sd2_clk>,
> > +			 <&cpg_clocks R8A7790_CLK_SD1>, <&cpg_clocks 
R8A7790_CLK_SD0>,
> > +			 <&mmc0_clk>;
> > +		#clock-cells = <1>;
> > +		clock-output-names =
> > +			"tpu0", "mmcif1", "sdhi3", "sdhi2",
> > +			 "sdhi1", "sdhi0", "mmcif0";
> > +		renesas,clock-indices = <
> > +			R8A7790_CLK_TPU0 R8A7790_CLK_MMCIF1 R8A7790_CLK_SDHI3
> > +			R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1 R8A7790_CLK_SDHI0
> > +			R8A7790_CLK_MMCIF0
> > +		>;
> > +	};

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] clk: shmobile: Add MSTP clock support

[Kumar Gala]

On Oct 29, 2013, at 7:06 PM, Laurent Pinchart wrote:

> Hi Kumar,
> 
> Thank you for the review.
> 
> On Tuesday 29 October 2013 18:36:06 Kumar Gala wrote:
>> On Oct 29, 2013, at 9:55 AM, Laurent Pinchart wrote:
>>> MSTP clocks are gate clocks controlled through a register that handles
>>> up to 32 clocks. The register is often sparsely populated.
>>> 
>>> Those clocks are found on Renesas ARM SoCs.
>>> 
>>> Signed-off-by: Laurent Pinchart
>>> <laurent.pinchart+renesas@ideasonboard.com>
>>> ---
>>> .../bindings/clock/renesas,cpg-mstp-clocks.txt     |  47 +++++
>>> drivers/clk/shmobile/Makefile                      |   1 +
>>> drivers/clk/shmobile/clk-mstp.c                    | 229 +++++++++++++++++
>>> include/dt-bindings/clock/r8a7790-clock.h          |  56 +++++
>>> 4 files changed, 333 insertions(+)
>>> create mode 100644
>>> Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
>>> create mode 100644 drivers/clk/shmobile/clk-mstp.c
>>> create mode 100644 include/dt-bindings/clock/r8a7790-clock.h
>>> 
>>> diff --git
>>> a/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
>>> b/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt new
>>> file mode 100644
>>> index 0000000..b3a1ce0
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
>>> @@ -0,0 +1,47 @@
>>> +* Renesas R8A7790 MSTP Clocks
>> 
>> can we spell out MSTP once in the heading?
> 
> Sure thing. It stands for Module Stop.
> 
>>> +
>>> +The CPG can gate SoC device clocks. The gates are organized in groups of
>>> up to
>>> +32 gates.
>>> +
>>> +This device tree binding describes a single 32 gate clocks group per
>>> node.
>>> +Clocks are referenced by user nodes by the MSTP node phandle and the
>>> clock
>>> +index in the group, from 0 to 31.
>>> +
>>> +Required Properties:
>>> +
>>> +  - compatible: Must be one of the following
>>> +    - "renesas,r8a7790-mstp-clocks" for R8A7790 (R-Car H2) MSTP gate
>>> clocks
>>> +    - "renesas,cpg-mstp-clock" for generic MSTP gate clocks
>>> +  - reg: Base address and length of the memory resource used by the MSTP
>>> +    clocks
>>> +  - clocks: Reference to the parent clocks
>>> +  - #clock-cells: Must be 1
>>> +  - clock-output-names: The name of the clocks as free-form strings
>>> +  - renesas,indices: Index of the gate clocks (0 to 31)
>> 
>> Index of the gate clock into what?
> 
> Into the 32 gates clock group. Groups have 32 entries with a 32-bit register 
> that controls 32 gate clocks. The groups (and thus registers) are sparsly 
> populated, this property lists the indices for the register bits corresponding 
> to the clocks.
> 
> Would
> 
>  - renesas,indices: Indices of the gate clocks into the group (0 to 31)
> 
> be explicit enough ?

I'm still confused.  As I look at the code, I'm not quite clear how renesas,indices relates to a register or register bits.

> 
>>> +
>>> +The clocks, clock-output-names and renesas,indices properties contain one
>>> +entry per gate. The MSTP groups are sparsely populated. Unimplemented
>>> gates
>> per gate clock. (?)
> 
> I'll change that.
> 
>>> +must not be declared.
>>> +
>>> +
>>> +Example
>>> +-------
>>> +
>>> +	#include <dt-bindings/clock/r8a7790-clock.h>
>>> +
>>> +	mstp3_clks: mstp3_clks {
> 
> mstp3_clks@e615013c I suppose.

yes, missed that
`
> 
>>> +		compatible = "renesas,r8a7790-mstp-clocks", "renesas,cpg-mstp-
> clocks";
>>> +		reg = <0 0xe615013c 0 4>, <0 0xe6150048 0 4>;
>>> +		clocks = <&cp_clk>, <&mmc1_clk>, <&sd3_clk>, <&sd2_clk>,
>>> +			 <&cpg_clocks R8A7790_CLK_SD1>, <&cpg_clocks 
> R8A7790_CLK_SD0>,
>>> +			 <&mmc0_clk>;
>>> +		#clock-cells = <1>;
>>> +		clock-output-names =
>>> +			"tpu0", "mmcif1", "sdhi3", "sdhi2",
>>> +			 "sdhi1", "sdhi0", "mmcif0";
>>> +		renesas,clock-indices = <
>>> +			R8A7790_CLK_TPU0 R8A7790_CLK_MMCIF1 R8A7790_CLK_SDHI3
>>> +			R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1 R8A7790_CLK_SDHI0
>>> +			R8A7790_CLK_MMCIF0
>>> +		>;
>>> +	};
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH 2/3] clk: shmobile: Add MSTP clock support

[Laurent Pinchart]

Hi Kumar,

On Tuesday 29 October 2013 19:19:35 Kumar Gala wrote:
> On Oct 29, 2013, at 7:06 PM, Laurent Pinchart wrote:
> > On Tuesday 29 October 2013 18:36:06 Kumar Gala wrote:
> >> On Oct 29, 2013, at 9:55 AM, Laurent Pinchart wrote:
> >>> MSTP clocks are gate clocks controlled through a register that handles
> >>> up to 32 clocks. The register is often sparsely populated.
> >>> 
> >>> Those clocks are found on Renesas ARM SoCs.
> >>> 
> >>> Signed-off-by: Laurent Pinchart
> >>> <laurent.pinchart+renesas@ideasonboard.com>
> >>> ---
> >>> .../bindings/clock/renesas,cpg-mstp-clocks.txt     |  47 +++++
> >>> drivers/clk/shmobile/Makefile                      |   1 +
> >>> drivers/clk/shmobile/clk-mstp.c                    | 229 +++++++++++++++
> >>> include/dt-bindings/clock/r8a7790-clock.h          |  56 +++++
> >>> 4 files changed, 333 insertions(+)
> >>> create mode 100644
> >>> Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
> >>> create mode 100644 drivers/clk/shmobile/clk-mstp.c
> >>> create mode 100644 include/dt-bindings/clock/r8a7790-clock.h
> >>> 
> >>> diff --git
> >>> a/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
> >>> b/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
> >>> new
> >>> file mode 100644
> >>> index 0000000..b3a1ce0
> >>> --- /dev/null
> >>> +++
> >>> b/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
> >>> @@ -0,0 +1,47 @@
> >>> +* Renesas R8A7790 MSTP Clocks
> >> 
> >> can we spell out MSTP once in the heading?
> > 
> > Sure thing. It stands for Module Stop.
> > 
> >>> +
> >>> +The CPG can gate SoC device clocks. The gates are organized in groups
> >>> of> up to
> >>> +32 gates.
> >>> +
> >>> +This device tree binding describes a single 32 gate clocks group per
> >>> node.
> >>> +Clocks are referenced by user nodes by the MSTP node phandle and the
> >>> clock
> >>> +index in the group, from 0 to 31.
> >>> +
> >>> +Required Properties:
> >>> +
> >>> +  - compatible: Must be one of the following
> >>> +    - "renesas,r8a7790-mstp-clocks" for R8A7790 (R-Car H2) MSTP gate
> >>> clocks
> >>> +    - "renesas,cpg-mstp-clock" for generic MSTP gate clocks
> >>> +  - reg: Base address and length of the memory resource used by the
> >>> MSTP
> >>> +    clocks
> >>> +  - clocks: Reference to the parent clocks
> >>> +  - #clock-cells: Must be 1
> >>> +  - clock-output-names: The name of the clocks as free-form strings
> >>> +  - renesas,indices: Index of the gate clocks (0 to 31)
> >> 
> >> Index of the gate clock into what?
> > 
> > Into the 32 gates clock group. Groups have 32 entries with a 32-bit
> > register that controls 32 gate clocks. The groups (and thus registers)
> > are sparsly populated, this property lists the indices for the register
> > bits corresponding to the clocks.
> > 
> > Would
> > 
> >  - renesas,indices: Indices of the gate clocks into the group (0 to 31)
> > 
> > be explicit enough ?
> 
> I'm still confused.  As I look at the code, I'm not quite clear how
> renesas,indices relates to a register or register bits.

OK, this probably means that the documentation isn't clear enough. Let me try 
to explain the situation, I'll then rephrase the bindings documentation.

Renesas SoCs have a large number of gate clocks, referred to as the MSTP 
clocks. Those gate clocks are controlled by a single bit each. From a control 
point of view, those bits are located in 32-bit registers referred to as the 
MSTP registers. Each MSTP register can thus control up to 32 gate clocks. As 
they are sparsely populated they usually control less than 32 gate clocks and 
have reserved bits for the clocks that are not present in the hardware. The 
reserved bits are randomly placed in the registers.

On the DT side, each MSTP register gets a DT node. The clocks handled by that 
register are listed in DT. The driver needs to map each clock to its bit index 
in the MSTP register. There are two main ways to do so:

- Specifying all 32 bits in the DT node, with 32 parent clocks and 32 clock 
output names. Reserved bits would get a dummy parent and an empty clock output 
name. The bit index would in that case be the clock index in the clock output 
names list with a one-to-one mapping between the clock cell and the clock 
index in the clock output names list.

- Specifying the available clocks only. No dummy parent clock and empty clock 
output name would be used. In that case there would be no one-to-one mapping 
between the clock cell and the corresponding clock index in the clock output 
names list. The mapping is then specified through the renesas,clock-indices 
property. Each entry in the property stores the bit number of the 
corresponding clock in the MSTP register.

Let's take MSTP3 of the R8A7790 as an example. The register controls the 
following clocks:

0		IIC2
1-3		Reserved
4		TPU0
5		MMC1
6-9		Reserved
10		IrDA
11		SDHI3
12		SDHI2
13		SDHI1
14		SDHI0
15		MMC0
16-17	Reserved
18		IIC0
19		PCIEC
20-22	Reserved
23		IIC1
24-27	Reserved
28		SSUSB
29		CMT1
30		USBDMAC0
31		USBDMAC1

The corresponding DT node would thus have the following properties

		clock-output-names =
			"iic2", "tpu0", "mmcif1", "irda", "sdhi3", "sdhi2", "sdhi1",
			"sdhi0", "mmcif0", "iic0", "pciec", "iic1", "ssusb", "cmt1",
			"usbdmac0", "usbcma1";
		renesas,clock-indices = <
				0 4 5 10 11 12 13 14 15 18 19 23 28 29 30 31
		>

The clock cell in clock users corresponds to the hardware bit index, not the 
index of the clock in the clock output names list. To make referencing clocks 
less error-prone, macros are defined for all bit indices in <dt-
bindings/clock/r8a7790-clock.h>. Using this macros in the provider, we get

		renesas,clock-indices = <
				R8A7790_CLK_IIC0 R8A7790_CLK_TPU0 R8A7790_CLK_MMCIF1
				R8A7790_CLK_IRDA R8A7790_CLK_SDHI3 R8A7790_CLK_SDHI2
				R8A7790_CLK_SDHI1 R8A7790_CLK_SDHI0 R8A7790_CLK_MMCIF0
				R8A7790_CLK_IIC0 R8A7790_CLK_PCIEC R8A7790_CLK_IIC1
				R8A7790_CLK_SSUSB R8A7790_CLK_CMT1 R8A7790_CLK_USBDMAC0
				R8A7790_CLK_USBDMAC1
		>;

Is the explanation clear enough ?

> >>> +
> >>> +The clocks, clock-output-names and renesas,indices properties contain
> >>> one
> >>> +entry per gate. The MSTP groups are sparsely populated. Unimplemented
> >>> gates
> >> 
> >> per gate clock. (?)
> > 
> > I'll change that.
> > 
> >>> +must not be declared.
> >>> +
> >>> +
> >>> +Example
> >>> +-------
> >>> +
> >>> +	#include <dt-bindings/clock/r8a7790-clock.h>
> >>> +
> >>> +	mstp3_clks: mstp3_clks {
> > 
> > mstp3_clks@e615013c I suppose.
> 
> yes, missed that
> `
> 
> >>> +		compatible = "renesas,r8a7790-mstp-clocks", "renesas,cpg-mstp-
> > 
> > clocks";
> > 
> >>> +		reg = <0 0xe615013c 0 4>, <0 0xe6150048 0 4>;
> >>> +		clocks = <&cp_clk>, <&mmc1_clk>, <&sd3_clk>, <&sd2_clk>,
> >>> +			 <&cpg_clocks R8A7790_CLK_SD1>, <&cpg_clocks
> > 
> > R8A7790_CLK_SD0>,
> > 
> >>> +			 <&mmc0_clk>;
> >>> +		#clock-cells = <1>;
> >>> +		clock-output-names =
> >>> +			"tpu0", "mmcif1", "sdhi3", "sdhi2",
> >>> +			 "sdhi1", "sdhi0", "mmcif0";
> >>> +		renesas,clock-indices = <
> >>> +			R8A7790_CLK_TPU0 R8A7790_CLK_MMCIF1 R8A7790_CLK_SDHI3
> >>> +			R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1 R8A7790_CLK_SDHI0
> >>> +			R8A7790_CLK_MMCIF0
> >>> +		>;
> >>> +	};
-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] clk: shmobile: Add MSTP clock support

[Simon Horman]

On Tue, Oct 29, 2013 at 03:55:10PM +0100, Laurent Pinchart wrote:
> MSTP clocks are gate clocks controlled through a register that handles
> up to 32 clocks. The register is often sparsely populated.
> 
> Those clocks are found on Renesas ARM SoCs.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  .../bindings/clock/renesas,cpg-mstp-clocks.txt     |  47 +++++
>  drivers/clk/shmobile/Makefile                      |   1 +
>  drivers/clk/shmobile/clk-mstp.c                    | 229 +++++++++++++++++++++
>  include/dt-bindings/clock/r8a7790-clock.h          |  56 +++++
>  4 files changed, 333 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
>  create mode 100644 drivers/clk/shmobile/clk-mstp.c
>  create mode 100644 include/dt-bindings/clock/r8a7790-clock.h

[snip]

> diff --git a/include/dt-bindings/clock/r8a7790-clock.h b/include/dt-bindings/clock/r8a7790-clock.h
> new file mode 100644
> index 0000000..19f2b48
> --- /dev/null
> +++ b/include/dt-bindings/clock/r8a7790-clock.h

I wonder if it would be best to put this in a separate patch
as it is SoC-specific.

> @@ -0,0 +1,56 @@
> +/*
> + * Copyright 2013 Ideas On Board SPRL
> + *
> + * 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 __DT_BINDINGS_CLOCK_R8A7790_H__
> +#define __DT_BINDINGS_CLOCK_R8A7790_H__
> +
> +/* MSTP1 */
> +#define R8A7790_CLK_CMT0		20

I believe that R8A7790_CLK_CMT0 should be 24.

> +
> +/* MSTP2 */
> +#define R8A7790_CLK_SCIFA2		2
> +#define R8A7790_CLK_SCIFA1		3
> +#define R8A7790_CLK_SCIFA0		4
> +#define R8A7790_CLK_SCIFB0		6
> +#define R8A7790_CLK_SCIFB1		7
> +#define R8A7790_CLK_SCIFB2		16
> +
> +/* MSTP3 */
> +#define R8A7790_CLK_TPU0		4
> +#define R8A7790_CLK_MMCIF1		5
> +#define R8A7790_CLK_SDHI3		11
> +#define R8A7790_CLK_SDHI2		12
> +#define R8A7790_CLK_SDHI1		13
> +#define R8A7790_CLK_SDHI0		14
> +#define R8A7790_CLK_MMCIF0		15
> +
> +/* MSTP5 */
> +#define R8A7790_CLK_THERMAL		22
> +
> +/* MSTP7 */
> +#define R8A7790_CLK_HSCIF1		16
> +#define R8A7790_CLK_HSCIF0		17
> +#define R8A7790_CLK_SCIF1		20
> +#define R8A7790_CLK_SCIF0		21
> +#define R8A7790_CLK_DU2			22
> +#define R8A7790_CLK_DU1			23
> +#define R8A7790_CLK_DU0			24
> +#define R8A7790_CLK_LVDS1		25
> +#define R8A7790_CLK_LVDS0		26
> +
> +/* MSTP8 */
> +#define R8A7790_CLK_ETHER		13
> +
> +/* MSTP9 */
> +#define R8A7790_CLK_I2C3		28
> +#define R8A7790_CLK_I2C2		29
> +#define R8A7790_CLK_I2C1		30
> +#define R8A7790_CLK_I2C0		31
> +
> +#endif /* __DT_BINDINGS_CLOCK_R8A7790_H__ */
> -- 
> 1.8.1.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/3] clk: shmobile: Add MSTP clock support

[Laurent Pinchart]

Hi Simon,

On Wednesday 06 November 2013 11:09:31 Simon Horman wrote:
> On Tue, Oct 29, 2013 at 03:55:10PM +0100, Laurent Pinchart wrote:
> > MSTP clocks are gate clocks controlled through a register that handles
> > up to 32 clocks. The register is often sparsely populated.
> > 
> > Those clocks are found on Renesas ARM SoCs.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  .../bindings/clock/renesas,cpg-mstp-clocks.txt     |  47 +++++
> >  drivers/clk/shmobile/Makefile                      |   1 +
> >  drivers/clk/shmobile/clk-mstp.c                    | 229 ++++++++++++++++
> >  include/dt-bindings/clock/r8a7790-clock.h          |  56 +++++
> >  4 files changed, 333 insertions(+)
> >  create mode 100644
> >  Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
> >  create mode 100644 drivers/clk/shmobile/clk-mstp.c
> >  create mode 100644 include/dt-bindings/clock/r8a7790-clock.h
> 
> [snip]
> 
> > diff --git a/include/dt-bindings/clock/r8a7790-clock.h
> > b/include/dt-bindings/clock/r8a7790-clock.h new file mode 100644
> > index 0000000..19f2b48
> > --- /dev/null
> > +++ b/include/dt-bindings/clock/r8a7790-clock.h
> 
> I wonder if it would be best to put this in a separate patch
> as it is SoC-specific.
> 
> > @@ -0,0 +1,56 @@
> > +/*
> > + * Copyright 2013 Ideas On Board SPRL
> > + *
> > + * 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 __DT_BINDINGS_CLOCK_R8A7790_H__
> > +#define __DT_BINDINGS_CLOCK_R8A7790_H__
> > +
> > +/* MSTP1 */
> > +#define R8A7790_CLK_CMT0		20
> 
> I believe that R8A7790_CLK_CMT0 should be 24.

Good catch, thank you.

> > +
> > +/* MSTP2 */
> > +#define R8A7790_CLK_SCIFA2		2
> > +#define R8A7790_CLK_SCIFA1		3
> > +#define R8A7790_CLK_SCIFA0		4
> > +#define R8A7790_CLK_SCIFB0		6
> > +#define R8A7790_CLK_SCIFB1		7
> > +#define R8A7790_CLK_SCIFB2		16
> > +
> > +/* MSTP3 */
> > +#define R8A7790_CLK_TPU0		4
> > +#define R8A7790_CLK_MMCIF1		5
> > +#define R8A7790_CLK_SDHI3		11
> > +#define R8A7790_CLK_SDHI2		12
> > +#define R8A7790_CLK_SDHI1		13
> > +#define R8A7790_CLK_SDHI0		14
> > +#define R8A7790_CLK_MMCIF0		15
> > +
> > +/* MSTP5 */
> > +#define R8A7790_CLK_THERMAL		22
> > +
> > +/* MSTP7 */
> > +#define R8A7790_CLK_HSCIF1		16
> > +#define R8A7790_CLK_HSCIF0		17
> > +#define R8A7790_CLK_SCIF1		20
> > +#define R8A7790_CLK_SCIF0		21
> > +#define R8A7790_CLK_DU2			22
> > +#define R8A7790_CLK_DU1			23
> > +#define R8A7790_CLK_DU0			24
> > +#define R8A7790_CLK_LVDS1		25
> > +#define R8A7790_CLK_LVDS0		26
> > +
> > +/* MSTP8 */
> > +#define R8A7790_CLK_ETHER		13
> > +
> > +/* MSTP9 */
> > +#define R8A7790_CLK_I2C3		28
> > +#define R8A7790_CLK_I2C2		29
> > +#define R8A7790_CLK_I2C1		30
> > +#define R8A7790_CLK_I2C0		31
> > +
> > +#endif /* __DT_BINDINGS_CLOCK_R8A7790_H__ */
-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] clk: shmobile: Add MSTP clock support

[Magnus Damm]

Hi Laurent,

On Tue, Oct 29, 2013 at 11:55 PM, Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
> MSTP clocks are gate clocks controlled through a register that handles
> up to 32 clocks. The register is often sparsely populated.
>
> Those clocks are found on Renesas ARM SoCs.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  .../bindings/clock/renesas,cpg-mstp-clocks.txt     |  47 +++++
>  drivers/clk/shmobile/Makefile                      |   1 +
>  drivers/clk/shmobile/clk-mstp.c                    | 229 +++++++++++++++++++++
>  include/dt-bindings/clock/r8a7790-clock.h          |  56 +++++
>  4 files changed, 333 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
>  create mode 100644 drivers/clk/shmobile/clk-mstp.c

Thanks for this. The MSTP driver looks fine to me, but..

>  create mode 100644 include/dt-bindings/clock/r8a7790-clock.h

.. why do you bundle r8a7790-specific bits with the generic MSTP
driver in this patch?

It looks to me that you want these r8a7790 bits included with the
r8a7790 support code.

Cheers,

/ magnus

Re: [PATCH 2/3] clk: shmobile: Add MSTP clock support

[Laurent Pinchart]

Hi Magnus,

On Wednesday 06 November 2013 17:33:39 Magnus Damm wrote:
> On Tue, Oct 29, 2013 at 11:55 PM, Laurent Pinchart wrote:
> > MSTP clocks are gate clocks controlled through a register that handles
> > up to 32 clocks. The register is often sparsely populated.
> > 
> > Those clocks are found on Renesas ARM SoCs.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  .../bindings/clock/renesas,cpg-mstp-clocks.txt     |  47 +++++
> >  drivers/clk/shmobile/Makefile                      |   1 +
> >  drivers/clk/shmobile/clk-mstp.c                    | 229 ++++++++++++++++
> >  include/dt-bindings/clock/r8a7790-clock.h          |  56 +++++
> >  4 files changed, 333 insertions(+)
> >  create mode 100644
> >  Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
> >  create mode 100644 drivers/clk/shmobile/clk-mstp.c
> 
> Thanks for this. The MSTP driver looks fine to me, but..
> 
> >  create mode 100644 include/dt-bindings/clock/r8a7790-clock.h
> 
> .. why do you bundle r8a7790-specific bits with the generic MSTP
> driver in this patch?
> 
> It looks to me that you want these r8a7790 bits included with the
> r8a7790 support code.

My mistake, I'll fix that.

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

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 <laurent.pinchart+renesas@ideasonboard.com>
---
 .../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 <laurent.pinchart@ideasonboard.com>
+ *
+ * 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 <laurent.pinchart@ideasonboard.com>
+ *
+ * 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

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

[Kumar Gala]

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 <laurent.pinchart+renesas@ideasonboard.com>
> ---
> .../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 <galak@codeaurora.org>

- k



-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

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

[Magnus Damm]

On Tue, Oct 29, 2013 at 11:55 PM, Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> 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 <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  .../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 <laurent.pinchart@ideasonboard.com>
> + *
> + * 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

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

[Laurent Pinchart]

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
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  .../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 <laurent.pinchart@ideasonboard.com>
> > + *
> > + * 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.

-- 
Regards,

Laurent Pinchart

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

[Magnus Damm]

Hi Laurent,

On Wed, Nov 6, 2013 at 8:47 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> 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
>> > <laurent.pinchart+renesas@ideasonboard.com>
>> > ---

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

Spaghetti-O's? =) I agree that some special handling is needed.

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

Yeah, it seems that interface isn't such a good match. Thanks.

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

Thanks for listing these. I have come across another option:

Use of_update_property() and of_property_read_u32() to pass using a
property, see below.

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

Yeah, I too would be interested to hear what Mike thinks about this matter.

Please see below for a prototype patch on top of your code that shows
the driver side of my DT property proposal. The omitted SoC side
becomes slightly more complicated but that code can be shared between
multiple R-Car SoCs so I think it should be acceptable size-wise.

With this patch applied the interface between the driver and the SoC
code is a DT property (that needs documentation) and
of_clk_init(NULL). No more evil link time symbol dependencies between
arch/ and drivers/...

 drivers/clk/shmobile/clk-r8a7790.c |   15 ++++++---------
 include/linux/clk/shmobile.h       |   19 -------------------
 2 files changed, 6 insertions(+), 28 deletions(-)

--- 0018/drivers/clk/shmobile/clk-r8a7790.c
+++ work/drivers/clk/shmobile/clk-r8a7790.c     2013-11-06 16:59:59.000000000 +0
900
@@ -12,7 +12,6 @@

 #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>
@@ -70,8 +69,6 @@ static const struct clk_div_table cpg_sd
        { 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)
@ -80,6 +77,12 @@ static void __init r8a7790_cpg_clocks_in
        struct r8a7790_cpg *cpg;
        struct clk **clks;
        unsigned int i;
+       u32 cpg_mode;
+
+       if (of_property_read_u32(node, "cpg-mode", &cpg_mode)) {
+               pr_err("%s: failed to determine CPG mode\n", __func__);
+               return;
+       }

        cpg = kzalloc(sizeof(*cpg), GFP_KERNEL);
        clks = kzalloc(CPG_NUM_CLOCKS * sizeof(*clks), GFP_KERNEL);
@ -168,9 +171,3 @@ static void __init r8a7790_cpg_clocks_in
 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);
-}
--- 0018/include/linux/clk/shmobile.h
+++ /dev/null   2013-06-03 21:41:10.638032047 +0900
@@ -1,19 +0,0 @@
-/*
- * Copyright 2013 Ideas On Board SPRL
- *
- * Contact: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
- *
- * 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

Cheers,

/ magnus

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

[Laurent Pinchart]

Hi Magnus,

(CC'ing Grant Likely)

On Wednesday 06 November 2013 17:19:48 Magnus Damm wrote:
> On Wed, Nov 6, 2013 at 8:47 AM, Laurent Pinchart wrote:
> > 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
> >> > <laurent.pinchart+renesas@ideasonboard.com>
> >> > ---
> >> > 
> >> > +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.
> 
> Spaghetti-O's? =) I agree that some special handling is needed.
> 
> >> 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.
> 
> Yeah, it seems that interface isn't such a good match. Thanks.
> 
> > 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.
>
> Thanks for listing these. I have come across another option:
> 
> Use of_update_property() and of_property_read_u32() to pass using a
> property, see below.
> 
> > 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.
> 
> Yeah, I too would be interested to hear what Mike thinks about this matter.
> 
> Please see below for a prototype patch on top of your code that shows
> the driver side of my DT property proposal. The omitted SoC side
> becomes slightly more complicated but that code can be shared between
> multiple R-Car SoCs so I think it should be acceptable size-wise.
> 
> With this patch applied the interface between the driver and the SoC
> code is a DT property (that needs documentation) and
> of_clk_init(NULL). No more evil link time symbol dependencies between
> arch/ and drivers/...

This feels a bit like a DT abuse to me. I'm not strongly opposed to this 
approach, but I would need an ack from Mike and Grant before implementing it.

>  drivers/clk/shmobile/clk-r8a7790.c |   15 ++++++---------
>  include/linux/clk/shmobile.h       |   19 -------------------
>  2 files changed, 6 insertions(+), 28 deletions(-)
> 
> --- 0018/drivers/clk/shmobile/clk-r8a7790.c
> +++ work/drivers/clk/shmobile/clk-r8a7790.c     2013-11-06
> 16:59:59.000000000 +0 900
> @@ -12,7 +12,6 @@
> 
>  #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>
> @@ -70,8 +69,6 @@ static const struct clk_div_table cpg_sd
>         { 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)
> @ -80,6 +77,12 @@ static void __init r8a7790_cpg_clocks_in
>         struct r8a7790_cpg *cpg;
>         struct clk **clks;
>         unsigned int i;
> +       u32 cpg_mode;
> +
> +       if (of_property_read_u32(node, "cpg-mode", &cpg_mode)) {
> +               pr_err("%s: failed to determine CPG mode\n", __func__);
> +               return;
> +       }
> 
>         cpg = kzalloc(sizeof(*cpg), GFP_KERNEL);
>         clks = kzalloc(CPG_NUM_CLOCKS * sizeof(*clks), GFP_KERNEL);
> @ -168,9 +171,3 @@ static void __init r8a7790_cpg_clocks_in
>  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);
> -}
> --- 0018/include/linux/clk/shmobile.h
> +++ /dev/null   2013-06-03 21:41:10.638032047 +0900
> @@ -1,19 +0,0 @@
> -/*
> - * Copyright 2013 Ideas On Board SPRL
> - *
> - * Contact: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> - *
> - * 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

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

[Kuninori Morimoto]

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

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

[Laurent Pinchart]

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 ?

-- 
Regards,

Laurent Pinchart

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

[Kuninori Morimoto]

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

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

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

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

[Kuninori Morimoto]

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

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

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Kuninori Morimoto]

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

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

[Kuninori Morimoto]

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

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

[Laurent Pinchart]

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 ? :-)

-- 
Regards,

Laurent Pinchart

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

[Kuninori Morimoto]

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

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

[Laurent Pinchart]

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 ?

-- 
Regards,

Laurent Pinchart

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

[Kuninori Morimoto]

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

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

[Simon Horman]

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 <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  .../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 <laurent.pinchart@ideasonboard.com>
> + *
> + * 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 <laurent.pinchart@ideasonboard.com>
> + *
> + * 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Laurent Pinchart]

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
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  .../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 <laurent.pinchart@ideasonboard.com>
> > + *
> > + * 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 <laurent.pinchart@ideasonboard.com>
> > + *
> > + * 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

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

[Simon Horman]

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
> > > <laurent.pinchart+renesas@ideasonboard.com>
> > > ---
> > > 
> > >  .../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 <laurent.pinchart@ideasonboard.com>
> > > + *
> > > + * 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 <laurent.pinchart@ideasonboard.com>
> > > + *
> > > + * 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
>