Change in coreboot[master]: soc/intel/elkhartlake: Fix EHL mainboard build fail errors

[Lean Sheng Tan (Code Review)]

Lean Sheng Tan has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47804 )

Change subject: soc/intel/elkhartlake: Fix EHL mainboard build fail errors
......................................................................

soc/intel/elkhartlake: Fix EHL mainboard build fail errors

When EHL initial mainboard patch is uploaded, there are some build
errors caused by EHL soc codes. Here are the fixes:
1. include gpio_op.asl to resolve undefined variables in scs.asl
2. remove unused variables in fsp_params.c
3. rearrage sequences of #includes to fix build dependency of
   soc/gpio_defs.h in intelblocks/gpio.h
4. add the __weak to mainboard_memory_init_params function

Signed-off-by: Tan, Lean Sheng 
Change-Id: Idaa8b0b5301742287665abde065ad72965bc62b3
---
M src/soc/intel/elkhartlake/acpi/gpio.asl
M src/soc/intel/elkhartlake/fsp_params.c
M src/soc/intel/elkhartlake/include/soc/gpio.h
M src/soc/intel/elkhartlake/romstage/fsp_params.c
4 files changed, 3 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/47804/1

diff --git a/src/soc/intel/elkhartlake/acpi/gpio.asl b/src/soc/intel/elkhartlake/acpi/gpio.asl
index ac0d1df..27cb503 100644
--- a/src/soc/intel/elkhartlake/acpi/gpio.asl
+++ b/src/soc/intel/elkhartlake/acpi/gpio.asl
@@ -3,6 +3,7 @@
 #include <intelblocks/gpio.h>
 #include <soc/gpio_defs.h>
 #include <soc/intel/common/acpi/gpio.asl>
+#include <soc/intel/common/block/acpi/acpi/gpio_op.asl>
 #include <soc/irq.h>
 #include <soc/pcr_ids.h>

diff --git a/src/soc/intel/elkhartlake/fsp_params.c b/src/soc/intel/elkhartlake/fsp_params.c
index 3d740fb..83a3699 100644
--- a/src/soc/intel/elkhartlake/fsp_params.c
+++ b/src/soc/intel/elkhartlake/fsp_params.c
@@ -44,17 +44,13 @@

static void parse_devicetree(FSP_S_CONFIG *params)
 {
-	const struct soc_intel_elkhartlake_config *config = config_of_soc();
    /* TODO: Update with UPD override as FSP matures */
 }

/* UPD parameters to be initialized before SiliconInit */
 void platform_fsp_silicon_init_params_cb(FSPS_UPD *supd)
 {
-	unsigned int i;
-	struct device *dev;
    FSP_S_CONFIG *params = &supd->FspsConfig;
-	struct soc_intel_elkhartlake_config *config = config_of_soc();

/* Parse device tree and fill in FSP UPDs */
    parse_devicetree(params);
diff --git a/src/soc/intel/elkhartlake/include/soc/gpio.h b/src/soc/intel/elkhartlake/include/soc/gpio.h
index 6cca742..367df82 100644
--- a/src/soc/intel/elkhartlake/include/soc/gpio.h
+++ b/src/soc/intel/elkhartlake/include/soc/gpio.h
@@ -3,8 +3,8 @@
 #ifndef _SOC_ELKHARTLAKE_GPIO_H_
 #define _SOC_ELKHARTLAKE_GPIO_H_

-#include <intelblocks/gpio.h>
 #include <soc/gpio_defs.h>
+#include <intelblocks/gpio.h>

#define CROS_GPIO_NAME		"INT34C8"
 #define CROS_GPIO_COMM0_NAME    "INT34C8:00"
diff --git a/src/soc/intel/elkhartlake/romstage/fsp_params.c b/src/soc/intel/elkhartlake/romstage/fsp_params.c
index 3961dfc..0fa8451 100644
--- a/src/soc/intel/elkhartlake/romstage/fsp_params.c
+++ b/src/soc/intel/elkhartlake/romstage/fsp_params.c
@@ -25,7 +25,7 @@
    mainboard_memory_init_params(mupd);
 }

