Re: [U-Boot] [PATCH v2 5/5] x86: acpi: Generate SPCR table

On 15.11.18 18:58, Andy Shevchenko wrote:

Microsoft specifies a SPCR (Serial Port Console Redirection Table) [1]. Let's provide it in U-Boot.

[1] https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85)....

Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com

arch/x86/include/asm/acpi_table.h | 2 + arch/x86/lib/acpi_table.c | 85 +++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+)

diff --git a/arch/x86/include/asm/acpi_table.h b/arch/x86/include/asm/acpi_table.h index 51cc806673..e3b65cff66 100644 --- a/arch/x86/include/asm/acpi_table.h +++ b/arch/x86/include/asm/acpi_table.h @@ -327,6 +327,8 @@ struct acpi_global_nvs; #define ACPI_DBG2_USB_XHCI 0x0000 #define ACPI_DBG2_USB_EHCI 0x0001

+#define ACPI_DBG2_UNKNOWN 0x00FF

/* SPCR (Serial Port Console Redirection table) */ struct __packed acpi_spcr { struct acpi_table_header header; diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c index c6b2026613..d8b44aea02 100644 --- a/arch/x86/lib/acpi_table.c +++ b/arch/x86/lib/acpi_table.c @@ -12,6 +12,7 @@ #include <cpu.h> #include <dm.h> #include <dm/uclass-internal.h> +#include <serial.h> #include <version.h> #include <asm/acpi/global_nvs.h> #include <asm/acpi_table.h> @@ -338,6 +339,82 @@ static void acpi_create_mcfg(struct acpi_mcfg *mcfg) header->checksum = table_compute_checksum((void *)mcfg, header->length); }

+static void acpi_create_spcr(struct acpi_spcr *spcr) +{

• struct acpi_table_header *header = &(spcr->header);
• struct serial_device_info info = {0};
• int access_size;
• int ret;
• /* Fill out header fields */
• acpi_fill_header(header, "SPCR");
• header->length = sizeof(struct acpi_spcr);
• header->revision = 2;
• ret = serial_getinfo(&info);
• if (ret)

• spcr->interface_type = ACPI_DBG2_UNKNOWN;
  

• else

• spcr->interface_type = ACPI_DBG2_16550_COMPATIBLE;
  

This sounds like pretty subtle semantics. Could you just include the interface type in &info?

The main problem I'm seeing is that your UART might be a PL011 compatible which could still implement getinfo() but then wouldn't be 16660 compatible.

• debug("UART type %u (serial_getinfo() rc=%d)\n", spcr->interface_type, ret);
• /* Encode baud rate */
• switch (info.baudrate) {
• case 9600:

• spcr->baud_rate = 3;
  

• break;
  

• case 19200:

• spcr->baud_rate = 4;
  

• break;
  

• case 57600:

• spcr->baud_rate = 6;
  

• break;
  

• case 115200:

• spcr->baud_rate = 7;
  

• break;
  

Are any higher (and lower) ones specified too? If so, you may want to add them as well.

• default:

• spcr->baud_rate = 0;
  

• break;
  

• }
• /* Encode register access size */
• switch (info.reg_shift) {
• case 0:

• access_size = ACPI_ACCESS_SIZE_BYTE_ACCESS;
  

• break;
  

• case 1:

• access_size = ACPI_ACCESS_SIZE_WORD_ACCESS;
  

• break;
  

• case 2:

• access_size = ACPI_ACCESS_SIZE_DWORD_ACCESS;
  

• break;
  

• case 3:

• access_size = ACPI_ACCESS_SIZE_QWORD_ACCESS;
  

• break;
  

• default:

• access_size = ACPI_ACCESS_SIZE_UNDEFINED;
  

• break;
  

• }
• spcr->serial_port.space_id = ACPI_ADDRESS_SPACE_MEMORY;

Can you guarantee that? Should probably be part of &info as well if addr is.

• spcr->serial_port.bit_width = info.reg_width;
• spcr->serial_port.bit_offset = info.reg_offset;
• spcr->serial_port.access_size = access_size;
• spcr->serial_port.addrl = info.addr >> 0;
• spcr->serial_port.addrh = info.addr >> 32;
• /* REVISIT: Hard coded values for now */
• spcr->parity = 0;
• spcr->stop_bits = 1;

IMHO those should be part of &info as well. If you have to hard code them, better hard code them in the driver rather than the generic ACPI generation code.

Alex

• /* REVISIT: No PCI devices for now */
• spcr->pci_device_id = 0xffff;
• spcr->pci_vendor_id = 0xffff;
• /* Fix checksum */
• header->checksum = table_compute_checksum((void *)spcr, header->length);

+}

/*

• QEMU's version of write_acpi_tables is defined in drivers/misc/qfw.c

*/ @@ -352,6 +429,7 @@ ulong write_acpi_tables(ulong start) struct acpi_fadt *fadt; struct acpi_mcfg *mcfg; struct acpi_madt *madt;

• struct acpi_spcr *spcr; int i;

  current = start;

@@ -440,6 +518,13 @@ ulong write_acpi_tables(ulong start) acpi_add_table(rsdp, mcfg); current = ALIGN(current, 16);

• debug("ACPI: * SPCR\n");

• spcr = (struct acpi_spcr *)current;

• acpi_create_spcr(spcr);

• current += spcr->header.length;

• acpi_add_table(rsdp, spcr);

• current = ALIGN(current, 16);

• debug("current = %x\n", current);

  acpi_rsdp_addr = (unsigned long)rsdp;