NetBSD Problem Report #53716

From clare@csel.org  Sat Nov 10 13:25:26 2018
Return-Path: <clare@csel.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 627D17A1B1
	for <gnats-bugs@gnats.NetBSD.org>; Sat, 10 Nov 2018 13:25:26 +0000 (UTC)
Message-Id: <20181110132028.E4DCB7B0BD@devel.csel.org>
Date: Sat, 10 Nov 2018 22:20:28 +0900 (JST)
From: clare@csel.org
Reply-To: clare@csel.org
To: gnats-bugs@NetBSD.org
Subject: PCengines APU2 requires AHCI enabler
X-Send-Pr-Version: 3.95

>Number:         53716
>Category:       kern
>Synopsis:       PCengines APU2 requires AHCI enabler
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          change-request
>Submitter-Id:   net
>Arrival-Date:   Sat Nov 10 13:30:00 +0000 2018
>Last-Modified:  Sat Nov 24 18:20:01 +0000 2018
>Originator:     Shinichi Doyashiki
>Release:        NetBSD 8.0_STABLE
>Organization:
	at home
>Environment:
System: NetBSD devel.csel.org 8.0_STABLE NetBSD 8.0_STABLE (APU1C_8) #2: Sat Oct 6 20:53:12 JST 2018 clare@devel.csel.org:/export/stage/stable-8/src/sys/arch/amd64/compile/APU1C_8 amd64
Architecture: x86_64
Machine: amd64
>Description:
	The PCengines APU2C4 uses AMD Hudson AHCI SATA controller,
	but its BIOS does not initialize the PCI hardware as
	we desired.

	Since we have no DMA handling for AMD Hudson IDE chipset,
	we require switching the chipset from IDE/Legacy mode to
	AHCI mode, to get reasonable performance.

>How-To-Repeat:
	Get an APU2C4 hardware, and boot NetBSD kernel on it.

>Fix:
	I wrote a quick hack patch as following:

cvs diff: Diffing .
Index: ahcisata_pci.c
===================================================================
RCS file: /export/cvsroot/netbsd/src/sys/dev/pci/ahcisata_pci.c,v
retrieving revision 1.38
diff -u -r1.38 ahcisata_pci.c
--- ahcisata_pci.c	13 Oct 2016 17:11:09 -0000	1.38
+++ ahcisata_pci.c	10 Nov 2018 12:47:42 -0000
@@ -238,6 +238,39 @@
 	force = ((ahci_pci_has_quirk( PCI_VENDOR(pa->pa_id),
 	    PCI_PRODUCT(pa->pa_id)) & AHCI_PCI_QUIRK_FORCE) != 0);

+        /*
+         * The PCengines APU2's system BIOS does not handle AHCI device
+         * as we desired.  We should fix it by re-program registers.
+         * In general, AMD Hudson SATA device should be used in AHCI mode
+	  * rather than IDE mode, since we have no DMA driver for it.
+         *
+         * see ftp://kolibrios.org/users/art_zh/doc/public/A50_RPR.pdf
+         * for register programming details.
+         */
+        if (PCI_VENDOR(pa->pa_id) == PCI_VENDOR_AMD &&
+            PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_HUDSON_SATA)
+        {
+                int reg;
+
+                printf("ahcisata: re-program IDE to AHCI\n");
+                /* make subclass/interface register is writeable */
+                reg = pci_conf_read(pa->pa_pc, pa->pa_tag, 0x40);
+                reg |= 1;
+                pci_conf_write(pa->pa_pc, pa->pa_tag, 0x40, reg);
+                /* rewrite subclass/interface register */
+                reg = pci_conf_read(pa->pa_pc, pa->pa_tag, 0x08);
+                reg &= ~0x0000FF00; /* clear register 0x09 */
+                reg |=  0x00000100; /* set 0x09 to 0x01 */
+                reg &= ~0x00FF0000; /* clear register 0x0A */
+                reg |=  0x00060000; /* set 0x0A to 0x06 */
+                pci_conf_write(pa->pa_pc, pa->pa_tag, 0x08, reg);
+                pa->pa_class = reg; /* writeback to the our cache */
+                /* make subclass/interface register is not writeable */
+                reg = pci_conf_read(pa->pa_pc, pa->pa_tag, 0x40);
+                reg &= ~1;
+                pci_conf_write(pa->pa_pc, pa->pa_tag, 0x40, reg);
+        }
+
 	/* 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 ||

>Audit-Trail:
From: matthew green <mrg@eterna.com.au>
To: gnats-bugs@NetBSD.org, clare@csel.org
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org,
    netbsd-bugs@netbsd.org
Subject: re: kern/53716: PCengines APU2 requires AHCI enabler
Date: Sun, 11 Nov 2018 08:45:06 +1100

 hi Shinichi,


 can you re-work this as a quirk rather than as a direct
 check?  see eg ahci_pci_has_quirk() and friends, similar
 to the existing AHCI_PCI_QUIRK_FORCE check immediately
 above your new code, and then move the code that makes
 the actual change into its own function so that the
 match function remains relatively short and clear.

 infact, could you make it such that the match simply
 returns non zero, and move the actual switch into the
 attach function?  generally, match should not change
 any device state, only attach should.  eg, for the case
 that a more specific driver may exist for this card it
 will return a higher match value than ahci itself, and
 the match function will have made a change to the
 device that may not be expected, and the other driver
 (who will have attach called, not ahci), will be
 started with the device in an unexpected state.

 thanks!


 .mrg.

From: clare@csel.org
To: gnats-bugs@NetBSD.org
Cc: matthew green <mrg@eterna.com.au>, kern-bug-people@netbsd.org,
 gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: kern/53716: PCengines APU2 requires AHCI enabler
Date: Sun, 11 Nov 2018 20:00:47 +0900

 I re-worked the patch as following:

 Index: sys/dev/ic/ahcisatavar.h
 ===================================================================
 RCS file: /export/cvsroot/netbsd/src/sys/dev/ic/ahcisatavar.h,v
 retrieving revision 1.17
 diff -u -r1.17 ahcisatavar.h
 --- sys/dev/ic/ahcisatavar.h	24 May 2015 22:30:05 -0000	1.17
 +++ sys/dev/ic/ahcisatavar.h	11 Nov 2018 05:30:32 -0000
 @@ -60,6 +60,7 @@
  #define AHCI_QUIRK_BADPMP	__BIT(2)  /* broken PMP support, ignore */
  #define AHCI_QUIRK_BADPMPRESET	__BIT(3)  /* broken PMP support for reset */
  #define AHCI_QUIRK_SKIP_RESET	__BIT(4)  /* skip drive reset sequence */
 +#define AHCI_PCI_QUIRK_FIXUP	__BIT(5)  /* interface needs fixup */

  	uint32_t sc_ahci_cap;	/* copy of AHCI_CAP */
  	int sc_ncmds; /* number of command slots */
 Index: sys/dev/pci/ahcisata_pci.c
 ===================================================================
 RCS file: /export/cvsroot/netbsd/src/sys/dev/pci/ahcisata_pci.c,v
 retrieving revision 1.38
 diff -u -r1.38 ahcisata_pci.c
 --- sys/dev/pci/ahcisata_pci.c	13 Oct 2016 17:11:09 -0000	1.38
 +++ sys/dev/pci/ahcisata_pci.c	11 Nov 2018 10:57:34 -0000
 @@ -194,6 +194,8 @@
  	    AHCI_PCI_QUIRK_FORCE },
  	{ PCI_VENDOR_ASMEDIA, PCI_PRODUCT_ASMEDIA_ASM1061_12,
  	    AHCI_PCI_QUIRK_FORCE },
 +	{ PCI_VENDOR_AMD, PCI_PRODUCT_AMD_HUDSON_SATA,
 +	    AHCI_PCI_QUIRK_FORCE | AHCI_PCI_QUIRK_FIXUP },
  };

  struct ahci_pci_softc {
 @@ -208,7 +210,7 @@
  static void ahci_pci_attach(device_t, device_t, void *);
  static int  ahci_pci_detach(device_t, int);
  static bool ahci_pci_resume(device_t, const pmf_qual_t *);
 -
 +static void ahci_pci_fixup_interface(device_t);

  CFATTACH_DECL_NEW(ahcisata_pci, sizeof(struct ahci_pci_softc),
      ahci_pci_match, ahci_pci_attach, ahci_pci_detach, NULL);
 @@ -273,7 +275,17 @@
  	pci_intr_handle_t intrhandle;
  	char intrbuf[PCI_INTRSTR_LEN];

 +	pci_aprint_devinfo(pa, "AHCI disk controller");
 +	
  	sc->sc_atac.atac_dev = self;
 +	sc->sc_ahci_quirks = ahci_pci_has_quirk(PCI_VENDOR(pa->pa_id),
 +					    PCI_PRODUCT(pa->pa_id));
 +
 +	psc->sc_pc = pa->pa_pc;
 +	psc->sc_pcitag = pa->pa_tag;
 +
 +	if (sc->sc_ahci_quirks & AHCI_PCI_QUIRK_FIXUP)
 +		ahci_pci_fixup_interface(self);

  	if (pci_mapreg_map(pa, AHCI_PCI_ABAR,
  	    PCI_MAPREG_TYPE_MEM | PCI_MAPREG_MEM_TYPE_32BIT, 0,
 @@ -281,11 +293,7 @@
  		aprint_error_dev(self, "can't map ahci registers\n");
  		return;
  	}
 -	psc->sc_pc = pa->pa_pc;
 -	psc->sc_pcitag = pa->pa_tag;

 -	pci_aprint_devinfo(pa, "AHCI disk controller");
 -	
  	if (pci_intr_map(pa, &intrhandle) != 0) {
  		aprint_error_dev(self, "couldn't map interrupt\n");
  		return;
 @@ -302,8 +310,6 @@

  	sc->sc_dmat = pa->pa_dmat;

 -	sc->sc_ahci_quirks = ahci_pci_has_quirk(PCI_VENDOR(pa->pa_id),
 -					    PCI_PRODUCT(pa->pa_id));

  	ahci_cap_64bit = (AHCI_READ(sc, AHCI_CAP) & AHCI_CAP_64BIT) != 0;
  	ahci_bad_64bit = ((sc->sc_ahci_quirks & AHCI_PCI_QUIRK_BAD64) != 0);
 @@ -364,3 +370,58 @@

  	return true;
  }
 +
 +/*
 + * Switch devices in legacy IDE interface to native AHCI, if desired.
 + *
 + * In some devices, especially in PCengines APU2 series, the BIOS left
 + * AHCI devices in IDE/legacy mode to simplify their code and some
 + * unknown compatibility reasons.  Since we have no PCI-IDE DMA
 + * handler for the AMD Hudson SATA IDE controller, we should re-program
 + * interfacing mode and use it in native AHCI.
 + *
 + * see ftp://kolibrios.org/users/art_zh/doc/public/A50_RPR.pdf
 + * for register programming details.
 + */
 +static void
 +ahci_pci_fixup_interface(device_t dv)
 +{
 +	struct ahci_pci_softc *psc = device_private(dv);
 +	int cc;
 +	int reg;
 +
 +	/*
 +	 * check the interface mode in IDE/legacy
 +	 */
 +	cc = pci_conf_read(psc->sc_pc, psc->sc_pcitag, 0x08);
 +	if (!(PCI_CLASS(cc) == PCI_CLASS_MASS_STORAGE &&
 +	    PCI_SUBCLASS(cc) == PCI_SUBCLASS_MASS_STORAGE_IDE))
 +		return;
 +
 +	aprint_normal_dev(dv, "switch legacy IDE to native AHCI\n");
 +
 +	/*
 +	 * make subclass/interface register is writeable.
 +	 */
 +	reg = pci_conf_read(psc->sc_pc, psc->sc_pcitag, 0x40);
 +	reg |= 1;
 +	pci_conf_write(psc->sc_pc, psc->sc_pcitag, 0x40, reg);
 +
 +	/*
 +	 * rewrite subclass/interface register.
 +	 * this causes switch interface logic.
 +	 */
 +	reg = pci_conf_read(psc->sc_pc, psc->sc_pcitag, PCI_CLASS_REG);
 +	reg &= ~PCI_SUBCLASS(PCI_SUBCLASS_MASK);
 +	reg |=  PCI_SUBCLASS(PCI_SUBCLASS_MASS_STORAGE_SATA);
 +	reg &= ~PCI_INTERFACE(PCI_INTERFACE_MASK);
 +	reg |=  PCI_INTERFACE(PCI_INTERFACE_SATA_AHCI);
 +	pci_conf_write(psc->sc_pc, psc->sc_pcitag, PCI_CLASS_REG, reg);
 +
 +	/*
 +	 * make subclass/interface register is read-only.
 +	 */
 +	reg = pci_conf_read(psc->sc_pc, psc->sc_pcitag, 0x40);
 +	reg &= ~1;
 +	pci_conf_write(psc->sc_pc, psc->sc_pcitag, 0x40, reg);
 +}


 -- 
 Shinichi Doyashiki <clare@csel.org>

From: clare@csel.org
To: gnats-bugs@NetBSD.org
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org,
 netbsd-bugs@netbsd.org
Subject: Re: kern/53716: PCengines APU2 requires AHCI enabler
Date: Sun, 25 Nov 2018 03:18:09 +0900

 the most recent, mainline APU2 system firmware provides AHCI mode
 from the boot time, so no more required the patch under recent
 firmware environment.  http://pcengines.github.io

 if you want to avoid patching, you can upgrade the firmware.
 if you want to stay on legacy firmware, you will want to apply the patch.


 -- 
 Shinichi Doyashiki <clare@csel.org>

Home	PR Database Search