LSI problem, endianness & cleanups

[Alex Deucher]

On Wed, Jul 13, 2011 at 1:46 AM, Benjamin Herrenschmidt
<benh at kernel.crashing.org> wrote:
> Hi Alex !
>
> A couple of things I noticed while doing some of the fixes I posted
> recently:
>
>  - There seem to be what could be a HW issue, I would appreciate if you
> could double check with your HW guys, at least on rv610 (I haven't been
> able to test on other cards).
>
> When MSI is -disabled- (using old style LSIs), my PCI-E bridge detected
> an invalid DMA after every interrupt. I am not sure I managed to capture
> the address properly, it might be 0 but I need to double check with my
> own HW guys.
>
> This doesn't happen when MSI is enabled.
>
> I -looks- like the chip might be trying to shoot an MSI even when MSIs
> aren't enabled in the config space, possibly using the (stale) content
> of the config space MSI address register. Can you check if your HW folks
> know anything about this ?
>

I'll see what I can find out.

>  - There's a lot of horrible duplication of register definitions :-) I
> think there's at least 3 copies of CP_RB_CNTL and associated bits. One
> thing I'd like to cleanup (not -that- but related) is the way we set the
> endian swap bits.
>
> Basically, we currently have #ifdef's that duplicate entire multi-line
> statements changing one bit (almost always the same).
>
> I'm thinking about instead defining in a .h something like
>
> #ifdef __BIG_ENDIAN
> #define CP_RB_SWAP      CP_RB_SWAP_32BIT
> #else
> #define CP_RB_SWAP      0
> #endif
>
> And using CP_RB_SWAP everywhere, removing all the dups (which are bug prone
> as always with dups). I'd like to make sure you are ok with that before I
> jump and do it, and especially ok with having a _single_ instance of that bit
> def for all radeon variants since the bits are in the same place everywhere.
>

Seems ok to me.  But as you mentioned in IRC, I think in most cases we
should just do the swap on the CPU.  Also we generally use the same
bits for all the endian swap fields so we could do something like:

#define ENDIAN_NONE 0
#define ENDIAN_8IN16 1
#define ENDIAN_8IN32 2

#ifdef __BIG_ENDIAN
#define DFLT_SWAP ENDIAN_8IN32
#else
#define DFLT_SWAP ENDIAN_NONE
#endif

And use it for all the relevant fields.

Alex