Change in coreboot[master]: mb/google/dedede: Remove pad termination for RAM_STRAP_4

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40531 )

Change subject: mb/google/dedede: Remove pad termination for RAM_STRAP_4
......................................................................

mb/google/dedede: Remove pad termination for RAM_STRAP_4

The stuffed resistor straps are weaker compared to the internal pull-up.
This can cause the GPIO to read '1' always. Remove the internal pull-up.

BUG=b:154301008
TEST=Build and boot the mainboard.

Change-Id: Ib640211b9f50dfb0174a570eda1625bacbebb855
Signed-off-by: Karthikeyan Ramasubramanian 
---
M src/mainboard/google/dedede/variants/baseboard/gpio.c
1 file changed, 2 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/40531/1

diff --git a/src/mainboard/google/dedede/variants/baseboard/gpio.c b/src/mainboard/google/dedede/variants/baseboard/gpio.c
index a4ce97d..58f5ca3 100644
--- a/src/mainboard/google/dedede/variants/baseboard/gpio.c
+++ b/src/mainboard/google/dedede/variants/baseboard/gpio.c
@@ -348,7 +348,7 @@

/* S0  : RAM_STRAP_4 */
-	PAD_CFG_GPI(GPP_S0, UP_5K, DEEP),
+	PAD_CFG_GPI(GPP_S0, NONE, DEEP),
    /* S1  : RSVD_STRAP */
    PAD_NC(GPP_S1, NONE),
    /* S2  : DMIC1_CLK */
@@ -418,7 +418,7 @@
    PAD_CFG_GPO(GPP_H19, 0, DEEP),

/* S0  : RAM_STRAP_4 */
-	PAD_CFG_GPI(GPP_S0, UP_5K, DEEP),
+	PAD_CFG_GPI(GPP_S0, NONE, DEEP),
 };

const struct pad_config *__weak variant_gpio_table(size_t *num)

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40531 )

Change subject: mb/google/dedede: Remove pad termination for RAM_STRAP_4
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/40531/1//COMMIT_MSG@10 
PS1, Line 10: Remove the internal pull-up.
What is the impact of doing this? Does this mean that we cannot really guarantee whether single or dual channel configuration is being used? Are all boards for the current board version have both channels populated? If yes, do we need to have a workaround in coreboot to ensure it behaves correctly?

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40531 )

Change subject: mb/google/dedede: Remove pad termination for RAM_STRAP_4
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/40531/1//COMMIT_MSG@10 
PS1, Line 10: Remove the internal pull-up.
> What is the impact of doing this? Does this mean that we cannot really guarantee whether single or d […]
> What is the impact of doing this? Does this mean that we cannot really guarantee whether single or dual channel configuration is being used?

On boards where the strap is not stuffed, we cannot really guarantee whether single or dual channel config is being used.
> Are all boards for the current board version have both channels populated?

All the current board versions have both channels populated.
> If yes, do we need to have a workaround in coreboot to ensure it behaves correctly?

One work-around that I can think of is, if the GPIO is reading as 0 and CBI is not populated then fallback to the dual channel configuration. Else use single channel configuration.

If GPIO is reading as 1, then use dual-channel configuration.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40531 )

Change subject: mb/google/dedede: Remove pad termination for RAM_STRAP_4
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/40531/1//COMMIT_MSG@10 
PS1, Line 10: Remove the internal pull-up.
> All the current board versions have both channels populated.

Does that mean that all the boards from current hardware revision have external PD stuffed?

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40531 )

Change subject: mb/google/dedede: Remove pad termination for RAM_STRAP_4
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/40531/1//COMMIT_MSG@10 
PS1, Line 10: Remove the internal pull-up.
> > All the current board versions have both channels populated. […]

Sorry I have to take a step back.
> Does that mean that all the boards from current hardware revision have external PD stuffed?

External PD stuffed -> Single channel configuration
External PU stuffed -> Dual channel configuration

Currently there are 3 revisions of board.
1) No resistor strapping stuffed - Dual channel configuration
2) External PU stuffed - Dual channel configuration
3) External PD stuffed - Single Channel configuration

[Marco Chen (Code Review)]

Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40531 )

Change subject: mb/google/dedede: Remove pad termination for RAM_STRAP_4
......................................................................

Patch Set 1: Code-Review+2

[Paul Menzel (Code Review)]

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

Change subject: mb/google/dedede: Remove pad termination for RAM_STRAP_4
......................................................................

Patch Set 1:

Please update the commit message, once Furquan’s questions are done.

