From mboxrd@z Thu Jan 1 00:00:00 1970 From: miquel.raynal@bootlin.com (Miquel Raynal) Date: Fri, 4 May 2018 10:32:20 +0200 Subject: [PATCH 09/17] irqchip/irq-mvebu-icu: support ICU subnodes In-Reply-To: <20180502101300.49cc3623@windsurf.home> References: <20180421135537.24716-1-miquel.raynal@bootlin.com> <20180421135537.24716-10-miquel.raynal@bootlin.com> <20180502101300.49cc3623@windsurf.home> Message-ID: <20180504103220.3504e73b@xps13> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Thomas, On Wed, 2 May 2018 10:13:00 +0200, Thomas Petazzoni wrote: > Hello Miqu?l, > > On Sat, 21 Apr 2018 15:55:29 +0200, Miquel Raynal wrote: > > Introduce new bindings for the ICU. > > Perhaps this should explain *why* we need new bindings. Sure, I changed the whole message by: The ICU can handle several type of interrupt, each of them being handled differently on AP side. On CP side, the ICU should be able to make the distinction between each interrupt group by pointing to the right parent. This is done through the introduction of new bindings, presenting the ICU node as the parent of multiple ICU sub-nodes, each of them being an interrupt type with a different interrupt parent. ICU interrupt 'clients' now directly point to the right sub-node, avoiding the need for the extra ICU_GRP_* parameter. ICU subnodes are probed automatically with devm_platform_populate(). If the node as no child, the probe function for NSRs will still be called 'manually' in order to preserve backward compatibility with DT using the old binding. > > > Each DT subnode of the ICU represents a type of interrupt that should > > be handled separately. Add the possibility for the ICU to have subnodes > > and probe each of them automatically with devm_platform_populate(). If > > the node as no child, the probe function for NSRs will still be called > > 'manually'. > > ... in order to preserve backward compatibility with Device Trees > using the old binding. Added, see above. > > > +static struct mvebu_icu *mvebu_dev_get_drvdata(struct platform_device *pdev) > > The function should be prefixed by mvebu_icu_, not just mvebu_. Changed. > > > +{ > > + struct mvebu_icu *icu; > > + > > + icu = dev_get_drvdata(&pdev->dev); > > + if (icu) { > > + /* Legacy bindings: get the device data */ > > I find this comment weird, because it doesn't document what the test > just below is doing. Refactored a bit the comments in this section. > > > + if (!icu->legacy_bindings) > > + return ERR_PTR(-EINVAL); > > + } else { > > + /* New bindings: get the parent device (ICU) data */ > > + icu = dev_get_drvdata(pdev->dev.parent); > > + if (!icu) > > + return ERR_PTR(-ENODEV); > > + if (icu->legacy_bindings) > > + return ERR_PTR(-EINVAL); > > + } > > > > @@ -144,7 +170,10 @@ mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, > > goto free_irqd; > > } > > > > - icu_irqd->icu_group = fwspec->param[0]; > > + if (icu->legacy_bindings) > > + icu_irqd->icu_group = fwspec->param[0]; > > + else > > + icu_irqd->icu_group = ICU_GRP_NSR; > > In practice here fwspec->param[0] is always going to be equal to > ICU_GRP_NSR, but OK, the test makes sense as in a future commit, the > "else" case will be changed to support SEIs. > > > +static const struct of_device_id mvebu_icu_nsr_of_match[] = { > > + { .compatible = "marvell,cp110-icu-nsr", }, > > + {}, > > +}; > > + > > +static struct platform_driver mvebu_icu_nsr_driver = { > > + .probe = mvebu_icu_nsr_probe, > > + .driver = { > > + .name = "mvebu-icu-nsr", > > + .of_match_table = mvebu_icu_nsr_of_match, > > + }, > > +}; > > +builtin_platform_driver(mvebu_icu_nsr_driver); > > I'm not sure why you call this icu_nsr here, and change it later to > icu_subset. Wouldn't it make sense to call it right away with the final > name ? Note that this is not a very strong request to change this > aspect, I'm fine with how it's done today, it's just that I would have > done it differently. I thought calling it icu_subset right now would not be very clear as there are not "ICU subset" other than NSR interrupts, but I changed it, this is not a big deal. > > Other than that, looks good to me. > > Thomas Thanks for the review, Miqu?l -- Miquel Raynal, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com From mboxrd@z Thu Jan 1 00:00:00 1970 From: Miquel Raynal Subject: Re: [PATCH 09/17] irqchip/irq-mvebu-icu: support ICU subnodes Date: Fri, 4 May 2018 10:32:20 +0200 Message-ID: <20180504103220.3504e73b@xps13> References: <20180421135537.24716-1-miquel.raynal@bootlin.com> <20180421135537.24716-10-miquel.raynal@bootlin.com> <20180502101300.49cc3623@windsurf.home> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20180502101300.49cc3623@windsurf.home> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Thomas Petazzoni Cc: Mark Rutland , Andrew Lunn , Jason Cooper , devicetree@vger.kernel.org, Marc Zyngier , Catalin Marinas , Gregory Clement , Haim Boot , Will Deacon , Maxime Chevallier , Nadav Haklai , Antoine Tenart , Rob Herring , Thomas Gleixner , Hanna Hawa , linux-arm-kernel@lists.infradead.org, Sebastian Hesselbarth List-Id: devicetree@vger.kernel.org SGkgVGhvbWFzLAoKT24gV2VkLCAyIE1heSAyMDE4IDEwOjEzOjAwICswMjAwLCBUaG9tYXMgUGV0 YXp6b25pCjx0aG9tYXMucGV0YXp6b25pQGJvb3RsaW4uY29tPiB3cm90ZToKCj4gSGVsbG8gTWlx dcOobCwKPiAKPiBPbiBTYXQsIDIxIEFwciAyMDE4IDE1OjU1OjI5ICswMjAwLCBNaXF1ZWwgUmF5 bmFsIHdyb3RlOgo+ID4gSW50cm9kdWNlIG5ldyBiaW5kaW5ncyBmb3IgdGhlIElDVS4gIAo+IAo+ IFBlcmhhcHMgdGhpcyBzaG91bGQgZXhwbGFpbiAqd2h5KiB3ZSBuZWVkIG5ldyBiaW5kaW5ncy4K ClN1cmUsIEkgY2hhbmdlZCB0aGUgd2hvbGUgbWVzc2FnZSBieToKCiAgICBUaGUgSUNVIGNhbiBo YW5kbGUgc2V2ZXJhbCB0eXBlIG9mIGludGVycnVwdCwgZWFjaCBvZiB0aGVtIGJlaW5nCiAgICBo YW5kbGVkIGRpZmZlcmVudGx5IG9uIEFQIHNpZGUuIE9uIENQIHNpZGUsIHRoZSBJQ1Ugc2hvdWxk IGJlIGFibGUKICAgIHRvIG1ha2UgdGhlIGRpc3RpbmN0aW9uIGJldHdlZW4gZWFjaCBpbnRlcnJ1 cHQgZ3JvdXAgYnkgcG9pbnRpbmcgdG8KICAgIHRoZSByaWdodCBwYXJlbnQuCiAgICAKICAgIFRo aXMgaXMgZG9uZSB0aHJvdWdoIHRoZSBpbnRyb2R1Y3Rpb24gb2YgbmV3IGJpbmRpbmdzLCBwcmVz ZW50aW5nCiAgICB0aGUgSUNVIG5vZGUgYXMgdGhlIHBhcmVudCBvZiBtdWx0aXBsZSBJQ1Ugc3Vi LW5vZGVzLCBlYWNoIG9mIHRoZW0KICAgIGJlaW5nIGFuIGludGVycnVwdCB0eXBlIHdpdGggYSBk aWZmZXJlbnQgaW50ZXJydXB0IHBhcmVudC4gSUNVCiAgICBpbnRlcnJ1cHQgJ2NsaWVudHMnIG5v dyBkaXJlY3RseSBwb2ludCB0byB0aGUgcmlnaHQgc3ViLW5vZGUsCiAgICBhdm9pZGluZyB0aGUg bmVlZCBmb3IgdGhlIGV4dHJhIElDVV9HUlBfKiBwYXJhbWV0ZXIuCiAgICAKICAgIElDVSBzdWJu b2RlcyBhcmUgcHJvYmVkIGF1dG9tYXRpY2FsbHkgd2l0aAogICAgZGV2bV9wbGF0Zm9ybV9wb3B1 bGF0ZSgpLiBJZiB0aGUgbm9kZSBhcyBubyBjaGlsZCwgdGhlIHByb2JlCiAgICBmdW5jdGlvbiBm b3IgTlNScyB3aWxsIHN0aWxsIGJlIGNhbGxlZCAnbWFudWFsbHknIGluIG9yZGVyIHRvCiAgICBw cmVzZXJ2ZSBiYWNrd2FyZCBjb21wYXRpYmlsaXR5IHdpdGggRFQgdXNpbmcgdGhlIG9sZCBiaW5k aW5nLgogICAgCj4gCj4gPiBFYWNoIERUIHN1Ym5vZGUgb2YgdGhlIElDVSByZXByZXNlbnRzIGEg dHlwZSBvZiBpbnRlcnJ1cHQgdGhhdCBzaG91bGQKPiA+IGJlIGhhbmRsZWQgc2VwYXJhdGVseS4g QWRkIHRoZSBwb3NzaWJpbGl0eSBmb3IgdGhlIElDVSB0byBoYXZlIHN1Ym5vZGVzCj4gPiBhbmQg cHJvYmUgZWFjaCBvZiB0aGVtIGF1dG9tYXRpY2FsbHkgd2l0aCBkZXZtX3BsYXRmb3JtX3BvcHVs YXRlKCkuIElmCj4gPiB0aGUgbm9kZSBhcyBubyBjaGlsZCwgdGhlIHByb2JlIGZ1bmN0aW9uIGZv ciBOU1JzIHdpbGwgc3RpbGwgYmUgY2FsbGVkCj4gPiAnbWFudWFsbHknLiAgCj4gCj4gIC4uLiBp biBvcmRlciB0byBwcmVzZXJ2ZSBiYWNrd2FyZCBjb21wYXRpYmlsaXR5IHdpdGggRGV2aWNlIFRy ZWVzCj4gIHVzaW5nIHRoZSBvbGQgYmluZGluZy4KCkFkZGVkLCBzZWUgYWJvdmUuCgo+IAo+ID4g K3N0YXRpYyBzdHJ1Y3QgbXZlYnVfaWN1ICptdmVidV9kZXZfZ2V0X2RydmRhdGEoc3RydWN0IHBs YXRmb3JtX2RldmljZSAqcGRldikgIAo+IAo+IFRoZSBmdW5jdGlvbiBzaG91bGQgYmUgcHJlZml4 ZWQgYnkgbXZlYnVfaWN1Xywgbm90IGp1c3QgbXZlYnVfLgoKQ2hhbmdlZC4KCj4gCj4gPiArewo+ ID4gKwlzdHJ1Y3QgbXZlYnVfaWN1ICppY3U7Cj4gPiArCj4gPiArCWljdSA9IGRldl9nZXRfZHJ2 ZGF0YSgmcGRldi0+ZGV2KTsKPiA+ICsJaWYgKGljdSkgewo+ID4gKwkJLyogTGVnYWN5IGJpbmRp bmdzOiBnZXQgdGhlIGRldmljZSBkYXRhICovICAKPiAKPiBJIGZpbmQgdGhpcyBjb21tZW50IHdl aXJkLCBiZWNhdXNlIGl0IGRvZXNuJ3QgZG9jdW1lbnQgd2hhdCB0aGUgdGVzdAo+IGp1c3QgYmVs b3cgaXMgZG9pbmcuCgpSZWZhY3RvcmVkIGEgYml0IHRoZSBjb21tZW50cyBpbiB0aGlzIHNlY3Rp b24uCgo+IAo+ID4gKwkJaWYgKCFpY3UtPmxlZ2FjeV9iaW5kaW5ncykKPiA+ICsJCQlyZXR1cm4g RVJSX1BUUigtRUlOVkFMKTsKPiA+ICsJfSBlbHNlIHsKPiA+ICsJCS8qIE5ldyBiaW5kaW5nczog Z2V0IHRoZSBwYXJlbnQgZGV2aWNlIChJQ1UpIGRhdGEgKi8KPiA+ICsJCWljdSA9IGRldl9nZXRf ZHJ2ZGF0YShwZGV2LT5kZXYucGFyZW50KTsKPiA+ICsJCWlmICghaWN1KQo+ID4gKwkJCXJldHVy biBFUlJfUFRSKC1FTk9ERVYpOwo+ID4gKwkJaWYgKGljdS0+bGVnYWN5X2JpbmRpbmdzKQo+ID4g KwkJCXJldHVybiBFUlJfUFRSKC1FSU5WQUwpOwo+ID4gKwl9ICAKPiAKPiAKPiA+IEBAIC0xNDQs NyArMTcwLDEwIEBAIG12ZWJ1X2ljdV9pcnFfZG9tYWluX2FsbG9jKHN0cnVjdCBpcnFfZG9tYWlu ICpkb21haW4sIHVuc2lnbmVkIGludCB2aXJxLAo+ID4gIAkJZ290byBmcmVlX2lycWQ7Cj4gPiAg CX0KPiA+ICAKPiA+IC0JaWN1X2lycWQtPmljdV9ncm91cCA9IGZ3c3BlYy0+cGFyYW1bMF07Cj4g PiArCWlmIChpY3UtPmxlZ2FjeV9iaW5kaW5ncykKPiA+ICsJCWljdV9pcnFkLT5pY3VfZ3JvdXAg PSBmd3NwZWMtPnBhcmFtWzBdOwo+ID4gKwllbHNlCj4gPiArCQlpY3VfaXJxZC0+aWN1X2dyb3Vw ID0gSUNVX0dSUF9OU1I7ICAKPiAKPiBJbiBwcmFjdGljZSBoZXJlIGZ3c3BlYy0+cGFyYW1bMF0g aXMgYWx3YXlzIGdvaW5nIHRvIGJlIGVxdWFsIHRvCj4gSUNVX0dSUF9OU1IsIGJ1dCBPSywgdGhl IHRlc3QgbWFrZXMgc2Vuc2UgYXMgaW4gYSBmdXR1cmUgY29tbWl0LCB0aGUKPiAiZWxzZSIgY2Fz ZSB3aWxsIGJlIGNoYW5nZWQgdG8gc3VwcG9ydCBTRUlzLgo+IAo+ID4gK3N0YXRpYyBjb25zdCBz dHJ1Y3Qgb2ZfZGV2aWNlX2lkIG12ZWJ1X2ljdV9uc3Jfb2ZfbWF0Y2hbXSA9IHsKPiA+ICsJeyAu Y29tcGF0aWJsZSA9ICJtYXJ2ZWxsLGNwMTEwLWljdS1uc3IiLCB9LAo+ID4gKwl7fSwKPiA+ICt9 Owo+ID4gKwo+ID4gK3N0YXRpYyBzdHJ1Y3QgcGxhdGZvcm1fZHJpdmVyIG12ZWJ1X2ljdV9uc3Jf ZHJpdmVyID0gewo+ID4gKwkucHJvYmUgID0gbXZlYnVfaWN1X25zcl9wcm9iZSwKPiA+ICsJLmRy aXZlciA9IHsKPiA+ICsJCS5uYW1lID0gIm12ZWJ1LWljdS1uc3IiLAo+ID4gKwkJLm9mX21hdGNo X3RhYmxlID0gbXZlYnVfaWN1X25zcl9vZl9tYXRjaCwKPiA+ICsJfSwKPiA+ICt9Owo+ID4gK2J1 aWx0aW5fcGxhdGZvcm1fZHJpdmVyKG12ZWJ1X2ljdV9uc3JfZHJpdmVyKTsgIAo+IAo+IEknbSBu b3Qgc3VyZSB3aHkgeW91IGNhbGwgdGhpcyBpY3VfbnNyIGhlcmUsIGFuZCBjaGFuZ2UgaXQgbGF0 ZXIgdG8KPiBpY3Vfc3Vic2V0LiBXb3VsZG4ndCBpdCBtYWtlIHNlbnNlIHRvIGNhbGwgaXQgcmln aHQgYXdheSB3aXRoIHRoZSBmaW5hbAo+IG5hbWUgPyBOb3RlIHRoYXQgdGhpcyBpcyBub3QgYSB2 ZXJ5IHN0cm9uZyByZXF1ZXN0IHRvIGNoYW5nZSB0aGlzCj4gYXNwZWN0LCBJJ20gZmluZSB3aXRo IGhvdyBpdCdzIGRvbmUgdG9kYXksIGl0J3MganVzdCB0aGF0IEkgd291bGQgaGF2ZQo+IGRvbmUg aXQgZGlmZmVyZW50bHkuCgpJIHRob3VnaHQgY2FsbGluZyBpdCBpY3Vfc3Vic2V0IHJpZ2h0IG5v dyB3b3VsZCBub3QgYmUgdmVyeSBjbGVhciBhcwp0aGVyZSBhcmUgbm90ICJJQ1Ugc3Vic2V0IiBv dGhlciB0aGFuIE5TUiBpbnRlcnJ1cHRzLCBidXQgSSBjaGFuZ2VkIGl0LAp0aGlzIGlzIG5vdCBh IGJpZyBkZWFsLgoKPiAKPiBPdGhlciB0aGFuIHRoYXQsIGxvb2tzIGdvb2QgdG8gbWUuCj4gCj4g VGhvbWFzCgpUaGFua3MgZm9yIHRoZSByZXZpZXcsCk1pcXXDqGwKCgotLSAKTWlxdWVsIFJheW5h bCwgQm9vdGxpbiAoZm9ybWVybHkgRnJlZSBFbGVjdHJvbnMpCkVtYmVkZGVkIExpbnV4IGFuZCBL ZXJuZWwgZW5naW5lZXJpbmcKaHR0cHM6Ly9ib290bGluLmNvbQoKX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX18KbGludXgtYXJtLWtlcm5lbCBtYWlsaW5nIGxp c3QKbGludXgtYXJtLWtlcm5lbEBsaXN0cy5pbmZyYWRlYWQub3JnCmh0dHA6Ly9saXN0cy5pbmZy YWRlYWQub3JnL21haWxtYW4vbGlzdGluZm8vbGludXgtYXJtLWtlcm5lbAo=