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

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

[Dirk Behme]

In scif_uart_init() uart->baud is set to BAUD_AUTO. So its a basic error
if this is different later. Detect this by an ASSERT, but remove the
dead code normally never reached.

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

diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
index 51a2233..ca88c0f 100644
--- a/xen/drivers/char/scif-uart.c
+++ b/xen/drivers/char/scif-uart.c
@@ -143,23 +143,12 @@ 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);
-    }
+    ASSERT( uart->baud == BAUD_AUTO );
+
+    /* 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);
-- 
2.8.0


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

[PATCH 2/3] xen/arm: drivers: scif: Remove unused variables

[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    | 13 +------------
 xen/include/asm-arm/scif-uart.h |  1 -
 2 files changed, 1 insertion(+), 13 deletions(-)

diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
index ca88c0f..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,14 +141,6 @@ static void __init scif_uart_init_preirq(struct serial_port *port)
     }
     scif_writew(uart, SCIF_SCSMR, val);
 
-    ASSERT( uart->clock_hz > 0 );
-    ASSERT( uart->baud == BAUD_AUTO );
-
-    /* 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);
 
@@ -292,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 3/3] xen/arm: drivers: scif: Add clock auto detection

[Dirk Behme]

Besides the 14MHz external clock, the SCIF might be clocked by an
internal 66MHz clock. Detect this clock based on the SCIF_DL register
being 0 (internal clock) or != 0 (external clock).

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

diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
index bc157fe..678f46b 100644
--- a/xen/drivers/char/scif-uart.c
+++ b/xen/drivers/char/scif-uart.c
@@ -107,8 +107,19 @@ 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);
+    /*
+     * Select Baud rate generator output as a clock source
+     * The clock source can be an internal or external clock.
+     * Depending on this the DL register is either 0 or contains
+     * the divisor. I.e. we can use this to detect the clock
+     * source and based on this can configure the CKE[1:0] bits
+     * of the SCSCR register.
+     */
+    if ( scif_readw(uart, SCIF_DL) )
+        scif_writew(uart, SCIF_SCSCR, SCSCR_CKE10); /* External clk */
+    else
+        scif_writew(uart, SCIF_SCSCR, SCSCR_CKE00); /* Internal clk */
+
 
     /* Setup protocol format and Baud rate, select Asynchronous mode */
     val = 0;
-- 
2.8.0


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

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

[Oleksandr Tyshchenko]

Hi, Dirk.

On Tue, Jun 21, 2016 at 12:15 PM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
> In scif_uart_init() uart->baud is set to BAUD_AUTO. So its a basic error
> if this is different later. Detect this by an ASSERT, but remove the
> dead code normally never reached.
>
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> ---
>  xen/drivers/char/scif-uart.c | 23 ++++++-----------------
>  1 file changed, 6 insertions(+), 17 deletions(-)
>
> diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
> index 51a2233..ca88c0f 100644
> --- a/xen/drivers/char/scif-uart.c
> +++ b/xen/drivers/char/scif-uart.c
> @@ -143,23 +143,12 @@ 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);

This part of code might be used for people who are not satisfied with
default baudrate which has been set in U-Boot.
If we remove this we will take away the opportunity to just change
uart->baud from BAUD_AUTO to desired one.

