Re: [Xen-devel] [PATCH v2 2/3] xen/arm: Add new driver for R-Car Gen2 UART

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Iurii,

On 21/01/15 14:16, Iurii Konovalenko wrote:
> diff --git a/xen/drivers/char/rcar2-uart.c b/xen/drivers/char/rcar2-uart.c
> +static void rcar2_uart_interrupt(int irq, void *data, struct cpu_user_regs 
> *regs)
> +{
> +    struct serial_port *port = data;
> +    struct rcar2_uart *uart = port->uart;
> +    uint16_t status, ctrl;
> +
> +    ctrl = rcar2_readw(uart, SCIF_SCSCR);
> +    status = rcar2_readw(uart, SCIF_SCFSR) & ~SCFSR_TEND;
> +    /* Ignore next flag if TX Interrupt is disabled */
> +    if ( !(ctrl & SCSCR_TIE) )
> +        status &= ~SCFSR_TDFE;
> +
> +    while ( status != 0 )
> +    {
> +        /* TX Interrupt */
> +        if ( status & SCFSR_TDFE )
> +        {
> +            serial_tx_interrupt(port, regs);
> +
> +            if ( port->txbufc == port->txbufp )
> +            {
> +                /*
> +                 * There is no data bytes to send. We have to disable
> +                 * TX Interrupt to prevent us from getting stuck in the
> +                 * interrupt handler
> +                 */
> +                ctrl = rcar2_readw(uart, SCIF_SCSCR);
> +                ctrl &= ~SCSCR_TIE;
> +                rcar2_writew(uart, SCIF_SCSCR, ctrl);
> +            }

serial_start_tx and serial_stop_tx callback have been recently
introduced to start/stop transmission.

I think you could use them to replace this if (and maybe some others).

[..]

> +static void __init rcar2_uart_init_preirq(struct serial_port *port)
> +{
> +    struct rcar2_uart *uart = port->uart;
> +    unsigned int divisor;
> +    uint16_t val;
> +
> +    /*
> +     * Wait until last bit has been transmitted. This is needed for a smooth
> +     * transition when we come from early printk
> +     */
> +    while ( !(rcar2_readw(uart, SCIF_SCFSR) & SCFSR_TEND) );

Would it be possible that it get stucks forever?

[..]

> +    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 );
> +        rcar2_writew(uart, SCIF_DL, (uint16_t)divisor);
> +        /* Selects the frequency divided clock (SC_CLK external input) */
> +        rcar2_writew(uart, SCIF_CKS, 0);
> +        /*
> +         * TODO: should be uncommented
> +         * udelay(1000000 / uart->baud + 1);
> +         */

Why didn't you uncommented?

[..]

> +static int rcar2_uart_tx_ready(struct serial_port *port)
> +{
> +    struct rcar2_uart *uart = port->uart;
> +    uint16_t cnt;
> +
> +    /* Check for empty space in TX FIFO */
> +    if ( !(rcar2_readw(uart, SCIF_SCFSR) & SCFSR_TDFE) )
> +        return 0;
> +
> +    /*
> +     * It seems that the Framework has a data bytes to send.
> +     * Enable TX Interrupt disabled from interrupt handler before
> +     */
> +    if ( uart->irq_en )
> +        rcar2_writew(uart, SCIF_SCSCR, rcar2_readw(uart, SCIF_SCSCR) |
> +                     SCSCR_TIE);

I think you can replace it by implementing the callback start_tx.

[..]

> +static int __init rcar2_uart_init(struct dt_device_node *dev,
> +                                 const void *data)
> +{
> +    const char *config = data;
> +    struct rcar2_uart *uart;
> +    int res;
> +    u64 addr, size;
> +
> +    if ( strcmp(config, "") )
> +        printk("WARNING: UART configuration is not supported\n");
> +
> +    uart = &rcar2_com;
> +
> +    uart->clock_hz  = SCIF_CLK_FREQ;
> +    uart->baud      = BAUD_AUTO;
> +    uart->data_bits = 8;
> +    uart->parity    = PARITY_NONE;
> +    uart->stop_bits = 1;
> +
> +    res = dt_device_get_address(dev, 0, &addr, &size);
> +    if ( res )
> +    {
> +        printk("rcar2-uart: Unable to retrieve the base"
> +                     " address of the UART\n");
> +        return res;
> +    }
> +
> +    uart->regs = ioremap_nocache(addr, size);
> +    if ( !uart->regs )
> +    {
> +        printk("rcar2-uart: Unable to map the UART memory\n");
> +        return -ENOMEM;
> +    }
> +
> +    res = platform_get_irq(dev, 0);
> +    if ( res < 0 )
> +    {
> +        printk("rcar2-uart: Unable to retrieve the IRQ\n");
> +        return res;

if platform_get_irq fails, the driver will leak the mapping made with
ioremap_cache. I would invert the 2 call:

res = platform_get_irq(dev, 0);
if ( res < 0 )
        ...

uart->regs = ioremap_nocache(addr, size);
if ( !uart->regs )
        ...

Regards,

-- 
Julien Grall

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