[PATCH v4 0/5] clk: mvebu: clock drivers for Marvell Armada 7K/8K

[PATCH v4 0/5] clk: mvebu: clock drivers for Marvell Armada 7K/8K

[Thomas Petazzoni]

Hello,

Here is a new version of the clock drivers for the Marvell Armada
7K/8K. Changes since v3:

 - Addition of a driver for the CP110 system controller, which
   provides clocks. The CP110 is the other big HW block contained in
   the Armada 7K/8K, next to the AP806.

 - Everything rebase on v4.6-rc1.

 - Addition of Rob Herring Acked-by on the AP806 DT bindings.

 - Minor adjustements to the A806 DT binding documentation: the
   proposed clock names have been changed with a "ap-" suffix, so that
   we don't have any naming conflicts between the clocks provided by
   the AP806 and the clocks provided by the CP110. Since only the
   proposed clock names were changed, I kept Rob Herring
   Acked-by. Rob, let me know if there is any issue with this.

Thanks!

Thomas

Thomas Petazzoni (5):
  clk: unconditionally recurse into clk/mvebu/
  dt-bindings: arm: add DT binding for Marvell AP806 system controller
  clk: mvebu: new driver for Armada AP806 system controller
  dt-bindings: arm: add DT binding for Marvell CP110 system controller
  clk: mvebu: new driver for Armada CP110 system controller

 .../arm/marvell/ap806-system-controller.txt        |  35 +++
 .../arm/marvell/cp110-system-controller0.txt       |  88 +++++++
 drivers/clk/Makefile                               |   2 +-
 drivers/clk/mvebu/Kconfig                          |   6 +
 drivers/clk/mvebu/Makefile                         |   2 +
 drivers/clk/mvebu/ap806-system-controller.c        | 106 +++++++++
 drivers/clk/mvebu/cp110-system-controller.c        | 265 +++++++++++++++++++++
 7 files changed, 503 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/arm/marvell/ap806-system-controller.txt
 create mode 100644 Documentation/devicetree/bindings/arm/marvell/cp110-system-controller0.txt
 create mode 100644 drivers/clk/mvebu/ap806-system-controller.c
 create mode 100644 drivers/clk/mvebu/cp110-system-controller.c

-- 
2.6.4

[PATCH v4 1/5] 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 46869d69..f9ad66e 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

Re: [PATCH v4 1/5] clk: unconditionally recurse into clk/mvebu/

[Stephen Boyd]

On 03/27, Thomas Petazzoni wrote:
> 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>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v4 2/5] dt-bindings: arm: add DT binding for Marvell AP806 system controller

[Thomas Petazzoni]

This commit adds the Device Tree binding documentation for the system
controller found in Marvell AP806 HW block, which is one of the core
HW blocks of the 64-bits Marvell Armada 7K/8K family.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../arm/marvell/ap806-system-controller.txt        | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/marvell/ap806-system-controller.txt

diff --git a/Documentation/devicetree/bindings/arm/marvell/ap806-system-controller.txt b/Documentation/devicetree/bindings/arm/marvell/ap806-system-controller.txt
new file mode 100644
index 0000000..8968371
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/marvell/ap806-system-controller.txt
@@ -0,0 +1,35 @@
+Marvell Armada AP806 System Controller
+======================================
+
+The AP806 is one of the two core HW blocks of the Marvell Armada 7K/8K
+SoCs. It contains a system controller, which provides a number
+registers giving access to numerous features: clocks, pin-muxing and
+many other SoC configuration items. This DT binding allows to describe
+this system controller.
+
+The Device Tree node representing the AP806 system controller provides
+a number of clocks:
+
+ - 0: clock of CPU cluster 0
+ - 1: clock of CPU cluster 1
+ - 2: fixed PLL at 1200 Mhz
+ - 3: MSS clock, derived from the fixed PLL
+
+Required properties:
+
+ - compatible: must be:
+     "marvell,ap806-system-controller", "syscon"
+ - reg: register area of the AP806 system controller
+ - #clock-cells: must be set to 1
+ - clock-output-names: must be defined to:
+    "ap-cpu-cluster-0", "ap-cpu-cluster-1", "ap-fixed", "ap-mss"
+
+Example:
+
+	syscon: system-controller@6f4000 {
+		compatible = "marvell,ap806-system-controller", "syscon";
+		#clock-cells = <1>;
+		clock-output-names = "ap-cpu-cluster-0", "ap-cpu-cluster-1",
+				     "ap-fixed", "ap-mss";
+		reg = <0x6f4000 0x1000>;
+	};
-- 
2.6.4

Re: [PATCH v4 2/5] dt-bindings: arm: add DT binding for Marvell AP806 system controller

[Stephen Boyd]

On 03/27, Thomas Petazzoni wrote:
> This commit adds the Device Tree binding documentation for the system
> controller found in Marvell AP806 HW block, which is one of the core
> HW blocks of the 64-bits Marvell Armada 7K/8K family.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Acked-by: Rob Herring <robh@kernel.org>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v4 3/5] clk: mvebu: new driver for Armada AP806 system controller

[Thomas Petazzoni]

The Armada AP806 system controller, amongst other things, provides a
number of clocks for the platform: the CPU cluster clocks, whose
frequencies are found by reading the Sample At Reset register, one
fixed clock, and another clock derived from the fixed clock, which is
the one used by most peripherals in AP806.

The AP806 is one of the two core HW blocks used in the Marvell 7K/8K
SoCs.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/clk/mvebu/Kconfig                   |   3 +
 drivers/clk/mvebu/Makefile                  |   1 +
 drivers/clk/mvebu/ap806-system-controller.c | 106 ++++++++++++++++++++++++++++
 3 files changed, 110 insertions(+)
 create mode 100644 drivers/clk/mvebu/ap806-system-controller.c

diff --git a/drivers/clk/mvebu/Kconfig b/drivers/clk/mvebu/Kconfig
index eaee8f0..bf7ae00 100644
--- a/drivers/clk/mvebu/Kconfig
+++ b/drivers/clk/mvebu/Kconfig
@@ -29,6 +29,9 @@ config ARMADA_XP_CLK
 	select MVEBU_CLK_COMMON
 	select MVEBU_CLK_CPU
 
+config ARMADA_AP806_SYSCON
+	bool
+
 config DOVE_CLK
 	bool
 	select MVEBU_CLK_COMMON
diff --git a/drivers/clk/mvebu/Makefile b/drivers/clk/mvebu/Makefile
index 8866115..f4aa481 100644
--- a/drivers/clk/mvebu/Makefile
+++ b/drivers/clk/mvebu/Makefile
@@ -7,6 +7,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_XP_CLK)	+= armada-xp.o
+obj-$(CONFIG_ARMADA_AP806_SYSCON) += ap806-system-controller.o
 obj-$(CONFIG_DOVE_CLK)		+= dove.o dove-divider.o
 obj-$(CONFIG_KIRKWOOD_CLK)	+= kirkwood.o
 obj-$(CONFIG_ORION_CLK)		+= orion.o
diff --git a/drivers/clk/mvebu/ap806-system-controller.c b/drivers/clk/mvebu/ap806-system-controller.c
new file mode 100644
index 0000000..c33c209
--- /dev/null
+++ b/drivers/clk/mvebu/ap806-system-controller.c
@@ -0,0 +1,106 @@
+/*
+ * Marvell Armada AP806 System Controller
+ *
+ * Copyright (C) 2016 Marvell
+ *
+ * Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
+ *
+ * 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.
+ */
+
+#define pr_fmt(fmt) "ap806-system-controller: " fmt
+
+#include <linux/kernel.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/io.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/regmap.h>
+
+#define AP806_SAR_REG			0x400
+#define AP806_SAR_CLKFREQ_MODE_MASK	0x1f
+
+#define AP806_CLK_NUM 			4
+
+static struct clk *ap806_clks[AP806_CLK_NUM];
+
+static struct clk_onecell_data ap806_clk_data = {
+	.clks = ap806_clks,
+	.clk_num = AP806_CLK_NUM,
+};
+
+static void __init ap806_syscon_clk_init(struct device_node *np)
+{
+	unsigned int freq_mode, cpuclk_freq;
+	const char *name, *fixedclk_name;
+	struct regmap *regmap;
+	u32 reg;
+
+	regmap = syscon_node_to_regmap(np);
+	if (IS_ERR(regmap)) {
+		pr_err("cannot get regmap\n");
+		return;
+	}
+
+	if (regmap_read(regmap, AP806_SAR_REG, &reg)) {
+		pr_err("cannot read from regmap\n");
+		return;
+	}
+
+	freq_mode = reg & AP806_SAR_CLKFREQ_MODE_MASK;
+	switch(freq_mode) {
+	case 0x0 ... 0x5:
+		cpuclk_freq = 2000;
+		break;
+	case 0x6 ... 0xB:
+		cpuclk_freq = 1800;
+		break;
+	case 0xC ... 0x11:
+		cpuclk_freq = 1600;
+		break;
+	case 0x12 ... 0x16:
+		cpuclk_freq = 1400;
+		break;
+	case 0x17 ... 0x19:
+		cpuclk_freq = 1300;
+		break;
+	default:
+		pr_err("invalid SAR value\n");
+		return;
+	}
+
+	/* Convert to hertz */
+	cpuclk_freq *= 1000 * 1000;
+
+	/* CPU clocks depend on the Sample At Reset configuration */
+	of_property_read_string_index(np, "clock-output-names",
+				      0, &name);
+	ap806_clks[0] = clk_register_fixed_rate(NULL, name, NULL,
+						0, cpuclk_freq);
+
+	of_property_read_string_index(np, "clock-output-names",
+				      1, &name);
+	ap806_clks[1] = clk_register_fixed_rate(NULL, name, NULL, 0,
+						cpuclk_freq);
+
+	/* Fixed clock is always 1200 Mhz */
+	of_property_read_string_index(np, "clock-output-names",
+				      2, &fixedclk_name);
+	ap806_clks[2] = clk_register_fixed_rate(NULL, fixedclk_name, NULL, 0,
+						1200 * 1000 * 1000);
+
+	/* MSS Clock is fixed clock divided by 6 */
+	of_property_read_string_index(np, "clock-output-names",
+				      3, &name);
+	ap806_clks[3] = clk_register_fixed_factor(NULL, name, fixedclk_name,
+						  0, 1, 6);
+
+	of_clk_add_provider(np, of_clk_src_onecell_get, &ap806_clk_data);
+}
+
+CLK_OF_DECLARE(ap806_syscon_clk, "marvell,ap806-system-controller",
+	       ap806_syscon_clk_init);
-- 
2.6.4

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

Re: [PATCH v4 3/5] clk: mvebu: new driver for Armada AP806 system controller

[Stephen Boyd]

On 03/27, Thomas Petazzoni wrote:
> diff --git a/drivers/clk/mvebu/ap806-system-controller.c b/drivers/clk/mvebu/ap806-system-controller.c
> new file mode 100644
> index 0000000..c33c209
> --- /dev/null
> +++ b/drivers/clk/mvebu/ap806-system-controller.c
> @@ -0,0 +1,106 @@
> +
> +#define pr_fmt(fmt) "ap806-system-controller: " fmt
> +
> +#include <linux/kernel.h>
> +#include <linux/clk.h>

Is this used?

> +#include <linux/clk-provider.h>
> +#include <linux/io.h>

Is this used?

> +#include <linux/mfd/syscon.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/regmap.h>
> +
> +#define AP806_SAR_REG			0x400
> +#define AP806_SAR_CLKFREQ_MODE_MASK	0x1f
> +
> +#define AP806_CLK_NUM 			4
> +
> +static struct clk *ap806_clks[AP806_CLK_NUM];
> +
> +static struct clk_onecell_data ap806_clk_data = {
> +	.clks = ap806_clks,
> +	.clk_num = AP806_CLK_NUM,
> +};
> +
> +static void __init ap806_syscon_clk_init(struct device_node *np)

Can this be a platform driver instead?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/5] clk: mvebu: new driver for Armada AP806 system controller

[Thomas Petazzoni]

Hello,

On Fri, 1 Apr 2016 18:27:31 -0700, Stephen Boyd wrote:

> > +#include <linux/kernel.h>
> > +#include <linux/clk.h>
> 
> Is this used?

No, fixed!

> > +#include <linux/clk-provider.h>
> > +#include <linux/io.h>
> 
> Is this used?

Ditto!

> > +static void __init ap806_syscon_clk_init(struct device_node *np)
> 
> Can this be a platform driver instead?

I've moved to a platform driver, and it seems to work fine. Will be in
v5. Thanks for the review!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 4/5] dt-bindings: arm: add DT binding for Marvell CP110 system controller

[Thomas Petazzoni]

This commit adds the DT binding documentation for the Marvell CP110
system controller, which is part of the CP110 HW block, itself used in
the Marvell Armada 7K and 8K SoCs.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 .../arm/marvell/cp110-system-controller0.txt       | 88 ++++++++++++++++++++++
 1 file changed, 88 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/marvell/cp110-system-controller0.txt

