In article <43D6B26E.1070409@tadpole.com>
garrett_damore@tadpole.com wrote:

>     http://garrett.damore.org/software/netbsd/aupci.diff
 :
> Constructive criticism is welcomed.  Flames to /dev/null.

Mostly it looks fine for me, but I'd like some more cosmetics:

> +extern void board_init(void);

I think function declarations don't need "extern".

> +	return (cabernet_obio);

No parentheses are needed around the return value, as share/misc/style.
(though many other sources have them)

> +evbmips_iointr(u_int32_t status, u_int32_t cause, u_int32_t pc,
> +    u_int32_t ipending)

uintXX_t is better rather than u_intXX_t.

> +# CPU support
> +options		ALCHEMY_AU1000

Use "options<space><tab>".

> +options 	DIAGNOSTIC	# extra kernel sanity checking

DIAGNOSTIC should be enabled or disalbed?
It has been disabled in all other GENERIC-like config.

> +options         SCSIVERBOSE     # human readable SCSI error messages

Spaces by pasto? Oh, pulled from PB1000...

> +options	_MIPS_PADDR_T_64BIT

This should be in <machine/types.h> or each config file?
As you asked before on port-mips, maybe we have to rethink
how we should handle userland binaries...

> +boolean_t
> +au1000_match(au_chipdep_t **cpp)
> +{
> +
> +	if (MIPS_PRID_COPTS(cpu_id) == MIPS_AU1000) {
> +		*cpp = &au1000_chipdep;
> +		return TRUE;
> +	}
> +	return FALSE;
> +}
 :
> +au_chipdep(void)
> +{
> +
> +	if (au_chip != NULL)
> +		return (au_chip);
> +
> +	if ((au_chip == NULL) &&
> +#ifdef	ALCHEMY_AU1000
> +	    (!au1000_match(&au_chip)) &&
> +#endif
> +#ifdef	ALCHEMY_AU1100
> +	    (!au1100_match(&au_chip)) &&
> +#endif
> +#ifdef	ALCHEMY_AU1500
> +	    (!au1500_match(&au_chip)) &&
> +#endif
> +#ifdef	ALCHEMY_AU1550
> +	    (!au1550_match(&au_chip)) &&
> +#endif
> +	    (au_chip == NULL)) {

Hmm, the name of au*_match() implies the match function of
driver(9) for me. IMO it looks better to use switch() against
MIPS_PRID_COPTS(cpu_id) and then call au1*_init() functions,
as the previous aubus.c:aubus_attach() does.

> -#define	REGVAL(x)	*((volatile u_int32_t *)(MIPS_PHYS_TO_KSEG1((x))))
> +#define	REGVAL(x)	*((__volatile u_int32_t *)(MIPS_PHYS_TO_KSEG1((x))))

all __volatile has been changed to volatile recently?

> +#ifndef	AU_WIRED_EXTENT_SZ
> +#define	AU_WIRED_EXTENT_SZ	EXTENT_FIXED_STORAGE_SIZE(10)
> +#endif

This value could be smaller. The static strage is only required
by devices mapped before VM is initialized, i.e. only some
console devices do. During cpu_configure(9) (after MD cpu_startup())
we could pass EX_MALLOCOK to extent(9).

> +#define AU_WIRED_RR(TYPE,BYTES)

#defile<space> or #define<tab>?
Anyway, they should be consistent.

> +#if !defined(_MIPS_PADDR_T_64BIT) && !defined(_LP64)

Hmm, mips64 toolchain will define _LP64? ;-)

> +#if defined(__MIPSEB__)

Isn't it better to use "#if _BYTE_ORDER == BIG_ENDIAN"?

> +	/* align it down to start of phys addr */
> +	off = pa & (MIPS3_WIRED_SIZE - 1);

This could use MIPS3_WIRED_OFFMASK.

> +file	arch/mips/alchemy/au_chipdep.c
> +file	arch/mips/alchemy/au1000_chipdep.c	alchemy_au1000
> +file	arch/mips/alchemy/au1100_chipdep.c	alchemy_au1100
> +file	arch/mips/alchemy/au1500_chipdep.c	alchemy_au1500
> +file	arch/mips/alchemy/au1550_chipdep.c	alchemy_au1550

"_chipdep" is needed for these files?
Just au1xx0.c and au1[015][05]0.c look fine for me.

> +#if defined(CHIP_LITTLE_ENDIAN)
> +#define	CHIP_SWAP16(x)	letoh16(x)
> +#define	CHIP_SWAP32(x)	letoh32(x)
> +#define	CHIP_SWAP64(x)	letoh64(x)
> +#define	CHIP_NEED_STREAM	1
> +#elif defined(CHIP_BIG_ENDIAN)
> +#define	CHIP_SWAP16(x)	betoh16(x)
> +#define	CHIP_SWAP32(x)	betoh32(x)
> +#define	CHIP_SWAP64(x)	betoh64(x)
> +#define	CHIP_NEED_STREAM	1	
> +#else
> +#define	CHIP_SWAP16(x)	(x)
> +#define	CHIP_SWAP32(x)	(x)
> +#define	CHIP_SWAP64(x)	(x)
> +#endif

I think it's better to have both HTOCHIP{16,32,64}()
and CHIP{16,32,64}TOH() macros for readability
and consistency even though they are identical.

BTW, [bl]etoh{16,32,64}() should be [bl]e{16,32,64}toh().
(we are not OpenBSD...)


As you said you don't have to be so perfect and
I think your patch is good enough to commit.
Just I'm a bit paranoid :-)
---
Izumi Tsutsui