Change in coreboot[master]: sb/intel/lynxpoint/me_9.x.c: Reorder functions

[Angel Pons (Code Review)]

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

Change subject: sb/intel/lynxpoint/me_9.x.c: Reorder functions
......................................................................

sb/intel/lynxpoint/me_9.x.c: Reorder functions

This file is shared between ramstage and smm. However, not everything is
used in both ramstage and smm, so there's a lot of `__unused` tags. So,
reorder the functions so that they build only for stages needing them.

Tested with BUILD_TIMELESS=1, Asrock B85M Pro4 remains identical.

Change-Id: I3143f07ed845e0c6b1444817029a437db3b959e3
Signed-off-by: Angel Pons 
---
M src/southbridge/intel/lynxpoint/me_9.x.c
1 file changed, 167 insertions(+), 180 deletions(-)

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

diff --git a/src/southbridge/intel/lynxpoint/me_9.x.c b/src/southbridge/intel/lynxpoint/me_9.x.c
index d182e31..eaeab0f 100644
--- a/src/southbridge/intel/lynxpoint/me_9.x.c
+++ b/src/southbridge/intel/lynxpoint/me_9.x.c
@@ -31,24 +31,8 @@
 #include <vendorcode/google/chromeos/gnvs.h>
 #endif

-/* Path that the BIOS should take based on ME state */
-static const char *const me_bios_path_values[] __unused = {
-	[ME_NORMAL_BIOS_PATH]		= "Normal",
-	[ME_S3WAKE_BIOS_PATH]		= "S3 Wake",
-	[ME_ERROR_BIOS_PATH]		= "Error",
-	[ME_RECOVERY_BIOS_PATH]		= "Recovery",
-	[ME_DISABLE_BIOS_PATH]		= "Disable",
-	[ME_FIRMWARE_UPDATE_BIOS_PATH]	= "Firmware Update",
-};
-static int intel_me_read_mbp(me_bios_payload *mbp_data, struct device *dev);
-
 /* MMIO base address for MEI interface */
 static u32 *mei_base_address;
-#ifdef __SIMPLE_DEVICE__
-void intel_me_mbp_clear(pci_devfn_t dev);
-#else
-void intel_me_mbp_clear(struct device *dev);
-#endif

static void mei_dump(void *ptr, int dword, int offset, const char *type)
 {
@@ -426,8 +410,10 @@
  * the MBP has been read and cleared.
  */
 #ifdef __SIMPLE_DEVICE__
+void intel_me_mbp_clear(pci_devfn_t dev);
 void intel_me_mbp_clear(pci_devfn_t dev)
 #else
+void intel_me_mbp_clear(struct device *dev);
 void intel_me_mbp_clear(struct device *dev)
 #endif
 {
@@ -450,73 +436,10 @@
    }
 }

-static void __unused me_print_fw_version(mbp_fw_version_name *vers_name)
-{
-	if (!vers_name) {
-		printk(BIOS_ERR, "ME: mbp missing version report\n");
-		return;
-	}
-
-	printk(BIOS_DEBUG, "ME: found version %d.%d.%d.%d\n",
-	       vers_name->major_version, vers_name->minor_version,
-	       vers_name->hotfix_version, vers_name->build_version);
-}
-
-static inline void print_cap(const char *name, int state)
-{
-	printk(BIOS_DEBUG, "ME Capability: %-41s : %sabled\n",
-	       name, state ? " en" : "dis");
-}
-
-/* Get ME Firmware Capabilities */
-static int mkhi_get_fwcaps(mbp_mefwcaps *cap)
-{
-	u32 rule_id = 0;
-	struct me_fwcaps cap_msg;
-	struct mkhi_header mkhi = {
-		.group_id       = MKHI_GROUP_ID_FWCAPS,
-		.command        = MKHI_FWCAPS_GET_RULE,
-	};
-
-	/* Send request and wait for response */
-	if (mei_sendrecv_mkhi(&mkhi, &rule_id, sizeof(u32),
-			      &cap_msg, sizeof(cap_msg)) < 0) {
-		printk(BIOS_ERR, "ME: GET FWCAPS message failed\n");
-		return -1;
-	}
-	*cap = cap_msg.caps_sku;
-	return 0;
-}
-
-/* Get ME Firmware Capabilities */
-static void __unused me_print_fwcaps(mbp_mefwcaps *cap)
-{
-	mbp_mefwcaps local_caps;
-	if (!cap) {
-		cap = &local_caps;
-		printk(BIOS_ERR, "ME: mbp missing fwcaps report\n");
-		if (mkhi_get_fwcaps(cap))
-			return;
-	}
-
-	print_cap("Full Network manageability", cap->full_net);
-	print_cap("Regular Network manageability", cap->std_net);
-	print_cap("Manageability", cap->manageability);
-	print_cap("IntelR Anti-Theft (AT)", cap->intel_at);
-	print_cap("IntelR Capability Licensing Service (CLS)", cap->intel_cls);
-	print_cap("IntelR Power Sharing Technology (MPC)", cap->intel_mpc);
-	print_cap("ICC Over Clocking", cap->icc_over_clocking);
-	print_cap("Protected Audio Video Path (PAVP)", cap->pavp);
-	print_cap("IPV6", cap->ipv6);
-	print_cap("KVM Remote Control (KVM)", cap->kvm);
-	print_cap("Outbreak Containment Heuristic (OCH)", cap->och);
-	print_cap("Virtual LAN (VLAN)", cap->vlan);
-	print_cap("TLS", cap->tls);
-	print_cap("Wireless LAN (WLAN)", cap->wlan);
-}
+#ifdef __SIMPLE_DEVICE__

