Change in coreboot[master]: nb/intel/ironlake: Drop casts from DEFAULT_{MCHBAR,DMIBAR}

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

Angel Pons (Code Review)

14 Sep 2020 14 Sep '20

7:43 p.m.

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45378 )

Change subject: nb/intel/ironlake: Drop casts from DEFAULT_{MCHBAR,DMIBAR} ......................................................................

nb/intel/ironlake: Drop casts from DEFAULT_{MCHBAR,DMIBAR}

This allows dropping some casts. Changes the binary, though.

Change-Id: Icff7919f7321a08338db2f0a765ebd605fd00ae2 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/ironlake/early_init.c M src/northbridge/intel/ironlake/ironlake.h 2 files changed, 2 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/45378/1

diff --git a/src/northbridge/intel/ironlake/early_init.c b/src/northbridge/intel/ironlake/early_init.c index fa89bd9..e0120fe 100644 --- a/src/northbridge/intel/ironlake/early_init.c +++ b/src/northbridge/intel/ironlake/early_init.c @@ -19,9 +19,9 @@ /* Set up all hardcoded northbridge BARs */ pci_write_config32(PCI_DEV(0, 0x00, 0), EPBAR, DEFAULT_EPBAR | 1); pci_write_config32(PCI_DEV(0, 0x00, 0), EPBAR + 4, 0); - pci_write_config32(PCI_DEV(0, 0x00, 0), MCHBAR, (uintptr_t)DEFAULT_MCHBAR | 1); + pci_write_config32(PCI_DEV(0, 0x00, 0), MCHBAR, DEFAULT_MCHBAR | 1); pci_write_config32(PCI_DEV(0, 0x00, 0), MCHBAR + 4, 0); - pci_write_config32(PCI_DEV(0, 0x00, 0), DMIBAR, (uintptr_t)DEFAULT_DMIBAR | 1); + pci_write_config32(PCI_DEV(0, 0x00, 0), DMIBAR, DEFAULT_DMIBAR | 1); pci_write_config32(PCI_DEV(0, 0x00, 0), DMIBAR + 4, 0);

/* Set C0000-FFFFF to access RAM on both reads and writes */ diff --git a/src/northbridge/intel/ironlake/ironlake.h b/src/northbridge/intel/ironlake/ironlake.h index aa9a8b7..9faa661 100644 --- a/src/northbridge/intel/ironlake/ironlake.h +++ b/src/northbridge/intel/ironlake/ironlake.h @@ -26,13 +26,8 @@ #define IRONLAKE_SERVER 2

/* Northbridge BARs */ -#ifndef __ACPI__ -#define DEFAULT_MCHBAR ((u8 *)0xfed10000) /* 16 KB */ -#define DEFAULT_DMIBAR ((u8 *)0xfed18000) /* 4 KB */ -#else #define DEFAULT_MCHBAR 0xfed10000 /* 16 KB */ #define DEFAULT_DMIBAR 0xfed18000 /* 4 KB */ -#endif #define DEFAULT_EPBAR 0xfed19000 /* 4 KB */

#define QUICKPATH_BUS 0xff

Attachments:

attachment.htm (text/html — 4.2 KB)

Show replies by date

Arthur Heymans (Code Review)

15 Sep 15 Sep

3:28 a.m.

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

Change subject: nb/intel/ironlake: Drop casts from DEFAULT_{MCHBAR,DMIBAR} ......................................................................

Patch Set 1:

There seem to be quite some differences in the generated raminit.o files so I think it's better to test this on hardware.

-- To view, visit https://review.coreboot.org/c/coreboot/+/45378 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icff7919f7321a08338db2f0a765ebd605fd00ae2 Gerrit-Change-Number: 45378 Gerrit-PatchSet: 1 Gerrit-Owner: 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-Comment-Date: Tue, 15 Sep 2020 06:28:28 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Paul Menzel (Code Review)

5:08 a.m.

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45378 )

Change subject: nb/intel/ironlake: Drop casts from DEFAULT_{MCHBAR,DMIBAR} ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45378/1//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/45378/1//COMMIT_MSG@10 PS1, Line 10: Why is the distinction between ACPI and not ACPI not needed anymore?

-- To view, visit https://review.coreboot.org/c/coreboot/+/45378 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icff7919f7321a08338db2f0a765ebd605fd00ae2 Gerrit-Change-Number: 45378 Gerrit-PatchSet: 1 Gerrit-Owner: 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: Tue, 15 Sep 2020 08:08:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Angel Pons (Code Review)

6:26 a.m.

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

