diff mbox series

[V3,2/5] xen/arm: drivers: scif: Extend driver to handle other interfaces

Message ID 1554718489-11318-3-git-send-email-olekstysh@gmail.com (mailing list archive)
State Superseded
Headers show
Series Renesas Stout board support (R-Car Gen2) | expand

Commit Message

Oleksandr Tyshchenko April 8, 2019, 10:14 a.m. UTC 
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Extend driver to be able to handle other SCIF(X) compatible
interfaces as well. These interfaces have lot in common,
but mostly differ in offsets and bits for some registers.

Introduce "port_params" array to keep interface specific things.

The "data" field in struct dt_device_match is used for recognizing
what interface is present on a target board.

Please note, nothing has been technically changed for Renesas "Lager"
and other supported boards (SCIF).

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
CC: Julien Grall <julien.grall@arm.com>

---
    Changes in v2:
        - Name a enum for describing interfaces this driver supports
        - Use local variable for "params" where appropriate
        - Use "data" field in struct dt_device_match instead of calling
          dt_device_is_compatible()
        - Don't check for "overrun_reg != status_reg" condition during
          initialization

    Changes in v3:
        - This patch is a result of splitting an initial patch
          "xen/arm: drivers: scif: Add support for SCIFA compatible UARTs"
          and only reworks a driver
        - Drop "port_type" variable from scif_uart_init(), pass a pointer
          directly
---
 xen/drivers/char/scif-uart.c    | 119 ++++++++++++++++++++++++++++------------
 xen/include/asm-arm/scif-uart.h |   4 --
 2 files changed, 85 insertions(+), 38 deletions(-)

Comments

Julien Grall April 14, 2019, 4:55 p.m. UTC | #1 
Hi Oleksandr,

On 4/8/19 11:14 AM, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> Extend driver to be able to handle other SCIF(X) compatible
> interfaces as well. These interfaces have lot in common,
> but mostly differ in offsets and bits for some registers.

Can you briefly explain in the commit message what differs?

[...]

> @@ -65,13 +100,16 @@ static void scif_uart_interrupt(int irq, void *data, struct cpu_user_regs *regs)
>               serial_rx_interrupt(port, regs);
>   
>           /* Error Interrupt */
> -        if ( status & SCIF_ERRORS )
> -            scif_writew(uart, SCIF_SCFSR, ~SCIF_ERRORS);
> -        if ( scif_readw(uart, SCIF_SCLSR) & SCLSR_ORER )
> -            scif_writew(uart, SCIF_SCLSR, 0);
> +        if ( status & params->error_mask )
> +            scif_writew(uart, params->status_reg, ~params->error_mask);
> +        if ( params->overrun_reg != params->status_reg )

Below you will write the same register twice if overrun_reg == 
status_reg (see scif_uart_init_preirq). Would there be any issue if you 
do the same here?

Similarly, you seem to define overrun_mask for SCIFA (see next patch) 
but it will never be used as, AFAICT, status_reg == overrun_reg.

