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

[Uwe Hermann]

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


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


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


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


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


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


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


Uwe.
-- 
http://www.hermann-uwe.de  | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org