| Message ID | 20130319170933.28337.50448.stgit@localhost |
|---|---|
| State | New |
| Headers | show |
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 > >
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
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
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
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
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
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...)
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.
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
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
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
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.
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
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
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.
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?
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.
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.
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.
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
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 --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
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