From mboxrd@z Thu Jan 1 00:00:00 1970 From: miquel.raynal@bootlin.com (Miquel Raynal) Date: Thu, 23 Aug 2018 13:44:10 +0200 Subject: [PATCH v4 09/14] irqchip/irq-mvebu-icu: add support for System Error Interrupts (SEI) In-Reply-To: <9c3168e2-3653-5359-7ddb-9e6ac1b47bb8@arm.com> References: <20180705124011.7661-1-miquel.raynal@bootlin.com> <20180705124011.7661-10-miquel.raynal@bootlin.com> <20180821110825.2fbec982@xps13> <07e90507-1d51-6435-b006-c46f865a636c@arm.com> <20180821122819.726c434b@xps13> <9c3168e2-3653-5359-7ddb-9e6ac1b47bb8@arm.com> Message-ID: <20180823134410.2df84fa4@xps13> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Marc, Marc Zyngier wrote on Tue, 21 Aug 2018 11:37:47 +0100: > On 21/08/18 11:28, Miquel Raynal wrote: > > Hi Marc, > > > > Marc Zyngier wrote on Tue, 21 Aug 2018 10:19:04 > > +0100: > > > >> On 21/08/18 10:08, Miquel Raynal wrote: > >>> Hi Marc, > >>> > >>> I'm fine with the rest of the comments, please find just one last > >>> question below. > >>> > >>> [...] > >>> > >>>>> @@ -133,12 +164,36 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, > >>>>> return -EINVAL; > >>>>> } > >>>>> > >>>>> - /* Mask the type to prevent wrong DT configuration */ > >>>>> - *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; > >>>>> + /* > >>>>> + * The ICU receives level-interrupts. MSI SEI are > >>>>> + * edge-interrupts while MSI NSR are level-interrupts. Update the type > >>>>> + * accordingly for the parent irqchip. > >>>>> + */ > >>>>> + if (msi_data->subset_data->icu_group == ICU_GRP_SEI) > >>>>> + *type = IRQ_TYPE_EDGE_RISING; > >>>> > >>>> That's interesting. How is the resampling done here? > >>> > >>> I'm not sure to understand the question. What does 'resampling' means > >>> in such context? MSI SEIs are of type "edge" and use the traditional > >>> MSI signalling infrastructure. I'm asking to be sure not to ignore > >>> something wrong in my code. > >> > >> You seems to be turning a level interrupt into an edge. > > > > If is an SEI interrupt, it cannot be a level interrupt and the type > > will be IRQ_TYPE_EDGE_RISING. > > > >> You can do that, > >> but only if you resample the level on EOI (and regenerate the > >> corresponding edge if the level is still high). > > > > What is before is a '& IRQ_TYPE_SENSE_MASK' operation. In theory, > > *type could be anything of IRQ_TYPE_{EDGE,LEVEL}_* at this moment. But, > > as stated above, it cannot be anything else than IRQ_TYPE_EDGE_RISING. > > I thought more clear to enforce it but if this unclear and useless, > > let's drop it? > > I think overriding the trigger that way is very confusing. Either it > comes from DT, or it comes from the MSI framwork. In both cases, it has > to be correct, and overriding it is just papering over other bugs. > > I'd rather you put a WARN_ON if you encounter an illegal interrupt trigger. Actually I do remember now why I enforced the type: Let's say my thermal IP in a CP110 triggers an interrupt. This interrupt is of type LEVEL_HIGH, and it is declared in the DT as: thermal: thermal-sensor at ... { [...] interrupts-extended = <&icu_sei 116 IRQ_TYPE_LEVEL_HIGH>; }; The ICU callback ->translate() will be called with fwspec being the C view of the above "interrupts-extended" property, so the interrupt type will be LEVEL_HIGH. However, the "icu_sei" parent is the SEI IP in the AP806 and this IP only receives edge interrupts. What happens is that the SEI's ->set_type() callback is called with LEVEL_HIGH type (while it only supports EDGE_RISING interrupts) and I want the driver to throw an error in this case. My way of handling this was to force *type to be EDGE_RISING in the ICU ->translate() callback whenever an SEI was triggered. As this seems not to be correct, how would you handle such situation? Thanks, Miqu?l From mboxrd@z Thu Jan 1 00:00:00 1970 From: Miquel Raynal Subject: Re: [PATCH v4 09/14] irqchip/irq-mvebu-icu: add support for System Error Interrupts (SEI) Date: Thu, 23 Aug 2018 13:44:10 +0200 Message-ID: <20180823134410.2df84fa4@xps13> References: <20180705124011.7661-1-miquel.raynal@bootlin.com> <20180705124011.7661-10-miquel.raynal@bootlin.com> <20180821110825.2fbec982@xps13> <07e90507-1d51-6435-b006-c46f865a636c@arm.com> <20180821122819.726c434b@xps13> <9c3168e2-3653-5359-7ddb-9e6ac1b47bb8@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <9c3168e2-3653-5359-7ddb-9e6ac1b47bb8@arm.com> 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: Marc Zyngier Cc: Mark Rutland , Andrew Lunn , Jason Cooper , devicetree@vger.kernel.org, Antoine Tenart , Catalin Marinas , Gregory Clement , Haim Boot , Will Deacon , Maxime Chevallier , Nadav Haklai , Rob Herring , Thomas Petazzoni , Thomas Gleixner , Hanna Hawa , linux-arm-kernel@lists.infradead.org, Sebastian Hesselbarth List-Id: devicetree@vger.kernel.org SGkgTWFyYywKCk1hcmMgWnluZ2llciA8bWFyYy56eW5naWVyQGFybS5jb20+IHdyb3RlIG9uIFR1 ZSwgMjEgQXVnIDIwMTggMTE6Mzc6NDcKKzAxMDA6Cgo+IE9uIDIxLzA4LzE4IDExOjI4LCBNaXF1 ZWwgUmF5bmFsIHdyb3RlOgo+ID4gSGkgTWFyYywKPiA+IAo+ID4gTWFyYyBaeW5naWVyIDxtYXJj Lnp5bmdpZXJAYXJtLmNvbT4gd3JvdGUgb24gVHVlLCAyMSBBdWcgMjAxOCAxMDoxOTowNAo+ID4g KzAxMDA6Cj4gPiAgIAo+ID4+IE9uIDIxLzA4LzE4IDEwOjA4LCBNaXF1ZWwgUmF5bmFsIHdyb3Rl OiAgCj4gPj4+IEhpIE1hcmMsCj4gPj4+Cj4gPj4+IEknbSBmaW5lIHdpdGggdGhlIHJlc3Qgb2Yg dGhlIGNvbW1lbnRzLCBwbGVhc2UgZmluZCBqdXN0IG9uZSBsYXN0Cj4gPj4+IHF1ZXN0aW9uIGJl bG93Lgo+ID4+Pgo+ID4+PiBbLi4uXQo+ID4+PiAgICAgCj4gPj4+Pj4gQEAgLTEzMywxMiArMTY0 LDM2IEBAIG12ZWJ1X2ljdV9pcnFfZG9tYWluX3RyYW5zbGF0ZShzdHJ1Y3QgaXJxX2RvbWFpbiAq ZCwgc3RydWN0IGlycV9md3NwZWMgKmZ3c3BlYywKPiA+Pj4+PiAgCQlyZXR1cm4gLUVJTlZBTDsK PiA+Pj4+PiAgCX0KPiA+Pj4+PiAgCj4gPj4+Pj4gLQkvKiBNYXNrIHRoZSB0eXBlIHRvIHByZXZl bnQgd3JvbmcgRFQgY29uZmlndXJhdGlvbiAqLwo+ID4+Pj4+IC0JKnR5cGUgPSBmd3NwZWMtPnBh cmFtWzJdICYgSVJRX1RZUEVfU0VOU0VfTUFTSzsKPiA+Pj4+PiArCS8qCj4gPj4+Pj4gKwkgKiBU aGUgSUNVIHJlY2VpdmVzIGxldmVsLWludGVycnVwdHMuIE1TSSBTRUkgYXJlCj4gPj4+Pj4gKwkg KiBlZGdlLWludGVycnVwdHMgd2hpbGUgTVNJIE5TUiBhcmUgbGV2ZWwtaW50ZXJydXB0cy4gVXBk YXRlIHRoZSB0eXBlCj4gPj4+Pj4gKwkgKiBhY2NvcmRpbmdseSBmb3IgdGhlIHBhcmVudCBpcnFj aGlwLgo+ID4+Pj4+ICsJICovCj4gPj4+Pj4gKwlpZiAobXNpX2RhdGEtPnN1YnNldF9kYXRhLT5p Y3VfZ3JvdXAgPT0gSUNVX0dSUF9TRUkpCj4gPj4+Pj4gKwkJKnR5cGUgPSBJUlFfVFlQRV9FREdF X1JJU0lORzsgICAgICAKPiA+Pj4+Cj4gPj4+PiBUaGF0J3MgaW50ZXJlc3RpbmcuIEhvdyBpcyB0 aGUgcmVzYW1wbGluZyBkb25lIGhlcmU/ICAgIAo+ID4+Pgo+ID4+PiBJJ20gbm90IHN1cmUgdG8g dW5kZXJzdGFuZCB0aGUgcXVlc3Rpb24uIFdoYXQgZG9lcyAncmVzYW1wbGluZycgbWVhbnMKPiA+ Pj4gaW4gc3VjaCBjb250ZXh0PyBNU0kgU0VJcyBhcmUgb2YgdHlwZSAiZWRnZSIgYW5kIHVzZSB0 aGUgdHJhZGl0aW9uYWwKPiA+Pj4gTVNJIHNpZ25hbGxpbmcgaW5mcmFzdHJ1Y3R1cmUuIEknbSBh c2tpbmcgdG8gYmUgc3VyZSBub3QgdG8gaWdub3JlCj4gPj4+IHNvbWV0aGluZyB3cm9uZyBpbiBt eSBjb2RlLiAgICAKPiA+Pgo+ID4+IFlvdSBzZWVtcyB0byBiZSB0dXJuaW5nIGEgbGV2ZWwgaW50 ZXJydXB0IGludG8gYW4gZWRnZS4gIAo+ID4gCj4gPiBJZiBpcyBhbiBTRUkgaW50ZXJydXB0LCBp dCBjYW5ub3QgYmUgYSBsZXZlbCBpbnRlcnJ1cHQgYW5kIHRoZSB0eXBlCj4gPiB3aWxsIGJlIElS UV9UWVBFX0VER0VfUklTSU5HLgo+ID4gICAKPiA+PiBZb3UgY2FuIGRvIHRoYXQsCj4gPj4gYnV0 IG9ubHkgaWYgeW91IHJlc2FtcGxlIHRoZSBsZXZlbCBvbiBFT0kgKGFuZCByZWdlbmVyYXRlIHRo ZQo+ID4+IGNvcnJlc3BvbmRpbmcgZWRnZSBpZiB0aGUgbGV2ZWwgaXMgc3RpbGwgaGlnaCkuICAK PiA+IAo+ID4gV2hhdCBpcyBiZWZvcmUgaXMgYSAnJiBJUlFfVFlQRV9TRU5TRV9NQVNLJyBvcGVy YXRpb24uIEluIHRoZW9yeSwKPiA+ICp0eXBlIGNvdWxkIGJlIGFueXRoaW5nIG9mIElSUV9UWVBF X3tFREdFLExFVkVMfV8qIGF0IHRoaXMgbW9tZW50LiBCdXQsCj4gPiBhcyBzdGF0ZWQgYWJvdmUs IGl0IGNhbm5vdCBiZSBhbnl0aGluZyBlbHNlIHRoYW4gSVJRX1RZUEVfRURHRV9SSVNJTkcuCj4g PiBJIHRob3VnaHQgbW9yZSBjbGVhciB0byBlbmZvcmNlIGl0IGJ1dCBpZiB0aGlzIHVuY2xlYXIg YW5kIHVzZWxlc3MsCj4gPiBsZXQncyBkcm9wIGl0PyAgCj4gCj4gSSB0aGluayBvdmVycmlkaW5n IHRoZSB0cmlnZ2VyIHRoYXQgd2F5IGlzIHZlcnkgY29uZnVzaW5nLiBFaXRoZXIgaXQKPiBjb21l cyBmcm9tIERULCBvciBpdCBjb21lcyBmcm9tIHRoZSBNU0kgZnJhbXdvcmsuIEluIGJvdGggY2Fz ZXMsIGl0IGhhcwo+IHRvIGJlIGNvcnJlY3QsIGFuZCBvdmVycmlkaW5nIGl0IGlzIGp1c3QgcGFw ZXJpbmcgb3ZlciBvdGhlciBidWdzLgo+IAo+IEknZCByYXRoZXIgeW91IHB1dCBhIFdBUk5fT04g aWYgeW91IGVuY291bnRlciBhbiBpbGxlZ2FsIGludGVycnVwdCB0cmlnZ2VyLgoKQWN0dWFsbHkg SSBkbyByZW1lbWJlciBub3cgd2h5IEkgZW5mb3JjZWQgdGhlIHR5cGU6CgpMZXQncyBzYXkgbXkg dGhlcm1hbCBJUCBpbiBhIENQMTEwIHRyaWdnZXJzIGFuIGludGVycnVwdC4gVGhpcwppbnRlcnJ1 cHQgaXMgb2YgdHlwZSBMRVZFTF9ISUdILCBhbmQgaXQgaXMgZGVjbGFyZWQgaW4gdGhlIERUIGFz OgoKICAgIHRoZXJtYWw6IHRoZXJtYWwtc2Vuc29yQC4uLiB7CiAgICAgICAgWy4uLl0KICAgICAg ICBpbnRlcnJ1cHRzLWV4dGVuZGVkID0gPCZpY3Vfc2VpIDExNiBJUlFfVFlQRV9MRVZFTF9ISUdI PjsKICAgIH07CgpUaGUgSUNVIGNhbGxiYWNrIC0+dHJhbnNsYXRlKCkgd2lsbCBiZSBjYWxsZWQg d2l0aCBmd3NwZWMgYmVpbmcgdGhlIEMKdmlldyBvZiB0aGUgYWJvdmUgImludGVycnVwdHMtZXh0 ZW5kZWQiIHByb3BlcnR5LCBzbyB0aGUgaW50ZXJydXB0IHR5cGUKd2lsbCBiZSBMRVZFTF9ISUdI LgoKSG93ZXZlciwgdGhlICJpY3Vfc2VpIiBwYXJlbnQgaXMgdGhlIFNFSSBJUCBpbiB0aGUgQVA4 MDYgYW5kIHRoaXMgSVAKb25seSByZWNlaXZlcyBlZGdlIGludGVycnVwdHMuIFdoYXQgaGFwcGVu cyBpcyB0aGF0IHRoZSBTRUkncwotPnNldF90eXBlKCkgY2FsbGJhY2sgaXMgY2FsbGVkIHdpdGgg TEVWRUxfSElHSCB0eXBlICh3aGlsZSBpdApvbmx5IHN1cHBvcnRzIEVER0VfUklTSU5HIGludGVy cnVwdHMpIGFuZCBJIHdhbnQgdGhlIGRyaXZlciB0byB0aHJvdyBhbgplcnJvciBpbiB0aGlzIGNh c2UuCgpNeSB3YXkgb2YgaGFuZGxpbmcgdGhpcyB3YXMgdG8gZm9yY2UgKnR5cGUgdG8gYmUgRURH RV9SSVNJTkcgaW4gdGhlIElDVQotPnRyYW5zbGF0ZSgpIGNhbGxiYWNrIHdoZW5ldmVyIGFuIFNF SSB3YXMgdHJpZ2dlcmVkLiBBcyB0aGlzIHNlZW1zIG5vdAp0byBiZSBjb3JyZWN0LCBob3cgd291 bGQgeW91IGhhbmRsZSBzdWNoIHNpdHVhdGlvbj8KClRoYW5rcywKTWlxdcOobAoKX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KbGludXgtYXJtLWtlcm5lbCBt YWlsaW5nIGxpc3QKbGludXgtYXJtLWtlcm5lbEBsaXN0cy5pbmZyYWRlYWQub3JnCmh0dHA6Ly9s aXN0cy5pbmZyYWRlYWQub3JnL21haWxtYW4vbGlzdGluZm8vbGludXgtYXJtLWtlcm5lbAo=