-void mainboard_memory_init_params(FSPM_UPD *mupd)
+__weak void mainboard_memory_init_params(FSPM_UPD *mupd)
 {
    /* TODO: Update later together with UPD updates */
 }

[Lean Sheng Tan (Code Review)]

Hello Maulik V Vaghela, Paul Menzel, Mario Scheithauer, Subrata Banik, Aamir Bohra, Werner Zeh, Patrick Rudolph,

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

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

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

Change subject: soc/intel/elkhartlake: Fix EHL mainboard build fail errors
......................................................................

soc/intel/elkhartlake: Fix EHL mainboard build fail errors

When EHL initial mainboard patch is uploaded, there are some build
errors caused by EHL soc codes. Here are the fixes:
1. include gpio_op.asl to resolve undefined variables in scs.asl
2. remove unused variables in fsp_params.c
3. rearrage sequences of #includes to fix build dependency of
   soc/gpio_defs.h in intelblocks/gpio.h
4. add the __weak to mainboard_memory_init_params function
5. add the missing _len as per this patch changes
   https://review.coreboot.org/c/coreboot/+/45873

Signed-off-by: Tan, Lean Sheng 
Change-Id: Idaa8b0b5301742287665abde065ad72965bc62b3
---
M src/soc/intel/elkhartlake/acpi/gpio.asl
M src/soc/intel/elkhartlake/fsp_params.c
M src/soc/intel/elkhartlake/include/soc/gpio.h
M src/soc/intel/elkhartlake/romstage/fsp_params.c
M src/soc/intel/elkhartlake/romstage/romstage.c
5 files changed, 4 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/47804/2

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47804 )

Change subject: soc/intel/elkhartlake: Fix EHL mainboard build fail errors
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/47804/2/src/soc/intel/elkhartlake/i... 
File src/soc/intel/elkhartlake/include/soc/gpio.h:

https://review.coreboot.org/c/coreboot/+/47804/2/src/soc/intel/elkhartlake/i... 
PS2, Line 7: #include <intelblocks/gpio.h>
This file per sé does not need any include at all. Wouldn't it be better to include the needed files where they are realy used and maintain the order there?

[Lean Sheng Tan (Code Review)]

Lean Sheng Tan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47804 )

Change subject: soc/intel/elkhartlake: Fix EHL mainboard build fail errors
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47804/2/src/soc/intel/elkhartlake/i... 
File src/soc/intel/elkhartlake/include/soc/gpio.h:

https://review.coreboot.org/c/coreboot/+/47804/2/src/soc/intel/elkhartlake/i... 
PS2, Line 7: #include <intelblocks/gpio.h>
> This file per sé does not need any include at all. […]

I tried to remove them but the build failed. Can we maintain here first then revisit this to find where to put this for all platforms? Where do you suggest to place them?

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47804 )

Change subject: soc/intel/elkhartlake: Fix EHL mainboard build fail errors
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47804/3/src/soc/intel/elkhartlake/i... 
File src/soc/intel/elkhartlake/include/soc/gpio.h:

https://review.coreboot.org/c/coreboot/+/47804/3/src/soc/intel/elkhartlake/i... 
PS3, Line 6: #include <soc/gpio_defs.h>
           : #include <intelblocks/gpio.h>
From the first look it seems like soc/gpio_defs.h defines "GPIO_NUM_PAD_CFG_REGS" which is then used in intelblocks/gpio.h for struct pad_config. The other candidate is "NUM_GPI_STATUS_REGS", which is defined in soc/gpio_defs.h and used in intelblocks/gpio.h.
It is kind of strange that we rely on a proper include order in a file in order to satisfy the needs in a common block path. From a first look this #includes would be better placed directly there where they are needed. Is there something that prevents us to include soc/gpio_dfefs.h directly in intelblocks/gpio.h as already done with soc/gpio.h? You could then (I haven't tried yet) include intelblocks/gpio.h directly in baseboard/gpio.h and get rid of these two lines completely. This would, in my point of view, make these inclusion dependency much more clear. As a follow up, we could clean that on other socs, too as at least jasperlake follows the same scheme.

What are your thoughs about it?

[Lean Sheng Tan (Code Review)]

Lean Sheng Tan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47804 )

Change subject: soc/intel/elkhartlake: Fix EHL mainboard build fail errors
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47804/3/src/soc/intel/elkhartlake/i... 
File src/soc/intel/elkhartlake/include/soc/gpio.h:

https://review.coreboot.org/c/coreboot/+/47804/3/src/soc/intel/elkhartlake/i... 
PS3, Line 6: #include <soc/gpio_defs.h>
           : #include <intelblocks/gpio.h>
> From the first look it seems like soc/gpio_defs. […]

Hi Werner,
I was very happy when i moved #include <soc/gpio_defs.h> from here to intelblocks/gpio_defs.h and it worked. However, I got stumbled when I am porting this to all intel platforms, and it is not really possible or it required big changes for cannonlake as this is the code in cannonlake/incluse/soc/gpio.h:
#if CONFIG(SOC_INTEL_CANNONLAKE_PCH_H)
#include <soc/gpio_defs_cnp_h.h>
#define CROS_GPIO_DEVICE_NAME	"INT3450:00"
#else
#include <soc/gpio_defs.h>
#define CROS_GPIO_DEVICE_NAME	"INT34BB:00"

Do you have any better idea on how could we work this out?

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47804 )

Change subject: soc/intel/elkhartlake: Fix EHL mainboard build fail errors
......................................................................

Patch Set 4: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/47804/3/src/soc/intel/elkhartlake/i... 
File src/soc/intel/elkhartlake/include/soc/gpio.h:

https://review.coreboot.org/c/coreboot/+/47804/3/src/soc/intel/elkhartlake/i... 
PS3, Line 6: #include <soc/gpio_defs.h>
           : #include <intelblocks/gpio.h>
> Hi Werner, […]

I see...strange enough that we have this dependency here.
I guess the most transparent way in terms of your commit would be to continue with the order-dependency and clean this up later for all SoCs at once.

[Lean Sheng Tan (Code Review)]

Lean Sheng Tan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47804 )

Change subject: soc/intel/elkhartlake: Fix EHL mainboard build fail errors
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47804/3/src/soc/intel/elkhartlake/i... 
File src/soc/intel/elkhartlake/include/soc/gpio.h:

https://review.coreboot.org/c/coreboot/+/47804/3/src/soc/intel/elkhartlake/i... 
PS3, Line 6: #include <soc/gpio_defs.h>
           : #include <intelblocks/gpio.h>
> I see...strange enough that we have this dependency here. […]

Thanks for your feedback. Let me discuss this with other intel projects developers internally.

[Lean Sheng Tan (Code Review)]

Lean Sheng Tan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47804 )

Change subject: soc/intel/elkhartlake: Fix EHL mainboard build fail errors
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47804/2/src/soc/intel/elkhartlake/i... 
File src/soc/intel/elkhartlake/include/soc/gpio.h:

https://review.coreboot.org/c/coreboot/+/47804/2/src/soc/intel/elkhartlake/i... 
PS2, Line 7: #include <intelblocks/gpio.h>
> I tried to remove them but the build failed. […]

Ack

[Patrick Georgi (Code Review)]

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

Change subject: soc/intel/elkhartlake: Fix EHL mainboard build fail errors
......................................................................

soc/intel/elkhartlake: Fix EHL mainboard build fail errors

When EHL initial mainboard patch is uploaded, there are some build
errors caused by EHL soc codes. Here are the fixes:
1. include gpio_op.asl to resolve undefined variables in scs.asl
2. remove unused variables in fsp_params.c
3. rearrage sequences of #includes to fix build dependency of
   soc/gpio_defs.h in intelblocks/gpio.h
4. add the __weak to mainboard_memory_init_params function
5. add the missing _len as per this patch changes
   https://review.coreboot.org/c/coreboot/+/45873

