diff mbox

[v5] clk: add MOXA ART SoCs clock driver

Message ID 1374221837-23773-1-git-send-email-jonas.jensen@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jonas Jensen July 19, 2013, 8:17 a.m. UTC 
This patch adds MOXA ART SoCs clock driver support.

Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
---

Notes:
    Changes since v4:
    
    1. remove unnecessary header includes
    
    Applies to next-20130716

 .../bindings/clock/moxa,moxart-core-clock.txt      | 23 ++++++++
 drivers/clk/Makefile                               |  1 +
 drivers/clk/clk-moxart.c                           | 69 ++++++++++++++++++++++
 3 files changed, 93 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/moxa,moxart-core-clock.txt
 create mode 100644 drivers/clk/clk-moxart.c

Comments

Mark Rutland July 22, 2013, 9:21 a.m. UTC | #1 
On Fri, Jul 19, 2013 at 09:17:17AM +0100, Jonas Jensen wrote:
> This patch adds MOXA ART SoCs clock driver support.
> 
> Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
> ---
> 
> Notes:
>     Changes since v4:
>     
>     1. remove unnecessary header includes
>     
>     Applies to next-20130716
> 
>  .../bindings/clock/moxa,moxart-core-clock.txt      | 23 ++++++++
>  drivers/clk/Makefile                               |  1 +
>  drivers/clk/clk-moxart.c                           | 69 ++++++++++++++++++++++
>  3 files changed, 93 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/moxa,moxart-core-clock.txt
>  create mode 100644 drivers/clk/clk-moxart.c
> 
> diff --git a/Documentation/devicetree/bindings/clock/moxa,moxart-core-clock.txt b/Documentation/devicetree/bindings/clock/moxa,moxart-core-clock.txt
> new file mode 100644
> index 0000000..528c8b0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/moxa,moxart-core-clock.txt
> @@ -0,0 +1,23 @@
> +Device Tree Clock bindings for arch-moxart
> +
> +This binding uses the common clock binding[1].
> +
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +MOXA ART SoCs allow to determine core clock frequencies by reading
> +a register.
> +
> +Required properties:
> +- compatible : Should be "moxa,moxart-core-clock"
> +- #clock-cells : Should be 0
> +- reg : Should contain registers location and length
> +- clock-output-names : Should be "coreclk"
> +
> +For example:
> +
> +	coreclk: core-clock@98100000 {
> +		compatible = "moxa,moxart-core-clock";
> +		#clock-cells = <0>;
> +		reg = <0x98100000 0x34>;
> +		clock-output-names = "coreclk";
> +	};
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 4038c2b..933622f 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_COMMON_CLK)	+= clk-composite.o
>  
>  # SoCs specific
>  obj-$(CONFIG_ARCH_BCM2835)	+= clk-bcm2835.o
> +obj-$(CONFIG_ARCH_MOXART)	+= clk-moxart.o
>  obj-$(CONFIG_ARCH_NOMADIK)	+= clk-nomadik.o
>  obj-$(CONFIG_ARCH_HIGHBANK)	+= clk-highbank.o
>  obj-$(CONFIG_ARCH_NSPIRE)	+= clk-nspire.o
> diff --git a/drivers/clk/clk-moxart.c b/drivers/clk/clk-moxart.c
> new file mode 100644
> index 0000000..5559371
> --- /dev/null
> +++ b/drivers/clk/clk-moxart.c
> @@ -0,0 +1,69 @@
> +/*
> + * MOXA ART SoCs clock driver.
> + *
> + * Copyright (C) 2013 Jonas Jensen
> + *
> + * Jonas Jensen <jonas.jensen@gmail.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2.  This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/io.h>
> +#include <linux/of_address.h>
> +#include <linux/clkdev.h>
> +
> +void __init moxart_of_clk_init(struct device_node *node)
> +{
> +	static void __iomem *base;
> +	struct clk *clk;
> +	unsigned long rate;
> +	unsigned int mul, val, div;
> +	const char *name;
> +
> +	base = of_iomap(node, 0);
> +	if (IS_ERR(base))
> +		panic("%s: of_iomap failed\n", node->full_name);

As far as I'm aware, of_iomap returns NULL, not an ERR_PTR value.

Also, is it necessary to panic() ? In v1 of the patch you used pr_err...

Thanks,
Mark.

> +
> +	mul = (readl(base + 0x30) >> 3) & 0x1ff;
> +	val = (readl(base + 0x0c) >> 4) & 0x7;
> +
> +	switch (val) {
> +	case 1:
> +		div = 3;
> +		break;
> +	case 2:
> +		div = 4;
> +		break;
> +	case 3:
> +		div = 6;
> +		break;
> +	case 4:
> +		div = 8;
> +		break;
> +	default:
> +		div = 2;
> +		break;
> +	}
> +
> +	/*
> +	 * the rate calculation below is only tested and proven
> +	 * to be true for UC-7112-LX
> +	 *
> +	 * UC-7112-LX: mul=80 val=0
> +	 *
> +	 * to support other moxart SoC hardware, this may need
> +	 * a change, though it's possible it works there too
> +	 */
> +	rate = (mul * 1200000 / div);
> +
> +	of_property_read_string(node, "clock-output-names", &name);
> +	clk = clk_register_fixed_rate(NULL, name, NULL, CLK_IS_ROOT, rate);
> +	clk_register_clkdev(clk, NULL, name);
> +	of_clk_add_provider(node, of_clk_src_simple_get, clk);
> +
> +	iounmap(base);
> +}
> +CLK_OF_DECLARE(moxart_core_clock, "moxa,moxart-core-clock", moxart_of_clk_init);
> -- 
> 1.8.2.1
> 
>
Tomasz Figa July 23, 2013, 8:09 a.m. UTC | #2 
On Monday 22 of July 2013 10:21:38 Mark Rutland wrote:
> On Fri, Jul 19, 2013 at 09:17:17AM +0100, Jonas Jensen wrote:
> > This patch adds MOXA ART SoCs clock driver support.
> > 
> > Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
> > ---
> > 
> > Notes:
> >     Changes since v4:
> >     
> >     1. remove unnecessary header includes
> >     
> >     Applies to next-20130716
> >  
> >  .../bindings/clock/moxa,moxart-core-clock.txt      | 23 ++++++++
> >  drivers/clk/Makefile                               |  1 +
> >  drivers/clk/clk-moxart.c                           | 69
> >  ++++++++++++++++++++++ 3 files changed, 93 insertions(+)
> >  create mode 100644
> >  Documentation/devicetree/bindings/clock/moxa,moxart-core-clock.txt
> >  create mode 100644 drivers/clk/clk-moxart.c
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/clock/moxa,moxart-core-clock.txt
> > b/Documentation/devicetree/bindings/clock/moxa,moxart-core-clock.txt
> > new file mode 100644
> > index 0000000..528c8b0
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/clock/moxa,moxart-core-clock.txt
> > @@ -0,0 +1,23 @@
> > +Device Tree Clock bindings for arch-moxart
> > +
> > +This binding uses the common clock binding[1].
> > +
> > +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> > +
> > +MOXA ART SoCs allow to determine core clock frequencies by reading
> > +a register.
> > +
> > +Required properties:
> > +- compatible : Should be "moxa,moxart-core-clock"
> > +- #clock-cells : Should be 0
> > +- reg : Should contain registers location and length
> > +- clock-output-names : Should be "coreclk"
> > +
> > +For example:
> > +
> > +	coreclk: core-clock@98100000 {
> > +		compatible = "moxa,moxart-core-clock";
> > +		#clock-cells = <0>;
> > +		reg = <0x98100000 0x34>;
> > +		clock-output-names = "coreclk";
> > +	};
> > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> > index 4038c2b..933622f 100644
> > --- a/drivers/clk/Makefile
> > +++ b/drivers/clk/Makefile
> > @@ -11,6 +11,7 @@ obj-$(CONFIG_COMMON_CLK)	+= clk-composite.o
> > 
> >  # SoCs specific
> >  obj-$(CONFIG_ARCH_BCM2835)	+= clk-bcm2835.o
> > 
> > +obj-$(CONFIG_ARCH_MOXART)	+= clk-moxart.o
> > 
> >  obj-$(CONFIG_ARCH_NOMADIK)	+= clk-nomadik.o
> >  obj-$(CONFIG_ARCH_HIGHBANK)	+= clk-highbank.o
> >  obj-$(CONFIG_ARCH_NSPIRE)	+= clk-nspire.o
> > 
> > diff --git a/drivers/clk/clk-moxart.c b/drivers/clk/clk-moxart.c
> > new file mode 100644
> > index 0000000..5559371
> > --- /dev/null
> > +++ b/drivers/clk/clk-moxart.c
> > @@ -0,0 +1,69 @@
> > +/*
> > + * MOXA ART SoCs clock driver.
> > + *
> > + * Copyright (C) 2013 Jonas Jensen
> > + *
> > + * Jonas Jensen <jonas.jensen@gmail.com>
> > + *
> > + * This file is licensed under the terms of the GNU General Public
> > + * License version 2.  This program is licensed "as is" without any
> > + * warranty of any kind, whether express or implied.
> > + */
> > +
> > +#include <linux/clk-provider.h>
> > +#include <linux/io.h>
> > +#include <linux/of_address.h>
> > +#include <linux/clkdev.h>
> > +
> > +void __init moxart_of_clk_init(struct device_node *node)
> > +{
> > +	static void __iomem *base;
> > +	struct clk *clk;
> > +	unsigned long rate;
> > +	unsigned int mul, val, div;
> > +	const char *name;
> > +
> > +	base = of_iomap(node, 0);
> > +	if (IS_ERR(base))
> > +		panic("%s: of_iomap failed\n", node->full_name);
> 
> As far as I'm aware, of_iomap returns NULL, not an ERR_PTR value.
> 
> Also, is it necessary to panic() ? In v1 of the patch you used pr_err...

This is a good question in general, not only for this driver.

IMHO if you know early that something is really wrong, to the point that 
the system won't be able to work normally (and failing to initialize 
clocks looks like it) it is better to let the user/developer know that 
something is utterly wrong at this point without letting the system run 
for a bit more and fail anyway with a harder to track down error message.

