[PATCHv3 0/4] clk: socfpga: Add clock driver for Arria10

[PATCHv3 0/4] clk: socfpga: Add clock driver for Arria10

[dinguyen]

From: Dinh Nguyen <dinguyen@opensource.altera.com>

Hi,

This patch series add the clock driver for the Arria10 platform. Although the
Arria10 SoC's clock framework has some similarities the Cyclone/Arria 5, the
differences are enough to warrant it's own driver, rather than polluting the
existing driver with platform lookups.

v3:
- Fix sparse warning of assigning an integer 0 instead of NULL to a pointer.

v2:
- Update the DTS bindings doucment to have the new Arria10 clocks.
- Add an l4_sys_free_clk node. The l4_sys_free_clk is similar to the
  l4_sp_clk, but cannot be gated.

*** BLURB HERE ***

Dinh Nguyen (4):
  clk: socfpga: update clk.h so for Arria10 platform to use
  clk: socfpga: add a clock driver for the Arria 10 platform
  ARM: socfpga: dts: add clocks to the Arria10 platform
  Documentation: DT bindings: document the clocks for Arria10

 .../devicetree/bindings/clock/altr_socfpga.txt     |  17 +-
 arch/arm/boot/dts/socfpga_arria10.dtsi             | 309 ++++++++++++++++++++-
 drivers/clk/socfpga/Makefile                       |   1 +
 drivers/clk/socfpga/clk-gate-a10.c                 | 187 +++++++++++++
 drivers/clk/socfpga/clk-gate.c                     |   4 -
 drivers/clk/socfpga/clk-periph-a10.c               | 131 +++++++++
 drivers/clk/socfpga/clk-pll-a10.c                  | 132 +++++++++
 drivers/clk/socfpga/clk.c                          |   7 +-
 drivers/clk/socfpga/clk.h                          |  10 +-
 9 files changed, 782 insertions(+), 16 deletions(-)
 create mode 100644 drivers/clk/socfpga/clk-gate-a10.c
 create mode 100644 drivers/clk/socfpga/clk-periph-a10.c
 create mode 100644 drivers/clk/socfpga/clk-pll-a10.c

-- 
2.2.1

[PATCHv3 1/4] clk: socfpga: update clk.h so for Arria10 platform to use

[dinguyen]

From: Dinh Nguyen <dinguyen@opensource.altera.com>

There are 5 possible parent clocks for the SoCFPGA Arria10. Move the define
SYSMGR_SDMMC_CTRL_SET and streq() to clk.h so that the Arria clock driver
can use.

Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
---
 drivers/clk/socfpga/clk-gate.c | 4 ----
 drivers/clk/socfpga/clk.h      | 6 +++++-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/socfpga/clk-gate.c b/drivers/clk/socfpga/clk-gate.c
index dd3a78c..607ab35 100644
--- a/drivers/clk/socfpga/clk-gate.c
+++ b/drivers/clk/socfpga/clk-gate.c
@@ -32,14 +32,10 @@
 #define SOCFPGA_MMC_CLK			"sdmmc_clk"
 #define SOCFPGA_GPIO_DB_CLK_OFFSET	0xA8
 
-#define streq(a, b) (strcmp((a), (b)) == 0)
-
 #define to_socfpga_gate_clk(p) container_of(p, struct socfpga_gate_clk, hw.hw)
 
 /* SDMMC Group for System Manager defines */
 #define SYSMGR_SDMMCGRP_CTRL_OFFSET    0x108
