From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 12 Jul 2016 18:31:18 -0700 From: Brian Norris To: Shawn Lin Cc: devicetree@vger.kernel.org, Heiko Stuebner , Arnd Bergmann , Marc Zyngier , linux-pci@vger.kernel.org, Wenrui Li , linux-kernel@vger.kernel.org, Doug Anderson , linux-rockchip@lists.infradead.org, Rob Herring , Bjorn Helgaas Subject: Re: [PATCH v6 1/2] Documentation: bindings: add dt doc for Rockchip PCIe controller Message-ID: <20160713013117.GA130126@google.com> References: <1467789398-13501-1-git-send-email-shawn.lin@rock-chips.com> <20160707003912.GB100467@google.com> <59b328c9-3a21-36fa-7f28-4ba24d77fef0@rock-chips.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 In-Reply-To: <59b328c9-3a21-36fa-7f28-4ba24d77fef0@rock-chips.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: Hi Shawn, On Wed, Jul 13, 2016 at 09:10:15AM +0800, Shawn Lin wrote: > 在 2016/7/7 8:39, Brian Norris 写道: > >On Wed, Jul 06, 2016 at 03:16:37PM +0800, Shawn Lin wrote: > >>+ #interrupt-cells = <1>; > >>+ interrupt-map-mask = <0 0 0 7>; > >>+ interrupt-map = <0 0 0 1 &pcie0_intc 1>, > >>+ <0 0 0 2 &pcie0_intc 2>, > >>+ <0 0 0 3 &pcie0_intc 3>, > >>+ <0 0 0 4 &pcie0_intc 4>; > > > >I'm a little lost on this one, so forgive my ignorance; how did you > >determine the last value in each entry (i.e., the 1, 2, 3, and 4 IRQ > >numbers for pcie0_intc)? IIUC, those are supposed to represent indeces > >into the IRQ status register found in the PCIe interrupt status > >register, and so they should be 0-based (i.e., 0, 1, 2, 3). And then > >you'd have: > > > > interrupt-map = <0 0 0 1 &pcie0_intc 0>, > > <0 0 0 2 &pcie0_intc 1>, > > <0 0 0 3 &pcie0_intc 2>, > > <0 0 0 4 &pcie0_intc 3>; > > > >But then, I never got this sub-node binding to work quite right, so I > >may be missing something. > > > >EDIT: ooh, I see what's going on! I'll comment on the driver as well, > >but it looks like you're translating the register status to a HW IRQ > >number with 'ffs(reg)', which yields a 1-based index. I think it is most > >sensible to use a 0-based index (i.e., 'ffs(reg) - 1'). Now, that only > >will work if you get the whole interrupt-map + interrupt-controller > >thing right (i.e., using a subnode for the interrupt controller) -- > >otherwise, IRQ mapping might not work right. I suspect that's one reason > >the original driver writer might have used 1-based indexing in the first > >place. > > yes, I got it but.....what's the difference? At some level, it's a matter of preference. But when you're talking about the rk3399 PCIe "interrupt controller" domain, it seems that you should be talking about HW bits in the controller -- i.e., you have a 4-bit interrupt status bitfield, that we typically call [0:3]. If you use [1:4], then you have to remember to subtract 1 mentally when mapping to the actual HW bit. I believe that confusion (since bitfields normally count from 0) might have helped cause the infinite loop bug I noticed too. And I also think that counting from 0 helps clarify the fact that your interrupt controller indexing is an independent numbering from the PCI interrupt numbering, even though they happen to map 1:1. But then, PCI INTx numbering is kinda weird already, as it starts from 1. So maybe it's just as valid to say our domain starts from 1 as well. > You still need to get the whole interrupt-map + interrupt-controller > things right and the code(ffs(reg) - 1)if applied your suggestion. Yes, of course. And I already sent you patches that do that. > Look at most of the docs for pcie bindings, I saw they also take > 0-base index, how about? I don't know which ones you're referring to. I see that altera-pcie.txt supports interrupt indeces counting from 1, but that's probably because they're using the same broken binding that was in your ~v3 patches (where the pcie node has both 'interrupt-controller' and 'interrupt-map', with phandles to itself), so they had no other choice. If you still think it makes more sense to count from 1, then I won't stop you. Regards, Brian From mboxrd@z Thu Jan 1 00:00:00 1970 From: Brian Norris Subject: Re: [PATCH v6 1/2] Documentation: bindings: add dt doc for Rockchip PCIe controller Date: Tue, 12 Jul 2016 18:31:18 -0700 Message-ID: <20160713013117.GA130126@google.com> References: <1467789398-13501-1-git-send-email-shawn.lin@rock-chips.com> <20160707003912.GB100467@google.com> <59b328c9-3a21-36fa-7f28-4ba24d77fef0@rock-chips.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: <59b328c9-3a21-36fa-7f28-4ba24d77fef0-TNX95d0MmH7DzftRWevZcw@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+glpar-linux-rockchip=m.gmane.org-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org To: Shawn Lin Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Heiko Stuebner , Arnd Bergmann , Marc Zyngier , linux-pci-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Wenrui Li , linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Doug Anderson , linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, Rob Herring , Bjorn Helgaas List-Id: linux-rockchip.vger.kernel.org SGkgU2hhd24sCgpPbiBXZWQsIEp1bCAxMywgMjAxNiBhdCAwOToxMDoxNUFNICswODAwLCBTaGF3 biBMaW4gd3JvdGU6Cj4g5ZyoIDIwMTYvNy83IDg6MzksIEJyaWFuIE5vcnJpcyDlhpnpgZM6Cj4g Pk9uIFdlZCwgSnVsIDA2LCAyMDE2IGF0IDAzOjE2OjM3UE0gKzA4MDAsIFNoYXduIExpbiB3cm90 ZToKPiA+PisJI2ludGVycnVwdC1jZWxscyA9IDwxPjsKPiA+PisJaW50ZXJydXB0LW1hcC1tYXNr ID0gPDAgMCAwIDc+Owo+ID4+KwlpbnRlcnJ1cHQtbWFwID0gPDAgMCAwIDEgJnBjaWUwX2ludGMg MT4sCj4gPj4rCQkJPDAgMCAwIDIgJnBjaWUwX2ludGMgMj4sCj4gPj4rCQkJPDAgMCAwIDMgJnBj aWUwX2ludGMgMz4sCj4gPj4rCQkJPDAgMCAwIDQgJnBjaWUwX2ludGMgND47Cj4gPgo+ID5JJ20g YSBsaXR0bGUgbG9zdCBvbiB0aGlzIG9uZSwgc28gZm9yZ2l2ZSBteSBpZ25vcmFuY2U7IGhvdyBk aWQgeW91Cj4gPmRldGVybWluZSB0aGUgbGFzdCB2YWx1ZSBpbiBlYWNoIGVudHJ5IChpLmUuLCB0 aGUgMSwgMiwgMywgYW5kIDQgSVJRCj4gPm51bWJlcnMgZm9yIHBjaWUwX2ludGMpPyBJSVVDLCB0 aG9zZSBhcmUgc3VwcG9zZWQgdG8gcmVwcmVzZW50IGluZGVjZXMKPiA+aW50byB0aGUgSVJRIHN0 YXR1cyByZWdpc3RlciBmb3VuZCBpbiB0aGUgUENJZSBpbnRlcnJ1cHQgc3RhdHVzCj4gPnJlZ2lz dGVyLCBhbmQgc28gdGhleSBzaG91bGQgYmUgMC1iYXNlZCAoaS5lLiwgMCwgMSwgMiwgMykuIEFu ZCB0aGVuCj4gPnlvdSdkIGhhdmU6Cj4gPgo+ID4JaW50ZXJydXB0LW1hcCA9IDwwIDAgMCAxICZw Y2llMF9pbnRjIDA+LAo+ID4JCQk8MCAwIDAgMiAmcGNpZTBfaW50YyAxPiwKPiA+CQkJPDAgMCAw IDMgJnBjaWUwX2ludGMgMj4sCj4gPgkJCTwwIDAgMCA0ICZwY2llMF9pbnRjIDM+Owo+ID4KPiA+ QnV0IHRoZW4sIEkgbmV2ZXIgZ290IHRoaXMgc3ViLW5vZGUgYmluZGluZyB0byB3b3JrIHF1aXRl IHJpZ2h0LCBzbyBJCj4gPm1heSBiZSBtaXNzaW5nIHNvbWV0aGluZy4KPiA+Cj4gPkVESVQ6IG9v aCwgSSBzZWUgd2hhdCdzIGdvaW5nIG9uISBJJ2xsIGNvbW1lbnQgb24gdGhlIGRyaXZlciBhcyB3 ZWxsLAo+ID5idXQgaXQgbG9va3MgbGlrZSB5b3UncmUgdHJhbnNsYXRpbmcgdGhlIHJlZ2lzdGVy IHN0YXR1cyB0byBhIEhXIElSUQo+ID5udW1iZXIgd2l0aCAnZmZzKHJlZyknLCB3aGljaCB5aWVs ZHMgYSAxLWJhc2VkIGluZGV4LiBJIHRoaW5rIGl0IGlzIG1vc3QKPiA+c2Vuc2libGUgdG8gdXNl IGEgMC1iYXNlZCBpbmRleCAoaS5lLiwgJ2ZmcyhyZWcpIC0gMScpLiBOb3csIHRoYXQgb25seQo+ ID53aWxsIHdvcmsgaWYgeW91IGdldCB0aGUgd2hvbGUgaW50ZXJydXB0LW1hcCArIGludGVycnVw dC1jb250cm9sbGVyCj4gPnRoaW5nIHJpZ2h0IChpLmUuLCB1c2luZyBhIHN1Ym5vZGUgZm9yIHRo ZSBpbnRlcnJ1cHQgY29udHJvbGxlcikgLS0KPiA+b3RoZXJ3aXNlLCBJUlEgbWFwcGluZyBtaWdo dCBub3Qgd29yayByaWdodC4gSSBzdXNwZWN0IHRoYXQncyBvbmUgcmVhc29uCj4gPnRoZSBvcmln aW5hbCBkcml2ZXIgd3JpdGVyIG1pZ2h0IGhhdmUgdXNlZCAxLWJhc2VkIGluZGV4aW5nIGluIHRo ZSBmaXJzdAo+ID5wbGFjZS4KPiAKPiB5ZXMsIEkgZ290IGl0IGJ1dC4uLi4ud2hhdCdzIHRoZSBk aWZmZXJlbmNlPwoKQXQgc29tZSBsZXZlbCwgaXQncyBhIG1hdHRlciBvZiBwcmVmZXJlbmNlLiBC dXQgd2hlbiB5b3UncmUgdGFsa2luZwphYm91dCB0aGUgcmszMzk5IFBDSWUgImludGVycnVwdCBj b250cm9sbGVyIiBkb21haW4sIGl0IHNlZW1zIHRoYXQgeW91CnNob3VsZCBiZSB0YWxraW5nIGFi b3V0IEhXIGJpdHMgaW4gdGhlIGNvbnRyb2xsZXIgLS0gaS5lLiwgeW91IGhhdmUgYQo0LWJpdCBp bnRlcnJ1cHQgc3RhdHVzIGJpdGZpZWxkLCB0aGF0IHdlIHR5cGljYWxseSBjYWxsIFswOjNdLiBJ ZiB5b3UKdXNlIFsxOjRdLCB0aGVuIHlvdSBoYXZlIHRvIHJlbWVtYmVyIHRvIHN1YnRyYWN0IDEg bWVudGFsbHkgd2hlbiBtYXBwaW5nCnRvIHRoZSBhY3R1YWwgSFcgYml0LiBJIGJlbGlldmUgdGhh dCBjb25mdXNpb24gKHNpbmNlIGJpdGZpZWxkcyBub3JtYWxseQpjb3VudCBmcm9tIDApIG1pZ2h0 IGhhdmUgaGVscGVkIGNhdXNlIHRoZSBpbmZpbml0ZSBsb29wIGJ1ZyBJIG5vdGljZWQKdG9vLiBB bmQgSSBhbHNvIHRoaW5rIHRoYXQgY291bnRpbmcgZnJvbSAwIGhlbHBzIGNsYXJpZnkgdGhlIGZh Y3QgdGhhdAp5b3VyIGludGVycnVwdCBjb250cm9sbGVyIGluZGV4aW5nIGlzIGFuIGluZGVwZW5k ZW50IG51bWJlcmluZyBmcm9tIHRoZQpQQ0kgaW50ZXJydXB0IG51bWJlcmluZywgZXZlbiB0aG91 Z2ggdGhleSBoYXBwZW4gdG8gbWFwIDE6MS4KCkJ1dCB0aGVuLCBQQ0kgSU5UeCBudW1iZXJpbmcg aXMga2luZGEgd2VpcmQgYWxyZWFkeSwgYXMgaXQgc3RhcnRzIGZyb20KMS4gU28gbWF5YmUgaXQn cyBqdXN0IGFzIHZhbGlkIHRvIHNheSBvdXIgZG9tYWluIHN0YXJ0cyBmcm9tIDEgYXMgd2VsbC4K Cj4gWW91IHN0aWxsIG5lZWQgdG8gZ2V0IHRoZSB3aG9sZSBpbnRlcnJ1cHQtbWFwICsgaW50ZXJy dXB0LWNvbnRyb2xsZXIKPiB0aGluZ3MgcmlnaHQgYW5kIHRoZSBjb2RlKGZmcyhyZWcpIC0gMSlp ZiBhcHBsaWVkIHlvdXIgc3VnZ2VzdGlvbi4KClllcywgb2YgY291cnNlLiBBbmQgSSBhbHJlYWR5 IHNlbnQgeW91IHBhdGNoZXMgdGhhdCBkbyB0aGF0LgoKPiBMb29rIGF0IG1vc3Qgb2YgdGhlIGRv Y3MgZm9yIHBjaWUgYmluZGluZ3MsIEkgc2F3IHRoZXkgYWxzbyB0YWtlCj4gMC1iYXNlIGluZGV4 LCBob3cgYWJvdXQ/CgpJIGRvbid0IGtub3cgd2hpY2ggb25lcyB5b3UncmUgcmVmZXJyaW5nIHRv LiBJIHNlZSB0aGF0IGFsdGVyYS1wY2llLnR4dApzdXBwb3J0cyBpbnRlcnJ1cHQgaW5kZWNlcyBj b3VudGluZyBmcm9tIDEsIGJ1dCB0aGF0J3MgcHJvYmFibHkgYmVjYXVzZQp0aGV5J3JlIHVzaW5n IHRoZSBzYW1lIGJyb2tlbiBiaW5kaW5nIHRoYXQgd2FzIGluIHlvdXIgfnYzIHBhdGNoZXMKKHdo ZXJlIHRoZSBwY2llIG5vZGUgaGFzIGJvdGggJ2ludGVycnVwdC1jb250cm9sbGVyJyBhbmQKJ2lu dGVycnVwdC1tYXAnLCB3aXRoIHBoYW5kbGVzIHRvIGl0c2VsZiksIHNvIHRoZXkgaGFkIG5vIG90 aGVyIGNob2ljZS4KCklmIHlvdSBzdGlsbCB0aGluayBpdCBtYWtlcyBtb3JlIHNlbnNlIHRvIGNv dW50IGZyb20gMSwgdGhlbiBJIHdvbid0CnN0b3AgeW91LgoKUmVnYXJkcywKQnJpYW4KCl9fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCkxpbnV4LXJvY2tjaGlw IG1haWxpbmcgbGlzdApMaW51eC1yb2NrY2hpcEBsaXN0cy5pbmZyYWRlYWQub3JnCmh0dHA6Ly9s aXN0cy5pbmZyYWRlYWQub3JnL21haWxtYW4vbGlzdGluZm8vbGludXgtcm9ja2NoaXAK