/* Send END OF POST message to the ME */
-static int __unused mkhi_end_of_post(void)
+static int mkhi_end_of_post(void)
 {
    struct mkhi_header mkhi = {
    	.group_id	= MKHI_GROUP_ID_GEN,
@@ -535,8 +458,6 @@
    return 0;
 }

-#ifdef __SIMPLE_DEVICE__
-
 void intel_me_finalize_smm(void)
 {
    struct me_hfs hfs;
@@ -768,94 +689,6 @@
    return 0;
 }

-/* Check whether ME is present and do basic init */
-static void intel_me_init(struct device *dev)
-{
-	struct southbridge_intel_lynxpoint_config *config = dev->chip_info;
-	me_bios_path path = intel_me_path(dev);
-	me_bios_payload mbp_data;
-
-	/* Do initial setup and determine the BIOS path */
-	printk(BIOS_NOTICE, "ME: BIOS path: %s\n", me_bios_path_values[path]);
-
-	if (path == ME_NORMAL_BIOS_PATH) {
-		/* Validate the extend register */
-		intel_me_extend_valid(dev);
-	}
-
-	memset(&mbp_data, 0, sizeof(mbp_data));
-
-	/*
-	 * According to the ME9 BWG, BIOS is required to fetch MBP data in
-	 * all boot flows except S3 Resume.
-	 */
-
-	/* Prepare MEI MMIO interface */
-	if (intel_mei_setup(dev) < 0)
-		return;
-
-	if (intel_me_read_mbp(&mbp_data, dev))
-		return;
-
-	if (CONFIG_DEFAULT_CONSOLE_LOGLEVEL >= BIOS_DEBUG) {
-		me_print_fw_version(mbp_data.fw_version_name);
-
-		if (CONFIG(DEBUG_INTEL_ME))
-			me_print_fwcaps(mbp_data.fw_capabilities);
-
-		if (mbp_data.plat_time) {
-			printk(BIOS_DEBUG, "ME: Wake Event to ME Reset:      %u ms\n",
-			       mbp_data.plat_time->wake_event_mrst_time_ms);
-			printk(BIOS_DEBUG, "ME: ME Reset to Platform Reset:  %u ms\n",
-			       mbp_data.plat_time->mrst_pltrst_time_ms);
-			printk(BIOS_DEBUG, "ME: Platform Reset to CPU Reset: %u ms\n",
-			       mbp_data.plat_time->pltrst_cpurst_time_ms);
-		}
-	}
-
-	/* Set clock enables according to devicetree */
-	if (config && config->icc_clock_disable)
-		me_icc_set_clock_enables(config->icc_clock_disable);
-
-	/*
-	 * Leave the ME unlocked. It will be locked via SMI command later.
-	 */
-}
-
-static void intel_me_enable(struct device *dev)
-{
-	/* Avoid talking to the device in S3 path */
-	if (acpi_is_wakeup_s3()) {
-		dev->enabled = 0;
-		pch_disable_devfn(dev);
-	}
-}
-
-static struct device_operations device_ops = {
-	.read_resources		= pci_dev_read_resources,
-	.set_resources		= pci_dev_set_resources,
-	.enable_resources	= pci_dev_enable_resources,
-	.enable			= intel_me_enable,
-	.init			= intel_me_init,
-	.ops_pci		= &pci_dev_ops_pci,
-};
-
-static const unsigned short pci_device_ids[] = {
-	0x8c3a, /* Mobile */
-	0x9c3a, /* Low Power */
-	0
-};
-
-static const struct pci_driver intel_me __pci_driver = {
-	.ops     = &device_ops,
-	.vendor  = PCI_VENDOR_ID_INTEL,
-	.devices = pci_device_ids,
-};
-
-#endif /* !__SIMPLE_DEVICE__ */
-
-/******************************************************************************
- *									     */
 static u32 me_to_host_words_pending(void)
 {
    struct mei_csr me;
@@ -875,7 +708,7 @@
  * mbp seems to be following its own flow, let's retrieve it in a dedicated
  * function.
  */
-static int __unused intel_me_read_mbp(me_bios_payload *mbp_data, struct device *dev)
+static int intel_me_read_mbp(me_bios_payload *mbp_data, struct device *dev)
 {
    mbp_header mbp_hdr;
    u32 me2host_pending;
@@ -884,11 +717,7 @@
    struct mbp_payload *mbp;
    int i;

-#ifdef __SIMPLE_DEVICE__
-	pci_read_dword_ptr(PCI_BDF(dev), &hfs2, PCI_ME_HFS2);
-#else
    pci_read_dword_ptr(dev, &hfs2, PCI_ME_HFS2);
-#endif

if (!hfs2.mbp_rdy) {
    	printk(BIOS_ERR, "ME: MBP not ready\n");
@@ -998,10 +827,168 @@
    return 0;

mbp_failure:
-#ifdef __SIMPLE_DEVICE__
-	intel_me_mbp_give_up(PCI_BDF(dev));
-#else
    intel_me_mbp_give_up(dev);
-#endif
+
    return -1;
 }
+
+static void me_print_fw_version(mbp_fw_version_name *vers_name)
+{
+	if (!vers_name) {
+		printk(BIOS_ERR, "ME: mbp missing version report\n");
+		return;
+	}
+
+	printk(BIOS_DEBUG, "ME: found version %d.%d.%d.%d\n",
+	       vers_name->major_version, vers_name->minor_version,
+	       vers_name->hotfix_version, vers_name->build_version);
+}
+
+static inline void print_cap(const char *name, int state)
+{
+	printk(BIOS_DEBUG, "ME Capability: %-41s : %sabled\n",
+	       name, state ? " en" : "dis");
+}
+
+/* Get ME Firmware Capabilities */
+static int mkhi_get_fwcaps(mbp_mefwcaps *cap)
+{
+	u32 rule_id = 0;
+	struct me_fwcaps cap_msg;
+	struct mkhi_header mkhi = {
+		.group_id       = MKHI_GROUP_ID_FWCAPS,
+		.command        = MKHI_FWCAPS_GET_RULE,
+	};
+
+	/* Send request and wait for response */
+	if (mei_sendrecv_mkhi(&mkhi, &rule_id, sizeof(u32),
+			      &cap_msg, sizeof(cap_msg)) < 0) {
+		printk(BIOS_ERR, "ME: GET FWCAPS message failed\n");
+		return -1;
+	}
+	*cap = cap_msg.caps_sku;
+	return 0;
+}
+
+/* Get ME Firmware Capabilities */
+static void me_print_fwcaps(mbp_mefwcaps *cap)
+{
+	mbp_mefwcaps local_caps;
+	if (!cap) {
+		cap = &local_caps;
+		printk(BIOS_ERR, "ME: mbp missing fwcaps report\n");
+		if (mkhi_get_fwcaps(cap))
+			return;
+	}
+
+	print_cap("Full Network manageability", cap->full_net);
+	print_cap("Regular Network manageability", cap->std_net);
+	print_cap("Manageability", cap->manageability);
+	print_cap("IntelR Anti-Theft (AT)", cap->intel_at);
+	print_cap("IntelR Capability Licensing Service (CLS)", cap->intel_cls);
+	print_cap("IntelR Power Sharing Technology (MPC)", cap->intel_mpc);
+	print_cap("ICC Over Clocking", cap->icc_over_clocking);
+	print_cap("Protected Audio Video Path (PAVP)", cap->pavp);
+	print_cap("IPV6", cap->ipv6);
+	print_cap("KVM Remote Control (KVM)", cap->kvm);
+	print_cap("Outbreak Containment Heuristic (OCH)", cap->och);
+	print_cap("Virtual LAN (VLAN)", cap->vlan);
+	print_cap("TLS", cap->tls);
+	print_cap("Wireless LAN (WLAN)", cap->wlan);
+}
+
+/* Path that the BIOS should take based on ME state */
+static const char *const me_bios_path_values[] = {
+	[ME_NORMAL_BIOS_PATH]		= "Normal",
+	[ME_S3WAKE_BIOS_PATH]		= "S3 Wake",
+	[ME_ERROR_BIOS_PATH]		= "Error",
+	[ME_RECOVERY_BIOS_PATH]		= "Recovery",
+	[ME_DISABLE_BIOS_PATH]		= "Disable",
+	[ME_FIRMWARE_UPDATE_BIOS_PATH]	= "Firmware Update",
+};
+
+/* Check whether ME is present and do basic init */
+static void intel_me_init(struct device *dev)
+{
+	struct southbridge_intel_lynxpoint_config *config = dev->chip_info;
+	me_bios_path path = intel_me_path(dev);
+	me_bios_payload mbp_data;
+
+	/* Do initial setup and determine the BIOS path */
+	printk(BIOS_NOTICE, "ME: BIOS path: %s\n", me_bios_path_values[path]);
+
+	if (path == ME_NORMAL_BIOS_PATH) {
+		/* Validate the extend register */
+		intel_me_extend_valid(dev);
+	}
+
+	memset(&mbp_data, 0, sizeof(mbp_data));
+
+	/*
+	 * According to the ME9 BWG, BIOS is required to fetch MBP data in
+	 * all boot flows except S3 Resume.
+	 */
+
+	/* Prepare MEI MMIO interface */
+	if (intel_mei_setup(dev) < 0)
+		return;
+
+	if (intel_me_read_mbp(&mbp_data, dev))
+		return;
+
+	if (CONFIG_DEFAULT_CONSOLE_LOGLEVEL >= BIOS_DEBUG) {
+		me_print_fw_version(mbp_data.fw_version_name);
+
+		if (CONFIG(DEBUG_INTEL_ME))
+			me_print_fwcaps(mbp_data.fw_capabilities);
+
+		if (mbp_data.plat_time) {
+			printk(BIOS_DEBUG, "ME: Wake Event to ME Reset:      %u ms\n",
+			       mbp_data.plat_time->wake_event_mrst_time_ms);
+			printk(BIOS_DEBUG, "ME: ME Reset to Platform Reset:  %u ms\n",
+			       mbp_data.plat_time->mrst_pltrst_time_ms);
+			printk(BIOS_DEBUG, "ME: Platform Reset to CPU Reset: %u ms\n",
+			       mbp_data.plat_time->pltrst_cpurst_time_ms);
+		}
+	}
+
+	/* Set clock enables according to devicetree */
+	if (config && config->icc_clock_disable)
+		me_icc_set_clock_enables(config->icc_clock_disable);
+
+	/*
+	 * Leave the ME unlocked. It will be locked via SMI command later.
+	 */
+}
+
+static void intel_me_enable(struct device *dev)
+{
+	/* Avoid talking to the device in S3 path */
+	if (acpi_is_wakeup_s3()) {
+		dev->enabled = 0;
+		pch_disable_devfn(dev);
+	}
+}
+
+static struct device_operations device_ops = {
+	.read_resources		= pci_dev_read_resources,
+	.set_resources		= pci_dev_set_resources,
+	.enable_resources	= pci_dev_enable_resources,
+	.enable			= intel_me_enable,
+	.init			= intel_me_init,
+	.ops_pci		= &pci_dev_ops_pci,
+};
+
+static const unsigned short pci_device_ids[] = {
+	0x8c3a, /* Mobile */
+	0x9c3a, /* Low Power */
+	0
+};
+
+static const struct pci_driver intel_me __pci_driver = {
+	.ops     = &device_ops,
+	.vendor  = PCI_VENDOR_ID_INTEL,
+	.devices = pci_device_ids,
+};
+
+#endif /* !__SIMPLE_DEVICE__ */

[Angel Pons (Code Review)]

Hello Patrick Rudolph,

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

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

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

Change subject: sb/intel/lynxpoint/me_9.x.c: Reorder functions
......................................................................

sb/intel/lynxpoint/me_9.x.c: Reorder functions

This file is shared between ramstage and smm. However, not everything is
used in both ramstage and smm, so there's a lot of `__unused` tags. So,
reorder the functions so that they build only for stages needing them.

Tested with BUILD_TIMELESS=1, Asrock B85M Pro4 remains identical.

Change-Id: I3143f07ed845e0c6b1444817029a437db3b959e3
Signed-off-by: Angel Pons 
---
M src/southbridge/intel/lynxpoint/me_9.x.c
1 file changed, 165 insertions(+), 179 deletions(-)

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

[Angel Pons (Code Review)]

Hello build bot (Jenkins), Patrick Rudolph,

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

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

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

Change subject: sb/intel/lynxpoint/me_9.x.c: Reorder functions
......................................................................

sb/intel/lynxpoint/me_9.x.c: Reorder functions

This file is shared between ramstage and smm. However, not everything is
used in both ramstage and smm, so there's a lot of `__unused` tags. So,
reorder the functions so that they build only for stages needing them.

Tested with BUILD_TIMELESS=1, Asrock B85M Pro4 remains identical.

Change-Id: I3143f07ed845e0c6b1444817029a437db3b959e3
Signed-off-by: Angel Pons 
---
M src/southbridge/intel/lynxpoint/me_9.x.c
1 file changed, 165 insertions(+), 179 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/42631/4

Change in coreboot[master]: sb/intel/lynxpoint/me.c: Reorder functions

[Angel Pons (Code Review)]

Hello build bot (Jenkins), Patrick Rudolph,

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

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

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

Change subject: sb/intel/lynxpoint/me.c: Reorder functions
......................................................................

sb/intel/lynxpoint/me.c: Reorder functions

Rearrange the code to ease comparing against Broadwell.

Tested with BUILD_TIMELESS=1, Asrock B85M Pro4 remains identical.

Change-Id: I3143f07ed845e0c6b1444817029a437db3b959e3
Signed-off-by: Angel Pons 
---
M src/southbridge/intel/lynxpoint/me.c
1 file changed, 114 insertions(+), 115 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/42631/5

Change in coreboot[master]: sb/intel/lynxpoint/me.c: Reorder functions

[Nico Huber (Code Review)]

Attention is currently required from: Angel Pons, Arthur Heymans.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42631 )

Change subject: sb/intel/lynxpoint/me.c: Reorder functions
......................................................................

Patch Set 5: Code-Review+1

(1 comment)

File src/southbridge/intel/lynxpoint/me.c:

https://review.coreboot.org/c/coreboot/+/42631/comment/309f09cf_be4d05a1 
PS5, Line 758:  * Return 1 to indicate success (send LOCK+EOP with NOACK)
I see no such thing?

Change in coreboot[master]: sb/intel/lynxpoint/me.c: Reorder functions

[Angel Pons (Code Review)]

Attention is currently required from: Nico Huber, Arthur Heymans.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42631 )

Change subject: sb/intel/lynxpoint/me.c: Reorder functions
......................................................................

Patch Set 5:

(1 comment)

File src/southbridge/intel/lynxpoint/me.c:

https://review.coreboot.org/c/coreboot/+/42631/comment/fa0b53f9_751b43f5 
PS5, Line 758:  * Return 1 to indicate success (send LOCK+EOP with NOACK)
> I see no such thing?

Yeah... Looks like Broadwell does things differently there. I'll revise.

Change in coreboot[master]: sb/intel/lynxpoint/me.c: Reorder functions

[Angel Pons (Code Review)]

Attention is currently required from: Nico Huber, Arthur Heymans.
Hello build bot (Jenkins), Nico Huber, Arthur Heymans, Patrick Rudolph,

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

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

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

Change subject: sb/intel/lynxpoint/me.c: Reorder functions
......................................................................

sb/intel/lynxpoint/me.c: Reorder functions

Rearrange the code to ease comparing against Broadwell.

Tested with BUILD_TIMELESS=1, Asrock B85M Pro4 remains identical.

Change-Id: I3143f07ed845e0c6b1444817029a437db3b959e3
Signed-off-by: Angel Pons 
---
M src/southbridge/intel/lynxpoint/me.c
1 file changed, 113 insertions(+), 115 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/42631/7

Change in coreboot[master]: sb/intel/lynxpoint/me.c: Reorder functions

[Angel Pons (Code Review)]

Attention is currently required from: Nico Huber, Arthur Heymans.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42631 )