diff --git a/Documentation/devicetree/bindings/arm/marvell/cp110-system-controller0.txt b/Documentation/devicetree/bindings/arm/marvell/cp110-system-controller0.txt
new file mode 100644
index 0000000..e8883da
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/marvell/cp110-system-controller0.txt
@@ -0,0 +1,88 @@
+Marvell Armada CP110 System Controller 0
+========================================
+
+The CP110 is one of the two core HW blocks of the Marvell Armada 7K/8K
+SoCs. It contains two sets of system control registers, System
+Controller 0 and System Controller 1. This Device Tree binding allows
+to describe the first system controller, which provides registers to
+configure various aspects of the SoC.
+
+The Device Tree node representing this System Controller 0 provides a
+number of clocks:
+
+ - a set of core clocks
+ - a set of gatable clocks
+
+Those clocks can be referenced by other Device Tree nodes using two
+cells:
+ - The first cell must be 0 or 1. 0 for the core clocks and 1 for the
+   gatable clocks.
+ - The second cell identifies the particular core clock or gatable
+   clocks.
+
+The following clocks are available:
+ - Core clocks
+   - 0 0	APLL
+   - 0 1	PPv2 core
+   - 0 2	EIP
+   - 0 3	Core
+   - 0 4	NAND core
+ - Gatable clocks
+   - 1 0	Audio
+   - 1 1	Comm Unit
+   - 1 2	NAND
+   - 1 3	PPv2
+   - 1 4	SDIO
+   - 1 5	MG Domain
+   - 1 6	MG Core
+   - 1 7	XOR1
+   - 1 8	XOR0
+   - 1 9	GOP DP
+   - 1 11	PCIe x1 0
+   - 1 12	PCIe x1 1
+   - 1 13	PCIe x4
+   - 1 14	PCIe / XOR
+   - 1 15	SATA
+   - 1 16	SATA USB
+   - 1 18	GOP Macro
+   - 1 22	USB3H0
+   - 1 23	USB3H1
+   - 1 24	USB3 Device
+   - 1 25	EIP150
+   - 1 26	EIP197
+
+Required properties:
+
+ - compatible: must be:
+     "marvell,cp110-system-controller0", "syscon";
+ - reg: register area of the CP110 system controller 0
+ - #clock-cells: must be set to 2
+ - core-clock-output-names must be set to:
+    "cpm-apll", "cpm-ppv2-core", "cpm-eip", "cpm-core", "cpm-nand-core"
+ - gatable-clock-indices must be set to:
+	<0>, <1>, <2>, <3>, <4>, <5>, <6>, <7>, <8>,
+	<9>, <11>, <12>, <13>, <14>, <15>, <16>, <18>,
+	<22>, <23>, <24>, <25>, <26>
+ - gatable-clock-output-names must be set to:
+	"cpm-audio", "cpm-communit", "cpm-nand", "cpm-ppv2", "cpm-sdio",
+	"cpm-mg-domain", "cpm-mg-core", "cpm-xor1", "cpm-xor0", "cpm-gop-dp", "cpm-pcie_x10",
+	"cpm-pcie_x11", "cpm-pcie_x4", "cpm-pcie-xor", "cpm-sata", "cpm-sata-usb",
+	"cpm-gop-macro", "cpm-usb3h0", "cpm-usb3h1", "cpm-usb3dev", "cpm-eip150",
+	"cpm-eip197"
+
+Example:
+
+	cpm_syscon0: system-controller@440000 {
+		compatible = "marvell,cp110-system-controller0", "syscon";
+		reg = <0x440000 0x1000>;
+		#clock-cells = <2>;
+		core-clock-output-names = "cpm-apll", "cpm-ppv2-core", "cpm-eip", "cpm-core", "cpm-nand-core";
+		gate-clock-indices = <0>, <1>, <2>, <3>, <4>, <5>, <6>, <7>, <8>,
+			<9>, <11>, <12>, <13>, <14>, <15>, <16>, <18>,
+			<22>, <23>, <24>, <25>, <26>;
+		gate-clock-output-names = "cpm-audio", "cpm-communit", "cpm-nand", "cpm-ppv2", "cpm-sdio",
+			"cpm-mg-domain", "cpm-mg-core", "cpm-xor1", "cpm-xor0", "cpm-gop-dp", "cpm-pcie_x10",
+			"cpm-pcie_x11", "cpm-pcie_x4", "cpm-pcie-xor", "cpm-sata", "cpm-sata-usb",
+			"cpm-gop-macro", "cpm-usb3h0", "cpm-usb3h1", "cpm-usb3dev", "cpm-eip150",
+			"cpm-eip197";
+	};
-- 
2.6.4

Re: [PATCH v4 4/5] dt-bindings: arm: add DT binding for Marvell CP110 system controller

[Rob Herring]

On Sun, Mar 27, 2016 at 11:26:16AM +0200, Thomas Petazzoni wrote:
> This commit adds the DT binding documentation for the Marvell CP110
> system controller, which is part of the CP110 HW block, itself used in
> the Marvell Armada 7K and 8K SoCs.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  .../arm/marvell/cp110-system-controller0.txt       | 88 ++++++++++++++++++++++
>  1 file changed, 88 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/marvell/cp110-system-controller0.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/marvell/cp110-system-controller0.txt b/Documentation/devicetree/bindings/arm/marvell/cp110-system-controller0.txt
> new file mode 100644
> index 0000000..e8883da
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/marvell/cp110-system-controller0.txt
> @@ -0,0 +1,88 @@
> +Marvell Armada CP110 System Controller 0
> +========================================
> +
> +The CP110 is one of the two core HW blocks of the Marvell Armada 7K/8K
> +SoCs. It contains two sets of system control registers, System
> +Controller 0 and System Controller 1. This Device Tree binding allows
> +to describe the first system controller, which provides registers to
> +configure various aspects of the SoC.
> +
> +The Device Tree node representing this System Controller 0 provides a
> +number of clocks:
> +
> + - a set of core clocks
> + - a set of gatable clocks
> +
> +Those clocks can be referenced by other Device Tree nodes using two
> +cells:
> + - The first cell must be 0 or 1. 0 for the core clocks and 1 for the
> +   gatable clocks.
> + - The second cell identifies the particular core clock or gatable
> +   clocks.
> +
> +The following clocks are available:
> + - Core clocks
> +   - 0 0	APLL
> +   - 0 1	PPv2 core
> +   - 0 2	EIP
> +   - 0 3	Core
> +   - 0 4	NAND core
> + - Gatable clocks
> +   - 1 0	Audio
> +   - 1 1	Comm Unit
> +   - 1 2	NAND
> +   - 1 3	PPv2
> +   - 1 4	SDIO
> +   - 1 5	MG Domain
> +   - 1 6	MG Core
> +   - 1 7	XOR1
> +   - 1 8	XOR0
> +   - 1 9	GOP DP
> +   - 1 11	PCIe x1 0
> +   - 1 12	PCIe x1 1
> +   - 1 13	PCIe x4
> +   - 1 14	PCIe / XOR
> +   - 1 15	SATA
> +   - 1 16	SATA USB
> +   - 1 18	GOP Macro
> +   - 1 22	USB3H0
> +   - 1 23	USB3H1
> +   - 1 24	USB3 Device
> +   - 1 25	EIP150
> +   - 1 26	EIP197
> +
> +Required properties:
> +
> + - compatible: must be:
> +     "marvell,cp110-system-controller0", "syscon";

