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

[Alexander Graf]

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).aspx
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko at 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;
>