Re: [PATCH v3 2/4] clk: exynos5410: register clocks using common clock framework

2013-11-10 Thread Tomasz Figa
Hi,

On Thursday 07 of November 2013 12:12:47 Vyacheslav Tyrtov wrote:
> From: Tarek Dakhran 
> 
> The EXYNOS5410 clocks are statically listed and registered
> using the Samsung specific common clock helper functions.

Thanks for keeping up with addressing the comments. However there are
still few things that need to be corrected. Please see my comments inline.

> Signed-off-by: Tarek Dakhran 
> Signed-off-by: Vyacheslav Tyrtov 
> ---
>  .../devicetree/bindings/clock/exynos5410-clock.txt |  37 
>  drivers/clk/samsung/Makefile   |   1 +
>  drivers/clk/samsung/clk-exynos5410.c   | 239 
> +
>  include/dt-bindings/clock/exynos5410.h | 175 +++
>  4 files changed, 452 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/clock/exynos5410-clock.txt
>  create mode 100644 drivers/clk/samsung/clk-exynos5410.c
>  create mode 100644 include/dt-bindings/clock/exynos5410.h
> 
> diff --git a/Documentation/devicetree/bindings/clock/exynos5410-clock.txt 
> b/Documentation/devicetree/bindings/clock/exynos5410-clock.txt
> new file mode 100644
> index 000..a462da231
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/exynos5410-clock.txt
> @@ -0,0 +1,37 @@
> +* Samsung Exynos5410 Clock Controller
> +
> +The Exynos5410 clock controller generates and supplies clock to various
> +controllers within the Exynos5410 SoC.
> +
> +Required Properties:
> +
> +- compatible: should be "samsung,exynos5410-clock"
> +
> +- reg: physical base address of the controller and length of memory mapped
> +  region.
> +
> +- #clock-cells: should be 1.

If there are any external clocks that need to be provided (and I believe
there are), you should mention them in the documentation.

Also, to make this more future-proof, I would add clock-names and clocks
properties to the binding, listing all those external clocks. It's just
about the binding definition - you don't have to implement this in the
driver yet, as we don't have the framework to handle this at early system
initialization. However, when we finally implement this in the Common
Clock Framework, we will not have to change existing DT bindings.

> +
> +All available clocks are defined as preprocessor macros in
> +dt-bindings/clock/exynos5410.h header and can be used in device
> +tree sources.
> +
> +Example 1: An example of a clock controller node is listed below.
> +
> + clock: clock-controller@0x1001 {
> + compatible = "samsung,exynos5410-clock";
> + reg = <0x1001 0x3>;
> + #clock-cells = <1>;
> + };
> +
> +Example 2: UART controller node that consumes the clock generated by the 
> clock
> +controller. Refer to the standard clock bindings for information
> +about 'clocks' and 'clock-names' property.
> +
> + serial@12C2 {
> + compatible = "samsung,exynos4210-uart";
> + reg = <0x12C0 0x100>;
> + interrupts = <0 51 0>;
> + clocks = < CLK_UART0>, < CLK_SCLK_UART0>;
> + clock-names = "uart", "clk_uart_baud0";
> + };
[snip]
> diff --git a/include/dt-bindings/clock/exynos5410.h 
> b/include/dt-bindings/clock/exynos5410.h
> new file mode 100644
> index 000..9b4a58b
> --- /dev/null
> +++ b/include/dt-bindings/clock/exynos5410.h
> @@ -0,0 +1,175 @@
> +#ifndef _DT_BINDINGS_CLOCK_EXYNOS_5410_H
> +#define _DT_BINDINGS_CLOCK_EXYNOS_5410_H
> +
> +/* core clocks */
> +#define CLK_FIN_PLL 1

This is external clock, not provided by this clock controller, isn't it?

> +#define CLK_FOUT_APLL 2
> +#define CLK_FOUT_CPLL 3
> +#define CLK_FOUT_DPLL 4
> +#define CLK_FOUT_EPLL 5
[snip]
> +#define CLK_ACLK_G3D 500
> +#define CLK_G3D 501
> +#define CLK_SMMU_MIXER 502
> +
> +/* mux clocks */
> +#define CLK_MOUT_HDMI 640

This definition does not seem to be used anywhere in the driver itself.

> +
> +/* divider clocks */
> +#define CLK_DOUT_PIXEL 768

Ditto.

Please don't define IDs for clocks that are not yet provided by the
driver.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v3 2/4] clk: exynos5410: register clocks using common clock framework

2013-11-10 Thread Tomasz Figa
Hi,

On Thursday 07 of November 2013 12:12:47 Vyacheslav Tyrtov wrote:
 From: Tarek Dakhran t.dakh...@samsung.com
 
 The EXYNOS5410 clocks are statically listed and registered
 using the Samsung specific common clock helper functions.

Thanks for keeping up with addressing the comments. However there are
still few things that need to be corrected. Please see my comments inline.

 Signed-off-by: Tarek Dakhran t.dakh...@samsung.com
 Signed-off-by: Vyacheslav Tyrtov v.tyr...@samsung.com
 ---
  .../devicetree/bindings/clock/exynos5410-clock.txt |  37 
  drivers/clk/samsung/Makefile   |   1 +
  drivers/clk/samsung/clk-exynos5410.c   | 239 
 +
  include/dt-bindings/clock/exynos5410.h | 175 +++
  4 files changed, 452 insertions(+)
  create mode 100644 
 Documentation/devicetree/bindings/clock/exynos5410-clock.txt
  create mode 100644 drivers/clk/samsung/clk-exynos5410.c
  create mode 100644 include/dt-bindings/clock/exynos5410.h
 
 diff --git a/Documentation/devicetree/bindings/clock/exynos5410-clock.txt 
 b/Documentation/devicetree/bindings/clock/exynos5410-clock.txt
 new file mode 100644
 index 000..a462da231
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/clock/exynos5410-clock.txt
 @@ -0,0 +1,37 @@
 +* Samsung Exynos5410 Clock Controller
 +
 +The Exynos5410 clock controller generates and supplies clock to various
 +controllers within the Exynos5410 SoC.
 +
 +Required Properties:
 +
 +- compatible: should be samsung,exynos5410-clock
 +
 +- reg: physical base address of the controller and length of memory mapped
 +  region.
 +
 +- #clock-cells: should be 1.

If there are any external clocks that need to be provided (and I believe
there are), you should mention them in the documentation.

Also, to make this more future-proof, I would add clock-names and clocks
properties to the binding, listing all those external clocks. It's just
about the binding definition - you don't have to implement this in the
driver yet, as we don't have the framework to handle this at early system
initialization. However, when we finally implement this in the Common
Clock Framework, we will not have to change existing DT bindings.

 +
 +All available clocks are defined as preprocessor macros in
 +dt-bindings/clock/exynos5410.h header and can be used in device
 +tree sources.
 +
 +Example 1: An example of a clock controller node is listed below.
 +
 + clock: clock-controller@0x1001 {
 + compatible = samsung,exynos5410-clock;
 + reg = 0x1001 0x3;
 + #clock-cells = 1;
 + };
 +
 +Example 2: UART controller node that consumes the clock generated by the 
 clock
 +controller. Refer to the standard clock bindings for information
 +about 'clocks' and 'clock-names' property.
 +
 + serial@12C2 {
 + compatible = samsung,exynos4210-uart;
 + reg = 0x12C0 0x100;
 + interrupts = 0 51 0;
 + clocks = clock CLK_UART0, clock CLK_SCLK_UART0;
 + clock-names = uart, clk_uart_baud0;
 + };
[snip]
 diff --git a/include/dt-bindings/clock/exynos5410.h 
 b/include/dt-bindings/clock/exynos5410.h
 new file mode 100644
 index 000..9b4a58b
 --- /dev/null
 +++ b/include/dt-bindings/clock/exynos5410.h
 @@ -0,0 +1,175 @@
 +#ifndef _DT_BINDINGS_CLOCK_EXYNOS_5410_H
 +#define _DT_BINDINGS_CLOCK_EXYNOS_5410_H
 +
 +/* core clocks */
 +#define CLK_FIN_PLL 1

