Change in coreboot[master]: [TESTME]lenovo/x220: Attempt to fix broken DRAM init

[Patrick Rudolph (Code Review)]

Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39043 )

Change subject: [TESTME]lenovo/x220: Attempt to fix broken DRAM init
......................................................................

[TESTME]lenovo/x220: Attempt to fix broken DRAM init

Enable power on WWAN as it has SMBUS connected.
Might resolv an issue where DRAM init fails as no EEPROM is
found on the bus.

Untested.

Change-Id: Ia7a2ca370124ecf743b000998b56855d5ed8f573
Signed-off-by: Patrick Rudolph 
---
M src/ec/lenovo/h8/Makefile.inc
M src/mainboard/lenovo/x220/early_init.c
2 files changed, 15 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/39043/1

diff --git a/src/ec/lenovo/h8/Makefile.inc b/src/ec/lenovo/h8/Makefile.inc
index 51c11be..9c8687a 100644
--- a/src/ec/lenovo/h8/Makefile.inc
+++ b/src/ec/lenovo/h8/Makefile.inc
@@ -18,6 +18,8 @@
 ramstage-y += panic.c
 endif

+romstage += wwan.c
+
 ramstage-y += h8.c
 ramstage-y += bluetooth.c
 ramstage-y += wwan.c
diff --git a/src/mainboard/lenovo/x220/early_init.c b/src/mainboard/lenovo/x220/early_init.c
index 3429c1b..ae16f0c 100644
--- a/src/mainboard/lenovo/x220/early_init.c
+++ b/src/mainboard/lenovo/x220/early_init.c
@@ -24,6 +24,7 @@
 #include <southbridge/intel/bd82x6x/pch.h>
 #include <southbridge/intel/common/gpio.h>
 #include <cpu/x86/msr.h>
+#include <ec/lenovo/h8/h8.h>

void mainboard_fill_pei_data(struct pei_data *pei_data)
 {
@@ -74,6 +75,18 @@
    *pei_data = pei_data_template;
 }

+void mainboard_early_init(int s3_resume)
+{
+	/*
+	 * The WWAN slot has SMBUS connected. Turn on power to make sure
+	 * SMBUS is usable and DRAM init will succeed.
+	 * ramstage will turn it off, in case it's not needed.
+	 * TODO: Does GPIO42 (SMB_3B_EN) help here?
+	 */
+	if (h8_has_wwan())
+		h8_wwan_enable(true);
+}
+
 void mainboard_get_spd(spd_raw_data *spd, bool id_only)
 {
    read_spd (&spd[0], 0x50, id_only);

[Patrick Rudolph (Code Review)]

Hello Alexander Couzens, Patrick Rudolph, build bot (Jenkins), Patrick Georgi, Martin Roth,

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

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

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

Change subject: [TESTME]lenovo/x220: Attempt to fix broken DRAM init
......................................................................

[TESTME]lenovo/x220: Attempt to fix broken DRAM init

Enable power on WWAN as it has SMBUS connected.
Might resolv an issue where DRAM init fails as no EEPROM is
found on the bus.

Untested.

Change-Id: Ia7a2ca370124ecf743b000998b56855d5ed8f573
Signed-off-by: Patrick Rudolph 
---
M src/ec/lenovo/h8/Makefile.inc
M src/mainboard/lenovo/x220/early_init.c
2 files changed, 14 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/39043/2

[Patrick Rudolph (Code Review)]

Hello Alexander Couzens, Patrick Rudolph, build bot (Jenkins), Patrick Georgi, Martin Roth,

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

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

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

Change subject: [TESTME]lenovo/x220: Attempt to fix broken DRAM init
......................................................................

[TESTME]lenovo/x220: Attempt to fix broken DRAM init

Enable power on WWAN as it has SMBUS connected.
Might resolv an issue where DRAM init fails as no EEPROM is
found on the bus.

Untested.

Change-Id: Ia7a2ca370124ecf743b000998b56855d5ed8f573
Signed-off-by: Patrick Rudolph 
---
M src/ec/lenovo/h8/Makefile.inc
M src/mainboard/lenovo/x220/early_init.c
2 files changed, 14 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/39043/3

[Patrick Rudolph (Code Review)]

Hello Alexander Couzens, Patrick Rudolph, build bot (Jenkins), Patrick Georgi, Martin Roth,

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

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

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

Change subject: [TESTME]lenovo/x220: Attempt to fix broken DRAM init
......................................................................

[TESTME]lenovo/x220: Attempt to fix broken DRAM init

Enable power on WWAN as it has SMBUS connected.
Might resolv an issue where DRAM init fails as no EEPROM is
found on the bus.

Untested.

Change-Id: Ia7a2ca370124ecf743b000998b56855d5ed8f573
Signed-off-by: Patrick Rudolph 
---
M src/ec/lenovo/h8/Makefile.inc
M src/ec/lenovo/h8/wwan.c
M src/mainboard/lenovo/x220/early_init.c
3 files changed, 15 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/39043/4

[Richard Hughes (Code Review)]

Richard Hughes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39043 )