This block is really the same across SoCs? 

> + - reg: register area of the CP110 system controller 0
> + - #clock-cells: must be set to 2
> + - core-clock-output-names must be set to:
> +    "cpm-apll", "cpm-ppv2-core", "cpm-eip", "cpm-core", "cpm-nand-core"
> + - gatable-clock-indices must be set to:
> +	<0>, <1>, <2>, <3>, <4>, <5>, <6>, <7>, <8>,
> +	<9>, <11>, <12>, <13>, <14>, <15>, <16>, <18>,
> +	<22>, <23>, <24>, <25>, <26>

You aren't skipping very many spots. I'd just fill the unused names in 
with "none" or something.

> + - gatable-clock-output-names must be set to:
> +	"cpm-audio", "cpm-communit", "cpm-nand", "cpm-ppv2", "cpm-sdio",
> +	"cpm-mg-domain", "cpm-mg-core", "cpm-xor1", "cpm-xor0", "cpm-gop-dp", "cpm-pcie_x10",
> +	"cpm-pcie_x11", "cpm-pcie_x4", "cpm-pcie-xor", "cpm-sata", "cpm-sata-usb",
> +	"cpm-gop-macro", "cpm-usb3h0", "cpm-usb3h1", "cpm-usb3dev", "cpm-eip150",
> +	"cpm-eip197"
> +
> +Example:
> +
> +	cpm_syscon0: system-controller@440000 {
> +		compatible = "marvell,cp110-system-controller0", "syscon";
> +		reg = <0x440000 0x1000>;
> +		#clock-cells = <2>;
> +		core-clock-output-names = "cpm-apll", "cpm-ppv2-core", "cpm-eip", "cpm-core", "cpm-nand-core";
> +		gate-clock-indices = <0>, <1>, <2>, <3>, <4>, <5>, <6>, <7>, <8>,
> +			<9>, <11>, <12>, <13>, <14>, <15>, <16>, <18>,
> +			<22>, <23>, <24>, <25>, <26>;
> +		gate-clock-output-names = "cpm-audio", "cpm-communit", "cpm-nand", "cpm-ppv2", "cpm-sdio",
> +			"cpm-mg-domain", "cpm-mg-core", "cpm-xor1", "cpm-xor0", "cpm-gop-dp", "cpm-pcie_x10",
> +			"cpm-pcie_x11", "cpm-pcie_x4", "cpm-pcie-xor", "cpm-sata", "cpm-sata-usb",
> +			"cpm-gop-macro", "cpm-usb3h0", "cpm-usb3h1", "cpm-usb3dev", "cpm-eip150",
> +			"cpm-eip197";
> +	};
> -- 
> 2.6.4
> 

Re: [PATCH v4 4/5] dt-bindings: arm: add DT binding for Marvell CP110 system controller

[Thomas Petazzoni]

Hello,

On Mon, 28 Mar 2016 14:47:56 -0500, Rob Herring wrote:

> > +Required properties:
> > +
> > + - compatible: must be:
> > +     "marvell,cp110-system-controller0", "syscon";
> 
> This block is really the same across SoCs? 

As per my knowledge, it is the same across 7020, 7040, 8020 and 8040,
where the CP part is named CP110. My understanding is that in future
SoCs, when the CP part will change, the CP part will have a different
name, i.e CP115 or 120 or something (these are invented names, I have
no idea how Marvell will name the future CPs).

So I believe cp110-system-controller0 properly uniquely identifies this
IP block.

> > + - reg: register area of the CP110 system controller 0
> > + - #clock-cells: must be set to 2
> > + - core-clock-output-names must be set to:
> > +    "cpm-apll", "cpm-ppv2-core", "cpm-eip", "cpm-core", "cpm-nand-core"
> > + - gatable-clock-indices must be set to:
> > +	<0>, <1>, <2>, <3>, <4>, <5>, <6>, <7>, <8>,
> > +	<9>, <11>, <12>, <13>, <14>, <15>, <16>, <18>,
> > +	<22>, <23>, <24>, <25>, <26>
> 
> You aren't skipping very many spots. I'd just fill the unused names in 
> with "none" or something.

and then remove the gatable-clock-indices property altogether?

Thanks for the review!

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

Re: [PATCH v4 4/5] dt-bindings: arm: add DT binding for Marvell CP110 system controller

[Thomas Petazzoni]

Hello,

On Mon, 28 Mar 2016 14:47:56 -0500, Rob Herring wrote:

> > + - reg: register area of the CP110 system controller 0
> > + - #clock-cells: must be set to 2
> > + - core-clock-output-names must be set to:
> > +    "cpm-apll", "cpm-ppv2-core", "cpm-eip", "cpm-core", "cpm-nand-core"
> > + - gatable-clock-indices must be set to:
> > +	<0>, <1>, <2>, <3>, <4>, <5>, <6>, <7>, <8>,
> > +	<9>, <11>, <12>, <13>, <14>, <15>, <16>, <18>,
> > +	<22>, <23>, <24>, <25>, <26>
> 
> You aren't skipping very many spots. I'd just fill the unused names in 
> with "none" or something.
> 
> > + - gatable-clock-output-names must be set to:
> > +	"cpm-audio", "cpm-communit", "cpm-nand", "cpm-ppv2", "cpm-sdio",
> > +	"cpm-mg-domain", "cpm-mg-core", "cpm-xor1", "cpm-xor0", "cpm-gop-dp", "cpm-pcie_x10",
> > +	"cpm-pcie_x11", "cpm-pcie_x4", "cpm-pcie-xor", "cpm-sata", "cpm-sata-usb",
> > +	"cpm-gop-macro", "cpm-usb3h0", "cpm-usb3h1", "cpm-usb3dev", "cpm-eip150",
> > +	"cpm-eip197"

In fact, the more I think of it, the less I find encoding the clock
output names in the DT to be useful for such a driver. For generic
clock drivers, it makes complete sense, but here the driver is really
tied to the specific system controller of that SoC, so the clock names
will not change.

