Change in coreboot[master]: nb/i945: Rewrite peg_bits for MCHBAR16(UPMC1)

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

HAOUAS Elyes (Code Review)

14 Apr 2020 14 Apr '20

9:38 a.m.

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

Change subject: nb/i945: Rewrite peg_bits for MCHBAR16(UPMC1) ......................................................................

nb/i945: Rewrite peg_bits for MCHBAR16(UPMC1)

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

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/40370/1

diff --git a/src/northbridge/intel/i945/raminit.c b/src/northbridge/intel/i945/raminit.c index 4353a9c..bb886d4 100644 --- a/src/northbridge/intel/i945/raminit.c +++ b/src/northbridge/intel/i945/raminit.c @@ -2171,18 +2171,14 @@ MCHBAR32(C1DRC1) = reg32;

if (CONFIG(NORTHBRIDGE_INTEL_SUBTYPE_I945GM)) { - if (i945_silicon_revision() > 1) { - /* FIXME bits 5 and 0 only if PCIe graphics is disabled */ - u16 peg_bits = (1 << 5) | (1 << 0); - + /* FIXME bits 5 and 0 only if PCIe graphics is disabled */ + u16 peg_bits = MCHBAR16(UPMC1); + peg_bits &= ((1 << 5) | (1 << 0)); + if (i945_silicon_revision() > 1) MCHBAR16(UPMC1) = 0x1010 | peg_bits; - } else { - /* FIXME bits 5 and 0 only if PCIe graphics is disabled */ - u16 peg_bits = (1 << 5) | (1 << 0); - + else /* Rev 0 and 1 */ MCHBAR16(UPMC1) = 0x0010 | peg_bits; - } }

reg16 = MCHBAR16(UPMC2);

Attachments:

attachment.htm (text/html — 3.5 KB)

Show replies by date

Angel Pons (Code Review)

16 Apr 16 Apr

6:42 a.m.

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

Change subject: nb/i945: Rewrite peg_bits for MCHBAR16(UPMC1) ......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40370/3/src/northbridge/intel/i945/... File src/northbridge/intel/i945/raminit.c:

https://review.coreboot.org/c/coreboot/+/40370/3/src/northbridge/intel/i945/... PS3, Line 2174: /* FIXME bits 5 and 0 only if PCIe graphics is disabled */ Why would you need to risk breaking things by changing this???

This is just wrong. The entire 16 bits of UPMC1 were updated before, and that is most likely intentional. Unless you have *reliable* documentation (not a mere datasheet where half the things are missing and the other half are reserved) that shows this is wrong, *and* that doing it this way fixes something without breaking anything else, then please don't change things.

In any case, you can still make things simpler without affecting the behavior:

u16 peg_bits = (1 << 4);

/* FIXME bits 5 and 0 only if PCIe graphics is disabled */ peg_bits |= (1 << 5) | (1 << 0);

if (i945_silicon_revision() > 1) peg_bits |= (1 << 12);

MCHBAR16(UPMC1) = peg_bits;

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

HAOUAS Elyes (Code Review)

7:47 a.m.

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

Change subject: nb/i945: Rewrite peg_bits for MCHBAR16(UPMC1) ......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40370/3/src/northbridge/intel/i945/... File src/northbridge/intel/i945/raminit.c:

https://review.coreboot.org/c/coreboot/+/40370/3/src/northbridge/intel/i945/... PS3, Line 2174: /* FIXME bits 5 and 0 only if PCIe graphics is disabled */

...

Why would you need to risk breaking things by changing this??? […]

why will this break something???? We all know that there is no documentation .... and what we used to read registry on vendor (see util/inetl* ) and some reverse engineering. so I did that and found it written that way.

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

HAOUAS Elyes (Code Review)

7:51 a.m.

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

Change subject: nb/i945: Rewrite peg_bits for MCHBAR16(UPMC1) ......................................................................

Abandoned

HAOUAS Elyes (Code Review)

13 Feb 13 Feb

midnight

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

Change subject: nb/i945: Rewrite peg_bits for MCHBAR16(UPMC1) ......................................................................

Restored

HAOUAS Elyes (Code Review)

12:01 a.m.

New subject: Change in coreboot[master]: nb/i945/raminit: Simplify if statement blocks

Hello build bot (Jenkins), Patrick Rudolph,

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

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

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

Change subject: nb/i945/raminit: Simplify if statement blocks ......................................................................

nb/i945/raminit: Simplify if statement blocks

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

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/40370/4

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

Angel Pons (Code Review)

2:40 a.m.

New subject: Change in coreboot[master]: nb/i945/raminit: Simplify if statement blocks

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

Change subject: nb/i945/raminit: Simplify if statement blocks ......................................................................

Patch Set 5: Code-Review+1

-- To view, visit https://review.coreboot.org/c/coreboot/+/40370 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3c9e848077cdd13158dbfd19326b95b42fc1c1fb Gerrit-Change-Number: 40370 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:40:00 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

HAOUAS Elyes (Code Review)

3:29 a.m.

New subject: Change in coreboot[master]: nb/i945/raminit: Simplify if statement blocks

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

Change subject: nb/i945/raminit: Simplify if statement blocks ......................................................................

Patch Set 5:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/40370/comment/44abaa1f_88c44766 PS3, Line 2174: /* FIXME bits 5 and 0 only if PCIe graphics is disabled */

...

why will this break something???? […]

Ack

-- To view, visit https://review.coreboot.org/c/coreboot/+/40370 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3c9e848077cdd13158dbfd19326b95b42fc1c1fb Gerrit-Change-Number: 40370 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:29:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Comment-In-Reply-To: HAOUAS Elyes ehaouas@noos.fr Gerrit-MessageType: comment

HAOUAS Elyes (Code Review)

17 Feb 17 Feb

10:26 a.m.

New subject: Change in coreboot[master]: nb/i945/raminit: Simplify if statement blocks

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

Change subject: nb/i945/raminit: Simplify if statement blocks ......................................................................

Abandoned

-- To view, visit https://review.coreboot.org/c/coreboot/+/40370 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3c9e848077cdd13158dbfd19326b95b42fc1c1fb Gerrit-Change-Number: 40370 Gerrit-PatchSet: 6 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: abandon

1444

days inactive

1753

days old

coreboot-gerrit@coreboot.org

8 comments

2 participants

tags (0)

participants (2)

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