Re: [PATCH v12 17/21] h8300: clock driver

2015-08-07 Thread Michael Turquette
Hello Sato-san,

Unfortunately this patch did not Cc myself, Stephen Boyd or the
linux-...@vger.kernel.org mailing list. As such Stephen and I did not
have a chance to review it. Even more unfortunate was that it was ninja
merged by maintainers without our ack. :-/

Quoting Yoshinori Sato (2015-05-10 23:26:36)
> Signed-off-by: Yoshinori Sato 
> ---
>  .../bindings/clock/renesas,h8300-div-clock.txt |  24 
>  .../bindings/clock/renesas,h8s2678-pll-clock.txt   |  23 
>  drivers/clk/Makefile   |   1 +
>  drivers/clk/h8300/Makefile |   2 +
>  drivers/clk/h8300/clk-div.c|  53 
>  drivers/clk/h8300/clk-h8s2678.c| 147 
> +
>  6 files changed, 250 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/clock/renesas,h8300-div-clock.txt
>  create mode 100644 
> Documentation/devicetree/bindings/clock/renesas,h8s2678-pll-clock.txt
>  create mode 100644 drivers/clk/h8300/Makefile
>  create mode 100644 drivers/clk/h8300/clk-div.c
>  create mode 100644 drivers/clk/h8300/clk-h8s2678.c
> 
> diff --git 
> a/Documentation/devicetree/bindings/clock/renesas,h8300-div-clock.txt 
> b/Documentation/devicetree/bindings/clock/renesas,h8300-div-clock.txt
> new file mode 100644
> index 000..36c2b52
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/renesas,h8300-div-clock.txt
> @@ -0,0 +1,24 @@
> +* Renesas H8/300 divider clock
> +
> +Required Properties:
> +
> +  - compatible: Must be "renesas,sh73a0-h8300-div-clock"
> +
> +  - clocks: Reference to the parent clocks ("extal1" and "extal2")
> +
> +  - #clock-cells: Must be 1
> +
> +  - reg: Base address and length of the divide rate selector
> +
> +  - renesas,width: bit width of selector
> +
> +Example
> +---
> +
> +   cclk: cclk {
> +   compatible = "renesas,h8300-div-clock";
> +   clocks = <>;
> +   #clock-cells = <0>;
> +   reg = <0xfee01b 2>;
> +   renesas,width = <2>;
> +   };

I could not find any info on this clock in the H8S/2678 reference
manual[0]. Could you point me to the right documentation?

> diff --git 
> a/Documentation/devicetree/bindings/clock/renesas,h8s2678-pll-clock.txt 
> b/Documentation/devicetree/bindings/clock/renesas,h8s2678-pll-clock.txt
> new file mode 100644
> index 000..500cdadb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/renesas,h8s2678-pll-clock.txt
> @@ -0,0 +1,23 @@
> +Renesas H8S2678 PLL clock
> +
> +This device is Clock multiplyer
> +
> +Required Properties:
> +
> +  - compatible: Must be "renesas,h8s2678-pll-clock"
> +
> +  - clocks: Reference to the parent clocks
> +
> +  - #clock-cells: Must be 0
> +
> +  - reg: Two rate selector (Multiply / Divide) register address
> +
> +Example
> +---
> +
> +   pllclk: pllclk {
> +   compatible = "renesas,h8s2678-pll-clock";
> +   clocks = <>;
> +   #clock-cells = <0>;
> +   reg = <0xfee03b 2>, <0xfee045 2>;
> +   };

Is there really only one clock output? According to figure 21.1 there is
the "System clock to φ pin" output and the "Internal clock to peripheral
modules" output.

I am wondering if clock-cells should be 1 instead of zero and support
both of these output signals?

As a nitpick, I think it would have been better to name the node "cpg"
as it is listed in Section 21. pllclk is only one of the two registers
that make up the cpg. Something like:

cpg: clock-controller@fee03b

If you do decide to have clock-cells greater than zero, you might find
the following threads helpful. They describe how to craft a
clock-controller style binding:

http://lkml.kernel.org/r/<20150411001231.18916.93186@quantum>