-#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel) \
-	((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0))
 
 static u8 socfpga_clk_get_parent(struct clk_hw *hwclk)
 {
diff --git a/drivers/clk/socfpga/clk.h b/drivers/clk/socfpga/clk.h
index d291f60..b09a5d5 100644
--- a/drivers/clk/socfpga/clk.h
+++ b/drivers/clk/socfpga/clk.h
@@ -26,9 +26,13 @@
 #define CLKMGR_L4SRC		0x70
 #define CLKMGR_PERPLL_SRC	0xAC
 
-#define SOCFPGA_MAX_PARENTS		3
+#define SOCFPGA_MAX_PARENTS		5
 #define div_mask(width) ((1 << (width)) - 1)
 
+#define streq(a, b) (strcmp((a), (b)) == 0)
+#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel) \
+	((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0))
+
 extern void __iomem *clk_mgr_base_addr;
 
 void __init socfpga_pll_init(struct device_node *node);
-- 
2.2.1

[PATCHv3 2/4] clk: socfpga: add a clock driver for the Arria 10 platform

[dinguyen]

From: Dinh Nguyen <dinguyen@opensource.altera.com>

The clocks on the Arria 10 platform is a bit different than the Cyclone/Arria 5
platform that it should just have it's own driver.

Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
---
v3: Assign pointer to NULL instead of integer 0 per sparse check
---
 drivers/clk/socfpga/Makefile         |   1 +
 drivers/clk/socfpga/clk-gate-a10.c   | 187 +++++++++++++++++++++++++++++++++++
 drivers/clk/socfpga/clk-periph-a10.c | 131 ++++++++++++++++++++++++
 drivers/clk/socfpga/clk-pll-a10.c    | 132 +++++++++++++++++++++++++
 drivers/clk/socfpga/clk.c            |   7 +-
 drivers/clk/socfpga/clk.h            |   4 +
 6 files changed, 461 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/socfpga/clk-gate-a10.c
 create mode 100644 drivers/clk/socfpga/clk-periph-a10.c
 create mode 100644 drivers/clk/socfpga/clk-pll-a10.c

diff --git a/drivers/clk/socfpga/Makefile b/drivers/clk/socfpga/Makefile
index 7e2d15a..d8bb239 100644
--- a/drivers/clk/socfpga/Makefile
+++ b/drivers/clk/socfpga/Makefile
@@ -2,3 +2,4 @@ obj-y += clk.o
 obj-y += clk-gate.o
 obj-y += clk-pll.o
 obj-y += clk-periph.o
+obj-y += clk-pll-a10.o clk-periph-a10.o clk-gate-a10.o
diff --git a/drivers/clk/socfpga/clk-gate-a10.c b/drivers/clk/socfpga/clk-gate-a10.c
new file mode 100644
index 0000000..fadf6f7
--- /dev/null
+++ b/drivers/clk/socfpga/clk-gate-a10.c
@@ -0,0 +1,187 @@
+/*
+ * Copyright (C) 2015 Altera Corporation. All rights reserved
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/clk.h>
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
+#include <linux/io.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+
+#include "clk.h"
+
+#define streq(a, b) (strcmp((a), (b)) == 0)
+
+#define to_socfpga_gate_clk(p) container_of(p, struct socfpga_gate_clk, hw.hw)
+
+/* SDMMC Group for System Manager defines */
+#define SYSMGR_SDMMCGRP_CTRL_OFFSET	0x28
+
+static unsigned long socfpga_gate_clk_recalc_rate(struct clk_hw *hwclk,
+	unsigned long parent_rate)
+{
+	struct socfpga_gate_clk *socfpgaclk = to_socfpga_gate_clk(hwclk);
+	u32 div = 1, val;
+
+	if (socfpgaclk->fixed_div)
+		div = socfpgaclk->fixed_div;
+	else if (socfpgaclk->div_reg) {
+		val = readl(socfpgaclk->div_reg) >> socfpgaclk->shift;
+		val &= div_mask(socfpgaclk->width);
+			div = (1 << val);
+	}
+
+	return parent_rate / div;
+}
+
+static int socfpga_clk_prepare(struct clk_hw *hwclk)
+{
+	struct socfpga_gate_clk *socfpgaclk = to_socfpga_gate_clk(hwclk);
+	struct regmap *sys_mgr_base_addr;
+	int i;
+	u32 hs_timing;
+	u32 clk_phase[2];
+
+	if (socfpgaclk->clk_phase[0] || socfpgaclk->clk_phase[1]) {
+		sys_mgr_base_addr = syscon_regmap_lookup_by_compatible("altr,sys-mgr");
+		if (IS_ERR(sys_mgr_base_addr)) {
+			pr_err("%s: failed to find altr,sys-mgr regmap!\n", __func__);
+			return -EINVAL;
+		}
+
+		for (i = 0; i < 2; i++) {
+			switch (socfpgaclk->clk_phase[i]) {
+			case 0:
+				clk_phase[i] = 0;
+				break;
+			case 45:
+				clk_phase[i] = 1;
+				break;
+			case 90:
+				clk_phase[i] = 2;
+				break;
+			case 135:
+				clk_phase[i] = 3;
+				break;
+			case 180:
+				clk_phase[i] = 4;
+				break;
+			case 225:
+				clk_phase[i] = 5;
+				break;
+			case 270:
+				clk_phase[i] = 6;
+				break;
+			case 315:
+				clk_phase[i] = 7;
+				break;
+			default:
+				clk_phase[i] = 0;
+				break;
+			}
+		}
+
+		hs_timing = SYSMGR_SDMMC_CTRL_SET(clk_phase[0], clk_phase[1]);
+		regmap_write(sys_mgr_base_addr, SYSMGR_SDMMCGRP_CTRL_OFFSET,
+			     hs_timing);
+	}
+	return 0;
+}
+
+static struct clk_ops gateclk_ops = {
+	.prepare = socfpga_clk_prepare,
+	.recalc_rate = socfpga_gate_clk_recalc_rate,
+};
+
+static void __init __socfpga_gate_init(struct device_node *node,
+	const struct clk_ops *ops)
+{
+	u32 clk_gate[2];
+	u32 div_reg[3];
+	u32 clk_phase[2];
+	u32 fixed_div;
+	struct clk *clk;
+	struct socfpga_gate_clk *socfpga_clk;
+	const char *clk_name = node->name;
+	const char *parent_name[SOCFPGA_MAX_PARENTS];
+	struct clk_init_data init;
+	int rc;
+	int i = 0;
+
+	socfpga_clk = kzalloc(sizeof(*socfpga_clk), GFP_KERNEL);
+	if (WARN_ON(!socfpga_clk))
+		return;
+
+	rc = of_property_read_u32_array(node, "clk-gate", clk_gate, 2);
+	if (rc)
+		clk_gate[0] = 0;
+
+	if (clk_gate[0]) {
+		socfpga_clk->hw.reg = clk_mgr_a10_base_addr + clk_gate[0];
+		socfpga_clk->hw.bit_idx = clk_gate[1];
+
+		gateclk_ops.enable = clk_gate_ops.enable;
+		gateclk_ops.disable = clk_gate_ops.disable;
+	}
+
+	rc = of_property_read_u32(node, "fixed-divider", &fixed_div);
+	if (rc)
+		socfpga_clk->fixed_div = 0;
+	else
+		socfpga_clk->fixed_div = fixed_div;
+
+	rc = of_property_read_u32_array(node, "div-reg", div_reg, 3);
+	if (!rc) {
+		socfpga_clk->div_reg = clk_mgr_a10_base_addr + div_reg[0];
+		socfpga_clk->shift = div_reg[1];
+		socfpga_clk->width = div_reg[2];
+	} else {
+		socfpga_clk->div_reg = NULL;
+	}
+
+	rc = of_property_read_u32_array(node, "clk-phase", clk_phase, 2);
+	if (!rc) {
+		socfpga_clk->clk_phase[0] = clk_phase[0];
+		socfpga_clk->clk_phase[1] = clk_phase[1];
+	}
+
+	of_property_read_string(node, "clock-output-names", &clk_name);
+
+	init.name = clk_name;
+	init.ops = ops;
+	init.flags = 0;
+	while (i < SOCFPGA_MAX_PARENTS && (parent_name[i] =
+			of_clk_get_parent_name(node, i)) != NULL)
+		i++;
+
+	init.parent_names = parent_name;
+	init.num_parents = i;
+	socfpga_clk->hw.hw.init = &init;
+
+	clk = clk_register(NULL, &socfpga_clk->hw.hw);
+	if (WARN_ON(IS_ERR(clk))) {
+		kfree(socfpga_clk);
+		return;
+	}
+	rc = of_clk_add_provider(node, of_clk_src_simple_get, clk);
+	if (WARN_ON(rc))
+		return;
+}
+
+void __init socfpga_a10_gate_init(struct device_node *node)
+{
+	__socfpga_gate_init(node, &gateclk_ops);
+}
diff --git a/drivers/clk/socfpga/clk-periph-a10.c b/drivers/clk/socfpga/clk-periph-a10.c
new file mode 100644
index 0000000..81b9274
--- /dev/null
+++ b/drivers/clk/socfpga/clk-periph-a10.c
@@ -0,0 +1,131 @@
+/*
+ * Copyright (C) 2015 Altera Corporation. All rights reserved
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/clk.h>
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
+#include <linux/io.h>
+#include <linux/of.h>
+
+#include "clk.h"
+
+#define CLK_MGR_FREE_SHIFT		16
+#define CLK_MGR_FREE_MASK		0x7
+
+#define SOCFPGA_MPU_FREE_CLK		"mpu_free_clk"
+#define SOCFPGA_NOC_FREE_CLK		"noc_free_clk"
+#define SOCFPGA_SDMMC_FREE_CLK		"sdmmc_free_clk"
+#define to_socfpga_periph_clk(p) container_of(p, struct socfpga_periph_clk, hw.hw)
+
+static unsigned long clk_periclk_recalc_rate(struct clk_hw *hwclk,
+					     unsigned long parent_rate)
+{
+	struct socfpga_periph_clk *socfpgaclk = to_socfpga_periph_clk(hwclk);
+	u32 div;
+
+	if (socfpgaclk->fixed_div) {
+		div = socfpgaclk->fixed_div;
+	} else if (socfpgaclk->div_reg) {
+		div = readl(socfpgaclk->div_reg) >> socfpgaclk->shift;
+		div &= div_mask(socfpgaclk->width);
+		div += 1;
+	} else {
+		div = ((readl(socfpgaclk->hw.reg) & 0x7ff) + 1);
+	}
+
+	return parent_rate / div;
+}
+
+static u8 clk_periclk_get_parent(struct clk_hw *hwclk)
+{
+	struct socfpga_periph_clk *socfpgaclk = to_socfpga_periph_clk(hwclk);
+	u32 clk_src;
+
+	clk_src = readl(socfpgaclk->hw.reg);
+	if (streq(hwclk->init->name, SOCFPGA_MPU_FREE_CLK) ||
+	    streq(hwclk->init->name, SOCFPGA_NOC_FREE_CLK) ||
+	    streq(hwclk->init->name, SOCFPGA_SDMMC_FREE_CLK))
+		return (clk_src >> CLK_MGR_FREE_SHIFT) &
+			CLK_MGR_FREE_MASK;
+	else
+		return 0;
+}
+
+static const struct clk_ops periclk_ops = {
+	.recalc_rate = clk_periclk_recalc_rate,
+	.get_parent = clk_periclk_get_parent,
+};
+
+static __init void __socfpga_periph_init(struct device_node *node,
+	const struct clk_ops *ops)
+{
+	u32 reg;
+	struct clk *clk;
+	struct socfpga_periph_clk *periph_clk;
+	const char *clk_name = node->name;
+	const char *parent_name;
+	struct clk_init_data init;
+	int rc;
+	u32 fixed_div;
+	u32 div_reg[3];
+
+	of_property_read_u32(node, "reg", &reg);
+
+	periph_clk = kzalloc(sizeof(*periph_clk), GFP_KERNEL);
+	if (WARN_ON(!periph_clk))
+		return;
+
+	periph_clk->hw.reg = clk_mgr_a10_base_addr + reg;
+
+	rc = of_property_read_u32_array(node, "div-reg", div_reg, 3);
+	if (!rc) {
+		periph_clk->div_reg = clk_mgr_a10_base_addr + div_reg[0];
+		periph_clk->shift = div_reg[1];
+		periph_clk->width = div_reg[2];
+	} else {
+		periph_clk->div_reg = NULL;
+	}
+
+	rc = of_property_read_u32(node, "fixed-divider", &fixed_div);
+	if (rc)
+		periph_clk->fixed_div = 0;
+	else
+		periph_clk->fixed_div = fixed_div;
+
+	of_property_read_string(node, "clock-output-names", &clk_name);
+
+	init.name = clk_name;
+	init.ops = ops;
+	init.flags = 0;
+
+	parent_name = of_clk_get_parent_name(node, 0);
+	init.num_parents = 1;
+	init.parent_names = &parent_name;
+
+	periph_clk->hw.hw.init = &init;
+
+	clk = clk_register(NULL, &periph_clk->hw.hw);
+	if (WARN_ON(IS_ERR(clk))) {
+		kfree(periph_clk);
+		return;
+	}
+	rc = of_clk_add_provider(node, of_clk_src_simple_get, clk);
+
+}
+
+void __init socfpga_a10_periph_init(struct device_node *node)
+{
+	__socfpga_periph_init(node, &periclk_ops);
+}
diff --git a/drivers/clk/socfpga/clk-pll-a10.c b/drivers/clk/socfpga/clk-pll-a10.c
new file mode 100644
index 0000000..2adc2f5
--- /dev/null
+++ b/drivers/clk/socfpga/clk-pll-a10.c
@@ -0,0 +1,132 @@
+/*
+ * Copyright (C) 2015 Altera Corporation. All rights reserved
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/clk.h>
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+
+#include "clk.h"
+
+/* Clock Manager offsets */
+#define CLK_MGR_PLL_CLK_SRC_SHIFT	8
+#define CLK_MGR_PLL_CLK_SRC_MASK	0x3
+
+/* Clock bypass bits */
+#define SOCFPGA_PLL_BG_PWRDWN		0
+#define SOCFPGA_PLL_PWR_DOWN		1
+#define SOCFPGA_PLL_EXT_ENA		2
+#define SOCFPGA_PLL_DIVF_MASK		0x00001FFF
+#define SOCFPGA_PLL_DIVF_SHIFT	0
+#define SOCFPGA_PLL_DIVQ_MASK		0x003F0000
+#define SOCFPGA_PLL_DIVQ_SHIFT	16
+#define SOCFGPA_MAX_PARENTS	5
+
+#define SOCFPGA_MAIN_PLL_CLK		"main_pll"
+#define SOCFPGA_PERIP_PLL_CLK		"periph_pll"
+
+#define to_socfpga_clk(p) container_of(p, struct socfpga_pll, hw.hw)
+
+void __iomem *clk_mgr_a10_base_addr;
+
+static unsigned long clk_pll_recalc_rate(struct clk_hw *hwclk,
+					 unsigned long parent_rate)
+{
+	struct socfpga_pll *socfpgaclk = to_socfpga_clk(hwclk);
+	unsigned long divf, divq, reg;
+	unsigned long long vco_freq;
+
+	/* read VCO1 reg for numerator and denominator */
+	reg = readl(socfpgaclk->hw.reg + 0x4);
+	divf = (reg & SOCFPGA_PLL_DIVF_MASK) >> SOCFPGA_PLL_DIVF_SHIFT;
+	divq = (reg & SOCFPGA_PLL_DIVQ_MASK) >> SOCFPGA_PLL_DIVQ_SHIFT;
+	vco_freq = (unsigned long long)parent_rate * (divf + 1);
+	do_div(vco_freq, (1 + divq));
+	return (unsigned long)vco_freq;
+}
+
+static u8 clk_pll_get_parent(struct clk_hw *hwclk)
+{
+	struct socfpga_pll *socfpgaclk = to_socfpga_clk(hwclk);
+	u32 pll_src;
+
+	pll_src = readl(socfpgaclk->hw.reg);
+
+	return (pll_src >> CLK_MGR_PLL_CLK_SRC_SHIFT) &
+		CLK_MGR_PLL_CLK_SRC_MASK;
+}
+
+
+static struct clk_ops clk_pll_ops = {
+	.recalc_rate = clk_pll_recalc_rate,
+	.get_parent = clk_pll_get_parent,
+};
+
+static __init struct clk *__socfpga_pll_init(struct device_node *node,
+	const struct clk_ops *ops)
+{
+	u32 reg;
+	struct clk *clk;
+	struct socfpga_pll *pll_clk;
+	const char *clk_name = node->name;
+	const char *parent_name[SOCFGPA_MAX_PARENTS];
+	struct clk_init_data init;
+	struct device_node *clkmgr_np;
+	int rc;
+	int i = 0;
+
+	of_property_read_u32(node, "reg", &reg);
+
+	pll_clk = kzalloc(sizeof(*pll_clk), GFP_KERNEL);
+	if (WARN_ON(!pll_clk))
+		return NULL;
+
+	clkmgr_np = of_find_compatible_node(NULL, NULL, "altr,clk-mgr");
+	clk_mgr_a10_base_addr = of_iomap(clkmgr_np, 0);
+	BUG_ON(!clk_mgr_a10_base_addr);
+	pll_clk->hw.reg = clk_mgr_a10_base_addr + reg;
+
+	of_property_read_string(node, "clock-output-names", &clk_name);
+
+	init.name = clk_name;
+	init.ops = ops;
+	init.flags = 0;
+
+	while (i < SOCFGPA_MAX_PARENTS && (parent_name[i] =
+			of_clk_get_parent_name(node, i)) != NULL)
+		i++;
+	init.num_parents = i;
+	init.parent_names = parent_name;
+	pll_clk->hw.hw.init = &init;
+
+	pll_clk->hw.bit_idx = SOCFPGA_PLL_EXT_ENA;
+	clk_pll_ops.enable = clk_gate_ops.enable;
+	clk_pll_ops.disable = clk_gate_ops.disable;
+
+	clk = clk_register(NULL, &pll_clk->hw.hw);
+	if (WARN_ON(IS_ERR(clk))) {
+		kfree(pll_clk);
+		return NULL;
+	}
+	rc = of_clk_add_provider(node, of_clk_src_simple_get, clk);
+	return clk;
+}
+
+void __init socfpga_a10_pll_init(struct device_node *node)
+{
+	__socfpga_pll_init(node, &clk_pll_ops);
+}
diff --git a/drivers/clk/socfpga/clk.c b/drivers/clk/socfpga/clk.c
index 43db947..7564d2e 100644
--- a/drivers/clk/socfpga/clk.c
+++ b/drivers/clk/socfpga/clk.c
@@ -24,4 +24,9 @@
 CLK_OF_DECLARE(socfpga_pll_clk, "altr,socfpga-pll-clock", socfpga_pll_init);
 CLK_OF_DECLARE(socfpga_perip_clk, "altr,socfpga-perip-clk", socfpga_periph_init);
 CLK_OF_DECLARE(socfpga_gate_clk, "altr,socfpga-gate-clk", socfpga_gate_init);
-
+CLK_OF_DECLARE(socfpga_a10_pll_clk, "altr,socfpga-a10-pll-clock",
+	       socfpga_a10_pll_init);
+CLK_OF_DECLARE(socfpga_a10_perip_clk, "altr,socfpga-a10-perip-clk",
+	       socfpga_a10_periph_init);
+CLK_OF_DECLARE(socfpga_a10_gate_clk, "altr,socfpga-a10-gate-clk",
+	       socfpga_a10_gate_init);
diff --git a/drivers/clk/socfpga/clk.h b/drivers/clk/socfpga/clk.h
index b09a5d5..6989847 100644
--- a/drivers/clk/socfpga/clk.h
+++ b/drivers/clk/socfpga/clk.h
@@ -34,10 +34,14 @@
 	((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0))
 
 extern void __iomem *clk_mgr_base_addr;
+extern void __iomem *clk_mgr_a10_base_addr;
 
 void __init socfpga_pll_init(struct device_node *node);
 void __init socfpga_periph_init(struct device_node *node);
 void __init socfpga_gate_init(struct device_node *node);
+void __init socfpga_a10_pll_init(struct device_node *node);
+void __init socfpga_a10_periph_init(struct device_node *node);
+void __init socfpga_a10_gate_init(struct device_node *node);
 
 struct socfpga_pll {
 	struct clk_gate	hw;
-- 
2.2.1

[PATCHv3 3/4] ARM: socfpga: dts: add clocks to the Arria10 platform

[dinguyen]

From: Dinh Nguyen <dinguyen@opensource.altera.com>

Add all the clock nodes for the Arria10 platform. At the same time, update
the peripherals with their respective clocks property.

Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
---
v2: Add the l4_sys_free_clk node
---
 arch/arm/boot/dts/socfpga_arria10.dtsi | 309 ++++++++++++++++++++++++++++++++-
 1 file changed, 305 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/socfpga_arria10.dtsi b/arch/arm/boot/dts/socfpga_arria10.dtsi
index 6c3ad92..ef2c946 100644
--- a/arch/arm/boot/dts/socfpga_arria10.dtsi
+++ b/arch/arm/boot/dts/socfpga_arria10.dtsi
@@ -86,6 +86,21 @@
 					#address-cells = <1>;
 					#size-cells = <0>;
 
+					cb_intosc_hs_div2_clk: cb_intosc_hs_div2_clk {
+						#clock-cells = <0>;
+						compatible = "fixed-clock";
+					};
+
+					cb_intosc_ls_clk: cb_intosc_ls_clk {
+						#clock-cells = <0>;
+						compatible = "fixed-clock";
+					};
+
+					f2s_free_clk: f2s_free_clk {
+						#clock-cells = <0>;
+						compatible = "fixed-clock";
+					};
+
 					osc1: osc1 {
 						#clock-cells = <0>;
 						compatible = "fixed-clock";
@@ -95,16 +110,286 @@
 						#address-cells = <1>;
 						#size-cells = <0>;
 						#clock-cells = <0>;
-						compatible = "altr,socfpga-pll-clock";
-						clocks = <&osc1>;
+						compatible = "altr,socfpga-a10-pll-clock";
+						clocks = <&osc1>, <&cb_intosc_ls_clk>,
+							 <&f2s_free_clk>;
+						reg = <0x40>;
+
+						main_mpu_base_clk: main_mpu_base_clk {
+							#clock-cells = <0>;
+							compatible = "altr,socfpga-a10-perip-clk";
+							clocks = <&main_pll>;
+							div-reg = <0x140 0 11>;
+						};
+
+						main_noc_base_clk: main_noc_base_clk {
+							#clock-cells = <0>;
+							compatible = "altr,socfpga-a10-perip-clk";
+							clocks = <&main_pll>;
+							div-reg = <0x144 0 11>;
+						};
+
+						main_emaca_clk: main_emaca_clk {
+							#clock-cells = <0>;
+							compatible = "altr,socfpga-a10-perip-clk";
+							clocks = <&main_pll>;
+							reg = <0x68>;
+						};
+
+						main_emacb_clk: main_emacb_clk {
+							#clock-cells = <0>;
+							compatible = "altr,socfpga-a10-perip-clk";
+							clocks = <&main_pll>;
+							reg = <0x6C>;
+						};
+
+						main_emac_ptp_clk: main_emac_ptp_clk {
+							#clock-cells = <0>;
+							compatible = "altr,socfpga-a10-perip-clk";
+							clocks = <&main_pll>;
+							reg = <0x70>;
+						};
+
+						main_gpio_db_clk: main_gpio_db_clk {
+							#clock-cells = <0>;
+							compatible = "altr,socfpga-a10-perip-clk";
+							clocks = <&main_pll>;
+							reg = <0x74>;
+						};
+
+						main_sdmmc_clk: main_sdmmc_clk {
+							#clock-cells = <0>;
+							compatible = "altr,socfpga-a10-perip-clk"
+;
+							clocks = <&main_pll>;
+							reg = <0x78>;
+						};
+
+						main_s2f_usr0_clk: main_s2f_usr0_clk {
+							#clock-cells = <0>;
+							compatible = "altr,socfpga-a10-perip-clk";
+							clocks = <&main_pll>;
+							reg = <0x7C>;
+						};
+
+						main_s2f_usr1_clk: main_s2f_usr1_clk {
+							#clock-cells = <0>;
+							compatible = "altr,socfpga-a10-perip-clk";
+							clocks = <&main_pll>;
+							reg = <0x80>;
+						};
+
+						main_hmc_pll_ref_clk: main_hmc_pll_ref_clk {
+							#clock-cells = <0>;
+							compatible = "altr,socfpga-a10-perip-clk";
+							clocks = <&main_pll>;
+							reg = <0x84>;
+						};
+
+						main_periph_ref_clk: main_periph_ref_clk {
+							#clock-cells = <0>;
+							compatible = "altr,socfpga-a10-perip-clk";
+							clocks = <&main_pll>;
+							reg = <0x9C>;
+						};
 					};
 
 					periph_pll: periph_pll {
 						#address-cells = <1>;
 						#size-cells = <0>;
 						#clock-cells = <0>;
-						compatible = "altr,socfpga-pll-clock";
-						clocks = <&osc1>;
+						compatible = "altr,socfpga-a10-pll-clock";
+						clocks = <&osc1>, <&cb_intosc_ls_clk>,
+							 <&f2s_free_clk>, <&main_periph_ref_clk>;
+						reg = <0xC0>;
+
+						peri_mpu_base_clk: peri_mpu_base_clk {
+							#clock-cells = <0>;
+							compatible = "altr,socfpga-a10-perip-clk";
+							clocks = <&periph_pll>;
+							div-reg = <0x140 16 11>;
+						};
+
+						peri_noc_base_clk: peri_noc_base_clk {
+							#clock-cells = <0>;
+							compatible = "altr,socfpga-a10-perip-clk";
+							clocks = <&periph_pll>;
+							div-reg = <0x144 16 11>;
+						};
+
+						peri_emaca_clk: peri_emaca_clk {
+							#clock-cells = <0>;
+							compatible = "altr,socfpga-a10-perip-clk";
+							clocks = <&periph_pll>;
+							reg = <0xE8>;
+						};
+
+						peri_emacb_clk: peri_emacb_clk {
+							#clock-cells = <0>;
+							compatible = "altr,socfpga-a10-perip-clk";
+							clocks = <&periph_pll>;
+							reg = <0xEC>;
+						};
+
+						peri_emac_ptp_clk: peri_emac_ptp_clk {
+							#clock-cells = <0>;
+							compatible = "altr,socfpga-a10-perip-clk";
+							clocks = <&periph_pll>;
+							reg = <0xF0>;
+						};
+
+						peri_gpio_db_clk: peri_gpio_db_clk {
+							#clock-cells = <0>;
+							compatible = "altr,socfpga-a10-perip-clk";
+							clocks = <&periph_pll>;
+							reg = <0xF4>;
+						};
+
+						peri_sdmmc_clk: peri_sdmmc_clk {
+							#clock-cells = <0>;
+							compatible = "altr,socfpga-a10-perip-clk";
+							clocks = <&periph_pll>;
+							reg = <0xF8>;
+						};
+
+						peri_s2f_usr0_clk: peri_s2f_usr0_clk {
+							#clock-cells = <0>;
+							compatible = "altr,socfpga-a10-perip-clk";
+							clocks = <&periph_pll>;
+							reg = <0xFC>;
+						};
+
+						peri_s2f_usr1_clk: peri_s2f_usr1_clk {
+							#clock-cells = <0>;
+							compatible = "altr,socfpga-a10-perip-clk";
+							clocks = <&periph_pll>;
+							reg = <0x100>;
+						};
+
+						peri_hmc_pll_ref_clk: peri_hmc_pll_ref_clk {
+							#clock-cells = <0>;
+							compatible = "altr,socfpga-a10-perip-clk";
+							clocks = <&periph_pll>;
+							reg = <0x104>;
+						};
+					};
+
+					mpu_free_clk: mpu_free_clk {
+						#clock-cells = <0>;
+						compatible = "altr,socfpga-a10-perip-clk";
+						clocks = <&main_mpu_base_clk>, <&peri_mpu_base_clk>,
+							 <&osc1>, <&cb_intosc_hs_div2_clk>,
+							 <&f2s_free_clk>;
+						reg = <0x60>;
+					};
+
+					noc_free_clk: noc_free_clk {
+						#clock-cells = <0>;
+						compatible = "altr,socfpga-a10-perip-clk";
+						clocks = <&main_noc_base_clk>, <&peri_noc_base_clk>,
+							 <&osc1>, <&cb_intosc_hs_div2_clk>,
+							 <&f2s_free_clk>;
+						reg = <0x64>;
+					};
+
+					s2f_user1_free_clk: s2f_user1_free_clk {
+						#clock-cells = <0>;
+						compatible = "altr,socfpga-a10-perip-clk";
+						clocks = <&main_s2f_usr1_clk>, <&peri_s2f_usr1_clk>,
+							 <&osc1>, <&cb_intosc_hs_div2_clk>,
+							 <&f2s_free_clk>;
+						reg = <0x104>;
+					};
+
+					sdmmc_free_clk: sdmmc_free_clk {
+						#clock-cells = <0>;
+						compatible = "altr,socfpga-a10-perip-clk";
+						clocks = <&main_sdmmc_clk>, <&peri_sdmmc_clk>,
+							 <&osc1>, <&cb_intosc_hs_div2_clk>,
+							 <&f2s_free_clk>;
+						fixed-divider = <4>;
+						reg = <0xF8>;
+					};
+
+					l4_sys_free_clk: l4_sys_free_clk {
+						#clock-cells = <0>;
+						compatible = "altr,socfpga-a10-perip-clk";
+						clocks = <&noc_free_clk>;
+						fixed-divider = <4>;
+					};
+
+					l4_main_clk: l4_main_clk {
+						#clock-cells = <0>;
+						compatible = "altr,socfpga-a10-gate-clk";
+						clocks = <&noc_free_clk>;
+						div-reg = <0xA8 0 2>;
+						clk-gate = <0x48 1>;
+					};
+
+					l4_mp_clk: l4_mp_clk {
+						#clock-cells = <0>;
+						compatible = "altr,socfpga-a10-gate-clk";
+						clocks = <&noc_free_clk>;
+						div-reg = <0xA8 8 2>;
+						clk-gate = <0x48 2>;
+					};
+
+					l4_sp_clk: l4_sp_clk {
+						#clock-cells = <0>;
+						compatible = "altr,socfpga-a10-gate-clk";
+						clocks = <&noc_free_clk>;
+						div-reg = <0xA8 16 2>;
+						clk-gate = <0x48 3>;
+					};
+
+					mpu_periph_clk: mpu_periph_clk {
+						#clock-cells = <0>;
+						compatible = "altr,socfpga-a10-gate-clk";
+						clocks = <&mpu_free_clk>;
+						fixed-divider = <4>;
+						clk-gate = <0x48 0>;
+					};
+
+					sdmmc_clk: sdmmc_clk {
+						#clock-cells = <0>;
+						compatible = "altr,socfpga-a10-gate-clk";
+						clocks = <&sdmmc_free_clk>;
+						clk-gate = <0xC8 5>;
+					};
+
+					qspi_clk: qspi_clk {
+						#clock-cells = <0>;
+						compatible = "altr,socfpga-a10-gate-clk";
+						clocks = <&l4_main_clk>;
+						clk-gate = <0xC8 11>;
+					};
+
+					nand_clk: nand_clk {
+						#clock-cells = <0>;
+						compatible = "altr,socfpga-a10-gate-clk";
+						clocks = <&l4_mp_clk>;
+						clk-gate = <0xC8 10>;
+					};
+
+					spi_m_clk: spi_m_clk {
+						#clock-cells = <0>;
+						compatible = "altr,socfpga-a10-gate-clk";
+						clocks = <&l4_main_clk>;
+						clk-gate = <0xC8 9>;
+					};
+
+					usb_clk: usb_clk {
+						#clock-cells = <0>;
+						compatible = "altr,socfpga-a10-gate-clk";
+						clocks = <&l4_mp_clk>;
+						clk-gate = <0xC8 8>;
+					};
+
+					s2f_usr1_clk: s2f_usr1_clk {
+						#clock-cells = <0>;
+						compatible = "altr,socfpga-a10-gate-clk";
+						clocks = <&peri_s2f_usr1_clk>;
+						clk-gate = <0xC8 6>;
 					};
 				};
 		};