In addition, those gatable clocks are not independent from each other:
many of them are parent from others (this wasn't encoded into the
driver until now, because I didn't had this info until now). So the
driver will anyway have to have a lot of knowledge about which clock is
child/parent of which one. Which means the driver is anyway really
tied to that specific system controller.

Does it still make sense to encode the clock names in the DT in such a
case, or should the driver be providing the clock names?

(I don't have a strong opinion either way, I'm just asking what is the
preference of the DT and clock maintainers on the matter.).

Thanks!

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

Re: [PATCH v4 4/5] dt-bindings: arm: add DT binding for Marvell CP110 system controller

[Thomas Petazzoni]

Hello,

On Wed, 13 Apr 2016 18:01:22 +0200, Thomas Petazzoni wrote:

> In fact, the more I think of it, the less I find encoding the clock
> output names in the DT to be useful for such a driver. For generic
> clock drivers, it makes complete sense, but here the driver is really
> tied to the specific system controller of that SoC, so the clock names
> will not change.

My bad: there will be two instances of the CP110 system controller, one
for the master CP110 and one for the slave CP110. So having the
possibility of setting the clock names in the DT is actually a good
thing, so I'll keep it.

Thanks!

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

Re: [PATCH v4 4/5] dt-bindings: arm: add DT binding for Marvell CP110 system controller

[Geert Uytterhoeven]

Bonsoir Thomas,

On Wed, Apr 13, 2016 at 6:01 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> On Mon, 28 Mar 2016 14:47:56 -0500, Rob Herring wrote:
>
>> > + - reg: register area of the CP110 system controller 0
>> > + - #clock-cells: must be set to 2
>> > + - core-clock-output-names must be set to:
>> > +    "cpm-apll", "cpm-ppv2-core", "cpm-eip", "cpm-core", "cpm-nand-core"
>> > + - gatable-clock-indices must be set to:
>> > +   <0>, <1>, <2>, <3>, <4>, <5>, <6>, <7>, <8>,
>> > +   <9>, <11>, <12>, <13>, <14>, <15>, <16>, <18>,
>> > +   <22>, <23>, <24>, <25>, <26>
>>
>> You aren't skipping very many spots. I'd just fill the unused names in
>> with "none" or something.
>>
>> > + - gatable-clock-output-names must be set to:
>> > +   "cpm-audio", "cpm-communit", "cpm-nand", "cpm-ppv2", "cpm-sdio",
>> > +   "cpm-mg-domain", "cpm-mg-core", "cpm-xor1", "cpm-xor0", "cpm-gop-dp", "cpm-pcie_x10",
>> > +   "cpm-pcie_x11", "cpm-pcie_x4", "cpm-pcie-xor", "cpm-sata", "cpm-sata-usb",
>> > +   "cpm-gop-macro", "cpm-usb3h0", "cpm-usb3h1", "cpm-usb3dev", "cpm-eip150",
>> > +   "cpm-eip197"
>
> In fact, the more I think of it, the less I find encoding the clock
> output names in the DT to be useful for such a driver. For generic
> clock drivers, it makes complete sense, but here the driver is really
> tied to the specific system controller of that SoC, so the clock names
> will not change.
>
> In addition, those gatable clocks are not independent from each other:
> many of them are parent from others (this wasn't encoded into the
> driver until now, because I didn't had this info until now). So the
> driver will anyway have to have a lot of knowledge about which clock is
> child/parent of which one. Which means the driver is anyway really
> tied to that specific system controller.
>
> Does it still make sense to encode the clock names in the DT in such a
> case, or should the driver be providing the clock names?
>
> (I don't have a strong opinion either way, I'm just asking what is the
> preference of the DT and clock maintainers on the matter.).

>From my experience with clock drivers for Renesas SoCs, life is much easier
when handling all of this (clock names, parents, ...) in C. The same is true
BTW for PM Domains.

The clock controllers of r8a7791 and r8a7795 are very similar, and could
be handled similarly.
Compare the DT hierarchy for CPG and MSTP/MSSR in
arch/arm/boot/dts/r8a7791.dtsi and arch/arm64/boot/dts/renesas/r8a7795.dtsi.

Binding definitions are
    include/dt-bindings/clock/r8a7791-clock.h
vs
    include/dt-bindings/clock/r8a7795-cpg-mssr.h

Corresponding drivers are
    drivers/clk/renesas/clk-rcar-gen2.c
    drivers/clk/renesas/clk-mstp.c
vs.
    drivers/clk/renesas/r8a7795-cpg-mssr.c
    drivers/clk/renesas/renesas-cpg-mssr.c
    drivers/clk/renesas/renesas-cpg-mssr.h

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v4 4/5] dt-bindings: arm: add DT binding for Marvell CP110 system controller

[Thomas Petazzoni]

Hello,

On Thu, 14 Apr 2016 21:19:54 +0200, Geert Uytterhoeven wrote:

> From my experience with clock drivers for Renesas SoCs, life is much easier
> when handling all of this (clock names, parents, ...) in C. The same is true
> BTW for PM Domains.
> 
> The clock controllers of r8a7791 and r8a7795 are very similar, and could
> be handled similarly.
> Compare the DT hierarchy for CPG and MSTP/MSSR in
> arch/arm/boot/dts/r8a7791.dtsi and arch/arm64/boot/dts/renesas/r8a7795.dtsi.

Thanks for your feedback. However in practice, there will ultimately be
two instances of this clock controller, as some variants of the SoC
will contain two CP parts. Due to this, it is going to be quite
convenient to have the clock names configurable from the DT, as I will
be able to give the same clock on the master CP and the slave CP a
different name.

That's actually why my binding documentation uses names like
"cpm-<foo>" for all the clocks, where "cpm" stands for "CP Master". In
the future, another instance will have clocks named "cps-<foo>" for "CP
Slave".

Best regards,

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

Re: [PATCH v4 4/5] dt-bindings: arm: add DT binding for Marvell CP110 system controller

[Geert Uytterhoeven]

Hi Thomas,

On Sat, Apr 23, 2016 at 4:22 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> On Thu, 14 Apr 2016 21:19:54 +0200, Geert Uytterhoeven wrote:
>> From my experience with clock drivers for Renesas SoCs, life is much easier
>> when handling all of this (clock names, parents, ...) in C. The same is true
>> BTW for PM Domains.
>>
>> The clock controllers of r8a7791 and r8a7795 are very similar, and could
>> be handled similarly.
>> Compare the DT hierarchy for CPG and MSTP/MSSR in
>> arch/arm/boot/dts/r8a7791.dtsi and arch/arm64/boot/dts/renesas/r8a7795.dtsi.
>
> Thanks for your feedback. However in practice, there will ultimately be
> two instances of this clock controller, as some variants of the SoC
> will contain two CP parts. Due to this, it is going to be quite
> convenient to have the clock names configurable from the DT, as I will
> be able to give the same clock on the master CP and the slave CP a
> different name.

You could still prefix the names by e.g. an instance number.

> That's actually why my binding documentation uses names like
> "cpm-<foo>" for all the clocks, where "cpm" stands for "CP Master". In
> the future, another instance will have clocks named "cps-<foo>" for "CP
> Slave".

So the names are actually part of the binding, while nothing really relies
on the actual names, right? Clocks are referred to by clock specifier (phandle
+ some optonal bits).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v4 5/5] clk: mvebu: new driver for Armada CP110 system controller

[Thomas Petazzoni]

The Armada CP110 system controller provides, amongst other things, a
number of clocks for the platform: a small number of core clocks, and
then a number of gatable clocks, derived from some of the core
clocks. Those clocks are configured via registers of the CP110 System
Controller.