http://lkml.kernel.org/r/<20150724034229.642.88156@quantum>

As an additional thought, it looks like the module stop registers are
mixed in with the clock registers. When you decide to write a reset
driver for these platforms you might want to re-use this existing dt
binding description and put the reset code into your clock provider
driver. Grep for reset.h in the drivers/clk/ directory for some
examples.

[0] http://documentation.renesas.com/doc/products/mpumcu/rej09b0283_2678hm.pdf

Regards,
Mike
--
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 v12 17/21] h8300: clock driver

2015-08-07 Thread Michael Turquette
Hello Sato-san,

Unfortunately this patch did not Cc myself, Stephen Boyd or the
linux-...@vger.kernel.org mailing list. As such Stephen and I did not
have a chance to review it. Even more unfortunate was that it was ninja
merged by maintainers without our ack. :-/

Quoting Yoshinori Sato (2015-05-10 23:26:36)
 Signed-off-by: Yoshinori Sato ys...@users.sourceforge.jp
 ---
  .../bindings/clock/renesas,h8300-div-clock.txt |  24 
  .../bindings/clock/renesas,h8s2678-pll-clock.txt   |  23 
  drivers/clk/Makefile   |   1 +
  drivers/clk/h8300/Makefile |   2 +
  drivers/clk/h8300/clk-div.c|  53 
  drivers/clk/h8300/clk-h8s2678.c| 147 
 +
  6 files changed, 250 insertions(+)
  create mode 100644 
 Documentation/devicetree/bindings/clock/renesas,h8300-div-clock.txt
  create mode 100644 
 Documentation/devicetree/bindings/clock/renesas,h8s2678-pll-clock.txt
  create mode 100644 drivers/clk/h8300/Makefile
  create mode 100644 drivers/clk/h8300/clk-div.c
  create mode 100644 drivers/clk/h8300/clk-h8s2678.c
 
 diff --git 
 a/Documentation/devicetree/bindings/clock/renesas,h8300-div-clock.txt 
 b/Documentation/devicetree/bindings/clock/renesas,h8300-div-clock.txt
 new file mode 100644
 index 000..36c2b52
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/clock/renesas,h8300-div-clock.txt
 @@ -0,0 +1,24 @@
 +* Renesas H8/300 divider clock
 +
 +Required Properties:
 +
 +  - compatible: Must be renesas,sh73a0-h8300-div-clock
 +
 +  - clocks: Reference to the parent clocks (extal1 and extal2)
 +
 +  - #clock-cells: Must be 1
 +
 +  - reg: Base address and length of the divide rate selector
 +
 +  - renesas,width: bit width of selector
 +
 +Example
 +---
 +
 +   cclk: cclk {
 +   compatible = renesas,h8300-div-clock;
 +   clocks = xclk;
 +   #clock-cells = 0;
 +   reg = 0xfee01b 2;
 +   renesas,width = 2;
 +   };

I could not find any info on this clock in the H8S/2678 reference
manual[0]. Could you point me to the right documentation?

 diff --git 
 a/Documentation/devicetree/bindings/clock/renesas,h8s2678-pll-clock.txt 
 b/Documentation/devicetree/bindings/clock/renesas,h8s2678-pll-clock.txt
 new file mode 100644
 index 000..500cdadb
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/clock/renesas,h8s2678-pll-clock.txt
 @@ -0,0 +1,23 @@
 +Renesas H8S2678 PLL clock
 +
 +This device is Clock multiplyer
 +
 +Required Properties:
 +
 +  - compatible: Must be renesas,h8s2678-pll-clock
 +
 +  - clocks: Reference to the parent clocks
 +
 +  - #clock-cells: Must be 0
 +
 +  - reg: Two rate selector (Multiply / Divide) register address
 +
 +Example
 +---
 +
 +   pllclk: pllclk {
 +   compatible = renesas,h8s2678-pll-clock;
 +   clocks = xclk;
 +   #clock-cells = 0;
 +   reg = 0xfee03b 2, 0xfee045 2;
 +   };