@@ -256,6 +541,8 @@
 			reg = <0xff808000 0x1000>;
 			interrupts = <0 98 IRQ_TYPE_LEVEL_HIGH>;
 			fifo-depth = <0x400>;
+			clocks = <&l4_mp_clk>, <&sdmmc_free_clk>;
+			clock-names = "biu", "ciu";
 			status = "disabled";
 		};
 
@@ -281,30 +568,39 @@
 			compatible = "arm,cortex-a9-twd-timer";
 			reg = <0xffffc600 0x100>;
 			interrupts = <1 13 0xf04>;
+			clocks = <&mpu_periph_clk>;
 		};
 
 		timer0: timer0@ffc02700 {
 			compatible = "snps,dw-apb-timer";
 			interrupts = <0 115 IRQ_TYPE_LEVEL_HIGH>;
 			reg = <0xffc02700 0x100>;
+			clocks = <&l4_sp_clk>;
+			clock-names = "timer";
 		};
 
 		timer1: timer1@ffc02800 {
 			compatible = "snps,dw-apb-timer";
 			interrupts = <0 116 IRQ_TYPE_LEVEL_HIGH>;
 			reg = <0xffc02800 0x100>;
+			clocks = <&l4_sp_clk>;
+			clock-names = "timer";
 		};
 
 		timer2: timer2@ffd00000 {
 			compatible = "snps,dw-apb-timer";
 			interrupts = <0 117 IRQ_TYPE_LEVEL_HIGH>;
 			reg = <0xffd00000 0x100>;
+			clocks = <&l4_sys_free_clk>;
+			clock-names = "timer";
 		};
 
 		timer3: timer3@ffd00100 {
 			compatible = "snps,dw-apb-timer";
 			interrupts = <0 118 IRQ_TYPE_LEVEL_HIGH>;
 			reg = <0xffd01000 0x100>;
+			clocks = <&l4_sys_free_clk>;
+			clock-names = "timer";
 		};
 
 		uart0: serial0@ffc02000 {
@@ -322,6 +618,7 @@
 			interrupts = <0 111 IRQ_TYPE_LEVEL_HIGH>;
 			reg-shift = <2>;
 			reg-io-width = <4>;
+			clocks = <&l4_sp_clk>;
 			status = "disabled";
 		};
 
