[PATCH] clk: add specified-rate clock

[PATCH] clk: add specified-rate clock

[James Hogan]

The frequency of some SoC's external oscillators (for example TZ1090's
XTAL1) are configured by the board using pull-ups/pull-downs of
configuration pins, the logic values of which are automatically latched
on reset and available in an SoC register. Add a generic clock component
and DT bindings to handle this.

It behaves similar to a fixed rate clock (read-only), except it needs
information about a register field (reg, shift, width), and the
clock-frequency is a mapping from register field values to clock
frequencies.

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Mike Turquette <mturquette@linaro.org>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Rob Landley <rob@landley.net>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>
---
 .../devicetree/bindings/clock/specified-clock.txt  |  39 ++++
 drivers/clk/Makefile                               |   1 +
 drivers/clk/clk-specified-rate.c                   | 201 +++++++++++++++++++++
 include/linux/clk-provider.h                       |  37 ++++
 4 files changed, 278 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/specified-clock.txt
 create mode 100644 drivers/clk/clk-specified-rate.c

diff --git a/Documentation/devicetree/bindings/clock/specified-clock.txt b/Documentation/devicetree/bindings/clock/specified-clock.txt
new file mode 100644
index 0000000..b36ccf9
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/specified-clock.txt
@@ -0,0 +1,39 @@
+Binding for fixed-rate clock sources with readable configuration.
+
+This binding uses the common clock binding[1].
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible         : Shall be "specified-clock".
+- #clock-cells       : From common clock binding; shall be set to 0.
+- reg                : Address of configuration register.
+- shift              : Shift of config value field in configuration register.
+- width              : Width of config value field in configuration register.
+- clock-frequency    : Frequency mapping of clock. Consecutive pairs of cells
+                       represent the config value to match and the clock
+                       frequency in Hz for that config value.
+
+Optional properties:
+- clock-output-names : From common clock binding.
+
+Example:
+	clock {
+		compatible = "specified-clock";
+		#clock-cells = <0>;
+		reg = <0x02004004 0x4>;	/* CR_PERIP_RESET_CFG */
+		shift = <8>;		/* FXTAL */
+		width = <4>;
+		clock-frequency =
+		/*	 FXTAL	Frequency */
+			<0	16384000>,
+			<1	19200000>,
+			<2	24000000>,
+			<3	24576000>,
+			<4	26000000>,
+			<5	36000000>,
+			<6	36864000>,
+			<7	38400000>,
+			<8	40000000>;
+		clock-output-names = "xtal1";
+	};
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index e7f7fe9..1343179 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_COMMON_CLK)	+= clk-fixed-rate.o
 obj-$(CONFIG_COMMON_CLK)	+= clk-gate.o
 obj-$(CONFIG_COMMON_CLK)	+= clk-mux.o
 obj-$(CONFIG_COMMON_CLK)	+= clk-composite.o
+obj-$(CONFIG_COMMON_CLK)	+= clk-specified-rate.o
 
 # SoCs specific
 obj-$(CONFIG_ARCH_BCM2835)	+= clk-bcm2835.o
