[PATCH v2 0/6] Add Marvell berlin4ct clk support

[PATCH v2 0/6] Add Marvell berlin4ct clk support

[Jisheng Zhang]

Add berlin4ct clk driver. The berlin4ct SoC contains:

two kinds of PLL: normal PLL and AVPLL. The normal PLL support is done.
The AVPLL support is in TODO list.

two kinds of clk: normal clk and gate clk. The normal clk supports changing
divider, selecting clock source, disabling/enabling etc. The gate clk only
supports disabling/enabling. Both are supported in this series.

Since v1:
 - rebased on v4.4-rc1
 - refactor out common code from clk-berlin4ct.c
 - s/lock/berlin4ct_gateclk_lock
 - drop WARN_ON for an allocation failure
 - s/kzalloc/kcalloc
 - add necessary handling in error code path, such as free memory, iounmap etc.
 - initialize init.flags
 - drop vcodiv_berlin[] array


Jisheng Zhang (6):
  clk: berlin: add common pll driver
  clk: berlin: add common clk driver for newer SoCs
  clk: berlin: add common gateclk driver for newer SoCs
  clk: berlin: add clk support for berlin4ct
  dt-bindings: add binding for marvell berlin4ct SoC
  arm64: dts: berlin4ct: add pll and clock nodes

 .../bindings/clock/marvell,berlin4ct.txt           |  38 ++++
 arch/arm64/boot/dts/marvell/berlin4ct.dtsi         |  38 ++++
 drivers/clk/berlin/Makefile                        |   1 +
 drivers/clk/berlin/clk-berlin4ct.c                 |  97 ++++++++++
 drivers/clk/berlin/clk.c                           | 203 +++++++++++++++++++++
 drivers/clk/berlin/clk.h                           |  45 +++++
 drivers/clk/berlin/gate.c                          |  73 ++++++++
 drivers/clk/berlin/pll.c                           | 133 ++++++++++++++
 include/dt-bindings/clock/berlin4ct.h              |  56 ++++++
 9 files changed, 684 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/marvell,berlin4ct.txt
 create mode 100644 drivers/clk/berlin/clk-berlin4ct.c
 create mode 100644 drivers/clk/berlin/clk.c
 create mode 100644 drivers/clk/berlin/clk.h
 create mode 100644 drivers/clk/berlin/gate.c
 create mode 100644 drivers/clk/berlin/pll.c
 create mode 100644 include/dt-bindings/clock/berlin4ct.h

-- 
2.6.2

[PATCH v2 1/6] clk: berlin: add common pll driver

[Jisheng Zhang]

Add pll driver for Marvell SoCs newer than BG2, BG2CD, BG2Q.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/clk/berlin/Makefile |   1 +
 drivers/clk/berlin/pll.c    | 133 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 134 insertions(+)
 create mode 100644 drivers/clk/berlin/pll.c

diff --git a/drivers/clk/berlin/Makefile b/drivers/clk/berlin/Makefile
index 2a36ab7..eee42b0 100644
--- a/drivers/clk/berlin/Makefile
+++ b/drivers/clk/berlin/Makefile
@@ -1,4 +1,5 @@
 obj-y += berlin2-avpll.o berlin2-pll.o berlin2-div.o
+obj-y += pll.o
 obj-$(CONFIG_MACH_BERLIN_BG2)	+= bg2.o
 obj-$(CONFIG_MACH_BERLIN_BG2CD)	+= bg2.o
 obj-$(CONFIG_MACH_BERLIN_BG2Q)	+= bg2q.o
