From mboxrd@z Thu Jan 1 00:00:00 1970 From: lars@metafoo.de (Lars-Peter Clausen) Date: Fri, 08 Mar 2013 19:08:10 +0100 Subject: RFC: Zynq Clock Controller In-Reply-To: References: <37032699-343e-485c-80e0-9b23e3706c58@VA3EHSMHS013.ehs.local> <1362570681.5269.98.camel@coredoba.hi.pengutronix.de> <51385FA3.8080105@metafoo.de> <2a91857c-473f-4f92-b2b1-e7ac4679b72a@AM1EHSMHS007.ehs.local> <51390E92.2080806@metafoo.de> <51398F54.5080802@metafoo.de> Message-ID: <513A290A.3060502@metafoo.de> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 03/08/2013 06:38 PM, S?ren Brinkmann wrote: > On Fri, Mar 08, 2013 at 08:12:20AM +0100, Lars-Peter Clausen wrote: >> On 03/08/2013 12:25 AM, S?ren Brinkmann wrote: >>> On Thu, Mar 07, 2013 at 11:02:58PM +0100, Lars-Peter Clausen wrote: >>>> On 03/07/2013 08:11 PM, S?ren Brinkmann wrote: >>>>> On Thu, Mar 07, 2013 at 10:36:35AM +0100, Lars-Peter Clausen wrote: >>>>>> On 03/06/2013 06:27 PM, S?ren Brinkmann wrote: >>>>>>> Hi Jan, >>>>>>> >>>>>>> what a small world. Good to hear from you. >>>>>>> >>>>>>> On Wed, Mar 06, 2013 at 12:51:21PM +0100, Jan L?bbe wrote: >>>>>>>> Hi S?ren, >>>>>>>> >>>>>>>> On Tue, 2013-03-05 at 12:04 -0800, S?ren Brinkmann wrote: >>>>>>>>> For this reasons, I'd like to propose moving Zynq into the same >>>>>>>>> direction. I.e. adding a clock controller with the following DT >>>>>>>>> description (details may change but the general idea should become >>>>>>>>> clear): >>>>>>>>> clkc: clkc { >>>>>>>>> #clock-cells = <1>; >>>>>>>>> compatible = "xlnx,ps7-clkc"; >>>>>>>>> ps_clk_frequency = <33333333>; # board x-tal >>>>>>>>> # optional props >>>>>>>>> gem0_emio_clk_freq = <125000000>; >>>>>>>>> gem1_emio_clk_freq = <50000000>; >>>>>>>>> can_mio_clk_freq_xx = <1234>; # this is possible 54 times with xx = 00..53 >>>>>>>>> }; >>>>>> >>>>>> I definitely prefer the way it is right now in upstream, where we have one >>>>>> dt node per clock. It is more descriptive and also more extensible. And you >>>>>> also don't have to remember the clock index, and can use the phandle >>>>>> directly instead. >>>>> The problems I see with that are: >>>>> - with the clock controller we can model the clock tree as we need without changing the DT all the time >>>> >>>> When do we need to change the clock tree? The clock tree is pretty much fixed >>>> in hardware. And the DT describes the hardware, so I don't so a problem there. >>>> >>>>> - w/o a clock controller there is no block which has knowledge of the whole clock tree (unless we parse the DT), and could conduct reparent operations and similar >>>> >>>> The clk framework builds a representation of the clock tree. Each clock should >>>> be able to handle re-parenting on it's own, without knowing about the other >>>> clocks in the tree, the parent is selected by a field in the clocks register. >>>> It doesn't even have to know who the parents are, the clk framework will take >>>> care of all of this and just say, ok, switch your input clock to X, where X is >>>> a simple integer number The clk framework will also take care of re-calculating >>>> all the updates frequencies after re-parenting. >>> Nope, clk_set_parent() takes two 'struct *clk' as argument. So, you have to have those of the clock to change and all its parents. A device driver for example, which gets a clock through clk_get() does not have that information and should not, since it should not have to be aware of the SOC's clock hierarchy, IMHO. >>> >> >> Yes, you global clk_set_parent() method takes two clk structs. But the clock >> framework takes care of all the magic of mapping that second clk struct to a >> integer number, based on the clks parent list. So your set_parent callback in >> your clk_ops takes as parameters your clk and the index of the parent. > Right, but who/what in your scheme is supposed to call the high level API function requiring knowledge about all those 'struct *clk' and their hierarchy? This is where I think a big block has the advantage since it has a holistic view on the clock tree. > How would you use the clock controller driver to do re-parent operations? I really can't see how it will help to implement re-parent operations. >> >>>> >>>>> - once the clock controller is properly defined the clock connection should be contained in a dtsi which never changes. So, regarding remembering outputs, I don't see a problem. I rather see issues with having a pile of clocks described in the DT. I have currently 44 outputs proposed, and to model the whole tree a lot of more clocks are used (I started modeling everything with the clock primitives and removed custom clock implementations, except for the PLLs). Having all those in the DT does not really help, IMHO. >>>> >>>> Well, except if you want to use external clocks for ethernet or CAN. Also you >>>> don't need to change the dtsi either if you have a separate node for each clock. >>>> >>>> To avoid misunderstandings let me clarify that I don't want to have one node >>>> per clock, but rather one node per clock block (or whatever they are called). >>>> The combination of input select + divider + output gate. E.g. the UART clock >>>> block with it's 3 inputs and two outputs. >>>> >>>> Also the APER clocks should probably be one node with 24 outputs. >>> Okay, so instead of having one block encapsulating all clocks, you want it on a finer granularity. I don't see huge differences why that should be advantageous? It would just mean to create several blocks with their custom DT bindings instead of a single one. Just the abstraction level would be a little different. >> >> The DT is supposed to describe the hardware, not the software. These are the >> basic blocks. The are independent of each other and mostly orthogonal. It's the >> same IP block instantiated a couple of times next to each other (sometimes with >> slightly different parameters). They just happen to have their register mapped >> in the same IO region. The SLCR is just a container which contains all these >> blocks. Very much the same way the whole SoC is a container which contains the >> different peripheral blocks, etc. By you rationale we could argue, why do we >> need a dt node for each peripheral and not just simply one ZYNQ node? > I see us ending up here with custom clock blocks for each and every peripheral. They all look similar on first look but have all their slight differences. So, it's either different implementations or having a lot of DT properties to make a single implementation work for all the variations. That's why I prefer the single controller solution. > The DT description will always be an abstraction. You prefer grouping clocks for functional units, whereas I put them all into the same drawer labeled 'clocks'. So how many parameters are there? - Number of input clocks: We get these by listing the parent clocks in the clocks property. - Number of output clocks: We get this through the output-clock-names property. - Whether the output clocks can be gated, this requires a custom property. - Number of 6bit dividers in the chain, can be 0, 1 or 2. Needs a custom property as well So with these 4 properties you can describe ~90% of the clocks found on the Zynq. And then there are the CAN and GEM clocks, which probably require custom clock implementation since they are kind of special. Your approach ends up with hundreds of lines of open-coding the instantiation of all the clock gates and dividers found in the slcr. That's just madness. - Lars From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lars-Peter Clausen Subject: Re: RFC: Zynq Clock Controller Date: Fri, 08 Mar 2013 19:08:10 +0100 Message-ID: <513A290A.3060502@metafoo.de> References: <37032699-343e-485c-80e0-9b23e3706c58@VA3EHSMHS013.ehs.local> <1362570681.5269.98.camel@coredoba.hi.pengutronix.de> <51385FA3.8080105@metafoo.de> <2a91857c-473f-4f92-b2b1-e7ac4679b72a@AM1EHSMHS007.ehs.local> <51390E92.2080806@metafoo.de> <51398F54.5080802@metafoo.de> 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?B?U8O2cmVuIEJyaW5rbWFubg==?= Cc: Josh Cartwright , Mike Turquette , =?UTF-8?B?SmFuIEzDvGJiZQ==?= , Sascha Hauer , Michal Simek , linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, git-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org, Peter Crosthwaite , devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org List-Id: devicetree@vger.kernel.org T24gMDMvMDgvMjAxMyAwNjozOCBQTSwgU8O2cmVuIEJyaW5rbWFubiB3cm90ZToKPiBPbiBGcmks IE1hciAwOCwgMjAxMyBhdCAwODoxMjoyMEFNICswMTAwLCBMYXJzLVBldGVyIENsYXVzZW4gd3Jv dGU6Cj4+IE9uIDAzLzA4LzIwMTMgMTI6MjUgQU0sIFPDtnJlbiBCcmlua21hbm4gd3JvdGU6Cj4+ PiBPbiBUaHUsIE1hciAwNywgMjAxMyBhdCAxMTowMjo1OFBNICswMTAwLCBMYXJzLVBldGVyIENs YXVzZW4gd3JvdGU6Cj4+Pj4gT24gMDMvMDcvMjAxMyAwODoxMSBQTSwgU8O2cmVuIEJyaW5rbWFu biB3cm90ZToKPj4+Pj4gT24gVGh1LCBNYXIgMDcsIDIwMTMgYXQgMTA6MzY6MzVBTSArMDEwMCwg TGFycy1QZXRlciBDbGF1c2VuIHdyb3RlOgo+Pj4+Pj4gT24gMDMvMDYvMjAxMyAwNjoyNyBQTSwg U8O2cmVuIEJyaW5rbWFubiB3cm90ZToKPj4+Pj4+PiBIaSBKYW4sCj4+Pj4+Pj4gIAo+Pj4+Pj4+ IHdoYXQgYSBzbWFsbCB3b3JsZC4gR29vZCB0byBoZWFyIGZyb20geW91Lgo+Pj4+Pj4+Cj4+Pj4+ Pj4gT24gV2VkLCBNYXIgMDYsIDIwMTMgYXQgMTI6NTE6MjFQTSArMDEwMCwgSmFuIEzDvGJiZSB3 cm90ZToKPj4+Pj4+Pj4gSGkgU8O2cmVuLAo+Pj4+Pj4+Pgo+Pj4+Pj4+PiBPbiBUdWUsIDIwMTMt MDMtMDUgYXQgMTI6MDQgLTA4MDAsIFPDtnJlbiBCcmlua21hbm4gd3JvdGU6Cj4+Pj4+Pj4+PiBG b3IgdGhpcyByZWFzb25zLCBJJ2QgbGlrZSB0byBwcm9wb3NlIG1vdmluZyBaeW5xIGludG8gdGhl IHNhbWUKPj4+Pj4+Pj4+IGRpcmVjdGlvbi4gSS5lLiBhZGRpbmcgYSBjbG9jayBjb250cm9sbGVy IHdpdGggdGhlIGZvbGxvd2luZyBEVAo+Pj4+Pj4+Pj4gZGVzY3JpcHRpb24gKGRldGFpbHMgbWF5 IGNoYW5nZSBidXQgdGhlIGdlbmVyYWwgaWRlYSBzaG91bGQgYmVjb21lCj4+Pj4+Pj4+PiBjbGVh cik6Cj4+Pj4+Pj4+PiAJY2xrYzogY2xrYyB7Cj4+Pj4+Pj4+PiAgICAgICAgICAgICAgICAgI2Ns b2NrLWNlbGxzID0gPDE+Owo+Pj4+Pj4+Pj4gICAgICAgICAgICAgICAgIGNvbXBhdGlibGUgPSAi eGxueCxwczctY2xrYyI7Cj4+Pj4+Pj4+PiAgICAgICAgICAgICAgICAgcHNfY2xrX2ZyZXF1ZW5j eSA9IDwzMzMzMzMzMz47CSMgYm9hcmQgeC10YWwKPj4+Pj4+Pj4+ICAgICAgICAgICAgICAgICAj IG9wdGlvbmFsIHByb3BzCj4+Pj4+Pj4+PiAgICAgICAgICAgICAgICAgZ2VtMF9lbWlvX2Nsa19m cmVxID0gPDEyNTAwMDAwMD47Cj4+Pj4+Pj4+PiAgICAgICAgICAgICAgICAgZ2VtMV9lbWlvX2Ns a19mcmVxID0gPDUwMDAwMDAwPjsKPj4+Pj4+Pj4+ICAgICAgICAgICAgICAgICBjYW5fbWlvX2Ns a19mcmVxX3h4ID0gPDEyMzQ+OyAjIHRoaXMgaXMgcG9zc2libGUgNTQgdGltZXMgd2l0aCB4eCA9 IDAwLi41Mwo+Pj4+Pj4+Pj4gICAgICAgICB9Owo+Pj4+Pj4KPj4+Pj4+IEkgZGVmaW5pdGVseSBw cmVmZXIgdGhlIHdheSBpdCBpcyByaWdodCBub3cgaW4gdXBzdHJlYW0sIHdoZXJlIHdlIGhhdmUg b25lCj4+Pj4+PiBkdCBub2RlIHBlciBjbG9jay4gSXQgaXMgbW9yZSBkZXNjcmlwdGl2ZSBhbmQg YWxzbyBtb3JlIGV4dGVuc2libGUuIEFuZCB5b3UKPj4+Pj4+IGFsc28gZG9uJ3QgaGF2ZSB0byBy ZW1lbWJlciB0aGUgY2xvY2sgaW5kZXgsIGFuZCBjYW4gdXNlIHRoZSBwaGFuZGxlCj4+Pj4+PiBk aXJlY3RseSBpbnN0ZWFkLgo+Pj4+PiBUaGUgcHJvYmxlbXMgSSBzZWUgd2l0aCB0aGF0IGFyZToK Pj4+Pj4gIC0gd2l0aCB0aGUgY2xvY2sgY29udHJvbGxlciB3ZSBjYW4gbW9kZWwgdGhlIGNsb2Nr IHRyZWUgYXMgd2UgbmVlZCB3aXRob3V0IGNoYW5naW5nIHRoZSBEVCBhbGwgdGhlIHRpbWUKPj4+ Pgo+Pj4+IFdoZW4gZG8gd2UgbmVlZCB0byBjaGFuZ2UgdGhlIGNsb2NrIHRyZWU/IFRoZSBjbG9j ayB0cmVlIGlzIHByZXR0eSBtdWNoIGZpeGVkCj4+Pj4gaW4gaGFyZHdhcmUuIEFuZCB0aGUgRFQg ZGVzY3JpYmVzIHRoZSBoYXJkd2FyZSwgc28gSSBkb24ndCBzbyBhIHByb2JsZW0gdGhlcmUuCj4+ Pj4KPj4+Pj4gIC0gdy9vIGEgY2xvY2sgY29udHJvbGxlciB0aGVyZSBpcyBubyBibG9jayB3aGlj aCBoYXMga25vd2xlZGdlIG9mIHRoZSB3aG9sZSBjbG9jayB0cmVlICh1bmxlc3Mgd2UgcGFyc2Ug dGhlIERUKSwgYW5kIGNvdWxkIGNvbmR1Y3QgcmVwYXJlbnQgb3BlcmF0aW9ucyBhbmQgc2ltaWxh cgo+Pj4+Cj4+Pj4gVGhlIGNsayBmcmFtZXdvcmsgYnVpbGRzIGEgcmVwcmVzZW50YXRpb24gb2Yg dGhlIGNsb2NrIHRyZWUuIEVhY2ggY2xvY2sgc2hvdWxkCj4+Pj4gYmUgYWJsZSB0byBoYW5kbGUg cmUtcGFyZW50aW5nIG9uIGl0J3Mgb3duLCB3aXRob3V0IGtub3dpbmcgYWJvdXQgdGhlIG90aGVy Cj4+Pj4gY2xvY2tzIGluIHRoZSB0cmVlLCB0aGUgcGFyZW50IGlzIHNlbGVjdGVkIGJ5IGEgZmll bGQgaW4gdGhlIGNsb2NrcyByZWdpc3Rlci4KPj4+PiBJdCBkb2Vzbid0IGV2ZW4gaGF2ZSB0byBr bm93IHdobyB0aGUgcGFyZW50cyBhcmUsIHRoZSBjbGsgZnJhbWV3b3JrIHdpbGwgdGFrZQo+Pj4+ IGNhcmUgb2YgYWxsIG9mIHRoaXMgYW5kIGp1c3Qgc2F5LCBvaywgc3dpdGNoIHlvdXIgaW5wdXQg Y2xvY2sgdG8gWCwgd2hlcmUgWCBpcwo+Pj4+IGEgc2ltcGxlIGludGVnZXIgbnVtYmVyIFRoZSBj bGsgZnJhbWV3b3JrIHdpbGwgYWxzbyB0YWtlIGNhcmUgb2YgcmUtY2FsY3VsYXRpbmcKPj4+PiBh bGwgdGhlIHVwZGF0ZXMgZnJlcXVlbmNpZXMgYWZ0ZXIgcmUtcGFyZW50aW5nLgo+Pj4gTm9wZSwg Y2xrX3NldF9wYXJlbnQoKSB0YWtlcyB0d28gJ3N0cnVjdCAqY2xrJyBhcyBhcmd1bWVudC4gU28s IHlvdSBoYXZlIHRvIGhhdmUgdGhvc2Ugb2YgdGhlIGNsb2NrIHRvIGNoYW5nZSBhbmQgYWxsIGl0 cyBwYXJlbnRzLiBBIGRldmljZSBkcml2ZXIgZm9yIGV4YW1wbGUsIHdoaWNoIGdldHMgYSBjbG9j ayB0aHJvdWdoIGNsa19nZXQoKSBkb2VzIG5vdCBoYXZlIHRoYXQgaW5mb3JtYXRpb24gYW5kIHNo b3VsZCBub3QsIHNpbmNlIGl0IHNob3VsZCBub3QgaGF2ZSB0byBiZSBhd2FyZSBvZiB0aGUgU09D J3MgY2xvY2sgaGllcmFyY2h5LCBJTUhPLgo+Pj4KPj4KPj4gWWVzLCB5b3UgZ2xvYmFsIGNsa19z ZXRfcGFyZW50KCkgbWV0aG9kIHRha2VzIHR3byBjbGsgc3RydWN0cy4gQnV0IHRoZSBjbG9jawo+ PiBmcmFtZXdvcmsgdGFrZXMgY2FyZSBvZiBhbGwgdGhlIG1hZ2ljIG9mIG1hcHBpbmcgdGhhdCBz ZWNvbmQgY2xrIHN0cnVjdCB0byBhCj4+IGludGVnZXIgbnVtYmVyLCBiYXNlZCBvbiB0aGUgY2xr cyBwYXJlbnQgbGlzdC4gU28geW91ciBzZXRfcGFyZW50IGNhbGxiYWNrIGluCj4+IHlvdXIgY2xr X29wcyB0YWtlcyBhcyBwYXJhbWV0ZXJzIHlvdXIgY2xrIGFuZCB0aGUgaW5kZXggb2YgdGhlIHBh cmVudC4KPiBSaWdodCwgYnV0IHdoby93aGF0IGluIHlvdXIgc2NoZW1lIGlzIHN1cHBvc2VkIHRv IGNhbGwgdGhlIGhpZ2ggbGV2ZWwgQVBJIGZ1bmN0aW9uIHJlcXVpcmluZyBrbm93bGVkZ2UgYWJv dXQgYWxsIHRob3NlICdzdHJ1Y3QgKmNsaycgYW5kIHRoZWlyIGhpZXJhcmNoeT8gVGhpcyBpcyB3 aGVyZSBJIHRoaW5rIGEgYmlnIGJsb2NrIGhhcyB0aGUgYWR2YW50YWdlIHNpbmNlIGl0IGhhcyBh IGhvbGlzdGljIHZpZXcgb24gdGhlIGNsb2NrIHRyZWUuCj4gCgpIb3cgd291bGQgeW91IHVzZSB0 aGUgY2xvY2sgY29udHJvbGxlciBkcml2ZXIgdG8gZG8gcmUtcGFyZW50IG9wZXJhdGlvbnM/IEkK cmVhbGx5IGNhbid0IHNlZSBob3cgaXQgd2lsbCBoZWxwIHRvIGltcGxlbWVudCByZS1wYXJlbnQg b3BlcmF0aW9ucy4KCj4+Cj4+Pj4KPj4+Pj4gIC0gb25jZSB0aGUgY2xvY2sgY29udHJvbGxlciBp cyBwcm9wZXJseSBkZWZpbmVkIHRoZSBjbG9jayBjb25uZWN0aW9uIHNob3VsZCBiZSBjb250YWlu ZWQgaW4gYSBkdHNpIHdoaWNoIG5ldmVyIGNoYW5nZXMuIFNvLCByZWdhcmRpbmcgcmVtZW1iZXJp bmcgb3V0cHV0cywgSSBkb24ndCBzZWUgYSBwcm9ibGVtLiBJIHJhdGhlciBzZWUgaXNzdWVzIHdp dGggaGF2aW5nIGEgcGlsZSBvZiBjbG9ja3MgZGVzY3JpYmVkIGluIHRoZSBEVC4gSSBoYXZlIGN1 cnJlbnRseSA0NCBvdXRwdXRzIHByb3Bvc2VkLCBhbmQgdG8gbW9kZWwgdGhlIHdob2xlIHRyZWUg YSBsb3Qgb2YgbW9yZSBjbG9ja3MgYXJlIHVzZWQgKEkgc3RhcnRlZCBtb2RlbGluZyBldmVyeXRo aW5nIHdpdGggdGhlIGNsb2NrIHByaW1pdGl2ZXMgYW5kIHJlbW92ZWQgY3VzdG9tIGNsb2NrIGlt cGxlbWVudGF0aW9ucywgZXhjZXB0IGZvciB0aGUgUExMcykuIEhhdmluZyBhbGwgdGhvc2UgaW4g dGhlIERUIGRvZXMgbm90IHJlYWxseSBoZWxwLCBJTUhPLgo+Pj4+Cj4+Pj4gV2VsbCwgZXhjZXB0 IGlmIHlvdSB3YW50IHRvIHVzZSBleHRlcm5hbCBjbG9ja3MgZm9yIGV0aGVybmV0IG9yIENBTi4g QWxzbyB5b3UKPj4+PiBkb24ndCBuZWVkIHRvIGNoYW5nZSB0aGUgZHRzaSBlaXRoZXIgaWYgeW91 IGhhdmUgYSBzZXBhcmF0ZSBub2RlIGZvciBlYWNoIGNsb2NrLgo+Pj4+Cj4+Pj4gVG8gYXZvaWQg bWlzdW5kZXJzdGFuZGluZ3MgbGV0IG1lIGNsYXJpZnkgdGhhdCBJIGRvbid0IHdhbnQgdG8gaGF2 ZSBvbmUgbm9kZQo+Pj4+IHBlciBjbG9jaywgYnV0IHJhdGhlciBvbmUgbm9kZSBwZXIgY2xvY2sg YmxvY2sgKG9yIHdoYXRldmVyIHRoZXkgYXJlIGNhbGxlZCkuCj4+Pj4gVGhlIGNvbWJpbmF0aW9u IG9mIGlucHV0IHNlbGVjdCArIGRpdmlkZXIgKyBvdXRwdXQgZ2F0ZS4gRS5nLiB0aGUgVUFSVCBj bG9jawo+Pj4+IGJsb2NrIHdpdGggaXQncyAzIGlucHV0cyBhbmQgdHdvIG91dHB1dHMuCj4+Pj4K Pj4+PiBBbHNvIHRoZSBBUEVSIGNsb2NrcyBzaG91bGQgcHJvYmFibHkgYmUgb25lIG5vZGUgd2l0 aCAyNCBvdXRwdXRzLgo+Pj4gT2theSwgc28gaW5zdGVhZCBvZiBoYXZpbmcgb25lIGJsb2NrIGVu Y2Fwc3VsYXRpbmcgYWxsIGNsb2NrcywgeW91IHdhbnQgaXQgb24gYSBmaW5lciBncmFudWxhcml0 eS4gSSBkb24ndCBzZWUgaHVnZSBkaWZmZXJlbmNlcyB3aHkgdGhhdCBzaG91bGQgYmUgYWR2YW50 YWdlb3VzPyBJdCB3b3VsZCBqdXN0IG1lYW4gdG8gY3JlYXRlIHNldmVyYWwgYmxvY2tzIHdpdGgg dGhlaXIgY3VzdG9tIERUIGJpbmRpbmdzIGluc3RlYWQgb2YgYSBzaW5nbGUgb25lLiBKdXN0IHRo ZSBhYnN0cmFjdGlvbiBsZXZlbCB3b3VsZCBiZSBhIGxpdHRsZSBkaWZmZXJlbnQuCj4+Cj4+IFRo ZSBEVCBpcyBzdXBwb3NlZCB0byBkZXNjcmliZSB0aGUgaGFyZHdhcmUsIG5vdCB0aGUgc29mdHdh cmUuIFRoZXNlIGFyZSB0aGUKPj4gYmFzaWMgYmxvY2tzLiBUaGUgYXJlIGluZGVwZW5kZW50IG9m IGVhY2ggb3RoZXIgYW5kIG1vc3RseSBvcnRob2dvbmFsLiBJdCdzIHRoZQo+PiBzYW1lIElQIGJs b2NrIGluc3RhbnRpYXRlZCBhIGNvdXBsZSBvZiB0aW1lcyBuZXh0IHRvIGVhY2ggb3RoZXIgKHNv bWV0aW1lcyB3aXRoCj4+IHNsaWdodGx5IGRpZmZlcmVudCBwYXJhbWV0ZXJzKS4gVGhleSBqdXN0 IGhhcHBlbiB0byBoYXZlIHRoZWlyIHJlZ2lzdGVyIG1hcHBlZAo+PiBpbiB0aGUgc2FtZSBJTyBy ZWdpb24uIFRoZSBTTENSIGlzIGp1c3QgYSBjb250YWluZXIgd2hpY2ggY29udGFpbnMgYWxsIHRo ZXNlCj4+IGJsb2Nrcy4gVmVyeSBtdWNoIHRoZSBzYW1lIHdheSB0aGUgd2hvbGUgU29DIGlzIGEg Y29udGFpbmVyIHdoaWNoIGNvbnRhaW5zIHRoZQo+PiBkaWZmZXJlbnQgcGVyaXBoZXJhbCBibG9j a3MsIGV0Yy4gQnkgeW91IHJhdGlvbmFsZSB3ZSBjb3VsZCBhcmd1ZSwgd2h5IGRvIHdlCj4+IG5l ZWQgYSBkdCBub2RlIGZvciBlYWNoIHBlcmlwaGVyYWwgYW5kIG5vdCBqdXN0IHNpbXBseSBvbmUg WllOUSBub2RlPwo+IEkgc2VlIHVzIGVuZGluZyB1cCBoZXJlIHdpdGggY3VzdG9tIGNsb2NrIGJs b2NrcyBmb3IgZWFjaCBhbmQgZXZlcnkgcGVyaXBoZXJhbC4gVGhleSBhbGwgbG9vayBzaW1pbGFy IG9uIGZpcnN0IGxvb2sgYnV0IGhhdmUgYWxsIHRoZWlyIHNsaWdodCBkaWZmZXJlbmNlcy4gU28s IGl0J3MgZWl0aGVyIGRpZmZlcmVudCBpbXBsZW1lbnRhdGlvbnMgb3IgaGF2aW5nIGEgbG90IG9m IERUIHByb3BlcnRpZXMgdG8gbWFrZSBhIHNpbmdsZSBpbXBsZW1lbnRhdGlvbiB3b3JrIGZvciBh bGwgdGhlIHZhcmlhdGlvbnMuIFRoYXQncyB3aHkgSSBwcmVmZXIgdGhlIHNpbmdsZSBjb250cm9s bGVyIHNvbHV0aW9uLgo+IFRoZSBEVCBkZXNjcmlwdGlvbiB3aWxsIGFsd2F5cyBiZSBhbiBhYnN0 cmFjdGlvbi4gWW91IHByZWZlciBncm91cGluZyBjbG9ja3MgZm9yIGZ1bmN0aW9uYWwgdW5pdHMs IHdoZXJlYXMgSSBwdXQgdGhlbSBhbGwgaW50byB0aGUgc2FtZSBkcmF3ZXIgbGFiZWxlZCAnY2xv Y2tzJy4KClNvIGhvdyBtYW55IHBhcmFtZXRlcnMgYXJlIHRoZXJlPwotIE51bWJlciBvZiBpbnB1 dCBjbG9ja3M6IFdlIGdldCB0aGVzZSBieSBsaXN0aW5nIHRoZSBwYXJlbnQgY2xvY2tzIGluIHRo ZQpjbG9ja3MgcHJvcGVydHkuCi0gTnVtYmVyIG9mIG91dHB1dCBjbG9ja3M6IFdlIGdldCB0aGlz IHRocm91Z2ggdGhlIG91dHB1dC1jbG9jay1uYW1lcwogIHByb3BlcnR5LgotIFdoZXRoZXIgdGhl IG91dHB1dCBjbG9ja3MgY2FuIGJlIGdhdGVkLCB0aGlzIHJlcXVpcmVzIGEgY3VzdG9tIHByb3Bl cnR5LgotIE51bWJlciBvZiA2Yml0IGRpdmlkZXJzIGluIHRoZSBjaGFpbiwgY2FuIGJlIDAsIDEg b3IgMi4gTmVlZHMgYSBjdXN0b20KICBwcm9wZXJ0eSBhcyB3ZWxsCgpTbyB3aXRoIHRoZXNlIDQg cHJvcGVydGllcyB5b3UgY2FuIGRlc2NyaWJlIH45MCUgb2YgdGhlIGNsb2NrcyBmb3VuZCBvbiB0 aGUKWnlucS4KCkFuZCB0aGVuIHRoZXJlIGFyZSB0aGUgQ0FOIGFuZCBHRU0gY2xvY2tzLCB3aGlj aCBwcm9iYWJseSByZXF1aXJlIGN1c3RvbQpjbG9jayBpbXBsZW1lbnRhdGlvbiBzaW5jZSB0aGV5 IGFyZSBraW5kIG9mIHNwZWNpYWwuCgpZb3VyIGFwcHJvYWNoIGVuZHMgdXAgd2l0aCBodW5kcmVk cyBvZiBsaW5lcyBvZiBvcGVuLWNvZGluZyB0aGUKaW5zdGFudGlhdGlvbiBvZiBhbGwgdGhlIGNs b2NrIGdhdGVzIGFuZCBkaXZpZGVycyBmb3VuZCBpbiB0aGUgc2xjci4gVGhhdCdzCmp1c3QgbWFk bmVzcy4KCi0gTGFycwoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX18KZGV2aWNldHJlZS1kaXNjdXNzIG1haWxpbmcgbGlzdApkZXZpY2V0cmVlLWRpc2N1c3NA bGlzdHMub3psYWJzLm9yZwpodHRwczovL2xpc3RzLm96bGFicy5vcmcvbGlzdGluZm8vZGV2aWNl dHJlZS1kaXNjdXNzCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933088Ab3CHSGg (ORCPT ); Fri, 8 Mar 2013 13:06:36 -0500 Received: from smtp-out-071.synserver.de ([212.40.185.71]:1201 "EHLO smtp-out-069.synserver.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1759812Ab3CHSGd (ORCPT ); Fri, 8 Mar 2013 13:06:33 -0500 X-SynServer-TrustedSrc: 1 X-SynServer-AuthUser: lars@metafoo.de X-SynServer-PPID: 1525 Message-ID: <513A290A.3060502@metafoo.de> Date: Fri, 08 Mar 2013 19:08:10 +0100 From: Lars-Peter Clausen User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0.12) Gecko/20130116 Icedove/10.0.12 MIME-Version: 1.0 To: =?UTF-8?B?U8O2cmVuIEJyaW5rbWFubg==?= CC: =?UTF-8?B?SmFuIEzDvGJiZQ==?= , Sascha Hauer , Mike Turquette , Josh Cartwright , Michal Simek , Peter Crosthwaite , Prashant Gaikwad , devicetree-discuss@lists.ozlabs.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, git@xilinx.com Subject: Re: RFC: Zynq Clock Controller References: <37032699-343e-485c-80e0-9b23e3706c58@VA3EHSMHS013.ehs.local> <1362570681.5269.98.camel@coredoba.hi.pengutronix.de> <51385FA3.8080105@metafoo.de> <2a91857c-473f-4f92-b2b1-e7ac4679b72a@AM1EHSMHS007.ehs.local> <51390E92.2080806@metafoo.de> <51398F54.5080802@metafoo.de> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 03/08/2013 06:38 PM, Sören Brinkmann wrote: > On Fri, Mar 08, 2013 at 08:12:20AM +0100, Lars-Peter Clausen wrote: >> On 03/08/2013 12:25 AM, Sören Brinkmann wrote: >>> On Thu, Mar 07, 2013 at 11:02:58PM +0100, Lars-Peter Clausen wrote: >>>> On 03/07/2013 08:11 PM, Sören Brinkmann wrote: >>>>> On Thu, Mar 07, 2013 at 10:36:35AM +0100, Lars-Peter Clausen wrote: >>>>>> On 03/06/2013 06:27 PM, Sören Brinkmann wrote: >>>>>>> Hi Jan, >>>>>>> >>>>>>> what a small world. Good to hear from you. >>>>>>> >>>>>>> On Wed, Mar 06, 2013 at 12:51:21PM +0100, Jan Lübbe wrote: >>>>>>>> Hi Sören, >>>>>>>> >>>>>>>> On Tue, 2013-03-05 at 12:04 -0800, Sören Brinkmann wrote: >>>>>>>>> For this reasons, I'd like to propose moving Zynq into the same >>>>>>>>> direction. I.e. adding a clock controller with the following DT >>>>>>>>> description (details may change but the general idea should become >>>>>>>>> clear): >>>>>>>>> clkc: clkc { >>>>>>>>> #clock-cells = <1>; >>>>>>>>> compatible = "xlnx,ps7-clkc"; >>>>>>>>> ps_clk_frequency = <33333333>; # board x-tal >>>>>>>>> # optional props >>>>>>>>> gem0_emio_clk_freq = <125000000>; >>>>>>>>> gem1_emio_clk_freq = <50000000>; >>>>>>>>> can_mio_clk_freq_xx = <1234>; # this is possible 54 times with xx = 00..53 >>>>>>>>> }; >>>>>> >>>>>> I definitely prefer the way it is right now in upstream, where we have one >>>>>> dt node per clock. It is more descriptive and also more extensible. And you >>>>>> also don't have to remember the clock index, and can use the phandle >>>>>> directly instead. >>>>> The problems I see with that are: >>>>> - with the clock controller we can model the clock tree as we need without changing the DT all the time >>>> >>>> When do we need to change the clock tree? The clock tree is pretty much fixed >>>> in hardware. And the DT describes the hardware, so I don't so a problem there. >>>> >>>>> - w/o a clock controller there is no block which has knowledge of the whole clock tree (unless we parse the DT), and could conduct reparent operations and similar >>>> >>>> The clk framework builds a representation of the clock tree. Each clock should >>>> be able to handle re-parenting on it's own, without knowing about the other >>>> clocks in the tree, the parent is selected by a field in the clocks register. >>>> It doesn't even have to know who the parents are, the clk framework will take >>>> care of all of this and just say, ok, switch your input clock to X, where X is >>>> a simple integer number The clk framework will also take care of re-calculating >>>> all the updates frequencies after re-parenting. >>> Nope, clk_set_parent() takes two 'struct *clk' as argument. So, you have to have those of the clock to change and all its parents. A device driver for example, which gets a clock through clk_get() does not have that information and should not, since it should not have to be aware of the SOC's clock hierarchy, IMHO. >>> >> >> Yes, you global clk_set_parent() method takes two clk structs. But the clock >> framework takes care of all the magic of mapping that second clk struct to a >> integer number, based on the clks parent list. So your set_parent callback in >> your clk_ops takes as parameters your clk and the index of the parent. > Right, but who/what in your scheme is supposed to call the high level API function requiring knowledge about all those 'struct *clk' and their hierarchy? This is where I think a big block has the advantage since it has a holistic view on the clock tree. > How would you use the clock controller driver to do re-parent operations? I really can't see how it will help to implement re-parent operations. >> >>>> >>>>> - once the clock controller is properly defined the clock connection should be contained in a dtsi which never changes. So, regarding remembering outputs, I don't see a problem. I rather see issues with having a pile of clocks described in the DT. I have currently 44 outputs proposed, and to model the whole tree a lot of more clocks are used (I started modeling everything with the clock primitives and removed custom clock implementations, except for the PLLs). Having all those in the DT does not really help, IMHO. >>>> >>>> Well, except if you want to use external clocks for ethernet or CAN. Also you >>>> don't need to change the dtsi either if you have a separate node for each clock. >>>> >>>> To avoid misunderstandings let me clarify that I don't want to have one node >>>> per clock, but rather one node per clock block (or whatever they are called). >>>> The combination of input select + divider + output gate. E.g. the UART clock >>>> block with it's 3 inputs and two outputs. >>>> >>>> Also the APER clocks should probably be one node with 24 outputs. >>> Okay, so instead of having one block encapsulating all clocks, you want it on a finer granularity. I don't see huge differences why that should be advantageous? It would just mean to create several blocks with their custom DT bindings instead of a single one. Just the abstraction level would be a little different. >> >> The DT is supposed to describe the hardware, not the software. These are the >> basic blocks. The are independent of each other and mostly orthogonal. It's the >> same IP block instantiated a couple of times next to each other (sometimes with >> slightly different parameters). They just happen to have their register mapped >> in the same IO region. The SLCR is just a container which contains all these >> blocks. Very much the same way the whole SoC is a container which contains the >> different peripheral blocks, etc. By you rationale we could argue, why do we >> need a dt node for each peripheral and not just simply one ZYNQ node? > I see us ending up here with custom clock blocks for each and every peripheral. They all look similar on first look but have all their slight differences. So, it's either different implementations or having a lot of DT properties to make a single implementation work for all the variations. That's why I prefer the single controller solution. > The DT description will always be an abstraction. You prefer grouping clocks for functional units, whereas I put them all into the same drawer labeled 'clocks'. So how many parameters are there? - Number of input clocks: We get these by listing the parent clocks in the clocks property. - Number of output clocks: We get this through the output-clock-names property. - Whether the output clocks can be gated, this requires a custom property. - Number of 6bit dividers in the chain, can be 0, 1 or 2. Needs a custom property as well So with these 4 properties you can describe ~90% of the clocks found on the Zynq. And then there are the CAN and GEM clocks, which probably require custom clock implementation since they are kind of special. Your approach ends up with hundreds of lines of open-coding the instantiation of all the clock gates and dividers found in the slcr. That's just madness. - Lars