Re: [coreboot] [PATCH] inteltool: i965, i975, ICH8M and Darwin support.

[Stefan Reinauer]

Uwe Hermann wrote:
> I was about to ack this, but it fails to build, see below. I also added
> a few smaller comments to the review, but the build issue is the only
> really important one.
>
> On Sun, Nov 30, 2008 at 10:07:55PM +0100, Stefan Reinauer wrote:
> > +#ifndef DARWIN
> > +#include <sys/io.h>
> > +#else
> > +#include <DirectIO/darwinio.h>
> > +#endif
>
> Maybe a small paragraph with build instructions and URL for DirectIO
> etc. in the README/manpage would be good (that's for another patch though).

Inteltool has no README yet. It seems it should have one.

The DirectIO code can be downloaded here:

http://www.coresystems.de/en/directio
> > #define PCI_DEVICE_ID_INTEL_82845	0x1a30
> >  #define PCI_DEVICE_ID_INTEL_82945P	0x2770
> >  #define PCI_DEVICE_ID_INTEL_82945GM	0x27a0
> > +#define PCI_DEVICE_ID_INTEL_PM965       0x2a00
> > +#define PCI_DEVICE_ID_INTEL_82975X      0x277c
>
> These two use spaces, not TABs, and are thus not properly aligned,
> please change to TABs in the commit.

fixed.
> > for (i = 0; i < size; i += 4) {
> > +#ifdef DARWIN
> > if (i==0x14) {
>                  ^ ^ 
>        add spaces here, please.

fixed.
> > 	/* Reading this register will hang a macbook pro */
> > 	printf("pmbase+0x%04x: 0xXXXXXXXX\n", i);
>
> Any idea why? Hardware problem or OS problem? Does Linux on Apple
> hardware have the same issue? If yes, the "#ifdef DARWIN" check may
> not be correct/complete.

I am not sure yet. There is no Linux on that box. It might even be a
generic ICH8 thing. Time will show and bring a patch when we find out.
> > +void unmap_physical(void *virt_addr, int len)
> > +{
> >   munmap((void *)rcba, size);
> >
> > +}
>
> This part fails to build, as 'rcba' is never defined. I guess 'virt_addr' is
> the correct thing to use here (?) If yes, the (void *) cast is not
> needed, too, I think.
>
> Also, s/size/len/, otherwise it won't build.

awkward and fixed.
> > Index: Makefile
> >
> > --- Makefile	(revision 3783)
> > +++ Makefile	(working copy)
> > @@ -29,6 +29,13 @@
> >
> > OBJS = inteltool.o cpu.o gpio.o rootcmplx.o powermgt.o memory.o pcie.o
> >
> > +OS_ARCH	= $(shell uname)
> > +ifeq ($(OS_ARCH), Darwin)
> > +CFLAGS += -DDARWIN -I/usr/local/include 
> > +LDFLAGS = -framework IOKit -framework DirectIO -L/usr/local/lib -lpci -lz
>
> Maybe /usr/local should be easily configurable (adding it as variable,
> or reusing PREFIX or so)?

The default installation of pciutils is /usr/local/, and usually on OSX
it would be installed as a .pkg.
Using PREFIX is wrong, because that's the install prefix of inteltool,
not of pciutils.

While the above always works in the default case and in the case that
pciutils' PREFIX was just /usr, one might say the right way to do this
is by looking for pciutils on the system, ie with autoconf. But that's
outside of the scope of this patch.
> > +# OBJS += darwinio.o
> > +endif
> >
> > all: pciutils dep $(PROGRAM)
> >
> > $(PROGRAM): $(OBJS)
> > @@ -41,10 +48,10 @@
> >    rm -f .dependencies
> >
> > dep:
> > @$(CC) -MM *.c > .dependencies
> > @$(CC) $(CFLAGS) -MM *.c > .dependencies
> >
> > pciutils:
> > @echo; echo -n "Checking for pciutils and zlib... "
> > @echo; /bin/echo -n "Checking for pciutils and zlib... "
>
> Why this change? If it's needed, the "@echo" should also have the
> full path then?

The builtin echo does not do -n. Not sure why. This is GNU Make 3.81
from OSX 10.5.5/Xcode 3.1

Stefan