diff --git a/drivers/clk/clk-specified-rate.c b/drivers/clk/clk-specified-rate.c
new file mode 100644
index 0000000..8f78033
--- /dev/null
+++ b/drivers/clk/clk-specified-rate.c
@@ -0,0 +1,201 @@
+/*
+ * Copyright (C) 2010-2011 Canonical Ltd <jeremy.kerr@canonical.com>
+ * Copyright (C) 2011-2012 Mike Turquette, Linaro Ltd <mturquette@linaro.org>
+ * Copyright (C) 2013 Imagination Technologies Ltd.
+ *
+ * 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.
+ *
+ * Fixed rate clock implementation with rate specified in a register field.
+ * Based on fixed rate clock implementation.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/slab.h>
+
+/*
+ * DOC: basic specified-rate clock that cannot gate
+ *
+ * Traits of this clock:
+ * prepare - clk_(un)prepare only ensures parents are prepared
+ * enable - clk_enable only ensures parents are enabled
+ * rate - rate is always a fixed value.  No clk_set_rate support
+ * parent - fixed parent.  No clk_set_parent support
+ */
+
+#define to_clk_specified_rate(_hw) \
+	container_of(_hw, struct clk_specified_rate, hw)
+
+static unsigned long clk_specified_rate_recalc_rate(struct clk_hw *hw,
+		unsigned long parent_rate)
+{
+	struct clk_specified_rate *specified = to_clk_specified_rate(hw);
+	struct clk_specified_rate_entry *entry;
+	u32 val;
+	unsigned int i;
+
+	/* read configuration field */
+	val = readl(specified->reg);
+	val >>= specified->shift;
+	val &= (1 << specified->width) - 1;
+
+	/* match the value in the mapping */
+	for (i = 0; i < specified->num_rates; ++i) {
+		entry = &specified->rates[i];
+		if (val == entry->value)
+			return entry->rate;
+	}
+
+	/* unknown rate! */
+	return 0;
+}
+
+const struct clk_ops clk_specified_rate_ops = {
+	.recalc_rate = clk_specified_rate_recalc_rate,
+};
+EXPORT_SYMBOL_GPL(clk_specified_rate_ops);
+
+/**
+ * clk_register_specified_rate - register specified-rate clock
+ * @dev:		device that is registering this clock
+ * @name:		name of this clock
+ * @parent_name:	name of clock's parent
+ * @flags:		framework-specific flags
+ * @reg:		config register
+ * @shift:		shift into config register of frequency field
+ * @width:		width of frequency field in config register
+ * @rates:		value->rate mapping entries
+ * @num_rates:		number of rates in @rates
+ */
+struct clk *clk_register_specified_rate(struct device *dev, const char *name,
+		const char *parent_name, unsigned long flags,
+		void __iomem *reg, u8 shift, u8 width,
+		struct clk_specified_rate_entry *rates,
+		unsigned long num_rates)
+{
+	struct clk_specified_rate *specified;
+	struct clk *clk;
+	struct clk_init_data init;
+
+	/* allocate specified-rate clock */
+	specified = kzalloc(sizeof(struct clk_specified_rate), GFP_KERNEL);
+	if (!specified) {
+		pr_err("%s(%s): could not allocate specified clk\n",
+		       __func__, name);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	init.name = name;
+	init.ops = &clk_specified_rate_ops;
+	init.flags = flags | CLK_IS_BASIC;
+	init.parent_names = (parent_name ? &parent_name : NULL);
+	init.num_parents = (parent_name ? 1 : 0);
+
+	/* struct clk_specified_rate assignments */
+	specified->reg = reg;
+	specified->shift = shift;
+	specified->width = width;
+	specified->rates = rates;
+	specified->num_rates = num_rates;
+	specified->hw.init = &init;
+
+	/* register the clock */
+	clk = clk_register(dev, &specified->hw);
+
+	if (IS_ERR(clk))
+		kfree(specified);
+
+	return clk;
+}
+
+#ifdef CONFIG_OF
+/**
+ * of_specified_clk_setup() - Setup function for specified fixed rate clock
+ */
+void __init of_specified_clk_setup(struct device_node *node)
+{
+	struct clk *clk;
+	const char *clk_name = node->name;
+	u32 shift, width, rate;
+	void __iomem *reg;
+	int len, num_rates, i;
+	struct property *prop;
+	struct clk_specified_rate_entry *rates;
+	const __be32 *p;
+
+	of_property_read_string(node, "clock-output-names", &clk_name);
+
+	if (of_property_read_u32(node, "shift", &shift)) {
+		pr_err("%s(%s): could not read shift property\n",
+		       __func__, clk_name);
+		return;
+	}
+
+	if (of_property_read_u32(node, "width", &width)) {
+		pr_err("%s(%s): could not read width property\n",
+		       __func__, clk_name);
+		return;
+	}
+
+	reg = of_iomap(node, 0);
+	if (!reg) {
+		pr_err("%s(%s): of_iomap failed\n",
+		       __func__, clk_name);
+		return;
+	}
+
+	/* check clock-frequency exists */
+	prop = of_find_property(node, "clock-frequency", &len);
+	if (!prop) {
+		pr_err("%s(%s): could not find clock-frequency property\n",
+		       __func__, clk_name);
+		goto err_iounmap;
+	}
+
+	if (len & (sizeof(u32)*2 - 1)) {
+		pr_err("%s(%s): clock-frequency has invalid size of %d bytes\n",
+		       __func__, clk_name, len);
+		goto err_iounmap;
+	}
+	num_rates = len / (sizeof(*rates)*2);
+
+	rates = kzalloc(sizeof(*rates)*num_rates, GFP_KERNEL);
+	if (!rates) {
+		pr_err("%s(%s): could not allocate %d rate mapping entries\n",
+		       __func__, clk_name, num_rates);
+		goto err_iounmap;
+	}
+
+	/* extract rate mapping */
+	for (i = 0, p = of_prop_next_u32(prop, NULL, &rates[0].value);
+	     p;
+	     ++i, p = of_prop_next_u32(prop, p, &rates[i].value)) {
+		p = of_prop_next_u32(prop, p, &rate);
+		rates[i].rate = rate;
+		pr_debug("%s(%s): map %u -> %lu Hz\n",
+			 __func__, clk_name, rates[i].value, rates[i].rate);
+	}
+
+	clk = clk_register_specified_rate(NULL, clk_name, NULL, CLK_IS_ROOT,
+					  reg, shift, width, rates, num_rates);
+	if (IS_ERR(clk))
+		goto err_kfree;
+
+	of_clk_add_provider(node, of_clk_src_simple_get, clk);
+
+	return;
+
+err_kfree:
+	kfree(rates);
+err_iounmap:
+	iounmap(reg);
+}
+EXPORT_SYMBOL_GPL(of_specified_clk_setup);
+CLK_OF_DECLARE(specified_clk, "specified-clock", of_specified_clk_setup);
+#endif
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 1186098..218a406 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -196,6 +196,43 @@ struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
 void of_fixed_clk_setup(struct device_node *np);
 
 /**
+ * struct clk_specified_rate_entry - a single possible specified rate
+ * @value:	value to match in config register
+ * @rate:	rate to use when config register matches @value
+ */
+struct clk_specified_rate_entry {
+	u32		value;
+	unsigned long	rate;
+};
+
+/**
+ * struct clk_specified_rate - specified-rate clock
+ * @hw:		handle between common and hardware-specific interfaces
+ * @reg:	register containing rate specifier field
+ * @shift:	shift to rate specifier field
+ * @width:	width of rate specifier field
+ * @rates:	mapping of specified frequencies
+ * @num_rates:	number of rates in array
+ */
+struct clk_specified_rate {
+	struct		clk_hw hw;
+	void __iomem	*reg;
+	u8		shift;
+	u8		width;
+	struct clk_specified_rate_entry	*rates;
+	unsigned int	num_rates;
+};
+
+extern const struct clk_ops clk_specified_rate_ops;
+struct clk *clk_register_specified_rate(struct device *dev, const char *name,
+		const char *parent_names, unsigned long flags,
+		void __iomem *reg, u8 shift, u8 width,
+		struct clk_specified_rate_entry *rates,
+		unsigned long num_rates);
+
+void of_specified_clk_setup(struct device_node *np);
+
+/**
  * struct clk_gate - gating clock
  *
  * @hw:		handle between common and hardware-specific interfaces
-- 
1.8.1.2

Re: [PATCH] clk: add specified-rate clock

[Mike Turquette]

Quoting James Hogan (2013-05-10 05:44:22)
> The frequency of some SoC's external oscillators (for example TZ1090's
> XTAL1) are configured by the board using pull-ups/pull-downs of
> configuration pins, the logic values of which are automatically latched
> on reset and available in an SoC register. Add a generic clock component
> and DT bindings to handle this.
> 
> It behaves similar to a fixed rate clock (read-only), except it needs
> information about a register field (reg, shift, width), and the
> clock-frequency is a mapping from register field values to clock
> frequencies.
> 

James,

Thanks for sending this!  It looks mostly good and is a useful clock
type to support.  Comments below.

<snip>
> diff --git a/Documentation/devicetree/bindings/clock/specified-clock.txt b/Documentation/devicetree/bindings/clock/specified-clock.txt
> new file mode 100644
> index 0000000..b36ccf9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/specified-clock.txt
> @@ -0,0 +1,39 @@
> +Binding for fixed-rate clock sources with readable configuration.
> +
> +This binding uses the common clock binding[1].
> +
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +Required properties:
> +- compatible         : Shall be "specified-clock".
> +- #clock-cells       : From common clock binding; shall be set to 0.
> +- reg                : Address of configuration register.
> +- shift              : Shift of config value field in configuration register.
> +- width              : Width of config value field in configuration register.

It might be better to make this a mask instead of the width.  We have
already hit this issue with the mux table on Nvidia where arbitrary
masks are necessary.  Mask + shift probably helps future-proof us a bit.

The rest of the binding looks good.

<snip>
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index e7f7fe9..1343179 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_COMMON_CLK)        += clk-fixed-rate.o
>  obj-$(CONFIG_COMMON_CLK)       += clk-gate.o
>  obj-$(CONFIG_COMMON_CLK)       += clk-mux.o
>  obj-$(CONFIG_COMMON_CLK)       += clk-composite.o
> +obj-$(CONFIG_COMMON_CLK)       += clk-specified-rate.o

One thing that does occur to me is that this could potentially be
combined with the fixed-rate clock.  If the properties for a specified
rate existing in the DT data then this code applies, otherwise the
existing fixed-rate code is used.  I don't have a strong opinion about
combining the code, but something to consider.

<snip>
> +#ifdef CONFIG_OF
> +/**
> + * of_specified_clk_setup() - Setup function for specified fixed rate clock
> + */
> +void __init of_specified_clk_setup(struct device_node *node)
> +{
> +       struct clk *clk;
> +       const char *clk_name = node->name;
> +       u32 shift, width, rate;
> +       void __iomem *reg;
> +       int len, num_rates, i;
> +       struct property *prop;
> +       struct clk_specified_rate_entry *rates;
> +       const __be32 *p;
> +
> +       of_property_read_string(node, "clock-output-names", &clk_name);
> +
> +       if (of_property_read_u32(node, "shift", &shift)) {
> +               pr_err("%s(%s): could not read shift property\n",
> +                      __func__, clk_name);
> +               return;
> +       }
> +
> +       if (of_property_read_u32(node, "width", &width)) {
> +               pr_err("%s(%s): could not read width property\n",
> +                      __func__, clk_name);
> +               return;
> +       }
> +
> +       reg = of_iomap(node, 0);
> +       if (!reg) {
> +               pr_err("%s(%s): of_iomap failed\n",
> +                      __func__, clk_name);
> +               return;
> +       }
> +
> +       /* check clock-frequency exists */
> +       prop = of_find_property(node, "clock-frequency", &len);
> +       if (!prop) {
> +               pr_err("%s(%s): could not find clock-frequency property\n",
> +                      __func__, clk_name);
> +               goto err_iounmap;
> +       }
> +
> +       if (len & (sizeof(u32)*2 - 1)) {
> +               pr_err("%s(%s): clock-frequency has invalid size of %d bytes\n",
> +                      __func__, clk_name, len);
> +               goto err_iounmap;
> +       }
> +       num_rates = len / (sizeof(*rates)*2);

This tripped me up for a few minutes.  I think it is a bit weird to
count bytes as a way to validate length and determine the number of
pairs.

I have written a binding for mux clocks which can have a table of
parents and the code below shows how I parsed the table:

+       u32 pair[2];
+
+       table_size = of_count_phandle_with_args(node, "table", "#clock-cells");
+
+       if (table_size < 1)
+               return NULL;
+
+       table = kzalloc(sizeof(struct clk_div_table) * table_size, GFP_KERNEL);
+       if (!table) {
+               pr_err("%s: unable to allocate memory for %s table\n", __func__, node->name);
+               return NULL;
+       }
+
+       for (i = 0; i < table_size; i++) {
+               if (!of_property_read_u32_array(node, "table", pair, ARRAY_SIZE(pair))) {
+                       table[i].val = pair[0];
+                       table[i].div = pair[1];
+               }
+       }

I'll be posting the mux-clock binding soon :-)

Regards,
Mike

Re: [PATCH] clk: add specified-rate clock

[James Hogan]

On 29/05/13 18:39, Mike Turquette wrote:
> Quoting James Hogan (2013-05-10 05:44:22)
>> The frequency of some SoC's external oscillators (for example TZ1090's
>> XTAL1) are configured by the board using pull-ups/pull-downs of
>> configuration pins, the logic values of which are automatically latched
>> on reset and available in an SoC register. Add a generic clock component
>> and DT bindings to handle this.
>>
>> It behaves similar to a fixed rate clock (read-only), except it needs
>> information about a register field (reg, shift, width), and the
>> clock-frequency is a mapping from register field values to clock
>> frequencies.
>>
> 
> James,
> 
> Thanks for sending this!  It looks mostly good and is a useful clock
> type to support.  Comments below.

Hi Mike,

Sorry for slight delay getting back to you. I had another think about
this stuff yesterday...

> 
> <snip>
>> diff --git a/Documentation/devicetree/bindings/clock/specified-clock.txt b/Documentation/devicetree/bindings/clock/specified-clock.txt
>> new file mode 100644
>> index 0000000..b36ccf9
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/specified-clock.txt
>> @@ -0,0 +1,39 @@
>> +Binding for fixed-rate clock sources with readable configuration.
>> +
>> +This binding uses the common clock binding[1].
>> +
>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>> +
>> +Required properties:
>> +- compatible         : Shall be "specified-clock".
>> +- #clock-cells       : From common clock binding; shall be set to 0.
>> +- reg                : Address of configuration register.
>> +- shift              : Shift of config value field in configuration register.
>> +- width              : Width of config value field in configuration register.
> 
> It might be better to make this a mask instead of the width.  We have
> already hit this issue with the mux table on Nvidia where arbitrary
> masks are necessary.  Mask + shift probably helps future-proof us a bit.

Yes, thanks. I've now borrowed the bit-mask and bit-shift from your mux
binding proposals (including defaulting shift to ffs(mask)-1 ... nice).

> 
> The rest of the binding looks good.
> 
> <snip>
>> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
>> index e7f7fe9..1343179 100644
>> --- a/drivers/clk/Makefile
>> +++ b/drivers/clk/Makefile
>> @@ -8,6 +8,7 @@ obj-$(CONFIG_COMMON_CLK)        += clk-fixed-rate.o
>>  obj-$(CONFIG_COMMON_CLK)       += clk-gate.o
>>  obj-$(CONFIG_COMMON_CLK)       += clk-mux.o
>>  obj-$(CONFIG_COMMON_CLK)       += clk-composite.o
>> +obj-$(CONFIG_COMMON_CLK)       += clk-specified-rate.o
> 
> One thing that does occur to me is that this could potentially be
> combined with the fixed-rate clock.  If the properties for a specified
> rate existing in the DT data then this code applies, otherwise the
> existing fixed-rate code is used.  I don't have a strong opinion about
> combining the code, but something to consider.

That's actually a much neater solution. Because the clock is still
fixed, the register value can be read once while processing the DT node,
and an otherwise normal fixed rate clock created at the right frequency.
It doesn't even need to allocate memory to store the table.

The remaining question is whether to extend the fixed-clock binding or
have a separate one. I'm in two minds. On the one hand it is a
fixed-rate clock, on the other hand the only shared properties at the
moment would be standard clock properties (clock-output-names etc), so
I'm probably leaning towards a separate binding. I'll send an updated
patchset soon.

> 
> <snip>
>> +#ifdef CONFIG_OF
>> +/**
>> + * of_specified_clk_setup() - Setup function for specified fixed rate clock
>> + */
>> +void __init of_specified_clk_setup(struct device_node *node)
>> +{
>> +       struct clk *clk;
>> +       const char *clk_name = node->name;
>> +       u32 shift, width, rate;
>> +       void __iomem *reg;
>> +       int len, num_rates, i;
>> +       struct property *prop;
>> +       struct clk_specified_rate_entry *rates;
>> +       const __be32 *p;
>> +
>> +       of_property_read_string(node, "clock-output-names", &clk_name);
>> +
>> +       if (of_property_read_u32(node, "shift", &shift)) {
>> +               pr_err("%s(%s): could not read shift property\n",
>> +                      __func__, clk_name);
>> +               return;
>> +       }
>> +
>> +       if (of_property_read_u32(node, "width", &width)) {
>> +               pr_err("%s(%s): could not read width property\n",
>> +                      __func__, clk_name);
>> +               return;
>> +       }
>> +
>> +       reg = of_iomap(node, 0);
>> +       if (!reg) {
>> +               pr_err("%s(%s): of_iomap failed\n",
>> +                      __func__, clk_name);
>> +               return;
>> +       }
>> +
>> +       /* check clock-frequency exists */
>> +       prop = of_find_property(node, "clock-frequency", &len);
>> +       if (!prop) {
>> +               pr_err("%s(%s): could not find clock-frequency property\n",
>> +                      __func__, clk_name);
>> +               goto err_iounmap;
>> +       }
>> +
>> +       if (len & (sizeof(u32)*2 - 1)) {
>> +               pr_err("%s(%s): clock-frequency has invalid size of %d bytes\n",
>> +                      __func__, clk_name, len);
>> +               goto err_iounmap;
>> +       }
>> +       num_rates = len / (sizeof(*rates)*2);
> 
> This tripped me up for a few minutes.  I think it is a bit weird to
> count bytes as a way to validate length and determine the number of
> pairs.

Yes, and I actually recently discovered that this is wrong anyway. rates
is a struct of two elements so it ends up dividing twice, and it just so
happened that all the boards I have (until I hacked up qemu to report a
different frequency) specify a value in the first half of those defined. :)

Thanks
James

Re: [PATCH] clk: add specified-rate clock

[Tomasz Figa]

Hi James, Mike,

On Wednesday 13 of November 2013 14:09:56 James Hogan wrote:
> On 29/05/13 18:39, Mike Turquette wrote:
> > Quoting James Hogan (2013-05-10 05:44:22)
> >> The frequency of some SoC's external oscillators (for example TZ1090's
> >> XTAL1) are configured by the board using pull-ups/pull-downs of
> >> configuration pins, the logic values of which are automatically latched
> >> on reset and available in an SoC register. Add a generic clock component
> >> and DT bindings to handle this.
> >>
> >> It behaves similar to a fixed rate clock (read-only), except it needs
> >> information about a register field (reg, shift, width), and the
> >> clock-frequency is a mapping from register field values to clock
> >> frequencies.
> >>
> > 
> > James,
> > 
> > Thanks for sending this!  It looks mostly good and is a useful clock
> > type to support.  Comments below.
> 
> Hi Mike,
> 
> Sorry for slight delay getting back to you. I had another think about
> this stuff yesterday...
> 

