[RFC v3 01/14] WIP: clk: add Atheros AR724X/AR913X/AR933X SoCs clock driver

[RFC v3 01/14] WIP: clk: add Atheros AR724X/AR913X/AR933X SoCs clock driver

[Antony Pavlov]

TODO: get pll registers base address from devicetree node

Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
Cc: Alban Bedel <albeu@free.fr>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: linux-clk@vger.kernel.org
Cc: linux-mips@linux-mips.org
Cc: devicetree@vger.kernel.org
---
 drivers/clk/Makefile                  |   1 +
 drivers/clk/clk-ath79.c               | 193 ++++++++++++++++++++++++++++++++++
 include/dt-bindings/clock/ath79-clk.h |  22 ++++
 3 files changed, 216 insertions(+)

diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 820714c..5101763 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -18,6 +18,7 @@ endif
 # hardware specific clock types
 # please keep this section sorted lexicographically by file/directory path name
 obj-$(CONFIG_MACH_ASM9260)		+= clk-asm9260.o
+obj-$(CONFIG_ATH79)			+= clk-ath79.o
 obj-$(CONFIG_COMMON_CLK_AXI_CLKGEN)	+= clk-axi-clkgen.o
 obj-$(CONFIG_ARCH_AXXIA)		+= clk-axm5516.o
 obj-$(CONFIG_COMMON_CLK_CDCE706)	+= clk-cdce706.o
diff --git a/drivers/clk/clk-ath79.c b/drivers/clk/clk-ath79.c
new file mode 100644
index 0000000..75338a7
--- /dev/null
+++ b/drivers/clk/clk-ath79.c
@@ -0,0 +1,193 @@
+/*
+ * Clock driver for Atheros AR724X/AR913X/AR933X SoCs
+ *
+ * Copyright (C) 2010-2011 Jaiganesh Narayanan <jnarayanan@atheros.com>
+ * Copyright (C) 2011 Gabor Juhos <juhosg@openwrt.org>
+ * Copyright (C) 2015 Alban Bedel <albeu@free.fr>
+ * Copyright (C) 2016 Antony Pavlov <antonynpavlov@gmail.com>
+ *
+ * Parts of this file are based on Atheros' 2.6.15/2.6.31 BSP
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that 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.
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include "clk.h"
+
+#include <dt-bindings/clock/ath79-clk.h>
+
+#include "asm/mach-ath79/ar71xx_regs.h"
+#include "asm/mach-ath79/ath79.h"
+
+#define MHZ (1000 * 1000)
+
+#define AR724X_BASE_FREQ	(40 * MHZ)
+
+static struct clk *ath79_clks[ATH79_CLK_END];
+
+static struct clk_onecell_data clk_data = {
+	.clks = ath79_clks,
+	.clk_num = ARRAY_SIZE(ath79_clks),
+};
+
+static struct clk *__init ath79_add_sys_clkdev(
+	const char *id, unsigned long rate)
+{
+	struct clk *clk;
+	int err;
+
+	clk = clk_register_fixed_rate(NULL, id, NULL, CLK_IS_ROOT, rate);
+	if (!clk)
+		panic("failed to allocate %s clock structure", id);
+
+	err = clk_register_clkdev(clk, id, NULL);
+	if (err)
+		panic("unable to register %s clock device", id);
+
+	return clk;
+}
+
+static void __init ar724x_clk_init(struct device_node *np)
+{
+	struct clk *ref_clk;
+	unsigned long of_ref_rate;
+	unsigned long ref_rate;
+	unsigned long cpu_rate;
+	unsigned long ddr_rate;
+	unsigned long ahb_rate;
+	u32 pll;
+	u32 freq;
+	u32 div;
+
+	ref_clk = of_clk_get(np, 0);
+	if (IS_ERR(ref_clk)) {
+		pr_err("%s: of_clk_get failed\n", np->full_name);
+		return;
+	}
+
+	of_ref_rate = clk_get_rate(ref_clk);
+
+	ref_rate = AR724X_BASE_FREQ;
+
+	if (of_ref_rate != ref_rate) {
+		pr_err("ref_rate != of_ref_rate\n");
+		ref_rate = of_ref_rate;
+	}
+
+	pll = ath79_pll_rr(AR724X_PLL_REG_CPU_CONFIG);
+
+	div = ((pll >> AR724X_PLL_FB_SHIFT) & AR724X_PLL_FB_MASK);
+	freq = div * ref_rate;
+
+	div = ((pll >> AR724X_PLL_REF_DIV_SHIFT) & AR724X_PLL_REF_DIV_MASK) * 2;
+	freq /= div;
+
+	cpu_rate = freq;
+
+	div = ((pll >> AR724X_DDR_DIV_SHIFT) & AR724X_DDR_DIV_MASK) + 1;
+	ddr_rate = freq / div;
+
+	div = (((pll >> AR724X_AHB_DIV_SHIFT) & AR724X_AHB_DIV_MASK) + 1) * 2;
+	ahb_rate = cpu_rate / div;
+
+	ath79_clks[ATH79_CLK_REF] = ath79_add_sys_clkdev("ref", ref_rate);
+	ath79_clks[ATH79_CLK_CPU] = ath79_add_sys_clkdev("cpu", cpu_rate);
+	ath79_clks[ATH79_CLK_DDR] = ath79_add_sys_clkdev("ddr", ddr_rate);
+	ath79_clks[ATH79_CLK_AHB] = ath79_add_sys_clkdev("ahb", ahb_rate);
+	ath79_clks[ATH79_CLK_WDT] = ath79_add_sys_clkdev("wdt", ahb_rate);
+	ath79_clks[ATH79_CLK_UART] = ath79_add_sys_clkdev("uart", ahb_rate);
+
+	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
+}
+
+static void __init ar933x_clk_init(struct device_node *np)
+{
+	struct clk *ref_clk;
+	unsigned long of_ref_rate;
+	unsigned long ref_rate;
+	unsigned long cpu_rate;
+	unsigned long ddr_rate;
+	unsigned long ahb_rate;
+	u32 clock_ctrl;
+	u32 cpu_config;
+	u32 freq;
+	u32 t;
+
+	ref_clk = of_clk_get(np, 0);
+	if (IS_ERR(ref_clk)) {
+		pr_err("%s: of_clk_get failed\n", np->full_name);
+		return;
+	}
+
+	of_ref_rate = clk_get_rate(ref_clk);
+
+	t = ath79_reset_rr(AR933X_RESET_REG_BOOTSTRAP);
+	if (t & AR933X_BOOTSTRAP_REF_CLK_40)
+		ref_rate = 40 * MHZ;
+	else
+		ref_rate = 25 * MHZ;
+
+	if (ref_rate != of_ref_rate) {
+		pr_err("ref_rate != of_ref_rate\n");
+		ref_rate = of_ref_rate;
+	}
+
+	clock_ctrl = ath79_pll_rr(AR933X_PLL_CLOCK_CTRL_REG);
+	if (clock_ctrl & AR933X_PLL_CLOCK_CTRL_BYPASS) {
+		cpu_rate = ref_rate;
+		ahb_rate = ref_rate;
+		ddr_rate = ref_rate;
+	} else {
+		cpu_config = ath79_pll_rr(AR933X_PLL_CPU_CONFIG_REG);
+
+		t = (cpu_config >> AR933X_PLL_CPU_CONFIG_REFDIV_SHIFT) &
+		    AR933X_PLL_CPU_CONFIG_REFDIV_MASK;
+		freq = ref_rate / t;
+
+		t = (cpu_config >> AR933X_PLL_CPU_CONFIG_NINT_SHIFT) &
+		    AR933X_PLL_CPU_CONFIG_NINT_MASK;
+		freq *= t;
+
+		t = (cpu_config >> AR933X_PLL_CPU_CONFIG_OUTDIV_SHIFT) &
+		    AR933X_PLL_CPU_CONFIG_OUTDIV_MASK;
+		if (t == 0)
+			t = 1;
+
+		freq >>= t;
+
+		t = ((clock_ctrl >> AR933X_PLL_CLOCK_CTRL_CPU_DIV_SHIFT) &
+		     AR933X_PLL_CLOCK_CTRL_CPU_DIV_MASK) + 1;
+		cpu_rate = freq / t;
+
+		t = ((clock_ctrl >> AR933X_PLL_CLOCK_CTRL_DDR_DIV_SHIFT) &
+		      AR933X_PLL_CLOCK_CTRL_DDR_DIV_MASK) + 1;
+		ddr_rate = freq / t;
+
+		t = ((clock_ctrl >> AR933X_PLL_CLOCK_CTRL_AHB_DIV_SHIFT) &
+		     AR933X_PLL_CLOCK_CTRL_AHB_DIV_MASK) + 1;
+		ahb_rate = freq / t;
+	}
+
+	ath79_clks[ATH79_CLK_REF] = ath79_add_sys_clkdev("ref", ref_rate);
+	ath79_clks[ATH79_CLK_CPU] = ath79_add_sys_clkdev("cpu", cpu_rate);
+	ath79_clks[ATH79_CLK_DDR] = ath79_add_sys_clkdev("ddr", ddr_rate);
+	ath79_clks[ATH79_CLK_AHB] = ath79_add_sys_clkdev("ahb", ahb_rate);
+	ath79_clks[ATH79_CLK_WDT] = ath79_add_sys_clkdev("wdt", ahb_rate);
+	ath79_clks[ATH79_CLK_UART] = ath79_add_sys_clkdev("uart", ref_rate);
+
+	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
+}
+CLK_OF_DECLARE(ar9130_clk, "qca,ar9130-pll", ar724x_clk_init);
+CLK_OF_DECLARE(ar933x_clk, "qca,ar9330-pll", ar933x_clk_init);
diff --git a/include/dt-bindings/clock/ath79-clk.h b/include/dt-bindings/clock/ath79-clk.h
new file mode 100644
index 0000000..1c6fb04
--- /dev/null
+++ b/include/dt-bindings/clock/ath79-clk.h
@@ -0,0 +1,22 @@
+/*
+ * Copyright (C) 2014, 2016 Antony Pavlov <antonynpavlov@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#ifndef __DT_BINDINGS_ATH79_CLK_H
+#define __DT_BINDINGS_ATH79_CLK_H
+
+#define ATH79_CLK_REF		0
+#define ATH79_CLK_CPU		1
+#define ATH79_CLK_DDR		2
+#define ATH79_CLK_AHB		3
+#define ATH79_CLK_WDT		4
+#define ATH79_CLK_UART		5
+
+#define ATH79_CLK_END		6
+
+#endif /* __DT_BINDINGS_ATH79_CLK_H */
-- 
2.6.2

[RFC v3 03/14] MIPS: dts: qca: ar9132: use dt-bindings/clock/ath79-clk.h macros

[Antony Pavlov]

The old ar9132 clk implementation (see arch/mips/ath79/clock.c)
have had only 3 devicetree clks (cpu, ddr, ahb). Two additional
clocks (wdt and uart) have realized as aliases for ahb clock.
In the old ar9132 clk implementation the wdt and uart clocks
were inaccessible via devicetree so index "2" used for reference
to ahb clock instead, e.g.

    clocks = <&pll 2>;

In the new ar9132 clk implementation (see drivers/clk/clk-ath79.c)
the wdt and uart clks are accessible via devicetree
so appropriate ATH79_CLK_WDT and ATH79_CLK_UART are used in ar9132.dtsi.

Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
Cc: Alban Bedel <albeu@free.fr>
Cc: linux-mips@linux-mips.org
Cc: devicetree@vger.kernel.org
Cc: linux-clk@vger.kernel.org
---
 arch/mips/boot/dts/qca/ar9132.dtsi | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/mips/boot/dts/qca/ar9132.dtsi b/arch/mips/boot/dts/qca/ar9132.dtsi
index 13d0439..4a537ea 100644
--- a/arch/mips/boot/dts/qca/ar9132.dtsi
+++ b/arch/mips/boot/dts/qca/ar9132.dtsi
@@ -1,3 +1,5 @@
+#include <dt-bindings/clock/ath79-clk.h>
+
 / {
 	compatible = "qca,ar9132";
 
@@ -57,7 +59,7 @@
 				reg = <0x18020000 0x20>;
 				interrupts = <3>;
 
-				clocks = <&pll 2>;
+				clocks = <&pll ATH79_CLK_UART>;
 				clock-names = "uart";
 
 				reg-io-width = <4>;
@@ -100,7 +102,7 @@
 
 				interrupts = <4>;
 
-				clocks = <&pll 2>;
+				clocks = <&pll ATH79_CLK_WDT>;
 				clock-names = "wdt";
 			};
 
@@ -129,7 +131,7 @@
 			compatible = "qca,ar9132-spi", "qca,ar7100-spi";
 			reg = <0x1f000000 0x10>;
 
-			clocks = <&pll 2>;
+			clocks = <&pll ATH79_CLK_AHB>;
 			clock-names = "ahb";
 
 			status = "disabled";
-- 
2.6.2

[RFC v3 04/14] MIPS: dts: qca: ar9132: make extosc-related description shorter

[Antony Pavlov]

Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
Cc: Alban Bedel <albeu@free.fr>
Cc: linux-mips@linux-mips.org
Cc: devicetree@vger.kernel.org
Cc: linux-clk@vger.kernel.org
---
 arch/mips/boot/dts/qca/ar9132.dtsi               |  9 ++++++++-
 arch/mips/boot/dts/qca/ar9132_tl_wr1043nd_v1.dts | 14 ++++----------
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/arch/mips/boot/dts/qca/ar9132.dtsi b/arch/mips/boot/dts/qca/ar9132.dtsi
index 4a537ea..cd1602f 100644
--- a/arch/mips/boot/dts/qca/ar9132.dtsi
+++ b/arch/mips/boot/dts/qca/ar9132.dtsi
@@ -28,6 +28,13 @@
 					<&ddr_ctrl 0>, <&ddr_ctrl 1>;
 	};
 
+	extosc: oscillator {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+
+		/* The board must provide the extosc clock frequency */
+	};
+
 	ahb {
 		compatible = "simple-bus";
 		ranges;
@@ -89,8 +96,8 @@
 						"qca,ar9130-pll";
 				reg = <0x18050000 0x20>;
 
+				clocks = <&extosc>;
 				clock-names = "ref";
-				/* The board must provides the ref clock */
 
 				#clock-cells = <1>;
 				clock-output-names = "cpu", "ddr", "ahb";
diff --git a/arch/mips/boot/dts/qca/ar9132_tl_wr1043nd_v1.dts b/arch/mips/boot/dts/qca/ar9132_tl_wr1043nd_v1.dts
index 003015a..53057ca 100644
--- a/arch/mips/boot/dts/qca/ar9132_tl_wr1043nd_v1.dts
+++ b/arch/mips/boot/dts/qca/ar9132_tl_wr1043nd_v1.dts
@@ -18,21 +18,11 @@
 		reg = <0x0 0x2000000>;
 	};
 