diff --git a/drivers/clk/berlin/pll.c b/drivers/clk/berlin/pll.c
new file mode 100644
index 0000000..435445e
--- /dev/null
+++ b/drivers/clk/berlin/pll.c
@@ -0,0 +1,133 @@
+/*
+ * Copyright (c) 2015 Marvell Technology Group Ltd.
+ *
+ * Author: Jisheng Zhang <jszhang@marvell.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/slab.h>
+
+#define PLL_CTRL0	0x0
+#define PLL_CTRL1	0x4
+#define PLL_CTRL2	0x8
+#define PLL_CTRL3	0xC
+#define PLL_CTRL4	0x10
+#define PLL_STATUS	0x14
+
+#define PLL_SOURCE_MAX	2
+
+struct berlin_pll {
+	struct clk_hw	hw;
+	void __iomem	*ctrl;
+	void __iomem	*bypass;
+	u8		bypass_shift;
+};
+
+#define to_berlin_pll(hw)       container_of(hw, struct berlin_pll, hw)
+
+static unsigned long berlin_pll_recalc_rate(struct clk_hw *hw,
+					    unsigned long parent_rate)
+{
+	u32 val, fbdiv, rfdiv, vcodivsel, bypass;
+	struct berlin_pll *pll = to_berlin_pll(hw);
+
+	bypass = readl_relaxed(pll->bypass);
+	if (bypass & (1 << pll->bypass_shift))
+		return parent_rate;
+
+	val = readl_relaxed(pll->ctrl + PLL_CTRL0);
+	fbdiv = (val >> 12) & 0x1FF;
+	rfdiv = (val >> 3) & 0x1FF;
+	val = readl_relaxed(pll->ctrl + PLL_CTRL1);
+	vcodivsel = (val >> 9) & 0x7;
+	return parent_rate * fbdiv * 4 / rfdiv /
+		(1 << vcodivsel);
+}
+
+static u8 berlin_pll_get_parent(struct clk_hw *hw)
+{
+	struct berlin_pll *pll = to_berlin_pll(hw);
+	u32 bypass = readl_relaxed(pll->bypass);
+
+	return !!(bypass & (1 << pll->bypass_shift));
+}
+
+static const struct clk_ops berlin_pll_ops = {
+	.recalc_rate	= berlin_pll_recalc_rate,
+	.get_parent	= berlin_pll_get_parent,
+};
+
+static void __init berlin_pll_setup(struct device_node *np)
+{
+	struct clk_init_data init;
+	struct berlin_pll *pll;
+	const char *parent_names[PLL_SOURCE_MAX];
+	struct clk *clk;
+	int ret, num_parents;
+	u8 bypass_shift;
+
+	num_parents = of_clk_get_parent_count(np);
+	if (num_parents <= 0 || num_parents > PLL_SOURCE_MAX)
+		return;
+
+	ret = of_property_read_u8(np, "bypass-shift", &bypass_shift);
+	if (ret)
+		return;
+
+	of_clk_parent_fill(np, parent_names, num_parents);
+
+	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
+	if (!pll)
+		return;
+
+	pll->ctrl = of_iomap(np, 0);
+	if (WARN_ON(!pll->ctrl))
+		goto err_iomap_ctrl;
+
+	pll->bypass = of_iomap(np, 1);
+	if (WARN_ON(!pll->bypass))
+		goto err_iomap_bypass;
+
+	init.name = np->name;
+	init.flags = 0;
+	init.ops = &berlin_pll_ops;
+	init.parent_names = parent_names;
+	init.num_parents = num_parents;
+
+	pll->hw.init = &init;
+	pll->bypass_shift = bypass_shift;
+
+	clk = clk_register(NULL, &pll->hw);
+	if (WARN_ON(IS_ERR(clk)))
+		goto err_clk_register;
+
+	ret = of_clk_add_provider(np, of_clk_src_simple_get, clk);
+	if (WARN_ON(ret))
+		goto err_clk_add;
+	return;
+
+err_clk_add:
+	clk_unregister(clk);
+err_clk_register:
+	iounmap(pll->bypass);
+err_iomap_bypass:
+	iounmap(pll->ctrl);
+err_iomap_ctrl:
+	kfree(pll);
+}
+CLK_OF_DECLARE(berlin_pll, "marvell,berlin-pll", berlin_pll_setup);
-- 
2.6.2

Re: [PATCH v2 1/6] clk: berlin: add common pll driver

[Sebastian Hesselbarth]

On 20.11.2015 09:42, Jisheng Zhang wrote:
> Add pll driver for Marvell SoCs newer than BG2, BG2CD, BG2Q.
> 
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>  drivers/clk/berlin/Makefile |   1 +
>  drivers/clk/berlin/pll.c    | 133 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 134 insertions(+)
>  create mode 100644 drivers/clk/berlin/pll.c
> 
> diff --git a/drivers/clk/berlin/Makefile b/drivers/clk/berlin/Makefile
> index 2a36ab7..eee42b0 100644
> --- a/drivers/clk/berlin/Makefile
> +++ b/drivers/clk/berlin/Makefile
> @@ -1,4 +1,5 @@
>  obj-y += berlin2-avpll.o berlin2-pll.o berlin2-div.o
> +obj-y += pll.o

Jisheng,

please keep the naming style of files as we already have,
e.g. at least name the files for this driver berlin4-pll.

Even better you find the differences and merge it with
the berlin2-pll driver.

In general, I am not going to Ack any Berlin clock drivers
that expose the clock tree in any way. We recently merged
the Berlin2 clock stuff to a common OF node, I am not going
through the same mess for BG4 again.

>  obj-$(CONFIG_MACH_BERLIN_BG2)	+= bg2.o
>  obj-$(CONFIG_MACH_BERLIN_BG2CD)	+= bg2.o
>  obj-$(CONFIG_MACH_BERLIN_BG2Q)	+= bg2q.o
> diff --git a/drivers/clk/berlin/pll.c b/drivers/clk/berlin/pll.c
> new file mode 100644
> index 0000000..435445e
> --- /dev/null
> +++ b/drivers/clk/berlin/pll.c
> @@ -0,0 +1,133 @@
> +/*
> + * Copyright (c) 2015 Marvell Technology Group Ltd.
> + *
> + * Author: Jisheng Zhang <jszhang@marvell.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/slab.h>
> +
> +#define PLL_CTRL0	0x0
> +#define PLL_CTRL1	0x4
> +#define PLL_CTRL2	0x8
> +#define PLL_CTRL3	0xC
> +#define PLL_CTRL4	0x10
> +#define PLL_STATUS	0x14
> +
> +#define PLL_SOURCE_MAX	2
> +
> +struct berlin_pll {
> +	struct clk_hw	hw;
> +	void __iomem	*ctrl;
> +	void __iomem	*bypass;
> +	u8		bypass_shift;
> +};
> +
> +#define to_berlin_pll(hw)       container_of(hw, struct berlin_pll, hw)
> +
> +static unsigned long berlin_pll_recalc_rate(struct clk_hw *hw,
> +					    unsigned long parent_rate)
> +{
> +	u32 val, fbdiv, rfdiv, vcodivsel, bypass;
> +	struct berlin_pll *pll = to_berlin_pll(hw);
> +
> +	bypass = readl_relaxed(pll->bypass);
> +	if (bypass & (1 << pll->bypass_shift))
> +		return parent_rate;

Bypass could be modelled as a ccf clock-mux:

REF ---+------------|\
       |    _____   | |-- OUT
       +---| PLL |--|/

Please reuse what is already available.

> +	val = readl_relaxed(pll->ctrl + PLL_CTRL0);
> +	fbdiv = (val >> 12) & 0x1FF;
> +	rfdiv = (val >> 3) & 0x1FF;

Please get rid of any magic numbers.

> +	val = readl_relaxed(pll->ctrl + PLL_CTRL1);
> +	vcodivsel = (val >> 9) & 0x7;
> +	return parent_rate * fbdiv * 4 / rfdiv /
> +		(1 << vcodivsel);

A comment at the top of recalc_rate how output frequency is
calculated would be great.

> +}
> +
> +static u8 berlin_pll_get_parent(struct clk_hw *hw)
> +{
> +	struct berlin_pll *pll = to_berlin_pll(hw);
> +	u32 bypass = readl_relaxed(pll->bypass);
> +
> +	return !!(bypass & (1 << pll->bypass_shift));
> +}
> +
> +static const struct clk_ops berlin_pll_ops = {
> +	.recalc_rate	= berlin_pll_recalc_rate,
> +	.get_parent	= berlin_pll_get_parent,
> +};
> +
> +static void __init berlin_pll_setup(struct device_node *np)
> +{
> +	struct clk_init_data init;
> +	struct berlin_pll *pll;
> +	const char *parent_names[PLL_SOURCE_MAX];
> +	struct clk *clk;
> +	int ret, num_parents;
> +	u8 bypass_shift;
> +
> +	num_parents = of_clk_get_parent_count(np);
> +	if (num_parents <= 0 || num_parents > PLL_SOURCE_MAX)
> +		return;
> +
> +	ret = of_property_read_u8(np, "bypass-shift", &bypass_shift);
> +	if (ret)
> +		return;

The name "bypass" implies you can either choose to output the PLL
generated clock or pass the parent clock, i.e. bypass the PLL.

How can you choose from two parents then?

> +	of_clk_parent_fill(np, parent_names, num_parents);
> +
> +	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
> +	if (!pll)
> +		return;
> +
> +	pll->ctrl = of_iomap(np, 0);
> +	if (WARN_ON(!pll->ctrl))
> +		goto err_iomap_ctrl;
> +
> +	pll->bypass = of_iomap(np, 1);
> +	if (WARN_ON(!pll->bypass))
> +		goto err_iomap_bypass;
> +
> +	init.name = np->name;
> +	init.flags = 0;
> +	init.ops = &berlin_pll_ops;
> +	init.parent_names = parent_names;
> +	init.num_parents = num_parents;
> +
> +	pll->hw.init = &init;
> +	pll->bypass_shift = bypass_shift;
> +
> +	clk = clk_register(NULL, &pll->hw);
> +	if (WARN_ON(IS_ERR(clk)))
> +		goto err_clk_register;
> +
> +	ret = of_clk_add_provider(np, of_clk_src_simple_get, clk);
> +	if (WARN_ON(ret))
> +		goto err_clk_add;
> +	return;
> +
> +err_clk_add:
> +	clk_unregister(clk);
> +err_clk_register:
> +	iounmap(pll->bypass);
> +err_iomap_bypass:
> +	iounmap(pll->ctrl);
> +err_iomap_ctrl:
> +	kfree(pll);
> +}
> +CLK_OF_DECLARE(berlin_pll, "marvell,berlin-pll", berlin_pll_setup);

Just to repeat: I will not Ack any clk driver that is exposed to DT
except a single bg4 clock complex node. Have a look at BG2x clock
drivers and rework bg4 to have the same structure.

Sebastian

[PATCH v2 2/6] clk: berlin: add common clk driver for newer SoCs

[Jisheng Zhang]

Add common clk driver for Marvell SoCs newer than BG2, BG2CD, BG2Q.
berlin_clk_setup() is provided to setup and register such kind of clks.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/clk/berlin/Makefile |   2 +-
 drivers/clk/berlin/clk.c    | 203 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/berlin/clk.h    |  33 +++++++
 3 files changed, 237 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/berlin/clk.c
 create mode 100644 drivers/clk/berlin/clk.h

diff --git a/drivers/clk/berlin/Makefile b/drivers/clk/berlin/Makefile
index eee42b0..ee2e09d 100644
--- a/drivers/clk/berlin/Makefile
+++ b/drivers/clk/berlin/Makefile
@@ -1,5 +1,5 @@
 obj-y += berlin2-avpll.o berlin2-pll.o berlin2-div.o
-obj-y += pll.o
+obj-y += pll.o clk.o
 obj-$(CONFIG_MACH_BERLIN_BG2)	+= bg2.o
 obj-$(CONFIG_MACH_BERLIN_BG2CD)	+= bg2.o
 obj-$(CONFIG_MACH_BERLIN_BG2Q)	+= bg2q.o
diff --git a/drivers/clk/berlin/clk.c b/drivers/clk/berlin/clk.c
new file mode 100644
index 0000000..70f2b9d
--- /dev/null
+++ b/drivers/clk/berlin/clk.c
@@ -0,0 +1,203 @@
+/*
+ * Copyright (c) 2015 Marvell Technology Group Ltd.
+ *
+ * Author: Jisheng Zhang <jszhang@marvell.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/slab.h>
+
+#include "clk.h"
+
+#define CLKEN		(1 << 0)
+#define CLKPLLSEL_MASK	7
+#define CLKPLLSEL_SHIFT	1
+#define CLKPLLSWITCH	(1 << 4)
+#define CLKSWITCH	(1 << 5)
+#define CLKD3SWITCH	(1 << 6)
+#define CLKSEL_MASK	7
+#define CLKSEL_SHIFT	7
+
+#define CLK_SOURCE_MAX	5
+
+struct berlin_clk {
+	struct clk_hw hw;
+	void __iomem *base;
+};
+
+#define to_berlin_clk(hw)	container_of(hw, struct berlin_clk, hw)
+
+static u8 clk_div[] = {1, 2, 4, 6, 8, 12, 1, 1};
+
+static unsigned long berlin_clk_recalc_rate(struct clk_hw *hw,
+					unsigned long parent_rate)
+{
+	u32 val, divider;
+	struct berlin_clk *clk = to_berlin_clk(hw);
+
+	val = readl_relaxed(clk->base);
+	if (val & CLKD3SWITCH)
+		divider = 3;
+	else {
+		if (val & CLKSWITCH) {
+			val >>= CLKSEL_SHIFT;
+			val &= CLKSEL_MASK;
+			divider = clk_div[val];
+		} else
+			divider = 1;
+	}
+
+	return parent_rate / divider;
+}
+
+static u8 berlin_clk_get_parent(struct clk_hw *hw)
+{
+	u32 val;
+	struct berlin_clk *clk = to_berlin_clk(hw);
+
+	val = readl_relaxed(clk->base);
+	if (val & CLKPLLSWITCH) {
+		val >>= CLKPLLSEL_SHIFT;
+		val &= CLKPLLSEL_MASK;
+		return val;
+	}
+
+	return 0;
+}
+
+static int berlin_clk_enable(struct clk_hw *hw)
+{
+	u32 val;
+	struct berlin_clk *clk = to_berlin_clk(hw);
+
+	val = readl_relaxed(clk->base);
+	val |= CLKEN;
+	writel_relaxed(val, clk->base);
+
+	return 0;
+}
+
+static void berlin_clk_disable(struct clk_hw *hw)
+{
+	u32 val;
+	struct berlin_clk *clk = to_berlin_clk(hw);
+
+	val = readl_relaxed(clk->base);
+	val &= ~CLKEN;
+	writel_relaxed(val, clk->base);
+}
+
+static int berlin_clk_is_enabled(struct clk_hw *hw)
+{
+	u32 val;
+	struct berlin_clk *clk = to_berlin_clk(hw);
+
+	val = readl_relaxed(clk->base);
+	val &= CLKEN;
+
+	return val ? 1 : 0;
+}
+
+static const struct clk_ops berlin_clk_ops = {
+	.recalc_rate	= berlin_clk_recalc_rate,
+	.get_parent	= berlin_clk_get_parent,
+	.enable		= berlin_clk_enable,
+	.disable	= berlin_clk_disable,
+	.is_enabled	= berlin_clk_is_enabled,
+};
+
+static struct clk * __init
+berlin_clk_register(const char *name, int num_parents,
+		    const char **parent_names, unsigned long flags,
+		    void __iomem *base)
+{
+	struct clk *clk;
+	struct berlin_clk *bclk;
+	struct clk_init_data init;
+
+	bclk = kzalloc(sizeof(*bclk), GFP_KERNEL);
+	if (!bclk)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.ops = &berlin_clk_ops;
+	init.parent_names = parent_names;
+	init.num_parents = num_parents;
+	init.flags = flags;
+
+	bclk->base = base;
+	bclk->hw.init = &init;
+
+	clk = clk_register(NULL, &bclk->hw);
+	if (IS_ERR(clk))
+		kfree(bclk);
+
+	return clk;
+}
+
+void __init berlin_clk_setup(struct device_node *np,
+			     const struct clk_desc *descs,
+			     struct clk_onecell_data *clk_data,
+			     int n)
+{
+	int i, ret, num_parents;
+	void __iomem *base;
+	struct clk **clks;
+	const char *parent_names[CLK_SOURCE_MAX];
+
+	num_parents = of_clk_get_parent_count(np);
+	if (num_parents <= 0 || num_parents > CLK_SOURCE_MAX)
+		return;
+
+	of_clk_parent_fill(np, parent_names, num_parents);
+
+	clks = kcalloc(n, sizeof(struct clk *), GFP_KERNEL);
+	if (!clks)
+		return;
+
+	base = of_iomap(np, 0);
+	if (WARN_ON(!base))
+		goto err_iomap;
+
+	for (i = 0; i < n; i++) {
+		struct clk *clk;
+
+		clk = berlin_clk_register(descs[i].name,
+				num_parents, parent_names,
+				descs[i].flags,
+				base + descs[i].offset);
+		if (WARN_ON(IS_ERR(clks[i])))
+			goto err_clk_register;
+		clks[i] = clk;
+	}
+
+	clk_data->clks = clks;
+	clk_data->clk_num = i;
+
+	ret = of_clk_add_provider(np, of_clk_src_onecell_get, clk_data);
+	if (WARN_ON(ret))
+		goto err_clk_register;
+	return;
+
+err_clk_register:
+	for (i = 0; i < n; i++)
+		clk_unregister(clks[i]);
+	iounmap(base);
+err_iomap:
+	kfree(clks);
+}
diff --git a/drivers/clk/berlin/clk.h b/drivers/clk/berlin/clk.h
new file mode 100644
index 0000000..5e5680e
--- /dev/null
+++ b/drivers/clk/berlin/clk.h
@@ -0,0 +1,33 @@
+/*
+ * Copyright (c) 2015 Marvell Technology Group Ltd.
+ *
+ * Author: Jisheng Zhang <jszhang@marvell.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __BERLIN_CLK_H
+#define __BERLIN_CLK_H
+
+struct clk_desc {
+	const char	*name;
+	u32		offset;
+	unsigned long	flags;
+};
+
+void __init berlin_clk_setup(struct device_node *np,
+			     const struct clk_desc *desc,
+			     struct clk_onecell_data *clk_data,
+			     int n);
+
+#endif
-- 
2.6.2

Re: [PATCH v2 2/6] clk: berlin: add common clk driver for newer SoCs

[Sebastian Hesselbarth]

On 20.11.2015 09:42, Jisheng Zhang wrote:
> Add common clk driver for Marvell SoCs newer than BG2, BG2CD, BG2Q.
> berlin_clk_setup() is provided to setup and register such kind of clks.
> 
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>  drivers/clk/berlin/Makefile |   2 +-
>  drivers/clk/berlin/clk.c    | 203 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/berlin/clk.h    |  33 +++++++
>  3 files changed, 237 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/clk/berlin/clk.c
>  create mode 100644 drivers/clk/berlin/clk.h
> 
> diff --git a/drivers/clk/berlin/Makefile b/drivers/clk/berlin/Makefile
> index eee42b0..ee2e09d 100644
> --- a/drivers/clk/berlin/Makefile
> +++ b/drivers/clk/berlin/Makefile
> @@ -1,5 +1,5 @@
>  obj-y += berlin2-avpll.o berlin2-pll.o berlin2-div.o
> -obj-y += pll.o
> +obj-y += pll.o clk.o

Same comment about the naming convention.

>  obj-$(CONFIG_MACH_BERLIN_BG2)	+= bg2.o
>  obj-$(CONFIG_MACH_BERLIN_BG2CD)	+= bg2.o
>  obj-$(CONFIG_MACH_BERLIN_BG2Q)	+= bg2q.o
> diff --git a/drivers/clk/berlin/clk.c b/drivers/clk/berlin/clk.c
> new file mode 100644
> index 0000000..70f2b9d
> --- /dev/null
> +++ b/drivers/clk/berlin/clk.c
> @@ -0,0 +1,203 @@
> +/*
> + * Copyright (c) 2015 Marvell Technology Group Ltd.
> + *
> + * Author: Jisheng Zhang <jszhang@marvell.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/slab.h>
> +
> +#include "clk.h"
> +
> +#define CLKEN		(1 << 0)

#define CLKEN	BIT(0)

> +#define CLKPLLSEL_MASK	7

Please use hex numbers for the mask.

> +#define CLKPLLSEL_SHIFT	1
> +#define CLKPLLSWITCH	(1 << 4)
> +#define CLKSWITCH	(1 << 5)
> +#define CLKD3SWITCH	(1 << 6)

BIT() again.

> +#define CLKSEL_MASK	7

Hex again.

> +#define CLKSEL_SHIFT	7
> +
> +#define CLK_SOURCE_MAX	5
> +
> +struct berlin_clk {
> +	struct clk_hw hw;
> +	void __iomem *base;
> +};
> +
> +#define to_berlin_clk(hw)	container_of(hw, struct berlin_clk, hw)
> +
> +static u8 clk_div[] = {1, 2, 4, 6, 8, 12, 1, 1};

Hmm, this pretty much looks like berlin2-div dividers...

> +static unsigned long berlin_clk_recalc_rate(struct clk_hw *hw,
> +					unsigned long parent_rate)
> +{
> +	u32 val, divider;
> +	struct berlin_clk *clk = to_berlin_clk(hw);
> +
> +	val = readl_relaxed(clk->base);
> +	if (val & CLKD3SWITCH)
> +		divider = 3;

and this looks like berlin2-div structure, doesn't it?

Again, please reuse what is already available.

Sebastian

> +	else {
> +		if (val & CLKSWITCH) {
> +			val >>= CLKSEL_SHIFT;
> +			val &= CLKSEL_MASK;
> +			divider = clk_div[val];
> +		} else
> +			divider = 1;
> +	}
> +
> +	return parent_rate / divider;
> +}
> +
> +static u8 berlin_clk_get_parent(struct clk_hw *hw)
> +{
> +	u32 val;
> +	struct berlin_clk *clk = to_berlin_clk(hw);
> +
> +	val = readl_relaxed(clk->base);
> +	if (val & CLKPLLSWITCH) {
> +		val >>= CLKPLLSEL_SHIFT;
> +		val &= CLKPLLSEL_MASK;
> +		return val;
> +	}
> +
> +	return 0;
> +}
> +
> +static int berlin_clk_enable(struct clk_hw *hw)
> +{
> +	u32 val;
> +	struct berlin_clk *clk = to_berlin_clk(hw);
> +
> +	val = readl_relaxed(clk->base);
> +	val |= CLKEN;
> +	writel_relaxed(val, clk->base);
> +
> +	return 0;
> +}
> +
> +static void berlin_clk_disable(struct clk_hw *hw)
> +{
> +	u32 val;
> +	struct berlin_clk *clk = to_berlin_clk(hw);
> +
> +	val = readl_relaxed(clk->base);
> +	val &= ~CLKEN;
> +	writel_relaxed(val, clk->base);
> +}
> +
> +static int berlin_clk_is_enabled(struct clk_hw *hw)
> +{
> +	u32 val;
> +	struct berlin_clk *clk = to_berlin_clk(hw);
> +
> +	val = readl_relaxed(clk->base);
> +	val &= CLKEN;
> +
> +	return val ? 1 : 0;
> +}
> +
> +static const struct clk_ops berlin_clk_ops = {
> +	.recalc_rate	= berlin_clk_recalc_rate,
> +	.get_parent	= berlin_clk_get_parent,
> +	.enable		= berlin_clk_enable,
> +	.disable	= berlin_clk_disable,
> +	.is_enabled	= berlin_clk_is_enabled,
> +};
> +
> +static struct clk * __init
> +berlin_clk_register(const char *name, int num_parents,
> +		    const char **parent_names, unsigned long flags,
> +		    void __iomem *base)
> +{
> +	struct clk *clk;
> +	struct berlin_clk *bclk;
> +	struct clk_init_data init;
> +
> +	bclk = kzalloc(sizeof(*bclk), GFP_KERNEL);
> +	if (!bclk)
> +		return ERR_PTR(-ENOMEM);
> +
> +	init.name = name;
> +	init.ops = &berlin_clk_ops;
> +	init.parent_names = parent_names;
> +	init.num_parents = num_parents;
> +	init.flags = flags;
> +
> +	bclk->base = base;
> +	bclk->hw.init = &init;
> +
> +	clk = clk_register(NULL, &bclk->hw);
> +	if (IS_ERR(clk))
> +		kfree(bclk);
> +
> +	return clk;
> +}
> +
> +void __init berlin_clk_setup(struct device_node *np,
> +			     const struct clk_desc *descs,
> +			     struct clk_onecell_data *clk_data,
> +			     int n)
> +{
> +	int i, ret, num_parents;
> +	void __iomem *base;
> +	struct clk **clks;
> +	const char *parent_names[CLK_SOURCE_MAX];
> +
> +	num_parents = of_clk_get_parent_count(np);
> +	if (num_parents <= 0 || num_parents > CLK_SOURCE_MAX)
> +		return;
> +
> +	of_clk_parent_fill(np, parent_names, num_parents);
> +
> +	clks = kcalloc(n, sizeof(struct clk *), GFP_KERNEL);
> +	if (!clks)
> +		return;
> +
> +	base = of_iomap(np, 0);
> +	if (WARN_ON(!base))
> +		goto err_iomap;
> +
> +	for (i = 0; i < n; i++) {
> +		struct clk *clk;
> +
> +		clk = berlin_clk_register(descs[i].name,
> +				num_parents, parent_names,
> +				descs[i].flags,
> +				base + descs[i].offset);
> +		if (WARN_ON(IS_ERR(clks[i])))
> +			goto err_clk_register;
> +		clks[i] = clk;
> +	}
> +
> +	clk_data->clks = clks;
> +	clk_data->clk_num = i;
> +
> +	ret = of_clk_add_provider(np, of_clk_src_onecell_get, clk_data);
> +	if (WARN_ON(ret))
> +		goto err_clk_register;
> +	return;
> +
> +err_clk_register:
> +	for (i = 0; i < n; i++)
> +		clk_unregister(clks[i]);
> +	iounmap(base);
> +err_iomap:
> +	kfree(clks);
> +}
> diff --git a/drivers/clk/berlin/clk.h b/drivers/clk/berlin/clk.h
> new file mode 100644
> index 0000000..5e5680e
> --- /dev/null
> +++ b/drivers/clk/berlin/clk.h
> @@ -0,0 +1,33 @@
> +/*
> + * Copyright (c) 2015 Marvell Technology Group Ltd.
> + *
> + * Author: Jisheng Zhang <jszhang@marvell.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __BERLIN_CLK_H
> +#define __BERLIN_CLK_H
> +
> +struct clk_desc {
> +	const char	*name;
> +	u32		offset;
> +	unsigned long	flags;
> +};
> +
> +void __init berlin_clk_setup(struct device_node *np,
> +			     const struct clk_desc *desc,
> +			     struct clk_onecell_data *clk_data,
> +			     int n);
> +
> +#endif
> 

[PATCH v2 3/6] clk: berlin: add common gateclk driver for newer SoCs

[Jisheng Zhang]

Add common gateclk driver for Marvell SoCs newer than BG2, BG2CD,
BG2Q. berlin_gateclk_setup() is provided to setup and register such
kind of clks.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/clk/berlin/Makefile |  2 +-
 drivers/clk/berlin/clk.h    | 12 ++++++++
 drivers/clk/berlin/gate.c   | 73 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 86 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/berlin/gate.c

diff --git a/drivers/clk/berlin/Makefile b/drivers/clk/berlin/Makefile
index ee2e09d..fc92151 100644
--- a/drivers/clk/berlin/Makefile
+++ b/drivers/clk/berlin/Makefile
@@ -1,5 +1,5 @@
 obj-y += berlin2-avpll.o berlin2-pll.o berlin2-div.o
-obj-y += pll.o clk.o
+obj-y += pll.o clk.o gate.o
 obj-$(CONFIG_MACH_BERLIN_BG2)	+= bg2.o
 obj-$(CONFIG_MACH_BERLIN_BG2CD)	+= bg2.o
 obj-$(CONFIG_MACH_BERLIN_BG2Q)	+= bg2q.o
diff --git a/drivers/clk/berlin/clk.h b/drivers/clk/berlin/clk.h
index 5e5680e..4d8af6c 100644
--- a/drivers/clk/berlin/clk.h
+++ b/drivers/clk/berlin/clk.h
@@ -25,9 +25,21 @@ struct clk_desc {
 	unsigned long	flags;
 };
 
+struct gateclk_desc {
+	const char	*name;
+	const char	*parent_name;
+	u8		bit_idx;
+	unsigned long	flags;
+};
+
 void __init berlin_clk_setup(struct device_node *np,
 			     const struct clk_desc *desc,
 			     struct clk_onecell_data *clk_data,
 			     int n);
 
+void __init berlin_gateclk_setup(struct device_node *np,
+				 const struct gateclk_desc *descs,
+				 struct clk_onecell_data *clk_data,
+				 int n);
+
 #endif
diff --git a/drivers/clk/berlin/gate.c b/drivers/clk/berlin/gate.c
new file mode 100644
index 0000000..35200d2
--- /dev/null
+++ b/drivers/clk/berlin/gate.c
@@ -0,0 +1,73 @@
+/*
+ * Copyright (c) 2015 Marvell Technology Group Ltd.
+ *
+ * Author: Jisheng Zhang <jszhang@marvell.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/slab.h>
+#include <linux/spinlock_types.h>
+
+#include "clk.h"
+
+static DEFINE_SPINLOCK(berlin_gateclk_lock);
+
+void __init berlin_gateclk_setup(struct device_node *np,
+				 const struct gateclk_desc *descs,
+				 struct clk_onecell_data *clk_data,
+				 int n)
+{
+	int i, ret;
+	void __iomem *base;
+	struct clk **clks;
+
+	clks = kcalloc(n, sizeof(struct clk *), GFP_KERNEL);
+	if (!clks)
+		return;
+
+	base = of_iomap(np, 0);
+	if (WARN_ON(!base))
+		goto err_iomap;
+
+	for (i = 0; i < n; i++) {
+		struct clk *clk;
+
+		clk = clk_register_gate(NULL, descs[i].name,
+				descs[i].parent_name,
+				descs[i].flags, base,
+				descs[i].bit_idx, 0,
+				&berlin_gateclk_lock);
+		if (WARN_ON(IS_ERR(clk)))
+			goto err_clk_register;
+		clks[i] = clk;
+	}
+
+	clk_data->clks = clks;
+	clk_data->clk_num = i;
+
+	ret = of_clk_add_provider(np, of_clk_src_onecell_get, clk_data);
+	if (WARN_ON(ret))
+		goto err_clk_register;
+	return;
+
+err_clk_register:
+	for (i = 0; i < n; i++)
+		clk_unregister(clks[i]);
+	iounmap(base);
+err_iomap:
+	kfree(clks);
+}
-- 
2.6.2

[PATCH v2 4/6] clk: berlin: add clk support for berlin4ct

[Jisheng Zhang]

This patch supports the gateclk and berlin-clk in berlin4ct SoC.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/clk/berlin/Makefile        |  2 +-
 drivers/clk/berlin/clk-berlin4ct.c | 97 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 98 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/berlin/clk-berlin4ct.c

diff --git a/drivers/clk/berlin/Makefile b/drivers/clk/berlin/Makefile
index fc92151..accfc3a 100644
--- a/drivers/clk/berlin/Makefile
+++ b/drivers/clk/berlin/Makefile
@@ -1,5 +1,5 @@
 obj-y += berlin2-avpll.o berlin2-pll.o berlin2-div.o
-obj-y += pll.o clk.o gate.o
+obj-y += pll.o clk.o gate.o clk-berlin4ct.o
 obj-$(CONFIG_MACH_BERLIN_BG2)	+= bg2.o
 obj-$(CONFIG_MACH_BERLIN_BG2CD)	+= bg2.o
 obj-$(CONFIG_MACH_BERLIN_BG2Q)	+= bg2q.o
diff --git a/drivers/clk/berlin/clk-berlin4ct.c b/drivers/clk/berlin/clk-berlin4ct.c
new file mode 100644
index 0000000..0d994a4
--- /dev/null
+++ b/drivers/clk/berlin/clk-berlin4ct.c
@@ -0,0 +1,97 @@
+/*
+ * Copyright (c) 2015 Marvell Technology Group Ltd.
+ *
+ * Author: Jisheng Zhang <jszhang@marvell.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/clk-provider.h>
+
+#include "clk.h"
+
+static struct clk_onecell_data gateclk_data;
+static struct clk_onecell_data clk_data;
+
+static const struct gateclk_desc berlin4ct_gates[] __initconst = {
+	{ "tspsysclk",		"perifsysclk",	0 },
+	{ "usb0coreclk",	"perifsysclk",	1 },
+	{ "zspsysclk",		"perifsysclk",	2 },
+	{ "sdiosysclk",		"perifsysclk",	3 },
+	{ "ethcoreclk",		"perifsysclk",	4 },
+	{ "pcie0sys",		"perifsysclk",	6 },
+	{ "sata0core",		"perifsysclk",	7 },
+	{ "nfcsysclk",		"perifsysclk",	8 },
+	{ "emmcsysclk",		"perifsysclk",	9 },
+	{ "ihb0sysclk",		"perifsysclk",	10 },
+};
+
+static void __init berlin4ct_gateclk_setup(struct device_node *np)
+{
+	int n = ARRAY_SIZE(berlin4ct_gates);
+
+	berlin_gateclk_setup(np, berlin4ct_gates, &gateclk_data, n);
+}
+CLK_OF_DECLARE(berlin4ct_gateclk, "marvell,berlin4ct-gateclk",
+	       berlin4ct_gateclk_setup);
+
+static const struct clk_desc berlin4ct_descs[] __initconst = {
+	{ "cpufastrefclk",	0x0 },
+	{ "memfastrefclk",	0x4 },
+	{ "cfgclk",		0x20,	CLK_IGNORE_UNUSED },
+	{ "perifsysclk",	0x24,	CLK_IGNORE_UNUSED },
+	{ "hbclk",		0x28 },
+	{ "atbclk",		0x2c },
+	{ "decoderclk",		0x40 },
+	{ "decoderm3clk",	0x44 },
+	{ "decoderpcubeclk",	0x48 },
+	{ "encoderclk",		0x4c },
+	{ "ovpcoreclk",		0x50 },
+	{ "gfx2dcoreclk",	0x60 },
+	{ "gfx3dcoreclk",	0x64 },
+	{ "gfx3dshclk",		0x68 },
+	{ "gfx3dsysclk",	0x6c },
+	{ "gfx2dsysclk",	0x70 },
+	{ "aviosysclk",		0x80 },
+	{ "vppsysclk",		0x84 },
+	{ "eddcclk",		0x88 },
+	{ "aviobiuclk",		0x8c },
+	{ "zspclk",		0xa0 },
+	{ "tspclk",		0xc0 },
+	{ "tsprefclk",		0xc4 },
+	{ "ndsclk",		0xc8 },
+	{ "nocsclk",		0xcc },
+	{ "apbcoreclk",		0xd0,	CLK_IGNORE_UNUSED },
+	{ "emmcclk",		0xe0 },
+	{ "sd0clk",		0xe4 },
+	{ "sd1clk",		0xe8 },
+	{ "dllmstrefclk",	0xec },
+	{ "gethrgmiiclk",	0xf0 },
+	{ "gethrgmiisysclk",	0xf4 },
+	{ "usim0clk",		0x100 },
+	{ "pcietestclk",	0x110 },
+	{ "usb2testclk",	0x120 },
+	{ "usb3testclk",	0x124 },
+	{ "usb3coreclk",	0x128 },
+	{ "nfceccclk",		0x130 },
+	{ "bcmclk",		0x140 },
+};
+
+static void __init berlin4ct_clk_setup(struct device_node *np)
+{
+	int n = ARRAY_SIZE(berlin4ct_descs);
+
+	berlin_clk_setup(np, berlin4ct_descs, &clk_data, n);
+}
+CLK_OF_DECLARE(berlin4ct_clk, "marvell,berlin4ct-clk",
+	       berlin4ct_clk_setup);
-- 
2.6.2

Re: [PATCH v2 4/6] clk: berlin: add clk support for berlin4ct

[Sebastian Hesselbarth]

On 20.11.2015 09:42, Jisheng Zhang wrote:
> This patch supports the gateclk and berlin-clk in berlin4ct SoC.
> 
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>  drivers/clk/berlin/Makefile        |  2 +-
>  drivers/clk/berlin/clk-berlin4ct.c | 97 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 98 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/clk/berlin/clk-berlin4ct.c
> 
> diff --git a/drivers/clk/berlin/Makefile b/drivers/clk/berlin/Makefile
> index fc92151..accfc3a 100644
> --- a/drivers/clk/berlin/Makefile
> +++ b/drivers/clk/berlin/Makefile
> @@ -1,5 +1,5 @@
>  obj-y += berlin2-avpll.o berlin2-pll.o berlin2-div.o
> -obj-y += pll.o clk.o gate.o
> +obj-y += pll.o clk.o gate.o clk-berlin4ct.o

This will always compile clk-berlin4ct unconditionally on bg2x too.

Also, keep the naming style.

Sebastian

>  obj-$(CONFIG_MACH_BERLIN_BG2)	+= bg2.o
>  obj-$(CONFIG_MACH_BERLIN_BG2CD)	+= bg2.o
>  obj-$(CONFIG_MACH_BERLIN_BG2Q)	+= bg2q.o
> diff --git a/drivers/clk/berlin/clk-berlin4ct.c b/drivers/clk/berlin/clk-berlin4ct.c
> new file mode 100644
> index 0000000..0d994a4
> --- /dev/null
> +++ b/drivers/clk/berlin/clk-berlin4ct.c
> @@ -0,0 +1,97 @@
> +/*
> + * Copyright (c) 2015 Marvell Technology Group Ltd.
> + *
> + * Author: Jisheng Zhang <jszhang@marvell.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/clk-provider.h>
> +
> +#include "clk.h"
> +
> +static struct clk_onecell_data gateclk_data;
> +static struct clk_onecell_data clk_data;
> +
> +static const struct gateclk_desc berlin4ct_gates[] __initconst = {
> +	{ "tspsysclk",		"perifsysclk",	0 },
> +	{ "usb0coreclk",	"perifsysclk",	1 },
> +	{ "zspsysclk",		"perifsysclk",	2 },
> +	{ "sdiosysclk",		"perifsysclk",	3 },
> +	{ "ethcoreclk",		"perifsysclk",	4 },
> +	{ "pcie0sys",		"perifsysclk",	6 },
> +	{ "sata0core",		"perifsysclk",	7 },
> +	{ "nfcsysclk",		"perifsysclk",	8 },
> +	{ "emmcsysclk",		"perifsysclk",	9 },
> +	{ "ihb0sysclk",		"perifsysclk",	10 },
> +};
> +
> +static void __init berlin4ct_gateclk_setup(struct device_node *np)
> +{
> +	int n = ARRAY_SIZE(berlin4ct_gates);
> +
> +	berlin_gateclk_setup(np, berlin4ct_gates, &gateclk_data, n);
> +}
> +CLK_OF_DECLARE(berlin4ct_gateclk, "marvell,berlin4ct-gateclk",
> +	       berlin4ct_gateclk_setup);
> +
> +static const struct clk_desc berlin4ct_descs[] __initconst = {
> +	{ "cpufastrefclk",	0x0 },
> +	{ "memfastrefclk",	0x4 },
> +	{ "cfgclk",		0x20,	CLK_IGNORE_UNUSED },
> +	{ "perifsysclk",	0x24,	CLK_IGNORE_UNUSED },
> +	{ "hbclk",		0x28 },
> +	{ "atbclk",		0x2c },
> +	{ "decoderclk",		0x40 },
> +	{ "decoderm3clk",	0x44 },
> +	{ "decoderpcubeclk",	0x48 },
> +	{ "encoderclk",		0x4c },
> +	{ "ovpcoreclk",		0x50 },
> +	{ "gfx2dcoreclk",	0x60 },
> +	{ "gfx3dcoreclk",	0x64 },
> +	{ "gfx3dshclk",		0x68 },
> +	{ "gfx3dsysclk",	0x6c },
> +	{ "gfx2dsysclk",	0x70 },
> +	{ "aviosysclk",		0x80 },
> +	{ "vppsysclk",		0x84 },
> +	{ "eddcclk",		0x88 },
> +	{ "aviobiuclk",		0x8c },
> +	{ "zspclk",		0xa0 },
> +	{ "tspclk",		0xc0 },
> +	{ "tsprefclk",		0xc4 },
> +	{ "ndsclk",		0xc8 },
> +	{ "nocsclk",		0xcc },
> +	{ "apbcoreclk",		0xd0,	CLK_IGNORE_UNUSED },
> +	{ "emmcclk",		0xe0 },
> +	{ "sd0clk",		0xe4 },
> +	{ "sd1clk",		0xe8 },
> +	{ "dllmstrefclk",	0xec },
> +	{ "gethrgmiiclk",	0xf0 },
> +	{ "gethrgmiisysclk",	0xf4 },
> +	{ "usim0clk",		0x100 },
> +	{ "pcietestclk",	0x110 },
> +	{ "usb2testclk",	0x120 },
> +	{ "usb3testclk",	0x124 },
> +	{ "usb3coreclk",	0x128 },
> +	{ "nfceccclk",		0x130 },
> +	{ "bcmclk",		0x140 },
> +};
> +
> +static void __init berlin4ct_clk_setup(struct device_node *np)
> +{
> +	int n = ARRAY_SIZE(berlin4ct_descs);
> +
> +	berlin_clk_setup(np, berlin4ct_descs, &clk_data, n);
> +}
> +CLK_OF_DECLARE(berlin4ct_clk, "marvell,berlin4ct-clk",
> +	       berlin4ct_clk_setup);
> 

Re: [PATCH v2 4/6] clk: berlin: add clk support for berlin4ct

[Jisheng Zhang]

On Fri, 20 Nov 2015 21:56:34 +0100
Sebastian Hesselbarth wrote:

> On 20.11.2015 09:42, Jisheng Zhang wrote:
> > This patch supports the gateclk and berlin-clk in berlin4ct SoC.
> > 
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> >  drivers/clk/berlin/Makefile        |  2 +-
> >  drivers/clk/berlin/clk-berlin4ct.c | 97 ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 98 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/clk/berlin/clk-berlin4ct.c
> > 
> > diff --git a/drivers/clk/berlin/Makefile b/drivers/clk/berlin/Makefile
> > index fc92151..accfc3a 100644
> > --- a/drivers/clk/berlin/Makefile
> > +++ b/drivers/clk/berlin/Makefile
> > @@ -1,5 +1,5 @@
> >  obj-y += berlin2-avpll.o berlin2-pll.o berlin2-div.o
> > -obj-y += pll.o clk.o gate.o
> > +obj-y += pll.o clk.o gate.o clk-berlin4ct.o  
> 
> This will always compile clk-berlin4ct unconditionally on bg2x too.

Indeed. this also always compile bg2x clk driver on BG4x too. Is it fine to
introduce config option? any suggestion?

> 
> Also, keep the naming style.

will do in v3

> 
> Sebastian
> 
> >  obj-$(CONFIG_MACH_BERLIN_BG2)	+= bg2.o
> >  obj-$(CONFIG_MACH_BERLIN_BG2CD)	+= bg2.o
> >  obj-$(CONFIG_MACH_BERLIN_BG2Q)	+= bg2q.o
> > diff --git a/drivers/clk/berlin/clk-berlin4ct.c b/drivers/clk/berlin/clk-berlin4ct.c
> > new file mode 100644
> > index 0000000..0d994a4
> > --- /dev/null
> > +++ b/drivers/clk/berlin/clk-berlin4ct.c
> > @@ -0,0 +1,97 @@
> > +/*
> > + * Copyright (c) 2015 Marvell Technology Group Ltd.
> > + *
> > + * Author: Jisheng Zhang <jszhang@marvell.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms and conditions of the GNU General Public License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> > + * more details.
> > + *
> > + * You should have received a copy of the GNU General Public License along with
> > + * this program.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <linux/clk-provider.h>
> > +
> > +#include "clk.h"
> > +
> > +static struct clk_onecell_data gateclk_data;
> > +static struct clk_onecell_data clk_data;
> > +
> > +static const struct gateclk_desc berlin4ct_gates[] __initconst = {
> > +	{ "tspsysclk",		"perifsysclk",	0 },
> > +	{ "usb0coreclk",	"perifsysclk",	1 },
> > +	{ "zspsysclk",		"perifsysclk",	2 },
> > +	{ "sdiosysclk",		"perifsysclk",	3 },
> > +	{ "ethcoreclk",		"perifsysclk",	4 },
> > +	{ "pcie0sys",		"perifsysclk",	6 },
> > +	{ "sata0core",		"perifsysclk",	7 },
> > +	{ "nfcsysclk",		"perifsysclk",	8 },
> > +	{ "emmcsysclk",		"perifsysclk",	9 },
> > +	{ "ihb0sysclk",		"perifsysclk",	10 },
> > +};
> > +
> > +static void __init berlin4ct_gateclk_setup(struct device_node *np)
> > +{
> > +	int n = ARRAY_SIZE(berlin4ct_gates);
> > +
> > +	berlin_gateclk_setup(np, berlin4ct_gates, &gateclk_data, n);
> > +}
> > +CLK_OF_DECLARE(berlin4ct_gateclk, "marvell,berlin4ct-gateclk",
> > +	       berlin4ct_gateclk_setup);
> > +
> > +static const struct clk_desc berlin4ct_descs[] __initconst = {
> > +	{ "cpufastrefclk",	0x0 },
> > +	{ "memfastrefclk",	0x4 },
> > +	{ "cfgclk",		0x20,	CLK_IGNORE_UNUSED },
> > +	{ "perifsysclk",	0x24,	CLK_IGNORE_UNUSED },
> > +	{ "hbclk",		0x28 },
> > +	{ "atbclk",		0x2c },
> > +	{ "decoderclk",		0x40 },
> > +	{ "decoderm3clk",	0x44 },
> > +	{ "decoderpcubeclk",	0x48 },
> > +	{ "encoderclk",		0x4c },
> > +	{ "ovpcoreclk",		0x50 },
> > +	{ "gfx2dcoreclk",	0x60 },
> > +	{ "gfx3dcoreclk",	0x64 },
> > +	{ "gfx3dshclk",		0x68 },
> > +	{ "gfx3dsysclk",	0x6c },
> > +	{ "gfx2dsysclk",	0x70 },
> > +	{ "aviosysclk",		0x80 },
> > +	{ "vppsysclk",		0x84 },
> > +	{ "eddcclk",		0x88 },
> > +	{ "aviobiuclk",		0x8c },
> > +	{ "zspclk",		0xa0 },
> > +	{ "tspclk",		0xc0 },
> > +	{ "tsprefclk",		0xc4 },
> > +	{ "ndsclk",		0xc8 },
> > +	{ "nocsclk",		0xcc },
> > +	{ "apbcoreclk",		0xd0,	CLK_IGNORE_UNUSED },
> > +	{ "emmcclk",		0xe0 },
> > +	{ "sd0clk",		0xe4 },
> > +	{ "sd1clk",		0xe8 },
> > +	{ "dllmstrefclk",	0xec },
> > +	{ "gethrgmiiclk",	0xf0 },
> > +	{ "gethrgmiisysclk",	0xf4 },
> > +	{ "usim0clk",		0x100 },
> > +	{ "pcietestclk",	0x110 },
> > +	{ "usb2testclk",	0x120 },
> > +	{ "usb3testclk",	0x124 },
> > +	{ "usb3coreclk",	0x128 },
> > +	{ "nfceccclk",		0x130 },
> > +	{ "bcmclk",		0x140 },
> > +};
> > +
> > +static void __init berlin4ct_clk_setup(struct device_node *np)
> > +{
> > +	int n = ARRAY_SIZE(berlin4ct_descs);
> > +
> > +	berlin_clk_setup(np, berlin4ct_descs, &clk_data, n);
> > +}
> > +CLK_OF_DECLARE(berlin4ct_clk, "marvell,berlin4ct-clk",
> > +	       berlin4ct_clk_setup);
> >   
> 

[PATCH v2 5/6] dt-bindings: add binding for marvell berlin4ct SoC

[Jisheng Zhang]

This adds a dt-binding include for Marvell berlin4ct clock IDs.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 .../bindings/clock/marvell,berlin4ct.txt           | 38 +++++++++++++++
 include/dt-bindings/clock/berlin4ct.h              | 56 ++++++++++++++++++++++
 2 files changed, 94 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/marvell,berlin4ct.txt
 create mode 100644 include/dt-bindings/clock/berlin4ct.h

diff --git a/Documentation/devicetree/bindings/clock/marvell,berlin4ct.txt b/Documentation/devicetree/bindings/clock/marvell,berlin4ct.txt
new file mode 100644
index 0000000..a489473
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/marvell,berlin4ct.txt
@@ -0,0 +1,38 @@
+* Marvell berlin4ct Clock Controllers
+
+This binding uses the common clock binding[1].
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+The berlin4ct clock subsystem generates and supplies clock to various
+controllers within the berlin4ct SoC. The berlin4ct contains 3 clock controller
+blocks: pll, gateclk, berlin-clk.
+
+Required Properties:
+
+- compatible: should be one of the following.
+  - "marvell,berlin-pll" - pll compatible
+  - "marvell,berlin4ct-clk" - berlin clk compatible
+  - "marvell,berlin4ct-gateclk" - gateclk compatible
+- reg: physical base address of the clock controller and length of memory mapped
+  region. For pll, the second reg defines the bypass register base address and
+  length of memory mapped region.
+- #clock-cells: for pll should 0, for gateclk and berlin clk should be 1.
+- #bypass-shift: the bypass bit in bypass register.
+
+Example:
+
+syspll: syspll {
+	compatible = "marvell,berlin-pll";
+	reg = <0xea0200 0x14>, <0xea0710 4>;
+	#clock-cells = <0>;
+	clocks = <&osc>;
+	bypass-shift = /bits/ 8 <0>;
+};
+
+clk: clk {
+	compatible = "marvell,berlin4ct-clk";
+	reg = <0xea0720 0x144>;
+	#clock-cells = <1>;
+	clocks = <&syspll>;
+};
diff --git a/include/dt-bindings/clock/berlin4ct.h b/include/dt-bindings/clock/berlin4ct.h
new file mode 100644
index 0000000..abd5873
--- /dev/null
+++ b/include/dt-bindings/clock/berlin4ct.h
@@ -0,0 +1,56 @@
+/*
+ * Berlin BG4CT clock tree IDs
+ */
+
+/* GATE CLK */
+#define GATECLK_TSPSYS		0
+#define GATECLK_USB0CORE	1
+#define GATECLK_ZSPSYS		2
+#define GATECLK_SDIOSYS		3
+#define GATECLK_ETHCORE		4
+#define GATECLK_PCIE0SYS	5
+#define GATECLK_SATA0CORE	6
+#define GATECLK_NFCSYS		7
+#define GATECLK_EMMCSYS		8
+#define GATECLK_IHB0SYS		9
+
+/* BERLIN CLK */
+#define CLK_CPUFASTREF		0
+#define CLK_MEMFASTREF		1
+#define CLK_CFG			2
+#define CLK_PERIFSYS		3
+#define CLK_HB			4
+#define CLK_ATB			5
+#define CLK_DECODER		6
+#define CLK_DECODERM3		7
+#define CLK_DECODERPCUBE	8
+#define CLK_ENCODER		9
+#define CLK_OVPCORE		10
+#define CLK_GFX2DCORE		11
+#define CLK_GFX3DCORE		12
+#define CLK_GFX3DSH		13
+#define CLK_GFX3DSYS		14
+#define CLK_GFX2DSYS		15
+#define CLK_AVIOSYS		16
+#define CLK_VPPSYS		17
+#define CLK_EDDC		18
+#define CLK_AVIOBIU		19
+#define CLK_ZSP			20
+#define CLK_TSP			21
+#define CLK_TSPREF		22
+#define CLK_NDS			23
+#define CLK_NOCS		24
+#define CLK_APBCORE		25
+#define CLK_EMMC		26
+#define CLK_SD0			27
+#define CLK_SD1			28
+#define CLK_DLLMSTREF		29
+#define CLK_GETHRGMII		30
+#define CLK_GETHRGMIISYS	31
+#define CLK_USIM0		32
+#define CLK_PCIETEST		33
+#define CLK_USB2TEST		34
+#define CLK_USB3TEST		35
+#define CLK_USB3CORE		36
+#define CLK_NFCECC		37
+#define CLK_BCM			38
-- 
2.6.2

