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

[Michael Zaidman]

On Mon, Mar 22, 2010 at 10:14 PM, Wolfgang Denk <wd at denx.de> wrote:
> 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.
>

Sorry, I meant that this patch is based on the "Serial support
extended up to 6 COMs" patch  (See
http://lists.denx.de/pipermail/u-boot/2010-March/068796.html).

BTW, If one patch depends on another - what is the correct way to
specify this dependency on the e-mail list?

>> 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.

Of course. This patch should be applied after
http://lists.denx.de/pipermail/u-boot/2010-March/068796.html patch.
>
>> +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.
>

OK, will be done.

>
> "uartest"? "uart" + "est" ?  Or did you mean "uarttest" ?
 >
> Anyway - the name makes no sense to me. Please see below.

OK, will be corrected.

>> +     "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.

OK, will be fixed.

> 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.
>

Strange, it is what I thought I did in the newly added
/post/drivers/serial.c file in this commit. It is possible to run it
via "diag run uart" command and it meets all requirements you
mentioned above.

Just copied this fragment of the patch below for your convenience.

diff --git a/post/drivers/serial.c b/post/drivers/serial.c
new file mode 100644
index 0000000..4ace9d0
--- /dev/null
+++ b/post/drivers/serial.c
@@ -0,0 +1,56 @@
+/*
[snip]
+
+#include <common.h>
+#include <ns16550.h>
+#include <post.h>
+
+#if CONFIG_POST & CONFIG_SYS_POST_UART
+
+extern int serial_test(int com_port, int loopback_mode);
+extern int get_number_of_serial_ports(void);
+extern NS16550_t get_serial_port(int index);
+
+DECLARE_GLOBAL_DATA_PTR;
+
+int __uart_post_test (int flags)
+{
+       int port;
+       int ret = 0;
+
+       udelay(10000); /* make sure the stdout has been finished */
+       for (port = 1;  (get_serial_port(port-1) != NULL) &&
+               (port <= get_number_of_serial_ports()); port++) {
+               if (serial_test(port, 0)) {
+                       post_log("Serial COM%d internal loop-back "
+                                       "test @%d bps ", port, gd->baudrate);
+                       ret = 1;
+               }
+       }
+       return ret;
+}
+int uart_post_test(int flags) __attribute__((weak, alias("__uart_post_test")));
+
+#endif

Also, I had doubt where to place the serial (actually uart) test in
the POST framework. Till now uart tests are located in POST cpu
specific code. This test is not cpu specific, rather 16550 specific.
So I placed it under post\drivers and defined the uart_post_test
routine as weak in order do not interfere with board specific uart
POST tests.

Thanks,
Michael