-	extosc: oscillator {
-		compatible = "fixed-clock";
-		#clock-cells = <0>;
-		clock-frequency = <40000000>;
-	};
-
 	ahb {
 		apb {
 			uart@18020000 {
 				status = "okay";
 			};
-
-			pll-controller@18050000 {
-				clocks = <&extosc>;
-			};
 		};
 
 		spi@1f000000 {
@@ -110,3 +100,7 @@
 		};
 	};
 };
+
+&extosc {
+	clock-frequency = <40000000>;
+};
-- 
2.6.2

Re: [RFC v3 01/14] WIP: clk: add Atheros AR724X/AR913X/AR933X SoCs clock driver

[Alban]

On Sat, 23 Jan 2016 23:17:18 +0300
Antony Pavlov <antonynpavlov@gmail.com> wrote:

> TODO: get pll registers base address from devicetree node
> 
> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> Cc: Alban Bedel <albeu@free.fr>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@codeaurora.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Cc: linux-mips@linux-mips.org
> Cc: devicetree@vger.kernel.org
> ---
>  drivers/clk/Makefile                  |   1 +
>  drivers/clk/clk-ath79.c               | 193 ++++++++++++++++++++++++++++++++++
>  include/dt-bindings/clock/ath79-clk.h |  22 ++++
>  3 files changed, 216 insertions(+)
> 
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 820714c..5101763 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -18,6 +18,7 @@ endif
>  # hardware specific clock types
>  # please keep this section sorted lexicographically by file/directory path name
>  obj-$(CONFIG_MACH_ASM9260)		+= clk-asm9260.o
> +obj-$(CONFIG_ATH79)			+= clk-ath79.o
>  obj-$(CONFIG_COMMON_CLK_AXI_CLKGEN)	+= clk-axi-clkgen.o
>  obj-$(CONFIG_ARCH_AXXIA)		+= clk-axm5516.o
>  obj-$(CONFIG_COMMON_CLK_CDCE706)	+= clk-cdce706.o
> diff --git a/drivers/clk/clk-ath79.c b/drivers/clk/clk-ath79.c
> new file mode 100644
> index 0000000..75338a7
> --- /dev/null
> +++ b/drivers/clk/clk-ath79.c
> @@ -0,0 +1,193 @@
> +/*
> + * Clock driver for Atheros AR724X/AR913X/AR933X SoCs
> + *
> + * Copyright (C) 2010-2011 Jaiganesh Narayanan <jnarayanan@atheros.com>
> + * Copyright (C) 2011 Gabor Juhos <juhosg@openwrt.org>
> + * Copyright (C) 2015 Alban Bedel <albeu@free.fr>
> + * Copyright (C) 2016 Antony Pavlov <antonynpavlov@gmail.com>
> + *
> + * Parts of this file are based on Atheros' 2.6.15/2.6.31 BSP
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that 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.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/clkdev.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include "clk.h"
> +
> +#include <dt-bindings/clock/ath79-clk.h>
> +
> +#include "asm/mach-ath79/ar71xx_regs.h"
> +#include "asm/mach-ath79/ath79.h"
> +
> +#define MHZ (1000 * 1000)
> +
> +#define AR724X_BASE_FREQ	(40 * MHZ)
> +
> +static struct clk *ath79_clks[ATH79_CLK_END];
> +
> +static struct clk_onecell_data clk_data = {
> +	.clks = ath79_clks,
> +	.clk_num = ARRAY_SIZE(ath79_clks),
> +};
> +
> +static struct clk *__init ath79_add_sys_clkdev(
> +	const char *id, unsigned long rate)
> +{
> +	struct clk *clk;
> +	int err;
> +
> +	clk = clk_register_fixed_rate(NULL, id, NULL, CLK_IS_ROOT, rate);
> +	if (!clk)
> +		panic("failed to allocate %s clock structure", id);
> +
> +	err = clk_register_clkdev(clk, id, NULL);
> +	if (err)
> +		panic("unable to register %s clock device", id);
> +
> +	return clk;
> +}
>
> +static void __init ar724x_clk_init(struct device_node *np)
> +{
> +	struct clk *ref_clk;
> +	unsigned long of_ref_rate;
> +	unsigned long ref_rate;
> +	unsigned long cpu_rate;
> +	unsigned long ddr_rate;
> +	unsigned long ahb_rate;
> +	u32 pll;
> +	u32 freq;
> +	u32 div;
> +
> +	ref_clk = of_clk_get(np, 0);
> +	if (IS_ERR(ref_clk)) {
> +		pr_err("%s: of_clk_get failed\n", np->full_name);
> +		return;
> +	}

It would be better to have this function take the ref clock as
argument, to allow using it for both OF and legacy platforms.

> +	of_ref_rate = clk_get_rate(ref_clk);
> +
> +	ref_rate = AR724X_BASE_FREQ;
> +
> +	if (of_ref_rate != ref_rate) {
> +		pr_err("ref_rate != of_ref_rate\n");
> +		ref_rate = of_ref_rate;
> +	}

I don't think that this test is really useful.

> +	pll = ath79_pll_rr(AR724X_PLL_REG_CPU_CONFIG);
> +
> +	div = ((pll >> AR724X_PLL_FB_SHIFT) & AR724X_PLL_FB_MASK);
> +	freq = div * ref_rate;
> +
> +	div = ((pll >> AR724X_PLL_REF_DIV_SHIFT) & AR724X_PLL_REF_DIV_MASK) * 2;
> +	freq /= div;
> +
> +	cpu_rate = freq;
> +
> +	div = ((pll >> AR724X_DDR_DIV_SHIFT) & AR724X_DDR_DIV_MASK) + 1;
> +	ddr_rate = freq / div;
> +
> +	div = (((pll >> AR724X_AHB_DIV_SHIFT) & AR724X_AHB_DIV_MASK) + 1) * 2;
> +	ahb_rate = cpu_rate / div;

For a new driver it would make sense to use clk_register_divider() and
similar generic building blocks.

> +	ath79_clks[ATH79_CLK_REF] = ath79_add_sys_clkdev("ref", ref_rate);
> +	ath79_clks[ATH79_CLK_CPU] = ath79_add_sys_clkdev("cpu", cpu_rate);
> +	ath79_clks[ATH79_CLK_DDR] = ath79_add_sys_clkdev("ddr", ddr_rate);
> +	ath79_clks[ATH79_CLK_AHB] = ath79_add_sys_clkdev("ahb", ahb_rate);
> +	ath79_clks[ATH79_CLK_WDT] = ath79_add_sys_clkdev("wdt", ahb_rate);
> +	ath79_clks[ATH79_CLK_UART] = ath79_add_sys_clkdev("uart", ahb_rate);

You shouldn't add ref, wdt and uart, they are not needed and make the
driver incompatible with the current DT bindings.

> +	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
> +}
> +
> +static void __init ar933x_clk_init(struct device_node *np)
> +{
> +	struct clk *ref_clk;
> +	unsigned long of_ref_rate;
> +	unsigned long ref_rate;
> +	unsigned long cpu_rate;
> +	unsigned long ddr_rate;
> +	unsigned long ahb_rate;
> +	u32 clock_ctrl;
> +	u32 cpu_config;
> +	u32 freq;
> +	u32 t;
> +
> +	ref_clk = of_clk_get(np, 0);
> +	if (IS_ERR(ref_clk)) {
> +		pr_err("%s: of_clk_get failed\n", np->full_name);
> +		return;
> +	}
> +
> +	of_ref_rate = clk_get_rate(ref_clk);
> +
> +	t = ath79_reset_rr(AR933X_RESET_REG_BOOTSTRAP);
> +	if (t & AR933X_BOOTSTRAP_REF_CLK_40)
> +		ref_rate = 40 * MHZ;
> +	else
> +		ref_rate = 25 * MHZ;
> +
> +	if (ref_rate != of_ref_rate) {
> +		pr_err("ref_rate != of_ref_rate\n");
> +		ref_rate = of_ref_rate;
> +	}
> +
> +	clock_ctrl = ath79_pll_rr(AR933X_PLL_CLOCK_CTRL_REG);
> +	if (clock_ctrl & AR933X_PLL_CLOCK_CTRL_BYPASS) {
> +		cpu_rate = ref_rate;
> +		ahb_rate = ref_rate;
> +		ddr_rate = ref_rate;
> +	} else {
> +		cpu_config = ath79_pll_rr(AR933X_PLL_CPU_CONFIG_REG);
> +
> +		t = (cpu_config >> AR933X_PLL_CPU_CONFIG_REFDIV_SHIFT) &
> +		    AR933X_PLL_CPU_CONFIG_REFDIV_MASK;
> +		freq = ref_rate / t;
> +
> +		t = (cpu_config >> AR933X_PLL_CPU_CONFIG_NINT_SHIFT) &
> +		    AR933X_PLL_CPU_CONFIG_NINT_MASK;
> +		freq *= t;
> +
> +		t = (cpu_config >> AR933X_PLL_CPU_CONFIG_OUTDIV_SHIFT) &
> +		    AR933X_PLL_CPU_CONFIG_OUTDIV_MASK;
> +		if (t == 0)
> +			t = 1;
> +
> +		freq >>= t;
> +
> +		t = ((clock_ctrl >> AR933X_PLL_CLOCK_CTRL_CPU_DIV_SHIFT) &
> +		     AR933X_PLL_CLOCK_CTRL_CPU_DIV_MASK) + 1;
> +		cpu_rate = freq / t;
> +
> +		t = ((clock_ctrl >> AR933X_PLL_CLOCK_CTRL_DDR_DIV_SHIFT) &
> +		      AR933X_PLL_CLOCK_CTRL_DDR_DIV_MASK) + 1;
> +		ddr_rate = freq / t;
> +
> +		t = ((clock_ctrl >> AR933X_PLL_CLOCK_CTRL_AHB_DIV_SHIFT) &
> +		     AR933X_PLL_CLOCK_CTRL_AHB_DIV_MASK) + 1;
> +		ahb_rate = freq / t;
> +	}
> +
> +	ath79_clks[ATH79_CLK_REF] = ath79_add_sys_clkdev("ref", ref_rate);
> +	ath79_clks[ATH79_CLK_CPU] = ath79_add_sys_clkdev("cpu", cpu_rate);
> +	ath79_clks[ATH79_CLK_DDR] = ath79_add_sys_clkdev("ddr", ddr_rate);
> +	ath79_clks[ATH79_CLK_AHB] = ath79_add_sys_clkdev("ahb", ahb_rate);
> +	ath79_clks[ATH79_CLK_WDT] = ath79_add_sys_clkdev("wdt", ahb_rate);
> +	ath79_clks[ATH79_CLK_UART] = ath79_add_sys_clkdev("uart", ref_rate);
> +
> +	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
> +}
> +CLK_OF_DECLARE(ar9130_clk, "qca,ar9130-pll", ar724x_clk_init);
> +CLK_OF_DECLARE(ar933x_clk, "qca,ar9330-pll", ar933x_clk_init);
> diff --git a/include/dt-bindings/clock/ath79-clk.h b/include/dt-bindings/clock/ath79-clk.h
> new file mode 100644
> index 0000000..1c6fb04
> --- /dev/null
> +++ b/include/dt-bindings/clock/ath79-clk.h
> @@ -0,0 +1,22 @@
> +/*
> + * Copyright (C) 2014, 2016 Antony Pavlov <antonynpavlov@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#ifndef __DT_BINDINGS_ATH79_CLK_H
> +#define __DT_BINDINGS_ATH79_CLK_H
> +
> +#define ATH79_CLK_REF		0
> +#define ATH79_CLK_CPU		1
> +#define ATH79_CLK_DDR		2
> +#define ATH79_CLK_AHB		3
> +#define ATH79_CLK_WDT		4
> +#define ATH79_CLK_UART		5
> +
> +#define ATH79_CLK_END		6
> +
> +#endif /* __DT_BINDINGS_ATH79_CLK_H */

