Attention is currently required from: Lance Zhao, Furquan Shaikh, Martin Roth. Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49317 )
Change subject: drivers/uart: Add ACPI SPCR table generation ......................................................................
Patch Set 4:
(9 comments)
File src/drivers/uart/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/49317/comment/1aac6e3e_9aa2c520 PS4, Line 219: uart_acpi_write_spcr Can you follow the patter of the other ACPI tables? i.e., acpi_create_spcr and acpi_fill_spcr
https://review.coreboot.org/c/coreboot/+/49317/comment/9554373d_fb7cf915 PS4, Line 248: get_spcr_baudrate It's a bit confusing mixing devicetree properties with kconfig properties. Can we pick one or the other?
File src/drivers/uart/acpi/chip.h:
https://review.coreboot.org/c/coreboot/+/49317/comment/2ae6b845_d35e89d6 PS4, Line 8: drivers_uart_acpi_config So this struct is about defining UART peripherals not UART controllers. AFAIK coreboot doesn't have a common UART controller chip.
File src/drivers/uart/uart8250io.c:
https://review.coreboot.org/c/coreboot/+/49317/comment/2d620ea0_68c8be55 PS4, Line 126: addrl In order for this to properly work, we need to configure the UART with a 1.8MHz clock.
https://review.coreboot.org/c/coreboot/+/49317/comment/3ac40223_e08f9a20 PS4, Line 131: access_size Missing .bit_width = 8;
File src/drivers/uart/uart8250mem.c:
https://review.coreboot.org/c/coreboot/+/49317/comment/e9bb49a5_887f9049 PS4, Line 164: access_size Missing .bit_width=32
https://review.coreboot.org/c/coreboot/+/49317/comment/32702c6b_8f8e1c42 PS4, Line 166: access_size Missing .bit_width=8
File src/include/acpi/acpi_device.h:
https://review.coreboot.org/c/coreboot/+/49317/comment/be181c8b_a0bc1952 PS4, Line 463: acpi_spcr Can you add this to acpi/acpi.h. It's where the other tables are defined.
https://review.coreboot.org/c/coreboot/+/49317/comment/d698ba22_0ba5182e PS4, Line 476: flow_control
stop bits is offset 60 and terminal is offset 62, looks like offset 61 is reasonable.
Ack
-
Raul Rangel (Code Review)