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

2016-06-02 Thread 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

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

2016-06-01 Thread 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: [Xen-devel] [PATCH v2] arm/acpi: Add Server Base System Architecture UART support

2016-06-01 Thread 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 
 ---
 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

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

2016-06-01 Thread 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 
---
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: [Xen-devel] [PATCH v2] arm/acpi: Add Server Base System Architecture UART support

2016-06-01 Thread 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 
>> ---
>> 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: [Xen-devel] [PATCH v2] arm/acpi: Add Server Base System Architecture UART support

2016-06-01 Thread 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 
---
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: [Xen-devel] [PATCH v2] arm/acpi: Add Server Base System Architecture UART support

2016-06-01 Thread 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  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 
>> ---
>> 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;

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

2016-06-01 Thread Wei Chen 
Hi Shanker,

On 31 May 2016 at 22: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.
>
> Signed-off-by: Shanker Donthineni 
> ---
> 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,

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

2016-05-31 Thread 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 
---
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