--nextPart51760526.MYs8sR5UP9
Content-Type: text/plain;
  charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline

Thank you for the review comments.  My responses below.

On Saturday 28 January 2006 8:57 am, Izumi Tsutsui wrote:
> 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".

True.  Not sure what the style guide says here, but I'm happy enough to rem=
ove=20
them.

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

True.  Old habits die hard I guess.  (The Solaris coding style requires the=
m. =20
It is one of the very few ways in which the Sun KNF style differs from=20
BSD's.)  I guess I missed this one.

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

Agreed.  I think this is legacy as I basically moved this function from=20
another file.  I'm not sure which is better -- consistency across nested=20
calls (legacy here), or changing to the new style.
>
> > +# 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.

There isn't really a GENERIC config.  This came from the PB1000 config file=
=2E =20
I figured it best to minimize arbitrary changes.  If we want this commented=
=20
out, I'd prefer to do it in a separate commit, making sure it is=20
non-controversial.  (I don't know who else besides Simon and I use Alchemy=
=20
processors with NetBSD right now, but I'd like to make sure.)

>
> > +options         SCSIVERBOSE     # human readable SCSI error messages
>
> Spaces by pasto? Oh, pulled from PB1000...

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

I like it better centralized.  If you want to use PCI, or PCMCIA, or the=20
internal LCD driver, you have to have this.  Pretty much every Alchemy part=
=20
has stuff above 4GB.  For now we don't support all these devices, but we wi=
ll=20
eventually (or so I assume), and so I think we really want this option to b=
e=20
universal on all Alchemy parts.

As far as I can tell, userland binaries are not impacted.  At least I'm abl=
e=20
to use most of the usual tools that grovel kmem -- ps, top, netstat, vmstat=
,=20
etc.

>
> > +boolean_t
> > +au1000_match(au_chipdep_t **cpp)
> > +{
> > +
> > +	if (MIPS_PRID_COPTS(cpu_id) =3D=3D MIPS_AU1000) {
> > +		*cpp =3D &au1000_chipdep;
> > +		return TRUE;
> > +	}
> > +	return FALSE;
> > +}
> >
> > +au_chipdep(void)
> > +{
> > +
> > +	if (au_chip !=3D NULL)
> > +		return (au_chip);
> > +
> > +	if ((au_chip =3D=3D 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 =3D=3D 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.

I could convert the function to do this, I suppose.  In many ways the match=
ing=20
logic is similar to driver match logic (I guess I modeled after that).  I=20
like this style better, as it can accomodate different changes later if we=
=20
find that MIPS_PRID_COPTS(cpu_id) is not sufficient to identify a part.

Also, if I don't do it this way, then I have to extern the au_chipdep data=
=20
structures for each part.

Even if I switch to a case table, I still need to have #ifdef's around each=
=20
case, as the necessary processor support routines may not be included in th=
e=20
config file.

Anyway, I'll make the conversion if there is sufficient belief that I shoul=
d=20
do so, but right now my first preference is to leave it as is.

>
> > -#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?

I noticed that it had.  I guessed this is to make us more strictly C99=20
conformant?

>
> > +#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).

=46rankly, this is the result of my not being entirely confident in a few=20
things:

	1) the VM initialization logic -- making sure that the ordering constraint=
 is=20
met.  As you say, we might have to map e.g. a framebuffer console device=20
before VM is initialized.  (Not necessarily PCI either -- the Au1100 and=20
Au1200 parts have on-chip LCD controllers that live up above 4GB.)  Also,=20
what about devices that the kernel and root filesystem might be located on?=
 =20
Again, this probably represents ignorance on my part w.r.t. the NetBSD VM=20
system.  (I know the Solaris VM, but it is quite different.)

	2) considerations about blocking allocations, extent, and the fact that I=
=20
might have to wire down stuff in interrupt context.  (Right now this doesn'=
t=20
happen, but I thought better safe now than sorry later.)

Anyway, for now I'd prefer to leave it alone, and then possibly change it=20
later after a commit if we are confident that it is safe to do so.
>
> > +#define AU_WIRED_RR(TYPE,BYTES)
>
> #defile<space> or #define<tab>?
> Anyway, they should be consistent.

Yes.  I prefer #define<tab>, but I think a lot of previous stuff may use=20
<space>.  I tried to be consistent in most places, but I might have=20
accidentally reverted to <tab> out of habit.

> > +#if !defined(_MIPS_PADDR_T_64BIT) && !defined(_LP64)
>
> Hmm, mips64 toolchain will define _LP64? ;-)

It had better.  Otherwise it isn't standards conformant.  (Not sure which=20
standard requires it -- probably C99 at least.)

> > +#if defined(__MIPSEB__)
>
> Isn't it better to use "#if _BYTE_ORDER =3D=3D BIG_ENDIAN"?

Possibly.  The code will only ever be used in mips, so it probably doesn't=
=20
matter, other than to suit taste.

>
> > +	/* align it down to start of phys addr */
> > +	off =3D pa & (MIPS3_WIRED_SIZE - 1);
>
> This could use MIPS3_WIRED_OFFMASK.

Good point.
>
> > +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.

I prefered the longer name for two (admittedly weak) reasons:

	1) it reflects that these files really are specializations of chipdep
	2) it avoids possible filename conflict in the build system later.  (NetBS=
D=20
kernel builds don't permit same-named source files in different directories=
=20
=2D- something that caught me off-guard once and bit me.)

However, if there is some reason why the names should be shortened, I'm=20
willing to do so.  (Linker or filesystem naming limitations on some=20
platforms?)

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

A reasonable point.  I can make that change.
>
> BTW, [bl]etoh{16,32,64}() should be [bl]e{16,32,64}toh().
> (we are not OpenBSD...)

OK... :-)

>
>
> 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 :-)

OK, so I want to get a little more consensus, if possible, unless you feel=
=20
confident in saying it is safe for me to commit.  (Simonb has been mostly=20
silent, at least when I've pinged him of late.  Not sure who else can speak=
=20
for the platform right now.)

Again, thanks for the review.

	-- Garrett

> ---
> Izumi Tsutsui

=2D-=20
Garrett D'Amore, Principal Software Engineer
Tadpole Computer / Computing Technologies Division,
General Dynamics C4 Systems
http://www.tadpolecomputer.com/
Phone: 951 325-2134  Fax: 951 325-2191

--nextPart51760526.MYs8sR5UP9
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2 (SunOS)

iQEVAwUAQ9vCSP49Sp1nAoU7AQK8tgf/ZP74nkQVs+thzs1aDSXmucabnMcZnJG+
Kqjeg6+4SuIJVM5v04dE44GUaiGGImYhuOJt62YePrP8du//TqfA1yKAH95QnNpu
wTlWhZjTJS56j55kofTLR3NkoRdkEMIIlwSyjdhi1kYQ2aVzgg+tVsnXyPHFq8C0
609sZRs0RFxVy5H4kh/Wwjodw/Eeu1KEQ7dpWmcVUvGtQGD84hMKr+yYnVDw3slX
OVu7usxs461f8Sh6rC0y/sG462zMBJc4y9HieVaKgip2EmWJR6Tq6zQWTkB5PW7F
LrnippnPfG5CRdiw3dH0JoAFF/lUJP3XN4hYFoCTBXq6ZJpBTBAN/Q==
=sMPQ
-----END PGP SIGNATURE-----

--nextPart51760526.MYs8sR5UP9--