[PATCH 0/6] Add CPU clock support for Armada 7K/8K

[PATCH 0/6] Add CPU clock support for Armada 7K/8K

[Gregory CLEMENT]

Hello,

This series allows to mange the cpu clock for Armada 7K/8K. For these
SoCs, the CPUs share the same clock by cluster, so actually the clock
management is done at cluster level.

As for the other Armada 7K/8K clocks it is possible to have multiple
AP so here again we need to have unique name: the purpose of the second
patch is to share a common code which will be used in 3 drivers.

The last 2 patch enable the driver at dt and platform level and will
be applied through the mvebu subsystem.

Gregory

Gregory CLEMENT (6): dt-bindings: ap806: add the cluster clock node in
  the syscon file clk: mvebu: add helper file for Armada AP and CP
  clocks clk: mvebu: add CPU clock driver for Armada 7K/8K clk: mvebu:
  ap806: Fix clock name for the cluster arm64: marvell: enable the
  Armada 7K/8K CPU clk driver arm64: dts: marvell: Add cpu clock node
  on Armada 7K/8K

 .../arm/marvell/ap806-system-controller.txt   |  22 ++
 arch/arm64/Kconfig.platforms                  |   1 +
 .../boot/dts/marvell/armada-ap806-quad.dtsi   |   4 +
 arch/arm64/boot/dts/marvell/armada-ap806.dtsi |   6 +
 drivers/clk/mvebu/Kconfig                     |   8 +
 drivers/clk/mvebu/Makefile                    |   2 +
 drivers/clk/mvebu/ap-cpu-clk.c                | 265 ++++++++++++++++++
 drivers/clk/mvebu/ap806-system-controller.c   |  24 +-
 drivers/clk/mvebu/armada_ap_cp_helper.c       |  28 ++
 drivers/clk/mvebu/armada_ap_cp_helper.h       |  11 +
 drivers/clk/mvebu/cp110-system-controller.c   |  32 +--
 11 files changed, 361 insertions(+), 42 deletions(-)
 create mode 100644 drivers/clk/mvebu/ap-cpu-clk.c
 create mode 100644 drivers/clk/mvebu/armada_ap_cp_helper.c
 create mode 100644 drivers/clk/mvebu/armada_ap_cp_helper.h

-- 
2.19.0

[PATCH 1/6] dt-bindings: ap806: add the cluster clock node in the syscon file

[Gregory CLEMENT]

Document the device tree binding for the cluster clock controllers found
in the Armada 7K/8K SoCs.

Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
 .../arm/marvell/ap806-system-controller.txt   | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/marvell/ap806-system-controller.txt b/Documentation/devicetree/bindings/arm/marvell/ap806-system-controller.txt
index 3fd21bb7cb37..8f281816a6b8 100644
--- a/Documentation/devicetree/bindings/arm/marvell/ap806-system-controller.txt
+++ b/Documentation/devicetree/bindings/arm/marvell/ap806-system-controller.txt
@@ -136,3 +136,25 @@ ap_syscon1: system-controller@6f8000 {
 		#thermal-sensor-cells = <1>;
 	};
 };
+
+Cluster clocks:
+---------------
+
+Device Tree Clock bindings for cluster clock of AP806 Marvell. Each
+cluster contain up to 2 CPUs running at the same frequency.
+
+Required properties:
+- compatible: must be  "marvell,ap806-cpu-clock";
+- #clock-cells : should be set to 1.
+- clocks : shall be the input parents clock phandle for the clock.
+
+ap_syscon1: system-controller@6f8000 {
+	compatible = "syscon", "simple-mfd";
+	reg = <0x6f8000 0x1000>;
+
+	cpu_clk: clock-cpu {
+		compatible = "marvell,ap806-cpu-clock";
+		clocks = <&ap_clk 0>, <&ap_clk 1>;
+		#clock-cells = <1>;
+	};
+};
-- 
2.19.0

Re: [PATCH 1/6] dt-bindings: ap806: add the cluster clock node in the syscon file

[Stephen Boyd]

Quoting Gregory CLEMENT (2018-09-22 11:17:04)
> Document the device tree binding for the cluster clock controllers found
> in the Armada 7K/8K SoCs.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> ---

Please Cc relevant DT maintainers and lists when submitting binding patches.

>  .../arm/marvell/ap806-system-controller.txt   | 22 +++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/marvell/ap806-system-controller.txt b/Documentation/devicetree/bindings/arm/marvell/ap806-system-controller.txt
> index 3fd21bb7cb37..8f281816a6b8 100644
> --- a/Documentation/devicetree/bindings/arm/marvell/ap806-system-controller.txt
> +++ b/Documentation/devicetree/bindings/arm/marvell/ap806-system-controller.txt
> @@ -136,3 +136,25 @@ ap_syscon1: system-controller@6f8000 {
>                 #thermal-sensor-cells = <1>;
>         };
>  };
> +
> +Cluster clocks:
> +---------------
> +
> +Device Tree Clock bindings for cluster clock of AP806 Marvell. Each
> +cluster contain up to 2 CPUs running at the same frequency.
> +
> +Required properties:
> +- compatible: must be  "marvell,ap806-cpu-clock";
> +- #clock-cells : should be set to 1.
> +- clocks : shall be the input parents clock phandle for the clock.
> +
> +ap_syscon1: system-controller@6f8000 {
> +       compatible = "syscon", "simple-mfd";
> +       reg = <0x6f8000 0x1000>;
> +
> +       cpu_clk: clock-cpu {
> +               compatible = "marvell,ap806-cpu-clock";
> +               clocks = <&ap_clk 0>, <&ap_clk 1>;
> +               #clock-cells = <1>;
> +       };
> +};

[PATCH 2/6] clk: mvebu: add helper file for Armada AP and CP clocks

[Gregory CLEMENT]

Clock drivers for Armada AP and Armada CP use the same function to
generate unique clock name. A third drivers is coming with the same
need, so it's time to move this function in a common file.

Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
 drivers/clk/mvebu/Kconfig                   |  5 ++++
 drivers/clk/mvebu/Makefile                  |  1 +
 drivers/clk/mvebu/ap806-system-controller.c | 24 ++++------------
 drivers/clk/mvebu/armada_ap_cp_helper.c     | 28 ++++++++++++++++++
 drivers/clk/mvebu/armada_ap_cp_helper.h     | 11 +++++++
 drivers/clk/mvebu/cp110-system-controller.c | 32 ++++++---------------
 6 files changed, 59 insertions(+), 42 deletions(-)
 create mode 100644 drivers/clk/mvebu/armada_ap_cp_helper.c
 create mode 100644 drivers/clk/mvebu/armada_ap_cp_helper.h

diff --git a/drivers/clk/mvebu/Kconfig b/drivers/clk/mvebu/Kconfig
index fddc8ac5faff..5492fae3f0ab 100644
--- a/drivers/clk/mvebu/Kconfig
+++ b/drivers/clk/mvebu/Kconfig
@@ -7,6 +7,9 @@ config MVEBU_CLK_CPU
 config MVEBU_CLK_COREDIV
 	bool
 
+config ARMADA_AP_CP_HELPER
+	bool
+
 config ARMADA_370_CLK
 	bool
 	select MVEBU_CLK_COMMON
@@ -34,9 +37,11 @@ config ARMADA_XP_CLK
 
 config ARMADA_AP806_SYSCON
 	bool
+	select ARMADA_AP_CP_HELPER
 
 config ARMADA_CP110_SYSCON
 	bool
+	select ARMADA_AP_CP_HELPER
 
 config DOVE_CLK
 	bool
diff --git a/drivers/clk/mvebu/Makefile b/drivers/clk/mvebu/Makefile
index 93ac3685271f..6638ad962ec1 100644
--- a/drivers/clk/mvebu/Makefile
+++ b/drivers/clk/mvebu/Makefile
@@ -2,6 +2,7 @@
 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_AP_CP_HELPER) += armada_ap_cp_helper.o
 
 obj-$(CONFIG_ARMADA_370_CLK)	+= armada-370.o
 obj-$(CONFIG_ARMADA_375_CLK)	+= armada-375.o
diff --git a/drivers/clk/mvebu/ap806-system-controller.c b/drivers/clk/mvebu/ap806-system-controller.c
index ea54a874bbda..0a58824ff053 100644
--- a/drivers/clk/mvebu/ap806-system-controller.c
+++ b/drivers/clk/mvebu/ap806-system-controller.c
@@ -10,11 +10,11 @@
 
 #define pr_fmt(fmt) "ap806-system-controller: " fmt
 
+#include "armada_ap_cp_helper.h"
 #include <linux/clk-provider.h>
 #include <linux/mfd/syscon.h>
 #include <linux/init.h>
 #include <linux/of.h>
-#include <linux/of_address.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 
@@ -30,18 +30,6 @@ static struct clk_onecell_data ap806_clk_data = {
 	.clk_num = AP806_CLK_NUM,
 };
 
