Change in coreboot[master]: drivers/ipmi: Add option to not query BMC

[Patrick Rudolph (Code Review)]

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

Change subject: drivers/ipmi: Add option to not query BMC
......................................................................

drivers/ipmi: Add option to not query BMC

The BMC on SuperMicro X11SSH takes 34seconds to start the IPMI KCS, bu
the default timeout of the IPMI KCS code is just 100msec.

To not wait additional 34seconds on boot, add the possibility to
hardcode the IPMI version in devicetree.

Tested on SuperMicro X11SSH.
The IPMI driver doesn't fail with an timeout any more.

Change-Id: I22c6885eae6fd7c778ac37b18f95b8775e9064e3
Signed-off-by: Patrick Rudolph 
---
M src/drivers/ipmi/chip.h
M src/drivers/ipmi/ipmi_kcs_ops.c
2 files changed, 25 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/34569/1

diff --git a/src/drivers/ipmi/chip.h b/src/drivers/ipmi/chip.h
index eb8b4e6..2123c76 100644
--- a/src/drivers/ipmi/chip.h
+++ b/src/drivers/ipmi/chip.h
@@ -24,6 +24,15 @@
    u8 gpe_interrupt;
    u8 have_apic;
    u32 apic_interrupt;
+	/* Do not send IPMI_BMC_GET_DEVICE_ID to BMC, but used hardcoded
+	 * IPMI version instead.
+	 * This can be used if the BMC takes a long time to boot:
+	 * AST2400 on SuperMicro X11SSH: 34seconds
+	 */
+	u8 no_detect_ipmi_version;
+	/* The IPMI version to advertise of 'no_detect_ipmi_version' is set */
+	u8 ipmi_revision_major;
+	u8 ipmi_revision_minor;
 };