[Karthik Ramasubramanian (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Marco Chen,

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

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

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

Change subject: mb/google/dedede: Remove pad termination for RAM_STRAP_4
......................................................................

mb/google/dedede: Remove pad termination for RAM_STRAP_4

The stuffed resistor straps are weaker compared to the internal pull-up.
This can cause the GPIO to read '1' always. Remove the internal pull-up.
Also read the GPIO only on the boards where the board version is
populated.

BUG=b:154301008
TEST=Build and boot the mainboard.

Change-Id: Ib640211b9f50dfb0174a570eda1625bacbebb855
Signed-off-by: Karthikeyan Ramasubramanian 
---
M src/mainboard/google/dedede/romstage.c
M src/mainboard/google/dedede/variants/baseboard/gpio.c
2 files changed, 9 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/40531/2

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40531 )

Change subject: mb/google/dedede: Remove pad termination for RAM_STRAP_4
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/40531/1//COMMIT_MSG@10 
PS1, Line 10: Remove the internal pull-up.
> Sorry I have to take a step back. […]

Done

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40531 )

Change subject: mb/google/dedede: Remove pad termination for RAM_STRAP_4
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40531/2/src/mainboard/google/dedede... 
File src/mainboard/google/dedede/romstage.c:

https://review.coreboot.org/c/coreboot/+/40531/2/src/mainboard/google/dedede... 
PS2, Line 26: if (!google_chromeec_get_board_version(&board_ver))
            : 		half_populated = !gpio_get(GPIO_MEM_CH_SEL);
Does this need to happen in baseboard? Or is this a particular board variant that needs this?

[Marco Chen (Code Review)]

Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40531 )

Change subject: mb/google/dedede: Remove pad termination for RAM_STRAP_4
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40531/2/src/mainboard/google/dedede... 
File src/mainboard/google/dedede/romstage.c:

https://review.coreboot.org/c/coreboot/+/40531/2/src/mainboard/google/dedede... 
PS2, Line 26: if (!google_chromeec_get_board_version(&board_ver))
            : 		half_populated = !gpio_get(GPIO_MEM_CH_SEL);
> Does this need to happen in baseboard? Or is this a particular board variant that needs this?

I believe we can put here first until hit the real case?

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40531 )

Change subject: mb/google/dedede: Remove pad termination for RAM_STRAP_4
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40531/2/src/mainboard/google/dedede... 
File src/mainboard/google/dedede/romstage.c:

https://review.coreboot.org/c/coreboot/+/40531/2/src/mainboard/google/dedede... 
PS2, Line 26: if (!google_chromeec_get_board_version(&board_ver))
            : 		half_populated = !gpio_get(GPIO_MEM_CH_SEL);
> I believe we can put here first until hit the real case?

Both the existing variants (Waddledoo and waddledee) have one phase where the board version is not populated.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40531 )

Change subject: mb/google/dedede: Remove pad termination for RAM_STRAP_4
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40531/2/src/mainboard/google/dedede... 
File src/mainboard/google/dedede/romstage.c:

https://review.coreboot.org/c/coreboot/+/40531/2/src/mainboard/google/dedede... 
PS2, Line 26: if (!google_chromeec_get_board_version(&board_ver))
            : 		half_populated = !gpio_get(GPIO_MEM_CH_SEL);
> Both the existing variants (Waddledoo and waddledee) have one phase where the board version is not p […]

Sure, but there will be future derivatives too. Is this something that we want to be the default? Should there be a function:

variant_is_half_populated()

with a weak implementation that returns GPIO_MEM_CH_SEL based on board version check?

Also, are both variants -- waddledoo and waddledee -- missing the external pulls?

[Marco Chen (Code Review)]

Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40531 )

Change subject: mb/google/dedede: Remove pad termination for RAM_STRAP_4
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40531/2/src/mainboard/google/dedede... 
File src/mainboard/google/dedede/romstage.c:

https://review.coreboot.org/c/coreboot/+/40531/2/src/mainboard/google/dedede... 
PS2, Line 26: if (!google_chromeec_get_board_version(&board_ver))
            : 		half_populated = !gpio_get(GPIO_MEM_CH_SEL);
> Sure, but there will be future derivatives too. […]

1. all OEM projects MUST be started with CBI provisioned so board version would be 0 instead of None. As a result, may I know what other deviation we allow future projects to have the deviation here?
2. Dee proto 0 and Doo pre-proto (without CBI provisioned) didn't be built with any SKU in single channel DRAM so default 'half_populated = 0' is good?
3. Of course, if anything wrong in the future device and need workaround for a board version then we can add for sure.

