From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [RFC PATCH] xen/arm: Manage uart TX interrupt correctly Date: Mon, 08 Dec 2014 13:47:09 +0000 Message-ID: <5485ABDD.10301@linaro.org> References: <1417830124-3914-1-git-send-email-vijay.kilari@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1417830124-3914-1-git-send-email-vijay.kilari@gmail.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: vijay.kilari@gmail.com, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, stefano.stabellini@citrix.com, tim@xen.org, xen-devel@lists.xen.org Cc: Prasun.Kapoor@caviumnetworks.com, vijaya.kumar@caviumnetworks.com, manish.jaggi@caviumnetworks.com List-Id: xen-devel@lists.xenproject.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 > > 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 > --- > 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