#endif /* _IMPI_CHIP_H_ */
diff --git a/src/drivers/ipmi/ipmi_kcs_ops.c b/src/drivers/ipmi/ipmi_kcs_ops.c
index 0cc4e0a..95864e8 100644
--- a/src/drivers/ipmi/ipmi_kcs_ops.c
+++ b/src/drivers/ipmi/ipmi_kcs_ops.c
@@ -62,12 +62,27 @@
 {
    struct ipmi_devid_rsp rsp;
    uint32_t man_id = 0, prod_id = 0;
+	struct drivers_ipmi_config *conf = NULL;

if (!dev->enabled)
    	return;

+	printk(BIOS_DEBUG, "IPMI: PNP KCS 0x%x\n", dev->path.pnp.port);
+
+	if (dev->chip_info)
+		conf = dev->chip_info;
+
    /* Get IPMI version for ACPI and SMBIOS */
-	if (!ipmi_get_device_id(dev, &rsp)) {
+	if (conf && conf->no_detect_ipmi_version) {
+		printk(BIOS_INFO, "IPMI: Assuming BMC is installed\n");
+
+		ipmi_revision_major = conf->ipmi_revision_major;
+		ipmi_revision_minor = conf->ipmi_revision_minor;
+
+		printk(BIOS_INFO, "IPMI: Version %01x.%01x\n",
+		       ipmi_revision_major, ipmi_revision_minor);
+	} else if (!ipmi_get_device_id(dev, &rsp)) {
+		/* Queried the IPMI revision from BMC */
    	ipmi_revision_minor = IPMI_IPMI_VERSION_MINOR(rsp.ipmi_version);
    	ipmi_revision_major = IPMI_IPMI_VERSION_MAJOR(rsp.ipmi_version);

[Paul Menzel (Code Review)]

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

Change subject: drivers/ipmi: Add option to not query BMC
......................................................................

Patch Set 1:

(17 comments)

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

https://review.coreboot.org/c/coreboot/+/34569/1//COMMIT_MSG@9 
PS1, Line 9: 34seconds
Please add a space before the unit.

https://review.coreboot.org/c/coreboot/+/34569/1//COMMIT_MSG@9 
PS1, Line 9: SuperMicro
Supermicro

https://review.coreboot.org/c/coreboot/+/34569/1//COMMIT_MSG@9 
PS1, Line 9: bu
but

https://review.coreboot.org/c/coreboot/+/34569/1//COMMIT_MSG@10 
PS1, Line 10: timeout
time-out

https://review.coreboot.org/c/coreboot/+/34569/1//COMMIT_MSG@10 
PS1, Line 10: 100msec
Space: 100 msec

https://review.coreboot.org/c/coreboot/+/34569/1//COMMIT_MSG@12 
PS1, Line 12: 34seconds
34 s

https://review.coreboot.org/c/coreboot/+/34569/1//COMMIT_MSG@15 
PS1, Line 15: SuperMicro
Supermicro

https://review.coreboot.org/c/coreboot/+/34569/1//COMMIT_MSG@16 
PS1, Line 16: timeout
time-out

https://review.coreboot.org/c/coreboot/+/34569/1//COMMIT_MSG@16 
PS1, Line 16: an
a

https://review.coreboot.org/c/coreboot/+/34569/1/src/drivers/ipmi/chip.h 
File src/drivers/ipmi/chip.h:

https://review.coreboot.org/c/coreboot/+/34569/1/src/drivers/ipmi/chip.h@27 
PS1, Line 27: used
use

https://review.coreboot.org/c/coreboot/+/34569/1/src/drivers/ipmi/chip.h@27 
PS1, Line 27: 	/* Do not send IPMI_BMC_GET_DEVICE_ID to BMC, but used hardcoded
Please use:

```
/*
 * …
 */
```

https://review.coreboot.org/c/coreboot/+/34569/1/src/drivers/ipmi/chip.h@30 
PS1, Line 30: SuperMicro
Supermicro

https://review.coreboot.org/c/coreboot/+/34569/1/src/drivers/ipmi/chip.h@30 
PS1, Line 30: 34seconds
34 s

https://review.coreboot.org/c/coreboot/+/34569/1/src/drivers/ipmi/chip.h@32 
PS1, Line 32: u8
bool?

https://review.coreboot.org/c/coreboot/+/34569/1/src/drivers/ipmi/chip.h@33 
PS1, Line 33: of
if

https://review.coreboot.org/c/coreboot/+/34569/1/src/drivers/ipmi/ipmi_kcs_o... 
File src/drivers/ipmi/ipmi_kcs_ops.c:

https://review.coreboot.org/c/coreboot/+/34569/1/src/drivers/ipmi/ipmi_kcs_o... 
PS1, Line 77: 		printk(BIOS_INFO, "IPMI: Assuming BMC is installed\n");
This could be added to the print line below.

https://review.coreboot.org/c/coreboot/+/34569/1/src/drivers/ipmi/ipmi_kcs_o... 
PS1, Line 83: 		       ipmi_revision_major, ipmi_revision_minor);
Too bad the line is duplicated in the next if-else branch. You could add a 
`return` in the else branch, and then move the printk statement out of the if else statement.

[Patrick Rudolph (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: drivers/ipmi: Add option to not query BMC
......................................................................

drivers/ipmi: Add option to not query BMC

The BMC on Supermicro X11SSH takes 34 seconds to start the IPMI KCS, but
the default timeout of the IPMI KCS code is just 100 msec.

To not wait additional 34 seconds on boot, add the possibility to
hardcode the IPMI version in devicetree.

Tested on Supermicro X11SSH.
The IPMI driver doesn't fail with a time-out any more.

Change-Id: I22c6885eae6fd7c778ac37b18f95b8775e9064e3
Signed-off-by: Patrick Rudolph 
---
M src/drivers/ipmi/chip.h
M src/drivers/ipmi/ipmi_kcs_ops.c
2 files changed, 26 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/34569/2

[Patrick Rudolph (Code Review)]

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

Change subject: drivers/ipmi: Add option to not query BMC
......................................................................

Patch Set 2:

(17 comments)

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

https://review.coreboot.org/c/coreboot/+/34569/1//COMMIT_MSG@9 
PS1, Line 9: bu
> but

Done

https://review.coreboot.org/c/coreboot/+/34569/1//COMMIT_MSG@9 
PS1, Line 9: 34seconds
> Please add a space before the unit.

Done

https://review.coreboot.org/c/coreboot/+/34569/1//COMMIT_MSG@9 
PS1, Line 9: SuperMicro
> Supermicro

Done

https://review.coreboot.org/c/coreboot/+/34569/1//COMMIT_MSG@10 
PS1, Line 10: 100msec
> Space: 100 msec

Done

https://review.coreboot.org/c/coreboot/+/34569/1//COMMIT_MSG@10 
PS1, Line 10: timeout
> time-out

Done

https://review.coreboot.org/c/coreboot/+/34569/1//COMMIT_MSG@12 
PS1, Line 12: 34seconds
> 34 s

Done

https://review.coreboot.org/c/coreboot/+/34569/1//COMMIT_MSG@15 
PS1, Line 15: SuperMicro
> Supermicro

Done

https://review.coreboot.org/c/coreboot/+/34569/1//COMMIT_MSG@16 
PS1, Line 16: timeout
> time-out

Done

https://review.coreboot.org/c/coreboot/+/34569/1//COMMIT_MSG@16 
PS1, Line 16: an
> a

Done

https://review.coreboot.org/c/coreboot/+/34569/1/src/drivers/ipmi/chip.h 
File src/drivers/ipmi/chip.h:

https://review.coreboot.org/c/coreboot/+/34569/1/src/drivers/ipmi/chip.h@27 
PS1, Line 27: used
> use

Done

https://review.coreboot.org/c/coreboot/+/34569/1/src/drivers/ipmi/chip.h@27 
PS1, Line 27: 	/* Do not send IPMI_BMC_GET_DEVICE_ID to BMC, but used hardcoded
> Please use: […]

according to the coding style guidelines it's ok to use.

https://review.coreboot.org/c/coreboot/+/34569/1/src/drivers/ipmi/chip.h@30 
PS1, Line 30: SuperMicro
> Supermicro

Done

https://review.coreboot.org/c/coreboot/+/34569/1/src/drivers/ipmi/chip.h@30 
PS1, Line 30: 34seconds
> 34 s

Done

https://review.coreboot.org/c/coreboot/+/34569/1/src/drivers/ipmi/chip.h@32 
PS1, Line 32: u8
> bool?

Done

https://review.coreboot.org/c/coreboot/+/34569/1/src/drivers/ipmi/chip.h@33 
PS1, Line 33: of
> if

Done

https://review.coreboot.org/c/coreboot/+/34569/1/src/drivers/ipmi/ipmi_kcs_o... 
File src/drivers/ipmi/ipmi_kcs_ops.c:

https://review.coreboot.org/c/coreboot/+/34569/1/src/drivers/ipmi/ipmi_kcs_o... 
PS1, Line 77: 		printk(BIOS_INFO, "IPMI: Assuming BMC is installed\n");
> This could be added to the print line below.

Done

https://review.coreboot.org/c/coreboot/+/34569/1/src/drivers/ipmi/ipmi_kcs_o... 
PS1, Line 83: 		       ipmi_revision_major, ipmi_revision_minor);
> Too bad the line is duplicated in the next if-else branch. You could add a  […]

Done

Change in coreboot[master]: drivers/ipmi: Add option to wait for BMC

[Patrick Rudolph (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: drivers/ipmi: Add option to wait for BMC
......................................................................

drivers/ipmi: Add option to wait for BMC

The BMC on Supermicro X11SSH takes 34 seconds to start the IPMI KCS, but
the default timeout of the IPMI KCS code is just 100 msec.

Add a configurable timeout option to wait for the BMC to become ready.
As it only should boot very long after power on reset, it's not a
problem on reset or warm boot.

Tested on Supermicro X11SSH.
The IPMI driver doesn't fail with a time-out any more.

Change-Id: I22c6885eae6fd7c778ac37b18f95b8775e9064e3
Signed-off-by: Patrick Rudolph 
---
M src/drivers/ipmi/chip.h
M src/drivers/ipmi/ipmi_kcs_ops.c
2 files changed, 37 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/34569/3

Change in coreboot[master]: drivers/ipmi: Add option to wait for BMC

[Philipp Deppenwiese (Code Review)]

Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34569 )

Change subject: drivers/ipmi: Add option to wait for BMC
......................................................................

Patch Set 3: Code-Review+2

Change in coreboot[master]: drivers/ipmi: Add option to wait for BMC

[Martin Roth (Code Review)]

Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34569 )

Change subject: drivers/ipmi: Add option to wait for BMC
......................................................................

drivers/ipmi: Add option to wait for BMC

The BMC on Supermicro X11SSH takes 34 seconds to start the IPMI KCS, but
the default timeout of the IPMI KCS code is just 100 msec.

Add a configurable timeout option to wait for the BMC to become ready.
As it only should boot very long after power on reset, it's not a
problem on reset or warm boot.

Tested on Supermicro X11SSH.
The IPMI driver doesn't fail with a time-out any more.

Change-Id: I22c6885eae6fd7c778ac37b18f95b8775e9064e3
Signed-off-by: Patrick Rudolph 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/34569
Tested-by: build bot (Jenkins) 
Reviewed-by: Philipp Deppenwiese 
---
M src/drivers/ipmi/chip.h
M src/drivers/ipmi/ipmi_kcs_ops.c
2 files changed, 37 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Philipp Deppenwiese: Looks good to me, approved

diff --git a/src/drivers/ipmi/chip.h b/src/drivers/ipmi/chip.h
index eb8b4e6..1c5afe7 100644
--- a/src/drivers/ipmi/chip.h
+++ b/src/drivers/ipmi/chip.h
@@ -24,6 +24,17 @@
    u8 gpe_interrupt;
    u8 have_apic;
    u32 apic_interrupt;
+	/*
+	 * Wait for BMC to boot.
+	 * This can be used if the BMC takes a long time to boot after PoR:
+	 * AST2400 on Supermicro X11SSH: 34 s
+	 */
+	bool wait_for_bmc;
+	/*
+	 * The timeout in seconds to wait for the IPMI service to be loaded.
+	 * Will be used if wait_for_bmc is true.
+	 */
+	u16 bmc_boot_timeout;
 };

#endif /* _IMPI_CHIP_H_ */
diff --git a/src/drivers/ipmi/ipmi_kcs_ops.c b/src/drivers/ipmi/ipmi_kcs_ops.c
index 0cc4e0a..21102bb 100644
--- a/src/drivers/ipmi/ipmi_kcs_ops.c
+++ b/src/drivers/ipmi/ipmi_kcs_ops.c
@@ -32,6 +32,7 @@
 #endif
 #include <version.h>
 #include <delay.h>
+#include <timer.h>
 #include "ipmi_kcs.h"
 #include "chip.h"

@@ -62,12 +63,37 @@
 {
    struct ipmi_devid_rsp rsp;
    uint32_t man_id = 0, prod_id = 0;
+	struct drivers_ipmi_config *conf = NULL;

if (!dev->enabled)
    	return;

+	printk(BIOS_DEBUG, "IPMI: PNP KCS 0x%x\n", dev->path.pnp.port);
+
+	if (dev->chip_info)
+		conf = dev->chip_info;
+
    /* Get IPMI version for ACPI and SMBIOS */
+	if (conf && conf->wait_for_bmc && conf->bmc_boot_timeout) {
+		struct stopwatch sw;
+		stopwatch_init_msecs_expire(&sw, conf->bmc_boot_timeout * 1000);
+		printk(BIOS_DEBUG, "IPMI: Waiting for BMC...\n");
+
+		while (!stopwatch_expired(&sw)) {
+			if (inb(dev->path.pnp.port) != 0xff)
+				break;
+			mdelay(100);
+		}
+		if (stopwatch_expired(&sw)) {
+			printk(BIOS_INFO, "IPMI: Waiting for BMC timed out\n");
+			/* Don't write tables if communication failed */
+			dev->enabled = 0;
+			return;
+		}
+	}
+
    if (!ipmi_get_device_id(dev, &rsp)) {
+		/* Queried the IPMI revision from BMC */
    	ipmi_revision_minor = IPMI_IPMI_VERSION_MINOR(rsp.ipmi_version);
    	ipmi_revision_major = IPMI_IPMI_VERSION_MAJOR(rsp.ipmi_version);