-static char *ap806_unique_name(struct device *dev, struct device_node *np,
-			       char *name)
-{
-	const __be32 *reg;
-	u64 addr;
-
-	reg = of_get_property(np, "reg", NULL);
-	addr = of_translate_address(np, reg);
-	return devm_kasprintf(dev, GFP_KERNEL, "%llx-%s",
-			(unsigned long long)addr, name);
-}
-
 static int ap806_syscon_common_probe(struct platform_device *pdev,
 				     struct device_node *syscon_node)
 {
@@ -109,7 +97,7 @@ static int ap806_syscon_common_probe(struct platform_device *pdev,
 	cpuclk_freq *= 1000 * 1000;
 
 	/* CPU clocks depend on the Sample At Reset configuration */
-	name = ap806_unique_name(dev, syscon_node, "cpu-cluster-0");
+	name = ap_cp_unique_name(dev, syscon_node, "cpu-cluster-0");
 	ap806_clks[0] = clk_register_fixed_rate(dev, name, NULL,
 						0, cpuclk_freq);
 	if (IS_ERR(ap806_clks[0])) {
@@ -117,7 +105,7 @@ static int ap806_syscon_common_probe(struct platform_device *pdev,
 		goto fail0;
 	}
 
-	name = ap806_unique_name(dev, syscon_node, "cpu-cluster-1");
+	name = ap_cp_unique_name(dev, syscon_node, "cpu-cluster-1");
 	ap806_clks[1] = clk_register_fixed_rate(dev, name, NULL, 0,
 						cpuclk_freq);
 	if (IS_ERR(ap806_clks[1])) {
@@ -126,7 +114,7 @@ static int ap806_syscon_common_probe(struct platform_device *pdev,
 	}
 
 	/* Fixed clock is always 1200 Mhz */
-	fixedclk_name = ap806_unique_name(dev, syscon_node, "fixed");
+	fixedclk_name = ap_cp_unique_name(dev, syscon_node, "fixed");
 	ap806_clks[2] = clk_register_fixed_rate(dev, fixedclk_name, NULL,
 						0, 1200 * 1000 * 1000);
 	if (IS_ERR(ap806_clks[2])) {
@@ -135,7 +123,7 @@ static int ap806_syscon_common_probe(struct platform_device *pdev,
 	}
 
 	/* MSS Clock is fixed clock divided by 6 */
-	name = ap806_unique_name(dev, syscon_node, "mss");
+	name = ap_cp_unique_name(dev, syscon_node, "mss");
 	ap806_clks[3] = clk_register_fixed_factor(NULL, name, fixedclk_name,
 						  0, 1, 6);
 	if (IS_ERR(ap806_clks[3])) {
@@ -144,7 +132,7 @@ static int ap806_syscon_common_probe(struct platform_device *pdev,
 	}
 
 	/* SDIO(/eMMC) Clock is fixed clock divided by 3 */
-	name = ap806_unique_name(dev, syscon_node, "sdio");
+	name = ap_cp_unique_name(dev, syscon_node, "sdio");
 	ap806_clks[4] = clk_register_fixed_factor(NULL, name,
 						  fixedclk_name,
 						  0, 1, 3);
diff --git a/drivers/clk/mvebu/armada_ap_cp_helper.c b/drivers/clk/mvebu/armada_ap_cp_helper.c
new file mode 100644
index 000000000000..5f2457719b10
--- /dev/null
+++ b/drivers/clk/mvebu/armada_ap_cp_helper.c
@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Marvell Armada AP and CP110 helper
+ *
+ * Copyright (C) 2018 Marvell
+ *
+ * Gregory Clement <gregory.clement@bootlin.com>
+ *
+ */
+
+#include "armada_ap_cp_helper.h"
+#include <linux/device.h>
+
+char *ap_cp_unique_name(struct device *dev, struct device_node *np,
+			const char *name)
+{
+	const __be32 *reg;
+	u64 addr;
+
+	/* Do not create a name if there is no clock */
+	if (!name)
+		return NULL;
+
+	reg = of_get_property(np, "reg", NULL);
+	addr = of_translate_address(np, reg);
+	return devm_kasprintf(dev, GFP_KERNEL, "%llx-%s",
+			      (unsigned long long)addr, name);
+}
diff --git a/drivers/clk/mvebu/armada_ap_cp_helper.h b/drivers/clk/mvebu/armada_ap_cp_helper.h
new file mode 100644
index 000000000000..23aa29f3fcf7
--- /dev/null
+++ b/drivers/clk/mvebu/armada_ap_cp_helper.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+#ifndef __ARMADA_AP_CP_HELPER_H
+#define __ARMADA_AP_CP_HELPER_H
+
+#include <linux/of.h>
+#include <linux/of_address.h>
+
+char *ap_cp_unique_name(struct device *dev, struct device_node *np,
+			const char *name);
+#endif
diff --git a/drivers/clk/mvebu/cp110-system-controller.c b/drivers/clk/mvebu/cp110-system-controller.c
index 9781b1bf5998..e5f6c4b114b0 100644
--- a/drivers/clk/mvebu/cp110-system-controller.c
+++ b/drivers/clk/mvebu/cp110-system-controller.c
@@ -26,11 +26,11 @@
 
 #define pr_fmt(fmt) "cp110-system-controller: " fmt
 
+#include "armada_ap_cp_helper.h"
 #include <linux/clk-provider.h>
 #include <linux/mfd/syscon.h>
 #include <linux/init.h>
 #include <linux/of.h>
-#include <linux/of_address.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
@@ -212,22 +212,6 @@ static struct clk_hw *cp110_of_clk_get(struct of_phandle_args *clkspec,
 	return ERR_PTR(-EINVAL);
 }
 
-static char *cp110_unique_name(struct device *dev, struct device_node *np,
-			       const char *name)
-{
-	const __be32 *reg;
-	u64 addr;
-
-	/* Do not create a name if there is no clock */
-	if (!name)
-		return NULL;
-
-	reg = of_get_property(np, "reg", NULL);
-	addr = of_translate_address(np, reg);
-	return devm_kasprintf(dev, GFP_KERNEL, "%llx-%s",
-			      (unsigned long long)addr, name);
-}
-
 static int cp110_syscon_common_probe(struct platform_device *pdev,
 				     struct device_node *syscon_node)
 {
@@ -261,7 +245,7 @@ static int cp110_syscon_common_probe(struct platform_device *pdev,
 	cp110_clk_data->num = CP110_CLK_NUM;
 
 	/* Register the PLL0 which is the root of the hw tree */
-	pll0_name = cp110_unique_name(dev, syscon_node, "pll0");
+	pll0_name = ap_cp_unique_name(dev, syscon_node, "pll0");
 	hw = clk_hw_register_fixed_rate(NULL, pll0_name, NULL, 0,
 					1000 * 1000 * 1000);
 	if (IS_ERR(hw)) {
@@ -272,7 +256,7 @@ static int cp110_syscon_common_probe(struct platform_device *pdev,
 	cp110_clks[CP110_CORE_PLL0] = hw;
 
 	/* PPv2 is PLL0/3 */
-	ppv2_name = cp110_unique_name(dev, syscon_node, "ppv2-core");
+	ppv2_name = ap_cp_unique_name(dev, syscon_node, "ppv2-core");
 	hw = clk_hw_register_fixed_factor(NULL, ppv2_name, pll0_name, 0, 1, 3);
 	if (IS_ERR(hw)) {
 		ret = PTR_ERR(hw);
@@ -282,7 +266,7 @@ static int cp110_syscon_common_probe(struct platform_device *pdev,
 	cp110_clks[CP110_CORE_PPV2] = hw;
 
 	/* X2CORE clock is PLL0/2 */
-	x2core_name = cp110_unique_name(dev, syscon_node, "x2core");
+	x2core_name = ap_cp_unique_name(dev, syscon_node, "x2core");
 	hw = clk_hw_register_fixed_factor(NULL, x2core_name, pll0_name,
 					  0, 1, 2);
 	if (IS_ERR(hw)) {
@@ -293,7 +277,7 @@ static int cp110_syscon_common_probe(struct platform_device *pdev,
 	cp110_clks[CP110_CORE_X2CORE] = hw;
 
 	/* Core clock is X2CORE/2 */
-	core_name = cp110_unique_name(dev, syscon_node, "core");
+	core_name = ap_cp_unique_name(dev, syscon_node, "core");
 	hw = clk_hw_register_fixed_factor(NULL, core_name, x2core_name,
 					  0, 1, 2);
 	if (IS_ERR(hw)) {
@@ -303,7 +287,7 @@ static int cp110_syscon_common_probe(struct platform_device *pdev,
 
 	cp110_clks[CP110_CORE_CORE] = hw;
 	/* NAND can be either PLL0/2.5 or core clock */
-	nand_name = cp110_unique_name(dev, syscon_node, "nand-core");
+	nand_name = ap_cp_unique_name(dev, syscon_node, "nand-core");
 	if (nand_clk_ctrl & NF_CLOCK_SEL_400_MASK)
 		hw = clk_hw_register_fixed_factor(NULL, nand_name,
 						   pll0_name, 0, 2, 5);
@@ -318,7 +302,7 @@ static int cp110_syscon_common_probe(struct platform_device *pdev,
 	cp110_clks[CP110_CORE_NAND] = hw;
 
 	/* SDIO clock is PLL0/2.5 */
-	sdio_name = cp110_unique_name(dev, syscon_node, "sdio-core");
+	sdio_name = ap_cp_unique_name(dev, syscon_node, "sdio-core");
 	hw = clk_hw_register_fixed_factor(NULL, sdio_name,
 					  pll0_name, 0, 2, 5);
 	if (IS_ERR(hw)) {
@@ -330,7 +314,7 @@ static int cp110_syscon_common_probe(struct platform_device *pdev,
 
 	/* create the unique name for all the gate clocks */
 	for (i = 0; i < ARRAY_SIZE(gate_base_names); i++)
-		gate_name[i] =	cp110_unique_name(dev, syscon_node,
+		gate_name[i] =	ap_cp_unique_name(dev, syscon_node,
 						  gate_base_names[i]);
 
 	for (i = 0; i < ARRAY_SIZE(gate_base_names); i++) {
-- 
2.19.0

Re: [PATCH 2/6] clk: mvebu: add helper file for Armada AP and CP clocks

[Stephen Boyd]

Quoting Gregory CLEMENT (2018-09-22 11:17:05)
> diff --git a/drivers/clk/mvebu/armada_ap_cp_helper.c b/drivers/clk/mvebu/armada_ap_cp_helper.c
> new file mode 100644
> index 000000000000..5f2457719b10
> --- /dev/null
> +++ b/drivers/clk/mvebu/armada_ap_cp_helper.c
> @@ -0,0 +1,28 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Marvell Armada AP and CP110 helper
> + *
> + * Copyright (C) 2018 Marvell
> + *
> + * Gregory Clement <gregory.clement@bootlin.com>
> + *
> + */
> +
> +#include "armada_ap_cp_helper.h"
> +#include <linux/device.h>
> +
> +char *ap_cp_unique_name(struct device *dev, struct device_node *np,
> +                       const char *name)
> +{
> +       const __be32 *reg;
> +       u64 addr;
> +
> +       /* Do not create a name if there is no clock */
> +       if (!name)
> +               return NULL;
> +
> +       reg = of_get_property(np, "reg", NULL);
> +       addr = of_translate_address(np, reg);
> +       return devm_kasprintf(dev, GFP_KERNEL, "%llx-%s",
> +                             (unsigned long long)addr, name);

We probably don't want to leak physical addresses into debugfs, but I
guess this is how it's already been, so maybe something in the future
should change this for security purposes.

> +}
> diff --git a/drivers/clk/mvebu/armada_ap_cp_helper.h b/drivers/clk/mvebu/armada_ap_cp_helper.h
> new file mode 100644
> index 000000000000..23aa29f3fcf7
> --- /dev/null
> +++ b/drivers/clk/mvebu/armada_ap_cp_helper.h
> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +
> +#ifndef __ARMADA_AP_CP_HELPER_H
> +#define __ARMADA_AP_CP_HELPER_H
> +
> +#include <linux/of.h>
> +#include <linux/of_address.h>

Drop these includes? And forward declare 

struct device;
struct device_node;

> +
> +char *ap_cp_unique_name(struct device *dev, struct device_node *np,
> +                       const char *name);
> +#endif

[PATCH 3/6] clk: mvebu: add CPU clock driver for Armada 7K/8K

[Gregory CLEMENT]

The CPU frequency is managed at the AP level for the Armada 7K/8K. The
CPU frequency is modified by cluster: the CPUs of the same cluster have
the same frequency.

This patch adds the clock driver that will be used by CPUFreq, it is
based on the work of Omri Itach <omrii@marvell.com>.

Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
 drivers/clk/mvebu/Kconfig      |   3 +
 drivers/clk/mvebu/Makefile     |   1 +
 drivers/clk/mvebu/ap-cpu-clk.c | 265 +++++++++++++++++++++++++++++++++
 3 files changed, 269 insertions(+)
 create mode 100644 drivers/clk/mvebu/ap-cpu-clk.c

diff --git a/drivers/clk/mvebu/Kconfig b/drivers/clk/mvebu/Kconfig
index 5492fae3f0ab..9d0b2f7cee21 100644
--- a/drivers/clk/mvebu/Kconfig
+++ b/drivers/clk/mvebu/Kconfig
@@ -39,6 +39,9 @@ config ARMADA_AP806_SYSCON
 	bool
 	select ARMADA_AP_CP_HELPER
 
+config ARMADA_AP_CPU_CLK
+	bool
+
 config ARMADA_CP110_SYSCON
 	bool
 	select ARMADA_AP_CP_HELPER
diff --git a/drivers/clk/mvebu/Makefile b/drivers/clk/mvebu/Makefile
index 6638ad962ec1..04464cef0f06 100644
--- a/drivers/clk/mvebu/Makefile
+++ b/drivers/clk/mvebu/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_ARMADA_37XX_CLK)	+= armada-37xx-tbg.o
 obj-$(CONFIG_ARMADA_37XX_CLK)	+= armada-37xx-periph.o
 obj-$(CONFIG_ARMADA_XP_CLK)	+= armada-xp.o mv98dx3236.o
 obj-$(CONFIG_ARMADA_AP806_SYSCON) += ap806-system-controller.o
+obj-$(CONFIG_ARMADA_AP_CPU_CLK) += ap-cpu-clk.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
diff --git a/drivers/clk/mvebu/ap-cpu-clk.c b/drivers/clk/mvebu/ap-cpu-clk.c
new file mode 100644
index 000000000000..1b498cfe9191
--- /dev/null
+++ b/drivers/clk/mvebu/ap-cpu-clk.c
@@ -0,0 +1,265 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Marvell Armada AP CPU Clock Controller
+ *
+ * Copyright (C) 2018 Marvell
+ *
+ * Omri Itach <omrii@marvell.com>
+ * Gregory Clement <gregory.clement@bootlin.com>
+ */
+
+#define pr_fmt(fmt) "ap-cpu-clk: " fmt
+
+#include "armada_ap_cp_helper.h"
+#include <linux/clk-provider.h>
+#include <linux/clk.h>
+#include <linux/init.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define AP806_CPU_CLUSTER0		0
+#define AP806_CPU_CLUSTER1		1
+#define AP806_CPUS_PER_CLUSTER		2
+#define APN806_CPU1_MASK		0x1
+
+#define APN806_CLUSTER_NUM_OFFSET	8
+#define APN806_CLUSTER_NUM_MASK		(1 << APN806_CLUSTER_NUM_OFFSET)
+
+#define APN806_MAX_DIVIDER		32
+
+/* AP806 CPU DFS register mapping*/
+#define AP806_CA72MP2_0_PLL_CR_0_REG_OFFSET		0x278
+#define AP806_CA72MP2_0_PLL_CR_1_REG_OFFSET		0x280
+#define AP806_CA72MP2_0_PLL_CR_2_REG_OFFSET		0x284
+#define AP806_CA72MP2_0_PLL_SR_REG_OFFSET		0xC94
+
+#define AP806_CA72MP2_0_PLL_CR_CLUSTER_OFFSET		0x14
+#define AP806_PLL_CR_0_CPU_CLK_DIV_RATIO_OFFSET		0
+#define AP806_PLL_CR_0_CPU_CLK_DIV_RATIO_MASK \
+			(0x3f << AP806_PLL_CR_0_CPU_CLK_DIV_RATIO_OFFSET)
+#define AP806_PLL_CR_0_CPU_CLK_RELOAD_FORCE_OFFSET	24
+#define AP806_PLL_CR_0_CPU_CLK_RELOAD_FORCE_MASK \
+			(0x1 << AP806_PLL_CR_0_CPU_CLK_RELOAD_FORCE_OFFSET)
+#define AP806_PLL_CR_0_CPU_CLK_RELOAD_RATIO_OFFSET	16
+#define AP806_CA72MP2_0_PLL_RATIO_STATE			11
+
+#define to_clk(hw) container_of(hw, struct ap_cpu_clk, hw)
+
+/*
+ * struct ap806_clk: CPU cluster clock controller instance
+ * @cluster: Cluster clock controller index
+ * @clk_name: Cluster clock controller name
+ * @dev : Cluster clock device
+ * @hw: HW specific structure of Cluster clock controller
+ * @pll_cr_base: CA72MP2 Register base (Device Sample at Reset register)
+ */
+struct ap_cpu_clk {
+	int cluster;
+	const char *clk_name;
+	struct device *dev;
+	struct clk_hw hw;
+	struct regmap *pll_cr_base;
+};
+
+static struct clk **cluster_clks;
+static struct clk_onecell_data clk_data;
+
+static unsigned long ap_cpu_clk_recalc_rate(struct clk_hw *hw,
+					    unsigned long parent_rate)
+{
+	struct ap_cpu_clk *clk = to_clk(hw);
+	unsigned int cpu_clkdiv_reg = AP806_CA72MP2_0_PLL_CR_0_REG_OFFSET +
+		(clk->cluster * AP806_CA72MP2_0_PLL_CR_CLUSTER_OFFSET);
+	int cpu_clkdiv_ratio;
+
+	regmap_read(clk->pll_cr_base, cpu_clkdiv_reg, &cpu_clkdiv_ratio);
+	cpu_clkdiv_ratio &= AP806_PLL_CR_0_CPU_CLK_DIV_RATIO_MASK;
+	cpu_clkdiv_ratio >>= AP806_PLL_CR_0_CPU_CLK_DIV_RATIO_OFFSET;
+
+	return parent_rate / cpu_clkdiv_ratio;
+}
+
+static int ap_cpu_clk_set_rate(struct clk_hw *hw, unsigned long rate,
+			      unsigned long parent_rate)
+{
+	struct ap_cpu_clk *clk = to_clk(hw);
+	int reg, divider = parent_rate / rate;
+	unsigned int cpu_clkdiv_reg, cpu_force_reg, cpu_ratio_reg;
+
+	cpu_clkdiv_reg = AP806_CA72MP2_0_PLL_CR_0_REG_OFFSET +
+		(clk->cluster * AP806_CA72MP2_0_PLL_CR_CLUSTER_OFFSET);
+	cpu_force_reg = AP806_CA72MP2_0_PLL_CR_1_REG_OFFSET +
+		(clk->cluster * AP806_CA72MP2_0_PLL_CR_CLUSTER_OFFSET);
+	cpu_ratio_reg = AP806_CA72MP2_0_PLL_CR_2_REG_OFFSET +
+		(clk->cluster * AP806_CA72MP2_0_PLL_CR_CLUSTER_OFFSET);
+
+	/* 1. Set CPU divider */
+	regmap_update_bits(clk->pll_cr_base, cpu_clkdiv_reg,
+			   AP806_PLL_CR_0_CPU_CLK_DIV_RATIO_MASK, divider);
+
+	/* 2. Set Reload force */
+	regmap_update_bits(clk->pll_cr_base, cpu_force_reg,
+			   AP806_PLL_CR_0_CPU_CLK_RELOAD_FORCE_MASK,
+			   AP806_PLL_CR_0_CPU_CLK_RELOAD_FORCE_MASK);
+
+	/* 3. Set Reload ratio */
+	regmap_update_bits(clk->pll_cr_base, cpu_ratio_reg,
+			   BIT(AP806_PLL_CR_0_CPU_CLK_RELOAD_RATIO_OFFSET),
+			   BIT(AP806_PLL_CR_0_CPU_CLK_RELOAD_RATIO_OFFSET));
+
+
+	/* 4. Wait for stabilizing CPU Clock */
+	do {
+		regmap_read(clk->pll_cr_base,
+			    AP806_CA72MP2_0_PLL_SR_REG_OFFSET,
+			    &reg);
+	} while (!(reg & BIT(clk->cluster * AP806_CA72MP2_0_PLL_RATIO_STATE)));
+
+	/* 5. Clear Reload ratio */
+	regmap_update_bits(clk->pll_cr_base, cpu_ratio_reg,
+			   BIT(AP806_PLL_CR_0_CPU_CLK_RELOAD_RATIO_OFFSET), 0);
+
+	return 0;
+}
+
+
+static long ap_cpu_clk_round_rate(struct clk_hw *hw, unsigned long rate,
+				 unsigned long *parent_rate)
+{
+	int divider = *parent_rate / rate;
+
+	if (divider > APN806_MAX_DIVIDER)
+		divider = APN806_MAX_DIVIDER;
+
+	return *parent_rate / divider;
+}
+
+static const struct clk_ops ap_cpu_clk_ops = {
+	.recalc_rate	= ap_cpu_clk_recalc_rate,
+	.round_rate	= ap_cpu_clk_round_rate,
+	.set_rate	= ap_cpu_clk_set_rate,
+};
+
+static int ap_cpu_clock_probe(struct platform_device *pdev)
+{
+	int ret, nclusters = 0, cluster_index = 0;
+	struct device *dev = &pdev->dev;
+	struct device_node *dn, *np = dev->of_node;
+	struct ap_cpu_clk *ap_cpu_clk;
+	struct regmap *regmap;
+
+	regmap = syscon_node_to_regmap(np->parent);
+	if (IS_ERR(regmap)) {
+		pr_err("cannot get pll_cr_base regmap\n");
+		return PTR_ERR(regmap);
+	}
+
+	/*
+	 * AP806 has 4 cpus and DFS for AP806 is controlled per
+	 * cluster (2 CPUs per cluster), cpu0 and cpu1 are fixed to
+	 * cluster0 while cpu2 and cpu3 are fixed to cluster1 whether
+	 * they are enabled or not.  Since cpu0 is the boot cpu, then
+	 * cluster0 must exist.  If cpu2 or cpu3 is enabled, cluster1
+	 * will exist and the cluster number is 2; otherwise the
+	 * cluster number is 1.
+	 */
+	nclusters = 1;
+	for_each_node_by_type(dn, "cpu") {
+		int cpu, err;
+
+		err = of_property_read_u32(dn, "reg", &cpu);
+		if (WARN_ON(err))
+			return err;
+
+		/* If cpu2 or cpu3 is enabled */
+		if ((cpu & APN806_CLUSTER_NUM_MASK)) {
+			nclusters = 2;
+			break;
+		}
+	}
+	/*
+	 * DFS for AP806 is controlled per cluster (2 CPUs per cluster),
+	 * so allocate structs per cluster
+	 */
+	ap_cpu_clk = devm_kcalloc(dev, nclusters, sizeof(*ap_cpu_clk),
+				  GFP_KERNEL);
+	if (WARN_ON(!ap_cpu_clk))
+		return -ENOMEM;
+
+	cluster_clks = devm_kcalloc(dev, nclusters, sizeof(*cluster_clks),
+				    GFP_KERNEL);
+	if (WARN_ON(!cluster_clks))
+		return -ENOMEM;
+
+	for_each_node_by_type(dn, "cpu") {
+		char *clk_name = "cpu-cluster-0";
+		struct clk_init_data init;
+		const char *parent_name;
+		struct clk *clk, *parent;
+		int cpu, err;
+
+		err = of_property_read_u32(dn, "reg", &cpu);
+		if (WARN_ON(err))
+			return err;
+
+		cluster_index = (cpu & APN806_CLUSTER_NUM_MASK);
+		cluster_index >>= APN806_CLUSTER_NUM_OFFSET;
+
+		/* Initialize once for one cluster */
+		if (cluster_clks[cluster_index])
+			continue;
+
+		parent = of_clk_get(np, cluster_index);
+		if (IS_ERR(parent)) {
+			dev_err(dev, "Could not get the clock parent\n");
+			return -EINVAL;
+		}
+		parent_name =  __clk_get_name(parent);
+		clk_name[12] += cluster_index;
+		ap_cpu_clk[cluster_index].clk_name =
+			ap_cp_unique_name(dev, np->parent, clk_name);
+		ap_cpu_clk[cluster_index].cluster = cluster_index;
+		ap_cpu_clk[cluster_index].pll_cr_base = regmap;
+		ap_cpu_clk[cluster_index].hw.init = &init;
+		ap_cpu_clk[cluster_index].dev = dev;
+
+		init.name = ap_cpu_clk[cluster_index].clk_name;
+		init.ops = &ap_cpu_clk_ops;
+		init.num_parents = 1;
+		init.parent_names = &parent_name;
+		init.flags = CLK_GET_RATE_NOCACHE;
+
+		clk = devm_clk_register(dev, &ap_cpu_clk[cluster_index].hw);
+		if (IS_ERR(clk))
+			return PTR_ERR(clk);
+
+		cluster_clks[cluster_index] = clk;
+	}
+
+	clk_data.clk_num = cluster_index + 1;
+	clk_data.clks = cluster_clks;
+
+	ret = of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
+	if (ret)
+		dev_err(dev, "failed to register OF clock provider\n");
+
+	return ret;
+}
+
+static const struct of_device_id ap_cpu_clock_of_match[] = {
+	{ .compatible = "marvell,ap806-cpu-clock", },
+	{ }
+};
+
+static struct platform_driver ap_cpu_clock_driver = {
+	.probe = ap_cpu_clock_probe,
+	.driver		= {
+		.name	= "marvell-ap-cpu-clock",
+		.of_match_table = ap_cpu_clock_of_match,
+		.suppress_bind_attrs = true,
+	},
+};
+builtin_platform_driver(ap_cpu_clock_driver);
-- 
2.19.0

Re: [PATCH 3/6] clk: mvebu: add CPU clock driver for Armada 7K/8K

[Stephen Boyd]

Quoting Gregory CLEMENT (2018-09-22 11:17:06)
> diff --git a/drivers/clk/mvebu/ap-cpu-clk.c b/drivers/clk/mvebu/ap-cpu-clk.c
> new file mode 100644
> index 000000000000..1b498cfe9191
> --- /dev/null
> +++ b/drivers/clk/mvebu/ap-cpu-clk.c
> @@ -0,0 +1,265 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Marvell Armada AP CPU Clock Controller
> + *
> + * Copyright (C) 2018 Marvell
> + *
> + * Omri Itach <omrii@marvell.com>
> + * Gregory Clement <gregory.clement@bootlin.com>
> + */
> +
> +#define pr_fmt(fmt) "ap-cpu-clk: " fmt
> +
> +#include "armada_ap_cp_helper.h"

Does this include need to come first? Preferably local includes come
after any <linux> ones.

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

Is this used?

> +#include <linux/mfd/syscon.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#define AP806_CPU_CLUSTER0             0
> +#define AP806_CPU_CLUSTER1             1
> +#define AP806_CPUS_PER_CLUSTER         2
> +#define APN806_CPU1_MASK               0x1
> +
> +#define APN806_CLUSTER_NUM_OFFSET      8
> +#define APN806_CLUSTER_NUM_MASK                (1 << APN806_CLUSTER_NUM_OFFSET)
> +
> +#define APN806_MAX_DIVIDER             32
> +
> +/* AP806 CPU DFS register mapping*/
> +#define AP806_CA72MP2_0_PLL_CR_0_REG_OFFSET            0x278
> +#define AP806_CA72MP2_0_PLL_CR_1_REG_OFFSET            0x280
> +#define AP806_CA72MP2_0_PLL_CR_2_REG_OFFSET            0x284
> +#define AP806_CA72MP2_0_PLL_SR_REG_OFFSET              0xC94
> +
> +#define AP806_CA72MP2_0_PLL_CR_CLUSTER_OFFSET          0x14
> +#define AP806_PLL_CR_0_CPU_CLK_DIV_RATIO_OFFSET                0
> +#define AP806_PLL_CR_0_CPU_CLK_DIV_RATIO_MASK \
> +                       (0x3f << AP806_PLL_CR_0_CPU_CLK_DIV_RATIO_OFFSET)
> +#define AP806_PLL_CR_0_CPU_CLK_RELOAD_FORCE_OFFSET     24
> +#define AP806_PLL_CR_0_CPU_CLK_RELOAD_FORCE_MASK \
> +                       (0x1 << AP806_PLL_CR_0_CPU_CLK_RELOAD_FORCE_OFFSET)
> +#define AP806_PLL_CR_0_CPU_CLK_RELOAD_RATIO_OFFSET     16
> +#define AP806_CA72MP2_0_PLL_RATIO_STATE                        11
> +
> +#define to_clk(hw) container_of(hw, struct ap_cpu_clk, hw)
> +
> +/*
> + * struct ap806_clk: CPU cluster clock controller instance
> + * @cluster: Cluster clock controller index
> + * @clk_name: Cluster clock controller name
> + * @dev : Cluster clock device
> + * @hw: HW specific structure of Cluster clock controller
> + * @pll_cr_base: CA72MP2 Register base (Device Sample at Reset register)
> + */
> +struct ap_cpu_clk {
> +       int cluster;

unsigned?

> +       const char *clk_name;
> +       struct device *dev;
> +       struct clk_hw hw;
> +       struct regmap *pll_cr_base;
> +};
> +
> +static struct clk **cluster_clks;
> +static struct clk_onecell_data clk_data;

Can you give these some better names that are ap_cpu specific? Grepping
for clk_data will make lots of hits otherwise because they're so
generic.

> +
> +static unsigned long ap_cpu_clk_recalc_rate(struct clk_hw *hw,
> +                                           unsigned long parent_rate)
> +{
> +       struct ap_cpu_clk *clk = to_clk(hw);
> +       unsigned int cpu_clkdiv_reg = AP806_CA72MP2_0_PLL_CR_0_REG_OFFSET +
> +               (clk->cluster * AP806_CA72MP2_0_PLL_CR_CLUSTER_OFFSET);

Nitpick: This is pretty unreadable. Can you just declare the variables
up here and then do the assignment later on in the function and avoid
splitting across many lines?

> +       int cpu_clkdiv_ratio;
> +
> +       regmap_read(clk->pll_cr_base, cpu_clkdiv_reg, &cpu_clkdiv_ratio);
> +       cpu_clkdiv_ratio &= AP806_PLL_CR_0_CPU_CLK_DIV_RATIO_MASK;
> +       cpu_clkdiv_ratio >>= AP806_PLL_CR_0_CPU_CLK_DIV_RATIO_OFFSET;
> +
> +       return parent_rate / cpu_clkdiv_ratio;
> +}
> +
> +static int ap_cpu_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> +                             unsigned long parent_rate)
> +{
> +       struct ap_cpu_clk *clk = to_clk(hw);
> +       int reg, divider = parent_rate / rate;
> +       unsigned int cpu_clkdiv_reg, cpu_force_reg, cpu_ratio_reg;
> +
> +       cpu_clkdiv_reg = AP806_CA72MP2_0_PLL_CR_0_REG_OFFSET +
> +               (clk->cluster * AP806_CA72MP2_0_PLL_CR_CLUSTER_OFFSET);
> +       cpu_force_reg = AP806_CA72MP2_0_PLL_CR_1_REG_OFFSET +
> +               (clk->cluster * AP806_CA72MP2_0_PLL_CR_CLUSTER_OFFSET);
> +       cpu_ratio_reg = AP806_CA72MP2_0_PLL_CR_2_REG_OFFSET +
> +               (clk->cluster * AP806_CA72MP2_0_PLL_CR_CLUSTER_OFFSET);
> +
> +       /* 1. Set CPU divider */
> +       regmap_update_bits(clk->pll_cr_base, cpu_clkdiv_reg,
> +                          AP806_PLL_CR_0_CPU_CLK_DIV_RATIO_MASK, divider);
> +
> +       /* 2. Set Reload force */
> +       regmap_update_bits(clk->pll_cr_base, cpu_force_reg,
> +                          AP806_PLL_CR_0_CPU_CLK_RELOAD_FORCE_MASK,
> +                          AP806_PLL_CR_0_CPU_CLK_RELOAD_FORCE_MASK);
> +
> +       /* 3. Set Reload ratio */
> +       regmap_update_bits(clk->pll_cr_base, cpu_ratio_reg,
> +                          BIT(AP806_PLL_CR_0_CPU_CLK_RELOAD_RATIO_OFFSET),
> +                          BIT(AP806_PLL_CR_0_CPU_CLK_RELOAD_RATIO_OFFSET));
> +
> +
> +       /* 4. Wait for stabilizing CPU Clock */
> +       do {
> +               regmap_read(clk->pll_cr_base,
> +                           AP806_CA72MP2_0_PLL_SR_REG_OFFSET,
> +                           &reg);
> +       } while (!(reg & BIT(clk->cluster * AP806_CA72MP2_0_PLL_RATIO_STATE)));

Use regmap_read_poll_timeout() API here and give some large timeout
value so that we don't get stuck waiting here forever?

> +
> +       /* 5. Clear Reload ratio */

I'm not sure we really need these comments. They're just saying what the
code is doing, so they don't add much value.

> +       regmap_update_bits(clk->pll_cr_base, cpu_ratio_reg,
> +                          BIT(AP806_PLL_CR_0_CPU_CLK_RELOAD_RATIO_OFFSET), 0);
> +
> +       return 0;
> +}
> +
> +
> +static long ap_cpu_clk_round_rate(struct clk_hw *hw, unsigned long rate,
> +                                unsigned long *parent_rate)
> +{
> +       int divider = *parent_rate / rate;
> +
> +       if (divider > APN806_MAX_DIVIDER)
> +               divider = APN806_MAX_DIVIDER;

	divider = min(divider, APN806_MAX_DIVIDER);

> +
> +       return *parent_rate / divider;
> +}
> +
> +static const struct clk_ops ap_cpu_clk_ops = {
> +       .recalc_rate    = ap_cpu_clk_recalc_rate,
> +       .round_rate     = ap_cpu_clk_round_rate,
> +       .set_rate       = ap_cpu_clk_set_rate,
> +};
> +
> +static int ap_cpu_clock_probe(struct platform_device *pdev)
> +{
> +       int ret, nclusters = 0, cluster_index = 0;
> +       struct device *dev = &pdev->dev;
> +       struct device_node *dn, *np = dev->of_node;
> +       struct ap_cpu_clk *ap_cpu_clk;
> +       struct regmap *regmap;
> +
> +       regmap = syscon_node_to_regmap(np->parent);

Can we just call dev_get_remap() on pdev->dev.parent?

> +       if (IS_ERR(regmap)) {
> +               pr_err("cannot get pll_cr_base regmap\n");
> +               return PTR_ERR(regmap);
> +       }
> +
> +       /*
> +        * AP806 has 4 cpus and DFS for AP806 is controlled per
> +        * cluster (2 CPUs per cluster), cpu0 and cpu1 are fixed to
> +        * cluster0 while cpu2 and cpu3 are fixed to cluster1 whether
> +        * they are enabled or not.  Since cpu0 is the boot cpu, then
> +        * cluster0 must exist.  If cpu2 or cpu3 is enabled, cluster1
> +        * will exist and the cluster number is 2; otherwise the
> +        * cluster number is 1.
> +        */
> +       nclusters = 1;
> +       for_each_node_by_type(dn, "cpu") {

Isn't there some sort of for_each_cpu_node() API that can be used here?

> +               int cpu, err;
> +
> +               err = of_property_read_u32(dn, "reg", &cpu);
> +               if (WARN_ON(err))
> +                       return err;
> +
> +               /* If cpu2 or cpu3 is enabled */
> +               if ((cpu & APN806_CLUSTER_NUM_MASK)) {
> +                       nclusters = 2;
> +                       break;
> +               }
> +       }
> +       /*
> +        * DFS for AP806 is controlled per cluster (2 CPUs per cluster),
> +        * so allocate structs per cluster
> +        */
> +       ap_cpu_clk = devm_kcalloc(dev, nclusters, sizeof(*ap_cpu_clk),
> +                                 GFP_KERNEL);
> +       if (WARN_ON(!ap_cpu_clk))
> +               return -ENOMEM;
> +
> +       cluster_clks = devm_kcalloc(dev, nclusters, sizeof(*cluster_clks),
> +                                   GFP_KERNEL);
> +       if (WARN_ON(!cluster_clks))

Drop the WARN_ON please. If allocation fails we'll already get a large
stack trace and messages.

> +               return -ENOMEM;
> +
> +       for_each_node_by_type(dn, "cpu") {
> +               char *clk_name = "cpu-cluster-0";
> +               struct clk_init_data init;
> +               const char *parent_name;
> +               struct clk *clk, *parent;
> +               int cpu, err;
> +
> +               err = of_property_read_u32(dn, "reg", &cpu);
> +               if (WARN_ON(err))
> +                       return err;
> +
> +               cluster_index = (cpu & APN806_CLUSTER_NUM_MASK);

Nitpick: Drop useless parenthesis please.

> +               cluster_index >>= APN806_CLUSTER_NUM_OFFSET;
> +
> +               /* Initialize once for one cluster */
> +               if (cluster_clks[cluster_index])
> +                       continue;
> +
> +               parent = of_clk_get(np, cluster_index);
> +               if (IS_ERR(parent)) {
> +                       dev_err(dev, "Could not get the clock parent\n");
> +                       return -EINVAL;
> +               }
> +               parent_name =  __clk_get_name(parent);
> +               clk_name[12] += cluster_index;
> +               ap_cpu_clk[cluster_index].clk_name =
> +                       ap_cp_unique_name(dev, np->parent, clk_name);
> +               ap_cpu_clk[cluster_index].cluster = cluster_index;
> +               ap_cpu_clk[cluster_index].pll_cr_base = regmap;
> +               ap_cpu_clk[cluster_index].hw.init = &init;
> +               ap_cpu_clk[cluster_index].dev = dev;
> +
> +               init.name = ap_cpu_clk[cluster_index].clk_name;
> +               init.ops = &ap_cpu_clk_ops;
> +               init.num_parents = 1;
> +               init.parent_names = &parent_name;
> +               init.flags = CLK_GET_RATE_NOCACHE;

Can you add a comment why we need NOCACHE here? It isn't clear why this
is important.

> +
> +               clk = devm_clk_register(dev, &ap_cpu_clk[cluster_index].hw);

Please use the clk_hw based registration APIs.

> +               if (IS_ERR(clk))
> +                       return PTR_ERR(clk);
> +
> +               cluster_clks[cluster_index] = clk;
> +       }
> +
> +       clk_data.clk_num = cluster_index + 1;
> +       clk_data.clks = cluster_clks;
> +
> +       ret = of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);

And the clk_hw based provider APIs.

> +       if (ret)
> +               dev_err(dev, "failed to register OF clock provider\n");
> +
> +       return ret;
> +}
> +

Re: [PATCH 3/6] clk: mvebu: add CPU clock driver for Armada 7K/8K

[Gregory CLEMENT]

Hi Stephen,
 
 On mer., oct. 17 2018, Stephen Boyd <sboyd@kernel.org> wrote:

> Quoting Gregory CLEMENT (2018-09-22 11:17:06)
>> diff --git a/drivers/clk/mvebu/ap-cpu-clk.c b/drivers/clk/mvebu/ap-cpu-clk.c
>> new file mode 100644
>> index 000000000000..1b498cfe9191
>> --- /dev/null
>> +++ b/drivers/clk/mvebu/ap-cpu-clk.c
>> @@ -0,0 +1,265 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Marvell Armada AP CPU Clock Controller
>> + *
>> + * Copyright (C) 2018 Marvell
>> + *
>> + * Omri Itach <omrii@marvell.com>
>> + * Gregory Clement <gregory.clement@bootlin.com>
>> + */
>> +
>> +#define pr_fmt(fmt) "ap-cpu-clk: " fmt
>> +
>> +#include "armada_ap_cp_helper.h"
>
> Does this include need to come first? Preferably local includes come
> after any <linux> ones.

OK

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

During development but no more now, I remove it.

>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +
>> +#define AP806_CPU_CLUSTER0             0
>> +#define AP806_CPU_CLUSTER1             1
>> +#define AP806_CPUS_PER_CLUSTER         2
>> +#define APN806_CPU1_MASK               0x1
>> +
>> +#define APN806_CLUSTER_NUM_OFFSET      8
>> +#define APN806_CLUSTER_NUM_MASK                (1 << APN806_CLUSTER_NUM_OFFSET)
>> +
>> +#define APN806_MAX_DIVIDER             32
>> +
>> +/* AP806 CPU DFS register mapping*/
>> +#define AP806_CA72MP2_0_PLL_CR_0_REG_OFFSET            0x278
>> +#define AP806_CA72MP2_0_PLL_CR_1_REG_OFFSET            0x280
>> +#define AP806_CA72MP2_0_PLL_CR_2_REG_OFFSET            0x284
>> +#define AP806_CA72MP2_0_PLL_SR_REG_OFFSET              0xC94
>> +
>> +#define AP806_CA72MP2_0_PLL_CR_CLUSTER_OFFSET          0x14
>> +#define AP806_PLL_CR_0_CPU_CLK_DIV_RATIO_OFFSET                0
>> +#define AP806_PLL_CR_0_CPU_CLK_DIV_RATIO_MASK \
>> +                       (0x3f << AP806_PLL_CR_0_CPU_CLK_DIV_RATIO_OFFSET)
>> +#define AP806_PLL_CR_0_CPU_CLK_RELOAD_FORCE_OFFSET     24
>> +#define AP806_PLL_CR_0_CPU_CLK_RELOAD_FORCE_MASK \
>> +                       (0x1 << AP806_PLL_CR_0_CPU_CLK_RELOAD_FORCE_OFFSET)
>> +#define AP806_PLL_CR_0_CPU_CLK_RELOAD_RATIO_OFFSET     16
>> +#define AP806_CA72MP2_0_PLL_RATIO_STATE                        11
>> +
>> +#define to_clk(hw) container_of(hw, struct ap_cpu_clk, hw)
>> +
>> +/*
>> + * struct ap806_clk: CPU cluster clock controller instance
>> + * @cluster: Cluster clock controller index
>> + * @clk_name: Cluster clock controller name
>> + * @dev : Cluster clock device
>> + * @hw: HW specific structure of Cluster clock controller
>> + * @pll_cr_base: CA72MP2 Register base (Device Sample at Reset register)
>> + */
>> +struct ap_cpu_clk {
>> +       int cluster;
>
> unsigned?

I don't expect to have so many clusters that a signed int would be too
short :)

Actually I used int to let the compiler choose what ever he wants.

But if you really want I can use a unsigned int

>
>> +       const char *clk_name;
>> +       struct device *dev;
>> +       struct clk_hw hw;
>> +       struct regmap *pll_cr_base;
>> +};
>> +
>> +static struct clk **cluster_clks;
>> +static struct clk_onecell_data clk_data;
>
> Can you give these some better names that are ap_cpu specific? Grepping
> for clk_data will make lots of hits otherwise because they're so
> generic.

OK I will try to be more creative ;)

>
>> +
>> +static unsigned long ap_cpu_clk_recalc_rate(struct clk_hw *hw,
>> +                                           unsigned long parent_rate)
>> +{
>> +       struct ap_cpu_clk *clk = to_clk(hw);
>> +       unsigned int cpu_clkdiv_reg = AP806_CA72MP2_0_PLL_CR_0_REG_OFFSET +
>> +               (clk->cluster * AP806_CA72MP2_0_PLL_CR_CLUSTER_OFFSET);
>
> Nitpick: This is pretty unreadable. Can you just declare the variables
> up here and then do the assignment later on in the function and avoid
> splitting across many lines?

OK

>
>> +       int cpu_clkdiv_ratio;
>> +
>> +       regmap_read(clk->pll_cr_base, cpu_clkdiv_reg, &cpu_clkdiv_ratio);
>> +       cpu_clkdiv_ratio &= AP806_PLL_CR_0_CPU_CLK_DIV_RATIO_MASK;
>> +       cpu_clkdiv_ratio >>= AP806_PLL_CR_0_CPU_CLK_DIV_RATIO_OFFSET;
>> +
>> +       return parent_rate / cpu_clkdiv_ratio;
>> +}
>> +
>> +static int ap_cpu_clk_set_rate(struct clk_hw *hw, unsigned long rate,
>> +                             unsigned long parent_rate)
>> +{
>> +       struct ap_cpu_clk *clk = to_clk(hw);
>> +       int reg, divider = parent_rate / rate;
>> +       unsigned int cpu_clkdiv_reg, cpu_force_reg, cpu_ratio_reg;
>> +
>> +       cpu_clkdiv_reg = AP806_CA72MP2_0_PLL_CR_0_REG_OFFSET +
>> +               (clk->cluster * AP806_CA72MP2_0_PLL_CR_CLUSTER_OFFSET);
>> +       cpu_force_reg = AP806_CA72MP2_0_PLL_CR_1_REG_OFFSET +
>> +               (clk->cluster * AP806_CA72MP2_0_PLL_CR_CLUSTER_OFFSET);
>> +       cpu_ratio_reg = AP806_CA72MP2_0_PLL_CR_2_REG_OFFSET +
>> +               (clk->cluster * AP806_CA72MP2_0_PLL_CR_CLUSTER_OFFSET);
>> +
>> +       /* 1. Set CPU divider */
>> +       regmap_update_bits(clk->pll_cr_base, cpu_clkdiv_reg,
>> +                          AP806_PLL_CR_0_CPU_CLK_DIV_RATIO_MASK, divider);
>> +
>> +       /* 2. Set Reload force */
>> +       regmap_update_bits(clk->pll_cr_base, cpu_force_reg,
>> +                          AP806_PLL_CR_0_CPU_CLK_RELOAD_FORCE_MASK,
>> +                          AP806_PLL_CR_0_CPU_CLK_RELOAD_FORCE_MASK);
>> +
>> +       /* 3. Set Reload ratio */
>> +       regmap_update_bits(clk->pll_cr_base, cpu_ratio_reg,
>> +                          BIT(AP806_PLL_CR_0_CPU_CLK_RELOAD_RATIO_OFFSET),
>> +                          BIT(AP806_PLL_CR_0_CPU_CLK_RELOAD_RATIO_OFFSET));
>> +
>> +
>> +       /* 4. Wait for stabilizing CPU Clock */
>> +       do {
>> +               regmap_read(clk->pll_cr_base,
>> +                           AP806_CA72MP2_0_PLL_SR_REG_OFFSET,
>> +                           &reg);
>> +       } while (!(reg & BIT(clk->cluster * AP806_CA72MP2_0_PLL_RATIO_STATE)));
>
> Use regmap_read_poll_timeout() API here and give some large timeout
> value so that we don't get stuck waiting here forever?

Indeed regmap_read_poll_timeout() seems a nice improvement

>
>> +
>> +       /* 5. Clear Reload ratio */
>
> I'm not sure we really need these comments. They're just saying what the
> code is doing, so they don't add much value.

Actually, these 5 steps directly come from the datasheet.

>
>> +       regmap_update_bits(clk->pll_cr_base, cpu_ratio_reg,
>> +                          BIT(AP806_PLL_CR_0_CPU_CLK_RELOAD_RATIO_OFFSET), 0);
>> +
>> +       return 0;
>> +}
>> +
>> +
>> +static long ap_cpu_clk_round_rate(struct clk_hw *hw, unsigned long rate,
>> +                                unsigned long *parent_rate)
>> +{
>> +       int divider = *parent_rate / rate;
>> +
>> +       if (divider > APN806_MAX_DIVIDER)
>> +               divider = APN806_MAX_DIVIDER;
>
> 	divider = min(divider, APN806_MAX_DIVIDER);

OK
>
>> +
>> +       return *parent_rate / divider;
>> +}
>> +
>> +static const struct clk_ops ap_cpu_clk_ops = {
>> +       .recalc_rate    = ap_cpu_clk_recalc_rate,
>> +       .round_rate     = ap_cpu_clk_round_rate,
>> +       .set_rate       = ap_cpu_clk_set_rate,
>> +};
>> +
>> +static int ap_cpu_clock_probe(struct platform_device *pdev)
>> +{
>> +       int ret, nclusters = 0, cluster_index = 0;
>> +       struct device *dev = &pdev->dev;
>> +       struct device_node *dn, *np = dev->of_node;
>> +       struct ap_cpu_clk *ap_cpu_clk;
>> +       struct regmap *regmap;
>> +
>> +       regmap = syscon_node_to_regmap(np->parent);
>
> Can we just call dev_get_remap() on pdev->dev.parent?

we could do regmap = dev_get_regmap(pdev->dev.parent, NULL); instead of
this line. But is it really better?

>
>> +       if (IS_ERR(regmap)) {
>> +               pr_err("cannot get pll_cr_base regmap\n");
>> +               return PTR_ERR(regmap);
>> +       }
>> +
>> +       /*
>> +        * AP806 has 4 cpus and DFS for AP806 is controlled per
>> +        * cluster (2 CPUs per cluster), cpu0 and cpu1 are fixed to
>> +        * cluster0 while cpu2 and cpu3 are fixed to cluster1 whether
>> +        * they are enabled or not.  Since cpu0 is the boot cpu, then
>> +        * cluster0 must exist.  If cpu2 or cpu3 is enabled, cluster1
>> +        * will exist and the cluster number is 2; otherwise the
>> +        * cluster number is 1.
>> +        */
>> +       nclusters = 1;
>> +       for_each_node_by_type(dn, "cpu") {
>
> Isn't there some sort of for_each_cpu_node() API that can be used
> here?

Not when I wrote the code but since v4.20-rc1 we have
for_each_of_cpu_node(). So I am going to use it now.

>
>> +               int cpu, err;
>> +
>> +               err = of_property_read_u32(dn, "reg", &cpu);
>> +               if (WARN_ON(err))
>> +                       return err;
>> +
>> +               /* If cpu2 or cpu3 is enabled */
>> +               if ((cpu & APN806_CLUSTER_NUM_MASK)) {
>> +                       nclusters = 2;
>> +                       break;
>> +               }
>> +       }
>> +       /*
>> +        * DFS for AP806 is controlled per cluster (2 CPUs per cluster),
>> +        * so allocate structs per cluster
>> +        */
>> +       ap_cpu_clk = devm_kcalloc(dev, nclusters, sizeof(*ap_cpu_clk),
>> +                                 GFP_KERNEL);
>> +       if (WARN_ON(!ap_cpu_clk))
>> +               return -ENOMEM;
>> +
>> +       cluster_clks = devm_kcalloc(dev, nclusters, sizeof(*cluster_clks),
>> +                                   GFP_KERNEL);
>> +       if (WARN_ON(!cluster_clks))
>
> Drop the WARN_ON please. If allocation fails we'll already get a large
> stack trace and messages.

OK

>
>> +               return -ENOMEM;
>> +
>> +       for_each_node_by_type(dn, "cpu") {
>> +               char *clk_name = "cpu-cluster-0";
>> +               struct clk_init_data init;
>> +               const char *parent_name;
>> +               struct clk *clk, *parent;
>> +               int cpu, err;
>> +
>> +               err = of_property_read_u32(dn, "reg", &cpu);
>> +               if (WARN_ON(err))
>> +                       return err;
>> +
>> +               cluster_index = (cpu & APN806_CLUSTER_NUM_MASK);
>
> Nitpick: Drop useless parenthesis please.

OK
>
>> +               cluster_index >>= APN806_CLUSTER_NUM_OFFSET;
>> +
>> +               /* Initialize once for one cluster */
>> +               if (cluster_clks[cluster_index])
>> +                       continue;
>> +
>> +               parent = of_clk_get(np, cluster_index);
>> +               if (IS_ERR(parent)) {
>> +                       dev_err(dev, "Could not get the clock parent\n");
>> +                       return -EINVAL;
>> +               }
>> +               parent_name =  __clk_get_name(parent);
>> +               clk_name[12] += cluster_index;
>> +               ap_cpu_clk[cluster_index].clk_name =
>> +                       ap_cp_unique_name(dev, np->parent, clk_name);
>> +               ap_cpu_clk[cluster_index].cluster = cluster_index;
>> +               ap_cpu_clk[cluster_index].pll_cr_base = regmap;
>> +               ap_cpu_clk[cluster_index].hw.init = &init;
>> +               ap_cpu_clk[cluster_index].dev = dev;
>> +
>> +               init.name = ap_cpu_clk[cluster_index].clk_name;
>> +               init.ops = &ap_cpu_clk_ops;
>> +               init.num_parents = 1;
>> +               init.parent_names = &parent_name;
>> +               init.flags = CLK_GET_RATE_NOCACHE;
>
> Can you add a comment why we need NOCACHE here? It isn't clear why this
> is important.

I think there was the idea that the clock rate could be modified by
something else that the clk driver, but now I am not sure it is the
case. I will check it and either add a comment or just remove it.

>
>> +
>> +               clk = devm_clk_register(dev, &ap_cpu_clk[cluster_index].hw);
>
> Please use the clk_hw based registration APIs.

OK

>
>> +               if (IS_ERR(clk))
>> +                       return PTR_ERR(clk);
>> +
>> +               cluster_clks[cluster_index] = clk;
>> +       }
>> +
>> +       clk_data.clk_num = cluster_index + 1;
>> +       clk_data.clks = cluster_clks;
>> +
>> +       ret = of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
>
> And the clk_hw based provider APIs.

OK
>
>> +       if (ret)
>> +               dev_err(dev, "failed to register OF clock provider\n");
>> +
>> +       return ret;
>> +}
>> +

Thanks,

Gregory

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH 3/6] clk: mvebu: add CPU clock driver for Armada 7K/8K

[Gregory CLEMENT]

Hi Stephen,
 
 On ven., nov. 16 2018, Gregory CLEMENT <gregory.clement@bootlin.com> wrote:

>>> +static int ap_cpu_clock_probe(struct platform_device *pdev)
>>> +{
>>> +       int ret, nclusters = 0, cluster_index = 0;
>>> +       struct device *dev = &pdev->dev;
>>> +       struct device_node *dn, *np = dev->of_node;
>>> +       struct ap_cpu_clk *ap_cpu_clk;
>>> +       struct regmap *regmap;
>>> +
>>> +       regmap = syscon_node_to_regmap(np->parent);
>>
>> Can we just call dev_get_remap() on pdev->dev.parent?
>
> we could do regmap = dev_get_regmap(pdev->dev.parent, NULL); instead of
> this line. But is it really better?

Actually we can't, because we really depend on a syscon and at this
moment there is no regmap. It is the syscon_node_to_regmap function which
creates this regmap when needed.

Gregory

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH 3/6] clk: mvebu: add CPU clock driver for Armada 7K/8K

[Stephen Boyd]

Quoting Gregory CLEMENT (2018-11-27 08:27:40)
> Hi Stephen,
>  
>  On ven., nov. 16 2018, Gregory CLEMENT <gregory.clement@bootlin.com> wrote:
> 
> >>> +static int ap_cpu_clock_probe(struct platform_device *pdev)
> >>> +{
> >>> +       int ret, nclusters = 0, cluster_index = 0;
> >>> +       struct device *dev = &pdev->dev;
> >>> +       struct device_node *dn, *np = dev->of_node;
> >>> +       struct ap_cpu_clk *ap_cpu_clk;
> >>> +       struct regmap *regmap;
> >>> +
> >>> +       regmap = syscon_node_to_regmap(np->parent);
> >>
> >> Can we just call dev_get_remap() on pdev->dev.parent?
> >
> > we could do regmap = dev_get_regmap(pdev->dev.parent, NULL); instead of
> > this line. But is it really better?
> 
> Actually we can't, because we really depend on a syscon and at this
> moment there is no regmap. It is the syscon_node_to_regmap function which
> creates this regmap when needed.
> 

Ah ok. Nevermind then.

Re: [PATCH 3/6] clk: mvebu: add CPU clock driver for Armada 7K/8K

[Stephen Boyd]

Quoting Gregory CLEMENT (2018-11-15 15:22:59)
>  On mer., oct. 17 2018, Stephen Boyd <sboyd@kernel.org> wrote:
> 
> > Quoting Gregory CLEMENT (2018-09-22 11:17:06)
> >> + * @cluster: Cluster clock controller index
> >> + * @clk_name: Cluster clock controller name
> >> + * @dev : Cluster clock device
> >> + * @hw: HW specific structure of Cluster clock controller
> >> + * @pll_cr_base: CA72MP2 Register base (Device Sample at Reset register)
> >> + */
> >> +struct ap_cpu_clk {
> >> +       int cluster;
> >
> > unsigned?
> 
> I don't expect to have so many clusters that a signed int would be too
> short :)
> 
> Actually I used int to let the compiler choose what ever he wants.
> 
> But if you really want I can use a unsigned int

It's not just about opening up a wider range of values. It's also about
being more explicit with the type by indicating a negative value is
impossible/doesn't make sense for the code. So yes please, change it to
unsigned int.

> 
> >
> >> +       const char *clk_name;
> >> +       struct device *dev;
> >> +       struct clk_hw hw;
> >> +       struct regmap *pll_cr_base;
> >> +};
> >> +
> >> +static struct clk **cluster_clks;
> >> +static struct clk_onecell_data clk_data;
> >
> > Can you give these some better names that are ap_cpu specific? Grepping
> > for clk_data will make lots of hits otherwise because they're so
> > generic.
> 
> OK I will try to be more creative ;)

Thanks.

> >> +                          BIT(AP806_PLL_CR_0_CPU_CLK_RELOAD_RATIO_OFFSET),
> >> +                          BIT(AP806_PLL_CR_0_CPU_CLK_RELOAD_RATIO_OFFSET));
> >> +
> >> +
> >> +       /* 4. Wait for stabilizing CPU Clock */
> >> +       do {
> >> +               regmap_read(clk->pll_cr_base,
> >> +                           AP806_CA72MP2_0_PLL_SR_REG_OFFSET,
> >> +                           &reg);
> >> +       } while (!(reg & BIT(clk->cluster * AP806_CA72MP2_0_PLL_RATIO_STATE)));
> >
> > Use regmap_read_poll_timeout() API here and give some large timeout
> > value so that we don't get stuck waiting here forever?
> 
> Indeed regmap_read_poll_timeout() seems a nice improvement
> 
> >
> >> +
> >> +       /* 5. Clear Reload ratio */
> >
> > I'm not sure we really need these comments. They're just saying what the
> > code is doing, so they don't add much value.
> 
> Actually, these 5 steps directly come from the datasheet.

Ok, still doesn't mean we need to copy the datasheet into code comments
though.

> 
> >
> >> +       regmap_update_bits(clk->pll_cr_base, cpu_ratio_reg,
> >> +                          BIT(AP806_PLL_CR_0_CPU_CLK_RELOAD_RATIO_OFFSET), 0);
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +
> >> +static long ap_cpu_clk_round_rate(struct clk_hw *hw, unsigned long rate,
> >> +                                unsigned long *parent_rate)
> >> +{
> >> +       int divider = *parent_rate / rate;
> >> +
> >> +       if (divider > APN806_MAX_DIVIDER)
> >> +               divider = APN806_MAX_DIVIDER;
> >
> >       divider = min(divider, APN806_MAX_DIVIDER);
> 
> OK
> >
> >> +
> >> +       return *parent_rate / divider;
> >> +}
> >> +
> >> +static const struct clk_ops ap_cpu_clk_ops = {
> >> +       .recalc_rate    = ap_cpu_clk_recalc_rate,
> >> +       .round_rate     = ap_cpu_clk_round_rate,
> >> +       .set_rate       = ap_cpu_clk_set_rate,
> >> +};
> >> +
> >> +static int ap_cpu_clock_probe(struct platform_device *pdev)
> >> +{
> >> +       int ret, nclusters = 0, cluster_index = 0;
> >> +       struct device *dev = &pdev->dev;
> >> +       struct device_node *dn, *np = dev->of_node;
> >> +       struct ap_cpu_clk *ap_cpu_clk;
> >> +       struct regmap *regmap;
> >> +
> >> +       regmap = syscon_node_to_regmap(np->parent);
> >
> > Can we just call dev_get_remap() on pdev->dev.parent?
> 
> we could do regmap = dev_get_regmap(pdev->dev.parent, NULL); instead of
> this line. But is it really better?

It allows us to ignore 'syscon' in this driver and drop the include of
that header file? 

> 
> >
> >> +       if (IS_ERR(regmap)) {
> >> +               pr_err("cannot get pll_cr_base regmap\n");
> >> +               return PTR_ERR(regmap);
> >> +       }
> >> +
> >> +       /*
> >> +        * AP806 has 4 cpus and DFS for AP806 is controlled per
> >> +        * cluster (2 CPUs per cluster), cpu0 and cpu1 are fixed to
> >> +        * cluster0 while cpu2 and cpu3 are fixed to cluster1 whether
> >> +        * they are enabled or not.  Since cpu0 is the boot cpu, then
> >> +        * cluster0 must exist.  If cpu2 or cpu3 is enabled, cluster1
> >> +        * will exist and the cluster number is 2; otherwise the
> >> +        * cluster number is 1.
> >> +        */
> >> +       nclusters = 1;
> >> +       for_each_node_by_type(dn, "cpu") {
> >
> > Isn't there some sort of for_each_cpu_node() API that can be used
> > here?
> 
> Not when I wrote the code but since v4.20-rc1 we have
> for_each_of_cpu_node(). So I am going to use it now.

Awesome!

> >
> >> +               cluster_index >>= APN806_CLUSTER_NUM_OFFSET;
> >> +
> >> +               /* Initialize once for one cluster */
> >> +               if (cluster_clks[cluster_index])
> >> +                       continue;
> >> +
> >> +               parent = of_clk_get(np, cluster_index);
> >> +               if (IS_ERR(parent)) {
> >> +                       dev_err(dev, "Could not get the clock parent\n");
> >> +                       return -EINVAL;
> >> +               }
> >> +               parent_name =  __clk_get_name(parent);
> >> +               clk_name[12] += cluster_index;
> >> +               ap_cpu_clk[cluster_index].clk_name =
> >> +                       ap_cp_unique_name(dev, np->parent, clk_name);
> >> +               ap_cpu_clk[cluster_index].cluster = cluster_index;
> >> +               ap_cpu_clk[cluster_index].pll_cr_base = regmap;
> >> +               ap_cpu_clk[cluster_index].hw.init = &init;
> >> +               ap_cpu_clk[cluster_index].dev = dev;
> >> +
> >> +               init.name = ap_cpu_clk[cluster_index].clk_name;
> >> +               init.ops = &ap_cpu_clk_ops;
> >> +               init.num_parents = 1;
> >> +               init.parent_names = &parent_name;
> >> +               init.flags = CLK_GET_RATE_NOCACHE;
> >
> > Can you add a comment why we need NOCACHE here? It isn't clear why this
> > is important.
> 
> I think there was the idea that the clock rate could be modified by
> something else that the clk driver, but now I am not sure it is the
> case. I will check it and either add a comment or just remove it.

Ok.

[PATCH 4/6] clk: mvebu: ap806: Fix clock name for the cluster

[Gregory CLEMENT]

Actually, the clocks exposed for the cluster are not the CPU clocks, but
the PLL clock used as entry clock for the CPU clocks. The CPU clock will
be managed by a driver submitting in the following patches.

Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
 drivers/clk/mvebu/ap806-system-controller.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/mvebu/ap806-system-controller.c b/drivers/clk/mvebu/ap806-system-controller.c
index 0a58824ff053..73ba8fd7860f 100644
--- a/drivers/clk/mvebu/ap806-system-controller.c
+++ b/drivers/clk/mvebu/ap806-system-controller.c
@@ -97,7 +97,7 @@ static int ap806_syscon_common_probe(struct platform_device *pdev,
 	cpuclk_freq *= 1000 * 1000;
 
 	/* CPU clocks depend on the Sample At Reset configuration */
-	name = ap_cp_unique_name(dev, syscon_node, "cpu-cluster-0");
+	name = ap_cp_unique_name(dev, syscon_node, "pll-cluster-0");
 	ap806_clks[0] = clk_register_fixed_rate(dev, name, NULL,
 						0, cpuclk_freq);
 	if (IS_ERR(ap806_clks[0])) {
@@ -105,7 +105,7 @@ static int ap806_syscon_common_probe(struct platform_device *pdev,
 		goto fail0;
 	}
 
-	name = ap_cp_unique_name(dev, syscon_node, "cpu-cluster-1");
+	name = ap_cp_unique_name(dev, syscon_node, "pll-cluster-1");
 	ap806_clks[1] = clk_register_fixed_rate(dev, name, NULL, 0,
 						cpuclk_freq);
 	if (IS_ERR(ap806_clks[1])) {
-- 
2.19.0

Re: [PATCH 4/6] clk: mvebu: ap806: Fix clock name for the cluster

[Stephen Boyd]

Quoting Gregory CLEMENT (2018-09-22 11:17:07)
> Actually, the clocks exposed for the cluster are not the CPU clocks, but
> the PLL clock used as entry clock for the CPU clocks. The CPU clock will
> be managed by a driver submitting in the following patches.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>

Does this need a fixes tag?

Re: [PATCH 4/6] clk: mvebu: ap806: Fix clock name for the cluster

[Gregory CLEMENT]

Hi Stephen,
 
 On mer., oct. 17 2018, Stephen Boyd <sboyd@kernel.org> wrote:

> Quoting Gregory CLEMENT (2018-09-22 11:17:07)
>> Actually, the clocks exposed for the cluster are not the CPU clocks, but
>> the PLL clock used as entry clock for the CPU clocks. The CPU clock will
>> be managed by a driver submitting in the following patches.
>> 
>> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
>
> Does this need a fixes tag?

It doesn't fix a regression so I don't thing it needs a fixes tag.

Gregory

>

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH 4/6] clk: mvebu: ap806: Fix clock name for the cluster

[Stephen Boyd]

Quoting Gregory CLEMENT (2018-11-23 06:42:24)
> Hi Stephen,
>  
>  On mer., oct. 17 2018, Stephen Boyd <sboyd@kernel.org> wrote:
> 
> > Quoting Gregory CLEMENT (2018-09-22 11:17:07)
> >> Actually, the clocks exposed for the cluster are not the CPU clocks, but
> >> the PLL clock used as entry clock for the CPU clocks. The CPU clock will
> >> be managed by a driver submitting in the following patches.
> >> 
> >> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> >
> > Does this need a fixes tag?
> 
> It doesn't fix a regression so I don't thing it needs a fixes tag.
> 

Would it help anyone who is backporting the stack of patches to support
this? I would liberally add a Fixes tag here in case it helps anyone
learn that this was incorrect all along.

[PATCH 5/6] arm64: marvell: enable the Armada 7K/8K CPU clk driver

[Gregory CLEMENT]

This commit makes sure the driver for the Armada 7K/8K CPU clock is
enabled.

Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
 arch/arm64/Kconfig.platforms | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index 393d2b524284..cf6888ecd9f4 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -119,6 +119,7 @@ config ARCH_MESON
 
 config ARCH_MVEBU
 	bool "Marvell EBU SoC Family"
+	select ARMADA_AP_CPU_CLK
 	select ARMADA_AP806_SYSCON
 	select ARMADA_CP110_SYSCON
 	select ARMADA_37XX_CLK
-- 
2.19.0

[PATCH 6/6] arm64: dts: marvell: Add cpu clock node on Armada 7K/8K

[Gregory CLEMENT]

Add cpu clock node on AP

Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
 arch/arm64/boot/dts/marvell/armada-ap806-quad.dtsi | 4 ++++
 arch/arm64/boot/dts/marvell/armada-ap806.dtsi      | 6 ++++++
 2 files changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-ap806-quad.dtsi b/arch/arm64/boot/dts/marvell/armada-ap806-quad.dtsi
index 64632c873888..f2fd777d1944 100644
--- a/arch/arm64/boot/dts/marvell/armada-ap806-quad.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-ap806-quad.dtsi
@@ -21,6 +21,7 @@
 			reg = <0x000>;
 			enable-method = "psci";
 			cpu-idle-states = <&CPU_SLEEP_0>;
+			clocks = <&cpu_clk 0>;
 		};
 		cpu1: cpu@1 {
 			device_type = "cpu";
@@ -28,6 +29,7 @@
 			reg = <0x001>;
 			enable-method = "psci";
 			cpu-idle-states = <&CPU_SLEEP_0>;
+			clocks = <&cpu_clk 0>;
 		};
 		cpu2: cpu@100 {
 			device_type = "cpu";
@@ -35,6 +37,7 @@
 			reg = <0x100>;
 			enable-method = "psci";
 			cpu-idle-states = <&CPU_SLEEP_0>;
+			clocks = <&cpu_clk 1>;
 		};
 		cpu3: cpu@101 {
 			device_type = "cpu";
@@ -42,6 +45,7 @@
 			reg = <0x101>;
 			enable-method = "psci";
 			cpu-idle-states = <&CPU_SLEEP_0>;
+			clocks = <&cpu_clk 1>;
 		};
 	};
 };
diff --git a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
index 4a65e4e830aa..27c840e91abe 100644
--- a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
@@ -280,6 +280,12 @@
 				#address-cells = <1>;
 				#size-cells = <1>;
 
+				cpu_clk: clock-cpu {
+					compatible = "marvell,ap806-cpu-clock";
+					clocks = <&ap_clk 0>, <&ap_clk 1>;
+					#clock-cells = <1>;
+				};
+
 				ap_thermal: thermal-sensor@80 {
 					compatible = "marvell,armada-ap806-thermal";
 					reg = <0x80 0x10>;
-- 
2.19.0

Re: [PATCH 6/6] arm64: dts: marvell: Add cpu clock node on Armada 7K/8K

[Stephen Boyd]

+Rob

Quoting Gregory CLEMENT (2018-09-22 11:17:09)
> diff --git a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
> index 4a65e4e830aa..27c840e91abe 100644
> --- a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
> @@ -280,6 +280,12 @@
>                                 #address-cells = <1>;
>                                 #size-cells = <1>;
>  
> +                               cpu_clk: clock-cpu {
> +                                       compatible = "marvell,ap806-cpu-clock";
> +                                       clocks = <&ap_clk 0>, <&ap_clk 1>;
> +                                       #clock-cells = <1>;
> +                               };

This looks like the wrong place because there isn't a reg property. It
should go to the root of the tree. And then it looks like we're adding
something to DT to get a driver to probe, which is improper DT design.

Re: [PATCH 6/6] arm64: dts: marvell: Add cpu clock node on Armada 7K/8K

[Gregory CLEMENT]

Hi Stephen,
 
 On ven., oct. 12 2018, Stephen Boyd <sboyd@kernel.org> wrote:

> +Rob
>
> Quoting Gregory CLEMENT (2018-09-22 11:17:09)
>> diff --git a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
>> index 4a65e4e830aa..27c840e91abe 100644
>> --- a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
>> +++ b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
>> @@ -280,6 +280,12 @@
>>                                 #address-cells = <1>;
>>                                 #size-cells = <1>;
>>  
>> +                               cpu_clk: clock-cpu {
>> +                                       compatible = "marvell,ap806-cpu-clock";
>> +                                       clocks = <&ap_clk 0>, <&ap_clk 1>;
>> +                                       #clock-cells = <1>;
>> +                               };
>
> This looks like the wrong place because there isn't a reg property. It

There is no reg property because we are inside a syscon node where the
registers are shared between multiple IPs.

> should go to the root of the tree. And then it looks like we're adding
> something to DT to get a driver to probe, which is improper DT design.

There is nothing related to the driver, this subnode describes the way
the hardware is designed. Under the system controller node there are
several IPs , like the CPU clocks, but also the GPIO or the pinctrl.

Gregory

>

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH 6/6] arm64: dts: marvell: Add cpu clock node on Armada 7K/8K

[Stephen Boyd]

Quoting Gregory CLEMENT (2018-11-23 07:02:56)
> Hi Stephen,
>  
>  On ven., oct. 12 2018, Stephen Boyd <sboyd@kernel.org> wrote:
> 
> > +Rob
> >
> > Quoting Gregory CLEMENT (2018-09-22 11:17:09)
> >> diff --git a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
> >> index 4a65e4e830aa..27c840e91abe 100644
> >> --- a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
> >> +++ b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
> >> @@ -280,6 +280,12 @@
> >>                                 #address-cells = <1>;
> >>                                 #size-cells = <1>;
> >>  
> >> +                               cpu_clk: clock-cpu {
> >> +                                       compatible = "marvell,ap806-cpu-clock";
> >> +                                       clocks = <&ap_clk 0>, <&ap_clk 1>;
> >> +                                       #clock-cells = <1>;
> >> +                               };
> >
> > This looks like the wrong place because there isn't a reg property. It
> 
> There is no reg property because we are inside a syscon node where the
> registers are shared between multiple IPs.

The node right after this node has a reg property. What's going on?

> 
> > should go to the root of the tree. And then it looks like we're adding
> > something to DT to get a driver to probe, which is improper DT design.
> 
> There is nothing related to the driver, this subnode describes the way
> the hardware is designed. Under the system controller node there are
> several IPs , like the CPU clocks, but also the GPIO or the pinctrl.
> 

Ok. And some of them have reg properties and others don't? We should
somehow deprecate the idea of a syscon with child nodes. It leads to
this weird twisting of DT. The only use for syscon from my perspective
is when we have a register region that other random devices with their
own 'reg' property need to peek/poke random things into.