Change subject: [TESTME]lenovo/x220: Attempt to fix broken DRAM init
......................................................................

Patch Set 4: Code-Review+1

This fixes boot on my X220 with a Sierra Wireless AirPrime WWAN card installed. Before the SMBUS failed to init, which caused the native raminit to fail. Well done!

Change in coreboot[master]: lenovo/*: Fix DRAM init when WWAN card is present

[Patrick Rudolph (Code Review)]

Hello Alexander Couzens, Patrick Rudolph, Richard Hughes, build bot (Jenkins), Patrick Georgi, Martin Roth,

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

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

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

Change subject: lenovo/*: Fix DRAM init when WWAN card is present
......................................................................

lenovo/*: Fix DRAM init when WWAN card is present

The WWAN slot isn't powered by default. In case a WWAN card, that
support SMBUS is installed, it pulls the lines low.

Enable power on WWAN on those devices that connect SMBUS to the
WWAN slot.

Resolves an issue where DRAM init fails as no EEPROM could be
found on the bus.

Tested on X220: Boots with WWAN card inserted.

Change-Id: Ia7a2ca370124ecf743b000998b56855d5ed8f573
Signed-off-by: Patrick Rudolph 
---
M src/ec/lenovo/h8/Makefile.inc
M src/ec/lenovo/h8/wwan.c
M src/mainboard/lenovo/t420/early_init.c
M src/mainboard/lenovo/t430/early_init.c
M src/mainboard/lenovo/t520/early_init.c
M src/mainboard/lenovo/t530/early_init.c
M src/mainboard/lenovo/x220/early_init.c
M src/mainboard/lenovo/x230/early_init.c
8 files changed, 54 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/39043/5

Change in coreboot[master]: lenovo/*: Fix DRAM init when WWAN card is present

[Patrick Rudolph (Code Review)]

Hello Alexander Couzens, Patrick Rudolph, Richard Hughes, build bot (Jenkins), Patrick Georgi, Martin Roth,

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

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

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

Change subject: lenovo/*: Fix DRAM init when WWAN card is present
......................................................................

lenovo/*: Fix DRAM init when WWAN card is present

The WWAN slot isn't powered by default. In case a WWAN card, that
support SMBUS is installed, it pulls the lines low.

Enable power on WWAN on those devices that connect SMBUS to the
WWAN slot.

Resolves an issue where DRAM init fails as no EEPROM could be
found on the bus.

Tested on X220: Boots with WWAN card inserted.

Change-Id: Ia7a2ca370124ecf743b000998b56855d5ed8f573
Signed-off-by: Patrick Rudolph 
---
M src/ec/lenovo/h8/Makefile.inc
M src/ec/lenovo/h8/wwan.c
M src/mainboard/lenovo/t420/early_init.c
M src/mainboard/lenovo/t430/early_init.c
M src/mainboard/lenovo/t520/early_init.c
M src/mainboard/lenovo/t530/early_init.c
M src/mainboard/lenovo/x220/early_init.c
M src/mainboard/lenovo/x230/early_init.c
8 files changed, 54 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/39043/6

Change in coreboot[master]: lenovo/*: Fix DRAM init when WWAN card is present

[Paul Menzel (Code Review)]

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

Change subject: lenovo/*: Fix DRAM init when WWAN card is present
......................................................................

Patch Set 6: Code-Review+1

(5 comments)

https://review.coreboot.org/c/coreboot/+/39043/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/39043/6//COMMIT_MSG@10 
PS6, Line 10: support SMBUS is installed, it pulls the lines low.
… ,that supports SMBus, …

https://review.coreboot.org/c/coreboot/+/39043/6//COMMIT_MSG@12 
PS6, Line 12: SMBUS
SMBus

https://review.coreboot.org/c/coreboot/+/39043/6//COMMIT_MSG@15 
PS6, Line 15: EEPROM
Sorry for being dense, but the EEPROMs on the memory modules containing SPD data?

https://review.coreboot.org/c/coreboot/+/39043/6/src/mainboard/lenovo/t420/e... 
File src/mainboard/lenovo/t420/early_init.c:

https://review.coreboot.org/c/coreboot/+/39043/6/src/mainboard/lenovo/t420/e... 
PS6, Line 80: SMBUS
SMBus

https://review.coreboot.org/c/coreboot/+/39043/6/src/mainboard/lenovo/t430/e... 
File src/mainboard/lenovo/t430/early_init.c:

https://review.coreboot.org/c/coreboot/+/39043/6/src/mainboard/lenovo/t430/e... 
PS6, Line 74: SMBUS
SMBus

Change in coreboot[master]: lenovo/*: Fix DRAM init when WWAN card is present

[Patrick Rudolph (Code Review)]

Hello Alexander Couzens, Patrick Rudolph, Arthur Heymans, Richard Hughes, Paul Menzel, build bot (Jenkins), Patrick Georgi, Martin Roth,

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

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

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

Change subject: lenovo/*: Fix DRAM init when WWAN card is present
......................................................................

lenovo/*: Fix DRAM init when WWAN card is present

The WWAN slot isn't powered by default. In case a WWAN card, that
support SMBus, is installed it pulls the lines low.

Enable power on WWAN on those devices that connect SMBus to the
WWAN slot.

Resolves an issue where DRAM init fails as no EEPROM could be
found on the bus and the SPD couldn't be read.

Tested on X220: Boots with WWAN card inserted.

Change-Id: Ia7a2ca370124ecf743b000998b56855d5ed8f573
Signed-off-by: Patrick Rudolph 
---
M src/ec/lenovo/h8/Makefile.inc
M src/ec/lenovo/h8/wwan.c
M src/mainboard/lenovo/t420/early_init.c
M src/mainboard/lenovo/t430/early_init.c
M src/mainboard/lenovo/t520/early_init.c
M src/mainboard/lenovo/t530/early_init.c
M src/mainboard/lenovo/x220/early_init.c
M src/mainboard/lenovo/x230/early_init.c
8 files changed, 54 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/39043/7

Change in coreboot[master]: lenovo/*: Fix DRAM init when WWAN card is present

[Patrick Rudolph (Code Review)]

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

Change subject: lenovo/*: Fix DRAM init when WWAN card is present
......................................................................

Patch Set 7:

(5 comments)

https://review.coreboot.org/c/coreboot/+/39043/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/39043/6//COMMIT_MSG@10 
PS6, Line 10: support SMBUS is installed, it pulls the lines low.
> … ,that supports SMBus, …

Done

https://review.coreboot.org/c/coreboot/+/39043/6//COMMIT_MSG@12 
PS6, Line 12: SMBUS
> SMBus

Done

https://review.coreboot.org/c/coreboot/+/39043/6//COMMIT_MSG@15 
PS6, Line 15: EEPROM
> Sorry for being dense, but the EEPROMs on the memory modules containing SPD data?

Done

https://review.coreboot.org/c/coreboot/+/39043/6/src/mainboard/lenovo/t420/e... 
File src/mainboard/lenovo/t420/early_init.c:

https://review.coreboot.org/c/coreboot/+/39043/6/src/mainboard/lenovo/t420/e... 
PS6, Line 80: SMBUS
> SMBus

Done

https://review.coreboot.org/c/coreboot/+/39043/6/src/mainboard/lenovo/t430/e... 
File src/mainboard/lenovo/t430/early_init.c:

https://review.coreboot.org/c/coreboot/+/39043/6/src/mainboard/lenovo/t430/e... 
PS6, Line 74: SMBUS
> SMBus

Done

Change in coreboot[master]: lenovo/*: Fix DRAM init when WWAN card is present

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39043 )

Change subject: lenovo/*: Fix DRAM init when WWAN card is present
......................................................................

Patch Set 7:

(2 comments)

I would prefer that we use a consistent decision to enable it
across all stages of coreboot. If a specific card always needs
power, the user shouldn't turn it off. What would be left to
do is to move the current enabling/disabling from ramstage to
romstage. e.g.

void h8_early_init(void)
    {
            /* comment why we might need it early */
            h8_wwan_enable(h8_has_wwan(dev) && h8_wwan_nv_enable());
    }

