[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v1 2/4] xen/arm: drivers: scif: Add support for SCIFA compatible UARTs



Hi Oleksandr,

On 06/08/18 19:35, Oleksandr Tyshchenko wrote:
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>

Extend existing driver to be able to handle SCIFA interface as well.
SCIF and SCIFA have lot in common, though SCIFA has different
offsets and bits for some registers.

Use compatible string to recognize what interface is present
on a target board.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
CC: Julien Grall <julien.grall@xxxxxxx>
---
  xen/drivers/char/scif-uart.c    | 129 +++++++++++++++++++++++++++++++---------
  xen/include/asm-arm/scif-uart.h |  44 ++++++++++++--
  2 files changed, 138 insertions(+), 35 deletions(-)

diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
index 465fb34..d1eb713 100644
--- a/xen/drivers/char/scif-uart.c
+++ b/xen/drivers/char/scif-uart.c
@@ -1,7 +1,7 @@
  /*
   * xen/drivers/char/scif-uart.c
   *
- * Driver for SCIF (Serial communication interface with FIFO)
+ * Driver for SCIF(A) (Serial communication interface with FIFO (A))
   * compatible UART.
   *
   * Oleksandr Tyshchenko <oleksandr.tyshchenko@xxxxxxxxxxxxxxx>
@@ -40,8 +40,57 @@ static struct scif_uart {
      char __iomem *regs;
      struct irqaction irqaction;
      struct vuart_info vuart;
+    const struct port_params *params;
  } scif_com = {0};
+enum

Should not this enum has a name?

+{
+    SCIF_PORT,
+    SCIFA_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,
+    },
+
+    [SCIFA_PORT] =
+    {
+        .status_reg   = SCIFA_SCASSR,
+        .tx_fifo_reg  = SCIFA_SCAFTDR,
+        .rx_fifo_reg  = SCIFA_SCAFRDR,
+        .overrun_reg  = SCIFA_SCASSR,
+        .overrun_mask = SCASSR_ORER,
+        .error_mask   = SCASSR_PER | SCASSR_FER | SCASSR_BRK | SCASSR_ER |
+                        SCASSR_ORER,
+        .irq_flags    = SCASCR_RIE | SCASCR_TIE | SCASCR_DRIE | SCASCR_ERIE |
+                        SCASCR_BRIE,
+        .fifo_size    = 64,
+    },
+};
+
  static void scif_uart_interrupt(int irq, void *data, struct cpu_user_regs 
*regs)
  {
      struct serial_port *port = data;
@@ -49,7 +98,7 @@ static void scif_uart_interrupt(int irq, void *data, struct 
cpu_user_regs *regs)
      uint16_t status, ctrl;
ctrl = scif_readw(uart, SCIF_SCSCR);
-    status = scif_readw(uart, SCIF_SCFSR) & ~SCFSR_TEND;
+    status = scif_readw(uart, uart->params->status_reg) & ~SCFSR_TEND;

I would prefer if you introduce a local variable for the params. This would avoid to write uart->params everywhere.

      /* Ignore next flag if TX Interrupt is disabled */
      if ( !(ctrl & SCSCR_TIE) )
          status &= ~SCFSR_TDFE;
@@ -65,13 +114,19 @@ 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 & uart->params->error_mask )
+            scif_writew(uart, uart->params->status_reg,
+                        ~uart->params->error_mask);
+        if ( uart->params->overrun_reg != uart->params->status_reg )
+        {
+            if ( scif_readw(uart, uart->params->overrun_reg) &
+                 uart->params->overrun_mask )
+                scif_writew(uart, uart->params->overrun_reg,
+                            ~uart->params->overrun_mask);
+        }
ctrl = scif_readw(uart, SCIF_SCSCR);
-        status = scif_readw(uart, SCIF_SCFSR) & ~SCFSR_TEND;
+        status = scif_readw(uart, uart->params->status_reg) & ~SCFSR_TEND;
          /* Ignore next flag if TX Interrupt is disabled */
          if ( !(ctrl & SCSCR_TIE) )
              status &= ~SCFSR_TDFE;
@@ -86,7 +141,7 @@ static void __init scif_uart_init_preirq(struct serial_port 
*port)
       * 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, uart->params->status_reg) & SCFSR_TEND) );
