Skip to contentSkip to conversation

irq: irq and pci_ids patch for Intel Ibex Peak DeviceIDs

PATCH Superseded Seth Heasley 3 participants 6 messages Aug 27, 2008 – Aug 28, 2008

Subsystems (3)
x86 PCI I2C subsystem host drivers
lore
Share & tools
lore

Seth Heasley

author
#0 lore raw

PATCH 1/1 irq: irq and pci_ids patch for Intel Ibex Peak DeviceIDs

This patch updates the Intel Ibex Peak (PCH) LPC and SMBus Controller DeviceIDs.

Signed-off-by Seth Heasley <[email protected]>
include/linux/pci_ids.h renamed from include/linux/pci_ids.h.orig +33 -3
@@ -2428,9 +2428,39 @@
 #define PCI_DEVICE_ID_INTEL_ICH10_3	0x3a1a
 #define PCI_DEVICE_ID_INTEL_ICH10_4	0x3a30
 #define PCI_DEVICE_ID_INTEL_ICH10_5	0x3a60
-#define PCI_DEVICE_ID_INTEL_PCH_0	0x3b10
-#define PCI_DEVICE_ID_INTEL_PCH_1	0x3b11
-#define PCI_DEVICE_ID_INTEL_PCH_2	0x3b30
+#define PCI_DEVICE_ID_INTEL_PCH_0	0x3b00
+#define PCI_DEVICE_ID_INTEL_PCH_1	0x3b01
+#define PCI_DEVICE_ID_INTEL_PCH_2	0x3b02
+#define PCI_DEVICE_ID_INTEL_PCH_3	0x3b03
+#define PCI_DEVICE_ID_INTEL_PCH_4	0x3b04
+#define PCI_DEVICE_ID_INTEL_PCH_5	0x3b05
+#define PCI_DEVICE_ID_INTEL_PCH_6	0x3b06
+#define PCI_DEVICE_ID_INTEL_PCH_7	0x3b07
+#define PCI_DEVICE_ID_INTEL_PCH_8	0x3b08
+#define PCI_DEVICE_ID_INTEL_PCH_9	0x3b09
+#define PCI_DEVICE_ID_INTEL_PCH_10	0x3b0a
+#define PCI_DEVICE_ID_INTEL_PCH_11	0x3b0b
+#define PCI_DEVICE_ID_INTEL_PCH_12	0x3b0c
+#define PCI_DEVICE_ID_INTEL_PCH_13	0x3b0d
+#define PCI_DEVICE_ID_INTEL_PCH_14	0x3b0e
+#define PCI_DEVICE_ID_INTEL_PCH_15	0x3b0f
+#define PCI_DEVICE_ID_INTEL_PCH_16	0x3b10
+#define PCI_DEVICE_ID_INTEL_PCH_17	0x3b11
+#define PCI_DEVICE_ID_INTEL_PCH_18	0x3b12
+#define PCI_DEVICE_ID_INTEL_PCH_19	0x3b13
+#define PCI_DEVICE_ID_INTEL_PCH_20	0x3b14
+#define PCI_DEVICE_ID_INTEL_PCH_21	0x3b15
+#define PCI_DEVICE_ID_INTEL_PCH_22	0x3b16
+#define PCI_DEVICE_ID_INTEL_PCH_23	0x3b17
+#define PCI_DEVICE_ID_INTEL_PCH_24	0x3b18
+#define PCI_DEVICE_ID_INTEL_PCH_25	0x3b19
+#define PCI_DEVICE_ID_INTEL_PCH_26	0x3b1a
+#define PCI_DEVICE_ID_INTEL_PCH_27	0x3b1b
+#define PCI_DEVICE_ID_INTEL_PCH_28	0x3b1c
+#define PCI_DEVICE_ID_INTEL_PCH_29	0x3b1d
+#define PCI_DEVICE_ID_INTEL_PCH_30	0x3b1e
+#define PCI_DEVICE_ID_INTEL_PCH_31	0x3b1f
+#define PCI_DEVICE_ID_INTEL_PCH_32	0x3b30
 #define PCI_DEVICE_ID_INTEL_IOAT_SNB	0x402f
 #define PCI_DEVICE_ID_INTEL_5100_16	0x65f0
 #define PCI_DEVICE_ID_INTEL_5100_21	0x65f5
arch/x86/pci/irq.c renamed from arch/x86/pci/irq.c.orig +30 -0
@@ -592,6 +592,36 @@
 	case PCI_DEVICE_ID_INTEL_ICH10_3:
 	case PCI_DEVICE_ID_INTEL_PCH_0:
 	case PCI_DEVICE_ID_INTEL_PCH_1:
+	case PCI_DEVICE_ID_INTEL_PCH_2:
+	case PCI_DEVICE_ID_INTEL_PCH_3:
+	case PCI_DEVICE_ID_INTEL_PCH_4:
+	case PCI_DEVICE_ID_INTEL_PCH_5:
+	case PCI_DEVICE_ID_INTEL_PCH_6:
+	case PCI_DEVICE_ID_INTEL_PCH_7:
+	case PCI_DEVICE_ID_INTEL_PCH_8:
+	case PCI_DEVICE_ID_INTEL_PCH_9:
+	case PCI_DEVICE_ID_INTEL_PCH_10:
+	case PCI_DEVICE_ID_INTEL_PCH_11:
+	case PCI_DEVICE_ID_INTEL_PCH_12:
+	case PCI_DEVICE_ID_INTEL_PCH_13:
+	case PCI_DEVICE_ID_INTEL_PCH_14:
+	case PCI_DEVICE_ID_INTEL_PCH_15:
+	case PCI_DEVICE_ID_INTEL_PCH_16:
+	case PCI_DEVICE_ID_INTEL_PCH_17:
+	case PCI_DEVICE_ID_INTEL_PCH_18:
+	case PCI_DEVICE_ID_INTEL_PCH_19:
+	case PCI_DEVICE_ID_INTEL_PCH_20:
+	case PCI_DEVICE_ID_INTEL_PCH_21:
+	case PCI_DEVICE_ID_INTEL_PCH_22:
+	case PCI_DEVICE_ID_INTEL_PCH_23:
+	case PCI_DEVICE_ID_INTEL_PCH_24:
+	case PCI_DEVICE_ID_INTEL_PCH_25:
+	case PCI_DEVICE_ID_INTEL_PCH_26:
+	case PCI_DEVICE_ID_INTEL_PCH_27:
+	case PCI_DEVICE_ID_INTEL_PCH_28:
+	case PCI_DEVICE_ID_INTEL_PCH_29:
+	case PCI_DEVICE_ID_INTEL_PCH_30:
+	case PCI_DEVICE_ID_INTEL_PCH_31:
 		r->name = "PIIX/ICH";
 		r->get = pirq_piix_get;
 		r->set = pirq_piix_set;
#1 lore raw

Hi Seth,

Quoted 7 lines · Seth Heasley

This patch updates the Intel Ibex Peak (PCH) LPC and SMBus Controller DeviceIDs.

Signed-off-by Seth Heasley <[email protected]>

include/linux/pci_ids.h line 2433

Quoted 9 lines
include/linux/pci_ids.h renamed from include/linux/pci_ids.h.orig +3 -3
@@ -2428,9 +2428,39 @@
 #define PCI_DEVICE_ID_INTEL_ICH10_3	0x3a1a
 #define PCI_DEVICE_ID_INTEL_ICH10_4	0x3a30
 #define PCI_DEVICE_ID_INTEL_ICH10_5	0x3a60
-#define PCI_DEVICE_ID_INTEL_PCH_0	0x3b10
-#define PCI_DEVICE_ID_INTEL_PCH_1	0x3b11
-#define PCI_DEVICE_ID_INTEL_PCH_2	0x3b30
+#define PCI_DEVICE_ID_INTEL_PCH_0	0x3b00
+#define PCI_DEVICE_ID_INTEL_PCH_1	0x3b01
+#define PCI_DEVICE_ID_INTEL_PCH_2	0x3b02

Changing device ID definitions that way is really bad practice. It needs to be synchronized between all involved subsystems.

Quoted 23 lines
include/linux/pci_ids.h renamed from include/linux/pci_ids.h.orig +23 -0
+#define PCI_DEVICE_ID_INTEL_PCH_3	0x3b03
+#define PCI_DEVICE_ID_INTEL_PCH_4	0x3b04
+#define PCI_DEVICE_ID_INTEL_PCH_5	0x3b05
+#define PCI_DEVICE_ID_INTEL_PCH_6	0x3b06
+#define PCI_DEVICE_ID_INTEL_PCH_7	0x3b07
+#define PCI_DEVICE_ID_INTEL_PCH_8	0x3b08
+#define PCI_DEVICE_ID_INTEL_PCH_9	0x3b09
+#define PCI_DEVICE_ID_INTEL_PCH_10	0x3b0a
+#define PCI_DEVICE_ID_INTEL_PCH_11	0x3b0b
+#define PCI_DEVICE_ID_INTEL_PCH_12	0x3b0c
+#define PCI_DEVICE_ID_INTEL_PCH_13	0x3b0d
+#define PCI_DEVICE_ID_INTEL_PCH_14	0x3b0e
+#define PCI_DEVICE_ID_INTEL_PCH_15	0x3b0f
+#define PCI_DEVICE_ID_INTEL_PCH_16	0x3b10
+#define PCI_DEVICE_ID_INTEL_PCH_17	0x3b11
+#define PCI_DEVICE_ID_INTEL_PCH_18	0x3b12
+#define PCI_DEVICE_ID_INTEL_PCH_19	0x3b13
+#define PCI_DEVICE_ID_INTEL_PCH_20	0x3b14
+#define PCI_DEVICE_ID_INTEL_PCH_21	0x3b15
+#define PCI_DEVICE_ID_INTEL_PCH_22	0x3b16
+#define PCI_DEVICE_ID_INTEL_PCH_23	0x3b17
+#define PCI_DEVICE_ID_INTEL_PCH_24	0x3b18
+#define PCI_DEVICE_ID_INTEL_PCH_25	0x3b19

include/linux/pci_ids.h line 2466

Quoted 10 lines
include/linux/pci_ids.h renamed from include/linux/pci_ids.h.orig +7 -0
+#define PCI_DEVICE_ID_INTEL_PCH_26	0x3b1a
+#define PCI_DEVICE_ID_INTEL_PCH_27	0x3b1b
+#define PCI_DEVICE_ID_INTEL_PCH_28	0x3b1c
+#define PCI_DEVICE_ID_INTEL_PCH_29	0x3b1d
+#define PCI_DEVICE_ID_INTEL_PCH_30	0x3b1e
+#define PCI_DEVICE_ID_INTEL_PCH_31	0x3b1f
+#define PCI_DEVICE_ID_INTEL_PCH_32	0x3b30
 #define PCI_DEVICE_ID_INTEL_IOAT_SNB	0x402f
 #define PCI_DEVICE_ID_INTEL_5100_16	0x65f0
 #define PCI_DEVICE_ID_INTEL_5100_21	0x65f5

BTW, what's the point of these defines? I get the idea of giving nice names to device IDs if these names are explicit and really describe the device. But the Intel south bridge device names are nothing more than arbitrary numbers, so you are replacing a set of arbitrary numbers by another set of arbitrary numbers. I see little benefit in doing this, given the cost it has. For what it's worth, several subsystems (ata, sound...) have already stopped using these defines and I understand them completely: depending on changes done to include/linux/pci_ids.h is more pain than is worth.

Can't we stop defining these IDs now and start using the hexadecimal numbers in all the drivers directly?

Quoted 26 lines
arch/x86/pci/irq.c renamed from arch/x86/pci/irq.c.orig +20 -0
@@ -592,6 +592,36 @@
 	case PCI_DEVICE_ID_INTEL_ICH10_3:
 	case PCI_DEVICE_ID_INTEL_PCH_0:
 	case PCI_DEVICE_ID_INTEL_PCH_1:
+	case PCI_DEVICE_ID_INTEL_PCH_2:
+	case PCI_DEVICE_ID_INTEL_PCH_3:
+	case PCI_DEVICE_ID_INTEL_PCH_4:
+	case PCI_DEVICE_ID_INTEL_PCH_5:
+	case PCI_DEVICE_ID_INTEL_PCH_6:
+	case PCI_DEVICE_ID_INTEL_PCH_7:
+	case PCI_DEVICE_ID_INTEL_PCH_8:
+	case PCI_DEVICE_ID_INTEL_PCH_9:
+	case PCI_DEVICE_ID_INTEL_PCH_10:
+	case PCI_DEVICE_ID_INTEL_PCH_11:
+	case PCI_DEVICE_ID_INTEL_PCH_12:
+	case PCI_DEVICE_ID_INTEL_PCH_13:
+	case PCI_DEVICE_ID_INTEL_PCH_14:
+	case PCI_DEVICE_ID_INTEL_PCH_15:
+	case PCI_DEVICE_ID_INTEL_PCH_16:
+	case PCI_DEVICE_ID_INTEL_PCH_17:
+	case PCI_DEVICE_ID_INTEL_PCH_18:
+	case PCI_DEVICE_ID_INTEL_PCH_19:
+	case PCI_DEVICE_ID_INTEL_PCH_20:
+	case PCI_DEVICE_ID_INTEL_PCH_21:

arch/x86/pci/irq.c line 624

Quoted 10 lines
arch/x86/pci/irq.c renamed from arch/x86/pci/irq.c.orig +10 -0
+	case PCI_DEVICE_ID_INTEL_PCH_22:
+	case PCI_DEVICE_ID_INTEL_PCH_23:
+	case PCI_DEVICE_ID_INTEL_PCH_24:
+	case PCI_DEVICE_ID_INTEL_PCH_25:
+	case PCI_DEVICE_ID_INTEL_PCH_26:
+	case PCI_DEVICE_ID_INTEL_PCH_27:
+	case PCI_DEVICE_ID_INTEL_PCH_28:
+	case PCI_DEVICE_ID_INTEL_PCH_29:
+	case PCI_DEVICE_ID_INTEL_PCH_30:
+	case PCI_DEVICE_ID_INTEL_PCH_31:

I am no PCI IRQ routing expert, but I have to admit that I a bit skeptical that all the PCH functions are IRQ routers. You're adding as many entries here for the PCH than there have been for all Intel chips in the past 10 years or so...

Quoted 3 lines
arch/x86/pci/irq.c renamed from arch/x86/pci/irq.c.orig +0 -0
 		r->name = "PIIX/ICH";
 		r->get = pirq_piix_get;
 		r->set = pirq_piix_set;
Signature · 1 line
Jean Delvare
#2 lore raw
Quoted 57 lines · Jean Delvare

Hi Seth,

Quoted 11 lines · Seth Heasley

This patch updates the Intel Ibex Peak (PCH) LPC and SMBus Controller DeviceIDs.

Signed-off-by Seth Heasley <[email protected]>

