On Fri, Jun 24, 2011 at 9:35 PM, Mark Kettenis <[email protected]>
wrote:
>> From: Jonathan Matthew
>> Date: Fri, 24 Jun 2011 16:49:50 +1000
>>
>> > Which shows how badly the chosen name for that function really is.
>> >
>> > I did some digging into the issue. If you look at 3400 docs, you'll
>> > see that description of the AHCI_REG_CAP registers says that the BIOS
>> > should set the BIOS should set the AHCI_REG_CAP_SPM bit (the port
>> > multiplier capability bit) to 0. The documented default value for
>> > this register has the bit set to 1 though. Also, the revision history
>> > mentions the removal of port multiplier functionality. I guess what's
>> > happened here is that Intel tried to support port multipliers,
>> > discovered that it didn't work properly and told BIOS developers to
>> > disable it. Unsurprisingly some BIOS developers didn't get the memo
>> > and left the bit in its default state. So it's obvious that all
>> > 3400/5-Series chipset AHCI variants are affected, and we should apply
>> > the same fix to all of them.
>> >
>> > I looked at other chipsets as well, and the documentation strongly
>> > suggests that the older (ICH) chipsets have the same issue. The
>> > specified default value has the bit turned on, but the bit is either
>> > undocumented or is documented as "set to 0 by the BIOS". And even the
>> > new 6-series PCH seems to be affected. On the other hand Intel still
>> > claims that port multipliers are supported by some of the ICH10
>> > variants. I'm really curious wether there are any machines where port
>> > multipliers work on the Intel AHCI controller ports. If not, perhaps
>> > we should blacklist all of the Intel chipsets.
>>
>> The only mentions I can see on Intel's website of port multipliers
>> being supported are on overview pages for 3400 and ICH9, and when you
>> look at the data sheets for those, the revision history says it was
>> removed. For all the other chipsets I looked at, they don't mention it
>> on the overview page and say "port multipliers not supported" in the
>> data sheet. I'm not sure what to make of the default values for the
>> AHCI capabilities register - several of the other bits don't seem to
>> match up with the descriptions, even in cases where they haven't
>> withdrawn support for a feature.
>
> But the default value, and the language describing the SPM bit really
> is very similar across ICH7, ICH8, ICH10, 3400/5-Series and
> C200/6-Series. For all these chipsets the datasheet says that the
> BIOS should set the bit to 0. That is no accident. It is the same
> piece of VHDL (or whatever Intel uses to design the logic) that's
> simply evolving a bit from generation to generation. But the port
> multiplier functionality stays non-functional. People have reported
> issues with ICH7, ICH9 and 3400. So really, all ICH and PCH variants
> are affected and we should quirk them all.
Good point, I agree. Easier to list them all now rather than waiting
for people to report them one by one.
>> > Arguably, the chosen fix (the AHCI_F_IPMS_PROBE quirk) isn't quite
>> > right. It is clear that (some) Intel AHCI variants simply don't
>> > support port multipliers, so we should just skip the probe. On the
>> > other hand, as long as this quirk works, we don't need to introduce
>> > another one.
>>
>> The quirk was introduced for ATI SB600 and SB700 AHCIs where port
>> multipliers do actually work, so I don't really like using it to fix
>> devices where they don't. I just didn't realise that's what I was
>> doing until now.
>
> Hmm, well, the SB600 databook says that port multipliers are supported
> on ASIC revision A13 and above. So I wonder if the workaround is only
> necessary for the older revisions whithout port multiplier support.
I'll have to check this on Monday when I can get at the machine, but
I'm pretty sure I've had a port multiplier working on an SB600 that
required this quirk.
untested diff follows.
Index: ahci.c
===================================================================
RCS file: /cvs/src/sys/dev/pci/ahci.c,v
retrieving revision 1.180
diff -u -r1.180 ahci.c
--- ahci.c 14 Jun 2011 10:40:14 -0000 1.180
+++ ahci.c 24 Jun 2011 12:43:53 -0000
@@ -418,6 +418,7 @@
#define AHCI_F_NO_NCQ (1<<0)
#define AHCI_F_IGN_FR (1<<1)
#define AHCI_F_IPMS_PROBE (1<<2) /* IPMS on failed PMP probe
*/
+#define AHCI_F_NO_PMP (1<<3) /* ignore PMP capability */
u_int sc_ncmds;
@@ -456,7 +457,7 @@
struct pci_attach_args *);
int ahci_amd_hudson2_attach(struct ahci_softc *,
struct pci_attach_args *);
-int ahci_intel_3400_1_attach(struct ahci_softc *,
+int ahci_intel_attach(struct ahci_softc *,
struct pci_attach_args *);
int ahci_nvidia_mcp_attach(struct ahci_softc *,
struct pci_attach_args *);
@@ -480,8 +481,42 @@
{ PCI_VENDOR_ATI, PCI_PRODUCT_ATI_SBX00_SATA_6,
NULL, ahci_ati_sb700_attach },
+ { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_6SERIES_AHCI_1,
+ NULL, ahci_intel_attach },
+ { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_6SERIES_AHCI_2,
+ NULL, ahci_intel_attach },
+ { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_6321ESB_AHCI,
+ NULL, ahci_intel_attach },
+ { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_82801GR_AHCI,
+ NULL, ahci_intel_attach },
+ { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_82801GBM_AHCI,
+ NULL, ahci_intel_attach },
+ { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_82801H_AHCI_6P,
+ NULL, ahci_intel_attach },
+ { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_82801H_AHCI_4P,
+ NULL, ahci_intel_attach },
+ { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_82801HBM_AHCI,
+ NULL, ahci_intel_attach },
+ { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_82801I_AHCI_1,
+ NULL, ahci_intel_attach },
+ { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_82801I_AHCI_2,
+ NULL, ahci_intel_attach },
+ { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_82801I_AHCI_3,
+ NULL, ahci_intel_attach },
+ { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_82801JD_AHCI,
+ NULL, ahci_intel_attach },
+ { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_82801JI_AHCI,
+ NULL, ahci_intel_attach },
{ PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_3400_AHCI_1,
- NULL, ahci_intel_3400_1_attach },
+ NULL, ahci_intel_attach },
+ { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_3400_AHCI_2,
+ NULL, ahci_intel_attach },
+ { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_3400_AHCI_3,
+ NULL, ahci_intel_attach },
+ { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_3400_AHCI_4,
+ NULL, ahci_intel_attach },
+ { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_EP80579_AHCI,
+ NULL, ahci_intel_attach },
{ PCI_VENDOR_NVIDIA, PCI_PRODUCT_NVIDIA_MCP65_AHCI_2,
NULL, ahci_nvidia_mcp_attach },
@@ -716,9 +751,9 @@
}
int
-ahci_intel_3400_1_attach(struct ahci_softc *sc, struct pci_attach_args *pa)
+ahci_intel_attach(struct ahci_softc *sc, struct pci_attach_args *pa)
{
- sc->sc_flags |= AHCI_F_IPMS_PROBE;
+ sc->sc_flags |= AHCI_F_NO_PMP;
return (0);
}
@@ -2089,6 +2124,7 @@
}
if (pmp == 0 ||
+ (ap->ap_sc->sc_flags & AHCI_F_NO_PMP) ||
!ISSET(ahci_read(ap->ap_sc, AHCI_REG_CAP), AHCI_REG_CAP_SPM)) {
goto err;
}