@@ -335,6 +632,8 @@
 			compatible = "snps,dwc2";
 			reg = <0xffb00000 0xffff>;
 			interrupts = <0 95 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&usb_clk>;
+			clock-names = "otg";
 			phys = <&usbphy0>;
 			phy-names = "usb2-phy";
 			status = "disabled";
@@ -353,6 +652,7 @@
 			compatible = "snps,dw-wdt";
 			reg = <0xffd00200 0x100>;
 			interrupts = <0 119 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&l4_sys_free_clk>;
 			status = "disabled";
 		};
 
@@ -360,6 +660,7 @@
 			compatible = "snps,dw-wdt";
 			reg = <0xffd00300 0x100>;
 			interrupts = <0 120 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&l4_sys_free_clk>;
 			status = "disabled";
 		};
 	};
-- 
2.2.1

[PATCHv3 4/4] Documentation: DT bindings: document the clocks for Arria10

[dinguyen]

From: Dinh Nguyen <dinguyen@opensource.altera.com>

Update the bindings document for the clocks on the SoCFPGA Arria10 platform.
Also fix up a spelling error for the "altr,socfpga-perip-clk".

Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
---
 .../devicetree/bindings/clock/altr_socfpga.txt          | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/altr_socfpga.txt b/Documentation/devicetree/bindings/clock/altr_socfpga.txt
index f72e80e..317e9cc 100644
--- a/Documentation/devicetree/bindings/clock/altr_socfpga.txt
+++ b/Documentation/devicetree/bindings/clock/altr_socfpga.txt
@@ -7,11 +7,15 @@ This binding uses the common clock binding[1].
 Required properties:
 - compatible : shall be one of the following:
 	"altr,socfpga-pll-clock" - for a PLL clock
-	"altr,socfpga-perip-clock" - The peripheral clock divided from the
+	"altr,socfpga-perip-clk" - The peripheral clock divided from the
 		PLL clock.
 	"altr,socfpga-gate-clk" - Clocks that directly feed peripherals and
 		can get gated.
-
+	"altr,socfpga-a10-pll-clock" - for a PLL clock on the Arria10.
+	"altr,socfpga-a10-perip-clk" - The peripheral clock divided from the
+		PLL clock on the Arria10.
+	"altr,socfpga-a10-gate-clk" - Clocks that directly feed peripherals and
+		can be gated.
 - reg : shall be the control register offset from CLOCK_MANAGER's base for the clock.
 - clocks : shall be the input parent clock phandle for the clock. This is
 	either an oscillator or a pll output.
@@ -19,10 +23,11 @@ Required properties:
 
 Optional properties:
 - fixed-divider : If clocks have a fixed divider value, use this property.
-- clk-gate : For "socfpga-gate-clk", clk-gate contains the gating register
-        and the bit index.
-- div-reg : For "socfpga-gate-clk" and "socfpga-periph-clock", div-reg contains
-	the divider register, bit shift, and width.
+- clk-gate : For "socfpga-gate-clk" and "altr,socfpga-a10-gate-clk", clk-gate
+	contains the gating register and the bit index.
+- div-reg : For "socfpga-gate-clk", "socfpga-perip-clk",
+	"altr,socfpga-a10-gate-clk", and "altr,socfpga-a10-perip-clk", div-reg
+	contains the divider register, bit shift, and width.
 - clk-phase : For the sdmmc_clk, contains the value of the clock phase that controls
 	the SDMMC CIU clock. The first value is the clk_sample(smpsel), and the second
 	value is the cclk_in_drv(drvsel). The clk-phase is used to enable the correct
-- 
2.2.1

Re: [PATCHv3 2/4] clk: socfpga: add a clock driver for the Arria 10 platform

[Stephen Boyd]

