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

[Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5652 bytes --]

On Thu, Jan 22, 2015 at 4:44 PM, Julien Grall <julien.grall@linaro.org>
wrote:
>
> Hi Iurii,

Hi Julien,
It's Oleksandr. Thank you for your comments.

>
>
> 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).

Am I right that you are speaking about this patch "[PATCH v1] xen/arm:
Manage pl011 uart TX interrupt correctly"?
http://www.gossamer-threads.com/lists/xen/devel/359033

I will rewrite code to use these callbacks.

>
>
> [..]
>
> > +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?

I don't think so. We just waiting for transmission to end. But anyway, I
can break this loop by timeout.

>
> [..]
>
> > +    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?

Ah, I just recollected about this. This is due to driver get stucks in
udelay().
http://lists.xen.org/archives/html/xen-devel/2014-10/msg01935.html

Now, we come from U-Boot with arch timer enabled.
I will try your suggestion (about moving init_xen_time() before
console_init_preirq()) again.
I hope that we can uncomment this call.

>
> [..]
>
> > +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.

ok.

>
> [..]
>
> > +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 )

Sounds reasonable. ok.

>         ...
>
> Regards,
>
> --
> Julien Grall

-- 

Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com

[-- Attachment #1.2: Type: text/html, Size: 8042 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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