Change subject: sb/intel/lynxpoint/me.c: Reorder functions
......................................................................

Patch Set 7:

(1 comment)

File src/southbridge/intel/lynxpoint/me.c:

https://review.coreboot.org/c/coreboot/+/42631/comment/8d2fd4d0_e796d437 
PS5, Line 758:  * Return 1 to indicate success (send LOCK+EOP with NOACK)
> Yeah... Looks like Broadwell does things differently there. I'll revise.

Done

Change in coreboot[master]: sb/intel/lynxpoint/me.c: Reorder functions

[Nico Huber (Code Review)]

Attention is currently required from: Angel Pons, Arthur Heymans.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42631 )

Change subject: sb/intel/lynxpoint/me.c: Reorder functions
......................................................................

Patch Set 7: Code-Review+2

Change in coreboot[master]: sb/intel/lynxpoint/me.c: Reorder functions

[Nico Huber (Code Review)]

Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42631 )

Change subject: sb/intel/lynxpoint/me.c: Reorder functions
......................................................................

sb/intel/lynxpoint/me.c: Reorder functions

Rearrange the code to ease comparing against Broadwell.

Tested with BUILD_TIMELESS=1, Asrock B85M Pro4 remains identical.

Change-Id: I3143f07ed845e0c6b1444817029a437db3b959e3
Signed-off-by: Angel Pons 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/42631
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
---
M src/southbridge/intel/lynxpoint/me.c
1 file changed, 113 insertions(+), 115 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved

diff --git a/src/southbridge/intel/lynxpoint/me.c b/src/southbridge/intel/lynxpoint/me.c
index d7b600a..b028c63 100644
--- a/src/southbridge/intel/lynxpoint/me.c
+++ b/src/southbridge/intel/lynxpoint/me.c
@@ -36,7 +36,6 @@
    [ME_DISABLE_BIOS_PATH]		= "Disable",
    [ME_FIRMWARE_UPDATE_BIOS_PATH]	= "Firmware Update",
 };
-static int intel_me_read_mbp(me_bios_payload *mbp_data, struct device *dev);

/* MMIO base address for MEI interface */
 static u8 *mei_base_address;
@@ -388,6 +387,30 @@
    return 0;
 }

+static inline int mei_sendrecv_icc(struct icc_header *icc,
+				   void *req_data, int req_bytes,
+				   void *rsp_data, int rsp_bytes)
+{
+	struct icc_header icc_rsp;
+
+	/* Send header */
+	if (mei_send_header(MEI_ADDRESS_ICC, MEI_HOST_ADDRESS,
+			    icc, sizeof(*icc), req_bytes ? 0 : 1) < 0)
+		return -1;
+
+	/* Send data if available */
+	if (req_bytes && mei_send_data(MEI_ADDRESS_ICC, MEI_HOST_ADDRESS,
+				       req_data, req_bytes) < 0)
+		return -1;
+
+	/* Read header and data, if needed */
+	if (rsp_bytes && mei_recv_msg(&icc_rsp, sizeof(icc_rsp),
+				      rsp_data, rsp_bytes) < 0)
+		return -1;
+
+	return 0;
+}
+
 /*
  * mbp give up routine. This path is taken if hfs.mpb_rdy is 0 or the read
  * state machine on the BIOS end doesn't match the ME's state machine.
@@ -550,30 +573,6 @@
    RCBA32_OR(FD2, PCH_DISABLE_MEI1);
 }

-static inline int mei_sendrecv_icc(struct icc_header *icc,
-				   void *req_data, int req_bytes,
-				   void *rsp_data, int rsp_bytes)
-{
-	struct icc_header icc_rsp;
-
-	/* Send header */
-	if (mei_send_header(MEI_ADDRESS_ICC, MEI_HOST_ADDRESS,
-			    icc, sizeof(*icc), req_bytes ? 0 : 1) < 0)
-		return -1;
-
-	/* Send data if available */
-	if (req_bytes && mei_send_data(MEI_ADDRESS_ICC, MEI_HOST_ADDRESS,
-				       req_data, req_bytes) < 0)
-		return -1;
-
-	/* Read header and data, if needed */
-	if (rsp_bytes && mei_recv_msg(&icc_rsp, sizeof(icc_rsp),
-				      rsp_data, rsp_bytes) < 0)
-		return -1;
-
-	return 0;
-}
-
 static int me_icc_set_clock_enables(u32 mask)
 {
    struct icc_clock_enables_msg clk = {
@@ -592,7 +591,6 @@
    	printk(BIOS_ERR, "ME: ICC SET CLOCK ENABLES message failed\n");
    	return -1;
    }
-
    printk(BIOS_INFO, "ME: ICC SET CLOCK ENABLES 0x%08x\n", mask);
    return 0;
 }
