All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V3 0/5] Renesas Stout board support (R-Car Gen2)
@ 2019-04-08 10:14 ` Oleksandr Tyshchenko
  0 siblings, 0 replies; 32+ messages in thread
From: Oleksandr Tyshchenko @ 2019-04-08 10:14 UTC (permalink / raw)
  To: xen-devel; +Cc: Oleksandr Tyshchenko, julien.grall, sstabellini

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Hi, all.

The purpose of this patch series is to add required support to be able to run
Xen on Renesas Stout board [1] which uses SCIFA compatible UART as a console
interface.

Actually Xen already has support for SCIF compatible UARTs which are used on
Renesas Lager (R-Car Gen2), Salvator-X, H3ULCB/M3ULCB (R-Car Gen3) and other
development boards. So this patch series extends existing support to be able
to handle both interfaces.

----------

Current patch series is based on the following commit 1c6504163595d45e47a01750318c2b7b50541cbe
and tested on Stout (ARM32) and H3ULCB (ARM64) boards.

You can find current patch series here:
repo: https://github.com/otyshchenko1/xen.git branch: stout_upstream1

You can find previous discussions here:
[V1] https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg21058.html
[V2] https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg37518.html

----------

In order to run Xen on Stout board you need "PSCI-enabled" U-Boot (not upsteamed yet).
You can find corresponding patches for U-Boot here:
http://u-boot.10912.n7.nabble.com/PATCH-0-3-PSCI-support-for-r8a7790-SoC-Lager-Stout-boards-td357352.html

Have a plan to update Xen Wiki regarding this board.

[1] https://elinux.org/R-Car/Boards/Stout

Oleksandr Tyshchenko (5):
  xen/arm: Clarify usage of earlyprintk for Lager board
  xen/arm: drivers: scif: Extend driver to handle other interfaces
  xen/arm: drivers: scif: Add support for SCIFA compatible UARTs
  xen/arm: Extend SCIF early prink code to handle other interfaces
  xen/arm: Add early printk support for SCIFA compatible UARTs

 docs/misc/arm/early-printk.txt    |   2 +-
 xen/arch/arm/Rules.mk             |   7 ++
 xen/arch/arm/arm32/debug-scif.inc |  22 ++++--
 xen/drivers/char/scif-uart.c      | 137 ++++++++++++++++++++++++++++----------
 xen/include/asm-arm/scif-uart.h   |  44 ++++++++++--
 5 files changed, 163 insertions(+), 49 deletions(-)

-- 
2.7.4


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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [Xen-devel] [PATCH V3 0/5] Renesas Stout board support (R-Car Gen2)
@ 2019-04-08 10:14 ` Oleksandr Tyshchenko
  0 siblings, 0 replies; 32+ messages in thread
From: Oleksandr Tyshchenko @ 2019-04-08 10:14 UTC (permalink / raw)
  To: xen-devel; +Cc: Oleksandr Tyshchenko, julien.grall, sstabellini

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Hi, all.

The purpose of this patch series is to add required support to be able to run
Xen on Renesas Stout board [1] which uses SCIFA compatible UART as a console
interface.

Actually Xen already has support for SCIF compatible UARTs which are used on
Renesas Lager (R-Car Gen2), Salvator-X, H3ULCB/M3ULCB (R-Car Gen3) and other
development boards. So this patch series extends existing support to be able
to handle both interfaces.

----------

Current patch series is based on the following commit 1c6504163595d45e47a01750318c2b7b50541cbe
and tested on Stout (ARM32) and H3ULCB (ARM64) boards.

You can find current patch series here:
repo: https://github.com/otyshchenko1/xen.git branch: stout_upstream1

You can find previous discussions here:
[V1] https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg21058.html
[V2] https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg37518.html

----------

In order to run Xen on Stout board you need "PSCI-enabled" U-Boot (not upsteamed yet).
You can find corresponding patches for U-Boot here:
http://u-boot.10912.n7.nabble.com/PATCH-0-3-PSCI-support-for-r8a7790-SoC-Lager-Stout-boards-td357352.html

Have a plan to update Xen Wiki regarding this board.

[1] https://elinux.org/R-Car/Boards/Stout

Oleksandr Tyshchenko (5):
  xen/arm: Clarify usage of earlyprintk for Lager board
  xen/arm: drivers: scif: Extend driver to handle other interfaces
  xen/arm: drivers: scif: Add support for SCIFA compatible UARTs
  xen/arm: Extend SCIF early prink code to handle other interfaces
  xen/arm: Add early printk support for SCIFA compatible UARTs

 docs/misc/arm/early-printk.txt    |   2 +-
 xen/arch/arm/Rules.mk             |   7 ++
 xen/arch/arm/arm32/debug-scif.inc |  22 ++++--
 xen/drivers/char/scif-uart.c      | 137 ++++++++++++++++++++++++++++----------
 xen/include/asm-arm/scif-uart.h   |  44 ++++++++++--
 5 files changed, 163 insertions(+), 49 deletions(-)

-- 
2.7.4


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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH V3 1/5] xen/arm: Clarify usage of earlyprintk for Lager board
@ 2019-04-08 10:14   ` Oleksandr Tyshchenko
  0 siblings, 0 replies; 32+ messages in thread
From: Oleksandr Tyshchenko @ 2019-04-08 10:14 UTC (permalink / raw)
  To: xen-devel; +Cc: Oleksandr Tyshchenko, julien.grall, sstabellini

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Current sentence is not entirely correct. Since SCIF0 interface is
applicable for Lager board, but is not applicable for all R-Car H2
based boards. For example, Stout board uses SCIFA0 interface.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Acked-by: Julien Grall <julien.grall@arm.com>
---
 docs/misc/arm/early-printk.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/misc/arm/early-printk.txt b/docs/misc/arm/early-printk.txt
index f765f59..b23c54f 100644
--- a/docs/misc/arm/early-printk.txt
+++ b/docs/misc/arm/early-printk.txt
@@ -39,7 +39,7 @@ the name of the machine:
   - fastmodel: printk on ARM Fastmodel software emulators
   - hikey960: printk with pl011 with Hikey 960
   - juno: printk with pl011 on Juno platform
-  - lager: printk with SCIF0 on Renesas R-Car H2 processors
+  - lager: printk with SCIF0 on Renesas Lager board (R-Car H2 processor)
   - midway: printk with the pl011 on Calxeda Midway processors
   - mvebu: printk with the MVEBU for Marvell Armada 3700 SoCs
   - omap5432: printk with UART3 on TI OMAP5432 processors
-- 
2.7.4


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

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [Xen-devel] [PATCH V3 1/5] xen/arm: Clarify usage of earlyprintk for Lager board
@ 2019-04-08 10:14   ` Oleksandr Tyshchenko
  0 siblings, 0 replies; 32+ messages in thread
From: Oleksandr Tyshchenko @ 2019-04-08 10:14 UTC (permalink / raw)
  To: xen-devel; +Cc: Oleksandr Tyshchenko, julien.grall, sstabellini

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Current sentence is not entirely correct. Since SCIF0 interface is
applicable for Lager board, but is not applicable for all R-Car H2
based boards. For example, Stout board uses SCIFA0 interface.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Acked-by: Julien Grall <julien.grall@arm.com>
---
 docs/misc/arm/early-printk.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/misc/arm/early-printk.txt b/docs/misc/arm/early-printk.txt
