Re: Nvidia MCP67 SATA controller

[Manuel Bouyer <bouyer%antioche.eu.org@localhost>]

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.

-- 
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
     NetBSD: 26 ans d'experience feront toujours la difference
--