This is external clock, not provided by this clock controller, isn't it?

 +#define CLK_FOUT_APLL 2
 +#define CLK_FOUT_CPLL 3
 +#define CLK_FOUT_DPLL 4
 +#define CLK_FOUT_EPLL 5
[snip]
 +#define CLK_ACLK_G3D 500
 +#define CLK_G3D 501
 +#define CLK_SMMU_MIXER 502
 +
 +/* mux clocks */
 +#define CLK_MOUT_HDMI 640

This definition does not seem to be used anywhere in the driver itself.

 +
 +/* divider clocks */
 +#define CLK_DOUT_PIXEL 768

Ditto.

Please don't define IDs for clocks that are not yet provided by the
driver.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[PATCH v3 2/4] clk: exynos5410: register clocks using common clock framework

2013-11-07 Thread Vyacheslav Tyrtov
From: Tarek Dakhran 

The EXYNOS5410 clocks are statically listed and registered
using the Samsung specific common clock helper functions.

Signed-off-by: Tarek Dakhran 
Signed-off-by: Vyacheslav Tyrtov 
---
 .../devicetree/bindings/clock/exynos5410-clock.txt |  37 
 drivers/clk/samsung/Makefile   |   1 +
 drivers/clk/samsung/clk-exynos5410.c   | 239 +
 include/dt-bindings/clock/exynos5410.h | 175 +++
 4 files changed, 452 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/exynos5410-clock.txt
 create mode 100644 drivers/clk/samsung/clk-exynos5410.c
 create mode 100644 include/dt-bindings/clock/exynos5410.h

diff --git a/Documentation/devicetree/bindings/clock/exynos5410-clock.txt 
b/Documentation/devicetree/bindings/clock/exynos5410-clock.txt
new file mode 100644
index 000..a462da231
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/exynos5410-clock.txt
@@ -0,0 +1,37 @@
+* Samsung Exynos5410 Clock Controller
+
+The Exynos5410 clock controller generates and supplies clock to various
+controllers within the Exynos5410 SoC.
+
+Required Properties:
+
+- compatible: should be "samsung,exynos5410-clock"
+
+- reg: physical base address of the controller and length of memory mapped
+  region.
+
+- #clock-cells: should be 1.
+
+All available clocks are defined as preprocessor macros in
+dt-bindings/clock/exynos5410.h header and can be used in device
+tree sources.
+
+Example 1: An example of a clock controller node is listed below.
+
+   clock: clock-controller@0x1001 {
+   compatible = "samsung,exynos5410-clock";
+   reg = <0x1001 0x3>;
+   #clock-cells = <1>;
+   };
+
+Example 2: UART controller node that consumes the clock generated by the clock
+  controller. Refer to the standard clock bindings for information
+  about 'clocks' and 'clock-names' property.
+
+   serial@12C2 {
+   compatible = "samsung,exynos4210-uart";
+   reg = <0x12C0 0x100>;
+   interrupts = <0 51 0>;
+   clocks = < CLK_UART0>, < CLK_SCLK_UART0>;
+   clock-names = "uart", "clk_uart_baud0";
+   };
diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
index 3413380..5a446ca 100644
--- a/drivers/clk/samsung/Makefile
+++ b/drivers/clk/samsung/Makefile
@@ -5,6 +5,7 @@
 obj-$(CONFIG_COMMON_CLK)   += clk.o clk-pll.o
 obj-$(CONFIG_ARCH_EXYNOS4) += clk-exynos4.o
 obj-$(CONFIG_SOC_EXYNOS5250)   += clk-exynos5250.o
+obj-$(CONFIG_SOC_EXYNOS5410)   += clk-exynos5410.o
 obj-$(CONFIG_SOC_EXYNOS5420)   += clk-exynos5420.o
 obj-$(CONFIG_SOC_EXYNOS5440)   += clk-exynos5440.o
 obj-$(CONFIG_ARCH_EXYNOS)  += clk-exynos-audss.o