It's not so obvious, though, what kinds of failure should be considered 
critical. IMHO any failure in low level initialization code, but this is 
debatable, I guess.

Best regards,
Tomasz
Mike Turquette July 26, 2013, 10:32 p.m. UTC | #3 
Quoting Tomasz Figa (2013-07-23 01:09:06)
> On Monday 22 of July 2013 10:21:38 Mark Rutland wrote:
> > On Fri, Jul 19, 2013 at 09:17:17AM +0100, Jonas Jensen wrote:
> > > This patch adds MOXA ART SoCs clock driver support.
> > > 
> > > Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
> > > ---
> > > 
> > > Notes:
> > >     Changes since v4:
> > >     
> > >     1. remove unnecessary header includes
> > >     
> > >     Applies to next-20130716
> > >  
> > >  .../bindings/clock/moxa,moxart-core-clock.txt      | 23 ++++++++
> > >  drivers/clk/Makefile                               |  1 +
> > >  drivers/clk/clk-moxart.c                           | 69
> > >  ++++++++++++++++++++++ 3 files changed, 93 insertions(+)
> > >  create mode 100644
> > >  Documentation/devicetree/bindings/clock/moxa,moxart-core-clock.txt
> > >  create mode 100644 drivers/clk/clk-moxart.c
> > > 
> > > diff --git
> > > a/Documentation/devicetree/bindings/clock/moxa,moxart-core-clock.txt
> > > b/Documentation/devicetree/bindings/clock/moxa,moxart-core-clock.txt
> > > new file mode 100644
> > > index 0000000..528c8b0
> > > --- /dev/null
> > > +++
> > > b/Documentation/devicetree/bindings/clock/moxa,moxart-core-clock.txt
> > > @@ -0,0 +1,23 @@
> > > +Device Tree Clock bindings for arch-moxart
> > > +
> > > +This binding uses the common clock binding[1].
> > > +
> > > +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > +
> > > +MOXA ART SoCs allow to determine core clock frequencies by reading
> > > +a register.
> > > +
> > > +Required properties:
> > > +- compatible : Should be "moxa,moxart-core-clock"
> > > +- #clock-cells : Should be 0
> > > +- reg : Should contain registers location and length
> > > +- clock-output-names : Should be "coreclk"
> > > +
> > > +For example:
> > > +
> > > +   coreclk: core-clock@98100000 {
> > > +           compatible = "moxa,moxart-core-clock";
> > > +           #clock-cells = <0>;
> > > +           reg = <0x98100000 0x34>;
> > > +           clock-output-names = "coreclk";
> > > +   };
> > > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> > > index 4038c2b..933622f 100644
> > > --- a/drivers/clk/Makefile
> > > +++ b/drivers/clk/Makefile
> > > @@ -11,6 +11,7 @@ obj-$(CONFIG_COMMON_CLK)  += clk-composite.o
> > > 
> > >  # SoCs specific
> > >  obj-$(CONFIG_ARCH_BCM2835) += clk-bcm2835.o
> > > 
> > > +obj-$(CONFIG_ARCH_MOXART)  += clk-moxart.o
> > > 
> > >  obj-$(CONFIG_ARCH_NOMADIK) += clk-nomadik.o
> > >  obj-$(CONFIG_ARCH_HIGHBANK)        += clk-highbank.o
> > >  obj-$(CONFIG_ARCH_NSPIRE)  += clk-nspire.o
> > > 
> > > diff --git a/drivers/clk/clk-moxart.c b/drivers/clk/clk-moxart.c
> > > new file mode 100644
> > > index 0000000..5559371
> > > --- /dev/null
> > > +++ b/drivers/clk/clk-moxart.c
> > > @@ -0,0 +1,69 @@
> > > +/*
> > > + * MOXA ART SoCs clock driver.
> > > + *
> > > + * Copyright (C) 2013 Jonas Jensen
> > > + *
> > > + * Jonas Jensen <jonas.jensen@gmail.com>
> > > + *
> > > + * This file is licensed under the terms of the GNU General Public
> > > + * License version 2.  This program is licensed "as is" without any
> > > + * warranty of any kind, whether express or implied.
> > > + */
> > > +
> > > +#include <linux/clk-provider.h>
> > > +#include <linux/io.h>
> > > +#include <linux/of_address.h>
> > > +#include <linux/clkdev.h>
> > > +
> > > +void __init moxart_of_clk_init(struct device_node *node)
> > > +{
> > > +   static void __iomem *base;
> > > +   struct clk *clk;
> > > +   unsigned long rate;
> > > +   unsigned int mul, val, div;
> > > +   const char *name;
> > > +
> > > +   base = of_iomap(node, 0);
> > > +   if (IS_ERR(base))
> > > +           panic("%s: of_iomap failed\n", node->full_name);
> > 
> > As far as I'm aware, of_iomap returns NULL, not an ERR_PTR value.
> > 
> > Also, is it necessary to panic() ? In v1 of the patch you used pr_err...
> 
> This is a good question in general, not only for this driver.
> 
> IMHO if you know early that something is really wrong, to the point that 
> the system won't be able to work normally (and failing to initialize 
> clocks looks like it) it is better to let the user/developer know that 
> something is utterly wrong at this point without letting the system run 
> for a bit more and fail anyway with a harder to track down error message.