The CP110 is the other core HW block (next to the AP806) used in the
Marvel Armada 7K and 8K SoCs.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/clk/mvebu/Kconfig                   |   3 +
 drivers/clk/mvebu/Makefile                  |   1 +
 drivers/clk/mvebu/cp110-system-controller.c | 265 ++++++++++++++++++++++++++++
 3 files changed, 269 insertions(+)
 create mode 100644 drivers/clk/mvebu/cp110-system-controller.c

diff --git a/drivers/clk/mvebu/Kconfig b/drivers/clk/mvebu/Kconfig
index bf7ae00..3165da7 100644
--- a/drivers/clk/mvebu/Kconfig
+++ b/drivers/clk/mvebu/Kconfig
@@ -32,6 +32,9 @@ config ARMADA_XP_CLK
 config ARMADA_AP806_SYSCON
 	bool
 
+config ARMADA_CP110_SYSCON
+	bool
+
 config DOVE_CLK
 	bool
 	select MVEBU_CLK_COMMON
diff --git a/drivers/clk/mvebu/Makefile b/drivers/clk/mvebu/Makefile
index f4aa481..7172ef6 100644
--- a/drivers/clk/mvebu/Makefile
+++ b/drivers/clk/mvebu/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_ARMADA_38X_CLK)	+= armada-38x.o
 obj-$(CONFIG_ARMADA_39X_CLK)	+= armada-39x.o
 obj-$(CONFIG_ARMADA_XP_CLK)	+= armada-xp.o
 obj-$(CONFIG_ARMADA_AP806_SYSCON) += ap806-system-controller.o
+obj-$(CONFIG_ARMADA_CP110_SYSCON) += cp110-system-controller.o
 obj-$(CONFIG_DOVE_CLK)		+= dove.o dove-divider.o
 obj-$(CONFIG_KIRKWOOD_CLK)	+= kirkwood.o
 obj-$(CONFIG_ORION_CLK)		+= orion.o
diff --git a/drivers/clk/mvebu/cp110-system-controller.c b/drivers/clk/mvebu/cp110-system-controller.c
new file mode 100644
index 0000000..7b22503
--- /dev/null
+++ b/drivers/clk/mvebu/cp110-system-controller.c
@@ -0,0 +1,265 @@
+/*
+ * Marvell Armada CP110 System Controller
+ *
+ * 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.
+ */
+
+/*
+ * CP110 has 5 core clocks:
+ *
+ *  - APLL		(1 Ghz)
+ *    - PPv2 core	(1/3 APLL)
+ *    - EIP		(1/2 APLL)
+ *      - Core		(1/2 EIP)
+ *
+ *  - NAND clock, which is either:
+ *    - Equal to the core clock
+ *    - 2/5 APLL
+ *
+ * CP110 has 32 gatable clocks, for the various peripherals in the IP:
+ *
+ *  - Most have the "Core" clocks as their parents
+ *  - The NAND gatable clock has NAND-core as its parent
+ *  - The PPv2 gatable clock has PPv2-core as its parent
+ *  - The EIP* gatable clocks have EIP as their parent
+ */
+
+#define pr_fmt(fmt) "cp110-system-controller: " fmt
+
+#include <linux/kernel.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/io.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#define CP110_PM_CLOCK_GATING_REG	0x220
+#define CP110_NAND_FLASH_CLK_CTRL_REG	0x700
+#define    NF_CLOCK_SEL_400_MASK	BIT(0)
+
+enum {
+	CP110_CLK_TYPE_CORE,
+	CP110_CLK_TYPE_GATABLE,
+};
+
+#define CP110_MAX_CORE_CLOCKS		5
+#define CP110_MAX_GATABLE_CLOCKS	32
+
+#define CP110_CLK_NUM \
+	(CP110_MAX_CORE_CLOCKS + CP110_MAX_GATABLE_CLOCKS)
+
+/* A few gatable clocks need special handling */
+#define CP110_GATE_NAND			2
+#define CP110_GATE_PPV2			3
+#define CP110_GATE_EIP150		25
+#define CP110_GATE_EIP197		26
+
+static struct clk *cp110_clks[CP110_CLK_NUM];
+
+static struct clk_onecell_data cp110_clk_data = {
+	.clks = cp110_clks,
+	.clk_num = CP110_CLK_NUM,
+};
+
+struct cp110_gate_clk {
+	struct clk_hw hw;
+	struct regmap *regmap;
+	u8 bit_idx;
+};
+
+static int cp110_gate_enable(struct clk_hw *hw)
+{
+	struct cp110_gate_clk *gate =
+		container_of(hw, struct cp110_gate_clk, hw);
+
+	regmap_update_bits(gate->regmap, CP110_PM_CLOCK_GATING_REG,
+			   BIT(gate->bit_idx), BIT(gate->bit_idx));
+
+	return 0;
+}
+
+static void cp110_gate_disable(struct clk_hw *hw)
+{
+	struct cp110_gate_clk *gate =
+		container_of(hw, struct cp110_gate_clk, hw);
+	regmap_update_bits(gate->regmap, CP110_PM_CLOCK_GATING_REG,
+			   BIT(gate->bit_idx), 0);
+}
+
+static int cp110_gate_is_enabled(struct clk_hw *hw)
+{
+	struct cp110_gate_clk *gate =
+		container_of(hw, struct cp110_gate_clk, hw);
+	u32 val;
+
+	regmap_read(gate->regmap, CP110_PM_CLOCK_GATING_REG, &val);
+
+	return val & BIT(gate->bit_idx);
+}
+
+static const struct clk_ops cp110_gate_ops = {
+	.enable = cp110_gate_enable,
+	.disable = cp110_gate_disable,
+	.is_enabled = cp110_gate_is_enabled,
+};
+
+static struct clk *cp110_register_gate(const char *name, const char *parent_name,
+				       struct regmap *regmap, u8 bit_idx)
+{
+	struct cp110_gate_clk *gate;
+	struct clk *clk;
+	struct clk_init_data init;
+
+	gate = kzalloc(sizeof(*gate), GFP_KERNEL);
+	if (!gate)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.ops = &cp110_gate_ops;
+	init.flags = CLK_IS_BASIC;
+	init.parent_names = &parent_name;
+	init.num_parents = 1;
+
+	gate->regmap = regmap;
+	gate->bit_idx = bit_idx;
+	gate->hw.init = &init;
+
+	clk = clk_register(NULL, &gate->hw);
+	if (IS_ERR(clk))
+		kfree(gate);
+
+	return clk;
+}
+
+static struct clk *cp110_of_clk_get(struct of_phandle_args *clkspec, void *data)
+{
+	struct clk_onecell_data *clk_data = data;
+	unsigned int type = clkspec->args[0];
+	unsigned int idx = clkspec->args[1];
+
+	if (type == CP110_CLK_TYPE_CORE) {
+		if (idx > CP110_MAX_CORE_CLOCKS)
+			return ERR_PTR(-EINVAL);
+		return clk_data->clks[idx];
+	} else if (type == CP110_CLK_TYPE_GATABLE) {
+		if (idx > CP110_MAX_GATABLE_CLOCKS)
+			return ERR_PTR(-EINVAL);
+		return clk_data->clks[CP110_MAX_CORE_CLOCKS + idx];
+	}
+
+	return ERR_PTR(-EINVAL);
+}
+
+static void __init cp110_syscon_clk_init(struct device_node *np)
+{
+	struct regmap *regmap;
+	const char *name, *apll_name, *core_name, *eip_name, *nand_name;
+	u32 nand_clk_ctrl;
+	int clkidx = 0, i;
+
+	regmap = syscon_node_to_regmap(np);
+	if (IS_ERR(regmap)) {
+		pr_err("cannot get regmap\n");
+		return;
+	}
+
+	if (regmap_read(regmap, CP110_NAND_FLASH_CLK_CTRL_REG, &nand_clk_ctrl)) {
+		pr_err("cannot read from regmap\n");
+		return;
+	}
+
+	/*
+	 * Register core clocks
+	 */
+
+        /* Register the APLL which is the root of the clk tree */
+	of_property_read_string_index(np, "core-clock-output-names",
+                                      0, &apll_name);
+        cp110_clks[0] =
+		clk_register_fixed_rate(NULL, apll_name, NULL, 0,
+					1000 * 1000 * 1000);
+
+        /* PPv2 is APLL/3 */
+	of_property_read_string_index(np, "core-clock-output-names",
+				      1, &name);
+        cp110_clks[1] =
+		clk_register_fixed_factor(NULL, name, apll_name,
+					  0, 1, 3);
+	clkidx++;
+
+        /* EIP clock is APLL/2 */
+	of_property_read_string_index(np, "core-clock-output-names",
+				      2, &eip_name);
+        cp110_clks[2] =
+		clk_register_fixed_factor(NULL, eip_name, apll_name, 0, 1, 2);
+	clkidx++;
+
+        /* Core clock is EIP/2 */
+	of_property_read_string_index(np, "core-clock-output-names",
+				      3, &core_name);
+        cp110_clks[3] =
+		clk_register_fixed_factor(NULL, core_name, eip_name, 0, 1, 2);
+	clkidx++;
+
+        /* NAND can be either APLL/2.5 or core clock */
+	of_property_read_string_index(np, "core-clock-output-names",
+				      4, &nand_name);
+        if (nand_clk_ctrl & NF_CLOCK_SEL_400_MASK)
+                cp110_clks[4] =
+			clk_register_fixed_factor(NULL, nand_name,
+						  apll_name, 0, 2, 5);
+        else
+                cp110_clks[4] =
+			clk_register_fixed_factor(NULL, nand_name,
+						  core_name, 0, 1, 1);
+
+	/*
+	 * Register gatable clocks
+	 */
+	for (i = 0; i < CP110_MAX_GATABLE_CLOCKS; i++) {
+		const char *parent;
+		int clkidx, ret;
+
+		ret = of_property_read_string_index(np, "gate-clock-output-names",
+						    i, &name);
+		/* Reached the end of the list? */
+		if (ret < 0)
+			break;
+
+		of_property_read_u32_index(np, "gate-clock-indices", i, &clkidx);
+
+		switch(clkidx) {
+		case CP110_GATE_NAND:
+			parent = nand_name;
+			break;
+		case CP110_GATE_PPV2:
+			parent = "ppv2-core";
+			break;
+		case CP110_GATE_EIP150:
+		case CP110_GATE_EIP197:
+			parent = "eip";
+			break;
+		default:
+			parent = "core";
+			break;
+		}
+
+		cp110_clks[CP110_MAX_CORE_CLOCKS + clkidx] =
+			cp110_register_gate(name, parent,
+					    regmap, clkidx);
+	}
+
+	of_clk_add_provider(np, cp110_of_clk_get, &cp110_clk_data);
+}
+
+CLK_OF_DECLARE(cp110_syscon_clk, "marvell,cp110-system-controller0",
+	       cp110_syscon_clk_init);
-- 
2.6.4

