dw-dma: fix regression

[lyakh]

Commit e2b994e ("dw-dma: cleanup the whole driver") introduced
bugs in bit-field calculations. This patch fixes them.
I think we want to have this for 1.3

[lgirdwood]

I assume these bits are only used on CAVS 1.8+ ?

[lyakh]

I assume these bits are only used on CAVS 1.8+ ?

At least I didn't change their use, I only fixed their definition. Looking at the sources those fields are only used for SPI and dai.c, where they are initialised from FIFO handshakes. As far as I understand that code is used on all platforms. But as long as handshakes are <= 15, that change has no effect.

[tlauda]

Why do they overlap? It doesn't seem correct.

[lyakh]

they don't overlap. x above has its low 4 bits = 0. So it's 0 or in the range 0x10 <= x <= 0x30 When you shift it by 24 bits you get bits 27..24 unaffected and bits 29..28 set. When you shift it by 26 bits you get bits 29..26 unaffected and only bits 31..30 set.

[tlauda]

But doing it this way makes it look weird and not right. Register definitions should be aligned with the real hardware. Better way would be to change DW_CFGH_DST and DW_CFGH_SRC macros:
(DW_CFGH_DST_PER_EXT(((x) & 0x30) >> 4) | DW_CFGH_DST_PER((x) & 0xF))

[xiulipan]

I agreed with @tlauda
If the only valid bits is 31,30, we should change the X value but not the definition here.
If we need some X value with low 4 bit is 0, you should add some comments here to explain.

[lyakh]

@tlauda sure, I agree, that this way the code is working around the definition. But it was your patch to begin using SET_BITS() here, so, I didn't want to change that. As for your proposed new definition, it's equivalent to mine, but it adds redundancy. It first shifts right by four bits to then shift left by 30 bits, which is in this case equivalent to just shifting left by 26 bits, which is exactly what my patch does. We can add a comment as @xiulipan has suggested, sure.

[tlauda]

@lyakh Yes, it's redundant, but at least the definition is aligned with the hardware register definition. It doesn't seem right to modify it just to avoid redundancy. That's why the DW_CFGH_DST and DW_CFGH_SRC macros were introduced - to hide the internal logic of programming x value.

[xiulipan]

@lyakh @tlauda
Then let make the definition here map to hardware and add comment or change the logic in DW_CFGH_DST and DW_CFGH_SRC

[lyakh]

I think I've got a better version of the patch now.

[xiulipan]

Maybe add some inline comment to explain the >>4 is better.