> -    }
> -    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);
> -    }
> +    ASSERT( uart->baud == BAUD_AUTO );
> +
> +    /* 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);
> --
> 2.8.0
>



-- 

Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com

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

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

[Julien Grall]

On 21/06/16 11:16, Oleksandr Tyshchenko wrote:
> Hi, Dirk.

Hello Oleksandr,

> On Tue, Jun 21, 2016 at 12:15 PM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
>> In scif_uart_init() uart->baud is set to BAUD_AUTO. So its a basic error
>> if this is different later. Detect this by an ASSERT, but remove the
>> dead code normally never reached.
>>
>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>> ---
>>   xen/drivers/char/scif-uart.c | 23 ++++++-----------------
>>   1 file changed, 6 insertions(+), 17 deletions(-)
>>
>> diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
>> index 51a2233..ca88c0f 100644
>> --- a/xen/drivers/char/scif-uart.c
>> +++ b/xen/drivers/char/scif-uart.c
>> @@ -143,23 +143,12 @@ 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);
>
> This part of code might be used for people who are not satisfied with
> default baudrate which has been set in U-Boot.

Can you elaborate? If the baud rate is different, it will not be 
possible to get output from U-boot.

> If we remove this we will take away the opportunity to just change
> uart->baud from BAUD_AUTO to desired one.

I have some doubt that the current code is valid. The clock frequency is 
hardcoded (see SCIF_CLK_FREQ), so are you saying that the frequency is 
always the same across all the platforms?

I would rather avoid to keep dead code (or not accessible without 
hacking Xen). For what is worth, we recently removed a similar code from 
the PL011 driver as this should be correctly configured by the firmware.

>> -    }
>> -    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);
>> -    }
>> +    ASSERT( uart->baud == BAUD_AUTO );
>> +
>> +    /* 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);
>> --
>> 2.8.0
>>

Regards,

-- 
Julien Grall

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

Re: [PATCH 2/3] xen/arm: drivers: scif: Remove unused variables

[Julien Grall]

Hello Dirk,

On 21/06/16 10:15, 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.

 From my understanding, this patch is removing code you just added on 
the previous patch. I would prefer if you squash this patch into #1.

Regards,

>
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> ---
>   xen/drivers/char/scif-uart.c    | 13 +------------
>   xen/include/asm-arm/scif-uart.h |  1 -
>   2 files changed, 1 insertion(+), 13 deletions(-)
>
> diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
> index ca88c0f..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,14 +141,6 @@ static void __init scif_uart_init_preirq(struct serial_port *port)
>       }
>       scif_writew(uart, SCIF_SCSMR, val);
>
> -    ASSERT( uart->clock_hz > 0 );
> -    ASSERT( uart->baud == BAUD_AUTO );
> -
> -    /* 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);
>
> @@ -292,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 3/3] xen/arm: drivers: scif: Add clock auto detection

[Julien Grall]

Hello Dirk,

On 21/06/16 10:15, Dirk Behme wrote:
> Besides the 14MHz external clock, the SCIF might be clocked by an
> internal 66MHz clock. Detect this clock based on the SCIF_DL register
> being 0 (internal clock) or != 0 (external clock).

Do you have a public link to the specification?

> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> ---
>   xen/drivers/char/scif-uart.c | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
> index bc157fe..678f46b 100644
> --- a/xen/drivers/char/scif-uart.c
> +++ b/xen/drivers/char/scif-uart.c
> @@ -107,8 +107,19 @@ 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);
> +    /*
> +     * Select Baud rate generator output as a clock source
> +     * The clock source can be an internal or external clock.
> +     * Depending on this the DL register is either 0 or contains
> +     * the divisor. I.e. we can use this to detect the clock
> +     * source and based on this can configure the CKE[1:0] bits
> +     * of the SCSCR register.
> +     */
> +    if ( scif_readw(uart, SCIF_DL) )
> +        scif_writew(uart, SCIF_SCSCR, SCSCR_CKE10); /* External clk */
> +    else
> +        scif_writew(uart, SCIF_SCSCR, SCSCR_CKE00); /* Internal clk */

Why would we need to select the baud rate generator if the baud has been 
configured by the firmware?

> +

Please drop this newline.

>
>       /* Setup protocol format and Baud rate, select Asynchronous mode */
>       val = 0;
>

Regards,

-- 
Julien Grall

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

Re: [PATCH 3/3] xen/arm: drivers: scif: Add clock auto detection

[Dirk Behme]

Hi Julien,

