Re: [U-Boot] [PATCH 2/2] Added serial loopback tests accessible via CLI and POST
Dear Michael Zaidman,
In message <1269267894-15324-1-git-send-email-michael.zaid...@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
> ---
> 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
> #include
> +#include
>
> #ifdef CONFIG_NS87308
> #include
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(1); /* 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",
> + " \n"
> + " where, is \"internal\" or \"external\"\n"
> + "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: w...@denx.de
Everyting looks interesting until you do it. Then you find it's just
another job. - Terry Pratchett, _Moving Pictures_
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/2] Added serial loopback tests accessible via CLI and POST
On Mon, Mar 22, 2010 at 10:14 PM, Wolfgang Denk wrote:
> Dear Michael Zaidman,
>
> In message <1269267894-15324-1-git-send-email-michael.zaid...@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
>> ---
>> 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
>> #include
>> +#include
>>
>> #ifdef CONFIG_NS87308
>> #include
>
> 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",
>> + " \n"
>> + " where, is \"internal\" or \"external\"\n"
>> + " 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 000..4ace9d0
--- /dev/null
+++ b/post/drivers/serial.c
@@ -0,0 +1,56 @@
+/*
[snip]
+
+#include
+#include
+#include
+
+#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(1); /* 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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/2] Added serial loopback tests accessible via CLI and POST
Dear Michael Zaidman, In message <660c0f821003230315i54c79d5au12377695cb85a...@mail.gmail.com> you wrote: > > 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? Such patches should be submitted as one series, so dependency is implicitly resolved. Posting a link to a mailing list archive or the message ID (so you can look it up at http://mid.gmane.org/) work fine. > > 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. Then what is the "uart[t]est" command needed for? > 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. They are locaed in post/cpu/ iff they are CPU specific. Otherfwise they go like any other driver tests in the driver's directory. > 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. I don't get this. Where is the weak part needed? Either I have only one type of UART (then the weak is not needed as only onedriver is enabled), or I have both "CPU specific" and "generic" (16550 based) UARTs, in which case I eventually might ant to test _both_ of them (then the weak will not work). 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: w...@denx.de Don't tell me how hard you work. Tell me how much you get done. -- James J. Ling ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/2] Added serial loopback tests accessible via CLI and POST
Dear Wolfgang, On Tue, Mar 23, 2010 at 1:31 PM, Wolfgang Denk wrote: [snip] > Then what is the "uart[t]est" command needed for? For two reasons: 1) It gets parameters such internal/external loopback and COM number while "diag run uart" performs only local loopback through all COMs. Thus, it can be used at production to perform external loopback tests. 2) This test will be available even when compiled without POST for reasons of POST tests unavailability for particular cpu/board or specific board memory layout constrains. > [snip] > >> 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. > > I don't get this. Where is the weak part needed? Either I have only > one type of UART (then the weak is not needed as only onedriver is > enabled), or I have both "CPU specific" and "generic" (16550 based) > UARTs, in which case I eventually might ant to test _both_ of them > (then the weak will not work). > Currently, we have only CONFIG_SYS_POST_UART which will cause both files to be compiled in the case of mpc8xx or ppc4xx CPUs. Which will lead to the linker failure if no weak definition will be used. Do you mean we should do it at the makefile level by adding CONFIG_ specifing which file should be compiled and linked? Thanks, Michael ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/2] Added serial loopback tests accessible via CLI and POST
Dear Michael Zaidman, In message <660c0f821003230641s5716a04cn2b15becf7c457...@mail.gmail.com> you wrote: > > > Then what is the "uart[t]est" command needed for? > > For two reasons: > 1) It gets parameters such internal/external loopback and COM number > while "diag run uart" performs only local loopback through all COMs. > Thus, it can be used at production to perform external loopback tests. This does not fit into the POST framework then, it seems. > 2) This test will be available even when compiled without POST for > reasons of POST tests unavailability for particular cpu/board or > specific board memory layout constrains. I don't like such chimera code. > > I don't get this. Where is the weak part needed? Either I have only > > one type of UART (then the weak is not needed as only onedriver is > > enabled), or I have both "CPU specific" and "generic" (16550 based) > > UARTs, in which case I eventually might ant to test _both_ of them > > (then the weak will not work). > > > Currently, we have only CONFIG_SYS_POST_UART which will cause both > files to be compiled in the case of mpc8xx or ppc4xx CPUs. Which will > lead to the linker failure if no weak definition will be used. > Do you mean we should do it at the makefile level by adding CONFIG_ > specifing which file should be compiled and linked? I think this is a consequence of trying to squeeze soemthing into a framework which doesn't fit. POST and production test code should be kept separate. If they share common code, fine. 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: w...@denx.de You have the capacity to learn from mistakes. You'll learn a lot today. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/2] Added serial loopback tests accessible via CLI and POST
Dear Wolfgang, On Wed, Mar 31, 2010 at 12:27 AM, Wolfgang Denk wrote: > > I think this is a consequence of trying to squeeze soemthing into a > framework which doesn't fit. POST and production test code should be > kept separate. If they share common code, fine. > > Best regards, > > Wolfgang Denk Strange, is not it what I did in the patch? I added POST uart tests for 16550 compatible uarts in the post/drivers/serial.c file and added uarttest command for production testing in the drivers/serial/serial.c file. Thus these tests are separated and serve different purposes. And both of them share common code. Probably it makes sense to divide the patch into two separate patches? Regards, Michael ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