Is there really only one clock output? According to figure 21.1 there is
the System clock to φ pin output and the Internal clock to peripheral
modules output.

I am wondering if clock-cells should be 1 instead of zero and support
both of these output signals?

As a nitpick, I think it would have been better to name the node cpg
as it is listed in Section 21. pllclk is only one of the two registers
that make up the cpg. Something like:

cpg: clock-controller@fee03b

If you do decide to have clock-cells greater than zero, you might find
the following threads helpful. They describe how to craft a
clock-controller style binding:

http://lkml.kernel.org/r/20150411001231.18916.93186@quantum

http://lkml.kernel.org/r/20150724034229.642.88156@quantum

As an additional thought, it looks like the module stop registers are
mixed in with the clock registers. When you decide to write a reset
driver for these platforms you might want to re-use this existing dt
binding description and put the reset code into your clock provider
driver. Grep for reset.h in the drivers/clk/ directory for some
examples.

[0] http://documentation.renesas.com/doc/products/mpumcu/rej09b0283_2678hm.pdf

Regards,
Mike
--
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 v12 17/21] h8300: clock driver

2015-05-11 Thread Yoshinori Sato
Signed-off-by: Yoshinori Sato 
---
 .../bindings/clock/renesas,h8300-div-clock.txt |  24 
 .../bindings/clock/renesas,h8s2678-pll-clock.txt   |  23 
 drivers/clk/Makefile   |   1 +
 drivers/clk/h8300/Makefile |   2 +
 drivers/clk/h8300/clk-div.c|  53 
 drivers/clk/h8300/clk-h8s2678.c| 147 +
 6 files changed, 250 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/clock/renesas,h8300-div-clock.txt
 create mode 100644 
Documentation/devicetree/bindings/clock/renesas,h8s2678-pll-clock.txt
 create mode 100644 drivers/clk/h8300/Makefile
 create mode 100644 drivers/clk/h8300/clk-div.c
 create mode 100644 drivers/clk/h8300/clk-h8s2678.c

diff --git 
a/Documentation/devicetree/bindings/clock/renesas,h8300-div-clock.txt 
b/Documentation/devicetree/bindings/clock/renesas,h8300-div-clock.txt
new file mode 100644
index 000..36c2b52
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/renesas,h8300-div-clock.txt
@@ -0,0 +1,24 @@
+* Renesas H8/300 divider clock
+
+Required Properties:
+
+  - compatible: Must be "renesas,sh73a0-h8300-div-clock"
+
+  - clocks: Reference to the parent clocks ("extal1" and "extal2")
+
+  - #clock-cells: Must be 1
+
+  - reg: Base address and length of the divide rate selector
+
+  - renesas,width: bit width of selector
+
+Example
+---
+
+   cclk: cclk {
+   compatible = "renesas,h8300-div-clock";
+   clocks = <>;
+   #clock-cells = <0>;
+   reg = <0xfee01b 2>;
+   renesas,width = <2>;
+   };
diff --git 
a/Documentation/devicetree/bindings/clock/renesas,h8s2678-pll-clock.txt 
b/Documentation/devicetree/bindings/clock/renesas,h8s2678-pll-clock.txt
new file mode 100644
index 000..500cdadb
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/renesas,h8s2678-pll-clock.txt
@@ -0,0 +1,23 @@
+Renesas H8S2678 PLL clock
+
+This device is Clock multiplyer
+
+Required Properties:
+
+  - compatible: Must be "renesas,h8s2678-pll-clock"
+
+  - clocks: Reference to the parent clocks
+
+  - #clock-cells: Must be 0
+
+  - reg: Two rate selector (Multiply / Divide) register address
+
+Example
+---
+
+   pllclk: pllclk {
+   compatible = "renesas,h8s2678-pll-clock";
+   clocks = <>;
+   #clock-cells = <0>;
+   reg = <0xfee03b 2>, <0xfee045 2>;
+   };
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 3d00c25..9df871d 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -73,3 +73,4 @@ obj-$(CONFIG_ARCH_U8500)  += ux500/
 obj-$(CONFIG_COMMON_CLK_VERSATILE) += versatile/
 obj-$(CONFIG_X86)  += x86/
 obj-$(CONFIG_ARCH_ZYNQ)+= zynq/