You can let the user/developer know that something is utterly wrong with
a WARN.

Where panics and BUGs come back to haunt you is when you are bringing up
a new platform or SoC that uses this clock driver, but this clock driver
fails pending some subtle change for the new platform/SoC.

But perhaps your bootloader at least set the clocks up for you sensibly
and your other device drivers might bail out due to not having clocks
but at least you could boot your new system.  But putting a panic/BUG
here makes the above scenario impossible.

> 
> It's not so obvious, though, what kinds of failure should be considered 
> critical. IMHO any failure in low level initialization code, but this is 
> debatable, I guess.

Agreed that's it's hard to know when to use them. But if you can get
away with a WARN then I always err on the cautious side and try to
prevent panicing the system explicitly.

Regards,
Mike

> 
> Best regards,
> Tomasz
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/clock/moxa,moxart-core-clock.txt b/Documentation/devicetree/bindings/clock/moxa,moxart-core-clock.txt
new file mode 100644
index 0000000..528c8b0
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/moxa,moxart-core-clock.txt
@@ -0,0 +1,23 @@ 
+Device Tree Clock bindings for arch-moxart
+
+This binding uses the common clock binding[1].
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+MOXA ART SoCs allow to determine core clock frequencies by reading
+a register.
+
+Required properties:
+- compatible : Should be "moxa,moxart-core-clock"
+- #clock-cells : Should be 0
+- reg : Should contain registers location and length
+- clock-output-names : Should be "coreclk"
+
+For example:
+
+	coreclk: core-clock@98100000 {
+		compatible = "moxa,moxart-core-clock";
+		#clock-cells = <0>;
+		reg = <0x98100000 0x34>;
+		clock-output-names = "coreclk";
+	};
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 4038c2b..933622f 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -11,6 +11,7 @@  obj-$(CONFIG_COMMON_CLK)	+= clk-composite.o
 
 # SoCs specific
 obj-$(CONFIG_ARCH_BCM2835)	+= clk-bcm2835.o