Re: [PATCH v2 5/6] dt-bindings: add binding for marvell berlin4ct SoC

[Rob Herring]

On Fri, Nov 20, 2015 at 04:42:31PM +0800, Jisheng Zhang wrote:
> This adds a dt-binding include for Marvell berlin4ct clock IDs.
> 
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>

Acked-by: Rob Herring <robh@kernel.org>

One typo below though.

> +Required Properties:
> +
> +- compatible: should be one of the following.
> +  - "marvell,berlin-pll" - pll compatible
> +  - "marvell,berlin4ct-clk" - berlin clk compatible
> +  - "marvell,berlin4ct-gateclk" - gateclk compatible
> +- reg: physical base address of the clock controller and length of memory mapped
> +  region. For pll, the second reg defines the bypass register base address and
> +  length of memory mapped region.
> +- #clock-cells: for pll should 0, for gateclk and berlin clk should be 1.
> +- #bypass-shift: the bypass bit in bypass register.
     ^

This should be dropped.

> +
> +Example:
> +
> +syspll: syspll {
> +	compatible = "marvell,berlin-pll";
> +	reg = <0xea0200 0x14>, <0xea0710 4>;
> +	#clock-cells = <0>;
> +	clocks = <&osc>;
> +	bypass-shift = /bits/ 8 <0>;
> +};