/* Disable TX/RX parts and all interrupts */
      scif_writew(uart, SCIF_SCSCR, 0);
@@ -95,10 +150,13 @@ 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, uart->params->status_reg);
+    scif_writew(uart, uart->params->status_reg, 0);
+    if ( uart->params->overrun_reg != uart->params->status_reg )

I don't think there is any harm to reset the registers twice when overrun_reg != status_reg.

+    {
+        scif_readw(uart, uart->params->overrun_reg);
+        scif_writew(uart, uart->params->overrun_reg, 0);
+    }
/* Setup trigger level for TX/RX FIFOs */
      scif_writew(uart, SCIF_SCFCR, SCFCR_RTRG11 | SCFCR_TTRG11);
@@ -122,14 +180,19 @@ 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, uart->params->status_reg) & uart->params->error_mask 
)
+        scif_writew(uart, uart->params->status_reg, ~uart->params->error_mask);
+    if ( uart->params->overrun_reg != uart->params->status_reg )
+    {
+        if ( scif_readw(uart, uart->params->overrun_reg) &
+             uart->params->overrun_mask )
+            scif_writew(uart, uart->params->overrun_reg,
+                        ~uart->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);
+                uart->params->irq_flags);

The indentation looks wrong here.

  }
static void scif_uart_suspend(struct serial_port *port)
@@ -148,24 +211,25 @@ static int scif_uart_tx_ready(struct serial_port *port)
      uint16_t cnt;
/* Check for empty space in TX FIFO */
-    if ( !(scif_readw(uart, SCIF_SCFSR) & SCFSR_TDFE) )
+    if ( !(scif_readw(uart, 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 <= uart->params->fifo_size );
- return (SCIF_FIFO_MAX_SIZE - cnt);
+    return (uart->params->fifo_size - cnt);
  }
static void scif_uart_putc(struct serial_port *port, char c)
  {
      struct scif_uart *uart = port->uart;
- scif_writeb(uart, SCIF_SCFTDR, c);
+    scif_writeb(uart, 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, uart->params->status_reg,
+                scif_readw(uart, uart->params->status_reg) &
+                ~(SCFSR_TEND | SCFSR_TDFE));
  }
static int scif_uart_getc(struct serial_port *port, char *pc)
@@ -173,15 +237,16 @@ static int scif_uart_getc(struct serial_port *port, char 
*pc)
      struct scif_uart *uart = port->uart;
/* Check for available data bytes in RX FIFO */
-    if ( !(scif_readw(uart, SCIF_SCFSR) & (SCFSR_RDF | SCFSR_DR)) )
+    if ( !(scif_readw(uart, uart->params->status_reg) &
+          (SCFSR_RDF | SCFSR_DR)) )
          return 0;
- *pc = scif_readb(uart, SCIF_SCFRDR);
+    *pc = scif_readb(uart, uart->params->rx_fifo_reg);
/* dummy read */
-    scif_readw(uart, SCIF_SCFSR);
+    scif_readw(uart, uart->params->status_reg);
      /* Clear required RX flags */
-    scif_writew(uart, SCIF_SCFSR, ~(SCFSR_RDF | SCFSR_DR));
+    scif_writew(uart, uart->params->status_reg, ~(SCFSR_RDF | SCFSR_DR));
return 1;
  }
@@ -265,10 +330,15 @@ static int __init scif_uart_init(struct dt_device_node 
*dev,
          return -ENOMEM;
      }
+ if ( dt_device_is_compatible(dev, "renesas,scifa") )
+        uart->params = &port_params[SCIFA_PORT];
+    else
+        uart->params = &port_params[SCIF_PORT];

You can use the field .data in dt_device_match to store specific data and then use dt_match_node to find the correct node.

+
      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 */
