Re: [Xen-devel] [PATCH 2/2] xen: Fix 16550 UART console for HP Moonshot (Aarch64) platform

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Bhupinder,

On 02/11/17 10:13, Bhupinder Thakur wrote:
>      The console was not working on HP Moonshot (HPE Proliant Aarch64) because
>      the UART registers were accessed as 8-bit aligned addresses. However,
>      registers are 32-bit aligned for HP Moonshot.
>
>      Since ACPI/SPCR table does not specify the register shift to be applied to 
> the
>      register offset, this patch implements an erratum to correctly set the 
> register
>      shift for HP Moonshot.
>
>      Similar erratum was implemented in linux:
Can you remove the tab at the beginning of each of the lines above?

>      commit 79a648328d2a604524a30523ca763fbeca0f70e3
>      Author: Loc Ho <lho@xxxxxxx>
>      Date:   Mon Jul 3 14:33:09 2017 -0700
>
>          ACPI: SPCR: Workaround for APM X-Gene 8250 UART 32-alignment errata
>
>          APM X-Gene verion 1 and 2 have an 8250 UART with its register
>          aligned to 32-bit. In addition, the latest released BIOS
>          encodes the access field as 8-bit access instead 32-bit access.
>          This causes no console with ACPI boot as the console
>          will not match X-Gene UART port due to the lack of mmio32
>          option.
>
>          Signed-off-by: Loc Ho <lho@xxxxxxx>
>          Acked-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>          Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>
> ---
> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> CC: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> CC: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Tim Deegan <tim@xxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Julien Grall <julien.grall@xxxxxxx>
>
>   xen/drivers/char/ns16550.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 40 insertions(+), 2 deletions(-)
>
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index b3f6d85..e716aba 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1542,6 +1542,33 @@ DT_DEVICE_END
>   #ifdef CONFIG_ACPI
>   #include <xen/acpi.h>
>   
> +/*
> + * APM X-Gene v1 and v2 UART hardware is an 16550 like device but has its
> + * register aligned to 32-bit. In addition, the BIOS also encoded the
> + * access width to be 8 bits. This function detects this errata condition.
> + */
> +static bool xgene_8250_erratum_present(struct acpi_table_spcr *tb)
> +{
> +    bool xgene_8250 = false;
> +
> +    if ( tb->interface_type != ACPI_DBG2_16550_COMPATIBLE )
> +        return false;
> +
> +    if ( memcmp(tb->header.oem_id, "APMC0D", ACPI_OEM_ID_SIZE) &&
> +         memcmp(tb->header.oem_id, "HPE   ", ACPI_OEM_ID_SIZE) )
> +        return false;
> +
> +    if ( !memcmp(tb->header.oem_table_id, "XGENESPC",
> +         ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 0 )
Please align ACPI_OEM_TABLE_ID_SIZE with ( after memcmp.

> +        xgene_8250 = true;
> +
> +    if ( !memcmp(tb->header.oem_table_id, "ProLiant",
> +         ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 1 )
Ditto.

> +        xgene_8250 = true;
> +
> +    return xgene_8250;
> +}
> +
>   static int __init ns16550_acpi_uart_init(const void *data)
>   {
>       struct ns16550 *uart;
> @@ -1568,9 +1595,20 @@ static int __init ns16550_acpi_uart_init(const void 
> *data)
>       uart->io_base = spcr->serial_port.address;
>       uart->irq = spcr->interrupt;
>       uart->reg_width = spcr->serial_port.bit_width/8;
> -    uart->reg_shift = 0;
> -    uart->io_size = UART_MAX_REG<<uart->reg_shift;
>   
> +    if ( xgene_8250_erratum_present(spcr) )
> +    {
> +        /*
> +         * for xgene v1 and v2 the registers are 32-bit and so a
> +         * register shift of 2 has to be applied to get the
> +         * correct register offset.
> +         */
> +        uart->reg_shift = 2;
> +    }
> +    else
> +        uart->reg_shift = 0;
> +
> +    uart->io_size = UART_MAX_REG<<uart->reg_shift;
I am not why this change here. It might because of diff is confused?

>       irq_set_type(spcr->interrupt, spcr->interrupt_type);
>   
>       uart->vuart.base_addr = uart->io_base;
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel