From mboxrd@z Thu Jan 1 00:00:00 1970 From: mturquette@linaro.org (Mike Turquette) Date: Tue, 26 Mar 2013 19:18:44 -0700 Subject: RFC v2: Zynq Clock Controller In-Reply-To: References: <27dae808-1d3a-4001-8eb2-b0a6e2a34b8f@AM1EHSMHS013.ehs.local> <515093B4.2090701@wwwdotorg.org> <8fd7278d-37c5-48ff-8d06-edc64d532a96@VA3EHSMHS037.ehs.local> <5150DEB6.7070208@wwwdotorg.org> Message-ID: <20130327021844.4014.43809@quantum> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Quoting S?ren Brinkmann (2013-03-25 17:03:24) > On Mon, Mar 25, 2013 at 05:33:10PM -0600, Stephen Warren wrote: > > On 03/25/2013 12:27 PM, S?ren Brinkmann wrote: > > > Hi Stephen, > > > > > > On Mon, Mar 25, 2013 at 12:13:08PM -0600, Stephen Warren wrote: > > >> On 03/20/2013 05:56 PM, S?ren Brinkmann wrote: > > >>> Hi, > > >>> > > >>> I spent some time working on this and incorporating feedback. Here's an updated proposal for a clock controller for Zynq: > > >>> > > >>> Required properties: > > >>> - #clock-cells : Must be 1 > > >>> - compatible : "xlnx,ps7-clkc" (this may become 'xlnx,zynq-clkc' terminology differs a bit between Xilinx internal and mainline) > > >>> - ps-clk-frequency : Frequency of the oscillator providing ps_clk in HZ > > >>> (usually 33 MHz oscillators are used for Zynq platforms) > > >> > > >> This may have been mentioned before, but shouldn't the input clock be > > >> represented as an actual clock in DT, and hence as an entry in this > > >> node's clocks property? The crystal/... itself can be represented in DT > > >> as a fixed-clock. > > > Lars-Peter brought that up, too. Please refer to my answer to him. > > > > > >> > > >>> - clock-output-names : List of strings used to name the clock outputs. Shall be a list of the outputs given below. > > >> > > >> That shouldn't be required. > > > > > > When I want to support of_clk_get_parent_name() for my clocks, I think > > > it is. And I'm inclined to not brake this functionality. > > > > The solution here is to make clock parent names irrelevant. > > > > Also note that device tree is supposed to describe HW. As such, this > > kind of internal implementation detail of the Linux clock driver should > > have basically zero effect on the DT binding definition. > > > > >>> Optional properties: > > >>> - clocks : as described in the clock bindings > > >>> - clock-names : as described in the clock bindings > > >> > > >> I think clocks should be required, with at least the main crystal clock > > >> input always present, but perhaps having some optional entries for the > > >> (E)MIO feature you mention. > > > > > > This is why I have the xtal separate. This way these props are purely > > > optional and the xtal frequency is obtained separately. It also makes it > > > a little easier internally, because I don't have to cope with a variable > > > name for the xtal this way. > > > > > > Describing the xtal as fixed clock in DT means a mandatory entry for > > > 'clocks' and 'clock-names' and a variable name for the xtal clock. I > > > wanted to avoid this. > > > > I don't see any benefit with some properties being purely optional. > > Having optional entries in a property seems just fine to me. > > > > The name of the crystal clock should be irrelevant; that issue simply > > needs to be fixed. It's driving too much of this discussion, and it will > > be irrelevant once it's fixed. > > > > >>> Example: > > >>> clkc: clkc { > > >>> #clock-cells = <1>; > > >>> compatible = "xlnx,ps7-clkc"; > > >>> ps-clk-frequency = <33333333>; > > >>> clock-output-names = "armpll", "ddrpll", "iopll", "cpu_6or4x", "cpu_3or2x", "cpu_2x", "cpu_1x", "ddr2x", "ddr3x", "dci", "lqspi", "smc", "pcap", "gem0", "gem1", "fclk0", "fclk1", "fclk2", "fclk3", "can0", "can1", "sdio0", "sdio1", "uart0", "uart1", "spi0", "spi1", "dma", "usb0_aper", "usb1_aper", "gem0_aper", "gem1_aper", "sdio0_aper", "sdio1_aper", "spi0_aper", "spi1_aper", "can0_aper", "can1_aper", "i2c0_aper", "i2c1_aper", "uart0_aper", "uart1_aper", "gpio_aper", "lqspi_aper", "smc_aper", "swdt", "dbg_trc", "dbg_apb"; /* long list... explanation below */ > > >>> /* optional props */ > > >>> clocks = <&clkc 16>, <&clk_foo>; > > >>> clock-names = "gem1_emio_clk", "can_mio_clk_23"; > > >>> }; > > >>> > > >>> The downside of supporting this is, that I don't see a way around explicitly listing the clock output names in the DT. > > >> > > >> (Please wrap your emails to ~74 characters or so) > > > I changed my settings. > > > > > >> > > >> As Mike mentioned off-list, one can create a new clk registration API > > >> that takes a struct clk* as parent rather than a char *clk_name. > > > > > > Then we also have to make sure clocks are probed in a specific order. To > > > obtain a 'struct clk *' through clk_get() the requested clock has to be > > > already been probed. Currently clock probing relies purely on data present > > > in DT. This makes this proposal not that trivial, IMHO. > > > > Simply use deferred probe. > This would require major changes to the whole clock probing mechanism. Which mechanism are you referring to? > Currently, clocks can not defer probing. And in case of circular > dependencies in the clock tree, it would rather require a multiple steps > probe. Simply deferring won't be enough. Circular dependencies in the clock tree? Anyways the clock core maintains an orphan list that was originally designed for this problem: registering a clock before that clock's parent is registered. Just for kicks I have actually registered all of OMAP4's clocks in exactly the opposite order and everything still works. Of course this makes the clock registration parent-discovery code O(n^2), but that is the same worst case for deferred probe. As long as a key exists to link two clocks together then registration order shouldn't matter. Originally this key was an immutable clk name string. If clock parents are specified in DT then that should also suffice, in place of doing string comparisons. If no key to pair up clocks exists then yes we will have to rely purely on the struct clk *opaque_cookie and register clocks in order. Regards, Mike > > S?ren From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mike Turquette Subject: Re: RFC v2: Zynq Clock Controller Date: Tue, 26 Mar 2013 19:18:44 -0700 Message-ID: <20130327021844.4014.43809@quantum> References: <27dae808-1d3a-4001-8eb2-b0a6e2a34b8f@AM1EHSMHS013.ehs.local> <515093B4.2090701@wwwdotorg.org> <8fd7278d-37c5-48ff-8d06-edc64d532a96@VA3EHSMHS037.ehs.local> <5150DEB6.7070208@wwwdotorg.org> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: devicetree-discuss-bounces+gldd-devicetree-discuss=m.gmane.org-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org Sender: "devicetree-discuss" To: =?utf-8?q?S=C3=B6ren_Brinkmann?= , Stephen Warren Cc: Josh Cartwright , =?utf-8?q?Jan_L=C3=BCbbe?= , devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org, Michal Simek , linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Lars-Peter Clausen , git-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org, Peter Crosthwaite , Sascha Hauer , linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org List-Id: devicetree@vger.kernel.org UXVvdGluZyBTw7ZyZW4gQnJpbmttYW5uICgyMDEzLTAzLTI1IDE3OjAzOjI0KQo+IE9uIE1vbiwg TWFyIDI1LCAyMDEzIGF0IDA1OjMzOjEwUE0gLTA2MDAsIFN0ZXBoZW4gV2FycmVuIHdyb3RlOgo+ ID4gT24gMDMvMjUvMjAxMyAxMjoyNyBQTSwgU8O2cmVuIEJyaW5rbWFubiB3cm90ZToKPiA+ID4g SGkgU3RlcGhlbiwKPiA+ID4gCj4gPiA+IE9uIE1vbiwgTWFyIDI1LCAyMDEzIGF0IDEyOjEzOjA4 UE0gLTA2MDAsIFN0ZXBoZW4gV2FycmVuIHdyb3RlOgo+ID4gPj4gT24gMDMvMjAvMjAxMyAwNTo1 NiBQTSwgU8O2cmVuIEJyaW5rbWFubiB3cm90ZToKPiA+ID4+PiBIaSwKPiA+ID4+Pgo+ID4gPj4+ IEkgc3BlbnQgc29tZSB0aW1lIHdvcmtpbmcgb24gdGhpcyBhbmQgaW5jb3Jwb3JhdGluZyBmZWVk YmFjay4gSGVyZSdzIGFuIHVwZGF0ZWQgcHJvcG9zYWwgZm9yIGEgY2xvY2sgY29udHJvbGxlciBm b3IgWnlucToKPiA+ID4+Pgo+ID4gPj4+IFJlcXVpcmVkIHByb3BlcnRpZXM6Cj4gPiA+Pj4gIC0g I2Nsb2NrLWNlbGxzIDogTXVzdCBiZSAxCj4gPiA+Pj4gIC0gY29tcGF0aWJsZSA6ICJ4bG54LHBz Ny1jbGtjIiAgKHRoaXMgbWF5IGJlY29tZSAneGxueCx6eW5xLWNsa2MnIHRlcm1pbm9sb2d5IGRp ZmZlcnMgYSBiaXQgYmV0d2VlbiBYaWxpbnggaW50ZXJuYWwgYW5kIG1haW5saW5lKQo+ID4gPj4+ ICAtIHBzLWNsay1mcmVxdWVuY3kgOiBGcmVxdWVuY3kgb2YgdGhlIG9zY2lsbGF0b3IgcHJvdmlk aW5nIHBzX2NsayBpbiBIWgo+ID4gPj4+ICAgICAgICAgICAgICAgICAgICAgICh1c3VhbGx5IDMz IE1IeiBvc2NpbGxhdG9ycyBhcmUgdXNlZCBmb3IgWnlucSBwbGF0Zm9ybXMpCj4gPiA+Pgo+ID4g Pj4gVGhpcyBtYXkgaGF2ZSBiZWVuIG1lbnRpb25lZCBiZWZvcmUsIGJ1dCBzaG91bGRuJ3QgdGhl IGlucHV0IGNsb2NrIGJlCj4gPiA+PiByZXByZXNlbnRlZCBhcyBhbiBhY3R1YWwgY2xvY2sgaW4g RFQsIGFuZCBoZW5jZSBhcyBhbiBlbnRyeSBpbiB0aGlzCj4gPiA+PiBub2RlJ3MgY2xvY2tzIHBy b3BlcnR5PyBUaGUgY3J5c3RhbC8uLi4gaXRzZWxmIGNhbiBiZSByZXByZXNlbnRlZCBpbiBEVAo+ ID4gPj4gYXMgYSBmaXhlZC1jbG9jay4KPiA+ID4gTGFycy1QZXRlciBicm91Z2h0IHRoYXQgdXAs IHRvby4gUGxlYXNlIHJlZmVyIHRvIG15IGFuc3dlciB0byBoaW0uCj4gPiA+IAo+ID4gPj4KPiA+ ID4+PiAgLSBjbG9jay1vdXRwdXQtbmFtZXMgOiBMaXN0IG9mIHN0cmluZ3MgdXNlZCB0byBuYW1l IHRoZSBjbG9jayBvdXRwdXRzLiBTaGFsbCBiZSBhIGxpc3Qgb2YgdGhlIG91dHB1dHMgZ2l2ZW4g YmVsb3cuCj4gPiA+Pgo+ID4gPj4gVGhhdCBzaG91bGRuJ3QgYmUgcmVxdWlyZWQuCj4gPiA+Cj4g PiA+IFdoZW4gSSB3YW50IHRvIHN1cHBvcnQgb2ZfY2xrX2dldF9wYXJlbnRfbmFtZSgpIGZvciBt eSBjbG9ja3MsIEkgdGhpbmsKPiA+ID4gaXQgaXMuIEFuZCBJJ20gaW5jbGluZWQgdG8gbm90IGJy YWtlIHRoaXMgZnVuY3Rpb25hbGl0eS4KPiA+IAo+ID4gVGhlIHNvbHV0aW9uIGhlcmUgaXMgdG8g bWFrZSBjbG9jayBwYXJlbnQgbmFtZXMgaXJyZWxldmFudC4KPiA+IAo+ID4gQWxzbyBub3RlIHRo YXQgZGV2aWNlIHRyZWUgaXMgc3VwcG9zZWQgdG8gZGVzY3JpYmUgSFcuIEFzIHN1Y2gsIHRoaXMK PiA+IGtpbmQgb2YgaW50ZXJuYWwgaW1wbGVtZW50YXRpb24gZGV0YWlsIG9mIHRoZSBMaW51eCBj bG9jayBkcml2ZXIgc2hvdWxkCj4gPiBoYXZlIGJhc2ljYWxseSB6ZXJvIGVmZmVjdCBvbiB0aGUg RFQgYmluZGluZyBkZWZpbml0aW9uLgo+ID4gCj4gPiA+Pj4gT3B0aW9uYWwgcHJvcGVydGllczoK PiA+ID4+PiAgLSBjbG9ja3MgOiBhcyBkZXNjcmliZWQgaW4gdGhlIGNsb2NrIGJpbmRpbmdzCj4g PiA+Pj4gIC0gY2xvY2stbmFtZXMgOiBhcyBkZXNjcmliZWQgaW4gdGhlIGNsb2NrIGJpbmRpbmdz Cj4gPiA+Pgo+ID4gPj4gSSB0aGluayBjbG9ja3Mgc2hvdWxkIGJlIHJlcXVpcmVkLCB3aXRoIGF0 IGxlYXN0IHRoZSBtYWluIGNyeXN0YWwgY2xvY2sKPiA+ID4+IGlucHV0IGFsd2F5cyBwcmVzZW50 LCBidXQgcGVyaGFwcyBoYXZpbmcgc29tZSBvcHRpb25hbCBlbnRyaWVzIGZvciB0aGUKPiA+ID4+ IChFKU1JTyBmZWF0dXJlIHlvdSBtZW50aW9uLgo+ID4gPgo+ID4gPiBUaGlzIGlzIHdoeSBJIGhh dmUgdGhlIHh0YWwgc2VwYXJhdGUuIFRoaXMgd2F5IHRoZXNlIHByb3BzIGFyZSBwdXJlbHkKPiA+ ID4gb3B0aW9uYWwgYW5kIHRoZSB4dGFsIGZyZXF1ZW5jeSBpcyBvYnRhaW5lZCBzZXBhcmF0ZWx5 LiBJdCBhbHNvIG1ha2VzIGl0Cj4gPiA+IGEgbGl0dGxlIGVhc2llciBpbnRlcm5hbGx5LCBiZWNh dXNlIEkgZG9uJ3QgaGF2ZSB0byBjb3BlIHdpdGggYSB2YXJpYWJsZQo+ID4gPiBuYW1lIGZvciB0 aGUgeHRhbCB0aGlzIHdheS4KPiA+ID4gCj4gPiA+IERlc2NyaWJpbmcgdGhlIHh0YWwgYXMgZml4 ZWQgY2xvY2sgaW4gRFQgbWVhbnMgYSBtYW5kYXRvcnkgZW50cnkgZm9yCj4gPiA+ICdjbG9ja3Mn IGFuZCAnY2xvY2stbmFtZXMnIGFuZCBhIHZhcmlhYmxlIG5hbWUgZm9yIHRoZSB4dGFsIGNsb2Nr LiBJCj4gPiA+IHdhbnRlZCB0byBhdm9pZCB0aGlzLgo+ID4gCj4gPiBJIGRvbid0IHNlZSBhbnkg YmVuZWZpdCB3aXRoIHNvbWUgcHJvcGVydGllcyBiZWluZyBwdXJlbHkgb3B0aW9uYWwuCj4gPiBI YXZpbmcgb3B0aW9uYWwgZW50cmllcyBpbiBhIHByb3BlcnR5IHNlZW1zIGp1c3QgZmluZSB0byBt ZS4KPiA+IAo+ID4gVGhlIG5hbWUgb2YgdGhlIGNyeXN0YWwgY2xvY2sgc2hvdWxkIGJlIGlycmVs ZXZhbnQ7IHRoYXQgaXNzdWUgc2ltcGx5Cj4gPiBuZWVkcyB0byBiZSBmaXhlZC4gSXQncyBkcml2 aW5nIHRvbyBtdWNoIG9mIHRoaXMgZGlzY3Vzc2lvbiwgYW5kIGl0IHdpbGwKPiA+IGJlIGlycmVs ZXZhbnQgb25jZSBpdCdzIGZpeGVkLgo+ID4gCj4gPiA+Pj4gRXhhbXBsZToKPiA+ID4+PiAgICAg ICAgIGNsa2M6IGNsa2Mgewo+ID4gPj4+ICAgICAgICAgICAgICAgICAjY2xvY2stY2VsbHMgPSA8 MT47Cj4gPiA+Pj4gICAgICAgICAgICAgICAgIGNvbXBhdGlibGUgPSAieGxueCxwczctY2xrYyI7 Cj4gPiA+Pj4gICAgICAgICAgICAgICAgIHBzLWNsay1mcmVxdWVuY3kgPSA8MzMzMzMzMzM+Owo+ ID4gPj4+ICAgICAgICAgICAgICAgICBjbG9jay1vdXRwdXQtbmFtZXMgPSAiYXJtcGxsIiwgImRk cnBsbCIsICJpb3BsbCIsICJjcHVfNm9yNHgiLCAiY3B1XzNvcjJ4IiwgImNwdV8yeCIsICJjcHVf MXgiLCAiZGRyMngiLCAiZGRyM3giLCAiZGNpIiwgImxxc3BpIiwgInNtYyIsICJwY2FwIiwgImdl bTAiLCAiZ2VtMSIsICJmY2xrMCIsICJmY2xrMSIsICJmY2xrMiIsICJmY2xrMyIsICJjYW4wIiwg ImNhbjEiLCAic2RpbzAiLCAic2RpbzEiLCAidWFydDAiLCAidWFydDEiLCAic3BpMCIsICJzcGkx IiwgImRtYSIsICJ1c2IwX2FwZXIiLCAidXNiMV9hcGVyIiwgImdlbTBfYXBlciIsICJnZW0xX2Fw ZXIiLCAic2RpbzBfYXBlciIsICJzZGlvMV9hcGVyIiwgInNwaTBfYXBlciIsICJzcGkxX2FwZXIi LCAiY2FuMF9hcGVyIiwgImNhbjFfYXBlciIsICJpMmMwX2FwZXIiLCAiaTJjMV9hcGVyIiwgInVh cnQwX2FwZXIiLCAidWFydDFfYXBlciIsICJncGlvX2FwZXIiLCAibHFzcGlfYXBlciIsICJzbWNf YXBlciIsICJzd2R0IiwgImRiZ190cmMiLCAiZGJnX2FwYiI7ICAvKiBsb25nIGxpc3QuLi4gZXhw bGFuYXRpb24gYmVsb3cgKi8KPiA+ID4+PiAgICAgICAgICAgICAgICAgLyogb3B0aW9uYWwgcHJv cHMgKi8KPiA+ID4+PiAgICAgICAgICAgICAgICAgY2xvY2tzID0gPCZjbGtjIDE2PiwgPCZjbGtf Zm9vPjsKPiA+ID4+PiAgICAgICAgICAgICAgICAgY2xvY2stbmFtZXMgPSAiZ2VtMV9lbWlvX2Ns ayIsICJjYW5fbWlvX2Nsa18yMyI7Cj4gPiA+Pj4gICAgICAgICB9Owo+ID4gPj4+Cj4gPiA+Pj4g VGhlIGRvd25zaWRlIG9mIHN1cHBvcnRpbmcgdGhpcyBpcywgdGhhdCBJIGRvbid0IHNlZSBhIHdh eSBhcm91bmQgZXhwbGljaXRseSBsaXN0aW5nIHRoZSBjbG9jayBvdXRwdXQgbmFtZXMgaW4gdGhl IERULgo+ID4gPj4KPiA+ID4+IChQbGVhc2Ugd3JhcCB5b3VyIGVtYWlscyB0byB+NzQgY2hhcmFj dGVycyBvciBzbykKPiA+ID4gSSBjaGFuZ2VkIG15IHNldHRpbmdzLgo+ID4gPiAKPiA+ID4+Cj4g PiA+PiBBcyBNaWtlIG1lbnRpb25lZCBvZmYtbGlzdCwgb25lIGNhbiBjcmVhdGUgYSBuZXcgY2xr IHJlZ2lzdHJhdGlvbiBBUEkKPiA+ID4+IHRoYXQgdGFrZXMgYSBzdHJ1Y3QgY2xrKiBhcyBwYXJl bnQgcmF0aGVyIHRoYW4gYSBjaGFyICpjbGtfbmFtZS4KPiA+ID4KPiA+ID4gVGhlbiB3ZSBhbHNv IGhhdmUgdG8gbWFrZSBzdXJlIGNsb2NrcyBhcmUgcHJvYmVkIGluIGEgc3BlY2lmaWMgb3JkZXIu IFRvCj4gPiA+IG9idGFpbiBhICdzdHJ1Y3QgY2xrIConIHRocm91Z2ggY2xrX2dldCgpIHRoZSBy ZXF1ZXN0ZWQgY2xvY2sgaGFzIHRvIGJlCj4gPiA+IGFscmVhZHkgYmVlbiBwcm9iZWQuIEN1cnJl bnRseSBjbG9jayBwcm9iaW5nIHJlbGllcyBwdXJlbHkgb24gZGF0YSBwcmVzZW50Cj4gPiA+IGlu IERULiBUaGlzIG1ha2VzIHRoaXMgcHJvcG9zYWwgbm90IHRoYXQgdHJpdmlhbCwgSU1ITy4KPiA+ IAo+ID4gU2ltcGx5IHVzZSBkZWZlcnJlZCBwcm9iZS4KPiBUaGlzIHdvdWxkIHJlcXVpcmUgbWFq b3IgY2hhbmdlcyB0byB0aGUgd2hvbGUgY2xvY2sgcHJvYmluZyBtZWNoYW5pc20uCgpXaGljaCBt ZWNoYW5pc20gYXJlIHlvdSByZWZlcnJpbmcgdG8/Cgo+IEN1cnJlbnRseSwgY2xvY2tzIGNhbiBu b3QgZGVmZXIgcHJvYmluZy4gQW5kIGluIGNhc2Ugb2YgY2lyY3VsYXIKPiBkZXBlbmRlbmNpZXMg aW4gdGhlIGNsb2NrIHRyZWUsIGl0IHdvdWxkIHJhdGhlciByZXF1aXJlIGEgbXVsdGlwbGUgc3Rl cHMKPiBwcm9iZS4gU2ltcGx5IGRlZmVycmluZyB3b24ndCBiZSBlbm91Z2guCgpDaXJjdWxhciBk ZXBlbmRlbmNpZXMgaW4gdGhlIGNsb2NrIHRyZWU/CgpBbnl3YXlzIHRoZSBjbG9jayBjb3JlIG1h aW50YWlucyBhbiBvcnBoYW4gbGlzdCB0aGF0IHdhcyBvcmlnaW5hbGx5CmRlc2lnbmVkIGZvciB0 aGlzIHByb2JsZW06IHJlZ2lzdGVyaW5nIGEgY2xvY2sgYmVmb3JlIHRoYXQgY2xvY2sncwpwYXJl bnQgaXMgcmVnaXN0ZXJlZC4gIEp1c3QgZm9yIGtpY2tzIEkgaGF2ZSBhY3R1YWxseSByZWdpc3Rl cmVkIGFsbCBvZgpPTUFQNCdzIGNsb2NrcyBpbiBleGFjdGx5IHRoZSBvcHBvc2l0ZSBvcmRlciBh bmQgZXZlcnl0aGluZyBzdGlsbCB3b3Jrcy4KT2YgY291cnNlIHRoaXMgbWFrZXMgdGhlIGNsb2Nr IHJlZ2lzdHJhdGlvbiBwYXJlbnQtZGlzY292ZXJ5IGNvZGUKTyhuXjIpLCBidXQgdGhhdCBpcyB0 aGUgc2FtZSB3b3JzdCBjYXNlIGZvciBkZWZlcnJlZCBwcm9iZS4KCkFzIGxvbmcgYXMgYSBrZXkg ZXhpc3RzIHRvIGxpbmsgdHdvIGNsb2NrcyB0b2dldGhlciB0aGVuIHJlZ2lzdHJhdGlvbgpvcmRl ciBzaG91bGRuJ3QgbWF0dGVyLiAgT3JpZ2luYWxseSB0aGlzIGtleSB3YXMgYW4gaW1tdXRhYmxl IGNsayBuYW1lCnN0cmluZy4KCklmIGNsb2NrIHBhcmVudHMgYXJlIHNwZWNpZmllZCBpbiBEVCB0 aGVuIHRoYXQgc2hvdWxkIGFsc28gc3VmZmljZSwgaW4KcGxhY2Ugb2YgZG9pbmcgc3RyaW5nIGNv bXBhcmlzb25zLgoKSWYgbm8ga2V5IHRvIHBhaXIgdXAgY2xvY2tzIGV4aXN0cyB0aGVuIHllcyB3 ZSB3aWxsIGhhdmUgdG8gcmVseSBwdXJlbHkKb24gdGhlIHN0cnVjdCBjbGsgKm9wYXF1ZV9jb29r aWUgYW5kIHJlZ2lzdGVyIGNsb2NrcyBpbiBvcmRlci4KClJlZ2FyZHMsCk1pa2UKCj4gCj4gICAg ICAgICBTw7ZyZW4KX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X18KZGV2aWNldHJlZS1kaXNjdXNzIG1haWxpbmcgbGlzdApkZXZpY2V0cmVlLWRpc2N1c3NAbGlz dHMub3psYWJzLm9yZwpodHRwczovL2xpc3RzLm96bGFicy5vcmcvbGlzdGluZm8vZGV2aWNldHJl ZS1kaXNjdXNzCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753729Ab3C0CTK (ORCPT ); Tue, 26 Mar 2013 22:19:10 -0400 Received: from mail-pa0-f47.google.com ([209.85.220.47]:55690 "EHLO mail-pa0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751625Ab3C0CTH convert rfc822-to-8bit (ORCPT ); Tue, 26 Mar 2013 22:19:07 -0400 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8BIT To: =?utf-8?q?S=C3=B6ren_Brinkmann?= , Stephen Warren From: Mike Turquette In-Reply-To: Cc: Josh Cartwright , Michal Simek , Peter Crosthwaite , Lars-Peter Clausen , Prashant Gaikwad , , , , , =?utf-8?q?Jan_L=C3=BCbbe?= , Sascha Hauer , Peter De Schrijver References: <27dae808-1d3a-4001-8eb2-b0a6e2a34b8f@AM1EHSMHS013.ehs.local> <515093B4.2090701@wwwdotorg.org> <8fd7278d-37c5-48ff-8d06-edc64d532a96@VA3EHSMHS037.ehs.local> <5150DEB6.7070208@wwwdotorg.org> Message-ID: <20130327021844.4014.43809@quantum> User-Agent: alot/0.3.3+ Subject: Re: RFC v2: Zynq Clock Controller Date: Tue, 26 Mar 2013 19:18:44 -0700 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Quoting Sören Brinkmann (2013-03-25 17:03:24) > On Mon, Mar 25, 2013 at 05:33:10PM -0600, Stephen Warren wrote: > > On 03/25/2013 12:27 PM, Sören Brinkmann wrote: > > > Hi Stephen, > > > > > > On Mon, Mar 25, 2013 at 12:13:08PM -0600, Stephen Warren wrote: > > >> On 03/20/2013 05:56 PM, Sören Brinkmann wrote: > > >>> Hi, > > >>> > > >>> I spent some time working on this and incorporating feedback. Here's an updated proposal for a clock controller for Zynq: > > >>> > > >>> Required properties: > > >>> - #clock-cells : Must be 1 > > >>> - compatible : "xlnx,ps7-clkc" (this may become 'xlnx,zynq-clkc' terminology differs a bit between Xilinx internal and mainline) > > >>> - ps-clk-frequency : Frequency of the oscillator providing ps_clk in HZ > > >>> (usually 33 MHz oscillators are used for Zynq platforms) > > >> > > >> This may have been mentioned before, but shouldn't the input clock be > > >> represented as an actual clock in DT, and hence as an entry in this > > >> node's clocks property? The crystal/... itself can be represented in DT > > >> as a fixed-clock. > > > Lars-Peter brought that up, too. Please refer to my answer to him. > > > > > >> > > >>> - clock-output-names : List of strings used to name the clock outputs. Shall be a list of the outputs given below. > > >> > > >> That shouldn't be required. > > > > > > When I want to support of_clk_get_parent_name() for my clocks, I think > > > it is. And I'm inclined to not brake this functionality. > > > > The solution here is to make clock parent names irrelevant. > > > > Also note that device tree is supposed to describe HW. As such, this > > kind of internal implementation detail of the Linux clock driver should > > have basically zero effect on the DT binding definition. > > > > >>> Optional properties: > > >>> - clocks : as described in the clock bindings > > >>> - clock-names : as described in the clock bindings > > >> > > >> I think clocks should be required, with at least the main crystal clock > > >> input always present, but perhaps having some optional entries for the > > >> (E)MIO feature you mention. > > > > > > This is why I have the xtal separate. This way these props are purely > > > optional and the xtal frequency is obtained separately. It also makes it > > > a little easier internally, because I don't have to cope with a variable > > > name for the xtal this way. > > > > > > Describing the xtal as fixed clock in DT means a mandatory entry for > > > 'clocks' and 'clock-names' and a variable name for the xtal clock. I > > > wanted to avoid this. > > > > I don't see any benefit with some properties being purely optional. > > Having optional entries in a property seems just fine to me. > > > > The name of the crystal clock should be irrelevant; that issue simply > > needs to be fixed. It's driving too much of this discussion, and it will > > be irrelevant once it's fixed. > > > > >>> Example: > > >>> clkc: clkc { > > >>> #clock-cells = <1>; > > >>> compatible = "xlnx,ps7-clkc"; > > >>> ps-clk-frequency = <33333333>; > > >>> clock-output-names = "armpll", "ddrpll", "iopll", "cpu_6or4x", "cpu_3or2x", "cpu_2x", "cpu_1x", "ddr2x", "ddr3x", "dci", "lqspi", "smc", "pcap", "gem0", "gem1", "fclk0", "fclk1", "fclk2", "fclk3", "can0", "can1", "sdio0", "sdio1", "uart0", "uart1", "spi0", "spi1", "dma", "usb0_aper", "usb1_aper", "gem0_aper", "gem1_aper", "sdio0_aper", "sdio1_aper", "spi0_aper", "spi1_aper", "can0_aper", "can1_aper", "i2c0_aper", "i2c1_aper", "uart0_aper", "uart1_aper", "gpio_aper", "lqspi_aper", "smc_aper", "swdt", "dbg_trc", "dbg_apb"; /* long list... explanation below */ > > >>> /* optional props */ > > >>> clocks = <&clkc 16>, <&clk_foo>; > > >>> clock-names = "gem1_emio_clk", "can_mio_clk_23"; > > >>> }; > > >>> > > >>> The downside of supporting this is, that I don't see a way around explicitly listing the clock output names in the DT. > > >> > > >> (Please wrap your emails to ~74 characters or so) > > > I changed my settings. > > > > > >> > > >> As Mike mentioned off-list, one can create a new clk registration API > > >> that takes a struct clk* as parent rather than a char *clk_name. > > > > > > Then we also have to make sure clocks are probed in a specific order. To > > > obtain a 'struct clk *' through clk_get() the requested clock has to be > > > already been probed. Currently clock probing relies purely on data present > > > in DT. This makes this proposal not that trivial, IMHO. > > > > Simply use deferred probe. > This would require major changes to the whole clock probing mechanism. Which mechanism are you referring to? > Currently, clocks can not defer probing. And in case of circular > dependencies in the clock tree, it would rather require a multiple steps > probe. Simply deferring won't be enough. Circular dependencies in the clock tree? Anyways the clock core maintains an orphan list that was originally designed for this problem: registering a clock before that clock's parent is registered. Just for kicks I have actually registered all of OMAP4's clocks in exactly the opposite order and everything still works. Of course this makes the clock registration parent-discovery code O(n^2), but that is the same worst case for deferred probe. As long as a key exists to link two clocks together then registration order shouldn't matter. Originally this key was an immutable clk name string. If clock parents are specified in DT then that should also suffice, in place of doing string comparisons. If no key to pair up clocks exists then yes we will have to rely purely on the struct clk *opaque_cookie and register clocks in order. Regards, Mike > > Sören