[PATCH v3 1/2] xen/arm: drivers: scif: Remove dead code

[PATCH v3 1/2] xen/arm: drivers: scif: Remove dead code

[Dirk Behme]

The two struct members baud and clock_hz are in the end read only
variables nowhere used for anything useful. Removing them makes
the code much simpler without changing any functionality.

Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
---
 xen/drivers/char/scif-uart.c    | 24 +-----------------------
 xen/include/asm-arm/scif-uart.h |  1 -
 2 files changed, 1 insertion(+), 24 deletions(-)

diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
index 51a2233..bc157fe 100644
--- a/xen/drivers/char/scif-uart.c
+++ b/xen/drivers/char/scif-uart.c
@@ -41,7 +41,7 @@
 #define scif_writew(uart, off, val)    writew((val), (uart)->regs + (off))
 
 static struct scif_uart {
-    unsigned int baud, clock_hz, data_bits, parity, stop_bits;
+    unsigned int data_bits, parity, stop_bits;
     unsigned int irq;
     char __iomem *regs;
     struct irqaction irqaction;
@@ -87,7 +87,6 @@ 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;
-    unsigned int divisor;
     uint16_t val;
 
     /*
@@ -142,25 +141,6 @@ static void __init scif_uart_init_preirq(struct serial_port *port)
     }
     scif_writew(uart, SCIF_SCSMR, val);
 
-    ASSERT( uart->clock_hz > 0 );
-    if ( uart->baud != BAUD_AUTO )
-    {
-        /* Setup desired Baud rate */
-        divisor = uart->clock_hz / (uart->baud << 4);
-        ASSERT( divisor >= 1 && divisor <= (uint16_t)UINT_MAX );
-        scif_writew(uart, SCIF_DL, (uint16_t)divisor);
-        /* Selects the frequency divided clock (SC_CLK external input) */
-        scif_writew(uart, SCIF_CKS, 0);
-        udelay(1000000 / uart->baud + 1);
-    }
-    else
-    {
-        /* Read current Baud rate */
-        divisor = scif_readw(uart, SCIF_DL);
-        ASSERT( divisor >= 1 && divisor <= (uint16_t)UINT_MAX );
-        uart->baud = uart->clock_hz / (divisor << 4);
-    }
-
     /* Setup trigger level for TX/RX FIFOs */
     scif_writew(uart, SCIF_SCFCR, SCFCR_RTRG11 | SCFCR_TTRG11);
 
@@ -303,8 +283,6 @@ static int __init scif_uart_init(struct dt_device_node *dev,
 
     uart = &scif_com;
 
-    uart->clock_hz  = SCIF_CLK_FREQ;
-    uart->baud      = BAUD_AUTO;
     uart->data_bits = 8;
     uart->parity    = PARITY_NONE;
     uart->stop_bits = 1;
diff --git a/xen/include/asm-arm/scif-uart.h b/xen/include/asm-arm/scif-uart.h
index 7a9f639..d030b26 100644
--- a/xen/include/asm-arm/scif-uart.h
+++ b/xen/include/asm-arm/scif-uart.h
@@ -22,7 +22,6 @@
 #define __ASM_ARM_SCIF_UART_H
 
 #define SCIF_FIFO_MAX_SIZE    16
-#define SCIF_CLK_FREQ         14745600
 
 /* Register offsets */
 #define SCIF_SCSMR     (0x00)    /* Serial mode register           */
-- 
2.8.0


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

[PATCH v3 2/2] xen/arm: drivers: scif: Don't overwrite firmware settings

[Dirk Behme]

Besides the 14MHz external clock, the SCIF might be clocked by an
internal 66MHz clock. If this is the case, the current clock source
selection breaks this configuration. Same for the settings done by
the firmware for data bits, stop bits and parity.

Completely drop this and rely on the settings done by the firmware.

Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
---
 xen/drivers/char/scif-uart.c | 40 ----------------------------------------
 1 file changed, 40 deletions(-)

diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
index bc157fe..f9ae257 100644
--- a/xen/drivers/char/scif-uart.c
+++ b/xen/drivers/char/scif-uart.c
@@ -41,7 +41,6 @@
 #define scif_writew(uart, off, val)    writew((val), (uart)->regs + (off))
 
 static struct scif_uart {
-    unsigned int data_bits, parity, stop_bits;
     unsigned int irq;
     char __iomem *regs;
     struct irqaction irqaction;
@@ -87,7 +86,6 @@ 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;
-    uint16_t val;
 
     /*
      * Wait until last bit has been transmitted. This is needed for a smooth
@@ -107,40 +105,6 @@ static void __init scif_uart_init_preirq(struct serial_port *port)
     scif_readw(uart, SCIF_SCLSR);
     scif_writew(uart, SCIF_SCLSR, 0);
 
-    /* Select Baud rate generator output as a clock source */
-    scif_writew(uart, SCIF_SCSCR, SCSCR_CKE10);
-
-    /* Setup protocol format and Baud rate, select Asynchronous mode */
-    val = 0;
-    ASSERT( uart->data_bits >= 7 && uart->data_bits <= 8 );
-    if ( uart->data_bits == 7 )
-        val |= SCSMR_CHR;
-    else
-        val &= ~SCSMR_CHR;
-
-    ASSERT( uart->stop_bits >= 1 && uart->stop_bits <= 2 );
-    if ( uart->stop_bits == 2 )
-        val |= SCSMR_STOP;
-    else
-        val &= ~SCSMR_STOP;
-
-    ASSERT( uart->parity >= PARITY_NONE && uart->parity <= PARITY_ODD );
-    switch ( uart->parity )
-    {
-    case PARITY_NONE:
-        val &= ~SCSMR_PE;
-        break;
-
-    case PARITY_EVEN:
-        val |= SCSMR_PE;
-        break;
-
-    case PARITY_ODD:
-        val |= SCSMR_PE | SCSMR_ODD;
-        break;
-    }
-    scif_writew(uart, SCIF_SCSMR, val);
-
     /* Setup trigger level for TX/RX FIFOs */
     scif_writew(uart, SCIF_SCFCR, SCFCR_RTRG11 | SCFCR_TTRG11);
 
@@ -283,10 +247,6 @@ static int __init scif_uart_init(struct dt_device_node *dev,
 
     uart = &scif_com;
 
-    uart->data_bits = 8;
-    uart->parity    = PARITY_NONE;
-    uart->stop_bits = 1;
-
     res = dt_device_get_address(dev, 0, &addr, &size);
     if ( res )
     {
-- 
2.8.0


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

Re: [PATCH v3 1/2] xen/arm: drivers: scif: Remove dead code

[Julien Grall]

(CC "THE REST" maintainers)

Hi Dirk,

On 22/06/16 12:49, Dirk Behme wrote:
> The two struct members baud and clock_hz are in the end read only
> variables nowhere used for anything useful. Removing them makes
> the code much simpler without changing any functionality.
>
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>

Note that technically this driver is supported by "THE REST" 
maintainers. It might be worth to add it under "ARM (W/ VIRTUALISATION 
EXTENSIONS) ARCHITECTURE" as it is already the case for the other ARM 
specific UART drivers.

FWIW: Reviewed-by: Julien Grall <julien.grall@arm.com>

Regards,

> ---
>   xen/drivers/char/scif-uart.c    | 24 +-----------------------
>   xen/include/asm-arm/scif-uart.h |  1 -
>   2 files changed, 1 insertion(+), 24 deletions(-)
>
> diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
> index 51a2233..bc157fe 100644
> --- a/xen/drivers/char/scif-uart.c
> +++ b/xen/drivers/char/scif-uart.c
> @@ -41,7 +41,7 @@
>   #define scif_writew(uart, off, val)    writew((val), (uart)->regs + (off))
>
>   static struct scif_uart {
> -    unsigned int baud, clock_hz, data_bits, parity, stop_bits;
> +    unsigned int data_bits, parity, stop_bits;
>       unsigned int irq;
>       char __iomem *regs;
>       struct irqaction irqaction;
> @@ -87,7 +87,6 @@ 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;
> -    unsigned int divisor;
>       uint16_t val;
>
>       /*
> @@ -142,25 +141,6 @@ static void __init scif_uart_init_preirq(struct serial_port *port)
>       }
>       scif_writew(uart, SCIF_SCSMR, val);
>
> -    ASSERT( uart->clock_hz > 0 );
> -    if ( uart->baud != BAUD_AUTO )
> -    {
> -        /* Setup desired Baud rate */
> -        divisor = uart->clock_hz / (uart->baud << 4);
> -        ASSERT( divisor >= 1 && divisor <= (uint16_t)UINT_MAX );
> -        scif_writew(uart, SCIF_DL, (uint16_t)divisor);
> -        /* Selects the frequency divided clock (SC_CLK external input) */
> -        scif_writew(uart, SCIF_CKS, 0);
> -        udelay(1000000 / uart->baud + 1);
> -    }
> -    else
> -    {
> -        /* Read current Baud rate */
> -        divisor = scif_readw(uart, SCIF_DL);
> -        ASSERT( divisor >= 1 && divisor <= (uint16_t)UINT_MAX );
> -        uart->baud = uart->clock_hz / (divisor << 4);
> -    }
> -
>       /* Setup trigger level for TX/RX FIFOs */
>       scif_writew(uart, SCIF_SCFCR, SCFCR_RTRG11 | SCFCR_TTRG11);
>
> @@ -303,8 +283,6 @@ static int __init scif_uart_init(struct dt_device_node *dev,
>
>       uart = &scif_com;
>
> -    uart->clock_hz  = SCIF_CLK_FREQ;
> -    uart->baud      = BAUD_AUTO;
>       uart->data_bits = 8;
>       uart->parity    = PARITY_NONE;
>       uart->stop_bits = 1;
> diff --git a/xen/include/asm-arm/scif-uart.h b/xen/include/asm-arm/scif-uart.h
> index 7a9f639..d030b26 100644
> --- a/xen/include/asm-arm/scif-uart.h
> +++ b/xen/include/asm-arm/scif-uart.h
> @@ -22,7 +22,6 @@
>   #define __ASM_ARM_SCIF_UART_H
>
>   #define SCIF_FIFO_MAX_SIZE    16
> -#define SCIF_CLK_FREQ         14745600
>
>   /* Register offsets */
>   #define SCIF_SCSMR     (0x00)    /* Serial mode register           */
>

-- 
Julien Grall

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

Re: [PATCH v3 2/2] xen/arm: drivers: scif: Don't overwrite firmware settings

[Julien Grall]

Hi Dirk,

On 22/06/16 12:49, Dirk Behme wrote:
> Besides the 14MHz external clock, the SCIF might be clocked by an
> internal 66MHz clock. If this is the case, the current clock source
> selection breaks this configuration. Same for the settings done by
> the firmware for data bits, stop bits and parity.
>
> Completely drop this and rely on the settings done by the firmware.
>
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>

Reviewed-by: Julien Grall <julien.grall@arm.com>

Regards,

> ---
>   xen/drivers/char/scif-uart.c | 40 ----------------------------------------
>   1 file changed, 40 deletions(-)
>
> diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
> index bc157fe..f9ae257 100644
> --- a/xen/drivers/char/scif-uart.c
> +++ b/xen/drivers/char/scif-uart.c
> @@ -41,7 +41,6 @@
>   #define scif_writew(uart, off, val)    writew((val), (uart)->regs + (off))
>
>   static struct scif_uart {
> -    unsigned int data_bits, parity, stop_bits;
>       unsigned int irq;
>       char __iomem *regs;
>       struct irqaction irqaction;
> @@ -87,7 +86,6 @@ 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;
> -    uint16_t val;
>
>       /*
>        * Wait until last bit has been transmitted. This is needed for a smooth
> @@ -107,40 +105,6 @@ static void __init scif_uart_init_preirq(struct serial_port *port)
>       scif_readw(uart, SCIF_SCLSR);
>       scif_writew(uart, SCIF_SCLSR, 0);
>
> -    /* Select Baud rate generator output as a clock source */
> -    scif_writew(uart, SCIF_SCSCR, SCSCR_CKE10);
> -
> -    /* Setup protocol format and Baud rate, select Asynchronous mode */
> -    val = 0;
> -    ASSERT( uart->data_bits >= 7 && uart->data_bits <= 8 );
> -    if ( uart->data_bits == 7 )
> -        val |= SCSMR_CHR;
> -    else
> -        val &= ~SCSMR_CHR;
> -
> -    ASSERT( uart->stop_bits >= 1 && uart->stop_bits <= 2 );
> -    if ( uart->stop_bits == 2 )
> -        val |= SCSMR_STOP;
> -    else
> -        val &= ~SCSMR_STOP;
> -
> -    ASSERT( uart->parity >= PARITY_NONE && uart->parity <= PARITY_ODD );
> -    switch ( uart->parity )
> -    {
> -    case PARITY_NONE:
> -        val &= ~SCSMR_PE;
> -        break;
> -
> -    case PARITY_EVEN:
> -        val |= SCSMR_PE;
> -        break;
> -
> -    case PARITY_ODD:
> -        val |= SCSMR_PE | SCSMR_ODD;
> -        break;
> -    }
> -    scif_writew(uart, SCIF_SCSMR, val);
> -
>       /* Setup trigger level for TX/RX FIFOs */
>       scif_writew(uart, SCIF_SCFCR, SCFCR_RTRG11 | SCFCR_TTRG11);
>
> @@ -283,10 +247,6 @@ static int __init scif_uart_init(struct dt_device_node *dev,
>
>       uart = &scif_com;
>
> -    uart->data_bits = 8;
> -    uart->parity    = PARITY_NONE;
> -    uart->stop_bits = 1;
> -
>       res = dt_device_get_address(dev, 0, &addr, &size);
>       if ( res )
>       {
>

-- 
Julien Grall

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

Re: [PATCH v3 1/2] xen/arm: drivers: scif: Remove dead code

[Wei Liu]

On Thu, Jun 23, 2016 at 04:09:39PM +0100, Julien Grall wrote:
> (CC "THE REST" maintainers)
> 
> Hi Dirk,
> 
> On 22/06/16 12:49, Dirk Behme wrote:
> >The two struct members baud and clock_hz are in the end read only
> >variables nowhere used for anything useful. Removing them makes
> >the code much simpler without changing any functionality.
> >
> >Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> 
> Note that technically this driver is supported by "THE REST" maintainers. It
> might be worth to add it under "ARM (W/ VIRTUALISATION EXTENSIONS)
> ARCHITECTURE" as it is already the case for the other ARM specific UART
> drivers.
> 

FWIW this sounds reasonable to me.

Wei.

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