+obj-$(CONFIG_H8300)+= h8300/
diff --git a/drivers/clk/h8300/Makefile b/drivers/clk/h8300/Makefile
new file mode 100644
index 000..b86427c3
--- /dev/null
+++ b/drivers/clk/h8300/Makefile
@@ -0,0 +1,2 @@
+obj-y += clk-div.o
+obj-$(CONFIG_H8S2678) += clk-h8s2678.o
diff --git a/drivers/clk/h8300/clk-div.c b/drivers/clk/h8300/clk-div.c
new file mode 100644
index 000..56f9eba
--- /dev/null
+++ b/drivers/clk/h8300/clk-div.c
@@ -0,0 +1,53 @@
+/*
+ * H8/300 divide clock driver
+ *
+ * Copyright 2015 Yoshinori Sato 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+static DEFINE_SPINLOCK(clklock);
+
+static void __init h8300_div_clk_setup(struct device_node *node)
+{
+   unsigned int num_parents;
+   struct clk *clk;
+   const char *clk_name = node->name;
+   const char *parent_name;
+   void __iomem *divcr = NULL;
+   int width;
+
+   num_parents = of_clk_get_parent_count(node);
+   if (num_parents < 1) {
+   pr_err("%s: no parent found", clk_name);
+   return;
+   }
+
+   divcr = of_iomap(node, 0);
+   if (divcr == NULL) {
+   pr_err("%s: failed to map divide register", clk_name);
+   goto error;
+   }
+
+   parent_name = of_clk_get_parent_name(node, 0);
+   of_property_read_u32(node, "renesas,width", );
+   clk = clk_register_divider(NULL, clk_name, parent_name,
+  CLK_SET_RATE_GATE, divcr, 0, width,
+  CLK_DIVIDER_POWER_OF_TWO, );
+   if (!IS_ERR(clk)) {
+   of_clk_add_provider(node, of_clk_src_simple_get, clk);
+   return;
+   }
+   pr_err("%s: failed to register %s div clock (%ld)\n",
+  __func__, clk_name, PTR_ERR(clk));
+error:
+   if (divcr)
+   iounmap(divcr);
+}
+
+CLK_OF_DECLARE(h8300_div_clk, "renesas,h8300-div-clock", h8300_div_clk_setup);
diff --git a/drivers/clk/h8300/clk-h8s2678.c b/drivers/clk/h8300/clk-h8s2678.c

[PATCH v12 17/21] h8300: clock driver

2015-05-11 Thread Yoshinori Sato
Signed-off-by: Yoshinori Sato ys...@users.sourceforge.jp
---
 .../bindings/clock/renesas,h8300-div-clock.txt |  24 
 .../bindings/clock/renesas,h8s2678-pll-clock.txt   |  23 
 drivers/clk/Makefile   |   1 +
 drivers/clk/h8300/Makefile |   2 +
 drivers/clk/h8300/clk-div.c|  53 
 drivers/clk/h8300/clk-h8s2678.c| 147 +
 6 files changed, 250 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/clock/renesas,h8300-div-clock.txt
 create mode 100644 
Documentation/devicetree/bindings/clock/renesas,h8s2678-pll-clock.txt
 create mode 100644 drivers/clk/h8300/Makefile
 create mode 100644 drivers/clk/h8300/clk-div.c
 create mode 100644 drivers/clk/h8300/clk-h8s2678.c

diff --git 
a/Documentation/devicetree/bindings/clock/renesas,h8300-div-clock.txt 
b/Documentation/devicetree/bindings/clock/renesas,h8300-div-clock.txt
new file mode 100644
index 000..36c2b52
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/renesas,h8300-div-clock.txt
@@ -0,0 +1,24 @@
+* Renesas H8/300 divider clock
+
+Required Properties:
+
+  - compatible: Must be renesas,sh73a0-h8300-div-clock
+
+  - clocks: Reference to the parent clocks (extal1 and extal2)
+
+  - #clock-cells: Must be 1
+
+  - reg: Base address and length of the divide rate selector
+
+  - renesas,width: bit width of selector
+
+Example
+---
+
+   cclk: cclk {
+   compatible = renesas,h8300-div-clock;
+   clocks = xclk;
+   #clock-cells = 0;
+   reg = 0xfee01b 2;
+   renesas,width = 2;
+   };
diff --git 
a/Documentation/devicetree/bindings/clock/renesas,h8s2678-pll-clock.txt 
b/Documentation/devicetree/bindings/clock/renesas,h8s2678-pll-clock.txt
new file mode 100644
index 000..500cdadb
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/renesas,h8s2678-pll-clock.txt
@@ -0,0 +1,23 @@
+Renesas H8S2678 PLL clock
+
+This device is Clock multiplyer
+
+Required Properties:
+
+  - compatible: Must be renesas,h8s2678-pll-clock
+
+  - clocks: Reference to the parent clocks
+
+  - #clock-cells: Must be 0
+
+  - reg: Two rate selector (Multiply / Divide) register address
+
+Example
+---
+
+   pllclk: pllclk {
+   compatible = renesas,h8s2678-pll-clock;
+   clocks = xclk;
+   #clock-cells = 0;
+   reg = 0xfee03b 2, 0xfee045 2;
+   };
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 3d00c25..9df871d 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -73,3 +73,4 @@ obj-$(CONFIG_ARCH_U8500)  += ux500/
 obj-$(CONFIG_COMMON_CLK_VERSATILE) += versatile/
 obj-$(CONFIG_X86)  += x86/
 obj-$(CONFIG_ARCH_ZYNQ)+= zynq/
+obj-$(CONFIG_H8300)+= h8300/
diff --git a/drivers/clk/h8300/Makefile b/drivers/clk/h8300/Makefile
new file mode 100644
index 000..b86427c3
--- /dev/null
+++ b/drivers/clk/h8300/Makefile
@@ -0,0 +1,2 @@
+obj-y += clk-div.o
+obj-$(CONFIG_H8S2678) += clk-h8s2678.o
diff --git a/drivers/clk/h8300/clk-div.c b/drivers/clk/h8300/clk-div.c
new file mode 100644
index 000..56f9eba
--- /dev/null
+++ b/drivers/clk/h8300/clk-div.c
@@ -0,0 +1,53 @@
+/*
+ * H8/300 divide clock driver
+ *
+ * Copyright 2015 Yoshinori Sato ys...@users.sourceforge.jp
+ */
+
+#include linux/clk.h
+#include linux/clkdev.h
+#include linux/clk-provider.h
+#include linux/err.h
+#include linux/of.h
+#include linux/of_address.h
+
+static DEFINE_SPINLOCK(clklock);
+
+static void __init h8300_div_clk_setup(struct device_node *node)
+{
+   unsigned int num_parents;
+   struct clk *clk;
+   const char *clk_name = node-name;
+   const char *parent_name;
+   void __iomem *divcr = NULL;
+   int width;
+
+   num_parents = of_clk_get_parent_count(node);
+   if (num_parents  1) {
+   pr_err(%s: no parent found, clk_name);
+   return;
+   }
+
+   divcr = of_iomap(node, 0);
+   if (divcr == NULL) {
+   pr_err(%s: failed to map divide register, clk_name);
+   goto error;
+   }
+
+   parent_name = of_clk_get_parent_name(node, 0);
+   of_property_read_u32(node, renesas,width, width);
+   clk = clk_register_divider(NULL, clk_name, parent_name,
+  CLK_SET_RATE_GATE, divcr, 0, width,
+  CLK_DIVIDER_POWER_OF_TWO, clklock);
+   if (!IS_ERR(clk)) {
+   of_clk_add_provider(node, of_clk_src_simple_get, clk);
+   return;
+   }
+   pr_err(%s: failed to register %s div clock (%ld)\n,
+  __func__, clk_name, PTR_ERR(clk));
+error:
+   if (divcr)
+   iounmap(divcr);
+}
+
+CLK_OF_DECLARE(h8300_div_clk,