On 21.06.2016 14:20, Julien Grall wrote:
> Hello Dirk,
>
> On 21/06/16 10:15, Dirk Behme wrote:
>> Besides the 14MHz external clock, the SCIF might be clocked by an
>> internal 66MHz clock. Detect this clock based on the SCIF_DL register
>> being 0 (internal clock) or != 0 (external clock).
>
> Do you have a public link to the specification?


I have to check this ;)


>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>> ---
>>   xen/drivers/char/scif-uart.c | 15 +++++++++++++--
>>   1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
>> index bc157fe..678f46b 100644
>> --- a/xen/drivers/char/scif-uart.c
>> +++ b/xen/drivers/char/scif-uart.c
>> @@ -107,8 +107,19 @@ 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);
>> +    /*
>> +     * Select Baud rate generator output as a clock source
>> +     * The clock source can be an internal or external clock.
>> +     * Depending on this the DL register is either 0 or contains
>> +     * the divisor. I.e. we can use this to detect the clock
>> +     * source and based on this can configure the CKE[1:0] bits
>> +     * of the SCSCR register.
>> +     */
>> +    if ( scif_readw(uart, SCIF_DL) )
>> +        scif_writew(uart, SCIF_SCSCR, SCSCR_CKE10); /* External clk */
>> +    else
>> +        scif_writew(uart, SCIF_SCSCR, SCSCR_CKE00); /* Internal clk */
>
> Why would we need to select the baud rate generator if the baud has been
> configured by the firmware?


Just to get the correct understanding: The proposal is to just remove 
the code which (wrongly) overwrites the correct settings done by the 
firmware?

I.e. instead of doing the same thing the firmware is already doing, 
again (the if .. else ), the proposal is simply dropping the


  -    /* Select Baud rate generator output as a clock source */
  -    scif_writew(uart, SCIF_SCSCR, SCSCR_CKE10);

?


Best regards

Dirk



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

Re: [PATCH 3/3] xen/arm: drivers: scif: Add clock auto detection

[Julien Grall]

On 21/06/16 13:30, Dirk Behme wrote:
>>> diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
>>> index bc157fe..678f46b 100644
>>> --- a/xen/drivers/char/scif-uart.c
>>> +++ b/xen/drivers/char/scif-uart.c
>>> @@ -107,8 +107,19 @@ 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);
>>> +    /*
>>> +     * Select Baud rate generator output as a clock source
>>> +     * The clock source can be an internal or external clock.
>>> +     * Depending on this the DL register is either 0 or contains
>>> +     * the divisor. I.e. we can use this to detect the clock
>>> +     * source and based on this can configure the CKE[1:0] bits
>>> +     * of the SCSCR register.
>>> +     */
>>> +    if ( scif_readw(uart, SCIF_DL) )
>>> +        scif_writew(uart, SCIF_SCSCR, SCSCR_CKE10); /* External clk */
>>> +    else
>>> +        scif_writew(uart, SCIF_SCSCR, SCSCR_CKE00); /* Internal clk */
>>
>> Why would we need to select the baud rate generator if the baud has been
>> configured by the firmware?
>
>
> Just to get the correct understanding: The proposal is to just remove
> the code which (wrongly) overwrites the correct settings done by the
> firmware?
>
> I.e. instead of doing the same thing the firmware is already doing,
> again (the if .. else ), the proposal is simply dropping the
>
>
>   -    /* Select Baud rate generator output as a clock source */
>   -    scif_writew(uart, SCIF_SCSCR, SCSCR_CKE10);
>
> ?

Yes. However I don't have any spec in hand so I am not sure if it is 
correct.

Regards,

-- 
Julien Grall

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

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

[Oleksandr Tyshchenko]

On Tue, Jun 21, 2016 at 3:15 PM, Julien Grall <julien.grall@arm.com> wrote:
>
>
> On 21/06/16 11:16, Oleksandr Tyshchenko wrote:
>>
>> Hi, Dirk.
>
>
> Hello Oleksandr,