@@ -738,93 +736,6 @@
    return 0;
 }

-/* Check whether ME is present and do basic init */
-static void intel_me_init(struct device *dev)
-{
-	struct southbridge_intel_lynxpoint_config *config = dev->chip_info;
-	me_bios_path path = intel_me_path(dev);
-	me_bios_payload mbp_data;
-
-	/* Do initial setup and determine the BIOS path */
-	printk(BIOS_NOTICE, "ME: BIOS path: %s\n", me_bios_path_values[path]);
-
-	if (path == ME_NORMAL_BIOS_PATH) {
-		/* Validate the extend register */
-		intel_me_extend_valid(dev);
-	}
-
-	memset(&mbp_data, 0, sizeof(mbp_data));
-
-	/*
-	 * According to the ME9 BWG, BIOS is required to fetch MBP data in
-	 * all boot flows except S3 Resume.
-	 */
-
-	/* Prepare MEI MMIO interface */
-	if (intel_mei_setup(dev) < 0)
-		return;
-
-	if (intel_me_read_mbp(&mbp_data, dev))
-		return;
-
-	if (CONFIG_DEFAULT_CONSOLE_LOGLEVEL >= BIOS_DEBUG) {
-		me_print_fw_version(mbp_data.fw_version_name);
-
-		if (CONFIG(DEBUG_INTEL_ME))
-			me_print_fwcaps(mbp_data.fw_capabilities);
-
-		if (mbp_data.plat_time) {
-			printk(BIOS_DEBUG, "ME: Wake Event to ME Reset:      %u ms\n",
-			       mbp_data.plat_time->wake_event_mrst_time_ms);
-			printk(BIOS_DEBUG, "ME: ME Reset to Platform Reset:  %u ms\n",
-			       mbp_data.plat_time->mrst_pltrst_time_ms);
-			printk(BIOS_DEBUG, "ME: Platform Reset to CPU Reset: %u ms\n",
-			       mbp_data.plat_time->pltrst_cpurst_time_ms);
-		}
-	}
-
-	/* Set clock enables according to devicetree */
-	if (config && config->icc_clock_disable)
-		me_icc_set_clock_enables(config->icc_clock_disable);
-
-	/*
-	 * Leave the ME unlocked. It will be locked later.
-	 */
-}
-
-static void intel_me_enable(struct device *dev)
-{
-	/* Avoid talking to the device in S3 path */
-	if (acpi_is_wakeup_s3()) {
-		dev->enabled = 0;
-		pch_disable_devfn(dev);
-	}
-}
-
-static struct device_operations device_ops = {
-	.read_resources		= pci_dev_read_resources,
-	.set_resources		= pci_dev_set_resources,
-	.enable_resources	= pci_dev_enable_resources,
-	.enable			= intel_me_enable,
-	.init			= intel_me_init,
-	.final			= intel_me_finalize,
-	.ops_pci		= &pci_dev_ops_pci,
-};
-
-static const unsigned short pci_device_ids[] = {
-	PCI_DEVICE_ID_INTEL_LPT_H_MEI,
-	PCI_DEVICE_ID_INTEL_LPT_LP_MEI,
-	0
-};
-
-static const struct pci_driver intel_me __pci_driver = {
-	.ops     = &device_ops,
-	.vendor  = PCI_VENDOR_ID_INTEL,
-	.devices = pci_device_ids,
-};
-
-/******************************************************************************
- *									     */
 static u32 me_to_host_words_pending(void)
 {
    struct mei_csr me;
@@ -841,8 +752,10 @@
 };