Just a random idea that came to my mind while reading this thread:

What about modelling this as a set of fixed rate clocks fed into
a read-only mux?

Best regards,
Tomasz

> > 
> > <snip>
> >> diff --git a/Documentation/devicetree/bindings/clock/specified-clock.txt b/Documentation/devicetree/bindings/clock/specified-clock.txt
> >> new file mode 100644
> >> index 0000000..b36ccf9
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/clock/specified-clock.txt
> >> @@ -0,0 +1,39 @@
> >> +Binding for fixed-rate clock sources with readable configuration.
> >> +
> >> +This binding uses the common clock binding[1].
> >> +
> >> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> >> +
> >> +Required properties:
> >> +- compatible         : Shall be "specified-clock".
> >> +- #clock-cells       : From common clock binding; shall be set to 0.
> >> +- reg                : Address of configuration register.
> >> +- shift              : Shift of config value field in configuration register.
> >> +- width              : Width of config value field in configuration register.
> > 
> > It might be better to make this a mask instead of the width.  We have
> > already hit this issue with the mux table on Nvidia where arbitrary
> > masks are necessary.  Mask + shift probably helps future-proof us a bit.
> 
> Yes, thanks. I've now borrowed the bit-mask and bit-shift from your mux
> binding proposals (including defaulting shift to ffs(mask)-1 ... nice).
> 
> > 
> > The rest of the binding looks good.
> > 
> > <snip>
> >> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> >> index e7f7fe9..1343179 100644
> >> --- a/drivers/clk/Makefile
> >> +++ b/drivers/clk/Makefile
> >> @@ -8,6 +8,7 @@ obj-$(CONFIG_COMMON_CLK)        += clk-fixed-rate.o
> >>  obj-$(CONFIG_COMMON_CLK)       += clk-gate.o
> >>  obj-$(CONFIG_COMMON_CLK)       += clk-mux.o
> >>  obj-$(CONFIG_COMMON_CLK)       += clk-composite.o
> >> +obj-$(CONFIG_COMMON_CLK)       += clk-specified-rate.o
> > 
> > One thing that does occur to me is that this could potentially be
> > combined with the fixed-rate clock.  If the properties for a specified
> > rate existing in the DT data then this code applies, otherwise the
> > existing fixed-rate code is used.  I don't have a strong opinion about
> > combining the code, but something to consider.
> 
> That's actually a much neater solution. Because the clock is still
> fixed, the register value can be read once while processing the DT node,
> and an otherwise normal fixed rate clock created at the right frequency.
> It doesn't even need to allocate memory to store the table.
> 
> The remaining question is whether to extend the fixed-clock binding or
> have a separate one. I'm in two minds. On the one hand it is a
> fixed-rate clock, on the other hand the only shared properties at the
> moment would be standard clock properties (clock-output-names etc), so
> I'm probably leaning towards a separate binding. I'll send an updated
> patchset soon.
> 
> > 
> > <snip>
> >> +#ifdef CONFIG_OF
> >> +/**
> >> + * of_specified_clk_setup() - Setup function for specified fixed rate clock
> >> + */
> >> +void __init of_specified_clk_setup(struct device_node *node)
> >> +{
> >> +       struct clk *clk;
> >> +       const char *clk_name = node->name;
> >> +       u32 shift, width, rate;
> >> +       void __iomem *reg;
> >> +       int len, num_rates, i;
> >> +       struct property *prop;
> >> +       struct clk_specified_rate_entry *rates;
> >> +       const __be32 *p;
> >> +
> >> +       of_property_read_string(node, "clock-output-names", &clk_name);
> >> +
> >> +       if (of_property_read_u32(node, "shift", &shift)) {
> >> +               pr_err("%s(%s): could not read shift property\n",
> >> +                      __func__, clk_name);
> >> +               return;
> >> +       }
> >> +
> >> +       if (of_property_read_u32(node, "width", &width)) {
> >> +               pr_err("%s(%s): could not read width property\n",
> >> +                      __func__, clk_name);
> >> +               return;
> >> +       }
> >> +
> >> +       reg = of_iomap(node, 0);
> >> +       if (!reg) {
> >> +               pr_err("%s(%s): of_iomap failed\n",
> >> +                      __func__, clk_name);
> >> +               return;
> >> +       }
> >> +
> >> +       /* check clock-frequency exists */
> >> +       prop = of_find_property(node, "clock-frequency", &len);
> >> +       if (!prop) {
> >> +               pr_err("%s(%s): could not find clock-frequency property\n",
> >> +                      __func__, clk_name);
> >> +               goto err_iounmap;
> >> +       }
> >> +
> >> +       if (len & (sizeof(u32)*2 - 1)) {
> >> +               pr_err("%s(%s): clock-frequency has invalid size of %d bytes\n",
> >> +                      __func__, clk_name, len);
> >> +               goto err_iounmap;
> >> +       }
> >> +       num_rates = len / (sizeof(*rates)*2);
> > 
> > This tripped me up for a few minutes.  I think it is a bit weird to
> > count bytes as a way to validate length and determine the number of
> > pairs.
> 
> Yes, and I actually recently discovered that this is wrong anyway. rates
> is a struct of two elements so it ends up dividing twice, and it just so
> happened that all the boards I have (until I hacked up qemu to report a
> different frequency) specify a value in the first half of those defined. :)
> 
> Thanks
> James
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] clk: add specified-rate clock

