Re: [PATCH v6 1/2] Documentation: bindings: add dt doc for Rockchip PCIe controller

From: Shawn Lin
Date: Tue Jul 12 2016 - 21:46:35 EST

å 2016/7/13 9:31, Brian Norris åé:
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

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.

If that's the fact of how we should numbering our index base, we should
probably start if from 5 as the layout of INTx is

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.

I don't have a hard opinion for the index base as I think it's trivial.
So if it's more sensible to you, I will apply your suggestion.


Best Regards
Shawn Lin