index f765f59..b23c54f 100644
--- a/docs/misc/arm/early-printk.txt
+++ b/docs/misc/arm/early-printk.txt
@@ -39,7 +39,7 @@ the name of the machine:
   - fastmodel: printk on ARM Fastmodel software emulators
   - hikey960: printk with pl011 with Hikey 960
   - juno: printk with pl011 on Juno platform
-  - lager: printk with SCIF0 on Renesas R-Car H2 processors
+  - lager: printk with SCIF0 on Renesas Lager board (R-Car H2 processor)
   - midway: printk with the pl011 on Calxeda Midway processors
   - mvebu: printk with the MVEBU for Marvell Armada 3700 SoCs
   - omap5432: printk with UART3 on TI OMAP5432 processors
-- 
2.7.4


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

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH V3 2/5] xen/arm: drivers: scif: Extend driver to handle other interfaces
@ 2019-04-08 10:14   ` Oleksandr Tyshchenko
  0 siblings, 0 replies; 32+ messages in thread
From: Oleksandr Tyshchenko @ 2019-04-08 10:14 UTC (permalink / raw)
  To: xen-devel; +Cc: Oleksandr Tyshchenko, julien.grall, sstabellini

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

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 */
-- 
2.7.4


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

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [Xen-devel] [PATCH V3 2/5] xen/arm: drivers: scif: Extend driver to handle other interfaces
@ 2019-04-08 10:14   ` Oleksandr Tyshchenko
  0 siblings, 0 replies; 32+ messages in thread
From: Oleksandr Tyshchenko @ 2019-04-08 10:14 UTC (permalink / raw)
  To: xen-devel; +Cc: Oleksandr Tyshchenko, julien.grall, sstabellini

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

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 */
-- 
2.7.4


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

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH V3 3/5] xen/arm: drivers: scif: Add support for SCIFA compatible UARTs
@ 2019-04-08 10:14   ` Oleksandr Tyshchenko
  0 siblings, 0 replies; 32+ messages in thread
From: Oleksandr Tyshchenko @ 2019-04-08 10:14 UTC (permalink / raw)
  To: xen-devel; +Cc: Oleksandr Tyshchenko, julien.grall, sstabellini

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

For the driver to be able to handle SCIFA interface as well,
this patch just adds the following:
- SCIFA related macros
- New element in "port_params" array to keep SCIFA specific things
- SCIFA compatible string

This patch makes possible to use existing driver for Renesas "Stout"
board based on R-Car H2 SoC (SCIFA).

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

---
    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 adds SCIFA support
---
 xen/drivers/char/scif-uart.c    | 18 +++++++++++++++++-
 xen/include/asm-arm/scif-uart.h | 40 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
index 958f717..94cb230 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@globallogic.com>
@@ -46,6 +46,7 @@ static struct scif_uart {
 enum port_types
 {
     SCIF_PORT,
+    SCIFA_PORT,
     NR_PORTS,
 };
 
@@ -74,6 +75,20 @@ static const struct port_params port_params[NR_PORTS] =
         .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)
@@ -279,6 +294,7 @@ static struct uart_driver __read_mostly scif_uart_driver = {
 static const struct dt_device_match scif_uart_dt_match[] __initconst =
 {
     { .compatible = "renesas,scif",  .data = (void *)SCIF_PORT },
+    { .compatible = "renesas,scifa", .data = (void *)SCIFA_PORT },
     { /* sentinel */ },
 };
 
diff --git a/xen/include/asm-arm/scif-uart.h b/xen/include/asm-arm/scif-uart.h
index c343f2f..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@globallogic.com>
  * Copyright (C) 2014, Globallogic.
@@ -21,7 +21,7 @@
 #ifndef __ASM_ARM_SCIF_UART_H
 #define __ASM_ARM_SCIF_UART_H
 
-/* 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        */
@@ -79,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 */
 
 /*
-- 
2.7.4


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

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [Xen-devel] [PATCH V3 3/5] xen/arm: drivers: scif: Add support for SCIFA compatible UARTs
@ 2019-04-08 10:14   ` Oleksandr Tyshchenko
  0 siblings, 0 replies; 32+ messages in thread
From: Oleksandr Tyshchenko @ 2019-04-08 10:14 UTC (permalink / raw)
  To: xen-devel; +Cc: Oleksandr Tyshchenko, julien.grall, sstabellini

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

For the driver to be able to handle SCIFA interface as well,
this patch just adds the following:
- SCIFA related macros
- New element in "port_params" array to keep SCIFA specific things
- SCIFA compatible string

This patch makes possible to use existing driver for Renesas "Stout"
board based on R-Car H2 SoC (SCIFA).

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

---
    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 adds SCIFA support
---
 xen/drivers/char/scif-uart.c    | 18 +++++++++++++++++-
 xen/include/asm-arm/scif-uart.h | 40 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
index 958f717..94cb230 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@globallogic.com>
@@ -46,6 +46,7 @@ static struct scif_uart {
 enum port_types
 {
     SCIF_PORT,
+    SCIFA_PORT,
     NR_PORTS,
 };
 
@@ -74,6 +75,20 @@ static const struct port_params port_params[NR_PORTS] =
         .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)
