[U-Boot] [PATCH 2/2] Added serial loopback tests accessible via CLI and POST

[Wolfgang Denk]

Dear Michael Zaidman,

In message <1269267894-15324-1-git-send-email-michael.zaidman at gmail.com> you wrote:
> Serial loopback internal/external tests. Is based on my previous commit
> 078a9c4898e7802086b362baa44ad48b8ad1baed

This information is useless - no such commit ID is available in
mainline:

-> cd /home/git/u-boot
-> git show 078a9c4898e7
fatal: ambiguous argument '078a9c4898e7': unknown revision or path not in the working tree.

> Signed-off-by: Michael Zaidman <michael.zaidman at gmail.com>
> ---
>  drivers/serial/serial.c |   87 +++++++++++++++++++++++++++++++++++++++++++++++
>  post/drivers/Makefile   |    2 +-
>  post/drivers/serial.c   |   56 ++++++++++++++++++++++++++++++
>  3 files changed, 144 insertions(+), 1 deletions(-)
>  create mode 100644 post/drivers/serial.c
> 
> diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c
> index 0b30ff4..10b5ab1 100644
> --- a/drivers/serial/serial.c
> +++ b/drivers/serial/serial.c
> @@ -46,6 +46,7 @@
>  
>  #include <common.h>
>  #include <ns16550.h>
> +#include <asm/io.h>
>  
>  #ifdef CONFIG_NS87308
>  #include <ns87308.h>

There is no such code anywhere in mainline.

> +NS16550_t get_serial_port(int index)
> +{
> +	return serial_ports[index];
> +}
> +
> +int get_number_of_serial_ports(void)
> +{
> +	return MAX_SER_DEV;
> +}

You might want to add and use these helpers in the previous patch.

> +static int __serial_test (int com_port, int loopback_mode)
> +{
> +	int i, loops, ret = 0;
> +	NS16550_t port = serial_ports[com_port-1];
> +
> +	if (loopback_mode == 0) /* Set local loop-back mode */
> +		out_8(&port->mcr, (in_8(&port->mcr) | UART_MCR_LOOP));
> +
> +	for (i = 0; (i < 256) && !ret; i++) {
> +		NS16550_putc(port, i);
> +		for (loops = 1000; loops; loops --) {
> +			if (NS16550_tstc(port)) {
> +				if (NS16550_getc(port) != i)
> +					ret = 1;
> +				break;
> +			}
> +			udelay(100); /* tstc timeout = 1000 times * 100us */
> +		}
> +		if (loops == 0)
> +			ret = 1;
> +	}
> +
> +	if (loopback_mode == 0) /* Remove local loop-back mode */
> +		out_8(&port->mcr, (in_8(&port->mcr) & ~UART_MCR_LOOP));
> +
> +	return ret;
> +}
> +int serial_test(int com_port, int loopback_mode) __attribute__((weak, alias("__serial_test")));
> +
> +int do_serial_test (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
> +{
> +	int port;
> +	int mode = 0; /* local loop-back mode */
> +
> +	if (argc < 3) {
> +		printf("Err: Invalid number of arguments!\n\n");
> +		cmd_usage(cmdtp);
> +		return 1;
> +	}
> +
> +	if (strcmp(argv[1],"external") == 0) {
> +		mode = 1;
> +	}
> +	else
> +	if (strcmp(argv[1],"internal") != 0) {
> +		printf("Err: Invalid loop-back mode!\n\n");
> +		return 1;
> +	}
> +
> +	port = simple_strtoul(argv[2], NULL, 10);
> +	if ((serial_ports[port-1] == NULL) || (port < 1) || (port > MAX_SER_DEV)) {
> +		printf("Err: Invalid COM port number!\n\n");
> +		return 1;
> +	}
> +
> +	printf("Serial COM%d %s loop-back "
> +			"test @%d bps ", port, argv[1], gd->baudrate);
> +	if (port == CONFIG_CONS_INDEX)
> +		udelay(10000); /* make sure the stdout has been finished */
> +	if (serial_test(port, mode) != 0) {
> +		puts("FAILED\n");
> +		return 1;
> +	}
> +
> +	puts("PASSED\n");
> +	return 0;
> +}
> +
> +U_BOOT_CMD(
> +	uartest, 255, 1, do_serial_test,

"uartest"? "uart" + "est" ?  Or did you mean "uarttest" ? 

Anyway - the name makes no sense to me. Please see below.

> +	"Loop-back test for serial com port",
> +	"<loop-back mode> <serial com port>\n"
> +	"   where,  <loop-back mode> is \"internal\" or \"external\"\n"
> +	"           <com port> is number of serial com port (1..N)"
> +);

Such test code must be made configurable, i. e. it is not acceptable
to include this for all boards - please uase some CONFIG_ option for
it.


Also, the implementation violates the POST framework. If you add this
as POST code, then please fit it into the existing framework, i. e.
make it compile- and runtime configurable like other POST tests, run
it as sub-command of the "diag" command (rename the command then),
and log the results as the other tests.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Everyting looks interesting until you do it. Then you find it's  just
another job.                     - Terry Pratchett, _Moving Pictures_