Re: [PATCH v4 5/5] clk: mvebu: new driver for Armada CP110 system controller

[Stephen Boyd]

On 03/27, Thomas Petazzoni wrote:
> diff --git a/drivers/clk/mvebu/cp110-system-controller.c b/drivers/clk/mvebu/cp110-system-controller.c
> new file mode 100644
> index 0000000..7b22503
> --- /dev/null
> +++ b/drivers/clk/mvebu/cp110-system-controller.c
> @@ -0,0 +1,265 @@
> +/*
> +#include <linux/kernel.h>
> +#include <linux/clk.h>

What's this for?

> +#include <linux/clk-provider.h>
> +#include <linux/io.h>

What's this for?

> +#include <linux/mfd/syscon.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
[..]
> +static int cp110_gate_enable(struct clk_hw *hw)
> +{
> +	struct cp110_gate_clk *gate =
> +		container_of(hw, struct cp110_gate_clk, hw);

Consider a macro to make this fit on one line.

> +
> +	regmap_update_bits(gate->regmap, CP110_PM_CLOCK_GATING_REG,
> +			   BIT(gate->bit_idx), BIT(gate->bit_idx));
> +
[..]
> +static struct clk *cp110_register_gate(const char *name, const char *parent_name,
> +				       struct regmap *regmap, u8 bit_idx)
> +{
> +	struct cp110_gate_clk *gate;
> +	struct clk *clk;
> +	struct clk_init_data init;
> +
> +	gate = kzalloc(sizeof(*gate), GFP_KERNEL);
> +	if (!gate)
> +		return ERR_PTR(-ENOMEM);
> +
> +	init.name = name;
> +	init.ops = &cp110_gate_ops;
> +	init.flags = CLK_IS_BASIC;

No basic please.

> +	init.parent_names = &parent_name;
> +	init.num_parents = 1;
> +
> +	gate->regmap = regmap;
> +	gate->bit_idx = bit_idx;
> +	gate->hw.init = &init;
> +
> +	clk = clk_register(NULL, &gate->hw);
> +	if (IS_ERR(clk))
> +		kfree(gate);
> +
> +	return clk;
> +}
> +
> +static struct clk *cp110_of_clk_get(struct of_phandle_args *clkspec, void *data)
> +{
> +	struct clk_onecell_data *clk_data = data;
> +	unsigned int type = clkspec->args[0];
> +	unsigned int idx = clkspec->args[1];
> +
> +	if (type == CP110_CLK_TYPE_CORE) {
> +		if (idx > CP110_MAX_CORE_CLOCKS)
> +			return ERR_PTR(-EINVAL);
> +		return clk_data->clks[idx];
> +	} else if (type == CP110_CLK_TYPE_GATABLE) {
> +		if (idx > CP110_MAX_GATABLE_CLOCKS)
> +			return ERR_PTR(-EINVAL);
> +		return clk_data->clks[CP110_MAX_CORE_CLOCKS + idx];
> +	}
> +
> +	return ERR_PTR(-EINVAL);
> +}
> +
> +static void __init cp110_syscon_clk_init(struct device_node *np)

Any reason this can't be a platform driver?

> +{
> +	struct regmap *regmap;
> +	const char *name, *apll_name, *core_name, *eip_name, *nand_name;
> +	u32 nand_clk_ctrl;
> +	int clkidx = 0, i;
> +
> +	regmap = syscon_node_to_regmap(np);

Isn't this the only driver for the node? Why not just make the
regmap ourselves in the driver here?

> +	if (IS_ERR(regmap)) {
> +		pr_err("cannot get regmap\n");
> +		return;
> +	}
> +
> +	if (regmap_read(regmap, CP110_NAND_FLASH_CLK_CTRL_REG, &nand_clk_ctrl)) {
> +		pr_err("cannot read from regmap\n");
> +		return;
> +	}
> +
> +	/*
> +	 * Register core clocks
> +	 */