/*
- * mbp seems to be following its own flow, let's retrieve it in a dedicated
- * function.
+ * Read and print ME MBP data
+ *
+ * Return -1 to indicate a problem (give up)
+ * Return 0 to indicate success (send LOCK+EOP)
  */
 static int intel_me_read_mbp(me_bios_payload *mbp_data, struct device *dev)
 {
@@ -961,3 +874,88 @@
    intel_me_mbp_give_up(dev);
    return -1;
 }
+
+/* Check whether ME is present and do basic init */
+static void intel_me_init(struct device *dev)
+{
+	struct southbridge_intel_lynxpoint_config *config = dev->chip_info;
+	me_bios_path path = intel_me_path(dev);
+	me_bios_payload mbp_data;
+
+	/* Do initial setup and determine the BIOS path */
+	printk(BIOS_NOTICE, "ME: BIOS path: %s\n", me_bios_path_values[path]);
+
+	if (path == ME_NORMAL_BIOS_PATH) {
+		/* Validate the extend register */
+		intel_me_extend_valid(dev);
+	}
+
+	memset(&mbp_data, 0, sizeof(mbp_data));
+
+	/*
+	 * According to the ME9 BWG, BIOS is required to fetch MBP data in
+	 * all boot flows except S3 Resume.
+	 */
+
+	/* Prepare MEI MMIO interface */
+	if (intel_mei_setup(dev) < 0)
+		return;
+
+	if (intel_me_read_mbp(&mbp_data, dev))
+		return;
+
+	if (CONFIG_DEFAULT_CONSOLE_LOGLEVEL >= BIOS_DEBUG) {
+		me_print_fw_version(mbp_data.fw_version_name);
+
+		if (CONFIG(DEBUG_INTEL_ME))
+			me_print_fwcaps(mbp_data.fw_capabilities);
+
+		if (mbp_data.plat_time) {
+			printk(BIOS_DEBUG, "ME: Wake Event to ME Reset:      %u ms\n",
+			       mbp_data.plat_time->wake_event_mrst_time_ms);
+			printk(BIOS_DEBUG, "ME: ME Reset to Platform Reset:  %u ms\n",
+			       mbp_data.plat_time->mrst_pltrst_time_ms);
+			printk(BIOS_DEBUG, "ME: Platform Reset to CPU Reset: %u ms\n",
+			       mbp_data.plat_time->pltrst_cpurst_time_ms);
+		}
+	}
+
+	/* Set clock enables according to devicetree */
+	if (config && config->icc_clock_disable)
+		me_icc_set_clock_enables(config->icc_clock_disable);
+
+	/*
+	 * Leave the ME unlocked. It will be locked later.
+	 */
+}
+
+static void intel_me_enable(struct device *dev)
+{
+	/* Avoid talking to the device in S3 path */
+	if (acpi_is_wakeup_s3()) {
+		dev->enabled = 0;
+		pch_disable_devfn(dev);
+	}
+}
+
+static struct device_operations device_ops = {
+	.read_resources		= pci_dev_read_resources,
+	.set_resources		= pci_dev_set_resources,
+	.enable_resources	= pci_dev_enable_resources,
+	.enable			= intel_me_enable,
+	.init			= intel_me_init,
+	.final			= intel_me_finalize,
+	.ops_pci		= &pci_dev_ops_pci,
+};
+
+static const unsigned short pci_device_ids[] = {
+	PCI_DEVICE_ID_INTEL_LPT_H_MEI,
+	PCI_DEVICE_ID_INTEL_LPT_LP_MEI,
+	0
+};
+
+static const struct pci_driver intel_me __pci_driver = {
+	.ops     = &device_ops,
+	.vendor  = PCI_VENDOR_ID_INTEL,
+	.devices = pci_device_ids,
+};

