Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 09 Sep 2007 13:41:17 -0700
From:      Nate Lawson <nate@root.org>
To:        Kostik Belousov <kostikbel@gmail.com>
Cc:        acpi@freebsd.org, current@freebsd.org
Subject:   Re: PATCH: ecng for 6.x and 7.x
Message-ID:  <46E45A6D.1010805@root.org>
In-Reply-To: <20070907133901.GL53667@deviant.kiev.zoral.com.ua>
References:  <46E0777A.8070901@root.org> <20070907133901.GL53667@deviant.kiev.zoral.com.ua>

next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format.
--------------040800050701000906010408
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit

Kostik Belousov wrote:
> On Thu, Sep 06, 2007 at 02:56:10PM -0700, Nate Lawson wrote:
>> I've done some major rework on the EC driver.  This should help with
>> various problems, including timeouts while checking battery status or
>> temperature.  The attached patches are for 6.x and 7.x.  Please test and
>> let me know if you get any new errors on dmesg or if it fixes things for
>> you (especially HP/Compaq laptop owners).
> Ok, I tryed it on Acer 2490-kind of laptop. I see no difference with the
> patch, battery status seems to be reported correctly both before and after
> applying it.
> 
> The only thing I want to note is that reboot on the laptop is turned into
> poweroff. You said in the followup to original letter that poweroff is
> turned into reboot, I did not checked it; this is opposite.

Please test this revised version.  All it changes is forcing polling
mode back on during reboot/shutdown.

-Nate

--------------040800050701000906010408
Content-Type: text/x-patch;
 name="ecng-6b.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="ecng-6b.diff"

--- sys/dev/acpica/acpi_ec.c	4 Sep 2007 22:40:39 -0000	1.65.2.3
+++ sys/dev/acpica/acpi_ec.c	9 Sep 2007 20:35:53 -0000
@@ -1,5 +1,5 @@
 /*-
- * Copyright (c) 2003 Nate Lawson
+ * Copyright (c) 2003-2007 Nate Lawson
  * Copyright (c) 2000 Michael Smith
  * Copyright (c) 2000 BSDi
  * All rights reserved.
@@ -25,115 +25,6 @@
  * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
  * SUCH DAMAGE.
  */
-/*-
- ******************************************************************************
- *
- * 1. Copyright Notice
- *
- * Some or all of this work - Copyright (c) 1999, Intel Corp.  All rights
- * reserved.
- *
- * 2. License
- *
- * 2.1. This is your license from Intel Corp. under its intellectual property
- * rights.  You may have additional license terms from the party that provided
- * you this software, covering your right to use that party's intellectual
- * property rights.
- *
- * 2.2. Intel grants, free of charge, to any person ("Licensee") obtaining a
- * copy of the source code appearing in this file ("Covered Code") an
- * irrevocable, perpetual, worldwide license under Intel's copyrights in the
- * base code distributed originally by Intel ("Original Intel Code") to copy,
- * make derivatives, distribute, use and display any portion of the Covered
- * Code in any form, with the right to sublicense such rights; and
- *
- * 2.3. Intel grants Licensee a non-exclusive and non-transferable patent
- * license (with the right to sublicense), under only those claims of Intel
- * patents that are infringed by the Original Intel Code, to make, use, sell,
- * offer to sell, and import the Covered Code and derivative works thereof
- * solely to the minimum extent necessary to exercise the above copyright
- * license, and in no event shall the patent license extend to any additions
- * to or modifications of the Original Intel Code.  No other license or right
- * is granted directly or by implication, estoppel or otherwise;
- *
- * The above copyright and patent license is granted only if the following
- * conditions are met:
- *
- * 3. Conditions 
- *
- * 3.1. Redistribution of Source with Rights to Further Distribute Source.  
- * Redistribution of source code of any substantial portion of the Covered
- * Code or modification with rights to further distribute source must include
- * the above Copyright Notice, the above License, this list of Conditions,
- * and the following Disclaimer and Export Compliance provision.  In addition,
- * Licensee must cause all Covered Code to which Licensee contributes to
- * contain a file documenting the changes Licensee made to create that Covered
- * Code and the date of any change.  Licensee must include in that file the
- * documentation of any changes made by any predecessor Licensee.  Licensee 
- * must include a prominent statement that the modification is derived,
- * directly or indirectly, from Original Intel Code.
- *
- * 3.2. Redistribution of Source with no Rights to Further Distribute Source.  
- * Redistribution of source code of any substantial portion of the Covered
- * Code or modification without rights to further distribute source must
- * include the following Disclaimer and Export Compliance provision in the
- * documentation and/or other materials provided with distribution.  In
- * addition, Licensee may not authorize further sublicense of source of any
- * portion of the Covered Code, and must include terms to the effect that the
- * license from Licensee to its licensee is limited to the intellectual
- * property embodied in the software Licensee provides to its licensee, and
- * not to intellectual property embodied in modifications its licensee may
- * make.
- *
- * 3.3. Redistribution of Executable. Redistribution in executable form of any
- * substantial portion of the Covered Code or modification must reproduce the
- * above Copyright Notice, and the following Disclaimer and Export Compliance
- * provision in the documentation and/or other materials provided with the
- * distribution.
- *
- * 3.4. Intel retains all right, title, and interest in and to the Original
- * Intel Code.
- *
- * 3.5. Neither the name Intel nor any other trademark owned or controlled by
- * Intel shall be used in advertising or otherwise to promote the sale, use or
- * other dealings in products derived from or relating to the Covered Code
- * without prior written authorization from Intel.
- *
- * 4. Disclaimer and Export Compliance
- *
- * 4.1. INTEL MAKES NO WARRANTY OF ANY KIND REGARDING ANY SOFTWARE PROVIDED
- * HERE.  ANY SOFTWARE ORIGINATING FROM INTEL OR DERIVED FROM INTEL SOFTWARE
- * IS PROVIDED "AS IS," AND INTEL WILL NOT PROVIDE ANY SUPPORT,  ASSISTANCE,
- * INSTALLATION, TRAINING OR OTHER SERVICES.  INTEL WILL NOT PROVIDE ANY
- * UPDATES, ENHANCEMENTS OR EXTENSIONS.  INTEL SPECIFICALLY DISCLAIMS ANY
- * IMPLIED WARRANTIES OF MERCHANTABILITY, NONINFRINGEMENT AND FITNESS FOR A
- * PARTICULAR PURPOSE. 
- *
- * 4.2. IN NO EVENT SHALL INTEL HAVE ANY LIABILITY TO LICENSEE, ITS LICENSEES
- * OR ANY OTHER THIRD PARTY, FOR ANY LOST PROFITS, LOST DATA, LOSS OF USE OR
- * COSTS OF PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES, OR FOR ANY INDIRECT,
- * SPECIAL OR CONSEQUENTIAL DAMAGES ARISING OUT OF THIS AGREEMENT, UNDER ANY
- * CAUSE OF ACTION OR THEORY OF LIABILITY, AND IRRESPECTIVE OF WHETHER INTEL
- * HAS ADVANCE NOTICE OF THE POSSIBILITY OF SUCH DAMAGES.  THESE LIMITATIONS
- * SHALL APPLY NOTWITHSTANDING THE FAILURE OF THE ESSENTIAL PURPOSE OF ANY
- * LIMITED REMEDY.
- *
- * 4.3. Licensee shall not export, either directly or indirectly, any of this
- * software or system incorporating such software without first obtaining any
- * required license or other approval from the U. S. Department of Commerce or
- * any other agency or department of the United States Government.  In the
- * event Licensee exports any such software from the United States or
- * re-exports any such software from a foreign destination, Licensee shall
- * ensure that the distribution and export/re-export of the software is in
- * compliance with all laws, regulations, orders, or other restrictions of the
- * U.S. Export Administration Regulations. Licensee agrees that neither it nor
- * any of its subsidiaries will export/re-export any technical data, process,
- * software, or service, directly or indirectly, to any country for which the
- * United States government or any agency thereof requires an export license,
- * other governmental approval, or letter of assurance, without first obtaining
- * such license, approval or letter.
- *
- *****************************************************************************/
 
 #include <sys/cdefs.h>
 __FBSDID("$FreeBSD$");
@@ -142,9 +33,9 @@
 #include <sys/param.h>
 #include <sys/kernel.h>
 #include <sys/bus.h>
+#include <sys/lock.h>
 #include <sys/malloc.h>
 #include <sys/module.h>
-#include <sys/lock.h>
 #include <sys/sx.h>
 
 #include <machine/bus.h>
@@ -188,15 +79,15 @@
  *  | | | +--------- Burst Mode Enabled?
  *  | | +----------- SCI Event?
  *  | +------------- SMI Event?
- *  +--------------- <Reserved>
+ *  +--------------- <reserved>
  *
  */
 typedef UINT8				EC_STATUS;
 
 #define EC_FLAG_OUTPUT_BUFFER		((EC_STATUS) 0x01)
 #define EC_FLAG_INPUT_BUFFER		((EC_STATUS) 0x02)
