From: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> To: Martin Fuzzey <mfuzzey-mB3Nsq4MPf1BDgjK7y7TUQ@public.gmane.org> Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org, Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org Subject: Re: [RFC PATCH] CLK: Allow parent clock and rate to be configured in DT. Date: Mon, 25 Mar 2013 14:29:35 +0100 [thread overview] Message-ID: <20130325132935.GE1906@pengutronix.de> (raw) In-Reply-To: <51503007.5020403-mB3Nsq4MPf1BDgjK7y7TUQ@public.gmane.org> 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 -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
WARNING: multiple messages have this Message-ID (diff)
From: s.hauer@pengutronix.de (Sascha Hauer) To: linux-arm-kernel@lists.infradead.org Subject: [RFC PATCH] CLK: Allow parent clock and rate to be configured in DT. Date: Mon, 25 Mar 2013 14:29:35 +0100 [thread overview] Message-ID: <20130325132935.GE1906@pengutronix.de> (raw) In-Reply-To: <51503007.5020403@parkeon.com> 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 -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
next prev parent reply other threads:[~2013-03-25 13:29 UTC|newest] Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top 2013-03-19 17:09 [RFC PATCH] CLK: Allow parent clock and rate to be configured in DT Martin Fuzzey 2013-03-19 17:09 ` Martin Fuzzey 2013-03-25 10:17 ` Sascha Hauer 2013-03-25 10:17 ` Sascha Hauer [not found] ` <20130325101707.GZ1906-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> 2013-03-25 11:07 ` Martin Fuzzey 2013-03-25 11:07 ` Martin Fuzzey [not found] ` <51503007.5020403-mB3Nsq4MPf1BDgjK7y7TUQ@public.gmane.org> 2013-03-25 13:29 ` Sascha Hauer [this message] 2013-03-25 13:29 ` Sascha Hauer [not found] ` <20130325132935.GE1906-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> 2013-03-26 11:12 ` Martin Fuzzey 2013-03-26 11:12 ` Martin Fuzzey [not found] ` <51518296.7000500-mB3Nsq4MPf1BDgjK7y7TUQ@public.gmane.org> 2013-03-27 8:59 ` Sascha Hauer 2013-03-27 8:59 ` Sascha Hauer 2013-04-04 23:08 ` Fabio Estevam 2013-04-04 23:08 ` Fabio Estevam 2013-04-06 1:07 ` Matt Sealey 2013-04-06 1:07 ` Matt Sealey 2013-04-06 1:33 ` Matt Sealey 2013-04-06 1:33 ` Matt Sealey 2013-04-06 13:21 ` Tomasz Figa 2013-04-06 13:21 ` Tomasz Figa 2013-04-06 13:31 ` Tomasz Figa 2013-04-06 13:31 ` Tomasz Figa 2013-04-06 17:51 ` Martin Fuzzey 2013-04-06 17:51 ` Martin Fuzzey [not found] ` <CALBypN4mHwWZNiAQqErh1bL1sPHNuRbO5-yxzY+R1enQqEJOSQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2013-04-06 19:24 ` Matt Sealey 2013-04-06 19:24 ` Matt Sealey [not found] ` <CAKGA1b=Z4M6t4BVFyfqxq=iZ6MHGbgHf5WodGbTCSaC7E_b7FA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2013-04-06 19:40 ` Fabio Estevam 2013-04-06 19:40 ` Fabio Estevam 2013-04-07 13:26 ` Sascha Hauer 2013-04-07 13:26 ` Sascha Hauer [not found] ` <20130407132623.GP1906-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> 2013-04-07 15:50 ` Matt Sealey 2013-04-07 15:50 ` Matt Sealey [not found] ` <CAKGA1b=AcQsA-P-pR+in+9CzqW=XfEBhdoR+AC7QCLYfUhQqJg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2013-04-07 16:00 ` Fabio Estevam 2013-04-07 16:00 ` Fabio Estevam [not found] ` <CAOMZO5ARwOLdSg4Np_HB2m7zvTNE94bJBUh3R-oG=xcP4R6Y-Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2013-04-07 16:23 ` Matt Sealey 2013-04-07 16:23 ` Matt Sealey [not found] ` <CAKGA1bnt_wrNPg2JdAu=ac+WiUm8pVaGh3Tjrt3NvgdFeLxB8A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2013-04-07 16:34 ` Matt Sealey 2013-04-07 16:34 ` Matt Sealey [not found] ` <CAKGA1bnhMz-18RvUq1Bx-b_AztwspYC+Q+3QGYf=kBtMe1nq2w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2013-04-07 21:14 ` Tomasz Figa 2013-04-07 21:14 ` Tomasz Figa 2013-04-08 9:35 ` Martin Fuzzey 2013-04-08 9:35 ` Martin Fuzzey 2013-04-08 20:00 ` Sascha Hauer 2013-04-08 20:00 ` Sascha Hauer
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20130325132935.GE1906@pengutronix.de \ --to=s.hauer-bicnvbalz9megne8c9+irq@public.gmane.org \ --cc=devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org \ --cc=linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org \ --cc=mfuzzey-mB3Nsq4MPf1BDgjK7y7TUQ@public.gmane.org \ --cc=mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.