Change in coreboot[master]: nb/i945: Reorder sdram_setup_processor_side()

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

HAOUAS Elyes (Code Review)

14 Apr 2020 14 Apr '20

4:29 p.m.

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

Change subject: nb/i945: Reorder sdram_setup_processor_side() ......................................................................

nb/i945: Reorder sdram_setup_processor_side()

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

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/40368/1

diff --git a/src/northbridge/intel/i945/raminit.c b/src/northbridge/intel/i945/raminit.c index 329914e..dad1cb2 100644 --- a/src/northbridge/intel/i945/raminit.c +++ b/src/northbridge/intel/i945/raminit.c @@ -2714,13 +2714,12 @@

static void sdram_setup_processor_side(void) { - if (i945_silicon_revision() == 0) + if (i945_silicon_revision() == 0) { MCHBAR32(FSBPMC3) |= (1 << 2); + MCHBAR32(SLPCTL) |= (1 << 8); + }

MCHBAR8(0xb00) |= 1; - - if (i945_silicon_revision() == 0) - MCHBAR32(SLPCTL) |= (1 << 8); }

/**

Attachments:

attachment.htm (text/html — 2.8 KB)

Show replies by date

Arthur Heymans (Code Review)

16 Apr 16 Apr

12:01 p.m.

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

Change subject: nb/i945: Reorder sdram_setup_processor_side() ......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40368/3//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/40368/3//COMMIT_MSG@7 PS3, Line 7: Reorder why? You might want to use a variable instead of calling i945_silicon_revision() twice?

-- To view, visit https://review.coreboot.org/c/coreboot/+/40368 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ida21eb648f92ff0545dc0ce4da3aa9135b1260a9 Gerrit-Change-Number: 40368 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: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 16 Apr 2020 12:01:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

HAOUAS Elyes (Code Review)

12:41 p.m.

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

Change subject: nb/i945: Reorder sdram_setup_processor_side() ......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40368/3//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/40368/3//COMMIT_MSG@7 PS3, Line 7: Reorder

...

why? You might want to use a variable instead of calling i945_silicon_revision() twice?

I'm not using a variable. I just call i945_silicon_revision() one time and write 2 MCHBAR registers.

-- To view, visit https://review.coreboot.org/c/coreboot/+/40368 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ida21eb648f92ff0545dc0ce4da3aa9135b1260a9 Gerrit-Change-Number: 40368 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: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 16 Apr 2020 12:41:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Arthur Heymans arthur@aheymans.xyz Gerrit-MessageType: comment

Angel Pons (Code Review)

1:17 p.m.

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

Change subject: nb/i945: Reorder sdram_setup_processor_side() ......................................................................

Patch Set 3: Code-Review-1

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/40368/3/src/northbridge/intel/i945/... PS3, Line 2717: if (i945_silicon_revision() == 0) { Changing the order of undocumented register writes is not a good idea. What if there's an electrical dependency between these?

-- To view, visit https://review.coreboot.org/c/coreboot/+/40368 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ida21eb648f92ff0545dc0ce4da3aa9135b1260a9 Gerrit-Change-Number: 40368 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: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 16 Apr 2020 13:17:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

HAOUAS Elyes (Code Review)

2:33 p.m.

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

Change subject: nb/i945: Reorder sdram_setup_processor_side() ......................................................................

Patch Set 3:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/40368/3/src/northbridge/intel/i945/... PS3, Line 2717: if (i945_silicon_revision() == 0) {

...

Changing the order of undocumented register writes is not a good idea. […]

I've checked, apple macbook21 did the same on vendore rom.

I don't have that hardware to test, and I'm fine to drop current changes

-- To view, visit https://review.coreboot.org/c/coreboot/+/40368 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ida21eb648f92ff0545dc0ce4da3aa9135b1260a9 Gerrit-Change-Number: 40368 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: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 16 Apr 2020 14:33:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Gerrit-MessageType: comment

HAOUAS Elyes (Code Review)

2:33 p.m.

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

Change subject: nb/i945: Reorder sdram_setup_processor_side() ......................................................................

Abandoned

if (i945_silicon_revision() == 0) ....

-- To view, visit https://review.coreboot.org/c/coreboot/+/40368 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ida21eb648f92ff0545dc0ce4da3aa9135b1260a9 Gerrit-Change-Number: 40368 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: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: abandon

1626

days inactive

1628

days old

coreboot-gerrit@coreboot.org

5 comments

3 participants

tags (0)

participants (3)

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