[James Hogan]

On 13/11/13 14:18, Tomasz Figa wrote:
> Hi James, Mike,
> 
> On Wednesday 13 of November 2013 14:09:56 James Hogan wrote:
>> On 29/05/13 18:39, Mike Turquette wrote:
>>> Quoting James Hogan (2013-05-10 05:44:22)
>>>> The frequency of some SoC's external oscillators (for example TZ1090's
>>>> XTAL1) are configured by the board using pull-ups/pull-downs of
>>>> configuration pins, the logic values of which are automatically latched
>>>> on reset and available in an SoC register. Add a generic clock component
>>>> and DT bindings to handle this.
>>>>
>>>> It behaves similar to a fixed rate clock (read-only), except it needs
>>>> information about a register field (reg, shift, width), and the
>>>> clock-frequency is a mapping from register field values to clock
>>>> frequencies.
>>>>
>>>
>>> James,
>>>
>>> Thanks for sending this!  It looks mostly good and is a useful clock
>>> type to support.  Comments below.
>>
>> Hi Mike,
>>
>> Sorry for slight delay getting back to you. I had another think about
>> this stuff yesterday...
>>
> 
> Just a random idea that came to my mind while reading this thread:
> 
> What about modelling this as a set of fixed rate clocks fed into
> a read-only mux?

