Change in ...coreboot[master]: asus/am1i-a: Fix UART 0 port while preserving the UART 1 functionality
Mike Banon has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33796
Change subject: asus/am1i-a: Fix UART 0 port while preserving the UART 1 functionality ......................................................................
asus/am1i-a: Fix UART 0 port while preserving the UART 1 functionality
It has been observed by me and Elisenda Cuadros / Gergely Kiss [1] that the boot process of this board is super slow when UART 0 is being used - even if nothing is connected to it. Fix UART 0 by initializing it at romstage.
[1] https://mail.coreboot.org/pipermail/coreboot/2018-February/086132.html
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: I6579aa8fd092da84f8afdcc33496db45c582919f --- M src/mainboard/asus/am1i-a/romstage.c 1 file changed, 4 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/33796/1
diff --git a/src/mainboard/asus/am1i-a/romstage.c b/src/mainboard/asus/am1i-a/romstage.c index 4b172ea..258ca03 100644 --- a/src/mainboard/asus/am1i-a/romstage.c +++ b/src/mainboard/asus/am1i-a/romstage.c @@ -25,7 +25,8 @@ #include <superio/ite/it8623e/it8623e.h>
#define ITE_CONFIG_REG_CC 0x02 -#define SERIAL_DEV PNP_DEV(0x2e, IT8623E_SP2) +#define SERIAL_DEV1 PNP_DEV(0x2e, IT8623E_SP1) +#define SERIAL_DEV2 PNP_DEV(0x2e, IT8623E_SP2) #define GPIO_DEV PNP_DEV(0x2e, IT8623E_GPIO) #define CLKIN_DEV PNP_DEV(0x2e, IT8623E_GPIO) #define ENVC_DEV PNP_DEV(0x2e, IT8623E_EC) @@ -160,7 +161,8 @@ ite_evc_conf(ENVC_DEV);
ite_conf_clkin(CLKIN_DEV, ITE_UART_CLK_PREDIVIDE_48); - ite_enable_serial(SERIAL_DEV, CONFIG_TTYS0_BASE); + ite_enable_serial(SERIAL_DEV1, CONFIG_TTYS0_BASE); + ite_enable_serial(SERIAL_DEV2, CONFIG_TTYS0_BASE); ite_kill_watchdog(GPIO_DEV);
/*
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33796 )
Change subject: asus/am1i-a: Fix UART 0 port while preserving the UART 1 functionality ......................................................................
Patch Set 1:
Replacement of CB:33776
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33796 )
Change subject: asus/am1i-a: Fix UART 0 port while preserving the UART 1 functionality ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33796/1/src/mainboard/asus/am1i-a/romstage.c File src/mainboard/asus/am1i-a/romstage.c:
https://review.coreboot.org/#/c/33796/1/src/mainboard/asus/am1i-a/romstage.c... PS1, Line 166: ite_kill_watchdog(GPIO_DEV); It's not allowed to configure same IO range for two devices.
Hello Kyösti Mälkki, Angel Pons, Arthur Heymans, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33796
to look at the new patch set (#2).
Change subject: asus/am1i-a: Fix UART 0 port while preserving UART 1 functionality ......................................................................
asus/am1i-a: Fix UART 0 port while preserving UART 1 functionality
It has been observed by me and Elisenda Cuadros / Gergely Kiss [1] that the boot process of this board is super slow when UART 0 is being used - even if nothing is connected to it. Fix UART 0 by initializing it at romstage.
[1] https://mail.coreboot.org/pipermail/coreboot/2018-February/086132.html
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: I6579aa8fd092da84f8afdcc33496db45c582919f --- M src/mainboard/asus/am1i-a/romstage.c 1 file changed, 8 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/33796/2
Hello Kyösti Mälkki, Angel Pons, Arthur Heymans, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33796
to look at the new patch set (#3).
Change subject: asus/am1i-a: Fix UART 0 port while preserving UART 1 functionality ......................................................................
asus/am1i-a: Fix UART 0 port while preserving UART 1 functionality
It has been observed by me and Elisenda Cuadros / Gergely Kiss [1] that the boot process of this board is super slow when UART 0 is being used - even if nothing is connected to it. Fix UART 0 by initializing it at romstage.
[1] https://mail.coreboot.org/pipermail/coreboot/2018-February/086132.html
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: I6579aa8fd092da84f8afdcc33496db45c582919f --- M src/mainboard/asus/am1i-a/romstage.c 1 file changed, 6 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/33796/3
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33796 )
Change subject: asus/am1i-a: Fix UART 0 port while preserving UART 1 functionality ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/33796/1/src/mainboard/asus/am1i-a/romstage.c File src/mainboard/asus/am1i-a/romstage.c:
https://review.coreboot.org/#/c/33796/1/src/mainboard/asus/am1i-a/romstage.c... PS1, Line 166: ite_kill_watchdog(GPIO_DEV);
It's not allowed to configure same IO range for two devices.
Thank you, I hope it is good now. Please check the latest revision
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33796 )
Change subject: asus/am1i-a: Fix UART 0 port while preserving UART 1 functionality ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/33796/3/src/mainboard/asus/am1i-a/romstage.c File src/mainboard/asus/am1i-a/romstage.c:
https://review.coreboot.org/#/c/33796/3/src/mainboard/asus/am1i-a/romstage.c... PS3, Line 29: CONFIG_TTYS0_BASE use CONFIG_UART_FOR_CONSOLE. You may also want to make sure it does not compile if an invalid CONFIG_UART_FOR_CONSOLE is set.
Hello Kyösti Mälkki, Angel Pons, Arthur Heymans, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33796
to look at the new patch set (#4).
Change subject: asus/am1i-a: Fix UART 0 port while preserving UART 1 functionality ......................................................................
asus/am1i-a: Fix UART 0 port while preserving UART 1 functionality
It has been observed by me and Elisenda Cuadros / Gergely Kiss [1] that the boot process of this board is super slow when UART 0 is being used - even if nothing is connected to it. Fix UART 0 by initializing it at romstage.
[1] https://mail.coreboot.org/pipermail/coreboot/2018-February/086132.html
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: I6579aa8fd092da84f8afdcc33496db45c582919f --- M src/mainboard/asus/am1i-a/romstage.c 1 file changed, 6 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/33796/4
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33796 )
Change subject: asus/am1i-a: Fix UART 0 port while preserving UART 1 functionality ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/33796/3/src/mainboard/asus/am1i-a/romstage.c File src/mainboard/asus/am1i-a/romstage.c:
https://review.coreboot.org/#/c/33796/3/src/mainboard/asus/am1i-a/romstage.c... PS3, Line 29: CONFIG_TTYS0_BASE
use CONFIG_UART_FOR_CONSOLE
Done
You may also want to make sure it does not compile if an invalid CONFIG_UART_FOR_CONSOLE is set.
Please could you advise the best way how to do this?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33796 )
Change subject: asus/am1i-a: Fix UART 0 port while preserving UART 1 functionality ......................................................................
Patch Set 4: Code-Review+2
In the case of legacy PNP UARTs in superio, almost nobody cares about checking UART_FOR_CONSOLE at build time. You could use '#error' for >1.
Also: CB:31934 (if rebased and completely applied) will revisit this.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33796 )
Change subject: asus/am1i-a: Fix UART 0 port while preserving UART 1 functionality ......................................................................
Patch Set 4: Code-Review+1
(2 comments)
Much better, thank you.
https://review.coreboot.org/#/c/33796/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33796/4//COMMIT_MSG@7 PS4, Line 7: Fix UART 0 port while preserving UART 1 functionality I'd update the summary, e.g.: Enable UART according to CONFIG_UART_FOR_CONSOLE
(commit message would also need an update)
https://review.coreboot.org/#/c/33796/3/src/mainboard/asus/am1i-a/romstage.c File src/mainboard/asus/am1i-a/romstage.c:
https://review.coreboot.org/#/c/33796/3/src/mainboard/asus/am1i-a/romstage.c... PS3, Line 29: CONFIG_TTYS0_BASE
use CONFIG_UART_FOR_CONSOLE […]
The only valid values are 0 and 1, aren't they? Explicitly check for them:
if CONFIG_UART_FOR_CONSOLE == 0 #define SERIAL_DEV PNP_DEV(0x2e, IT8623E_SP1) #elif CONFIG_UART_FOR_CONSOLE == 1 #define SERIAL_DEV PNP_DEV(0x2e, IT8623E_SP2) #else #error "Invalid value for CONFIG_UART_FOR_CONSOLE" #endif
(and while we are at it, let's put UART 0 above UART 1)
Hello Kyösti Mälkki, Angel Pons, Arthur Heymans, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33796
to look at the new patch set (#5).
Change subject: asus/am1i-a: Enable UART according to CONFIG_UART_FOR_CONSOLE ......................................................................
asus/am1i-a: Enable UART according to CONFIG_UART_FOR_CONSOLE
It has been observed by me and Elisenda Cuadros / Gergely Kiss [1] that the boot process of this board is super slow when UART 0 is being used - even if nothing is connected to it. Enable UART according to CONFIG_UART_FOR_CONSOLE - and, if UART 0 is selected, it will be initialized at romstage and this problem will not happen.
[1] https://mail.coreboot.org/pipermail/coreboot/2018-February/086132.html
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: I6579aa8fd092da84f8afdcc33496db45c582919f --- M src/mainboard/asus/am1i-a/romstage.c 1 file changed, 8 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/33796/5
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33796 )
Change subject: asus/am1i-a: Enable UART according to CONFIG_UART_FOR_CONSOLE ......................................................................
Patch Set 5:
(2 comments)
Thank you very much for your help and beautiful code, I hope it is better now.
https://review.coreboot.org/#/c/33796/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33796/4//COMMIT_MSG@7 PS4, Line 7: Fix UART 0 port while preserving UART 1 functionality
I'd update the summary, e.g.: Enable UART according to CONFIG_UART_FOR_CONSOLE […]
Done.
https://review.coreboot.org/#/c/33796/3/src/mainboard/asus/am1i-a/romstage.c File src/mainboard/asus/am1i-a/romstage.c:
https://review.coreboot.org/#/c/33796/3/src/mainboard/asus/am1i-a/romstage.c... PS3, Line 29: CONFIG_TTYS0_BASE
The only valid values are 0 and 1, aren't they? Explicitly check for them: […]
Done.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33796 )
Change subject: asus/am1i-a: Enable UART according to CONFIG_UART_FOR_CONSOLE ......................................................................
Patch Set 5: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33796 )
Change subject: asus/am1i-a: Enable UART according to CONFIG_UART_FOR_CONSOLE ......................................................................
Patch Set 5: Code-Review+1
Kyösti Mälkki has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33796 )
Change subject: asus/am1i-a: Enable UART according to CONFIG_UART_FOR_CONSOLE ......................................................................
asus/am1i-a: Enable UART according to CONFIG_UART_FOR_CONSOLE
It has been observed by me and Elisenda Cuadros / Gergely Kiss [1] that the boot process of this board is super slow when UART 0 is being used - even if nothing is connected to it. Enable UART according to CONFIG_UART_FOR_CONSOLE - and, if UART 0 is selected, it will be initialized at romstage and this problem will not happen.
[1] https://mail.coreboot.org/pipermail/coreboot/2018-February/086132.html
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: I6579aa8fd092da84f8afdcc33496db45c582919f Reviewed-on: https://review.coreboot.org/c/coreboot/+/33796 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net --- M src/mainboard/asus/am1i-a/romstage.c 1 file changed, 8 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, approved Paul Menzel: Looks good to me, but someone else must approve
diff --git a/src/mainboard/asus/am1i-a/romstage.c b/src/mainboard/asus/am1i-a/romstage.c index 4b172ea..5e1218a 100644 --- a/src/mainboard/asus/am1i-a/romstage.c +++ b/src/mainboard/asus/am1i-a/romstage.c @@ -25,7 +25,15 @@ #include <superio/ite/it8623e/it8623e.h>
#define ITE_CONFIG_REG_CC 0x02 + +#if CONFIG_UART_FOR_CONSOLE == 0 +#define SERIAL_DEV PNP_DEV(0x2e, IT8623E_SP1) +#elif CONFIG_UART_FOR_CONSOLE == 1 #define SERIAL_DEV PNP_DEV(0x2e, IT8623E_SP2) +#else +#error "Invalid value for CONFIG_UART_FOR_CONSOLE" +#endif + #define GPIO_DEV PNP_DEV(0x2e, IT8623E_GPIO) #define CLKIN_DEV PNP_DEV(0x2e, IT8623E_GPIO) #define ENVC_DEV PNP_DEV(0x2e, IT8623E_EC)
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Kyösti Mälkki (Code Review)
-
Mike Banon (Code Review)
-
Paul Menzel (Code Review)