See patch.
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.
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
Stefan Reinauer wrote:
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.
Does the msrtool configure script run well on Darwin?
//Peter
Peter Stuge wrote:
Stefan Reinauer wrote:
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.
Does the msrtool configure script run well on Darwin?
Only a very quick test:
./configure -n searching for compiler (gcc cc icc)... ./configure: line 28: seq: command not found using in PATH -n searching for install (install ginstall)... ./configure: line 28: seq: command not found using in PATH
configured using the following settings:
VERSION=3766 CC= CFLAGS= -Os -Wall -Werror LDFLAGS= INSTALL= PREFIX=/usr/local
-n creating Makefile... done
on 04/12/2008 11:59 Stefan Reinauer said the following:
Peter Stuge wrote:
Stefan Reinauer wrote:
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.
Does the msrtool configure script run well on Darwin?
Only a very quick test:
./configure -n searching for compiler (gcc cc icc)... ./configure: line 28: seq: command not found using in PATH -n searching for install (install ginstall)... ./configure: line 28: seq: command not found using in PATH
configured using the following settings:
VERSION=3766 CC= CFLAGS= -Os -Wall -Werror LDFLAGS= INSTALL= PREFIX=/usr/local
-n creating Makefile... done
I tried it on FreeBSD with the following tweaks: replace seq with gseq and explicitly use /usr/local/bin/bash to execute configure (it's not in POSIX shell syntax):
$ bash ./configure searching for compiler (gcc cc icc)... using /usr/bin/gcc in PATH searching for install (install ginstall)... using /usr/bin/install in PATH finding CFLAGS for libpci (from pciutils)... using: -Os -Wall -Werror -I/usr/local/include finding LDFLAGS for libpci (from pciutils)... using: -L/usr/local/lib -lpci -lz
configured using the following settings:
VERSION=3770M CC=gcc CFLAGS= -Os -Wall -Werror -I/usr/local/include LDFLAGS= -L/usr/local/lib -lpci -lz INSTALL=install PREFIX=/usr/local
creating Makefile... done
Am Donnerstag, den 04.12.2008, 10:59 +0100 schrieb Stefan Reinauer:
./configure -n searching for compiler (gcc cc icc)... ./configure: line 28: seq: command not found using in PATH -n searching for install (install ginstall)...
That looks like "echo -n" - printf would be better
./configure: line 28: seq: command not found using in PATH
For this, if [ `uname -s` = "Darwin" ]; then seq() { if [ $# -eq 3 ]; then jot $(( ($3 - $1 + $2)/$2 )) $1 $3 else jot $2 $1 $2 fi } fi
should work. I'm not sure about the uname -s, and it should also cover the BSDs (which also have jot, but not seq). Also, the $(( )) syntax might or might not be supported, and probably should be replaced by `echo ... | bc` or something like that.
Regards, Patrick
on 04/12/2008 01:56 Stefan Reinauer said the following:
Uwe Hermann wrote:
...
On Sun, Nov 30, 2008 at 10:07:55PM +0100, Stefan Reinauer wrote:
...
/* 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.
From publicly available ICH9 spec (I guess ICH8 is the same here):
<quote> LV2 — Level 2 Register I/O Address: PMBASE + 14h ... Reads to this register return all 0s, writes to this register have no effect. Reads to this register generate a “enter a level 2 power state” (C2) to the clock control logic. This will cause the STPCLK# signal to go active, and stay active until a break event occurs. Throttling (due either to THTL_EN or FORCE_THTL) will be ignored. ... NOTE: This register should not be used by IA-64 processors or systems with more than 1 logical processor, unless appropriate semaphoring software has been put in place to ensure that all threads/processors are ready for the C2 state when the read to this register occurs. </quote>
The subsequent byte registers are LV3, LV4, LV5 and for mobile version LV6 intended for entering C3, C4, C5 and C6 states.
Hi Andriy,
excellent! This made me add the long overdue register descriptions for the PMBASE area to the patch, which I can use very well for the project I'm working on.
Attached is a new patch that should solve the issues the right way.
Andriy Gapon wrote:
From publicly available ICH9 spec (I guess ICH8 is the same here):
<quote> LV2 — Level 2 Register I/O Address: PMBASE + 14h ... Reads to this register return all 0s, writes to this register have no effect. Reads to this register generate a “enter a level 2 power state” (C2) to the clock control logic. This will cause the STPCLK# signal to go active, and stay active until a break event occurs. Throttling (due either to THTL_EN or FORCE_THTL) will be ignored. ... NOTE: This register should not be used by IA-64 processors or systems with more than 1 logical processor, unless appropriate semaphoring software has been put in place to ensure that all threads/processors are ready for the C2 state when the read to this register occurs. </quote>
The subsequent byte registers are LV3, LV4, LV5 and for mobile version LV6 intended for entering C3, C4, C5 and C6 states.
On Thu, Dec 04, 2008 at 02:44:45PM +0100, Stefan Reinauer wrote:
Hi Andriy,
excellent! This made me add the long overdue register descriptions for the PMBASE area to the patch, which I can use very well for the project I'm working on.
Attached is a new patch that should solve the issues the right way.
Oops, didn't notice the new patch earlier, please ignore my other ACK and commit this one.
Acked-by: Uwe Hermann uwe@hermann-uwe.de
Uwe.
Uwe Hermann wrote:
On Thu, Dec 04, 2008 at 02:44:45PM +0100, Stefan Reinauer wrote:
Hi Andriy,
excellent! This made me add the long overdue register descriptions for the PMBASE area to the patch, which I can use very well for the project I'm working on.
Attached is a new patch that should solve the issues the right way.
Oops, didn't notice the new patch earlier, please ignore my other ACK and commit this one.
Acked-by: Uwe Hermann uwe@hermann-uwe.de
Uwe.
Thanks, r3794