@@ -279,6 +294,7 @@ static struct uart_driver __read_mostly scif_uart_driver = {
 static const struct dt_device_match scif_uart_dt_match[] __initconst =
 {
     { .compatible = "renesas,scif",  .data = (void *)SCIF_PORT },
+    { .compatible = "renesas,scifa", .data = (void *)SCIFA_PORT },
     { /* sentinel */ },
 };
 
diff --git a/xen/include/asm-arm/scif-uart.h b/xen/include/asm-arm/scif-uart.h
index c343f2f..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@globallogic.com>
  * Copyright (C) 2014, Globallogic.
@@ -21,7 +21,7 @@
 #ifndef __ASM_ARM_SCIF_UART_H
 #define __ASM_ARM_SCIF_UART_H
 
-/* 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        */
@@ -79,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 */
 
 /*
-- 
2.7.4


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

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH V3 4/5] xen/arm: Extend SCIF early prink code to handle other interfaces
@ 2019-04-08 10:14   ` Oleksandr Tyshchenko
  0 siblings, 0 replies; 32+ messages in thread
From: Oleksandr Tyshchenko @ 2019-04-08 10:14 UTC (permalink / raw)
  To: xen-devel; +Cc: Oleksandr Tyshchenko, julien.grall, sstabellini

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Extend early prink code 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 "EARLY_PRINTK_VERSION" config option to choose which
interface version should be used (to properly apply register offsets).

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

The "EARLY_PRINTK_VERSION" option for that board should be empty:
CONFIG_EARLY_PRINTK=scif,0xe6e60000

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

---
    Changes in v3:
        - It was decided not to introduce new debug-scifa.inc
          for handling SCIFA interface, but to extend existing
          debug-scif.inc for handling both interfaces.
          This patch is a result of splitting an initial patch
          "xen/arm: Add SCIFA UART support for early printk"
          and only reworks a code
---
 xen/arch/arm/Rules.mk             |  7 +++++++
 xen/arch/arm/arm32/debug-scif.inc | 13 +++++++++----
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
index f264592..3d9a0ed 100644
--- a/xen/arch/arm/Rules.mk
+++ b/xen/arch/arm/Rules.mk
@@ -68,6 +68,13 @@ EARLY_PRINTK_INIT_UART := y
 EARLY_PRINTK_BAUD := $(word 3,$(EARLY_PRINTK_CFG))
 endif
 endif
+ifeq ($(EARLY_PRINTK_INC),scif)
+ifneq ($(word 3,$(EARLY_PRINTK_CFG)),)
+CFLAGS-y += -DEARLY_PRINTK_VERSION_$(word 3,$(EARLY_PRINTK_CFG))
+else
+CFLAGS-y += -DEARLY_PRINTK_VERSION_NONE
+endif
+endif
 
 ifneq ($(EARLY_PRINTK_INC),)
 EARLY_PRINTK := y
diff --git a/xen/arch/arm/arm32/debug-scif.inc b/xen/arch/arm/arm32/debug-scif.inc
index 143f05d..a8d2eae 100644
--- a/xen/arch/arm/arm32/debug-scif.inc
+++ b/xen/arch/arm/arm32/debug-scif.inc
@@ -19,6 +19,11 @@
 
 #include <asm/scif-uart.h>
 
+#ifdef EARLY_PRINTK_VERSION_NONE
+#define STATUS_REG    SCIF_SCFSR
+#define TX_FIFO_REG   SCIF_SCFTDR
+#endif
+
 /*
  * SCIF UART wait UART to be ready to transmit
  * rb: register which contains the UART base address
@@ -26,7 +31,7 @@
  */
 .macro early_uart_ready rb rc
 1:
-        ldrh   \rc, [\rb, #SCIF_SCFSR]   /* <- SCFSR (status register) */
+        ldrh   \rc, [\rb, #STATUS_REG]   /* Read status register */
         tst    \rc, #SCFSR_TDFE          /* Check TDFE bit */
         beq    1b                        /* Wait for the UART to be ready */
 .endm
@@ -37,10 +42,10 @@
  * rt: register which contains the character to transmit
  */
 .macro early_uart_transmit rb rt
-        strb   \rt, [\rb, #SCIF_SCFTDR]                  /* -> SCFTDR (data register) */
-        ldrh   \rt, [\rb, #SCIF_SCFSR]                   /* <- SCFSR (status register) */
+        strb   \rt, [\rb, #TX_FIFO_REG]                  /* Write data register */
+        ldrh   \rt, [\rb, #STATUS_REG]                   /* Read status register */
         and    \rt, \rt, #(~(SCFSR_TEND | SCFSR_TDFE))   /* Clear TEND and TDFE bits */
-        strh   \rt, [\rb, #SCIF_SCFSR]                   /* -> SCFSR (status register) */
+        strh   \rt, [\rb, #STATUS_REG]                   /* Write status register */
 .endm
 
 /*
-- 
2.7.4


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

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [Xen-devel] [PATCH V3 4/5] xen/arm: Extend SCIF early prink code to handle other interfaces
@ 2019-04-08 10:14   ` Oleksandr Tyshchenko
  0 siblings, 0 replies; 32+ messages in thread
From: Oleksandr Tyshchenko @ 2019-04-08 10:14 UTC (permalink / raw)
  To: xen-devel; +Cc: Oleksandr Tyshchenko, julien.grall, sstabellini

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Extend early prink code 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 "EARLY_PRINTK_VERSION" config option to choose which
interface version should be used (to properly apply register offsets).

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

The "EARLY_PRINTK_VERSION" option for that board should be empty:
CONFIG_EARLY_PRINTK=scif,0xe6e60000

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

---
    Changes in v3:
        - It was decided not to introduce new debug-scifa.inc
          for handling SCIFA interface, but to extend existing
          debug-scif.inc for handling both interfaces.
          This patch is a result of splitting an initial patch
          "xen/arm: Add SCIFA UART support for early printk"
          and only reworks a code
---
 xen/arch/arm/Rules.mk             |  7 +++++++
 xen/arch/arm/arm32/debug-scif.inc | 13 +++++++++----
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
index f264592..3d9a0ed 100644
--- a/xen/arch/arm/Rules.mk
+++ b/xen/arch/arm/Rules.mk
@@ -68,6 +68,13 @@ EARLY_PRINTK_INIT_UART := y
 EARLY_PRINTK_BAUD := $(word 3,$(EARLY_PRINTK_CFG))
 endif
 endif
+ifeq ($(EARLY_PRINTK_INC),scif)
+ifneq ($(word 3,$(EARLY_PRINTK_CFG)),)
+CFLAGS-y += -DEARLY_PRINTK_VERSION_$(word 3,$(EARLY_PRINTK_CFG))
+else
+CFLAGS-y += -DEARLY_PRINTK_VERSION_NONE
+endif
+endif
 
 ifneq ($(EARLY_PRINTK_INC),)
 EARLY_PRINTK := y
diff --git a/xen/arch/arm/arm32/debug-scif.inc b/xen/arch/arm/arm32/debug-scif.inc
index 143f05d..a8d2eae 100644
--- a/xen/arch/arm/arm32/debug-scif.inc
+++ b/xen/arch/arm/arm32/debug-scif.inc
@@ -19,6 +19,11 @@
 
 #include <asm/scif-uart.h>
 
+#ifdef EARLY_PRINTK_VERSION_NONE
+#define STATUS_REG    SCIF_SCFSR
+#define TX_FIFO_REG   SCIF_SCFTDR
+#endif
+
 /*
  * SCIF UART wait UART to be ready to transmit
  * rb: register which contains the UART base address
@@ -26,7 +31,7 @@
  */
 .macro early_uart_ready rb rc
 1:
-        ldrh   \rc, [\rb, #SCIF_SCFSR]   /* <- SCFSR (status register) */
+        ldrh   \rc, [\rb, #STATUS_REG]   /* Read status register */
         tst    \rc, #SCFSR_TDFE          /* Check TDFE bit */
         beq    1b                        /* Wait for the UART to be ready */
 .endm
@@ -37,10 +42,10 @@
  * rt: register which contains the character to transmit
  */
 .macro early_uart_transmit rb rt
-        strb   \rt, [\rb, #SCIF_SCFTDR]                  /* -> SCFTDR (data register) */
-        ldrh   \rt, [\rb, #SCIF_SCFSR]                   /* <- SCFSR (status register) */
+        strb   \rt, [\rb, #TX_FIFO_REG]                  /* Write data register */
+        ldrh   \rt, [\rb, #STATUS_REG]                   /* Read status register */
         and    \rt, \rt, #(~(SCFSR_TEND | SCFSR_TDFE))   /* Clear TEND and TDFE bits */
-        strh   \rt, [\rb, #SCIF_SCFSR]                   /* -> SCFSR (status register) */
+        strh   \rt, [\rb, #STATUS_REG]                   /* Write status register */
 .endm
 
 /*
-- 
2.7.4


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

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH V3 5/5] xen/arm: Add early printk support for SCIFA compatible UARTs
@ 2019-04-08 10:14   ` Oleksandr Tyshchenko
  0 siblings, 0 replies; 32+ messages in thread
From: Oleksandr Tyshchenko @ 2019-04-08 10:14 UTC (permalink / raw)
  To: xen-devel; +Cc: Oleksandr Tyshchenko, julien.grall, sstabellini

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

This patch makes possible to use existing early prink code
for Renesas "Stout" board based on R-Car H2 SoC (SCIFA).

The "EARLY_PRINTK_VERSION" for that board should be 'A':
CONFIG_EARLY_PRINTK=scif,0xe6c40000,A

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

---
    Changes in v3:
        - It was decided not to introduce new debug-scifa.inc
          for handling SCIFA interface, but to extend existing
          debug-scif.inc for handling both interfaces.
          This patch is a result of splitting an initial patch
          "xen/arm: Add SCIFA UART support for early printk"
          and only adds a support.
---
 xen/arch/arm/arm32/debug-scif.inc | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/arm32/debug-scif.inc b/xen/arch/arm/arm32/debug-scif.inc
index a8d2eae..94a18ea 100644
--- a/xen/arch/arm/arm32/debug-scif.inc
+++ b/xen/arch/arm/arm32/debug-scif.inc
@@ -1,7 +1,7 @@
 /*
  * xen/arch/arm/arm32/debug-scif.inc
  *
- * SCIF specific debug code
+ * SCIF(A) specific debug code
  *
  * Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
  * Copyright (C) 2014, Globallogic.
@@ -22,10 +22,13 @@
 #ifdef EARLY_PRINTK_VERSION_NONE
 #define STATUS_REG    SCIF_SCFSR
 #define TX_FIFO_REG   SCIF_SCFTDR
+#elif EARLY_PRINTK_VERSION_A
+#define STATUS_REG    SCIFA_SCASSR
+#define TX_FIFO_REG   SCIFA_SCAFTDR
 #endif
 
 /*
- * SCIF UART wait UART to be ready to transmit
+ * SCIF(A) UART wait UART to be ready to transmit
  * rb: register which contains the UART base address
  * rc: scratch register
  */
@@ -37,7 +40,7 @@
 .endm
 
 /*
- * SCIF UART transmit character
+ * SCIF(A) UART transmit character
  * rb: register which contains the UART base address
  * rt: register which contains the character to transmit
  */
-- 
2.7.4


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

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [Xen-devel] [PATCH V3 5/5] xen/arm: Add early printk support for SCIFA compatible UARTs
@ 2019-04-08 10:14   ` Oleksandr Tyshchenko
  0 siblings, 0 replies; 32+ messages in thread
From: Oleksandr Tyshchenko @ 2019-04-08 10:14 UTC (permalink / raw)
  To: xen-devel; +Cc: Oleksandr Tyshchenko, julien.grall, sstabellini

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

This patch makes possible to use existing early prink code
for Renesas "Stout" board based on R-Car H2 SoC (SCIFA).

The "EARLY_PRINTK_VERSION" for that board should be 'A':
CONFIG_EARLY_PRINTK=scif,0xe6c40000,A

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

---
    Changes in v3:
        - It was decided not to introduce new debug-scifa.inc
          for handling SCIFA interface, but to extend existing
          debug-scif.inc for handling both interfaces.
          This patch is a result of splitting an initial patch
          "xen/arm: Add SCIFA UART support for early printk"
          and only adds a support.
---
 xen/arch/arm/arm32/debug-scif.inc | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/arm32/debug-scif.inc b/xen/arch/arm/arm32/debug-scif.inc
index a8d2eae..94a18ea 100644
--- a/xen/arch/arm/arm32/debug-scif.inc
+++ b/xen/arch/arm/arm32/debug-scif.inc
@@ -1,7 +1,7 @@
 /*
  * xen/arch/arm/arm32/debug-scif.inc
  *
- * SCIF specific debug code
+ * SCIF(A) specific debug code
  *
  * Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
  * Copyright (C) 2014, Globallogic.
@@ -22,10 +22,13 @@
 #ifdef EARLY_PRINTK_VERSION_NONE
 #define STATUS_REG    SCIF_SCFSR
 #define TX_FIFO_REG   SCIF_SCFTDR
+#elif EARLY_PRINTK_VERSION_A
+#define STATUS_REG    SCIFA_SCASSR
+#define TX_FIFO_REG   SCIFA_SCAFTDR
 #endif
 
 /*
- * SCIF UART wait UART to be ready to transmit
+ * SCIF(A) UART wait UART to be ready to transmit
  * rb: register which contains the UART base address
  * rc: scratch register
  */
@@ -37,7 +40,7 @@
 .endm
 
 /*
- * SCIF UART transmit character
+ * SCIF(A) UART transmit character
  * rb: register which contains the UART base address
  * rt: register which contains the character to transmit
  */
-- 
2.7.4


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

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH V3 2/5] xen/arm: drivers: scif: Extend driver to handle other interfaces
@ 2019-04-14 16:55     ` Julien Grall
  0 siblings, 0 replies; 32+ messages in thread
From: Julien Grall @ 2019-04-14 16:55 UTC (permalink / raw)
  To: Oleksandr Tyshchenko, xen-devel; +Cc: Oleksandr Tyshchenko, sstabellini

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,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH V3 2/5] xen/arm: drivers: scif: Extend driver to handle other interfaces
@ 2019-04-14 16:55     ` Julien Grall
  0 siblings, 0 replies; 32+ messages in thread
From: Julien Grall @ 2019-04-14 16:55 UTC (permalink / raw)
  To: Oleksandr Tyshchenko, xen-devel; +Cc: Oleksandr Tyshchenko, sstabellini

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,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH V3 3/5] xen/arm: drivers: scif: Add support for SCIFA compatible UARTs
@ 2019-04-14 16:57     ` Julien Grall
  0 siblings, 0 replies; 32+ messages in thread
From: Julien Grall @ 2019-04-14 16:57 UTC (permalink / raw)
  To: Oleksandr Tyshchenko, xen-devel; +Cc: Oleksandr Tyshchenko, sstabellini

Hi Olekansdr,

On 4/8/19 11:14 AM, Oleksandr Tyshchenko wrote:
> diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
> index 958f717..94cb230 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@globallogic.com>
> @@ -46,6 +46,7 @@ static struct scif_uart {
>   enum port_types
>   {
>       SCIF_PORT,
> +    SCIFA_PORT,
>       NR_PORTS,
>   };
>   
> @@ -74,6 +75,20 @@ static const struct port_params port_params[NR_PORTS] =
>           .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,

As mentioned in patch #2, overrun_mask will never be used here. Can you 
please confirm whether this is intended?

Cheers,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH V3 3/5] xen/arm: drivers: scif: Add support for SCIFA compatible UARTs
@ 2019-04-14 16:57     ` Julien Grall
  0 siblings, 0 replies; 32+ messages in thread
From: Julien Grall @ 2019-04-14 16:57 UTC (permalink / raw)
  To: Oleksandr Tyshchenko, xen-devel; +Cc: Oleksandr Tyshchenko, sstabellini

Hi Olekansdr,

On 4/8/19 11:14 AM, Oleksandr Tyshchenko wrote:
> diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
> index 958f717..94cb230 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@globallogic.com>
> @@ -46,6 +46,7 @@ static struct scif_uart {
>   enum port_types
>   {
>       SCIF_PORT,
> +    SCIFA_PORT,
>       NR_PORTS,
>   };
>   
> @@ -74,6 +75,20 @@ static const struct port_params port_params[NR_PORTS] =
>           .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,

As mentioned in patch #2, overrun_mask will never be used here. Can you 
please confirm whether this is intended?

Cheers,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH V3 4/5] xen/arm: Extend SCIF early prink code to handle other interfaces
@ 2019-04-14 17:48     ` Julien Grall
  0 siblings, 0 replies; 32+ messages in thread
From: Julien Grall @ 2019-04-14 17:48 UTC (permalink / raw)
  To: Oleksandr Tyshchenko, xen-devel; +Cc: Oleksandr Tyshchenko, sstabellini

Hi,

On 4/8/19 11:14 AM, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> Extend early prink code 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 "EARLY_PRINTK_VERSION" config option to choose which
> interface version should be used (to properly apply register offsets).
> 
> Please note, nothing has been technically changed for Renesas "Lager"
> and other supported boards (SCIF).
> 
> The "EARLY_PRINTK_VERSION" option for that board should be empty:
> CONFIG_EARLY_PRINTK=scif,0xe6e60000
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> CC: Julien Grall <julien.grall@arm.com>
> 
> ---
>      Changes in v3:
>          - It was decided not to introduce new debug-scifa.inc
>            for handling SCIFA interface, but to extend existing
>            debug-scif.inc for handling both interfaces.
>            This patch is a result of splitting an initial patch
>            "xen/arm: Add SCIFA UART support for early printk"
>            and only reworks a code
> ---
>   xen/arch/arm/Rules.mk             |  7 +++++++
>   xen/arch/arm/arm32/debug-scif.inc | 13 +++++++++----

You want to update docs/misc/arm/early-printk.txt with the new option.

Otherwise, the code looks good to me.

Cheers,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH V3 4/5] xen/arm: Extend SCIF early prink code to handle other interfaces
@ 2019-04-14 17:48     ` Julien Grall
  0 siblings, 0 replies; 32+ messages in thread
From: Julien Grall @ 2019-04-14 17:48 UTC (permalink / raw)
  To: Oleksandr Tyshchenko, xen-devel; +Cc: Oleksandr Tyshchenko, sstabellini

Hi,

On 4/8/19 11:14 AM, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> Extend early prink code 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 "EARLY_PRINTK_VERSION" config option to choose which
> interface version should be used (to properly apply register offsets).
> 
> Please note, nothing has been technically changed for Renesas "Lager"
> and other supported boards (SCIF).
> 
> The "EARLY_PRINTK_VERSION" option for that board should be empty:
> CONFIG_EARLY_PRINTK=scif,0xe6e60000
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> CC: Julien Grall <julien.grall@arm.com>
> 
> ---
>      Changes in v3:
>          - It was decided not to introduce new debug-scifa.inc
>            for handling SCIFA interface, but to extend existing
>            debug-scif.inc for handling both interfaces.
>            This patch is a result of splitting an initial patch
>            "xen/arm: Add SCIFA UART support for early printk"
>            and only reworks a code
> ---
>   xen/arch/arm/Rules.mk             |  7 +++++++
>   xen/arch/arm/arm32/debug-scif.inc | 13 +++++++++----

You want to update docs/misc/arm/early-printk.txt with the new option.

Otherwise, the code looks good to me.

Cheers,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH V3 5/5] xen/arm: Add early printk support for SCIFA compatible UARTs
@ 2019-04-14 17:56     ` Julien Grall
  0 siblings, 0 replies; 32+ messages in thread
From: Julien Grall @ 2019-04-14 17:56 UTC (permalink / raw)
  To: Oleksandr Tyshchenko, xen-devel; +Cc: Oleksandr Tyshchenko, sstabellini

Hi Oleksandr,

On 4/8/19 11:14 AM, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> This patch makes possible to use existing early prink code
> for Renesas "Stout" board based on R-Car H2 SoC (SCIFA).
> 
> The "EARLY_PRINTK_VERSION" for that board should be 'A':
> CONFIG_EARLY_PRINTK=scif,0xe6c40000,A
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> CC: Julien Grall <julien.grall@arm.com>
> 
> ---
>      Changes in v3:
>          - It was decided not to introduce new debug-scifa.inc
>            for handling SCIFA interface, but to extend existing
>            debug-scif.inc for handling both interfaces.
>            This patch is a result of splitting an initial patch
>            "xen/arm: Add SCIFA UART support for early printk"
>            and only adds a support.
> ---
>   xen/arch/arm/arm32/debug-scif.inc | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/debug-scif.inc b/xen/arch/arm/arm32/debug-scif.inc
> index a8d2eae..94a18ea 100644
> --- a/xen/arch/arm/arm32/debug-scif.inc
> +++ b/xen/arch/arm/arm32/debug-scif.inc
> @@ -1,7 +1,7 @@
>   /*
>    * xen/arch/arm/arm32/debug-scif.inc
>    *
> - * SCIF specific debug code
> + * SCIF(A) specific debug code
>    *
>    * Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
>    * Copyright (C) 2014, Globallogic.
> @@ -22,10 +22,13 @@
>   #ifdef EARLY_PRINTK_VERSION_NONE
>   #define STATUS_REG    SCIF_SCFSR
>   #define TX_FIFO_REG   SCIF_SCFTDR
> +#elif EARLY_PRINTK_VERSION_A
> +#define STATUS_REG    SCIFA_SCASSR
> +#define TX_FIFO_REG   SCIFA_SCAFTDR
>   #endif
>   
>   /*
> - * SCIF UART wait UART to be ready to transmit
> + * SCIF(A) UART wait UART to be ready to transmit

When SCIFB will be introduced, we will have to modify this comment 
again. As we are only dealing with any version of SCIC in this file, 
could we just rework the comment to avoid mentioning the UART name?

>    * rb: register which contains the UART base address
>    * rc: scratch register
>    */
> @@ -37,7 +40,7 @@
>   .endm
>   
>   /*
> - * SCIF UART transmit character
> + * SCIF(A) UART transmit character

Same here.

>    * rb: register which contains the UART base address
>    * rt: register which contains the character to transmit
>    */
> 

Cheers,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH V3 5/5] xen/arm: Add early printk support for SCIFA compatible UARTs
@ 2019-04-14 17:56     ` Julien Grall
  0 siblings, 0 replies; 32+ messages in thread
From: Julien Grall @ 2019-04-14 17:56 UTC (permalink / raw)
  To: Oleksandr Tyshchenko, xen-devel; +Cc: Oleksandr Tyshchenko, sstabellini

Hi Oleksandr,

On 4/8/19 11:14 AM, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> This patch makes possible to use existing early prink code
> for Renesas "Stout" board based on R-Car H2 SoC (SCIFA).
> 
> The "EARLY_PRINTK_VERSION" for that board should be 'A':
> CONFIG_EARLY_PRINTK=scif,0xe6c40000,A
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> CC: Julien Grall <julien.grall@arm.com>
> 
> ---
>      Changes in v3:
>          - It was decided not to introduce new debug-scifa.inc
>            for handling SCIFA interface, but to extend existing
>            debug-scif.inc for handling both interfaces.
>            This patch is a result of splitting an initial patch
>            "xen/arm: Add SCIFA UART support for early printk"
>            and only adds a support.
> ---
>   xen/arch/arm/arm32/debug-scif.inc | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/debug-scif.inc b/xen/arch/arm/arm32/debug-scif.inc
> index a8d2eae..94a18ea 100644
> --- a/xen/arch/arm/arm32/debug-scif.inc
> +++ b/xen/arch/arm/arm32/debug-scif.inc
> @@ -1,7 +1,7 @@
>   /*
>    * xen/arch/arm/arm32/debug-scif.inc
>    *
> - * SCIF specific debug code
> + * SCIF(A) specific debug code
>    *
>    * Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
>    * Copyright (C) 2014, Globallogic.
> @@ -22,10 +22,13 @@
>   #ifdef EARLY_PRINTK_VERSION_NONE
>   #define STATUS_REG    SCIF_SCFSR
>   #define TX_FIFO_REG   SCIF_SCFTDR
> +#elif EARLY_PRINTK_VERSION_A
> +#define STATUS_REG    SCIFA_SCASSR
> +#define TX_FIFO_REG   SCIFA_SCAFTDR
>   #endif
>   
>   /*
> - * SCIF UART wait UART to be ready to transmit
> + * SCIF(A) UART wait UART to be ready to transmit

When SCIFB will be introduced, we will have to modify this comment 
again. As we are only dealing with any version of SCIC in this file, 
could we just rework the comment to avoid mentioning the UART name?

>    * rb: register which contains the UART base address
>    * rc: scratch register
>    */
> @@ -37,7 +40,7 @@
>   .endm
>   
>   /*
> - * SCIF UART transmit character
> + * SCIF(A) UART transmit character

Same here.

>    * rb: register which contains the UART base address
>    * rt: register which contains the character to transmit
>    */
> 

Cheers,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH V3 2/5] xen/arm: drivers: scif: Extend driver to handle other interfaces
@ 2019-04-15 11:30       ` Oleksandr
  0 siblings, 0 replies; 32+ messages in thread
From: Oleksandr @ 2019-04-15 11:30 UTC (permalink / raw)
  To: Julien Grall, xen-devel; +Cc: Oleksandr Tyshchenko, sstabellini


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,
>
-- 
Regards,

Oleksandr Tyshchenko


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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH V3 2/5] xen/arm: drivers: scif: Extend driver to handle other interfaces
@ 2019-04-15 11:30       ` Oleksandr
  0 siblings, 0 replies; 32+ messages in thread
From: Oleksandr @ 2019-04-15 11:30 UTC (permalink / raw)
  To: Julien Grall, xen-devel; +Cc: Oleksandr Tyshchenko, sstabellini


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,
>
-- 
Regards,

Oleksandr Tyshchenko


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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH V3 4/5] xen/arm: Extend SCIF early prink code to handle other interfaces
@ 2019-04-15 11:38       ` Oleksandr
  0 siblings, 0 replies; 32+ messages in thread
From: Oleksandr @ 2019-04-15 11:38 UTC (permalink / raw)
  To: Julien Grall, xen-devel; +Cc: Oleksandr Tyshchenko, sstabellini


On 14.04.19 20:48, Julien Grall wrote:
> Hi,

Hi Julien


>
> On 4/8/19 11:14 AM, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> Extend early prink code 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 "EARLY_PRINTK_VERSION" config option to choose which
>> interface version should be used (to properly apply register offsets).
>>
>> Please note, nothing has been technically changed for Renesas "Lager"
>> and other supported boards (SCIF).
>>
>> The "EARLY_PRINTK_VERSION" option for that board should be empty:
>> CONFIG_EARLY_PRINTK=scif,0xe6e60000
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> CC: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>      Changes in v3:
>>          - It was decided not to introduce new debug-scifa.inc
>>            for handling SCIFA interface, but to extend existing
>>            debug-scif.inc for handling both interfaces.
>>            This patch is a result of splitting an initial patch
>>            "xen/arm: Add SCIFA UART support for early printk"
>>            and only reworks a code
>> ---
>>   xen/arch/arm/Rules.mk             |  7 +++++++
>>   xen/arch/arm/arm32/debug-scif.inc | 13 +++++++++----
>
> You want to update docs/misc/arm/early-printk.txt with the new option.

Sure, will update.


>
> Otherwise, the code looks good to me.
>
> Cheers,
>
-- 
Regards,

Oleksandr Tyshchenko


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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH V3 4/5] xen/arm: Extend SCIF early prink code to handle other interfaces
@ 2019-04-15 11:38       ` Oleksandr
  0 siblings, 0 replies; 32+ messages in thread
From: Oleksandr @ 2019-04-15 11:38 UTC (permalink / raw)
  To: Julien Grall, xen-devel; +Cc: Oleksandr Tyshchenko, sstabellini


On 14.04.19 20:48, Julien Grall wrote:
> Hi,

Hi Julien


>
> On 4/8/19 11:14 AM, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> Extend early prink code 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 "EARLY_PRINTK_VERSION" config option to choose which
>> interface version should be used (to properly apply register offsets).
>>
>> Please note, nothing has been technically changed for Renesas "Lager"
>> and other supported boards (SCIF).
>>
>> The "EARLY_PRINTK_VERSION" option for that board should be empty:
>> CONFIG_EARLY_PRINTK=scif,0xe6e60000
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> CC: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>      Changes in v3:
>>          - It was decided not to introduce new debug-scifa.inc
>>            for handling SCIFA interface, but to extend existing
>>            debug-scif.inc for handling both interfaces.
>>            This patch is a result of splitting an initial patch
>>            "xen/arm: Add SCIFA UART support for early printk"
>>            and only reworks a code
>> ---
>>   xen/arch/arm/Rules.mk             |  7 +++++++
>>   xen/arch/arm/arm32/debug-scif.inc | 13 +++++++++----
>
> You want to update docs/misc/arm/early-printk.txt with the new option.

Sure, will update.


>
> Otherwise, the code looks good to me.
>
> Cheers,
>
-- 
Regards,

Oleksandr Tyshchenko


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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH V3 5/5] xen/arm: Add early printk support for SCIFA compatible UARTs
@ 2019-04-15 11:43       ` Oleksandr
  0 siblings, 0 replies; 32+ messages in thread
From: Oleksandr @ 2019-04-15 11:43 UTC (permalink / raw)
  To: Julien Grall, xen-devel; +Cc: Oleksandr Tyshchenko, sstabellini


On 14.04.19 20:56, Julien Grall wrote:
> Hi Oleksandr,

Hi Julien


>
> On 4/8/19 11:14 AM, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> This patch makes possible to use existing early prink code
>> for Renesas "Stout" board based on R-Car H2 SoC (SCIFA).
>>
>> The "EARLY_PRINTK_VERSION" for that board should be 'A':
>> CONFIG_EARLY_PRINTK=scif,0xe6c40000,A
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> CC: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>      Changes in v3:
>>          - It was decided not to introduce new debug-scifa.inc
>>            for handling SCIFA interface, but to extend existing
>>            debug-scif.inc for handling both interfaces.
>>            This patch is a result of splitting an initial patch
>>            "xen/arm: Add SCIFA UART support for early printk"
>>            and only adds a support.
>> ---
>>   xen/arch/arm/arm32/debug-scif.inc | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm32/debug-scif.inc 
>> b/xen/arch/arm/arm32/debug-scif.inc
>> index a8d2eae..94a18ea 100644
>> --- a/xen/arch/arm/arm32/debug-scif.inc
>> +++ b/xen/arch/arm/arm32/debug-scif.inc
>> @@ -1,7 +1,7 @@
>>   /*
>>    * xen/arch/arm/arm32/debug-scif.inc
>>    *
>> - * SCIF specific debug code
>> + * SCIF(A) specific debug code
>>    *
>>    * Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
>>    * Copyright (C) 2014, Globallogic.
>> @@ -22,10 +22,13 @@
>>   #ifdef EARLY_PRINTK_VERSION_NONE
>>   #define STATUS_REG    SCIF_SCFSR
>>   #define TX_FIFO_REG   SCIF_SCFTDR
>> +#elif EARLY_PRINTK_VERSION_A
>> +#define STATUS_REG    SCIFA_SCASSR
>> +#define TX_FIFO_REG   SCIFA_SCAFTDR
>>   #endif
>>     /*
>> - * SCIF UART wait UART to be ready to transmit
>> + * SCIF(A) UART wait UART to be ready to transmit
>
> When SCIFB will be introduced, we will have to modify this comment 
> again. As we are only dealing with any version of SCIC in this file, 
> could we just rework the comment to avoid mentioning the UART name?

I think, yes. What about using SCIF(X)? Shall I update 
scif-uart.c/scif-uart.h which has SCIF(A) string?


>
>>    * rb: register which contains the UART base address
>>    * rc: scratch register
>>    */
>> @@ -37,7 +40,7 @@
>>   .endm
>>     /*
>> - * SCIF UART transmit character
>> + * SCIF(A) UART transmit character
>
> Same here.
>
>>    * rb: register which contains the UART base address
>>    * rt: register which contains the character to transmit
>>    */
>>
>
> Cheers,
>
-- 
Regards,

Oleksandr Tyshchenko


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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH V3 5/5] xen/arm: Add early printk support for SCIFA compatible UARTs
@ 2019-04-15 11:43       ` Oleksandr
  0 siblings, 0 replies; 32+ messages in thread
From: Oleksandr @ 2019-04-15 11:43 UTC (permalink / raw)
  To: Julien Grall, xen-devel; +Cc: Oleksandr Tyshchenko, sstabellini


On 14.04.19 20:56, Julien Grall wrote:
> Hi Oleksandr,

Hi Julien


>
> On 4/8/19 11:14 AM, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> This patch makes possible to use existing early prink code
>> for Renesas "Stout" board based on R-Car H2 SoC (SCIFA).
>>
>> The "EARLY_PRINTK_VERSION" for that board should be 'A':
>> CONFIG_EARLY_PRINTK=scif,0xe6c40000,A
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> CC: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>      Changes in v3:
>>          - It was decided not to introduce new debug-scifa.inc
>>            for handling SCIFA interface, but to extend existing
>>            debug-scif.inc for handling both interfaces.
>>            This patch is a result of splitting an initial patch
>>            "xen/arm: Add SCIFA UART support for early printk"
>>            and only adds a support.
>> ---
>>   xen/arch/arm/arm32/debug-scif.inc | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm32/debug-scif.inc 
>> b/xen/arch/arm/arm32/debug-scif.inc
>> index a8d2eae..94a18ea 100644
>> --- a/xen/arch/arm/arm32/debug-scif.inc
>> +++ b/xen/arch/arm/arm32/debug-scif.inc
>> @@ -1,7 +1,7 @@
>>   /*
>>    * xen/arch/arm/arm32/debug-scif.inc
>>    *
>> - * SCIF specific debug code
>> + * SCIF(A) specific debug code
>>    *
>>    * Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
>>    * Copyright (C) 2014, Globallogic.
>> @@ -22,10 +22,13 @@
>>   #ifdef EARLY_PRINTK_VERSION_NONE
>>   #define STATUS_REG    SCIF_SCFSR
>>   #define TX_FIFO_REG   SCIF_SCFTDR
>> +#elif EARLY_PRINTK_VERSION_A
>> +#define STATUS_REG    SCIFA_SCASSR
>> +#define TX_FIFO_REG   SCIFA_SCAFTDR
>>   #endif
>>     /*
>> - * SCIF UART wait UART to be ready to transmit
>> + * SCIF(A) UART wait UART to be ready to transmit
>
> When SCIFB will be introduced, we will have to modify this comment 
> again. As we are only dealing with any version of SCIC in this file, 
> could we just rework the comment to avoid mentioning the UART name?

I think, yes. What about using SCIF(X)? Shall I update 
scif-uart.c/scif-uart.h which has SCIF(A) string?


>
>>    * rb: register which contains the UART base address
>>    * rc: scratch register
>>    */
>> @@ -37,7 +40,7 @@
>>   .endm
>>     /*
>> - * SCIF UART transmit character
>> + * SCIF(A) UART transmit character
>
> Same here.
>
>>    * rb: register which contains the UART base address
>>    * rt: register which contains the character to transmit
>>    */
>>
>
> Cheers,
>
-- 
Regards,

Oleksandr Tyshchenko


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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH V3 2/5] xen/arm: drivers: scif: Extend driver to handle other interfaces
@ 2019-04-16  9:11         ` Julien Grall
  0 siblings, 0 replies; 32+ messages in thread
From: Julien Grall @ 2019-04-16  9:11 UTC (permalink / raw)
  To: Oleksandr, xen-devel; +Cc: Oleksandr Tyshchenko, sstabellini

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,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH V3 2/5] xen/arm: drivers: scif: Extend driver to handle other interfaces
@ 2019-04-16  9:11         ` Julien Grall
  0 siblings, 0 replies; 32+ messages in thread
From: Julien Grall @ 2019-04-16  9:11 UTC (permalink / raw)
  To: Oleksandr, xen-devel; +Cc: Oleksandr Tyshchenko, sstabellini

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,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH V3 5/5] xen/arm: Add early printk support for SCIFA compatible UARTs
@ 2019-04-16  9:16         ` Julien Grall
  0 siblings, 0 replies; 32+ messages in thread
From: Julien Grall @ 2019-04-16  9:16 UTC (permalink / raw)
  To: Oleksandr, xen-devel; +Cc: Oleksandr Tyshchenko, sstabellini

Hi Oleksandr,

On 4/15/19 12:43 PM, Oleksandr wrote:
> 
> On 14.04.19 20:56, Julien Grall wrote:
>> On 4/8/19 11:14 AM, Oleksandr Tyshchenko wrote:
>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>
>>> This patch makes possible to use existing early prink code
>>> for Renesas "Stout" board based on R-Car H2 SoC (SCIFA).
>>>
>>> The "EARLY_PRINTK_VERSION" for that board should be 'A':
>>> CONFIG_EARLY_PRINTK=scif,0xe6c40000,A
>>>
>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>> CC: Julien Grall <julien.grall@arm.com>
>>>
>>> ---
>>>      Changes in v3:
>>>          - It was decided not to introduce new debug-scifa.inc
>>>            for handling SCIFA interface, but to extend existing
>>>            debug-scif.inc for handling both interfaces.
>>>            This patch is a result of splitting an initial patch
>>>            "xen/arm: Add SCIFA UART support for early printk"
>>>            and only adds a support.
>>> ---
>>>   xen/arch/arm/arm32/debug-scif.inc | 9 ++++++---
>>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/arm32/debug-scif.inc 
>>> b/xen/arch/arm/arm32/debug-scif.inc
>>> index a8d2eae..94a18ea 100644
>>> --- a/xen/arch/arm/arm32/debug-scif.inc
>>> +++ b/xen/arch/arm/arm32/debug-scif.inc
>>> @@ -1,7 +1,7 @@
>>>   /*
>>>    * xen/arch/arm/arm32/debug-scif.inc
>>>    *
>>> - * SCIF specific debug code
>>> + * SCIF(A) specific debug code
>>>    *
>>>    * Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
>>>    * Copyright (C) 2014, Globallogic.
>>> @@ -22,10 +22,13 @@
>>>   #ifdef EARLY_PRINTK_VERSION_NONE
>>>   #define STATUS_REG    SCIF_SCFSR
>>>   #define TX_FIFO_REG   SCIF_SCFTDR
>>> +#elif EARLY_PRINTK_VERSION_A
>>> +#define STATUS_REG    SCIFA_SCASSR
>>> +#define TX_FIFO_REG   SCIFA_SCAFTDR
>>>   #endif
>>>     /*
>>> - * SCIF UART wait UART to be ready to transmit
>>> + * SCIF(A) UART wait UART to be ready to transmit
>>
>> When SCIFB will be introduced, we will have to modify this comment 
>> again. As we are only dealing with any version of SCIC in this file, 
>> could we just rework the comment to avoid mentioning the UART name?
> 
> I think, yes. What about using SCIF(X)? Shall I update 
> scif-uart.c/scif-uart.h which has SCIF(A) string?

In comments, I would just drop completely "SCIF(A)".

For copyright header, we can either keep SCIF(A) or use SCIF(X). Which 
ever you prefer.

Cheers,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH V3 5/5] xen/arm: Add early printk support for SCIFA compatible UARTs
@ 2019-04-16  9:16         ` Julien Grall
  0 siblings, 0 replies; 32+ messages in thread
From: Julien Grall @ 2019-04-16  9:16 UTC (permalink / raw)
  To: Oleksandr, xen-devel; +Cc: Oleksandr Tyshchenko, sstabellini

Hi Oleksandr,

On 4/15/19 12:43 PM, Oleksandr wrote:
> 
> On 14.04.19 20:56, Julien Grall wrote:
>> On 4/8/19 11:14 AM, Oleksandr Tyshchenko wrote:
>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>
>>> This patch makes possible to use existing early prink code
>>> for Renesas "Stout" board based on R-Car H2 SoC (SCIFA).
>>>
>>> The "EARLY_PRINTK_VERSION" for that board should be 'A':
>>> CONFIG_EARLY_PRINTK=scif,0xe6c40000,A
>>>
>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>> CC: Julien Grall <julien.grall@arm.com>
>>>
>>> ---
>>>      Changes in v3:
>>>          - It was decided not to introduce new debug-scifa.inc
>>>            for handling SCIFA interface, but to extend existing
>>>            debug-scif.inc for handling both interfaces.
>>>            This patch is a result of splitting an initial patch
>>>            "xen/arm: Add SCIFA UART support for early printk"
>>>            and only adds a support.
>>> ---
>>>   xen/arch/arm/arm32/debug-scif.inc | 9 ++++++---
>>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/arm32/debug-scif.inc 
>>> b/xen/arch/arm/arm32/debug-scif.inc
>>> index a8d2eae..94a18ea 100644
>>> --- a/xen/arch/arm/arm32/debug-scif.inc
>>> +++ b/xen/arch/arm/arm32/debug-scif.inc
>>> @@ -1,7 +1,7 @@
>>>   /*
>>>    * xen/arch/arm/arm32/debug-scif.inc
>>>    *
>>> - * SCIF specific debug code
>>> + * SCIF(A) specific debug code
>>>    *
>>>    * Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
>>>    * Copyright (C) 2014, Globallogic.
>>> @@ -22,10 +22,13 @@
>>>   #ifdef EARLY_PRINTK_VERSION_NONE
>>>   #define STATUS_REG    SCIF_SCFSR
>>>   #define TX_FIFO_REG   SCIF_SCFTDR
>>> +#elif EARLY_PRINTK_VERSION_A
>>> +#define STATUS_REG    SCIFA_SCASSR
>>> +#define TX_FIFO_REG   SCIFA_SCAFTDR
>>>   #endif
>>>     /*
>>> - * SCIF UART wait UART to be ready to transmit
>>> + * SCIF(A) UART wait UART to be ready to transmit
>>
>> When SCIFB will be introduced, we will have to modify this comment 
>> again. As we are only dealing with any version of SCIC in this file, 
>> could we just rework the comment to avoid mentioning the UART name?
> 
> I think, yes. What about using SCIF(X)? Shall I update 
> scif-uart.c/scif-uart.h which has SCIF(A) string?

In comments, I would just drop completely "SCIF(A)".

For copyright header, we can either keep SCIF(A) or use SCIF(X). Which 
ever you prefer.

Cheers,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH V3 2/5] xen/arm: drivers: scif: Extend driver to handle other interfaces
@ 2019-04-16 13:26           ` Oleksandr
  0 siblings, 0 replies; 32+ messages in thread
From: Oleksandr @ 2019-04-16 13:26 UTC (permalink / raw)
  To: Julien Grall, xen-devel; +Cc: Oleksandr Tyshchenko, sstabellini


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,
>
-- 
Regards,

Oleksandr Tyshchenko


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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH V3 2/5] xen/arm: drivers: scif: Extend driver to handle other interfaces
@ 2019-04-16 13:26           ` Oleksandr
  0 siblings, 0 replies; 32+ messages in thread
From: Oleksandr @ 2019-04-16 13:26 UTC (permalink / raw)
  To: Julien Grall, xen-devel; +Cc: Oleksandr Tyshchenko, sstabellini


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,
>
-- 
Regards,

Oleksandr Tyshchenko


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

^ permalink raw reply	[flat|nested] 32+ messages in thread

end of thread, other threads:[~2019-04-16 13:27 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-08 10:14 [PATCH V3 0/5] Renesas Stout board support (R-Car Gen2) Oleksandr Tyshchenko
2019-04-08 10:14 ` [Xen-devel] " Oleksandr Tyshchenko
2019-04-08 10:14 ` [PATCH V3 1/5] xen/arm: Clarify usage of earlyprintk for Lager board Oleksandr Tyshchenko
2019-04-08 10:14   ` [Xen-devel] " Oleksandr Tyshchenko
2019-04-08 10:14 ` [PATCH V3 2/5] xen/arm: drivers: scif: Extend driver to handle other interfaces Oleksandr Tyshchenko
2019-04-08 10:14   ` [Xen-devel] " Oleksandr Tyshchenko
2019-04-14 16:55   ` Julien Grall
2019-04-14 16:55     ` [Xen-devel] " Julien Grall
2019-04-15 11:30     ` Oleksandr
2019-04-15 11:30       ` [Xen-devel] " Oleksandr
2019-04-16  9:11       ` Julien Grall
2019-04-16  9:11         ` [Xen-devel] " Julien Grall
2019-04-16 13:26         ` Oleksandr
2019-04-16 13:26           ` [Xen-devel] " Oleksandr
2019-04-08 10:14 ` [PATCH V3 3/5] xen/arm: drivers: scif: Add support for SCIFA compatible UARTs Oleksandr Tyshchenko
2019-04-08 10:14   ` [Xen-devel] " Oleksandr Tyshchenko
2019-04-14 16:57   ` Julien Grall
2019-04-14 16:57     ` [Xen-devel] " Julien Grall
2019-04-08 10:14 ` [PATCH V3 4/5] xen/arm: Extend SCIF early prink code to handle other interfaces Oleksandr Tyshchenko
2019-04-08 10:14   ` [Xen-devel] " Oleksandr Tyshchenko
2019-04-14 17:48   ` Julien Grall
2019-04-14 17:48     ` [Xen-devel] " Julien Grall
2019-04-15 11:38     ` Oleksandr
2019-04-15 11:38       ` [Xen-devel] " Oleksandr
2019-04-08 10:14 ` [PATCH V3 5/5] xen/arm: Add early printk support for SCIFA compatible UARTs Oleksandr Tyshchenko
2019-04-08 10:14   ` [Xen-devel] " Oleksandr Tyshchenko
2019-04-14 17:56   ` Julien Grall
2019-04-14 17:56     ` [Xen-devel] " Julien Grall
2019-04-15 11:43     ` Oleksandr
2019-04-15 11:43       ` [Xen-devel] " Oleksandr
2019-04-16  9:16       ` Julien Grall
2019-04-16  9:16         ` [Xen-devel] " Julien Grall

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.