Ok. Not really useful comment.

> +
> +        /* Register the APLL which is the root of the clk tree */
> +	of_property_read_string_index(np, "core-clock-output-names",
> +                                      0, &apll_name);
> +        cp110_clks[0] =
> +		clk_register_fixed_rate(NULL, apll_name, NULL, 0,
> +					1000 * 1000 * 1000);
> +
> +        /* PPv2 is APLL/3 */
> +	of_property_read_string_index(np, "core-clock-output-names",
> +				      1, &name);
> +        cp110_clks[1] =
> +		clk_register_fixed_factor(NULL, name, apll_name,
> +					  0, 1, 3);
> +	clkidx++;
> +
> +        /* EIP clock is APLL/2 */
> +	of_property_read_string_index(np, "core-clock-output-names",
> +				      2, &eip_name);
> +        cp110_clks[2] =
> +		clk_register_fixed_factor(NULL, eip_name, apll_name, 0, 1, 2);
> +	clkidx++;
> +
> +        /* Core clock is EIP/2 */
> +	of_property_read_string_index(np, "core-clock-output-names",
> +				      3, &core_name);
> +        cp110_clks[3] =
> +		clk_register_fixed_factor(NULL, core_name, eip_name, 0, 1, 2);
> +	clkidx++;
> +
> +        /* NAND can be either APLL/2.5 or core clock */
> +	of_property_read_string_index(np, "core-clock-output-names",
> +				      4, &nand_name);
> +        if (nand_clk_ctrl & NF_CLOCK_SEL_400_MASK)
> +                cp110_clks[4] =

Weird indentation here. Please use tabs throughout.

> +			clk_register_fixed_factor(NULL, nand_name,
> +						  apll_name, 0, 2, 5);
> +        else
> +                cp110_clks[4] =
> +			clk_register_fixed_factor(NULL, nand_name,
> +						  core_name, 0, 1, 1);
> +
> +	/*
> +	 * Register gatable clocks
> +	 */
> +	for (i = 0; i < CP110_MAX_GATABLE_CLOCKS; i++) {
> +		const char *parent;
> +		int clkidx, ret;
> +
> +		ret = of_property_read_string_index(np, "gate-clock-output-names",
> +						    i, &name);
> +		/* Reached the end of the list? */
> +		if (ret < 0)
> +			break;
> +
> +		of_property_read_u32_index(np, "gate-clock-indices", i, &clkidx);
> +
> +		switch(clkidx) {
> +		case CP110_GATE_NAND:
> +			parent = nand_name;
> +			break;
> +		case CP110_GATE_PPV2:
> +			parent = "ppv2-core";
> +			break;
> +		case CP110_GATE_EIP150:
> +		case CP110_GATE_EIP197:
> +			parent = "eip";
> +			break;
> +		default:
> +			parent = "core";
> +			break;
> +		}
> +
> +		cp110_clks[CP110_MAX_CORE_CLOCKS + clkidx] =
> +			cp110_register_gate(name, parent,
> +					    regmap, clkidx);
> +	}
> +
> +	of_clk_add_provider(np, cp110_of_clk_get, &cp110_clk_data);

What if this fails?


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 5/5] clk: mvebu: new driver for Armada CP110 system controller

[Thomas Petazzoni]

Hello,

On Fri, 1 Apr 2016 18:25:41 -0700, Stephen Boyd wrote:

> > +#include <linux/kernel.h>
> > +#include <linux/clk.h>
> 
> What's this for?
> 
> > +#include <linux/clk-provider.h>
> > +#include <linux/io.h>
> 
> What's this for?

Both removed.

> > +static int cp110_gate_enable(struct clk_hw *hw)
> > +{
> > +	struct cp110_gate_clk *gate =
> > +		container_of(hw, struct cp110_gate_clk, hw);
> 
> Consider a macro to make this fit on one line.

Done!

> > +	init.name = name;
> > +	init.ops = &cp110_gate_ops;
> > +	init.flags = CLK_IS_BASIC;
> 
> No basic please.

Fixed!


> > +static void __init cp110_syscon_clk_init(struct device_node *np)
> 
> Any reason this can't be a platform driver?

Changed to be a platform driver.

> > +{
> > +	struct regmap *regmap;
> > +	const char *name, *apll_name, *core_name, *eip_name, *nand_name;
> > +	u32 nand_clk_ctrl;
> > +	int clkidx = 0, i;
> > +
> > +	regmap = syscon_node_to_regmap(np);
> 
> Isn't this the only driver for the node? Why not just make the
> regmap ourselves in the driver here?

It is for now. However, I expect that we will probably have sub-nodes
for various other devices whose registers belong to this system
controller. More specifically, there are some pin-muxing registers in
there. Using the syscon facility entirely automates the creation of the
regmap, and makes it easily accessible to other devices who might want
to poke into some of the system controller registers.

> > +	/*
> > +	 * Register core clocks
> > +	 */
> 
> Ok. Not really useful comment.

Removed!


> > +        /* NAND can be either APLL/2.5 or core clock */
> > +	of_property_read_string_index(np, "core-clock-output-names",
> > +				      4, &nand_name);
> > +        if (nand_clk_ctrl & NF_CLOCK_SEL_400_MASK)
> > +                cp110_clks[4] =
> 
> Weird indentation here. Please use tabs throughout.

Yeah, seems like I copy/pasted or something. Fixed to use tabs.

> > +	of_clk_add_provider(np, cp110_of_clk_get, &cp110_clk_data);
> 
> What if this fails?

Error checking added everywhere. There are numerous other clk drivers
that don't do error checking, so I did the same, especially since a
failure to register a clock is most likely going to be fatal to the
system booting. No clock -> no UART, no UART -> the system doesn't boot
up to userspace.

But fair enough, I've added error checking in the ->probe() function
(of both the AP806 and CP110 clk drivers).

Thanks again for the review!

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