Yes, that had occurred to me too. I suppose the arguments against would be:
* it doesn't describe the hardware, there is no mux, just a fixed rate
clock with a discoverable frequency.
* it would sort of work for my small case of only having 9 possible
frequencies (although it would be a bit verbose), but wouldn't scale
nicely or be extendible to if the frequency was encoded more
continuously in the register value. E.g. if the frequency was 1MHz *
(the register value - 1) or something crazy like that. Of course that's
conjecture and SoC designers probably aren't going to want to use more
pins for bootstrap config than necessary.

Cheers
James

Re: [PATCH] clk: add specified-rate clock

[Tomasz Figa]

On Wednesday 13 of November 2013 14:31:25 James Hogan wrote:
> On 13/11/13 14:18, Tomasz Figa wrote:
> > Hi James, Mike,
> > 
> > On Wednesday 13 of November 2013 14:09:56 James Hogan wrote:
> >> On 29/05/13 18:39, Mike Turquette wrote:
> >>> Quoting James Hogan (2013-05-10 05:44:22)
> >>>> The frequency of some SoC's external oscillators (for example TZ1090's
> >>>> XTAL1) are configured by the board using pull-ups/pull-downs of
> >>>> configuration pins, the logic values of which are automatically latched
> >>>> on reset and available in an SoC register. Add a generic clock component
> >>>> and DT bindings to handle this.
> >>>>
> >>>> It behaves similar to a fixed rate clock (read-only), except it needs
> >>>> information about a register field (reg, shift, width), and the
> >>>> clock-frequency is a mapping from register field values to clock
> >>>> frequencies.
> >>>>
> >>>
> >>> James,
> >>>
> >>> Thanks for sending this!  It looks mostly good and is a useful clock
> >>> type to support.  Comments below.
> >>
> >> Hi Mike,
> >>
> >> Sorry for slight delay getting back to you. I had another think about
> >> this stuff yesterday...
> >>
> > 
> > Just a random idea that came to my mind while reading this thread:
> > 
> > What about modelling this as a set of fixed rate clocks fed into
> > a read-only mux?
> 
> Yes, that had occurred to me too. I suppose the arguments against would be:
> * it doesn't describe the hardware, there is no mux, just a fixed rate
> clock with a discoverable frequency.