https://review.coreboot.org/c/coreboot/+/39043/7//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/39043/7//COMMIT_MSG@10 
PS7, Line 10: support SMBus, is installed it pulls the lines low.
Looks like such a card would not be spec compliant:

"Care should be taken in the design of both the input and output stages
  of SMBus devices, in order not to load the bus when their power plane
  is turned off, i.e. powered-down devices must provide no leakage path
  to ground."

Probably worth to mention here.

https://review.coreboot.org/c/coreboot/+/39043/7/src/mainboard/lenovo/t420/e... 
File src/mainboard/lenovo/t420/early_init.c:

https://review.coreboot.org/c/coreboot/+/39043/7/src/mainboard/lenovo/t420/e... 
PS7, Line 82: 	 * ramstage will turn it off, in case it's not needed.
How to tell if it's not needed? There may be more going on on the
SMBus than just reading SPDs.

Change in coreboot[master]: lenovo/*: Fix DRAM init when WWAN card is present

[Patrick Rudolph (Code Review)]

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

Change subject: lenovo/*: Fix DRAM init when WWAN card is present
......................................................................

Patch Set 7:

(1 comment)
> Patch Set 7:
>
> (2 comments)
>
> I would prefer that we use a consistent decision to enable it
> across all stages of coreboot. If a specific card always needs
> power, the user shouldn't turn it off. What would be left to
> do is to move the current enabling/disabling from ramstage to
> romstage. e.g.
> void h8_early_init(void)
> {
>         /* comment why we might need it early */
>         h8_wwan_enable(h8_has_wwan(dev) && h8_wwan_nv_enable());
> }

