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

[Oleksandr Tyshchenko <oleksandr.tyshchenko@xxxxxxxxxxxxxxx>]

On Thu, Jan 22, 2015 at 6:18 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> On 22/01/15 16:04, Oleksandr Tyshchenko wrote:
>>
>>
>> On Thu, Jan 22, 2015 at 4:44 PM, Julien Grall <julien.grall@xxxxxxxxxx
>> <mailto:julien.grall@xxxxxxxxxx>> 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.
I understand, then I will implement local delay func in uart driver
based on READ_SYSREG64(CNTPCT_EL0).

>
> Regards,
>
> --
> Julien Grall



-- 

Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com

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