On 05/07, dinguyen@opensource.altera.com wrote:
> diff --git a/drivers/clk/socfpga/clk-gate-a10.c b/drivers/clk/socfpga/clk-gate-a10.c
> new file mode 100644
> index 0000000..fadf6f7
> --- /dev/null
> +++ b/drivers/clk/socfpga/clk-gate-a10.c
> @@ -0,0 +1,187 @@
[...]
> +
> +static int socfpga_clk_prepare(struct clk_hw *hwclk)
> +{
> +	struct socfpga_gate_clk *socfpgaclk = to_socfpga_gate_clk(hwclk);
> +	struct regmap *sys_mgr_base_addr;
> +	int i;
> +	u32 hs_timing;
> +	u32 clk_phase[2];
> +
> +	if (socfpgaclk->clk_phase[0] || socfpgaclk->clk_phase[1]) {
> +		sys_mgr_base_addr = syscon_regmap_lookup_by_compatible("altr,sys-mgr");
> +		if (IS_ERR(sys_mgr_base_addr)) {

Is there a reason the syscon is grabbed lazily in prepare? Why
not get it before registering this clock?

> +			pr_err("%s: failed to find altr,sys-mgr regmap!\n", __func__);
> +			return -EINVAL;
> +		}
> +
> +		for (i = 0; i < 2; i++) {

i < ARRAY_SIZE(clk_phase) ?

> +			switch (socfpgaclk->clk_phase[i]) {
> +			case 0:
> +				clk_phase[i] = 0;
> +				break;
> +			case 45:
> +				clk_phase[i] = 1;
> +				break;
> +			case 90:
> +				clk_phase[i] = 2;
> +				break;
> +			case 135:
> +				clk_phase[i] = 3;
> +				break;
> +			case 180:
> +				clk_phase[i] = 4;
> +				break;
> +			case 225:
> +				clk_phase[i] = 5;
> +				break;
> +			case 270:
> +				clk_phase[i] = 6;
> +				break;
> +			case 315:
> +				clk_phase[i] = 7;
> +				break;
> +			default:
> +				clk_phase[i] = 0;
> +				break;
> +			}
> +		}
> +
> +		hs_timing = SYSMGR_SDMMC_CTRL_SET(clk_phase[0], clk_phase[1]);
> +		regmap_write(sys_mgr_base_addr, SYSMGR_SDMMCGRP_CTRL_OFFSET,
> +			     hs_timing);
> +	}
> +	return 0;
> +}
> +
> +static struct clk_ops gateclk_ops = {

const?

> +	.prepare = socfpga_clk_prepare,
> +	.recalc_rate = socfpga_gate_clk_recalc_rate,
> +};
> +
> diff --git a/drivers/clk/socfpga/clk-periph-a10.c b/drivers/clk/socfpga/clk-periph-a10.c
> new file mode 100644
> index 0000000..81b9274
> --- /dev/null
> +++ b/drivers/clk/socfpga/clk-periph-a10.c
> @@ -0,0 +1,131 @@
[...]
> + */
> +#include <linux/clk.h>

Are you using this include?

> +#include <linux/clkdev.h>

Are you using this include?

Applies to every file added in this patch.

