Hello Martin Roth, Rob Barnes,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/40322
to review the following change.
Change subject: soc/amd/picasso: Cleanup legacy UART config ......................................................................
soc/amd/picasso: Cleanup legacy UART config
Cleaned up configuration of the legacy UART. Added Kconfig options for the mapping between UART and legacy I/O decode.
TEST=Manual, boot trembyle, verify UART configured correctly in log. serial8250: ttyS3 at I/O 0x2e8 (IRQ = 3, base_baud = 115200) is a 16550A $ io_write8 0x2e8 97 -> a is output on console BUG=b:143283592
Signed-off-by: Rob Barnes robbarnes@google.com Change-Id: Id08ff6428d4019303ebb6e44e13aba480cf1fde2 Reviewed-on: https://chromium-review.googlesource.com/2037891 Reviewed-by: Martin Roth martinroth@google.com Tested-by: Martin Roth martinroth@google.com --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/include/soc/southbridge.h M src/soc/amd/picasso/southbridge.c M src/soc/amd/picasso/uart.c 4 files changed, 39 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/40322/1
diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig index d9211b4..8279fd8 100644 --- a/src/soc/amd/picasso/Kconfig +++ b/src/soc/amd/picasso/Kconfig @@ -157,6 +157,7 @@
config PICASSO_UART_LEGACY bool "Decode legacy I/O range" + default n depends on PICASSO_UART help Assign I/O 3F8, 2F8, etc. to a Picasso UART. Only a single UART may @@ -164,6 +165,19 @@ console. A UART accessed with I/O does not allow all the features of MMIO. The MMIO decode is still present when this option is used.
+config PICASSO_UART_LEGACY_RANGE + int "Console uart legacy I/O range" + depends on PICASSO_UART_LEGACY + range 0 3 + default 0 + help + Assign legacy I/O range to the Picasso console UART. + Possible I/O ranges: + 0: 2e8 + 1: 2f8 + 2: 3e8 + 3: 3f8 + config CONSOLE_UART_BASE_ADDRESS depends on CONSOLE_SERIAL && PICASSO_UART hex diff --git a/src/soc/amd/picasso/include/soc/southbridge.h b/src/soc/amd/picasso/include/soc/southbridge.h index a134245..3c13711 100644 --- a/src/soc/amd/picasso/include/soc/southbridge.h +++ b/src/soc/amd/picasso/include/soc/southbridge.h @@ -226,10 +226,8 @@ #define FCH_AOAC_STAT0 BIT(6) #define FCH_AOAC_STAT1 BIT(7)
-#define FCH_UART_LEGACY_DECODE 0xfedc0020 -#define FCH_LEGACY_3F8_SH 3 -#define FCH_LEGACY_2F8_SH 1 -#define FCH_LEGACY_3E8_SH 2 +#define FCH_LEGACY_UART_DECODE 0xfedc0020 +#define FCH_LEGACY_UART_WHICH_SHIFT 8
#define PM1_LIMIT 16 #define GPE0_LIMIT 28 @@ -322,6 +320,7 @@ void fch_pre_init(void); void fch_early_init(void); void set_uart_config(int idx); +void set_uart_legacy_config(int uart_idx, int range_idx);
/* Initialize all the i2c buses that are marked with early init. */ void i2c_soc_early_init(void); diff --git a/src/soc/amd/picasso/southbridge.c b/src/soc/amd/picasso/southbridge.c index 6bedab0..fa3a71c 100644 --- a/src/soc/amd/picasso/southbridge.c +++ b/src/soc/amd/picasso/southbridge.c @@ -276,6 +276,9 @@ sb_reset_i2c_slaves(); if (CONFIG(PICASSO_UART)) set_uart_config(CONFIG_UART_FOR_CONSOLE); + if (CONFIG(PICASSO_UART_LEGACY)) + set_uart_legacy_config(CONFIG_UART_FOR_CONSOLE, + CONFIG_PICASSO_UART_LEGACY_RANGE); }
static void print_num_status_bits(int num_bits, uint32_t status, diff --git a/src/soc/amd/picasso/uart.c b/src/soc/amd/picasso/uart.c index 14a43c1..38e0865 100644 --- a/src/soc/amd/picasso/uart.c +++ b/src/soc/amd/picasso/uart.c @@ -39,10 +39,28 @@ return uart_info[idx].base; }
+ +void set_uart_legacy_config(int uart_idx, int range_idx) +{ + uint16_t uart_leg; + uint8_t uart_leg_which_offset; + + if (uart_idx < 0 || uart_idx > 3 || range_idx < 0 || range_idx > 3) + return; + + uart_leg = read16((void *)FCH_LEGACY_UART_DECODE); + /* Map uart_idx to io range_idx */ + uart_leg_which_offset = range_idx * 2 + FCH_LEGACY_UART_WHICH_SHIFT; + uart_leg &= ~(0x3 << uart_leg_which_offset); + uart_leg |= uart_idx << uart_leg_which_offset; + /* Enable io range */ + uart_leg |= 1 << range_idx; + write16((void *)FCH_LEGACY_UART_DECODE, uart_leg); +} + void set_uart_config(int idx) { uint32_t uart_ctrl; - uint16_t uart_leg;
if (idx < 0 || idx > ARRAY_SIZE(uart_info)) return; @@ -55,20 +73,6 @@ sm_pci_write32(SMB_UART_CONFIG, uart_ctrl); }
- if (CONFIG(PICASSO_UART_LEGACY) && idx != 3) { - /* Force 3F8 if idx=0, 2F8 if idx=1, 3E8 if idx=2 */ - - /* TODO: make clearer once PPR is updated */ - uart_leg = (idx << 8) | (idx << 10) | (idx << 12) | (idx << 14); - if (idx == 0) - uart_leg |= 1 << FCH_LEGACY_3F8_SH; - else if (idx == 1) - uart_leg |= 1 << FCH_LEGACY_2F8_SH; - else if (idx == 2) - uart_leg |= 1 << FCH_LEGACY_3E8_SH; - - write16((void *)FCH_UART_LEGACY_DECODE, uart_leg); - } }
unsigned int uart_platform_refclk(void)
Hello build bot (Jenkins), Martin Roth, Rob Barnes,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40322
to look at the new patch set (#2).
Change subject: soc/amd/picasso: Cleanup legacy UART config ......................................................................
soc/amd/picasso: Cleanup legacy UART config
Cleaned up configuration of the legacy UART. Added Kconfig options for the mapping between UART and legacy I/O decode.
TEST=Manual, boot trembyle, verify UART configured correctly in log. serial8250: ttyS3 at I/O 0x2e8 (IRQ = 3, base_baud = 115200) is a 16550A $ io_write8 0x2e8 97 -> a is output on console
BUG=b:143283592 BUG=b:153675918
Signed-off-by: Rob Barnes robbarnes@google.com Change-Id: Id08ff6428d4019303ebb6e44e13aba480cf1fde2 Reviewed-on: https://chromium-review.googlesource.com/2037891 Reviewed-by: Martin Roth martinroth@google.com Tested-by: Martin Roth martinroth@google.com --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/include/soc/southbridge.h M src/soc/amd/picasso/southbridge.c M src/soc/amd/picasso/uart.c 4 files changed, 39 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/40322/2
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40322 )
Change subject: soc/amd/picasso: Cleanup legacy UART config ......................................................................
Patch Set 2: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40322 )
Change subject: soc/amd/picasso: Cleanup legacy UART config ......................................................................
Patch Set 2: Code-Review+1
(6 comments)
https://review.coreboot.org/c/coreboot/+/40322/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40322/2//COMMIT_MSG@7 PS2, Line 7: Cleanup Clean up
https://review.coreboot.org/c/coreboot/+/40322/2//COMMIT_MSG@9 PS2, Line 9: Cleaned Clean up
https://review.coreboot.org/c/coreboot/+/40322/2//COMMIT_MSG@10 PS2, Line 10: Added Add
https://review.coreboot.org/c/coreboot/+/40322/2//COMMIT_MSG@18 PS2, Line 18: BUG=b:143283592 nit: Are the two bug IDs intended to be here? I've no idea of what they are about
https://review.coreboot.org/c/coreboot/+/40322/2/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/40322/2/src/soc/amd/picasso/Kconfig... PS2, Line 169: uart UART
https://review.coreboot.org/c/coreboot/+/40322/2/src/soc/amd/picasso/uart.c File src/soc/amd/picasso/uart.c:
https://review.coreboot.org/c/coreboot/+/40322/2/src/soc/amd/picasso/uart.c@... PS2, Line 42: double empty line
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40322 )
Change subject: soc/amd/picasso: Cleanup legacy UART config ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40322/2/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/40322/2/src/soc/amd/picasso/Kconfig... PS2, Line 169: uart
UART
Does it need a prompt?
And why is this configured in Kconfig and not the devicetree (where resources usually are)? What is done to avoid resource conflicts? e.g. for a board with super-i/o and a UART at 0x2e8, one could accidentally select the same here
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40322 )
Change subject: soc/amd/picasso: Cleanup legacy UART config ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40322/2/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/40322/2/src/soc/amd/picasso/Kconfig... PS2, Line 169: uart
Does it need a prompt? […]
on the Mandolin reference board there are I think all four UARTs routed to headers, so this probably shouldn't be a devicetree setting there. Also the UART for console selection in menuconfig asks for an UART number to use
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40322 )
Change subject: soc/amd/picasso: Cleanup legacy UART config ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40322/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40322/2//COMMIT_MSG@18 PS2, Line 18: BUG=b:143283592
nit: Are the two bug IDs intended to be here? I've no idea of what they are about
this is intended. these are for the chromium downstream bugtracker and there is the original bug and the issue regarding the upstreaming. When the patch lands in upstream and gets pulled back into some downstream branch, this will add a comment in the internal issue tracker. so yeah, for upstream this has no practical use, but in downstream this is very useful
Hello build bot (Jenkins), Martin Roth, Angel Pons, Rob Barnes,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40322
to look at the new patch set (#3).
Change subject: soc/amd/picasso: Clean up legacy UART config ......................................................................
soc/amd/picasso: Clean up legacy UART config
Clean up configuration of the legacy UART and add Kconfig options for the mapping between UART and legacy I/O decode.
TEST=Manual, boot trembyle, verify UART configured correctly in log. serial8250: ttyS3 at I/O 0x2e8 (IRQ = 3, base_baud = 115200) is a 16550A $ io_write8 0x2e8 97 -> a is output on console
BUG=b:143283592 BUG=b:153675918
Signed-off-by: Rob Barnes robbarnes@google.com Change-Id: Id08ff6428d4019303ebb6e44e13aba480cf1fde2 Reviewed-on: https://chromium-review.googlesource.com/2037891 Reviewed-by: Martin Roth martinroth@google.com Tested-by: Martin Roth martinroth@google.com --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/include/soc/southbridge.h M src/soc/amd/picasso/southbridge.c M src/soc/amd/picasso/uart.c 4 files changed, 41 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/40322/3
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40322 )
Change subject: soc/amd/picasso: Clean up legacy UART config ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40322/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40322/2//COMMIT_MSG@7 PS2, Line 7: Cleanup
Clean up
Done
https://review.coreboot.org/c/coreboot/+/40322/2//COMMIT_MSG@9 PS2, Line 9: Cleaned
Clean up
Done
https://review.coreboot.org/c/coreboot/+/40322/2//COMMIT_MSG@10 PS2, Line 10: Added
Add
Done
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40322 )
Change subject: soc/amd/picasso: Clean up legacy UART config ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40322/2/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/40322/2/src/soc/amd/picasso/Kconfig... PS2, Line 169: uart
on the Mandolin reference board there are I think all four UARTs routed to headers, so this probably […]
fixed the UART comment, but unsure if I should mark Nico's comment as solved
https://review.coreboot.org/c/coreboot/+/40322/2/src/soc/amd/picasso/uart.c File src/soc/amd/picasso/uart.c:
https://review.coreboot.org/c/coreboot/+/40322/2/src/soc/amd/picasso/uart.c@... PS2, Line 42:
double empty line
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40322 )
Change subject: soc/amd/picasso: Clean up legacy UART config ......................................................................
Patch Set 3:
(1 comment)
Haha, turns out I forgot to ask what this is used for? is it for software without a memory-mapped UART driver?
https://review.coreboot.org/c/coreboot/+/40322/2/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/40322/2/src/soc/amd/picasso/Kconfig... PS2, Line 169: uart
fixed the UART comment, but unsure if I should mark Nico's comment as solved
UART_FOR_CONSOLE decides which i/o ports the console writes to (imho, the prompt should be hidden unless there really are multiple i/o port sets decoded on a board, but that's a different story).
I do realize now that UART_FOR_CONSOLE is re-purposed in `picasso/ southbridge.c`. It looks like the console will only work in 4 out of the possible 16 combinations of CONSOLE_FOR_UART and PICASSO_ UART_LEGACY_RANGE. Or do I miss something? How about ditching this Kconfig and using TTYS0_BASE instead? (note, that one is very badly named, it's actually kind of a UART_FOR_CONSOLE_IO_BASE)
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40322 )
Change subject: soc/amd/picasso: Clean up legacy UART config ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40322/2/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/40322/2/src/soc/amd/picasso/Kconfig... PS2, Line 169: uart
UART_FOR_CONSOLE decides which i/o ports the console writes to (imho, […]
Sorry, I think this was all wrong. It doesn't have anything to do with the console, does it?
I don't understand the use case. Is it: If you want one of the Picasso UARTs in legacy mode for later software, enable serial console output in coreboot, select the UART for the console, set the expected i/o range here? That seems like a lot indirection.
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40322 )
Change subject: soc/amd/picasso: Clean up legacy UART config ......................................................................
Patch Set 3: Code-Review-1
Patch Set 3:
(1 comment)
Haha, turns out I forgot to ask what this is used for? is it for software without a memory-mapped UART driver?
The motive is to provide an option to enable legacy uart on picasso. I don't know when this would be used except for the case where the mmio driver is not available. The PICASSO_UART_LEGACY_RANGE was added because there was a request for more flexibility in the original review. Reviewing this second time, I too am unsure if this flexibility is needed it should probably just be mapped 0:0x3f8, 1:0x2f8, 2:0x3e8, 3:0x2e8. This would match uart8250io.c. Since this isn't even used at the moment, I suggest we skip this patch and I'll take another look at cleaning this up when I have some time.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40322 )
Change subject: soc/amd/picasso: Clean up legacy UART config ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40322/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40322/2//COMMIT_MSG@18 PS2, Line 18: BUG=b:143283592
this is intended. […]
Right, I was wondering why there were two of them, as there's usually one per change. Makes sense, though.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40322 )
Change subject: soc/amd/picasso: Clean up legacy UART config ......................................................................
Patch Set 3:
Haha, turns out I forgot to ask what this is used for? is it for software without a memory-mapped UART driver?
The motive is to provide an option to enable legacy uart on picasso. I don't know when this would be used except for the case where the mmio driver is not available. The PICASSO_UART_LEGACY_RANGE was added because there was a request for more flexibility in the original review. Reviewing this second time, I too am unsure if this flexibility is needed it should probably just be mapped 0:0x3f8, 1:0x2f8, 2:0x3e8, 3:0x2e8. This would match uart8250io.c. Since this isn't even used at the moment, I suggest we skip this patch and I'll take another look at cleaning this up when I have some time.
Thanks. What I'm mostly concerned about now is that this is mixed with console options. UART_FOR_CONSOLE depends on CONSOLE_SERIAL. So if anybody would want any Picasso UART with legacy interface they'd be forced to enable serial console. That seems odd.
Hello build bot (Jenkins), Martin Roth, Angel Pons, Rob Barnes,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40322
to look at the new patch set (#4).
Change subject: [WIP] [UNTESTED] soc/amd/picasso: Clean up legacy UART config ......................................................................
[WIP] [UNTESTED] soc/amd/picasso: Clean up legacy UART config
Clean up configuration of the legacy UART and add Kconfig options for the mapping between UART and legacy I/O decode.
BUG=b:143283592 BUG=b:153675918 BRANCH=zork
Signed-off-by: Rob Barnes robbarnes@google.com Signed-off-by: Felix Held felix-coreboot@felixheld.de Change-Id: Id08ff6428d4019303ebb6e44e13aba480cf1fde2 Reviewed-on: https://chromium-review.googlesource.com/2037891 --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/include/soc/southbridge.h M src/soc/amd/picasso/uart.c 3 files changed, 61 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/40322/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40322 )
Change subject: [WIP] [UNTESTED] soc/amd/picasso: Clean up legacy UART config ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40322/4/src/soc/amd/picasso/uart.c File src/soc/amd/picasso/uart.c:
https://review.coreboot.org/c/coreboot/+/40322/4/src/soc/amd/picasso/uart.c@... PS4, Line 153: if (CONFIG(PICASSO_UART_LEGACY)) { braces {} are not necessary for single statement blocks
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40322 )
Change subject: [WIP] [UNTESTED] soc/amd/picasso: Clean up legacy UART config ......................................................................
Patch Set 4: Code-Review-1
it should probably just be mapped 0:0x3f8, 1:0x2f8, 2:0x3e8, 3:0x2e8.
done
Thanks. What I'm mostly concerned about now is that this is mixed with console options. UART_FOR_CONSOLE depends on CONSOLE_SERIAL. So if anybody would want any Picasso UART with legacy interface they'd be forced to enable serial console. That seems odd.
fixed
haven't tested the patch on hardware yet, but wanted to push it before the weekend
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40322 )
Change subject: [WIP] [UNTESTED] soc/amd/picasso: Clean up legacy UART config ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40322/4/src/soc/amd/picasso/include... File src/soc/amd/picasso/include/soc/southbridge.h:
https://review.coreboot.org/c/coreboot/+/40322/4/src/soc/amd/picasso/include... PS4, Line 225: #define FCH_LEGACY_UART_MAP_SHIFT 8 nit: If these are bitfield definitions, I'd indent them with 2 spaces as done in the rest of the file
https://review.coreboot.org/c/coreboot/+/40322/4/src/soc/amd/picasso/uart.c File src/soc/amd/picasso/uart.c:
https://review.coreboot.org/c/coreboot/+/40322/4/src/soc/amd/picasso/uart.c@... PS4, Line 63: leg it's a warlking UART! 😄
https://review.coreboot.org/c/coreboot/+/40322/4/src/soc/amd/picasso/uart.c@... PS4, Line 82: const uint8_t range_idx[ARRAY_SIZE(uart_info)] = { : FCH_LEGACY_UART_RANGE_3F8, : FCH_LEGACY_UART_RANGE_2F8, : FCH_LEGACY_UART_RANGE_3E8, : FCH_LEGACY_UART_RANGE_2E8, : }; How about adding this information to uart_info instead?
Hello build bot (Jenkins), Martin Roth, Angel Pons, Rob Barnes,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40322
to look at the new patch set (#5).
Change subject: [WIP] [UNTESTED] soc/amd/picasso: Clean up legacy UART config ......................................................................
[WIP] [UNTESTED] soc/amd/picasso: Clean up legacy UART config
Clean up configuration of the legacy UART and add Kconfig options for the mapping between UART and legacy I/O decode.
BUG=b:143283592 BUG=b:153675918 BRANCH=zork
Signed-off-by: Rob Barnes robbarnes@google.com Signed-off-by: Felix Held felix-coreboot@felixheld.de Change-Id: Id08ff6428d4019303ebb6e44e13aba480cf1fde2 Reviewed-on: https://chromium-review.googlesource.com/2037891 --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/include/soc/southbridge.h M src/soc/amd/picasso/uart.c 3 files changed, 61 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/40322/5
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40322 )
Change subject: [WIP] [UNTESTED] soc/amd/picasso: Clean up legacy UART config ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40322/4/src/soc/amd/picasso/include... File src/soc/amd/picasso/include/soc/southbridge.h:
https://review.coreboot.org/c/coreboot/+/40322/4/src/soc/amd/picasso/include... PS4, Line 225: #define FCH_LEGACY_UART_MAP_SHIFT 8
nit: If these are bitfield definitions, I'd indent them with 2 spaces as done in the rest of the fil […]
Done
https://review.coreboot.org/c/coreboot/+/40322/4/src/soc/amd/picasso/uart.c File src/soc/amd/picasso/uart.c:
https://review.coreboot.org/c/coreboot/+/40322/4/src/soc/amd/picasso/uart.c@... PS4, Line 63: leg
it's a warlking UART! 😄
yeah, that does sound a bit odd; changed it to uart_legacy_decode
https://review.coreboot.org/c/coreboot/+/40322/4/src/soc/amd/picasso/uart.c@... PS4, Line 82: const uint8_t range_idx[ARRAY_SIZE(uart_info)] = { : FCH_LEGACY_UART_RANGE_3F8, : FCH_LEGACY_UART_RANGE_2F8, : FCH_LEGACY_UART_RANGE_3E8, : FCH_LEGACY_UART_RANGE_2E8, : };
How about adding this information to uart_info instead?
I didn't add that to uart_info for two reasons: uart_info contains information that's hard-wired in the chip while the mapping of the SoC UARTs to the legacy UART I/O regions is configurable and this table maps the I/O region to the UART that a legacy UART with the same number would have. When PICASSO_UART_LEGACY isn't set, which is the more common case, this function will get optimized out and I'm not sure if tat part of the mapping table would also be optimized out if it was added to uart_info
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40322 )
Change subject: [WIP] [UNTESTED] soc/amd/picasso: Clean up legacy UART config ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40322/5/src/soc/amd/picasso/uart.c File src/soc/amd/picasso/uart.c:
https://review.coreboot.org/c/coreboot/+/40322/5/src/soc/amd/picasso/uart.c@... PS5, Line 153: if (CONFIG(PICASSO_UART_LEGACY)) { braces {} are not necessary for single statement blocks
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40322 )
Change subject: [WIP] [UNTESTED] soc/amd/picasso: Clean up legacy UART config ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40322/5/src/soc/amd/picasso/uart.c File src/soc/amd/picasso/uart.c:
https://review.coreboot.org/c/coreboot/+/40322/5/src/soc/amd/picasso/uart.c@... PS5, Line 66: 3 Use a #define for max uart?
Hello build bot (Jenkins), Martin Roth, Angel Pons, Rob Barnes,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40322
to look at the new patch set (#6).
Change subject: [WIP] [UNTESTED] soc/amd/picasso: Clean up legacy UART config ......................................................................
[WIP] [UNTESTED] soc/amd/picasso: Clean up legacy UART config
Clean up configuration of the legacy UART and add Kconfig options for the mapping between UART and legacy I/O decode.
BUG=b:143283592 BUG=b:153675918 BRANCH=zork
Signed-off-by: Rob Barnes robbarnes@google.com Signed-off-by: Felix Held felix-coreboot@felixheld.de Change-Id: Id08ff6428d4019303ebb6e44e13aba480cf1fde2 Reviewed-on: https://chromium-review.googlesource.com/2037891 --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/include/soc/southbridge.h M src/soc/amd/picasso/uart.c 3 files changed, 61 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/40322/6
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40322 )
Change subject: [WIP] [UNTESTED] soc/amd/picasso: Clean up legacy UART config ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40322/6/src/soc/amd/picasso/uart.c File src/soc/amd/picasso/uart.c:
https://review.coreboot.org/c/coreboot/+/40322/6/src/soc/amd/picasso/uart.c@... PS6, Line 153: if (CONFIG(PICASSO_UART_LEGACY)) { braces {} are not necessary for single statement blocks
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40322 )
Change subject: [WIP] [UNTESTED] soc/amd/picasso: Clean up legacy UART config ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40322/5/src/soc/amd/picasso/uart.c File src/soc/amd/picasso/uart.c:
https://review.coreboot.org/c/coreboot/+/40322/5/src/soc/amd/picasso/uart.c@... PS5, Line 66: 3
Use a #define for max uart?
i'll use >= ARRAY_SIZE(uart_info) instead
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40322 )
Change subject: [WIP] [UNTESTED] soc/amd/picasso: Clean up legacy UART config ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40322/2/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/40322/2/src/soc/amd/picasso/Kconfig... PS2, Line 169: uart
Sorry, I think this was all wrong. It doesn't have anything to do […]
Done
Hello build bot (Jenkins), Martin Roth, Angel Pons, Rob Barnes,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40322
to look at the new patch set (#7).
Change subject: soc/amd/picasso: Clean up legacy UART config ......................................................................
soc/amd/picasso: Clean up legacy UART config
Clean up configuration of the legacy UART and add Kconfig options for the mapping between UART and legacy I/O decode.
BUG=b:143283592 BUG=b:153675918 TEST=Linux detects an additional legacy serial port for each active MMIO one if PICASSO_UART_LEGACY is selected. BRANCH=zork
Signed-off-by: Rob Barnes robbarnes@google.com Signed-off-by: Felix Held felix-coreboot@felixheld.de Change-Id: Id08ff6428d4019303ebb6e44e13aba480cf1fde2 Reviewed-on: https://chromium-review.googlesource.com/2037891 --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/include/soc/southbridge.h M src/soc/amd/picasso/uart.c 3 files changed, 60 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/40322/7
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40322 )
Change subject: soc/amd/picasso: Clean up legacy UART config ......................................................................
Patch Set 7:
the remaining thing I'm not sure about is if the ACPI entries for the MMIO UARTs should be hidden when PICASSO_UART_LEGACY is set, so that the same UARTs won't get exposed to the OS twice. If a SoC serial port is used as console, it'll still use the MMIO interface, but that is no issue there
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40322 )
Change subject: soc/amd/picasso: Clean up legacy UART config ......................................................................
Patch Set 7:
Patch Set 7:
the remaining thing I'm not sure about is if the ACPI entries for the MMIO UARTs should be hidden when PICASSO_UART_LEGACY is set, so that the same UARTs won't get exposed to the OS twice. If a SoC serial port is used as console, it'll still use the MMIO interface, but that is no issue there
I suspect resources should become reserved instead of part of an AMD0020 device. Existing content LGTM.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40322 )
Change subject: soc/amd/picasso: Clean up legacy UART config ......................................................................
Patch Set 7: Code-Review+2
Patch Set 7:
Patch Set 7:
the remaining thing I'm not sure about is if the ACPI entries for the MMIO UARTs should be hidden when PICASSO_UART_LEGACY is set, so that the same UARTs won't get exposed to the OS twice. If a SoC serial port is used as console, it'll still use the MMIO interface, but that is no issue there
I suspect resources should become reserved instead of part of an AMD0020 device. Existing content LGTM.
Let's worry about ACPI details later. I think the whole reason we wanted the legacy addresses supported was for niche environments that didn't support the MMIO UARTs. So I'm not sure the ACPI will be important there anyway. And AFAIK the UARTs are still accessible w/MMIO, so describing them may still be the right thing to do.
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40322 )
Change subject: soc/amd/picasso: Clean up legacy UART config ......................................................................
soc/amd/picasso: Clean up legacy UART config
Clean up configuration of the legacy UART and add Kconfig options for the mapping between UART and legacy I/O decode.
BUG=b:143283592 BUG=b:153675918 TEST=Linux detects an additional legacy serial port for each active MMIO one if PICASSO_UART_LEGACY is selected. BRANCH=zork
Signed-off-by: Rob Barnes robbarnes@google.com Signed-off-by: Felix Held felix-coreboot@felixheld.de Change-Id: Id08ff6428d4019303ebb6e44e13aba480cf1fde2 Reviewed-on: https://chromium-review.googlesource.com/2037891 Reviewed-on: https://review.coreboot.org/c/coreboot/+/40322 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/include/soc/southbridge.h M src/soc/amd/picasso/uart.c 3 files changed, 60 insertions(+), 25 deletions(-)
Approvals: build bot (Jenkins): Verified Marshall Dawson: Looks good to me, approved
diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig index 29ebc6d..3b12089 100644 --- a/src/soc/amd/picasso/Kconfig +++ b/src/soc/amd/picasso/Kconfig @@ -308,12 +308,10 @@
config PICASSO_UART_LEGACY bool "Decode legacy I/O range" - depends on PICASSO_CONSOLE_UART # TODO: shouldn't depend on this help - Assign I/O 3F8, 2F8, etc. to a Picasso UART. Only a single UART may - decode legacy addresses and this option enables the one used for the - console. A UART accessed with I/O does not allow all the features - of MMIO. The MMIO decode is still present when this option is used. + Assign I/O 3F8, 2F8, etc. to a Picasso UART. A UART accessed with I/O + does not allow all the features of MMIO. The MMIO decode is still + present when this option is used.
config CONSOLE_UART_BASE_ADDRESS depends on CONSOLE_SERIAL && PICASSO_CONSOLE_UART diff --git a/src/soc/amd/picasso/include/soc/southbridge.h b/src/soc/amd/picasso/include/soc/southbridge.h index 547f602..222858a 100644 --- a/src/soc/amd/picasso/include/soc/southbridge.h +++ b/src/soc/amd/picasso/include/soc/southbridge.h @@ -221,10 +221,14 @@ #define FCH_AOAC_STAT0 BIT(6) #define FCH_AOAC_STAT1 BIT(7)
-#define FCH_UART_LEGACY_DECODE 0xfedc0020 -#define FCH_LEGACY_3F8_SH 3 -#define FCH_LEGACY_2F8_SH 1 -#define FCH_LEGACY_3E8_SH 2 +#define FCH_LEGACY_UART_DECODE (ALINK_AHB_ADDRESS + 0x20) /* 0xfedc0020 */ +#define FCH_LEGACY_UART_MAP_SHIFT 8 +#define FCH_LEGACY_UART_MAP_SIZE 2 +#define FCH_LEGACY_UART_MAP_MASK 0x3 +#define FCH_LEGACY_UART_RANGE_2E8 0 +#define FCH_LEGACY_UART_RANGE_2F8 1 +#define FCH_LEGACY_UART_RANGE_3E8 2 +#define FCH_LEGACY_UART_RANGE_3F8 3
#define PM1_LIMIT 16 #define GPE0_LIMIT 28 @@ -279,6 +283,7 @@ void southbridge_init(void *chip_info); void fch_pre_init(void); void fch_early_init(void); +void set_uart_legacy_config(unsigned int uart_idx, unsigned int range_idx);
/* Initialize all the i2c buses that are marked with early init. */ void i2c_soc_early_init(void); diff --git a/src/soc/amd/picasso/uart.c b/src/soc/amd/picasso/uart.c index 2ef1979..1aa42ef 100644 --- a/src/soc/amd/picasso/uart.c +++ b/src/soc/amd/picasso/uart.c @@ -41,15 +41,59 @@ return uart_info[idx].base; }
+static bool get_uart_idx(uintptr_t base, unsigned int *idx) +{ + for (unsigned int i = 0; i < ARRAY_SIZE(uart_info); i++) { + if (base == uart_info[i].base) { + *idx = i; + return true; + } + } + + return false; +} + void clear_uart_legacy_config(void) { - write16((void *)FCH_UART_LEGACY_DECODE, 0); + write16((void *)FCH_LEGACY_UART_DECODE, 0); +} + +void set_uart_legacy_config(unsigned int uart_idx, unsigned int range_idx) +{ + uint16_t uart_legacy_decode; + uint8_t uart_map_offset; + + if (uart_idx >= ARRAY_SIZE(uart_info) || range_idx >= ARRAY_SIZE(uart_info)) + return; + + uart_legacy_decode = read16((void *)FCH_LEGACY_UART_DECODE); + /* Map uart_idx to io range_idx */ + uart_map_offset = range_idx * FCH_LEGACY_UART_MAP_SIZE + FCH_LEGACY_UART_MAP_SHIFT; + uart_legacy_decode &= ~(FCH_LEGACY_UART_MAP_MASK << uart_map_offset); + uart_legacy_decode |= uart_idx << uart_map_offset; + /* Enable io range */ + uart_legacy_decode |= 1 << range_idx; + write16((void *)FCH_LEGACY_UART_DECODE, uart_legacy_decode); +} + +static void enable_uart_legacy_decode(uintptr_t base) +{ + unsigned int idx; + const uint8_t range_idx[ARRAY_SIZE(uart_info)] = { + FCH_LEGACY_UART_RANGE_3F8, + FCH_LEGACY_UART_RANGE_2F8, + FCH_LEGACY_UART_RANGE_3E8, + FCH_LEGACY_UART_RANGE_2E8, + }; + + if (get_uart_idx(base, &idx)) { + set_uart_legacy_config(idx, range_idx[idx]); + } }
void set_uart_config(unsigned int idx) { uint32_t uart_ctrl; - uint16_t uart_leg;
if (idx >= ARRAY_SIZE(uart_info)) return; @@ -62,20 +106,6 @@ sm_pci_write32(SMB_UART_CONFIG, uart_ctrl); }
- if (CONFIG(PICASSO_UART_LEGACY) && idx != 3) { - /* Force 3F8 if idx=0, 2F8 if idx=1, 3E8 if idx=2 */ - - /* TODO: make clearer once PPR is updated */ - uart_leg = (idx << 8) | (idx << 10) | (idx << 12) | (idx << 14); - if (idx == 0) - uart_leg |= 1 << FCH_LEGACY_3F8_SH; - else if (idx == 1) - uart_leg |= 1 << FCH_LEGACY_2F8_SH; - else if (idx == 2) - uart_leg |= 1 << FCH_LEGACY_3E8_SH; - - write16((void *)FCH_UART_LEGACY_DECODE, uart_leg); - } }
static const char *uart_acpi_name(const struct device *dev) @@ -120,6 +150,8 @@ if (dev->enabled) { power_on_aoac_device(dev_id); wait_for_aoac_enabled(dev_id); + if (CONFIG(PICASSO_UART_LEGACY)) + enable_uart_legacy_decode(dev->path.mmio.addr); } else { power_off_aoac_device(dev_id); }
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40322 )
Change subject: soc/amd/picasso: Clean up legacy UART config ......................................................................
Patch Set 8:
Automatic boot test returned (PASS/FAIL/TOTAL): 8/1/9 "QEMU x86 q35/ich9" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19705 "QEMU x86 q35/ich9" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19704 "QEMU x86 i440fx/piix4" (x86_64) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/19703 "QEMU x86 i440fx/piix4" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19702 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/19701 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19709 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19708 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19707 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19706
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
build bot (Jenkins) (Code Review)
-
Felix Held (Code Review)
-
Marshall Dawson (Code Review)
-
Martin Roth (Code Review)
-
Nico Huber (Code Review)
-
Rob Barnes (Code Review)