Change subject: nb/intel/ironlake: Drop casts from DEFAULT_{MCHBAR,DMIBAR} ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45378/1//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/45378/1//COMMIT_MSG@10 PS1, Line 10:

...

Why is the distinction between ACPI and not ACPI not needed anymore?

The distinction is because ACPI doesn't understand C-style casts. However, these MMIO base addresses are used as pointers in C, so there was a cast for convenience.

I dropped the casts from the non-ACPI definitions for DEFAULT_MCHBAR and DEFAULT_DMIBAR. The end result is that both definitions are the same, so I collapsed them. The other casts that are dropped are the two uintptr_t casts in early_init.c (I think I ate a word, it should say `some other casts`). On Sandy Bridge, there were many more cases where casts could be dropped.

I'll try to add that `other` word to the commit message (or even reword it and mention early_init.c). However, I don't think it's useful to explain why the distinction is no longer necessary. I may add a sentence explaining why I want to remove the definition with casts (it's basically to not require any extra headers in memmap.h, added in a follow-up). And the reason I'm doing this is to uniformize old platforms.

-- To view, visit https://review.coreboot.org/c/coreboot/+/45378 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icff7919f7321a08338db2f0a765ebd605fd00ae2 Gerrit-Change-Number: 45378 Gerrit-PatchSet: 1 Gerrit-Owner: 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: Tue, 15 Sep 2020 09:26:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: comment

Angel Pons (Code Review)

7:14 a.m.

Hello build bot (Jenkins), Patrick Rudolph,

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

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

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

Change subject: nb/intel/ironlake: Drop casts from DEFAULT_{MCHBAR,DMIBAR} ......................................................................

nb/intel/ironlake: Drop casts from DEFAULT_{MCHBAR,DMIBAR}

There's no need to wrap these macros with casts. Removing them allows dropping more casts in `early_init.c`. Changes the binary, though.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/45378/2

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

Angel Pons (Code Review)

7:14 a.m.

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

Change subject: nb/intel/ironlake: Drop casts from DEFAULT_{MCHBAR,DMIBAR} ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45378/1//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/45378/1//COMMIT_MSG@10 PS1, Line 10:

...

The distinction is because ACPI doesn't understand C-style casts. […]

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/45378 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icff7919f7321a08338db2f0a765ebd605fd00ae2 Gerrit-Change-Number: 45378 Gerrit-PatchSet: 1 Gerrit-Owner: 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: Tue, 15 Sep 2020 10:14:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel paulepanter@users.sourceforge.net Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Gerrit-MessageType: comment

Paul Menzel (Code Review)

7:21 a.m.

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45378 )

Change subject: nb/intel/ironlake: Drop casts from DEFAULT_{MCHBAR,DMIBAR} ......................................................................

Patch Set 2: Code-Review+1

-- To view, visit https://review.coreboot.org/c/coreboot/+/45378 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icff7919f7321a08338db2f0a765ebd605fd00ae2 Gerrit-Change-Number: 45378 Gerrit-PatchSet: 2 Gerrit-Owner: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-Comment-Date: Tue, 15 Sep 2020 10:21:08 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Patrick Georgi (Code Review)

7 Dec 7 Dec

11:24 a.m.

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45378 )

Change subject: nb/intel/ironlake: Drop casts from DEFAULT_{MCHBAR,DMIBAR} ......................................................................

Patch Set 3:

...

Patch Set 1:

There seem to be quite some differences in the generated raminit.o files so I think it's better to test this on hardware.

It might end up being interpreted as an offset into a u32 array or something like that (which means that the address value is multiplied by 4), so yes, any differences need care

-- To view, visit https://review.coreboot.org/c/coreboot/+/45378 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icff7919f7321a08338db2f0a765ebd605fd00ae2 Gerrit-Change-Number: 45378 Gerrit-PatchSet: 3 Gerrit-Owner: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Patrick Georgi pgeorgi@google.com Gerrit-Comment-Date: Mon, 07 Dec 2020 14:24:14 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Patrick Georgi (Code Review)

11:41 a.m.