> +        {
> +            if ( scif_readw(uart, params->overrun_reg) & params->overrun_mask )
> +                scif_writew(uart, params->overrun_reg, ~params->overrun_mask); > +        }
>   
>           ctrl = scif_readw(uart, SCIF_SCSCR);
> -        status = scif_readw(uart, SCIF_SCFSR) & ~SCFSR_TEND;
> +        status = scif_readw(uart, params->status_reg) & ~SCFSR_TEND;
>           /* Ignore next flag if TX Interrupt is disabled */
>           if ( !(ctrl & SCSCR_TIE) )
>               status &= ~SCFSR_TDFE;
> @@ -81,12 +119,13 @@ static void scif_uart_interrupt(int irq, void *data, struct cpu_user_regs *regs)
>   static void __init scif_uart_init_preirq(struct serial_port *port)
>   {
>       struct scif_uart *uart = port->uart;
> +    const struct port_params *params = uart->params;
>   
>       /*
>        * Wait until last bit has been transmitted. This is needed for a smooth
>        * transition when we come from early printk
>        */
> -    while ( !(scif_readw(uart, SCIF_SCFSR) & SCFSR_TEND) );
> +    while ( !(scif_readw(uart, params->status_reg) & SCFSR_TEND) );
>   
>       /* Disable TX/RX parts and all interrupts */
>       scif_writew(uart, SCIF_SCSCR, 0);
> @@ -95,10 +134,10 @@ static void __init scif_uart_init_preirq(struct serial_port *port)
>       scif_writew(uart, SCIF_SCFCR, SCFCR_RFRST | SCFCR_TFRST);
>   
>       /* Clear all errors and flags */
> -    scif_readw(uart, SCIF_SCFSR);
> -    scif_writew(uart, SCIF_SCFSR, 0);
> -    scif_readw(uart, SCIF_SCLSR);
> -    scif_writew(uart, SCIF_SCLSR, 0);
> +    scif_readw(uart, params->status_reg);
> +    scif_writew(uart, params->status_reg, 0);
> +    scif_readw(uart, params->overrun_reg);
> +    scif_writew(uart, params->overrun_reg, 0);
>   
>       /* Setup trigger level for TX/RX FIFOs */
>       scif_writew(uart, SCIF_SCFCR, SCFCR_RTRG11 | SCFCR_TTRG11);
> @@ -111,6 +150,7 @@ static void __init scif_uart_init_preirq(struct serial_port *port)
>   static void __init scif_uart_init_postirq(struct serial_port *port)
>   {
>       struct scif_uart *uart = port->uart;
> +    const struct port_params *params = uart->params;
>       int rc;
>   
>       uart->irqaction.handler = scif_uart_interrupt;
> @@ -122,14 +162,17 @@ static void __init scif_uart_init_postirq(struct serial_port *port)
>                   uart->irq);
>   
>       /* Clear all errors */
> -    if ( scif_readw(uart, SCIF_SCFSR) & SCIF_ERRORS )
> -        scif_writew(uart, SCIF_SCFSR, ~SCIF_ERRORS);
> -    if ( scif_readw(uart, SCIF_SCLSR) & SCLSR_ORER )
> -        scif_writew(uart, SCIF_SCLSR, 0);
> +    if ( scif_readw(uart, params->status_reg) & params->error_mask )
> +        scif_writew(uart, params->status_reg, ~params->error_mask);
> +    if ( params->overrun_reg != params->status_reg )

Same question here.

[...]

> +static const struct dt_device_match scif_uart_dt_match[] __initconst =
> +{
> +    { .compatible = "renesas,scif",  .data = (void *)SCIF_PORT },
> +    { /* sentinel */ },
> +};
> +
>   static int __init scif_uart_init(struct dt_device_node *dev,
>                                    const void *data)
>   {
> +    const struct dt_device_match *match;
>       const char *config = data;
>       struct scif_uart *uart;
>       int res;
> @@ -265,10 +319,13 @@ static int __init scif_uart_init(struct dt_device_node *dev,
>           return -ENOMEM;
>       }
>   
> +    match = dt_match_node(scif_uart_dt_match, dev);

Technically dt_match_node may return NULL here. This should never be the 
case as you know it will always match.

So I would add an ASSERT(match) here.

Cheers,
Oleksandr Tyshchenko April 15, 2019, 11:30 a.m. UTC | #2 
On 14.04.19 19:55, Julien Grall wrote:
> Hi Oleksandr,

Hi Julien


>
> On 4/8/19 11:14 AM, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> Extend driver to be able to handle other SCIF(X) compatible
>> interfaces as well. These interfaces have lot in common,
>> but mostly differ in offsets and bits for some registers.
>
> Can you briefly explain in the commit message what differs?

Sure


>
> [...]
>
>> @@ -65,13 +100,16 @@ static void scif_uart_interrupt(int irq, void 
>> *data, struct cpu_user_regs *regs)
>>               serial_rx_interrupt(port, regs);
>>             /* Error Interrupt */
>> -        if ( status & SCIF_ERRORS )
>> -            scif_writew(uart, SCIF_SCFSR, ~SCIF_ERRORS);
>> -        if ( scif_readw(uart, SCIF_SCLSR) & SCLSR_ORER )
>> -            scif_writew(uart, SCIF_SCLSR, 0);
>> +        if ( status & params->error_mask )
>> +            scif_writew(uart, params->status_reg, ~params->error_mask);
>> +        if ( params->overrun_reg != params->status_reg )
>
> Below you will write the same register twice if overrun_reg == 
> status_reg (see scif_uart_init_preirq). Would there be any issue if 
> you do the same here?

I didn't expect any issue to write the same register twice during 
initialization to simplify the code, that why I agreed to remove the 
check in V1.

But I am not sure about doing so here. We could simplify the code by 
just removing "if ( params->overrun_reg != params->status_reg )",

but we would need to do extra operation for SCIFA which has overrun_reg 
== status_reg.

What way do you prefer?


>
> Similarly, you seem to define overrun_mask for SCIFA (see next patch) 
> but it will never be used as, AFAICT, status_reg == overrun_reg.
>> +        {
>> +            if ( scif_readw(uart, params->overrun_reg) & 
>> params->overrun_mask )
>> +                scif_writew(uart, params->overrun_reg, 
>> ~params->overrun_mask); > +        }
>>             ctrl = scif_readw(uart, SCIF_SCSCR);
>> -        status = scif_readw(uart, SCIF_SCFSR) & ~SCFSR_TEND;
>> +        status = scif_readw(uart, params->status_reg) & ~SCFSR_TEND;
>>           /* Ignore next flag if TX Interrupt is disabled */
>>           if ( !(ctrl & SCSCR_TIE) )
>>               status &= ~SCFSR_TDFE;
>> @@ -81,12 +119,13 @@ static void scif_uart_interrupt(int irq, void 
>> *data, struct cpu_user_regs *regs)
>>   static void __init scif_uart_init_preirq(struct serial_port *port)
>>   {
>>       struct scif_uart *uart = port->uart;
>> +    const struct port_params *params = uart->params;
>>         /*
>>        * Wait until last bit has been transmitted. This is needed for 
>> a smooth
>>        * transition when we come from early printk
>>        */
>> -    while ( !(scif_readw(uart, SCIF_SCFSR) & SCFSR_TEND) );
>> +    while ( !(scif_readw(uart, params->status_reg) & SCFSR_TEND) );
>>         /* Disable TX/RX parts and all interrupts */
>>       scif_writew(uart, SCIF_SCSCR, 0);
>> @@ -95,10 +134,10 @@ static void __init scif_uart_init_preirq(struct 
>> serial_port *port)
>>       scif_writew(uart, SCIF_SCFCR, SCFCR_RFRST | SCFCR_TFRST);
>>         /* Clear all errors and flags */
>> -    scif_readw(uart, SCIF_SCFSR);
>> -    scif_writew(uart, SCIF_SCFSR, 0);
>> -    scif_readw(uart, SCIF_SCLSR);
>> -    scif_writew(uart, SCIF_SCLSR, 0);
>> +    scif_readw(uart, params->status_reg);
>> +    scif_writew(uart, params->status_reg, 0);
>> +    scif_readw(uart, params->overrun_reg);
>> +    scif_writew(uart, params->overrun_reg, 0);
>>         /* Setup trigger level for TX/RX FIFOs */
>>       scif_writew(uart, SCIF_SCFCR, SCFCR_RTRG11 | SCFCR_TTRG11);
>> @@ -111,6 +150,7 @@ static void __init scif_uart_init_preirq(struct 
>> serial_port *port)
>>   static void __init scif_uart_init_postirq(struct serial_port *port)
>>   {
>>       struct scif_uart *uart = port->uart;
>> +    const struct port_params *params = uart->params;
>>       int rc;
>>         uart->irqaction.handler = scif_uart_interrupt;
>> @@ -122,14 +162,17 @@ static void __init 
>> scif_uart_init_postirq(struct serial_port *port)
>>                   uart->irq);
>>         /* Clear all errors */
>> -    if ( scif_readw(uart, SCIF_SCFSR) & SCIF_ERRORS )
>> -        scif_writew(uart, SCIF_SCFSR, ~SCIF_ERRORS);
>> -    if ( scif_readw(uart, SCIF_SCLSR) & SCLSR_ORER )
>> -        scif_writew(uart, SCIF_SCLSR, 0);
>> +    if ( scif_readw(uart, params->status_reg) & params->error_mask )
>> +        scif_writew(uart, params->status_reg, ~params->error_mask);
>> +    if ( params->overrun_reg != params->status_reg )
>
> Same question here.
>
> [...]

Please see the answer above.


>
>> +static const struct dt_device_match scif_uart_dt_match[] __initconst =
>> +{
>> +    { .compatible = "renesas,scif",  .data = (void *)SCIF_PORT },
>> +    { /* sentinel */ },
>> +};
>> +
>>   static int __init scif_uart_init(struct dt_device_node *dev,
>>                                    const void *data)
>>   {
>> +    const struct dt_device_match *match;
>>       const char *config = data;
>>       struct scif_uart *uart;
>>       int res;
>> @@ -265,10 +319,13 @@ static int __init scif_uart_init(struct 
>> dt_device_node *dev,
>>           return -ENOMEM;
>>       }
>>   +    match = dt_match_node(scif_uart_dt_match, dev);
>
> Technically dt_match_node may return NULL here. This should never be 
> the case as you know it will always match.
>
> So I would add an ASSERT(match) here.

OK


>
> Cheers,
>
Julien Grall April 16, 2019, 9:11 a.m. UTC | #3 
Hi Oleksandr,

On 4/15/19 12:30 PM, Oleksandr wrote:
> 
> On 14.04.19 19:55, Julien Grall wrote:
>> Hi Oleksandr,
> 
> Hi Julien
> 
> 
>>
>> On 4/8/19 11:14 AM, Oleksandr Tyshchenko wrote:
>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>
>>> Extend driver to be able to handle other SCIF(X) compatible
>>> interfaces as well. These interfaces have lot in common,
>>> but mostly differ in offsets and bits for some registers.
>>
>> Can you briefly explain in the commit message what differs?
> 
> Sure
> 
> 
>>
>> [...]
>>
>>> @@ -65,13 +100,16 @@ static void scif_uart_interrupt(int irq, void 
>>> *data, struct cpu_user_regs *regs)
>>>               serial_rx_interrupt(port, regs);
>>>             /* Error Interrupt */
>>> -        if ( status & SCIF_ERRORS )
>>> -            scif_writew(uart, SCIF_SCFSR, ~SCIF_ERRORS);
>>> -        if ( scif_readw(uart, SCIF_SCLSR) & SCLSR_ORER )
>>> -            scif_writew(uart, SCIF_SCLSR, 0);
>>> +        if ( status & params->error_mask )
>>> +            scif_writew(uart, params->status_reg, ~params->error_mask);
>>> +        if ( params->overrun_reg != params->status_reg )
>>
>> Below you will write the same register twice if overrun_reg == 
>> status_reg (see scif_uart_init_preirq). Would there be any issue if 
>> you do the same here?
> 
> I didn't expect any issue to write the same register twice during 
> initialization to simplify the code, that why I agreed to remove the 
> check in V1.
> 
> But I am not sure about doing so here. We could simplify the code by 
> just removing "if ( params->overrun_reg != params->status_reg )",
> 
> but we would need to do extra operation for SCIFA which has overrun_reg 
> == status_reg.
> 
> What way do you prefer?

It is not about preference, it is about correctness. In my previous 
reply, I pointed out that you define overrun_mask for SCIFA but it will 
never be used. Without any explanation, the code looks pretty wrong.

Looking at the next patch, you get away from any problem with SCIFA 
because the overrun_mask is a subset of error_mask. But I still don't 
see why trying to prevent to write a second time is an issue, the more 
the Linux driver does not seem to do this dance.

So at best, you need to explain in the commit message why you try to 
skip the register when it is the same.

Cheers,
Oleksandr Tyshchenko April 16, 2019, 1:26 p.m. UTC | #4 
On 16.04.19 12:11, Julien Grall wrote:
> Hi Oleksandr,

Hi Julien


>
> On 4/15/19 12:30 PM, Oleksandr wrote:
>>
>> On 14.04.19 19:55, Julien Grall wrote:
>>> Hi Oleksandr,
>>
>> Hi Julien
>>
>>
>>>
>>> On 4/8/19 11:14 AM, Oleksandr Tyshchenko wrote:
>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>
>>>> Extend driver to be able to handle other SCIF(X) compatible
>>>> interfaces as well. These interfaces have lot in common,
>>>> but mostly differ in offsets and bits for some registers.
>>>
>>> Can you briefly explain in the commit message what differs?
>>
>> Sure
>>
>>
>>>
>>> [...]
>>>
>>>> @@ -65,13 +100,16 @@ static void scif_uart_interrupt(int irq, void 
>>>> *data, struct cpu_user_regs *regs)
>>>>               serial_rx_interrupt(port, regs);
>>>>             /* Error Interrupt */
>>>> -        if ( status & SCIF_ERRORS )
>>>> -            scif_writew(uart, SCIF_SCFSR, ~SCIF_ERRORS);
>>>> -        if ( scif_readw(uart, SCIF_SCLSR) & SCLSR_ORER )
>>>> -            scif_writew(uart, SCIF_SCLSR, 0);
>>>> +        if ( status & params->error_mask )
>>>> +            scif_writew(uart, params->status_reg, 
>>>> ~params->error_mask);
>>>> +        if ( params->overrun_reg != params->status_reg )
>>>
>>> Below you will write the same register twice if overrun_reg == 
>>> status_reg (see scif_uart_init_preirq). Would there be any issue if 
>>> you do the same here?
>>
>> I didn't expect any issue to write the same register twice during 
>> initialization to simplify the code, that why I agreed to remove the 
>> check in V1.
>>
>> But I am not sure about doing so here. We could simplify the code by 
>> just removing "if ( params->overrun_reg != params->status_reg )",
>>
>> but we would need to do extra operation for SCIFA which has 
>> overrun_reg == status_reg.
>>
>> What way do you prefer?
>
> It is not about preference, it is about correctness. In my previous 
> reply, I pointed out that you define overrun_mask for SCIFA but it 
> will never be used. Without any explanation, the code looks pretty wrong.
>
> Looking at the next patch, you get away from any problem with SCIFA 
> because the overrun_mask is a subset of error_mask. But I still don't 
> see why trying to prevent to write a second time is an issue, the more 
> the Linux driver does not seem to do this dance.
>
> So at best, you need to explain in the commit message why you try to 
> skip the register when it is the same.

I got your point.


I can make the following changes:

1. Drop "if ( params->overrun_reg != params->status_reg )" check everywhere.

2. Remove overrun_bit (SCASSR_ORER) from error_mask for SCIFA.

This way we will get a much cleaner code where overrun_mask is used for 
both interfaces.

What do you think?


>
> Cheers,
>
diff mbox series

Patch

diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
index 465fb34..958f717 100644
--- a/xen/drivers/char/scif-uart.c
+++ b/xen/drivers/char/scif-uart.c
@@ -40,16 +40,51 @@  static struct scif_uart {
     char __iomem *regs;
     struct irqaction irqaction;
     struct vuart_info vuart;
+    const struct port_params *params;
 } scif_com = {0};
 
+enum port_types
+{
+    SCIF_PORT,
+    NR_PORTS,
+};
+
+struct port_params
+{
+    unsigned int status_reg;
+    unsigned int tx_fifo_reg;
+    unsigned int rx_fifo_reg;
+    unsigned int overrun_reg;
+    unsigned int overrun_mask;
+    unsigned int error_mask;
+    unsigned int irq_flags;
+    unsigned int fifo_size;
+};
+
+static const struct port_params port_params[NR_PORTS] =
+{
+    [SCIF_PORT] =
+    {
+        .status_reg   = SCIF_SCFSR,
+        .tx_fifo_reg  = SCIF_SCFTDR,
+        .rx_fifo_reg  = SCIF_SCFRDR,
+        .overrun_reg  = SCIF_SCLSR,
+        .overrun_mask = SCLSR_ORER,
+        .error_mask   = SCFSR_PER | SCFSR_FER | SCFSR_BRK | SCFSR_ER,
+        .irq_flags    = SCSCR_RIE | SCSCR_TIE | SCSCR_REIE,
+        .fifo_size    = 16,
+    },
+};
+
 static void scif_uart_interrupt(int irq, void *data, struct cpu_user_regs *regs)
 {
     struct serial_port *port = data;
     struct scif_uart *uart = port->uart;
+    const struct port_params *params = uart->params;
     uint16_t status, ctrl;
 
     ctrl = scif_readw(uart, SCIF_SCSCR);
-    status = scif_readw(uart, SCIF_SCFSR) & ~SCFSR_TEND;
+    status = scif_readw(uart, params->status_reg) & ~SCFSR_TEND;
     /* Ignore next flag if TX Interrupt is disabled */
     if ( !(ctrl & SCSCR_TIE) )
         status &= ~SCFSR_TDFE;
@@ -65,13 +100,16 @@  static void scif_uart_interrupt(int irq, void *data, struct cpu_user_regs *regs)
             serial_rx_interrupt(port, regs);
 
         /* Error Interrupt */
-        if ( status & SCIF_ERRORS )
-            scif_writew(uart, SCIF_SCFSR, ~SCIF_ERRORS);
-        if ( scif_readw(uart, SCIF_SCLSR) & SCLSR_ORER )
-            scif_writew(uart, SCIF_SCLSR, 0);
+        if ( status & params->error_mask )
+            scif_writew(uart, params->status_reg, ~params->error_mask);
+        if ( params->overrun_reg != params->status_reg )
+        {
+            if ( scif_readw(uart, params->overrun_reg) & params->overrun_mask )
+                scif_writew(uart, params->overrun_reg, ~params->overrun_mask);
+        }
 
         ctrl = scif_readw(uart, SCIF_SCSCR);
-        status = scif_readw(uart, SCIF_SCFSR) & ~SCFSR_TEND;
+        status = scif_readw(uart, params->status_reg) & ~SCFSR_TEND;
         /* Ignore next flag if TX Interrupt is disabled */
         if ( !(ctrl & SCSCR_TIE) )
             status &= ~SCFSR_TDFE;
@@ -81,12 +119,13 @@  static void scif_uart_interrupt(int irq, void *data, struct cpu_user_regs *regs)
 static void __init scif_uart_init_preirq(struct serial_port *port)
 {
     struct scif_uart *uart = port->uart;
+    const struct port_params *params = uart->params;
 
     /*
      * Wait until last bit has been transmitted. This is needed for a smooth
      * transition when we come from early printk
      */
-    while ( !(scif_readw(uart, SCIF_SCFSR) & SCFSR_TEND) );
+    while ( !(scif_readw(uart, params->status_reg) & SCFSR_TEND) );
 
     /* Disable TX/RX parts and all interrupts */
     scif_writew(uart, SCIF_SCSCR, 0);
@@ -95,10 +134,10 @@  static void __init scif_uart_init_preirq(struct serial_port *port)
     scif_writew(uart, SCIF_SCFCR, SCFCR_RFRST | SCFCR_TFRST);
 
     /* Clear all errors and flags */
-    scif_readw(uart, SCIF_SCFSR);
-    scif_writew(uart, SCIF_SCFSR, 0);
-    scif_readw(uart, SCIF_SCLSR);
-    scif_writew(uart, SCIF_SCLSR, 0);
+    scif_readw(uart, params->status_reg);
+    scif_writew(uart, params->status_reg, 0);
+    scif_readw(uart, params->overrun_reg);
+    scif_writew(uart, params->overrun_reg, 0);
 
     /* Setup trigger level for TX/RX FIFOs */
     scif_writew(uart, SCIF_SCFCR, SCFCR_RTRG11 | SCFCR_TTRG11);
@@ -111,6 +150,7 @@  static void __init scif_uart_init_preirq(struct serial_port *port)
 static void __init scif_uart_init_postirq(struct serial_port *port)
 {
     struct scif_uart *uart = port->uart;
+    const struct port_params *params = uart->params;
     int rc;
 
     uart->irqaction.handler = scif_uart_interrupt;
@@ -122,14 +162,17 @@  static void __init scif_uart_init_postirq(struct serial_port *port)
                 uart->irq);
 
     /* Clear all errors */
-    if ( scif_readw(uart, SCIF_SCFSR) & SCIF_ERRORS )
-        scif_writew(uart, SCIF_SCFSR, ~SCIF_ERRORS);
-    if ( scif_readw(uart, SCIF_SCLSR) & SCLSR_ORER )
-        scif_writew(uart, SCIF_SCLSR, 0);
+    if ( scif_readw(uart, params->status_reg) & params->error_mask )
+        scif_writew(uart, params->status_reg, ~params->error_mask);
+    if ( params->overrun_reg != params->status_reg )
+    {
+        if ( scif_readw(uart, params->overrun_reg) & params->overrun_mask )
+            scif_writew(uart, params->overrun_reg, ~params->overrun_mask);
+    }
 
     /* Enable TX/RX and Error Interrupts  */
     scif_writew(uart, SCIF_SCSCR, scif_readw(uart, SCIF_SCSCR) |
-                 SCSCR_TIE | SCSCR_RIE | SCSCR_REIE);
+                params->irq_flags);
 }
 
 static void scif_uart_suspend(struct serial_port *port)
@@ -145,43 +188,47 @@  static void scif_uart_resume(struct serial_port *port)
 static int scif_uart_tx_ready(struct serial_port *port)
 {
     struct scif_uart *uart = port->uart;
+    const struct port_params *params = uart->params;
     uint16_t cnt;
 
     /* Check for empty space in TX FIFO */
-    if ( !(scif_readw(uart, SCIF_SCFSR) & SCFSR_TDFE) )
+    if ( !(scif_readw(uart, params->status_reg) & SCFSR_TDFE) )
         return 0;
 
      /* Check number of data bytes stored in TX FIFO */
     cnt = scif_readw(uart, SCIF_SCFDR) >> 8;
-    ASSERT( cnt >= 0 && cnt <= SCIF_FIFO_MAX_SIZE );
+    ASSERT( cnt >= 0 && cnt <= params->fifo_size );
 
-    return (SCIF_FIFO_MAX_SIZE - cnt);
+    return (params->fifo_size - cnt);
 }
 
 static void scif_uart_putc(struct serial_port *port, char c)
 {
     struct scif_uart *uart = port->uart;
+    const struct port_params *params = uart->params;
 
-    scif_writeb(uart, SCIF_SCFTDR, c);
+    scif_writeb(uart, params->tx_fifo_reg, c);
     /* Clear required TX flags */
-    scif_writew(uart, SCIF_SCFSR, scif_readw(uart, SCIF_SCFSR) &
-                 ~(SCFSR_TEND | SCFSR_TDFE));
+    scif_writew(uart, params->status_reg,
+                scif_readw(uart, params->status_reg) &
+                ~(SCFSR_TEND | SCFSR_TDFE));
 }
 
 static int scif_uart_getc(struct serial_port *port, char *pc)
 {
     struct scif_uart *uart = port->uart;
+    const struct port_params *params = uart->params;
 
     /* Check for available data bytes in RX FIFO */
-    if ( !(scif_readw(uart, SCIF_SCFSR) & (SCFSR_RDF | SCFSR_DR)) )
+    if ( !(scif_readw(uart, params->status_reg) & (SCFSR_RDF | SCFSR_DR)) )
         return 0;
 
-    *pc = scif_readb(uart, SCIF_SCFRDR);
+    *pc = scif_readb(uart, params->rx_fifo_reg);
 
     /* dummy read */
-    scif_readw(uart, SCIF_SCFSR);
+    scif_readw(uart, params->status_reg);
     /* Clear required RX flags */
-    scif_writew(uart, SCIF_SCFSR, ~(SCFSR_RDF | SCFSR_DR));
+    scif_writew(uart, params->status_reg, ~(SCFSR_RDF | SCFSR_DR));
 
     return 1;
 }
@@ -229,9 +276,16 @@  static struct uart_driver __read_mostly scif_uart_driver = {
     .vuart_info   = scif_vuart_info,
 };
 
+static const struct dt_device_match scif_uart_dt_match[] __initconst =
+{
+    { .compatible = "renesas,scif",  .data = (void *)SCIF_PORT },
+    { /* sentinel */ },
+};
+
 static int __init scif_uart_init(struct dt_device_node *dev,
                                  const void *data)
 {
+    const struct dt_device_match *match;
     const char *config = data;
     struct scif_uart *uart;
     int res;
@@ -265,10 +319,13 @@  static int __init scif_uart_init(struct dt_device_node *dev,
         return -ENOMEM;
     }
 
+    match = dt_match_node(scif_uart_dt_match, dev);
+    uart->params = &port_params[(enum port_types)match->data];
+
     uart->vuart.base_addr  = addr;
     uart->vuart.size       = size;
-    uart->vuart.data_off   = SCIF_SCFTDR;
-    uart->vuart.status_off = SCIF_SCFSR;
+    uart->vuart.data_off   = uart->params->tx_fifo_reg;
+    uart->vuart.status_off = uart->params->status_reg;
     uart->vuart.status     = SCFSR_TDFE;
 
     /* Register with generic serial driver */
@@ -279,12 +336,6 @@  static int __init scif_uart_init(struct dt_device_node *dev,
     return 0;
 }
 
-static const struct dt_device_match scif_uart_dt_match[] __initconst =
-{
-    DT_MATCH_COMPATIBLE("renesas,scif"),
-    { /* sentinel */ },
-};
-
 DT_DEVICE_START(scif_uart, "SCIF UART", DEVICE_SERIAL)
     .dt_match = scif_uart_dt_match,
     .init = scif_uart_init,
diff --git a/xen/include/asm-arm/scif-uart.h b/xen/include/asm-arm/scif-uart.h
index 8137850..c343f2f 100644
--- a/xen/include/asm-arm/scif-uart.h
+++ b/xen/include/asm-arm/scif-uart.h
@@ -21,8 +21,6 @@ 
 #ifndef __ASM_ARM_SCIF_UART_H
 #define __ASM_ARM_SCIF_UART_H
 
-#define SCIF_FIFO_MAX_SIZE    16
-
 /* Register offsets */
 #define SCIF_SCSMR     (0x00)    /* Serial mode register           */
 #define SCIF_SCBRR     (0x04)    /* Bit rate register              */
@@ -57,8 +55,6 @@ 
 #define SCFSR_RDF     (1 << 1)    /* Receive FIFO Data Full */
 #define SCFSR_DR      (1 << 0)    /* Receive Data Ready */
 
-#define SCIF_ERRORS    (SCFSR_PER | SCFSR_FER | SCFSR_ER | SCFSR_BRK)
-
 /* Line Status Register (SCLSR) */
 #define SCLSR_TO      (1 << 2)    /* Timeout */
 #define SCLSR_ORER    (1 << 0)    /* Overrun Error */