[Karthik Ramasubramanian (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Marco Chen,

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

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

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

Change subject: mb/google/dedede: Remove pad termination for RAM_STRAP_4
......................................................................

mb/google/dedede: Remove pad termination for RAM_STRAP_4

The stuffed resistor straps are weaker compared to the internal pull-up.
This can cause the GPIO to read '1' always. Remove the internal pull-up.
Also read the GPIO only on the boards where the board version is
populated.

BUG=b:154301008
TEST=Build and boot the mainboard.

Change-Id: Ib640211b9f50dfb0174a570eda1625bacbebb855
Signed-off-by: Karthikeyan Ramasubramanian 
---
M src/mainboard/google/dedede/romstage.c
M src/mainboard/google/dedede/variants/baseboard/gpio.c
M src/mainboard/google/dedede/variants/baseboard/include/baseboard/variants.h
M src/mainboard/google/dedede/variants/baseboard/memory.c
M src/mainboard/google/dedede/variants/waddledee/Makefile.inc
A src/mainboard/google/dedede/variants/waddledee/memory.c
M src/mainboard/google/dedede/variants/waddledoo/Makefile.inc
A src/mainboard/google/dedede/variants/waddledoo/memory.c
8 files changed, 63 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/40531/3

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40531 )

Change subject: mb/google/dedede: Remove pad termination for RAM_STRAP_4
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40531/2/src/mainboard/google/dedede... 
File src/mainboard/google/dedede/romstage.c:

https://review.coreboot.org/c/coreboot/+/40531/2/src/mainboard/google/dedede... 
PS2, Line 26: if (!google_chromeec_get_board_version(&board_ver))
            : 		half_populated = !gpio_get(GPIO_MEM_CH_SEL);
> […]

Fair feedback. Only waddledee and waddledoo will have the strap unstuffed. So no need to penalize other upcoming boards with a call to EC. Added a weak variant specific function.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40531 )

Change subject: mb/google/dedede: Remove pad termination for RAM_STRAP_4
......................................................................

Patch Set 3: Code-Review+2

[Patrick Georgi (Code Review)]

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

Change subject: mb/google/dedede: Remove pad termination for RAM_STRAP_4
......................................................................

mb/google/dedede: Remove pad termination for RAM_STRAP_4

The stuffed resistor straps are weaker compared to the internal pull-up.
This can cause the GPIO to read '1' always. Remove the internal pull-up.
Also read the GPIO only on the boards where the board version is
populated.

BUG=b:154301008
TEST=Build and boot the mainboard.

Change-Id: Ib640211b9f50dfb0174a570eda1625bacbebb855
Signed-off-by: Karthikeyan Ramasubramanian 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/40531
Tested-by: build bot (Jenkins) 
Reviewed-by: Furquan Shaikh 
---
M src/mainboard/google/dedede/romstage.c
M src/mainboard/google/dedede/variants/baseboard/gpio.c
M src/mainboard/google/dedede/variants/baseboard/include/baseboard/variants.h
M src/mainboard/google/dedede/variants/baseboard/memory.c
M src/mainboard/google/dedede/variants/waddledee/Makefile.inc
A src/mainboard/google/dedede/variants/waddledee/memory.c
M src/mainboard/google/dedede/variants/waddledoo/Makefile.inc
A src/mainboard/google/dedede/variants/waddledoo/memory.c
8 files changed, 63 insertions(+), 3 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved

diff --git a/src/mainboard/google/dedede/romstage.c b/src/mainboard/google/dedede/romstage.c
index 2efaaf1..7a700f4 100644
--- a/src/mainboard/google/dedede/romstage.c
+++ b/src/mainboard/google/dedede/romstage.c
@@ -18,7 +18,7 @@
    	.read_type = READ_SPD_CBFS,
    	.spd_spec.spd_index = variant_memory_sku(),
    };
-	bool half_populated = !gpio_get(GPIO_MEM_CH_SEL);
+	bool half_populated = variant_mem_is_half_populated();

memcfg_init(&memupd->FspmConfig, board_cfg, &spd_info, half_populated);
 }
diff --git a/src/mainboard/google/dedede/variants/baseboard/gpio.c b/src/mainboard/google/dedede/variants/baseboard/gpio.c
index b09d6c1..6adb35b 100644
--- a/src/mainboard/google/dedede/variants/baseboard/gpio.c
+++ b/src/mainboard/google/dedede/variants/baseboard/gpio.c
@@ -348,7 +348,7 @@

/* S0  : RAM_STRAP_4 */
-	PAD_CFG_GPI(GPP_S0, UP_5K, DEEP),
+	PAD_CFG_GPI(GPP_S0, NONE, DEEP),
    /* S1  : RSVD_STRAP */
    PAD_NC(GPP_S1, NONE),
    /* S2  : DMIC1_CLK */
@@ -418,7 +418,7 @@
    PAD_CFG_GPO(GPP_H19, 0, DEEP),

/* S0  : RAM_STRAP_4 */
-	PAD_CFG_GPI(GPP_S0, UP_5K, DEEP),
+	PAD_CFG_GPI(GPP_S0, NONE, DEEP),
 };

const struct pad_config *__weak variant_gpio_table(size_t *num)
diff --git a/src/mainboard/google/dedede/variants/baseboard/include/baseboard/variants.h b/src/mainboard/google/dedede/variants/baseboard/include/baseboard/variants.h
index 8fd5119..48c1419 100644
--- a/src/mainboard/google/dedede/variants/baseboard/include/baseboard/variants.h
+++ b/src/mainboard/google/dedede/variants/baseboard/include/baseboard/variants.h
@@ -33,4 +33,11 @@
 /* Return memory SKU for the variant */
 int variant_memory_sku(void);

+/**
+ * Get data whether memory channel is half-populated or not
+ *
+ * @return false on boards where memory channel is half-populated, true otherwise.
+ */
+bool variant_mem_is_half_populated(void);
+
 #endif /*__BASEBOARD_VARIANTS_H__ */
diff --git a/src/mainboard/google/dedede/variants/baseboard/memory.c b/src/mainboard/google/dedede/variants/baseboard/memory.c
index 08c3bde..120cb4e 100644
--- a/src/mainboard/google/dedede/variants/baseboard/memory.c
+++ b/src/mainboard/google/dedede/variants/baseboard/memory.c
@@ -70,3 +70,8 @@

return gpio_base2_value(spd_gpios, ARRAY_SIZE(spd_gpios));
 }
+
+bool __weak variant_mem_is_half_populated(void)
+{
+	return !gpio_get(GPIO_MEM_CH_SEL);
+}
diff --git a/src/mainboard/google/dedede/variants/waddledee/Makefile.inc b/src/mainboard/google/dedede/variants/waddledee/Makefile.inc
index fb9b4f4..d3d6452 100644
--- a/src/mainboard/google/dedede/variants/waddledee/Makefile.inc
+++ b/src/mainboard/google/dedede/variants/waddledee/Makefile.inc
@@ -7,3 +7,5 @@

SPD_SOURCES = Micron_MT53E512M32D2NP_2GB	#0b0000
 SPD_SOURCES += empty				#0b0001
+
+romstage-y += memory.c
diff --git a/src/mainboard/google/dedede/variants/waddledee/memory.c b/src/mainboard/google/dedede/variants/waddledee/memory.c
new file mode 100644
index 0000000..d1e8af2
--- /dev/null
+++ b/src/mainboard/google/dedede/variants/waddledee/memory.c
@@ -0,0 +1,22 @@
+/*
+ * This file is part of the coreboot project.
+ *
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include <baseboard/variants.h>
+#include <baseboard/gpio.h>
+#include <ec/google/chromeec/ec.h>
+#include <gpio.h>
+
+bool variant_mem_is_half_populated(void)
+{
+	uint32_t board_ver;
+
+	/* On boards where board version is populated, ram strap is also populated */
+	if (!google_chromeec_get_board_version(&board_ver))
+		return !gpio_get(GPIO_MEM_CH_SEL);
+
+	return false;
+}
diff --git a/src/mainboard/google/dedede/variants/waddledoo/Makefile.inc b/src/mainboard/google/dedede/variants/waddledoo/Makefile.inc
index 71042c0..75cbb6a 100644
--- a/src/mainboard/google/dedede/variants/waddledoo/Makefile.inc
+++ b/src/mainboard/google/dedede/variants/waddledoo/Makefile.inc
@@ -7,3 +7,5 @@

SPD_SOURCES = empty				#0b0000
 SPD_SOURCES += Micron_MT53E512M32D2NP_2GB	#0b0001
+
+romstage-y += memory.c
diff --git a/src/mainboard/google/dedede/variants/waddledoo/memory.c b/src/mainboard/google/dedede/variants/waddledoo/memory.c
new file mode 100644
index 0000000..d1e8af2
--- /dev/null
+++ b/src/mainboard/google/dedede/variants/waddledoo/memory.c
@@ -0,0 +1,22 @@
+/*
+ * This file is part of the coreboot project.
+ *
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include <baseboard/variants.h>
+#include <baseboard/gpio.h>
+#include <ec/google/chromeec/ec.h>
+#include <gpio.h>
+
+bool variant_mem_is_half_populated(void)
+{
+	uint32_t board_ver;
+
+	/* On boards where board version is populated, ram strap is also populated */
+	if (!google_chromeec_get_board_version(&board_ver))
+		return !gpio_get(GPIO_MEM_CH_SEL);
+
+	return false;
+}