[PATCH 0/5] berlin: initial support for the clocks

[PATCH 0/5] berlin: initial support for the clocks

[Alexandre Belloni]

This series adds support for the berlin PLLs. This allows to remove the bogus
fixed clocks that are used in the SoCs dts includes.

For now, I have left out the AVPLL to ease reviewing.

This is tested on a BG2Q DMP.

Alexandre Belloni (5):
  clk: berlin: add support for berlin plls
  clk: berlin: add berlin clocks DT bindings documentation
  ARM: berlin/dt: add cpupll and syspll support to BG2Q
  ARM: berlin/dt: add cpupll and syspll support to BG2CD
  ARM: berlin/dt: add cpupll and syspll support to BG2

 .../devicetree/bindings/clock/berlin-clock.txt     |  29 ++++++
 arch/arm/boot/dts/berlin2.dtsi                     |  56 ++++++++----
 arch/arm/boot/dts/berlin2cd.dtsi                   |  56 ++++++++----
 arch/arm/boot/dts/berlin2q.dtsi                    |  20 ++++-
 drivers/clk/Makefile                               |   1 +
 drivers/clk/berlin/Makefile                        |   1 +
 drivers/clk/berlin/clk.h                           |  35 ++++++++
 drivers/clk/berlin/pll-berlin2.c                   |  41 +++++++++
 drivers/clk/berlin/pll-berlin2q.c                  |  41 +++++++++
 drivers/clk/berlin/pll.c                           | 100 +++++++++++++++++++++
 10 files changed, 344 insertions(+), 36 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/berlin-clock.txt
 create mode 100644 drivers/clk/berlin/Makefile
 create mode 100644 drivers/clk/berlin/clk.h
 create mode 100644 drivers/clk/berlin/pll-berlin2.c
 create mode 100644 drivers/clk/berlin/pll-berlin2q.c
 create mode 100644 drivers/clk/berlin/pll.c

-- 
1.8.3.2

[PATCH 1/5] clk: berlin: add support for berlin plls

[Alexandre Belloni]

This drivers allows to provide DT clocks for the cpu and system PLLs found on
Marvell Berlin SoCs.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 drivers/clk/Makefile              |   1 +
 drivers/clk/berlin/Makefile       |   1 +
 drivers/clk/berlin/clk.h          |  35 +++++++++++++
 drivers/clk/berlin/pll-berlin2.c  |  41 ++++++++++++++++
 drivers/clk/berlin/pll-berlin2q.c |  41 ++++++++++++++++
 drivers/clk/berlin/pll.c          | 100 ++++++++++++++++++++++++++++++++++++++
 6 files changed, 219 insertions(+)
 create mode 100644 drivers/clk/berlin/Makefile
 create mode 100644 drivers/clk/berlin/clk.h
 create mode 100644 drivers/clk/berlin/pll-berlin2.c
 create mode 100644 drivers/clk/berlin/pll-berlin2q.c
 create mode 100644 drivers/clk/berlin/pll.c

diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index a367a9831717..4a2602737c27 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_ARCH_VT8500)		+= clk-vt8500.o
 obj-$(CONFIG_COMMON_CLK_WM831X)		+= clk-wm831x.o
 obj-$(CONFIG_COMMON_CLK_XGENE)		+= clk-xgene.o
 obj-$(CONFIG_COMMON_CLK_AT91)		+= at91/
+obj-$(CONFIG_ARCH_BERLIN)		+= berlin/
 obj-$(CONFIG_ARCH_HI3xxx)		+= hisilicon/
 obj-$(CONFIG_COMMON_CLK_KEYSTONE)	+= keystone/
 ifeq ($(CONFIG_COMMON_CLK), y)
