Change in ...coreboot[master]: nb/intel/x4x/rcven.c: Remove variable set but not used

[HAOUAS Elyes (Code Review)]

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

Change subject: nb/intel/x4x/rcven.c: Remove variable set but not used
......................................................................

nb/intel/x4x/rcven.c: Remove variable set but not used

Change-Id: I13d6593e283f0a9e6603e19ccfda116f3b145e52
Signed-off-by: Elyes HAOUAS 
---
M src/northbridge/intel/x4x/rcven.c
1 file changed, 2 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/32948/1

diff --git a/src/northbridge/intel/x4x/rcven.c b/src/northbridge/intel/x4x/rcven.c
index 7c49838..36a6ebd 100644
--- a/src/northbridge/intel/x4x/rcven.c
+++ b/src/northbridge/intel/x4x/rcven.c
@@ -41,7 +41,6 @@

static u8 sampledqs(u32 addr, u8 lane, u8 channel)
 {
-	volatile u32 strobe;
    u32 sample_offset = 0x400 * channel + 0x561 + lane * 4;

/* Reset the DQS probe */
@@ -50,7 +49,8 @@
    MCHBAR8(RESET_CNTL(channel)) |= 0x2;
    udelay(2);
    barrier();
-	strobe = read32((u32 *)addr);
+	/* Read strobe */
+	read32((u32 *)addr);
    barrier();
    return (MCHBAR8(sample_offset) >> 6) & 1;
 }

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32948 )

Change subject: nb/intel/x4x/rcven.c: Remove variable set but not used
......................................................................

Patch Set 1: Code-Review-1

(1 comment)

The resulting assembly is definitely not the same... You'd want to test this on hardware

https://review.coreboot.org/#/c/32948/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32948/1//COMMIT_MSG@7 
PS1, Line 7: Remove variable set but not used
The whole purpose of this variable is to sample the strobe so it is normal that it does not get used, hence the use of the volatile keyword. Also the resulting assembly is different with this change.

[HAOUAS Elyes (Code Review)]

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

Change subject: nb/intel/x4x/rcven.c: Remove variable set but not used
......................................................................

Abandoned

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32948 )

Change subject: nb/intel/x4x/rcven.c: Remove variable set but not used
......................................................................

Patch Set 1:

(1 comment)

Elyes, wait for others to comment, but I think you can restore this.

https://review.coreboot.org/#/c/32948/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32948/1//COMMIT_MSG@7 
PS1, Line 7: Remove variable set but not used
> The whole purpose of this variable is to sample the strobe so it is normal that it does not get used […]

The code used read32() which alone would prevent compiler from optimizing out the MMIO read.

before:

47:	0f ae f0             	mfence 
  4a:	8b 07                	mov    (%edi),%eax
  4c:	89 44 24 1c          	mov    %eax,0x1c(%esp)
  50:	0f ae f0             	mfence

after:

47:	0f ae f0             	mfence 
  4a:	8b 07                	mov    (%edi),%eax
  4c:	0f ae f0             	mfence

I don't see why we would ever need to declare a variable stored on the stack as volatile like the code currently does. I think this fix was correct.

[HAOUAS Elyes (Code Review)]

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

Change subject: nb/intel/x4x/rcven.c: Remove variable set but not used
......................................................................

Restored

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32948 )

Change subject: nb/intel/x4x/rcven.c: Remove variable set but not used
......................................................................

Patch Set 1: -Code-Review

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32948 )

Change subject: nb/intel/x4x/rcven.c: Remove variable set but not used
......................................................................

Patch Set 1: Code-Review+1

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32948 )

Change subject: nb/intel/x4x/rcven.c: Remove variable set but not used
......................................................................

Patch Set 3: Code-Review+2

[Nico Huber (Code Review)]

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32948 )

Change subject: nb/intel/x4x/rcven.c: Remove variable set but not used
......................................................................

nb/intel/x4x/rcven.c: Remove variable set but not used

Change-Id: I13d6593e283f0a9e6603e19ccfda116f3b145e52
Signed-off-by: Elyes HAOUAS 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32948
Reviewed-by: Felix Held 
Reviewed-by: Kyösti Mälkki 
Tested-by: build bot (Jenkins) 
---
M src/northbridge/intel/x4x/rcven.c
1 file changed, 2 insertions(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Kyösti Mälkki: Looks good to me, but someone else must approve
  Felix Held: Looks good to me, approved

diff --git a/src/northbridge/intel/x4x/rcven.c b/src/northbridge/intel/x4x/rcven.c
index 7c49838..36a6ebd 100644
--- a/src/northbridge/intel/x4x/rcven.c
+++ b/src/northbridge/intel/x4x/rcven.c
@@ -41,7 +41,6 @@

static u8 sampledqs(u32 addr, u8 lane, u8 channel)
 {
-	volatile u32 strobe;
    u32 sample_offset = 0x400 * channel + 0x561 + lane * 4;

/* Reset the DQS probe */
@@ -50,7 +49,8 @@
    MCHBAR8(RESET_CNTL(channel)) |= 0x2;
    udelay(2);
    barrier();
-	strobe = read32((u32 *)addr);
+	/* Read strobe */
+	read32((u32 *)addr);
    barrier();
    return (MCHBAR8(sample_offset) >> 6) & 1;
 }