bootblock doesn't need SMBUS, only romstage and ramstage do.
If such a card is present it can never be disabled in romstage.
What should work is

h8_wwan_enable(h8_has_wwan(dev));

in romstage, but there's no nice way to reference the EC's struct device.

https://review.coreboot.org/c/coreboot/+/39043/7/src/mainboard/lenovo/t420/e... 
File src/mainboard/lenovo/t420/early_init.c:

https://review.coreboot.org/c/coreboot/+/39043/7/src/mainboard/lenovo/t420/e... 
PS7, Line 82: 	 * ramstage will turn it off, in case it's not needed.
> How to tell if it's not needed? There may be more going on on the […]

There's the AT24 EEPROM, too. It need some changes to handle reading it in DEV_INIT. Then it can be turned off in DEV_FINALIZE.

Change in coreboot[master]: lenovo/*: Fix DRAM init when WWAN card is present

[Angel Pons (Code Review)]

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

Change subject: lenovo/*: Fix DRAM init when WWAN card is present
......................................................................

Patch Set 7:

(1 comment)

File src/mainboard/lenovo/x220/early_init.c:

https://review.coreboot.org/c/coreboot/+/39043/comment/a89e65e1_32f544f4 
PS7, Line 85: 	if (CONFIG(BOARD_LENOVO_X220))
Why this distinction?

[M] Change in coreboot[master]: lenovo/*: Fix DRAM init when WWAN card is present

[Martin L Roth (Code Review)]

Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/39043?usp=email )

Change subject: lenovo/*: Fix DRAM init when WWAN card is present
......................................................................

Abandoned

This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.