diff --git a/drivers/clk/berlin/Makefile b/drivers/clk/berlin/Makefile
new file mode 100644
index 000000000000..4f6580e2a3ee
--- /dev/null
+++ b/drivers/clk/berlin/Makefile
@@ -0,0 +1 @@
+obj-y += pll.o pll-berlin2.o pll-berlin2q.o
diff --git a/drivers/clk/berlin/clk.h b/drivers/clk/berlin/clk.h
new file mode 100644
index 000000000000..41459db31a02
--- /dev/null
+++ b/drivers/clk/berlin/clk.h
@@ -0,0 +1,35 @@
+/*
+ * Copyright (c) 2013 Marvell Technology Group Ltd.
+ *
+ * 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
+
+#include <linux/clk-provider.h>
+
+struct berlin_pllmap {
+	const u8	*vcodiv;
+	u32		fbdiv_mask;
+	u32		rfdiv_mask;
+	u32		divsel_mask;
+	u8		fbdiv_shift;
+	u8		rfdiv_shift;
+	u8		divsel_shift;
+	u8		mult;
+};
+
+extern void __init berlin_pll_setup(struct device_node *np,
+				struct berlin_pllmap *map);
+
+#endif /* BERLIN_CLK_H */
diff --git a/drivers/clk/berlin/pll-berlin2.c b/drivers/clk/berlin/pll-berlin2.c
new file mode 100644
index 000000000000..5f5b4674f811
--- /dev/null
+++ b/drivers/clk/berlin/pll-berlin2.c
@@ -0,0 +1,41 @@
+/*
+ * Copyright (c) 2014 Marvell Technology Group Ltd.
+ *
+ * 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/io.h>
+#include <linux/of.h>
+#include <linux/kernel.h>
+#include <linux/clk-provider.h>
+
+#include "clk.h"
+
+static const u8 vcodiv_berlin2[] = {10, 15, 20, 25, 30, 40, 50, 60, 80};
+
+static struct berlin_pllmap berlin_pll_map = {
+	.vcodiv = vcodiv_berlin2,
+	.fbdiv_mask = 0x1FF,
+	.rfdiv_mask = 0x1F,
+	.divsel_mask = 0xF,
+	.fbdiv_shift = 6,
+	.rfdiv_shift = 1,
+	.divsel_shift = 7,
+	.mult = 10,
+};
+
+static void __init berlin2_pll_setup(struct device_node *np)
+{
+	berlin_pll_setup(np, &berlin_pll_map);
+}
+CLK_OF_DECLARE(berlin2q_pll, "marvell,berlin2-pll", berlin2_pll_setup);
+
diff --git a/drivers/clk/berlin/pll-berlin2q.c b/drivers/clk/berlin/pll-berlin2q.c
new file mode 100644
index 000000000000..8ddfa69cf8c4
--- /dev/null
+++ b/drivers/clk/berlin/pll-berlin2q.c
@@ -0,0 +1,41 @@
+/*
+ * Copyright (c) 2013 Marvell Technology Group Ltd.
+ *
+ * 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/io.h>
+#include <linux/of.h>
+#include <linux/kernel.h>
+#include <linux/clk-provider.h>
+
+#include "clk.h"
+
+static const u8 vcodiv_berlin2q[] = {1, 0, 2, 0, 3, 4, 0, 6, 8};
+
+static struct berlin_pllmap berlin2q_pll_map = {
+	.vcodiv = vcodiv_berlin2q,
+	.fbdiv_mask = 0x1FF,
+	.rfdiv_mask = 0x1F,
+	.divsel_mask = 0xF,
+	.fbdiv_shift = 7,
+	.rfdiv_shift = 2,
+	.divsel_shift = 9,
+	.mult = 1,
+};
+
+static void __init berlin2q_pll_setup(struct device_node *np)
+{
+	berlin_pll_setup(np, &berlin2q_pll_map);
+}
+CLK_OF_DECLARE(berlin2q_pll, "marvell,berlin2q-pll", berlin2q_pll_setup);
+
diff --git a/drivers/clk/berlin/pll.c b/drivers/clk/berlin/pll.c
new file mode 100644
index 000000000000..188b385af436
--- /dev/null
+++ b/drivers/clk/berlin/pll.c
@@ -0,0 +1,100 @@
+/*
+ * Copyright (c) 2013 Marvell Technology Group Ltd.
+ *
+ * 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/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/slab.h>
+#include <linux/kernel.h>
+#include <linux/clk-provider.h>
+
+#include "clk.h"
+
+struct berlin_pll {
+	struct clk_hw	hw;
+	void __iomem	*base;
+	void		*data;
+};
+
+#define to_berlin_pll(hw)	container_of(hw, struct berlin_pll, hw)
+
+#define PLL_CTRL0	0x00
+#define PLL_CTRL1	0x04
+
+static inline u32 berlin_pll_read(struct berlin_pll *pll, unsigned long offset)
+{
+	return readl_relaxed(pll->base + offset);
+}
+
+static unsigned long berlin_pll_recalc_rate(struct clk_hw *hw,
+					unsigned long parent_rate)
+{
+	struct berlin_pll *pll = to_berlin_pll(hw);
+	struct berlin_pllmap *map = pll->data;
+	u32 val, fbdiv, rfdiv, vcodivsel;
+
+	val = berlin_pll_read(pll, PLL_CTRL0);
+	fbdiv = (val >> map->fbdiv_shift) & map->fbdiv_mask;
+	rfdiv = (val >> map->rfdiv_shift) & map->rfdiv_mask;
+	if (rfdiv == 0)
+		rfdiv = 1;
+
+	val = berlin_pll_read(pll, PLL_CTRL1);
+	vcodivsel = (val >> map->divsel_shift) & map->divsel_mask;
+
+	return parent_rate * fbdiv * map->mult / rfdiv /
+		map->vcodiv[vcodivsel];
+}
+
+static const struct clk_ops berlin_pll_ops = {
+	.recalc_rate	= berlin_pll_recalc_rate,
+};
+
+void __init berlin_pll_setup(struct device_node *np,
+			struct berlin_pllmap *map)
+{
+	struct clk_init_data init;
+	struct berlin_pll *pll;
+	const char *parent_name;
+	struct clk *clk;
+	int ret;
+
+	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
+	if (WARN_ON(!pll))
+		return;
+
+	pll->base = of_iomap(np, 0);
+	if (WARN_ON(!pll->base))
+		return;
+
+	pll->data = map;
+
+	init.name = np->name;
+	init.ops = &berlin_pll_ops;
+	parent_name = of_clk_get_parent_name(np, 0);
+	init.parent_names = &parent_name;
+	init.num_parents = 1;
+
+	pll->hw.init = &init;
+
+	clk = clk_register(NULL, &pll->hw);
+	if (WARN_ON(IS_ERR(clk)))
+		return;
+
+	ret = of_clk_add_provider(np, of_clk_src_simple_get, clk);
+	if (WARN_ON(ret))
+		return;
+}
+
-- 
1.8.3.2

Re: [PATCH 1/5] clk: berlin: add support for berlin plls

[Sebastian Hesselbarth]

On 03/21/2014 12:43 PM, Alexandre Belloni wrote:
> This drivers allows to provide DT clocks for the cpu and system PLLs found on
> Marvell Berlin SoCs.
>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
>   drivers/clk/Makefile              |   1 +
>   drivers/clk/berlin/Makefile       |   1 +
>   drivers/clk/berlin/clk.h          |  35 +++++++++++++
>   drivers/clk/berlin/pll-berlin2.c  |  41 ++++++++++++++++
>   drivers/clk/berlin/pll-berlin2q.c |  41 ++++++++++++++++
>   drivers/clk/berlin/pll.c          | 100 ++++++++++++++++++++++++++++++++++++++
>   6 files changed, 219 insertions(+)
>   create mode 100644 drivers/clk/berlin/Makefile
>   create mode 100644 drivers/clk/berlin/clk.h
>   create mode 100644 drivers/clk/berlin/pll-berlin2.c
>   create mode 100644 drivers/clk/berlin/pll-berlin2q.c
>   create mode 100644 drivers/clk/berlin/pll.c
>
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index a367a9831717..4a2602737c27 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_ARCH_VT8500)		+= clk-vt8500.o
>   obj-$(CONFIG_COMMON_CLK_WM831X)		+= clk-wm831x.o
>   obj-$(CONFIG_COMMON_CLK_XGENE)		+= clk-xgene.o
>   obj-$(CONFIG_COMMON_CLK_AT91)		+= at91/
> +obj-$(CONFIG_ARCH_BERLIN)		+= berlin/
>   obj-$(CONFIG_ARCH_HI3xxx)		+= hisilicon/
>   obj-$(CONFIG_COMMON_CLK_KEYSTONE)	+= keystone/

I'll have a look at clk/Makefile later myself, but alphabetic ordering
seems to be broken by ARCH_HI3xxx already. Anyway, we shouldn't just
add another violator.

>   ifeq ($(CONFIG_COMMON_CLK), y)
> diff --git a/drivers/clk/berlin/Makefile b/drivers/clk/berlin/Makefile
> new file mode 100644
> index 000000000000..4f6580e2a3ee
> --- /dev/null
> +++ b/drivers/clk/berlin/Makefile
> @@ -0,0 +1 @@
> +obj-y += pll.o pll-berlin2.o pll-berlin2q.o

obj-y += pll.o
obj-$(CONFIG_MACH_BERLIN_BG2)	+= pll-berlin2.o
obj-$(CONFIG_MACH_BERLIN_BG2CD)	+= pll-berlin2.o
obj-$(CONFIG_MACH_BERLIN_BG2Q)	+= pll-berlin2q.o

Which reminds me, that we forgot to add MACH_BERLIN_BG2Q to
arch/arm/mach-berlin/Kconfig. Can you spin a patch?

> diff --git a/drivers/clk/berlin/clk.h b/drivers/clk/berlin/clk.h

I prefer to have this include named "common.h" as I guess we will
dump all common declarations in here.

> new file mode 100644
> index 000000000000..41459db31a02
> --- /dev/null
> +++ b/drivers/clk/berlin/clk.h
> @@ -0,0 +1,35 @@
> +/*
> + * Copyright (c) 2013 Marvell Technology Group Ltd.
> + *
> + * 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
> +
> +#include <linux/clk-provider.h>

What is this include good for in here?

You will need struct device_node declared, but a simple

struct device_node;

should be sufficient.

> +struct berlin_pllmap {
> +	const u8	*vcodiv;
> +	u32		fbdiv_mask;
> +	u32		rfdiv_mask;
> +	u32		divsel_mask;
> +	u8		fbdiv_shift;
> +	u8		rfdiv_shift;
> +	u8		divsel_shift;
> +	u8		mult;
> +};
> +
> +extern void __init berlin_pll_setup(struct device_node *np,
> +				struct berlin_pllmap *map);
> +
> +#endif /* BERLIN_CLK_H */
> diff --git a/drivers/clk/berlin/pll-berlin2.c b/drivers/clk/berlin/pll-berlin2.c
> new file mode 100644
> index 000000000000..5f5b4674f811
> --- /dev/null
> +++ b/drivers/clk/berlin/pll-berlin2.c
> @@ -0,0 +1,41 @@
> +/*
> + * Copyright (c) 2014 Marvell Technology Group Ltd.
> + *
> + * 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/io.h>
> +#include <linux/of.h>
> +#include <linux/kernel.h>
> +#include <linux/clk-provider.h>

Please sort includes alphabetically, it will save us some pain
in the future.

> +#include "clk.h"

#include "common.h"

> +static const u8 vcodiv_berlin2[] = {10, 15, 20, 25, 30, 40, 50, 60, 80};
> +
> +static struct berlin_pllmap berlin_pll_map = {
> +	.vcodiv = vcodiv_berlin2,
> +	.fbdiv_mask = 0x1FF,
> +	.rfdiv_mask = 0x1F,
> +	.divsel_mask = 0xF,

divsel_mask allows 16 possible values, vcodiv_berlin2[] only provides
9, and common pll driver below does not know how many valid vcodiv
values are passed. That can become dangerous..

I'd rather extend vcodiv_berlin2 to full divsel range and provide
safe (=1) divisiors. This way wrong/new register values will only
break clock frequency derived.

> +	.fbdiv_shift = 6,
> +	.rfdiv_shift = 1,
> +	.divsel_shift = 7,

Have .foo_mask and .foo_shift together?

> +	.mult = 10,
> +};
> +
> +static void __init berlin2_pll_setup(struct device_node *np)
> +{
> +	berlin_pll_setup(np, &berlin_pll_map);
> +}
> +CLK_OF_DECLARE(berlin2q_pll, "marvell,berlin2-pll", berlin2_pll_setup);
> +
> diff --git a/drivers/clk/berlin/pll-berlin2q.c b/drivers/clk/berlin/pll-berlin2q.c
> new file mode 100644
> index 000000000000..8ddfa69cf8c4
> --- /dev/null
> +++ b/drivers/clk/berlin/pll-berlin2q.c
> @@ -0,0 +1,41 @@
> +/*
> + * Copyright (c) 2013 Marvell Technology Group Ltd.

nit: 2014?

> + * 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/io.h>
> +#include <linux/of.h>
> +#include <linux/kernel.h>
> +#include <linux/clk-provider.h>

same as above.

> +#include "clk.h"

ditto.

> +static const u8 vcodiv_berlin2q[] = {1, 0, 2, 0, 3, 4, 0, 6, 8};
> +
> +static struct berlin_pllmap berlin2q_pll_map = {
> +	.vcodiv = vcodiv_berlin2q,
> +	.fbdiv_mask = 0x1FF,
> +	.rfdiv_mask = 0x1F,
> +	.divsel_mask = 0xF,
> +	.fbdiv_shift = 7,
> +	.rfdiv_shift = 2,
> +	.divsel_shift = 9,
> +	.mult = 1,
> +};

ditto.

> +static void __init berlin2q_pll_setup(struct device_node *np)
> +{
> +	berlin_pll_setup(np, &berlin2q_pll_map);
> +}
> +CLK_OF_DECLARE(berlin2q_pll, "marvell,berlin2q-pll", berlin2q_pll_setup);
> +
> diff --git a/drivers/clk/berlin/pll.c b/drivers/clk/berlin/pll.c
> new file mode 100644
> index 000000000000..188b385af436
> --- /dev/null
> +++ b/drivers/clk/berlin/pll.c
> @@ -0,0 +1,100 @@
> +/*
> + * Copyright (c) 2013 Marvell Technology Group Ltd.

nit: 2014?

> + * 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/io.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/slab.h>
> +#include <linux/kernel.h>
> +#include <linux/clk-provider.h>

same as above.

> +#include "clk.h"

ditto.

> +struct berlin_pll {
> +	struct clk_hw	hw;
> +	void __iomem	*base;
> +	void		*data;

struct berlin2_pllmap *map instead?

> +};
> +
> +#define to_berlin_pll(hw)	container_of(hw, struct berlin_pll, hw)
> +
> +#define PLL_CTRL0	0x00
> +#define PLL_CTRL1	0x04
> +
> +static inline u32 berlin_pll_read(struct berlin_pll *pll, unsigned long offset)
> +{
> +	return readl_relaxed(pll->base + offset);
> +}
> +
> +static unsigned long berlin_pll_recalc_rate(struct clk_hw *hw,
> +					unsigned long parent_rate)

Maybe have a nice little comment on how pll output frequency is
calculated out of the 5 variables?

> +{
> +	struct berlin_pll *pll = to_berlin_pll(hw);
> +	struct berlin_pllmap *map = pll->data;
> +	u32 val, fbdiv, rfdiv, vcodivsel;
> +
> +	val = berlin_pll_read(pll, PLL_CTRL0);
> +	fbdiv = (val >> map->fbdiv_shift) & map->fbdiv_mask;
> +	rfdiv = (val >> map->rfdiv_shift) & map->rfdiv_mask;
> +	if (rfdiv == 0)
> +		rfdiv = 1;
> +
> +	val = berlin_pll_read(pll, PLL_CTRL1);
> +	vcodivsel = (val >> map->divsel_shift) & map->divsel_mask;
> +
> +	return parent_rate * fbdiv * map->mult / rfdiv /
> +		map->vcodiv[vcodivsel];

IMHO

parent_rate *= fbdiv * map->mult;
parent_rate /= rfdiv;
return parent_rate / map->vcodiv[vcodivsel];

makes it slightly more readable, but that is up to you.

> +}
> +
> +static const struct clk_ops berlin_pll_ops = {
> +	.recalc_rate	= berlin_pll_recalc_rate,
> +};
> +
> +void __init berlin_pll_setup(struct device_node *np,
> +			struct berlin_pllmap *map)
> +{
> +	struct clk_init_data init;
> +	struct berlin_pll *pll;
> +	const char *parent_name;
> +	struct clk *clk;
> +	int ret;
> +
> +	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
> +	if (WARN_ON(!pll))
> +		return;
> +
> +	pll->base = of_iomap(np, 0);
> +	if (WARN_ON(!pll->base))
> +		return;
> +
> +	pll->data = map;

pll->map = map;

> +
> +	init.name = np->name;
> +	init.ops = &berlin_pll_ops;
> +	parent_name = of_clk_get_parent_name(np, 0);
> +	init.parent_names = &parent_name;
> +	init.num_parents = 1;
> +
> +	pll->hw.init = &init;
> +
> +	clk = clk_register(NULL, &pll->hw);
> +	if (WARN_ON(IS_ERR(clk)))
> +		return;
> +
> +	ret = of_clk_add_provider(np, of_clk_src_simple_get, clk);
> +	if (WARN_ON(ret))
> +		return;
> +}
> +
>

Re: [PATCH 1/5] clk: berlin: add support for berlin plls

[Alexandre Belloni]

[all commentis I agree on are snipped]

On 21/03/2014 at 13:49:32 +0100, Sebastian Hesselbarth wrote :
> On 03/21/2014 12:43 PM, Alexandre Belloni wrote:
> obj-y += pll.o
> obj-$(CONFIG_MACH_BERLIN_BG2)	+= pll-berlin2.o
> obj-$(CONFIG_MACH_BERLIN_BG2CD)	+= pll-berlin2.o
> obj-$(CONFIG_MACH_BERLIN_BG2Q)	+= pll-berlin2q.o
> 
> Which reminds me, that we forgot to add MACH_BERLIN_BG2Q to
> arch/arm/mach-berlin/Kconfig. Can you spin a patch?
> 

I will do that.

> >+static const u8 vcodiv_berlin2[] = {10, 15, 20, 25, 30, 40, 50, 60, 80};
> >+
> >+static struct berlin_pllmap berlin_pll_map = {
> >+	.vcodiv = vcodiv_berlin2,
> >+	.fbdiv_mask = 0x1FF,
> >+	.rfdiv_mask = 0x1F,
> >+	.divsel_mask = 0xF,
> 
> divsel_mask allows 16 possible values, vcodiv_berlin2[] only provides
> 9, and common pll driver below does not know how many valid vcodiv
> values are passed. That can become dangerous..
> 
> I'd rather extend vcodiv_berlin2 to full divsel range and provide
> safe (=1) divisiors. This way wrong/new register values will only
> break clock frequency derived.
> 

Good catch ! Then, what about simply shrinking the mask so that we don't
overflow the table. We'll put it back to its supposed real value whant
we know what are the remaining divisors (my guess is that they are already
all listed here). I would say that we are getting the divisor wrong if
divsel > 8 anyway.

> >+	.fbdiv_shift = 6,
> >+	.rfdiv_shift = 1,
> >+	.divsel_shift = 7,
> 
> Have .foo_mask and .foo_shift together?
> 

This will make the struct larger but I don't really have an opinion.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 1/5] clk: berlin: add support for berlin plls

[Alexandre Belloni]

On 21/03/2014 at 16:45:39 +0100, Alexandre Belloni wrote :
> > >+	.fbdiv_shift = 6,
> > >+	.rfdiv_shift = 1,
> > >+	.divsel_shift = 7,
> > 
> > Have .foo_mask and .foo_shift together?
> > 
> 
> This will make the struct larger but I don't really have an opinion.
> 

Forget that one, I'll take a break...

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 1/5] clk: berlin: add support for berlin plls

[Sebastian Hesselbarth]

On 03/21/2014 04:45 PM, Alexandre Belloni wrote:
> [all commentis I agree on are snipped]

:)

> On 21/03/2014 at 13:49:32 +0100, Sebastian Hesselbarth wrote :
>> On 03/21/2014 12:43 PM, Alexandre Belloni wrote:
>> obj-y += pll.o
>> obj-$(CONFIG_MACH_BERLIN_BG2)	+= pll-berlin2.o
>> obj-$(CONFIG_MACH_BERLIN_BG2CD)	+= pll-berlin2.o
>> obj-$(CONFIG_MACH_BERLIN_BG2Q)	+= pll-berlin2q.o
>>
>> Which reminds me, that we forgot to add MACH_BERLIN_BG2Q to
>> arch/arm/mach-berlin/Kconfig. Can you spin a patch?
>>
>
> I will do that.
>
>>> +static const u8 vcodiv_berlin2[] = {10, 15, 20, 25, 30, 40, 50, 60, 80};
>>> +
>>> +static struct berlin_pllmap berlin_pll_map = {
>>> +	.vcodiv = vcodiv_berlin2,
>>> +	.fbdiv_mask = 0x1FF,
>>> +	.rfdiv_mask = 0x1F,
>>> +	.divsel_mask = 0xF,
>>
>> divsel_mask allows 16 possible values, vcodiv_berlin2[] only provides
>> 9, and common pll driver below does not know how many valid vcodiv
>> values are passed. That can become dangerous..
>>
>> I'd rather extend vcodiv_berlin2 to full divsel range and provide
>> safe (=1) divisiors. This way wrong/new register values will only
>> break clock frequency derived.
>>
>
> Good catch ! Then, what about simply shrinking the mask so that we don't
> overflow the table. We'll put it back to its supposed real value whant
> we know what are the remaining divisors (my guess is that they are already
> all listed here). I would say that we are getting the divisor wrong if
> divsel > 8 anyway.

Hmm, maybe I should look up valid vcodiv myself, but your vcodiv_berlin2
has 9 values and I guess they are all valid, aren't they?

The next possible, larger mask where 0-8 fits in, is 0xf. You used that
above and that reveals 16 possible indices.

The only option for shrinking the table that I see, would be min/max
allowed indices, but that is as useful as having a slightly larger
table.

>>> +	.fbdiv_shift = 6,
>>> +	.rfdiv_shift = 1,
>>> +	.divsel_shift = 7,
>>
>> Have .foo_mask and .foo_shift together?
>>
>
> This will make the struct larger but I don't really have an opinion.

Maybe, I wasn't clear enough. Just assign .foo_mask and .foo_shift in
subsequent lines of code, i.e.

static struct berlin_pllmap berlin_pll_map = {
	.vcodiv = vcodiv_berlin2,
	.fbdiv_mask = 0x1FF,
	.fbdiv_shift = 6,
	.rfdiv_mask = 0x1F,
	.rfdiv_shift = 1,
	.divsel_mask = 0xF,
	.divsel_shift = 7,
};

Sebastian

Re: [PATCH 1/5] clk: berlin: add support for berlin plls

[Alexandre Belloni]

On 21/03/2014 at 17:03:52 +0100, Sebastian Hesselbarth wrote :
> On 03/21/2014 04:45 PM, Alexandre Belloni wrote:
> >[all commentis I agree on are snipped]
> 
> :)
> 
> >On 21/03/2014 at 13:49:32 +0100, Sebastian Hesselbarth wrote :
> >>On 03/21/2014 12:43 PM, Alexandre Belloni wrote:
> >>obj-y += pll.o
> >>obj-$(CONFIG_MACH_BERLIN_BG2)	+= pll-berlin2.o
> >>obj-$(CONFIG_MACH_BERLIN_BG2CD)	+= pll-berlin2.o
> >>obj-$(CONFIG_MACH_BERLIN_BG2Q)	+= pll-berlin2q.o
> >>
> >>Which reminds me, that we forgot to add MACH_BERLIN_BG2Q to
> >>arch/arm/mach-berlin/Kconfig. Can you spin a patch?
> >>
> >
> >I will do that.
> >
> >>>+static const u8 vcodiv_berlin2[] = {10, 15, 20, 25, 30, 40, 50, 60, 80};
> >>>+
> >>>+static struct berlin_pllmap berlin_pll_map = {
> >>>+	.vcodiv = vcodiv_berlin2,
> >>>+	.fbdiv_mask = 0x1FF,
> >>>+	.rfdiv_mask = 0x1F,
> >>>+	.divsel_mask = 0xF,
> >>
> >>divsel_mask allows 16 possible values, vcodiv_berlin2[] only provides
> >>9, and common pll driver below does not know how many valid vcodiv
> >>values are passed. That can become dangerous..
> >>
> >>I'd rather extend vcodiv_berlin2 to full divsel range and provide
> >>safe (=1) divisiors. This way wrong/new register values will only
> >>break clock frequency derived.
> >>
> >
> >Good catch ! Then, what about simply shrinking the mask so that we don't
> >overflow the table. We'll put it back to its supposed real value whant
> >we know what are the remaining divisors (my guess is that they are already
> >all listed here). I would say that we are getting the divisor wrong if
> >divsel > 8 anyway.
> 
> Hmm, maybe I should look up valid vcodiv myself, but your vcodiv_berlin2
> has 9 values and I guess they are all valid, aren't they?
> 
> The next possible, larger mask where 0-8 fits in, is 0xf. You used that
> above and that reveals 16 possible indices.
> 
> The only option for shrinking the table that I see, would be min/max
> allowed indices, but that is as useful as having a slightly larger
> table.
> 

You are absolutely right :) I definitely need to take a break, right now !

> >>>+	.fbdiv_shift = 6,
> >>>+	.rfdiv_shift = 1,
> >>>+	.divsel_shift = 7,
> >>
> >>Have .foo_mask and .foo_shift together?
> >>
> >
> >This will make the struct larger but I don't really have an opinion.
> 
> Maybe, I wasn't clear enough. Just assign .foo_mask and .foo_shift in
> subsequent lines of code, i.e.
> 
> static struct berlin_pllmap berlin_pll_map = {
> 	.vcodiv = vcodiv_berlin2,
> 	.fbdiv_mask = 0x1FF,
> 	.fbdiv_shift = 6,
> 	.rfdiv_mask = 0x1F,
> 	.rfdiv_shift = 1,
> 	.divsel_mask = 0xF,
> 	.divsel_shift = 7,
> };
> 

yeah, I figured that out a few minutes ago...

Thanks again for your reviews and patience !

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 1/5] clk: berlin: add support for berlin plls

[Alexandre Belloni]

On 21/03/2014 at 13:49:32 +0100, Sebastian Hesselbarth wrote :
> On 03/21/2014 12:43 PM, Alexandre Belloni wrote:
> >diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> >index a367a9831717..4a2602737c27 100644
> >--- a/drivers/clk/Makefile
> >+++ b/drivers/clk/Makefile
> >@@ -29,6 +29,7 @@ obj-$(CONFIG_ARCH_VT8500)		+= clk-vt8500.o
> >  obj-$(CONFIG_COMMON_CLK_WM831X)		+= clk-wm831x.o
> >  obj-$(CONFIG_COMMON_CLK_XGENE)		+= clk-xgene.o
> >  obj-$(CONFIG_COMMON_CLK_AT91)		+= at91/
> >+obj-$(CONFIG_ARCH_BERLIN)		+= berlin/
> >  obj-$(CONFIG_ARCH_HI3xxx)		+= hisilicon/
> >  obj-$(CONFIG_COMMON_CLK_KEYSTONE)	+= keystone/
> 
> I'll have a look at clk/Makefile later myself, but alphabetic ordering
> seems to be broken by ARCH_HI3xxx already. Anyway, we shouldn't just
> add another violator.
> 

BTW,  don't think there is any issue here, the Makefile states:
# please keep this section sorted lexicographically by file/directory path name



-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 1/5] clk: berlin: add support for berlin plls

[Sebastian Hesselbarth]

On 03/21/2014 04:56 PM, Alexandre Belloni wrote:
> On 21/03/2014 at 13:49:32 +0100, Sebastian Hesselbarth wrote :
>> On 03/21/2014 12:43 PM, Alexandre Belloni wrote:
>>> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
>>> index a367a9831717..4a2602737c27 100644
>>> --- a/drivers/clk/Makefile
>>> +++ b/drivers/clk/Makefile
>>> @@ -29,6 +29,7 @@ obj-$(CONFIG_ARCH_VT8500)		+= clk-vt8500.o
>>>   obj-$(CONFIG_COMMON_CLK_WM831X)		+= clk-wm831x.o
>>>   obj-$(CONFIG_COMMON_CLK_XGENE)		+= clk-xgene.o
>>>   obj-$(CONFIG_COMMON_CLK_AT91)		+= at91/
>>> +obj-$(CONFIG_ARCH_BERLIN)		+= berlin/
>>>   obj-$(CONFIG_ARCH_HI3xxx)		+= hisilicon/
>>>   obj-$(CONFIG_COMMON_CLK_KEYSTONE)	+= keystone/
>>
>> I'll have a look at clk/Makefile later myself, but alphabetic ordering
>> seems to be broken by ARCH_HI3xxx already. Anyway, we shouldn't just
>> add another violator.
>>
>
> BTW,  don't think there is any issue here, the Makefile states:
> # please keep this section sorted lexicographically by file/directory path name

Ok, thanks for the clarification. As I said, I should have
looked it up myself in the first place.

Sebastian

[PATCH 2/5] clk: berlin: add berlin clocks DT bindings documentation

[Alexandre Belloni]

Cc: devicetree@vger.kernel.org
Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 .../devicetree/bindings/clock/berlin-clock.txt     | 29 ++++++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/berlin-clock.txt

diff --git a/Documentation/devicetree/bindings/clock/berlin-clock.txt b/Documentation/devicetree/bindings/clock/berlin-clock.txt
new file mode 100644
index 000000000000..ebc78f9b93a9
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/berlin-clock.txt
@@ -0,0 +1,29 @@
+Device Tree Clock bindings for Marvell Berlin clocks
+
+This binding uses the common clock binding[1].
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : shall be one of the following:
+	"marvell,berlin2-pll" or
+	"marvell,berlin2q-pll":
+		CPU PLL and System PLL
+- reg : Address and length of the clock register set.
+- #clock-cells : from common clock binding; shall be set to 0.
+- clocks : shall be the input parent clock phandle for the clock.
+
+smclk: sysmgr-clock {
+	compatible = "fixed-clock";
+	#clock-cells = <0>;
+	clock-frequency = <25000000>;
+};
+
+cpupll: cpupll {
+	compatible = "marvell,berlin2-pll";
+	clocks = <&smclk>;
+	#clock-cells = <0>;
+	reg = <0xf7ea003c 8>;
+};
+
+
-- 
1.8.3.2

Re: [PATCH 2/5] clk: berlin: add berlin clocks DT bindings documentation

[Mark Rutland]

On Fri, Mar 21, 2014 at 11:43:37AM +0000, Alexandre Belloni wrote:
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
>  .../devicetree/bindings/clock/berlin-clock.txt     | 29 ++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/berlin-clock.txt
> 
> diff --git a/Documentation/devicetree/bindings/clock/berlin-clock.txt b/Documentation/devicetree/bindings/clock/berlin-clock.txt
> new file mode 100644
> index 000000000000..ebc78f9b93a9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/berlin-clock.txt
> @@ -0,0 +1,29 @@
> +Device Tree Clock bindings for Marvell Berlin clocks
> +
> +This binding uses the common clock binding[1].
> +
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +Required properties:
> +- compatible : shall be one of the following:
> +	"marvell,berlin2-pll" or
> +	"marvell,berlin2q-pll":
> +		CPU PLL and System PLL
> +- reg : Address and length of the clock register set.
> +- #clock-cells : from common clock binding; shall be set to 0.
> +- clocks : shall be the input parent clock phandle for the clock.

Nit: clocks aren't just phandles. Either define the full type (phandle +
clock-specifier pair), or don't define the type at all given it's a
standard property.

Otherwise this looks fine to me.

Cheers,
Mark.

> +
> +smclk: sysmgr-clock {
> +	compatible = "fixed-clock";
> +	#clock-cells = <0>;
> +	clock-frequency = <25000000>;
> +};
> +
> +cpupll: cpupll {
> +	compatible = "marvell,berlin2-pll";
> +	clocks = <&smclk>;
> +	#clock-cells = <0>;
> +	reg = <0xf7ea003c 8>;
> +};
> +
> +
> -- 
> 1.8.3.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH 2/5] clk: berlin: add berlin clocks DT bindings documentation

[Sebastian Hesselbarth]

On 03/21/2014 12:43 PM, Alexandre Belloni wrote:
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
>   .../devicetree/bindings/clock/berlin-clock.txt     | 29 ++++++++++++++++++++++
>   1 file changed, 29 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/clock/berlin-clock.txt
>
> diff --git a/Documentation/devicetree/bindings/clock/berlin-clock.txt b/Documentation/devicetree/bindings/clock/berlin-clock.txt
> new file mode 100644
> index 000000000000..ebc78f9b93a9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/berlin-clock.txt
> @@ -0,0 +1,29 @@
> +Device Tree Clock bindings for Marvell Berlin clocks
> +
> +This binding uses the common clock binding[1].
> +
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +Required properties:
> +- compatible : shall be one of the following:
> +	"marvell,berlin2-pll" or

nit: just use a comma instead of "or" otherwise that "or" will
be pushed to the new last compatible every time we add a compatible.

> +	"marvell,berlin2q-pll":
> +		CPU PLL and System PLL
> +- reg : Address and length of the clock register set.
> +- #clock-cells : from common clock binding; shall be set to 0.
> +- clocks : shall be the input parent clock phandle for the clock.
> +
> +smclk: sysmgr-clock {
> +	compatible = "fixed-clock";
> +	#clock-cells = <0>;
> +	clock-frequency = <25000000>;
> +};
> +
> +cpupll: cpupll {

Reminder: Should receive an update when you change the corresponding
nodes to cpupll: pll@foo

> +	compatible = "marvell,berlin2-pll";
> +	clocks = <&smclk>;
> +	#clock-cells = <0>;
> +	reg = <0xf7ea003c 8>;
> +};
> +
> +
>

[PATCH 3/5] ARM: berlin/dt: add cpupll and syspll support to BG2Q

[Alexandre Belloni]

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 arch/arm/boot/dts/berlin2q.dtsi | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/berlin2q.dtsi b/arch/arm/boot/dts/berlin2q.dtsi
index 07452a7483fa..19d2c82b0664 100644
--- a/arch/arm/boot/dts/berlin2q.dtsi
+++ b/arch/arm/boot/dts/berlin2q.dtsi
@@ -59,10 +59,26 @@
 		clock-frequency = <100000000>;
 	};
 
+	syspll: syspll {
+		compatible = "marvell,berlin2q-pll";
+		clocks = <&smclk>;
+		#clock-cells = <0>;
+		reg = <0xf7ea0030 8>;
+	};
+
+	cpupll: cpupll {
+		compatible = "marvell,berlin2q-pll";
+		clocks = <&smclk>;
+		#clock-cells = <0>;
+		reg = <0xf7dd0170 8>;
+	};
+
 	cpuclk: cpu-clock {
-		compatible = "fixed-clock";
+		compatible = "fixed-factor-clock";
+		clocks = <&cpupll>;
 		#clock-cells = <0>;
-		clock-frequency = <1200000000>;
+		clock-div = <1>;
+		clock-mult = <1>;
 	};
 
 	twdclk: twdclk {
-- 
1.8.3.2

Re: [PATCH 3/5] ARM: berlin/dt: add cpupll and syspll support to BG2Q

[Sebastian Hesselbarth]

On 03/21/2014 12:43 PM, Alexandre Belloni wrote:

Alexandre,

Thanks for starting this! I'll start with the most obvious
things first and have a closer look on it later.

Missing commit description here.

> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
>   arch/arm/boot/dts/berlin2q.dtsi | 20 ++++++++++++++++++--
>   1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/boot/dts/berlin2q.dtsi b/arch/arm/boot/dts/berlin2q.dtsi
> index 07452a7483fa..19d2c82b0664 100644
> --- a/arch/arm/boot/dts/berlin2q.dtsi
> +++ b/arch/arm/boot/dts/berlin2q.dtsi
> @@ -59,10 +59,26 @@
>   		clock-frequency = <100000000>;
>   	};
>
> +	syspll: syspll {

syspll: pll@ea0030 {

and sort it in between other SoC nodes below. This will
most likely break clocks in v3.14 but v3.15 will receive
proper clock init ordering.

> +		compatible = "marvell,berlin2q-pll";
> +		clocks = <&smclk>;
> +		#clock-cells = <0>;
> +		reg = <0xf7ea0030 8>;
> +	};
> +
> +	cpupll: cpupll {

dito.

> +		compatible = "marvell,berlin2q-pll";
> +		clocks = <&smclk>;
> +		#clock-cells = <0>;
> +		reg = <0xf7dd0170 8>;
> +	};
> +
>   	cpuclk: cpu-clock {
> -		compatible = "fixed-clock";
> +		compatible = "fixed-factor-clock";
> +		clocks = <&cpupll>;
>   		#clock-cells = <0>;
> -		clock-frequency = <1200000000>;
> +		clock-div = <1>;
> +		clock-mult = <1>;

Hmm, you probably know better than me, but if cpuclk == cpupll
is always true we don't need another clk layer here. If you
can scale down cpuclk from cpupll and we just have no driver
for it, I am fine with it.

Sebastian

>   	};
>
>   	twdclk: twdclk {
>

Re: [PATCH 3/5] ARM: berlin/dt: add cpupll and syspll support to BG2Q

[Alexandre Belloni]

On 21/03/2014 at 13:11:29 +0100, Sebastian Hesselbarth wrote :
> On 03/21/2014 12:43 PM, Alexandre Belloni wrote:
> 
> Alexandre,
> 
> Thanks for starting this! I'll start with the most obvious
> things first and have a closer look on it later.
> 

I will rework and wait for your other comments before sending a new
version. BTW, I've set up a branch on github if you want to try on your
platforms:

https://github.com/alexandrebelloni/linux.git topic/berlin-clk

> Missing commit description here.
> 
> >Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> >---
> >  arch/arm/boot/dts/berlin2q.dtsi | 20 ++++++++++++++++++--
> >  1 file changed, 18 insertions(+), 2 deletions(-)
> >
> >diff --git a/arch/arm/boot/dts/berlin2q.dtsi b/arch/arm/boot/dts/berlin2q.dtsi
> >index 07452a7483fa..19d2c82b0664 100644
> >--- a/arch/arm/boot/dts/berlin2q.dtsi
> >+++ b/arch/arm/boot/dts/berlin2q.dtsi
> >@@ -59,10 +59,26 @@
> >  		clock-frequency = <100000000>;
> >  	};
> >
> >+	syspll: syspll {
> 
> syspll: pll@ea0030 {
> 
> and sort it in between other SoC nodes below. This will
> most likely break clocks in v3.14 but v3.15 will receive
> proper clock init ordering.
> 

I will do across all the dtsi, don't bother repeating yourself :)

> >+		compatible = "marvell,berlin2q-pll";
> >+		clocks = <&smclk>;
> >+		#clock-cells = <0>;
> >+		reg = <0xf7ea0030 8>;
> >+	};
> >+
> >+	cpupll: cpupll {
> 
> dito.
> 
> >+		compatible = "marvell,berlin2q-pll";
> >+		clocks = <&smclk>;
> >+		#clock-cells = <0>;
> >+		reg = <0xf7dd0170 8>;
> >+	};
> >+
> >  	cpuclk: cpu-clock {
> >-		compatible = "fixed-clock";
> >+		compatible = "fixed-factor-clock";
> >+		clocks = <&cpupll>;
> >  		#clock-cells = <0>;
> >-		clock-frequency = <1200000000>;
> >+		clock-div = <1>;
> >+		clock-mult = <1>;
> 
> Hmm, you probably know better than me, but if cpuclk == cpupll
> is always true we don't need another clk layer here. If you
> can scale down cpuclk from cpupll and we just have no driver
> for it, I am fine with it.
> 

You can actually switch CPU clk from CPU pll to smclk. I'm not sure this
is completely useful yet though, probably for suspend ?

Also, while I'm not sure this is a good reason, other clocks are derived
from CPU pll and have another divider.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 3/5] ARM: berlin/dt: add cpupll and syspll support to BG2Q

[Sebastian Hesselbarth]

On 03/21/2014 01:17 PM, Alexandre Belloni wrote:
> On 21/03/2014 at 13:11:29 +0100, Sebastian Hesselbarth wrote :
>> On 03/21/2014 12:43 PM, Alexandre Belloni wrote:
>>
>> Alexandre,
>>
>> Thanks for starting this! I'll start with the most obvious
>> things first and have a closer look on it later.
>>
>
> I will rework and wait for your other comments before sending a new
> version. BTW, I've set up a branch on github if you want to try on your
> platforms:
>
> https://github.com/alexandrebelloni/linux.git topic/berlin-clk
>
>> Missing commit description here.
>>
>>> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>>> ---
>>>   arch/arm/boot/dts/berlin2q.dtsi | 20 ++++++++++++++++++--
>>>   1 file changed, 18 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm/boot/dts/berlin2q.dtsi b/arch/arm/boot/dts/berlin2q.dtsi
>>> index 07452a7483fa..19d2c82b0664 100644
>>> --- a/arch/arm/boot/dts/berlin2q.dtsi
>>> +++ b/arch/arm/boot/dts/berlin2q.dtsi
>>> @@ -59,10 +59,26 @@
>>>   		clock-frequency = <100000000>;
>>>   	};
>>>
>>> +	syspll: syspll {
>>
>> syspll: pll@ea0030 {
>>
>> and sort it in between other SoC nodes below. This will
>> most likely break clocks in v3.14 but v3.15 will receive
>> proper clock init ordering.
>>
>
> I will do across all the dtsi, don't bother repeating yourself :)
>
>>> +		compatible = "marvell,berlin2q-pll";
>>> +		clocks = <&smclk>;
>>> +		#clock-cells = <0>;
>>> +		reg = <0xf7ea0030 8>;
>>> +	};
>>> +
>>> +	cpupll: cpupll {
>>
>> dito.
>>
>>> +		compatible = "marvell,berlin2q-pll";
>>> +		clocks = <&smclk>;
>>> +		#clock-cells = <0>;
>>> +		reg = <0xf7dd0170 8>;
>>> +	};
>>> +
>>>   	cpuclk: cpu-clock {
>>> -		compatible = "fixed-clock";
>>> +		compatible = "fixed-factor-clock";
>>> +		clocks = <&cpupll>;
>>>   		#clock-cells = <0>;
>>> -		clock-frequency = <1200000000>;
>>> +		clock-div = <1>;
>>> +		clock-mult = <1>;
>>
>> Hmm, you probably know better than me, but if cpuclk == cpupll
>> is always true we don't need another clk layer here. If you
>> can scale down cpuclk from cpupll and we just have no driver
>> for it, I am fine with it.
>>
>
> You can actually switch CPU clk from CPU pll to smclk. I'm not sure this
> is completely useful yet though, probably for suspend ?

Then it should be clk mux instead?

> Also, while I'm not sure this is a good reason, other clocks are derived
> from CPU pll and have another divider.

I have no strong opinion, but a fixed-factor-clock with 1:1 just to
rename cpupll to cpuclk seems a bit wasty ;)

If there is a mux, we should add it now - no matter if we are ever
going to make any use of it. For the derived clocks we should be
careful if they actually depend on cpuclk or always cpupll.

If your (current) knowledge of the berlin clock trees is almost as
bad as mine, we can also ignore cpuclk mux if you prefer.

Sebastian

Re: [PATCH 3/5] ARM: berlin/dt: add cpupll and syspll support to BG2Q

[Alexandre Belloni]

On 21/03/2014 at 13:29:31 +0100, Sebastian Hesselbarth wrote :
> >>Hmm, you probably know better than me, but if cpuclk == cpupll
> >>is always true we don't need another clk layer here. If you
> >>can scale down cpuclk from cpupll and we just have no driver
> >>for it, I am fine with it.
> >>
> >
> >You can actually switch CPU clk from CPU pll to smclk. I'm not sure this
> >is completely useful yet though, probably for suspend ?
> 
> Then it should be clk mux instead?
> 
> >Also, while I'm not sure this is a good reason, other clocks are derived
> >from CPU pll and have another divider.
> 
> I have no strong opinion, but a fixed-factor-clock with 1:1 just to
> rename cpupll to cpuclk seems a bit wasty ;)
> 
> If there is a mux, we should add it now - no matter if we are ever
> going to make any use of it. For the derived clocks we should be
> careful if they actually depend on cpuclk or always cpupll.
> 
> If your (current) knowledge of the berlin clock trees is almost as
> bad as mine, we can also ignore cpuclk mux if you prefer.
> 

Yeah, fact is I know there is a mux but I don't know yet how to get/set
its state so I will ignore it until we have more info.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 4/5] ARM: berlin/dt: add cpupll and syspll support to BG2CD

[Alexandre Belloni]

This also moves the clocks from the clocks container node to the root.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 arch/arm/boot/dts/berlin2cd.dtsi | 56 ++++++++++++++++++++++++++++------------
 1 file changed, 39 insertions(+), 17 deletions(-)

diff --git a/arch/arm/boot/dts/berlin2cd.dtsi b/arch/arm/boot/dts/berlin2cd.dtsi
index 094968c27533..c84013c1597c 100644
--- a/arch/arm/boot/dts/berlin2cd.dtsi
+++ b/arch/arm/boot/dts/berlin2cd.dtsi
@@ -30,24 +30,46 @@
 		};
 	};
 
-	clocks {
-		smclk: sysmgr-clock {
-			compatible = "fixed-clock";
-			#clock-cells = <0>;
-			clock-frequency = <25000000>;
-		};
+	smclk: sysmgr-clock {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <25000000>;
+	};
 
-		cfgclk: cfg-clock {
-			compatible = "fixed-clock";
-			#clock-cells = <0>;
-			clock-frequency = <75000000>;
-		};
+	cfgclk: cfg-clock {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <75000000>;
+	};
 
-		sysclk: system-clock {
-			compatible = "fixed-clock";
-			#clock-cells = <0>;
-			clock-frequency = <300000000>;
-		};
+	syspll: syspll {
+		compatible = "marvell,berlin2-pll";
+		clocks = <&smclk>;
+		#clock-cells = <0>;
+		reg = <0xf7ea0014 8>;
+	};
+
+	cpupll: cpupll {
+		compatible = "marvell,berlin2-pll";
+		clocks = <&smclk>;
+		#clock-cells = <0>;
+		reg = <0xf7ea003c 8>;
+	};
+
+	cpuclk: cpu-clock {
+		compatible = "fixed-factor-clock";
+		clocks = <&cpupll>;
+		#clock-cells = <0>;
+		clock-div = <1>;
+		clock-mult = <1>;
+	};
+
+	twdclk: twdclk {
+		compatible = "fixed-factor-clock";
+		#clock-cells = <0>;
+		clocks = <&cpuclk>;
+		clock-mult = <1>;
+		clock-div = <3>;
 	};
 
 	soc {
@@ -76,7 +98,7 @@
 			compatible = "arm,cortex-a9-twd-timer";
 			reg = <0xad0600 0x20>;
 			interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH>;
-			clocks = <&sysclk>;
+			clocks = <&twdclk>;
 		};
 
 		apb@e80000 {
-- 
1.8.3.2

Re: [PATCH 4/5] ARM: berlin/dt: add cpupll and syspll support to BG2CD

[Sebastian Hesselbarth]

On 03/21/2014 12:43 PM, Alexandre Belloni wrote:
> This also moves the clocks from the clocks container node to the root.

Please leave a word on the original intention of the patch here, too.

> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
>   arch/arm/boot/dts/berlin2cd.dtsi | 56 ++++++++++++++++++++++++++++------------
>   1 file changed, 39 insertions(+), 17 deletions(-)
>
> diff --git a/arch/arm/boot/dts/berlin2cd.dtsi b/arch/arm/boot/dts/berlin2cd.dtsi
> index 094968c27533..c84013c1597c 100644
> --- a/arch/arm/boot/dts/berlin2cd.dtsi
> +++ b/arch/arm/boot/dts/berlin2cd.dtsi
> @@ -30,24 +30,46 @@
>   		};
>   	};
>
> -	clocks {
> -		smclk: sysmgr-clock {
> -			compatible = "fixed-clock";
> -			#clock-cells = <0>;
> -			clock-frequency = <25000000>;
> -		};
> +	smclk: sysmgr-clock {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <25000000>;
> +	};
>
> -		cfgclk: cfg-clock {
> -			compatible = "fixed-clock";
> -			#clock-cells = <0>;
> -			clock-frequency = <75000000>;
> -		};
> +	cfgclk: cfg-clock {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <75000000>;
> +	};
>
> -		sysclk: system-clock {
> -			compatible = "fixed-clock";
> -			#clock-cells = <0>;
> -			clock-frequency = <300000000>;
> -		};
> +	syspll: syspll {

syspll: pll@ea0014 and sort it in SoC nodes.

> +		compatible = "marvell,berlin2-pll";
> +		clocks = <&smclk>;
> +		#clock-cells = <0>;
> +		reg = <0xf7ea0014 8>;
> +	};
> +
> +	cpupll: cpupll {

ditto.

> +		compatible = "marvell,berlin2-pll";
> +		clocks = <&smclk>;
> +		#clock-cells = <0>;
> +		reg = <0xf7ea003c 8>;
> +	};
> +
> +	cpuclk: cpu-clock {
> +		compatible = "fixed-factor-clock";
> +		clocks = <&cpupll>;
> +		#clock-cells = <0>;
> +		clock-div = <1>;
> +		clock-mult = <1>;

Same comment about cpuclk to cpupll relation.

> +	};
> +
> +	twdclk: twdclk {
> +		compatible = "fixed-factor-clock";
> +		#clock-cells = <0>;
> +		clocks = <&cpuclk>;
> +		clock-mult = <1>;
> +		clock-div = <3>;
>   	};
>
>   	soc {
> @@ -76,7 +98,7 @@
>   			compatible = "arm,cortex-a9-twd-timer";
>   			reg = <0xad0600 0x20>;
>   			interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH>;
> -			clocks = <&sysclk>;
> +			clocks = <&twdclk>;
>   		};
>
>   		apb@e80000 {
>

[PATCH 5/5] ARM: berlin/dt: add cpupll and syspll support to BG2

[Alexandre Belloni]

This also moves the clocks from the clocks container node to the root.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 arch/arm/boot/dts/berlin2.dtsi | 56 +++++++++++++++++++++++++++++-------------
 1 file changed, 39 insertions(+), 17 deletions(-)

diff --git a/arch/arm/boot/dts/berlin2.dtsi b/arch/arm/boot/dts/berlin2.dtsi
index 56a1af2f1052..4b82076ef1ed 100644
--- a/arch/arm/boot/dts/berlin2.dtsi
+++ b/arch/arm/boot/dts/berlin2.dtsi
@@ -37,24 +37,46 @@
 		};
 	};
 
-	clocks {
-		smclk: sysmgr-clock {
-			compatible = "fixed-clock";
-			#clock-cells = <0>;
-			clock-frequency = <25000000>;
-		};
+	smclk: sysmgr-clock {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <25000000>;
+	};
 
-		cfgclk: cfg-clock {
-			compatible = "fixed-clock";
-			#clock-cells = <0>;
-			clock-frequency = <100000000>;
-		};
+	cfgclk: cfg-clock {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <100000000>;
+	};
 
-		sysclk: system-clock {
-			compatible = "fixed-clock";
-			#clock-cells = <0>;
-			clock-frequency = <400000000>;
-		};
+	syspll: syspll {
+		compatible = "marvell,berlin2-pll";
+		clocks = <&smclk>;
+		#clock-cells = <0>;
+		reg = <0xf7ea0014 8>;
+	};
+
+	cpupll: cpupll {
+		compatible = "marvell,berlin2-pll";
+		clocks = <&smclk>;
+		#clock-cells = <0>;
+		reg = <0xf7ea003c 8>;
+	};
+
+	cpuclk: cpu-clock {
+		compatible = "fixed-factor-clock";
+		clocks = <&cpupll>;
+		#clock-cells = <0>;
+		clock-div = <1>;
+		clock-mult = <1>;
+	};
+
+	twdclk: twdclk {
+		compatible = "fixed-factor-clock";
+		#clock-cells = <0>;
+		clocks = <&cpuclk>;
+		clock-mult = <1>;
+		clock-div = <3>;
 	};
 
 	soc {
@@ -83,7 +105,7 @@
 			compatible = "arm,cortex-a9-twd-timer";
 			reg = <0xad0600 0x20>;
 			interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH>;
-			clocks = <&sysclk>;
+			clocks = <&twdclk>;
 		};
 
 		apb@e80000 {
-- 
1.8.3.2

Re: [PATCH 5/5] ARM: berlin/dt: add cpupll and syspll support to BG2

[Sebastian Hesselbarth]

On 03/21/2014 12:43 PM, Alexandre Belloni wrote:
> This also moves the clocks from the clocks container node to the root.

Same comments as for patch 4/5.

> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
>   arch/arm/boot/dts/berlin2.dtsi | 56 +++++++++++++++++++++++++++++-------------
>   1 file changed, 39 insertions(+), 17 deletions(-)
>
> diff --git a/arch/arm/boot/dts/berlin2.dtsi b/arch/arm/boot/dts/berlin2.dtsi
> index 56a1af2f1052..4b82076ef1ed 100644
> --- a/arch/arm/boot/dts/berlin2.dtsi
> +++ b/arch/arm/boot/dts/berlin2.dtsi
> @@ -37,24 +37,46 @@
>   		};
>   	};
>
> -	clocks {
> -		smclk: sysmgr-clock {
> -			compatible = "fixed-clock";
> -			#clock-cells = <0>;
> -			clock-frequency = <25000000>;
> -		};
> +	smclk: sysmgr-clock {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <25000000>;
> +	};
>
> -		cfgclk: cfg-clock {
> -			compatible = "fixed-clock";
> -			#clock-cells = <0>;
> -			clock-frequency = <100000000>;
> -		};
> +	cfgclk: cfg-clock {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <100000000>;
> +	};
>
> -		sysclk: system-clock {
> -			compatible = "fixed-clock";
> -			#clock-cells = <0>;
> -			clock-frequency = <400000000>;
> -		};
> +	syspll: syspll {
> +		compatible = "marvell,berlin2-pll";
> +		clocks = <&smclk>;
> +		#clock-cells = <0>;
> +		reg = <0xf7ea0014 8>;
> +	};
> +
> +	cpupll: cpupll {
> +		compatible = "marvell,berlin2-pll";
> +		clocks = <&smclk>;
> +		#clock-cells = <0>;
> +		reg = <0xf7ea003c 8>;
> +	};
> +
> +	cpuclk: cpu-clock {
> +		compatible = "fixed-factor-clock";
> +		clocks = <&cpupll>;
> +		#clock-cells = <0>;
> +		clock-div = <1>;
> +		clock-mult = <1>;
> +	};
> +
> +	twdclk: twdclk {
> +		compatible = "fixed-factor-clock";
> +		#clock-cells = <0>;
> +		clocks = <&cpuclk>;
> +		clock-mult = <1>;
> +		clock-div = <3>;
>   	};
>
>   	soc {
> @@ -83,7 +105,7 @@
>   			compatible = "arm,cortex-a9-twd-timer";
>   			reg = <0xad0600 0x20>;
>   			interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH>;
> -			clocks = <&sysclk>;
> +			clocks = <&twdclk>;
>   		};
>
>   		apb@e80000 {
>

[PATCH 0/5] Berlin: add clock support

[Alexandre Belloni]

This patch set adds support for the clocks having their own register set. Other
clocks are available but they are sharing a common register in addition of their
own registers. They will be supported in a following series.

Also, I realized that when moving the pll nodes out of the clocks container,
they didn't get reorder.

Alexandre Belloni (5):
  clk: berlin: add support for clocks
  clk: berlin: add berlin clocks DT bindings documentation
  ARM: berlin/dt: add sdio clocks to BG2
  ARM: berlin/dt: add sdio clocks to BG2CD
  ARM: berlin/dt: add sdio clocks to BG2Q

 .../devicetree/bindings/clock/berlin-clock.txt     |   9 +-
 arch/arm/boot/dts/berlin2.dtsi                     |  42 ++++---
 arch/arm/boot/dts/berlin2cd.dtsi                   |  42 ++++---
 arch/arm/boot/dts/berlin2q.dtsi                    |  28 +++--
 drivers/clk/berlin/Makefile                        |   2 +-
 drivers/clk/berlin/clk.c                           | 132 +++++++++++++++++++++
 6 files changed, 218 insertions(+), 37 deletions(-)
 create mode 100644 drivers/clk/berlin/clk.c

-- 
1.9.1

[PATCH 2/5] clk: berlin: add berlin clocks DT bindings documentation

[Alexandre Belloni]

Document the newly added berlin clock driver

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
Cc: devicetree@vger.kernel.org
 Documentation/devicetree/bindings/clock/berlin-clock.txt | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/clock/berlin-clock.txt b/Documentation/devicetree/bindings/clock/berlin-clock.txt
index 49b7860bffb8..00f0053ef587 100644
--- a/Documentation/devicetree/bindings/clock/berlin-clock.txt
+++ b/Documentation/devicetree/bindings/clock/berlin-clock.txt
@@ -9,6 +9,8 @@ Required properties:
 	"marvell,berlin2-pll",
 	"marvell,berlin2q-pll":
 		CPU PLL and System PLL
+	"marvell,berlin2-clk":
+		simple clocks
 - reg: Address and length of the clock register set.
 - #clock-cells: from common clock binding; shall be set to 0.
 - clocks: from common clock binding
@@ -26,4 +28,9 @@ cpupll: cpupll@ea003c {
 	reg = <0xea003c 0x8>;
 };
 
-
+sdio0xinclk: sdio0xinclk@ea023c {
+	compatible = "marvell,berlin-clk";
+	clocks = <&syspll>;
+	#clock-cells = <0>;
+	reg = <0xea023c 4>;
+};
-- 
1.9.1

Re: [PATCH 2/5] clk: berlin: add berlin clocks DT bindings documentation

[Sebastian Hesselbarth]

On 04/23/2014 04:01 PM, Alexandre Belloni wrote:
> Document the newly added berlin clock driver
> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
> Cc: devicetree@vger.kernel.org
>  Documentation/devicetree/bindings/clock/berlin-clock.txt | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/berlin-clock.txt b/Documentation/devicetree/bindings/clock/berlin-clock.txt
> index 49b7860bffb8..00f0053ef587 100644
> --- a/Documentation/devicetree/bindings/clock/berlin-clock.txt
> +++ b/Documentation/devicetree/bindings/clock/berlin-clock.txt
> @@ -9,6 +9,8 @@ Required properties:
>  	"marvell,berlin2-pll",
>  	"marvell,berlin2q-pll":
>  		CPU PLL and System PLL
> +	"marvell,berlin2-clk":
> +		simple clocks
>  - reg: Address and length of the clock register set.
>  - #clock-cells: from common clock binding; shall be set to 0.
>  - clocks: from common clock binding
> @@ -26,4 +28,9 @@ cpupll: cpupll@ea003c {
>  	reg = <0xea003c 0x8>;
>  };
>  
> -
> +sdio0xinclk: sdio0xinclk@ea023c {

Yuck! ;)

Can't we just simply use "clock@<addr>" for the node name and
have the driver compute the name like simple-bus does?

Sebastian

> +	compatible = "marvell,berlin-clk";
> +	clocks = <&syspll>;
> +	#clock-cells = <0>;
> +	reg = <0xea023c 4>;
> +};
>