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

[Julien Grall <julien.grall@linaro.org>]

On 22/01/15 16:04, Oleksandr Tyshchenko wrote:
> 
> 
> On Thu, Jan 22, 2015 at 4:44 PM, Julien Grall <julien.grall@linaro.org
> <mailto:julien.grall@linaro.org>> wrote:

> Hi Julien,

Hi Oleksandr,

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

Yes. FYI, Ian is planning to cherry-pick in Xen 4.5.

> I will rewrite code to use these callbacks.

Thanks!

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

It's not necessary to move to a timeout. We have other place with such
loop (see most UART interrupt handlers).

I should have add OOI before my question.

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

I though about it, and I don't think we should move init_xen_time early.
It's depends to platform_init() and processor_id(). Although, we can
remove the processor_id().

My main concern is we will print lots useful message with early printk.

If early printk is not available (for example when debug=n), we will
loose those messages.

Unless we find a way to keep those messages until the console is
initialize, we should not move xen_init_time earlier.

Regards,

-- 
Julien Grall