@@ -282,6 +352,7 @@ static int __init scif_uart_init(struct dt_device_node *dev,
  static const struct dt_device_match scif_uart_dt_match[] __initconst =
  {
      DT_MATCH_COMPATIBLE("renesas,scif"),
+    DT_MATCH_COMPATIBLE("renesas,scifa"),
      { /* sentinel */ },
  };
diff --git a/xen/include/asm-arm/scif-uart.h b/xen/include/asm-arm/scif-uart.h
index 8137850..bce3404 100644
--- a/xen/include/asm-arm/scif-uart.h
+++ b/xen/include/asm-arm/scif-uart.h
@@ -2,7 +2,7 @@
   * xen/include/asm-arm/scif-uart.h
   *
   * Common constant definition between early printk and the UART driver
- * for the SCIF compatible UART.
+ * for the SCIF(A) compatible UART.
   *
   * Oleksandr Tyshchenko <oleksandr.tyshchenko@xxxxxxxxxxxxxxx>
   * Copyright (C) 2014, Globallogic.
@@ -21,9 +21,7 @@
  #ifndef __ASM_ARM_SCIF_UART_H
  #define __ASM_ARM_SCIF_UART_H
-#define SCIF_FIFO_MAX_SIZE 16
-
-/* Register offsets */
+/* Register offsets (SCIF) */
  #define SCIF_SCSMR     (0x00)    /* Serial mode register           */
  #define SCIF_SCBRR     (0x04)    /* Bit rate register              */
  #define SCIF_SCSCR     (0x08)    /* Serial control 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 */
@@ -83,6 +79,42 @@
  #define SCFCR_TTRG10    (SCFCR_TTRG1)
  #define SCFCR_TTRG11    (SCFCR_TTRG1 | SCFCR_TTRG0)
+/* Register offsets (SCIFA) */
+#define SCIFA_SCASMR     (0x00)    /* Serial mode register           */
+#define SCIFA_SCABRR     (0x04)    /* Bit rate register              */
+#define SCIFA_SCASCR     (0x08)    /* Serial control register        */
+#define SCIFA_SCATDSR    (0x0C)    /* Transmit data stop register    */
+#define SCIFA_SCAFER     (0x10)    /* FIFO error count register      */
+#define SCIFA_SCASSR     (0x14)    /* Serial status register         */
+#define SCIFA_SCAFCR     (0x18)    /* FIFO control register          */
+#define SCIFA_SCAFDR     (0x1C)    /* FIFO data count register       */
+#define SCIFA_SCAFTDR    (0x20)    /* Transmit FIFO data register    */
+#define SCIFA_SCAFRDR    (0x24)    /* Receive FIFO data register     */
+#define SCIFA_SCAPCR     (0x30)    /* Serial port control register   */
+#define SCIFA_SCAPDR     (0x34)    /* Serial port data register      */
+
+/* Serial Control Register (SCASCR) */
+#define SCASCR_ERIE     (1 << 10)    /* Receive Error Interrupt Enable */
+#define SCASCR_BRIE     (1 << 9)     /* Break Interrupt Enable */
+#define SCASCR_DRIE     (1 << 8)     /* Receive Data Ready Interrupt Enable */
+#define SCASCR_TIE      (1 << 7)     /* Transmit Interrupt Enable */
+#define SCASCR_RIE      (1 << 6)     /* Receive Interrupt Enable */
+#define SCASCR_TE       (1 << 5)     /* Transmit Enable */
+#define SCASCR_RE       (1 << 4)     /* Receive Enable */
+#define SCASCR_CKE0     (1 << 0)     /* Clock Enable 0 */
+
+/* Serial Status Register (SCASSR) */
+#define SCASSR_ORER    (1 << 9)    /* Overrun Error */
+#define SCASSR_TSF     (1 << 8)    /* Transmit Data Stop */
+#define SCASSR_ER      (1 << 7)    /* Receive Error */
+#define SCASSR_TEND    (1 << 6)    /* Transmission End */
+#define SCASSR_TDFE    (1 << 5)    /* Transmit FIFO Data Empty */
+#define SCASSR_BRK     (1 << 4)    /* Break Detect */
+#define SCASSR_FER     (1 << 3)    /* Framing Error */
+#define SCASSR_PER     (1 << 2)    /* Parity Error */
+#define SCASSR_RDF     (1 << 1)    /* Receive FIFO Data Full */
+#define SCASSR_DR      (1 << 0)    /* Receive Data Ready */
+
  #endif /* __ASM_ARM_SCIF_UART_H */
/*


Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.