Hi Steven,
On Mon, Jul 02, 2018 at 02:00:54PM -0500, Steven J. Hill wrote:
> Remove all unused data types and macros. Define a new macro that
> neatens up the file. Convert the remaining structures to use
> __BITFIELD_FIELD. Use common QLM structure in PCIe code related
> to this clean-up.
Thanks - this is looking better, I've just a few comments/questions
below.
> -#define CVMX_CIU_EN2_PPX_IP4(offset) (CVMX_ADD_IO_SEG(0x000107000000A400ull) + ((offset) & 15) * 8)
> -#define CVMX_CIU_EN2_PPX_IP4_W1C(offset) (CVMX_ADD_IO_SEG(0x000107000000CC00ull) + ((offset) & 15) * 8)
> -#define CVMX_CIU_EN2_PPX_IP4_W1S(offset) (CVMX_ADD_IO_SEG(0x000107000000AC00ull) + ((offset) & 15) * 8)
Offsets here are masked to 4 bits, but after this patch they're only
masked to 16 bits - is that correct?
> -#define CVMX_CIU_INTX_EN0(offset) (CVMX_ADD_IO_SEG(0x0001070000000200ull) + ((offset) & 63) * 16)
> -#define CVMX_CIU_INTX_EN0_W1C(offset) (CVMX_ADD_IO_SEG(0x0001070000002200ull) + ((offset) & 63) * 16)
> -#define CVMX_CIU_INTX_EN0_W1S(offset) (CVMX_ADD_IO_SEG(0x0001070000006200ull) + ((offset) & 63) * 16)
> -#define CVMX_CIU_INTX_EN1(offset) (CVMX_ADD_IO_SEG(0x0001070000000208ull) + ((offset) & 63) * 16)
> -#define CVMX_CIU_INTX_EN1_W1C(offset) (CVMX_ADD_IO_SEG(0x0001070000002208ull) + ((offset) & 63) * 16)
> -#define CVMX_CIU_INTX_EN1_W1S(offset) (CVMX_ADD_IO_SEG(0x0001070000006208ull) + ((offset) & 63) * 16)
> -#define CVMX_CIU_INTX_SUM0(offset) (CVMX_ADD_IO_SEG(0x0001070000000000ull) + ((offset) & 63) * 8)
Similarly these ones are all masked to 6 bits, but after your patch
they're only masked to 18 bits.
> +#define CVMX_CIU_SUM2_PPX_IP4(c) CVMX_CIU_ADDR(0x8c00, c, 0x0ffff, 8)
And this one changes from being masked to 4 bits, to being masked to 16
bits.
> +static inline uint64_t CVMX_CIU_PP_POKEX(unsigned int coreid)
> {
> switch (cvmx_get_octeon_family()) {
> - case OCTEON_CN30XX & OCTEON_FAMILY_MASK:
> - return CVMX_ADD_IO_SEG(0x0001070000000580ull) + (offset) * 8;
> - case OCTEON_CN52XX & OCTEON_FAMILY_MASK:
> - case OCTEON_CNF71XX & OCTEON_FAMILY_MASK:
> - case OCTEON_CN61XX & OCTEON_FAMILY_MASK:
> - case OCTEON_CN70XX & OCTEON_FAMILY_MASK:
> - return CVMX_ADD_IO_SEG(0x0001070000000580ull) + (offset) * 8;
> - case OCTEON_CN31XX & OCTEON_FAMILY_MASK:
> - case OCTEON_CN50XX & OCTEON_FAMILY_MASK:
> - return CVMX_ADD_IO_SEG(0x0001070000000580ull) + (offset) * 8;
> - case OCTEON_CN38XX & OCTEON_FAMILY_MASK:
> - case OCTEON_CN58XX & OCTEON_FAMILY_MASK:
> - return CVMX_ADD_IO_SEG(0x0001070000000580ull) + (offset) * 8;
> - case OCTEON_CN56XX & OCTEON_FAMILY_MASK:
> - return CVMX_ADD_IO_SEG(0x0001070000000580ull) + (offset) * 8;
> - case OCTEON_CN66XX & OCTEON_FAMILY_MASK:
> - return CVMX_ADD_IO_SEG(0x0001070000000580ull) + (offset) * 8;
> - case OCTEON_CN63XX & OCTEON_FAMILY_MASK:
> - return CVMX_ADD_IO_SEG(0x0001070000000580ull) + (offset) * 8;
> case OCTEON_CN68XX & OCTEON_FAMILY_MASK:
> - return CVMX_ADD_IO_SEG(0x0001070100100200ull) + (offset) * 8;
> + return CVMX_CIU_ADDR(0x100100200, coreid, 0xffff, 8);
> case OCTEON_CNF75XX & OCTEON_FAMILY_MASK:
> case OCTEON_CN73XX & OCTEON_FAMILY_MASK:
> case OCTEON_CN78XX & OCTEON_FAMILY_MASK:
> - return CVMX_ADD_IO_SEG(0x0001010000030000ull) + (offset) * 8;
> + return (CVMX_CIU_ADDR(0x000030000, coreid, 0xffff, 8) -
> + 0x60000000000ull);
The brackets around the expression are redundant here.
> +static inline uint64_t CVMX_CIU_WDOGX(unsigned int coreid)
> {
> switch (cvmx_get_octeon_family()) {
> - case OCTEON_CN30XX & OCTEON_FAMILY_MASK:
> - return CVMX_ADD_IO_SEG(0x0001070000000500ull) + (offset) * 8;
> - case OCTEON_CN52XX & OCTEON_FAMILY_MASK:
> - case OCTEON_CNF71XX & OCTEON_FAMILY_MASK:
> - case OCTEON_CN61XX & OCTEON_FAMILY_MASK:
> - case OCTEON_CN70XX & OCTEON_FAMILY_MASK:
> - return CVMX_ADD_IO_SEG(0x0001070000000500ull) + (offset) * 8;
> - case OCTEON_CN31XX & OCTEON_FAMILY_MASK:
> - case OCTEON_CN50XX & OCTEON_FAMILY_MASK:
> - return CVMX_ADD_IO_SEG(0x0001070000000500ull) + (offset) * 8;
> - case OCTEON_CN38XX & OCTEON_FAMILY_MASK:
> - case OCTEON_CN58XX & OCTEON_FAMILY_MASK:
> - return CVMX_ADD_IO_SEG(0x0001070000000500ull) + (offset) * 8;
> - case OCTEON_CN56XX & OCTEON_FAMILY_MASK:
> - return CVMX_ADD_IO_SEG(0x0001070000000500ull) + (offset) * 8;
> - case OCTEON_CN66XX & OCTEON_FAMILY_MASK:
> - return CVMX_ADD_IO_SEG(0x0001070000000500ull) + (offset) * 8;
> - case OCTEON_CN63XX & OCTEON_FAMILY_MASK:
> - return CVMX_ADD_IO_SEG(0x0001070000000500ull) + (offset) * 8;
> case OCTEON_CN68XX & OCTEON_FAMILY_MASK:
> - return CVMX_ADD_IO_SEG(0x0001070100100000ull) + (offset) * 8;
> + return CVMX_CIU_ADDR(0x100100000, coreid, 0xffff, 8);
> case OCTEON_CNF75XX & OCTEON_FAMILY_MASK:
> case OCTEON_CN73XX & OCTEON_FAMILY_MASK:
> case OCTEON_CN78XX & OCTEON_FAMILY_MASK:
> - return CVMX_ADD_IO_SEG(0x0001010000020000ull) + (offset) * 8;
> + return (CVMX_CIU_ADDR(0x000020000, coreid, 0xffff, 8) -
> + 0x0000060000000000ull);
Same here - redundant brackets.
Apart from those the content looks OK to me now - my only other niggle
is that there's so much of it! 10k lines is a long patch. Perhaps you
could split this into a few separate patches? For example:
1) Remove all the totally unused structs & macros. This would still be
long but as it's all deletions review is fairly straightforward.
2) Unify cvmx_ciu_qlm1 & cvmx_ciu_qlm0 into cvmx_ciu_qlm.
3) Convert the remaining structs to use __BITFIELD_FIELD.
4) Clean up the switch statements.
Thanks,
Paul
