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

[Julien Grall <julien.grall@xxxxxxx>]

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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel