[PATCH 0/3] clk: mvebu: initial support for AP806 clocks

[PATCH 0/3] clk: mvebu: initial support for AP806 clocks

[Thomas Petazzoni]

Hello,

The following set of patches add initial support for the main clocks
found in the AP806 hardware block, which is the central block used in
the newest Marvell 7K and 8K 64-bits processors.

Two new drivers are added, one for the so-called "core" clocks and one
for the "ring" clocks. The latter are child clocks from one of the
core clocks. The "core" clocks are fixed rate clocks, but their rate
is calculated from values in the Sample At Reset register.

The support for the Marvell 7K/8K platform (Device Tree, etc.) will be
submitted as a separate patch series.

Thanks!

Thomas

Thomas Petazzoni (3):
  clk: unconditionally recurse into clk/mvebu/
  clk: mvebu: add AP806 core clock driver
  clk: mvebu: add AP806 ring clock driver

 .../clock/mvebu-armada-ap806-core-clock.txt        |  33 ++++++
 .../clock/mvebu-armada-ap806-ring-clock.txt        |  32 ++++++
 drivers/clk/Makefile                               |   2 +-
 drivers/clk/mvebu/Kconfig                          |   6 ++
 drivers/clk/mvebu/Makefile                         |   3 +-
 drivers/clk/mvebu/ap806-core.c                     | 112 +++++++++++++++++++++
 drivers/clk/mvebu/ap806-ring.c                     |  65 ++++++++++++
 7 files changed, 251 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/mvebu-armada-ap806-core-clock.txt
 create mode 100644 Documentation/devicetree/bindings/clock/mvebu-armada-ap806-ring-clock.txt
 create mode 100644 drivers/clk/mvebu/ap806-core.c
 create mode 100644 drivers/clk/mvebu/ap806-ring.c

-- 
2.6.4

[PATCH 1/3] clk: unconditionally recurse into clk/mvebu/

[Thomas Petazzoni]

The drivers/clk/mvebu directory is only being built when
CONFIG_PLAT_ORION=y. As we are going to support additional mvebu
platforms in drivers/clk/mvebu, which don't have CONFIG_PLAT_ORION=y,
we need to recurse into this directory regardless of the value of
CONFIG_PLAT_ORION.

Since all files in drivers/clk/mvebu/ are already conditionally
compiled depending on various Kconfig options, we can recurse
unconditionally into drivers/clk/mvebu without any other change.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/clk/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index b038e36..e08bb0e 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -61,7 +61,7 @@ obj-$(CONFIG_ARCH_MEDIATEK)		+= mediatek/
 ifeq ($(CONFIG_COMMON_CLK), y)
 obj-$(CONFIG_ARCH_MMP)			+= mmp/
 endif
-obj-$(CONFIG_PLAT_ORION)		+= mvebu/
+obj-y					+= mvebu/
 obj-$(CONFIG_ARCH_MESON)		+= meson/
 obj-$(CONFIG_ARCH_MXS)			+= mxs/
 obj-$(CONFIG_MACH_PISTACHIO)		+= pistachio/
-- 
2.6.4

[PATCH 1/3] clk: unconditionally recurse into clk/mvebu/

[Marcin Wojtas]

Hi Thomas,



2016-02-15 8:34 GMT+01:00 Thomas Petazzoni
<thomas.petazzoni@free-electrons.com>:
> The drivers/clk/mvebu directory is only being built when
> CONFIG_PLAT_ORION=y. As we are going to support additional mvebu
> platforms in drivers/clk/mvebu, which don't have CONFIG_PLAT_ORION=y,
> we need to recurse into this directory regardless of the value of
> CONFIG_PLAT_ORION.
>
> Since all files in drivers/clk/mvebu/ are already conditionally
> compiled depending on various Kconfig options, we can recurse
> unconditionally into drivers/clk/mvebu without any other change.
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/clk/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index b038e36..e08bb0e 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -61,7 +61,7 @@ obj-$(CONFIG_ARCH_MEDIATEK)           += mediatek/
>  ifeq ($(CONFIG_COMMON_CLK), y)
>  obj-$(CONFIG_ARCH_MMP)                 += mmp/
>  endif
> -obj-$(CONFIG_PLAT_ORION)               += mvebu/
> +obj-y                                  += mvebu/

I know it works with obj-y, but how about using
obj-$(CONFIG_ARCH_MVEBU)? A person editing this Kconfig in future
wouldn't have to run git blame in order to ensure with your commit
log, that he doesn't compile anything more than planned.

Best regards,
Marcin

[PATCH 1/3] clk: unconditionally recurse into clk/mvebu/

[Gregory CLEMENT]

Hi Marcin,
 
 On lun., f?vr. 15 2016, Marcin Wojtas <mw@semihalf.com> wrote:

> Hi Thomas,
>
>
>
> 2016-02-15 8:34 GMT+01:00 Thomas Petazzoni
> <thomas.petazzoni@free-electrons.com>:
>> The drivers/clk/mvebu directory is only being built when
>> CONFIG_PLAT_ORION=y. As we are going to support additional mvebu
>> platforms in drivers/clk/mvebu, which don't have CONFIG_PLAT_ORION=y,
>> we need to recurse into this directory regardless of the value of
>> CONFIG_PLAT_ORION.
>>
>> Since all files in drivers/clk/mvebu/ are already conditionally
>> compiled depending on various Kconfig options, we can recurse
>> unconditionally into drivers/clk/mvebu without any other change.
>>
>> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>> ---
>>  drivers/clk/Makefile | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
>> index b038e36..e08bb0e 100644
>> --- a/drivers/clk/Makefile
>> +++ b/drivers/clk/Makefile
>> @@ -61,7 +61,7 @@ obj-$(CONFIG_ARCH_MEDIATEK)           += mediatek/
>>  ifeq ($(CONFIG_COMMON_CLK), y)
>>  obj-$(CONFIG_ARCH_MMP)                 += mmp/
>>  endif
>> -obj-$(CONFIG_PLAT_ORION)               += mvebu/
>> +obj-y                                  += mvebu/
>
> I know it works with obj-y, but how about using
> obj-$(CONFIG_ARCH_MVEBU)? A person editing this Kconfig in future
> wouldn't have to run git blame in order to ensure with your commit
> log, that he doesn't compile anything more than planned.

I think it is still possible to have CONFIG_PLAT_ORION selected without
having CONFIG_ARCH_MVEBU. So for your concern, adding a comment would be
enough.

Gregory


>
> Best regards,
> Marcin

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 2/3] clk: mvebu: add AP806 core clock driver

[Thomas Petazzoni]

This commit adds a new driver to handle the core clocks found in the
AP806 HW block, which is the core block of all Armada 7K and 8K
Marvell 64-bits processors. This core clock driver reads the
Sample-At-Reset register to determine the frequencies of several core
clocks: DDR, Ring and CPU clocks.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 .../clock/mvebu-armada-ap806-core-clock.txt        |  33 ++++++
 drivers/clk/mvebu/Kconfig                          |   3 +
 drivers/clk/mvebu/Makefile                         |   2 +-
 drivers/clk/mvebu/ap806-core.c                     | 112 +++++++++++++++++++++
 4 files changed, 149 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/clock/mvebu-armada-ap806-core-clock.txt
 create mode 100644 drivers/clk/mvebu/ap806-core.c

diff --git a/Documentation/devicetree/bindings/clock/mvebu-armada-ap806-core-clock.txt b/Documentation/devicetree/bindings/clock/mvebu-armada-ap806-core-clock.txt
new file mode 100644
index 0000000..b2131bb
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/mvebu-armada-ap806-core-clock.txt
@@ -0,0 +1,33 @@
+* Clock bindings for Marvell MVEBU AP806 Core clocks
+
+The Marvell MVEBU Armada 7K/8K SoCs contain a block called AP806,
+hosting the CPU and other core components of the CPU. This Device Tree
+binding allows to describe the core clocks of the AP806, whose
+frequencies are determined by reading the Sample-At-Reset (SAR)
+register.
+
+Clock consumers must specify the desired clock by having the clock ID
+in its "clocks" phandle cell.
+
+The following is a list of provided IDs and clock names for the core
+Armada AP806 clocks:
+
+ 0 = DDR
+ 1 = Ring
+ 2 = CPU
+
+Required properties:
+- compatible: must be be one of the following:
+	"marvell,armada-ap806-core-clock"
+- reg: must be the register address of the Sample-At-Reset (SAR) register
+- #clock-cells: from common clock binding; shall be set to 1
+- clock-output-names: name of the output clocks
+
+Example:
+
+	coreclk: clk at 0x6F8204 {
+		compatible = "marvell,armada-ap806-core-clock";
+		reg = <0x6F8204 0x04>;
+		#clock-cells = <1>;
+		clock-output-names = "ddr", "ring", "cpu";
+	};
diff --git a/drivers/clk/mvebu/Kconfig b/drivers/clk/mvebu/Kconfig
index 2769625..fd84172 100644
--- a/drivers/clk/mvebu/Kconfig
+++ b/drivers/clk/mvebu/Kconfig
@@ -42,3 +42,6 @@ config KIRKWOOD_CLK
 config ORION_CLK
 	bool
 	select MVEBU_CLK_COMMON
+
+config ARMADA_AP806_CORE_CLK
+	bool
diff --git a/drivers/clk/mvebu/Makefile b/drivers/clk/mvebu/Makefile
index 8866115..db5c28c 100644
--- a/drivers/clk/mvebu/Makefile
+++ b/drivers/clk/mvebu/Makefile
@@ -1,11 +1,11 @@
 obj-$(CONFIG_MVEBU_CLK_COMMON)	+= common.o
 obj-$(CONFIG_MVEBU_CLK_CPU) 	+= clk-cpu.o
 obj-$(CONFIG_MVEBU_CLK_COREDIV)	+= clk-corediv.o
-
 obj-$(CONFIG_ARMADA_370_CLK)	+= armada-370.o
 obj-$(CONFIG_ARMADA_375_CLK)	+= armada-375.o
 obj-$(CONFIG_ARMADA_38X_CLK)	+= armada-38x.o
 obj-$(CONFIG_ARMADA_39X_CLK)	+= armada-39x.o
