diff mbox

[RFC] CLK: Allow parent clock and rate to be configured in DT.

Message ID 20130319170933.28337.50448.stgit@localhost (mailing list archive)
State New, archived
Headers show

Commit Message

Martin Fuzzey March 19, 2013, 5:09 p.m. UTC
Even on platforms where the entire clock tree is not represented in the DT
it can still be useful to allow parents and rates to be set from the DT.

An example of such a case is when a multiplexable clock output from a SOC
is used to supply external chips (eg an audio codec connected to the i.MX53
cko1 pin).

The cko1 pin can output various internal clock signals but, in
order to obtain a suitable frequency for the codec, an appropriate parent must
be selected.

Another example is setting root clock dividers.

This is board specific rather than device driver or platform clock framework
specific information and thus would be better in the DT.

Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
---
Sending as RFC for the moment in a single patch together with an example
for i.MX53.  Will split if this goes anywhere.
---
 .../bindings/clock/clock-configuration.txt         |   35 +++++++++
 arch/arm/mach-imx/clk-imx51-imx53.c                |    9 +-
 drivers/clk/Makefile                               |    1 
 drivers/clk/clk-configuration.c                    |   79 ++++++++++++++++++++
 include/linux/clk.h                                |    5 +
 5 files changed, 122 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/clock-configuration.txt
 create mode 100644 drivers/clk/clk-configuration.c

Comments

Sascha Hauer March 25, 2013, 10:17 a.m. UTC | #1
On Tue, Mar 19, 2013 at 06:09:33PM +0100, Martin Fuzzey wrote:
> Even on platforms where the entire clock tree is not represented in the DT
> it can still be useful to allow parents and rates to be set from the DT.
> 
> An example of such a case is when a multiplexable clock output from a SOC
> is used to supply external chips (eg an audio codec connected to the i.MX53
> cko1 pin).
> 
> The cko1 pin can output various internal clock signals but, in
> order to obtain a suitable frequency for the codec, an appropriate parent must
> be selected.
> 
> Another example is setting root clock dividers.
> 
> This is board specific rather than device driver or platform clock framework
> specific information and thus would be better in the DT.

I see what the patch does and that it could be very useful, but there's
a problem: The devicetree is for hardware *description*, not
*configuration*.

