[Xen-devel,3/3,v3] xen: Fix 16550 UART console for HP Moonshot (Aarch64) platform

Message ID 1511523552-23628-4-git-send-email-bhupinder.thakur@linaro.org
State New
Headers show
Series
  • xen: ACPI/SPCR based initialization of 8250 UART
Related show

Commit Message

Bhupinder Thakur Nov. 24, 2017, 11:39 a.m.
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 in the following commit:

    commit 79a648328d2a604524a30523ca763fbeca0f70e3
    Author: Loc Ho <lho@apm.com>
    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@apm.com>
    Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---
Changes since v2:
- removed the use of local variable xgene_8250 in xgene_8250_erratum_present

CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Julien Grall <julien.grall@arm.com>

 xen/drivers/char/ns16550.c | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

Comments

Konrad Rzeszutek Wilk Nov. 24, 2017, 1:51 p.m. | #1
On Fri, Nov 24, 2017 at 05:09:12PM +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 in the following commit:
> 
>     commit 79a648328d2a604524a30523ca763fbeca0f70e3
>     Author: Loc Ho <lho@apm.com>
>     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@apm.com>
>     Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>     Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> Changes since v2:
> - removed the use of local variable xgene_8250 in xgene_8250_erratum_present
> 
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: George Dunlap <George.Dunlap@eu.citrix.com>
> CC: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Tim Deegan <tim@xen.org>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Julien Grall <julien.grall@arm.com>
> 
>  xen/drivers/char/ns16550.c | 38 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index af4712f..8c4720a 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1571,6 +1571,30 @@ DT_DEVICE_END
>  #endif /* HAS_DEVICE_TREE */
>  
>  #if defined(CONFIG_ACPI) && defined(CONFIG_ARM)
> +/*
> + * 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)
> +{
> +    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 )
> +        return true;
> +
> +    if ( !memcmp(tb->header.oem_table_id, "ProLiant",
> +         ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 1 )
> +        return true;
> +
> +    return false;
> +}
>  
>  static int ns16550_init_acpi(struct ns16550 **puart)
>  {
> @@ -1596,7 +1620,19 @@ static int ns16550_init_acpi(struct ns16550 **puart)
>      uart->io_base   = spcr->serial_port.address;
>      uart->irq       = spcr->interrupt;
>      uart->reg_width = spcr->serial_port.bit_width / 8;
> -    uart->reg_shift = 0;
> +
> +    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_NUM_REGS << uart->reg_shift;
>  
>      irq_set_type(spcr->interrupt, spcr->interrupt_type);
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
Jan Beulich Nov. 27, 2017, 10:06 a.m. | #2
>>> On 24.11.17 at 12:39, <bhupinder.thakur@linaro.org> wrote:
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1571,6 +1571,30 @@ DT_DEVICE_END
>  #endif /* HAS_DEVICE_TREE */
>  
>  #if defined(CONFIG_ACPI) && defined(CONFIG_ARM)
> +/*
> + * 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)

Is this really to be considered an erratum? From the description it
doesn't sound like this couldn't have been a deliberate decision.
IOW - does their behavior contradict any spec? (ACPI not providing
information in field and access width looks suspicious too - GAS fields
exist for both.)

Jan
Julien Grall Nov. 27, 2017, 10:30 a.m. | #3
+ Graeme

On 27/11/17 10:06, Jan Beulich wrote:
>>>> On 24.11.17 at 12:39, <bhupinder.thakur@linaro.org> wrote:
>> --- a/xen/drivers/char/ns16550.c
>> +++ b/xen/drivers/char/ns16550.c
>> @@ -1571,6 +1571,30 @@ DT_DEVICE_END
>>   #endif /* HAS_DEVICE_TREE */
>>   
>>   #if defined(CONFIG_ACPI) && defined(CONFIG_ARM)
>> +/*
>> + * 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)
> 
> Is this really to be considered an erratum? From the description it
> doesn't sound like this couldn't have been a deliberate decision.
> IOW - does their behavior contradict any spec? (ACPI not providing
> information in field and access width looks suspicious too - GAS fields
> exist for both.)

I believe the problem here is the firmware table does not describe 
correctly the hardware. I have CCed Graeme which might be able to confirm.

Cheers,
Graeme Gregory Nov. 27, 2017, 10:37 a.m. | #4
On 27 November 2017 at 10:30, Julien Grall <julien.grall@linaro.org> wrote:
> + Graeme
>
> On 27/11/17 10:06, Jan Beulich wrote:
>>>>>
>>>>> On 24.11.17 at 12:39, <bhupinder.thakur@linaro.org> wrote:
>>>
>>> --- a/xen/drivers/char/ns16550.c
>>> +++ b/xen/drivers/char/ns16550.c
>>> @@ -1571,6 +1571,30 @@ DT_DEVICE_END
>>>   #endif /* HAS_DEVICE_TREE */
>>>     #if defined(CONFIG_ACPI) && defined(CONFIG_ARM)
>>> +/*
>>> + * 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)
>>
>>
>> Is this really to be considered an erratum? From the description it
>> doesn't sound like this couldn't have been a deliberate decision.
>> IOW - does their behavior contradict any spec? (ACPI not providing
>> information in field and access width looks suspicious too - GAS fields
>> exist for both.)
>
>
> I believe the problem here is the firmware table does not describe correctly
> the hardware. I have CCed Graeme which might be able to confirm.
>
Yup, their firmware is wrong, we did tell them many times!

Graeme

Patch

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index af4712f..8c4720a 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -1571,6 +1571,30 @@  DT_DEVICE_END
 #endif /* HAS_DEVICE_TREE */
 
 #if defined(CONFIG_ACPI) && defined(CONFIG_ARM)
+/*
+ * 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)
+{
+    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 )
+        return true;
+
+    if ( !memcmp(tb->header.oem_table_id, "ProLiant",
+         ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 1 )
+        return true;
+
+    return false;
+}
 
 static int ns16550_init_acpi(struct ns16550 **puart)
 {
@@ -1596,7 +1620,19 @@  static int ns16550_init_acpi(struct ns16550 **puart)
     uart->io_base   = spcr->serial_port.address;
     uart->irq       = spcr->interrupt;
     uart->reg_width = spcr->serial_port.bit_width / 8;
-    uart->reg_shift = 0;
+
+    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_NUM_REGS << uart->reg_shift;
 
     irq_set_type(spcr->interrupt, spcr->interrupt_type);