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

[Brian Norris]

Hi,

On Wed, Jul 13, 2016 at 09:45:43AM +0800, Shawn Lin wrote:
> å 2016/7/13 9:31, Brian Norris åé:
> >On Wed, Jul 13, 2016 at 09:10:15AM +0800, Shawn Lin wrote:
> >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
> PCIE_CLIENT_INT_STATUS[5:8]... ?

Possibly better than starting from 1, but IMO also doesn't make sense,
because the other bits aren't interrupts you want to translate on behalf
of other devices (are they?) -- they're interrupt bits consumed by the
host controller itself. (If they are possibly needed for translation,
then sure, index the entire status register and handle it in the driver,
and start the INTx mapping from 5 here.)

[...]

> >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.

It's simple, but I think it influenced code understanding and bugginess.

> So if it's more sensible to you, I will apply your suggestion.

Well, I was just offering my opinion. I think it makes more sense, but
maybe it doesn't to you.

Brian