diff --git a/drivers/clk/samsung/clk-exynos5410.c 
b/drivers/clk/samsung/clk-exynos5410.c
new file mode 100644
index 000..33d8c8c
--- /dev/null
+++ b/drivers/clk/samsung/clk-exynos5410.c
@@ -0,0 +1,239 @@
+/*
+ * Copyright (c) 2013 Samsung Electronics Co., Ltd.
+ * Author: Tarek Dakhran 
+ *
+ * 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.
+ *
+ * Common Clock Framework support for Exynos5410 SoC.
+*/
+
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "clk.h"
+
+#define APLL_LOCK   0x0
+#define APLL_CON0   0x100
+#define CPLL_LOCK   0x10020
+#define CPLL_CON0   0x10120
+#define MPLL_LOCK   0x4000
+#define MPLL_CON0   0x4100
+#define BPLL_LOCK   0x20010
+#define BPLL_CON0   0x20110
+#define KPLL_LOCK   0x28000
+#define KPLL_CON0   0x28100
+
+#define SRC_CPU0x200
+#define DIV_CPU0   0x500
+#define SRC_CPERI1 0x4204
+#define DIV_TOP0   0x10510
+#define DIV_TOP1   0x10514
+#define DIV_FSYS1  0x1054c
+#define DIV_FSYS2  0x10550
+#define DIV_PERIC0 0x10558
+#define SRC_TOP0   0x10210
+#define SRC_TOP1   0x10214
+#define SRC_TOP2   0x10218
+#define SRC_FSYS   0x10244
+#define SRC_PERIC0 0x10250
+#define SRC_MASK_FSYS  0x10340
+#define SRC_MASK_PERIC00x10350
+#define GATE_BUS_FSYS0 0x10740
+#define GATE_IP_FSYS   0x10944
+#define GATE_IP_PERIC  0x10950
+#define GATE_IP_PERIS  0x10960
+#define SRC_CDREX  0x20200
+#define SRC_KFC0x28200
+#define DIV_KFC0   0x28500
+
+/* list of PLLs */
+enum exynos5410_plls {
+   apll, cpll, mpll,
+   bpll, kpll,
+   nr_plls /* number of PLLs */
+};
+
+/*
+ * list of controller registers to be saved and restored during a
+ * suspend/resume 

[PATCH v3 2/4] clk: exynos5410: register clocks using common clock framework

2013-11-07 Thread Vyacheslav Tyrtov
From: Tarek Dakhran t.dakh...@samsung.com

The EXYNOS5410 clocks are statically listed and registered
using the Samsung specific common clock helper functions.

Signed-off-by: Tarek Dakhran t.dakh...@samsung.com
Signed-off-by: Vyacheslav Tyrtov v.tyr...@samsung.com
---
 .../devicetree/bindings/clock/exynos5410-clock.txt |  37 
 drivers/clk/samsung/Makefile   |   1 +
 drivers/clk/samsung/clk-exynos5410.c   | 239 +
 include/dt-bindings/clock/exynos5410.h | 175 +++
 4 files changed, 452 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/exynos5410-clock.txt
 create mode 100644 drivers/clk/samsung/clk-exynos5410.c
 create mode 100644 include/dt-bindings/clock/exynos5410.h

diff --git a/Documentation/devicetree/bindings/clock/exynos5410-clock.txt 
b/Documentation/devicetree/bindings/clock/exynos5410-clock.txt
new file mode 100644
index 000..a462da231
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/exynos5410-clock.txt
@@ -0,0 +1,37 @@
+* Samsung Exynos5410 Clock Controller
+
+The Exynos5410 clock controller generates and supplies clock to various
+controllers within the Exynos5410 SoC.
+
+Required Properties:
+
+- compatible: should be samsung,exynos5410-clock
+
+- reg: physical base address of the controller and length of memory mapped
+  region.
+
+- #clock-cells: should be 1.
+
+All available clocks are defined as preprocessor macros in
+dt-bindings/clock/exynos5410.h header and can be used in device
+tree sources.
+
+Example 1: An example of a clock controller node is listed below.
+
+   clock: clock-controller@0x1001 {
+   compatible = samsung,exynos5410-clock;
+   reg = 0x1001 0x3;
+   #clock-cells = 1;
+   };
+
+Example 2: UART controller node that consumes the clock generated by the clock
+  controller. Refer to the standard clock bindings for information
+  about 'clocks' and 'clock-names' property.
+
+   serial@12C2 {
+   compatible = samsung,exynos4210-uart;
+   reg = 0x12C0 0x100;
+   interrupts = 0 51 0;
+   clocks = clock CLK_UART0, clock CLK_SCLK_UART0;
+   clock-names = uart, clk_uart_baud0;
+   };
diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
index 3413380..5a446ca 100644
--- a/drivers/clk/samsung/Makefile
+++ b/drivers/clk/samsung/Makefile
@@ -5,6 +5,7 @@
 obj-$(CONFIG_COMMON_CLK)   += clk.o clk-pll.o
 obj-$(CONFIG_ARCH_EXYNOS4) += clk-exynos4.o
 obj-$(CONFIG_SOC_EXYNOS5250)   += clk-exynos5250.o
+obj-$(CONFIG_SOC_EXYNOS5410)   += clk-exynos5410.o
 obj-$(CONFIG_SOC_EXYNOS5420)   += clk-exynos5420.o
 obj-$(CONFIG_SOC_EXYNOS5440)   += clk-exynos5440.o
 obj-$(CONFIG_ARCH_EXYNOS)  += clk-exynos-audss.o
diff --git a/drivers/clk/samsung/clk-exynos5410.c 
b/drivers/clk/samsung/clk-exynos5410.c
new file mode 100644
index 000..33d8c8c
--- /dev/null
+++ b/drivers/clk/samsung/clk-exynos5410.c
@@ -0,0 +1,239 @@
+/*
+ * Copyright (c) 2013 Samsung Electronics Co., Ltd.
+ * Author: Tarek Dakhran t.dakh...@samsung.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.
+ *
+ * Common Clock Framework support for Exynos5410 SoC.
+*/
+
+#include dt-bindings/clock/exynos5410.h
+
+#include linux/clk.h
+#include linux/clkdev.h
+#include linux/clk-provider.h
+#include linux/of.h
+#include linux/of_address.h
+
+#include clk.h
+
+#define APLL_LOCK   0x0
+#define APLL_CON0   0x100
+#define CPLL_LOCK   0x10020
+#define CPLL_CON0   0x10120
+#define MPLL_LOCK   0x4000
+#define MPLL_CON0   0x4100
+#define BPLL_LOCK   0x20010
+#define BPLL_CON0   0x20110
+#define KPLL_LOCK   0x28000
+#define KPLL_CON0   0x28100
+
+#define SRC_CPU0x200
+#define DIV_CPU0   0x500
+#define SRC_CPERI1 0x4204
+#define DIV_TOP0   0x10510
+#define DIV_TOP1   0x10514
+#define DIV_FSYS1  0x1054c
+#define DIV_FSYS2  0x10550
+#define DIV_PERIC0 0x10558
+#define SRC_TOP0   0x10210
+#define SRC_TOP1   0x10214
+#define SRC_TOP2   0x10218
+#define SRC_FSYS   0x10244
+#define SRC_PERIC0 0x10250
+#define SRC_MASK_FSYS  0x10340
+#define SRC_MASK_PERIC00x10350
+#define GATE_BUS_FSYS0 0x10740
+#define GATE_IP_FSYS   0x10944
+#define GATE_IP_PERIC  0x10950
+#define GATE_IP_PERIS  0x10960
+#define SRC_CDREX  0x20200
+#define SRC_KFC0x28200
+#define DIV_KFC0   0x28500
+
+/* list of PLLs */
+enum exynos5410_plls {
+   apll, cpll,