+obj-$(CONFIG_ARMADA_AP806_CORE_CLK) += ap806-core.o
 obj-$(CONFIG_ARMADA_XP_CLK)	+= armada-xp.o
 obj-$(CONFIG_DOVE_CLK)		+= dove.o dove-divider.o
 obj-$(CONFIG_KIRKWOOD_CLK)	+= kirkwood.o
diff --git a/drivers/clk/mvebu/ap806-core.c b/drivers/clk/mvebu/ap806-core.c
new file mode 100644
index 0000000..56c81cd
--- /dev/null
+++ b/drivers/clk/mvebu/ap806-core.c
@@ -0,0 +1,112 @@
+/*
+ * Marvell Armada AP806 core clocks
+ *
+ * Copyright (C) 2016 Marvell
+ *
+ * Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/kernel.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+
+/*
+ * AP806 PLLs:
+ *   0 - DDR
+ *   1 - Ring
+ *   2 - CPU
+ */
+
+#define AP806_PLL_NUM	3
+#define AP806_PLL_FREQ  7
+
+/* SAR parameters to get the PLL data */
+struct apclk_sar {
+	int mask;
+	int offset;
+	const char *name;
+};
+
+static const struct apclk_sar
+ap806_core_clk_sar[AP806_PLL_NUM]  __initconst = {
+	{ .mask = 0x7,	.offset = 21 },
+	{ .mask = 0x7,	.offset = 18 },
+	{ .mask = 0x7,	.offset = 15 },
+};
+
+static struct clk *ap806_core_clks[AP806_PLL_NUM];
+
+static struct clk_onecell_data ap806_core_clk_data = {
+	.clks = ap806_core_clks,
+	.clk_num = AP806_PLL_NUM,
+};
+
+/* mapping between SAR value to frequency */
+static const u32
+ap806_core_clk_freq[AP806_PLL_NUM][AP806_PLL_FREQ] __initconst = {
+	{ 2400000000, 2100000000, 1800000000,
+	  1600000000, 1300000000, 1300000000,
+	  1300000000 },
+	{ 2000000000, 1800000000, 1600000000,
+	  1400000000, 1200000000, 1200000000,
+	  1200000000 },
+	{ 2500000000, 2200000000, 2000000000,
+	  1700000000, 1600000000, 1200000000,
+	  1200000000 },
+};
+
+static unsigned long __init ap806_core_clk_get_freq(u32 reg, int clk_idx)
+{
+	int freq_idx;
+	const struct apclk_sar *clk_info;
+
+	clk_info = &ap806_core_clk_sar[clk_idx];
+
+	freq_idx = (reg >> clk_info->offset) & clk_info->mask;
+	if (WARN_ON(freq_idx > AP806_PLL_FREQ))
+		return 0;
+	else
+		return ap806_core_clk_freq[clk_idx][freq_idx];
+}
+
+static void __init ap806_core_clk_init(struct device_node *np)
+{
+	void __iomem *base;
+	u32 reg;
+	int i;
+
+	base = of_iomap(np, 0);
+	if (WARN_ON(!base))
+		return;
+
+	reg = readl(base);
+
+	iounmap(base);
+
+	for (i = 0; i < AP806_PLL_NUM; i++) {
+		unsigned long freq;
+		const char *name;
+
+		freq = ap806_core_clk_get_freq(reg, i);
+
+		of_property_read_string_index(np, "clock-output-names",
+					      i, &name);
+
+		ap806_core_clks[i] =
+			clk_register_fixed_rate(NULL, name, NULL,
+						CLK_IS_ROOT, freq);
+	}
+
+	of_clk_add_provider(np, of_clk_src_onecell_get,
+			    &ap806_core_clk_data);
+}
+
+CLK_OF_DECLARE(ap806_core_clk, "marvell,armada-ap806-core-clock",
+	       ap806_core_clk_init);
-- 
2.6.4

[PATCH 2/3] clk: mvebu: add AP806 core clock driver

[Rob Herring]

On Mon, Feb 15, 2016 at 08:34:57AM +0100, Thomas Petazzoni wrote:
> This commit adds a new driver to handle the core clocks found in the
> AP806 HW block, which is the core block of all Armada 7K and 8K
> Marvell 64-bits processors. This core clock driver reads the
> Sample-At-Reset register to determine the frequencies of several core
> clocks: DDR, Ring and CPU clocks.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  .../clock/mvebu-armada-ap806-core-clock.txt        |  33 ++++++
>  drivers/clk/mvebu/Kconfig                          |   3 +
>  drivers/clk/mvebu/Makefile                         |   2 +-
>  drivers/clk/mvebu/ap806-core.c                     | 112 +++++++++++++++++++++
>  4 files changed, 149 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/clock/mvebu-armada-ap806-core-clock.txt
>  create mode 100644 drivers/clk/mvebu/ap806-core.c
> 
> diff --git a/Documentation/devicetree/bindings/clock/mvebu-armada-ap806-core-clock.txt b/Documentation/devicetree/bindings/clock/mvebu-armada-ap806-core-clock.txt
> new file mode 100644
> index 0000000..b2131bb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/mvebu-armada-ap806-core-clock.txt
> @@ -0,0 +1,33 @@
> +* Clock bindings for Marvell MVEBU AP806 Core clocks
> +
> +The Marvell MVEBU Armada 7K/8K SoCs contain a block called AP806,
> +hosting the CPU and other core components of the CPU. This Device Tree
> +binding allows to describe the core clocks of the AP806, whose
> +frequencies are determined by reading the Sample-At-Reset (SAR)
> +register.

What else is in the AP806?

> +
> +Clock consumers must specify the desired clock by having the clock ID
> +in its "clocks" phandle cell.
> +
> +The following is a list of provided IDs and clock names for the core
> +Armada AP806 clocks:
> +
> + 0 = DDR
> + 1 = Ring
> + 2 = CPU
> +
> +Required properties:
> +- compatible: must be be one of the following:
> +	"marvell,armada-ap806-core-clock"

I'd expect this to be a sub-node of a syscon block or just a single 
clock provider for the full block. Hard to tell reviewing this without 
context of what the full clock tree looks like. 

> +- reg: must be the register address of the Sample-At-Reset (SAR) register
> +- #clock-cells: from common clock binding; shall be set to 1
> +- clock-output-names: name of the output clocks
> +
> +Example:
> +
> +	coreclk: clk at 0x6F8204 {

Drop 0x and lowercase hex.

> +		compatible = "marvell,armada-ap806-core-clock";
> +		reg = <0x6F8204 0x04>;

lowercase hex.

> +		#clock-cells = <1>;
> +		clock-output-names = "ddr", "ring", "cpu";
> +	};

[PATCH 2/3] clk: mvebu: add AP806 core clock driver

[Thomas Petazzoni]

Dear Rob Herring,

On Sun, 21 Feb 2016 20:53:57 -0600, Rob Herring wrote:

> > +The Marvell MVEBU Armada 7K/8K SoCs contain a block called AP806,
> > +hosting the CPU and other core components of the CPU. This Device Tree
> > +binding allows to describe the core clocks of the AP806, whose
> > +frequencies are determined by reading the Sample-At-Reset (SAR)
> > +register.
> 
> What else is in the AP806?

Lots of things: CPU core, caches, GIC, XOR engines, UART, I2C
controller, etc.

> > +Clock consumers must specify the desired clock by having the clock ID
> > +in its "clocks" phandle cell.
> > +
> > +The following is a list of provided IDs and clock names for the core
> > +Armada AP806 clocks:
> > +
> > + 0 = DDR
> > + 1 = Ring
> > + 2 = CPU
> > +
> > +Required properties:
> > +- compatible: must be be one of the following:
> > +	"marvell,armada-ap806-core-clock"
> 
> I'd expect this to be a sub-node of a syscon block or just a single 
> clock provider for the full block. Hard to tell reviewing this without 
> context of what the full clock tree looks like. 

I indeed wondered about adding a syscon block. However, I don't have at
this point a full datasheet for the AP806, so it is hard to get a good
view of what the register set looks like to create a proper syscon. And
I don't have better informations about the full clock tree.

Such information will come later, and we can rework the drivers and DT
bindings accordingly. Those DT bindings cannot be stable, as the
platform is under heavy development and we'll probably discover some
issues down the road.

Right now, the only information I have about the AP806 clock tree are
about those core clocks and ring clocks.

> > +- reg: must be the register address of the Sample-At-Reset (SAR) register
> > +- #clock-cells: from common clock binding; shall be set to 1
> > +- clock-output-names: name of the output clocks
> > +
> > +Example:
> > +
> > +	coreclk: clk at 0x6F8204 {
> 
> Drop 0x and lowercase hex.
> 
> > +		compatible = "marvell,armada-ap806-core-clock";
> > +		reg = <0x6F8204 0x04>;
> 
> lowercase hex.

Sure, will fix.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 2/3] clk: mvebu: add AP806 core clock driver

[Andrew Lunn]

> Such information will come later, and we can rework the drivers and DT
> bindings accordingly. Those DT bindings cannot be stable, as the
> platform is under heavy development and we'll probably discover some
> issues down the road.

Hi Thomas

Maybe add a big fat warning that the bindings are unstable, and the DT
blob must be kept in sync with the kernel?

     Andrew

[PATCH 2/3] clk: mvebu: add AP806 core clock driver

[Michael Turquette]

Quoting Andrew Lunn (2016-02-22 06:16:17)
> > Such information will come later, and we can rework the drivers and DT
> > bindings accordingly. Those DT bindings cannot be stable, as the
> > platform is under heavy development and we'll probably discover some
> > issues down the road.
> 
> Hi Thomas
> 
> Maybe add a big fat warning that the bindings are unstable, and the DT
> blob must be kept in sync with the kernel?

+1 and feel free to blame the lack of documentation.  No one can expect
bindings to be finalized when the chip topology is not fully understood.

Regards,
Mike

> 
>      Andrew

[PATCH 2/3] clk: mvebu: add AP806 core clock driver

[Rob Herring]

