[U-Boot] [PATCH] inka4x0: Add hardware diagnosis functions for inka4x0

[Detlev Zundel <dzu@denx.de>]

Hi Wolfgang,

> Dear Detlev Zundel,
>
> In message <1237914158-15693-6-git-send-email-dzu@denx.de> you wrote:
>> This patch adds advanced diagnosis functions for the inka4x0 board.
>> 
>> Signed-off-by: Andreas Pfefferle <ap@denx.de>
>> Signed-off-by: Detlev Zundel <ap@denx.de>
> ...
>> +	extern int inkadiag_init_r (void);
>> +	/*
>> +	  * The command table used for the subcommands of inkadiag
>> +	  * needs to be relocated manually.
>> +	  */
>
> Incorrect multiline comment style?

Nothing escapes the eye of an eagle ;)

>> +}
>> +
>>  int misc_init_f (void)
>>  {
>>  	char tmp[10];
>> diff --git a/board/inka4x0/inkadiag.c b/board/inka4x0/inkadiag.c
>> new file mode 100644
>> index 0000000..bdbf652
> ...
>> +#define LED_HIGH(NUM) \
>> +    do { \
>> +	    out_be32((unsigned *)MPC5XXX_GPT##NUM##_ENABLE,		\
>> +		     in_be32((unsigned *)MPC5XXX_GPT##NUM##_ENABLE) | 0x10); \
>> +    } while (0)
>
> Please use:
>
> 	setbits_be32 ((unsigned *)MPC5XXX_GPT##NUM##_ENABLE,  0x10); 
>
> instead (and so on in the rest of the code).

Done.

>> +		struct mpc5xxx_gpio *gpio = (struct mpc5xxx_gpio *)MPC5XXX_GPIO;
>> +		if (which == 1) {
>> +			/* drawer1 */
>> +			if (state) {
>> +				gpio->simple_dvo &= ~0x1000;
>> +				udelay(1);
>> +				gpio->simple_dvo |= 0x1000;
>> +			} else {
>> +				gpio->simple_dvo |= 0x1000;
>> +				udelay(1);
>> +				gpio->simple_dvo &= ~0x1000;
>> +			}
>> +		}
>> +		if (which == 2) {
>> +			/* drawer 2 */
>> +			if (state) {
>> +				gpio->simple_dvo &= ~0x2000;
>> +				udelay(1);
>> +				gpio->simple_dvo |= 0x2000;
>> +			} else {
>> +				gpio->simple_dvo |= 0x2000;
>> +				udelay(1);
>> +				gpio->simple_dvo &= ~0x2000;
>
> Please use accessor functions to write to I/O ports.

Fixed.

>> +static int ser_init(volatile struct mpc5xxx_psc *psc, int baudrate)
>> +{
>> +	unsigned long baseclk;
>> +	int div;
>> +
>> +	/* reset PSC */
>> +	psc->command = PSC_SEL_MODE_REG_1;
>
> Should we not use accessor function to access the device registers
> (here and in the following code) ?

Indeed we should.  But here we go, I can understand, why this wasn't
done the first time around.  The new code looks like this:

	/* reset PSC */
	out_8(&psc->command, PSC_SEL_MODE_REG_1);

	/* select clock sources */

	out_be16(&psc->psc_clock_select, 0);
	baseclk = (gd->ipb_clk + 16) / 32;

	/* switch to UART mode */
	out_be32(&psc->sicr, 0);

	/* configure parity, bit length and so on */

	out_8(&psc->mode, PSC_MODE_8_BITS | PSC_MODE_PARNONE);
	out_8(&psc->mode, PSC_MODE_ONE_STOP);

	/* set up UART divisor */
	div = (baseclk + (baudrate / 2)) / baudrate;
	out_8(&psc->ctur, (div >> 8) & 0xff);
	out_8(&psc->ctlr, div & 0xff);

	/* disable all interrupts */
	out_be16(&psc->psc_imr, 0);

	/* reset and enable Rx/Tx */
	out_8(&psc->command, PSC_RST_RX);
	out_8(&psc->command, PSC_RST_TX);
	out_8(&psc->command, PSC_RX_ENABLE | PSC_TX_ENABLE);


Look at the wonderful mix of out_{8,be16,be32}.  What a heart refreshing
break from the monotonic coding routine :(

Actully this is due to to mpc5xxx.h:

struct mpc5xxx_psc {
	volatile u8	mode;		/* PSC + 0x00 */
	volatile u8	reserved0[3];
	union {				/* PSC + 0x04 */
		volatile u16	status;
		volatile u16	clock_select;
	} sr_csr;
#define psc_status	sr_csr.status
#define psc_clock_select sr_csr.clock_select
	volatile u16	reserved1;
	volatile u8	command;	/* PSC + 0x08 */
	volatile u8	reserved2[3];
	union {				/* PSC + 0x0c */
		volatile u8	buffer_8;
		volatile u16	buffer_16;
		volatile u32	buffer_32;
	} buffer;
#define psc_buffer_8	buffer.buffer_8
#define psc_buffer_16	buffer.buffer_16
#define psc_buffer_32	buffer.buffer_32
	union {				/* PSC + 0x10 */
		volatile u8	ipcr;
		volatile u8	acr;
	} ipcr_acr;
#define psc_ipcr	ipcr_acr.ipcr
#define psc_acr		ipcr_acr.acr
	volatile u8	reserved3[3];
	union {				/* PSC + 0x14 */
		volatile u16	isr;
		volatile u16	imr;
	} isr_imr;


Git-blame points the finger to someone called 'wd'.  Maybe he knows
about the advantages of such structure definitions?  

On second check the corresponding header file in Linux (mpc52xx_uart.h) has
stuff like this introduced by Kumar Gala, maybe he can enlighten me?

To be honest, I wouldn't even dare accessing memory mapped registers in
the non-native size, but maybe I'm missing something here.

>> +static int do_inkadiag_serial(cmd_tbl_t *cmdtp, int flag, int argc,
>> +			      char *argv[]) {
>> +	if (argc < 5) {
>> +		cmd_usage(cmdtp);
>> +		return 1;
>> +	}
>> +
>> +	argc--;
>> +	argv++;
>> +
>> +	unsigned int num = simple_strtol(argv[0], NULL, 0);
>
> Please NEVER declare variables right in the middle of the code!!

I won't, I PROMISE.

> Did this actually pass a compile test?

Heck, do you think I dare send a patch without compiling?

>> +		if (mode & 4) {
>> +			/* set data terminal ready */
>> +			serial_out(num, UART_MCR, UART_MCR_DTR);
>> +			udelay(10);
>> +			/* check data set ready and carrier detect */
>> +			if ((serial_in(num, UART_MSR) &
>> +			     (UART_MSR_DSR | UART_MSR_DCD))
>> +			    != (UART_MSR_DSR | UART_MSR_DCD))
>> +				return -1;
>> +		}
>> +
>> +		/* write each message-character, read it back, and display it */
>> +		int i, len = strlen(argv[3]);
>
> Ditto. Please make sure to fix this globally.

I believe I succeeded.

>> +		for (i = 0; i < len; ++i) {
>> +			int j = 0;
>> +			while ((serial_in(num, UART_LSR) & UART_LSR_THRE) ==
>> +				0x00) {
>
> This is actually difficult to read. Please reconsider indentation /
> line wrapping / adding a blank line.

The switch to using struct NS16650 allowed for nicer layout without
violating line length.

>> +				if (j++ > CONFIG_SYS_HZ)
>> +					break;
>> +				udelay(10);
>> +			}
>> +			unsigned char data = serial_in(num, UART_RX);
>> +			printf("%c", data);
>> +		}
>> +		printf("\n\n");
>> +		serial_out(num, UART_MCR, 0x00);
>> +	} else {
>> +		int address = 0;
>> +		int irq;
>> +
>> +		switch (num) {
>> +		case 8:
>> +			address = MPC5XXX_PSC6;
>> +			irq = MPC5XXX_PSC6_IRQ;
>> +			break;
>> +		case 9:
>> +			address = MPC5XXX_PSC3;
>> +			irq = MPC5XXX_PSC3_IRQ;
>> +			break;
>> +		case 10:
>> +			address = MPC5XXX_PSC2;
>> +			irq = MPC5XXX_PSC2_IRQ;
>> +			break;
>> +		case 11:
>> +			address = MPC5XXX_PSC1;
>> +			irq = MPC5XXX_PSC1_IRQ;
>> +			break;
>> +		}
>
> "irq" seems to be unused (accept for the assignments) in  this  code.
> Wasn't there a compiler warning?

Nope.

>> +#define BUZZER_GPT	(MPC5XXX_GPT + 0x60)	/* GPT6 */
>> +static void buzzer_turn_on(unsigned int freq)
>> +{
>> +	struct mpc5xxx_gpt *gpt = (struct mpc5xxx_gpt *)(BUZZER_GPT);
>> +
>> +	const u32 prescale = gd->ipb_clk / freq / 128;
>> +	const u32 count = 128;
>> +	const u32 width = 64;
>> +
>> +	gpt->cir = (prescale << 16) | count;
>> +	gpt->pwmcr = width << 16;
>> +	gpt->emsr = 3;		/* Timer enabled for PWM */
>
> Accessor functions?

Yep.

>> +}
>> +
>> +static void buzzer_turn_off(void)
>> +{
>> +	struct mpc5xxx_gpt *gpt = (struct mpc5xxx_gpt *)(BUZZER_GPT);
>> +	gpt->emsr = 0;
>
> Accessor functions?

Again, not a question.

>> +}
>> +
>> +static int do_inkadiag_buzzer(cmd_tbl_t *cmdtp, int flag, int argc,
>> +			      char *argv[]) {
>> +
>> +	if (argc != 3) {
>> +		cmd_usage(cmdtp);
>> +		return 1;
>> +	}
>> +
>> +	argc--;
>> +	argv++;
>> +
>> +	unsigned int period = simple_strtol(argv[0], NULL, 0);
>
> Declaration in the middle of the code.

Fixed.

>> +	if (!period)
>> +		printf("Zero period is senseless\n");
>> +	argc--;
>> +	argv++;
>> +
>> +	unsigned int freq = simple_strtol(argv[0], NULL, 0);
>
> Declaration in the middle of the code.

What insolence!  Fixed.

>> +	/* avoid zero prescale in buzzer_turn_on() */
>> +	if (freq > gd->ipb_clk / 128) {
>> +		printf("%dHz exceeds maximum (%ldHz)\n", freq,
>> +		       gd->ipb_clk / 128);
>> +	} else if (!freq)
>> +		printf("Zero frequency is senseless\n");
>> +	else
>> +		buzzer_turn_on(freq);
>> +
>> +	clear_ctrlc();
>> +	int prev = disable_ctrlc(0);
>> +
>> +	printf("Buzzing for %d ms. Type ^C to abort!\n\n", period);
>> +
>> +	int i = 0;
>
> Declaration in the middle of the code.

Fixed.

>> +	while (!ctrlc() && (i++ < CONFIG_SYS_HZ))
>> +		udelay(period);
>> +
>> +	clear_ctrlc();
>> +	disable_ctrlc(prev);
>> +
>> +	buzzer_turn_off();
>> +
>> +	return 0;
>> +}
>> +
>> +static int do_inkadiag_help(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]);
>> +
>> +cmd_tbl_t cmd_inkadiag_sub[] = {
>> +	U_BOOT_CMD_MKENT(io, 1, 1, do_inkadiag_io, "read digital input",
>> +			 "<drawer1|drawer2|other> [value] - get or set specified signal\n"),
>> +	U_BOOT_CMD_MKENT(serial, 4, 1, do_inkadiag_serial, "test serial port",
>> +			 "<num> <mode> <baudrate> <msg>  - test uart num [0..11] in mode\n"
>> +			 "and baudrate with msg\n"),
>> +	U_BOOT_CMD_MKENT(buzzer, 2, 1, do_inkadiag_buzzer, "activate buzzer",
>> +			 "<period> <freq> - turn buzzer on for period ms with freq hz\n"),
>> +	U_BOOT_CMD_MKENT(help, 4, 1, do_inkadiag_help, "get help",
>> +			 "[command] - get help for command\n"),
>> +};
>
> Lines too long.

Personally I do not think that the new version is any more readable.  To
fix the "problem", I can split the string or violate indentation.  As
the first seems very unattractive from a reading point of view, I
actually willingly decided for the latter.  I hope you don't criticize
that in the next round.

>> +static int do_inkadiag_help(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) {
>> +	extern int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp,
>> +			     int flag, int argc, char *argv[]);
>> +	/* do_help prints command name - we prepend inkadiag to our subcommands! */
>
> Lines too long.

Again I believe the new version is harder to read, but I don't really
care.

Cheers
  Detlev

-- 
We have a live-manual.  It's called emacs-devel at gnu.org.
You can stick to just reading it, but you can skip to a specific chapter
by simply sending an email asking for it ;-)
                                    -- Stefan Monnier
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de