[M] Change in coreboot[master]: sb/intel/lynxpoint/me.c: Reorder functions

[Elyes Haouas (Code Review)]

Attention is currently required from: Angel Pons.

Elyes Haouas has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42631 )

Change subject: sb/intel/lynxpoint/me.c: Reorder functions
......................................................................

Patch Set 8:

(1 comment)

File src/southbridge/intel/lynxpoint/me.c:

https://review.coreboot.org/c/coreboot/+/42631/comment/3085c772_8e7b597b 
PS8, Line 402: mei_send_data
This function returns always 0, isn't it?

[M] Change in coreboot[master]: sb/intel/lynxpoint/me.c: Reorder functions

[Angel Pons (Code Review)]

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

Change subject: sb/intel/lynxpoint/me.c: Reorder functions
......................................................................

Patch Set 8:

(1 comment)

File src/southbridge/intel/lynxpoint/me.c:

https://review.coreboot.org/c/coreboot/+/42631/comment/a61590e8_2e21fcf7 
PS8, Line 402: mei_send_data
> This function returns always 0, isn't it?

Not sure why.

[M] Change in coreboot[master]: sb/intel/lynxpoint/me.c: Reorder functions

[Elyes Haouas (Code Review)]

Attention is currently required from: Angel Pons.

Elyes Haouas has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42631 )

Change subject: sb/intel/lynxpoint/me.c: Reorder functions
......................................................................

Patch Set 8:

(1 comment)

File src/southbridge/intel/lynxpoint/me.c:

https://review.coreboot.org/c/coreboot/+/42631/comment/65fa9343_ca898df0 
PS8, Line 402: mei_send_data
> Not sure why.

line #258

[M] Change in coreboot[master]: sb/intel/lynxpoint/me.c: Reorder functions

[Angel Pons (Code Review)]

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

Change subject: sb/intel/lynxpoint/me.c: Reorder functions
......................................................................

Patch Set 8:

(1 comment)

File src/southbridge/intel/lynxpoint/me.c:

https://review.coreboot.org/c/coreboot/+/42631/comment/2fadf63c_7e46b30a 
PS8, Line 402: mei_send_data
> line #258

Sorry for being too succint. No idea why the function never returns an error, nor whether it should return errors in certain cases.