Patrick Georgi has uploaded a new patch set (#4) to the change originally created by Angel Pons. ( https://review.coreboot.org/c/coreboot/+/45378 )

Change subject: nb/intel/ironlake: Drop casts from DEFAULT_{MCHBAR,DMIBAR} ......................................................................

nb/intel/ironlake: Drop casts from DEFAULT_{MCHBAR,DMIBAR}

There's no need to wrap these macros with casts. Removing them allows dropping more casts in `early_init.c`.

To avoid binary changes the casts are put into the {MCH,DMI,EP}BAR{8,16,32} macros instead where they are needed to reach the right memory locations.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/45378/4

-- To view, visit https://review.coreboot.org/c/coreboot/+/45378 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icff7919f7321a08338db2f0a765ebd605fd00ae2 Gerrit-Change-Number: 45378 Gerrit-PatchSet: 4 Gerrit-Owner: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-MessageType: newpatchset

Angel Pons (Code Review)

11:43 a.m.

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

Change subject: nb/intel/ironlake: Drop casts from DEFAULT_{MCHBAR,DMIBAR} ......................................................................

Patch Set 4: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/45378 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icff7919f7321a08338db2f0a765ebd605fd00ae2 Gerrit-Change-Number: 45378 Gerrit-PatchSet: 4 Gerrit-Owner: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-Comment-Date: Mon, 07 Dec 2020 14:43:34 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Patrick Georgi (Code Review)

12:24 p.m.

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45378 )

Change subject: nb/intel/ironlake: Drop casts from DEFAULT_{MCHBAR,DMIBAR} ......................................................................

nb/intel/ironlake: Drop casts from DEFAULT_{MCHBAR,DMIBAR}

There's no need to wrap these macros with casts. Removing them allows dropping more casts in `early_init.c`.

To avoid binary changes the casts are put into the {MCH,DMI,EP}BAR{8,16,32} macros instead where they are needed to reach the right memory locations.

Change-Id: Icff7919f7321a08338db2f0a765ebd605fd00ae2 Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/45378 Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/northbridge/intel/ironlake/early_init.c M src/northbridge/intel/ironlake/ironlake.h 2 files changed, 11 insertions(+), 16 deletions(-)

Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved

/* Set C0000-FFFFF to access RAM on both reads and writes */ diff --git a/src/northbridge/intel/ironlake/ironlake.h b/src/northbridge/intel/ironlake/ironlake.h index 86c6054..8abc3fc 100644 --- a/src/northbridge/intel/ironlake/ironlake.h +++ b/src/northbridge/intel/ironlake/ironlake.h @@ -25,13 +25,8 @@ #define IRONLAKE_SERVER 2

#define QUICKPATH_BUS 0xff @@ -100,9 +95,9 @@ * 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(x) (*((volatile u8 *)((u8 *)DEFAULT_MCHBAR + (x)))) +#define MCHBAR16(x) (*((volatile u16 *)((u8 *)DEFAULT_MCHBAR + (x)))) +#define MCHBAR32(x) (*((volatile u32 *)((u8 *)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)) @@ -116,9 +111,9 @@ * EPBAR - Egress Port Root Complex Register Block */

-#define EPBAR8(x) (*((volatile u8 *)(DEFAULT_EPBAR + (x)))) -#define EPBAR16(x) (*((volatile u16 *)(DEFAULT_EPBAR + (x)))) -#define EPBAR32(x) (*((volatile u32 *)(DEFAULT_EPBAR + (x)))) +#define EPBAR8(x) (*((volatile u8 *)((u8 *)DEFAULT_EPBAR + (x)))) +#define EPBAR16(x) (*((volatile u16 *)((u8 *)DEFAULT_EPBAR + (x)))) +#define EPBAR32(x) (*((volatile u32 *)((u8 *)DEFAULT_EPBAR + (x))))

#include "registers/epbar.h"

@@ -126,9 +121,9 @@ * DMIBAR */

-#define DMIBAR8(x) (*((volatile u8 *)(DEFAULT_DMIBAR + (x)))) -#define DMIBAR16(x) (*((volatile u16 *)(DEFAULT_DMIBAR + (x)))) -#define DMIBAR32(x) (*((volatile u32 *)(DEFAULT_DMIBAR + (x)))) +#define DMIBAR8(x) (*((volatile u8 *)((u8 *)DEFAULT_DMIBAR + (x)))) +#define DMIBAR16(x) (*((volatile u16 *)((u8 *)DEFAULT_DMIBAR + (x)))) +#define DMIBAR32(x) (*((volatile u32 *)((u8 *)DEFAULT_DMIBAR + (x))))

#include "registers/dmibar.h"

-- To view, visit https://review.coreboot.org/c/coreboot/+/45378 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icff7919f7321a08338db2f0a765ebd605fd00ae2 Gerrit-Change-Number: 45378 Gerrit-PatchSet: 5 Gerrit-Owner: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-MessageType: merged

1304

days inactive

1388

days old

coreboot-gerrit@coreboot.org

10 comments

4 participants

tags (0)

participants (4)

Angel Pons (Code Review)
Arthur Heymans (Code Review)
Patrick Georgi (Code Review)
Paul Menzel (Code Review)