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

[Julien Grall <julien.grall@arm.com>]

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