From mboxrd@z Thu Jan 1 00:00:00 1970 From: ezequiel.garcia@free-electrons.com (Ezequiel Garcia) Date: Sat, 8 Jun 2013 15:38:52 -0300 Subject: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding In-Reply-To: <20130607200054.GA9010@obsidianresearch.com> References: <1370623671-7748-1-git-send-email-ezequiel.garcia@free-electrons.com> <1370623671-7748-5-git-send-email-ezequiel.garcia@free-electrons.com> <201306072101.44694.arnd@arndb.de> <20130607200054.GA9010@obsidianresearch.com> Message-ID: <20130608183851.GB2354@localhost> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Jason, Arnd: Thanks for your reviews! I agree with most of your suggestions so far. However, I'd like to discuss one point before we move forward with the other (imo, less importants) issues. See below. On Fri, Jun 07, 2013 at 02:00:54PM -0600, Jason Gunthorpe wrote: [...] > > > Is the ranges right though? I was expecting this: > > ranges = <0 0x012f0000 0 0x8000000> > > The 2nd address cell in the 2dword space should almost always be 0. > > The 2nd address cell should be interprited as the offset within the > target's window, not as some kind of physical base address. > > >>+ (Note that the windowid cell is encoding the target ID = 0x01 and attribute > >>+ ID = 0x2f, and the selected base address for the window is 0xe8000000). > > ... The proper place to indicate the base address for the window is in > the mbus ranges: > > mbus { > ranges = <0x012f0000 0 0xe8000000 0x8000000> > devbus-bootcs { > ranges = <0 0x012f0000 0 0x8000000> > } > } > > We shouldn't mangle the DT format just to make it convenient for > humans to write - if this is a major problem then I'd try to use the > preprocessor first.. There are several reasonable solutions down that > path, IMHO. > Right. I think we have two options here for laying the DT ranges. 1) This is the proposal implied in the patchset I sent: mbus { ranges = < we only put the internal-reg translation here> devbus-bootcs { ranges = <0 {target_id/attribute} {window_physical_base} {size}> } } Of course the above DT will be actually incomplete, for it'll lack a proper ranges entry to translate the devbus-bootcs address. So we chosed to do it dynamically in the mbus driver (see patch 05/14), and add the missing entry. The information of the physical window base address is in this case in each child (devbus-bootcs, bootrom, and so on). The MBus driver walks each of its first-level children and allocates the window based on the address declared in the ranges property of each child, as shown above. This is done mostly to avoid having that in the mbus node, and the nightmare to maintain it produces. See below. 2) This is what Jason is proposing in his mail: mbus { ranges = <{target_id/attribute} 0 {window_physical_base} {size}> devbus-bootcs { ranges = <0 {target_id/attribute} 0 {size}> } } Of course this looks much cleaner, but it forces a lot of duplication in the DT files. Now, if you see some of the recent patches we've been sending, I think this duplication is very error-prone, and it'll be a nightmare to maintain. Let me propose an example to show this duplication: Let's suppose we have a board "A" with its armada-A.dts, and a common one armada.dtsi. The common dtsi file would have this ranges property: /* armada.dtsi */ mbus { ranges = < internal_regs_id 0 internal_regs_base internal_regs_size bootrom_id 0 bootrom_base bootrom_size > } The A board has a NOR connected to some devbus, so we need to add it to the ranges, but also need to duplicate the ones in the common dtsi: /* armada-A.dts */ mbus { ranges = < internal_regs_id 0 internal_regs_base internal_regs_size bootrom_id 0 bootrom_base bootrom_size devbus0_id 0 devbus0_base devbus0_size > } Now, if we add something at the common level, and extend the ranges property in the common armada.dtsi, we also have to go through *each* of the per-board dts files (for *each* board) adding that entry, because entries *need* to be duplicated. Otherwise you're effectively "shadowing" the entries. It is precisely for this reason that I've decided to adopt option #1 instead! Now, I'm not saying I like that option particularly. In fact it has a couple issues as well: 1. The DT is *incomplete* and needs to be completed by the MBus driver which, IMHO, sucks. 2. Changing the DT dynamically in the kernel, means that new properties are allocated to replace old ones, but the old ones are *never* released. So if for any reason we do this often, we're effectively "leaking" memory. So, I'm not saying option #1 is nice, but rather that it seems it'll be less trouble to maintain. What do you think? -- Ezequiel Garc?a, Free Electrons Embedded Linux, Kernel and Android Engineering http://free-electrons.com From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ezequiel Garcia Subject: Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding Date: Sat, 8 Jun 2013 15:38:52 -0300 Message-ID: <20130608183851.GB2354@localhost> References: <1370623671-7748-1-git-send-email-ezequiel.garcia@free-electrons.com> <1370623671-7748-5-git-send-email-ezequiel.garcia@free-electrons.com> <201306072101.44694.arnd@arndb.de> <20130607200054.GA9010@obsidianresearch.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: <20130607200054.GA9010-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> 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: Jason Gunthorpe Cc: Lior Amsalem , Jason Cooper , Andrew Lunn , devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org, Maen Suleiman , linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, Sebastian Hesselbarth List-Id: devicetree@vger.kernel.org SGkgSmFzb24sIEFybmQ6CgpUaGFua3MgZm9yIHlvdXIgcmV2aWV3cyEKCkkgYWdyZWUgd2l0aCBt b3N0IG9mIHlvdXIgc3VnZ2VzdGlvbnMgc28gZmFyLiBIb3dldmVyLCBJJ2QgbGlrZSB0byBkaXNj dXNzCm9uZSBwb2ludCBiZWZvcmUgd2UgbW92ZSBmb3J3YXJkIHdpdGggdGhlIG90aGVyIChpbW8s IGxlc3MgaW1wb3J0YW50cykKaXNzdWVzLiBTZWUgYmVsb3cuCgpPbiBGcmksIEp1biAwNywgMjAx MyBhdCAwMjowMDo1NFBNIC0wNjAwLCBKYXNvbiBHdW50aG9ycGUgd3JvdGU6ClsuLi5dCj4gCj4g Cj4gSXMgdGhlIHJhbmdlcyByaWdodCB0aG91Z2g/IEkgd2FzIGV4cGVjdGluZyB0aGlzOgo+IAo+ ICByYW5nZXMgPSA8MCAgMHgwMTJmMDAwMCAwICAweDgwMDAwMDA+Cj4gCj4gVGhlIDJuZCBhZGRy ZXNzIGNlbGwgaW4gdGhlIDJkd29yZCBzcGFjZSBzaG91bGQgYWxtb3N0IGFsd2F5cyBiZSAwLgo+ IAo+IFRoZSAybmQgYWRkcmVzcyBjZWxsIHNob3VsZCBiZSBpbnRlcnByaXRlZCBhcyB0aGUgb2Zm c2V0IHdpdGhpbiB0aGUKPiB0YXJnZXQncyB3aW5kb3csIG5vdCBhcyBzb21lIGtpbmQgb2YgcGh5 c2ljYWwgYmFzZSBhZGRyZXNzLgo+IAo+ID4+KyAoTm90ZSB0aGF0IHRoZSB3aW5kb3dpZCBjZWxs IGlzIGVuY29kaW5nIHRoZSB0YXJnZXQgSUQgPSAweDAxIGFuZCBhdHRyaWJ1dGUKPiA+PisgSUQg PSAweDJmLCBhbmQgdGhlIHNlbGVjdGVkIGJhc2UgYWRkcmVzcyBmb3IgdGhlIHdpbmRvdyBpcyAw eGU4MDAwMDAwKS4KPiAKPiAuLi4gVGhlIHByb3BlciBwbGFjZSB0byBpbmRpY2F0ZSB0aGUgYmFz ZSBhZGRyZXNzIGZvciB0aGUgd2luZG93IGlzIGluCj4gdGhlIG1idXMgcmFuZ2VzOgo+IAo+IG1i dXMgewo+ICAgIHJhbmdlcyA9IDwweDAxMmYwMDAwIDAgIDB4ZTgwMDAwMDAgIDB4ODAwMDAwMD4K PiAgICBkZXZidXMtYm9vdGNzIHsKPiAgICAgICAgcmFuZ2VzID0gPDAgIDB4MDEyZjAwMDAgMCAg MHg4MDAwMDAwPgo+ICAgIH0KPiB9Cj4gCj4gV2Ugc2hvdWxkbid0IG1hbmdsZSB0aGUgRFQgZm9y bWF0IGp1c3QgdG8gbWFrZSBpdCBjb252ZW5pZW50IGZvcgo+IGh1bWFucyB0byB3cml0ZSAtIGlm IHRoaXMgaXMgYSBtYWpvciBwcm9ibGVtIHRoZW4gSSdkIHRyeSB0byB1c2UgdGhlCj4gcHJlcHJv Y2Vzc29yIGZpcnN0Li4gVGhlcmUgYXJlIHNldmVyYWwgcmVhc29uYWJsZSBzb2x1dGlvbnMgZG93 biB0aGF0Cj4gcGF0aCwgSU1ITy4KPiAKClJpZ2h0LiBJIHRoaW5rIHdlIGhhdmUgdHdvIG9wdGlv bnMgaGVyZSBmb3IgbGF5aW5nIHRoZSBEVCByYW5nZXMuCgoxKSBUaGlzIGlzIHRoZSBwcm9wb3Nh bCBpbXBsaWVkIGluIHRoZSBwYXRjaHNldCBJIHNlbnQ6CgptYnVzIHsKCXJhbmdlcyA9IDwgd2Ug b25seSBwdXQgdGhlIGludGVybmFsLXJlZyB0cmFuc2xhdGlvbiBoZXJlPgoJZGV2YnVzLWJvb3Rj cyB7CgkJcmFuZ2VzID0gPDAge3RhcmdldF9pZC9hdHRyaWJ1dGV9IHt3aW5kb3dfcGh5c2ljYWxf YmFzZX0ge3NpemV9PgoJfQp9CgpPZiBjb3Vyc2UgdGhlIGFib3ZlIERUIHdpbGwgYmUgYWN0dWFs bHkgaW5jb21wbGV0ZSwgZm9yIGl0J2xsIGxhY2sgYSBwcm9wZXIgcmFuZ2VzCmVudHJ5IHRvIHRy YW5zbGF0ZSB0aGUgZGV2YnVzLWJvb3RjcyBhZGRyZXNzLiBTbyB3ZSBjaG9zZWQgdG8gZG8gaXQg ZHluYW1pY2FsbHkKaW4gdGhlIG1idXMgZHJpdmVyIChzZWUgcGF0Y2ggMDUvMTQpLCBhbmQgYWRk IHRoZSBtaXNzaW5nIGVudHJ5LgoKVGhlIGluZm9ybWF0aW9uIG9mIHRoZSBwaHlzaWNhbCB3aW5k b3cgYmFzZSBhZGRyZXNzIGlzIGluIHRoaXMgY2FzZSBpbgplYWNoIGNoaWxkIChkZXZidXMtYm9v dGNzLCBib290cm9tLCBhbmQgc28gb24pLiBUaGUgTUJ1cyBkcml2ZXIgd2Fsa3MKZWFjaCBvZiBp dHMgZmlyc3QtbGV2ZWwgY2hpbGRyZW4gYW5kIGFsbG9jYXRlcyB0aGUgd2luZG93IGJhc2VkIG9u IHRoZQphZGRyZXNzIGRlY2xhcmVkIGluIHRoZSByYW5nZXMgcHJvcGVydHkgb2YgZWFjaCBjaGls ZCwgYXMgc2hvd24gYWJvdmUuCgpUaGlzIGlzIGRvbmUgbW9zdGx5IHRvIGF2b2lkIGhhdmluZyB0 aGF0IGluIHRoZSBtYnVzIG5vZGUsIGFuZCB0aGUgbmlnaHRtYXJlCnRvIG1haW50YWluIGl0IHBy b2R1Y2VzLiBTZWUgYmVsb3cuCgoyKSBUaGlzIGlzIHdoYXQgSmFzb24gaXMgcHJvcG9zaW5nIGlu IGhpcyBtYWlsOgoKbWJ1cyB7CglyYW5nZXMgPSA8e3RhcmdldF9pZC9hdHRyaWJ1dGV9IDAge3dp bmRvd19waHlzaWNhbF9iYXNlfSB7c2l6ZX0+CglkZXZidXMtYm9vdGNzIHsKCQlyYW5nZXMgPSA8 MCB7dGFyZ2V0X2lkL2F0dHJpYnV0ZX0gMCB7c2l6ZX0+Cgl9Cn0KCk9mIGNvdXJzZSB0aGlzIGxv b2tzIG11Y2ggY2xlYW5lciwgYnV0IGl0IGZvcmNlcyBhIGxvdCBvZiBkdXBsaWNhdGlvbgppbiB0 aGUgRFQgZmlsZXMuIE5vdywgaWYgeW91IHNlZSBzb21lIG9mIHRoZSByZWNlbnQgcGF0Y2hlcyB3 ZSd2ZSBiZWVuCnNlbmRpbmcsIEkgdGhpbmsgdGhpcyBkdXBsaWNhdGlvbiBpcyB2ZXJ5IGVycm9y LXByb25lLCBhbmQgaXQnbGwgYmUgYQpuaWdodG1hcmUgdG8gbWFpbnRhaW4uIExldCBtZSBwcm9w b3NlIGFuIGV4YW1wbGUgdG8gc2hvdyB0aGlzCmR1cGxpY2F0aW9uOgoKTGV0J3Mgc3VwcG9zZSB3 ZSBoYXZlIGEgYm9hcmQgIkEiIHdpdGggaXRzIGFybWFkYS1BLmR0cywKYW5kIGEgY29tbW9uIG9u ZSBhcm1hZGEuZHRzaS4KClRoZSBjb21tb24gZHRzaSBmaWxlIHdvdWxkIGhhdmUgdGhpcyByYW5n ZXMgcHJvcGVydHk6CgovKiBhcm1hZGEuZHRzaSAqLwptYnVzIHsKCXJhbmdlcyA9IDwgaW50ZXJu YWxfcmVnc19pZCAwIGludGVybmFsX3JlZ3NfYmFzZSBpbnRlcm5hbF9yZWdzX3NpemUKCQkgICAg ICAgICBib290cm9tX2lkIDAgICAgICAgYm9vdHJvbV9iYXNlICAgICAgIGJvb3Ryb21fc2l6ZSA+ Cn0KClRoZSBBIGJvYXJkIGhhcyBhIE5PUiBjb25uZWN0ZWQgdG8gc29tZSBkZXZidXMsIHNvIHdl IG5lZWQgdG8gYWRkIGl0CnRvIHRoZSByYW5nZXMsIGJ1dCBhbHNvIG5lZWQgdG8gZHVwbGljYXRl IHRoZSBvbmVzIGluIHRoZSBjb21tb24gZHRzaToKCi8qIGFybWFkYS1BLmR0cyAqLwptYnVzIHsK CXJhbmdlcyA9IDwgaW50ZXJuYWxfcmVnc19pZCAwIGludGVybmFsX3JlZ3NfYmFzZSBpbnRlcm5h bF9yZWdzX3NpemUKCQkgICAgICAgICBib290cm9tX2lkIDAgICAgICAgYm9vdHJvbV9iYXNlICAg ICAgIGJvb3Ryb21fc2l6ZQoJCSAgICAgICAgIGRldmJ1czBfaWQgMCAgICAgICBkZXZidXMwX2Jh c2UgICAgICAgZGV2YnVzMF9zaXplID4KfQoKTm93LCBpZiB3ZSBhZGQgc29tZXRoaW5nIGF0IHRo ZSBjb21tb24gbGV2ZWwsIGFuZCBleHRlbmQgdGhlIHJhbmdlcwpwcm9wZXJ0eSBpbiB0aGUgY29t bW9uIGFybWFkYS5kdHNpLCB3ZSBhbHNvIGhhdmUgdG8gZ28gdGhyb3VnaCAqZWFjaCogb2YKdGhl IHBlci1ib2FyZCBkdHMgZmlsZXMgKGZvciAqZWFjaCogYm9hcmQpIGFkZGluZyB0aGF0IGVudHJ5 LCBiZWNhdXNlCmVudHJpZXMgKm5lZWQqIHRvIGJlIGR1cGxpY2F0ZWQuIE90aGVyd2lzZSB5b3Un cmUgZWZmZWN0aXZlbHkKInNoYWRvd2luZyIgdGhlIGVudHJpZXMuCgpJdCBpcyBwcmVjaXNlbHkg Zm9yIHRoaXMgcmVhc29uIHRoYXQgSSd2ZSBkZWNpZGVkIHRvIGFkb3B0IG9wdGlvbiAjMQppbnN0 ZWFkISBOb3csIEknbSBub3Qgc2F5aW5nIEkgbGlrZSB0aGF0IG9wdGlvbiBwYXJ0aWN1bGFybHku CkluIGZhY3QgaXQgaGFzIGEgY291cGxlIGlzc3VlcyBhcyB3ZWxsOgoKICAxLiBUaGUgRFQgaXMg KmluY29tcGxldGUqIGFuZCBuZWVkcyB0byBiZSBjb21wbGV0ZWQgYnkgdGhlIE1CdXMKICAgICBk cml2ZXIgd2hpY2gsIElNSE8sIHN1Y2tzLgoKICAyLiBDaGFuZ2luZyB0aGUgRFQgZHluYW1pY2Fs bHkgaW4gdGhlIGtlcm5lbCwgbWVhbnMgdGhhdCBuZXcKICAgICBwcm9wZXJ0aWVzIGFyZSBhbGxv Y2F0ZWQgdG8gcmVwbGFjZSBvbGQgb25lcywgYnV0IHRoZSBvbGQgb25lcwogICAgIGFyZSAqbmV2 ZXIqIHJlbGVhc2VkLiBTbyBpZiBmb3IgYW55IHJlYXNvbiB3ZSBkbyB0aGlzIG9mdGVuLAogICAg IHdlJ3JlIGVmZmVjdGl2ZWx5ICJsZWFraW5nIiBtZW1vcnkuCgpTbywgSSdtIG5vdCBzYXlpbmcg b3B0aW9uICMxIGlzIG5pY2UsIGJ1dCByYXRoZXIgdGhhdCBpdCBzZWVtcyBpdCdsbApiZSBsZXNz IHRyb3VibGUgdG8gbWFpbnRhaW4uCgpXaGF0IGRvIHlvdSB0aGluaz8KLS0gCkV6ZXF1aWVsIEdh cmPDrWEsIEZyZWUgRWxlY3Ryb25zCkVtYmVkZGVkIExpbnV4LCBLZXJuZWwgYW5kIEFuZHJvaWQg RW5naW5lZXJpbmcKaHR0cDovL2ZyZWUtZWxlY3Ryb25zLmNvbQpfX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fXwpkZXZpY2V0cmVlLWRpc2N1c3MgbWFpbGluZyBs aXN0CmRldmljZXRyZWUtZGlzY3Vzc0BsaXN0cy5vemxhYnMub3JnCmh0dHBzOi8vbGlzdHMub3ps YWJzLm9yZy9saXN0aW5mby9kZXZpY2V0cmVlLWRpc2N1c3MK