[PATCH v2 6/6] arm64: dts: berlin4ct: add pll and clock nodes

[Jisheng Zhang]

Add syspll, mempll, cpupll, gateclk and berlin-clk nodes.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 arch/arm64/boot/dts/marvell/berlin4ct.dtsi | 38 ++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/arch/arm64/boot/dts/marvell/berlin4ct.dtsi b/arch/arm64/boot/dts/marvell/berlin4ct.dtsi
index a4a1876..808a997 100644
--- a/arch/arm64/boot/dts/marvell/berlin4ct.dtsi
+++ b/arch/arm64/boot/dts/marvell/berlin4ct.dtsi
@@ -42,6 +42,7 @@
  *     OTHER DEALINGS IN THE SOFTWARE.
  */
 
+#include <dt-bindings/clock/berlin4ct.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 
 / {
@@ -135,6 +136,22 @@
 			interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
 		};
 
+		cpupll: cpupll {
+			compatible = "marvell,berlin-pll";
+			reg = <0x922000 0x14>, <0xea0710 4>;
+			#clock-cells = <0>;
+			clocks = <&osc>, <&clk CLK_CPUFASTREF>;
+			bypass-shift = /bits/ 8 <2>;
+		};
+
+		mempll: mempll {
+			compatible = "marvell,berlin-pll";
+			reg = <0x940034 0x14>, <0xea0710 4>;
+			#clock-cells = <0>;
+			clocks = <&osc>, <&clk CLK_MEMFASTREF>;
+			bypass-shift = /bits/ 8 <1>;
+		};
+
 		apb@e80000 {
 			compatible = "simple-bus";
 			#address-cells = <1>;
@@ -225,6 +242,27 @@
 			};
 		};
 
