Re: [PATCH 1/1] serial: omap-serial: Add support for kernel debugger

[Cosmin Cojocar <cosmin.cojocar@gmail.com>]

Hi Ionut,

Ionut Nicu wrote:
> Hi Cosmin,
> 
> On Sat, 2010-11-20 at 21:51 +0100, Cosmin Cojocar wrote:
>> The kgdb invokes the poll_put_char and poll_get_char when communicating
>> with the host. This patch also changes the initialization order because the
>> kgdb will check at the very beginning, if there is a valid serial
>> driver.
>>
>> Signed-off-by: Cosmin Cojocar <cosmin.cojocar@gmail.com>
>> ---
>>  drivers/serial/Makefile      |    2 +-
>>  drivers/serial/omap-serial.c |   38 ++++++++++++++++++++++++++++++++++++--
>>  2 files changed, 37 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
>> index c570576..ad86113 100644
>> --- a/drivers/serial/Makefile
>> +++ b/drivers/serial/Makefile
>> @@ -80,6 +80,7 @@ obj-$(CONFIG_SERIAL_NETX) += netx-serial.o
>>  obj-$(CONFIG_SERIAL_OF_PLATFORM) += of_serial.o
>>  obj-$(CONFIG_SERIAL_OF_PLATFORM_NWPSERIAL) += nwpserial.o
>>  obj-$(CONFIG_SERIAL_KS8695) += serial_ks8695.o
>> +obj-$(CONFIG_SERIAL_OMAP) += omap-serial.o
>>  obj-$(CONFIG_KGDB_SERIAL_CONSOLE) += kgdboc.o
>>  obj-$(CONFIG_SERIAL_QE) += ucc_uart.o
>>  obj-$(CONFIG_SERIAL_TIMBERDALE)	+= timbuart.o
>> @@ -88,4 +89,3 @@ obj-$(CONFIG_SERIAL_ALTERA_JTAGUART) += altera_jtaguart.o
>>  obj-$(CONFIG_SERIAL_ALTERA_UART) += altera_uart.o
>>  obj-$(CONFIG_SERIAL_MRST_MAX3110)	+= mrst_max3110.o
>>  obj-$(CONFIG_SERIAL_MFD_HSU)	+= mfd.o
>> -obj-$(CONFIG_SERIAL_OMAP) += omap-serial.o
>> diff --git a/drivers/serial/omap-serial.c b/drivers/serial/omap-serial.c
>> index 03a96db..df6ba03 100644
>> --- a/drivers/serial/omap-serial.c
>> +++ b/drivers/serial/omap-serial.c
>> @@ -866,14 +866,44 @@ serial_omap_type(struct uart_port *port)
>>  	return up->name;
>>  }
>>  
>> +#define BOTH_EMPTY (UART_LSR_TEMT | UART_LSR_THRE)
>> +
>> +#ifdef CONFIG_CONSOLE_POLL
>> +static void serial_omap_poll_put_char(struct uart_port *port, unsigned char ch)
>> +{
>> +	struct uart_omap_port *up = (struct uart_omap_port *)port;
>> +	unsigned int status;
>> +
>> +	do {
>> +		status = serial_in(up, UART_LSR);
>> +		udelay(1);
>> +	} while ((status & BOTH_EMPTY) != BOTH_EMPTY);
>> +
> 
> Shouldn't you use wait_for_xmitr() instead? If you do that, then you
> don't need to move the BOTH_EMPTY macro anymore.
> 
Initially, I had used the wait_for_xmitr, but the gdb couldn't establish
a connection. I will retest it.

>> +	serial_out(up, UART_TX, ch);
>> +}
>> +
>> +static int serial_omap_poll_get_char(struct uart_port *port)
>> +{
>> +	struct uart_omap_port *up = (struct uart_omap_port *)port;
>> +	unsigned int status;
>> +	int ch;
>> +
>> +	do {
>> +		status = serial_in(up, UART_LSR);
>> +		udelay(1);
>> +	} while ((status & UART_LSR_DR) != UART_LSR_DR);
>> +
> 
> I think you should be returning NO_POLL_CHAR if the condition is false,
> instead of busy looping.
> 
>> +	ch = (int)serial_in(up, UART_RX);
>> +	return ch;
>> +}
> 
> I don't think you need the extra ch variable. Just return serial_in(up,
> UART_RX).
> 
Here, I agree with your comments.

> Cheers,
> Ionut.
> 
> 

Regards,
Cosmin