On Mon, Feb 22, 2016 at 12:32 PM, Michael Turquette
<mturquette@baylibre.com> wrote:
> Quoting Andrew Lunn (2016-02-22 06:16:17)
>> > Such information will come later, and we can rework the drivers and DT
>> > bindings accordingly. Those DT bindings cannot be stable, as the
>> > platform is under heavy development and we'll probably discover some
>> > issues down the road.
>>
>> Hi Thomas
>>
>> Maybe add a big fat warning that the bindings are unstable, and the DT
>> blob must be kept in sync with the kernel?
>
> +1 and feel free to blame the lack of documentation.  No one can expect
> bindings to be finalized when the chip topology is not fully understood.

I can understand not understanding the full clock tree. I have "full"
documentation of a Marvell chip and don't understand the clock tree
fully. But I can't believe you don't have some sense of how many
clocks you have to deal with. 10? 100? 1000? What I see is 2 nodes of
a single register each for clocks at roughly the same address. That
tells me your binding is too fine grained. If you really don't know
what is right, then err on the side of a single clock provider node
and don't put the clock details in DT.

Rob

[PATCH 2/3] clk: mvebu: add AP806 core clock driver

[Michael Turquette]

Quoting Rob Herring (2016-02-22 11:42:49)
> On Mon, Feb 22, 2016 at 12:32 PM, Michael Turquette
> <mturquette@baylibre.com> wrote:
> > Quoting Andrew Lunn (2016-02-22 06:16:17)
> >> > Such information will come later, and we can rework the drivers and DT
> >> > bindings accordingly. Those DT bindings cannot be stable, as the
> >> > platform is under heavy development and we'll probably discover some
> >> > issues down the road.
> >>
> >> Hi Thomas
> >>
> >> Maybe add a big fat warning that the bindings are unstable, and the DT
> >> blob must be kept in sync with the kernel?
> >
> > +1 and feel free to blame the lack of documentation.  No one can expect
> > bindings to be finalized when the chip topology is not fully understood.
> 
> I can understand not understanding the full clock tree. I have "full"
> documentation of a Marvell chip and don't understand the clock tree
> fully. But I can't believe you don't have some sense of how many
> clocks you have to deal with. 10? 100? 1000? What I see is 2 nodes of
> a single register each for clocks at roughly the same address. That
> tells me your binding is too fine grained. If you really don't know
> what is right, then err on the side of a single clock provider node
> and don't put the clock details in DT.

I like the "err on the side of caution" part. I'll add that to
clock-bindings.txt.

Regards,
Mike

> 
> Rob

[PATCH 2/3] clk: mvebu: add AP806 core clock driver

[Thomas Petazzoni]

Hello,

On Mon, 22 Feb 2016 13:42:49 -0600, Rob Herring wrote:

> >> Maybe add a big fat warning that the bindings are unstable, and the DT
> >> blob must be kept in sync with the kernel?
> >
> > +1 and feel free to blame the lack of documentation.  No one can expect
> > bindings to be finalized when the chip topology is not fully understood.
> 
> I can understand not understanding the full clock tree. I have "full"
> documentation of a Marvell chip and don't understand the clock tree
> fully. But I can't believe you don't have some sense of how many
> clocks you have to deal with. 10? 100? 1000? What I see is 2 nodes of
> a single register each for clocks at roughly the same address. That
> tells me your binding is too fine grained. If you really don't know
> what is right, then err on the side of a single clock provider node
> and don't put the clock details in DT.

I don't quite understand the reasoning behind this conclusion. We know
for sure that those two registers control only those core and ring
clocks. Maybe there are other registers controlling other clocks,  that
we don't know. But for sure, those two registers only give details
about those core and ring clocks, so I don't see what would be the
usefulness of merging that into a single DT node.

We would lose the fact that the relationship between the ring clocks
and one of the core clock is represented in the DT. Instead of having a
clear <&coreclk X> or <&ringclk Y> reference, we would have a
mysterious <&allclk XYZ> reference, which doesn't tell immediately
whether it's a core clock or ring clock.

So, while I definitely agree that a syscon is probably in order to
cover all the system registers, I don't see the point of having a
single node to cover all the clocks.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 2/3] clk: mvebu: add AP806 core clock driver

[Thomas Petazzoni]

Rob, Michael, Stephen,

On Tue, 23 Feb 2016 10:25:50 +0100, Thomas Petazzoni wrote:

> > >> Maybe add a big fat warning that the bindings are unstable, and the DT
> > >> blob must be kept in sync with the kernel?
> > >
> > > +1 and feel free to blame the lack of documentation.  No one can expect
> > > bindings to be finalized when the chip topology is not fully understood.
> > 
> > I can understand not understanding the full clock tree. I have "full"
> > documentation of a Marvell chip and don't understand the clock tree
> > fully. But I can't believe you don't have some sense of how many
> > clocks you have to deal with. 10? 100? 1000? What I see is 2 nodes of
> > a single register each for clocks at roughly the same address. That
> > tells me your binding is too fine grained. If you really don't know
> > what is right, then err on the side of a single clock provider node
> > and don't put the clock details in DT.
> 
> I don't quite understand the reasoning behind this conclusion. We know
> for sure that those two registers control only those core and ring
> clocks. Maybe there are other registers controlling other clocks,  that
> we don't know. But for sure, those two registers only give details
> about those core and ring clocks, so I don't see what would be the
> usefulness of merging that into a single DT node.
> 
> We would lose the fact that the relationship between the ring clocks
> and one of the core clock is represented in the DT. Instead of having a
> clear <&coreclk X> or <&ringclk Y> reference, we would have a
> mysterious <&allclk XYZ> reference, which doesn't tell immediately
> whether it's a core clock or ring clock.
> 
> So, while I definitely agree that a syscon is probably in order to
> cover all the system registers, I don't see the point of having a
> single node to cover all the clocks.