+		syspll: syspll {
+			compatible = "marvell,berlin-pll";
+			reg = <0xea0200 0x14>, <0xea0710 4>;
+			#clock-cells = <0>;
+			clocks = <&osc>;
+			bypass-shift = /bits/ 8 <0>;
+		};
+
+		gateclk: gateclk {
+			compatible = "marvell,berlin4ct-gateclk";
+			reg = <0xea0700 4>;
+			#clock-cells = <1>;
+		};
+
+		clk: clk {
+			compatible = "marvell,berlin4ct-clk";
+			reg = <0xea0720 0x144>;
+			#clock-cells = <1>;
+			clocks = <&syspll>;
+		};
+
 		soc_pinctrl: pin-controller@ea8000 {
 			compatible = "marvell,berlin4ct-soc-pinctrl";
 			reg = <0xea8000 0x14>;
-- 
2.6.2

Re: [PATCH v2 6/6] arm64: dts: berlin4ct: add pll and clock nodes

[Sebastian Hesselbarth]

On 20.11.2015 09:42, Jisheng Zhang wrote:
> Add syspll, mempll, cpupll, gateclk and berlin-clk nodes.
> 
> Signed-off-by: Jisheng Zhang <jszhang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> ---
>  arch/arm64/boot/dts/marvell/berlin4ct.dtsi | 38 ++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/marvell/berlin4ct.dtsi b/arch/arm64/boot/dts/marvell/berlin4ct.dtsi
> index a4a1876..808a997 100644
> --- a/arch/arm64/boot/dts/marvell/berlin4ct.dtsi
> +++ b/arch/arm64/boot/dts/marvell/berlin4ct.dtsi
> @@ -42,6 +42,7 @@
>   *     OTHER DEALINGS IN THE SOFTWARE.
>   */
>  
> +#include <dt-bindings/clock/berlin4ct.h>
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>  
>  / {
> @@ -135,6 +136,22 @@
>  			interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
>  		};
>  
> +		cpupll: cpupll {
> +			compatible = "marvell,berlin-pll";
> +			reg = <0x922000 0x14>, <0xea0710 4>;
> +			#clock-cells = <0>;
> +			clocks = <&osc>, <&clk CLK_CPUFASTREF>;
> +			bypass-shift = /bits/ 8 <2>;
> +		};
> +
> +		mempll: mempll {
> +			compatible = "marvell,berlin-pll";
> +			reg = <0x940034 0x14>, <0xea0710 4>;

Whenever you see overlapping/repeating reg ranges, e.g. <0xea0710 4>
you can be sure you are not representing HW structure but driver
structure here.

Please merge clocks/gates/plls to a single clock complex node
and deal with the internals by using "simple-mfd" and "syscon" regmaps.

> +			#clock-cells = <0>;
> +			clocks = <&osc>, <&clk CLK_MEMFASTREF>;
> +			bypass-shift = /bits/ 8 <1>;
> +		};
> +
>  		apb@e80000 {
>  			compatible = "simple-bus";
>  			#address-cells = <1>;
> @@ -225,6 +242,27 @@
>  			};
>  		};
>  
> +		syspll: syspll {
> +			compatible = "marvell,berlin-pll";
> +			reg = <0xea0200 0x14>, <0xea0710 4>;
> +			#clock-cells = <0>;
> +			clocks = <&osc>;
> +			bypass-shift = /bits/ 8 <0>;
> +		};
> +
> +		gateclk: gateclk {
> +			compatible = "marvell,berlin4ct-gateclk";
> +			reg = <0xea0700 4>;
> +			#clock-cells = <1>;
> +		};
> +
> +		clk: clk {
> +			compatible = "marvell,berlin4ct-clk";
> +			reg = <0xea0720 0x144>;

Looking at the reg ranges, I'd say that they are all clock related
and pretty close to each other:

gateclk: reg = <0xea0700 4>;
bypass:  reg = <0xea0710 4>;
clk:     reg = <0xea0720 0x144>;

So, please just follow the OF/driver structure we already
have for Berlin2.

Sebastian

> +			#clock-cells = <1>;
> +			clocks = <&syspll>;
> +		};
> +
>  		soc_pinctrl: pin-controller@ea8000 {
>  			compatible = "marvell,berlin4ct-soc-pinctrl";
>  			reg = <0xea8000 0x14>;
> 

--
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 v2 6/6] arm64: dts: berlin4ct: add pll and clock nodes

[Jisheng Zhang]

Dear Sebastian,

On Fri, 20 Nov 2015 22:06:59 +0100
Sebastian Hesselbarth wrote:

> On 20.11.2015 09:42, Jisheng Zhang wrote:
> > Add syspll, mempll, cpupll, gateclk and berlin-clk nodes.
> > 
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> >  arch/arm64/boot/dts/marvell/berlin4ct.dtsi | 38 ++++++++++++++++++++++++++++++
> >  1 file changed, 38 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/marvell/berlin4ct.dtsi b/arch/arm64/boot/dts/marvell/berlin4ct.dtsi
> > index a4a1876..808a997 100644
> > --- a/arch/arm64/boot/dts/marvell/berlin4ct.dtsi
> > +++ b/arch/arm64/boot/dts/marvell/berlin4ct.dtsi
> > @@ -42,6 +42,7 @@
> >   *     OTHER DEALINGS IN THE SOFTWARE.
> >   */
> >  
> > +#include <dt-bindings/clock/berlin4ct.h>
> >  #include <dt-bindings/interrupt-controller/arm-gic.h>
> >  
> >  / {
> > @@ -135,6 +136,22 @@
> >  			interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
> >  		};
> >  
> > +		cpupll: cpupll {
> > +			compatible = "marvell,berlin-pll";
> > +			reg = <0x922000 0x14>, <0xea0710 4>;
> > +			#clock-cells = <0>;
> > +			clocks = <&osc>, <&clk CLK_CPUFASTREF>;
> > +			bypass-shift = /bits/ 8 <2>;
> > +		};
> > +
> > +		mempll: mempll {
> > +			compatible = "marvell,berlin-pll";
> > +			reg = <0x940034 0x14>, <0xea0710 4>;  
> 
> Whenever you see overlapping/repeating reg ranges, e.g. <0xea0710 4>
> you can be sure you are not representing HW structure but driver
> structure here.
> 
> Please merge clocks/gates/plls to a single clock complex node
> and deal with the internals by using "simple-mfd" and "syscon" regmaps.
> 
> > +			#clock-cells = <0>;
> > +			clocks = <&osc>, <&clk CLK_MEMFASTREF>;
> > +			bypass-shift = /bits/ 8 <1>;
> > +		};
> > +
> >  		apb@e80000 {
> >  			compatible = "simple-bus";
> >  			#address-cells = <1>;
> > @@ -225,6 +242,27 @@
> >  			};
> >  		};
> >  
> > +		syspll: syspll {
> > +			compatible = "marvell,berlin-pll";
> > +			reg = <0xea0200 0x14>, <0xea0710 4>;
> > +			#clock-cells = <0>;
> > +			clocks = <&osc>;
> > +			bypass-shift = /bits/ 8 <0>;
> > +		};
> > +
> > +		gateclk: gateclk {
> > +			compatible = "marvell,berlin4ct-gateclk";
> > +			reg = <0xea0700 4>;
> > +			#clock-cells = <1>;
> > +		};
> > +
> > +		clk: clk {
> > +			compatible = "marvell,berlin4ct-clk";
> > +			reg = <0xea0720 0x144>;  
> 
> Looking at the reg ranges, I'd say that they are all clock related
> and pretty close to each other:
> 
> gateclk: reg = <0xea0700 4>;
> bypass:  reg = <0xea0710 4>;
> clk:     reg = <0xea0720 0x144>;

Although these ranges sit close, but we should represent HW structure as you
said.

First of all, let me describe the clks/plls in BG4CT. BG4CT contains:

two kinds of PLL: normal PLL and AVPLL. These PLLs are put with their users
together. For example: mempll pll registers <0xf7940034, 0x14> is put together
with mem controller registers. AVPLL control registers are put with AV devices.
You can also check mempll, cpupll and syspll ranges:

cpupll: <0x922000 0x14>

mempll: <0x940034 0x14>

syspll: <0xea0200 0x14>


We have three normal PLLS: cpupll, mempll and syspll. All these three PLLs use
25MHZ osc as clocksource. These plls can be bypassed. when syspll is bypassed
the 25MHZ osc is directly output to syspllclk. When mempll/cpupll is bypassed,
its corresponding fastrefclk is directly output to ddrphyclk/cpuclk: 


       ---25MHZ osc----------|\
           ________            | |-- syspllclk
       ---| SYSPLL |---------|/



       ---cpufastrefclk------|\
           ________            | |-- cpuclk
       ---| CPUPLL |---------|/


       ---memfastrefclk------|\
           ________            | |-- ddrphyclk
       ---| MEMPLL |---------|/

NOTE: the fastrefclk is the so called normal clk below.



two kinds of clk: normal clk and gate clk. The normal clk supports changing
divider, selecting clock source, disabling/enabling etc. The gate clk only
supports disabling/enabling. normal clks use syspllclk as clocksource, while
gate clks use perifsysclk as clocksource.


So what's the representing HW structure in fact? Here is my proposal:

1. have mempll, cpupll and syspll node in dts

2. one gateclk node in dts for gateclks

3. one normalclk node in dts for normal clks

4. one ccf clock-mux for cpuclk, ddrphyclk and syspllclk.

what do you think?


>From another side, let's have a look at driver/clk/mvebu. As can be seen,
different clks register are close each other, for example, gateclk and coreclk
in arch/arm/boot/dts/armada-xp.dtsi.

And drivers/clk/sunxi, arch/arm/boot/dts/sun7i-a20.dtsi, the pll4, pll12, gt_clk
and ahb*, apb* etc... 

why these SoCs don't merge clocks/gates/plls to a single clock complex node? 
I think that's because they are representing real HW structure.

Thanks,
Jisheng


> 
> So, please just follow the OF/driver structure we already
> have for Berlin2.
> 
> Sebastian
> 
> > +			#clock-cells = <1>;
> > +			clocks = <&syspll>;
> > +		};
> > +
> >  		soc_pinctrl: pin-controller@ea8000 {
> >  			compatible = "marvell,berlin4ct-soc-pinctrl";
> >  			reg = <0xea8000 0x14>;
> >   
> 

Re: [PATCH v2 6/6] arm64: dts: berlin4ct: add pll and clock nodes

[Jisheng Zhang]

Dear all,

On Mon, 23 Nov 2015 15:21:58 +0800
Jisheng Zhang wrote:

> Dear Sebastian,
> 
> On Fri, 20 Nov 2015 22:06:59 +0100
> Sebastian Hesselbarth wrote:
> 
> > On 20.11.2015 09:42, Jisheng Zhang wrote:  
> > > Add syspll, mempll, cpupll, gateclk and berlin-clk nodes.
> > > 
> > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > > ---
> > >  arch/arm64/boot/dts/marvell/berlin4ct.dtsi | 38 ++++++++++++++++++++++++++++++
> > >  1 file changed, 38 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/marvell/berlin4ct.dtsi b/arch/arm64/boot/dts/marvell/berlin4ct.dtsi
> > > index a4a1876..808a997 100644
> > > --- a/arch/arm64/boot/dts/marvell/berlin4ct.dtsi
> > > +++ b/arch/arm64/boot/dts/marvell/berlin4ct.dtsi
> > > @@ -42,6 +42,7 @@
> > >   *     OTHER DEALINGS IN THE SOFTWARE.
> > >   */
> > >  
> > > +#include <dt-bindings/clock/berlin4ct.h>
> > >  #include <dt-bindings/interrupt-controller/arm-gic.h>
> > >  
> > >  / {
> > > @@ -135,6 +136,22 @@
> > >  			interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
> > >  		};
> > >  
> > > +		cpupll: cpupll {
> > > +			compatible = "marvell,berlin-pll";
> > > +			reg = <0x922000 0x14>, <0xea0710 4>;
> > > +			#clock-cells = <0>;
> > > +			clocks = <&osc>, <&clk CLK_CPUFASTREF>;
> > > +			bypass-shift = /bits/ 8 <2>;
> > > +		};
> > > +
> > > +		mempll: mempll {
> > > +			compatible = "marvell,berlin-pll";
> > > +			reg = <0x940034 0x14>, <0xea0710 4>;    
> > 
> > Whenever you see overlapping/repeating reg ranges, e.g. <0xea0710 4>
> > you can be sure you are not representing HW structure but driver
> > structure here.
> > 
> > Please merge clocks/gates/plls to a single clock complex node
> > and deal with the internals by using "simple-mfd" and "syscon" regmaps.
> >   
> > > +			#clock-cells = <0>;
> > > +			clocks = <&osc>, <&clk CLK_MEMFASTREF>;
> > > +			bypass-shift = /bits/ 8 <1>;
> > > +		};
> > > +
> > >  		apb@e80000 {
> > >  			compatible = "simple-bus";
> > >  			#address-cells = <1>;
> > > @@ -225,6 +242,27 @@
> > >  			};
> > >  		};
> > >  
> > > +		syspll: syspll {
> > > +			compatible = "marvell,berlin-pll";
> > > +			reg = <0xea0200 0x14>, <0xea0710 4>;
> > > +			#clock-cells = <0>;
> > > +			clocks = <&osc>;
> > > +			bypass-shift = /bits/ 8 <0>;
> > > +		};
> > > +
> > > +		gateclk: gateclk {
> > > +			compatible = "marvell,berlin4ct-gateclk";
> > > +			reg = <0xea0700 4>;
> > > +			#clock-cells = <1>;
> > > +		};
> > > +
> > > +		clk: clk {
> > > +			compatible = "marvell,berlin4ct-clk";
> > > +			reg = <0xea0720 0x144>;    
> > 
> > Looking at the reg ranges, I'd say that they are all clock related
> > and pretty close to each other:
> > 
> > gateclk: reg = <0xea0700 4>;
> > bypass:  reg = <0xea0710 4>;
> > clk:     reg = <0xea0720 0x144>;  
> 
> Although these ranges sit close, but we should represent HW structure as you
> said.
> 
> First of all, let me describe the clks/plls in BG4CT. BG4CT contains:
> 
> two kinds of PLL: normal PLL and AVPLL. These PLLs are put with their users
> together. For example: mempll pll registers <0xf7940034, 0x14> is put together
> with mem controller registers. AVPLL control registers are put with AV devices.
> You can also check mempll, cpupll and syspll ranges:
> 
> cpupll: <0x922000 0x14>
> 
> mempll: <0x940034 0x14>
> 
> syspll: <0xea0200 0x14>
> 
> 
> We have three normal PLLS: cpupll, mempll and syspll. All these three PLLs use
> 25MHZ osc as clocksource. These plls can be bypassed. when syspll is bypassed
> the 25MHZ osc is directly output to syspllclk. When mempll/cpupll is bypassed,
> its corresponding fastrefclk is directly output to ddrphyclk/cpuclk: 
> 
> 
>        ---25MHZ osc----------|\
>            ________            | |-- syspllclk
>        ---| SYSPLL |---------|/
> 
> 
> 
>        ---cpufastrefclk------|\
>            ________            | |-- cpuclk
>        ---| CPUPLL |---------|/
> 
> 
>        ---memfastrefclk------|\
>            ________            | |-- ddrphyclk
>        ---| MEMPLL |---------|/
> 
> NOTE: the fastrefclk is the so called normal clk below.
> 
> 
> 
> two kinds of clk: normal clk and gate clk. The normal clk supports changing
> divider, selecting clock source, disabling/enabling etc. The gate clk only
> supports disabling/enabling. normal clks use syspllclk as clocksource, while
> gate clks use perifsysclk as clocksource.
> 


I hope I have described the BG4CT HW clk/pll clearly, I really need your advice
about my proposal. The clk nodes in my proposal will finally look like:


                cpupll: cpupll {
                        compatible = "marvell,berlin-pll";
                        reg = <0x922000 0x14>
                        #clock-cells = <0>;
                        clocks = <&osc>;
                };

		mempll: mempll {
                        compatible = "marvell,berlin-pll";
                        reg = <0x940034 0x14>
                        #clock-cells = <0>;
                        clocks = <&osc>;
                };

                syspll: syspll {
                        compatible = "marvell,berlin-pll";
                        reg = <0xea0200 0x14>
                        #clock-cells = <0>;
                        clocks = <&osc>;
                };

		pllclk: pllclk {
                        compatible = "marvell,berlin4ct-pllclk";
                        reg = <0xea0710 4>
                        #clock-cells = <1>;
		};

                gateclk: gateclk {
                        compatible = "marvell,berlin4ct-gateclk";
                        reg = <0xea0700 4>;
                        #clock-cells = <1>;
                };

                clk: clk {
                        compatible = "marvell,berlin4ct-clk";
                        reg = <0xea0720 0x144>;
                        #clock-cells = <1>;
                        clocks = <&pllclk SYSPLLCLK>;
                };

Using the ccf clk-mux, there's no overlapping/repeating reg ranges any more.

If you want more BG4CT clk/pll details, please let me know.

Thanks,
Jisheng

> 
> So what's the representing HW structure in fact? Here is my proposal:
> 
> 1. have mempll, cpupll and syspll node in dts
> 
> 2. one gateclk node in dts for gateclks
> 
> 3. one normalclk node in dts for normal clks
> 
> 4. one ccf clock-mux for cpuclk, ddrphyclk and syspllclk.
> 
> what do you think?
> 
> 
> From another side, let's have a look at driver/clk/mvebu. As can be seen,
> different clks register are close each other, for example, gateclk and coreclk
> in arch/arm/boot/dts/armada-xp.dtsi.
> 
> And drivers/clk/sunxi, arch/arm/boot/dts/sun7i-a20.dtsi, the pll4, pll12, gt_clk
> and ahb*, apb* etc... 
> 
> why these SoCs don't merge clocks/gates/plls to a single clock complex node? 
> I think that's because they are representing real HW structure.
> 
> Thanks,
> Jisheng
> 
> 
> > 
> > So, please just follow the OF/driver structure we already
> > have for Berlin2.
> > 
> > Sebastian
> >   
> > > +			#clock-cells = <1>;
> > > +			clocks = <&syspll>;
> > > +		};
> > > +
> > >  		soc_pinctrl: pin-controller@ea8000 {
> > >  			compatible = "marvell,berlin4ct-soc-pinctrl";
> > >  			reg = <0xea8000 0x14>;
> > >     
> >   
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 6/6] arm64: dts: berlin4ct: add pll and clock nodes

