Bryant Ou has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45405 )
Change subject: drivers/uart: Override uart base address ......................................................................
drivers/uart: Override uart base address
Add CONFIG_UART_OVERRIDE_BASE_ADDR to select the function, platform overrides the base address by providing a uart_platform_base routine.
Signed-off-by: Bryant Ou Bryant.Ou.Q@gmail.com Change-Id: I2079bd1e5ffa209553383b6aafe3b8724849ba2a --- M src/drivers/uart/Kconfig M src/drivers/uart/uart8250io.c 2 files changed, 9 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/45405/1
diff --git a/src/drivers/uart/Kconfig b/src/drivers/uart/Kconfig index 41b870f..9cbb359 100644 --- a/src/drivers/uart/Kconfig +++ b/src/drivers/uart/Kconfig @@ -33,6 +33,13 @@ Set to "y" when the platform overrides the uart_platform_refclk routine.
+config UART_OVERRIDE_BASE_ADDR + bool + default n + help + Set to "y" when the platform overrides the base address by providing + a uart_platform_base routine. + config DRIVERS_UART_8250MEM bool default n diff --git a/src/drivers/uart/uart8250io.c b/src/drivers/uart/uart8250io.c index d0841de..c5317af 100644 --- a/src/drivers/uart/uart8250io.c +++ b/src/drivers/uart/uart8250io.c @@ -75,6 +75,7 @@ ENABLE_TRACE; }
+#if !CONFIG(UART_OVERRIDE_BASE_ADDR) static const unsigned int bases[] = { 0x3f8, 0x2f8, 0x3e8, 0x2e8 };
uintptr_t uart_platform_base(unsigned int idx) @@ -83,6 +84,7 @@ return bases[idx]; return 0; } +#endif
void uart_init(unsigned int idx) {
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45405 )
Change subject: drivers/uart: Override uart base address ......................................................................
Patch Set 1:
This change is only affecting uart8250io but there's not a proper dependency on the Kconfig nor is the option named accordingly. If the desire is to have this option generic then all the touch points will need to be edited.
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45405 )
Change subject: drivers/uart: Override uart base address ......................................................................
Patch Set 1:
I don't think you want to override the uart_init function here. You just want to change the CONFIG from 3f8 to a different port defined by VPD. That should be genric and not platform specific code.
Hello build bot (Jenkins), Patrick Rudolph, Jonathan Zhang, Ryback Hung, Philipp Deppenwiese, Tim Chu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45405
to look at the new patch set (#2).
Change subject: drivers/uart: Override uart base address ......................................................................
drivers/uart: Override uart base address
Modify uart_platform_base as weak function for platform override, uart base address can be modified by some conditions, so it shouldn't be fixed at this point.
Signed-off-by: Bryant Ou Bryant.Ou.Q@gmail.com Change-Id: I2079bd1e5ffa209553383b6aafe3b8724849ba2a --- M src/drivers/uart/uart8250io.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/45405/2
Bryant Ou has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45405 )
Change subject: drivers/uart: Override uart base address ......................................................................
Patch Set 2:
Patch Set 1:
I don't think you want to override the uart_init function here. You just want to change the CONFIG from 3f8 to a different port defined by VPD. That should be genric and not platform specific code.
Yes, that's my purpose, I modify the function as weak, so I could override it without adding kconfig for platform.
Bryant Ou has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45405 )
Change subject: drivers/uart: Override uart base address ......................................................................
Patch Set 2:
Patch Set 1:
This change is only affecting uart8250io but there's not a proper dependency on the Kconfig nor is the option named accordingly. If the desire is to have this option generic then all the touch points will need to be edited.
I modify the function as weak, then override it in platform without adding kconfig.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45405 )
Change subject: drivers/uart: Override uart base address ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45405/2/src/drivers/uart/uart8250io... File src/drivers/uart/uart8250io.c:
https://review.coreboot.org/c/coreboot/+/45405/2/src/drivers/uart/uart8250io... PS2, Line 78: static const unsigned int bases[] = { 0x3f8, 0x2f8, 0x3e8, 0x2e8 }; Do you want to be able to use any base address, or just let VPD choose one of these? I think we could let `CONFIG_UART_FOR_CONSOLE` be a variable instead of a Kconfig constant.
Bryant Ou has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45405 )
Change subject: drivers/uart: Override uart base address ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45405/2/src/drivers/uart/uart8250io... File src/drivers/uart/uart8250io.c:
https://review.coreboot.org/c/coreboot/+/45405/2/src/drivers/uart/uart8250io... PS2, Line 78: static const unsigned int bases[] = { 0x3f8, 0x2f8, 0x3e8, 0x2e8 };
Do you want to be able to use any base address, or just let VPD choose one of these? I think we coul […]
Use any base address. I don't know how to let CONFIG_UART_FOR_CONSOLE be a variale, is it too complex?
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45405 )
Change subject: drivers/uart: Override uart base address ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45405/2/src/drivers/uart/uart8250io... File src/drivers/uart/uart8250io.c:
https://review.coreboot.org/c/coreboot/+/45405/2/src/drivers/uart/uart8250io... PS2, Line 78: static const unsigned int bases[] = { 0x3f8, 0x2f8, 0x3e8, 0x2e8 };
Use any base address. […]
What address would you use that isn't listed? I think that bases[] is the only valid settings. It is structured this way so that menuconfig works. I'm trying to understand the use case for change this runtime and not build time. Also, this doesn't handle the early uart/serial init.
I don't think that a weak function boots the right way. I'd rather a CONFIG option that is more deterministic, but I don't know what the current coreboot policy is on this. It is hard to tell which function is being built, but I guess this is more obvious since it would need to be in the mainboard you are building.
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45405 )
Change subject: drivers/uart: Override uart base address ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45405/2/src/drivers/uart/uart8250io... File src/drivers/uart/uart8250io.c:
https://review.coreboot.org/c/coreboot/+/45405/2/src/drivers/uart/uart8250io... PS2, Line 78: static const unsigned int bases[] = { 0x3f8, 0x2f8, 0x3e8, 0x2e8 };
What address would you use that isn't listed? I think that bases[] is the only valid settings. […]
Marc, For DeltaLake, we are in the process of polishing the firmware image for it to be used by other teams. The image needs to be user (non-coreboot engineers) friendly, but also debug-able when problem arises. Therefore, we need console UART (as well as log level) to be able to be changed without having to rebuild an image.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45405 )
Change subject: drivers/uart: Override uart base address ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45405/2/src/drivers/uart/uart8250io... File src/drivers/uart/uart8250io.c:
https://review.coreboot.org/c/coreboot/+/45405/2/src/drivers/uart/uart8250io... PS2, Line 78: static const unsigned int bases[] = { 0x3f8, 0x2f8, 0x3e8, 0x2e8 };
Marc, For DeltaLake, we are in the process of polishing the firmware image for it to be used by othe […]
So why not use a cmos option and the get_option function? Look at how init_log_level in console/init.c works. You can set a bitfield for the typical values or if you wanted to allow any value, you can use an int.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45405 )
Change subject: drivers/uart: Override uart base address ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45405/2/src/drivers/uart/uart8250io... File src/drivers/uart/uart8250io.c:
https://review.coreboot.org/c/coreboot/+/45405/2/src/drivers/uart/uart8250io... PS2, Line 78: static const unsigned int bases[] = { 0x3f8, 0x2f8, 0x3e8, 0x2e8 };
So why not use a cmos option and the get_option function? Look at how init_log_level in console/ini […]
As said in another patch, no one said that `get_option` should be restricted to CMOS values only. It can be expanded to use VPD settings as well.
Actually, this just gave me an idea. I have CB:42136 and friends that would benefit from having adjustable settings not stored in CMOS, and VPD fits that purpose very well.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45405 )
Change subject: drivers/uart: Override uart base address ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45405/2/src/drivers/uart/uart8250io... File src/drivers/uart/uart8250io.c:
https://review.coreboot.org/c/coreboot/+/45405/2/src/drivers/uart/uart8250io... PS2, Line 78: static const unsigned int bases[] = { 0x3f8, 0x2f8, 0x3e8, 0x2e8 };
As said in another patch, no one said that `get_option` should be restricted to CMOS values only. It can be expanded to use VPD settings as well.
As I mentioned in https://review.coreboot.org/c/coreboot/+/45408/3/src/mainboard/ocp/deltalake..., I think we should be careful with using VPD for things it wasn't designed for. VPD was not designed to be efficient in pre-RAM stages. It seems a particularly bad idea to use it to configure something like the UART which is basically the very first thing coreboot needs to initialize on boot. I think(?) the CMOS option table has way less dependencies and is generally a lot "lighter weight", so it seems better suited for this sort of stuff. (That's also why I'm not sure expanding get_option() to VPD is a good idea because it makes it hard to factor in these kinds of differences when it's all merged into one framework. Something that would make get_option() unconditionally try to read VPD on every call just in case that particular option is defined in there would probably be a bad idea.)
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45405 )
Change subject: drivers/uart: Override uart base address ......................................................................
Patch Set 2:
I've read through the comments here but missed what the actual problem statement is. Can somebody wrap it up, please?
Non-standard i/o addresses were mentioned but how can that be more user-friendly when one can enter numbers that don't work? At least early coreboot code used to configure static addresses, hence only these could be selected for consoles.
Is this about which physical port is used? Or which address will be assigned to that port? Does the address matter after coreboot is done?
Hello build bot (Jenkins), Patrick Rudolph, Jonathan Zhang, Ryback Hung, Philipp Deppenwiese, Tim Chu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45405
to look at the new patch set (#3).
Change subject: console: Override uart base address ......................................................................
console: Override uart base address
Add a new CONFIG_OVERRIDE_UART_FOR_CONSOLE token to override the index of uart port, platform use a get_uart_for_console routine to decide what index value should be used for console.
Signed-off-by: Bryant Ou Bryant.Ou.Q@gmail.com Change-Id: I2079bd1e5ffa209553383b6aafe3b8724849ba2a --- M src/console/Kconfig M src/include/console/uart.h 2 files changed, 25 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/45405/3
Bryant Ou has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45405 )
Change subject: console: Override uart base address ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45405/2/src/drivers/uart/uart8250io... File src/drivers/uart/uart8250io.c:
https://review.coreboot.org/c/coreboot/+/45405/2/src/drivers/uart/uart8250io... PS2, Line 78: static const unsigned int bases[] = { 0x3f8, 0x2f8, 0x3e8, 0x2e8 };
As said in another patch, no one said that `get_option` should be restricted to CMOS values only. […]
I've changed the other way to override the index value, use a routine in mainboard then return which index of uart port is used.
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45405 )
Change subject: console: Override uart base address ......................................................................
Patch Set 3: Code-Review+1
(1 comment)
This is more consistent with the other console overrides.
https://review.coreboot.org/c/coreboot/+/45405/2/src/drivers/uart/uart8250io... File src/drivers/uart/uart8250io.c:
https://review.coreboot.org/c/coreboot/+/45405/2/src/drivers/uart/uart8250io... PS2, Line 78: static const unsigned int bases[] = { 0x3f8, 0x2f8, 0x3e8, 0x2e8 };
Marc, For DeltaLake, we are in the process of polishing the firmware image for it to be used by othe […]
I was trying to understand how it changes to get to the best support. It does seem like some kind of platform override would work here, but I'm not sure that weak calls vs platforms or a build time config option like the OVERRIDE_BAUD would be more correct. It seems that there are more weak functions in coreboot now, but that has its own issues.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45405 )
Change subject: console: Override uart base address ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45405/2/src/drivers/uart/uart8250io... File src/drivers/uart/uart8250io.c:
https://review.coreboot.org/c/coreboot/+/45405/2/src/drivers/uart/uart8250io... PS2, Line 78: static const unsigned int bases[] = { 0x3f8, 0x2f8, 0x3e8, 0x2e8 };
I was trying to understand how it changes to get to the best support. […]
I had an insane idea not too long ago: the `dynconfig()` macro
When there's no `dynconfig backend` (e.g. CMOS, VPD) to change options after building, this would be equivalent to the `CONFIG()` macro, and evaluate to a constant expression. However, if some `dynconfig backend` is enabled, then it would become a function call, which would retrieve the value through that backend. This would allow reusing the same Kconfig symbol names in the dynconfig backends (with some prefix to differentiate them from regular Kconfig symbols).
I didn't think about implementing this though, which is why I say it's an insane idea.
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45405 )
Change subject: console: Override uart base address ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45405/2/src/drivers/uart/uart8250io... File src/drivers/uart/uart8250io.c:
https://review.coreboot.org/c/coreboot/+/45405/2/src/drivers/uart/uart8250io... PS2, Line 78: static const unsigned int bases[] = { 0x3f8, 0x2f8, 0x3e8, 0x2e8 };
I had an insane idea not too long ago: the `dynconfig()` macro […]
This is not an insane idea. ;-) Coreboot should have a framework for behavior customization without a different image. With server market, this is a requirement. Now we can customize FSP (through VPD), we should be able to do the same for coreboot. Linux kernel has many command line parameters that allow such customization.
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45405 )
Change subject: console: Override uart base address ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45405/2/src/drivers/uart/uart8250io... File src/drivers/uart/uart8250io.c:
https://review.coreboot.org/c/coreboot/+/45405/2/src/drivers/uart/uart8250io... PS2, Line 78: static const unsigned int bases[] = { 0x3f8, 0x2f8, 0x3e8, 0x2e8 };
This is not an insane idea. ;-) […]
It seems that get_option is the right way to do this with the addition of the mainboard override. Connecting it to the CONFIG option is the missing part.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45405 )
Change subject: console: Override uart base address ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45405/2/src/drivers/uart/uart8250io... File src/drivers/uart/uart8250io.c:
https://review.coreboot.org/c/coreboot/+/45405/2/src/drivers/uart/uart8250io... PS2, Line 78: static const unsigned int bases[] = { 0x3f8, 0x2f8, 0x3e8, 0x2e8 };
It seems that get_option is the right way to do this with the addition of the mainboard override. […]
Something very important I forgot about: typing. The current `get_option` implementation lacks typing. There's no way to know if "foo" is a string, a bool, an int, a char, an uint64_t...
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45405 )
Change subject: console: Override uart base address ......................................................................
Patch Set 5: Code-Review+1
Please fix the build issue.
Bryant Ou has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45405 )
Change subject: console: Override uart base address ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45405/2/src/drivers/uart/uart8250io... File src/drivers/uart/uart8250io.c:
https://review.coreboot.org/c/coreboot/+/45405/2/src/drivers/uart/uart8250io... PS2, Line 78: static const unsigned int bases[] = { 0x3f8, 0x2f8, 0x3e8, 0x2e8 };
Something very important I forgot about: typing. […]
File is restored.
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45405 )
Change subject: console: Override uart base address ......................................................................
Patch Set 6: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45405 )
Change subject: console: Override uart base address ......................................................................
console: Override uart base address
Add a new CONFIG_OVERRIDE_UART_FOR_CONSOLE token to override the index of uart port, platform use a get_uart_for_console routine to decide what index value should be used for console.
Signed-off-by: Bryant Ou Bryant.Ou.Q@gmail.com Change-Id: I2079bd1e5ffa209553383b6aafe3b8724849ba2a Reviewed-on: https://review.coreboot.org/c/coreboot/+/45405 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Marc Jones marc@marcjonesconsulting.com Reviewed-by: Jonathan Zhang jonzhang@fb.com --- M src/console/Kconfig M src/include/console/uart.h 2 files changed, 25 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Marc Jones: Looks good to me, approved Jonathan Zhang: Looks good to me, but someone else must approve
diff --git a/src/console/Kconfig b/src/console/Kconfig index 283488c..548b701 100644 --- a/src/console/Kconfig +++ b/src/console/Kconfig @@ -58,6 +58,14 @@ comment "device-specific UART" depends on HAVE_UART_SPECIAL
+config OVERRIDE_UART_FOR_CONSOLE + bool + help + Set to "y" when the platform overrides the index of uart port by providing + a get_uart_for_console routine. + +if !OVERRIDE_UART_FOR_CONSOLE + config UART_FOR_CONSOLE int prompt "Index for UART port to use for console" if !FIXED_UART_FOR_CONSOLE @@ -87,6 +95,8 @@ comment "Serial port base address = 0x2e8" depends on DRIVERS_UART_8250IO && UART_FOR_CONSOLE = 3
+endif + config UART_OVERRIDE_BAUDRATE bool help diff --git a/src/include/console/uart.h b/src/include/console/uart.h index 6126a89..2e23d43 100644 --- a/src/include/console/uart.h +++ b/src/include/console/uart.h @@ -20,6 +20,18 @@ } #endif
+#if CONFIG(OVERRIDE_UART_FOR_CONSOLE) +/* Return the index of uart port, define this in your platform + * when need to use variables to override the index. + */ +unsigned int get_uart_for_console(void); +#else +static inline unsigned int get_uart_for_console(void) +{ + return CONFIG_UART_FOR_CONSOLE; +} +#endif + /* Returns the divisor value for a given baudrate. * The formula to satisfy is: * refclk / divisor = baudrate * oversample @@ -56,15 +68,15 @@ #if __CONSOLE_SERIAL_ENABLE__ static inline void __uart_init(void) { - uart_init(CONFIG_UART_FOR_CONSOLE); + uart_init(get_uart_for_console()); } static inline void __uart_tx_byte(u8 data) { - uart_tx_byte(CONFIG_UART_FOR_CONSOLE, data); + uart_tx_byte(get_uart_for_console(), data); } static inline void __uart_tx_flush(void) { - uart_tx_flush(CONFIG_UART_FOR_CONSOLE); + uart_tx_flush(get_uart_for_console()); } #else static inline void __uart_init(void) {}
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
Bryant Ou (Code Review)
-
Jonathan Zhang (Code Review)
-
Julius Werner (Code Review)
-
Marc Jones (Code Review)
-
Martin Roth (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)