On Thu, Jan 22, 2015 at 4:44 PM, Julien Grall <julien.gr...@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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
