[v3] clk: add MOXA ART SoCs clock driver

Commit Message

Jonas Jensen July 17, 2013, 1:23 p.m. UTC

This patch adds MOXA ART SoCs clock driver support.

Signed-off-by: Jonas Jensen <[e-mail]>
---

Notes:
    Changes since v2:
    
    1. add devicetree bindings document
    
    Applies to next-20130716

 .../bindings/clock/moxa,moxart-core-clock.txt      | 19 +++++
 drivers/clk/Makefile                               |  1 +
 drivers/clk/clk-moxart.c                           | 82 ++++++++++++++++++++++
 3 files changed, 102 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 18, 2013, 9:50 a.m. UTC | #1

On Wed, Jul 17, 2013 at 02:23:16PM +0100, Jonas Jensen wrote:
> This patch adds MOXA ART SoCs clock driver support.
> 
> Signed-off-by: Jonas Jensen <[e-mail]>
> ---
> 
> Notes:
>     Changes since v2:
>     
>     1. add devicetree bindings document
>     
>     Applies to next-20130716
> 
>  .../bindings/clock/moxa,moxart-core-clock.txt      | 19 +++++
>  drivers/clk/Makefile                               |  1 +
>  drivers/clk/clk-moxart.c                           | 82 ++++++++++++++++++++++
>  3 files changed, 102 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..cf69361
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/moxa,moxart-core-clock.txt
> @@ -0,0 +1,19 @@
> +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"
> +- reg : Should contain registers location and length
> +
> +For example:
> +
> +	clk: core-clock@98100000 {
> +		compatible = "moxa,moxart-core-clock";
> +		reg = <0x98100000 0x34>;
> +	};
> 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..79c27f4
> --- /dev/null
> +++ b/drivers/clk/clk-moxart.c
> @@ -0,0 +1,82 @@
> +/*
> + * MOXA ART SoCs clock driver.
> + *
> + * Copyright (C) 2013 Jonas Jensen
> + *
> + * Jonas Jensen <[e-mail]>
> + *
> + * 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/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/of_address.h>
> +#include <linux/clkdev.h>
> +
> +static const struct of_device_id moxart_apb_clock_match[] = {
> +	{ .compatible = "moxa,moxart-apb-clock" },
> +	{ }
> +};
> +
> +void __init moxart_of_clk_init(struct device_node *node)
> +{
> +	static void __iomem *base;
> +	struct device_node *clk_node;
> +	struct clk *clk;
> +	unsigned long rate;
> +	unsigned int mul, val, div;
> +
> +	base = of_iomap(node, 0);
> +	if (IS_ERR(base))
> +		panic("%s: of_iomap failed\n", node->full_name);
> +
> +	clk_node = of_find_matching_node(NULL, moxart_apb_clock_match);
> +	if (!clk_node)
> +		panic("%s: can't find clock DT node\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);
> +
> +	clk = clk_register_fixed_rate(NULL, "clkapb", NULL, CLK_IS_ROOT, rate);
> +	clk_register_clkdev(clk, NULL, "clkapb");
> +	of_clk_add_provider(clk_node, of_clk_src_simple_get, clk);

This confuses me. moxart_of_clk_init gets called because there was a
"moxa,moxart-core-clock", node in the dt, but the driver only seems to
use the information to figure out the configuration of another clock
("moxa,moxart-apb-clock"), and never registers a clock specifically for
the core-clock.

I couldn't find "moxa,moxart-apb-clock" described in mainline. COuld you
describe the relationship between core-clock and apb-clock?

Thanks,
Mark.

> +
> +	iounmap(base);
> +}
> +CLK_OF_DECLARE(moxart_core_clock, "moxa,moxart-core-clock", moxart_of_clk_init);
> -- 
> 1.8.2.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

Jonas Jensen July 18, 2013, 10:36 a.m. UTC | #2

Hi Mark, thanks for taking a look at this.

On 18 July 2013 11:50, Mark Rutland <[e-mail]> wrote:
> This confuses me. moxart_of_clk_init gets called because there was a
> "moxa,moxart-core-clock", node in the dt, but the driver only seems to
> use the information to figure out the configuration of another clock
> ("moxa,moxart-apb-clock"), and never registers a clock specifically for
> the core-clock.
>
> I couldn't find "moxa,moxart-apb-clock" described in mainline. COuld you
> describe the relationship between core-clock and apb-clock?

It's true core-clock exist only so the register can be mapped.

apb-clock is part of a patch set that will add new device tree files for the
MOXA ART SoC, but it's not in mainline:

http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/181757.html

apb-clock could be a fixed rate 48MHz DT only clock, but because we can't
be sure it's 48MHz on all platforms, reading it from a register with core-clock
is more portable.


Best regards,
Jonas

Mark Rutland July 18, 2013, 11:02 a.m. UTC | #3

On Thu, Jul 18, 2013 at 11:36:40AM +0100, Jonas Jensen wrote:
> Hi Mark, thanks for taking a look at this.
> 
> On 18 July 2013 11:50, Mark Rutland <[e-mail]> wrote:
> > This confuses me. moxart_of_clk_init gets called because there was a
> > "moxa,moxart-core-clock", node in the dt, but the driver only seems to
> > use the information to figure out the configuration of another clock
> > ("moxa,moxart-apb-clock"), and never registers a clock specifically for
> > the core-clock.
> >
> > I couldn't find "moxa,moxart-apb-clock" described in mainline. COuld you
> > describe the relationship between core-clock and apb-clock?
> 
> It's true core-clock exist only so the register can be mapped.

Ok. I'm just concerned that the linkage isn't explicit or obvious.

> 
> apb-clock is part of a patch set that will add new device tree files for the
> MOXA ART SoC, but it's not in mainline:
> 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/181757.html
> 
> apb-clock could be a fixed rate 48MHz DT only clock, but because we can't
> be sure it's 48MHz on all platforms, reading it from a register with core-clock
> is more portable.

This does leave apb-clock completely dependent on core-clock, and unless
I've missed something there's no linkage between the two described in
the dt.

How does core-clock physically relate to apb-clock? Does it feed or is
it fed by apb-clock?

Are we always guaranteed to have core-clock if we have apb-clock, and is
it part of the same block in hardware? If so we could describe the
amalgamation as a provider with two clock outputs, with core-clock's
registers for configuration at probe-time.

Thanks,
Mark.

> 
> 
> Best regards,
> Jonas
>

Jonas Jensen July 18, 2013, 11:55 a.m. UTC | #4

On 18 July 2013 13:02, Mark Rutland <[e-mail]> wrote:
> Ok. I'm just concerned that the linkage isn't explicit or obvious.

> This does leave apb-clock completely dependent on core-clock, and unless
> I've missed something there's no linkage between the two described in
> the dt.

I can add a description in the core-clock binding and also for
apb-clock pointing
out that it's set from core-clock.

> How does core-clock physically relate to apb-clock? Does it feed or is
> it fed by apb-clock?

apb-clock is entirely a DT construct used by drivers to get the fixed
rate 48MHz.
It's not fed by core-clock more than what happens in probe.

For UC-7112-LX, drivers using apb-clock are: clocksource, MMC, watchdog

Because clocksource relies on apb-clock, a successful probe of
core-clock is critical.

Commonly, drivers look up the apb-clock node and call clk_get_rate.

> Are we always guaranteed to have core-clock if we have apb-clock, and is
> it part of the same block in hardware? If so we could describe the
> amalgamation as a provider with two clock outputs, with core-clock's
> registers for configuration at probe-time.

Yes, as described above, there can not be a apb-clock without core-clock.


I think drivers could find and use core-clock instead. Maybe the abstraction
of apb-clock is unnecessary?


Best regards,
Jonas

Mark Rutland July 18, 2013, 1:56 p.m. UTC | #5

On Thu, Jul 18, 2013 at 12:55:26PM +0100, Jonas Jensen wrote:
> On 18 July 2013 13:02, Mark Rutland <[e-mail]> wrote:
> > Ok. I'm just concerned that the linkage isn't explicit or obvious.
> 
> > This does leave apb-clock completely dependent on core-clock, and unless
> > I've missed something there's no linkage between the two described in
> > the dt.
> 
> I can add a description in the core-clock binding and also for
> apb-clock pointing
> out that it's set from core-clock.
> 
> > How does core-clock physically relate to apb-clock? Does it feed or is
> > it fed by apb-clock?
> 
> apb-clock is entirely a DT construct used by drivers to get the fixed
> rate 48MHz.
> It's not fed by core-clock more than what happens in probe.
> 
> For UC-7112-LX, drivers using apb-clock are: clocksource, MMC, watchdog
> 
> Because clocksource relies on apb-clock, a successful probe of
> core-clock is critical.
> 
> Commonly, drivers look up the apb-clock node and call clk_get_rate.

When you say look up the apb-clock node, I assume they find it via their
clocks property with of_clk_get or similar, rather than scanning the
tree for a "moxart,moxa-apb-clock" node specifically?

> 
> > Are we always guaranteed to have core-clock if we have apb-clock, and is
> > it part of the same block in hardware? If so we could describe the
> > amalgamation as a provider with two clock outputs, with core-clock's
> > registers for configuration at probe-time.
> 
> Yes, as described above, there can not be a apb-clock without core-clock.
> 
> 
> I think drivers could find and use core-clock instead. Maybe the abstraction
> of apb-clock is unnecessary?

Given the above, I think it would make more sense to have core-clock
registered, and have it passed to the devices currently passed
apb-clock.

Thanks,
Mark.

> 
> 
> Best regards,
> Jonas
>

Jonas Jensen July 18, 2013, 2:25 p.m. UTC | #6

On 18 July 2013 15:56, Mark Rutland <[e-mail]> wrote:
> When you say look up the apb-clock node, I assume they find it via their
> clocks property with of_clk_get or similar, rather than scanning the
> tree for a "moxart,moxa-apb-clock" node specifically?

Yes, all use of_clk_get(node, 0), which is fortunate because e.g.
clocksource is already merged with linaro's clockevent drivers tree.

That should mean I can get away with only changing the DT files :)

> Given the above, I think it would make more sense to have core-clock
> registered, and have it passed to the devices currently passed
> apb-clock.

OK, I'll return with an updated version.


Best regards,
Jonas

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..cf69361
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/moxa,moxart-core-clock.txt
@@ -0,0 +1,19 @@ 
+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"
+- reg : Should contain registers location and length
+
+For example:
+
+	clk: core-clock@98100000 {
+		compatible = "moxa,moxart-core-clock";
+		reg = <0x98100000 0x34>;
+	};
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..79c27f4
--- /dev/null
+++ b/drivers/clk/clk-moxart.c
@@ -0,0 +1,82 @@ 
+/*
+ * MOXA ART SoCs clock driver.
+ *
+ * Copyright (C) 2013 Jonas Jensen
+ *
+ * Jonas Jensen <[e-mail]>
+ *
+ * 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/platform_device.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/of_address.h>
+#include <linux/clkdev.h>
+
+static const struct of_device_id moxart_apb_clock_match[] = {
+	{ .compatible = "moxa,moxart-apb-clock" },
+	{ }
+};
+
+void __init moxart_of_clk_init(struct device_node *node)
+{
+	static void __iomem *base;
+	struct device_node *clk_node;
+	struct clk *clk;
+	unsigned long rate;
+	unsigned int mul, val, div;
+
+	base = of_iomap(node, 0);
+	if (IS_ERR(base))
+		panic("%s: of_iomap failed\n", node->full_name);
+
+	clk_node = of_find_matching_node(NULL, moxart_apb_clock_match);
+	if (!clk_node)
+		panic("%s: can't find clock DT node\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);
+
+	clk = clk_register_fixed_rate(NULL, "clkapb", NULL, CLK_IS_ROOT, rate);
+	clk_register_clkdev(clk, NULL, "clkapb");
+	of_clk_add_provider(clk_node, of_clk_src_simple_get, clk);
+
+	iounmap(base);
+}
+CLK_OF_DECLARE(moxart_core_clock, "moxa,moxart-core-clock", moxart_of_clk_init);