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

[Julien Grall <julien.grall@arm.com>]

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

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.

Regards,

-- 
Julien Grall

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