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

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

[Shanker Donthineni]

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.

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);
+}
+
+static void __init pl011_init_preirq(struct serial_port *port)
+{
+    struct pl011 *uart = port->uart;
+    unsigned int cr;
+
+    /* No interrupts, please. */
+    pl011_write(uart, IMSC, 0);
+
+    if ( !uart->sbsa )
+    {
+        /* Definitely no DMA */
+        pl011_write(uart, DMACR, 0x0);
+
+        /* Configure baudrate */
+	pl011_init_baudrate(port);
+    }
     /* Clear errors */
     pl011_write(uart, RSR, 0);
 
@@ -121,10 +133,13 @@ static void __init pl011_init_preirq(struct serial_port *port)
     pl011_write(uart, IMSC, 0);
     pl011_write(uart, ICR, ALLI);
 
-    /* Enable the UART for RX and TX; keep RTS and DTR */
-    cr = pl011_read(uart, CR);
-    cr &= RTS | DTR;
-    pl011_write(uart, CR, cr | RXE | TXE | UARTEN);
+    if ( !uart->sbsa )
+    {
+        /* Enable the UART for RX and TX; keep RTS and DTR */
+        cr = pl011_read(uart, CR);
+        cr &= RTS | DTR;
+        pl011_write(uart, CR, cr | RXE | TXE | UARTEN);
+    }
 }
 
 static void __init pl011_init_postirq(struct serial_port *port)
@@ -226,7 +241,7 @@ static struct uart_driver __read_mostly pl011_driver = {
     .vuart_info   = pl011_vuart,
 };
 
-static int __init pl011_uart_init(int irq, u64 addr, u64 size)
+static int __init pl011_uart_init(int irq, u64 addr, u64 size, bool sbsa)
 {
     struct pl011 *uart;
 
@@ -237,6 +252,7 @@ static int __init pl011_uart_init(int irq, u64 addr, u64 size)
     uart->data_bits = 8;
     uart->parity    = PARITY_NONE;
     uart->stop_bits = 1;
+    uart->sbsa = sbsa;
 
     uart->regs = ioremap_nocache(addr, size);
     if ( !uart->regs )
@@ -285,7 +301,7 @@ static int __init pl011_dt_uart_init(struct dt_device_node *dev,
         return -EINVAL;
     }
 
-    res = pl011_uart_init(res, addr, size);
+    res = pl011_uart_init(res, addr, size, false);
     if ( res < 0 )
     {
         printk("pl011: Unable to initialize\n");
@@ -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;
+
     /* trigger/polarity information is not available in spcr */
     irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH);
 
     res = pl011_uart_init(spcr->interrupt, spcr->serial_port.address,
-                          PAGE_SIZE);
+                          PAGE_SIZE, sbsa);
     if ( res < 0 )
     {
         printk("pl011: Unable to initialize\n");
         return res;
     }
 
+
+
     return 0;
 }
 
@@ -344,6 +365,11 @@ ACPI_DEVICE_START(apl011, "PL011 UART", DEVICE_SERIAL)
         .class_type = ACPI_DBG2_PL011,
         .init = pl011_acpi_uart_init,
 ACPI_DEVICE_END
