Re: Nvidia MCP67 SATA controller

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

On Wed, Jun 18, 2008 at 08:04:21PM +0200, Manuel Bouyer wrote:
> On Wed, Jun 18, 2008 at 07:30:41PM +0200, Dieter Baron wrote:
> > hi,
> > 
> > the Nvidia MCP67 SATA controller does not work using the viaide driver
> > (in native pci compatibility mode), as various users have reported.
> > 
> >   Attached is a diff that forces it into AHCI mode and thus attaches
> > ahcisata.  The method to switch to AHCI mode was taken from the
> > FreeBSD source tree.
> > 
> >   This works quite well most of the time, but sometimes the chip seems
> > to get in a strange state after the switch.  It then returns bogus
> > data when identifying the drive:
> >     wd0 at atabus1 drive 0: <ST506>
> > (or similar, written from memory) instead of 
> >     wd0 at atabus1 drive 0: <Hitachi HDT725032VLA360>
> > and hangs when trying to read from the disk (still during boot).
> > 
> >   If the boot is successful, the controller is stable.  I've used it
> > to reconstruct a 320gb raid1.
> > 
> > 
> >   Any idea why this sometimes fails?  Any other comments about the
> > patch (the debug printf()s will be removed)?  I want to commit it once
> > it's fixed and cleaned up.
> > 
> > Anyone have the datasheet for the MCP67 SATA controller?
> > 
> >                                     thanks,
> >                                     dillo
> 
> > Index: 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
> > --- ahcisata_pci.c  20 Mar 2008 16:15:57 -0000      1.11
> > +++ ahcisata_pci.c  18 Jun 2008 17:21:32 -0000
> > @@ -60,6 +60,9 @@
> >  static void ahci_pci_attach(device_t, device_t, void *);
> >  static bool ahci_pci_resume(device_t PMF_FN_PROTO);
> >  
> > +static bool ahci_pci_enable_ahci_mode(bus_space_tag_t, bus_space_handle_t,
> > +                                 uint32_t);
> > +
> >  
> >  CFATTACH_DECL_NEW(ahcisata_pci, sizeof(struct ahci_pci_softc),
> >      ahci_pci_match, ahci_pci_attach, NULL, NULL);
> > @@ -71,12 +74,21 @@
> >     bus_space_tag_t regt;
> >     bus_space_handle_t regh;
> >     bus_size_t size;
> > +   uint32_t ghc;
> >     int ret = 0;
> > +   int force_ahci = 0;
> > +
> > +   if (PCI_VENDOR(pa->pa_id) == PCI_VENDOR_NVIDIA &&
> > +       PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_NVIDIA_MCP67_SATA) {
> > +           printf("ahcisata: forcing ahci mode for MCP67\n");
> > +           force_ahci = 1;
> > +   }
> >  
> >     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)) {
> > +        PCI_SUBCLASS(pa->pa_class) == PCI_SUBCLASS_MASS_STORAGE_RAID ||
> > +        force_ahci)) {
> >             /* 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,
> > @@ -85,8 +97,16 @@
> >             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;
> > +           else {
> > +                   ghc = bus_space_read_4(regt, regh, AHCI_GHC);
> > +                   printf("ahcisata: ghc=[%08x]\n", (int)ghc);
> > +                   if (ghc & AHCI_GHC_AE)
> > +                           ret = 3;
> > +                   else if (force_ahci) {
> > +                           if (ahci_pci_enable_ahci_mode(regt, regh, ghc))
> > +                                   ret = 3;
> > +                   }
> > +           }
> >             bus_space_unmap(regt, regh, size);
> >             return ret;
> >     }
> > @@ -164,3 +184,36 @@
> >  
> >     return true;
> >  }
> > +
> > +
> > +static bool
> > +ahci_pci_enable_ahci_mode(bus_space_tag_t regt, bus_space_handle_t regh,
> > +                     uint32_t ghc)
> > +{
> > +   printf("ahcisata: putting chip in AHCI mode\n");
> > +
> > +   ghc |= AHCI_GHC_AE;
> > +   bus_space_write_4(regt, regh, AHCI_GHC, ghc);
> > +
> > +   ghc |= AHCI_GHC_HR;
> > +   bus_space_write_4(regt, regh, AHCI_GHC, ghc);
> > +
> > +   DELAY(1000000);
> > +
> > +   ghc = bus_space_read_4(regt, regh, AHCI_GHC);
> > +   if (ghc & AHCI_GHC_HR) {
> > +           printf("ahcisata: reset faild\n");
> > +           return false;
> > +   }
> > +   
> > +   ghc |= AHCI_GHC_AE;
> > +   bus_space_write_4(regt, regh, AHCI_GHC, ghc);
> > +
> > +   ghc = bus_space_read_4(regt, regh, AHCI_GHC);
> > +   if ((ghc & AHCI_GHC_AE) == 0) {
> > +           printf("ahcisata: enabling ahci mode failed\n");
> > +           return false;
> > +   }
> > +
> > +   return true;
> > +}
> 
> This is almost a duplicate of ahci_reset(), which is the first thing done
> in ahci_attach(). I think you don't need this function at all,
> just set the AHCI_GHC_AE bit in the force_ahci case, and let ahci_attach()
> to the reset.

  I tried that first, but then it never worked the first time after
power on.  I had to do a reset when the first boot hung, then the
second boot would succeed.

                                        yours,
                                        dillo