Change in coreboot[master]: mb/facebook/fbg1701: Remove ONBOARD_MICRON_MEM

[Frans Hendriks (Code Review)]

Frans Hendriks has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35742 )

Change subject: mb/facebook/fbg1701: Remove ONBOARD_MICRON_MEM
......................................................................

mb/facebook/fbg1701: Remove ONBOARD_MICRON_MEM

Config ONBOARD_MICRON_MEM and ONBOARD_SAMSUNG_MEM are available.
These configs are used to determine if Samsung or Micron onboard memory is
assembled. This can not detected run-time.

One CONFIG value will be enough to determine if Samsung memory is assembled.
Only oldest HW revision contains Samsung module, so set CONFIG_ONBOARD_SAMSUNG memory
to default No.

BUG=N/A
TEST=Boot and verified on Facebook FBG-1701

Change-Id: Id65e92bd4b8d4fe3a6b87dec9bf77e3a62e1be96
Signed-off-by: Frans Hendriks 
---
M src/mainboard/facebook/fbg1701/Kconfig
M src/mainboard/facebook/fbg1701/romstage.c
2 files changed, 2 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/35742/1

diff --git a/src/mainboard/facebook/fbg1701/Kconfig b/src/mainboard/facebook/fbg1701/Kconfig
index ce90758..066008e 100644
--- a/src/mainboard/facebook/fbg1701/Kconfig
+++ b/src/mainboard/facebook/fbg1701/Kconfig
@@ -31,21 +31,12 @@
    select INTEL_GMA_HAVE_VBT
    select GENERIC_SPD_BIN

-choice
-	prompt "Onboard memory manufacturer"
-	default ONBOARD_MICRON_MEM
-
 config ONBOARD_SAMSUNG_MEM
    bool "Samsung"
+	default n
    help
      Samsung K4B8G1646D memory

-config ONBOARD_MICRON_MEM
-	bool "Micron"
-	help
-	  Micron MT41K512M16HA memory
-endchoice
-
 config MAINBOARD_DIR
    string
    default facebook/fbg1701
diff --git a/src/mainboard/facebook/fbg1701/romstage.c b/src/mainboard/facebook/fbg1701/romstage.c
index e2e37d6..7b68914 100644
--- a/src/mainboard/facebook/fbg1701/romstage.c
+++ b/src/mainboard/facebook/fbg1701/romstage.c
@@ -31,7 +31,7 @@
    struct region_device spd_rdev;
    u8 spd_index = 0;

-	if (CONFIG(ONBOARD_MICRON_MEM))
+	if (!CONFIG(ONBOARD_SAMSUNG_MEM))
    	spd_index = 1;
    if (get_spd_cbfs_rdev(&spd_rdev, spd_index) < 0)
    	die("spd.bin not found\n");

[Paul Menzel (Code Review)]

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

Change subject: mb/facebook/fbg1701: Remove ONBOARD_MICRON_MEM
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/35742/1//COMMIT_MSG@9 
PS1, Line 9: Config ONBOARD_MICRON_MEM and ONBOARD_SAMSUNG_MEM are available.
           : These configs are used to determine if Samsung or Micron onboard memory is
           : assembled. This can not detected run-time.
           : 
           : One CONFIG value will be enough to determine if Samsung memory is assembled.
           : Only oldest HW revision contains Samsung module, so set CONFIG_ONBOARD_SAMSUNG memory
           : to default No.
Please adhere to the text width of 75 characters.

[Patrick Rudolph (Code Review)]

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

Change subject: mb/facebook/fbg1701: Remove ONBOARD_MICRON_MEM
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35742/1/src/mainboard/facebook/fbg1... 
File src/mainboard/facebook/fbg1701/Kconfig:

https://review.coreboot.org/c/coreboot/+/35742/1/src/mainboard/facebook/fbg1... 
PS1, Line 35: 	bool "Samsung"
find better Kconfig text. something like "Onboard memory manufacturer Samsung"

[Frans Hendriks (Code Review)]

Hello Patrick Rudolph, Felix Held, Arthur Heymans, Patrick Rudolph, Lance Zhao, build bot (Jenkins),

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

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

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

Change subject: mb/facebook/fbg1701: Remove ONBOARD_MICRON_MEM
......................................................................

mb/facebook/fbg1701: Remove ONBOARD_MICRON_MEM

Config ONBOARD_MICRON_MEM and ONBOARD_SAMSUNG_MEM are available.
These configs are used to determine if Samsung or Micron onboard memory is
assembled. This can not detected run-time.

One CONFIG value will be enough to determine if Samsung memory is
assembled.
Only oldest HW revision contains Samsung module, so set
CONFIG_ONBOARD_SAMSUNG memory to default No.

BUG=N/A
TEST=Boot and verified on Facebook FBG-1701