Re: [RFC v3 01/14] WIP: clk: add Atheros AR724X/AR913X/AR933X SoCs clock driver

[Stephen Boyd]

On 01/23, Antony Pavlov wrote:
> TODO: get pll registers base address from devicetree node
> 
> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> Cc: Alban Bedel <albeu@free.fr>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@codeaurora.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Cc: linux-mips@linux-mips.org
> Cc: devicetree@vger.kernel.org
> ---

Is there a binding document for this?

>  drivers/clk/Makefile                  |   1 +
>  drivers/clk/clk-ath79.c               | 193 ++++++++++++++++++++++++++++++++++
>  include/dt-bindings/clock/ath79-clk.h |  22 ++++
>  3 files changed, 216 insertions(+)
> 
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 820714c..5101763 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> diff --git a/drivers/clk/clk-ath79.c b/drivers/clk/clk-ath79.c
> new file mode 100644
> index 0000000..75338a7
> --- /dev/null
> +++ b/drivers/clk/clk-ath79.c
> @@ -0,0 +1,193 @@
> +/*
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/clkdev.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include "clk.h"

Don't include this header.

> +
> +#include <dt-bindings/clock/ath79-clk.h>
> +
> +#include "asm/mach-ath79/ar71xx_regs.h"
> +#include "asm/mach-ath79/ath79.h"

Can we get away without including these headers?

> +
> +#define MHZ (1000 * 1000)
> +
> +#define AR724X_BASE_FREQ	(40 * MHZ)
> +
> +static struct clk *ath79_clks[ATH79_CLK_END];
> +
> +static struct clk_onecell_data clk_data = {
> +	.clks = ath79_clks,
> +	.clk_num = ARRAY_SIZE(ath79_clks),
> +};
> +
> +static struct clk *__init ath79_add_sys_clkdev(
> +	const char *id, unsigned long rate)
> +{
> +	struct clk *clk;
> +	int err;
> +
> +	clk = clk_register_fixed_rate(NULL, id, NULL, CLK_IS_ROOT, rate);
> +	if (!clk)
> +		panic("failed to allocate %s clock structure", id);
> +
> +	err = clk_register_clkdev(clk, id, NULL);

Why are we using clkdev? Can't we use DT lookups?

> +	if (err)
> +		panic("unable to register %s clock device", id);
> +
> +	return clk;
> +}
> +
> +static void __init ar724x_clk_init(struct device_node *np)
> +{
> +	struct clk *ref_clk;
> +	unsigned long of_ref_rate;
> +	unsigned long ref_rate;
> +	unsigned long cpu_rate;
> +	unsigned long ddr_rate;
> +	unsigned long ahb_rate;
> +	u32 pll;
> +	u32 freq;
> +	u32 div;
> +
> +	ref_clk = of_clk_get(np, 0);
> +	if (IS_ERR(ref_clk)) {
> +		pr_err("%s: of_clk_get failed\n", np->full_name);
> +		return;
> +	}
> +
> +	of_ref_rate = clk_get_rate(ref_clk);
> +
> +	ref_rate = AR724X_BASE_FREQ;
> +
> +	if (of_ref_rate != ref_rate) {
> +		pr_err("ref_rate != of_ref_rate\n");
> +		ref_rate = of_ref_rate;
> +	}
> +
> +	pll = ath79_pll_rr(AR724X_PLL_REG_CPU_CONFIG);
> +
> +	div = ((pll >> AR724X_PLL_FB_SHIFT) & AR724X_PLL_FB_MASK);
> +	freq = div * ref_rate;
> +
> +	div = ((pll >> AR724X_PLL_REF_DIV_SHIFT) & AR724X_PLL_REF_DIV_MASK) * 2;
> +	freq /= div;
> +
> +	cpu_rate = freq;
> +
> +	div = ((pll >> AR724X_DDR_DIV_SHIFT) & AR724X_DDR_DIV_MASK) + 1;
> +	ddr_rate = freq / div;
> +
> +	div = (((pll >> AR724X_AHB_DIV_SHIFT) & AR724X_AHB_DIV_MASK) + 1) * 2;
> +	ahb_rate = cpu_rate / div;
> +
> +	ath79_clks[ATH79_CLK_REF] = ath79_add_sys_clkdev("ref", ref_rate);
> +	ath79_clks[ATH79_CLK_CPU] = ath79_add_sys_clkdev("cpu", cpu_rate);
> +	ath79_clks[ATH79_CLK_DDR] = ath79_add_sys_clkdev("ddr", ddr_rate);
> +	ath79_clks[ATH79_CLK_AHB] = ath79_add_sys_clkdev("ahb", ahb_rate);
> +	ath79_clks[ATH79_CLK_WDT] = ath79_add_sys_clkdev("wdt", ahb_rate);
> +	ath79_clks[ATH79_CLK_UART] = ath79_add_sys_clkdev("uart", ahb_rate);
> +
> +	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);

What if this fails?

> +}
> +
> +static void __init ar933x_clk_init(struct device_node *np)
> +{
> +	struct clk *ref_clk;
> +	unsigned long of_ref_rate;
> +	unsigned long ref_rate;
> +	unsigned long cpu_rate;
> +	unsigned long ddr_rate;
> +	unsigned long ahb_rate;
> +	u32 clock_ctrl;
> +	u32 cpu_config;
> +	u32 freq;
> +	u32 t;
> +
> +	ref_clk = of_clk_get(np, 0);
> +	if (IS_ERR(ref_clk)) {
> +		pr_err("%s: of_clk_get failed\n", np->full_name);
> +		return;
> +	}
> +
> +	of_ref_rate = clk_get_rate(ref_clk);
> +
> +	t = ath79_reset_rr(AR933X_RESET_REG_BOOTSTRAP);
> +	if (t & AR933X_BOOTSTRAP_REF_CLK_40)
> +		ref_rate = 40 * MHZ;
> +	else
> +		ref_rate = 25 * MHZ;
> +
> +	if (ref_rate != of_ref_rate) {

Does this happen? I'd prefer we find a way to avoid calling
of_clk_get() and clk_get_rate() in this driver.

> +		pr_err("ref_rate != of_ref_rate\n");
> +		ref_rate = of_ref_rate;
> +	}
> +
> +	clock_ctrl = ath79_pll_rr(AR933X_PLL_CLOCK_CTRL_REG);
> +	if (clock_ctrl & AR933X_PLL_CLOCK_CTRL_BYPASS) {
> +		cpu_rate = ref_rate;
> +		ahb_rate = ref_rate;
> +		ddr_rate = ref_rate;

So if it's in bypass, why not register fixed factor clocks of
1/1 and set the parent to ref_clk?

> +	} else {
> +		cpu_config = ath79_pll_rr(AR933X_PLL_CPU_CONFIG_REG);
> +
> +		t = (cpu_config >> AR933X_PLL_CPU_CONFIG_REFDIV_SHIFT) &
> +		    AR933X_PLL_CPU_CONFIG_REFDIV_MASK;
> +		freq = ref_rate / t;
> +
> +		t = (cpu_config >> AR933X_PLL_CPU_CONFIG_NINT_SHIFT) &
> +		    AR933X_PLL_CPU_CONFIG_NINT_MASK;
> +		freq *= t;
> +
> +		t = (cpu_config >> AR933X_PLL_CPU_CONFIG_OUTDIV_SHIFT) &
> +		    AR933X_PLL_CPU_CONFIG_OUTDIV_MASK;
> +		if (t == 0)
> +			t = 1;
> +
> +		freq >>= t;
> +
> +		t = ((clock_ctrl >> AR933X_PLL_CLOCK_CTRL_CPU_DIV_SHIFT) &
> +		     AR933X_PLL_CLOCK_CTRL_CPU_DIV_MASK) + 1;
> +		cpu_rate = freq / t;
> +
> +		t = ((clock_ctrl >> AR933X_PLL_CLOCK_CTRL_DDR_DIV_SHIFT) &
> +		      AR933X_PLL_CLOCK_CTRL_DDR_DIV_MASK) + 1;
> +		ddr_rate = freq / t;
> +
> +		t = ((clock_ctrl >> AR933X_PLL_CLOCK_CTRL_AHB_DIV_SHIFT) &
> +		     AR933X_PLL_CLOCK_CTRL_AHB_DIV_MASK) + 1;
> +		ahb_rate = freq / t;

These look like something we could implement as real clocks with
clk_ops. The parent still looks like ref_clk, but we would be
reading hardware in recalc_rate to figure out what the rate is.

> +	}
> +
> +	ath79_clks[ATH79_CLK_REF] = ath79_add_sys_clkdev("ref", ref_rate);
> +	ath79_clks[ATH79_CLK_CPU] = ath79_add_sys_clkdev("cpu", cpu_rate);
> +	ath79_clks[ATH79_CLK_DDR] = ath79_add_sys_clkdev("ddr", ddr_rate);
> +	ath79_clks[ATH79_CLK_AHB] = ath79_add_sys_clkdev("ahb", ahb_rate);
> +	ath79_clks[ATH79_CLK_WDT] = ath79_add_sys_clkdev("wdt", ahb_rate);
> +	ath79_clks[ATH79_CLK_UART] = ath79_add_sys_clkdev("uart", ref_rate);
> +
> +	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
> +}
> +CLK_OF_DECLARE(ar9130_clk, "qca,ar9130-pll", ar724x_clk_init);
> +CLK_OF_DECLARE(ar933x_clk, "qca,ar9330-pll", ar933x_clk_init);

Is there a reason this isn't a platform driver?

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

Re: [RFC v3 01/14] WIP: clk: add Atheros AR724X/AR913X/AR933X SoCs clock driver

[Antony Pavlov]

On Mon, 25 Jan 2016 23:21:56 +0100
Alban <albeu@free.fr> wrote:

> On Sat, 23 Jan 2016 23:17:18 +0300
> Antony Pavlov <antonynpavlov@gmail.com> wrote:
>=20
> > TODO: get pll registers base address from devicetree node
> >=20
> > Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> > Cc: Alban Bedel <albeu@free.fr>
> > Cc: Michael Turquette <mturquette@baylibre.com>
> > Cc: Stephen Boyd <sboyd@codeaurora.org>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: linux-clk@vger.kernel.org
> > Cc: linux-mips@linux-mips.org
> > Cc: devicetree@vger.kernel.org
> > ---
> >  drivers/clk/Makefile                  |   1 +
> >  drivers/clk/clk-ath79.c               | 193 ++++++++++++++++++++++++++=
++++++++
> >  include/dt-bindings/clock/ath79-clk.h |  22 ++++
> >  3 files changed, 216 insertions(+)
> >=20
> > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> > index 820714c..5101763 100644
> > --- a/drivers/clk/Makefile
> > +++ b/drivers/clk/Makefile
> > @@ -18,6 +18,7 @@ endif
> >  # hardware specific clock types
> >  # please keep this section sorted lexicographically by file/directory =
path name
> >  obj-$(CONFIG_MACH_ASM9260)		+=3D clk-asm9260.o
> > +obj-$(CONFIG_ATH79)			+=3D clk-ath79.o
> >  obj-$(CONFIG_COMMON_CLK_AXI_CLKGEN)	+=3D clk-axi-clkgen.o
> >  obj-$(CONFIG_ARCH_AXXIA)		+=3D clk-axm5516.o
> >  obj-$(CONFIG_COMMON_CLK_CDCE706)	+=3D clk-cdce706.o
> > diff --git a/drivers/clk/clk-ath79.c b/drivers/clk/clk-ath79.c
> > new file mode 100644
> > index 0000000..75338a7
> > --- /dev/null
> > +++ b/drivers/clk/clk-ath79.c
> > @@ -0,0 +1,193 @@
> > +/*
> > + * Clock driver for Atheros AR724X/AR913X/AR933X SoCs
> > + *
> > + * Copyright (C) 2010-2011 Jaiganesh Narayanan <jnarayanan@atheros.com>
> > + * Copyright (C) 2011 Gabor Juhos <juhosg@openwrt.org>
> > + * Copyright (C) 2015 Alban Bedel <albeu@free.fr>
> > + * Copyright (C) 2016 Antony Pavlov <antonynpavlov@gmail.com>
> > + *
> > + * Parts of this file are based on Atheros' 2.6.15/2.6.31 BSP
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 of
> > + * the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that 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.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/clkdev.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include "clk.h"
> > +
> > +#include <dt-bindings/clock/ath79-clk.h>
> > +
> > +#include "asm/mach-ath79/ar71xx_regs.h"
> > +#include "asm/mach-ath79/ath79.h"
> > +
> > +#define MHZ (1000 * 1000)
> > +
> > +#define AR724X_BASE_FREQ	(40 * MHZ)
> > +
> > +static struct clk *ath79_clks[ATH79_CLK_END];
> > +
> > +static struct clk_onecell_data clk_data =3D {
> > +	.clks =3D ath79_clks,
> > +	.clk_num =3D ARRAY_SIZE(ath79_clks),
> > +};
> > +
> > +static struct clk *__init ath79_add_sys_clkdev(
> > +	const char *id, unsigned long rate)
> > +{
> > +	struct clk *clk;
> > +	int err;
> > +
> > +	clk =3D clk_register_fixed_rate(NULL, id, NULL, CLK_IS_ROOT, rate);
> > +	if (!clk)
> > +		panic("failed to allocate %s clock structure", id);
> > +
> > +	err =3D clk_register_clkdev(clk, id, NULL);
> > +	if (err)
> > +		panic("unable to register %s clock device", id);
> > +
> > +	return clk;
> > +}
> >
> > +static void __init ar724x_clk_init(struct device_node *np)
> > +{
> > +	struct clk *ref_clk;
> > +	unsigned long of_ref_rate;
> > +	unsigned long ref_rate;
> > +	unsigned long cpu_rate;
> > +	unsigned long ddr_rate;
> > +	unsigned long ahb_rate;
> > +	u32 pll;
> > +	u32 freq;
> > +	u32 div;
> > +
> > +	ref_clk =3D of_clk_get(np, 0);
> > +	if (IS_ERR(ref_clk)) {
> > +		pr_err("%s: of_clk_get failed\n", np->full_name);
> > +		return;
> > +	}
>=20
> It would be better to have this function take the ref clock as
> argument, to allow using it for both OF and legacy platforms.

I'll try to use this idea in v5 patch version.

> > +	of_ref_rate =3D clk_get_rate(ref_clk);
> > +
> > +	ref_rate =3D AR724X_BASE_FREQ;
> > +
> > +	if (of_ref_rate !=3D ref_rate) {
> > +		pr_err("ref_rate !=3D of_ref_rate\n");
> > +		ref_rate =3D of_ref_rate;
> > +	}
>=20
> I don't think that this test is really useful.

Yes, I can make this check optional.

> > +	pll =3D ath79_pll_rr(AR724X_PLL_REG_CPU_CONFIG);
> > +
> > +	div =3D ((pll >> AR724X_PLL_FB_SHIFT) & AR724X_PLL_FB_MASK);
> > +	freq =3D div * ref_rate;
> > +
> > +	div =3D ((pll >> AR724X_PLL_REF_DIV_SHIFT) & AR724X_PLL_REF_DIV_MASK)=
 * 2;
> > +	freq /=3D div;
> > +
> > +	cpu_rate =3D freq;
> > +
> > +	div =3D ((pll >> AR724X_DDR_DIV_SHIFT) & AR724X_DDR_DIV_MASK) + 1;
> > +	ddr_rate =3D freq / div;
> > +
> > +	div =3D (((pll >> AR724X_AHB_DIV_SHIFT) & AR724X_AHB_DIV_MASK) + 1) *=
 2;
> > +	ahb_rate =3D cpu_rate / div;
>=20
> For a new driver it would make sense to use clk_register_divider() and
> similar generic building blocks.
>=20
> > +	ath79_clks[ATH79_CLK_REF] =3D ath79_add_sys_clkdev("ref", ref_rate);
> > +	ath79_clks[ATH79_CLK_CPU] =3D ath79_add_sys_clkdev("cpu", cpu_rate);
> > +	ath79_clks[ATH79_CLK_DDR] =3D ath79_add_sys_clkdev("ddr", ddr_rate);
> > +	ath79_clks[ATH79_CLK_AHB] =3D ath79_add_sys_clkdev("ahb", ahb_rate);
> > +	ath79_clks[ATH79_CLK_WDT] =3D ath79_add_sys_clkdev("wdt", ahb_rate);
> > +	ath79_clks[ATH79_CLK_UART] =3D ath79_add_sys_clkdev("uart", ahb_rate);
>=20
> You shouldn't add ref, wdt and uart, they are not needed and make the
> driver incompatible with the current DT bindings.

Please describe the situation then this incompatibility does matter.

Current ath79 dt support is very preliminary and the only dt user
is 5-years old TP-Link WR1043ND so it's near impossible to break somethink.

Anyway current ath79 dt binding is somewhat broken (see __your__ message 'R=
e: [RFC 1/4] WIP: MIPS: ath79: make ar933x clks more devicetree-friendly' f=
rom 'Thu, 21 Jan 2016 12:03:20 +0100').


--=A0
Best regards,
=A0 Antony Pavlov

Re: [RFC v3 01/14] WIP: clk: add Atheros AR724X/AR913X/AR933X SoCs clock driver

[Antony Pavlov]

On Fri, 29 Jan 2016 16:27:34 -0800
Stephen Boyd <sboyd@codeaurora.org> wrote:

> On 01/23, Antony Pavlov wrote:
> > TODO: get pll registers base address from devicetree node
> >=20
> > Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> > Cc: Alban Bedel <albeu@free.fr>
> > Cc: Michael Turquette <mturquette@baylibre.com>
> > Cc: Stephen Boyd <sboyd@codeaurora.org>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: linux-clk@vger.kernel.org
> > Cc: linux-mips@linux-mips.org
> > Cc: devicetree@vger.kernel.org
> > ---
>=20
> Is there a binding document for this?

Yes, there is the binding document: Documentation/devicetree/bindings/clock=
/qca,ath79-pll.txt
Anyway I suppose I have to update it in RFCv5.

> >  drivers/clk/Makefile                  |   1 +
> >  drivers/clk/clk-ath79.c               | 193 ++++++++++++++++++++++++++=
++++++++
> >  include/dt-bindings/clock/ath79-clk.h |  22 ++++
> >  3 files changed, 216 insertions(+)
> >=20
> > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> > index 820714c..5101763 100644
> > --- a/drivers/clk/Makefile
> > +++ b/drivers/clk/Makefile
> > diff --git a/drivers/clk/clk-ath79.c b/drivers/clk/clk-ath79.c
> > new file mode 100644
> > index 0000000..75338a7
> > --- /dev/null
> > +++ b/drivers/clk/clk-ath79.c
> > @@ -0,0 +1,193 @@
> > +/*
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/clkdev.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include "clk.h"
>=20
> Don't include this header.

Ok, I'll try to drop it in RFC v5.


> > +
> > +#include <dt-bindings/clock/ath79-clk.h>
> > +
> > +#include "asm/mach-ath79/ar71xx_regs.h"
> > +#include "asm/mach-ath79/ath79.h"
>=20
> Can we get away without including these headers?

I have dropped ath79.h and ath79-clk.h in RFCv4.

> > +
> > +#define MHZ (1000 * 1000)
> > +
> > +#define AR724X_BASE_FREQ	(40 * MHZ)
> > +
> > +static struct clk *ath79_clks[ATH79_CLK_END];
> > +
> > +static struct clk_onecell_data clk_data =3D {
> > +	.clks =3D ath79_clks,
> > +	.clk_num =3D ARRAY_SIZE(ath79_clks),
> > +};
> > +
> > +static struct clk *__init ath79_add_sys_clkdev(
> > +	const char *id, unsigned long rate)
> > +{
> > +	struct clk *clk;
> > +	int err;
> > +
> > +	clk =3D clk_register_fixed_rate(NULL, id, NULL, CLK_IS_ROOT, rate);
> > +	if (!clk)
> > +		panic("failed to allocate %s clock structure", id);
> > +
> > +	err =3D clk_register_clkdev(clk, id, NULL);
>=20
> Why are we using clkdev? Can't we use DT lookups?

Hmm, I have just reused legacy ath79_add_sys_clkdev() function.
I'll try to see how to use DT lookups.

> > +	if (err)
> > +		panic("unable to register %s clock device", id);
> > +
> > +	return clk;
> > +}
> > +
> > +static void __init ar724x_clk_init(struct device_node *np)
> > +{
> > +	struct clk *ref_clk;
> > +	unsigned long of_ref_rate;
> > +	unsigned long ref_rate;
> > +	unsigned long cpu_rate;
> > +	unsigned long ddr_rate;
> > +	unsigned long ahb_rate;
> > +	u32 pll;
> > +	u32 freq;
> > +	u32 div;
> > +
> > +	ref_clk =3D of_clk_get(np, 0);
> > +	if (IS_ERR(ref_clk)) {
> > +		pr_err("%s: of_clk_get failed\n", np->full_name);
> > +		return;
> > +	}
> > +
> > +	of_ref_rate =3D clk_get_rate(ref_clk);
> > +
> > +	ref_rate =3D AR724X_BASE_FREQ;
> > +
> > +	if (of_ref_rate !=3D ref_rate) {
> > +		pr_err("ref_rate !=3D of_ref_rate\n");
> > +		ref_rate =3D of_ref_rate;
> > +	}
> > +
> > +	pll =3D ath79_pll_rr(AR724X_PLL_REG_CPU_CONFIG);
> > +
> > +	div =3D ((pll >> AR724X_PLL_FB_SHIFT) & AR724X_PLL_FB_MASK);
> > +	freq =3D div * ref_rate;
> > +
> > +	div =3D ((pll >> AR724X_PLL_REF_DIV_SHIFT) & AR724X_PLL_REF_DIV_MASK)=
 * 2;
> > +	freq /=3D div;
> > +
> > +	cpu_rate =3D freq;
> > +
> > +	div =3D ((pll >> AR724X_DDR_DIV_SHIFT) & AR724X_DDR_DIV_MASK) + 1;
> > +	ddr_rate =3D freq / div;
> > +
> > +	div =3D (((pll >> AR724X_AHB_DIV_SHIFT) & AR724X_AHB_DIV_MASK) + 1) *=
 2;
> > +	ahb_rate =3D cpu_rate / div;
> > +
> > +	ath79_clks[ATH79_CLK_REF] =3D ath79_add_sys_clkdev("ref", ref_rate);
> > +	ath79_clks[ATH79_CLK_CPU] =3D ath79_add_sys_clkdev("cpu", cpu_rate);
> > +	ath79_clks[ATH79_CLK_DDR] =3D ath79_add_sys_clkdev("ddr", ddr_rate);
> > +	ath79_clks[ATH79_CLK_AHB] =3D ath79_add_sys_clkdev("ahb", ahb_rate);
> > +	ath79_clks[ATH79_CLK_WDT] =3D ath79_add_sys_clkdev("wdt", ahb_rate);
> > +	ath79_clks[ATH79_CLK_UART] =3D ath79_add_sys_clkdev("uart", ahb_rate);
> > +
> > +	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
>=20
> What if this fails?

Ok, I'll add return code result checks as drivers/clk/ingenic/cgu.c does.
=20
> > +}
> > +
> > +static void __init ar933x_clk_init(struct device_node *np)
> > +{
> > +	struct clk *ref_clk;
> > +	unsigned long of_ref_rate;
> > +	unsigned long ref_rate;
> > +	unsigned long cpu_rate;
> > +	unsigned long ddr_rate;
> > +	unsigned long ahb_rate;
> > +	u32 clock_ctrl;
> > +	u32 cpu_config;
> > +	u32 freq;
> > +	u32 t;
> > +
> > +	ref_clk =3D of_clk_get(np, 0);
> > +	if (IS_ERR(ref_clk)) {
> > +		pr_err("%s: of_clk_get failed\n", np->full_name);
> > +		return;
> > +	}
> > +
> > +	of_ref_rate =3D clk_get_rate(ref_clk);
> > +
> > +	t =3D ath79_reset_rr(AR933X_RESET_REG_BOOTSTRAP);
> > +	if (t & AR933X_BOOTSTRAP_REF_CLK_40)
> > +		ref_rate =3D 40 * MHZ;
> > +	else
> > +		ref_rate =3D 25 * MHZ;
> > +
> > +	if (ref_rate !=3D of_ref_rate) {
>=20
> Does this happen? I'd prefer we find a way to avoid calling
> of_clk_get() and clk_get_rate() in this driver.
>=20
> > +		pr_err("ref_rate !=3D of_ref_rate\n");
> > +		ref_rate =3D of_ref_rate;
> > +	}
> > +
> > +	clock_ctrl =3D ath79_pll_rr(AR933X_PLL_CLOCK_CTRL_REG);
> > +	if (clock_ctrl & AR933X_PLL_CLOCK_CTRL_BYPASS) {
> > +		cpu_rate =3D ref_rate;
> > +		ahb_rate =3D ref_rate;
> > +		ddr_rate =3D ref_rate;
>=20
> So if it's in bypass, why not register fixed factor clocks of
> 1/1 and set the parent to ref_clk?

I suppose that realising clocks as "real clocks with clk_ops" will be more =
simple
solution.

> > +	} else {
> > +		cpu_config =3D ath79_pll_rr(AR933X_PLL_CPU_CONFIG_REG);
> > +
> > +		t =3D (cpu_config >> AR933X_PLL_CPU_CONFIG_REFDIV_SHIFT) &
> > +		    AR933X_PLL_CPU_CONFIG_REFDIV_MASK;
> > +		freq =3D ref_rate / t;
> > +
> > +		t =3D (cpu_config >> AR933X_PLL_CPU_CONFIG_NINT_SHIFT) &
> > +		    AR933X_PLL_CPU_CONFIG_NINT_MASK;
> > +		freq *=3D t;
> > +
> > +		t =3D (cpu_config >> AR933X_PLL_CPU_CONFIG_OUTDIV_SHIFT) &
> > +		    AR933X_PLL_CPU_CONFIG_OUTDIV_MASK;
> > +		if (t =3D=3D 0)
> > +			t =3D 1;
> > +
> > +		freq >>=3D t;
> > +
> > +		t =3D ((clock_ctrl >> AR933X_PLL_CLOCK_CTRL_CPU_DIV_SHIFT) &
> > +		     AR933X_PLL_CLOCK_CTRL_CPU_DIV_MASK) + 1;
> > +		cpu_rate =3D freq / t;
> > +
> > +		t =3D ((clock_ctrl >> AR933X_PLL_CLOCK_CTRL_DDR_DIV_SHIFT) &
> > +		      AR933X_PLL_CLOCK_CTRL_DDR_DIV_MASK) + 1;
> > +		ddr_rate =3D freq / t;
> > +
> > +		t =3D ((clock_ctrl >> AR933X_PLL_CLOCK_CTRL_AHB_DIV_SHIFT) &
> > +		     AR933X_PLL_CLOCK_CTRL_AHB_DIV_MASK) + 1;
> > +		ahb_rate =3D freq / t;
>=20
> These look like something we could implement as real clocks with
> clk_ops. The parent still looks like ref_clk, but we would be
> reading hardware in recalc_rate to figure out what the rate is.

Hmm. I have already realized this solution in 2014 for barebox
please see http://lists.infradead.org/pipermail/barebox/2014-March/018414.h=
tml

Current RFC v3 driver is based on arch/mips/ath79/clock.c, but
I suppose I have to use barebox driver as a base for RFC v5 driver.

> > +	}
> > +
> > +	ath79_clks[ATH79_CLK_REF] =3D ath79_add_sys_clkdev("ref", ref_rate);
> > +	ath79_clks[ATH79_CLK_CPU] =3D ath79_add_sys_clkdev("cpu", cpu_rate);
> > +	ath79_clks[ATH79_CLK_DDR] =3D ath79_add_sys_clkdev("ddr", ddr_rate);
> > +	ath79_clks[ATH79_CLK_AHB] =3D ath79_add_sys_clkdev("ahb", ahb_rate);
> > +	ath79_clks[ATH79_CLK_WDT] =3D ath79_add_sys_clkdev("wdt", ahb_rate);
> > +	ath79_clks[ATH79_CLK_UART] =3D ath79_add_sys_clkdev("uart", ref_rate);
> > +
> > +	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
> > +}
> > +CLK_OF_DECLARE(ar9130_clk, "qca,ar9130-pll", ar724x_clk_init);
> > +CLK_OF_DECLARE(ar933x_clk, "qca,ar9330-pll", ar933x_clk_init);
>=20
> Is there a reason this isn't a platform driver?

We already have legacy platform driver arch/mips/ath79/clock.c.
I'm trying to realize modern clk driver and then adapt it to use with legac=
y code.

--=A0
Best regards,
=A0 Antony Pavlov

Re: [RFC v3 01/14] WIP: clk: add Atheros AR724X/AR913X/AR933X SoCs clock driver

[Alban]

On Sun, 31 Jan 2016 23:41:55 +0300
Antony Pavlov <antonynpavlov@gmail.com> wrote:

> > > +	ath79_clks[ATH79_CLK_REF] = ath79_add_sys_clkdev("ref", ref_rate);
> > > +	ath79_clks[ATH79_CLK_CPU] = ath79_add_sys_clkdev("cpu", cpu_rate);
> > > +	ath79_clks[ATH79_CLK_DDR] = ath79_add_sys_clkdev("ddr", ddr_rate);
> > > +	ath79_clks[ATH79_CLK_AHB] = ath79_add_sys_clkdev("ahb", ahb_rate);
> > > +	ath79_clks[ATH79_CLK_WDT] = ath79_add_sys_clkdev("wdt", ahb_rate);
> > > +	ath79_clks[ATH79_CLK_UART] = ath79_add_sys_clkdev("uart", ahb_rate);  
> > 
> > You shouldn't add ref, wdt and uart, they are not needed and make the
> > driver incompatible with the current DT bindings.  
> 
> Please describe the situation then this incompatibility does matter.
> 
> Current ath79 dt support is very preliminary and the only dt user
> is 5-years old TP-Link WR1043ND so it's near impossible to break somethink.
> 
> Anyway current ath79 dt binding is somewhat broken (see __your__ message
> 'Re: [RFC 1/4] WIP: MIPS: ath79: make ar933x clks more
> devicetree-friendly' from 'Thu, 21 Jan 2016 12:03:20 +0100').

The point is that DT is about describing the hardware in a consistent
and OS independent manner. It shouldn't be modeled just to suit some
existing code. So it is no big deal if the code doesn't use all the
informations provided by the DT, like here where the input clock is not
*yet* used by the code. However it is a no-go to extend the binding to
add things that don't exists in the hardware just to suit some old code.

I agree we might need to clear a few things regarding the UART clock in
the newer SoC, in particular if the UART use the output of the PLL
pre-divider or something similar. Then we would need to rework the DT
binding for the those SoC. However with the current knowledge I don't
see any need to change the biding yet.

Alban