Signed-off-by: Tan, Lean Sheng 
Change-Id: Idaa8b0b5301742287665abde065ad72965bc62b3
Reviewed-on: https://review.coreboot.org/c/coreboot/+/47804
Reviewed-by: Werner Zeh 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/elkhartlake/acpi/gpio.asl
M src/soc/intel/elkhartlake/fsp_params.c
M src/soc/intel/elkhartlake/include/soc/gpio.h
M src/soc/intel/elkhartlake/romstage/fsp_params.c
M src/soc/intel/elkhartlake/romstage/romstage.c
5 files changed, 4 insertions(+), 7 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Werner Zeh: Looks good to me, approved

diff --git a/src/soc/intel/elkhartlake/acpi/gpio.asl b/src/soc/intel/elkhartlake/acpi/gpio.asl
index ac0d1df..27cb503 100644
--- a/src/soc/intel/elkhartlake/acpi/gpio.asl
+++ b/src/soc/intel/elkhartlake/acpi/gpio.asl
@@ -3,6 +3,7 @@
 #include <intelblocks/gpio.h>
 #include <soc/gpio_defs.h>
 #include <soc/intel/common/acpi/gpio.asl>
+#include <soc/intel/common/block/acpi/acpi/gpio_op.asl>
 #include <soc/irq.h>
 #include <soc/pcr_ids.h>

diff --git a/src/soc/intel/elkhartlake/fsp_params.c b/src/soc/intel/elkhartlake/fsp_params.c
index 3d740fb..83a3699 100644
--- a/src/soc/intel/elkhartlake/fsp_params.c
+++ b/src/soc/intel/elkhartlake/fsp_params.c
@@ -44,17 +44,13 @@

static void parse_devicetree(FSP_S_CONFIG *params)
 {
-	const struct soc_intel_elkhartlake_config *config = config_of_soc();
    /* TODO: Update with UPD override as FSP matures */
 }

/* UPD parameters to be initialized before SiliconInit */
 void platform_fsp_silicon_init_params_cb(FSPS_UPD *supd)
 {
-	unsigned int i;
-	struct device *dev;
    FSP_S_CONFIG *params = &supd->FspsConfig;
-	struct soc_intel_elkhartlake_config *config = config_of_soc();

/* Parse device tree and fill in FSP UPDs */
    parse_devicetree(params);
diff --git a/src/soc/intel/elkhartlake/include/soc/gpio.h b/src/soc/intel/elkhartlake/include/soc/gpio.h
index 6cca742..367df82 100644
--- a/src/soc/intel/elkhartlake/include/soc/gpio.h
+++ b/src/soc/intel/elkhartlake/include/soc/gpio.h
@@ -3,8 +3,8 @@
 #ifndef _SOC_ELKHARTLAKE_GPIO_H_
 #define _SOC_ELKHARTLAKE_GPIO_H_

-#include <intelblocks/gpio.h>
 #include <soc/gpio_defs.h>
+#include <intelblocks/gpio.h>

#define CROS_GPIO_NAME		"INT34C8"
 #define CROS_GPIO_COMM0_NAME    "INT34C8:00"
diff --git a/src/soc/intel/elkhartlake/romstage/fsp_params.c b/src/soc/intel/elkhartlake/romstage/fsp_params.c
index 3961dfc..0fa8451 100644
--- a/src/soc/intel/elkhartlake/romstage/fsp_params.c
+++ b/src/soc/intel/elkhartlake/romstage/fsp_params.c
@@ -25,7 +25,7 @@
    mainboard_memory_init_params(mupd);
 }

-void mainboard_memory_init_params(FSPM_UPD *mupd)
+__weak void mainboard_memory_init_params(FSPM_UPD *mupd)
 {
    /* TODO: Update later together with UPD updates */
 }
diff --git a/src/soc/intel/elkhartlake/romstage/romstage.c b/src/soc/intel/elkhartlake/romstage/romstage.c
index ae9cc4a..7ca0318 100644
--- a/src/soc/intel/elkhartlake/romstage/romstage.c
+++ b/src/soc/intel/elkhartlake/romstage/romstage.c
@@ -62,7 +62,7 @@
    /* Allow mainboard to override DRAM part number. */
    dram_part_num = mainboard_get_dram_part_num();
    if (dram_part_num) {
-		dram_part_num = strlen(dram_part_num);
+		dram_part_num_len = strlen(dram_part_num);
    	is_dram_part_overridden = true;
    }