So, I had a discussion with Marvell engineers and a closer look at the
matter. It turns out that those two clock registers are in the middle
of an area called DFX (Design for Testability), which contain registers
to fine tune the silicon and detect manufacturing issues. This area of
registers is most likely never going to be publicly documented, and the
fact that those two clock-related registers were placed there was more
an oversight than a real architecture choice.

For this reason, there is in fact no real benefit in mapping the entire
area using a syscon, and having the two DT nodes for the core clocks
and ring clocks, mapping just their own registers is by far the
simplest solution considering the register layout.

We would prefer to keep things as proposed in terms of DT
representation, with the understanding that the submission of the Linux
support for this platform comes very early in the chip development
cycle, and that as such, the DT bindings should be considered unstable.

Thanks a lot,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 3/3] clk: mvebu: add AP806 ring clock driver

[Thomas Petazzoni]

This commit adds a new driver to handle the ring clocks found in the
AP806 HW block, which is the core block of all Armada 7K and 8K
Marvell 64-bits processors. Those ring clocks are derived from the
core ring clock handled by the AP806 core clock driver. The ring
clocks are used by various peripherals inside the AP806.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 .../clock/mvebu-armada-ap806-ring-clock.txt        | 32 +++++++++++
 drivers/clk/mvebu/Kconfig                          |  3 +
 drivers/clk/mvebu/Makefile                         |  1 +
 drivers/clk/mvebu/ap806-ring.c                     | 65 ++++++++++++++++++++++
 4 files changed, 101 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/mvebu-armada-ap806-ring-clock.txt
 create mode 100644 drivers/clk/mvebu/ap806-ring.c

diff --git a/Documentation/devicetree/bindings/clock/mvebu-armada-ap806-ring-clock.txt b/Documentation/devicetree/bindings/clock/mvebu-armada-ap806-ring-clock.txt
new file mode 100644
index 0000000..5602bdc
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/mvebu-armada-ap806-ring-clock.txt
@@ -0,0 +1,32 @@
+* Clock bindings for Marvell MVEBU AP806 Ring clocks
+
+The Marvell MVEBU Armada 7K/8K SoCs contain a block called AP806,
+hosting the CPU and other core components of the CPU. This Device Tree
+binding allows to describe the ring clocks of the AP806, which are
+derived from the Ring Core clock, after a dividing factor.
+
+The following is a list of provided IDs and clock names on Armada
+AP806 RING dividers:
+
+ 0 = Ring 0
+ 1 = Ring 2
+ 2 = Ring 3
+ 3 = Ring 4
+ 4 = Ring 5
+
+Required properties:
+- compatible: must be one of the following:
+	"marvell,armada-ap806-ring-clock"
+- reg: must be the register holding the divider values for ring clocks
+- #clock-cells : from common clock binding; shall be set to 1
+- clock-output-names: name of the output clocks
+
+Example:
+
+	ringclk: clk at 0x6F8250 {
+		compatible = "marvell,armada-ap806-ring-clock";
+		reg = <0x6F8250 0x04>;
+		#clock-cells = <1>;
+		clock-output-names = "ring-0", "ring-2", "ring-3", "ring-4", "ring-5";
+		clocks = <&coreclk 1>;
+	};
diff --git a/drivers/clk/mvebu/Kconfig b/drivers/clk/mvebu/Kconfig
index fd84172..5d30aac 100644
--- a/drivers/clk/mvebu/Kconfig
+++ b/drivers/clk/mvebu/Kconfig
@@ -45,3 +45,6 @@ config ORION_CLK
 
 config ARMADA_AP806_CORE_CLK
 	bool
+
+config ARMADA_AP806_RING_CLK
+	bool
diff --git a/drivers/clk/mvebu/Makefile b/drivers/clk/mvebu/Makefile
index db5c28c..48ca2d3 100644
--- a/drivers/clk/mvebu/Makefile
+++ b/drivers/clk/mvebu/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_ARMADA_375_CLK)	+= armada-375.o
 obj-$(CONFIG_ARMADA_38X_CLK)	+= armada-38x.o
 obj-$(CONFIG_ARMADA_39X_CLK)	+= armada-39x.o
 obj-$(CONFIG_ARMADA_AP806_CORE_CLK) += ap806-core.o
+obj-$(CONFIG_ARMADA_AP806_RING_CLK) += ap806-ring.o
 obj-$(CONFIG_ARMADA_XP_CLK)	+= armada-xp.o
 obj-$(CONFIG_DOVE_CLK)		+= dove.o dove-divider.o
 obj-$(CONFIG_KIRKWOOD_CLK)	+= kirkwood.o