Hello Julien.
>
>
>> On Tue, Jun 21, 2016 at 12:15 PM, Dirk Behme <dirk.behme@de.bosch.com>
>> wrote:
>>>
>>> In scif_uart_init() uart->baud is set to BAUD_AUTO. So its a basic error
>>> if this is different later. Detect this by an ASSERT, but remove the
>>> dead code normally never reached.
>>>
>>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>>> ---
>>>   xen/drivers/char/scif-uart.c | 23 ++++++-----------------
>>>   1 file changed, 6 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
>>> index 51a2233..ca88c0f 100644
>>> --- a/xen/drivers/char/scif-uart.c
>>> +++ b/xen/drivers/char/scif-uart.c
>>> @@ -143,23 +143,12 @@ 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);
>>
>>
>> This part of code might be used for people who are not satisfied with
>> default baudrate which has been set in U-Boot.
>
>
> Can you elaborate? If the baud rate is different, it will not be possible to
> get output from U-boot.
>
>> If we remove this we will take away the opportunity to just change
>> uart->baud from BAUD_AUTO to desired one.
>
>
> I have some doubt that the current code is valid. The clock frequency is
> hardcoded (see SCIF_CLK_FREQ), so are you saying that the frequency is
> always the same across all the platforms?

No.

This driver has been initially written for Renesas Lager board based
on R-Car H2 SoC which
has SCIF compatible UART. And the current code is valid for it. I have
tested both auto and
variable (38400, 115200) baudrates.
But, I am afraid that current code won't be suitable for
all of the boards which have the same UART IP. It depends at least
from clock source
(external/internal) and clock frequency.