+obj-$(CONFIG_ARCH_MOXART)	+= clk-moxart.o
 obj-$(CONFIG_ARCH_NOMADIK)	+= clk-nomadik.o
 obj-$(CONFIG_ARCH_HIGHBANK)	+= clk-highbank.o
 obj-$(CONFIG_ARCH_NSPIRE)	+= clk-nspire.o
diff --git a/drivers/clk/clk-moxart.c b/drivers/clk/clk-moxart.c
new file mode 100644
index 0000000..5559371
--- /dev/null
+++ b/drivers/clk/clk-moxart.c
@@ -0,0 +1,69 @@ 
+/*
+ * MOXA ART SoCs clock driver.
+ *
+ * Copyright (C) 2013 Jonas Jensen
+ *
+ * Jonas Jensen <jonas.jensen@gmail.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/io.h>
+#include <linux/of_address.h>
+#include <linux/clkdev.h>
+
+void __init moxart_of_clk_init(struct device_node *node)
+{
+	static void __iomem *base;
+	struct clk *clk;
+	unsigned long rate;
+	unsigned int mul, val, div;
+	const char *name;
+
+	base = of_iomap(node, 0);
+	if (IS_ERR(base))
+		panic("%s: of_iomap failed\n", node->full_name);
+
+	mul = (readl(base + 0x30) >> 3) & 0x1ff;
+	val = (readl(base + 0x0c) >> 4) & 0x7;
+
+	switch (val) {
+	case 1:
+		div = 3;
+		break;
+	case 2:
+		div = 4;
+		break;
+	case 3:
+		div = 6;
+		break;
+	case 4:
+		div = 8;
+		break;
+	default:
+		div = 2;
+		break;
+	}
+
+	/*
+	 * the rate calculation below is only tested and proven
+	 * to be true for UC-7112-LX
+	 *
+	 * UC-7112-LX: mul=80 val=0
+	 *
+	 * to support other moxart SoC hardware, this may need
+	 * a change, though it's possible it works there too
+	 */
+	rate = (mul * 1200000 / div);
+
+	of_property_read_string(node, "clock-output-names", &name);
+	clk = clk_register_fixed_rate(NULL, name, NULL, CLK_IS_ROOT, rate);
+	clk_register_clkdev(clk, NULL, name);
+	of_clk_add_provider(node, of_clk_src_simple_get, clk);
+
+	iounmap(base);
+}
+CLK_OF_DECLARE(moxart_core_clock, "moxa,moxart-core-clock", moxart_of_clk_init);