lkml.org 
[lkml]   [2008]   [Aug]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRE: [PATCH 2.6.27-rc4] irq: irq and pci_ids patch for Intel Ibex Peak DeviceIDs
    >On Thu, 28 Aug 2008 08:18:19 -0700, Heasley, Seth wrote:
    >> >On Thursday, August 28, 2008 12:49 am Jean Delvare wrote:
    >> >> Hi Seth,
    >> >>
    >> >> On Wed, 27 Aug 2008 16:58:26 -0700, Seth Heasley wrote:
    >> >> > This patch updates the Intel Ibex Peak (PCH) LPC and SMBus
    >Controller
    >> >> > DeviceIDs.
    >> >> >
    >> >> > Signed-off-by: Seth Heasley <seth.heasley@intel.com>
    >> >> >
    >> >> > --- 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
    >> >> > -#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.
    >> >>
    >> >> > --- linux-2.6/arch/x86/pci/irq.c.orig 2008-08-27
    >> >11:53:13.000000000 -0700
    >> >> > +++ linux-2.6/arch/x86/pci/irq.c 2008-08-27 12:07:21.000000000 -
    >0700
    >> >> > @@ -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:
    >> >>
    >> >> 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...
    >> >>
    >> >> > 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,
    >> >--
    >> >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.
    >
    >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.

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


    \
     
     \ /
      Last update: 2008-08-28 17:45    [W:4.067 / U:0.868 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site