diff --git a/drivers/clk/mvebu/ap806-ring.c b/drivers/clk/mvebu/ap806-ring.c
new file mode 100644
index 0000000..154ee55
--- /dev/null
+++ b/drivers/clk/mvebu/ap806-ring.c
@@ -0,0 +1,65 @@
+/*
+ * Marvell Armada AP806 ring clocks
+ *
+ * Copyright (C) 2016 Marvell
+ *
+ * Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/kernel.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+
+#define AP806_RING_DIV_NUM	5
+
+static struct clk *ap806_ring_clks[AP806_RING_DIV_NUM];
+
+static struct clk_onecell_data ap806_ring_clk_data = {
+	.clks = ap806_ring_clks,
+	.clk_num = AP806_RING_DIV_NUM,
+};
+
+static void __init ap806_ring_clk_init(struct device_node *np)
+{
+	void __iomem *base;
+	const char *parent;
+	u32 reg;
+	int i;
+
+	base = of_iomap(np, 0);
+	if (WARN_ON(!base))
+		return;
+
+	reg = readl(base);
+
+	iounmap(base);
+
+	parent = of_clk_get_parent_name(np, 0);
+
+	for (i = 0; i < AP806_RING_DIV_NUM; i++) {
+		unsigned long divider;
+		const char *name;
+
+		/* Each clock is represented by 6 bits */
+		divider = (reg >> (6 * i)) & 0x3f;
+
+		of_property_read_string_index(np, "clock-output-names",
+					      i, &name);
+
+		ap806_ring_clks[i] =
+			clk_register_fixed_factor(NULL, name, parent,
+						  0, 1, divider);
+	}
+
+	of_clk_add_provider(np, of_clk_src_onecell_get, &ap806_ring_clk_data);
+}
+
+CLK_OF_DECLARE(ap806_ring_clk, "marvell,armada-ap806-ring-clock",
+	       ap806_ring_clk_init);
-- 
2.6.4

[PATCH 3/3] clk: mvebu: add AP806 ring clock driver

[Rob Herring]

On Mon, Feb 15, 2016 at 08:34:58AM +0100, Thomas Petazzoni wrote:
> This commit adds a new driver to handle the ring clocks found in the
> AP806 HW block, which is the core block of all Armada 7K and 8K
> Marvell 64-bits processors. Those ring clocks are derived from the
> core ring clock handled by the AP806 core clock driver. The ring
> clocks are used by various peripherals inside the AP806.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  .../clock/mvebu-armada-ap806-ring-clock.txt        | 32 +++++++++++

Combine this with the other patch. Similar comments as the other one.

>  drivers/clk/mvebu/Kconfig                          |  3 +
>  drivers/clk/mvebu/Makefile                         |  1 +
>  drivers/clk/mvebu/ap806-ring.c                     | 65 ++++++++++++++++++++++
>  4 files changed, 101 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/mvebu-armada-ap806-ring-clock.txt
>  create mode 100644 drivers/clk/mvebu/ap806-ring.c

[PATCH 3/3] clk: mvebu: add AP806 ring clock driver

[Thomas Petazzoni]

Dear Rob Herring,

On Sun, 21 Feb 2016 20:54:01 -0600, Rob Herring wrote:
> On Mon, Feb 15, 2016 at 08:34:58AM +0100, Thomas Petazzoni wrote:
> > This commit adds a new driver to handle the ring clocks found in the
> > AP806 HW block, which is the core block of all Armada 7K and 8K
> > Marvell 64-bits processors. Those ring clocks are derived from the
> > core ring clock handled by the AP806 core clock driver. The ring
> > clocks are used by various peripherals inside the AP806.
> > 
> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > ---
> >  .../clock/mvebu-armada-ap806-ring-clock.txt        | 32 +++++++++++
> 
> Combine this with the other patch. Similar comments as the other one.

What do you mean by "combine"? It certainly doesn't make sense to merge
the two patches: there is one patch per driver, so the DT binding
documentation comes together with the driver it corresponds to.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 0/3] clk: mvebu: initial support for AP806 clocks

[Michael Turquette]

Hi Thomas,

Quoting Thomas Petazzoni (2016-02-14 23:34:55)
> Hello,
> 
> The following set of patches add initial support for the main clocks
> found in the AP806 hardware block, which is the central block used in
> the newest Marvell 7K and 8K 64-bits processors.
> 
> Two new drivers are added, one for the so-called "core" clocks and one
> for the "ring" clocks. The latter are child clocks from one of the
> core clocks. The "core" clocks are fixed rate clocks, but their rate
> is calculated from values in the Sample At Reset register.
> 
> The support for the Marvell 7K/8K platform (Device Tree, etc.) will be
> submitted as a separate patch series.

Yay, new SoC! Just FYI, we worked out an agreement in December[0] that
the clk git tree can take DT bindings and headers, and we'll always host
them in an immutable branch by default in case arm-soc (or anyone else)
needs them. Stephen and I are doing this now.

It's nice for us to review those bindings at the same time that we
review the driver.

[0] http://lkml.kernel.org/r/<20151231001859.19557.99512@quark.deferred.io>

Best regards,
Mike