Change-Id: Id65e92bd4b8d4fe3a6b87dec9bf77e3a62e1be96
Signed-off-by: Frans Hendriks 
---
M src/mainboard/facebook/fbg1701/Kconfig
M src/mainboard/facebook/fbg1701/romstage.c
2 files changed, 2 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/35742/2

[Frans Hendriks (Code Review)]

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35742 )

Change subject: mb/facebook/fbg1701: Remove ONBOARD_MICRON_MEM
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/35742/1//COMMIT_MSG@9 
PS1, Line 9: Config ONBOARD_MICRON_MEM and ONBOARD_SAMSUNG_MEM are available.
           : These configs are used to determine if Samsung or Micron onboard memory is
           : assembled. This can not detected run-time.
           : 
           : One CONFIG value will be enough to determine if Samsung memory is assembled.
           : Only oldest HW revision contains Samsung module, so set CONFIG_ONBOARD_SAMSUNG memory
           : to default No.
> Please adhere to the text width of 75 characters.

Done

[Frans Hendriks (Code Review)]

Hello Patrick Rudolph, Felix Held, Arthur Heymans, Patrick Rudolph, Lance Zhao, build bot (Jenkins),

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

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

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

Change subject: mb/facebook/fbg1701: Remove ONBOARD_MICRON_MEM
......................................................................

mb/facebook/fbg1701: Remove ONBOARD_MICRON_MEM

Config ONBOARD_MICRON_MEM and ONBOARD_SAMSUNG_MEM are available.
These configs are used to determine if Samsung or Micron onboard memory is
assembled. This can not detected run-time.

One CONFIG value will be enough to determine if Samsung memory is
assembled.
Only oldest HW revision contains Samsung module, so set
CONFIG_ONBOARD_SAMSUNG memory to default No.

BUG=N/A
TEST=Boot and verified on Facebook FBG-1701

Change-Id: Id65e92bd4b8d4fe3a6b87dec9bf77e3a62e1be96
Signed-off-by: Frans Hendriks 
---
M src/mainboard/facebook/fbg1701/Kconfig
M src/mainboard/facebook/fbg1701/romstage.c
2 files changed, 3 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/35742/3

[Frans Hendriks (Code Review)]

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35742 )

Change subject: mb/facebook/fbg1701: Remove ONBOARD_MICRON_MEM
......................................................................

Patch Set 3:

(1 comment)

Upload new patchset

https://review.coreboot.org/c/coreboot/+/35742/1/src/mainboard/facebook/fbg1... 
File src/mainboard/facebook/fbg1701/Kconfig:

https://review.coreboot.org/c/coreboot/+/35742/1/src/mainboard/facebook/fbg1... 
PS1, Line 35: 	bool "Samsung"
> find better Kconfig text. […]

Done

[Patrick Rudolph (Code Review)]

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

Change subject: mb/facebook/fbg1701: Remove ONBOARD_MICRON_MEM
......................................................................

Patch Set 3: Code-Review+2

[Paul Menzel (Code Review)]

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

Change subject: mb/facebook/fbg1701: Remove ONBOARD_MICRON_MEM
......................................................................

Patch Set 3:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/35742/3//COMMIT_MSG@9 
PS3, Line 9: Config ONBOARD_MICRON_MEM and ONBOARD_SAMSUNG_MEM are available.
           : These configs are used to determine if Samsung or Micron onboard memory is
           : assembled. This can not detected run-time.
           : 
           : One CONFIG value will be enough to determine if Samsung memory is
           : assembled.
           : Only oldest HW revision contains Samsung module, so set
           : CONFIG_ONBOARD_SAMSUNG memory to default No.
If I run it through Vim, the text looks like below.

```
Config ONBOARD_MICRON_MEM and ONBOARD_SAMSUNG_MEM are available.  These
configs are used to determine if Samsung or Micron onboard memory is
assembled. This can not detected run-time.

One CONFIG value will be enough to determine if Samsung memory is
assembled.  Only oldest HW revision contains Samsung module, so set
CONFIG_ONBOARD_SAMSUNG memory to default No.
```

[Frans Hendriks (Code Review)]

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35742 )

Change subject: mb/facebook/fbg1701: Remove ONBOARD_MICRON_MEM
......................................................................

Patch Set 3:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/35742/3//COMMIT_MSG@9 
PS3, Line 9: Config ONBOARD_MICRON_MEM and ONBOARD_SAMSUNG_MEM are available.
           : These configs are used to determine if Samsung or Micron onboard memory is
           : assembled. This can not detected run-time.
           : 
           : One CONFIG value will be enough to determine if Samsung memory is
           : assembled.
           : Only oldest HW revision contains Samsung module, so set
           : CONFIG_ONBOARD_SAMSUNG memory to default No.
