Re: Nvidia MCP67 SATA controller

[Dieter Baron <dillo%danbala.tuwien.ac.at@localhost>]

On Sat, Jun 28, 2008 at 12:06:21AM +0200, Manuel Bouyer wrote:
> On Fri, Jun 27, 2008 at 11:55:00AM +0200, Dieter Baron wrote:
> > hi,
> > 
> > attached is a cleaned up version of my previous patch to make MCP65
> > and MCP67 SATA controllers work: Make ahcisata match, even if the
> > controller is not in AHCI mode; increase the delay after a channel
> > reset to 500ms (from 100ms).  I've also restructured the match
> > function to only map the registers if we actually check which mode the
> > controller is in.
> 
> I think this part of the patch is wrong. We should not match if we
> can't map the AHCI registers, as it's not going to work anyway.
> The BIOS may have configured it in pciide-compatible mode, which which
> case the AHCI registers may not be mappable.

  Okay, I've reworked the patch to always map (and unmap) the register
if we are going to match the device.  New version attached.

                                        yours,
                                        dillo

Index: sys/dev/ic/ahcisata_core.c
===================================================================
RCS file: /cvsroot/src/sys/dev/ic/ahcisata_core.c,v
retrieving revision 1.15
diff -u -r1.15 ahcisata_core.c
--- sys/dev/ic/ahcisata_core.c  7 May 2008 13:52:12 -0000       1.15
+++ sys/dev/ic/ahcisata_core.c  1 Jul 2008 09:01:43 -0000
@@ -604,8 +604,8 @@
                    AHCI_P_IX_DHRS);
                /* and start operations */
                ahci_channel_start(sc, chp);
-               /* wait 100ms before actually starting operations */
-               tsleep(&sc, PRIBIO, "ahciprb", mstohz(100));
+               /* wait 500ms before actually starting operations */
+               tsleep(&sc, PRIBIO, "ahciprb", mstohz(500));
                break;
 
        default:
Index: sys/dev/pci/ahcisata_pci.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/ahcisata_pci.c,v
retrieving revision 1.11
diff -u -r1.11 ahcisata_pci.c
--- sys/dev/pci/ahcisata_pci.c  20 Mar 2008 16:15:57 -0000      1.11
+++ sys/dev/pci/ahcisata_pci.c  1 Jul 2008 09:01:43 -0000
@@ -49,6 +49,15 @@
 #include <dev/pci/pciidevar.h>
 #include <dev/ic/ahcisatavar.h>
 
+#define AHCI_PCI_QUIRK_FORCE   1       /* force attach */
+
+static const struct pci_quirkdata ahci_pci_quirks[] = {
+       { PCI_VENDOR_NVIDIA, PCI_PRODUCT_NVIDIA_MCP65_SATA,
+           AHCI_PCI_QUIRK_FORCE },
+       { PCI_VENDOR_NVIDIA, PCI_PRODUCT_NVIDIA_MCP67_SATA,
+           AHCI_PCI_QUIRK_FORCE },
+};
+
 struct ahci_pci_softc {
        struct ahci_softc ah_sc;
        pci_chipset_tag_t sc_pc;
@@ -58,6 +67,8 @@
 
 static int  ahci_pci_match(device_t, cfdata_t, void *);
 static void ahci_pci_attach(device_t, device_t, void *);
+const struct pci_quirkdata *ahci_pci_lookup_quirkdata(pci_vendor_id_t,
+                                                     pci_product_id_t);
 static bool ahci_pci_resume(device_t PMF_FN_PROTO);
 
 
@@ -72,25 +83,31 @@
        bus_space_handle_t regh;
        bus_size_t size;
        int ret = 0;
+       const struct pci_quirkdata *quirks;
+
+       quirks = ahci_pci_lookup_quirkdata(PCI_VENDOR(pa->pa_id),
+                                          PCI_PRODUCT(pa->pa_id));
 
-       if (PCI_CLASS(pa->pa_class) == PCI_CLASS_MASS_STORAGE &&
-           ((PCI_SUBCLASS(pa->pa_class) == PCI_SUBCLASS_MASS_STORAGE_SATA &&
+       /* if wrong class and not forced by quirks, don't match */
+       if ((PCI_CLASS(pa->pa_class) != PCI_CLASS_MASS_STORAGE ||
+           ((PCI_SUBCLASS(pa->pa_class) != PCI_SUBCLASS_MASS_STORAGE_SATA ||
+            PCI_INTERFACE(pa->pa_class) != PCI_INTERFACE_SATA_AHCI) &&
+            PCI_SUBCLASS(pa->pa_class) != PCI_SUBCLASS_MASS_STORAGE_RAID)) &&
+           (quirks == NULL || (quirks->quirks & AHCI_PCI_QUIRK_FORCE) == 0))
+               return 0;
+
+       if (pci_mapreg_map(pa, AHCI_PCI_ABAR,
+           PCI_MAPREG_TYPE_MEM | PCI_MAPREG_MEM_TYPE_32BIT, 0,
+           &regt, &regh, NULL, &size) != 0)
+               return 0;
+
+       if ((PCI_SUBCLASS(pa->pa_class) == PCI_SUBCLASS_MASS_STORAGE_SATA &&
             PCI_INTERFACE(pa->pa_class) == PCI_INTERFACE_SATA_AHCI) ||
-            PCI_SUBCLASS(pa->pa_class) == PCI_SUBCLASS_MASS_STORAGE_RAID)) {
-               /* check if the chip is in ahci mode */
-               if (pci_mapreg_map(pa, AHCI_PCI_ABAR,
-                   PCI_MAPREG_TYPE_MEM | PCI_MAPREG_MEM_TYPE_32BIT, 0,
-                   &regt, &regh, NULL, &size) != 0)
-                       return 0;
-               if (PCI_SUBCLASS(pa->pa_class) == PCI_SUBCLASS_MASS_STORAGE_SATA
-                   && PCI_INTERFACE(pa->pa_class) == PCI_INTERFACE_SATA_AHCI)
-                       ret = 3;
-               else if (bus_space_read_4(regt, regh, AHCI_GHC) & AHCI_GHC_AE)
-                       ret = 3;
-               bus_space_unmap(regt, regh, size);
-               return ret;
-       }
+           (quirks && quirks->quirks & AHCI_PCI_QUIRK_FORCE) ||
+           (bus_space_read_4(regt, regh, AHCI_GHC) & AHCI_GHC_AE))
+               ret = 3;
 
+       bus_space_unmap(regt, regh, size);
        return ret;
 }
 
@@ -164,3 +181,16 @@
 
        return true;
 }
+
+const struct pci_quirkdata *
+ahci_pci_lookup_quirkdata(pci_vendor_id_t vendor, pci_product_id_t product)
+{
+       int i;
+
+       for (i = 0; i < (sizeof ahci_pci_quirks / sizeof ahci_pci_quirks[0]);
+            i++)
+               if (vendor == ahci_pci_quirks[i].vendor &&
+                   product == ahci_pci_quirks[i].product)
+                       return (&ahci_pci_quirks[i]);
+       return (NULL);
+}