> +#include <linux/clk-provider.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +
> +#include "clk.h"
> +
> +#define CLK_MGR_FREE_SHIFT		16
> +#define CLK_MGR_FREE_MASK		0x7
> +
> +#define SOCFPGA_MPU_FREE_CLK		"mpu_free_clk"
> +#define SOCFPGA_NOC_FREE_CLK		"noc_free_clk"
> +#define SOCFPGA_SDMMC_FREE_CLK		"sdmmc_free_clk"
[..]
> +
> +static __init void __socfpga_periph_init(struct device_node *node,
> +	const struct clk_ops *ops)
> +{
[..]
> +	init.name = clk_name;
> +	init.ops = ops;
> +	init.flags = 0;
> +
> +	parent_name = of_clk_get_parent_name(node, 0);
> +	init.num_parents = 1;
> +	init.parent_names = &parent_name;
> +
> +	periph_clk->hw.hw.init = &init;
> +
> +	clk = clk_register(NULL, &periph_clk->hw.hw);
> +	if (WARN_ON(IS_ERR(clk))) {
> +		kfree(periph_clk);
> +		return;
> +	}
> +	rc = of_clk_add_provider(node, of_clk_src_simple_get, clk);

Why not check the return value?

> +
> diff --git a/drivers/clk/socfpga/clk-pll-a10.c b/drivers/clk/socfpga/clk-pll-a10.c
> new file mode 100644
> index 0000000..2adc2f5
> --- /dev/null
> +++ b/drivers/clk/socfpga/clk-pll-a10.c
[..]
> +
> +static u8 clk_pll_get_parent(struct clk_hw *hwclk)
> +{
> +	struct socfpga_pll *socfpgaclk = to_socfpga_clk(hwclk);
> +	u32 pll_src;
> +
> +	pll_src = readl(socfpgaclk->hw.reg);
> +
> +	return (pll_src >> CLK_MGR_PLL_CLK_SRC_SHIFT) &
> +		CLK_MGR_PLL_CLK_SRC_MASK;
> +}
> +
> +

Nitpick: Single newline please.

> +static struct clk_ops clk_pll_ops = {

const?

> +	.recalc_rate = clk_pll_recalc_rate,
> +	.get_parent = clk_pll_get_parent,
> +};
> +
> +static __init struct clk *__socfpga_pll_init(struct device_node *node,

__init goes after the return type, doesn't it?

> +	const struct clk_ops *ops)
> +{
> +	u32 reg;
> +	struct clk *clk;
> +	struct socfpga_pll *pll_clk;
> +	const char *clk_name = node->name;
> +	const char *parent_name[SOCFGPA_MAX_PARENTS];
> +	struct clk_init_data init;
> +	struct device_node *clkmgr_np;
> +	int rc;
> +	int i = 0;
> +
> +	of_property_read_u32(node, "reg", &reg);
> +
> +	pll_clk = kzalloc(sizeof(*pll_clk), GFP_KERNEL);
> +	if (WARN_ON(!pll_clk))
> +		return NULL;
> +
> +	clkmgr_np = of_find_compatible_node(NULL, NULL, "altr,clk-mgr");

I haven't looked at the binding, but I would expect there to be
some phandle to this node somewhere so that we don't have to
search the whole tree?

> +	clk_mgr_a10_base_addr = of_iomap(clkmgr_np, 0);
> +	BUG_ON(!clk_mgr_a10_base_addr);
> +	pll_clk->hw.reg = clk_mgr_a10_base_addr + reg;
> +
> +	of_property_read_string(node, "clock-output-names", &clk_name);
> +
> +	init.name = clk_name;
> +	init.ops = ops;
[..]
> diff --git a/drivers/clk/socfpga/clk.h b/drivers/clk/socfpga/clk.h
> index b09a5d5..6989847 100644
> --- a/drivers/clk/socfpga/clk.h
> +++ b/drivers/clk/socfpga/clk.h
> @@ -34,10 +34,14 @@
>  	((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0))
>  
>  extern void __iomem *clk_mgr_base_addr;
> +extern void __iomem *clk_mgr_a10_base_addr;
>  
>  void __init socfpga_pll_init(struct device_node *node);
>  void __init socfpga_periph_init(struct device_node *node);
>  void __init socfpga_gate_init(struct device_node *node);
> +void __init socfpga_a10_pll_init(struct device_node *node);
> +void __init socfpga_a10_periph_init(struct device_node *node);
> +void __init socfpga_a10_gate_init(struct device_node *node);

__init is useless on prototypes. It's not your fault for copying
previous code, but it would be good to avoid adding more and to
clean this up in some other patch.

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

Re: [PATCHv3 2/4] clk: socfpga: add a clock driver for the Arria 10 platform

[Dinh Nguyen]

On 5/15/15 7:52 PM, Stephen Boyd wrote:
> On 05/07, dinguyen@opensource.altera.com wrote:
>> diff --git a/drivers/clk/socfpga/clk-gate-a10.c b/drivers/clk/socfpga/clk-gate-a10.c
>> new file mode 100644
>> index 0000000..fadf6f7
>> --- /dev/null
>> +++ b/drivers/clk/socfpga/clk-gate-a10.c
>> @@ -0,0 +1,187 @@
> [...]
>> +
>> +static int socfpga_clk_prepare(struct clk_hw *hwclk)
>> +{
>> +	struct socfpga_gate_clk *socfpgaclk = to_socfpga_gate_clk(hwclk);
>> +	struct regmap *sys_mgr_base_addr;
>> +	int i;
>> +	u32 hs_timing;
>> +	u32 clk_phase[2];
>> +
>> +	if (socfpgaclk->clk_phase[0] || socfpgaclk->clk_phase[1]) {
>> +		sys_mgr_base_addr = syscon_regmap_lookup_by_compatible("altr,sys-mgr");
>> +		if (IS_ERR(sys_mgr_base_addr)) {
> 
> Is there a reason the syscon is grabbed lazily in prepare? Why
> not get it before registering this clock?

This syscon node is only associated with clocks that have a clk-phase
property, which on the SoCFPGA platform, is the SD/MMC clocks. The way
to implement this went through quite a few rounds of discussion for the
Cyclone5/Arria5 platform before settling to this method.

The reason why syscon is grabbed here is that the setting of the clock
phase must be done before enabling of the clock, so it seem that prepare
was a good place. Should this be move moved to the socfpga_gate_init()
instead?

> 
>> +			pr_err("%s: failed to find altr,sys-mgr regmap!\n", __func__);
>> +			return -EINVAL;
>> +		}
>> +
>> +		for (i = 0; i < 2; i++) {
> 
> i < ARRAY_SIZE(clk_phase) ?
> 

Will fix.

>> +			switch (socfpgaclk->clk_phase[i]) {
>> +			case 0:
>> +				clk_phase[i] = 0;
>> +				break;
>> +			case 45:
>> +				clk_phase[i] = 1;
>> +				break;
>> +			case 90:
>> +				clk_phase[i] = 2;
>> +				break;
>> +			case 135:
>> +				clk_phase[i] = 3;
>> +				break;
>> +			case 180:
>> +				clk_phase[i] = 4;
>> +				break;
>> +			case 225:
>> +				clk_phase[i] = 5;
>> +				break;
>> +			case 270:
>> +				clk_phase[i] = 6;
>> +				break;
>> +			case 315:
>> +				clk_phase[i] = 7;
>> +				break;
>> +			default:
>> +				clk_phase[i] = 0;
>> +				break;
>> +			}
>> +		}
>> +
>> +		hs_timing = SYSMGR_SDMMC_CTRL_SET(clk_phase[0], clk_phase[1]);
>> +		regmap_write(sys_mgr_base_addr, SYSMGR_SDMMCGRP_CTRL_OFFSET,
>> +			     hs_timing);
>> +	}
>> +	return 0;
>> +}
>> +
>> +static struct clk_ops gateclk_ops = {
> 
> const?
> 

I cannot make this a const as I am assigning the .enable/.disable to use
the common clk_gate_ops.

>> +	.prepare = socfpga_clk_prepare,
>> +	.recalc_rate = socfpga_gate_clk_recalc_rate,
>> +};
>> +
>> diff --git a/drivers/clk/socfpga/clk-periph-a10.c b/drivers/clk/socfpga/clk-periph-a10.c
>> new file mode 100644
>> index 0000000..81b9274
>> --- /dev/null
>> +++ b/drivers/clk/socfpga/clk-periph-a10.c
>> @@ -0,0 +1,131 @@
> [...]
>> + */
>> +#include <linux/clk.h>
> 
> Are you using this include?
> 
>> +#include <linux/clkdev.h>
> 
> Are you using this include?
> 
> Applies to every file added in this patch.

Removed...

> 
>> +#include <linux/clk-provider.h>
>> +#include <linux/io.h>
>> +#include <linux/of.h>
>> +
>> +#include "clk.h"
>> +
>> +#define CLK_MGR_FREE_SHIFT		16
>> +#define CLK_MGR_FREE_MASK		0x7
>> +
>> +#define SOCFPGA_MPU_FREE_CLK		"mpu_free_clk"
>> +#define SOCFPGA_NOC_FREE_CLK		"noc_free_clk"
>> +#define SOCFPGA_SDMMC_FREE_CLK		"sdmmc_free_clk"
> [..]
>> +
>> +static __init void __socfpga_periph_init(struct device_node *node,
>> +	const struct clk_ops *ops)
>> +{
> [..]
>> +	init.name = clk_name;
>> +	init.ops = ops;
>> +	init.flags = 0;
>> +
>> +	parent_name = of_clk_get_parent_name(node, 0);
>> +	init.num_parents = 1;
>> +	init.parent_names = &parent_name;
>> +
>> +	periph_clk->hw.hw.init = &init;
>> +
>> +	clk = clk_register(NULL, &periph_clk->hw.hw);
>> +	if (WARN_ON(IS_ERR(clk))) {
>> +		kfree(periph_clk);
>> +		return;
>> +	}
>> +	rc = of_clk_add_provider(node, of_clk_src_simple_get, clk);
> 
> Why not check the return value?

Added check...

> 
>> +
>> diff --git a/drivers/clk/socfpga/clk-pll-a10.c b/drivers/clk/socfpga/clk-pll-a10.c
>> new file mode 100644
>> index 0000000..2adc2f5
>> --- /dev/null
>> +++ b/drivers/clk/socfpga/clk-pll-a10.c
> [..]
>> +
>> +static u8 clk_pll_get_parent(struct clk_hw *hwclk)
>> +{
>> +	struct socfpga_pll *socfpgaclk = to_socfpga_clk(hwclk);
>> +	u32 pll_src;
>> +
>> +	pll_src = readl(socfpgaclk->hw.reg);
>> +
>> +	return (pll_src >> CLK_MGR_PLL_CLK_SRC_SHIFT) &
>> +		CLK_MGR_PLL_CLK_SRC_MASK;
>> +}
>> +
>> +
> 
> Nitpick: Single newline please.
> 

Cleaned up...
>> +static struct clk_ops clk_pll_ops = {
> 
> const?
> 

Same comment applies here as I'm using clk_get_ops for .enable/.disable.

>> +	.recalc_rate = clk_pll_recalc_rate,
>> +	.get_parent = clk_pll_get_parent,
>> +};
>> +
>> +static __init struct clk *__socfpga_pll_init(struct device_node *node,
> 
> __init goes after the return type, doesn't it?
> 
>> +	const struct clk_ops *ops)
>> +{
>> +	u32 reg;
>> +	struct clk *clk;
>> +	struct socfpga_pll *pll_clk;
>> +	const char *clk_name = node->name;
>> +	const char *parent_name[SOCFGPA_MAX_PARENTS];
>> +	struct clk_init_data init;
>> +	struct device_node *clkmgr_np;
>> +	int rc;
>> +	int i = 0;
>> +
>> +	of_property_read_u32(node, "reg", &reg);
>> +
>> +	pll_clk = kzalloc(sizeof(*pll_clk), GFP_KERNEL);
>> +	if (WARN_ON(!pll_clk))
>> +		return NULL;
>> +
>> +	clkmgr_np = of_find_compatible_node(NULL, NULL, "altr,clk-mgr");
> 
> I haven't looked at the binding, but I would expect there to be
> some phandle to this node somewhere so that we don't have to
> search the whole tree?
> 

I'm putting all of the clocks under the clk-mgr node as much of this
clock driver was derived from clk-highbank.

>> +	clk_mgr_a10_base_addr = of_iomap(clkmgr_np, 0);
>> +	BUG_ON(!clk_mgr_a10_base_addr);
>> +	pll_clk->hw.reg = clk_mgr_a10_base_addr + reg;
>> +
>> +	of_property_read_string(node, "clock-output-names", &clk_name);
>> +
>> +	init.name = clk_name;
>> +	init.ops = ops;
> [..]
>> diff --git a/drivers/clk/socfpga/clk.h b/drivers/clk/socfpga/clk.h
>> index b09a5d5..6989847 100644
>> --- a/drivers/clk/socfpga/clk.h
>> +++ b/drivers/clk/socfpga/clk.h
>> @@ -34,10 +34,14 @@
>>  	((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0))
>>  
>>  extern void __iomem *clk_mgr_base_addr;
>> +extern void __iomem *clk_mgr_a10_base_addr;
>>  
>>  void __init socfpga_pll_init(struct device_node *node);
>>  void __init socfpga_periph_init(struct device_node *node);
>>  void __init socfpga_gate_init(struct device_node *node);
>> +void __init socfpga_a10_pll_init(struct device_node *node);
>> +void __init socfpga_a10_periph_init(struct device_node *node);
>> +void __init socfpga_a10_gate_init(struct device_node *node);
> 
> __init is useless on prototypes. It's not your fault for copying
> previous code, but it would be good to avoid adding more and to
> clean this up in some other patch.
> 

Will clean up.

Thanks for reviewing this patch.

Dinh

Re: [PATCHv3 2/4] clk: socfpga: add a clock driver for the Arria 10 platform

[Stephen Boyd]

On 05/19/15 09:29, Dinh Nguyen wrote:
>
> On 5/15/15 7:52 PM, Stephen Boyd wrote:
>> On 05/07, dinguyen@opensource.altera.com wrote:
>>> +
>>> +static int socfpga_clk_prepare(struct clk_hw *hwclk)
>>> +{
>>> +	struct socfpga_gate_clk *socfpgaclk = to_socfpga_gate_clk(hwclk);
>>> +	struct regmap *sys_mgr_base_addr;
>>> +	int i;
>>> +	u32 hs_timing;
>>> +	u32 clk_phase[2];
>>> +
>>> +	if (socfpgaclk->clk_phase[0] || socfpgaclk->clk_phase[1]) {
>>> +		sys_mgr_base_addr = syscon_regmap_lookup_by_compatible("altr,sys-mgr");
>>> +		if (IS_ERR(sys_mgr_base_addr)) {
>> Is there a reason the syscon is grabbed lazily in prepare? Why
>> not get it before registering this clock?
> This syscon node is only associated with clocks that have a clk-phase
> property, which on the SoCFPGA platform, is the SD/MMC clocks. The way
> to implement this went through quite a few rounds of discussion for the
> Cyclone5/Arria5 platform before settling to this method.
>
> The reason why syscon is grabbed here is that the setting of the clock
> phase must be done before enabling of the clock, so it seem that prepare
> was a good place. Should this be move moved to the socfpga_gate_init()
> instead?

I was expecting the regmap to be found before the clock is registered
and stored away into the  socfpga_gate_clk structure. Getting the regmap
during prepare is akin to ioremapping a register region during prepare,
which doesn't sound right at all. Maybe there's some good reason in the
earlier discussions? Any hints?

>>> +			switch (socfpgaclk->clk_phase[i]) {
>>> +			case 0:
>>> +				clk_phase[i] = 0;
>>> +				break;
>>> +			case 45:
>>> +				clk_phase[i] = 1;
>>> +				break;
>>> +			case 90:
>>> +				clk_phase[i] = 2;
>>> +				break;
>>> +			case 135:
>>> +				clk_phase[i] = 3;
>>> +				break;
>>> +			case 180:
>>> +				clk_phase[i] = 4;
>>> +				break;
>>> +			case 225:
>>> +				clk_phase[i] = 5;
>>> +				break;
>>> +			case 270:
>>> +				clk_phase[i] = 6;
>>> +				break;
>>> +			case 315:
>>> +				clk_phase[i] = 7;
>>> +				break;
>>> +			default:
>>> +				clk_phase[i] = 0;
>>> +				break;
>>> +			}
>>> +		}
>>> +
>>> +		hs_timing = SYSMGR_SDMMC_CTRL_SET(clk_phase[0], clk_phase[1]);
>>> +		regmap_write(sys_mgr_base_addr, SYSMGR_SDMMCGRP_CTRL_OFFSET,
>>> +			     hs_timing);
>>> +	}
>>> +	return 0;
>>> +}
>>> +
>>> +static struct clk_ops gateclk_ops = {
>> const?
>>
> I cannot make this a const as I am assigning the .enable/.disable to use
> the common clk_gate_ops.
>
>

Hm.. ok. Maybe we should export those functions to modules.

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

Re: [PATCHv3 2/4] clk: socfpga: add a clock driver for the Arria 10 platform

[Dinh Nguyen]

On 5/19/15 4:50 PM, Stephen Boyd wrote:
> On 05/19/15 09:29, Dinh Nguyen wrote:
>>
>> On 5/15/15 7:52 PM, Stephen Boyd wrote:
>>> On 05/07, dinguyen@opensource.altera.com wrote:
>>>> +
>>>> +static int socfpga_clk_prepare(struct clk_hw *hwclk)
>>>> +{
>>>> +	struct socfpga_gate_clk *socfpgaclk = to_socfpga_gate_clk(hwclk);
>>>> +	struct regmap *sys_mgr_base_addr;
>>>> +	int i;
>>>> +	u32 hs_timing;
>>>> +	u32 clk_phase[2];
>>>> +
>>>> +	if (socfpgaclk->clk_phase[0] || socfpgaclk->clk_phase[1]) {
>>>> +		sys_mgr_base_addr = syscon_regmap_lookup_by_compatible("altr,sys-mgr");
>>>> +		if (IS_ERR(sys_mgr_base_addr)) {
>>> Is there a reason the syscon is grabbed lazily in prepare? Why
>>> not get it before registering this clock?
>> This syscon node is only associated with clocks that have a clk-phase
>> property, which on the SoCFPGA platform, is the SD/MMC clocks. The way
>> to implement this went through quite a few rounds of discussion for the
>> Cyclone5/Arria5 platform before settling to this method.
>>
>> The reason why syscon is grabbed here is that the setting of the clock
>> phase must be done before enabling of the clock, so it seem that prepare
>> was a good place. Should this be move moved to the socfpga_gate_init()
>> instead?
> 
> I was expecting the regmap to be found before the clock is registered
> and stored away into the  socfpga_gate_clk structure. Getting the regmap
> during prepare is akin to ioremapping a register region during prepare,
> which doesn't sound right at all. Maybe there's some good reason in the
> earlier discussions? Any hints?
> 

Ah okay, the earlier discussions revolve mainly around moving the regmap
from the SD/MMC driver into the clock driver. But there weren't any
issue raised for putting the regmap in the prepare function.

If you're curious, here are the links to the discussion for adding the
clk-phase to the driver:

http://archive.arm.linux.org.uk/lurker/message/20131212.203042.d37c8ee9.en.html

http://archive.arm.linux.org.uk/lurker/message/20140109.213116.1f13b27a.en.html

But perhaps putting the regmap lookup in the init function is the
correct way to do this?

Thanks,
Dinh

Re: [PATCHv3 2/4] clk: socfpga: add a clock driver for the Arria 10 platform

[Stephen Boyd]

On 05/19/15 16:12, Dinh Nguyen wrote:
>
> On 5/19/15 4:50 PM, Stephen Boyd wrote:
>> On 05/19/15 09:29, Dinh Nguyen wrote:
>>> On 5/15/15 7:52 PM, Stephen Boyd wrote:
>>>> On 05/07, dinguyen@opensource.altera.com wrote:
>>>>> +
>>>>> +static int socfpga_clk_prepare(struct clk_hw *hwclk)
>>>>> +{
>>>>> +	struct socfpga_gate_clk *socfpgaclk = to_socfpga_gate_clk(hwclk);
>>>>> +	struct regmap *sys_mgr_base_addr;
>>>>> +	int i;
>>>>> +	u32 hs_timing;
>>>>> +	u32 clk_phase[2];
>>>>> +
>>>>> +	if (socfpgaclk->clk_phase[0] || socfpgaclk->clk_phase[1]) {
>>>>> +		sys_mgr_base_addr = syscon_regmap_lookup_by_compatible("altr,sys-mgr");
>>>>> +		if (IS_ERR(sys_mgr_base_addr)) {
>>>> Is there a reason the syscon is grabbed lazily in prepare? Why
>>>> not get it before registering this clock?
>>> This syscon node is only associated with clocks that have a clk-phase
>>> property, which on the SoCFPGA platform, is the SD/MMC clocks. The way
>>> to implement this went through quite a few rounds of discussion for the
>>> Cyclone5/Arria5 platform before settling to this method.
>>>
>>> The reason why syscon is grabbed here is that the setting of the clock
>>> phase must be done before enabling of the clock, so it seem that prepare
>>> was a good place. Should this be move moved to the socfpga_gate_init()
>>> instead?
>> I was expecting the regmap to be found before the clock is registered
>> and stored away into the  socfpga_gate_clk structure. Getting the regmap
>> during prepare is akin to ioremapping a register region during prepare,
>> which doesn't sound right at all. Maybe there's some good reason in the
>> earlier discussions? Any hints?
>>
> Ah okay, the earlier discussions revolve mainly around moving the regmap
> from the SD/MMC driver into the clock driver. But there weren't any
> issue raised for putting the regmap in the prepare function.
>
> If you're curious, here are the links to the discussion for adding the
> clk-phase to the driver:
>
> http://archive.arm.linux.org.uk/lurker/message/20131212.203042.d37c8ee9.en.html
>
> http://archive.arm.linux.org.uk/lurker/message/20140109.213116.1f13b27a.en.html
>
> But perhaps putting the regmap lookup in the init function is the
> correct way to do this?

Yes that would seem more appropriate. I suspect this lazy approach is
done because syscon isn't ready when of_clk_init() runs though. If this
was written to be a proper device driver with probe defer support this
wouldn't be a problem.

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