> 
> Thanks!
> 
> Thomas
> 
> Thomas Petazzoni (3):
>   clk: unconditionally recurse into clk/mvebu/
>   clk: mvebu: add AP806 core clock driver
>   clk: mvebu: add AP806 ring clock driver
> 
>  .../clock/mvebu-armada-ap806-core-clock.txt        |  33 ++++++
>  .../clock/mvebu-armada-ap806-ring-clock.txt        |  32 ++++++
>  drivers/clk/Makefile                               |   2 +-
>  drivers/clk/mvebu/Kconfig                          |   6 ++
>  drivers/clk/mvebu/Makefile                         |   3 +-
>  drivers/clk/mvebu/ap806-core.c                     | 112 +++++++++++++++++++++
>  drivers/clk/mvebu/ap806-ring.c                     |  65 ++++++++++++
>  7 files changed, 251 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/clock/mvebu-armada-ap806-core-clock.txt
>  create mode 100644 Documentation/devicetree/bindings/clock/mvebu-armada-ap806-ring-clock.txt
>  create mode 100644 drivers/clk/mvebu/ap806-core.c
>  create mode 100644 drivers/clk/mvebu/ap806-ring.c
> 
> -- 
> 2.6.4
> 

[PATCH 0/3] clk: mvebu: initial support for AP806 clocks

[Michael Turquette]

Thomas,

On Mon, Feb 15, 2016 at 3:31 PM, Michael Turquette
<mturquette@baylibre.com> wrote:
> Hi Thomas,
>
> Quoting Thomas Petazzoni (2016-02-14 23:34:55)
>> Hello,
>>
>> The following set of patches add initial support for the main clocks
>> found in the AP806 hardware block, which is the central block used in
>> the newest Marvell 7K and 8K 64-bits processors.
>>
>> Two new drivers are added, one for the so-called "core" clocks and one
>> for the "ring" clocks. The latter are child clocks from one of the
>> core clocks. The "core" clocks are fixed rate clocks, but their rate
>> is calculated from values in the Sample At Reset register.
>>
>> The support for the Marvell 7K/8K platform (Device Tree, etc.) will be
>> submitted as a separate patch series.
>
> Yay, new SoC! Just FYI, we worked out an agreement in December[0] that
> the clk git tree can take DT bindings and headers, and we'll always host
> them in an immutable branch by default in case arm-soc (or anyone else)
> needs them. Stephen and I are doing this now.

Oops, I misunderstood your point (and I should have looked at the
patches before replying). The patches in this series do indeed have
the clk DT binding descriptions.

Pardon the noise, nothing to see here, move along ;-)

Regards,
Mike

>
> It's nice for us to review those bindings at the same time that we
> review the driver.
>
> [0] http://lkml.kernel.org/r/<20151231001859.19557.99512@quark.deferred.io>
>
> Best regards,
> Mike
>
>>
>> Thanks!
>>
>> Thomas
>>
>> Thomas Petazzoni (3):
>>   clk: unconditionally recurse into clk/mvebu/
>>   clk: mvebu: add AP806 core clock driver
>>   clk: mvebu: add AP806 ring clock driver
>>
>>  .../clock/mvebu-armada-ap806-core-clock.txt        |  33 ++++++
>>  .../clock/mvebu-armada-ap806-ring-clock.txt        |  32 ++++++
>>  drivers/clk/Makefile                               |   2 +-
>>  drivers/clk/mvebu/Kconfig                          |   6 ++
>>  drivers/clk/mvebu/Makefile                         |   3 +-
>>  drivers/clk/mvebu/ap806-core.c                     | 112 +++++++++++++++++++++
>>  drivers/clk/mvebu/ap806-ring.c                     |  65 ++++++++++++
>>  7 files changed, 251 insertions(+), 2 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/clock/mvebu-armada-ap806-core-clock.txt
>>  create mode 100644 Documentation/devicetree/bindings/clock/mvebu-armada-ap806-ring-clock.txt
>>  create mode 100644 drivers/clk/mvebu/ap806-core.c
>>  create mode 100644 drivers/clk/mvebu/ap806-ring.c
>>
>> --
>> 2.6.4
>>



-- 
Michael Turquette
CEO
BayLibre - At the Heart of Embedded Linux
http://baylibre.com/

[PATCH 0/3] clk: mvebu: initial support for AP806 clocks

[Thomas Petazzoni]

Hello Michael,

On Mon, 15 Feb 2016 15:36:49 -0800, Michael Turquette wrote:

> > Yay, new SoC! Just FYI, we worked out an agreement in December[0] that
> > the clk git tree can take DT bindings and headers, and we'll always host
> > them in an immutable branch by default in case arm-soc (or anyone else)
> > needs them. Stephen and I are doing this now.
> 
> Oops, I misunderstood your point (and I should have looked at the
> patches before replying). The patches in this series do indeed have
> the clk DT binding descriptions.
> 
> Pardon the noise, nothing to see here, move along ;-)

Yes, each driver comes with its DT binding documentation in the same
patch. Unfortunately, some maintainers prefer to have the DT binding
documentation as separate patches from the driver itself, while some
other maintainers prefer to have both in the same patch. And I never
remember which maintainer has which preference.

So, let me know if you're fine with having both in the same patches. If
not, I can respin by splitting the DT binding documentation in its own
patches.

Other than that, any comments on the drivers themselves? Since they are
quite trivial, I would ideally like to see them in 4.6, if at all
possible.

Thanks a lot!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com