Change in coreboot[master]: nb/intel/{haswell,ironlake,pineview,sandybridge,x4x}: Refactor MCHBAR...

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45487 )

Change subject: nb/intel/{haswell,ironlake,pineview,sandybridge,x4x}: Refactor MCHBARx_AND_OR
......................................................................

nb/intel/{haswell,ironlake,pineview,sandybridge,x4x}: Refactor MCHBARx_AND_OR

Add MCHBAR_AND_OR macro and refactor MCHBARx_AND_OR, MCHBARx_AND and MCHBARx_OR macros.

Change-Id: Ica23f58ffd79b17d4d68bd7a34a719691d654525
Signed-off-by: Elyes HAOUAS 
---
M src/northbridge/intel/haswell/haswell.h
M src/northbridge/intel/ironlake/ironlake.h
M src/northbridge/intel/pineview/pineview.h
M src/northbridge/intel/sandybridge/sandybridge.h
M src/northbridge/intel/x4x/x4x.h
5 files changed, 91 insertions(+), 63 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/45487/1

diff --git a/src/northbridge/intel/haswell/haswell.h b/src/northbridge/intel/haswell/haswell.h
index 9a99c2a..fe37fb7 100644
--- a/src/northbridge/intel/haswell/haswell.h
+++ b/src/northbridge/intel/haswell/haswell.h
@@ -41,18 +41,24 @@
  * MCHBAR
  */

