Re: [RFC PATCH] xen/arm: Manage uart TX interrupt correctly

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

Hi Vijay,

You are fixing the pl011 driver, not all the UART. So the commit title
should at least contain the word "pl011".

On 06/12/14 01:42, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> On pl011.c when TX interrupt is received and

Why do you give the filename rather than the UART?

> TX buffer is empty, TX interrupt is not disabled and
> hence UART interrupt routine see TX interrupt always
> in MIS register and cpu loops infinitly.

I'm sorry to say that, but this sentence is difficult to understand.

> With this patch, mask and umask TX interrupt

s/umask/unmask

> when required

You need to explain when it's required...

> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
>  xen/drivers/char/pl011.c  |   18 ++++++++++++++++++
>  xen/drivers/char/serial.c |   30 +++++++++++++++++++++++++++++-
>  xen/include/xen/serial.h  |    4 ++++
>  3 files changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
> index dd19ce8..ad48df3 100644
> --- a/xen/drivers/char/pl011.c
> +++ b/xen/drivers/char/pl011.c
> @@ -109,6 +109,8 @@ static void __init pl011_init_preirq(struct serial_port *port)
>              panic("pl011: No Baud rate configured\n");
>          uart->baud = (uart->clock_hz << 2) / divisor;
>      }
> +    /* Trigger RX interrupt at 1/2 full, TX interrupt at 7/8 empty */
> +    pl011_write(uart, IFLS, (2<<3 | 0));

The RX change seems to come from nowhere. You have to explain why you
need it in the commit message.

As you add start_tx/stop_tx, I don't think this has to be kept.

[..]

> diff --git a/xen/drivers/char/serial.c b/xen/drivers/char/serial.c
> index 44026b1..d2ce8a8 100644
> --- a/xen/drivers/char/serial.c
> +++ b/xen/drivers/char/serial.c
> @@ -76,6 +76,19 @@ void serial_tx_interrupt(struct serial_port *port, struct cpu_user_regs *regs)
>          cpu_relax();
>      }
>  
> +    if ( port->txbufc == port->txbufp )
> +    {
> +        /* Disable TX. nothing to send */
> +        if ( port->driver->stop_tx != NULL )
> +            port->driver->stop_tx(port);

Can you introduce helpers for both stop_tx and start_tx? It would avoid
to add if ( ... ) port->driver->...( ) every time.

Regards,

-- 
Julien Grall