[U-Boot] [PATCH] Hardware diagnosis for inka4x0 boards.
Wolfgang Denk
wd at denx.de
Wed Dec 10 00:57:25 CET 2008
Dear ap at denx.de,
In message <1227214731-19542-1-git-send-email-ap at denx.de> you wrote:
> RnJvbTogQW5kcmVhcyBQZmVmZmVybGUgPGFwQGRlbnguZGU+CgpUaGlzIHBhdGNoIGFkZHMgZGlh
> Z25vc2lzIGZ1bmN0aW9ucyBmb3IgdGhlIGJ1enplciwgVUFSVHMgYW5kIGRpZ2l0YWwKSU9zIG9u
> IGlua2E0eDAgaGFyZHdhcmUuCsKgIMKgClNpZ25lZC1vZmYtYnk6IEFuZHJlYXMgUGZlZmZlcmxl
> IDxhcEBkZW54LmRlPgotLS0KIGJvYXJkL2lua2E0eDAvTWFrZWZpbGUgICB8ICAgIDIgKy0KIGJv
> YXJkL2lua2E0eDAvaW5rYWRpYWcuYyB8ICA1MTUgKysrKysrKysrKysrKysrKysrKysrKysrKysr
> KysrKysrKysrKysrKysrKysrKwogMiBmaWxlcyBjaGFuZ2VkLCA1MTYgaW5zZXJ0aW9ucygrKSwg
> MSBkZWxldGlvbnMoLSkKIGNyZWF0ZSBtb2RlIDEwMDY0NCBib2FyZC9pbmthNHgwL2lua2FkaWFn
> LmMKCmRpZmYgLS1naXQgYS9ib2FyZC9pbmthNHgwL01ha2VmaWxlIGIvYm9hcmQvaW5rYTR4MC9N
> YWtlZmlsZQppbmRleCA0NDJlMmQwLi4yMjY0ZGFlIDEwMDY0NAotLS0gYS9ib2FyZC9pbmthNHgw
> L01ha2VmaWxlCisrKyBiL2JvYXJkL2lua2E0eDAvTWFrZWZpbGUKQEAgLTI1LDcgKzI1LDcgQEAg
> aW5jbHVkZSAkKFRPUERJUikvY29uZmlnLm1rCiAKIExJQgk9ICQob2JqKWxpYiQoQk9BUkQpLmEK
> IAotQ09CSlMJOj0gJChCT0FSRCkubworQ09CSlMJOj0gJChCT0FSRCkubyAgaW5rYWRpYWcubwog
> CiBTUkNTCTo9ICQoU09CSlM6Lm89LlMpICQoQ09CSlM6Lm89LmMpCiBPQkpTCTo9ICQoYWRkcHJl
> Zml4ICQob2JqKSwkKENPQkpTKSkKZGlmZiAtLWdpdCBhL2JvYXJkL2lua2E0eDAvaW5rYWRpYWcu
...
Please do not post base64 encoded messages!
Any further such postings will be ignored.
> This patch adds diagnosis functions for the buzzer, UARTs and digital
Such hardware test code should be implemented as part of the POST
system, i. e. please create a post/board/inka4x0/ directory for it,
and make it part of the POST.
> diff --git a/board/inka4x0/inkadiag.c b/board/inka4x0/inkadiag.c
> new file mode 100644
> index 0000000..6194b58
> --- /dev/null
> +++ b/board/inka4x0/inkadiag.c
...
> +#define LED_HIGH(NUM)\
> +out_be32((unsigned *)MPC5XXX_GPT##NUM##_ENABLE,\
> + in_be32((unsigned *)MPC5XXX_GPT##NUM##_ENABLE) | 0x10)
> +
> +#define LED_LOW(NUM)\
> +out_be32((unsigned *)MPC5XXX_GPT##NUM##_ENABLE,\
> + in_be32((unsigned *)MPC5XXX_GPT##NUM##_ENABLE) & ~0x10)
Please use "do { ...} while (0)" wrappers for these macros.
> +static int io_helper(int argc, char *argv[])
> +{
> + unsigned int state = inka_digin_get_input();
> + if (strcmp(argv[1], "drawer1") == 0) {
> + printf("exit code: 0x%X\n",
> + (state & DIGIN_DRAWER_SW1) >> (ffs(DIGIN_DRAWER_SW1) - 1));
> + } else if (strcmp(argv[1], "drawer2") == 0) {
> + printf("exit code: 0x%X\n",
> + (state & DIGIN_DRAWER_SW2) >> (ffs(DIGIN_DRAWER_SW2) - 1));
> + } else if (strcmp(argv[1], "other") == 0) {
> + printf("exit code: 0x%X\n",
> + ((state & DIGIN_KEYB_MASK) >> (ffs(DIGIN_KEYB_MASK) - 1))
> + | ((state & DIGIN_TOUCHSCR_MASK) >>
> + (ffs(DIGIN_TOUCHSCR_MASK) - 2)));
> + } else {
> + printf("Invalid argument: %s\n", argv[1]);
> + return -1;
> + }
> + return 0;
Please use the existing command parser code here and below.
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +static int ser_init(volatile struct mpc5xxx_psc *psc, int baudrate)
> +{
Maybe there should be some comments what these functions are doing?
> +static void ser_putc(volatile struct mpc5xxx_psc *psc, const char c)
> +{
> + /* Wait for last character to go. */
> + int i = 0;
> + while (!(psc->psc_status & PSC_SR_TXEMP) && (i++ < CONFIG_SYS_HZ))
> + udelay(10);
> + psc->psc_buffer_8 = c;
What exactly has CONFIG_SYS_HZ to do with that? I cannot understand
this sort of magic.
> +static int ser_getc(volatile struct mpc5xxx_psc *psc)
> +{
> + /* Wait for a character to arrive. */
> + int i = 0;
> + while (!(psc->psc_status & PSC_SR_RXRDY) && (i++ < CONFIG_SYS_HZ))
> + udelay(10);
Ditto here.
> + return psc->psc_buffer_8;
> +}
> +
> +#define SERIAL_PORT_BASE (u_char *)0x80000000
> +
> +#define UART_RX 0 /* In: Receive buffer (DLAB=0) */
> +#define UART_TX 0 /* Out: Transmit buffer (DLAB=0) */
> +#define UART_DLL 0 /* Out: Divisor Latch Low (DLAB=1) */
> +
> +#define UART_LCR 3 /* Out: Line Control Register */
> +#define UART_MCR 4 /* Out: Modem Control Register */
...
Lots of #defines right in the middle of a C file? That's uygly. Please
move these in a separate header file (or at least at the begin of the
C file).
> +static unsigned char serial_in(unsigned char num, int offset)
> +{
> + return in_8(SERIAL_PORT_BASE + (num << 3) + offset);
> +}
> +
> +static void serial_out(unsigned char num, int offset, unsigned char value)
> +{
> + out_8(SERIAL_PORT_BASE + (num << 3) + offset, value);
> +}
The amount of comments in this file sucessfully avoids all risks of
fatiguing the reader...
> + if (strcmp(argv[2], "115200") == 0)
> + combrd = 1;
> + else if (strcmp(argv[2], "57600") == 0)
> + combrd = 2;
> + else if (strcmp(argv[2], "38400") == 0)
> + combrd = 3;
> + else if (strcmp(argv[2], "19200") == 0)
> + combrd = 6;
> + else if (strcmp(argv[2], "9600") == 0)
> + combrd = 12;
> + else if (strcmp(argv[2], "2400") == 0)
> + combrd = 48;
> + else if (strcmp(argv[2], "1200") == 0)
> + combrd = 96;
> + else if (strcmp(argv[2], "300") == 0)
> + combrd = 384;
> + else
> + printf("Invalid baudrate: %s", argv[2]);
Instead of calling strcmp() again and again, you could convert the
number just once and use a switch. Or table lookup. See existing code.
> + if (mode & 1)
> + /* turn on 'loopback' mode */
> + serial_out(num, UART_MCR, UART_MCR_LOOP);
> + else {
Multiline statements require braces.
> + /* establish the UART's operational parameters */
> + /* set DLAB=1 */
Multiline comment style is
/*
* establish the UART's operational parameters
* set DLAB=1
*/
> + volatile struct mpc5xxx_psc *psc =
> + (struct mpc5xxx_psc *)address;
Indentation not by TAB.
> + if (freq > gd->ipb_clk / 128)
> + printf("%dHz exceeds maximum (%dHz)\n", freq,
> + gd->ipb_clk / 128);
> + else if (!freq)
Multiline statements require braces.
> +static int TEST_FUNCTION(inkadiag) (cmd_tbl_t *cmdtp, int flag, int argc,
> + char *argv[]) {
> + switch (argc) {
> + case 1:
> + PRINT_TEST_USAGE(inkadiag);
"break;" missing?
> + default:
> + argc--;
> + argv++;
> + CHECK_TEST(io);
> + CHECK_TEST(serial);
> + CHECK_TEST(buzzer);
> + }
> + PRINT_TEST_USAGE(inkadiag);
> +}
> +
> +U_BOOT_CMD(inkadiag, 6, 1, TEST_FUNCTION(inkadiag),
> + "inkadiag- inka diagnosis\n",
> + "[inkadiag what ...]\n"
> + " - perform a diagnosis on inka hardware\n"
> + "'inkadiag' performs hardware tests.\n\n"
> + "Without arguments, inkadiag prints a list of available tests.\n\n"
> + "To get detailed help information for specific tests you can type\n"
> + "'inkadiag what' with a test name 'what' as argument.\n");
Please no prose here - be terse, please.
Viele Grüße,
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
"Anyone attempting to generate random numbers by deterministic means
is, of course, living in a state of sin." - John Von Neumann
More information about the U-Boot
mailing list