-#define MCHBAR8(x)  (*((volatile u8  *)(DEFAULT_MCHBAR + (x))))
-#define MCHBAR16(x) (*((volatile u16 *)(DEFAULT_MCHBAR + (x))))
-#define MCHBAR32(x) (*((volatile u32 *)(DEFAULT_MCHBAR + (x))))
-#define MCHBAR8_AND(x,  and) (MCHBAR8(x)  = MCHBAR8(x)  & (and))
-#define MCHBAR16_AND(x, and) (MCHBAR16(x) = MCHBAR16(x) & (and))
-#define MCHBAR32_AND(x, and) (MCHBAR32(x) = MCHBAR32(x) & (and))
-#define MCHBAR8_OR(x,  or) (MCHBAR8(x)  = MCHBAR8(x)  | (or))
-#define MCHBAR16_OR(x, or) (MCHBAR16(x) = MCHBAR16(x) | (or))
-#define MCHBAR32_OR(x, or) (MCHBAR32(x) = MCHBAR32(x) | (or))
-#define MCHBAR8_AND_OR(x,  and, or) (MCHBAR8(x)  = (MCHBAR8(x)  & (and)) | (or))
-#define MCHBAR16_AND_OR(x, and, or) (MCHBAR16(x) = (MCHBAR16(x) & (and)) | (or))
-#define MCHBAR32_AND_OR(x, and, or) (MCHBAR32(x) = (MCHBAR32(x) & (and)) | (or))
+#define MCHBAR8(x) (*((volatile u8 *)(DEFAULT_MCHBAR + x)))
+#define MCHBAR16(x) (*((volatile u16 *)(DEFAULT_MCHBAR + x)))
+#define MCHBAR32(x) (*((volatile u32 *)(DEFAULT_MCHBAR + x)))
+
+#define MCHBAR_AND_OR(bits, x, and, or) \
+	(MCHBAR##bits(x) = ((MCHBAR##bits(x) & (and)) | (or)))
+
+#define MCHBAR8_AND(x, and) MCHBAR_AND_OR(8, x, and, 0)
+#define MCHBAR16_AND(x, and) MCHBAR_AND_OR(8, x, and, 0)
+#define MCHBAR32_AND(x, and) MCHBAR_AND_OR(8, x, and, 0)
+
+#define MCHBAR8_OR(x, or) MCHBAR_AND_OR(8, x, ~0UL, or)
+#define MCHBAR16_OR(x, or) MCHBAR_AND_OR(16, x, ~0UL, or)
+#define MCHBAR32_OR(x, or) MCHBAR_AND_OR(32, x, ~0UL, or)
+
+#define MCHBAR8_AND_OR(x, and, or) MCHBAR_AND_OR(8, x, and, or)
+#define MCHBAR16_AND_OR(x, and, or) MCHBAR_AND_OR(16, x, and, or)
+#define MCHBAR32_AND_OR(x, and, or) MCHBAR_AND_OR(32, x, and, or)

/* As there are many registers, define them on a separate file */
 #include "mchbar_regs.h"
diff --git a/src/northbridge/intel/ironlake/ironlake.h b/src/northbridge/intel/ironlake/ironlake.h
index 325de5b..9d11edd 100644
--- a/src/northbridge/intel/ironlake/ironlake.h
+++ b/src/northbridge/intel/ironlake/ironlake.h
@@ -103,18 +103,25 @@
  * MCHBAR
  */

-#define MCHBAR8(x)			(*((volatile u8  *)(DEFAULT_MCHBAR + (x))))
-#define MCHBAR16(x)			(*((volatile u16 *)(DEFAULT_MCHBAR + (x))))
-#define MCHBAR32(x)			(*((volatile u32 *)(DEFAULT_MCHBAR + (x))))
-#define MCHBAR8_AND(x,  and)		(MCHBAR8(x)  = MCHBAR8(x)  & (and))
-#define MCHBAR16_AND(x, and)		(MCHBAR16(x) = MCHBAR16(x) & (and))
-#define MCHBAR32_AND(x, and)		(MCHBAR32(x) = MCHBAR32(x) & (and))
-#define MCHBAR8_OR(x,  or)		(MCHBAR8(x)  = MCHBAR8(x)  | (or))
-#define MCHBAR16_OR(x, or)		(MCHBAR16(x) = MCHBAR16(x) | (or))
-#define MCHBAR32_OR(x, or)		(MCHBAR32(x) = MCHBAR32(x) | (or))
-#define MCHBAR8_AND_OR(x,  and, or)	(MCHBAR8(x)  = (MCHBAR8(x)  & (and)) | (or))
-#define MCHBAR16_AND_OR(x, and, or)	(MCHBAR16(x) = (MCHBAR16(x) & (and)) | (or))
-#define MCHBAR32_AND_OR(x, and, or)	(MCHBAR32(x) = (MCHBAR32(x) & (and)) | (or))
+#define MCHBAR8(x) (*((volatile u8 *)(DEFAULT_MCHBAR + x)))
+#define MCHBAR16(x) (*((volatile u16 *)(DEFAULT_MCHBAR + x)))
+#define MCHBAR32(x) (*((volatile u32 *)(DEFAULT_MCHBAR + x)))
+
+#define MCHBAR_AND_OR(bits, x, and, or) \
+	(MCHBAR##bits(x) = ((MCHBAR##bits(x) & (and)) | (or)))
+
+#define MCHBAR8_AND(x, and) MCHBAR_AND_OR(8, x, and, 0)
+#define MCHBAR16_AND(x, and) MCHBAR_AND_OR(8, x, and, 0)
+#define MCHBAR32_AND(x, and) MCHBAR_AND_OR(8, x, and, 0)
+
+#define MCHBAR8_OR(x, or) MCHBAR_AND_OR(8, x, ~0UL, or)
+#define MCHBAR16_OR(x, or) MCHBAR_AND_OR(16, x, ~0UL, or)
+#define MCHBAR32_OR(x, or) MCHBAR_AND_OR(32, x, ~0UL, or)
+
+#define MCHBAR8_AND_OR(x, and, or) MCHBAR_AND_OR(8, x, and, or)
+#define MCHBAR16_AND_OR(x, and, or) MCHBAR_AND_OR(16, x, and, or)
+#define MCHBAR32_AND_OR(x, and, or) MCHBAR_AND_OR(32, x, and, or)
+
 /*
  * EPBAR - Egress Port Root Complex Register Block
  */
diff --git a/src/northbridge/intel/pineview/pineview.h b/src/northbridge/intel/pineview/pineview.h
index 47257dd..c565d0d 100644
--- a/src/northbridge/intel/pineview/pineview.h
+++ b/src/northbridge/intel/pineview/pineview.h
@@ -38,18 +38,24 @@
  * MCHBAR
  */

-#define MCHBAR8(x)  (*((volatile u8  *)(DEFAULT_MCHBAR + (x))))
-#define MCHBAR16(x) (*((volatile u16 *)(DEFAULT_MCHBAR + (x))))
-#define MCHBAR32(x) (*((volatile u32 *)(DEFAULT_MCHBAR +  x)))	/* FIXME: causes changes */
-#define MCHBAR8_AND(x,  and) (MCHBAR8(x)  = MCHBAR8(x)  & (and))
-#define MCHBAR16_AND(x, and) (MCHBAR16(x) = MCHBAR16(x) & (and))
-#define MCHBAR32_AND(x, and) (MCHBAR32(x) = MCHBAR32(x) & (and))
-#define MCHBAR8_OR(x,   or)  (MCHBAR8(x)  = MCHBAR8(x)  | (or))
-#define MCHBAR16_OR(x,  or)  (MCHBAR16(x) = MCHBAR16(x) | (or))
-#define MCHBAR32_OR(x,  or)  (MCHBAR32(x) = MCHBAR32(x) | (or))
-#define MCHBAR8_AND_OR(x,  and, or) (MCHBAR8(x)  = (MCHBAR8(x)  & (and)) | (or))
-#define MCHBAR16_AND_OR(x, and, or) (MCHBAR16(x) = (MCHBAR16(x) & (and)) | (or))
-#define MCHBAR32_AND_OR(x, and, or) (MCHBAR32(x) = (MCHBAR32(x) & (and)) | (or))
+#define MCHBAR8(x) (*((volatile u8 *)(DEFAULT_MCHBAR + x)))
+#define MCHBAR16(x) (*((volatile u16 *)(DEFAULT_MCHBAR + x)))
+#define MCHBAR32(x) (*((volatile u32 *)(DEFAULT_MCHBAR + x)))
+
+#define MCHBAR_AND_OR(bits, x, and, or) \
+	(MCHBAR##bits(x) = ((MCHBAR##bits(x) & (and)) | (or)))
+
+#define MCHBAR8_AND(x, and) MCHBAR_AND_OR(8, x, and, 0)
+#define MCHBAR16_AND(x, and) MCHBAR_AND_OR(8, x, and, 0)
+#define MCHBAR32_AND(x, and) MCHBAR_AND_OR(8, x, and, 0)
+
+#define MCHBAR8_OR(x, or) MCHBAR_AND_OR(8, x, ~0UL, or)
+#define MCHBAR16_OR(x, or) MCHBAR_AND_OR(16, x, ~0UL, or)
+#define MCHBAR32_OR(x, or) MCHBAR_AND_OR(32, x, ~0UL, or)
+
+#define MCHBAR8_AND_OR(x, and, or) MCHBAR_AND_OR(8, x, and, or)
+#define MCHBAR16_AND_OR(x, and, or) MCHBAR_AND_OR(16, x, and, or)
+#define MCHBAR32_AND_OR(x, and, or) MCHBAR_AND_OR(32, x, and, or)

/* As there are many registers, define them on a separate file */

diff --git a/src/northbridge/intel/sandybridge/sandybridge.h b/src/northbridge/intel/sandybridge/sandybridge.h
index 72724a3..5afba72 100644
--- a/src/northbridge/intel/sandybridge/sandybridge.h
+++ b/src/northbridge/intel/sandybridge/sandybridge.h
@@ -64,18 +64,24 @@
  * MCHBAR
  */

-#define MCHBAR8(x)  (*((volatile u8  *)(DEFAULT_MCHBAR + (x))))
-#define MCHBAR16(x) (*((volatile u16 *)(DEFAULT_MCHBAR + (x))))
-#define MCHBAR32(x) (*((volatile u32 *)(DEFAULT_MCHBAR + (x))))
-#define MCHBAR8_AND(x,  and) (MCHBAR8(x)  = MCHBAR8(x)  & (and))
-#define MCHBAR16_AND(x, and) (MCHBAR16(x) = MCHBAR16(x) & (and))
-#define MCHBAR32_AND(x, and) (MCHBAR32(x) = MCHBAR32(x) & (and))
-#define MCHBAR8_OR(x,  or) (MCHBAR8(x)  = MCHBAR8(x)  | (or))
-#define MCHBAR16_OR(x, or) (MCHBAR16(x) = MCHBAR16(x) | (or))
-#define MCHBAR32_OR(x, or) (MCHBAR32(x) = MCHBAR32(x) | (or))
-#define MCHBAR8_AND_OR(x,  and, or) (MCHBAR8(x)  = (MCHBAR8(x)  & (and)) | (or))
-#define MCHBAR16_AND_OR(x, and, or) (MCHBAR16(x) = (MCHBAR16(x) & (and)) | (or))
-#define MCHBAR32_AND_OR(x, and, or) (MCHBAR32(x) = (MCHBAR32(x) & (and)) | (or))
+#define MCHBAR8(x) (*((volatile u8 *)(DEFAULT_MCHBAR + x)))
+#define MCHBAR16(x) (*((volatile u16 *)(DEFAULT_MCHBAR + x)))
+#define MCHBAR32(x) (*((volatile u32 *)(DEFAULT_MCHBAR + x)))
+
+#define MCHBAR_AND_OR(bits, x, and, or) \
+	(MCHBAR##bits(x) = ((MCHBAR##bits(x) & (and)) | (or)))
+
+#define MCHBAR8_AND(x, and) MCHBAR_AND_OR(8, x, and, 0)
+#define MCHBAR16_AND(x, and) MCHBAR_AND_OR(8, x, and, 0)
+#define MCHBAR32_AND(x, and) MCHBAR_AND_OR(8, x, and, 0)
+
+#define MCHBAR8_OR(x, or) MCHBAR_AND_OR(8, x, ~0UL, or)
+#define MCHBAR16_OR(x, or) MCHBAR_AND_OR(16, x, ~0UL, or)
+#define MCHBAR32_OR(x, or) MCHBAR_AND_OR(32, x, ~0UL, or)
+
+#define MCHBAR8_AND_OR(x, and, or) MCHBAR_AND_OR(8, x, and, or)
+#define MCHBAR16_AND_OR(x, and, or) MCHBAR_AND_OR(16, x, and, or)
+#define MCHBAR32_AND_OR(x, and, or) MCHBAR_AND_OR(32, x, and, or)

/* As there are many registers, define them on a separate file */
 #include "mchbar_regs.h"
diff --git a/src/northbridge/intel/x4x/x4x.h b/src/northbridge/intel/x4x/x4x.h
index 45785a0..d2a6fdc 100644
--- a/src/northbridge/intel/x4x/x4x.h
+++ b/src/northbridge/intel/x4x/x4x.h
@@ -40,21 +40,24 @@
  * MCHBAR
  */

-#define MCHBAR8(x) (*((volatile u8 *)(DEFAULT_MCHBAR + (x))))
-#define MCHBAR16(x) (*((volatile u16 *)(DEFAULT_MCHBAR + (x))))
-#define MCHBAR32(x) (*((volatile u32 *)(DEFAULT_MCHBAR + (x))))
-#define MCHBAR8_AND(x, and) (MCHBAR8(x) = MCHBAR8(x) & (and))
-#define MCHBAR8_OR(x, or) (MCHBAR8(x) = MCHBAR8(x) | (or))
-#define MCHBAR8_AND_OR(x, and, or) \
-	(MCHBAR8(x) = (MCHBAR8(x) & (and)) | (or))
-#define MCHBAR16_AND(x, and) (MCHBAR16(x) = MCHBAR16(x) & (and))
-#define MCHBAR16_OR(x, or) (MCHBAR16(x) = MCHBAR16(x) | (or))
-#define MCHBAR16_AND_OR(x, and, or) \
-	(MCHBAR16(x) = (MCHBAR16(x) & (and)) | (or))
-#define MCHBAR32_AND(x, and) (MCHBAR32(x) = MCHBAR32(x) & (and))
-#define MCHBAR32_OR(x, or) (MCHBAR32(x) = MCHBAR32(x) | (or))
-#define MCHBAR32_AND_OR(x, and, or) \
-	(MCHBAR32(x) = (MCHBAR32(x) & (and)) | (or))
+#define MCHBAR8(x) (*((volatile u8 *)(DEFAULT_MCHBAR + x)))
+#define MCHBAR16(x) (*((volatile u16 *)(DEFAULT_MCHBAR + x)))
+#define MCHBAR32(x) (*((volatile u32 *)(DEFAULT_MCHBAR + x)))
+
+#define MCHBAR_AND_OR(bits, x, and, or) \
+	(MCHBAR##bits(x) = ((MCHBAR##bits(x) & (and)) | (or)))
+
+#define MCHBAR8_AND(x, and) MCHBAR_AND_OR(8, x, and, 0)
+#define MCHBAR16_AND(x, and) MCHBAR_AND_OR(8, x, and, 0)
+#define MCHBAR32_AND(x, and) MCHBAR_AND_OR(8, x, and, 0)
+
+#define MCHBAR8_OR(x, or) MCHBAR_AND_OR(8, x, ~0UL, or)
+#define MCHBAR16_OR(x, or) MCHBAR_AND_OR(16, x, ~0UL, or)
+#define MCHBAR32_OR(x, or) MCHBAR_AND_OR(32, x, ~0UL, or)
+
+#define MCHBAR8_AND_OR(x, and, or) MCHBAR_AND_OR(8, x, and, or)
+#define MCHBAR16_AND_OR(x, and, or) MCHBAR_AND_OR(16, x, and, or)
+#define MCHBAR32_AND_OR(x, and, or) MCHBAR_AND_OR(32, x, and, or)

#define CHDECMISC	0x111
 #define STACKED_MEM	(1 << 1)

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45487 )

Change subject: nb/intel/{haswell,ironlake,pineview,sandybridge,x4x}: Refactor MCHBARx_AND_OR
......................................................................

Patch Set 1: Code-Review-1

Um... Why? What is this good for?

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45487 )

Change subject: nb/intel/{haswell,ironlake,pineview,sandybridge,x4x}: Refactor MCHBARx_AND_OR
......................................................................

Patch Set 1:
> Patch Set 1: Code-Review-1
>
> Um... Why? What is this good for?

much more elegant

[HAOUAS Elyes (Code Review)]

Hello build bot (Jenkins), Damien Zammit, Angel Pons, Arthur Heymans, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/45487

to look at the new patch set (#2).

Change subject: nb/intel/{haswell,ironlake,pineview,sandybridge,x4x}: Refactor MCHBARx_AND_OR
......................................................................

nb/intel/{haswell,ironlake,pineview,sandybridge,x4x}: Refactor MCHBARx_AND_OR

Add MCHBAR_AND_OR macro and refactor MCHBARx_AND_OR, MCHBARx_AND and MCHBARx_OR macros.

Change-Id: Ica23f58ffd79b17d4d68bd7a34a719691d654525
Signed-off-by: Elyes HAOUAS 
---
M src/northbridge/intel/haswell/haswell.h
M src/northbridge/intel/ironlake/ironlake.h
M src/northbridge/intel/pineview/pineview.h
M src/northbridge/intel/sandybridge/sandybridge.h
M src/northbridge/intel/x4x/x4x.h
5 files changed, 91 insertions(+), 63 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/45487/2

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45487 )

Change subject: nb/intel/{haswell,ironlake,pineview,sandybridge,x4x}: Refactor MCHBARx_AND_OR
......................................................................

Patch Set 2: Code-Review-1

still getting the same error as the old code

error: overflow in conversion from 'int' to 'u8' {aka 'volatile unsigned char'} changes value from '(int)*4275126567 & -256 | 64' to '64' [-Werror=overflow]

The old MCHBARx_AND_OR and new one are definitely a very bad idea!

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45487 )

Change subject: nb/intel/{haswell,ironlake,pineview,sandybridge,x4x}: Refactor MCHBARx_AND_OR
......................................................................

Patch Set 2:
> Patch Set 2: Code-Review-1
>
> still getting the same error as the old code
>
> error: overflow in conversion from 'int' to 'u8' {aka 'volatile unsigned char'} changes value from '(int)*4275126567 & -256 | 64' to '64' [-Werror=overflow]
>
> The old MCHBARx_AND_OR and new one are definitely a very bad idea!

Overflow error fixed here: https://review.coreboot.org/c/coreboot/+/45517

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/45487 )

Change subject: nb/intel/{haswell,ironlake,pineview,sandybridge,x4x}: Refactor MCHBARx_AND_OR
......................................................................

Abandoned

See 45517