For me, a set of configuration pins that select clock frequency sounds
like a mux. I'd rather say that there are no physical clocks that would
be its inputs, but that's probably just an implementation detail.

> * it would sort of work for my small case of only having 9 possible
> frequencies (although it would be a bit verbose), but wouldn't scale
> nicely or be extendible to if the frequency was encoded more
> continuously in the register value. E.g. if the frequency was 1MHz *
> (the register value - 1) or something crazy like that.

I guess you would need an implementation specific code to calculate the
frequency in such cases, which is not what you suggested above (but
rather "a mapping from register field values to clock frequencies"
inside a property).

Best regards,
Tomasz

Re: [PATCH] clk: add specified-rate clock

[James Hogan]

On 13/11/13 14:38, Tomasz Figa wrote:
> On Wednesday 13 of November 2013 14:31:25 James Hogan wrote:
>> On 13/11/13 14:18, Tomasz Figa wrote:
>>> Hi James, Mike,
>>>
>>> On Wednesday 13 of November 2013 14:09:56 James Hogan wrote:
>>>> On 29/05/13 18:39, Mike Turquette wrote:
>>>>> Quoting James Hogan (2013-05-10 05:44:22)
>>>>>> The frequency of some SoC's external oscillators (for example TZ1090's
>>>>>> XTAL1) are configured by the board using pull-ups/pull-downs of
>>>>>> configuration pins, the logic values of which are automatically latched
>>>>>> on reset and available in an SoC register. Add a generic clock component
>>>>>> and DT bindings to handle this.
>>>>>>
>>>>>> It behaves similar to a fixed rate clock (read-only), except it needs
>>>>>> information about a register field (reg, shift, width), and the
>>>>>> clock-frequency is a mapping from register field values to clock
>>>>>> frequencies.
>>>>>>
>>>>>
>>>>> James,
>>>>>
>>>>> Thanks for sending this!  It looks mostly good and is a useful clock
>>>>> type to support.  Comments below.
>>>>
>>>> Hi Mike,
>>>>
>>>> Sorry for slight delay getting back to you. I had another think about
>>>> this stuff yesterday...
>>>>
>>>
>>> Just a random idea that came to my mind while reading this thread:
>>>
>>> What about modelling this as a set of fixed rate clocks fed into
>>> a read-only mux?
>>
>> Yes, that had occurred to me too. I suppose the arguments against would be:
>> * it doesn't describe the hardware, there is no mux, just a fixed rate
>> clock with a discoverable frequency.
> 
> For me, a set of configuration pins that select clock frequency sounds
> like a mux. I'd rather say that there are no physical clocks that would
> be its inputs, but that's probably just an implementation detail.

Sure, it sounds a bit like one and has a similar register interface, but
it's still not one. The hardware to latch the pins on reset and present
the logic values in a register is independent of the hardware to get the
clock signal from the fixed rate oscillator on the board into the SoC.

>> * it would sort of work for my small case of only having 9 possible
>> frequencies (although it would be a bit verbose), but wouldn't scale
>> nicely or be extendible to if the frequency was encoded more
>> continuously in the register value. E.g. if the frequency was 1MHz *
>> (the register value - 1) or something crazy like that.
> 
> I guess you would need an implementation specific code to calculate the
> frequency in such cases, which is not what you suggested above (but
> rather "a mapping from register field values to clock frequencies"
> inside a property).

Indeed. It's worth noting though that we already have a variety of such
variations handled by the generic clock divider driver (table based
lookup, one based, power of two, etc), so we should probably allow for
others to extend this later too.

Cheers
James