> If I run it through Vim, the text looks like below. […]

Can you clarify the issue?

[Frans Hendriks (Code Review)]

Hello Patrick Rudolph, Felix Held, Arthur Heymans, Patrick Rudolph, Lance Zhao, build bot (Jenkins),

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

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

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

Change subject: mb/facebook/fbg1701: Remove ONBOARD_MICRON_MEM
......................................................................

mb/facebook/fbg1701: Remove ONBOARD_MICRON_MEM

ONBOARD_MICRON_MEM and ONBOARD_SAMSUNG_MEM are available.
These are used to determine if Samsung or Micron onboard memory is
assembled. This can not detected run-time.

Choice is replaced by one config.
Only oldest HW revision contains Samsung module, so set
CONFIG_ONBOARD_SAMSUNG memory to default No.

BUG=N/A
TEST=Boot and verified on Facebook FBG-1701

Change-Id: Id65e92bd4b8d4fe3a6b87dec9bf77e3a62e1be96
Signed-off-by: Frans Hendriks 
---
M src/mainboard/facebook/fbg1701/Kconfig
M src/mainboard/facebook/fbg1701/romstage.c
2 files changed, 3 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/35742/4

[Frans Hendriks (Code Review)]

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35742 )

Change subject: mb/facebook/fbg1701: Remove ONBOARD_MICRON_MEM
......................................................................

Patch Set 4:

(1 comment)

Comment has been updated.

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

https://review.coreboot.org/c/coreboot/+/35742/3//COMMIT_MSG@9 
PS3, Line 9: Config ONBOARD_MICRON_MEM and ONBOARD_SAMSUNG_MEM are available.
           : These configs are used to determine if Samsung or Micron onboard memory is
           : assembled. This can not detected run-time.
           : 
           : One CONFIG value will be enough to determine if Samsung memory is
           : assembled.
           : Only oldest HW revision contains Samsung module, so set
           : CONFIG_ONBOARD_SAMSUNG memory to default No.
> Can you clarify the issue?

Done

[Patrick Georgi (Code Review)]

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

Change subject: mb/facebook/fbg1701: Remove ONBOARD_MICRON_MEM
......................................................................

mb/facebook/fbg1701: Remove ONBOARD_MICRON_MEM

ONBOARD_MICRON_MEM and ONBOARD_SAMSUNG_MEM are available.
These are used to determine if Samsung or Micron onboard memory is
assembled. This can not detected run-time.

Choice is replaced by one config.
Only oldest HW revision contains Samsung module, so set
CONFIG_ONBOARD_SAMSUNG memory to default No.

BUG=N/A
TEST=Boot and verified on Facebook FBG-1701

Change-Id: Id65e92bd4b8d4fe3a6b87dec9bf77e3a62e1be96
Signed-off-by: Frans Hendriks 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/35742
Tested-by: build bot (Jenkins) 
Reviewed-by: Patrick Rudolph 
---
M src/mainboard/facebook/fbg1701/Kconfig
M src/mainboard/facebook/fbg1701/romstage.c
2 files changed, 3 insertions(+), 12 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Rudolph: Looks good to me, approved

diff --git a/src/mainboard/facebook/fbg1701/Kconfig b/src/mainboard/facebook/fbg1701/Kconfig
index 402165d..0aa4acc 100644
--- a/src/mainboard/facebook/fbg1701/Kconfig
+++ b/src/mainboard/facebook/fbg1701/Kconfig
@@ -31,21 +31,12 @@
    select INTEL_GMA_HAVE_VBT
    select GENERIC_SPD_BIN

-choice
-	prompt "Onboard memory manufacturer"
-	default ONBOARD_MICRON_MEM
-
 config ONBOARD_SAMSUNG_MEM
-	bool "Samsung"
+	bool "Onboard memory manufacturer Samsung"
+	default n
    help
      Samsung K4B8G1646D memory

-config ONBOARD_MICRON_MEM
-	bool "Micron"
-	help
-	  Micron MT41K512M16HA memory
-endchoice
-
 config MAINBOARD_DIR
    string
    default facebook/fbg1701
diff --git a/src/mainboard/facebook/fbg1701/romstage.c b/src/mainboard/facebook/fbg1701/romstage.c
index d6b475c..879cc9a 100644
--- a/src/mainboard/facebook/fbg1701/romstage.c
+++ b/src/mainboard/facebook/fbg1701/romstage.c
@@ -35,7 +35,7 @@
    struct region_device spd_rdev;
    u8 spd_index = 0;

-	if (CONFIG(ONBOARD_MICRON_MEM))
+	if (!CONFIG(ONBOARD_SAMSUNG_MEM))
    	spd_index = 1;
    if (get_spd_cbfs_rdev(&spd_rdev, spd_index) < 0)
    	die("spd.bin not found\n");