Johnny Lin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45103 )
Change subject: Add config IPMI_KCS_IO_TIMEOUT for IPMI KCS timeout value ......................................................................
Add config IPMI_KCS_IO_TIMEOUT for IPMI KCS timeout value
The current value of 1000 cycles of 100ns would see timeout occurs on OCP Delta Lake, especially when the log level is smaller than 8 because the prink(BIOS_SPEW, ..) in ipmi_kcs_status() creates delay and avoid the problem, but after setting log level to 4 we see the timeout occurs. Default remains as 1000.
Change-Id: I42ede1d9200bb5d0dbb455d2ff66e2816f10e86b Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/drivers/ipmi/Kconfig M src/drivers/ipmi/ipmi_kcs.c 2 files changed, 9 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/45103/1
diff --git a/src/drivers/ipmi/Kconfig b/src/drivers/ipmi/Kconfig index 44ed17e..f86f609 100644 --- a/src/drivers/ipmi/Kconfig +++ b/src/drivers/ipmi/Kconfig @@ -33,3 +33,10 @@ help The PNP base address of BMC KCS. It must be equal to the pnp port value defined in devicetree for chip drivers/ipmi. + +config IPMI_KCS_IO_TIMEOUT + int + default 1000 + depends on IPMI_KCS + help + The timeout time for a single KCS IO in the unit of 100 microseconds. diff --git a/src/drivers/ipmi/ipmi_kcs.c b/src/drivers/ipmi/ipmi_kcs.c index 1d6b71c..e1f6c75 100644 --- a/src/drivers/ipmi/ipmi_kcs.c +++ b/src/drivers/ipmi/ipmi_kcs.c @@ -35,7 +35,7 @@
static int wait_ibf_timeout(int port) { - int timeout = 1000; + int timeout = CONFIG_IPMI_KCS_IO_TIMEOUT; do { if (!(ipmi_kcs_status(port) & IPMI_KCS_IBF)) return 0; @@ -47,7 +47,7 @@
static int wait_obf_timeout(int port) { - int timeout = 1000; + int timeout = CONFIG_IPMI_KCS_IO_TIMEOUT; do { if ((ipmi_kcs_status(port) & IPMI_KCS_OBF)) return 0;
Johnny Lin has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/45103 )
Change subject: drivers/ipmi: Add config IPMI_KCS_IO_TIMEOUT for IPMI KCS timeout value ......................................................................
drivers/ipmi: Add config IPMI_KCS_IO_TIMEOUT for IPMI KCS timeout value
The current value of 1000 cycles of 100ns would see timeout occurs on OCP Delta Lake, especially when the log level is smaller than 8 because the prink(BIOS_SPEW, ..) in ipmi_kcs_status() creates delay and avoid the problem, but after setting log level to 4 we see the timeout occurs. Default remains as 1000.
Change-Id: I42ede1d9200bb5d0dbb455d2ff66e2816f10e86b Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/drivers/ipmi/Kconfig M src/drivers/ipmi/ipmi_kcs.c 2 files changed, 9 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/45103/2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45103 )
Change subject: drivers/ipmi: Add config IPMI_KCS_IO_TIMEOUT for IPMI KCS timeout value ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45103/2/src/drivers/ipmi/Kconfig File src/drivers/ipmi/Kconfig:
https://review.coreboot.org/c/coreboot/+/45103/2/src/drivers/ipmi/Kconfig@42 PS2, Line 42: The timeout time for a single KCS IO in the unit of 100 microseconds. If possible, please normalize the unit to μs or ms, and add the unit to the variable name.
https://review.coreboot.org/c/coreboot/+/45103/2/src/drivers/ipmi/ipmi_kcs.c File src/drivers/ipmi/ipmi_kcs.c:
https://review.coreboot.org/c/coreboot/+/45103/2/src/drivers/ipmi/ipmi_kcs.c... PS2, Line 43: } while (timeout--); Please rewrite using `wait_us()` or `wait_ms()` from `src/include/timer.h`.
Hello build bot (Jenkins), Patrick Rudolph, Jonathan Zhang, Jingle Hsu, Angel Pons, Morgan Jang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45103
to look at the new patch set (#3).
Change subject: drivers/ipmi: Add CONFIG_IPMI_KCS_TIMEOUT_MS for IPMI KCS timeout value ......................................................................
drivers/ipmi: Add CONFIG_IPMI_KCS_TIMEOUT_MS for IPMI KCS timeout value
With the current timeout of 1000 cycles of 100 microsecond would see timeout occurs on OCP Delta Lake if the log level is set to values smaller than 8. Because the prink(BIOS_SPEW, ..) in ipmi_kcs_status() creates delay and avoid the problem, but after setting the log level to 4 we see some timeout occurs.
The unit is millisecond and default value is set to 5000 according to IPMI spec v2.0 rev 1.1 Sec. 9.15, a five-second timeout or greater is recommended.
Tested=On OCP Delta Lake, with log level 4 cannot observe timeout occurs.
Change-Id: I42ede1d9200bb5d0dbb455d2ff66e2816f10e86b Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/drivers/ipmi/Kconfig M src/drivers/ipmi/ipmi_kcs.c 2 files changed, 20 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/45103/3
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45103 )
Change subject: drivers/ipmi: Add CONFIG_IPMI_KCS_TIMEOUT_MS for IPMI KCS timeout value ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45103/2/src/drivers/ipmi/Kconfig File src/drivers/ipmi/Kconfig:
https://review.coreboot.org/c/coreboot/+/45103/2/src/drivers/ipmi/Kconfig@42 PS2, Line 42: The timeout time for a single KCS IO in the unit of 100 microseconds.
If possible, please normalize the unit to μs or ms, and add the unit to the variable name.
Done
https://review.coreboot.org/c/coreboot/+/45103/2/src/drivers/ipmi/ipmi_kcs.c File src/drivers/ipmi/ipmi_kcs.c:
https://review.coreboot.org/c/coreboot/+/45103/2/src/drivers/ipmi/ipmi_kcs.c... PS2, Line 43: } while (timeout--);
Please rewrite using `wait_us()` or `wait_ms()` from `src/include/timer.h`.
Done
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45103 )
Change subject: drivers/ipmi: Add CONFIG_IPMI_KCS_TIMEOUT_MS for IPMI KCS timeout value ......................................................................
Patch Set 4: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45103 )
Change subject: drivers/ipmi: Add CONFIG_IPMI_KCS_TIMEOUT_MS for IPMI KCS timeout value ......................................................................
Patch Set 4:
(1 comment)
One more nit. Thank you for following up on the suggestions.
https://review.coreboot.org/c/coreboot/+/45103/4/src/drivers/ipmi/ipmi_kcs.c File src/drivers/ipmi/ipmi_kcs.c:
https://review.coreboot.org/c/coreboot/+/45103/4/src/drivers/ipmi/ipmi_kcs.c... PS4, Line 42: return 0; If one branch using braces, all branches need to.
https://doc.coreboot.org/coding_style.html
Hello build bot (Jenkins), Patrick Rudolph, Jonathan Zhang, Jingle Hsu, Angel Pons, Morgan Jang, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45103
to look at the new patch set (#5).
Change subject: drivers/ipmi: Add CONFIG_IPMI_KCS_TIMEOUT_MS for IPMI KCS timeout value ......................................................................
drivers/ipmi: Add CONFIG_IPMI_KCS_TIMEOUT_MS for IPMI KCS timeout value
With the current timeout of 1000 cycles of 100 microsecond would see timeout occurs on OCP Delta Lake if the log level is set to values smaller than 8. Because the prink(BIOS_SPEW, ..) in ipmi_kcs_status() creates delay and avoid the problem, but after setting the log level to 4 we see some timeout occurs.
The unit is millisecond and the default value is set to 5000 according to IPMI spec v2.0 rev 1.1 Sec. 9.15, a five-second timeout or greater is recommended.
Tested=On OCP Delta Lake, with log level 4 cannot observe timeout occurs.
Change-Id: I42ede1d9200bb5d0dbb455d2ff66e2816f10e86b Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/drivers/ipmi/Kconfig M src/drivers/ipmi/ipmi_kcs.c 2 files changed, 22 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/45103/5
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45103 )
Change subject: drivers/ipmi: Add CONFIG_IPMI_KCS_TIMEOUT_MS for IPMI KCS timeout value ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45103/4/src/drivers/ipmi/ipmi_kcs.c File src/drivers/ipmi/ipmi_kcs.c:
https://review.coreboot.org/c/coreboot/+/45103/4/src/drivers/ipmi/ipmi_kcs.c... PS4, Line 42: return 0;
If one branch using braces, all branches need to. […]
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45103 )
Change subject: drivers/ipmi: Add CONFIG_IPMI_KCS_TIMEOUT_MS for IPMI KCS timeout value ......................................................................
Patch Set 5: Code-Review+1
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45103 )
Change subject: drivers/ipmi: Add CONFIG_IPMI_KCS_TIMEOUT_MS for IPMI KCS timeout value ......................................................................
Patch Set 5: Code-Review+1
Hello build bot (Jenkins), Patrick Rudolph, Jonathan Zhang, Paul Menzel, Jingle Hsu, Angel Pons, Morgan Jang, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45103
to look at the new patch set (#6).
Change subject: drivers/ipmi: Add CONFIG_IPMI_KCS_TIMEOUT_MS for IPMI KCS timeout value ......................................................................
drivers/ipmi: Add CONFIG_IPMI_KCS_TIMEOUT_MS for IPMI KCS timeout value
With the current timeout of 1000 cycles of 100 microsecond would see timeout occurs on OCP Delta Lake if the log level is set to values smaller than 8. Because the prink(BIOS_SPEW, ..) in ipmi_kcs_status() creates delay and avoid the problem, but after setting the log level to 4 we see some timeout occurs.
The unit is millisecond and the default value is set to 5000 according to IPMI spec v2.0 rev 1.1 Sec. 9.15, a five-second timeout or greater is recommended.
Tested=On OCP Delta Lake, with log level 4 cannot observe timeout occurs.
Change-Id: I42ede1d9200bb5d0dbb455d2ff66e2816f10e86b Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/drivers/ipmi/Kconfig M src/drivers/ipmi/ipmi_kcs.c 2 files changed, 22 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/45103/6
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45103 )
Change subject: drivers/ipmi: Add CONFIG_IPMI_KCS_TIMEOUT_MS for IPMI KCS timeout value ......................................................................
Patch Set 6: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45103 )
Change subject: drivers/ipmi: Add CONFIG_IPMI_KCS_TIMEOUT_MS for IPMI KCS timeout value ......................................................................
drivers/ipmi: Add CONFIG_IPMI_KCS_TIMEOUT_MS for IPMI KCS timeout value
With the current timeout of 1000 cycles of 100 microsecond would see timeout occurs on OCP Delta Lake if the log level is set to values smaller than 8. Because the prink(BIOS_SPEW, ..) in ipmi_kcs_status() creates delay and avoid the problem, but after setting the log level to 4 we see some timeout occurs.
The unit is millisecond and the default value is set to 5000 according to IPMI spec v2.0 rev 1.1 Sec. 9.15, a five-second timeout or greater is recommended.
Tested=On OCP Delta Lake, with log level 4 cannot observe timeout occurs.
Change-Id: I42ede1d9200bb5d0dbb455d2ff66e2816f10e86b Signed-off-by: Johnny Lin johnny_lin@wiwynn.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/45103 Reviewed-by: Patrick Georgi pgeorgi@google.com Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/drivers/ipmi/Kconfig M src/drivers/ipmi/ipmi_kcs.c 2 files changed, 22 insertions(+), 18 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, approved Paul Menzel: Looks good to me, but someone else must approve Angel Pons: Looks good to me, but someone else must approve
diff --git a/src/drivers/ipmi/Kconfig b/src/drivers/ipmi/Kconfig index 44ed17e..1137dcf 100644 --- a/src/drivers/ipmi/Kconfig +++ b/src/drivers/ipmi/Kconfig @@ -33,3 +33,12 @@ help The PNP base address of BMC KCS. It must be equal to the pnp port value defined in devicetree for chip drivers/ipmi. + +config IPMI_KCS_TIMEOUT_MS + int + default 5000 + depends on IPMI_KCS + help + The time unit is millisecond for each IPMI KCS transfer. + IPMI spec v2.0 rev 1.1 Sec. 9.15, a five-second timeout or + greater is recommended. diff --git a/src/drivers/ipmi/ipmi_kcs.c b/src/drivers/ipmi/ipmi_kcs.c index 1d6b71c..f8c64c6 100644 --- a/src/drivers/ipmi/ipmi_kcs.c +++ b/src/drivers/ipmi/ipmi_kcs.c @@ -3,7 +3,7 @@ #include <console/console.h> #include <device/device.h> #include <arch/io.h> -#include <delay.h> +#include <timer.h> #include "ipmi_kcs.h"
#define IPMI_KCS_STATE(_x) ((_x) >> 6) @@ -35,27 +35,22 @@
static int wait_ibf_timeout(int port) { - int timeout = 1000; - do { - if (!(ipmi_kcs_status(port) & IPMI_KCS_IBF)) - return 0; - udelay(100); - } while (timeout--); - printk(BIOS_ERR, "wait_ibf timeout!\n"); - return timeout; + if (!wait_ms(CONFIG_IPMI_KCS_TIMEOUT_MS, !(ipmi_kcs_status(port) & IPMI_KCS_IBF))) { + printk(BIOS_ERR, "wait_ibf timeout!\n"); + return 1; + } else { + return 0; + } }
static int wait_obf_timeout(int port) { - int timeout = 1000; - do { - if ((ipmi_kcs_status(port) & IPMI_KCS_OBF)) - return 0; - udelay(100); - } while (timeout--); - - printk(BIOS_ERR, "wait_obf timeout!\n"); - return timeout; + if (!wait_ms(CONFIG_IPMI_KCS_TIMEOUT_MS, (ipmi_kcs_status(port) & IPMI_KCS_OBF))) { + printk(BIOS_ERR, "wait_obf timeout!\n"); + return 1; + } else { + return 0; + } }
-
Angel Pons (Code Review) -
Johnny Lin (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)