+
+ACPI_DEVICE_START(asbsa32_uart, "SBSA32 UART", DEVICE_SERIAL)
+        .class_type = ACPI_DBG2_SBSA_32,
+        .init = pl011_acpi_uart_init,
+ACPI_DEVICE_END
 #endif
 
 /*
-- 
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
a Linux Foundation Collaborative Project


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

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

[Wei Chen]

Hi Shanker,

On 31 May 2016 at 22:02, Shanker Donthineni <shankerd@codeaurora.org> 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.
>
> 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);
> +}

Should we have to move these codes into a separate function?
And stop bit, parity are not baudrate setting code.

> +
> +static void __init pl011_init_preirq(struct serial_port *port)
> +{
> +    struct pl011 *uart = port->uart;
> +    unsigned int cr;
> +
> +    /* No interrupts, please. */
> +    pl011_write(uart, IMSC, 0);
> +
> +    if ( !uart->sbsa )
> +    {
> +        /* Definitely no DMA */
> +        pl011_write(uart, DMACR, 0x0);
> +
> +        /* Configure baudrate */
> +      pl011_init_baudrate(port);
Tab should be replaced by spaces.

> +    }
>      /* Clear errors */
>      pl011_write(uart, RSR, 0);
>
> @@ -121,10 +133,13 @@ static void __init pl011_init_preirq(struct serial_port *port)
>      pl011_write(uart, IMSC, 0);
>      pl011_write(uart, ICR, ALLI);
>
> -    /* Enable the UART for RX and TX; keep RTS and DTR */
> -    cr = pl011_read(uart, CR);
> -    cr &= RTS | DTR;
> -    pl011_write(uart, CR, cr | RXE | TXE | UARTEN);
> +    if ( !uart->sbsa )
> +    {
> +        /* Enable the UART for RX and TX; keep RTS and DTR */
> +        cr = pl011_read(uart, CR);
> +        cr &= RTS | DTR;
> +        pl011_write(uart, CR, cr | RXE | TXE | UARTEN);
> +    }
>  }
>
>  static void __init pl011_init_postirq(struct serial_port *port)
> @@ -226,7 +241,7 @@ static struct uart_driver __read_mostly pl011_driver = {
>      .vuart_info   = pl011_vuart,
>  };
>
> -static int __init pl011_uart_init(int irq, u64 addr, u64 size)
> +static int __init pl011_uart_init(int irq, u64 addr, u64 size, bool sbsa)
I would add a type parameter instead of sbsa, and pass
spcr->interface_type to it.
>  {
>      struct pl011 *uart;
>
> @@ -237,6 +252,7 @@ static int __init pl011_uart_init(int irq, u64 addr, u64 size)
>      uart->data_bits = 8;
>      uart->parity    = PARITY_NONE;
>      uart->stop_bits = 1;
> +    uart->sbsa = sbsa;
>
>      uart->regs = ioremap_nocache(addr, size);
>      if ( !uart->regs )
> @@ -285,7 +301,7 @@ static int __init pl011_dt_uart_init(struct dt_device_node *dev,
>          return -EINVAL;
>      }
>
> -    res = pl011_uart_init(res, addr, size);
> +    res = pl011_uart_init(res, addr, size, false);
>      if ( res < 0 )
>      {
>          printk("pl011: Unable to initialize\n");
> @@ -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;
> +

I would move this code to pl011_uart_init.

>      /* trigger/polarity information is not available in spcr */
>      irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH);
>
>      res = pl011_uart_init(spcr->interrupt, spcr->serial_port.address,
> -                          PAGE_SIZE);
> +                          PAGE_SIZE, sbsa);
>      if ( res < 0 )
>      {
>          printk("pl011: Unable to initialize\n");
>          return res;
>      }
>
> +
> +
These two new lines are spurious changes.
>      return 0;
>  }
>
> @@ -344,6 +365,11 @@ ACPI_DEVICE_START(apl011, "PL011 UART", DEVICE_SERIAL)
>          .class_type = ACPI_DBG2_PL011,
>          .init = pl011_acpi_uart_init,
>  ACPI_DEVICE_END
> +
> +ACPI_DEVICE_START(asbsa32_uart, "SBSA32 UART", DEVICE_SERIAL)
> +        .class_type = ACPI_DBG2_SBSA_32,
> +        .init = pl011_acpi_uart_init,
> +ACPI_DEVICE_END
>  #endif
>
>  /*
> --
> Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

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

[Shanker Donthineni]

Hi Wui,

On 06/01/2016 01:47 AM, Wei Chen wrote:
> Hi Shanker,
>
> On 31 May 2016 at 22:02, Shanker Donthineni <shankerd@codeaurora.org> 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.
>>
>> 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);
>> +}
> Should we have to move these codes into a separate function?
> And stop bit, parity are not baudrate setting code.
No need to move here and also it is not related to baudrate. Anyway
this register should not be accessed for SBSA interface.

I am okay to keep this change at the original place.
>> +
>> +static void __init pl011_init_preirq(struct serial_port *port)
>> +{
>> +    struct pl011 *uart = port->uart;
>> +    unsigned int cr;
>> +
>> +    /* No interrupts, please. */
>> +    pl011_write(uart, IMSC, 0);
>> +
>> +    if ( !uart->sbsa )
>> +    {
>> +        /* Definitely no DMA */
>> +        pl011_write(uart, DMACR, 0x0);
>> +
>> +        /* Configure baudrate */
>> +      pl011_init_baudrate(port);
> Tab should be replaced by spaces.

I'll fix it in v3 patch.
>> +    }
>>      /* Clear errors */
>>      pl011_write(uart, RSR, 0);
>>
>> @@ -121,10 +133,13 @@ static void __init pl011_init_preirq(struct 
>> serial_port *port)
>>      pl011_write(uart, IMSC, 0);
>>      pl011_write(uart, ICR, ALLI);
>>
>> -    /* Enable the UART for RX and TX; keep RTS and DTR */
>> -    cr = pl011_read(uart, CR);
>> -    cr &= RTS | DTR;
>> -    pl011_write(uart, CR, cr | RXE | TXE | UARTEN);
>> +    if ( !uart->sbsa )
>> +    {
>> +        /* Enable the UART for RX and TX; keep RTS and DTR */
>> +        cr = pl011_read(uart, CR);
>> +        cr &= RTS | DTR;
>> +        pl011_write(uart, CR, cr | RXE | TXE | UARTEN);
>> +    }
>>  }
>>
>>  static void __init pl011_init_postirq(struct serial_port *port)
>> @@ -226,7 +241,7 @@ static struct uart_driver __read_mostly pl011_driver = 
>> {
>>      .vuart_info   = pl011_vuart,
>>  };
>>
>> -static int __init pl011_uart_init(int irq, u64 addr, u64 size)
>> +static int __init pl011_uart_init(int irq, u64 addr, u64 size, bool sbsa)
> I would add a type parameter instead of sbsa, and pass
> spcr->interface_type to it.
This function is common to both the ACPI and DTB, don't like to add an
another gaurd '#ifdef CONFIG_ACPI' inside this function.

>>  {
>>      struct pl011 *uart;
>>
>> @@ -237,6 +252,7 @@ static int __init pl011_uart_init(int irq, u64 addr, 
>> u64 size)
>>      uart->data_bits = 8;
>>      uart->parity    = PARITY_NONE;
>>      uart->stop_bits = 1;
>> +    uart->sbsa = sbsa;
>>
>>      uart->regs = ioremap_nocache(addr, size);
>>      if ( !uart->regs )
>> @@ -285,7 +301,7 @@ static int __init pl011_dt_uart_init(struct 
>> dt_device_node *dev,
>>          return -EINVAL;
>>      }
>>
>> -    res = pl011_uart_init(res, addr, size);
>> +    res = pl011_uart_init(res, addr, size, false);
>>      if ( res < 0 )
>>      {
>>          printk("pl011: Unable to initialize\n");
>> @@ -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;
>> +
> I would move this code to pl011_uart_init.
See above comment, ACPI_DBG2_SBSA_32 is not defined for DTS based xen.

>>      /* trigger/polarity information is not available in spcr */
>>      irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH);
>>
>>      res = pl011_uart_init(spcr->interrupt, spcr->serial_port.address,
>> -                          PAGE_SIZE);
>> +                          PAGE_SIZE, sbsa);
>>      if ( res < 0 )
>>      {
>>          printk("pl011: Unable to initialize\n");
>>          return res;
>>      }
>>
>> +
>> +
> These two new lines are spurious changes.
I'll fix it.

>>      return 0;
>>  }
>>
>> @@ -344,6 +365,11 @@ ACPI_DEVICE_START(apl011, "PL011 UART", 
>> DEVICE_SERIAL)
>>          .class_type = ACPI_DBG2_PL011,
>>          .init = pl011_acpi_uart_init,
>>  ACPI_DEVICE_END
>> +
>> +ACPI_DEVICE_START(asbsa32_uart, "SBSA32 UART", DEVICE_SERIAL)
>> +        .class_type = ACPI_DBG2_SBSA_32,
>> +        .init = pl011_acpi_uart_init,
>> +ACPI_DEVICE_END
>>  #endif
>>
>>  /*
>> --
>> Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel

-- 
Shanker Donthineni
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project


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

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

[Julien Grall]

Hello Shanker,

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.

> 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.

> @@ -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.

Regards,

-- 
Julien Grall

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

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

[Shanker Donthineni]

On 06/01/2016 08:49 AM, Julien Grall wrote:
> Hello Shanker,
>
> 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.

>> @@ -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.

> Regards,
>

-- 
Shanker Donthineni
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project


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

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

[Julien Grall]

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

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

[Andre Przywara]

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

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

[Julien Grall]

Hi Andre,

On 01/06/16 16:56, Andre Przywara wrote:
>>> 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.

Thank you for the explanation.

> So if possible we should switch to 32-bit accessors for the SBSA UART.

The driver use 32-bit accessors exclusively.

>
>> 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.

If so, I would replace the read to UARTMIS by a combination UARTIMSC and 
UARTRIS to avoid any issue with the UART based on SBSA v2.3.

Cheers,

-- 
Julien Grall

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

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

[Shanker Donthineni]

On 06/01/2016 11:18 AM, Julien Grall wrote:
> Hi Andre,
>
> On 01/06/16 16:56, Andre Przywara wrote:
>>>> 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.
>
> Thank you for the explanation.
>
>> So if possible we should switch to 32-bit accessors for the SBSA UART.
>
> The driver use 32-bit accessors exclusively.
>
>>
>>> 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.
>
> If so, I would replace the read to UARTMIS by a combination UARTIMSC and UARTRIS to avoid any issue with the UART based on SBSA v2.3.
>
I'll change the code to use registers UARTIMSC and UARTRIS instead of  UARTMIS.
> Cheers,
>

-- 
Shanker Donthineni
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project


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