+#define EC_FLAG_DATA_IS_CMD		((EC_STATUS) 0x08)
 #define EC_FLAG_BURST_MODE		((EC_STATUS) 0x10)
-#define EC_FLAG_SCI			((EC_STATUS) 0x20)
 
 /*
  * EC_EVENT:
@@ -208,6 +99,10 @@
 #define EC_EVENT_OUTPUT_BUFFER_FULL	((EC_EVENT) 0x01)
 #define EC_EVENT_INPUT_BUFFER_EMPTY	((EC_EVENT) 0x02)
 #define EC_EVENT_SCI			((EC_EVENT) 0x20)
+#define EC_EVENT_SMI			((EC_EVENT) 0x40)
+
+/* Data byte returned after burst enable indicating it was successful. */
+#define EC_BURST_ACK			0x90
 
 /*
  * Register access primitives
@@ -227,11 +122,11 @@
 /* Embedded Controller Boot Resources Table (ECDT) */
 typedef struct {
     ACPI_TABLE_HEADER		header;
-    ACPI_GENERIC_ADDRESS	control;
-    ACPI_GENERIC_ADDRESS	data;
-    UINT32			uid;
-    UINT8			gpe_bit;
-    char			ec_id[0];
+    ACPI_GENERIC_ADDRESS	Control;
+    ACPI_GENERIC_ADDRESS	Data;
+    UINT32			Uid;
+    UINT8			Gpe;
+    char			Id[0];
 } ACPI_TABLE_ECDT;
 
 /* Additional params to pass from the probe routine */
@@ -243,7 +138,7 @@
 };
 
 /* Indicate that this device has already been probed via ECDT. */
-#define DEV_ECDT(x)		(acpi_get_magic(x) == (int)&acpi_ec_devclass)
+#define DEV_ECDT(x)	(acpi_get_magic(x) == (uintptr_t)&acpi_ec_devclass)
 
 /*
  * Driver softc.
@@ -254,7 +149,6 @@
     int			ec_uid;
     ACPI_HANDLE		ec_gpehandle;
     UINT8		ec_gpebit;
-    UINT8		ec_csrvalue;
     
     int			ec_data_rid;
     struct resource	*ec_data_res;
@@ -268,6 +162,9 @@
 
     int			ec_glk;
     int			ec_glkhandle;
+    int			ec_burstactive;
+    int			ec_sci_pend;
+    u_int		ec_gencount;
 };
 
 /*
@@ -277,11 +174,11 @@
  */
 #define EC_LOCK_TIMEOUT	1000
 
-/* Default interval in microseconds for the status polling loop. */
-#define EC_POLL_DELAY	10
+/* Default delay in microseconds between each run of the status polling loop. */
+#define EC_POLL_DELAY	5
 
-/* Total time in ms spent in the poll loop waiting for a response. */
-#define EC_POLL_TIMEOUT	100
+/* Total time in ms spent waiting for a response from EC. */
+#define EC_TIMEOUT	750
 
 #define EVENT_READY(event, status)			\
 	(((event) == EC_EVENT_OUTPUT_BUFFER_FULL &&	\
@@ -289,36 +186,46 @@
 	 ((event) == EC_EVENT_INPUT_BUFFER_EMPTY && 	\
 	 ((status) & EC_FLAG_INPUT_BUFFER) == 0))
 
-static int	ec_poll_timeout = EC_POLL_TIMEOUT;
-TUNABLE_INT("hw.acpi.ec.poll_timeout", &ec_poll_timeout);
-
 ACPI_SERIAL_DECL(ec, "ACPI embedded controller");
 
-static __inline ACPI_STATUS
+SYSCTL_DECL(_debug_acpi);
+SYSCTL_NODE(_debug_acpi, OID_AUTO, ec, CTLFLAG_RD, NULL, "EC debugging");
+
+static int	ec_burst_mode;
+TUNABLE_INT("debug.acpi.ec.burst", &ec_burst_mode);
+SYSCTL_INT(_debug_acpi_ec, OID_AUTO, burst, CTLFLAG_RW, &ec_burst_mode, 0,
+    "Enable use of burst mode (faster for nearly all systems)");
+static int	ec_polled_mode;
+TUNABLE_INT("debug.acpi.ec.polled", &ec_polled_mode);
+SYSCTL_INT(_debug_acpi_ec, OID_AUTO, polled, CTLFLAG_RW, &ec_polled_mode, 0,
+    "Force use of polled mode (only if interrupt mode doesn't work)");
+static int	ec_timeout = EC_TIMEOUT;
+TUNABLE_INT("debug.acpi.ec.timeout", &ec_timeout);
+SYSCTL_INT(_debug_acpi_ec, OID_AUTO, timeout, CTLFLAG_RW, &ec_timeout,
+    EC_TIMEOUT, "Total time spent waiting for a response (poll+sleep)");
+
+static ACPI_STATUS
 EcLock(struct acpi_ec_softc *sc)
 {
     ACPI_STATUS	status;
 
-    /* Always acquire the exclusive lock. */
+    /* If _GLK is non-zero, acquire the global lock. */
     status = AE_OK;
-    ACPI_SERIAL_BEGIN(ec);
-
-    /* If _GLK is non-zero, also acquire the global lock. */
     if (sc->ec_glk) {
 	status = AcpiAcquireGlobalLock(EC_LOCK_TIMEOUT, &sc->ec_glkhandle);
 	if (ACPI_FAILURE(status))
-	    ACPI_SERIAL_END(ec);
+	    return (status);
     }
-
+    ACPI_SERIAL_BEGIN(ec);
     return (status);
 }
 
-static __inline void
+static void
 EcUnlock(struct acpi_ec_softc *sc)
 {
+    ACPI_SERIAL_END(ec);
     if (sc->ec_glk)
 	AcpiReleaseGlobalLock(sc->ec_glkhandle);
-    ACPI_SERIAL_END(ec);
 }
 
 static uint32_t		EcGpeHandler(void *Context);
@@ -328,7 +235,8 @@
 				ACPI_PHYSICAL_ADDRESS Address,
 				UINT32 width, ACPI_INTEGER *Value,
 				void *Context, void *RegionContext);
-static ACPI_STATUS	EcWaitEvent(struct acpi_ec_softc *sc, EC_EVENT Event);
+static ACPI_STATUS	EcWaitEvent(struct acpi_ec_softc *sc, EC_EVENT Event,
+				u_int gen_count);
 static ACPI_STATUS	EcCommand(struct acpi_ec_softc *sc, EC_COMMAND cmd);
 static ACPI_STATUS	EcRead(struct acpi_ec_softc *sc, UINT8 Address,
 				UINT8 *Data);
@@ -369,7 +277,9 @@
  * Look for an ECDT and if we find one, set up default GPE and 
  * space handlers to catch attempts to access EC space before
  * we have a real driver instance in place.
- * TODO: if people report invalid ECDTs, add a tunable to disable them.
+ *
+ * TODO: Some old Gateway laptops need us to fake up an ECDT or
+ * otherwise attach early so that _REG methods can run.
  */
 void
 acpi_ec_ecdt_probe(device_t parent)
@@ -387,20 +297,20 @@
     status = AcpiGetFirmwareTable("ECDT", 1, ACPI_LOGICAL_ADDRESSING, &hdr);
     ecdt = (ACPI_TABLE_ECDT *)hdr;
     if (ACPI_FAILURE(status) ||
-	ecdt->control.RegisterBitWidth != 8 ||
-	ecdt->data.RegisterBitWidth != 8) {
+	ecdt->Control.RegisterBitWidth != 8 ||
+	ecdt->Data.RegisterBitWidth != 8) {
 	return;
     }
 
     /* Create the child device with the given unit number. */
-    child = BUS_ADD_CHILD(parent, 0, "acpi_ec", ecdt->uid);
+    child = BUS_ADD_CHILD(parent, 0, "acpi_ec", ecdt->Uid);
     if (child == NULL) {
 	printf("%s: can't add child\n", __func__);
 	return;
     }
 
     /* Find and save the ACPI handle for this device. */
-    status = AcpiGetHandle(NULL, ecdt->ec_id, &h);
+    status = AcpiGetHandle(NULL, ecdt->Id, &h);
     if (ACPI_FAILURE(status)) {
 	device_delete_child(parent, child);
 	printf("%s: can't get handle\n", __func__);
@@ -409,9 +319,9 @@
     acpi_set_handle(child, h);
 
     /* Set the data and CSR register addresses. */
-    bus_set_resource(child, SYS_RES_IOPORT, 0, ecdt->data.Address,
+    bus_set_resource(child, SYS_RES_IOPORT, 0, ecdt->Data.Address,
 	/*count*/1);
-    bus_set_resource(child, SYS_RES_IOPORT, 1, ecdt->control.Address,
+    bus_set_resource(child, SYS_RES_IOPORT, 1, ecdt->Control.Address,
 	/*count*/1);
 
     /*
@@ -423,11 +333,11 @@
      */
     params = malloc(sizeof(struct acpi_ec_params), M_TEMP, M_WAITOK | M_ZERO);
     params->gpe_handle = NULL;
-    params->gpe_bit = ecdt->gpe_bit;
-    params->uid = ecdt->uid;
+    params->gpe_bit = ecdt->Gpe;
+    params->uid = ecdt->Uid;
     acpi_GetInteger(h, "_GLK", &params->glk);
     acpi_set_private(child, params);
-    acpi_set_magic(child, (int)&acpi_ec_devclass);
+    acpi_set_magic(child, (uintptr_t)&acpi_ec_devclass);
 
     /* Finish the attach process. */
     if (device_probe_and_attach(child) != 0)
@@ -688,100 +598,92 @@
     struct acpi_ec_softc	*sc = (struct acpi_ec_softc *)Context;
     UINT8			Data;
     ACPI_STATUS			Status;
-    EC_STATUS			EcStatus;
     char			qxx[5];
 
     ACPI_FUNCTION_TRACE((char *)(uintptr_t)__func__);
     KASSERT(Context != NULL, ("EcGpeQueryHandler called with NULL"));
 
+    /* Serialize user access with EcSpaceHandler(). */
     Status = EcLock(sc);
     if (ACPI_FAILURE(Status)) {
-	ACPI_VPRINT(sc->ec_dev, acpi_device_get_parent_softc(sc->ec_dev),
-		    "GpeQuery lock error: %s\n", AcpiFormatException(Status));
+	device_printf(sc->ec_dev, "GpeQuery lock error: %s\n",
+	    AcpiFormatException(Status));
 	return;
     }
 
     /*
-     * If the EC_SCI bit of the status register is not set, then pass
-     * it along to any potential waiters as it may be an IBE/OBF event.
-     */
-    EcStatus = EC_GET_CSR(sc);
-    if ((EcStatus & EC_EVENT_SCI) == 0) {
-	CTR1(KTR_ACPI, "ec event was not SCI, status %#x", EcStatus);
-	sc->ec_csrvalue = EcStatus;
-	wakeup(&sc->ec_csrvalue);
-	EcUnlock(sc);
-	goto re_enable;
-    }
-
-    /*
      * Send a query command to the EC to find out which _Qxx call it
      * wants to make.  This command clears the SCI bit and also the
-     * interrupt source since we are edge-triggered.
+     * interrupt source since we are edge-triggered.  To prevent the GPE
+     * that may arise from running the query from causing another query
+     * to be queued, we clear the pending flag only after running it.
      */
     Status = EcCommand(sc, EC_COMMAND_QUERY);
+    sc->ec_sci_pend = FALSE;
     if (ACPI_FAILURE(Status)) {
 	EcUnlock(sc);
-	ACPI_VPRINT(sc->ec_dev, acpi_device_get_parent_softc(sc->ec_dev),
-		    "GPE query failed - %s\n", AcpiFormatException(Status));
-	goto re_enable;
+	device_printf(sc->ec_dev, "GPE query failed: %s\n",
+	    AcpiFormatException(Status));
+	return;
     }
     Data = EC_GET_DATA(sc);
-    EcUnlock(sc);
 
     /* Ignore the value for "no outstanding event". (13.3.5) */
-    CTR2(KTR_ACPI, "ec query ok,%s running _Q%02x", Data ? "" : " not", Data);
-    if (Data == 0)
-	goto re_enable;
+    CTR2(KTR_ACPI, "ec query ok,%s running _Q%02X", Data ? "" : " not", Data);
+    if (Data == 0) {
+	EcUnlock(sc);
+	return;
+    }
 
     /* Evaluate _Qxx to respond to the controller. */
-    sprintf(qxx, "_Q%02x", Data);
+    snprintf(qxx, sizeof(qxx), "_Q%02X", Data);
     AcpiUtStrupr(qxx);
     Status = AcpiEvaluateObject(sc->ec_handle, qxx, NULL, NULL);
     if (ACPI_FAILURE(Status) && Status != AE_NOT_FOUND) {
-	ACPI_VPRINT(sc->ec_dev, acpi_device_get_parent_softc(sc->ec_dev),
-		    "evaluation of GPE query method %s failed - %s\n", 
-		    qxx, AcpiFormatException(Status));
+	device_printf(sc->ec_dev, "evaluation of query method %s failed: %s\n",
+	    qxx, AcpiFormatException(Status));
     }
 
-re_enable:
-    /* Re-enable the GPE event so we'll get future requests. */
-    Status = AcpiEnableGpe(sc->ec_gpehandle, sc->ec_gpebit, ACPI_NOT_ISR);
-    if (ACPI_FAILURE(Status))
-	printf("EcGpeQueryHandler: AcpiEnableEvent failed\n");
+    EcUnlock(sc);
 }
 
 /*
- * Handle a GPE.  Currently we only handle SCI events as others must
- * be handled by polling in EcWaitEvent().  This is because some ECs
- * treat events as level when they should be edge-triggered.
+ * The GPE handler is called when IBE/OBF or SCI events occur.  We are
+ * called from an unknown lock context.
  */
 static uint32_t
 EcGpeHandler(void *Context)
 {
     struct acpi_ec_softc *sc = Context;
     ACPI_STATUS		       Status;
+    EC_STATUS		       EcStatus;
 
     KASSERT(Context != NULL, ("EcGpeHandler called with NULL"));
+    CTR0(KTR_ACPI, "ec gpe handler start");
 
     /*
-     * Disable further GPEs while we handle this one.  Since we are directly
-     * called by ACPI-CA and it may have unknown locks held, we specify the
-     * ACPI_ISR flag to keep it from acquiring any more mutexes (which could
-     * potentially sleep.)
+     * Notify EcWaitEvent() that the status register is now fresh.  If we
+     * didn't do this, it wouldn't be possible to distinguish an old IBE
+     * from a new one, for example when doing a write transaction (writing
+     * address and then data values.)
      */
-    AcpiDisableGpe(sc->ec_gpehandle, sc->ec_gpebit, ACPI_ISR);
+    atomic_add_int(&sc->ec_gencount, 1);
+    wakeup(&sc->ec_gencount);
 
-    /* Schedule the GPE query handler. */
-    Status = AcpiOsQueueForExecution(OSD_PRIORITY_GPE, EcGpeQueryHandler,
-		Context);
-    if (ACPI_FAILURE(Status)) {
-	printf("Queuing GPE query handler failed.\n");
-	Status = AcpiEnableGpe(sc->ec_gpehandle, sc->ec_gpebit, ACPI_ISR);
-	if (ACPI_FAILURE(Status))
-	    printf("EcGpeHandler: AcpiEnableEvent failed\n");
+    /*
+     * If the EC_SCI bit of the status register is set, queue a query handler.
+     * It will run the query and _Qxx method later, under the lock.
+     */
+    EcStatus = EC_GET_CSR(sc);
+    if ((EcStatus & EC_EVENT_SCI) && !sc->ec_sci_pend) {
+	CTR0(KTR_ACPI, "ec gpe queueing query handler");
+	Status = AcpiOsQueueForExecution(OSD_PRIORITY_GPE, EcGpeQueryHandler,
+	    Context);
+	if (ACPI_SUCCESS(Status))
+	    sc->ec_sci_pend = TRUE;
+	else
+	    printf("EcGpeHandler: queuing GPE query handler failed\n");
     }
-
     return (0);
 }
 
@@ -825,6 +727,18 @@
     EcAddr = Address;
     Status = AE_ERROR;
 
+    /*
+     * If booting, check if we need to run the query handler.  If so, we
+     * we call it directly here since our thread taskq is not active yet.
+     */
+    if (cold || rebooting) {
+	if ((EC_GET_CSR(sc) & EC_EVENT_SCI)) {
+	    CTR0(KTR_ACPI, "ec running gpe handler directly");
+	    EcGpeQueryHandler(sc);
+	}
+    }
+
+    /* Serialize with EcGpeQueryHandler() at transaction granularity. */
     Status = EcLock(sc);
     if (ACPI_FAILURE(Status))
 	return_ACPI_STATUS (Status);
@@ -850,149 +764,188 @@
 	if (ACPI_FAILURE(Status))
 	    break;
     }
-
     EcUnlock(sc);
+
     return_ACPI_STATUS (Status);
 }
 
 static ACPI_STATUS
-EcWaitEvent(struct acpi_ec_softc *sc, EC_EVENT Event)
+EcWaitEvent(struct acpi_ec_softc *sc, EC_EVENT Event, u_int gen_count)
 {
     EC_STATUS	EcStatus;
     ACPI_STATUS	Status;
-    int		count, i, period, retval, slp_ival;
+    int		count, i, slp_ival;
 
     ACPI_SERIAL_ASSERT(ec);
     Status = AE_NO_HARDWARE_RESPONSE;
 
-    /* 
-     * Wait for 1 us before checking the CSR.  Testing shows about
-     * 50% of requests complete in 1 us and 90% of them complete
-     * in 5 us or less.
-     */
-    AcpiOsStall(1);
-
     /*
-     * Poll the EC status register for up to 1 ms in chunks of 10 us 
-     * to detect completion of the last command.
+     * The main CPU should be much faster than the EC.  So the status should
+     * be "not ready" when we start waiting.  But if the main CPU is really
+     * slow, it's possible we see the current "ready" response.  Since that
+     * can't be distinguished from the previous response in polled mode,
+     * this is a potential issue.  We really should have interrupts enabled
+     * during boot so there is no ambiguity in polled mode.  For now, let's
+     * collect some stats about how many systems actually have this issue.
      */
-    for (i = 0; i < 1000 / EC_POLL_DELAY; i++) {
+    if (cold || rebooting || ec_polled_mode) {
+	static int	once;
+
 	EcStatus = EC_GET_CSR(sc);
-	if (EVENT_READY(Event, EcStatus)) {
-	    Status = AE_OK;
-	    break;
+	if (!once && EVENT_READY(Event, EcStatus)) {
+	    device_printf(sc->ec_dev,
+		"warning: EC done before starting event wait\n");
+	    once = 1;
 	}
-	AcpiOsStall(EC_POLL_DELAY);
     }
-    period = i * EC_POLL_DELAY;
 
-    /*
-     * If we still don't have a response and we're up and running, wait up
-     * to ec_poll_timeout ms for completion, sleeping for chunks of 10 ms.
-     */
-    slp_ival = 0;
-    if (Status != AE_OK) {
-	retval = ENXIO;
-	count = ec_poll_timeout / 10;
-	if (count == 0)
+    /* Wait for event by polling or GPE (interrupt). */
+    if (cold || rebooting || ec_polled_mode) {
+	count = (ec_timeout * 1000) / EC_POLL_DELAY;
+	if (count <= 0)
 	    count = 1;
-	slp_ival = hz / 100;
-	if (slp_ival == 0)
-	    slp_ival = 1;
 	for (i = 0; i < count; i++) {
-	    if (retval != 0)
-		EcStatus = EC_GET_CSR(sc);
-	    else
-		EcStatus = sc->ec_csrvalue;
+	    EcStatus = EC_GET_CSR(sc);
+	    if (sc->ec_burstactive && !(EcStatus & EC_FLAG_BURST_MODE)) {
+		CTR0(KTR_ACPI, "ec burst disabled in waitevent (poll)");
+		sc->ec_burstactive = FALSE;
+	    }
 	    if (EVENT_READY(Event, EcStatus)) {
+		CTR1(KTR_ACPI, "ec poll wait ready, status %#x", EcStatus);
 		Status = AE_OK;
 		break;
 	    }
-	    if (!cold)
-		retval = tsleep(&sc->ec_csrvalue, PZERO, "ecpoll", slp_ival);
-	    else
-		AcpiOsStall(10000);
+	    AcpiOsStall(EC_POLL_DELAY);
+	}
+    } else {
+	slp_ival = hz / 1000;
+	if (slp_ival != 0) {
+	    count = ec_timeout / slp_ival;
+	} else {
+	    /* hz has less than 1000 Hz resolution so scale timeout. */
+	    slp_ival = 1;
+	    count = ec_timeout / (1000 / hz);
 	}
-    }
-
-    /* Calculate new delay and log it. */
-    if (slp_ival > 0)
-	period += i * 10000;
-    CTR2(KTR_ACPI, "ec got event %#x after %d us", EcStatus, period);
 
+	/*
+	 * Wait for the GPE to signal the status changed, checking the
+	 * status register each time.
+	 */
+	for (i = 0; i < count; i++) {
+	    if (gen_count != sc->ec_gencount) {
+		/*
+		 * Record new generation count.  It's possible the GPE was
+		 * just to notify us that a query is needed and we need to
+		 * wait for a second GPE to signal the completion of the
+		 * event we are actually waiting for.
+		 */
+		gen_count = sc->ec_gencount;
+		EcStatus = EC_GET_CSR(sc);
+		if (sc->ec_burstactive && !(EcStatus & EC_FLAG_BURST_MODE)) {
+		    CTR0(KTR_ACPI, "ec burst disabled in waitevent (sleep)");
+		    sc->ec_burstactive = FALSE;
+		}
+		if (EVENT_READY(Event, EcStatus)) {
+		    CTR1(KTR_ACPI, "ec sleep wait ready, status %#x", EcStatus);
+		    Status = AE_OK;
+		    break;
+		}
+	    }
+	    tsleep(&sc->ec_gencount, PZERO, "ecgpe", slp_ival);
+	}
+    }
+    if (Status != AE_OK)
+	    CTR0(KTR_ACPI, "error: ec wait timed out");
     return (Status);
-}    
+}
 
 static ACPI_STATUS
 EcCommand(struct acpi_ec_softc *sc, EC_COMMAND cmd)
 {
-    ACPI_STATUS	Status;
-    EC_EVENT	Event;
+    ACPI_STATUS	status;
+    EC_EVENT	event;
+    EC_STATUS	ec_status;
+    u_int	gen_count;
 
     ACPI_SERIAL_ASSERT(ec);
 
+    /* Don't use burst mode if user disabled it. */
+    if (!ec_burst_mode && cmd == EC_COMMAND_BURST_ENABLE)
+	return (AE_ERROR);
+
     /* Decide what to wait for based on command type. */
     switch (cmd) {
     case EC_COMMAND_READ:
     case EC_COMMAND_WRITE:
     case EC_COMMAND_BURST_DISABLE:
-	Event = EC_EVENT_INPUT_BUFFER_EMPTY;
+	event = EC_EVENT_INPUT_BUFFER_EMPTY;
 	break;
     case EC_COMMAND_QUERY:
     case EC_COMMAND_BURST_ENABLE:
-	Event = EC_EVENT_OUTPUT_BUFFER_FULL;
+	event = EC_EVENT_OUTPUT_BUFFER_FULL;
 	break;
     default:
-	ACPI_VPRINT(sc->ec_dev, acpi_device_get_parent_softc(sc->ec_dev),
-		    "EcCommand: Invalid command %#x\n", cmd);
+	device_printf(sc->ec_dev, "EcCommand: invalid command %#x\n", cmd);
 	return (AE_BAD_PARAMETER);
     }
 
     /* Run the command and wait for the chosen event. */
+    CTR1(KTR_ACPI, "ec running command %#x", cmd);
+    gen_count = sc->ec_gencount;
     EC_SET_CSR(sc, cmd);
-    Status = EcWaitEvent(sc, Event);
-    if (ACPI_FAILURE(Status)) {
-	ACPI_VPRINT(sc->ec_dev, acpi_device_get_parent_softc(sc->ec_dev),
-		    "EcCommand: no response to %#x\n", cmd);
-    }
-
-    return (Status);
+    status = EcWaitEvent(sc, event, gen_count);
+    if (ACPI_SUCCESS(status)) {
+	/* If we succeeded, burst flag should now be present. */
+	if (cmd == EC_COMMAND_BURST_ENABLE) {
+	    ec_status = EC_GET_CSR(sc);
+	    if ((ec_status & EC_FLAG_BURST_MODE) == 0)
+		status = AE_ERROR;
+	}
+    } else
+	device_printf(sc->ec_dev, "EcCommand: no response to %#x\n", cmd);
+    return (status);
 }
 
 static ACPI_STATUS
 EcRead(struct acpi_ec_softc *sc, UINT8 Address, UINT8 *Data)
 {
-    ACPI_STATUS	Status;
+    ACPI_STATUS	status;
+    UINT8 data;
+    u_int gen_count;
 
     ACPI_SERIAL_ASSERT(ec);
     CTR1(KTR_ACPI, "ec read from %#x", Address);
 
-#ifdef notyet
     /* If we can't start burst mode, continue anyway. */
-    EcCommand(sc, EC_COMMAND_BURST_ENABLE);
-#endif
+    status = EcCommand(sc, EC_COMMAND_BURST_ENABLE);
+    if (status == AE_OK) {
+    	data = EC_GET_DATA(sc);
+	if (data == EC_BURST_ACK) {
+	    CTR0(KTR_ACPI, "ec burst enabled");
+	    sc->ec_burstactive = TRUE;
+	}
+    }
 
-    Status = EcCommand(sc, EC_COMMAND_READ);
-    if (ACPI_FAILURE(Status))
-	return (Status);
+    status = EcCommand(sc, EC_COMMAND_READ);
+    if (ACPI_FAILURE(status))
+	return (status);
 
+    gen_count = sc->ec_gencount;
     EC_SET_DATA(sc, Address);
-    Status = EcWaitEvent(sc, EC_EVENT_OUTPUT_BUFFER_FULL);
-    if (ACPI_FAILURE(Status)) {
-	ACPI_VPRINT(sc->ec_dev, acpi_device_get_parent_softc(sc->ec_dev),
-		    "EcRead: Failed waiting for EC to send data.\n");
-	return (Status);
+    status = EcWaitEvent(sc, EC_EVENT_OUTPUT_BUFFER_FULL, gen_count);
+    if (ACPI_FAILURE(status)) {
+	device_printf(sc->ec_dev, "EcRead: failed waiting to get data\n");
+	return (status);
     }
-
     *Data = EC_GET_DATA(sc);
 
-#ifdef notyet
     if (sc->ec_burstactive) {
-	Status = EcCommand(sc, EC_COMMAND_BURST_DISABLE);
-	if (ACPI_FAILURE(Status))
-	    return (Status);
+	sc->ec_burstactive = FALSE;
+	status = EcCommand(sc, EC_COMMAND_BURST_DISABLE);
+	if (ACPI_FAILURE(status))
+	    return (status);
+	CTR0(KTR_ACPI, "ec disabled burst ok");
     }
-#endif
 
     return (AE_OK);
 }    
@@ -1000,43 +953,50 @@
 static ACPI_STATUS
 EcWrite(struct acpi_ec_softc *sc, UINT8 Address, UINT8 *Data)
 {
-    ACPI_STATUS	Status;
+    ACPI_STATUS	status;
+    UINT8 data;
+    u_int gen_count;
 
     ACPI_SERIAL_ASSERT(ec);
     CTR2(KTR_ACPI, "ec write to %#x, data %#x", Address, *Data);
 
-#ifdef notyet
     /* If we can't start burst mode, continue anyway. */
-    EcCommand(sc, EC_COMMAND_BURST_ENABLE);
-#endif
+    status = EcCommand(sc, EC_COMMAND_BURST_ENABLE);
+    if (status == AE_OK) {
+    	data = EC_GET_DATA(sc);
+	if (data == EC_BURST_ACK) {
+	    CTR0(KTR_ACPI, "ec burst enabled");
+	    sc->ec_burstactive = TRUE;
+	}
+    }
 
-    Status = EcCommand(sc, EC_COMMAND_WRITE);
-    if (ACPI_FAILURE(Status))
-	return (Status);
+    status = EcCommand(sc, EC_COMMAND_WRITE);
+    if (ACPI_FAILURE(status))
+	return (status);
 
+    gen_count = sc->ec_gencount;
     EC_SET_DATA(sc, Address);
-    Status = EcWaitEvent(sc, EC_EVENT_INPUT_BUFFER_EMPTY);
-    if (ACPI_FAILURE(Status)) {
-	ACPI_VPRINT(sc->ec_dev, acpi_device_get_parent_softc(sc->ec_dev),
-		    "EcRead: Failed waiting for EC to process address\n");
-	return (Status);
+    status = EcWaitEvent(sc, EC_EVENT_INPUT_BUFFER_EMPTY, gen_count);
+    if (ACPI_FAILURE(status)) {
+	device_printf(sc->ec_dev, "EcRead: failed waiting for sent address\n");
+	return (status);
     }
 
+    gen_count = sc->ec_gencount;
     EC_SET_DATA(sc, *Data);
-    Status = EcWaitEvent(sc, EC_EVENT_INPUT_BUFFER_EMPTY);
-    if (ACPI_FAILURE(Status)) {
-	ACPI_VPRINT(sc->ec_dev, acpi_device_get_parent_softc(sc->ec_dev),
-		    "EcWrite: Failed waiting for EC to process data\n");
-	return (Status);
+    status = EcWaitEvent(sc, EC_EVENT_INPUT_BUFFER_EMPTY, gen_count);
+    if (ACPI_FAILURE(status)) {
+	device_printf(sc->ec_dev, "EcWrite: failed waiting for sent data\n");
+	return (status);
     }
 
-#ifdef notyet
     if (sc->ec_burstactive) {
-	Status = EcCommand(sc, EC_COMMAND_BURST_DISABLE);
-	if (ACPI_FAILURE(Status))
-	    return (Status);
+	sc->ec_burstactive = FALSE;
+	status = EcCommand(sc, EC_COMMAND_BURST_DISABLE);
+	if (ACPI_FAILURE(status))
+	    return (status);
+	CTR0(KTR_ACPI, "ec disabled burst ok");
     }
-#endif
 
     return (AE_OK);
 }

--------------040800050701000906010408
Content-Type: text/x-patch;
 name="ecng-7b.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="ecng-7b.diff"

--- sys/dev/acpica/acpi_ec.c	15 Jun 2007 18:02:33 -0000	1.75
+++ sys/dev/acpica/acpi_ec.c	6 Sep 2007 22:23:52 -0000
@@ -1,5 +1,5 @@
 /*-
- * Copyright (c) 2003 Nate Lawson
+ * Copyright (c) 2003-2007 Nate Lawson
  * Copyright (c) 2000 Michael Smith
  * Copyright (c) 2000 BSDi
  * All rights reserved.
@@ -25,115 +25,6 @@
  * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
  * SUCH DAMAGE.
  */
-/*-
- ******************************************************************************
- *
- * 1. Copyright Notice
- *
- * Some or all of this work - Copyright (c) 1999, Intel Corp.  All rights
- * reserved.
- *
- * 2. License
- *
- * 2.1. This is your license from Intel Corp. under its intellectual property
- * rights.  You may have additional license terms from the party that provided
- * you this software, covering your right to use that party's intellectual
- * property rights.
- *
- * 2.2. Intel grants, free of charge, to any person ("Licensee") obtaining a
- * copy of the source code appearing in this file ("Covered Code") an
- * irrevocable, perpetual, worldwide license under Intel's copyrights in the
- * base code distributed originally by Intel ("Original Intel Code") to copy,
- * make derivatives, distribute, use and display any portion of the Covered
- * Code in any form, with the right to sublicense such rights; and
- *
- * 2.3. Intel grants Licensee a non-exclusive and non-transferable patent
- * license (with the right to sublicense), under only those claims of Intel
- * patents that are infringed by the Original Intel Code, to make, use, sell,
- * offer to sell, and import the Covered Code and derivative works thereof
- * solely to the minimum extent necessary to exercise the above copyright
- * license, and in no event shall the patent license extend to any additions
- * to or modifications of the Original Intel Code.  No other license or right
- * is granted directly or by implication, estoppel or otherwise;
- *
- * The above copyright and patent license is granted only if the following
- * conditions are met:
- *
- * 3. Conditions 
- *
- * 3.1. Redistribution of Source with Rights to Further Distribute Source.  
- * Redistribution of source code of any substantial portion of the Covered
- * Code or modification with rights to further distribute source must include
- * the above Copyright Notice, the above License, this list of Conditions,
- * and the following Disclaimer and Export Compliance provision.  In addition,
- * Licensee must cause all Covered Code to which Licensee contributes to
- * contain a file documenting the changes Licensee made to create that Covered
- * Code and the date of any change.  Licensee must include in that file the
- * documentation of any changes made by any predecessor Licensee.  Licensee 
- * must include a prominent statement that the modification is derived,
- * directly or indirectly, from Original Intel Code.
- *
- * 3.2. Redistribution of Source with no Rights to Further Distribute Source.  
- * Redistribution of source code of any substantial portion of the Covered
- * Code or modification without rights to further distribute source must
- * include the following Disclaimer and Export Compliance provision in the
- * documentation and/or other materials provided with distribution.  In
- * addition, Licensee may not authorize further sublicense of source of any
- * portion of the Covered Code, and must include terms to the effect that the
- * license from Licensee to its licensee is limited to the intellectual
- * property embodied in the software Licensee provides to its licensee, and
- * not to intellectual property embodied in modifications its licensee may
- * make.
- *
- * 3.3. Redistribution of Executable. Redistribution in executable form of any
- * substantial portion of the Covered Code or modification must reproduce the
- * above Copyright Notice, and the following Disclaimer and Export Compliance
- * provision in the documentation and/or other materials provided with the
- * distribution.
- *
- * 3.4. Intel retains all right, title, and interest in and to the Original
- * Intel Code.
- *
- * 3.5. Neither the name Intel nor any other trademark owned or controlled by
- * Intel shall be used in advertising or otherwise to promote the sale, use or
- * other dealings in products derived from or relating to the Covered Code
- * without prior written authorization from Intel.
- *
- * 4. Disclaimer and Export Compliance
- *
- * 4.1. INTEL MAKES NO WARRANTY OF ANY KIND REGARDING ANY SOFTWARE PROVIDED
- * HERE.  ANY SOFTWARE ORIGINATING FROM INTEL OR DERIVED FROM INTEL SOFTWARE
- * IS PROVIDED "AS IS," AND INTEL WILL NOT PROVIDE ANY SUPPORT,  ASSISTANCE,
- * INSTALLATION, TRAINING OR OTHER SERVICES.  INTEL WILL NOT PROVIDE ANY
- * UPDATES, ENHANCEMENTS OR EXTENSIONS.  INTEL SPECIFICALLY DISCLAIMS ANY
- * IMPLIED WARRANTIES OF MERCHANTABILITY, NONINFRINGEMENT AND FITNESS FOR A
- * PARTICULAR PURPOSE. 
- *
- * 4.2. IN NO EVENT SHALL INTEL HAVE ANY LIABILITY TO LICENSEE, ITS LICENSEES
- * OR ANY OTHER THIRD PARTY, FOR ANY LOST PROFITS, LOST DATA, LOSS OF USE OR
- * COSTS OF PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES, OR FOR ANY INDIRECT,
- * SPECIAL OR CONSEQUENTIAL DAMAGES ARISING OUT OF THIS AGREEMENT, UNDER ANY
- * CAUSE OF ACTION OR THEORY OF LIABILITY, AND IRRESPECTIVE OF WHETHER INTEL
- * HAS ADVANCE NOTICE OF THE POSSIBILITY OF SUCH DAMAGES.  THESE LIMITATIONS
- * SHALL APPLY NOTWITHSTANDING THE FAILURE OF THE ESSENTIAL PURPOSE OF ANY
- * LIMITED REMEDY.
- *
- * 4.3. Licensee shall not export, either directly or indirectly, any of this
- * software or system incorporating such software without first obtaining any
- * required license or other approval from the U. S. Department of Commerce or
- * any other agency or department of the United States Government.  In the
- * event Licensee exports any such software from the United States or
- * re-exports any such software from a foreign destination, Licensee shall
- * ensure that the distribution and export/re-export of the software is in
- * compliance with all laws, regulations, orders, or other restrictions of the
- * U.S. Export Administration Regulations. Licensee agrees that neither it nor
- * any of its subsidiaries will export/re-export any technical data, process,
- * software, or service, directly or indirectly, to any country for which the
- * United States government or any agency thereof requires an export license,
- * other governmental approval, or letter of assurance, without first obtaining
- * such license, approval or letter.
- *
- *****************************************************************************/
 
 #include <sys/cdefs.h>
 __FBSDID("$FreeBSD$");
@@ -248,7 +139,6 @@
     int			ec_uid;
     ACPI_HANDLE		ec_gpehandle;
     UINT8		ec_gpebit;
-    UINT8		ec_csrvalue;
     
     int			ec_data_rid;
     struct resource	*ec_data_res;
@@ -260,11 +150,11 @@
     bus_space_tag_t	ec_csr_tag;
     bus_space_handle_t	ec_csr_handle;
 
-    struct mtx		ec_mtx;
     int			ec_glk;
     int			ec_glkhandle;
     int			ec_burstactive;
     int			ec_sci_pend;
+    u_int		ec_gencount;
 };
 
 /*
@@ -275,13 +165,10 @@
 #define EC_LOCK_TIMEOUT	1000
 
 /* Default delay in microseconds between each run of the status polling loop. */
-#define EC_POLL_DELAY	10
-
-/* Default time in microseconds spent polling before sleep waiting. */
-#define EC_POLL_TIME	500
+#define EC_POLL_DELAY	5
 
 /* Total time in ms spent waiting for a response from EC. */
-#define EC_TIMEOUT	500
+#define EC_TIMEOUT	750
 
 #define EVENT_READY(event, status)			\
 	(((event) == EC_EVENT_OUTPUT_BUFFER_FULL &&	\
@@ -298,17 +185,17 @@
 TUNABLE_INT("debug.acpi.ec.burst", &ec_burst_mode);
 SYSCTL_INT(_debug_acpi_ec, OID_AUTO, burst, CTLFLAG_RW, &ec_burst_mode, 0,
     "Enable use of burst mode (faster for nearly all systems)");
-static int	ec_poll_time = EC_POLL_TIME;
-TUNABLE_INT("debug.acpi.ec.poll_time", &ec_poll_time);
-SYSCTL_INT(_debug_acpi_ec, OID_AUTO, poll_time, CTLFLAG_RW, &ec_poll_time,
-    EC_POLL_TIME, "Time spent polling vs. sleeping (CPU intensive)");
+static int	ec_polled_mode;
+TUNABLE_INT("debug.acpi.ec.polled", &ec_polled_mode);
+SYSCTL_INT(_debug_acpi_ec, OID_AUTO, polled, CTLFLAG_RW, &ec_polled_mode, 0,
+    "Force use of polled mode (only if interrupt mode doesn't work)");
 static int	ec_timeout = EC_TIMEOUT;
 TUNABLE_INT("debug.acpi.ec.timeout", &ec_timeout);
 SYSCTL_INT(_debug_acpi_ec, OID_AUTO, timeout, CTLFLAG_RW, &ec_timeout,
     EC_TIMEOUT, "Total time spent waiting for a response (poll+sleep)");
 
-static __inline ACPI_STATUS
-EcLock(struct acpi_ec_softc *sc, int serialize)
+static ACPI_STATUS
+EcLock(struct acpi_ec_softc *sc)
 {
     ACPI_STATUS	status;
 
@@ -319,25 +206,14 @@
 	if (ACPI_FAILURE(status))
 	    return (status);
     }
-
-    /*
-     * If caller is executing a series of commands, acquire the exclusive lock
-     * to serialize with other users.
-     * To sync with bottom-half interrupt handler, always acquire the mutex.
-     */
-    if (serialize)
-	ACPI_SERIAL_BEGIN(ec);
-    mtx_lock(&sc->ec_mtx);
-
+    ACPI_SERIAL_BEGIN(ec);
     return (status);
 }
 
-static __inline void
+static void
 EcUnlock(struct acpi_ec_softc *sc)
 {
-    mtx_unlock(&sc->ec_mtx);
-    if (sx_xlocked(&ec_sxlock))
-	ACPI_SERIAL_END(ec);
+    ACPI_SERIAL_END(ec);
     if (sc->ec_glk)
 	AcpiReleaseGlobalLock(sc->ec_glkhandle);
 }
@@ -349,7 +225,8 @@
 				ACPI_PHYSICAL_ADDRESS Address,
 				UINT32 width, ACPI_INTEGER *Value,
 				void *Context, void *RegionContext);
-static ACPI_STATUS	EcWaitEvent(struct acpi_ec_softc *sc, EC_EVENT Event);
+static ACPI_STATUS	EcWaitEvent(struct acpi_ec_softc *sc, EC_EVENT Event,
+				u_int gen_count);
 static ACPI_STATUS	EcCommand(struct acpi_ec_softc *sc, EC_COMMAND cmd);
 static ACPI_STATUS	EcRead(struct acpi_ec_softc *sc, UINT8 Address,
 				UINT8 *Data);
@@ -390,7 +267,9 @@
  * Look for an ECDT and if we find one, set up default GPE and 
  * space handlers to catch attempts to access EC space before
  * we have a real driver instance in place.
- * TODO: if people report invalid ECDTs, add a tunable to disable them.
+ *
+ * TODO: Some old Gateway laptops need us to fake up an ECDT or
+ * otherwise attach early so that _REG methods can run.
  */
 void
 acpi_ec_ecdt_probe(device_t parent)
@@ -578,7 +457,6 @@
     params = acpi_get_private(dev);
     sc->ec_dev = dev;
     sc->ec_handle = acpi_get_handle(dev);
-    mtx_init(&sc->ec_mtx, "ACPI EC lock", NULL, MTX_DEF);
 
     /* Retrieve previously probed values via device ivars. */
     sc->ec_glk = params->glk;
@@ -661,7 +539,6 @@
     if (sc->ec_data_res)
 	bus_release_resource(sc->ec_dev, SYS_RES_IOPORT, sc->ec_data_rid,
 			     sc->ec_data_res);
-    mtx_destroy(&sc->ec_mtx);
     return (ENXIO);
 }
 
@@ -715,57 +592,52 @@
     KASSERT(Context != NULL, ("EcGpeQueryHandler called with NULL"));
 
     /* Serialize user access with EcSpaceHandler(). */
-    Status = EcLock(sc, TRUE);
+    Status = EcLock(sc);
     if (ACPI_FAILURE(Status)) {
-	ACPI_VPRINT(sc->ec_dev, acpi_device_get_parent_softc(sc->ec_dev),
-		    "GpeQuery lock error: %s\n", AcpiFormatException(Status));
+	device_printf(sc->ec_dev, "GpeQuery lock error: %s\n",
+	    AcpiFormatException(Status));
 	return;
     }
 
     /*
      * Send a query command to the EC to find out which _Qxx call it
      * wants to make.  This command clears the SCI bit and also the
-     * interrupt source since we are edge-triggered.
+     * interrupt source since we are edge-triggered.  To prevent the GPE
+     * that may arise from running the query from causing another query
+     * to be queued, we clear the pending flag only after running it.
      */
     Status = EcCommand(sc, EC_COMMAND_QUERY);
+    sc->ec_sci_pend = FALSE;
     if (ACPI_FAILURE(Status)) {
 	EcUnlock(sc);
-	ACPI_VPRINT(sc->ec_dev, acpi_device_get_parent_softc(sc->ec_dev),
-		    "GPE query failed - %s\n", AcpiFormatException(Status));
-	goto re_enable;
+	device_printf(sc->ec_dev, "GPE query failed: %s\n",
+	    AcpiFormatException(Status));
+	return;
     }
     Data = EC_GET_DATA(sc);
-    sc->ec_sci_pend = FALSE;
-
-    /* Drop locks before evaluating _Qxx method since it may trigger GPEs. */
-    EcUnlock(sc);
 
     /* Ignore the value for "no outstanding event". (13.3.5) */
-    CTR2(KTR_ACPI, "ec query ok,%s running _Q%02x", Data ? "" : " not", Data);
-    if (Data == 0)
-	goto re_enable;
+    CTR2(KTR_ACPI, "ec query ok,%s running _Q%02X", Data ? "" : " not", Data);
+    if (Data == 0) {
+	EcUnlock(sc);
+	return;
+    }
 
     /* Evaluate _Qxx to respond to the controller. */
-    snprintf(qxx, sizeof(qxx), "_Q%02x", Data);
+    snprintf(qxx, sizeof(qxx), "_Q%02X", Data);
     AcpiUtStrupr(qxx);
     Status = AcpiEvaluateObject(sc->ec_handle, qxx, NULL, NULL);
     if (ACPI_FAILURE(Status) && Status != AE_NOT_FOUND) {
-	ACPI_VPRINT(sc->ec_dev, acpi_device_get_parent_softc(sc->ec_dev),
-		    "evaluation of GPE query method %s failed - %s\n", 
-		    qxx, AcpiFormatException(Status));
+	device_printf(sc->ec_dev, "evaluation of query method %s failed: %s\n",
+	    qxx, AcpiFormatException(Status));
     }
 
-re_enable:
-    /* Re-enable the GPE event so we'll get future requests. */
-    Status = AcpiEnableGpe(sc->ec_gpehandle, sc->ec_gpebit, ACPI_ISR);
-    if (ACPI_FAILURE(Status))
-	printf("EcGpeQueryHandler: AcpiEnableEvent failed\n");
+    EcUnlock(sc);
 }
 
 /*
- * Handle a GPE.  Currently we only handle SCI events as others must
- * be handled by polling in EcWaitEvent().  This is because some ECs
- * treat events as level when they should be edge-triggered.
+ * The GPE handler is called when IBE/OBF or SCI events occur.  We are
+ * called from an unknown lock context.
  */
 static uint32_t
 EcGpeHandler(void *Context)
@@ -773,68 +645,32 @@
     struct acpi_ec_softc *sc = Context;
     ACPI_STATUS		       Status;
     EC_STATUS		       EcStatus;
-    int			       query_pend;
 
     KASSERT(Context != NULL, ("EcGpeHandler called with NULL"));
+    CTR0(KTR_ACPI, "ec gpe handler start");
 
     /*
-     * Disable further GPEs while we handle this one.  Since we are directly
-     * called by ACPI-CA and it may have unknown locks held, we specify the
-     * ACPI_ISR flag to keep it from acquiring any more mutexes (although
-     * sleeping would be ok since we're in an ithread.)
+     * Notify EcWaitEvent() that the status register is now fresh.  If we
+     * didn't do this, it wouldn't be possible to distinguish an old IBE
+     * from a new one, for example when doing a write transaction (writing
+     * address and then data values.)
      */
-    AcpiDisableGpe(sc->ec_gpehandle, sc->ec_gpebit, ACPI_ISR);
-
-    /* For interrupt (GPE) handler, don't acquire serialization lock. */
-    Status = EcLock(sc, FALSE);
-    if (ACPI_FAILURE(Status)) {
-	ACPI_VPRINT(sc->ec_dev, acpi_device_get_parent_softc(sc->ec_dev),
-		    "GpeQuery lock error: %s\n", AcpiFormatException(Status));
-	return (-1);
-    }
+    atomic_add_int(&sc->ec_gencount, 1);
+    wakeup(&sc->ec_gencount);
 
     /*
-     * If burst was active, but the status bit was cleared, the EC had to
-     * exit burst mode for some reason.  Record this for later.
+     * If the EC_SCI bit of the status register is set, queue a query handler.
+     * It will run the query and _Qxx method later, under the lock.
      */
     EcStatus = EC_GET_CSR(sc);
-    if (sc->ec_burstactive && (EcStatus & EC_FLAG_BURST_MODE) == 0) {
-	CTR0(KTR_ACPI, "ec burst disabled in query handler");
-	sc->ec_burstactive = FALSE;
-    }
-
-    /*
-     * If the EC_SCI bit of the status register is not set, then pass
-     * it along to any potential waiters as it may be an IBE/OBF event.
-     * If it is set, queue a query handler.
-     */
-    query_pend = FALSE;
-    if ((EcStatus & EC_EVENT_SCI) == 0) {
-	CTR1(KTR_ACPI, "ec event was IBE/OBF, status %#x", EcStatus);
-	sc->ec_csrvalue = EcStatus;
-	wakeup(&sc->ec_csrvalue);
-    } else if (!sc->ec_sci_pend) {
-	/* SCI bit set and no pending query handler, so schedule one. */
-	CTR0(KTR_ACPI, "ec queueing gpe handler");
+    if ((EcStatus & EC_EVENT_SCI) && !sc->ec_sci_pend) {
+	CTR0(KTR_ACPI, "ec gpe queueing query handler");
 	Status = AcpiOsExecute(OSL_GPE_HANDLER, EcGpeQueryHandler, Context);
-	if (ACPI_SUCCESS(Status)) {
+	if (ACPI_SUCCESS(Status))
 	    sc->ec_sci_pend = TRUE;
-	    query_pend = TRUE;
-	} else
-	    printf("Queuing GPE query handler failed.\n");
-    }
-
-    /*
-     * If we didn't queue a query handler, which will eventually re-enable
-     * the GPE, re-enable it right now so we can get more events.
-     */
-    if (!query_pend) {
-	Status = AcpiEnableGpe(sc->ec_gpehandle, sc->ec_gpebit, ACPI_ISR);
-	if (ACPI_FAILURE(Status))
-	    printf("EcGpeHandler: AcpiEnableGpe failed\n");
+	else
+	    printf("EcGpeHandler: queuing GPE query handler failed\n");
     }
-
-    EcUnlock(sc);
     return (0);
 }
 
@@ -878,8 +714,19 @@
     EcAddr = Address;
     Status = AE_ERROR;
 
-    /* Grab serialization lock to hold across command sequence. */
-    Status = EcLock(sc, TRUE);
+    /*
+     * If booting, check if we need to run the query handler.  If so, we
+     * we call it directly here since our thread taskq is not active yet.
+     */
+    if (cold || rebooting) {
+	if ((EC_GET_CSR(sc) & EC_EVENT_SCI)) {
+	    CTR0(KTR_ACPI, "ec running gpe handler directly");
+	    EcGpeQueryHandler(sc);
+	}
+    }
+
+    /* Serialize with EcGpeQueryHandler() at transaction granularity. */
+    Status = EcLock(sc);
     if (ACPI_FAILURE(Status))
 	return_ACPI_STATUS (Status);
 
@@ -910,83 +757,94 @@
 }
 
 static ACPI_STATUS
-EcWaitEvent(struct acpi_ec_softc *sc, EC_EVENT Event)
+EcWaitEvent(struct acpi_ec_softc *sc, EC_EVENT Event, u_int gen_count)
 {
     EC_STATUS	EcStatus;
     ACPI_STATUS	Status;
-    int		count, i, retval, slp_ival;
+    int		count, i, slp_ival;
 
     ACPI_SERIAL_ASSERT(ec);
     Status = AE_NO_HARDWARE_RESPONSE;
-    EcStatus = 0;
 
     /*
-     * Poll for up to ec_poll_time microseconds since many ECs complete
-     * the command quickly, especially if in burst mode.
+     * The main CPU should be much faster than the EC.  So the status should
+     * be "not ready" when we start waiting.  But if the main CPU is really
+     * slow, it's possible we see the current "ready" response.  Since that
+     * can't be distinguished from the previous response in polled mode,
+     * this is a potential issue.  We really should have interrupts enabled
+     * during boot so there is no ambiguity in polled mode.  For now, let's
+     * collect some stats about how many systems actually have this issue.
      */
-#if 0 /* Enable this as a possible workaround if EC times out. */
-    AcpiOsStall(EC_POLL_DELAY);
-#endif
-    count = ec_poll_time / EC_POLL_DELAY;
-    if (count <= 0)
-	count = 1;
-    for (i = 0; i < count; i++) {
+    if (cold || rebooting || ec_polled_mode) {
+	static int	once;
+
 	EcStatus = EC_GET_CSR(sc);
-	if (sc->ec_burstactive && (EcStatus & EC_FLAG_BURST_MODE) == 0) {
-	    CTR0(KTR_ACPI, "ec burst disabled in waitevent (poll)");
-	    sc->ec_burstactive = FALSE;
+	if (!once && EVENT_READY(Event, EcStatus)) {
+	    device_printf(sc->ec_dev,
+		"warning: EC done before starting event wait\n");
+	    once = 1;
 	}
-	if (EVENT_READY(Event, EcStatus)) {
-	    CTR1(KTR_ACPI, "ec poll wait ready, status %#x", EcStatus);
-	    Status = AE_OK;
-	    break;
-	}
-	AcpiOsStall(EC_POLL_DELAY);
     }
 
-    /*
-     * If we still don't have a response and we're up and running, wait up
-     * to ec_timeout ms for completion, sleeping for chunks of 1 ms or the
-     * smallest resolution hz supports.
-     */
-    slp_ival = 0;
-    if (Status != AE_OK) {
-	retval = ENXIO;
-	if (!cold) {
-	    slp_ival = hz / 1000;
-	    if (slp_ival != 0) {
-		count = ec_timeout / slp_ival;
-	    } else {
-		/* hz has less than 1000 Hz resolution so scale timeout. */
-		slp_ival = 1;
-		count = ec_timeout / (1000 / hz);
-	    }
-	} else
-	    count = ec_timeout;
+    /* Wait for event by polling or GPE (interrupt). */
+    if (cold || rebooting || ec_polled_mode) {
+	count = (ec_timeout * 1000) / EC_POLL_DELAY;
+	if (count <= 0)
+	    count = 1;
 	for (i = 0; i < count; i++) {
-	    if (retval != 0)
-		EcStatus = EC_GET_CSR(sc);
-	    else
-		EcStatus = sc->ec_csrvalue;
-	    if (sc->ec_burstactive && (EcStatus & EC_FLAG_BURST_MODE) == 0) {
-		CTR0(KTR_ACPI, "ec burst disabled in waitevent (slp)");
+	    EcStatus = EC_GET_CSR(sc);
+	    if (sc->ec_burstactive && !(EcStatus & EC_FLAG_BURST_MODE)) {
+		CTR0(KTR_ACPI, "ec burst disabled in waitevent (poll)");
 		sc->ec_burstactive = FALSE;
 	    }
 	    if (EVENT_READY(Event, EcStatus)) {
-		CTR1(KTR_ACPI, "ec sleep wait ready, status %#x", EcStatus);
+		CTR1(KTR_ACPI, "ec poll wait ready, status %#x", EcStatus);
 		Status = AE_OK;
 		break;
 	    }
-	    if (!cold) {
-		retval = msleep(&sc->ec_csrvalue, &sc->ec_mtx, PZERO, "ecpoll",
-		    slp_ival);
-	    } else
-		AcpiOsStall(1000);
+	    AcpiOsStall(EC_POLL_DELAY);
+	}
+    } else {
+	slp_ival = hz / 1000;
+	if (slp_ival != 0) {
+	    count = ec_timeout / slp_ival;
+	} else {
+	    /* hz has less than 1000 Hz resolution so scale timeout. */
+	    slp_ival = 1;
+	    count = ec_timeout / (1000 / hz);
 	}
-    }
 
+	/*
+	 * Wait for the GPE to signal the status changed, checking the
+	 * status register each time.
+	 */
+	for (i = 0; i < count; i++) {
+	    if (gen_count != sc->ec_gencount) {
+		/*
+		 * Record new generation count.  It's possible the GPE was
+		 * just to notify us that a query is needed and we need to
+		 * wait for a second GPE to signal the completion of the
+		 * event we are actually waiting for.
+		 */
+		gen_count = sc->ec_gencount;
+		EcStatus = EC_GET_CSR(sc);
+		if (sc->ec_burstactive && !(EcStatus & EC_FLAG_BURST_MODE)) {
+		    CTR0(KTR_ACPI, "ec burst disabled in waitevent (sleep)");
+		    sc->ec_burstactive = FALSE;
+		}
+		if (EVENT_READY(Event, EcStatus)) {
+		    CTR1(KTR_ACPI, "ec sleep wait ready, status %#x", EcStatus);
+		    Status = AE_OK;
+		    break;
+		}
+	    }
+	    tsleep(&sc->ec_gencount, PZERO, "ecgpe", slp_ival);
+	}
+    }
+    if (Status != AE_OK)
+	    CTR0(KTR_ACPI, "error: ec wait timed out");
     return (Status);
-}    
+}
 
 static ACPI_STATUS
 EcCommand(struct acpi_ec_softc *sc, EC_COMMAND cmd)
@@ -994,6 +852,7 @@
     ACPI_STATUS	status;
     EC_EVENT	event;
     EC_STATUS	ec_status;
+    u_int	gen_count;
 
     ACPI_SERIAL_ASSERT(ec);
 
@@ -1013,15 +872,15 @@
 	event = EC_EVENT_OUTPUT_BUFFER_FULL;
 	break;
     default:
-	ACPI_VPRINT(sc->ec_dev, acpi_device_get_parent_softc(sc->ec_dev),
-		    "EcCommand: Invalid command %#x\n", cmd);
+	device_printf(sc->ec_dev, "EcCommand: invalid command %#x\n", cmd);
 	return (AE_BAD_PARAMETER);
     }
 
     /* Run the command and wait for the chosen event. */
     CTR1(KTR_ACPI, "ec running command %#x", cmd);
+    gen_count = sc->ec_gencount;
     EC_SET_CSR(sc, cmd);
-    status = EcWaitEvent(sc, event);
+    status = EcWaitEvent(sc, event, gen_count);
     if (ACPI_SUCCESS(status)) {
 	/* If we succeeded, burst flag should now be present. */
 	if (cmd == EC_COMMAND_BURST_ENABLE) {
@@ -1029,11 +888,8 @@
 	    if ((ec_status & EC_FLAG_BURST_MODE) == 0)
 		status = AE_ERROR;
 	}
-    } else {
-	ACPI_VPRINT(sc->ec_dev, acpi_device_get_parent_softc(sc->ec_dev),
-		    "EcCommand: no response to %#x\n", cmd);
-    }
-
+    } else
+	device_printf(sc->ec_dev, "EcCommand: no response to %#x\n", cmd);
     return (status);
 }
 
@@ -1042,6 +898,7 @@
 {
     ACPI_STATUS	status;
     UINT8 data;
+    u_int gen_count;
 
     ACPI_SERIAL_ASSERT(ec);
     CTR1(KTR_ACPI, "ec read from %#x", Address);
@@ -1060,21 +917,20 @@
     if (ACPI_FAILURE(status))
 	return (status);
 
+    gen_count = sc->ec_gencount;
     EC_SET_DATA(sc, Address);
-    status = EcWaitEvent(sc, EC_EVENT_OUTPUT_BUFFER_FULL);
+    status = EcWaitEvent(sc, EC_EVENT_OUTPUT_BUFFER_FULL, gen_count);
     if (ACPI_FAILURE(status)) {
-	ACPI_VPRINT(sc->ec_dev, acpi_device_get_parent_softc(sc->ec_dev),
-		    "EcRead: Failed waiting for EC to send data.\n");
+	device_printf(sc->ec_dev, "EcRead: failed waiting to get data\n");
 	return (status);
     }
-
     *Data = EC_GET_DATA(sc);
 
     if (sc->ec_burstactive) {
+	sc->ec_burstactive = FALSE;
 	status = EcCommand(sc, EC_COMMAND_BURST_DISABLE);
 	if (ACPI_FAILURE(status))
 	    return (status);
-	sc->ec_burstactive = FALSE;
 	CTR0(KTR_ACPI, "ec disabled burst ok");
     }
 
@@ -1086,6 +942,7 @@
 {
     ACPI_STATUS	status;
     UINT8 data;
+    u_int gen_count;
 
     ACPI_SERIAL_ASSERT(ec);
     CTR2(KTR_ACPI, "ec write to %#x, data %#x", Address, *Data);
@@ -1104,27 +961,27 @@
     if (ACPI_FAILURE(status))
 	return (status);
 
+    gen_count = sc->ec_gencount;
     EC_SET_DATA(sc, Address);
-    status = EcWaitEvent(sc, EC_EVENT_INPUT_BUFFER_EMPTY);
+    status = EcWaitEvent(sc, EC_EVENT_INPUT_BUFFER_EMPTY, gen_count);
     if (ACPI_FAILURE(status)) {
-	ACPI_VPRINT(sc->ec_dev, acpi_device_get_parent_softc(sc->ec_dev),
-		    "EcRead: Failed waiting for EC to process address\n");
+	device_printf(sc->ec_dev, "EcRead: failed waiting for sent address\n");
 	return (status);
     }
 
+    gen_count = sc->ec_gencount;
     EC_SET_DATA(sc, *Data);
-    status = EcWaitEvent(sc, EC_EVENT_INPUT_BUFFER_EMPTY);
+    status = EcWaitEvent(sc, EC_EVENT_INPUT_BUFFER_EMPTY, gen_count);
     if (ACPI_FAILURE(status)) {
-	ACPI_VPRINT(sc->ec_dev, acpi_device_get_parent_softc(sc->ec_dev),
-		    "EcWrite: Failed waiting for EC to process data\n");
+	device_printf(sc->ec_dev, "EcWrite: failed waiting for sent data\n");
 	return (status);
     }
 
     if (sc->ec_burstactive) {
+	sc->ec_burstactive = FALSE;
 	status = EcCommand(sc, EC_COMMAND_BURST_DISABLE);
 	if (ACPI_FAILURE(status))
 	    return (status);
-	sc->ec_burstactive = FALSE;
 	CTR0(KTR_ACPI, "ec disabled burst ok");
     }
 

--------------040800050701000906010408--



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?46E45A6D.1010805>