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 4:44 PM, Julien Grall <julien.grall@xxxxxxxxxx> 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel