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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Thu, Nov 09, 2017 at 03:49:24PM +0530, 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:
> 
>     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>

Any particular reason you offset this whole commit description by four spaces?

> 
> 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(-)

This is v2 posting, but I don't see what changed.

Usually you do something like this:

v1: New posting
v2: Nothing changed from v1.

or

v1: New posting
v2: Added more folks on CC
    Added consts in XYZ..

> 
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index cf42fce..bb01c46 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1517,6 +1517,33 @@ static int ns16550_init_dt(struct ns16550 *uart,
>  
>  #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 )
> +        xgene_8250 = true;

Why not just 'return true' ?

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

And return true here too?
> +
> +    return xgene_8250;

And then this is just 'return false' and you don't have xgen_8250 on the stack?

> +}
> +
>  static int ns16550_init_acpi(struct ns16550 *uart,
>                               const void *data)
>  {
> @@ -1539,9 +1566,20 @@ static int ns16550_init_acpi(struct ns16550 *uart,
>      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

s/for/For/
> +         * 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;
>      irq_set_type(spcr->interrupt, spcr->interrupt_type);
>  
>      return 0;
> -- 
> 2.7.4
> 

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