>
> I would rather avoid to keep dead code (or not accessible without hacking
> Xen). For what is worth, we recently removed a similar code from the PL011
> driver as this should be correctly configured by the firmware.
>
>>> -    }
>>> -    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);
>>> -    }
>>> +    ASSERT( uart->baud == BAUD_AUTO );
>>> +
>>> +    /* 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);
>>> --
>>> 2.8.0
>>>
>
> Regards,
>
> --
> Julien Grall



-- 

Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com

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

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

[Dirk Behme]

Hello Oleksandr,

On 21.06.2016 14:54, Oleksandr Tyshchenko wrote:
> On Tue, Jun 21, 2016 at 3:15 PM, Julien Grall <julien.grall@arm.com> wrote:
>>
>>
>> On 21/06/16 11:16, Oleksandr Tyshchenko wrote:
>>>
>>> Hi, Dirk.
>>
>>
>> Hello Oleksandr,
>
> Hello Julien.
>>
>>
>>> On Tue, Jun 21, 2016 at 12:15 PM, Dirk Behme <dirk.behme@de.bosch.com>
>>> wrote:
>>>>
>>>> In scif_uart_init() uart->baud is set to BAUD_AUTO. So its a basic error
>>>> if this is different later. Detect this by an ASSERT, but remove the
>>>> dead code normally never reached.
>>>>
>>>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>>>> ---
>>>>   xen/drivers/char/scif-uart.c | 23 ++++++-----------------
>>>>   1 file changed, 6 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
>>>> index 51a2233..ca88c0f 100644
>>>> --- a/xen/drivers/char/scif-uart.c
>>>> +++ b/xen/drivers/char/scif-uart.c
>>>> @@ -143,23 +143,12 @@ 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);
>>>
>>>
>>> This part of code might be used for people who are not satisfied with
>>> default baudrate which has been set in U-Boot.
>>
>>
>> Can you elaborate? If the baud rate is different, it will not be possible to
>> get output from U-boot.
>>
>>> If we remove this we will take away the opportunity to just change
>>> uart->baud from BAUD_AUTO to desired one.
>>
>>
>> I have some doubt that the current code is valid. The clock frequency is
>> hardcoded (see SCIF_CLK_FREQ), so are you saying that the frequency is
>> always the same across all the platforms?
>
> No.
>
> This driver has been initially written for Renesas Lager board based
> on R-Car H2 SoC which
> has SCIF compatible UART. And the current code is valid for it. I have
> tested both auto and
> variable (38400, 115200) baudrates.


Could you help me a little to understand this?

The driver has

scif_uart_init()
{
...
struct scif_uart *uart;
...
uart->baud      = BAUD_AUTO;
...
}

I checked the code for struct scif_uart but it isn't used anywhere 
outside this driver. So uart->baud is a driver local variable, which 
looks to me isn't used anywhere useful.

What have I missed?

Best regards

Dirk


> But, I am afraid that current code won't be suitable for
> all of the boards which have the same UART IP. It depends at least
> from clock source
> (external/internal) and clock frequency.
>
>>
>> I would rather avoid to keep dead code (or not accessible without hacking
>> Xen). For what is worth, we recently removed a similar code from the PL011
>> driver as this should be correctly configured by the firmware.
>>
>>>> -    }
>>>> -    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);
>>>> -    }
>>>> +    ASSERT( uart->baud == BAUD_AUTO );
>>>> +
>>>> +    /* 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);
>>>> --
>>>> 2.8.0
>>>>
>>
>> Regards,

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

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

[Julien Grall]

On 21/06/16 13:54, Oleksandr Tyshchenko wrote:
> On Tue, Jun 21, 2016 at 3:15 PM, Julien Grall <julien.grall@arm.com> wrote:
>>> On Tue, Jun 21, 2016 at 12:15 PM, Dirk Behme <dirk.behme@de.bosch.com>
>>> wrote:
>> I have some doubt that the current code is valid. The clock frequency is
>> hardcoded (see SCIF_CLK_FREQ), so are you saying that the frequency is
>> always the same across all the platforms?
>
> No.
>
> This driver has been initially written for Renesas Lager board based
> on R-Car H2 SoC which
> has SCIF compatible UART. And the current code is valid for it. I have
> tested both auto and
> variable (38400, 115200) baudrates.
> But, I am afraid that current code won't be suitable for
> all of the boards which have the same UART IP. It depends at least
> from clock source
> (external/internal) and clock frequency.

In this case, the code should either be fixed by reading the clock 
frequency from the firmware tables or be dropped.

I would lean towards the later because the user cannot specify the baud 
rate to Xen.

Regards,

-- 
Julien Grall

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

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

[Oleksandr Tyshchenko]

On Tue, Jun 21, 2016 at 4:07 PM, Julien Grall <julien.grall@arm.com> wrote:
>
>
> On 21/06/16 13:54, Oleksandr Tyshchenko wrote:
>>
>> On Tue, Jun 21, 2016 at 3:15 PM, Julien Grall <julien.grall@arm.com>
>> wrote:
>>>>
>>>> On Tue, Jun 21, 2016 at 12:15 PM, Dirk Behme <dirk.behme@de.bosch.com>
>>>> wrote:
>>>
>>> I have some doubt that the current code is valid. The clock frequency is
>>> hardcoded (see SCIF_CLK_FREQ), so are you saying that the frequency is
>>> always the same across all the platforms?
>>
>>
>> No.
>>
>> This driver has been initially written for Renesas Lager board based
>> on R-Car H2 SoC which
>> has SCIF compatible UART. And the current code is valid for it. I have
>> tested both auto and
>> variable (38400, 115200) baudrates.
>> But, I am afraid that current code won't be suitable for
>> all of the boards which have the same UART IP. It depends at least
>> from clock source
>> (external/internal) and clock frequency.
>
>
> In this case, the code should either be fixed by reading the clock frequency
> from the firmware tables or be dropped.
>
> I would lean towards the later because the user cannot specify the baud rate
> to Xen.
Agree.

>
> Regards,
>
> --
> Julien Grall



-- 

Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com

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

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

[Oleksandr Tyshchenko]

On Tue, Jun 21, 2016 at 4:01 PM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
> Hello Oleksandr,
>
>
> On 21.06.2016 14:54, Oleksandr Tyshchenko wrote:
>>
>> On Tue, Jun 21, 2016 at 3:15 PM, Julien Grall <julien.grall@arm.com>
>> wrote:
>>>
>>>
>>>
>>> On 21/06/16 11:16, Oleksandr Tyshchenko wrote:
>>>>
>>>>
>>>> Hi, Dirk.
>>>
>>>
>>>
>>> Hello Oleksandr,
>>
>>
>> Hello Julien.
>>>
>>>
>>>
>>>> On Tue, Jun 21, 2016 at 12:15 PM, Dirk Behme <dirk.behme@de.bosch.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> In scif_uart_init() uart->baud is set to BAUD_AUTO. So its a basic
>>>>> error
>>>>> if this is different later. Detect this by an ASSERT, but remove the
>>>>> dead code normally never reached.
>>>>>
>>>>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>>>>> ---
>>>>>   xen/drivers/char/scif-uart.c | 23 ++++++-----------------
>>>>>   1 file changed, 6 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/xen/drivers/char/scif-uart.c
>>>>> b/xen/drivers/char/scif-uart.c
>>>>> index 51a2233..ca88c0f 100644
>>>>> --- a/xen/drivers/char/scif-uart.c
>>>>> +++ b/xen/drivers/char/scif-uart.c
>>>>> @@ -143,23 +143,12 @@ 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);
>>>>
>>>>
>>>>
>>>> This part of code might be used for people who are not satisfied with
>>>> default baudrate which has been set in U-Boot.
>>>
>>>
>>>
>>> Can you elaborate? If the baud rate is different, it will not be possible
>>> to
>>> get output from U-boot.
>>>
>>>> If we remove this we will take away the opportunity to just change
>>>> uart->baud from BAUD_AUTO to desired one.
>>>
>>>
>>>
>>> I have some doubt that the current code is valid. The clock frequency is
>>> hardcoded (see SCIF_CLK_FREQ), so are you saying that the frequency is
>>> always the same across all the platforms?
>>
>>
>> No.
>>
>> This driver has been initially written for Renesas Lager board based
>> on R-Car H2 SoC which
>> has SCIF compatible UART. And the current code is valid for it. I have
>> tested both auto and
>> variable (38400, 115200) baudrates.
>
>
>
> Could you help me a little to understand this?
>
> The driver has
>
> scif_uart_init()
> {
> ...
> struct scif_uart *uart;
> ...
> uart->baud      = BAUD_AUTO;
> ...
> }
>
> I checked the code for struct scif_uart but it isn't used anywhere outside
> this driver. So uart->baud is a driver local variable, which looks to me
> isn't used anywhere useful.
>
> What have I missed?

Unfortunately, I don't have recent Xen sources right now to be 100%
sure. But, it seems you are right: uart->baud is a driver local
variable.

>
> Best regards
>
> Dirk
>
>
>
>> But, I am afraid that current code won't be suitable for
>> all of the boards which have the same UART IP. It depends at least
>> from clock source
>> (external/internal) and clock frequency.
>>
>>>
>>> I would rather avoid to keep dead code (or not accessible without hacking
>>> Xen). For what is worth, we recently removed a similar code from the
>>> PL011
>>> driver as this should be correctly configured by the firmware.
>>>
>>>>> -    }
>>>>> -    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);
>>>>> -    }
>>>>> +    ASSERT( uart->baud == BAUD_AUTO );
>>>>> +
>>>>> +    /* 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);
>>>>> --
>>>>> 2.8.0
>>>>>
>>>
>>> Regards,



-- 

Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com

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