--- linux-2.6/include/linux/pci_ids.h.orig 2008-08-27 11:54:07.000000000
-0700 +++ linux-2.6/include/linux/pci_ids.h 2008-08-27 12:01:53.000000000
-0700 @@ -2428,9 +2428,39 @@
#define PCI_DEVICE_ID_INTEL_ICH10_3 0x3a1a
#define PCI_DEVICE_ID_INTEL_ICH10_4 0x3a30
#define PCI_DEVICE_ID_INTEL_ICH10_5 0x3a60

include/linux/pci_ids.h line 2433

Quoted 6 lines
include/linux/pci_ids.h renamed from include/linux/pci_ids.h.orig +3 -3
-#define PCI_DEVICE_ID_INTEL_PCH_0	0x3b10
-#define PCI_DEVICE_ID_INTEL_PCH_1	0x3b11
-#define PCI_DEVICE_ID_INTEL_PCH_2	0x3b30
+#define PCI_DEVICE_ID_INTEL_PCH_0	0x3b00
+#define PCI_DEVICE_ID_INTEL_PCH_1	0x3b01
+#define PCI_DEVICE_ID_INTEL_PCH_2	0x3b02

Changing device ID definitions that way is really bad practice. It needs to be synchronized between all involved subsystems.

Quoted 33 lines
arch/x86/pci/irq.c renamed from arch/x86/pci/irq.c.orig +27 -0
@@ -592,6 +592,36 @@
 	case PCI_DEVICE_ID_INTEL_ICH10_3:
 	case PCI_DEVICE_ID_INTEL_PCH_0:
 	case PCI_DEVICE_ID_INTEL_PCH_1:
+	case PCI_DEVICE_ID_INTEL_PCH_2:
+	case PCI_DEVICE_ID_INTEL_PCH_3:
+	case PCI_DEVICE_ID_INTEL_PCH_4:
+	case PCI_DEVICE_ID_INTEL_PCH_5:
+	case PCI_DEVICE_ID_INTEL_PCH_6:
+	case PCI_DEVICE_ID_INTEL_PCH_7:
+	case PCI_DEVICE_ID_INTEL_PCH_8:
+	case PCI_DEVICE_ID_INTEL_PCH_9:
+	case PCI_DEVICE_ID_INTEL_PCH_10:
+	case PCI_DEVICE_ID_INTEL_PCH_11:
+	case PCI_DEVICE_ID_INTEL_PCH_12:
+	case PCI_DEVICE_ID_INTEL_PCH_13:
+	case PCI_DEVICE_ID_INTEL_PCH_14:
+	case PCI_DEVICE_ID_INTEL_PCH_15:
+	case PCI_DEVICE_ID_INTEL_PCH_16:
+	case PCI_DEVICE_ID_INTEL_PCH_17:
+	case PCI_DEVICE_ID_INTEL_PCH_18:
+	case PCI_DEVICE_ID_INTEL_PCH_19:
+	case PCI_DEVICE_ID_INTEL_PCH_20:
+	case PCI_DEVICE_ID_INTEL_PCH_21:
+	case PCI_DEVICE_ID_INTEL_PCH_22:
+	case PCI_DEVICE_ID_INTEL_PCH_23:
+	case PCI_DEVICE_ID_INTEL_PCH_24:
+	case PCI_DEVICE_ID_INTEL_PCH_25:
+	case PCI_DEVICE_ID_INTEL_PCH_26:
+	case PCI_DEVICE_ID_INTEL_PCH_27:
+	case PCI_DEVICE_ID_INTEL_PCH_28:
Quoted 12 lines

arch/x86/pci/irq.c line 624

Quoted 3 lines
arch/x86/pci/irq.c renamed from arch/x86/pci/irq.c.orig +3 -0
+	case PCI_DEVICE_ID_INTEL_PCH_29:
+	case PCI_DEVICE_ID_INTEL_PCH_30:
+	case PCI_DEVICE_ID_INTEL_PCH_31:

I am no PCI IRQ routing expert, but I have to admit that I a bit skeptical that all the PCH functions are IRQ routers. You're adding as many entries here for the PCH than there have been for all Intel chips in the past 10 years or so...

Quoted 3 lines
arch/x86/pci/irq.c renamed from arch/x86/pci/irq.c.orig +0 -0
 		r->name = "PIIX/ICH";
 		r->get = pirq_piix_get;
 		r->set = pirq_piix_set;

Yeah, this has me confused now too. I remember specifically asking if the other PCHs needed to be added to this list when the last patch was applied. What happened? Can you give us some more background here, Seth? The changelog should definitely include an explanation of why the IDs need to be changed (i.e. why the old commit was wrong).

Thanks,

Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
author
#3 lore raw
Quoted 81 lines · Jean Delvare
Quoted 9 lines

Hi Seth,

Quoted 6 lines · Seth Heasley

This patch updates the Intel Ibex Peak (PCH) LPC and SMBus Controller DeviceIDs.

Signed-off-by Seth Heasley <[email protected]>

--- linux-2.6/include/linux/pci_ids.h.orig 2008-08-27

11:54:07.000000000

Quoted 1 line
Quoted 1 line

-0700 +++ linux-2.6/include/linux/pci_ids.h 2008-08-27

12:01:53.000000000

Quoted 5 lines
Quoted 5 lines

-0700 @@ -2428,9 +2428,39 @@

 #define PCI_DEVICE_ID_INTEL_ICH10_3        0x3a1a
 #define PCI_DEVICE_ID_INTEL_ICH10_4        0x3a30
 #define PCI_DEVICE_ID_INTEL_ICH10_5        0x3a60

-#define PCI_DEVICE_ID_INTEL_PCH_0 0x3b10

Quoted 10 lines

include/linux/pci_ids.h line 2433

Quoted 5 lines
include/linux/pci_ids.h renamed from include/linux/pci_ids.h.orig +3 -2
-#define PCI_DEVICE_ID_INTEL_PCH_1  0x3b11
-#define PCI_DEVICE_ID_INTEL_PCH_2  0x3b30
+#define PCI_DEVICE_ID_INTEL_PCH_0  0x3b00
+#define PCI_DEVICE_ID_INTEL_PCH_1  0x3b01
+#define PCI_DEVICE_ID_INTEL_PCH_2  0x3b02

Changing device ID definitions that way is really bad practice. It needs to be synchronized between all involved subsystems.

Quoted 1 line

--- linux-2.6/arch/x86/pci/irq.c.orig 2008-08-27

11:53:13.000000000 -0700

Quoted 32 lines
Quoted 32 lines

+++ linux-2.6/arch/x86/pci/irq.c 2008-08-27 12:07:21.000000000 -0700

arch/x86/pci/irq.c renamed from arch/x86/pci/irq.c.orig +27 -0
@@ -592,6 +592,36 @@
    case PCI_DEVICE_ID_INTEL_ICH10_3:
    case PCI_DEVICE_ID_INTEL_PCH_0:
    case PCI_DEVICE_ID_INTEL_PCH_1:
+   case PCI_DEVICE_ID_INTEL_PCH_2:
+   case PCI_DEVICE_ID_INTEL_PCH_3:
+   case PCI_DEVICE_ID_INTEL_PCH_4:
+   case PCI_DEVICE_ID_INTEL_PCH_5:
+   case PCI_DEVICE_ID_INTEL_PCH_6:
+   case PCI_DEVICE_ID_INTEL_PCH_7:
+   case PCI_DEVICE_ID_INTEL_PCH_8:
+   case PCI_DEVICE_ID_INTEL_PCH_9:
+   case PCI_DEVICE_ID_INTEL_PCH_10:
+   case PCI_DEVICE_ID_INTEL_PCH_11:
+   case PCI_DEVICE_ID_INTEL_PCH_12:
+   case PCI_DEVICE_ID_INTEL_PCH_13:
+   case PCI_DEVICE_ID_INTEL_PCH_14:
+   case PCI_DEVICE_ID_INTEL_PCH_15:
+   case PCI_DEVICE_ID_INTEL_PCH_16:
+   case PCI_DEVICE_ID_INTEL_PCH_17:
+   case PCI_DEVICE_ID_INTEL_PCH_18:
+   case PCI_DEVICE_ID_INTEL_PCH_19:
+   case PCI_DEVICE_ID_INTEL_PCH_20:
+   case PCI_DEVICE_ID_INTEL_PCH_21:
+   case PCI_DEVICE_ID_INTEL_PCH_22:
+   case PCI_DEVICE_ID_INTEL_PCH_23:
+   case PCI_DEVICE_ID_INTEL_PCH_24:
+   case PCI_DEVICE_ID_INTEL_PCH_25:
+   case PCI_DEVICE_ID_INTEL_PCH_26:
+   case PCI_DEVICE_ID_INTEL_PCH_27:
+   case PCI_DEVICE_ID_INTEL_PCH_28:
Quoted 12 lines

arch/x86/pci/irq.c line 624

Quoted 3 lines
arch/x86/pci/irq.c renamed from arch/x86/pci/irq.c.orig +3 -0
+   case PCI_DEVICE_ID_INTEL_PCH_29:
+   case PCI_DEVICE_ID_INTEL_PCH_30:
+   case PCI_DEVICE_ID_INTEL_PCH_31:

I am no PCI IRQ routing expert, but I have to admit that I a bit skeptical that all the PCH functions are IRQ routers. You're adding as many entries here for the PCH than there have been for all Intel chips in the past 10 years or so...

Quoted 3 lines
arch/x86/pci/irq.c renamed from arch/x86/pci/irq.c.orig +0 -0
            r->name = "PIIX/ICH";
            r->get = pirq_piix_get;
            r->set = pirq_piix_set;

Yeah, this has me confused now too. I remember specifically asking if the other PCHs needed to be added to this list when the last patch was applied. What happened? Can you give us some more background here, Seth? The changelog should definitely include an explanation of why the IDs need to be
changed (i.e. why the old commit was wrong).

Quoted 3 lines

Thanks,
--
Jesse Barnes, Intel Open Source Technology Center

I'm not sure what's "messy," given that I changed what was required to be changed, then added the new IDs. These IDs are as follows:

LPC Controller: 3b00-3b1f (final ID set in Firmware, but 32 possibilities)
SMBus: 3b30

In terms of the irq stuff, I'm adding only the LPC Controller IDs there. There are just a lot of them. Normally we have a handful of IDs, but in this case the list is longer.

I suppose what "messy" means is, I should have kept the existing defines and only added the new? I can resubmit if that's the way it should be done.

I will also patch the Documentation file. It wasn't in the list I was given by Jason, but I'm onboard with saving Jean the trouble.

Please advise on how I should proceed here.

-Seth

#4 lore raw
Quoted 88 lines · Seth
Quoted 81 lines · Jean Delvare
Quoted 9 lines

Hi Seth,

Quoted 6 lines · Seth Heasley

This patch updates the Intel Ibex Peak (PCH) LPC and SMBus Controller DeviceIDs.

Signed-off-by Seth Heasley <[email protected]>

--- linux-2.6/include/linux/pci_ids.h.orig 2008-08-27

11:54:07.000000000

Quoted 1 line
Quoted 1 line

-0700 +++ linux-2.6/include/linux/pci_ids.h 2008-08-27

12:01:53.000000000

Quoted 5 lines
Quoted 5 lines

-0700 @@ -2428,9 +2428,39 @@

 #define PCI_DEVICE_ID_INTEL_ICH10_3        0x3a1a
 #define PCI_DEVICE_ID_INTEL_ICH10_4        0x3a30
 #define PCI_DEVICE_ID_INTEL_ICH10_5        0x3a60

-#define PCI_DEVICE_ID_INTEL_PCH_0 0x3b10

Quoted 10 lines

include/linux/pci_ids.h line 2433

Quoted 5 lines
include/linux/pci_ids.h renamed from include/linux/pci_ids.h.orig +3 -2
-#define PCI_DEVICE_ID_INTEL_PCH_1  0x3b11
-#define PCI_DEVICE_ID_INTEL_PCH_2  0x3b30
+#define PCI_DEVICE_ID_INTEL_PCH_0  0x3b00
+#define PCI_DEVICE_ID_INTEL_PCH_1  0x3b01
+#define PCI_DEVICE_ID_INTEL_PCH_2  0x3b02

Changing device ID definitions that way is really bad practice. It needs to be synchronized between all involved subsystems.

Quoted 1 line

--- linux-2.6/arch/x86/pci/irq.c.orig 2008-08-27

11:53:13.000000000 -0700

Quoted 32 lines
Quoted 32 lines

+++ linux-2.6/arch/x86/pci/irq.c 2008-08-27 12:07:21.000000000 -0700

arch/x86/pci/irq.c renamed from arch/x86/pci/irq.c.orig +27 -0
@@ -592,6 +592,36 @@
    case PCI_DEVICE_ID_INTEL_ICH10_3:
    case PCI_DEVICE_ID_INTEL_PCH_0:
    case PCI_DEVICE_ID_INTEL_PCH_1:
+   case PCI_DEVICE_ID_INTEL_PCH_2:
+   case PCI_DEVICE_ID_INTEL_PCH_3:
+   case PCI_DEVICE_ID_INTEL_PCH_4:
+   case PCI_DEVICE_ID_INTEL_PCH_5:
+   case PCI_DEVICE_ID_INTEL_PCH_6:
+   case PCI_DEVICE_ID_INTEL_PCH_7:
+   case PCI_DEVICE_ID_INTEL_PCH_8:
+   case PCI_DEVICE_ID_INTEL_PCH_9:
+   case PCI_DEVICE_ID_INTEL_PCH_10:
+   case PCI_DEVICE_ID_INTEL_PCH_11:
+   case PCI_DEVICE_ID_INTEL_PCH_12:
+   case PCI_DEVICE_ID_INTEL_PCH_13:
+   case PCI_DEVICE_ID_INTEL_PCH_14:
+   case PCI_DEVICE_ID_INTEL_PCH_15:
+   case PCI_DEVICE_ID_INTEL_PCH_16:
+   case PCI_DEVICE_ID_INTEL_PCH_17:
+   case PCI_DEVICE_ID_INTEL_PCH_18:
+   case PCI_DEVICE_ID_INTEL_PCH_19:
+   case PCI_DEVICE_ID_INTEL_PCH_20:
+   case PCI_DEVICE_ID_INTEL_PCH_21:
+   case PCI_DEVICE_ID_INTEL_PCH_22:
+   case PCI_DEVICE_ID_INTEL_PCH_23:
+   case PCI_DEVICE_ID_INTEL_PCH_24:
+   case PCI_DEVICE_ID_INTEL_PCH_25:
+   case PCI_DEVICE_ID_INTEL_PCH_26:
+   case PCI_DEVICE_ID_INTEL_PCH_27:
+   case PCI_DEVICE_ID_INTEL_PCH_28:
Quoted 12 lines

arch/x86/pci/irq.c line 624

Quoted 3 lines
arch/x86/pci/irq.c renamed from arch/x86/pci/irq.c.orig +3 -0
+   case PCI_DEVICE_ID_INTEL_PCH_29:
+   case PCI_DEVICE_ID_INTEL_PCH_30:
+   case PCI_DEVICE_ID_INTEL_PCH_31:

I am no PCI IRQ routing expert, but I have to admit that I a bit skeptical that all the PCH functions are IRQ routers. You're adding as many entries here for the PCH than there have been for all Intel chips in the past 10 years or so...

Quoted 3 lines
arch/x86/pci/irq.c renamed from arch/x86/pci/irq.c.orig +0 -0
            r->name = "PIIX/ICH";
            r->get = pirq_piix_get;
            r->set = pirq_piix_set;

Yeah, this has me confused now too. I remember specifically asking if the other PCHs needed to be added to this list when the last patch was applied.. What happened? Can you give us some more background here, Seth? The changelog should definitely include an explanation of why the IDs need to be
changed (i.e. why the old commit was wrong).

Quoted 3 lines

Thanks,
--
Jesse Barnes, Intel Open Source Technology Center

I'm not sure what's "messy," given that I changed what was required to be changed, then added the new IDs. These IDs are as follows:

Quoted 6 lines

LPC Controller: 3b00-3b1f (final ID set in Firmware, but 32 possibilities)
SMBus: 3b30

In terms of the irq stuff, I'm adding only the LPC Controller IDs there. There are just a lot of them. Normally we have a handful of IDs, but in this case the list is longer.

So basically, you (Intel) have no clue what IDs will be used, and instead of waiting until you have the information, you prefer to add a bunch of device IDs to the Linux kernel, most of which will never exist? This is making the kernel code bigger and slower. Not by much, but still... That's bad engineering, really. Especially if you don't clean up afterwards, as happens to be the case for the ICH10. I compared the ICH10 datasheet with the IDs we have in the kernel and only 3 of the 6 defined device IDs are actually used by existing ICH10 chips.

Quoted 3 lines

I suppose what "messy" means is, I should have kept the existing defines and only added the new? I can resubmit if that's the way it should be done.

That's one part of the messiness, yes. Which underlines how bad the symbol naming scheme is to start with.

Quoted 2 lines

I will also patch the Documentation file. It wasn't in the list I was given by Jason, but I'm onboard with saving Jean the trouble.

Thanks.

Quoted 1 line

Please advise on how I should proceed here.

I gave my opinion on it - now up to Jesse to decide.

Signature · 1 line
Jean Delvare
author
#5 lore raw
Quoted 102 lines · Seth
Quoted 5 lines
Quoted 5 lines · Jean Delvare
Quoted 4 lines

Hi Seth,

Quoted 1 line · Seth Heasley

This patch updates the Intel Ibex Peak (PCH) LPC and SMBus

Controller

Quoted 22 lines
Quoted 22 lines
Quoted 5 lines
Quoted 5 lines

DeviceIDs.

Signed-off-by Seth Heasley <[email protected]>

--- linux-2.6/include/linux/pci_ids.h.orig 2008-08-27

11:54:07.000000000

Quoted 1 line
Quoted 1 line

-0700 +++ linux-2.6/include/linux/pci_ids.h 2008-08-27

12:01:53.000000000

Quoted 14 lines

include/linux/pci_ids.h line 2433

Quoted 10 lines

-0700 @@ -2428,9 +2428,39 @@

 #define PCI_DEVICE_ID_INTEL_ICH10_3        0x3a1a
 #define PCI_DEVICE_ID_INTEL_ICH10_4        0x3a30
 #define PCI_DEVICE_ID_INTEL_ICH10_5        0x3a60
include/linux/pci_ids.h renamed from include/linux/pci_ids.h.orig +3 -3
-#define PCI_DEVICE_ID_INTEL_PCH_0  0x3b10
-#define PCI_DEVICE_ID_INTEL_PCH_1  0x3b11
-#define PCI_DEVICE_ID_INTEL_PCH_2  0x3b30
+#define PCI_DEVICE_ID_INTEL_PCH_0  0x3b00
+#define PCI_DEVICE_ID_INTEL_PCH_1  0x3b01
+#define PCI_DEVICE_ID_INTEL_PCH_2  0x3b02

Changing device ID definitions that way is really bad practice. It needs to be synchronized between all involved subsystems.

Quoted 3 lines
Quoted 3 lines
Quoted 1 line
Quoted 1 line

--- linux-2.6/arch/x86/pci/irq.c.orig 2008-08-27

11:53:13.000000000 -0700

Quoted 1 line
Quoted 1 line

+++ linux-2.6/arch/x86/pci/irq.c 2008-08-27 12:07:21.000000000 -

0700

Quoted 40 lines
Quoted 40 lines
Quoted 40 lines
Quoted 24 lines
arch/x86/pci/irq.c renamed from arch/x86/pci/irq.c.orig +20 -0
@@ -592,6 +592,36 @@
    case PCI_DEVICE_ID_INTEL_ICH10_3:
    case PCI_DEVICE_ID_INTEL_PCH_0:
    case PCI_DEVICE_ID_INTEL_PCH_1:
+   case PCI_DEVICE_ID_INTEL_PCH_2:
+   case PCI_DEVICE_ID_INTEL_PCH_3:
+   case PCI_DEVICE_ID_INTEL_PCH_4:
+   case PCI_DEVICE_ID_INTEL_PCH_5:
+   case PCI_DEVICE_ID_INTEL_PCH_6:
+   case PCI_DEVICE_ID_INTEL_PCH_7:
+   case PCI_DEVICE_ID_INTEL_PCH_8:
+   case PCI_DEVICE_ID_INTEL_PCH_9:
+   case PCI_DEVICE_ID_INTEL_PCH_10:
+   case PCI_DEVICE_ID_INTEL_PCH_11:
+   case PCI_DEVICE_ID_INTEL_PCH_12:
+   case PCI_DEVICE_ID_INTEL_PCH_13:
+   case PCI_DEVICE_ID_INTEL_PCH_14:
+   case PCI_DEVICE_ID_INTEL_PCH_15:
+   case PCI_DEVICE_ID_INTEL_PCH_16:
+   case PCI_DEVICE_ID_INTEL_PCH_17:
+   case PCI_DEVICE_ID_INTEL_PCH_18:
+   case PCI_DEVICE_ID_INTEL_PCH_19:
+   case PCI_DEVICE_ID_INTEL_PCH_20:
+   case PCI_DEVICE_ID_INTEL_PCH_21:

arch/x86/pci/irq.c line 624

Quoted 10 lines
arch/x86/pci/irq.c renamed from arch/x86/pci/irq.c.orig +10 -0
+   case PCI_DEVICE_ID_INTEL_PCH_22:
+   case PCI_DEVICE_ID_INTEL_PCH_23:
+   case PCI_DEVICE_ID_INTEL_PCH_24:
+   case PCI_DEVICE_ID_INTEL_PCH_25:
+   case PCI_DEVICE_ID_INTEL_PCH_26:
+   case PCI_DEVICE_ID_INTEL_PCH_27:
+   case PCI_DEVICE_ID_INTEL_PCH_28:
+   case PCI_DEVICE_ID_INTEL_PCH_29:
+   case PCI_DEVICE_ID_INTEL_PCH_30:
+   case PCI_DEVICE_ID_INTEL_PCH_31:

I am no PCI IRQ routing expert, but I have to admit that I a bit skeptical that all the PCH functions are IRQ routers. You're adding as many entries here for the PCH than there have been for all Intel chips in the past 10 years or so...

Quoted 5 lines
Quoted 5 lines
Quoted 3 lines

arch/x86/pci/irq.c line 627

Quoted 3 lines
arch/x86/pci/irq.c renamed from arch/x86/pci/irq.c.orig +0 -0
            r->name = "PIIX/ICH";
            r->get = pirq_piix_get;
            r->set = pirq_piix_set;

Yeah, this has me confused now too. I remember specifically asking if

the

Quoted 1 line
Quoted 1 line

other PCHs needed to be added to this list when the last patch was

applied..

Quoted 2 lines
Quoted 2 lines

What happened? Can you give us some more background here, Seth? The changelog should definitely include an explanation of why the IDs need

to

Quoted 11 lines
Quoted 6 lines

be
changed (i.e. why the old commit was wrong).

Thanks,
--
Jesse Barnes, Intel Open Source Technology Center

I'm not sure what's "messy," given that I changed what was required to be changed, then added the new IDs. These IDs are as follows:

LPC Controller: 3b00-3b1f (final ID set in Firmware, but 32

possibilities)

Quoted 5 lines

SMBus: 3b30

In terms of the irq stuff, I'm adding only the LPC Controller IDs there. There are just a lot of them. Normally we have a handful of IDs, but in this case the list is longer.

Quoted 9 lines

So basically, you (Intel) have no clue what IDs will be used, and instead of waiting until you have the information, you prefer to add a bunch of device IDs to the Linux kernel, most of which will never exist? This is making the kernel code bigger and slower. Not by much, but still... That's bad engineering, really. Especially if you don't clean up afterwards, as happens to be the case for the ICH10. I compared the ICH10 datasheet with the IDs we have in the kernel and only 3 of the 6 defined device IDs are actually used by existing ICH10 chips.

I can't speak to the engineering, although I agree that blocking out 32 IDs is strange. I don't believe it's a matter of not knowing what will be used but rather that the firmware has the flexibility to change the ID in that range.

On the ICH10 side, we don't have a datasheet released yet for one of the skus of the product (the other sku has not launched). The other three IDs are from that sku. No cleanup will be needed.

Quoted 8 lines
Quoted 4 lines

I suppose what "messy" means is, I should have kept the existing defines and only added the new? I can resubmit if that's the way it should be done.

That's one part of the messiness, yes. Which underlines how bad the symbol naming scheme is to start with.

I have no history here, but I'm inclined to agree it's a strange naming scheme.

-Seth