diff mbox

[v3,3/5] clk: dt: binding for basic multiplexer clock

Message ID 1371795256-16703-4-git-send-email-mturquette@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Mike Turquette June 21, 2013, 6:14 a.m. UTC 
Device Tree binding for the basic clock multiplexer, plus the setup
function to register the clock.  Based on the existing fixed-clock
binding.

Includes minor beautification of clk-provider.h where some whitespace is
added and of_fixed_factor_clock_setup is relocated to maintain a
consistent style.

Tero Kristo contributed helpful bug fixes to this patch.

Signed-off-by: Mike Turquette <mturquette@linaro.org>
---
Changes since v2:
* added hiword-mask property to the binding
* changed bit-shift property from u8 to u32 in the dt binding

Changes since v1:
* pass shift value into clk_register_mux_table
* s/multiplexor/multiplexer/
* removed debug prints
* mask is u32, shift is u8
* DT property names use dashes instead of underscores
* DT property names are more verbose
* shift property is optional in binding and can be auto-generated from a
  full 32-bit mask

 .../devicetree/bindings/clock/mux-clock.txt        | 79 ++++++++++++++++++++++
 drivers/clk/clk-mux.c                              | 65 +++++++++++++++++-
 include/linux/clk-provider.h                       |  5 +-
 3 files changed, 147 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/mux-clock.txt

Comments

Haojian Zhuang June 25, 2013, 8:27 a.m. UTC | #1 
On 21 June 2013 14:14, Mike Turquette <mturquette@linaro.org> wrote:
> Device Tree binding for the basic clock multiplexer, plus the setup
> function to register the clock.  Based on the existing fixed-clock
> binding.
>
> Includes minor beautification of clk-provider.h where some whitespace is
> added and of_fixed_factor_clock_setup is relocated to maintain a
> consistent style.
>
> Tero Kristo contributed helpful bug fixes to this patch.
>
> Signed-off-by: Mike Turquette <mturquette@linaro.org>
> ---
> Changes since v2:
> * added hiword-mask property to the binding
> * changed bit-shift property from u8 to u32 in the dt binding
>
> Changes since v1:
> * pass shift value into clk_register_mux_table
> * s/multiplexor/multiplexer/
> * removed debug prints
> * mask is u32, shift is u8
> * DT property names use dashes instead of underscores
> * DT property names are more verbose
> * shift property is optional in binding and can be auto-generated from a
>   full 32-bit mask
>
>  .../devicetree/bindings/clock/mux-clock.txt        | 79 ++++++++++++++++++++++
>  drivers/clk/clk-mux.c                              | 65 +++++++++++++++++-
>  include/linux/clk-provider.h                       |  5 +-
>  3 files changed, 147 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/clock/mux-clock.txt
>
> diff --git a/Documentation/devicetree/bindings/clock/mux-clock.txt b/Documentation/devicetree/bindings/clock/mux-clock.txt
> new file mode 100644
> index 0000000..54cb8d1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/mux-clock.txt
> @@ -0,0 +1,79 @@
> +Binding for simple mux clock.
> +
> +This binding uses the common clock binding[1].  It assumes a
> +register-mapped multiplexer with multiple input clock signals or
> +parents, one of which can be selected as output.  This clock does not
> +gate or adjust the parent rate via a divider or multiplier.
> +
> +By default the "clocks" property lists the parents in the same order
> +as they are programmed into the regster.  E.g:
> +
> +       clocks = <&foo_clock>, <&bar_clock>, <&baz_clock>;
> +
> +results in programming the register as follows:
> +
> +register value         selected parent clock
> +0                      foo_clock
> +1                      bar_clock
> +2                      baz_clock
> +
> +Some clock controller IPs do not allow a value of zero to be programmed
> +into the register, instead indexing begins at 1.  The optional property
> +"index_one" modified the scheme as follows:
> +
> +register value         selected clock parent
> +1                      foo_clock
> +2                      bar_clock
> +3                      baz_clock
> +
> +Additionally an optional table of bit and parent pairs may be supplied
> +like so:
> +
> +       table = <&foo_clock 0x0>, <&bar_clock, 0x2>, <&baz_clock, 0x4>;
> +
> +where the first value in the pair is the parent clock and the second
> +value is the bitfield to be programmed into the register.
> +
> +The binding must provide the register to control the mux and the mask
> +for the corresponding control bits.  Optionally the number of bits to
> +shift that mask if necessary.  If the shift value is missing it is the
> +same as supplying a zero shift.
> +
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +Required properties:
> +- compatible : shall be "mux-clock".
> +- #clock-cells : from common clock binding; shall be set to 0.
> +- clocks : link phandles of parent clocks
> +- reg : base address for register controlling adjustable mux
> +- bit-mask : arbitrary bitmask for programming the adjustable mux
> +
> +Optional properties:
> +- clock-output-names : From common clock binding.
> +- table : array of integer pairs defining parents & bitfield values
> +- bit-shift : number of bits to shift the bit-mask, defaults to
> +  (ffs(mask) - 1) if not present
> +- index-starts-at-one : valid input select programming starts at 1, not
> +  zero
> +- hiword-mask : lower half of the register programs the mux, upper half
> +  of the register indicates bits that were updated in the lower half
> +
> +Examples:
> +       clock: clock@4a008100 {
> +               compatible = "mux-clock";
> +               #clock-cells = <0>;
> +               clocks = <&clock_foo>, <&clock_bar>, <&clock_baz>;
> +               reg = <0x4a008100 0x4>
> +               mask = <0x3>;
> +               index_one;
> +       };
> +
> +       clock: clock@4a008100 {
> +               #clock-cells = <0>;
> +               compatible = "mux-clock";
> +               clocks = <&clock_foo>, <&clock_bar>, <&clock_baz>;
> +               reg = <0x4a008100 0x4>;
> +               mask = <0x3>;
> +               shift = <0>;
> +               table = <&clock_foo 1>, <&clock_bar 2>, <&clock_baz 3>;
> +       };
> diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
> index 614444c..4751bce 100644
> --- a/drivers/clk/clk-mux.c
> +++ b/drivers/clk/clk-mux.c
> @@ -1,7 +1,7 @@
>  /*
>   * Copyright (C) 2011 Sascha Hauer, Pengutronix <s.hauer@pengutronix.de>
>   * Copyright (C) 2011 Richard Zhao, Linaro <richard.zhao@linaro.org>
> - * Copyright (C) 2011-2012 Mike Turquette, Linaro Ltd <mturquette@linaro.org>
> + * Copyright (C) 2011-2013 Mike Turquette, Linaro Ltd <mturquette@linaro.org>
>   *
>   * 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
> @@ -16,6 +16,8 @@
>  #include <linux/slab.h>
>  #include <linux/io.h>
>  #include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
>
>  /*
>   * DOC: basic adjustable multiplexer clock that cannot gate
> @@ -166,3 +168,64 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
>                                       flags, reg, shift, mask, clk_mux_flags,
>                                       NULL, lock);
>  }
> +
> +#ifdef CONFIG_OF
> +/**
> + * of_mux_clk_setup() - Setup function for simple mux rate clock
> + */
> +void of_mux_clk_setup(struct device_node *node)
> +{
> +       struct clk *clk;
> +       const char *clk_name = node->name;
> +       void __iomem *reg;
> +       int num_parents;
> +       const char **parent_names;
> +       int i;
> +       u8 clk_mux_flags = 0;
> +       u32 mask = 0;
> +       u32 shift = 0;
> +
> +       of_property_read_string(node, "clock-output-names", &clk_name);
> +
> +       num_parents = of_clk_get_parent_count(node);
> +       if (num_parents < 1) {
> +               pr_err("%s: mux-clock %s must have parent(s)\n",
> +                               __func__, node->name);
> +               return;
> +       }
> +
> +       parent_names = kzalloc((sizeof(char*) * num_parents),
> +                       GFP_KERNEL);
> +
> +       for (i = 0; i < num_parents; i++)
> +               parent_names[i] = of_clk_get_parent_name(node, i);
> +
> +       reg = of_iomap(node, 0);

As you mentioned above,  reg : base address for register controlling
adjustable mux.

Each clock node has the reg property. It means that we have to
allocate one page memory
for each register since of_iomap() is used at here. We always have
lots of registers with
continuous address in SoC, like 0x40a00040, 0x40a00044, .... Then
we'll waste too much
memory in system.

How about only define common function to parse property? Make register
mapping handled
by vendor's clock driver.

Regards
Haojian
Mike Turquette June 25, 2013, 5:40 p.m. UTC | #2 
Quoting Haojian Zhuang (2013-06-25 01:27:58)
> On 21 June 2013 14:14, Mike Turquette <mturquette@linaro.org> wrote:
> > Device Tree binding for the basic clock multiplexer, plus the setup
> > function to register the clock.  Based on the existing fixed-clock
> > binding.
> >
> > Includes minor beautification of clk-provider.h where some whitespace is
> > added and of_fixed_factor_clock_setup is relocated to maintain a
> > consistent style.
> >
> > Tero Kristo contributed helpful bug fixes to this patch.
> >
> > Signed-off-by: Mike Turquette <mturquette@linaro.org>
> > ---
> > Changes since v2:
> > * added hiword-mask property to the binding
> > * changed bit-shift property from u8 to u32 in the dt binding
> >
> > Changes since v1:
> > * pass shift value into clk_register_mux_table
> > * s/multiplexor/multiplexer/
> > * removed debug prints
> > * mask is u32, shift is u8
> > * DT property names use dashes instead of underscores
> > * DT property names are more verbose
> > * shift property is optional in binding and can be auto-generated from a
> >   full 32-bit mask
> >
> >  .../devicetree/bindings/clock/mux-clock.txt        | 79 ++++++++++++++++++++++
> >  drivers/clk/clk-mux.c                              | 65 +++++++++++++++++-
> >  include/linux/clk-provider.h                       |  5 +-
> >  3 files changed, 147 insertions(+), 2 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/clock/mux-clock.txt
> >
> > diff --git a/Documentation/devicetree/bindings/clock/mux-clock.txt b/Documentation/devicetree/bindings/clock/mux-clock.txt
> > new file mode 100644
> > index 0000000..54cb8d1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/mux-clock.txt
> > @@ -0,0 +1,79 @@
> > +Binding for simple mux clock.
> > +
> > +This binding uses the common clock binding[1].  It assumes a
> > +register-mapped multiplexer with multiple input clock signals or
> > +parents, one of which can be selected as output.  This clock does not
> > +gate or adjust the parent rate via a divider or multiplier.
> > +
> > +By default the "clocks" property lists the parents in the same order
> > +as they are programmed into the regster.  E.g:
> > +
> > +       clocks = <&foo_clock>, <&bar_clock>, <&baz_clock>;
> > +
> > +results in programming the register as follows:
> > +
> > +register value         selected parent clock
> > +0                      foo_clock
> > +1                      bar_clock
> > +2                      baz_clock
> > +
> > +Some clock controller IPs do not allow a value of zero to be programmed
> > +into the register, instead indexing begins at 1.  The optional property
> > +"index_one" modified the scheme as follows:
> > +
> > +register value         selected clock parent
> > +1                      foo_clock
> > +2                      bar_clock
> > +3                      baz_clock
> > +
> > +Additionally an optional table of bit and parent pairs may be supplied
> > +like so:
> > +
> > +       table = <&foo_clock 0x0>, <&bar_clock, 0x2>, <&baz_clock, 0x4>;
> > +
> > +where the first value in the pair is the parent clock and the second
> > +value is the bitfield to be programmed into the register.
> > +
> > +The binding must provide the register to control the mux and the mask
> > +for the corresponding control bits.  Optionally the number of bits to
> > +shift that mask if necessary.  If the shift value is missing it is the
> > +same as supplying a zero shift.
> > +
> > +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> > +
> > +Required properties:
> > +- compatible : shall be "mux-clock".
> > +- #clock-cells : from common clock binding; shall be set to 0.
> > +- clocks : link phandles of parent clocks
> > +- reg : base address for register controlling adjustable mux
> > +- bit-mask : arbitrary bitmask for programming the adjustable mux
> > +
> > +Optional properties:
> > +- clock-output-names : From common clock binding.
> > +- table : array of integer pairs defining parents & bitfield values
> > +- bit-shift : number of bits to shift the bit-mask, defaults to
> > +  (ffs(mask) - 1) if not present
> > +- index-starts-at-one : valid input select programming starts at 1, not
> > +  zero
> > +- hiword-mask : lower half of the register programs the mux, upper half
> > +  of the register indicates bits that were updated in the lower half
> > +
> > +Examples:
> > +       clock: clock@4a008100 {
> > +               compatible = "mux-clock";
> > +               #clock-cells = <0>;
> > +               clocks = <&clock_foo>, <&clock_bar>, <&clock_baz>;
> > +               reg = <0x4a008100 0x4>
> > +               mask = <0x3>;
> > +               index_one;
> > +       };
> > +
> > +       clock: clock@4a008100 {
> > +               #clock-cells = <0>;
> > +               compatible = "mux-clock";
> > +               clocks = <&clock_foo>, <&clock_bar>, <&clock_baz>;
> > +               reg = <0x4a008100 0x4>;
> > +               mask = <0x3>;
> > +               shift = <0>;
> > +               table = <&clock_foo 1>, <&clock_bar 2>, <&clock_baz 3>;
> > +       };
> > diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
> > index 614444c..4751bce 100644
> > --- a/drivers/clk/clk-mux.c
> > +++ b/drivers/clk/clk-mux.c
> > @@ -1,7 +1,7 @@
> >  /*
> >   * Copyright (C) 2011 Sascha Hauer, Pengutronix <s.hauer@pengutronix.de>
> >   * Copyright (C) 2011 Richard Zhao, Linaro <richard.zhao@linaro.org>
> > - * Copyright (C) 2011-2012 Mike Turquette, Linaro Ltd <mturquette@linaro.org>
> > + * Copyright (C) 2011-2013 Mike Turquette, Linaro Ltd <mturquette@linaro.org>
> >   *
> >   * 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
> > @@ -16,6 +16,8 @@
> >  #include <linux/slab.h>
> >  #include <linux/io.h>
> >  #include <linux/err.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> >
> >  /*
> >   * DOC: basic adjustable multiplexer clock that cannot gate
> > @@ -166,3 +168,64 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
> >                                       flags, reg, shift, mask, clk_mux_flags,
> >                                       NULL, lock);
> >  }
> > +
> > +#ifdef CONFIG_OF
> > +/**
> > + * of_mux_clk_setup() - Setup function for simple mux rate clock
> > + */
> > +void of_mux_clk_setup(struct device_node *node)
> > +{
> > +       struct clk *clk;
> > +       const char *clk_name = node->name;
> > +       void __iomem *reg;
> > +       int num_parents;
> > +       const char **parent_names;
> > +       int i;
> > +       u8 clk_mux_flags = 0;
> > +       u32 mask = 0;
> > +       u32 shift = 0;
> > +
> > +       of_property_read_string(node, "clock-output-names", &clk_name);
> > +
> > +       num_parents = of_clk_get_parent_count(node);
> > +       if (num_parents < 1) {
> > +               pr_err("%s: mux-clock %s must have parent(s)\n",
> > +                               __func__, node->name);
> > +               return;
> > +       }
> > +
> > +       parent_names = kzalloc((sizeof(char*) * num_parents),
> > +                       GFP_KERNEL);
> > +
> > +       for (i = 0; i < num_parents; i++)
> > +               parent_names[i] = of_clk_get_parent_name(node, i);
> > +
> > +       reg = of_iomap(node, 0);
> 
> As you mentioned above,  reg : base address for register controlling
> adjustable mux.
> 
> Each clock node has the reg property. It means that we have to
> allocate one page memory
> for each register since of_iomap() is used at here. We always have
> lots of registers with
> continuous address in SoC, like 0x40a00040, 0x40a00044, .... Then
> we'll waste too much
> memory in system.
> 
> How about only define common function to parse property? Make register
> mapping handled
> by vendor's clock driver.

This is probably a good idea. It is independent of the binding though.
The reg property can still be used as described above and however the OS
interacts with it is an implementation detail that can be sorted out
later.

I'll look into your suggestion though, since it could save some memory.

Regards,
Mike

> 
> Regards
> Haojian
Mark Rutland June 26, 2013, 8:40 a.m. UTC | #3 
Hi,

I have a couple of minor comments.

On Fri, Jun 21, 2013 at 07:14:14AM +0100, Mike Turquette wrote:
> Device Tree binding for the basic clock multiplexer, plus the setup
> function to register the clock.  Based on the existing fixed-clock
> binding.
> 
> Includes minor beautification of clk-provider.h where some whitespace is
> added and of_fixed_factor_clock_setup is relocated to maintain a
> consistent style.
> 
> Tero Kristo contributed helpful bug fixes to this patch.
> 
> Signed-off-by: Mike Turquette <mturquette@linaro.org>
> ---
> Changes since v2:
> * added hiword-mask property to the binding
> * changed bit-shift property from u8 to u32 in the dt binding
> 
> Changes since v1:
> * pass shift value into clk_register_mux_table
> * s/multiplexor/multiplexer/
> * removed debug prints
> * mask is u32, shift is u8
> * DT property names use dashes instead of underscores
> * DT property names are more verbose
> * shift property is optional in binding and can be auto-generated from a
>   full 32-bit mask
> 
>  .../devicetree/bindings/clock/mux-clock.txt        | 79 ++++++++++++++++++++++
>  drivers/clk/clk-mux.c                              | 65 +++++++++++++++++-
>  include/linux/clk-provider.h                       |  5 +-
>  3 files changed, 147 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/clock/mux-clock.txt
> 
> diff --git a/Documentation/devicetree/bindings/clock/mux-clock.txt b/Documentation/devicetree/bindings/clock/mux-clock.txt
> new file mode 100644
> index 0000000..54cb8d1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/mux-clock.txt
> @@ -0,0 +1,79 @@
> +Binding for simple mux clock.
> +
> +This binding uses the common clock binding[1].  It assumes a
> +register-mapped multiplexer with multiple input clock signals or
> +parents, one of which can be selected as output.  This clock does not
> +gate or adjust the parent rate via a divider or multiplier.
> +
> +By default the "clocks" property lists the parents in the same order
> +as they are programmed into the regster.  E.g:
> +
> +	clocks = <&foo_clock>, <&bar_clock>, <&baz_clock>;
> +
> +results in programming the register as follows:
> +
> +register value		selected parent clock
> +0			foo_clock
> +1			bar_clock
> +2			baz_clock
> +
> +Some clock controller IPs do not allow a value of zero to be programmed
> +into the register, instead indexing begins at 1.  The optional property
> +"index_one" modified the scheme as follows:

I assume this is meant to be "index-starts-at-one", given the code and
changelog?

> +
> +register value		selected clock parent
> +1			foo_clock
> +2			bar_clock
> +3			baz_clock
> +
> +Additionally an optional table of bit and parent pairs may be supplied
> +like so:
> +
> +	table = <&foo_clock 0x0>, <&bar_clock, 0x2>, <&baz_clock, 0x4>;
> +
> +where the first value in the pair is the parent clock and the second
> +value is the bitfield to be programmed into the register.
> +
> +The binding must provide the register to control the mux and the mask
> +for the corresponding control bits.  Optionally the number of bits to
> +shift that mask if necessary.  If the shift value is missing it is the
> +same as supplying a zero shift.
> +
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +Required properties:
> +- compatible : shall be "mux-clock".
> +- #clock-cells : from common clock binding; shall be set to 0.
> +- clocks : link phandles of parent clocks
> +- reg : base address for register controlling adjustable mux
> +- bit-mask : arbitrary bitmask for programming the adjustable mux
> +
> +Optional properties:
> +- clock-output-names : From common clock binding.
> +- table : array of integer pairs defining parents & bitfield values
> +- bit-shift : number of bits to shift the bit-mask, defaults to
> +  (ffs(mask) - 1) if not present
> +- index-starts-at-one : valid input select programming starts at 1, not
> +  zero
> +- hiword-mask : lower half of the register programs the mux, upper half
> +  of the register indicates bits that were updated in the lower half
> +
> +Examples:
> +	clock: clock@4a008100 {
> +		compatible = "mux-clock";
> +		#clock-cells = <0>;
> +		clocks = <&clock_foo>, <&clock_bar>, <&clock_baz>;
> +		reg = <0x4a008100 0x4>
> +		mask = <0x3>;
> +		index_one;

And here too?

[...]

> +/**
> + * of_mux_clk_setup() - Setup function for simple mux rate clock
> + */
> +void of_mux_clk_setup(struct device_node *node)
> +{
> +	struct clk *clk;
> +	const char *clk_name = node->name;
> +	void __iomem *reg;
> +	int num_parents;
> +	const char **parent_names;
> +	int i;
> +	u8 clk_mux_flags = 0;
> +	u32 mask = 0;
> +	u32 shift = 0;
> +
> +	of_property_read_string(node, "clock-output-names", &clk_name);
> +
> +	num_parents = of_clk_get_parent_count(node);
> +	if (num_parents < 1) {
> +		pr_err("%s: mux-clock %s must have parent(s)\n",
> +				__func__, node->name);
> +		return;
> +	}
> +
> +	parent_names = kzalloc((sizeof(char*) * num_parents),
> +			GFP_KERNEL);
> +
> +	for (i = 0; i < num_parents; i++)
> +		parent_names[i] = of_clk_get_parent_name(node, i);
> +
> +	reg = of_iomap(node, 0);

Would it not be a good idea to check this isn't NULL before we pass it
to clk_register_mux_table later?

> +
> +	if (of_property_read_u32(node, "bit-mask", &mask)) {
> +		pr_err("%s: missing bit-mask property for %s\n", __func__, node->name);
> +		return;
> +	}
> +
> +	if (of_property_read_u32(node, "bit-shift", &shift)) {
> +		shift = __ffs(mask);
> +		pr_debug("%s: bit-shift property defaults to 0x%x for %s\n",
> +				__func__, shift, node->name);
> +	}
> +
> +	if (of_property_read_bool(node, "index-starts-at-one"))
> +		clk_mux_flags |= CLK_MUX_INDEX_ONE;
> +
> +	if (of_property_read_bool(node, "hiword-mask"))
> +		clk_mux_flags |= CLK_MUX_HIWORD_MASK;
> +
> +	clk = clk_register_mux_table(NULL, clk_name, parent_names, num_parents,
> +			0, reg, (u8) shift, mask, clk_mux_flags,
> +			NULL, NULL);

Why do you need the (u8) cast on shift? Isn't that implicit?

Thanks,
Mark.
Mike Turquette July 9, 2013, 1:58 p.m. UTC | #4 
Quoting Mark Rutland (2013-06-26 01:40:58)
> Hi,
> 
> I have a couple of minor comments.
> 
> On Fri, Jun 21, 2013 at 07:14:14AM +0100, Mike Turquette wrote:
> > Device Tree binding for the basic clock multiplexer, plus the setup
> > function to register the clock.  Based on the existing fixed-clock
> > binding.
> > 
> > Includes minor beautification of clk-provider.h where some whitespace is
> > added and of_fixed_factor_clock_setup is relocated to maintain a
> > consistent style.
> > 
> > Tero Kristo contributed helpful bug fixes to this patch.
> > 
> > Signed-off-by: Mike Turquette <mturquette@linaro.org>
> > ---
> > Changes since v2:
> > * added hiword-mask property to the binding
> > * changed bit-shift property from u8 to u32 in the dt binding
> > 
> > Changes since v1:
> > * pass shift value into clk_register_mux_table
> > * s/multiplexor/multiplexer/
> > * removed debug prints
> > * mask is u32, shift is u8
> > * DT property names use dashes instead of underscores
> > * DT property names are more verbose
> > * shift property is optional in binding and can be auto-generated from a
> >   full 32-bit mask
> > 
> >  .../devicetree/bindings/clock/mux-clock.txt        | 79 ++++++++++++++++++++++
> >  drivers/clk/clk-mux.c                              | 65 +++++++++++++++++-
> >  include/linux/clk-provider.h                       |  5 +-
> >  3 files changed, 147 insertions(+), 2 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/clock/mux-clock.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/mux-clock.txt b/Documentation/devicetree/bindings/clock/mux-clock.txt
> > new file mode 100644
> > index 0000000..54cb8d1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/mux-clock.txt
> > @@ -0,0 +1,79 @@
> > +Binding for simple mux clock.
> > +
> > +This binding uses the common clock binding[1].  It assumes a
> > +register-mapped multiplexer with multiple input clock signals or
> > +parents, one of which can be selected as output.  This clock does not
> > +gate or adjust the parent rate via a divider or multiplier.
> > +
> > +By default the "clocks" property lists the parents in the same order
> > +as they are programmed into the regster.  E.g:
> > +
> > +     clocks = <&foo_clock>, <&bar_clock>, <&baz_clock>;
> > +
> > +results in programming the register as follows:
> > +
> > +register value               selected parent clock
> > +0                    foo_clock
> > +1                    bar_clock
> > +2                    baz_clock
> > +
> > +Some clock controller IPs do not allow a value of zero to be programmed
> > +into the register, instead indexing begins at 1.  The optional property
> > +"index_one" modified the scheme as follows:
> 
> I assume this is meant to be "index-starts-at-one", given the code and
> changelog?
> 
> > +
> > +register value               selected clock parent
> > +1                    foo_clock
> > +2                    bar_clock
> > +3                    baz_clock
> > +
> > +Additionally an optional table of bit and parent pairs may be supplied
> > +like so:
> > +
> > +     table = <&foo_clock 0x0>, <&bar_clock, 0x2>, <&baz_clock, 0x4>;
> > +
> > +where the first value in the pair is the parent clock and the second
> > +value is the bitfield to be programmed into the register.
> > +
> > +The binding must provide the register to control the mux and the mask
> > +for the corresponding control bits.  Optionally the number of bits to
> > +shift that mask if necessary.  If the shift value is missing it is the
> > +same as supplying a zero shift.
> > +
> > +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> > +
> > +Required properties:
> > +- compatible : shall be "mux-clock".
> > +- #clock-cells : from common clock binding; shall be set to 0.
> > +- clocks : link phandles of parent clocks
> > +- reg : base address for register controlling adjustable mux
> > +- bit-mask : arbitrary bitmask for programming the adjustable mux
> > +
> > +Optional properties:
> > +- clock-output-names : From common clock binding.
> > +- table : array of integer pairs defining parents & bitfield values
> > +- bit-shift : number of bits to shift the bit-mask, defaults to
> > +  (ffs(mask) - 1) if not present
> > +- index-starts-at-one : valid input select programming starts at 1, not
> > +  zero
> > +- hiword-mask : lower half of the register programs the mux, upper half
> > +  of the register indicates bits that were updated in the lower half
> > +
> > +Examples:
> > +     clock: clock@4a008100 {
> > +             compatible = "mux-clock";
> > +             #clock-cells = <0>;
> > +             clocks = <&clock_foo>, <&clock_bar>, <&clock_baz>;
> > +             reg = <0x4a008100 0x4>
> > +             mask = <0x3>;
> > +             index_one;
> 
> And here too?
> 
> [...]
> 
> > +/**
> > + * of_mux_clk_setup() - Setup function for simple mux rate clock
> > + */
> > +void of_mux_clk_setup(struct device_node *node)
> > +{
> > +     struct clk *clk;
> > +     const char *clk_name = node->name;
> > +     void __iomem *reg;
> > +     int num_parents;
> > +     const char **parent_names;
> > +     int i;
> > +     u8 clk_mux_flags = 0;
> > +     u32 mask = 0;
> > +     u32 shift = 0;
> > +
> > +     of_property_read_string(node, "clock-output-names", &clk_name);
> > +
> > +     num_parents = of_clk_get_parent_count(node);
> > +     if (num_parents < 1) {
> > +             pr_err("%s: mux-clock %s must have parent(s)\n",
> > +                             __func__, node->name);
> > +             return;
> > +     }
> > +
> > +     parent_names = kzalloc((sizeof(char*) * num_parents),
> > +                     GFP_KERNEL);
> > +
> > +     for (i = 0; i < num_parents; i++)
> > +             parent_names[i] = of_clk_get_parent_name(node, i);
> > +
> > +     reg = of_iomap(node, 0);
> 
> Would it not be a good idea to check this isn't NULL before we pass it
> to clk_register_mux_table later?
> 
> > +
> > +     if (of_property_read_u32(node, "bit-mask", &mask)) {
> > +             pr_err("%s: missing bit-mask property for %s\n", __func__, node->name);
> > +             return;
> > +     }
> > +
> > +     if (of_property_read_u32(node, "bit-shift", &shift)) {
> > +             shift = __ffs(mask);
> > +             pr_debug("%s: bit-shift property defaults to 0x%x for %s\n",
> > +                             __func__, shift, node->name);
> > +     }
> > +
> > +     if (of_property_read_bool(node, "index-starts-at-one"))
> > +             clk_mux_flags |= CLK_MUX_INDEX_ONE;
> > +
> > +     if (of_property_read_bool(node, "hiword-mask"))
> > +             clk_mux_flags |= CLK_MUX_HIWORD_MASK;
> > +
> > +     clk = clk_register_mux_table(NULL, clk_name, parent_names, num_parents,
> > +                     0, reg, (u8) shift, mask, clk_mux_flags,
> > +                     NULL, NULL);
> 
> Why do you need the (u8) cast on shift? Isn't that implicit?

All of your suggestions are good. I'll roll them into the next version.

Regards,
Mike
> 
> Thanks,
> Mark.
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/clock/mux-clock.txt b/Documentation/devicetree/bindings/clock/mux-clock.txt
new file mode 100644
index 0000000..54cb8d1
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/mux-clock.txt
@@ -0,0 +1,79 @@ 
+Binding for simple mux clock.
+
+This binding uses the common clock binding[1].  It assumes a
+register-mapped multiplexer with multiple input clock signals or
+parents, one of which can be selected as output.  This clock does not
+gate or adjust the parent rate via a divider or multiplier.
+
+By default the "clocks" property lists the parents in the same order
+as they are programmed into the regster.  E.g:
+
+	clocks = <&foo_clock>, <&bar_clock>, <&baz_clock>;
+
+results in programming the register as follows:
+
+register value		selected parent clock
+0			foo_clock
+1			bar_clock
+2			baz_clock
+
+Some clock controller IPs do not allow a value of zero to be programmed
+into the register, instead indexing begins at 1.  The optional property
+"index_one" modified the scheme as follows:
+
+register value		selected clock parent
+1			foo_clock
+2			bar_clock
+3			baz_clock
+
+Additionally an optional table of bit and parent pairs may be supplied
+like so:
+
+	table = <&foo_clock 0x0>, <&bar_clock, 0x2>, <&baz_clock, 0x4>;
+
+where the first value in the pair is the parent clock and the second
+value is the bitfield to be programmed into the register.
+
+The binding must provide the register to control the mux and the mask
+for the corresponding control bits.  Optionally the number of bits to
+shift that mask if necessary.  If the shift value is missing it is the
+same as supplying a zero shift.
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : shall be "mux-clock".
+- #clock-cells : from common clock binding; shall be set to 0.
+- clocks : link phandles of parent clocks
+- reg : base address for register controlling adjustable mux
+- bit-mask : arbitrary bitmask for programming the adjustable mux
+
+Optional properties:
+- clock-output-names : From common clock binding.
+- table : array of integer pairs defining parents & bitfield values
+- bit-shift : number of bits to shift the bit-mask, defaults to
+  (ffs(mask) - 1) if not present
+- index-starts-at-one : valid input select programming starts at 1, not
+  zero
+- hiword-mask : lower half of the register programs the mux, upper half
+  of the register indicates bits that were updated in the lower half
+
+Examples:
+	clock: clock@4a008100 {
+		compatible = "mux-clock";
+		#clock-cells = <0>;
+		clocks = <&clock_foo>, <&clock_bar>, <&clock_baz>;
+		reg = <0x4a008100 0x4>
+		mask = <0x3>;
+		index_one;
+	};
+
+	clock: clock@4a008100 {
+		#clock-cells = <0>;
+		compatible = "mux-clock";
+		clocks = <&clock_foo>, <&clock_bar>, <&clock_baz>;
+		reg = <0x4a008100 0x4>;
+		mask = <0x3>;
+		shift = <0>;
+		table = <&clock_foo 1>, <&clock_bar 2>, <&clock_baz 3>;
+	};
diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
index 614444c..4751bce 100644
--- a/drivers/clk/clk-mux.c
+++ b/drivers/clk/clk-mux.c
@@ -1,7 +1,7 @@ 
 /*
  * Copyright (C) 2011 Sascha Hauer, Pengutronix <s.hauer@pengutronix.de>
  * Copyright (C) 2011 Richard Zhao, Linaro <richard.zhao@linaro.org>
- * Copyright (C) 2011-2012 Mike Turquette, Linaro Ltd <mturquette@linaro.org>
+ * Copyright (C) 2011-2013 Mike Turquette, Linaro Ltd <mturquette@linaro.org>
  *
  * 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
@@ -16,6 +16,8 @@ 
 #include <linux/slab.h>
 #include <linux/io.h>
 #include <linux/err.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
 
 /*
  * DOC: basic adjustable multiplexer clock that cannot gate
@@ -166,3 +168,64 @@  struct clk *clk_register_mux(struct device *dev, const char *name,
 				      flags, reg, shift, mask, clk_mux_flags,
 				      NULL, lock);
 }
+
+#ifdef CONFIG_OF
+/**
+ * of_mux_clk_setup() - Setup function for simple mux rate clock
+ */
+void of_mux_clk_setup(struct device_node *node)
+{
+	struct clk *clk;
+	const char *clk_name = node->name;
+	void __iomem *reg;
+	int num_parents;
+	const char **parent_names;
+	int i;
+	u8 clk_mux_flags = 0;
+	u32 mask = 0;
+	u32 shift = 0;
+
+	of_property_read_string(node, "clock-output-names", &clk_name);
+
+	num_parents = of_clk_get_parent_count(node);
+	if (num_parents < 1) {
+		pr_err("%s: mux-clock %s must have parent(s)\n",
+				__func__, node->name);
+		return;
+	}
+
+	parent_names = kzalloc((sizeof(char*) * num_parents),
+			GFP_KERNEL);
+
+	for (i = 0; i < num_parents; i++)
+		parent_names[i] = of_clk_get_parent_name(node, i);
+
+	reg = of_iomap(node, 0);
+
+	if (of_property_read_u32(node, "bit-mask", &mask)) {
+		pr_err("%s: missing bit-mask property for %s\n", __func__, node->name);
+		return;
+	}
+
+	if (of_property_read_u32(node, "bit-shift", &shift)) {
+		shift = __ffs(mask);
+		pr_debug("%s: bit-shift property defaults to 0x%x for %s\n",
+				__func__, shift, node->name);
+	}
+
+	if (of_property_read_bool(node, "index-starts-at-one"))
+		clk_mux_flags |= CLK_MUX_INDEX_ONE;
+
+	if (of_property_read_bool(node, "hiword-mask"))
+		clk_mux_flags |= CLK_MUX_HIWORD_MASK;
+
+	clk = clk_register_mux_table(NULL, clk_name, parent_names, num_parents,
+			0, reg, (u8) shift, mask, clk_mux_flags,
+			NULL, NULL);
+
+	if (!IS_ERR(clk))
+		of_clk_add_provider(node, of_clk_src_simple_get, clk);
+}
+EXPORT_SYMBOL_GPL(of_mux_clk_setup);
+CLK_OF_DECLARE(mux_clk, "mux-clock", of_mux_clk_setup);
+#endif
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 8730cb9..24a04b8 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -340,7 +340,7 @@  struct clk *clk_register_mux_table(struct device *dev, const char *name,
 		void __iomem *reg, u8 shift, u32 mask,
 		u8 clk_mux_flags, u32 *table, spinlock_t *lock);
 
-void of_fixed_factor_clk_setup(struct device_node *node);
+void of_mux_clk_setup(struct device_node *node);
 
 /**
  * struct clk_fixed_factor - fixed multiplier and divider clock
@@ -361,10 +361,13 @@  struct clk_fixed_factor {
 };
 
 extern struct clk_ops clk_fixed_factor_ops;
+
 struct clk *clk_register_fixed_factor(struct device *dev, const char *name,
 		const char *parent_name, unsigned long flags,
 		unsigned int mult, unsigned int div);
 
+void of_fixed_factor_clk_setup(struct device_node *node);
+
 /***
  * struct clk_composite - aggregate clock of mux, divider and gate clocks
  *