Re: [PATCH v2] arm/acpi: Add Server Base System Architecture UART support

[Andre Przywara <andre.przywara@arm.com>]

Hi,

On 01/06/16 16:40, Julien Grall wrote:
> Hello Shanker,
> 
> On 01/06/16 16:14, Shanker Donthineni wrote:
>>
>>
>> On 06/01/2016 08:49 AM, Julien Grall wrote:
>>> On 31/05/16 15:02, Shanker Donthineni wrote:
>>>> The ARM Server Base System Architecture describes a generic UART
>>>> interface. It doesn't support clock control registers, modem
>>>> control, DMA and hardware flow control features. So, extend the
>>>> driver probe() to handle SBSA interface and skip the accessing
>>>> PL011 registers that are not described in SBSA document.
>>>
>>> Please mention the version of the spec in the commit message.
>>>
>> Sure, I'll do.
>>>> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
>>>> ---
>>>> Changes since v1:
>>>>      Don't access UART registers that are not part of SBSA document.
>>>>      Move setting baudrate function to a separate function.
>>>>
>>>>    xen/drivers/char/pl011.c | 56
>>> +++++++++++++++++++++++++++++++++++-------------
>>>>    1 file changed, 41 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
>>>> index 1212d5c..b57f3b0 100644
>>>> --- a/xen/drivers/char/pl011.c
>>>> +++ b/xen/drivers/char/pl011.c
>>>> @@ -41,6 +41,7 @@ static struct pl011 {
>>>>        /* struct timer timer; */
>>>>        /* unsigned int timeout_ms; */
>>>>        /* bool_t probing, intr_works; */
>>>> +    bool sbsa;  /* ARM SBSA generic interface */
>>>>    } pl011_com = {0};
>>>>
>>>>    /* These parity settings can be ORed directly into the LCR. */
>>>> @@ -81,17 +82,10 @@ static void pl011_interrupt(int irq, void *data,
>>> struct cpu_user_regs *regs)
>>>>        }
>>>>    }
>>>>
>>>> -static void __init pl011_init_preirq(struct serial_port *port)
>>>> +static void __init pl011_init_baudrate(struct serial_port *port)
>>>>    {
>>>>        struct pl011 *uart = port->uart;
>>>>        unsigned int divisor;
>>>> -    unsigned int cr;
>>>> -
>>>> -    /* No interrupts, please. */
>>>> -    pl011_write(uart, IMSC, 0);
>>>> -
>>>> -    /* Definitely no DMA */
>>>> -    pl011_write(uart, DMACR, 0x0);
>>>>
>>>>        /* Line control and baud-rate generator. */
>>>>        if ( uart->baud != BAUD_AUTO )
>>>> @@ -114,6 +108,24 @@ static void __init pl011_init_preirq(struct
>>> serial_port *port)
>>>>                                | FEN
>>>>                                | ((uart->stop_bits - 1) << 3)
>>>>                                | uart->parity);
>>>> +}
>>>
>>> As mentioned on the previous version, the code to set/read the
>>> baudrate is just wrong. The clock frequency is hardcoded rather than
>>> read from the firmware.
>>>
>>> However, the baudrate is always set to BAUD_AUTO for this driver, and
>>> never used after. So all this code should be dropped.
>>>
>> SPCR (non-SBSA) spec has baudrate, stop and parity  information, I
>> think we should support setting the correct baudrate according to SPCR
>> table instead of removing the code.
>> I need your opinion on this.
> 
> Which is not useful because we don't have the clock frequency in hand to
> compute the divisor.
> 
> For ACPI, it just not available in the SPCR. For Device-Tree, we would
> need to parse the list of clocks which could be cumbersome.
> 
> I think it is fine to expect the firmware to configure the UART baudrate
> properly if required.
> 
>>
>>>> @@ -315,6 +331,7 @@ static int __init pl011_acpi_uart_init(const void
>>> *data)
>>>>    {
>>>>        acpi_status status;
>>>>        struct acpi_table_spcr *spcr = NULL;
>>>> +    bool sbsa;
>>>>        int res;
>>>>
>>>>        status = acpi_get_table(ACPI_SIG_SPCR, 0,
>>>> @@ -326,17 +343,21 @@ static int __init pl011_acpi_uart_init(const void
>>> *data)
>>>>            return -EINVAL;
>>>>        }
>>>>
>>>> +    sbsa = (spcr->interface_type == ACPI_DBG2_SBSA_32) ? true : false;
>>>
>>> sbsa = (spcr->interface_type == ACPI_DBG2_SBSA_32);
>>>
>>> However, can you explain why you kept ACPI_DBG2_SBSA_32 and not
>>> ACPI_DBG2_SBSA? The former is deprecated whilst the latter is the
>>> official one.
>>>
>> Qualcomm Technologies QDF2XXX ARM SBSA serial port hardware require
>> registers access should be 32-bit, so our firmware sets interface type
>> to  ACPI_DBG2_SBSA_32.
>>
>> I would like to support both the interfaces
>> (ACPI_DBG2_SBSA_32/ACPI_DBG2_SBSA) If you are okay.
> 
> I am a bit puzzled, so your UART is only supporting 32-bit access (i.e
> no 16-bit and 8-bit access)?

Just to clarify: the SBSA spec is not very clear in this respect, as it
only speaks of "a set of 32-bit registers". But this has been
interpreted as "supports 32-bit accesses". So there was a patch lately
in Linux to change all accesses to SBSA UARTs to 32-bit accessors
(writel/readl), because there is at least this one mentioned platform
that requires this, while all the other relevant platforms we could get
hold of can also cope with 32-bit accesses. This may not be true for all
legacy PL011 implementations out there, but for the SBSA subset this is
deemed a safe assumption.
So if possible we should switch to 32-bit accessors for the SBSA UART.

> If so your platform is based on SBSA v2.3, and therefore the PL011
> driver needs more modification to support properly your platform. For
> instance, the register UARTMIS is not present in v2.3 but used by the
> driver.

I think this has been changed in the spec lately, it was present in
earlier revisions of the spec.

Cheers,
Andre.

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