> +For example:
> +	clock-configuration {
> +		compatible = "clock-configuration";
> +		clko1 {
> +			clocks = <&clks 160>; /* cko1_sel */
> +			parent = <&clks 114>; /* pll3_sw */
> +		};
> +
> +		esdhca {
> +			clocks = <&clks 102>; /* esdhc_a_podf */
> +			clock-frequency = <200000000>;
> +		};

This example shows this. For some reason we adjust the esdhc frequency
to 200MHz in the code currently, but this is because it matches our
current usecase. Once you move this into devicetree, we can't change
this anymore in the kernel, even if we find a much better way to adjust
the frequency in the future (i.e. smaller values might be good for power
savings, higher values might increase performance, we even might
dynamically change this frequency).


So no, this shouldn't be in the devicetree, even though it's very
tempting to do so.

I wonder when someone comes up with a 'configtree' where we could put in
such stuff.

Sascha

> +
> +		esdhcb {
> +			clocks = <&clks 103>; /* esdhc_b_podf */
> +			clock-frequency = <200000000>;
> +		};
> +	};
> +
> +
> diff --git a/arch/arm/mach-imx/clk-imx51-imx53.c b/arch/arm/mach-imx/clk-imx51-imx53.c
> index 872a7bc..fd74795 100644
> --- a/arch/arm/mach-imx/clk-imx51-imx53.c
> +++ b/arch/arm/mach-imx/clk-imx51-imx53.c
> @@ -432,7 +432,7 @@ int __init mx51_clocks_init(unsigned long rate_ckil, unsigned long rate_osc,
>  	val |= 1 << 23;
>  	writel(val, MXC_CCM_CLPCR);
>  
> -	return 0;
> +	return of_clk_configure();
>  }
>  
>  int __init mx53_clocks_init(unsigned long rate_ckil, unsigned long rate_osc,
> @@ -523,10 +523,6 @@ int __init mx53_clocks_init(unsigned long rate_ckil, unsigned long rate_osc,
>  	clk_register_clkdev(clk[dummy], "ahb", "sdhci-esdhc-imx53.3");
>  	clk_register_clkdev(clk[esdhc4_per_gate], "per", "sdhci-esdhc-imx53.3");
>  
> -	/* set SDHC root clock to 200MHZ*/
> -	clk_set_rate(clk[esdhc_a_podf], 200000000);
> -	clk_set_rate(clk[esdhc_b_podf], 200000000);
> -
>  	/* System timer */
>  	mxc_timer_init(MX53_IO_ADDRESS(MX53_GPT1_BASE_ADDR), MX53_INT_GPT);
>  
> @@ -536,8 +532,7 @@ int __init mx53_clocks_init(unsigned long rate_ckil, unsigned long rate_osc,
>  
>  	r = clk_round_rate(clk[usboh3_per_gate], 54000000);
>  	clk_set_rate(clk[usboh3_per_gate], r);
> -
> -	return 0;
> +	return of_clk_configure();
>  }
>  
>  #ifdef CONFIG_OF
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 300d477..bf364dd 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_COMMON_CLK)	+= clk-fixed-factor.o
>  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-configuration.o
>  
>  # SoCs specific
>  obj-$(CONFIG_ARCH_BCM2835)	+= clk-bcm2835.o
> diff --git a/drivers/clk/clk-configuration.c b/drivers/clk/clk-configuration.c
> new file mode 100644
> index 0000000..ee70619
> --- /dev/null
> +++ b/drivers/clk/clk-configuration.c
> @@ -0,0 +1,79 @@
> +/*
> + * 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.
> + *
> + * Device tree clock parent, rate configuration
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/of.h>
> +
> +
> +static int configure_one_clock(struct clk *clk, struct device_node *np)
> +{
> +	int ret;
> +	struct of_phandle_args clkspec;
> +	struct clk *parent;
> +	u32 rate;
> +
> +	ret = of_parse_phandle_with_args(np, "parent", "#clock-cells", 0,
> +					&clkspec);
> +	if (!ret) {
> +		parent = of_clk_get_from_provider(&clkspec);
> +		if (!IS_ERR(parent)) {
> +			ret = clk_set_parent(clk, parent);
> +			clk_put(parent);
> +		}
> +		of_node_put(clkspec.np);
> +		if (ret)
> +			goto err;
> +	}
> +
> +	ret = 0;
> +	if (!of_property_read_u32(np, "clock-frequency", &rate))
> +		ret = clk_set_rate(clk, rate);
> +
> +err:
> +	return ret;
> +
> +}
> +
> +/**
> + * of_clk_configure - configure clocks from device tree
> + *
> + * Allows parent and rate to be set from nodes having
> + * clock-configuration compatible property.
> + *
> + * See binding documentation for example
> + *
> + * Returns 0 on success, -EERROR otherwise.
> + */
> +int of_clk_configure()
> +{
> +	struct device_node *config_node, *np;
> +	struct clk *clk;
> +	int err_count = 0;
> +	int ret = 0;
> +
> +	for_each_compatible_node(config_node, NULL, "clock-configuration") {
> +		for_each_child_of_node(config_node, np) {
> +			clk = of_clk_get(np, 0);
> +			if (IS_ERR(clk)) {
> +				pr_warn("%s: Failed to obtain clock configuration for %s : %d\n", __func__, np->name);
> +				err_count++;
> +			} else {
> +				if (configure_one_clock(clk, np))
> +					err_count++;
> +				clk_put(clk);
> +			}
> +		}
> +	}
> +
> +	if (err_count) {
> +		pr_warn("%s: Failed %d clocks\n", __func__, err_count);
> +		ret = -EINVAL;
> +	}
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(of_clk_configure);
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index b3ac22d..4f7f605 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -368,6 +368,7 @@ struct of_phandle_args;
>  struct clk *of_clk_get(struct device_node *np, int index);
>  struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
>  struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec);
> +int of_clk_configure(void);
>  #else
>  static inline struct clk *of_clk_get(struct device_node *np, int index)
>  {
> @@ -378,6 +379,10 @@ static inline struct clk *of_clk_get_by_name(struct device_node *np,
>  {
>  	return ERR_PTR(-ENOENT);
>  }
> +static inline int of_clk_configure(void)
> +{
> +	return 0;
> +}
>  #endif
>  
>  #endif
> 
>
Martin Fuzzey March 25, 2013, 11:07 a.m. UTC | #2
Hi Sascha,

thanks for the comments.

[corrected Mike's email address - I originally sent it to the old one]

On 25/03/13 11:17, Sascha Hauer wrote:
> On Tue, Mar 19, 2013 at 06:09:33PM +0100, Martin Fuzzey wrote:
>> Even on platforms where the entire clock tree is not represented in the DT
>> it can still be useful to allow parents and rates to be set from the DT.
>>
>> An example of such a case is when a multiplexable clock output from a SOC
>> is used to supply external chips (eg an audio codec connected to the i.MX53
>> cko1 pin).
>>
>> The cko1 pin can output various internal clock signals but, in
>> order to obtain a suitable frequency for the codec, an appropriate parent must
>> be selected.
>>
>> Another example is setting root clock dividers.
>>
>> This is board specific rather than device driver or platform clock framework
>> specific information and thus would be better in the DT.
> I see what the patch does and that it could be very useful, but there's
> a problem: The devicetree is for hardware *description*, not
> *configuration*.
>
>> +For example:
>> +	clock-configuration {
>> +		compatible = "clock-configuration";
>> +		clko1 {
>> +			clocks = <&clks 160>; /* cko1_sel */
>> +			parent = <&clks 114>; /* pll3_sw */
>> +		};
>> +
>> +		esdhca {
>> +			clocks = <&clks 102>; /* esdhc_a_podf */
>> +			clock-frequency = <200000000>;
>> +		};
> This example shows this. For some reason we adjust the esdhc frequency
> to 200MHz in the code currently, but this is because it matches our
> current usecase. Once you move this into devicetree, we can't change
> this anymore in the kernel, even if we find a much better way to adjust
> the frequency in the future (i.e. smaller values might be good for power
> savings, higher values might increase performance, we even might
> dynamically change this frequency).
>
Yes but the problem is that the rates are currently set in 
clk-imx51-53.c which should be generic
to all such platforms whereas, as you point out, there may well be valid 
reasons to choose
different values depending on the board design and / or intended 
application.

The cko case is even worse - due to a board design decision that clock 
needs to have
a frequency suitable for supplying some external chip. We don't want 
board specific code in
the platform clock code and we're trying to get away from board files...

> So no, this shouldn't be in the devicetree, even though it's very
> tempting to do so.
>
> I wonder when someone comes up with a 'configtree' where we could put in
> such stuff.
I don't understand why we need a seperate tree for this why can't just
a subtree of the DT be used?

After all the DT already contains "configuration" nodes such as "chosen".

Indeed Documentation/devicetree/usage-model.txt  says:
     "The chosen node may also optionally contain an arbitrary number of
     additional properties for platform-specific configuration data."

So what stops us simply placing the clock-configuration node above under 
chosen? (which already works - just tested it)

If the issue is that hardware vendors should be able to supply a DT 
without knowing
the configuration parameters couldn't that be resolved by letting the 
bootloader
merge DT subtrees this would give us "configtree" without adding more 
code to
the kernel to handle it.

Regards,

Martin
Sascha Hauer March 25, 2013, 1:29 p.m. UTC | #3
On Mon, Mar 25, 2013 at 12:07:51PM +0100, Martin Fuzzey wrote:
> Hi Sascha,
> 
> thanks for the comments.
> 
> [corrected Mike's email address - I originally sent it to the old one]
> 
> On 25/03/13 11:17, Sascha Hauer wrote:
> >On Tue, Mar 19, 2013 at 06:09:33PM +0100, Martin Fuzzey wrote:
> >>Even on platforms where the entire clock tree is not represented in the DT
> >>it can still be useful to allow parents and rates to be set from the DT.
> >>
> >>An example of such a case is when a multiplexable clock output from a SOC
> >>is used to supply external chips (eg an audio codec connected to the i.MX53
> >>cko1 pin).
> >>
> >>The cko1 pin can output various internal clock signals but, in
> >>order to obtain a suitable frequency for the codec, an appropriate parent must
> >>be selected.
> >>
> >>Another example is setting root clock dividers.
> >>
> >>This is board specific rather than device driver or platform clock framework
> >>specific information and thus would be better in the DT.
> >I see what the patch does and that it could be very useful, but there's
> >a problem: The devicetree is for hardware *description*, not
> >*configuration*.
> >
> >>+For example:
> >>+	clock-configuration {
> >>+		compatible = "clock-configuration";
> >>+		clko1 {
> >>+			clocks = <&clks 160>; /* cko1_sel */
> >>+			parent = <&clks 114>; /* pll3_sw */
> >>+		};
> >>+
> >>+		esdhca {
> >>+			clocks = <&clks 102>; /* esdhc_a_podf */
> >>+			clock-frequency = <200000000>;
> >>+		};
> >This example shows this. For some reason we adjust the esdhc frequency
> >to 200MHz in the code currently, but this is because it matches our
> >current usecase. Once you move this into devicetree, we can't change
> >this anymore in the kernel, even if we find a much better way to adjust
> >the frequency in the future (i.e. smaller values might be good for power
> >savings, higher values might increase performance, we even might
> >dynamically change this frequency).
> >
> Yes but the problem is that the rates are currently set in
> clk-imx51-53.c which should be generic
> to all such platforms whereas, as you point out, there may well be
> valid reasons to choose
> different values depending on the board design and / or intended
> application.

Put it differently. OpenBSD might have much better clock support.
Imagine it can dynamically figure out the correct esdhc frequencies
for different usecases on the fly. In this situation it would be
counterproductive if Linux requires static values for these in the
devicetree.

> 
> The cko case is even worse - due to a board design decision that
> clock needs to have
> a frequency suitable for supplying some external chip. We don't want
> board specific code in
> the platform clock code and we're trying to get away from board files...

The codec could be provided a phandle to the cko clk. This is hardware
description and is fine for putting into the devicetree.

> 
> >So no, this shouldn't be in the devicetree, even though it's very
> >tempting to do so.
> >
> >I wonder when someone comes up with a 'configtree' where we could put in
> >such stuff.
> I don't understand why we need a seperate tree for this why can't just
> a subtree of the DT be used?
> 
> After all the DT already contains "configuration" nodes such as "chosen".
> 
> Indeed Documentation/devicetree/usage-model.txt  says:
>     "The chosen node may also optionally contain an arbitrary number of
>     additional properties for platform-specific configuration data."
> 
> So what stops us simply placing the clock-configuration node above
> under chosen? (which already works - just tested it)

What stops us doing so is that currently we have the policy that the
devicetree is hardware description only, even if there are already
counteraxamples in the devicetree.

I know that we currently have no place to put such information. There
recently was a similar discussion with CMA descriptions in the
devicetree and I remember several other discussions of the same type,
all of which ended at the dont-know-but-not-in-the-devicetree point.

Sascha
Martin Fuzzey March 26, 2013, 11:12 a.m. UTC | #4
On 25/03/13 14:29, Sascha Hauer wrote:
> Put it differently. OpenBSD might have much better clock support. 
> Imagine it can dynamically figure out the correct esdhc frequencies 
> for different usecases on the fly. In this situation it would be 
> counterproductive if Linux requires static values for these in the 
> devicetree. 
It shouldn't *require* them.
If they are in a separate subtree of the DT OpenBSD would be free to 
ignore them and use its better method.
>> The cko case is even worse - due to a board design decision that
>> clock needs to have
>> a frequency suitable for supplying some external chip. We don't want
>> board specific code in
>> the platform clock code and we're trying to get away from board files...
> The codec could be provided a phandle to the cko clk. This is hardware
> description and is fine for putting into the devicetree.
Sure but just the phandle isn't enough.
We also need the frequency and the parent.

For the frequency the driver could, and maybe should, set it (and indeed 
I've submitted a patch for this for the sgtl5000 driver).

But IMHO it's not the driver's business to be setting the parent clock 
(the driver just gets a phandle and
shouldn't know if it can be reparented).

Maybe if and when the clock framework learns how to reparent clocks 
during set_rate() this problem may go away but
I'm not entirely comfortable with that - I'm sure there are cases where 
it will be better to manually specify the parent clock.

> I know that we currently have no place to put such information. There
> recently was a similar discussion with CMA descriptions in the
> devicetree and I remember several other discussions of the same type,
> all of which ended at the dont-know-but-not-in-the-devicetree point.
Yes, I looked up the CMA discussion which, AFAICT ended with a proposition
to use chosen to which there was no reply :(

I quite understand (and agree with) not wanting to scatter linux 
specific configuration
data all over the DT but, given that there are clearly usecases for this 
type of
configuration,  I fail to see the conceptual difference between:

1) Pure hardware DT plus a completely seperate "config-tree"
2) DT with a configuration node ("chosen" or probably better something 
like "linux-config")

Except that 1) requires more work to implement.
Even if the DT parser and tooling were reused it would still require:
* A means of passing another blob to the kernel
* A means of providing the parsed data to drivers

Both of these are obtained "for free" with solution 2), whilst still 
retaining the
separation of the tree into "hardware" and "configuration"

It would be possible for hardware vendors to ship the pure hardware part 
and then
add the configuration node either at runtime in the bootloader or by a 
preprocessing
tool "dtcat"?

Other OSs would simply ignore the "linux-config" node (and could add 
"bsd-config" node too)

Just my 2 centimes,

Martin
Sascha Hauer March 27, 2013, 8:59 a.m. UTC | #5
On Tue, Mar 26, 2013 at 12:12:22PM +0100, Martin Fuzzey wrote:
> On 25/03/13 14:29, Sascha Hauer wrote:
> >Put it differently. OpenBSD might have much better clock support.
> >Imagine it can dynamically figure out the correct esdhc
> >frequencies for different usecases on the fly. In this situation
> >it would be counterproductive if Linux requires static values for
> >these in the devicetree.
> It shouldn't *require* them.
> If they are in a separate subtree of the DT OpenBSD would be free to
> ignore them and use its better method.
> >>The cko case is even worse - due to a board design decision that
> >>clock needs to have
> >>a frequency suitable for supplying some external chip. We don't want
> >>board specific code in
> >>the platform clock code and we're trying to get away from board files...
> >The codec could be provided a phandle to the cko clk. This is hardware
> >description and is fine for putting into the devicetree.
> Sure but just the phandle isn't enough.
> We also need the frequency and the parent.
> 
> For the frequency the driver could, and maybe should, set it (and
> indeed I've submitted a patch for this for the sgtl5000 driver).
> 
> But IMHO it's not the driver's business to be setting the parent
> clock (the driver just gets a phandle and
> shouldn't know if it can be reparented).
> 
> Maybe if and when the clock framework learns how to reparent clocks
> during set_rate() this problem may go away but
> I'm not entirely comfortable with that - I'm sure there are cases
> where it will be better to manually specify the parent clock.
> 
> >I know that we currently have no place to put such information. There
> >recently was a similar discussion with CMA descriptions in the
> >devicetree and I remember several other discussions of the same type,
> >all of which ended at the dont-know-but-not-in-the-devicetree point.
> Yes, I looked up the CMA discussion which, AFAICT ended with a proposition
> to use chosen to which there was no reply :(
> 
> I quite understand (and agree with) not wanting to scatter linux
> specific configuration
> data all over the DT but, given that there are clearly usecases for
> this type of
> configuration,  I fail to see the conceptual difference between:
> 
> 1) Pure hardware DT plus a completely seperate "config-tree"
> 2) DT with a configuration node ("chosen" or probably better
> something like "linux-config")
> 
> Except that 1) requires more work to implement.
> Even if the DT parser and tooling were reused it would still require:
> * A means of passing another blob to the kernel
> * A means of providing the parsed data to drivers
> 
> Both of these are obtained "for free" with solution 2), whilst still
> retaining the
> separation of the tree into "hardware" and "configuration"
> 
> It would be possible for hardware vendors to ship the pure hardware
> part and then
> add the configuration node either at runtime in the bootloader or by
> a preprocessing
> tool "dtcat"?
> 
> Other OSs would simply ignore the "linux-config" node (and could add
> "bsd-config" node too)

I'm absolutely not against doing this. I'm only against introducing this
through the back door. I think we need a good place in the devicetree
where to put such configuration stuff and we need some agreement what
(and what not) should be there.

Sascha
Fabio Estevam April 4, 2013, 11:08 p.m. UTC | #6
Hi Sascha,

On Mon, Mar 25, 2013 at 7:17 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:

>> +For example:
>> +     clock-configuration {
>> +             compatible = "clock-configuration";
>> +             clko1 {
>> +                     clocks = <&clks 160>; /* cko1_sel */
>> +                     parent = <&clks 114>; /* pll3_sw */
>> +             };
>> +
>> +             esdhca {
>> +                     clocks = <&clks 102>; /* esdhc_a_podf */
>> +                     clock-frequency = <200000000>;
>> +             };
>
> This example shows this. For some reason we adjust the esdhc frequency
> to 200MHz in the code currently, but this is because it matches our
> current usecase. Once you move this into devicetree, we can't change
> this anymore in the kernel, even if we find a much better way to adjust
> the frequency in the future (i.e. smaller values might be good for power
> savings, higher values might increase performance, we even might
> dynamically change this frequency).

What if we use Martin's idea, but without the "clock-frequency" option
and only pass the parent information? This way the driver can find the
better clock as you described. Something like:

     clock-parent {
             compatible = "clock-parent";
             clko1 {
                     clocks = <&clks 160>; /* cko1_sel */
                    parent = <&clks 114>; /* pll3_sw */
             };

This could be useful for removing the imx6q_sabrelite_cko1_setup()
function from arch/arm/mach-imx/mach-imx6q.c.

I would like to add audio support for another board and would like to
avoid to do the same as imx6q_sabrelite_cko1_setup()  for setting up
the CLKO, if possible.

Regards,

Fabio Estevam
Matt Sealey April 6, 2013, 1:07 a.m. UTC | #7
On Thu, Apr 4, 2013 at 6:08 PM, Fabio Estevam <festevam@gmail.com> wrote:
>
> This could be useful for removing the imx6q_sabrelite_cko1_setup()
> function from arch/arm/mach-imx/mach-imx6q.c.
>
> I would like to add audio support for another board and would like to
> avoid to do the same as imx6q_sabrelite_cko1_setup()  for setting up
> the CLKO, if possible.

You know, we have the same problem on one of our designs here (CLKO is
used on MX53 for audio codec and camera device, shared) - the current
solution is hack it into platform support in a BSP kernel.

If we move to device tree, we know and you know the solution already;
all this clock setup HAS to be done in the bootloader.

The device tree really, really is only a way to describe the
configuration as it exists or to describe alternatives - for instance,
a clock with 10 parents may be described as having 10 parents, and the
binding (in complicated cases) or driver (if it is simple as a value
from 0 to 7 and the field width is 3 bits for example) will determine
how that alternative can be selected (and by consequence, what the
current setting is).

The device tree concept is NOT a place to dump configuration items for
your board as hardcoded values to try and force something you could
have done elsewhere. On i.MX53 you cannot boot a kernel verbatim - you
at least have to run through the Boot ROM and supply a DCD table or
plugins first. This is where you figure things like this out.

In a case where you have, say, an audio codec and it has a dynamic
input clock that you want to change on the fly, first of all you would
not hardcode a frequency into a device tree, second of all there is
nothing stopping you from doing that right now anyway. If the clock is
static and fixed frequency and can only be turned on and off, there
are clock bindings for this already. If it is static and can never be
turned off, then there are clock bindings for this too, and in this
case the proviso is that the clock is ALREADY turned on and ALREADY
stable before you enter the kernel otherwise the description is by
it's very definition invalid.

If the clock frequency in hardware is not what you wanted when the
driver comes up, and you only have an on/off switch, it is not up to
the device tree binding to describe how to go about configuring the
system to make sure. You cannot in good conscience put a clock
frequency "to be set later" in the device tree along with a clock
phandle, simply because that means the device tree no longer describes
the hardware accurately - the clock phandle is a valid clock, the
hardware will tell you a frequency from registers in the clock driver,
the device tree will disagree. How do you know which one is the
correct value, or if the frequency in the tree is a suggestion rather
than a description?

I am totally against this (sorry Sascha..). Let's put some effort into
fixing the bootloaders rather than trying to use the device tree to
enforce the ridiculous assumption that "Linux kernel does not trust
the bootloader". Once the bindings and trees are out of the kernel
source, you're going to HAVE to trust what the bootloader passes, be
it pregenerated compiled blob (which needs to be written to match the
hardware state the bootloader finishes up at) or dynamically generated
at runtime during the boot process (which can describe to the bit what
the hardware is doing). If you are testing this on Beagle XM you can
fix your U-Boot easily. New boards will have to be designed *with the
idea of device trees and working configurations in mind* - that is a
fact of life, in fact this is how it should be in reality these days
anyway (most HW designers will do initial bringup of the board - at
least a minimal working configuration, in a restricted environment
where they can use test pads to measure clocks, voltage outputs and
levels of devices to ensure both production was successful and that
the design is in itself electrically sound. This code 99% of the time
ends up in the bootloader... sometimes not, when the board was
designed by one group and the software written by the community, but
in this case the community needs to learn board bringup and proper
behavior...)
Matt Sealey April 6, 2013, 1:33 a.m. UTC | #8
Okay now I have finished my criticism, I will make some productive
suggestions :)

How about we implement a system as follows; modify the clock framework
and bindings such that we not only describe all the parents possible
for a clock, we also enter into the device tree the current parent? I
actually didn't fully realise it until now, but that just isn't in
there (it's derived from the list in the parents property and the
current hardware setting!).

That way, at least on clock initialization it will be able to warn
that the parent in the device tree is not the one in hardware, and -
with suitable options or by default - warn that it is setting the
parent to the one in the device tree. That solves the "which parent I
want" problem without involving ACTUAL configuration, and meets
Sascha's requirement that it is not done by a back door of
configuration items. So to summarize, the process is

* Device tree describes all parents of a clock in parents property
* Device tree describes which parent is the "current" parent
* Linux parses the tree and registers the clock - checks if the parent
in HW is the same as the one in DT
 - if it is not the same, print a horrific warning to the kernel log
 - if we desire to (and it should be possible to make this a debug
option for clock subsystem) set the parent to the one in the DT (also
printing a horrific warning that it has done so)

To solve the issue of "setting clocks to fixed rates", I would suggest
we do it by way of a similar system to the cpu frequency drivers or
regulators, and use operating points or ranges. For a good chunk of
devices, actual clock gating, rate changing and power management are
possible and highly useful in myriad different ways depending on the
device. In this way, an audio codec may end up with a list of
"operating-points" in a node which describes all the valid clock
rates. For example the SGTL5000 can accept many input clocks, the
CS42L52 audio codec can also accept many input clock rates - and the
audio frequency support is determined by those clock rates. For the
latter example, this huge table;

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/sound/soc/codecs/cs42l52.c#n690

.. is a move-to-device-tree candidate. The driver could pick a
suitable operating point from a range (which it could determine by the
rates the clock phandle supports), and you can even "suggest" one the
same way we do for regulators - only supply a single value. That way,
everything looks like it is being done on the basis of power
management, WOULD be used as power management on a vast chunk of
devices, and can be "restricted" to only work for specific values on
systems which didn't have the dynamism of others (for instance the
MC13892 PMIC has many regulators with wide ranges of voltages, but on
most systems they are tied to specific, fixed voltage rails so giving
a range makes no sense, and there's no real driver that can take
responsibility for setting them. But some use cases may tie the same
regulator output to a dynamic voltage rail, so you can't get away with
just being fixed or just being totally rangey)

How does that sound (audio codec pun not entirely intentional..)?
Matt Sealey <matt@genesi-usa.com>
Product Development Analyst, Genesi USA, Inc.


On Fri, Apr 5, 2013 at 8:07 PM, Matt Sealey <matt@genesi-usa.com> wrote:
> On Thu, Apr 4, 2013 at 6:08 PM, Fabio Estevam <festevam@gmail.com> wrote:
>>
>> This could be useful for removing the imx6q_sabrelite_cko1_setup()
>> function from arch/arm/mach-imx/mach-imx6q.c.
>>
>> I would like to add audio support for another board and would like to
>> avoid to do the same as imx6q_sabrelite_cko1_setup()  for setting up
>> the CLKO, if possible.
>
> You know, we have the same problem on one of our designs here (CLKO is
> used on MX53 for audio codec and camera device, shared) - the current
> solution is hack it into platform support in a BSP kernel.
>
> If we move to device tree, we know and you know the solution already;
> all this clock setup HAS to be done in the bootloader.
>
> The device tree really, really is only a way to describe the
> configuration as it exists or to describe alternatives - for instance,
> a clock with 10 parents may be described as having 10 parents, and the
> binding (in complicated cases) or driver (if it is simple as a value
> from 0 to 7 and the field width is 3 bits for example) will determine
> how that alternative can be selected (and by consequence, what the
> current setting is).
>
> The device tree concept is NOT a place to dump configuration items for
> your board as hardcoded values to try and force something you could
> have done elsewhere. On i.MX53 you cannot boot a kernel verbatim - you
> at least have to run through the Boot ROM and supply a DCD table or
> plugins first. This is where you figure things like this out.
>
> In a case where you have, say, an audio codec and it has a dynamic
> input clock that you want to change on the fly, first of all you would
> not hardcode a frequency into a device tree, second of all there is
> nothing stopping you from doing that right now anyway. If the clock is
> static and fixed frequency and can only be turned on and off, there
> are clock bindings for this already. If it is static and can never be
> turned off, then there are clock bindings for this too, and in this
> case the proviso is that the clock is ALREADY turned on and ALREADY
> stable before you enter the kernel otherwise the description is by
> it's very definition invalid.
>
> If the clock frequency in hardware is not what you wanted when the
> driver comes up, and you only have an on/off switch, it is not up to
> the device tree binding to describe how to go about configuring the
> system to make sure. You cannot in good conscience put a clock
> frequency "to be set later" in the device tree along with a clock
> phandle, simply because that means the device tree no longer describes
> the hardware accurately - the clock phandle is a valid clock, the
> hardware will tell you a frequency from registers in the clock driver,
> the device tree will disagree. How do you know which one is the
> correct value, or if the frequency in the tree is a suggestion rather
> than a description?
>
> I am totally against this (sorry Sascha..). Let's put some effort into
> fixing the bootloaders rather than trying to use the device tree to
> enforce the ridiculous assumption that "Linux kernel does not trust
> the bootloader". Once the bindings and trees are out of the kernel
> source, you're going to HAVE to trust what the bootloader passes, be
> it pregenerated compiled blob (which needs to be written to match the
> hardware state the bootloader finishes up at) or dynamically generated
> at runtime during the boot process (which can describe to the bit what
> the hardware is doing). If you are testing this on Beagle XM you can
> fix your U-Boot easily. New boards will have to be designed *with the
> idea of device trees and working configurations in mind* - that is a
> fact of life, in fact this is how it should be in reality these days
> anyway (most HW designers will do initial bringup of the board - at
> least a minimal working configuration, in a restricted environment
> where they can use test pads to measure clocks, voltage outputs and
> levels of devices to ensure both production was successful and that
> the design is in itself electrically sound. This code 99% of the time
> ends up in the bootloader... sometimes not, when the board was
> designed by one group and the software written by the community, but
> in this case the community needs to learn board bringup and proper
> behavior...)
>
> --
> Matt Sealey <matt@genesi-usa.com>
> Product Development Analyst, Genesi USA, Inc.
Tomasz Figa April 6, 2013, 1:21 p.m. UTC | #9
Hi,

[RANT]

I tend to disagree about this whole hype about device tree usage for other 
things than pure hardware description. I don't think device tree should be  
a way to force some kind of new world order, but rather a more convenient 
and more maintainable (than board files) way of support hardware platforms 
in Linux kernel.

On Friday 05 of April 2013 20:07:09 Matt Sealey wrote:
> On Thu, Apr 4, 2013 at 6:08 PM, Fabio Estevam <festevam@gmail.com> 
wrote:
> > This could be useful for removing the imx6q_sabrelite_cko1_setup()
> > function from arch/arm/mach-imx/mach-imx6q.c.
> > 
> > I would like to add audio support for another board and would like to
> > avoid to do the same as imx6q_sabrelite_cko1_setup()  for setting up
> > the CLKO, if possible.
> 
> You know, we have the same problem on one of our designs here (CLKO is
> used on MX53 for audio codec and camera device, shared) - the current
> solution is hack it into platform support in a BSP kernel.
> 
> If we move to device tree, we know and you know the solution already;
> all this clock setup HAS to be done in the bootloader.

Assuming that you can change the bootloader... Isn't bootloader this piece 
of code that you shouldn't touch if it's working (especially in production 
environments)?

> The device tree really, really is only a way to describe the
> configuration as it exists or to describe alternatives - for instance,
> a clock with 10 parents may be described as having 10 parents, and the
> binding (in complicated cases) or driver (if it is simple as a value
> from 0 to 7 and the field width is 3 bits for example) will determine
> how that alternative can be selected (and by consequence, what the
> current setting is).
> 
> The device tree concept is NOT a place to dump configuration items for
> your board as hardcoded values to try and force something you could
> have done elsewhere. On i.MX53 you cannot boot a kernel verbatim - you
> at least have to run through the Boot ROM and supply a DCD table or
> plugins first. This is where you figure things like this out.

Why not? For Linux and most of ARM-based platforms device tree is just a 
replacement for board files. They are not going to be used with any other 
imaginary kernel supporting device tree. On many of them even the device 
tree blob will be distributed along with kernel image, if not simply 
appended to the zImage.

I think it should be up to the board/platform maintainer whether they want 
to limit device tree on particular boards/platforms just to hardware 
description or extend it to handle everything that was originally handled 
by board files.

> In a case where you have, say, an audio codec and it has a dynamic
> input clock that you want to change on the fly, first of all you would
> not hardcode a frequency into a device tree, second of all there is
> nothing stopping you from doing that right now anyway. If the clock is
> static and fixed frequency and can only be turned on and off, there
> are clock bindings for this already. If it is static and can never be
> turned off, then there are clock bindings for this too, and in this
> case the proviso is that the clock is ALREADY turned on and ALREADY
> stable before you enter the kernel otherwise the description is by
> it's very definition invalid.
> 
> If the clock frequency in hardware is not what you wanted when the
> driver comes up, and you only have an on/off switch, it is not up to
> the device tree binding to describe how to go about configuring the
> system to make sure. You cannot in good conscience put a clock
> frequency "to be set later" in the device tree along with a clock
> phandle, simply because that means the device tree no longer describes
> the hardware accurately - the clock phandle is a valid clock, the
> hardware will tell you a frequency from registers in the clock driver,
> the device tree will disagree. How do you know which one is the
> correct value, or if the frequency in the tree is a suggestion rather
> than a description?

Sure. This is (and has always been) something to account for when bringing 
up new platforms from the ground up.

But you can't always have control over the bootloader. What's wrong in 
letting the kernel configure the board itself? It must configure most of 
the hardware anyway, based on platform data (either located in board files 
or parsed from device tree).

Why not to make the kernel independent from the bootloader at all? Then 
the bootloader could just do some minimal initialization needed to load a 
kernel image from flash memory and launch it (+ some code to allow 
flashing of new images).

> I am totally against this (sorry Sascha..). Let's put some effort into
> fixing the bootloaders rather than trying to use the device tree to
> enforce the ridiculous assumption that "Linux kernel does not trust
> the bootloader". Once the bindings and trees are out of the kernel
> source, you're going to HAVE to trust what the bootloader passes, be
> it pregenerated compiled blob (which needs to be written to match the
> hardware state the bootloader finishes up at) or dynamically generated
> at runtime during the boot process (which can describe to the bit what
> the hardware is doing). If you are testing this on Beagle XM you can
> fix your U-Boot easily. New boards will have to be designed *with the
> idea of device trees and working configurations in mind* - that is a
> fact of life, in fact this is how it should be in reality these days
> anyway (most HW designers will do initial bringup of the board - at
> least a minimal working configuration, in a restricted environment
> where they can use test pads to measure clocks, voltage outputs and
> levels of devices to ensure both production was successful and that
> the design is in itself electrically sound. This code 99% of the time
> ends up in the bootloader... sometimes not, when the board was
> designed by one group and the software written by the community, but
> in this case the community needs to learn board bringup and proper
> behavior...)

Well, so you are basically suggesting to throw away Linux support for 
boards/platforms designed without device tree in mind and on which 
replacing the bootloader is simply infeasible or sometimes even 
impossible.

I don't think that ARM Linux is adopting device tree just to have device 
tree. There are many problems that can be solved using device tree, like 
multiplatform support, separation of board support from kernel code, 
binding consumers with providers (PHYs, GPIOs, clocks, etc.). Why do we 
have to cover all these advantages behind a curtain of new problems?

[/RANT]

Best regards,
Tomasz
Tomasz Figa April 6, 2013, 1:31 p.m. UTC | #10
CCing Sylwester and Mike (correctly)

On Saturday 06 of April 2013 15:21:19 Tomasz Figa wrote:
> Hi,
> 
> [RANT]
> 
> I tend to disagree about this whole hype about device tree usage for
> other things than pure hardware description. I don't think device tree
> should be a way to force some kind of new world order, but rather a
> more convenient and more maintainable (than board files) way of support
> hardware platforms in Linux kernel.
> 
> On Friday 05 of April 2013 20:07:09 Matt Sealey wrote:
> > On Thu, Apr 4, 2013 at 6:08 PM, Fabio Estevam <festevam@gmail.com>
> 
> wrote:
> > > This could be useful for removing the imx6q_sabrelite_cko1_setup()
> > > function from arch/arm/mach-imx/mach-imx6q.c.
> > > 
> > > I would like to add audio support for another board and would like
> > > to
> > > avoid to do the same as imx6q_sabrelite_cko1_setup()  for setting up
> > > the CLKO, if possible.
> > 
> > You know, we have the same problem on one of our designs here (CLKO is
> > used on MX53 for audio codec and camera device, shared) - the current
> > solution is hack it into platform support in a BSP kernel.
> > 
> > If we move to device tree, we know and you know the solution already;
> > all this clock setup HAS to be done in the bootloader.
> 
> Assuming that you can change the bootloader... Isn't bootloader this
> piece of code that you shouldn't touch if it's working (especially in
> production environments)?
> 
> > The device tree really, really is only a way to describe the
> > configuration as it exists or to describe alternatives - for instance,
> > a clock with 10 parents may be described as having 10 parents, and the
> > binding (in complicated cases) or driver (if it is simple as a value
> > from 0 to 7 and the field width is 3 bits for example) will determine
> > how that alternative can be selected (and by consequence, what the
> > current setting is).
> > 
> > The device tree concept is NOT a place to dump configuration items for
> > your board as hardcoded values to try and force something you could
> > have done elsewhere. On i.MX53 you cannot boot a kernel verbatim - you
> > at least have to run through the Boot ROM and supply a DCD table or
> > plugins first. This is where you figure things like this out.
> 
> Why not? For Linux and most of ARM-based platforms device tree is just a
> replacement for board files. They are not going to be used with any
> other imaginary kernel supporting device tree. On many of them even the
> device tree blob will be distributed along with kernel image, if not
> simply appended to the zImage.
> 
> I think it should be up to the board/platform maintainer whether they
> want to limit device tree on particular boards/platforms just to
> hardware description or extend it to handle everything that was
> originally handled by board files.
> 
> > In a case where you have, say, an audio codec and it has a dynamic
> > input clock that you want to change on the fly, first of all you would
> > not hardcode a frequency into a device tree, second of all there is
> > nothing stopping you from doing that right now anyway. If the clock is
> > static and fixed frequency and can only be turned on and off, there
> > are clock bindings for this already. If it is static and can never be
> > turned off, then there are clock bindings for this too, and in this
> > case the proviso is that the clock is ALREADY turned on and ALREADY
> > stable before you enter the kernel otherwise the description is by
> > it's very definition invalid.
> > 
> > If the clock frequency in hardware is not what you wanted when the
> > driver comes up, and you only have an on/off switch, it is not up to
> > the device tree binding to describe how to go about configuring the
> > system to make sure. You cannot in good conscience put a clock
> > frequency "to be set later" in the device tree along with a clock
> > phandle, simply because that means the device tree no longer describes
> > the hardware accurately - the clock phandle is a valid clock, the
> > hardware will tell you a frequency from registers in the clock driver,
> > the device tree will disagree. How do you know which one is the
> > correct value, or if the frequency in the tree is a suggestion rather
> > than a description?
> 
> Sure. This is (and has always been) something to account for when
> bringing up new platforms from the ground up.
> 
> But you can't always have control over the bootloader. What's wrong in
> letting the kernel configure the board itself? It must configure most of
> the hardware anyway, based on platform data (either located in board
> files or parsed from device tree).
> 
> Why not to make the kernel independent from the bootloader at all? Then
> the bootloader could just do some minimal initialization needed to load
> a kernel image from flash memory and launch it (+ some code to allow
> flashing of new images).
> 
> > I am totally against this (sorry Sascha..). Let's put some effort into
> > fixing the bootloaders rather than trying to use the device tree to
> > enforce the ridiculous assumption that "Linux kernel does not trust
> > the bootloader". Once the bindings and trees are out of the kernel
> > source, you're going to HAVE to trust what the bootloader passes, be
> > it pregenerated compiled blob (which needs to be written to match the
> > hardware state the bootloader finishes up at) or dynamically generated
> > at runtime during the boot process (which can describe to the bit what
> > the hardware is doing). If you are testing this on Beagle XM you can
> > fix your U-Boot easily. New boards will have to be designed *with the
> > idea of device trees and working configurations in mind* - that is a
> > fact of life, in fact this is how it should be in reality these days
> > anyway (most HW designers will do initial bringup of the board - at
> > least a minimal working configuration, in a restricted environment
> > where they can use test pads to measure clocks, voltage outputs and
> > levels of devices to ensure both production was successful and that
> > the design is in itself electrically sound. This code 99% of the time
> > ends up in the bootloader... sometimes not, when the board was
> > designed by one group and the software written by the community, but
> > in this case the community needs to learn board bringup and proper
> > behavior...)
> 
> Well, so you are basically suggesting to throw away Linux support for
> boards/platforms designed without device tree in mind and on which
> replacing the bootloader is simply infeasible or sometimes even
> impossible.
> 
> I don't think that ARM Linux is adopting device tree just to have device
> tree. There are many problems that can be solved using device tree,
> like multiplatform support, separation of board support from kernel
> code, binding consumers with providers (PHYs, GPIOs, clocks, etc.). Why
> do we have to cover all these advantages behind a curtain of new
> problems?
> 
> [/RANT]
> 
> Best regards,
> Tomasz
Martin Fuzzey April 6, 2013, 5:51 p.m. UTC | #11
On Sat, Apr 6, 2013 at 3:21 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi,
>
> [RANT]
>
> I tend to disagree about this whole hype about device tree usage for other
> things than pure hardware description. I don't think device tree should be
> a way to force some kind of new world order, but rather a more convenient
> and more maintainable (than board files) way of support hardware platforms
> in Linux kernel.
>
Totally agree

> On Friday 05 of April 2013 20:07:09 Matt Sealey wrote:
>>
>> The device tree concept is NOT a place to dump configuration items for
>> your board as hardcoded values to try and force something you could
>> have done elsewhere. On i.MX53 you cannot boot a kernel verbatim - you
>> at least have to run through the Boot ROM and supply a DCD table or
>> plugins first. This is where you figure things like this out.
>

You're suggesting setting the frequencies, parents etc in the DCD table??
Why?
IMHO that's horribly unreadable.

> But you can't always have control over the bootloader. What's wrong in
> letting the kernel configure the board itself? It must configure most of
> the hardware anyway, based on platform data (either located in board files
> or parsed from device tree).
>
> Why not to make the kernel independent from the bootloader at all? Then
> the bootloader could just do some minimal initialization needed to load a
> kernel image from flash memory and launch it (+ some code to allow
> flashing of new images).
>

Yes that's my opinion too.

I think the bootloader should really do as little as possible since:
* It's generally harder and/or riskier to update the bootloader than
the kernel (althugh somewhat less true these days with boot from SD)
* There are multiple bootloaders (u-boot, barebox, ...) but only one kernel
* It makes it easier to do product families where you have a common
bootloader and kernel image with different DTBs per variant. You can
then put all the DTBs in the boot partition and use a bootloader
variable to chose the right one.

>> I am totally against this (sorry Sascha..). Let's put some effort into
>> fixing the bootloaders rather than trying to use the device tree to
>> enforce the ridiculous assumption that "Linux kernel does not trust
>> the bootloader".
Why is this "rediculous" IMHO the kernel shouldn't trust the
bootloader, beyond having setting up the hardware sufficiently to
reliably and safely execute code.

>> Once the bindings and trees are out of the kernel
>> source, you're going to HAVE to trust what the bootloader passes, be
>> it pregenerated compiled blob (which needs to be written to match the
>> hardware state the bootloader finishes up at) or dynamically generated
>> at runtime during the boot process (which can describe to the bit what
>> the hardware is doing).

You have to trust the DTB yes but that is not the same as trusting the
bootloader to have setup the hardware.
I fail to see what difference it makes if the DTS is inside or outside
the kernel source. Even today you can compile a DTB from out of tree
DTS and pass it to a mainline kernel.


Regards,

Martin
Matt Sealey April 6, 2013, 7:24 p.m. UTC | #12
On Sat, Apr 6, 2013 at 12:51 PM, Martin Fuzzey <mfuzzey@gmail.com> wrote:
> On Sat, Apr 6, 2013 at 3:21 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> Hi,
>>
>> [RANT]
>>
>> I tend to disagree about this whole hype about device tree usage for other
>> things than pure hardware description. I don't think device tree should be
>> a way to force some kind of new world order, but rather a more convenient
>> and more maintainable (than board files) way of support hardware platforms
>> in Linux kernel.
>>
> Totally agree
>
>> On Friday 05 of April 2013 20:07:09 Matt Sealey wrote:
>>>
>>> The device tree concept is NOT a place to dump configuration items for
>>> your board as hardcoded values to try and force something you could
>>> have done elsewhere. On i.MX53 you cannot boot a kernel verbatim - you
>>> at least have to run through the Boot ROM and supply a DCD table or
>>> plugins first. This is where you figure things like this out.
>>
>
> You're suggesting setting the frequencies, parents etc in the DCD table??

It's one option.. you should also be doing IOMUX somewhere very early
too for *everything* you can, as changing pad settings on the fly can
be electrically problematic. You wouldn't change an input to an output
8 seconds into the board's boot process if you could do it 8ns into
it. Our HW designers constantly freak out over doing pinctrl stuff so
late in the process..

> Why?

Because the earlier you do it, the better.

> IMHO that's horribly unreadable.

So is most of the low level init code for anything; fact of life. Have
you ever looked at the lib/ and kernel/ directories in the kernel
tree? The code that deals with low-level debug and earlyprintk?

The device tree isn't meant to make that part easier it's to describe
what you did there (and potentially verify it ;)

>> Why not to make the kernel independent from the bootloader at all? Then
>> the bootloader could just do some minimal initialization needed to load a
>> kernel image from flash memory and launch it (+ some code to allow
>> flashing of new images).
>
> Yes that's my opinion too.
>
> I think the bootloader should really do as little as possible since:
> * It's generally harder and/or riskier to update the bootloader than
> the kernel (althugh somewhat less true these days with boot from SD)

Although you cannot guarantee your system can even boot from SD (or
even has an SD slot to do so). It could be SPI NOR or MLC NAND on some
bus, or something more esoteric. Updating a bootloader is not really
that risky anymore - I would agree outright that it is undesirable to
do too often, but there are ways around this that don't involve adding
to the device tree in such a way that you are basically explaining all
the items the bootloader forgot (or was so badly written, it did not
do). If you want USB networking, you need to bring up USB. If you want
to store the environment, you need to set up some environment storage.
You're configuring the memory controller, memory clock, regulators and
getting to the point you have a serial console that will effectively
load a kernel from your chosen place to memory and execute it - at
this point I don't understand why setting CLKO's parent and it's
divider (two.. register.. writes...) is somehow so difficult and
requires a device tree binding.

Once it's set up, as I said there are bindings for fixed
always-running clocks, fixed toggled clocks, and dynamic clocks, which
covers all cases.

> * There are multiple bootloaders (u-boot, barebox, ...) but only one kernel

No, I can think of multiple operating systems that could possibly run
here.. VxWorks, Integrity, QNX, L4 (Fiasco, Pistachio), NetBSD,
FreeBSD,  and in Sascha's example, OpenBSD.. Darwin/XNU? We're walkng
further into territory of "it will never happen anyway" but I know the
first 7 boot on a ton of i.MX processors and they could all use device
tree at some point.

All of which need to do the same setup anyway - Linux requires one
hell of a lot of low level initialization of the CPU on ARM and x86
platforms; if we're moving errata fixes out to the bootloader to deal
with TrustZone on ARM, then there is already a lot of "unreadable"
assembly code in there. To boot Linux from SD card on i.MX you need to
set up the SD card IOMUX (Boot ROM does this for the one you for the
one you chose, but if you did not choose it, it isn't set up).

The fragmentation of bootloaders (U-Boot, barebox, moboot..) is not a
technology issue but a political one. The stuff we're talking about
here is usually cut & pastable and if the vendor of the hardware
releases an opensource bootloader - eminently so. If it's a closed
bootloader, well... you're SOL. That is a problem, but that is not a
problem the device tree is meant to solve. The correct way to deal
with that is refuse Linux support until they do things right, or have
things broken until they do things right. Eventually they will figure
it out.. but the idea would be to not have a lot of platform setup
code living in the kernel and moving around between DIFFERENT kernels.
The device tree is one solution - describe an interface between
unknown bootloader "foo" and operating system kernel "bar" - but that
doesn't mean it replaces the concept that it needs

> * It makes it easier to do product families where you have a common
> bootloader and kernel image with different DTBs per variant. You can
> then put all the DTBs in the boot partition and use a bootloader
> variable to chose the right one.

That doesn't mean you can't have the bootloader know which
device/board it is running on. How does that variable get set? If that
variable is set at runtime, surely then it can perform all this setup
before it sets the variable?

>>> I am totally against this (sorry Sascha..). Let's put some effort into
>>> fixing the bootloaders rather than trying to use the device tree to
>>> enforce the ridiculous assumption that "Linux kernel does not trust
>>> the bootloader".
>
> Why is this "rediculous" IMHO the kernel shouldn't trust the
> bootloader, beyond having setting up the hardware sufficiently to
> reliably and safely execute code.

Here we are merely differing on the definition of "sufficiently to
reliably and safely execute code".

We're in an age of SoCs, I remember in the desktop PowerPC days and
older x86 days of northbridges, southbridges and peripherals you only
had responsibility to bring up the CPU, but again you had to configure
a PCI host controller, USB, IDE drives, display, mouse and keyboard of
some kind... and setting up one clock that's not used right now but
will be by the OS is NOT a lot of code on top of all that. Now most of
that is basically, practically required.. on most of the boards we're
looking at supporting in the Linux ARM DT kernel support right now,
it's pretty much all esoteric development boards with more pin headers
than actual features, and Android development kits that are just
tablets without cases - they are designed to have you mess with the
bootloader.

If we don't support those boards properly by properly implementing
bootloader support and setup, it means consumer devices based on them
do not follow the best practises of doing this stuff in the bootloader
before the OS - which is a more lean, easier to test environment
anyway. We're talking here about one device where we want a clock
output for a codec from the SoC, did anyone even sit down, put a scope
on a test pad and be sure that the clock is stable, not full of
jitter, and has the right logic levels? The best place to do that is
after you set it up, no-autoboot U-Boot in most of these cases and for
more proprietary systems, since most hardware designers end up doing
this anyway, usually the code is around somewhere.

>>> Once the bindings and trees are out of the kernel
>>> source, you're going to HAVE to trust what the bootloader passes, be
>>> it pregenerated compiled blob (which needs to be written to match the
>>> hardware state the bootloader finishes up at) or dynamically generated
>>> at runtime during the boot process (which can describe to the bit what
>>> the hardware is doing).
>
> You have to trust the DTB yes but that is not the same as trusting the
> bootloader to have setup the hardware.
>
> I fail to see what difference it makes if the DTS is inside or outside
> the kernel source. Even today you can compile a DTB from out of tree
> DTS and pass it to a mainline kernel.

Indeed, but you can guarantee in the time it took you to do that
someone changed a binding because the source code for the DTS, and the
documentation for the binding, is still part of Linux and it means
Linux "hack it in and we'll do it properly later" design methodology
comes into play.

If you have a fixed clock which cannot change, and the bare minimum
functionality implemented is gating, you need to set this up in the
bootloader and - at option, depending on if it's needed to boot - gate
it. Then tell Linux there's a fixed rate clock and it can turn it on
and off via the device tree.. that's the solution here, it's already
written and has bindings, we don't need new ones to cover the case
whereby someone forgot to do so.

I am going to stand by my little proposal; we modify the clock
initialization so effectively check whether the parent in the DTB is
the same as the one in hardware, and if not then report this fact (and
optionally set that clock's parent to the one in the DTB). This is a
sly back-door approach in one sense, but in the future when people
build a board, put initial revision of bootloader code on the board,
boot a kernel with a device tree that is not describing the hardware,
they will see warnings; "Your clock "fooclk" isn't the same as your DT
says it is. Fix the bootloader, please."

And they will. This means the bootloader gets easier to trust for
every new board.

As for giving devices some configuration so they can "force" a clock
frequency, I don't like that idea. In the event it is derived from a
clock somewhere, in the audio codec case and in many others it is
better to do this on the basis of power management or configuration
"selection" - you describe all the possible ways it can be configured
for certain features (for instance, 27MHz clock frequency allows audio
rates of 8000, 16000, 32000, 44100, 48000Hz and 24MHz allows 8000,
16000, 32000, 44100, 48000, 88200, 96000Hz) and the driver can make an
assessment of what clock it has - clk_get_rate tells it which clock it
has (lets assume the board booted with a 27MHz clock). If it can set
the clock to one that supports more rates, it can do so - in this
case, it would decide it wants 96KHz support, so it clk_set_rate it to
24MHz. The frequency support (and bit format support too) should be in
the device tree so that given a supplied clock phandle it can find out
the current rate, derive supported modes, and then set a better rate
if that's functional. "Setting the clock rate based on configuration
items in some clock-configuration tree" is too much of a specific fix,
for in this case one particular device. But most devices can support a
list of potential configurations FOR THAT DEVICE (the same way a clock
can have many parents already, but only one can be set) and the DT is
exactly there to describe these, and the best practise code in the
driver would basically - by the back door - fix the clock for us.

I still say the desired clock, implemented by the HW designer, should
be done in the bootloader though (that way no clock changes happen
inside Linux) and we can figure a much, MUCH more consistent way of
allowing drivers to actually "set the clock it wants" without it being
a clock-subsystem thing. It is the device that has the requirement for
the clock frequency after all, not the clock subsystem.
Fabio Estevam April 6, 2013, 7:40 p.m. UTC | #13
Matt,

On Sat, Apr 6, 2013 at 4:24 PM, Matt Sealey <matt@genesi-usa.com> wrote:

> Indeed, but you can guarantee in the time it took you to do that
> someone changed a binding because the source code for the DTS, and the
> documentation for the binding, is still part of Linux and it means
> Linux "hack it in and we'll do it properly later" design methodology
> comes into play.
>
> If you have a fixed clock which cannot change, and the bare minimum
> functionality implemented is gating, you need to set this up in the
> bootloader and - at option, depending on if it's needed to boot - gate
> it. Then tell Linux there's a fixed rate clock and it can turn it on
> and off via the device tree.. that's the solution here, it's already
> written and has bindings, we don't need new ones to cover the case
> whereby someone forgot to do so.
>
> I am going to stand by my little proposal; we modify the clock
> initialization so effectively check whether the parent in the DTB is
> the same as the one in hardware, and if not then report this fact (and
> optionally set that clock's parent to the one in the DTB). This is a
> sly back-door approach in one sense, but in the future when people
> build a board, put initial revision of bootloader code on the board,
> boot a kernel with a device tree that is not describing the hardware,
> they will see warnings; "Your clock "fooclk" isn't the same as your DT
> says it is. Fix the bootloader, please."

Care to submit a patch about your proposal for the codec clock example, please?

It makes much easier to get comments/testing, etc in a real patch format.

I will be glad to test your proposal if you submit a patch for it.

Thanks,

Fabio Estevam
Sascha Hauer April 7, 2013, 1:26 p.m. UTC | #14
On Sat, Apr 06, 2013 at 03:21:19PM +0200, Tomasz Figa wrote:
> Hi,
> 
> [RANT]
> 
> I tend to disagree about this whole hype about device tree usage for other 
> things than pure hardware description. I don't think device tree should be  
> a way to force some kind of new world order, but rather a more convenient 
> and more maintainable (than board files) way of support hardware platforms 
> in Linux kernel.

Honestly I'm annoyed by this aswell. The devicetree contains a nice and
complete hardware description and it seems convenient to put hardware
related configuration data there aswell.

The problem is that hardware description and configuration data are two
completely different sets of data. The hardware description is static
for a given board and should (ideally) never change. The configuration
data instead is often usecase specific and changes over the lifetime of
a board. The configuration data can only handle a single (or maybe a
table of) static setup(s). It's a good way to specify a sane default or
a very special setup, but doesn't handle the case when some OS (or
version thereof) wants to have a static setup and another wants to
figure out the same data dynamically.

For these reasons I am against throwing the two data sets into a single
pot. Still I also want to have the devicetree way to configure some
static setup items.

People are already working on devicetree overlays.  Maybe it would be
possible to have some kind of configuration overlay for the devicetree.
This would make it possible to store the data in different places and to
exchange the configuration data while keeping the hardware description.
Also board designers could describe the hardware and give one or more
usage hints without forcing anybody to actually use them.

Sascha
Matt Sealey April 7, 2013, 3:50 p.m. UTC | #15
On Sun, Apr 7, 2013 at 8:26 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Sat, Apr 06, 2013 at 03:21:19PM +0200, Tomasz Figa wrote:
>> Hi,
>>
>> [RANT]
>>
>> I tend to disagree about this whole hype about device tree usage for other
>> things than pure hardware description. I don't think device tree should be
>> a way to force some kind of new world order, but rather a more convenient
>> and more maintainable (than board files) way of support hardware platforms
>> in Linux kernel.
>
> Honestly I'm annoyed by this aswell. The devicetree contains a nice and
> complete hardware description and it seems convenient to put hardware
> related configuration data there aswell.

It's convenient because it means less work for one person; but I posit
that the work has already been done or the board would have never
passed post-production testing i.e. does this clock output correctly?
If so, then the code was written to make it do so. if it was not
integrated into the bootloader, this is an engineering problem easier
tackled by doing that than creating a whole new device tree binding.

The reason device tree, in my opinion, is not for "telling the OS how
to force reconfiguration of the hardware so it works," is simply
because engineering problems as above do not get tackled at all. If
you can write a completely awful bootloader that mis-configures the
board and causes myriad problems, but then get trapped in a cycle of
"Linux works fine, and we have a deadline, so.. fuck it" then what
you're doing is forcing the kind of awful code that we ended up with
in BSPs, the kind of awful code that existed BEFORE the move to device
tree.

In the case where we could "move the clko setup for SabreLite into the
device tree," this is an impetus we all agree on that we should not be
doing board-specific hacks if we can help it. But you don't move one
hack to another place and suddenly it quits being a hack. It will just
be someone else's responsibility.

> The problem is that hardware description and configuration data are two
> completely different sets of data. The hardware description is static
> for a given board and should (ideally) never change. The configuration
> data instead is often usecase specific and changes over the lifetime of
> a board. The configuration data can only handle a single (or maybe a
> table of) static setup(s). It's a good way to specify a sane default or
> a very special setup, but doesn't handle the case when some OS (or
> version thereof) wants to have a static setup and another wants to
> figure out the same data dynamically.



> For these reasons I am against throwing the two data sets into a single
> pot. Still I also want to have the devicetree way to configure some
> static setup items.

Well as I proposed, instead of having a special magic configuration
data and a backdoor to re-doing work that should be done elsewhere, we
should basically turn one of them into a kind of sanity check - it is
much, much more desirable for Linux to fudge around weirdly described
hardware and warn about it and there are many examples of this in the
Linux kernel. Embarrassingly, two of the examples are for Genesi
platforms on PowerPC, and one that immediately springs to mind is the
one about piix_smbus not finding a configured smbus address. This is a
problem in most VM BIOS implementations, since they do not have fans
to control, they don't bother implementing or virtualizing it or
telling the OS something is there (and it would require supporting a
ton of different fan controllers to figure different operating
systems), but Linux will always, always pop this warning up for me.
And it is correct to do so.. there are real life BIOS implementations
that do not contain any data about where the smbus controller is, and
most "A*** Super Overclock Fan Utility" on Windows are hardcoded for a
very specific PC model to work around this.

The sanity check is, in a properly described clock, you would list
it's suitable parents and the parent you THINK your hardware has set
right now - or, the only one your hardware CAN support. You can hijack
this a little such that if the parent is not the one in the DTB then
it gets set, with a warning (and for nitpickers like me, never turn on
this since we would prefer to fix our bootloader until it doesn't spit
a warning AND everything works).

For magic clock-frequency definition at the device level, this is best
done with power management in mind, the method will work everywhere
and have the same basic concept and binding, but it HAS to be
device-specific - not clock-specific or clock-subsystem specific. I do
not think it is the DTB's place to "tell Linux how to configure the
clock subsystem", just tell it where those clocks are and what the
current hardware settings are if they are not derivable from the
hardware itself. In the case we cannot tell by reading a register what
the current configuration is, or it is fundamentally more complicated
at the time of single clock initialization, then we need to provide
this data in the DTB anyway, and this binding would suit the
complicated version AND all the dirt simple implementations
(complicated version = has distinct benefit by abstracting the
hardware, simple version = effects a sanity check).

If we're dealing with audio codecs, fix the audio codec to accept a
*table* of possible clock values and if that hardware operates
differently per clock value, then add that data in there too (like the
cs42l52 audio codec). Where the software and hardware is a little more
dynamic, the same binding works here - and the usual trick is just
force one configuration possible out of many. The board designer can
look at all the possibilities and choose the one or two or all that
will work with the design, and it can go into the device tree.

In the case where we have a board design here where the clock
frequency MUST be set and MUST be fixed and MUST NEVER be changed
while it is running (i.e. audio codec and camera sharing a master
clock, and glitching the audio clock just to set it to the "right"
frequency would make the camera stop working), I can't approve of
anything proposed so far as it does not take this into account.
Remember, your device may not own the clock it is trying to use, and
you make it more difficult to coordinate between subsystems when
you're basically blanket hardcoding values in places. And I think the
"little overlay with configuration data" makes no sense when this is
entirely a bootloader vs. driver-that-doesn't-do-enough problem.

There are ways around every specific problem we're discussing here
without having magic, forced configuration tables. There will be ways
around having magic, forced configuration tables for everything else,
I guarantee it..

> Also board designers could describe the hardware and give one or more
> usage hints without forcing anybody to actually use them.

Usually the "hint" from a board designer is not really a hint, but the
only way the board was designed to work. You can't just "modify" their
suggestion on what clock to use, or the restrictions in place on
routing, signalling etc. at a whim just because it is configurable at
the SoC level. The device tree serves to tell Linux how the board was
designed, where things have been placed so it can abstract the
thousands upon thousands of lines of duplicated
platform_device_register lines and static data floating around. But
the original concept was that as the firmware booted, and initialized
all the devices capable of being used in the system, it would add each
one with it's post-initialization data to the device tree and pass
this to the OS so that it would not need to guess, or re-perform
initialization. And this is the point I would like to see stick -
Linux should not be manually re-initializing everything, reconfiguring
every little thing when the bootloader so carefully did it already.
There is a use case for catching fundamental flaws in the bootloader
setup and fixing them, but then you have a heady mix of boards which
specify EVERY configuration item possible and some which are very
simply descriptions of valid, usable state at bootloader->kernel
transition time.

That is a weird dichotomy to have and a decision to be made; do we put
all our effort into making the data fit into device tree or do we just
do it in the bootloader (which we would have had to have coded as real
platform test code before the boards passed production testing
anyway)? It is not a world I want to live in where the answer is NOT
to use the code written to pass production test in the bootloader.
Doing it in the device tree just means people who never learned about
board production, low level testing never, ever learn about that, and
those people TEND to make very strange decisions about what data needs
to be in the device tree. We already have with pinctrl - if the
bootloader set up pins already to boot from SD card, why do we specify
those pins again in Linux for it to configure them exactly the same
way, again? I know there are only a couple of blobs in the Babbage DT
that really need setting because U-Boot has already configured them
but they're in there - and Linux sets all those values again. Some of
the things it is setting up again, *IF* (and it is possible) it causes
a glitch in logic or an I/O direction change at any point can put the
board electrically in a completely undesirable state. 50,000 times
later your board is burnt, but how would you ever know this caused it?

I should also note, it is NOT simpler to add an entry to device tree
for configuration, since you need to write a binding that describes it
and a driver on the other side to parse the data the way the binding
describes to implement that configuration. Then you need to add the
entry to the device tree to test it. Why not use the minimal code
required to just set that hardware at a low level to the configuration
you wanted, at Boot ROM or Bootloader time, and describe that with the
bindings that already exist?

--
Matt Sealey <matt@genesi-usa.com>
Product Development Analyst, Genesi USA, Inc.
Fabio Estevam April 7, 2013, 4 p.m. UTC | #16
On Sun, Apr 7, 2013 at 12:50 PM, Matt Sealey <matt@genesi-usa.com> wrote:

> If we're dealing with audio codecs, fix the audio codec to accept a
> *table* of possible clock values and if that hardware operates
> differently per clock value, then add that data in there too (like the
> cs42l52 audio codec). Where the software and hardware is a little more
> dynamic, the same binding works here - and the usual trick is just
> force one configuration possible out of many. The board designer can
> look at all the possibilities and choose the one or two or all that
> will work with the design, and it can go into the device tree.

Sorry, but I have a hard time following a so long response.

Can't you just send patches to the list with your proposal so that we
can move forward with the idea?
Matt Sealey April 7, 2013, 4:23 p.m. UTC | #17
On Sun, Apr 7, 2013 at 11:00 AM, Fabio Estevam <festevam@gmail.com> wrote:
> On Sun, Apr 7, 2013 at 12:50 PM, Matt Sealey <matt@genesi-usa.com> wrote:
>
>> If we're dealing with audio codecs, fix the audio codec to accept a
>> *table* of possible clock values and if that hardware operates
>> differently per clock value, then add that data in there too (like the
>> cs42l52 audio codec). Where the software and hardware is a little more
>> dynamic, the same binding works here - and the usual trick is just
>> force one configuration possible out of many. The board designer can
>> look at all the possibilities and choose the one or two or all that
>> will work with the design, and it can go into the device tree.
>
> Sorry, but I have a hard time following a so long response.
>
> Can't you just send patches to the list with your proposal so that we
> can move forward with the idea?

Sure.
Matt Sealey April 7, 2013, 4:34 p.m. UTC | #18
Or actually, let me rephrase that; sure, when we decide that the
actual clock tree is described IN the device tree and not in Linux -
the only thing I have to test this on is i.MX and OMAP and it's all a
huge table in the Linux kernel which the DT uses to reference
arbitrary indices into a provider array...

I could add desired parents to the provider array but that wouldn't
really fix the configuration problem. Can we get an agreement that
actually, no matter how "unwieldy" or "unreadable" some people think
it might be, that the entire clock tree for a board should end up in
it's device tree (muxes, selects, gates and all) with suitable
bindings? This is the only way you'll get clock "hacks" out of the
source.

I do have a kernel tree for some 3.8-rc (which you know of already)
which started work on this and I had some success with it. I'll need a
little while to move it forward after Shawn moved some things around,
rebase all the work.. there are more problems to fix with how
everything "works" right now than just a need for configuration data,
which is why I am so opposed to adding that configuration data. It
supposes that we are giving complete descriptions already anyway, and
my proposal relies on having complete descriptions anyway, when we are
not in either case. Give me a couple weeks and I'll probably have it
done, if nothing moves under my feet again..

At the point where we have two proposals here; implement 100 clocks as
device tree nodes, or implement multiple ways of providing clock
reparenting and frequency setting information as a configuration
clock, I would rather implement those 100 clocks as nodes..
Matt Sealey <matt@genesi-usa.com>
Product Development Analyst, Genesi USA, Inc.


On Sun, Apr 7, 2013 at 11:23 AM, Matt Sealey <matt@genesi-usa.com> wrote:
> On Sun, Apr 7, 2013 at 11:00 AM, Fabio Estevam <festevam@gmail.com> wrote:
>> On Sun, Apr 7, 2013 at 12:50 PM, Matt Sealey <matt@genesi-usa.com> wrote:
>>
>>> If we're dealing with audio codecs, fix the audio codec to accept a
>>> *table* of possible clock values and if that hardware operates
>>> differently per clock value, then add that data in there too (like the
>>> cs42l52 audio codec). Where the software and hardware is a little more
>>> dynamic, the same binding works here - and the usual trick is just
>>> force one configuration possible out of many. The board designer can
>>> look at all the possibilities and choose the one or two or all that
>>> will work with the design, and it can go into the device tree.
>>
>> Sorry, but I have a hard time following a so long response.
>>
>> Can't you just send patches to the list with your proposal so that we
>> can move forward with the idea?
>
> Sure.
>
> --
> Matt Sealey <matt@genesi-usa.com>
> Product Development Analyst, Genesi USA, Inc.
Tomasz Figa April 7, 2013, 9:14 p.m. UTC | #19
On Sunday 07 of April 2013 11:34:14 Matt Sealey wrote:
> Or actually, let me rephrase that; sure, when we decide that the
> actual clock tree is described IN the device tree and not in Linux -
> the only thing I have to test this on is i.MX and OMAP and it's all a
> huge table in the Linux kernel which the DT uses to reference
> arbitrary indices into a provider array...

I'm not sure what's wrong with that.

> I could add desired parents to the provider array but that wouldn't
> really fix the configuration problem.

I'm not really seeing any configuration problem here. This is an 
artificial problem created by looking at device tree way too 
conservatively, without any significant reason.

> Can we get an agreement that
> actually, no matter how "unwieldy" or "unreadable" some people think
> it might be, that the entire clock tree for a board should end up in
> it's device tree (muxes, selects, gates and all) with suitable
> bindings? This is the only way you'll get clock "hacks" out of the
> source.

Hardware initialization is not really a hack...

Going this way, why to keep any initialization code inside drivers at all? 
Let's initialize USB, MMC, display controllers in the bootloader and just 
make the driver handle user requests assuming that the hardware is already 
initialized.

> I do have a kernel tree for some 3.8-rc (which you know of already)
> which started work on this and I had some success with it. I'll need a
> little while to move it forward after Shawn moved some things around,
> rebase all the work.. there are more problems to fix with how
> everything "works" right now than just a need for configuration data,
> which is why I am so opposed to adding that configuration data. It
> supposes that we are giving complete descriptions already anyway, and
> my proposal relies on having complete descriptions anyway, when we are
> not in either case. Give me a couple weeks and I'll probably have it
> done, if nothing moves under my feet again..
> 
> At the point where we have two proposals here; implement 100 clocks as
> device tree nodes, or implement multiple ways of providing clock
> reparenting and frequency setting information as a configuration
> clock, I would rather implement those 100 clocks as nodes..

...or 200 or 300 or 400... (yes, there are platforms which have so many 
elements in their clock networks and they are not uncommon), good luck 
with parsing this at runtime (either with CPU or yourself when reading
a dts file).

It would make some sense if all those basic elements (divs, muxes, gates) 
on all platforms would be identical, but each platform will have its own 
specific quirks, for which such uber-generic binding would have to 
account.

Now, why the other option is to have multiple ways? What's holding us from 
defining a single way of doing so?

Best regards,
Tomasz

> Matt Sealey <matt@genesi-usa.com>
> Product Development Analyst, Genesi USA, Inc.
> 
> On Sun, Apr 7, 2013 at 11:23 AM, Matt Sealey <matt@genesi-usa.com> 
wrote:
> > On Sun, Apr 7, 2013 at 11:00 AM, Fabio Estevam <festevam@gmail.com> 
wrote:
> >> On Sun, Apr 7, 2013 at 12:50 PM, Matt Sealey <matt@genesi-usa.com> 
wrote:
> >>> If we're dealing with audio codecs, fix the audio codec to accept a
> >>> *table* of possible clock values and if that hardware operates
> >>> differently per clock value, then add that data in there too (like
> >>> the
> >>> cs42l52 audio codec). Where the software and hardware is a little
> >>> more
> >>> dynamic, the same binding works here - and the usual trick is just
> >>> force one configuration possible out of many. The board designer can
> >>> look at all the possibilities and choose the one or two or all that
> >>> will work with the design, and it can go into the device tree.
> >> 
> >> Sorry, but I have a hard time following a so long response.
> >> 
> >> Can't you just send patches to the list with your proposal so that we
> >> can move forward with the idea?
> > 
> > Sure.
> > 
> > --
> > Matt Sealey <matt@genesi-usa.com>
> > Product Development Analyst, Genesi USA, Inc.
Martin Fuzzey April 8, 2013, 9:35 a.m. UTC | #20
On 07/04/13 15:26, Sascha Hauer wrote:
>
> Honestly I'm annoyed by this aswell. The devicetree contains a nice and
> complete hardware description and it seems convenient to put hardware
> related configuration data there aswell.
Yes
> The problem is that hardware description and configuration data are two
> completely different sets of data. The hardware description is static
> for a given board and should (ideally) never change. The configuration
> data instead is often usecase specific and changes over the lifetime of
> a board. The configuration data can only handle a single (or maybe a
> table of) static setup(s). It's a good way to specify a sane default or
> a very special setup, but doesn't handle the case when some OS (or
> version thereof) wants to have a static setup and another wants to
> figure out the same data dynamically.
Agreed
> For these reasons I am against throwing the two data sets into a single
> pot. Still I also want to have the devicetree way to configure some
> static setup items.
Sure but why does using the DT for both mean "throwing them into a 
single pot?"

I think we need to seperate the ideas of "DT as a container format" and 
"semantics of DT nodes".

The format is the same everywhere but the semantics could change in 
different parts of the tree.

Since the DT is a tree structure surely all we need to do is agree on a 
designated configuration root node
"linux-config" for example under which we put all configuration related 
stuff specific to linux whilst
retaining the "hardware description only" rule for the rest of the DT.

Martin
Sascha Hauer April 8, 2013, 8 p.m. UTC | #21
On Mon, Apr 08, 2013 at 11:35:29AM +0200, Martin Fuzzey wrote:
> On 07/04/13 15:26, Sascha Hauer wrote:
> >
> >Honestly I'm annoyed by this aswell. The devicetree contains a nice and
> >complete hardware description and it seems convenient to put hardware
> >related configuration data there aswell.
> Yes
> >The problem is that hardware description and configuration data are two
> >completely different sets of data. The hardware description is static
> >for a given board and should (ideally) never change. The configuration
> >data instead is often usecase specific and changes over the lifetime of
> >a board. The configuration data can only handle a single (or maybe a
> >table of) static setup(s). It's a good way to specify a sane default or
> >a very special setup, but doesn't handle the case when some OS (or
> >version thereof) wants to have a static setup and another wants to
> >figure out the same data dynamically.
> Agreed
> >For these reasons I am against throwing the two data sets into a single
> >pot. Still I also want to have the devicetree way to configure some
> >static setup items.
> Sure but why does using the DT for both mean "throwing them into a
> single pot?"

Because most likely without further intervention they would end up in
the dts files in the kernel tree. This for example happened to the
mtd partitions for several boards. This means the kernel dts files now
enforce a certain partitioning which is not very nice.

> 
> I think we need to seperate the ideas of "DT as a container format"
> and "semantics of DT nodes".
> 
> The format is the same everywhere but the semantics could change in
> different parts of the tree.
> 
> Since the DT is a tree structure surely all we need to do is agree
> on a designated configuration root node
> "linux-config" for example under which we put all configuration
> related stuff specific to linux whilst
> retaining the "hardware description only" rule for the rest of the DT.

Fine with me, but I was also referring to the dts source files. Ideally
even the bootloader has a devicetree dtb and a configuration overlay dtb
so that both could be changed independently.

Sascha
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/clock/clock-configuration.txt b/Documentation/devicetree/bindings/clock/clock-configuration.txt
new file mode 100644
index 0000000..482b0ff
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/clock-configuration.txt
@@ -0,0 +1,35 @@ 
+This binding allows clocks to configured by setting their parent and/or rate.
+
+Configurations are specified subnodes of nodes with
+compatible="clock-configuration"
+
+The subnode properties are:
+
+Required properties:
+clocks: 		phandle of clock to configure in clock consumer format
+
+Optional properties:
+parent:			phandle of clock to use as parent in clock consumer format
+clock-rate:		clock rate to use
+
+
+For example:
+	clock-configuration {
+		compatible = "clock-configuration";
+		clko1 {
+			clocks = <&clks 160>; /* cko1_sel */
+			parent = <&clks 114>; /* pll3_sw */
+		};
+
+		esdhca {
+			clocks = <&clks 102>; /* esdhc_a_podf */
+			clock-frequency = <200000000>;
+		};
+
+		esdhcb {
+			clocks = <&clks 103>; /* esdhc_b_podf */
+			clock-frequency = <200000000>;
+		};
+	};
+
+
diff --git a/arch/arm/mach-imx/clk-imx51-imx53.c b/arch/arm/mach-imx/clk-imx51-imx53.c
index 872a7bc..fd74795 100644
--- a/arch/arm/mach-imx/clk-imx51-imx53.c
+++ b/arch/arm/mach-imx/clk-imx51-imx53.c
@@ -432,7 +432,7 @@  int __init mx51_clocks_init(unsigned long rate_ckil, unsigned long rate_osc,
 	val |= 1 << 23;
 	writel(val, MXC_CCM_CLPCR);
 
-	return 0;
+	return of_clk_configure();
 }
 
 int __init mx53_clocks_init(unsigned long rate_ckil, unsigned long rate_osc,
@@ -523,10 +523,6 @@  int __init mx53_clocks_init(unsigned long rate_ckil, unsigned long rate_osc,
 	clk_register_clkdev(clk[dummy], "ahb", "sdhci-esdhc-imx53.3");
 	clk_register_clkdev(clk[esdhc4_per_gate], "per", "sdhci-esdhc-imx53.3");
 
-	/* set SDHC root clock to 200MHZ*/
-	clk_set_rate(clk[esdhc_a_podf], 200000000);
-	clk_set_rate(clk[esdhc_b_podf], 200000000);
-
 	/* System timer */
 	mxc_timer_init(MX53_IO_ADDRESS(MX53_GPT1_BASE_ADDR), MX53_INT_GPT);
 
@@ -536,8 +532,7 @@  int __init mx53_clocks_init(unsigned long rate_ckil, unsigned long rate_osc,
 
 	r = clk_round_rate(clk[usboh3_per_gate], 54000000);
 	clk_set_rate(clk[usboh3_per_gate], r);
-
-	return 0;
+	return of_clk_configure();
 }
 
 #ifdef CONFIG_OF
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 300d477..bf364dd 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -7,6 +7,7 @@  obj-$(CONFIG_COMMON_CLK)	+= clk-fixed-factor.o
 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-configuration.o
 
 # SoCs specific
 obj-$(CONFIG_ARCH_BCM2835)	+= clk-bcm2835.o
diff --git a/drivers/clk/clk-configuration.c b/drivers/clk/clk-configuration.c
new file mode 100644
index 0000000..ee70619
--- /dev/null
+++ b/drivers/clk/clk-configuration.c
@@ -0,0 +1,79 @@ 
+/*
+ * 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.
+ *
+ * Device tree clock parent, rate configuration
+ */
+
+#include <linux/clk.h>
+#include <linux/of.h>
+
+
+static int configure_one_clock(struct clk *clk, struct device_node *np)
+{
+	int ret;
+	struct of_phandle_args clkspec;
+	struct clk *parent;
+	u32 rate;
+
+	ret = of_parse_phandle_with_args(np, "parent", "#clock-cells", 0,
+					&clkspec);
+	if (!ret) {
+		parent = of_clk_get_from_provider(&clkspec);
+		if (!IS_ERR(parent)) {
+			ret = clk_set_parent(clk, parent);
+			clk_put(parent);
+		}
+		of_node_put(clkspec.np);
+		if (ret)
+			goto err;
+	}
+
+	ret = 0;
+	if (!of_property_read_u32(np, "clock-frequency", &rate))
+		ret = clk_set_rate(clk, rate);
+
+err:
+	return ret;
+
+}
+
+/**
+ * of_clk_configure - configure clocks from device tree
+ *
+ * Allows parent and rate to be set from nodes having
+ * clock-configuration compatible property.
+ *
+ * See binding documentation for example
+ *
+ * Returns 0 on success, -EERROR otherwise.
+ */
+int of_clk_configure()
+{
+	struct device_node *config_node, *np;
+	struct clk *clk;
+	int err_count = 0;
+	int ret = 0;
+
+	for_each_compatible_node(config_node, NULL, "clock-configuration") {
+		for_each_child_of_node(config_node, np) {
+			clk = of_clk_get(np, 0);
+			if (IS_ERR(clk)) {
+				pr_warn("%s: Failed to obtain clock configuration for %s : %d\n", __func__, np->name);
+				err_count++;
+			} else {
+				if (configure_one_clock(clk, np))
+					err_count++;
+				clk_put(clk);
+			}
+		}
+	}
+
+	if (err_count) {
+		pr_warn("%s: Failed %d clocks\n", __func__, err_count);
+		ret = -EINVAL;
+	}
+	return ret;
+}
+EXPORT_SYMBOL_GPL(of_clk_configure);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index b3ac22d..4f7f605 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -368,6 +368,7 @@  struct of_phandle_args;
 struct clk *of_clk_get(struct device_node *np, int index);
 struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
 struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec);
+int of_clk_configure(void);
 #else
 static inline struct clk *of_clk_get(struct device_node *np, int index)
 {
@@ -378,6 +379,10 @@  static inline struct clk *of_clk_get_by_name(struct device_node *np,
 {
 	return ERR_PTR(-ENOENT);
 }
+static inline int of_clk_configure(void)
+{
+	return 0;
+}
 #endif
 
 #endif