Change in coreboot[master]: nb/i945: Drop unneeded loop

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

HAOUAS Elyes (Code Review)

14 Apr 2020 14 Apr '20

6:33 p.m.

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

Change subject: nb/i945: Drop unneeded loop ......................................................................

nb/i945: Drop unneeded loop

At sdram_power_management() function, MCHBAR32(UPMC3) bit 16 is already set.

Change-Id: Iefa53ceaa4fb9dd7868ea59a21d9bd11f3bf27b9 Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/northbridge/intel/i945/raminit.c 1 file changed, 0 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/40369/1

diff --git a/src/northbridge/intel/i945/raminit.c b/src/northbridge/intel/i945/raminit.c index dad1cb2..4353a9c 100644 --- a/src/northbridge/intel/i945/raminit.c +++ b/src/northbridge/intel/i945/raminit.c @@ -2147,7 +2147,6 @@ u16 reg16; u32 reg32; int integrated_graphics = pci_read_config16(PCI_DEV(0, 0x00, 0), GGC) & 2; - int i;

reg32 = MCHBAR32(C0DRT2); reg32 &= 0xffffff00; @@ -2193,11 +2192,6 @@

MCHBAR32(UPMC3) = 0x000f06ff;

- for (i = 0; i < 5; i++) { - MCHBAR32(UPMC3) &= ~(1 << 16); - MCHBAR32(UPMC3) |= (1 << 16); - } - MCHBAR32(GIPMC1) = 0x8000000c;

reg16 = MCHBAR16(CPCTL);

Attachments:

attachment.htm (text/html — 2.9 KB)

Show replies by date

Angel Pons (Code Review)

16 Apr 16 Apr

3:22 p.m.

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

Change subject: nb/i945: Drop unneeded loop ......................................................................

Patch Set 3: Code-Review-1

Removing code that you don't fully understand is a surefire way to break things. Don't forget that some registers directly control hardware states, and some other registers (e.g. SCIP on ICH7) can even be changed by the hardware itself!

-- To view, visit https://review.coreboot.org/c/coreboot/+/40369 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iefa53ceaa4fb9dd7868ea59a21d9bd11f3bf27b9 Gerrit-Change-Number: 40369 Gerrit-PatchSet: 3 Gerrit-Owner: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 16 Apr 2020 13:22:51 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

HAOUAS Elyes (Code Review)

4:38 p.m.

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

Change subject: nb/i945: Drop unneeded loop ......................................................................

Patch Set 3:

...

Patch Set 3: Code-Review-1

Removing code that you don't fully understand is a surefire way to break things. Don't forget that some registers directly control hardware states, and some other registers (e.g. SCIP on ICH7) can even be changed by the hardware itself!

Indeed, I don't understand...! I've asked to test on IRC, isn't it ? I've tested current on my board (desktop version) and I've checked, apple/macbook21 vendore did the same.

So please teach me why we have this loop ? Thx

-- To view, visit https://review.coreboot.org/c/coreboot/+/40369 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iefa53ceaa4fb9dd7868ea59a21d9bd11f3bf27b9 Gerrit-Change-Number: 40369 Gerrit-PatchSet: 3 Gerrit-Owner: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 16 Apr 2020 14:38:46 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

HAOUAS Elyes (Code Review)

4:51 p.m.

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

Change subject: nb/i945: Drop unneeded loop ......................................................................

Abandoned

Angel Pons (Code Review)

4:52 p.m.

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

Change subject: nb/i945: Drop unneeded loop ......................................................................

Patch Set 3:

...

Patch Set 3:

...
Patch Set 3: Code-Review-1

Removing code that you don't fully understand is a surefire way to break things. Don't forget that some registers directly control hardware states, and some other registers (e.g. SCIP on ICH7) can even be changed by the hardware itself!

Indeed, I don't understand...! I've asked to test on IRC, isn't it ? I've tested current on my board (desktop version) and I've checked, apple/macbook21 vendore did the same.

So please teach me why we have this loop ? Thx

I've just checked and this hasn't been touched since it was added. It would be nice to know why, but I guess we will never know...

-- To view, visit https://review.coreboot.org/c/coreboot/+/40369 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iefa53ceaa4fb9dd7868ea59a21d9bd11f3bf27b9 Gerrit-Change-Number: 40369 Gerrit-PatchSet: 3 Gerrit-Owner: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 16 Apr 2020 14:52:09 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

HAOUAS Elyes (Code Review)

13 Feb 13 Feb

8:46 a.m.

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

Change subject: nb/i945: Drop unneeded loop ......................................................................

Restored

HAOUAS Elyes (Code Review)

8:50 a.m.

Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,

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

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

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

Change subject: nb/i945: Drop unneeded loop ......................................................................

nb/i945: Drop unneeded loop

At sdram_power_management() function, MCHBAR32(UPMC3) bit 16 is already set.

Change-Id: Iefa53ceaa4fb9dd7868ea59a21d9bd11f3bf27b9 Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/northbridge/intel/i945/raminit.c 1 file changed, 0 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/40369/4

-- To view, visit https://review.coreboot.org/c/coreboot/+/40369 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iefa53ceaa4fb9dd7868ea59a21d9bd11f3bf27b9 Gerrit-Change-Number: 40369 Gerrit-PatchSet: 4 Gerrit-Owner: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

HAOUAS Elyes (Code Review)

9:49 a.m.

Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,

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

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

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

Change subject: nb/i945: Drop unneeded loop ......................................................................

nb/i945: Drop unneeded loop

At sdram_power_management() function, MCHBAR32(UPMC3) bit 16 is already set.

Change-Id: Iefa53ceaa4fb9dd7868ea59a21d9bd11f3bf27b9 Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/northbridge/intel/i945/raminit.c 1 file changed, 0 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/40369/5

-- To view, visit https://review.coreboot.org/c/coreboot/+/40369 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iefa53ceaa4fb9dd7868ea59a21d9bd11f3bf27b9 Gerrit-Change-Number: 40369 Gerrit-PatchSet: 5 Gerrit-Owner: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

Angel Pons (Code Review)

11:42 a.m.

Attention is currently required from: HAOUAS Elyes. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40369 )

Change subject: nb/i945: Drop unneeded loop ......................................................................

Patch Set 5:

(1 comment)

File src/northbridge/intel/i945/raminit.c:

https://review.coreboot.org/c/coreboot/+/40369/comment/7550f80a_3d6a5e71 PS5, Line 2177: MCHBAR32(UPMC3) |= (1 << 16); This toggles the bit back and forth. Why? I have no clue.

-- To view, visit https://review.coreboot.org/c/coreboot/+/40369 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iefa53ceaa4fb9dd7868ea59a21d9bd11f3bf27b9 Gerrit-Change-Number: 40369 Gerrit-PatchSet: 5 Gerrit-Owner: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Attention: HAOUAS Elyes ehaouas@noos.fr Gerrit-Comment-Date: Sat, 13 Feb 2021 10:42:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

HAOUAS Elyes (Code Review)

12:27 p.m.

Attention is currently required from: Angel Pons. HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40369 )

Change subject: nb/i945: Drop unneeded loop ......................................................................

Patch Set 5:

(1 comment)

File src/northbridge/intel/i945/raminit.c:

https://review.coreboot.org/c/coreboot/+/40369/comment/7daa55ec_7aff5c29 PS5, Line 2177: MCHBAR32(UPMC3) |= (1 << 16);

...

This toggles the bit back and forth. Why? I have no clue.

this loop is definitely useless

-- To view, visit https://review.coreboot.org/c/coreboot/+/40369 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iefa53ceaa4fb9dd7868ea59a21d9bd11f3bf27b9 Gerrit-Change-Number: 40369 Gerrit-PatchSet: 5 Gerrit-Owner: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Attention: Angel Pons th3fanbus@gmail.com Gerrit-Comment-Date: Sat, 13 Feb 2021 11:27:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Gerrit-MessageType: comment

HAOUAS Elyes (Code Review)

1:22 p.m.

Hello build bot (Jenkins), Patrick Rudolph,

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

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

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

Change subject: nb/i945: Drop unneeded loop ......................................................................

nb/i945: Drop unneeded loop

At sdram_power_management() function, MCHBAR32(UPMC3) bit 16 is already set.

Change-Id: Iefa53ceaa4fb9dd7868ea59a21d9bd11f3bf27b9 Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/northbridge/intel/i945/raminit.c 1 file changed, 0 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/40369/6

HAOUAS Elyes (Code Review)

17 Feb 17 Feb

7:26 p.m.

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

Change subject: nb/i945: Drop unneeded loop ......................................................................

Abandoned

1165

days inactive

1474

days old

coreboot-gerrit@coreboot.org

11 comments

2 participants

tags (0)

participants (2)

Angel Pons (Code Review)
HAOUAS Elyes (Code Review)