[Sebastian Hesselbarth]

On 23.11.2015 08:21, Jisheng Zhang wrote:
> On Fri, 20 Nov 2015 22:06:59 +0100
> Sebastian Hesselbarth wrote:
>> On 20.11.2015 09:42, Jisheng Zhang wrote:
>>> Add syspll, mempll, cpupll, gateclk and berlin-clk nodes.
>>>
>>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
>>> ---
[...]
>>> +		syspll: syspll {
>>> +			compatible = "marvell,berlin-pll";
>>> +			reg = <0xea0200 0x14>, <0xea0710 4>;
>>> +			#clock-cells = <0>;
>>> +			clocks = <&osc>;
>>> +			bypass-shift = /bits/ 8 <0>;
>>> +		};
>>> +
>>> +		gateclk: gateclk {
>>> +			compatible = "marvell,berlin4ct-gateclk";
>>> +			reg = <0xea0700 4>;
>>> +			#clock-cells = <1>;
>>> +		};
>>> +
>>> +		clk: clk {
>>> +			compatible = "marvell,berlin4ct-clk";
>>> +			reg = <0xea0720 0x144>;
>>
>> Looking at the reg ranges, I'd say that they are all clock related
>> and pretty close to each other:
>>
>> gateclk: reg = <0xea0700 4>;
>> bypass:  reg = <0xea0710 4>;
>> clk:     reg = <0xea0720 0x144>;
>
> Although these ranges sit close, but we should represent HW structure as you
> said.

Then how do you argue that you have to share the gate clock register
with every PLL? The answer is quite simple: You are not separating by
HW either but existing SW API.

If you would really want to just describe the HW, then you'd have to
have a single node for _all_ clocks that get controlled by 0xea0700/0x4,
feed some 32+ clocks into the node, and out again. Obviously, this
isn't what we want, right?

So, the idea of berlin2 sysctrl nodes (and similar other SoCs) is: Some
SoCs just dump some functions into a bunch of registers with no
particular order. We'd never find a good representation for that in DT,
so we don't bother to try but let the driver implementation deal with
the mess. Using "simple-mfd" is a nice solution to scattered registers
please have a look at it and come up with a cleaner separation for bg4
clock.

> First of all, let me describe the clks/plls in BG4CT. BG4CT contains:
>
> two kinds of PLL: normal PLL and AVPLL. These PLLs are put with their users
> together. For example: mempll pll registers <0xf7940034, 0x14> is put together
> with mem controller registers. AVPLL control registers are put with AV devices.

Why didn't you choose to have a memory-controller node that provides
mempll clock then? I am open to having multiple nodes providing clocks
but having a node for every clock in any subsystem is something I'll
not even think about.

> You can also check mempll, cpupll and syspll ranges:
>
> cpupll: <0x922000 0x14>
>
> mempll: <0x940034 0x14>
>
> syspll: <0xea0200 0x14>
>
>
> We have three normal PLLS: cpupll, mempll and syspll. All these three PLLs use
> 25MHZ osc as clocksource. These plls can be bypassed. when syspll is bypassed
> the 25MHZ osc is directly output to syspllclk. When mempll/cpupll is bypassed,
> its corresponding fastrefclk is directly output to ddrphyclk/cpuclk:
>
>
>         ---25MHZ osc----------|\
>             ________            | |-- syspllclk
>         ---| SYSPLL |---------|/
>
>
>
>         ---cpufastrefclk------|\
>             ________            | |-- cpuclk
>         ---| CPUPLL |---------|/
>
>
>         ---memfastrefclk------|\
>             ________            | |-- ddrphyclk
>         ---| MEMPLL |---------|/
>
> NOTE: the fastrefclk is the so called normal clk below.
>
>
>
> two kinds of clk: normal clk and gate clk. The normal clk supports changing
> divider, selecting clock source, disabling/enabling etc. The gate clk only
> supports disabling/enabling. normal clks use syspllclk as clocksource, while
> gate clks use perifsysclk as clocksource.
>
>
> So what's the representing HW structure in fact? Here is my proposal:
> 1. have mempll, cpupll and syspll node in dts

No.

> 2. one gateclk node in dts for gateclks

No.

> 3. one normalclk node in dts for normal clks

No.

> 4. one ccf clock-mux for cpuclk, ddrphyclk and syspllclk.

No.

> what do you think?

I think that the current separation is not a good one. I am open for
suggestions but I am not accepting single PLL/clock nodes.

>  From another side, let's have a look at driver/clk/mvebu. As can be seen,
> different clks register are close each other, for example, gateclk and coreclk
> in arch/arm/boot/dts/armada-xp.dtsi.
>
> And drivers/clk/sunxi, arch/arm/boot/dts/sun7i-a20.dtsi, the pll4, pll12, gt_clk
> and ahb*, apb* etc...
>
> why these SoCs don't merge clocks/gates/plls to a single clock complex node?
> I think that's because they are representing real HW structure.

These SoC (at least mvebu) didn't merge them into a single clock
complex node because nobody had a better idea or an impression of
the consequences. Looking back with the idea of syscon/simple-mfd
we probably would have chosen to separate differently.

>> So, please just follow the OF/driver structure we already
>> have for Berlin2.

To repeat: "please just follow the OF/driver structure we already
have for Berlin2"

Sebastian

>>> +			#clock-cells = <1>;
>>> +			clocks = <&syspll>;
>>> +		};
>>> +
>>>   		soc_pinctrl: pin-controller@ea8000 {
>>>   			compatible = "marvell,berlin4ct-soc-pinctrl";
>>>   			reg = <0xea8000 0x14>;
>>>
>>
>

Re: [PATCH v2 6/6] arm64: dts: berlin4ct: add pll and clock nodes

[Jisheng Zhang]

On Mon, 23 Nov 2015 09:30:42 +0100
Sebastian Hesselbarth wrote:

> On 23.11.2015 08:21, Jisheng Zhang wrote:
> > On Fri, 20 Nov 2015 22:06:59 +0100
> > Sebastian Hesselbarth wrote:  
> >> On 20.11.2015 09:42, Jisheng Zhang wrote:  
> >>> Add syspll, mempll, cpupll, gateclk and berlin-clk nodes.
> >>>
> >>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> >>> ---  
> [...]
> >>> +		syspll: syspll {
> >>> +			compatible = "marvell,berlin-pll";
> >>> +			reg = <0xea0200 0x14>, <0xea0710 4>;
> >>> +			#clock-cells = <0>;
> >>> +			clocks = <&osc>;
> >>> +			bypass-shift = /bits/ 8 <0>;
> >>> +		};
> >>> +
> >>> +		gateclk: gateclk {
> >>> +			compatible = "marvell,berlin4ct-gateclk";
> >>> +			reg = <0xea0700 4>;
> >>> +			#clock-cells = <1>;
> >>> +		};
> >>> +
> >>> +		clk: clk {
> >>> +			compatible = "marvell,berlin4ct-clk";
> >>> +			reg = <0xea0720 0x144>;  
> >>
> >> Looking at the reg ranges, I'd say that they are all clock related
> >> and pretty close to each other:
> >>
> >> gateclk: reg = <0xea0700 4>;
> >> bypass:  reg = <0xea0710 4>;
> >> clk:     reg = <0xea0720 0x144>;  
> >
> > Although these ranges sit close, but we should represent HW structure as you
> > said.  
> 
> Then how do you argue that you have to share the gate clock register
> with every PLL? The answer is quite simple: You are not separating by
> HW either but existing SW API.

No, PLLs don't share register any more. You can find what all clock nodes will
look like in:

http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/387322.html

> 
> If you would really want to just describe the HW, then you'd have to
> have a single node for _all_ clocks that get controlled by 0xea0700/0x4,
> feed some 32+ clocks into the node, and out again. Obviously, this
> isn't what we want, right?

I represented the HW by "kind", for example, gateclks, common-clks, pllclk.
And let common PLLs follow this rule as well:

one node for all common plls

reg <0x922000 0x14>, <0x940034 0x14>, <0xea0200 0x14>

> 
> So, the idea of berlin2 sysctrl nodes (and similar other SoCs) is: Some
> SoCs just dump some functions into a bunch of registers with no
> particular order. We'd never find a good representation for that in DT,
> so we don't bother to try but let the driver implementation deal with
> the mess. Using "simple-mfd" is a nice solution to scattered registers
> please have a look at it and come up with a cleaner separation for bg4
> clock.
> 
> > First of all, let me describe the clks/plls in BG4CT. BG4CT contains:
> >
> > two kinds of PLL: normal PLL and AVPLL. These PLLs are put with their users
> > together. For example: mempll pll registers <0xf7940034, 0x14> is put together
> > with mem controller registers. AVPLL control registers are put with AV devices.  
> 
> Why didn't you choose to have a memory-controller node that provides
> mempll clock then? I am open to having multiple nodes providing clocks
> but having a node for every clock in any subsystem is something I'll
> not even think about.

OK. As you said, "SoCs just dump some functions into a bunch of registers with
no particular order", BG4CT is now cleaner, all common-clks are put together,
gate-clks are put together, pllclks are put together, only the common PLLs
are dumped here and there. So how about representing the HW by "kind", one
node for common plls, one node for gateclks, one node for common clks and 
one node for pllclks?

                pll: pll {
                        compatible = "marvell,berlin4ct-pll";
                        reg = <0x922000 0x14>, <0x940034 0x14>, <0xea0200 0x14>
                        #clock-cells = <0>;
                        clocks = <&osc>;
                };

		pllclk: pllclk {
                        compatible = "marvell,berlin4ct-pllclk";
                        reg = <0xea0710 4>
                        #clock-cells = <1>;
		};

                gateclk: gateclk {
                        compatible = "marvell,berlin4ct-gateclk";
                        reg = <0xea0700 4>;
                        #clock-cells = <1>;
                };

                clk: clk {
                        compatible = "marvell,berlin4ct-clk";
                        reg = <0xea0720 0x144>;
                        #clock-cells = <1>;
                        clocks = <&pllclk SYSPLLCLK>;
                };

thanks

> 
> > You can also check mempll, cpupll and syspll ranges:
> >
> > cpupll: <0x922000 0x14>
> >
> > mempll: <0x940034 0x14>
> >
> > syspll: <0xea0200 0x14>
> >
> >
> > We have three normal PLLS: cpupll, mempll and syspll. All these three PLLs use
> > 25MHZ osc as clocksource. These plls can be bypassed. when syspll is bypassed
> > the 25MHZ osc is directly output to syspllclk. When mempll/cpupll is bypassed,
> > its corresponding fastrefclk is directly output to ddrphyclk/cpuclk:
> >
> >
> >         ---25MHZ osc----------|\
> >             ________            | |-- syspllclk
> >         ---| SYSPLL |---------|/
> >
> >
> >
> >         ---cpufastrefclk------|\
> >             ________            | |-- cpuclk
> >         ---| CPUPLL |---------|/
> >
> >
> >         ---memfastrefclk------|\
> >             ________            | |-- ddrphyclk
> >         ---| MEMPLL |---------|/
> >
> > NOTE: the fastrefclk is the so called normal clk below.
> >
> >
> >
> > two kinds of clk: normal clk and gate clk. The normal clk supports changing
> > divider, selecting clock source, disabling/enabling etc. The gate clk only
> > supports disabling/enabling. normal clks use syspllclk as clocksource, while
> > gate clks use perifsysclk as clocksource.
> >
> >
> > So what's the representing HW structure in fact? Here is my proposal:
> > 1. have mempll, cpupll and syspll node in dts  
> 
> No.
> 
> > 2. one gateclk node in dts for gateclks  
> 
> No.
> 
> > 3. one normalclk node in dts for normal clks  
> 
> No.
> 
> > 4. one ccf clock-mux for cpuclk, ddrphyclk and syspllclk.  
> 
> No.
> 
> > what do you think?  
> 
> I think that the current separation is not a good one. I am open for
> suggestions but I am not accepting single PLL/clock nodes.
> 
> >  From another side, let's have a look at driver/clk/mvebu. As can be seen,
> > different clks register are close each other, for example, gateclk and coreclk
> > in arch/arm/boot/dts/armada-xp.dtsi.
> >
> > And drivers/clk/sunxi, arch/arm/boot/dts/sun7i-a20.dtsi, the pll4, pll12, gt_clk
> > and ahb*, apb* etc...
> >
> > why these SoCs don't merge clocks/gates/plls to a single clock complex node?
> > I think that's because they are representing real HW structure.  
> 
> These SoC (at least mvebu) didn't merge them into a single clock
> complex node because nobody had a better idea or an impression of
> the consequences. Looking back with the idea of syscon/simple-mfd
> we probably would have chosen to separate differently.
> 
> >> So, please just follow the OF/driver structure we already
> >> have for Berlin2.  
> 
> To repeat: "please just follow the OF/driver structure we already
> have for Berlin2"
> 
> Sebastian
> 
> >>> +			#clock-cells = <1>;
> >>> +			clocks = <&syspll>;
> >>> +		};
> >>> +
> >>>   		soc_pinctrl: pin-controller@ea8000 {
> >>>   			compatible = "marvell,berlin4ct-soc-pinctrl";
> >>>   			reg = <0xea8000 0x14>;
> >>>  
> >>  
> >  
> 

Re: [PATCH v2 6/6] arm64: dts: berlin4ct: add pll and clock nodes

[Jisheng Zhang]

Dear Sebastian,

On Mon, 23 Nov 2015 16:54:44 +0800
Jisheng Zhang wrote:

> On Mon, 23 Nov 2015 09:30:42 +0100
> Sebastian Hesselbarth wrote:
> 
> > On 23.11.2015 08:21, Jisheng Zhang wrote:  
> > > On Fri, 20 Nov 2015 22:06:59 +0100
> > > Sebastian Hesselbarth wrote:    
> > >> On 20.11.2015 09:42, Jisheng Zhang wrote:    
> > >>> Add syspll, mempll, cpupll, gateclk and berlin-clk nodes.
> > >>>
> > >>> Signed-off-by: Jisheng Zhang <jszhang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> > >>> ---    
> > [...]  
> > >>> +		syspll: syspll {
> > >>> +			compatible = "marvell,berlin-pll";
> > >>> +			reg = <0xea0200 0x14>, <0xea0710 4>;
> > >>> +			#clock-cells = <0>;
> > >>> +			clocks = <&osc>;
> > >>> +			bypass-shift = /bits/ 8 <0>;
> > >>> +		};
> > >>> +
> > >>> +		gateclk: gateclk {
> > >>> +			compatible = "marvell,berlin4ct-gateclk";
> > >>> +			reg = <0xea0700 4>;
> > >>> +			#clock-cells = <1>;
> > >>> +		};
> > >>> +
> > >>> +		clk: clk {
> > >>> +			compatible = "marvell,berlin4ct-clk";
> > >>> +			reg = <0xea0720 0x144>;    
> > >>
> > >> Looking at the reg ranges, I'd say that they are all clock related
> > >> and pretty close to each other:
> > >>
> > >> gateclk: reg = <0xea0700 4>;
> > >> bypass:  reg = <0xea0710 4>;
> > >> clk:     reg = <0xea0720 0x144>;    
> > >
> > > Although these ranges sit close, but we should represent HW structure as you
> > > said.    
> > 
> > Then how do you argue that you have to share the gate clock register
> > with every PLL? The answer is quite simple: You are not separating by
> > HW either but existing SW API.  
> 
> No, PLLs don't share register any more. You can find what all clock nodes will
> look like in:
> 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/387322.html
> 
> > 
> > If you would really want to just describe the HW, then you'd have to
> > have a single node for _all_ clocks that get controlled by 0xea0700/0x4,
> > feed some 32+ clocks into the node, and out again. Obviously, this
> > isn't what we want, right?  
> 
> I represented the HW by "kind", for example, gateclks, common-clks, pllclk.
> And let common PLLs follow this rule as well:
> 
> one node for all common plls
> 
> reg <0x922000 0x14>, <0x940034 0x14>, <0xea0200 0x14>
> 
> > 
> > So, the idea of berlin2 sysctrl nodes (and similar other SoCs) is: Some
> > SoCs just dump some functions into a bunch of registers with no
> > particular order. We'd never find a good representation for that in DT,
> > so we don't bother to try but let the driver implementation deal with
> > the mess. Using "simple-mfd" is a nice solution to scattered registers
> > please have a look at it and come up with a cleaner separation for bg4
> > clock.
> >   
> > > First of all, let me describe the clks/plls in BG4CT. BG4CT contains:
> > >
> > > two kinds of PLL: normal PLL and AVPLL. These PLLs are put with their users
> > > together. For example: mempll pll registers <0xf7940034, 0x14> is put together
> > > with mem controller registers. AVPLL control registers are put with AV devices.    
> > 
> > Why didn't you choose to have a memory-controller node that provides
> > mempll clock then? I am open to having multiple nodes providing clocks
> > but having a node for every clock in any subsystem is something I'll
> > not even think about.  
> 
> OK. As you said, "SoCs just dump some functions into a bunch of registers with
> no particular order", BG4CT is now cleaner, all common-clks are put together,
> gate-clks are put together, pllclks are put together, only the common PLLs
> are dumped here and there. So how about representing the HW by "kind", one
> node for common plls, one node for gateclks, one node for common clks and 
> one node for pllclks?
> 
>                 pll: pll {
>                         compatible = "marvell,berlin4ct-pll";
>                         reg = <0x922000 0x14>, <0x940034 0x14>, <0xea0200 0x14>
>                         #clock-cells = <0>;

should be "#clock-cells = <1>;"

>                         clocks = <&osc>;
>                 };
> 
> 		pllclk: pllclk {
>                         compatible = "marvell,berlin4ct-pllclk";
>                         reg = <0xea0710 4>
>                         #clock-cells = <1>;
> 		};
> 
>                 gateclk: gateclk {
>                         compatible = "marvell,berlin4ct-gateclk";
>                         reg = <0xea0700 4>;
>                         #clock-cells = <1>;
>                 };
> 
>                 clk: clk {
>                         compatible = "marvell,berlin4ct-clk";
>                         reg = <0xea0720 0x144>;
>                         #clock-cells = <1>;
>                         clocks = <&pllclk SYSPLLCLK>;
>                 };
> 

there's no a node for every clock with this proposal, all clks/plls are separated
by "kind". Is this OK for you?

thanks
--
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 v2 6/6] arm64: dts: berlin4ct: add pll and clock nodes

[Sebastian Hesselbarth]

On 24.11.2015 03:35, Jisheng Zhang wrote:
> On Mon, 23 Nov 2015 16:54:44 +0800
> Jisheng Zhang wrote:
>> On Mon, 23 Nov 2015 09:30:42 +0100
>> Sebastian Hesselbarth wrote:
>>> On 23.11.2015 08:21, Jisheng Zhang wrote:
>>>> On Fri, 20 Nov 2015 22:06:59 +0100
>>>> Sebastian Hesselbarth wrote:
>>>>> On 20.11.2015 09:42, Jisheng Zhang wrote:
>>>>>> Add syspll, mempll, cpupll, gateclk and berlin-clk nodes.
>>>>>>
>>>>>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
>>>>>> ---
>>> [...]
>>>>>> +		syspll: syspll {
>>>>>> +			compatible = "marvell,berlin-pll";
>>>>>> +			reg = <0xea0200 0x14>, <0xea0710 4>;
>>>>>> +			#clock-cells = <0>;
>>>>>> +			clocks = <&osc>;
>>>>>> +			bypass-shift = /bits/ 8 <0>;
>>>>>> +		};
>>>>>> +
>>>>>> +		gateclk: gateclk {
>>>>>> +			compatible = "marvell,berlin4ct-gateclk";
>>>>>> +			reg = <0xea0700 4>;
>>>>>> +			#clock-cells = <1>;
>>>>>> +		};
>>>>>> +
>>>>>> +		clk: clk {
>>>>>> +			compatible = "marvell,berlin4ct-clk";
>>>>>> +			reg = <0xea0720 0x144>;
>>>>>
>>>>> Looking at the reg ranges, I'd say that they are all clock related
>>>>> and pretty close to each other:
>>>>>
>>>>> gateclk: reg = <0xea0700 4>;
>>>>> bypass:  reg = <0xea0710 4>;
>>>>> clk:     reg = <0xea0720 0x144>;
>>>>
>>>> Although these ranges sit close, but we should represent HW structure as you
>>>> said.
>>>
>>> Then how do you argue that you have to share the gate clock register
>>> with every PLL? The answer is quite simple: You are not separating by
>>> HW either but existing SW API.
>>
>> No, PLLs don't share register any more. You can find what all clock nodes will
>> look like in:

Jisheng,

referring to the sunxi clock related thread, I am glad you finally
picked up the idea of merging clock nodes. Before you start reworking
bg4 clocks, I think I should be a little bit more clear about what I
expect to be the outcome.

When I said "one single clock complex node", I was referring to the
clocks located within the system-controller reg region, i.e. those at
0xea0000. With bg2x we came to the conclusion that those registers
cannot be not cleanly separated by functions, so we decided to have
a single system-controller simple-mfd node with sub-nodes for
pinctrl, clock, reset, and whatever we will find there in the future.

Please also follow this scheme for bg4 because when you start looking
at reset, you'll likely see the same issues we were facing: Either you
have a reset controller node with a plethora of reg property entries
or you constantly undermine the concept of requested resources by using
of_iomap().

Using simple-mfd is a compromise between detailed HW description and
usability. It will also automatically deal with concurrent accesses to
the same register for e.g. clock and reset because simple-mfd and syscon
install an access lock for the reg region. Even though there may be no
real concurrent accesses to the same register, it does no real harm
because we are locking resisters that aren't supposed to be used in
high-speed applications. Some of them are touched once at boot, most
of them are never touched by Linux at all.

For the other PLLs at <0x922000 0x14> and <0x940034 0x14>, I do accept
that they are not part of the system-controller sub-node. For the short
run, I would accept separate nodes for the PLLs alone, but on the long
run they should be hidden within the functional node they belong to,
i.e. mempll should be a clock provided by some memory-controller node.
As soon as you look at power saving modes for the memory-controller,
you would need access to memory-controller register _and_ mempll anyway.

We do have our DT tagged unstable, so we still can easily revert our
limited view of some nodes later.

BTW, if the clock provided by mempll is used to generate peripheral
clocks that are dealt with in the sysctrl clock complex, you should,
of course, expose that relation in DT, e.g.:

sysctrl: system-controller {
	reg = <0xea0700 0xfoo>;

	sysclk: clock {
		#clock-cells = <1>;
		clocks = <&osc>, <&memctrl 0>;
		clock-names = "osc", "mempll";
	};
};

memctrl: memory-controller {
	reg = <0x920000 0xbar>;
	/*
	 * clock-cells can also be 0
	 * if there is no other clock provided
	 */
	#clock-cells = <1>;

	clocks = <&osc>;
	clock-names = "osc";
};

some-peripheral: bla {
	clocks = <&sysclk SOME_PERIPHERAL_CLOCK_ID>;
};

Sebastian

>>> If you would really want to just describe the HW, then you'd have to
>>> have a single node for _all_ clocks that get controlled by 0xea0700/0x4,
>>> feed some 32+ clocks into the node, and out again. Obviously, this
>>> isn't what we want, right?
>>
>> I represented the HW by "kind", for example, gateclks, common-clks, pllclk.
>> And let common PLLs follow this rule as well:
>>
>> one node for all common plls
>>
>> reg <0x922000 0x14>, <0x940034 0x14>, <0xea0200 0x14>
>>
>>>
>>> So, the idea of berlin2 sysctrl nodes (and similar other SoCs) is: Some
>>> SoCs just dump some functions into a bunch of registers with no
>>> particular order. We'd never find a good representation for that in DT,
>>> so we don't bother to try but let the driver implementation deal with
>>> the mess. Using "simple-mfd" is a nice solution to scattered registers
>>> please have a look at it and come up with a cleaner separation for bg4
>>> clock.
>>>
>>>> First of all, let me describe the clks/plls in BG4CT. BG4CT contains:
>>>>
>>>> two kinds of PLL: normal PLL and AVPLL. These PLLs are put with their users
>>>> together. For example: mempll pll registers <0xf7940034, 0x14> is put together
>>>> with mem controller registers. AVPLL control registers are put with AV devices.
>>>
>>> Why didn't you choose to have a memory-controller node that provides
>>> mempll clock then? I am open to having multiple nodes providing clocks
>>> but having a node for every clock in any subsystem is something I'll
>>> not even think about.
>>
>> OK. As you said, "SoCs just dump some functions into a bunch of registers with
>> no particular order", BG4CT is now cleaner, all common-clks are put together,
>> gate-clks are put together, pllclks are put together, only the common PLLs
>> are dumped here and there. So how about representing the HW by "kind", one
>> node for common plls, one node for gateclks, one node for common clks and
>> one node for pllclks?
>>
>>                  pll: pll {
>>                          compatible = "marvell,berlin4ct-pll";
>>                          reg = <0x922000 0x14>, <0x940034 0x14>, <0xea0200 0x14>
>>                          #clock-cells = <0>;
>
> should be "#clock-cells = <1>;"
>
>>                          clocks = <&osc>;
>>                  };
>>
>> 		pllclk: pllclk {
>>                          compatible = "marvell,berlin4ct-pllclk";
>>                          reg = <0xea0710 4>
>>                          #clock-cells = <1>;
>> 		};
>>
>>                  gateclk: gateclk {
>>                          compatible = "marvell,berlin4ct-gateclk";
>>                          reg = <0xea0700 4>;
>>                          #clock-cells = <1>;
>>                  };
>>
>>                  clk: clk {
>>                          compatible = "marvell,berlin4ct-clk";
>>                          reg = <0xea0720 0x144>;
>>                          #clock-cells = <1>;
>>                          clocks = <&pllclk SYSPLLCLK>;
>>                  };
>>
>
> there's no a node for every clock with this proposal, all clks/plls are separated
> by "kind". Is this OK for you?
>
> thanks
>

Re: [PATCH v2 6/6] arm64: dts: berlin4ct: add pll and clock nodes

[Jisheng Zhang]

On Fri, 27 Nov 2015 08:51:27 +0100
Sebastian Hesselbarth wrote:

> On 24.11.2015 03:35, Jisheng Zhang wrote:
> > On Mon, 23 Nov 2015 16:54:44 +0800
> > Jisheng Zhang wrote:  
> >> On Mon, 23 Nov 2015 09:30:42 +0100
> >> Sebastian Hesselbarth wrote:  
> >>> On 23.11.2015 08:21, Jisheng Zhang wrote:  
> >>>> On Fri, 20 Nov 2015 22:06:59 +0100
> >>>> Sebastian Hesselbarth wrote:  
> >>>>> On 20.11.2015 09:42, Jisheng Zhang wrote:  
> >>>>>> Add syspll, mempll, cpupll, gateclk and berlin-clk nodes.
> >>>>>>
> >>>>>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> >>>>>> ---  
> >>> [...]  
> >>>>>> +		syspll: syspll {
> >>>>>> +			compatible = "marvell,berlin-pll";
> >>>>>> +			reg = <0xea0200 0x14>, <0xea0710 4>;
> >>>>>> +			#clock-cells = <0>;
> >>>>>> +			clocks = <&osc>;
> >>>>>> +			bypass-shift = /bits/ 8 <0>;
> >>>>>> +		};
> >>>>>> +
> >>>>>> +		gateclk: gateclk {
> >>>>>> +			compatible = "marvell,berlin4ct-gateclk";
> >>>>>> +			reg = <0xea0700 4>;
> >>>>>> +			#clock-cells = <1>;
> >>>>>> +		};
> >>>>>> +
> >>>>>> +		clk: clk {
> >>>>>> +			compatible = "marvell,berlin4ct-clk";
> >>>>>> +			reg = <0xea0720 0x144>;  
> >>>>>
> >>>>> Looking at the reg ranges, I'd say that they are all clock related
> >>>>> and pretty close to each other:
> >>>>>
> >>>>> gateclk: reg = <0xea0700 4>;
> >>>>> bypass:  reg = <0xea0710 4>;
> >>>>> clk:     reg = <0xea0720 0x144>;  
> >>>>
> >>>> Although these ranges sit close, but we should represent HW structure as you
> >>>> said.  
> >>>
> >>> Then how do you argue that you have to share the gate clock register
> >>> with every PLL? The answer is quite simple: You are not separating by
> >>> HW either but existing SW API.  
> >>
> >> No, PLLs don't share register any more. You can find what all clock nodes will
> >> look like in:  
> 
> Jisheng,
> 
> referring to the sunxi clock related thread, I am glad you finally
> picked up the idea of merging clock nodes. Before you start reworking
> bg4 clocks, I think I should be a little bit more clear about what I
> expect to be the outcome.
> 
> When I said "one single clock complex node", I was referring to the
> clocks located within the system-controller reg region, i.e. those at
> 0xea0000. With bg2x we came to the conclusion that those registers
> cannot be not cleanly separated by functions, so we decided to have
> a single system-controller simple-mfd node with sub-nodes for
> pinctrl, clock, reset, and whatever we will find there in the future.
> 
> Please also follow this scheme for bg4 because when you start looking
> at reset, you'll likely see the same issues we were facing: Either you
> have a reset controller node with a plethora of reg property entries
> or you constantly undermine the concept of requested resources by using
> of_iomap().
> 
> Using simple-mfd is a compromise between detailed HW description and
> usability. It will also automatically deal with concurrent accesses to
> the same register for e.g. clock and reset because simple-mfd and syscon
> install an access lock for the reg region. Even though there may be no
> real concurrent accesses to the same register, it does no real harm
> because we are locking resisters that aren't supposed to be used in
> high-speed applications. Some of them are touched once at boot, most
> of them are never touched by Linux at all.

Thank you so much for the detailed information. It do help me to have
a better understanding why.

> 
> For the other PLLs at <0x922000 0x14> and <0x940034 0x14>, I do accept
> that they are not part of the system-controller sub-node. For the short
> run, I would accept separate nodes for the PLLs alone, but on the long
> run they should be hidden within the functional node they belong to,
> i.e. mempll should be a clock provided by some memory-controller node.
> As soon as you look at power saving modes for the memory-controller,
> you would need access to memory-controller register _and_ mempll anyway.
> 
> We do have our DT tagged unstable, so we still can easily revert our
> limited view of some nodes later.
> 
> BTW, if the clock provided by mempll is used to generate peripheral
> clocks that are dealt with in the sysctrl clock complex, you should,

mempll is only for ddrphy clk. So we are lucky ;)

Thanks,
Jisheng

> of course, expose that relation in DT, e.g.:
> 
> sysctrl: system-controller {
> 	reg = <0xea0700 0xfoo>;
> 
> 	sysclk: clock {
> 		#clock-cells = <1>;
> 		clocks = <&osc>, <&memctrl 0>;
> 		clock-names = "osc", "mempll";
> 	};
> };
> 
> memctrl: memory-controller {
> 	reg = <0x920000 0xbar>;
> 	/*
> 	 * clock-cells can also be 0
> 	 * if there is no other clock provided
> 	 */
> 	#clock-cells = <1>;
> 
> 	clocks = <&osc>;
> 	clock-names = "osc";
> };
> 
> some-peripheral: bla {
> 	clocks = <&sysclk SOME_PERIPHERAL_CLOCK_ID>;
> };
> 
> Sebastian
> 

Re: [PATCH v2 6/6] arm64: dts: berlin4ct: add pll and clock nodes

[Jisheng Zhang]

On Fri, 27 Nov 2015 16:39:37 +0800
Jisheng Zhang wrote:

> On Fri, 27 Nov 2015 08:51:27 +0100
> Sebastian Hesselbarth wrote:
> 
> > On 24.11.2015 03:35, Jisheng Zhang wrote:  
> > > On Mon, 23 Nov 2015 16:54:44 +0800
> > > Jisheng Zhang wrote:    
> > >> On Mon, 23 Nov 2015 09:30:42 +0100
> > >> Sebastian Hesselbarth wrote:    
> > >>> On 23.11.2015 08:21, Jisheng Zhang wrote:    
> > >>>> On Fri, 20 Nov 2015 22:06:59 +0100
> > >>>> Sebastian Hesselbarth wrote:    
> > >>>>> On 20.11.2015 09:42, Jisheng Zhang wrote:    
> > >>>>>> Add syspll, mempll, cpupll, gateclk and berlin-clk nodes.
> > >>>>>>
> > >>>>>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > >>>>>> ---    
> > >>> [...]    
> > >>>>>> +		syspll: syspll {
> > >>>>>> +			compatible = "marvell,berlin-pll";
> > >>>>>> +			reg = <0xea0200 0x14>, <0xea0710 4>;
> > >>>>>> +			#clock-cells = <0>;
> > >>>>>> +			clocks = <&osc>;
> > >>>>>> +			bypass-shift = /bits/ 8 <0>;
> > >>>>>> +		};
> > >>>>>> +
> > >>>>>> +		gateclk: gateclk {
> > >>>>>> +			compatible = "marvell,berlin4ct-gateclk";
> > >>>>>> +			reg = <0xea0700 4>;
> > >>>>>> +			#clock-cells = <1>;
> > >>>>>> +		};
> > >>>>>> +
> > >>>>>> +		clk: clk {
> > >>>>>> +			compatible = "marvell,berlin4ct-clk";
> > >>>>>> +			reg = <0xea0720 0x144>;    
> > >>>>>
> > >>>>> Looking at the reg ranges, I'd say that they are all clock related
> > >>>>> and pretty close to each other:
> > >>>>>
> > >>>>> gateclk: reg = <0xea0700 4>;
> > >>>>> bypass:  reg = <0xea0710 4>;
> > >>>>> clk:     reg = <0xea0720 0x144>;    
> > >>>>
> > >>>> Although these ranges sit close, but we should represent HW structure as you
> > >>>> said.    
> > >>>
> > >>> Then how do you argue that you have to share the gate clock register
> > >>> with every PLL? The answer is quite simple: You are not separating by
> > >>> HW either but existing SW API.    
> > >>
> > >> No, PLLs don't share register any more. You can find what all clock nodes will
> > >> look like in:    
> > 
> > Jisheng,
> > 
> > referring to the sunxi clock related thread, I am glad you finally
> > picked up the idea of merging clock nodes. Before you start reworking
> > bg4 clocks, I think I should be a little bit more clear about what I
> > expect to be the outcome.
> > 
> > When I said "one single clock complex node", I was referring to the
> > clocks located within the system-controller reg region, i.e. those at
> > 0xea0000. With bg2x we came to the conclusion that those registers
> > cannot be not cleanly separated by functions, so we decided to have
> > a single system-controller simple-mfd node with sub-nodes for
> > pinctrl, clock, reset, and whatever we will find there in the future.
> > 
> > Please also follow this scheme for bg4 because when you start looking
> > at reset, you'll likely see the same issues we were facing: Either you
> > have a reset controller node with a plethora of reg property entries
> > or you constantly undermine the concept of requested resources by using
> > of_iomap().
> > 
> > Using simple-mfd is a compromise between detailed HW description and
> > usability. It will also automatically deal with concurrent accesses to
> > the same register for e.g. clock and reset because simple-mfd and syscon
> > install an access lock for the reg region. Even though there may be no
> > real concurrent accesses to the same register, it does no real harm
> > because we are locking resisters that aren't supposed to be used in
> > high-speed applications. Some of them are touched once at boot, most
> > of them are never touched by Linux at all.  
> 
> Thank you so much for the detailed information. It do help me to have
> a better understanding why.
> 
> > 
> > For the other PLLs at <0x922000 0x14> and <0x940034 0x14>, I do accept
> > that they are not part of the system-controller sub-node. For the short
> > run, I would accept separate nodes for the PLLs alone, but on the long
> > run they should be hidden within the functional node they belong to,
> > i.e. mempll should be a clock provided by some memory-controller node.
> > As soon as you look at power saving modes for the memory-controller,
> > you would need access to memory-controller register _and_ mempll anyway.
> > 
> > We do have our DT tagged unstable, so we still can easily revert our
> > limited view of some nodes later.
> > 
> > BTW, if the clock provided by mempll is used to generate peripheral
> > clocks that are dealt with in the sysctrl clock complex, you should,  
> 
> mempll is only for ddrphy clk. So we are lucky ;)

oops, no, we are not lucky. mempll and memfastrefclk are clk-muxed to ddrphy clk
which is dealt within the complex big clock block. So is for cpupll, cpufastrefclk
But it's not that hard to add this support in the code.

Thanks for the example, I do need that....


> 
> Thanks,
> Jisheng
> 
> > of course, expose that relation in DT, e.g.:
> > 
> > sysctrl: system-controller {
> > 	reg = <0xea0700 0xfoo>;
> > 
> > 	sysclk: clock {
> > 		#clock-cells = <1>;
> > 		clocks = <&osc>, <&memctrl 0>;
> > 		clock-names = "osc", "mempll";
> > 	};
> > };
> > 
> > memctrl: memory-controller {
> > 	reg = <0x920000 0xbar>;
> > 	/*
> > 	 * clock-cells can also be 0
> > 	 * if there is no other clock provided
> > 	 */
> > 	#clock-cells = <1>;
> > 
> > 	clocks = <&osc>;
> > 	clock-names = "osc";
> > };
> > 
> > some-peripheral: bla {
> > 	clocks = <&sysclk SOME_PERIPHERAL_CLOCK_ID>;
> > };
> > 
> > Sebastian
> >